Repository navigation
fix(studio-server): undoing a change takes the media ledger back with the film - #5016
Conversation
Edit accuracy: accurate 1556 (base branch 1556), smooth 1352 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencecho
left a comment
There was a problem hiding this comment.
Requesting changes on e732a057. The fix does what it says for a new project, but for the people who hit this regression it makes the first Undo after upgrading delete the media ledger instead of undoing their edit. I reproduced it; the fix is small.
Blocking: first Undo after upgrade deletes .media/manifest.jsonl
- A project whose history log was opened by 0.8.123 (the #4966 code) has a baseline with no ledger (
withoutHiddenPathsstripped it on read). Those are exactly the projects that show the cutout-undo regression, so they have a ledger on disk. - With this PR the ledger is a kept path again. On the next sweep it is a file on disk that the log does not know, so it is filed as an
outsideentry "Changed outside the app" withbefore = null. next()treatsoutsideentries as everyone's, sostep("back")(Cmd+Z) picks that entry first.- Repro (head tarball): I opened a project that has
index.htmland.media/manifest.jsonlwith the head code minus the ledger line (that is #4966'sisHistoryPath), made one edit, closed it, and reopened with the real head. Entries after the firststep("back")were: person "edit"[index.html], outside "Changed outside the app"[.media/manifest.jsonl, before null], person "Undid: Changed outside the app"[.media/manifest.jsonl]. The ledger is gone from disk andindex.htmlis still the editedv2. A directundoof that entry also deletes the ledger. - Impact as I see it: one Cmd+Z that touches the wrong thing (the person's edit is not undone, the ledger vanishes), once per affected project. I did not check whether Redo brings the ledger back, and the media files under
.media/stay. - Suggested fix: when a log is read, adopt a kept hidden path that is on disk and absent from the log into the baseline and
trackedwithout recording a change (or reconcile it before the first sweep). A test that opens a project whose log lacks the ledger, with the ledger on disk, and assertslist()is empty and the firststep("back")undoes the person's edit would pin it.
What I verified (head tarball, deps built, NODE_ENV=test)
- The head is
e732a057, not draft, not stacked (1 commit, 2 files, same as the effective diff vsmain). No other review exists on it. src/history+projectSignature: 153 pass / 2 skipped, 3 runs;tsc,oxlintandoxfmtclean. The new test is the only new case, and it fails without the ledger line.- 5 mutants: 3 caught (ledger dropped, Studio manifests dropped from the kept set, kept-check removed). 2 survive: keeping all of
.media/(startsWith(".media/")) and keeping anymanifest.jsonlbasename. Nothing pins that other.media/*files stay out, which the PR text says is intended.
Non-blocking
.media/holds more than the ledger:preferences.jsonandrecipes/<name>/*(committed project files,prefs-store.mjs,recipe-store.mjs), the media files themselves (.media/images/…,audio/…), andindex.md. All were tracked before #4966 and are not now. After this PR Undo takes the ledger back but leaves the files andindex.mdit describes. The PR names this as out of scope; I'm recording it because I flagged the broad hidden-path rule on #4966 and approved it as non-blocking, and this ledger case is that rule's consequence.- I did not run Desktop's
personWriteEntrytest (it needs a release).
CI. 66 checks pass, 13 skipped, none failing or pending, including the edit-accuracy gate (1556, same as base). CI is a reference; the finding above is from the repro.
I'll re-review the new head promptly. This is a review verdict, not authorization to merge or deploy.
— Review by tai (pr-review)
… the first Undo undoes the edit
…e sweep would see it A ledger behind a link was read straight into the baseline, then filed as a deletion by the first sweep. The take-in now asks the same scan the sweep uses. The cutout test also pins that other .media files and other manifest.jsonl names stay out of history.
|
Thanks for the repro. Addressed at 9fdb1fa:
Mixed versions: an older version that rewrites a marked log drops the mark and the ledger with it, so that log is again one that never names the ledger and is treated like a 0.8.123 log. The PR body states that tradeoff. |
terencecho
left a comment
There was a problem hiding this comment.
Approving 9fdb1fa9. My blocker on e732a057 is resolved: a project whose history was written by 0.8.123 now takes the media ledger in on the first open, so the first Undo undoes the person's edit and the ledger stays. I am retracting the changes requested there.
It touches 3 files (historyLog.ts, projectHistory.ts, projectHistory.test.ts). Not draft, not stacked: 4 commits on main, same 3 files in main...9fdb1fa9. It is 10 behind main, and none of those commits touch studio-server/src/history; GitHub reports it mergeable.
What I verified (head tarball, deps built, NODE_ENV=test)
src/history+projectSignature: 147 pass / 2 skipped, 3 runs.tsc --noEmit,oxlintandoxfmt --checkclean.- Running the head tests against the previous head's source (
e732a057): the new upgrade test fails there, so it exercises the fix. - The upgrade path with real code, not an edited log. I wrote the history with
main's source (0.8.123's behaviour): a person's edit toindex.htmlthat also changed.media/manifest.jsonl. I then reopened it with the head source:list()shows only the person's edit, no "Changed outside the app" entry.- The first Cmd+Z undoes
index.html(back toA) and the ledger is still on disk. One732a057the same state deleted it. - The baseline record now carries
keepsLedger: true, a second open is stable, and Redo restores the edit.
- Other states, each checked on head:
- No ledger at upgrade, one made while the project is closed: filed as an
outsidechange. - A ledger behind a symlink (file link, and the test's directory link): not taken in, and nothing from outside the project is written into the history directory.
- A log from before 0.8.123 where an entry created the ledger, with the marker removed: no phantom entry, and undoing that entry deletes the ledger.
- No ledger at upgrade, one made while the project is closed: filed as an
- 13 mutants of the new code: 8 caught (marker never read, never written, take-in skipped, link guard removed, the gate removed, ledger not kept, take-in never stores, first open without the marker). 5 survive, below.
Non-blocking
- Survivors, take-in: (a) skipping
named(MEDIA_LEDGER)and (b) skippingbaseline.has(...): the pre-0.8.123 case above is right on head, but no test pins either guard. (c) The take-in not setting the marker and (d) not persisting it: when there was no ledger at upgrade, a ledger made later while closed is still filed as a change on head (I ran it), but a mutant that drops the marker or the persist passes the suite. A test that upgrades with no ledger and then adds one while closed would pin both. (e) Writing the marker unconditionally is equivalent, because the take-in always runs first. - "As it stands" is the intended limit. For a project whose cutout ran under 0.8.123, the ledger edit was never recorded, so the first Undo undoes the film and leaves the ledger naming the cutout media. That is the state before this PR; nothing can restore what was never recorded. The PR text says so.
- Other
.media/*files (preferences.json,recipes/*, the media files) andindex.mdare still untracked after #4966. The PR names this as out of scope; I'm recording it again because it was the non-blocking note on #4966 that produced this ledger case. - Not exercised: Desktop's
personWriteEntrytest (needs a release with the fix) and an older version rewriting a marked log, which the PR describes and I did not run.
CI. All checks at this head finished: 93 passed, 1 skipped, 0 failing, 0 pending, including the edit-accuracy gate (1556, same as base). CI is a reference; the verdict rests on the evidence above.
Reviewed on the PR head 9fdb1fa9; my earlier changes-requested review on e732a057 is the only other review. This is a review verdict, not authorization to merge or deploy beyond what the gate already does.
— Review by tai (pr-review)
What changed
Project history keeps the media ledger,
.media/manifest.jsonl, again. Since #4966 history skipped every hidden path except Studio's two manifests, so the ledger's record of a cutout or import was never part of the change. Undoing the change then took the film back but left the ledger naming media the film no longer uses.A project that already ran 0.8.123 has a log that never names the ledger. On the first open after upgrading, history takes the ledger into the log's baseline as it stands, the way a first open records any file, so it is not filed as a new change and the first Undo undoes the person's edit, not the ledger. The log's baseline record then carries a
keepsLedgermark, so later opens treat a ledger that appears as any other change. The take-in only accepts a ledger the sweep itself would list, so a ledger behind a link is never read into history.Why here
isHistoryPathis the one owner of which hidden paths history keeps. The ledger joins Studio's manifests in one kept set (KEPT_HIDDEN_PATHS); every other hidden name (a tool's record,.DS_Store) stays out.What I measured
index.html) and passes with it, 3 runs in a row..media/or any hiddenmanifest.jsonlfails it (both mutants checked red).What I did NOT exercise
tests/personWriteEntry.test.mjs, which found this at 0.8.123: it needs a release with this fix; not run against this branch..media/(prefs, cached media) were history-tracked before fix(studio-server): history never files a hidden file as a change #4966 and stay out after this PR; only the ledger was in scope. Whether any of them should come back is not decided here.Size
Small on purpose: a lone fix for a regression from #4966 in this repo, with no other open change here to carry it.