Fix Review showing Git changes read before the page came back

caffold/src/static_assets.rs

@@ -1152,14 +1152,65 @@ pub(crate) fn get(path: &str) -> Option<StaticAsset> {
         "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<StaticAsset> {
         "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,

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

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 {

frontend/components/git-compare-browser/compare-tree.js → 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() {

frontend/components/git-compare-browser/compare-tree.test.js → 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());
 

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 });
+}