From 27be0e6fd7c52ed1d57a959ef0e46f0273593b52 Mon Sep 17 00:00:00 2001 From: sudacode Date: Mon, 11 May 2026 23:48:32 -0700 Subject: [PATCH] fix: address coderabbit subtitle follow-ups --- ...PR-57-latest-CodeRabbit-review-comments.md | 70 +++++++++++++++++++ src/core/services/mpv.test.ts | 17 +++++ src/core/services/mpv.ts | 2 +- src/core/services/stats-window.ts | 4 +- .../tokenizer/parser-selection-stage.test.ts | 32 +++++++++ .../tokenizer/parser-selection-stage.ts | 2 +- src/renderer/style.css | 6 +- 7 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 backlog/tasks/task-345 - Address-PR-57-latest-CodeRabbit-review-comments.md diff --git a/backlog/tasks/task-345 - Address-PR-57-latest-CodeRabbit-review-comments.md b/backlog/tasks/task-345 - Address-PR-57-latest-CodeRabbit-review-comments.md new file mode 100644 index 00000000..8a479553 --- /dev/null +++ b/backlog/tasks/task-345 - Address-PR-57-latest-CodeRabbit-review-comments.md @@ -0,0 +1,70 @@ +--- +id: TASK-345 +title: Address PR 57 latest CodeRabbit review comments +status: Done +assignee: + - codex +created_date: '2026-05-12 06:35' +updated_date: '2026-05-12 06:38' +labels: + - pr-review + - coderabbit +dependencies: [] +references: + - 'https://github.com/ksyasuda/SubMiner/pull/57' +priority: medium +--- + +## Description + + +Assess the 2026-05-11 CodeRabbit review on PR #57 and address still-valid actionable comments with minimal changes. Current comments cover mpv subtitle playback unpause behavior, parser-selection empty reading handling, and overlay focus selector accessibility. + + +## Acceptance Criteria + +- [x] #1 Still-valid CodeRabbit comments from the latest PR #57 review are fixed or explicitly documented as skipped with rationale. +- [x] #2 Regression coverage is added for behavior-affecting fixes where practical before production code changes. +- [x] #3 Relevant targeted checks pass locally. + + +## Implementation Plan + + +1. Inspect current code and nearby tests for the three latest CodeRabbit comments on PR #57. +2. Add regression tests first for behavior-impacting findings: mpv playNextSubtitle unpauses when pause state is unknown, and parser selection preserves combined reading for empty segment readings. +3. Apply minimal production fixes plus the CSS :focus-visible selector change. +4. Run targeted test commands for touched areas and update task notes/final status. + + +## Implementation Notes + + +Added red tests for the two behavior comments. `bun test src/core/services/mpv.test.ts` fails because playNextSubtitle skips unpause when pause state is null. `bun test src/core/services/tokenizer/parser-selection-stage.test.ts` fails because empty grammar-ending reading clears preceding combined reading. + +Implemented all three latest CodeRabbit findings. Added regressions for mpv unknown pause state and parser-selection empty reading handling; changed overlay focus selectors to :focus-visible. Also fixed existing Prettier failure in src/core/services/stats-window.ts. Verification passed: targeted tests, typecheck, test:fast, test:env, format:check:src, build, test:smoke:dist, changelog:lint, changelog:pr-check. + + +## Final Summary + + +Addressed the latest CodeRabbit comments on PR #57. + +Changes: +- `playNextSubtitle` now sends `pause=false` unless mpv is explicitly known to be playing, covering startup/reconnect unknown pause state. +- Parser selection no longer slices combined readings for empty grammar-ending readings, preserving preceding token readings. +- Overlay focus suppression now targets `:focus-visible` selectors. +- Applied Prettier to `src/core/services/stats-window.ts` to clear the existing formatting gate failure. + +Verification: +- `bun test src/core/services/mpv.test.ts` +- `bun test src/core/services/tokenizer/parser-selection-stage.test.ts` +- `bun run typecheck` +- `bun run test:fast` +- `bun run test:env` +- `bun run format:check:src` +- `bun run build` +- `bun run test:smoke:dist` +- `bun run changelog:lint` +- `bun run changelog:pr-check` + diff --git a/src/core/services/mpv.test.ts b/src/core/services/mpv.test.ts index f318f5f9..8b949a14 100644 --- a/src/core/services/mpv.test.ts +++ b/src/core/services/mpv.test.ts @@ -515,6 +515,23 @@ test('MpvIpcClient playNextSubtitle starts playback from paused state and auto-p ]); }); +test('MpvIpcClient playNextSubtitle starts playback when pause state is unknown', () => { + const commands: unknown[] = []; + const client = new MpvIpcClient('/tmp/mpv.sock', makeDeps()); + (client as any).send = (payload: unknown) => { + commands.push(payload); + return true; + }; + + client.playNextSubtitle(); + + assert.equal((client as any).pendingPauseAtSubEnd, true); + assert.deepEqual(commands, [ + { command: ['sub-seek', 1] }, + { command: ['set_property', 'pause', false] }, + ]); +}); + test('MpvIpcClient playNextSubtitle still auto-pauses at end while already playing', async () => { const commands: unknown[] = []; const client = new MpvIpcClient('/tmp/mpv.sock', makeDeps()); diff --git a/src/core/services/mpv.ts b/src/core/services/mpv.ts index 25c1e73c..9525c43a 100644 --- a/src/core/services/mpv.ts +++ b/src/core/services/mpv.ts @@ -525,7 +525,7 @@ export class MpvIpcClient implements MpvClient { this.pendingPauseAtSubEnd = true; this.pauseAtTime = null; this.send({ command: ['sub-seek', 1] }); - if (this.playbackPaused === true) { + if (this.playbackPaused !== false) { this.send({ command: ['set_property', 'pause', false] }); } } diff --git a/src/core/services/stats-window.ts b/src/core/services/stats-window.ts index 85904129..83aade76 100644 --- a/src/core/services/stats-window.ts +++ b/src/core/services/stats-window.ts @@ -51,7 +51,9 @@ function showStatsWindow(window: BrowserWindow, options: StatsWindowOptions): vo promoteStatsWindowLevel(window); window.show(); placementBounds = syncStatsWindowBounds(window, bounds) ?? placementBounds; - if (!ensureHyprlandWindowFloatingByTitle({ title: STATS_WINDOW_TITLE, bounds: placementBounds })) { + if ( + !ensureHyprlandWindowFloatingByTitle({ title: STATS_WINDOW_TITLE, bounds: placementBounds }) + ) { placementBounds = syncStatsWindowBounds(window, bounds) ?? placementBounds; } window.focus(); diff --git a/src/core/services/tokenizer/parser-selection-stage.test.ts b/src/core/services/tokenizer/parser-selection-stage.test.ts index 59f9c5e1..5a58afa3 100644 --- a/src/core/services/tokenizer/parser-selection-stage.test.ts +++ b/src/core/services/tokenizer/parser-selection-stage.test.ts @@ -187,6 +187,38 @@ test('splits trailing grammar endings when later segments are standalone words', ); }); +test('keeps preceding reading when standalone grammar ending has empty reading', () => { + const parseResults = [ + makeParseItem('scanning-parser', [ + [ + { text: '猫', reading: 'ねこ', headword: '猫' }, + { text: 'です', reading: '', headword: 'です' }, + ], + ]), + ]; + + const tokens = selectYomitanParseTokens(parseResults, () => false, 'headword'); + assert.deepEqual( + tokens?.map((token) => ({ + surface: token.surface, + reading: token.reading, + headword: token.headword, + })), + [ + { + surface: '猫', + reading: 'ねこ', + headword: '猫', + }, + { + surface: 'です', + reading: '', + headword: 'です', + }, + ], + ); +}); + test('splits trailing ja-nai grammar endings from preceding content', () => { const parseResults = [ makeParseItem('scanning-parser', [ diff --git a/src/core/services/tokenizer/parser-selection-stage.ts b/src/core/services/tokenizer/parser-selection-stage.ts index f51b8fb1..1f0e9eda 100644 --- a/src/core/services/tokenizer/parser-selection-stage.ts +++ b/src/core/services/tokenizer/parser-selection-stage.ts @@ -270,7 +270,7 @@ export function mapYomitanParseResultItemToMergedTokens( const segmentHeadword = extractYomitanHeadword(segment); if (isStandaloneGrammarEndingSegment(segment)) { combinedSurface = combinedSurface.slice(0, -segmentText.length); - if (typeof segment.reading === 'string') { + if (typeof segment.reading === 'string' && segment.reading.length > 0) { combinedReading = combinedReading.slice(0, -segment.reading.length); } flushCombinedToken(segmentStart); diff --git a/src/renderer/style.css b/src/renderer/style.css index 050e1cd8..5248ebc6 100644 --- a/src/renderer/style.css +++ b/src/renderer/style.css @@ -40,9 +40,9 @@ body { 'Hiragino Kaku Gothic ProN', 'Yu Gothic', 'Arial Unicode MS', Arial, sans-serif; } -html:focus, -body:focus, -#overlay:focus { +html:focus-visible, +body:focus-visible, +#overlay:focus-visible { outline: none; }