Repository navigation
fix(editor): load file diffs without waiting for background Git work - #3386
Conversation
Bound snapshot reads separately from repository status scans. Show loading and failure states while the existing diff editor waits for its content.
 [Medium impact] The PR appears safe to merge, with a non-blocking gap in the notice shown during retries. Findings
Fix with agent prompt### Issue 1
apps/emdash-desktop/src/core/features/editor/contributions/browser/monaco/sticky-diff-editor.tsx:99-100
Retrying a failed Git side sets its status to `loading`. For an already loaded diff, `diffNotice` then removes the banner because there is no error. But the editor keeps `readOnly` enabled and the save command still returns early. Users see a diff that looks ready but cannot be edited or saved while the retry runs. Keep a loading banner visible until both sides recover.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryFile diffs use a separate queue with at most two reads per repository family, so background Git work no longer holds them up.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
Git[Repository Git work] --> Ordered[Existing ordered queue]
Diff[Diff content request] --> Reads[Separate queue: two reads]
Reads --> OID[Resolve blob OID]
OID --> Content[Read blob content]
Close[Last owner closes] --> Cancel[Cancel queued or active read]
Reviews (2) · Last reviewed commit: "fix(editor): keep a loaded diff read-onl..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Content queries stay cached after their last lease, and a query only aborts its fetch on dispose, so closing a diff left its read running. With at most two content reads per repository family, reads from closed diffs could delay the next diff. Releasing the last owner of an entry with a read in flight now evicts it, which cancels the queued or active read; reopening starts fresh. Move the content-read scheduling notes to the git runtime doc, next to the repository-family ordering they depart from.
The status overlay covered the editor whenever a side was loading or failed, including after a diff was shown. A refresh failure then hid the diff while the modified buffer kept focus, so typing and Cmd+S still changed and saved a buffer the user could not see. Cover the editor only until a diff is first attached. After that, keep the last loaded diff visible, make it read-only, and name the failure in a banner above it; editing resumes when both sides recover. The save command also refuses to save while the editor is read-only.
|
Thanks for this. Moving content reads to their own lane is the right fix. I pushed two commits on top for the review findings:
I also moved the scheduling notes to |
| if (attached) { | ||
| if (!error) return null; |
There was a problem hiding this comment.
Retrying a failed Git side sets its status to loading. For an already loaded diff, diffNotice then removes the banner because there is no error. But the editor keeps readOnly enabled and the save command still returns early. Users see a diff that looks ready but cannot be edited or saved while the retry runs. Keep a loading banner visible until both sides recover.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/emdash-desktop/src/core/features/editor/contributions/browser/monaco/sticky-diff-editor.tsx
Line: 99-100
Comment:
**Retry hides the loading notice**
Retrying a failed Git side sets its status to `loading`. For an already loaded diff, `diffNotice` then removes the banner because there is no error. But the editor keeps `readOnly` enabled and the save command still returns early. Users see a diff that looks ready but cannot be edited or saved while the retry runs. Keep a loading banner visible until both sides recover.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid. Fixed in 7c373de: while a side reloads, an attached diff now shows a "Reloading diff..." banner, and stays read-only until both sides recover. The browser test covers error → retry → ready.
Retrying a failed side moves it back to loading. The notice only covered errors for an attached diff, so the banner vanished while the editor stayed read-only, and the diff looked ready but ignored edits and Cmd+S. Show a reloading banner until both sides recover.
jschwxrz
left a comment
jschwxrz
left a comment
There was a problem hiding this comment.
Content reads now run in their own lane with real cancellation, and the diff editor covers the editor only until a diff is first shown; after that, failures and reloads keep the diff visible and read-only under a banner. CI is green on top of the git 2.56 test fix (#3388). Thanks!
Description
Changed-file diffs now load while background Git scans run. The editor shows a loading state or an error instead of a blank pane.
Git snapshots use a separate queue with at most two reads per repository family. Each read resolves a blob OID before loading its content. Mutations keep their existing ordering and object-store locks. Closing a content query cancels its queued or active read.
The Monaco editor stays mounted while content loads or recovers. Missing sides still render as empty content for added and deleted files.
Related issues
Fixes #3382.
Testing
pnpm run formatandpnpm run format:checkpnpm run lintpnpm run typecheckEMDASH_TEST_SKIP_BROWSER=1 pnpm run test -- --maxWorkers=4sticky-diff-editor.browser.test.tsxin ChromiumOn base, a held background job prevented content reads from starting. The scheduling regression failed with this assertion.
The browser regression failed with this assertion on base.
The tests now cover reads during a blocked background job, bounded concurrency, cancellation, and repository disposal. Browser coverage checks errors on either side, recovery, and empty sides for added or deleted files.
The non-browser workspace run passes, including 4,869 desktop tests. All 11 focused Monaco Chromium tests also pass.
The broad desktop browser suite crashes in the Playwright route handler on both this branch and unchanged base. The same command produces this runner error.
Screenshot/Recording (if applicable)
The same Chromium fixture at 900 × 500, while the original side loads:
Checklist