fix(core): don't save the inline Portable Text editor when nothing changed - #3074
fix(core): don't save the inline Portable Text editor when nothing changed#3074eisenbruch wants to merge 2 commits into
Conversation
…anged The inline editor compared a fresh serialization, whose _keys are minted by Math.random() on every call, against the raw stored value, so the check never matched. Every blur out of the body and every pagehide saved a new draft that differed only in keys. Change detection now compares the editor's ProseMirror document with the one last known to be stored (Node.eq): the loaded document once the editor exists, then the document whose blocks each successful save sent. Keys never enter the comparison, an undone edit is not saved, and an edit typed during an in-flight save is no longer absorbed into the baseline. Closes emdash-cms#2877
🦋 Changeset detectedLatest commit: dc88e02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound: phantom saves happen because every serialization mints new Portable Text _keys, so the old JSON.stringify comparison never matched. Moving change detection into ProseMirror’s structural Node.eq fixes that without having to normalize keys, and capturing the sent document as the new baseline prevents in-flight edits from being absorbed. This is the right fix in the right place.
I checked the diff, the full changed file, the new unit tests, and the changeset. No logic bugs or regressions in the production code. The fix correctly avoids PUTs on unchanged blur/pagehide, still saves real edits once, and preserves the shape of the stored body. The changeset is user-facing and accurate.
I have two minor suggestions: the new test relies on reading .editor from the rendered ProseMirror DOM node, which is not a documented @tiptap/react API, and the comment above savedDocRef over-explains the prior bug rather than stating the current invariant. Neither is a blocker.
Overall this is a clean, targeted bug fix with reasonable test coverage.
| function editorOf(editable: HTMLElement): TestEditor { | ||
| const editor = (editable as HTMLElement & { editor?: TestEditor }).editor; |
There was a problem hiding this comment.
[suggestion] The test reads (editable as HTMLElement & { editor?: TestEditor }).editor to get the TipTap editor instance from the rendered ProseMirror DOM node. This is not a documented API of @tiptap/react’s <EditorContent>; if the DOM node does not carry an editor property, the expect(editor).toBeDefined() assertion fails and the "real edit" / "undo" tests never exercise the actual regression. Existing tests in this directory drive the component through DOM events or custom events instead.
Please verify this property exists in the installed TipTap version. If it does not, replace it with a stable command path—ideally by driving the ProseMirror element with input/keyboard events, or by adding a small test-only hook/ref rather than relying on internal DOM attachments.
| // The editor document as last known to be stored: the one the loaded | ||
| // content produced, then whatever each successful save sent. Saves compare | ||
| // against it structurally. Comparing serialized Portable Text against the | ||
| // raw `value` never matched, because serialization mints new `_key`s on | ||
| // every call, so every blur and every page leave saved a new draft. |
There was a problem hiding this comment.
[suggestion] This comment is mostly a historical explanation of the previous bug. The first sentence is a useful invariant; the rest re-states why the old key-based comparison failed. That rationale already belongs in the changeset and the PR description. For code readers, keep the docstring and drop the historical justification:
| // The editor document as last known to be stored: the one the loaded | |
| // content produced, then whatever each successful save sent. Saves compare | |
| // against it structurally. Comparing serialized Portable Text against the | |
| // raw `value` never matched, because serialization mints new `_key`s on | |
| // every call, so every blur and every page leave saved a new draft. | |
| // The editor document as last known to be stored: the loaded document, | |
| // then the document whose blocks were sent by each successful save. | |
| // Saves compare against it structurally with ProseMirror `Node.eq`. | |
| const savedDocRef = React.useRef<Editor["state"]["doc"] | null>(null); |
…ads view.dom.editor Review follow-ups on emdash-cms#3074: the savedDocRef comment now states what it holds rather than the old bug, and the test explains its use of the editor instance Tiptap attaches to the view's root element.
github-actions
Bot
added
review/needs-rereview
1 participant
What does this PR do?
With visual editing on, the inline Portable Text editor saves the body every time focus leaves it and every time the page is left, even when nobody typed. Each save creates a draft that differs from the live version only in
_keyvalues, so entries show "Pending changes" that no one made.Closes #2877
Cause
pmToPortableText()mints a new_keyfor every block, span and link markDef on every call (k()isMath.random()), and spans refer to link markDefs by those keys.save()comparedJSON.stringify(getBlocks())againstinitialRef, which starts as the raw stored value and, after a save, becomes another fresh serialization. The two never match.handleBlurand thepagehidekeepalive flush both callsave(), so an untouched body is saved on every blur and on every page leave.Fix
One file,
InlinePortableTextEditor.tsx. Change detection moves from serialized Portable Text to the editor's own document:save()returns early wheneditor.state.doc.eq(savedDoc), ProseMirror's structural equality over node types, attributes, marks and text. ProseMirror nodes have no_key, so the random keys never enter the comparison.getBlocks()again.Two side effects, both improvements:
initialRef = getBlocks()after the response picked it up, so that edit could never be saved.The stored shape does not change: the PUT still sends the editor's blocks with their keys. Making keys deterministic, so they stop churning on real saves, is a separate change and is not in this PR.
Can this hide a real edit?
No.
Node.eqcompares the whole document tree: node types and order, attributes (heading level, alignment, list fields, image and link attributes), marks and text. Reordering, splitting or merging blocks, typing, or changing a link all produce a document that is noteq. The only thing it cannot see is_key, which lives only in the serialized Portable Text, and ignoring_keyis the point.I first tried a canonical-JSON comparison that dropped
_keyand remapped link-mark references to their markDef position. It worked, but it needed about 40 lines of normalization, plus care for every place Portable Text carries a key reference. Comparing the editor document avoids that class of mistake.Type of change
Checklist
pnpm typecheckpasses: no errors in the changed files.tsgo --noEmitinpackages/corereports errors only in untouched files (src/api/handlers/registry.ts,src/registry/*,src/plugins/types.ts,src/utils/slugify.ts) in my environment.pnpm lintpasses:oxlint --type-awareclean on the changed filespnpm testpasses (or targeted tests for my change): all 15 files intests/unit/componentspass (89 tests)pnpm formathas been runemdash: patch)AI-generated code disclosure
Screenshots / test output
Not applicable (no UI change). New file
tests/unit/components/inline-portable-text-unchanged-save.test.ts; the stored body includes a link paragraph. Against currentmainwithout the fix:With the fix, all four pass.