From 7103da6484176a5866a13d6b894e48af59fd82f4 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 3 Aug 2026 19:09:42 +0000 Subject: [PATCH] bind the fork banner's comparison to the workspace it describes (#10483) * fix: bind the fork banner's comparison to the workspace it describes Co-Authored-By: Claude Opus 5 (1M context) * fix: invalidate in-flight comparisons when leaving a fork Co-Authored-By: Claude Opus 5 (1M context) * fix: drop a CI summary fetched for a superseded comparison Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../lib/components/ForkWorkspaceBanner.svelte | 78 +++++++++++++++---- 1 file changed, 63 insertions(+), 15 deletions(-) diff --git a/frontend/src/lib/components/ForkWorkspaceBanner.svelte b/frontend/src/lib/components/ForkWorkspaceBanner.svelte index d033e881f4..c37370bcb0 100644 --- a/frontend/src/lib/components/ForkWorkspaceBanner.svelte +++ b/frontend/src/lib/components/ForkWorkspaceBanner.svelte @@ -13,6 +13,9 @@ let loading = $state(false) let comparison: WorkspaceComparison | undefined = $state(undefined) + /** Workspace `comparison` describes; control flow only, never rendered. */ + let comparisonFor: string | undefined = undefined + let requestSeq = 0 let error: string | undefined = $state(undefined) let currentWorkspaceData = $derived($userWorkspaces.find((w) => w.id === $workspaceStore)) @@ -31,23 +34,42 @@ const drafts = useWorkspaceDrafts(() => (isFork ? ($workspaceStore ?? undefined) : undefined)) const draftCount = $derived(drafts.count) + // Every read of `comparison` that decides what the banner says or where its button + // goes must go through this: anything else — in flight, failed, or a tally skipped + // outright — counts zero of everything, which is indistinguishable from "nothing to + // deploy". Typed helper avoids the `never`-inference quirk on `$state` in `$derived`. + function isAnswerable(c: WorkspaceComparison | undefined, isLoading: boolean): boolean { + return !isLoading && !!c && !c.skipped_comparison + } + const hasAnswer = $derived(isAnswerable(comparison, loading)) + // Fork is fully in sync with its parent (comparison ran, no ahead/behind diffs). - // Typed helper avoids the $state `never`-inference quirk on `comparison` in $derived. function isUpToDate(c: WorkspaceComparison | undefined): boolean { return !!c && !c.skipped_comparison && c.summary.total_diffs === 0 } - let upToDate = $derived(isUpToDate(comparison)) + let upToDate = $derived(hasAnswer && isUpToDate(comparison)) // Up to date with the parent but local drafts are pending — show the draft // state (same text + CTA as the draft banner) instead of "Everything is up to date". let showDraftsOnly = $derived(upToDate && draftCount > 0) + // Leaving for a workspace with no comparison of its own has to invalidate whatever + // is in flight too, or that response lands as this one's answer — and its CI counts + // would be fetched for paths this workspace may not even have. + function dropComparison() { + requestSeq++ + comparison = undefined + comparisonFor = undefined + loading = false + resetCiTestSummary() + } + $effect(() => { ;[$workspaceStore, parentWorkspaceId] untrack(() => { if (isFork && $workspaceStore) { checkForChanges() } else { - comparison = undefined + dropComparison() } }) }) @@ -56,40 +78,57 @@ if (isFork && $workspaceStore) { checkForChanges() } else { - comparison = undefined + dropComparison() } }) async function checkForChanges() { - if (!$workspaceStore || !parentWorkspaceId) { + const ws = $workspaceStore + const parent = parentWorkspaceId + if (!ws || !parent) { return } + // A comparison only ever describes the workspace it was requested for. The + // component survives a fork switch, so drop the previous fork's rows before + // fetching rather than let them answer for this one, and let only the newest + // request write — responses can land out of order, and a late one would + // otherwise paint another fork's counts over the current answer. + if (comparisonFor !== ws) { + comparison = undefined + comparisonFor = undefined + resetCiTestSummary() + } + const seq = ++requestSeq loading = true error = undefined try { // Compare with parent workspace (shared single-flight fetch — the chat // diff tool reuses this result instead of recomputing the comparison) - const result = await fetchWorkspaceComparison(parentWorkspaceId, $workspaceStore) + const result = await fetchWorkspaceComparison(parent, ws) + if (seq !== requestSeq) return comparison = result + comparisonFor = ws } catch (e) { + if (seq !== requestSeq) return console.error('Failed to compare workspaces:', e) error = `Failed to check for changes: ${e}` - // Still show banner if there's an error, but with error message + // Show the banner with the error rather than the rows we failed to refresh: + // on a switch those belong to the fork we just left. + comparison = undefined + comparisonFor = undefined } finally { - loading = false + if (seq === requestSeq) loading = false } } // Opens the direction the button offers, so the label and the list agree: a fork // with nothing to deploy lands on the update side, not on an empty deploy list. - // Both read `comparisonLoaded` — an unknown comparison counts zero of everything, - // which is indistinguishable from "nothing to deploy". function openComparisonDrawer() { if (parentWorkspaceId && $workspaceStore) { - const dir = comparisonLoaded && changesAhead === 0 ? '&dir=update' : '' + const dir = hasAnswer && changesAhead === 0 ? '&dir=update' : '' goto('/forks/compare?workspace_id=' + encodeURIComponent($workspaceStore) + dir, { replaceState: true }) @@ -109,17 +148,29 @@ let ciTestRunning = $state(0) let ciTestTotal = $state(0) + // These describe the rows of one comparison, so they are dropped with it. + function resetCiTestSummary() { + ciTestPassing = 0 + ciTestFailing = 0 + ciTestRunning = 0 + ciTestTotal = 0 + } + async function fetchCiTestSummary() { if (!$workspaceStore || !comparison?.diffs) return const items = comparison.diffs .filter((d) => d.kind === 'script' || d.kind === 'flow' || d.kind === 'resource') .map((d) => ({ path: d.path, kind: d.kind as 'script' | 'flow' | 'resource' })) if (items.length === 0) return + // Counted for the comparison current at call time — the poll below outlives a + // fork switch, and a slow batch for the fork we left would repaint this one's. + const seq = requestSeq try { const batch = await ScriptService.getCiTestResultsBatch({ workspace: $workspaceStore, requestBody: { items } }) + if (seq !== requestSeq) return let passing = 0 let failing = 0 let running = 0 @@ -161,9 +212,6 @@ function countDir(c: WorkspaceComparison | undefined, mergeIntoParent: boolean): number { return c?.diffs.filter((d) => diffActionableInDirection(d, mergeIntoParent)).length ?? 0 } - // A comparison in flight is not an answer: on a fork switch the component stays - // mounted and `comparison` still holds the previous fork's rows. - const comparisonLoaded = $derived(!loading && comparison !== undefined) const changesAhead = $derived(countDir(comparison, true)) const changesBehind = $derived(countDir(comparison, false)) @@ -359,7 +407,7 @@ > {#if showDraftsOnly} Review & deploy drafts - {:else if !comparisonLoaded || changesAhead > 0} + {:else if !hasAnswer || changesAhead > 0} Review & Deploy Changes {:else} Review & Update fork