Refactor contest navigation to use navigable contests and add slugPrefix getter. #2363
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bundle ReportChanges will increase total bundle size by 110 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/models/contest.ts (1)
39-41: Add unit tests for slugPrefix.Since this property is used for navigation filtering, please add unit tests to verify:
- Basic case: "contest-123" → "contest"
- Edge cases mentioned above
- Integration with ContestPageNavigationComponent
Would you like me to help generate the unit test cases?
app/components/contest-page/navigation.ts (2)
30-32: Consider memoizing the navigableContests resultThe implementation is clean and logical. However, since this getter might be called frequently and performs array filtering, consider memoizing the result to optimize performance, especially if allContests or the current contest changes infrequently.
+ @tracked private _navigableContests: ContestModel[] | null = null; get navigableContests(): ContestModel[] { + if (!this._navigableContests) { + this._navigableContests = this.args.allContests.filter( + (contest) => contest.slugPrefix === this.args.contest.slugPrefix + ); + } + return this._navigableContests; - return this.args.allContests.filter((contest) => contest.slugPrefix === this.args.contest.slugPrefix); } + // Add this method to reset cache when dependencies change + private resetNavigableContestsCache() { + this._navigableContests = null; + }
40-40: LGTM! Consider simplifying the previousContest getterThe update to use sortedNavigableContests is correct. However, the logic could be simplified to match the style of nextContest.
get previousContest(): ContestModel | null { - if (this.currentContestIndex > 0) { - return this.sortedNavigableContests[this.currentContestIndex - 1] || null; - } else { - return null; - } + return this.sortedNavigableContests[this.currentContestIndex - 1] || null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/components/contest-page/navigation.ts (2 hunks)
- app/models/contest.ts (1 hunks)
🔇 Additional comments (4)
app/models/contest.ts (1)
39-41: Verify handling of edge cases in slugPrefix.The implementation looks correct for basic cases, but please verify the behavior with these edge cases:
- Slugs with multiple number segments (e.g., "contest-1-2")
- Slugs ending with just a hyphen
- Slugs without any numbers
Let's check existing slug patterns in the codebase:
✅ Verification successful
Based on the search results, I can now verify the slugPrefix implementation. The codebase shows:
- The slugPrefix property is used by ContestPageNavigationComponent to filter contests with the same prefix
- The test files show contest slugs follow a pattern of base name with numeric suffix (e.g. 'weekly-1', 'weekly-2', etc.)
- No edge cases were found where slugs have multiple number segments or end with just a hyphen
- The regex pattern
-\d+$correctly handles all the contest slug patterns found in the codebaseThe slugPrefix implementation is correct and handles all existing slug patterns appropriately.
The regex pattern
-\d+$is well-suited for the contest slugs in use:
- Matches trailing hyphen followed by digits at the end of string
- Won't match mid-string numbers or hyphen-only endings
- Returns the base slug prefix as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all contest slug patterns to verify edge cases # Expected: List of unique slug patterns to validate the regex # Search for slug assignments or definitions in the codebase rg -g '*.ts' -g '*.js' 'slug.*=.*["\'].*-\d' -A 1Length of output: 93
Script:
#!/bin/bash # Let's try a different approach to find slug patterns # Search for slug-related code and patterns in the codebase # Look for slug assignments or usage rg -g '*.{ts,js}' 'slug.*=.*["\''].*-' -A 1 # Also search for test files that might contain slug examples rg -g '*test*.{ts,js}' -g '*spec*.{ts,js}' 'slug.*=.*["\''].*-' -A 1 # Search for any slug property definitions ast-grep --pattern 'slug: string'Length of output: 322
Script:
#!/bin/bash # Fix the regex syntax and try again with simpler patterns # Look for slug patterns in the codebase rg -g '*.{ts,js}' 'slug.*=' -A 1 # Look for test cases involving slugs rg -g '*test*.{ts,js}' -g '*spec*.{ts,js}' 'slug' -A 1 # Look for slug-related function implementations ast-grep --pattern 'slug($$$) { $$$ }'Length of output: 111142
app/components/contest-page/navigation.ts (3)
19-19: LGTM! Consistent rename to sortedNavigableContestsThe update maintains functionality while improving clarity by being more specific about the nature of the sorted contests being indexed.
46-47: LGTM! Clean implementation of sortedNavigableContestsThe renamed getter maintains the same sorting logic while working with the filtered navigable contests. The implementation is clean and consistent with the overall refactoring.
35-35: Verify removal of date-based checks for next contestThe logic has been simplified to just return the next contest in the sorted array. Please confirm if removing the previous date-based checks (current date and one-week threshold) was intentional, as this changes the behavior of what contests are considered "next".
✅ Verification successful
Removal of date-based checks was intentional and correct
The change was part of a larger refactor that improved contest navigation. The previous date-based check (one week threshold) is no longer needed because:
- The navigation now only shows contests with the same
slugPrefix(filtered innavigableContestsgetter)- Contests are still properly sorted by
startsAtdate- The removed date check was an arbitrary restriction that limited navigation between related contests
This simplification makes the code more maintainable while preserving the core functionality of navigating between related contests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any tests or documentation that might reference the date-based navigation behavior rg -l "nextContest|contest.*navigation.*date" --type=ts --type=testLength of output: 98
Script:
#!/bin/bash # Search for any tests or related files that might reference the date-based navigation behavior rg -l "nextContest|contest.*navigation.*date" --type=ts # Look for the previous implementation to understand what was changed rg -B5 -A5 "nextContest" --type=ts # Look for any test files specifically fd "test|spec" -e tsLength of output: 3048
Script:
#!/bin/bash # Let's look at the git history of this file to see the previous implementation git log -p app/components/contest-page/navigation.ts # Also check the ContestModel to understand what properties are available cat app/models/contest.tsLength of output: 7246
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/acceptance/contests-test.js (2 hunks)
🔇 Additional comments (1)
tests/acceptance/contests-test.js (1)
165-177: LGTM! Comprehensive test coverage for header navigation.The new assertions thoroughly verify the "Next" button functionality, including:
- Navigation through multiple contests
- Proper URL updates
- Boundary condition when reaching the last contest
| assert.strictEqual(currentURL(), '/contests/weekly-4', 'Next button works'); | ||
|
|
||
| await contestsPage.prizeDetailsNavigation.clickOnNextContestButton(); | ||
| assert.strictEqual(currentURL(), '/contests/weekly-5', 'Next button works'); | ||
|
|
||
| assert.strictEqual(currentURL(), '/contests/weekly-3', 'Next button is disabled when the next contest is the second contest from present'); | ||
| await contestsPage.prizeDetailsNavigation.clickOnNextContestButton(); | ||
| assert.strictEqual(currentURL(), '/contests/weekly-5', 'Next button is disabled when contest is last'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce test duplication.
While the assertions are thorough, there's significant duplication between the header and prize details navigation tests. Consider extracting common navigation test logic into a shared helper function.
Example refactor:
function assertNavigationBehavior(assert, navigationComponent, description) {
await navigationComponent.clickOnNextContestButton();
assert.strictEqual(currentURL(), '/contests/weekly-4', `${description} Next button works`);
await navigationComponent.clickOnNextContestButton();
assert.strictEqual(currentURL(), '/contests/weekly-5', `${description} Next button works`);
await navigationComponent.clickOnNextContestButton();
assert.strictEqual(currentURL(), '/contests/weekly-5', `${description} Next button is disabled when contest is last`);
}
// Usage in tests:
test('header navigation buttons work', async function(assert) {
// ... existing setup ...
await assertNavigationBehavior(assert, contestsPage.headerNavigation, 'Header navigation:');
});
test('prize details navigation buttons work', async function(assert) {
// ... existing setup ...
await assertNavigationBehavior(assert, contestsPage.prizeDetailsNavigation, 'Prize details navigation:');
});
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/components/contest-page/navigation.hbs (2)
7-12: Consider accessibility implications of abbreviated text.While "Prev" saves space, consider using
aria-label="Previous"to ensure screen readers announce the full word for better accessibility.<LinkTo @route="contest" @model={{this.previousContest.slug}} @disabled={{this.isPreviousContestDisabled}} + aria-label="Previous" class="flex items-center rounded-l-md border border-r-[0.5px] border-white/15 hover:border-white/20 hover:bg-white/5 pl-1.5 pr-2 py-1.5 {{if this.isPreviousContestDisabled 'pointer-events-none opacity-50'}}" data-test-previous-contest-button >
21-22: Consider standardizing border width values.The use of
border-[0.5px]might cause inconsistent rendering across browsers. Consider using standard Tailwind border utilities or a full pixel value.- class="flex items-center rounded-r-md border border-l-[0.5px] border-white/15 hover:border-white/20 hover:bg-white/5 pl-2 pr-1.5 py-1.5 + class="flex items-center rounded-r-md border border-l border-white/15 hover:border-white/20 hover:bg-white/5 pl-2 pr-1.5 py-1.5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/components/contest-page/navigation.hbs (2 hunks)
🔇 Additional comments (1)
app/components/contest-page/navigation.hbs (1)
Line range hint
1-29: Overall implementation looks good!The navigation component is well-structured with proper:
- Semantic HTML
- Test attributes
- Disabled state handling
- Consistent styling
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests