feat(walkthrough): stabilize navigation footer (#2002)
This commit is contained in:
@@ -898,7 +898,7 @@ func setupWalkthroughFiles(e *emitter, prefix string) bool {
|
||||
}
|
||||
runGitCmd := makeGitRunner(wd)
|
||||
|
||||
fixtures := []string{"walkthrough_a.txt", "walkthrough_b.txt", "walkthrough_c.txt", "walkthrough_base.txt"}
|
||||
fixtures := []string{"walkthrough_a.txt", "walkthrough_b.txt", "walkthrough_c.txt"}
|
||||
_ = runGitCmd(append([]string{"rm", "--force"}, fixtures...)...)
|
||||
_ = runGitCmd("commit", "-m", "cleanup walkthrough fixtures")
|
||||
|
||||
@@ -911,11 +911,24 @@ func setupWalkthroughFiles(e *emitter, prefix string) bool {
|
||||
"line 1: B\nline 2: BASE\nline 3: BASE\nline 4: WALKTHROUGH_CHANGE_B\nline 5: WALKTHROUGH_CHANGE_B\n")
|
||||
ok = ok && writeWalkthroughFile(e, runGitCmd, prefix, "walkthrough_c.txt",
|
||||
"line 1: C\nline 2: BASE\n", "line 1: C\nline 2: WALKTHROUGH_CHANGE_C\n")
|
||||
ok = ok && writeWalkthroughFile(e, runGitCmd, prefix, "walkthrough_base.txt",
|
||||
"line 1: WALKTHROUGH_UNCHANGED\nline 2: supporting context\n", "")
|
||||
const baseContent = "line 1: WALKTHROUGH_UNCHANGED\nline 2: seeded on main\n"
|
||||
if !walkthroughBaseIsClean(runGitCmd, baseContent) {
|
||||
_ = runGitCmd("rm", "--force", "walkthrough_base.txt")
|
||||
_ = runGitCmd("commit", "-m", "cleanup walkthrough base fixture")
|
||||
ok = ok && writeWalkthroughFile(e, runGitCmd, prefix, "walkthrough_base.txt", baseContent, "")
|
||||
}
|
||||
return ok
|
||||
}
|
||||
|
||||
func walkthroughBaseIsClean(runGitCmd func(args ...string) error, expectedContent string) bool {
|
||||
content, err := os.ReadFile("walkthrough_base.txt")
|
||||
return err == nil &&
|
||||
string(content) == expectedContent &&
|
||||
runGitCmd("ls-files", "--error-unmatch", "walkthrough_base.txt") == nil &&
|
||||
runGitCmd("diff", "--quiet", "--", "walkthrough_base.txt") == nil &&
|
||||
runGitCmd("diff", "--cached", "--quiet", "--", "walkthrough_base.txt") == nil
|
||||
}
|
||||
|
||||
func emitWalkthroughTour(e *emitter, doneText string) {
|
||||
fixedDelay(100)
|
||||
e.text("Let me walk you through the change.")
|
||||
|
||||
@@ -236,13 +236,13 @@ export function WalkthroughFloatingWindow({
|
||||
data-testid="walkthrough-floating"
|
||||
data-mobile-variant="bottom-sheet"
|
||||
className={cn(
|
||||
"fixed inset-x-2 bottom-[calc(0.5rem+var(--app-status-bar-height))] z-[43] max-h-[calc(100dvh-1rem)] overflow-y-auto rounded-xl",
|
||||
"fixed inset-x-2 bottom-[calc(0.5rem+var(--app-status-bar-height)+env(safe-area-inset-bottom,0px))] z-[43] max-h-[calc(100dvh-1rem-env(safe-area-inset-bottom,0px))] overflow-hidden rounded-xl",
|
||||
"sm:inset-x-auto sm:bottom-auto sm:w-[400px] sm:max-w-[calc(100vw-2rem)]",
|
||||
)}
|
||||
style={isDesktop && position ? { left: position.x, top: position.y } : undefined}
|
||||
onPointerDown={onPointerDown}
|
||||
>
|
||||
<WalkthroughStepInner onClose={onClose} onSelectFile={openFile} />
|
||||
<WalkthroughStepInner onClose={onClose} onSelectFile={openFile} layout="floating" />
|
||||
</div>
|
||||
</>
|
||||
);
|
||||
|
||||
@@ -20,6 +20,7 @@ import {
|
||||
import type { TaskWalkthrough, WalkthroughStep } from "@/lib/types/http";
|
||||
import type { WalkthroughComment } from "@/lib/state/slices/comments";
|
||||
import { CommentForm } from "./comment-form";
|
||||
import { cn } from "@kandev/ui/lib/utils";
|
||||
|
||||
export const WALKTHROUGH_STEP_BODY_CLASS =
|
||||
"prose prose-sm dark:prose-invert max-w-none text-left text-sm leading-6 [overflow-wrap:anywhere] [text-align:left] [&_li]:text-left [&_p]:my-2 [&_p]:text-left";
|
||||
@@ -103,7 +104,10 @@ function StepNavigation({
|
||||
onNext: () => void;
|
||||
}) {
|
||||
return (
|
||||
<div className="flex items-center gap-2 px-4 py-2 border-t border-border">
|
||||
<div
|
||||
className="flex shrink-0 items-center gap-2 border-t border-border px-4 py-2"
|
||||
data-testid="walkthrough-navigation"
|
||||
>
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
@@ -205,9 +209,11 @@ function useWalkthroughStepFeedback(params: {
|
||||
export function WalkthroughStepInner({
|
||||
onClose,
|
||||
onSelectFile,
|
||||
layout = "inline",
|
||||
}: {
|
||||
onClose: () => void;
|
||||
onSelectFile?: OpenFileFn;
|
||||
layout?: "inline" | "floating";
|
||||
}) {
|
||||
const activeTaskId = useAppStore((s) => s.tasks.activeTaskId);
|
||||
const sessionId = useAppStore((s) => s.tasks.activeSessionId);
|
||||
@@ -236,10 +242,25 @@ export function WalkthroughStepInner({
|
||||
}, [activeTaskId, walkthrough, markSeen]);
|
||||
if (!activeTaskId || !walkthrough) return null;
|
||||
if (!step) return null;
|
||||
const isFloating = layout === "floating";
|
||||
const lineLabel = step.line_end ? `Lines ${step.line}–${step.line_end}` : `Line ${step.line}`;
|
||||
const navigation = (
|
||||
<StepNavigation
|
||||
activeStep={activeStep}
|
||||
stepCount={stepCount}
|
||||
onPrevious={() => setActiveStep(activeTaskId, activeStep - 1)}
|
||||
onNext={() => setActiveStep(activeTaskId, activeStep + 1)}
|
||||
/>
|
||||
);
|
||||
|
||||
return (
|
||||
<div className="flex max-h-[calc(100dvh-2rem)] flex-col rounded-xl border-l-2 border-primary/60 border border-border bg-card shadow-lg sm:max-h-[min(78vh,720px)]">
|
||||
<div
|
||||
className={cn(
|
||||
"flex max-h-[calc(100dvh-2rem)] flex-col rounded-xl border border-border border-l-2 border-primary/60 bg-card shadow-lg sm:max-h-[min(78vh,720px)]",
|
||||
isFloating &&
|
||||
"h-[calc(100dvh-1rem-var(--app-status-bar-height)-env(safe-area-inset-bottom,0px))] overflow-hidden sm:h-[min(78vh,720px)]",
|
||||
)}
|
||||
>
|
||||
<StepHeader
|
||||
activeStep={activeStep}
|
||||
stepCount={stepCount}
|
||||
@@ -250,13 +271,8 @@ export function WalkthroughStepInner({
|
||||
step={step}
|
||||
onOpenFile={() => revealFileAtLine(openFile, step.file, step.line, step.repo)}
|
||||
/>
|
||||
<StepNavigation
|
||||
activeStep={activeStep}
|
||||
stepCount={stepCount}
|
||||
onPrevious={() => setActiveStep(activeTaskId, activeStep - 1)}
|
||||
onNext={() => setActiveStep(activeTaskId, activeStep + 1)}
|
||||
/>
|
||||
<div className="px-4 pb-3 pt-1">
|
||||
{!isFloating ? navigation : null}
|
||||
<div className="shrink-0 px-4 pb-3 pt-1">
|
||||
<CommentForm
|
||||
key={`${walkthrough.id}:${activeStep}`}
|
||||
onSubmit={addWalkthroughFeedback}
|
||||
@@ -265,6 +281,7 @@ export function WalkthroughStepInner({
|
||||
autoFocus={false}
|
||||
/>
|
||||
</div>
|
||||
{isFloating ? navigation : null}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,118 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { renderHook } from "@testing-library/react";
|
||||
import type { EditorView } from "@codemirror/view";
|
||||
|
||||
const hookState = vi.hoisted(() => ({
|
||||
tasks: { activeTaskId: "task-1" },
|
||||
walkthroughs: {
|
||||
activeStepByTaskId: { "task-1": 0 },
|
||||
byTaskId: {
|
||||
"task-1": {
|
||||
steps: [
|
||||
{
|
||||
file: "walkthrough_a.txt",
|
||||
line: 2,
|
||||
line_end: 3,
|
||||
text: "Explain this range",
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
},
|
||||
}));
|
||||
|
||||
const anchorMocks = vi.hoisted(() => ({
|
||||
clear: vi.fn(),
|
||||
isVisible: vi.fn(() => true),
|
||||
set: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@/components/state-provider", () => ({
|
||||
useAppStore: (selector: (state: typeof hookState) => unknown) => selector(hookState),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/walkthrough-open-state", () => ({
|
||||
useIsWalkthroughOpenForTask: () => true,
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/walkthrough-editor-anchor", () => ({
|
||||
clearWalkthroughEditorAnchor: anchorMocks.clear,
|
||||
isWalkthroughAnchorTargetVisible: anchorMocks.isVisible,
|
||||
setWalkthroughEditorAnchor: anchorMocks.set,
|
||||
}));
|
||||
|
||||
vi.mock("@codemirror/view", () => ({
|
||||
EditorView: { scrollIntoView: vi.fn() },
|
||||
}));
|
||||
|
||||
import { useCodeMirrorWalkthroughRange } from "./use-codemirror-walkthrough-range";
|
||||
|
||||
function rect(left: number, top: number, width: number, height: number): DOMRect {
|
||||
return { left, top, width, height, right: left + width, bottom: top + height } as DOMRect;
|
||||
}
|
||||
|
||||
function createView(): EditorView {
|
||||
const dom = document.createElement("div");
|
||||
document.body.append(dom);
|
||||
vi.spyOn(dom, "getBoundingClientRect").mockReturnValue(rect(20, 30, 500, 300));
|
||||
return {
|
||||
coordsAtPos: (position: number) =>
|
||||
position < 30 ? { left: 24, top: 70, bottom: 90 } : { left: 24, top: 90, bottom: 110 },
|
||||
dispatch: vi.fn(),
|
||||
dom,
|
||||
scrollDOM: dom,
|
||||
state: {
|
||||
doc: {
|
||||
line: (line: number) => ({ from: line * 10, to: line * 10 + 5 }),
|
||||
lines: 3,
|
||||
},
|
||||
},
|
||||
} as unknown as EditorView;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
anchorMocks.clear.mockClear();
|
||||
anchorMocks.isVisible.mockClear();
|
||||
anchorMocks.isVisible.mockReturnValue(true);
|
||||
anchorMocks.set.mockClear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals();
|
||||
document.body.replaceChildren();
|
||||
});
|
||||
|
||||
describe("useCodeMirrorWalkthroughRange", () => {
|
||||
it("preserves the range box but clears the anchor when its target is occluded", () => {
|
||||
const view = createView();
|
||||
const area = document.createElement("div");
|
||||
document.body.append(area);
|
||||
const editorAreaRef = { current: area };
|
||||
vi.spyOn(area, "getBoundingClientRect").mockReturnValue(rect(0, 0, 600, 400));
|
||||
vi.stubGlobal("requestAnimationFrame", (callback: FrameRequestCallback) => {
|
||||
callback(0);
|
||||
return 1;
|
||||
});
|
||||
vi.stubGlobal("cancelAnimationFrame", vi.fn());
|
||||
vi.stubGlobal(
|
||||
"ResizeObserver",
|
||||
class {
|
||||
disconnect() {}
|
||||
observe() {}
|
||||
},
|
||||
);
|
||||
anchorMocks.isVisible.mockReturnValue(false);
|
||||
|
||||
const { result } = renderHook(() =>
|
||||
useCodeMirrorWalkthroughRange({
|
||||
view,
|
||||
editorAreaRef,
|
||||
path: "walkthrough_a.txt",
|
||||
}),
|
||||
);
|
||||
|
||||
expect(result.current).toMatchObject({ startLine: 2, endLine: 3 });
|
||||
expect(anchorMocks.clear).toHaveBeenCalledWith("task-1:0::walkthrough_a.txt:cm");
|
||||
expect(anchorMocks.set).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -112,12 +112,11 @@ export function useCodeMirrorWalkthroughRange({
|
||||
clearWalkthroughEditorAnchor(anchorKey);
|
||||
return;
|
||||
}
|
||||
setBox(measured.box);
|
||||
if (!isWalkthroughAnchorTargetVisible(view.dom, measured.viewportRect)) {
|
||||
setBox(null);
|
||||
clearWalkthroughEditorAnchor(anchorKey);
|
||||
return;
|
||||
}
|
||||
setBox(measured.box);
|
||||
setWalkthroughEditorAnchor({
|
||||
key: anchorKey,
|
||||
taskId,
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { act, renderHook } from "@testing-library/react";
|
||||
import type { editor as monacoEditor } from "monaco-editor";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
const hookState = vi.hoisted(() => ({
|
||||
tasks: { activeTaskId: "task-1" },
|
||||
@@ -21,6 +21,12 @@ const hookState = vi.hoisted(() => ({
|
||||
},
|
||||
}));
|
||||
|
||||
const anchorMocks = vi.hoisted(() => ({
|
||||
clear: vi.fn(),
|
||||
isVisible: vi.fn(() => true),
|
||||
set: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@/components/state-provider", () => ({
|
||||
useAppStore: (selector: (state: typeof hookState) => unknown) => selector(hookState),
|
||||
}));
|
||||
@@ -29,6 +35,12 @@ vi.mock("@/lib/walkthrough-open-state", () => ({
|
||||
useIsWalkthroughOpenForTask: () => true,
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/walkthrough-editor-anchor", () => ({
|
||||
clearWalkthroughEditorAnchor: anchorMocks.clear,
|
||||
isWalkthroughAnchorTargetVisible: anchorMocks.isVisible,
|
||||
setWalkthroughEditorAnchor: anchorMocks.set,
|
||||
}));
|
||||
|
||||
const WALKTHROUGH_FILE = "walkthrough_a.txt";
|
||||
|
||||
import {
|
||||
@@ -94,6 +106,45 @@ function createModelSwitchingEditor(initialLineCount: number) {
|
||||
};
|
||||
}
|
||||
|
||||
function rect(left: number, top: number, width: number, height: number): DOMRect {
|
||||
return { left, top, width, height, right: left + width, bottom: top + height } as DOMRect;
|
||||
}
|
||||
|
||||
function createGeometryEditor() {
|
||||
const dom = document.createElement("div");
|
||||
document.body.append(dom);
|
||||
vi.spyOn(dom, "getBoundingClientRect").mockReturnValue(rect(20, 30, 500, 300));
|
||||
const noopSubscription = { dispose: vi.fn() };
|
||||
const editor = {
|
||||
createDecorationsCollection: () => ({ set: vi.fn() }),
|
||||
getDomNode: () => dom,
|
||||
getModel: () => ({ id: "model-0", getLineCount: () => 3 }),
|
||||
getScrolledVisiblePosition: ({ lineNumber }: { lineNumber: number }) => ({
|
||||
top: lineNumber * 20,
|
||||
left: 24,
|
||||
height: 20,
|
||||
}),
|
||||
onDidChangeModel: () => noopSubscription,
|
||||
onDidChangeModelContent: () => noopSubscription,
|
||||
onDidLayoutChange: () => noopSubscription,
|
||||
onDidScrollChange: () => noopSubscription,
|
||||
revealLinesInCenter: vi.fn(),
|
||||
} as unknown as monacoEditor.IStandaloneCodeEditor;
|
||||
return { dom, editor };
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
anchorMocks.clear.mockClear();
|
||||
anchorMocks.isVisible.mockClear();
|
||||
anchorMocks.isVisible.mockReturnValue(true);
|
||||
anchorMocks.set.mockClear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals();
|
||||
document.body.replaceChildren();
|
||||
});
|
||||
|
||||
describe("getWalkthroughEditorRange", () => {
|
||||
it("returns the active walkthrough range for a matching editor file", () => {
|
||||
expect(
|
||||
@@ -135,6 +186,42 @@ describe("clampWalkthroughRangeToLineCount", () => {
|
||||
});
|
||||
|
||||
describe("useMonacoWalkthroughRange", () => {
|
||||
it("preserves the range box but clears the anchor when its target is occluded", () => {
|
||||
const { dom, editor } = createGeometryEditor();
|
||||
const area = document.createElement("div");
|
||||
document.body.append(area);
|
||||
const editorAreaRef = { current: area };
|
||||
vi.spyOn(area, "getBoundingClientRect").mockReturnValue(rect(0, 0, 600, 400));
|
||||
vi.stubGlobal("requestAnimationFrame", (callback: FrameRequestCallback) => {
|
||||
callback(0);
|
||||
return 1;
|
||||
});
|
||||
vi.stubGlobal("cancelAnimationFrame", vi.fn());
|
||||
vi.stubGlobal(
|
||||
"ResizeObserver",
|
||||
class {
|
||||
disconnect() {}
|
||||
observe() {}
|
||||
},
|
||||
);
|
||||
anchorMocks.isVisible.mockReturnValue(false);
|
||||
|
||||
const { result } = renderHook(() =>
|
||||
useMonacoWalkthroughRange({
|
||||
editor,
|
||||
editorAreaRef,
|
||||
path: WALKTHROUGH_FILE,
|
||||
}),
|
||||
);
|
||||
|
||||
expect(result.current).toMatchObject({ startLine: 2, endLine: 3 });
|
||||
expect(anchorMocks.clear).toHaveBeenCalledWith("task-1:0::walkthrough_a.txt");
|
||||
expect(anchorMocks.set).not.toHaveBeenCalled();
|
||||
dom.remove();
|
||||
});
|
||||
});
|
||||
|
||||
describe("useMonacoWalkthroughRange model changes", () => {
|
||||
it("reclamps the active range after Monaco switches models", () => {
|
||||
const fake = createModelSwitchingEditor(2);
|
||||
renderHook(() =>
|
||||
|
||||
@@ -212,12 +212,11 @@ export function useMonacoWalkthroughRange({
|
||||
return;
|
||||
}
|
||||
const editorDom = editor.getDomNode();
|
||||
setBox(measured.box);
|
||||
if (!isWalkthroughAnchorTargetVisible(editorDom, measured.viewportRect)) {
|
||||
setBox(null);
|
||||
clearWalkthroughEditorAnchor(anchorKey);
|
||||
return;
|
||||
}
|
||||
setBox(measured.box);
|
||||
setWalkthroughEditorAnchor({
|
||||
key: anchorKey,
|
||||
taskId,
|
||||
|
||||
@@ -103,6 +103,18 @@ test.describe("Mobile code walkthrough", () => {
|
||||
await expect(card.getByTestId("walkthrough-step-title")).toContainText("Overview");
|
||||
await expect(card.getByTestId("walkthrough-step-body")).toContainText("ELI5:");
|
||||
await expect(session.walkthroughEditorRange()).toBeVisible({ timeout: 15_000 });
|
||||
|
||||
const initialNavigation = await card.getByTestId("walkthrough-navigation").boundingBox();
|
||||
if (!initialNavigation) throw new Error("walkthrough navigation missing before step change");
|
||||
await card.getByTestId("walkthrough-next").tap();
|
||||
await expect(card.getByTestId("walkthrough-step-header")).toContainText("Step 2 / 5");
|
||||
const nextNavigation = await card.getByTestId("walkthrough-navigation").boundingBox();
|
||||
if (!nextNavigation) throw new Error("walkthrough navigation missing after step change");
|
||||
expect(Math.abs(nextNavigation.y - initialNavigation.y)).toBeLessThan(1);
|
||||
const viewport = testPage.viewportSize();
|
||||
if (!viewport) throw new Error("walkthrough viewport unavailable");
|
||||
expect(nextNavigation.y).toBeGreaterThanOrEqual(0);
|
||||
expect(nextNavigation.y + nextNavigation.height).toBeLessThanOrEqual(viewport.height);
|
||||
});
|
||||
|
||||
test("can discard a walkthrough from the touch launcher", async ({
|
||||
|
||||
@@ -180,12 +180,22 @@ test.describe("Code walkthrough", () => {
|
||||
await expect(card.getByTestId("walkthrough-step-file")).toContainText("walkthrough_a.txt");
|
||||
await expect(card.getByTestId("walkthrough-prev")).toBeDisabled();
|
||||
|
||||
const initialNavigation = await card.getByTestId("walkthrough-navigation").boundingBox();
|
||||
if (!initialNavigation) throw new Error("walkthrough navigation missing before step change");
|
||||
|
||||
const expectStep = async (n: number, text: string) => {
|
||||
await card.getByTestId("walkthrough-next").click();
|
||||
await expect(header).toContainText(`Step ${n} / 5`);
|
||||
await expect(body).toContainText(text);
|
||||
};
|
||||
await expectStep(2, "WALKTHROUGH_CHANGE_A");
|
||||
const nextNavigation = await card.getByTestId("walkthrough-navigation").boundingBox();
|
||||
if (!nextNavigation) throw new Error("walkthrough navigation missing after step change");
|
||||
expect(Math.abs(nextNavigation.y - initialNavigation.y)).toBeLessThan(1);
|
||||
const viewport = testPage.viewportSize();
|
||||
if (viewport) {
|
||||
expect(nextNavigation.y + nextNavigation.height).toBeLessThanOrEqual(viewport.height);
|
||||
}
|
||||
await expectStep(3, "WALKTHROUGH_CHANGE_B");
|
||||
await expectStep(4, "WALKTHROUGH_CHANGE_C");
|
||||
await expectStep(5, "WALKTHROUGH_UNCHANGED");
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
---
|
||||
spec: docs/specs/walkthrough-navigation-layout/spec.md
|
||||
created: 2026-07-28
|
||||
status: done
|
||||
---
|
||||
|
||||
# Fix Plan: Stable Walkthrough Navigation
|
||||
|
||||
## Root cause
|
||||
|
||||
`WalkthroughStepInner` only constrains its maximum height. Its explanation,
|
||||
navigation, and comment form therefore size the card together; the body's
|
||||
`flex-1` has no definite height to fill. A longer or shorter step consequently
|
||||
moves the navigation row.
|
||||
|
||||
## Overview
|
||||
|
||||
Give the walkthrough surface a viewport-bounded, definite height and make the
|
||||
step explanation its single internal scroll owner. Render the navigation as the
|
||||
final fixed card footer so **Next** has a stable location while the current
|
||||
step changes. Prove the geometry in existing desktop and mobile walkthrough
|
||||
Playwright coverage.
|
||||
|
||||
## Frontend
|
||||
|
||||
### Walkthrough card composition
|
||||
|
||||
- `apps/web/components/diff/walkthrough-step-card.tsx` — make the shared inner
|
||||
card a definite-height flex column; preserve a scrollable step body and move
|
||||
the navigation row into the fixed footer after the feedback form.
|
||||
- `apps/web/components/diff/walkthrough-floating-window.tsx` — give the
|
||||
desktop floating card and phone bottom sheet the available bounded height,
|
||||
while retaining the desktop drag position, mobile bottom alignment, and
|
||||
app-status-bar clearance.
|
||||
|
||||
### Mobile design contract
|
||||
|
||||
- **Outcome / entry point:** advance an open walkthrough from its existing
|
||||
bottom-sheet launcher.
|
||||
- **Exemplar:** the existing `WalkthroughFloatingWindow` bottom-sheet
|
||||
presentation supplies the surface and dismissal behavior; this change keeps
|
||||
its primary action visible rather than introducing a new phone pattern.
|
||||
- **Hierarchy / primary action:** explanation scrolls above; **Next** remains
|
||||
the final, visible primary action.
|
||||
- **Surface / rationale:** a bottom sheet remains appropriate because the
|
||||
walkthrough is a temporary companion to the open code editor, not a primary
|
||||
route. One bounded sheet and its interior own the vertical scroll.
|
||||
- **Geometry:** use dynamic viewport sizing, preserve safe-area/status-bar
|
||||
clearance, keep controls at least their existing touch target size, and keep
|
||||
document horizontal overflow at zero.
|
||||
|
||||
## Tests
|
||||
|
||||
- No new non-trivial pure helper is introduced; the regression is rendered
|
||||
geometry and is covered at the browser level.
|
||||
|
||||
## E2E Tests
|
||||
|
||||
- **Scenario:** desktop steps with different content lengths. **File:**
|
||||
`apps/web/e2e/tests/review/walkthrough.spec.ts`. **Proof:** compare the
|
||||
navigation-footer bounding box before and after advancing, then assert the
|
||||
next step renders.
|
||||
- **Scenario:** mobile bottom-sheet walkthrough. **File:**
|
||||
`apps/web/e2e/tests/review/mobile-walkthrough.spec.ts`. **Proof:** advance
|
||||
through steps and assert the navigation footer remains visible, contained by
|
||||
the viewport, and in the same card position.
|
||||
|
||||
## Implementation Wave
|
||||
|
||||
1. [Stable navigation footer](task-01-stable-navigation-footer.md) — done, sequential.
|
||||
|
||||
## Risks
|
||||
|
||||
- A fixed height can hide long text if the body is not the sole scroll owner.
|
||||
- Phone viewport and app-status-bar offsets can put a bottom action behind
|
||||
browser chrome if dynamic height and existing bottom clearance are not kept.
|
||||
- Hiding an occluded anchor must not also hide the editor range indicator;
|
||||
preserve the range while clearing only the connector anchor.
|
||||
@@ -0,0 +1,70 @@
|
||||
---
|
||||
id: "walkthrough-navigation-layout-01"
|
||||
title: "Stable navigation footer"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/walkthrough-navigation-layout/spec.md"
|
||||
---
|
||||
|
||||
# Task 01: Stable navigation footer
|
||||
|
||||
## Acceptance
|
||||
|
||||
1. The open walkthrough gives its explanation an internal scroll region and
|
||||
keeps its Previous/Next footer in a stable final-card position while steps
|
||||
change.
|
||||
2. The desktop floating window remains draggable; the phone bottom sheet keeps
|
||||
its navigation visible, reachable, and inside the dynamic viewport.
|
||||
3. Desktop and `mobile-chrome` E2E coverage prove the navigation geometry with
|
||||
step changes and retain the existing walkthrough progression behavior.
|
||||
|
||||
## Verification
|
||||
|
||||
```sh
|
||||
(
|
||||
set -e
|
||||
cd apps/web
|
||||
pnpm e2e:run tests/review/walkthrough.spec.ts
|
||||
pnpm e2e:run tests/review/mobile-walkthrough.spec.ts -- --project=mobile-chrome
|
||||
)
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/components/diff/walkthrough-step-card.tsx`
|
||||
- `apps/web/components/diff/walkthrough-floating-window.tsx`
|
||||
- `apps/web/e2e/tests/review/walkthrough.spec.ts`
|
||||
- `apps/web/e2e/tests/review/mobile-walkthrough.spec.ts`
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Parallelism
|
||||
|
||||
Sequential; the regression tests and shared walkthrough components change
|
||||
together.
|
||||
|
||||
## Inputs
|
||||
|
||||
- [Stable Walkthrough Navigation spec](../../specs/walkthrough-navigation-layout/spec.md)
|
||||
- [Fix plan](plan.md)
|
||||
- Existing `WalkthroughFloatingWindow` mobile bottom-sheet and desktop drag
|
||||
behavior.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report the failing regression-test evidence, implementation files, focused
|
||||
desktop/mobile E2E results, visual verification, risks, and updated task/plan
|
||||
status.
|
||||
|
||||
## Completion record
|
||||
|
||||
- Desktop and `mobile-chrome` walkthrough E2E passed with stable navigation
|
||||
geometry assertions.
|
||||
- Fresh synthetic desktop and mobile captures confirmed the footer is visible,
|
||||
reachable, and does not overlap the viewport edge.
|
||||
- The remaining risk is browser-specific safe-area behavior; the floating
|
||||
surface reserves `safe-area-inset-bottom` for the navigation footer.
|
||||
@@ -131,6 +131,7 @@ Subscription quota tracking and per-agent cheap-model profile routing.
|
||||
| [mobile-task-navigation](ui/mobile-task-navigation.md) | shipped |
|
||||
| [task-layout-profiles](ui/task-layout-profiles.md) | draft |
|
||||
| [task-surface-refresh](ui/task-surface-refresh.md) | draft |
|
||||
| [walkthrough-navigation-layout](walkthrough-navigation-layout/spec.md) | shipped |
|
||||
| [agent-message-comments](ui/agent-message-comments.md) | shipped |
|
||||
| [external-vcs-file-links](ui/external-vcs-file-links.md) | shipped |
|
||||
| [task-listing-display-preferences](ui/task-listing-display-preferences.md) | shipped |
|
||||
|
||||
@@ -0,0 +1,48 @@
|
||||
---
|
||||
status: shipped
|
||||
created: 2026-07-28
|
||||
owner: product
|
||||
---
|
||||
|
||||
# Stable Walkthrough Navigation
|
||||
|
||||
## Why
|
||||
|
||||
People progress through a code walkthrough one step at a time. When one step
|
||||
has more explanation than another, the primary **Next** control moves, forcing
|
||||
them to find it again instead of keeping a reliable interaction target.
|
||||
|
||||
## What
|
||||
|
||||
- The walkthrough card keeps **Previous** and **Next** in a fixed footer
|
||||
position while it remains open.
|
||||
- Step titles, file metadata, and explanation text may vary in length without
|
||||
moving that footer between steps.
|
||||
- The explanation region scrolls within the card when it needs more space; the
|
||||
footer and feedback controls remain available.
|
||||
- Desktop preserves the draggable floating-card experience. On phones, the
|
||||
existing bottom-sheet walkthrough keeps the same capability with a
|
||||
thumb-reachable navigation footer, safe-area clearance, and no page-level
|
||||
horizontal overflow.
|
||||
|
||||
## Scenarios
|
||||
|
||||
- **GIVEN** an open desktop walkthrough whose current and following steps have
|
||||
different explanation lengths, **WHEN** the user selects **Next**, **THEN**
|
||||
the navigation footer retains its vertical position within the card and the
|
||||
next step content is shown.
|
||||
- **GIVEN** a walkthrough explanation taller than its available content area,
|
||||
**WHEN** the user scrolls it, **THEN** the explanation scrolls without
|
||||
obscuring or moving **Previous** and **Next**.
|
||||
- **GIVEN** a phone viewport with an open walkthrough bottom sheet, **WHEN**
|
||||
the user advances through differently sized steps, **THEN** the navigation
|
||||
footer remains visible, reachable, and within the viewport.
|
||||
- **GIVEN** a walkthrough sheet covers its editor anchor, **WHEN** the active
|
||||
step changes, **THEN** the editor range indicator remains rendered while its
|
||||
occluded connector is suppressed.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Changing walkthrough step content, ordering, or persistence.
|
||||
- Adding keyboard shortcuts, swipe navigation, or a progress scrubber.
|
||||
- Changing the floating-card drag behavior or its anchoring connector.
|
||||
Reference in New Issue
Block a user