Persist video playback speed preference

[morgmart]

Summary

• persist the selected desktop video playback speed as a device-level preference
• apply the shared preference to inline and review video players, including after a reload
• cover persistence, validation, cross-window updates, and the end-to-end playback flow

Verification

• node --test desktop/src/shared/lib/videoPlaybackSpeedPreference.test.mjs
• cd desktop && pnpm exec tsc --noEmit
• cd desktop && pnpm check:px-text
• cd desktop && pnpm test
• cd desktop && pnpm build:e2e && pnpm exec playwright test --project=smoke video-attachment.spec.ts
• pre-push: desktop check, file-size check, typecheck, and 6,416 desktop tests

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is e7e29937a145aca7a3c7f5436b07e32c6c20087f...6961ee3078ef627102834116fc11100bb8eb3fc3.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 6961ee3078ef627102834116fc11100bb8eb3fc3 to authorize a new review.
Any previous review applies only to its recorded range.

[jedwards27]

Review verdict: request changes

Reviewed base d595806fc3b9c9758992e39b9b51cbb5f55791b0 through exact head ddb44dba883f315d4da371df45acbf63f52276ab as :bot: Jude’s code review agent.

Required changes

1. Bind the cross-window test to subscriber notification, not only the module cache. desktop/src/shared/lib/videoPlaybackSpeedPreference.test.mjs:60-66 invokes the captured storage callback and checks the cached value, but never subscribes or verifies that an already-mounted consumer is notified. Removing notifyListeners() from the storage-event path at desktop/src/shared/lib/videoPlaybackSpeedPreference.ts:58 leaves all six preference tests passing; mounted inline/review players would then remain stale after another window changes the preference. Please subscribe before firing the event and assert one notification (and none for an unchanged value), or add a two-page behavioral test that proves an already-mounted player's label and HTMLVideoElement.playbackRate update. The notification-removal mutation must fail.

2. Make the new smoke regression identify the newly emitted player deterministically. desktop/tests/e2e/video-attachment.spec.ts:1609-1611 uses timeline-global getByTestId("video-player").last(). In a clean full-file run, the test timed out twice before passing retry 2 because .last() rebound to the already-playing earlier player; isolated runs passed 3/3. This both introduces suite-order flakiness and can test the wrong video. Locate the player through the emitted event/message row or another stable unique identity, then rerun the full file repeatedly without retries.

3. Document or privatize the new exported API. Root AGENTS.md:147-150 requires doc comments for new public API. DEFAULT_VIDEO_PLAYBACK_SPEED, parseVideoPlaybackSpeed, and getVideoPlaybackSpeed are newly exported at desktop/src/shared/lib/videoPlaybackSpeedPreference.ts:15,26,78 without doc comments. Add concise API documentation or keep them module-private if external use is unnecessary.

Evidence and residual risk

The implementation paths inspected were otherwise coherent: stored values are allowlisted; malformed/read-failure values fall back to 1x; same-window changes notify subscribers; storage events handle changed and clear cases; and inline/review video elements consume the shared value. Local exact-head results were 6,416/6,416 package tests, typecheck, check, build, and the targeted persistence E2E 1/1; git diff --check was clean. GitHub's Desktop Core and smoke shards 2–4 were still running at submission, with no reported failures.

Native packaged multi-window delivery, OS keyboard traversal/accessibility, and installed-build theme/zoom behavior were not directly observed. Those are confidence gaps for release/native QA after the deterministic regression coverage is repaired, not additional author defects.

[jedwards27]

Re-review verdict: approve

Re-reviewed base d595806fc3b9c9758992e39b9b51cbb5f55791b0 through exact head 6961ee3078ef627102834116fc11100bb8eb3fc3 as :bot: Jude’s code review agent. The three blockers from the prior head are repaired, and no new concrete defect remains.

Repaired blockers

• Cross-window notification is now causally covered. The test subscribes before dispatching the storage event, asserts exactly one notification, and verifies unchanged values do not notify again (desktop/src/shared/lib/videoPlaybackSpeedPreference.test.mjs:61-76). Removing the production notifyListeners() call now fails that test on the intended assertion (5 pass / 1 fail); restored exact-head source passes 6/6.
• The persistence E2E now identifies each emitted video deterministically. The helper captures the emitted event ID and scopes the player through its message row (desktop/tests/e2e/video-attachment.spec.ts:1594-1618), then checks the exact second/reloaded player's label and HTMLVideoElement.playbackRate (:1637-1669). The new row passed 3/3 in complete-file runs with CI=1 and retries disabled. One older unchanged context-menu row failed once in the third run; the persistence row still passed, and the failing code exists unchanged at the base, so this is not attributed to the PR.
• Public API documentation is complete. All nine added exports now have doc comments (desktop/src/shared/lib/videoPlaybackSpeedPreference.ts:3-16,22-34,66-86,89-115), including the three previously missing comments.

Validation

At this exact head: Desktop package tests pass 6,416/6,416; just desktop-check, just desktop-typecheck, just desktop-build, E2E build, and git diff --check pass. Storage validation/failure recovery, same-window and external-window notification, listener cleanup, and both inline/review application paths were traced. The PR remains mergeable with the pinned base/head and no failing GitHub checks; Desktop Core and smoke shard 4 were still running at submission.

Packaged native multi-window/WKWebView delivery and OS-level keyboard/AX/theme/zoom behavior were not directly observed. Release/native QA owns that residual confidence gap; no author action is required.

[jedwards27]

Review verdict: approve

Re-reviewed base d595806fc3b9c9758992e39b9b51cbb5f55791b0 through exact head 6961ee3078ef627102834116fc11100bb8eb3fc3 as :bot: Jude’s code review agent.

The three blockers from the prior head are repaired:

• The external-window storage test now subscribes, asserts exactly one notification, and suppresses unchanged-value notifications. Removing the production notifyListeners() call makes the targeted suite fail at the intended assertion (5 pass / 1 fail); restored source passes 6/6.
• The persistence E2E now scopes each player through the emitted message ID rather than timeline-global .last(). The changed test passed 3/3 in full-file, no-retry runs; two complete runs passed 13/13. A third run's only failure was an older unchanged context-menu row and is not attributable to this PR.
• All newly exported preference APIs now have the required doc comments.

The implementation trace found no remaining persistence, validation, listener-lifecycle, inline/review-player, control-state, or accessibility defect. Exact-head validation passed the Desktop package suite (6,416/6,416), check, typecheck, build, E2E build, git diff --check, the causal mutation, and the repaired playback regression. GitHub still reports the PR mergeable at the pinned head.

Packaged native multi-window/WKWebView delivery, OS-level keyboard/accessibility, and installed-build theme/zoom behavior were not directly observed. Those remain release/native-QA confidence gaps with no author action required; exact-head CI/release gates retain ownership of their normal coverage.