Round-aware multi-round cap leaderboard display - #475
Open
zacjones93 wants to merge 4 commits into
Open
Conversation
Contributor
WalkthroughThis PR extends multi-round time-with-cap scoring by adding a per-round breakdown display to competition leaderboards. The server API exposes fetched per-round cap status and values; client UI components render these breakdowns under aggregate scores; backfill diagnostics track scoreType fallback risks; seed data validates the flow; and ADR-0014 documents the design. ChangesRound-aware competition leaderboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f88c8244f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| client, | ||
| "scores", | ||
| scoredRows.map((row) => { | ||
| const value = aggregateScoreValue(row) as number |
There was a problem hiding this comment.
Use the captured row.scoreValue when inserting scores.score_value here. Recomputing from aggregateScoreValue(row) currently flattens many seeded rows (especially cappedRoundCount === 2) because roundValues() assigns both round values to TIME_CAP_MS, which makes multiple teams land on the same 36:00 aggregate and changes rank/sort behavior versus the MWFC snapshot this seeder is meant to reproduce.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
lat.md/domain.md (1)
157-158: 💤 Low valueConsider using wiki link syntax for ADR cross-references.
The coding guidelines specify using wiki link syntax
[[target]]or[[target|alias]]for cross-references. While the current Markdown link syntax works, consider whether ADR references should follow the wiki link pattern for consistency:-[ADR-0014](../docs/adr/0014-round-aware-multi-round-cap-scoring-followup.md) records the next design step: +[[../docs/adr/0014-round-aware-multi-round-cap-scoring-followup.md|ADR-0014]] records the next design step:The same pattern appears on lines 243 and 271.
As per coding guidelines: "Use wiki links (
[[target]]or[[target|alias]]) to cross-reference sections and source code inlat.md/files"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lat.md/domain.md` around lines 157 - 158, Replace the plain Markdown link to ADR-0014 with the repo's wiki-link style so ADR cross-references follow the coding guideline; specifically locate the existing Markdown link text referencing ADR-0014 (the “[ADR-0014](../docs/adr/0014-round-aware-multi-round-cap-scoring-followup.md)” instance) and change it to a wiki link form like [[docs/adr/0014-round-aware-multi-round-cap-scoring-followup|ADR-0014]] (and do the same for the other occurrences of the Markdown link pattern noted in the file), ensuring references to ADR-0014 and related symbols such as cappedRoundCount use the wiki-link syntax consistently.apps/wodsmith-start/scripts/seed/index.ts (1)
39-39: ⚡ Quick winAdd
//@lat:linkage for the new seeder wiring.Please add a single code-reference comment near this registration to tie it to the relevant
lat.mdsection for the round-breakdown seed flow.As per coding guidelines,
**/*.{ts,tsx,js,jsx,py,rs,go,c,h}: Add code reference comments (//@lat: [[section-id]]...) to tie source code to design concepts and test specifications in lat.md.Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-start/scripts/seed/index.ts` at line 39, Add a single code-reference comment linking this seeder to its lat.md section: next to the import and the place where seedMwfcRoundBreakdown is registered/used (symbol: seedMwfcRoundBreakdown) add a line comment in the form "// `@lat`: [[section-id]]" (replace [[section-id]] with the actual round-breakdown section id from lat.md) so both the import and the registration/usage are annotated.apps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.ts (1)
317-317: ⚡ Quick winAdd
//@lat:references on the seeder entry flow.Please add code-reference comments that map this exported seeder flow to the relevant
lat.mdsection(s).As per coding guidelines,
**/*.{ts,tsx,js,jsx,py,rs,go,c,h}: Add code reference comments (//@lat: [[section-id]]...) to tie source code to design concepts and test specifications in lat.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.ts` at line 317, Add code-reference comments linking this seeder's entry flow to the appropriate lat.md sections by inserting one or more lines like // `@lat`: [[section-id]] immediately above the exported seeder flow (the exported function seed) so the seeder entry is explicitly tied to the design/test spec; ensure the section IDs you reference map to the relevant lat.md topics for the MWFC round breakdown and add multiple // `@lat`: entries if the flow spans several sections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.ts`:
- Around line 240-267: The deleteSeedRows cleanup is incomplete: ensure you also
delete the seeded team_memberships, teams, and users (in that dependency order)
to avoid duplicate-ID conflicts on reruns. Update deleteSeedRows to execute
DELETE statements for team_memberships (WHERE team_id IN (...) OR user_id IN
(...)), then DELETE FROM teams WHERE id IN (...), and finally DELETE FROM users
WHERE id IN (...), passing the corresponding id arrays (e.g. derived from rows,
TEAM_IDS, USER_IDS or whatever variables hold inserted ids) as parameters and
keeping the existing pattern of client.execute; ensure you delete dependents
before parents (team_memberships → teams → users).
- Around line 323-586: Wrap the whole multi-table seeding sequence (starting at
deleteSeedRows(client) and including every subsequent batchInsert calls and the
scoredRows/video_submissions logic) in a single database transaction by using
the DB client's transaction API (e.g., client.transaction(async (tx) => { ... })
or db.transaction), replacing usages of client with the transaction handle (tx)
inside the block so deleteSeedRows(tx) and all batchInsert(tx, ...) run in the
same transaction, and ensure you commit on success and rollback on error
(propagate the error after rollback).
- Around line 16-34: Replace the declared type alias "SeedRow" with an interface
by changing "type SeedRow = { ... }" to "interface SeedRow { ... }" so the shape
follows the TS object-shape guideline (keep all property names and unions
identical); then add the required traceability comments "// `@lat`:
[[section-id]]" in this seeder file — place one directly above the new interface
declaration and any other top-level section comments (e.g., above the
dataset/rows or export) so the seeder contains visible // `@lat` entries for
traceability while preserving existing identifiers like SeedRow, registrationId,
userId, and the seeder export.
In `@apps/wodsmith-start/src/components/competition-leaderboard-table.tsx`:
- Around line 184-211: Add the missing design reference comment for the new
component by inserting a single-line `// `@lat`: [[section-id]]` comment
immediately above the RoundBreakdown function declaration (the function named
RoundBreakdown that takes a result prop of type
CompetitionLeaderboardEntry["eventResults"][number]). Ensure the section-id
matches the appropriate design/concept identifier per guidelines and place the
comment in the same file so the component is tied to its design spec.
In `@apps/wodsmith-start/src/components/online-competition-leaderboard-table.tsx`:
- Around line 221-248: RoundBreakdown is duplicated; extract it into a single
shared component (e.g., export function RoundBreakdown) that accepts result:
CompetitionLeaderboardEntry["eventResults"][number], add the required "`@lat`:
[[domain#Leaderboard#Round Breakdown Display]]" comment at the top of the new
file, and replace the local definitions in both
competition-leaderboard-table.tsx files with an import of the shared
RoundBreakdown; ensure the new component returns null when result.rawScore ===
null or result.rounds.length <= 1 and preserves the existing rendering logic for
round.status === "cap" and round.formatted/secondaryValue.
In `@apps/wodsmith-start/test/components/leaderboard-page-content.test.tsx`:
- Around line 229-296: Add a single `@lat`: specification reference comment
immediately adjacent to the test named "displays multi-round score breakdowns
under aggregate scores" (the it(...) block) — place the comment next to that
test (e.g., directly above the it(...) line), ensure there is exactly one `@lat`:
comment for this test and do not add duplicate `@lat`: comments elsewhere in the
file.
---
Nitpick comments:
In `@apps/wodsmith-start/scripts/seed/index.ts`:
- Line 39: Add a single code-reference comment linking this seeder to its lat.md
section: next to the import and the place where seedMwfcRoundBreakdown is
registered/used (symbol: seedMwfcRoundBreakdown) add a line comment in the form
"// `@lat`: [[section-id]]" (replace [[section-id]] with the actual
round-breakdown section id from lat.md) so both the import and the
registration/usage are annotated.
In `@apps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.ts`:
- Line 317: Add code-reference comments linking this seeder's entry flow to the
appropriate lat.md sections by inserting one or more lines like // `@lat`:
[[section-id]] immediately above the exported seeder flow (the exported function
seed) so the seeder entry is explicitly tied to the design/test spec; ensure the
section IDs you reference map to the relevant lat.md topics for the MWFC round
breakdown and add multiple // `@lat`: entries if the flow spans several sections.
In `@lat.md/domain.md`:
- Around line 157-158: Replace the plain Markdown link to ADR-0014 with the
repo's wiki-link style so ADR cross-references follow the coding guideline;
specifically locate the existing Markdown link text referencing ADR-0014 (the
“[ADR-0014](../docs/adr/0014-round-aware-multi-round-cap-scoring-followup.md)”
instance) and change it to a wiki link form like
[[docs/adr/0014-round-aware-multi-round-cap-scoring-followup|ADR-0014]] (and do
the same for the other occurrences of the Markdown link pattern noted in the
file), ensuring references to ADR-0014 and related symbols such as
cappedRoundCount use the wiki-link syntax consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 409f5e74-a360-4866-9584-18a324f94d99
⛔ Files ignored due to path filters (1)
lat.md/.cache/vectors.dbis excluded by!**/.cache/**,!**/*.db
📒 Files selected for processing (9)
apps/wodsmith-start/scripts/seed/index.tsapps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.tsapps/wodsmith-start/src/components/competition-leaderboard-table.tsxapps/wodsmith-start/src/components/online-competition-leaderboard-table.tsxapps/wodsmith-start/src/server-fns/competition-score-fns.tsapps/wodsmith-start/src/server/competition-leaderboard.tsapps/wodsmith-start/test/components/leaderboard-page-content.test.tsxdocs/adr/0014-round-aware-multi-round-cap-scoring-followup.mdlat.md/domain.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.ts">
<violation number="1" location="apps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.ts:493">
P1: `aggregateScoreValue(row)` returns `2 * TIME_CAP_MS` (2,160,000) for every row with `cappedRoundCount === 2` because `roundValues()` sets both round values to the flat cap. This collapses 8 distinct team scores into the same aggregate, breaking rank ordering against the source leaderboard snapshot. Use `row.scoreValue` directly to preserve the captured aggregate.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| client, | ||
| "scores", | ||
| scoredRows.map((row) => { | ||
| const value = aggregateScoreValue(row) as number |
Contributor
There was a problem hiding this comment.
P1: aggregateScoreValue(row) returns 2 * TIME_CAP_MS (2,160,000) for every row with cappedRoundCount === 2 because roundValues() sets both round values to the flat cap. This collapses 8 distinct team scores into the same aggregate, breaking rank ordering against the source leaderboard snapshot. Use row.scoreValue directly to preserve the captured aggregate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/scripts/seed/seeders/22-mwfc-round-breakdown.ts, line 493:
<comment>`aggregateScoreValue(row)` returns `2 * TIME_CAP_MS` (2,160,000) for every row with `cappedRoundCount === 2` because `roundValues()` sets both round values to the flat cap. This collapses 8 distinct team scores into the same aggregate, breaking rank ordering against the source leaderboard snapshot. Use `row.scoreValue` directly to preserve the captured aggregate.</comment>
<file context>
@@ -0,0 +1,591 @@
+ client,
+ "scores",
+ scoredRows.map((row) => {
+ const value = aggregateScoreValue(row) as number
+ const status = row.cappedRoundCount > 0 ? "cap" : "scored"
+ return {
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/wodsmith-start/src/lib/scoring/sort/compare.ts">
<violation number="1" location="apps/wodsmith-start/src/lib/scoring/sort/compare.ts:74">
P2: Comparator secondary tie-breaking no longer matches `computeSortKey`, which can produce different leaderboard orderings between in-memory and persisted sorting.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| const aSecondary = a.timeCap?.secondaryValue ?? 0 | ||
| const bSecondary = b.timeCap?.secondaryValue ?? 0 | ||
| const aSecondary = getCapSecondaryValue(a) |
Contributor
There was a problem hiding this comment.
P2: Comparator secondary tie-breaking no longer matches computeSortKey, which can produce different leaderboard orderings between in-memory and persisted sorting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/lib/scoring/sort/compare.ts, line 74:
<comment>Comparator secondary tie-breaking no longer matches `computeSortKey`, which can produce different leaderboard orderings between in-memory and persisted sorting.</comment>
<file context>
@@ -59,8 +71,8 @@ export function compareScores(a: Score, b: Score): number {
- const aSecondary = a.timeCap?.secondaryValue ?? 0
- const bSecondary = b.timeCap?.secondaryValue ?? 0
+ const aSecondary = getCapSecondaryValue(a)
+ const bSecondary = getCapSecondaryValue(b)
const secondaryDiff = bSecondary - aSecondary // Higher is better
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by cubic
Adds round-aware multi-round cap breakdowns to leaderboards and adds a new tiebreak: when cap count and aggregate time are equal, rank by total reps-at-cap. Aggregate remains the sum of round times; capped rounds show CAP with cap time and reps-at-cap.
timeCapor summed round reps).scoreTypefallback diagnostics with counts and per-score warnings.Written for commit 0374968. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes