Skip to content

[diffs/edit] fix editor state persisting - #1048

Open
ije wants to merge 10 commits into
mainfrom
ije/fix-state-persisting
Open

[diffs/edit] fix editor state persisting#1048
ije wants to merge 10 commits into
mainfrom
ije/fix-state-persisting

Conversation

@ije

@ije ije commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator
  • add sharedEditor prop for EditProvider

    function EditorApp()  {
      const editor = useMemo(
        () => new Editor<undefined>({ persistState: true, onChange: handleChange }),
        [handleChange]
      );
      
      return (
        <EditProvider sharedEditor={editor}>
          <File file={activeFile} edit />
        </EditProvider>
      );
    }
  • fix state persisting ignore scroll top position with persistState option set to true.

    Screen.Recording.2026-08-04.at.01.37.51.mov
  • cache editor instances by editorOptions

  • re-structure edit docs page

@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
pierre-docs-diffs Ready Ready Preview Aug 3, 2026 6:06pm
pierre-docs-diffshub Ready Ready Preview Aug 3, 2026 6:06pm
pierre-docs-trees Ready Ready Preview Aug 3, 2026 6:06pm
pierrejs-diff-demo Ready Ready Preview Aug 3, 2026 6:06pm

Request Review

@ije
ije requested review from amadeus and necolas August 3, 2026 17:42

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: de9a0cad7a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +943 to +946
const persistedCacheKey =
this.#options.persistState === true
? requirePersistedCacheKey(fileOrDiff)
: undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid reusing cached documents for recreated diffs

When persistState is true for a FileDiff, this now derives a persisted cache key and allows #getCachedTextDocument to win for diffs as well as files. If a React surface recreates FileDiffMetadata from the original before/after after an edit (same cacheKey) instead of reusing the mutated metadata object, FileDiff.attachEditor has no __prepareFile-style rerender path, so the editor's TextDocument/API and the already-rendered diff DOM diverge and the next edit applies against hidden cached text. Restrict cached-document reuse to File or add a diff prepare/rerender path.

Useful? React with 👍 / 👎.

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.

fixed in 9b2aff5

Comment thread packages/diffs/src/editor/editor.ts Outdated
Comment on lines +1288 to +1291
// scrollTop is deliberately not part of this staleness check: the
// surface can legitimately adjust vertical scroll while an async read
// is in flight (height reconciliation, clamping), and that must not
// block the restore.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep async restores from overriding user scroll

With async persistStateStorage (including indexedDB), a user can scroll the editor before get(cacheKey) resolves. Because the stale-restore guard compares scrollLeft but deliberately ignores the newly persisted scrollTop, the delayed restore can still call #setViewportScrollTop and yank the viewport back to the old stored position; please either include vertical scroll in the staleness signal after layout reconciliation or distinguish internal layout adjustments from user scrolls.

Useful? React with 👍 / 👎.

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.

Added user scroll detection during async state restoration in 6558158

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.

1 participant