diff --git a/backlog/tasks/task-255 - Add-overlay-playlist-browser-modal-for-sibling-video-files-and-mpv-queue.md b/backlog/tasks/task-255 - Add-overlay-playlist-browser-modal-for-sibling-video-files-and-mpv-queue.md index 2e005a41..1024c4ad 100644 --- a/backlog/tasks/task-255 - Add-overlay-playlist-browser-modal-for-sibling-video-files-and-mpv-queue.md +++ b/backlog/tasks/task-255 - Add-overlay-playlist-browser-modal-for-sibling-video-files-and-mpv-queue.md @@ -5,7 +5,7 @@ status: In Progress assignee: - codex created_date: '2026-03-30 05:46' -updated_date: '2026-03-31 01:42' +updated_date: '2026-03-31 04:57' labels: - feature - overlay @@ -26,8 +26,8 @@ Add an in-session overlay modal that opens from a keybinding during active playb - [ ] #2 The modal shows video files from the current media file's parent directory in best-effort episode order and highlights the current file when present. - [ ] #3 The modal shows the active mpv playlist/queue with enough metadata to identify the current item and queued order. - [ ] #4 The user can add a directory file to the mpv playlist, remove playlist items, and reorder playlist items from the modal using both mouse and keyboard interactions. -- [ ] #5 Modal state stays in sync after playlist mutations so the rendered queue reflects mpv's current playlist order. -- [ ] #6 Feature coverage includes automated tests for ordering/playlist behavior and docs or shortcut/help updates for the new modal. +- [x] #5 Modal state stays in sync after playlist mutations so the rendered queue reflects mpv's current playlist order. +- [x] #6 Feature coverage includes automated tests for ordering/playlist behavior and docs or shortcut/help updates for the new modal. ## Implementation Plan @@ -42,6 +42,8 @@ Add an in-session overlay modal that opens from a keybinding during active playb 7. Run targeted test lanes first, then the maintained verification gate relevant to the touched surfaces; update task notes/criteria as checks pass. 2026-03-30 CodeRabbit follow-up: 1) add failing runtime coverage for unreadable playlist-browser file stat failures, 2) add failing renderer coverage for stale snapshot UI reset on refresh failure/close, 3) add failing renderer coverage to block playlist-browser open when another modal already owns the overlay, 4) implement minimal fixes, 5) rerun targeted tests plus typecheck for touched surfaces. + +2026-03-30 current CodeRabbit round: verify 4 unresolved threads, ignore already-fixed outdated dblclick thread if current code matches, add failing-first coverage for selection preservation / timestamp fixture consistency / string test-clock alignment, implement minimal fixes, rerun targeted tests plus typecheck. ## Implementation Notes @@ -70,4 +72,8 @@ Addressed CodeRabbit follow-ups on the playlist browser PR: clamped stale playin Additional follow-up: moved playlist-browser keydown handling ahead of keyboard-driven lookup controls so KeyH/ArrowLeft/ArrowRight and related chords are routed to the modal first. Verification refreshed with `bun test src/main/runtime/playlist-browser-runtime.test.ts src/renderer/modals/playlist-browser.test.ts src/renderer/handlers/keyboard.test.ts`, `bun run typecheck`, and `bun run build`. Split playlist-browser UI row rendering into `src/renderer/modals/playlist-browser-renderer.ts` and left `src/renderer/modals/playlist-browser.ts` as the controller/wiring layer. Moved playlist-browser IPC/runtime wiring into `src/main/runtime/playlist-browser-ipc.ts` and collapsed the `src/main.ts` registration block to use that helper. Verification after refactor: `bun run typecheck`, `bun run build`, `bun test src/main/runtime/playlist-browser-runtime.test.ts src/renderer/modals/playlist-browser.test.ts src/renderer/handlers/keyboard.test.ts`. + +2026-03-30 PR #37 unresolved CodeRabbit threads currently reduce to three likely-actionable items plus one outdated renderer dblclick thread to verify against HEAD before touching code. + +2026-03-30 Addressed latest unresolved CodeRabbit items on PR #37: preserved playlist-browser selection across mutation snapshots, taught nowMs() to honor string-backed test clocks so it stays aligned with currentDbTimestamp(), and normalized maintenance test timestamp fixtures to toDbTimestamp(). The older playlist-browser dblclick thread remains unresolved in GitHub state but current HEAD already contains that fix in playlist-browser-renderer.ts. diff --git a/src/core/services/immersion-tracker/maintenance.test.ts b/src/core/services/immersion-tracker/maintenance.test.ts index 8f68700c..ab9af881 100644 --- a/src/core/services/immersion-tracker/maintenance.test.ts +++ b/src/core/services/immersion-tracker/maintenance.test.ts @@ -95,22 +95,22 @@ test('pruneRawRetention skips disabled retention windows', () => { INSERT INTO imm_videos ( video_id, video_key, canonical_title, source_type, duration_ms, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - 1, 'local:/tmp/video.mkv', 'Video', 1, 0, '${nowMs}', '${nowMs}' + 1, 'local:/tmp/video.mkv', 'Video', 1, 0, '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); INSERT INTO imm_sessions ( session_id, session_uuid, video_id, started_at_ms, ended_at_ms, status, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - 1, 'session-1', 1, '${nowMs - 1_000}', '${nowMs - 500}', 2, '${nowMs}', '${nowMs}' + 1, 'session-1', 1, '${toDbTimestamp(nowMs - 1_000)}', '${toDbTimestamp(nowMs - 500)}', 2, '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); INSERT INTO imm_session_telemetry ( session_id, sample_ms, total_watched_ms, active_watched_ms, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - 1, '${nowMs - 2_000}', 0, 0, '${nowMs}', '${nowMs}' + 1, '${toDbTimestamp(nowMs - 2_000)}', 0, 0, '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); INSERT INTO imm_session_events ( session_id, event_type, ts_ms, payload_json, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - 1, 1, '${nowMs - 3_000}', '{}', '${nowMs}', '${nowMs}' + 1, 1, '${toDbTimestamp(nowMs - 3_000)}', '{}', '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); `); @@ -162,17 +162,17 @@ test('raw retention keeps rollups and rollup retention prunes them separately', INSERT INTO imm_videos ( video_id, video_key, canonical_title, source_type, duration_ms, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - 1, 'local:/tmp/video.mkv', 'Video', 1, 0, '${nowMs}', '${nowMs}' + 1, 'local:/tmp/video.mkv', 'Video', 1, 0, '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); INSERT INTO imm_sessions ( session_id, session_uuid, video_id, started_at_ms, ended_at_ms, status, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - 1, 'session-1', 1, '${nowMs - 200_000_000}', '${nowMs - 199_999_000}', 2, '${nowMs}', '${nowMs}' + 1, 'session-1', 1, '${toDbTimestamp(nowMs - 200_000_000)}', '${toDbTimestamp(nowMs - 199_999_000)}', 2, '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); INSERT INTO imm_session_telemetry ( session_id, sample_ms, total_watched_ms, active_watched_ms, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - 1, '${nowMs - 200_000_000}', 0, 0, '${nowMs}', '${nowMs}' + 1, '${toDbTimestamp(nowMs - 200_000_000)}', 0, 0, '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); INSERT INTO imm_daily_rollups ( rollup_day, video_id, total_sessions, total_active_min, total_lines_seen, @@ -184,7 +184,7 @@ test('raw retention keeps rollups and rollup retention prunes them separately', rollup_month, video_id, total_sessions, total_active_min, total_lines_seen, total_tokens_seen, total_cards, CREATED_DATE, LAST_UPDATE_DATE ) VALUES ( - ${oldMonth}, 1, 1, 10, 1, 1, 1, '${nowMs}', '${nowMs}' + ${oldMonth}, 1, 1, 10, 1, 1, 1, '${toDbTimestamp(nowMs)}', '${toDbTimestamp(nowMs)}' ); `); diff --git a/src/core/services/immersion-tracker/time.test.ts b/src/core/services/immersion-tracker/time.test.ts index 08c5f54a..53951df2 100644 --- a/src/core/services/immersion-tracker/time.test.ts +++ b/src/core/services/immersion-tracker/time.test.ts @@ -5,3 +5,14 @@ import { nowMs } from './time.js'; test('nowMs returns wall-clock epoch milliseconds', () => { assert.ok(nowMs() > 1_600_000_000_000); }); + +test('nowMs honors string-backed test clock values', () => { + const previousNowMs = globalThis.__subminerTestNowMs; + globalThis.__subminerTestNowMs = '1700000000123.9'; + + try { + assert.equal(nowMs(), 1_700_000_000_123); + } finally { + globalThis.__subminerTestNowMs = previousNowMs; + } +}); diff --git a/src/core/services/immersion-tracker/time.ts b/src/core/services/immersion-tracker/time.ts index 11205c83..6323de39 100644 --- a/src/core/services/immersion-tracker/time.ts +++ b/src/core/services/immersion-tracker/time.ts @@ -2,11 +2,24 @@ declare global { var __subminerTestNowMs: number | string | undefined; } -export function nowMs(): number { - const testNowMs = globalThis.__subminerTestNowMs; +function getMockNowMs(testNowMs: number | string | undefined): number | null { if (typeof testNowMs === 'number' && Number.isFinite(testNowMs)) { return Math.floor(testNowMs); } + if (typeof testNowMs === 'string') { + const parsed = Number(testNowMs.trim()); + if (Number.isFinite(parsed)) { + return Math.trunc(parsed); + } + } + return null; +} + +export function nowMs(): number { + const mockedNowMs = getMockNowMs(globalThis.__subminerTestNowMs); + if (mockedNowMs !== null) { + return mockedNowMs; + } const perf = globalThis.performance; if (perf && Number.isFinite(perf.timeOrigin)) { diff --git a/src/renderer/modals/playlist-browser.test.ts b/src/renderer/modals/playlist-browser.test.ts index 7941559f..c41524df 100644 --- a/src/renderer/modals/playlist-browser.test.ts +++ b/src/renderer/modals/playlist-browser.test.ts @@ -141,6 +141,68 @@ function createSnapshot(): PlaylistBrowserSnapshot { }; } +function createMutationSnapshot(): PlaylistBrowserSnapshot { + return { + directoryPath: '/tmp/show', + directoryAvailable: true, + directoryStatus: '/tmp/show', + currentFilePath: '/tmp/show/Show - S01E02.mkv', + playingIndex: 0, + directoryItems: [ + { + path: '/tmp/show/Show - S01E01.mkv', + basename: 'Show - S01E01.mkv', + episodeLabel: 'S1E1', + isCurrentFile: false, + }, + { + path: '/tmp/show/Show - S01E02.mkv', + basename: 'Show - S01E02.mkv', + episodeLabel: 'S1E2', + isCurrentFile: true, + }, + { + path: '/tmp/show/Show - S01E03.mkv', + basename: 'Show - S01E03.mkv', + episodeLabel: 'S1E3', + isCurrentFile: false, + }, + ], + playlistItems: [ + { + index: 1, + id: 2, + filename: '/tmp/show/Show - S01E02.mkv', + title: 'Episode 2', + displayLabel: 'Episode 2', + current: true, + playing: true, + path: '/tmp/show/Show - S01E02.mkv', + }, + { + index: 2, + id: 3, + filename: '/tmp/show/Show - S01E03.mkv', + title: 'Episode 3', + displayLabel: 'Episode 3', + current: false, + playing: false, + path: '/tmp/show/Show - S01E03.mkv', + }, + { + index: 0, + id: 1, + filename: '/tmp/show/Show - S01E01.mkv', + title: 'Episode 1', + displayLabel: 'Episode 1', + current: false, + playing: false, + path: '/tmp/show/Show - S01E01.mkv', + }, + ], + }; +} + test('playlist browser modal opens with playlist-focused current item selection', async () => { const globals = globalThis as typeof globalThis & { window?: unknown; document?: unknown }; const previousWindow = globals.window; @@ -298,6 +360,111 @@ test('playlist browser modal action buttons stop double-click propagation', asyn } }); +test('playlist browser preserves prior selection across mutation snapshots', async () => { + const globals = globalThis as typeof globalThis & { window?: unknown; document?: unknown }; + const previousWindow = globals.window; + const previousDocument = globals.document; + + Object.defineProperty(globalThis, 'window', { + configurable: true, + value: { + electronAPI: { + getPlaylistBrowserSnapshot: async () => ({ + ...createSnapshot(), + directoryItems: [ + ...createSnapshot().directoryItems, + { + path: '/tmp/show/Show - S01E03.mkv', + basename: 'Show - S01E03.mkv', + episodeLabel: 'S1E3', + isCurrentFile: false, + }, + ], + playlistItems: [ + ...createSnapshot().playlistItems, + { + index: 2, + id: 3, + filename: '/tmp/show/Show - S01E03.mkv', + title: 'Episode 3', + displayLabel: 'Episode 3', + current: false, + playing: false, + path: '/tmp/show/Show - S01E03.mkv', + }, + ], + }), + notifyOverlayModalOpened: () => {}, + notifyOverlayModalClosed: () => {}, + focusMainWindow: async () => {}, + setIgnoreMouseEvents: () => {}, + appendPlaylistBrowserFile: async () => ({ + ok: true, + message: 'Queued file', + snapshot: createMutationSnapshot(), + }), + playPlaylistBrowserIndex: async () => ({ ok: true, message: 'ok', snapshot: createSnapshot() }), + removePlaylistBrowserIndex: async () => ({ ok: true, message: 'ok', snapshot: createSnapshot() }), + movePlaylistBrowserIndex: async () => ({ ok: true, message: 'ok', snapshot: createSnapshot() }), + } as unknown as ElectronAPI, + focus: () => {}, + }, + }); + Object.defineProperty(globalThis, 'document', { + configurable: true, + value: { + createElement: () => createPlaylistRow(), + }, + }); + + try { + const state = createRendererState(); + const ctx = { + state, + platform: { + shouldToggleMouseIgnore: false, + }, + dom: { + overlay: { + classList: createClassList(), + focus: () => {}, + }, + playlistBrowserModal: createFakeElement(), + playlistBrowserTitle: createFakeElement(), + playlistBrowserStatus: createFakeElement(), + playlistBrowserDirectoryList: createListStub(), + playlistBrowserPlaylistList: createListStub(), + playlistBrowserClose: createFakeElement(), + }, + }; + + const modal = createPlaylistBrowserModal(ctx as never, { + modalStateReader: { isAnyModalOpen: () => false }, + syncSettingsModalSubtitleSuppression: () => {}, + }); + + await modal.openPlaylistBrowserModal(); + state.playlistBrowserActivePane = 'directory'; + state.playlistBrowserSelectedDirectoryIndex = 2; + state.playlistBrowserSelectedPlaylistIndex = 0; + + await modal.handlePlaylistBrowserKeydown({ + key: 'Enter', + code: 'Enter', + preventDefault: () => {}, + ctrlKey: false, + metaKey: false, + shiftKey: false, + } as never); + + assert.equal(state.playlistBrowserSelectedDirectoryIndex, 2); + assert.equal(state.playlistBrowserSelectedPlaylistIndex, 2); + } finally { + Object.defineProperty(globalThis, 'window', { configurable: true, value: previousWindow }); + Object.defineProperty(globalThis, 'document', { configurable: true, value: previousDocument }); + } +}); + test('playlist browser modal keydown routes append, remove, reorder, tab switch, and play', async () => { const globals = globalThis as typeof globalThis & { window?: unknown; document?: unknown }; const previousWindow = globals.window; diff --git a/src/renderer/modals/playlist-browser.ts b/src/renderer/modals/playlist-browser.ts index 6c002541..0fc3a81b 100644 --- a/src/renderer/modals/playlist-browser.ts +++ b/src/renderer/modals/playlist-browser.ts @@ -1,4 +1,5 @@ import type { + PlaylistBrowserDirectoryItem, PlaylistBrowserMutationResult, PlaylistBrowserQueueItem, PlaylistBrowserSnapshot, @@ -23,6 +24,68 @@ function buildDefaultStatus(snapshot: PlaylistBrowserSnapshot): string { return `${directoryCount} sibling videos ยท ${playlistCount} queued`; } +function getDefaultDirectorySelectionIndex(snapshot: PlaylistBrowserSnapshot): number { + const directoryIndex = snapshot.directoryItems.findIndex((item) => item.isCurrentFile); + return clampIndex(directoryIndex >= 0 ? directoryIndex : 0, snapshot.directoryItems.length); +} + +function getDefaultPlaylistSelectionIndex(snapshot: PlaylistBrowserSnapshot): number { + const playlistIndex = + snapshot.playingIndex ?? snapshot.playlistItems.findIndex((item) => item.current || item.playing); + return clampIndex(playlistIndex >= 0 ? playlistIndex : 0, snapshot.playlistItems.length); +} + +function resolvePreservedIndex( + previousIndex: number, + previousItems: T[], + nextItems: T[], + matchIndex: (previousItem: T) => number, +): number { + if (nextItems.length <= 0) return 0; + if (previousItems.length <= 0) return clampIndex(previousIndex, nextItems.length); + + const normalizedPreviousIndex = clampIndex(previousIndex, previousItems.length); + const previousItem = previousItems[normalizedPreviousIndex]; + const matchedIndex = previousItem ? matchIndex(previousItem) : -1; + return clampIndex(matchedIndex >= 0 ? matchedIndex : normalizedPreviousIndex, nextItems.length); +} + +function resolveDirectorySelectionIndex( + snapshot: PlaylistBrowserSnapshot, + previousSnapshot: PlaylistBrowserSnapshot, + previousIndex: number, +): number { + return resolvePreservedIndex( + previousIndex, + previousSnapshot.directoryItems, + snapshot.directoryItems, + (previousItem: PlaylistBrowserDirectoryItem) => + snapshot.directoryItems.findIndex((item) => item.path === previousItem.path), + ); +} + +function resolvePlaylistSelectionIndex( + snapshot: PlaylistBrowserSnapshot, + previousSnapshot: PlaylistBrowserSnapshot, + previousIndex: number, +): number { + return resolvePreservedIndex( + previousIndex, + previousSnapshot.playlistItems, + snapshot.playlistItems, + (previousItem: PlaylistBrowserQueueItem) => { + if (previousItem.id !== null) { + const byId = snapshot.playlistItems.findIndex((item) => item.id === previousItem.id); + if (byId >= 0) return byId; + } + if (previousItem.path) { + return snapshot.playlistItems.findIndex((item) => item.path === previousItem.path); + } + return -1; + }, + ); +} + export function createPlaylistBrowserModal( ctx: RendererContext, options: { @@ -52,17 +115,25 @@ export function createPlaylistBrowserModal( ctx.dom.playlistBrowserStatus.classList.remove('error'); } - function syncSelection(snapshot: PlaylistBrowserSnapshot): void { - const directoryIndex = snapshot.directoryItems.findIndex((item) => item.isCurrentFile); - const playlistIndex = - snapshot.playingIndex ?? snapshot.playlistItems.findIndex((item) => item.current || item.playing); - ctx.state.playlistBrowserSelectedDirectoryIndex = clampIndex( - directoryIndex >= 0 ? directoryIndex : 0, - snapshot.directoryItems.length, + function syncSelection( + snapshot: PlaylistBrowserSnapshot, + previousSnapshot: PlaylistBrowserSnapshot | null, + ): void { + if (!previousSnapshot) { + ctx.state.playlistBrowserSelectedDirectoryIndex = getDefaultDirectorySelectionIndex(snapshot); + ctx.state.playlistBrowserSelectedPlaylistIndex = getDefaultPlaylistSelectionIndex(snapshot); + return; + } + + ctx.state.playlistBrowserSelectedDirectoryIndex = resolveDirectorySelectionIndex( + snapshot, + previousSnapshot, + ctx.state.playlistBrowserSelectedDirectoryIndex, ); - ctx.state.playlistBrowserSelectedPlaylistIndex = clampIndex( - playlistIndex >= 0 ? playlistIndex : 0, - snapshot.playlistItems.length, + ctx.state.playlistBrowserSelectedPlaylistIndex = resolvePlaylistSelectionIndex( + snapshot, + previousSnapshot, + ctx.state.playlistBrowserSelectedPlaylistIndex, ); } @@ -102,8 +173,9 @@ export function createPlaylistBrowserModal( } function applySnapshot(snapshot: PlaylistBrowserSnapshot): void { + const previousSnapshot = ctx.state.playlistBrowserSnapshot; ctx.state.playlistBrowserSnapshot = snapshot; - syncSelection(snapshot); + syncSelection(snapshot, previousSnapshot); render(); }