From 2aeb58ab485f1de27509c7217509cf346b4d8779 Mon Sep 17 00:00:00 2001 From: Taehoon Moon Date: Fri, 23 Oct 2026 00:10:23 +0900 Subject: [PATCH 1/2] Fix Review showing Git changes read before the page came back After the page or Review came back, the scope on screen kept its old list until the reread finished, and the other scope was never read again, so Branch could keep saying "No changes" after new commits. Choosing a scope now reads data from before the return again, places that said nothing changed show loading or the read's error meanwhile, and a reread reloads the open diff, source, or preview in place. --- docs/architecture/frontend.md | 19 +- .../tasks/(detail)/(review)/layout.js | 150 ++++++- .../tasks/(detail)/(review)/layout.test.js | 65 +++ .../tests/e2e/support/task-review-fixture.js | 40 +- frontend/tests/e2e/tasks/review-git.spec.js | 406 ++++++++++++++++++ .../tests/e2e/tasks/review-lifecycle.spec.js | 29 ++ 6 files changed, 689 insertions(+), 20 deletions(-) diff --git a/docs/architecture/frontend.md b/docs/architecture/frontend.md index 8696f424..bc3ad0de 100644 --- a/docs/architecture/frontend.md +++ b/docs/architecture/frontend.md @@ -1322,9 +1322,11 @@ modification time, and answer a browser that already holds that version with 304 without reading the file. A file without a modification time is sent with `no-store`. Review's file viewer fetches the image or document it previews itself, once for the selected file, and draws it from that copy. It asks the -server again only when Review learns the file may have changed: the watch -reports a change to it or recovers. An answer that names the same version, the -`ETag`, leaves the drawn copy in place; another version is drawn instead. +server again only when Review reloads the preview because the file may have +changed: the watch reports a change to it or recovers, or Review reads the +scope's Git data again while the preview is on screen. An answer that names the +same version, the `ETag`, leaves the drawn copy in place; another version is +drawn instead. Images and documents also answer one `Range: bytes=` range with 206 and `Content-Range`, reading only that part of the file, and a range that starts @@ -1436,6 +1438,17 @@ with every subject snapshot; a handoff with the same identity, roots, and route keeps the newer Task fields, such as branch and HEAD, for later requests and starts no request. +When its root watch recovers or a cached entry connects again, Integrated +Review treats the Working Tree status and the Branch refs and comparison it +holds as outdated, and drops answers to requests sent before that moment. It +reads the scope on screen again at once, and the other scope when it is +chosen. While outdated data is read again, an empty list and the viewer's +"No changes in this scope." show loading instead, and the read's error if it +fails; a list with files and an open diff stay as they are until the answer +arrives. After reading Git data again, the viewer reloads a diff, source, or +preview already on screen without the loading state and keeps its reading +position. + ### Git `caffold-task-git-layout` is a direct shared Detail child under the pathless diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js index 92c18710..28e5abbf 100644 --- a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js @@ -49,6 +49,7 @@ class CaffoldTaskReview extends HTMLElement { connectedCallback() { this.ensureRendered(); this.active = true; + this.markScopesOutdated(); this.syncReview({ reactivated: true }); } @@ -75,9 +76,12 @@ class CaffoldTaskReview extends HTMLElement { this.refs = null; this.statusError = null; this.compareError = null; + this.statusOutdated = false; + this.branchOutdated = false; this.statusGeneration = 0; this.branchGeneration = 0; this.viewerGeneration = 0; + this.shownViewerStateKey = ""; this.watchUnsubscribe = null; this.watchPath = ""; this.pendingWatchViewerRefresh = false; @@ -207,6 +211,7 @@ class CaffoldTaskReview extends HTMLElement { const routeChanged = Boolean( this.previousRouteKey && this.previousRouteKey !== nextRouteKey, ); + const scopeChanged = routeChanged && previousRoute.scope !== nextRoute.scope; const revealFileSelection = Boolean( nextRoute.navigator === "files" && nextRoute.path && @@ -240,7 +245,7 @@ class CaffoldTaskReview extends HTMLElement { this.requestRoute(normalizedForTask); return true; } - this.syncReview({ contextChanged, routeChanged, revealFileSelection }); + this.syncReview({ contextChanged, routeChanged, scopeChanged, revealFileSelection }); return true; } @@ -464,11 +469,14 @@ class CaffoldTaskReview extends HTMLElement { this.refs = null; this.statusError = null; this.compareError = null; + this.statusOutdated = false; + this.branchOutdated = false; this.pendingWatchViewerRefresh = false; this.previousRouteKey = ""; this.workingTree()?.reset(); this.branchTree()?.reset(); this.fileNavigator()?.clearSelectedPath(); + this.shownViewerStateKey = ""; this.viewer()?.setEmpty(); } @@ -478,6 +486,16 @@ class CaffoldTaskReview extends HTMLElement { this.viewerGeneration += 1; } + // Git may have changed while the page was away or Review was hidden, so + // the data held for each scope is outdated until a request sent after + // this point answers. + markScopesOutdated() { + this.statusOutdated = true; + this.branchOutdated = true; + this.statusGeneration += 1; + this.branchGeneration += 1; + } + syncReview(options = {}) { if (!this.active || !this.task) { return; @@ -491,20 +509,27 @@ class CaffoldTaskReview extends HTMLElement { }); this.subscribeWatch(taskRootPath(this.task)); if (this.task.worktree) { - if (options.contextChanged || options.reactivated || !this.status) { + if ( + options.contextChanged || + options.reactivated || + !this.status || + (options.scopeChanged && this.route.scope === "working" && this.statusOutdated) + ) { void this.refreshWorking(); } if ( this.route.scope === "branch" && (options.reactivated || + (options.scopeChanged && this.branchOutdated) || !this.refs || !this.compare || this.compare.baseRef !== this.route.baseRef) ) { - void this.ensureBranchData({ force: options.reactivated }); + void this.ensureBranchData({ force: this.branchOutdated }); } } if (!this.route.path) { + this.shownViewerStateKey = ""; this.viewer()?.setEmpty(); } else if ( options.contextChanged || @@ -571,13 +596,16 @@ class CaffoldTaskReview extends HTMLElement { branch: this.task?.worktree?.branch ?? null, }); } + this.patchRereadState("working"); try { const status = await getGitStatus(rootPath); if (!this.acceptRequest("status", generation, rootPath)) { return null; } const hadStatus = Boolean(this.status); + const wasOutdated = this.statusOutdated; this.status = status; + this.statusOutdated = false; this.statusError = null; this.patchErrorState(); if (hadStatus) { @@ -590,11 +618,17 @@ class CaffoldTaskReview extends HTMLElement { const canRefreshViewer = this.route.path && this.route.scope === "working"; const pendingWatchViewerRefresh = this.pendingWatchViewerRefresh; this.pendingWatchViewerRefresh = false; + // A watch change reloads the viewer only for its own file, unless the + // status it replaces was read before the page came back. const refreshViewer = options.background - ? canRefreshViewer && pendingWatchViewerRefresh + ? canRefreshViewer && (pendingWatchViewerRefresh || wasOutdated) : canRefreshViewer; if (refreshViewer) { - void this.loadViewer({ background: Boolean(options.background) }); + void this.loadViewer({ + background: + Boolean(options.background) || + (hadStatus && this.shownViewerStateKey === this.viewerStateKey()), + }); } return status; } catch (error) { @@ -605,6 +639,7 @@ class CaffoldTaskReview extends HTMLElement { if (!this.status) { this.workingTree()?.setError(error, { rootPath }); } + this.patchRereadState("working", error); this.patchErrorState(); return null; } @@ -616,6 +651,7 @@ class CaffoldTaskReview extends HTMLElement { return null; } const generation = ++this.branchGeneration; + this.patchRereadState("branch"); try { const refs = force || !this.refs ? await getGitRefs(rootPath) : this.refs; if (!this.acceptRequest("branch", generation, rootPath)) { @@ -645,6 +681,7 @@ class CaffoldTaskReview extends HTMLElement { } const hadCompare = Boolean(this.compare); this.compare = compare; + this.branchOutdated = false; this.compareError = null; this.patchErrorState(); this.branchTree()?.setEmptyMessage( @@ -659,7 +696,9 @@ class CaffoldTaskReview extends HTMLElement { this.patchControls(); this.patchEmptyStates(); if (this.route.path && this.route.scope === "branch") { - void this.loadViewer(); + void this.loadViewer({ + background: hadCompare && this.shownViewerStateKey === this.viewerStateKey(), + }); } return compare; } catch (error) { @@ -670,6 +709,7 @@ class CaffoldTaskReview extends HTMLElement { if (!this.compare) { this.branchTree()?.setError(error, this.refs?.repository ?? null); } + this.patchRereadState("branch", error); this.patchErrorState(); return null; } @@ -689,10 +729,20 @@ class CaffoldTaskReview extends HTMLElement { async loadViewer(options = {}) { const selectedPath = this.logicalSelectedPath(); if (!selectedPath || !this.active) { + this.shownViewerStateKey = ""; this.viewer()?.setEmpty(); return; } const generation = ++this.viewerGeneration; + const stateKey = this.viewerStateKey(); + // A background load follows a change the watch reported for the file, a + // recovered watch, or a reread of Git data while the view is on screen: + // it keeps the reading position and asks again whether a previewed file + // changed. Any other load shows the file the viewer already holds. + const background = Boolean(options.background); + if (!background) { + this.shownViewerStateKey = ""; + } const sourceMode = this.route.viewer === "source"; const previewMode = this.route.viewer === "preview"; const fileMode = sourceMode || previewMode; @@ -706,16 +756,13 @@ class CaffoldTaskReview extends HTMLElement { } const change = this.selectedChange(); if (fileMode && change?.deleted) { + this.shownViewerStateKey = ""; this.viewer()?.setNotice( "This file was deleted in the selected scope. Diff remains available.", { title: fileNameFromPath(selectedPath) }, ); return; } - // A background load follows a change the watch reported for the file or - // a recovered watch, so the file may have changed; any other load shows - // the file the viewer already holds. - const recheck = Boolean(options.background); if (previewMode && representations.previewKind === "document") { const entry = this.fileNavigator()?.entryForPath(selectedPath); this.viewer()?.setDocument({ @@ -724,7 +771,8 @@ class CaffoldTaskReview extends HTMLElement { name: fileNameFromPath(selectedPath), size: entry?.size, modifiedMs: entry?.modifiedMs, - }, { recheck }); + }, { recheck: background }); + this.shownViewerStateKey = stateKey; return; } if (previewMode && representations.previewKind === "image") { @@ -735,10 +783,12 @@ class CaffoldTaskReview extends HTMLElement { imageType: imageTypeLabel(selectedPath), size: entry?.size, modifiedMs: entry?.modifiedMs, - }, { recheck }); + }, { recheck: background }); + this.shownViewerStateKey = stateKey; return; } if (sourceMode && !representations.source) { + this.shownViewerStateKey = ""; this.viewer()?.setNotice("Source is unavailable for this file.", { actionLabel: "View preview", action: "view-preview", @@ -747,6 +797,7 @@ class CaffoldTaskReview extends HTMLElement { return; } if (previewMode && !representations.previewKind) { + this.shownViewerStateKey = ""; this.viewer()?.setNotice("Preview is unavailable for this file.", { actionLabel: "View source", action: "view-source", @@ -755,6 +806,18 @@ class CaffoldTaskReview extends HTMLElement { return; } if (!fileMode && !change) { + this.shownViewerStateKey = ""; + const branch = this.route.scope === "branch"; + if (branch ? this.branchOutdated : this.statusOutdated) { + const error = branch ? this.compareError : this.statusError; + const presentation = this.diffPresentation(selectedPath, null); + if (error) { + this.viewer()?.setError(presentation, error); + } else { + this.viewer()?.setLoading(presentation); + } + return; + } this.viewer()?.setNotice("No changes in this scope.", { actionLabel: "View source", action: "view-source", @@ -763,8 +826,7 @@ class CaffoldTaskReview extends HTMLElement { return; } const viewer = this.viewer(); - const saved = this.viewerScroll.get(this.viewerStateKey()); - const background = Boolean(options.background); + const saved = this.viewerScroll.get(stateKey); const presentation = fileMode ? sourceViewerPresentation({ path: selectedPath }) : this.diffPresentation(selectedPath, change); @@ -812,11 +874,13 @@ class CaffoldTaskReview extends HTMLElement { presentation: loadedPresentation, }); } + this.shownViewerStateKey = stateKey; if (!background && !(sourceMode && this.route.line)) { this.restoreViewerPosition(saved); } } catch (error) { if (this.acceptViewer(generation, selectedPath)) { + this.shownViewerStateKey = ""; viewer?.setError(presentation, error); } } @@ -961,10 +1025,65 @@ class CaffoldTaskReview extends HTMLElement { } patchEmptyStates() { - const workingEmpty = Boolean(this.status && (this.status.files?.length ?? 0) === 0); + const workingEmpty = Boolean( + this.status && !this.statusOutdated && (this.status.files?.length ?? 0) === 0, + ); this.querySelector(".task-review-empty-action")?.toggleAttribute("hidden", !workingEmpty); } + // While a scope's data from before the page came back is read again, the + // places where that data says nothing changed show loading, then the read's + // error if it fails. + patchRereadState(scope, error = null) { + const working = scope === "working"; + if (!(working ? this.statusOutdated : this.branchOutdated)) { + return; + } + const data = working ? this.status : this.compare; + if (data && !data.files?.length) { + if (working) { + const rootPath = taskWorktreeRootPath(this.task); + if (error) { + this.workingTree()?.setError(error, { rootPath }); + } else { + this.workingTree()?.setLoading({ + rootPath, + branch: this.task?.worktree?.branch ?? null, + }); + } + } else { + const repository = this.refs?.repository ?? null; + if (error) { + this.branchTree()?.setError(error, repository); + } else { + this.branchTree()?.setLoading(repository, { + baseRef: this.route.baseRef, + headRef: taskCompareHeadRef(this.task, this.refs), + }); + } + } + } + if (working) { + this.patchEmptyStates(); + } + if ( + !data || + this.route.scope !== scope || + !this.route.path || + this.route.viewer !== "diff" || + this.selectedChange() + ) { + return; + } + const presentation = this.diffPresentation(this.logicalSelectedPath(), null); + this.shownViewerStateKey = ""; + if (error) { + this.viewer()?.setError(presentation, error); + } else { + this.viewer()?.setLoading(presentation); + } + } + syncSelection() { const logicalPath = this.logicalSelectedPath(); this.workingTree()?.setSelectedPath(logicalPath); @@ -1063,6 +1182,7 @@ class CaffoldTaskReview extends HTMLElement { } async refreshAll() { + this.markScopesOutdated(); const requests = [ this.fileNavigator()?.requestRefresh({ allDirectories: true, diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js index fd2d4ad3..06926681 100644 --- a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js @@ -234,8 +234,10 @@ test("drops a Branch compare that returns after Review leaves Branch", async (t) }, route: { scope: "branch", baseRef: "origin/main", path: "" }, branchGeneration: 0, + branchOutdated: false, compare: null, acceptRequest: review.acceptRequest, + patchRereadState: review.patchRereadState, branchTree: () => branchTree, patchControls() {}, patchErrorState() {}, @@ -254,3 +256,66 @@ test("drops a Branch compare that returns after Review leaves Branch", async (t) assert.equal(await late, null); assert.equal(drawn.length, 1); }); + +test("drops a Working Tree status read sent before the page came back", async (t) => { + const previousFetch = globalThis.fetch; + const previousWindow = globalThis.window; + const statuses = []; + globalThis.window = { location: { origin: "http://caffold.test" } }; + globalThis.fetch = (input, ...options) => { + if (new URL(input).pathname !== "/api/git/status") { + return previousFetch(input, ...options); + } + const response = Promise.withResolvers(); + statuses.push(response); + return response.promise; + }; + t.after(() => { + globalThis.fetch = previousFetch; + if (previousWindow === undefined) delete globalThis.window; + else globalThis.window = previousWindow; + }); + const clean = { repository: { rootPath: "/repo" }, files: [] }; + const changed = { + repository: { rootPath: "/repo" }, + files: [{ path: "src/planner.rs", status: "M" }], + }; + const drawn = []; + const workingTree = { + setLoading() {}, + setStatus: (status) => drawn.push(status), + updateStatus: (status) => drawn.push(status), + }; + const owner = { + active: true, + isConnected: true, + task: { threadId: "thread-a", worktree: { rootPath: "/repo", branch: "main" } }, + route: { scope: "branch", baseRef: "origin/main", path: "" }, + status: clean, + statusOutdated: false, + branchOutdated: false, + statusGeneration: 0, + branchGeneration: 0, + pendingWatchViewerRefresh: false, + acceptRequest: review.acceptRequest, + patchRereadState: review.patchRereadState, + workingTree: () => workingTree, + patchErrorState() {}, + patchEmptyStates() {}, + syncSelection() {}, + }; + + const sentBefore = review.refreshWorking.call(owner, { background: true }); + review.markScopesOutdated.call(owner); + statuses.at(-1).resolve(new Response(JSON.stringify(changed))); + assert.equal(await sentBefore, null); + assert.equal(owner.status, clean); + assert.equal(owner.statusOutdated, true); + assert.equal(drawn.length, 0); + + const sentAfter = review.refreshWorking.call(owner, { background: true }); + statuses.at(-1).resolve(new Response(JSON.stringify(changed))); + assert.deepEqual(await sentAfter, changed); + assert.equal(owner.statusOutdated, false); + assert.deepEqual(drawn, [changed]); +}); diff --git a/frontend/tests/e2e/support/task-review-fixture.js b/frontend/tests/e2e/support/task-review-fixture.js index ba886cca..c91d970f 100644 --- a/frontend/tests/e2e/support/task-review-fixture.js +++ b/frontend/tests/e2e/support/task-review-fixture.js @@ -13,8 +13,11 @@ export async function installTaskReviewFixture(page) { let cleanWorkingTree = false; let cleanBranch = false; let failNextGitStatus = false; + let failNextCompare = false; + let statusGate = null; let diffGate = null; let workingDiffText = "new planner behavior"; + let branchDiffText = "new branch behavior"; const compareDelays = new Map(); const compareGates = new Map(); let refs = [ @@ -28,8 +31,9 @@ export async function installTaskReviewFixture(page) { ]; const compareDiffBaseRefs = []; - await page.route(/\/api\/git\/status(?:\?|$)/, (route) => { + await page.route(/\/api\/git\/status(?:\?|$)/, async (route) => { gitStatusRequests += 1; + await statusGate?.promise; if (failNextGitStatus) { failNextGitStatus = false; return route.fulfill({ @@ -252,6 +256,14 @@ export async function installTaskReviewFixture(page) { await new Promise((resolve) => setTimeout(resolve, delay)); } await compareGates.get(baseRef)?.promise; + if (failNextCompare) { + failNextCompare = false; + return route.fulfill({ + status: 503, + contentType: "application/json", + body: JSON.stringify({ error: "fixture compare unavailable" }), + }); + } const path = baseRef === "origin/release" ? "src/release.rs" : "src/planner.rs"; return route.fulfill({ contentType: "application/json", @@ -293,7 +305,7 @@ export async function installTaskReviewFixture(page) { `+++ b/${relativePath}`, "@@ -60 +60 @@", "-old branch behavior", - "+new branch behavior", + `+${branchDiffText}`, ].join("\n"), }), }); @@ -339,9 +351,15 @@ export async function installTaskReviewFixture(page) { set failNextGitStatus(value) { failNextGitStatus = value; }, + set failNextCompare(value) { + failNextCompare = value; + }, set workingDiffText(value) { workingDiffText = value; }, + set branchDiffText(value) { + branchDiffText = value; + }, setCompareDelay(baseRef, delayMs) { compareDelays.set(baseRef, delayMs); }, @@ -363,6 +381,24 @@ export async function installTaskReviewFixture(page) { releaseGate(); }; }, + holdStatus() { + if (statusGate) { + throw new Error("Status requests already held"); + } + let releaseGate; + const gate = { + promise: new Promise((resolve) => { + releaseGate = resolve; + }), + }; + statusGate = gate; + return () => { + if (statusGate === gate) { + statusGate = null; + } + releaseGate(); + }; + }, holdDiff() { if (diffGate) { throw new Error("Diff requests already held"); diff --git a/frontend/tests/e2e/tasks/review-git.spec.js b/frontend/tests/e2e/tasks/review-git.spec.js index 44cc6df1..d835ff27 100644 --- a/frontend/tests/e2e/tasks/review-git.spec.js +++ b/frontend/tests/e2e/tasks/review-git.spec.js @@ -911,6 +911,322 @@ test("refreshes Branch after returning from the background", { tag: "@viewport-i ).toHaveText("-0"); }); +test("shows Branch loading where it said nothing changed until it is read again after returning", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanBranch = true; + }, + }); + await selectTaskReviewScope(tasksPage, "branch"); + const compareTree = taskReview.locator("caffold-git-compare-tree"); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(compareTree).toContainText("No changes compared with origin/main."); + await taskReview.getByRole("button", { name: "Files", exact: true }).click(); + await taskReview + .locator('caffold-file-navigator button[data-file-tree-path="src/planner.rs"]') + .click(); + await taskReview.getByRole("button", { name: "Changes", exact: true }).click(); + await expect(viewer).toContainText("No changes in this scope."); + + await leavePage(page); + reviewScenario.cleanBranch = false; + const comparesBefore = reviewScenario.gitCompareRequests; + const releaseCompare = reviewScenario.holdCompare("origin/main"); + try { + await returnToPage(page); + await expect.poll(() => reviewScenario.gitCompareRequests).toBe(comparesBefore + 1); + await expect(compareTree).toContainText("Loading compare..."); + await expect(compareTree.locator(".compare-file-count")).toHaveText(""); + await expect(viewer.locator(".loading-panel")).toHaveCount(1); + await expect(viewer).not.toContainText("No changes in this scope."); + } finally { + releaseCompare(); + } + await expect( + compareTree.locator('button[data-file-tree-path="src/planner.rs"]'), + ).toBeVisible(); + await expect(viewer).toContainText("new branch behavior"); +}); + +test("reads Branch again when it is chosen after returning on Working Tree", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanBranch = true; + }, + }); + await selectTaskReviewScope(tasksPage, "branch"); + const compareTree = taskReview.locator("caffold-git-compare-tree"); + await expect(compareTree).toContainText("No changes compared with origin/main."); + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + await expect( + taskReview.locator('caffold-git-diff-changes-tree button[data-file-tree-kind="file"]'), + ).toHaveCount(4); + + await leavePage(page); + reviewScenario.cleanBranch = false; + const statusBefore = reviewScenario.gitStatusRequests; + await returnToPage(page); + await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 1); + + const refsBefore = reviewScenario.gitRefsRequests; + const comparesBefore = reviewScenario.gitCompareRequests; + const releaseCompare = reviewScenario.holdCompare("origin/main"); + try { + await selectTaskReviewScope(tasksPage, "branch"); + await expect.poll(() => reviewScenario.gitCompareRequests).toBe(comparesBefore + 1); + expect(reviewScenario.gitRefsRequests).toBe(refsBefore + 1); + await expect(compareTree).toContainText("Loading compare..."); + } finally { + releaseCompare(); + } + await expect( + compareTree.locator('button[data-file-tree-path="src/planner.rs"]'), + ).toBeVisible(); +}); + +test("reads Working Tree again when it is chosen after returning on Branch", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanWorkingTree = true; + }, + }); + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + const changes = taskReview.locator("caffold-git-diff-changes-tree"); + const reviewBranch = taskReview.getByRole("button", { name: "Review branch changes" }); + await expect(changes).toContainText("No changes."); + await expect(reviewBranch).toBeVisible(); + await selectTaskReviewScope(tasksPage, "branch"); + await expect( + taskReview.locator('caffold-git-compare-tree button[data-file-tree-path="src/planner.rs"]'), + ).toBeVisible(); + + await leavePage(page); + reviewScenario.cleanWorkingTree = false; + const comparesBefore = reviewScenario.gitCompareRequests; + await returnToPage(page); + await expect.poll(() => reviewScenario.gitCompareRequests).toBe(comparesBefore + 1); + + const statusBefore = reviewScenario.gitStatusRequests; + const releaseStatus = reviewScenario.holdStatus(); + try { + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 1); + await expect(changes).toContainText("Loading changes..."); + await expect(reviewBranch).toBeHidden(); + } finally { + releaseStatus(); + } + await expect(changes.locator('button[data-file-tree-kind="file"]')).toHaveCount(4); + await expect(reviewBranch).toBeHidden(); +}); + +test("updates the loading viewer when a watch change replaces the Working Tree read after returning", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanWorkingTree = true; + }, + }); + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(taskReview.locator("caffold-git-diff-changes-tree")).toContainText("No changes."); + await taskReview.getByRole("button", { name: "Files", exact: true }).click(); + await taskReview + .locator('caffold-file-navigator button[data-file-tree-path="src/planner.rs"]') + .click(); + await taskReview.getByRole("button", { name: "Changes", exact: true }).click(); + await expect(viewer).toContainText("No changes in this scope."); + + await leavePage(page); + reviewScenario.cleanWorkingTree = false; + const statusBefore = reviewScenario.gitStatusRequests; + const releaseStatus = reviewScenario.holdStatus(); + try { + await returnToPage(page); + await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 1); + await expect(viewer.locator(".loading-panel")).toHaveCount(1); + await page.evaluate(() => { + const source = window.__caffoldMockEventSources.find( + (candidate) => + candidate.url.startsWith("/api/watch?") && candidate.readyState !== 2, + ); + if (!source) { + throw new Error("Missing Task Review watch source"); + } + source.emit("change", { + revision: 2, + paths: ["src/other.rs"], + gitStatusChanged: true, + gitRefsChanged: false, + overflow: false, + }); + }); + await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 2); + } finally { + releaseStatus(); + } + await expect(viewer).toContainText("new planner behavior"); + await expect(viewer.locator(".loading-panel")).toHaveCount(0); +}); + +test("shows the error where Branch was loading when reading it again fails", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanBranch = true; + }, + }); + await selectTaskReviewScope(tasksPage, "branch"); + const compareTree = taskReview.locator("caffold-git-compare-tree"); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(compareTree).toContainText("No changes compared with origin/main."); + await taskReview.getByRole("button", { name: "Files", exact: true }).click(); + await taskReview + .locator('caffold-file-navigator button[data-file-tree-path="src/planner.rs"]') + .click(); + await taskReview.getByRole("button", { name: "Changes", exact: true }).click(); + await expect(viewer).toContainText("No changes in this scope."); + + await leavePage(page); + reviewScenario.failNextCompare = true; + await returnToPage(page); + await expect(taskReview.locator(".task-review-error")).toHaveText( + "Branch comparison update failed: fixture compare unavailable", + ); + await expect(compareTree).toContainText("fixture compare unavailable"); + await expect(compareTree).not.toContainText("No changes compared with origin/main."); + await expect(viewer.locator(".error-panel")).toContainText("fixture compare unavailable"); + await expect(viewer).not.toContainText("No changes in this scope."); +}); + +test("shows the error where Working Tree was loading when reading it again fails", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanWorkingTree = true; + }, + }); + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + const changes = taskReview.locator("caffold-git-diff-changes-tree"); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(changes).toContainText("No changes."); + await taskReview.getByRole("button", { name: "Files", exact: true }).click(); + await taskReview + .locator('caffold-file-navigator button[data-file-tree-path="src/planner.rs"]') + .click(); + await taskReview.getByRole("button", { name: "Changes", exact: true }).click(); + await expect(viewer).toContainText("No changes in this scope."); + + await leavePage(page); + reviewScenario.failNextGitStatus = true; + await returnToPage(page); + await expect(taskReview.locator(".task-review-error")).toHaveText( + "Working tree update failed: fixture status unavailable", + ); + await expect(changes).toContainText("fixture status unavailable"); + await expect(changes).not.toContainText("No changes."); + await expect(taskReview.getByRole("button", { name: "Review branch changes" })).toBeHidden(); + await expect(viewer.locator(".error-panel")).toContainText("fixture status unavailable"); + await expect(viewer).not.toContainText("No changes in this scope."); +}); + +test("keeps the open Branch diff where it was when Branch is read again", { tag: "@viewport-independent" }, async ({ + page, +}) => { + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.branchDiffText = longDiffText("branch line"); + }, + }); + await selectTaskReviewScope(tasksPage, "branch"); + await taskReview + .locator('caffold-git-compare-tree button[data-file-tree-path="src/planner.rs"]') + .click(); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(viewer).toContainText("branch line 200"); + const top = await scrollViewerAndWatchReload(viewer, "caffold-diff-viewer", ".diff-lines"); + + const diffsBefore = reviewScenario.gitCompareDiffRequests; + await emitGitRefsChanged(page); + await expect.poll(() => reviewScenario.gitCompareDiffRequests).toBe(diffsBefore + 1); + await expect + .poll(() => readViewerReload(viewer, "caffold-diff-viewer", ".diff-lines")) + .toEqual({ reloaded: true, loadingSeen: false, top }); +}); + +test("keeps the open Working Tree diff where it was after returning", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.workingDiffText = longDiffText("working line"); + }, + }); + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + await taskReview + .locator('caffold-git-diff-changes-tree button[data-file-tree-relative-path="planner.rs"]') + .click(); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(viewer).toContainText("working line 200"); + const top = await scrollViewerAndWatchReload(viewer, "caffold-diff-viewer", ".diff-lines"); + + const diffsBefore = reviewScenario.gitDiffRequests; + await leavePage(page); + await returnToPage(page); + await expect.poll(() => reviewScenario.gitDiffRequests).toBe(diffsBefore + 1); + await expect + .poll(() => readViewerReload(viewer, "caffold-diff-viewer", ".diff-lines")) + .toEqual({ reloaded: true, loadingSeen: false, top }); +}); + +test("keeps the open Branch Source where it was when Branch is read again", { tag: "@viewport-independent" }, async ({ + page, +}) => { + let fileReads = 0; + await page.route(/\/api\/file(?:\?|$)/, (route) => { + fileReads += 1; + return route.fallback(); + }); + const { tasksPage, taskReview } = await openCompletedTaskForReview(page); + await selectTaskReviewScope(tasksPage, "branch"); + const plannerChange = taskReview.locator( + 'caffold-git-compare-tree button[data-file-tree-path="src/planner.rs"]', + ); + await expect(plannerChange).toBeVisible(); + // Choosing Source before the file opens it once, with no line carried over + // from the diff to scroll to afterwards. + await taskReview.getByRole("button", { name: "Source", exact: true }).click(); + const openReads = fileReads; + await plannerChange.click(); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(viewer).toContainText("planner source"); + expect(fileReads).toBe(openReads + 1); + const top = await scrollViewerAndWatchReload(viewer, "caffold-code-viewer", ".code-lines"); + + const readsBefore = fileReads; + await emitGitRefsChanged(page); + await expect.poll(() => fileReads).toBe(readsBefore + 1); + await expect + .poll(() => readViewerReload(viewer, "caffold-code-viewer", ".code-lines")) + .toEqual({ reloaded: true, loadingSeen: false, top }); +}); + test("falls back when the selected Branch base disappears", { tag: "@all-viewports" }, async ({ page }) => { const { reviewScenario, taskScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page); @@ -1705,3 +2021,93 @@ async function emitGitRefsChanged(page) { }); }); } + +async function controlPageVisibility(page) { + await page.addInitScript(() => { + window.__caffoldVisibilityState = "visible"; + Object.defineProperty(document, "visibilityState", { + configurable: true, + get: () => window.__caffoldVisibilityState, + }); + }); +} + +// The watch must have been ready once for the return to count as a recovery. +async function leavePage(page) { + await page.evaluate(() => { + const source = window.__caffoldMockEventSources.find( + (candidate) => + candidate.url.startsWith("/api/watch?") && candidate.readyState !== 2, + ); + if (!source) { + throw new Error("Missing Task Review watch source"); + } + source.emit("ready", { + revision: 1, + scopePath: "src", + repositoryRootPath: "src", + }); + window.__caffoldVisibilityState = "hidden"; + document.dispatchEvent(new Event("visibilitychange")); + }); + await expect + .poll(() => + page.evaluate(() => + window.__caffoldMockEventSources + .filter((source) => source.url.startsWith("/api/watch?")) + .every((source) => source.readyState === 2), + ), + ) + .toBe(true); +} + +async function returnToPage(page) { + await page.evaluate(() => { + window.__caffoldVisibilityState = "visible"; + document.dispatchEvent(new Event("visibilitychange")); + }); +} + +function longDiffText(label) { + return Array.from({ length: 200 }, (_, index) => `${label} ${index + 1}`).join("\n+"); +} + +async function scrollViewerAndWatchReload(viewer, contentSelector, scrollSelector) { + return viewer.evaluate((element, { contentSelector, scrollSelector }) => { + const scroller = element.querySelector(`${contentSelector} ${scrollSelector}`); + scroller.scrollTop = scroller.scrollHeight / 2; + const probe = { + content: element.querySelector(contentSelector), + loadingSeen: false, + record(records) { + for (const record of records) { + for (const node of record.addedNodes) { + if ( + node instanceof Element && + (node.matches(".loading-panel") || node.querySelector(".loading-panel")) + ) { + probe.loadingSeen = true; + } + } + } + }, + }; + probe.observer = new MutationObserver((records) => probe.record(records)); + probe.observer.observe(element, { childList: true, subtree: true }); + window.__reviewViewerReloadProbe = probe; + return scroller.scrollTop; + }, { contentSelector, scrollSelector }); +} + +async function readViewerReload(viewer, contentSelector, scrollSelector) { + return viewer.evaluate((element, { contentSelector, scrollSelector }) => { + const probe = window.__reviewViewerReloadProbe; + probe.record(probe.observer.takeRecords()); + const content = element.querySelector(contentSelector); + return { + reloaded: Boolean(content) && content !== probe.content, + loadingSeen: probe.loadingSeen, + top: content?.querySelector(scrollSelector)?.scrollTop ?? null, + }; + }, { contentSelector, scrollSelector }); +} diff --git a/frontend/tests/e2e/tasks/review-lifecycle.spec.js b/frontend/tests/e2e/tasks/review-lifecycle.spec.js index 2e29fdfe..7c5629f1 100644 --- a/frontend/tests/e2e/tasks/review-lifecycle.spec.js +++ b/frontend/tests/e2e/tasks/review-lifecycle.spec.js @@ -104,6 +104,35 @@ test("reopens the selected Review scope at its last semantic route", { tag: "@al ).toHaveAttribute("aria-current", "true"); }); +test("reads Branch again when it is chosen after Review reopens on Working Tree", { tag: "@viewport-independent" }, async ({ + page, +}) => { + const { reviewScenario, taskScenario, tasksPage, taskReview } = + await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanBranch = true; + }, + }); + await selectTaskReviewScope(tasksPage, "branch"); + const compareTree = taskReview.locator("caffold-git-compare-tree"); + await expect(compareTree).toContainText("No changes compared with origin/main."); + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + await tasksPage.getByRole("button", { name: "Conversation", exact: true }).click(); + await expect(page).toHaveURL(`/tasks/${taskScenario.threadId}`); + + reviewScenario.cleanBranch = false; + const statusBefore = reviewScenario.gitStatusRequests; + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 1); + + const comparesBefore = reviewScenario.gitCompareRequests; + await selectTaskReviewScope(tasksPage, "branch"); + await expect( + compareTree.locator('button[data-file-tree-path="src/planner.rs"]'), + ).toBeVisible(); + expect(reviewScenario.gitCompareRequests).toBe(comparesBefore + 1); +}); + test("keeps the selected Review viewer mounted during canonical task sync", { tag: "@all-viewports" }, async ({ page, }) => { From 236101951d4a5840eb81b3c344b0cd1a465f7f44 Mon Sep 17 00:00:00 2001 From: Taehoon Moon Date: Fri, 23 Oct 2026 01:36:36 +0900 Subject: [PATCH 2/2] Split Review's Working Tree, Branch, and file viewer into components The Review layout read Working Tree status, the Branch compare, and the open file itself. Each now has its own component with the read state after a return as an explicit model, and the layout keeps the route, selection, and Git watch. Retrying a failed Branch reread now shows loading in the viewer instead of the last error, and Working Tree's loading text is not redrawn when another reread starts. --- caffold/src/static_assets.rs | 96 +- docs/architecture/frontend.md | 53 +- frontend/components/git-compare-browser.js | 2 +- .../compare-tree.css => git-compare-tree.css} | 0 .../compare-tree.js => git-compare-tree.js} | 8 +- ...-tree.test.js => git-compare-tree.test.js} | 4 +- .../(detail)/(review)/changes-reading.js | 100 ++ .../(detail)/(review)/changes-reading.test.js | 82 ++ .../(review)/components/branch-changes.css | 9 + .../(review)/components/branch-changes.js | 297 ++++++ .../components/branch-changes.test.js | 79 ++ .../(review)/components/selected-file.css | 9 + .../(review)/components/selected-file.js | 277 +++++ .../(review)/components/selected-file.test.js | 122 +++ .../components/selected-file/content.js | 66 ++ .../components/selected-file/content.test.js | 92 ++ .../components/working-tree-changes.css | 37 + .../components/working-tree-changes.js | 279 +++++ .../components/working-tree-changes.test.js | 83 ++ .../components}/changes-tree.css | 0 .../components}/changes-tree.js | 0 .../components}/changes-tree.test.js | 2 +- .../tasks/(detail)/(review)/layout.css | 34 +- .../tasks/(detail)/(review)/layout.js | 992 ++++-------------- .../tasks/(detail)/(review)/layout.test.js | 146 +-- .../tasks/(detail)/(review)/layout/paths.js | 73 ++ .../tasks/(detail)/(review)/layout/route.js | 72 ++ .../(detail)/(review)/representations.js | 46 + frontend/service-worker.js | 19 +- frontend/styles.css | 7 +- .../tests/contracts/css-ownership.test.mjs | 25 +- .../contracts/review-responsive.test.mjs | 2 +- .../task-workspace-structure.test.mjs | 6 +- frontend/tests/e2e/app-shell.spec.js | 2 +- frontend/tests/e2e/tasks/review-git.spec.js | 113 +- 35 files changed, 2191 insertions(+), 1043 deletions(-) rename frontend/components/{git-compare-browser/compare-tree.css => git-compare-tree.css} (100%) rename frontend/components/{git-compare-browser/compare-tree.js => git-compare-tree.js} (98%) rename frontend/components/{git-compare-browser/compare-tree.test.js => git-compare-tree.test.js} (98%) create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.test.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.test.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.test.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.test.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.test.js rename frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/{ => working-tree-changes/components}/changes-tree.css (100%) rename frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/{ => working-tree-changes/components}/changes-tree.js (100%) rename frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/{ => working-tree-changes/components}/changes-tree.test.js (94%) create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/paths.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/route.js create mode 100644 frontend/pages/(task-workspace)/tasks/(detail)/(review)/representations.js diff --git a/caffold/src/static_assets.rs b/caffold/src/static_assets.rs index 1ef9567e..18f102dd 100644 --- a/caffold/src/static_assets.rs +++ b/caffold/src/static_assets.rs @@ -1152,14 +1152,65 @@ pub(crate) fn get(path: &str) -> Option { "pages/(task-workspace)/tasks/(detail)/(review)/layout.js" => Some(js(include_str!( "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js" ))), - "pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css" => { + "pages/(task-workspace)/tasks/(detail)/(review)/layout/paths.js" => Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/paths.js" + ))), + "pages/(task-workspace)/tasks/(detail)/(review)/layout/route.js" => Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/route.js" + ))), + "pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.js" => { + Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.js" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/representations.js" => { + Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/representations.js" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css" => { Some(css(include_str!( - "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css" + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css" ))) } - "pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.js" => { + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js" => { Some(js(include_str!( - "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.js" + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css" => { + Some(css(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js" => { + Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css" => { + Some(css(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js" => { + Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.js" => { + Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.js" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css" => { + Some(css(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css" + ))) + } + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.js" => { + Some(js(include_str!( + "../../frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.js" ))) } "pages/(task-workspace)/tasks/components/active-task-list.css" => Some(css(include_str!( @@ -1268,11 +1319,11 @@ pub(crate) fn get(path: &str) -> Option { "components/git-compare-browser.js" => Some(js(include_str!( "../../frontend/components/git-compare-browser.js" ))), - "components/git-compare-browser/compare-tree.css" => Some(css(include_str!( - "../../frontend/components/git-compare-browser/compare-tree.css" + "components/git-compare-tree.css" => Some(css(include_str!( + "../../frontend/components/git-compare-tree.css" ))), - "components/git-compare-browser/compare-tree.js" => Some(js(include_str!( - "../../frontend/components/git-compare-browser/compare-tree.js" + "components/git-compare-tree.js" => Some(js(include_str!( + "../../frontend/components/git-compare-tree.js" ))), "pages/(task-workspace)/tasks/(detail)/(git)/(log)/layout.css" => Some(css(include_str!( "../../frontend/pages/(task-workspace)/tasks/(detail)/(git)/(log)/layout.css" @@ -2033,8 +2084,7 @@ mod tests { .windows(b"caffold-git-compare-browser".len()) .any(|window| window == b"caffold-git-compare-browser") ); - let compare_tree = - get("components/git-compare-browser/compare-tree.js").expect("compare tree js"); + let compare_tree = get("components/git-compare-tree.js").expect("compare tree js"); assert_eq!(compare_tree.content_type, "text/javascript; charset=utf-8"); assert!( compare_tree @@ -2480,6 +2530,18 @@ mod tests { "pages/(task-workspace)/tasks/(detail)/(review)/layout.js", b"caffold-task-review".as_slice(), ), + ( + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js", + b"caffold-review-working-tree-changes".as_slice(), + ), + ( + "pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js", + b"caffold-review-branch-changes".as_slice(), + ), + ( + "pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js", + b"caffold-review-selected-file".as_slice(), + ), ] { let asset = get(path).expect("tasks component js"); assert_eq!(asset.content_type, "text/javascript; charset=utf-8"); @@ -2707,6 +2769,18 @@ mod tests { "pages/(task-workspace)/tasks/(detail)/(review)/layout.css", b"caffold-task-review".as_slice(), ), + ( + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css", + b"caffold-review-working-tree-changes".as_slice(), + ), + ( + "pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css", + b"caffold-review-branch-changes".as_slice(), + ), + ( + "pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css", + b"caffold-review-selected-file".as_slice(), + ), ( "pages/(task-workspace)/tasks/components/task-status.css", b"caffold-tasks-page".as_slice(), @@ -2720,7 +2794,7 @@ mod tests { assert!(get("pages/tasks/page.css").is_none()); let diff_changes_tree = - get("pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.js") + get("pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.js") .expect("integrated review changes tree js"); assert_eq!( diff_changes_tree.content_type, diff --git a/docs/architecture/frontend.md b/docs/architecture/frontend.md index bc3ad0de..7ad9b84f 100644 --- a/docs/architecture/frontend.md +++ b/docs/architecture/frontend.md @@ -1284,10 +1284,26 @@ to [Product Workflows](../product/workflows.md#current-plan-documents). review. It owns: - scope, navigator, viewer, base ref, and selected path; -- Git status, branch refs/compare, file, diff, and source requests; -- one root watch while active; +- one root watch while active, whose changes and recovery it hands to its + children; - Changes/Files, Diff, and file-capability-aware Source/Preview reconciliation; -- disclosure, selection, and scroll. +- the pane layout, the error notice, and each navigator's scroll. + +Three Review components below it own one part each, and each wraps a component +that only draws: + +- `caffold-review-working-tree-changes` reads Git status and lists the + uncommitted changes in `caffold-git-diff-changes-tree`, with the + Review branch changes action when there are none; +- `caffold-review-branch-changes` reads the branch refs and the comparison, + checks the chosen base, and lists the Task branch's changes in the shared + `caffold-git-compare-tree`; +- `caffold-review-selected-file` decides what `caffold-review-file-viewer` + shows for the selected file and representation, loads it, and keeps each + view's reading position. + +Both changes components answer the selected file's change and diff for the +viewer, and raise intents for the route changes they ask for. The Integrated Review owner resolves file-open intents to a path and supported representation together. Text files support Source, Markdown adds Preview, @@ -1438,15 +1454,19 @@ with every subject snapshot; a handoff with the same identity, roots, and route keeps the newer Task fields, such as branch and HEAD, for later requests and starts no request. -When its root watch recovers or a cached entry connects again, Integrated -Review treats the Working Tree status and the Branch refs and comparison it -holds as outdated, and drops answers to requests sent before that moment. It -reads the scope on screen again at once, and the other scope when it is -chosen. While outdated data is read again, an empty list and the viewer's -"No changes in this scope." show loading instead, and the read's error if it -fails; a list with files and an open diff stay as they are until the answer -arrives. After reading Git data again, the viewer reloads a diff, source, or -preview already on screen without the loading state and keeps its reading +Each changes component tracks how far its read has come in one explicit graph +(`changes-reading.js`): no data, a first read, a failed first read, data read +since Review last came back, data from before that, a reread of it, or a +failed reread. When the root watch recovers or a cached entry connects again, +both components mark their data as from before the return and drop answers to +requests sent before it. Review reads the scope on screen again at once, and +the other scope when it is chosen. While data from before the return is read +again, an empty list and the viewer's "No changes in this scope." show +loading, and the read's error if it fails; a list with files and an open diff +stay as they are until the answer arrives. Loading or an error replaces a list +only when that choice changes, so one reread following another keeps its +loading text. After reading Git data again, the viewer reloads a diff, source, +or preview already on screen without the loading state and keeps its reading position. ### Git @@ -1933,7 +1953,13 @@ frontend/ | | |-- conversation-shortcuts/components/fork-dialog.js | | |-- github-shortcuts.js | | `-- summary.js -| |-- (review)/layout.js +| |-- (review)/ +| | |-- layout.js +| | |-- changes-reading.js +| | `-- components/ +| | |-- working-tree-changes.js +| | |-- branch-changes.js +| | `-- selected-file.js | |-- (git)/ | | |-- layout.js | | |-- compare/page.js @@ -1948,6 +1974,7 @@ frontend/ |-- file-navigator.js |-- file-viewer.js |-- git-compare-browser.js + |-- git-compare-tree.js |-- markdown-preview.js |-- pagination.js |-- pdf-viewer.js diff --git a/frontend/components/git-compare-browser.js b/frontend/components/git-compare-browser.js index d7400668..c67b7289 100644 --- a/frontend/components/git-compare-browser.js +++ b/frontend/components/git-compare-browser.js @@ -1,7 +1,7 @@ import { getGitCompare, getGitCompareDiff, getGitRefs } from "../api.js"; import { diffViewerPresentation } from "./file-viewer-presentation.js"; import "./file-viewer.js"; -import "./git-compare-browser/compare-tree.js"; +import "./git-compare-tree.js"; import "./pane-resizer.js"; import { REVIEW_SINGLE_PANE_MEDIA_QUERY } from "./review-responsive.js"; import { diff --git a/frontend/components/git-compare-browser/compare-tree.css b/frontend/components/git-compare-tree.css similarity index 100% rename from frontend/components/git-compare-browser/compare-tree.css rename to frontend/components/git-compare-tree.css diff --git a/frontend/components/git-compare-browser/compare-tree.js b/frontend/components/git-compare-tree.js similarity index 98% rename from frontend/components/git-compare-browser/compare-tree.js rename to frontend/components/git-compare-tree.js index 1f91d80e..8909207d 100644 --- a/frontend/components/git-compare-browser/compare-tree.js +++ b/frontend/components/git-compare-tree.js @@ -1,14 +1,14 @@ import { buildFileTreeNodes, FILE_TREE_SELECT_EVENT, -} from "../file-tree.js"; +} from "./file-tree.js"; import { emptyActionHintScope, mergeActionHintScopes, selectActionHintTarget, -} from "../../action-hint-scope.js"; -import { emptyScrollSurfaceScope } from "../../scroll-scope.js"; -import { showLoadingText } from "../loading-text.js"; +} from "../action-hint-scope.js"; +import { emptyScrollSurfaceScope } from "../scroll-scope.js"; +import { showLoadingText } from "./loading-text.js"; class CaffoldGitCompareTree extends HTMLElement { connectedCallback() { diff --git a/frontend/components/git-compare-browser/compare-tree.test.js b/frontend/components/git-compare-tree.test.js similarity index 98% rename from frontend/components/git-compare-browser/compare-tree.test.js rename to frontend/components/git-compare-tree.test.js index a797cd5c..2890008d 100644 --- a/frontend/components/git-compare-browser/compare-tree.test.js +++ b/frontend/components/git-compare-tree.test.js @@ -3,10 +3,10 @@ import test, { after } from "node:test"; import { installCustomElementUnitRegistry, -} from "../../tests/support/custom-element-unit.js"; +} from "../tests/support/custom-element-unit.js"; const registry = installCustomElementUnitRegistry(); -await import("./compare-tree.js"); +await import("./git-compare-tree.js"); const compareTree = registry.element("caffold-git-compare-tree").prototype; after(() => registry.restore()); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.js new file mode 100644 index 00000000..dbd301b4 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.js @@ -0,0 +1,100 @@ +// How far a Working Tree or Branch read has come: whether it holds data, +// whether that data was read before the page or Review last came back, and +// whether a first read or a reread is running or has failed. The data itself, +// the last error, and request generations stay with the changes component. + +const EMPTY = "empty"; +const LOADING = "loading"; +const FAILED = "failed"; +const CURRENT = "current"; +const OUTDATED = "outdated"; +const REREADING = "rereading"; +const REREAD_FAILED = "rereadFailed"; + +// The complete transition table; an event a node does not list is rejected. +// A completion can only arrive for a read started after the last return or +// reset, because both invalidate earlier requests. +const EDGES = Object.freeze({ + [EMPTY]: { start: LOADING, return: EMPTY, reset: EMPTY }, + [LOADING]: { + start: LOADING, + succeed: CURRENT, + fail: FAILED, + return: EMPTY, + reset: EMPTY, + }, + [FAILED]: { start: LOADING, return: FAILED, reset: EMPTY }, + [CURRENT]: { + start: CURRENT, + succeed: CURRENT, + fail: CURRENT, + return: OUTDATED, + reset: EMPTY, + }, + [OUTDATED]: { start: REREADING, return: OUTDATED, reset: EMPTY }, + [REREADING]: { + start: REREADING, + succeed: CURRENT, + fail: REREAD_FAILED, + return: OUTDATED, + reset: EMPTY, + }, + [REREAD_FAILED]: { start: REREADING, return: REREAD_FAILED, reset: EMPTY }, +}); + +export class ChangesReading { + #node = EMPTY; + + get state() { + return STATES[this.#node]; + } + + start() { + return this.#apply("start"); + } + + succeed() { + return this.#apply("succeed"); + } + + fail() { + return this.#apply("fail"); + } + + markReturned() { + return this.#apply("return"); + } + + reset() { + return this.#apply("reset"); + } + + #apply(event) { + const previous = this.state; + const next = EDGES[this.#node][event]; + if (!next) { + return { accepted: false, previous }; + } + this.#node = next; + return { accepted: true, previous }; + } +} + +const STATES = Object.freeze({ + [EMPTY]: state({}), + [LOADING]: state({ reading: true }), + [FAILED]: state({ failed: true }), + [CURRENT]: state({ hasData: true }), + [OUTDATED]: state({ hasData: true, fromBeforeReturn: true }), + [REREADING]: state({ hasData: true, fromBeforeReturn: true, reading: true }), + [REREAD_FAILED]: state({ hasData: true, fromBeforeReturn: true, failed: true }), +}); + +function state({ + hasData = false, + fromBeforeReturn = false, + reading = false, + failed = false, +}) { + return Object.freeze({ hasData, fromBeforeReturn, reading, failed }); +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.test.js new file mode 100644 index 00000000..0fa2b299 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.test.js @@ -0,0 +1,82 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { ChangesReading } from "./changes-reading.js"; + +const facts = { + empty: { hasData: false, fromBeforeReturn: false, reading: false, failed: false }, + loading: { hasData: false, fromBeforeReturn: false, reading: true, failed: false }, + failed: { hasData: false, fromBeforeReturn: false, reading: false, failed: true }, + current: { hasData: true, fromBeforeReturn: false, reading: false, failed: false }, + outdated: { hasData: true, fromBeforeReturn: true, reading: false, failed: false }, + rereading: { hasData: true, fromBeforeReturn: true, reading: true, failed: false }, + rereadFailed: { hasData: true, fromBeforeReturn: true, reading: false, failed: true }, +}; + +// The events that lead from a new reading to each state. +const paths = { + empty: [], + loading: ["start"], + failed: ["start", "fail"], + current: ["start", "succeed"], + outdated: ["start", "succeed", "markReturned"], + rereading: ["start", "succeed", "markReturned", "start"], + rereadFailed: ["start", "succeed", "markReturned", "start", "fail"], +}; + +// Where each event leads from each state; null means the event is rejected. +const edges = { + empty: { start: "loading", succeed: null, fail: null, markReturned: "empty", reset: "empty" }, + loading: { start: "loading", succeed: "current", fail: "failed", markReturned: "empty", reset: "empty" }, + failed: { start: "loading", succeed: null, fail: null, markReturned: "failed", reset: "empty" }, + current: { start: "current", succeed: "current", fail: "current", markReturned: "outdated", reset: "empty" }, + outdated: { start: "rereading", succeed: null, fail: null, markReturned: "outdated", reset: "empty" }, + rereading: { start: "rereading", succeed: "current", fail: "rereadFailed", markReturned: "outdated", reset: "empty" }, + rereadFailed: { start: "rereading", succeed: null, fail: null, markReturned: "rereadFailed", reset: "empty" }, +}; + +function readingAt(name) { + const reading = new ChangesReading(); + for (const event of paths[name]) { + assert.equal(reading[event]().accepted, true, `${name} path ${event}`); + } + assert.deepEqual(reading.state, facts[name], `reaches ${name}`); + return reading; +} + +test("reaches every reading state with distinct facts", () => { + for (const name of Object.keys(facts)) { + readingAt(name); + } + const distinct = new Set(Object.values(facts).map((value) => JSON.stringify(value))); + assert.equal(distinct.size, Object.keys(facts).length); +}); + +test("follows every allowed transition and rejects the rest without moving", () => { + for (const [from, events] of Object.entries(edges)) { + for (const [event, to] of Object.entries(events)) { + const reading = readingAt(from); + const result = reading[event](); + assert.deepEqual(result.previous, facts[from], `${from} ${event} previous`); + if (to === null) { + assert.equal(result.accepted, false, `${from} rejects ${event}`); + assert.deepEqual(reading.state, facts[from], `${from} stays after ${event}`); + } else { + assert.equal(result.accepted, true, `${from} accepts ${event}`); + assert.deepEqual(reading.state, facts[to], `${from} ${event} -> ${to}`); + } + } + } +}); + +test("a completion after a return cannot make data from before it current", () => { + const reading = readingAt("current"); + reading.markReturned(); + assert.equal(reading.succeed().accepted, false); + assert.deepEqual(reading.state, facts.outdated); + reading.start(); + reading.markReturned(); + assert.deepEqual(reading.state, facts.outdated); + assert.equal(reading.fail().accepted, false); + assert.deepEqual(reading.state, facts.outdated); +}); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css new file mode 100644 index 00000000..e41ee4d9 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css @@ -0,0 +1,9 @@ +caffold-review-branch-changes { + display: grid; + + & > caffold-git-compare-tree { + min-width: 0; + min-height: 0; + height: 100%; + } +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js new file mode 100644 index 00000000..6c7fe2f2 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js @@ -0,0 +1,297 @@ +import { getGitCompare, getGitCompareDiff, getGitRefs } from "#app/api.js"; +import "#components/git-compare-tree.js"; +import { ChangesReading } from "../changes-reading.js"; + +class CaffoldReviewBranchChanges extends HTMLElement { + connectedCallback() { + this.ensureRendered(); + } + + disconnectedCallback() { + this.generation += 1; + } + + ensureRendered() { + if (this.rendered) { + return; + } + this.rendered = true; + this.rootPath = ""; + this.branch = null; + this.baseRef = ""; + this.shown = false; + this.refs = null; + this.compare = null; + this.error = null; + this.reading = new ChangesReading(); + this.listShows = ""; + this.generation = 0; + this.selectedPath = ""; + + this.innerHTML = ``; + + this.addEventListener("caffold:select-compare-base", (event) => { + event.stopPropagation(); + this.selectBase(event.detail?.baseRef); + }); + } + + setContext({ rootPath = "", branch = null, baseRef = "" } = {}) { + this.ensureRendered(); + this.branch = branch; + if (rootPath !== this.rootPath) { + this.rootPath = rootPath; + this.baseRef = baseRef; + this.refs = null; + this.compare = null; + this.error = null; + this.reading.reset(); + this.listShows = ""; + this.generation += 1; + this.tree()?.reset(); + return; + } + if (baseRef !== this.baseRef) { + this.baseRef = baseRef; + this.compare = null; + this.error = null; + this.reading.reset(); + } + } + + setShown(shown) { + this.ensureRendered(); + this.shown = Boolean(shown); + } + + // Git may have changed while the page was away or Review was hidden, so + // the refs and comparison held are outdated until a request sent after + // this point answers. + markOutdated() { + this.ensureRendered(); + this.reading.markReturned(); + this.generation += 1; + this.showReadingState(); + } + + forgetRefs() { + this.ensureRendered(); + this.refs = null; + } + + async read({ force = false } = {}) { + const rootPath = this.rootPath; + if (!rootPath) { + return null; + } + const generation = ++this.generation; + this.reading.start(); + this.showReadingState(); + try { + const refs = force || !this.refs ? await getGitRefs(rootPath) : this.refs; + if (!this.acceptsRead(generation, rootPath)) { + return null; + } + this.refs = refs; + const baseRef = selectedCompareBaseRef(this.baseRef, refs); + this.tree()?.setBaseSelection({ refs: refs.refs, value: baseRef }); + if (this.baseRef !== baseRef) { + this.requestBase(baseRef); + return null; + } + if (!this.compare) { + this.tree()?.setLoading(refs.repository, { + baseRef, + headRef: this.headRef(refs), + }); + this.listShows = "loading"; + } + const compare = await getGitCompare(rootPath, baseRef, this.headRef(refs)); + if (!this.acceptsRead(generation, rootPath)) { + return null; + } + const { accepted, previous } = this.reading.succeed(); + if (!accepted) { + return null; + } + this.compare = compare; + this.error = null; + this.tree()?.setEmptyMessage( + `No changes compared with ${baseRef || "the selected base"}.`, + ); + if (previous.hasData) { + this.tree()?.updateCompare(compare); + } else { + this.tree()?.setCompare(compare); + } + this.listShows = "data"; + this.tree()?.setSelectedPath(this.selectedPath); + this.announceRead({ + ok: true, + hadData: previous.hasData, + wasOutdated: previous.fromBeforeReturn, + }); + return compare; + } catch (error) { + if (!this.acceptsRead(generation, rootPath)) { + return null; + } + if (!this.reading.fail().accepted) { + return null; + } + this.error = error; + this.showReadingState(); + this.announceRead({ ok: false, error }); + return null; + } + } + + snapshot() { + this.ensureRendered(); + const { fromBeforeReturn, failed } = this.reading.state; + return { + data: this.compare, + error: this.error, + outdated: fromBeforeReturn, + failed, + hasRefs: Boolean(this.refs), + }; + } + + change(path) { + const file = (this.compare?.files ?? []).find((entry) => entry.path === path); + if (!file) { + return null; + } + return { + file, + deleted: `${file.status ?? ""}`.includes("D"), + kind: file.untracked ? "untracked" : file.category || "unstaged", + }; + } + + readDiff(path) { + return getGitCompareDiff(this.rootPath, this.baseRef, this.headRef(), path); + } + + diffKind() { + return `${this.baseRef}...${this.headRef()}`; + } + + setSelectedPath(path) { + this.ensureRendered(); + this.selectedPath = path ?? ""; + this.tree()?.setSelectedPath(this.selectedPath); + } + + captureListScroll() { + return this.tree()?.captureListScroll() ?? null; + } + + restoreListScroll(scroll) { + this.tree()?.restoreListScroll(scroll); + } + + actionHintScope(options = {}) { + return this.tree()?.actionHintScope(options); + } + + scrollSurfaceScope(options = {}) { + return this.tree()?.scrollSurfaceScope(options); + } + + selectBase(baseRef) { + const next = `${baseRef ?? ""}`; + if ( + !next || + next === this.baseRef || + !this.refs?.refs?.some((ref) => ref.name === next) + ) { + return; + } + this.requestBase(next); + } + + requestBase(baseRef) { + this.dispatchEvent( + new CustomEvent("caffold:review-base-intent", { + bubbles: true, + detail: { baseRef }, + }), + ); + } + + headRef(refs = this.refs) { + return this.branch || refs?.defaultHeadRef || "HEAD"; + } + + acceptsRead(generation, rootPath) { + return ( + this.isConnected && + this.shown && + generation === this.generation && + rootPath === this.rootPath + ); + } + + announceRead(detail) { + this.dispatchEvent( + new CustomEvent("caffold:review-changes-read", { bubbles: true, detail }), + ); + } + + // The list shows the read's error where it holds no comparison, and loading + // or the read's error where it holds only a comparison from before the page + // came back that lists no changes. It is drawn again only when that choice + // changes. A first comparison shows loading once the refs name its base. + showReadingState() { + const { hasData, fromBeforeReturn, reading, failed } = this.reading.state; + const hidesData = + !hasData || (fromBeforeReturn && !this.compare?.files?.length); + const shows = !hidesData + ? "data" + : !hasData + ? failed ? "error" : this.listShows + : reading + ? "loading" + : failed + ? "error" + : "data"; + if (shows === this.listShows) { + return; + } + this.listShows = shows; + const repository = this.refs?.repository ?? null; + if (shows === "loading") { + this.tree()?.setLoading(repository, { + baseRef: this.baseRef, + headRef: this.headRef(), + }); + } else if (shows === "error") { + this.tree()?.setError(this.error, repository); + } else if (shows === "data") { + this.tree()?.updateCompare(this.compare); + } + } + + tree() { + return this.querySelector(":scope > caffold-git-compare-tree"); + } +} + +if (!customElements.get("caffold-review-branch-changes")) { + customElements.define("caffold-review-branch-changes", CaffoldReviewBranchChanges); +} + +function selectedCompareBaseRef(preferredRef, refsPayload = {}) { + const refs = Array.isArray(refsPayload.refs) ? refsPayload.refs : []; + const hasRef = (candidate) => + Boolean(candidate && refs.some((ref) => ref.name === candidate)); + if (hasRef(preferredRef)) { + return `${preferredRef}`; + } + if (hasRef(refsPayload.defaultBaseRef)) { + return `${refsPayload.defaultBaseRef}`; + } + return refs[0]?.name || refsPayload.defaultBaseRef || ""; +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.test.js new file mode 100644 index 00000000..bfb20efd --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.test.js @@ -0,0 +1,79 @@ +import assert from "node:assert/strict"; +import test, { after } from "node:test"; + +import { + installCustomElementUnitRegistry, +} from "../../../../../../tests/support/custom-element-unit.js"; +import { ChangesReading } from "../changes-reading.js"; + +const registry = installCustomElementUnitRegistry(); +await import("./branch-changes.js"); +const branchChanges = registry.element("caffold-review-branch-changes").prototype; +after(() => registry.restore()); + +test("drops a Branch compare that returns after Review leaves Branch", async (t) => { + const previousFetch = globalThis.fetch; + const previousWindow = globalThis.window; + const compares = []; + globalThis.window = { location: { origin: "http://caffold.test" } }; + globalThis.fetch = (input, ...options) => { + if (new URL(input).pathname !== "/api/git/compare") { + return previousFetch(input, ...options); + } + const response = Promise.withResolvers(); + compares.push(response); + return response.promise; + }; + t.after(() => { + globalThis.fetch = previousFetch; + if (previousWindow === undefined) delete globalThis.window; + else globalThis.window = previousWindow; + }); + const payload = { + baseRef: "origin/main", + headRef: "main", + files: [{ path: "src/planner.rs", repoRelativePath: "planner.rs", status: "M" }], + }; + const drawn = []; + const tree = { + setBaseSelection() {}, + setLoading() {}, + setEmptyMessage() {}, + setSelectedPath() {}, + setCompare: (compare) => drawn.push(compare), + updateCompare: (compare) => drawn.push(compare), + }; + const owner = { + isConnected: true, + shown: true, + rootPath: "/repo", + branch: "main", + baseRef: "origin/main", + refs: { + repository: { rootPath: "/repo" }, + refs: [{ name: "origin/main", kind: "remote" }], + defaultBaseRef: "origin/main", + }, + compare: null, + error: null, + reading: new ChangesReading(), + generation: 0, + selectedPath: "", + acceptsRead: branchChanges.acceptsRead, + showReadingState() {}, + headRef: branchChanges.headRef, + announceRead() {}, + tree: () => tree, + }; + + const current = branchChanges.read.call(owner); + compares.at(-1).resolve(new Response(JSON.stringify(payload))); + assert.deepEqual(await current, payload); + assert.equal(drawn.length, 1); + + const late = branchChanges.read.call(owner); + owner.shown = false; + compares.at(-1).resolve(new Response(JSON.stringify(payload))); + assert.equal(await late, null); + assert.equal(drawn.length, 1); +}); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css new file mode 100644 index 00000000..f375d7dc --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css @@ -0,0 +1,9 @@ +caffold-review-selected-file { + display: grid; + + & > caffold-review-file-viewer { + min-width: 0; + min-height: 0; + height: 100%; + } +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js new file mode 100644 index 00000000..55de7f3c --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js @@ -0,0 +1,277 @@ +import { readFile } from "#app/api.js"; +import { fileStatusPresentation } from "#app/file-status.js"; +import { fileNameFromPath, imageTypeLabel } from "#components/dom.js"; +import "#components/file-viewer.js"; +import { + diffViewerPresentation, + sourceViewerPresentation, +} from "#components/file-viewer-presentation.js"; +import { contentFor } from "./selected-file/content.js"; + +class CaffoldReviewSelectedFile extends HTMLElement { + connectedCallback() { + this.ensureRendered(); + } + + disconnectedCallback() { + this.viewer()?.deactivate?.(); + this.generation += 1; + } + + ensureRendered() { + if (this.rendered) { + return; + } + this.rendered = true; + this.rootPath = ""; + this.worktreeRootPath = ""; + this.view = null; + this.generation = 0; + this.shownStateKey = ""; + this.positions = new Map(); + this.pendingLines = new Map(); + + this.innerHTML = ``; + this.viewer()?.setCloseLabel("Back to navigator"); + this.viewer()?.setCloseMode("back"); + + this.addEventListener("click", (event) => { + const action = event.target instanceof Element + ? event.target.closest('[data-action="view-source"], [data-action="view-preview"]') + : null; + if (!action || !this.contains(action)) { + return; + } + event.stopPropagation(); + this.dispatchEvent( + new CustomEvent("caffold:review-representation-intent", { + bubbles: true, + detail: { + representation: action.dataset.action === "view-preview" ? "preview" : "source", + }, + }), + ); + }); + } + + setContext({ rootPath = "", worktreeRootPath = "" } = {}) { + this.ensureRendered(); + if (rootPath === this.rootPath && worktreeRootPath === this.worktreeRootPath) { + return; + } + this.rootPath = rootPath; + this.worktreeRootPath = worktreeRootPath; + this.view = null; + this.generation += 1; + this.shownStateKey = ""; + this.viewer()?.setEmpty(); + } + + // `view` names the selected file in its representation, with what the + // shown Working Tree or Branch changes say about it. `force` loads it even + // when the viewer already holds something other than an empty or error state. + show(view, { force = false } = {}) { + this.ensureRendered(); + this.rememberPosition(view); + this.view = view; + if (!view.path) { + this.shownStateKey = ""; + this.viewer()?.setEmpty(); + return; + } + if (force || ["empty", "error"].includes(this.viewer()?.state?.status)) { + void this.load(); + } + } + + // A reread loads the view again in the background only when the viewer + // already shows it, so a first look still shows loading and the line it + // last left. + reload(view, { background = false, reread = false } = {}) { + this.ensureRendered(); + this.view = view; + void this.load({ + background: background || (reread && this.shownStateKey === view.stateKey), + }); + } + + actionHintScope(options = {}) { + return this.viewer()?.actionHintScope(options); + } + + scrollSurfaceScope(options = {}) { + return this.viewer()?.scrollSurfaceScope(options); + } + + keyboardNavigationContexts(options = {}) { + return this.viewer()?.keyboardNavigationContexts(options) ?? []; + } + + async load(options = {}) { + const view = this.view; + const viewer = this.viewer(); + if (!view?.path) { + this.shownStateKey = ""; + viewer?.setEmpty(); + return; + } + const generation = ++this.generation; + const { stateKey, logicalPath, line } = view; + // A background load follows a change the watch reported for the file, a + // recovered watch, or a reread of Git data while the view is on screen: + // it keeps the reading position and asks again whether a previewed file + // changed. Any other load shows the file the viewer already holds. + const background = Boolean(options.background); + if (!background) { + this.shownStateKey = ""; + } + const content = contentFor(view); + if (content.kind === "unchanged") { + return; + } + if (content.kind === "notice") { + this.shownStateKey = ""; + viewer?.setNotice(content.message, content.options); + return; + } + if (content.kind === "loading" || content.kind === "error") { + this.shownStateKey = ""; + const presentation = this.diffPresentation(view, null); + if (content.kind === "error") { + viewer?.setError(presentation, content.error); + } else { + viewer?.setLoading(presentation); + } + return; + } + if (content.kind === "document") { + viewer?.setDocument({ + kind: content.documentKind, + path: logicalPath, + name: fileNameFromPath(logicalPath), + size: view.entry?.size, + modifiedMs: view.entry?.modifiedMs, + }, { recheck: background }); + this.shownStateKey = stateKey; + return; + } + if (content.kind === "image") { + viewer?.setImage({ + path: logicalPath, + name: fileNameFromPath(logicalPath), + imageType: imageTypeLabel(logicalPath), + size: view.entry?.size, + modifiedMs: view.entry?.modifiedMs, + }, { recheck: background }); + this.shownStateKey = stateKey; + return; + } + const sourceLine = content.kind === "file" ? line : null; + const saved = this.positions.get(stateKey); + const presentation = content.kind === "diff" + ? this.diffPresentation(view, content.change) + : sourceViewerPresentation({ path: logicalPath }); + if (!background) { + viewer?.setLoading(presentation); + } + const viewerOptions = background + ? { preserveScroll: true } + : sourceLine + ? { line: sourceLine } + : { scroll: saved?.scroll ?? null }; + try { + if (content.kind === "diff") { + const diff = await view.changes.readDiff(); + if (!this.acceptsLoad(generation, logicalPath)) { + return; + } + viewer?.setDiff(diff, { + ...viewerOptions, + presentation: this.diffPresentation(view, content.change, diff), + }); + } else { + const file = await readFile(logicalPath); + if (!this.acceptsLoad(generation, logicalPath)) { + return; + } + if (content.kind === "markdown") { + viewer?.setMarkdown(file, { ...viewerOptions, imageRoot: this.rootPath }); + } else { + viewer?.setFile(file, viewerOptions); + } + } + this.shownStateKey = stateKey; + if (!background && !sourceLine) { + this.restorePosition(stateKey, saved); + } + } catch (error) { + if (this.acceptsLoad(generation, logicalPath)) { + this.shownStateKey = ""; + viewer?.setError(presentation, error); + } + } + } + + diffPresentation(view, change, diff = {}) { + return diffViewerPresentation({ + ...diff, + repository: diff.repository ?? { rootPath: this.worktreeRootPath }, + path: diff.path ?? view.logicalPath, + repoRelativePath: diff.repoRelativePath ?? view.path, + kind: diff.kind || view.changes.diffKind || change?.kind || "", + status: change?.file + ? fileStatusPresentation(change.file.status, change.file).code + : "", + }); + } + + acceptsLoad(generation, logicalPath) { + return ( + this.isConnected && + generation === this.generation && + logicalPath === this.view?.logicalPath + ); + } + + // Leaving a view keeps its position for a later return. Switching only the + // representation of the same file carries the visible line to the new one. + rememberPosition(nextView) { + const previous = this.view; + if (!previous?.path || previous.stateKey === nextView.stateKey) { + return; + } + const line = this.viewer()?.visibleLine?.() ?? null; + this.positions.set(previous.stateKey, { + scroll: this.viewer()?.captureContentScroll?.() ?? null, + line, + }); + if ( + line && + previous.scope === nextView.scope && + previous.path === nextView.path && + previous.representation !== nextView.representation + ) { + this.pendingLines.set(nextView.stateKey, line); + } + } + + restorePosition(stateKey, saved) { + const line = saved?.line ?? this.pendingLines.get(stateKey); + if (!line) { + return; + } + this.pendingLines.delete(stateKey); + requestAnimationFrame(() => { + this.viewer()?.scrollToLine(line); + window.setTimeout(() => this.viewer()?.scrollToLine(line), 40); + }); + } + + viewer() { + return this.querySelector(":scope > caffold-review-file-viewer"); + } +} + +if (!customElements.get("caffold-review-selected-file")) { + customElements.define("caffold-review-selected-file", CaffoldReviewSelectedFile); +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.test.js new file mode 100644 index 00000000..42671f07 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.test.js @@ -0,0 +1,122 @@ +import assert from "node:assert/strict"; +import test, { after } from "node:test"; + +import { + installCustomElementUnitRegistry, +} from "../../../../../../tests/support/custom-element-unit.js"; + +const registry = installCustomElementUnitRegistry(); +await import("./selected-file.js"); +const selectedFile = registry.element("caffold-review-selected-file").prototype; +after(() => registry.restore()); + +function diffView(path, readDiff) { + return { + stateKey: `working:${path}:diff`, + scope: "working", + path, + logicalPath: `repo/${path}`, + representation: "diff", + line: null, + entry: null, + changes: { + ready: true, + outdated: false, + error: null, + change: { + file: { path: `repo/${path}`, status: "M" }, + deleted: false, + kind: "unstaged", + }, + diffKind: "", + readDiff, + }, + }; +} + +test("drops a diff that returns after another file is shown", async () => { + const first = Promise.withResolvers(); + const second = Promise.withResolvers(); + const drawn = []; + const viewer = { + state: { status: "diff" }, + setLoading() {}, + setNotice() {}, + setError() {}, + setDiff: (diff) => drawn.push(diff.path), + }; + const owner = { + isConnected: true, + generation: 0, + shownStateKey: "", + positions: new Map(), + pendingLines: new Map(), + rootPath: "repo", + worktreeRootPath: "repo", + view: diffView("a.js", () => first.promise), + viewer: () => viewer, + diffPresentation: selectedFile.diffPresentation, + acceptsLoad: selectedFile.acceptsLoad, + restorePosition() {}, + }; + + const firstLoad = selectedFile.load.call(owner); + owner.view = diffView("b.js", () => second.promise); + const secondLoad = selectedFile.load.call(owner); + second.resolve({ path: "repo/b.js", diff: "" }); + await secondLoad; + first.resolve({ path: "repo/a.js", diff: "" }); + await firstLoad; + + assert.deepEqual(drawn, ["repo/b.js"]); + assert.equal(owner.shownStateKey, "working:b.js:diff"); +}); + +test("keeps the position of a view it leaves and carries the line to another representation", () => { + const owner = { + view: diffView("a.js"), + positions: new Map(), + pendingLines: new Map(), + viewer: () => ({ + visibleLine: () => 42, + captureContentScroll: () => ({ top: 10 }), + }), + }; + const source = { + ...diffView("a.js"), + stateKey: "working:a.js:source", + representation: "source", + }; + + selectedFile.rememberPosition.call(owner, source); + assert.deepEqual(owner.positions.get("working:a.js:diff"), { + scroll: { top: 10 }, + line: 42, + }); + assert.equal(owner.pendingLines.get("working:a.js:source"), 42); + + owner.view = source; + selectedFile.rememberPosition.call(owner, diffView("b.js")); + assert.equal(owner.pendingLines.has("working:b.js:diff"), false); +}); + +test("keeps the reading position on a reread only while it shows the same view", () => { + const loads = []; + const owner = { + shownStateKey: "working:a.js:diff", + ensureRendered() {}, + load: (options) => loads.push(options.background), + }; + const source = { + ...diffView("a.js"), + stateKey: "working:a.js:source", + representation: "source", + }; + + selectedFile.reload.call(owner, diffView("a.js"), { reread: true }); + selectedFile.reload.call(owner, source, { reread: true }); + selectedFile.reload.call(owner, source, { background: true }); + selectedFile.reload.call(owner, diffView("a.js")); + + assert.deepEqual(loads, [true, false, true, false]); +}); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.js new file mode 100644 index 00000000..6dadb228 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.js @@ -0,0 +1,66 @@ +import { fileNameFromPath } from "#components/dom.js"; +import { fileRepresentationCapabilities } from "../../representations.js"; + +// What the viewer shows for the selected file in its representation, from what +// the shown Working Tree or Branch changes say about that file. "unchanged" +// keeps whatever the viewer holds until the changes are read. +export function contentFor({ logicalPath, representation, changes }) { + const sourceMode = representation === "source"; + const previewMode = representation === "preview"; + const fileMode = sourceMode || previewMode; + if (!fileMode && !changes.ready) { + return { kind: "unchanged" }; + } + const title = fileNameFromPath(logicalPath); + const change = changes.change; + if (fileMode && change?.deleted) { + return notice( + "This file was deleted in the selected scope. Diff remains available.", + { title }, + ); + } + const representations = fileRepresentationCapabilities(logicalPath); + if (previewMode && representations.previewKind === "document") { + return { kind: "document", documentKind: representations.documentKind }; + } + if (previewMode && representations.previewKind === "image") { + return { kind: "image" }; + } + if (sourceMode && !representations.source) { + return notice("Source is unavailable for this file.", { + actionLabel: "View preview", + action: "view-preview", + title, + }); + } + if (previewMode && !representations.previewKind) { + return notice("Preview is unavailable for this file.", { + actionLabel: "View source", + action: "view-source", + title, + }); + } + if (!fileMode && !change) { + if (changes.outdated) { + return changes.failed + ? { kind: "error", error: changes.error } + : { kind: "loading" }; + } + return notice("No changes in this scope.", { + actionLabel: "View source", + action: "view-source", + title, + }); + } + if (previewMode) { + return { kind: "markdown" }; + } + if (sourceMode) { + return { kind: "file" }; + } + return { kind: "diff", change }; +} + +function notice(message, options) { + return { kind: "notice", message, options }; +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.test.js new file mode 100644 index 00000000..5db81d76 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.test.js @@ -0,0 +1,92 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { contentFor } from "./content.js"; + +const modified = { + file: { path: "repo/src/a.js", status: "M" }, + deleted: false, + kind: "unstaged", +}; + +function view(path, representation, changes = {}) { + return { + logicalPath: `repo/${path}`, + representation, + changes: { + ready: true, + outdated: false, + failed: false, + error: null, + change: null, + ...changes, + }, + }; +} + +test("waits for the changes before deciding a diff", () => { + assert.deepEqual( + contentFor(view("src/a.js", "diff", { ready: false })), + { kind: "unchanged" }, + ); + assert.equal(contentFor(view("src/a.js", "source", { ready: false })).kind, "file"); +}); + +test("loads a changed file's diff, source, or Markdown preview", () => { + assert.deepEqual( + contentFor(view("src/a.js", "diff", { change: modified })), + { kind: "diff", change: modified }, + ); + assert.deepEqual(contentFor(view("src/a.js", "source")), { kind: "file" }); + assert.deepEqual(contentFor(view("README.md", "preview")), { kind: "markdown" }); +}); + +test("hands images and documents to the viewer", () => { + assert.deepEqual(contentFor(view("art/logo.png", "preview")), { kind: "image" }); + assert.deepEqual( + contentFor(view("docs/report.pdf", "preview")), + { kind: "document", documentKind: "pdf" }, + ); +}); + +test("explains files the scope or representation cannot show", () => { + assert.deepEqual( + contentFor(view("src/a.js", "source", { change: { ...modified, deleted: true } })), + { + kind: "notice", + message: "This file was deleted in the selected scope. Diff remains available.", + options: { title: "a.js" }, + }, + ); + assert.deepEqual(contentFor(view("art/logo.png", "source")), { + kind: "notice", + message: "Source is unavailable for this file.", + options: { actionLabel: "View preview", action: "view-preview", title: "logo.png" }, + }); + assert.deepEqual(contentFor(view("src/a.js", "preview")), { + kind: "notice", + message: "Preview is unavailable for this file.", + options: { actionLabel: "View source", action: "view-source", title: "a.js" }, + }); + assert.deepEqual(contentFor(view("src/a.js", "diff")), { + kind: "notice", + message: "No changes in this scope.", + options: { actionLabel: "View source", action: "view-source", title: "a.js" }, + }); +}); + +test("does not say a file is unchanged from changes read before the page came back", () => { + const error = new Error("compare unavailable"); + assert.deepEqual( + contentFor(view("src/a.js", "diff", { outdated: true })), + { kind: "loading" }, + ); + assert.deepEqual( + contentFor(view("src/a.js", "diff", { outdated: true, failed: true, error })), + { kind: "error", error }, + ); + assert.deepEqual( + contentFor(view("src/a.js", "diff", { outdated: true, change: modified })), + { kind: "diff", change: modified }, + ); +}); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css new file mode 100644 index 00000000..649258cf --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css @@ -0,0 +1,37 @@ +caffold-review-working-tree-changes { + display: grid; + + & > caffold-git-diff-changes-tree { + min-width: 0; + min-height: 0; + height: 100%; + } + + & > .working-tree-changes-empty-action { + position: absolute; + z-index: 2; + inset: calc( + var(--review-header-height) + var(--interface-space-16) + var(--interface-space-8) + ) + var(--interface-space-5) auto; + box-sizing: border-box; + width: fit-content; + max-width: calc(100% - var(--interface-space-10)); + margin: 0; + padding: var(--interface-space-5); + border: 1px solid var(--border); + border-radius: 6px; + background: var(--surface); + box-shadow: 0 8px 24px rgb(var(--shadow-rgb) / 10%); + color: var(--muted); + font-size: var(--interface-meta-font-size); + } + + & > .working-tree-changes-empty-action[hidden] { + display: none; + } + + & > .working-tree-changes-empty-action p { + margin: 0 0 var(--interface-space-4); + } +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js new file mode 100644 index 00000000..8a687570 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js @@ -0,0 +1,279 @@ +import { getGitDiff, getGitStatus } from "#app/api.js"; +import { + buttonActionHintTarget, + mergeActionHintScopes, +} from "#app/action-hints.js"; +import { ChangesReading } from "../changes-reading.js"; +import "./working-tree-changes/components/changes-tree.js"; + +const BRANCH_ACTION_SELECTOR = + ':scope > .working-tree-changes-empty-action:not([hidden]) > button[data-review-action="review-branch"]'; + +class CaffoldReviewWorkingTreeChanges extends HTMLElement { + connectedCallback() { + this.ensureRendered(); + } + + disconnectedCallback() { + this.generation += 1; + } + + ensureRendered() { + if (this.rendered) { + return; + } + this.rendered = true; + this.rootPath = ""; + this.branch = null; + this.status = null; + this.error = null; + this.reading = new ChangesReading(); + this.listShows = ""; + this.generation = 0; + this.selectedPath = ""; + + this.innerHTML = ` + + + `; + + this.addEventListener("click", (event) => { + const button = event.target instanceof Element + ? event.target.closest('[data-review-action="review-branch"]') + : null; + if (!button || !this.contains(button)) { + return; + } + event.stopPropagation(); + this.dispatchEvent( + new CustomEvent("caffold:review-branch-intent", { bubbles: true }), + ); + }); + } + + setContext({ rootPath = "", branch = null } = {}) { + this.ensureRendered(); + this.branch = branch; + if (rootPath === this.rootPath) { + return; + } + this.rootPath = rootPath; + this.status = null; + this.error = null; + this.reading.reset(); + this.listShows = ""; + this.generation += 1; + this.tree()?.reset(); + this.patchEmptyAction(); + } + + // Git may have changed while the page was away or Review was hidden, so + // the status held is outdated until a request sent after this point answers. + markOutdated() { + this.ensureRendered(); + this.reading.markReturned(); + this.generation += 1; + this.showReadingState(); + } + + async read({ background = false } = {}) { + const rootPath = this.rootPath; + if (!rootPath) { + return null; + } + const generation = ++this.generation; + this.reading.start(); + this.showReadingState(); + try { + const status = await getGitStatus(rootPath); + if (!this.acceptsRead(generation, rootPath)) { + return null; + } + const { accepted, previous } = this.reading.succeed(); + if (!accepted) { + return null; + } + this.status = status; + this.error = null; + if (previous.hasData) { + this.tree()?.updateStatus(status); + } else { + this.tree()?.setStatus(status); + } + this.listShows = "data"; + this.tree()?.setSelectedPath(this.selectedPath); + this.showReadingState(); + this.announceRead({ + ok: true, + hadData: previous.hasData, + wasOutdated: previous.fromBeforeReturn, + background, + }); + return status; + } catch (error) { + if (!this.acceptsRead(generation, rootPath)) { + return null; + } + if (!this.reading.fail().accepted) { + return null; + } + this.error = error; + this.showReadingState(); + this.announceRead({ ok: false, error, background }); + return null; + } + } + + snapshot() { + this.ensureRendered(); + const { fromBeforeReturn, failed } = this.reading.state; + return { + data: this.status, + error: this.error, + outdated: fromBeforeReturn, + failed, + }; + } + + change(path) { + const file = (this.status?.files ?? []).find((entry) => entry.path === path); + if (!file) { + return null; + } + return { + file, + deleted: `${file.status ?? ""}`.includes("D"), + kind: file.untracked ? "untracked" : file.category || "unstaged", + }; + } + + readDiff(path) { + return getGitDiff(this.rootPath, path, this.change(path)?.kind); + } + + diffKind() { + return ""; + } + + setSelectedPath(path) { + this.ensureRendered(); + this.selectedPath = path ?? ""; + this.tree()?.setSelectedPath(this.selectedPath); + } + + captureListScroll() { + return this.tree()?.captureListScroll() ?? null; + } + + restoreListScroll(scroll) { + this.tree()?.restoreListScroll(scroll); + } + + actionHintScope({ + scopeId = "", + actionId = "", + disclosureActionId = "", + branchTargetId = "", + branchActionId = "", + clipRoots = [], + isCurrent = () => true, + } = {}) { + const branchControl = this.querySelector(BRANCH_ACTION_SELECTOR); + const branchScope = branchControl && !branchControl.disabled + ? { + targets: [buttonActionHintTarget({ + invalidationOwner: this, + id: branchTargetId, + actionId: branchActionId, + label: branchControl.textContent?.trim() || "Review branch changes", + control: branchControl, + clipRoots, + isActionable: () => + this.isConnected && + isCurrent() && + this.querySelector(BRANCH_ACTION_SELECTOR) === branchControl && + !branchControl.disabled, + })], + mutationRoots: [branchControl.parentElement].filter(Boolean), + scrollRoots: [], + } + : null; + return mergeActionHintScopes( + branchScope, + this.tree()?.actionHintScope({ + scopeId, + actionId, + disclosureActionId, + clipRoots, + }), + ); + } + + scrollSurfaceScope(options = {}) { + return this.tree()?.scrollSurfaceScope(options); + } + + acceptsRead(generation, rootPath) { + return ( + this.isConnected && + generation === this.generation && + rootPath === this.rootPath + ); + } + + announceRead(detail) { + this.dispatchEvent( + new CustomEvent("caffold:review-changes-read", { bubbles: true, detail }), + ); + } + + // The list shows loading or the read's error where it holds no status, or + // only status from before the page came back that lists no changes. It is + // drawn again only when that choice changes. + showReadingState() { + const { hasData, fromBeforeReturn, reading, failed } = this.reading.state; + const hidesData = + !hasData || (fromBeforeReturn && !this.status?.files?.length); + const shows = !hidesData + ? "data" + : reading + ? "loading" + : failed + ? "error" + : hasData + ? "data" + : this.listShows; + if (shows !== this.listShows) { + this.listShows = shows; + if (shows === "loading") { + this.tree()?.setLoading({ rootPath: this.rootPath, branch: this.branch }); + } else if (shows === "error") { + this.tree()?.setError(this.error, { rootPath: this.rootPath }); + } else if (shows === "data") { + this.tree()?.updateStatus(this.status); + } + } + this.patchEmptyAction(); + } + + patchEmptyAction() { + const { hasData, fromBeforeReturn } = this.reading.state; + const empty = hasData && !fromBeforeReturn && !this.status?.files?.length; + this.querySelector(":scope > .working-tree-changes-empty-action") + ?.toggleAttribute("hidden", !empty); + } + + tree() { + return this.querySelector(":scope > caffold-git-diff-changes-tree"); + } +} + +if (!customElements.get("caffold-review-working-tree-changes")) { + customElements.define( + "caffold-review-working-tree-changes", + CaffoldReviewWorkingTreeChanges, + ); +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.test.js new file mode 100644 index 00000000..2ca6d616 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.test.js @@ -0,0 +1,83 @@ +import assert from "node:assert/strict"; +import test, { after } from "node:test"; + +import { + installCustomElementUnitRegistry, +} from "../../../../../../tests/support/custom-element-unit.js"; +import { ChangesReading } from "../changes-reading.js"; + +const registry = installCustomElementUnitRegistry(); +await import("./working-tree-changes.js"); +const workingTreeChanges = registry + .element("caffold-review-working-tree-changes") + .prototype; +after(() => registry.restore()); + +test("drops a Working Tree status read sent before the page came back", async (t) => { + const previousFetch = globalThis.fetch; + const previousWindow = globalThis.window; + const statuses = []; + globalThis.window = { location: { origin: "http://caffold.test" } }; + globalThis.fetch = (input, ...options) => { + if (new URL(input).pathname !== "/api/git/status") { + return previousFetch(input, ...options); + } + const response = Promise.withResolvers(); + statuses.push(response); + return response.promise; + }; + t.after(() => { + globalThis.fetch = previousFetch; + if (previousWindow === undefined) delete globalThis.window; + else globalThis.window = previousWindow; + }); + const clean = { repository: { rootPath: "/repo" }, files: [] }; + const changed = { + repository: { rootPath: "/repo" }, + files: [{ path: "src/planner.rs", status: "M" }], + }; + const drawn = []; + const announced = []; + const tree = { + setLoading() {}, + setSelectedPath() {}, + setStatus: (status) => drawn.push(status), + updateStatus: (status) => drawn.push(status), + }; + const reading = new ChangesReading(); + reading.start(); + reading.succeed(); + const owner = { + isConnected: true, + rootPath: "/repo", + branch: "main", + status: clean, + error: null, + reading, + generation: 0, + selectedPath: "", + ensureRendered() {}, + acceptsRead: workingTreeChanges.acceptsRead, + showReadingState() {}, + announceRead: (detail) => announced.push(detail), + tree: () => tree, + }; + + const sentBefore = workingTreeChanges.read.call(owner, { background: true }); + workingTreeChanges.markOutdated.call(owner); + statuses.at(-1).resolve(new Response(JSON.stringify(changed))); + assert.equal(await sentBefore, null); + assert.equal(owner.status, clean); + assert.equal(owner.reading.state.fromBeforeReturn, true); + assert.equal(drawn.length, 0); + assert.deepEqual(announced, []); + + const sentAfter = workingTreeChanges.read.call(owner, { background: true }); + statuses.at(-1).resolve(new Response(JSON.stringify(changed))); + assert.deepEqual(await sentAfter, changed); + assert.equal(owner.reading.state.fromBeforeReturn, false); + assert.deepEqual(drawn, [changed]); + assert.deepEqual(announced, [ + { ok: true, hadData: true, wasOutdated: true, background: true }, + ]); +}); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css similarity index 100% rename from frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css rename to frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.js similarity index 100% rename from frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.js rename to frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.js diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.test.js similarity index 94% rename from frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.test.js rename to frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.test.js index bc0f4f22..127a9ddb 100644 --- a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.test.js +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.test.js @@ -3,7 +3,7 @@ import test, { after } from "node:test"; import { installCustomElementUnitRegistry, -} from "../../../../../../tests/support/custom-element-unit.js"; +} from "../../../../../../../../tests/support/custom-element-unit.js"; const registry = installCustomElementUnitRegistry(); await import("./changes-tree.js"); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.css b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.css index b83a60af..62938394 100644 --- a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.css +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.css @@ -83,9 +83,9 @@ caffold-task-review { & .task-review-viewer-pane, & .task-review-navigator, & caffold-file-navigator, - & caffold-git-diff-changes-tree, - & caffold-git-compare-tree, - & caffold-review-file-viewer { + & caffold-review-working-tree-changes, + & caffold-review-branch-changes, + & caffold-review-selected-file { min-width: 0; min-height: 0; height: 100%; @@ -169,34 +169,6 @@ caffold-task-review { display: none; } - & .task-review-empty-action { - position: absolute; - z-index: 2; - inset: calc( - var(--review-header-height) + var(--interface-space-16) + var(--interface-space-8) - ) - var(--interface-space-5) auto; - box-sizing: border-box; - width: fit-content; - max-width: calc(100% - var(--interface-space-10)); - margin: 0; - padding: var(--interface-space-5); - border: 1px solid var(--border); - border-radius: 6px; - background: var(--surface); - box-shadow: 0 8px 24px rgb(var(--shadow-rgb) / 10%); - color: var(--muted); - font-size: var(--interface-meta-font-size); - } - - & .task-review-empty-action[hidden] { - display: none; - } - - & .task-review-empty-action p { - margin: 0 0 var(--interface-space-4); - } - & .task-review-layout > caffold-pane-resizer { left: var(--task-review-panel-width, 320px); } diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js index 28e5abbf..897de23a 100644 --- a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.js @@ -1,37 +1,36 @@ -import { - getGitCompare, - getGitCompareDiff, - getGitDiff, - getGitRefs, - getGitStatus, - readFile, -} from "#app/api.js"; import "#components/file-navigator.js"; -import "#components/file-viewer.js"; import "#components/segmented-control.js"; -import { - diffViewerPresentation, - sourceViewerPresentation, -} from "#components/file-viewer-presentation.js"; -import { fileStatusPresentation } from "#app/file-status.js"; -import "#components/git-compare-browser/compare-tree.js"; -import "./components/changes-tree.js"; -import { - documentPreviewKind, - fileNameFromPath, - imageTypeLabel, - isPreviewableImagePath, -} from "#components/dom.js"; import "#components/pane-resizer.js"; +import "./components/working-tree-changes.js"; +import "./components/branch-changes.js"; +import "./components/selected-file.js"; +import { + fileRepresentationCapabilities, + isFileRepresentation, +} from "./representations.js"; import { subscribeToWatch, watchChangeAffectsPath, } from "#app/watch.js"; import { cleanLogicalPath } from "../../task-format.js"; import { taskThreadId } from "../../task-list-model.js"; +import { + normalizeForTask, + normalizeReviewRoute, + reviewContextKey, + reviewRouteKey, + routeForSelectedFile, + taskRouteForReview, + viewerStateKey, +} from "./layout/route.js"; +import { + joinLogicalPath, + relativeTaskPath, + taskRootPath, + taskWorktreeRootPath, +} from "./layout/paths.js"; import { ACTION_HINT_ACTION, - buttonActionHintTarget, emptyActionHintScope, hasActionHintLayoutBox, mergeActionHintScopes, @@ -49,15 +48,13 @@ class CaffoldTaskReview extends HTMLElement { connectedCallback() { this.ensureRendered(); this.active = true; - this.markScopesOutdated(); + this.markChangesOutdated(); this.syncReview({ reactivated: true }); } disconnectedCallback() { - this.viewer()?.deactivate?.(); this.active = false; this.unsubscribeWatch(); - this.invalidateRequests(); this.fileNavigator()?.setWatchActive(false); } @@ -71,23 +68,10 @@ class CaffoldTaskReview extends HTMLElement { this.contextKey = ""; this.route = normalizeReviewRoute(); this.previousRouteKey = ""; - this.status = null; - this.compare = null; - this.refs = null; - this.statusError = null; - this.compareError = null; - this.statusOutdated = false; - this.branchOutdated = false; - this.statusGeneration = 0; - this.branchGeneration = 0; - this.viewerGeneration = 0; - this.shownViewerStateKey = ""; this.watchUnsubscribe = null; this.watchPath = ""; this.pendingWatchViewerRefresh = false; this.navigatorScroll = new Map(); - this.viewerScroll = new Map(); - this.pendingRepresentationLine = new Map(); this.liveUpdates = null; this.innerHTML = ` @@ -101,14 +85,10 @@ class CaffoldTaskReview extends HTMLElement {
- - +
- +
@@ -126,7 +106,7 @@ class CaffoldTaskReview extends HTMLElement {
- +
@@ -136,14 +116,28 @@ class CaffoldTaskReview extends HTMLElement { this.fileNavigator()?.setLiveUpdates(this.liveUpdates); this.fileNavigator()?.setWatchActive(false); this.fileNavigator()?.setRefreshVisible(false); - this.viewer()?.setCloseLabel("Back to navigator"); - this.viewer()?.setCloseMode("back"); this.applyPanelWidth(); - this.addEventListener("click", (event) => this.handleClick(event)); this.addEventListener("caffold:segmented-control-intent", (event) => { this.handleAxisIntent(event); }); + this.addEventListener("caffold:review-branch-intent", (event) => { + event.stopPropagation(); + this.updateAxis("scope", "branch"); + }); + this.addEventListener("caffold:review-representation-intent", (event) => { + event.stopPropagation(); + this.updateAxis("viewer", event.detail?.representation); + }); + this.addEventListener("caffold:review-base-intent", (event) => { + event.stopPropagation(); + this.captureLocalState(); + this.requestRoute({ ...this.route, baseRef: `${event.detail?.baseRef ?? ""}` }); + }); + this.addEventListener("caffold:review-changes-read", (event) => { + event.stopPropagation(); + this.handleChangesRead(event.target, event.detail ?? {}); + }); this.addEventListener("caffold:open-git-diff", (event) => { if (!closestElement(event.target, "caffold-git-diff-changes-tree")) { return; @@ -158,13 +152,6 @@ class CaffoldTaskReview extends HTMLElement { event.stopPropagation(); this.selectLogicalPath(event.detail?.path); }); - this.addEventListener("caffold:select-compare-base", (event) => { - if (!closestElement(event.target, "caffold-git-compare-tree")) { - return; - } - event.stopPropagation(); - this.selectCompareBase(event.detail?.baseRef); - }); this.addEventListener("caffold:file-navigator-open-file", (event) => { event.stopPropagation(); this.selectLogicalPath(event.detail?.path); @@ -172,7 +159,7 @@ class CaffoldTaskReview extends HTMLElement { this.addEventListener("caffold:file-navigator-refresh-selected", (event) => { event.stopPropagation(); if (isFileRepresentation(this.route.viewer)) { - void this.loadViewer({ background: true }); + this.selectedFile()?.reload(this.currentView(), { background: true }); } }); this.addEventListener("caffold:pane-resize", (event) => { @@ -223,6 +210,7 @@ class CaffoldTaskReview extends HTMLElement { // Detail hands the context over on every Task update; keep its newest // branch and HEAD for later requests without restarting any load. this.task = task; + this.syncChildContexts(); return false; } if (contextChanged) { @@ -230,15 +218,12 @@ class CaffoldTaskReview extends HTMLElement { this.contextKey = contextKey; } else if (routeChanged) { this.captureLocalState(); - if (previousRoute.baseRef !== nextRoute.baseRef) { - this.compare = null; - this.compareError = null; - } } this.active = true; this.task = task; this.route = nextRoute; this.previousRouteKey = nextRouteKey; + this.syncChildContexts(); const normalizedForTask = normalizeForTask(nextRoute, task); if (reviewRouteKey(normalizedForTask) !== nextRouteKey) { @@ -310,31 +295,6 @@ class CaffoldTaskReview extends HTMLElement { labelForChoice: (choice) => `Show ${choice.label}`, }) : null; - const emptyControl = this.querySelector( - ':scope > .task-review-workspace > .task-review-layout > .task-review-navigator-pane > .task-review-navigator[data-review-navigator="working"] > .task-review-empty-action:not([hidden]) > button[data-review-action="review-branch"]', - ); - const emptyScope = navigatorActive && emptyControl && !emptyControl.disabled - ? { - targets: [buttonActionHintTarget({ - invalidationOwner: this, - id: `${scopeId}:scope:branch`, - actionId: ACTION_HINT_ACTION.REVIEW_AXIS, - label: emptyControl.textContent?.trim() || "Review branch changes", - control: emptyControl, - clipRoots: navigatorClipRoots, - isActionable: () => - this.isConnected && - this.active && - !this.hidden && - this.querySelector( - ':scope > .task-review-workspace > .task-review-layout > .task-review-navigator-pane > .task-review-navigator[data-review-navigator="working"] > .task-review-empty-action:not([hidden]) > button[data-review-action="review-branch"]', - ) === emptyControl && - !emptyControl.disabled, - })], - mutationRoots: [emptyControl.parentElement].filter(Boolean), - scrollRoots: [], - } - : null; const navigatorScope = !navigatorActive ? null : this.route.navigator === "files" @@ -346,21 +306,24 @@ class CaffoldTaskReview extends HTMLElement { clipRoots: navigatorClipRoots, }) : this.route.scope === "branch" - ? this.branchTree()?.actionHintScope({ + ? this.branchChanges()?.actionHintScope({ scopeId: `${scopeId}:branch`, actionId: ACTION_HINT_ACTION.FILE_OPEN, disclosureActionId: ACTION_HINT_ACTION.DISCLOSURE_TOGGLE, selectActionId: ACTION_HINT_ACTION.CONTROL_SELECT_OPEN, clipRoots: navigatorClipRoots, }) - : this.workingTree()?.actionHintScope({ + : this.workingTreeChanges()?.actionHintScope({ scopeId: `${scopeId}:working`, actionId: ACTION_HINT_ACTION.FILE_OPEN, disclosureActionId: ACTION_HINT_ACTION.DISCLOSURE_TOGGLE, + branchTargetId: `${scopeId}:scope:branch`, + branchActionId: ACTION_HINT_ACTION.REVIEW_AXIS, clipRoots: navigatorClipRoots, + isCurrent: () => this.isConnected && this.active && !this.hidden, }); const viewerScope = viewerActive && this.route.path - ? this.viewer()?.actionHintScope({ + ? this.selectedFile()?.actionHintScope({ scopeId: `${scopeId}:viewer`, actionId: ACTION_HINT_ACTION.PARENT, noticeActionId: ACTION_HINT_ACTION.REVIEW_AXIS, @@ -388,7 +351,6 @@ class CaffoldTaskReview extends HTMLElement { return mergeActionHintScopes( navigatorAxis, viewerAxis, - emptyScope, navigatorScope, resizerScope, viewerScope, @@ -419,12 +381,12 @@ class CaffoldTaskReview extends HTMLElement { clipRoots: navigatorClipRoots, }) : this.route.scope === "branch" - ? this.branchTree()?.scrollSurfaceScope({ + ? this.branchChanges()?.scrollSurfaceScope({ scopeId: `${scopeId}:branch`, label: "Branch changes", clipRoots: navigatorClipRoots, }) - : this.workingTree()?.scrollSurfaceScope({ + : this.workingTreeChanges()?.scrollSurfaceScope({ scopeId: `${scopeId}:working`, label: "Working tree changes", clipRoots: navigatorClipRoots, @@ -432,7 +394,7 @@ class CaffoldTaskReview extends HTMLElement { } const viewerScope = hasScrollLayoutBox(viewerPane) && this.route.path - ? this.viewer()?.scrollSurfaceScope({ + ? this.selectedFile()?.scrollSurfaceScope({ scopeId: `${scopeId}:viewer`, clipRoots: viewerClipRoots, }) @@ -456,44 +418,37 @@ class CaffoldTaskReview extends HTMLElement { ) { return []; } - return this.viewer()?.keyboardNavigationContexts({ + return this.selectedFile()?.keyboardNavigationContexts({ scopeId: `review:${encodeURIComponent(subjectId)}:viewer`, }) ?? []; } resetContext() { this.unsubscribeWatch(); - this.invalidateRequests(); - this.status = null; - this.compare = null; - this.refs = null; - this.statusError = null; - this.compareError = null; - this.statusOutdated = false; - this.branchOutdated = false; this.pendingWatchViewerRefresh = false; this.previousRouteKey = ""; - this.workingTree()?.reset(); - this.branchTree()?.reset(); this.fileNavigator()?.clearSelectedPath(); - this.shownViewerStateKey = ""; - this.viewer()?.setEmpty(); } - invalidateRequests() { - this.statusGeneration += 1; - this.branchGeneration += 1; - this.viewerGeneration += 1; + syncChildContexts() { + const worktreeRootPath = taskWorktreeRootPath(this.task); + const branch = this.task?.worktree?.branch ?? null; + this.workingTreeChanges()?.setContext({ rootPath: worktreeRootPath, branch }); + this.branchChanges()?.setContext({ + rootPath: worktreeRootPath, + branch, + baseRef: this.route.baseRef, + }); + this.branchChanges()?.setShown(this.route.scope === "branch"); + this.selectedFile()?.setContext({ + rootPath: taskRootPath(this.task), + worktreeRootPath, + }); } - // Git may have changed while the page was away or Review was hidden, so - // the data held for each scope is outdated until a request sent after - // this point answers. - markScopesOutdated() { - this.statusOutdated = true; - this.branchOutdated = true; - this.statusGeneration += 1; - this.branchGeneration += 1; + markChangesOutdated() { + this.workingTreeChanges()?.markOutdated(); + this.branchChanges()?.markOutdated(); } syncReview(options = {}) { @@ -509,36 +464,30 @@ class CaffoldTaskReview extends HTMLElement { }); this.subscribeWatch(taskRootPath(this.task)); if (this.task.worktree) { + const working = this.workingTreeChanges().snapshot(); if ( options.contextChanged || options.reactivated || - !this.status || - (options.scopeChanged && this.route.scope === "working" && this.statusOutdated) + !working.data || + (options.scopeChanged && this.route.scope === "working" && working.outdated) ) { - void this.refreshWorking(); - } - if ( - this.route.scope === "branch" && - (options.reactivated || - (options.scopeChanged && this.branchOutdated) || - !this.refs || - !this.compare || - this.compare.baseRef !== this.route.baseRef) - ) { - void this.ensureBranchData({ force: this.branchOutdated }); + void this.readChanges(this.workingTreeChanges()); + } + if (this.route.scope === "branch") { + const branch = this.branchChanges().snapshot(); + if ( + options.reactivated || + (options.scopeChanged && branch.outdated) || + !branch.hasRefs || + !branch.data + ) { + void this.readChanges(this.branchChanges(), { force: branch.outdated }); + } } } - if (!this.route.path) { - this.shownViewerStateKey = ""; - this.viewer()?.setEmpty(); - } else if ( - options.contextChanged || - options.routeChanged || - options.reactivated || - ["empty", "error"].includes(this.viewer()?.state?.status) - ) { - void this.loadViewer(); - } + this.selectedFile()?.show(this.currentView(), { + force: Boolean(options.contextChanged || options.routeChanged || options.reactivated), + }); if (options.contextChanged || options.routeChanged || options.reactivated) { this.restoreNavigatorScroll(); } @@ -584,362 +533,102 @@ class CaffoldTaskReview extends HTMLElement { } } - async refreshWorking(options = {}) { - const rootPath = taskWorktreeRootPath(this.task); - if (!rootPath) { - return null; - } - const generation = ++this.statusGeneration; - if (!this.status) { - this.workingTree()?.setLoading({ - rootPath, - branch: this.task?.worktree?.branch ?? null, - }); - } - this.patchRereadState("working"); - try { - const status = await getGitStatus(rootPath); - if (!this.acceptRequest("status", generation, rootPath)) { - return null; - } - const hadStatus = Boolean(this.status); - const wasOutdated = this.statusOutdated; - this.status = status; - this.statusOutdated = false; - this.statusError = null; - this.patchErrorState(); - if (hadStatus) { - this.workingTree()?.updateStatus(status); - } else { - this.workingTree()?.setStatus(status); - } - this.syncSelection(); - this.patchEmptyStates(); - const canRefreshViewer = this.route.path && this.route.scope === "working"; - const pendingWatchViewerRefresh = this.pendingWatchViewerRefresh; - this.pendingWatchViewerRefresh = false; - // A watch change reloads the viewer only for its own file, unless the - // status it replaces was read before the page came back. - const refreshViewer = options.background - ? canRefreshViewer && (pendingWatchViewerRefresh || wasOutdated) - : canRefreshViewer; - if (refreshViewer) { - void this.loadViewer({ - background: - Boolean(options.background) || - (hadStatus && this.shownViewerStateKey === this.viewerStateKey()), - }); - } - return status; - } catch (error) { - if (!this.acceptRequest("status", generation, rootPath)) { - return null; - } - this.statusError = error; - if (!this.status) { - this.workingTree()?.setError(error, { rootPath }); - } - this.patchRereadState("working", error); - this.patchErrorState(); - return null; - } + readChanges(changes, options = {}) { + const reading = changes.read(options); + this.followRereadInViewer(changes); + return reading; } - async ensureBranchData({ force = false } = {}) { - const rootPath = taskWorktreeRootPath(this.task); - if (!rootPath) { - return null; + handleChangesRead(changes, { ok, hadData, wasOutdated, background = false }) { + if (changes !== this.workingTreeChanges() && changes !== this.branchChanges()) { + return; } - const generation = ++this.branchGeneration; - this.patchRereadState("branch"); - try { - const refs = force || !this.refs ? await getGitRefs(rootPath) : this.refs; - if (!this.acceptRequest("branch", generation, rootPath)) { - return null; - } - this.refs = refs; - const baseRef = selectedCompareBaseRef(this.route.baseRef, refs); - this.branchTree()?.setBaseSelection({ refs: refs.refs, value: baseRef }); - if (this.route.baseRef !== baseRef) { - this.requestRoute({ ...this.route, baseRef }); - return null; - } - this.patchControls(); - if (!this.compare) { - this.branchTree()?.setLoading(refs.repository, { - baseRef, - headRef: taskCompareHeadRef(this.task, refs), - }); - } - const compare = await getGitCompare( - rootPath, - baseRef, - taskCompareHeadRef(this.task, refs), - ); - if (!this.acceptRequest("branch", generation, rootPath)) { - return null; - } - const hadCompare = Boolean(this.compare); - this.compare = compare; - this.branchOutdated = false; - this.compareError = null; - this.patchErrorState(); - this.branchTree()?.setEmptyMessage( - `No changes compared with ${baseRef || "the selected base"}.`, - ); - if (hadCompare) { - this.branchTree()?.updateCompare(compare); - } else { - this.branchTree()?.setCompare(compare); - } - this.syncSelection(); - this.patchControls(); - this.patchEmptyStates(); + this.patchErrorState(); + if (!ok) { + this.followRereadInViewer(changes); + return; + } + if (changes === this.branchChanges()) { if (this.route.path && this.route.scope === "branch") { - void this.loadViewer({ - background: hadCompare && this.shownViewerStateKey === this.viewerStateKey(), - }); - } - return compare; - } catch (error) { - if (!this.acceptRequest("branch", generation, rootPath)) { - return null; - } - this.compareError = error; - if (!this.compare) { - this.branchTree()?.setError(error, this.refs?.repository ?? null); + this.selectedFile()?.reload(this.currentView(), { reread: hadData }); } - this.patchRereadState("branch", error); - this.patchErrorState(); - return null; - } - } - - acceptRequest(owner, generation, rootPath) { - return ( - this.active && - this.isConnected && - (owner !== "branch" || this.route.scope === "branch") && - generation === - (owner === "branch" ? this.branchGeneration : this.statusGeneration) && - rootPath === taskWorktreeRootPath(this.task) - ); - } - - async loadViewer(options = {}) { - const selectedPath = this.logicalSelectedPath(); - if (!selectedPath || !this.active) { - this.shownViewerStateKey = ""; - this.viewer()?.setEmpty(); return; } - const generation = ++this.viewerGeneration; - const stateKey = this.viewerStateKey(); - // A background load follows a change the watch reported for the file, a - // recovered watch, or a reread of Git data while the view is on screen: - // it keeps the reading position and asks again whether a previewed file - // changed. Any other load shows the file the viewer already holds. - const background = Boolean(options.background); - if (!background) { - this.shownViewerStateKey = ""; + const canRefreshViewer = this.route.path && this.route.scope === "working"; + const pendingWatchViewerRefresh = this.pendingWatchViewerRefresh; + this.pendingWatchViewerRefresh = false; + // A watch change reloads the viewer only for its own file, unless the + // status it replaces was read before the page came back. + const refreshViewer = background + ? canRefreshViewer && (pendingWatchViewerRefresh || wasOutdated) + : canRefreshViewer; + if (refreshViewer) { + this.selectedFile()?.reload(this.currentView(), { background, reread: hadData }); } - const sourceMode = this.route.viewer === "source"; - const previewMode = this.route.viewer === "preview"; - const fileMode = sourceMode || previewMode; - const representations = fileRepresentationCapabilities(selectedPath); + } + + // Where the shown changes from before the page came back say the selected + // file did not change, the viewer follows their reread: loading while it + // runs, then the read's error if it fails. + followRereadInViewer(changes) { if ( - !fileMode && - ((this.route.scope === "working" && !this.status) || - (this.route.scope === "branch" && !this.compare)) + changes !== this.shownChanges() || + !this.route.path || + this.route.viewer !== "diff" ) { return; } - const change = this.selectedChange(); - if (fileMode && change?.deleted) { - this.shownViewerStateKey = ""; - this.viewer()?.setNotice( - "This file was deleted in the selected scope. Diff remains available.", - { title: fileNameFromPath(selectedPath) }, - ); - return; - } - if (previewMode && representations.previewKind === "document") { - const entry = this.fileNavigator()?.entryForPath(selectedPath); - this.viewer()?.setDocument({ - kind: representations.documentKind, - path: selectedPath, - name: fileNameFromPath(selectedPath), - size: entry?.size, - modifiedMs: entry?.modifiedMs, - }, { recheck: background }); - this.shownViewerStateKey = stateKey; + const { data, outdated } = changes.snapshot(); + if (!outdated || !data || changes.change(this.logicalSelectedPath())) { return; } - if (previewMode && representations.previewKind === "image") { - const entry = this.fileNavigator()?.entryForPath(selectedPath); - this.viewer()?.setImage({ - path: selectedPath, - name: fileNameFromPath(selectedPath), - imageType: imageTypeLabel(selectedPath), - size: entry?.size, - modifiedMs: entry?.modifiedMs, - }, { recheck: background }); - this.shownViewerStateKey = stateKey; - return; - } - if (sourceMode && !representations.source) { - this.shownViewerStateKey = ""; - this.viewer()?.setNotice("Source is unavailable for this file.", { - actionLabel: "View preview", - action: "view-preview", - title: fileNameFromPath(selectedPath), - }); - return; - } - if (previewMode && !representations.previewKind) { - this.shownViewerStateKey = ""; - this.viewer()?.setNotice("Preview is unavailable for this file.", { - actionLabel: "View source", - action: "view-source", - title: fileNameFromPath(selectedPath), - }); - return; - } - if (!fileMode && !change) { - this.shownViewerStateKey = ""; - const branch = this.route.scope === "branch"; - if (branch ? this.branchOutdated : this.statusOutdated) { - const error = branch ? this.compareError : this.statusError; - const presentation = this.diffPresentation(selectedPath, null); - if (error) { - this.viewer()?.setError(presentation, error); - } else { - this.viewer()?.setLoading(presentation); - } - return; - } - this.viewer()?.setNotice("No changes in this scope.", { - actionLabel: "View source", - action: "view-source", - title: fileNameFromPath(selectedPath), - }); - return; - } - const viewer = this.viewer(); - const saved = this.viewerScroll.get(stateKey); - const presentation = fileMode - ? sourceViewerPresentation({ path: selectedPath }) - : this.diffPresentation(selectedPath, change); - if (!background) { - viewer?.setLoading(presentation); - } - const viewerOptions = background - ? { preserveScroll: true } - : sourceMode && this.route.line - ? { line: this.route.line } - : { scroll: saved?.scroll ?? null }; - try { - if (fileMode) { - const file = await readFile(selectedPath); - if (!this.acceptViewer(generation, selectedPath)) { - return; - } - if (previewMode) { - viewer?.setMarkdown(file, { - ...viewerOptions, - imageRoot: taskRootPath(this.task), - }); - } else { - viewer?.setFile(file, viewerOptions); - } - } else { - const diff = this.route.scope === "branch" - ? await getGitCompareDiff( - taskWorktreeRootPath(this.task), - this.route.baseRef, - taskCompareHeadRef(this.task, this.refs), - selectedPath, - ) - : await getGitDiff( - taskWorktreeRootPath(this.task), - selectedPath, - change.kind, - ); - if (!this.acceptViewer(generation, selectedPath)) { - return; - } - const loadedPresentation = this.diffPresentation(selectedPath, change, diff); - viewer?.setDiff(diff, { - ...viewerOptions, - presentation: loadedPresentation, - }); - } - this.shownViewerStateKey = stateKey; - if (!background && !(sourceMode && this.route.line)) { - this.restoreViewerPosition(saved); - } - } catch (error) { - if (this.acceptViewer(generation, selectedPath)) { - this.shownViewerStateKey = ""; - viewer?.setError(presentation, error); - } - } + this.selectedFile()?.reload(this.currentView()); } - diffPresentation(selectedPath, change, diff = {}) { - return diffViewerPresentation({ - ...diff, - repository: diff.repository ?? { rootPath: taskWorktreeRootPath(this.task) }, - path: diff.path ?? selectedPath, - repoRelativePath: diff.repoRelativePath ?? this.route.path, - kind: - diff.kind || - (this.route.scope === "branch" - ? `${this.route.baseRef}...${taskCompareHeadRef(this.task, this.refs)}` - : change?.kind ?? ""), - status: change?.file - ? fileStatusPresentation(change.file.status, change.file).code - : "", - }); - } - - acceptViewer(generation, selectedPath) { - return ( - this.active && - this.isConnected && - generation === this.viewerGeneration && - selectedPath === this.logicalSelectedPath() - ); + currentView() { + const logicalPath = this.logicalSelectedPath(); + return { + stateKey: viewerStateKey(this.route), + scope: this.route.scope, + path: this.route.path, + logicalPath, + representation: this.route.viewer, + line: this.route.line, + entry: logicalPath ? this.fileNavigator()?.entryForPath(logicalPath) : null, + changes: this.viewerChanges(logicalPath), + }; } - selectedChange() { - const selected = this.logicalSelectedPath(); - const files = this.route.scope === "branch" - ? this.compare?.files ?? [] - : this.status?.files ?? []; - const file = files.find((entry) => entry.path === selected); - if (!file) { - return null; + viewerChanges(logicalPath) { + const changes = this.task?.worktree ? this.shownChanges() : null; + if (!changes) { + return { + ready: false, + outdated: false, + failed: false, + error: null, + change: null, + diffKind: "", + readDiff: null, + }; } + const { data, error, outdated, failed } = changes.snapshot(); return { - file, - deleted: `${file.status ?? ""}`.includes("D"), - kind: file.untracked ? "untracked" : file.category || "unstaged", + ready: Boolean(data), + outdated, + failed, + error, + change: logicalPath ? changes.change(logicalPath) : null, + diffKind: changes.diffKind(), + readDiff: () => changes.readDiff(logicalPath), }; } captureLocalState() { - const navigator = this.activeNavigator(); - const scroll = navigatorScroll(navigator); + const scroll = navigatorScroll(this.activeNavigator()); if (scroll) { this.navigatorScroll.set(this.navigatorStateKey(), scroll); } - if (this.route.path) { - this.viewerScroll.set(this.viewerStateKey(), { - scroll: this.viewer()?.captureContentScroll?.() ?? null, - line: this.viewer()?.visibleLine?.() ?? null, - }); - } } restoreNavigatorScroll() { @@ -949,19 +638,6 @@ class CaffoldTaskReview extends HTMLElement { ); } - restoreViewerPosition(saved) { - const line = - saved?.line ?? this.pendingRepresentationLine.get(this.viewerStateKey()); - if (!line) { - return; - } - this.pendingRepresentationLine.delete(this.viewerStateKey()); - requestAnimationFrame(() => { - this.viewer()?.scrollToLine(line); - window.setTimeout(() => this.viewer()?.scrollToLine(line), 40); - }); - } - patchControls() { const gitAvailable = Boolean(this.task?.worktree); const navigatorChoices = [ @@ -994,7 +670,7 @@ class CaffoldTaskReview extends HTMLElement { } patchErrorState() { - const error = this.route.scope === "branch" ? this.compareError : this.statusError; + const error = this.shownChanges()?.snapshot().error; const message = error ? `${this.route.scope === "branch" ? "Branch comparison" : "Working tree"} update failed: ${error.message}` : ""; @@ -1021,99 +697,15 @@ class CaffoldTaskReview extends HTMLElement { this.dataset.reviewViewer = this.route.viewer; this.applyPanelWidth(); this.applySidePane(); - this.patchEmptyStates(); - } - - patchEmptyStates() { - const workingEmpty = Boolean( - this.status && !this.statusOutdated && (this.status.files?.length ?? 0) === 0, - ); - this.querySelector(".task-review-empty-action")?.toggleAttribute("hidden", !workingEmpty); - } - - // While a scope's data from before the page came back is read again, the - // places where that data says nothing changed show loading, then the read's - // error if it fails. - patchRereadState(scope, error = null) { - const working = scope === "working"; - if (!(working ? this.statusOutdated : this.branchOutdated)) { - return; - } - const data = working ? this.status : this.compare; - if (data && !data.files?.length) { - if (working) { - const rootPath = taskWorktreeRootPath(this.task); - if (error) { - this.workingTree()?.setError(error, { rootPath }); - } else { - this.workingTree()?.setLoading({ - rootPath, - branch: this.task?.worktree?.branch ?? null, - }); - } - } else { - const repository = this.refs?.repository ?? null; - if (error) { - this.branchTree()?.setError(error, repository); - } else { - this.branchTree()?.setLoading(repository, { - baseRef: this.route.baseRef, - headRef: taskCompareHeadRef(this.task, this.refs), - }); - } - } - } - if (working) { - this.patchEmptyStates(); - } - if ( - !data || - this.route.scope !== scope || - !this.route.path || - this.route.viewer !== "diff" || - this.selectedChange() - ) { - return; - } - const presentation = this.diffPresentation(this.logicalSelectedPath(), null); - this.shownViewerStateKey = ""; - if (error) { - this.viewer()?.setError(presentation, error); - } else { - this.viewer()?.setLoading(presentation); - } } syncSelection() { const logicalPath = this.logicalSelectedPath(); - this.workingTree()?.setSelectedPath(logicalPath); - this.branchTree()?.setSelectedPath(logicalPath); + this.workingTreeChanges()?.setSelectedPath(logicalPath); + this.branchChanges()?.setSelectedPath(logicalPath); this.fileNavigator()?.setSelectedPath(logicalPath); } - handleClick(event) { - const action = closestElement(event.target, "[data-review-action]"); - if (action && this.contains(action)) { - event.stopPropagation(); - if (action.dataset.reviewAction === "review-branch") { - this.updateAxis("scope", "branch"); - } - return; - } - const noticeAction = closestElement( - event.target, - '[data-action="view-source"], [data-action="view-preview"]', - ); - if (noticeAction && this.contains(noticeAction)) { - event.stopPropagation(); - this.updateAxis( - "viewer", - noticeAction.dataset.action === "view-preview" ? "preview" : "source", - ); - return; - } - } - handleAxisIntent(event) { const control = event.target instanceof Element ? event.target.closest("caffold-segmented-control[data-review-axis]") @@ -1131,13 +723,6 @@ class CaffoldTaskReview extends HTMLElement { return; } this.captureLocalState(); - if (field === "viewer" && this.route.path) { - const line = this.viewer()?.visibleLine?.(); - if (line) { - const nextKey = viewerStateKey({ ...this.route, viewer: value }); - this.pendingRepresentationLine.set(nextKey, line); - } - } this.requestRoute({ ...this.route, [field]: value }); } @@ -1149,19 +734,6 @@ class CaffoldTaskReview extends HTMLElement { this.requestRoute(routeForSelectedFile(this.route, relative)); } - selectCompareBase(baseRef) { - const next = `${baseRef ?? ""}`; - if ( - !next || - next === this.route.baseRef || - !this.refs?.refs?.some((ref) => ref.name === next) - ) { - return; - } - this.captureLocalState(); - this.requestRoute({ ...this.route, baseRef: next }); - } - clearSelectedPath() { if (!this.route.path) { return; @@ -1182,7 +754,7 @@ class CaffoldTaskReview extends HTMLElement { } async refreshAll() { - this.markScopesOutdated(); + this.markChangesOutdated(); const requests = [ this.fileNavigator()?.requestRefresh({ allDirectories: true, @@ -1192,8 +764,8 @@ class CaffoldTaskReview extends HTMLElement { if (this.task?.worktree) { requests.push( this.route.scope === "branch" - ? this.ensureBranchData({ force: true }) - : this.refreshWorking(), + ? this.readChanges(this.branchChanges(), { force: true }) + : this.readChanges(this.workingTreeChanges()), ); } await Promise.allSettled(requests.filter(Boolean)); @@ -1237,13 +809,13 @@ class CaffoldTaskReview extends HTMLElement { this.pendingWatchViewerRefresh ||= this.route.scope === "working" && watchChangeAffectsPath(change, this.logicalSelectedPath()); - void this.refreshWorking({ background: true }); + void this.readChanges(this.workingTreeChanges(), { background: true }); } if (this.task?.worktree && change.gitRefsChanged) { if (this.route.scope === "branch") { - void this.ensureBranchData({ force: true }); + void this.readChanges(this.branchChanges(), { force: true }); } else { - this.refs = null; + this.branchChanges()?.forgetRefs(); } } }, @@ -1306,32 +878,32 @@ class CaffoldTaskReview extends HTMLElement { return `${this.route.scope}:${this.route.navigator}`; } - viewerStateKey() { - return viewerStateKey(this.route); - } - activeNavigator() { return this.route.navigator === "files" ? this.fileNavigator() - : this.route.scope === "branch" - ? this.branchTree() - : this.workingTree(); + : this.shownChanges(); + } + + shownChanges() { + return this.route.scope === "branch" + ? this.branchChanges() + : this.workingTreeChanges(); } - workingTree() { - return this.querySelector("caffold-git-diff-changes-tree"); + workingTreeChanges() { + return this.querySelector("caffold-review-working-tree-changes"); } - branchTree() { - return this.querySelector("caffold-git-compare-tree"); + branchChanges() { + return this.querySelector("caffold-review-branch-changes"); } fileNavigator() { return this.querySelector("caffold-file-navigator"); } - viewer() { - return this.querySelector("caffold-review-file-viewer"); + selectedFile() { + return this.querySelector("caffold-review-selected-file"); } resizer() { @@ -1350,206 +922,6 @@ if (!customElements.get("caffold-task-review")) { customElements.define("caffold-task-review", CaffoldTaskReview); } -function normalizeReviewRoute(route = {}, task = null) { - const path = reviewFilePath(route?.path); - return { - scope: route?.reviewScope === "branch" ? "branch" : "working", - navigator: route?.reviewNavigator === "files" ? "files" : "changes", - viewer: ["source", "preview"].includes(route?.reviewViewer) - ? route.reviewViewer - : "diff", - path, - line: path ? positiveLine(route?.line) : null, - baseRef: `${route?.baseRef ?? ""}`, - ...(task ? { threadId: taskThreadId(task) } : {}), - }; -} - -function taskRouteForReview(threadId, state) { - return { - kind: "tasks", - threadId, - review: true, - reviewScope: state.scope, - reviewNavigator: state.navigator, - reviewViewer: state.viewer, - path: state.path, - line: state.line, - baseRef: state.baseRef, - }; -} - -function normalizeForTask(route, task) { - const taskRoute = task?.worktree - ? route - : { - ...route, - scope: "working", - navigator: "files", - viewer: route.viewer === "preview" ? "preview" : "source", - baseRef: "", - }; - const viewer = representationForFile(taskRoute.viewer, taskRoute.path); - return viewer === taskRoute.viewer ? taskRoute : { ...taskRoute, viewer }; -} - -function reviewRouteKey(route) { - return [route.scope, route.navigator, route.viewer, route.path, route.line, route.baseRef].join("\u0000"); -} - -function viewerStateKey(route) { - return `${route.scope}:${route.path}:${route.viewer}`; -} - -function isFileRepresentation(viewer) { - return viewer === "source" || viewer === "preview"; -} - -function routeForSelectedFile(route, path) { - return { - ...route, - viewer: representationForFile(route.viewer, path), - path, - line: null, - }; -} - -function representationForFile(viewer, path) { - if (!path || viewer === "diff") { - return viewer; - } - const representations = fileRepresentationCapabilities(path); - if ( - (viewer === "source" && representations.source) || - (viewer === "preview" && representations.previewKind) - ) { - return viewer; - } - return representations.source ? "source" : "preview"; -} - -function fileRepresentationCapabilities(path) { - const documentKind = documentPreviewKind(path); - if (documentKind) { - return { source: false, previewKind: "document", documentKind }; - } - const imagePreview = isPreviewableImagePath(path); - return { - source: !imagePreview || isSvgPath(path), - previewKind: imagePreview - ? "image" - : isPreviewableMarkdownPath(path) - ? "markdown" - : null, - }; -} - -function isPreviewableMarkdownPath(path) { - return /\.(?:md|markdown)$/i.test(`${path ?? ""}`); -} - -function isSvgPath(path) { - return /\.svg$/i.test(`${path ?? ""}`); -} - -function reviewContextKey(task) { - return [taskThreadId(task), taskRootPath(task), taskWorktreeRootPath(task)].join("\u0000"); -} - -function taskRootPath(task) { - const path = task?.worktree?.rootPath || task?.cwdPath || task?.cwd || "."; - return cleanLogicalPath(path) || "."; -} - -function taskWorktreeRootPath(task) { - const path = task?.worktree?.rootPath; - return path ? cleanLogicalPath(path) || "." : ""; -} - -function taskCompareHeadRef(task, refs = null) { - return task?.worktree?.branch || refs?.defaultHeadRef || "HEAD"; -} - -function selectedCompareBaseRef(preferredRef, refsPayload = {}) { - const refs = Array.isArray(refsPayload.refs) ? refsPayload.refs : []; - const hasRef = (candidate) => - Boolean(candidate && refs.some((ref) => ref.name === candidate)); - if (hasRef(preferredRef)) { - return `${preferredRef}`; - } - if (hasRef(refsPayload.defaultBaseRef)) { - return `${refsPayload.defaultBaseRef}`; - } - return refs[0]?.name || refsPayload.defaultBaseRef || ""; -} - -function relativeTaskPath(path, rootPath) { - const normalizedPath = cleanLogicalPath(path); - const normalizedRoot = cleanLogicalPath(rootPath) || "."; - if (!normalizedPath) { - return ""; - } - if (normalizedPath === normalizedRoot) { - return ""; - } - const rootSegments = logicalSegments(normalizedRoot); - const pathSegments = logicalSegments(normalizedPath); - const common = rootSegments.findIndex((segment, index) => pathSegments[index] !== segment); - const commonLength = common < 0 - ? Math.min(rootSegments.length, pathSegments.length) - : common; - return [ - ...Array(rootSegments.length - commonLength).fill(".."), - ...pathSegments.slice(commonLength), - ].join("/"); -} - -function joinLogicalPath(root, relative) { - const segments = logicalSegments(cleanLogicalPath(root) || "."); - for (const segment of reviewFilePath(relative).split("/")) { - if (!segment) { - continue; - } - if (segment === "..") { - if (!segments.length) { - return ""; - } - segments.pop(); - } else { - segments.push(segment); - } - } - return segments.join("/"); -} - -function reviewFilePath(path) { - const segments = []; - for (const segment of `${path ?? ""}`.replaceAll("\\", "/").split("/")) { - if (!segment || segment === ".") { - continue; - } - if (segment === "..") { - if (segments.length && segments.at(-1) !== "..") { - segments.pop(); - } else { - segments.push(segment); - } - } else { - segments.push(segment); - } - } - return segments.join("/"); -} - -function logicalSegments(path) { - return `${path ?? ""}`.split("/").filter((segment) => segment && segment !== "."); -} - -function positiveLine(line) { - const value = Number(line); - return Number.isInteger(value) && value > 0 ? value : null; -} - function navigatorScroll(navigator) { if (navigator?.captureScroll) { return navigator.captureScroll(); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js index 06926681..f9c69254 100644 --- a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout.test.js @@ -37,7 +37,6 @@ test("merges Review axes with only the active navigator and selected viewer", () route: { navigator: "changes", scope: "working", viewer: "diff", path: "" }, ensureRendered() {}, querySelector(selector) { - if (selector.includes("empty-action")) return null; if (selector.includes("navigator-pane")) return panes.navigator; if (selector.includes("viewer-pane")) return panes.viewer; throw new Error(`Unexpected selector: ${selector}`); @@ -45,14 +44,14 @@ test("merges Review axes with only the active navigator and selected viewer", () axisControl(axis) { return { actionHintScope: () => scope(`${axis}-axis`) }; }, - workingTree() { + workingTreeChanges() { return { actionHintScope: (options) => { calls.working += 1; actionOptions.working = options; return scope("working"); } }; }, - branchTree() { + branchChanges() { return { actionHintScope: (options) => { calls.branch += 1; actionOptions.branch = options; @@ -66,7 +65,7 @@ test("merges Review axes with only the active navigator and selected viewer", () return scope("files"); } }; }, - viewer() { + selectedFile() { return { actionHintScope: (options) => { calls.viewer += 1; actionOptions.viewer = options; @@ -133,12 +132,11 @@ test("composes the shared Review panel resizer through its public scope", () => contextKey: "thread-a\0/repo", route: { navigator: "changes", scope: "working", path: "" }, ensureRendered() {}, - querySelector(selector) { - if (selector.includes("empty-action")) return null; + querySelector() { return { getClientRects: () => [{}] }; }, axisControl: () => null, - workingTree: () => ({ actionHintScope: () => scope("working") }), + workingTreeChanges: () => ({ actionHintScope: () => scope("working") }), resizer: () => panelResizer, }; @@ -172,12 +170,12 @@ test("merges only visible Review navigator and viewer Scroll leaves", () => { querySelector(selector) { return selector.includes("navigator-pane") ? panes.navigator : panes.viewer; }, - workingTree: () => ({ + workingTreeChanges: () => ({ scrollSurfaceScope: () => ({ surfaces: [navigatorSurface] }), }), - branchTree: () => null, + branchChanges: () => null, fileNavigator: () => null, - viewer: () => ({ + selectedFile: () => ({ scrollSurfaceScope: () => ({ surfaces: [viewerSurface] }), }), }; @@ -191,131 +189,3 @@ test("merges only visible Review navigator and viewer Scroll leaves", () => { panes.viewer.visible = false; assert.deepEqual(review.scrollSurfaceScope.call(owner).surfaces, []); }); - -test("drops a Branch compare that returns after Review leaves Branch", async (t) => { - const previousFetch = globalThis.fetch; - const previousWindow = globalThis.window; - const compares = []; - globalThis.window = { location: { origin: "http://caffold.test" } }; - globalThis.fetch = (input, ...options) => { - if (new URL(input).pathname !== "/api/git/compare") { - return previousFetch(input, ...options); - } - const response = Promise.withResolvers(); - compares.push(response); - return response.promise; - }; - t.after(() => { - globalThis.fetch = previousFetch; - if (previousWindow === undefined) delete globalThis.window; - else globalThis.window = previousWindow; - }); - const payload = { - baseRef: "origin/main", - headRef: "main", - files: [{ path: "src/planner.rs", repoRelativePath: "planner.rs", status: "M" }], - }; - const drawn = []; - const branchTree = { - setBaseSelection() {}, - setLoading() {}, - setEmptyMessage() {}, - setCompare: (compare) => drawn.push(compare), - updateCompare: (compare) => drawn.push(compare), - }; - const owner = { - active: true, - isConnected: true, - task: { threadId: "thread-a", worktree: { rootPath: "/repo", branch: "main" } }, - refs: { - repository: { rootPath: "/repo" }, - refs: [{ name: "origin/main", kind: "remote" }], - defaultBaseRef: "origin/main", - }, - route: { scope: "branch", baseRef: "origin/main", path: "" }, - branchGeneration: 0, - branchOutdated: false, - compare: null, - acceptRequest: review.acceptRequest, - patchRereadState: review.patchRereadState, - branchTree: () => branchTree, - patchControls() {}, - patchErrorState() {}, - patchEmptyStates() {}, - syncSelection() {}, - }; - - const current = review.ensureBranchData.call(owner); - compares.at(-1).resolve(new Response(JSON.stringify(payload))); - assert.deepEqual(await current, payload); - assert.equal(drawn.length, 1); - - const late = review.ensureBranchData.call(owner); - owner.route = { ...owner.route, scope: "working" }; - compares.at(-1).resolve(new Response(JSON.stringify(payload))); - assert.equal(await late, null); - assert.equal(drawn.length, 1); -}); - -test("drops a Working Tree status read sent before the page came back", async (t) => { - const previousFetch = globalThis.fetch; - const previousWindow = globalThis.window; - const statuses = []; - globalThis.window = { location: { origin: "http://caffold.test" } }; - globalThis.fetch = (input, ...options) => { - if (new URL(input).pathname !== "/api/git/status") { - return previousFetch(input, ...options); - } - const response = Promise.withResolvers(); - statuses.push(response); - return response.promise; - }; - t.after(() => { - globalThis.fetch = previousFetch; - if (previousWindow === undefined) delete globalThis.window; - else globalThis.window = previousWindow; - }); - const clean = { repository: { rootPath: "/repo" }, files: [] }; - const changed = { - repository: { rootPath: "/repo" }, - files: [{ path: "src/planner.rs", status: "M" }], - }; - const drawn = []; - const workingTree = { - setLoading() {}, - setStatus: (status) => drawn.push(status), - updateStatus: (status) => drawn.push(status), - }; - const owner = { - active: true, - isConnected: true, - task: { threadId: "thread-a", worktree: { rootPath: "/repo", branch: "main" } }, - route: { scope: "branch", baseRef: "origin/main", path: "" }, - status: clean, - statusOutdated: false, - branchOutdated: false, - statusGeneration: 0, - branchGeneration: 0, - pendingWatchViewerRefresh: false, - acceptRequest: review.acceptRequest, - patchRereadState: review.patchRereadState, - workingTree: () => workingTree, - patchErrorState() {}, - patchEmptyStates() {}, - syncSelection() {}, - }; - - const sentBefore = review.refreshWorking.call(owner, { background: true }); - review.markScopesOutdated.call(owner); - statuses.at(-1).resolve(new Response(JSON.stringify(changed))); - assert.equal(await sentBefore, null); - assert.equal(owner.status, clean); - assert.equal(owner.statusOutdated, true); - assert.equal(drawn.length, 0); - - const sentAfter = review.refreshWorking.call(owner, { background: true }); - statuses.at(-1).resolve(new Response(JSON.stringify(changed))); - assert.deepEqual(await sentAfter, changed); - assert.equal(owner.statusOutdated, false); - assert.deepEqual(drawn, [changed]); -}); diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/paths.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/paths.js new file mode 100644 index 00000000..7411ac3c --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/paths.js @@ -0,0 +1,73 @@ +import { cleanLogicalPath } from "#tasks/task-format.js"; + +export function taskRootPath(task) { + const path = task?.worktree?.rootPath || task?.cwdPath || task?.cwd || "."; + return cleanLogicalPath(path) || "."; +} + +export function taskWorktreeRootPath(task) { + const path = task?.worktree?.rootPath; + return path ? cleanLogicalPath(path) || "." : ""; +} + +export function relativeTaskPath(path, rootPath) { + const normalizedPath = cleanLogicalPath(path); + const normalizedRoot = cleanLogicalPath(rootPath) || "."; + if (!normalizedPath) { + return ""; + } + if (normalizedPath === normalizedRoot) { + return ""; + } + const rootSegments = logicalSegments(normalizedRoot); + const pathSegments = logicalSegments(normalizedPath); + const common = rootSegments.findIndex((segment, index) => pathSegments[index] !== segment); + const commonLength = common < 0 + ? Math.min(rootSegments.length, pathSegments.length) + : common; + return [ + ...Array(rootSegments.length - commonLength).fill(".."), + ...pathSegments.slice(commonLength), + ].join("/"); +} + +export function joinLogicalPath(root, relative) { + const segments = logicalSegments(cleanLogicalPath(root) || "."); + for (const segment of reviewFilePath(relative).split("/")) { + if (!segment) { + continue; + } + if (segment === "..") { + if (!segments.length) { + return ""; + } + segments.pop(); + } else { + segments.push(segment); + } + } + return segments.join("/"); +} + +export function reviewFilePath(path) { + const segments = []; + for (const segment of `${path ?? ""}`.replaceAll("\\", "/").split("/")) { + if (!segment || segment === ".") { + continue; + } + if (segment === "..") { + if (segments.length && segments.at(-1) !== "..") { + segments.pop(); + } else { + segments.push(segment); + } + } else { + segments.push(segment); + } + } + return segments.join("/"); +} + +function logicalSegments(path) { + return `${path ?? ""}`.split("/").filter((segment) => segment && segment !== "."); +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/route.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/route.js new file mode 100644 index 00000000..6cc748df --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/layout/route.js @@ -0,0 +1,72 @@ +import { taskThreadId } from "#tasks/task-list-model.js"; +import { representationForFile } from "../representations.js"; +import { reviewFilePath, taskRootPath, taskWorktreeRootPath } from "./paths.js"; + +export function normalizeReviewRoute(route = {}, task = null) { + const path = reviewFilePath(route?.path); + return { + scope: route?.reviewScope === "branch" ? "branch" : "working", + navigator: route?.reviewNavigator === "files" ? "files" : "changes", + viewer: ["source", "preview"].includes(route?.reviewViewer) + ? route.reviewViewer + : "diff", + path, + line: path ? positiveLine(route?.line) : null, + baseRef: `${route?.baseRef ?? ""}`, + ...(task ? { threadId: taskThreadId(task) } : {}), + }; +} + +export function taskRouteForReview(threadId, state) { + return { + kind: "tasks", + threadId, + review: true, + reviewScope: state.scope, + reviewNavigator: state.navigator, + reviewViewer: state.viewer, + path: state.path, + line: state.line, + baseRef: state.baseRef, + }; +} + +export function normalizeForTask(route, task) { + const taskRoute = task?.worktree + ? route + : { + ...route, + scope: "working", + navigator: "files", + viewer: route.viewer === "preview" ? "preview" : "source", + baseRef: "", + }; + const viewer = representationForFile(taskRoute.viewer, taskRoute.path); + return viewer === taskRoute.viewer ? taskRoute : { ...taskRoute, viewer }; +} + +export function reviewRouteKey(route) { + return [route.scope, route.navigator, route.viewer, route.path, route.line, route.baseRef].join("\u0000"); +} + +export function viewerStateKey(route) { + return `${route.scope}:${route.path}:${route.viewer}`; +} + +export function routeForSelectedFile(route, path) { + return { + ...route, + viewer: representationForFile(route.viewer, path), + path, + line: null, + }; +} + +export function reviewContextKey(task) { + return [taskThreadId(task), taskRootPath(task), taskWorktreeRootPath(task)].join("\u0000"); +} + +function positiveLine(line) { + const value = Number(line); + return Number.isInteger(value) && value > 0 ? value : null; +} diff --git a/frontend/pages/(task-workspace)/tasks/(detail)/(review)/representations.js b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/representations.js new file mode 100644 index 00000000..81b2de51 --- /dev/null +++ b/frontend/pages/(task-workspace)/tasks/(detail)/(review)/representations.js @@ -0,0 +1,46 @@ +import { + documentPreviewKind, + isPreviewableImagePath, +} from "#components/dom.js"; + +export function isFileRepresentation(viewer) { + return viewer === "source" || viewer === "preview"; +} + +export function representationForFile(viewer, path) { + if (!path || viewer === "diff") { + return viewer; + } + const representations = fileRepresentationCapabilities(path); + if ( + (viewer === "source" && representations.source) || + (viewer === "preview" && representations.previewKind) + ) { + return viewer; + } + return representations.source ? "source" : "preview"; +} + +export function fileRepresentationCapabilities(path) { + const documentKind = documentPreviewKind(path); + if (documentKind) { + return { source: false, previewKind: "document", documentKind }; + } + const imagePreview = isPreviewableImagePath(path); + return { + source: !imagePreview || isSvgPath(path), + previewKind: imagePreview + ? "image" + : isPreviewableMarkdownPath(path) + ? "markdown" + : null, + }; +} + +function isPreviewableMarkdownPath(path) { + return /\.(?:md|markdown)$/i.test(`${path ?? ""}`); +} + +function isSvgPath(path) { + return /\.svg$/i.test(`${path ?? ""}`); +} diff --git a/frontend/service-worker.js b/frontend/service-worker.js index 5f26439d..6ddf1cf6 100644 --- a/frontend/service-worker.js +++ b/frontend/service-worker.js @@ -318,8 +318,19 @@ const APP_SHELL_ASSETS = [ "/assets/pages/(task-workspace)/tasks/(detail)/(task)/components/conversation/components/work-details.js", "/assets/pages/(task-workspace)/tasks/(detail)/(review)/layout.css", "/assets/pages/(task-workspace)/tasks/(detail)/(review)/layout.js", - "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css", - "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/layout/paths.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/layout/route.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/changes-reading.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/representations.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.js", + "/assets/pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file/content.js", "/assets/pages/(task-workspace)/tasks/components/active-task-list.css", "/assets/pages/(task-workspace)/tasks/components/active-task-list.js", "/assets/pages/(task-workspace)/tasks/components/active-task-list/components/section.css", @@ -358,8 +369,8 @@ const APP_SHELL_ASSETS = [ "/assets/pages/(task-workspace)/tasks/(detail)/(git)/compare/page.js", "/assets/components/git-compare-browser.css", "/assets/components/git-compare-browser.js", - "/assets/components/git-compare-browser/compare-tree.css", - "/assets/components/git-compare-browser/compare-tree.js", + "/assets/components/git-compare-tree.css", + "/assets/components/git-compare-tree.js", "/assets/pages/(task-workspace)/tasks/(detail)/(github)/layout.css", "/assets/pages/(task-workspace)/tasks/(detail)/terminal/page.css", "/assets/pages/(task-workspace)/tasks/(detail)/terminal/page.js", diff --git a/frontend/styles.css b/frontend/styles.css index 4ce5ddd2..5cf7e180 100644 --- a/frontend/styles.css +++ b/frontend/styles.css @@ -110,14 +110,17 @@ @import "./pages/(task-workspace)/tasks/new/page.css"; @import "./pages/(task-workspace)/tasks/(detail)/(git)/layout.css"; @import "./pages/(task-workspace)/tasks/(detail)/(git)/components/controls.css"; -@import "./pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css"; +@import "./pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css"; +@import "./pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css"; +@import "./pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css"; +@import "./pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css"; @import "./pages/(task-workspace)/tasks/(detail)/(git)/(log)/layout.css"; @import "./pages/(task-workspace)/tasks/(detail)/(git)/(log)/list/page.css"; @import "./pages/(task-workspace)/tasks/(detail)/(git)/(log)/commit/page.css"; @import "./pages/(task-workspace)/tasks/(detail)/(git)/(log)/commit/components/changes-tree.css"; @import "./pages/(task-workspace)/tasks/(detail)/(git)/compare/page.css"; @import "./components/git-compare-browser.css"; -@import "./components/git-compare-browser/compare-tree.css"; +@import "./components/git-compare-tree.css"; @import "./pages/(task-workspace)/tasks/(detail)/(github)/layout.css"; @import "./pages/(task-workspace)/tasks/(detail)/terminal/page.css"; @import "./pages/(task-workspace)/tasks/(detail)/terminal/components/special-keys.css"; diff --git a/frontend/tests/contracts/css-ownership.test.mjs b/frontend/tests/contracts/css-ownership.test.mjs index 286788fe..48c7d96f 100644 --- a/frontend/tests/contracts/css-ownership.test.mjs +++ b/frontend/tests/contracts/css-ownership.test.mjs @@ -28,13 +28,25 @@ const ownership = new Map([ ], ["components/git-compare-browser.css", ["caffold-git-compare-browser"]], [ - "components/git-compare-browser/compare-tree.css", + "components/git-compare-tree.css", ["caffold-git-compare-tree"], ], [ - "pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css", + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes.css", + ["caffold-review-working-tree-changes"], + ], + [ + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css", ["caffold-git-diff-changes-tree"], ], + [ + "pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.css", + ["caffold-review-branch-changes"], + ], + [ + "pages/(task-workspace)/tasks/(detail)/(review)/components/selected-file.css", + ["caffold-review-selected-file"], + ], ["components/pdf-viewer.css", ["caffold-pdf-viewer"]], ["components/docx-viewer.css", ["caffold-docx-viewer"]], ["components/pptx-viewer.css", ["caffold-pptx-viewer"]], @@ -649,13 +661,16 @@ const componentChildren = new Map([ "caffold-task-review", [ "caffold-file-navigator", - "caffold-git-diff-changes-tree", - "caffold-git-compare-tree", + "caffold-review-working-tree-changes", + "caffold-review-branch-changes", "caffold-pane-resizer", - "caffold-review-file-viewer", + "caffold-review-selected-file", "caffold-segmented-control", ], ], + ["caffold-review-working-tree-changes", ["caffold-git-diff-changes-tree"]], + ["caffold-review-branch-changes", ["caffold-git-compare-tree"]], + ["caffold-review-selected-file", ["caffold-review-file-viewer"]], [ "caffold-git-compare-browser", [ diff --git a/frontend/tests/contracts/review-responsive.test.mjs b/frontend/tests/contracts/review-responsive.test.mjs index d7e9c667..9bb5930d 100644 --- a/frontend/tests/contracts/review-responsive.test.mjs +++ b/frontend/tests/contracts/review-responsive.test.mjs @@ -11,7 +11,7 @@ function readFrontend(path) { test("review file workspaces share one single-pane boundary", () => { const layoutStyles = [ "components/git-compare-browser.css", - "pages/(task-workspace)/tasks/(detail)/(review)/components/changes-tree.css", + "pages/(task-workspace)/tasks/(detail)/(review)/components/working-tree-changes/components/changes-tree.css", "pages/(task-workspace)/tasks/(detail)/(git)/(log)/commit/page.css", "pages/(task-workspace)/tasks/(detail)/(github)/(pulls)/files/page.css", ]; diff --git a/frontend/tests/contracts/task-workspace-structure.test.mjs b/frontend/tests/contracts/task-workspace-structure.test.mjs index 937a9f99..30638549 100644 --- a/frontend/tests/contracts/task-workspace-structure.test.mjs +++ b/frontend/tests/contracts/task-workspace-structure.test.mjs @@ -37,8 +37,8 @@ test("task workspace declares one shared master pane and one detail pane", () => const taskSummary = readFrontend( "pages/(task-workspace)/tasks/(detail)/(task)/components/summary.js", ); - const taskReview = readFrontend( - "pages/(task-workspace)/tasks/(detail)/(review)/layout.js", + const reviewBranchChanges = readFrontend( + "pages/(task-workspace)/tasks/(detail)/(review)/components/branch-changes.js", ); const settingsWorkspace = readFrontend( "pages/(task-workspace)/settings/layout.js", @@ -104,7 +104,7 @@ test("task workspace declares one shared master pane and one detail pane", () => assert.doesNotMatch(tasksPage, / reviewScenario.gitStatusRequests).toBe(statusBefore + 1); await expect(viewer.locator(".loading-panel")).toHaveCount(1); - await page.evaluate(() => { - const source = window.__caffoldMockEventSources.find( - (candidate) => - candidate.url.startsWith("/api/watch?") && candidate.readyState !== 2, - ); - if (!source) { - throw new Error("Missing Task Review watch source"); - } - source.emit("change", { - revision: 2, - paths: ["src/other.rs"], - gitStatusChanged: true, - gitRefsChanged: false, - overflow: false, - }); - }); + await emitGitStatusChanged(page, ["src/other.rs"]); await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 2); } finally { releaseStatus(); @@ -1080,6 +1065,83 @@ test("updates the loading viewer when a watch change replaces the Working Tree r await expect(viewer.locator(".loading-panel")).toHaveCount(0); }); +test("shows Branch loading, not the last error, while a failed reread is read again", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanBranch = true; + }, + }); + await selectTaskReviewScope(tasksPage, "branch"); + const compareTree = taskReview.locator("caffold-git-compare-tree"); + const viewer = taskReview.locator("caffold-review-file-viewer"); + await expect(compareTree).toContainText("No changes compared with origin/main."); + await taskReview.getByRole("button", { name: "Files", exact: true }).click(); + await taskReview + .locator('caffold-file-navigator button[data-file-tree-path="src/planner.rs"]') + .click(); + await taskReview.getByRole("button", { name: "Changes", exact: true }).click(); + await expect(viewer).toContainText("No changes in this scope."); + + await leavePage(page); + reviewScenario.failNextCompare = true; + await returnToPage(page); + await expect(viewer.locator(".error-panel")).toContainText("fixture compare unavailable"); + + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + await expect(viewer).toContainText("new planner behavior"); + const comparesBefore = reviewScenario.gitCompareRequests; + const releaseCompare = reviewScenario.holdCompare("origin/main"); + try { + await selectTaskReviewScope(tasksPage, "branch"); + await expect.poll(() => reviewScenario.gitCompareRequests).toBe(comparesBefore + 1); + await expect(viewer.locator(".loading-panel")).toHaveCount(1); + await expect(viewer.locator(".error-panel")).toHaveCount(0); + } finally { + releaseCompare(); + } + await expect(viewer).toContainText("No changes in this scope."); +}); + +test("keeps the Working Tree loading text while another reread starts", { tag: "@viewport-independent" }, async ({ + page, +}) => { + await controlPageVisibility(page); + const { reviewScenario, tasksPage, taskReview } = await openCompletedTaskForReview(page, { + configureReview(review) { + review.cleanWorkingTree = true; + }, + }); + await tasksPage.getByRole("button", { name: "Working Tree", exact: true }).click(); + const changes = taskReview.locator("caffold-git-diff-changes-tree"); + await expect(changes).toContainText("No changes."); + + await leavePage(page); + const statusBefore = reviewScenario.gitStatusRequests; + const releaseStatus = reviewScenario.holdStatus(); + try { + await returnToPage(page); + await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 1); + const loadingText = changes.locator("caffold-loading-text"); + await expect(loadingText).toHaveText("Loading changes..."); + await loadingText.evaluate((element) => { + window.__workingTreeLoadingText = element; + }); + await emitGitStatusChanged(page, ["src/other.rs"]); + await expect.poll(() => reviewScenario.gitStatusRequests).toBe(statusBefore + 2); + expect( + await changes.evaluate( + (tree) => tree.querySelector("caffold-loading-text") === window.__workingTreeLoadingText, + ), + ).toBe(true); + } finally { + releaseStatus(); + } + await expect(changes).toContainText("No changes."); +}); + test("shows the error where Branch was loading when reading it again fails", { tag: "@viewport-independent" }, async ({ page, }) => { @@ -2022,6 +2084,25 @@ async function emitGitRefsChanged(page) { }); } +async function emitGitStatusChanged(page, paths) { + await page.evaluate((changedPaths) => { + const source = window.__caffoldMockEventSources.find( + (candidate) => + candidate.url.startsWith("/api/watch?") && candidate.readyState !== 2, + ); + if (!source) { + throw new Error("Missing Task Review watch source"); + } + source.emit("change", { + revision: Date.now(), + paths: changedPaths, + gitStatusChanged: true, + gitRefsChanged: false, + overflow: false, + }); + }, paths); +} + async function controlPageVisibility(page) { await page.addInitScript(() => { window.__caffoldVisibilityState = "visible";