diff --git a/.planning/phases/01-stabilize-video-pipeline/01-09-SUMMARY.md b/.planning/phases/01-stabilize-video-pipeline/01-09-SUMMARY.md new file mode 100644 index 0000000..aa615e1 --- /dev/null +++ b/.planning/phases/01-stabilize-video-pipeline/01-09-SUMMARY.md @@ -0,0 +1,204 @@ +--- +phase: 01-stabilize-video-pipeline +plan: 09 +subsystem: offscreen-recorder + background-sw + popup + manifest +tags: + - getDisplayMedia + - displaySurface + - cursor + - toolbar + - chrome.action.onClicked + - chrome.notifications + - chrome.runtime.onStartup + - badge-state-machine + - manifest + - popup-save-only + - D-15-display-surface +requires: + - 01-08 (Plan 01-08 SUMMARY + Tier-1 SW-bundle-import gate GREEN) +provides: + - REQ-video-ring-buffer:closure-via-D-15-display-surface + - operator-facing badge state machine (REC/OFF/ERROR) + recovery notifications + - getDisplayMedia constraint hint (displaySurface:'monitor', cursor:'always') + + post-grant enforcement (wrong-display-surface RECORDING_ERROR) + - toolbar-onClicked direct-to-picker flow (popup SAVE-only) + - onStartup notification inviting fresh recording on browser launch +affects: + - src/offscreen/recorder.ts (CaptureErrorCode union + classifyCaptureError + + getDisplayMedia call site + post-grant validation block) + - src/background/index.ts (badge state machine + 3 mode helpers + 3 new + listener registrations + RECORDING_ERROR onMessage branch + + initialize() setIdleMode call) + - src/popup/index.ts (SAVE-only — checkPermissions + requestPermissions + removed; popupState defaults to hasPermissions:true so SAVE is enabled) + - manifest.json (added 'notifications' permission) + - smoke.sh (SHARE_TARGET monitor mode + locale-fallback comment + + HTML stub instruction updates; T+/wall overlay preserved) +tech-stack: + added: + - chrome.action.onClicked (MV3) + - chrome.notifications.create / clear / onClicked + - chrome.runtime.onStartup + - MediaTrackConstraints displaySurface + cursor (Screen Capture spec) + patterns: + - 3-state badge state machine driven by setBadgeState helper + - setPopup('' ⇄ html) dance gating onClicked vs popup-open + - Constraint-hint-vs-enforcement gap: getDisplayMedia constraints are + HINTS per W3C spec; post-grant track.getSettings().displaySurface + check is the actual enforcement + - Notification id prefix namespace ('mokosh-startup-', 'mokosh-recovery-') + for spoofing mitigation (T-1-09-01) + - Defense-in-depth try/catch around all chrome.* listener registrations + so unit-test stubs that don't define every surface don't crash SW load +key-files: + created: + - tests/offscreen/display-surface-constraint.test.ts (4 tests) + - tests/background/toolbar-action.test.ts (5 tests inc. W-02 Test E) + - tests/background/badge-state-machine.test.ts (4 tests) + - tests/background/onstartup-notification.test.ts (4 tests) + modified: + - src/offscreen/recorder.ts (CaptureErrorCode + getDisplayMedia + validation) + - src/background/index.ts (~150 LOC of helpers + listeners + handler) + - src/popup/index.ts (~50 LOC removed, defaults flipped) + - manifest.json (1 permission added) + - smoke.sh (3 string literals + 14 comment lines) +decisions: + - displaySurface constraint is shipped with strict deep-equality test + pinning the EXACT constraints object {video:{displaySurface:'monitor', + cursor:'always'},audio:false} — a future refactor that drops either + constraint silently fails loudly. cursor:'always' was opportunistically + lifted from the Phase 5 deferral list per the I-03 fix. + - Post-grant validation is the actual enforcement (spec treats + displaySurface as a hint, not a hard constraint). Stream tracks are + stopped + mediaStream nulled + Error thrown — the existing catch + block routes it through classifyCaptureError + RECORDING_ERROR. + - setPopup '' ⇄ html dance chosen over removing default_popup entirely + (Option B per the plan interfaces): keeps the manifest stable + + lets the SW dynamically gate onClicked vs popup-open per state. + - Popup defaults flipped to hasPermissions:true / isRecording:true + because under the SAVE-only charter the popup only opens when + recording is already active (setPopup html path is set by + setRecordingMode/setErrorMode). + - All Plan 01-09 listener registrations wrapped in try/catch to + preserve the 6 pre-existing background tests (request-id-protocol + + port-lifecycle-continuous) whose chrome stubs predate + chrome.action/notifications/onStartup. Production MV3 always + provides these surfaces. + - smoke.sh SHARE_TARGET set to English 'Entire screen' with a 14-line + locale-fallback comment citing chromium/grit generated_resources.grd + as the authoritative source. Per W-04 fix the prior citation to + desktop_capture_device_uma_types.cc (UMA enums) was wrong and dropped. +metrics: + duration: ~35 minutes (executor agent) + completed: 2026-05-17 + task_count: 4 autonomous (Task 5 is operator-gated checkpoint, deferred) + files_modified: 9 (4 created + 5 modified) + net_loc: +1200 (~887 test LOC + ~310 src LOC + ~14 smoke comment lines) +--- + +# Phase 1 Plan 9: Toolbar-onClicked Direct-to-Picker + Display-Surface Monitor + Badge State Machine + Notifications + SAVE-only Popup + +Activation-cost reduction (3 clicks → 2) + footgun retirement (operator-picks-tab no longer reaches recording) + operator-visible recording state via badge + OS notifications inviting fresh starts and surfacing errors. + +## One-Liner + +`displaySurface:'monitor' + cursor:'always'` getDisplayMedia constraints with post-grant enforcement; `chrome.action.onClicked` toolbar-to-picker flow (popup SAVE-only); 3-state badge machine (REC/OFF/ERROR) + `chrome.runtime.onStartup` notification + `chrome.notifications.onClicked` recovery flow. + +## What Landed + +### Tests (17 new, all GREEN end-of-plan) + +| Test File | Tests | Contract Pinned | +| --- | --- | --- | +| `tests/offscreen/display-surface-constraint.test.ts` | 4 | getDisplayMedia exact constraints + non-monitor pick teardown + monitor over-fire guard + classifyCaptureError branch | +| `tests/background/toolbar-action.test.ts` | 5 (A-E) | onClicked registered + no-record→start + recording→no-double-start + setPopup state-machine + popup SAVE-only init contract (W-02 Test E) | +| `tests/background/badge-state-machine.test.ts` | 4 | REC palette + OFF palette (init) + ERROR palette + RECORDING_ERROR-triggered ERROR transition | +| `tests/background/onstartup-notification.test.ts` | 4 | onStartup registered + creates mokosh-startup- notification + onClicked clears + START_RECORDING + RECORDING_ERROR triggers mokosh-recovery- | + +### Source Changes + +- **`src/offscreen/recorder.ts`** — `CaptureErrorCode` union extended with `'wrong-display-surface'`; `classifyCaptureError` matches the prefix; `getDisplayMedia` call carries `{video:{displaySurface:'monitor',cursor:'always'},audio:false}` (typed via explicit widening cast — lib.dom.d.ts omits `cursor`); post-grant validation reads `track.getSettings().displaySurface`, on mismatch tears down stream + nulls mediaStream + throws `wrong-display-surface` Error which routes through the existing catch+RECORDING_ERROR pipeline. + +- **`src/background/index.ts`** — Added badge palette constants (SCREAMING_SNAKE), `setBadgeState(state: 'REC'|'OFF'|'ERROR')` helper (3-state machine), `setIdleMode`/`setRecordingMode`/`setErrorMode` mode helpers, `startVideoCapture` wires `setRecordingMode` on success and `setErrorMode` in catch, `chrome.action.onClicked` direct-to-picker listener, `chrome.runtime.onStartup` notification creator, `chrome.notifications.onClicked` mokosh-prefix-gated handler, RECORDING_ERROR onMessage branch with recovery notification, `initialize()` calls `setIdleMode` at boot. All registrations defensively wrapped in try/catch. + +- **`src/popup/index.ts`** — Removed `checkPermissions` + `requestPermissions` functions; `popupState` defaults to `hasPermissions:true, isRecording:true` under SAVE-only charter; `init()` no longer fires REQUEST_PERMISSIONS; empty-state copy points operator back to toolbar icon. + +- **`manifest.json`** — Added `"notifications"` to permissions array. + +- **`smoke.sh`** — `SHARE_TARGET="Entire screen"` (locale-English) + 14-line locale-fallback comment block citing Chromium grit generated_resources.grd as authoritative source for IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_SCREEN identifier (locale strings drift across Chrome versions); HTML title made distinct from screen-source string; `
    ` and intro paragraph updated to reflect entire-screen sharing. T+/wall timer overlay (commit 923aaca) preserved. + +## Test Counts + +- Baseline before Plan 01-09: 15 files / 64 tests / all GREEN. +- After Task 1 RED commit: 16 files / 68 tests / 4 RED (Tests 1, 2, 3, 4 → only Test 3 GREEN because no over-fire from a non-existent validation block). +- After Task 2 GREEN: 16 files / 68 tests / all GREEN. +- After Task 3 RED commit: 18 files / 81 tests / 13 RED. +- After Task 4 GREEN: 18 files / 81 tests / all GREEN. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 3 - Blocking] Vitest node env: `navigator` getter-only** +- **Found during:** Task 1 (running the preserved RED test). +- **Issue:** `globalThis.navigator` in Vitest's node env has only a getter; direct assignment throws `Cannot set property navigator of # which has only a getter`. All 4 preserved tests failed at the env-setup step instead of for the right contract reasons. +- **Fix:** Added `installNavigatorStub(spy)` helper that uses `Object.defineProperty(globalThis, 'navigator', {value, configurable:true, writable:true})` instead of direct assignment. All 4 navigator assignments rewired to the helper. +- **Files modified:** `tests/offscreen/display-surface-constraint.test.ts` +- **Commit:** 333e0dc + +**2. [Rule 3 - Blocking] jsdom not installed; W-02 Test E refactored to node-env** +- **Found during:** Task 3 (running the new test files). +- **Issue:** The plan's W-02 Test E originally used `@vitest-environment jsdom` pragma but `jsdom` is not in `node_modules`. Installing it would be an unscoped dependency add. +- **Fix:** Refactored Test E to a node-env-friendly form that manually stubs the document surface the popup module touches (`getElementById('saveButton'/'statusMessage')`, `querySelector('.button-text')`, `addEventListener` on both button and document, plus a `_docListeners` map so the test can synthesize the `DOMContentLoaded` event). Test E still pins both contracts (E1: no REQUEST_PERMISSIONS at popup-open; E2: saveButton enabled). +- **Files modified:** `tests/background/toolbar-action.test.ts` +- **Commit:** 2d7ff7d + +**3. [Rule 1 - Bug] TypeScript: `cursor` not in `MediaTrackConstraints`** +- **Found during:** Task 2 (post-source-edit `npx tsc --noEmit`). +- **Issue:** lib.dom.d.ts's `MediaTrackConstraints` type omits `cursor` (the Screen Capture spec defines it but TypeScript's bundled types lag; cited MDN). Initial GREEN edit failed TSC with `TS2353: Object literal may only specify known properties`. +- **Fix:** Used explicit type-widening cast `as DisplayMediaStreamOptions & {video:{displaySurface:'monitor';cursor:'always'}}` (NOT `as any` per project style — CLAUDE.md "Don't ignore lint/type errors without researching proper fixes first") to add the field precisely. +- **Files modified:** `src/offscreen/recorder.ts` +- **Commit:** de162b4 + +**4. [Rule 1 - Bug] Pre-existing background tests broken by new chrome.* listener registrations** +- **Found during:** Task 4 (full-suite run after GREEN edit). +- **Issue:** 6 pre-existing background tests (5 in `request-id-protocol.test.ts`, 1 in `port-lifecycle-continuous.test.ts`) crashed at module load with `TypeError: Cannot read properties of undefined (reading 'onClicked')` because their chrome stubs predate `chrome.action`, `chrome.notifications`, `chrome.runtime.onStartup`. +- **Fix:** Wrapped each of the 3 new top-level listener `addListener` calls in `try/catch` (matches the existing defensive pattern in the file, e.g. `chrome.offscreen?.hasDocument` check at line 673). Production MV3 always provides these surfaces. +- **Files modified:** `src/background/index.ts` +- **Commit:** 06dee24 + +## Tier-1 SW-Bundle-Import Gate + +GREEN throughout. The gate exercises `npm run build` + Layer 1 (SW module loads under jsdom-like environment) + Layer 2 (`remuxSegments` runs with the Buffer polyfill). The Plan 01-09 changes (new listener registrations, badge state machine, popup SAVE-only) compile and bundle cleanly; the Buffer polyfill from Plan 01-08 commit dd7bf00 + ebml CJS resolution from cc6e81a both remain intact (no `vite.config.ts` edits in this plan). + +## Task 5 Status: Operator Checkpoint — Deferred + +Task 5 (`checkpoint:human-verify`) requires an operator to run `npm run build` + `./smoke.sh`, open Chrome, load the unpacked extension, and walk through 13 end-to-end verification steps covering: notification at browser startup, monitor-only picker, badge transitions REC/OFF/ERROR, recovery notification on stop-sharing, popup-opens-when-recording, idle-toolbar-click-triggers-picker. The how-to-verify section in the plan file lists all 13 steps + the resume-signal contract. + +The autonomous side (Tasks 1-4) is complete with 17/17 new tests GREEN and full suite 18 files / 81 tests / all GREEN. tsc clean; build clean; `dist/manifest.json` carries the `notifications` permission. + +## Architectural Notes Worth Carrying Forward + +- **Constraint-hint-vs-enforcement gap.** `getDisplayMedia({video:{displaySurface:'monitor',...}})` is a HINT to Chrome's picker, NOT a hard constraint — operators can still pick a tab or window. Post-grant `track.getSettings().displaySurface` is the actual enforcement. Future plans that add new MediaTrackConstraints should treat constraints as preferences and add post-grant validation for anything load-bearing. + +- **setPopup '' ⇄ html dance.** chrome.action.onClicked.addListener fires ONLY when no default_popup is set (or it's been cleared via `chrome.action.setPopup({popup:''})`). Dynamically swapping the popup is the canonical Chrome pattern for "click-action depends on state" toolbar buttons. The SAVE-only popup charter relies on this — when recording is OFF the click does start-action; when REC it opens SAVE. + +- **mokosh- notification id namespace.** All Plan 01-09 notifications use `mokosh-startup-` or `mokosh-recovery-` prefixes with `Date.now()` suffix for uniqueness. The onClicked handler validates the prefix before any side effect (T-1-09-01 spoofing mitigation). Future notification work should keep extending this namespace (e.g. `mokosh-savefail-` for save-error toasts). + +- **Defense-in-depth chrome.* try/catch.** Every chrome API call that touches a surface a unit-test stub might not fully implement is wrapped in try/catch + a logger.warn. This is now the project's documented pattern (per the deviation 4 footnote) — unit tests should be able to load the SW module without crashing even if they only stub the subset they care about. + +## Self-Check: PASSED + +- FOUND: tests/offscreen/display-surface-constraint.test.ts +- FOUND: tests/background/toolbar-action.test.ts +- FOUND: tests/background/badge-state-machine.test.ts +- FOUND: tests/background/onstartup-notification.test.ts +- FOUND: .planning/phases/01-stabilize-video-pipeline/01-09-SUMMARY.md +- FOUND: 333e0dc (Task 1 RED) +- FOUND: de162b4 (Task 2 GREEN) +- FOUND: 2d7ff7d (Task 3 RED) +- FOUND: 06dee24 (Task 4 GREEN) + +## Known Stubs + +None. The popup empty-state copy points operators to use the toolbar icon — this is intentional UX, not a stub (the toolbar icon is the start path; the popup is the save path). No placeholder data or hardcoded empty values flowing to UI.