Skip to content

fix(web): isDestroyed editor check - #777

Open
hejsztynx wants to merge 4 commits into
mainfrom
@ksienkiewicz/fix-web-destroyed-editor-check
Open

fix(web): isDestroyed editor check#777
hejsztynx wants to merge 4 commits into
mainfrom
@ksienkiewicz/fix-web-destroyed-editor-check

Conversation

@hejsztynx

@hejsztynx hejsztynx commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

There is a well-known issue with TipTap editor where it doesn't necessarily check for its isDestroyed state, when trying to use e.g. commands.

ueberdosis/tiptap#1451

Here this flow resulted in an uncaught error Cannot read properties of null (reading 'commands'):

There was a found edge-case, when you re-rendered EnrichedTextInput with new htmlStyle and defaultValue:

  1. resolvedHtml changes because of the new htmlStyle
  2. tiptapContent changes because of the new defaultValue
  3. useEditor runs and recreates the input as tiptapContent changed - this is a hook so actual recreation will happen after the render
  4. useEffect with commands.normalizeBoldInStyledHeadings gets scheduled to run after the render. The closure here captures the stale editor, before useEditor actually ran.
  5. After render useEditor successfully runs and updates editor
  6. The latter useEffect runs with a stale editor, which crashes the app

After the added check the 6. will never run, but editor changed, so the same useEffect will correctly run on the next render.

Provided safety-checks, when using commands, outside of the TipTap internal state, in EnrichedTextInput.

all web e2e tests pass

Compatibility

OS Implemented
iOS
Android
Web

Checklist

  • E2E tests are passing
  • Required E2E tests have been added (N/A)

Copilot AI lite review requested due to automatic review settings August 18, 2026 11:25

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a safety wrapper around Tiptap editor operations to avoid running commands on a null/destroyed editor instance.

Changes:

  • Introduced runSafelyInEditor helper to guard editor command execution.
  • Wrapped multiple editor command invocations (focus, blur, setContent, normalization) with the new helper.
  • Updated imperative handle methods to use the safety wrapper.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/web/EnrichedTextInput.tsx Outdated
Comment thread src/web/EnrichedTextInput.tsx Outdated
Comment thread src/web/EnrichedTextInput.tsx Outdated
Comment thread src/web/EnrichedTextInput.tsx Outdated
@hejsztynx
hejsztynx requested a lite review from Copilot August 18, 2026 11:46

Copilot AI 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.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/web/EnrichedTextInput.tsx:189

  • The guard is evaluated against editorInstanceRef.current, but the callback re-reads editorInstanceRef.current again. If the ref changes between the guard check and the callback execution, you may end up blurring a different editor instance (or skipping blur due to optional chaining) than the one you validated. Capture the current editor in a local variable and use that consistently inside the callback, or change runSafelyInEditor to pass the validated editor into the callback (e.g., (safeEditor) => safeEditor.commands.blur()).
        runSafelyInEditor(editorInstanceRef.current, () =>
          editorInstanceRef.current?.commands.blur()
        );

src/web/EnrichedTextInput.tsx:93

  • Using !!editor && ... && toRun() relies on short-circuit side effects, which is harder to read and makes it difficult to extend (e.g., logging, debugging, returning a status). Consider rewriting this as an explicit if block. Also, consider passing the validated editor into the callback so call sites don’t need to close over potentially-stale variables or use optional chaining inside the guarded callback.
function runSafelyInEditor(editor: Editor | null, toRun: () => void) {
  !!editor && !editor.isDestroyed && toRun();
}

@hejsztynx
hejsztynx requested a lite review from Copilot August 18, 2026 11:52

Copilot AI 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.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/web/EnrichedTextInput.tsx:364

  • These imperative methods now return T | null because runSafelyInEditor returns null when the editor is unavailable/destroyed. If EnrichedTextInputInstance previously exposed focus/blur/setValue as returning void or a non-nullable boolean, this is a behavioral and typing change for callers. Consider preserving the prior return contract by (a) making these methods explicitly void and no-op when unsafe, or (b) returning a non-nullable sentinel (e.g., false) when the editor can’t run, or (c) updating EnrichedTextInputInstance to explicitly allow null and documenting the new behavior.
      focus: () => runSafelyInEditor(editor, (e) => e.commands.focus()),
      blur: () => runSafelyInEditor(editor, (e) => e.commands.blur()),
      setValue: (value: string) =>
        runSafelyInEditor(editor, (e) =>
          e.commands.setContent(
            prepareHtmlForTiptap(
              value,
              useHtmlNormalizerRef.current,
              sanitizationConfigRef.current
            )
          )
        ),

src/web/EnrichedTextInput.tsx:99

  • runSafelyInEditor forces callers into a T | null return type, which can easily leak into public APIs (as in the imperative handle) or encourage silently ignoring failures. A more maintainable pattern is to either (1) provide a dedicated runSafelyInEditorVoid(editor, fn): void for side-effecting commands, or (2) accept a required fallback value so the function returns T (no union), making call sites intentional about what happens when the editor is unavailable.
function runSafelyInEditor<T>(
  editor: Editor | null,
  toRun: (editor: Editor) => T
): T | null {
  if (editor && !editor.isDestroyed) {
    return toRun(editor);
  }
  return null;
}

@szydlovsky szydlovsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The safety net looks really good (code-wise).

Comment thread src/web/EnrichedTextInput.tsx Outdated
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.

3 participants