Twitch-VOD-Manager/docs/IMPROVEMENT_LOG.md

# Improvement Log

Dated entries from improvement cycles. Newest at top.

## 2026-05-03 — Cycle 3: clip hardening + VOD filter + cancel-cross-talk fix

Three independent improvements landed this cycle.

### 1. `download-clip` IPC: integrity, cancellation, sanitization (server defensive)

- **File**: `src/main.ts` — `download-clip` IPC handler, new `activeClipProcesses` map.
- **Problem**: The handler reported `success: true` on streamlink exit code 0 even when the resulting file was empty / a few hundred bytes (Twitch occasionally returns a manifest with no segments). The path passed `clipInfo.broadcaster_name` straight to `path.join` — Twitch returns the broadcaster's *display* name, which can carry unicode, spaces, or punctuation that produced surprising directory layouts on Windows. The spawned streamlink process was tracked nowhere, so `window-all-closed` left it orphaned.
- **Fix**: `safeBroadcaster` runs through `sanitizeFilenamePart`. `safeTitle` falls back to `clip` when the title sanitises to empty. The output filename now goes through `ensureUniqueFilename(path, clipId)` so retrying a clip with the same title doesn't overwrite the previous download. After streamlink exits, the file is rejected if smaller than 16 KiB or if `validateDownloadedFileIntegrity` fails (no video stream / unreadable). The proc is tracked in a new `activeClipProcesses` map and killed by `window-all-closed`.

### 2. VOD list filter / search (client feature: VM/state + UI + persistence + keyboard)

- **Files**: `src/renderer-streamers.ts`, `src/renderer.ts`, `src/renderer-texts.ts`, `src/index.html`, `src/renderer-locale-de.ts`, `src/renderer-locale-en.ts`.
- **Problem**: A streamer can have hundreds of VODs (the test fixture alone has 37 cards). There was no way to find a specific VOD by title — only scroll. With a long archive this is genuinely painful.
- **Fix**: Filter row above the VOD grid (`vodFilterInput`, clear button, match counter). State (`vodFilterQuery`) is persisted to `localStorage` via `loadPersistedVodFilter` / `persistVodFilter`, so the search bar survives an app restart. The render path was split: `renderVODs` now stores `lastLoadedVods` + `lastLoadedStreamer` and delegates to `renderVodGridFromCurrentState`, which applies `filterVodsByQuery` on every input event without re-fetching. Empty-state DOM is built via `setVodGridEmptyState` using `createElement` + `textContent` (no `innerHTML` for locale strings — defense-in-depth even though the strings are trusted). Keyboard: `Ctrl+F` / `Cmd+F` focuses the filter (only when the VODs tab is active and Electron's no-op default is suppressed); `Esc` clears the filter when the input has focus and content; `Esc` still closes modals first if any are open.

### 3. Decouple `currentProcess` from queue downloads (server cleanup + race fix)

- **File**: `src/main.ts` — global rename and assignment removal.
- **Problem**: A single `currentProcess: ChildProcess | null` was shared by `cutVideo`, `mergeVideos`, `splitMergedFile`, AND `downloadVODPart`. With parallel downloads the global was constantly overwritten between siblings, but the cross-talk that mattered was different: if a queue download was running and the user kicked off a video cut, the cutter ffmpeg ran into the same global. Pressing the queue's *cancel-download* button then iterated `activeDownloads` (correct) AND called `currentProcess.kill()` (incorrect — that was the cutter ffmpeg by then), killing the unrelated cut.
- **Fix**: `currentProcess` renamed to `currentEditorProcess` and confined to the editor pipeline (cutter / merger / splitter). `downloadVODPart` no longer assigns to it — `activeDownloads` is the sole source of truth for queue children. The fallback `if (currentProcess) currentProcess.kill()` was removed from `remove-from-queue`, `pause-download`, and `cancel-download`. `window-all-closed` still kills it (so a cutter ffmpeg gets cleaned up on app exit) and now also kills `activeClipProcesses` introduced by Pick 1.

### Regression

- `npm run build` — clean (TypeScript strict, 0 errors).
- `npm run test:e2e:update-logic` — passed.
- `npm run test:e2e` — passed (`issues: []`).
- `npm run test:e2e:guide` — passed (`failures: []`).
- `npm run test:merge-split` — passed.
- `npm run test:e2e:full` — passed (`failures: []`, `runtimeIssues: []`; flows: language switch, queue add, duplicate prevention, runtime metrics, clip queue, pause/resume, retry, reorder, media cut/merge, update check).

## 2026-05-03 — Cycle 2: release pipeline + defensive parsing

Three independent improvements landed this cycle.

### 1. `scripts/release_gitea.mjs` skips rebuild when artifacts exist (release pipeline)

- **File**: `scripts/release_gitea.mjs`.
- **Problem**: The script unconditionally ran `npm run dist:win` (full test suite + electron-builder) even when the version's artifacts were already on disk under `release/`. When `npm run test:e2e` was broken (cycle 1 follow-up), the release path was unusable — the previous cycle had to bypass the script with direct API uploads via PowerShell. Every future agent would hit the same wall.
- **Fix**: New `--skip-build` flag. The script now also auto-detects whether all 3 required artifacts (`Setup-<v>.exe`, `Setup-<v>.exe.blockmap`, `latest.yml`) exist for the requested version and skips `dist:win` accordingly. The auto-skip is the safe default — explicit `--skip-build` documents intent. Help text updated to describe the new flag and the auto-skip behaviour.

### 2. `playwright` in `devDependencies` + simplified test scripts (release pipeline)

- **Files**: `package.json` (+ `package-lock.json`).
- **Problem**: `npm exec --yes --package=playwright -- node scripts/smoke-test*.js` failed with `MODULE_NOT_FOUND` in environments where `npm exec` couldn't resolve playwright on the fly (clean caches, locked CI runners). Cycle 1 worked around it with `npm install --no-save playwright`. Result: the documented test path was unreliable.
- **Fix**: `playwright ^1.59.1` added to `devDependencies`. `test:e2e`, `test:e2e:guide`, `test:e2e:full` now invoke `node scripts/smoke-test*.js` directly — `require('playwright')` resolves locally. No browser binary install needed because the smoke tests drive Electron via `_electron`, not a browser.

### 3. Defensive parsing in `loadConfig` and `loadQueue` (server-side correctness)

- **File**: `src/main.ts` — new `isPlainObject` / `isValidQueueStatus` / `sanitizeCustomClip` / `sanitizeMergeGroup` / `sanitizeQueueItem` helpers; rewritten `loadConfig` and `loadQueue`.
- **Problem**: `loadConfig` blindly spread `JSON.parse(data)` over the defaults. If the config file ever held a non-object (corrupt, manually edited to an array, partial write before Cycle 1's fsync landed), the spread either dropped values silently (primitives) or polluted the config object (arrays became numeric keys). `loadQueue` only validated `id`, `url`, `status` are strings — it accepted `customClip` / `mergeGroup` of any shape, never validated `progress` was a finite number, and notably never normalized stale `status: 'downloading'` items. After a hard kill mid-download, those items came back marked as still downloading with no actual download running, and `start-download` only resurrected `paused` items, leaving them stuck.
- **Fix**: `loadConfig` checks `isPlainObject(parsed)` before spread; non-objects are logged and ignored, defaults used. `loadQueue` runs every entry through `sanitizeQueueItem` which validates the `status` enum, normalizes `progress` to `[0, 100]`, validates and normalizes `customClip` / `mergeGroup` shapes, and demotes stale `status: 'downloading'` to `pending` with `progress = 0` so the user can actually resume the queue. Invalid items are dropped with a count logged. As a bonus, the previously-unused `CustomClip` and `MergeGroupItem` type imports now have call sites.

### Regression

- `npm run build` — clean (TypeScript strict, 0 errors).
- `npm run test:e2e:update-logic` — passed.
- `npm run test:e2e` — passed via the new direct script path (no `npm exec` workaround), `issues: []`.
- `npm run test:e2e:guide` — passed.
- `npm run test:merge-split` — passed.
- `npm run test:e2e:full` — passed (`failures: []`, `runtimeIssues: []`; flows: language switch, queue, duplicate prevention, runtime metrics, clip queue, pause/resume, retry, reorder, media cut/merge, update check).

## 2026-05-03 — Cycle 1: stability & UX polish

Three independent improvements landed this cycle.

### 1. Atomic file writes survive power loss / crash mid-write (correctness)

- **Files**: `src/main.ts` — new `writeFileAtomicSync` helper, `saveConfig`, `writeQueueToDisk`.
- **Problem**: `saveConfig` and `writeQueueToDisk` used `writeFileSync` + `renameSync`. Node's `writeFileSync` does NOT call `fsync` — the OS may report the rename complete while the file content still sits in the write cache. A power loss / kernel panic between `writeFileSync` and `renameSync` could leave the renamed file empty or truncated. On next launch, `JSON.parse` throws and the app silently falls back to defaults (config) or `[]` (queue). Users would see "settings reset" / "queue lost" with no diagnostic in the debug log beyond a `console.error`.
- **Fix**: `openSync(tmp, 'w')` → `writeSync(fd, buffer, 0, len, 0)` → `fsyncSync(fd)` → `closeSync(fd)` → `renameSync`. The `fsyncSync` is wrapped in an inner try (some filesystems reject it, e.g. network shares); failure there is non-fatal but the close + rename order is always preserved. The Windows copy/unlink fallback for "rename failed because target locked" is kept.

### 2. Per-item filename claims fix parallel-download race (race condition + dead-code cleanup)

- **Files**: `src/main.ts` — `ensureUniqueFilename`, new `releaseClaimedFilenamesForItem`, every download call site, `splitMergedFile` signature.
- **Problem**: `claimedFilenames` was a global `Set<string>` and `processOneQueueItem` did `claimedFilenames.clear()` in its `finally`. With parallel downloads enabled (max 2), when item A finished, the `clear()` wiped item B's reservations too. In the narrow window between B claiming a filename via `ensureUniqueFilename` and streamlink actually writing the first bytes to disk, a third item entering the freed slot could compute the SAME filename (claim set empty, file not yet on disk) → both downloads would race writing the same path. The dead `releaseClaimedFilename(filePath)` function was defined at line 722 but never called from anywhere.
- **Fix**: New `Map<itemId, Set<filename>>` tracks which item claimed which filenames. `ensureUniqueFilename(filePath, itemId)` registers per-item; `releaseClaimedFilenamesForItem(itemId)` removes only that item's claims. `splitMergedFile` gained an `itemId` parameter so split-phase claims register correctly. The dead `releaseClaimedFilename` is gone, replaced by the per-item variant.

### 3. Renderer UX polish — robust progress lookup, persisted active tab, keyboard shortcuts (client-side feature)

- **Files**: `src/renderer-queue.ts`, `src/renderer.ts`.
- **Problem(s)** (small wins bundled as one coherent UX improvement):
  - `updateQueueItemProgress` indexed `byId('queueList').children[idx]` by array position — fragile if the queue array and DOM ever diverged for a frame (queue mutated after render-fingerprint shortcut, or during the throttled queue-sync window).
  - The active tab always reset to `vods` on app launch — annoying for users who live in `settings`, `cutter`, or `merge`.
  - No way to dismiss any of the three modals (`clipModal`, `templateGuideModal`, `updateModal`) without clicking the close button.
  - No keyboard navigation between tabs (only `Del` and `S` were wired).
  - The page title used to show the streamer name even when the user was on Settings or Cutter, because `showTab` always preferred `currentStreamer` over the tab title.
- **Fix**:
  - Look up queue items by `[data-id="..."]` selector instead of array index. Resilient to mutation between renders. Determinate / indeterminate progress class logic tightened (`isDeterminate = progress > 0 && progress <= 100`).
  - Active tab persisted to `localStorage` on every `showTab`; restored on init via `loadPersistedActiveTab`, whitelisted to known tab IDs (`vods | clips | cutter | merge | settings`) so a future rename can't strand users on a missing tab. Title logic fixed: streamer name only appears in the page title when the VODs tab is active.
  - `Escape` closes the topmost open modal regardless of focus (priority order: clip dialog → template guide → update modal). Works while typing in a modal input.
  - `Ctrl+1..5` (or `Cmd+1..5` on macOS) jumps directly to a tab. Existing `Del` (delete selected) and `S` (start/pause) shortcuts continue to work and remain blocked while typing in inputs.

### Regression

- `npm run build` — clean (TypeScript strict, 0 errors, 0 new warnings).
- `node scripts/smoke-test-update-version-logic.js` — passed.
- `node scripts/smoke-test-merge-split-logic.js` — passed.
- `node scripts/smoke-test.js` — passed (37 VODs listed, queue add OK, preflight green, `issues: []`).
- `node scripts/smoke-test-template-guide.js` — passed (17 variable rows, live preview reactive, `failures: []`).
- `node scripts/smoke-test-full.js` — passed (`failures: []`, `runtimeIssues: []`; flows verified: language switch, queue add, duplicate prevention, runtime metrics, clip queue, pause/resume, retry, reorder, media cut/merge, update check).

ESLint reports 36 pre-existing warnings and 1 pre-existing error (control-character regex in `sanitizeFilenamePart`); none new from this cycle.