diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index 86801a5828..79a9e9aef1 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -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", diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index 86fae1c3ba..d7ae92c811 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -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, diff --git a/apps/backend/internal/orchestrator/executor/executor_test.go b/apps/backend/internal/orchestrator/executor/executor_test.go index a638d71d12..5258005c90 100644 --- a/apps/backend/internal/orchestrator/executor/executor_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_test.go @@ -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)) diff --git a/apps/web/components/editors/monaco/use-monaco-walkthrough-range.test.ts b/apps/web/components/editors/monaco/use-monaco-walkthrough-range.test.ts index c67333aabc..5ff22c138d 100644 --- a/apps/web/components/editors/monaco/use-monaco-walkthrough-range.test.ts +++ b/apps/web/components/editors/monaco/use-monaco-walkthrough-range.test.ts @@ -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 }); + }); +}); diff --git a/apps/web/components/editors/monaco/use-monaco-walkthrough-range.ts b/apps/web/components/editors/monaco/use-monaco-walkthrough-range.ts index c69ead5632..cf15e682eb 100644 --- a/apps/web/components/editors/monaco/use-monaco-walkthrough-range.ts +++ b/apps/web/components/editors/monaco/use-monaco-walkthrough-range.ts @@ -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(null); - const [box, setBox] = useState(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(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; diff --git a/apps/web/e2e/tests/review/walkthrough.spec.ts b/apps/web/e2e/tests/review/walkthrough.spec.ts index 6202c8b3f0..de768da96c 100644 --- a/apps/web/e2e/tests/review/walkthrough.spec.ts +++ b/apps/web/e2e/tests/review/walkthrough.spec.ts @@ -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");