fix: preserve session-backed task navigation (#2601)

2026-08-13 11:08:24 +01:00
parent 10606d13a7
commit df388c125d
7 changed files with 451 additions and 6 deletions
@@ -198,14 +198,17 @@ describe("syncActiveTaskSession", () => {
    const setActiveSessionAuto = vi.fn();
    const setActiveTask = vi.fn();

-    syncActiveTaskSession({
+    const applied = syncActiveTaskSession({
      initialTaskId: "task-1",
      fallbackTaskId: null,
      initialSessionId: "session-1",
+      activeTaskId: null,
+      previousRouteTaskId: undefined,
      setActiveSessionAuto,
      setActiveTask,
    });

+    expect(applied).toBe(true);
    expect(setActiveSessionAuto).toHaveBeenCalledWith("task-1", "session-1");
    expect(setActiveTask).not.toHaveBeenCalled();
  });
@@ -214,17 +217,76 @@ describe("syncActiveTaskSession", () => {
    const setActiveSessionAuto = vi.fn();
    const setActiveTask = vi.fn();

-    syncActiveTaskSession({
+    const applied = syncActiveTaskSession({
      initialTaskId: "task-1",
      fallbackTaskId: null,
      initialSessionId: null,
+      activeTaskId: null,
+      previousRouteTaskId: undefined,
      setActiveSessionAuto,
      setActiveTask,
    });

+    expect(applied).toBe(true);
    expect(setActiveTask).toHaveBeenCalledWith("task-1");
    expect(setActiveSessionAuto).not.toHaveBeenCalled();
  });
+
+  it("applies a changed route over the previous active task", () => {
+    const setActiveSessionAuto = vi.fn();
+    const setActiveTask = vi.fn();
+
+    const applied = syncActiveTaskSession({
+      initialTaskId: "task-2",
+      fallbackTaskId: null,
+      initialSessionId: "session-2",
+      activeTaskId: "task-1",
+      previousRouteTaskId: "task-1",
+      setActiveSessionAuto,
+      setActiveTask,
+    });
+
+    expect(applied).toBe(true);
+    expect(setActiveSessionAuto).toHaveBeenCalledWith("task-2", "session-2");
+    expect(setActiveTask).not.toHaveBeenCalled();
+  });
+
+  it("adopts a session that arrives for the current route", () => {
+    const setActiveSessionAuto = vi.fn();
+    const setActiveTask = vi.fn();
+
+    const applied = syncActiveTaskSession({
+      initialTaskId: "task-1",
+      fallbackTaskId: null,
+      initialSessionId: "session-1",
+      activeTaskId: "task-1",
+      previousRouteTaskId: "task-1",
+      setActiveSessionAuto,
+      setActiveTask,
+    });
+
+    expect(applied).toBe(true);
+    expect(setActiveSessionAuto).toHaveBeenCalledWith("task-1", "session-1");
+    expect(setActiveTask).not.toHaveBeenCalled();
+  });
+
+  it("does not restore an unchanged route over an in-place sibling selection", () => {
+    const setActiveSessionAuto = vi.fn();
+    const setActiveTask = vi.fn();
+    const applied = syncActiveTaskSession({
+      initialTaskId: "missing-task",
+      fallbackTaskId: null,
+      initialSessionId: "sibling-session",
+      activeTaskId: "sibling-task",
+      previousRouteTaskId: "missing-task",
+      setActiveSessionAuto,
+      setActiveTask,
+    });
+
+    expect(applied).toBe(false);
+    expect(setActiveSessionAuto).not.toHaveBeenCalled();
+    expect(setActiveTask).not.toHaveBeenCalled();
+  });
 });

 describe("resolveEffectiveTask archived state", () => {
@@ -201,13 +201,18 @@ export function syncActiveTaskSession(params: {
  initialTaskId: string | undefined;
  fallbackTaskId: string | null | undefined;
  initialSessionId: string | null;
+  activeTaskId: string | null;
+  previousRouteTaskId: string | null | undefined;
  setActiveSessionAuto: (taskId: string, sessionId: string) => void;
  setActiveTask: (taskId: string) => void;
-}) {
+}): boolean {
  const taskId = params.initialTaskId ?? params.fallbackTaskId;
-  if (!taskId) return;
+  if (!taskId) return false;
+  const routeChanged = params.previousRouteTaskId !== taskId;
+  if (!routeChanged && params.activeTaskId !== taskId) return false;
  if (params.initialSessionId) params.setActiveSessionAuto(taskId, params.initialSessionId);
  else params.setActiveTask(taskId);
+  return true;
 }

 export function resolveTaskIds(task: Task | null) {
@@ -260,6 +260,12 @@ function useTaskPageData(
  const activeTaskId = useAppStore((state) => state.tasks.activeTaskId);
  const setActiveSessionAuto = useAppStore((state) => state.setActiveSessionAuto);
  const setActiveTask = useAppStore((state) => state.setActiveTask);
+  const previousRouteTaskId = useRef<string | null | undefined>(undefined);
+  // Route synchronization must see the current selection when route data or
+  // delayed session hydration changes, but a sidebar selection must not
+  // retrigger it and overwrite the session chosen by task selection.
+  const activeTaskIdForSyncRef = useRef(activeTaskId);
+  activeTaskIdForSyncRef.current = activeTaskId;

  // Validate that activeSessionId belongs to activeTaskId to prevent showing
  // messages from an unrelated session when navigating to a task without sessions.
@@ -282,13 +288,16 @@ function useTaskPageData(
  const effectiveSessionId = validatedActiveSessionId ?? initialSessionId;

  useEffect(() => {
-    syncActiveTaskSession({
+    const applied = syncActiveTaskSession({
      initialTaskId: initialTask?.id,
      fallbackTaskId,
      initialSessionId,
+      activeTaskId: activeTaskIdForSyncRef.current,
+      previousRouteTaskId: previousRouteTaskId.current,
      setActiveSessionAuto,
      setActiveTask,
    });
+    if (applied) previousRouteTaskId.current = initialTask?.id ?? fallbackTaskId;
  }, [initialTask?.id, fallbackTaskId, initialSessionId, setActiveSessionAuto, setActiveTask]);

  const { repositories } = useRepositories(task?.workspace_id ?? null, Boolean(task?.workspace_id));
@@ -1,5 +1,6 @@
 import { test, expect } from "../../fixtures/test-base";
 import { SidebarTasksPage } from "../../pages/sidebar-tasks-page";
+import { SessionPage } from "../../pages/session-page";
 import { openBlockedTaskLoadingState } from "./task-loading-state-helpers";

 test.describe("Task loading state", () => {
@@ -47,4 +48,131 @@ test.describe("Task loading state", () => {
    await expect(testPage).toHaveURL(new RegExp(`/t/${sibling.id}$`));
    await expect(testPage.getByTestId("task-load-error-state")).toBeHidden({ timeout: 10_000 });
  });
+
+  test("keeps a session-backed sibling active", async ({ testPage, apiClient, seedData }) => {
+    const sibling = await apiClient.createTaskWithAgent(
+      seedData.workspaceId,
+      "Missing route session sibling",
+      seedData.agentProfileId,
+      {
+        description: "/e2e:simple-message",
+        workflow_id: seedData.workflowId,
+        workflow_step_id: seedData.startStepId,
+        repository_ids: [seedData.repositoryId],
+      },
+    );
+
+    if (!sibling.session_id) throw new Error("session-backed sibling has no session");
+
+    await testPage.goto(`/t/missing-task-route-session?workspaceId=${seedData.workspaceId}`);
+    await expect(testPage.getByTestId("task-load-error-state")).toBeVisible({ timeout: 10_000 });
+
+    const sidebar = new SidebarTasksPage(testPage);
+    await expect(sidebar.row(sibling.id)).toBeVisible({ timeout: 10_000 });
+    await sidebar.plainClick(sibling.id);
+
+    await expect(testPage).toHaveURL(new RegExp(`/t/${sibling.id}$`));
+    await expect(testPage.getByTestId("task-load-error-state")).toBeHidden({ timeout: 10_000 });
+    await expect(testPage.getByTestId("dockview-task-layout")).toBeVisible({ timeout: 10_000 });
+    await expect(sidebar.row(sibling.id)).toHaveAttribute("data-active", "true", {
+      timeout: 10_000,
+    });
+  });
+
+  test("restores the last selected session after an A to B to A switch", async ({
+    testPage,
+    apiClient,
+    seedData,
+  }) => {
+    test.setTimeout(120_000);
+
+    const taskA = await apiClient.createTaskWithAgent(
+      seedData.workspaceId,
+      "Session route sync task A",
+      seedData.agentProfileId,
+      {
+        description: "/e2e:simple-message",
+        workflow_id: seedData.workflowId,
+        workflow_step_id: seedData.startStepId,
+        repository_ids: [seedData.repositoryId],
+      },
+    );
+    const taskB = await apiClient.createTaskWithAgent(
+      seedData.workspaceId,
+      "Session route sync task B",
+      seedData.agentProfileId,
+      {
+        description: "/e2e:simple-message",
+        workflow_id: seedData.workflowId,
+        workflow_step_id: seedData.startStepId,
+        repository_ids: [seedData.repositoryId],
+      },
+    );
+
+    if (!taskA.session_id || !taskB.session_id) {
+      throw new Error("session-backed route sync tasks have no primary session");
+    }
+
+    await testPage.goto(`/t/${taskA.id}`);
+    const session = new SessionPage(testPage);
+    await session.waitForLoad();
+
+    await session.openNewSessionDialog();
+    await expect(session.newSessionDialog()).toBeVisible({ timeout: 5_000 });
+    await session.newSessionPromptInput().fill("/e2e:simple-message");
+    await session.newSessionStartButton().click();
+    await expect(session.newSessionDialog()).not.toBeVisible({ timeout: 10_000 });
+
+    let secondarySessionId: string | undefined;
+    await expect
+      .poll(
+        async () => {
+          const { sessions } = await apiClient.listTaskSessions(taskA.id);
+          secondarySessionId = sessions.find((item) => item.id !== taskA.session_id)?.id;
+          return secondarySessionId ?? null;
+        },
+        { timeout: 30_000, message: "Waiting for task A secondary session" },
+      )
+      .toBeTruthy();
+
+    await expect(session.sessionTabBySessionId(secondarySessionId!)).toBeVisible({
+      timeout: 15_000,
+    });
+    await session.sessionTabBySessionId(secondarySessionId!).click();
+    await expect
+      .poll(async () =>
+        testPage.evaluate(
+          () =>
+            (
+              window as unknown as {
+                __KANDEV_E2E_STORE__?: { getState: () => { tasks: { activeSessionId: string } } };
+              }
+            ).__KANDEV_E2E_STORE__?.getState().tasks.activeSessionId,
+        ),
+      )
+      .toBe(secondarySessionId);
+
+    await session.clickTaskInSidebar("Session route sync task B");
+    await expect(testPage).toHaveURL(new RegExp(`/t/${taskB.id}$`), { timeout: 15_000 });
+    await session.waitForLoad();
+    await expect(session.sessionTabBySessionId(taskB.session_id)).toBeVisible({
+      timeout: 15_000,
+    });
+
+    await session.clickTaskInSidebar("Session route sync task A");
+    await expect(testPage).toHaveURL(new RegExp(`/t/${taskA.id}$`), { timeout: 15_000 });
+    await session.waitForLoad();
+    await expect
+      .poll(async () =>
+        testPage.evaluate(
+          () =>
+            (
+              window as unknown as {
+                __KANDEV_E2E_STORE__?: { getState: () => { tasks: { activeSessionId: string } } };
+              }
+            ).__KANDEV_E2E_STORE__?.getState().tasks.activeSessionId,
+        ),
+      )
+      .toBe(secondarySessionId);
+  });
 });
@@ -0,0 +1,125 @@
+---
+spec: docs/specs/missing-task-route-recovery/spec.md
+created: 2026-08-12
+status: complete
+---
+
+# Implementation Plan: Missing Task Session Navigation
+
+## Overview
+
+Guard route initialization so that stale route props cannot replace a later sidebar selection. Add unit coverage for the guard and a session-backed browser regression.
+
+## Root cause
+
+The missing route keeps its task ID in `TaskPageContent` after an in-place sidebar switch. The sidebar updates the store and URL without a route remount.
+
+A loaded sibling session then changes `agent.taskSessionId`. This change runs the route synchronization effect again. The effect pairs the sibling session with the missing route ID and restores the error state.
+
+The current browser test creates a sibling without a session. That setup does not enter the failing fast path.
+
+## Frontend
+
+### Route-to-store synchronization
+
+- Extend `syncActiveTaskSession` with `activeTaskId` and `previousRouteTaskId` inputs.
+- Change its return type to `boolean`. The value reports whether the function applied the route state.
+
+```typescript
+export function syncActiveTaskSession(params: {
+  initialTaskId: string | undefined;
+  fallbackTaskId: string | null | undefined;
+  initialSessionId: string | null;
+  activeTaskId: string | null;
+  previousRouteTaskId: string | null | undefined;
+  setActiveSessionAuto: (taskId: string, sessionId: string) => void;
+  setActiveTask: (taskId: string) => void;
+}): boolean;
+```
+
+- Return `true` for the first route initialization and for a changed route task ID.
+- Return `true` when the active store task still matches the unchanged route. This rule permits delayed session adoption for the current route.
+- Return `false` when the route ID is unchanged and the active store task differs. This rule preserves an in-place sidebar selection.
+- Track the prior route task ID with `useRef<string | null | undefined>(undefined)` in `useTaskPageData`.
+- Update the ref only when `syncActiveTaskSession` returns `true`.
+- Read the latest active task ID through a ref, but do not include it as a
+  reactive effect dependency. Sidebar task selection owns preferred-session
+  restoration and must not be followed by route synchronization that reapplies
+  the route's primary session.
+
+### URL and Dockview boundaries
+
+- Keep `replaceTaskUrl` as a non-navigating URL update.
+- Keep the existing Dockview environment switch and session selection paths.
+- Do not add a route remount or a hard refresh. Both changes can discard the fast workbench transition.
+
+### Mobile design contract
+
+- The change only guards shared task and session state. It does not change layout, controls, touch behavior, or scrolling.
+- The phone error state keeps the existing task-overview action.
+- The existing phone scenario continues to cover recovery from an unavailable route.
+- No new phone browser test is necessary for this desktop-sidebar path.
+
+## Tests
+
+- **What:** The first route initialization can select its route task.
+- **File:** `apps/web/components/task/task-page-content-helpers.test.ts`.
+- **How:** Call `syncActiveTaskSession` with no prior route task ID.
+
+- **What:** A changed route task ID can replace a stale active task.
+- **File:** `apps/web/components/task/task-page-content-helpers.test.ts`.
+- **How:** Call `syncActiveTaskSession` with different prior and current route task IDs.
+
+- **What:** The current route can adopt a session that becomes available later.
+- **File:** `apps/web/components/task/task-page-content-helpers.test.ts`.
+- **How:** Use the same route and active task ID.
+
+- **What:** An unchanged stale route cannot replace an in-place sibling selection.
+- **File:** `apps/web/components/task/task-page-content-helpers.test.ts`.
+- **How:** Use the same route ID and a different active store task ID. This assertion must fail before the production change.
+
+## E2E Tests
+
+- **Scenario:** Given a missing task route and a sibling with a loaded primary session, selecting the sibling keeps its workbench active.
+- **File:** `apps/web/e2e/tests/task/task-loading-state.spec.ts`.
+- **Setup:** Create the sibling with `createTaskWithAgent` and the existing mock-agent profile.
+- **What to validate:** The URL selects the sibling. The error state disappears. The Dockview workbench appears. The sibling row stays active after session updates.
+- Keep the existing no-session sibling scenario. It covers the prepare-session path.
+- **Scenario:** Given task A has a primary and a non-primary session, switching
+  A → B → A preserves the selected non-primary session.
+- **What to validate:** The route returns to A while the active-session store
+  value remains the non-primary session.
+
+## Implementation Waves And Parallel Candidates
+
+Wave 1:
+
+- [x] [Task 01: Guard missing-route task synchronization](task-01-guard-route-task-sync.md)
+
+The task is sequential. The unit guard and browser regression describe one state transition.
+
+## Risks and out of scope
+
+- The guard must still let a real route change replace the active task.
+- The guard must still let the current route adopt a session that arrives later.
+- A ref must distinguish the first route synchronization from an unchanged missing route.
+- Changes to `replaceTaskUrl`, Dockview layouts, backend task lookup, and archive behavior are out of scope.
+- The repair adds no API, WebSocket, persistence, permission, or user-interface contract.
+
+## Verification Results
+
+- RED browser run: the focused session-backed sibling E2E reproduced the visible
+  task error state after sibling selection.
+- RED unit run: the stale-route case failed because `syncActiveTaskSession` returned
+  `undefined` and re-applied stale route state.
+- GREEN unit run: the focused helper suite passed, 22 tests.
+- GREEN browser run: the focused production-build E2E passed, 1 test.
+- `pnpm run typecheck` passed.
+- Targeted Prettier check passed.
+- Targeted ESLint passed.
+- Review fixup RED E2E: the A → B → A secondary-session regression returned
+  task A's primary session after returning from task B.
+- Review fixup GREEN E2E: the same production-build regression passed after
+  removing the active-task-only effect trigger and reading the current active
+  task from a ref.
+- Full task-loading-state production-build E2E passed, 4 tests.
@@ -0,0 +1,104 @@
+---
+id: "01-guard-route-task-sync"
+title: "Guard missing-route task synchronization"
+status: done
+wave: 1
+depends_on: []
+plan: "plan.md"
+spec: "../../specs/missing-task-route-recovery/spec.md"
+---
+
+# Task 01: Guard Missing-Route Task Synchronization
+
+Prevent an unchanged missing route from replacing a later session-backed sidebar selection.
+
+## Acceptance
+
+- Route initialization and real route changes still select the route task and its available session.
+- An unchanged stale route cannot pair its task ID with a session from the active sibling task.
+- The session-backed browser regression keeps the sibling URL, active row, and Dockview workbench after session updates.
+- Returning from another task preserves a deliberately selected non-primary session.
+
+## TDD sequence
+
+1. Add the session-backed browser scenario. Run it before production changes and record the expected error-state failure.
+2. Add a stale-route unit case to `syncActiveTaskSession`. Record the expected setter assertion failure.
+3. Extend the helper inputs and add the minimum effect guard that passes the unit cases.
+4. Run the focused unit cases and web typecheck.
+5. Rebuild the production web assets through the managed E2E command. Run the browser scenario again.
+6. Run the A to B to A secondary-session regression before closing review fixup.
+
+## Verification
+
+If `apps/node_modules` is absent, run this command first:
+
+```bash
+cd apps && pnpm install --frozen-lockfile
+```
+
+Run these commands in order:
+
+```bash
+cd apps/web && pnpm e2e:run tests/task/task-loading-state.spec.ts -- --grep "keeps a session-backed sibling active"
+cd apps && pnpm --filter @kandev/web exec vitest run components/task/task-page-content-helpers.test.ts
+cd apps/web && pnpm run typecheck
+cd apps && pnpm exec prettier --check web/components/task/task-page-content.tsx web/components/task/task-page-content-helpers.ts web/components/task/task-page-content-helpers.test.ts web/e2e/tests/task/task-loading-state.spec.ts
+cd apps && pnpm --filter @kandev/web exec eslint components/task/task-page-content.tsx components/task/task-page-content-helpers.ts components/task/task-page-content-helpers.test.ts e2e/tests/task/task-loading-state.spec.ts
+cd apps/web && pnpm e2e:run tests/task/task-loading-state.spec.ts -- --grep "keeps a session-backed sibling active"
+```
+
+The first browser command is the RED run. The final browser command is the GREEN run and must rebuild the production assets.
+
+## Files likely touched
+
+- `apps/web/components/task/task-page-content.tsx`
+- `apps/web/components/task/task-page-content-helpers.ts`
+- `apps/web/components/task/task-page-content-helpers.test.ts`
+- `apps/web/e2e/tests/task/task-loading-state.spec.ts`
+- `docs/plans/missing-task-session-navigation/plan.md`
+- `docs/plans/missing-task-session-navigation/task-01-guard-route-task-sync.md`
+
+## Dependencies
+
+None.
+
+## Parallelism
+
+`sequential`. The unit guard and browser regression cover the same state transition.
+
+## Inputs
+
+- The session-backed regression scenario in `docs/specs/missing-task-route-recovery/spec.md`.
+- The confirmed production-build failure in the current task conversation.
+- `syncActiveTaskSession` and `useTaskPageData` as the current route synchronization path.
+- `selectTaskWithLayout` and `replaceTaskUrl` as the current in-place switch path.
+- The existing no-session test in `apps/web/e2e/tests/task/task-loading-state.spec.ts`.
+
+## Risks
+
+- An over-strict guard can block a real route change.
+- An over-strict guard can block delayed session adoption for the current route.
+- A URL-navigation change can remount the workbench and expand the repair scope.
+
+## Output contract
+
+Report the RED and GREEN results. Report all changed files and exact command outcomes. Update this task and `plan.md` in the same conversation.
+
+## Results
+
+- RED browser run reproduced the task-load error after selecting a
+  session-backed sibling from the missing route.
+- RED unit run failed because the helper returned `undefined` for the stale
+  route case.
+- GREEN unit run passed 22 focused tests.
+- GREEN production-build E2E passed the session-backed sibling scenario.
+- Web typecheck, targeted Prettier, and targeted ESLint passed.
+- No new mobile test was needed because the repair changes shared route/store
+  state only. The existing mobile missing-route recovery scenario is unchanged.
+- Review fixup RED production-build E2E returned task A's primary session after
+  an A to B to A switch.
+- Review fixup GREEN production-build E2E preserved task A's selected
+  non-primary session.
+- The synchronization effect now reads the current active task from a ref and
+  is not triggered by active-task changes alone.
+- Full task-loading-state production-build E2E passed, 4 tests.
@@ -29,6 +29,11 @@ the desktop sidebar is hidden.
 - When the resolved workspace contains other visible tasks, the desktop task
  sidebar shows them and its rows remain usable navigation targets. The invalid
  route ID is not synthesized as a sidebar task.
+- When a visible sibling has a loaded session, selecting its sidebar row keeps
+  that sibling active after later session and WebSocket updates.
+- When a task has multiple sessions, selecting a non-primary session, visiting
+  another task, and returning to the first task restores the selected session
+  instead of reapplying the route's primary session.
 - The unavailable state includes a visible action back to the task overview for
  the resolved workspace. The action is keyboard accessible, touch reachable,
  and usable when no workspace can be resolved.
@@ -46,6 +51,12 @@ the desktop sidebar is hidden.
  the user cold-loads `/t/<missing-id>`, **THEN** the unavailable state appears,
  the desktop sidebar lists the other task instead of **No tasks yet**, and
  selecting that row opens the valid task.
+- **GIVEN** a visible sibling with a loaded primary session, **WHEN** the user
+  selects it from a missing task route, **THEN** the sibling route and workbench
+  stay active after session hydration and WebSocket updates.
+- **GIVEN** task A has a selected non-primary session, **WHEN** the user
+  switches to task B and returns to task A, **THEN** task A keeps that selected
+  session active.
 - **GIVEN** the active-workspace cookie and user settings identify different
  authorized workspaces, **WHEN** a missing task route is cold-loaded, **THEN**
  fallback task data comes from the cookie-selected workspace according to the
@@ -81,6 +92,7 @@ the desktop sidebar is hidden.
 - Changing Office task-detail not-found behavior under `/office/tasks/:id`.
 - Retrying backend failures beyond the existing task-detail client retry.

-## Implementation plan
+## Implementation plans

 - [Missing task route recovery](../../plans/missing-task-route-recovery/plan.md)
+- [Missing task session navigation repair](../../plans/missing-task-session-navigation/plan.md)