mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-05-13 08:12:54 -07:00
fix: address PR-57 CodeRabbit findings and CI failures
- use filtered word counts in media detail session token aggregation - cancel fullscreen refresh burst on exit via updateLinuxMpvFullscreenOverlayRefreshBurst - guard Hyprland JSON.parse in try/catch; exclude windowtitle from geometry events - narrow focus suppression from :focus to :focus-visible - apply JLPT lock selectors to word-name-match tokens (N1–N5)
This commit is contained in:
+40
@@ -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
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
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.
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [ ] #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.
|
||||
<!-- AC:END -->
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
<!-- SECTION:PLAN:BEGIN -->
|
||||
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.
|
||||
<!-- SECTION:PLAN:END -->
|
||||
@@ -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
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
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`.
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [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.
|
||||
<!-- AC:END -->
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
<!-- SECTION:PLAN:BEGIN -->
|
||||
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.
|
||||
<!-- SECTION:PLAN:END -->
|
||||
|
||||
## Implementation Notes
|
||||
|
||||
<!-- SECTION:NOTES:BEGIN -->
|
||||
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.
|
||||
<!-- SECTION:NOTES:END -->
|
||||
|
||||
## Final Summary
|
||||
|
||||
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||
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.
|
||||
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||
@@ -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();
|
||||
|
||||
@@ -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
|
||||
|
||||
+8
-5
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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 };
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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',
|
||||
]) {
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
|
||||
@@ -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' ||
|
||||
|
||||
Reference in New Issue
Block a user