Uh oh!
There was an error while loading. Please reload this page.
🐛 Avoid aborting settled fetches - #234
Conversation
📝 WalkthroughWalkthrough
ChangesFetch cancellation lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant EffectionScope
participant FetchApi
participant NativeFetch
EffectionScope->>FetchApi: start fetch operation
FetchApi->>NativeFetch: fetch(input, signal)
NativeFetch-->>FetchApi: settle response or rejection
FetchApi->>FetchApi: mark request settled
EffectionScope->>FetchApi: run cleanup
FetchApi->>NativeFetch: abort if request is unsettled
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
commit: |
Uh oh!
There was an error while loading. Please reload this page.
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 `@fetch/fetch.test.ts`:
- Around line 203-259: Migrate the tests in the cancellation describe block of
fetch/fetch.test.ts to the Node.js test runner, replacing `@effectionx/vitest`
usage with the corresponding test APIs from `@effectionx/bdd`. Update imports and
test declarations to use the required bdd helpers while preserving both
cancellation assertions and existing test behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67b42de5-5555-4a86-8925-5b1801d0aa25
📒 Files selected for processing (5)
fetch/README.mdfetch/api.tsfetch/fetch.test.tsfetch/fetch.tsfetch/package.json
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Motivation
Node's native fetch can raise a late Undici exception when Effection tears down a scope and aborts a request whose fetch promise has already settled. This is reproducible with a
data:URL and can surface after the owning operation has completed.Closes#232.
Approach
@effectionx/fetchto0.2.1.data:regression test plus deterministic in-flight cancellation coverage.The lifecycle boundary is the native fetch promise's settlement: once it produces a
Response(or rejects), later scope teardown does not abort it.Verification
pnpm test— 384 passed, 6 skippedpnpm checkpnpm lintpnpm fmt:checkpnpm buildpnpm syncSummary by CodeRabbit
Bug Fixes
Documentation
Chores