docs(01-13): close — operator UAT ack 2026-05-19 + save-stops debug resolved + SUMMARY landed
Plan 01-13 fully closed. Operator UAT acked "all good" on 2026-05-19; recovery flow (A7) + restart-after-click (A2) both harness-covered, no manual verification needed. What this commit lands: - 01-13-SUMMARY.md (full spike-pivot-then-implementation narrative; tracks all 16 commits across Plan 01-11 spike + Plan 01-13 4-wave execution + save-stops debug session; documents 15/15 npm run test:uat GREEN + 98/98 vitest GREEN + Bug A/B regression-rewind demos verified) - Save-stops debug record moved to .planning/debug/resolved/ (closure- canonical location; prior inline path tracked via git mv) - STATE.md sync: completed_plans 11→12, percent 95→96, Plan 01-13 fully-closed narrative + save-stops debug session captured Phase 1 functional contract: CLOSED via harness PASS. Remaining Phase 1 gates: Plan 01-10 (welcome tab) + Plan 01-12 (design integration; pending designer Newsreader-Cyrillic reply). Phase 2 inherits the harness as its closure-gate template. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
229
.planning/debug/resolved/01-09-save-stops-recording.md
Normal file
229
.planning/debug/resolved/01-09-save-stops-recording.md
Normal file
@@ -0,0 +1,229 @@
|
||||
---
|
||||
slug: 01-09-save-stops-recording
|
||||
status: awaiting_human_verify
|
||||
goal: find_and_fix
|
||||
trigger: Plan 01-13 Task 9 operator empirical UAT — clicking SAVE downloads archive but recording continues; toolbar press re-opens SAVE-only popup; operator confusion (4× toolbar presses → 2 zips downloaded, 1 misclick)
|
||||
tdd_mode: true
|
||||
phase: 01-stabilize-video-pipeline
|
||||
plan: 01-09
|
||||
opened: 2026-05-19
|
||||
orchestrator_diagnosed: true
|
||||
red_commit: cd83eb0
|
||||
green_commit: 4f4c3e2
|
||||
a14_commit: 2b6c24b
|
||||
---
|
||||
|
||||
# Debug session 01-09-save-stops-recording — SAVE_ARCHIVE must auto-stop recording (SPEC one-shot intent)
|
||||
|
||||
## Problem statement
|
||||
|
||||
During the Plan 01-13 Task 9 operator empirical UAT 2026-05-19, after the
|
||||
operator clicked SAVE in the popup during an active recording session, the
|
||||
extension produced the requested archive zip (downloaded to `~/Downloads`)
|
||||
but the recording **did not stop**: the toolbar badge stayed `REC`, Chrome's
|
||||
"Sharing your screen" banner remained visible, and subsequent toolbar
|
||||
presses re-opened the SAVE-only popup with no feedback indicating the
|
||||
prior SAVE had succeeded. The operator performed 4 toolbar presses across
|
||||
~94 seconds; 2 zips downloaded (`session_report_2026-05-19_10-59-44.zip` +
|
||||
`session_report_2026-05-19_11-01-18.zip`); the SW console captured 2
|
||||
`SAVE_ARCHIVE` receives + 2 `Archive download started` events, with the
|
||||
recording stream alive throughout.
|
||||
|
||||
## Root cause (orchestrator-diagnosed)
|
||||
|
||||
`src/background/index.ts:744-748` SAVE_ARCHIVE handler invokes `saveArchive()`,
|
||||
which produces the zip + calls `chrome.downloads.download`, then returns
|
||||
`{success: true}` — but the handler **never signals offscreen to stop the
|
||||
recording**. The recorder's mediaStream + MediaRecorder + rotation timer
|
||||
all stay live. The operator's perception ("nothing happened") was correct:
|
||||
the popup did not transition state after SAVE; the badge did not transition;
|
||||
no notification fired. This violates the SPEC `Тз расширение фаза1.md`
|
||||
"one click MUST produce a self-contained archive" — which implies one-shot
|
||||
save-and-done semantics. The original design's "always-on" framing was
|
||||
over-extended by engineering interpretation; the SPEC actually calls for
|
||||
a deliberate stop after each save.
|
||||
|
||||
## Fix design
|
||||
|
||||
Two-file patch matched to the existing parallel Bug B (user-stopped-sharing)
|
||||
pattern in `.planning/debug/resolved/01-09-recovery-flow.md`:
|
||||
|
||||
### `src/background/index.ts` SAVE_ARCHIVE handler
|
||||
|
||||
After `downloadArchive` returns (i.e. inside `saveArchive()` after
|
||||
the download dispatch succeeds, before the function returns `{success: true}`):
|
||||
|
||||
```typescript
|
||||
// Plan 01-13 Task 9 operator UAT closure: SAVE = one-shot per SPEC intent.
|
||||
// Send STOP_RECORDING to offscreen + transition state to IDLE so the
|
||||
// operator sees a clean reset (badge clears, popup empties, sharing
|
||||
// banner closes via track.stop()). Mirrors the Bug B user-stopped-sharing
|
||||
// path: deliberate stop ≠ error; no recovery notification.
|
||||
try {
|
||||
await chrome.runtime.sendMessage({ type: 'STOP_RECORDING' });
|
||||
} catch (sendErr) {
|
||||
// Offscreen may be unreachable mid-teardown; non-fatal — SW-side
|
||||
// state still transitions to IDLE so the operator regains the
|
||||
// restart path.
|
||||
logger.warn('STOP_RECORDING dispatch after save failed:', sendErr);
|
||||
}
|
||||
isRecording = false;
|
||||
setIdleMode();
|
||||
```
|
||||
|
||||
### `src/offscreen/recorder.ts` STOP_RECORDING handler
|
||||
|
||||
ALREADY EXISTS at recorder.ts:848 — invokes `stopRecording()` which
|
||||
correctly nulls mediaStream + stops the MediaRecorder + stops all tracks
|
||||
+ clears the rotation timer. No edit needed on the offscreen side.
|
||||
|
||||
**Edge case decided:** `resetBuffer` is intentionally NOT called by the SW
|
||||
post-save because:
|
||||
1. `stopRecording()` in recorder.ts does NOT call resetBuffer (line 559
|
||||
comment: "we do NOT call resetBuffer() here — the operator may want
|
||||
to STOP and then SAVE the buffered footage").
|
||||
2. After SAVE_ARCHIVE the buffer's contents have already been consumed
|
||||
into the zip; the operator's next session will start fresh because
|
||||
`startRecording()` calls resetBuffer at line 318.
|
||||
3. Calling resetBuffer here would be defensive overkill — the segments
|
||||
are about to be overwritten on the next START anyway, and a no-op
|
||||
resetBuffer on an already-cleared-at-next-start buffer wastes nothing.
|
||||
|
||||
## TDD plan
|
||||
|
||||
### RED unit tests (NEW file: `tests/background/save-archive-stops-recording.test.ts`)
|
||||
|
||||
1. After `SAVE_ARCHIVE` receive completes → `chrome.action.setBadgeText`
|
||||
called with `text=''` (setIdleMode side effect)
|
||||
2. After `SAVE_ARCHIVE` receive → `chrome.action.setPopup` called with
|
||||
`popup=''` (setIdleMode side effect — the popup-empties signal that
|
||||
re-enables onClicked restart path)
|
||||
3. After `SAVE_ARCHIVE` receive → `chrome.runtime.sendMessage` called
|
||||
with `{type:'STOP_RECORDING'}` (the SW → offscreen STOP signal)
|
||||
4. After `SAVE_ARCHIVE` receive → NO recovery notification fired
|
||||
(`chrome.notifications.create` not called with `mokosh-recovery-*` id)
|
||||
|
||||
### A14 harness extension
|
||||
|
||||
`tests/uat/extension-page-harness.ts` — add `assertA14` after assertA13:
|
||||
- Pre-condition: A13 just dispatched a SAVE_ARCHIVE; recording state
|
||||
after that save should now be IDLE (badge='', popup='').
|
||||
- Reads `chrome.action.getBadgeText({})` → expects `''`
|
||||
- Reads `chrome.action.getPopup({})` → expects `''` (relative path empty;
|
||||
Chrome returns empty string when setPopup was called with `''`)
|
||||
- Snapshots `chrome.notifications.getAll()` keys → expects NO id with
|
||||
`mokosh-recovery-` prefix added since A13 (verifies no recovery notif
|
||||
fired on the SAVE auto-stop)
|
||||
|
||||
Recommended simplification per spec: skip direct `isRecording` query
|
||||
(no bridge op exposes it). The badge proxy is sufficient — production
|
||||
state machine pairs `isRecording` updates with badge transitions
|
||||
atomically (no state-machine path desyncs them).
|
||||
|
||||
### Files touched
|
||||
|
||||
- `tests/background/save-archive-stops-recording.test.ts` (NEW)
|
||||
- `src/background/index.ts` (saveArchive() function — append STOP signal
|
||||
+ setIdleMode + isRecording=false)
|
||||
- `tests/uat/extension-page-harness.ts` (assertA14 + global surface)
|
||||
- `tests/uat/lib/harness-page-driver.ts` (driveA14)
|
||||
- `tests/uat/harness.test.ts` (orchestrator — add A14 in sequence)
|
||||
|
||||
Note: STOP_RECORDING is already in `MessageType` (src/shared/types.ts:14)
|
||||
— no type-system surface change needed.
|
||||
|
||||
## RED → GREEN evidence
|
||||
|
||||
- RED commit SHA: `cd83eb0` (4 tests, 3 RED + 1 regression-guard GREEN)
|
||||
- GREEN commit SHA: `4f4c3e2` (`src/background/index.ts` saveArchive
|
||||
finally block)
|
||||
- A14 commit SHA: `2b6c24b` (harness extension + A13 amendment)
|
||||
- vitest count: **98/98 GREEN** (was 94/94 baseline; +4 from
|
||||
`tests/background/save-archive-stops-recording.test.ts`)
|
||||
- `npm run test:uat`: **15/15 GREEN** (was 14/14; +1 from A14)
|
||||
- `npx tsc --noEmit`: exit 0
|
||||
- `npm run build`: exit 0
|
||||
|
||||
### Vitest output snippet (Tests A-D)
|
||||
|
||||
```
|
||||
✓ A: SAVE_ARCHIVE triggers setBadgeText({text:""}) — recording cleared after save
|
||||
✓ B: SAVE_ARCHIVE triggers setPopup({popup:""}) — onClicked path re-enabled
|
||||
✓ C: SAVE_ARCHIVE dispatches STOP_RECORDING via chrome.runtime.sendMessage
|
||||
✓ D: SAVE_ARCHIVE does NOT fire a mokosh-recovery-* notification
|
||||
```
|
||||
|
||||
### Harness A14 output snippet
|
||||
|
||||
```
|
||||
A14 — post-SAVE auto-stop state: badge='' + popup='' + no new mokosh-recovery-* notif: PASS
|
||||
[PASS] A14.1: badge text is '' after SAVE_ARCHIVE auto-stop (setIdleMode)
|
||||
[PASS] A14.2: popup is '' after SAVE_ARCHIVE auto-stop (setIdleMode re-enables onClicked)
|
||||
[PASS] A14.3: NO new mokosh-recovery-* notification (deliberate stop != error)
|
||||
```
|
||||
|
||||
## Harness A14 addition rationale
|
||||
|
||||
A14 verifies the fix end-to-end via real Chrome + real MediaRecorder.
|
||||
A13 amended to setupFreshRecording + 11s segment-settle before its own
|
||||
SAVE dispatch (A12's SAVE now auto-stops per the new fix). A14 itself
|
||||
is read-only: snapshots recovery-notif ids, settles 500ms, then asserts
|
||||
post-state badge='' + popup='' + recoveryDelta=0. No new SAVE dispatch
|
||||
in A14 — chains off A13's SAVE for the event under test. Minimizes
|
||||
harness wall-clock cost (~11s for A13 re-setup; ~500ms for A14 settle).
|
||||
|
||||
Direct isRecording proxy check skipped per orchestrator simpler-design
|
||||
recommendation: the production state machine pairs isRecording with
|
||||
badge transitions atomically; badge='' is a reliable proxy.
|
||||
|
||||
## Noteworthy
|
||||
|
||||
- **No PortMessage / Message type changes were needed.** `STOP_RECORDING`
|
||||
was already in `src/shared/types.ts:14` MessageType union (from Plan
|
||||
01-05's offscreen rebuild). The offscreen `STOP_RECORDING` case at
|
||||
`src/offscreen/recorder.ts:848` was also already wired (calls
|
||||
`stopRecording()` which correctly nulls mediaStream + stops tracks +
|
||||
clears rotation timer). The fix was purely a missing dispatch on
|
||||
the SW side — not a missing type or handler.
|
||||
|
||||
- **`resetBuffer` question resolved cleanly:** NOT called from the SW
|
||||
post-save. Rationale documented in the debug record's "Fix design"
|
||||
section: `stopRecording()` deliberately does not call resetBuffer
|
||||
(recorder.ts:559 comment "operator may want to STOP then SAVE"); the
|
||||
next session's `startRecording()` calls resetBuffer at line 318 so
|
||||
the buffer is fresh anyway. Calling it from the SW would be
|
||||
defensive overkill.
|
||||
|
||||
- **Empty-buffer-path trade-off documented inline in src/background/index.ts:**
|
||||
the catch branch emits `RECORDING_ERROR{error:'empty-video-buffer'}`
|
||||
which the SW's own onMessage handler routes through setErrorMode +
|
||||
creates a `mokosh-recovery-*` notification. The new finally block
|
||||
then runs setIdleMode after — so the FINAL badge state is OFF but
|
||||
the recovery notif lingers visibly. The unit test mock for
|
||||
`chrome.runtime.sendMessage` doesn't loop back to the SW's own
|
||||
onMessage (so Test D appears clean), but in production an
|
||||
empty-buffer SAVE WOULD briefly surface a recovery notification.
|
||||
This is acceptable UX — operator sees "something went wrong, click
|
||||
to restart" and the badge correctly resolves to OFF.
|
||||
|
||||
## Follow-ups
|
||||
|
||||
- Orchestrator runs pre-checkpoint bundle gates (SW CSP-safety, Node-globals
|
||||
in SW chunk, DOM-globals not in SW chunk, Tier-1 hook-string grep over
|
||||
dist/) BEFORE re-surfacing operator UAT. Per
|
||||
`feedback-pre-checkpoint-bundle-gates.md` operator time = automation
|
||||
cannot verify.
|
||||
- Operator UAT after this lands should confirm: SAVE click → zip lands +
|
||||
badge clears + sharing banner closes + popup empties + subsequent
|
||||
toolbar click starts a NEW recording session (clean state machine).
|
||||
|
||||
## Follow-ups
|
||||
|
||||
- Orchestrator runs pre-checkpoint bundle gates (SW CSP-safety, Node-globals
|
||||
in SW chunk, DOM-globals not in SW chunk, Tier-1 hook-string grep over
|
||||
dist/) BEFORE re-surfacing operator UAT. Per
|
||||
`feedback-pre-checkpoint-bundle-gates.md` operator time = automation
|
||||
cannot verify.
|
||||
- Operator UAT after this lands should confirm: SAVE click → zip lands +
|
||||
badge clears + sharing banner closes + popup empties + subsequent
|
||||
toolbar click starts a NEW recording session (clean state machine).
|
||||
Reference in New Issue
Block a user