feat(tokenizer): use Yomitan word classes for subtitle POS filtering#57
Open
feat(tokenizer): use Yomitan word classes for subtitle POS filtering#57
Conversation
📝 WalkthroughWalkthroughTokenizer now preserves Yomitan ChangesTokenizer / Subtitle Annotation (single DAG)
MPV fullscreen, Linux overlay refresh & lifecycle (single DAG)
Miscellaneous unrelated DAGs (single cohort)
Possibly related PRs
Suggested labels
✨ Finishing Touches✨ Simplify code
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/services/tokenizer/parser-enrichment-stage.ts (1)
306-334:⚠️ Potential issue | 🟠 MajorTreat blank POS fields as missing during backfill.
The new completeness check treats
''and whitespace-only POS fields as incomplete, but both fill paths still use??. That means tokens withpos2: ''/pos3: ''will never be populated from MeCab and stay permanently half-enriched.Suggested fix
+function coalesceMissingPosField( + current: string | undefined, + fallback: string | undefined, +): string | undefined { + return typeof current === 'string' && current.trim().length > 0 ? current : fallback; +} + function fillMissingPos1BySurfaceSequence( tokens: MergedToken[], byTrimmedSurface: Map<string, IndexedMecabToken[]>, ): MergedToken[] { @@ return { ...token, - pos1: token.pos1 ?? best.pos1, - pos2: token.pos2 ?? best.pos2, - pos3: token.pos3 ?? best.pos3, + pos1: coalesceMissingPosField(token.pos1, best.pos1), + pos2: coalesceMissingPosField(token.pos2, best.pos2), + pos3: coalesceMissingPosField(token.pos3, best.pos3), }; }); } @@ return { ...token, - pos1: token.pos1 ?? metadata.pos1, - pos2: token.pos2 ?? metadata.pos2, - pos3: token.pos3 ?? metadata.pos3, + pos1: coalesceMissingPosField(token.pos1, metadata.pos1), + pos2: coalesceMissingPosField(token.pos2, metadata.pos2), + pos3: coalesceMissingPosField(token.pos3, metadata.pos3), }; });Also applies to: 387-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/parser-enrichment-stage.ts` around lines 306 - 334, The completeness check now treats whitespace-only POS fields as missing, but the backfill still uses nullish coalescing (pos1: token.pos1 ?? best.pos1 etc.), so blank strings never get replaced; change each backfill to test for a non-empty trimmed value (e.g., use token.pos1?.trim() ? token.pos1 : best.pos1) for pos1, pos2, pos3 so whitespace/empty strings are replaced by MeCab values; make the same change in the second analogous block (the other backfill around the lowerBoundByIndex/cursor logic) so both enrichment paths use trimmed emptiness checks instead of ??.
🧹 Nitpick comments (2)
src/core/services/tokenizer/annotation-stage.test.ts (1)
790-799: Optional: simplify this test fixture to only relevant deps.
isKnownWord/getJlptLevelstubs here are unrelated to the assertion and can be removed (or asserted) to keep the test intent tighter.✂️ Suggested simplification
- const result = annotateTokens( - tokens, - makeDeps({ - isKnownWord: (text) => text === 'た' || text === '負', - getJlptLevel: (text) => (text === 'た' || text === '負' ? 'N3' : null), - }), - { - minSentenceWordsForNPlusOne: 1, - }, - ); + const result = annotateTokens(tokens, makeDeps(), { + minSentenceWordsForNPlusOne: 1, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/annotation-stage.test.ts` around lines 790 - 799, The test fixture passes extraneous stubs (isKnownWord/getJlptLevel) into makeDeps when calling annotateTokens; simplify by removing those unused stubs and only providing the minimal dependencies required for the assertion (or replace them with noop/default implementations), keeping the annotateTokens call and the minSentenceWordsForNPlusOne option intact; if you want to ensure they aren't used, add an explicit assertion/mock that verifies isKnownWord/getJlptLevel are not invoked rather than stubbing meaningful behavior.src/core/services/tokenizer/subtitle-annotation-filter.ts (1)
23-26: Optional: deduplicate shared standalone phrase literals.
たって/だってare maintained in multiple constants. Consider deriving both consumers from one source list to reduce drift risk.♻️ Suggested refactor
+const STANDALONE_GRAMMAR_PARTICLE_PHRASES = new Set(['たって', 'だって']); + const SUBTITLE_ANNOTATION_EXCLUDED_TERMS = new Set([ 'あ', 'ああ', 'ええ', 'うう', 'おお', 'くれ', - 'たって', 'って', - 'だって', + ...STANDALONE_GRAMMAR_PARTICLE_PHRASES, 'はあ', 'はは', 'べき', 'へえ', 'ふう', 'ほう', 'もんか', 'ものか', ]); -const STANDALONE_GRAMMAR_PARTICLE_PHRASES = new Set(['たって', 'だって']);Also applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/subtitle-annotation-filter.ts` around lines 23 - 26, Multiple literal standalone phrases like 'たって' and 'だって' are duplicated across constants in subtitle-annotation-filter.ts; create a single shared constant (e.g., SHARED_STANDALONE_PHRASES) containing those common entries and replace the duplicated arrays referenced by the two consumers (the constants/arrays that currently list 'たって'/'だって') so both derive from that one source to avoid drift and ensure future updates stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/tokenizer.ts`:
- Around line 784-789: The code currently returns tokens produced by
parseWithYomitanInternalParser without calling stripSubtitleAnnotationMetadata
when applyAnnotationStage returns early, which lets subtitle-excluded tokens
keep annotation fields; update the no-annotation return path so that before
returning (after const yomitanTokens / when annotatedTokens may be skipped) you
call stripSubtitleAnnotationMetadata(yomitanTokens) and use that filtered array
in the returned object; ensure stripSubtitleAnnotationMetadata is invoked in the
branch that currently returns early from applyAnnotationStage and that the
returned object still uses annotationOptions.sourceText and other existing
fields unchanged.
---
Outside diff comments:
In `@src/core/services/tokenizer/parser-enrichment-stage.ts`:
- Around line 306-334: The completeness check now treats whitespace-only POS
fields as missing, but the backfill still uses nullish coalescing (pos1:
token.pos1 ?? best.pos1 etc.), so blank strings never get replaced; change each
backfill to test for a non-empty trimmed value (e.g., use token.pos1?.trim() ?
token.pos1 : best.pos1) for pos1, pos2, pos3 so whitespace/empty strings are
replaced by MeCab values; make the same change in the second analogous block
(the other backfill around the lowerBoundByIndex/cursor logic) so both
enrichment paths use trimmed emptiness checks instead of ??.
---
Nitpick comments:
In `@src/core/services/tokenizer/annotation-stage.test.ts`:
- Around line 790-799: The test fixture passes extraneous stubs
(isKnownWord/getJlptLevel) into makeDeps when calling annotateTokens; simplify
by removing those unused stubs and only providing the minimal dependencies
required for the assertion (or replace them with noop/default implementations),
keeping the annotateTokens call and the minSentenceWordsForNPlusOne option
intact; if you want to ensure they aren't used, add an explicit assertion/mock
that verifies isKnownWord/getJlptLevel are not invoked rather than stubbing
meaningful behavior.
In `@src/core/services/tokenizer/subtitle-annotation-filter.ts`:
- Around line 23-26: Multiple literal standalone phrases like 'たって' and 'だって'
are duplicated across constants in subtitle-annotation-filter.ts; create a
single shared constant (e.g., SHARED_STANDALONE_PHRASES) containing those common
entries and replace the duplicated arrays referenced by the two consumers (the
constants/arrays that currently list 'たって'/'だって') so both derive from that one
source to avoid drift and ensure future updates stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 26d4b00d-d9bd-41b3-8054-dc6976da93e5
⛔ Files ignored due to path filters (2)
backlog/tasks/task-304 - Fix-N1-sentence-boundary-counting-across-Yomitan-punctuation-gaps.mdis excluded by!backlog/**backlog/tasks/task-305 - Use-Yomitan-word-classes-for-subtitle-token-POS-filtering.mdis excluded by!backlog/**
📒 Files selected for processing (9)
src/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/core/services/tokenizer/yomitan-parser-runtime.test.tssrc/core/services/tokenizer/yomitan-parser-runtime.tssrc/token-merger.ts
Deploying subminer-docs with
|
| Latest commit: |
4d5bf3d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0edfeb54.subminer.pages.dev |
| Branch Preview URL: | https://tokenizer-updates.subminer.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/core/services/tokenizer/annotation-stage.test.ts (1)
1073-1320: Recommend splitting this test file by concern.Grouping exclusion predicates, metadata stripping, and N+1 behavior into separate suites/files would keep this TS module easier to evolve.
As per coding guidelines, "TypeScript files should be kept small and follow existing patterns".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/annotation-stage.test.ts` around lines 1073 - 1320, The test file currently mixes three concerns around annotateTokens (exclusion predicates, metadata stripping, and N+1 behavior) which makes it large and hard to maintain; split the suite into separate test modules (e.g., annotateTokens.exclusions.test.ts, annotateTokens.metadata.test.ts, annotateTokens.nplusone.test.ts) by moving the relevant tests that reference annotateTokens and makeDeps (the tests for standalone helpers/particles/phrases that clear annotations, the tests asserting frequencyRank/jlptLevel cleared, and the tests asserting isNPlusOneTarget behavior) into their respective files, retain the same test names and assertions, update imports for annotateTokens, makeDeps, makeToken, and PartOfSpeech in each new file, and run/update the test runner configuration if needed so the new files are discovered.src/core/services/tokenizer.test.ts (1)
3062-3249: Consider splitting this test file into focused suites.This file now mixes parser selection, enrichment, exclusion, and N+1 behavior in one very large TS module. Splitting by concern would improve failure triage and long-term maintainability.
As per coding guidelines, "TypeScript files should be kept small and follow existing patterns".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer.test.ts` around lines 3062 - 3249, The test file mixes multiple concerns; split tests into focused suites by creating separate test modules for parser selection (tests referencing tokenizeWithMecab behavior), enrichment (tests asserting frequencyRank/jlptLevel like "tokenizeSubtitle keeps frequency enrichment..."), exclusion/non-independent pos2 behavior (the "excludes default non-independent pos2..." test), and N+1/known-word behavior (tests using isKnownWord/getNPlusOneEnabled), moving the corresponding test cases that call tokenizeSubtitle and use makeDepsFromYomitanTokens into those files, export or import shared helpers (makeDepsFromYomitanTokens, PartOfSpeech) so each new file can reuse them, keep test names and assertions intact, and update any test runner imports/exports so all new test files are discovered by the test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.ts`:
- Around line 1944-1958: The refreshLinuxVisibleOverlayAfterMpvFullscreenChange
function currently calls mainWindow.show() after hide(), which will steal focus
on Linux because the overlay is focusable; replace that call with
mainWindow.showInactive() so the overlay becomes visible without activating
input focus. Update the code in
refreshLinuxVisibleOverlayAfterMpvFullscreenChange (where it obtains mainWindow
via overlayManager.getMainWindow and calls ensureOverlayWindowLevel) to use
showInactive() instead of show(), keeping the preceding hide() and
ensureOverlayWindowLevel calls unchanged.
In `@src/token-merger.ts`:
- Around line 373-384: The gap-check is currently slicing the unnormalized
sourceText which can desynchronize offsets computed from the normalized text
(see markNPlusOneTargets); update hasSentenceBoundaryInSourceGap to normalize
sourceText with the same normalization routine used when computing token offsets
(the same function used around line 181) before creating gap =
sourceText.slice(previousEnd, nextStart), and apply the same
normalization-before-slicing change to the other gap-check sites referenced
(around lines 391 and 434-437) so all sentence-boundary checks operate on the
normalized text that matches the token offset space.
In `@src/window-trackers/hyprland-tracker.ts`:
- Around line 351-357: The getHyprlandMonitors function can throw when running
execSync('hyprctl -j monitors') or when parseHyprctlMonitors fails, causing
caller resolveHyprlandWindowGeometry to be unable to fall back to
client.at/client.size; wrap the execSync + parseHyprctlMonitors call in a
try/catch and return null on any error or parse failure so the caller can
degrade gracefully, referencing getHyprlandMonitors, execSync('hyprctl -j
monitors'), parseHyprctlMonitors, and
resolveHyprlandWindowGeometry/client.at/client.size.
---
Nitpick comments:
In `@src/core/services/tokenizer.test.ts`:
- Around line 3062-3249: The test file mixes multiple concerns; split tests into
focused suites by creating separate test modules for parser selection (tests
referencing tokenizeWithMecab behavior), enrichment (tests asserting
frequencyRank/jlptLevel like "tokenizeSubtitle keeps frequency enrichment..."),
exclusion/non-independent pos2 behavior (the "excludes default non-independent
pos2..." test), and N+1/known-word behavior (tests using
isKnownWord/getNPlusOneEnabled), moving the corresponding test cases that call
tokenizeSubtitle and use makeDepsFromYomitanTokens into those files, export or
import shared helpers (makeDepsFromYomitanTokens, PartOfSpeech) so each new file
can reuse them, keep test names and assertions intact, and update any test
runner imports/exports so all new test files are discovered by the test suite.
In `@src/core/services/tokenizer/annotation-stage.test.ts`:
- Around line 1073-1320: The test file currently mixes three concerns around
annotateTokens (exclusion predicates, metadata stripping, and N+1 behavior)
which makes it large and hard to maintain; split the suite into separate test
modules (e.g., annotateTokens.exclusions.test.ts,
annotateTokens.metadata.test.ts, annotateTokens.nplusone.test.ts) by moving the
relevant tests that reference annotateTokens and makeDeps (the tests for
standalone helpers/particles/phrases that clear annotations, the tests asserting
frequencyRank/jlptLevel cleared, and the tests asserting isNPlusOneTarget
behavior) into their respective files, retain the same test names and
assertions, update imports for annotateTokens, makeDeps, makeToken, and
PartOfSpeech in each new file, and run/update the test runner configuration if
needed so the new files are discovered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 543913f2-92af-471b-991e-a5da46d21d75
⛔ Files ignored due to path filters (7)
backlog/tasks/task-306 - Fix-Hyprland-fullscreen-overlay-geometry-and-hover-pause.mdis excluded by!backlog/**backlog/tasks/task-307 - Exclude-kana-only-words-from-N1-subtitle-targets.mdis excluded by!backlog/**backlog/tasks/task-308 - Restore-persistent-JLPT-subtitle-underlines.mdis excluded by!backlog/**changes/306-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/307-kana-nplusone-targets.mdis excluded by!changes/**changes/308-jlpt-underlines.mdis excluded by!changes/**docs-site/troubleshooting.mdis excluded by!docs-site/**
📒 Files selected for processing (21)
src/core/services/mpv-properties.tssrc/core/services/mpv-protocol.test.tssrc/core/services/mpv-protocol.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/main.tssrc/main/runtime/mpv-client-event-bindings.test.tssrc/main/runtime/mpv-client-event-bindings.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/renderer/handlers/mouse.test.tssrc/renderer/handlers/mouse.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.ts
✅ Files skipped from review due to trivial changes (2)
- src/core/services/mpv-properties.ts
- src/main/runtime/mpv-client-event-bindings.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/window-trackers/hyprland-tracker.ts (1)
304-313: Coalesce burst polling without a synchronous immediate call.
scheduleGeometryPollBurst()callspollGeometry()inline, andpollGeometry()usesexecSync. On noisy geometry events (windowtitle*,workspace*), this can block the event loop repeatedly. Consider scheduling0msinstead of immediate execution so events are coalesced first.Suggested diff
private scheduleGeometryPollBurst(): void { - this.pollGeometry(); for (const timeout of this.pollTimeouts) { clearTimeout(timeout); } - this.pollTimeouts = [50, 150, 300].map((delayMs) => { + this.pollTimeouts = [0, 50, 150, 300].map((delayMs) => { const pollTimeout = setTimeout(() => { this.pollTimeouts = this.pollTimeouts.filter((timeout) => timeout !== pollTimeout); this.pollGeometry(); }, delayMs); return pollTimeout;Also applies to: 321-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/window-trackers/hyprland-tracker.ts` around lines 304 - 313, scheduleGeometryPollBurst currently calls pollGeometry synchronously (which uses execSync) and can block the event loop on noisy events; change it to schedule an immediate (setTimeout(..., 0)) call instead of invoking pollGeometry inline and similarly replace any other direct pollGeometry calls in this function (and the related block at 321-324) so the initial poll is queued and all bursts are coalesced via pollTimeouts; update scheduleGeometryPollBurst to push a 0ms timeout that calls pollGeometry and removes itself from this.pollTimeouts, and ensure pollTimeouts is still cleared at the start of the method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/window-trackers/hyprland-tracker.ts`:
- Around line 304-313: scheduleGeometryPollBurst currently calls pollGeometry
synchronously (which uses execSync) and can block the event loop on noisy
events; change it to schedule an immediate (setTimeout(..., 0)) call instead of
invoking pollGeometry inline and similarly replace any other direct pollGeometry
calls in this function (and the related block at 321-324) so the initial poll is
queued and all bursts are coalesced via pollTimeouts; update
scheduleGeometryPollBurst to push a 0ms timeout that calls pollGeometry and
removes itself from this.pollTimeouts, and ensure pollTimeouts is still cleared
at the start of the method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 879ad861-b578-4566-98ff-6f7f41b0c593
📒 Files selected for processing (3)
src/main.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/token-merger.ts
ksyasuda
force-pushed
the
tokenizer-updates
branch
from
7749c60 to
c150fce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main.ts (1)
1914-1978: Extract the Linux fullscreen refresh helper out ofsrc/main.ts.This logic is reasonable, but adding another platform-specific timer/burst helper to an already massive file makes
src/main.tsharder to maintain. Moving the Linux fullscreen refresh constants and helpers into a small runtime module would better match the rest of the runtime composition pattern.As per coding guidelines,
TypeScript files should be kept small and follow existing patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 1914 - 1978, Extract the Linux fullscreen refresh logic (const LINUX_MPV_FULLSCREEN_OVERLAY_REFRESH_DELAYS_MS, state var linuxMpvFullscreenOverlayRefreshTimeouts, and the functions clearLinuxMpvFullscreenOverlayRefreshTimeouts, refreshLinuxVisibleOverlayAfterMpvFullscreenChange, scheduleLinuxVisibleOverlayFullscreenRefreshBurst) into a new small runtime module; move the constants/vars and functions out of src/main.ts, export the functions, and update src/main.ts to import and call the exported schedule/clear helpers. In the new module import any runtime dependencies used inside those functions (overlayManager, overlayVisibilityRuntime, ensureOverlayWindowLevel) from their existing modules, preserve the timeout unref() behavior and the same filtering logic when clearing timeouts, and ensure types (ReturnType<typeof setTimeout>) are kept so the new file compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/tokenizer.ts`:
- Around line 355-364: The current check in
resolvePartOfSpeechFromYomitanWordClasses only matches the exact 'aux' token so
subclasses like 'aux-v' or 'aux-adj' fall through; update the auxiliary
detection to mirror the verb/noun pattern (e.g., use a startsWith/substring test
or any match that treats tokens beginning with 'aux' as auxiliary) and still
return { partOfSpeech: PartOfSpeech.bound_auxiliary, pos1: '助動詞' } so auxiliary
subclasses are correctly classified and retain subtitle exclusion and
JLPT/frequency suppression behavior.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 1914-1978: Extract the Linux fullscreen refresh logic (const
LINUX_MPV_FULLSCREEN_OVERLAY_REFRESH_DELAYS_MS, state var
linuxMpvFullscreenOverlayRefreshTimeouts, and the functions
clearLinuxMpvFullscreenOverlayRefreshTimeouts,
refreshLinuxVisibleOverlayAfterMpvFullscreenChange,
scheduleLinuxVisibleOverlayFullscreenRefreshBurst) into a new small runtime
module; move the constants/vars and functions out of src/main.ts, export the
functions, and update src/main.ts to import and call the exported schedule/clear
helpers. In the new module import any runtime dependencies used inside those
functions (overlayManager, overlayVisibilityRuntime, ensureOverlayWindowLevel)
from their existing modules, preserve the timeout unref() behavior and the same
filtering logic when clearing timeouts, and ensure types (ReturnType<typeof
setTimeout>) are kept so the new file compiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c7c11784-5070-4f43-b3f3-50f40f2db837
⛔ Files ignored due to path filters (10)
backlog/tasks/task-304 - Fix-N1-sentence-boundary-counting-across-Yomitan-punctuation-gaps.mdis excluded by!backlog/**backlog/tasks/task-305 - Use-Yomitan-word-classes-for-subtitle-token-POS-filtering.mdis excluded by!backlog/**backlog/tasks/task-306 - Fix-Hyprland-fullscreen-overlay-geometry-and-hover-pause.mdis excluded by!backlog/**backlog/tasks/task-307 - Exclude-kana-only-words-from-N1-subtitle-targets.mdis excluded by!backlog/**backlog/tasks/task-308 - Restore-persistent-JLPT-subtitle-underlines.mdis excluded by!backlog/**changes/305-tokenizer-word-class-pos-filtering.mdis excluded by!changes/**changes/306-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/307-kana-nplusone-targets.mdis excluded by!changes/**changes/308-jlpt-underlines.mdis excluded by!changes/**docs-site/troubleshooting.mdis excluded by!docs-site/**
📒 Files selected for processing (28)
src/core/services/mpv-properties.tssrc/core/services/mpv-protocol.test.tssrc/core/services/mpv-protocol.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.test.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/core/services/tokenizer/yomitan-parser-runtime.test.tssrc/core/services/tokenizer/yomitan-parser-runtime.tssrc/main.tssrc/main/runtime/mpv-client-event-bindings.test.tssrc/main/runtime/mpv-client-event-bindings.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/renderer/handlers/mouse.test.tssrc/renderer/handlers/mouse.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.ts
✅ Files skipped from review due to trivial changes (7)
- src/core/services/mpv.test.ts
- src/core/services/overlay-window.ts
- src/main/runtime/mpv-client-event-bindings.test.ts
- src/core/services/tokenizer/parser-enrichment-stage.test.ts
- src/main/runtime/mpv-main-event-main-deps.ts
- src/renderer/style.css
- src/window-trackers/hyprland-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/runtime/mpv-main-event-bindings.ts
- src/main/runtime/mpv-main-event-main-deps.test.ts
- src/renderer/handlers/mouse.test.ts
- src/core/services/mpv-protocol.ts
- src/renderer/handlers/mouse.ts
- src/core/services/tokenizer/yomitan-parser-runtime.ts
- src/token-merger.ts
- src/core/services/tokenizer/subtitle-annotation-filter.ts
- src/window-trackers/hyprland-tracker.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/services/tokenizer.test.ts (1)
3062-3790: Please split these new tokenizer scenarios into smaller test files.This file is already very large; continuing to add dense blocks here makes navigation and maintenance harder. Consider extracting these additions into focused suites (for example:
tokenizer.word-classes.test.tsandtokenizer.subtitle-exclusions.test.ts).As per coding guidelines,
TypeScript files should be kept small and follow existing patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer.test.ts` around lines 3062 - 3790, The tokenizer test additions (tests invoking tokenizeSubtitle with scenarios like "uses Yomitan word classes..." "subtitle-exclusions", e.g. tests referencing PartOfSpeech and helper makeDepsFromYomitanTokens/makeDeps) should be split into smaller focused test files; create new suites such as tokenizer.word-classes.test.ts (move tests about Yomitan wordClasses, auxiliary subclasses, detailed MeCab POS) and tokenizer.subtitle-exclusions.test.ts (move tests about frequency/N+1 exclusions, interjections, merged tokens, hoverable grammar tails), copy the relevant tests (preserving describe/test names and any helper usages of makeDeps/makeDepsFromYomitanTokens and PartOfSpeech), update any shared helper imports so each new file imports the same helpers currently used in tokenizer.test.ts, and remove the moved tests from the original file to keep it small and consistent with existing test patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.ts`:
- Around line 3815-3824: The scheduled Linux fullscreen-refresh burst created
via scheduleLinuxVisibleOverlayFullscreenRefreshBurst (called from the
onFullscreenChange closure) can outlive the visible overlay and act on a
recreated window; fix by capturing the cancellation handle returned from
scheduleLinuxVisibleOverlayFullscreenRefreshBurst when calling it (from
onFullscreenChange) and invoke that cancel function whenever the visible overlay
is hidden or closed (e.g., in the overlay visibility hide/close handlers that
call into overlayManager / overlayVisibilityRuntime and any recreate path),
ensuring you also keep the existing quit-time clear; update the visible-overlay
hide/close code paths to call the stored cancel before destroying or replacing
the window so pending callbacks don’t run against the new window.
In `@src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts`:
- Around line 35-40: Replace the fixed 10ms sleep with a deterministic wait:
instead of awaiting new Promise(setTimeout(..., 10)), poll or await until the
expected callback has been invoked (e.g., wait in a loop with short yields until
calls.includes('updateVisibleOverlayVisibility') becomes true) or use controlled
timers (sinon/lolex fake timers) and advance them so the first setTimeout(…, 0)
runs; ensure the assertions then run after that awaited condition so the checks
for updateVisibleOverlayVisibility, hide, showInactive, and
ensureOverlayWindowLevel are stable.
---
Nitpick comments:
In `@src/core/services/tokenizer.test.ts`:
- Around line 3062-3790: The tokenizer test additions (tests invoking
tokenizeSubtitle with scenarios like "uses Yomitan word classes..."
"subtitle-exclusions", e.g. tests referencing PartOfSpeech and helper
makeDepsFromYomitanTokens/makeDeps) should be split into smaller focused test
files; create new suites such as tokenizer.word-classes.test.ts (move tests
about Yomitan wordClasses, auxiliary subclasses, detailed MeCab POS) and
tokenizer.subtitle-exclusions.test.ts (move tests about frequency/N+1
exclusions, interjections, merged tokens, hoverable grammar tails), copy the
relevant tests (preserving describe/test names and any helper usages of
makeDeps/makeDepsFromYomitanTokens and PartOfSpeech), update any shared helper
imports so each new file imports the same helpers currently used in
tokenizer.test.ts, and remove the moved tests from the original file to keep it
small and consistent with existing test patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 590b81a7-2e46-4e08-a72f-21858ee25599
⛔ Files ignored due to path filters (2)
backlog/tasks/task-309 - Accept-modified-follow-up-digits-for-multi-line-sentence-mining.mdis excluded by!backlog/**changes/309-multi-mine-modified-digits.mdis excluded by!changes/**
📒 Files selected for processing (18)
package.jsonplugin/subminer/session_bindings.luascripts/test-plugin-session-bindings.luasrc/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/main.tssrc/main/runtime/app-lifecycle-actions.test.tssrc/main/runtime/app-lifecycle-actions.tssrc/main/runtime/app-lifecycle-main-cleanup.test.tssrc/main/runtime/app-lifecycle-main-cleanup.tssrc/main/runtime/composers/startup-lifecycle-composer.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/renderer/handlers/keyboard.test.tssrc/renderer/handlers/keyboard.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.ts
✅ Files skipped from review due to trivial changes (2)
- src/main/runtime/composers/startup-lifecycle-composer.test.ts
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/token-merger.ts
- src/window-trackers/hyprland-tracker.ts
- src/core/services/tokenizer/annotation-stage.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/services/tokenizer/annotation-stage.test.ts (1)
369-493: Collapse these exclusion cases into table-driven assertions.The coverage is solid, but both blocks repeat the same single-token setup and “all annotation fields cleared” assertions many times. A small helper plus subtests/data tables would keep this already-large file much easier to extend when more exclusions are added.
As per coding guidelines, "TypeScript files should be kept small and follow existing patterns".
Also applies to: 1207-1418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/annotation-stage.test.ts` around lines 369 - 493, The tests repeatedly construct single-token cases to assert shouldExcludeTokenFromSubtitleAnnotations returns true; refactor by replacing the repeated test blocks with a single table-driven test: create a data array of token-parameter objects (using makeToken and PartOfSpeech values) and iterate (or use test.each) to run a subtest per entry, asserting shouldExcludeTokenFromSubtitleAnnotations(token) === true and including token.surface as the subtest name; consolidate similar groups (e.g., standalone particles, grammar helpers, single-kana) into grouped tables to keep the file compact and follow existing patterns.src/token-merger.ts (1)
286-327: Reuse the existing kana-normalization path for N+1 eligibility.
subtitle-annotation-filter.tsalready defines similar kana-only exclusion behavior, but this new helper stack reimplements it on raw code points. That gives N+1 targeting a second “kana-only” definition and makes drift likely if the normalization rules change in one place but not the other. Please route this through the shared tokenizer helper, or normalize with the same path before checking here.As per coding guidelines, "TypeScript files should be kept small and follow existing patterns".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/token-merger.ts` around lines 286 - 327, Replace the ad-hoc kana detection in isKanaChar/isKanaCandidateIgnorableChar/isKanaOnlyText with the shared tokenizer/normalization used by subtitle-annotation-filter.ts: import the canonical normalization/tokenizer helper (the function used there to produce the kana-normalized text) and call it from normalizeSourceTextForTokenOffsets or at the top of isKanaOnlyText, then apply the existing kana-only check against that normalized output instead of raw code points; update isKanaOnlyText to return false on empty normalized result and keep the existing hasKana logic but operating on the shared-normalized string so both modules use the same normalization path and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/services/tokenizer/annotation-stage.test.ts`:
- Around line 369-493: The tests repeatedly construct single-token cases to
assert shouldExcludeTokenFromSubtitleAnnotations returns true; refactor by
replacing the repeated test blocks with a single table-driven test: create a
data array of token-parameter objects (using makeToken and PartOfSpeech values)
and iterate (or use test.each) to run a subtest per entry, asserting
shouldExcludeTokenFromSubtitleAnnotations(token) === true and including
token.surface as the subtest name; consolidate similar groups (e.g., standalone
particles, grammar helpers, single-kana) into grouped tables to keep the file
compact and follow existing patterns.
In `@src/token-merger.ts`:
- Around line 286-327: Replace the ad-hoc kana detection in
isKanaChar/isKanaCandidateIgnorableChar/isKanaOnlyText with the shared
tokenizer/normalization used by subtitle-annotation-filter.ts: import the
canonical normalization/tokenizer helper (the function used there to produce the
kana-normalized text) and call it from normalizeSourceTextForTokenOffsets or at
the top of isKanaOnlyText, then apply the existing kana-only check against that
normalized output instead of raw code points; update isKanaOnlyText to return
false on empty normalized result and keep the existing hasKana logic but
operating on the shared-normalized string so both modules use the same
normalization path and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09925e0f-1aee-477d-87d8-47e87150e0cc
⛔ Files ignored due to path filters (1)
backlog/tasks/task-310 - Suppress-N1-highlight-for-kana-only-candidate-sentences.mdis excluded by!backlog/**
📒 Files selected for processing (5)
src/core/services/tokenizer/annotation-stage.test.tssrc/main.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/token-merger.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/tokenizer/annotation-stage.test.ts`:
- Around line 672-709: The test uses a mixed-script surface "スイッチ…" (contains
punctuation) so it won't catch regressions in isKanaOnlyText; update the third
token created by makeToken in the test named "annotateTokens does not mark
kana-only unknown target with subtitle punctuation as N+1" to be truly kana-only
(e.g., change surface from "スイッチ…" to a pure-kana string like "スイッチ" or a
hiragana variant "すいっち" and ensure headword/reading match), so annotateTokens
and the isKanaOnlyText guard are exercised as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d82198ce-4984-4044-86ea-04153e9ee2cf
⛔ Files ignored due to path filters (5)
backlog/tasks/task-310 - Suppress-N1-highlight-for-kana-only-candidate-sentences.mdis excluded by!backlog/**backlog/tasks/task-311 - Suppress-auxiliary-inflection-fragments-from-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-312 - Suppress-ja-nai-explanatory-ending-subtitle-annotations.mdis excluded by!backlog/**changes/311-auxiliary-inflection-annotation-filter.mdis excluded by!changes/**changes/312-grammar-ending-annotation-filter.mdis excluded by!changes/**
📒 Files selected for processing (7)
src/core/services/tokenizer.test.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/main.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/token-merger.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/token-merger.ts
ksyasuda
force-pushed
the
tokenizer-updates
branch
from
251b6cd to
c7996e3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/runtime/app-lifecycle-actions.test.ts (1)
47-52: ⚡ Quick winStrengthen this test with order assertions for the new cleanup step.
includes(...)can pass even if the timeout cleanup runs too late. Consider asserting it occurs before window-destroy calls to lock in intended shutdown safety.Suggested assertion additions
assert.ok(calls.includes('clear-windows-visible-overlay-poll')); assert.ok(calls.includes('clear-linux-mpv-fullscreen-overlay-refresh-timeouts')); + assert.ok( + calls.indexOf('clear-linux-mpv-fullscreen-overlay-refresh-timeouts') < + calls.indexOf('destroy-main-overlay-window'), + ); assert.ok(calls.indexOf('flush-mpv-log') < calls.indexOf('destroy-socket'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/runtime/app-lifecycle-actions.test.ts` around lines 47 - 52, The test currently only checks presence of the new timeout-cleanup steps; strengthen it by asserting their order relative to window-destroy steps: use the existing calls array and add index comparisons such as assert.ok(calls.indexOf('clear-windows-visible-overlay-poll') < calls.indexOf('destroy-tray')) and assert.ok(calls.indexOf('clear-linux-mpv-fullscreen-overlay-refresh-timeouts') < calls.indexOf('destroy-tray')) (or compare against whichever window-destroy symbol is used in the test) so both cleanup actions run before any destroy-window/destroy-tray teardown steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/runtime/app-lifecycle-actions.test.ts`:
- Around line 47-52: The test currently only checks presence of the new
timeout-cleanup steps; strengthen it by asserting their order relative to
window-destroy steps: use the existing calls array and add index comparisons
such as assert.ok(calls.indexOf('clear-windows-visible-overlay-poll') <
calls.indexOf('destroy-tray')) and
assert.ok(calls.indexOf('clear-linux-mpv-fullscreen-overlay-refresh-timeouts') <
calls.indexOf('destroy-tray')) (or compare against whichever window-destroy
symbol is used in the test) so both cleanup actions run before any
destroy-window/destroy-tray teardown steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: afd75b99-5954-49d0-891f-d7e25e4ea7e7
⛔ Files ignored due to path filters (17)
backlog/tasks/task-304 - Fix-N1-sentence-boundary-counting-across-Yomitan-punctuation-gaps.mdis excluded by!backlog/**backlog/tasks/task-305 - Use-Yomitan-word-classes-for-subtitle-token-POS-filtering.mdis excluded by!backlog/**backlog/tasks/task-306 - Fix-Hyprland-fullscreen-overlay-geometry-and-hover-pause.mdis excluded by!backlog/**backlog/tasks/task-307 - Exclude-kana-only-words-from-N1-subtitle-targets.mdis excluded by!backlog/**backlog/tasks/task-308 - Restore-persistent-JLPT-subtitle-underlines.mdis excluded by!backlog/**backlog/tasks/task-309 - Accept-modified-follow-up-digits-for-multi-line-sentence-mining.mdis excluded by!backlog/**backlog/tasks/task-310 - Suppress-N1-highlight-for-kana-only-candidate-sentences.mdis excluded by!backlog/**backlog/tasks/task-311 - Suppress-auxiliary-inflection-fragments-from-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-312 - Suppress-ja-nai-explanatory-ending-subtitle-annotations.mdis excluded by!backlog/**changes/305-tokenizer-word-class-pos-filtering.mdis excluded by!changes/**changes/306-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/307-kana-nplusone-targets.mdis excluded by!changes/**changes/308-jlpt-underlines.mdis excluded by!changes/**changes/309-multi-mine-modified-digits.mdis excluded by!changes/**changes/311-auxiliary-inflection-annotation-filter.mdis excluded by!changes/**changes/312-grammar-ending-annotation-filter.mdis excluded by!changes/**docs-site/troubleshooting.mdis excluded by!docs-site/**
📒 Files selected for processing (40)
package.jsonplugin/subminer/session_bindings.luascripts/test-plugin-session-bindings.luasrc/core/services/mpv-properties.tssrc/core/services/mpv-protocol.test.tssrc/core/services/mpv-protocol.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.test.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/core/services/tokenizer/yomitan-parser-runtime.test.tssrc/core/services/tokenizer/yomitan-parser-runtime.tssrc/main.tssrc/main/runtime/app-lifecycle-actions.test.tssrc/main/runtime/app-lifecycle-actions.tssrc/main/runtime/app-lifecycle-main-cleanup.test.tssrc/main/runtime/app-lifecycle-main-cleanup.tssrc/main/runtime/composers/startup-lifecycle-composer.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/main/runtime/mpv-client-event-bindings.test.tssrc/main/runtime/mpv-client-event-bindings.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/renderer/handlers/keyboard.test.tssrc/renderer/handlers/keyboard.tssrc/renderer/handlers/mouse.test.tssrc/renderer/handlers/mouse.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.ts
✅ Files skipped from review due to trivial changes (14)
- src/core/services/mpv-protocol.test.ts
- src/core/services/mpv.test.ts
- src/main/runtime/mpv-main-event-main-deps.ts
- src/main/runtime/composers/startup-lifecycle-composer.test.ts
- src/core/services/tokenizer/yomitan-parser-runtime.test.ts
- src/core/services/mpv-properties.ts
- src/main/runtime/mpv-client-event-bindings.test.ts
- src/main/runtime/mpv-main-event-bindings.ts
- src/renderer/handlers/keyboard.test.ts
- src/core/services/mpv.ts
- src/core/services/tokenizer.ts
- src/main.ts
- src/token-merger.ts
- src/core/services/tokenizer.test.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- src/window-trackers/hyprland-tracker.test.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts
- package.json
- src/core/services/tokenizer/parser-enrichment-stage.test.ts
- src/main/runtime/mpv-client-event-bindings.ts
- src/renderer/style.css
- src/main/runtime/app-lifecycle-main-cleanup.ts
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/main/runtime/app-lifecycle-main-cleanup.test.ts
- src/core/services/mpv-protocol.ts
- src/core/services/overlay-window.ts
- scripts/test-plugin-session-bindings.lua
- src/renderer/handlers/mouse.ts
- src/core/services/tokenizer/yomitan-parser-runtime.ts
- src/core/services/tokenizer/annotation-stage.ts
- src/renderer/handlers/keyboard.ts
- src/window-trackers/hyprland-tracker.ts
- src/core/services/tokenizer/subtitle-annotation-filter.ts
- src/core/services/tokenizer/annotation-stage.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/services/tokenizer/parser-selection-stage.ts (1)
27-43: ⚡ Quick winCentralize grammar-ending lexicon to prevent split/filter drift.
STANDALONE_GRAMMAR_ENDINGSis now a second hardcoded grammar list parallel tosrc/core/services/tokenizer/subtitle-annotation-filter.tsrules. Divergence here will cause parser-splitting and annotation-filter behavior to silently desync. Consider moving these shared endings into one tokenizer constants module consumed by both stages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/parser-selection-stage.ts` around lines 27 - 43, STANDALONE_GRAMMAR_ENDINGS duplicates a grammar lexicon from subtitle-annotation-filter.ts causing potential drift; extract this set into a shared tokenizer constants module (e.g., export const STANDALONE_GRAMMAR_ENDINGS from a new or existing tokenizer/constants.ts), replace the local declaration in parser-selection-stage.ts with an import of that exported constant, and update subtitle-annotation-filter.ts to import the same constant so both stages consume the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/services/tokenizer/parser-selection-stage.ts`:
- Around line 27-43: STANDALONE_GRAMMAR_ENDINGS duplicates a grammar lexicon
from subtitle-annotation-filter.ts causing potential drift; extract this set
into a shared tokenizer constants module (e.g., export const
STANDALONE_GRAMMAR_ENDINGS from a new or existing tokenizer/constants.ts),
replace the local declaration in parser-selection-stage.ts with an import of
that exported constant, and update subtitle-annotation-filter.ts to import the
same constant so both stages consume the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 536dd360-9d89-4c99-8d6b-7d811df19d13
⛔ Files ignored due to path filters (3)
backlog/tasks/task-315 - Suppress-annotations-for-standalone-じゃない-and-です-ending-tokens.mdis excluded by!backlog/**backlog/tasks/task-316 - Fix-macOS-launcher-playback-exit-with-background-stats-daemon.mdis excluded by!backlog/**changes/316-macos-playback-stats-daemon.mdis excluded by!changes/**
📒 Files selected for processing (9)
src/core/services/tokenizer.test.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/parser-selection-stage.test.tssrc/core/services/tokenizer/parser-selection-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/main/runtime/stats-server-routing.test.tssrc/main/runtime/stats-server-routing.ts
✅ Files skipped from review due to trivial changes (1)
- src/core/services/tokenizer/annotation-stage.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/services/tokenizer/subtitle-annotation-filter.ts
ksyasuda
force-pushed
the
tokenizer-updates
branch
from
03c840c to
738ae17
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
scripts/test-plugin-session-bindings.lua (1)
128-132: ⚡ Quick winAssert exact CLI argv length to catch silent argument drift.
You already validate values at indices 1..3; adding an argv length check makes this regression stricter.
Proposed fix
local call = recorded.async_calls[`#recorded.async_calls`] assert_true(call ~= nil, "modified digit should invoke CLI action") +assert_true(`#call` == 3, "CLI action should pass exactly 3 argv entries") assert_true(call[1] == "/tmp/subminer", "CLI action should use configured binary") assert_true(call[2] == "--mine-sentence-count", "CLI action should mine sentence count") assert_true(call[3] == "3", "CLI action should pass selected count")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-plugin-session-bindings.lua` around lines 128 - 132, Add a strict argv length check after grabbing call from recorded.async_calls: assert that `#call` equals 3 so silent argument drift is detected; update the test near the existing assertions that reference call and recorded.async_calls (i.e., after local call = recorded.async_calls[`#recorded.async_calls`]) to include an assertion that the argument vector length is exactly 3 with an explanatory message.src/core/services/tokenizer.test.ts (2)
3078-3155: ⚡ Quick winBroaden the
wordClassesmapping matrix a bit.These cases only exercise
prtandaux-v, while the runtime mapping in this PR also covers verb, adjective, and noun-like classes. Adding one representative case for each remaining family would close the main regression gap with very little extra setup.Possible table-driven shape
+for (const { + wordClass, + expectedPartOfSpeech, + expectedPos1, +} of [ + { wordClass: 'v1', expectedPartOfSpeech: PartOfSpeech.verb, expectedPos1: '動詞' }, + { wordClass: 'adj-i', expectedPartOfSpeech: PartOfSpeech.i_adjective, expectedPos1: '形容詞' }, + { wordClass: 'n', expectedPartOfSpeech: PartOfSpeech.noun, expectedPos1: '名詞' }, +]) { + test(`tokenizeSubtitle maps ${wordClass}`, async () => { + // existing makeDepsFromYomitanTokens setup... + }); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer.test.ts` around lines 3078 - 3155, Tests for tokenizeSubtitle only cover 'prt' and 'aux-v' Yomitan wordClasses but the runtime mapping also handles verb, adjective, and noun-like classes; add additional table-driven test cases using makeDepsFromYomitanTokens to include one representative token each with wordClasses like 'v' (or 'vb'), 'adj' (or 'adj-i'), and a noun family (e.g., 'n'/'noun') and assert tokenizeSubtitle returns the expected PartOfSpeech (PartOfSpeech.verb, PartOfSpeech.adjective, PartOfSpeech.noun or appropriate constants), plus correct pos1/pos2 when available; place these alongside the existing tests referencing tokenizeSubtitle and PartOfSpeech so the mapping coverage is exercised without changing runtime code.
4246-4449: 🏗️ Heavy liftConsider moving these filter-centric cases into a focused test file.
This batch is validating subtitle-annotation filtering more than top-level tokenization, and
src/core/services/tokenizer.test.tsis already getting hard to scan. Moving these scenarios intosrc/core/services/tokenizer/annotation-stage.test.tsor a dedicated filter test would keep the integration suite tighter.As per coding guidelines, "TypeScript files should be kept small and follow existing patterns."
Also applies to: 4670-4914
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer.test.ts` around lines 4246 - 4449, These two filter-focused tests (the ones named "tokenizeSubtitle clears annotations for ja-nai explanatory endings while preserving lexical content" and "tokenizeSubtitle clears annotations for standalone polite copula endings without POS metadata") should be moved out of the large tokenizer.test.ts and into a new focused test file named annotation-stage.test.ts; copy the tests verbatim but update their imports to reference the same helpers (tokenizeSubtitle, makeDepsFromYomitanTokens, PartOfSpeech) from the module exports used in the original file, ensure any test helpers remain exported/imported so the tests compile, and run the suite to confirm the new test file is detected by the runner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-plugin-session-bindings.lua`:
- Line 13: Replace the hardcoded os.execute("mkdir -p .tmp") call with a
shell-agnostic creation: detect the platform via package.config:sub(1,1) and
call os.execute("mkdir .tmp") on Windows vs os.execute("mkdir -p .tmp") on Unix,
or alternatively use a Lua filesystem API (e.g., lfs.mkdir) if available; update
the os.execute invocation accordingly so the test creates the .tmp directory on
both Windows and Unix runners.
In `@src/core/services/tokenizer.ts`:
- Around line 99-100: The early-return branches call
stripSubtitleAnnotationMetadata(tokens) without forwarding the
TokenizerAnnotationOptions (including the new sourceText), so
sentence-boundary-aware filtering never sees sourceText; update each call site
(the early-return path and the other occurrences around the tokenization logic)
to forward the same TokenizerAnnotationOptions object (or at minimum {
sourceText }) into stripSubtitleAnnotationMetadata(tokens, options), and if
necessary update stripSubtitleAnnotationMetadata's signature to accept and use
TokenizerAnnotationOptions so sourceText is honored during metadata stripping.
In `@src/core/services/tokenizer/parser-selection-stage.ts`:
- Around line 27-43: STANDALONE_GRAMMAR_ENDINGS only covers the "です"/"じゃない"
family but must match the broader copula/explanatory forms used by
subtitle-annotation-filter; update the STANDALONE_GRAMMAR_ENDINGS constant to
include other copula/explanatory endings (e.g., だ, だった, では, ではない, でしょう, だろう, だよ,
だね, なんだ, etc.) so splitting behavior mirrors the subtitle exclusion rules, and
make the same extended change to the duplicate list found around the 162-168
range to keep both definitions consistent (use the existing constant name
STANDALONE_GRAMMAR_ENDINGS to locate and update both occurrences).
---
Nitpick comments:
In `@scripts/test-plugin-session-bindings.lua`:
- Around line 128-132: Add a strict argv length check after grabbing call from
recorded.async_calls: assert that `#call` equals 3 so silent argument drift is
detected; update the test near the existing assertions that reference call and
recorded.async_calls (i.e., after local call =
recorded.async_calls[`#recorded.async_calls`]) to include an assertion that the
argument vector length is exactly 3 with an explanatory message.
In `@src/core/services/tokenizer.test.ts`:
- Around line 3078-3155: Tests for tokenizeSubtitle only cover 'prt' and 'aux-v'
Yomitan wordClasses but the runtime mapping also handles verb, adjective, and
noun-like classes; add additional table-driven test cases using
makeDepsFromYomitanTokens to include one representative token each with
wordClasses like 'v' (or 'vb'), 'adj' (or 'adj-i'), and a noun family (e.g.,
'n'/'noun') and assert tokenizeSubtitle returns the expected PartOfSpeech
(PartOfSpeech.verb, PartOfSpeech.adjective, PartOfSpeech.noun or appropriate
constants), plus correct pos1/pos2 when available; place these alongside the
existing tests referencing tokenizeSubtitle and PartOfSpeech so the mapping
coverage is exercised without changing runtime code.
- Around line 4246-4449: These two filter-focused tests (the ones named
"tokenizeSubtitle clears annotations for ja-nai explanatory endings while
preserving lexical content" and "tokenizeSubtitle clears annotations for
standalone polite copula endings without POS metadata") should be moved out of
the large tokenizer.test.ts and into a new focused test file named
annotation-stage.test.ts; copy the tests verbatim but update their imports to
reference the same helpers (tokenizeSubtitle, makeDepsFromYomitanTokens,
PartOfSpeech) from the module exports used in the original file, ensure any test
helpers remain exported/imported so the tests compile, and run the suite to
confirm the new test file is detected by the runner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ee2588e5-c33c-491e-854f-98eeb9b483fc
⛔ Files ignored due to path filters (20)
backlog/tasks/task-304 - Fix-N1-sentence-boundary-counting-across-Yomitan-punctuation-gaps.mdis excluded by!backlog/**backlog/tasks/task-305 - Use-Yomitan-word-classes-for-subtitle-token-POS-filtering.mdis excluded by!backlog/**backlog/tasks/task-306 - Fix-Hyprland-fullscreen-overlay-geometry-and-hover-pause.mdis excluded by!backlog/**backlog/tasks/task-307 - Exclude-kana-only-words-from-N1-subtitle-targets.mdis excluded by!backlog/**backlog/tasks/task-308 - Restore-persistent-JLPT-subtitle-underlines.mdis excluded by!backlog/**backlog/tasks/task-309 - Accept-modified-follow-up-digits-for-multi-line-sentence-mining.mdis excluded by!backlog/**backlog/tasks/task-310 - Suppress-N1-highlight-for-kana-only-candidate-sentences.mdis excluded by!backlog/**backlog/tasks/task-311 - Suppress-auxiliary-inflection-fragments-from-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-312 - Suppress-ja-nai-explanatory-ending-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-315 - Suppress-annotations-for-standalone-じゃない-and-です-ending-tokens.mdis excluded by!backlog/**backlog/tasks/task-316 - Fix-macOS-launcher-playback-exit-with-background-stats-daemon.mdis excluded by!backlog/**changes/305-tokenizer-word-class-pos-filtering.mdis excluded by!changes/**changes/306-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/307-kana-nplusone-targets.mdis excluded by!changes/**changes/308-jlpt-underlines.mdis excluded by!changes/**changes/309-multi-mine-modified-digits.mdis excluded by!changes/**changes/311-auxiliary-inflection-annotation-filter.mdis excluded by!changes/**changes/312-grammar-ending-annotation-filter.mdis excluded by!changes/**changes/316-macos-playback-stats-daemon.mdis excluded by!changes/**docs-site/troubleshooting.mdis excluded by!docs-site/**
📒 Files selected for processing (44)
package.jsonplugin/subminer/session_bindings.luascripts/test-plugin-session-bindings.luasrc/core/services/mpv-properties.tssrc/core/services/mpv-protocol.test.tssrc/core/services/mpv-protocol.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.test.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/core/services/tokenizer/parser-selection-stage.test.tssrc/core/services/tokenizer/parser-selection-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/core/services/tokenizer/yomitan-parser-runtime.test.tssrc/core/services/tokenizer/yomitan-parser-runtime.tssrc/main.tssrc/main/runtime/app-lifecycle-actions.test.tssrc/main/runtime/app-lifecycle-actions.tssrc/main/runtime/app-lifecycle-main-cleanup.test.tssrc/main/runtime/app-lifecycle-main-cleanup.tssrc/main/runtime/composers/startup-lifecycle-composer.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/main/runtime/mpv-client-event-bindings.test.tssrc/main/runtime/mpv-client-event-bindings.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/main/runtime/stats-server-routing.test.tssrc/main/runtime/stats-server-routing.tssrc/renderer/handlers/keyboard.test.tssrc/renderer/handlers/keyboard.tssrc/renderer/handlers/mouse.test.tssrc/renderer/handlers/mouse.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.ts
✅ Files skipped from review due to trivial changes (21)
- src/core/services/mpv-properties.ts
- src/main/runtime/composers/startup-lifecycle-composer.test.ts
- src/core/services/tokenizer/parser-enrichment-stage.test.ts
- src/main/runtime/mpv-client-event-bindings.ts
- src/core/services/mpv-protocol.test.ts
- src/main/runtime/app-lifecycle-main-cleanup.test.ts
- src/main/runtime/mpv-client-event-bindings.test.ts
- src/renderer/handlers/keyboard.test.ts
- src/renderer/style.css
- src/core/services/overlay-window.ts
- src/main/runtime/stats-server-routing.test.ts
- src/main/runtime/mpv-main-event-main-deps.ts
- src/renderer/handlers/keyboard.ts
- src/core/services/tokenizer/yomitan-parser-runtime.test.ts
- plugin/subminer/session_bindings.lua
- src/core/services/mpv.ts
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/core/services/tokenizer/annotation-stage.ts
- src/main/runtime/mpv-main-event-main-deps.test.ts
- src/core/services/mpv.test.ts
- src/renderer/handlers/mouse.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/core/services/mpv-protocol.ts
- src/main/runtime/mpv-main-event-bindings.ts
- src/main/runtime/stats-server-routing.ts
- package.json
- src/main/runtime/app-lifecycle-actions.ts
- src/main/runtime/app-lifecycle-main-cleanup.ts
- src/main/runtime/app-lifecycle-actions.test.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts
- src/window-trackers/hyprland-tracker.test.ts
- src/window-trackers/hyprland-tracker.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts
- src/token-merger.ts
- src/core/services/tokenizer/subtitle-annotation-filter.ts
- src/core/services/tokenizer/annotation-stage.test.ts
ksyasuda
force-pushed
the
tokenizer-updates
branch
from
738ae17 to
910c240
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/runtime/mpv-main-event-main-deps.ts`:
- Around line 85-93: The two checks are inconsistent:
hasInitialPlaybackQuitOnDisconnectArg() includes managedPlayback, jellyfinPlay,
and youtubePlay, but shouldQuitOnDisconnectWhenOverlayRuntimeInitialized() only
checks managedPlayback and youtubePlay; update
shouldQuitOnDisconnectWhenOverlayRuntimeInitialized (in
mpv-main-event-main-deps.ts) to use the same predicate as
hasInitialPlaybackQuitOnDisconnectArg by checking
deps.appState.initialArgs?.managedPlayback ||
deps.appState.initialArgs?.jellyfinPlay ||
deps.appState.initialArgs?.youtubePlay so the overlay-initialized path arms
quit-on-disconnect the same way.
In `@src/renderer/handlers/mouse.test.ts`:
- Around line 1346-1354: The test currently stubs document.elementFromPoint to
always return ctx.dom.subtitleContainer so it doesn't actually validate that
handlePrimaryMouseEnter uses event.clientX/Y; change the stub to inspect its (x,
y) args and return ctx.dom.subtitleContainer only when those coordinates fall
inside the subtitle container's bounds (use
ctx.dom.subtitleContainer.getBoundingClientRect() or an explicit rect on
ctx.dom), otherwise return null; update the similar stub at the other location
mentioned (around the block at 1373-1379) so both tests perform real hit-testing
based on event coordinates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ced4bfa8-fa8b-42c1-b28e-cc1f31ca4b27
⛔ Files ignored due to path filters (26)
backlog/tasks/task-304 - Fix-N1-sentence-boundary-counting-across-Yomitan-punctuation-gaps.mdis excluded by!backlog/**backlog/tasks/task-305 - Use-Yomitan-word-classes-for-subtitle-token-POS-filtering.mdis excluded by!backlog/**backlog/tasks/task-306 - Fix-Hyprland-fullscreen-overlay-geometry-and-hover-pause.mdis excluded by!backlog/**backlog/tasks/task-307 - Exclude-kana-only-words-from-N1-subtitle-targets.mdis excluded by!backlog/**backlog/tasks/task-308 - Restore-persistent-JLPT-subtitle-underlines.mdis excluded by!backlog/**backlog/tasks/task-309 - Accept-modified-follow-up-digits-for-multi-line-sentence-mining.mdis excluded by!backlog/**backlog/tasks/task-310 - Suppress-N1-highlight-for-kana-only-candidate-sentences.mdis excluded by!backlog/**backlog/tasks/task-311 - Suppress-auxiliary-inflection-fragments-from-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-312 - Suppress-ja-nai-explanatory-ending-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-315 - Suppress-annotations-for-standalone-じゃない-and-です-ending-tokens.mdis excluded by!backlog/**backlog/tasks/task-316 - Fix-macOS-launcher-playback-exit-with-background-stats-daemon.mdis excluded by!backlog/**backlog/tasks/task-318 - Keep-JLPT-underline-color-fixed-after-lookup-selection.mdis excluded by!backlog/**backlog/tasks/task-319 - Suppress-annotations-for-expressive-interjection-subtitles.mdis excluded by!backlog/**backlog/tasks/task-320 - Refresh-current-subtitle-known-word-highlight-after-successful-mining.mdis excluded by!backlog/**changes/305-tokenizer-word-class-pos-filtering.mdis excluded by!changes/**changes/306-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/307-kana-nplusone-targets.mdis excluded by!changes/**changes/308-jlpt-underlines.mdis excluded by!changes/**changes/309-multi-mine-modified-digits.mdis excluded by!changes/**changes/311-auxiliary-inflection-annotation-filter.mdis excluded by!changes/**changes/312-grammar-ending-annotation-filter.mdis excluded by!changes/**changes/316-macos-playback-stats-daemon.mdis excluded by!changes/**changes/318-jlpt-underline-selection.mdis excluded by!changes/**changes/319-interjection-annotation-filter.mdis excluded by!changes/**changes/320-current-subtitle-known-highlight.mdis excluded by!changes/**docs-site/troubleshooting.mdis excluded by!docs-site/**
📒 Files selected for processing (47)
package.jsonplugin/subminer/session_bindings.luascripts/test-plugin-session-bindings.luasrc/anki-integration.test.tssrc/anki-integration.tssrc/anki-integration/known-word-cache.tssrc/core/services/mpv-properties.tssrc/core/services/mpv-protocol.test.tssrc/core/services/mpv-protocol.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.test.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/core/services/tokenizer/parser-selection-stage.test.tssrc/core/services/tokenizer/parser-selection-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/core/services/tokenizer/yomitan-parser-runtime.test.tssrc/core/services/tokenizer/yomitan-parser-runtime.tssrc/main.tssrc/main/runtime/app-lifecycle-actions.test.tssrc/main/runtime/app-lifecycle-actions.tssrc/main/runtime/app-lifecycle-main-cleanup.test.tssrc/main/runtime/app-lifecycle-main-cleanup.tssrc/main/runtime/composers/startup-lifecycle-composer.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/main/runtime/mpv-client-event-bindings.test.tssrc/main/runtime/mpv-client-event-bindings.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/main/runtime/stats-server-routing.test.tssrc/main/runtime/stats-server-routing.tssrc/renderer/handlers/keyboard.test.tssrc/renderer/handlers/keyboard.tssrc/renderer/handlers/mouse.test.tssrc/renderer/handlers/mouse.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.ts
✅ Files skipped from review due to trivial changes (11)
- src/core/services/overlay-window.ts
- src/main/runtime/mpv-main-event-main-deps.test.ts
- src/renderer/handlers/keyboard.ts
- src/renderer/handlers/keyboard.test.ts
- package.json
- src/main/runtime/composers/startup-lifecycle-composer.test.ts
- src/renderer/style.css
- scripts/test-plugin-session-bindings.lua
- src/main/runtime/stats-server-routing.test.ts
- src/core/services/mpv.test.ts
- src/core/services/mpv-protocol.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts
- src/main/runtime/mpv-main-event-bindings.ts
- src/renderer/subtitle-render.test.ts
- src/core/services/mpv.ts
- plugin/subminer/session_bindings.lua
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/core/services/tokenizer/annotation-stage.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts
- src/core/services/tokenizer/subtitle-annotation-filter.ts
- src/window-trackers/hyprland-tracker.test.ts
- src/main/runtime/app-lifecycle-actions.ts
- src/main/runtime/mpv-client-event-bindings.ts
- src/core/services/tokenizer/yomitan-parser-runtime.test.ts
- src/core/services/tokenizer/parser-enrichment-stage.test.ts
- src/renderer/handlers/mouse.ts
- src/window-trackers/hyprland-tracker.ts
- src/main/runtime/stats-server-routing.ts
- src/core/services/tokenizer/annotation-stage.test.ts
- src/main/runtime/mpv-client-event-bindings.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/core/services/tokenizer/parser-selection-stage.ts (1)
145-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't require
headwordsbefore splitting standalone endings.The later split path already falls back to
segmentTextwhen no headword is present, so thisheadword.length > 0gate makestext-onlyです/じゃないsegments stay merged into the previous token even thoughYomitanParseSegment.headwordsis optional.Suggested fix
function isStandaloneGrammarEndingSegment(segment: YomitanParseSegment): boolean { const surface = segment.text?.trim() ?? ''; const headword = extractYomitanHeadword(segment).trim(); - return ( - headword.length > 0 && - (isStandaloneGrammarEndingText(surface) || isStandaloneGrammarEndingText(headword)) - ); + return isStandaloneGrammarEndingText(surface) || isStandaloneGrammarEndingText(headword); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/parser-selection-stage.ts` around lines 145 - 150, The guard requiring a non-empty headword in isStandaloneGrammarEndingSegment prevents text-only endings (e.g. "です", "じゃない") from being recognized; remove the headword.length > 0 check so the function returns true when either the trimmed segment.text (surface) or the extracted headword matches via isStandaloneGrammarEndingText, using extractYomitanHeadword(segment) and segment.text as-is to allow the existing fallback path to handle missing headwords.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/anki-integration/card-creation.ts`:
- Around line 251-258: The manual-update path is still overwriting expression
audio because sentenceAudioField may be the fallback config.fields.audio even
when the note lacks a sentence-audio field; change the logic so you only write
updatedFields[sentenceAudioField] when that field actually exists on the note.
After computing sentenceAudioField (from getResolvedSentenceAudioFieldName())
add a guard that checks noteInfo.fields.hasOwnProperty(sentenceAudioField) (or
similar existence check) and skip the mergeFieldValue/update if the field is not
present; keep using deps.mergeFieldValue for the real sentence-audio field. Add
a regression test that runs the manual clipboard update on a note missing
SentenceAudio to ensure no ExpressionAudio is overwritten.
In `@src/core/services/tokenizer/subtitle-annotation-filter.ts`:
- Around line 17-20: Remove the literal phrases 'たって' and 'だって' from the broad
blacklist (SUBTITLE_ANNOTATION_EXCLUDED_TERMS) so they don't suppress tokens
whose reading normalizes to those strings; instead rely only on the dedicated
STANDALONE_GRAMMAR_PARTICLE_PHRASES / STANDALONE_GRAMMAR_PARTICLE_PHRASES_SET
and the isStandaloneGrammarParticle() helper to filter real standalone-particle
cases, and keep isExcludedByTerm() behavior unchanged (it should continue to
check both term and reading but no longer match these two phrases via the broad
blacklist).
---
Duplicate comments:
In `@src/core/services/tokenizer/parser-selection-stage.ts`:
- Around line 145-150: The guard requiring a non-empty headword in
isStandaloneGrammarEndingSegment prevents text-only endings (e.g. "です", "じゃない")
from being recognized; remove the headword.length > 0 check so the function
returns true when either the trimmed segment.text (surface) or the extracted
headword matches via isStandaloneGrammarEndingText, using
extractYomitanHeadword(segment) and segment.text as-is to allow the existing
fallback path to handle missing headwords.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d8ed1cb0-6c1f-4a43-a724-cef7d9057eeb
⛔ Files ignored due to path filters (7)
backlog/tasks/task-315 - Suppress-annotations-for-standalone-じゃない-and-です-ending-tokens.mdis excluded by!backlog/**backlog/tasks/task-321 - Preserve-word-audio-during-manual-clipboard-card-updates.mdis excluded by!backlog/**changes/321-grammar-ending-pattern-filter.mdis excluded by!changes/**changes/322-preserve-word-audio-manual-update.mdis excluded by!changes/**docs-site/anki-integration.mdis excluded by!docs-site/**docs-site/configuration.mdis excluded by!docs-site/**docs-site/mining-workflow.mdis excluded by!docs-site/**
📒 Files selected for processing (8)
src/anki-integration/card-creation-manual-update.test.tssrc/anki-integration/card-creation.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/grammar-ending.tssrc/core/services/tokenizer/parser-selection-stage.test.tssrc/core/services/tokenizer/parser-selection-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/services/tokenizer/annotation-stage.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/runtime/mpv-main-event-main-deps.ts (1)
85-97: ⚡ Quick winDeduplicate quit-on-disconnect predicate to prevent drift.
Both methods now match (great), but the duplicated condition is easy to desync again. Please extract a shared local predicate and reuse it.
♻️ Proposed refactor
+ const hasInitialPlaybackQuitOnDisconnectArg = (): boolean => + Boolean( + deps.appState.initialArgs?.managedPlayback || + deps.appState.initialArgs?.jellyfinPlay || + deps.appState.initialArgs?.youtubePlay, + ); + return () => ({ @@ - hasInitialPlaybackQuitOnDisconnectArg: () => - Boolean( - deps.appState.initialArgs?.managedPlayback || - deps.appState.initialArgs?.jellyfinPlay || - deps.appState.initialArgs?.youtubePlay, - ), + hasInitialPlaybackQuitOnDisconnectArg, @@ - shouldQuitOnDisconnectWhenOverlayRuntimeInitialized: () => - Boolean( - deps.appState.initialArgs?.managedPlayback || - deps.appState.initialArgs?.jellyfinPlay || - deps.appState.initialArgs?.youtubePlay, - ), + shouldQuitOnDisconnectWhenOverlayRuntimeInitialized: + hasInitialPlaybackQuitOnDisconnectArg,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/runtime/mpv-main-event-main-deps.ts` around lines 85 - 97, The two duplicated predicates in hasInitialPlaybackQuitOnDisconnectArg and shouldQuitOnDisconnectWhenOverlayRuntimeInitialized should be extracted into a shared predicate and reused to avoid drift: create a local helper (e.g., const initialPlaybackQuitOnDisconnect = () => Boolean(deps.appState.initialArgs?.managedPlayback || deps.appState.initialArgs?.jellyfinPlay || deps.appState.initialArgs?.youtubePlay) or a simple boolean constant computed once) and replace the existing inline Boolean(...) expressions in both hasInitialPlaybackQuitOnDisconnectArg and shouldQuitOnDisconnectWhenOverlayRuntimeInitialized to call/return that shared symbol; ensure isOverlayRuntimeInitialized remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/runtime/mpv-main-event-main-deps.ts`:
- Around line 85-97: The two duplicated predicates in
hasInitialPlaybackQuitOnDisconnectArg and
shouldQuitOnDisconnectWhenOverlayRuntimeInitialized should be extracted into a
shared predicate and reused to avoid drift: create a local helper (e.g., const
initialPlaybackQuitOnDisconnect = () =>
Boolean(deps.appState.initialArgs?.managedPlayback ||
deps.appState.initialArgs?.jellyfinPlay ||
deps.appState.initialArgs?.youtubePlay) or a simple boolean constant computed
once) and replace the existing inline Boolean(...) expressions in both
hasInitialPlaybackQuitOnDisconnectArg and
shouldQuitOnDisconnectWhenOverlayRuntimeInitialized to call/return that shared
symbol; ensure isOverlayRuntimeInitialized remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09f6fde7-e263-4f27-a0e0-3065ef275458
⛔ Files ignored due to path filters (2)
backlog/tasks/task-322 - Fix-failing-CI-checks-on-PR-57.mdis excluded by!backlog/**changes/319-interjection-annotation-filter.mdis excluded by!changes/**
📒 Files selected for processing (7)
scripts/test-plugin-session-bindings.luasrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/renderer/handlers/mouse.test.tssrc/renderer/subtitle-render.test.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/test-plugin-session-bindings.lua
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/core/services/tokenizer/annotation-stage.ts
- src/core/services/tokenizer.ts
- src/renderer/handlers/mouse.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/subminer/process.lua (1)
298-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the startup gate for duplicate auto-starts while readiness is still pending.
Line 302 unconditionally disarms the gate. If
file-loadedfires again beforesubminer-autoplay-ready, this path resumes playback early and bypasses the intended pause-until-ready behavior.💡 Suggested fix
if state.overlay_running then if overrides.auto_start_trigger == true then subminer_log("debug", "process", "Auto-start ignored because overlay is already running") local socket_path = overrides.socket_path or opts.socket_path - disarm_auto_play_ready_gate() + if not state.auto_play_ready_gate_armed then + disarm_auto_play_ready_gate() + end local visibility_action = resolve_visible_overlay_startup() and "show-visible-overlay" or "hide-visible-overlay" run_control_command_async(visibility_action, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/subminer/process.lua` around lines 298 - 303, In the branch where state.overlay_running and overrides.auto_start_trigger are true, do not unconditionally call disarm_auto_play_ready_gate(); instead first call resolve_visible_overlay_startup() and inspect its return (e.g., visibility_action) and only call disarm_auto_play_ready_gate() when the action indicates the overlay should start immediately (not when it indicates defer/wait), so duplicate file-loaded events won't prematurely release the auto-play gate; update the sequence around state.overlay_running, overrides.auto_start_trigger, disarm_auto_play_ready_gate(), and resolve_visible_overlay_startup() accordingly.
🧹 Nitpick comments (2)
src/main/runtime/mpv-main-event-main-deps.ts (1)
85-97: ⚡ Quick winDeduplicate quit-on-disconnect predicate to prevent future drift.
Both exported checks now match, but they duplicate the same condition and can diverge again during future edits.
♻️ Proposed refactor
+ const hasInitialPlaybackQuitOnDisconnectArg = (): boolean => + Boolean( + deps.appState.initialArgs?.managedPlayback || + deps.appState.initialArgs?.jellyfinPlay || + deps.appState.initialArgs?.youtubePlay, + ); + return () => ({ @@ - hasInitialPlaybackQuitOnDisconnectArg: () => - Boolean( - deps.appState.initialArgs?.managedPlayback || - deps.appState.initialArgs?.jellyfinPlay || - deps.appState.initialArgs?.youtubePlay, - ), + hasInitialPlaybackQuitOnDisconnectArg, @@ - shouldQuitOnDisconnectWhenOverlayRuntimeInitialized: () => - Boolean( - deps.appState.initialArgs?.managedPlayback || - deps.appState.initialArgs?.jellyfinPlay || - deps.appState.initialArgs?.youtubePlay, - ), + shouldQuitOnDisconnectWhenOverlayRuntimeInitialized: hasInitialPlaybackQuitOnDisconnectArg,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/runtime/mpv-main-event-main-deps.ts` around lines 85 - 97, Extract the duplicated quit-on-disconnect check into a single shared predicate and have both hasInitialPlaybackQuitOnDisconnectArg and shouldQuitOnDisconnectWhenOverlayRuntimeInitialized call it; specifically, create a helper (e.g., initialPlaybackQuitOnDisconnect or getInitialPlaybackQuitOnDisconnect) that returns Boolean(deps.appState.initialArgs?.managedPlayback || deps.appState.initialArgs?.jellyfinPlay || deps.appState.initialArgs?.youtubePlay) and replace the duplicated inline logic in both exported checks to call that helper so the condition is defined in one place and cannot drift.scripts/test-plugin-start-gate.lua (1)
898-903: ⚡ Quick winAdd a duplicate
file-loaded-before-ready regression case.This scenario currently calls the second
file-loadedonly after the first ready signal. Add a variant where the secondfile-loadedfires beforesubminer-autoplay-readyto protect pause-until-ready semantics from premature gate disarm regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-plugin-start-gate.lua` around lines 898 - 903, Add a regression variant that fires the second file-loaded before the ready signal to ensure pause-until-ready semantics hold: duplicate the existing sequence around fire_event(recorded, "file-loaded") and recorded.script_messages["subminer-autoplay-ready"]() so that you call fire_event(recorded, "file-loaded") a second time prior to invoking recorded.script_messages["subminer-autoplay-ready"](), then invoke the ready handler and repeat the same assertions; locate the block using fire_event, recorded, and the "subminer-autoplay-ready" key to insert the new pre-ready file-loaded case and reuse the same assert_true checks to validate behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugin/subminer/process.lua`:
- Around line 298-303: In the branch where state.overlay_running and
overrides.auto_start_trigger are true, do not unconditionally call
disarm_auto_play_ready_gate(); instead first call
resolve_visible_overlay_startup() and inspect its return (e.g.,
visibility_action) and only call disarm_auto_play_ready_gate() when the action
indicates the overlay should start immediately (not when it indicates
defer/wait), so duplicate file-loaded events won't prematurely release the
auto-play gate; update the sequence around state.overlay_running,
overrides.auto_start_trigger, disarm_auto_play_ready_gate(), and
resolve_visible_overlay_startup() accordingly.
---
Nitpick comments:
In `@scripts/test-plugin-start-gate.lua`:
- Around line 898-903: Add a regression variant that fires the second
file-loaded before the ready signal to ensure pause-until-ready semantics hold:
duplicate the existing sequence around fire_event(recorded, "file-loaded") and
recorded.script_messages["subminer-autoplay-ready"]() so that you call
fire_event(recorded, "file-loaded") a second time prior to invoking
recorded.script_messages["subminer-autoplay-ready"](), then invoke the ready
handler and repeat the same assertions; locate the block using fire_event,
recorded, and the "subminer-autoplay-ready" key to insert the new pre-ready
file-loaded case and reuse the same assert_true checks to validate behavior.
In `@src/main/runtime/mpv-main-event-main-deps.ts`:
- Around line 85-97: Extract the duplicated quit-on-disconnect check into a
single shared predicate and have both hasInitialPlaybackQuitOnDisconnectArg and
shouldQuitOnDisconnectWhenOverlayRuntimeInitialized call it; specifically,
create a helper (e.g., initialPlaybackQuitOnDisconnect or
getInitialPlaybackQuitOnDisconnect) that returns
Boolean(deps.appState.initialArgs?.managedPlayback ||
deps.appState.initialArgs?.jellyfinPlay ||
deps.appState.initialArgs?.youtubePlay) and replace the duplicated inline logic
in both exported checks to call that helper so the condition is defined in one
place and cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 92879433-e70b-44bb-a315-51df020b22e7
⛔ Files ignored due to path filters (6)
backlog/tasks/task-322 - Fix-failing-CI-checks-on-PR-57.mdis excluded by!backlog/**backlog/tasks/task-323 - Fix-macOS-overlay-hiding-while-mpv-remains-active.mdis excluded by!backlog/**backlog/tasks/task-324 - Fix-mpv-playlist-changes-re-running-app-warmups.mdis excluded by!backlog/**changes/319-interjection-annotation-filter.mdis excluded by!changes/**changes/323-macos-overlay-tracker-flaps.mdis excluded by!changes/**changes/324-mpv-playlist-overlay-reuse.mdis excluded by!changes/**
📒 Files selected for processing (11)
plugin/subminer/process.luascripts/test-plugin-session-bindings.luascripts/test-plugin-start-gate.luasrc/core/services/overlay-visibility.test.tssrc/core/services/overlay-visibility.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/renderer/handlers/mouse.test.tssrc/renderer/subtitle-render.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/renderer/handlers/mouse.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/services/tokenizer.ts
- src/core/services/tokenizer/annotation-stage.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/subminer/process.lua (1)
298-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t clear an active startup gate on duplicate auto-start.
When a duplicate
file-loadedarrives before readiness, Line 302 disarms the currently armed gate and resumes playback early. That bypassesauto_start_pause_until_readyfor the active startup cycle.Proposed fix
if state.overlay_running then if overrides.auto_start_trigger == true then subminer_log("debug", "process", "Auto-start ignored because overlay is already running") local socket_path = overrides.socket_path or opts.socket_path - disarm_auto_play_ready_gate() + -- Preserve an active startup gate; only clear stale/inactive state. + if not state.auto_play_ready_gate_armed then + disarm_auto_play_ready_gate() + end local visibility_action = resolve_visible_overlay_startup() and "show-visible-overlay" or "hide-visible-overlay"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/subminer/process.lua` around lines 298 - 303, The current branch disarms the startup gate unconditionally when a duplicate auto-start arrives while state.overlay_running, which lets playback resume early; change the logic so disarm_auto_play_ready_gate() is not called for duplicate triggers when state.overlay_running is true. Specifically, in the block handling overrides.auto_start_trigger and state.overlay_running, remove or skip the call to disarm_auto_play_ready_gate() (keep the log and socket_path resolution), and only call disarm_auto_play_ready_gate() from the path that actually starts a new overlay (the branch where state.overlay_running is false or where you initiate startup), ensuring auto_start_pause_until_ready remains enforced for the active startup cycle.
🧹 Nitpick comments (3)
src/renderer/subtitle-render.test.ts (1)
910-912: ⚡ Quick winHarden the “no underline” guard to catch shorthand
text-decoration.Current check blocks
text-decoration-line: underlinebut still allowstext-decoration: underline .... Add one more negative assertion so this regression can’t slip through.Proposed test hardening
assert.doesNotMatch(plainJlptBlock, /(?:^|\n)\s*color\s*:/m); assert.doesNotMatch(plainJlptBlock, /text-decoration-line:\s*underline;/); + assert.doesNotMatch(plainJlptBlock, /text-decoration\s*:[^;]*\bunderline\b/i); assert.match( plainJlptBlock, new RegExp(`border-bottom:\\s*2px\\s+solid\\s+var\\(--subtitle-jlpt-n${level}-color,`),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/subtitle-render.test.ts` around lines 910 - 912, The test currently ensures no 'text-decoration-line: underline' but misses shorthand 'text-decoration: underline'; update the assertions around plainJlptBlock (used in subtitle-render.test) to also assert.doesNotMatch(plainJlptBlock, /(?:^|\n)\s*text-decoration\s*:/) or a regex specifically blocking 'text-decoration:\s*underline' so any shorthand underline usage is rejected; modify the block where assert.doesNotMatch(plainJlptBlock, /text-decoration-line:\s*underline;/) appears to include this additional negative assertion referencing plainJlptBlock.src/main/runtime/mpv-main-event-actions.test.ts (1)
196-224: ⚡ Quick winAdd a rejection-path test for the new post-watch error handler.
This test only validates the success path. Please also assert that a rejected
maybeRunAnilistPostWatchUpdatecallslogErrorand does not break subsequent handler behavior.Proposed test addition
+test('time-pos handler logs when post-watch update rejects', async () => { + const calls: string[] = []; + const timeHandler = createHandleMpvTimePosChangeHandler({ + recordPlaybackPosition: (time) => calls.push(`time:${time}`), + reportJellyfinRemoteProgress: (force) => calls.push(`progress:${force ? 'force' : 'normal'}`), + refreshDiscordPresence: () => calls.push('presence'), + maybeRunAnilistPostWatchUpdate: async () => { + throw new Error('boom'); + }, + logError: () => calls.push('post-watch-error'), + }); + + timeHandler({ time: 12.5 }); + await Promise.resolve(); // allow catch microtask to run + + assert.deepEqual(calls, ['time:12.5', 'progress:normal', 'presence', 'post-watch-error']); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/runtime/mpv-main-event-actions.test.ts` around lines 196 - 224, The test currently only covers the success path for createHandleMpvTimePosChangeHandler's maybeRunAnilistPostWatchUpdate; add a counterpart that simulates maybeRunAnilistPostWatchUpdate rejecting and assert that the supplied logError is called and the rest of the handlers still run in order. Specifically, create a test that supplies a maybeRunAnilistPostWatchUpdate which returns a rejected promise, a logError spy that records calls, then invoke timeHandler({ time: ... }) followed by pauseHandler({ paused: ... }) and assert the calls array includes the 'post-watch-error' entry (or that logError was called) in place of the success 'post-watch' and that subsequent progress/presence/pause handlers still execute as before; use the same helper factories createHandleMpvTimePosChangeHandler and createHandleMpvPauseChangeHandler to locate the code under test.scripts/test-plugin-start-gate.lua (1)
909-925: ⚡ Quick winAdd a pre-ready duplicate case to lock gate semantics.
This scenario only tests a duplicate
file-loadedafter the first ready signal. Add a sibling case where the secondfile-loadedhappens beforesubminer-autoplay-ready, then assert pause remains armed until the first ready event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-plugin-start-gate.lua` around lines 909 - 925, Add a sibling test case in scripts/test-plugin-start-gate.lua that simulates a second "file-loaded" firing before the first "subminer-autoplay-ready" to verify that the pause-until-ready gate stays armed until the ready event; locate the existing duplicate case near the assertions using helpers like count_osd_message(recorded.osd, ...), count_property_set(recorded.property_sets, "pause", ...), and the event names "file-loaded" and "subminer-autoplay-ready", then create a new test sequence that triggers: initial file-loaded -> pause should be set true, trigger a duplicate file-loaded (pre-ready) -> assert pause remains true and no extra ready/tokenization messages are emitted yet, then emit subminer-autoplay-ready -> assert tokenization ready message appears once and pause is cleared once (count_property_set "pause", false == 1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/anilist/anilist-updater.ts`:
- Around line 239-245: The current return unconditionally substitutes
parseMediaInfo(target) values for season/episode when guessit (season/episode)
are undefined; instead, gate use of fallback.season and fallback.episode by the
parser confidence—only apply fallback values when parseMediaInfo(target)
indicates high confidence (e.g., confidence >= a safe threshold or a
highConfidence flag). Update the logic around the return in anilist-updater.ts
(the block that calls parseMediaInfo, buildGuessitTitle and returns
title/alternativeTitle/year/season/episode) to check fallback.confidence (or
equivalent) before assigning fallback.season/fallback.episode so low-confidence
parser guesses do not bypass the downstream guard in anilist-post-watch.ts.
---
Outside diff comments:
In `@plugin/subminer/process.lua`:
- Around line 298-303: The current branch disarms the startup gate
unconditionally when a duplicate auto-start arrives while state.overlay_running,
which lets playback resume early; change the logic so
disarm_auto_play_ready_gate() is not called for duplicate triggers when
state.overlay_running is true. Specifically, in the block handling
overrides.auto_start_trigger and state.overlay_running, remove or skip the call
to disarm_auto_play_ready_gate() (keep the log and socket_path resolution), and
only call disarm_auto_play_ready_gate() from the path that actually starts a new
overlay (the branch where state.overlay_running is false or where you initiate
startup), ensuring auto_start_pause_until_ready remains enforced for the active
startup cycle.
---
Nitpick comments:
In `@scripts/test-plugin-start-gate.lua`:
- Around line 909-925: Add a sibling test case in
scripts/test-plugin-start-gate.lua that simulates a second "file-loaded" firing
before the first "subminer-autoplay-ready" to verify that the pause-until-ready
gate stays armed until the ready event; locate the existing duplicate case near
the assertions using helpers like count_osd_message(recorded.osd, ...),
count_property_set(recorded.property_sets, "pause", ...), and the event names
"file-loaded" and "subminer-autoplay-ready", then create a new test sequence
that triggers: initial file-loaded -> pause should be set true, trigger a
duplicate file-loaded (pre-ready) -> assert pause remains true and no extra
ready/tokenization messages are emitted yet, then emit subminer-autoplay-ready
-> assert tokenization ready message appears once and pause is cleared once
(count_property_set "pause", false == 1).
In `@src/main/runtime/mpv-main-event-actions.test.ts`:
- Around line 196-224: The test currently only covers the success path for
createHandleMpvTimePosChangeHandler's maybeRunAnilistPostWatchUpdate; add a
counterpart that simulates maybeRunAnilistPostWatchUpdate rejecting and assert
that the supplied logError is called and the rest of the handlers still run in
order. Specifically, create a test that supplies a
maybeRunAnilistPostWatchUpdate which returns a rejected promise, a logError spy
that records calls, then invoke timeHandler({ time: ... }) followed by
pauseHandler({ paused: ... }) and assert the calls array includes the
'post-watch-error' entry (or that logError was called) in place of the success
'post-watch' and that subsequent progress/presence/pause handlers still execute
as before; use the same helper factories createHandleMpvTimePosChangeHandler and
createHandleMpvPauseChangeHandler to locate the code under test.
In `@src/renderer/subtitle-render.test.ts`:
- Around line 910-912: The test currently ensures no 'text-decoration-line:
underline' but misses shorthand 'text-decoration: underline'; update the
assertions around plainJlptBlock (used in subtitle-render.test) to also
assert.doesNotMatch(plainJlptBlock, /(?:^|\n)\s*text-decoration\s*:/) or a regex
specifically blocking 'text-decoration:\s*underline' so any shorthand underline
usage is rejected; modify the block where assert.doesNotMatch(plainJlptBlock,
/text-decoration-line:\s*underline;/) appears to include this additional
negative assertion referencing plainJlptBlock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e173e4ec-d560-4c1e-95ac-42223551abd9
⛔ Files ignored due to path filters (14)
backlog/tasks/task-322 - Fix-failing-CI-checks-on-PR-57.mdis excluded by!backlog/**backlog/tasks/task-323 - Fix-macOS-overlay-hiding-while-mpv-remains-active.mdis excluded by!backlog/**backlog/tasks/task-324 - Fix-mpv-playlist-changes-re-running-app-warmups.mdis excluded by!backlog/**backlog/tasks/task-325 - Keep-JLPT-underline-color-fixed-with-combined-lookup-annotations.mdis excluded by!backlog/**backlog/tasks/task-326 - Fix-AniList-post-watch-update-after-skipped-completion-threshold.mdis excluded by!backlog/**backlog/tasks/task-327 - Restore-stats-daemon-deferral-when-launching-playback.mdis excluded by!backlog/**backlog/tasks/task-328 - Keep-subtitle-prefetch-running-after-immediate-cached-annotation-render.mdis excluded by!backlog/**changes/319-interjection-annotation-filter.mdis excluded by!changes/**changes/323-macos-overlay-tracker-flaps.mdis excluded by!changes/**changes/324-mpv-playlist-overlay-reuse.mdis excluded by!changes/**changes/325-jlpt-combined-annotation-underlines.mdis excluded by!changes/**changes/326-anilist-time-position-post-watch.mdis excluded by!changes/**changes/327-stats-daemon-deferral.mdis excluded by!changes/**changes/328-subtitle-prefetch-cache-hit-resume.mdis excluded by!changes/**
📒 Files selected for processing (19)
plugin/subminer/process.luascripts/test-plugin-session-bindings.luascripts/test-plugin-start-gate.luasrc/core/services/anilist/anilist-updater.test.tssrc/core/services/anilist/anilist-updater.tssrc/core/services/overlay-visibility.test.tssrc/core/services/overlay-visibility.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/main/runtime/mpv-main-event-actions.test.tssrc/main/runtime/mpv-main-event-actions.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/main/runtime/stats-server-routing.test.tssrc/main/runtime/stats-server-routing.tssrc/renderer/handlers/mouse.test.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/main/runtime/mpv-main-event-main-deps.ts
- src/core/services/tokenizer/annotation-stage.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/renderer/handlers/mouse.test.ts
- src/core/services/tokenizer.ts
- scripts/test-plugin-session-bindings.lua
ksyasuda
force-pushed
the
tokenizer-updates
branch
from
a143ad1 to
5f7c5f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/anki-integration/known-word-cache.ts (1)
174-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn value can miss cache-reset mutations.
Line 174 can clear cache state, but Lines 183/189 may still return
false. Since downstream notifications now key off this boolean, real cache changes can be dropped.Suggested fix
appendFromNoteInfo(noteInfo: KnownWordCacheNoteInfo): boolean { if (!this.isKnownWordCacheEnabled() || !this.shouldAddMinedWordsImmediately()) { return false; } + let didMutateCache = false; const currentStateKey = this.getKnownWordCacheStateKey(); if (this.knownWordsStateKey && this.knownWordsStateKey !== currentStateKey) { + didMutateCache = this.knownWords.size > 0 || this.noteWordsById.size > 0; this.clearKnownWordCacheState(); } if (!this.knownWordsStateKey) { this.knownWordsStateKey = currentStateKey; } const preferredFields = this.getImmediateAppendFields(); if (!preferredFields) { - return false; + return didMutateCache; } const nextWords = this.extractNormalizedKnownWordsFromNoteInfo(noteInfo, preferredFields); const changed = this.replaceNoteSnapshot(noteInfo.noteId, nextWords); if (!changed) { - return false; + return didMutateCache; } if (this.knownWordsLastRefreshedAtMs <= 0) { this.knownWordsLastRefreshedAtMs = Date.now(); } @@ - return true; + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/anki-integration/known-word-cache.ts` around lines 174 - 190, The code may clear cache state via this.clearKnownWordCacheState() but still return false later, dropping a real mutation; modify the logic in the method that contains this.knownWordsStateKey checks so the result reflects cache mutations: introduce a local changed flag (or set changed = true when this.clearKnownWordCacheState() runs), then continue to compute nextWords and call this.replaceNoteSnapshot(noteInfo.noteId, nextWords) and return the boolean OR of the clear-induced change and replaceNoteSnapshot result. Ensure references to knownWordsStateKey, clearKnownWordCacheState(), getImmediateAppendFields(), extractNormalizedKnownWordsFromNoteInfo(), and replaceNoteSnapshot() are used to locate and update the code.src/main/runtime/anilist-post-watch.ts (1)
125-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAcquire
inFlightbefore async gating to prevent duplicate AniList writes.At Line 126 you read
getInFlight(), butsetInFlight(true)is only set at Line 172 after awaited work. Two concurrent calls can both pass the guard and race into duplicate AniList updates.Proposed fix
export function createMaybeRunAnilistPostWatchUpdateHandler(deps: { @@ }) { return async (options: AnilistPostWatchRunOptions = {}): Promise<void> => { if (deps.getInFlight()) { return; } + deps.setInFlight(true); - const force = options.force === true; - - const resolved = deps.getResolvedConfig(); - if (!deps.isAnilistTrackingEnabled(resolved)) { - return; - } + try { + const force = options.force === true; + + const resolved = deps.getResolvedConfig(); + if (!deps.isAnilistTrackingEnabled(resolved)) { + return; + } @@ - deps.setInFlight(true); - try { await deps.processNextAnilistRetryUpdate(); @@ - } finally { - deps.setInFlight(false); - } + } finally { + deps.setInFlight(false); + } }; }
🧹 Nitpick comments (1)
src/core/services/ipc.test.ts (1)
305-351: ⚡ Quick winAdd a rejection-path test for the manual-mark AniList callback.
Please add a case where
runAnilistPostWatchUpdateOnManualMarkthrows and verify the handler still returnstruefor a successful mark. This will lock in failure isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/ipc.test.ts` around lines 305 - 351, Add a new test that verifies when markActiveVideoWatched returns true but runAnilistPostWatchUpdateOnManualMark throws, the IPC handler still returns true and the thrown error is isolated; use the existing test setup (createFakeIpcRegistrar, createRegisterIpcDeps, createFakeImmersionTracker) but make runAnilistPostWatchUpdateOnManualMark async () => { throw new Error('..') } and assert the result from handlers.handle.get(IPC_CHANNELS.command.markActiveVideoWatched)?.({}) is true and that calls include 'mark' (and optionally ensure 'anilist' is not required); place this alongside the other registerIpcHandlers tests so it covers the rejection path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/ipc.ts`:
- Around line 390-395: The IPC handler for
IPC_CHANNELS.command.markActiveVideoWatched currently lets exceptions from
deps.runAnilistPostWatchUpdateOnManualMark propagate and fail the whole call;
change the handler so that after awaiting
deps.immersionTracker?.markActiveVideoWatched() you still return the boolean
result even if the post-watch sync fails by wrapping the call to
runAnilistPostWatchUpdateOnManualMark in a try/catch (or otherwise handling
rejection) and logging the error (do not rethrow), so markActiveVideoWatched()
success is never invalidated by runAnilistPostWatchUpdateOnManualMark failures.
---
Outside diff comments:
In `@src/anki-integration/known-word-cache.ts`:
- Around line 174-190: The code may clear cache state via
this.clearKnownWordCacheState() but still return false later, dropping a real
mutation; modify the logic in the method that contains this.knownWordsStateKey
checks so the result reflects cache mutations: introduce a local changed flag
(or set changed = true when this.clearKnownWordCacheState() runs), then continue
to compute nextWords and call this.replaceNoteSnapshot(noteInfo.noteId,
nextWords) and return the boolean OR of the clear-induced change and
replaceNoteSnapshot result. Ensure references to knownWordsStateKey,
clearKnownWordCacheState(), getImmediateAppendFields(),
extractNormalizedKnownWordsFromNoteInfo(), and replaceNoteSnapshot() are used to
locate and update the code.
---
Nitpick comments:
In `@src/core/services/ipc.test.ts`:
- Around line 305-351: Add a new test that verifies when markActiveVideoWatched
returns true but runAnilistPostWatchUpdateOnManualMark throws, the IPC handler
still returns true and the thrown error is isolated; use the existing test setup
(createFakeIpcRegistrar, createRegisterIpcDeps, createFakeImmersionTracker) but
make runAnilistPostWatchUpdateOnManualMark async () => { throw new Error('..') }
and assert the result from
handlers.handle.get(IPC_CHANNELS.command.markActiveVideoWatched)?.({}) is true
and that calls include 'mark' (and optionally ensure 'anilist' is not required);
place this alongside the other registerIpcHandlers tests so it covers the
rejection path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2af1ebd9-8289-4fed-8d46-38f01e435574
⛔ Files ignored due to path filters (48)
backlog/tasks/task-304 - Fix-N1-sentence-boundary-counting-across-Yomitan-punctuation-gaps.mdis excluded by!backlog/**backlog/tasks/task-305 - Use-Yomitan-word-classes-for-subtitle-token-POS-filtering.mdis excluded by!backlog/**backlog/tasks/task-306 - Fix-Hyprland-fullscreen-overlay-geometry-and-hover-pause.mdis excluded by!backlog/**backlog/tasks/task-307 - Exclude-kana-only-words-from-N1-subtitle-targets.mdis excluded by!backlog/**backlog/tasks/task-308 - Restore-persistent-JLPT-subtitle-underlines.mdis excluded by!backlog/**backlog/tasks/task-309 - Accept-modified-follow-up-digits-for-multi-line-sentence-mining.mdis excluded by!backlog/**backlog/tasks/task-310 - Suppress-N1-highlight-for-kana-only-candidate-sentences.mdis excluded by!backlog/**backlog/tasks/task-311 - Suppress-auxiliary-inflection-fragments-from-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-312 - Suppress-ja-nai-explanatory-ending-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-315 - Suppress-annotations-for-standalone-じゃない-and-です-ending-tokens.mdis excluded by!backlog/**backlog/tasks/task-316 - Fix-macOS-launcher-playback-exit-with-background-stats-daemon.mdis excluded by!backlog/**backlog/tasks/task-318 - Keep-JLPT-underline-color-fixed-after-lookup-selection.mdis excluded by!backlog/**backlog/tasks/task-319 - Suppress-annotations-for-expressive-interjection-subtitles.mdis excluded by!backlog/**backlog/tasks/task-320 - Refresh-current-subtitle-known-word-highlight-after-successful-mining.mdis excluded by!backlog/**backlog/tasks/task-321 - Preserve-word-audio-during-manual-clipboard-card-updates.mdis excluded by!backlog/**backlog/tasks/task-322 - Fix-failing-CI-checks-on-PR-57.mdis excluded by!backlog/**backlog/tasks/task-323 - Fix-macOS-overlay-hiding-while-mpv-remains-active.mdis excluded by!backlog/**backlog/tasks/task-324 - Fix-mpv-playlist-changes-re-running-app-warmups.mdis excluded by!backlog/**backlog/tasks/task-325 - Keep-JLPT-underline-color-fixed-with-combined-lookup-annotations.mdis excluded by!backlog/**backlog/tasks/task-326 - Fix-AniList-post-watch-update-after-skipped-completion-threshold.mdis excluded by!backlog/**backlog/tasks/task-327 - Restore-stats-daemon-deferral-when-launching-playback.mdis excluded by!backlog/**backlog/tasks/task-328 - Keep-subtitle-prefetch-running-after-immediate-cached-annotation-render.mdis excluded by!backlog/**backlog/tasks/task-331 - Address-unresolved-CodeRabbit-comments-on-PR-57.mdis excluded by!backlog/**backlog/tasks/task-332 - Fix-subtitle-frequency-annotation-missing-ranks-shown-in-Yomitan-popup.mdis excluded by!backlog/**changes/305-tokenizer-word-class-pos-filtering.mdis excluded by!changes/**changes/306-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/307-kana-nplusone-targets.mdis excluded by!changes/**changes/308-jlpt-underlines.mdis excluded by!changes/**changes/309-multi-mine-modified-digits.mdis excluded by!changes/**changes/311-auxiliary-inflection-annotation-filter.mdis excluded by!changes/**changes/312-grammar-ending-annotation-filter.mdis excluded by!changes/**changes/316-macos-playback-stats-daemon.mdis excluded by!changes/**changes/318-jlpt-underline-selection.mdis excluded by!changes/**changes/319-interjection-annotation-filter.mdis excluded by!changes/**changes/320-current-subtitle-known-highlight.mdis excluded by!changes/**changes/321-grammar-ending-pattern-filter.mdis excluded by!changes/**changes/322-preserve-word-audio-manual-update.mdis excluded by!changes/**changes/323-macos-overlay-tracker-flaps.mdis excluded by!changes/**changes/324-mpv-playlist-overlay-reuse.mdis excluded by!changes/**changes/325-jlpt-combined-annotation-underlines.mdis excluded by!changes/**changes/326-anilist-time-position-post-watch.mdis excluded by!changes/**changes/327-stats-daemon-deferral.mdis excluded by!changes/**changes/328-subtitle-prefetch-cache-hit-resume.mdis excluded by!changes/**changes/332-subtitle-frequency-ordinal-prefix.mdis excluded by!changes/**docs-site/anki-integration.mdis excluded by!docs-site/**docs-site/configuration.mdis excluded by!docs-site/**docs-site/mining-workflow.mdis excluded by!docs-site/**docs-site/troubleshooting.mdis excluded by!docs-site/**
📒 Files selected for processing (64)
package.jsonplugin/subminer/lifecycle.luaplugin/subminer/process.luaplugin/subminer/session_bindings.luascripts/test-plugin-session-bindings.luascripts/test-plugin-start-gate.luasrc/anki-integration.test.tssrc/anki-integration.tssrc/anki-integration/card-creation-manual-update.test.tssrc/anki-integration/card-creation.tssrc/anki-integration/known-word-cache.tssrc/core/services/anilist/anilist-updater.test.tssrc/core/services/anilist/anilist-updater.tssrc/core/services/ipc.test.tssrc/core/services/ipc.tssrc/core/services/mpv-properties.tssrc/core/services/mpv-protocol.test.tssrc/core/services/mpv-protocol.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-visibility.test.tssrc/core/services/overlay-visibility.tssrc/core/services/overlay-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/grammar-ending.tssrc/core/services/tokenizer/parser-enrichment-stage.test.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/core/services/tokenizer/parser-selection-stage.test.tssrc/core/services/tokenizer/parser-selection-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/core/services/tokenizer/yomitan-parser-runtime.test.tssrc/core/services/tokenizer/yomitan-parser-runtime.tssrc/main.tssrc/main/dependencies.tssrc/main/runtime/anilist-post-watch.test.tssrc/main/runtime/anilist-post-watch.tssrc/main/runtime/app-lifecycle-actions.test.tssrc/main/runtime/app-lifecycle-actions.tssrc/main/runtime/app-lifecycle-main-cleanup.test.tssrc/main/runtime/app-lifecycle-main-cleanup.tssrc/main/runtime/composers/startup-lifecycle-composer.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/main/runtime/mpv-client-event-bindings.test.tssrc/main/runtime/mpv-client-event-bindings.tssrc/main/runtime/mpv-main-event-actions.test.tssrc/main/runtime/mpv-main-event-actions.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/main/runtime/stats-server-routing.test.tssrc/main/runtime/stats-server-routing.tssrc/renderer/handlers/keyboard.test.tssrc/renderer/handlers/keyboard.tssrc/renderer/handlers/mouse.test.tssrc/renderer/handlers/mouse.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.ts
✅ Files skipped from review due to trivial changes (19)
- package.json
- src/core/services/mpv.ts
- src/main/runtime/app-lifecycle-actions.test.ts
- src/main/runtime/stats-server-routing.ts
- src/core/services/overlay-visibility.test.ts
- src/main/runtime/composers/startup-lifecycle-composer.test.ts
- src/renderer/handlers/keyboard.test.ts
- src/main/runtime/mpv-client-event-bindings.ts
- src/main/runtime/mpv-main-event-bindings.ts
- src/core/services/tokenizer/parser-enrichment-stage.test.ts
- src/core/services/mpv.test.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts
- src/main/runtime/app-lifecycle-main-cleanup.test.ts
- src/core/services/tokenizer/parser-selection-stage.test.ts
- src/main/runtime/mpv-main-event-main-deps.test.ts
- src/core/services/tokenizer/subtitle-annotation-filter.ts
- src/token-merger.ts
- src/main/runtime/app-lifecycle-main-cleanup.ts
- src/window-trackers/hyprland-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- src/main/runtime/stats-server-routing.test.ts
- scripts/test-plugin-session-bindings.lua
- src/anki-integration/card-creation-manual-update.test.ts
- src/window-trackers/hyprland-tracker.test.ts
- plugin/subminer/session_bindings.lua
- src/core/services/overlay-window.ts
- src/anki-integration.test.ts
- src/renderer/handlers/mouse.test.ts
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/main/runtime/mpv-client-event-bindings.test.ts
- src/renderer/handlers/keyboard.ts
- src/core/services/tokenizer/parser-selection-stage.ts
- src/anki-integration/card-creation.ts
- src/main/runtime/app-lifecycle-actions.ts
- src/main/runtime/mpv-main-event-actions.ts
- src/core/services/anilist/anilist-updater.test.ts
- src/renderer/style.css
- src/core/services/anilist/anilist-updater.ts
- plugin/subminer/process.lua
- src/anki-integration.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts
ksyasuda
added a commit
that referenced
this pull request
May 4, 2026
- Acquire AniList post-watch in-flight lock before async gating to prevent duplicate writes - Isolate manual watched mark result from AniList post-watch callback failures - Report known-word cache clears as mutations during immediate append when state existed - Add regression tests for each fix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/core/services/tokenizer/annotation-stage.test.ts (1)
518-679: 🏗️ Heavy liftSplit this new subtitle-filter matrix into focused or table-driven tests.
These additions are valuable, but keeping dozens of near-identical exclusion/preservation cases inline in one file will make future failures noisy and the file harder to navigate. Please move this matrix into a dedicated subtitle-filter spec or collapse the repeated setup/assertions behind shared helpers. As per coding guidelines,
**/*.{ts,tsx}: TypeScript files should be kept small and follow existing patterns.Also applies to: 814-1032, 1361-1854
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/annotation-stage.test.ts` around lines 518 - 679, The new long matrix of near-duplicate tests for shouldExcludeTokenFromSubtitleAnnotations should be extracted into a focused, table-driven spec (e.g., subtitle-filter or subtitle-filter.spec) or collapsed behind a shared helper; create a new test file and move these cases (including the loops and uses of makeToken and assert.equal) into a single data-driven test that iterates an array of {surface, headword, reading, partOfSpeech, pos1, pos2, expected} entries, reusing makeToken and a small assertion helper, then delete the inlined blocks from this file to keep each TypeScript test file small and consistent with existing patterns.src/core/services/tokenizer.test.ts (1)
3078-3155: 🏗️ Heavy liftBreak these new tokenizer regressions into focused suites.
The new coverage is good, but adding it here pushes even more unrelated scenarios into an already very large spec. Please split the word-class, subtitle-filter, and N+1 regressions into dedicated test files/helpers so the main tokenizer test remains navigable and failures map to one concern at a time. As per coding guidelines,
**/*.{ts,tsx}: TypeScript files should be kept small and follow existing patterns.Also applies to: 3233-3285, 4309-4512, 4733-4977
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer.test.ts` around lines 3078 - 3155, The test file has grown with multiple unrelated tokenizer scenarios—split the new word-class, subtitle-filter, and N+1 regression tests into focused suites: create new test files (e.g., tokenizer.wordclass.test.ts, tokenizer.subtitle-filter.test.ts, tokenizer.nplusone.test.ts) and move the tests that call tokenizeSubtitle and makeDepsFromYomitanTokens (the three shown and the other ranges noted) into the appropriate new files; keep assertions referencing PartOfSpeech, tokenizeSubtitle, and makeDepsFromYomitanTokens intact, and update any imports/exports so each new file only contains the related tests to match existing test patterns and keep each spec small and focused.src/core/services/tokenizer/annotation-stage.ts (1)
152-203: 🏗️ Heavy liftExtract the frequency exception rules out of
annotation-stage.ts.
shouldAllowOrdinalPrefixNounFrequencyis another surface/POS-specific escape hatch in a file that already owns annotation orchestration, JLPT lookup, known-word resolution, and subtitle filtering. Please move the frequency-allowance heuristics into a dedicated filter/rule module so future exceptions stay local and testable. As per coding guidelines,**/*.{ts,tsx}: TypeScript files should be kept small and follow existing patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/tokenizer/annotation-stage.ts` around lines 152 - 203, The frequency-exemption heuristics (e.g., shouldAllowOrdinalPrefixNounFrequency) need to be extracted from annotation-stage.ts into a dedicated module (suggest name: frequencyRules or frequencyExemptions) so these surface/POS-specific allowlists are isolated and testable; move shouldAllowOrdinalPrefixNounFrequency and any related helpers (and their dependencies like splitNormalizedTagParts, normalizePos1Tag/normalizePos2Tag usages) into that module, export a single entrypoint (e.g., allowFrequencyForToken(token, pos1Exclusions, pos2Exclusions) or specific predicate functions) and update isFrequencyExcludedByPos to call the new exported function(s) instead of the in-file implementation while keeping existing calls to isSingleKanaFrequencyNoiseToken, shouldAllowContentLedMergedTokenFrequency, and isExcludedByTagSet unaffected; add unit tests for the new module covering ordinal-prefix and content-led merged-token cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/ipc.ts`:
- Around line 395-399: The catch block currently accesses (error as
Error).message which can throw if error is null/undefined; update the catch in
the markActiveVideoWatched/related handler in ipc.ts to safely derive the
message (e.g., check error instanceof Error and use error.message, otherwise
stringify or log the error value) and pass that safe string or the original
error to console.warn so logging itself cannot throw.
---
Nitpick comments:
In `@src/core/services/tokenizer.test.ts`:
- Around line 3078-3155: The test file has grown with multiple unrelated
tokenizer scenarios—split the new word-class, subtitle-filter, and N+1
regression tests into focused suites: create new test files (e.g.,
tokenizer.wordclass.test.ts, tokenizer.subtitle-filter.test.ts,
tokenizer.nplusone.test.ts) and move the tests that call tokenizeSubtitle and
makeDepsFromYomitanTokens (the three shown and the other ranges noted) into the
appropriate new files; keep assertions referencing PartOfSpeech,
tokenizeSubtitle, and makeDepsFromYomitanTokens intact, and update any
imports/exports so each new file only contains the related tests to match
existing test patterns and keep each spec small and focused.
In `@src/core/services/tokenizer/annotation-stage.test.ts`:
- Around line 518-679: The new long matrix of near-duplicate tests for
shouldExcludeTokenFromSubtitleAnnotations should be extracted into a focused,
table-driven spec (e.g., subtitle-filter or subtitle-filter.spec) or collapsed
behind a shared helper; create a new test file and move these cases (including
the loops and uses of makeToken and assert.equal) into a single data-driven test
that iterates an array of {surface, headword, reading, partOfSpeech, pos1, pos2,
expected} entries, reusing makeToken and a small assertion helper, then delete
the inlined blocks from this file to keep each TypeScript test file small and
consistent with existing patterns.
In `@src/core/services/tokenizer/annotation-stage.ts`:
- Around line 152-203: The frequency-exemption heuristics (e.g.,
shouldAllowOrdinalPrefixNounFrequency) need to be extracted from
annotation-stage.ts into a dedicated module (suggest name: frequencyRules or
frequencyExemptions) so these surface/POS-specific allowlists are isolated and
testable; move shouldAllowOrdinalPrefixNounFrequency and any related helpers
(and their dependencies like splitNormalizedTagParts,
normalizePos1Tag/normalizePos2Tag usages) into that module, export a single
entrypoint (e.g., allowFrequencyForToken(token, pos1Exclusions, pos2Exclusions)
or specific predicate functions) and update isFrequencyExcludedByPos to call the
new exported function(s) instead of the in-file implementation while keeping
existing calls to isSingleKanaFrequencyNoiseToken,
shouldAllowContentLedMergedTokenFrequency, and isExcludedByTagSet unaffected;
add unit tests for the new module covering ordinal-prefix and content-led
merged-token cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5b4eac4-9ec3-4032-acd7-7bd3bcbaee60
⛔ Files ignored due to path filters (4)
backlog/tasks/task-333 - Suppress-aru-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-334 - Assess-and-address-PR-57-latest-CodeRabbit-comments.mdis excluded by!backlog/**changes/333-aru-annotation-filter.mdis excluded by!changes/**changes/334-coderabbit-followups.mdis excluded by!changes/**
📒 Files selected for processing (10)
src/anki-integration/known-word-cache.test.tssrc/anki-integration/known-word-cache.tssrc/core/services/ipc.test.tssrc/core/services/ipc.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/main/runtime/anilist-post-watch.test.tssrc/main/runtime/anilist-post-watch.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/runtime/anilist-post-watch.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/anki-integration/known-word-cache.ts
- src/core/services/ipc.test.ts
- src/core/services/tokenizer/subtitle-annotation-filter.ts
Comment on lines
+395
to
+399
| } catch (error) { | ||
| console.warn( | ||
| 'Failed to run AniList post-watch update after manual watched mark:', | ||
| (error as Error).message, | ||
| ); |
There was a problem hiding this comment.
Harden catch logging to avoid re-throwing on non-Error values.
At Line 398, (error as Error).message can itself throw when error is null/undefined, which can still fail markActiveVideoWatched despite the try/catch intent.
Proposed fix
} catch (error) {
+ const errorMessage = error instanceof Error ? error.message : String(error);
console.warn(
'Failed to run AniList post-watch update after manual watched mark:',
- (error as Error).message,
+ errorMessage,
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/services/ipc.ts` around lines 395 - 399, The catch block currently
accesses (error as Error).message which can throw if error is null/undefined;
update the catch in the markActiveVideoWatched/related handler in ipc.ts to
safely derive the message (e.g., check error instanceof Error and use
error.message, otherwise stringify or log the error value) and pass that safe
string or the original error to console.warn so logging itself cannot throw.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/hyprland-window-placement.ts`:
- Around line 135-151: The synchronous hyprctl calls use the local alias run
(set from options.execFileSync ?? execFileSync) without a timeout, so wrap both
invocations—when fetching clients (run('hyprctl', ['-j', 'clients'], { encoding:
'utf-8' })) and when dispatching placement args (run('hyprctl', args, { stdio:
'ignore' }))—to include a timeout option (e.g., timeout: <ms>) to avoid blocking
the Electron main thread; add a clear constant name (e.g., HYPRCTL_TIMEOUT_MS)
if one doesn’t exist and pass it into both run calls, preserving any existing
options (encoding/stdio) while merging the timeout, and apply the same pattern
in findHyprlandWindowForPlacement call sites and the similar calls in
src/window-trackers/hyprland-tracker.ts.
In `@src/core/services/tokenizer/annotation-stage.ts`:
- Around line 624-631: The code unconditionally clears known-state by returning
{ ...strippedToken, isKnown: false } after calling
sharedStripSubtitleAnnotationMetadata, which strips annotation metadata for
excluded kanji tokens; instead preserve the original token's known state. Fix by
returning strippedToken as-is (or explicitly set isKnown to token.isKnown)
rather than forcing false—update the return near the call to
sharedStripSubtitleAnnotationMetadata (variables: token, strippedToken, property
isKnown).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e204c32e-6232-479c-9d34-65b5134616ff
⛔ Files ignored due to path filters (15)
backlog/completed/task-336 - Fix-Hyprland-fullscreen-overlay-downward-offset.mdis excluded by!backlog/**backlog/completed/task-339 - Stop-pinning-Hyprland-overlay-windows.mdis excluded by!backlog/**backlog/tasks/task-337 - Fix-transient-Linux-safeStorage-failure-poisoning-AniList-token-store.mdis excluded by!backlog/**backlog/tasks/task-338 - Fix-known-word-highlight-on-standalone-subtitle-particles.mdis excluded by!backlog/**backlog/tasks/task-340 - Restore-default-replay-and-next-subtitle-overlay-keybindings.mdis excluded by!backlog/**changes/336-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/336-overlay-focus-ring.mdis excluded by!changes/**changes/337-anilist-safe-storage-retry.mdis excluded by!changes/**changes/338-known-word-particle-highlights.mdis excluded by!changes/**changes/339-hyprland-overlay-pin.mdis excluded by!changes/**changes/340-default-subtitle-keybindings.mdis excluded by!changes/**docs-site/configuration.mdis excluded by!docs-site/**docs-site/public/config.example.jsoncis excluded by!docs-site/**docs-site/shortcuts.mdis excluded by!docs-site/**docs-site/usage.mdis excluded by!docs-site/**
📒 Files selected for processing (30)
config.example.jsoncpackage.jsonplugin/subminer/session_bindings.luascripts/test-plugin-session-bindings.luasrc/config/definitions/defaults-core.tssrc/config/definitions/domain-registry.test.tssrc/core/services/anilist/anilist-token-store.test.tssrc/core/services/anilist/anilist-token-store.tssrc/core/services/hyprland-window-placement.test.tssrc/core/services/hyprland-window-placement.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-manager.test.tssrc/core/services/overlay-window-config.test.tssrc/core/services/overlay-window-options.tssrc/core/services/overlay-window.tssrc/core/services/session-bindings.test.tssrc/core/services/stats-window-runtime.tssrc/core/services/stats-window.test.tssrc/core/services/stats-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/renderer/overlay-legacy-cleanup.test.tssrc/renderer/style.csssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.tsstats/src/App.tsxstats/src/styles/globals.cssstats/src/styles/globals.test.ts
💤 Files with no reviewable changes (1)
- src/core/services/anilist/anilist-token-store.ts
✅ Files skipped from review due to trivial changes (7)
- src/core/services/overlay-window-config.test.ts
- stats/src/styles/globals.test.ts
- stats/src/App.tsx
- src/config/definitions/domain-registry.test.ts
- src/window-trackers/hyprland-tracker.test.ts
- src/core/services/overlay-manager.test.ts
- src/window-trackers/hyprland-tracker.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/renderer/style.css
Comment on lines
+135
to
+151
| const run = options.execFileSync ?? execFileSync; | ||
| try { | ||
| const clients = parseHyprlandClients( | ||
| String(run('hyprctl', ['-j', 'clients'], { encoding: 'utf-8' })), | ||
| ); | ||
| const client = findHyprlandWindowForPlacement(clients, { | ||
| pid: options.pid ?? process.pid, | ||
| title: options.title, | ||
| }); | ||
| if (!client) { | ||
| return false; | ||
| } | ||
|
|
||
| const dispatches = buildHyprlandPlacementDispatches(client, options.bounds); | ||
| for (const args of dispatches) { | ||
| run('hyprctl', args, { stdio: 'ignore' }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== hyprctl / execFileSync usage =="
rg -n --type=ts -C2 "hyprctl|execFileSync|ensureHyprlandWindowFloatingByTitle"
echo
echo "== Direct run(...) command option blocks in hyprland-window-placement.ts =="
rg -n --type=ts -C3 "run\\('hyprctl'" src/core/services/hyprland-window-placement.tsRepository: ksyasuda/SubMiner
Length of output: 12538
🏁 Script executed:
#!/bin/bash
# Find all direct hyprctl invocations (execSync and execFileSync)
echo "=== All hyprctl calls with context ==="
rg -n "execSync|execFileSync" -A5 -B2 src/ --type=ts | grep -A5 -B2 "hyprctl"
echo ""
echo "=== Check Node.js version requirement ==="
if [ -f "package.json" ]; then
jq '.engines.node // .engines // "not specified"' package.json
fi
echo ""
echo "=== Timeout option in tests ==="
rg -n "timeout" src/core/services/hyprland-window-placement.test.ts --type=tsRepository: ksyasuda/SubMiner
Length of output: 1908
🏁 Script executed:
#!/bin/bash
# Check if there are existing examples of timeout usage in the codebase
echo "=== Existing timeout patterns in execFileSync/execSync ==="
rg -n "timeout\s*:" --type=ts | head -20
echo ""
echo "=== Check hyprland-tracker.ts for all hyprctl calls ==="
rg -n "hyprctl" src/window-trackers/hyprland-tracker.ts -A3 -B3 --type=tsRepository: ksyasuda/SubMiner
Length of output: 2576
🌐 Web query:
Node.js execFileSync timeout option support version requirement
💡 Result:
The timeout option for child_process.execFileSync has been supported since Node.js v0.11.12, the version when execFileSync was added. This option is inherited from child_process.spawnSync, which also gained timeout support at the same time (v0.11.12). Official Node.js documentation for versions v16.x, v18.x, v20.x, v22.x, and v25.x all list timeout as an available option with default undefined, confirming long-standing support. A 2015 documentation commit references timeout behavior, and third-party sources like execa note it as a "long time" feature for sync methods predating async spawn support (added in v14+).
Citations:
- 1: https://nodejs.org/docs/latest-v18.x/api/child_process.html
- 2: https://nodejs.org/api/child_process.html
- 3: nodejs/node@5c3f50b
- 4:
timeoutoption's improper behavior sindresorhus/execa#726 - 5: nodejs/node@95aa032
- 6: child_process: add timeout to spawn and fork nodejs/node#37256
- 7: https://nodejs.org/download/release/v20.5.1/docs/api/child_process.html
- 8: https://nodejs.org/docs/latest-v16.x/api/child_process.html
- 9: https://nodejs.org/download/release/v16.19.0/docs/api/child_process.html
Add timeouts to blocking hyprctl calls to prevent main-thread stalls.
Lines 138 and 150 in src/core/services/hyprland-window-placement.ts execute synchronous external commands without timeout protection. If hyprctl hangs, the Electron main thread will block indefinitely, freezing window operations. The timeout option is available on execFileSync and is already used elsewhere in the codebase (e.g., win32.ts:199).
Proposed fix
+const HYPRCTL_TIMEOUT_MS = 750;
+
export function ensureHyprlandWindowFloatingByTitle(options: {
@@
const run = options.execFileSync ?? execFileSync;
try {
const clients = parseHyprlandClients(
- String(run('hyprctl', ['-j', 'clients'], { encoding: 'utf-8' })),
+ String(
+ run('hyprctl', ['-j', 'clients'], {
+ encoding: 'utf-8',
+ timeout: HYPRCTL_TIMEOUT_MS,
+ killSignal: 'SIGKILL',
+ }),
+ ),
);
@@
const dispatches = buildHyprlandPlacementDispatches(client, options.bounds);
for (const args of dispatches) {
- run('hyprctl', args, { stdio: 'ignore' });
+ run('hyprctl', args, {
+ stdio: 'ignore',
+ timeout: HYPRCTL_TIMEOUT_MS,
+ killSignal: 'SIGKILL',
+ });
}Note: Similar uncovered calls exist in src/window-trackers/hyprland-tracker.ts (lines 322 and 356) that should receive the same treatment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/services/hyprland-window-placement.ts` around lines 135 - 151, The
synchronous hyprctl calls use the local alias run (set from options.execFileSync
?? execFileSync) without a timeout, so wrap both invocations—when fetching
clients (run('hyprctl', ['-j', 'clients'], { encoding: 'utf-8' })) and when
dispatching placement args (run('hyprctl', args, { stdio: 'ignore' }))—to
include a timeout option (e.g., timeout: <ms>) to avoid blocking the Electron
main thread; add a clear constant name (e.g., HYPRCTL_TIMEOUT_MS) if one doesn’t
exist and pass it into both run calls, preserving any existing options
(encoding/stdio) while merging the timeout, and apply the same pattern in
findHyprlandWindowForPlacement call sites and the similar calls in
src/window-trackers/hyprland-tracker.ts.
- Carry matched headword wordClasses from termsFind into YomitanScanToken - Map recognized Yomitan wordClasses to SubMiner coarse POS before annotation - MeCab enrichment now fills only missing POS fields, preserving existing coarse pos1 - Exclude standalone grammar particles, して helper fragments, and single-kana surfaces from annotations - Respect source-text punctuation gaps when counting N+1 sentence words - Preserve known-word highlight on excluded kanji-containing tokens - Add backlog tasks 304 (N+1 boundary bug) and 305 (wordClasses POS, done)
- return a cancel handle from the Linux refresh burst scheduler - clear pending refresh bursts when overlays hide or windows close - tighten the burst test polling to wait for the async refresh
- Treat kana-only tokens with surrounding subtitle punctuation (…, ―, etc.) as kana-only so they are not promoted to N+1 targets - Exclude unknown tokens filtered from N+1 targeting from the minSentenceWords count so filtered kana-only unknowns cannot satisfy sentence length threshold - Add regression tests for kana-only candidate suppression and filtered-unknown padding cases
- Hide annotation metadata for auxiliary inflection and ja-nai endings - Preserve lexical `くれる` forms and add regression coverage
- Use a pure-kana fixture for the subtitle token N+1 case - Update task notes for the latest CodeRabbit follow-up
- Ignore background stats daemons during regular app startup - Split standalone grammar endings before applying annotations - Clear helper-span annotations for auxiliary-only tokens
- Extract `grammar-ending.ts` with `isStandaloneGrammarEndingText` / `isSubtitleGrammarEndingText` pattern matchers - Replace `STANDALONE_GRAMMAR_ENDINGS` set in parser-selection-stage with shared matcher - Replace generated phrase sets in subtitle-annotation-filter with shared matcher - Remove stale duplicate subtitle-exclusion constants and helpers from annotation-stage - Manual clipboard card updates now write only to the sentence audio field, leaving word/expression audio untouched
- Add `type: fixed` / `area:` frontmatter to `changes/319` to pass `changelog:lint` - Thread `TokenizerAnnotationOptions` through `stripSubtitleAnnotationMetadata` so `sourceText` is honored - Include `jellyfinPlay` in `shouldQuitOnDisconnectWhenOverlayRuntimeInitialized` predicate - Make mouse test `elementFromPoint` stubs coordinate-sensitive - Make Lua test `.tmp` mkdir portable on Windows
- keep visible overlays alive during transient macOS tracker loss - reuse the running mpv overlay path on playlist navigation - update regression coverage and changelog fragments
- Replace JLPT `text-decoration` underlines with `border-bottom` so Chromium selection/hover cannot repaint them to another annotation's color - Lock JLPT underline color for combined annotation selectors (known, n+1, frequency) and character hover/selection states - Trigger AniList post-watch check on every mpv time-position update to catch skipped completion thresholds - Fall back to filename-parser season/episode when guessit omits them
- Acquire AniList post-watch in-flight lock before async gating to prevent duplicate writes - Isolate manual watched mark result from AniList post-watch callback failures - Report known-word cache clears as mutations during immediate append when state existed - Add regression tests for each fix
- Gate setup success on token persistence: `saveToken` now returns `boolean`; on failure, keeps the setup window open instead of reporting success - Config reload passes `allowSetupPrompt: false` so playback reloads don't re-open the setup window - Add regression test for persistence-failure path
- Force-apply exact Hyprland move/resize/setprop dispatches when bounds are provided - Stop pinning overlay windows; toggle pin off when Hyprland reports pinned=true - Compensate stats overlay outer placement for Electron/Wayland content insets - Make stats overlay window and page opaque so mpv cannot show through transparent insets - Constrain stats app to h-screen with internal scroll so content covers mpv from y=0 - Lock overlay/stats window titles against page-title-updated events - Add regression coverage for placement dispatches, inset compensation, and CSS overlay mode
ksyasuda
force-pushed
the
tokenizer-updates
branch
from
cea3c20 to
4d5bf3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/renderer/subtitle-render.test.ts (1)
922-927: ⚡ Quick winMake the JLPT-only check tolerant of CSS rule grouping.
Requiring an empty extracted block is brittle; if this selector is later grouped into a shared rule, the test will false-fail even when no color override is introduced. Checking for the absence of text-color declarations is more stable.
Suggested test tweak
- assert.equal( - jlptOnlyColorBlock, - '', - `word-jlpt-n${level} (without other annotations) must not set text color — JLPT only paints the underline`, - ); + assert.doesNotMatch( + jlptOnlyColorBlock, + /(?:^|\n)\s*(?:color|-webkit-text-fill-color)\s*:/m, + `word-jlpt-n${level} (without other annotations) must not set text color — JLPT only paints the underline`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/subtitle-render.test.ts` around lines 922 - 927, The test currently asserts jlptOnlyColorBlock === '' which breaks if the selector is grouped; instead assert that the extracted block (from extractClassBlock(cssText, buildJlptColorSelector(level))) does not contain any text color declarations: check jlptOnlyColorBlock for absence of patterns like "color\s*:" (and optionally "fill\s*:" if SVG/text-fill is relevant) using a regex-based negative assertion so grouping won't cause false failures; update the assertion to use a doesNotMatch/doesNotContain-style assertion against that regex and keep the rest of the test intact.src/core/services/overlay-window.ts (1)
1-2: Align with codebase import style for consistency.The default import
import electron from 'electron'is safe in this project (esModuleInterop is enabled), but the majority of the codebase uses named imports instead. Switch to named imports to match the established pattern:import { BrowserWindow, screen } from 'electron'; import type { Session } from 'electron';Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/services/overlay-window.ts` around lines 1 - 2, Replace the default import from 'electron' with named imports to match project style: import BrowserWindow and screen as regular imports and keep Session as a type import; update any other default-import occurrences (the second instance noted) to the same pattern so all electron imports in this module use named imports instead of the default electron export.src/main.ts (2)
4269-4271: ⚡ Quick winDeduplicate Anki callback wiring to avoid drift.
setKnownWordCacheUpdatedCallbackis registered in two places with the same function. Extracting a single helper for Anki callback registration will reduce maintenance risk when callback setup changes.♻️ Suggested refactor
+function attachAnkiIntegrationCallbacks(integration: AnkiIntegration | null): void { + integration?.setRecordCardsMinedCallback(recordTrackedCardsMined); + integration?.setKnownWordCacheUpdatedCallback(refreshCurrentSubtitleAfterKnownWordUpdate); +} + function initializeOverlayRuntime(): void { initializeOverlayRuntimeHandler(); - appState.ankiIntegration?.setRecordCardsMinedCallback(recordTrackedCardsMined); - appState.ankiIntegration?.setKnownWordCacheUpdatedCallback( - refreshCurrentSubtitleAfterKnownWordUpdate, - ); + attachAnkiIntegrationCallbacks(appState.ankiIntegration); syncOverlayMpvSubtitleSuppression(); } @@ setAnkiIntegration: (integration: AnkiIntegration | null) => { appState.ankiIntegration = integration; - appState.ankiIntegration?.setRecordCardsMinedCallback(recordTrackedCardsMined); - appState.ankiIntegration?.setKnownWordCacheUpdatedCallback( - refreshCurrentSubtitleAfterKnownWordUpdate, - ); + attachAnkiIntegrationCallbacks(appState.ankiIntegration); },Also applies to: 4979-4981
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 4269 - 4271, Extract a small helper (e.g., registerAnkiKnownWordCallback) that takes appState (or appState.ankiIntegration) and wires appState.ankiIntegration.setKnownWordCacheUpdatedCallback(refreshCurrentSubtitleAfterKnownWordUpdate); then replace both inline registrations with calls to this helper; reference the existing symbols appState.ankiIntegration, setKnownWordCacheUpdatedCallback, and refreshCurrentSubtitleAfterKnownWordUpdate so the single helper is used wherever the callback was previously set.
1410-1424: 🏗️ Heavy liftConsider extracting Linux fullscreen-refresh orchestration out of
src/main.ts.The new scheduling/cancellation flow is cohesive, but it adds more responsibility to an already very large entrypoint. Moving this orchestration behind a focused runtime module would better preserve maintainability.
As per coding guidelines, "
**/*.{ts,tsx}: TypeScript files should be kept small and follow existing patterns".Also applies to: 3862-3872, 5480-5511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 1410 - 1424, The new scheduling/cancellation logic (symbols: cancelLinuxMpvFullscreenOverlayRefreshBurst, cancelPendingLinuxMpvFullscreenOverlayRefreshBurst, clearScheduledSubtitlePrefetchRefresh, subtitlePrefetchRefreshTimer, SEEK_THRESHOLD_SECONDS) should be extracted from src/main.ts into a focused runtime module: create a new module (e.g., linuxFullscreenRefresh) that owns the timers, the CancelLinuxMpvFullscreenOverlayRefreshBurst type and the three functions, move the implementation there, export the public API (start/schedule, cancelPendingLinuxMpvFullscreenOverlayRefreshBurst, clearScheduledSubtitlePrefetchRefresh, and SEEK_THRESHOLD_SECONDS if widely used), and update main.ts to import and call those exports; ensure no runtime behavior changes and update any other places referenced in the review (lines ~3862-3872, ~5480-5511) to use the new module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/services/overlay-window.ts`:
- Around line 56-59: The call to ensureHyprlandWindowFloatingByTitle is in the
hot geometry update path (after normalizeOverlayWindowBoundsForPlatform ->
window.setBounds) and makes synchronous hyprctl calls that can block the main
thread; to fix this, remove direct calls from that hot path and instead debounce
or defer them: e.g., replace the immediate ensureHyprlandWindowFloatingByTitle({
title: window.getTitle(), bounds }) with a debounced / scheduled invocation
(triggered only after a short idle period or on explicit rebind/layer
transitions), or move the call into the rebind/layer-change handler so
normalizeOverlayWindowBoundsForPlatform and window.setBounds remain fast and
non-blocking. Ensure the debounced function still uses window.getTitle() and the
computed bounds when invoked.
In `@src/renderer/subtitle-render.test.ts`:
- Around line 1105-1123: The test loop that asserts JLPT underline color locking
misses the 'word-name-match' annotation, so add 'word-name-match' to the array
of annotation classes checked in the for loop (the block that iterates over
['word-known', 'word-n-plus-one', ...]) so extractClassBlock(cssText,
`#subtitleRoot .word.word-jlpt-n${level}.${annotationClass}`) is also validated
for 'word-name-match' and the assert.match continues to verify the
text-decoration-color rule for that class.
---
Nitpick comments:
In `@src/core/services/overlay-window.ts`:
- Around line 1-2: Replace the default import from 'electron' with named imports
to match project style: import BrowserWindow and screen as regular imports and
keep Session as a type import; update any other default-import occurrences (the
second instance noted) to the same pattern so all electron imports in this
module use named imports instead of the default electron export.
In `@src/main.ts`:
- Around line 4269-4271: Extract a small helper (e.g.,
registerAnkiKnownWordCallback) that takes appState (or appState.ankiIntegration)
and wires
appState.ankiIntegration.setKnownWordCacheUpdatedCallback(refreshCurrentSubtitleAfterKnownWordUpdate);
then replace both inline registrations with calls to this helper; reference the
existing symbols appState.ankiIntegration, setKnownWordCacheUpdatedCallback, and
refreshCurrentSubtitleAfterKnownWordUpdate so the single helper is used wherever
the callback was previously set.
- Around line 1410-1424: The new scheduling/cancellation logic (symbols:
cancelLinuxMpvFullscreenOverlayRefreshBurst,
cancelPendingLinuxMpvFullscreenOverlayRefreshBurst,
clearScheduledSubtitlePrefetchRefresh, subtitlePrefetchRefreshTimer,
SEEK_THRESHOLD_SECONDS) should be extracted from src/main.ts into a focused
runtime module: create a new module (e.g., linuxFullscreenRefresh) that owns the
timers, the CancelLinuxMpvFullscreenOverlayRefreshBurst type and the three
functions, move the implementation there, export the public API (start/schedule,
cancelPendingLinuxMpvFullscreenOverlayRefreshBurst,
clearScheduledSubtitlePrefetchRefresh, and SEEK_THRESHOLD_SECONDS if widely
used), and update main.ts to import and call those exports; ensure no runtime
behavior changes and update any other places referenced in the review (lines
~3862-3872, ~5480-5511) to use the new module.
In `@src/renderer/subtitle-render.test.ts`:
- Around line 922-927: The test currently asserts jlptOnlyColorBlock === ''
which breaks if the selector is grouped; instead assert that the extracted block
(from extractClassBlock(cssText, buildJlptColorSelector(level))) does not
contain any text color declarations: check jlptOnlyColorBlock for absence of
patterns like "color\s*:" (and optionally "fill\s*:" if SVG/text-fill is
relevant) using a regex-based negative assertion so grouping won't cause false
failures; update the assertion to use a doesNotMatch/doesNotContain-style
assertion against that regex and keep the rest of the test intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebb685a1-7fff-458b-bb15-473526b4592a
⛔ Files ignored due to path filters (68)
backlog/completed/task-336 - Fix-Hyprland-fullscreen-overlay-downward-offset.mdis excluded by!backlog/**backlog/completed/task-339 - Stop-pinning-Hyprland-overlay-windows.mdis excluded by!backlog/**backlog/tasks/task-304 - Fix-N1-sentence-boundary-counting-across-Yomitan-punctuation-gaps.mdis excluded by!backlog/**backlog/tasks/task-305 - Use-Yomitan-word-classes-for-subtitle-token-POS-filtering.mdis excluded by!backlog/**backlog/tasks/task-306 - Fix-Hyprland-fullscreen-overlay-geometry-and-hover-pause.mdis excluded by!backlog/**backlog/tasks/task-307 - Exclude-kana-only-words-from-N1-subtitle-targets.mdis excluded by!backlog/**backlog/tasks/task-308 - Restore-persistent-JLPT-subtitle-underlines.mdis excluded by!backlog/**backlog/tasks/task-309 - Accept-modified-follow-up-digits-for-multi-line-sentence-mining.mdis excluded by!backlog/**backlog/tasks/task-310 - Suppress-N1-highlight-for-kana-only-candidate-sentences.mdis excluded by!backlog/**backlog/tasks/task-311 - Suppress-auxiliary-inflection-fragments-from-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-312 - Suppress-ja-nai-explanatory-ending-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-315 - Suppress-annotations-for-standalone-じゃない-and-です-ending-tokens.mdis excluded by!backlog/**backlog/tasks/task-316 - Fix-macOS-launcher-playback-exit-with-background-stats-daemon.mdis excluded by!backlog/**backlog/tasks/task-318 - Keep-JLPT-underline-color-fixed-after-lookup-selection.mdis excluded by!backlog/**backlog/tasks/task-319 - Suppress-annotations-for-expressive-interjection-subtitles.mdis excluded by!backlog/**backlog/tasks/task-320 - Refresh-current-subtitle-known-word-highlight-after-successful-mining.mdis excluded by!backlog/**backlog/tasks/task-321 - Preserve-word-audio-during-manual-clipboard-card-updates.mdis excluded by!backlog/**backlog/tasks/task-322 - Fix-failing-CI-checks-on-PR-57.mdis excluded by!backlog/**backlog/tasks/task-323 - Fix-macOS-overlay-hiding-while-mpv-remains-active.mdis excluded by!backlog/**backlog/tasks/task-324 - Fix-mpv-playlist-changes-re-running-app-warmups.mdis excluded by!backlog/**backlog/tasks/task-325 - Keep-JLPT-underline-color-fixed-with-combined-lookup-annotations.mdis excluded by!backlog/**backlog/tasks/task-326 - Fix-AniList-post-watch-update-after-skipped-completion-threshold.mdis excluded by!backlog/**backlog/tasks/task-327 - Restore-stats-daemon-deferral-when-launching-playback.mdis excluded by!backlog/**backlog/tasks/task-328 - Keep-subtitle-prefetch-running-after-immediate-cached-annotation-render.mdis excluded by!backlog/**backlog/tasks/task-331 - Address-unresolved-CodeRabbit-comments-on-PR-57.mdis excluded by!backlog/**backlog/tasks/task-332 - Fix-subtitle-frequency-annotation-missing-ranks-shown-in-Yomitan-popup.mdis excluded by!backlog/**backlog/tasks/task-333 - Suppress-aru-subtitle-annotations.mdis excluded by!backlog/**backlog/tasks/task-334 - Assess-and-address-PR-57-latest-CodeRabbit-comments.mdis excluded by!backlog/**backlog/tasks/task-335 - Fix-Linux-AniList-setup-gate-using-stored-keyring-token.mdis excluded by!backlog/**backlog/tasks/task-337 - Fix-transient-Linux-safeStorage-failure-poisoning-AniList-token-store.mdis excluded by!backlog/**backlog/tasks/task-338 - Fix-known-word-highlight-on-standalone-subtitle-particles.mdis excluded by!backlog/**backlog/tasks/task-340 - Restore-default-replay-and-next-subtitle-overlay-keybindings.mdis excluded by!backlog/**changes/305-tokenizer-word-class-pos-filtering.mdis excluded by!changes/**changes/306-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/307-kana-nplusone-targets.mdis excluded by!changes/**changes/308-jlpt-underlines.mdis excluded by!changes/**changes/309-multi-mine-modified-digits.mdis excluded by!changes/**changes/311-auxiliary-inflection-annotation-filter.mdis excluded by!changes/**changes/312-grammar-ending-annotation-filter.mdis excluded by!changes/**changes/316-macos-playback-stats-daemon.mdis excluded by!changes/**changes/318-jlpt-underline-selection.mdis excluded by!changes/**changes/319-interjection-annotation-filter.mdis excluded by!changes/**changes/320-current-subtitle-known-highlight.mdis excluded by!changes/**changes/321-grammar-ending-pattern-filter.mdis excluded by!changes/**changes/322-preserve-word-audio-manual-update.mdis excluded by!changes/**changes/323-macos-overlay-tracker-flaps.mdis excluded by!changes/**changes/324-mpv-playlist-overlay-reuse.mdis excluded by!changes/**changes/325-jlpt-combined-annotation-underlines.mdis excluded by!changes/**changes/326-anilist-time-position-post-watch.mdis excluded by!changes/**changes/327-stats-daemon-deferral.mdis excluded by!changes/**changes/328-subtitle-prefetch-cache-hit-resume.mdis excluded by!changes/**changes/332-subtitle-frequency-ordinal-prefix.mdis excluded by!changes/**changes/333-aru-annotation-filter.mdis excluded by!changes/**changes/334-coderabbit-followups.mdis excluded by!changes/**changes/335-anilist-linux-token-setup.mdis excluded by!changes/**changes/336-hyprland-fullscreen-overlay.mdis excluded by!changes/**changes/336-overlay-focus-ring.mdis excluded by!changes/**changes/337-anilist-safe-storage-retry.mdis excluded by!changes/**changes/338-known-word-particle-highlights.mdis excluded by!changes/**changes/339-hyprland-overlay-pin.mdis excluded by!changes/**changes/340-default-subtitle-keybindings.mdis excluded by!changes/**docs-site/anki-integration.mdis excluded by!docs-site/**docs-site/configuration.mdis excluded by!docs-site/**docs-site/mining-workflow.mdis excluded by!docs-site/**docs-site/public/config.example.jsoncis excluded by!docs-site/**docs-site/shortcuts.mdis excluded by!docs-site/**docs-site/troubleshooting.mdis excluded by!docs-site/**docs-site/usage.mdis excluded by!docs-site/**
📒 Files selected for processing (91)
config.example.jsoncpackage.jsonplugin/subminer/lifecycle.luaplugin/subminer/process.luaplugin/subminer/session_bindings.luascripts/test-plugin-session-bindings.luascripts/test-plugin-start-gate.luasrc/anki-integration.test.tssrc/anki-integration.tssrc/anki-integration/card-creation-manual-update.test.tssrc/anki-integration/card-creation.tssrc/anki-integration/known-word-cache.test.tssrc/anki-integration/known-word-cache.tssrc/config/definitions/defaults-core.tssrc/config/definitions/domain-registry.test.tssrc/core/services/anilist/anilist-token-store.test.tssrc/core/services/anilist/anilist-token-store.tssrc/core/services/anilist/anilist-updater.test.tssrc/core/services/anilist/anilist-updater.tssrc/core/services/hyprland-window-placement.test.tssrc/core/services/hyprland-window-placement.tssrc/core/services/ipc.test.tssrc/core/services/ipc.tssrc/core/services/mpv-properties.tssrc/core/services/mpv-protocol.test.tssrc/core/services/mpv-protocol.tssrc/core/services/mpv.test.tssrc/core/services/mpv.tssrc/core/services/overlay-manager.test.tssrc/core/services/overlay-visibility.test.tssrc/core/services/overlay-visibility.tssrc/core/services/overlay-window-config.test.tssrc/core/services/overlay-window-options.tssrc/core/services/overlay-window.tssrc/core/services/session-bindings.test.tssrc/core/services/stats-window-runtime.tssrc/core/services/stats-window.test.tssrc/core/services/stats-window.tssrc/core/services/tokenizer.test.tssrc/core/services/tokenizer.tssrc/core/services/tokenizer/annotation-stage.test.tssrc/core/services/tokenizer/annotation-stage.tssrc/core/services/tokenizer/grammar-ending.tssrc/core/services/tokenizer/parser-enrichment-stage.test.tssrc/core/services/tokenizer/parser-enrichment-stage.tssrc/core/services/tokenizer/parser-selection-stage.test.tssrc/core/services/tokenizer/parser-selection-stage.tssrc/core/services/tokenizer/subtitle-annotation-filter.tssrc/core/services/tokenizer/yomitan-parser-runtime.test.tssrc/core/services/tokenizer/yomitan-parser-runtime.tssrc/main.tssrc/main/dependencies.tssrc/main/runtime/anilist-post-watch.test.tssrc/main/runtime/anilist-post-watch.tssrc/main/runtime/anilist-setup-protocol-main-deps.test.tssrc/main/runtime/anilist-setup-protocol.test.tssrc/main/runtime/anilist-setup-protocol.tssrc/main/runtime/anilist-setup.test.tssrc/main/runtime/anilist-setup.tssrc/main/runtime/app-lifecycle-actions.test.tssrc/main/runtime/app-lifecycle-actions.tssrc/main/runtime/app-lifecycle-main-cleanup.test.tssrc/main/runtime/app-lifecycle-main-cleanup.tssrc/main/runtime/composers/anilist-setup-composer.test.tssrc/main/runtime/composers/startup-lifecycle-composer.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.tssrc/main/runtime/linux-mpv-fullscreen-overlay-refresh.tssrc/main/runtime/mpv-client-event-bindings.test.tssrc/main/runtime/mpv-client-event-bindings.tssrc/main/runtime/mpv-main-event-actions.test.tssrc/main/runtime/mpv-main-event-actions.tssrc/main/runtime/mpv-main-event-bindings.tssrc/main/runtime/mpv-main-event-main-deps.test.tssrc/main/runtime/mpv-main-event-main-deps.tssrc/main/runtime/startup-config.test.tssrc/main/runtime/startup-config.tssrc/main/runtime/stats-server-routing.test.tssrc/main/runtime/stats-server-routing.tssrc/renderer/handlers/keyboard.test.tssrc/renderer/handlers/keyboard.tssrc/renderer/handlers/mouse.test.tssrc/renderer/handlers/mouse.tssrc/renderer/overlay-legacy-cleanup.test.tssrc/renderer/style.csssrc/renderer/subtitle-render.test.tssrc/token-merger.tssrc/window-trackers/hyprland-tracker.test.tssrc/window-trackers/hyprland-tracker.tsstats/src/App.tsxstats/src/styles/globals.cssstats/src/styles/globals.test.ts
💤 Files with no reviewable changes (1)
- src/core/services/anilist/anilist-token-store.ts
✅ Files skipped from review due to trivial changes (38)
- stats/src/styles/globals.test.ts
- src/main/runtime/composers/anilist-setup-composer.test.ts
- src/main/runtime/stats-server-routing.test.ts
- stats/src/App.tsx
- src/main/runtime/mpv-client-event-bindings.ts
- src/core/services/stats-window-runtime.ts
- src/main/runtime/anilist-setup-protocol-main-deps.test.ts
- src/core/services/mpv-protocol.test.ts
- src/core/services/tokenizer/parser-enrichment-stage.test.ts
- src/main/runtime/stats-server-routing.ts
- src/window-trackers/hyprland-tracker.test.ts
- src/renderer/handlers/keyboard.test.ts
- src/main/runtime/mpv-main-event-bindings.ts
- src/main/runtime/app-lifecycle-main-cleanup.test.ts
- src/core/services/stats-window.test.ts
- src/config/definitions/domain-registry.test.ts
- src/core/services/session-bindings.test.ts
- src/renderer/handlers/keyboard.ts
- src/main/runtime/anilist-setup.test.ts
- src/anki-integration.ts
- src/anki-integration.test.ts
- src/core/services/anilist/anilist-updater.test.ts
- src/main/runtime/mpv-client-event-bindings.test.ts
- src/main/runtime/composers/startup-lifecycle-composer.test.ts
- src/core/services/mpv.ts
- src/core/services/stats-window.ts
- src/core/services/overlay-manager.test.ts
- config.example.jsonc
- src/main/runtime/mpv-main-event-actions.test.ts
- src/core/services/tokenizer.ts
- src/main/runtime/mpv-main-event-main-deps.test.ts
- src/core/services/tokenizer/yomitan-parser-runtime.test.ts
- src/main/runtime/startup-config.test.ts
- src/window-trackers/hyprland-tracker.ts
- src/core/services/tokenizer/grammar-ending.ts
- src/core/services/ipc.test.ts
- stats/src/styles/globals.css
- src/core/services/tokenizer/yomitan-parser-runtime.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- src/core/services/overlay-window-config.test.ts
- src/core/services/overlay-window-options.ts
- plugin/subminer/lifecycle.lua
- src/main/runtime/app-lifecycle-actions.ts
- src/core/services/anilist/anilist-updater.ts
- src/core/services/ipc.ts
- src/main/runtime/app-lifecycle-main-cleanup.ts
- src/main/runtime/startup-config.ts
- src/renderer/overlay-legacy-cleanup.test.ts
- src/core/services/mpv-protocol.ts
- src/main/runtime/mpv-main-event-actions.ts
- src/main/runtime/mpv-main-event-main-deps.ts
- src/main/runtime/anilist-setup-protocol.test.ts
- src/main/runtime/anilist-setup.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.test.ts
- src/renderer/handlers/mouse.ts
- src/core/services/hyprland-window-placement.ts
- src/core/services/tokenizer/parser-enrichment-stage.ts
- src/core/services/hyprland-window-placement.test.ts
- src/main/runtime/anilist-setup-protocol.ts
- src/main/runtime/linux-mpv-fullscreen-overlay-refresh.ts
- src/core/services/tokenizer/annotation-stage.ts
- src/config/definitions/defaults-core.ts
- scripts/test-plugin-session-bindings.lua
- src/anki-integration/known-word-cache.test.ts
- src/token-merger.ts
- src/main/runtime/app-lifecycle-actions.test.ts
- src/anki-integration/card-creation.ts
- src/main/runtime/anilist-post-watch.test.ts
- package.json
- plugin/subminer/process.lua
- src/renderer/style.css
- src/core/services/tokenizer/subtitle-annotation-filter.ts
- src/renderer/handlers/mouse.test.ts
- src/core/services/tokenizer/annotation-stage.test.ts
Comment on lines
+56
to
59
| const bounds = normalizeOverlayWindowBoundsForPlatform(geometry, process.platform, screen); | ||
| window.setBounds(bounds); | ||
| ensureHyprlandWindowFloatingByTitle({ title: window.getTitle(), bounds }); | ||
| } |
There was a problem hiding this comment.
Avoid synchronous Hyprland placement calls on every bounds update.
Line 58 pulls ensureHyprlandWindowFloatingByTitle into the geometry hot path; that helper performs synchronous hyprctl calls, which can block the Electron main thread during frequent resize/move updates. Please debounce/collapse this path or trigger it only on explicit rebind/layer transitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/services/overlay-window.ts` around lines 56 - 59, The call to
ensureHyprlandWindowFloatingByTitle is in the hot geometry update path (after
normalizeOverlayWindowBoundsForPlatform -> window.setBounds) and makes
synchronous hyprctl calls that can block the main thread; to fix this, remove
direct calls from that hot path and instead debounce or defer them: e.g.,
replace the immediate ensureHyprlandWindowFloatingByTitle({ title:
window.getTitle(), bounds }) with a debounced / scheduled invocation (triggered
only after a short idle period or on explicit rebind/layer transitions), or move
the call into the rebind/layer-change handler so
normalizeOverlayWindowBoundsForPlatform and window.setBounds remain fast and
non-blocking. Ensure the debounced function still uses window.getTitle() and the
computed bounds when invoked.
Comment on lines
+1105
to
+1123
| for (const annotationClass of [ | ||
| 'word-known', | ||
| 'word-n-plus-one', | ||
| 'word-frequency-single', | ||
| 'word-frequency-band-2', | ||
| ]) { | ||
| const combinedAnnotationBlock = extractClassBlock( | ||
| cssText, | ||
| `#subtitleRoot .word.word-jlpt-n${level}.${annotationClass}`, | ||
| ); | ||
| assert.match( | ||
| combinedAnnotationBlock, | ||
| new RegExp( | ||
| `text-decoration-color:\\s*var\\(--subtitle-jlpt-n${level}-color,\\s*#[0-9a-f]{6}\\)\\s*!important;`, | ||
| 'i', | ||
| ), | ||
| `combined JLPT ${annotationClass} selector should lock underline color`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add word-name-match to the JLPT lock coverage.
This loop covers known, N+1, and frequency tokens, but it skips the name-match path, which is also higher priority than JLPT styling. A regression in that combination would slip through.
Suggested coverage addition
for (const annotationClass of [
'word-known',
'word-n-plus-one',
+ 'word-name-match',
'word-frequency-single',
'word-frequency-band-2',
]) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const annotationClass of [ | |
| 'word-known', | |
| 'word-n-plus-one', | |
| 'word-frequency-single', | |
| 'word-frequency-band-2', | |
| ]) { | |
| const combinedAnnotationBlock = extractClassBlock( | |
| cssText, | |
| `#subtitleRoot .word.word-jlpt-n${level}.${annotationClass}`, | |
| ); | |
| assert.match( | |
| combinedAnnotationBlock, | |
| new RegExp( | |
| `text-decoration-color:\\s*var\\(--subtitle-jlpt-n${level}-color,\\s*#[0-9a-f]{6}\\)\\s*!important;`, | |
| 'i', | |
| ), | |
| `combined JLPT ${annotationClass} selector should lock underline color`, | |
| ); | |
| } | |
| for (const annotationClass of [ | |
| 'word-known', | |
| 'word-n-plus-one', | |
| 'word-name-match', | |
| 'word-frequency-single', | |
| 'word-frequency-band-2', | |
| ]) { | |
| const combinedAnnotationBlock = extractClassBlock( | |
| cssText, | |
| `#subtitleRoot .word.word-jlpt-n${level}.${annotationClass}`, | |
| ); | |
| assert.match( | |
| combinedAnnotationBlock, | |
| new RegExp( | |
| `text-decoration-color:\\s*var\\(--subtitle-jlpt-n${level}-color,\\s*#[0-9a-f]{6}\\)\\s*!important;`, | |
| 'i', | |
| ), | |
| `combined JLPT ${annotationClass} selector should lock underline color`, | |
| ); | |
| } |
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 1116-1119: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
text-decoration-color:\\s*var\\(--subtitle-jlpt-n${level}-color,\\s*#[0-9a-f]{6}\\)\\s*!important;,
'i',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/subtitle-render.test.ts` around lines 1105 - 1123, The test loop
that asserts JLPT underline color locking misses the 'word-name-match'
annotation, so add 'word-name-match' to the array of annotation classes checked
in the for loop (the block that iterates over ['word-known', 'word-n-plus-one',
...]) so extractClassBlock(cssText, `#subtitleRoot
.word.word-jlpt-n${level}.${annotationClass}`) is also validated for
'word-name-match' and the assert.match continues to verify the
text-decoration-color rule for that class.
- Add `shouldAllowHonorificPrefixNounFrequency` to exempt お/ご/御 + noun merged tokens from frequency exclusion - Add regression test for `ご機嫌` asserting rank 5484 is preserved after MeCab enrichment and annotation - Close TASK-341
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/services/tokenizer.test.ts (1)
4233-5067: 🏗️ Heavy liftConsider table-driving or splitting these exclusion-rule cases.
This block adds many near-identical fixtures and token-summary assertions that mostly vary by inputs and expected cleared fields. A small shared helper plus a case matrix, or a dedicated subtitle-annotation test file, would make future rule changes much easier to maintain in this already very large TypeScript test file.
As per coding guidelines,
**/*.{ts,tsx}: TypeScript files should be kept small and follow existing patterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/services/tokenizer.test.ts` around lines 4233 - 5067, The tests repeat the same pattern around tokenizeSubtitle and makeDepsFromYomitanTokens with only inputs and expected cleared fields changing; refactor into a table-driven helper to reduce duplication: create a shared helper (e.g., runAnnotationClearCase) that calls tokenizeSubtitle with a provided token fixture from makeDepsFromYomitanTokens, computes tokenSummary, and asserts expected entries, then replace each duplicated test with a lightweight row in a case matrix (surface/input, mecab tokens, deps overrides, expectations). Optionally move these cases into a new test module to keep the original file small; ensure helper references include tokenizeSubtitle, makeDepsFromYomitanTokens and tokenSummary to locate where to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/services/tokenizer.test.ts`:
- Around line 3233-3285: The test incorrectly asserts that the kanji token '点'
should have isKnown cleared; update the assertion in the tokenizeSubtitle test
so the excluded-kanji case preserves isKnown for the '点' token (change the
expectation on result.tokens?.[1]?.isKnown to true), and ensure any related
assertions about frequencyRank and jlptLevel remain consistent with the
preserved-known behavior; locate this check in the test around the
tokenizeSubtitle invocation and the assertions for result.tokens?.[1].
---
Nitpick comments:
In `@src/core/services/tokenizer.test.ts`:
- Around line 4233-5067: The tests repeat the same pattern around
tokenizeSubtitle and makeDepsFromYomitanTokens with only inputs and expected
cleared fields changing; refactor into a table-driven helper to reduce
duplication: create a shared helper (e.g., runAnnotationClearCase) that calls
tokenizeSubtitle with a provided token fixture from makeDepsFromYomitanTokens,
computes tokenSummary, and asserts expected entries, then replace each
duplicated test with a lightweight row in a case matrix (surface/input, mecab
tokens, deps overrides, expectations). Optionally move these cases into a new
test module to keep the original file small; ensure helper references include
tokenizeSubtitle, makeDepsFromYomitanTokens and tokenSummary to locate where to
update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7ba20e81-5736-48b9-a4f4-0095fce55be6
⛔ Files ignored due to path filters (1)
backlog/tasks/task-341 - Fix-frequency-highlight-for-honorific-prefix-noun-tokens.mdis excluded by!backlog/**
📒 Files selected for processing (2)
src/core/services/tokenizer.test.tssrc/core/services/tokenizer/annotation-stage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/services/tokenizer/annotation-stage.ts
Comment on lines
+3233
to
+3285
| test('tokenizeSubtitle clears known-word highlight for exact non-independent kanji noun tokens', async () => { | ||
| const result = await tokenizeSubtitle( | ||
| 'その点', | ||
| makeDepsFromYomitanTokens( | ||
| [ | ||
| { surface: 'その', reading: 'その', headword: 'その' }, | ||
| { surface: '点', reading: 'てん', headword: '点' }, | ||
| ], | ||
| { | ||
| isKnownWord: (text) => text === '点' || text === 'てん', | ||
| getFrequencyDictionaryEnabled: () => true, | ||
| getFrequencyRank: (text) => (text === '点' ? 1384 : null), | ||
| getJlptLevel: (text) => (text === '点' ? 'N3' : null), | ||
| tokenizeWithMecab: async () => [ | ||
| { | ||
| headword: 'その', | ||
| surface: 'その', | ||
| reading: 'ソノ', | ||
| startPos: 0, | ||
| endPos: 2, | ||
| partOfSpeech: PartOfSpeech.other, | ||
| pos1: '連体詞', | ||
| isMerged: false, | ||
| isKnown: false, | ||
| isNPlusOneTarget: false, | ||
| }, | ||
| { | ||
| headword: '点', | ||
| surface: '点', | ||
| reading: 'テン', | ||
| startPos: 2, | ||
| endPos: 3, | ||
| partOfSpeech: PartOfSpeech.noun, | ||
| pos1: '名詞', | ||
| pos2: '非自立', | ||
| pos3: '一般', | ||
| isMerged: false, | ||
| isKnown: false, | ||
| isNPlusOneTarget: false, | ||
| }, | ||
| ], | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| assert.equal(result.tokens?.length, 2); | ||
| assert.equal(result.tokens?.[0]?.isKnown, false); | ||
| assert.equal(result.tokens?.[1]?.surface, '点'); | ||
| assert.equal(result.tokens?.[1]?.isKnown, false); | ||
| assert.equal(result.tokens?.[1]?.isNPlusOneTarget, false); | ||
| assert.equal(result.tokens?.[1]?.frequencyRank, undefined); | ||
| assert.equal(result.tokens?.[1]?.jlptLevel, undefined); | ||
| }); |
There was a problem hiding this comment.
Preserve isKnown for excluded kanji token 点.
Line 3281 expects isKnown to be cleared, but this is the exact excluded-kanji case the PR is supposed to preserve. Keeping this assertion as false would let a regression slip through on the new isKnown retention behavior.
Proposed test fix
-test('tokenizeSubtitle clears known-word highlight for exact non-independent kanji noun tokens', async () => {
+test('tokenizeSubtitle preserves known-word highlight for exact non-independent kanji noun tokens', async () => {
@@
- assert.equal(result.tokens?.[1]?.isKnown, false);
+ assert.equal(result.tokens?.[1]?.isKnown, true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/services/tokenizer.test.ts` around lines 3233 - 3285, The test
incorrectly asserts that the kanji token '点' should have isKnown cleared; update
the assertion in the tokenizeSubtitle test so the excluded-kanji case preserves
isKnown for the '点' token (change the expectation on result.tokens?.[1]?.isKnown
to true), and ensure any related assertions about frequencyRank and jlptLevel
remain consistent with the preserved-known behavior; locate this check in the
test around the tokenizeSubtitle invocation and the assertions for
result.tokens?.[1].
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wordClassesfrom YomitantermsFindresults throughYomitanScanTokenintoMergedTokenPOS metadata — no vendored Yomitan changes requiredprt,aux,v*,adj-i/ix,adj-na, noun-like) to SubMiner coarsePartOfSpeech/pos1before annotation filteringpos1+pos2+pos3; fills missing detail fields while preserving existing coarsepos1set by Yomitan word class mappingisKnownstatus on excluded tokens that have kanji surface/headword, clearing frequency/JLPT/N+1 metadatasourceTextthrough annotation options to support sentence-boundary counting across Yomitan punctuation gaps (groundwork for TASK-304)Testing
bun test src/core/services/tokenizer/yomitan-parser-runtime.test.ts— new test:requestYomitanScanTokens preserves matched headword word classesbun test src/core/services/tokenizer.test.ts— new tests: Yomitan word class → POS mapping, MeCab detail fill with coarse POS already set, known-word highlight for non-independent kanji nouns; updated particle JLPT test expectationbun test src/core/services/tokenizer/annotation-stage.test.ts— new tests: して/particle/single-kana/もんか/くれ/って/べき exclusion, source-text sentence boundary gap, preserved isKnown for excluded kanji tokensbun run typecheck— passedSummary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist
changes/, or this PR is labeledskip-changelog