Feat/contest name management #6322
Conversation
WalkthroughAdd contest name support and editing: DB migration with backfill, model callbacks/validation for default names, controller refactor for name/status/deadline updates, new name-edit modal component and Stimulus handlers, view/style updates to display contest names, and corresponding tests/factory updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
app/components/admin/contest_update_name_modal_component.html.erb (1)
1-42: Component structure is well-formed.The modal follows Bootstrap and Rails conventions with proper accessibility attributes, i18n integration, and form structure. The name field is correctly marked as required.
Note: Past review comments from hound[bot] flagged trailing whitespace and missing trailing newline issues. Ensure those are addressed.
app/components/admin/contest_update_deadline_modal_component.html.erb (1)
1-50: Modal component is correctly structured.The deadline-focused modal appropriately:
- Uses semantic IDs and ARIA labels for accessibility
- Sets a minimum deadline of the current time to prevent past dates
- Pre-fills the current deadline in the correct format
Note: Past review flagged a missing trailing newline at line 50.
app/components/admin/contest_host_new_modal_component.html.erb (1)
47-47: Verify the trailing newline is present.A past review comment noted a missing trailing newline. Ensure the file ends with a newline character to comply with POSIX standards and maintain consistency across the codebase.
Run the following script to verify:
#!/bin/bash # Check if the file ends with a newline if [ -n "$(tail -c 1 app/components/admin/contest_host_new_modal_component.html.erb)" ]; then echo "Missing trailing newline" else echo "Trailing newline present" fiapp/views/admin/contests/index.html.erb (1)
16-17: Trailing whitespace detected.Multiple lines contain trailing whitespace. While these are formatting issues already flagged by the linter, please ensure they're cleaned up for code quality.
Also applies to: 33-34, 36-37, 44-45, 52-53, 62-63, 71-71, 77-77, 90-90, 92-93
🧹 Nitpick comments (4)
db/migrate/20251028123342_add_name_to_contests.rb (1)
4-4: Consider adding a length limit to the name column.Adding a length constraint at the database level provides an additional layer of validation and helps prevent potential storage issues.
Apply this diff:
- add_column :contests, :name, :string + add_column :contests, :name, :string, limit: 255app/components/admin/contest_host_new_modal_component.html.erb (1)
30-33: Consider adding a maxlength attribute to the name field.Adding a
maxlengthattribute that matches the database column limit (if one exists) provides immediate user feedback and prevents form submission with overly long values.If the database column has a length limit (e.g., 255 characters), apply this diff:
<%= form.label :name, t("contest.host_new_contest_modal.sub_heading") %> - <%= form.text_field :name, class: "form-control" %> + <%= form.text_field :name, class: "form-control", maxlength: 255 %>spec/requests/contests_update_name_spec.rb (1)
41-50: Consider instance-specific stubbing instead ofallow_any_instance_of.While the test correctly verifies the save failure branch, using
allow_any_instance_ofat Line 43 is generally discouraged in favor of stubbing specific instances. However, for this internal failure simulation, it's acceptable.If you want to improve this in the future:
allow(contest).to receive(:update).and_return(false) patch admin_contest_path(contest), params: { contest: { name: "Test Name That Fails" } }Note: This would require adjusting how the controller loads the contest instance.
spec/models/contest_spec.rb (1)
25-34: Consider testing observable behavior instead of internal state.While the test correctly verifies custom name preservation, Line 32 tests the internal
@default_name_assignedinstance variable. Testing internal state can make tests brittle—if the implementation changes (e.g., using a different flag name or mechanism), the test breaks even if the observable behavior remains correct.The custom name preservation is already verified by Line 30. Consider removing the instance variable assertion unless documenting this internal contract is intentionally part of the test strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/assets/stylesheets/contests.scss(1 hunks)app/components/admin/contest_host_new_modal_component.html.erb(1 hunks)app/components/admin/contest_update_deadline_modal_component.html.erb(2 hunks)app/components/admin/contest_update_name_modal_component.html.erb(1 hunks)app/components/admin/contest_update_name_modal_component.rb(1 hunks)app/components/contest/card_component.html.erb(1 hunks)app/controllers/admin/contests_controller.rb(4 hunks)app/javascript/controllers/contest_controller.js(2 hunks)app/models/contest.rb(1 hunks)app/views/admin/contests/index.html.erb(3 hunks)app/views/contests/leaderboard.html.erb(1 hunks)app/views/contests/show.html.erb(1 hunks)config/locales/views/contests/en.yml(4 hunks)db/migrate/20251028123342_add_name_to_contests.rb(1 hunks)db/schema.rb(2 hunks)spec/components/admin/contest_host_new_modal_component_spec.rb(1 hunks)spec/components/admin/contest_update_deadline_modal_component_spec.rb(1 hunks)spec/components/admin/contest_update_name_modal_component_spec.rb(1 hunks)spec/components/contest/card_component_spec.rb(1 hunks)spec/factories/contest.rb(1 hunks)spec/models/contest_spec.rb(1 hunks)spec/requests/contests_create_spec.rb(1 hunks)spec/requests/contests_update_deadline_spec.rb(1 hunks)spec/requests/contests_update_name_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-08T16:36:20.199Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5943
File: spec/rails_helper.rb:49-53
Timestamp: 2025-08-08T16:36:20.199Z
Learning: CircuitVerse: In spec/rails_helper.rb, keep Flipper :contests enabled by default before each example to avoid mass tagging (contests: true) and potential order-dependent failures. Flipper state is reset per example, and specs that assert "feature off" explicitly disable the flag. Prefer incremental migration to opt-in later rather than changing the default in the same PR.
Applied to files:
spec/components/contest/card_component_spec.rbspec/factories/contest.rbspec/requests/contests_update_name_spec.rbspec/requests/contests_update_deadline_spec.rbapp/models/contest.rbspec/components/admin/contest_update_name_modal_component_spec.rbapp/controllers/admin/contests_controller.rbspec/requests/contests_create_spec.rbspec/models/contest_spec.rbspec/components/admin/contest_host_new_modal_component_spec.rbapp/views/admin/contests/index.html.erbdb/migrate/20251028123342_add_name_to_contests.rb
📚 Learning: 2025-08-09T04:52:11.948Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5957
File: spec/requests/contests_submissions_create_guards_spec.rb:36-39
Timestamp: 2025-08-09T04:52:11.948Z
Learning: In CircuitVerse specs, prefer using `I18n.t` over `I18n.t!` in behavior tests to keep them focused on controller behavior rather than translation validation. Translation completeness should be enforced globally (e.g., through test environment configuration or dedicated translation specs) rather than sprinkling `t!` in individual specs.
Applied to files:
spec/components/contest/card_component_spec.rbspec/components/admin/contest_update_name_modal_component_spec.rbspec/models/contest_spec.rb
📚 Learning: 2024-10-26T19:38:55.468Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5117
File: app/views/users/shared/_links.html.erb_spec.rb:0-0
Timestamp: 2024-10-26T19:38:55.468Z
Learning: In `app/views/users/shared/_links.html.erb_spec.rb`, before reorganizing test setups or making significant changes, it's preferred to consult with the maintainer to prevent breaking existing functionality.
Applied to files:
spec/requests/contests_update_name_spec.rbspec/requests/contests_update_deadline_spec.rbspec/requests/contests_create_spec.rb
📚 Learning: 2025-06-13T10:51:06.810Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5799
File: spec/controllers/contest_controller_spec.rb:129-129
Timestamp: 2025-06-13T10:51:06.810Z
Learning: The RSpec helper method `sign_in_random_user` is defined in `spec/support/spec_utils.rb`, so occurrences of this method in specs do not need changes.
Applied to files:
spec/requests/contests_create_spec.rb
📚 Learning: 2024-11-02T08:12:55.712Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/projects/_add_collaborator_modal.html.erb:24-27
Timestamp: 2024-11-02T08:12:55.712Z
Learning: In `app/views/projects/_add_collaborator_modal.html.erb`, adding `aria-describedby` and `required` attributes to the email select input in the add collaborator modal causes emails to become invalid.
Applied to files:
app/components/admin/contest_host_new_modal_component.html.erbapp/components/admin/contest_update_deadline_modal_component.html.erb
📚 Learning: 2024-11-01T14:59:56.969Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/assets/stylesheets/select2_overrides.scss:7-7
Timestamp: 2024-11-01T14:59:56.969Z
Learning: Adjusting responsive breakpoints in `app/assets/stylesheets/select2_overrides.scss` for `.select2-container` and `.select2-selection--multiple` interferes with functionality. Avoid suggesting these changes in future reviews.
Applied to files:
app/assets/stylesheets/contests.scss
📚 Learning: 2024-11-02T06:42:31.664Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/users/registrations/new.html.erb:22-22
Timestamp: 2024-11-02T06:42:31.664Z
Learning: In the project, when considering replacing `form-group` with `mb-3` in form files (e.g., `app/views/projects/_form.html.erb`, `app/views/assignments/_form.html.erb`), be aware that the current spacing and alignment classes are intentionally designed for specific needs across different pages. Changing these classes could lead to unintended layout shifts or visual inconsistencies. To maintain the original design, we should keep the existing classes unless there's a specific reason to change them.
Applied to files:
app/assets/stylesheets/contests.scss
📚 Learning: 2024-11-01T15:03:20.706Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/users/passwords/edit.html.erb:39-44
Timestamp: 2024-11-01T15:03:20.706Z
Learning: In the project, avoid updating button classes to Bootstrap 5 conventions if it changes how buttons are styled, as this change has not been made intentionally to preserve styling consistency.
Applied to files:
app/assets/stylesheets/contests.scss
📚 Learning: 2024-11-02T05:24:32.670Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/groups/new.html.erb:18-21
Timestamp: 2024-11-02T05:24:32.670Z
Learning: Changes to layout patterns in `app/views/groups/new.html.erb`, such as column widths, are part of the site's original design and should be approved by the UI/UX team before making adjustments, to maintain the intended visual consistency across the site.
Applied to files:
app/views/admin/contests/index.html.erb
📚 Learning: 2024-11-02T04:39:18.387Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/groups/new.html.erb:5-5
Timestamp: 2024-11-02T04:39:18.387Z
Learning: In `app/views/groups/new.html.erb`, the multiple `col` classes are intentionally kept as is to avoid breaking the UI. They should not be simplified.
Applied to files:
app/views/admin/contests/index.html.erb
🧬 Code graph analysis (8)
spec/components/contest/card_component_spec.rb (1)
app/controllers/admin/contests_controller.rb (1)
create(12-29)
spec/requests/contests_update_name_spec.rb (2)
app/controllers/admin/contests_controller.rb (1)
create(12-29)spec/support/flipper_helpers.rb (1)
enable_contests!(12-14)
spec/requests/contests_update_deadline_spec.rb (2)
app/controllers/admin/contests_controller.rb (1)
create(12-29)spec/support/flipper_helpers.rb (1)
enable_contests!(12-14)
app/models/contest.rb (1)
app/models/assignment.rb (1)
validates(3-89)
app/components/admin/contest_update_name_modal_component.rb (1)
app/components/admin/contest_update_deadline_modal_component.rb (1)
initialize(3-10)
app/controllers/admin/contests_controller.rb (1)
app/services/contest_scheduler.rb (2)
call(3-11)call(4-10)
spec/requests/contests_create_spec.rb (1)
spec/support/flipper_helpers.rb (1)
enable_contests!(12-14)
spec/models/contest_spec.rb (2)
app/services/notify_user.rb (1)
new(3-78)app/controllers/admin/contests_controller.rb (1)
create(12-29)
🪛 ESLint
app/javascript/controllers/contest_controller.js
[error] 28-28: 'action' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 42-42: 'action' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 80-80: Newline required at end of file but not found.
(eol-last)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (34)
spec/components/admin/contest_update_deadline_modal_component_spec.rb (1)
11-11: LGTM!The modal ID update correctly reflects the component rename to focus specifically on deadline updates.
app/javascript/controllers/contest_controller.js (1)
17-46: Well-structured modal separation.The refactor correctly splits the single modal into dedicated handlers for name and deadline updates. Each handler appropriately updates the form action and pre-fills the relevant input field.
spec/requests/contests_update_deadline_spec.rb (1)
5-55: Excellent test coverage for deadline updates.The spec comprehensively covers edge cases:
- Invalid date format handling
- Past date rejection
- Successful update flow
- Save failure branch
The use of
be_within(1.minute).of(...)for deadline comparisons appropriately handles time precision and async behavior.app/controllers/admin/contests_controller.rb (3)
31-43: Clean separation of update concerns.The control flow correctly routes different update types to dedicated handlers, improving maintainability and clarity.
55-73: LGTM: Proper parameter scoping.The dedicated param methods correctly use Rails 8's
params.expectsyntax and provide appropriate scoping for each update type, preventing unintended modifications.
106-119: Verify i18n key matches the validation logic.Line 110 rejects deadlines that are not in the future (
<= Time.zone.now), using the i18n key.deadline_in_future. Ensure the translation text correctly conveys "Deadline must be in the future" rather than a present-tense statement.app/assets/stylesheets/contests.scss (2)
180-201: Update button styles are well-structured.The new styles appropriately handle the update buttons layout using flexbox, with proper spacing and color theming. The image filter for the deadline button icon is a pragmatic approach to color inversion.
22-243: Comprehensive styling for contest management UI.The extensive style additions provide a cohesive visual system for the contest management features, including status indicators, action buttons, and card layouts. Good use of SCSS variables maintains theming consistency.
db/schema.rb (1)
13-13: LGTM: Schema updated for contest name support.The schema correctly reflects the addition of the
namecolumn to the contests table, supporting the core feature of this PR.Also applies to: 176-176
app/views/contests/show.html.erb (1)
11-11: LGTM!The change to display the contest name directly is clean and aligns with the PR objectives to show meaningful contest names instead of generic IDs.
app/views/contests/leaderboard.html.erb (1)
4-6: LGTM!Consistent with the contest name display changes across the application. The implementation is straightforward and correct.
app/components/contest/card_component.html.erb (1)
21-21: LGTM!The card component now displays the contest name consistently with other views in the application.
spec/factories/contest.rb (1)
5-5: LGTM!Adding a sequence for the name attribute is a good practice that ensures unique contest names in tests and aligns with the new name field requirement.
spec/components/contest/card_component_spec.rb (2)
6-15: LGTM!The test correctly validates that the component displays the contest name. Using a specific name ("Final Circuit Challenge") in the fixture makes the test expectations clear and explicit.
17-25: LGTM!Good use of the factory-generated name via
contest.namein the expectation, which ensures the test adapts if the factory sequence changes.spec/components/admin/contest_update_name_modal_component_spec.rb (1)
23-31: Perfect! The evidence is conclusive. The contest controller'ssetAdminModals()method explicitly handles the:contest_idplaceholder replacement:let action = form.attr('action').replace(':contest_id', contestId); form.attr('action', action);This occurs when the Bootstrap modal fires its
show.bs.modalevent, replacing the placeholder with the actual contest ID from$(e.relatedTarget).data('contest').id. The test correctly verifies this intended behavior.
No action required—the
:contest_idplaceholder behavior is correct.The component intentionally uses a literal
":contest_id"placeholder in the form action. Client-side JavaScript in the Stimulus controller replaces it with the actual contest ID when the modal is shown, before form submission. The test accurately verifies this design pattern, which is consistently applied across multiple modal components (name and deadline). No issues exist.app/components/admin/contest_update_name_modal_component.rb (1)
1-10: LGTM! Clean ViewComponent implementation.The component follows ViewComponent conventions and matches the existing pattern used in
Admin::ContestUpdateDeadlineModalComponent. The structure is straightforward and appropriate for a modal component.app/models/contest.rb (2)
12-22: LGTM! Well-structured validation and callback setup.The name presence validation and callback orchestration are appropriate. The
before_validationcallback ensures a name is set before validation runs, andafter_commithandles normalization after the record has an ID.
35-46: LGTM! Thoughtful normalization implementation.The normalization logic is well-designed:
- Line 36 guards against missing IDs
- Lines 38-41 optimize by avoiding redundant updates when the name already matches
- Line 43 appropriately uses
update_columnto skip validations (preventing infinite callback loops) and is properly documented with rubocop directivesspec/requests/contests_create_spec.rb (3)
8-13: LGTM! Proper test setup with dependency isolation.The authentication helper change to
login_as(admin, scope: :user)is more explicit, and stubbingContestScheduler.callproperly isolates the test from side effects.
15-29: LGTM! Comprehensive test for auto-generated names.The test correctly verifies that contests created without a custom name receive an auto-generated name starting with "Contest #", and confirms the expected redirect and flash message.
31-49: LGTM! Thorough test for custom name handling.The test properly verifies that custom names are preserved during contest creation and checks all expected side effects (redirect, flash message, database state).
spec/requests/contests_update_name_spec.rb (4)
8-14: LGTM! Appropriate test data setup.Using
update_columnto set the initial name bypasses callbacks and validations, which is appropriate for test setup to establish a known starting state.
18-28: LGTM! Comprehensive happy path test.The test properly verifies the successful name update flow, including redirect, flash message, and database persistence.
30-39: LGTM! Proper validation failure test.The test correctly verifies that blank names are rejected with appropriate error messages and that the original name remains unchanged.
52-60: LGTM! Proper authorization guard test.The test correctly verifies that unauthenticated users are redirected to sign-in and cannot modify contest names.
spec/models/contest_spec.rb (1)
12-23: LGTM! Thorough test for auto-generated names.The tests properly verify both the placeholder name assignment (before validation) and the final normalization (after commit), covering the complete default naming lifecycle.
config/locales/views/contests/en.yml (2)
65-78: LGTM! Well-structured locale additions.The new localization keys for name input and update modals are properly nested and follow existing patterns. The renaming of
update_contest_modaltoupdate_contest_deadline_modalmaintains clarity now that there are separate name and deadline modals.
124-139: LGTM! Complete admin locale coverage.The new admin keys provide comprehensive coverage for name editing functionality, including success messages, error messages, and alt text for accessibility.
spec/components/admin/contest_host_new_modal_component_spec.rb (2)
6-17: LGTM! Improved test structure.Using
let(:default_deadline)improves consistency, and splitting the test into focused scenarios (visible elements vs. hidden values) enhances readability. The visible name input is properly verified.
19-31: LGTM! Thorough hidden values test.The test correctly verifies that the default status and deadline are present as hidden inputs with properly formatted values.
app/views/admin/contests/index.html.erb (3)
59-59: LGTM! Improved contest identification.Displaying
contest.nameinstead of a generic "Contest ID" significantly improves the user experience by showing meaningful, human-readable contest names.
70-83: LGTM! Well-structured update button group.The separate update buttons for name and deadline provide focused, single-responsibility actions. Each button properly targets its respective modal with appropriate data attributes and accessible alt text.
89-92: LGTM! Proper modal component rendering.The three modal components (Close, Update Name, Update Deadline) are correctly rendered for each contest, providing scoped modal instances.
| def set_default_name | ||
| return if name.present? | ||
|
|
||
| @default_name_assigned = true | ||
| next_number = id || (Contest.unscoped.maximum(:id).to_i + 1) | ||
|
|
||
| self.name = "Contest ##{next_number}" | ||
| end |
There was a problem hiding this comment.
Minor race condition in concurrent contest creation.
When multiple contests are created simultaneously without names, Line 30's Contest.unscoped.maximum(:id).to_i + 1 could return identical values, resulting in temporary duplicate placeholder names before normalization. While the after_commit callback corrects this, there's a brief window where two contests could have the same placeholder name.
For a low-traffic admin feature, this is acceptable. However, if contest creation becomes high-volume, consider using database sequences or row-level locking.
🤖 Prompt for AI Agents
In app/models/contest.rb around lines 26 to 33, the placeholder name generation
uses Contest.unscoped.maximum(:id).to_i + 1 which can race under concurrent
creations and produce duplicate temporary names; fix by deriving the next number
from a database-side sequence or by acquiring a row-level lock when computing
the max id. Concretely: either add and use a dedicated DB sequence (call nextval
on it when assigning the default name) or wrap the maximum(:id) lookup and
assignment in a transaction that locks the contests table/rows (SELECT MAX(id)
... FOR UPDATE or lock the table) so concurrent processes cannot read the same
max id; update set_default_name to use that sequence or locked transaction to
compute next_number.
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| class="btn-close ms-auto" | ||
| data-bs-dismiss="modal" | ||
| aria-label="<%= t('contest.close') %>"></button> | ||
| </div> |
There was a problem hiding this comment.
Extra whitespace detected at end of line.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/views/admin/contests/index.html.erb(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: test
🔇 Additional comments (3)
app/views/admin/contests/index.html.erb (3)
53-53: LGTM! Contest name display correctly implemented.The contest name is now displayed instead of a generated ID label, which aligns with the PR objectives. The link remains functional with
contest_path(contest.id), and Rails auto-escapes the output for XSS protection.
79-79: LGTM! New modal component properly integrated.The
Admin::ContestUpdateNameModalComponentis correctly rendered for each contest, following the established pattern of other modal components in the view. The placement is logical and supports the new name editing functionality.
68-68: No changes needed — deadline is guaranteed non-nil.The verification confirms that
contest.deadlinecannot be nil:
- The Contest model enforces
validates :deadline, presence: true- The database schema enforces
null: falseon the deadline columnSince deadline presence is guaranteed at both the model and database layers, calling
.strftime()on line 68 is safe and does not require an explicit nil check. The code is correct as written.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/javascript/controllers/contest_controller.js (1)
80-80: Add trailing newline at end of file.ESLint requires a trailing newline at the end of the file.
Apply this diff:
} } +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/admin/contest_host_new_modal_component.html.erb(1 hunks)app/javascript/controllers/contest_controller.js(1 hunks)app/views/admin/contests/index.html.erb(3 hunks)db/migrate/20251028123342_add_name_to_contests.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- db/migrate/20251028123342_add_name_to_contests.rb
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5943
File: spec/rails_helper.rb:49-53
Timestamp: 2025-08-08T16:36:20.199Z
Learning: CircuitVerse: In spec/rails_helper.rb, keep Flipper :contests enabled by default before each example to avoid mass tagging (contests: true) and potential order-dependent failures. Flipper state is reset per example, and specs that assert "feature off" explicitly disable the flag. Prefer incremental migration to opt-in later rather than changing the default in the same PR.
📚 Learning: 2024-11-02T08:12:55.712Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/projects/_add_collaborator_modal.html.erb:24-27
Timestamp: 2024-11-02T08:12:55.712Z
Learning: In `app/views/projects/_add_collaborator_modal.html.erb`, adding `aria-describedby` and `required` attributes to the email select input in the add collaborator modal causes emails to become invalid.
Applied to files:
app/views/admin/contests/index.html.erbapp/components/admin/contest_host_new_modal_component.html.erb
📚 Learning: 2025-08-08T16:36:20.199Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5943
File: spec/rails_helper.rb:49-53
Timestamp: 2025-08-08T16:36:20.199Z
Learning: CircuitVerse: In spec/rails_helper.rb, keep Flipper :contests enabled by default before each example to avoid mass tagging (contests: true) and potential order-dependent failures. Flipper state is reset per example, and specs that assert "feature off" explicitly disable the flag. Prefer incremental migration to opt-in later rather than changing the default in the same PR.
Applied to files:
app/views/admin/contests/index.html.erbapp/components/admin/contest_host_new_modal_component.html.erb
📚 Learning: 2024-11-02T06:44:33.057Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/users/sessions/new.html.erb:25-28
Timestamp: 2024-11-02T06:44:33.057Z
Learning: In `app/views/users/sessions/new.html.erb`, for the password toggle functionality, prefer to use inline `onclick` attributes like `onclick="togglePassword()"` to keep the implementation straightforward and ensure compatibility across different browsers and devices. Avoid suggesting refactoring to unobtrusive JavaScript unless there is a significant benefit.
Applied to files:
app/views/admin/contests/index.html.erb
📚 Learning: 2025-04-18T11:40:37.078Z
Learnt from: mahmoodalisha
Repo: CircuitVerse/CircuitVerse PR: 5646
File: app/component/delete_modal_component.html.erb:12-14
Timestamp: 2025-04-18T11:40:37.078Z
Learning: CircuitVerse uses the traditional Rails UJS `method: :delete` approach for delete links rather than Turbo's `data: { turbo_method: :delete }`. For consistency with the rest of the codebase, delete links should continue using `method: :delete`.
Applied to files:
app/components/admin/contest_host_new_modal_component.html.erb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
app/components/admin/contest_host_new_modal_component.html.erb (1)
19-43: LGTM! The deprecatedlocal: trueparameter has been correctly replaced.The form structure is correct, wrapping both modal body and footer sections. The new contest name input field is properly integrated using standard Rails form helpers.
app/views/admin/contests/index.html.erb (1)
63-71: LGTM! The data attribute issue has been correctly fixed.Lines 63 and 68 now pass
contest_id: contest.idas scalar values instead of passing the entire Ruby object. This ensures JavaScript can properly access the contest ID.
| <%= link_to "#", | ||
| data: { bs_toggle: "modal", | ||
| bs_target: "#close-contest-confirmation-modal", | ||
| bs_target: "#close-contest-confirmation-modal", | ||
| contest: contest }, | ||
| class: "contest-container-close-button" do %> | ||
| <span><%= t("contest.close") %></span> | ||
| <span><%= t("contest.close") %></span> | ||
| <% end %> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the close modal's data attribute usage.
Line 32 passes contest: contest as a data attribute, which will stringify the Ruby object. For consistency with the update modals (lines 63 and 68), this should pass contest_id: contest.id instead.
Check if the close modal's JavaScript handler expects a contest ID:
🏁 Script executed:
#!/bin/bash
# Search for the close-contest-confirmation-modal handler
rg -n -A 10 "close-contest-confirmation-modal" app/javascript/ app/components/Length of output: 1159
I need to find how the modal component uses the passed data and check the JavaScript handler. Let me search for more context:
Let me search for the modal component's implementation and JavaScript handlers:
Now I need to search the codebase directly to verify the issue. Let me check the actual implementation:
🏁 Script executed:
#!/bin/bash
# Find the contest index view and check lines around 29-35 and 63-68
fd -t f "contests.*index" app/views/admin/ -exec cat {} \;Length of output: 250
Let me try a simpler approach:
🏁 Script executed:
#!/bin/bash
# Find and display the contests index view
find . -name "index.html.erb" -path "*/admin/contests/*" 2>/dev/null | head -1 | xargs catLength of output: 4405
🏁 Script executed:
#!/bin/bash
# Also search for the modal component implementation
find . -name "contest_close_modal_component.html.erb" 2>/dev/null | xargs catLength of output: 1226
🏁 Script executed:
#!/bin/bash
# Search for JavaScript handlers that process modal data
rg -n "show.bs.modal|close.*modal" --type=js --type=ts app/javascript/ 2>/dev/null | head -20Length of output: 732
Let me check the modal component's Ruby class and look for any close modal handler:
🏁 Script executed:
#!/bin/bash
# Find the contest close modal component Ruby class
find . -name "contest_close_modal_component.rb" 2>/dev/null | xargs catLength of output: 164
🏁 Script executed:
#!/bin/bash
# Check the full contest_controller.js for any close modal handler
cat app/javascript/controllers/contest_controller.jsLength of output: 3439
Remove the unnecessary contest data attribute—the close modal doesn't use it.
The close modal is rendered as a ViewComponent and uses the @contest variable directly in its form; it has no JavaScript handler that would access the data attribute. The inconsistency with update modals (which do pass contest_id for their JavaScript handlers) is valid, but in this case, the data attribute should be removed entirely rather than changed, since the close modal doesn't need it.
Lines 32-35 should be:
<%= link_to "#",
data: { bs_toggle: "modal",
bs_target: "#close-contest-confirmation-modal" },
class: "contest-container-close-button" do %>
<span><%= t("contest.close") %></span>
<% end %>The update modals at lines 63 and 68 correctly pass contest_id because their show.bs.modal handlers extract it via JavaScript to construct form actions. The close modal doesn't require this pattern.
🤖 Prompt for AI Agents
In app/views/admin/contests/index.html.erb around lines 29 to 35, remove the
unnecessary data attribute `contest:` from the link that opens the
close-contest-confirmation-modal because the modal ViewComponent uses @contest
directly and no JS reads that data; update the data hash to only include
bs_toggle and bs_target so the link becomes data: { bs_toggle: "modal",
bs_target: "#close-contest-confirmation-modal" } while keeping the same class
and inner content.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/controllers/contest_controller.js(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5943
File: spec/rails_helper.rb:49-53
Timestamp: 2025-08-08T16:36:20.199Z
Learning: CircuitVerse: In spec/rails_helper.rb, keep Flipper :contests enabled by default before each example to avoid mass tagging (contests: true) and potential order-dependent failures. Flipper state is reset per example, and specs that assert "feature off" explicitly disable the flag. Prefer incremental migration to opt-in later rather than changing the default in the same PR.
📚 Learning: 2024-11-02T08:12:55.712Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/projects/_add_collaborator_modal.html.erb:24-27
Timestamp: 2024-11-02T08:12:55.712Z
Learning: In `app/views/projects/_add_collaborator_modal.html.erb`, adding `aria-describedby` and `required` attributes to the email select input in the add collaborator modal causes emails to become invalid.
Applied to files:
app/javascript/controllers/contest_controller.js
📚 Learning: 2024-11-02T06:44:33.057Z
Learnt from: salmoneatenbybear
Repo: CircuitVerse/CircuitVerse PR: 5135
File: app/views/users/sessions/new.html.erb:25-28
Timestamp: 2024-11-02T06:44:33.057Z
Learning: In `app/views/users/sessions/new.html.erb`, for the password toggle functionality, prefer to use inline `onclick` attributes like `onclick="togglePassword()"` to keep the implementation straightforward and ensure compatibility across different browsers and devices. Avoid suggesting refactoring to unobtrusive JavaScript unless there is a significant benefit.
Applied to files:
app/javascript/controllers/contest_controller.js
🪛 ESLint
app/javascript/controllers/contest_controller.js
[error] 81-82: Too many blank lines at the end of file. Max of 0 allowed.
(no-multiple-empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
app/javascript/controllers/contest_controller.js (1)
16-47: LGTM! Clean refactor with all previous issues addressed.The separation of the update modal into distinct name and deadline handlers is well-structured. All previously flagged issues have been correctly fixed:
- Data attributes now correctly use
data('contestId')- Variables properly use
const- Form and input targeting is explicit and correct
|
Hello @tachyons , @VaibhavUpreti , @vedant-jain03 the Pull Request is now ready for final review. |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
1 participant
Fixes #6010
Describe the changes you have made in this PR -
Primary Goal: Implements full functionality for naming contests across the application.
Creation Logic (Clean Code Fix):
-Added a visible Contest Name (Optional) text field to the "Host new contest" modal.
-Implemented robust logic in the Admin::ContestsController#create method to ensure the contest always has a name, even if the field is left blank.
-The Model is the Source of Truth: Moved the default name assignment logic to the Contest Model's before_validation callback, ensuring clean data integrity.
Editing Functionality:
-Added a Contest Name input field to the Update Contest Modal.
-Updated the Admin::ContestsController#update method to safely accept and save the new name alongside the validated deadline.
-Updated the Contest List and Show pages to display the actual contest name instead of the generic "Contest #ID".
Screenshots of the UI changes (If any) -
PR.video.circuitverse.mp4
Checklist before requesting a review
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
New Features
Improvements
Chores
Tests