-
Notifications
You must be signed in to change notification settings - Fork 10.3k
fix(web): re-evaluate Plan-mode auto-open when the post-turn file list settles #6842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
e082536
0b2de39
366ec5b
7654774
8b294b7
c0bdae5
cc37e27
fa90a42
df05a31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -288,8 +288,10 @@ import { useIframeKeepAlivePool } from './IframeKeepAlivePool'; | |
| import { invalidateHtmlSourceSnapshotProject } from './html-source-snapshot-cache'; | ||
| import { | ||
| decideAutoOpenAfterWrite, | ||
| reevaluateAutoOpenOnFilesSettled, | ||
| selectAutoOpenProducedArtifact, | ||
| selectAutoOpenTurnArtifact, | ||
| type AutoOpenSettleRequest, | ||
| } from './auto-open-file'; | ||
| import { buildRepoImportPrompt, designSystemNeedsRepoConnect } from './design-system-github-evidence'; | ||
| import { isDesignSystemProject, resolveProjectDesignSystemId } from './design-system-project'; | ||
|
|
@@ -690,6 +692,12 @@ let liveArtifactEventSequence = 0; | |
| // local literal to respect the web↔daemon boundary. | ||
| const BRAND_KIT_FILE = 'brand.html'; | ||
| const BRAND_EMPTY_TRANSCRIPT_RETRY_DELAYS_MS = [120, 500, 1_200, 2_000] as const; | ||
| // How long after a turn ends its auto-open decision keeps being re-evaluated | ||
| // against newly settled file lists (issue #5352). Wide enough to cover a | ||
| // chokidar burst plus the coalescing window and the refetch behind it; short | ||
| // enough that a file landing much later reads as the user's own work, not the | ||
| // turn's, and is left alone. | ||
| const AUTO_OPEN_SETTLE_WINDOW_MS = 15_000; | ||
| const CHAT_PANEL_WIDTH_STORAGE_KEY = 'open-design.project.chatPanelWidth'; | ||
| const DEFAULT_CHAT_PANEL_WIDTH = 460; | ||
| const MIN_CHAT_PANEL_WIDTH = 345; | ||
|
|
@@ -2422,6 +2430,24 @@ export function ProjectView({ | |
| tabs: [], | ||
| active: null, | ||
| }); | ||
| // Mirror for the run-completion continuation, which reads the active tab | ||
| // long after the render that captured it. | ||
| const openTabsActiveRef = useRef<string | null>(openTabsState.active); | ||
| openTabsActiveRef.current = openTabsState.active; | ||
| // The turn whose auto-open decision is still being re-evaluated as post-turn | ||
| // file lists settle (issue #5352); null when no turn is in that window. | ||
| // | ||
| // Carries the generation that armed it. A turn's completion continuation is | ||
| // unawaited, so an older turn can reach the arming site after a newer send has | ||
| // already started; without an owner token that older finalizer reinstalls its | ||
| // own producedFiles/resolver, and the NEW turn's file-list generations then | ||
| // drive the OLD turn's artifact into focus. | ||
| const pendingAutoOpenSettleRef = useRef<{ | ||
| readonly generation: number; | ||
| readonly request: AutoOpenSettleRequest<ProjectFile>; | ||
| } | null>(null); | ||
| // Monotonic auto-open owner token; bumped once per send. | ||
| const autoOpenSettleGenerationRef = useRef(0); | ||
| // Artifact context for the header actions (settings gear, handoff) that live | ||
| // in this workspace's header alongside FileViewer's present/share/download. | ||
| // Mirrors the artifact_id / artifact_kind that FileViewer attaches, derived | ||
|
|
@@ -3726,6 +3752,36 @@ export function ProjectView({ | |
| }); | ||
| }, [daemonLive, refreshWorkspaceItems, filesRefresh]); | ||
|
|
||
| // Issue #5352 — post-turn settle re-evaluation. The turn-end auto-open pass | ||
| // decides from ONE `fresh: true` read taken right after the daemon reports | ||
| // terminal status, and the generated file does not always appear in it: the | ||
| // daemon's own write, the chokidar event, its coalescing window and the | ||
| // refetch behind it all settle on their own clocks. This re-runs that same | ||
| // decision against each settled list until it resolves, its window expires, | ||
| // or focus moves somewhere the turn did not put it. | ||
| const evaluateAutoOpenSettle = useCallback(() => { | ||
| const pending = pendingAutoOpenSettleRef.current; | ||
| if (!pending) return; | ||
| // A watch armed by a superseded turn must never drive focus, even if it | ||
| // somehow survived the ownership check at the arming site. | ||
| if (pending.generation !== autoOpenSettleGenerationRef.current) { | ||
| pendingAutoOpenSettleRef.current = null; | ||
| return; | ||
| } | ||
| const decision = reevaluateAutoOpenOnFilesSettled(pending.request, projectFilesRef.current, { | ||
| now: Date.now(), | ||
| activeFileName: openTabsActiveRef.current, | ||
| }); | ||
| if (!decision.keepWatching) pendingAutoOpenSettleRef.current = null; | ||
| if (decision.openFileName) requestOpenFile(decision.openFileName); | ||
| }, [requestOpenFile]); | ||
|
|
||
| // Later lists: every accepted file-list generation (and every focus change, | ||
| // which can retire the watch) re-runs the decision. | ||
| useEffect(() => { | ||
| evaluateAutoOpenSettle(); | ||
| }, [committedFilesGeneration, evaluateAutoOpenSettle, openTabsState.active]); | ||
|
|
||
| // Live-reload: when the daemon's chokidar watcher reports a file change, | ||
| // bump filesRefresh so the file list refetches with new mtimes — which | ||
| // propagates through to FileViewer iframes via PR #384's ?v=${mtime} | ||
|
|
@@ -6924,6 +6980,18 @@ export function ProjectView({ | |
| // agent finishes and surface anything new (e.g. a generated .pptx) | ||
| // as download chips on the assistant message. | ||
| const beforeFileNames = new Set(preTurnFileNames); | ||
| // A new turn owns auto-open from here on: drop any previous turn's | ||
| // still-open settle watch so a late file list cannot pull focus back to | ||
| // the artifact of the turn before this one. | ||
| // | ||
| // Clearing alone is not enough. The previous turn's completion | ||
| // continuation is unawaited, so it can still be sitting in its post-run | ||
| // awaits and reach the arming site AFTER this line runs. Bumping the | ||
| // generation gives that finalizer a token to check against, so it can | ||
| // recognise that it no longer owns auto-open instead of reinstalling | ||
| // itself over this turn. | ||
| const autoOpenSettleGeneration = ++autoOpenSettleGenerationRef.current; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: this token is advanced only after the async preflight/queue gates, and it is not tied to the active conversation. ProjectView intentionally stays mounted while activeConversationId changes, so if Turn A's finalizer is pending and the user switches to conversation B (or B is waiting in AMR preflight), this line has not run; A can later arm/evaluate its request against B's file-list generations and focus A's artifact. Advance/invalidate the owner before those awaits and clear/retire the pending request on conversation/authority changes (or store the conversation id in the request and reject mismatches). Add a switch-before-finalizer regression. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
||
| pendingAutoOpenSettleRef.current = null; | ||
| // Pending Write/Edit tool invocations for this run: tool_use_id -> path. | ||
| // Keeping this local prevents a superseded stream's late tool_result from | ||
| // consuming a replacement run's colliding tool id. | ||
|
|
@@ -7293,6 +7361,23 @@ export function ProjectView({ | |
| // and attach the new files to the assistant message as download | ||
| // chips. | ||
| void (async () => { | ||
| // Focus witness for the settle watch, sampled HERE — at terminal | ||
| // handoff, before any of the awaits below. Reading the active tab | ||
| // after them would let a tab the USER selected while the post-run | ||
| // refresh and artifact persistence were in flight become this | ||
| // turn's baseline, and the watcher's focus-move guard would then | ||
| // read the user's own choice as "where the turn put focus" and pull | ||
| // them back out of it. | ||
| const activeFileNameAtTerminalHandoff = openTabsActiveRef.current; | ||
| // Files this turn's OWN auto-open moves focus to during the | ||
| // continuation below. Those are auto activations, so they must stay | ||
| // turn-owned rather than reading as the user moving on — and they | ||
| // are not knowable at the moment the witness above is sampled. | ||
| const turnAutoOpenedFileNames = new Set<string>(); | ||
| const requestTurnOpenFile = (fileName: string) => { | ||
| turnAutoOpenedFileNames.add(fileName); | ||
| requestOpenFile(fileName); | ||
| }; | ||
| try { | ||
| // A settled shared file-list read from before the daemon exit can | ||
| // otherwise win the race with the file-change invalidation and | ||
|
|
@@ -7329,7 +7414,7 @@ export function ProjectView({ | |
| artifactPersistenceSucceeded = true; | ||
| savedArtifactRef.current = sameTurnWrite.name; | ||
| completionSelectedAutoOpen = true; | ||
| requestOpenFile(sameTurnWrite.name); | ||
| requestTurnOpenFile(sameTurnWrite.name); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: the new wrapper only protects the sameTurnWrite branch. If that branch misses, the else path immediately calls persistArtifact, whose pointer and successful-write paths still call raw requestOpenFile (ProjectView.tsx:3617 and 3682). If turn A is awaiting that persistence while turn B starts, A can resolve the write and focus its artifact after the generation changes; this request never reaches requestRunOpenFile. Pass the generation-aware opener into persistArtifact or remove its internal auto-open and issue requestTurnOpenFile only after the guarded persistence completes, then add a delayed-persistence overlap regression. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
||
| } else { | ||
| const persistence = await persistArtifact(artifactToPersist, nextFiles, finalText); | ||
| if (persistence.ok) artifactPersistenceSucceeded = true; | ||
|
|
@@ -7376,20 +7461,31 @@ export function ProjectView({ | |
| project.id, | ||
| projectDetail.resolvedDir, | ||
| ) ?? []; | ||
| const turnArtifactToOpen = selectAutoOpenTurnArtifact(produced, nextFiles, { | ||
| // Captured (not read through the live Set) because the `finally` | ||
| // below clears `traceTouchedFilePaths` as soon as this turn ends, | ||
| // while the settle re-evaluation runs after it. | ||
| const turnTouchedPaths = [ | ||
| ...traceTouchedFilePaths, | ||
| ...(authoritativeArtifactPaths ?? []), | ||
| ]; | ||
| const turnAutoOpenOptionsFor = ( | ||
| files: ReadonlyArray<ProjectFile>, | ||
| ) => ({ | ||
| ...autoOpenArtifactOptions, | ||
| turnStartedAt: startedAt, | ||
| turnEndedAt: endedAt ?? null, | ||
| agentTouchedFileNames: resolveAgentTouchedFileNames( | ||
| [ | ||
| ...traceTouchedFilePaths, | ||
| ...(authoritativeArtifactPaths ?? []), | ||
| ], | ||
| nextFiles, | ||
| turnTouchedPaths, | ||
| files, | ||
| project.id, | ||
| projectDetail.resolvedDir, | ||
| ), | ||
| }); | ||
| const turnArtifactToOpen = selectAutoOpenTurnArtifact( | ||
| produced, | ||
| nextFiles, | ||
| turnAutoOpenOptionsFor(nextFiles), | ||
| ); | ||
| const producedArtifactToOpen = selectAutoOpenProducedArtifact( | ||
| [ | ||
| ...provenTraceTouchedFiles(), | ||
|
|
@@ -7404,7 +7500,37 @@ export function ProjectView({ | |
| ); | ||
| if (producedArtifactToOpen) { | ||
| completionSelectedAutoOpen = true; | ||
| requestOpenFile(producedArtifactToOpen); | ||
| requestTurnOpenFile(producedArtifactToOpen); | ||
| } | ||
| // Issue #5352: `nextFiles` above is a single post-run read. When | ||
| // the generated file has not surfaced in it yet, the selection | ||
| // just made is wrong — it either found nothing or ranked a | ||
| // support file first — and nothing revisits it. Hand the same | ||
| // turn inputs to the settle watcher so the next file lists that | ||
| // land re-run the selection instead. | ||
| // | ||
| // Only if this turn still owns auto-open. Everything above ran | ||
| // behind unawaited awaits, so a newer send may already have taken | ||
| // over; arming here would then let THIS turn's artifact ride the | ||
| // NEXT turn's file-list generations into focus. | ||
| if (autoOpenSettleGenerationRef.current === autoOpenSettleGeneration) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: this check is too late to protect the completion path. If Turn A is paused in the awaited refresh/persistence and Turn B starts, A can execute requestTurnOpenFile(sameTurnWrite.name) or requestTurnOpenFile(producedArtifactToOpen) above (and persistArtifact can call requestOpenFile) before reaching this if. The mismatch then skips only pendingAutoOpenSettleRef; the stale openRequest has already been sent and can focus A's file during B. Make every completion auto-open request generation-aware, including the artifact-persistence path, and add an overlap regression with a previewable post-run artifact. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
||
| pendingAutoOpenSettleRef.current = { | ||
| generation: autoOpenSettleGeneration, | ||
| request: { | ||
| producedFiles: produced, | ||
| resolveTurnOptions: turnAutoOpenOptionsFor, | ||
| requestedFileName: producedArtifactToOpen ?? null, | ||
| activeFileNameAtTurnEnd: activeFileNameAtTerminalHandoff, | ||
| turnOwnedFileNames: [...turnAutoOpenedFileNames], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: this snapshot only contains files opened through requestTurnOpenFile in the completion continuation. The earlier per-write callbacks were changed to requestRunOpenFile, so an accepted per-write open such as plan.md that settles after terminal handoff is absent from turnOwnedFileNames. When the settle list later selects index.html, activeFileName=plan.md is then neither the handoff baseline, requestedFileName, nor a recorded turn-owned name, so reevaluateAutoOpenOnFilesSettled retires instead of upgrading even though the same turn caused the focus. Keep a run-scoped set updated by every generation-accepted opener and snapshot that set here, with a delayed same-run per-write-then-settle regression. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
||
| deadline: Date.now() + AUTO_OPEN_SETTLE_WINDOW_MS, | ||
| }, | ||
| }; | ||
| // The list this turn wanted may already have landed while the | ||
| // completion path was still running (persisting the artifact, | ||
| // diffing produced files). Nothing re-renders on arming alone, | ||
| // so evaluate once here rather than waiting for a further | ||
| // refresh that may never come. | ||
| evaluateAutoOpenSettle(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: the conversation guard can be bypassed by the unawaited finalizer's stale closure. This call uses the evaluateAutoOpenSettle function captured by the handleSend render that started conversation A; after the user switches to B, that old evaluator still compares pending.conversationId against A's captured activeConversationId (the handleSend dependency list does not refresh this in-flight closure). If the settled artifact is already in projectFilesRef when the finalizer arms, it can request A's file before B's next effect clears the pending ref. Read the current conversation from a ref inside the evaluator (or call a current ref-held evaluator), and add a switch-before-finalizer test with the target already present in the accepted list. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
||
| } | ||
| const deliveryCandidate: ChatMessage = { | ||
| ...latestAssistantMsg, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: this focus witness is not the actual workspace focus.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.openTabsState.activecontains only persisted tab activations, whileFileWorkspace.activatePending()deliberately changes its localactiveTabwithout callingonTabsStateChangefor an unsaved sketch. Interleaving: the terminal handoff recordsnotes.md; while the completion read is held, the user activates a pending sketch; whenindex.htmlsettles,openTabsActiveRefstill reportsnotes.md, so the watcher treats the handoff as unchanged and opens the generated file over the sketch the user chose. The same name-based witness cannot distinguish a later manual activation of a file that the run auto-opened. Propagate the effective workspace active tab plus a monotonic activation/user witness (including transient tabs) into the settle request, retire it after any user activation after handoff, and add a ProjectView regression that activates a pending sketch while the finalizer is held.