githubnext/chopinPublic

Let's compose rich plans together

Undo and redo each person's own document edits #352

MergedMaggieAppletonmerged 6 commits intomainfromrefs/pull/352/head
  1. MA

    ## Why Document editing had no working undo or redo: shared Lexical history was disabled without a Yjs undo manager.

    ## What changed - Track each editor binding's own changes with a Yjs `UndoManager`, leaving peer and Planner changes outside personal history. - Rehearse undo and redo steps before applying them when they could affect shared content. Reject steps that would delete another writer's work, change authoritative projections, or produce invalid MDX. - Drop a refused step while retaining older history. Show a brief, accessible “Can't undo” notice with the reason. - Start fresh history after an epoch reset. - Renew the reviewed design-contract hashes for the editor wiring and status notice.

    ## Verification The 25 focused history tests, including randomized multi-peer cases, `bun run types`, and `bun run ci` passed locally on the rebased commits. Required GitHub checks will run on the final published head.

    ## Visuals Before: text remains after ⌘Z. ![Before undo](https://github.com/user-attachments/assets/6d8a0211-b3b3-494e-9a50-fade688951d8)

    After: ![Text undone](https://github.com/user-attachments/assets/7c6190d1-ae8d-48cf-8b48-68a48e8dd6bf)

    Refused undo: ![Undo refusal notice](https://github.com/user-attachments/assets/4595810d-825e-4c48-aa78-b2c042b69bd3)

  2. MA
    Author

    **Review:** Changes needed

    Wiring the Yjs UndoManager to the local binding origin is the right approach. Basic undo/redo works, and the keyboard mapping is correct: on Mac ⌘Z / ⇧⌘Z work, and with the platform forced to Windows, Ctrl+Z, Ctrl+Y and Ctrl+Shift+Z all work. Ctrl/Meta+Z is ignored while the editor is locked. But I found two ways an undo can corrupt the shared document. One leaves the document unopenable.

    ### 1. Blocking: undoing a block a peer has typed in makes the document unopenable `history.ts` `foreign()` / `deleteFilter`: the filter keeps the peer-touched `XmlText`/`XmlElement`. But it still deletes that element's **attribute items** (`parentSub !== null`: `__type`, `__format`, `__indent`, table `__align`…), because they are this client's own items and are not "foreign". The kept block ends up with `getAttributes() === {}`.

    - Clients that are already open keep their stale Lexical node, so the problem is hidden. The PR's test "keeps a paragraph this person created once a peer has typed in it" passes for this reason. - A fresh load cannot build the block. A headless newcomer built from `Y.encodeStateAsUpdate(me.doc)` silently drops the whole paragraph, including the peer's text. - **Reproduced in a real browser** (fake server, octocat + hubot). octocat: Enter, types "Mine." hubot: types " Theirs." at the end of that paragraph. octocat: ⌘Z. Result: - Both live editors still show `Mine. Theirs.` (the undo looks like it did nothing). - The stored canonical source has `Start here.\n\n Theirs.`. - A fresh load shows only `Start here.`. - After the room was evicted, reopening fails with `session:error "stored plan source does not match its Yjs checkpoint"`. **Both documents I tested this on are now unopenable.** A document where no undo happened still opens. - Tables do the same: undoing a 3×3 table creation after a peer typed in one cell leaves a 1×1 table whose table and row nodes have `__type === undefined`.

    Fix: never delete an attribute/map entry (`item.parentSub !== null`) whose parent type survives the undo. Also add a regression test that rebuilds a **fresh** headless editor from the Yjs state after the undo and compares it to the live editors and `exportPlan`, for a paragraph, a list item and a table.

    ### 2. Blocking: a no-op top step lets Yjs undo an unchecked projection step `step()` only runs `touchesProjection` on `stack.at(-1)`. But Yjs `popStackItem` loops `while (stack.length > 0 && currStackItem === null)`: if the top item changes nothing (a peer already deleted my text), the same `undo()` call pops and applies the **next** item without the check.

    Reproduced with the PR's harness: 1. Insert `$createResearchNode(...)`. 2. Wait >500 ms (a new capture). 3. Type " x" into a paragraph. 4. The peer deletes that text. 5. ⌘Z.

    The research projection is removed from every peer, which is exactly the batch the server rejects (plan:reset / epoch rebuild). Fix: decide at capture time instead. On `stack-item-added` (and on redo-stack add), if the item touches a projection, clear the stack at and below it. Or pop no-op items manually and re-check each one before calling `undo()`/`redo()`. Add this case as a test.

    ### 3. Should fix: the setTimeout(0) reaction window can drop real keystrokes `react()` removes `binding` from `trackedOrigins` until a macrotask. Chrome prioritises input tasks over timers, so a keystroke that arrives right after a remote update can run before the timer and go untracked. With an active peer or a streaming Planner, the timer is re-armed on every update, so this happens often. The visible effect: "Hello" with an untracked `l` undoes to a stray `l`. An untracked deletion also makes the next tracked item a no-op, which then triggers the fall-through in #2.

    Consider limiting the exclusion to the synchronous/microtask window, e.g. a flag cleared in `queueMicrotask` after Lexical's commit and its nested updates. Or make the exclusion specific: tag MDXEditor's trailing-paragraph update and drop only that update. Also note that `react()` fires for the UndoManager's own transactions. That is intended, but it means a keystroke right after an undo can be dropped too.

    ### Minor - Ctrl/Meta+Z handling is fine. Meta+Z also undoes on Windows (Lexical `CONTROL_OR_META`), which is harmless. - Not exercised: undo across an epoch reset (a remount gives a fresh manager, which looks right) and IME composition. The fake Planner was unavailable (8797 was held by another server), so a Planner edit arriving between my edits was only covered by the unit-level remote-origin path.

    ### Checks - `bun test packages/editor/src/history.test.ts`: 10 pass. `bun test packages/editor`: 493 pass. - CI: `e2e` passes and `container` passes. `format, lint, types, tests` fails only on the pinned design-contract hash for `packages/editor/src/collaboration.tsx`, as declared.

  3. MA
    Author

    Thanks. Fixed in 38ef59ba. I replaced the per-item rules with one check that runs on a copy of the document before each undo or redo.

    1. **Kept block losing its attributes.** `deleteFilter` and its partial deletes are gone. Each undo or redo now replays first on a `gc: false` copy of the doc. Redo pointers are carried over, and the replay includes Yjs passing over no-op steps. The real step runs only if: - it deletes nothing another client wrote, - every protected projection's serialized MDX is unchanged (compared the same way `protectProjections` does), and - the live-applied export equals a fresh load's export and passes dialect validation.

    If any check fails, the history ends there, so undoing a block a peer typed in now does nothing. New tests cover a paragraph, a list item and a table: each checks fresh load == every live editor == exported source. In the browser, I reran your repro (octocat Enter + "Mine.", hubot " Theirs.", octocat ⌘Z). Both live editors and a fresh load show the same text, with no reset or session error. 2. **No-op top step.** The replay calls the same `undo()`/`redo()` on a copy of the stack, so whichever step Yjs actually reaches is the one checked. Your scenario is a test: the research projection stays on every peer. 3. **Reaction window.** It's now a few microtasks, re-armed per remote change, so no input task can land inside it. A test types in the next task (`setImmediate`) right after a remote change and checks the keystroke is undoable.

    I also added a randomized stress test: 6 seeds × 60 rounds, 3 peers plus a server mirror, with type, insert and delete text, tables, research inserts, server decisions, undo and redo over a document with a Questionnaire. After every round it asserts fresh load == all live editors == server and that no projection count ever drops. About 85% of undos ran and the rest were refused.

    `bun test packages/editor apps/web`: 1003 pass. `bun run types` is clean. The only `ci` failure is still the pinned hash for `collaboration.tsx`; the new hash is in the PR body.

    One thing I found along the way: a Questionnaire node's `exportJSON` drops `thread` after an incremental Yjs sync, but its serialized MDX doesn't change. That's why the projection comparison uses MDX rather than node JSON.

  4. MA
    Author

    **Hold — please don't merge yet.** This changes collaborative sync correctness and an adversarial review of the latest commit is still in progress. I'll post the verdict here.

    🤖 Generated with [Claude Code](https://claude.com/claude-code)

  5. MA
    Author

    **Review:** Looks good

    I re-reviewed 38ef59ba. Both blockers from my first review are fixed, and I couldn't build an undo that corrupts the document.

    ### What I reran - **My earlier adversarial tests** (paragraph, table, and research projection after a no-op top step) all pass now. In each case the undo is refused, the research projection stays on every peer, and a newcomer loaded from the Yjs state matches the live editors. - **In a real browser** (fake server, octocat and hubot), I checked that the live editors, a fresh load after the room was evicted, and the stored source all agree: - A peer typed into my paragraph, then I undid: consistent, and the undo was refused. - A peer typed into my list item, then I pressed ⌘Z twice: consistent, refused. - Both people edited separate blocks, then I undid: consistent, and my own paragraph was undone. - No `plan:reset` or `session:error` in any run. Both documents reopen. - **Tests:** `bun test packages/editor/src/history.test.ts` passes 18, including the 6×60 stress test.

    ### Should fix (small) 1. **`same()` compares projections as a Set** (`history.ts`, `projections`/`same`). Two identical projections collapse into one entry, so an undo that leaves a Decision or Research **twice** passes the check. The server's `collect` rejects that ("`id` appears twice"), which means a `plan:reset`. A plausible way to get there: I move a card, a peer moves it again, then I undo. Yjs restores the original without deleting the peer's copy. I couldn't finish this in the headless harness, because a Lexical-level move of a decorator there loses its `__state` on peers (see the note below). Fix: compare sorted arrays (a multiset), or key by `id` and reject duplicates the way `protectProjections` does.

    ### Performance (flag) Every undo or redo builds two headless editors from the full state and exports MDX twice. All timings are for the synchronous ⌘Z handler:

    | Setup | Time per undo | | --- | --- | | Chromium on an M-series Mac, ~10k words | 18–30 ms | | Same, 4× CPU throttle (roughly a mid-range Windows laptop) | 82–102 ms | | Bun headless, 10.8k words, 5 tables, 20 Decisions | 36–52 ms |

    So a heavy document on an ordinary machine is over the ~50 ms budget, and holding ⌘Z (about 30 repeats a second) will freeze typing. This doesn't block merging, but it's worth a follow-up: - Skip the fresh-load comparison when the step deletes no element or attribute items and touches no protected subtree. Plain typing is most undos. - Or keep one shadow mirror up to date instead of rebuilding two editors each time. - At minimum, debounce the check while the key is repeating.

    ### UX: refused undo Right now a refused undo does nothing **and** silently throws away the rest of the person's history, including older edits that were safe to undo. From the user's side, ⌘Z just stops working. I recommend a quiet, transient status near the document status chip (not a toast), shown once per refusal: "Can't undo — others have edited this since." Separately, consider clearing only the refused step and the steps below it, which is what you have now, but saying so. The message matters more than the mechanism. Without it, a silent no-op reads as a bug, which is the same complaint that started this PR.

    ### Note (not this PR) In the headless two-peer harness, moving a Decision or Research node with Lexical `insertAfter` reaches peers without its `__state`, so `exportPlan` fails validation (`Research requires id`). That may be a harness artifact, but it would be worth checking that dragging a card in the real app round-trips to peers.

    ### Checks - `e2e`: pass. `container`: pass. - `format, lint, types, tests`: fails only on the pinned design-contract hash for `packages/editor/src/collaboration.tsx`, as declared.

  6. MaggieAppletonhead-ref-force-pushed 483c6a2ce4b8last month on GitHub

  7. MA
    Author

    Thanks. All of this is in 483c6a2c, rebased on main.

    1. **Duplicates:** projections are now compared as a sorted multiset of their serialized MDX. A new test moves a card, has a peer move it again, then undoes: it's refused, and every mirror keeps exactly one Decision. The test fails under the old Set comparison. 2. **Performance:** there is now a quick path for undoing a step that only removes prose this client typed. That's its own characters, the text nodes holding them, and top-level paragraphs, headings or quotes with no foreign content inside. It needs no rehearsal. Each run of the stress test (now 10 seeds) checks that the quick path is used at least once, and every time it is, it asserts the full rehearsal agrees. Measured in Chromium on a 10,064-word document, timing the undo handler with real ⌘Z:

    | | 1× | 4× throttle | |---|---|---| | Undo typing within existing text | 0.1–0.5 ms | 0.2–2.4 ms | | Undo a new paragraph | 0.4 ms | 1.3–1.6 ms | | Structural undo, e.g. a markdown-shortcut list (full rehearsal) | ~59 ms | ~250 ms |

    I didn't keep a shadow copy. It would double the per-update cost of every remote change for every client, to save roughly half of a path that's used only for structural undos. Most of that path's cost is building the fresh-load state, and the shadow wouldn't avoid that.

    I also didn't add key-repeat coalescing. Coalescing would mean deferring undo past the keydown, which could reorder it with typing. Undoing typing is already well under a frame. 3. **UX:** a refused step now drops only itself, plus any empty steps Yjs passed over, and older steps stay. Each older step is rehearsed when its turn comes; a test covers this. The status pill shows "Can't undo · Others have edited this since." (or "This change can't be reversed here." for card or invalid cases) for 3 s, using the existing `notice` level, which is `aria-live` polite. The screenshot is in the PR body.

    `history.test.ts`: 24 pass. `bun test packages/editor apps/web`: 1011 pass. 7 icon/token tests timed out under machine load and pass when rerun with a longer timeout. `bun run types` is clean. `ci` fails only on the pinned hashes for `collaboration.tsx` and `plan-editor.tsx`; the new hashes are in the PR body.

    Heads-up: `status.tsx` and `plan-editor.tsx` overlap with #297. Whichever of the two lands second needs a small merge.

  8. MA
    Author

    Lifting the hold on this PR: the re-review follow-ups are in 483c6a2c. CI for that commit was still running when I posted this. The 'format, lint, types, tests' job is expected to fail only on the pinned design-contract hashes for `collaboration.tsx` and `plan-editor.tsx`, which need a review (new hashes are in the PR body). I ran types and tests locally and they pass.

  9. MA
    Author

    **Hold re-applied briefly.** The latest commit adds a fast path that skips the replay check for some undos. A focused re-check of that path is in progress; I'll post the verdict here.

    🤖 Generated with [Claude Code](https://claude.com/claude-code)

  10. MA
    Author

    **Review:** Looks good

    Hold lifted.

    This was a focused adversarial check of 483c6a2c's quick path for undoing typing (`typing()` in `history.ts`). For every case below I compared the quick-path verdict with the full rehearsal (`rehearse(…, false)`), then undid. Afterwards I asserted that a fresh load matched every live editor and the server mirror, and that the projection count was unchanged.

    | Case | Quick path | Full rehearsal | Result | | --- | --- | --- | --- | | Peer bolds my newly typed text | used | ok | consistent | | Peer bolds part of my new paragraph (text split) | declined | refused | consistent, refused | | Peer adds a link inside my new paragraph | declined | refused | consistent, refused | | Peer adds a link after my typing in an existing paragraph | used | ok | consistent | | Peer splits my new paragraph | declined | ok | consistent | | Peer merges my paragraph into the previous one | declined (no-op step) | ok | consistent | | Peer edits the text inside my new paragraph | declined | refused | consistent, refused | | Server puts a Decision right after my new last paragraph | used | ok | Decision kept, consistent | | Server puts a Decision between two paragraphs I created in one step | used | ok | Decision kept, consistent | | **Peer sets alignment/indent on my new paragraph** | **used** | **refused ("others")** | consistent; the peer's alignment goes with my paragraph |

    None of these corrupts fresh-load vs live, removes a projection, or loses peer text.

    **One mismatch, not blocking:** the last row. `own()` walks only the block's children, not its `_map` attributes. So the quick path deletes a peer-written `__format`/`__indent` along with my paragraph, while the full rehearsal refuses that step. Nothing breaks, and only the peer's styling of my own block is lost. But it contradicts "deletes nothing another client wrote" and the stress test's claim that the two paths agree. That's because the stress test never edits block attributes. Fix: also reject a block whose `_map` has a live item from another client, and add block alignment/indent to the stress actions.

    **Comment anchors:** a peer's comment anchored on text I then undo becomes orphaned. That happens on both paths and is the documented anchor behaviour, so I didn't count it as a regression.

    ### Also checked - My earlier adversarial tests: paragraph, table and the research no-op fall-through all pass. The tests that move a card in the harness still fail before any undo runs: the moved node loses its `__state`, which is the harness problem I noted last time. - Undo latency on a 10.8k-word document is now 0.3–0.4 ms per typing undo. - `history.test.ts`: 24 pass, including the stress test. - Browser on port 8848, octocat and hubot, comparing live editors, a fresh load after the room was evicted, and the stored source: - Paragraph a peer typed in: consistent. ⌘Z is refused and the "Can't undo · Others have edited this since." pill shows quietly bottom-right. - List item a peer typed in: consistent. The second ⌘Z now undoes my older "Start here." step, so a refused step no longer clears the rest of my history. - My own paragraph: consistent. - No `plan:reset` or `session:error` in any run.

  11. MA
    Author

    Fixed in 65b3186f. The quick path now declines whenever any current property of the block, or of a text node inside it, was set by another client, such as alignment, indent or bold. Those cases go to the full check, which refuses them as "others". A new test (I add a paragraph, a peer centres it, I undo) checks that the quick path declines and the paragraph survives; it fails without the change. The randomized stress test now also sets alignment, indent and bold on other people's prose. `history.test.ts`: 25 pass. Types are clean. The pinned hashes are unchanged.

  12. MaggieAppletonreferenced this in Make tabs authorable in place and label empty table headers #382last month on GitHub

  13. MaggieAppletonhead-ref-force-pushed 33e8d44eccaflast month on GitHub

  14. MA
    Author

    I rebased onto main in 33e8d44e. The undo logic is unchanged. Main now renders `PlanStatus` in the room header, so the refusal moved onto `PlanState.refused`, which clears itself after 3 s. `room-workspace.tsx` shows it beside "Edits not saved" as a `notice`-level "Can't undo" label, with the reason in a tooltip and a polite `role="status"` region for screen readers. It sits outside `PlanStatus`, so clearing it never triggers `announcement()`'s "Reconnected". I checked this in the browser: the header showed "Can't undo", then it disappeared and "Reconnected" never appeared. `history.test.ts` 25 pass (including the stress test); `bun test --timeout 60000 packages/editor apps/web` 1094 pass; types are clean. In CI, e2e and container pass. The format/lint/types/tests job fails, which I expect is the pinned hashes for `collaboration.tsx` and `plan-editor.tsx`; I haven't opened the CI log, but they're the only local `ci` failures. The `plan-editor.tsx` hash in the PR body is updated.

  15. MaggieAppletonauto-rebase-enabledlast month on GitHub

  16. MaggieAppletonauto-merge-disabledlast month on GitHub

  17. MaggieAppletonhead-ref-force-pushed d0c7b26c2f53last month on GitHub

  18. MaggieAppletonauto-rebase-enabledlast month on GitHub

  19. MaggieAppletonmerged 91d8eb77e493last month on GitHub

  20. MaggieAppletonclosedlast month on GitHub

  21. MaggieAppletonhead-ref-deletedlast month on GitHub