From a77df053a643db06c80cff64bf27bf0c30aba7b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Mon, 4 May 2026 00:53:21 +0100 Subject: [PATCH] refactor(web): drop redundant VCS split button from task top bar (#804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(web): drop redundant VCS split button from task top bar All git operations (commit, push, pull, rebase, merge, create PR, force push) are reachable from the Changes panel — its timeline exposes stage/commit and the per-section action row provides push/force-push/create-PR, while the panel header carries the pull/rebase/merge dropdown. The session command palette mirrors the same set for keyboard users. The expanded review dialog keeps its own VcsSplitButton because the changes timeline is hidden behind the modal there. Also removes the now-orphaned 'required' overflow flag (the VCS item was its only caller) and the e2e helper + spec that targeted the removed primary-action button. * test(e2e): drop task-topbar-overflow spec relying on removed VCS button The 1100px viewport test forced overflow by counting on the VCS button's width; with VcsSplitButton gone the scenario no longer holds. Overflow priority logic is already covered by topbar-action-overflow.test.ts. Co-authored-by: João Salavisa --------- Co-authored-by: Kandev Agent Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: João Salavisa --- apps/web/components/task/task-top-bar.tsx | 24 +- .../task/topbar-action-overflow.test.ts | 19 +- .../task/topbar-action-overflow.tsx | 7 +- apps/web/e2e/pages/session-page.ts | 8 - .../e2e/tests/pr/vcs-button-context.spec.ts | 277 ------------------ .../tests/task/task-topbar-overflow.spec.ts | 79 ----- 6 files changed, 5 insertions(+), 409 deletions(-) delete mode 100644 apps/web/e2e/tests/pr/vcs-button-context.spec.ts delete mode 100644 apps/web/e2e/tests/task/task-topbar-overflow.spec.ts diff --git a/apps/web/components/task/task-top-bar.tsx b/apps/web/components/task/task-top-bar.tsx index a830e8aa17..d5d627039d 100644 --- a/apps/web/components/task/task-top-bar.tsx +++ b/apps/web/components/task/task-top-bar.tsx @@ -26,7 +26,6 @@ import { EditorsMenu } from "@/components/task/editors-menu"; import { BranchPathPopover } from "@/components/task/branch-path-popover"; import { LayoutPresetSelector } from "@/components/task/layout-preset-selector"; import { DocumentControls } from "@/components/task/document/document-controls"; -import { VcsSplitButton } from "@/components/vcs-split-button"; import { PRTopbarButton } from "@/components/github/pr-topbar-button"; import { JiraTicketButton, extractJiraKey } from "@/components/jira/jira-ticket-button"; import { JiraLinkButton } from "@/components/jira/jira-link-button"; @@ -179,7 +178,6 @@ const TaskTopBar = memo(function TaskTopBar({ void; isArchived?: boolean; @@ -526,24 +522,6 @@ function TopBarRight({ }); } - if (!isArchived) { - items.push({ - id: "vcs", - label: "Primary version control action", - priority: 100, - required: true, - content: ( - - - - ), - }); - } - items.push({ id: "attention", label: "Task status and attention", diff --git a/apps/web/components/task/topbar-action-overflow.test.ts b/apps/web/components/task/topbar-action-overflow.test.ts index 16fe17f073..d0436853a4 100644 --- a/apps/web/components/task/topbar-action-overflow.test.ts +++ b/apps/web/components/task/topbar-action-overflow.test.ts @@ -2,20 +2,17 @@ import { describe, expect, it } from "vitest"; import { getHiddenTopbarActionIds } from "./topbar-action-overflow"; const QUICK_CHAT = "quick-chat"; -const VCS = "vcs"; const ATTENTION = "attention"; const TOOLS = "tools"; const items = [ { id: QUICK_CHAT, priority: 20 }, - { id: VCS, priority: 100, required: true }, { id: ATTENTION, priority: 80 }, { id: TOOLS, priority: 10 }, ]; const widths = new Map([ [QUICK_CHAT, 88], - [VCS, 180], [ATTENTION, 160], [TOOLS, 120], ]); @@ -37,7 +34,7 @@ describe("getHiddenTopbarActionIds", () => { expect( getHiddenTopbarActionIds({ items, - availableWidth: 500, + availableWidth: 320, itemWidths: widths, gap: 8, overflowTriggerWidth: 40, @@ -49,23 +46,11 @@ describe("getHiddenTopbarActionIds", () => { expect( getHiddenTopbarActionIds({ items, - availableWidth: 410, + availableWidth: 230, itemWidths: widths, gap: 8, overflowTriggerWidth: 40, }), ).toEqual([QUICK_CHAT, TOOLS]); }); - - it("keeps required actions inline even when all secondary actions overflow", () => { - expect( - getHiddenTopbarActionIds({ - items, - availableWidth: 230, - itemWidths: widths, - gap: 8, - overflowTriggerWidth: 40, - }), - ).toEqual([QUICK_CHAT, ATTENTION, TOOLS]); - }); }); diff --git a/apps/web/components/task/topbar-action-overflow.tsx b/apps/web/components/task/topbar-action-overflow.tsx index bba16a59c6..52de62e6eb 100644 --- a/apps/web/components/task/topbar-action-overflow.tsx +++ b/apps/web/components/task/topbar-action-overflow.tsx @@ -11,11 +11,10 @@ export type TopbarOverflowItem = { id: string; label: string; priority: number; - required?: boolean; content: ReactNode; }; -type TopbarOverflowMetricItem = Pick; +type TopbarOverflowMetricItem = Pick; type HiddenTopbarActionArgs = { items: TopbarOverflowMetricItem[]; @@ -77,9 +76,7 @@ export function getHiddenTopbarActionIds({ fallbackItemWidth = DEFAULT_FALLBACK_ITEM_WIDTH, }: HiddenTopbarActionArgs): string[] { const hiddenIds = new Set(); - const hideableItems = [...items] - .filter((item) => !item.required) - .sort((a, b) => a.priority - b.priority); + const hideableItems = [...items].sort((a, b) => a.priority - b.priority); while ( totalActionWidth({ diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index f5ff20cba7..5af7bffaec 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -339,14 +339,6 @@ export class SessionPage { return this.page.getByTestId("pr-topbar-button"); } - /** VCS split button primary action. Pass action to match a specific state. */ - vcsPrimaryButton(action?: "commit" | "push" | "pr" | "rebase"): Locator { - if (action) { - return this.page.getByTestId(`vcs-primary-${action}`); - } - return this.page.locator('[data-testid^="vcs-primary-"]'); - } - /** PR detail panel (auto-shown when task has an associated PR). */ prDetailPanel(): Locator { return this.page.getByTestId("pr-detail-panel"); diff --git a/apps/web/e2e/tests/pr/vcs-button-context.spec.ts b/apps/web/e2e/tests/pr/vcs-button-context.spec.ts deleted file mode 100644 index c8a05a52fa..0000000000 --- a/apps/web/e2e/tests/pr/vcs-button-context.spec.ts +++ /dev/null @@ -1,277 +0,0 @@ -import { test, expect } from "../../fixtures/test-base"; -import type { ApiClient } from "../../helpers/api-client"; -import { makeGitEnv } from "../../helpers/git-helper"; -import { KanbanPage } from "../../pages/kanban-page"; -import { SessionPage } from "../../pages/session-page"; -import type { Page } from "@playwright/test"; -import fs from "node:fs"; -import path from "node:path"; -import { execSync } from "node:child_process"; - -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -class GitHelper { - constructor( - private repoDir: string, - private env: NodeJS.ProcessEnv, - ) {} - - exec(cmd: string): string { - const lockPath = path.join(this.repoDir, ".git", "index.lock"); - for (let attempt = 0; attempt < 3; attempt++) { - if (fs.existsSync(lockPath)) fs.unlinkSync(lockPath); - try { - return execSync(cmd, { cwd: this.repoDir, env: this.env, encoding: "utf8" }); - } catch (err) { - const msg = (err as Error).message ?? ""; - if (msg.includes("index.lock") && attempt < 2) { - execSync("sleep 0.2"); - continue; - } - throw err; - } - } - throw new Error(`git exec failed after 3 attempts: ${cmd}`); - } - - createFile(name: string, content: string) { - fs.writeFileSync(path.join(this.repoDir, name), content); - } - - /** Set up a bare remote so the local branch can track ahead/behind. */ - setupRemote(tmpDir: string): void { - const bareDir = path.join(tmpDir, "repos", "e2e-repo-bare.git"); - execSync(`git clone --bare "${this.repoDir}" "${bareDir}"`, { env: this.env }); - this.exec(`git remote add origin "${bareDir}"`); - this.exec("git fetch origin"); - this.exec("git branch --set-upstream-to=origin/main main"); - } - - commitFile(name: string, content: string, message: string): void { - this.createFile(name, content); - this.exec("git add -A"); - this.exec(`git commit -m "${message}"`); - } -} - -async function createStandardProfile(apiClient: ApiClient, name: string) { - const { agents } = await apiClient.listAgents(); - const agentId = agents[0]?.id; - if (!agentId) { - throw new Error(`E2E setup failed: no agent available for profile "${name}"`); - } - return apiClient.createAgentProfile(agentId, name, { - model: "mock-fast", - auto_approve: true, - cli_passthrough: false, - }); -} - -async function openTaskSession(page: Page, title: string): Promise { - const kanban = new KanbanPage(page); - await kanban.goto(); - - const card = kanban.taskCardByTitle(title); - await expect(card).toBeVisible({ timeout: 15_000 }); - await card.click(); - await expect(page).toHaveURL(/\/t\//, { timeout: 15_000 }); - - const session = new SessionPage(page); - await session.waitForLoad(); - return session; -} - -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - -test.describe("VCS split button context", () => { - test("shows Push when PR exists and commits are ahead", async ({ - testPage, - apiClient, - seedData, - backend, - }) => { - test.setTimeout(120_000); - - const profile = await createStandardProfile(apiClient, "VCS Push Profile"); - - const task = await apiClient.createTaskWithAgent( - seedData.workspaceId, - "VCS Push Button Test", - profile.id, - { - description: "/e2e:simple-message", - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repository_ids: [seedData.repositoryId], - }, - ); - - const session = await openTaskSession(testPage, "VCS Push Button Test"); - await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); - - // Set up git with a remote so we can get "ahead" status - const repoDir = path.join(backend.tmpDir, "repos", "e2e-repo"); - const gitEnv = makeGitEnv(backend.tmpDir); - const git = new GitHelper(repoDir, gitEnv); - git.setupRemote(backend.tmpDir); - - // Make a local commit → now 1 ahead of origin/main - git.commitFile("push-test.txt", "push content", "feat: push test commit"); - - // Before associating a PR, the button should show "Create PR" - // (ahead > 0, no PR) - await expect(session.vcsPrimaryButton("pr")).toBeVisible({ timeout: 30_000 }); - - // Now associate an open PR with the task - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: task.id, - owner: "testorg", - repo: "testrepo", - pr_number: 50, - pr_url: "https://github.com/testorg/testrepo/pull/50", - pr_title: "Feature branch", - head_branch: "main", - base_branch: "develop", - author_login: "test-user", - additions: 10, - deletions: 2, - }); - - // Wait for the PR topbar button to appear (confirms PR is in store) - await expect(session.prTopbarButton()).toBeVisible({ timeout: 30_000 }); - - // The VCS button should now show "Push" instead of "Create PR" - await expect(session.vcsPrimaryButton("push")).toBeVisible({ timeout: 15_000 }); - await expect(session.vcsPrimaryButton("push")).toContainText("Push"); - }); - - test("shows Create PR when ahead but no PR exists", async ({ - testPage, - apiClient, - seedData, - backend, - }) => { - test.setTimeout(120_000); - - const profile = await createStandardProfile(apiClient, "VCS PR Profile"); - - await apiClient.createTaskWithAgent(seedData.workspaceId, "VCS PR Button Test", profile.id, { - description: "/e2e:simple-message", - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repository_ids: [seedData.repositoryId], - }); - - const session = await openTaskSession(testPage, "VCS PR Button Test"); - await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); - - // Set up git with a remote - const repoDir = path.join(backend.tmpDir, "repos", "e2e-repo"); - const gitEnv = makeGitEnv(backend.tmpDir); - const git = new GitHelper(repoDir, gitEnv); - - // Only set up remote if not already configured - try { - git.exec("git remote get-url origin"); - } catch { - git.setupRemote(backend.tmpDir); - } - - // Push current state to origin, then make a new commit to be ahead - git.exec("git push origin main"); - git.commitFile("pr-test.txt", "pr content", "feat: pr test commit"); - - // No PR associated → should show "Create PR" - await expect(session.vcsPrimaryButton("pr")).toBeVisible({ timeout: 30_000 }); - await expect(session.vcsPrimaryButton("pr")).toContainText("Create PR"); - }); - - test("shows Commit when there are uncommitted files even with open PR", async ({ - testPage, - apiClient, - seedData, - backend, - }) => { - test.setTimeout(120_000); - - const profile = await createStandardProfile(apiClient, "VCS Commit Profile"); - - const task = await apiClient.createTaskWithAgent( - seedData.workspaceId, - "VCS Commit Priority Test", - profile.id, - { - description: "/e2e:simple-message", - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repository_ids: [seedData.repositoryId], - }, - ); - - const session = await openTaskSession(testPage, "VCS Commit Priority Test"); - await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); - - // Associate an open PR - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: task.id, - owner: "testorg", - repo: "testrepo", - pr_number: 55, - pr_url: "https://github.com/testorg/testrepo/pull/55", - pr_title: "Commit Priority PR", - head_branch: "main", - base_branch: "develop", - author_login: "test-user", - additions: 5, - deletions: 1, - }); - - await expect(session.prTopbarButton()).toBeVisible({ timeout: 30_000 }); - - // Create an uncommitted file - const repoDir = path.join(backend.tmpDir, "repos", "e2e-repo"); - const git = new GitHelper(repoDir, makeGitEnv(backend.tmpDir)); - git.createFile("uncommitted.txt", "uncommitted content"); - - // Even with open PR, uncommitted files → "Commit" takes priority - await expect(session.vcsPrimaryButton("commit")).toBeVisible({ timeout: 30_000 }); - await expect(session.vcsPrimaryButton("commit")).toContainText("Commit"); - }); - - test("defaults to Commit when no changes and no PR", async ({ - testPage, - apiClient, - seedData, - }) => { - test.setTimeout(120_000); - - const profile = await createStandardProfile(apiClient, "VCS Default Profile"); - - await apiClient.createTaskWithAgent( - seedData.workspaceId, - "VCS Default Button Test", - profile.id, - { - description: "/e2e:simple-message", - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repository_ids: [seedData.repositoryId], - }, - ); - - const session = await openTaskSession(testPage, "VCS Default Button Test"); - await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); - - // No changes, no PR → default is "Commit" - await expect(session.vcsPrimaryButton("commit")).toBeVisible({ timeout: 15_000 }); - await expect(session.vcsPrimaryButton("commit")).toContainText("Commit"); - }); -}); diff --git a/apps/web/e2e/tests/task/task-topbar-overflow.spec.ts b/apps/web/e2e/tests/task/task-topbar-overflow.spec.ts deleted file mode 100644 index ea533bd410..0000000000 --- a/apps/web/e2e/tests/task/task-topbar-overflow.spec.ts +++ /dev/null @@ -1,79 +0,0 @@ -import { test, expect } from "../../fixtures/test-base"; -import type { Page } from "@playwright/test"; - -type TopbarMetrics = { - workflowRight: number; - actionsLeft: number; - actionsRight: number; - headerRight: number; -}; - -async function readTopbarMetrics(page: Page) { - return page.evaluate(() => { - const header = document.querySelector('[data-testid="task-topbar"]'); - const workflow = document.querySelector('[data-testid="workflow-stepper"]'); - const actions = document.querySelector('[data-testid="topbar-action-overflow"]'); - if (!header || !workflow || !actions) return null; - - const headerRect = header.getBoundingClientRect(); - const workflowRect = workflow.getBoundingClientRect(); - const actionsRect = actions.getBoundingClientRect(); - - return { - workflowRight: workflowRect.right, - actionsLeft: actionsRect.left, - actionsRight: actionsRect.right, - headerRight: headerRect.right, - }; - }); -} - -test.describe("Task topbar overflow", () => { - test("keeps contextual PR actions inline before quick chat overflows", async ({ - testPage, - apiClient, - seedData, - }) => { - await testPage.setViewportSize({ width: 1100, height: 720 }); - - const task = await apiClient.createTask( - seedData.workspaceId, - "Responsive topbar PR review task", - { - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repository_ids: [seedData.repositoryId], - }, - ); - - await apiClient.mockGitHubAssociateTaskPR({ - task_id: task.id, - owner: "testorg", - repo: "testrepo", - pr_number: 761, - pr_url: "https://github.com/testorg/testrepo/pull/761", - pr_title: "Responsive topbar overflow", - head_branch: "feature/topbar-overflow", - base_branch: "main", - author_login: "test-user", - additions: 23, - deletions: 3, - }); - - await testPage.goto(`/t/${task.id}`); - - await expect(testPage.getByTestId("task-topbar")).toBeVisible({ timeout: 10_000 }); - await expect(testPage.getByTestId("pr-topbar-button")).toBeVisible({ timeout: 15_000 }); - await expect(testPage.getByTestId("topbar-action-overflow-trigger")).toBeVisible({ - timeout: 10_000, - }); - - const metrics = await readTopbarMetrics(testPage); - expect(metrics).not.toBeNull(); - expect(metrics!.workflowRight).toBeLessThanOrEqual(metrics!.actionsLeft + 1); - expect(metrics!.actionsRight).toBeLessThanOrEqual(metrics!.headerRight + 1); - - await testPage.getByTestId("topbar-action-overflow-trigger").click(); - await expect(testPage.getByRole("button", { name: "Chat" })).toBeVisible(); - }); -});