From b70e33764f06228c78cd64ff2b2f85083ee56e50 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 5 Aug 2026 19:40:06 -0700 Subject: [PATCH] Revert #12658: show PR status on active and done workspaces too (#12825) Stacked on the #12793 revert. Widens the passive-identity set from {inactive} back to {active, done, inactive}, so the PR/check glyph returns to the left status lane for workspaces that are actively being worked, not just idle ones. Tradeoff, deliberate: #12658 was not purely a regression. It also fixed #8813, where an active workspace with branch identity and no PR showed the grey branch glyph instead of the emerald Active dot. This revert reintroduces that, and removes its e2e guard. The left lane holds one glyph, so activity, branch identity, and review status cannot all be shown. This picks review status. --- .../sidebar/WorktreeCard.pr-display.test.tsx | 6 +- .../WorktreeCard.quick-actions.test.tsx | 17 +--- .../src/components/sidebar/WorktreeCard.tsx | 2 +- .../sidebar/WorktreeCardStatusSlot.test.tsx | 43 ++-------- .../sidebar/WorktreeCardStatusSlot.tsx | 10 ++- .../worktree-card-status-indicator.spec.ts | 84 ------------------- 6 files changed, 20 insertions(+), 142 deletions(-) delete mode 100644 tests/e2e/worktree-card-status-indicator.spec.ts diff --git a/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx b/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx index a219841b949..fef27ea71cc 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx @@ -60,7 +60,7 @@ vi.mock('@/components/ui/tooltip', () => ({ })) vi.mock('./use-worktree-activity-status', () => ({ - useWorktreeActivityStatus: () => 'inactive' + useWorktreeActivityStatus: () => 'active' })) vi.mock('./CacheTimer', () => ({ @@ -171,8 +171,8 @@ describe('WorktreeCard linked PR display', () => { ) - expect(markup).toContain('Inactive') - expect(markup).toContain('bg-neutral-500/40') + expect(markup).toContain('Active') + expect(markup).toContain('bg-emerald-500') expect(markup).not.toContain('PR: Open') expect(markup).not.toContain('Linked PR #456') }, 20_000) diff --git a/src/renderer/src/components/sidebar/WorktreeCard.quick-actions.test.tsx b/src/renderer/src/components/sidebar/WorktreeCard.quick-actions.test.tsx index 54322d38ffc..7456cce2faa 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.quick-actions.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.quick-actions.test.tsx @@ -21,7 +21,6 @@ let tabsByWorktree: Record = {} let ptyIdsByTabId: Record = {} let browserTabsByWorktree: Record = {} let settings: Partial | null = null -let activityStatus = 'idle' let projectGroups: unknown[] = [] let workspaceDeleteModifierPressed = false let gitConflictOperationByWorktree: Record = {} @@ -61,7 +60,7 @@ vi.mock('@/components/ui/tooltip', () => ({ })) vi.mock('./use-worktree-activity-status', () => ({ - useWorktreeActivityStatus: () => activityStatus + useWorktreeActivityStatus: () => 'idle' })) vi.mock('./CacheTimer', () => ({ @@ -145,7 +144,6 @@ describe('WorktreeCard quick actions', () => { projectGroups = [] workspaceDeleteModifierPressed = false gitConflictOperationByWorktree = {} - activityStatus = 'idle' }) it('marks the unread toggle as a workspace-board-preserving action', () => { @@ -254,19 +252,6 @@ describe('WorktreeCard quick actions', () => { expect(markup).not.toContain('lucide-git-branch') }) - it('keeps the quiet status dot when the branch card property is off', () => { - settings = { experimentalNewWorktreeCardStyle: true } - worktreeCardProperties = ['status'] - activityStatus = 'inactive' - - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('bg-neutral-500/40') - expect(markup).not.toContain('lucide-git-branch') - }) - it('does not render a pending first-agent rename title badge', () => { const markup = renderToStaticMarkup( ) : null} diff --git a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx index 8f204527c9a..8911c1783c5 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx @@ -192,7 +192,7 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).not.toContain('PR checks: Failed') }) - it('keeps the active dot ahead of PR status when new card style is on', () => { + it('uses PR status instead of the quiet active dot when new card style is on', () => { const markup = renderToStaticMarkup( { /> ) - expect(markup).toContain('Active') + expect(markup).toContain('PR checks: Failed') expect(markup).toContain('inline-flex size-5 items-center justify-center') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('PR checks: Failed') - }) - - it('keeps the emerald activity dot ahead of branch identity when new card style is on', () => { - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('Active') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('lucide-git-branch') + expect(markup).toContain('size-[13px] translate-x-px') + expect(markup).toContain('text-rose-500/85') + expect(markup).not.toContain('bg-emerald-500') }) it('uses the unified compact review glyph for GitLab MR status', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( { expect(markup).not.toContain('lucide-git-merge') }) - it('keeps the done dot ahead of PR status when new card style is on', () => { + it('uses PR status instead of the quiet done dot when new card style is on', () => { mocks.status = 'done' const markup = renderToStaticMarkup( { /> ) - expect(markup).toContain('Done') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('PR checks: Failed') + expect(markup).toContain('PR checks: Failed') + expect(markup).not.toContain('bg-emerald-500') }) it('uses PR status instead of the inactive dot when new card style is on', () => { @@ -298,8 +277,7 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).not.toContain('bg-neutral-500/40') }) - it('uses a branch icon with branch-only tooltip copy on quiet rows', () => { - mocks.status = 'inactive' + it('uses a branch icon with branch-only tooltip copy by default', () => { const markup = renderToStaticMarkup( { }) it('uses context-aware branch or folder path tooltip copy', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( { }) it('overlays an unread badge on PR status when new card style is on', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( { }) it('overlays an unread badge on the branch icon in new card style', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( (['inactive']) +const QUIET_REVIEW_REPLACEABLE_STATUSES = new Set(['active', 'done', 'inactive']) // Why: a missing review display can also mean provider state is unavailable, // so the passive label names the identity cue without claiming no review exists. function getDefaultBranchIdentityLabel(): string { @@ -104,13 +103,16 @@ export function WorktreeCardStatusSlot({ const status = useWorktreeActivityStatus(worktreeId) const statusLabel = getWorktreeStatusLabel(status) || status const canShowReviewStatus = - newCardStyle && showStatus && prDisplay !== null && IDENTITY_REPLACEABLE_STATUSES.has(status) + newCardStyle && + showStatus && + prDisplay !== null && + QUIET_REVIEW_REPLACEABLE_STATUSES.has(status) const canShowBranchStatus = newCardStyle && showStatus && hasBranchIdentity && prDisplay === null && - IDENTITY_REPLACEABLE_STATUSES.has(status) + QUIET_REVIEW_REPLACEABLE_STATUSES.has(status) const passiveStatusLabel = canShowReviewStatus && prDisplay ? getReviewStatusTooltip(prDisplay) diff --git a/tests/e2e/worktree-card-status-indicator.spec.ts b/tests/e2e/worktree-card-status-indicator.spec.ts deleted file mode 100644 index 78075cb26c2..00000000000 --- a/tests/e2e/worktree-card-status-indicator.spec.ts +++ /dev/null @@ -1,84 +0,0 @@ -// Regression coverage for active status taking precedence over passive identity (#8813). - -import type { Locator, Page } from '@stablyai/playwright-test' -import { test, expect } from './helpers/orca-app' -import { - waitForSessionReady, - waitForActiveWorktree, - getAllWorktreeIds, - ensureTerminalVisible -} from './helpers/store' -import { worktreeRow, worktreeRowSurface } from './worktree-row-locators' - -function statusLane(page: Page, worktreeId: string): Locator { - return worktreeRow(page, worktreeId).locator('[data-worktree-card-status-slot]').first() -} - -function statusDot(page: Page, worktreeId: string): Locator { - return statusLane(page, worktreeId).locator('span.bg-emerald-500').first() -} - -function branchIdentityGlyph(page: Page, worktreeId: string): Locator { - return statusLane(page, worktreeId).locator('svg.lucide-git-branch') -} - -// PTY liveness, not the tab row, drives the activity heuristic. -async function waitForLivePty(page: Page, worktreeId: string): Promise { - await expect - .poll( - async () => - page.evaluate((id) => { - const state = window.__store!.getState() - return (state.tabsByWorktree[id] ?? []).some( - (tab) => (state.ptyIdsByTabId[tab.id] ?? []).length > 0 - ) - }, worktreeId), - { timeout: 30_000, message: `No live PTY attached for worktree ${worktreeId}` } - ) - .toBe(true) -} - -test.describe('Worktree card status indicator', () => { - test.beforeEach(async ({ orcaPage }) => { - await waitForSessionReady(orcaPage) - await waitForActiveWorktree(orcaPage) - }) - - test('paints the emerald Active dot instead of the grey branch glyph once a workspace goes live', async ({ - orcaPage - }) => { - const liveWorktreeId = await waitForActiveWorktree(orcaPage) - await ensureTerminalVisible(orcaPage) - await waitForLivePty(orcaPage, liveWorktreeId) - - // The regression requires both the new card style and branch identity. - await orcaPage.evaluate(async () => { - const state = window.__store!.getState() - await state.updateSettings({ experimentalNewWorktreeCardStyle: true }) - state.setWorktreeCardProperties(['status', 'unread', 'branch']) - }) - - const quietWorktreeId = (await getAllWorktreeIds(orcaPage)).find((id) => id !== liveWorktreeId) - if (!quietWorktreeId) { - throw new Error('Seeded repo did not expose a second worktree to keep quiet') - } - - await expect(branchIdentityGlyph(orcaPage, quietWorktreeId)).toBeVisible() - await expect(statusLane(orcaPage, quietWorktreeId)).toHaveText('Branch') - - await expect(statusDot(orcaPage, liveWorktreeId)).toBeVisible() - await expect(statusLane(orcaPage, liveWorktreeId)).toHaveText('Active') - await expect(branchIdentityGlyph(orcaPage, liveWorktreeId)).toHaveCount(0) - - await worktreeRowSurface(orcaPage, quietWorktreeId).click() - await expect - .poll(async () => orcaPage.evaluate(() => window.__store!.getState().activeWorktreeId)) - .toBe(quietWorktreeId) - await ensureTerminalVisible(orcaPage) - await waitForLivePty(orcaPage, quietWorktreeId) - - await expect(statusDot(orcaPage, quietWorktreeId)).toBeVisible() - await expect(statusLane(orcaPage, quietWorktreeId)).toHaveText('Active') - await expect(branchIdentityGlyph(orcaPage, quietWorktreeId)).toHaveCount(0) - }) -})