From d6e6e29b5eba386151606b352d4be950ac3af909 Mon Sep 17 00:00:00 2001 From: sudacode Date: Mon, 10 Aug 2026 23:06:00 -0700 Subject: [PATCH] fix(stats): fix data loss and error handling in anime merge/move - flush pending telemetry before merge/move so in-progress session time isn't dropped from lifetime totals - repoint subtitle lines by video_id instead of anime_id so lines recorded before the async title parse assigns a link aren't stranded - stop absorbing metadata into the target when moving an episode out of an emptied entry (a move isn't a same-show claim) - return 404 only for missing episode/target, not storage failures; return 404 when a merge folds nothing - keep AnimeMergeDialog open while a merge is in flight instead of letting dismiss race the request - surface library load failures in LibraryEntryPicker instead of showing an empty list --- .../services/__tests__/stats-server.test.ts | 39 +++++++++++++++ .../services/immersion-tracker-service.ts | 7 +++ .../__tests__/anime-merge.test.ts | 48 +++++++++++++++++++ .../services/immersion-tracker/anime-merge.ts | 26 ++++++++-- .../immersion-tracker/anime-season-repair.ts | 13 +++-- .../services/stats-server/library-routes.ts | 13 ++++- .../src/components/anime/AnimeMergeDialog.tsx | 17 +++++-- stats/src/components/anime/EpisodeList.tsx | 4 +- .../components/anime/LibraryEntryPicker.tsx | 14 +++++- 9 files changed, 164 insertions(+), 17 deletions(-) diff --git a/src/core/services/__tests__/stats-server.test.ts b/src/core/services/__tests__/stats-server.test.ts index 475566fa..b1512221 100644 --- a/src/core/services/__tests__/stats-server.test.ts +++ b/src/core/services/__tests__/stats-server.test.ts @@ -3109,6 +3109,26 @@ Aligned English subtitle }); }); + it('POST /api/stats/anime/:animeId/merge reports a merge that folded nothing as 404', async () => { + const app = createStatsApp( + createMockTracker({ + mergeAnime: async (targetAnimeId: number) => ({ + survivingAnimeId: targetAnimeId, + mergedAnimeIds: [], + movedVideos: 0, + }), + } as Partial), + ); + + const res = await app.request('/api/stats/anime/7/merge', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: '{"sourceAnimeIds":[8]}', + }); + + assert.equal(res.status, 404); + }); + it('PATCH /api/stats/media/:videoId/anime reports an unknown target as 404', async () => { const app = createStatsApp( createMockTracker({ @@ -3127,6 +3147,25 @@ Aligned English subtitle assert.equal(res.status, 404); }); + it('PATCH /api/stats/media/:videoId/anime does not disguise storage failures as 404', async () => { + const app = createStatsApp( + createMockTracker({ + moveVideoToAnime: async () => { + throw new Error('database is locked'); + }, + } as Partial), + ); + + const res = await app.request('/api/stats/media/12/anime', { + method: 'PATCH', + headers: { 'Content-Type': 'application/json' }, + body: '{"animeId":7}', + }); + + assert.notEqual(res.status, 404); + assert.equal(res.status >= 500, true); + }); + it('POST /api/stats/anki/browse returns 400 for missing noteId', async () => { const app = createStatsApp(createMockTracker()); const res = await app.request('/api/stats/anki/browse', { method: 'POST' }); diff --git a/src/core/services/immersion-tracker-service.ts b/src/core/services/immersion-tracker-service.ts index 77704711..ce2464ec 100644 --- a/src/core/services/immersion-tracker-service.ts +++ b/src/core/services/immersion-tracker-service.ts @@ -772,11 +772,18 @@ export class ImmersionTrackerService { if (pendingVideoId !== undefined) { await this.pendingAnimeMetadataUpdates.get(pendingVideoId); } + // Both of these rebuild the lifetime summaries, which recompute from the + // database: queued telemetry has to land first or the active session's + // watch time is dropped from the merged totals. + this.flushTelemetry(true); + this.flushNow(); return mergeAnimeRecords(this.db, targetAnimeId, sourceAnimeIds); } async moveVideoToAnime(videoId: number, targetAnimeId: number): Promise { await this.pendingAnimeMetadataUpdates.get(videoId); + this.flushTelemetry(true); + this.flushNow(); return moveVideoToAnimeQuery(this.db, videoId, targetAnimeId); } diff --git a/src/core/services/immersion-tracker/__tests__/anime-merge.test.ts b/src/core/services/immersion-tracker/__tests__/anime-merge.test.ts index d5e371ad..8c95aa14 100644 --- a/src/core/services/immersion-tracker/__tests__/anime-merge.test.ts +++ b/src/core/services/immersion-tracker/__tests__/anime-merge.test.ts @@ -157,6 +157,33 @@ test('mergeAnimeRecords folds episodes, lines and lifetime totals into the targe }); }); +test('mergeAnimeRecords repoints subtitle lines recorded before the anime link landed', () => { + withDb((db) => { + insertAnime(db, { animeId: 1, key: 'show', title: 'Show' }); + insertAnime(db, { animeId: 2, key: 'show season 1', title: 'Show Season 1' }); + insertEpisode(db, { videoId: 1, animeId: 1 }); + insertEpisode(db, { videoId: 2, animeId: 2, season: 1 }); + // Lines are written with the video's anime_id at the time, which is NULL + // until the async title parse assigns one. + db.prepare( + `INSERT INTO imm_subtitle_lines(session_id, video_id, anime_id, line_index, text, CREATED_DATE, LAST_UPDATE_DATE) + VALUES (2, 2, NULL, 2, 'unlinked line', ?, ?)`, + ).run(BASE_MS, BASE_MS); + + mergeAnimeRecords(db, 1, [2]); + + assert.equal(lineAnimeIds(db, 1), 3); + const orphaned = Number( + ( + db + .prepare('SELECT COUNT(*) AS total FROM imm_subtitle_lines WHERE anime_id IS NULL') + .get() as { total: number } + ).total, + ); + assert.equal(orphaned, 0); + }); +}); + test('mergeAnimeRecords inherits metadata the target is missing without clobbering its own', () => { withDb((db) => { insertAnime(db, { animeId: 1, key: 'show', title: 'Show', titleRomaji: 'Shou' }); @@ -215,6 +242,27 @@ test('moveVideoToAnime moves one episode and prunes the emptied entry', () => { .prepare('SELECT total_active_ms AS activeMs FROM imm_lifetime_anime WHERE anime_id = 1') .get() as { activeMs: number }; assert.equal(lifetime.activeMs, 6000); + // The stray entry's AniList link is dropped, not inherited: a move makes no + // claim that the two entries are the same show. + const target = db + .prepare('SELECT anilist_id AS anilistId FROM imm_anime WHERE anime_id = 1') + .get() as { anilistId: number | null }; + assert.equal(target.anilistId, null); + }); +}); + +test('moveVideoToAnime is a no-op when the episode is already in the target entry', () => { + withDb((db) => { + insertAnime(db, { animeId: 1, key: 'show', title: 'Show' }); + insertEpisode(db, { videoId: 1, animeId: 1 }); + + const summary = moveVideoToAnime(db, 1, 1); + + assert.equal(summary.targetAnimeId, 1); + assert.equal(summary.previousAnimeId, 1); + assert.equal(summary.removedPreviousAnime, false); + assert.deepEqual(animeIds(db), [1]); + assert.equal(videoAnimeId(db, 1), 1); }); }); diff --git a/src/core/services/immersion-tracker/anime-merge.ts b/src/core/services/immersion-tracker/anime-merge.ts index 0622ab69..77f931a0 100644 --- a/src/core/services/immersion-tracker/anime-merge.ts +++ b/src/core/services/immersion-tracker/anime-merge.ts @@ -3,6 +3,9 @@ import { rebuildLifetimeSummariesInTransaction } from './lifetime'; import { toDbTimestamp } from './query-shared'; import { nowMs } from './time'; +/** Thrown when a move names an episode or destination entry that is not there. */ +export const UNKNOWN_MOVE_TARGET_MESSAGE = 'Unknown episode or target library entry'; + export interface AnimeMergeSummary { /** Library entry that owns every moved episode once the merge finishes. */ survivingAnimeId: number; @@ -161,11 +164,17 @@ export function mergeAnimeRecordsInTransaction( } const updatedAt = toDbTimestamp(nowMs()); + const sourceVideosStmt = db.prepare( + 'SELECT video_id AS videoId FROM imm_videos WHERE anime_id = ?', + ); const moveVideosStmt = db.prepare( 'UPDATE imm_videos SET anime_id = ?, LAST_UPDATE_DATE = ? WHERE anime_id = ?', ); + // Repointed per video rather than by anime_id: lines recorded before the + // async title parse assigns the link are stored with a NULL anime_id, and + // matching on the source id would strand them unattributed. const moveLinesStmt = db.prepare( - 'UPDATE imm_subtitle_lines SET anime_id = ?, LAST_UPDATE_DATE = ? WHERE anime_id = ?', + 'UPDATE imm_subtitle_lines SET anime_id = ?, LAST_UPDATE_DATE = ? WHERE video_id = ?', ); const dropLifetimeStmt = db.prepare('DELETE FROM imm_lifetime_anime WHERE anime_id = ?'); const dropAnimeStmt = db.prepare('DELETE FROM imm_anime WHERE anime_id = ?'); @@ -176,10 +185,15 @@ export function mergeAnimeRecordsInTransaction( } const sourceMetadata = readAnimeMetadata(db, sourceAnimeId); + const sourceVideoIds = (sourceVideosStmt.all(sourceAnimeId) as Array<{ videoId: number }>).map( + (row) => row.videoId, + ); const moved = moveVideosStmt.run(targetAnimeId, updatedAt, sourceAnimeId) as { changes: number; }; - moveLinesStmt.run(targetAnimeId, updatedAt, sourceAnimeId); + for (const videoId of sourceVideoIds) { + moveLinesStmt.run(targetAnimeId, updatedAt, videoId); + } dropLifetimeStmt.run(sourceAnimeId); dropAnimeStmt.run(sourceAnimeId); absorbAnimeMetadata(db, targetAnimeId, sourceMetadata, updatedAt); @@ -219,7 +233,7 @@ export function moveVideoToAnime( .prepare('SELECT anime_id AS animeId FROM imm_videos WHERE video_id = ?') .get(videoId) as { animeId: number | null } | null; if (!videoRow || !animeExists(db, targetAnimeId)) { - throw new Error('Unknown episode or target library entry'); + throw new Error(UNKNOWN_MOVE_TARGET_MESSAGE); } const previousAnimeId = videoRow.animeId; @@ -239,10 +253,12 @@ export function moveVideoToAnime( let removedPreviousAnime = false; if (previousAnimeId !== null && !hasAnimeReferences(db, previousAnimeId)) { - const sourceMetadata = readAnimeMetadata(db, previousAnimeId); + // The emptied entry's metadata is deliberately dropped rather than + // absorbed. A move says "this episode belongs elsewhere", not "these are + // the same show", and the entry being emptied is usually a mis-parse + // whose AniList link would be wrong for the target. db.prepare('DELETE FROM imm_lifetime_anime WHERE anime_id = ?').run(previousAnimeId); db.prepare('DELETE FROM imm_anime WHERE anime_id = ?').run(previousAnimeId); - absorbAnimeMetadata(db, targetAnimeId, sourceMetadata, updatedAt); removedPreviousAnime = true; } diff --git a/src/core/services/immersion-tracker/anime-season-repair.ts b/src/core/services/immersion-tracker/anime-season-repair.ts index 80aac9bc..76bc5740 100644 --- a/src/core/services/immersion-tracker/anime-season-repair.ts +++ b/src/core/services/immersion-tracker/anime-season-repair.ts @@ -360,9 +360,11 @@ export function resolveAnimeAnilistConflict( const summary = emptySummary(1); summary.movedVideos = merge.movedVideos; summary.deletedAnimeRows = merge.mergedAnimeIds.length; - summary.survivingAnimeId = survivingAnimeId; if (merge.mergedAnimeIds.length > 0) { summary.repaired = 1; + // Only reported once a row really absorbed the other, so callers never + // follow this to an anime id that was never written. + summary.survivingAnimeId = survivingAnimeId; } // Lifetime summaries are rebuilt by the caller off this summary, the same // as the redistribution path below. @@ -383,11 +385,16 @@ function canMergeAnilistConflict( anilistId: number, options: AnimeAnilistConflictOptions, ): boolean { + const targetRow = getAnimeRow(db, targetAnimeId); + if (!targetRow) { + // Nothing to merge with a row that no longer exists (a stale id from the + // caller); fall through to the redistribution path. + return false; + } if (options.survivor !== 'target') { // The target is the row about to disappear here, so an existing link of its // own means this is a mis-resolution rather than a duplicate: leave it be. - const targetRow = getAnimeRow(db, targetAnimeId); - if (targetRow?.anilist_id != null && targetRow.anilist_id !== anilistId) { + if (targetRow.anilist_id != null && targetRow.anilist_id !== anilistId) { return false; } } diff --git a/src/core/services/stats-server/library-routes.ts b/src/core/services/stats-server/library-routes.ts index 4fd6d6b6..dfe4f955 100644 --- a/src/core/services/stats-server/library-routes.ts +++ b/src/core/services/stats-server/library-routes.ts @@ -1,5 +1,6 @@ import type { Hono } from 'hono'; import { statsJson } from '../../../types/stats-http-contract.js'; +import { UNKNOWN_MOVE_TARGET_MESSAGE } from '../immersion-tracker/anime-merge.js'; import type { ImmersionTrackerService } from '../immersion-tracker-service.js'; import { buildSentenceSearchOptions, @@ -206,6 +207,9 @@ export function registerStatsLibraryRoutes( const sourceAnimeIds = parsePositiveIdList(body?.sourceAnimeIds).filter((id) => id !== animeId); if (sourceAnimeIds.length === 0) return c.body(null, 400); const summary = await tracker.mergeAnime(animeId, sourceAnimeIds); + // Nothing folded means the target or every source was already gone, so the + // caller should not be told the merge succeeded. + if (summary.mergedAnimeIds.length === 0) return c.body(null, 404); return c.json( statsJson('mergeAnime', { ok: true, @@ -232,8 +236,13 @@ export function registerStatsLibraryRoutes( removedPreviousAnime: summary.removedPreviousAnime, }), ); - } catch { - return c.body(null, 404); + } catch (error) { + // Only a missing episode or entry is a 404; storage failures must not be + // reported to the caller as "not found". + if (error instanceof Error && error.message === UNKNOWN_MOVE_TARGET_MESSAGE) { + return c.body(null, 404); + } + throw error; } }); } diff --git a/stats/src/components/anime/AnimeMergeDialog.tsx b/stats/src/components/anime/AnimeMergeDialog.tsx index 6c42b8ee..a9319c56 100644 --- a/stats/src/components/anime/AnimeMergeDialog.tsx +++ b/stats/src/components/anime/AnimeMergeDialog.tsx @@ -43,8 +43,18 @@ export function AnimeMergeDialog({ entries, onClose, onMerged }: AnimeMergeDialo } }; + // Dismissing mid-request would leave the caller unaware of a merge that is + // still going to land, so the backdrop and close button are inert until it + // resolves. + const handleDismiss = () => { + if (!merging) onClose(); + }; + return ( -
+
diff --git a/stats/src/components/anime/EpisodeList.tsx b/stats/src/components/anime/EpisodeList.tsx index d07a5593..daddafdc 100644 --- a/stats/src/components/anime/EpisodeList.tsx +++ b/stats/src/components/anime/EpisodeList.tsx @@ -205,7 +205,7 @@ export function EpisodeList({ setMoveError(null); setMovingEpisode(ep); }} - className={`w-5 h-5 rounded border border-ctp-surface2 text-transparent hover:border-ctp-blue/50 hover:text-ctp-blue hover:bg-ctp-blue/10 transition-colors text-xs flex items-center justify-center ${HOVER_REVEALED}`} + className={`w-5 h-5 rounded border border-ctp-surface2 text-transparent hover:border-ctp-blue/50 hover:text-ctp-blue focus-visible:text-ctp-blue hover:bg-ctp-blue/10 transition-colors text-xs flex items-center justify-center ${HOVER_REVEALED}`} title="Move to another library entry" aria-label="Move to another library entry" > @@ -217,7 +217,7 @@ export function EpisodeList({ e.stopPropagation(); void handleDeleteEpisode(ep.videoId, ep.canonicalTitle); }} - className={`w-5 h-5 rounded border border-ctp-surface2 text-transparent hover:border-ctp-red/50 hover:text-ctp-red hover:bg-ctp-red/10 transition-colors text-xs flex items-center justify-center ${HOVER_REVEALED}`} + className={`w-5 h-5 rounded border border-ctp-surface2 text-transparent hover:border-ctp-red/50 hover:text-ctp-red focus-visible:text-ctp-red hover:bg-ctp-red/10 transition-colors text-xs flex items-center justify-center ${HOVER_REVEALED}`} title="Delete episode" aria-label="Delete episode" > diff --git a/stats/src/components/anime/LibraryEntryPicker.tsx b/stats/src/components/anime/LibraryEntryPicker.tsx index 68663735..dd914009 100644 --- a/stats/src/components/anime/LibraryEntryPicker.tsx +++ b/stats/src/components/anime/LibraryEntryPicker.tsx @@ -25,6 +25,7 @@ export function LibraryEntryPicker({ onClose, }: LibraryEntryPickerProps) { const [entries, setEntries] = useState(null); + const [loadFailed, setLoadFailed] = useState(false); const [query, setQuery] = useState(initialQuery); const inputRef = useRef(null); @@ -37,7 +38,11 @@ export function LibraryEntryPicker({ if (!cancelled) setEntries(data); }) .catch(() => { - if (!cancelled) setEntries([]); + // Distinct from an empty library: telling the user "no other titles" + // when the request failed hides a retryable error. + if (cancelled) return; + setEntries([]); + setLoadFailed(true); }); return () => { cancelled = true; @@ -84,7 +89,12 @@ export function LibraryEntryPicker({
{entries === null &&
Loading...
} - {entries !== null && visible.length === 0 && ( + {loadFailed && ( +
+ Could not load the library. Close this dialog and try again. +
+ )} + {!loadFailed && entries !== null && visible.length === 0 && (
No other titles
)} {visible.map((entry) => (