fix(projects): relink moved media from registry - #235
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughProject loading now relinks stale screen and webcam media paths through the media-links registry. The lookup validates unique file matches. The relinker preserves unresolved data without mutating the original project. ChangesProject media relinking
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Line 3322: Update electron/ipc/handlers.ts at lines 3322 and 3396 in the
loadProjectFile and loadCurrentProjectFile flows: catch
getApprovedProjectSession failures after relinking, then return the relinked
project with a null session instead of aborting. Match the existing
explicit-path flow’s approval-failure handling so the renderer can provide
missing-media recovery.
In `@electron/media/mediaLinksRegistry.test.ts`:
- Around line 88-126: Add a test alongside the existing
findRelocatedMediaByStoredPath cases that registers a screen recording, changes
its file size afterward, and then looks it up using the original fingerprint
size. Assert that the lookup resolves to null, covering the changed-file
rejection branch while preserving the existing matching-path behavior.
In `@electron/media/mediaLinksRegistry.ts`:
- Around line 234-261: Update findRelocatedMediaByStoredPath to use
locale-independent toLowerCase() when normalizing both the stalePath basename
and each entry.lastKnownPath basename; keep the existing basename matching logic
unchanged.
In `@electron/media/projectMediaRelinker.test.ts`:
- Around line 19-52: Add a test alongside the existing relinkProjectMedia test
for an asset with a missing or ambiguous media-registry match. Assert that both
originalPath and cameraTrack.sourcePath remain unchanged in the relinked result
and that the original project is not mutated, while preserving the existing
successful-match coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88186dd4-a09a-46b8-9282-63d58df62e84
📒 Files selected for processing (5)
electron/ipc/handlers.tselectron/media/mediaLinksRegistry.test.tselectron/media/mediaLinksRegistry.tselectron/media/projectMediaRelinker.test.tselectron/media/projectMediaRelinker.ts
89e07a8 to
545043d
Compare
3867d01 to
9831b12
Compare
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
This is #212, and you read it properly: the registry already holds the answer and nothing consults it on load. The strictness in findRelocatedMediaByStoredPath is the right instinct — re-statting the candidate against its recorded fingerprint size, and returning null on matches.length !== 1 rather than taking the first hit, is the shape this needs. Cloning instead of mutating the parsed JSON is right too, and aligning loadProjectFile with the tolerant getApprovedProjectSession try/catch that loadProjectFileFromPath already had is a good drive-by.
Two things block it, and both are about where this runs and what it trusts.
The hook point only covers .openscreen import. Every real project open — including auto-load-last-project on launch — goes through DocumentService.getProject, which is untouched. So a project that is already broken stays broken forever, and media that moves after import is never relinked.
And the size check that makes the match safe is skipped whenever sizeBytes is absent, which is every legacy-migrated document. What is left is a case-insensitive basename match that can repoint a project at unrelated footage, attach that footage's webcam, log nothing, and then get persisted by the renderer. That is a worse failure than the one it fixes.
Details inline. The diagnosis is right; the placement and the fallback need rethinking.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Threads cleared. The relink now runs where every project open passes, and it refuses rather than guessing when the document recorded no size — which is the case that mattered, since silently repointing a project at unrelated footage is worse than leaving it visibly broken.
Approving. Noting for the record that this does not close #212 on its own: registerMediaLinks early-returns without a webcam or cursor sidecar, so a screen-only recording never enters the registry and the issue's literal repro on a fresh machine still fails. That needs the "locate this media" prompt or fingerprints persisted into the document.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Re-approving after the rebase — the previous approval was dismissed by the push. The conflict was imports only: relinkProjectMedia against main widening the Linux capture import to carry LinuxCaptureSourceKind. Both kept, both used.
The relink only ran in the `.openscreen` import handlers, which is the one moment a project is least likely to need it. Every real open — the project picker, the agent, the auto-load-last-project effect on launch — goes through DocumentService.getProject and never touched the stored paths, so a document already broken by issue getopenscreen#212 was re-read as broken forever, and media moved after the import was never noticed at all. The hook moves to that choke point, applied to the upgraded JSON so `documentSchema.parse` still validates what we hand back. The two import handlers keep their own call: they hand the parsed project straight to the renderer and use it to approve the recording session, before anything has been through DocumentService at all. loadCurrentProjectFile is dropped from the patch rather than moved. It is plumbed all the way out through preload, the native bridge and projectService, and nothing under src/ ever calls it, so patching it only made the diff look like it covered more ground than it did. An asset with no recorded `sizeBytes` is now refused instead of falling back to a case-insensitive basename compare. That fallback was not a weaker match, it was no match: `recording.mp4` is the least distinctive name a screen recorder can produce, and hitting it repointed the project at unrelated footage AND attached that footage's webcam, silently, with the renderer persisting the result on the next save. `sizeBytes` is optional in the schema and only DocumentService.addAsset ever writes it — migrateProjectDataToAxcutDocument does not — so every document migrated from v1.7 took that path. Media the user can see is missing is recoverable; media that is quietly someone else's is not. The size is now a required argument of findRelocatedMediaByStoredPath, so a caller with nothing to match on cannot ask for a guess. Every rewrite is logged, and so is the refusal: this changes paths the renderer then writes back to disk, which should not happen without a trace. Two stats, not a fingerprint, now decide whether there is anything to repair at all. With this running on every open (and on addAsset/removeAsset through getProject), the previous unconditional registry lookup would open and read 128KB of every asset in the project each time just to confirm that the paths already resolve. Known gap, unchanged by this: registerMediaLinks early-returns unless a webcam or cursor sidecar is known, so a screen-only recording never enters the registry and cannot be relinked by this mechanism at all. Issue getopenscreen#212's literal repro still needs either the "locate this media" prompt it asks for, or fingerprints persisted into the document.
8c1025c to
27df811
Compare
3 participants
Summary
Related issue
Fixes #212
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; this changes project loading behavior.
Testing
Summary by CodeRabbit
New Features
Bug Fixes