diff --git a/backlog/tasks/task-153 - Fix-character-dictionary-MRU-eviction-after-revisits.md b/backlog/tasks/task-153 - Fix-character-dictionary-MRU-eviction-after-revisits.md new file mode 100644 index 0000000..db4e1e4 --- /dev/null +++ b/backlog/tasks/task-153 - Fix-character-dictionary-MRU-eviction-after-revisits.md @@ -0,0 +1,68 @@ +--- +id: TASK-153 +title: Fix character dictionary MRU eviction after revisits +status: Done +assignee: + - '@codex' +created_date: '2026-03-10 07:56' +updated_date: '2026-03-10 08:48' +labels: + - character-dictionary + - yomitan + - anilist +dependencies: [] +references: + - >- + /home/sudacode/projects/japanese/SubMiner/src/main/runtime/character-dictionary-auto-sync.ts + - >- + /home/sudacode/projects/japanese/SubMiner/src/main/runtime/character-dictionary-auto-sync.test.ts + - >- + /home/sudacode/projects/japanese/SubMiner/src/main/character-dictionary-runtime.ts +documentation: + - /home/sudacode/projects/japanese/subminer-docs/development.md + - /home/sudacode/projects/japanese/subminer-docs/architecture.md +priority: high +--- + +## Description + + +Keep merged character dictionary retention truly MRU-based when a currently retained anime is revisited before opening a new title, so the oldest retained title is evicted instead of the revisited one. + + +## Acceptance Criteria + +- [x] #1 Revisiting an anime already retained in the merged character dictionary updates MRU ordering before the next new title is added. +- [x] #2 When retention exceeds maxLoaded after that revisit-plus-new-title sequence, the least recently used retained anime is evicted rather than the revisited title. +- [x] #3 Auto-sync rebuilds or reloads the merged dictionary so an evicted anime becomes available again when reopened. +- [x] #4 Regression tests cover the revisit-before-eviction flow. + + +## Implementation Plan + + +1. Reproduce the revisit-before-eviction scenario with a focused regression test in `src/main/runtime/character-dictionary-auto-sync.test.ts` using a retained set that revisits an existing anime before introducing a new anime. +2. Run the focused test to confirm current behavior fails for the expected MRU ordering / eviction case. +3. Adjust `src/main/runtime/character-dictionary-auto-sync.ts` so retained ordering stays MRU-correct across revisits and subsequent additions without regressing unchanged revisit behavior. +4. Re-run the focused auto-sync suite, then run the relevant broader checks required for handoff. + + +## Implementation Notes + + +Added a focused regression in `src/main/runtime/character-dictionary-auto-sync.test.ts` for the `1,2,3,1,4,1` revisit-before-eviction flow. The current MRU auto-sync logic passed: merged builds were `[1]`, `[2,1]`, `[3,2,1]`, `[1,3,2]`, `[4,1,3]`, `[1,4,3]`, so I have not reproduced the reported eviction bug in the in-process auto-sync service yet. + +Inspected local cache under `~/.config/SubMiner/character-dictionaries/` and found Little Witch Academia snapshot `anilist-21858.json` already present, so the reported regeneration path was not a snapshot-cache miss. The on-disk `merged.zip` revision (`9251ae23e136`) also differed from `auto-sync-state.json` (`bc16c5b5af17`), indicating a rebuilt merged dictionary artifact could land without the MRU state being persisted. + +Fixed `src/main/runtime/character-dictionary-auto-sync.ts` to persist the rebuilt retained-set state immediately after a merged dictionary revision/title is known, before later Yomitan mutation steps. This keeps MRU eviction state aligned even if import/settings work fails after the rebuild artifact is written. + +Added regression coverage in `src/main/runtime/character-dictionary-auto-sync.test.ts` for the revisit-before-eviction sequence and for post-build import failure preserving the rebuilt MRU state. Verified with `bun test src/main/runtime/character-dictionary-auto-sync.test.ts src/main/character-dictionary-runtime.test.ts`, `bun run typecheck`, `bun run changelog:lint`, and `bun run test:fast`. + + +## Final Summary + + +Persisted merged character-dictionary MRU state as soon as a rebuilt retained set is known in `src/main/runtime/character-dictionary-auto-sync.ts`, so revisits are not lost if later Yomitan import/settings work fails after `merged.zip` has already been rewritten. Added regression coverage for revisit-before-eviction ordering and import-failure state preservation in `src/main/runtime/character-dictionary-auto-sync.test.ts`, plus a changelog fragment in `changes/character-dictionary-mru-state-recovery.md`. + +Validation: `bun test src/main/runtime/character-dictionary-auto-sync.test.ts src/main/character-dictionary-runtime.test.ts`, `bun run typecheck`, `bun run changelog:lint`, `bun run test:fast`. + diff --git a/backlog/tasks/task-154 - Avoid-merged-dictionary-rebuilds-on-MRU-reorder-only-revisits.md b/backlog/tasks/task-154 - Avoid-merged-dictionary-rebuilds-on-MRU-reorder-only-revisits.md new file mode 100644 index 0000000..851db4a --- /dev/null +++ b/backlog/tasks/task-154 - Avoid-merged-dictionary-rebuilds-on-MRU-reorder-only-revisits.md @@ -0,0 +1,68 @@ +--- +id: TASK-154 +title: Avoid merged dictionary rebuilds on MRU reorder-only revisits +status: Done +assignee: + - '@codex' +created_date: '2026-03-10 09:16' +updated_date: '2026-03-10 09:22' +labels: + - character-dictionary + - yomitan + - anilist +dependencies: [] +references: + - >- + /home/sudacode/projects/japanese/SubMiner/src/main/runtime/character-dictionary-auto-sync.ts + - >- + /home/sudacode/projects/japanese/SubMiner/src/main/runtime/character-dictionary-auto-sync.test.ts + - >- + /home/sudacode/projects/japanese/SubMiner/src/main/character-dictionary-runtime.ts + - >- + /home/sudacode/projects/japanese/SubMiner/src/main/character-dictionary-runtime.test.ts +documentation: + - /home/sudacode/projects/japanese/subminer-docs/development.md + - /home/sudacode/projects/japanese/subminer-docs/architecture.md +priority: high +--- + +## Description + + +Keep per-title MRU retention ordering for eviction decisions, but do not rebuild or reimport the merged character dictionary when the retained set membership is unchanged and only the MRU order changes. + + +## Acceptance Criteria + +- [x] #1 Revisiting an anime already inside the retained set updates MRU ordering used for later eviction decisions without rebuilding or reimporting the merged dictionary when retained membership is unchanged. +- [x] #2 Merged dictionary revisions and contents are stable for the same retained membership regardless of MRU ordering. +- [x] #3 Adding a new anime that changes retained membership still rebuilds and imports the merged dictionary with the correct eviction behavior. +- [x] #4 Regression tests cover reorder-only revisits and stable merged revisions for equivalent retained sets. + + +## Implementation Plan + + +1. Add a failing auto-sync regression showing that a revisit which only reorders MRU state should update `activeMediaIds` but skip merged rebuild/import. +2. Add a runtime-level regression showing `buildMergedDictionary` produces a stable revision for equivalent retained memberships regardless of input order. +3. Update merged-dictionary build normalization and auto-sync rebuild gating so rebuilds are driven by retained membership changes (or snapshot changes), not MRU reordering alone. +4. Re-run focused dictionary tests plus the local verification lanes impacted by the change. + + +## Implementation Notes + + +Changed `src/main/runtime/character-dictionary-auto-sync.ts` so MRU order still updates in `activeMediaIds`, but merged rebuild/import now keys off retained membership changes rather than order-only reordering. Order-only revisits no longer force a merged ZIP rewrite or Yomitan reimport. + +Changed `src/main/character-dictionary-runtime.ts` to canonicalize merged dictionary media ids before building, which makes merged revisions stable for equivalent retained memberships regardless of MRU ordering. + +Updated regression coverage in `src/main/runtime/character-dictionary-auto-sync.test.ts` for reorder-only revisits and membership-change rebuilds, and extended `src/main/character-dictionary-runtime.test.ts` to assert stable merged revisions for reordered inputs. Verified with `bun test src/main/runtime/character-dictionary-auto-sync.test.ts src/main/character-dictionary-runtime.test.ts`, `bun run typecheck`, `bun run changelog:lint`, and `bun run test:fast`. + + +## Final Summary + + +Updated merged character-dictionary syncing so MRU ordering is still tracked for eviction in `src/main/runtime/character-dictionary-auto-sync.ts`, but reorder-only revisits no longer rebuild or reimport the merged dictionary unless retained membership or snapshot data changes. Canonicalized merged build input ordering in `src/main/character-dictionary-runtime.ts` so revisions remain stable for the same retained set regardless of MRU order, and added regressions in `src/main/runtime/character-dictionary-auto-sync.test.ts` plus `src/main/character-dictionary-runtime.test.ts`. Also updated `changes/character-dictionary-mru-state-recovery.md` to cover the new behavior. + +Validation: `bun test src/main/runtime/character-dictionary-auto-sync.test.ts src/main/character-dictionary-runtime.test.ts`, `bun run typecheck`, `bun run changelog:lint`, `bun run test:fast`. + diff --git a/changes/character-dictionary-mru-state-recovery.md b/changes/character-dictionary-mru-state-recovery.md new file mode 100644 index 0000000..9bda4c0 --- /dev/null +++ b/changes/character-dictionary-mru-state-recovery.md @@ -0,0 +1,4 @@ +type: fixed +area: dictionary + +- Persist merged character-dictionary MRU state as soon as a new retained set is built so revisits do not get dropped if later Yomitan import work fails, and skip merged dictionary rebuilds for reorder-only revisits when the retained anime set itself has not changed. diff --git a/src/main/character-dictionary-runtime.test.ts b/src/main/character-dictionary-runtime.test.ts index 2c4c882..e628011 100644 --- a/src/main/character-dictionary-runtime.test.ts +++ b/src/main/character-dictionary-runtime.test.ts @@ -2206,6 +2206,7 @@ test('buildMergedDictionary combines stored snapshots into one stable dictionary await runtime.getOrCreateCurrentSnapshot(); const merged = await runtime.buildMergedDictionary([21, 130298]); + const mergedReordered = await runtime.buildMergedDictionary([130298, 21]); const index = JSON.parse(readStoredZipEntry(merged.zipPath, 'index.json').toString('utf8')) as { title: string; }; @@ -2228,6 +2229,7 @@ test('buildMergedDictionary combines stored snapshots into one stable dictionary assert.equal(index.title, 'SubMiner Character Dictionary'); assert.equal(merged.entryCount >= 2, true); + assert.equal(merged.revision, mergedReordered.revision); assert.ok(frieren); assert.ok(alpha); assert.equal((frieren[5][0] as { type?: string }).type, 'structured-content'); diff --git a/src/main/character-dictionary-runtime.ts b/src/main/character-dictionary-runtime.ts index 08bbc28..dc21b3b 100644 --- a/src/main/character-dictionary-runtime.ts +++ b/src/main/character-dictionary-runtime.ts @@ -1980,6 +1980,16 @@ function buildMergedRevision(mediaIds: number[], snapshots: CharacterDictionaryS return hash.digest('hex').slice(0, 12); } +function normalizeMergedMediaIds(mediaIds: number[]): number[] { + return [ + ...new Set( + mediaIds + .filter((mediaId) => Number.isFinite(mediaId) && mediaId > 0) + .map((mediaId) => Math.floor(mediaId)), + ), + ].sort((left, right) => left - right); +} + export function createCharacterDictionaryRuntimeService(deps: CharacterDictionaryRuntimeDeps): { getOrCreateCurrentSnapshot: ( targetPath?: string, @@ -2154,9 +2164,7 @@ export function createCharacterDictionaryRuntimeService(deps: CharacterDictionar ); }, buildMergedDictionary: async (mediaIds: number[]) => { - const normalizedMediaIds = mediaIds - .filter((mediaId) => Number.isFinite(mediaId) && mediaId > 0) - .map((mediaId) => Math.floor(mediaId)); + const normalizedMediaIds = normalizeMergedMediaIds(mediaIds); const snapshotResults = await Promise.all( normalizedMediaIds.map((mediaId) => getOrCreateSnapshot(mediaId)), ); diff --git a/src/main/runtime/character-dictionary-auto-sync.test.ts b/src/main/runtime/character-dictionary-auto-sync.test.ts index 81cf892..69be781 100644 --- a/src/main/runtime/character-dictionary-auto-sync.test.ts +++ b/src/main/runtime/character-dictionary-auto-sync.test.ts @@ -150,7 +150,7 @@ test('auto sync skips rebuild/import on unchanged revisit when merged dictionary assert.deepEqual(imports, ['/tmp/merged.zip']); }); -test('auto sync rebuilds merged dictionary when MRU order changes', async () => { +test('auto sync updates MRU order without rebuilding merged dictionary when membership is unchanged', async () => { const userDataPath = makeTempDir(); const sequence = [1, 2, 1]; const mergedBuilds: number[][] = []; @@ -207,8 +207,14 @@ test('auto sync rebuilds merged dictionary when MRU order changes', async () => await runtime.runSyncNow(); await runtime.runSyncNow(); - assert.deepEqual(mergedBuilds, [[1], [2, 1], [1, 2]]); - assert.ok(deleted.length >= 2); + assert.deepEqual(mergedBuilds, [[1], [2, 1]]); + assert.equal(deleted.length, 1); + + const statePath = path.join(userDataPath, 'character-dictionaries', 'auto-sync-state.json'); + const state = JSON.parse(fs.readFileSync(statePath, 'utf8')) as { + activeMediaIds: number[]; + }; + assert.deepEqual(state.activeMediaIds, [1, 2]); }); test('auto sync evicts least recently used media from merged set', async () => { @@ -276,6 +282,137 @@ test('auto sync evicts least recently used media from merged set', async () => { assert.deepEqual(state.activeMediaIds, [4, 3, 2]); }); +test('auto sync keeps revisited media retained when a new title is added afterward', async () => { + const userDataPath = makeTempDir(); + const sequence = [1, 2, 3, 1, 4, 1]; + const mergedBuilds: number[][] = []; + let runIndex = 0; + let importedRevision: string | null = null; + + const runtime = createCharacterDictionaryAutoSyncRuntimeService({ + userDataPath, + getConfig: () => ({ + enabled: true, + maxLoaded: 3, + profileScope: 'all', + }), + getOrCreateCurrentSnapshot: async () => { + const mediaId = sequence[Math.min(runIndex, sequence.length - 1)]!; + runIndex += 1; + return { + mediaId, + mediaTitle: `Title ${mediaId}`, + entryCount: 10, + fromCache: true, + updatedAt: mediaId, + }; + }, + buildMergedDictionary: async (mediaIds) => { + mergedBuilds.push([...mediaIds]); + const revision = `rev-${mediaIds.join('-')}`; + return { + zipPath: `/tmp/${revision}.zip`, + revision, + dictionaryTitle: 'SubMiner Character Dictionary', + entryCount: mediaIds.length * 10, + }; + }, + getYomitanDictionaryInfo: async () => + importedRevision + ? [{ title: 'SubMiner Character Dictionary', revision: importedRevision }] + : [], + importYomitanDictionary: async (zipPath) => { + importedRevision = path.basename(zipPath, '.zip'); + return true; + }, + deleteYomitanDictionary: async () => { + importedRevision = null; + return true; + }, + upsertYomitanDictionarySettings: async () => true, + now: () => Date.now(), + }); + + await runtime.runSyncNow(); + await runtime.runSyncNow(); + await runtime.runSyncNow(); + await runtime.runSyncNow(); + await runtime.runSyncNow(); + await runtime.runSyncNow(); + + assert.deepEqual(mergedBuilds, [[1], [2, 1], [3, 2, 1], [4, 1, 3]]); + + const statePath = path.join(userDataPath, 'character-dictionaries', 'auto-sync-state.json'); + const state = JSON.parse(fs.readFileSync(statePath, 'utf8')) as { + activeMediaIds: number[]; + }; + assert.deepEqual(state.activeMediaIds, [1, 4, 3]); +}); + +test('auto sync persists rebuilt MRU state even if Yomitan import fails afterward', async () => { + const userDataPath = makeTempDir(); + const dictionariesDir = path.join(userDataPath, 'character-dictionaries'); + fs.mkdirSync(dictionariesDir, { recursive: true }); + fs.writeFileSync( + path.join(dictionariesDir, 'auto-sync-state.json'), + JSON.stringify( + { + activeMediaIds: [2, 3, 4], + mergedRevision: 'rev-2-3-4', + mergedDictionaryTitle: 'SubMiner Character Dictionary', + }, + null, + 2, + ), + ); + + const runtime = createCharacterDictionaryAutoSyncRuntimeService({ + userDataPath, + getConfig: () => ({ + enabled: true, + maxLoaded: 3, + profileScope: 'all', + }), + getOrCreateCurrentSnapshot: async () => ({ + mediaId: 1, + mediaTitle: 'Title 1', + entryCount: 10, + fromCache: true, + updatedAt: 1, + }), + buildMergedDictionary: async (mediaIds) => { + assert.deepEqual(mediaIds, [1, 2, 3]); + return { + zipPath: '/tmp/rev-1-2-3.zip', + revision: 'rev-1-2-3', + dictionaryTitle: 'SubMiner Character Dictionary', + entryCount: 30, + }; + }, + waitForYomitanMutationReady: async () => undefined, + getYomitanDictionaryInfo: async () => [], + importYomitanDictionary: async () => { + throw new Error('import failed'); + }, + deleteYomitanDictionary: async () => true, + upsertYomitanDictionarySettings: async () => true, + now: () => 1000, + }); + + await assert.rejects(runtime.runSyncNow(), /import failed/); + + const state = JSON.parse( + fs.readFileSync(path.join(dictionariesDir, 'auto-sync-state.json'), 'utf8'), + ) as { + activeMediaIds: number[]; + mergedRevision: string | null; + mergedDictionaryTitle: string | null; + }; + assert.deepEqual(state.activeMediaIds, [1, 2, 3]); + assert.equal(state.mergedRevision, 'rev-1-2-3'); + assert.equal(state.mergedDictionaryTitle, 'SubMiner Character Dictionary'); +}); + test('auto sync invokes completion callback after successful sync', async () => { const userDataPath = makeTempDir(); const completions: Array<{ mediaId: number; mediaTitle: string; changed: boolean }> = []; diff --git a/src/main/runtime/character-dictionary-auto-sync.ts b/src/main/runtime/character-dictionary-auto-sync.ts index e214bbe..1b0cc4c 100644 --- a/src/main/runtime/character-dictionary-auto-sync.ts +++ b/src/main/runtime/character-dictionary-auto-sync.ts @@ -107,6 +107,13 @@ function arraysEqual(left: number[], right: number[]): boolean { return true; } +function sameMembership(left: number[], right: number[]): boolean { + if (left.length !== right.length) return false; + const leftSorted = [...left].sort((a, b) => a - b); + const rightSorted = [...right].sort((a, b) => a - b); + return arraysEqual(leftSorted, rightSorted); +} + function buildSyncingMessage(mediaTitle: string): string { return `Updating character dictionary for ${mediaTitle}...`; } @@ -223,10 +230,11 @@ export function createCharacterDictionaryAutoSyncRuntimeService( `[dictionary:auto-sync] active AniList media set: ${nextActiveMediaIds.join(', ')}`, ); - const retainedChanged = !arraysEqual(nextActiveMediaIds, state.activeMediaIds); + const retainedOrderChanged = !arraysEqual(nextActiveMediaIds, state.activeMediaIds); + const retainedMembershipChanged = !sameMembership(nextActiveMediaIds, state.activeMediaIds); let merged: MergedCharacterDictionaryBuildResult | null = null; if ( - retainedChanged || + retainedMembershipChanged || !state.mergedRevision || !state.mergedDictionaryTitle || !snapshot.fromCache @@ -247,6 +255,12 @@ export function createCharacterDictionaryAutoSyncRuntimeService( throw new Error('Merged character dictionary state is incomplete.'); } + writeAutoSyncState(statePath, { + activeMediaIds: nextActiveMediaIds, + mergedRevision: merged?.revision ?? revision, + mergedDictionaryTitle: merged?.dictionaryTitle ?? dictionaryTitle, + }); + await deps.waitForYomitanMutationReady?.(); const dictionaryInfo = await withOperationTimeout( @@ -263,7 +277,7 @@ export function createCharacterDictionaryAutoSyncRuntimeService( existing === null || existingRevision === null || existingRevision !== revision; - let changed = merged !== null; + let changed = merged !== null || retainedOrderChanged; if (shouldImport) { deps.onSyncStatus?.({ @@ -299,11 +313,6 @@ export function createCharacterDictionaryAutoSyncRuntimeService( ); changed = changed || settingsUpdated === true; - writeAutoSyncState(statePath, { - activeMediaIds: nextActiveMediaIds, - mergedRevision: merged?.revision ?? revision, - mergedDictionaryTitle: merged?.dictionaryTitle ?? dictionaryTitle, - }); deps.logInfo?.( `[dictionary:auto-sync] synced AniList ${snapshot.mediaId}: ${dictionaryTitle} (${snapshot.entryCount} entries)`, );