feat(console): land shared editor model seam - #159
Conversation
Move the editor.model contract into a reusable store so CM6 views share serialized Rust-model revisions while preserving the existing CodeFileView API and honest unavailable state.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
📝 WalkthroughWalkthroughChangesEditor model document store
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentStore
participant BlockHost
participant editor.model
DocumentStore->>BlockHost: invoke editor.model operation
BlockHost->>editor.model: submit document and revision data
editor.model-->>BlockHost: return receipt or failure
BlockHost-->>DocumentStore: return model result
DocumentStore-->>DocumentStore: update snapshot and notify listeners
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR extracts the editor.model contract helpers from CodeFileView into a reusable Console module, and introduces a per-host DocumentStore to coordinate model-backed document edits/history while keeping existing CodeFileView exports stable.
Changes:
- Moved
editor.modelhelper types/functions intosrc/editor-model/index.tsand re-exported them fromCodeFileView.tsxfor compatibility. - Added
DocumentStore(with tests) to serialize model revision updates, notify subscribers, and support undo/redo. - Reordered the Console
testscript so extra args land on Vitest while still running the Railway env test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/console/src/views/CodeFileView.tsx | Removes in-file editor.model helper implementations and re-exports them from the new shared module. |
| apps/console/src/editor-model/index.ts | Adds shared editor.model helpers plus the new per-host DocumentStore implementation. |
| apps/console/src/editor-model/document-store.test.ts | Adds Vitest coverage for DocumentStore edit serialization and undo/redo behavior. |
| apps/console/package.json | Reorders test script command sequence to ensure forwarded args reach Vitest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async history(fileId: string, action: 'undo' | 'redo'): Promise<DocumentSnapshot> { | ||
| const entry = this.require(fileId); | ||
| await entry.syncChain; | ||
| if (!entry.available) return this.toSnapshot(fileId, entry); | ||
|
|
||
| entry.pendingCommands += 1; | ||
| this.emit(fileId, entry); | ||
| const state = await this.invoke( | ||
| historyEditorModelArgs(action, fileId, entry.revision), | ||
| fileId, | ||
| ); | ||
| const current = this.require(fileId); | ||
| current.pendingCommands = Math.max(0, current.pendingCommands - 1); | ||
| if (state) { | ||
| const changed = state.document !== current.document; | ||
| this.applyAuthoritative(current, state, true); | ||
| if (changed) current.generation += 1; | ||
| } else { | ||
| current.available = false; | ||
| current.reason ??= 'editor_model_transport_failed'; | ||
| } | ||
| this.emit(fileId, current); | ||
| return this.toSnapshot(fileId, current); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/console/src/editor-model/index.ts`:
- Around line 198-230: Chain the backend invokes in open() and history() through
each entry’s syncChain, matching dispatch()’s serialization pattern. Assign each
invoke promise to entry.syncChain so close() and subsequent operations await
completion of pending open/history work while preserving the existing state
updates and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c56cfbb2-d0af-4bcc-923e-77958593b94a
📒 Files selected for processing (4)
apps/console/package.jsonapps/console/src/editor-model/document-store.test.tsapps/console/src/editor-model/index.tsapps/console/src/views/CodeFileView.tsx
| async open(fileId: string, text: string): Promise<DocumentSnapshot> { | ||
| const existing = this.files.get(fileId); | ||
| if (existing) return this.toSnapshot(fileId, existing); | ||
|
|
||
| const entry: FileEntry = { | ||
| document: text, | ||
| generation: 0, | ||
| revision: 0, | ||
| available: false, | ||
| canUndo: false, | ||
| canRedo: false, | ||
| pendingCommands: 1, | ||
| listeners: new Set(), | ||
| syncChain: Promise.resolve(), | ||
| }; | ||
| this.files.set(fileId, entry); | ||
| this.emit(fileId, entry); | ||
|
|
||
| const state = await this.invoke(openEditorModelArgs(fileId, text), fileId); | ||
| const current = this.files.get(fileId); | ||
| if (!current) { | ||
| return this.toSnapshot(fileId, entry); | ||
| } | ||
| current.pendingCommands = 0; | ||
| if (state) { | ||
| this.applyAuthoritative(current, state, true); | ||
| } else { | ||
| current.available = false; | ||
| current.reason ??= 'editor_model_transport_failed'; | ||
| } | ||
| this.emit(fileId, current); | ||
| return this.toSnapshot(fileId, current); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Chain open() and history() invokes into entry.syncChain.
dispatch() correctly appends its network call to entry.syncChain (Line 259: entry.syncChain = entry.syncChain.then(...)). open() (Line 216) and history() (Line 297) do not. This breaks the class's stated invariant: "Local edits notify subscribers immediately, then serialize through the authoritative Rust revision so multiple CM6 views cannot race the same base revision."
Concrete races:
- If
dispatch()is called whilehistory()'s invoke is in flight, the synchronousentry.availablecheck at Line 243 still passes (it is set false only after the in-flight call resolves), sodispatch()chains onto the stale, already-resolvedentry.syncChainand fires its edit concurrently with the pending undo/redo call against the sameentry.revision. close()(Line 318) only awaitsentry.syncChain. Sinceopen()'s invoke is never added to that chain, a quick open-then-close sequence letsclose()delete the local entry and send theclosecommand to the backend while theopencommand is still in flight, risking a leaked or out-of-order model on the Rust side.
Chain both operations the same way dispatch() does, so close() and any subsequent dispatch()/history() truly wait for all prior backend work.
🔒 Proposed fix to chain `history()` into `entry.syncChain`
async history(fileId: string, action: 'undo' | 'redo'): Promise<DocumentSnapshot> {
const entry = this.require(fileId);
await entry.syncChain;
if (!entry.available) return this.toSnapshot(fileId, entry);
entry.pendingCommands += 1;
this.emit(fileId, entry);
- const state = await this.invoke(
- historyEditorModelArgs(action, fileId, entry.revision),
- fileId,
- );
- const current = this.require(fileId);
- current.pendingCommands = Math.max(0, current.pendingCommands - 1);
- if (state) {
- const changed = state.document !== current.document;
- this.applyAuthoritative(current, state, true);
- if (changed) current.generation += 1;
- } else {
- current.available = false;
- current.reason ??= 'editor_model_transport_failed';
- }
- this.emit(fileId, current);
- return this.toSnapshot(fileId, current);
+ const run = this.invoke(
+ historyEditorModelArgs(action, fileId, entry.revision),
+ fileId,
+ ).then((state) => {
+ const current = this.require(fileId);
+ current.pendingCommands = Math.max(0, current.pendingCommands - 1);
+ if (state) {
+ const changed = state.document !== current.document;
+ this.applyAuthoritative(current, state, true);
+ if (changed) current.generation += 1;
+ } else {
+ current.available = false;
+ current.reason ??= 'editor_model_transport_failed';
+ }
+ this.emit(fileId, current);
+ });
+ entry.syncChain = run.catch(() => {});
+ await run;
+ return this.toSnapshot(fileId, this.require(fileId));
}Apply the same pattern to open()'s invoke call so close() cannot race ahead of a pending open.
Also applies to: 290-313, 315-329
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/console/src/editor-model/index.ts` around lines 198 - 230, Chain the
backend invokes in open() and history() through each entry’s syncChain, matching
dispatch()’s serialization pattern. Assign each invoke promise to
entry.syncChain so close() and subsequent operations await completion of pending
open/history work while preserving the existing state updates and return
behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de94a3955
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| entry.syncChain = entry.syncChain.then(async () => { | ||
| const current = this.files.get(fileId); | ||
| if (!current) return; | ||
| const state = await this.invoke( | ||
| editEditorModelArgs(fileId, current.revision, modelEdits), | ||
| fileId, |
There was a problem hiding this comment.
When multiple edits are queued and the first model invocation fails, that chain step marks the entry unavailable, but the next callback still invokes the model with offsets captured from the optimistic document containing the failed edit. If transport recovers, the second edit can be applied at the wrong position in the unchanged Rust document and its response is then accepted as authoritative. Abort remaining queued edits, or rebase them, after any predecessor fails.
Useful? React with 👍 / 👎.
| async history(fileId: string, action: 'undo' | 'redo'): Promise<DocumentSnapshot> { | ||
| const entry = this.require(fileId); | ||
| await entry.syncChain; | ||
| if (!entry.available) return this.toSnapshot(fileId, entry); |
There was a problem hiding this comment.
When two views invoke undo or redo together, or an edit is dispatched while a history request is in flight, each operation can pass this await and send the same base revision because history() never appends its own request to syncChain. The backend must reject or lose one of those user operations, and responses can reconcile in a different order from invocation. Add history work to the same per-file chain used by edits.
Useful? React with 👍 / 👎.
| async close(fileId: string): Promise<void> { | ||
| const entry = this.files.get(fileId); | ||
| if (!entry) return; | ||
| await entry.syncChain; | ||
| this.files.delete(fileId); |
There was a problem hiding this comment.
When two CM6 views have opened the same file through this shared store, either view calling close() immediately deletes the sole entry and closes the backend model even though the other view may still be subscribed. The remaining view then throws document_store_not_open on its next dispatch or history action. Track per-file acquisitions/subscribers and only delete and close the model after the final view releases it.
Useful? React with 👍 / 👎.
| async open(fileId: string, text: string): Promise<DocumentSnapshot> { | ||
| const existing = this.files.get(fileId); | ||
| if (existing) return this.toSnapshot(fileId, existing); |
There was a problem hiding this comment.
When a second view calls open() while the first model request is still in flight, this early return resolves immediately with an unavailable, pending snapshot instead of awaiting the shared request; if the first request fails transiently, every later open() also returns that failed entry without contacting the recovered backend. Callers therefore cannot rely on an awaited open() having completed, and the file can remain permanently unavailable without an explicit close. Reuse the in-flight promise and retry entries that are no longer pending but unavailable.
Useful? React with 👍 / 👎.
| } else if (!state.available) { | ||
| this.applyAuthoritative(latest, state, false); |
There was a problem hiding this comment.
When the model rejects a single edit with available: false but includes its authoritative document, this branch deliberately avoids replacing the optimistic local text. Subscribers therefore continue displaying an edit that the Rust model did not accept, and that apparent change disappears when the file is reopened. Apply the returned document when present, while retaining the optimistic text only for failures that provide no authoritative state.
Useful? React with 👍 / 👎.
Summary
editor.modelcontract helpers into a reusable Console module while preserving the existingCodeFileViewexportsDocumentStorethat serializes Rust-model revisions, synchronizes subscribers, round-trips undo/redo, and reports unavailable state honestlyTest plan
npm --prefix apps/console run test -- --run document-store CodeFileView.modelnpx eslint src/editor-model/index.ts src/editor-model/document-store.test.ts src/views/CodeFileView.tsx src/views/CodeFileView.model.test.tsxnpm run gate:canonical-root && npm run gate:fenceSummary by CodeRabbit
New Features
Tests