fix(web): refresh walkthrough ranges on Monaco model changes (#1822)

* fix(web): refresh walkthrough ranges on Monaco model changes

* fix(web): avoid recentering walkthrough on line edits

* test(backend): isolate executor session snapshots

* test(backend): deep-copy executor session snapshots
This commit is contained in:
José Almeida
2026-07-20 17:34:15 +01:00
committed by GitHub
parent 801300645d
commit 3479fb601e
6 changed files with 401 additions and 29 deletions
+3 -2
View File
@@ -856,7 +856,7 @@ func walkthroughDemoArgs() map[string]interface{} {
wtStep("The change in A", "walkthrough_a.txt",
"Step 2: this is WALKTHROUGH_CHANGE_A the tour narrates.", 2, 3),
wtStep("File B", "walkthrough_b.txt",
"Step 3: WALKTHROUGH_CHANGE_B lives in file B.", 2, 0),
"Step 3: WALKTHROUGH_CHANGE_B lives in file B.", 4, 5),
wtStep("File C", "walkthrough_c.txt",
"Step 4: WALKTHROUGH_CHANGE_C lives in file C.", 2, 0),
wtStep("Unchanged file", "walkthrough_base.txt",
@@ -907,7 +907,8 @@ func setupWalkthroughFiles(e *emitter, prefix string) bool {
"line 1: ENTRY\nline 2: BASE\nline 3: BASE\n",
"line 1: ENTRY\nline 2: WALKTHROUGH_CHANGE_A\nline 3: WALKTHROUGH_CHANGE_A\n")
ok = ok && writeWalkthroughFile(e, runGitCmd, prefix, "walkthrough_b.txt",
"line 1: B\nline 2: BASE\n", "line 1: B\nline 2: WALKTHROUGH_CHANGE_B\n")
"line 1: B\nline 2: BASE\nline 3: BASE\nline 4: BASE\nline 5: BASE\n",
"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",
@@ -2,6 +2,7 @@ package executor
import (
"context"
"encoding/json"
"fmt"
"sync"
"testing"
@@ -371,7 +372,9 @@ func (m *mockRepository) GetTaskSession(ctx context.Context, id string) (*models
m.mu.Lock()
defer m.mu.Unlock()
if session, ok := m.sessions[id]; ok {
return session, nil
// SQLite scans each query into a fresh value. Match that ownership
// boundary so concurrent executor paths cannot share mutable rows.
return cloneMockTaskSession(session), nil
}
return nil, nil
}
@@ -403,6 +406,48 @@ func (m *mockRepository) UpdateTaskSessionIfCurrentState(
return true, nil
}
func cloneMockTaskSession(session *models.TaskSession) *models.TaskSession {
if session == nil {
return nil
}
clone := *session
clone.Metadata = cloneMockSessionMap(session.Metadata)
clone.AgentProfileSnapshot = cloneMockSessionMap(session.AgentProfileSnapshot)
clone.ExecutorSnapshot = cloneMockSessionMap(session.ExecutorSnapshot)
clone.EnvironmentSnapshot = cloneMockSessionMap(session.EnvironmentSnapshot)
clone.RepositorySnapshot = cloneMockSessionMap(session.RepositorySnapshot)
if session.Worktrees != nil {
clone.Worktrees = make([]*models.TaskSessionWorktree, len(session.Worktrees))
for i, worktree := range session.Worktrees {
if worktree == nil {
continue
}
worktreeClone := *worktree
clone.Worktrees[i] = &worktreeClone
}
}
if session.CompletedAt != nil {
completedAt := *session.CompletedAt
clone.CompletedAt = &completedAt
}
return &clone
}
func cloneMockSessionMap(values map[string]interface{}) map[string]interface{} {
if values == nil {
return nil
}
encoded, err := json.Marshal(values)
if err != nil {
panic(fmt.Sprintf("marshal mock task session map: %v", err))
}
var clone map[string]interface{}
if err := json.Unmarshal(encoded, &clone); err != nil {
panic(fmt.Sprintf("unmarshal mock task session map: %v", err))
}
return clone
}
func (m *mockRepository) UpdateTaskSessionAgentProfileSnapshot(
_ context.Context,
sessionID string,
@@ -65,6 +65,70 @@ func TestResumeTokenForExecutionProfile(t *testing.T) {
}
}
func TestMockRepositoryGetTaskSessionReturnsDetachedMutableFields(t *testing.T) {
completedAt := time.Now().UTC()
wantCompletedAt := completedAt
newSnapshot := func() map[string]interface{} {
return map[string]interface{}{
"top": "original",
"nested": map[string]interface{}{"value": "original"},
}
}
session := &models.TaskSession{
ID: "session-snapshot",
Metadata: newSnapshot(),
AgentProfileSnapshot: newSnapshot(),
ExecutorSnapshot: newSnapshot(),
EnvironmentSnapshot: newSnapshot(),
RepositorySnapshot: newSnapshot(),
Worktrees: []*models.TaskSessionWorktree{
{ID: "worktree-1", WorktreePath: "/original"},
nil,
},
CompletedAt: &completedAt,
}
repo := newMockRepository()
repo.sessions[session.ID] = session
snapshot, err := repo.GetTaskSession(context.Background(), session.ID)
if err != nil {
t.Fatalf("GetTaskSession failed: %v", err)
}
for _, values := range []map[string]interface{}{
snapshot.Metadata,
snapshot.AgentProfileSnapshot,
snapshot.ExecutorSnapshot,
snapshot.EnvironmentSnapshot,
snapshot.RepositorySnapshot,
} {
values["top"] = "changed"
values["nested"].(map[string]interface{})["value"] = "changed"
}
snapshot.Worktrees[0].WorktreePath = "/changed"
*snapshot.CompletedAt = wantCompletedAt.Add(time.Hour)
for name, values := range map[string]map[string]interface{}{
"metadata": session.Metadata,
"agent profile": session.AgentProfileSnapshot,
"executor": session.ExecutorSnapshot,
"environment": session.EnvironmentSnapshot,
"repository": session.RepositorySnapshot,
} {
if values["top"] != "original" {
t.Errorf("%s snapshot top-level value changed: %#v", name, values)
}
if values["nested"].(map[string]interface{})["value"] != "original" {
t.Errorf("%s snapshot nested value changed: %#v", name, values)
}
}
if session.Worktrees[0].WorktreePath != "/original" {
t.Errorf("stored worktree path = %q, want /original", session.Worktrees[0].WorktreePath)
}
if !session.CompletedAt.Equal(wantCompletedAt) {
t.Errorf("stored completed time = %s, want %s", session.CompletedAt, wantCompletedAt)
}
}
func TestPrepareSession_Success(t *testing.T) {
repo := newMockRepository()
agentManager := &mockAgentManager{}
@@ -335,8 +399,12 @@ func TestLaunchPreparedSession_Success(t *testing.T) {
if execution.SessionState != v1.TaskSessionStateStarting {
t.Errorf("Expected session state STARTING, got %s", execution.SessionState)
}
if session.TaskEnvironmentID != launchedEnvID {
t.Errorf("Expected session TaskEnvironmentID %q, got %q", launchedEnvID, session.TaskEnvironmentID)
storedSession, err := repo.GetTaskSession(context.Background(), session.ID)
if err != nil {
t.Fatalf("GetTaskSession failed: %v", err)
}
if storedSession.TaskEnvironmentID != launchedEnvID {
t.Errorf("Expected session TaskEnvironmentID %q, got %q", launchedEnvID, storedSession.TaskEnvironmentID)
}
if len(repo.createTaskEnvironmentCalls) != 1 {
t.Fatalf("Expected 1 CreateTaskEnvironment call, got %d", len(repo.createTaskEnvironmentCalls))
@@ -1,10 +1,99 @@
import { describe, expect, it } from "vitest";
import { act, renderHook } from "@testing-library/react";
import type { editor as monacoEditor } from "monaco-editor";
import { describe, expect, it, vi } from "vitest";
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",
},
],
},
},
},
}));
vi.mock("@/components/state-provider", () => ({
useAppStore: (selector: (state: typeof hookState) => unknown) => selector(hookState),
}));
vi.mock("@/lib/walkthrough-open-state", () => ({
useIsWalkthroughOpenForTask: () => true,
}));
const WALKTHROUGH_FILE = "walkthrough_a.txt";
import {
buildWalkthroughRangeDecorations,
clampWalkthroughRangeToLineCount,
getWalkthroughEditorRange,
useMonacoWalkthroughRange,
} from "./use-monaco-walkthrough-range";
function createModelSwitchingEditor(initialLineCount: number) {
let lineCount = initialLineCount;
let modelRevision = 0;
let model = { id: "model-0", getLineCount: () => lineCount };
const contentListeners = new Set<() => void>();
const modelListeners = new Set<() => void>();
const setDecorations = vi.fn();
const revealLinesInCenter = vi.fn();
const editor = {
createDecorationsCollection: () => ({ set: setDecorations }),
getModel: () => model,
onDidChangeModel: (listener: () => void) => {
modelListeners.add(listener);
return {
dispose: () => {
modelListeners.delete(listener);
},
};
},
onDidChangeModelContent: (listener: () => void) => {
contentListeners.add(listener);
return {
dispose: () => {
contentListeners.delete(listener);
},
};
},
revealLinesInCenter,
} as unknown as monacoEditor.IStandaloneCodeEditor;
return {
editor,
revealLinesInCenter,
setDecorations,
decoratedLines() {
const decorations = setDecorations.mock.lastCall?.[0] ?? [];
return decorations.map(
(decoration: monacoEditor.IModelDeltaDecoration) => decoration.range.startLineNumber,
);
},
listenerCounts() {
return { content: contentListeners.size, model: modelListeners.size };
},
changeLineCount(nextLineCount: number) {
lineCount = nextLineCount;
for (const listener of contentListeners) listener();
},
switchModel(nextLineCount: number) {
lineCount = nextLineCount;
modelRevision += 1;
model = { id: `model-${modelRevision}`, getLineCount: () => lineCount };
for (const listener of modelListeners) listener();
},
};
}
describe("getWalkthroughEditorRange", () => {
it("returns the active walkthrough range for a matching editor file", () => {
expect(
@@ -44,3 +133,88 @@ describe("clampWalkthroughRangeToLineCount", () => {
});
});
});
describe("useMonacoWalkthroughRange", () => {
it("reclamps the active range after Monaco switches models", () => {
const fake = createModelSwitchingEditor(2);
renderHook(() =>
useMonacoWalkthroughRange({
editor: fake.editor,
editorAreaRef: { current: null },
path: WALKTHROUGH_FILE,
}),
);
expect(fake.decoratedLines()).toEqual([2]);
act(() => fake.switchModel(3));
expect(fake.decoratedLines()).toEqual([2, 3]);
expect(fake.revealLinesInCenter).toHaveBeenLastCalledWith(2, 3);
});
it("reclamps when the current Monaco model line count changes", () => {
const fake = createModelSwitchingEditor(3);
renderHook(() =>
useMonacoWalkthroughRange({
editor: fake.editor,
editorAreaRef: { current: null },
path: WALKTHROUGH_FILE,
}),
);
act(() => fake.changeLineCount(2));
expect(fake.decoratedLines()).toEqual([2]);
expect(fake.revealLinesInCenter).toHaveBeenLastCalledWith(2, 2);
});
it("does not recenter for same-model line count changes outside the active range", () => {
const fake = createModelSwitchingEditor(3);
renderHook(() =>
useMonacoWalkthroughRange({
editor: fake.editor,
editorAreaRef: { current: null },
path: WALKTHROUGH_FILE,
}),
);
fake.revealLinesInCenter.mockClear();
act(() => fake.changeLineCount(4));
expect(fake.revealLinesInCenter).not.toHaveBeenCalled();
});
it("reapplies the range after switching to a model with the same line count", () => {
const fake = createModelSwitchingEditor(3);
renderHook(() =>
useMonacoWalkthroughRange({
editor: fake.editor,
editorAreaRef: { current: null },
path: WALKTHROUGH_FILE,
}),
);
fake.setDecorations.mockClear();
fake.revealLinesInCenter.mockClear();
act(() => fake.switchModel(3));
expect(fake.decoratedLines()).toEqual([2, 3]);
expect(fake.revealLinesInCenter).toHaveBeenLastCalledWith(2, 3);
});
it("unsubscribes from Monaco model events on unmount", () => {
const fake = createModelSwitchingEditor(3);
const { unmount } = renderHook(() =>
useMonacoWalkthroughRange({
editor: fake.editor,
editorAreaRef: { current: null },
path: WALKTHROUGH_FILE,
}),
);
expect(fake.listenerCounts()).toEqual({ content: 1, model: 1 });
unmount();
expect(fake.listenerCounts()).toEqual({ content: 0, model: 0 });
});
});
@@ -1,4 +1,12 @@
import { useEffect, useMemo, useRef, useState, type RefObject } from "react";
import {
useCallback,
useEffect,
useMemo,
useRef,
useState,
useSyncExternalStore,
type RefObject,
} from "react";
import type { editor as monacoEditor } from "monaco-editor";
import { useAppStore } from "@/components/state-provider";
import {
@@ -97,25 +105,40 @@ function useActiveWalkthroughStep() {
return { taskId, stepIndex, step: isOpen ? step : null };
}
export function useMonacoWalkthroughRange({
editor,
editorAreaRef,
path,
repo,
}: UseMonacoWalkthroughRangeOpts): WalkthroughRangeBox | null {
function useMonacoModelSnapshot(editor: monacoEditor.IStandaloneCodeEditor | null): string {
const subscribe = useCallback(
(onStoreChange: () => void) => {
if (!editor) return () => {};
const modelSubscription = editor.onDidChangeModel(onStoreChange);
const contentSubscription = editor.onDidChangeModelContent(onStoreChange);
return () => {
modelSubscription.dispose();
contentSubscription.dispose();
};
},
[editor],
);
const getSnapshot = useCallback(() => {
const model = editor?.getModel();
return model ? `${model.id}:${model.getLineCount()}` : "";
}, [editor]);
return useSyncExternalStore(subscribe, getSnapshot, getSnapshot);
}
function useMonacoWalkthroughDecorations(
editor: monacoEditor.IStandaloneCodeEditor | null,
range: WalkthroughEditorRange | null,
anchorKey: string,
modelSnapshot: string,
) {
const decorationsRef = useRef<monacoEditor.IEditorDecorationsCollection | null>(null);
const [box, setBox] = useState<WalkthroughRangeBox | null>(null);
const { taskId, stepIndex, step } = useActiveWalkthroughStep();
const range = useMemo(
() => getWalkthroughEditorRange({ path, repository_name: repo }, step),
[path, repo, step],
);
const clampedRange = useMemo(
() =>
range ? clampWalkthroughRangeToLineCount(range, editor?.getModel()?.getLineCount()) : null,
[editor, range],
);
const anchorKey = taskId ? `${taskId}:${stepIndex}:${repo ?? ""}:${path}` : "";
const revealedRef = useRef<{
editor: monacoEditor.IStandaloneCodeEditor;
model: monacoEditor.ITextModel | null;
anchorKey: string;
startLine: number;
endLine: number;
} | null>(null);
useEffect(() => {
if (!editor) return;
@@ -127,11 +150,48 @@ export function useMonacoWalkthroughRange({
}, [editor]);
useEffect(() => {
decorationsRef.current?.set(buildWalkthroughRangeDecorations(clampedRange));
if (editor && clampedRange) {
editor.revealLinesInCenter(clampedRange.startLine, clampedRange.endLine);
decorationsRef.current?.set(buildWalkthroughRangeDecorations(range));
if (!editor || !range) {
revealedRef.current = null;
return;
}
}, [clampedRange, editor]);
const next = { editor, model: editor.getModel(), anchorKey, ...range };
const previous = revealedRef.current;
if (
previous?.editor === next.editor &&
previous.model === next.model &&
previous.anchorKey === next.anchorKey &&
previous.startLine === next.startLine &&
previous.endLine === next.endLine
) {
return;
}
revealedRef.current = next;
editor.revealLinesInCenter(range.startLine, range.endLine);
}, [anchorKey, editor, modelSnapshot, range]);
}
export function useMonacoWalkthroughRange({
editor,
editorAreaRef,
path,
repo,
}: UseMonacoWalkthroughRangeOpts): WalkthroughRangeBox | null {
const [box, setBox] = useState<WalkthroughRangeBox | null>(null);
const { taskId, stepIndex, step } = useActiveWalkthroughStep();
const modelSnapshot = useMonacoModelSnapshot(editor);
const range = useMemo(
() => getWalkthroughEditorRange({ path, repository_name: repo }, step),
[path, repo, step],
);
const clampedRange = useMemo(
() =>
range ? clampWalkthroughRangeToLineCount(range, editor?.getModel()?.getLineCount()) : null,
[editor, modelSnapshot, range],
);
const anchorKey = taskId ? `${taskId}:${stepIndex}:${repo ?? ""}:${path}` : "";
useMonacoWalkthroughDecorations(editor, clampedRange, anchorKey, modelSnapshot);
useEffect(() => {
const area = editorAreaRef.current;
@@ -228,6 +228,30 @@ test.describe("Code walkthrough", () => {
{ timeout: 15_000 },
);
// Move from shorter file A to longer file B. The shared Monaco preview
// switches models after the walkthrough step changes; B's 4-5 range must
// be clamped against B, not A's previous model.
await card.getByTestId("walkthrough-next").click();
await expect(card.getByTestId("walkthrough-step-header")).toContainText("Step 3 / 5");
await expect(testPage.locator(".monaco-editor:visible .view-lines")).toContainText(
"WALKTHROUGH_CHANGE_B",
);
await expect(testPage.getByTestId("walkthrough-editor-range")).toHaveAttribute(
"data-line-range",
"4-5",
);
// Returning to A must also restore its original multi-line range.
await card.getByTestId("walkthrough-prev").click();
await expect(card.getByTestId("walkthrough-step-header")).toContainText("Step 2 / 5");
await expect(testPage.locator(".monaco-editor:visible .view-lines")).toContainText(
"WALKTHROUGH_CHANGE_A",
);
await expect(testPage.getByTestId("walkthrough-editor-range")).toHaveAttribute(
"data-line-range",
"2-3",
);
const before = await card.boundingBox();
if (!before) throw new Error("walkthrough card missing before drag");
const dragHandle = card.getByTestId("walkthrough-drag-handle");