Skip to content

Commit fa90a42

Browse files
maxmilianclaude
andcommitted
fix(web): re-ask a run-owned open request's owner when the activation runs
Every ownership check so far ran when the request was made. The workspace can park an activation behind an unsettled manual edit for an unbounded time, and the gate that releases it only re-checks the source tab and the activation sequence — neither of which a run or conversation handoff changes. So a request enqueued by run A could still focus A's file after the user started run B or moved to another chat. Requests now carry `isStillOwned`, re-asked at the moment the activation executes. A predicate rather than data because only the requester knows what owning it means: run-owned opens carry their generation and conversation, the settle watch carries its conversation, and a user's click carries nothing — a user's choice cannot go stale by waiting. This closes the class rather than another instance of it: anything that becomes false between enqueue and activation is now caught at activation. Also polls the delayed per-write assertion instead of asserting on the tick after the release. That race made a green guard look like a blocking one under full-suite load, which is an expensive way to be wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LQKCR9FRJiexWWUpcoFLpm
1 parent cc37e27 commit fa90a42

4 files changed

Lines changed: 167 additions & 20 deletions

File tree

apps/web/src/components/FileWorkspace.tsx

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,15 @@ export interface WorkspaceOpenRequest {
212212
name: string;
213213
nonce: number;
214214
source: WorkspaceOpenRequestSource;
215+
// Re-asked at the moment the activation actually runs, not when the request
216+
// is made. An activation can be parked behind an unsettled manual edit for an
217+
// unbounded time (see `afterActiveManualEditSettles`), and the requester's own
218+
// checks have long since passed by then — so whatever made this request the
219+
// right one to honour has to stay askable. Only the requester knows what that
220+
// is, which is why this is a predicate rather than data: 'internal' requests
221+
// carry their run's ownership, and a user's click carries nothing because a
222+
// user's choice cannot go stale.
223+
isStillOwned?: () => boolean;
215224
}
216225

217226
interface Props {
@@ -1605,16 +1614,23 @@ export function FileWorkspace({
16051614
function afterActiveManualEditSettles(
16061615
action: () => void,
16071616
origin: WorkspaceOpenRequestSource = 'user',
1617+
isStillOwned?: () => boolean,
16081618
) {
16091619
// Over-reporting is the safe direction and is deliberate here: this can
16101620
// double-count with the `activeTab` effect when the activation lands
16111621
// promptly. The count is only ever compared for equality, so an extra
16121622
// increment can retire a watch early — never move focus off the user's tab.
16131623
if (origin === 'user') onUserActivateTab?.();
1624+
// The owner is re-checked HERE rather than at the callsite, because this is
1625+
// the only place that knows whether the action ran immediately or waited.
1626+
const run = () => {
1627+
if (isStillOwned && !isStillOwned()) return;
1628+
action();
1629+
};
16141630
const sourceTab = activeTabRef.current;
16151631
const exit = manualEditExitHandlersRef.current.get(sourceTab);
16161632
if (!exit) {
1617-
action();
1633+
run();
16181634
return;
16191635
}
16201636
const sequence = ++requestedActivationSequenceRef.current;
@@ -1624,7 +1640,7 @@ export function FileWorkspace({
16241640
ok
16251641
&& sequence === requestedActivationSequenceRef.current
16261642
&& activeTabRef.current === sourceTab
1627-
) action();
1643+
) run();
16281644
});
16291645
}
16301646

@@ -1869,13 +1885,17 @@ export function FileWorkspace({
18691885
onTabsStateChange(next);
18701886
}
18711887

1872-
function setPersistedActive(name: string | null, origin: WorkspaceOpenRequestSource = 'user') {
1888+
function setPersistedActive(
1889+
name: string | null,
1890+
origin: WorkspaceOpenRequestSource = 'user',
1891+
isStillOwned?: () => boolean,
1892+
) {
18731893
const nextActive = name ?? defaultRootTab;
18741894
if (nextActive === activeTab) return;
18751895
afterActiveManualEditSettles(() => {
18761896
setActiveTab(nextActive);
18771897
commitTabsState(workspaceTabsState(persistedTabs, name));
1878-
}, origin);
1898+
}, origin, isStillOwned);
18791899
}
18801900

18811901
function openRequestedBrowserTab(request: BrowserOpenRequest) {
@@ -2084,6 +2104,7 @@ export function FileWorkspace({
20842104
// classify the user's click as the parent's and leave the settle watcher
20852105
// free to open a higher-ranked artifact over it.
20862106
const origin = openRequest.source;
2107+
const isStillOwned = openRequest.isStillOwned;
20872108
// Still marked for BOTH sources, so the reporter above never fires off the
20882109
// landed activation for a request that came through this prop: a run's own
20892110
// auto-open must not retire the watch that issued it, and a user-sourced one
@@ -2096,16 +2117,16 @@ export function FileWorkspace({
20962117
? DESIGN_FILES_TAB
20972118
: name;
20982119
parentRequestedActivationRef.current = nextActive;
2099-
setPersistedActive(nextActive, origin);
2120+
setPersistedActive(nextActive, origin, isStillOwned);
21002121
return;
21012122
}
21022123
if (isBrowserTabId(name) && browserTabs.some((tab) => tab.id === name)) {
21032124
parentRequestedActivationRef.current = name;
2104-
setPersistedActive(name, origin);
2125+
setPersistedActive(name, origin, isStillOwned);
21052126
return;
21062127
}
21072128
parentRequestedActivationRef.current = name;
2108-
openFile(name, { forcePersist: true, origin });
2129+
openFile(name, { forcePersist: true, origin, isStillOwned });
21092130
// eslint-disable-next-line react-hooks/exhaustive-deps
21102131
}, [openRequest]);
21112132

@@ -2170,7 +2191,11 @@ export function FileWorkspace({
21702191

21712192
function openFile(
21722193
name: string,
2173-
options?: { forcePersist?: boolean; origin?: WorkspaceOpenRequestSource },
2194+
options?: {
2195+
forcePersist?: boolean;
2196+
origin?: WorkspaceOpenRequestSource;
2197+
isStillOwned?: () => boolean;
2198+
},
21742199
) {
21752200
if (name === activeTab) return;
21762201
afterActiveManualEditSettles(() => {
@@ -2190,7 +2215,7 @@ export function FileWorkspace({
21902215
if (nextBrowserTabs !== browserTabs) setBrowserTabs(nextBrowserTabs);
21912216
commitTabsState(workspaceTabsState(nextTabs, name, nextBrowserTabs));
21922217
setActiveTab(name);
2193-
}, options?.origin ?? 'user');
2218+
}, options?.origin ?? 'user', options?.isStillOwned);
21942219
}
21952220
openFileRef.current = openFile;
21962221

apps/web/src/components/ProjectView.tsx

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3561,9 +3561,20 @@ export function ProjectView({
35613561
// gesture or opening on the project's own initiative. Defaulting either way
35623562
// would silently mislabel whichever kind of caller forgot it, and mislabelling
35633563
// a user click is exactly what lets the settle watcher overwrite her choice.
3564-
const requestOpenFile = useCallback((name: string, source: WorkspaceOpenRequestSource) => {
3564+
// `isStillOwned` is re-asked by the workspace when the activation actually
3565+
// runs. Everything checked here is checked at REQUEST time, and the workspace
3566+
// can park an activation behind an unsettled manual edit for an unbounded
3567+
// time — long enough for a newer run or another conversation to take over. A
3568+
// predicate rather than data because only the caller knows what owning this
3569+
// request means; a user's click passes none, since a user's choice cannot go
3570+
// stale by waiting.
3571+
const requestOpenFile = useCallback((
3572+
name: string,
3573+
source: WorkspaceOpenRequestSource,
3574+
isStillOwned?: () => boolean,
3575+
) => {
35653576
if (!name) return;
3566-
setOpenRequest({ name, nonce: Date.now(), source });
3577+
setOpenRequest({ name, nonce: Date.now(), source, isStillOwned });
35673578
}, []);
35683579
// Handed to ChatPane, whose file links and produced-file chips are clicked by
35693580
// the user; ChatPane itself stays a plain `(name) => void` consumer.
@@ -3844,7 +3855,18 @@ export function ProjectView({
38443855
userActivations: workspaceUserActivationsRef.current,
38453856
});
38463857
if (!decision.keepWatching) pendingAutoOpenSettleRef.current = null;
3847-
if (decision.openFileName) requestOpenFile(decision.openFileName, 'internal');
3858+
// The watch's own conversation guard runs above, at decision time; this
3859+
// carries it to activation time for the same reason the run opener does.
3860+
// Generation is not this path's fence — the watch is retired outright when a
3861+
// newer send arms over it — so conversation is the whole ownership here.
3862+
const watchConversationId = pending.conversationId;
3863+
if (decision.openFileName) {
3864+
requestOpenFile(
3865+
decision.openFileName,
3866+
'internal',
3867+
() => activeConversationIdRef.current === watchConversationId,
3868+
);
3869+
}
38483870
}, [requestOpenFile]);
38493871

38503872
// Later lists: every accepted file-list generation (and every focus change,
@@ -7097,10 +7119,16 @@ export function ProjectView({
70977119
// it. Checked here, at the moment the request actually goes out, rather
70987120
// than at arming: the whole point is that an unbounded amount of time can
70997121
// pass in between.
7122+
const runStillOwnsAutoOpen = () =>
7123+
autoOpenSettleGenerationRef.current === autoOpenSettleGeneration
7124+
&& activeConversationIdRef.current === runConversationId;
71007125
const requestRunOpenFile = (fileName: string) => {
7101-
if (autoOpenSettleGenerationRef.current !== autoOpenSettleGeneration) return false;
7102-
if (activeConversationIdRef.current !== runConversationId) return false;
7103-
requestOpenFile(fileName, 'internal');
7126+
if (!runStillOwnsAutoOpen()) return false;
7127+
// The same predicate travels with the request, because passing it here
7128+
// only proves the run owned auto-open when it asked. The workspace can
7129+
// hold the activation until a manual edit flushes, and the two lines
7130+
// above have already run by then.
7131+
requestOpenFile(fileName, 'internal', runStillOwnsAutoOpen);
71047132
runAutoOpenedFileNames.add(fileName);
71057133
return true;
71067134
};

apps/web/tests/components/FileWorkspace.userActivationTiming.test.tsx

Lines changed: 90 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@
1919
import { cleanup, render, waitFor } from '@testing-library/react';
2020
import { afterEach, describe, expect, it, vi } from 'vitest';
2121

22-
import { FileWorkspace } from '../../src/components/FileWorkspace';
22+
import {
23+
FileWorkspace,
24+
type WorkspaceOpenRequest,
25+
} from '../../src/components/FileWorkspace';
2326
import { I18nProvider } from '../../src/i18n';
2427
import type { ProjectFile } from '../../src/types';
2528

@@ -37,7 +40,10 @@ vi.mock('../../src/providers/registry', async () => {
3740
};
3841
});
3942

40-
const manualEditHeld = { open: false };
43+
const manualEditHeld: {
44+
open: boolean;
45+
release: ((settled: boolean) => void) | null;
46+
} = { open: false, release: null };
4147

4248
vi.mock('../../src/components/FileViewer', () => ({
4349
// Stands in for a file sitting in Manual Edit mode: it registers an exit
@@ -55,7 +61,12 @@ vi.mock('../../src/components/FileViewer', () => ({
5561
}) => {
5662
onManualEditExitHandlerChange?.(file.name, () => {
5763
manualEditHeld.open = true;
58-
return new Promise<boolean>(() => {});
64+
// Resolvable, unlike a plain never-settling promise: the ownership tests
65+
// below need the parked activation to be RELEASED after the handoff, to
66+
// show it is dropped at that moment rather than merely still waiting.
67+
return new Promise<boolean>((resolve) => {
68+
manualEditHeld.release = resolve;
69+
});
5970
});
6071
return <div data-testid="file-viewer">{file.name}</div>;
6172
},
@@ -80,7 +91,7 @@ const OTHER = textFile('other.md');
8091
function renderWorkspace(props: {
8192
onTabsStateChange: () => void;
8293
onUserActivateTab: () => void;
83-
openRequest?: { name: string; nonce: number; source: 'user' | 'internal' } | null;
94+
openRequest?: WorkspaceOpenRequest | null;
8495
}) {
8596
return render(
8697
<I18nProvider>
@@ -104,6 +115,7 @@ describe('FileWorkspace user-activation reporting', () => {
104115
afterEach(() => {
105116
cleanup();
106117
manualEditHeld.open = false;
118+
manualEditHeld.release = null;
107119
vi.clearAllMocks();
108120
});
109121

@@ -173,4 +185,78 @@ describe('FileWorkspace user-activation reporting', () => {
173185
await waitFor(() => expect(manualEditHeld.open).toBe(true));
174186
expect(onUserActivateTab).not.toHaveBeenCalled();
175187
});
188+
// Shared by the two ownership tests: render, then hand the workspace an
189+
// internal request whose owner predicate the test controls, and leave the
190+
// activation parked on the manual edit.
191+
async function parkInternalRequest(isStillOwned: () => boolean) {
192+
const onTabsStateChange = vi.fn();
193+
const onUserActivateTab = vi.fn();
194+
const request: WorkspaceOpenRequest = {
195+
name: 'other.md',
196+
nonce: 1,
197+
source: 'internal',
198+
isStillOwned,
199+
};
200+
201+
const { rerender } = renderWorkspace({ onTabsStateChange, onUserActivateTab });
202+
rerender(
203+
<I18nProvider>
204+
<FileWorkspace
205+
projectId="project-1"
206+
projectKind="prototype"
207+
files={[NOTES, OTHER]}
208+
liveArtifacts={[]}
209+
onRefreshFiles={vi.fn()}
210+
isDeck={false}
211+
tabsState={{ tabs: ['notes.md'], active: 'notes.md' }}
212+
onTabsStateChange={onTabsStateChange}
213+
onUserActivateTab={onUserActivateTab}
214+
openRequest={request}
215+
/>
216+
</I18nProvider>,
217+
);
218+
219+
// The activation must really be parked, or releasing it below proves nothing.
220+
await waitFor(() => expect(manualEditHeld.release).not.toBeNull());
221+
expect(onTabsStateChange).not.toHaveBeenCalled();
222+
return { onTabsStateChange };
223+
}
224+
225+
async function releaseManualEdit() {
226+
manualEditHeld.release?.(true);
227+
await waitFor(() => expect(manualEditHeld.release).not.toBeNull());
228+
await Promise.resolve();
229+
await Promise.resolve();
230+
}
231+
232+
// Positive control for the ownership test below: without it, "did not
233+
// activate" would also pass if the parked activation simply never ran.
234+
it('lands a parked internal activation whose run still owns it', async () => {
235+
const { onTabsStateChange } = await parkInternalRequest(() => true);
236+
237+
await releaseManualEdit();
238+
239+
await waitFor(() =>
240+
expect(onTabsStateChange).toHaveBeenCalledWith(
241+
expect.objectContaining({ active: 'other.md' }),
242+
),
243+
);
244+
});
245+
246+
it('drops a parked internal activation whose run lost ownership while it waited', async () => {
247+
// Reviewer #6842 (nettee, 2026-08-18, round 7): the requester's generation
248+
// and conversation checks ran when the request was made. This window is
249+
// unbounded, so a newer run or another conversation can take over inside
250+
// it — and the parked callback would still activate, because the gate only
251+
// re-checks the source tab and the activation sequence, neither of which a
252+
// handoff changes. Re-asking the owner at release is what stops it.
253+
let owned = true;
254+
const { onTabsStateChange } = await parkInternalRequest(() => owned);
255+
256+
// The handoff: a newer send, or the user moving to another chat.
257+
owned = false;
258+
await releaseManualEdit();
259+
260+
expect(onTabsStateChange).not.toHaveBeenCalled();
261+
});
176262
});

apps/web/tests/components/ProjectView.autoOpenSettle.test.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,7 +831,15 @@ describe('ProjectView auto-open settle watcher lifecycle', () => {
831831
});
832832

833833
await turn.releasePerWriteRead();
834-
expect(openRequestKeys().map((key) => key.name)).toContain('plan.md');
834+
// Polled rather than asserted on the tick after the release: the release
835+
// awaits a fixed number of microtasks, which is not a guarantee that the
836+
// continuation behind it has reached its open. Under full-suite load that
837+
// raced, and the failure looked like "the guard blocked it" rather than
838+
// "the assertion ran early" — the two are indistinguishable from an empty
839+
// list, which cost real debugging time.
840+
await waitFor(() =>
841+
expect(openRequestKeys().map((key) => key.name)).toContain('plan.md'),
842+
);
835843

836844
// The workspace follows the open request, exactly as it would in the app.
837845
await act(async () => {

0 commit comments

Comments
 (0)