From 745c11ebb0fac0d5b6635505d1c62214cc051608 Mon Sep 17 00:00:00 2001 From: Darrell van Swinderen Date: Wed, 23 Sep 2026 11:51:21 +0200 Subject: [PATCH] fix(compress): compressing works in a real library, and says how it went **Compression failed on every clip in a real library, after the encode.** Rows store paths with forward slashes, the way fast-glob returns them, and `CompressClipAction` handed that straight to `shell.trashItem`, which on Windows refuses it with "Failed to parse path". `MoveFileToTrashAction` already normalises for exactly this; compression now goes through it. Checked in Electron: a forward-slash path fails, a normalised one lands in the Recycle Bin. 232 of the 233 clips in the real library store their path that way. **It was silent about it.** The menu said "It will finish in the background" and nothing more, so a compressed clip, one left alone for being already small, and one that failed all looked the same, with the card's old size. `useCompressionResult` now follows the job: the card gets the new row the moment it lands, and one toast says "60.6 MB to 19.2 MB", why it was left alone, or why it failed. Batch compression reports each. **The bench could not have caught it**, because its shell stub deleted any path it was given. It now refuses a forward slash the way Windows does and stores the row's path the way a real library does, and it takes a file: `node scripts/compress-check.mjs `. Proved: without the fix it fails with "Failed to parse path"; with it, the real clip goes 60.6 to 19.2 MB. **The clip's name is bold in the question**, through a new `emphasis` option on the confirm dialog (plain text either side, never HTML). **A duplicate row found on the way.** `Edited_2026-09-19` had two rows, one per slash spelling, from a build older than the scan's path matching. The scan never removed the old one because the file exists. It now retires a second row for a file when that row holds nothing its twin lacks (`services/duplicateRows.ts`, unit-tested): rows only, never the file, and a duplicate carrying anything of its own is left and logged. In the test profile it removed the empty twin and kept the published row. Gates: `npm run check` 866; `compress-check.mjs` on the real clip, exit 0; `indexing`, `compress` and `modals` specs, 12 passed. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 10 +++ scripts/compress-check.mjs | 27 +++--- src/main/actions/CompressClipAction.ts | 9 +- src/main/actions/ScanAndSyncClipsAction.ts | 44 +++++++++- src/main/routes/clips.ts | 12 +++ src/main/services/duplicateRows.ts | 86 +++++++++++++++++++ src/renderer/src/App.vue | 1 + .../src/components/Base/BaseConfirmDialog.vue | 22 ++++- .../composables/clips/useCompressionResult.ts | 46 ++++++++++ .../composables/library/useBatchOperations.ts | 15 +++- src/renderer/src/composables/ui/useConfirm.ts | 7 ++ .../src/helpers/clipActionHandlers.ts | 12 ++- tests/unit/main/duplicateRows.spec.ts | 63 ++++++++++++++ 13 files changed, 333 insertions(+), 21 deletions(-) create mode 100644 src/main/services/duplicateRows.ts create mode 100644 src/renderer/src/composables/clips/useCompressionResult.ts create mode 100644 tests/unit/main/duplicateRows.spec.ts diff --git a/CLAUDE.md b/CLAUDE.md index abe00156..4213df8d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -291,6 +291,16 @@ notes**, which is the one place somebody wrote down what happened in a clip. Every non-trivial operation is a class in `src/main/actions/` extending `BaseAction` with a single `execute(input)`. **Add new business logic as an Action**, not inline in a route. +**Anything that sends a file to the Recycle Bin goes through `MoveFileToTrashAction`**, which +normalises the path. Rows store forward slashes (fast-glob's spelling) and `shell.trashItem` refuses +those with "Failed to parse path"; compressing a clip called it directly and failed, after the encode, +on every clip in a real library. `compress-check.mjs`'s shell stub now refuses a forward slash too. + +**A second row for one file is retired by the scan when it holds nothing of its own** +(`services/duplicateRows.ts`). Older builds indexed some files under both slash spellings; the scan +stopped making new ones but never removed an old one, so the clip showed twice. Only the row goes, +never the file, and a duplicate that carries anything its twin lacks is left and logged. + `ScanAndSyncClipsAction` has a **prune guard**: it refuses to delete rows when the videos folder looks empty or when one scan would remove more than half the library. A clip row carries the only copy of its tags, notes, display name, collections and stars, and an unmounted drive makes every file look diff --git a/scripts/compress-check.mjs b/scripts/compress-check.mjs index 334cbcb2..4ede12f9 100644 --- a/scripts/compress-check.mjs +++ b/scripts/compress-check.mjs @@ -109,14 +109,17 @@ const isGreen = ({ rgb: [r, g, b] }) => g > 60 && g - r > 40 && g - b > 40; /* ------------------------------------------------------------ the source */ announceRoot(ROOT); -const dir = join(ROOT, game); +// A file instead of a game folder runs the bench on that exact recording, +// which is how a clip somebody reports as not compressing gets reproduced. +const givenFile = VIDEO.test(game) && existsSync(game) ? game : null; +const dir = givenFile ? join(givenFile, '..') : join(ROOT, game); if (!existsSync(dir)) { console.error(`no such folder: ${dir}`); process.exit(2); } -let source = null; -for (const name of readdirSync(dir).filter((f) => VIDEO.test(f) && !f.startsWith('.'))) { +let source = givenFile; +for (const name of givenFile ? [] : readdirSync(dir).filter((f) => VIDEO.test(f) && !f.startsWith('.'))) { const candidate = join(dir, name); if ((await seconds(candidate)) >= MIN_SOURCE_SEC) { source = candidate; @@ -133,10 +136,11 @@ mkdirSync(TMP, { recursive: true }); const library = join(tmpdir(), `goodbit-compress-check-${Date.now()}`); const profile = join(library, '.profile'); -mkdirSync(join(library, game), { recursive: true }); +const folder = givenFile ? 'Clips' : game; +mkdirSync(join(library, folder), { recursive: true }); mkdirSync(profile, { recursive: true }); -const working = join(library, game, source.split(/[\\/]/).pop()); +const working = join(library, folder, source.split(/[\\/]/).pop()); copyFileSync(source, working); /* @@ -176,7 +180,9 @@ writeFileSync( // The real one moves a file to the Recycle Bin. There is no bin to check // here and leaving the original in place would make the rename fail, so it // is deleted, and the bench asserts the sequence rather than the bin. - 'export const shell = { trashItem: (p) => rm(p, { force: true }) };', + // Refuses a forward-slash path exactly as the real one does on Windows, which is + // how every compression in a real library failed while this bench passed. + 'export const shell = { trashItem: async (p) => { if (process.platform === "win32" && p.includes("/")) throw new Error("Failed to parse path"); await rm(p, { force: true }); } };', "export const screen = { getPrimaryDisplay: () => ({ id: 0, label: '' }), getAllDisplays: () => [] };", 'export default { app, shell, screen };', '', @@ -220,11 +226,12 @@ const before = { const clip = await repo.save( repo.create({ - filePath: working, + // Stored the way a real library stores it: fast-glob hands back forward slashes. + filePath: working.split(String.fromCharCode(92)).join("/"), relPath: working.slice(library.length + 1), filename: working.split(/[\\/]/).pop(), extension: 'mp4', - game, + game: folder, sizeBytes: before.bytes, durationSec: before.seconds, fileModifiedAt: before.mtime, @@ -309,7 +316,7 @@ ok('and the picture has colour in it', middle.saturation > 3, middle.saturation. /* ---------------------------------------- a clip that is already small */ console.log('\na clip already smaller than the preset aims for'); -const small = join(library, game, 'already-small.mp4'); +const small = join(library, folder, 'already-small.mp4'); await execFileAsync(ffmpeg, [ '-v', 'error', '-y', '-f', 'lavfi', '-i', 'testsrc=size=640x360:rate=30:duration=4', @@ -324,7 +331,7 @@ const smallRow = await repo.save( relPath: small.slice(library.length + 1), filename: 'already-small.mp4', extension: 'mp4', - game, + game: folder, sizeBytes: statSync(small).size, durationSec: 4, fileModifiedAt: statSync(small).mtime, diff --git a/src/main/actions/CompressClipAction.ts b/src/main/actions/CompressClipAction.ts index f75bbc64..6027f9f8 100644 --- a/src/main/actions/CompressClipAction.ts +++ b/src/main/actions/CompressClipAction.ts @@ -1,6 +1,6 @@ +import { MoveFileToTrashAction } from './MoveFileToTrashAction.js'; import path from 'node:path'; import fsPromises from 'node:fs/promises'; -import { shell } from 'electron'; import { BaseAction } from './BaseAction.js'; import { AppDataSource } from '../data-source.js'; import { Clip } from '../entity/Clip.js'; @@ -160,8 +160,11 @@ export class CompressClipAction extends BaseAction { } } + await this.retireDuplicateRows(); + const allClips = await clipRepo.find(); const missing = allClips.filter((clip) => !nowOnDisk.has(samePath(clip.filePath))); @@ -253,6 +256,45 @@ export class ScanAndSyncClipsAction extends BaseAction { return { added, updated, removed, total, hidden: Number(hiddenRow?.n ?? 0), pruneSkipped }; } -} + /** + * Remove a second row for a file that already has one, when it holds nothing + * of its own. See `services/duplicateRows.ts` for the rule; this is the + * reading and the deleting. Rows only: the file is never touched. + */ + private async retireDuplicateRows(): Promise { + const rows: Array> = await AppDataSource.query(` + SELECT c.id, c.filePath, c.displayName, c.notes, c.starred, c.published, c.openCount, + (SELECT COUNT(*) FROM clip_tags_tag t WHERE t.clipId = c.id) AS tags, + (SELECT COUNT(*) FROM good_bit g WHERE g.clipId = c.id) AS marks, + (SELECT COUNT(*) FROM collection_clips_clip k WHERE k.clipId = c.id) AS collections + FROM clip c + WHERE REPLACE(LOWER(c.filePath), char(92), '/') IN ( + SELECT REPLACE(LOWER(filePath), char(92), '/') FROM clip + GROUP BY REPLACE(LOWER(filePath), char(92), '/') HAVING COUNT(*) > 1 + )`); + if (!rows.length) return; + const plan = planDuplicateRows( + rows.map((row) => ({ + id: Number(row.id), + filePath: String(row.filePath), + displayName: (row.displayName as string | null) ?? null, + notes: (row.notes as string | null) ?? null, + starred: Boolean(row.starred), + published: Boolean(row.published), + openCount: Number(row.openCount ?? 0), + tags: Number(row.tags), + marks: Number(row.marks), + collections: Number(row.collections), + })), + ); + for (const group of plan.kept) { + console.warn(`[scan] ${group.path} has ${group.ids.length} rows that each hold something; left as they are`); + } + if (plan.remove.length) { + await AppDataSource.getRepository(Clip).delete(plan.remove); + console.log(`[scan] removed ${plan.remove.length} duplicate row(s) for files that already had one: ${plan.remove.join(', ')}`); + } + } +} diff --git a/src/main/routes/clips.ts b/src/main/routes/clips.ts index 028cbb67..7c559902 100644 --- a/src/main/routes/clips.ts +++ b/src/main/routes/clips.ts @@ -800,8 +800,20 @@ clipsRouter.post('/:id/compress', asyncHandler(async (req, res) => { // The row is the answer worth waiting for: the tile has to redraw with // the new size, and a refusal has to redraw with the old one. const after = await AppDataSource.getRepository(Clip).findOneBy({ id: clipId }); + // A refusal is an answer, not a failure, and the job says which in words + // the toast can show: before this it was a log line, so a clip that was + // left alone looked exactly like one that had been compressed. if (!result.replaced) { console.log(`[compress] ${clip.filename}: ${result.reason}, left alone`); + setJobProgress( + jobId, + 100, + result.reason === 'not-smaller' + ? 'Already about as small as a compression would make it, so it was left alone.' + : 'The compressed copy came out the wrong length, so the original was kept.', + ); + } else { + setJobProgress(jobId, 100, 'Compressed'); } completeJob(jobId, after ? ClipDTO.fromEntity(after) : undefined); }) diff --git a/src/main/services/duplicateRows.ts b/src/main/services/duplicateRows.ts new file mode 100644 index 00000000..f7bb33bc --- /dev/null +++ b/src/main/services/duplicateRows.ts @@ -0,0 +1,86 @@ +/** + * Two rows for one file, and which of them may go. + * + * Before the scan learned that `D:/Clips/x.mp4` and `D:\Clips\x.mp4` are the + * same file, a clip indexed by one path spelling and found again by the other + * got a second row. The scan stops making new ones, but it never removed an + * old one: the file exists, so neither row looks orphaned, and the library + * showed the clip twice, once with its publish state and once without. + * + * **Only a row that carries nothing is removed.** A clip row is the only copy + * of its tags, notes, name, stars, marks and collections, so a duplicate is + * retired only when everything it holds is already on the row that stays. + * Anything else is left, twice, and logged: a clip shown twice is an + * annoyance, and a clip whose notes were thrown away is a loss. + * + * The row only. The file is the same file either way and is never touched. + */ +export interface DuplicateCandidate { + id: number; + filePath: string; + displayName: string | null; + notes: string | null; + starred: boolean; + published: boolean; + openCount: number; + tags: number; + marks: number; + collections: number; +} + +export interface DuplicatePlan { + /** Rows safe to delete, because their twin holds everything they do. */ + remove: number[]; + /** Groups left alone because more than one row carries something, for the log. */ + kept: Array<{ path: string; ids: number[] }>; +} + +const BACKSLASH = String.fromCharCode(92); +const samePath = (value: string): string => value.split(BACKSLASH).join('/').toLowerCase(); + +/** How much a row holds that nothing else does. Higher stays. */ +function weight(row: DuplicateCandidate): number { + return ( + row.tags * 1000 + + row.marks * 1000 + + row.collections * 1000 + + (row.notes?.trim() ? 1000 : 0) + + (row.starred ? 100 : 0) + + (row.published ? 100 : 0) + + row.openCount + ); +} + +/** Whether `extra` holds anything `keeper` does not. */ +function carriesMore(extra: DuplicateCandidate, keeper: DuplicateCandidate): boolean { + if (extra.tags > 0 || extra.marks > 0 || extra.collections > 0) return true; + if (extra.notes?.trim() && extra.notes !== keeper.notes) return true; + if (extra.starred && !keeper.starred) return true; + if (extra.published && !keeper.published) return true; + const name = extra.displayName?.trim(); + if (name && name !== keeper.displayName?.trim()) return true; + return false; +} + +export function planDuplicateRows(rows: readonly DuplicateCandidate[]): DuplicatePlan { + const groups = new Map(); + for (const row of rows) { + const key = samePath(row.filePath); + const group = groups.get(key); + if (group) group.push(row); + else groups.set(key, [row]); + } + + const plan: DuplicatePlan = { remove: [], kept: [] }; + for (const [path, group] of groups) { + if (group.length < 2) continue; + // The richest row stays; the oldest breaks a tie, since it is the one + // anything outside the database is most likely to know about. + const ordered = [...group].sort((a, b) => weight(b) - weight(a) || a.id - b.id); + const [keeper, ...rest] = ordered; + const removable = rest.filter((row) => !carriesMore(row, keeper)); + plan.remove.push(...removable.map((row) => row.id)); + if (removable.length < rest.length) plan.kept.push({ path, ids: ordered.map((row) => row.id) }); + } + return plan; +} diff --git a/src/renderer/src/App.vue b/src/renderer/src/App.vue index e80abde9..44b39a37 100644 --- a/src/renderer/src/App.vue +++ b/src/renderer/src/App.vue @@ -43,6 +43,7 @@ onUnmounted(() => { :confirm-label="pending?.confirmLabel" :tone="pending?.tone" :icon="pending?.icon" + :emphasis="pending?.emphasis" @confirm="accept" @cancel="cancel" /> diff --git a/src/renderer/src/components/Base/BaseConfirmDialog.vue b/src/renderer/src/components/Base/BaseConfirmDialog.vue index 47338d64..20c6e7e1 100644 --- a/src/renderer/src/components/Base/BaseConfirmDialog.vue +++ b/src/renderer/src/components/Base/BaseConfirmDialog.vue @@ -1,5 +1,5 @@