diff --git a/.planning/debug/resolved/01-09-recovery-flow.md b/.planning/debug/resolved/01-09-recovery-flow.md new file mode 100644 index 0000000..cea156d --- /dev/null +++ b/.planning/debug/resolved/01-09-recovery-flow.md @@ -0,0 +1,196 @@ +--- +slug: 01-09-recovery-flow +status: RESOLVED +goal: find_and_fix +trigger: Plan 01-09 Task 5 operator empirical UAT surfaced operator-lockout after Chrome "Stop sharing" mid-recording +tdd_mode: true +phase: 01-stabilize-video-pipeline +plan: 01-09 +opened: 2026-05-17 +resolved: 2026-05-17 +red_commit: 91b4475 +green_commit: b9eeeeb +specialist_review: typescript-expert (not invoked — straight TDD fix, no design ambiguity) +--- + +# Debug session 01-09-recovery-flow — Bug B operator-lockout + +## Problem statement + +During Plan 01-09 Task 5 operator empirical UAT, after the operator clicks +Chrome's "Stop sharing" banner mid-recording, the extension enters a +locked-out state: the toolbar badge stays on ERROR (yellow `ERR`), the +popup remains in SAVE-only mode pointing at `src/popup/index.html`, and +the operator has no path to restart recording. Root mechanism: +`chrome.action.onClicked` only fires when `chrome.action.setPopup({popup: ''})` +is active (per the MV3 spec: a non-empty `default_popup` causes the toolbar +click to open the popup rather than fire `onClicked`). The `RECORDING_ERROR` +handler in `src/background/index.ts:725-744` unconditionally called +`setErrorMode()` regardless of the incoming `message.error` code, including +for `user-stopped-sharing` — which is not actually an error but a deliberate +end-of-session signal from the operator. With the popup pinned, the +toolbar-click restart path was gone. Operator perception: "recording never +stops, and I can't start it again." The accompanying recovery notification +was also failing for a separate reason (Bug A — icon files were placeholder +stubs below Chrome `imageUtil` minimums; resolved at a881bf0). Bug B is the +state-routing fix that remained. + +## Hypotheses verified by prior session (preserved, not re-verified) + +The prior gsd-debug session-manager completed empirical verification on +three hypotheses before context-anxiety pause; this session preserves the +diagnostic record without re-running. + +### H1: `chrome.action.onClicked` is gated by popup presence + +**Status:** VERIFIED. Per the W3C MV3 spec and the Chromium implementation +of `chrome.action`, the `onClicked` event fires for the toolbar icon click +only when `default_popup` is unset (empty string OR not specified in the +manifest). The Plan 01-09 design uses Option B (`setPopup('')` for idle, +`setPopup('src/popup/index.html')` for recording) precisely because clicking +when popup is set opens the popup instead of firing onClicked — pinning the +state machine forces the dispatch outcome. Confirmed by inspecting +`src/background/index.ts` `setIdleMode/setRecordingMode/setErrorMode` +(lines 93-108) and cross-referencing the existing +`tests/background/badge-state-machine.test.ts` Test D dispatch behavior. +Implication for Bug B: `setErrorMode()` flips popup to +`src/popup/index.html`, so any subsequent toolbar click opens the (empty, +SAVE-only) popup instead of firing onClicked. Operator restart path closed. + +### H2: Offscreen recorder emits `RECORDING_ERROR{error:'user-stopped-sharing'}` after `resetBuffer` + +**Status:** VERIFIED. `src/offscreen/recorder.ts:451-480` +(`onUserStoppedSharing`) is registered on every captured track via +`addEventListener('ended', onUserStoppedSharing, {once: true})`. The +handler is idempotent-guarded by `teardownInProgress`. On fire, it calls +`resetBuffer()` (line 458), stops the MediaRecorder, releases all tracks, +and emits `chrome.runtime.sendMessage({type:'RECORDING_ERROR', error: +'user-stopped-sharing'})` (line 471). The buffer is therefore EMPTY by +the time the SW receives the message — SAVE-only popup mode would have +nothing to save. Implication for Bug B: the IDLE landing state is +architecturally correct; SAVE-only mode (the current routing's outcome) +is meaningless because no segments remain. + +### H3: No alternative restart path exists when popup is pinned + +**Status:** VERIFIED. Reviewed the chrome.* surfaces the extension +exposes: `chrome.action.onClicked` (gated by popup), `chrome.notifications +.onClicked` (would work, but Bug A — corrupt icon files — was suppressing +the recovery notification; even after Bug A's a881bf0 fix, an unreliable +notification is not an acceptable primary restart UX), and `chrome.runtime +.onStartup` (only fires on browser session start, not mid-session). The +toolbar click is the canonical restart gesture per the Plan 01-09 charter +("per-session clicks drop from 3 to 2"). No backup path exists; the +toolbar click MUST be made operative after stop-sharing. + +## Fix design + +The patch applies conditional routing to the existing `RECORDING_ERROR` +case in `src/background/index.ts`: + +```typescript +case 'RECORDING_ERROR': { + const errorCode = (message as unknown as { error?: unknown }).error; + if (errorCode === 'user-stopped-sharing') { + isRecording = false; + setIdleMode(); + } else { + setErrorMode(); + // ... existing recovery-notification block preserved + } + return false; +} +``` + +Three sub-decisions: + +1. **`isRecording = false`** alongside `setIdleMode()` for the + stop-sharing branch. The offscreen recorder has already released + tracks and reset the buffer; the SW state must mirror that so a + subsequent `chrome.action.onClicked` doesn't trip the + `if (isRecording) return` guard at the top of the click handler + (`src/background/index.ts:811-815`). + +2. **Recovery notification suppressed** for stop-sharing. The operator + performed the action deliberately; surfacing a "Recording stopped" + notification would be UX noise. Genuine errors (codec-unsupported, + wrong-display-surface, etc.) keep the notification — those are + unexpected and the operator benefits from an explicit click-to-restart + surface. + +3. **Type-safe narrowing** via `(message as unknown as { error?: unknown }).error`. + The canonical `Message` interface (`src/shared/types.ts:25-29`) only + types `type/data/tabId`; the offscreen recorder emits the extra + `error` field as part of the `RECORDING_ERROR` wire shape. The + double-cast keeps us off `as any` per project type discipline. + +No refactor warranted — the conditional adds one narrow + one if/else. +Extracting a `routeRecordingError(code)` helper would just move the +same 2-arm switch one level down without complexity reduction. + +## RED → GREEN evidence + +### RED commit: `91b4475` + +Extended `tests/background/badge-state-machine.test.ts` with Tests E + F: + +- **Test E** (RED today): asserts `RECORDING_ERROR{error:'user-stopped-sharing'}` + triggers `setBadgeText({text:''})` + `setBadgeBackgroundColor({color:'#D32F2F'})` + + `setPopup({popup:''})`, AND does NOT trigger ERROR-state side effects + (no ERR text, no yellow color, no popup html re-set). Snapshot baseline + after init isolates the dispatch delta. +- **Test F** (GREEN today, preservation guard): asserts that any other + error code (representative: `'codec-unsupported'`) continues to route + through `setErrorMode` — badge ERR + yellow + popup html. Regression + pin against the fix over-rotating to IDLE for all codes. + +Vitest output at RED: +``` + FAIL tests/background/badge-state-machine.test.ts > Test E +AssertionError: expected 0 to be greater than or equal to 1 + → expect(offTextAfter - offTextBefore).toBeGreaterThanOrEqual(1); + + Test Files 1 failed (1) + Tests 1 failed | 5 passed (6) +``` + +### GREEN commit: `b9eeeeb` + +Patched `src/background/index.ts` RECORDING_ERROR case with conditional +routing per the fix design above. + +Vitest output at GREEN: +``` + Test Files 18 passed (18) + Tests 83 passed (83) +``` + +(Was 81/81 GREEN before this debug session; +2 from new Tests E+F. +Bug B test Tests E + F both GREEN.) + +Toolchain: +- `npx tsc --noEmit` → exit 0 +- `npm run build` → exit 0; `dist/manifest.json` and bundle assets + emitted cleanly. + +## Files modified + +- `tests/background/badge-state-machine.test.ts` (+157 lines: Tests E+F, + plus 2 helpers `badgeColorCallsFor` + `setPopupCallsFor`, plus header + comment expansion). +- `src/background/index.ts` (case-block patch only, lines 725-744 → + rewritten as conditional routing; +48 / −14). + +## Follow-ups for orchestrator + +- Pre-checkpoint bundle gates (SW CSP, Node-globals, Tier-1 + SW-bundle-import gate, manifest validation) — NOT performed by this + debug session per scope instruction. Orchestrator runs these before + any operator UAT re-surface. +- Operator UAT re-run validates both Bug A (recovery notification fires + via working icons) AND Bug B (badge transitions to OFF on stop-sharing + + toolbar click restarts recording). The recovery notification is now + ONLY emitted for genuine errors (not for stop-sharing) — UAT step 11 + expectation needs adjustment: after operator clicks "Stop sharing", + badge transitions to OFF (not ERROR), and NO recovery notification + fires. To restart, operator clicks the toolbar icon directly.