fix: QueueEvents failed events missing error, data, and context#54
Conversation
- Read event.error instead of event.data for failedReason - Include job.data in failed broadcast so listeners get the payload - Pass event context to error emission - Add data field to FailedEvent type - Add assertions for failedReason and data in test
|
@simontong is attempting to deploy a commit to the egeominotti's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughFailed job events now include the job's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/queueEvents.test.ts (1)
70-74: Add one assertion forerrorevent context to cover the new signature.Line 70 currently swallows
errorevents. Since this PR adds event context propagation, assert the second argument (JobEvent) includes the samejobIdto prevent regressions.✅ Suggested test hardening
- queueEvents.on('error', () => {}); + let errorContextJobId: string | undefined; + queueEvents.on('error', (_error, event) => { + errorContextJobId = event?.jobId; + }); @@ expect(typeof failedEvent!.failedReason).toBe('string'); expect(failedEvent!.failedReason).toContain('Test failure'); expect(failedEvent!.data).toMatchObject({ value: 42 }); + expect(errorContextJobId).toBe(job.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/queueEvents.test.ts` around lines 70 - 74, The test currently swallows error events via queueEvents.on('error', () => {}); update the test to capture the error handler's second argument (the JobEvent context) and assert that its jobId equals the jobId used in the test; specifically modify or replace the error listener registered on queueEvents.on('error', ...) to accept (err, event) and either push event.jobId to a new array or directly assert event.jobId matches the expected jobId (referencing queueEvents.on('error', ...) and the existing failedEvents push for pattern).src/application/operations/ack.ts (1)
1-308: Split this module to stay within the repository file-size limit.This file is 308 lines and currently exceeds the TS max-size rule.
As per coding guidelines
**/*.ts: MAX 300 lines per file - split if larger.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/operations/ack.ts` around lines 1 - 308, This file exceeds the 300-line limit; split it by moving related operations into separate modules: keep core single-job handlers (ackJob, failJob, AckContext) in the original file and extract batch operations (ackJobBatch, ackJobBatchWithResults and their small helper batchCtx usage) into a new file (e.g., ackBatch.ts), import/export the same symbols so external callers are unaffected, and ensure both files import shared helpers used here (groupByProcShard, extractJobs, groupByQueueShard, releaseResources, finalizeBatchAck, groupItemsByProcShard, extractJobsWithResults) and types (AckContext, JobId) unchanged; update any local imports/exports to preserve original API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/events.ts`:
- Around line 170-174: The emitted 'failed' payload sets failedReason to
event.error which can be undefined; update the this.emit('failed', { jobId:
event.jobId, failedReason, data: event.data }) call to guarantee a non-undefined
string (e.g., failedReason: (event.error ?? event.message ?? event.data?.error
?? 'Unknown failure').toString()) so consumers always receive a value, and
adjust the event typing if necessary to reflect the non-optional failedReason.
---
Nitpick comments:
In `@src/application/operations/ack.ts`:
- Around line 1-308: This file exceeds the 300-line limit; split it by moving
related operations into separate modules: keep core single-job handlers (ackJob,
failJob, AckContext) in the original file and extract batch operations
(ackJobBatch, ackJobBatchWithResults and their small helper batchCtx usage) into
a new file (e.g., ackBatch.ts), import/export the same symbols so external
callers are unaffected, and ensure both files import shared helpers used here
(groupByProcShard, extractJobs, groupByQueueShard, releaseResources,
finalizeBatchAck, groupItemsByProcShard, extractJobsWithResults) and types
(AckContext, JobId) unchanged; update any local imports/exports to preserve
original API surface.
In `@test/queueEvents.test.ts`:
- Around line 70-74: The test currently swallows error events via
queueEvents.on('error', () => {}); update the test to capture the error
handler's second argument (the JobEvent context) and assert that its jobId
equals the jobId used in the test; specifically modify or replace the error
listener registered on queueEvents.on('error', ...) to accept (err, event) and
either push event.jobId to a new array or directly assert event.jobId matches
the expected jobId (referencing queueEvents.on('error', ...) and the existing
failedEvents push for pattern).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f49d3321-725c-4b97-bdb6-cf75c15b64cb
📒 Files selected for processing (3)
src/application/operations/ack.tssrc/client/events.tstest/queueEvents.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/events.ts (1)
148-194: Good refactoring - all 12 EventType values are exhaustively handled.The
dispatchEventmethod cleanly centralizes event mapping logic. Verified against theEventTypeenum: Pushed, Pulled, Completed, Failed, Progress, Stalled, Removed, Delayed, Duplicated, Retried, WaitingChildren, and Drained are all covered.The fallback values for optional fields (
event.error ?? 'Job failed',event.prev ?? 'unknown',event.delay ?? 0) ensure consumers always receive valid payloads.Consider adding a
defaultcase with a debug log/warning to catch any futureEventTypeadditions that aren't handled:♻️ Optional: Add exhaustiveness safeguard
case EventType.Drained: this.emit('drained', { id: event.jobId }); break; + default: + // Exhaustiveness check - log unknown event types during development + console.warn(`[QueueEvents] Unhandled event type: ${(event as JobEvent).eventType}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/events.ts` around lines 148 - 194, Add a default/fallback branch to the switch in dispatchEvent to catch any future EventType values not explicitly handled: implement a default case that logs a warning (e.g., console.warn) including the unrecognized event.eventType and the event payload to aid debugging and monitoring; ensure the default does not throw so existing consumers aren't broken, but provides enough context to diagnose missing handlers for symbols dispatchEvent and EventType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/events.ts`:
- Around line 148-194: Add a default/fallback branch to the switch in
dispatchEvent to catch any future EventType values not explicitly handled:
implement a default case that logs a warning (e.g., console.warn) including the
unrecognized event.eventType and the event payload to aid debugging and
monitoring; ensure the default does not throw so existing consumers aren't
broken, but provides enough context to diagnose missing handlers for symbols
dispatchEvent and EventType.
|
thanks @simontong |
2 participants
Summary
events.tsreadevent.datainstead ofevent.errorfor the failed reasonack.tsdidn't includejob.data, so listeners couldn't see the payloaderrorevent only passed the Error object, not the originating eventChanges
src/application/operations/ack.ts: Includedata: job.datain failed broadcastsrc/client/events.ts: Readevent.errorforfailedReason, passevent.datathrough, add event context to error emissionsrc/client/events.ts: Adddata?field toFailedEventtype, updateerrorlistener signatures with optionalJobEventtest/queueEvents.test.ts: AssertfailedReasonis a string containing the error, anddatamatches the job payloadTest plan
bun test test/queueEvents.test.ts-- 10 pass, 0 failbun test-- full unit suitebun scripts/tcp/run-all-tests.tsbun scripts/embedded/run-all-tests.tsSummary by CodeRabbit
New Features
Tests