mirror of
https://github.com/stablyai/orca.git
synced 2026-11-15 18:37:24 +00:00
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 <code@tobiasdiez.com> Co-authored-by: alpenshtokk <wawesandways@gmail.com> * 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 <code@tobiasdiez.com> Co-authored-by: alpenshtokk <wawesandways@gmail.com>
This commit is contained in:
co-authored by
Tobias Diez
alpenshtokk
parent
a1667c0297
commit
30fa6b4398
@@ -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')
|
||||
})
|
||||
@@ -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 &&
|
||||
|
||||
@@ -19,6 +19,7 @@ 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}::`
|
||||
@@ -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) {
|
||||
|
||||
@@ -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<string, GitHubPRRefreshAlias>,
|
||||
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<string, GitHubPRRefreshAlias>,
|
||||
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<string, GitHubPRRefreshAlias>): void {
|
||||
const liveCacheKeyByWorktree = new Map<string, string>()
|
||||
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)
|
||||
}
|
||||
}
|
||||
@@ -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<typeof setTimeout> | 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)) {
|
||||
|
||||
@@ -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<string, GitHubPRRefreshAlias>,
|
||||
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<string, GitHubPRRefreshAlias>,
|
||||
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<string, GitHubPRRefreshAlias>): void {
|
||||
const liveCacheKeyByWorktree = new Map<string, string>()
|
||||
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<string, PRRefreshQueueEntry>()
|
||||
@@ -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
|
||||
|
||||
@@ -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. */
|
||||
|
||||
Reference in New Issue
Block a user