fix(review): reload patches when revision or comparison scope changes - #1741
Merged
Merged
Conversation
ReviewDiffView keyed its local patch map by file path only, so a cacheVersion bump or comparison/source/thread change left stale diff content rendered under new scope metadata, and obsolete in-flight responses could write into the current scope or poison the shared inline diff cache for a later remount. Tag the patch map with the thread/source/id/cacheVersion scope and reset it when the scope changes, drop async results that arrive for an obsolete scope, suppress duplicate fetches for paths already in flight, and include cacheVersion in the shared inline cache key so a stale response can never land under a key the new scope reads. Fixes #1732
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ReviewDiffViewkept its component-local patch map keyed by file path only. WhencacheVersion, comparison, source, or thread identity changed, paths already present were skipped by the loader, so stale diff content stayed rendered under new scope metadata. Obsolete in-flight responses could also write into the current scope, and their unconditionalcacheInlineDiffwrites could poison the shared inline cache that a later remount trusted.The patch map is now tagged with the
${threadId}:${source}:${id}:${cacheVersion}scope and reset when the scope changes. Async results apply only while their captured scope is current and the path is still unpopulated, a pending-paths guard suppresses duplicate fetches for files already loading, and the shared inline cache key includescacheVersionso a stale response can never land under a key the new scope reads. Lazy loading and expand/collapse behavior are unchanged.Verified by: regression tests shown failing before the fix and passing after (revision refresh, same-path comparison change, obsolete in-flight response, cache-poisoning remount), 145/145 diff and store tests,
tsc --noEmit, oxlint, plus a live app pass on the fixture workspace: an external edit moved the dirty set,files.changedbumpedcacheVersion, and the rendered patch swappedMARKER_V1toMARKER_V2in place; switching the same file between Unstaged and Staged rendered the correct content in both directions.Why
Fixes #1732. Users could review old code while the UI displayed current revision or comparison metadata.
UI Changes
Live verification captures attached:
MARKER_V1baseline and refreshedMARKER_V2after an external edit plusfiles.changed, then the sameagent.txtrendered under Staged and back under Unstaged with correct content each way.Review Notes
The obsolete in-flight response path is covered by unit test rather than live timing. Live verification ran in the web app rather than the Electron shell.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.