Revert "fix: explain merged workspace cleanup and keep visible reviews fresh (#41200)"

This reverts commit 30fa6b4398.

(cherry picked from commit 0b93e1bde5)
This commit is contained in:
Jinjing
2026-11-13 21:09:02 -08:00
committed by Brennan Benson
parent 12a22b7507
commit 48a88010c2
17 changed files with 19 additions and 326 deletions
@@ -217,19 +217,6 @@ 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 &&
@@ -7,13 +7,6 @@ const BACKGROUND_BUDGET_MAX = 20
const ACTIVE_BURST_WINDOW_MS = 30_000
const ACTIVE_BURST_MAX = 3
export function isSelectedBudgetedPRRefresh(entry: PRRefreshQueueEntry): boolean {
return (
entry.candidate.isSelected === true &&
(entry.reason === 'visible' || (entry.reason === 'swr' && entry.hosted !== undefined))
)
}
export function usesActiveRefreshPacing(entry: PRRefreshQueueEntry): boolean {
// Selected admission survives deselection while queued; periodic follow-ups clear the flag.
return (
@@ -26,7 +19,6 @@ export class PRRefreshPacing {
private readonly backgroundStarts: number[] = []
private readonly activeStartsByScope = new Map<string, number[]>()
private lastBackgroundStartAt = 0
private lastBudgetedStartWasSelected = false
clearActiveBurstWindow(windowId: number): void {
const windowPrefix = `${windowId}::`
@@ -37,8 +29,7 @@ export class PRRefreshPacing {
}
}
noteBackgroundStart(entry: PRRefreshQueueEntry): void {
this.lastBudgetedStartWasSelected = isSelectedBudgetedPRRefresh(entry)
noteBackgroundStart(): void {
const now = Date.now()
this.lastBackgroundStartAt = now
this.backgroundStarts.push(now)
@@ -59,11 +50,7 @@ export class PRRefreshPacing {
return Math.max(spacingDelay, windowDelay)
}
order(a: PRRefreshQueueEntry, b: PRRefreshQueueEntry): number {
const priority = this.priority(b) - this.priority(a)
if (priority !== 0) {
return priority
}
activeOrder(a: PRRefreshQueueEntry, b: PRRefreshQueueEntry): number {
if (!usesActiveRefreshPacing(a) || !usesActiveRefreshPacing(b)) {
return 0
}
@@ -73,15 +60,6 @@ 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 &&
isSelectedBudgetedPRRefresh(entry) &&
entry.bypassBackgroundBudget !== true
? 0
: entry.priority
}
entryDelay(entry: PRRefreshQueueEntry): number {
const activeDelay = usesActiveRefreshPacing(entry) ? this.nextActiveBurstDelay(entry) : 0
if (activeDelay > 0) {
@@ -196,7 +196,7 @@ export class PRRefreshQueueDrainer {
next.bypassBackgroundBudget !== true &&
isBudgetedBackground(next.reason)
) {
this.pacing.noteBackgroundStart(next)
this.pacing.noteBackgroundStart()
this.watchJobs.noteBudgetedStart(next)
}
if (next.watch && usesActiveRefreshPacing(next)) {
@@ -9,8 +9,7 @@ import {
isBudgetedBackground,
isMergeabilityPendingOutcome,
sameAliasRequestIdentity,
visibleCandidateAfterOutcome,
visibleFollowUpPriority
visibleCandidateAfterOutcome
} from './pr-refresh-candidate-policy'
import type {
PreparedPRBranchRead,
@@ -110,7 +109,7 @@ export class PRRefreshQueueScheduler {
candidate: current,
aliases: new Map(aliases.map((alias) => [alias.cacheKey, alias])),
reason: 'visible',
priority: visibleFollowUpPriority(current, priority),
priority,
dueAt,
queuedAt: this.queue.nextOrder(),
followUp: !foreground,
@@ -250,7 +249,7 @@ export class PRRefreshQueueScheduler {
candidate: followUpCandidate,
aliases: new Map(aliases.map((alias) => [alias.cacheKey, alias])),
reason: 'visible',
priority: visibleFollowUpPriority(followUpCandidate, priority),
priority,
dueAt,
queuedAt: this.queue.nextOrder(),
bypassBackgroundBudget: pendingDueAt !== null,
@@ -262,7 +261,7 @@ export class PRRefreshQueueScheduler {
ordered(): PRRefreshQueueEntry[] {
this.watchJobs.age(this.queue.values(), Date.now())
return this.queue.ordered((a, b) => this.pacing.order(a, b))
return this.queue.ordered((a, b) => this.pacing.activeOrder(a, b))
}
nextQueuedWakeDelay(excludedKey: string): number | null {
@@ -13,8 +13,7 @@ import {
POST_PUSH_DELAY_MS,
refreshKey,
sameAliasRequestIdentity,
shouldSkipFresh,
visibleFollowUpPriority
shouldSkipFresh
} from './pr-refresh-candidate-policy'
export type PRRefreshQueueEntry = {
@@ -123,7 +122,6 @@ 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
@@ -277,14 +275,14 @@ export class PRRefreshQueue {
}
ordered(
pacingOrder: (a: PRRefreshQueueEntry, b: PRRefreshQueueEntry) => number
activeOrder: (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 pacingOrder(a, b) || b.priority - a.priority || a.dueAt - b.dueAt
return b.priority - a.priority || activeOrder(a, b) || a.dueAt - b.dueAt
}
if (aReady !== bReady) {
return aReady ? -1 : 1
@@ -53,7 +53,7 @@ export function admitPreparedPRRead(
}
if (!next.watch && isBackground(next.reason) && !joinedRead) {
if (next.bypassBackgroundBudget !== true && isBudgetedBackground(next.reason)) {
pacing.noteBackgroundStart(next)
pacing.noteBackgroundStart()
watchJobs.noteBudgetedStart(next)
}
if (usesActiveRefreshPacing(next)) {
@@ -6,7 +6,6 @@ import { prChecksDone } from '../../../../shared/github/pull-requests/refresh/pr
export { prChecksDone } from '../../../../shared/github/pull-requests/refresh/pr-quiet-refresh'
import { reviewRefreshIntervalMs } from '../../../../shared/review-refresh-policy'
import type { PRRefreshQueue, PRRefreshQueueEntry } from './pr-refresh-queue'
import { isSelectedBudgetedPRRefresh } from './pr-refresh-pacing'
import type { PRRefreshRetryState } from './pr-refresh-retry-state'
export type PRWatchJob = NonNullable<PRRefreshQueueEntry['watch']>
@@ -97,10 +96,7 @@ export class PRWatchJobRunner {
}
noteBudgetedStart(entry: PRRefreshQueueEntry): void {
// Selected list reads have their own reserved turn; preserve the watch/list alternation.
if (!isSelectedBudgetedPRRefresh(entry)) {
this.lastBudgetedStartWasWatch = entry.watch !== undefined
}
this.lastBudgetedStartWasWatch = entry.watch !== undefined
}
/** Re-ranks watch entries before each pick, so aging needs no change to the queue's order. */
@@ -1,66 +0,0 @@
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 <div className="bg-sidebar p-2 text-foreground">{children}</div>
}
export const safeToDelete = defineFixture({
width: 300,
render: () => (
<Frame>
<SourceControlMergedReviewNotice
review={review}
worktree={worktree}
statusHeadOid="a1b2c3d"
uncommittedCount={0}
/>
</Frame>
)
})
export const uncommittedChanges = defineFixture({
width: 300,
render: () => (
<Frame>
<SourceControlMergedReviewNotice
review={review}
worktree={worktree}
statusHeadOid="a1b2c3d"
uncommittedCount={2}
/>
</Frame>
)
})
export const headMovedPastReview = defineFixture({
width: 300,
render: () => (
<Frame>
<SourceControlMergedReviewNotice
review={review}
worktree={worktree}
statusHeadOid="e4f5a6b"
uncommittedCount={0}
/>
</Frame>
)
})
@@ -1,119 +0,0 @@
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 (
<div
className="flex items-start gap-2 rounded-md border border-border bg-card px-2 py-1.5 text-[11px] text-muted-foreground"
role="status"
>
<GitMerge className="mt-0.5 size-3.5 shrink-0 text-callout-important" aria-hidden="true" />
<div className="flex min-w-0 flex-1 flex-col items-start gap-1.5">
<span>{message}</span>
{canDelete ? (
<Button
type="button"
variant="outline"
size="xs"
className="cursor-pointer"
disabled={isDeleting}
onClick={() =>
runWorktreeDelete(worktree.id, {
expectedHostId: worktree.hostId,
expectedInstanceId: worktree.instanceId,
forceConfirm: true
})
}
>
{isDeleting ? (
<LoaderCircle className="size-3.5 animate-spin" />
) : (
<Trash2 className="size-3.5" />
)}
{isDeleting
? t('auto.components.right.sidebar.HostedReviewActions.eefd50457e', 'Deleting...')
: t(
'auto.components.right.sidebar.HostedReviewActions.e4aca40024',
'Delete Workspace'
)}
</Button>
) : null}
</div>
</div>
)
}
@@ -8,7 +8,6 @@ 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'
@@ -136,19 +135,6 @@ export function SourceControlPanelContent(props: SourceControlPanelReadyProps) {
{/* Keep CommitArea through clean/mid-commit states (#1448); only active conflict operations replace the action surface. */}
<SourceControlForkPushNotice pushTarget={activeWorktree.pushTarget ?? null} />
{model.isFolder ? null : (
<SourceControlMergedReviewNotice
review={model.hostedReview}
worktree={activeWorktree}
statusHeadOid={
statusReadError === undefined && repositoryHuge === undefined
? model.activeGitStatusHead
: null
}
uncommittedCount={model.entries.length}
/>
)}
{shouldRenderCommitArea(
unresolvedConflicts.length,
conflictOperation,
@@ -108,8 +108,6 @@ 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(
@@ -161,18 +159,6 @@ 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([
@@ -96,7 +96,6 @@ 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) {
@@ -115,7 +114,7 @@ export function runWorktreeDelete(worktreeId: string, options: WorktreeDeleteOpt
])
}
: {}),
...(hasLineageChildren || options.forceConfirm ? { allowSkipConfirm: false } : {})
...(hasLineageChildren ? { allowSkipConfirm: false } : {})
})
}
@@ -15,7 +15,6 @@ export type WorktreeBatchDeleteOptions = {
export type WorktreeDeleteIdentity = Pick<Worktree, 'id' | 'instanceId' | 'hostId'>
export type WorktreeDeleteOptions = {
forceConfirm?: boolean
expectedInstanceId?: string
/** Pin deletion to the confirmed host because workspace IDs repeat across hosts. */
expectedHostId?: ExecutionHostId
@@ -114,9 +114,6 @@ 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)
}
@@ -151,17 +148,14 @@ 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)) &&
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 &&
!canShowSleepingStatus &&
QUIET_REVIEW_REPLACEABLE_STATUSES.has(status)
const canShowBranchStatus =
newCardStyle &&
@@ -184,14 +178,10 @@ 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: laneLabel })
: laneLabel
? t('workspace.activity.unreadLabel', '{{status}} · Unread', { status: reviewLabel })
: reviewLabel
// Working/permission own the lane; retain unread in accessible copy and restore its dot afterward.
const showNewCardUnreadAlert =
newCardStyle && isUnread && showStatus && status !== 'working' && status !== 'permission'
-2
View File
@@ -429,8 +429,6 @@
"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"
},
-5
View File
@@ -14789,11 +14789,6 @@
"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.",
@@ -1,33 +0,0 @@
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<HostedReviewInfo, 'state' | 'headSha' | 'confirmedContainedHeadOid'> | 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' }
}