test: cover cron/resend/clerk plugins + fix cron callback fault isolation - #293
Open
madebysaira wants to merge 1 commit into
Open
madebysaira wants to merge 1 commit into
madebysaira wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The cron callback fix currently introduces a forced async (microtask) execution boundary and includes an inaccurate explanatory comment, which conflicts with the PR’s stated “no production behavior changes” intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR increases Vitest coverage for the cron, resend, and clerk plugins while also adjusting the Cron callback fan-out behavior to better isolate listener failures during /cron/callback delivery.
Changes:
- Added new Vitest suites for
plugins/cron,plugins/resend, andplugins/clerkto raise coverage with meaningful behavior assertions. - Updated
CronPlugincallback delivery to catch per-callback/per-element failures so one failing listener doesn’t disrupt other deliveries.
File summaries
| File | Description |
|---|---|
| plugins/cron/index.ts | Changes callback fan-out to isolate listener failures during callback delivery. |
| plugins/cron/index.test.ts | Adds coverage for cron utils, task persistence/scheduling, callback route fan-out, and fault-isolation behavior. |
| plugins/resend/index.test.ts | Adds coverage for Resend plugin construction and sendEmail request/error handling behavior. |
| plugins/clerk/index.test.ts | Adds coverage for Clerk plugin construction defaults/validation and key early-exit paths in auth/session checks. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+79
to
+90
| // onEvent wraps callbacks async without awaiting them, | ||
| // so sync throws escape as unhandled rejections and | ||
| // abort delivery to later subscribers. Catch per | ||
| // element so one bad subscriber never starves the rest. | ||
| Promise.resolve() | ||
| .then(() => callback(element)) | ||
| .catch((error) => { | ||
| console.error( | ||
| 'Error in Cron event callback:', | ||
| error | ||
| ) | ||
| }) |
| ) | ||
| expect(updateCall).toBeDefined() | ||
| const params = updateCall[0].params as unknown[] | ||
| expect(params[0]).toBe('soon') |
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
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.
Contributes to the $250 bounty #71 (Vitest coverage above 75%).
New coverage (3 files, 28 tests, all green locally)
Real bug found while testing + fixed in this PR
Cron callback fault isolation (plugins/cron/index.ts): onEvent wraps callbacks async without awaiting them, so a sync listener throw escaped as an unhandled rejection AND aborted delivery to all later subscribers — one bad subscriber starved every other subscriber. The callback route now invokes each callback via Promise.resolve().then().catch() per element, so failures are logged per-subscriber and siblings still receive events. Test 'callback errors are contained per-listener (fault-isolation improvement)' covers it.
Validation
/claim #71