diff --git a/apps/web/components/review/review-diff-header.tsx b/apps/web/components/review/review-diff-header.tsx index 519106aef3..1779fe969d 100644 --- a/apps/web/components/review/review-diff-header.tsx +++ b/apps/web/components/review/review-diff-header.tsx @@ -26,6 +26,7 @@ type ReviewDiffHeaderProps = ReviewExternalLinkContext & { collapsed: boolean; wordWrap: boolean; expandUnchanged: boolean; + hasStickyRepoHeader?: boolean; onCheckboxChange: (checked: boolean | "indeterminate") => void; onDiscard: () => void; onOpenFile?: (filePath: string, repo?: string) => void; @@ -262,6 +263,7 @@ export function ReviewDiffHeader({ collapsed, wordWrap, expandUnchanged, + hasStickyRepoHeader = false, sessionId, onCheckboxChange, onDiscard, @@ -313,7 +315,8 @@ export function ReviewDiffHeader({ data-testid="review-file-header" data-file-path={file.path} className={cn( - "sticky top-0 z-10 border-b border-border/50 bg-card/95 backdrop-blur-sm", + "sticky z-10 border-b border-border/50 bg-card/95 backdrop-blur-sm", + hasStickyRepoHeader ? "top-8" : "top-0", !isMobile && "flex items-center gap-2 px-4 py-2", )} > diff --git a/apps/web/components/review/review-diff-list-groups.tsx b/apps/web/components/review/review-diff-list-groups.tsx index ac20c7f92d..f78813d90c 100644 --- a/apps/web/components/review/review-diff-list-groups.tsx +++ b/apps/web/components/review/review-diff-list-groups.tsx @@ -22,9 +22,10 @@ export function RepoGroupHeader({ // The empty-name group ("uncategorised" — no repository_name on its files) // gets a generic label so the user understands what they're looking at. const label = name || t("task:otherChanges"); + // Keep h-8 aligned with ReviewDiffHeader's top-8 and grouped sections' scroll-mt-8. return (
= 0 && @@ -164,6 +166,7 @@ type FileDiffSectionProps = { autoMarkOnScroll: boolean; wordWrap: boolean; enableWalkthroughAnnotations: boolean; + hasStickyRepoHeader: boolean; isSelected?: boolean; forceLoad?: boolean; onToggleReviewed: (key: string, reviewed: boolean) => void; @@ -539,6 +542,7 @@ function FileDiffSection({ autoMarkOnScroll, wordWrap, enableWalkthroughAnnotations, + hasStickyRepoHeader, isSelected, forceLoad, onToggleReviewed, @@ -584,7 +588,10 @@ function FileDiffSection({ }); return ( -
+
{ - test.describe.configure({ retries: 1, timeout: 180_000 }); + test.describe.configure({ timeout: 180_000 }); test("keeps nested scope and diff context touch-reachable", async ({ testPage, @@ -44,6 +47,7 @@ test.describe("Nested submodule Review on mobile", () => { hasText: /^vendor\/outer\/vendor\/inner$/, }); await expect(innerLabel).toBeVisible({ timeout: 15_000 }); + await expectStickyReviewHeaderClearance(review, "touch"); const innerHeader = review .getByTestId("review-file-header") diff --git a/apps/web/e2e/tests/review/submodule-review-helpers.ts b/apps/web/e2e/tests/review/submodule-review-helpers.ts index db3362cb23..990d7bcf57 100644 --- a/apps/web/e2e/tests/review/submodule-review-helpers.ts +++ b/apps/web/e2e/tests/review/submodule-review-helpers.ts @@ -2,6 +2,7 @@ import crypto from "node:crypto"; import fs from "node:fs"; import path from "node:path"; import { execFileSync } from "node:child_process"; +import { expect, type Locator } from "@playwright/test"; import type { ApiClient } from "../../helpers/api-client"; import { dwell } from "../../helpers/causal-waits"; import type { SeedData } from "../../fixtures/test-base"; @@ -11,6 +12,76 @@ const GIT_PROTOCOL_ARGS = ["-c", "protocol.file.allow=always"]; const GIT_INDEX_LOCK_ATTEMPTS = 3; const GIT_INDEX_LOCK_RETRY_MS = 300; +async function waitForFiniteAnimations(surface: Locator): Promise { + await surface.evaluate(async (element) => { + const animations = element.getAnimations({ subtree: true }).filter((animation) => { + const iterations = animation.effect?.getComputedTiming().iterations; + return typeof iterations === "number" && Number.isFinite(iterations); + }); + await Promise.all(animations.map((animation) => animation.finished.catch(() => undefined))); + }); +} + +export async function expectStickyReviewHeaderClearance( + review: Locator, + pointer: "mouse" | "touch", +): Promise { + const rootGroup = review.locator('[data-testid="changes-repo-group"][data-repository-name=""]'); + const repositoryHeader = rootGroup.getByTestId("changes-repo-header"); + const fileHeader = rootGroup.locator( + '[data-testid="review-file-header"][data-file-path="README.md"]', + ); + const fileSection = fileHeader.locator(".."); + const disclosure = fileHeader.getByRole("button", { + name: /^(Collapse|Expand) README\.md$/, + }); + + await expect(repositoryHeader).toContainText("Other changes"); + await expect(fileHeader).toHaveCount(1); + await expect(disclosure).toHaveCount(1); + await waitForFiniteAnimations(review); + await fileSection.evaluate((element) => + element.scrollIntoView({ behavior: "auto", block: "start" }), + ); + + await expect + .poll( + async () => { + const [repositoryBox, fileBox] = await Promise.all([ + repositoryHeader.boundingBox(), + fileHeader.boundingBox(), + ]); + const disclosureHit = await disclosure.evaluate((control) => { + const box = control.getBoundingClientRect(); + const hit = document.elementFromPoint(box.left + box.width / 2, box.top + box.height / 2); + return hit === control || (hit !== null && control.contains(hit)); + }); + const clearance = + repositoryBox && fileBox + ? Math.round((fileBox.y - (repositoryBox.y + repositoryBox.height)) * 100) / 100 + : null; + return { + clearance, + headersSeparated: clearance !== null && clearance >= 0, + disclosureHit, + }; + }, + { message: "repository header must not cover the current file header or disclosure" }, + ) + .toEqual({ + clearance: expect.any(Number), + headersSeparated: true, + disclosureHit: true, + }); + + if (pointer === "touch") { + await disclosure.tap(); + } else { + await disclosure.click(); + } + await expect(disclosure).toHaveAttribute("aria-expanded", "false"); +} + export type SubmoduleReviewFixture = { taskId: string; sessionId: string; @@ -165,7 +236,10 @@ export async function createSubmoduleReviewFixture( ), ); } - fs.appendFileSync(path.join(worktreePath, "README.md"), "parent working-tree change\n"); + fs.appendFileSync( + path.join(worktreePath, "README.md"), + `parent working-tree change\n${"root review line\n".repeat(80)}`, + ); fs.appendFileSync(path.join(outerWorktree, "README.md"), "outer committed change\n"); await commit(outerWorktree, env, "change outer submodule"); fs.appendFileSync(path.join(innerWorktree, "README.md"), "inner committed change\n"); diff --git a/apps/web/e2e/tests/review/submodule-review.spec.ts b/apps/web/e2e/tests/review/submodule-review.spec.ts index 6d7b2060e5..e0cff14081 100644 --- a/apps/web/e2e/tests/review/submodule-review.spec.ts +++ b/apps/web/e2e/tests/review/submodule-review.spec.ts @@ -3,7 +3,11 @@ import fs from "node:fs"; import path from "node:path"; import { test, expect } from "../../fixtures/test-base"; import { SessionPage } from "../../pages/session-page"; -import { createSubmoduleReviewFixture, readGitValue } from "./submodule-review-helpers"; +import { + createSubmoduleReviewFixture, + expectStickyReviewHeaderClearance, + readGitValue, +} from "./submodule-review-helpers"; test.describe("Submodule review fixture", () => { test("cleans source repositories when setup fails", async ({ apiClient, seedData, backend }) => { @@ -25,7 +29,7 @@ test.describe("Submodule review fixture", () => { }); test.describe("Nested submodule Review", () => { - test.describe.configure({ retries: 1, timeout: 180_000 }); + test.describe.configure({ timeout: 180_000 }); test("shows nested scopes and commits child gitlinks through the UI", async ({ testPage, @@ -86,6 +90,7 @@ test.describe("Nested submodule Review", () => { for (const expected of ["outer committed change", "inner committed change"]) { await expect.poll(() => session.reviewDiffText(), { timeout: 45_000 }).toContain(expected); } + await expectStickyReviewHeaderClearance(review, "mouse"); await expect( review.locator('[data-testid="review-file-row"][data-file-path="vendor/outer"]'), diff --git a/docs/plans/review-sticky-header-clearance/plan.md b/docs/plans/review-sticky-header-clearance/plan.md new file mode 100644 index 0000000000..89f2ed07ac --- /dev/null +++ b/docs/plans/review-sticky-header-clearance/plan.md @@ -0,0 +1,102 @@ +--- +spec: docs/specs/ui/submodule-review.md +created: 2026-08-21 +status: implemented +--- + +# Implementation Plan: Review sticky header clearance + +## Overview + +Keep Review's repository-scope header sticky while reserving a separate sticky lane for the current file header. Prove the overlap and hit-target failure first in the existing nested-submodule desktop and phone flows, then add one shared grouped-header offset used by both responsive file-header compositions. + +The repair changes only Review header geometry. Repository grouping, labels, ordering, file identity, actions, scrolling ownership, and Changes-panel headers remain unchanged. + +## Root cause + +`apps/web/components/review/review-diff-list.tsx` renders repository headers when the diff contains multiple repository scopes or its sole scope has a name. In the nested-submodule flow, workspace-root files form an empty-name group that `RepoGroupHeader` labels **Other changes**. + +`RepoGroupHeader` in `apps/web/components/review/review-diff-list-groups.tsx` and `ReviewDiffHeader` in `apps/web/components/review/review-diff-header.tsx` both use `position: sticky` with `top: 0`. The repository header has `z-20`, while the file header has `z-10`. Once scrolling clamps both headers to the scroll owner's top edge, the repository header paints over the file name and controls. A single unnamed repository does not reproduce the bug because `showRepoHeaders` is false and no repository header renders. + +## Frontend + +### Sticky repository and file lanes + +- `apps/web/components/review/review-diff-list-groups.tsx`: give `RepoGroupHeader` one explicit, stable header height using the existing spacing scale. Keep its sticky position, content, truncation, borders, and z-index. +- `apps/web/components/review/review-diff-list.tsx`: propagate whether repository headers are rendered into each `FileDiffSection`. Apply matching scroll margin to grouped file sections so programmatic file navigation still aligns the file header beneath the repository lane. +- `apps/web/components/review/review-diff-header.tsx`: use `top: 0` when no repository header exists and the repository-header height as the sticky inset when one does. Reuse the same inset for desktop and mobile header compositions; do not duplicate breakpoint-specific positioning. + +### Mobile design contract + +- **Desktop outcome:** the repository scope remains visible at the top of the Review diff, with the current file name and actions directly below it and fully clickable. +- **Mobile entry point:** the existing Changes action opens the same full-height Review dialog. No navigation or control moves. +- **Nearest shipped exemplar:** `apps/web/components/review/review-diff-header.tsx` and `apps/web/e2e/tests/review/mobile-submodule-review.spec.ts` remain the phone Review pattern; this repair changes only their shared sticky geometry. +- **Hierarchy and presentation:** repository scope first, current file identity second, diff content third. The existing full-height surface remains appropriate for dense diff content. +- **Scroll, viewport, and touch:** `review-diff-scroll` remains the single vertical scroll owner. Dialog viewport sizing, safe-area behavior, and existing touch-target sizes remain unchanged; the file disclosure control must stay the topmost hit target at its center. +- **Shared behavior:** repository grouping and the grouped-header flag remain shared. Desktop and mobile render different file-header content but consume the same sticky inset. +- **Parity proof:** the existing desktop and `mobile-chrome` nested-submodule scenarios assert identical non-overlap and hit-target contracts for the **Other changes** group. + +## Tests + +This is browser layout behavior. Happy DOM component tests cannot evaluate sticky positioning, stacking contexts, bounding boxes, or `document.elementFromPoint()`, so no React component test can provide a meaningful regression. + +- **What:** a grouped file header receives the repository-header sticky inset while an ungrouped file header retains `top: 0` behavior. + **File:** exercised through `apps/web/e2e/tests/review/submodule-review-helpers.ts` from both owning E2E specs. + **How:** scroll the first workspace-root file header to the scroll-owner start, compare the repository and file header bounding boxes, and check that the file disclosure control owns the element at its center point. + +## E2E Tests + +- **Scenario:** GIVEN root and nested-submodule changes, WHEN the desktop reviewer scrolls the root file beneath **Other changes**, THEN the repository header bottom does not cross the file header top and the file disclosure remains the real hit target. + **File:** `apps/web/e2e/tests/review/submodule-review.spec.ts` with shared geometry logic in `submodule-review-helpers.ts`. +- **Scenario:** GIVEN the same mixed scopes on a phone, WHEN the reviewer scrolls the root file beneath **Other changes**, THEN the same geometry and hit-target guarantees hold without changing the existing mobile Review composition. + **File:** `apps/web/e2e/tests/review/mobile-submodule-review.spec.ts` in the `mobile-chrome` project. +- **TDD order:** add both assertions and run them against current source. They must fail because both sticky headers resolve to `top: 0`; then make the minimal source change and rerun both against a fresh production build. + +## Verification Results + +- RED, desktop: the focused Chromium scenario failed with + `headersSeparated: false` and `disclosureHit: false` after the root file was + scrolled under **Other changes**. +- RED, phone: the focused `mobile-chrome` scenario failed with the same two + values. Both RED commands used `--workers=1 --retries=0`. +- GREEN, desktop: the focused Chromium scenario passed against a fresh + production build (`1 passed`). Its final section-scroll rerun also passed + (`1 passed`, 13.8s). +- GREEN, phone: the final `mobile-chrome` section-scroll run passed (`1 passed`, + 9.9s). One earlier attempt never reached the UI because the disposable Git + fixture encountered an `index.lock`; a clean run with Playwright retries + still disabled passed. +- Rendered geometry changed from a 30px repository header and `top: 0` file + header (30px nominal overlap) to a fixed 32px repository lane and 32px grouped + file inset (0px nominal overlap). Browser geometry and center-point hit tests + changed from false to true on both viewports. +- `pnpm exec vitest run components/review/review-diff-header.test.tsx` passed all + 8 tests. `pnpm run typecheck`, scoped ESLint, scoped Prettier, and + `git diff --check` passed. +- Managed E2E runs cleaned their disposable repositories. No generated output + is present in the tracked diff. + +## Implementation Waves And Parallel Candidates + +Wave 1 (sequential): + +- [x] [Task 01: Separate Review sticky headers](task-01-separate-review-sticky-headers.md) + +The E2E regression and shared header geometry describe one visual contract and touch overlapping files, so the task is not parallel-safe. + +## Risks + +- The grouped offset must match the repository header's rendered height; define both from the same spacing value instead of duplicating unrelated pixel values. +- Programmatic file jumps use `scrollIntoView({ block: "start" })`; grouped sections need matching scroll margin so the fixed lane does not obscure the selected file's first diff content. +- The ungrouped single-repository path must keep its current zero inset and must not gain blank space above file headers. + +## Out of scope + +- Changing repository grouping, the **Other changes** label, section ordering, or submodule identity. +- Making repository headers non-sticky or changing their z-index hierarchy. +- Redesigning Review, its toolbar, file tree, diff viewer, or mobile navigation. +- Changing the dockview Changes panel, backend Git data, APIs, persistence, or localization. + +## Open Questions + +None. diff --git a/docs/plans/review-sticky-header-clearance/task-01-separate-review-sticky-headers.md b/docs/plans/review-sticky-header-clearance/task-01-separate-review-sticky-headers.md new file mode 100644 index 0000000000..ae16b8fcbe --- /dev/null +++ b/docs/plans/review-sticky-header-clearance/task-01-separate-review-sticky-headers.md @@ -0,0 +1,113 @@ +--- +id: "01-separate-review-sticky-headers" +title: "Separate Review sticky headers" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/submodule-review.md" +--- + +# Task 01: Separate Review sticky headers + +## Intent + +Keep repository-scope and current-file headers simultaneously visible and interactive in Review without changing repository grouping or responsive composition. + +## Acceptance + +- With workspace-root and submodule changes, the sticky **Other changes** header ends at or above the current file header on desktop and phone. +- The current file disclosure control remains the topmost element at its center and can still be activated after the headers stick. +- A Review diff with no rendered repository header keeps the existing zero sticky inset and gains no empty header lane. +- Programmatic file navigation aligns grouped file sections below the repository header rather than hiding the selected header or first diff content. + +## Files likely touched + +- `apps/web/components/review/review-diff-list-groups.tsx` +- `apps/web/components/review/review-diff-list.tsx` +- `apps/web/components/review/review-diff-header.tsx` +- `apps/web/e2e/tests/review/submodule-review-helpers.ts` +- `apps/web/e2e/tests/review/submodule-review.spec.ts` +- `apps/web/e2e/tests/review/mobile-submodule-review.spec.ts` +- `docs/plans/review-sticky-header-clearance/plan.md` +- `docs/plans/review-sticky-header-clearance/task-01-separate-review-sticky-headers.md` + +## Dependencies + +None. + +## Parallelism + +`sequential`. Regression assertions and source changes share one sticky-layout contract and overlapping files. + +## Inputs + +- `docs/specs/ui/submodule-review.md`, especially the mixed root/submodule sticky-header requirement and scenario. +- `docs/plans/review-sticky-header-clearance/plan.md`, including **Root cause**, **Sticky repository and file lanes**, and **Mobile design contract**. +- `apps/web/components/review/review-diff-list.tsx`: `showRepoHeaders`, `FileDiffSection`, and programmatic `scrollIntoView` behavior. +- `/tdd`, `/e2e`, and `/mobile-parity` guidance. Use strict Red-Green-Refactor; no production change before both browser regressions fail for the expected overlap. + +## TDD sequence + +1. **RED:** add a shared nested-submodule Review assertion that scrolls the root file header to the top, requires the **Other changes** header to end above the file header, and requires `document.elementFromPoint()` at the disclosure center to resolve to the control. Invoke it from desktop and mobile specs. Run both with `--retries=0`; record failures showing intersecting boxes and the covering repository header owning the hit point. +2. **GREEN:** give the repository header a stable height, pass the grouped-header state to each file section, offset grouped file headers by that height, and give grouped sections matching scroll margin. Do not alter ungrouped headers. +3. **REFACTOR:** keep one shared geometry assertion and one shared spacing value per rendered relationship. Remove duplication without introducing a general sticky-header abstraction. +4. Rerun both E2E scenarios after the last source edit against the rebuilt production bundle. + +## Verification + +Bootstrap once if this worktree lacks dependencies: + +```bash +cd apps && pnpm install --frozen-lockfile +``` + +Run desktop and phone regressions in order. The first command rebuilds current production assets; the second reuses those unchanged assets: + +```bash +cd apps/web && pnpm e2e:run tests/review/submodule-review.spec.ts -- --grep "shows nested scopes and commits child gitlinks through the UI" --workers=1 --retries=0 +cd apps/web && pnpm e2e:run --no-build --project mobile-chrome tests/review/mobile-submodule-review.spec.ts -- --grep "keeps nested scope and diff context touch-reachable" --workers=1 --retries=0 +``` + +Run focused frontend checks: + +```bash +cd apps/web && pnpm run typecheck +cd apps && pnpm --filter @kandev/web exec eslint components/review/review-diff-list-groups.tsx components/review/review-diff-list.tsx components/review/review-diff-header.tsx e2e/tests/review/submodule-review-helpers.ts e2e/tests/review/submodule-review.spec.ts e2e/tests/review/mobile-submodule-review.spec.ts +cd apps && pnpm exec prettier --check web/components/review/review-diff-list-groups.tsx web/components/review/review-diff-list.tsx web/components/review/review-diff-header.tsx web/e2e/tests/review/submodule-review-helpers.ts web/e2e/tests/review/submodule-review.spec.ts web/e2e/tests/review/mobile-submodule-review.spec.ts ../docs/specs/ui/submodule-review.md ../docs/plans/review-sticky-header-clearance/plan.md ../docs/plans/review-sticky-header-clearance/task-01-separate-review-sticky-headers.md +git diff --check +``` + +## Output contract + +Report the RED failure messages for desktop and phone, final command outcomes and test counts, exact changed files, rendered geometry deltas, generated artifacts and cleanup evidence, residual risks, and synchronized task/plan status. State that no backend, API, persistence, localization, or external side effect changed. + +## Results + +- TDD RED reproduced the overlap in both focused browser scenarios. Desktop and + `mobile-chrome` each reported `headersSeparated: false` and + `disclosureHit: false` with retries disabled. +- GREEN gives `RepoGroupHeader` a fixed `h-8` lane. `ReviewDiffList` opts grouped + sections into matching `scroll-mt-8`, and `ReviewDiffHeader` uses `top-8` only + when that repository lane exists; direct and ungrouped callers default to + `top-0`. +- The shared E2E helper now scrolls the file section as production navigation + does, compares repository/file bounding boxes, verifies the disclosure owns + its center hit point, and activates it with mouse or touch. Final desktop and + phone runs each passed one focused scenario with `--retries=0`. +- Geometry delta: repository header 30px to 32px; grouped file sticky inset 0px + to 32px; nominal overlap 30px to 0px. Browser separation and hit-target states + changed false to true on desktop and phone. +- Changed code/tests: + `review-diff-list-groups.tsx`, `review-diff-list.tsx`, + `review-diff-header.tsx`, `submodule-review-helpers.ts`, + `submodule-review.spec.ts`, and `mobile-submodule-review.spec.ts`. Durable + records changed in the owning spec, plan, and this task file. +- Verification: header unit suite 8/8; web typecheck, scoped ESLint, scoped + Prettier, and `git diff --check` all passed. A single mobile setup attempt hit + a transient disposable-repository `index.lock`; a clean zero-retry run passed. +- Managed E2E cleanup removed disposable repositories; no generated artifact is + tracked. Residual risk is limited to keeping the three matching Tailwind + `8` spacing classes synchronized if this geometry changes later. +- No backend, API, persistence, localization, external-service, or other + external side effect changed. diff --git a/docs/specs/ui/submodule-review.md b/docs/specs/ui/submodule-review.md index 1491b04858..ef84604b59 100644 --- a/docs/specs/ui/submodule-review.md +++ b/docs/specs/ui/submodule-review.md @@ -24,6 +24,7 @@ Repositories that use Git submodules currently show only a changed gitlink commi - Stage, unstage, discard, file-at-ref, and per-file editing actions execute in the file's owning repository scope. - A commit-all operation that covers nested repositories commits the deepest changed submodules before their parents so every parent commit records the resulting child gitlink. Independent sibling repositories at the same depth may proceed in parallel. - Desktop keeps the existing Review split surface and file tree. Phone Review keeps its existing full-height diff surface; submodule identity is visible in the sticky repository/diff header, so no required cue depends on the desktop-only file tree or hover. +- When repository-scope and file headers are both sticky, they occupy separate vertical lanes. A scope header, including **Other changes** for the workspace root, never covers the current file identity or its controls on desktop or phone. ## API surface @@ -38,14 +39,14 @@ For a multi-repository task, a top-level attached repository keeps its existing ## Failure modes -| Condition | Observable behavior | -|---|---| -| A declared submodule is uninitialized, inaccessible, or its Git metadata is invalid | The child scope is skipped, the rest of the repository graph remains reviewable, and the parent gitlink change remains visible when changed. | -| One submodule status, log, or cumulative-diff request fails | Other repository scopes remain available and the existing per-repository partial-failure behavior applies. | -| A submodule comparison gitlink cannot be resolved from the parent comparison tree | Uncommitted child changes remain visible; committed-range collection follows the existing unresolved-base failure behavior for that scope without substituting another repository's base. | -| The same relative file path changes in a parent, sibling repository, and submodule | Review keeps separate identities and comments for every `(repository_name, path)` pair. | -| Commit-all fails in one child scope | Ancestor commits that depend on that child are not attempted; independent scopes report their own results through the existing partial-success UI. | -| A submodule is added or removed after agentctl's repository graph is built | It becomes part of nested review after the existing workspace rescan or session restart; automatic hot discovery of arbitrary graph changes is not required in this iteration. | +| Condition | Observable behavior | +| ----------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| A declared submodule is uninitialized, inaccessible, or its Git metadata is invalid | The child scope is skipped, the rest of the repository graph remains reviewable, and the parent gitlink change remains visible when changed. | +| One submodule status, log, or cumulative-diff request fails | Other repository scopes remain available and the existing per-repository partial-failure behavior applies. | +| A submodule comparison gitlink cannot be resolved from the parent comparison tree | Uncommitted child changes remain visible; committed-range collection follows the existing unresolved-base failure behavior for that scope without substituting another repository's base. | +| The same relative file path changes in a parent, sibling repository, and submodule | Review keeps separate identities and comments for every `(repository_name, path)` pair. | +| Commit-all fails in one child scope | Ancestor commits that depend on that child are not attempted; independent scopes report their own results through the existing partial-success UI. | +| A submodule is added or removed after agentctl's repository graph is built | It becomes part of nested review after the existing workspace rescan or session restart; automatic hot discovery of arbitrary graph changes is not required in this iteration. | ## Scenarios @@ -58,6 +59,7 @@ For a multi-repository task, a top-level attached repository keeps its existing - **GIVEN** `README.md` changes in the parent and `README.md` changes in a nested submodule, **WHEN** the user reviews, comments on, stages, or discards either file, **THEN** the action and review state apply only to the selected repository scope. - **GIVEN** staged changes in a nested submodule and its parent, **WHEN** the user commits all reviewed work, **THEN** Kandev commits the child first and the parent commit records the child's new commit ID. - **GIVEN** a phone-sized viewport, **WHEN** the user opens Review for a submodule file, **THEN** the sticky diff header identifies the submodule scope and the file remains reviewable without document-level horizontal overflow. +- **GIVEN** changed files in the workspace root and a nested submodule, **WHEN** the reviewer scrolls a root file beneath the sticky **Other changes** scope header on desktop or phone, **THEN** the scope header remains above the file header without intersecting its file identity or control hit targets. ## Out of scope