Harden launch papercuts and clean builds#105
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR tightens Convex useQuery typings across pages, removes unsafe casts in dataset list rendering, adds client-side dataset schema validation and normalization, applies the same normalization server-side in the Convex create mutation (throwing ConvexError for validation failures), migrates XLSX export from SheetJS to write-excel-file with formula-injection neutralization (also used for CSV), refactors table flashing timers, precomputes analytics identity values, removes dev console lint suppressions, and updates dependencies and ESLint ignores. Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Convex
participant Database
User->>Frontend: Enter dataset name & columns
Frontend->>Frontend: getSchemaValidationError()
alt Validation fails
Frontend->>User: Display error, block create
else Validation passes
Frontend->>Frontend: Normalize input (trim/coerce)
Frontend->>Convex: createDataset(normalizedInput)
Convex->>Convex: normalizeCreateDatasetInput(args)
alt Normalize fails
Convex->>Frontend: ConvexError
Frontend->>User: Display error
else Normalize succeeds
Convex->>Database: Insert normalized dataset
Database->>Convex: Persist complete
Convex->>Frontend: Success
Frontend->>User: Show created dataset
end
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
giaphutran12
left a comment
giaphutran12
left a comment
There was a problem hiding this comment.
Launch-gate pass from the Edward account: no changes requested from my review.
Evidence: temp worktree bun run lint exited 0 with existing warnings only; bun run build passed when run with dummy public build env (NEXT_PUBLIC_CONVEX_URL, NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY, NEXT_PUBLIC_BACKEND_URL) so no real env files were read. Checks are green.
I cannot approve/merge this from the same GitHub account that authored it; it still needs a non-author review to clear the required review gate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/convex/datasets.ts`:
- Around line 49-87: normalizeCreateDatasetInput currently trims
args.description but doesn't validate it; add a check that if args.description
is provided (truthy) then args.description.trim() is non-empty and throw an
Error (e.g., "Dataset description is required.") if it is empty; use the trimmed
value in the returned object (replace description: args.description.trim() with
the validated trimmedDescription or undefined) so whitespace-only descriptions
are rejected and true descriptions are stored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecac9cb6-7b5b-4be8-bf89-daaf27d506c8
📒 Files selected for processing (2)
README.mdfrontend/convex/datasets.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
MMeteorL
left a comment
MMeteorL
left a comment
There was a problem hiding this comment.
This is a really solid cleanup PR, just some minor concerns here. Replacing SheetJS was the right call, and the refactor into clean helper functions is much more readable and testable than the old monolithic export code. The decision to commit Convex generated files for zero-setup checkout is a genuine developer experience win. The create-dataset validation work, both client-side pre-flight and server-side normalization, closes a real gap where blank names and duplicate columns could silently create broken datasets.
For the comments:
These should block merge:
-
frontend/convex/_generated/api.d.ts:
The committed stubs use AnyApi which is fully untyped — every api.datasets.* and api.datasetRows.* call becomes any, silently disabling TypeScript type-checking for all Convex call sites across the whole frontend. The version already committed on other branches (ApiFromModules<{...}>) has proper inference. Use that approach instead — generate with a live instance or in CI, not a hand-written stub. -
frontend/convex/datasets.ts:
Plain Error thrown from a Convex mutation is treated as an internal server error — the message is stripped before it reaches the client. Users who bypass the frontend pre-flight (direct API call, or future validation drift) will see a generic "Server Error" with no actionable message. Use throw new ConvexError("Dataset name is required.") from convex/values.
The rest would be optional improvement.
|
|
||
| import type { AnyApi, AnyComponents } from "convex/server"; | ||
|
|
||
| export declare const api: AnyApi; |
There was a problem hiding this comment.
Committed stub types use AnyApi (fully untyped), silently eliminating TypeScript type-checking for every Convex call site in the frontend.
Suggested fix:
Replace AnyApi stubs with a properly generated file (run npx convex dev with a live instance and commit the result, or keep _generated/ gitignored and generate it in CI)
|
|
||
| function normalizeCreateDatasetInput(args: CreateDatasetInput) { | ||
| const name = args.name.trim(); | ||
| if (!name) { |
There was a problem hiding this comment.
normalizeCreateDatasetInput throws plain new Error(), not ConvexError — server validation failures surface as a generic opaque error on the client.
Suggested Fix:
Replace throw new Error(...) with throw new ConvexError(...) from convex/values for all user-facing validation messages in mutations
| }, FLASH_DURATION_MS); | ||
| flashTimersRef.current.add(timer); | ||
|
|
||
| const removeTimer = setTimeout(() => { |
There was a problem hiding this comment.
Nested setTimeout(0) + FLASH_DURATION_MS pattern: two rapid row updates share the first update's remove-timer, cutting the second flash short.
Suggested fix:
Track newFlashes per-timer by closing over it; each removeTimer should only clear the keys from its own addTimer batch
There was a problem hiding this comment.
Also, removeTimer created inside addTimer's callback can escape cleanup if the component unmounts in the same event-loop tick as a flash is triggered.
Suggested fix:
Return a cleanup from the [rows] effect that cancels the pending addTimer; or move addTimer to the ref so cleanup can cancel it
| const headers = table.getHeaderGroups()[0]?.headers ?? []; | ||
| const tableRows = table.getRowModel().rows; | ||
| const columnWidths = useMemo(() => headers.map((h) => h.getSize()), [headers]); | ||
| const columnWidths = headers.map((header) => header.getSize()); |
There was a problem hiding this comment.
Moving columnWidths from useMemo to an inline expression produces a new array reference every render, causing downstream itemData memoization to always miss
Suggested fix:
Re-wrap in useMemo and also memoize headers (e.g. via useCallback on table.getHeaderGroups) to make the memoization chain actually effective.
|
Addressed the latest CodeRabbit + Mengzhe review blockers in bb725f6:
Verification:
CodeRabbit is already reviewing the new head. @MMeteorL could you re-review when you get a chance? |
…rdening # Conflicts: # frontend/app/dashboard/page.tsx # frontend/app/dataset/[id]/page.tsx # frontend/app/dataset/new/page.tsx # frontend/app/page.tsx # frontend/components/table/use-row-change-detection.ts # frontend/convex/_generated/api.d.ts # frontend/convex/_generated/server.d.ts # frontend/convex/_generated/server.js # frontend/convex/datasets.ts
MMeteorL
left a comment
MMeteorL
left a comment
There was a problem hiding this comment.
LGTM! @simantak-dabhade Please review if this branch could be approved
2 participants
Summary
Verification