fix(deck): tolerate stale deleted category in move-card menu#94
Conversation
A category can be deleted (by this user in another tab or by a collaborator) while the move-card menu is open and still showing it. Toggling that stale category previously threw inside startTransition, tripping the error boundary instead of failing gracefully. Defense on both sides: - Client (`applyCategories`) filters memberships against the live `subcategories` list before dispatch via the new exported `filterLiveCategories` helper, so a stale entry is never sent. - Server actions silently drop / degrade unknown category names instead of throwing: `setCardCategories` filters them out, and `moveCardTo` degrades a deleted target to an uncategorized MAINBOARD move. Regression tests cover the client filter and both server-action paths.
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCategory updates now tolerate deleted categories. The builder filters stale memberships before dispatching, while deck actions drop unknown names and convert missing MAINBOARD targets into uncategorized moves. Tests cover surviving memberships, no-op behavior, and fallback moves. ChangesStale Category Tolerance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/_actions/deck/categories.ts (1)
286-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicate category existence logic into a shared helper.
The query to verify whether a category exists duplicates the logic found in
assertCategoryExists(lines 51-62). Consider extracting a shared helper function (e.g.,checkCategoryExists) to centralize this database lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/_actions/deck/categories.ts` around lines 286 - 297, The category existence lookup near effectiveCategory duplicates assertCategoryExists. Extract the shared deckCategory findUnique query into a helper such as checkCategoryExists, update assertCategoryExists and the effectiveCategory fallback logic to reuse it, and preserve the existing behavior for missing categories.
🤖 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.
Nitpick comments:
In `@app/_actions/deck/categories.ts`:
- Around line 286-297: The category existence lookup near effectiveCategory
duplicates assertCategoryExists. Extract the shared deckCategory findUnique
query into a helper such as checkCategoryExists, update assertCategoryExists and
the effectiveCategory fallback logic to reuse it, and preserve the existing
behavior for missing categories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 674cbe40-a0cd-4bf6-a46e-daceb2ca2979
📒 Files selected for processing (4)
app/_actions/deck/__tests__/categories.test.tsapp/_actions/deck/categories.tsapp/_components/builder/__tests__/move-card-menu.test.tsxapp/_components/builder/move-card-menu.tsx
assertCategoryExists and the moveCardTo effectiveCategory fallback ran the identical deckCategory.findUnique existence lookup. Extract it into a shared checkCategoryExists helper both reuse; missing-category behavior is unchanged (assert still throws, moveCardTo still degrades to an uncategorized move).
1 participant
Intent
Fix issue #88: a stale deleted category left in the open move-card menu threw an unhandled error inside startTransition and tripped the error boundary when the user (in another tab) or a collaborator deleted that category while the menu was open. Fix degrades gracefully on BOTH sides: (1) client filter (filterLiveCategories in move-card-menu.tsx) drops any category name no longer in the live subcategories registry before dispatch/send; (2) server action (setCardCategories, moveCardTo effectiveCategory fallback in app/_actions/deck/categories.ts) silently drops unknown category names instead of throwing. Two previously throw-asserting tests were intentionally flipped to assert graceful drop/degrade, plus new filterLiveCategories tests. THIS FOLLOW-UP COMMIT (a7145da) is a review nitpick: the category-existence lookup near effectiveCategory duplicated assertCategoryExists, so I extracted the shared deckCategory.findUnique into a checkCategoryExists helper that both assertCategoryExists and the effectiveCategory fallback reuse — behavior preserved exactly, minimal change. 58 tests pass, lint clean.
What Changed
move-card-menu.tsxnow filters category names against the live subcategories registry (filterLiveCategories) before dispatch/send, so a category deleted while the menu is open no longer throws insidestartTransitionand trips the error boundary.setCardCategoriesand themoveCardToeffectiveCategoryfallback (app/_actions/deck/categories.ts) silently drop unknown category names instead of throwing; the shareddeckCategory.findUniquelookup is extracted into acheckCategoryExistshelper reused byassertCategoryExistsand the fallback.filterLiveCategoriestests.Risk Assessment
✅ Low: Small, well-bounded graceful-degrade fix plus a behavior-preserving helper extraction; matches stated intent with tests updated, no material bugs found.
Testing
Installed deps (pnpm, frozen lockfile) and generated the Prisma client with a dummy DATABASE_URL to unblock the mocked-prisma unit suites, then ran the two affected test files: 64/64 pass. The suites directly reproduce the issue #88 race — a stale/deleted category left in the open move-card menu — and assert graceful degradation on both sides (client filter drops the stale name; server actions drop unknown names instead of throwing into the error boundary), including the two intentionally-flipped throw→drop assertions and the new filterLiveCategories tests. The follow-up commit under test is a pure helper-extraction refactor and the moveCardTo degrade test still exercises the shared checkCategoryExists path, confirming behavior is preserved. No browser screenshot/GIF was captured: this commit adds no rendered UI surface — move-card-menu.tsx's change is a defensive pure function and the fix's user-facing outcome is a non-event (the absence of an error-boundary crash), which the targeted client/server tests reproduce exactly; a full two-tab browser reproduction of the cross-tab deletion race would need a live Postgres + dev server and adds little over the tests that reproduce the precise race outcome. Working tree is clean; install artifacts (node_modules, lib/generated) are gitignored.
Evidence: Graceful-degrade test transcript (issue #88, both sides)
✓ server setCardCategories > silently drops a stale category no longer in the deck registry (issue #88) ✓ server setCardCategories > no-ops when every requested category is stale (issue #88) ✓ server moveCardTo > degrades to an uncategorized MAINBOARD move when the target subcategory was deleted (issue #88) ✓ client filterLiveCategories > drops a stale membership no longer in the live registry ✓ client filterLiveCategories > preserves order of the surviving memberships ✓ client filterLiveCategories > returns an empty list when every name is stale Test Files 2 passed (2) | Tests 64 passed (64)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
app/_actions/deck/categories.ts:303- moveCardTo's degrade path sets effectiveCategory=null when the target category was deleted, which makes nextCategories=[] and clears ALL of the card's existing valid category memberships, not just the deleted one. setCardCategories (the other graceful-drop path) only filters out the unknown name and keeps the rest. This divergence is authorized by the stated intent ('degrade to an uncategorized MAINBOARD move') but is more destructive than the sibling path — a collaborator deleting one category could wipe a card's other memberships on a move.✅ **Test** - passed
✅ No issues found.
vitest run app/_actions/deck/__tests__/categories.test.ts app/_components/builder/__tests__/move-card-menu.test.tsx— 64 passedserver graceful-degrade:setCardCategories > silently drops a stale category (issue #88),setCardCategories > no-ops when every requested category is stale (issue #88),moveCardTo > degrades to an uncategorized MAINBOARD move when the target subcategory was deleted (issue #88)client pure-fn:filterLiveCategories (issue #88)— drops stale membership, preserves order, returns empty when all staleConfirmed the extractedcheckCategoryExistspath still drives themoveCardTodegrade test (helper reuse, behavior preserved)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.