fix: don't restore a stale caret position onto content it wasn't computed against

[jimthedev]

useEditable restores the saved caret position on every render via a useLayoutEffect with no dependency array, assuming that render is always the direct result of its own onChange call. In practice, an unrelated re-render (triggered by something outside this hook, e.g. a parent component re-rendering for its own reasons) can land in the narrow window between one keystroke's keydown and keyup. When that happens, the DOM may already contain a newer native edit the hook doesn't know about yet, and forcibly restoring the old saved position onto it yanks the browser's own correct cursor to the wrong offset — corrupting every keystroke that follows.

This change tracks the content each saved position was computed against (state.expectedContent, set alongside state.position in update, flushChanges, undo/redo, and onSelect) and only restores the caret if the DOM still shows that exact content. If a stray render lands mid-keystroke, the restore is now skipped and the browser's native selection is left alone instead of being clobbered.

No public API changes; purely an internal robustness fix.

[Copilot]

🟡 Changes recommended

Normalize expected content before storing it to preserve documented caret adjustments.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR prevents stale caret restoration when the DOM changes between renders.

Changes:

• Tracks expected content with saved caret positions.
• Validates content before restoring selections.
• Updates tracking for edits, undo/redo, and selection changes.

File summaries

File	Summary
src/useEditable.ts	Adds content-aware caret restoration; expected content must be normalized to preserve caret adjustments.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

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

[ilteoood]

The fix is sound and well-scoped. The new gate (matchesExpectedContent) correctly distinguishes hook-driven renders from stray renders and prevents the caret from being clobbered mid-keystroke. Below are a few things worth tightening before merge, plus smaller observations.

---

Project-level concerns

• Commit message is fix: resolve typing bug but the PR targets a caret-staleness issue. Update to match the PR title (~fix: don't restore a stale caret position onto content it wasn't computed against) so git log isn't misleading.

• No regression test. package.json has no test runner config, so adding tests is a non-trivial scope expansion — but the bug has no reproduction once CI merges, and Storybook is the only validation surface in this repo. Even one Storybook story ("UncontrolledParentRerender" that triggers a sibling setState on every keystroke and asserts the caret sticks) would document the fix. Worth raising as a follow-up issue at minimum.

---

File-level observations

• insert() is a fourth path that benefits silently from the new check — worth a one-liner comment. insert (line 229) mutates the DOM directly via range.deleteContents/insertNode and sets the cursor via setCurrentRange; it never touches state.position or state.expectedContent and doesn't call onChange. With the new gate, a stray render landing between insert and the next flushChanges() correctly skips restore (DOM differs → guard returns false → cursor left untouched, which is the right outcome since insert already placed the cursor). Future readers will wonder why expectedContent isn't updated here — a one-line comment so it isn't a mystery.

• Disabled/unmount clears history/historyAt but not state.position/state.expectedContent (line 294). Pre-existing pattern, not a regression. If the host toggles disabled mid-keystroke and re-enables, the stale expectedContent could suppress the next caret restore. Benign because the matching guard at line 301 still runs on re-enable — but worth a comment if you're touching adjacent code.

---

Verified (no action needed)

• All four assignment paths set state.position and state.expectedContent together: lines 225, 368, 417, 482. No path can desync them.
• expectedContent === undefined short-circuit correctly preserves the "first selection / before any change" semantics.
• hasSelection extraction in onSelect (line 480) correctly mirrors both branches: position+expectedContent set on selection present; position=null+expectedContent=undefined on absent.
• TypeScript string | undefined matches every assignment site.
• History = [Position, string] is unchanged; history[1] is correctly used as expectedContent in undo/redo (line 417).

[ilteoood]

Docstring overstates the trigger. This says the bad render "can commit in between a keystroke's keydown and keyup." The guard actually runs on every render, and update, flushChanges, undo/redo, and onSelect all benefit from it. Reword to match the title: e.g. "Any render that isn't the direct result of this hook's own onChange call — e.g. one triggered by an unrelated state update elsewhere in the tree — can commit while the DOM already reflects content this hook hasn't observed yet. In the keystroke case this lands between keydown and keyup…"

[ilteoood]

Style nit: JSDoc vs // drift. This uses /** … */ while matchesExpectedContent (lines 147-156) and the rest of the file use //. Stick with // to match the file's convention.

[ilteoood]

update() and flushChanges()/onSelect() store expectedContent differently — this will silently skip caret restore in some parent setups.

• Here at line 225, expectedContent is set to the raw content parameter.
• At lines 368 and 482, expectedContent is set to toString(element), which always appends a trailing \n (see toString lines 46-47).

If a parent calls edit.update("foo") and renders the contenteditable as React-controlled JSX that doesn't add the trailing newline (the typical React-controlled pattern), the next render's DOM may be "foo" while state.expectedContent === "foo", so they could differ by a trailing \n and the guard returns false, silently skipping the caret restore.

Either factor a normalize(content: string) helper and use it at every assignment site (225, 368, 482), or document the parent-mirroring requirement at the top of matchesExpectedContent.

[ilteoood]

Per-render toString cost. This check (and the identical one at line 301) calls toString(element) on every render. toString walks the full DOM (lines 24-49). For long editors this doubles per-render stringification work on top of the walks already done elsewhere. Optional optimization: short-circuit by comparing element.textContent first and only walk when that suggests divergence.

[ilteoood]

flushChanges only updates state.position/state.expectedContent when state.queue.length > 0. Combined with the expectedContent === undefined short-circuit at line 156, a no-op flushChanges (after the first one has fired) leaves expectedContent === undefined, so the next render's guard returns true unconditionally.

Intentional per the matchesExpectedContent docstring ("before the first change"), but worth flagging: if you ever want defense-in-depth on browser-emitted native edits that produce empty mutation batches (rare, browser-dependent), you'd need this branch to set state.expectedContent unconditionally too.

[jimthedev]

Thanks for the thorough look.

The normalization one is a real bug and I'm glad it got caught. update() was the only site storing the raw string while everything else goes through toString(), so edit.update('abc') would've stored 'abc', read back 'abc\n', and quietly never restored the caret. I pulled the trailing-newline logic out of toString into a small normalize() helper and update() now runs its argument through that for both expectedContent and the caret delta. The delta was already off by one for callers who leave the newline off (pre-existing, not from this PR), but since the same helper fixes both I folded it in rather than leave update() half-working. Called that out in the description. onChange still receives whatever the caller passed, so nothing changes for the parent.

Also took the docstring rewording (agreed, the keystroke window was one example and I'd written it like it was the whole story) and added a note on insert() explaining why it doesn't touch expectedContent. Commit message is fixed up too, that was a leftover from before I'd narrowed the bug down.

A couple I'm going to leave as-is, happy to be talked out of it:

The // vs /** */ thing: the file already uses JSDoc on interface members (see Edit) and // for everything else, so the State field comment is following the existing split.
The textContent short-circuit: it isn't a safe pre-check as suggested because textContent doesn't turn
into \n or add the trailing newline, so it can disagree with toString on content that's actually identical. Agreed it's one extra walk per render, but it's the same order of cost as the walks the hook already does, so I'd rather keep the check simple and obviously correct.
The disabled / empty-queue flushChanges notes: agree they're both pre-existing and benign, the undefined short-circuit covers them. Didn't want to grow the diff for those.
On the regression test, agreed it'd be nice. There's no runner in the repo so I'd rather do a Storybook story as a follow-up than bolt test infra onto this PR. Happy to open an issue for it.

[ryan-roemer]

Thanks so much for the work @jimthedev and review @ilteoood ! 🎉

I'm approving as the code here LGTM.

This is non-blocking, but has anyone tried this change in react-live? No worries if not -- we should queue up a PR in react-live to next bring this in and we can just tackle it there if needed...

[jimthedev]

@ryan-roemer I have pnpm patching it into react-live. Next I'd like to work on a way for react-live not to focus trap. :D

[ryan-roemer]

OK -- this repo needs some infra updates especially around releases. I'm going to uplift that to try and match current react-live alongside adding changesets and get this thing released... Thanks all!

[jimthedev]

Great, thanks Ryan.