|
| 1 | +# Review round 2 — JS/mobile API alignment (fixes of round 1, `83327ee..e2f928c`) |
| 2 | + |
| 3 | +Four fresh read-only Opus reviews (2026-10-12): lifecycle/relaunch/carrier (**L2**), blackout + identifier privacy |
| 4 | +(**V2**), payloads/notify/options (**P2**), tests/docs/public API (**D2**). Findings below are NEW (regressions of the |
| 5 | +round-1 fixes, misses, bad interactions between fix groups). IDs: `R2L-n`, `R2V-n`, `R2P-n`, `R2D-n`. |
| 6 | +Round 1: `docs/review/mobile-alignment-round-1.md`. |
| 7 | + |
| 8 | +Product decisions taken with this round: |
| 9 | +- `relaunch()` keeps ALL integration seams AND the deployment identity (endpoint, appId/appVersion/appBuild, |
| 10 | + platformType, persist/recover, runtime scope handles) but resets Bugsee behaviour options (R2P-3). |
| 11 | +- Privacy options whose default is ON are **fail-closed on invalid values** (a value that is not boolean `true` turns |
| 12 | + them OFF, with an `onError` report), e.g. `captureNetworkBodies` (R2P-4). Options whose default is OFF only turn on |
| 13 | + for boolean `true`. |
| 14 | +- Android DOES persist the user identifier (corrects a wrong claim in PROGRESS/T9 notes); JS stays opt-in (PII). |
| 15 | +- Mobile APIs still without a JS equivalent (`deleteCollectedDataOnDevice`, `addNetworkEvent`, `logUnhandledException`, |
| 16 | + `testCrash`, on-demand `captureViewHierarchy()`, runtime secure-view/rect APIs, `MaxPending*` options) are recorded |
| 17 | + in the plan as follow-ups and are NOT part of this round. |
| 18 | + |
| 19 | +Groups (each fixed test-first by its own agent): **F1** lifecycle/stop safety, **F2** blackout/privacy, **F3** |
| 20 | +payloads/options, **F4** exports/docs/tests. |
| 21 | + |
| 22 | +## Findings and dispositions |
| 23 | + |
| 24 | +### R2L — lifecycle, relaunch, carrier (F1 unless noted) |
| 25 | +- **R2L-1 major, fix.** A second `stop()` while the first still drains (or after a timed-out one) returns "drained" |
| 26 | + at once (`client.ts` `!launched` path); node then writes `.stopped` and browser/webworker release the lock while the |
| 27 | + first stop's upload is still running (duplicate upload / live subtree claimed). Keep the in-flight drain promise in |
| 28 | + core and return it from a repeat `stop()`; platforms treat "core Stopping" as not drained; keep the killed-client |
| 29 | + release path separate. Reproduced by the reviewer (node, hung transport). |
| 30 | +- **R2L-2 major (regression of L8), fix.** `ReportHandler.after` runs only inside `assembleReport`, which is not built |
| 31 | + when a `triggerPipeline` is injected (Electron renderer, WebView): the app's `after` never runs there. Call it before |
| 32 | + `triggerPipeline.report(handled)` too (it is synchronous); tests for Electron renderer and WebView. |
| 33 | +- **R2L-3 major, fix.** `client.launch()` after `stop()` restarts only core: node heartbeat, exit/signal handlers, |
| 34 | + server instrumentation and carrier slot stay torn down (double clients, live subtree declared dead after 120 s); |
| 35 | + browser pagehide flush and webworker carrier not restored. Make the platform `launch()` re-arm what `stop()` tore |
| 36 | + down (or refuse `launch()` on a stopped platform client — choose the smaller safe design and test it). |
| 37 | +- **R2L-4 minor+major(D2-1), fix.** L1/V2 only reached node/browser: webworker, vercel-edge (also cloudflare, workerd, |
| 38 | + edge-light) and webview call `createClient` without `carrier` and without the `lifecycleEventsListener` option, so |
| 39 | + Launching/Launched are unobservable there, a relaunch loses the listener and ends a blackout. Declare + forward the |
| 40 | + option, pass `carrier: carrier ?? globalThis`; extend `export-parity.test-d.ts`. |
| 41 | +- **R2L-5 minor, fix.** The event queue + `delivering` flag are per emitter but the listener slot is shared on the |
| 42 | + carrier: a listener calling `launch()`/`relaunch()` from client A's Stopped gets B's Launching/Launched nested. |
| 43 | + Keep queue/flag on the carrier next to the slot. |
| 44 | +- **R2L-6 minor, fix.** `relaunch()` builds a new client; the old client's queued (offline) notifications are never |
| 45 | + resumed or handed over. Hand the queue over (or flush it before stop within the stop timeout) and test. |
| 46 | +- **R2L-7 minor, fix (shared with R2V-6).** A carried blackout cannot be ended while stopped (`endBlackout()` is |
| 47 | + refused, the carrier flag stays true): the next launch starts dark without a BlackoutStarted event. Allow `end()` to |
| 48 | + clear the carrier state while stopped (no events), keep fail-closed on `start`. |
| 49 | +- **R2L-8 minor, fix.** `relaunch()` gives up waiting for an in-flight launch after 5 s: with no `lastLaunch` it rejects |
| 50 | + "before launch()" although a launch is in flight, or races two builds. Wait for the pending launch without the bound |
| 51 | + (the bound applies to the OLD client's drain only). |
| 52 | +- **R2L-9 minor, fix.** A stale WebView `stop()` clears the carrier slot and `launchedClient` unconditionally; use an |
| 53 | + ownership-checked clear. |
| 54 | +- **R2L-10 minor, fix.** Webworker `stop()` closes IndexedDB without flushing the capture store (browser flushes first). |
| 55 | +- **R2L-11 nit, fix.** `RELAUNCH_KEPT_OPTIONS` (24 keys) pins transport, stores, window/document, indexedDB etc. on |
| 56 | + `globalThis` after the client is gone; accept (decision above) but document it, and add the platform-level |
| 57 | + "no options means defaults" tests back (node/browser L10 tests were changed to `relaunch(opts())`). |
| 58 | +- **R2L-12 nit, fix.** The kept `carrier` seam does nothing for a plain `relaunch()` after `launch(t, {carrier: X})` |
| 59 | + (looks on `globalThis`); look it up from the remembered options. |
| 60 | +- **R2L-13 nit, fix.** The kill path does not bump `generation`, contradicting the comment in `client.ts`. |
| 61 | +- **R2L-14 nit, fix.** Node `launch()` on a killed client still clears the stop marker though core launch is a no-op. |
| 62 | +- **R2L-15 nit, fix.** Two listeners (one stops on Launched, one launches on Stopping) can loop forever |
| 63 | + synchronously: cap the re-entrant rounds like blackout (fail closed: stop delivering, `onError`). |
| 64 | +- **R2D-2 major, fix (F1).** `RelaunchedAfterCrash` is exported but never emitted. Emit it when launch recovery finds |
| 65 | + evidence that the previous run did not stop cleanly (node: a recovered dead-sibling subtree / crash marker; browser: |
| 66 | + recovered incident marker) or, if no honest signal exists on a platform, document it as not emitted there. No fake |
| 67 | + emits. |
| 68 | + |
| 69 | +### R2V — blackout and identifier privacy (F2) |
| 70 | +- **R2V-1 major, fix.** Electron: `launchRenderer` returns before the handshake reply; an app that calls |
| 71 | + `startBlackout()` right after `await launchRenderer()` is a no-op (still in the launch hold) and the reply's |
| 72 | + `bo:false` then ENDS the blackout (also overrides an explicit `startInBlackout: true`). Keep three separate flags: |
| 73 | + launch hold (released by the handshake only), main's state (`blackout-start/end`), app-local state; dark if any is |
| 74 | + set. Test the exact sequence. |
| 75 | +- **R2V-2 major, fix.** WebView: `startBlackout()` does not stop the native host's video (no blackout message in |
| 76 | + `protocol.ts`). Add a bridge message kind the native receivers can apply, warn through `onError` when native does |
| 77 | + not acknowledge, and list the limit in `docs/design/blackout.md` until native support lands (the Android/iOS |
| 78 | + receivers live in other repos: file the follow-up). |
| 79 | +- **R2V-3 minor, fix.** Carrier-shared input sources stay bound to the FIRST client's blackout gate after |
| 80 | + `relaunch()` (wheel flush after `endBlackout()` leaks again; a relaunch during a blackout never captures wheel |
| 81 | + again). Late-bind the gate to the current carrier client (like the viewtree's `clientRef`). Reproduced |
| 82 | + (`r2v-wheel.mts`). |
| 83 | +- **R2V-4 minor (fail-open), fix.** The 100-round redelivery cap can leave a subscriber (replay!) in the wrong state; |
| 84 | + the client's own lifecycle listener is subscribed first. At the cap force the state to ACTIVE (fail closed), |
| 85 | + ignore re-entrant `set()` during delivery, deliver once to every out-of-date listener incl. the carrier writer. |
| 86 | + Reproduced (`r2v-cap.ts`). |
| 87 | +- **R2V-5 minor, fix.** `setUserIdentifier(non-string)` is a clear only in the browser wrapper; node/bun/deno/worker/ |
| 88 | + edge/webview/Electron main use the core setter, which stores the raw value and ships it as `email` in request.json |
| 89 | + and notify. Normalise in core `Environment` (non-string -> clear; `getUserIdentifier()` returns `null`). |
| 90 | +- **R2V-6 minor, fix.** See R2L-7. |
| 91 | +- **R2V-7 minor, fix.** Each `endBlackout()` restart with canvas replay leaks rrweb internals (a Worker per cycle, |
| 92 | + rAF loops); and the comment at `recorder.ts:87-89` overclaims. Pause canvas sampling instead of a full restart if |
| 93 | + the fork allows it, else terminate on stop via the fork API / cap restarts; correct the comment. |
| 94 | +- **R2V-8 minor, fix.** `createElectronMainControl` without `isBlackout` leaves renderers dark forever; send `bo:false` |
| 95 | + when no probe is given (or make it required in the type). |
| 96 | +- **R2V-9 minor, fix.** Electron pixel video can start after `stop()` (pending `hasPermission()`); stop generation |
| 97 | + checked after the await. |
| 98 | +- **R2V-10 nit, fix.** `docs/design/blackout.md` inaccuracies ("input presses/keys dropped at source" - source gates |
| 99 | + wheel only) and missing statements (carry across relaunch/stop, start/end ignored while not launched, the handshake |
| 100 | + rule, WebView native video). |
| 101 | +- **R2V-11 nit, fix.** `startInBlackout` emits BlackoutStarted before Launching and Electron renderers emit a spurious |
| 102 | + Started/Ended pair; suppress events for the launch hold. |
| 103 | +- **R2V-12 nit, fix.** Orphan `bugsee.blackout {active:false}` marker with `startInBlackout`/pre-chunk blackout; emit |
| 104 | + `true` first or none (document the viewer follow-up). |
| 105 | +- **R2V-13 nit, fix (docs).** Incident markers in IndexedDB/disk keep `userIdentifier` until upload and |
| 106 | + `clearUserIdentifier()` does not scrub them: document in the option JSDoc and PROGRESS ("nothing is written" is |
| 107 | + about the identifier store only); scrub pending markers' `userIdentifier` on clear if cheap. |
| 108 | + |
| 109 | +### R2P — payloads, options, capture (F3) |
| 110 | +- **R2P-1 major (regression of O12), fix.** The public `logException` wrapper becomes the top frame for non-Error |
| 111 | + throwables (`callSiteFrames` trims only at the internal named `logException`): every `logException('str')` groups |
| 112 | + under `Object.logException`; `skipFrames` skips the wrong frame. Pass a boundary the wrapper supplies; tighten the |
| 113 | + test to "first frame is `applicationCode`". Reproduced. |
| 114 | +- **R2P-2 major, fix.** `retryAfter` has no floor: `Retry-After: 0` or body `retryAfter: 0.0001` => 2283 requests in |
| 115 | + 3.2 s; the appserver itself answers 429 `retryAfter:1` for an oversize batch (`notify_max_batch_size`) forever. Use |
| 116 | + `max(hint, RETRY_INITIAL_MS)`, keep exponential backoff, halve the batch on repeated 429. Reproduced. |
| 117 | +- **R2P-3 major, fix.** `RELAUNCH_KEPT_OPTIONS` drops `endpoint`, `appId/appVersion/appBuild/platformType`, |
| 118 | + `persist/recover`, `asyncLocalStorage`, `partitionCaptureByTenant`, `globalScope/globalTarget/logger` -> a relaunch |
| 119 | + uploads to the default production endpoint (data-residency) with wrong app version. Keep them (decision above). |
| 120 | +- **R2P-4 major (privacy fail-open), fix.** Validation turned invalid values of `captureNetworkBodies` (default ON) |
| 121 | + such as `0 | null | ''` from OFF into ON. Fail closed: for ON-by-default privacy options anything that is not boolean |
| 122 | + `true` is OFF (+ `onError`); apply to every privacy-relevant option and add a table test. |
| 123 | +- **R2P-5 minor, fix.** Browser/node options read by truthiness (`persistUserIdentifier: 'false'` persists PII; |
| 124 | + `startInBlackout`, `captureViewHierarchy`, `captureInteractions`, report-UI flags, node `profiling`, `detectHangs`): |
| 125 | + add `...boolean` normalisers; invalid `notifyFlushDelay` must report; the dialog error message lists `verylow`. |
| 126 | + (Also R2D-11.) |
| 127 | +- **R2P-6 minor, fix.** A throwing user `onError` still loses the report (assembly passes the raw `onError`); wrap in |
| 128 | + the catch / pass `onErrorSafe`. Reproduced. |
| 129 | +- **R2P-7 minor, fix.** What `ReportHandler.after` adds is not in the crash-recovery marker (written at submit): |
| 130 | + recovered report differs from the live one. Document AND, since `after` is synchronous, run it before the marker is |
| 131 | + written for the handler-built request path where possible; keep assembly-time limits. |
| 132 | +- **R2P-8 minor, fix.** URL templating misses short ids / PII: nanoid (21), Firebase push ids (20), `ord_12345`, |
| 133 | + `42.json`, `+15551234567`, IPs, dates; over-templates long slugs containing a digit. Add rules (parity with |
| 134 | + Android where it exists); no ReDoS (verified up to 1e6 chars). Also uppercase `LONG_HEX` (mutation survivor m13) |
| 135 | + and uppercase `method` in the dedup key. |
| 136 | +- **R2P-9 nit, fix.** `decodeRequestFromMarker` with non-string `dataBase64` decodes garbage (check `typeof`); |
| 137 | + frozen report => `normalizeReportAttachments` throws and the marker then base64-encodes UNLIMITED attachments |
| 138 | + (re-opens P1: apply limits before encoding in the catch path); 3x3 MB base64+JSON is ~279 ms synchronous: chunk or |
| 139 | + document; `asInternalClient(browserPublicClient)` loses `reportDefaultBugPriority` (document/fix with R2D-3); |
| 140 | + `getDefaultNetworkSanitizer` missing from electron/vercel-edge/webworker/cloudflare/nuxt-server entries (R2D-4). |
| 141 | +- **R2D-3 major, fix (F3).** `reportDefaultBugPriority` only affects the browser dialog; `client.upload()`, node, edge, |
| 142 | + worker ignore it (Android applies it to every bug report). Move it to `COMMON_OPTION_DEFINITIONS`, apply it in |
| 143 | + core `applyReportBefore` for derived bug severities. |
| 144 | + |
| 145 | +### R2D — exports, docs, tests (F4 unless noted) |
| 146 | +- **R2D-1 major, fix** = R2L-4 (option on worker/edge/webview) — owned by F1. |
| 147 | +- **R2D-2** = owned by F1 (above). |
| 148 | +- **R2D-3** = owned by F3 (above). |
| 149 | +- **R2D-4 major, fix.** Directly imported packages lack the new value exports (`BugseeStatus`, `LifecycleEvents`, |
| 150 | + `getStatus`, `relaunch`, `addReportAttachment`, `getDefaultNetworkSanitizer`): vercel-edge, cloudflare, webworker, |
| 151 | + webview, electron (main/renderer), nextjs/remix/sveltekit/astro `/server` + `/edge`, astro/nuxt `/client`. Re-export |
| 152 | + from each (thin, same functions) and extend the parity tests to those packages. Check umbrella `relaunch` vs |
| 153 | + Next's `registerServer` defaults (document if relaunch re-runs the plain umbrella launch). |
| 154 | +- **R2D-5 major, fix (docs).** `maxDataSize` (`config.data-size`, rolling capture buffer) is a different option from |
| 155 | + Android/iOS `config.max-data-size` (disk cap on pending reports, default 150/50): add to Kept differences. |
| 156 | +- **R2D-6..12 minor/nit, fix (F4, docs):** http-error priority sentence (plan :36 contradicts :33); attachments row |
| 157 | + ("Android has no cap" is wrong: 1000; field is `userAttachments`); the "plain JS cannot forge code_dialog" |
| 158 | + overclaim and the stale `asInternalClient` JSDoc ("type-level cast only"); PROGRESS blackout entry lists relaunch |
| 159 | + carry as open (done) and says Android does not persist the identifier (wrong); plan header still "in progress"; |
| 160 | + notify kept-differences omits WebView; `detectHttpErrors` difference (4) is not a difference (Android also uses one |
| 161 | + synthetic frame); document that `relaunch()` after `stop()` is a no-op on Android, that `startInBlackout` / |
| 162 | + `persistUserIdentifier` / `lifecycleEventsListener` are JS-only keys, that `startBlackout` is a no-op before launch; |
| 163 | + `launch-status.ts` cites the wrong Android type; `reportDefaultErrorPriority` does not apply to http errors. |
| 164 | +- **R2D-13 minor, fix (F1/F4).** `emitLifecycleEvent` is on the public `BugseeClient`: app code can fake events; move |
| 165 | + to `InternalBugseeClient`. |
| 166 | +- **R2D-14 tests, fix (F4).** Add/strengthen tests for the surviving mutants: blackout redelivery cap (m2), uppercase |
| 167 | + hex (m13), `startInBlackout` ordering before providers and event order (m18), `unref()` of the relaunch timer (m21), |
| 168 | + notify queue-bytes boundary (m24), recorder marker order before the snapshot (m29). Fix weak/mislabelled tests: |
| 169 | + `export-parity.test.ts` lacks `addReportAttachment`; `export-parity.test-d.ts` lacks `LifecycleEventsListener`, |
| 170 | + `LifecycleEventType`, `ReportAttachmentInput`; `notify.launch.test.ts:143` title vs body, `:137` does not assert the |
| 171 | + wire; `bugsee/src/node.test.ts` repeat-launch asserts only `typeof`; wall-clock asserts in browser lifecycle tests; |
| 172 | + e2e hard-codes `'bugsee.user_identifier.'` instead of importing the prefix; `startInBlackout` e2e negatives can pass |
| 173 | + vacuously (add the "lit" controls for input/ui breadcrumbs and assert the markers); replace |
| 174 | + `waitForTimeout(600)` in `blackout.e2e.ts` with a condition wait. |
| 175 | +- **R2D-15, follow-up (not this round).** Mobile APIs/options with no JS equivalent (list above): recorded in the plan |
| 176 | + doc under "Remaining mobile gaps"; `deleteCollectedDataOnDevice` is the one to decide on next (privacy). |
0 commit comments