Conversation
… backed by new team scoring metrics.
feat: added swiss round table
…able Revert "feat: added swiss round table"
📝 WalkthroughWalkthroughThe PR adds comprehensive documentation for agents and tournament mechanics, introduces admin-reveal functionality for matches and teams with role-based visibility filtering, adds a match cleanup endpoint and database seeding script, refactors match/team services to support Buchholz calculations and enhanced filtering, and updates frontend components with tabbed group phase views, ranking tables, and admin-controlled visibility toggles. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Frontend
participant Server as Match/Team Service
participant Auth as Authorization
participant DB as Database
rect rgba(100, 150, 200, 0.5)
note over Client,DB: Admin Reveal Flow
Client->>Server: GET matches with adminReveal=true
Server->>Auth: Check if user is event admin
Auth-->>Server: Admin status result
alt Is Admin
Server->>DB: Fetch all match fields (scores, results)
DB-->>Server: Full match data
Server->>DB: Calculate Buchholz points
DB-->>Server: Buchholz values
Server-->>Client: Return full data (score, buchholzPoints, hadBye)
else Non-Admin
Server->>DB: Fetch limited fields (only finished results)
DB-->>Server: Match results
Server->>Server: Compute revealed_match_wins
Server-->>Client: Return limited data (computed revealed score)
end
end
rect rgba(200, 100, 100, 0.5)
note over Client,DB: Cleanup Flow
Client->>Server: PUT /cleanup-all/{eventId}/{phase}
Server->>Auth: Verify event admin
Auth-->>Server: Authorization check
Server->>DB: Acquire advisory lock for event
Server->>DB: Delete matches for phase
Server->>DB: Reset Swiss stats (score, buchholzPoints, hadBye)
Server->>DB: Reset current round
Server->>DB: Release lock
DB-->>Server: Cleanup complete
Server-->>Client: Success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…-phase-bracket-design-improvements
PaulicStudios
changed the base branch from
dev
to
537-ability-to-add-different-repo-templates
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/app/events/[id]/dashboard/dashboard.tsx (1)
652-713:⚠️ Potential issue | 🟡 MinorBoth cleanup buttons display a spinner when either cleanup is running.
cleanupMatchesMutation.isPendingistrueregardless of which phase is being cleaned up. The result is that when the SWISS cleanup runs, the "Clean Up Tournament matches" button also shows a loading spinner (and vice versa), even though only one operation is active.TanStack Query v5 exposes
mutation.variablesto identify the active call. Use it to scope the spinner to the correct button:🔧 Proposed fix using
cleanupMatchesMutation.variables- <Button - disabled={cleanupMatchesMutation.isPending} + <Button + disabled={cleanupMatchesMutation.isPending} onClick={() => { if ( confirm( "Are you sure you want to delete ALL Group Phase matches? This will also reset team scores!", ) ) { cleanupMatchesMutation.mutate("SWISS"); } }} variant="destructive" > - {cleanupMatchesMutation.isPending ? ( + {cleanupMatchesMutation.isPending && cleanupMatchesMutation.variables === "SWISS" ? ( <Loader2 className="mr-2 h-4 w-4 animate-spin" /> ) : ( <Trash2 className="mr-2 h-4 w-4" /> )} Clean Up Group Phase </Button>Apply the symmetric change to the "Clean Up Tournament matches" button (check
cleanupMatchesMutation.variables === "ELIMINATION").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/dashboard/dashboard.tsx around lines 652 - 713, Both cleanup buttons currently use cleanupMatchesMutation.isPending causing either button to show a spinner whenever any cleanup is running; scope the pending state to the phase by checking the mutation's variables: update the SWISS button's spinner/disabled logic to use cleanupMatchesMutation.isPending && cleanupMatchesMutation.variables === "SWISS" and update the Tournament button's spinner/disabled logic to use cleanupMatchesMutation.isPending && cleanupMatchesMutation.variables === "ELIMINATION" so each button only shows loading when its own mutate("SWISS") or mutate("ELIMINATION") call is active (keep the onClick mutate arguments as-is).api/src/match/match.controller.ts (1)
33-38:⚠️ Potential issue | 🔴 Critical
Boolean(adminRevealQuery)on a query-string parameter is incorrect —Boolean("false") === true.NestJS delivers
@Query()values as strings.Boolean("false")evaluates totruein JavaScript, so passing?adminRevealQuery=falsewill be misinterpreted astrue. This affects all three usages (lines 38, 88, 201). Use an explicit comparison instead.Proposed fix (apply to all three sites)
- Boolean(adminRevealQuery), + adminRevealQuery === "true",And update the type annotation to be honest about what's received:
- `@Query`("adminRevealQuery") adminRevealQuery: boolean, + `@Query`("adminRevealQuery") adminRevealQuery: string,Also applies to: 191-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/match/match.controller.ts` around lines 33 - 38, The controller is converting the query param adminRevealQuery with Boolean(adminRevealQuery) which misinterprets strings like "false"; change to an explicit string comparison (e.g., adminRevealQuery === "true") wherever this pattern is used (including the calls into matchService.getSwissMatches and the other two usages that forward adminRevealQuery), and update the parameter type from boolean to string (e.g., `@Query`("adminRevealQuery") adminRevealQuery: string) so the code accurately reflects the received type before converting via the explicit comparison.api/src/match/match.service.ts (1)
791-812:⚠️ Potential issue | 🟠 Major
calculateBuchholzPointsForTeam(old) only sums opponents from won matches, whilecalculateBuchholzPointsForTeams(new) correctly sums opponents from all matches.Standard Buchholz is the sum of all opponents' scores regardless of match outcome. The old method on line 795 filters by
winner: { id: teamId }, so it misses opponents from lost matches. The old method is still called fromcalculateBuchholzPoints(line 782), which persists incorrect values to the DB when the Swiss phase ends.Consider updating
calculateBuchholzPoints(line 777) to use the new batch method, or fix the old single-team method to include all matches.Proposed fix for `calculateBuchholzPoints`
async calculateBuchholzPoints(eventId: string): Promise<void> { const teams = await this.teamService.getTeamsForEvent(eventId); + const teamIds = teams.map((t) => t.id); + const buchholzMap = await this.calculateBuchholzPointsForTeams( + teamIds, + eventId, + false, + ); await Promise.all( teams.map(async (team) => { - const buchholzPoints = await this.calculateBuchholzPointsForTeam( - team.id, - eventId, - ); + const buchholzPoints = buchholzMap.get(team.id) || 0; await this.teamService.updateBuchholzPoints(team.id, buchholzPoints); }), ); }Also applies to: 1196-1266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/match/match.service.ts` around lines 791 - 812, The current calculateBuchholzPointsForTeam implementation only queries won matches (winner: { id: teamId }) so it omits opponents from lost/drawn matches; update it to mirror calculateBuchholzPointsForTeams by querying all matches containing the team (e.g., remove the winner filter and use where: { teams: { id: teamId } } or simply delegate to calculateBuchholzPointsForTeams to compute the sum), then sum opponent.score for every match rather than only wins; also update calculateBuchholzPoints (which calls calculateBuchholzPointsForTeam) to use the batch calculateBuchholzPointsForTeams path or the corrected single-team method so persisted Buchholz values are correct.
🧹 Nitpick comments (18)
frontend/app/events/[id]/bracket/graphView.tsx (5)
142-148:onClickhandler mixes closure variable and callback parameter for the state check.In both the bracket-match handler (line 144:
match.state) and the placement-match handler (line 177:placementMatch.state), the state is read from the outer closure variable while theidis read from theclickedMatchparameter. IfMatchNodealways invokesonClick(data.match), these are the same object, but it's inconsistent and fragile — a future refactor inMatchNodecould silently break the guard.Use the callback parameter consistently:
♻️ Suggested fix (bracket match)
onClick: (clickedMatch: Match) => { if ( - (match.state === MatchState.FINISHED || isEventAdmin) && + (clickedMatch.state === MatchState.FINISHED || isEventAdmin) && clickedMatch.id ) router.push(`/events/${eventId}/match/${clickedMatch.id}`); },♻️ Suggested fix (placement match)
onClick: (clickedMatch: Match) => { if ( - (placementMatch.state === MatchState.FINISHED || + (clickedMatch.state === MatchState.FINISHED || isEventAdmin) && clickedMatch.id ) router.push(`/events/${eventId}/match/${clickedMatch.id}`); },Also applies to: 175-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/bracket/graphView.tsx around lines 142 - 148, The onClick handlers mix closure variables and the callback parameter (e.g., reading match.state or placementMatch.state from the outer scope while using clickedMatch.id), which is fragile; update both handlers (the bracket onClick and the placement onClick) to consistently use the callback parameter's state (clickedMatch.state) instead of the closure variables (match.state or placementMatch.state) so the guard checks the same object you receive from MatchNode; locate the onClick definitions in graphView.tsx and replace uses of match.state and placementMatch.state with clickedMatch.state.
42-218: The entire graph-building logic lives in a singleuseEffect— consider extracting helpers.The effect spans ~175 lines covering placeholder generation, real-match grouping, node construction, placement-match handling, and edge wiring. Extracting pure helper functions (e.g.,
buildPlaceholderNodes,buildMatchNodes,buildEdges) would improve readability and make the individual pieces independently testable.This is non-urgent but would help maintainability as the bracket logic evolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/bracket/graphView.tsx around lines 42 - 218, The useEffect containing the bracket graph logic is too large; extract pure helper functions to improve readability and testability by moving placeholder generation, match-node construction (including handling bracketMatches and placementMatch), and edge wiring into separate functions such as buildPlaceholderNodes(matches, teamCount, VERTICAL_SPACING, ROUND_SPACING), buildMatchNodes(matchesByRound, totalRounds, lastRoundIndex, placementMatch, router, eventId, isEventAdmin) and buildEdges(nodeIdsByRound) that return arrays and a nodeIdsByRound map; then have the useEffect call these helpers to produce newNodes/newEdges and finally call setNodes/newEdges and setEdges, preserving existing symbols (useEffect, nodeIdsByRound, newNodes, newEdges, placementMatch, bracketMatches, setNodes, setEdges) and behavior.
220-253:fitViewOptionszoom range is narrower than the global zoom range — verify this is intentional.
fitViewOptionsconstrains initial fit tominZoom: 0.2/maxZoom: 1, while the<ReactFlow>element allowsminZoom: 0.05/maxZoom: 1.5. This means the auto-fit will never zoom below 0.2 even though the user can manually zoom down to 0.05. If this is intentional (preventing an overly zoomed-out first paint while still allowing manual deep zoom), it's fine — just worth a brief inline comment to prevent someone from "fixing" the apparent mismatch later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/bracket/graphView.tsx around lines 220 - 253, The fitViewOptions minZoom/maxZoom (minZoom: 0.2, maxZoom: 1) differ from the global ReactFlow minZoom/maxZoom props (minZoom: 0.05, maxZoom: 1.5), which causes auto-fit to be limited while manual zoom allows deeper zoom; either align the fitViewOptions to match the ReactFlow minZoom/maxZoom or, if the narrower initial-fit range is intentional, add a concise inline comment next to fitViewOptions in graphView.tsx explaining the rationale so future readers don't "fix" the mismatch (refer to the ReactFlow component props and the fitViewOptions object).
61-71: Replaceas anycasts with proper enum values for type safety.Lines 65–66 use
as anyto bypass type checking forstateandphase. Since bothMatchState(already imported on line 7) andMatchPhaseenums are available in the codebase with the required members, use the enum values directly:♻️ Suggested fix
- state: "PLANNED" as any, - phase: "ELIMINATION" as any, + state: MatchState.PLANNED, + phase: MatchPhase.ELIMINATION,Add
MatchPhaseto the import statement on line 7:import { MatchState, MatchPhase } from "@/app/actions/tournament-model";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/bracket/graphView.tsx around lines 61 - 71, The placeholderMatch object uses unsafe casts "as any" for state and phase; replace those with the proper enum members from MatchState and MatchPhase (e.g., MatchState.PLANNED and MatchPhase.ELIMINATION) and add MatchPhase to the import list alongside MatchState (import { MatchState, MatchPhase } from "@/app/actions/tournament-model"); update the placeholderMatch fields state and phase to use the enum values rather than casts.
25-35: Remove unusedisAdminViewprop from component signature and its caller.The
isAdminViewprop is declared but never referenced in the component. Removing it requires updating both the GraphView component signature and the caller infrontend/app/events/[id]/bracket/page.tsx.Changes needed
In
frontend/app/events/[id]/bracket/graphView.tsx:export default function GraphView({ matches, teamCount, isEventAdmin, - isAdminView, }: { matches: Match[]; teamCount: number; isEventAdmin: boolean; - isAdminView: boolean; }) {In
frontend/app/events/[id]/bracket/page.tsx:<GraphView matches={serializedMatches} teamCount={teamCount} isEventAdmin={eventAdmin} - isAdminView={isAdminView} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/bracket/graphView.tsx around lines 25 - 35, GraphView currently declares an unused prop isAdminView; remove isAdminView from the GraphView component signature (the exported function GraphView and its props type) and update any place that instantiates GraphView (callsites that pass isAdminView, e.g., the component usage in page.tsx) to stop passing that prop so props match the new signature; ensure you only remove the prop and its usage, leaving matches, teamCount, and isEventAdmin intact.frontend/components/match/MatchNode.tsx (3)
85-89:formatTeamNamecan be hoisted to module scopeIt's a pure, stateless utility with no closure over component state. Defining it inside the component recreates it on every render unnecessarily.
♻️ Proposed refactor
+const formatTeamName = (teamName: string, maxLength: number = 12) => { + return teamName.length > maxLength + ? `${teamName.substring(0, maxLength)}...` + : teamName; +}; + function MatchNode({ data }: MatchNodeProps) { ... - const formatTeamName = (teamName: string, maxLength: number = 12) => { - return teamName.length > maxLength - ? `${teamName.substring(0, maxLength)}...` - : teamName; - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/match/MatchNode.tsx` around lines 85 - 89, The pure utility formatTeamName defined inside the MatchNode component should be hoisted to module scope to avoid recreating it on every render; move the function declaration for formatTeamName out of the component (above the component export), keep its signature (teamName: string, maxLength: number = 12), and export it if other modules need it; update any references inside MatchNode to call the now-module-level formatTeamName and remove the inner definition to reduce renders and allocations.
7-8: React Flow imports are correct for v11; v12 migration is a major undertakingThe imports on lines 7-8 are correct for
reactflow 11.11.4. However, React Flow v12 (@xyflow/react, current stable: 12.10.0) introduces extensive breaking changes including node property renames (e.g.,xPos/yPos→positionAbsoluteX/positionAbsoluteY,width/height→node.measured.*), edge reconnect API refactor, mutability constraints, and utility function deprecations. A v11→v12 migration across this codebase would require significant refactoring beyond this file. Consider as a separate, planned initiative rather than an ad-hoc upgrade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/match/MatchNode.tsx` around lines 7 - 8, The import change in MatchNode.tsx incorrectly suggests migrating React Flow to v12; keep the current v11-compatible imports (useParams, useRouter from next/navigation and Handle, Position from reactflow) and do not swap to `@xyflow/react` or rename node props (e.g., xPos/yPos) here; if a migration is desired, revert any edits in MatchNode.tsx that reference `@xyflow/react` or v12-only APIs and instead add a short inline comment near the imports (MatchNode.tsx) stating the code targets reactflow v11 and that a coordinated, repo-wide v12 migration (updating node props, edge APIs, and mutability patterns) should be planned as a separate task.
105-120: AdduseUpdateNodeInternalshook to notify React Flow when handle visibility changesWhen handles are conditionally rendered and their visibility might change after a node mounts, React Flow's internal connection graph must be notified via
useUpdateNodeInternals. Even though the bracket graph typically recreates nodes from scratch, if a node'sshowTargetHandleorshowSourceHandleprops change on an already-mounted instance, the handles will be added/removed from the DOM. Without callingupdateNodeInternals, React Flow won't recalculate handle positions and edges may fail to connect properly.♻️ Proposed implementation
import { Handle, Position } from "reactflow"; +import { useUpdateNodeInternals } from "reactflow"; +import { useEffect } from "react";const router = useRouter(); const params = useParams<{ id?: string }>(); + const updateNodeInternals = useUpdateNodeInternals(); + const nodeId = data.match.id ?? ""; + + useEffect(() => { + if (nodeId) updateNodeInternals(nodeId); + }, [showTargetHandle, showSourceHandle, nodeId, updateNodeInternals]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/match/MatchNode.tsx` around lines 105 - 120, The MatchNode component conditionally renders handles (showTargetHandle/showSourceHandle) but doesn't notify React Flow when visibility changes; import and call the useUpdateNodeInternals hook inside MatchNode, get updateNodeInternals(node.id), and invoke it inside a useEffect that depends on [showTargetHandle, showSourceHandle, node.id] so React Flow recalculates handle positions when handles are added/removed; ensure you import useEffect and useUpdateNodeInternals and reference the component's node id when calling updateNodeInternals.frontend/app/events/[id]/bracket/actions.tsx (1)
16-20: Optional: removeadminRevealparam instead of setting it to"false"when toggling off.Currently the URL always contains
adminReveal(either"true"or"false"). Deleting the param whenvalueis false produces a cleaner URL and avoids sharing a URL that looks like it has an active toggle.♻️ Proposed refactor
onCheckedChange={(value) => { const params = new URLSearchParams(searchParams.toString()); - params.set("adminReveal", value ? "true" : "false"); + if (value) { + params.set("adminReveal", "true"); + } else { + params.delete("adminReveal"); + } router.replace(`?${params.toString()}`); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/bracket/actions.tsx around lines 16 - 20, When the toggle is turned off, remove the adminReveal search param instead of setting it to "false": in the onCheckedChange handler use params.delete("adminReveal") when value is false and params.set("adminReveal","true") when true; then call router.replace with either `?${params.toString()}` when params is non-empty or the bare path (e.g., router.pathname) when empty to avoid a trailing empty query string. Target the onCheckedChange closure that reads searchParams, manipulates params, and calls router.replace..agent/rules/website.md (1)
1-105: Near-duplicate ofAGENTS.md— consolidate into a single source of truth.Both
.agent/rules/website.mdandAGENTS.mdcontain identical guidance (project structure, build commands, code conventions, agent rules). Any future update will need to be applied to both files independently, which will inevitably lead to drift.Consider one of:
- Remove
.agent/rules/website.mdand have it referenceAGENTS.mdwith a one-liner.- Make
AGENTS.mdthe single canonical document and have.agent/rules/website.mdsimply@includeit (if your agent framework supports that).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agent/rules/website.md around lines 1 - 105, There’s a duplicate policy file: .agent/rules/website.md replicates AGENTS.md; remove the duplication by either deleting .agent/rules/website.md and replacing it with a one-line pointer to AGENTS.md or convert .agent/rules/website.md into an include/wrapper that references AGENTS.md (depending on agent framework support); update any internal references to use AGENTS.md as the canonical source and ensure CI/docs links still resolve (search for references to ".agent/rules/website.md" and update them to point to "AGENTS.md" or the new include).frontend/app/events/[id]/dashboard/dashboard.tsx (1)
654-661: Consider replacingconfirm()with a destructive dialog component.Native
confirm()is synchronous, blocks the browser event loop, cannot be styled to match the app's design system, and is suppressed in certain embedded environments (iframes). A controlled dialog (e.g.,AlertDialogfrom your UI library) provides a consistent, accessible confirmation experience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/dashboard/dashboard.tsx around lines 654 - 661, Replace the synchronous browser confirm() call inside the onClick handler with a controlled destructive confirmation dialog from the app UI library (e.g., AlertDialog): add local state (e.g., isDeleteDialogOpen / setIsDeleteDialogOpen) and open the dialog when the button is clicked, render the AlertDialog near the component currently containing the onClick, wire the dialog's confirm action to call cleanupMatchesMutation.mutate("SWISS") and then close the dialog, and wire the dialog's cancel/close action to simply close the dialog; ensure the dialog is styled and accessible according to the app design system and that any loading/error state from cleanupMatchesMutation is surfaced in the dialog UI.frontend/app/events/[id]/groups/RankingTable.tsx (2)
42-52:getMatchHistoryruns an O(N·M) scan on every render; consider pre-computing a lookup map.
getMatchHistoryis called once per team insidesortedTeams.map(), and each call iterates all matches. For large tournaments this is quadratic. A single pass to build aMap<teamId, result[]>before the render loop makes it O(N + M):♻️ Suggested optimisation
+ const historyByTeam = new Map<string, ("W" | "L" | "T")[]>(); + matches + .filter((m) => m.state === MatchState.FINISHED) + .sort((a, b) => a.round - b.round) + .forEach((m) => { + const result = (teamId: string) => (!m.winner ? "T" : m.winner.id === teamId ? "W" : "L") as "W" | "L" | "T"; + m.teams.forEach((t) => { + if (!historyByTeam.has(t.id)) historyByTeam.set(t.id, []); + historyByTeam.get(t.id)!.push(result(t.id)); + }); + }); - const getMatchHistory = (teamId: string) => { - return matches - .filter( - (m) => m.state === "FINISHED" && m.teams.some((t) => t.id === teamId), - ) - .sort((a, b) => a.round - b.round) - .map((m) => { - if (!m.winner) return "T"; - return m.winner.id === teamId ? "W" : "L"; - }); - };Then in the render loop replace
getMatchHistory(team.id)withhistoryByTeam.get(team.id) ?? [].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/groups/RankingTable.tsx around lines 42 - 52, getMatchHistory iterates all matches for every team causing O(N·M); instead precompute a Map<string, string[]> called historyByTeam by first sorting matches by round (or iterating matches in round order) and in a single pass pushing "W"/"L"/"T" into each involved team's array based on m.winner, keyed by team.id, so history arrays preserve round order. Replace calls to getMatchHistory(team.id) inside sortedTeams.map with historyByTeam.get(team.id) ?? [] (you can remove or keep getMatchHistory as a fallback). Ensure you reference the existing matches array and maintain the same "W"/"L"/"T" semantics when populating historyByTeam.
44-45: UseMatchState.FINISHEDinstead of the string literal.
graphView.tsxand other components in this PR import and useMatchState.FINISHEDfor the same comparison. Relying on the raw string"FINISHED"here means TypeScript won't catch a mismatch if the enum value changes.♻️ Suggested fix
+import { MatchState } from "@/app/actions/tournament-model"; ... .filter( - (m) => m.state === "FINISHED" && m.teams.some((t) => t.id === teamId), + (m) => m.state === MatchState.FINISHED && m.teams.some((t) => t.id === teamId), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/groups/RankingTable.tsx around lines 44 - 45, Replace the string literal "FINISHED" in the RankingTable filter callback with the enum value MatchState.FINISHED: update the predicate (m) => m.state === "FINISHED" && m.teams.some((t) => t.id === teamId) to use MatchState.FINISHED, and add the corresponding import for MatchState (same source used by graphView.tsx and other components) so TypeScript can type-check state comparisons.frontend/app/events/[id]/groups/graphView.tsx (1)
138-143:variant={undefined}is redundant; omit the prop to use the default.Explicitly passing
undefinedto an optional prop is equivalent to omitting it in React but can mislead readers into thinking it serves a purpose. If the defaultBackgroundVariant.Dotsis the intended appearance, drop thevariantattribute entirely.♻️ Suggested cleanup
<Background color="currentColor" className="opacity-10" gap={20} - variant={undefined} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/groups/graphView.tsx around lines 138 - 143, Remove the redundant explicit prop assignment variant={undefined} on the JSX Background component; simply omit the variant prop so the component uses its default (BackgroundVariant.Dots). Update the Background invocation in graphView.tsx (the Background component usage) by deleting the variant attribute to avoid misleading readers and rely on the default prop behavior.frontend/app/events/[id]/groups/GroupPhaseTabs.tsx (1)
34-38: Considerrouter.replaceinstead ofrouter.pushto avoid polluting browser history.Each tab switch currently pushes a new history entry. Users pressing the browser back button will cycle through tab changes instead of navigating to the previous page.
Proposed fix
const onTabChange = (value: string) => { const params = new URLSearchParams(searchParams.toString()); params.set("tab", value); - router.push(`${pathname}?${params.toString()}`, { scroll: false }); + router.replace(`${pathname}?${params.toString()}`, { scroll: false }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/groups/GroupPhaseTabs.tsx around lines 34 - 38, The onTabChange handler currently uses router.push which creates a new browser history entry on every tab change; update the function to call router.replace instead of router.push so tab switches replace the current history entry and don’t clutter the back-button navigation. Locate the onTabChange function that builds params from searchParams and calls router.push(`${pathname}?${params.toString()}`, { scroll: false }) and change that call to router.replace while keeping the same URL and options (e.g., scroll: false).frontend/app/actions/team.ts (1)
72-83: Inconsistent use of||vs??for fallback values.
getTeamByIdandgetMyEventTeamuse|| 0/|| false(lines 77-78, 103-104), whilegetTeamsForEventTable(lines 237-240) correctly uses?? 0/?? false. While not a bug here (since0andfalseare the intended fallbacks), prefer??consistently so that a future field with a meaningful falsy value doesn't silently get overwritten.Also applies to: 97-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/actions/team.ts` around lines 72 - 83, In getTeamById and getMyEventTeam (and the other occurrences around 97-109) replace uses of the logical OR fallback (e.g., "repo || ''", "buchholzPoints || 0", "hadBye || false") with the nullish coalescing operator (e.g., "repo ?? ''", "buchholzPoints ?? 0", "hadBye ?? false") so that only null/undefined are replaced and legitimate falsy values like 0 or false are preserved; mirror the approach already used in getTeamsForEventTable to keep behavior consistent across functions.api/src/match/match.controller.ts (1)
172-185: Unvalidatedphaseparameter cast toMatchPhaseviaas any.The
phaseroute param is typed asstringand then silently cast withas anyon line 184 (same pattern as line 169). If a caller passes an invalid phase value, the service will execute with a nonsensical filter. Consider using aParseEnumPipeor a manual validation step.Proposed fix
+import { MatchPhase } from "./entites/match.entity"; ... `@Put`("cleanup-all/:eventId/:phase") async cleanupMatches( `@Param`("eventId", ParseUUIDPipe) eventId: string, - `@Param`("phase") phase: string, + `@Param`("phase", new ParseEnumPipe(MatchPhase)) phase: MatchPhase, `@UserId`() userId: string, ) { ... - return this.matchService.cleanupMatchesInPhase(eventId, phase as any); + return this.matchService.cleanupMatchesInPhase(eventId, phase); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/match/match.controller.ts` around lines 172 - 185, The route handler cleanupMatches casts the string phase to MatchPhase via "as any" before calling matchService.cleanupMatchesInPhase; replace this unsafe cast with proper enum validation: import the MatchPhase enum and either use Nest's ParseEnumPipe on the `@Param` (e.g., `@Param`("phase", new ParseEnumPipe(MatchPhase)) phase: MatchPhase) so the framework rejects invalid phases automatically, or perform a manual check inside cleanupMatches (e.g., if (!Object.values(MatchPhase).includes(phase as any)) throw BadRequestException) and then call matchService.cleanupMatchesInPhase(eventId, phase) without any "as any" cast; update the method signature and remove the cast to ensure type-safety.api/src/team/team.service.ts (1)
407-426:team.scoreis selected in the non-reveal path (line 423) but overwritten during mapping — consider removing to avoid leaking the real score in the entity layer.The raw
team.scorefrom the database is fetched even whenrevealAllis false. Although it's overwritten in the final mapping (lines 503-506), the intermediate entity object carries the real score. If any future code accesses the entity before the mapping step, it would see the actual score.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/team/team.service.ts` around lines 407 - 426, The non-reveal branch of the query in team.service.ts currently selects the raw "team.score" into the entity (inside the else branch that builds the leftJoin on MatchEntity and the select([...]) array); remove "team.score" from that select array so the persistent entity never carries the real score when revealAll is false, relying instead on the computed fields (e.g., the added "revealed_match_wins" addSelect and the later mapping logic that sets the public/revealed score). Locate the else branch that calls leftJoin(MatchEntity, "match", ...) and edit the select([...]) to omit "team.score".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/match/match.controller.ts`:
- Around line 187-203: The getMatchById endpoint is missing the JwtAuthGuard so
the `@UserId`() decorator will receive undefined; add the JwtAuthGuard to this
route (e.g., annotate the getMatchById handler with `@UseGuards`(JwtAuthGuard) or
apply JwtAuthGuard at the controller level) and import JwtAuthGuard from the
auth module so request.user is populated and the userId-based admin reveal logic
in matchService.getMatchById works as intended; keep the existing `@UserId`()
parameter and Boolean(adminRevealQuery) handling unchanged.
In `@api/src/scripts/seed-users-teams.ts`:
- Around line 105-107: The outer .catch on bootstrap does not ensure the
initialized DataSource is destroyed; update bootstrap() (the function that calls
dataSource.initialize() and runs seeding logic) to wrap all post-initialize work
in a try/finally so dataSource.destroy() is always awaited (or, alternatively,
ensure the outer error handler calls dataSource.destroy() if dataSource was
initialized). Specifically, locate the dataSource.initialize() and surrounding
seeding logic in bootstrap(), enclose the seeding logic in try { ... } finally {
await dataSource.destroy(); } so any exceptions (e.g., in userRepository.save)
will still close the DB connection before process.exit(1).
In `@api/src/team/team.service.ts`:
- Around line 436-463: The sort logic mixes handling for "score" and
"buchholzPoints" and omits "buchholzPoints" from validSortColumns, producing a
misleading fallback for non-admins; add "buchholzPoints" to validSortColumns and
centralize the ordering logic around sortBy so flow is consistent: in the block
that checks validSortColumns, handle sortBy === "buchholzPoints" by calling
query.orderBy("team.buchholzPoints", direction) when revealAll is true, and when
revealAll is false either reject/skip the request (e.g., throw or return a
validation error) instead of falling back to revealed_match_wins, so you don't
accidentally make Buchholz behave identically to score; update references to
sortBy, validSortColumns, revealAll, and query.orderBy accordingly and remove
the separate trailing if (sortBy === "buchholzPoints") branch.
In `@frontend/app/events/`[id]/bracket/actions.tsx:
- Line 2: The Actions component currently imports and uses useSearchParams
(which requires a React Suspense boundary); wrap the Actions consumer in a
Suspense boundary at its call site so the static page build won't fail — locate
the place where Actions is rendered (the bracket page that conditionally renders
eventAdmin) and wrap that JSX with <Suspense fallback={null}>...</Suspense>
around <Actions />; keep the Actions component (which calls useSearchParams) as
a client component but ensure the parent render is suspended as shown.
In `@frontend/app/events/`[id]/groups/page.tsx:
- Around line 33-35: The page mixes two error-handling styles: isEventAdmin uses
handleError and returns a ServerActionResponse checked with isActionError, while
getSwissMatches, getTeamsForEventTable, and getTournamentTeamCount call
axiosInstance.get() directly and can throw uncaught errors; update those three
calls to use the same handleError wrapper (or wrap each axios call in try/catch
and return a ServerActionResponse error object) so they return the unified
ServerActionResponse shape, then guard each result with isActionError before
proceeding (references: getSwissMatches, getTeamsForEventTable,
getTournamentTeamCount, isEventAdmin, isActionError, handleError).
In `@frontend/components/match/MatchNode.tsx`:
- Around line 76-79: The component creates eventId from params (rawId) but
doesn't guard against an empty string, so calls like
router.push(`/events/${eventId}/teams/${team.id}`) can produce
`/events//teams/...`; update the navigation logic in MatchNode (where useRouter,
useParams, rawId, eventId and router.push are used) to check that eventId is a
non-empty string before calling router.push (or early-return/disable the click
when eventId is falsy), and ensure any existing guard (e.g., the if (team.id)
block) also includes a condition like `eventId` (or `eventId.length > 0`) to
prevent building malformed URLs.
---
Outside diff comments:
In `@api/src/match/match.controller.ts`:
- Around line 33-38: The controller is converting the query param
adminRevealQuery with Boolean(adminRevealQuery) which misinterprets strings like
"false"; change to an explicit string comparison (e.g., adminRevealQuery ===
"true") wherever this pattern is used (including the calls into
matchService.getSwissMatches and the other two usages that forward
adminRevealQuery), and update the parameter type from boolean to string (e.g.,
`@Query`("adminRevealQuery") adminRevealQuery: string) so the code accurately
reflects the received type before converting via the explicit comparison.
In `@api/src/match/match.service.ts`:
- Around line 791-812: The current calculateBuchholzPointsForTeam implementation
only queries won matches (winner: { id: teamId }) so it omits opponents from
lost/drawn matches; update it to mirror calculateBuchholzPointsForTeams by
querying all matches containing the team (e.g., remove the winner filter and use
where: { teams: { id: teamId } } or simply delegate to
calculateBuchholzPointsForTeams to compute the sum), then sum opponent.score for
every match rather than only wins; also update calculateBuchholzPoints (which
calls calculateBuchholzPointsForTeam) to use the batch
calculateBuchholzPointsForTeams path or the corrected single-team method so
persisted Buchholz values are correct.
In `@frontend/app/events/`[id]/dashboard/dashboard.tsx:
- Around line 652-713: Both cleanup buttons currently use
cleanupMatchesMutation.isPending causing either button to show a spinner
whenever any cleanup is running; scope the pending state to the phase by
checking the mutation's variables: update the SWISS button's spinner/disabled
logic to use cleanupMatchesMutation.isPending &&
cleanupMatchesMutation.variables === "SWISS" and update the Tournament button's
spinner/disabled logic to use cleanupMatchesMutation.isPending &&
cleanupMatchesMutation.variables === "ELIMINATION" so each button only shows
loading when its own mutate("SWISS") or mutate("ELIMINATION") call is active
(keep the onClick mutate arguments as-is).
---
Duplicate comments:
In `@AGENTS.md`:
- Around line 1-91: AGENTS.md is a duplicate of .agent/rules/website.md; remove
the duplication by deleting AGENTS.md (or replace its content with a short
pointer) and ensure .agent/rules/website.md remains the canonical source; update
any references or docs that point to AGENTS.md to reference
.agent/rules/website.md (search for references to "AGENTS.md" and change them to
".agent/rules/website.md") and run the repo search/lint to confirm no broken
links.
---
Nitpick comments:
In @.agent/rules/website.md:
- Around line 1-105: There’s a duplicate policy file: .agent/rules/website.md
replicates AGENTS.md; remove the duplication by either deleting
.agent/rules/website.md and replacing it with a one-line pointer to AGENTS.md or
convert .agent/rules/website.md into an include/wrapper that references
AGENTS.md (depending on agent framework support); update any internal references
to use AGENTS.md as the canonical source and ensure CI/docs links still resolve
(search for references to ".agent/rules/website.md" and update them to point to
"AGENTS.md" or the new include).
In `@api/src/match/match.controller.ts`:
- Around line 172-185: The route handler cleanupMatches casts the string phase
to MatchPhase via "as any" before calling matchService.cleanupMatchesInPhase;
replace this unsafe cast with proper enum validation: import the MatchPhase enum
and either use Nest's ParseEnumPipe on the `@Param` (e.g., `@Param`("phase", new
ParseEnumPipe(MatchPhase)) phase: MatchPhase) so the framework rejects invalid
phases automatically, or perform a manual check inside cleanupMatches (e.g., if
(!Object.values(MatchPhase).includes(phase as any)) throw BadRequestException)
and then call matchService.cleanupMatchesInPhase(eventId, phase) without any "as
any" cast; update the method signature and remove the cast to ensure
type-safety.
In `@api/src/team/team.service.ts`:
- Around line 407-426: The non-reveal branch of the query in team.service.ts
currently selects the raw "team.score" into the entity (inside the else branch
that builds the leftJoin on MatchEntity and the select([...]) array); remove
"team.score" from that select array so the persistent entity never carries the
real score when revealAll is false, relying instead on the computed fields
(e.g., the added "revealed_match_wins" addSelect and the later mapping logic
that sets the public/revealed score). Locate the else branch that calls
leftJoin(MatchEntity, "match", ...) and edit the select([...]) to omit
"team.score".
In `@frontend/app/actions/team.ts`:
- Around line 72-83: In getTeamById and getMyEventTeam (and the other
occurrences around 97-109) replace uses of the logical OR fallback (e.g., "repo
|| ''", "buchholzPoints || 0", "hadBye || false") with the nullish coalescing
operator (e.g., "repo ?? ''", "buchholzPoints ?? 0", "hadBye ?? false") so that
only null/undefined are replaced and legitimate falsy values like 0 or false are
preserved; mirror the approach already used in getTeamsForEventTable to keep
behavior consistent across functions.
In `@frontend/app/events/`[id]/bracket/actions.tsx:
- Around line 16-20: When the toggle is turned off, remove the adminReveal
search param instead of setting it to "false": in the onCheckedChange handler
use params.delete("adminReveal") when value is false and
params.set("adminReveal","true") when true; then call router.replace with either
`?${params.toString()}` when params is non-empty or the bare path (e.g.,
router.pathname) when empty to avoid a trailing empty query string. Target the
onCheckedChange closure that reads searchParams, manipulates params, and calls
router.replace.
In `@frontend/app/events/`[id]/bracket/graphView.tsx:
- Around line 142-148: The onClick handlers mix closure variables and the
callback parameter (e.g., reading match.state or placementMatch.state from the
outer scope while using clickedMatch.id), which is fragile; update both handlers
(the bracket onClick and the placement onClick) to consistently use the callback
parameter's state (clickedMatch.state) instead of the closure variables
(match.state or placementMatch.state) so the guard checks the same object you
receive from MatchNode; locate the onClick definitions in graphView.tsx and
replace uses of match.state and placementMatch.state with clickedMatch.state.
- Around line 42-218: The useEffect containing the bracket graph logic is too
large; extract pure helper functions to improve readability and testability by
moving placeholder generation, match-node construction (including handling
bracketMatches and placementMatch), and edge wiring into separate functions such
as buildPlaceholderNodes(matches, teamCount, VERTICAL_SPACING, ROUND_SPACING),
buildMatchNodes(matchesByRound, totalRounds, lastRoundIndex, placementMatch,
router, eventId, isEventAdmin) and buildEdges(nodeIdsByRound) that return arrays
and a nodeIdsByRound map; then have the useEffect call these helpers to produce
newNodes/newEdges and finally call setNodes/newEdges and setEdges, preserving
existing symbols (useEffect, nodeIdsByRound, newNodes, newEdges, placementMatch,
bracketMatches, setNodes, setEdges) and behavior.
- Around line 220-253: The fitViewOptions minZoom/maxZoom (minZoom: 0.2,
maxZoom: 1) differ from the global ReactFlow minZoom/maxZoom props (minZoom:
0.05, maxZoom: 1.5), which causes auto-fit to be limited while manual zoom
allows deeper zoom; either align the fitViewOptions to match the ReactFlow
minZoom/maxZoom or, if the narrower initial-fit range is intentional, add a
concise inline comment next to fitViewOptions in graphView.tsx explaining the
rationale so future readers don't "fix" the mismatch (refer to the ReactFlow
component props and the fitViewOptions object).
- Around line 61-71: The placeholderMatch object uses unsafe casts "as any" for
state and phase; replace those with the proper enum members from MatchState and
MatchPhase (e.g., MatchState.PLANNED and MatchPhase.ELIMINATION) and add
MatchPhase to the import list alongside MatchState (import { MatchState,
MatchPhase } from "@/app/actions/tournament-model"); update the placeholderMatch
fields state and phase to use the enum values rather than casts.
- Around line 25-35: GraphView currently declares an unused prop isAdminView;
remove isAdminView from the GraphView component signature (the exported function
GraphView and its props type) and update any place that instantiates GraphView
(callsites that pass isAdminView, e.g., the component usage in page.tsx) to stop
passing that prop so props match the new signature; ensure you only remove the
prop and its usage, leaving matches, teamCount, and isEventAdmin intact.
In `@frontend/app/events/`[id]/dashboard/dashboard.tsx:
- Around line 654-661: Replace the synchronous browser confirm() call inside the
onClick handler with a controlled destructive confirmation dialog from the app
UI library (e.g., AlertDialog): add local state (e.g., isDeleteDialogOpen /
setIsDeleteDialogOpen) and open the dialog when the button is clicked, render
the AlertDialog near the component currently containing the onClick, wire the
dialog's confirm action to call cleanupMatchesMutation.mutate("SWISS") and then
close the dialog, and wire the dialog's cancel/close action to simply close the
dialog; ensure the dialog is styled and accessible according to the app design
system and that any loading/error state from cleanupMatchesMutation is surfaced
in the dialog UI.
In `@frontend/app/events/`[id]/groups/graphView.tsx:
- Around line 138-143: Remove the redundant explicit prop assignment
variant={undefined} on the JSX Background component; simply omit the variant
prop so the component uses its default (BackgroundVariant.Dots). Update the
Background invocation in graphView.tsx (the Background component usage) by
deleting the variant attribute to avoid misleading readers and rely on the
default prop behavior.
In `@frontend/app/events/`[id]/groups/GroupPhaseTabs.tsx:
- Around line 34-38: The onTabChange handler currently uses router.push which
creates a new browser history entry on every tab change; update the function to
call router.replace instead of router.push so tab switches replace the current
history entry and don’t clutter the back-button navigation. Locate the
onTabChange function that builds params from searchParams and calls
router.push(`${pathname}?${params.toString()}`, { scroll: false }) and change
that call to router.replace while keeping the same URL and options (e.g.,
scroll: false).
In `@frontend/app/events/`[id]/groups/RankingTable.tsx:
- Around line 42-52: getMatchHistory iterates all matches for every team causing
O(N·M); instead precompute a Map<string, string[]> called historyByTeam by first
sorting matches by round (or iterating matches in round order) and in a single
pass pushing "W"/"L"/"T" into each involved team's array based on m.winner,
keyed by team.id, so history arrays preserve round order. Replace calls to
getMatchHistory(team.id) inside sortedTeams.map with historyByTeam.get(team.id)
?? [] (you can remove or keep getMatchHistory as a fallback). Ensure you
reference the existing matches array and maintain the same "W"/"L"/"T" semantics
when populating historyByTeam.
- Around line 44-45: Replace the string literal "FINISHED" in the RankingTable
filter callback with the enum value MatchState.FINISHED: update the predicate
(m) => m.state === "FINISHED" && m.teams.some((t) => t.id === teamId) to use
MatchState.FINISHED, and add the corresponding import for MatchState (same
source used by graphView.tsx and other components) so TypeScript can type-check
state comparisons.
In `@frontend/components/match/MatchNode.tsx`:
- Around line 85-89: The pure utility formatTeamName defined inside the
MatchNode component should be hoisted to module scope to avoid recreating it on
every render; move the function declaration for formatTeamName out of the
component (above the component export), keep its signature (teamName: string,
maxLength: number = 12), and export it if other modules need it; update any
references inside MatchNode to call the now-module-level formatTeamName and
remove the inner definition to reduce renders and allocations.
- Around line 7-8: The import change in MatchNode.tsx incorrectly suggests
migrating React Flow to v12; keep the current v11-compatible imports (useParams,
useRouter from next/navigation and Handle, Position from reactflow) and do not
swap to `@xyflow/react` or rename node props (e.g., xPos/yPos) here; if a
migration is desired, revert any edits in MatchNode.tsx that reference
`@xyflow/react` or v12-only APIs and instead add a short inline comment near the
imports (MatchNode.tsx) stating the code targets reactflow v11 and that a
coordinated, repo-wide v12 migration (updating node props, edge APIs, and
mutability patterns) should be planned as a separate task.
- Around line 105-120: The MatchNode component conditionally renders handles
(showTargetHandle/showSourceHandle) but doesn't notify React Flow when
visibility changes; import and call the useUpdateNodeInternals hook inside
MatchNode, get updateNodeInternals(node.id), and invoke it inside a useEffect
that depends on [showTargetHandle, showSourceHandle, node.id] so React Flow
recalculates handle positions when handles are added/removed; ensure you import
useEffect and useUpdateNodeInternals and reference the component's node id when
calling updateNodeInternals.
| @Get(":matchId") | ||
| async getMatchById( | ||
| @Param("matchId", ParseUUIDPipe) matchId: string, | ||
| @UserId() userId: string, | ||
| @Query("adminRevealQuery") adminRevealQuery: boolean, | ||
| ): Promise<MatchEntity> { | ||
| return await this.matchService.getMatchById(matchId); | ||
| return await this.matchService.getMatchById( | ||
| matchId, | ||
| { | ||
| teams: { | ||
| event: true, | ||
| }, | ||
| }, | ||
| userId, | ||
| Boolean(adminRevealQuery), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing @UseGuards(JwtAuthGuard) — userId will always be undefined.
The @UserId() decorator requires JwtAuthGuard to populate request.user. Without the guard, userId will always be undefined, so the admin-reveal logic in the service (if (userId) { ... }) will never trigger. Admin reveal on this endpoint is effectively dead code.
Proposed fix
+ `@UseGuards`(JwtAuthGuard)
`@Get`(":matchId")
async getMatchById(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/match/match.controller.ts` around lines 187 - 203, The getMatchById
endpoint is missing the JwtAuthGuard so the `@UserId`() decorator will receive
undefined; add the JwtAuthGuard to this route (e.g., annotate the getMatchById
handler with `@UseGuards`(JwtAuthGuard) or apply JwtAuthGuard at the controller
level) and import JwtAuthGuard from the auth module so request.user is populated
and the userId-based admin reveal logic in matchService.getMatchById works as
intended; keep the existing `@UserId`() parameter and Boolean(adminRevealQuery)
handling unchanged.
| bootstrap().catch((err) => { | ||
| console.error("Error seeding data:", err); | ||
| process.exit(1); |
There was a problem hiding this comment.
dataSource.destroy() not called in the outer error handler.
The event not found path (lines 48–50) calls dataSource.destroy() before exiting, but the outer .catch() does not. Any error thrown after the database is initialized (e.g., a failed userRepository.save) will leave the connection un-closed before process.exit(1).
While the OS reclaims TCP connections after the process exits, it's inconsistent with the explicit cleanup elsewhere and can cause confusing log noise from the DB server.
🐛 Proposed fix — use a try/finally in bootstrap
async function bootstrap() {
+ let dataSource: DataSource | undefined;
// ...
- await dataSource.initialize();
+ dataSource = new DataSource({ ... } as DataSourceOptions);
+ await dataSource.initialize();
// ...
- await dataSource.destroy();
}
bootstrap().catch((err) => {
console.error("Error seeding data:", err);
process.exit(1);
});A cleaner pattern wraps the post-initialize() work in try/finally:
await dataSource.initialize();
try {
// ... all seeding logic ...
console.log("Seeding completed successfully!");
} finally {
await dataSource.destroy();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/scripts/seed-users-teams.ts` around lines 105 - 107, The outer .catch
on bootstrap does not ensure the initialized DataSource is destroyed; update
bootstrap() (the function that calls dataSource.initialize() and runs seeding
logic) to wrap all post-initialize work in a try/finally so dataSource.destroy()
is always awaited (or, alternatively, ensure the outer error handler calls
dataSource.destroy() if dataSource was initialized). Specifically, locate the
dataSource.initialize() and surrounding seeding logic in bootstrap(), enclose
the seeding logic in try { ... } finally { await dataSource.destroy(); } so any
exceptions (e.g., in userRepository.save) will still close the DB connection
before process.exit(1).
| if (sortBy) { | ||
| const direction = searchDir?.toUpperCase() === "DESC" ? "DESC" : "ASC"; | ||
| let sortColumn = sortBy; | ||
|
|
||
| const validSortColumns = [ | ||
| "name", | ||
| "locked", | ||
| "repo", | ||
| "score", | ||
| "queueScore", | ||
| "createdAt", | ||
| "updatedAt", | ||
| ]; | ||
| if (validSortColumns.includes(sortBy)) { | ||
| query.orderBy(`team.${sortBy}`, direction as "ASC" | "DESC"); | ||
|
|
||
| if (validSortColumns.includes(sortColumn)) { | ||
| if (!revealAll && sortColumn === "score") { | ||
| query.orderBy("revealed_match_wins", direction as "ASC" | "DESC"); | ||
| } else { | ||
| query.orderBy(`team.${sortColumn}`, direction as "ASC" | "DESC"); | ||
| } | ||
| } | ||
|
|
||
| if (sortBy === "buchholzPoints") { | ||
| if (revealAll) { | ||
| query.orderBy("team.buchholzPoints", direction as "ASC" | "DESC"); | ||
| } else { | ||
| query.orderBy("revealed_match_wins", direction as "ASC" | "DESC"); | ||
| } | ||
| } |
There was a problem hiding this comment.
"buchholzPoints" and "score" are handled outside the validSortColumns whitelist, making the flow hard to follow and the fallback sort for non-admin buchholzPoints questionable.
"buchholzPoints" is not in validSortColumns (line 440), so the first if block (line 449) is skipped, and it falls through to a separate if on line 457. Additionally, "score" is in the whitelist but gets special-cased inside. The "buchholzPoints" non-reveal fallback (line 461) sorts by revealed_match_wins, which is the same as the "score" non-reveal fallback — so a user sorting by Buchholz will get the same ordering as sorting by score, which is misleading.
Consider unifying the sort logic, adding "buchholzPoints" to the validated set, and providing a meaningful non-admin Buchholz sort (or disallowing it for non-admins).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/team/team.service.ts` around lines 436 - 463, The sort logic mixes
handling for "score" and "buchholzPoints" and omits "buchholzPoints" from
validSortColumns, producing a misleading fallback for non-admins; add
"buchholzPoints" to validSortColumns and centralize the ordering logic around
sortBy so flow is consistent: in the block that checks validSortColumns, handle
sortBy === "buchholzPoints" by calling query.orderBy("team.buchholzPoints",
direction) when revealAll is true, and when revealAll is false either
reject/skip the request (e.g., throw or return a validation error) instead of
falling back to revealed_match_wins, so you don't accidentally make Buchholz
behave identically to score; update references to sortBy, validSortColumns,
revealAll, and query.orderBy accordingly and remove the separate trailing if
(sortBy === "buchholzPoints") branch.
| @@ -1,5 +1,30 @@ | |||
| "use client"; | |||
| import { useRouter, useSearchParams } from "next/navigation"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e tsx 'page' --search-path frontend/app/eventsRepository: 42core-team/website
Length of output: 557
🏁 Script executed:
cat -n frontend/app/events/[id]/bracket/page.tsxRepository: 42core-team/website
Length of output: 2654
🏁 Script executed:
cat frontend/package.json | jq '.dependencies | select(.next)'Repository: 42core-team/website
Length of output: 1808
🏁 Script executed:
grep -n "useSearchParams" frontend/app/events/[id]/bracket/actions.tsxRepository: 42core-team/website
Length of output: 170
useSearchParams() requires a <Suspense> boundary — production build will fail on static routes.
During production builds, a static page that calls useSearchParams from a Client Component must be wrapped in a Suspense boundary, otherwise the build fails with the Missing Suspense boundary with useSearchParams error. Reading search parameters through useSearchParams() without a Suspense boundary will opt the entire page into client-side rendering, which could cause your page to be blank until the client-side JavaScript has loaded.
The bracket page (frontend/app/events/[id]/bracket/page.tsx) is statically rendered, and the Actions component must be wrapped in <Suspense> at the call site:
Example fix
// In frontend/app/events/[id]/bracket/page.tsx
import { Suspense } from "react";
import Actions from "@/app/events/[id]/bracket/actions";
// ...
{eventAdmin && (
<div className="flex-shrink-0">
<Suspense fallback={null}>
<Actions />
</Suspense>
</div>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/events/`[id]/bracket/actions.tsx at line 2, The Actions
component currently imports and uses useSearchParams (which requires a React
Suspense boundary); wrap the Actions consumer in a Suspense boundary at its call
site so the static page build won't fail — locate the place where Actions is
rendered (the bracket page that conditionally renders eventAdmin) and wrap that
JSX with <Suspense fallback={null}>...</Suspense> around <Actions />; keep the
Actions component (which calls useSearchParams) as a client component but ensure
the parent render is suspended as shown.
| if (isActionError(eventAdmin)) { | ||
| throw new Error("Failed to verify admin status"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/app/events/[id]/groups/page.tsxRepository: 42core-team/website
Length of output: 2704
🏁 Script executed:
rg -n "getSwissMatches|getTeamsForEventTable|getTournamentTeamCount" --type ts -B 2 -A 10Repository: 42core-team/website
Length of output: 16276
🏁 Script executed:
cat -n frontend/app/actions/axios.tsRepository: 42core-team/website
Length of output: 1753
🏁 Script executed:
sed -n '207,250p' frontend/app/actions/team.tsRepository: 42core-team/website
Length of output: 1006
🏁 Script executed:
rg -n "isActionError" frontend/app/actions/errors.ts -A 5Repository: 42core-team/website
Length of output: 222
🏁 Script executed:
rg -n "export.*function isEventAdmin" frontend/app/actions/event.ts -A 10Repository: 42core-team/website
Length of output: 413
Inconsistent error handling patterns across fetched functions.
isEventAdmin uses the handleError wrapper and returns a ServerActionResponse (which can be checked with isActionError), but getSwissMatches, getTeamsForEventTable, and getTournamentTeamCount call axiosInstance.get() directly without error handling. On request failures, these functions would throw uncaught errors rather than return error objects, causing the page to crash. Consider applying the handleError pattern consistently across all four function calls for uniform error handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/events/`[id]/groups/page.tsx around lines 33 - 35, The page
mixes two error-handling styles: isEventAdmin uses handleError and returns a
ServerActionResponse checked with isActionError, while getSwissMatches,
getTeamsForEventTable, and getTournamentTeamCount call axiosInstance.get()
directly and can throw uncaught errors; update those three calls to use the same
handleError wrapper (or wrap each axios call in try/catch and return a
ServerActionResponse error object) so they return the unified
ServerActionResponse shape, then guard each result with isActionError before
proceeding (references: getSwissMatches, getTeamsForEventTable,
getTournamentTeamCount, isEventAdmin, isActionError, handleError).
| const router = useRouter(); | ||
| const params = useParams<{ id?: string }>(); | ||
| const rawId = params?.id; | ||
| const eventId = rawId ?? ""; |
There was a problem hiding this comment.
Missing guard for empty eventId produces malformed navigation URL
When params?.id is undefined (e.g., the component is rendered outside the /events/[id]/... route), eventId becomes "" and router.push(…) on Line 168 navigates to /events//teams/${team.id} — a malformed URL. The if (team.id) guard on Line 167 doesn't cover this case.
🛡️ Proposed fix
- const eventId = rawId ?? "";
+ const eventId = rawId ?? null;- if (team.id) {
+ if (team.id && eventId) {
router.push(`/events/${eventId}/teams/${team.id}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/components/match/MatchNode.tsx` around lines 76 - 79, The component
creates eventId from params (rawId) but doesn't guard against an empty string,
so calls like router.push(`/events/${eventId}/teams/${team.id}`) can produce
`/events//teams/...`; update the navigation logic in MatchNode (where useRouter,
useParams, rawId, eventId and router.push are used) to check that eventId is a
non-empty string before calling router.push (or early-return/disable the click
when eventId is falsy), and ensure any existing guard (e.g., the if (team.id)
block) also includes a condition like `eventId` (or `eventId.length > 0`) to
prevent building malformed URLs.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation