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 8696f424..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, @@ -1322,9 +1338,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 +1454,21 @@ 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. +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 `caffold-task-git-layout` is a direct shared Detail child under the pathless @@ -1920,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 @@ -1935,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 92c18710..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,14 +48,13 @@ class CaffoldTaskReview extends HTMLElement { connectedCallback() { this.ensureRendered(); this.active = true; + this.markChangesOutdated(); this.syncReview({ reactivated: true }); } disconnectedCallback() { - this.viewer()?.deactivate?.(); this.active = false; this.unsubscribeWatch(); - this.invalidateRequests(); this.fileNavigator()?.setWatchActive(false); } @@ -70,20 +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.statusGeneration = 0; - this.branchGeneration = 0; - this.viewerGeneration = 0; 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 = ` @@ -97,14 +85,10 @@ class CaffoldTaskReview extends HTMLElement {
- - +
- +
@@ -122,7 +106,7 @@ class CaffoldTaskReview extends HTMLElement {
- +
@@ -132,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; @@ -154,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); @@ -168,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) => { @@ -207,6 +198,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 && @@ -218,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) { @@ -225,22 +218,19 @@ 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) { this.requestRoute(normalizedForTask); return true; } - this.syncReview({ contextChanged, routeChanged, revealFileSelection }); + this.syncReview({ contextChanged, routeChanged, scopeChanged, revealFileSelection }); return true; } @@ -305,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" @@ -341,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, @@ -383,7 +351,6 @@ class CaffoldTaskReview extends HTMLElement { return mergeActionHintScopes( navigatorAxis, viewerAxis, - emptyScope, navigatorScope, resizerScope, viewerScope, @@ -414,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, @@ -427,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, }) @@ -451,31 +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.pendingWatchViewerRefresh = false; this.previousRouteKey = ""; - this.workingTree()?.reset(); - this.branchTree()?.reset(); this.fileNavigator()?.clearSelectedPath(); - 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, + }); + } + + markChangesOutdated() { + this.workingTreeChanges()?.markOutdated(); + this.branchChanges()?.markOutdated(); } syncReview(options = {}) { @@ -491,29 +464,30 @@ class CaffoldTaskReview extends HTMLElement { }); this.subscribeWatch(taskRootPath(this.task)); if (this.task.worktree) { - if (options.contextChanged || options.reactivated || !this.status) { - void this.refreshWorking(); - } + const working = this.workingTreeChanges().snapshot(); if ( - this.route.scope === "branch" && - (options.reactivated || - !this.refs || - !this.compare || - this.compare.baseRef !== this.route.baseRef) + options.contextChanged || + options.reactivated || + !working.data || + (options.scopeChanged && this.route.scope === "working" && working.outdated) ) { - void this.ensureBranchData({ force: options.reactivated }); + 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.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(); } @@ -559,323 +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, - }); - } - try { - const status = await getGitStatus(rootPath); - if (!this.acceptRequest("status", generation, rootPath)) { - return null; - } - const hadStatus = Boolean(this.status); - this.status = status; - 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; - const refreshViewer = options.background - ? canRefreshViewer && pendingWatchViewerRefresh - : canRefreshViewer; - if (refreshViewer) { - void this.loadViewer({ background: Boolean(options.background) }); - } - return status; - } catch (error) { - if (!this.acceptRequest("status", generation, rootPath)) { - return null; - } - this.statusError = error; - if (!this.status) { - this.workingTree()?.setError(error, { rootPath }); - } - 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; - 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.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(); + this.selectedFile()?.reload(this.currentView(), { reread: hadData }); } - 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.patchErrorState(); - return null; + return; + } + 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 }); } } - 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.viewer()?.setEmpty(); - return; - } - const generation = ++this.viewerGeneration; - 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.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({ - kind: representations.documentKind, - path: selectedPath, - name: fileNameFromPath(selectedPath), - size: entry?.size, - modifiedMs: entry?.modifiedMs, - }, { recheck }); + 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 }); - return; - } - if (sourceMode && !representations.source) { - this.viewer()?.setNotice("Source is unavailable for this file.", { - actionLabel: "View preview", - action: "view-preview", - title: fileNameFromPath(selectedPath), - }); - return; - } - if (previewMode && !representations.previewKind) { - this.viewer()?.setNotice("Preview is unavailable for this file.", { - actionLabel: "View source", - action: "view-source", - title: fileNameFromPath(selectedPath), - }); - return; - } - if (!fileMode && !change) { - 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(this.viewerStateKey()); - const background = Boolean(options.background); - 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, - }); - } - if (!background && !(sourceMode && this.route.line)) { - this.restoreViewerPosition(saved); - } - } catch (error) { - if (this.acceptViewer(generation, selectedPath)) { - viewer?.setError(presentation, error); - } - } - } - - 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 - : "", - }); + this.selectedFile()?.reload(this.currentView()); } - 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() { @@ -885,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 = [ @@ -930,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}` : ""; @@ -957,44 +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.status.files?.length ?? 0) === 0); - this.querySelector(".task-review-empty-action")?.toggleAttribute("hidden", !workingEmpty); } 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]") @@ -1012,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 }); } @@ -1030,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; @@ -1063,6 +754,7 @@ class CaffoldTaskReview extends HTMLElement { } async refreshAll() { + this.markChangesOutdated(); const requests = [ this.fileNavigator()?.requestRefresh({ allDirectories: true, @@ -1072,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)); @@ -1117,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(); } } }, @@ -1186,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() { @@ -1230,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 fd2d4ad3..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,66 +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, - compare: null, - acceptRequest: review.acceptRequest, - 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); -}); 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, / { + 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..db8c59fa 100644 --- a/frontend/tests/e2e/tasks/review-git.spec.js +++ b/frontend/tests/e2e/tasks/review-git.spec.js @@ -911,6 +911,384 @@ 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 emitGitStatusChanged(page, ["src/other.rs"]); + 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 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, +}) => { + 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 +2083,112 @@ 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"; + 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, }) => {