chore(quality): batch dedup pass across server + client#266
Conversation
There was a problem hiding this comment.
Pull request overview
Batch quality/dedup pass closing ~20 PLAN.md items. Pure refactors with no user-visible behavior changes: extracts shared helpers (runFfmpegProcess, sanitizeListWith, listDirectoryByExtension, makePathsProxy test helper), introduces a new bulk endpoint for media-collection items, collapses an N+1 in deleteSeason via bulkReassignSeason, memoizes resolveGlobalDisplayName with event-driven invalidation, factors <ModelSelect> / CollectionPickerShell / useLocalStorageBool / getCardProps on the client, teaches apiCore.request() about FormData, and consolidates the agent-prompt builder's task/review-loop/completion-bullet sections.
Changes:
- New server helpers + endpoint (
/items/bulk,bulkReassignSeason) and migration 019 to normalize characterdescription→physicalDescription. - Client component/hook extractions and
apiCoreFormData support. - Build hygiene:
run-migrations.jsskips*.test.js.
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/lib/ffmpeg.js + .test.js | Extract runFfmpegProcess with stderr-tail + abort cleanup; tests for branches. |
| server/lib/fileUtils.js + .test.js | Add listDirectoryByExtension helper + tests. |
| server/lib/storyBible.js + .test.js | Add sanitizeListWith; clarify sanitizeCharacter migration semantics. |
| server/lib/mockPathsDataRoot.js + .test.js | New shared vi.mock PATHS proxy helper. |
| server/lib/validation.js | Add mediaCollectionBulkItemsSchema. |
| server/services/mediaCollections.js + .test.js | New bulkUpdateCollectionItems + tests. |
| server/services/mediaAnnotations.js | Hoist identity lookup out of legacy-entry loop. |
| server/services/loras.js, imageGen/local.js, pipeline/musicLibrary.js | Adopt listDirectoryByExtension. |
| server/services/pipeline/audioMux.{js,test.js} | Delegate to runFfmpegProcess. |
| server/services/pipeline/issues.{js,test.js} | Add bulkReassignSeason + tests. |
| server/services/pipeline/seasons.js | Use bulkReassignSeason in deleteSeason. |
| server/services/pipeline/textStages.test.js | Use physicalDescription field. |
| server/services/sharing/annotationIdentity.js | Cache + invalidation via settings:updated. |
| server/services/settings.js | Add settingsEvents emitter. |
| server/services/sharing/.test.js, writersRoom/.test.js | Migrate to makePathsProxy. |
| server/services/agentPromptBuilder.{js,test.js} | Extract buildTaskBlock, buildReviewLoopFollowUpSection, buildCompletionGuidelineBullet; cover worktree branches. |
| server/routes/mediaCollections.{js,test.js} | Wire POST /items/bulk route. |
| scripts/run-migrations.js | Skip *.test.js files. |
| scripts/migrations/019-character-description-to-physical.js | New migration; log message still says "017" (see comment). |
| client/src/services/apiCore.js | Detect FormData; skip JSON content-type. |
| client/src/services/apiHealth.js, apiPipeline.js | Route uploads through request(). |
| client/src/hooks/useLocalStorageBool.js | New bool + JSON-persisted hooks. |
| client/src/hooks/useMediaAnnotations.js | Add getCardProps helper. |
| client/src/components/ModelSelect.jsx | New shared active+Legacy <select>. |
| client/src/components/media/CollectionPickerShell.jsx | Shared popover for add/bulk pickers. |
| client/src/components/media/{AddToCollectionMenu,BulkTargetPicker}.jsx | Adopt shared shell; collection-list passthrough. |
| client/src/components/meatspace/tabs/SettingsTab.jsx, pipeline/stages/AudioStage.jsx | Pass { silent: true } upload option. |
| client/src/pages/{VideoGen,CreativeDirector,ImageGen,MediaHistory,MediaCollectionDetail,UniverseBuilder,WritersRoom,PipelineSeries,Instances,ChiefOfStaff}.jsx | Adopt shared components/hooks; dedup local-storage state and card-props lookups; fix mergeVariations malformed-label drop. |
| PLAN.md | Tick off completed items. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes 20 items from PLAN.md "Code quality / dedup": Server helpers - `runFfmpegProcess` extracted in `server/lib/ffmpeg.js`, adopted by audioMux, upscale, faststart, generateThumbnail, extractEvaluationFrames - `sanitizeListWith` in `storyBible.js` (3 sites consolidated) - `listDirectoryByExtension` in `fileUtils.js` (3 sites) - `mockPathsDataRoot` test helper (8 test files migrated) Server services - `bulkUpdateCollectionItems` + `POST /api/media/collections/:id/items/bulk` with shared `validateItemInput` - `bulkReassignSeason` collapses N+1 writes in `deleteSeason` - `mediaAnnotations.readAll` hoists identity lookup, uses Promise.all - `annotationIdentity` lazy-subscribes to `settings:updated`, skips cache on settings-read failure (30s TTL invalidates on save) - Drop legacy `description` fallback in `sanitizeCharacter` + migration 017 - Universe sanitizer parity (already done in #255) agentPromptBuilder - Extract `buildTaskBlock`, `buildReviewLoopFollowUpSection`, `buildCompletionGuidelineBullet` - Tests for missing `worktreeCommitGuidance` branches Client - `<ModelSelect>` component (active+Legacy optgroup pattern, getLabel prop) - `CollectionPickerShell` shared by AddToCollectionMenu + BulkTargetPicker - `useLocalStorageBool` / `useLocalStoragePersisted` hooks (6 sites) - `apiCore.request()` honors FormData (skips JSON content-type) - `useMediaAnnotations.getCardProps(key)` (6 sites across 4 gallery pages) - `mergeVariations` drops malformed rows from both sides 5127 server tests pass, client build green.
- scripts/run-migrations.js: skip *.test.js files so the runner doesn't try to import a vitest module as a migration (Phase A added 018-categorize-universe-buckets.test.js) - server/lib/storyBible.js: restore read-side description fallback so a load-before-migration doesn't silently drop legacy text on next save - Rename migration 017-character-description-to-physical.js -> 019-... to avoid collision with upstream 017-volume-cover-concepts-stage.js - ffmpeg.js: strip spawn-failed prefix before re-prefixing in log - annotationIdentity.js + mockPathsDataRoot.js: doc-vs-code drift
0f5ee36 to
5bbffbd
Compare
Summary
Closes ~20 items from PLAN.md's Code quality / dedup section in a single batch. All pure refactors — no user-visible behavior changes.
Server helpers (new + dedup)
runFfmpegProcess({ bin, args, signal, stderrTailBytes })extracted inserver/lib/ffmpeg.js; adopted byaudioMux,upscaleVideo2x,optimizeForStreaming,generateThumbnail,extractEvaluationFrames. Includes AbortSignal listener cleanup.sanitizeListWith(raw, sanitizer, cap)inserver/lib/storyBible.js(3 sites)listDirectoryByExtension(dir, { extensions, mapEntry })inserver/lib/fileUtils.js(3 sites)mockPathsDataRoot()test helper inserver/lib/(8 test files migrated)Server services
POST /api/media/collections/:id/items/bulk— single read-modify-write{ add, remove }endpoint, replaces N round-trips for "Move N items" / bulk-select flowsbulkReassignSeasoncollapses N+1 writes indeleteSeasonmediaAnnotations.readAllhoists identity lookup out of the legacy-entry loop, fetches viaPromise.allannotationIdentitylazy-subscribes tosettings:updated; skips cache on settings-read failure (was caching the OS-username fallback for 30s on transient errors)sanitizeCharacterkeeps the defensivedescriptionread-side fallback; migration 019 normalizes the alias on diskagentPromptBuilder
buildTaskBlock,buildReviewLoopFollowUpSection,buildCompletionGuidelineBulletso the dual full/light prompt paths share their task block + review-loop boilerplateworktreeCommitGuidancebranches (isWorktreeOnExistingBranch=true,hasSlashdo && !willOpenPR)Client
<ModelSelect>component (active+Legacy<optgroup>pattern,getLabelfunction prop) shared by VideoGen + CreativeDirectorCollectionPickerShellshared byAddToCollectionMenu+BulkTargetPicker;MediaCollectionDetailreuses its already-fetched collections listuseLocalStorageBool/useLocalStoragePersistedhooks (6 sites)apiCore.request()honorsFormData— drops the JSON content-type so the browser sets the multipart boundary;apiHealth.uploadAppleHealthXml+apiPipeline.uploadPipelineMusicTrackmigrated onto ituseMediaAnnotations.getCardProps(key)consolidates{ starred, hasNote, onToggleStar }lookups across 6 sitesmergeVariations(UniverseBuilder) drops malformed-label rows from both sides so a fresh row with a missing label can't silently duplicateMigration & build hygiene (from review)
scripts/run-migrations.jsskips*.test.jsso the runner doesn't try to import vitest modules as migrations017-character-description-to-physical.js→019-…to avoid collision with the upstream017-volume-cover-concepts-stage.jsintroduced via rebaseTest plan
cd server && npm test— 5144 passed, 5 skipped (236 files)cd client && npm run build— green/simplifyran (reuse/quality/efficiency); findings fixed inline before PR🤖 Generated with Claude Code