Yuqiang/anonymouse in tournament #8170
Conversation
WalkthroughAdds an encrypted mixed-ID tournament flow: new route Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Router as CocoRouter
participant Utils as tournamentMixedIdHelper
participant LadderView
participant TournamentLB as TournamentLeaderboard
User->>Browser: Click converted tournament ladder link
Browser->>Router: GET /play/tournament/:levelID/:mixedID
Router->>LadderView: instantiate(levelID, mixedID)
LadderView->>Utils: decrypt(mixedID)
Utils-->>LadderView: { clanId, tournamentId }
LadderView->>TournamentLB: new TournamentLeaderboard({ league: clanId, tournament: tournamentId, tournamentState })
TournamentLB->>Browser: fetch leaderboard API (tournament or level)
Browser-->>TournamentLB: leaderboard response
TournamentLB-->>LadderView: render data
LadderView-->>Browser: render ladder page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-12-15T10:29:04.692ZApplied to files:
⏰ 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)
🔇 Additional comments (3)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/views/ladder/components/Leaderboard.js (2)
227-257: Add bounds checks before dereferencing leaderboard rows (ended path especially).
leaderboards[lkey][+rank]can be out-of-range (bad UI event payload / race during refresh), and ended path expectslevelSessionto exist. Suggest:- this.spectateTargets.humans = leaderboards[lkey][+rank].get('levelSession'); + const row0 = leaderboards[lkey]?.[+rank] + if (!row0) return + this.spectateTargets.humans = row0.get('levelSession')(similar for ogres and non-ended
_idbranch).
259-276: Admin “click player name” modal: ensuresessionIdexists before fetching.If an ended-tournament row is missing
levelSession,new LevelSession({ _id: sessionId })will fetch/db/level.session/undefined-like URLs. Quick guard:const sessionId = this.tournamentState === 'ended' ? leaderboards[id].get('levelSession') : leaderboards[id].get('_id') +if (!sessionId) returnapp/views/ladder/LadderTabView.js (1)
462-488: Guard invariant:tournamentState === 'ended'requirestournamentId.Without this, ended-state fetches can hit
/db/tournament/undefined/.... Suggest:constructor (level, team, session, limit, league, tournamentId, ageBracket, myTournamentSubmission, tournamentState) { super() ... this.tournamentId = tournamentId this.tournamentState = tournamentState + if (this.tournamentState === 'ended' && !this.tournamentId) { + throw new Error('tournamentId is required when tournamentState is ended') + } ... }
🧹 Nitpick comments (1)
app/views/ladder/components/Leaderboard.js (1)
36-78: Good propagation oftournamentState; consider defaulting to avoid accidental “not ended”.If
options.tournamentStateis missing, the view falls into the non-ended path (likely fine), but it may be safer to default explicitly:-({ league: this.league, tournament: this.tournament, tournamentState: this.tournamentState, leagueType: this.leagueType, course: this.course } = options) +({ league: this.league, tournament: this.tournament, tournamentState: this.tournamentState = null, leagueType: this.leagueType, course: this.course } = options)
📜 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 (20)
app/core/Router.js(1 hunks)app/core/utils.js(3 hunks)app/schemas/models/tournament.schema.js(1 hunks)app/templates/clans/clan-details.pug(1 hunks)app/templates/courses/tournament-tile-mixin.pug(1 hunks)app/templates/play/tournament_home.pug(1 hunks)app/views/clans/ClanDetailsView.js(1 hunks)app/views/courses/CoursesView.js(1 hunks)app/views/courses/TournamentsListModal.js(2 hunks)app/views/ladder/LadderTabView.js(6 hunks)app/views/ladder/LadderView.js(2 hunks)app/views/ladder/MainTournamentView.js(2 hunks)app/views/ladder/components/EditTournamentModal.vue(1 hunks)app/views/ladder/components/LadderPanel.vue(2 hunks)app/views/ladder/components/Leaderboard.js(6 hunks)app/views/landing-pages/league-v2/components/GlobalRankings.vue(2 hunks)app/views/landing-pages/league/PageLeagueGlobal.vue(5 hunks)app/views/play/level/ControlBarView.coffee(1 hunks)app/views/play/level/modal/HeroVictoryModal.js(1 hunks)ozaria/site/components/teacher-dashboard/BaseMyClasses/components/ClassLinksComponent.vue(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/views/clans/ClanDetailsView.js (2)
app/views/ladder/LadderView.js (2)
utils(24-24)utils(78-78)app/core/Router.js (1)
utils(34-34)
app/views/courses/TournamentsListModal.js (1)
app/views/ladder/LadderView.js (6)
utils(24-24)utils(78-78)require(20-20)require(21-21)require(25-25)require(26-26)
app/views/ladder/LadderView.js (1)
app/core/utils.js (2)
clanId(1847-1847)tournamentId(1848-1848)
🪛 Gitleaks (8.30.0)
app/core/utils.js
[high] 1807-1807: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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)
🔇 Additional comments (16)
app/schemas/models/tournament.schema.js (1)
43-43: LGTM!The addition of the
anonymousfield to the tournament schema is straightforward and correctly structured. The field is appropriately optional and includes clear title and description metadata.app/core/Router.js (1)
369-369: LGTM!The new tournament route with mixed ID parameter is correctly added and follows the established routing pattern. The route integrates well with the existing ladder routes.
app/views/clans/ClanDetailsView.js (1)
73-73: LGTM!The initialization of
tournamentHelperis correctly placed in the constructor and follows the pattern used consistently across other views in this PR.app/views/courses/TournamentsListModal.js (1)
13-13: LGTM!The import and initialization of
tournamentHelperare correctly implemented and follow the established pattern across the codebase.Also applies to: 29-29
app/templates/play/tournament_home.pug (1)
25-28: LGTM!The URL transformation using
tournamentHelper.convertUrlis correctly implemented. The change properly computes the tournament URL and applies it to the anchor element.app/views/ladder/MainTournamentView.js (1)
21-21: LGTM!The import and initialization of
tournamentHelperfollow the established pattern and are correctly placed in the constructor.Also applies to: 72-72
app/views/ladder/components/LadderPanel.vue (1)
89-89: LGTM!The import of utils and the conditional application of
convertUrlare correctly implemented. The logic appropriately applies URL transformation only when both clan and tournament context exist, which aligns with the mixed ID obfuscation feature.Also applies to: 154-167
app/views/courses/CoursesView.js (1)
136-136: LGTM!The initialization of
tournamentHelperis appropriately scoped to CodeCombat only and correctly placed within the existing tournament-related initialization block.app/views/play/level/modal/HeroVictoryModal.js (1)
529-531: LGTM! Tournament URL conversion is correctly applied.The URL transformation logic is sound: the tournament query parameter is appended first, then the URL is converted via
tournamentMixedIdHelper.convertUrl(), and finally the fragment identifier is added. This ensures the conversion operates on the complete tournament URL.app/views/ladder/components/EditTournamentModal.vue (1)
122-127: LGTM! Field rename is consistent.The rename from
anonymizetoanonymousis applied consistently across the id attribute, v-model binding, and label's for attribute. This aligns with the tournament schema change.ozaria/site/components/teacher-dashboard/BaseMyClasses/components/ClassLinksComponent.vue (1)
932-935: LGTM! Tournament URL generation is correctly implemented.The
tournamentUrlmethod properly encapsulates the URL construction and conversion logic usingutils.tournamentMixedIdHelper.convertUrl(). Good separation of concerns.app/views/landing-pages/league-v2/components/GlobalRankings.vue (1)
137-139: LGTM! Correct usage of tournament URL helper.The URL conversion is correctly applied using
tournamentMixedIdHelper.convertUrl(url)when both clan and tournament are present.app/views/play/level/ControlBarView.coffee (1)
146-155: The encrypt/decrypt implementation is properly implemented but uses obfuscation, not cryptographic encryption.The code correctly implements a symmetric encrypt/decrypt pair using XOR with a hardcoded secret key:
ControlBarView.coffeeline 155 callsencrypt(leagueID, tournamentId)and passes the result tohomeViewArgsLadderView.jsline 78 callsdecrypt(mixedID)to recoverclanIdandtournamentIdRouter.jsline 369 correctly maps the/play/tournament/:levelID/:mixedIDroute toLadderViewThe approach is not cryptographic encryption but client-side obfuscation (XOR with hardcoded key exposed in source). This is acceptable for the use case since this is for URL structuring and view argument passing, not authentication or sensitive data protection. The implementation is correct and consistent throughout the flow.
app/core/utils.js (1)
1888-2017: ExportingtournamentMixedIdHelperis fine—verify it’s not used as a real secret.Given the helper is exported broadly, ensure downstream code treats this strictly as URL obfuscation (not access control), and that server endpoints still enforce authorization based on the resolved clan/tournament IDs.
app/views/ladder/components/Leaderboard.js (1)
175-212: Ended vs non-ended row mapping looks consistent.The switched branching to
this.tournamentState === 'ended'matches the different row shapes (wins/losses/win-ratevsscore/when).app/views/ladder/LadderTabView.js (1)
490-558: Ended-state branching for collections/URLs is coherent.Switching to
TournamentLeaderboardCollectionand the tournament rankings endpoints whentournamentState === 'ended'aligns the data model with the UI changes.
mrfinch
left a comment
There was a problem hiding this comment.
Could you also resove AI bot comments?
| }) | ||
| } | ||
|
|
||
| const tournamentMixedIdHelper = { |
There was a problem hiding this comment.
could you explain how this is secure? I don't understand how encrypt / decrpyt function present in client can help.
Where did u get the logic from? Can you put a reference to it?
There was a problem hiding this comment.
it's not for secure but for obfuscation
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/views/ladder/LadderView.js (2)
68-82: Handle invalidmixedIDsafely to prevent view crashes.The
utils.tournamentMixedIdHelper.decrypt(mixedID)call on line 78 can throw or return invalid data when given malformed input. Wrap the decrypt operation in a try/catch block to handle errors gracefully:if (currentPath.includes('/play/tournament/')) { const mixedID = leagueTypeOrMixedId - const { clanId, tournamentId } = utils.tournamentMixedIdHelper.decrypt(mixedID) - this.leagueType = 'clan' - this.leagueID = clanId - this.tournamentId = tournamentId + try { + const { clanId, tournamentId } = utils.tournamentMixedIdHelper.decrypt(mixedID) + this.leagueType = 'clan' + this.leagueID = clanId + this.tournamentId = tournamentId + } catch (e) { + console.warn('Invalid tournament mixedID:', mixedID, e) + // Fall back to treating leagueTypeOrMixedId as leagueType + } }
329-335: Usethis.leagueTypeinstead of hardcoding'clan'for ended tournaments.Line 333 hardcodes
leagueType: 'clan'even thoughthis.leagueTypeis available and may be'course'for course-based tournaments. The non-ended branches (lines 342) correctly passthis.leagueType. Use the instance property to ensure correct behavior:this.insertSubView(this.ladderTab = new TournamentLeaderboard({ league: this.league, tournament: this.tournamentId, tournamentState: 'ended', - leagueType: 'clan', + leagueType: this.leagueType, myTournamentSubmission: this.myTournamentSubmission, }, this.level, this.sessions))
🧹 Nitpick comments (2)
app/views/play/level/PlayLevelView.coffee (1)
147-147: Consider validating the tournament parameter.The
tournamentquery parameter is read without validation. While the main encoding concern is in LevelLoader (see comments on app/lib/LevelLoader.coffee), consider adding basic validation here to catch malformed values early.app/views/play/SpectateView.js (1)
86-86: Query parameter naming inconsistency across views.SpectateView reads the
tidquery parameter, while PlayLevelView (line 147) readstournament. Both pass the value astIdto LevelLoader. If this naming difference is intentional (e.g., different URL conventions for spectate vs play modes), consider documenting it. Otherwise, standardizing to a single parameter name would improve consistency.
📜 Review details
Configuration used: defaults
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 (10)
app/lib/LevelLoader.coffee(3 hunks)app/templates/play/ladder/my_matches_tab.pug(2 hunks)app/views/ladder/LadderPlayModal.js(1 hunks)app/views/ladder/LadderView.js(2 hunks)app/views/ladder/MyMatchesTabView.js(1 hunks)app/views/ladder/components/Leaderboard.js(7 hunks)app/views/ladder/components/Leaderboard.vue(3 hunks)app/views/play/SpectateView.js(2 hunks)app/views/play/level/ControlBarView.coffee(1 hunks)app/views/play/level/PlayLevelView.coffee(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/play/level/ControlBarView.coffee
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mrfinch
Repo: codecombat/codecombat PR: 8170
File: app/templates/courses/tournament-tile-mixin.pug:14-16
Timestamp: 2025-12-15T10:29:04.692Z
Learning: In CodeCombat repository, tournamentHelper (utils.tournamentMixedIdHelper) should be initialized universally in all views that render tournament-related templates, not conditionally based on utils.isCodeCombat, to prevent runtime errors in non-CodeCombat deployments.
📚 Learning: 2025-12-15T10:29:04.692Z
Learnt from: mrfinch
Repo: codecombat/codecombat PR: 8170
File: app/templates/courses/tournament-tile-mixin.pug:14-16
Timestamp: 2025-12-15T10:29:04.692Z
Learning: In CodeCombat repository, tournamentHelper (utils.tournamentMixedIdHelper) should be initialized universally in all views that render tournament-related templates, not conditionally based on utils.isCodeCombat, to prevent runtime errors in non-CodeCombat deployments.
Applied to files:
app/views/play/level/PlayLevelView.coffeeapp/views/ladder/components/Leaderboard.vueapp/views/ladder/LadderView.js
📚 Learning: 2025-12-15T10:28:56.606Z
Learnt from: mrfinch
Repo: codecombat/codecombat PR: 8170
File: app/templates/courses/tournament-tile-mixin.pug:14-16
Timestamp: 2025-12-15T10:28:56.606Z
Learning: In CodeCombat, ensure tournamentHelper (utils.tournamentMixedIdHelper) is initialized in all views that render tournament-related templates, not only when utils.isCodeCombat is true. This prevents runtime errors in non-CodeCombat deployments. Apply this change to all pug templates that render tournament components (e.g., tournament tiles, lists) under app/templates. Verify by scanning for initialization points and confirm no conditional bypasses affect deployment.
Applied to files:
app/templates/play/ladder/my_matches_tab.pug
🧬 Code graph analysis (1)
app/views/ladder/LadderView.js (1)
app/core/utils.js (2)
clanId(1847-1847)tournamentId(1848-1848)
⏰ 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)
🔇 Additional comments (14)
app/views/ladder/components/Leaderboard.vue (3)
50-53: LGTM: Tournament prop definition is correct.The new
tournamentprop follows Vue conventions and integrates cleanly with the existing props structure.
63-68: LGTM: Clean computed property implementation.The
tournamentIdcomputed property correctly filters tournament context to arena score types, centralizing the conditional logic and keeping URL construction code clean.
248-250: LGTM: Tournament parameter correctly added to fight URLs.The conditional URL construction follows the existing pattern for league parameters and appropriately scopes tournament inclusion to clan leagues with arena score types.
app/views/ladder/LadderPlayModal.js (1)
145-147: No issues identified. The code correctly handles tournament as a string ID and league as an object with.idproperty, which reflects their different data structures in the application.app/templates/play/ladder/my_matches_tab.pug (2)
59-72: LGTM! Tournament context properly propagated to spectate URLs.The extraction of
tidfromview.options.tournamentand its conditional inclusion in the spectate URL is correctly implemented and consistent with the existing league parameter handling.
80-87: LGTM! Play URLs correctly handle league types and tournament context.The conditional logic properly constructs the play URL based on
leagueType, appending appropriate parameters for clan leagues, course leagues, and tournaments.app/views/ladder/components/Leaderboard.js (3)
40-43: LGTM! Constructor properly initializes tournamentState.The destructuring of
tournamentStatefrom options and its assignment tothis.tournamentStatecorrectly sets up the state-based branching used throughout the file. The continued use ofthis.tournamentfor ID-based checks (like line 43) is appropriate.
62-79: LGTM! TournamentState-based branching improves clarity.The replacement of
this.tournamentchecks with explicitthis.tournamentStatecomparisons makes the conditional logic clearer. The different data shapes for ended vs. active tournaments (wins/losses/win-rate vs. score/submitDate) are appropriate for their respective use cases.Also applies to: 181-211
229-229: LGTM! Consistent tournamentState usage throughout.The
tournamentStateparameter is correctly passed toLeaderboardData(line 229) and used consistently in conditional logic throughout the file for session ID selection, player ID extraction, and tournament submission handling.Also applies to: 247-247, 263-263, 269-269, 272-272
app/views/ladder/LadderView.js (2)
340-348: LGTM! Active tournament leaderboard correctly configured.The
TournamentLeaderboardinstantiation for active tournaments properly usesthis.leagueType(line 342) and passes all necessary context includingtournamentState,course, and theupdateSpectateListcallback.
352-352: LGTM! Tournament context properly passed to MyMatchesTabView.The
tournamentparameter is correctly passed toMyMatchesTabView, enabling the template to accessview.options.tournament(as seen in the my_matches_tab.pug changes).app/views/ladder/MyMatchesTabView.js (1)
175-177: Tournament context propagation is properly implemented.The change correctly adds tournament-specific handling to the user names request. The pattern difference you noted is actually correct:
this.options.leagueis an object (requiring.idaccessor) whilethis.options.tournamentreceivesthis.tournamentId, which is already an ID string. Both approaches are consistent with how the options are initialized in LadderView.app/views/play/level/PlayLevelView.coffee (1)
191-191: LGTM - Parameter correctly passed to LevelLoader.The
tournamentIdis appropriately passed astIdto LevelLoader options, enabling tournament-aware session loading.app/views/play/SpectateView.js (1)
135-135: LGTM - Parameter correctly passed to LevelLoader.The
tIdvalue is appropriately threaded through to LevelLoader for tournament-aware spectate session loading.
fix ENG-2164
Summary by CodeRabbit
New Features
UI
✏️ Tip: You can customize this high-level summary in your review settings.