#50 Enable list submission (PDF upload)#300
Merged
ianpaschal merged 17 commits intomainfrom Feb 24, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds list submission infrastructure across backend and frontend. It introduces a new listComments table and APIs (createListComment), implements full CRUD for lists with permission checks and comment deepening, adds file utilities including image-to-PDF conversion and file download actions, and expands schema and indices. It removes FlamesOfWarV4-specific model code, adds tournament-related helpers and actions for creating lists, and supplies numerous React components, hooks, styles, and service bindings for list viewing, uploading, commenting, and management. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
ianpaschal
force-pushed
the
50-enable-list-submission
branch
from
ebdcc5e to
9251f08
Compare
ianpaschal
force-pushed
the
50-enable-list-submission
branch
from
43abbef to
d979118
Compare
ianpaschal
force-pushed
the
50-enable-list-submission
branch
from
ee458a5 to
ff56b43
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 88
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts (4)
74-76:⚠️ Potential issue | 🔴 CriticalRuntime crash when all match results lack
detailsIf every
matchResultis skipped by the!detailsguard (line 36),rowsis empty androws[0]isundefined, causingObject.keys(undefined)to throw aTypeError.🐛 Proposed fix — guard empty rows
- // 3. Convert to CSV - const csv = [ - Object.keys(rows[0]).join(','), // Header - ...rows.map((row) => Object.values(row).join(',')), - ].join('\n'); + // 3. Convert to CSV + if (rows.length === 0) { + return null; + } + const csv = [ + Object.keys(rows[0]).join(','), // Header + ...rows.map((row) => Object.values(row).join(',')), + ].join('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` around lines 74 - 76, The CSV assembly assumes rows[0] exists and crashes if rows is empty; update the export logic in exportFowV4TournamentMatchData so it guards for empty rows before accessing rows[0] — e.g., check rows.length and if zero return an empty string (or a header-only CSV) instead of calling Object.keys(rows[0]); modify the block that currently builds the CSV from Object.keys(rows[0]) and rows.map(...) to conditionally build/join only when rows.length > 0.
15-15:⚠️ Potential issue | 🟡 MinorTypo in comment: "Gater" → "Gather"
📝 Proposed fix
- // 1. Gater base data + // 1. Gather base data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` at line 15, Fix the typo in the comment inside exportFowV4TournamentMatchData (the line "// 1. Gater base data"): change "Gater" to "Gather" so the comment reads "// 1. Gather base data" to improve clarity.
73-76:⚠️ Potential issue | 🟠 MajorCSV values are not escaped — any comma, quote, or newline in a value will corrupt the output
playerUser?.displayName,details.mission, placeholder names, etc. are written verbatim. A display name like"Smith, John"or a mission name containing a comma will split across columns and break every downstream parser.🐛 Proposed fix — add a CSV cell escape helper
+ const escapeCell = (value: unknown): string => { + const str = value === undefined || value === null ? '' : String(value); + return /[",\n\r]/.test(str) ? `"${str.replace(/"/g, '""')}"` : str; + }; + // 3. Convert to CSV + if (rows.length === 0) { + return null; + } const csv = [ Object.keys(rows[0]).join(','), // Header - ...rows.map((row) => Object.values(row).join(',')), + ...rows.map((row) => Object.values(row).map(escapeCell).join(',')), ].join('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` around lines 73 - 76, The CSV generation currently joins raw values into the csv constant which breaks when values contain commas, quotes, or newlines; create a helper function (e.g., escapeCsv(value)) that converts null/undefined to empty string, wraps values containing commas, quotes, or newlines in double quotes, and doubles any internal double quotes, then use it when building the CSV: apply escapeCsv to each header and to each entry inside the rows.map(Object.values(row)...) call so Object.keys(rows[0]) and Object.values(row) are both escaped before joining with commas.
79-80:⚠️ Potential issue | 🟠 MajorEvery export permanently stores an orphaned blob — unbounded storage growth
ctx.storage.store(...)creates a new object in Convex file storage on every call with no deletion, expiry, or reference tracking. Repeated exports (e.g., per round) will accumulate unreachable blobs indefinitely.Consider either:
- Deleting the previously stored export blob before creating a new one (requires storing the
storageIdon the tournament document), or- Returning the CSV directly to the caller instead of going through storage, if the consumer can handle the string payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` around lines 79 - 80, The export currently calls ctx.storage.store(...) and leaves orphaned blobs; modify exportFowV4TournamentMatchData (the code around ctx.storage.store/getUrl and the storageId variable) to either (A) persist the previous storageId on the tournament document (e.g., tournament.exportStorageId), call ctx.storage.delete(previousStorageId) before creating the new blob and then save the new storageId back to the tournament, or (B) avoid file storage entirely and return the CSV string directly to the caller (remove ctx.storage.store/getUrl usage and return csv). Implement one of these two approaches consistently and ensure you reference and update the storageId handling where ctx.storage.store and ctx.storage.getUrl are used.convex/_model/utils/getTournamentRegistrationsCsv.ts (1)
27-29:⚠️ Potential issue | 🟠 MajorRemove unsafe CSV field escaping — values with commas or quotes will corrupt output.
The code concatenates user fields directly into a CSV string without RFC 4180-compliant quoting or escaping. If
givenNameorfamilyNamecontains a comma, newline, or double-quote, the CSV row will be malformed and unparseable by standard CSV readers.Wrap string fields in double-quotes and escape any internal quotes:
const escapeCsvField = (value: string | undefined) => { const safe = value ?? ''; return `"${safe.replace(/"/g, '""')}"`; }; const rows = users.map((u) => `${escapeCsvField(u.givenName)},${escapeCsvField(u.familyName)},${escapeCsvField(u.email)}` );Note:
getShallowUserthrows when a user is not found, so null-dereference is not a concern. The?? ''for optionalgivenNameandfamilyNamefields is appropriate;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/utils/getTournamentRegistrationsCsv.ts` around lines 27 - 29, The CSV builder in getTournamentRegistrationsCsv.ts is concatenating user fields directly (users from getShallowUser) which breaks RFC 4180 when values contain commas, newlines, or quotes; implement an escapeCsvField helper that converts undefined to '' and replaces internal double-quotes with two double-quotes, then wrap the result in double-quotes, and use that helper when composing each row for u.givenName, u.familyName and u.email to produce safe CSV rows before joining them into the output string.convex/_model/matchResults/queries/getMatchResults.ts (1)
16-27:⚠️ Potential issue | 🔴 Critical
args.filteris accepted but never applied — all callers receive unfiltered, unscoped results.The function body only uses
args.paginationOpts;args.filteris silently ignored. Any call site passingtournamentId,tournamentPairingId, ortournamentCompetitorIdwill receive the full unfilteredmatchResultscollection instead of the expected subset. This is a functional correctness bug.The query should chain
.filter()or.withIndex()conditions based on the provided filter fields before paginating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/matchResults/queries/getMatchResults.ts` around lines 16 - 27, getMatchResults currently ignores args.filter and always paginates the entire matchResults collection; update the query built in getMatchResults to apply filters (or withIndex) before calling .paginate by checking the fields on args.filter (e.g., tournamentId, tournamentPairingId, tournamentCompetitorId) and chaining .filter(...) or .withIndex(...) on ctx.db.query('matchResults') as appropriate, then call .order('desc').paginate(args.paginationOpts) and keep the existing Promise.all(results.page.map(item => deepenMatchResult(ctx, item))) behavior; use the existing function names getMatchResults, getMatchResultsArgs, deepenMatchResult and the matchResults collection to locate where to add the conditional filter logic.convex/_model/lists/_helpers/deepenList.ts (1)
19-19:⚠️ Potential issue | 🟡 MinorStale JSDoc
@param— saystournamentbut the parameter isdoc.Proposed fix
- * `@param` tournament - Raw list document + * `@param` doc - Raw list document🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/_helpers/deepenList.ts` at line 19, JSDoc for the function in deepenList.ts is stale: the `@param` documents a parameter named "tournament" but the function actually takes "doc"; update the JSDoc to reflect the real parameter name ("doc") and, if helpful, clarify its description (e.g., "Raw list document") next to the `@param` for the deepenList function so the docstring matches the function signature and intent.src/services/files.ts (1)
31-57:⚠️ Potential issue | 🔴 Critical
setIsLoading(false)is never reached on a successful upload — addingreturn storageIdbypasses it.Before this PR, the
tryblock had noreturnstatement, so execution would fall through tosetIsLoading(false)at line 56 after success. With the newreturn storageIdat line 46, the function exits immediately andsetIsLoading(false)is unreachable on the success path. Theloadingstate will be stuck attrueafter every successful upload.🐛 Proposed fix — move to `finally`
mutation: async (file: Blob) => { setIsLoading(true); try { const response = await fetch(await generateUploadUrl(), { method: 'POST', headers: { 'Content-Type': file.type }, body: file, }); const { storageId } = await response.json(); if (config?.successMessage) { toast.success(config.successMessage); } if (config?.onSuccess) { config.onSuccess(storageId, file); } + return storageId; } catch (error) { console.error(error); if (error instanceof Error) { toast.error('Error', { description: error.message }); } if (config?.onError) { config.onError(error); } - return storageId; + } finally { + setIsLoading(false); } - setIsLoading(false); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/files.ts` around lines 31 - 57, The mutation function currently returns inside the try block which prevents the trailing setIsLoading(false) from running on success; move the setIsLoading(false) call into a finally block (or add a try/catch/finally and call setIsLoading(false) in finally) so it always runs regardless of success or error, keep the existing await generateUploadUrl(), toast/config.onSuccess logic and return storageId from the try, and preserve config.onError handling in the catch so loading state is cleared in all paths.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…mates.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 63
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
convex/_model/matchResults/queries/getMatchResults.ts (1)
16-26:⚠️ Potential issue | 🔴 Critical
filteris accepted in the args but never applied — the query always returns all results unfiltered.
args.filteris declared ingetMatchResultsArgsand will be passed by callers, butgetMatchResultsignores it entirely and always executes a plain unfiltered paginated query. Every caller relying on filtering bytournamentId,tournamentPairingId, ortournamentCompetitorIdwill silently receive unfiltered results.🐛 Proposed fix (example using index-based filtering)
export const getMatchResults = async ( ctx: QueryCtx, args: Infer<typeof getMatchResultsArgs>, ): Promise<PaginationResult<DeepMatchResult>> => { - const results = await ctx.db.query('matchResults').order('desc').paginate(args.paginationOpts); + let q = ctx.db.query('matchResults'); + if (args.filter?.tournamentId) { + q = q.withIndex('by_tournamentId', (idx) => idx.eq('tournamentId', args.filter!.tournamentId!)); + } else if (args.filter?.tournamentPairingId) { + q = q.withIndex('by_tournamentPairingId', (idx) => idx.eq('tournamentPairingId', args.filter!.tournamentPairingId!)); + } else if (args.filter?.tournamentCompetitorId) { + q = q.withIndex('by_tournamentCompetitorId', (idx) => idx.eq('tournamentCompetitorId', args.filter!.tournamentCompetitorId!)); + } + const results = await q.order('desc').paginate(args.paginationOpts); return { ...results, page: await Promise.all(results.page.map( async (item) => await deepenMatchResult(ctx, item), )), }; };Note: The exact index names must match those defined on the
matchResultstable schema. Adjust index names accordingly, or use.filter()as a fallback if indexes are not yet defined (though.filter()performs a full table scan).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/matchResults/queries/getMatchResults.ts` around lines 16 - 26, getMatchResults ignores getMatchResultsArgs.filter and always returns unfiltered results; update getMatchResults to apply args.filter when present by using index-based queries on the matchResults collection (e.g., choose the appropriate index for tournamentId, tournamentPairingId or tournamentCompetitorId) before calling .paginate(args.paginationOpts), and keep the existing mapping that calls deepenMatchResult for each page item; ensure you reference getMatchResultsArgs.filter, the matchResults query, and paginationOpts so callers requesting filtered pages receive only the matching results.convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts (2)
15-15:⚠️ Potential issue | 🟡 MinorTypo in comment: "Gater" → "Gather"
✏️ Proposed fix
- // 1. Gater base data + // 1. Gather base data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` at line 15, Typo in the comment inside exportFowV4TournamentMatchData — change the comment text "Gater base data" to "Gather base data" so the heading is spelled correctly; update the comment near the top of exportFowV4TournamentMatchData.ts where the step comment appears.
73-76:⚠️ Potential issue | 🔴 CriticalCrash on empty
rows, plus unescaped CSV valuesTwo distinct bugs here:
Runtime crash when
rowsis empty —rows[0]isundefinedwhenmatchResultsis empty or all entries lackdetails.Object.keys(undefined)throws aTypeError. The function signature already modelsnullas a valid return for this case, so guard explicitly.Unescaped CSV values —
Object.values(row).join(',')emits raw field values with no RFC 4180 quoting. Fields likedisplayName,mission, andbattlePlancan contain commas, double-quotes, or newlines, silently corrupting every affected row in the export.🐛 Proposed fix
+ // Guard against no rows + if (rows.length === 0) { + return null; + } + + // RFC 4180 escaping helper + const escapeCell = (value: unknown): string => { + const str = String(value ?? ''); + if (str.includes(',') || str.includes('"') || str.includes('\n')) { + return `"${str.replace(/"/g, '""')}"`; + } + return str; + }; + // 3. Convert to CSV const csv = [ Object.keys(rows[0]).join(','), // Header - ...rows.map((row) => Object.values(row).join(',')), + ...rows.map((row) => Object.values(row).map(escapeCell).join(',')), ].join('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` around lines 73 - 76, Guard against empty match rows and properly escape CSV fields: if rows is empty/undefined return null (the function already models null) instead of calling Object.keys(rows[0]); replace the csv construction that uses Object.keys(rows[0]) and Object.values(row).join(',') with logic that first derives headers only when rows.length>0 and otherwise returns null, and serialize each cell using an RFC‑4180 escape helper (wrap fields in double quotes, double any internal quotes, and preserve newlines) before joining with commas; reference the variables/expressions rows, csv, Object.keys(rows[0]) and Object.values(row).join(',') to locate where to apply the guard and the escape routine.convex/_model/lists/_helpers/deepenList.ts (1)
19-19:⚠️ Potential issue | 🟡 MinorStale
@paramname in JSDoc —tournamentshould bedoc.The parameter was renamed but the JSDoc was not updated.
📝 Proposed fix
- * `@param` tournament - Raw list document + * `@param` doc - Raw list document🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/_helpers/deepenList.ts` at line 19, Update the JSDoc for the deepenList function in deepenList.ts to use the current parameter name `doc` instead of the stale `tournament`: replace the `@param tournament` tag with `@param doc` and ensure the brief description matches the actual parameter meaning (e.g., "Raw list document" or similar) so the JSDoc aligns with the function signature (`deepenList` and its `doc` parameter).
♻️ Duplicate comments (37)
convex/_model/matchResults/queries/getMatchResults.ts (1)
9-13:⚠️ Potential issue | 🔴 Critical
tournamentPairingIdandtournamentCompetitorIdstill validate against the wrong table.This was flagged in a previous review and remains unresolved. Convex encodes the table name into the ID and validates it at the boundary — passing a
tournamentPairingsortournamentCompetitorsID wherev.id('tournaments')is expected will throw a runtime validation error.🐛 Proposed fix
filter: v.optional(v.object({ tournamentId: v.optional(v.id('tournaments')), - tournamentPairingId: v.optional(v.id('tournaments')), - tournamentCompetitorId: v.optional(v.id('tournaments')), + tournamentPairingId: v.optional(v.id('tournamentPairings')), + tournamentCompetitorId: v.optional(v.id('tournamentCompetitors')), })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/matchResults/queries/getMatchResults.ts` around lines 9 - 13, The filter schema incorrectly validates tournamentPairingId and tournamentCompetitorId using v.id('tournaments'); update the filter object in getMatchResults so tournamentPairingId uses v.id('tournamentPairings') and tournamentCompetitorId uses v.id('tournamentCompetitors') (keep tournamentId as v.id('tournaments')), ensuring the Convex ID table names match the actual tables referenced..eslintrc.cjs (1)
52-52:⚠️ Potential issue | 🟠 MajorNew rule added using the deprecated core form.
implicit-arrow-linebreakwas deprecated in ESLint v8.53.0 and is removed in v11.0.0. All other arrow-related styling in this file already uses@stylistic/jsscoped rules.🔧 Proposed fix
- "implicit-arrow-linebreak": ["error", "beside"], + "@stylistic/js/implicit-arrow-linebreak": ["error", "beside"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.eslintrc.cjs at line 52, Replace the deprecated core rule implicit-arrow-linebreak with the scoped stylistic rule used elsewhere: remove the "implicit-arrow-linebreak": ["error","beside"] entry and add the equivalent "@stylistic/js/implicit-arrow-linebreak": ["error","beside"] in the same rules object so the project uses the `@stylistic/js` scoped rule for arrow formatting.convex/_model/utils/getTournamentRegistrationsCsv.ts (1)
32-33:⚠️ Potential issue | 🟡 Minor
u.emailstill missing?? ''fallback, and fields remain unescaped.This formatting-only change left both previously raised issues unresolved:
u.emailcan emit"undefined"in the CSV, and any field containing a comma, double-quote, or newline will produce malformed rows.🛡️ Proposed fix
+ const escapeField = (v: string) => `"${v.replace(/"/g, '""')}"`; + const header = 'givenName,familyName,email'; - const rows = users.map((u) => `${u.givenName ?? ''},${u.familyName ?? ''},${u.email}`, - ); + const rows = users.map((u) => + [u.givenName ?? '', u.familyName ?? '', u.email ?? ''].map(escapeField).join(','), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/utils/getTournamentRegistrationsCsv.ts` around lines 32 - 33, The rows mapping currently emits u.email without a fallback and does not escape CSV-special characters; update the users.map that produces rows so each field (u.givenName, u.familyName, u.email) uses a fallback like ?? '' and is passed through an escapeCsv helper that doubles any internal double-quotes and wraps the field in double-quotes (e.g., escapeCsv = (s) => `"` + String(s ?? '').replace(/"/g, '""') + `"`), then join the escaped fields with commas when creating rows; refer to the variables rows, users.map, u.givenName, u.familyName, and u.email to locate the change.convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts (2)
45-55: Unresolved: remove dead commented-out codeThe commented-out
playerListvariable and all four commented field assignments (force_diagram,faction,formation_0,formation_1) are still present. These should be deleted entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` around lines 45 - 55, Remove the dead commented-out code: delete the commented declaration of playerList and the four commented field entries for player_{letter}_force_diagram, player_{letter}_faction, player_{letter}_formation_0, and player_{letter}_formation_1 in exportFowV4TournamentMatchData (the block that references matchResult and playerList). Ensure no other code still references the removed playerList symbol and run a quick grep to confirm there are no leftover commented duplicates.
58-58: Unresolved: wrong type cast indexesmatchResultwithDetailsKey
DetailsKeyiskeyof typeof matchResult.details, but it's used to indexmatchResultdirectly. The cast should beGeneralKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` at line 58, The index cast is wrong: DetailsKey is for matchResult.details but the code indexes matchResult itself; update the cast on matchResult[`player${i}Score` as ...] to use GeneralKey (or the correct keyof for matchResult) instead of DetailsKey so [`player_${letter}_score`] pulls from matchResult properly; locate the expression with matchResult and the DetailsKey cast in exportFowV4TournamentMatchData and replace the cast to GeneralKey (or adjust the type alias used) to match the actual object being indexed.src/components/generic/Timestamp/Timestamp.tsx (1)
23-31: Clickable<span>still inaccessible to keyboard users.The toggle span has no
role,tabIndex, oronKeyDownhandler — users navigating by keyboard cannot discover or activate it. This was flagged in a previous review and remains unresolved.♿ Proposed fix
<span className={clsx(styles.Timestamp, ...getStyleClassNames({ variant: 'ghost', corners: 'normal', }), className)} onClick={handleClick} + onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') handleClick(); }} + role="button" + tabIndex={0} + aria-label={absolute ? 'Show relative time' : 'Show absolute time'} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/generic/Timestamp/Timestamp.tsx` around lines 23 - 31, The span in the Timestamp component is clickable but not keyboard-accessible; update the element (in Timestamp.tsx where the span with className using styles.Timestamp and getTimestampLabel is rendered and handleClick is used) to behave like a button by adding role="button", tabIndex={0}, and an onKeyDown handler that invokes handleClick when Enter or Space is pressed (and prevents default for Space); keep the existing onClick and className props intact so mouse behavior and styling remain unchanged.src/components/generic/Timestamp/Timestamp.utils.ts (1)
6-6:mmrenders minutes in the month position — absolute timestamps display incorrect dates.
date-fnstokens are case-sensitive:MM= month,mm= minutes. This was flagged in a previous review and remains unresolved.🐛 Proposed fix
- return format(date, 'yyyy-mm-dd hh:mm:ss aa'); + return format(date, 'yyyy-MM-dd hh:mm:ss aa');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/generic/Timestamp/Timestamp.utils.ts` at line 6, The format string uses the wrong token for month causing minutes to appear in the month position; update the format call in Timestamp.utils.ts (the line returning format(date, ...)) to use the correct date-fns month token by replacing the first "mm" with "MM" so the format becomes "yyyy-MM-dd hh:mm:ss aa" (leave the rest of the function and imports as-is).src/components/FileButton/FileButton.tsx (3)
15-15:handleChangeis still unnecessarilyasync.No
awaitexists in the body; thePromise<void>return type is spurious. This was raised in a prior review and remains unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FileButton/FileButton.tsx` at line 15, The handleChange function in FileButton.tsx is declared async and returns Promise<void> but contains no await; remove the async keyword and the Promise<void> return type to make it a synchronous handler (change the signature to const handleChange = (event: ChangeEvent<HTMLInputElement>): void => { ... } or simply const handleChange = (event: ChangeEvent<HTMLInputElement>) => { ... }); update any callers if they rely on a Promise return (they should not for an input change handler).
29-29:Object.keys(accept)on astring[]still produces index strings, not MIME types — file picker filtering is broken.
Object.keys(['image/png', '.pdf'])→['0','1'], so<input accept>receives'0,1'and the browser applies no filter. This was raised in a prior review and remains unresolved.🐛 Proposed fix
- accept={Object.keys(accept).join(',')} + accept={accept.join(',')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FileButton/FileButton.tsx` at line 29, The file picker is using Object.keys(accept) which yields index strings for a string[] and breaks filtering; update the FileButton component to build the accept string from the actual values: if the incoming accept prop is an array (e.g. ['image/png', '.pdf']) use accept.join(',') for the input accept attribute, otherwise (if accept can be an object/map) fall back to Object.keys(accept).join(',') — change the expression in FileButton (where accept={Object.keys(accept).join(',')}) accordingly so the input receives real MIME/extensions not index strings.
22-25:<Button>nested inside<label>is still present — invalid HTML that breaks keyboard file-picker activation.The HTML spec forbids interactive content (a
<button>) inside a<label>. Keyboard users pressing Enter/Space will trigger theButton's own click handler rather than the file input, silently breaking non-pointer file selection. This was raised in a prior review and remains unresolved. TheButtoncomponent already spreads...props(includingonClickviaButtonHTMLAttributes), so the ref-based pattern proposed previously is directly applicable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FileButton/FileButton.tsx` around lines 22 - 25, The Button must not be nested inside a label; change FileButton to render a hidden file <input> with a ref (e.g., inputRef) and render the Button separately (not wrapped by <label>), strip onClick from the spread props passed to Button and instead attach an onClick handler on Button that calls inputRef.current?.click(); keep the input's id and other file attributes intact and forward other props (except onClick) to Button so keyboard activation triggers the input via the ref rather than having an interactive <button> inside a <label> (refer to FileButton, Button, id, props, and the input ref).src/components/CommentThread/CommentThread.tsx (3)
4-4:⚠️ Potential issue | 🟠 MajorStill importing from
node_modules/.../dist/types— use~/types/componentLibinstead.
ElementIntentis already re-exported fromsrc/types/componentLib.ts(as confirmed by the relevant snippet). Thenode_modulesdist path couples this file to the package's internal build structure.♻️ Proposed fix
- import { ElementIntent } from 'node_modules/@ianpaschal/combat-command-components/dist/types'; + import { ElementIntent } from '~/types/componentLib';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CommentThread/CommentThread.tsx` at line 4, The import in CommentThread.tsx is pulling ElementIntent from the package dist path which couples to node_modules internals; update the import to use the project's re-export by replacing the current import of ElementIntent with an import from '~/types/componentLib' (keep the symbol name ElementIntent) so the file uses the canonical re-export and avoids the package build path.
43-45:⚠️ Potential issue | 🟡 MinorEmpty icon
<div>always rendered;clsxwith a single static class is redundant.When
comment.iconisundefined, an empty<div>is still emitted.clsxadds no value for a single compile-time-known class.♻️ Proposed fix
- <div className={clsx(styles.CommentThread_Icon)}> - {comment.icon} - </div> + {comment.icon && ( + <div className={styles.CommentThread_Icon}> + {comment.icon} + </div> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CommentThread/CommentThread.tsx` around lines 43 - 45, In the CommentThread component, avoid always rendering an empty wrapper and remove the pointless clsx call: only render the icon container when comment.icon is present and use the static class name directly; locate the JSX using the CommentThread_Icon class (the div currently written as <div className={clsx(styles.CommentThread_Icon)}>{comment.icon}</div>) and change it so the div is conditionally returned only when comment.icon is defined, and drop clsx in favor of styles.CommentThread_Icon.
40-40:⚠️ Potential issue | 🟡 Minor
styles.CommentThread_Authoris not defined inCommentThread.module.scss.The SCSS module only defines
CommentThread,CommentThread_Header,CommentThread_Icon, andCommentThread_Body. Accessing an undefined key results inundefinedat runtime, soIdentityBadgereceivesclassName={undefined}and no styles are applied.🛠️ Options
// Option A — add the missing class to the SCSS: + &_Author { + // author-specific styles + } // Option B — if no specific styles are needed, remove the unused prop: - <IdentityBadge className={styles.CommentThread_Author} user={comment.user} size="tiny" /> + <IdentityBadge user={comment.user} size="tiny" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CommentThread/CommentThread.tsx` at line 40, The IdentityBadge is being passed styles.CommentThread_Author which doesn't exist in the SCSS module; either add a CommentThread_Author class to CommentThread.module.scss or change the component to use an existing key (e.g., styles.CommentThread_Header or styles.CommentThread_Icon) so className is not undefined — update the JSX in CommentThread.tsx where IdentityBadge is rendered and/or add the new .CommentThread_Author rule in the SCSS module to apply the intended styles.src/components/CommentThread/CommentThreadEvent.tsx (2)
4-4:⚠️ Potential issue | 🟠 MajorSame fragile
node_modulesdist import — use~/types/componentLibinstead.Identical unresolved issue as in
CommentThread.tsx.♻️ Proposed fix
- import { ElementIntent } from 'node_modules/@ianpaschal/combat-command-components/dist/types'; + import { ElementIntent } from '~/types/componentLib';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CommentThread/CommentThreadEvent.tsx` at line 4, Replace the fragile direct node_modules import in CommentThreadEvent.tsx: stop importing ElementIntent from 'node_modules/@ianpaschal/combat-command-components/dist/types' and instead import it from the project alias module (use '~/types/componentLib'); update the import statement that references ElementIntent so it uses the aliased path to ensure the type resolves reliably.
12-12:⚠️ Potential issue | 🔴 Critical
replies?: Comment[]resolves to the DOM globalComment, not a list-comment type.There is no local
Commenttype in this file, so TypeScript resolves the identifier to the built-in DOMComment(CharacterDatasubtype for<!-- ... -->). This is a silent type error — the field would be typed as an array of HTML comment nodes instead of the intended reply shape.🐛 Proposed fix
type EventData = { _creationTime: number; - replies?: Comment[]; + replies?: EventData[]; // self-referential, or import a shared comment/reply type content: ReactNode; icon: ReactElement; intent?: ElementIntent; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CommentThread/CommentThreadEvent.tsx` at line 12, The field replies?: Comment[] is currently resolving to the DOM global Comment type; change it to the correct reply/comment model by importing or referencing the project type (e.g., import { Comment as ApiComment } from "path/to/types" or use the local Reply/CommentRecord type) and update the declaration to replies?: ApiComment[] (or replies?: Reply[]), or alternatively declare a local interface for the intended reply shape and use that instead; ensure the chosen symbol matches the actual comment/reply type used elsewhere in the codebase (search for existing types like CommentModel, CommentDTO, Reply) and update any other usages in CommentThreadEvent to use the same imported/aliased type.package.json (1)
61-61: The prior review already flaggedpdf-libas unmaintained (last release: November 2021) and recommended@pdfme/pdf-libas the actively maintained alternative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 61, The dependency "pdf-lib": "^1.17.1" is unmaintained; replace it with the actively maintained fork by removing the "pdf-lib" entry and adding "@pdfme/pdf-lib" in package.json, then run your package manager to install; update all import/require statements that reference "pdf-lib" to "@pdfme/pdf-lib" (search for any occurrences of "pdf-lib" in source files) and adjust any API differences per `@pdfme/pdf-lib` docs so existing calls (constructors/factory funcs like PDFDocument, methods used across your codebase) compile and tests pass.convex/_model/tournamentRegistrations/_helpers/getAvailableActions.ts (1)
81-83: This was flagged in a previous review:CreateListis the only action without atournament.statusguard. As-is, organizers, captains, and players can see "Submit List" on unpublished/draft tournaments. The fix proposed earlier was:- if (isOrganizer || isCaptain || isSelf) { + if (isOrganizer || ((isCaptain || isSelf) && tournament.status === 'published')) { actions.push(TournamentRegistrationActionKey.CreateList); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournamentRegistrations/_helpers/getAvailableActions.ts` around lines 81 - 83, The CreateList action is being pushed unguarded so organizers/captains/players can see "Submit List" on draft/unpublished tournaments; update the condition in getAvailableActions (around isOrganizer/isCaptain/isSelf) to only push TournamentRegistrationActionKey.CreateList when tournament.status indicates a published/active tournament (e.g., tournament.status === 'published' or not 'draft'/'unpublished'), i.e., change the if that currently checks isOrganizer || isCaptain || isSelf to also require the tournament.status guard before calling actions.push(TournamentRegistrationActionKey.CreateList).src/components/generic/InputTextArea/InputTextArea.scss (1)
23-23: This concern was already raised in a previous review:4remis too small for the tournament description field inGeneralFields, and withresize: noneset, users cannot expand it. The suggestion was to add a localmin-heightoverride inGeneralFields.module.scss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/generic/InputTextArea/InputTextArea.scss` at line 23, Add a local override in GeneralFields.module.scss for the tournament description field to increase its min-height and allow user resizing: target the description field rendered by the GeneralFields component (the tournament description input) and override the global InputTextArea min-height (e.g., set min-height: 8rem) and either remove or change resize: none to resize: vertical so users can expand the textarea; ensure the selector specifically targets the GeneralFields description element so it only affects that instance.convex/_model/files/actions/getFileDownloadUrl.ts (1)
1-12: File namegetFileDownloadUrl.tsstill mismatches its exported symbols (getFileDownloadData/getFileDownloadDataArgs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/files/actions/getFileDownloadUrl.ts` around lines 1 - 12, The file name getFileDownloadUrl.ts doesn't match the exported symbols (getFileDownloadData and getFileDownloadDataArgs); either rename the exports to getFileDownloadUrl/getFileDownloadUrlArgs or rename the file to getFileDownloadData.ts so names align. Update both the exported function/const identifiers (getFileDownloadData, getFileDownloadDataArgs) and any imports elsewhere, or instead rename the file and leave exports unchanged; ensure references to getFileDownloadData and getFileDownloadDataArgs remain consistent across the codebase.convex/_model/tournamentCompetitors/_helpers/checkUsersAreTeammates.ts (1)
12-17: Run both registration queries in parallel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournamentCompetitors/_helpers/checkUsersAreTeammates.ts` around lines 12 - 17, The two sequential DB queries (assigning player0Records and player1Records via ctx.db.query('tournamentRegistrations') withIndex calls) should be executed in parallel to reduce latency: start both queries without awaiting, then use Promise.all to await both results and destructure into player0Records and player1Records (preserve the same .withIndex('by_user', q => q.eq('userId', ...)).collect() calls and variable names so downstream logic in checkUsersAreTeammates still works).src/components/ListDetails/index.ts (1)
1-4: 🛠️ Refactor suggestion | 🟠 Major
ListDetailsButtonstill missing from the barrel.This was flagged in a previous review. The barrel exports
ListDetailsandListDetailsPropsbut notListDetailsButton, leaving consumers to import it via the internal path directly.♻️ Suggested addition
export { ListDetails, type ListDetailsProps, } from './ListDetails'; +export { + ListDetailsButton, + type ListDetailsButtonProps, +} from './ListDetailsButton';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListDetails/index.ts` around lines 1 - 4, Barrel export missing ListDetailsButton: update the export list in this module to also re-export the ListDetailsButton symbol from './ListDetails' so consumers can import it from the barrel; locate the existing export that re-exports ListDetails and ListDetailsProps and add ListDetailsButton to the same export statement (ensuring the identifier matches the named export in the ListDetails module).src/components/ContextMenu/ContextMenu.tsx (1)
3-3:⚠️ Potential issue | 🔴 CriticalHardcoded
node_modules/path still not fixed.This was flagged in a previous review. Line 3 still imports
ElementSizevia the internaldist/typespath rather than the public package entry, which breaks under Yarn PnP, pnpm strict mode, andexports-map enforcement. Line 1 already shows the correct approach.🔧 Proposed fix
-import { ElementSize } from 'node_modules/@ianpaschal/combat-command-components/dist/types'; +import type { ElementSize } from '@ianpaschal/combat-command-components';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ContextMenu/ContextMenu.tsx` at line 3, The import in ContextMenu.tsx is using a hardcoded node_modules path for ElementSize which breaks package resolution; update the import to use the package's public entry instead (import ElementSize from '@ianpaschal/combat-command-components' or the named export from that package) and remove the '/dist/types' node_modules path so ElementSize is imported the same way as other symbols in this file.src/components/ListProvider/actions/useDeleteAction.tsx (1)
15-22: 🧹 Nitpick | 🔵 Trivial
closereferenced before its declaration — reorder hooks for clarity.
close(Line 22) is captured by theonSuccessclosure passed touseDeleteListat Line 15. This works at runtime because the closure readscloseonly after the mutation resolves (async), but the visual dependency flows upward, making the code harder to reason about.♻️ Suggested reorder
+ const { open, close } = useDialogInstance(); + const { mutation: deleteList } = useDeleteList({ onSuccess: (): void => { toast.success('List deleted!'); close(); }, }); - - const { open, close } = useDialogInstance();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListProvider/actions/useDeleteAction.tsx` around lines 15 - 22, The onSuccess closure passed into useDeleteList captures close before close is declared, making the dependency order confusing; move the useDialogInstance() call above the useDeleteList(...) call so that const { open, close } = useDialogInstance() is declared before const { mutation: deleteList } = useDeleteList({ onSuccess: () => { toast.success('List deleted!'); close(); } }), ensuring close is defined when referenced in the onSuccess handler.src/pages/ListDetailPage/ListDetailPage.tsx (1)
13-22:⚠️ Potential issue | 🟡 MinorNo loading or not-found/access-denied feedback — page silently renders empty.
useGetListreturnsundefinedwhile loading andnullwhen the list doesn't exist or failscheckListVisibility. Both currently fall through to an emptyPageWrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ListDetailPage/ListDetailPage.tsx` around lines 13 - 22, The page currently renders an empty PageWrapper because useGetList (data: list) is undefined while loading and null for not-found/access-denied; update ListDetailPage to handle these states: show a loading indicator when list is undefined (e.g., render a Spinner or "Loading..." inside PageWrapper), render a clear Not Found / Access Denied message when list is null, and only render <Card><ListDetails list={list} /></Card> when list is a non-null object; reference the useGetList hook, the list variable (data: list), PageWrapper, ListDetails and Card when making the changes.src/components/ListProvider/ListContextMenu.tsx (1)
16-20:.filter(Boolean)was already recommended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListProvider/ListContextMenu.tsx` around lines 16 - 20, The code in ListContextMenu renders ContextMenu with an actions array filtered using .filter((action) => !!action); replace this with the concise .filter(Boolean) to remove falsy entries; update the actions passed into ContextMenu in the ListContextMenu component (the array containing actions[ListActionKey.Download], actions[ListActionKey.Manage], actions[ListActionKey.Delete]) to use .filter(Boolean).src/components/generic/Warning/Warning.module.scss (1)
28-36:data-intent="default"styling rule is still missing.The
successandinfoadditions look correct. However, as flagged in a prior review, no&[data-intent="default"]rule exists even thoughDEFAULT_ICONSmaps'default'to an icon inWarning.tsx, leaving that intent unstyled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/generic/Warning/Warning.module.scss` around lines 28 - 36, The stylesheet is missing a rule for data-intent="default" while Warning.tsx (DEFAULT_ICONS) still uses 'default' as a valid intent; add a &[data-intent="default"] block in Warning.module.scss mirroring the pattern used for success/info and set color and background-color to the corresponding CSS variables (e.g., --text-color-default and --card-bg-default or whichever project vars are used for the neutral/default warning style) so the default intent is styled consistently.convex/_model/lists/mutations/updateList.ts (2)
35-38: 🧹 Nitpick | 🔵 Trivial
return awaitonPromise<void>— pre-existing unresolved nit.
ctx.db.patch()returnsPromise<void>; thereturn awaitadds an unnecessary microtask hop with no observable benefit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/mutations/updateList.ts` around lines 35 - 38, The function is using "return await ctx.db.patch(...)" even though ctx.db.patch returns a Promise<void>; remove the unnecessary "await" and simply return the promise (i.e., change "return await ctx.db.patch(...)" to "return ctx.db.patch(...)" in the updateList mutation), keeping the same payload (updated spread and modifiedAt: Date.now()) so behavior is identical but avoids the extra microtask hop.
32-38:⚠️ Potential issue | 🟠 MajorMissing access control — pre-existing unresolved issue.
The two TODOs and the missing ownership/lock checks are still present. Any authenticated (or even unauthenticated, depending on the public Convex endpoint) caller can patch any list document, including changing
userId(ownership transfer), since alleditableFieldsare included inupdateListArgs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/mutations/updateList.ts` around lines 32 - 38, Ensure updateList enforces ownership and lock checks before calling ctx.db.patch: first fetch the existing list via ctx.db.get(id) and throw if not found; deny any update if the document has a locked flag; verify the caller is authenticated and is the list owner (compare ctx.auth or caller userId to doc.userId) before allowing general edits; explicitly forbid changing ownership by disallowing updates to userId (remove userId from updateListArgs/editableFields) and only permit approve-change transitions if the caller is a Team Owner (TO) role—check the caller's role before applying any approvedChange/approval fields; then apply allowed patched fields plus modifiedAt and call ctx.db.patch(id, patch).src/components/generic/Warning/Warning.tsx (1)
44-44:cloneElementsilently overwrites anyclassNameon the passediconprop.This was flagged in a previous review; the fix to merge via
clsxhas not yet been applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/generic/Warning/Warning.tsx` at line 44, The cloneElement call in the Warning component overwrites any className on the provided icon prop; update the cloneElement invocation to merge the existing icon.props.className with styles.Warning_Icon using clsx (import clsx if missing) so both classes are preserved — i.e., when calling cloneElement(icon ?? DEFAULT_ICONS[intent], { className: clsx(icon?.props?.className, styles.Warning_Icon) }) ensure DEFAULT_ICONS and styles.Warning_Icon remain used as before.convex/_model/lists/_helpers/getAvailableActions.ts (2)
18-18:Commentenum member is missing its JSDoc — inconsistent with all other members.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/_helpers/getAvailableActions.ts` at line 18, The enum member Comment lacks a JSDoc like the other enum members; update the enum (the Comment member) to include a brief JSDoc summary matching the style/format used by the other members so documentation and linting remain consistent (i.e., add a short /** ... */ comment above the Comment enum member).
36-36:isSelfis not typed asboolean— its inferred type isId<'users'> | false | null.
userId && doc.userId === userIdshort-circuits tonullwhenuserIdisnull. TypeScript's narrowing onisSelfis imprecise, and this pattern can silently regress if the value is reused elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/_helpers/getAvailableActions.ts` at line 36, The variable isSelf currently gets a union type because of the short-circuit expression (userId && doc.userId === userId); change this to produce an explicit boolean so TypeScript narrows correctly: ensure you first check userId is non-null/undefined (e.g., userId != null) and then compare doc.userId === userId, or coerce the result to boolean (e.g., Boolean(...)/!!) so isSelf is typed as boolean; update the declaration of isSelf accordingly and adjust any downstream code that depends on its type (symbols: isSelf, userId, doc.userId).src/components/ManageListButton/ManageListButton.tsx (4)
66-73: Icon-only button has no accessible label (WCAG 2.1 SC 4.1.2).
triggerButtonrenders only<FileText />with noaria-label,title, or visible text. Screen readers will announce an unlabelled control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ManageListButton/ManageListButton.tsx` around lines 66 - 73, triggerButton renders an icon-only control with <FileText /> and no accessible name; update the triggerButton definition in ManageListButton to provide an explicit accessible label (e.g. add aria-label and/or title on the <Button> and/or include visually hidden text inside the button) so screen readers can announce it; ensure the label uses the same semantic intent as the control (e.g. "Manage list" or a prop-driven label) and keep the existing className, variant, and size props on the <Button> to avoid visual regressions.
39-48: Fire-and-forget mutations — errors are silently dropped.
createList(data)andupdateList({...})are called with no.catch(),try/catch, oronErrorcallback. Mutation failures produce no user feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ManageListButton/ManageListButton.tsx` around lines 39 - 48, handleSubmit currently fires createList(data) and updateList({...}) without handling rejections so mutation errors are silently dropped; update handleSubmit to await the mutation calls (or return their promises) and handle errors using try/catch or .catch(), calling the appropriate error handler (e.g., an onError callback, setError state, or a toast) to surface failures to the user; specifically modify handleSubmit to await createList(data) when selectedList is falsy and await updateList({...selectedList, ...data}) when truthy, and in the catch block call your existing UI error surface (or add one) so failures from createList and updateList are not ignored.
108-116:selectedListis not cleared when the multi-list Drawer closes.
onOpenChange={setDrawerOpen}never resetsselectedList. Stale list data will show if the Drawer is reopened without choosing a new item.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ManageListButton/ManageListButton.tsx` around lines 108 - 116, The Drawer is using onOpenChange={setDrawerOpen} so closing the drawer never clears selectedList; change the handler to a function that sets drawerOpen and when open is false calls setSelectedList(undefined) (or null) to reset the selection; update the Drawer prop (Drawer, onOpenChange) to use this inline handler so selectedList is cleared whenever the drawer closes.
75-87:lists.length === 1opens the drawer withselectedList === null— wrong content is rendered.The
<= 1branch uses the Drawer'striggerprop which only callssetDrawerOpen(true), neverhandleOpenList. When exactly one list exists,selectedListstaysnull, sodrawerContenteither creates a new list (ifcanManage) or rendersnull(if not), instead of showing the single existing list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ManageListButton/ManageListButton.tsx` around lines 75 - 87, The early return currently uses "if (lists.length <= 1)" which causes the drawer trigger to open without calling handleOpenList when there is exactly one list, leaving selectedList null; change the condition to "if (lists.length === 0)" so the single-list case falls through and the normal button/handleOpenList path runs (or alternatively wire the trigger to call handleOpenList when lists.length === 1); update references to lists, selectedList, setDrawerOpen, handleOpenList, drawerContent, and triggerButton accordingly.src/components/ListCreateDialog/ListCreateDialog.tsx (2)
89-91: CSS module class names still reference the wrong components — styles won't apply.
styles.MatchResultCreateDialog_ScrollArea,styles.TournamentDetailPage_Tabs, andstyles.TournamentDetailPage_TabBarare not defined inListCreateDialog.module.scss; CSS Modules returnsundefinedfor missing keys, silently dropping all layout styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListCreateDialog/ListCreateDialog.tsx` around lines 89 - 91, The JSX uses undefined CSS module keys — replace incorrect style keys used on ScrollArea, Tabs and TabsList (styles.MatchResultCreateDialog_ScrollArea, styles.TournamentDetailPage_Tabs, styles.TournamentDetailPage_TabBar) with the correct class names exported from ListCreateDialog.module.scss (or add those class names to the SCSS); update the props on ScrollArea, Tabs and TabsList to reference the real keys (e.g., styles.ListCreateDialog_ScrollArea, styles.ListCreateDialog_Tabs, styles.ListCreateDialog_TabBar) or whatever exact identifiers are exported so the module lookup returns defined strings and the layout styling is applied.
76-83:handleSubmitis still fully commented-out and the type mismatch remains.The
onClickon the Submit button (line 124) will pass aReact.MouseEventtohandleSubmit(data: object); the actual submission data (storageId,tab,tournamentRegistrationId) must be read from component state. Additionally the commented argument namerawStorageIddoes not match the mutation fieldstorageId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListCreateDialog/ListCreateDialog.tsx` around lines 76 - 83, Uncomment and fix handleSubmit so it does not expect a MouseEvent — change its signature to take no args or a proper form data type, read submission values from component state (storageId, tab, tournamentRegistrationId, gameSystem) inside handleSubmit, and call createList with the correct mutation field name storageId (not rawStorageId). Ensure the Submit button calls handleSubmit without forwarding the click event (so onClick={() => handleSubmit()} or onSubmit handler) and pass the gathered values into createList({ data, gameSystem, storageId, tournamentRegistrationId }).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
convex/_generated/api.d.tsis excluded by!**/_generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (106)
.eslintrc.cjs.gitignoreconvex/_model/common/_helpers/filterWithSearchTerm.tsconvex/_model/common/_helpers/getDocStrict.tsconvex/_model/files/actions/convertImageToPdf.tsconvex/_model/files/actions/getFileDownloadUrl.tsconvex/_model/files/index.tsconvex/_model/files/queries/getFileMetadata.tsconvex/_model/gameSystems/battlefront/flamesOfWarV4/_helpers/deepenListData.tsconvex/_model/gameSystems/battlefront/flamesOfWarV4/_model/listData.tsconvex/_model/gameSystems/battlefront/flamesOfWarV4/index.tsconvex/_model/gameSystems/index.tsconvex/_model/listComments/_helpers/deepenListComment.tsconvex/_model/listComments/index.tsconvex/_model/listComments/mutations/createListComment.tsconvex/_model/listComments/table.tsconvex/_model/listComments/types.tsconvex/_model/lists/_helpers/checkListSubmittedOnTime.tsconvex/_model/lists/_helpers/checkListVisibility.tsconvex/_model/lists/_helpers/deepenList.tsconvex/_model/lists/_helpers/getAvailableActions.tsconvex/_model/lists/index.tsconvex/_model/lists/mutations/createList.tsconvex/_model/lists/mutations/deleteList.tsconvex/_model/lists/mutations/importListData.tsconvex/_model/lists/mutations/updateList.tsconvex/_model/lists/queries/getList.tsconvex/_model/lists/queries/getListsByTournamentRegistration.tsconvex/_model/lists/queries/getListsByUser.tsconvex/_model/lists/table.tsconvex/_model/lists/types.tsconvex/_model/matchResults/queries/getMatchResults.tsconvex/_model/tournamentCompetitors/_helpers/checkUserIsTeamCaptain.tsconvex/_model/tournamentCompetitors/_helpers/checkUsersAreTeammates.tsconvex/_model/tournamentCompetitors/index.tsconvex/_model/tournamentRegistrations/_helpers/deepenTournamentRegistration.tsconvex/_model/tournamentRegistrations/_helpers/getAvailableActions.tsconvex/_model/tournaments/actions/exportFowV4TournamentMatchData.tsconvex/_model/tournaments/table.tsconvex/_model/utils/getTournamentRegistrationsCsv.tsconvex/files.tsconvex/listComments.tsconvex/lists.tsconvex/scheduledTasks.tsconvex/schema.tspackage.jsonsrc/api.tssrc/components/CommentThread/CommentThread.module.scsssrc/components/CommentThread/CommentThread.tsxsrc/components/CommentThread/CommentThreadEvent.tsxsrc/components/CommentThread/index.tssrc/components/ContextMenu/ContextMenu.tsxsrc/components/ContextMenu/ContextMenu.types.tssrc/components/FileButton/FileButton.module.scsssrc/components/FileButton/FileButton.tsxsrc/components/FileButton/index.tssrc/components/ListCommentForm/ListCommentForm.module.scsssrc/components/ListCommentForm/ListCommentForm.schema.tssrc/components/ListCommentForm/ListCommentForm.tsxsrc/components/ListCommentForm/index.tssrc/components/ListCreateDialog/ListCreateDialog.hooks.tsxsrc/components/ListCreateDialog/ListCreateDialog.module.scsssrc/components/ListCreateDialog/ListCreateDialog.tsxsrc/components/ListCreateDialog/index.tssrc/components/ListDetails/ListDetails.module.scsssrc/components/ListDetails/ListDetails.tsxsrc/components/ListDetails/ListDetailsButton.tsxsrc/components/ListDetails/ListDetailsTrigger.tsxsrc/components/ListDetails/index.tssrc/components/ListForm/ListForm.module.scsssrc/components/ListForm/ListForm.schema.tssrc/components/ListForm/ListForm.tsxsrc/components/ListForm/index.tssrc/components/ListProvider/ListContextMenu.tsxsrc/components/ListProvider/ListProvider.context.tsxsrc/components/ListProvider/ListProvider.hooks.tsxsrc/components/ListProvider/ListProvider.tsxsrc/components/ListProvider/actions/useDeleteAction.tsxsrc/components/ListProvider/actions/useDownloadAction.tsxsrc/components/ListProvider/actions/useEditAction.tsxsrc/components/ListProvider/index.tssrc/components/ManageListButton/ManageListButton.tsxsrc/components/ManageListButton/index.tssrc/components/TournamentCompetitorProvider/TournamentCompetitorContextMenu.tsxsrc/components/TournamentProvider/TournamentContextMenu.tsxsrc/components/TournamentRegistrationProvider/TournamentRegistrationContextMenu.tsxsrc/components/TournamentRegistrationProvider/TournamentRegistrationProvider.hooks.tssrc/components/TournamentRegistrationProvider/actions/useCreateListAction.tsxsrc/components/TournamentRegistrationProvider/index.tssrc/components/generic/InputTextArea/InputTextArea.scsssrc/components/generic/Timestamp/Timestamp.module.scsssrc/components/generic/Timestamp/Timestamp.tsxsrc/components/generic/Timestamp/Timestamp.utils.tssrc/components/generic/Warning/Warning.module.scsssrc/components/generic/Warning/Warning.tsxsrc/hooks/useFormDialog.tsxsrc/pages/ListDetailPage/ListDetailPage.tsxsrc/pages/ListDetailPage/index.tssrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.module.scsssrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.tsxsrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationListButton/index.tssrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationsTable/TournamentRegistrationsTable.tsxsrc/pages/TournamentDetailPage/TournamentDetailPage.tsxsrc/services/files.tssrc/services/listComments.tssrc/services/lists.ts
💤 Files with no reviewable changes (5)
- convex/_model/gameSystems/index.ts
- convex/_model/lists/mutations/importListData.ts
- convex/_model/gameSystems/battlefront/flamesOfWarV4/_helpers/deepenListData.ts
- convex/_model/gameSystems/battlefront/flamesOfWarV4/_model/listData.ts
- convex/_model/gameSystems/battlefront/flamesOfWarV4/index.ts
src/components/TournamentRegistrationProvider/actions/useCreateListAction.tsx
Show resolved
Hide resolved
...rDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 39
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)convex/_model/matchResults/queries/getMatchResults.ts (1)
16-26:⚠️ Potential issue | 🔴 Critical
args.filteris accepted but never applied — the filter is silently ignored at runtime.The function body only uses
args.paginationOpts;args.filteris never read. Every caller that passes atournamentId,tournamentPairingId, ortournamentCompetitorIdfilter will receive unfiltered results, which is a silent correctness failure — no error is thrown and the response looks valid but contains wrong data.🐛 Proposed fix (sketch)
export const getMatchResults = async ( ctx: QueryCtx, args: Infer<typeof getMatchResultsArgs>, ): Promise<PaginationResult<DeepMatchResult>> => { - const results = await ctx.db.query('matchResults').order('desc').paginate(args.paginationOpts); + let query = ctx.db.query('matchResults').order('desc'); + // Apply filters if provided + if (args.filter?.tournamentId) { + query = query.filter((q) => q.eq(q.field('tournamentId'), args.filter!.tournamentId)); + } + if (args.filter?.tournamentPairingId) { + query = query.filter((q) => q.eq(q.field('tournamentPairingId'), args.filter!.tournamentPairingId)); + } + if (args.filter?.tournamentCompetitorId) { + query = query.filter((q) => q.eq(q.field('tournamentCompetitorId'), args.filter!.tournamentCompetitorId)); + } + const results = await query.paginate(args.paginationOpts); return { ...results, page: await Promise.all(results.page.map( async (item) => await deepenMatchResult(ctx, item), )), }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/matchResults/queries/getMatchResults.ts` around lines 16 - 26, getMatchResults currently ignores args.filter causing unfiltered results; update the query to apply filters before ordering/pagination by using ctx.db.query('matchResults').filter(...) and conditionally add clauses for args.filter.tournamentId, args.filter.tournamentPairingId, and args.filter.tournamentCompetitorId (only when those values are defined). Ensure filters are composed conjunctively (AND), handle tournamentCompetitorId by matching the appropriate competitor fields on the matchResults record, then call .order('desc').paginate(args.paginationOpts) as before and keep the existing Promise.all mapping that calls deepenMatchResult for each page item.convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts (2)
15-15:⚠️ Potential issue | 🟡 MinorTypo: "Gater" → "Gather"
- // 1. Gater base data + // 1. Gather base data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` at line 15, Fix the typo in the inline comment inside exportFowV4TournamentMatchData: change the comment text "Gater base data" to "Gather base data" so the comment reads correctly and improves readability.
73-76:⚠️ Potential issue | 🔴 CriticalCrash when
rowsis empty:rows[0]isundefinedIf all match results are filtered out by the
!detailsguard (or there are no match results),rowsis an empty array andObject.keys(rows[0])throws aTypeErrorat runtime. The function already declaresPromise<string | null>as its return type, so an earlynullreturn is the clean fix.🐛 Proposed fix
+ // Guard: nothing to export + if (rows.length === 0) { + return null; + } + // 3. Convert to CSV const csv = [ Object.keys(rows[0]).join(','), // Header🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/tournaments/actions/exportFowV4TournamentMatchData.ts` around lines 73 - 76, The code builds csv using Object.keys(rows[0]) but crashes if rows is empty; update exportFowV4TournamentMatchData to check rows.length === 0 (or !rows || rows.length === 0) after rows is populated and return null early; otherwise proceed to build the csv string as before using Object.keys(rows[0]) and rows.map(...). Ensure the function still satisfies its Promise<string | null> return type and reference the existing local variable rows and csv in your change.convex/_model/lists/_helpers/deepenList.ts (1)
18-20:⚠️ Potential issue | 🟡 MinorStale
@paramname in JSDoc.
@param tournamentshould be@param docto match the actual parameter name.📝 Proposed fix
- * `@param` tournament - Raw list document + * `@param` doc - Raw list document🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/_helpers/deepenList.ts` around lines 18 - 20, The JSDoc for the deepenList function has a stale parameter name: change the `@param` tag that currently says "@param tournament" to "@param doc" so the documentation matches the actual function parameter name (doc) in deepenList; update any related JSDoc lines in the same comment block to ensure consistency with the function signature.src/services/files.ts (1)
31-57:⚠️ Potential issue | 🔴 Critical
setIsLoading(false)is unreachable on the success path —loadingstaystrueafter every successful upload.Before this PR, there was no early
returnin thetryblock, so execution always fell through to thesetIsLoading(false)on line 56. Addingreturn storageId(line 46) short-circuits the function on success, making line 56 only reachable from thecatchbranch. Any component that readsuseUploadFile().loadingwill be stuck in a loading state indefinitely after a successful upload.
useUploadDocument(also introduced in this PR) correctly usesfinally— apply the same pattern here:🐛 Proposed fix
return { mutation: async (file: Blob) => { setIsLoading(true); try { const response = await fetch(await generateUploadUrl(), { method: 'POST', headers: { 'Content-Type': file.type }, body: file, }); const { storageId } = await response.json(); if (config?.successMessage) { toast.success(config.successMessage); } if (config?.onSuccess) { config.onSuccess(storageId, file); } return storageId; } catch (error) { console.error(error); if (error instanceof Error) { toast.error('Error', { description: error.message }); } if (config?.onError) { config.onError(error); } - } - setIsLoading(false); + } finally { + setIsLoading(false); + } }, loading, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/files.ts` around lines 31 - 57, The upload mutation currently returns early inside the try block which makes setIsLoading(false) unreachable on the success path; change the async mutation (the mutation: async (file: Blob) => { ... }) to ensure setIsLoading(false) always runs by using a try/catch/finally pattern (or capturing storageId in the try and returning after a finally) so that setIsLoading(false) is executed in finally; keep existing toast and config.onSuccess/onError behavior but move the final return so the success path doesn't short-circuit the finally block.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
convex/_generated/api.d.tsis excluded by!**/_generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (106)
.eslintrc.cjs.gitignoreconvex/_model/common/_helpers/filterWithSearchTerm.tsconvex/_model/common/_helpers/getDocStrict.tsconvex/_model/files/actions/convertImageToPdf.tsconvex/_model/files/actions/getFileDownloadUrl.tsconvex/_model/files/index.tsconvex/_model/files/queries/getFileMetadata.tsconvex/_model/gameSystems/battlefront/flamesOfWarV4/_helpers/deepenListData.tsconvex/_model/gameSystems/battlefront/flamesOfWarV4/_model/listData.tsconvex/_model/gameSystems/battlefront/flamesOfWarV4/index.tsconvex/_model/gameSystems/index.tsconvex/_model/listComments/_helpers/deepenListComment.tsconvex/_model/listComments/index.tsconvex/_model/listComments/mutations/createListComment.tsconvex/_model/listComments/table.tsconvex/_model/listComments/types.tsconvex/_model/lists/_helpers/checkListSubmittedOnTime.tsconvex/_model/lists/_helpers/checkListVisibility.tsconvex/_model/lists/_helpers/deepenList.tsconvex/_model/lists/_helpers/getAvailableActions.tsconvex/_model/lists/index.tsconvex/_model/lists/mutations/createList.tsconvex/_model/lists/mutations/deleteList.tsconvex/_model/lists/mutations/importListData.tsconvex/_model/lists/mutations/updateList.tsconvex/_model/lists/queries/getList.tsconvex/_model/lists/queries/getListsByTournamentRegistration.tsconvex/_model/lists/queries/getListsByUser.tsconvex/_model/lists/table.tsconvex/_model/lists/types.tsconvex/_model/matchResults/queries/getMatchResults.tsconvex/_model/tournamentCompetitors/_helpers/checkUserIsTeamCaptain.tsconvex/_model/tournamentCompetitors/_helpers/checkUsersAreTeammates.tsconvex/_model/tournamentCompetitors/index.tsconvex/_model/tournamentRegistrations/_helpers/deepenTournamentRegistration.tsconvex/_model/tournamentRegistrations/_helpers/getAvailableActions.tsconvex/_model/tournaments/actions/exportFowV4TournamentMatchData.tsconvex/_model/tournaments/table.tsconvex/_model/utils/getTournamentRegistrationsCsv.tsconvex/files.tsconvex/listComments.tsconvex/lists.tsconvex/scheduledTasks.tsconvex/schema.tspackage.jsonsrc/api.tssrc/components/CommentThread/CommentThread.module.scsssrc/components/CommentThread/CommentThread.tsxsrc/components/CommentThread/CommentThreadEvent.tsxsrc/components/CommentThread/index.tssrc/components/ContextMenu/ContextMenu.tsxsrc/components/ContextMenu/ContextMenu.types.tssrc/components/FileButton/FileButton.module.scsssrc/components/FileButton/FileButton.tsxsrc/components/FileButton/index.tssrc/components/ListCommentForm/ListCommentForm.module.scsssrc/components/ListCommentForm/ListCommentForm.schema.tssrc/components/ListCommentForm/ListCommentForm.tsxsrc/components/ListCommentForm/index.tssrc/components/ListCreateDialog/ListCreateDialog.hooks.tsxsrc/components/ListCreateDialog/ListCreateDialog.module.scsssrc/components/ListCreateDialog/ListCreateDialog.tsxsrc/components/ListCreateDialog/index.tssrc/components/ListDetails/ListDetails.module.scsssrc/components/ListDetails/ListDetails.tsxsrc/components/ListDetails/ListDetailsButton.tsxsrc/components/ListDetails/ListDetailsTrigger.tsxsrc/components/ListDetails/index.tssrc/components/ListForm/ListForm.module.scsssrc/components/ListForm/ListForm.schema.tssrc/components/ListForm/ListForm.tsxsrc/components/ListForm/index.tssrc/components/ListProvider/ListContextMenu.tsxsrc/components/ListProvider/ListProvider.context.tsxsrc/components/ListProvider/ListProvider.hooks.tsxsrc/components/ListProvider/ListProvider.tsxsrc/components/ListProvider/actions/useDeleteAction.tsxsrc/components/ListProvider/actions/useDownloadAction.tsxsrc/components/ListProvider/actions/useEditAction.tsxsrc/components/ListProvider/index.tssrc/components/ManageListButton/ManageListButton.tsxsrc/components/ManageListButton/index.tssrc/components/TournamentCompetitorProvider/TournamentCompetitorContextMenu.tsxsrc/components/TournamentProvider/TournamentContextMenu.tsxsrc/components/TournamentRegistrationProvider/TournamentRegistrationContextMenu.tsxsrc/components/TournamentRegistrationProvider/TournamentRegistrationProvider.hooks.tssrc/components/TournamentRegistrationProvider/actions/useCreateListAction.tsxsrc/components/TournamentRegistrationProvider/index.tssrc/components/generic/InputTextArea/InputTextArea.scsssrc/components/generic/Timestamp/Timestamp.module.scsssrc/components/generic/Timestamp/Timestamp.tsxsrc/components/generic/Timestamp/Timestamp.utils.tssrc/components/generic/Warning/Warning.module.scsssrc/components/generic/Warning/Warning.tsxsrc/hooks/useFormDialog.tsxsrc/pages/ListDetailPage/ListDetailPage.tsxsrc/pages/ListDetailPage/index.tssrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.module.scsssrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.tsxsrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationListButton/index.tssrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationsTable/TournamentRegistrationsTable.tsxsrc/pages/TournamentDetailPage/TournamentDetailPage.tsxsrc/services/files.tssrc/services/listComments.tssrc/services/lists.ts
💤 Files with no reviewable changes (5)
- convex/_model/gameSystems/battlefront/flamesOfWarV4/_helpers/deepenListData.ts
- convex/_model/lists/mutations/importListData.ts
- convex/_model/gameSystems/index.ts
- convex/_model/gameSystems/battlefront/flamesOfWarV4/index.ts
- convex/_model/gameSystems/battlefront/flamesOfWarV4/_model/listData.ts
...rDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.tsx
Outdated
Show resolved
Hide resolved
...rDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.tsx
Outdated
Show resolved
Hide resolved
...rDetailPage/components/TournamentRegistrationListButton/TournamentRegistrationListButton.tsx
Outdated
Show resolved
Hide resolved
ianpaschal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/_model/matchResults/queries/getMatchResults.ts (2)
23-25: 🧹 Nitpick | 🔵 TrivialRedundant
async/awaitwrapper insidePromise.all.
deepenMatchResultalready returns aPromise; wrapping it in anasyncarrow andawait-ing it adds an unnecessary microtask allocation for every item.♻️ Proposed simplification
- page: await Promise.all(results.page.map( - async (item) => await deepenMatchResult(ctx, item), - )), + page: await Promise.all(results.page.map( + (item) => deepenMatchResult(ctx, item), + )),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/matchResults/queries/getMatchResults.ts` around lines 23 - 25, Redundant async/await in the Promise.all mapping: in getMatchResults.ts remove the async arrow and inner await when mapping results.page; replace page: await Promise.all(results.page.map(async (item) => await deepenMatchResult(ctx, item))) with page: await Promise.all(results.page.map(item => deepenMatchResult(ctx, item))) so deepenMatchResult (which already returns a Promise) is used directly and avoids extra microtasks.
20-26:⚠️ Potential issue | 🔴 CriticalApply filter to query and fix schema —
args.filteris accepted but never used; also remove invalid filter option.The query at line 20 ignores
args.filterentirely, always returning the full unfiltered table. Additionally, the filter schema acceptstournamentCompetitorIdbut that field does not exist on thematchResultstable, making it impossible to filter by that criterion.Fix requires two changes:
Apply filter to query using the correct snake_case index names:
by_tournament_id(notby_tournament)by_tournament_pairing_id(notby_pairing)Remove
tournamentCompetitorIdfrom the filter schema — the field doesn't exist on the table.Example fix
+ let query = ctx.db.query('matchResults'); + if (args.filter?.tournamentId) { + query = query.withIndex('by_tournament_id', (q) => q.eq('tournamentId', args.filter!.tournamentId!)); + } else if (args.filter?.tournamentPairingId) { + query = query.withIndex('by_tournament_pairing_id', (q) => q.eq('tournamentPairingId', args.filter!.tournamentPairingId!)); + } - const results = await ctx.db.query('matchResults').order('desc').paginate(args.paginationOpts); + const results = await query.order('desc').paginate(args.paginationOpts);Also update
getMatchResultsArgsto remove the invalid filter option:filter: v.optional(v.object({ tournamentId: v.optional(v.id('tournaments')), tournamentPairingId: v.optional(v.id('tournamentPairings')), - tournamentCompetitorId: v.optional(v.id('tournamentCompetitors')), })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/matchResults/queries/getMatchResults.ts` around lines 20 - 26, The query in getMatchResults currently ignores args.filter and the filter schema includes a non-existent tournamentCompetitorId; update the code so ctx.db.query('matchResults') applies filters from args.filter by using the correct index names (use .index('by_tournament_id') when filtering by tournamentId and .index('by_tournament_pairing_id') when filtering by pairingId) before ordering/paginating and keep using deepenMatchResult for mapping the page; also remove tournamentCompetitorId from the getMatchResultsArgs filter schema so only valid filter keys (tournamentId, pairingId, etc.) are accepted.
♻️ Duplicate comments (14)
src/components/CommentThread/CommentThreadEvent.tsx (2)
31-34:cornersstill omitted fromgetStyleClassNameson the icon wrapper.
CommentThread.tsxpassescorners: 'normal'togetStyleClassNames; the icon wrapper here doesn't. The SCSS overrides this visually, but it remains an API inconsistency across the component family.♻️ Proposed fix
<div className={clsx(styles.CommentThread_Icon, ...getStyleClassNames({ intent: event.intent, variant: 'passive', + corners: 'normal', }))}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CommentThread/CommentThreadEvent.tsx` around lines 31 - 34, The icon wrapper omits the corners arg when calling getStyleClassNames, causing an API inconsistency; update the className call in CommentThreadEvent (the div using clsx(styles.CommentThread_Icon, ...getStyleClassNames({...}))) to include corners: 'normal' alongside intent: event.intent and variant: 'passive' so it matches CommentThread.tsx and the component family.
12-12:⚠️ Potential issue | 🟡 Minor
EventData.repliesshould beEventData[], notComment[].
Comment(from./CommentThread) carries_id,body, anduser— fields that a timeline event doesn't have. Replies to an event would logically be further events, so the type should be self-referential:🐛 Proposed fix
type EventData = { _creationTime: number; - replies?: Comment[]; + replies?: EventData[]; content: ReactNode; icon: ReactElement; intent?: ElementIntent; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CommentThread/CommentThreadEvent.tsx` at line 12, The EventData type in CommentThreadEvent.tsx currently declares replies?: Comment[] but replies should be recursive (EventData[]) because timeline replies are events, not Comment objects; update the EventData declaration to use replies?: EventData[] (remove or stop using the Comment type import if it's only for this field) and then adjust any usages in renderers/helpers (e.g., where CommentThreadEvent or EventData is consumed) to expect EventData for nested replies.src/components/ListCommentForm/ListCommentForm.tsx (1)
79-101: 🧹 Nitpick | 🔵 TrivialUse
getValues()for imperative reads in approve/reject handlers.
watch()is subscription-oriented;getValues()is the idiomatic snapshot API for click handlers.♻️ Suggested change
const handleApprove = (): void => { const validFormData = validateForm(schema, { - ...form.watch(), + ...form.getValues(), ...forcedValues, userId: user?._id, }, form.setError); if (validFormData) { onSubmit({ ...validFormData, control: 'approved' }); form.reset(); } }; const handleReject = (): void => { const validFormData = validateForm(schema, { - ...form.watch(), + ...form.getValues(), ...forcedValues, userId: user?._id, }, form.setError); if (validFormData) { onSubmit({ ...validFormData, control: 'rejected' }); form.reset(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListCommentForm/ListCommentForm.tsx` around lines 79 - 101, The handlers handleApprove and handleReject should use form.getValues() instead of form.watch() for an imperative snapshot; update both to call validateForm(schema, { ...form.getValues(), ...forcedValues, userId: user?._id }, form.setError) and then proceed to call onSubmit({ ...validFormData, control: 'approved'|'rejected' }) and form.reset() as before so the snapshot is taken once at click time and not via a subscription-oriented API.src/components/ListCommentForm/ListCommentForm.schema.ts (1)
5-8:⚠️ Potential issue | 🟡 MinorRequire non-empty comments and verify Zod v4 error option.
bodycurrently permits empty strings; consider enforcing a non-empty comment. Also, if this repo is on Zod 4, confirm whetherz.string({ message: ... })is supported or should use the unifiederroroption.🔧 Proposed update
export const schema = z.object({ - body: z.string(), - listId: z.string({ message: 'List is required.' }).transform((val) => val as ListId), + body: z.string().min(1, 'Comment is required.'), + listId: z.string({ error: 'List is required.' }).transform((val) => val as ListId), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListCommentForm/ListCommentForm.schema.ts` around lines 5 - 8, The schema currently allows empty comment bodies and uses z.string({ message: ... }) which may not be valid in Zod v4; update schema by making body required via z.string().nonempty({ message: 'Comment is required.' }) (or .min(1, { message: ... }) if you prefer) and change listId to z.string().nonempty({ message: 'List is required.' }).transform((val) => val as ListId) so listId still casts to ListId; confirm Zod version and avoid passing a message directly into z.string(...) if v4 requires using .nonempty/.min/.refine for error messages.convex/_model/files/actions/convertImageToPdf.ts (1)
11-11:image/jpgis not a standard IANA MIME type.The standard MIME type for JPEG is
image/jpeg. While includingimage/jpgfor client compatibility is reasonable, a brief inline comment would clarify why it's there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/files/actions/convertImageToPdf.ts` at line 11, IMAGE_MIME_TYPES currently includes the nonstandard 'image/jpg'; leave it for client compatibility but add a brief inline comment next to IMAGE_MIME_TYPES explaining that the IANA-standard MIME type is 'image/jpeg' and 'image/jpg' is included intentionally for legacy/client compatibility (reference IMAGE_MIME_TYPES in convertImageToPdf.ts).src/components/ListDetails/ListDetails.module.scss (1)
5-10:⚠️ Potential issue | 🟡 MinorUnused "activity" grid track creates a spurious gap, and
&_Activitylacksgrid-area: activity.The grid template defines an "activity" row (Line 7), but no element is assigned
grid-area: activity. This causesgap: 1remto insert an extra gutter between the viewer and comments tracks (visible in the default single-column layout; hidden whendata-inset="true"setsgap: 0). Additionally,&_Activity(lines 63–70) omitsgrid-area: activity, so when the activity section is eventually rendered, it will auto-place rather than land in its designated slot.Consider removing "activity" from the grid template until the feature is implemented, or at minimum add
grid-area: activityto&_Activity.♻️ Interim fix — remove unimplemented area from grid template
.ListDetails { display: grid; - grid-template-areas: "viewer" "activity" "comments"; + grid-template-areas: "viewer" "comments"; grid-template-columns: minmax(0, 1fr); - grid-template-rows: minmax(0, 1fr) auto auto; + grid-template-rows: minmax(0, 1fr) auto; gap: 1rem;And for the horizontal variant:
&[data-orientation="horizontal"] { - grid-template-areas: "viewer comments" "activity comments"; + grid-template-areas: "viewer comments"; grid-template-columns: minmax(0, 1fr) 20rem; - grid-template-rows: minmax(0, 1fr) auto; + grid-template-rows: minmax(0, 1fr);When
_Activityis ready, also add:&_CommentList, &_Activity { + grid-area: activity; `@include` flex.column;Also applies to: 63-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListDetails/ListDetails.module.scss` around lines 5 - 10, The grid declares an unused "activity" row in .ListDetails' grid-template-areas which creates an extra gap and &_Activity also lacks grid-area: activity so it would auto-place; fix by either removing "activity" from .ListDetails' grid-template-areas and grid-template-rows until the activity feature is implemented (ensuring the two-row template and gaps match the single-column layout), or add grid-area: activity to the &_Activity rule so the activity element occupies the intended slot when rendered; update the horizontal variant accordingly if you choose to remove the area.convex/_model/files/actions/getFileDownloadData.ts (1)
24-28:⚠️ Potential issue | 🟠 MajorO(n²) byte-by-byte string concatenation — use chunked conversion for large files.
binary += String.fromCharCode(bytes[i])allocates a new string on every iteration. For a 1 MB PDF this is ~1 million string allocations with quadratic total work. Use a chunked approach to stay within call-stack limits and achieve linear time:⚡ Proposed fix (chunked, call-stack safe)
- let binary = ''; - for (let i = 0; i < bytes.length; i++) { - binary += String.fromCharCode(bytes[i]); - } - return btoa(binary); + const CHUNK = 0x8000; // 32 KB + const parts: string[] = []; + for (let i = 0; i < bytes.length; i += CHUNK) { + parts.push(String.fromCharCode(...bytes.subarray(i, i + CHUNK))); + } + return btoa(parts.join(''));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/files/actions/getFileDownloadData.ts` around lines 24 - 28, The current byte-by-byte string concatenation in getFileDownloadData (the loop that does binary += String.fromCharCode(bytes[i])) is O(n²) and must be replaced with a chunked conversion: iterate over the Uint8Array `bytes` in fixed-size chunks (e.g., 32k–64k), for each chunk convert the subarray to a string via String.fromCharCode.apply(null, subarray) (ensuring chunkSize is small enough to be call-stack safe), push each chunk into a `chunks` array, then join the chunks into a single `binary` string and call btoa(binary) to return the base64 result; update any variables (`binary`, `chunks`, `chunkSize`, loop over `bytes`) accordingly.convex/_model/lists/mutations/deleteList.ts (1)
20-26:⚠️ Potential issue | 🟠 MajorCascade-delete list comments to avoid dangling data.
Deleting the list without removing its
listCommentsleaves orphaned rows. Please delete related comments before removing the list.🧹 Suggested fix
const availableActions = await getAvailableActions(ctx, list); if (!availableActions.includes(ListActionKey.Delete)) { throw new ConvexError(getErrorMessage('USER_DOES_NOT_HAVE_PERMISSION')); } + + const comments = await ctx.db + .query('listComments') + .withIndex('by_list', (q) => q.eq('listId', args.id)) + .collect(); + await Promise.all(comments.map((c) => ctx.db.delete(c._id))); await ctx.db.delete(args.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/mutations/deleteList.ts` around lines 20 - 26, When deleting a list in deleteList (after getDocStrict and permission check via getAvailableActions and ListActionKey.Delete), first delete all related list comments to avoid orphaned rows by querying the listComments table for entries where listId === args.id and removing them (e.g., ctx.db.deleteMany or a loop over found comment ids), then call ctx.db.delete(args.id) to remove the list; ensure you perform the comment deletion before invoking ctx.db.delete and preserve error handling/transaction semantics if available.src/components/ListProvider/actions/useEditAction.tsx (1)
17-42:⚠️ Potential issue | 🔴 CriticalGuard against null auth before using
user!._id.
useAuth()can return null (e.g., session expiry), so theuser!._idassertion can throw when the form submits. Please add a guard and remove the non‑null assertion.🛡️ Suggested fix
const user = useAuth(); + if (!user) return null; const { mutation: updateList } = useUpdateList({ @@ onSubmit={(data) => { updateList({ ...data, _id: subject._id, - userId: user!._id, + userId: user._id, }); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListProvider/actions/useEditAction.tsx` around lines 17 - 42, The code uses user!._id in useEditAction when submitting the ListForm which can crash if useAuth() returns null; remove the non-null assertion and add a null guard: inside the onSubmit handler (where updateList is called) check that user is present (e.g., if (!user) { toast.error('Not authenticated'); close(); return; } or prevent submitting by disabling the form), then pass user._id to updateList only when user is defined; update references in useEditAction, ListForm onSubmit handling and any related logic to handle the unauthenticated case gracefully instead of using user!.src/components/ListForm/ListForm.tsx (2)
95-147: 🧹 Nitpick | 🔵 TrivialRemove the large commented-out block.
It’s dead code and distracts from the live implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListForm/ListForm.tsx` around lines 95 - 147, Remove the large commented-out block in ListForm.tsx that contains the old handleReplace variants, the file render/storageId/useGetFileMetadata code, handleDownload, and the FUTURE formations code (including useFieldArray, formationFields, appendFormation, removeFormation, handleAddFormation); delete these commented lines so only active implementation remains and ensure no live references to uploadFile, storageId or formation helpers are left unused elsewhere in the file.
50-55: 🧹 Nitpick | 🔵 TrivialAvoid spreading
existingValuesintodefaultValues.
Listincludes non-form fields (e.g., comments/availableActions). Prefer selecting onlyFormDatakeys.♻️ Suggested refactor
+ const sanitizedExisting = existingValues ? { + gameSystem: existingValues.gameSystem, + storageId: existingValues.storageId, + tournamentRegistrationId: existingValues.tournamentRegistrationId ?? null, + userId: existingValues.userId, + } : undefined; + const form = useForm<FormData>({ defaultValues: { ...defaultValues, - ...existingValues, + ...sanitizedExisting, ...forcedValues, }, mode: 'onSubmit', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListForm/ListForm.tsx` around lines 50 - 55, The current useForm defaultValues spreads existingValues which may contain non-form fields from the List (like comments/availableActions); instead, only merge keys that belong to the FormData shape. Modify the useForm call in ListForm (where defaultValues, existingValues, forcedValues are merged) to pick/clone only the FormData keys from existingValues before spreading (e.g., use a small helper or lodash.pick to extract the FormData fields) so non-form properties are not injected into the form state.convex/_model/lists/queries/getListsByUser.ts (1)
16-20:⚠️ Potential issue | 🟠 MajorUnbounded
.collect()can exceed read limits — paginate instead.
This fetches all lists for a user with no upper bound. Consider.paginate()with limit/continuation token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/queries/getListsByUser.ts` around lines 16 - 20, The query in getListsByUser uses an unbounded .collect() on ctx.db.query('lists').withIndex('by_user') which can exceed read limits; change to .paginate(...) instead and update the query handler signature to accept pagination parameters (e.g., args.limit and args.cursor/continuation token), call .paginate({ limit: args.limit ?? DEFAULT_LIMIT, cursor: args.cursor }) on the same index query, map/deepen the returned page.items via deepenList(ctx, r), filter with notNullOrUndefined, and return both the items and the next cursor (page.nextCursor) so callers can continue pagination.src/components/ListDetails/ListDetails.tsx (1)
48-48:⚠️ Potential issue | 🟡 MinorError state from
useGetFileMetadatanot consumed — perpetual spinner on fetch failure.If
getFileMetadatarejects or the storage ID is invalid, no error is surfaced; thePdfViewerwill stay in its loading state with no user-visible feedback.🛠️ Proposed fix
- const { data: file, loading } = useGetFileMetadata(list ? { id: list.storageId } : 'skip'); + const { data: file, loading, error: fileError } = useGetFileMetadata(list ? { id: list.storageId } : 'skip');Then handle
fileErrorbefore the return (e.g., render an error state or pass it intoPdfViewer).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListDetails/ListDetails.tsx` at line 48, The call to useGetFileMetadata in ListDetails currently ignores the error state causing PdfViewer to spin forever; update the hook destructure to include the error (e.g., fileError) from useGetFileMetadata and handle it before returning: if fileError render a compact error UI (message + retry button) or pass the error into PdfViewer via props (so it can stop loading and show an error), and ensure loading is only true while fetching and cleared on error so the spinner is not perpetual.convex/_model/lists/_helpers/deepenList.ts (1)
62-67: 🧹 Nitpick | 🔵 TrivialIndependent
getAvailableActionsandcheckListSubmittedOnTimeare still awaited sequentially.Both calls are independent and can be parallelised with
Promise.all.⚡ Proposed refactor
- return { - ...doc, - availableActions: await getAvailableActions(ctx, doc), - lastControlComment, - displayName: undefined, - comments, - onTime: await checkListSubmittedOnTime(ctx, doc), - user, - }; + const [availableActions, onTime] = await Promise.all([ + getAvailableActions(ctx, doc), + checkListSubmittedOnTime(ctx, doc), + ]); + return { + ...doc, + availableActions, + lastControlComment, + displayName: undefined, + comments, + onTime, + user, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/_helpers/deepenList.ts` around lines 62 - 67, getAvailableActions and checkListSubmittedOnTime are awaited sequentially causing unnecessary latency; update the assignment in deepenList (where availableActions and onTime are set) to run both calls in parallel via Promise.all (call getAvailableActions(ctx, doc) and checkListSubmittedOnTime(ctx, doc) together), then destructure the results into availableActions and onTime before returning the object so the rest of deepenList uses those values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.eslintrc.cjs:
- Around line 49-52: Change the incorrect ESLint rule ID
'@typescript-eslint//explicit-function-return-type' to the correct single-slash
form '@typescript-eslint/explicit-function-return-type' in the .eslintrc.cjs
rules object so ESLint can find the rule; update the entry where the
double-slash appears (the rule key using
'@typescript-eslint//explicit-function-return-type') to the corrected
'@typescript-eslint/explicit-function-return-type'.
In `@convex/_model/files/actions/convertImageToPdf.ts`:
- Around line 25-26: The error in convertImageToPdf.ts currently interpolates
user-controlled args.mimeType into the ConvexError; remove that raw echo and
instead throw a generic/whitelisted message (e.g., "Unsupported MIME type.
Expected one of: ...") or include only a safely sanitized/truncated/escaped
representation if you must show the value; update the check in the
isImageMimeType branch to use IMAGE_MIME_TYPES for context and ensure
ConvexError does not contain the unsanitized args.mimeType.
- Around line 29-32: Replace the raw string thrown in convertImageToPdf.ts with
the centralized error message helper: when ctx.storage.get(args.storageId)
returns falsy, throw new ConvexError(getErrorMessage('FILE_NOT_FOUND')) instead
of 'File not found in storage' so the code uses the same shared error catalog;
update the import if needed to reference getErrorMessage at the top of the file
alongside other imports and ensure ConvexError is preserved.
In `@convex/_model/files/actions/getFileDownloadData.ts`:
- Around line 14-28: getFileDownloadData currently loads the entire blob into
memory and base64-encodes it (using ctx.storage.get, blob.arrayBuffer and btoa),
risking memory/response-size exhaustion for large files; after retrieving the
blob (or before calling arrayBuffer) check blob.size against a defined
MAX_DOWNLOAD_BYTES constant and throw a ConvexError (e.g., using
getErrorMessage('FILE_TOO_LARGE')) if it exceeds the limit, so the function
fails fast instead of attempting to allocate huge buffers or produce oversized
base64 responses.
In `@convex/_model/lists/mutations/createList.ts`:
- Around line 9-33: editableFields currently includes a required userId which is
ignored by the server; change createListArgs so it does not require or accept
userId by deriving a new args schema that omits userId from editableFields
(e.g., create a clientEditableFields = { ...editableFields } without userId and
export createListArgs = v.object(clientEditableFields)), keep createList and
checkAuth as-is so the server continues to set userId from checkAuth(ctx) and
not from client input.
In `@src/components/ListDetails/ListDetails.tsx`:
- Line 113: The element ID "create-list-check" is a copy-paste artifact and
should be renamed to a descriptive, unique ID (e.g., "create-list-form" or
"create-list") to avoid misleading readers and potential htmlFor conflicts;
change the id value in ListDetails.tsx and update any corresponding label
htmlFor or querySelector usages that reference "create-list-check" so they point
to the new ID (search for "create-list-check" and replace with the chosen name
in the component).
- Around line 57-59: The guard in ListDetails.tsx treats a resolved null list
the same as an undefined loading state, causing <Spinner /> to render
indefinitely; update the check to only show the spinner while loading (e.g.,
when listLoading is true or list is undefined) and explicitly handle the case
when list === null (visibility denied / not found) by returning an appropriate
fallback (message, redirect, or empty state) instead of continuing to render the
spinner; locate the conditional around listLoading / list in the ListDetails
component and change it to differentiate undefined vs null and add the
null-handling branch.
In `@src/components/ListForm/ListForm.tsx`:
- Around line 86-93: The drop handler currently calls uploadFile even when the
form should be disabled; update the useDropzone onDrop to respect the same
disabled condition (loading || isUploadLoading) by short-circuiting and
returning early when disabled, and also set useDropzone options to prevent
interactions when disabled (e.g., set noClick/noKeyboard when disabled) so the
dropzone behavior matches the disabled state; update the onDrop closure that
calls uploadFile and the useDropzone config that currently uses
accept/disabled/isDragActive to reference the shared disabled flag.
---
Outside diff comments:
In `@convex/_model/matchResults/queries/getMatchResults.ts`:
- Around line 23-25: Redundant async/await in the Promise.all mapping: in
getMatchResults.ts remove the async arrow and inner await when mapping
results.page; replace page: await Promise.all(results.page.map(async (item) =>
await deepenMatchResult(ctx, item))) with page: await
Promise.all(results.page.map(item => deepenMatchResult(ctx, item))) so
deepenMatchResult (which already returns a Promise) is used directly and avoids
extra microtasks.
- Around line 20-26: The query in getMatchResults currently ignores args.filter
and the filter schema includes a non-existent tournamentCompetitorId; update the
code so ctx.db.query('matchResults') applies filters from args.filter by using
the correct index names (use .index('by_tournament_id') when filtering by
tournamentId and .index('by_tournament_pairing_id') when filtering by pairingId)
before ordering/paginating and keep using deepenMatchResult for mapping the
page; also remove tournamentCompetitorId from the getMatchResultsArgs filter
schema so only valid filter keys (tournamentId, pairingId, etc.) are accepted.
---
Duplicate comments:
In `@convex/_model/files/actions/convertImageToPdf.ts`:
- Line 11: IMAGE_MIME_TYPES currently includes the nonstandard 'image/jpg';
leave it for client compatibility but add a brief inline comment next to
IMAGE_MIME_TYPES explaining that the IANA-standard MIME type is 'image/jpeg' and
'image/jpg' is included intentionally for legacy/client compatibility (reference
IMAGE_MIME_TYPES in convertImageToPdf.ts).
In `@convex/_model/files/actions/getFileDownloadData.ts`:
- Around line 24-28: The current byte-by-byte string concatenation in
getFileDownloadData (the loop that does binary += String.fromCharCode(bytes[i]))
is O(n²) and must be replaced with a chunked conversion: iterate over the
Uint8Array `bytes` in fixed-size chunks (e.g., 32k–64k), for each chunk convert
the subarray to a string via String.fromCharCode.apply(null, subarray) (ensuring
chunkSize is small enough to be call-stack safe), push each chunk into a
`chunks` array, then join the chunks into a single `binary` string and call
btoa(binary) to return the base64 result; update any variables (`binary`,
`chunks`, `chunkSize`, loop over `bytes`) accordingly.
In `@convex/_model/lists/_helpers/deepenList.ts`:
- Around line 62-67: getAvailableActions and checkListSubmittedOnTime are
awaited sequentially causing unnecessary latency; update the assignment in
deepenList (where availableActions and onTime are set) to run both calls in
parallel via Promise.all (call getAvailableActions(ctx, doc) and
checkListSubmittedOnTime(ctx, doc) together), then destructure the results into
availableActions and onTime before returning the object so the rest of
deepenList uses those values.
In `@convex/_model/lists/mutations/deleteList.ts`:
- Around line 20-26: When deleting a list in deleteList (after getDocStrict and
permission check via getAvailableActions and ListActionKey.Delete), first delete
all related list comments to avoid orphaned rows by querying the listComments
table for entries where listId === args.id and removing them (e.g.,
ctx.db.deleteMany or a loop over found comment ids), then call
ctx.db.delete(args.id) to remove the list; ensure you perform the comment
deletion before invoking ctx.db.delete and preserve error handling/transaction
semantics if available.
In `@convex/_model/lists/queries/getListsByUser.ts`:
- Around line 16-20: The query in getListsByUser uses an unbounded .collect() on
ctx.db.query('lists').withIndex('by_user') which can exceed read limits; change
to .paginate(...) instead and update the query handler signature to accept
pagination parameters (e.g., args.limit and args.cursor/continuation token),
call .paginate({ limit: args.limit ?? DEFAULT_LIMIT, cursor: args.cursor }) on
the same index query, map/deepen the returned page.items via deepenList(ctx, r),
filter with notNullOrUndefined, and return both the items and the next cursor
(page.nextCursor) so callers can continue pagination.
In `@src/components/CommentThread/CommentThreadEvent.tsx`:
- Around line 31-34: The icon wrapper omits the corners arg when calling
getStyleClassNames, causing an API inconsistency; update the className call in
CommentThreadEvent (the div using clsx(styles.CommentThread_Icon,
...getStyleClassNames({...}))) to include corners: 'normal' alongside intent:
event.intent and variant: 'passive' so it matches CommentThread.tsx and the
component family.
- Line 12: The EventData type in CommentThreadEvent.tsx currently declares
replies?: Comment[] but replies should be recursive (EventData[]) because
timeline replies are events, not Comment objects; update the EventData
declaration to use replies?: EventData[] (remove or stop using the Comment type
import if it's only for this field) and then adjust any usages in
renderers/helpers (e.g., where CommentThreadEvent or EventData is consumed) to
expect EventData for nested replies.
In `@src/components/ListCommentForm/ListCommentForm.schema.ts`:
- Around line 5-8: The schema currently allows empty comment bodies and uses
z.string({ message: ... }) which may not be valid in Zod v4; update schema by
making body required via z.string().nonempty({ message: 'Comment is required.'
}) (or .min(1, { message: ... }) if you prefer) and change listId to
z.string().nonempty({ message: 'List is required.' }).transform((val) => val as
ListId) so listId still casts to ListId; confirm Zod version and avoid passing a
message directly into z.string(...) if v4 requires using .nonempty/.min/.refine
for error messages.
In `@src/components/ListCommentForm/ListCommentForm.tsx`:
- Around line 79-101: The handlers handleApprove and handleReject should use
form.getValues() instead of form.watch() for an imperative snapshot; update both
to call validateForm(schema, { ...form.getValues(), ...forcedValues, userId:
user?._id }, form.setError) and then proceed to call onSubmit({
...validFormData, control: 'approved'|'rejected' }) and form.reset() as before
so the snapshot is taken once at click time and not via a subscription-oriented
API.
In `@src/components/ListDetails/ListDetails.module.scss`:
- Around line 5-10: The grid declares an unused "activity" row in .ListDetails'
grid-template-areas which creates an extra gap and &_Activity also lacks
grid-area: activity so it would auto-place; fix by either removing "activity"
from .ListDetails' grid-template-areas and grid-template-rows until the activity
feature is implemented (ensuring the two-row template and gaps match the
single-column layout), or add grid-area: activity to the &_Activity rule so the
activity element occupies the intended slot when rendered; update the horizontal
variant accordingly if you choose to remove the area.
In `@src/components/ListDetails/ListDetails.tsx`:
- Line 48: The call to useGetFileMetadata in ListDetails currently ignores the
error state causing PdfViewer to spin forever; update the hook destructure to
include the error (e.g., fileError) from useGetFileMetadata and handle it before
returning: if fileError render a compact error UI (message + retry button) or
pass the error into PdfViewer via props (so it can stop loading and show an
error), and ensure loading is only true while fetching and cleared on error so
the spinner is not perpetual.
In `@src/components/ListForm/ListForm.tsx`:
- Around line 95-147: Remove the large commented-out block in ListForm.tsx that
contains the old handleReplace variants, the file
render/storageId/useGetFileMetadata code, handleDownload, and the FUTURE
formations code (including useFieldArray, formationFields, appendFormation,
removeFormation, handleAddFormation); delete these commented lines so only
active implementation remains and ensure no live references to uploadFile,
storageId or formation helpers are left unused elsewhere in the file.
- Around line 50-55: The current useForm defaultValues spreads existingValues
which may contain non-form fields from the List (like
comments/availableActions); instead, only merge keys that belong to the FormData
shape. Modify the useForm call in ListForm (where defaultValues, existingValues,
forcedValues are merged) to pick/clone only the FormData keys from
existingValues before spreading (e.g., use a small helper or lodash.pick to
extract the FormData fields) so non-form properties are not injected into the
form state.
In `@src/components/ListProvider/actions/useEditAction.tsx`:
- Around line 17-42: The code uses user!._id in useEditAction when submitting
the ListForm which can crash if useAuth() returns null; remove the non-null
assertion and add a null guard: inside the onSubmit handler (where updateList is
called) check that user is present (e.g., if (!user) { toast.error('Not
authenticated'); close(); return; } or prevent submitting by disabling the
form), then pass user._id to updateList only when user is defined; update
references in useEditAction, ListForm onSubmit handling and any related logic to
handle the unauthenticated case gracefully instead of using user!.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
convex/_generated/api.d.tsis excluded by!**/_generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.eslintrc.cjsconvex/_model/files/actions/convertImageToPdf.tsconvex/_model/files/actions/getFileDownloadData.tsconvex/_model/files/index.tsconvex/_model/files/queries/getFileMetadata.tsconvex/_model/listComments/table.tsconvex/_model/lists/_helpers/deepenList.tsconvex/_model/lists/_helpers/getAvailableActions.tsconvex/_model/lists/mutations/createList.tsconvex/_model/lists/mutations/deleteList.tsconvex/_model/lists/mutations/updateList.tsconvex/_model/lists/queries/getList.tsconvex/_model/lists/queries/getListsByTournamentRegistration.tsconvex/_model/lists/queries/getListsByUser.tsconvex/_model/lists/types.tsconvex/_model/matchResults/queries/getMatchResults.tsconvex/_model/tournamentRegistrations/_helpers/deepenTournamentRegistration.tsconvex/_model/tournamentRegistrations/_helpers/getAvailableActions.tspackage.jsonsrc/components/CommentThread/CommentThread.tsxsrc/components/CommentThread/CommentThreadEvent.tsxsrc/components/CommentThread/index.tssrc/components/ContextMenu/ContextMenu.tsxsrc/components/ContextMenu/ContextMenu.types.tssrc/components/ListCommentForm/ListCommentForm.module.scsssrc/components/ListCommentForm/ListCommentForm.schema.tssrc/components/ListCommentForm/ListCommentForm.tsxsrc/components/ListDetails/ListDetails.module.scsssrc/components/ListDetails/ListDetails.tsxsrc/components/ListDetails/ListDetailsButton.tsxsrc/components/ListDetails/index.tssrc/components/ListForm/ListForm.module.scsssrc/components/ListForm/ListForm.tsxsrc/components/ListProvider/ListContextMenu.tsxsrc/components/ListProvider/ListProvider.hooks.tsxsrc/components/ListProvider/actions/useDeleteAction.tsxsrc/components/ListProvider/actions/useDownloadAction.tsxsrc/components/ListProvider/actions/useEditAction.tsxsrc/components/ListProvider/index.tssrc/components/generic/Timestamp/Timestamp.module.scsssrc/components/generic/Timestamp/Timestamp.utils.tssrc/pages/TournamentCompetitorDetailPage/components/TournamentRegistrationsTable/TournamentRegistrationsTable.tsxsrc/services/lists.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
src/components/ListForm/ListForm.tsx (3)
101-153: 🧹 Nitpick | 🔵 TrivialRemove the large commented-out legacy block.
This is dead code (duplicate handlers / unused helpers) and should be deleted rather than left commented.
🧹 Suggested cleanup
- // const handleReplace = async (files: FileList): Promise<void> => { - // if (files[0]) { - // await uploadFile(files[0]); - // } - // }; - - // File render - // const storageId = form.watch('storageId'); - // const { data: file } = useGetFileMetadata(storageId ? { id: storageId } : 'skip'); - - // const handleReplace = async (event: ChangeEvent<HTMLInputElement>): Promise<void> => { - // if (!event.target.files || event.target.files.length === 0) { - // throw new Error('You must select an image to upload.'); - // } - // const file = event.target.files[0]; - // if (user && file) { - // await uploadFile(file); - // } - // }; - - // const handleDownload = async (): Promise<void> => { - // if (!file) { - // return; - // } - // try { - // const response = await fetch(file.url); - // const blob = await response.blob(); - // const url = window.URL.createObjectURL(blob); - // const link = document.createElement('a'); - // link.href = url; - // link.download = `list.${file.url.split('.').pop()}`; - // document.body.appendChild(link); - // link.click(); - // document.body.removeChild(link); - // window.URL.revokeObjectURL(url); - // } catch (err) { - // console.error('Download failed:', err); - // } - // }; - - // FUTURE: Add formations array - // const { fields: formationFields, append: appendFormation, remove: removeFormation } = useFieldArray({ - // control: form.control, - // name: 'data.formations', - // }); - // const handleAddFormation = (e: MouseEvent): void => { - // e.preventDefault(); - // appendFormation({ - // id: nanoid(), - // sourceId: PLACEHOLDER_UNIT_OPTIONS[0].value, - // }); - // };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListForm/ListForm.tsx` around lines 101 - 153, Remove the large commented-out legacy block in ListForm.tsx that contains duplicate/unused handlers and helpers (the commented handleReplace variations, the commented file render lines including storageId and useGetFileMetadata, the commented handleDownload, and the commented useFieldArray/formationFields/appendFormation/removeFormation/handleAddFormation). Delete the entire commented section so only active code remains; if any functionality from uploadFile, useGetFileMetadata or formation handling is still required, reintroduce it as a clean, uncommented implementation in the appropriate functions instead of leaving commented stubs.
86-99:⚠️ Potential issue | 🟠 MajorHonor
disabledin the dropzone state.
isDisabledignores thedisabledprop, so uploads can still occur while the form is read-only.🔧 Suggested fix
- const isDisabled = loading || isUploadLoading; + const isDisabled = disabled || loading || isUploadLoading;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListForm/ListForm.tsx` around lines 86 - 99, The dropzone ignores the component's disabled prop so uploads still work when the form is read-only; update the isDisabled computation to include the component prop (e.g., change "const isDisabled = loading || isUploadLoading;" to "const isDisabled = loading || isUploadLoading || disabled;") and keep passing isDisabled into useDropzone (disabled/noClick/noKeyboard) and the onDrop guard, so the dropzone truly respects the disabled state (references: isDisabled, useDropzone, onDrop, uploadFile).
50-55: 🧹 Nitpick | 🔵 TrivialSanitize
existingValuesbefore spreading into form defaults.
existingValuesis aListand likely includes non-form fields (e.g., user, comments, availableActions). Spreading it intodefaultValuesis noisy and risks collisions if field names overlap in the future.♻️ Suggested fix
- const form = useForm<FormData>({ - defaultValues: { - ...defaultValues, - ...existingValues, - ...forcedValues, - }, - mode: 'onSubmit', - }); + const sanitizedExisting = existingValues ? { + gameSystem: existingValues.gameSystem, + storageId: existingValues.storageId, + tournamentRegistrationId: existingValues.tournamentRegistrationId, + } : undefined; + const form = useForm<FormData>({ + defaultValues: { + ...defaultValues, + ...sanitizedExisting, + ...forcedValues, + }, + mode: 'onSubmit', + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListForm/ListForm.tsx` around lines 50 - 55, The current defaultValues spread includes the full existingValues object (from List) which may contain non-form fields; before passing to useForm<FormData> (the defaultValues/ existingValues/ forcedValues merge), create a sanitized object that only contains the specific form fields in FormData (e.g., pick title, description, tags, etc. used by the form) and spread that sanitizedExistingValues instead of the raw existingValues; update the ListForm component to derive sanitizedExistingValues (or use a helper like pickFormFields) and use it in the useForm defaultValues merge to avoid leaking unrelated fields like user, comments, or availableActions.src/components/ListDetails/ListDetails.tsx (2)
57-59:⚠️ Potential issue | 🟡 MinorDifferentiate “loading” vs “not found”.
A resolved
nulllist will keep the spinner up indefinitely. Return an explicit empty/not-found state after loading finishes.🔧 Suggested fix
- if (listLoading || !list) { - return <Spinner />; - } + if (listLoading) { + return <Spinner />; + } + if (!list) { + return <div>List not found.</div>; // replace with your app’s empty state + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListDetails/ListDetails.tsx` around lines 57 - 59, The component currently conflates loading and missing data by returning <Spinner /> when either listLoading or !list is true; update ListDetails to first check listLoading and return <Spinner /> only while loading, then after loading finishes check if list is null/undefined and return an explicit "not found" or empty state (e.g., a NotFound component or a friendly message) instead of the spinner; modify the conditional around listLoading and list in the ListDetails render (referencing listLoading, list, Spinner, and the ListDetails component) so loading and not-found states are rendered distinctly.
48-70:⚠️ Potential issue | 🟡 MinorHandle file metadata errors to avoid a stuck viewer.
If metadata lookup fails, the viewer can remain in a loading state with no feedback.
🔧 Suggested fix
- const { data: file, loading } = useGetFileMetadata(list ? { id: list.storageId } : 'skip'); + const { data: file, loading, error } = useGetFileMetadata(list ? { id: list.storageId } : 'skip'); ... - <PdfViewer + {error ? ( + <div>Unable to load list file.</div> + ) : ( + <PdfViewer className={clsx(styles.ListDetails_Viewer, ...getStyleClassNames({ border: inset ? 'top' : true, corners: inset ? false : 'normal', }))} file={file?.url ?? null} loading={loading} - /> + /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ListDetails/ListDetails.tsx` around lines 48 - 70, The file metadata call (useGetFileMetadata) can fail and leave the PdfViewer stuck loading; update the component to read the error result from useGetFileMetadata (alongside data/file and loading) and handle the error case: if error or if loading is false and file is null, render a fallback (e.g., an error message or placeholder) or pass loading={false} and file={null} to PdfViewer to avoid an indefinite spinner; ensure this logic runs before rendering PdfViewer and also consider the existing listLoading check so you only show the fallback when listLoading is false and list exists.convex/_model/files/actions/convertImageToPdf.ts (2)
26-28:⚠️ Potential issue | 🟡 MinorAvoid echoing raw MIME type in the error message.
args.mimeTypeis user-controlled input. Prefer a generic error string to avoid echoing raw input.🔧 Suggested fix
- if (!isImageMimeType(args.mimeType)) { - throw new ConvexError({ message: `Unsupported MIME type: ${args.mimeType}. Expected one of: ${IMAGE_MIME_TYPES.join(', ')}` }); - } + if (!isImageMimeType(args.mimeType)) { + throw new ConvexError({ message: `Unsupported MIME type. Expected one of: ${IMAGE_MIME_TYPES.join(', ')}` }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/files/actions/convertImageToPdf.ts` around lines 26 - 28, The current throw in convertImageToPdf.ts echoes user-controlled args.mimeType; change the ConvexError thrown in the isImageMimeType check so it uses a generic message (e.g., "Unsupported MIME type") and optionally list allowed types from IMAGE_MIME_TYPES without including the raw args.mimeType; update the throw in the block that calls isImageMimeType(args.mimeType) to remove args.mimeType from the message while keeping context (use IMAGE_MIME_TYPES.join(', ') or similar).
12-15:⚠️ Potential issue | 🟡 MinorDocument or normalize
image/jpgin the allow-list.
image/jpgis non-standard; if it’s kept for compatibility, add a short comment to explain why, or normalize upstream toimage/jpeg.📝 Suggested clarification
-const IMAGE_MIME_TYPES = ['image/png', 'image/jpeg', 'image/jpg'] as const; +const IMAGE_MIME_TYPES = ['image/png', 'image/jpeg', 'image/jpg'] as const; +// NOTE: 'image/jpg' is kept for legacy clients that send the non-standard type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/files/actions/convertImageToPdf.ts` around lines 12 - 15, IMAGE_MIME_TYPES currently includes the non‑standard 'image/jpg'; either normalize inputs or document why it’s present. Remove 'image/jpg' from IMAGE_MIME_TYPES and ensure callers normalize 'image/jpg' to 'image/jpeg', or keep it but add a short comment above IMAGE_MIME_TYPES describing that 'image/jpg' is included for historical/compatibility reasons and add normalization in isImageMimeType (e.g., map 'image/jpg' to 'image/jpeg' before checking). Update references to ImageMimeType and isImageMimeType accordingly so all upstream consumers receive the canonical 'image/jpeg' value.convex/_model/files/actions/getFileDownloadData.ts (1)
31-35:⚠️ Potential issue | 🟠 MajorAvoid O(n²) byte‑to‑string concatenation.
The current loop allocates a new string on every byte. Use chunked conversion to keep it linear and call‑stack safe.
⚡ Suggested fix
- let binary = ''; - for (let i = 0; i < bytes.length; i++) { - binary += String.fromCharCode(bytes[i]); - } - return btoa(binary); + const CHUNK = 0x8000; // 32 KB + let binary = ''; + for (let i = 0; i < bytes.length; i += CHUNK) { + binary += String.fromCharCode(...bytes.subarray(i, i + CHUNK)); + } + return btoa(binary);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/files/actions/getFileDownloadData.ts` around lines 31 - 35, The byte-to-base64 conversion inside getFileDownloadData is O(n²) because it builds string via repeated concatenation of "binary" from "bytes"; replace that loop with a chunked approach: iterate over "bytes" in fixed-size chunks (e.g. 0x8000), for each chunk convert it to a string with String.fromCharCode applied to the chunk or by creating a temporary chunk array and pushing the resulting string into an array, then join the array once and pass the final string to btoa("..."); ensure you use the existing variables "bytes", "binary" (or replace it with a chunks array) and return btoa(...) so the change remains local to getFileDownloadData.convex/_model/lists/_helpers/deepenList.ts (1)
63-67: 🧹 Nitpick | 🔵 TrivialParallelize independent lookups in the return object.
Line 63-67:
getAvailableActionsandcheckListSubmittedOnTimeare independent and can run inPromise.allto reduce latency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/_model/lists/_helpers/deepenList.ts` around lines 63 - 67, getAvailableActions(ctx, doc) and checkListSubmittedOnTime(ctx, doc) are independent but currently awaited sequentially in the returned object; run them in parallel using Promise.all to reduce latency by invoking Promise.all([getAvailableActions(ctx, doc), checkListSubmittedOnTime(ctx, doc)]) and then assign the results to availableActions and onTime respectively before returning the object (refer to symbols getAvailableActions, checkListSubmittedOnTime, availableActions, onTime, ctx, doc).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/_model/lists/_helpers/deepenList.ts`:
- Around line 48-53: The teammate authorization currently uses
checkUsersAreTeammates(ctx, tournamentRegistration.userId, currentUserId) which
checks teammates globally; change it to scope to this tournament competitor by
using tournamentRegistration.tournamentCompetitorId (or call a new helper like
checkUserIsCompetitorTeammate(ctx,
tournamentRegistration.tournamentCompetitorId, currentUserId)) so only teammates
within this competitor for the current tournament are considered; update the
authorizedRoles array (and any references to isAuthorized) to use that scoped
check alongside checkUserIsTournamentOrganizer(ctx, tournament._id,
currentUserId) and doc.userId === currentUserId.
- Around line 37-41: The query for list comments in deepenList.ts uses
ctx.db.query('listComments').withIndex('by_list', (q) => q.eq('listId',
doc._id)).collect() which does not enforce creation-time ordering, so
lastControlComment (used by isApproved on line 54) can be non-deterministic;
modify the query to apply ordering (use .order('desc') on the index query before
.collect()) so the results are deterministically newest-first and
lastControlComment selection is stable.
In `@src/components/ListProvider/actions/useDownloadAction.tsx`:
- Around line 16-31: The current handler in useDownloadAction.tsx sets link.href
to subject.fileUrl which will ignore the download attribute for cross-origin
storage URLs; change handler to fetch the file bytes (e.g., via
fetch(subject.fileUrl) or existing getFileDownloadData), convert the response to
a Blob, create a same-origin object URL with URL.createObjectURL(blob), set
link.href to that object URL and link.download to `${subject.displayName ??
'list'}.pdf`, click the link, then revoke the object URL with
URL.revokeObjectURL(objectUrl) and remove the link in finally; ensure errors
still trigger toast.error and handle large-file memory implications as
appropriate.
---
Duplicate comments:
In `@convex/_model/files/actions/convertImageToPdf.ts`:
- Around line 26-28: The current throw in convertImageToPdf.ts echoes
user-controlled args.mimeType; change the ConvexError thrown in the
isImageMimeType check so it uses a generic message (e.g., "Unsupported MIME
type") and optionally list allowed types from IMAGE_MIME_TYPES without including
the raw args.mimeType; update the throw in the block that calls
isImageMimeType(args.mimeType) to remove args.mimeType from the message while
keeping context (use IMAGE_MIME_TYPES.join(', ') or similar).
- Around line 12-15: IMAGE_MIME_TYPES currently includes the non‑standard
'image/jpg'; either normalize inputs or document why it’s present. Remove
'image/jpg' from IMAGE_MIME_TYPES and ensure callers normalize 'image/jpg' to
'image/jpeg', or keep it but add a short comment above IMAGE_MIME_TYPES
describing that 'image/jpg' is included for historical/compatibility reasons and
add normalization in isImageMimeType (e.g., map 'image/jpg' to 'image/jpeg'
before checking). Update references to ImageMimeType and isImageMimeType
accordingly so all upstream consumers receive the canonical 'image/jpeg' value.
In `@convex/_model/files/actions/getFileDownloadData.ts`:
- Around line 31-35: The byte-to-base64 conversion inside getFileDownloadData is
O(n²) because it builds string via repeated concatenation of "binary" from
"bytes"; replace that loop with a chunked approach: iterate over "bytes" in
fixed-size chunks (e.g. 0x8000), for each chunk convert it to a string with
String.fromCharCode applied to the chunk or by creating a temporary chunk array
and pushing the resulting string into an array, then join the array once and
pass the final string to btoa("..."); ensure you use the existing variables
"bytes", "binary" (or replace it with a chunks array) and return btoa(...) so
the change remains local to getFileDownloadData.
In `@convex/_model/lists/_helpers/deepenList.ts`:
- Around line 63-67: getAvailableActions(ctx, doc) and
checkListSubmittedOnTime(ctx, doc) are independent but currently awaited
sequentially in the returned object; run them in parallel using Promise.all to
reduce latency by invoking Promise.all([getAvailableActions(ctx, doc),
checkListSubmittedOnTime(ctx, doc)]) and then assign the results to
availableActions and onTime respectively before returning the object (refer to
symbols getAvailableActions, checkListSubmittedOnTime, availableActions, onTime,
ctx, doc).
In `@src/components/ListDetails/ListDetails.tsx`:
- Around line 57-59: The component currently conflates loading and missing data
by returning <Spinner /> when either listLoading or !list is true; update
ListDetails to first check listLoading and return <Spinner /> only while
loading, then after loading finishes check if list is null/undefined and return
an explicit "not found" or empty state (e.g., a NotFound component or a friendly
message) instead of the spinner; modify the conditional around listLoading and
list in the ListDetails render (referencing listLoading, list, Spinner, and the
ListDetails component) so loading and not-found states are rendered distinctly.
- Around line 48-70: The file metadata call (useGetFileMetadata) can fail and
leave the PdfViewer stuck loading; update the component to read the error result
from useGetFileMetadata (alongside data/file and loading) and handle the error
case: if error or if loading is false and file is null, render a fallback (e.g.,
an error message or placeholder) or pass loading={false} and file={null} to
PdfViewer to avoid an indefinite spinner; ensure this logic runs before
rendering PdfViewer and also consider the existing listLoading check so you only
show the fallback when listLoading is false and list exists.
In `@src/components/ListForm/ListForm.tsx`:
- Around line 101-153: Remove the large commented-out legacy block in
ListForm.tsx that contains duplicate/unused handlers and helpers (the commented
handleReplace variations, the commented file render lines including storageId
and useGetFileMetadata, the commented handleDownload, and the commented
useFieldArray/formationFields/appendFormation/removeFormation/handleAddFormation).
Delete the entire commented section so only active code remains; if any
functionality from uploadFile, useGetFileMetadata or formation handling is still
required, reintroduce it as a clean, uncommented implementation in the
appropriate functions instead of leaving commented stubs.
- Around line 86-99: The dropzone ignores the component's disabled prop so
uploads still work when the form is read-only; update the isDisabled computation
to include the component prop (e.g., change "const isDisabled = loading ||
isUploadLoading;" to "const isDisabled = loading || isUploadLoading ||
disabled;") and keep passing isDisabled into useDropzone
(disabled/noClick/noKeyboard) and the onDrop guard, so the dropzone truly
respects the disabled state (references: isDisabled, useDropzone, onDrop,
uploadFile).
- Around line 50-55: The current defaultValues spread includes the full
existingValues object (from List) which may contain non-form fields; before
passing to useForm<FormData> (the defaultValues/ existingValues/ forcedValues
merge), create a sanitized object that only contains the specific form fields in
FormData (e.g., pick title, description, tags, etc. used by the form) and spread
that sanitizedExistingValues instead of the raw existingValues; update the
ListForm component to derive sanitizedExistingValues (or use a helper like
pickFormFields) and use it in the useForm defaultValues merge to avoid leaking
unrelated fields like user, comments, or availableActions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.eslintrc.cjsconvex/_model/files/actions/convertImageToPdf.tsconvex/_model/files/actions/getFileDownloadData.tsconvex/_model/lists/_helpers/deepenList.tssrc/components/ListDetails/ListDetails.tsxsrc/components/ListForm/ListForm.tsxsrc/components/ListProvider/actions/useDownloadAction.tsx
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.
Resolves: #50
Summary by CodeRabbit
New Features
Improvements
Chores