[diffs/edit] Add persisted editor state caching#976
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41c0dc7bc6
ℹ️ 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".
| onAttach(nextEditor) { | ||
| handleEditorAttachRef.current(nextEditor); | ||
| }, | ||
| persistState: true, |
There was a problem hiding this comment.
When TreeApp callers pass files without cacheKey and two paths share the same FileContents.name (for example src/index.ts and test/index.ts, which was safe with the old path-keyed editorStateByPathRef), enabling editor persistence here makes the editor fall back to file.name for its TextDocument cache. Switching tabs then reuses the first file's cached document for the second file, so the editor can display and edit the wrong buffer. Since TreeApp already has the unique activePath, seed a path-based cache key before rendering or keep path-keyed persistence for this component.
Useful? React with 👍 / 👎.
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53514d3591
ℹ️ 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".
| ); | ||
| } | ||
|
|
||
| file = this.editor?.__prepareFile?.(file) ?? file; |
There was a problem hiding this comment.
When this hook substitutes cached persisted contents, it runs too late for VirtualizedFile: VirtualizedFile.render has already assigned this.file, computed the approximate height, and derived renderRange before delegating to super.render. If a persisted edit changed the line count or wrapped height and the virtualized surface remounts with the original file.contents (e.g. tab/theme switch), the virtualizer sizes and scroll range are based on stale contents while the DOM is rendered from the cached document, so restoring view.scrollTop can clamp to the old height or render the wrong window. The virtualized path needs to prepare the file before its layout calculations.
Useful? React with 👍 / 👎.
| if (file.cacheKey === undefined) { | ||
| file.cacheKey = file.name; | ||
| } | ||
| file.cacheKey ??= file.name; |
There was a problem hiding this comment.
cache key was meant as something that should be mostly controlled by the user because they know their architecture best and how to control uniqueness. This could cause subtle problems problems down stream if we are assigning based on file name here. I.E. what if they are looking at the same file but from different commit histories, and if they don't assign a cache key, then we could be clobbering things pretty signficantly.
Maybe we make it so that editor ONLY works if files have a cacheKey? Similar to how we only cache worker pool results if we have a cacheKey.
| * `contents` or `name`, you must update the `cacheKey`. */ | ||
| /** | ||
| * Identifies a file revision for worker highlighting and persisted editor | ||
| * state. Editor persistence falls back to `name` when this is omitted. |
There was a problem hiding this comment.
Related to my other comment. I don't think name fallback is safe, we should just make cacheKey a required flag if you want to edit i think?
|
@codex review again! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbad541df7
ℹ️ 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".
| function isLineCacheForFile(lineCache: LineCache, file: FileContents): boolean { | ||
| return file.cacheKey == null | ||
| ? lineCache.file === file | ||
| : lineCache.cacheKey === file.cacheKey; |
There was a problem hiding this comment.
When a caller renders an unkeyed FileContents object, then mutates that same object’s contents and forces another render, this identity-only match reuses the old split-line cache. Before this change unkeyed files were re-split on every render, so the rerender picked up the new text; now line counts, virtualization height, and rendered rows can stay stale for cache-key-less files even though cacheKey is documented as optional for read-only rendering.
Useful? React with 👍 / 👎.
Adds opt-in editor state and
TextDocumentpersistence across file renders.When persistence is enabled, the editor uses
file.cacheKey ?? file.nameto identify files. Callingfile.render({ file: newFile })stores the current editor state and restores the cached document, selection, scroll position, and undo history when that file is rendered again.Persistence remains disabled by default.
IndexedDB can be used for state that survives page reloads:
Custom storage implementations are also supported: