Skip to content

Prevent composer crash from Slate node-key errors (MAILSPRING-CLIENT-2X) #2795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bengotow wants to merge 1 commit into
base: master
Choose a base branch
from
+31 −3
Open

Prevent composer crash from Slate node-key errors (MAILSPRING-CLIENT-2X) #2795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions app/src/components/composer-editor/base-block-plugins.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,23 @@ function toggleBlockTypeWithBreakout(editor: Editor, type: string) {
if (idx !== -1) {
const depth = ancestors.size - idx;
if (depth > 0) {
editor.splitBlock(ancestors.size - idx);
for (let x = 0; x < depth; x++) editor.unwrapBlock({ type });
try {
editor.splitBlock(ancestors.size - idx);
for (let x = 0; x < depth; x++) editor.unwrapBlock({ type });
} catch (err) {
// Slate's `unwrapBlock` (unwrapBlockAtRange in slate/lib/slate.js) walks the
// ancestor's children and moves them out one at a time using node keys it
// collected before the split/unwind began. When breaking out of multiple levels
// of nested blocks in one go (eg. multi-level quoted replies), those keys can
// point at nodes that a prior split/unwrap already relocated or removed, and
// Slate's own `moveNodeByKey`/`assertPath` throws "could not find node with path
// or key" instead of recovering. This is a bug inside Slate's compound change
// function, not something we can fix from a plugin, so we just stop here rather
// than let the exception escape and abort the whole editor command mid-mutation.
// See MAILSPRING-CLIENT-2X.
console.warn('toggleBlockTypeWithBreakout: failed to break out of nested block', err);

@indent-staging indent-staging Bot Aug 13, 2026

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.

Latent: tmp.normalize stuck at false after this catch fires.

Both unwrapBlockAtRange (slate.js:7161) and insertFragmentAtRange (slate.js:6721) do their moves inside editor.withoutNormalizing(fn), and that helper (slate.js:11515) is not exception-safe:

function withoutNormalizing(fn) {
  var value = this.tmp.normalize;
  this.tmp.normalize = false;
  fn(controller);              // ← throws here
  this.tmp.normalize = value;  // never runs
  normalizeDirtyPaths(this);   // never runs
}

Once fn throws, this.tmp.normalize stays false for the lifetime of the Editor. normalizeDirtyPaths (slate.js:11697) is a no-op when tmp.normalize === false, so every subsequent command in this composer session runs without schema normalization. Dirty paths accumulate and the document can drift into invalid shapes that ComposerEditor.onChange's getDocumentBrokenReason recovery won't catch (only the most severe shapes are repaired).

The same applies to the sibling catch in composer-editor.tsx around editor.insertFragment. A pragmatic mitigation in both catches: try { editor.tmp.normalize = true; } catch {} (or reset via the controller) before returning, so at least the next command re-enables normalization. Scope is bounded to the current composer session — closing/reopening the draft rebuilds the Editor and clears the flag — but a user who hits this once will keep editing in a non-normalizing editor until then.

@bengotow bengotow Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thank you — confirmed against the actual vendored bundle and fixed in 6de5899.

One correction to the suggested mitigation: editor.tmp.normalize = true on its own wouldn't have worked here. The editor object these catch blocks receive is slate-react's <Editor> React component, not the core Slate editor — it has its own unrelated tmp (React-internal bookkeeping: resolves/mounted/updates/change/isUpdatingSelection, see slate-react.js ~5212), while the real tmp.normalize/tmp.dirty flags live on editor.controller (the core slate.Editor instance the component forwards commands to, per the "Mimic the API of the Editor controller" comment at slate-react.js:5238). So the fix reaches through editor.controller.tmp.normalize instead, and also calls .controller.normalize() to force an immediate re-normalization pass rather than just waiting for the next incidental command.

That second part turned out to matter for another reason you also flagged in the PR summary: Slate schedules its flush-to-onChange on a microtask queued by the first successful operation in a batch (slate.js:11179-11184), so a partial mutation from an interrupted command does reach ComposerEditor.onChange — my original commit message was wrong to claim otherwise. Calling .normalize() synchronously in the catch, before that pending microtask fires, cleans up the partial mutation so what actually reaches onChange is normalized rather than raw. Extracted both fixes into a shared recoverFromInterruptedSlateCommand helper used by both catch sites.

Generated by Claude Code

@bengotow bengotow Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thank you — confirmed against the actual vendored bundle and fixed in 6de5899.

One correction to the suggested mitigation: editor.tmp.normalize = true on its own wouldn't have worked here. The editor object these catch blocks receive is slate-react's <Editor> React component, not the core Slate editor — it has its own unrelated tmp (React-internal bookkeeping: resolves/mounted/updates/change/isUpdatingSelection, see slate-react.js ~5212), while the real tmp.normalize/tmp.dirty flags live on editor.controller (the core slate.Editor instance the component forwards commands to, per the "Mimic the API of the Editor controller" comment at slate-react.js:5238). So the fix reaches through editor.controller.tmp.normalize instead, and also calls .controller.normalize() to force an immediate re-normalization pass rather than just waiting for the next incidental command.

That second part turned out to matter for another reason you also flagged in the PR summary: Slate schedules its flush-to-onChange on a microtask queued by the first successful operation in a batch (slate.js:11179-11184), so a partial mutation from an interrupted command does reach ComposerEditor.onChange — my original commit message was wrong to claim otherwise. Calling .normalize() synchronously in the catch, before that pending microtask fires, cleans up the partial mutation so what actually reaches onChange is normalized rather than raw. Extracted both fixes into a shared recoverFromInterruptedSlateCommand helper used by both catch sites.

Generated by Claude Code

return;
}
}
editor.setBlocks(BLOCK_CONFIG.div.type);
} else {
Expand Down
15 changes: 14 additions & 1 deletion app/src/components/composer-editor/composer-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,20 @@ export class ComposerEditor extends React.Component<ComposerEditorProps, Compose
}
const value = convertFromHTML(html);
if (value && value.document) {
editor.insertFragment(value.document);
try {
editor.insertFragment(value.document);
} catch (err) {
// Slate's `insertFragment` (insertFragmentAtRange in slate/lib/slate.js) merges
// the pasted fragment's blocks into the surrounding document using node keys it
// captured before the merge/split steps that precede the merge. For certain
// multi-block fragments pasted into multi-block content, those keys can point at
// nodes that an earlier step in the same merge already relocated, and Slate's own
// `moveNodeByKey`/`assertPath` throws "could not find node with path or key"
// instead of recovering. This is a bug inside Slate's compound change function,
// not something we can fix from here, so we just drop the paste rather than let
// the exception escape mid-mutation. See MAILSPRING-CLIENT-2X.
console.warn('ComposerEditor: insertFragment failed, paste may be incomplete', err);
}
event.preventDefault();
return;
}
Expand Down
Loading