diff --git a/apps/web/e2e/tests/session/session-tab-management.spec.ts b/apps/web/e2e/tests/session/session-tab-management.spec.ts index 9626a4bf40..48baafd7bc 100644 --- a/apps/web/e2e/tests/session/session-tab-management.spec.ts +++ b/apps/web/e2e/tests/session/session-tab-management.spec.ts @@ -1,5 +1,8 @@ import { expect } from "@playwright/test"; import type { Page } from "@playwright/test"; +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; import { test } from "../../fixtures/test-base"; import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; @@ -241,6 +244,102 @@ test.describe("Session tab management — close behavior", () => { expect(sessions.map((s) => s.id)).toEqual([session2Id]); }); + test("deleting the active shared-environment session keeps Changes data visible", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(150_000); + + const { task, session, session1Id, session2Id } = await createTaskWithTwoSessions( + testPage, + apiClient, + seedData, + "Delete Active Session Keeps Changes", + ); + const worktreePath = seedData.repositoryPath; + const worktreeBranch = execFileSync("git", ["branch", "--show-current"], { + cwd: worktreePath, + encoding: "utf8", + }).trim(); + expect(worktreeBranch).toBeTruthy(); + + const localChange = "session-delete-change.txt"; + const remoteChange = "remote-session-delete-change.ts"; + const localChangePath = path.join(worktreePath, localChange); + fs.writeFileSync(localChangePath, "keep this change visible\n"); + + try { + await apiClient.mockGitHubReset(); + await apiClient.mockGitHubSetUser("test-user"); + await apiClient.mockGitHubAddPRs([ + { + number: 3157, + title: "Keep Changes visible after session deletion", + state: "open", + head_branch: worktreeBranch, + base_branch: "main", + author_login: "test-user", + repo_owner: "testorg", + repo_name: "testrepo", + additions: 1, + deletions: 0, + }, + ]); + await apiClient.mockGitHubAddPRFiles("testorg", "testrepo", 3157, [ + { + filename: remoteChange, + status: "added", + additions: 1, + deletions: 0, + patch: "@@ -0,0 +1 @@\n+export const kept = true;", + }, + ]); + await apiClient.mockGitHubAssociateTaskPR({ + task_id: task.id, + workspace_id: seedData.workspaceId, + repository_id: seedData.repositoryId, + owner: "testorg", + repo: "testrepo", + pr_number: 3157, + pr_url: "https://github.com/testorg/testrepo/pull/3157", + pr_title: "Keep Changes visible after session deletion", + head_branch: worktreeBranch, + base_branch: "main", + author_login: "test-user", + additions: 1, + deletions: 0, + }); + + await testPage.reload(); + await session.waitForLoad(); + await session.sessionTabBySessionId(session1Id).click(); + await session.clickTab("Changes"); + await expect(session.changesFileRow(localChange)).toBeVisible({ timeout: 20_000 }); + await session.expandPRChangesSection(); + await expect( + session.prFilesSection().locator(`[data-changes-file="${remoteChange}"]`), + ).toBeVisible({ timeout: 20_000 }); + + await session.sessionTabBySessionId(session1Id).click({ button: "right" }); + await session.contextMenuItem("Delete").click(); + const confirmation = testPage.getByTestId("session-delete-confirm-popover"); + await expect(confirmation).toBeVisible({ timeout: 5_000 }); + await confirmation.getByTestId("session-delete-confirm").click(); + + await expect(session.sessionTabBySessionId(session1Id)).not.toBeVisible({ timeout: 15_000 }); + await expect(session.sessionTabBySessionId(session2Id)).toBeVisible({ timeout: 5_000 }); + await session.clickTab("Changes"); + await expect(session.changesFileRow(localChange)).toBeVisible({ timeout: 20_000 }); + await session.expandPRChangesSection(); + await expect( + session.prFilesSection().locator(`[data-changes-file="${remoteChange}"]`), + ).toBeVisible({ timeout: 20_000 }); + } finally { + fs.rmSync(localChangePath, { force: true }); + } + }); + test("deleting then immediately switching tasks does not resurrect the tab", async ({ testPage, apiClient, diff --git a/apps/web/hooks/use-environment-session-id.test.ts b/apps/web/hooks/use-environment-session-id.test.ts new file mode 100644 index 0000000000..255e8e9aa5 --- /dev/null +++ b/apps/web/hooks/use-environment-session-id.test.ts @@ -0,0 +1,82 @@ +import { renderHook } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +type MockStoreState = { + tasks: { activeSessionId: string | null }; + environmentIdBySessionId: Record; +}; + +let mockStoreState: MockStoreState; + +vi.mock("@/components/state-provider", () => ({ + useAppStore: (selector: (state: MockStoreState) => unknown) => selector(mockStoreState), +})); + +import { useEnvironmentSessionId } from "./use-environment-session-id"; + +const ENVIRONMENT_ID = "environment-shared"; +const FIRST_SESSION_ID = "session-first"; +const SECOND_SESSION_ID = "session-second"; + +function setActiveSession(activeSessionId: string, sessionIds: string[]) { + mockStoreState = { + tasks: { activeSessionId }, + environmentIdBySessionId: Object.fromEntries( + sessionIds.map((sessionId) => [sessionId, ENVIRONMENT_ID]), + ), + }; +} + +beforeEach(() => { + setActiveSession(FIRST_SESSION_ID, [FIRST_SESSION_ID, SECOND_SESSION_ID]); +}); + +describe("useEnvironmentSessionId", () => { + // @covers AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.9 + it("promotes the active same-environment session after the cached session is purged", () => { + const { result, rerender } = renderHook(() => useEnvironmentSessionId()); + + expect(result.current).toBe(FIRST_SESSION_ID); + + setActiveSession(SECOND_SESSION_ID, [FIRST_SESSION_ID, SECOND_SESSION_ID]); + rerender(); + expect(result.current).toBe(FIRST_SESSION_ID); + + setActiveSession(SECOND_SESSION_ID, [SECOND_SESSION_ID]); + rerender(); + + expect(result.current).toBe(SECOND_SESSION_ID); + }); + + it("keeps a live lookup handle stable across same-environment session switches", () => { + const { result, rerender } = renderHook(() => useEnvironmentSessionId()); + + setActiveSession(SECOND_SESSION_ID, [FIRST_SESSION_ID, SECOND_SESSION_ID]); + rerender(); + + expect(result.current).toBe(FIRST_SESSION_ID); + }); + + it("uses the active session when the environment changes", () => { + const { result, rerender } = renderHook(() => useEnvironmentSessionId()); + + mockStoreState = { + tasks: { activeSessionId: SECOND_SESSION_ID }, + environmentIdBySessionId: { + [FIRST_SESSION_ID]: ENVIRONMENT_ID, + [SECOND_SESSION_ID]: "environment-second", + }, + }; + rerender(); + + expect(result.current).toBe(SECOND_SESSION_ID); + }); + + it("uses the active session as the pre-hydration lookup handle", () => { + setActiveSession(FIRST_SESSION_ID, []); + + const { result } = renderHook(() => useEnvironmentSessionId()); + + expect(result.current).toBe(FIRST_SESSION_ID); + }); +}); diff --git a/apps/web/hooks/use-environment-session-id.ts b/apps/web/hooks/use-environment-session-id.ts index 99671596a8..2b7799a683 100644 --- a/apps/web/hooks/use-environment-session-id.ts +++ b/apps/web/hooks/use-environment-session-id.ts @@ -18,7 +18,11 @@ export function useEnvironmentSessionId(): string | null { const selector = useCallback((state: StoreSlice) => { const sid = state.tasks.activeSessionId; const envId = sid ? (state.environmentIdBySessionId[sid] ?? sid) : null; - if (envId === cacheRef.current.envId) return cacheRef.current.sessionId; + const cachedSessionId = cacheRef.current.sessionId; + const cachedSessionIsUsable = + cachedSessionId === sid || + (cachedSessionId !== null && state.environmentIdBySessionId[cachedSessionId] === envId); + if (envId === cacheRef.current.envId && cachedSessionIsUsable) return cachedSessionId; cacheRef.current = { envId, sessionId: sid }; return sid; }, []); diff --git a/docs/plans/session-delete-changes-continuity/plan.md b/docs/plans/session-delete-changes-continuity/plan.md new file mode 100644 index 0000000000..ae06206a37 --- /dev/null +++ b/docs/plans/session-delete-changes-continuity/plan.md @@ -0,0 +1,129 @@ +--- +created: 2026-08-30 +status: implemented +requirements: + - REQ-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001 +system_design: + - ../../specs/tasks/system-design/session-delete-resource-cleanup.md +legacy_specs: [] +--- + +# Implementation Plan: Preserve Changes Data After Session Deletion + +## Overview + +Correct the environment-stable session selector so deleting its cached session +promotes the surviving active session as the agentctl lookup handle. Add the +selector regression first. Then prove that Changes keeps workspace and matching +pull-request data after same-environment active-session deletion. + +## Scope + +### In scope + +- Preserve the existing no-refetch behavior for ordinary switches between live + sessions that share a task environment. +- Replace a cached session handle after its environment mapping is purged and a + surviving active session maps to the same environment. +- Keep environment-keyed Git, cumulative-diff, and pull-request data visible in + the desktop Changes panel after active-session deletion. +- Cover the shared data path used by the existing mobile Changes surface. + +### Out of scope + +- Backend session deletion, task-environment ownership, or Git/PR APIs. +- Changes panel layout, navigation, touch behavior, copy, or empty-state design. +- Session-scoped file-review markers and their cache ownership. +- Changing when ordinary same-environment tab switches refetch workspace data. +- Recovering tasks that have no surviving session lookup handle. + +## Confirmed root cause + +`useEnvironmentSessionId` caches a session ID by environment identity. During +active-session deletion, `useSessionActions.remove` selects the surviving +session first. Then `removeTaskSession` purges the deleted session's environment +mapping. Both sessions have the same environment ID. Thus, the selector keeps +the deleted session after the mapping disappears. Git and commit hooks then use +caches keyed by the deleted session ID. Cumulative-diff requests fail with +`task session not found`. Branch-scoped PR selection also drops the fetched PR +files. + +The smallest reliable reproduction uses two sessions that share one task +environment. Load Changes through the first session. Delete that active session +and reopen Changes through the surviving session. The current implementation +shows the empty state. The environment cumulative diff and PR queries still +contain files. + +## Technical approach + +### Environment-stable lookup handle + +Update `apps/web/hooks/use-environment-session-id.ts`. Reuse the cached session +ID only when it is active or its current mapping equals the active environment. +If the cached mapping is absent, use the current active session. +Preserve the session-ID fallback while the active session's environment mapping +loads. + +Add `apps/web/hooks/use-environment-session-id.test.ts`. The regression named +`promotes the active same-environment session after the cached session is +purged` must fail before the correction. It must pass after the correction. +Adjacent cases retain the cached handle during a live same-environment switch. +They replace it during an environment switch. + +### Visible session-deletion regression + +Extend `apps/web/e2e/tests/session/session-tab-management.spec.ts` with +`deleting the active shared-environment session keeps Changes data visible`. +Use the existing two-session setup. Make sure that both sessions reference the +same environment. Seed a workspace change and a branch-matching mocked pull +request. Open Changes through the deletion target. Then delete it through the +visible session-tab UI. Reopen Changes. Make sure that the workspace file and +PR file remain visible through the surviving session. + +The desktop test exercises the shared `useChangesPanelData` path. No mobile +Playwright test is required because the correction changes only data-handle +normalization. `MobileChangesPanel` already consumes the same hook. This fix +does not change mobile composition, navigation, scrolling, touch targets, or +viewport behavior. + +## Tests + +- `AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.9` maps to + `apps/web/hooks/use-environment-session-id.test.ts`, including the failing + deleted-cache-handle regression and live same-environment stability case. +- Existing session-runtime purge tests continue to prove that only the deleted + session mapping is removed while the shared environment cache survives. + +## E2E tests + +- `AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.9` maps to + `apps/web/e2e/tests/session/session-tab-management.spec.ts` in the Chromium + project. The scenario deletes the active shared-environment session through + the UI and proves workspace and PR files remain visible in Changes. +- Mobile parity is covered at the shared data-hook boundary because this fix + makes no rendered or responsive change. The shipped exemplar remains + `apps/web/components/task/mobile/mobile-changes-panel.tsx`, which reuses + `useChangesPanelData`, `ChangesPanelHeader`, and `ChangesPanelBody`. + +## Work orders + +- [x] [Task 01: Preserve the live Changes lookup handle](task-01-preserve-live-changes-handle.md) + +## Verification results + +- The selector regression failed before the production correction because it + retained `session-first` after that session's environment mapping was purged. +- The focused Vitest suite passes with four cases. +- The web TypeScript check passes. +- The focused Chromium production-build E2E case passes with the workspace and + PR files visible after deletion. + +## Risks + +- Cache invalidation on every active-session switch causes needless + subscriptions and fetches. The correction must distinguish deleted handles + from live same-environment handles. +- An active session must remain usable while its environment mapping loads. + Treating every missing mapping as deletion causes a startup regression. +- The E2E pull request must match the live checkout branch and repository. This + prevents a fixture mismatch from changing the result for the wrong reason. diff --git a/docs/plans/session-delete-changes-continuity/task-01-preserve-live-changes-handle.md b/docs/plans/session-delete-changes-continuity/task-01-preserve-live-changes-handle.md new file mode 100644 index 0000000000..ca7e716ddb --- /dev/null +++ b/docs/plans/session-delete-changes-continuity/task-01-preserve-live-changes-handle.md @@ -0,0 +1,95 @@ +--- +id: "01-preserve-live-changes-handle" +title: "Preserve the live Changes lookup handle" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001 +acceptance_criteria: + - AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.9 +system_design: + - ../../specs/tasks/system-design/session-delete-resource-cleanup.md +--- + +# Task 01: Preserve the Live Changes Lookup Handle + +## Summary + +Make the environment-stable session selector retain only a live lookup handle. +Use unit and browser regressions to prove the correction. Deleting an active +session must not empty workspace or pull-request data from a shared task +environment. + +## In scope + +- Add the environment-session selector unit regression before production code. +- Correct cached-handle validation without changing ordinary live + same-environment switch behavior. +- Add a desktop session-deletion E2E regression that verifies workspace and PR + files remain in Changes. + +## Out of scope + +- Backend lifecycle or API changes. +- Changes panel presentation or mobile-specific composition changes. +- Broad refactors of environment-keyed frontend state. + +## Acceptance + +- A cached session remains stable while it is active or remains mapped to the + active environment. +- Removing the cached session's mapping promotes the current active session, + including when that session resolves to the same environment. +- Deleting the active session through the UI leaves workspace and matching PR + files visible in Changes through the surviving session. + +## Verification + +```bash +(cd apps && pnpm --filter @kandev/web test -- hooks/use-environment-session-id.test.ts) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm e2e:run tests/session/session-tab-management.spec.ts -- --grep "deleting the active shared-environment session keeps Changes data visible") +``` + +## Files likely touched + +- `apps/web/hooks/use-environment-session-id.ts` +- `apps/web/hooks/use-environment-session-id.test.ts` +- `apps/web/e2e/tests/session/session-tab-management.spec.ts` + +## Dependencies + +None. + +## Risks + +- The selector must not discard a valid cached handle during a normal + same-environment tab switch. +- The pre-hydration active-session fallback must remain valid. +- The PR fixture must be branch-scoped to the environment checkout. + +## Parallelism + +`sequential` + +## Inputs + +- `AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.9`. +- The frontend environment-handle continuity section in the paired system + design. +- `useSessionActions.remove` ordering and session-runtime purge behavior. +- Existing active-session deletion and PR Changes Playwright scenarios. + +## Results + +- Added a failing selector regression for a purged same-environment cache + handle, then corrected the selector to promote the surviving active session. +- Preserved live same-environment handle stability, environment-switch + replacement, and the pre-hydration active-session fallback in unit coverage. +- Added a Chromium regression that deletes the active session through the UI + and verifies that a workspace file and branch-matching PR file remain visible + in Changes. +- Verification passed: 4 focused Vitest cases, web TypeScript, and 1 focused + production-build Playwright case. diff --git a/docs/specs/tasks/requirements/session-delete-resource-cleanup.md b/docs/specs/tasks/requirements/session-delete-resource-cleanup.md index f9ca9b3dff..b6ad1687b4 100644 --- a/docs/specs/tasks/requirements/session-delete-resource-cleanup.md +++ b/docs/specs/tasks/requirements/session-delete-resource-cleanup.md @@ -2,6 +2,7 @@ status: draft system: tasks created: 2026-08-08 +updated: 2026-08-30 owners: - cfl --- @@ -27,6 +28,7 @@ A session is a conversation and execution reference inside a task; it is not the - **AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.6:** Task archive/delete, cascade archive/delete, workspace delete, quick-chat task expiry, and explicit task-environment reset remain the owners of physical cleanup. - **AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.7:** Task lifecycle cleanup discovers task-owned resources without joining through a session row and executes asynchronously through the durable cleanup worker. - **AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.8:** Resources borrowed by another task or referenced through a shared environment are preserved or transferred according to existing ownership rules. +- **AC-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001.9:** When an active session is deleted, another session can continue to use the same task environment. Desktop and mobile workspace-derived views shall continue to show current data. This data includes changed files, commits, and branch-matching remote contributions. The views shall not require a reload or replacement session. ## System design diff --git a/docs/specs/tasks/system-design/session-delete-resource-cleanup.md b/docs/specs/tasks/system-design/session-delete-resource-cleanup.md index 0eaeef54b6..5ccb5e4dfc 100644 --- a/docs/specs/tasks/system-design/session-delete-resource-cleanup.md +++ b/docs/specs/tasks/system-design/session-delete-resource-cleanup.md @@ -4,6 +4,7 @@ system: tasks requirements: - REQ-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001 created: 2026-08-08 +updated: 2026-08-30 owners: - cfl --- @@ -17,7 +18,7 @@ This design preserves the technical source detail for `REQ-TASKS-SESSION-DELETE- | Requirement | Design section | | --- | --- | -| `REQ-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001` | [Migrated source detail](#migrated-source-detail) | +| `REQ-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001` | [Migrated source detail](#migrated-source-detail), [Frontend environment-handle continuity](#frontend-environment-handle-continuity) | ## Migrated source detail @@ -66,6 +67,28 @@ task lifecycle operation takes responsibility for cleanup. say that the task workspace and files remain. They do not warn that uncommitted/unpushed workspace state will be removed. +## Frontend environment-handle continuity + +Workspace-derived frontend state is keyed by `TaskEnvironment`, while several +agentctl requests still require a live session ID as their lookup handle. +Across ordinary tab switches, `useEnvironmentSessionId` can retain a session ID +when both sessions resolve to the same environment. The retained handle is +valid only while it is active or remains mapped to that environment. + +Session deletion first selects a surviving session. Then it purges the deleted +session's runtime state. When the purge removes the retained mapping, +`useEnvironmentSessionId` promotes the current active session as the lookup +handle. The environment identity does not change. Environment-keyed caches for +Git status, commits, cumulative diffs, and remote contributions remain intact. +These caches continue to serve desktop and mobile Changes surfaces. File-review +markers remain session-scoped and are intentionally outside this continuity +guarantee. + +The selector does not treat a missing mapping as proof that an inactive cached +session is usable. Thus, downstream hooks do not use a deleted session ID as a +synthetic environment key. They also do not send agentctl requests for a +session that no longer exists. + ## Data Model ### `task_environments` @@ -312,6 +335,10 @@ remain the authorization boundary for physical cleanup. retries after failure and backend restart using its persisted snapshot. - If another task/session still borrows the environment, cleanup preserves or transfers the resource rather than deleting it. +- If the frontend removes the active session while another session shares its + environment, the environment cache remains authoritative and the surviving + session becomes the lookup handle. The Changes surface does not fall back to + an empty cache keyed by the deleted session ID. ## Persistence Guarantees @@ -337,6 +364,10 @@ remain the authorization boundary for physical cleanup. **THEN** it reuses the task workspace and observes the marker. - **GIVEN** two sessions sharing a task workspace, **WHEN** one is deleted, **THEN** the other continues using the same directory without interruption. +- **GIVEN** two sessions share a task environment and Changes shows workspace + commits or branch-matching remote contribution files, **WHEN** the active + session is deleted and the sibling becomes active, **THEN** desktop and mobile + Changes continue to show that environment's data without a reload. - **GIVEN** the final session was deleted and the backend restarted, **WHEN** the task is archived, **THEN** the task becomes archived and its canonical worktree is scheduled for asynchronous cleanup unless shared.