test(worker-bus): export v2 duckdb/inference handlers + parity gate tests - #286
Conversation
…ests Neither workers/v2/duckdb.worker.ts's handlers nor workers/v2/inference.worker.ts's handleInference were exported or unit-tested against real logic (only webllm.worker.ts had that, tests/unit/webllmWorkerHandler.test.ts) despite both already being registered as live WorkerBus v2 pools with zero production callers — docs/adr/0014-worker-generation-duplication.md's deferred migration needs these to actually match v1 before any consumer cuts over. Testing surfaced a real gap: initDuckDb()'s OPFS-unavailable catch block silently swallowed the failure, unlike v1's out-of-band OPFS_FALLBACK message that lets the UI warn users their analytics won't persist. Fixed by threading ctx.emitProgress into initDuckDb() and emitting an 'opfs-fallback' progress stage from the catch block — reuses the existing progress channel WebLLM already uses for download progress, no new message-protocol/schema change needed. First PR of the ADR-0014 consolidation sprint (5 PRs total, see .claude/plans/worker-generation-v1-vs-enumerated-fox.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds direct unit-test coverage for v2 DuckDB and inference worker handlers. DuckDB initialization now reports OPFS fallback progress, and both workers export handlers for test access. ChangesDuckDB worker handlers
Inference worker handler
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@workers/v2/duckdb.worker.ts`:
- Around line 45-46: Replace the multi-line QNBS-v3 annotations with one
immediate comment in the required “// QNBS-v3: [Reason / impact / creative
value]” format: workers/v2/duckdb.worker.ts lines 45-46 for the export rationale
and lines 76-79 for the fallback-channel rationale;
tests/unit/duckdbWorkerHandler.test.ts lines 2-5 for the suite rationale, 17-18
for hoisting, 31-32 for constructability, 58 for import order, 91-92 for the
shutdown mock, and 99-101 for state reset. Preserve each annotation’s original
rationale while collapsing it to a single bracketed line.
- Around line 75-85: Update workers/v2/duckdb.worker.ts lines 75-85 to retain
the OPFS connection separately, close it in the ATTACH failure catch path, then
create and assign the fallback connection. Update
tests/unit/duckdbWorkerHandler.test.ts lines 150-157 to use distinct OPFS and
fallback connections and assert the OPFS connection’s close() method is called.
In `@workers/v2/inference.worker.ts`:
- Around line 45-46: Replace the multi-line QNBS annotations with adjacent
single-line bracketed comments using the required “QNBS-v3: [Grund / Impact /
Kreativer Mehrwert]” format: update the handler export annotation in
workers/v2/inference.worker.ts at lines 45-46, collapse the file-purpose
annotation in tests/unit/inferenceWorkerHandlerV2.test.ts at lines 2-6, and
reformat the import-order annotation at line 17. Preserve each annotation’s
original intent.
🪄 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
Run ID: 4df0d1fa-34af-4230-be78-c6bafca5f8de
📒 Files selected for processing (4)
tests/unit/duckdbWorkerHandler.test.tstests/unit/inferenceWorkerHandlerV2.test.tsworkers/v2/duckdb.worker.tsworkers/v2/inference.worker.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…S-v3 format CodeRabbit review on #286 found a real bug: initDuckDb()'s OPFS catch block overwrote `connection` with the fallback connection without closing the first (partially-attached) one, leaking a DuckDB connection on every OPFS-attach failure. Fixed by tracking the OPFS attempt in its own local variable and closing it in the catch path before falling back. New regression test asserts close() is called. Also collapsed several multi-line QNBS-v3 comments in workers/v2/duckdb.worker.ts, workers/v2/inference.worker.ts, and both new test files to the required single bracketed-line format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workers/v2/duckdb.worker.ts (1)
81-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep OPFS fallback running if cleanup fails.
await opfsConnection?.close()can reject inside thiscatch, which skipsemitProgress('opfs-fallback', ...)and the fallbacknewDb.connect(), and also hides the original OPFS error. Make cleanup best-effort, log the close failure separately, and still continue to the fallback connection. Add a regression test whereclose()rejects but fallback init succeeds.🤖 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 `@workers/v2/duckdb.worker.ts` around lines 81 - 87, Make OPFS cleanup in the fallback catch best-effort: wrap opfsConnection.close() in its own error handling, log any close failure separately without replacing the original OPFS error, then always emit the opfs-fallback progress event and run newDb.connect(). Add a regression test covering a rejecting close() with successful fallback initialization.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@workers/v2/duckdb.worker.ts`:
- Around line 81-87: Make OPFS cleanup in the fallback catch best-effort: wrap
opfsConnection.close() in its own error handling, log any close failure
separately without replacing the original OPFS error, then always emit the
opfs-fallback progress event and run newDb.connect(). Add a regression test
covering a rejecting close() with successful fallback initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 061cd04e-91f6-4502-9d44-911a9ca35e0a
📒 Files selected for processing (4)
tests/unit/duckdbWorkerHandler.test.tstests/unit/inferenceWorkerHandlerV2.test.tsworkers/v2/duckdb.worker.tsworkers/v2/inference.worker.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- workers/v2/inference.worker.ts
- tests/unit/duckdbWorkerHandler.test.ts
- tests/unit/inferenceWorkerHandlerV2.test.ts
|
Addressed the outside-diff-range finding from the second CodeRabbit pass ( |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
First of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication tracked in ADR-0014. Plan:
.claude/plans/worker-generation-v1-vs-enumerated-fox.md.initDuckDb/handleQuery/handleExec/handleShutdown(workers/v2/duckdb.worker.ts) andhandleInference(workers/v2/inference.worker.ts), matching howwebllm.worker.ts'shandleWebLlmis already exported and unit-tested.tests/unit/duckdbWorkerHandler.test.ts(10 tests) andtests/unit/inferenceWorkerHandlerV2.test.ts(10 tests), mirroringtests/unit/webllmWorkerHandler.test.ts's pattern — mock the external library, exercise the real handler logic directly, no live worker/WorkerBus needed.initDuckDb()'s OPFS-unavailable catch block silently swallowed the failure. v1 (workers/duckdbWorker.ts) posts an out-of-bandOPFS_FALLBACKmessage so the UI can warn the user their analytics won't persist across reloads (private browsing, old Safari); v2 had the identical catch block but emitted nothing. Fixed by threadingctx.emitProgressintoinitDuckDb()and emitting an'opfs-fallback'progress stage — reuses the existing progress channel (same one WebLLM uses for download progress), no new message-protocol/schema change.Both v2 worker files had zero production callers before this PR (confirmed via repo-wide grep) — they were registered as live WorkerBus v2 pools but never exercised by real traffic, so this parity gate exists specifically to catch drift like the OPFS gap before any consumer (
duckdbClient.ts,localEmbeddingService.ts,localNlpService.ts) cuts over in the next 3 PRs.Test plan
pnpm exec vitest run tests/unit/duckdbWorkerHandler.test.ts tests/unit/inferenceWorkerHandlerV2.test.ts tests/unit/webllmWorkerHandler.test.ts— 25/25 passingpnpm run typecheck(exact CI command) — cleanpnpm run lint(Biome, staged files) — clean via pre-commit hook🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests