Repository navigation
fix(runtime-browser): isolate beforeSend failures - #22
Open
rajeshaipython-stack wants to merge 1 commit into
Open
rajeshaipython-stack wants to merge 1 commit into
rajeshaipython-stack wants to merge 1 commit into
Conversation
🚦 Pre-merge checks · ✅ 168 passed✅ Passed checks (168)
This comment is updated automatically whenever Autter reviews a new PR revision. |
💡 1 suggestion(s) — conventions, hardening and hygiene, not defectsInline comments are reserved for concrete defects. These are things worth knowing that the diff does not prove wrong — a missing hardening layer, a convention the repo usually follows, a file that historically changes alongside one you touched. Skim, adopt what fits, ignore the rest.
|
🤖 Release Notes CuratorImpact: patch — Isolates throwing Changelog entry (ready to paste):
Custom agent · runs after review · configured in Autter |
![autter-dev[bot]](https://avatars.githubusercontent.com/in/2920597?s=60&v=4){kind=small}
| try { | ||
| const mapped = opts.beforeSend(event); | ||
| if (!mapped) return; | ||
| event = mapped; |
There was a problem hiding this comment.
🟠 [deterministic] Biome: lint/style/noParameterAssign — Risk: 55/100
Reassigning a function parameter is confusing.
🛠 AI fix prompt (copy & paste into your coding agent)
Fix the Biome `lint/style/noParameterAssign` issue at packages/runtime-browser/src/index.ts:317: Reassigning a function parameter is confusing.
Flagged by Autter security & observability checks.
🧪 Autter test runAutter checked This PR changes 2 source files. No project test command ran for the touched workspace. It also executed 4 checks from the PR's test plan. Targeted agent checks: Autter ran targeted checks against the files this PR changed — it executed uncovered test-plan items and, where a changed file had no matching test, wrote a temporary test to verify the new behavior. Execution summary: 8 checks executed · 8 passed. Project test commandsNo project test commands were detected in this repository. Autter targeted verification5 tests executed · 5 passed. Code-level simulationsRevision
Declared tests: 16 test file(s) found — 1 ran, 0 not observed in suite output, 15 did not run. The project's command reported 0 of them. Autter ran the other 1 file one by one so every declared test has a verdict (1 passed, 0 failed). "Did not run" means no test command that Autter executed covers the file, so nothing in it was checked.
|
| Changed file | Related test | Result |
|---|---|---|
packages/runtime-browser/src/index.ts |
packages/runtime-browser/test/before-send.test.mjs |
✅ existing test passes |
🤖 Coverage-check evidence
packages/runtime-browser/src/index.ts
Temporary tests are written under .autter/scratch/ for verification only — they are never committed to the repository.
Test plan (from the PR description)
- ✅ Run
npm run build -w @autter/runtime-browserand confirm clean build with no new dependencies. — verified by agent execution - ✅ Run
npm run test -w @autter/runtime-browserand confirm newbefore-send.test.mjspasses. — verified by agent execution - ✅ Run
npm run size -w @autter/runtime-browserand confirm<5 KBbrotlied budget holds. — verified by agent execution - ✅ Manually verify with throwing
beforeSendthat page does not error and follow-on events still send via relay. — verified by agent execution
🤖 Agent-executed checks
✅ Run npm run build -w @autter/runtime-browser and confirm clean build with no new dependencies.
tsup ESM build success; dist/index.js 17.03 KB; DTS build success; no package manifest or lockfile changes.
✅ Run npm run test -w @autter/runtime-browser and confirm new before-send.test.mjs passes.
9 tests passed, 0 failed; beforeSend failures preserve application requests and later delivery passed.
✅ Run npm run size -w @autter/runtime-browser and confirm <5 KB brotlied budget holds.
Size limit 5 kB; measured 3.43 kB minified and brotlied.
✅ Manually verify with throwing beforeSend that page does not error and follow-on events still send via relay.
Focused scenario passed: throwing beforeSend does not throw from captureMessage, sends no dropped event, preserves fetch response/network error, and a later mapped event is delivered through mocked n…
⬜ items could not be verified automatically and still need a manual check.
Autter product walkAutter did not drive this change in a browser. This PR changes a backend/API, but Autter could not find a |
There was a problem hiding this comment.
Autter approved #22 with notes: the merge gate is clear, and 2 non-blocking finding(s) remain as review detail. (Also detected: 4 finding(s) dismissed as likely false positives by verification.)
Autter task listNo concrete follow-up tasks were generated for this PR. Generated from PR diff, blast radius, and context. Issues foundNo unresolved code findings remain for this revision — 4 finding(s) dismissed as likely false positives by verification. Dismissed findings stay listed with their verdicts in the Autter review dashboard. 2 additional non-blocking finding(s) are available in the Autter review. |
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
1 participant
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.
A throwing
beforeSendcallback currently escapes the browser SDK. When network capture observes an HTTP 503 response, that exception changes the application's resolved fetch into a rejection. On a network failure, it also replaces the original error.Catch callback exceptions and drop the affected telemetry event, preserving the original request result. Document this behavior and add regression coverage for response identity, original network errors, manual capture, intentional drops, and subsequent event mapping/delivery.
Validation:
git diff --checkpasses.Summary
Summary generated by Autter.
Why this change
User-supplied
beforeSendruns inside the browser SDK enqueue path, so a throwing callback risks breaking event capture and leaking errors into the host app. This change isolates those failures to preserve the SDK's fail-open behavior.What changes for users
Browser consumers keep using
beforeSendto mutate or drop (return null) events before send, but hook failures are now contained to the affected event. No wire-format or relay/ingester changes;@autter/runtime-next/clientre-exports pick up the fix automatically.Implementation and review notes
enqueueisolation inpackages/runtime-browser/src/index.ts(~L311–L324) — confirmbeforeSenderrors cannot throw or stall queue/flush.AutterBrowserOptions.beforeSenddoc touch-up (~L32) matches implemented throw-drops-event semantics.packages/runtime-browser/test/before-send.test.mjsfor throwing / drop / mutate coverage.<5 KBbrotlied size gate and zero-dependency constraint.Changes
opts.beforeSendinvocation insideenqueue()inpackages/runtime-browser/src/index.tswith try/catch so a throwing hook drops only the affectedBrowserEventand never propagates into the host app, queue, or flush timer.beforeSendcontract: return mutated event to send, returnnullto drop; throwing is now explicitly defined as drop-event fail-open path.AutterBrowserOptions.beforeSenddoc comment to document throw-drops-event semantics.packages/runtime-browser/test/before-send.test.mjscovering throwing hook,nulldrop, and mutate passthrough./v1/browserv1 payload,BrowserEventshape, relay validation, or ingester normalization; additive fail-open fix only.Acceptance Criteria
beforeSenddoes not throw fromenqueue/capture*and drops that event while subsequent events still flush.nulldrops the event; returning a mutated event sends the mutation.Test Plan
npm run build -w @autter/runtime-browserand confirm clean build with no new dependencies.npm run test -w @autter/runtime-browserand confirm newbefore-send.test.mjspasses.npm run size -w @autter/runtime-browserand confirm<5 KBbrotlied budget holds.beforeSendthat page does not error and follow-on events still send via relay.Rollback Plan
dc1513aand rebuild@autter/runtime-browser; no migration or ingester change to undo.Related Issues
No linked issue was identified in branch, commits, or diff context.
Written for commit dc1513a. Summary will update on new commits.