Skip to content

T4: blackout (startBlackout / endBlackout / isBlackout) - #40

Merged
krassx merged 2 commits into
review/base-1from
review/t4
Oct 12, 2026
Merged

krassx merged 2 commits into
review/base-1from
review/t4

Conversation

@krassx

@krassx krassx commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Review-only PR. This change is already on main (the repo works directly on main; see CLAUDE.md). The base branch is the commit it forked from, so the diff below is exactly this task. Merging this PR changes nothing. Review findings become follow-up commits on main. Plan: docs/design/mobile-api-alignment.md.

Mobile parity: Bugsee.startBlackout/endBlackout/isBlackout.

While active: replay recorder stopped (resumes with a fresh full snapshot), input presses, ui.* breadcrumbs, interaction performance transactions and the report-time viewtree are suppressed. Logs/network/events/traces continue. Idempotent, not counted. Real-Chromium e2e proves nothing from the blackout window reaches the replay.

Review focus (privacy)

  • DOM state that persists past endBlackout appears in the fresh snapshot (same as mobile video).
  • A press that began before and ended during the blackout leaves an unmatched begin.
  • video.aux viewport geometry and navigation breadcrumbs are not gated; app-supplied ui.* breadcrumbs are deliberately not gated.
  • Firefox/WebKit untested. BlackoutStarted/Ended events are wired on main in a later commit (e3e8253).

🤖 Generated with Claude Code

Core owns a tiny idempotent (not counted) BlackoutState, exposed through the
service container (BlackoutToken) with an internal onChange subscription for the
upcoming BlackoutStarted/BlackoutEnded lifecycle events. Visual providers take
a read-only BlackoutGate: the rrweb recorder is stopped for the window and
restarted (fresh full snapshot) at the end, input presses, ui.* breadcrumbs,
ui.interaction spans and the report-time viewtree are suppressed. Logs, network,
traces and events continue. Real-Chromium e2e proves a DOM change made during a
blackout is absent from the report's replay.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review: T4 blackout (startBlackout / endBlackout / isBlackout)

Reviewed 8a59c9b (e8826c8 is the CI retrigger) against the surrounding compositions, not just the diff: @bugsee/core client lifecycle, browser input source, WebView host bridge, Electron main/renderer/pixel-video, and the interaction/APM path. I did not run the package test suites in this pass.

The kernel piece is in good shape: idempotent, not counted, snapshot dispatch, isolated listener throws, and the browser replay path actually stops rrweb (session token drops late emits; a blackout already active when the lazy chunk resolves never records). The Chromium e2e pins the property that matters for DOM replay.

The privacy contract does not hold once you leave that browser-replay happy path. startBlackout() lives on the shared BugseeClient, so WebView and Electron expose it, isBlackout() returns true, and visual capture continues. Even on the wired browser path, the input source keeps gesture/wheel state across the edge, so coordinates collected during the blackout land in input.json after endBlackout().

Findings

Sev Issue
P1 Shared API is a no-op for WebView (JS input/ui still stream; native video is never told) and Electron (renderer replay + pixel video.webm keep running when the session owner calls startBlackout)
P1 Browser input source is not gated — #openGestures / #penPaths / #mouseDragPaths and #wheelAccum survive the blackout, so a press or scroll begun inside the window is emitted after endBlackout()
P2 Interaction source drops by delivery-time isActive(), not event time — a PerformanceObserver callback that fires after endBlackout() records a click that happened during it
P2 stop() does not clear the flag, so launch() after a blacked-out stop() starts a permanently dark session (Android clears on stop)
P2 WebView inertClient is missing the new methods (TypeError on the path that exists to prevent that); restrictedClientFor spreads the real client, so a repeat launch() can endBlackout()

Accepted as documented in the PR body (not re-reported): persistent DOM in the fresh snapshot; unmatched begin when a pre-blackout press ends during the window; video.aux / navigation crumbs / app-supplied ui.* breadcrumbs not gated; BlackoutStarted/Ended deferred to T1.

Overall risk: High

Merge recommendation: Request changes

Do not treat startBlackout() as shipping privacy until the session-owning compositions honor it and the input source cannot replay a blackout gesture.

Most important to fix

  1. Electron: forward main blackout on the control channel (including the session handshake) and stop/clear pixel video for the span.
  2. WebView: pass the gate into createInputProvider / createUiBreadcrumbProvider, and tell native to start/end its own blackout. Block the methods on restrictedClientFor; add noops on inertClient.
  3. Input source: subscribe to the gate; refuse to collect during a blackout; clear #openGestures, #penPaths, #mouseDragPaths, #hoverIds, and #wheelAccum on both edges.
  4. Interactions: drop by event startTime overlapping a blackout span, not by isActive() at callback time.
  5. Lifecycle: stop() clears the flag (keep a pre-launch() hold if that remains the Android rule).

Positive

Replay is the right design (halt + fresh snapshot, not a mute flag). The lazy-load race is handled. Provider-level input/ui/viewtree gates and the real-browser e2e are the right shape for the browser path.

Open in Web View Automation 

Sent by Cursor Automation: Bugsee code review

Comment thread packages/core/src/client.ts
Comment thread packages/browser/src/launch.ts
Comment thread packages/browser/src/interaction-source.ts
@krassx

krassx commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Review findings triaged against current main (this PR branch is a frozen snapshot, so its diff does not show the fixes): Pointer gestures cannot span or start in a blackout (5bf4791); the rest was already fixed on main (WebView/Electron forwarding, interaction timing). Native Android/iOS receivers for the WebView blackout message remain a follow-up.

The overview PR #46 (head = main) was re-reviewed at ba608c4: no remaining findings, recommendation Approve.

🤖 Generated with Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-check vs origin/main @ ba608c4

Author triage is correct. This PR branch is a frozen T4 snapshot; the findings were verified against current main, not this diff.

Finding Verdict
P1 input source gesture/wheel span fully_addressed in 5bf4791: #dispatch returns while dark (no map writes); #clearGestures + #wheelAccum = null on both edges. Tests pin start-in-dark and span-across.
P1 WebView / Electron ignore the API fully_addressed on main: WebView binds the gate into the input source/provider and posts blackoutMessage; Electron forwards blackout-start/blackout-end (handshake bo) and pixel video stop/clear/no-snapshot.
P2 interaction delivery-time fully_addressed: BlackoutSpan + Event Timing startTime overlap, not isActive() at callback time.
P2 stop() latch / WebView inert+restricted fully_addressed as a later design: startBlackout is a no-op when not launched; endBlackout can lift a carried flag while stopped; inertClient noops the methods; restrictedClientFor allow-lists and refuses endBlackout.

Residual (accepted, out of this JS PR): native Android/iOS receivers for the WebView blackout message are still a host-SDK follow-up. Until those land, a legacy native host that never declares config.blackout keeps recording pixels; JS already warns when support is not declared.

Overall risk: Low (for JS main). Merge recommendation: Approve — this review-only PR changes nothing; the work is on main.

Open in Web View Automation 

Sent by Cursor Automation: Bugsee code review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant