Skip to content

fix: address review findings across ingest, io, and concurrency#61

Merged
jcserv merged 16 commits into
mainfrom
jservilla/refactors
Jun 7, 2026
Merged

fix: address review findings across ingest, io, and concurrency#61
jcserv merged 16 commits into
mainfrom
jservilla/refactors

Conversation

@jcserv

@jcserv jcserv commented Jun 7, 2026

Copy link
Copy Markdown
Owner

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.

  • Progress stream teardown (app/api/ingest/[runId]/progress/route.ts): delegate cancellation to pipeTo's own AbortSignal instead of calling source.cancel() manually. Calling cancel() conflicted with the lock pipeTo holds on the source and rejected with a TypeError.
  • Token upsert dedupe (workflows/scryfall/steps.ts): dedupe (cardId, tokenScryfallId) pairs before the ON CONFLICT insert. Duplicate conflict keys in one statement made Postgres reject with "ON CONFLICT DO UPDATE command cannot affect row a second time".
  • Concurrency guard (lib/concurrency.ts): floor + validate concurrency to a positive integer, throwing RangeError otherwise, so a bad cap can't silently spawn zero workers or fractional bounds.
  • JSON adapter quantity cap (lib/deck/io/adapters/json.ts): bound quantity by MAX_CARD_QTY in the Zod schema; oversized cards are now dropped with a warning instead of being accepted.
  • Status route auth (docs/runbook/ingest.md): doc now reflects that /api/ingest/status and /progress are guarded by CRON_SECRET Bearer.
  • Test fixes (map.test.ts, card-resolver.test.ts, json.test.ts): spy on telemetry.logWarn instead of console.error, document the mirrored FUZZY_CONCURRENCY constant, and cover the new quantity-cap rejection.

Fixes: #

Checklist

  • Tests pass (pnpm test) and lint is clean
  • New behavior is covered by tests
  • No revalidate / dynamic / unstable_cache — used 'use cache' + cacheLife/cacheTag
  • <Link> imported from app/_components/link.tsx, not next/link
  • Suspense fallbacks reserve layout space (explicit heights)

Screenshots / 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]
Loading

Summary by CodeRabbit

  • New Features

    • JSON deck import support
    • Ingestion APIs now require bearer authentication
    • CSP report-only security headers added site-wide
  • Bug Fixes

    • Enforced limits on card quantities and import line counts; import truncation warns users
    • Normalized price parsing and improved handling of invalid values
  • Improvements

    • Fuzzy card-name substitutions gated by confidence with user-facing warnings
    • Concurrency-limited card resolution and bounded progress streaming
    • Mana-cost calculations extended for additional symbol types
  • Tests

    • Expanded coverage for imports, resolution, fuzzy matching, concurrency, and ingest auth

@jcserv jcserv self-assigned this Jun 7, 2026
@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Deck Import & API Enhancements

Layer / File(s) Summary
Ingest API Authorization Gate
app/api/ingest/_auth.ts, app/api/ingest/_handler.ts, app/api/ingest/[runId]/progress/route.ts, app/api/ingest/status/route.ts, app/api/ingest/__tests__/progress.test.ts, app/api/ingest/__tests__/status.test.ts
Adds bearerMatches for constant-time Bearer token checks; applies auth to ingest status/progress routes and handler; progress SSE is bounded (5 min); tests cover auth and not-found cases.
Deck Import Input Bounds & Validation
lib/deck/io/consts.ts, lib/deck/io/line-parser.ts, lib/deck/io/adapters/dek.ts, lib/deck/io/intake.ts, lib/deck/io/__tests__/intake.test.ts, lib/deck/io/__tests__/line-parser.test.ts
Introduces MAX_CARD_LINES and MAX_CARD_QTY; line parser and DEK adapter require integer quantities within bounds; intake truncates oversized imports and emits truncation warnings; tests added.
JSON Deck Format Adapter
lib/deck/io/adapters/types.ts, lib/deck/io/adapters/json.ts, lib/deck/io/adapters/index.ts, lib/deck/io/serialize.ts, lib/deck/io/adapters/__tests__/json.test.ts
Adds Zod schemas and jsonAdapter (detect/parse), registers adapter for detection priority, and reuses shared JSON types in serializer. Tests cover round-trip and validation failures.
Concurrency Utilities & Fuzzy Card Resolution
lib/concurrency.ts, lib/deck/io/card-resolver.ts, lib/deck/io/resolve.ts, lib/deck/io/__tests__/card-resolver.test.ts, lib/deck/io/__tests__/resolve.test.ts, workflows/precon/steps.ts
Adds runWithConcurrency to limit workers; card resolver adds warnings per resolution and a confidence threshold (FUZZY_MIN_CONFIDENCE) so low-confidence fuzzy hits become unmatched; tests validate thresholds and concurrency caps; precon steps import shared helper.
Scryfall Data Pipeline Optimizations
lib/scryfall/map.ts, lib/scryfall/__tests__/map.test.ts, lib/stats/compute.ts, lib/stats/__tests__/compute.test.ts, workflows/_shared/ingest-lock.ts, workflows/precon/steps.ts, workflows/scryfall/steps.ts, workflows/scryfall/__tests__/steps.test.ts
Price parsing normalized to fixed 2-decimal format with warnings for invalid values; mana pip parsing handles Phyrexian and twobrid symbols; ingest lock treats only P2002 as duplicate-key fallback; token upserts moved to chunked $executeRaw bulk INSERT ... ON CONFLICT; tests updated.
Public Deck Exploration Input Validation
app/(ui)/decks/explore/actions.ts, app/(ui)/decks/explore/__tests__/actions.test.ts
loadMorePublicDecks uses Zod argsSchema to clamp page/pageSize, validate filters (enums/optionals), forward only defined filters, and compute pagination from clamped values; tests assert clamping and tolerant handling of invalid format.
Partner Keywords & Internal Visibility Changes
lib/deck/partner-keywords.ts, lib/deck/__tests__/partner-keywords.test.ts, lib/deck/mutation/plan.ts, lib/deck/queries.ts, lib/deck/mutation/__tests__/invariants.test.ts, next.config.ts
canHavePartner excludes Partner with ... from generic partner match; MutationPlan and selectCommanderName made internal (non-exported); removed unused test snapshot setup; Next.js adds report-only CSP and other security headers.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • jcserv/maindeck#43: Related to extending tests and behavior around the useCardSearch hook.
  • jcserv/maindeck#54: Overlaps with partner keyword refinement distinguishing "Partner" vs "Partner with …".

Poem

🐰 I hopped through schemas, streams, and clacks,
I bounded fuzzy hops and trimmed long stacks,
JSON decks now find their tidy home,
Tokens bulk up as prices comb,
A rabbit's patch of checks—now safe to roam.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'fix: address review findings across ingest, io, and concurrency' accurately describes the main focus of the changeset, which encompasses bug fixes and hardening across those three areas.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jservilla/refactors

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

app/(ui)/deck/[id]/play/__tests__/playtest-reducer.test.ts

Parsing error: The keyword 'import' is reserved

app/(ui)/deck/[id]/play/playtest-reducer.ts

Parsing error: The keyword 'import' is reserved

app/(ui)/decks/explore/__tests__/actions.test.ts

Parsing error: The keyword 'import' is reserved

  • 10 others

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Deduplicate (card_id, token_scryfall_id) conflict keys before the bulk token upsert.

upsertTokens() builds tokenRows from the original cards (not the deduped card set) and can therefore generate duplicate (cardId, part.id) pairs within the same 500-row INSERT ... ON CONFLICT (card_id, token_scryfall_id) DO UPDATE. Postgres rejects a single INSERT that tries to update the same conflict target multiple times (“cannot affect row a second time”). Build the upsert rows from a keyed Map/Set before 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 win

Wire a CSP reporting endpoint so Report-Only is actually observable.

Content-Security-Policy-Report-Only is set, but no report-uri/report-to directive 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 win

Add a regression test for over-limit quantity values.

The error-handling suite should include a payload with quantity above 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 value

Test relies on logWarn implementation detail.

These tests spy on console.error to verify logWarn behavior. 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 mocking logWarn directly 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 value

Consider documenting the magic constant's source.

FUZZY_CONCURRENCY duplicates the value from card-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

📥 Commits

Reviewing files that changed from the base of the PR and between b0ac6a3 and 1922b59.

📒 Files selected for processing (38)
  • app/(ui)/decks/explore/__tests__/actions.test.ts
  • app/(ui)/decks/explore/actions.ts
  • app/api/ingest/[runId]/progress/route.ts
  • app/api/ingest/__tests__/progress.test.ts
  • app/api/ingest/__tests__/status.test.ts
  • app/api/ingest/_auth.ts
  • app/api/ingest/_handler.ts
  • app/api/ingest/status/route.ts
  • lib/concurrency.ts
  • lib/deck/__tests__/partner-keywords.test.ts
  • lib/deck/io/__tests__/card-resolver.test.ts
  • lib/deck/io/__tests__/intake.test.ts
  • lib/deck/io/__tests__/line-parser.test.ts
  • lib/deck/io/__tests__/resolve.test.ts
  • lib/deck/io/adapters/__tests__/json.test.ts
  • lib/deck/io/adapters/dek.ts
  • lib/deck/io/adapters/index.ts
  • lib/deck/io/adapters/json.ts
  • lib/deck/io/adapters/types.ts
  • lib/deck/io/card-resolver.ts
  • lib/deck/io/consts.ts
  • lib/deck/io/intake.ts
  • lib/deck/io/line-parser.ts
  • lib/deck/io/resolve.ts
  • lib/deck/io/serialize.ts
  • lib/deck/mutation/__tests__/invariants.test.ts
  • lib/deck/mutation/plan.ts
  • lib/deck/partner-keywords.ts
  • lib/deck/queries.ts
  • lib/scryfall/__tests__/map.test.ts
  • lib/scryfall/map.ts
  • lib/stats/__tests__/compute.test.ts
  • lib/stats/compute.ts
  • next.config.ts
  • workflows/_shared/ingest-lock.ts
  • workflows/precon/steps.ts
  • workflows/scryfall/__tests__/steps.test.ts
  • workflows/scryfall/steps.ts
💤 Files with no reviewable changes (1)
  • lib/deck/mutation/tests/invariants.test.ts

Comment thread app/api/ingest/[runId]/progress/route.ts Outdated
Comment thread app/api/ingest/status/route.ts
Comment thread lib/concurrency.ts
Comment thread lib/deck/io/adapters/json.ts Outdated
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
@jcserv jcserv changed the title Jservilla/refactors fix: address review findings across ingest, io, and concurrency Jun 7, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1922b59 and bc084b4.

📒 Files selected for processing (8)
  • app/api/ingest/[runId]/progress/route.ts
  • docs/runbook/ingest.md
  • lib/concurrency.ts
  • lib/deck/io/__tests__/card-resolver.test.ts
  • lib/deck/io/adapters/__tests__/json.test.ts
  • lib/deck/io/adapters/json.ts
  • lib/scryfall/__tests__/map.test.ts
  • workflows/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

Comment thread docs/runbook/ingest.md
Comment on lines 111 to 116
`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
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
`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)
@jcserv jcserv merged commit d6db512 into main Jun 7, 2026
4 of 5 checks passed
@jcserv jcserv deleted the jservilla/refactors branch June 7, 2026 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant