From db816d4222ec115d5648eceff572163efa18993c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 30 Mar 2026 12:19:55 +0100 Subject: [PATCH] fix(web): deduplicate agent and session tabs in task view (#496) --- .agents/skills/e2e/SKILL.md | 21 +++++ .github/workflows/e2e-tests.yml | 8 +- .../components/task/dockview-session-tabs.ts | 15 ++-- apps/web/components/task/session-tab.tsx | 88 ++++++++++++------- apps/web/e2e/pages/session-page.ts | 35 ++++++-- apps/web/e2e/tests/diff-update.spec.ts | 4 +- .../web/e2e/tests/session-tab-cleanup.spec.ts | 81 +++++++++++++++++ .../lib/state/layout-manager/merger.test.ts | 66 ++++++++++++++ apps/web/lib/state/layout-manager/merger.ts | 41 ++++++--- 9 files changed, 298 insertions(+), 61 deletions(-) create mode 100644 apps/web/e2e/tests/session-tab-cleanup.spec.ts create mode 100644 apps/web/lib/state/layout-manager/merger.test.ts diff --git a/.agents/skills/e2e/SKILL.md b/.agents/skills/e2e/SKILL.md index 431d98cb51..b4b5f5f6db 100644 --- a/.agents/skills/e2e/SKILL.md +++ b/.agents/skills/e2e/SKILL.md @@ -78,6 +78,27 @@ make test-e2e-report # open HTML report from last run - **Flaky timeouts** — **never increase locator timeouts to fix flaky tests.** If a locator times out, the root cause is almost always something else: a setup failure, missing navigation, race condition, or the element genuinely not rendering. Investigate why the element never appears instead of giving it more time. Note: infrastructure health timeouts (30s in `fixtures/backend.ts`) and overall test timeouts (60s in `playwright.config.ts`) are separate and should not be modified either. - Screenshots on failure, video on first retry (CI) +### Debugging CI shard failures + +CI splits tests across 10 shards. To reproduce a specific shard locally: + +```bash +# List which tests are in a shard +npx playwright test --config e2e/playwright.config.ts --shard=2/10 --list + +# Run that shard locally (requires production build) +make build-backend build-web +cd apps/web && npx playwright test --config e2e/playwright.config.ts --shard=2/10 +``` + +E2E tests run against the **production build** (`next build`), not dev mode. Always rebuild with `make build-web` (or `pnpm --filter @kandev/web build`) after code changes before running E2E tests locally. + +## Selector guidelines + +- **Prefer `data-testid` selectors** over text-based locators. Text content can change when UI is updated (e.g., hiding a badge), breaking tests that match by text. Use `getByTestId()` or `locator("[data-testid='...']")` for stable targeting. +- **Use page object methods** like `clickSessionChatTab()` (stable `data-testid`) instead of `sessionTabByText("1")` (fragile text match) for session tabs. +- **Dropdown menus can detach** from the DOM when React re-renders the parent (e.g., WS events updating the sidebar). The `openSidebarMenuAndClick()` helper in `session-page.ts` retries the full open-click sequence on detachment — use this pattern for similar interactions. + ## TDD workflow Follow `/tdd` when writing E2E tests: diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 221c217f3d..b003d9a990 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -29,13 +29,13 @@ permissions: jobs: e2e: - name: E2E Shard ${{ matrix.shard }}/4 + name: E2E Shard ${{ matrix.shard }}/10 runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 15 strategy: fail-fast: false matrix: - shard: [1, 2, 3, 4] + shard: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] steps: - name: Checkout code @@ -70,7 +70,7 @@ jobs: - name: Run E2E tests (shard ${{ matrix.shard }}/4) working-directory: apps/web - run: npx playwright test --config e2e/playwright.config.ts --shard=${{ matrix.shard }}/4 + run: npx playwright test --config e2e/playwright.config.ts --shard=${{ matrix.shard }}/10 - name: Upload blob report if: always() diff --git a/apps/web/components/task/dockview-session-tabs.ts b/apps/web/components/task/dockview-session-tabs.ts index e688385c75..727cb9e791 100644 --- a/apps/web/components/task/dockview-session-tabs.ts +++ b/apps/web/components/task/dockview-session-tabs.ts @@ -85,6 +85,16 @@ export function useAutoSessionTab(effectiveSessionId: string | null) { if (!effectiveSessionId) return; const api = useDockviewStore.getState().api; if (!api) return; + + // Always remove the generic "chat" panel when a session is active — + // it's replaced by per-session tabs. Must run before the early return + // so restored layouts with both "chat" and session panels get cleaned up. + // Skip removal in maximized state to avoid triggering the safety net + // which could disrupt the saved maximize layout. + const chatPanel = api.getPanel("chat"); + if (chatPanel && !useDockviewStore.getState().preMaximizeLayout) { + api.removePanel(chatPanel); + } if (api.getPanel(`session:${effectiveSessionId}`)) { sessionTabCreatedRef.current.add(effectiveSessionId); return; @@ -96,11 +106,6 @@ export function useAutoSessionTab(effectiveSessionId: string | null) { sessionTabCreatedRef.current.add(effectiveSessionId); return; } - // Always remove the generic "chat" panel — it's replaced by per-session tabs - const chatPanel = api.getPanel("chat"); - if (chatPanel) { - api.removePanel(chatPanel); - } // Resolve position: prefer centerGroupId if the group still exists, // fall back to placing right of sidebar, or omit position entirely. const { centerGroupId } = useDockviewStore.getState(); diff --git a/apps/web/components/task/session-tab.tsx b/apps/web/components/task/session-tab.tsx index aae258679c..f8137848bc 100644 --- a/apps/web/components/task/session-tab.tsx +++ b/apps/web/components/task/session-tab.tsx @@ -200,6 +200,50 @@ function DeleteSessionDialog({ ); } +function SessionContextMenuItems({ + sessionState, + isPrimary, + actions, + onDelete, +}: { + sessionState: TaskSessionState | null; + isPrimary: boolean; + actions: ReturnType; + onDelete: () => void; +}) { + return ( + + + Set as Primary + + + {sessionState && isStoppable(sessionState) && ( + + Stop + + )} + {sessionState && isResumable(sessionState) && ( + + Resume + + )} + {sessionState && isDeletable(sessionState) && ( + + Delete + + )} + + + Close Others + + + ); +} + /** * Custom dockview tab for session panels. * Shows agent logo, index badge, and star for primary; right-click for lifecycle actions. @@ -222,6 +266,8 @@ export function SessionTab(props: IDockviewPanelHeaderProps) { if (agentLabel && api.title !== agentLabel) api.setTitle(agentLabel); }, [agentLabel, api]); + const showMultiSessionBadges = sessionCount > 1; + return ( <> @@ -230,10 +276,10 @@ export function SessionTab(props: IDockviewPanelHeaderProps) { data-testid={sessionId ? `session-tab-${sessionId}` : undefined} >
- {isPrimary && ( + {isPrimary && showMultiSessionBadges && ( )} - {sessionNumber != null && ( + {sessionNumber != null && showMultiSessionBadges && ( {sessionNumber} @@ -248,38 +294,12 @@ export function SessionTab(props: IDockviewPanelHeaderProps) {
- - - Set as Primary - - - {sessionState && isStoppable(sessionState) && ( - - Stop - - )} - {sessionState && isResumable(sessionState) && ( - - Resume - - )} - {sessionState && isDeletable(sessionState) && ( - setConfirmDelete(true)} - > - Delete - - )} - - - Close Others - - + setConfirmDelete(true)} + /> { - const taskRow = this.sidebar.locator('[role="button"]').filter({ hasText: title }); - await taskRow.hover(); - await taskRow.getByRole("button", { name: "Task actions" }).click(); - await this.page.getByRole("menuitem", { name: "Delete" }).click(); + await this.openSidebarMenuAndClick(title, "Delete"); } /** @@ -241,10 +238,38 @@ export class SessionPage { * Hovers to reveal the menu trigger, opens it, and clicks "Archive". */ async archiveTaskInSidebar(title: string): Promise { + await this.openSidebarMenuAndClick(title, "Archive"); + } + + /** + * Open a sidebar task's dropdown menu and click an item. + * Retries the full open-click sequence if the menu gets detached by a + * React re-render (e.g. WS-driven sidebar update) between open and click. + */ + private async openSidebarMenuAndClick( + title: string, + itemName: string, + retries = 3, + ): Promise { const taskRow = this.sidebar.locator('[role="button"]').filter({ hasText: title }); + for (let attempt = 0; attempt < retries; attempt++) { + try { + await taskRow.hover(); + await taskRow.getByRole("button", { name: "Task actions" }).click(); + const menuItem = this.page.getByRole("menuitem", { name: itemName }); + await menuItem.waitFor({ state: "visible", timeout: 3_000 }); + await menuItem.click({ timeout: 3_000 }); + return; + } catch { + // Menu was likely detached by a re-render — dismiss and retry + await this.page.keyboard.press("Escape"); + await this.page.waitForTimeout(500); + } + } + // Final attempt without catch await taskRow.hover(); await taskRow.getByRole("button", { name: "Task actions" }).click(); - await this.page.getByRole("menuitem", { name: "Archive" }).click(); + await this.page.getByRole("menuitem", { name: itemName }).click(); } stepperStep(name: string): Locator { diff --git a/apps/web/e2e/tests/diff-update.spec.ts b/apps/web/e2e/tests/diff-update.spec.ts index d6ff4ea8f7..41ffce61bb 100644 --- a/apps/web/e2e/tests/diff-update.spec.ts +++ b/apps/web/e2e/tests/diff-update.spec.ts @@ -122,7 +122,7 @@ test.describe("Diff update on file change", () => { }); // Click on the session tab to make the chat input visible again - await session.sessionTabByText("1").click(); + await session.clickSessionChatTab(); // Send another message to trigger the second modification await session.sendMessage("/e2e:diff-update-modify"); @@ -180,7 +180,7 @@ test.describe("Untracked file diff update", () => { }); // Click on the session tab to make the chat input visible again - await session.sessionTabByText("1").click(); + await session.clickSessionChatTab(); // Send another message to trigger the modification await session.sendMessage("/e2e:untracked-file-modify"); diff --git a/apps/web/e2e/tests/session-tab-cleanup.spec.ts b/apps/web/e2e/tests/session-tab-cleanup.spec.ts new file mode 100644 index 0000000000..6f3f01b292 --- /dev/null +++ b/apps/web/e2e/tests/session-tab-cleanup.spec.ts @@ -0,0 +1,81 @@ +import { test, expect } from "../fixtures/test-base"; +import { KanbanPage } from "../pages/kanban-page"; +import { SessionPage } from "../pages/session-page"; + +const DONE_STATES = ["COMPLETED", "WAITING_FOR_INPUT"]; + +async function createTaskAndNavigate( + testPage: import("@playwright/test").Page, + apiClient: import("../helpers/api-client").ApiClient, + seedData: import("../fixtures/test-base").SeedData, + title: string, +) { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + title, + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + + await expect + .poll( + async () => { + const { sessions } = await apiClient.listTaskSessions(task.id); + return DONE_STATES.includes(sessions[0]?.state ?? ""); + }, + { timeout: 30_000, message: "Waiting for session to finish" }, + ) + .toBe(true); + + const kanban = new KanbanPage(testPage); + await kanban.goto(); + const card = kanban.taskCardByTitle(title); + await expect(card).toBeVisible({ timeout: 10_000 }); + await card.click(); + await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); + + const session = new SessionPage(testPage); + await session.waitForLoad(); + await expect(session.chat.getByText("simple mock response", { exact: false })).toBeVisible({ + timeout: 15_000, + }); + + return { task, session }; +} + +test.describe("Session tab cleanup", () => { + test("single-session task shows only named agent tab without star or number", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(90_000); + + await createTaskAndNavigate(testPage, apiClient, seedData, "Single Session Tab Task"); + + // Should have exactly one session tab (the named agent tab) + const sessionTabs = testPage.locator("[data-testid^='session-tab-']"); + await expect(sessionTabs).toHaveCount(1, { timeout: 10_000 }); + + // The generic "Agent" permanent tab should NOT exist + // (it uses permanentTab tabComponent which has no data-testid, identified by text) + const permanentAgentTab = testPage.locator( + ".dv-default-tab:not(:has([data-testid^='session-tab-'])):has-text('Agent')", + ); + await expect(permanentAgentTab).toHaveCount(0); + + // The single session tab should NOT show a star icon + const star = sessionTabs.first().locator(".tabler-icon-star"); + await expect(star).toHaveCount(0); + + // The single session tab should NOT show a number badge + // (number badges are small spans with bg-foreground/10 containing a digit) + const numberBadge = sessionTabs.first().locator("span.rounded"); + await expect(numberBadge).toHaveCount(0); + }); +}); diff --git a/apps/web/lib/state/layout-manager/merger.test.ts b/apps/web/lib/state/layout-manager/merger.test.ts new file mode 100644 index 0000000000..e213e0a22e --- /dev/null +++ b/apps/web/lib/state/layout-manager/merger.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect } from "vitest"; +import type { LayoutState } from "./types"; +import { mergePanelsIntoPreset } from "./merger"; + +function makeLayout(centerPanels: Array<{ id: string; component: string }>): LayoutState { + return { + columns: [ + { + id: "sidebar", + groups: [{ panels: [{ id: "sidebar", component: "sidebar", title: "Sidebar" }] }], + }, + { + id: "center", + groups: [ + { + panels: centerPanels.map((p) => ({ ...p, title: p.id })), + }, + ], + }, + ], + }; +} + +describe("mergePanelsIntoPreset", () => { + it("replaces chat panel with session panels when session panels exist", () => { + const currentState = makeLayout([{ id: "session:abc123", component: "chat" }]); + const targetPreset = makeLayout([{ id: "chat", component: "chat" }]); + + const result = mergePanelsIntoPreset(currentState, targetPreset); + + const centerPanels = result.columns.find((c) => c.id === "center")!.groups[0].panels; + const panelIds = centerPanels.map((p) => p.id); + + expect(panelIds).toContain("session:abc123"); + expect(panelIds).not.toContain("chat"); + }); + + it("preserves chat panel when no session panels exist", () => { + const currentState = makeLayout([{ id: "chat", component: "chat" }]); + const targetPreset = makeLayout([{ id: "chat", component: "chat" }]); + + const result = mergePanelsIntoPreset(currentState, targetPreset); + + const centerPanels = result.columns.find((c) => c.id === "center")!.groups[0].panels; + const panelIds = centerPanels.map((p) => p.id); + + expect(panelIds).toContain("chat"); + }); + + it("preserves multiple session panels and drops chat", () => { + const currentState = makeLayout([ + { id: "session:abc", component: "chat" }, + { id: "session:def", component: "chat" }, + ]); + const targetPreset = makeLayout([{ id: "chat", component: "chat" }]); + + const result = mergePanelsIntoPreset(currentState, targetPreset); + + const centerPanels = result.columns.find((c) => c.id === "center")!.groups[0].panels; + const panelIds = centerPanels.map((p) => p.id); + + expect(panelIds).toContain("session:abc"); + expect(panelIds).toContain("session:def"); + expect(panelIds).not.toContain("chat"); + }); +}); diff --git a/apps/web/lib/state/layout-manager/merger.ts b/apps/web/lib/state/layout-manager/merger.ts index 4306e47a5c..ee44fe9a01 100644 --- a/apps/web/lib/state/layout-manager/merger.ts +++ b/apps/web/lib/state/layout-manager/merger.ts @@ -24,17 +24,14 @@ function collectPanelIds(state: LayoutState): Set { } /** - * Merge current live panels into a target preset layout. - * - * Captures panels from the current dockview state that aren't in the target - * preset and appends them as tabs in the center group. This prevents panels - * from being lost when switching layouts. + * Pure merge logic: merge extra panels from the current state into a target + * preset layout. Session panels (`session:*`) replace the generic `chat` + * panel rather than coexisting alongside it. */ -export function mergeCurrentPanelsIntoPreset( - api: DockviewApi, +export function mergePanelsIntoPreset( + currentState: LayoutState, targetPreset: LayoutState, ): LayoutState { - const currentState = fromDockviewApi(api); const currentPanels = collectAllPanels(currentState); const targetPanelIds = collectPanelIds(targetPreset); @@ -46,6 +43,8 @@ export function mergeCurrentPanelsIntoPreset( return targetPreset; } + const hasSessionPanels = extraPanels.some((p) => p.id.startsWith("session:")); + console.debug( "[layout-merger] merging extra panels into preset:", extraPanels.map((p) => p.id), @@ -57,13 +56,19 @@ export function mergeCurrentPanelsIntoPreset( const groups = col.groups.map((group, idx) => { if (idx !== 0) return group; - const existingIds = new Set(group.panels.map((p) => p.id)); + // If extra panels include session tabs, drop the generic "chat" + // placeholder — session panels replace it. + const basePanels = hasSessionPanels + ? group.panels.filter((p) => p.id !== "chat") + : group.panels; + + const existingIds = new Set(basePanels.map((p) => p.id)); const toAdd = extraPanels.filter((p) => !existingIds.has(p.id)); - if (toAdd.length === 0) return group; + if (toAdd.length === 0 && basePanels.length === group.panels.length) return group; return { ...group, - panels: [...group.panels, ...toAdd], + panels: [...basePanels, ...toAdd], }; }); @@ -72,3 +77,17 @@ export function mergeCurrentPanelsIntoPreset( return { columns: mergedColumns }; } + +/** + * Merge current live panels into a target preset layout. + * + * Captures panels from the current dockview state that aren't in the target + * preset and appends them as tabs in the center group. This prevents panels + * from being lost when switching layouts. + */ +export function mergeCurrentPanelsIntoPreset( + api: DockviewApi, + targetPreset: LayoutState, +): LayoutState { + return mergePanelsIntoPreset(fromDockviewApi(api), targetPreset); +}