Conversation
📝 WalkthroughWalkthroughThis PR adds a leaderboard widget displaying the top 10 players ranked by total points and derivens count. The feature includes a data model, database query function, handler integration to fetch leaderboard data, HTML template rendering with conditional display, and comprehensive CSS styling for the widget. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/repository/derive.go (1)
319-340: Consider indexing or caching this leaderboard query.
DerivenHandlernow executes it on every index request, but the available schema indexes only coverupload_logs(token_id, session_number)anduploaded_at. TheDISTINCT ON (player_name, derive_number) ... ORDER BY ... uploaded_at DESCpattern will get expensive asupload_logsgrows; a composite index on(player_name, derive_number, uploaded_at DESC)is the natural starting point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/derive.go` around lines 319 - 340, The leaderboard query in derive.go (executed by DerivenHandler) will become slow because it scans upload_logs with DISTINCT ON (player_name, derive_number) ordered by uploaded_at DESC; add a composite index on upload_logs(player_name, derive_number, uploaded_at DESC) to support that ordering (create it as CONCURRENTLY in a DB migration to avoid locking), and optionally add an index on deriven(number) or contributions(id) if not present; also consider adding a short-term cache layer (e.g., in-memory or Redis) for the DerivenHandler result to avoid running this heavy aggregation on every request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handlers/app/derive.go`:
- Around line 101-106: The leaderboard is fetched globally even when a
cityFilter is active; update the leaderboard retrieval in derive.go so it
respects the current cityFilter by passing the filter into
repository.GetLeaderboard (or call a repository method that accepts a city
parameter, e.g., GetLeaderboardByCity) using the existing cityFilter variable,
and handle nil/empty behavior consistently with the other queries;
alternatively, if you intend the leaderboard to remain global, change the
UI/logging around the leaderboard variable to explicitly label it as global so
it doesn't contradict the rest of the page.
In `@internal/repository/derive.go`:
- Around line 320-337: The query uses MAX(player_city) which returns the
alphabetically highest city instead of a deterministic/latest city; change the
query to pick player_city from the most recent upload row per player (by
uploaded_at) rather than aggregating with MAX. For example, replace the current
subquery/aggregation approach with one that selects per-player latest city
(using DISTINCT ON (ul.player_name) ORDER BY ul.player_name, ul.uploaded_at DESC
OR use ROW_NUMBER() OVER (PARTITION BY ul.player_name ORDER BY ul.uploaded_at
DESC) and filter row_number = 1) when producing player_city, then join that to
the derive aggregation (the parts using upload_logs ul, deriven d, contributions
c, derive_number and uploaded_at) so player_city is deterministic and based on
the latest upload.
- Around line 338-355: The SQL ordering is non-deterministic for ties so Rank
assignment in the loop (rows.Next(), scanning into models.LeaderboardEntry and
setting e.Rank) can flip between requests; modify the query ORDER BY clause
(used where rows is populated) to add a deterministic tiebreaker such as
player_id ASC or player_name ASC after unique_derives (e.g., ORDER BY
total_points DESC NULLS LAST, unique_derives DESC, player_name ASC), then ensure
the rows.Scan(...) in the loop still matches the selected columns for
models.LeaderboardEntry (PlayerName, PlayerCity, UniqueDerivens, TotalPoints) so
ranks are stable.
---
Nitpick comments:
In `@internal/repository/derive.go`:
- Around line 319-340: The leaderboard query in derive.go (executed by
DerivenHandler) will become slow because it scans upload_logs with DISTINCT ON
(player_name, derive_number) ordered by uploaded_at DESC; add a composite index
on upload_logs(player_name, derive_number, uploaded_at DESC) to support that
ordering (create it as CONCURRENTLY in a DB migration to avoid locking), and
optionally add an index on deriven(number) or contributions(id) if not present;
also consider adding a short-term cache layer (e.g., in-memory or Redis) for the
DerivenHandler result to avoid running this heavy aggregation on every request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0e1f31a-3471-4458-b3e1-86dfbb546293
📒 Files selected for processing (5)
internal/handlers/app/derive.gointernal/models/models.gointernal/repository/derive.goweb/static/style.cssweb/templates/app/deriven.html
| // Fetch leaderboard (top 10 players) | ||
| leaderboard, err := repository.GetLeaderboard(context.Background(), 10) | ||
| if err != nil { | ||
| log.Printf("Leaderboard error: %v", err) | ||
| leaderboard = nil | ||
| } |
There was a problem hiding this comment.
Clarify or scope the leaderboard when a city filter is active.
This handler already threads cityFilter into the count and ID list, but the leaderboard is always fetched globally. With ?city=..., the new widget will contradict the rest of the page unless it is also filtered or explicitly labeled as global.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/app/derive.go` around lines 101 - 106, The leaderboard is
fetched globally even when a cityFilter is active; update the leaderboard
retrieval in derive.go so it respects the current cityFilter by passing the
filter into repository.GetLeaderboard (or call a repository method that accepts
a city parameter, e.g., GetLeaderboardByCity) using the existing cityFilter
variable, and handle nil/empty behavior consistently with the other queries;
alternatively, if you intend the leaderboard to remain global, change the
UI/logging around the leaderboard variable to explicitly label it as global so
it doesn't contradict the rest of the page.
| SELECT | ||
| player_name, | ||
| COALESCE(MAX(player_city), '') AS player_city, | ||
| COUNT(*) AS unique_derives, | ||
| COALESCE(SUM(derive_points), 0) AS total_points | ||
| FROM ( | ||
| SELECT DISTINCT ON (ul.player_name, ul.derive_number) | ||
| ul.player_name, | ||
| ul.derive_number, | ||
| d.points AS derive_points, | ||
| COALESCE(c.user_city, '') AS player_city | ||
| FROM upload_logs ul | ||
| JOIN deriven d ON d.number = ul.derive_number | ||
| LEFT JOIN contributions c ON c.id = ul.contribution_id | ||
| WHERE ul.player_name IS NOT NULL AND ul.player_name != '' | ||
| ORDER BY ul.player_name, ul.derive_number, ul.uploaded_at DESC | ||
| ) sub | ||
| GROUP BY player_name |
There was a problem hiding this comment.
MAX(player_city) does not pick a meaningful city.
This returns the alphabetically highest city across a player's rows, not their latest or canonical one. As soon as someone uploads from multiple cities or fixes a typo, the leaderboard can show the wrong location. Pick the city from a deterministic row instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repository/derive.go` around lines 320 - 337, The query uses
MAX(player_city) which returns the alphabetically highest city instead of a
deterministic/latest city; change the query to pick player_city from the most
recent upload row per player (by uploaded_at) rather than aggregating with MAX.
For example, replace the current subquery/aggregation approach with one that
selects per-player latest city (using DISTINCT ON (ul.player_name) ORDER BY
ul.player_name, ul.uploaded_at DESC OR use ROW_NUMBER() OVER (PARTITION BY
ul.player_name ORDER BY ul.uploaded_at DESC) and filter row_number = 1) when
producing player_city, then join that to the derive aggregation (the parts using
upload_logs ul, deriven d, contributions c, derive_number and uploaded_at) so
player_city is deterministic and based on the latest upload.
| ORDER BY total_points DESC NULLS LAST, unique_derives DESC | ||
| LIMIT $1 | ||
| `, limit) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var entries []models.LeaderboardEntry | ||
| rank := 1 | ||
| for rows.Next() { | ||
| var e models.LeaderboardEntry | ||
| if err := rows.Scan(&e.PlayerName, &e.PlayerCity, &e.UniqueDerivens, &e.TotalPoints); err != nil { | ||
| return nil, err | ||
| } | ||
| e.Rank = rank | ||
| rank++ | ||
| entries = append(entries, e) |
There was a problem hiding this comment.
Add a deterministic tiebreaker for equal scores.
Players tied on total_points and unique_derives can come back in either order, and Rank is assigned from that result order on Lines 347-354. That makes tied players flip ranks between requests even when the data has not changed.
Suggested query tweak
- ORDER BY total_points DESC NULLS LAST, unique_derives DESC
+ ORDER BY total_points DESC NULLS LAST, unique_derives DESC, player_name ASC📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ORDER BY total_points DESC NULLS LAST, unique_derives DESC | |
| LIMIT $1 | |
| `, limit) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var entries []models.LeaderboardEntry | |
| rank := 1 | |
| for rows.Next() { | |
| var e models.LeaderboardEntry | |
| if err := rows.Scan(&e.PlayerName, &e.PlayerCity, &e.UniqueDerivens, &e.TotalPoints); err != nil { | |
| return nil, err | |
| } | |
| e.Rank = rank | |
| rank++ | |
| entries = append(entries, e) | |
| ORDER BY total_points DESC NULLS LAST, unique_derives DESC, player_name ASC | |
| LIMIT $1 | |
| `, limit) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var entries []models.LeaderboardEntry | |
| rank := 1 | |
| for rows.Next() { | |
| var e models.LeaderboardEntry | |
| if err := rows.Scan(&e.PlayerName, &e.PlayerCity, &e.UniqueDerivens, &e.TotalPoints); err != nil { | |
| return nil, err | |
| } | |
| e.Rank = rank | |
| rank++ | |
| entries = append(entries, e) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repository/derive.go` around lines 338 - 355, The SQL ordering is
non-deterministic for ties so Rank assignment in the loop (rows.Next(), scanning
into models.LeaderboardEntry and setting e.Rank) can flip between requests;
modify the query ORDER BY clause (used where rows is populated) to add a
deterministic tiebreaker such as player_id ASC or player_name ASC after
unique_derives (e.g., ORDER BY total_points DESC NULLS LAST, unique_derives
DESC, player_name ASC), then ensure the rows.Scan(...) in the loop still matches
the selected columns for models.LeaderboardEntry (PlayerName, PlayerCity,
UniqueDerivens, TotalPoints) so ranks are stable.
dmnktoe
left a comment
dmnktoe
left a comment
There was a problem hiding this comment.
Review: Leaderboard
Der schwierige Teil — die DISTINCT ON-Subquery, damit Mehrfach-Uploads auf dieselbe Derive nur einmal zählen — ist richtig gelöst, und points_indicator_1..3.png liegen tatsächlich in web/static/assets/images/ui/, die Top-3-Bilder greifen also. Ein paar Sachen würde ich vor dem Merge noch anfassen.
🟠 Korrektheit
1. COALESCE(MAX(player_city), '') liefert die alphabetisch grösste Stadt, nicht die richtige
Wer aus Berlin und aus Zwickau hochgeladen hat, steht im Leaderboard als „Zwickau" — nicht weil das der letzte Upload war, sondern weil Z > B. Die Subquery sortiert bereits nach ul.uploaded_at DESC; wenn die jüngste Stadt gemeint ist, muss die Information mit rausgetragen werden, z.B. (array_agg(player_city ORDER BY uploaded_at DESC))[1] oder ein Window-Function-Ansatz.
2. rows.Err() fehlt
GetLeaderboard prüft nach der Schleife nicht rows.Err(). Die fünf anderen iterierenden Funktionen in derselben Datei (internal/repository/derive.go:31,109,162,188,358) tun das. Ein Lesefehler mitten drin gibt aktuell eine stillschweigend gekürzte Liste zurück.
3. Gleichstände bekommen willkürlich verschiedene Ränge
rank++ pro Zeile heisst: zwei Leute mit 40 Punkten werden #3 und #4, und wer davon #3 wird, entscheidet die Sortierreihenfolge der DB. Bei einem Top-10-Board über eine überschaubare Teilnehmerzahl sind Gleichstände eher die Regel als die Ausnahme — RANK() OVER (ORDER BY total_points DESC) wäre ehrlicher.
4. ORDER BY total_points DESC NULLS LAST — total_points ist COALESCE(SUM(...), 0) und damit nie NULL. NULLS LAST ist toter Code und führt beim nächsten Lesen auf die falsche Fährte.
🟡 Konsistenz
5. internal/models/models.go ist nicht gofmt-formatiert
UniqueDerivens int
TotalPoints intUniqueDerivens sprengt das Alignment des Structs. Die CI hat keinen fmt-Check, es rutscht also durch — trotzdem einmal gofmt -w drüber.
6. UniqueDerivens ist doppelt plural. Im Rest der Codebase ist „Deriven" bereits der Plural von „Derive". UniqueDeriven oder DeriveCount fügt sich besser ein.
7. Der Fehlerpfad erreicht Sentry nicht. log.Printf + leaderboard = nil versteckt das Widget lautlos — bei {{if .Leaderboard}} sieht niemand, dass etwas kaputt ist. internal/sentryhelper wird genau dafür an anderen Stellen benutzt (#143 macht es so). Ein sentryhelper.CaptureException(c, err) dazu.
🟡 Performance
8. Die Query läuft bei jedem Aufruf von /, auch auf Seite 7 der Pagination, wo das Ergebnis identisch ist. DISTINCT ON + Join + Group + Sort ist nicht gratis. Entweder ein kurzer In-Process-Cache (das Leaderboard muss nicht sekundenaktuell sein), oder zumindest prüfen, dass Indizes auf upload_logs(player_name, derive_number, uploaded_at) und deriven(number) existieren.
Merge-Reihenfolge
#143, #144 und #145 hängen alle an a0d0d923 und überschneiden sich: alle drei hängen unten an web/static/style.css an, #144 und #145 ändern beide internal/handlers/app/derive.go und web/templates/app/deriven.html. Am wenigsten Konflikte gibt es bei #145 → #144 → #143, jeweils mit Rebase dazwischen.
Generated by Claude Code
1 participant
Summary by CodeRabbit