Repository navigation
fix(dashboard): discard superseded workspace reads (RACE-1) - #19
Merged
Merged
Conversation
WorkspaceDrawer awaited a file read and then applied it unconditionally, so a slow read could land after a fast one and overwrite the viewer: click file A, then file B, and A's contents paint under B's name. Three sites had this shape - activateSelection, handleEdit, and the create-file branch - and the file had no generation counter of any kind. createReadGuard issues a monotonic ticket; only the newest ticket may apply. runExclusive was not reusable here: it *blocks* concurrent calls, which is right for double-submit (PR #14) and wrong for reads, where every request should proceed and only the latest result may win. In activateSelection the ticket is taken BEFORE the media/non-text early returns. Otherwise selecting an image would not invalidate an in-flight text read, and that text would land on the image viewer - the same bug through a different door. The isCurrent checks guard the error path and the loading flag too, not just setContent: a superseded read must not surface its failure as if it were the current file's, and its finally must not clear a spinner the newer read owns. Extracted as a plain function rather than a hook for the same reason runExclusive was: WorkspaceDrawer is ~1500 lines and imports pdfjs at module scope, which throws DOMMatrix in jsdom, so mounting it in a test hangs rather than fails. readGuard.test.ts drives the ordering directly. Verified: tsc -b clean; eslint 0 errors (the 3 exhaustive-deps warnings on this file pre-exist - confirmed against a stashed tree); 6 new cases in readGuard.test.ts; 20/20 passing across src/pages/Agent/Workspace. The decisive check: reproducing the pre-fix code in isolation - reads with no isCurrent guard, slow A then fast B - fails with "expected 'A' to be 'B'". That is the bug, and the test catches exactly it.
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.
RACE-1 — stale workspace reads overwrite the viewer
A real correctness bug, not a polish item.
WorkspaceDrawerawaited a file read and applied it unconditionally, so a slow read could land after a fast one:Three sites had this shape —
activateSelection,handleEdit, and the create-file branch — and the file had no generation counter of any kind.The fix
createReadGuard(readGuard.ts) issues a monotonic ticket; only the newest ticket may apply.Why not reuse
runExclusive: it blocks concurrent calls. That's right for double-submit (PR #14) and wrong for reads — every request should proceed, and only the latest result may win.Two subtleties worth reviewing
The ticket is taken before the early returns.
activateSelectionreturns early for images and non-text files. Taking the ticket after those returns would mean selecting an image does not invalidate an in-flight text read — and that text would land on the image viewer. Same bug, different door.The guard covers the error path and the loading flag, not just
setContent. A superseded read must not surface its failure as if it were the current file's, and itsfinallymust not clear a spinner the newer read owns.Verification
tsc -bclean;eslint0 errors (3exhaustive-depswarnings on this file pre-exist — confirmed against a stashed tree)readGuard.test.ts; 20/20 passing acrosssrc/pages/Agent/Workspaceexpected 'A' to be 'B'. That is precisely the bug, and the test catches exactly it.Why a plain function, not a hook
Same reason as
runExclusive:WorkspaceDraweris ~1500 lines and importspdfjsat module scope, which throwsDOMMatrixin jsdom — mounting it in a test hangs rather than fails. The guard is driven directly instead.