feat(builder): add card browser with filters, syntax search, and responsive surfaces#64
Merged
Conversation
New in-deck card browser for searching and bulk-adding cards without leaving the deck editor. Backed by an offset-paginated browse API and a Filters tab that round-trips through the existing syntax query string. Key changes: - card-browser/: browser UI (modes, density toggle, scry tray, side panel, filter builder, target picker, bulk bar, in-deck badges) plus deck-browser context and use-card-browser hook - api/cards/browse: paginated browse route over searchCardsBySyntax - search: add OFFSET pagination to searchCardsBySyntax; add serializeWhere as the inverse of parseSyntax so Filters tab and syntax box share one source-of-truth query - deck-search-context: browseTick/requestBrowse to open the browser - header-search + deck-builder: wire the browse entry point and styles - tests: serialize-where round-trip, browse route, filter-builder
Polish the card browser added in the previous commit so it stays out of the way and dismisses naturally on mobile and desktop. Key changes: - Close the scry tray and side panel on outside pointer-down (ignoring Radix dropdown portals), replacing the explicit chevron close button - Render BulkBar inline as a full-width strip inside the tray instead of a floating pill, via a new `inline` prop - Hide the filmstrip until there's a query or active filter so an empty tray no longer covers the deck - Gate the filter-builder card-name field behind a `showName` prop; the tray's query bar already takes names, only the side panel shows it - Cap tray height and add safe-area padding; let toolbar and action row rows wrap on narrow widths
Contributor
|
Warning Review limit reached
More reviews will be available in 13 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive card browser feature for the deck builder, enabling players to search, filter, and add cards to their decks via desktop side-panel and mobile filmstrip interfaces. The implementation spans search infrastructure, interactive filtering UI, deck context operations, desktop/mobile browsing layouts, and integration into the main deck builder and search toolbar. ChangesCard Browser & Deck Operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
The card-browser surface shipped with use-card-browser, use-media-query, and browser-state untested, dropping global coverage below the 100/99 thresholds and failing CI. Key changes: - Add use-card-browser.test.tsx: fetch/debounce, empty-query gate, hasMore paging, 429 back-off + retry, error/abort/non-array paths, post-unmount guards, and showMore append/no-op/throw/non-array - Add use-media-query.test.tsx: match state, change toggle, unmount cleanup, and the server-snapshot fallback via SSR render - Fix use-card-browser: clear `error` on a successful page-one fetch so a 429 that retries to success drops the stale "Too many searches" warning (mirrors the header-search hook) - Exclude type-only browser-state.ts from coverage
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/_components/header-search/header-search-bar-deferred.tsx (1)
31-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse explicit pixel height utility for the Suspense fallback shell.
RestingInputis used as the<Suspense>fallback, but its reserved height usesh-9. Please switch the fallback shell to an explicit pixel class (e.g.h-[36px]) to align with the CLS guardrail.Suggested change
- <div className="flex items-center gap-2 h-9 px-2.5 rounded-md border border-input bg-muted/40 text-sm focus-within:bg-background focus-within:ring-1 focus-within:ring-ring transition-colors"> + <div className="flex items-center gap-2 h-[36px] px-2.5 rounded-md border border-input bg-muted/40 text-sm focus-within:bg-background focus-within:ring-1 focus-within:ring-ring transition-colors">As per coding guidelines, "Suspense fallbacks must reserve layout space with explicit pixel heights (e.g.
h-[20px]). Streaming content into a zero-height fallback causes CLS."Also applies to: 80-80
🤖 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/_components/header-search/header-search-bar-deferred.tsx` at line 31, The Suspense fallback uses <RestingInput onActivate={() => {}}> which currently reserves height via the utility h-9; update the fallback shell to use an explicit pixel height utility (for example h-[36px]) to prevent CLS. Locate the Suspense fallback in header-search-bar-deferred.tsx (the Suspense wrapping RestingInput) and replace or add the explicit pixel height class on the fallback component (RestingInput) so the reserved height is an explicit pixel value rather than a relative/utility height.Source: Coding guidelines
🧹 Nitpick comments (2)
app/globals.css (1)
351-358: 💤 Low valueConsider renaming
md-fade-infor clarity.The keyframe
md-fade-inonly animatestransform(translateY), notopacity, which might be unexpected given the name. While the paint-safe approach is correct and well-documented (lines 267–271), renaming to something likemd-slide-subtlewould better reflect the actual behavior.That said, the current implementation is functionally correct—elements remain visible (opacity: 1) even when animations are frozen.
🤖 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/globals.css` around lines 351 - 358, Rename the keyframes named md-fade-in to a clearer name that reflects the actual transform-only behavior (e.g., md-slide-subtle): update the `@keyframes` declaration (md-fade-in → md-slide-subtle) and update all references/usages of md-fade-in in the stylesheet (animation, animation-name, or any utility classes) to the new name so the animation name matches its translateY-only effect and avoids implying opacity changes.app/api/cards/browse/route.test.ts (1)
69-97: ⚡ Quick winAdd pagination normalization tests for invalid/large
limitandoffset.Once clamping is in place, add cases like
limit=abc,offset=-1, and very large offset to prevent regressions in request normalization.🤖 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/api/cards/browse/route.test.ts` around lines 69 - 97, Add tests to cover pagination normalization in the browse route: extend route.test.ts with cases that call GET(req("/api/cards/browse?...")) using invalid/edge pagination values (e.g. limit=abc, offset=-1, and a very large offset) after calling allow() and mocking searchMock (the mocked searchCardsBySyntax). For each case assert the response status and that searchMock was called with normalized numeric limit and offset values (inspect searchMock.mock.calls[0] to check the limit and offset arguments), and also assert searchMock is not called when input is rejected; reference the existing test helpers GET, req, allow, and the searchMock/searchCardsBySyntax call pattern to locate where to add these new tests.
🤖 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-browser/browser-card.tsx`:
- Around line 83-87: The overlay div currently unconditionally calls
onClick={(e) => e.stopPropagation()}, which swallows the parent card click
handler (the parent onClick that adds tiles) even when the overlay is visually
hidden; change this so the overlay only blocks propagation when it is actually
interactive—either by making the overlay non-interactive when hidden (use
pointer-events-none / group-hover:pointer-events-auto) or by conditionally
calling e.stopPropagation() (only when the overlay is visible or when selection
mode is active). Update the overlay element (the div with class "absolute
inset-0 ... group-hover:opacity-100" and the current onClick) to use one of
these approaches so the parent onClick can fire in non-select mode.
In `@app/_components/builder/card-browser/deck-browser-context.tsx`:
- Around line 133-144: The code optimistically dispatches local state updates
before awaiting server calls in startTransition: move the dispatch calls for
both the decrement branch and the remove branch to after their respective awaits
(updateCardQuantity and removeCardFromDeck) or implement an explicit rollback on
failure; specifically, in the block using startTransition where dc, deckId,
dispatch, updateCardQuantity and removeCardFromDeck are referenced, perform
await updateCardQuantity(deckId, dc.id, ...) then dispatch the "update" action,
and similarly await removeCardFromDeck(deckId, dc.id) then dispatch the "remove"
action (or catch errors and revert dispatch) so local state only changes after
server confirmation.
In `@app/_components/builder/card-browser/target-picker.tsx`:
- Line 20: Update the top-line JSDoc comment in target-picker.tsx to fix the
typo: change "adds land in" to "adds cards in" (so the comment correctly reads
that the browser chooses the mainboard category that cards are added in, with
`null` = uncategorized). Locate the comment above the TargetPicker component or
export and replace the incorrect phrase while preserving the rest of the comment
punctuation and formatting.
In `@app/_components/builder/card-browser/use-card-browser.ts`:
- Around line 74-96: The 429 retry path leaves the error message set, so even
after a later successful fetch the UI still shows an error; update the success
path in use-card-browser (where you process a non-429 successful response and
call setResults, setOffset, setHasMore, setLoading) to also clear any previous
error by calling setError("") (or null) after parsing results so the alert state
is removed on successful retries.
- Around line 112-131: The showMore function can append results from a prior
query after the query changes; fix it by adding a request guard: generate a
unique request key or use an AbortController (store in a ref like
currentRequestRef) at the start of showMore, capture the current query and/or
assign the request key to currentRequestRef, attach the controller to the fetch,
and before calling setResults/setOffset/setHasMore verify the request key still
matches currentRequestRef (or that the fetch wasn’t aborted) and that query
equals the captured query; if it doesn’t match, discard the response instead of
mutating state. Ensure you clean up/replace the ref when new browse actions or
query changes occur.
In `@app/api/cards/browse/route.ts`:
- Around line 53-60: The parsing uses bitwise OR which can wrap ints and allows
huge offsets; replace the `| 0` usage by robust numeric parsing and clamping:
parse limitRaw with parseInt(limitRaw ?? String(DEFAULT_LIMIT), 10) or Number(),
ensure it's finite, use Math.max(1, Math.min(MAX_LIMIT,
Math.trunc(parsedLimit))) to produce pageLimit; parse offsetRaw with
parseInt(offsetRaw ?? "0", 10), ensure finite, use Math.max(0,
Math.trunc(parsedOffset)) and then cap it with a safe MAX_OFFSET constant (e.g.,
introduce MAX_OFFSET) via Math.min(MAX_OFFSET, offset) to avoid expensive deep
scans; update references to limitRaw, pageLimit, offsetRaw, offset, MAX_LIMIT
and DEFAULT_LIMIT accordingly.
---
Outside diff comments:
In `@app/_components/header-search/header-search-bar-deferred.tsx`:
- Line 31: The Suspense fallback uses <RestingInput onActivate={() => {}}> which
currently reserves height via the utility h-9; update the fallback shell to use
an explicit pixel height utility (for example h-[36px]) to prevent CLS. Locate
the Suspense fallback in header-search-bar-deferred.tsx (the Suspense wrapping
RestingInput) and replace or add the explicit pixel height class on the fallback
component (RestingInput) so the reserved height is an explicit pixel value
rather than a relative/utility height.
---
Nitpick comments:
In `@app/api/cards/browse/route.test.ts`:
- Around line 69-97: Add tests to cover pagination normalization in the browse
route: extend route.test.ts with cases that call
GET(req("/api/cards/browse?...")) using invalid/edge pagination values (e.g.
limit=abc, offset=-1, and a very large offset) after calling allow() and mocking
searchMock (the mocked searchCardsBySyntax). For each case assert the response
status and that searchMock was called with normalized numeric limit and offset
values (inspect searchMock.mock.calls[0] to check the limit and offset
arguments), and also assert searchMock is not called when input is rejected;
reference the existing test helpers GET, req, allow, and the
searchMock/searchCardsBySyntax call pattern to locate where to add these new
tests.
In `@app/globals.css`:
- Around line 351-358: Rename the keyframes named md-fade-in to a clearer name
that reflects the actual transform-only behavior (e.g., md-slide-subtle): update
the `@keyframes` declaration (md-fade-in → md-slide-subtle) and update all
references/usages of md-fade-in in the stylesheet (animation, animation-name, or
any utility classes) to the new name so the animation name matches its
translateY-only effect and avoids implying opacity changes.
🪄 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: 0da2e392-2fb3-4e3e-84d4-bd30a952d7d9
📒 Files selected for processing (36)
app/_components/builder/card-browser/add-controls.tsxapp/_components/builder/card-browser/browser-card.tsxapp/_components/builder/card-browser/browser-state.tsapp/_components/builder/card-browser/bulk-bar.tsxapp/_components/builder/card-browser/card-browser.tsxapp/_components/builder/card-browser/color-pip.tsxapp/_components/builder/card-browser/condensed-row.tsxapp/_components/builder/card-browser/deck-browser-context.tsxapp/_components/builder/card-browser/density-toggle.tsxapp/_components/builder/card-browser/filter-builder.test.tsxapp/_components/builder/card-browser/filter-builder.tsxapp/_components/builder/card-browser/in-deck-badge.tsxapp/_components/builder/card-browser/mode-tabs.tsxapp/_components/builder/card-browser/scry-tray.tsxapp/_components/builder/card-browser/select-check.tsxapp/_components/builder/card-browser/side-panel.tsxapp/_components/builder/card-browser/syntax-input.tsxapp/_components/builder/card-browser/target-picker.tsxapp/_components/builder/card-browser/tray-card.tsxapp/_components/builder/card-browser/use-card-browser.tsapp/_components/builder/card-browser/use-media-query.tsapp/_components/builder/deck-builder.tsxapp/_components/builder/deck-search-context.tsxapp/_components/builder/decklist-toolbar.tsxapp/_components/deck/deck-action-row.tsxapp/_components/decks/deck-card-preview.tsxapp/_components/header-search/deck-mode-bar.tsxapp/_components/header-search/header-search-bar-deferred.tsxapp/_components/header-search/header-search-bar.tsxapp/_components/header-search/header-search-context.tsxapp/api/cards/browse/route.test.tsapp/api/cards/browse/route.tsapp/globals.csslib/search/__tests__/serialize-where.test.tslib/search/card-search.tslib/search/syntax-parser.ts
Color search queried the printed colors column; for a Commander-first deckbuilder `c:` means color identity. Plus pagination/leak hardening, a batched bulk-add, and SQL-shape test coverage that made the bug visible. Key changes: - card-search: `c:` filters `color_identity` not `c.colors`; short-circuit to [] when zero conditions so a lone `-foo` can't leak a whole-table top-N - editor-actions: add batched `addCardsToDeck` (one tx + one revalidation via applyChanges); rewrite bulk `addSelected` off the per-card await loop - use-card-browser: monotonic reqId guard drops stale `showMore` appends when the query changes mid-flight - browse route: trim before MAX_Q_LENGTH check; attach rateHeaders to 400 - card-search tests: pin generated SQL (color_identity/cmc/tsv), replace the empty-branch test to assert the [] short-circuit - syntax-parser: correct `colors` JSDoc to "color identity" Follow-up: no GIN index on `color_identity` — `@>` containment seq-scans; add `@@index([colorIdentity], type: Gin)` in a separate migration.
The `c:` color filter (searchCardsBySyntax) issues `color_identity @> ARRAY[...]::text[]`, which seq-scans without an index. Add a GIN index over the text[] column — default array_ops supports @>. Mirrors the card_search_tsv_idx pattern, including the CONCURRENTLY prod-rollout note for a zero-downtime apply.
Resolve still-valid PR review findings on the card browser surfaces. Key changes: - browser-card: make hover overlay non-interactive off-hover so the parent click adds a card in non-select mode (pointer-events-none + group-hover:pointer-events-auto) - browse route: replace `| 0` parsing with parseInt + finite/trunc clamping and cap offset via MAX_OFFSET to bound deep scans - browse route test: cover limit/offset normalization edge cases - header search: reserve fallback height with explicit px (h-[36px]) - globals.css: rename md-fade-in -> md-slide-subtle (transform-only) - target-picker: fix "adds land in" -> "adds cards in" doc typo
Coverage gate failed on three uncovered paths left after the card-browser work. All are test-only additions; no source change. - editor-actions: cover addCardsToDeck (map, per-card vs opts zone/category precedence, non-MAINBOARD category guard, InvariantViolation swallow, error propagation, non-owner 404) - use-card-browser: cover the post-unmount json/reject bail-outs and the stale showMore rejection after a mid-flight query change - serialize-where: cover the no-WUBRG color branch (emits no c: token)
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.
feat(builder): add card browser with filters, syntax search, and responsive surfaces
Description
Adds an in-builder card browser for searching the canonical card pool and adding
results straight into the open deck. Search is driven by a single query string that
both a structured Filters tab and a raw Syntax tab read and write, so the two
round-trip through one source of truth. Results render in a docked side panel on
desktop (≥1024px) and a bottom "Scry Tray" on smaller viewports, sharing one provider
mount so deck state and selection survive a breakpoint change.
Fixes: #22
What changed
Search backend
lib/search/syntax-parser.ts— Scryfall-syntax subset parser (c:,t:,cmc<op>N,o:, quoted/bare name fragments) plusserializeWhereinverse for round-tripping the Filters tab ↔ syntax box.app/api/cards/browse/route.ts—GET /api/cards/browseendpoint: rate-limited (90/60s per IP),qcapped at 64 chars, paginated (limit/offset, max 120). Empty query returns[]so the grid never streams the whole card table.lib/search/card-search.ts— wired syntax-parsedwhereinto card search.Browser UI (
app/_components/builder/card-browser/)card-browser.tsx— parent owning sharedrawquery / mode / density state; picksSidePanelvsScryTrayby viewport.side-panel.tsx,scry-tray.tsx— the two responsive surfaces.filter-builder.tsx— structured filter controls;syntax-input.tsx— raw query box;mode-tabs.tsx— Filters/Syntax toggle.deck-browser-context.tsx— provider holding deck cards, dispatch, categories, format, commander identity.use-card-browser.ts— fetch + pagination (showMore,hasMore,loading/loadingMore) hook;use-media-query.ts— breakpoint hook.browser-card.tsx,condensed-row.tsx,tray-card.tsx,density-toggle.tsx,add-controls.tsx,bulk-bar.tsx,select-check.tsx,target-picker.tsx,in-deck-badge.tsx,color-pip.tsx.Builder integration
deck-builder.tsx,decklist-toolbar.tsx,deck-search-context.tsx, header-search components — open/close wiring and deck-mode bar.app/globals.css— browser/tray styles.Checklist
pnpm test) and lint is cleanrevalidate/dynamic/unstable_cache— used'use cache'+cacheLife/cacheTag<Link>imported fromapp/_components/link.tsx, notnext/linkScreenshots / notes
Tests added:
filter-builder.test.tsx,api/cards/browse/route.test.ts,lib/search/__tests__/serialize-where.test.ts. No<Link>usage in the new code,so that checkbox is left unchecked rather than asserted.
Summary by CodeRabbit