Skip to content

fix(studio-server): undoing a change takes the media ledger back with the film - #5016

Merged
miguel-heygen merged 4 commits into
mainfrom
dchatsolid/media-ledger-history
Oct 4, 2026
Merged

miguel-heygen merged 4 commits into
mainfrom
dchatsolid/media-ledger-history

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

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 keepsLedger mark, 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

isHistoryPath is 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

  • New test "takes the media ledger back with the film when a change is undone" fails without the fix (the entry lists only index.html) and passes with it, 3 runs in a row.
  • "a log 0.8.123 wrote without the media ledger takes it in, so the first Undo undoes the edit" starts from that exact log (the person's edit recorded, the ledger on disk, the log never naming it). It fails without the fix (the first Undo deletes the ledger) and passes with it, 3 runs in a row.
  • "a log 0.8.123 wrote never takes in a ledger behind a link" fails with the link check removed (a phantom entry appears on open) and passes with it.
  • "a ledger made while the project was closed is a change like any file, once a log keeps the ledger" pins the marker.
  • The cutout test asserts the exact files of the entry, so keeping all of .media/ or any hidden manifest.jsonl fails it (both mutants checked red).
  • History and project-signature suites: 156 pass; the touched tests pass 3 runs in a row.

What I did NOT exercise

  • Desktop's tests/personWriteEntry.test.mjs, which found this at 0.8.123: it needs a release with this fix; not run against this branch.
  • An older version that rewrites a marked log drops the mark and the ledger with it, so the next open takes in the ledger as it stands, even one made while the project was closed. Such a log cannot tell the ledger the older version saw from a new one; taking it in is the side that never deletes the ledger on the first Undo.
  • Other files under .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.

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 4, 2026 13:13
@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 1556 (base branch 1556), smooth 1352 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (withoutHiddenPaths stripped 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 outside entry "Changed outside the app" with before = null.
  • next() treats outside entries as everyone's, so step("back") (Cmd+Z) picks that entry first.
  • Repro (head tarball): I opened a project that has index.html and .media/manifest.jsonl with the head code minus the ledger line (that is #4966's isHistoryPath), made one edit, closed it, and reopened with the real head. Entries after the first step("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 and index.html is still the edited v2. A direct undo of 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 tracked without 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 asserts list() is empty and the first step("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 vs main). No other review exists on it.
  • src/history + projectSignature: 153 pass / 2 skipped, 3 runs; tsc, oxlint and oxfmt clean. 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 any manifest.jsonl basename. Nothing pins that other .media/* files stay out, which the PR text says is intended.

Non-blocking

  1. .media/ holds more than the ledger: preferences.json and recipes/<name>/* (committed project files, prefs-store.mjs, recipe-store.mjs), the media files themselves (.media/images/…, audio/…), and index.md. All were tracked before #4966 and are not now. After this PR Undo takes the ledger back but leaves the files and index.md it 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.
  2. I did not run Desktop's personWriteEntry test (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)

…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.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks for the repro. Addressed at 9fdb1fa:

  • First Undo after upgrade deleting the ledger: a log that never names the ledger now takes it into its baseline on open, without recording a change, as you suggested. The test "a log 0.8.123 wrote without the media ledger takes it in, so the first Undo undoes the edit" starts from your exact state (the edit recorded, the ledger on disk, the baseline stripped of it). It fails without the fix: the first Undo deletes the ledger.
  • The take-in runs once. The log's baseline then carries a keepsLedger mark, so a ledger that appears later while the project is closed is filed as an outside change like any file (test: "a ledger made while the project was closed is a change like any file, once a log keeps the ledger").
  • The take-in only accepts a ledger the sweep itself lists, so a .media behind a link is never read into history (test: "a log 0.8.123 wrote never takes in a ledger behind a link", red with the check removed).
  • Your two surviving mutants: the cutout test now asserts the exact files of the entry, with a cached image under .media/ and a manifest.jsonl in another hidden folder. Keeping all of .media/ and keeping any manifest.jsonl name both fail it.
  • Non-blocking 1 (the rest of .media/): still out of scope here; recorded in the PR body as undecided.

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 terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, oxlint and oxfmt --check clean.
  • 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 to index.html that 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 to A) and the ledger is still on disk. On e732a057 the 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 outside change.
    • 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.
  • 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

  1. Survivors, take-in: (a) skipping named(MEDIA_LEDGER) and (b) skipping baseline.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.
  2. "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.
  3. Other .media/* files (preferences.json, recipes/*, the media files) and index.md are 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.
  4. Not exercised: Desktop's personWriteEntry test (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)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 4, 2026
Merged via the queue into main with commit a2f4160 Oct 4, 2026
169 checks passed
@miguel-heygen
miguel-heygen deleted the dchatsolid/media-ledger-history branch October 4, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants