show tournament start link only 2 hrs before the exam #8134
Conversation
WalkthroughThe tournament tile template now calculates time remaining in two-hour units instead of days, and displays an alert message when tournaments haven't started instead of showing a non-functional link. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/templates/courses/tournament-tile-mixin.pug (1)
32-34: Inconsistency: Display text uses days while link logic uses 2-hour units.The display text calculates time remaining in days (line 33), which creates confusing UX when combined with the 2-hour link visibility logic. For example, with 3 hours remaining, users would see "Starts in 0 days" but no link to join.
Update the display logic to match the 2-hour granularity:
if tournament.state == 'initializing' - now = new Date().getTime() - - time = (new Date(tournament.startDate).getTime() - now)/(24*3600*1000)|0 - span(data-i18n="tournament.estimate_days", data-i18n-options={time}) + - twoHours = 2*60*60*1000 + - time = Math.ceil((new Date(tournament.startDate).getTime() - now)/twoHours) + span(data-i18n="tournament.estimate_two_hour_periods", data-i18n-options={time})Alternatively, if maintaining the day-based display is preferred, consider showing "Less than 1 day" or hour-based text when very close to start time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/templates/courses/tournament-tile-mixin.pug(1 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). (2)
- GitHub Check: ESLint CI
- GitHub Check: Node.js CI (20.19.2)
| - twoHours = 2*60*60*1000 | ||
| - tournamentStart = new Date(tournament.startDate).getTime() | ||
| - timeLeft = (((tournamentStart - now)/twoHours) + 0.99)|0 | ||
| if timeLeft <= 1 |
There was a problem hiding this comment.
The + 0.99 creates an imprecise 2-hour boundary.
The calculation (((tournamentStart - now)/twoHours) + 0.99)|0 results in a boundary at approximately 2.02 hours (2 hours and 72 seconds) instead of exactly 2 hours as intended by the PR objective. This happens because timeLeft <= 1 requires (tournamentStart - now)/twoHours < 1.01.
Apply this diff for a cleaner, exact 2-hour cutoff:
- - twoHours = 2*60*60*1000
- - tournamentStart = new Date(tournament.startDate).getTime()
- - timeLeft = (((tournamentStart - now)/twoHours) + 0.99)|0
- if timeLeft <= 1
+ - twoHours = 2*60*60*1000
+ - tournamentStart = new Date(tournament.startDate).getTime()
+ if (tournamentStart - now) <= twoHours
- showLink = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - twoHours = 2*60*60*1000 | |
| - tournamentStart = new Date(tournament.startDate).getTime() | |
| - timeLeft = (((tournamentStart - now)/twoHours) + 0.99)|0 | |
| if timeLeft <= 1 | |
| - twoHours = 2*60*60*1000 | |
| - tournamentStart = new Date(tournament.startDate).getTime() | |
| if (tournamentStart - now) <= twoHours | |
| - showLink = true |
🤖 Prompt for AI Agents
In app/templates/courses/tournament-tile-mixin.pug around lines 6 to 9, the
current expression adds 0.99 to the hours calculation which shifts the intended
2-hour cutoff to ~2.02 hours; replace the hacky +0.99 truncation with a proper
ceiling: compute timeLeft using Math.ceil((tournamentStart - now) / twoHours)
(or an equivalent integer ceiling) so that timeLeft <= 1 becomes an exact 2-hour
boundary.
| - var showLadderImage = tournament.state != 'initializing' && view.ladderImageMap[tournament.levelOriginal] | ||
|
|
||
| a.ai-league-btn.tile(href=showLink ? `/play/ladder/${tournament.slug}/clan/${tournament.clan}?tournament=${tournament._id}`: 'javascript: void(0)', title=tournament.description) | ||
| a.ai-league-btn.tile(href=showLink ? `/play/ladder/${tournament.slug}/clan/${tournament.clan}?tournament=${tournament._id}`: 'javascript: window.alert("Tournament not started yet")', title=tournament.description) |
There was a problem hiding this comment.
Avoid javascript: protocol and internationalize the alert message.
Using javascript: in href attributes is discouraged for security and accessibility reasons. Additionally, the alert message is hardcoded in English instead of using the i18n system like other strings in this template.
Consider refactoring to handle the click event properly with a data attribute or onclick handler, and use the i18n system:
- a.ai-league-btn.tile(href=showLink ? `/play/ladder/${tournament.slug}/clan/${tournament.clan}?tournament=${tournament._id}`: 'javascript: window.alert("Tournament not started yet")', title=tournament.description)
+ a.ai-league-btn.tile(href=showLink ? `/play/ladder/${tournament.slug}/clan/${tournament.clan}?tournament=${tournament._id}`: '#', onclick=showLink ? '' : 'alert($.i18n.t("tournament.not_started_yet")); return false;', title=tournament.description)And add the translation key to your i18n files.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit