You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
docs(design): async origin capture for stackless rejections (BGSVIEWER-14) - #175
Design doc, measurements and probes only: no behaviour change in this PR. The doc has since been corrected against the review findings and brought to its as-built state (it records what #176 built and what real engines taught), so it cites files that exist once #176 has merged. Land #176 first, then this.
BGSVIEWER-14 is an unhandled NotSupportedError: The element has no supported sources. with zero frames. Measured in headless Chromium 156 and Firefox 151 (docs/design/async-origin-capture/probe.mjs): platform promises that fail asynchronously reject with no stack in Chromium (and fewer in Firefox); user-code Error rejections already carry stacks, so the global Promise needs no patching. The doc lists the measured stackless set, the 187 candidate methods from lib.dom.d.ts, a generic call-site wrapper (with the unhandledrejection-preserving derived-promise trap), costs/risks, and a plan.
WebKit was not part of the original measurements; it was run later in the e2e (design doc §12).
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Documentation
Added design documentation describing browser differences in asynchronous rejection stacks, candidate APIs, and a proposed approach for capturing call-site origins.
Documented edge cases, compatibility and performance considerations, testing plans, and unresolved platform questions. No runtime behavior changes are included.
Chores
Added browser-based tools to measure promise performance and inspect how different browsers report synchronous errors and unhandled rejections.
The PR adds a design document for capturing call-site origins of stackless asynchronous rejections. It also adds a Playwright probe that records rejection behavior across browser APIs and a benchmark that measures promise-wrapper and error-stack costs. The design document describes proposed capture behavior and related crash-pipeline changes. No implementation of the proposed design is included.
Suggested labels:javascript
Change: Other
Merge Risk: Low · previous review 3022f
This research-only change is mergeable with bounded follow-up. The scripts need portability and timeout-label fixes, and the design needs clarification before implementation. No production behavior changes.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly identifies the design topic, async origin capture, and related issue. It matches the documentation and measurement changes.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description check
The description states this PR adds a design document and measurement probes for async origin capture of stackless rejections (BGSVIEWER-14). It names the measured browsers, the probe script, and the …
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.)
Full details: Description check
Explanation
The description states this PR adds a design document and measurement probes for async origin capture of stackless rejections (BGSVIEWER-14). It names the measured browsers, the probe script, and the candidate-method catalog. It also states that no behavior changes are made. This relates directly to the changeset, so the check passes.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 93dc9084-b690-4434-9815-0299dcdcd45e
Commits
Reviewing files that changed from the base of the PR and between cc94ce3 and 3022f87.
Files selected for processing: 1
docs/design/async-origin-capture.md
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Review of PR #175: async origin capture design (BGSVIEWER-14)
This is a docs-only PR (a design doc and a probe script), so nothing was executed. I read both files and checked the doc's claims against packages/core/src/crash.ts and client.ts. I did not re-run the probe, so the browser measurements are taken as stated.
Read the design doc and probe
Cross-check the doc's claims against the existing code (crash.ts, client.ts, detection-providers.ts)
Post inline findings
Findings (inline)
P2 (async-origin-capture.md:153): the planned reuse of the syntheticFrames seam won't work for the target bug. The seam is only honoured when !(error instanceof Error), and a DOMException is an Error. The plan needs to extend the Error branch of buildCrashJson so it falls back to the remembered origin when the parsed frames are empty.
P3 (:152): the "has usable frames" regex treats any @ in the message as a frame, so the origin is dropped for stackless errors whose message contains @. Use parseStack instead.
P3 (:48): structuredClone is synchronous and its failure is a lost async stack, not a platform-API origin. The wrapper can't fix it, but it sits in the table the wrapper is meant to fix. There is also a small inconsistency between ".stack is undefined" (§1) and "absent or only Name: message" (§2).
P3 (probe.mjs:3): the script hard-codes the author's local absolute path, so it isn't reproducible. Its frame-counting regex can also overcount.
Summary
Overall risk: Low. There is no behaviour change, and the findings are about the accuracy of the design.
Merge recommendation: Approve after fixing finding 1 in the doc. Findings 2–4 can be fixed in the same pass or deferred.
Most important to fix: the instanceof Error gap in the seam (finding 1), because it would leave the original frames: [] bug unfixed if the plan were implemented as written.
Positives:
The measurement-first approach is useful, with a clear per-engine table.
The derived-promise trap that preserves unhandledrejection is called out.
The cost and risk section is honest.
Drift test and e2e are planned.
Not patching the global Promise is the right call.
Re-review of PR #175: async origin capture design (BGSVIEWER-14)
This push added §4's "one factory" section and bench.mjs. It is still docs and scripts only. I read both files and checked the doc against packages/core/src/crash.ts and packages/browser/src/detection-providers.ts. I did not run the probe or the bench, so the browser measurements are taken as stated.
Read the new revision and the earlier review comments
Check whether earlier findings were addressed
Post new findings inline
Earlier findings: none fixed yet
The two new commits only added the factory and bench sections, so these still apply:
P2async-origin-capture.md:153: the syntheticFrames seam is ignored for Error instances, and DOMException is one (crash.ts:333).
P3:152: the @ in the has-frames regex.
P3:47-48: the structuredClone and WebAssembly rows aren't addressed by the wrapper.
P3probe.mjs:3: hard-coded local path.
New findings (inline)
P3bench.mjs:3: the same hard-coded /Volumes/... path. The §4 overhead table, which is the main argument for default ON, can't be reproduced, and PW_firefox isn't documented.
P3async-origin-capture.md:207 (plan step 3): describeError (browser/src/detection-providers.ts:28-38) builds the report description from value.stack, separately from crash.json. Fixing only buildCrashJson leaves the description without a location. Route both through one helper and test that they agree.
P3async-origin-capture.md:153: "frames tagged as async origin" has no wire field. CrashFrame (crash.ts:12-30) has no marker, and no plan step touches the protocol schema or says how grouping treats origin frames. I did not check the worker, which is outside this repo.
Summary
Overall risk: Low. The PR changes no behaviour, so the risk is that the design misleads the implementation.
Merge recommendation: Fine to merge as a design doc once the P2 instanceof Error gap is written into the plan. Otherwise the plan, implemented as written, would still ship frames: [] for BGSVIEWER-14.
Most important to fix:
The Error branch of buildCrashJson must fall back to the remembered origin (:153).
Define the wire marker and grouping behaviour for origin frames before step 2 (:153).
Use parseStack rather than the regex to decide a reason is stackless (:152).
Cover describeError and the description in step 3 (:207).
Positives:
Measurement comes first, and the new bench quantifies the cost.
The derived-promise trap is handled correctly.
The factory-versus-hand-written comparison is a fair control.
The unmeasured items (WebKit, memory, microtask order) are listed openly.
Re-review of PR #175: async origin capture design (BGSVIEWER-14)
This push added the "which listed methods can sit in a tight loop" class table and the adaptive circuit breaker. The PR is still docs and scripts only. I read the new sections and checked the claim about the SDK's own calls against packages/util, packages/browser-utils and packages/trackers. I did not run the probe or the bench, so the browser measurements are taken as stated.
Earlier findings, none fixed in this push (see the existing inline comments):
P2: the syntheticFrames seam is ignored for Error instances, and a DOMException is one (:153).
P3: the has-frames regex treats any @ as a frame (:152).
P3: the structuredClone and WebAssembly rows can't be fixed by the wrapper (:47-48).
P3:describeError isn't covered by the plan (:229).
P3: the "async origin" tag has no wire field (:153).
P3:probe.mjs and bench.mjs hard-code /Volumes/External2TB/..., so they can't be reproduced.
New findings (inline)
P3 (async-origin-capture.md:184): "the SDK's own code is not affected" is incomplete. The SDK calls crypto.subtle.digest (util/src/sha256.ts:33), response.arrayBuffer() (browser-utils/src/fetch-transport.ts:57) and StorageManager.estimate/persisted (trackers/src/browser/storage-tracker.ts:26-27). These would share the app's per-method breaker budget and pay the capture cost. The wrappers should be bypassed for SDK-internal calls.
P3 (:194-198): the breaker's "one increment and one comparison / 0.2 µs" omits the window mechanism (a clock read or a timer). It isn't in bench.mjs, and the rejection path isn't benchmarked. The new class and breaker design is also not reflected in cost-and-risk item 1 (denyHot) or in plan step 2.
Summary
Overall risk: Low. There is no behaviour change, and the risk is that the design misleads the implementation.
Merge recommendation: Fine to merge as a design doc once the P2 instanceof Error gap from the earlier review is written into the plan.
Most important to fix:
The Error branch of buildCrashJson must fall back to the remembered origin (:153).
Define the wire marker and grouping behaviour for origin frames.
Exempt SDK-internal calls from the wrappers and the breaker.
Measure the breaker and reconcile the plan with the class table.
Positives:
The breaker is a sensible answer to "frequency is a property of the app, not the API".
The class table is explicit about which methods are on or off by default.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Deep review — async origin capture design (BGSVIEWER-14)
Docs only; no runtime change. The diagnosis is right: HTMLMediaElement.play() (and similar platform async failures) reject from a media/C++ task with a real DOMException whose .stack is missing, so nothing at detection time can recover a call site. The wrapper shape — call-time new Error(), return the derived promise that rethrows — matches the existing fetch interceptor (packages/capture/src/fetch-interceptor.ts:387-433) and correctly preserves unhandledrejection. Not patching the global Promise is the right call. The generator + drift test + real-browser e2e plan is the right honesty mechanism.
The consumption path as specified would not fix BGSVIEWER-14. That has to be corrected before this doc is used as the implementation spec.
Findings
P1 — syntheticFrames is ignored for real Errors, and the motivating report is a real Error
buildCrashJson only consults syntheticFrames on the !(error instanceof Error) branch (packages/core/src/crash.ts:332-355). A tested invariant forbids putting them on an Error (crash.test.ts:187-193: “does NOT put synthetic frames on a real Error — it has its own”). logException independently sets syntheticFrames to undefined whenever error instanceof Error (packages/core/src/client.ts:1757-1759). There is no captureException (removed in design v3; the public API is logException).
BGSVIEWER-14’s reason is NotSupportedError / DOMException, which is an Error. Plumbing the WeakMap origin in through syntheticFrames is a silent no-op: crash.json.exception.frames stays [], and the worker still has no grouping location (crash.ts cites worker/crash/managed/common.py:88).
The same consumers also drop the human-readable stack: describeError in packages/browser/src/detection-providers.ts:28-37 returns { summary } with no description when !value.stack. Even a future crash.json fix would leave request.json.description empty unless that path is specified too.
The seam that already runs on Error objects is enrichFrames (receives the error plus whatever parseStack produced — [] when .stack is missing). Node’s crashOf already threads it for this reason (packages/node/src/detection-providers.ts:39-52); browser crashOf does not.
P2 — remember() treats any engine frame as “usable”, so two measured rows stay unfixed
The predicate !/ \s+at |@/.test(stack) refuses to store an origin as soon as the reason has any at/@ line. §2 lists WebAssembly.compile/instantiate and structuredClone as Chromium “1 frame, no user frame”. Those stacks match the regex, so origin is dropped and the report still has no call site. structuredClone is also a sync throw, not a thenable; the generic wrapper returns p unchanged (or lets the throw escape) and never reaches remember.
P2 — default set and denyHot both claim Body.*
§2 measures Body.json as stackless on Firefox. §7.2 defaults ON for the measured set. §4 cost #1 puts Blob/Body.* on denyHot. One of those three statements has to give, or the Firefox json() case either stays broken or every Response#arrayBuffer/text/json pays new Error() + an extra promise.
P2 — “async origin” has no wire field; hidden would undo grouping
CrashFrame / StackFrame have no origin/kind tag. toCrashFrame is a closed allow-list (crash.ts:141-168), so a tag left only on StackFrame is dropped. The only existing “not a real throw site” flag is hidden, which the worker skips when picking the crash location (crash.ts:17-20). Marking origin frames hidden would leave BGSVIEWER-14 ungrouped again. Viewer distinction needs a new optional field that grouping ignores; until then, origin frames must be ordinary visible user frames.
Residual (not blocking)
probe.mjscreateRequires a hardcoded /Volumes/External2TB/... path, so the measured table cannot be re-run from this checkout. Plan §5 depends on turning that probe into e2e assertions.
WebKit is unmeasured (already an open question). Default-ON wrapping of play()/clipboard/getUserMedia should stay gated until a matching Playwright WebKit build exists.
Electron renderer is covered if the interceptor is installed from @bugsee/browserlaunchCore; no separate electron-renderer provider is required.
Overall risk: Medium
Docs-only, so this does not break production. Implementing the doc as written would ship the wrappers and still upload frames: [] for the bug that triggered the work.
Merge recommendation: Request changes
Most important to fix before this becomes the implementation spec:
Consume origin via enrichFrames (and describeError) on empty-stack Errors, not syntheticFrames.
Define “usable frames” as a user/call-site frame, not any engine line; handle sync stackless throws separately or drop them from the default set.
Resolve Body.json (default vs denyHot) and add a crash.json field if the viewer is expected to render origin frames distinctly.
Did not run the in-tree probe (host-absolute Playwright path). Read buildCrashJson / logException / browser+node detection providers / fetch interceptor against the doc.
Docs and measurement scripts only — no production behaviour change. I checked the plan against packages/core/src/crash.ts, packages/core/src/client.ts (logException / describeError), packages/core/src/crash.test.ts (syntheticFrames is ignored for a real Error), packages/browser/src/detection-providers.ts, and the existing fetch wrapper in packages/capture/src/fetch-interceptor.ts. I did not re-run probe.mjs / bench.mjs (both hard-code a host-absolute Playwright path).
Findings
P1 — syntheticFrames cannot attach origin frames to BGSVIEWER-14. The reason is a DOMException (Error); that seam is ignored for Errors. describeError and logException have the same gap, and there is no captureException.
P2 — remember's "usable frames" regex treats any engine frame (and any @ in the message) as located, so the WASM / 1-frame rows never get an origin.
P2 — Default-on measured set and denyHot both claim Body.* / Body.json.
P2 — createImageBitmap is default-on because it is stackless, then placed on the hot list whose breaker turns origin capture off in the frame-rate loops that justified calling it hot.
Overall risk: Low
No runtime change. The risk is treating this doc as the implementation spec.
Merge recommendation: Request changes
Fix the P1 seam in the plan before this is the spec for step 2/3. Resolve the three P2s in the same pass so the default set is internally consistent.
Most important to fix
Empty-stack Error fallback (enrichFrames, or extend the Error branch). Same helper for describeError / logException. Drop the captureException name.
Define "usable" via parseStack (user/call-site frames, not "any at/@").
One owner for Body.json.
Do not default-on createImageBitmap if the breaker will disable it on the hot path.
Do not mark origin frames hidden — the worker skips hidden when picking the grouping location, which would restore frames: [] grouping for BGSVIEWER-14.
Positives
Measurement-first, with a Chromium/Firefox split and WebKit called out as unmeasured.
The derived-promise trap is correct and matches the existing fetch interceptor (call.then(..., err => { …; throw error })).
Not patching the global Promise is the right call.
One factory + drift test + real-browser e2e is the maintainable shape.
Overhead was actually measured, including a hand-written control.
Re-review of PR #175: async origin capture design (BGSVIEWER-14), v2
This push rewrote §4 into a two-mode wrapper with an enrichFrames seam and arm-on-first-failure gating. It is still docs and scripts only. I read the full doc and checked its claims against packages/core/src/crash.ts, debug-id.ts, client.ts, and the browser and node detection-providers.ts. I did not run the probe or the bench, so the browser measurements are taken as stated.
Earlier findings now fixed in the doc
The syntheticFrames seam is replaced by enrichFrames (§4.5), and the cited line numbers match the code. Both Cursor reviews and the earlier P2 raised this.
describeError and the handled logException path are covered in §4.5.
The rate breaker is replaced by arm-on-first-failure (§4.4).
The SDK-wide @-regex is removed. See "usable frames" below, which is still open.
Still open from earlier reviews
The wire marker for origin frames is acknowledged only as an open question (§8). It must be decided before step 4.
The SDK's own calls to wrapped methods are not exempted: crypto.subtle.digest, response.arrayBuffer() and StorageManager.estimate. The doc line :255-257 is still wrong about this.
The structuredClone and WebAssembly rows are still listed as covered by the wrapper.
Body.json and createImageBitmap are still contradictory between the measured set and the hot list.
probe.mjs and bench.mjs still hard-code /Volumes/... paths.
New findings (inline)
P2 (:216): origin frames returned by enrichFrames never get a debug_id, because applyDebugIds runs before enrichment (crash.ts:202 vs :206). A minified call site would be unsymbolicatable. Also unclear is how the SDK wrapper frame is stripped on Firefox and Safari, which lack captureStackTrace.
P2 (:200-202): "armed after its first rejection" has no mechanism. Observing a rejection requires attaching a reaction, which is the §4.1 trap. As written the gate is circular or never fires, and the first failure of the measured rows (decode, FontFace.load, addIceCandidate) is lost by design.
P2 (:261-272): the v2 rewrite left the old policy next to the new one. The table still says off, opt-in and "see breaker", and cost-and-risk item 1 still describes denyHot. There are now three incompatible policies and the options shape cannot be derived.
P3 (:223): §10 is referenced three times but does not exist. The doc ends at §9.
P3 (:175-180):
"usable frames" is now undefined.
The bench does not measure the proposed new Error() plus captureStackTrace combination.
Patched-then mode retains the origin and target until the promise is GC'd, not until it settles.
P3 (:312): step 6 ignores the spec-first rule for system event names in CLAUDE.md (system-names.ts, spec:refresh, drift test).
P3 (probe.mjs:3): the repo-relative path fix is still missing. Several §4.1-§4.2 numbers come from scripts that are not in the PR.
Summary
Overall risk: Low. There is no behaviour change. The risk is that the design misleads the implementation.
Merge recommendation: Fine to merge as a design doc once the P2 items are fixed in the text, or tracked as open questions: the debug-id gap, the arming mechanism and the single gating policy.
Most important to fix:
Specify how the origin frames get debug_id, and how the wrapper frame is stripped on non-V8 engines.
Define the arming trigger without a reaction, or keep the measured-stackless rows always-on.
Reconcile the table, cost-and-risk and §4.4 into one always/armed/off policy.
Add the missing §10, or drop the references.
Positives:
The v2 revision responds to review and measures its claims, including the zone.js regression.
The two-mode wrapper is a good answer to the derived-promise trap.
Keeping this as the root-cause signal (§4.3) is a real improvement for play().
The enrichFrames seam is the right one and matches how node already threads it.
Docs + measurement scripts only — no production behaviour change. HEAD b18fe94a vs last review (f837dc0f). Checked the v2 plan against packages/core/src/crash.ts (buildException / enrichFrames / toCrashFrame / syntheticFrames Error invariant), packages/core/src/client.ts (logException, describeError, callSiteFrames), packages/browser/src/detection-providers.ts (browser crashOf still does not thread enrichFrames or event.promise), packages/angular/src/error.ts, packages/capture/src/fetch-interceptor.ts, and the fetch/sha256/storage call sites. Did not re-run probe.mjs / bench.mjs (host-absolute Playwright path). Did not run unit tests. CI lint on this branch is red (Biome on the new .mjs files; run 41444497535, and the HEAD lint job).
Prior findings at f837dc0f
P1 syntheticFrames / empty-stack Error — fully addressed. §4.5 now names enrichFrames (crash.ts:196-206) and the browser crashOf / describeError gaps correctly. buildException does call the enricher when parsed = []; that is the working seam.
P2 remember regex — partially addressed. The / \s+at |@/ predicate is gone. “usable frames” is still undefined, and §4.5 applies origin only when parsed.length === 0, so the Chromium WASM “1 frame, no user frame” and sync structuredClone rows in §2 stay unfixed.
P2 Body.json vs denyHot — not addressed. Still three owners (measured default-on, table off, denyHot).
P2 createImageBitmap + breaker — partially addressed. The 200/s breaker is gone; arm-on-first-failure still drops the one-shot failure that §2 measured, and the table still says “see breaker”.
P2 origin wire / hidden — partially addressed. Now an §8 open question. Residual: do not mark origin frames hidden — the worker skips those when picking the grouping location (crash.ts:17-20), which would restore empty grouping for BGSVIEWER-14. toCrashFrame is still a closed allow-list, so a tag left only on StackFrame is dropped.
P3 probe path — not addressed, and it now fails the lint gate.
New findings (v2)
The remaining P1 is the zone.js mode this revision added: patched-then lookup cannot retrieve origin or target state for awaited / logException paths, which is how Angular writes play().
Overall risk: Medium
No runtime change. Implementing v2 as specified would fix bare Chromium play() (rare set + enrichFrames) and would still ship frames: [] with no element state for Angular/await video.play(), leak <video> nodes via byReason, and contradict itself on Body.json / one-shot decode.
Merge recommendation: Request changes
Fix the patched-then lookup (or drop the false §4.3 fallback), pick one default policy per API, and make the probe lint/run from this repo before this is the implementation spec.
Most important to fix
Patched-then must store byReason (iframe-realm native then — already measured) so awaited and adapter logException paths work. Write §10. There is no captureException.
Always capture for the measured-stackless set; arm-on-first-failure skips the failure that matters. Delete leftover “breaker” wording.
One owner for Body.json.
Do not pin target in a WeakMap keyed by the exception; snapshot describer fields on reject. sanitizeUrl the system-event src.
Repo-relative Playwright require + Biome-clean scripts (CI lint is red).
Positives
The v1 P1 (wrong seam) is actually fixed in the plan, with the right file:line citations.
Two-mode wrapping is grounded in a real zone.js 0.14.10 measurement, not speculation; not patching Promise is still the right call.
Target/this as the stronger signal for play() is the right product insight; the capture-phase error listener is a good complement if src is scrubbed.
callSiteFrames already exists for wrapper-frame stripping on Chromium/Firefox/WebKit (stack.ts:272-276); the factory should use it.
Drift test + Chromium and Firefox e2e + a zone.js composition test is the right honesty mechanism.
This push adds §4.1a (pristine then), the §10 framework and custom-Promise survey, and by-reason lookup everywhere. It is docs only. I read the diff against b18fe94a and checked its citations against crash.ts, client.ts, browser/src/detection-providers.ts, the framework adapters and samples/angular-spa. I did not re-run the probe, bench or any zone.js measurements, so those are taken as stated.
Earlier findings now fixed in the doc
Patched-then lookup (Cursor P1): the lookup is now by reason on every path, via a pristine then. This also removes the zone.js regression from the previous version.
§10 was referenced but missing: it now exists.
enrichFrames reaching more paths: §4.5 and §10.3 now route the enricher through crashOfErrorEvent and the adapters. Client already takes an enrichFrames option (client.ts:585,1769), so the adapters inherit it without per-adapter wiring.
The §10 citations I checked match the code:crashOfErrorEvent at detection-providers.ts:54, samples/angular-spa pinning zone.js ~0.14.10, and the adapter files.
Still open from earlier reviews (not repeated inline)
Arming: arm-on-first-failure has no mechanism, and it loses the first failure of the measured rows (decode, FontFace.load, addIceCandidate, createImageBitmap).
One policy per row:
denyHot, "see breaker" and "guarded by the breaker" are still in the text (:286, :288, :297).
Body.json and createImageBitmap are still claimed by both the always-on and the hot lists.
debug_id and wrapper frame:
Frames added by enrichFrames never get a debug_id, because applyDebugIds runs before enrichment (crash.ts:202 vs :206).
Wrapper-frame stripping on Firefox and Safari is unspecified.
Target retention:
A WeakMap keyed by the reason keeps the HTMLMediaElement alive for as long as the app holds the error.
Firefox's late error event is not handled by reading at report time.
The src in the capture-phase listener needs sanitizeUrl.
Wire marker: the origin-frame marker is still an open question (§8). The frames must not be hidden.
probe.mjs:3 and bench.mjs:3: both still hard-code /Volumes/.... Biome lint failed on these files in an earlier CI run and I did not re-check it. Resolve Playwright relative to the repo.
Spec-first rule: step 6's new system event does not say that the spec comes first (system-names.ts, then spec:refresh).
New findings (inline)
P3 (:181-187, §4.1a):
The "still native at install" test has no definition now that the toString check is gone.
Step 2's "return p untouched" loses unhandledrejection if a reaction was already attached.
The hidden iframe's lifecycle and side effects are unstated: DOM replay serialisation, CSP, window.length, and what happens when it is removed.
P3 (:347): §8 still says awaited chains under zone.js "cannot be located". §4.1a and §10 now say they can.
P3 (:382): the Redux Toolkit "name+message match" cannot be built on the WeakMap store. A strong map would leak and could cite the wrong call site.
Also, reportReactError is in packages/react/src/report.ts, not error-boundary.ts.
Summary
Overall risk: Low. There is no behaviour change. The risk is that the design misleads the implementation.
Merge recommendation: Fine to merge as a design doc once the open P2 items are resolved in the text or recorded as open questions. Those are arming, one policy per row, debug_id on enriched frames, target retention and the lint failures on the .mjs scripts.
Most important to fix:
Define how the arming trigger works, or make the measured-stackless rows always-on.
Reconcile the table, cost-and-risk and §4.4 into one always/armed/off policy.
Specify debug_id for enriched frames and the wrapper-frame strip on non-V8 engines.
Snapshot the target description at rejection time instead of pinning the element.
Make the scripts repo-relative and lint-clean.
Tighten §4.1a as above.
Positives:
The framework survey is a large improvement. It found that capturing the global then breaks play() when Promise is replaced, and the design now guards against that.
Lookup by reason is the right invariant, and §10.4 lists what is unmeasured.
Re-review of PR #175 (async origin capture design, BGSVIEWER-14), pushes 90b9cf7f and aad867c0
These two commits only change biome.json. They exclude docs/design/async-origin-capture from Biome. The doc is unchanged since 3022f870. I re-read the doc, both scripts and crash.ts. I could not run pnpm/biome here (pnpm: command not found), so I did not re-run lint or the probes.
Fixed by these commits
The Biome failures on probe.mjs and bench.mjs (format, noAsyncPromiseExecutor) should be gone, because the files are no longer linted. I could not confirm this locally. Please check the CI lint job.
The excluded directory has no other contents, so nothing else drops out of lint.
New finding (inline, async-origin-capture.md:154)
P2 The wrapper puts an SDK frame into stacks that are already well located. isUserFrame (crash.ts:183-190) recognises SDK frames only by /@bugsee/ in the path. In a bundled app the wrapper frame is app.min.js:…, so it counts as a user frame. For APIs that throw synchronously, and for rejections whose stack is built at call time, the wrapper becomes the first non-hidden frame. That changes the grouping location for reports that work today. §4.2 only trims the wrapper from the SDK's own origin Error, not from stacks the engine builds. The doc needs a way to hide or strip these frames, plus an e2e in a minified bundle.
Still open from earlier reviews (not repeated inline)
P2 Arm-on-first-failure has no trigger and drops the first failure of the measured rows. The table still says "see breaker" and denyHot.
P2Body.json and createImageBitmap each have more than one owner.
P2byReason keeps target (the element) alive for as long as the app holds the error. "Released on settle" is wrong for a reason-keyed store.
P2 Core describeError (client.ts:171-184) is not named. captureException does not exist.
P2 §8 line 349 still says awaited zone.js chains cannot be located. That contradicts §4.1.
P2 The enricher-level .cause walk duplicates buildException's own recursion.
P2debug_id is applied before enrichFrames (crash.ts:202 vs :206), so origin frames get none.
P2/P3 Decided origin: 'call':
It is not carried by the plan. CrashFrame and toCrashFrame (crash.ts:141-168) drop unknown fields, and no step adds the field and schema ahead of step 4.
§4.5 line 236 and step 7 line 340 still call the protocol an open question.
Grouping behaviour, and how an older worker or viewer treats the frames, are not stated. The frames must not be hidden.
P3probe.mjs:3 and bench.mjs:3 still hard-code /Volumes/..., so the measurements cannot be reproduced.
Summary
Overall risk: Low. There is no behaviour change. The risk is that the design misleads the implementation.
Merge recommendation: Merge as a design record only after the open P2s are fixed in the text or recorded as open questions. The lint blocker looks resolved, pending CI.
Most important to fix:
Specify how the wrapper's own frame is kept out of grouping for stacks the engine builds (new).
Always capture the measured-stackless set, and reconcile the table, cost-and-risk and §4.4 into one always/armed/off policy.
Snapshot the target description when the promise rejects instead of holding the element. Name core describeError.
Add a plan step for origin: 'call' in CrashFrame, toCrashFrame and the schema, with a drift test, before step 4.
Delete the stale §8 zone.js bullet and the enricher .cause walk.
Make the scripts repo-relative.
Positives:
Excluding the research probes from lint is a reasonable way to unblock CI.
Choosing an optional origin: 'call' field over hidden or a marker frame name keeps grouping intact.
· branch async-origin-design
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Review: biome exclude for research probes (BGSVIEWER-14)
Docs + biome.json only. HEAD aad867c0 vs last review (3022f870). Two commits: exclude docs/design/async-origin-capture from biome, then format biome.json. I applied that config and ran biome check on the design doc and both .mjs files.
What this push fixes
The mjs Biome errors no longer fail pnpm lint — that part of the probe P2 is addressed. Excluding throwaway research scripts is a valid CI choice.
The pattern is too broad: biome also ignores docs/design/async-origin-capture.md (the spec). The host-absolute Playwright createRequire is unchanged, so the measured tables still cannot be reproduced from this checkout.
Prior P2s — still open
origin: 'call' is decided but would be dropped by toCrashFrame.
Arm-on-first-failure skips one-shot measured APIs; Body.json / createImageBitmap still have three owners.
byReason still pins target.
Enricher .cause walk still cannot recover axios/ky/ofetch origin.
Alpine / Preact-iso provider dedup is still not a property of the browser providers.
logExceptiondescribeError still unnamed; §8 still says awaited zone chains cannot be located.
Overall risk: Medium
No runtime change. CI lint is unblocked for the probes, at the cost of also dropping the spec from the format/lint gate. The implementation-spec gaps are unchanged.
Merge recommendation: Request changes
Narrow the ignore to the .mjs files (or docs/design/async-origin-capture/**) so the markdown spec stays in biome, and keep the remaining P2s in the doc (or as open questions).
Most important to fix
Ignore docs/design/async-origin-capture/** or *.mjs, not the path prefix that also matches the .md spec.
Carry origin: 'call' through StackFrame → toCrashFrame → CrashFrame; do not set hidden.
Always capture the measured-stackless set; one policy per row; snapshot target on reject.
Drop the enricher .cause walk; name client.tsdescribeError; delete the §8 zone leftover.
Positives
Excluding research probes from the lint gate is the right kind of fix once the scripts are acknowledged as throwaway. Formatting biome.json keeps that file itself from failing the formatter.
Docs + measurement scripts + a lint ignore. No production behaviour change. HEAD aad867c0 vs last reviewed unique design (0af17f19 / protocol decision 3022f870). This push only excludes docs/design/async-origin-capture/ from Biome and formats biome.json.
Checked the protocol decision and remaining claims against packages/core/src/crash.ts (CrashFrame 12–29, toCrashFrame 141–168, buildException 201–206), packages/core/src/stack.ts (StackFrame 19–41), packages/core/src/client.ts (describeError 171–184, logException 1735), packages/browser/src/detection-providers.ts, packages/core/src/dedup.ts, and packages/protocol/upload-contract.schema.json (no crash-frame schema). Ran pnpm lint: exit 0 (the two .mjs files are ignored). Did not re-run probe.mjs / bench.mjs. Did not run unit tests.
What this push fixes
The CI lint gate is green. !docs/design/async-origin-capture matches how !samples / !packages/protocol/spec already work; I confirmed Biome ignores probe.mjs and bench.mjs. That was the merge-blocking half of the probe P2.
The host-absolute Playwright createRequire('/Volumes/External2TB/...') is unchanged, so the measured tables still cannot be re-run from this checkout.
Prior findings
P1 syntheticFrames / patched-then — still fully addressed.
P2 Biome lint / CI red — fully addressed by the ignore.
P2 probe path — partially addressed. Lint gone; scripts still not runnable here.
P2 origin: 'call' copy path — not addressed. The field is decided (right call: not hidden, not a marker name) but toCrashFrame would drop it, and §4.5 / plan step 7 still talk as if the protocol is open.
All other P2s from 0af17f19 — not addressed (arm-on-first-failure, Body.json owners, byReason pins target, .cause walk, Alpine/Preact-iso dedupe, describeError / leftover captureException, §8 zone leftover).
Overall risk: Medium
No runtime change. Implementing the doc as written would fix bare and awaited Chromium play() via byReason + enrichFrames, then drop origin: 'call' on the wire, skip one-shot decode / FontFace.load / createImageBitmap / Body.json, leak <video> nodes, and leave Angular logException descriptions empty.
Merge recommendation: Request changes
The lint ignore is fine. The design is still not an implementation spec until the remaining P2s are fixed in the text or recorded as open questions.
Most important to fix
Carry origin: 'call' through StackFrame → toCrashFrame → CrashFrame; do not set hidden. Point the “schema + drift test” at those TypeScript types (crash.json frames are not in upload-contract.schema.json). Update §4.5 and plan step 7.
Always capture the measured-stackless set; one always / armed / off per row; delete leftover “breaker” / denyHot wording.
Snapshot describer fields on reject; do not pin HTMLMediaElement in byReason.
Drop the enricher-level .cause walk (fetch is not wrapped; buildException already recurses). Tag the browser providers if Alpine / Preact-iso must not double-report.
Name client.tsdescribeError; there is no captureException. Delete the §8 leftover that says awaited zone chains cannot be located.
Repo-relative Playwright require so the probes that justify default-on can actually run.
Positives
Choosing an optional origin: 'call' over hidden or a synthetic marker name is the correct grouping-preserving call.
Ignoring research probes is consistent with !packages/protocol/spec and unblocks CI without touching production lint.
Pristine then + reason-keyed lookup still holds up as the wrapper shape for BGSVIEWER-14.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Review: biome ignore only — design unchanged (BGSVIEWER-14)
Docs + research scripts only. HEAD aad867c0 vs last review (3022f870). Unique delta is biome.json excluding docs/design/async-origin-capture/ (90b9cf7f + format). I re-checked CrashFrame / toCrashFrame (packages/core/src/crash.ts:12-29, 141-168), StackFrame (packages/core/src/stack.ts:19-41), buildException + applyDebugIds (crash.ts:201-206), browser and core describeError, Angular originalError, and checkOrSetAlreadyCaught. pnpm exec biome check docs/design/async-origin-capture now processes 0 files. Did not re-run probe/bench or unit tests. CI on this SHA was still in progress at review time.
What this push fixes
The lint-gate half of the probe/Biome finding. Excluding the research directory matches !samples / !packages/protocol/spec. pnpm lint is no longer blocked by those scripts.
The host-absolute Playwright createRequire is unchanged. The design document is identical to 3022f870.
Remaining spec gaps (still ship-wrong if implemented as written)
origin: 'call' is decided but has no field on StackFrame / CrashFrame, and toCrashFrame is a closed allow-list that would drop it. §4.5 still says “protocol question”.
Arm-on-first-failure still skips the first (often only) rejection of the measured-stackless set.
Body.json / createImageBitmap still have three owners (denyHot / table off / measured on).
byReason still pins target (the <video>) until the app drops the exception.
Enricher-level .cause walk still cannot recover axios/ky/ofetch origin; buildException already recurses.
Alpine / Preact-iso “dedupe on reason identity” is not a property of the browser providers.
Handled-path describeError is still the core one that returns undefined when !stack.
§8 leftover still says awaited zone chains cannot be located (contradicts §4.1).
Zone wrapping (DISABLE_WRAPPING…=false) still delivers a wrapper Error that BugseeErrorHandler does not unwrap.
New: origin frames injected by enrichFrames never get debug_id — applyDebugIds runs on parsed before the enricher.
Overall risk: Medium
No runtime change. The biome ignore is correct. Implementing the rest of the doc as written still ships the same gaps as the last review, plus unsymbolicated origin frames.
Merge recommendation: Request changes
Land the lint exclusion. Name the CrashFrame / toCrashFrame / applyDebugIds / grouping contract next to the origin: 'call' decision, and fix or explicitly open-question the remaining P2s before this is the implementation spec.
Most important to fix
Carry origin: 'call' through StackFrame → toCrashFrame → CrashFrame; do not set hidden. Run applyDebugIds on frames after the enricher. Update §4.5 (it still says the protocol is open).
Always capture the measured-stackless set; one always / armed / off per row.
Snapshot describer fields on reject; drop target from byReason.
Drop the enricher .cause walk; tag browser providers if Alpine must not double-report; unwrap zone .rejection on the Angular path.
Name client.tsdescribeError; delete the §8 zone leftover.
Repo-relative Playwright (lint is done).
Positives
Excluding the research probes from Biome is the right way to unblock CI. origin: 'call' (not hidden, not a marker name) is still the correct grouping-preserving choice.
…ements, as-built notes, probe paths)
The same doc and probe fixes as on the implementation branch (PR #176), so this PR's review findings are answered by the file it carries.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
This push rewrites the doc into its "as built" state and adds §11-§14. I read the doc and both probes and checked the new references against origin/main and packages/core/src/crash.ts. I did not run the probes, the bench, lint or tests.
probe.mjs and bench.mjs resolve Playwright relative to the repo. PW_<engine> is read from the environment.
biome.json now ignores docs/design/async-origin-capture/**, so the .md spec is linted again.
Measured-stackless rows no longer conflict: Body.json is explicitly not wrapped, and createImageBitmap is armed. The old denyHot and breaker text is gone.
The stale zone.js "cannot be located" bullet is replaced (§8).
target retention is documented as a known limitation, as is the first-origin-wins rule (§4.2, §4.5, §11).
debug_id for enriched frames (§13) and origin: 'call' plumbing (§4.5) are now stated.
The .cause walk is now described honestly: it only helps ky and ofetch (§4.5, §10.4).
Alpine and Preact-iso double reports are moved to §10.5 as a gap that exists independent of this work.
Both describeErrors are named in §4.5.
Still open (small, doc-only)
P3captureException is still named at :266 and :444. It does not exist in packages/*/src; the handled-path API is logException.
P3reportReactError is cited under packages/react/src/error-boundary.ts at :445. The file layout has report.ts alongside it, so check the citation.
P3 §4.4 has two overlapping gating bullets ("AS BUILT" and "Every other row: armed…"). Merge them into one.
Summary
Overall risk: Low. There is no behaviour change, and the earlier P2 findings were largely addressed.
Drop captureException and fix the reportReactError path.
Merge the two §4.4 bullets.
Positives:
The doc now records what real engines taught: the core-js toString disguise, header-only stacks, and the wrapper frame on top of argument-validation stacks.
Accepted limitations are explicit, and the grouping and signature story (§13) is stated.
docs(design): bring the design doc to its corrected state answers the P2s from the last pass. This is still a docs-only PR; the as-built claims are the spec for #176.
Addressed
Probe/bench Playwright path is repo-relative (import.meta.url).
origin: 'call' is specified on StackFrame / CrashFrame / toCrashFrame; grouping is the first non-hidden user frame; enricher frames get debug_id in buildCrashJson (§4.5, §13).
Arm-on-first-failure is an accepted first-report gap; Body.json is deliberately off; createImageBitmap is armed; no separate denyHot.
Retention of target for the reason's lifetime is stated (§4.2, §11).
.cause walk is scoped to ky/ofetch; Alpine/Preact-iso double-report is called out as independent.
Both describeErrors are named; the leftover zone.js open question is marked RESOLVED.
Residual (P3, docs consistency after a “corrected state” pass)
§10.4 Astro row is still GAP; §10.5 says FIXED.
§10.6 still says “WebKit untested”; §7/§8/§12 say it was measured in e2e.
Zone wrapping (DISABLE_WRAPPING…=false) still lists only “pristine then”. The event reason is zone's {rejection, …} object, so a by-reason lookup of the DOMException misses unless .rejection is unwrapped. Default zone does not wrap; this is the opt-in row.
Also stale, not re-filed: §4.5 “passes neither today”; factory row { hot? } / capture: false; captureException in §10.3.
Risk: Low. Merge: approve with the three leftover contradictions cleaned up, or land as-is if #176 remains the source of truth for as-built notes.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review of PR #175 at 4e90b8b (prior aad867c). Unique +739/−1 vs main. Still design + probes only; behaviour lives on PR #176.
This push vs last review: the design is rewritten as the as-built record of #176 (§11–§14), and the probe/bench createRequire paths plus the 5 s timeout label are fixed.
Prior P2s, all addressed in this file:
origin: 'call' is on StackFrame / CrashFrame / toCrashFrame; enricher frames get debug_id (§4.5, §13). Matches #176crash.ts.
Arming is recorder.arm() from the enricher on any report path; first armed rejection has no call site (accepted); rare rows are always (§4.4).
Body.json is deliberately off; createImageBitmap is armed; no denyHot list.
Retention is keyed by the reason, not settle; target pin documented and accepted (§4.2, §11).
.cause walk scoped to ky/ofetch; axios XHR / Mithril are gaps (§4.5, §10.4).
Alpine / Preact-iso double events called out of scope (§10.5).
Both describeErrors and describeTarget named; adapters go through logException (§4.5, §7.5).
Zone leftover resolved by reason identity (§8). Playwright paths are repo-relative; noEventWithin5s replaces noUnhandledRejection.
No remaining P0–P2. Two P3 leftovers from the "corrected state" rewrite (inline).
Residual (not re-filed): Firefox play() Target line is still read at report time, so an unhandledrejection report can see error = null (Firefox already has a call site, §2). Zone wrapping-on (DISABLE_WRAPPING…=false) still does not walk .rejection. §4.5 still says the browser provider "passes neither today." Nuxt path still cites entry.js:43 (packages/nuxt/src/client.ts:49 is installBugseeErrorHandler).
The leftover-contradiction pass lands. Status now says as-built paths exist after #176 and that #176 should merge first. §10.4 Astro is FIXED (matches §10.5). §2 and §10.6 no longer say WebKit is untested. §4.5 no longer says the browser provider “passes neither today.”
Still open (P3): the zone.js DISABLE_WRAPPING_UNCAUGHT_PROMISE_REJECTION=false row still lists only “pristine then.” In that opt-in mode zone replaces event.reason with {rejection, promise, zone, task}, so originOf(event.reason) misses the DOMException unless .rejection is unwrapped. Default zone does not wrap. Same fix as last pass: unwrap, or mark the row as an accepted gap like Redux.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review of PR #175 at 251aa01 (prior 4e90b8b). Docs-only; +10/−8 in the design doc. Land #176 first, then this — the status line now says so, and the PR description matches.
This push vs last review: the two P3 leftovers are gone. Astro’s survey row is FIXED (matches §10.5). §2/§10.6 no longer call WebKit untested. §4.5 no longer says the browser provider “passes neither today.” Claude’s dead-reference P3 is addressed by the #176-first status line. Astro’s launch-window queue is closed in registerClient’s finally.
No remaining P0–P2.
Residual (not re-filed): zone wrapping-on (DISABLE_WRAPPING_UNCAUGHT_PROMISE_REJECTION=false) still lists only “pristine then.” The event reason is zone’s {rejection, …} object, so originOf(event.reason) misses unless .rejection is unwrapped. Default zone does not wrap.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review of 251aa01 (docs only; unique +741/−1 vs bf620ab3). Two commits since last look (aad867c0) bring the design to the as-built state of #176.
Prior findings:origin: 'call' on StackFrame/CrashFrame/toCrashFrame, applyDebugIds after enrichFrames, Body.json off, target retention, .cause walk limits, Alpine/Preact-iso not-deduped, both describeErrors, zone leftover, Angular default wrapping, probe createRequire(import.meta.url), biome /** — fully_addressed. Arm-on-first-failure cost is accepted and documented; the frequency-table Default column is the leftover (partially_addressed).
Checked against surrounding code on this branch (crash.ts still stamps applyDebugIds before enrichFrames; browser crashOf still has no enricher) and against origin/async-origin-impl (packages/capture/src/async-origin-table.ts, async-origin.tsarm() installs every armed row, WebKit play() e2e requires origin: 'call' on every frame). Did not run tests.
Findings
P2 — Hot-class Default column still says off; as-built and the rest of this doc say armed
docs/design/async-origin-capture.md:318 vs :237 / :323 and #176ASYNC_ORIGIN_TABLE.
P2 — WebKit play() is not “like Firefox”
:51 vs §2 Firefox play (has call site) and #176 WebKit e2e (frames.every(origin === 'call')).
Overall
Risk: Low (this PR has no production change; leftover contradictions would mis-map #176)
Positive: Status line now names the #176 landing order; Body.json gap, WeakMap retention, enricher arming, Astro launch-window, and the accepted first-failure hole are stated honestly
The two P2 leftovers from 251aa01 are gone, and so is the wrapping-row P3.
WebKit play() is stated as stackless (Chromium-like); the text now warns not to read it as Firefox’s play() row.
Hot class Default is armed, matching §4.4 and the as-built table. Factory row is { iface, methods, mode, targets?, why }; the 36 B token and capture: false / sampling are gone.
Zone DISABLE_WRAPPING…=false is an accepted gap: event.reason is {rejection, …}, by-reason lookup misses.
No remaining P0–P3. Residual: the Rare column still says on while the table’s mode is always (same policy, different word).
Risk: Low. Merge: approve. Land #176 first, then this.
Docs-only. Unique vs last look (251aa01a) is +7/−8 in docs/design/async-origin-capture.md. Behaviour still lives on PR #176 (open; land that first). I re-read the full design, the probes, and the as-built table/wrapper/enricher/e2e on async-origin-impl — not the 15-line delta in isolation.
This push vs last review: the two P2s close.
Hot-class Default is now armed (§4.4), including createImageBitmap. That matches #176ASYNC_ORIGIN_TABLE: Scheduler.postTask/yield, FileSystemWritableFileStream.*, GPUBuffer.mapAsync, GPUQueue.onSubmittedWorkDone, GPUDevice.popErrorScope, ImageCapture.grabFrame, ImageDecoder.decode, VideoFrame.copyTo, Video/AudioDecoder/Encoder.flush, OffscreenCanvas.convertToBlob, createImageBitmap are all mode: 'armed'. Row shape is { iface, methods, mode, targets?, why }. The 36 B token and capture: false / sampling leftovers are gone.
§2 no longer says WebKit “behaves like Firefox”. play() is stackless there (Chromium-like: #176 WebKit e2e asserts every crash.json frame origin: 'call', which the enricher only injects when the engine stack parsed to nothing). Tail-call elimination stays in §12.
Zone __zone_symbol__DISABLE_WRAPPING_UNCAUGHT_PROMISE_REJECTION=false now records the accepted miss: event.reason is {rejection, promise, zone, task}; originOf walks .cause only.
No unique P0–P3. I looked for leftover as-built contradictions (factory options, denyHot, Body.json policy, arm-all-at-once, enrichFrames vs syntheticFrames, Astro FIXED) and did not find a new one that would ship the wrong wrap set or drop Safari play().
Residual (not filed):wrapPromiseMethod is still the design name; as-built is installAsyncOrigins + inner wrap(). Bursty Default still says “armed for the measured-stackless ones” while the cited table arms the whole bursty set. Both are naming/summary leftovers, not a wrong policy.
Did not run tests (docs + research scripts only).
Overall risk: Low
Merge recommendation: approve — land #176 first, then this.
Most important issues to fix
None on this PR.
Positives
The class table, factory row, and WebKit paragraph now match the code and the e2e instead of the v1 leftovers. That is the job of an as-built design record.
…#176)
Async origin capture: wrap promise-returning platform APIs so a stackless async rejection (HTMLMediaElement.play() NotSupportedError, BGSVIEWER-14) is reported with the call site as its primary frames (origin: 'call'), the target element description, and a grouping signature. Browser SDK only; verified in real Chromium, Firefox and WebKit, with zone.js, Bluebird, core-js, React and Vue.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
#176 merged into this branch (baseRefName: async-origin-design). The squash 1e6c9f5a has the same tree as last-reviewed #176 HEAD 4446468a (empty git diff). No new production delta on this push.
This PR is now the vehicle that lands the as-built design and the implementation onto main. Prior reviews of both sides stand:
Design (650f234): hot class armed, WebKit play() stackless, zone wrapping accepted.
No remaining P0–P3 unique to this PR. Residuals that already shipped with #176 (not re-filed): registerClient({ document: A }) then relaunch({ window }) can drop document; core describeError still instanceof Error; AsyncOrigin.skip JSDoc vs runtime.
The reason will be displayed to describe this comment to others. Learn more.
Review (1e6c9f5) — implementation now lives on this PR
HEAD squash-merges #176 onto the design branch. Unique vs last look (650f234f) is +6334/−134 across 53 files: this is no longer docs-only. The PR body still says “no behaviour change; land #176 first” — that is stale. Merging this now ships default-on prototype wrapping for the measured stackless set.
Checked against the as-built capture path (installAsyncOrigins, createAsyncOriginReporting, browser crashOf/logException, buildCrashJsonenrichFrames), the table, Astro hydration, and prior #176 residuals. Did not run tests or CI this pass.
Prior docs P2s (WebKit play(), hot-class armed, factory row shape, zone .rejection) remain fully_addressed in the design doc.
P2 — wraps are installed before a published client exists, and a retry cannot rebind them
Location:packages/browser/src/launch.ts:1031 (no try/finally); packages/capture/src/async-origin.ts:310 (MARK skip)
Problem:installAsyncOrigins patches HTMLMediaElement.play (and the other always rows) as soon as buildLaunch reaches it, long before setCarrierClient. Wrappers are marked with Symbol.for('bugsee.asyncOrigin'). stop() — the only restorer — is registered later as a detection provider (launch.ts:1405) and on the public client (:1662). If anything throws in that window, or a second launch() races the first, stop() never runs. A later launch() sees MARK and skips wrapping (installed: []). Live play() still hits the orphaned wrapper/recorder; detection/logException use a new recorder and get no origin.
The window is large: wrap is at 1031, publish at 1705. A concrete throw site in between is the unguarded manifest loop (:1119-1120).
Impact: BGSVIEWER-14 stays fixed only until the first failed or overlapping launch. After that, stackless play() reports frames: [] again, until a full reload. Silent — no onError.
Scenario:
launch() installs wraps.
A contributed service manifest throws (or any throw after 1031), or two launch() calls overlap across await resolveBrowserDeviceId (:750) so both pass the singleton check.
play() is still wrapped by the dead recorder; the published client’s enricher cannot find the origin.
Fix: Tie wrapper lifetime to a successfully published client: try/finally { asyncOrigins?.stop() } on any path that does not hand off the client (including launchCore's catch when the carrier slot is still empty). Optionally have wrap() treat a MARKed method as rebindable to the new recorder instead of skip-only. Add a test: throw after install, assert play is restored and a second launch re-wraps and reports origin: 'call'.
Residuals (not re-filed)
logException of a non-Error always supplies syntheticFrames (client.ts:1763-1776); the real enricher then treats those as “already has frames” and never looks up origin. Same-realm DOMException is an Error, so unhandledrejection / adapter logException of play() still work. Cross-realm would also not be wrapped (iframe has its own prototype).
Astro lastHydration is written before start() (client.ts:67): a failed relaunch({ document: B }) then a plain relaunch() listens on B while core lastLaunch stays A. Losses only in the relaunch window.
registerClient({ document: A }) then relaunch({ window: W }) can split hydration target vs launch document (sibling P3 on #176).
Armed-row first failure has no call site (documented accepted price).
AsyncOrigin.skip JSDoc still says 1; Firefox/Safari path uses 2.
Overall risk: Medium — default-on prototype wrapping is the right design for BGSVIEWER-14; the lifecycle hole can silently undo it after a failed/racy launch.
Merge recommendation: Request changes
Most important to fix: Restore (or rebind) async-origin wraps whenever launch does not publish a client.
Positives:enrichFrames is the correct seam for a real Error; one definition of “has frames” (parseStack.length > 0); origin: 'call' + debug_id re-stamp; arming from the enricher covers React/Vue/logException; pristine then / zone.js path; Chromium/Firefox/WebKit e2e for play(); stop() / KILL_SDK restore when launch actually succeeds.
The reason will be displayed to describe this comment to others. Learn more.
P2 — wraps installed here are not restored if launch fails (or races) before the client is published.
installAsyncOrigins patches HTMLMediaElement.prototype.play (and the other always rows) immediately. Wrappers are marked Symbol.for('bugsee.asyncOrigin') (async-origin.ts:310). stop() is the only restorer, and it is not hooked until the detection provider (:1405) and the public client (:1662). setCarrierClient is at :1705.
If anything throws in that window — the unguarded getServiceManifests loop at :1119 is a concrete site — or a second launch() overlaps across await resolveBrowserDeviceId (:750) so both pass the singleton check, stop() never runs. A retry sees MARK and skips wrapping (installed: []). Live play() still hits the orphaned wrapper; the new recorder has no origin. BGSVIEWER-14 reports frames: [] again until a full reload, with no onError.
Wrap this in try/finally { asyncOrigins?.stop() } on any path that does not publish the client, and add a test: throw after install, assert the method is restored and a second launch re-wraps.
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
javascriptPull requests that update javascript code
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Design doc, measurements and probes only: no behaviour change in this PR. The doc has since been corrected against the review findings and brought to its as-built state (it records what #176 built and what real engines taught), so it cites files that exist once #176 has merged. Land #176 first, then this.
BGSVIEWER-14 is an unhandled
NotSupportedError: The element has no supported sources.with zero frames. Measured in headless Chromium 156 and Firefox 151 (docs/design/async-origin-capture/probe.mjs): platform promises that fail asynchronously reject with no stack in Chromium (and fewer in Firefox); user-codeErrorrejections already carry stacks, so the globalPromiseneeds no patching. The doc lists the measured stackless set, the 187 candidate methods fromlib.dom.d.ts, a generic call-site wrapper (with theunhandledrejection-preserving derived-promise trap), costs/risks, and a plan.WebKit was not part of the original measurements; it was run later in the e2e (design doc §12).
🤖 Generated with Claude Code