From 30fa6b43980ab33ddb119fc56aa1d9576445112e Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:49:04 -0800 Subject: [PATCH] fix: explain merged workspace cleanup and keep visible reviews fresh (#41200) * fix(github): keep the selected workspace's PR on its one-minute refresh Follow-up reads inherited the priority of whatever entry ran last. A workspace selected while its cached PR was still fresh never got a foreground entry, so its follow-ups competed with every visible card for the shared list budget and were read about every five minutes with 25 visible open PRs. Previously selected workspaces kept priority 80 forever. Follow-ups now take foreground priority only while their workspace is selected. * feat(source-control): say when a merged workspace is safe to delete Source Control's committed list is a merge-base diff against the base ref. A squash or rebase merge copies the commits onto the base with new IDs, so the list looks the same after the merge as before it. A notice now states whether the merged review contains everything: HEAD is the review's head (or the provider confirmed it is contained) and nothing is uncommitted. * fix(sidebar): keep PR status visible on sleeping workspaces The sleeping moon replaced the review glyph, hiding whether a sleeping workspace's PR was open, failing or merged. Review status now keeps the lane; the dimmed row already shows sleep and the accessible label names both. Co-authored-by: Tobias Diez Co-authored-by: alpenshtokk * fix(source-control): wait for a git status read before judging a merged workspace Before the first status read the change count is zero and HEAD may be stale, so the notice could call a dirty workspace safe to delete. Use the HEAD from the latest successful status read and say nothing until one completes. * chore(i18n): ship the merged-review plural copy in the runtime English catalog * fix: satisfy design-system, cast and flaky-test gates for the merged notice Use the callout-important token instead of raw purple, let Button own its type size, build the fixture worktree with the shared factory, pin the test clock and give the long simulated-time tests a reason for their timeout. * fix(github): align selected-refresh regression with feature ownership * refactor(github): give refresh queue aliases their concrete owner * fix(source-control): preserve cleanup warnings and reserve refresh fairness * fix(source-control): describe missing merged-head proof as unverified --------- Co-authored-by: Tobias Diez Co-authored-by: alpenshtokk --- .../pr-refresh-budget-fairness.test.ts | 73 +++++++++++ .../refresh/pr-refresh-candidate-policy.ts | 13 ++ .../refresh/pr-refresh-pacing.ts | 21 +++- .../refresh/pr-refresh-queue-aliases.ts | 47 +++++++ .../refresh/pr-refresh-queue-drainer.ts | 14 +-- .../pull-requests/refresh/pr-refresh-queue.ts | 58 ++------- .../refresh/pr-refresh-watch-jobs.ts | 5 +- ...merged-review-workspace-notice.fixture.tsx | 66 ++++++++++ .../merged-review-workspace-notice.test.tsx | 82 ++++++++++++ .../panel/merged-review-workspace-notice.tsx | 119 ++++++++++++++++++ .../source-control/panel/panel-content.tsx | 14 +++ .../deletion/delete-worktree-flow.test.ts | 14 +++ .../deletion/delete-worktree-flow.ts | 3 +- .../deletion/worktree-delete-request.ts | 1 + .../surface/WorktreeCardStatusSlot.tsx | 22 +++- .../src/i18n/en-runtime-required.json | 2 + src/renderer/src/i18n/locales/en.json | 5 + .../merged-review-workspace-readiness.test.ts | 73 +++++++++++ .../merged-review-workspace-readiness.ts | 33 +++++ 19 files changed, 599 insertions(+), 66 deletions(-) create mode 100644 src/main/github/pull-requests/refresh/pr-refresh-budget-fairness.test.ts create mode 100644 src/main/github/pull-requests/refresh/pr-refresh-queue-aliases.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.fixture.tsx create mode 100644 src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.test.tsx create mode 100644 src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.tsx create mode 100644 src/shared/source-control/decisions/merged-review-workspace-readiness.test.ts create mode 100644 src/shared/source-control/decisions/merged-review-workspace-readiness.ts diff --git a/src/main/github/pull-requests/refresh/pr-refresh-budget-fairness.test.ts b/src/main/github/pull-requests/refresh/pr-refresh-budget-fairness.test.ts new file mode 100644 index 00000000000..3f3880ab775 --- /dev/null +++ b/src/main/github/pull-requests/refresh/pr-refresh-budget-fairness.test.ts @@ -0,0 +1,73 @@ +import { expect, it } from 'vitest' +import { PRWatchJobRunner } from './pr-refresh-watch-jobs' +import { PRRefreshRetryState } from './pr-refresh-retry-state' +import { PRRefreshPacing } from './pr-refresh-pacing' +import { PRRefreshQueue, type PRRefreshQueueEntry } from './pr-refresh-queue' +import { makeCandidate } from './pr-refresh-coordinator-test-harness' + +it('yields a selected list read to another ready review before restoring its priority', () => { + const pacing = new PRRefreshPacing() + const queue = new PRRefreshQueue(() => {}) + const entry = (key: string, selected: boolean, priority: number): PRRefreshQueueEntry => ({ + key, + candidate: makeCandidate({ cacheKey: key, isSelected: selected }), + aliases: new Map(), + reason: 'visible', + priority, + dueAt: 0, + queuedAt: 0 + }) + const selected = entry('selected', true, 80) + const other = entry('other', false, 40) + queue.set(selected.key, selected) + queue.set(other.key, other) + const next = () => queue.ordered((a, b) => pacing.order(a, b))[0].key + expect(next()).toBe('selected') + pacing.noteBackgroundStart(selected) + expect(next()).toBe('other') + pacing.noteBackgroundStart(other) + expect(next()).toBe('selected') + pacing.noteBackgroundStart(selected) + queue.delete(other.key) + expect(next()).toBe('selected') +}) + +it('preserves a visible list turn when selected reads share the budget with aged watches', () => { + const pacing = new PRRefreshPacing() + const queue = new PRRefreshQueue(() => {}) + const watches = new PRWatchJobRunner(queue, new PRRefreshRetryState(), { schedule() {} }) + const entry = (key: string, selected: boolean): PRRefreshQueueEntry => ({ + key, + candidate: makeCandidate({ isSelected: selected }), + aliases: new Map(), + reason: 'visible', + priority: selected ? 80 : 40, + dueAt: 0, + queuedAt: 0 + }) + const selected = entry('selected', true) + const other = entry('other', false) + const watch: PRRefreshQueueEntry = { + ...entry('watch', false), + reason: 'watch', + watch: { prKey: 'pr' } + } + for (const item of [selected, other, watch]) { + queue.set(item.key, item) + } + const next = () => { + watches.age(queue.values(), 120_000) + return queue.ordered((a, b) => pacing.order(a, b))[0].key + } + const started = (item: PRRefreshQueueEntry) => { + pacing.noteBackgroundStart(item) + watches.noteBudgetedStart(item) + } + expect(next()).toBe('selected') + started(selected) + expect(next()).toBe('watch') + started(watch) + expect(next()).toBe('selected') + started(selected) + expect(next()).toBe('other') +}) diff --git a/src/main/github/pull-requests/refresh/pr-refresh-candidate-policy.ts b/src/main/github/pull-requests/refresh/pr-refresh-candidate-policy.ts index c846f032ada..de532e3d8ad 100644 --- a/src/main/github/pull-requests/refresh/pr-refresh-candidate-policy.ts +++ b/src/main/github/pull-requests/refresh/pr-refresh-candidate-policy.ts @@ -167,6 +167,19 @@ export function visibleCandidateAfterOutcome( } } +const SELECTED_VISIBLE_PRIORITY = 80 +const LIST_VISIBLE_PRIORITY = 40 + +/** The selected workspace's follow-up wins the next list-budget slot; a deselected one rejoins list order. */ +export function visibleFollowUpPriority( + candidate: GitHubPRRefreshCandidate, + priority: number +): number { + return candidate.isSelected === true + ? Math.max(priority, SELECTED_VISIBLE_PRIORITY) + : Math.min(priority, LIST_VISIBLE_PRIORITY) +} + export function hasStaleHead(candidate: GitHubPRRefreshCandidate): boolean { return ( candidate.currentHeadOid != null && diff --git a/src/main/github/pull-requests/refresh/pr-refresh-pacing.ts b/src/main/github/pull-requests/refresh/pr-refresh-pacing.ts index 1644d9c7f16..2013a5e05ae 100644 --- a/src/main/github/pull-requests/refresh/pr-refresh-pacing.ts +++ b/src/main/github/pull-requests/refresh/pr-refresh-pacing.ts @@ -19,6 +19,7 @@ export class PRRefreshPacing { private readonly backgroundStarts: number[] = [] private readonly activeStartsByScope = new Map() private lastBackgroundStartAt = 0 + private lastBudgetedStartWasSelected = false clearActiveBurstWindow(windowId: number): void { const windowPrefix = `${windowId}::` @@ -29,7 +30,9 @@ export class PRRefreshPacing { } } - noteBackgroundStart(): void { + noteBackgroundStart(entry: PRRefreshQueueEntry): void { + this.lastBudgetedStartWasSelected = + entry.reason === 'visible' && entry.candidate.isSelected === true const now = Date.now() this.lastBackgroundStartAt = now this.backgroundStarts.push(now) @@ -50,7 +53,11 @@ export class PRRefreshPacing { return Math.max(spacingDelay, windowDelay) } - activeOrder(a: PRRefreshQueueEntry, b: PRRefreshQueueEntry): number { + order(a: PRRefreshQueueEntry, b: PRRefreshQueueEntry): number { + const priority = this.priority(b) - this.priority(a) + if (priority !== 0) { + return priority + } if (!usesActiveRefreshPacing(a) || !usesActiveRefreshPacing(b)) { return 0 } @@ -60,6 +67,16 @@ export class PRRefreshPacing { return b.queuedAt - a.queuedAt } + private priority(entry: PRRefreshQueueEntry): number { + // Reserve the next budgeted slot for other ready reviews after a selected list read. + return this.lastBudgetedStartWasSelected && + entry.reason === 'visible' && + entry.candidate.isSelected === true && + entry.bypassBackgroundBudget !== true + ? 0 + : entry.priority + } + entryDelay(entry: PRRefreshQueueEntry): number { const activeDelay = usesActiveRefreshPacing(entry) ? this.nextActiveBurstDelay(entry) : 0 if (activeDelay > 0) { diff --git a/src/main/github/pull-requests/refresh/pr-refresh-queue-aliases.ts b/src/main/github/pull-requests/refresh/pr-refresh-queue-aliases.ts new file mode 100644 index 00000000000..52641c52914 --- /dev/null +++ b/src/main/github/pull-requests/refresh/pr-refresh-queue-aliases.ts @@ -0,0 +1,47 @@ +import type { GitHubPRRefreshAlias } from '../../../../shared/github/pull-requests/refresh/pull-request-refresh-types' + +/** Retire old-branch aliases when a linked-PR key survives worktree branch changes. */ +export function setLiveAlias( + aliases: Map, + alias: GitHubPRRefreshAlias +): void { + if (alias.worktreeId) { + for (const [cacheKey, existing] of aliases) { + if (cacheKey !== alias.cacheKey && existing.worktreeId === alias.worktreeId) { + aliases.delete(cacheKey) + } + } + } + aliases.set(alias.cacheKey, alias) +} + +/** Queued aliases are newer than request-time follow-ups; preserve them with their matching candidate. */ +export function mergeFollowUpAlias( + aliases: Map, + alias: GitHubPRRefreshAlias +): GitHubPRRefreshAlias | undefined { + if (alias.worktreeId) { + for (const existing of aliases.values()) { + if (existing.worktreeId === alias.worktreeId) { + return existing + } + } + } + setLiveAlias(aliases, alias) + return undefined +} + +/** Manual refresh writes copied maps back; keep the latest branch per worktree on re-entry too. */ +export function dropSupersededWorktreeAliases(aliases: Map): void { + const liveCacheKeyByWorktree = new Map() + for (const [cacheKey, alias] of aliases) { + if (!alias.worktreeId) { + continue + } + const superseded = liveCacheKeyByWorktree.get(alias.worktreeId) + if (superseded !== undefined) { + aliases.delete(superseded) + } + liveCacheKeyByWorktree.set(alias.worktreeId, cacheKey) + } +} diff --git a/src/main/github/pull-requests/refresh/pr-refresh-queue-drainer.ts b/src/main/github/pull-requests/refresh/pr-refresh-queue-drainer.ts index 9a4273da40b..17d3712655f 100644 --- a/src/main/github/pull-requests/refresh/pr-refresh-queue-drainer.ts +++ b/src/main/github/pull-requests/refresh/pr-refresh-queue-drainer.ts @@ -13,7 +13,8 @@ import { isMergeabilityPendingOutcome, sameAliasRequestIdentity, validateCandidate, - visibleCandidateAfterOutcome + visibleCandidateAfterOutcome, + visibleFollowUpPriority } from './pr-refresh-candidate-policy' import type { PRRefreshEventPublisher } from './pr-refresh-event-publisher' import { type PRRefreshPacing, usesActiveRefreshPacing } from './pr-refresh-pacing' @@ -33,7 +34,6 @@ export type PRRefreshRegistrationCheck = (candidate: GitHubPRRefreshCandidate) = export class PRRefreshQueueDrainer { private draining = false private timer: ReturnType | null = null - // Why: queued work can outlive its project; re-check on every removal path before Git runs. private registrationCheck: PRRefreshRegistrationCheck | null = null readonly watchJobs: PRWatchJobRunner @@ -52,9 +52,7 @@ export class PRRefreshQueueDrainer { } schedule(delay = 0): void { - if (this.timer) { - clearTimeout(this.timer) - } + clearTimeout(this.timer ?? undefined) this.timer = setTimeout(() => { this.timer = null // The drain serves every queued repo; a scheduling call inside a WSL user act must not lend it that act. @@ -125,7 +123,7 @@ export class PRRefreshQueueDrainer { candidate: followUpCandidate, aliases: new Map(aliases.map((alias) => [alias.cacheKey, alias])), reason: 'visible', - priority, + priority: visibleFollowUpPriority(followUpCandidate, priority), dueAt, queuedAt: this.queue.nextOrder(), bypassBackgroundBudget: pendingDueAt !== null, @@ -137,7 +135,7 @@ export class PRRefreshQueueDrainer { private ordered(): PRRefreshQueueEntry[] { this.watchJobs.age(this.queue.values(), Date.now()) - return this.queue.ordered((a, b) => this.pacing.activeOrder(a, b)) + return this.queue.ordered((a, b) => this.pacing.order(a, b)) } private nextQueuedWakeDelay(excludedKey: string): number | null { @@ -237,7 +235,7 @@ export class PRRefreshQueueDrainer { continue } if (next.bypassBackgroundBudget !== true && isBudgetedBackground(next.reason)) { - this.pacing.noteBackgroundStart() + this.pacing.noteBackgroundStart(next) this.watchJobs.noteBudgetedStart(next) } if (usesActiveRefreshPacing(next)) { diff --git a/src/main/github/pull-requests/refresh/pr-refresh-queue.ts b/src/main/github/pull-requests/refresh/pr-refresh-queue.ts index 2c315cc9d00..00caf6e68ae 100644 --- a/src/main/github/pull-requests/refresh/pr-refresh-queue.ts +++ b/src/main/github/pull-requests/refresh/pr-refresh-queue.ts @@ -12,7 +12,8 @@ import { refreshKey, sameAliasRequestIdentity, refreshIntervalForCandidate, - shouldSkipFresh + shouldSkipFresh, + visibleFollowUpPriority } from './pr-refresh-candidate-policy' export type PRRefreshQueueEntry = { @@ -40,51 +41,11 @@ export type PRRefreshEnqueue = { type PRRefreshRequestState = { until: number; requestSequence?: number } -/** Retire old-branch aliases when a linked-PR key survives worktree branch changes. */ -function setLiveAlias( - aliases: Map, - alias: GitHubPRRefreshAlias -): void { - if (alias.worktreeId) { - for (const [cacheKey, existing] of aliases) { - if (cacheKey !== alias.cacheKey && existing.worktreeId === alias.worktreeId) { - aliases.delete(cacheKey) - } - } - } - aliases.set(alias.cacheKey, alias) -} - -/** Queued aliases are newer than request-time follow-ups; preserve them with their matching candidate. */ -function mergeFollowUpAlias( - aliases: Map, - alias: GitHubPRRefreshAlias -): GitHubPRRefreshAlias | undefined { - if (alias.worktreeId) { - for (const existing of aliases.values()) { - if (existing.worktreeId === alias.worktreeId) { - return existing - } - } - } - setLiveAlias(aliases, alias) - return undefined -} - -/** Manual refresh writes copied maps back; keep the latest branch per worktree on re-entry too. */ -function dropSupersededWorktreeAliases(aliases: Map): void { - const liveCacheKeyByWorktree = new Map() - for (const [cacheKey, alias] of aliases) { - if (!alias.worktreeId) { - continue - } - const superseded = liveCacheKeyByWorktree.get(alias.worktreeId) - if (superseded !== undefined) { - aliases.delete(superseded) - } - liveCacheKeyByWorktree.set(alias.worktreeId, cacheKey) - } -} +import { + setLiveAlias, + mergeFollowUpAlias, + dropSupersededWorktreeAliases +} from './pr-refresh-queue-aliases' export class PRRefreshQueue { private readonly entries = new Map() @@ -155,6 +116,7 @@ export class PRRefreshQueue { continue } entry.candidate = candidateFor(entry.key, entry.candidate) + entry.priority = visibleFollowUpPriority(entry.candidate, entry.priority) entry.dueAt = Math.max( freshRetryAt(entry.candidate) ?? Date.now(), this.backgroundNotBefore.get(entry.key)?.until ?? 0 @@ -313,14 +275,14 @@ export class PRRefreshQueue { } ordered( - activeOrder: (a: PRRefreshQueueEntry, b: PRRefreshQueueEntry) => number + pacingOrder: (a: PRRefreshQueueEntry, b: PRRefreshQueueEntry) => number ): PRRefreshQueueEntry[] { const now = Date.now() return Array.from(this.entries.values()).sort((a, b) => { const aReady = a.dueAt <= now const bReady = b.dueAt <= now if (aReady && bReady) { - return b.priority - a.priority || activeOrder(a, b) || a.dueAt - b.dueAt + return pacingOrder(a, b) || b.priority - a.priority || a.dueAt - b.dueAt } if (aReady !== bReady) { return aReady ? -1 : 1 diff --git a/src/main/github/pull-requests/refresh/pr-refresh-watch-jobs.ts b/src/main/github/pull-requests/refresh/pr-refresh-watch-jobs.ts index f51c5f4ea79..9551d0e28a0 100644 --- a/src/main/github/pull-requests/refresh/pr-refresh-watch-jobs.ts +++ b/src/main/github/pull-requests/refresh/pr-refresh-watch-jobs.ts @@ -103,7 +103,10 @@ export class PRWatchJobRunner { } noteBudgetedStart(entry: PRRefreshQueueEntry): void { - this.lastBudgetedStartWasWatch = entry.watch !== undefined + // Selected list reads have their own reserved turn; preserve the watch/list alternation. + if (entry.reason !== 'visible' || entry.candidate.isSelected !== true) { + this.lastBudgetedStartWasWatch = entry.watch !== undefined + } } /** Re-ranks watch entries before each pick, so aging needs no change to the queue's order. */ diff --git a/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.fixture.tsx b/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.fixture.tsx new file mode 100644 index 00000000000..d401170403e --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.fixture.tsx @@ -0,0 +1,66 @@ +import { defineFixture } from '../../../../component-fixtures/component-fixture' +import type { HostedReviewInfo } from '../../../../../../shared/source-control/hosted-review/hosted-review-contracts' +import { makeWorktree } from '../../../worktree-jump/worktree-jump-palette-test-fixtures' +import { SourceControlMergedReviewNotice } from './merged-review-workspace-notice' + +const review: HostedReviewInfo = { + provider: 'github', + number: 412, + title: 'Add retry to sync', + state: 'merged', + url: 'https://github.com/acme/app/pull/412', + status: 'success', + updatedAt: '2026-11-10T00:00:00Z', + mergeable: 'UNKNOWN', + headSha: 'a1b2c3d' +} + +const worktree = makeWorktree('repo-1::/work/app-sync', 'app-sync', { + branch: 'refs/heads/feature/sync-retry' +}) + +function Frame({ children }: { children: React.ReactNode }) { + return
{children}
+} + +export const safeToDelete = defineFixture({ + width: 300, + render: () => ( + + + + ) +}) + +export const uncommittedChanges = defineFixture({ + width: 300, + render: () => ( + + + + ) +}) + +export const headMovedPastReview = defineFixture({ + width: 300, + render: () => ( + + + + ) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.test.tsx b/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.test.tsx new file mode 100644 index 00000000000..84de080ec37 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.test.tsx @@ -0,0 +1,82 @@ +// @vitest-environment happy-dom +import { act } from 'react' +import { createRoot } from 'react-dom/client' +import { expect, it, vi } from 'vitest' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { runProcess } from '@orca/process-host' + +const mocks = vi.hoisted(() => ({ runWorktreeDelete: vi.fn() })) +vi.mock('../../../sidebar/workspace-actions/deletion/delete-worktree-flow', () => mocks) +vi.mock('@/store/index', () => ({ + useAppStore: (selector: (state: { deleteStateByWorktreeId: object }) => unknown) => + selector({ deleteStateByWorktreeId: {} }) +})) + +import { SourceControlMergedReviewNotice } from './merged-review-workspace-notice' +import { makeWorktree } from '../../../worktree-jump/worktree-jump-palette-test-fixtures' + +it('requires the ignored-file confirmation for an ignored-only merged checkout', async () => { + const directory = await mkdtemp(path.join(tmpdir(), 'merged-notice-')) + const container = document.createElement('div') + const root = createRoot(container) + const git = async (args: string[]) => { + const result = await runProcess({ program: 'git', args, cwd: directory }) + if (result.code !== 0) { + throw new Error(result.stderr) + } + return result.stdout.trim() + } + try { + await git(['init', '--quiet']) + await writeFile(path.join(directory, '.gitignore'), '.env\n') + await git(['add', '.gitignore']) + await git([ + '-c', + 'user.name=Test', + '-c', + 'user.email=test@example.com', + 'commit', + '-m', + 'initial' + ]) + await writeFile(path.join(directory, '.env'), 'LOCAL_ONLY=value\n') + const head = await git(['rev-parse', 'HEAD']) + const status = await git(['status', '--porcelain']) + expect(status).toBe('') + const worktree = makeWorktree('ignored-only', 'Ignored only', { + path: directory, + instanceId: 'original' + }) + await act(async () => + root.render( + + ) + ) + await act(async () => container.querySelector('button')?.click()) + expect(mocks.runWorktreeDelete).toHaveBeenCalledWith('ignored-only', { + expectedHostId: undefined, + expectedInstanceId: 'original', + forceConfirm: true + }) + } finally { + await act(async () => root.unmount()) + await rm(directory, { recursive: true, force: true }) + } +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.tsx b/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.tsx new file mode 100644 index 00000000000..938a8dbdccf --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/panel/merged-review-workspace-notice.tsx @@ -0,0 +1,119 @@ +import React from 'react' +import { GitMerge, LoaderCircle, Trash2 } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { useAppStore } from '@/store/index' +import { useTranslate } from '@/i18n/use-translate' +import { + localizedHostedReviewCopy, + resolveSupportedHostedReviewCopyProvider +} from '@/i18n/hosted-review-localized-copy' +import type { HostedReviewInfo } from '../../../../../../shared/source-control/hosted-review/hosted-review-contracts' +import type { Worktree } from '../../../../../../shared/worktree/types' +import { resolveMergedReviewWorkspaceReadiness } from '../../../../../../shared/source-control/decisions/merged-review-workspace-readiness' +import { runWorktreeDelete } from '../../../sidebar/workspace-actions/deletion/delete-worktree-flow' +import { getDeleteStateForWorktreeHost } from '../../../sidebar/workspace-actions/deletion/worktree-delete-state-host-match' + +/** Says whether a merged workspace still holds work; the branch compare alone keeps squash-merged commits listed. */ +export function SourceControlMergedReviewNotice({ + review, + worktree, + statusHeadOid, + uncommittedCount +}: { + review: HostedReviewInfo | null + worktree: Worktree + /** HEAD from the latest successful git status read; null until one completes. */ + statusHeadOid: string | null + uncommittedCount: number +}): React.JSX.Element | null { + const t = useTranslate() + const isDeleting = useAppStore( + (s) => getDeleteStateForWorktreeHost(worktree, s.deleteStateByWorktreeId)?.isDeleting ?? false + ) + const readiness = resolveMergedReviewWorkspaceReadiness({ + review, + headOid: statusHeadOid, + uncommittedCount + }) + // Without a status read the change count is unknown, so say nothing rather than guess. + if (!review || !readiness || !statusHeadOid) { + return null + } + const label = `${ + localizedHostedReviewCopy(resolveSupportedHostedReviewCopyProvider(review.provider), t) + .shortLabel + } #${review.number}` + const canDelete = readiness.kind === 'safe-to-delete' && !worktree.isMainWorktree + + let message: string + if (readiness.kind === 'uncommitted-changes') { + message = t( + 'auto.components.right.sidebar.SourceControl.mergedReviewUncommitted', + '{{review}} is merged, but this workspace has {{count}} uncommitted changes that are not in it.', + { + review: label, + count: readiness.count, + defaultValue_one: + '{{review}} is merged, but this workspace has {{count}} uncommitted change that is not in it.' + } + ) + } else if (readiness.kind === 'head-unverified') { + message = t( + 'auto.components.right.sidebar.SourceControl.mergedReviewHeadMoved', + '{{review}} is merged, but Orca could not confirm that it includes this branch’s current commits.', + { review: label } + ) + } else if (worktree.isMainWorktree) { + message = t( + 'auto.components.right.sidebar.SourceControl.mergedReviewNothingLeft', + '{{review}} is merged and includes this branch’s committed changes.', + { review: label } + ) + } else { + message = t( + 'auto.components.right.sidebar.SourceControl.mergedReviewSafeToDelete', + '{{review}} is merged and includes this branch’s committed changes. Ignored files may still be lost when deleting this workspace.', + { review: label } + ) + } + + return ( +
+
+ ) +} diff --git a/src/renderer/src/components/right-sidebar/source-control/panel/panel-content.tsx b/src/renderer/src/components/right-sidebar/source-control/panel/panel-content.tsx index 122388ceffe..aef82b2e029 100644 --- a/src/renderer/src/components/right-sidebar/source-control/panel/panel-content.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/panel/panel-content.tsx @@ -8,6 +8,7 @@ import { SourceControlUncommittedSections } from '../listing/uncommitted-section import { CompareUnavailable } from '../sync/compare-summary' import { SourceControlCommitSurface } from './commit-surface' import { SourceControlForkPushNotice } from './fork-push-notice' +import { SourceControlMergedReviewNotice } from './merged-review-workspace-notice' import type { SourceControlPanelReadyProps } from './panel-props' import { useSourceControlScope } from './worktree-scope' import { openGitGraph } from '@/lib/source-control/graph/git-graph-tab' @@ -135,6 +136,19 @@ export function SourceControlPanelContent(props: SourceControlPanelReadyProps) { {/* Keep CommitArea through clean/mid-commit states (#1448); only active conflict operations replace the action surface. */} + {model.isFolder ? null : ( + + )} + {shouldRenderCommitArea( unresolvedConflicts.length, conflictOperation, diff --git a/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.test.ts b/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.test.ts index 179ab483b53..dc4e51ebbbb 100644 --- a/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.test.ts +++ b/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.test.ts @@ -108,6 +108,8 @@ vi.mock('./delete-worktree-failure-toast', () => ({ import { toast } from 'sonner' import { showDeleteWorktreeFailureToast } from './delete-worktree-failure-toast' +import { runWorktreeDelete } from './delete-worktree-flow' + import { runWorktreeDeletesInParallel } from './worktree-delete-execution' function setWorktrees( @@ -159,6 +161,18 @@ describe('delete worktree flow', () => { setWorktrees([]) }) + it('keeps the ignored-file confirmation available when a caller requires it despite skip-confirm', () => { + setWorktrees([{ id: 'ignored-only' }]) + mocks.state.settings.skipDeleteWorktreeConfirm = true + runWorktreeDelete('ignored-only', { forceConfirm: true }) + expect(mocks.state.openModal).toHaveBeenCalledWith('delete-worktree', { + worktreeId: 'ignored-only', + worktreeDeleteIdentities: [{ id: 'ignored-only', instanceId: 'ignored-only-instance' }], + allowSkipConfirm: false + }) + expect(mocks.state.removeWorktree).not.toHaveBeenCalled() + }) + it('revalidates each queued instance immediately before execution', async () => { // Model a host relying on renderer serialization for same-repo branch cleanup. setWorktrees([ diff --git a/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.ts b/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.ts index fffde41c774..df5298c931d 100644 --- a/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.ts +++ b/src/renderer/src/components/sidebar/workspace-actions/deletion/delete-worktree-flow.ts @@ -96,6 +96,7 @@ export function runWorktreeDelete(worktreeId: string, options: WorktreeDeleteOpt const hasLineageChildren = deleteLineage.descendants.length > 0 // Skip-confirm preferences never authorize discarding unsaved editor text. const skipConfirm = + options.forceConfirm !== true && (state.settings?.skipDeleteWorktreeConfirm ?? false) && collectRemovalUnsavedFiles(useAppStore.getState, [target]).length === 0 if (skipConfirm && !hasLineageChildren) { @@ -114,7 +115,7 @@ export function runWorktreeDelete(worktreeId: string, options: WorktreeDeleteOpt ]) } : {}), - ...(hasLineageChildren ? { allowSkipConfirm: false } : {}) + ...(hasLineageChildren || options.forceConfirm ? { allowSkipConfirm: false } : {}) }) } diff --git a/src/renderer/src/components/sidebar/workspace-actions/deletion/worktree-delete-request.ts b/src/renderer/src/components/sidebar/workspace-actions/deletion/worktree-delete-request.ts index 8a8da65caaf..04e926e6cc4 100644 --- a/src/renderer/src/components/sidebar/workspace-actions/deletion/worktree-delete-request.ts +++ b/src/renderer/src/components/sidebar/workspace-actions/deletion/worktree-delete-request.ts @@ -15,6 +15,7 @@ export type WorktreeBatchDeleteOptions = { export type WorktreeDeleteIdentity = Pick export type WorktreeDeleteOptions = { + forceConfirm?: boolean expectedInstanceId?: string /** Pin deletion to the confirmed host because workspace IDs repeat across hosts. */ expectedHostId?: ExecutionHostId diff --git a/src/renderer/src/components/sidebar/workspace-card/surface/WorktreeCardStatusSlot.tsx b/src/renderer/src/components/sidebar/workspace-card/surface/WorktreeCardStatusSlot.tsx index 5d2f1b671ca..a32b5015990 100644 --- a/src/renderer/src/components/sidebar/workspace-card/surface/WorktreeCardStatusSlot.tsx +++ b/src/renderer/src/components/sidebar/workspace-card/surface/WorktreeCardStatusSlot.tsx @@ -114,6 +114,9 @@ function getReviewStatusLabel(review: WorktreeCardPrDisplay, t: Translate = tran if (outcome === 'success') { return t('workspace.reviewStatus.checksPassing', '{{review}} checks: Passing', values) } + if (!review.state && !outcome) { + return getReviewLabel(review) + } return t('workspace.reviewStatus.open', '{{review}}: Open', values) } @@ -148,14 +151,17 @@ export function WorktreeCardStatusSlot({ const vmState = vmRuntime ? getEphemeralVmHostState(vmRuntime.status, vmWaking) : null const vmFailure = vmRuntime?.failure ?? null const statusLabel = getWorktreeStatusLabel(status) || status - // Sleeping remains distinct from quiet done/branch/review states; attention keeps its own glyph. - const canShowSleepingStatus = - newCardStyle && showStatus && isSleeping && QUIET_REVIEW_REPLACEABLE_STATUSES.has(status) const canShowReviewStatus = newCardStyle && showStatus && (prDisplay !== null || Boolean(collectionChecks)) && - !canShowSleepingStatus && + QUIET_REVIEW_REPLACEABLE_STATUSES.has(status) + // Review outranks sleep: the dimmed row already shows sleep, only the glyph can show the PR. + const canShowSleepingStatus = + newCardStyle && + showStatus && + isSleeping && + !canShowReviewStatus && QUIET_REVIEW_REPLACEABLE_STATUSES.has(status) const canShowBranchStatus = newCardStyle && @@ -178,10 +184,14 @@ export function WorktreeCardStatusSlot({ isReviewStale && canShowReviewStatus && (collectionChecks || prDisplay) ? `${passiveStatusLabel} · ${t('workspace.links.lastKnownStatus', 'Last known status')}` : passiveStatusLabel + const laneLabel = + canShowReviewStatus && isSleeping + ? `${reviewLabel} · ${getSleepingStatusLabel(t)}` + : reviewLabel const passiveStatusAnnouncement = newCardStyle && isUnread - ? t('workspace.activity.unreadLabel', '{{status}} · Unread', { status: reviewLabel }) - : reviewLabel + ? t('workspace.activity.unreadLabel', '{{status}} · Unread', { status: laneLabel }) + : laneLabel // Working/permission own the lane; retain unread in accessible copy and restore its dot afterward. const showNewCardUnreadAlert = newCardStyle && isUnread && showStatus && status !== 'working' && status !== 'permission' diff --git a/src/renderer/src/i18n/en-runtime-required.json b/src/renderer/src/i18n/en-runtime-required.json index 1ddc84b29ec..de2a463f586 100644 --- a/src/renderer/src/i18n/en-runtime-required.json +++ b/src/renderer/src/i18n/en-runtime-required.json @@ -429,6 +429,8 @@ "discardFailedSampleMore_other": "{{paths}}, +{{count}} more", "discardFailed_one": "Failed to discard {{count}} file", "discardFailed_other": "Failed to discard {{count}} files", + "mergedReviewUncommitted_one": "{{review}} is merged, but this workspace has {{count}} uncommitted change that is not in it.", + "mergedReviewUncommitted_other": "{{review}} is merged, but this workspace has {{count}} uncommitted changes that are not in it.", "noteCount_one": "{{count}} note", "noteCount_other": "{{count}} notes" }, diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 7bcaf318aee..dc1abe90ef6 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -14798,6 +14798,11 @@ "locallyResolvedConflictOfKind": "Conflict resolved locally ({{kind}})", "locallyResolvedConflict": "Conflict resolved locally", "pushesToForkTarget": "Pushes to fork {{target}}", + "mergedReviewUncommitted_one": "{{review}} is merged, but this workspace has {{count}} uncommitted change that is not in it.", + "mergedReviewUncommitted_other": "{{review}} is merged, but this workspace has {{count}} uncommitted changes that are not in it.", + "mergedReviewHeadMoved": "{{review}} is merged, but Orca could not confirm that it includes this branch’s current commits.", + "mergedReviewNothingLeft": "{{review}} is merged and includes this branch’s committed changes.", + "mergedReviewSafeToDelete": "{{review}} is merged and includes this branch’s committed changes. Ignored files may still be lost when deleting this workspace.", "abortRebaseConfirmTitle": "Abort rebase?", "abortMergeConfirmTitle": "Abort merge?", "abortRebaseConfirmDescription": "This cancels the rebase in progress and can discard conflict resolutions made during this rebase.", diff --git a/src/shared/source-control/decisions/merged-review-workspace-readiness.test.ts b/src/shared/source-control/decisions/merged-review-workspace-readiness.test.ts new file mode 100644 index 00000000000..e0f4aaac528 --- /dev/null +++ b/src/shared/source-control/decisions/merged-review-workspace-readiness.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from 'vitest' +import { resolveMergedReviewWorkspaceReadiness } from './merged-review-workspace-readiness' + +const merged = { state: 'merged' as const, headSha: 'abc123' } + +describe('resolveMergedReviewWorkspaceReadiness', () => { + it('is safe to delete when HEAD is the merged head and the tree is clean', () => { + expect( + resolveMergedReviewWorkspaceReadiness({ + review: merged, + headOid: 'ABC123', + uncommittedCount: 0 + }) + ).toEqual({ kind: 'safe-to-delete' }) + }) + + it('is safe to delete when the provider confirmed HEAD is in the merged review', () => { + expect( + resolveMergedReviewWorkspaceReadiness({ + review: { ...merged, headSha: 'updated-on-github', confirmedContainedHeadOid: 'local' }, + headOid: 'local', + uncommittedCount: 0 + }) + ).toEqual({ kind: 'safe-to-delete' }) + }) + + it('reports uncommitted changes before HEAD identity', () => { + expect( + resolveMergedReviewWorkspaceReadiness({ + review: merged, + headOid: 'abc123', + uncommittedCount: 3 + }) + ).toEqual({ kind: 'uncommitted-changes', count: 3 }) + }) + + it('leaves a different checkout head unverified without assuming its commits are absent', () => { + expect( + resolveMergedReviewWorkspaceReadiness({ + review: merged, + headOid: 'def456', + uncommittedCount: 0 + }) + ).toEqual({ kind: 'head-unverified' }) + }) + + it('leaves missing checkout heads and missing review proof unverified', () => { + expect( + resolveMergedReviewWorkspaceReadiness({ + review: merged, + headOid: null, + uncommittedCount: 0 + }) + ).toEqual({ kind: 'head-unverified' }) + expect( + resolveMergedReviewWorkspaceReadiness({ + review: { state: 'merged' }, + headOid: 'abc123', + uncommittedCount: 0 + }) + ).toEqual({ kind: 'head-unverified' }) + }) + + it('says nothing for reviews that are not merged', () => { + expect( + resolveMergedReviewWorkspaceReadiness({ + review: { ...merged, state: 'open' }, + headOid: 'abc123', + uncommittedCount: 0 + }) + ).toBeNull() + }) +}) diff --git a/src/shared/source-control/decisions/merged-review-workspace-readiness.ts b/src/shared/source-control/decisions/merged-review-workspace-readiness.ts new file mode 100644 index 00000000000..41f93f7dda0 --- /dev/null +++ b/src/shared/source-control/decisions/merged-review-workspace-readiness.ts @@ -0,0 +1,33 @@ +import type { HostedReviewInfo } from '../hosted-review/hosted-review-contracts' + +export type MergedReviewWorkspaceReadiness = + /** HEAD is the merged review's head (or GitHub confirmed it is in the review) and nothing is uncommitted. */ + | { kind: 'safe-to-delete' } + | { kind: 'uncommitted-changes'; count: number } + /** Merged, but this checkout's HEAD cannot be matched to the review, so its commits may be elsewhere. */ + | { kind: 'head-unverified' } + +/** + * Whether a workspace whose review merged still holds work. A squash or rebase merge copies commits onto + * the base with new IDs, so the branch compare keeps listing them; only HEAD identity proves they landed. + */ +export function resolveMergedReviewWorkspaceReadiness(input: { + review: Pick | null + headOid: string | null | undefined + uncommittedCount: number +}): MergedReviewWorkspaceReadiness | null { + const { review, uncommittedCount } = input + if (review?.state !== 'merged') { + return null + } + if (uncommittedCount > 0) { + return { kind: 'uncommitted-changes', count: uncommittedCount } + } + const head = input.headOid?.trim().toLowerCase() ?? '' + const reviewHeads = [review.headSha, review.confirmedContainedHeadOid].map( + (oid) => oid?.trim().toLowerCase() ?? '' + ) + return head.length > 0 && reviewHeads.includes(head) + ? { kind: 'safe-to-delete' } + : { kind: 'head-unverified' } +}