diff --git a/backlog/tasks/task-347 - Address-PR-57-CodeRabbit-review-round-after-stats-session-fix.md b/backlog/tasks/task-347 - Address-PR-57-CodeRabbit-review-round-after-stats-session-fix.md new file mode 100644 index 00000000..a88e6981 --- /dev/null +++ b/backlog/tasks/task-347 - Address-PR-57-CodeRabbit-review-round-after-stats-session-fix.md @@ -0,0 +1,40 @@ +--- +id: TASK-347 +title: Address PR 57 CodeRabbit review round after stats session fix +status: In Progress +assignee: + - codex +created_date: '2026-05-12 07:02' +updated_date: '2026-05-12 07:02' +labels: + - pr-review + - coderabbit + - ci +dependencies: [] +references: + - 'https://github.com/ksyasuda/SubMiner/pull/57' +priority: high +--- + +## Description + + +Assess and address the 2026-05-12 CodeRabbit review on PR #57 plus the current red GitHub Actions check. Latest comments cover stats session detail token aggregation, Linux fullscreen overlay refresh scheduling, Hyprland title-event polling, malformed Hyprland monitor JSON handling, and JLPT-lock test coverage for name matches. + + +## Acceptance Criteria + +- [ ] #1 Still-valid latest CodeRabbit findings on PR #57 are fixed or documented as skipped with rationale. +- [ ] #2 CI failure context is inspected and any repo-relevant failing tests or formatting issues are fixed. +- [ ] #3 Regression coverage is added for behavior changes where practical before production edits. +- [ ] #4 Relevant local verification passes. + + +## Implementation Plan + + +1. Inspect failing GitHub Actions log and current code around each latest CodeRabbit finding. +2. Add or update focused regression tests first for behavior changes: stats token aggregation, fullscreen refresh exit cancellation, Hyprland monitor parse failure, and title-only event filtering. +3. Apply minimal production fixes for still-valid findings, plus the subtitle-render duplicate test coverage item. +4. Run targeted tests first, then format/typecheck and broader relevant gates; update the task with results. + diff --git a/backlog/tasks/task-348 - Fix-PR-57-coverage-CI-focus-chrome-failure.md b/backlog/tasks/task-348 - Fix-PR-57-coverage-CI-focus-chrome-failure.md new file mode 100644 index 00000000..3b2d770c --- /dev/null +++ b/backlog/tasks/task-348 - Fix-PR-57-coverage-CI-focus-chrome-failure.md @@ -0,0 +1,56 @@ +--- +id: TASK-348 +title: Fix PR 57 coverage CI focus chrome failure +status: Done +assignee: + - '@codex' +created_date: '2026-05-12 07:02' +updated_date: '2026-05-12 07:11' +labels: + - ci + - bug +dependencies: [] +references: + - 'https://github.com/ksyasuda/SubMiner/pull/57' + - 'https://github.com/ksyasuda/SubMiner/actions/runs/25718536412' +priority: high +--- + +## Description + + +Investigate and fix current GitHub Actions `build-test-audit` failure on PR #57 (`tokenizer-updates`). CI fails during `bun run test:coverage:src` in the maintained source lane: `renderer stylesheet hides focus chrome on top-level overlay focus targets`. + + +## Acceptance Criteria + +- [x] #1 Root cause of the focus chrome coverage failure is identified from CI/local test output. +- [x] #2 A focused fix is applied without broad unrelated changes. +- [x] #3 Relevant local coverage/test command passes. +- [x] #4 Remote PR check status is rechecked or next CI action is documented. + + +## Implementation Plan + + +1. Reproduce the CI failure locally with `bun test src/renderer/overlay-legacy-cleanup.test.ts`. +2. Update the stale legacy cleanup assertion to expect top-level `:focus-visible` suppression and reject broad `:focus` suppression. +3. Run the targeted test and `bun run test:coverage:src` to match CI's failing lane. +4. Recheck PR checks or document that CI needs a push/rerun. + + +## Implementation Notes + + +CI/local root cause: `src/renderer/style.css` was intentionally changed to `html/body/#overlay:focus-visible`, but `src/renderer/overlay-legacy-cleanup.test.ts` still required broad `:focus` selectors. The stale assertion fails in `test:coverage:src`. + +Additional coverage-lane failure after first fix: `src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts` imported `updateLinuxMpvFullscreenOverlayRefreshBurst`, but `src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts` did not export/implement it. Added the helper to cancel existing bursts and schedule only while fullscreen is true. + +Verification passed: `bun test src/renderer/overlay-legacy-cleanup.test.ts`; `bun test src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts`; `bun run test:coverage:src`; `bun run format:check:src`. `gh pr checks 57` still reports the old failed `build-test-audit` run at run 25718536412; branch needs push/rerun for remote green. + + +## Final Summary + + +Fixed current PR #57 `build-test-audit` CI blockers. Updated the stale overlay legacy cleanup assertion to expect `:focus-visible` top-level focus suppression and guard against reintroducing broad `:focus` suppression. Added the missing `updateLinuxMpvFullscreenOverlayRefreshBurst` export used by the Linux fullscreen overlay refresh tests. Verification passed locally: focused overlay legacy cleanup test, focused Linux fullscreen refresh test, `bun run test:coverage:src`, and `bun run format:check:src`. Remote PR checks still show the old failed `build-test-audit` run until these local changes are pushed and CI reruns. + diff --git a/src/core/services/immersion-tracker/__tests__/query.test.ts b/src/core/services/immersion-tracker/__tests__/query.test.ts index 72cf0f27..c45828ce 100644 --- a/src/core/services/immersion-tracker/__tests__/query.test.ts +++ b/src/core/services/immersion-tracker/__tests__/query.test.ts @@ -3072,6 +3072,12 @@ test('media detail resolves retained sessions before lifetime summary exists', ( WHERE session_id = ? `, ).run(startedAtMs + 600_000, 600_000, 100, 990, 1, sessionId); + insertFilteredWordOccurrence(db, { + sessionId, + videoId, + occurrenceCount: 4, + startedAtMs, + }); assert.equal(getSessionSummaries(db, 1)[0]?.videoId, videoId); assert.equal( @@ -3089,7 +3095,7 @@ test('media detail resolves retained sessions before lifetime summary exists', ( assert.equal(detail.totalSessions, 1); assert.equal(detail.totalActiveMs, 600_000); assert.equal(detail.totalLinesSeen, 100); - assert.equal(detail.totalTokensSeen, 990); + assert.equal(detail.totalTokensSeen, 4); assert.equal(detail.totalCards, 1); } finally { db.close(); diff --git a/src/core/services/immersion-tracker/query-library.ts b/src/core/services/immersion-tracker/query-library.ts index 89a4f13e..69240462 100644 --- a/src/core/services/immersion-tracker/query-library.ts +++ b/src/core/services/immersion-tracker/query-library.ts @@ -243,6 +243,7 @@ export function getMediaLibrary(db: DatabaseSync): MediaLibraryRow[] { } export function getMediaDetail(db: DatabaseSync, videoId: number): MediaDetailRow | null { + const wordsExpr = sessionDisplayWordsExpr('s', 'swc', 'COALESCE(asm.tokensSeen, s.tokens_seen)'); return db .prepare( ` @@ -265,7 +266,7 @@ export function getMediaDetail(db: DatabaseSync, videoId: number): MediaDetailRo END AS totalCards, CASE WHEN lm.video_id IS NOT NULL THEN COALESCE(lm.total_tokens_seen, 0) - ELSE COALESCE(SUM(COALESCE(asm.tokensSeen, s.tokens_seen, 0)), 0) + ELSE COALESCE(SUM(${wordsExpr}), 0) END AS totalTokensSeen, CASE WHEN lm.video_id IS NOT NULL THEN COALESCE(lm.total_lines_seen, 0) @@ -290,6 +291,7 @@ export function getMediaDetail(db: DatabaseSync, videoId: number): MediaDetailRo LEFT JOIN imm_youtube_videos yv ON yv.video_id = v.video_id LEFT JOIN imm_sessions s ON s.video_id = v.video_id LEFT JOIN active_session_metrics asm ON asm.sessionId = s.session_id + LEFT JOIN session_word_counts swc ON swc.sessionId = s.session_id WHERE v.video_id = ? AND (lm.video_id IS NOT NULL OR s.session_id IS NOT NULL) GROUP BY v.video_id diff --git a/src/main.ts b/src/main.ts index 9ce74aa4..83ebc81e 100644 --- a/src/main.ts +++ b/src/main.ts @@ -36,7 +36,7 @@ import { createDiscordRpcClient } from './main/runtime/discord-rpc-client.js'; import { type CancelLinuxMpvFullscreenOverlayRefreshBurst, clearLinuxMpvFullscreenOverlayRefreshTimeouts, - scheduleLinuxVisibleOverlayFullscreenRefreshBurst, + updateLinuxMpvFullscreenOverlayRefreshBurst, } from './main/runtime/linux-mpv-fullscreen-overlay-refresh'; import { mergeAiConfig } from './ai/config'; @@ -3859,16 +3859,19 @@ const { } lastObservedTimePos = time; }, - onFullscreenChange: () => { - cancelLinuxMpvFullscreenOverlayRefreshBurst = - scheduleLinuxVisibleOverlayFullscreenRefreshBurst({ + onFullscreenChange: (fullscreen) => { + cancelLinuxMpvFullscreenOverlayRefreshBurst = updateLinuxMpvFullscreenOverlayRefreshBurst( + fullscreen, + { overlayManager: { getMainWindow: () => overlayManager.getMainWindow(), getVisibleOverlayVisible: () => overlayManager.getVisibleOverlayVisible(), }, overlayVisibilityRuntime, ensureOverlayWindowLevel: (window) => ensureOverlayWindowLevel(window), - }); + }, + cancelLinuxMpvFullscreenOverlayRefreshBurst, + ); }, onSubtitleTrackChange: (sid) => { scheduleSubtitlePrefetchRefresh(); diff --git a/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts b/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts index a466dce4..c4343b4d 100644 --- a/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts +++ b/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts @@ -2,6 +2,7 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { clearLinuxMpvFullscreenOverlayRefreshTimeouts, + updateLinuxMpvFullscreenOverlayRefreshBurst, scheduleLinuxVisibleOverlayFullscreenRefreshBurst, } from './linux-mpv-fullscreen-overlay-refresh'; @@ -48,3 +49,45 @@ test('linux mpv fullscreen overlay refresh burst schedules overlay refresh work } } }); + +test('linux mpv fullscreen overlay refresh update cancels burst when fullscreen exits', async () => { + const originalPlatformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { + configurable: true, + value: 'linux', + }); + + const calls: string[] = []; + + try { + const deps = { + overlayManager: { + getMainWindow: () => + ({ + hide: () => calls.push('hide'), + isDestroyed: () => false, + isVisible: () => true, + showInactive: () => calls.push('showInactive'), + }) as never, + getVisibleOverlayVisible: () => true, + }, + overlayVisibilityRuntime: { + updateVisibleOverlayVisibility: () => calls.push('updateVisibleOverlayVisibility'), + }, + ensureOverlayWindowLevel: () => calls.push('ensureOverlayWindowLevel'), + }; + + const cancel = updateLinuxMpvFullscreenOverlayRefreshBurst(true, deps, null); + const nextCancel = updateLinuxMpvFullscreenOverlayRefreshBurst(false, deps, cancel); + + await new Promise((resolve) => setTimeout(resolve, 80)); + + assert.equal(nextCancel, null); + assert.deepEqual(calls, []); + } finally { + clearLinuxMpvFullscreenOverlayRefreshTimeouts(); + if (originalPlatformDescriptor) { + Object.defineProperty(process, 'platform', originalPlatformDescriptor); + } + } +}); diff --git a/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts b/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts index a3c5be5b..419faed7 100644 --- a/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts +++ b/src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts @@ -67,4 +67,17 @@ export function scheduleLinuxVisibleOverlayFullscreenRefreshBurst( return clearLinuxMpvFullscreenOverlayRefreshTimeouts; } +export function updateLinuxMpvFullscreenOverlayRefreshBurst( + isFullscreen: boolean, + deps: LinuxMpvFullscreenOverlayRefreshDeps, + cancelCurrentBurst: CancelLinuxMpvFullscreenOverlayRefreshBurst | null, +): CancelLinuxMpvFullscreenOverlayRefreshBurst | null { + cancelCurrentBurst?.(); + if (!isFullscreen) { + return null; + } + + return scheduleLinuxVisibleOverlayFullscreenRefreshBurst(deps); +} + export { clearLinuxMpvFullscreenOverlayRefreshTimeouts }; diff --git a/src/renderer/overlay-legacy-cleanup.test.ts b/src/renderer/overlay-legacy-cleanup.test.ts index 34dff015..4ee0bbb3 100644 --- a/src/renderer/overlay-legacy-cleanup.test.ts +++ b/src/renderer/overlay-legacy-cleanup.test.ts @@ -28,9 +28,16 @@ test('renderer stylesheet no longer contains invisible-layer selectors', () => { assert.doesNotMatch(cssSource, /body\.layer-invisible/); }); -test('renderer stylesheet hides focus chrome on top-level overlay focus targets', () => { +test('renderer stylesheet only hides visible focus chrome on top-level overlay focus targets', () => { const cssSource = readWorkspaceFile('src/renderer/style.css'); - assert.match(cssSource, /html:focus,\s*body:focus,\s*#overlay:focus\s*\{[^}]*outline:\s*none;/s); + assert.match( + cssSource, + /html:focus-visible,\s*body:focus-visible,\s*#overlay:focus-visible\s*\{[^}]*outline:\s*none;/s, + ); + assert.doesNotMatch( + cssSource, + /html:focus,\s*body:focus,\s*#overlay:focus\s*\{[^}]*outline:\s*none;/s, + ); }); test('top-level readme avoids stale overlay-layers wording', () => { diff --git a/src/renderer/style.css b/src/renderer/style.css index 5248ebc6..cf8d3875 100644 --- a/src/renderer/style.css +++ b/src/renderer/style.css @@ -990,6 +990,7 @@ body.settings-modal-open [data-subminer-yomitan-popup-host='true'] { #subtitleRoot .word.word-jlpt-n1.word-known, #subtitleRoot .word.word-jlpt-n1.word-n-plus-one, +#subtitleRoot .word.word-jlpt-n1.word-name-match, #subtitleRoot .word.word-jlpt-n1.word-frequency-single, #subtitleRoot .word.word-jlpt-n1.word-frequency-band-1, #subtitleRoot .word.word-jlpt-n1.word-frequency-band-2, @@ -1006,6 +1007,7 @@ body.settings-modal-open [data-subminer-yomitan-popup-host='true'] { #subtitleRoot .word.word-jlpt-n2.word-known, #subtitleRoot .word.word-jlpt-n2.word-n-plus-one, +#subtitleRoot .word.word-jlpt-n2.word-name-match, #subtitleRoot .word.word-jlpt-n2.word-frequency-single, #subtitleRoot .word.word-jlpt-n2.word-frequency-band-1, #subtitleRoot .word.word-jlpt-n2.word-frequency-band-2, @@ -1022,6 +1024,7 @@ body.settings-modal-open [data-subminer-yomitan-popup-host='true'] { #subtitleRoot .word.word-jlpt-n3.word-known, #subtitleRoot .word.word-jlpt-n3.word-n-plus-one, +#subtitleRoot .word.word-jlpt-n3.word-name-match, #subtitleRoot .word.word-jlpt-n3.word-frequency-single, #subtitleRoot .word.word-jlpt-n3.word-frequency-band-1, #subtitleRoot .word.word-jlpt-n3.word-frequency-band-2, @@ -1038,6 +1041,7 @@ body.settings-modal-open [data-subminer-yomitan-popup-host='true'] { #subtitleRoot .word.word-jlpt-n4.word-known, #subtitleRoot .word.word-jlpt-n4.word-n-plus-one, +#subtitleRoot .word.word-jlpt-n4.word-name-match, #subtitleRoot .word.word-jlpt-n4.word-frequency-single, #subtitleRoot .word.word-jlpt-n4.word-frequency-band-1, #subtitleRoot .word.word-jlpt-n4.word-frequency-band-2, @@ -1054,6 +1058,7 @@ body.settings-modal-open [data-subminer-yomitan-popup-host='true'] { #subtitleRoot .word.word-jlpt-n5.word-known, #subtitleRoot .word.word-jlpt-n5.word-n-plus-one, +#subtitleRoot .word.word-jlpt-n5.word-name-match, #subtitleRoot .word.word-jlpt-n5.word-frequency-single, #subtitleRoot .word.word-jlpt-n5.word-frequency-band-1, #subtitleRoot .word.word-jlpt-n5.word-frequency-band-2, diff --git a/src/renderer/subtitle-render.test.ts b/src/renderer/subtitle-render.test.ts index 49619b68..c008bc8c 100644 --- a/src/renderer/subtitle-render.test.ts +++ b/src/renderer/subtitle-render.test.ts @@ -1105,6 +1105,7 @@ test('subtitle annotation CSS underlines JLPT tokens without changing token colo for (const annotationClass of [ 'word-known', 'word-n-plus-one', + 'word-name-match', 'word-frequency-single', 'word-frequency-band-2', ]) { diff --git a/src/window-trackers/hyprland-tracker.test.ts b/src/window-trackers/hyprland-tracker.test.ts index 2c645b00..6df5e8ce 100644 --- a/src/window-trackers/hyprland-tracker.test.ts +++ b/src/window-trackers/hyprland-tracker.test.ts @@ -3,6 +3,7 @@ import assert from 'node:assert/strict'; import { isHyprlandGeometryEvent, parseHyprctlClients, + parseHyprctlMonitors, resolveHyprlandWindowGeometry, selectHyprlandMpvWindow, type HyprlandClient, @@ -121,9 +122,16 @@ test('parseHyprctlClients tolerates non-json prefix output', () => { ]); }); -test('isHyprlandGeometryEvent treats fullscreenv2 as a geometry-changing event', () => { +test('parseHyprctlMonitors returns null for malformed JSON output', () => { + assert.equal(parseHyprctlMonitors('not-json'), null); + assert.equal(parseHyprctlMonitors('[{"id":0,"x":0,"y":0,"width":1920'), null); +}); + +test('isHyprlandGeometryEvent treats geometry events as geometry-changing only', () => { assert.equal(isHyprlandGeometryEvent('fullscreenv2'), true); assert.equal(isHyprlandGeometryEvent('workspacev2'), true); + assert.equal(isHyprlandGeometryEvent('windowtitle'), false); + assert.equal(isHyprlandGeometryEvent('windowtitlev2'), false); assert.equal(isHyprlandGeometryEvent('activewindowv2'), false); }); diff --git a/src/window-trackers/hyprland-tracker.ts b/src/window-trackers/hyprland-tracker.ts index 290a787d..867b757e 100644 --- a/src/window-trackers/hyprland-tracker.ts +++ b/src/window-trackers/hyprland-tracker.ts @@ -136,7 +136,12 @@ export function parseHyprctlClients(output: string): HyprlandClient[] | null { return null; } - const parsed = JSON.parse(jsonPayload) as unknown; + let parsed: unknown; + try { + parsed = JSON.parse(jsonPayload) as unknown; + } catch { + return null; + } if (!Array.isArray(parsed)) { return null; } @@ -150,7 +155,12 @@ export function parseHyprctlMonitors(output: string): HyprlandMonitor[] | null { return null; } - const parsed = JSON.parse(jsonPayload) as unknown; + let parsed: unknown; + try { + parsed = JSON.parse(jsonPayload) as unknown; + } catch { + return null; + } if (!Array.isArray(parsed)) { return null; } @@ -192,8 +202,6 @@ export function isHyprlandGeometryEvent(name: string): boolean { name === 'movewindowv2' || name === 'resizewindow' || name === 'resizewindowv2' || - name === 'windowtitle' || - name === 'windowtitlev2' || name === 'openwindow' || name === 'closewindow' || name === 'fullscreen' ||