From d5f887a8341ff53ef76064d17b053bc204c34bb4 Mon Sep 17 00:00:00 2001 From: sudacode Date: Tue, 28 Jul 2026 02:50:36 -0700 Subject: [PATCH] fix(stats): address CodeRabbit review on delete and vocabulary paths - Plan lexical removals inside each delete transaction. The plan drives a subtraction now rather than a recompute, so a snapshot taken before BEGIN IMMEDIATE could subtract totals that no longer match the rows removed, and only drift reaching <= 0 self-heals. - Await the active video's pending anime metadata before deleteAnime's guard, so a delete issued while the title association is still being parsed can't slip past and let the late update recreate the anime row. - Order vocabulary pages by (frequency, id). The oversample loop re-runs with a growing OFFSET, and tied frequencies gave no stable order, so rows could be skipped or repeated across pages. - Clear the delete error when a retry starts, so cancelling leaves no stale failure on screen. - Seed occurrences, kanji, rollups and cover art in the deleteAnime test; the empty-table assertions were passing against empty tables. - Rename both keyframes to kebab-case for stylelint keyframes-name-pattern. --- .../immersion-tracker-service.test.ts | 113 ++++++++++++++++-- .../services/immersion-tracker-service.ts | 8 ++ .../immersion-tracker/query-lexical.ts | 4 +- .../immersion-tracker/query-maintenance.ts | 77 ++++++------ .../src/components/anime/AnimeDetailView.tsx | 3 + stats/src/styles/globals.css | 8 +- 6 files changed, 160 insertions(+), 53 deletions(-) diff --git a/src/core/services/immersion-tracker-service.test.ts b/src/core/services/immersion-tracker-service.test.ts index d5f390c7..f7dbf77d 100644 --- a/src/core/services/immersion-tracker-service.test.ts +++ b/src/core/services/immersion-tracker-service.test.ts @@ -1466,6 +1466,64 @@ test('deleteVideo ignores the currently active video and keeps new writes flusha } }); +/** + * Attach the derived rows a real session would produce to every recorded line + * of `animeId`: word/kanji entries and their occurrences, monthly rollups, and + * cover art backed by a shared blob. + */ +function seedDerivedAnimeData(db: DatabaseSync, animeId: number): void { + const lines = db + .prepare( + 'SELECT line_id AS lineId, CREATED_DATE AS seenMs FROM imm_subtitle_lines WHERE anime_id = ?', + ) + .all(animeId) as Array<{ lineId: number; seenMs: number }>; + assert.ok(lines.length > 0, 'expected recorded subtitle lines to decorate'); + + db.prepare( + `INSERT INTO imm_words(id, headword, word, reading, part_of_speech, pos1, first_seen, last_seen, frequency) + VALUES (9001, '天気', '天気', 'てんき', 'noun', '名詞', 0, 0, 0)`, + ).run(); + db.prepare( + `INSERT INTO imm_kanji(id, kanji, first_seen, last_seen, frequency) VALUES (9101, '気', 0, 0, 0)`, + ).run(); + + const insertWordOccurrence = db.prepare( + 'INSERT INTO imm_word_line_occurrences(line_id, word_id, occurrence_count, seen_ms) VALUES (?, 9001, 1, ?)', + ); + const insertKanjiOccurrence = db.prepare( + 'INSERT INTO imm_kanji_line_occurrences(line_id, kanji_id, occurrence_count, seen_ms) VALUES (?, 9101, 1, ?)', + ); + for (const line of lines) { + insertWordOccurrence.run(line.lineId, line.seenMs); + insertKanjiOccurrence.run(line.lineId, line.seenMs); + } + db.prepare( + `UPDATE imm_words SET frequency = ?, first_seen = ?, last_seen = ? WHERE id = 9001`, + ).run(lines.length, 0, 0); + + const videoIds = ( + db + .prepare('SELECT video_id AS videoId FROM imm_videos WHERE anime_id = ?') + .all(animeId) as Array<{ videoId: number }> + ).map((row) => row.videoId); + const insertMonthlyRollup = db.prepare( + `INSERT INTO imm_monthly_rollups(rollup_month, video_id, total_sessions, CREATED_DATE, LAST_UPDATE_DATE) + VALUES (202401, ?, 1, '0', '0')`, + ); + const insertArt = db.prepare( + `INSERT INTO imm_media_art(video_id, anilist_id, cover_url, cover_blob, cover_blob_hash, fetched_at_ms, CREATED_DATE, LAST_UPDATE_DATE) + VALUES (?, 4242, 'https://example.test/cover.jpg', NULL, 'deadbeef', '0', '0', '0')`, + ); + db.prepare( + `INSERT INTO imm_cover_art_blobs(blob_hash, cover_blob, CREATED_DATE, LAST_UPDATE_DATE) + VALUES ('deadbeef', X'FFD8FFD9', '0', '0')`, + ).run(); + for (const videoId of videoIds) { + insertMonthlyRollup.run(videoId); + insertArt.run(videoId); + } +} + test('deleteAnime removes every episode, session and library row for the title', async () => { const dbPath = makeDbPath(); let tracker: ImmersionTrackerService | null = null; @@ -1492,6 +1550,29 @@ test('deleteAnime removes every episode, session and library row for the title', )?.anime_id; assert.ok(animeId); + // The tokenizer does not run in this harness, so attach vocabulary, kanji, + // rollups and cover art to the recorded lines by hand. Without them the + // "everything is gone" assertions below would pass against empty tables. + seedDerivedAnimeData(privateApi.db, animeId); + + const countOf = (sql: string): number => + (privateApi.db.prepare(sql).get() as { total: number }).total; + for (const table of [ + 'imm_words', + 'imm_kanji', + 'imm_word_line_occurrences', + 'imm_kanji_line_occurrences', + 'imm_daily_rollups', + 'imm_monthly_rollups', + 'imm_media_art', + 'imm_cover_art_blobs', + ]) { + assert.ok( + countOf(`SELECT COUNT(*) AS total FROM ${table}`) > 0, + `precondition: ${table} should hold rows before the delete`, + ); + } + const libraryBefore = await tracker.getAnimeLibrary(); assert.equal(libraryBefore.length, 1); assert.equal(libraryBefore[0]?.episodeCount, 2); @@ -1501,16 +1582,28 @@ test('deleteAnime removes every episode, session and library row for the title', const libraryAfter = await tracker.getAnimeLibrary(); assert.equal(libraryAfter.length, 0); - const countOf = (sql: string): number => - (privateApi.db.prepare(sql).get() as { total: number }).total; - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_anime'), 0); - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_lifetime_anime'), 0); - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_videos'), 0); - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_sessions'), 0); - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_subtitle_lines'), 0); - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_daily_rollups'), 0); - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_lifetime_media'), 0); - assert.equal(countOf('SELECT COUNT(*) AS total FROM imm_words'), 0); + for (const table of [ + 'imm_anime', + 'imm_lifetime_anime', + 'imm_videos', + 'imm_sessions', + 'imm_subtitle_lines', + 'imm_daily_rollups', + 'imm_monthly_rollups', + 'imm_lifetime_media', + 'imm_words', + 'imm_kanji', + 'imm_word_line_occurrences', + 'imm_kanji_line_occurrences', + 'imm_media_art', + 'imm_cover_art_blobs', + ]) { + assert.equal( + countOf(`SELECT COUNT(*) AS total FROM ${table}`), + 0, + `${table} should be empty after deleting the only title`, + ); + } } finally { tracker?.destroy(); cleanupDbPath(dbPath); diff --git a/src/core/services/immersion-tracker-service.ts b/src/core/services/immersion-tracker-service.ts index de862fe4..cbbe7c73 100644 --- a/src/core/services/immersion-tracker-service.ts +++ b/src/core/services/immersion-tracker-service.ts @@ -735,6 +735,14 @@ export class ImmersionTrackerService { } async deleteAnime(animeId: number): Promise { + // The active video's anime link is assigned asynchronously after the title + // is parsed, so a guard reading imm_videos too early sees a null and lets + // the delete through — then the late update recreates the anime row. + const pendingVideoId = this.sessionState?.videoId; + if (pendingVideoId !== undefined) { + await this.pendingAnimeMetadataUpdates.get(pendingVideoId); + } + const activeVideoId = this.sessionState?.videoId; if (activeVideoId !== undefined) { const activeAnime = this.db diff --git a/src/core/services/immersion-tracker/query-lexical.ts b/src/core/services/immersion-tracker/query-lexical.ts index 029e1ca0..13533158 100644 --- a/src/core/services/immersion-tracker/query-lexical.ts +++ b/src/core/services/immersion-tracker/query-lexical.ts @@ -125,7 +125,7 @@ export function getVocabularyStats( frequency, frequency_rank, first_seen, last_seen FROM imm_words ${whereClause} - ORDER BY frequency DESC + ORDER BY frequency DESC, id LIMIT ? OFFSET ? ) SELECT p.id AS wordId, p.headword, p.word, p.reading, @@ -137,7 +137,7 @@ export function getVocabularyStats( LEFT JOIN imm_word_line_occurrences o ON o.word_id = p.id LEFT JOIN imm_subtitle_lines sl ON sl.line_id = o.line_id AND sl.anime_id IS NOT NULL GROUP BY p.id - ORDER BY p.frequency DESC + ORDER BY p.frequency DESC, p.id `); const visibleRows: VocabularyStatsRow[] = []; let offset = 0; diff --git a/src/core/services/immersion-tracker/query-maintenance.ts b/src/core/services/immersion-tracker/query-maintenance.ts index af740401..9f9b1650 100644 --- a/src/core/services/immersion-tracker/query-maintenance.ts +++ b/src/core/services/immersion-tracker/query-maintenance.ts @@ -483,11 +483,14 @@ export function isVideoWatched(db: DatabaseSync, videoId: number): boolean { export function deleteSession(db: DatabaseSync, sessionId: number): void { const sessionIds = [sessionId]; - const lexicalRemovals = planLexicalRemovalsForSessions(db, sessionIds); - const affectedRollupGroups = getRollupGroupsForSessions(db, sessionIds); db.exec('BEGIN IMMEDIATE'); try { + // Measured inside the write lock: the plan records what the delete removes, + // and applying a plan taken against a different snapshot would subtract the + // wrong totals from imm_words/imm_kanji. + const lexicalRemovals = planLexicalRemovalsForSessions(db, sessionIds); + const affectedRollupGroups = getRollupGroupsForSessions(db, sessionIds); deleteSessionsByIds(db, sessionIds); applyLexicalRemovals(db, lexicalRemovals); rebuildLifetimeSummariesInTransaction(db); @@ -501,11 +504,11 @@ export function deleteSession(db: DatabaseSync, sessionId: number): void { export function deleteSessions(db: DatabaseSync, sessionIds: number[]): void { if (sessionIds.length === 0) return; - const lexicalRemovals = planLexicalRemovalsForSessions(db, sessionIds); - const affectedRollupGroups = getRollupGroupsForSessions(db, sessionIds); db.exec('BEGIN IMMEDIATE'); try { + const lexicalRemovals = planLexicalRemovalsForSessions(db, sessionIds); + const affectedRollupGroups = getRollupGroupsForSessions(db, sessionIds); deleteSessionsByIds(db, sessionIds); applyLexicalRemovals(db, lexicalRemovals); rebuildLifetimeSummariesInTransaction(db); @@ -526,30 +529,30 @@ export function deleteSessions(db: DatabaseSync, sessionIds: number[]): void { * pay for one full rebuild per episode. */ export function deleteAnime(db: DatabaseSync, animeId: number): void { - const videoIds = ( - db.prepare('SELECT video_id FROM imm_videos WHERE anime_id = ?').all(animeId) as Array<{ - video_id: number; - }> - ).map((row) => row.video_id); - - const lexicalRemovals = planLexicalRemovalsForVideos(db, videoIds); - const coverBlobHashes: string[] = []; - const sessionIds: number[] = []; - for (const videoId of videoIds) { - const artRow = db - .prepare('SELECT cover_blob_hash AS coverBlobHash FROM imm_media_art WHERE video_id = ?') - .get(videoId) as { coverBlobHash: string | null } | undefined; - if (artRow?.coverBlobHash) { - coverBlobHashes.push(artRow.coverBlobHash); - } - const sessions = db - .prepare('SELECT session_id FROM imm_sessions WHERE video_id = ?') - .all(videoId) as Array<{ session_id: number }>; - sessionIds.push(...sessions.map((session) => session.session_id)); - } - db.exec('BEGIN IMMEDIATE'); try { + const videoIds = ( + db.prepare('SELECT video_id FROM imm_videos WHERE anime_id = ?').all(animeId) as Array<{ + video_id: number; + }> + ).map((row) => row.video_id); + + const lexicalRemovals = planLexicalRemovalsForVideos(db, videoIds); + const coverBlobHashes: string[] = []; + const sessionIds: number[] = []; + for (const videoId of videoIds) { + const artRow = db + .prepare('SELECT cover_blob_hash AS coverBlobHash FROM imm_media_art WHERE video_id = ?') + .get(videoId) as { coverBlobHash: string | null } | undefined; + if (artRow?.coverBlobHash) { + coverBlobHashes.push(artRow.coverBlobHash); + } + const sessions = db + .prepare('SELECT session_id FROM imm_sessions WHERE video_id = ?') + .all(videoId) as Array<{ session_id: number }>; + sessionIds.push(...sessions.map((session) => session.session_id)); + } + deleteSessionsByIds(db, sessionIds); const deleteLinesStmt = db.prepare('DELETE FROM imm_subtitle_lines WHERE video_id = ?'); const deleteDailyStmt = db.prepare('DELETE FROM imm_daily_rollups WHERE video_id = ?'); @@ -578,22 +581,22 @@ export function deleteAnime(db: DatabaseSync, animeId: number): void { } export function deleteVideo(db: DatabaseSync, videoId: number): void { - const artRow = db - .prepare( - ` + db.exec('BEGIN IMMEDIATE'); + try { + const artRow = db + .prepare( + ` SELECT cover_blob_hash AS coverBlobHash FROM imm_media_art WHERE video_id = ? `, - ) - .get(videoId) as { coverBlobHash: string | null } | undefined; - const lexicalRemovals = planLexicalRemovalsForVideos(db, [videoId]); - const sessions = db - .prepare('SELECT session_id FROM imm_sessions WHERE video_id = ?') - .all(videoId) as Array<{ session_id: number }>; + ) + .get(videoId) as { coverBlobHash: string | null } | undefined; + const lexicalRemovals = planLexicalRemovalsForVideos(db, [videoId]); + const sessions = db + .prepare('SELECT session_id FROM imm_sessions WHERE video_id = ?') + .all(videoId) as Array<{ session_id: number }>; - db.exec('BEGIN IMMEDIATE'); - try { deleteSessionsByIds( db, sessions.map((session) => session.session_id), diff --git a/stats/src/components/anime/AnimeDetailView.tsx b/stats/src/components/anime/AnimeDetailView.tsx index d6cc8739..b8e28518 100644 --- a/stats/src/components/anime/AnimeDetailView.tsx +++ b/stats/src/components/anime/AnimeDetailView.tsx @@ -165,6 +165,9 @@ export function AnimeDetailView({ const handleDeleteAnime = async () => { if (isDeletingAnimeRef.current) return; isDeletingAnimeRef.current = true; + // Cleared up front so cancelling a retry doesn't leave the previous + // attempt's failure on screen. + setDeleteError(null); let confirmed = false; try { confirmed = await confirmAnimeDelete(detail.canonicalTitle, detail.episodeCount); diff --git a/stats/src/styles/globals.css b/stats/src/styles/globals.css index f18b80a7..457e87e9 100644 --- a/stats/src/styles/globals.css +++ b/stats/src/styles/globals.css @@ -80,7 +80,7 @@ body.overlay-mode #root { } /* Tab content entrance animation */ -@keyframes fadeSlideIn { +@keyframes fade-slide-in { from { opacity: 0; transform: translateY(6px); @@ -92,11 +92,11 @@ body.overlay-mode #root { } .animate-fade-in { - animation: fadeSlideIn 0.25s ease-out; + animation: fade-slide-in 0.25s ease-out; } /* Indeterminate progress sweep for the global delete indicator */ -@keyframes indeterminateSweep { +@keyframes indeterminate-sweep { from { transform: translateX(-100%); } @@ -106,5 +106,5 @@ body.overlay-mode #root { } .animate-indeterminate { - animation: indeterminateSweep 1.1s ease-in-out infinite; + animation: indeterminate-sweep 1.1s ease-in-out infinite; }