From 2e1747a145cd11c2f6ba7ef9fd8745a1a5b4455b Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Tue, 10 Nov 2026 09:49:11 +0700 Subject: [PATCH] fix(app): keep the Changes diff on screen when its panel reopens (#9241) Hiding the Changes panel releases its diff subscription. Reopening it starts a new stream, and since #8907 a new stream first publishes a loading snapshot (entries, no bodies) before its first revision commits. That snapshot replaced the settled diff in the query cache, so the panel showed the full loading spinner for about a second and then repainted the same diff. A loading snapshot now only counts on the first load. Once a diff has settled, shareCheckoutDiff keeps it until the new stream's ready revision lands, which replaces it in one step when the content changed. --- packages/app/e2e/browser/changes-pane.spec.ts | 24 +++ packages/app/src/data/push-router.test.ts | 200 ++++++++++++++++++ packages/app/src/git/diff-sharing.ts | 8 + 3 files changed, 232 insertions(+) diff --git a/packages/app/e2e/browser/changes-pane.spec.ts b/packages/app/e2e/browser/changes-pane.spec.ts index 026e2791c9..8bc76e2a37 100644 --- a/packages/app/e2e/browser/changes-pane.spec.ts +++ b/packages/app/e2e/browser/changes-pane.spec.ts @@ -306,6 +306,24 @@ test("streamed Changes shows the tree before completion and applies a one-file r await page.screenshot({ path: testInfo.outputPath("refreshed.png") }); }); +test("reopening compact Changes keeps the painted diff while it resubscribes", async ({ page }) => { + const workspace = await createWorkspaceWithMountedTabDiff(); + const stream = await holdDiffCompletion(page); + const explorer = await openCompactChanges(page, workspace); + stream.finish(); + const firstFile = explorer.getByTestId("diff-file-0"); + await expect(firstFile).toBeVisible({ timeout: 30_000 }); + const painted = await firstFile.getAttribute("aria-label"); + + await explorer.getByTestId("workspace-tab-files").click(); + stream.hold(); + await explorer.getByTestId("workspace-tab-changes_tree").click(); + await expect.poll(() => stream.held, { timeout: 30_000 }).toBeGreaterThan(0); + await expect(page.getByRole("progressbar", { name: "Loading..." })).toHaveCount(0); + await expect(firstFile).toHaveAccessibleName(painted ?? ""); + stream.finish(); +}); + async function createLargeStreamedWorkspace(): Promise { const workspace = await createWorkspaceWithManyTinyDiffs(252); await Promise.all( @@ -371,6 +389,12 @@ async function holdDiffCompletion(page: Page) { get completed() { return completed; }, + get held() { + return completions.length; + }, + hold: () => { + holding = true; + }, finish: () => { holding = false; completions.splice(0).forEach((send) => send()); diff --git a/packages/app/src/data/push-router.test.ts b/packages/app/src/data/push-router.test.ts index 3590cfe30c..cf5a4d5af6 100644 --- a/packages/app/src/data/push-router.test.ts +++ b/packages/app/src/data/push-router.test.ts @@ -2,6 +2,8 @@ import type { DiffState, OwnedSubscription, SubscriptionObserver } from "@getpas import { QueryClient, QueryObserver, skipToken } from "@tanstack/react-query"; import { describe, expect, it } from "vitest"; import type { MutableDaemonConfig, SessionOutboundMessage } from "@getpaseo/protocol/messages"; +import { deriveCheckoutDiffResult, type CheckoutDiffSnapshot } from "@/git/diff-snapshot"; +import { shareCheckoutDiff } from "@/git/diff-sharing"; import { checkoutDiffQueryKey } from "@/git/query-keys"; import { buildTerminalsQueryKey } from "@/screens/workspace/terminals/state"; import { daemonConfigQueryKey } from "@/data/daemon-config"; @@ -34,6 +36,7 @@ type RouterMessage = type RouterMessageType = RouterMessage["type"]; type RouterHandler = (message: RouterMessage) => void; type RouterClient = Parameters[0]["client"]; +type DiffFile = SubscribeCheckoutDiffResponseMessage["payload"]["files"][number]; const daemonConfig: MutableDaemonConfig = { relay: { enabled: false }, @@ -233,6 +236,49 @@ function createFakeClient(config: { rejectCheckoutDiffSubscribe?: boolean } = {} }; } +/** Matches the real diff observation: nothing is delivered until the stream publishes. */ +type SubscribedDiffState = DiffState & { subscriptionId: string }; + +function createDiffStreamClient() { + const subscriptions: Array<{ publish(state: DiffState): void }> = []; + const noSubscription = { + subscriptionId: "none", + ready: Promise.resolve(undefined as never), + subscribe: () => () => {}, + release: async () => {}, + }; + const client: RouterClient = { + async getProvidersSnapshot() { + throw new Error("Unexpected snapshot pull"); + }, + observeEvents: () => noSubscription, + observeTerminals: () => noSubscription, + observeDiff() { + const observers = new Set>(); + let released = false; + const subscriptionId = `diff-${subscriptions.length + 1}`; + subscriptions.push({ + publish: (state) => { + if (released) return; + for (const observer of observers) observer.snapshot({ ...state, subscriptionId }); + }, + }); + return { + subscriptionId, + ready: new Promise(() => {}), + subscribe: (observer: SubscriptionObserver) => { + observers.add(observer); + return () => observers.delete(observer); + }, + release: async () => { + released = true; + }, + }; + }, + }; + return { client, subscriptions, latest: () => subscriptions[subscriptions.length - 1]! }; +} + function providerUpdate(generatedAt: string): ProvidersSnapshotUpdateMessage { return { type: "providers_snapshot_update", @@ -388,6 +434,160 @@ describe("server data push router", () => { } }); + describe("a diff reopened from a retained panel", () => { + const serverId = "server-1"; + const cwd = "/repo"; + const queryKey = checkoutDiffQueryKey(serverId, cwd, "uncommitted", undefined, false); + + function diffFile(content: string): DiffFile { + return { + path: "README.md", + isNew: false, + isDeleted: false, + additions: 1, + deletions: 0, + hunks: [ + { + oldStart: 1, + oldCount: 1, + newStart: 1, + newCount: 1, + lines: [{ type: "add", content, tokens: [{ text: content, style: null }] }], + }, + ], + }; + } + + function loadingState(revision: number): DiffState { + return { + phase: "loading", + revision, + entries: [ + { + path: "README.md", + oldPath: null, + change: "modified", + content: "text", + additions: 1, + deletions: 0, + }, + ], + }; + } + + function readyState(revision: number, files: DiffFile[]): DiffState { + return { + phase: "ready", + revision, + resolved: { baseSha: "", headSha: "", mergeBaseSha: "" }, + files, + diffTooLarge: false, + }; + } + + function mountChangesPanel(fake: ReturnType) { + const queryClient = new QueryClient(); + const route = (enabled: boolean) => ({ + queryKey, + queryFn: skipToken as typeof skipToken, + gcTime: Infinity, + structuralSharing: shareCheckoutDiff, + meta: checkoutDiffPushRoute({ + enabled, + serverId, + cwd, + subscriptionId: `checkoutDiff:${JSON.stringify(queryKey)}`, + compare: { mode: "uncommitted", ignoreWhitespace: false }, + }), + }); + const observer = new QueryObserver(queryClient, route(true)); + const unsubscribeObserver = observer.subscribe(() => {}); + const unmountRouter = mountServerDataPushRouter({ + queryClient, + client: fake.client, + serverId, + }); + return { + hide: () => observer.setOptions(route(false)), + show: () => observer.setOptions(route(true)), + // What the Changes pane renders from. + diff: () => + deriveCheckoutDiffResult( + queryClient.getQueryData(queryKey) ?? null, + ), + unmount: () => { + unsubscribeObserver(); + unmountRouter(); + queryClient.clear(); + }, + }; + } + + it("keeps the painted diff through the reload when the daemon returns the same content", () => { + const fake = createDiffStreamClient(); + const panel = mountChangesPanel(fake); + try { + fake.latest().publish(readyState(1, [diffFile("same")])); + const painted = panel.diff().files; + expect(fake.subscriptions).toHaveLength(1); + + panel.hide(); + panel.show(); + expect(fake.subscriptions).toHaveLength(2); + + fake.latest().publish(loadingState(1)); + expect(panel.diff()).toMatchObject({ isLoading: false, entries: null }); + expect(panel.diff().files).toBe(painted); + + fake.latest().publish(readyState(1, [diffFile("same")])); + expect(panel.diff().isLoading).toBe(false); + expect(panel.diff().files).toBe(painted); + } finally { + panel.unmount(); + } + }); + + it("replaces the painted diff in one step when the daemon returns changed content", () => { + const fake = createDiffStreamClient(); + const panel = mountChangesPanel(fake); + try { + fake.latest().publish(readyState(1, [diffFile("before")])); + const painted = panel.diff().files; + + panel.hide(); + panel.show(); + fake.latest().publish(loadingState(1)); + expect(panel.diff().isLoading).toBe(false); + expect(panel.diff().files).toBe(painted); + + const changed = [diffFile("after")]; + fake.latest().publish(readyState(1, changed)); + expect(panel.diff().isLoading).toBe(false); + expect(panel.diff().files).toEqual(changed); + } finally { + panel.unmount(); + } + }); + + it("reports loading with the announced entries when no content has been painted", () => { + const fake = createDiffStreamClient(); + const panel = mountChangesPanel(fake); + try { + expect(panel.diff().isLoading).toBe(true); + + fake.latest().publish(loadingState(1)); + expect(panel.diff()).toMatchObject({ isLoading: true, files: [] }); + expect(panel.diff().entries).toHaveLength(1); + + fake.latest().publish(readyState(1, [diffFile("first")])); + expect(panel.diff()).toMatchObject({ isLoading: false, entries: null }); + expect(panel.diff().files).toHaveLength(1); + } finally { + panel.unmount(); + } + }); + }); + it("routes provider snapshot and daemon config payloads until detached", async () => { const queryClient = new QueryClient(); const fake = createFakeClient(); diff --git a/packages/app/src/git/diff-sharing.ts b/packages/app/src/git/diff-sharing.ts index d426960d9f..8571de6630 100644 --- a/packages/app/src/git/diff-sharing.ts +++ b/packages/app/src/git/diff-sharing.ts @@ -25,6 +25,10 @@ export function shareCheckoutDiff(previous: unknown, incoming: unknown): DiffPay const next = incoming as DiffPayload; const old = previous as DiffPayload | undefined; if (!old || old === next) return next; + // A new subscription announces its entries before the bodies arrive. That is + // progress on the first load only: once a diff has settled it stays on screen + // until the next revision commits, so an unchanged diff never repaints. + if (isLoadingSnapshot(next) && !isLoadingSnapshot(old)) return old; const files = retainDiffFiles(old.files, next.files); const error = replaceEqualDeep(old.error, next.error); return files === old.files && @@ -37,6 +41,10 @@ export function shareCheckoutDiff(previous: unknown, incoming: unknown): DiffPay : { ...next, files, error }; } +function isLoadingSnapshot(payload: DiffPayload): boolean { + return payload.entries !== undefined; +} + export function shareCommitFileDiff( previous: unknown, incoming: unknown,