fix(deck): make ghost card rows distinguishable for a11y and scope their Trash to the membership#91
Merged
Merged
Conversation
… the membership
Ghost rows (secondary-category fan-out copies of a multi-category DeckCard)
were indistinguishable to assistive tech and their Trash deleted the card from
the whole deck.
- Record the section category on each fan-out copy (groupByCategory) so a ghost
knows which membership it represents.
- Add an "(also in {primary category})" qualifier to ghost rows/tiles across the
sortable and read-only variants.
- Ghost-row Trash now strips only that secondary membership via setCardCategories;
the primary row keeps full-delete semantics.
Refs #89 (M10 from #30 multi-category cards review).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGhost category entries now retain their originating section category. Row and stack controls identify secondary membership and remove only that membership, while primary entries continue deleting the card from the deck. Tests cover metadata, accessibility labels, and removal side effects. ChangesGhost membership handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant CardRowSortable
participant DeckEditorState
participant DeckCategoriesActions
Editor->>CardRowSortable: remove secondary category entry
CardRowSortable->>DeckEditorState: dispatch move with remaining categories
CardRowSortable->>DeckCategoriesActions: persist remaining categories
DeckCategoriesActions-->>CardRowSortable: complete category update
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@app/_components/builder/card-row-sortable.tsx`:
- Around line 183-198: Update the ghost-row keyboard deletion wiring in
handleRowKeyDown to pass dc.isSecondary ? removeMembership : remove, ensuring
secondary rows strip only their membership while primary rows retain full
deletion behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f6ebf322-155c-4e8d-b265-312d7bed6104
📒 Files selected for processing (10)
CONTEXT.mdapp/_components/builder/card-row-sortable.test.tsxapp/_components/builder/card-row-sortable.tsxapp/_components/builder/card-row.test.tsxapp/_components/builder/card-row.tsxapp/_components/builder/card-stack-sortable.tsxapp/_components/builder/card-stack.tsxlib/deck/__tests__/group-sort.test.tslib/deck/group-sort.tslib/deck/zone-view.ts
Keyboard Delete/Backspace on a ghost (secondary) row passed only `remove` into handleRowKeyDown, fully deleting the DeckCard instead of stripping its secondary-category membership. Wire the keyboard path to route `dc.isSecondary ? removeMembership : remove`, mirroring the Trash button. Add regression tests covering Delete/Backspace on both ghost and primary rows.
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.
Intent
Give multi-category ghost rows an accessible qualifier and make their Trash remove only the secondary-category membership, not delete the whole card
What Changed
DeckCardCategorymembership rather than deleting the wholeDeckCard; primary-row Trash still removes the card from the deck. Threaded throughcard-row-sortable.tsx,card-row.tsx,card-stack-sortable.tsx,card-stack.tsx,group-sort.ts, andzone-view.ts.CONTEXT.md.Risk Assessment
✅ Low: Small, well-bounded a11y change that follows established component patterns; optimistic dispatch and server action wiring are correct and consistent with sibling mutations.
Testing
Ran the change's vitest suite (51 pass) covering the qualifier text, both Trash aria-labels, and that ghost Trash calls setCardCategories rather than removeCardFromDeck. Then verified end-to-end in the running app: seeded a multi-category deck, signed in as owner, and confirmed in the accessibility tree that the ghost Sol Ring row shows the sr-only "(also in ramp)" qualifier with a membership-scoped Trash label, while primary rows keep "from deck". Clicking the ghost Trash dropped the removal count 2→1, left Sol Ring in ramp, and the database confirmed only the removal membership was deleted (card row intact) — proving no whole-card deletion. Qualifier evidence is an accessibility-tree capture because the span is sr-only by design; screenshots show the editor surface but not the invisible text.
/var/folders/b2/rft4yv4177d6mpkbhbwj7svc0000gn/T/no-mistakes-evidence/01KXECW6GEKBG3Q0A5RTPW4P47/01-read-view-ghost.png)/var/folders/b2/rft4yv4177d6mpkbhbwj7svc0000gn/T/no-mistakes-evidence/01KXECW6GEKBG3Q0A5RTPW4P47/02-editor-ghost-trash-labels.png)/var/folders/b2/rft4yv4177d6mpkbhbwj7svc0000gn/T/no-mistakes-evidence/01KXECW6GEKBG3Q0A5RTPW4P47/03-after-ghost-trash.png)Evidence: Accessibility-tree + DB before/after evidence
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
app/_components/builder/card-row-sortable.tsx:277- Ghost-row sr-only qualifier announces only the primary category (dc.categories[0]). For a card with 3+ memberships (e.g. primary A, secondaries B and C), the ghost under C reads "(also in A)" and never mentions B, and the announcement doesn't name the section the ghost currently sits in (that context comes only from the section heading). Acceptable since the primary is the most useful single anchor, but the qualifier is incomplete for many-category cards.✅ **Test** - passed
✅ No issues found.
pnpm vitest run app/_components/builder/card-row-sortable.test.tsx app/_components/builder/card-row.test.tsx lib/deck/__tests__/group-sort.test.ts app/_components/builder/card-stack.tsx— 51 passSeeded a COMMANDER deck (Sol Ring primary inramp, secondary/ghost inremoval; Lightning Bolt inremoval) and drove/deck/ghostdeck0001in Chrome as the signed-in ownerCaptured accessibility tree: ghost row exposes sr-only(also in ramp), is non-draggable, and its Trash readsRemove Sol Ring from removal; primary rows readRemove … from deckClickedRemove Sol Ring from removal: removal section 2/12→1/12, Sol Ring remained in rampSQL check: only theremovaldeck_card_category row deleted; Sol Ring deck_card row (qty 1, MAINBOARD) preserved — card not deleted✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit