fix: address review findings across ingest, io, and concurrency#61
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds bearer auth to ingest endpoints and bounds SSE; enforces deck-import limits and stricter quantity validation; adds a JSON deck adapter; introduces concurrency helper and fuzzy-match confidence gating; normalizes Scryfall price parsing, updates token upserts to bulk SQL; various tests and Next.js security headers updated. ChangesDeck Import & API Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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
app/(ui)/deck/[id]/play/__tests__/playtest-reducer.test.tsParsing error: The keyword 'import' is reserved app/(ui)/deck/[id]/play/playtest-reducer.tsParsing error: The keyword 'import' is reserved app/(ui)/decks/explore/__tests__/actions.test.tsParsing error: The keyword 'import' is reserved
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workflows/scryfall/steps.ts (1)
447-473:⚠️ Potential issue | 🟠 MajorDeduplicate
(card_id, token_scryfall_id)conflict keys before the bulk token upsert.
upsertTokens()buildstokenRowsfrom the originalcards(not the deduped card set) and can therefore generate duplicate(cardId, part.id)pairs within the same 500-rowINSERT ... ON CONFLICT (card_id, token_scryfall_id) DO UPDATE. Postgres rejects a singleINSERTthat tries to update the same conflict target multiple times (“cannot affect row a second time”). Build the upsert rows from a keyedMap/Setbefore chunking.Proposed fix
async function upsertTokens( cards: ScryfallCard[], idByName: Map<string, number>, ): Promise<void> { - const tokenRows: { cardId: number; tokenName: string; tokenScryfallId: string }[] = []; + const tokenByKey = new Map< + string, + { cardId: number; tokenName: string; tokenScryfallId: string } + >(); for (const c of cards) { if (!c.all_parts) continue; const cardId = idByName.get(c.name); if (cardId === undefined) continue; for (const part of c.all_parts) { if (part.component === "token") { - tokenRows.push({ cardId, tokenName: part.name, tokenScryfallId: part.id }); + const key = `${cardId}:${part.id}`; + tokenByKey.set(key, { + cardId, + tokenName: part.name, + tokenScryfallId: part.id, + }); } } } + const tokenRows = Array.from(tokenByKey.values()); if (tokenRows.length === 0) return;🤖 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 `@workflows/scryfall/steps.ts` around lines 447 - 473, The bulk upsert can include duplicate (card_id, token_scryfall_id) pairs which causes Postgres to error; before chunking (i.e., before using UPSERT_CHUNK, rows, Prisma.join and the INSERT INTO card_token block) deduplicate tokenRows by keying on cardId + tokenScryfallId (use a Map or Set) so only one row per (cardId, tokenScryfallId) is emitted (if multiple tokenName values exist pick one deterministically or keep the last seen), then build chunks from that deduped array and run the existing INSERT ... ON CONFLICT logic (this change should be applied in upsertTokens()/where tokenRows is assembled).
🧹 Nitpick comments (4)
next.config.ts (1)
54-84: ⚡ Quick winWire a CSP reporting endpoint so Report-Only is actually observable.
Content-Security-Policy-Report-Onlyis set, but noreport-uri/report-todirective is present, so rollout telemetry is incomplete outside manual browser inspection.Suggested patch
async headers() { @@ const csp = [ "script-src 'self' https://cdn.vercel-insights.com https://vitals.vercel-insights.com", "frame-ancestors 'none'", + "report-uri /api/security/csp-report", ].join("; ");🤖 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 `@next.config.ts` around lines 54 - 84, The CSP is sent as Report-Only in the headers() function but lacks a reporting directive, so add a report endpoint directive to the csp string (e.g., append a report-uri or report-to directive like "report-to /csp-report" or "report-uri /csp-report") and update securityHeaders accordingly; then implement a corresponding route/handler to accept JSON/reporting payloads at that endpoint so violations are actually collected (use the same endpoint name referenced in the csp string). Ensure changes are made where csp and securityHeaders are defined inside headers() and that the reporting endpoint path matches the handler you add.lib/deck/io/adapters/__tests__/json.test.ts (1)
106-126: ⚡ Quick winAdd a regression test for over-limit
quantityvalues.The error-handling suite should include a payload with
quantityabove the allowed max so this validation contract stays protected.Suggested test addition
+import { MAX_CARD_QTY } from "../../consts"; @@ describe("jsonAdapter.parse — error handling", () => { + it("returns 0 cards and a warning when quantity exceeds MAX_CARD_QTY", () => { + const result = jsonAdapter.parse( + JSON.stringify({ + name: "Test Deck", + format: "COMMANDER", + visibility: "PRIVATE", + description: null, + cards: [ + { + name: "Sol Ring", + quantity: MAX_CARD_QTY + 1, + zone: "MAINBOARD", + category: null, + isFoil: false, + }, + ], + categories: [], + }), + ); + expect(result.cards).toHaveLength(0); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toContain("validation"); + });🤖 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 `@lib/deck/io/adapters/__tests__/json.test.ts` around lines 106 - 126, Add a regression test to the "jsonAdapter.parse — error handling" suite that passes a JSON payload containing a card with a quantity field exceeding the allowed maximum to ensure validation rejects it without throwing; call jsonAdapter.parse with JSON.stringify({ cards: [{ id: "x", quantity: 999999 }] }) (use whatever id/schema fields the adapter expects), then assert result.cards has length 0 and result.warnings contains a message mentioning "quantity" or "validation" (and/or an error count), mirroring the other tests' style so the validation contract for max quantity is covered.lib/scryfall/__tests__/map.test.ts (1)
264-278: 💤 Low valueTest relies on
logWarnimplementation detail.These tests spy on
console.errorto verifylogWarnbehavior. If the logging implementation changes (e.g., to a structured logger that doesn't use console.error), these tests will silently pass without verifying the warning was emitted. Consider whether mockinglogWarndirectly would be more robust.🤖 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 `@lib/scryfall/__tests__/map.test.ts` around lines 264 - 278, The tests spy on console.error which couples them to the current logging implementation; instead spy or mock the logWarn function directly to make the test resilient to logger changes: import the module that exports logWarn (the same logger used by toPrintingCreate/parsePrice), use vi.spyOn(loggerModule, "logWarn") before calling toPrintingCreate(...) and assert the spy was called with expect.stringContaining("parsePrice"), then restore the spy; update both specs that currently spy on console.error to use logWarn so the assertion validates the actual warning emission rather than console internals.lib/deck/io/__tests__/card-resolver.test.ts (1)
14-14: 💤 Low valueConsider documenting the magic constant's source.
FUZZY_CONCURRENCYduplicates the value fromcard-resolver.ts. If the source value changes, this test assertion could silently become incorrect. A brief comment noting the source would help maintainers keep them in sync.🤖 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 `@lib/deck/io/__tests__/card-resolver.test.ts` at line 14, The test defines a duplicated magic constant FUZZY_CONCURRENCY that mirrors the value in card-resolver.ts; either import the constant from card-resolver.ts into lib/deck/io/__tests__/card-resolver.test.ts so the test always uses the source-of-truth, or if import is undesirable, add a brief inline comment above FUZZY_CONCURRENCY documenting that its value must match the FUZZY_CONCURRENCY constant in card-resolver.ts (include the file and constant name) so future changes stay synchronized.
🤖 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/api/ingest/`[runId]/progress/route.ts:
- Around line 43-47: The timeout abort path fails because
source.pipeTo(writable) locks the ReadableStream and source.cancel() then
rejects; instead pass the abort signal into pipeTo so the piping is aborted
reliably: replace the current void source.pipeTo(writable).catch(...) call with
void source.pipeTo(writable, { signal }).catch(...), and remove (or stop calling
source.cancel() from) the signal.addEventListener("abort", ...) handler so you
don’t trigger a redundant cancel that causes TypeError; keep the existing
.catch(() => undefined) to swallow expected abort errors.
In `@app/api/ingest/status/route.ts`:
- Around line 33-36: The runbook's curl examples are missing the required
Authorization header for the protected GET handler in
app/api/ingest/status/route.ts; update all curl/snippet entries that call the
GET endpoint (referencing the GET function and bearerMatches check against
getEnv().CRON_SECRET) to include the header "Authorization: Bearer
<CRON_SECRET>" so requests supply the CRON_SECRET token and no longer receive
401 responses.
In `@lib/concurrency.ts`:
- Around line 5-25: The runWithConcurrency function must validate the
concurrency parameter before creating workers: ensure concurrency is a finite
positive integer (e.g., coerce with Math.floor and check Number.isFinite) and
throw a clear RangeError or TypeError if concurrency <= 0 or not a valid number;
then use the validated value in the Math.min(concurrency, items.length)
expression that builds the workers array. Update the validation near the start
of runWithConcurrency (before computing cursor/worker/workers) and reference the
existing symbols: runWithConcurrency, concurrency, workers, cursor, worker so
the function never constructs zero/negative workers and returns undefined
results.
In `@lib/deck/io/adapters/json.ts`:
- Line 8: The JSON import schema's quantity field currently uses
z.number().int().positive() and must enforce the same upper bound as the
import-validation layer; update the quantity schema (the symbol "quantity" using
z.number().int().positive()) to include .max(...) with the shared limit constant
used by the import-validation code (e.g., MAX_CARD_QUANTITY or the project
constant that defines the card-quantity cap) by importing that constant and
applying it to the zod chain so JSON imports cannot exceed the allowed maximum.
---
Outside diff comments:
In `@workflows/scryfall/steps.ts`:
- Around line 447-473: The bulk upsert can include duplicate (card_id,
token_scryfall_id) pairs which causes Postgres to error; before chunking (i.e.,
before using UPSERT_CHUNK, rows, Prisma.join and the INSERT INTO card_token
block) deduplicate tokenRows by keying on cardId + tokenScryfallId (use a Map or
Set) so only one row per (cardId, tokenScryfallId) is emitted (if multiple
tokenName values exist pick one deterministically or keep the last seen), then
build chunks from that deduped array and run the existing INSERT ... ON CONFLICT
logic (this change should be applied in upsertTokens()/where tokenRows is
assembled).
---
Nitpick comments:
In `@lib/deck/io/__tests__/card-resolver.test.ts`:
- Line 14: The test defines a duplicated magic constant FUZZY_CONCURRENCY that
mirrors the value in card-resolver.ts; either import the constant from
card-resolver.ts into lib/deck/io/__tests__/card-resolver.test.ts so the test
always uses the source-of-truth, or if import is undesirable, add a brief inline
comment above FUZZY_CONCURRENCY documenting that its value must match the
FUZZY_CONCURRENCY constant in card-resolver.ts (include the file and constant
name) so future changes stay synchronized.
In `@lib/deck/io/adapters/__tests__/json.test.ts`:
- Around line 106-126: Add a regression test to the "jsonAdapter.parse — error
handling" suite that passes a JSON payload containing a card with a quantity
field exceeding the allowed maximum to ensure validation rejects it without
throwing; call jsonAdapter.parse with JSON.stringify({ cards: [{ id: "x",
quantity: 999999 }] }) (use whatever id/schema fields the adapter expects), then
assert result.cards has length 0 and result.warnings contains a message
mentioning "quantity" or "validation" (and/or an error count), mirroring the
other tests' style so the validation contract for max quantity is covered.
In `@lib/scryfall/__tests__/map.test.ts`:
- Around line 264-278: The tests spy on console.error which couples them to the
current logging implementation; instead spy or mock the logWarn function
directly to make the test resilient to logger changes: import the module that
exports logWarn (the same logger used by toPrintingCreate/parsePrice), use
vi.spyOn(loggerModule, "logWarn") before calling toPrintingCreate(...) and
assert the spy was called with expect.stringContaining("parsePrice"), then
restore the spy; update both specs that currently spy on console.error to use
logWarn so the assertion validates the actual warning emission rather than
console internals.
In `@next.config.ts`:
- Around line 54-84: The CSP is sent as Report-Only in the headers() function
but lacks a reporting directive, so add a report endpoint directive to the csp
string (e.g., append a report-uri or report-to directive like "report-to
/csp-report" or "report-uri /csp-report") and update securityHeaders
accordingly; then implement a corresponding route/handler to accept
JSON/reporting payloads at that endpoint so violations are actually collected
(use the same endpoint name referenced in the csp string). Ensure changes are
made where csp and securityHeaders are defined inside headers() and that the
reporting endpoint path matches the handler you add.
🪄 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: 30cee0c8-44a7-4304-a6e4-438130616613
📒 Files selected for processing (38)
app/(ui)/decks/explore/__tests__/actions.test.tsapp/(ui)/decks/explore/actions.tsapp/api/ingest/[runId]/progress/route.tsapp/api/ingest/__tests__/progress.test.tsapp/api/ingest/__tests__/status.test.tsapp/api/ingest/_auth.tsapp/api/ingest/_handler.tsapp/api/ingest/status/route.tslib/concurrency.tslib/deck/__tests__/partner-keywords.test.tslib/deck/io/__tests__/card-resolver.test.tslib/deck/io/__tests__/intake.test.tslib/deck/io/__tests__/line-parser.test.tslib/deck/io/__tests__/resolve.test.tslib/deck/io/adapters/__tests__/json.test.tslib/deck/io/adapters/dek.tslib/deck/io/adapters/index.tslib/deck/io/adapters/json.tslib/deck/io/adapters/types.tslib/deck/io/card-resolver.tslib/deck/io/consts.tslib/deck/io/intake.tslib/deck/io/line-parser.tslib/deck/io/resolve.tslib/deck/io/serialize.tslib/deck/mutation/__tests__/invariants.test.tslib/deck/mutation/plan.tslib/deck/partner-keywords.tslib/deck/queries.tslib/scryfall/__tests__/map.test.tslib/scryfall/map.tslib/stats/__tests__/compute.test.tslib/stats/compute.tsnext.config.tsworkflows/_shared/ingest-lock.tsworkflows/precon/steps.tsworkflows/scryfall/__tests__/steps.test.tsworkflows/scryfall/steps.ts
💤 Files with no reviewable changes (1)
- lib/deck/mutation/tests/invariants.test.ts
Verified each review finding against current code and applied the still-valid fixes; skipped the CSP report-endpoint suggestion as a separate feature. Key changes: - progress route: abort the SSE pipe via pipeTo's signal instead of source.cancel(), which rejected with TypeError while pipeTo held the source lock - concurrency: validate concurrency is a positive integer, throwing RangeError instead of silently building zero workers - json adapter: cap import quantity at MAX_CARD_QTY, matching the text and dek import paths - scryfall steps: dedupe token rows on (cardId, tokenScryfallId) before the chunked UPSERT to avoid ON CONFLICT affecting a row twice - runbook: add the required Authorization: Bearer header to the status and progress curl examples - tests: add a max-quantity regression for the json adapter, spy on logWarn instead of console.error in map tests, document the synced FUZZY_CONCURRENCY constant
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 `@docs/runbook/ingest.md`:
- Around line 111-116: Add a note in the "progress" section that the SSE
endpoint GET /api/ingest/<runId>/progress is intentionally bounded by a 5‑minute
client timeout (AbortSignal.timeout(5 * 60_000) used in
app/api/ingest/[runId]/progress/route.ts), so operators know a disconnect after
~5 minutes is a normal timeout and not an ingest failure; reference that the
stream emits namespaced "progress" chunks from upsertBatch and suggest
reconnecting to continue monitoring if needed.
🪄 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: ca26f7e8-0947-4700-bdbf-b0e52bde3a43
📒 Files selected for processing (8)
app/api/ingest/[runId]/progress/route.tsdocs/runbook/ingest.mdlib/concurrency.tslib/deck/io/__tests__/card-resolver.test.tslib/deck/io/adapters/__tests__/json.test.tslib/deck/io/adapters/json.tslib/scryfall/__tests__/map.test.tsworkflows/scryfall/steps.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/scryfall/tests/map.test.ts
- workflows/scryfall/steps.ts
- lib/deck/io/tests/card-resolver.test.ts
- app/api/ingest/[runId]/progress/route.ts
- lib/deck/io/adapters/tests/json.test.ts
- lib/deck/io/adapters/json.ts
| `GET /api/ingest/<runId>/progress` streams namespaced (`progress`) chunks emitted by `upsertBatch` while a Scryfall ingest is running. Useful for watching a long ingest from a terminal: | ||
|
|
||
| ```bash | ||
| curl -N https://<deployment>/api/ingest/<runId>/progress | ||
| curl -N -H "Authorization: Bearer $CRON_SECRET" \ | ||
| https://<deployment>/api/ingest/<runId>/progress | ||
| ``` |
There was a problem hiding this comment.
Document the 5-minute SSE timeout in the progress section.
GET /api/ingest/<runId>/progress is intentionally bounded by AbortSignal.timeout(5 * 60_000) in app/api/ingest/[runId]/progress/route.ts. Please call this out here so operators don’t treat a normal timeout disconnect as an ingest failure.
Suggested doc patch
`GET /api/ingest/<runId>/progress` streams namespaced (`progress`) chunks emitted by `upsertBatch` while a Scryfall ingest is running. Useful for watching a long ingest from a terminal:
+The stream is intentionally time-bounded and will close after ~5 minutes; reconnect if you need to continue tailing.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `GET /api/ingest/<runId>/progress` streams namespaced (`progress`) chunks emitted by `upsertBatch` while a Scryfall ingest is running. Useful for watching a long ingest from a terminal: | |
| ```bash | |
| curl -N https://<deployment>/api/ingest/<runId>/progress | |
| curl -N -H "Authorization: Bearer $CRON_SECRET" \ | |
| https://<deployment>/api/ingest/<runId>/progress | |
| ``` | |
| `GET /api/ingest/<runId>/progress` streams namespaced (`progress`) chunks emitted by `upsertBatch` while a Scryfall ingest is running. Useful for watching a long ingest from a terminal: | |
| The stream is intentionally time-bounded and will close after ~5 minutes; reconnect if you need to continue tailing. | |
🤖 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 `@docs/runbook/ingest.md` around lines 111 - 116, Add a note in the "progress"
section that the SSE endpoint GET /api/ingest/<runId>/progress is intentionally
bounded by a 5‑minute client timeout (AbortSignal.timeout(5 * 60_000) used in
app/api/ingest/[runId]/progress/route.ts), so operators know a disconnect after
~5 minutes is a normal timeout and not an ingest failure; reference that the
stream emits namespaced "progress" chunks from upsertBatch and suggest
reconnecting to continue monitoring if needed.
Close the remaining coverage gaps that were tripping the global thresholds (lines <100%, branches <99%) and push every metric to 100%. Added tests: - concurrency: full runWithConcurrency suite incl. RangeError guard - explore actions: all-optional-filters-defined path - inventory action: printing vanishing between foil check and cardId lookup - legality: dedicated singletonRule block (zone filter, basics, violation) - manabase: unknown basic-land row name; null oracle text predicate path - mutation apply: category-only move writes data.category - inventory state: unpinned holding for a different card - playtest reducer: multi-commander cast/decrement, multi-card tap, move-onto-battlefield tapped preservation - header-search: non-array payload, cancel-after-fetch, cancel-after-json, reject-after-unmount Removed unreachable branches (no behavior change): - playtest reducer: dropped dead surveil guard + unreachable return - json adapter: issues[0]!.message (Zod failure always has an issue) - inventory state: dropped redundant WISHLIST check (HoldingState is binary)
Description
Hardening pass addressing review findings across the Scryfall ingest path, deck import/export (io), and the shared concurrency helper. No new features — each change fixes a latent correctness or robustness bug surfaced in review.
app/api/ingest/[runId]/progress/route.ts): delegate cancellation topipeTo's ownAbortSignalinstead of callingsource.cancel()manually. Callingcancel()conflicted with the lockpipeToholds on the source and rejected with aTypeError.workflows/scryfall/steps.ts): dedupe(cardId, tokenScryfallId)pairs before theON CONFLICTinsert. Duplicate conflict keys in one statement made Postgres reject with"ON CONFLICT DO UPDATE command cannot affect row a second time".lib/concurrency.ts): floor + validateconcurrencyto a positive integer, throwingRangeErrorotherwise, so a bad cap can't silently spawn zero workers or fractional bounds.lib/deck/io/adapters/json.ts): boundquantitybyMAX_CARD_QTYin the Zod schema; oversized cards are now dropped with a warning instead of being accepted.docs/runbook/ingest.md): doc now reflects that/api/ingest/statusand/progressare guarded byCRON_SECRETBearer.map.test.ts,card-resolver.test.ts,json.test.ts): spy ontelemetry.logWarninstead ofconsole.error, document the mirroredFUZZY_CONCURRENCYconstant, and cover the new quantity-cap rejection.Fixes: #
Checklist
pnpm test) and lint is cleanrevalidate/dynamic/unstable_cache— used'use cache'+cacheLife/cacheTag<Link>imported fromapp/_components/link.tsx, notnext/linkScreenshots / notes
Backend/infra-only changes — no UI surface. The last three checklist items (cache directives,
<Link>, Suspense) are N/A; nothing in this PR touches components, routes-with-rendering, or cache config.graph TD A[progress route] -->|pipeTo signal teardown| B[workflow readable] C[scryfall steps] -->|dedupe ON CONFLICT keys| D[(Postgres tokens)] E[json adapter] -->|MAX_CARD_QTY cap| F[deck import] G[concurrency helper] -->|RangeError guard| H[card-resolver fuzzy]Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests