Fix: Success-path stale check still clobbers loading flag on race (loadSymbols/loadAllFiles/loadRepoFiles) - #35
Open
pullapprove5[bot] wants to merge 1 commit into
Open
Conversation
loadAllFiles/loadRepoFiles (filesSlice) and loadSymbols (symbolsSlice)
detected a stale response on the success path but still did
`set({ ...Loading: false })` before returning. If comparison/repo A's
request resolved after a switch to B — whose own load had already
claimed the flag — A's handler reset it to false, so the UI reported
loading complete while B's request was still in flight.
Return without touching state instead, matching loadRepoSymbols. The
switch itself resets the flags (comparisonResetState/repoResetState), so
nothing gets stuck true. Same one-line change on loadSymbols' catch
path, which had the identical clobber.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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 was wrong
Three async loaders detected a stale response on their success path but still wrote to the store before bailing:
desktop/ui/stores/slices/filesSlice.ts—loadAllFiles,loadRepoFiles:if (isStale()) { set({ allFilesLoading: false }); return; }desktop/ui/stores/slices/symbolsSlice.ts—loadSymbols: same shape withsymbolsLoadingIf comparison/repo A's request resolves after the user switched to B — and B's own load has already set the loading flag to
true— A's handler resets that flag tofalse. The UI then renders "loading complete" (empty file panel / no symbols) while B's request is genuinely still in flight. This is the same race class fixed on the catch path in PR #18; these success paths were left untouched by that scoped PR.loadRepoSymbolsin the same file already has the clean idiom: a bareif (isStale()) return;.What changed
Each stale branch now returns without calling
set()at all, so a newer in-flight request keeps ownership of its own loading flag. I also applied the identical one-line change toloadSymbols' catch path, which had the same clobber a few lines below (the catch-path fix that PR #18 made infilesSlicenever landed here).Safety check on "could the flag get stuck
true?": every mutation ofcomparison/repoPathgoes throughsetRepoPath/setComparison/setCommitRange/switchReview, and all four spread a reset (comparisonResetState/diffDataResetState/repoResetState) that includesallFilesLoading: falseandsymbolsLoading: false. The switch itself clears the flags, so dropping the stale writer'sset()cannot strandloadSymbolsbehind itsif (symbolsLoading) returnguard.Tests
Added three cases mirroring the existing rejection-race tests:
filesSlice.test.ts— "discards a response that resolves after the comparison changed" (loadAllFiles) and "…after the repo changed" (loadRepoFiles): each asserts the stale payload isn't applied toallFilesand thatallFilesLoadingstaystrue.symbolsSlice.test.ts— same forloadSymbols' success path; also extended the existing rejection test to assertsymbolsLoadingstaystrue, which pins the catch-path half.All three new assertions on the loading flag fail against the old code.
Not verified
I could not run
scripts/test,vitest, ortsc: this sandbox has nonode_modulesand no network access to install dependencies. The change is TypeScript-only (no Rust touched), so the type check and the frontend test run should be exercised in CI or locally before merge.Opened by a PullApprove implementation run (implement-finding v1) for the finding PA-8 — Success-path stale check still clobbers loading flag on race (loadSymbols/loadAllFiles/loadRepoFiles).
Merging this is what closes the finding as fixed.