Refresh working tree diff after turns - #3906
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
the
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces a new diff preview refresh mechanism that changes runtime behavior. Multiple unresolved review comments identify potential cache invalidation gaps where diffs may not refresh correctly (e.g., HEAD changes, content changes without status changes, remote ref updates). These design concerns warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![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: 68f23c90ce
ℹ️ 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".
| const diffPreviewCacheKey = JSON.stringify({ | ||
| latestTurnId: latestTurn?.turnId ?? null, | ||
| workingTree: gitStatusQuery.data?.workingTree ?? null, | ||
| }); |
There was a problem hiding this comment.
When the current ref changes while the working tree summary is unchanged, this cache key does not change even though getReviewDiffPreview recomputes the branch-range diff from the current HEAD/branch. For example, switching between two clean branches leaves workingTree as an empty summary and latestTurnId unchanged, so the branch diff panel can keep reusing the previous ${baseRef}...HEAD preview. Include ref/HEAD-relevant status (or a VCS generation) in the cache key, not just the working-tree summary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 601d479ebe9a522c2bdff2ba0ba3f3e094b19044.
| const diffPreviewCacheKey = JSON.stringify({ | ||
| latestTurnId: latestTurn?.turnId ?? null, | ||
| workingTree: gitStatusQuery.data?.workingTree ?? null, | ||
| }); |
There was a problem hiding this comment.
When an already-listed changed file is edited without changing its path or numstat summary, this key stays identical while the patch returned by getReviewDiffPreview changes. This is especially easy for untracked files because the status summary records them as the same path with 0 insertions/deletions, so editing the file contents after the preview is cached leaves the Working tree panel rendering the old patch until something else changes the key. Use a content/diff generation or refreshed diff hash rather than only the lossy workingTree summary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 601d479ebe9a522c2bdff2ba0ba3f3e094b19044.
Dismissing prior approval to re-evaluate 601d479
github-actions
Bot
added
size:M
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 601d479ebe
ℹ️ 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".
| const latestTurn = orderedTurnDiffSummaries[0]; | ||
| const diffPreviewCacheKey = JSON.stringify({ | ||
| latestTurnId: latestTurn?.turnId ?? null, | ||
| localGeneration: gitStatusQuery.data?.localGeneration ?? null, |
There was a problem hiding this comment.
When the branch preview is based on a remote ref, a fetch can move that base ref while the working tree and HEAD stay unchanged; VcsStatusBroadcaster.updateCachedRemoteStatus publishes only remoteUpdated, and applyGitStatusStreamEvent preserves the old localGeneration, so this cache key stays identical even though getReviewDiffPreview recomputes ${baseRef}...HEAD against the new ref. Include a remote generation/fingerprint in the key for branch-range previews so the panel refreshes after remote status updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in fe250edfc44c7333230992e613175367d91442d5.
github-actions
Bot
added
size:L
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d83415d7d3
ℹ️ 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".
| const remoteRefs = yield* runGitStdout("GitVcsDriver.statusDetailsRemote.remoteRefs", cwd, [ | ||
| "for-each-ref", | ||
| "--sort=refname", | ||
| "--format=%(refname)%00%(objectname)", | ||
| "refs/remotes", | ||
| ]); |
There was a problem hiding this comment.
In repositories with many remote-tracking refs, this new for-each-ref refs/remotes output can exceed the default 1 MB stdout limit enforced by runGitStdout/executeGit; when that happens collectOutput fails with a GitCommandError, so every remote status refresh fails and the Git UI loses status even though the repository is otherwise healthy. Hash the ref stream/bounded chunks, or otherwise avoid materializing all remote refs under the default cap, before putting this on the status path.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 87b2fd2. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d525d2feee
ℹ️ 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".
| const remoteRefs = yield* runGitStdout("GitVcsDriver.statusDetailsRemote.remoteRefs", cwd, [ | ||
| "for-each-ref", | ||
| "--sort=refname", | ||
| "--format=%(refname)%00%(objectname)", | ||
| "refs/remotes", | ||
| ]); |
There was a problem hiding this comment.
On repositories with many remote-tracking refs, this new for-each-ref refs/remotes output is read through runGitStdout, which uses the default 1,000,000-byte output cap and turns overflow into a GitCommandError rather than truncating. A large branch namespace can therefore make statusDetailsRemote fail just from opening or refreshing VCS status, whereas the previous path did not enumerate every remote ref; hash the refs while streaming or otherwise handle large output so remote status still loads.
Useful? React with 👍 / 👎.
b7f47e9 to
3ada610
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ada6109f6
ℹ️ 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".
| if (current !== null) { | ||
| const registry = yield* AtomRegistry.AtomRegistry; | ||
| invalidateReviewDiffPreviews(registry, { environmentId }); |
There was a problem hiding this comment.
When a later turn edits an already-dirty file without changing its path or insertion/deletion counts, this invalidation never runs: CheckpointReactor refreshes local status after the turn, but VcsStatusBroadcaster.updateCachedLocalStatus publishes localUpdated only when the JSON fingerprint of the summary changes. The patch returned by review.getDiffPreview can therefore change while the mounted query keeps its old result indefinitely. Fresh evidence in this revision is that preview freshness now depends exclusively on receiving these status events, so invalidate after every checkpoint refresh or propagate a content generation even when the summary is identical.
Useful? React with 👍 / 👎.
| const registry = yield* AtomRegistry.AtomRegistry; | ||
| invalidateReviewDiffPreviews(registry, { environmentId }); |
There was a problem hiding this comment.
When the automatic status fetch moves a selected remote base such as origin/release without changing the current branch's divergence or PR fields, no remoteUpdated event is published and this invalidation is skipped, although ${baseRef}...HEAD now produces a different preview. The server invokes git fetch ... <remote> without a refspec (git fetch -h documents the repository-only form as git fetch [<repository> [<refspec>...]]), while VcsStatusBroadcaster fingerprints only VcsStatusRemoteResult, which contains no remote-ref generation. Fresh evidence in this revision is that the new event-driven cache signal has no other path for automatic remote fetches; propagate a ref generation/hash or invalidate after the fetch.
Useful? React with 👍 / 👎.
| readonly environmentId: EnvironmentId; | ||
| } | ||
|
|
||
| const diffPreviewRevisionByEnvironment = Atom.family((environmentId: EnvironmentId) => |
There was a problem hiding this comment.
When one environment contains multiple projects or worktrees, every post-snapshot status event increments this single environment-wide revision, so an event from any visible sidebar thread forces the active diff preview for an unrelated cwd to execute another potentially large Git diff and send it over the WebSocket. Web and mobile both mount additional status streams outside their review panels, making parallel agent activity in one environment a practical source of repeated unrelated preview requests; key the revision by normalized repository/CWD (and invalidate only matching preview inputs) instead.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
1 participant
What changed
Why
The Working tree diff preview could reuse a cached response after a later agent turn changed the repository. This was especially visible when a file created in one turn was deleted in the next: Git correctly omitted the deleted untracked file from a fresh preview, but the UI could continue rendering the earlier cached patch.
The earlier implementation solved this with
localGeneration, aremoteRefHashscan, and unconditional local-status publication. That conflicts with the resource-conscious ref architecture now onmain, which deduplicates status events and refreshes ref-backed data through explicit invalidation after mutations.This rewrite follows that architecture: actual status changes and settled ref mutations bump a lightweight client revision, and existing diff-preview atoms refresh reactively from that signal. There is no remote-ref hash subprocess, polling loop, always-publish behavior, or client-only cache-key churn.
Impact
Working tree and branch diff views refresh after relevant repository changes while retaining fingerprint deduplication and the resource bounds introduced by #4727 and #2679.
Validation
vp test run packages/client-runtime/src/state/runtime.test.ts packages/client-runtime/src/state/review.test.ts packages/client-runtime/src/state/vcs.test.ts packages/client-runtime/src/state/vcsAction.test.ts packages/client-runtime/src/state/sourceControl.test.ts(47 passed)vp fmt --checkandvp lintfor the six changed filesvp run --filter @t3tools/client-runtime typecheckNote
Medium Risk
Touches shared query atom wiring and VCS status streaming; extra refetches on status updates could increase RPC load but behavior is scoped per environment with tests.
Overview
Fixes stale working tree diff preview by re-running the cached
reviewGetDiffPreviewquery when Git state changes, instead of relying only on SWRstaleTime.Adds a per-environment
reviewDiffPreviewRevisionAtomandinvalidateReviewDiffPreviews, and wiresdiffPreviewto a new optionalrefreshSignalon environment RPC query atoms so subscribing to the revision triggers a refetch without changing the RPC payload.Invalidation runs when VCS status updates after the initial snapshot (
projectVcsStatusChanges/ refactoredstatusstream) and when cached refs are cleared (invalidateCachedVcsRefs). Tests cover environment scoping, reactive dependents, status-driven bumps, and ref-invalidation paths.Reviewed by Cursor Bugbot for commit 3ada610. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Refresh working tree diff previews after VCS status changes and ref invalidations
diffPreviewRevisionByEnvironment) in review.ts and aninvalidateReviewDiffPreviewsfunction that increments it.refreshSignaloption increateEnvironmentQueryAtomFamily.invalidateReviewDiffPreviewsfrom vcs.ts on every VCS status change after the initial snapshot, and from vcsRefInvalidation.ts when cached VCS refs are cleared.Macroscope summarized 3ada610.