refactor: solid leaderboards (@fehmer, @miodec)#7485
Conversation
fehmer
force-pushed
the
feature/solid-leaderboards
branch
from
1498b5e to
2faf95b
Compare
fehmer
force-pushed
the
feature/solid-leaderboards
branch
from
532da70 to
471ed1a
Compare
github-actions
bot
added
the
waiting for review
| if (params.page !== undefined) { | ||
| setPage(params.page - 1); | ||
| } |
There was a problem hiding this comment.
setPage(params.page - 1) can still produce negative / non-integer pages from user-edited URLs (schema allows any number). Clamp to >= 0 and preferably coerce+validate in LeaderboardUrlParamsSchema (e.g., .int().min(1)) so invalid URLs don’t trigger invalid API requests.
| export const LeaderboardUrlParamsSchema = z | ||
| .object({ | ||
| type: z.enum(["allTime", "daily", "weekly"]), | ||
| mode: ModeSchema.optional(), | ||
| mode2: z.string().optional(), | ||
| language: LanguageSchema.optional(), | ||
| yesterday: z.boolean().optional(), | ||
| lastWeek: z.boolean().optional(), | ||
| friendsOnly: z.boolean().optional(), | ||
| page: z.number().optional(), | ||
| goToUserPage: z.boolean().optional(), | ||
| }) |
There was a problem hiding this comment.
LeaderboardUrlParamsSchema.page is z.number().optional(), which accepts negatives/decimals. Since this is parsed directly from the URL, constrain it (e.g., .int().min(1)) and consider coercion if needed so readGetParameters can safely convert it to a 0-based index.
| if (response.body.data.entries.length === 0 && options.page !== 0) { | ||
| setPage(Math.floor(response.body.data.count / pageSize)); | ||
| } |
There was a problem hiding this comment.
When requesting a page beyond the end, entries.length === 0 triggers setPage(Math.floor(count / pageSize)). This is off-by-one when count is an exact multiple of pageSize (e.g. 100/50 => 2 but last valid page index is 1). Compute the last valid 0-based page with Math.max(0, Math.ceil(count / pageSize) - 1) (or Math.floor((count - 1) / pageSize) when count>0).
| mode: undefined, | ||
| mode2: undefined, | ||
| language: undefined, | ||
| previous: false, |
There was a problem hiding this comment.
normalizeSelection() always forces previous: false for weekly. This means changing any weekly option via the sidebar (e.g. toggling friends-only) will silently reset the user back to “this week” even if they selected “last week”. Preserve draft.previous (or default it) instead of hard-resetting it.
| previous: false, | |
| previous: draft.previous ?? false, |
| } | ||
| }); | ||
|
|
||
| setSelection({ ...getSelection(), ...newSelection } as Selection); | ||
|
|
There was a problem hiding this comment.
getSelection() is used but not imported from ../stores/leaderboard-selection, causing a compile error. Import it (or avoid needing it) before calling setSelection({...getSelection(), ...}).
| let newSelection: Partial<Selection> = { | ||
| type: params.type, | ||
| friendsOnly: params.friendsOnly ?? false, | ||
| }; | ||
|
|
||
| if (params.type === "weekly") { | ||
| newSelection.previous = params.lastWeek ?? false; | ||
| } else { | ||
| newSelection.mode = params.mode ?? "time"; | ||
| newSelection.mode2 = params.mode2 ?? "15"; | ||
| newSelection.language = params.language ?? "english"; | ||
| newSelection.previous = | ||
| (params.type === "daily" && params.yesterday) ?? false; | ||
| } | ||
| }); | ||
|
|
||
| setSelection({ ...getSelection(), ...newSelection } as Selection); | ||
|
|
There was a problem hiding this comment.
When params.type === "weekly", newSelection is merged with getSelection(). If the current selection is a speed leaderboard, this merge keeps mode/mode2/language as strings, which fails SelectionSchema for weekly (z.never().optional()), so setSelection will reject the update. Build a valid weekly selection by explicitly clearing those fields (set to undefined or omit) before calling setSelection (or avoid merging for weekly).
No description provided.