Skip to content

fix(editor): rendered preview stays stale after discarding changes#2433

Open
george-vii wants to merge 2 commits into
generalaction:mainfrom
george-vii:fix/editor-stale-preview-after-discard
Open

fix(editor): rendered preview stays stale after discarding changes#2433
george-vii wants to merge 2 commits into
generalaction:mainfrom
george-vii:fix/editor-stale-preview-after-discard

Conversation

@george-vii

Copy link
Copy Markdown

Bug

After discarding a change to a file open in the editor's rendered preview (markdown/html/svg), the preview keeps showing the discarded content even though the working tree is reverted and the Changes panel is clean. It only refreshes on a tab remount (close + reopen).

Repro

  1. Open a Markdown file in the rendered preview (eye view).
  2. Edit it so it appears under Changes.
  3. Discard the change while the preview is open.
  4. Changes panel goes clean, but the preview still renders the old line.

Root cause

Disk-driven buffer reloads (applyDiskUpdate, reloadFromDisk) update the Monaco buffer model inside a reloadingFromDisk guard. The buffer's onDidChangeContent listener intentionally bails while that guard is set (so a disk reload isn't treated as a user edit) — but that also skips bumping bufferVersions. The rendered preview's only reactive dependency is bufferVersions, so it never re-renders. The Monaco source view updates fine (bound to the model directly), which is why only preview mode is affected and a remount "fixes" it.

Fix

Bump bufferVersions explicitly in both reload paths after applying the reloaded content, decoupled from the dirty/autosave logic the listener guards. Covers discard, watcher-surfaced external edits, branch switches, and the conflict-dialog "Accept Incoming" path.

Testing

  • New regression test: a disk reload updates a clean open buffer and bumps bufferVersions.
  • typecheck, lint, format, and the node test project pass.
  • Verified in-app: discarding a change to an open Markdown preview now updates immediately.

Discarding a change (or any working-tree revert the fs-watcher surfaces)
reloaded the buffer model's content but left an open rendered preview
showing the old content until the tab was remounted.

Disk-driven buffer reloads (applyDiskUpdate, reloadFromDisk) update the
Monaco model inside a reloadingFromDisk guard, which makes the buffer's
change listener bail out and skip bumping bufferVersions. Read-only
reactive consumers (the rendered markdown/html/svg preview) subscribe to
bufferVersions, so they never re-rendered. Both reload paths now bump
bufferVersions after applying the reloaded content.

Adds a regression test: a disk reload updates a clean open buffer and
bumps bufferVersions.
@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a stale rendered preview bug in the Monaco-backed editor: after a discard (or any disk-driven buffer reload), the markdown/html/svg preview kept showing the old content because bufferVersions — the only reactive signal the preview listens to — was never bumped when onDidChangeContent intentionally bailed out during a reloadingFromDisk pass.

  • reloadFromDisk (conflict-dialog "Accept Incoming" path) and applyDiskUpdate (discard / external-edit / branch-switch path) now each explicitly bump bufferVersions inside a runInAction after applying the reloaded content, decoupled from the dirty/autosave logic that was previously silencing it.
  • A new regression test verifies that bufferVersions is incremented when invalidateModel reloads a clean open buffer with different disk content.

Confidence Score: 4/5

Safe to merge — the fix is narrowly scoped to two reload paths, the logic is correct, and the new regression test validates the primary scenario.

The bump logic is correct and well-placed in both reload paths. reloadFromDisk bumps bufferVersions unconditionally regardless of whether content actually changed (unlike applyDiskUpdate which guards on !newMatchesBuffer), and in applyDiskUpdate the version bump and dirty-flag clear are dispatched in two separate MobX transactions, leaving a narrow window where a reactive consumer could observe version-bumped-but-still-dirty state.

monaco-model-registry.ts around the reloadFromDisk method warrants a second look for the unconditional version bump and the split runInAction pattern in applyDiskUpdate.

Important Files Changed

Filename Overview
apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.ts Adds explicit bufferVersions bumps in reloadFromDisk and applyDiskUpdate after disk-driven buffer reloads, fixing stale preview rendering. Minor: reloadFromDisk bumps unconditionally (unlike applyDiskUpdate's content-diff guard), and the new bump in applyDiskUpdate uses a separate runInAction from the existing dirty-clear.
apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.test.ts Adds a regression test for the bufferVersions bump via the invalidateModel→applyDiskUpdate path; the reloadFromDisk path is not covered by the new test.

Sequence Diagram

sequenceDiagram
    participant UI as Discard / Branch Switch
    participant Reg as MonacoModelRegistry
    participant Disk as Disk Model
    participant Buf as Buffer Model
    participant Preview as Preview Renderer

    UI->>Reg: invalidateModel(diskUri)
    Reg->>Disk: readFile() new content
    Reg->>Reg: applyDiskUpdate(diskUri, entry, newContent)
    Reg->>Buf: "applyEdits reloadingFromDisk=true"
    Note over Buf: onDidChangeContent fires but bails
    Reg->>Reg: reloadingFromDisk.delete(bufferUri)
    Reg->>Reg: runInAction bufferVersions++ NEW
    Reg->>Reg: runInAction dirtyUris.delete
    Reg-->>Preview: MobX notifies
    Preview->>Buf: getValue renders new content

    UI->>Reg: reloadFromDisk(uri)
    Reg->>Disk: getValue
    Reg->>Buf: "setValue reloadingFromDisk=true"
    Note over Buf: onDidChangeContent fires but bails
    Reg->>Reg: reloadingFromDisk.delete(uri)
    Reg->>Reg: runInAction dirtyUris.delete + bufferVersions++ NEW
    Reg-->>Preview: MobX notifies
    Preview->>Buf: getValue renders new content
Loading

Comments Outside Diff (3)

  1. apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.ts, line 705-719 (link)

    P2 reloadFromDisk bumps version even when content is unchanged

    setValue(disk.model.getValue()) is called unconditionally, and then bufferVersions is always bumped. If the disk content is already identical to the buffer (e.g. a no-op "Accept Incoming" after an external edit that matches the open buffer), this fires an unnecessary preview re-render. Unlike applyDiskUpdate, which only bumps when !newMatchesBuffer, reloadFromDisk has no such guard. The outcome is correct but slightly inconsistent with the sister function's pattern and could cause spurious re-renders in edge cases.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.ts
    Line: 705-719
    
    Comment:
    **`reloadFromDisk` bumps version even when content is unchanged**
    
    `setValue(disk.model.getValue())` is called unconditionally, and then `bufferVersions` is always bumped. If the disk content is already identical to the buffer (e.g. a no-op "Accept Incoming" after an external edit that matches the open buffer), this fires an unnecessary preview re-render. Unlike `applyDiskUpdate`, which only bumps when `!newMatchesBuffer`, `reloadFromDisk` has no such guard. The outcome is correct but slightly inconsistent with the sister function's pattern and could cause spurious re-renders in edge cases.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.ts, line 841-863 (link)

    P2 Two separate runInAction calls for related observable writes

    The new bufferVersions bump and the existing dirtyUris.delete are dispatched in two separate MobX transactions. Between them, a MobX reaction (e.g. a computed that reads both) could observe an intermediate state where the version is already bumped but the dirty flag is not yet cleared. In the main discard path both observables matter to consumers. Merging them into a single runInAction would atomically update both and remove any window for inconsistent intermediate state.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.ts
    Line: 841-863
    
    Comment:
    **Two separate `runInAction` calls for related observable writes**
    
    The new `bufferVersions` bump and the existing `dirtyUris.delete` are dispatched in two separate MobX transactions. Between them, a MobX reaction (e.g. a `computed` that reads both) could observe an intermediate state where the version is already bumped but the dirty flag is not yet cleared. In the main discard path both observables matter to consumers. Merging them into a single `runInAction` would atomically update both and remove any window for inconsistent intermediate state.
    
    How can I resolve this? If you propose a fix, please make it concise.
  3. apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.test.ts, line 112-155 (link)

    P2 Test covers applyDiskUpdate only; reloadFromDisk is untested

    The new regression test exercises the invalidateModelapplyDiskUpdate path, but the "Accept Incoming" (reloadFromDisk) path — the other place where bufferVersions was patched — has no corresponding test. Adding a parallel test that calls reloadFromDisk directly would close the coverage gap and guard against the same regression re-appearing in the conflict-dialog flow.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.test.ts
    Line: 112-155
    
    Comment:
    **Test covers `applyDiskUpdate` only; `reloadFromDisk` is untested**
    
    The new regression test exercises the `invalidateModel``applyDiskUpdate` path, but the "Accept Incoming" (`reloadFromDisk`) path — the other place where `bufferVersions` was patched — has no corresponding test. Adding a parallel test that calls `reloadFromDisk` directly would close the coverage gap and guard against the same regression re-appearing in the conflict-dialog flow.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.ts:705-719
**`reloadFromDisk` bumps version even when content is unchanged**

`setValue(disk.model.getValue())` is called unconditionally, and then `bufferVersions` is always bumped. If the disk content is already identical to the buffer (e.g. a no-op "Accept Incoming" after an external edit that matches the open buffer), this fires an unnecessary preview re-render. Unlike `applyDiskUpdate`, which only bumps when `!newMatchesBuffer`, `reloadFromDisk` has no such guard. The outcome is correct but slightly inconsistent with the sister function's pattern and could cause spurious re-renders in edge cases.

### Issue 2 of 3
apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.ts:841-863
**Two separate `runInAction` calls for related observable writes**

The new `bufferVersions` bump and the existing `dirtyUris.delete` are dispatched in two separate MobX transactions. Between them, a MobX reaction (e.g. a `computed` that reads both) could observe an intermediate state where the version is already bumped but the dirty flag is not yet cleared. In the main discard path both observables matter to consumers. Merging them into a single `runInAction` would atomically update both and remove any window for inconsistent intermediate state.

### Issue 3 of 3
apps/emdash-desktop/src/renderer/lib/monaco/monaco-model-registry.test.ts:112-155
**Test covers `applyDiskUpdate` only; `reloadFromDisk` is untested**

The new regression test exercises the `invalidateModel``applyDiskUpdate` path, but the "Accept Incoming" (`reloadFromDisk`) path — the other place where `bufferVersions` was patched — has no corresponding test. Adding a parallel test that calls `reloadFromDisk` directly would close the coverage gap and guard against the same regression re-appearing in the conflict-dialog flow.

Reviews (1): Last reviewed commit: "fix(editor): refresh rendered preview af..." | Re-trigger Greptile

@george-vii

Copy link
Copy Markdown
Author

Addressed the review feedback:

  • reloadFromDisk untested — added a regression test for the conflict-dialog path: a dirty buffer plus an external disk change records a conflict, and reloadFromDisk ("Accept Incoming") replaces the buffer content, clears dirty/conflict state, and bumps bufferVersions. Verified the test fails if the reloadFromDisk bump is removed.
  • Unconditional bump in reloadFromDisk — left as-is. This path is only reachable from the conflict dialog, and applyDiskUpdate only records a conflict when buffer and disk content diverge, so a same-content bump is a rare edge (the user manually syncing the buffer before clicking Accept). The only bufferVersions consumers are pure render subscriptions, so the cost would be a single extra re-render.
  • Two runInAction blocks in applyDiskUpdate — left as-is. The version bump and the dirtyUris cleanup sit in different conditional branches, and no observer reads both observables together, so there's no observable intermediate state. Happy to merge them if you'd prefer it stylistically.

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