diff --git a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-errors.ts b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-errors.ts index 121b47fc0cc..111f094a4fd 100644 --- a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-errors.ts +++ b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-errors.ts @@ -8,21 +8,11 @@ import { remoteBrowserServiceUnavailableNotice } from './remote-browser-stream-status' -// Why: a runtime lacking browser.screencast.v1 will not grow it while this connection lives, so -// retrying that failure is unbounded work with a visible error each round. Tagged rather than -// message-matched so a reworded string cannot silently turn it back into an infinite retry. -export const REMOTE_BROWSER_STREAM_UNSUPPORTED = 'remote_browser_stream_unsupported' +// Why: a missing capability never appears mid-connection; tagged so a reworded message can't restore infinite retry. +const REMOTE_BROWSER_STREAM_UNSUPPORTED = 'remote_browser_stream_unsupported' -// Why: the runtime answers with these when the thing the stream is anchored to is gone from the -// host (worktree deleted, repo unregistered, capability absent). Retrying cannot bring it back, so -// they are permanent for this connection exactly like the capability tag above. Codes come from -// src/main/runtime/rpc/errors.ts rather than being invented here. -// Why `selector_not_found` is NOT here: it means "I could not resolve this right now", which is -// UNKNOWN, not proof the target is gone. Its producer is a live worktree scan behind a 1s-TTL cache, -// and the connectionId-gated fallback that shields SSH repos from scan lag does not cover purely -// local ones — so a slow scan can surface it transiently. Treating that as permanent would strand -// the pane forever, which is the exact bug this file exists to prevent. The three below are -// unambiguous: the host is telling us the thing itself no longer exists. +// Why: host says the anchor itself is gone (codes from src/main/runtime/rpc/errors.ts); retrying can't restore it. +// selector_not_found is deliberately excluded: a slow 1s-TTL worktree scan can emit it transiently. const REMOTE_BROWSER_STREAM_TARGET_GONE_CODES: ReadonlySet = new Set([ 'worktree_not_found_on_server', 'repo_not_found', @@ -80,18 +70,7 @@ export type RemoteBrowserStreamFailure = { logRawError: boolean } -// Why the message is decided here rather than at the call site: whether a failure is permanent and -// what the user should be told are the same judgement. A permanent failure is one we classified and -// understand, so its own message says something true and specific ("The selected runtime does not -// support remote browser streaming."); flattening that to "Lost connection" would be vaguer and -// wrong, since nothing was lost. Everything else carries a raw transport string written for logs, -// so the pane speaks for itself there and the raw text is left to the caller to log. -// -// Note what this deliberately does NOT decide: whether the user is offered a way back. Stopping -// automatic retries and removing the user's only recovery are different things, and conflating them -// is what stranded the pane in the first place. `selector_not_found` already had to be walked back -// out of the permanent set once (08260a54bf) — proof this classification can be wrong — so a -// misjudgement here must stay recoverable by hand. +// Why: a classified permanent failure keeps its own specific message; others get a pane-authored notice and the raw error is logged. Whether to offer reconnect is decided by status, not here. export function resolveRemoteBrowserStreamFailure( error: unknown, phase: 'opening' | 'restart' = 'restart' diff --git a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-lifecycle.ts b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-lifecycle.ts index 0dabac044d5..64c5f2e433a 100644 --- a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-lifecycle.ts +++ b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-lifecycle.ts @@ -108,9 +108,7 @@ export class RemoteBrowserStreamLifecycle { deps.closeMissingRemotePage(tokens.remotePage) return } - // Why classified here too: the same condition (a host that cannot stream) reaches both this - // path and the restart path, and it must not read as "unreachable" here and as its own - // specific message there. + // Why classified: a host that cannot stream must read the same here as on the restart path. const failure = resolveRemoteBrowserStreamFailure(error, 'opening') if (failure.logRawError) { console.warn('[browser-pane] remote browser failed to open:', error) @@ -177,9 +175,7 @@ export class RemoteBrowserStreamLifecycle { if (this.restartScheduler.isScheduled) { return } - // Why classified rather than forwarded: this was the one failure path still putting raw - // transport text in the UI ("Runtime environment pairing changed; refresh and try again"), - // which is written for logs and names our internals. The other two paths already classify. + // Why classified: raw transport text (e.g. "pairing changed") is for logs, not the pane. const failure = resolveRemoteBrowserStreamFailure(error) if (failure.logRawError) { console.warn('[browser-pane] remote stream resize failed:', error) @@ -294,19 +290,11 @@ export class RemoteBrowserStreamLifecycle { this.setStatus(remoteBrowserStreamStopped(message)) this.handleStreamClosed(token, false) }, - // Why 'stopped' and not 'retrying': a transport error is NOT guaranteed to be followed by - // a close. The web client's notifySubscriptionsError clears its subscription map and then - // delivers onError only, so on that path no close ever arrives — and 'retrying' would - // leave the pane busy forever with no way back, worse than the bug this PR fixes. When a - // close does follow (the usual case) it publishes 'retrying', which replaces this within - // the same tick. Landing in the actionable state and being corrected is safe; the reverse - // is not. - // Why the deadline is cancelled here: this is the one stream-ending path that keeps its - // token, so the 'never said ready' deadline stays armed and its guard still passes. It - // would then fire against a stream already declared stopped and republish 'retrying' — - // withdrawing the reconnect control 30s after handing it over, with no user action, and - // reopening the strand this whole feature exists to remove. Not clear(): a close that - // does follow must still refill the budget for a stream that had been healthy. + // Why 'stopped': notifySubscriptionsError delivers onError without a close, so 'retrying' + // would leave the pane busy forever; a following close replaces this with 'retrying'. + // Why cancel the deadline: this path keeps its token, so the armed ready-deadline would + // later republish 'retrying' over the reconnect control. Not clear(): a following close + // must still refill the budget for a stream that had been healthy. onTransportError: () => { this.liveness.stopWaitingForReady() this.setStatus(remoteBrowserStreamStopped(remoteBrowserStreamLostNotice())) diff --git a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-restart-attempt.ts b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-restart-attempt.ts index 6d925119383..3a326342d41 100644 --- a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-restart-attempt.ts +++ b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-restart-attempt.ts @@ -26,9 +26,7 @@ export type RemoteBrowserStreamRestartAttemptDeps = { adoptSubscription: (subscription: RemoteBrowserStreamSubscription) => void } -// One self-heal attempt for a dropped stream, as the scheduler wants it: resolves true to keep -// retrying, false to stop. Split from the lifecycle because a single attempt needs none of the -// lifecycle's own state — only the token it is retrying on behalf of. +// One self-heal attempt for a dropped stream: resolves true to keep retrying, false to stop. export function createRemoteBrowserStreamRestartAttempt( token: RemoteBrowserStreamToken, deps: RemoteBrowserStreamRestartAttemptDeps @@ -65,14 +63,10 @@ export function createRemoteBrowserStreamRestartAttempt( } const failure = resolveRemoteBrowserStreamFailure(error) if (failure.logRawError) { - // The raw text is transport-level and written for logs; keep it out of the UI but not out of - // reach, since nothing else records it. + // The raw transport text stays out of the UI, so this log is its only record. console.warn('[browser-pane] remote stream restart failed:', error) } - // Why giving up publishes 'stopped': abandoning automatic retries is not the same as taking - // away the user's last resort, and a classification we got wrong would otherwise strand the - // pane exactly as it did before this work. While attempts remain it stays 'retrying', which - // reports the failure without offering a control that competes with the next attempt. + // Why: giving up still publishes 'stopped' so a misclassified failure stays manually recoverable. lastNotice = failure.message deps.setStatus( failure.shouldRetry diff --git a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-status.ts b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-status.ts index e2842df8694..bf64189c37b 100644 --- a/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-status.ts +++ b/src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-status.ts @@ -1,25 +1,13 @@ import { translate } from '@/i18n/i18n' -// Why one value instead of the three booleans/strings this replaces (error, reconnectAvailable, -// busy): they always described a single thing — what this stream is doing right now — but were -// written independently from ~15 sites, so nothing stopped them disagreeing. Every defect three -// review rounds found in this area was one such disagreement: -// -// stopped, but no affordance -> the pane stranded (the bug this work exists to fix) -// stopped, but no message -> the control renders inside the message, so it could not render -// retrying, but affordance shown -> offered a manual retry ~500ms before the automatic one -// stopped, but still busy -> a spinner over a frozen frame, blocking the pane's own input -// -// Expressed as one tagged value, none of those four is a state you can write down. `busy`, the -// message, and whether the reconnect control appears are all derived from it, so they cannot drift. +// Why one tagged value: busy, message and reconnect availability are derived from it, so they cannot disagree. export type RemoteBrowserStreamStatus = | { kind: 'idle' } /** Establishing a stream: no stream yet, and no failure to report. */ | { kind: 'opening' } /** A confirmed-live stream; the host has sent 'ready'. */ | { kind: 'live' } - // Automatic recovery is running. The notice is null until an attempt has actually failed, so a - // blip the budget absorbs in 500ms stays invisible — which is the whole point of having a budget. + // notice stays null until an attempt fails, so a blip absorbed by the budget stays invisible. | { kind: 'retrying'; notice: string | null } /** Automatic recovery is over. This is the only state that offers a reconnect. */ | { kind: 'stopped'; notice: string } @@ -43,10 +31,7 @@ export function isRemoteBrowserStreamBusy(status: RemoteBrowserStreamStatus): bo /** The stream's own message. Incidental notices (input failures, URL validation) are separate. */ export function remoteBrowserStreamNotice(status: RemoteBrowserStreamStatus): string | null { - if (status.kind === 'stopped') { - return status.notice - } - return status.kind === 'retrying' ? status.notice : null + return status.kind === 'stopped' || status.kind === 'retrying' ? status.notice : null } // Why only 'stopped': while attempts remain, a manual control competes with the automatic recovery @@ -55,15 +40,6 @@ export function canReconnectRemoteBrowserStream(status: RemoteBrowserStreamStatu return status.kind === 'stopped' } -// Why the pane authors these rather than forwarding the failure's own message: those strings come -// from the transport layer and are written for logs (e.g. "Runtime environment is manually -// disconnected." — an RPC error with the code runtime_manually_disconnected). They name our -// internals rather than the user's situation and none of them change what the user can do, which -// the reconnect control already says. The raw error is still logged, so diagnosis keeps the detail -// the UI drops. A failure we classified ourselves keeps its own message: it is specific and true, -// and flattening "The selected runtime does not support remote browser streaming." into "lost -// connection" would be both vaguer and wrong. - /** The stream was established and then died. */ export function remoteBrowserStreamLostNotice(): string { return translate( diff --git a/src/renderer/src/components/right-sidebar/file-explorer-operation-owner.ts b/src/renderer/src/components/right-sidebar/file-explorer-operation-owner.ts index 06422959001..2c52ad62895 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-operation-owner.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-operation-owner.ts @@ -1,7 +1,12 @@ import { getConnectionIdFromState } from '@/lib/connection-context' import { useAppStore } from '@/store' import type { AppState } from '@/store/types' -import { parseExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' +import { + parseExecutionHostId, + toRuntimeExecutionHostId, + toSshExecutionHostId, + type ExecutionHostId +} from '../../../../shared/execution-host' import { parseWorkspaceKey } from '../../../../shared/workspace-scope' import { translate } from '@/i18n/i18n' import { @@ -16,6 +21,8 @@ import { } from '@/lib/worktree-operation-route' import { captureWorktreeOperationGenerationGuard } from '@/lib/worktree-operation-generation' +const ownerUnresolved = (): Error => new Error(getFileExplorerOwnerUnresolvedMessage()) + export type FileExplorerOperationRoute = { settings: { activeRuntimeEnvironmentId: string | null } connectionId?: string @@ -60,7 +67,7 @@ export function getFileExplorerOperationOwnerFromState( kind: 'runtime', environmentId: route.runtimeEnvironmentId, executionHostId: - route.executionHostId ?? `runtime:${encodeURIComponent(route.runtimeEnvironmentId)}` + route.executionHostId ?? toRuntimeExecutionHostId(route.runtimeEnvironmentId) } } if (route.executionHostId) { @@ -86,7 +93,7 @@ export function getFileExplorerOperationOwnerFromState( return { kind: 'runtime', environmentId: runtimeEnvironmentId, - executionHostId: `runtime:${encodeURIComponent(runtimeEnvironmentId)}` + executionHostId: toRuntimeExecutionHostId(runtimeEnvironmentId) } } if (connectionId === undefined) { @@ -114,7 +121,7 @@ export function getFileExplorerOperationRoute( return { settings: { activeRuntimeEnvironmentId: null }, connectionId: owner.connectionId, - expectedExecutionHostId: `ssh:${encodeURIComponent(owner.connectionId)}` + expectedExecutionHostId: toSshExecutionHostId(owner.connectionId) } case 'runtime': { const host = parseExecutionHostId(owner.executionHostId) @@ -135,15 +142,15 @@ export function requireMatchingFileExplorerOperationRoute( expectedOwner: FileExplorerOperationOwner | undefined ): FileExplorerOperationRoute { if (!expectedOwner || expectedOwner.kind === 'unresolved') { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + throw ownerUnresolved() } const currentOwner = getFileExplorerOperationOwner(worktreeId) if (JSON.stringify(currentOwner) !== JSON.stringify(expectedOwner)) { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + throw ownerUnresolved() } const route = getFileExplorerOperationRoute(expectedOwner) if (!route) { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + throw ownerUnresolved() } return route } @@ -153,18 +160,18 @@ export function captureFileExplorerOperationGuard( expectedOwner: FileExplorerOperationOwner | undefined ): FileExplorerOperationGuard { if (!worktreeId) { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + throw ownerUnresolved() } const route = requireMatchingFileExplorerOperationRoute(worktreeId, expectedOwner) const operationRoute = getFileExplorerGenerationRoute(expectedOwner) if (!operationRoute) { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + throw ownerUnresolved() } const generationGuard = captureWorktreeOperationGenerationGuard( useAppStore.getState, worktreeId, operationRoute, - () => new Error(getFileExplorerOwnerUnresolvedMessage()), + ownerUnresolved, () => getFileExplorerGenerationRoute(getFileExplorerOperationOwner(worktreeId)) ) const expectedSshConnectionGeneration = getExpectedSshConnectionGeneration( @@ -173,15 +180,15 @@ export function captureFileExplorerOperationGuard( ) const operationHost = parseExecutionHostId(operationRoute.executionHostId) if (!operationHost) { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + throw ownerUnresolved() } - if (operationHost?.kind === 'ssh' && expectedSshConnectionGeneration === undefined) { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + if (operationHost.kind === 'ssh' && expectedSshConnectionGeneration === undefined) { + throw ownerUnresolved() } const guardedRoute: FileExplorerOperationRoute = { ...route, expectedExecutionHostId: operationHost.kind === 'ssh' ? operationHost.id : 'local', - ...(operationHost?.kind === 'ssh' ? { expectedSshTargetId: operationHost.targetId } : {}), + ...(operationHost.kind === 'ssh' ? { expectedSshTargetId: operationHost.targetId } : {}), ...(expectedSshConnectionGeneration === undefined ? {} : { expectedSshConnectionGeneration }) } return { @@ -192,7 +199,7 @@ export function captureFileExplorerOperationGuard( getExpectedSshConnectionGeneration(useAppStore.getState(), operationRoute) !== expectedSshConnectionGeneration ) { - throw new Error(getFileExplorerOwnerUnresolvedMessage()) + throw ownerUnresolved() } return guardedRoute } @@ -222,7 +229,7 @@ function getFileExplorerGenerationRoute( return { executionHostId: 'local', runtimeEnvironmentId: null } case 'ssh': return { - executionHostId: `ssh:${encodeURIComponent(owner.connectionId)}`, + executionHostId: toSshExecutionHostId(owner.connectionId), runtimeEnvironmentId: null } case 'runtime': diff --git a/src/renderer/src/components/settings/repository-host-setup-options.ts b/src/renderer/src/components/settings/repository-host-setup-options.ts index 2e370d5c7ec..6b211096338 100644 --- a/src/renderer/src/components/settings/repository-host-setup-options.ts +++ b/src/renderer/src/components/settings/repository-host-setup-options.ts @@ -1,11 +1,8 @@ import { getExecutionHostLabel, type ExecutionHostId } from '../../../../shared/execution-host' import type { ExecutionHostRegistryEntry } from '../../../../shared/execution-host-registry' -import { - PROJECT_HOST_SETUP_RUNTIME_CAPABILITY, - WORKSPACE_RUN_CONTEXT_RUNTIME_CAPABILITY -} from '../../../../shared/protocol-version' import type { ProjectHostSetup, ProjectHostSetupState } from '../../../../shared/project-types' import { translate } from '@/i18n/i18n' +import { getHostSetupUnavailableDetail } from '@/lib/project-host-setup-options' import { pickerExecutionHosts } from '../../../../shared/managed-orcad-execution-host' export type SetupHostOption = { @@ -56,7 +53,7 @@ export function buildSetupHostOptions({ !host.aliasHostIds?.some((aliasHostId) => setupHostIds.has(aliasHostId)) ) .map((host) => { - const availability = getHostSetupAvailability(host) + const unavailableDetail = getHostSetupUnavailableDetail(host) // Why: import and clone require live remote providers, while an offline // host can still be recorded as a placeholder for later setup. const canUsePathActions = host.health === 'local' || host.health === 'available' @@ -64,57 +61,15 @@ export function buildSetupHostOptions({ id: host.id, label: host.label || getExecutionHostLabel(host.id), detail: - availability.isAvailable && !canUsePathActions - ? translate( + unavailableDetail ?? + (canUsePathActions + ? host.detail + : translate( 'auto.components.settings.RepositoryPane.hostSetupConnectionRequired', 'Connect this host before importing or cloning the project' - ) - : availability.detail, - isAvailable: availability.isAvailable, + )), + isAvailable: unavailableDetail === null, canUsePathActions } }) } - -function getHostSetupAvailability(host: ExecutionHostRegistryEntry): { - isAvailable: boolean - detail: string -} { - if (host.health === 'blocked') { - return { - isAvailable: false, - detail: translate( - 'auto.components.settings.RepositoryPane.hostSetupBlockedVersion', - 'Orca server version is incompatible' - ) - } - } - if (host.kind === 'runtime') { - const capabilities = host.capabilities - if (!capabilities) { - return { - isAvailable: false, - detail: translate( - 'auto.components.settings.RepositoryPane.hostSetupCheckingCapability', - 'Checking host capabilities' - ) - } - } - if ( - !capabilities.includes(PROJECT_HOST_SETUP_RUNTIME_CAPABILITY) || - !capabilities.includes(WORKSPACE_RUN_CONTEXT_RUNTIME_CAPABILITY) - ) { - return { - isAvailable: false, - detail: translate( - 'auto.components.settings.RepositoryPane.hostSetupMissingCapability', - 'Update Orca on this host to set up projects' - ) - } - } - } - return { - isAvailable: true, - detail: host.detail - } -} diff --git a/src/renderer/src/components/sidebar/add-repo-runtime-owner.ts b/src/renderer/src/components/sidebar/add-repo-runtime-owner.ts index 3bcc06d4a35..9aa46115355 100644 --- a/src/renderer/src/components/sidebar/add-repo-runtime-owner.ts +++ b/src/renderer/src/components/sidebar/add-repo-runtime-owner.ts @@ -4,6 +4,7 @@ import { toSshExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' +import { getActiveRuntimeTarget, type RuntimeClientTarget } from '@/runtime/runtime-rpc-client' export type CapturedRuntimeOwner = string | null | undefined @@ -33,3 +34,13 @@ export function worktreeRefreshOptions( ...(executionHostId ? { executionHostId } : {}) } } + +export function resolveAddRepoRuntimeTarget( + runtimeEnvironmentId: string | null | undefined, + settings: Parameters[0] +): RuntimeClientTarget { + const environmentId = runtimeEnvironmentId?.trim() + return environmentId + ? { kind: 'environment', environmentId } + : getActiveRuntimeTarget({ ...settings, activeRuntimeEnvironmentId: null }) +} diff --git a/src/renderer/src/components/sidebar/sidebar-host-options.ts b/src/renderer/src/components/sidebar/sidebar-host-options.ts index 6dfc5b041d5..ffd80395e51 100644 --- a/src/renderer/src/components/sidebar/sidebar-host-options.ts +++ b/src/renderer/src/components/sidebar/sidebar-host-options.ts @@ -4,6 +4,7 @@ import type { WorkspaceHostScope } from '../../../../shared/ui-chrome-types' import { ALL_EXECUTION_HOSTS_SCOPE, LOCAL_EXECUTION_HOST_ID, + toRuntimeExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' import { @@ -69,7 +70,7 @@ export function buildSidebarHostOptions(args: { } } const activeRuntimeHostId = args.settings?.activeRuntimeEnvironmentId?.trim() - ? (`runtime:${encodeURIComponent(args.settings.activeRuntimeEnvironmentId.trim())}` as const) + ? toRuntimeExecutionHostId(args.settings.activeRuntimeEnvironmentId.trim()) : null return buildExecutionHostRegistry({ repos: args.repos, diff --git a/src/renderer/src/components/sidebar/useAddRepoCloneFlow.ts b/src/renderer/src/components/sidebar/useAddRepoCloneFlow.ts index 10912af1870..a81a3eb3be6 100644 --- a/src/renderer/src/components/sidebar/useAddRepoCloneFlow.ts +++ b/src/renderer/src/components/sidebar/useAddRepoCloneFlow.ts @@ -1,7 +1,7 @@ import { useCallback, useEffect, useRef, useState, type Dispatch, type SetStateAction } from 'react' import { toast } from 'sonner' import { useAppStore } from '@/store' -import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' +import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import type { AddRepoExistingWorkspaceSource } from '../../../../shared/telemetry-events' import type { Repo } from '../../../../shared/repo-types' import { getCloneDestinationAutoFill } from './clone-defaults' @@ -9,7 +9,7 @@ import type { AddRepoDialogStep } from './add-repo-dialog-types' import { translate } from '@/i18n/i18n' import { extractIpcErrorMessage } from '@/lib/ipc-error' import { upsertAddedRepoWithProjectHostSetup } from './add-repo-store-upsert' -import { worktreeRefreshOptions } from './add-repo-runtime-owner' +import { resolveAddRepoRuntimeTarget, worktreeRefreshOptions } from './add-repo-runtime-owner' import type { ExecutionHostId } from '../../../../shared/execution-host' export function useAddRepoCloneFlow({ @@ -126,16 +126,16 @@ export function useAddRepoCloneFlow({ } const requestHostToken = hostTokenRef.current const gen = ++cloneGenRef.current + const isCurrent = (): boolean => + gen === cloneGenRef.current && requestHostToken === hostTokenRef.current setIsCloning(true) setCloneError(null) setCloneProgress(null) try { - const target = activeRuntimeEnvironmentId?.trim() - ? { kind: 'environment' as const, environmentId: activeRuntimeEnvironmentId.trim() } - : getActiveRuntimeTarget({ - ...useAppStore.getState().settings, - activeRuntimeEnvironmentId: null - }) + const target = resolveAddRepoRuntimeTarget( + activeRuntimeEnvironmentId, + useAppStore.getState().settings + ) const repo = sshTargetId?.trim() ? await window.api.repos.cloneRemote({ connectionId: sshTargetId.trim(), @@ -158,7 +158,7 @@ export function useAddRepoCloneFlow({ url: trimmedUrl, destination: cloneDestination.trim() })) as Repo) - if (gen !== cloneGenRef.current || requestHostToken !== hostTokenRef.current) { + if (!isCurrent()) { return } const { repo: ownedRepo } = upsertAddedRepoWithProjectHostSetup(repo, { @@ -173,18 +173,18 @@ export function useAddRepoCloneFlow({ // should fall through to project reveal instead of leaving the add flow open. const ownerOptions = worktreeRefreshOptions(activeRuntimeEnvironmentId, sshTargetId) await fetchWorktrees(ownedRepo.id, ownerOptions) - if (gen !== cloneGenRef.current || requestHostToken !== hostTokenRef.current) { + if (!isCurrent()) { return } await onGitRepoReady(ownedRepo.id, 'clone_url', ownerOptions.executionHostId) } catch (err) { - if (gen !== cloneGenRef.current || requestHostToken !== hostTokenRef.current) { + if (!isCurrent()) { return } const message = extractIpcErrorMessage(err, String(err)) setCloneError(message) } finally { - if (gen === cloneGenRef.current && requestHostToken === hostTokenRef.current) { + if (isCurrent()) { setIsCloning(false) } } diff --git a/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts b/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts index 4a01734d195..2a8eff35c02 100644 --- a/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts +++ b/src/renderer/src/components/sidebar/useCreateRepo.default-checkout.test.ts @@ -149,7 +149,7 @@ describe('useCreateRepo default-checkout handoff', () => { expect(mocks.storeState.projectHostSetups).toEqual( expect.arrayContaining([expect.objectContaining({ repoId: repo.id, path: repo.path })]) ) - expect(mocks.onGitRepoReady).toHaveBeenCalledWith(repo.id) + expect(mocks.onGitRepoReady).toHaveBeenCalledWith(repo.id, undefined) }) it('returns the selected parent directory after the local picker applies it', async () => { @@ -187,7 +187,7 @@ describe('useCreateRepo default-checkout handoff', () => { expect(mocks.fetchWorktrees).toHaveBeenCalledWith(repo.id, { requireAuthoritative: true }) - expect(mocks.onGitRepoReady).toHaveBeenCalledWith(repo.id) + expect(mocks.onGitRepoReady).toHaveBeenCalledWith(repo.id, undefined) expect(mocks.stateSetters[STATE_ERROR_MESSAGE]).not.toHaveBeenCalledWith( 'Could not refresh project worktrees. Try again.' ) diff --git a/src/renderer/src/components/sidebar/useCreateRepo.ts b/src/renderer/src/components/sidebar/useCreateRepo.ts index e136a1c4010..f73b3e2790a 100644 --- a/src/renderer/src/components/sidebar/useCreateRepo.ts +++ b/src/renderer/src/components/sidebar/useCreateRepo.ts @@ -6,13 +6,13 @@ import { useAppStore } from '@/store' import { useMountedRef } from '@/hooks/useMountedRef' import { activateAndRevealWorktree } from '@/lib/worktree-activation' import { markOnboardingProjectAdded } from '@/lib/onboarding-project-checklist' -import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' +import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import { isGitRepoKind } from '../../../../shared/repo-kind' import type { Repo } from '../../../../shared/repo-types' import { translate } from '@/i18n/i18n' import { extractIpcErrorMessage } from '@/lib/ipc-error' import { upsertAddedRepoWithProjectHostSetup } from './add-repo-store-upsert' -import { worktreeRefreshOptions } from './add-repo-runtime-owner' +import { resolveAddRepoRuntimeTarget, worktreeRefreshOptions } from './add-repo-runtime-owner' import type { ExecutionHostId } from '../../../../shared/execution-host' export function useCreateRepo( @@ -93,15 +93,17 @@ export function useCreateRepo( } const requestHostToken = hostTokenRef.current const gen = ++createGenRef.current + const isCurrent = (): boolean => + gen === createGenRef.current && + requestHostToken === hostTokenRef.current && + mountedRef.current setIsCreating(true) setCreateError(null) try { - const target = options.runtimeEnvironmentId?.trim() - ? { kind: 'environment' as const, environmentId: options.runtimeEnvironmentId.trim() } - : getActiveRuntimeTarget({ - ...useAppStore.getState().settings, - activeRuntimeEnvironmentId: null - }) + const target = resolveAddRepoRuntimeTarget( + options.runtimeEnvironmentId, + useAppStore.getState().settings + ) // Why: Create Project is intentionally Git-only; non-Git folders use the // existing add-folder flows instead of this path. const createKind = 'git' as const @@ -130,11 +132,7 @@ export function useCreateRepo( }) // Why: if the user closed the dialog or clicked Back mid-create, // createGenRef was bumped by resetCreateState. Ignore stale results. - if ( - gen !== createGenRef.current || - requestHostToken !== hostTokenRef.current || - !mountedRef.current - ) { + if (!isCurrent()) { return } if ('error' in result) { @@ -170,40 +168,23 @@ export function useCreateRepo( } ) } + const ownerOptions = worktreeRefreshOptions(options.runtimeEnvironmentId, options.sshTargetId) if (isGitRepoKind(repo)) { // Why: Git repos use the shared default-checkout completion path. // Why: if refresh is temporarily non-authoritative, the shared opener // still reveals the project so the user is not left in a completed add flow. - const ownerOptions = worktreeRefreshOptions( - options.runtimeEnvironmentId, - options.sshTargetId - ) await fetchWorktrees(repo.id, ownerOptions) - if ( - gen !== createGenRef.current || - requestHostToken !== hostTokenRef.current || - !mountedRef.current - ) { + if (!isCurrent()) { return } - await (ownerOptions.executionHostId - ? onGitRepoReady?.(repo.id, ownerOptions.executionHostId) - : onGitRepoReady?.(repo.id)) + await onGitRepoReady?.(repo.id, ownerOptions.executionHostId) } else { // Why: folder repos skip the Git default-checkout handoff, so activate the synthetic // root workspace before closing. Matches addNonGitFolder's behavior. - const ownerOptions = worktreeRefreshOptions( - options.runtimeEnvironmentId, - options.sshTargetId - ) await (ownerOptions.executionHostId ? fetchWorktrees(repo.id, { executionHostId: ownerOptions.executionHostId }) : fetchWorktrees(repo.id)) - if ( - gen !== createGenRef.current || - requestHostToken !== hostTokenRef.current || - !mountedRef.current - ) { + if (!isCurrent()) { return } const folderWorktree = useAppStore @@ -225,22 +206,14 @@ export function useCreateRepo( closeModal() } } catch (err) { - if ( - gen !== createGenRef.current || - requestHostToken !== hostTokenRef.current || - !mountedRef.current - ) { + if (!isCurrent()) { return } setCreateError(extractIpcErrorMessage(err, String(err))) } finally { // Why: only clear the loading state if this invocation is still current; // a superseded create must not flip the flag back off for a new flow. - if ( - gen === createGenRef.current && - requestHostToken === hostTokenRef.current && - mountedRef.current - ) { + if (isCurrent()) { setIsCreating(false) } } diff --git a/src/renderer/src/lib/bounded-generation-map.test.ts b/src/renderer/src/lib/bounded-generation-map.test.ts new file mode 100644 index 00000000000..7412a6de586 --- /dev/null +++ b/src/renderer/src/lib/bounded-generation-map.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest' +import { createBoundedGenerationMap } from './bounded-generation-map' + +describe('createBoundedGenerationMap', () => { + it('advances a key to a strictly larger generation', () => { + const map = createBoundedGenerationMap(4) + expect(map.get('a')).toBe(0) + map.advance('a') + const first = map.get('a') + map.advance('a') + expect(map.get('a')).toBeGreaterThan(first) + }) + + it('keeps insertion order for eviction when a key is re-advanced', () => { + const map = createBoundedGenerationMap(2) + map.advance('a') + map.advance('b') + map.advance('a') + map.advance('c') + // 'a' keeps its first-inserted slot, so it is evicted despite being re-advanced. + expect(map.get('b')).toBe(2) + expect(map.get('a')).toBe(3) + expect(map.get('c')).toBe(4) + }) + + it('reads an evicted key as the highest evicted generation', () => { + const map = createBoundedGenerationMap(1) + map.advance('a') + map.advance('a') + map.advance('b') + expect(map.get('a')).toBe(2) + expect(map.get('never-seen')).toBe(2) + }) + + it('reset clears generations, sequence and the evicted floor', () => { + const map = createBoundedGenerationMap(1) + map.advance('a') + map.advance('b') + map.reset() + expect(map.get('a')).toBe(0) + expect(map.get('b')).toBe(0) + map.advance('a') + expect(map.get('a')).toBe(1) + }) +}) diff --git a/src/renderer/src/lib/bounded-generation-map.ts b/src/renderer/src/lib/bounded-generation-map.ts new file mode 100644 index 00000000000..475bf580f58 --- /dev/null +++ b/src/renderer/src/lib/bounded-generation-map.ts @@ -0,0 +1,31 @@ +export type BoundedGenerationMap = { + get: (key: string) => number + advance: (key: string) => void + reset: () => void +} + +// Why: evicted keys read the highest evicted generation, so a stale snapshot never matches a fresh one. +export function createBoundedGenerationMap(max: number): BoundedGenerationMap { + const generations = new Map() + let sequence = 0 + let evicted = 0 + return { + get: (key) => generations.get(key) ?? evicted, + advance: (key) => { + generations.set(key, ++sequence) + while (generations.size > max) { + const oldest = generations.keys().next() + if (oldest.done) { + break + } + evicted = Math.max(evicted, generations.get(oldest.value) ?? 0) + generations.delete(oldest.value) + } + }, + reset: () => { + generations.clear() + sequence = 0 + evicted = 0 + } + } +} diff --git a/src/renderer/src/lib/project-host-setup-options.ts b/src/renderer/src/lib/project-host-setup-options.ts index d7a98c5f9bb..8776a50c935 100644 --- a/src/renderer/src/lib/project-host-setup-options.ts +++ b/src/renderer/src/lib/project-host-setup-options.ts @@ -15,6 +15,7 @@ import { } from '../../../shared/protocol-version' import type { ProjectHostSetup } from '../../../shared/project-types' import type { Repo } from '../../../shared/repo-types' +import { translate } from '@/i18n/i18n' export type ProjectHostSetupOption = | { @@ -180,7 +181,11 @@ function buildNeedsSetupOptions({ ) .map((host) => { const pendingSetup = pendingSetupByHost.get(host.id) - const availability = getHostSetupAvailability(host) + // Why: disconnected hosts cannot confirm project setup or runtime capabilities, + // so connection state needs to win over setup guidance. + const unavailableDetail = + getHostHealthUnavailableDetail(host.health) ?? getHostSetupUnavailableDetail(host) + const isAvailable = unavailableDetail === null const connectAction = getHostConnectAction(host) return { id: `needs-setup:${host.id}`, @@ -188,14 +193,12 @@ function buildNeedsSetupOptions({ projectId, hostId: host.id, label: host.label || getExecutionHostLabel(host.id), - detail: availability.isAvailable - ? pendingSetup - ? getPendingSetupDetail(pendingSetup) - : 'Project location not set' - : availability.detail, - isAvailable: availability.isAvailable, + detail: + unavailableDetail ?? + (pendingSetup ? getPendingSetupDetail(pendingSetup) : 'Project location not set'), + isAvailable, attention: host.health === 'error', - canSetLocation: canSetProjectLocation(projectId, availability.isAvailable, pendingSetup), + canSetLocation: canSetProjectLocation(projectId, isAvailable, pendingSetup), ...(connectAction ? { connectAction } : {}) } }) @@ -213,46 +216,32 @@ function isRuntimeOwnedSshSetupHost(hostId: ExecutionHostId): boolean { return parsed?.kind === 'ssh' && isRuntimeOwnedSshTargetId(parsed.targetId) } -function getHostSetupAvailability(host: ExecutionHostRegistryEntry): { - isAvailable: boolean - detail: string -} { +export function getHostSetupUnavailableDetail(host: ExecutionHostRegistryEntry): string | null { if (host.health === 'blocked') { - return { - isAvailable: false, - detail: 'Orca server version is incompatible' - } + return translate( + 'auto.components.settings.RepositoryPane.hostSetupBlockedVersion', + 'Orca server version is incompatible' + ) } - // Why: disconnected hosts cannot confirm project setup or runtime capabilities, - // so connection state needs to win over setup guidance. - const healthUnavailableDetail = getHostHealthUnavailableDetail(host.health) - if (healthUnavailableDetail) { - return { - isAvailable: false, - detail: healthUnavailableDetail - } + if (host.kind !== 'runtime') { + return null } - if (host.kind === 'runtime') { - if (!host.capabilities) { - return { - isAvailable: false, - detail: 'Checking host capabilities' - } - } - if ( - !host.capabilities.includes(PROJECT_HOST_SETUP_RUNTIME_CAPABILITY) || - !host.capabilities.includes(WORKSPACE_RUN_CONTEXT_RUNTIME_CAPABILITY) - ) { - return { - isAvailable: false, - detail: 'Update Orca on this host to set up projects' - } - } + if (!host.capabilities) { + return translate( + 'auto.components.settings.RepositoryPane.hostSetupCheckingCapability', + 'Checking host capabilities' + ) } - return { - isAvailable: true, - detail: '' + if ( + !host.capabilities.includes(PROJECT_HOST_SETUP_RUNTIME_CAPABILITY) || + !host.capabilities.includes(WORKSPACE_RUN_CONTEXT_RUNTIME_CAPABILITY) + ) { + return translate( + 'auto.components.settings.RepositoryPane.hostSetupMissingCapability', + 'Update Orca on this host to set up projects' + ) } + return null } function getHostHealthUnavailableDetail( diff --git a/src/renderer/src/lib/repo-slug-index.ts b/src/renderer/src/lib/repo-slug-index.ts index 1cdfbc17a70..752e2fdfe18 100644 --- a/src/renderer/src/lib/repo-slug-index.ts +++ b/src/renderer/src/lib/repo-slug-index.ts @@ -31,11 +31,11 @@ import { type SlugIndex } from './repo-slug-cache' import { githubRepoIdentityKey } from '../../../shared/github/repository-identity-key' +import { createBoundedGenerationMap } from './bounded-generation-map' export { lookupReposBySlugFromCache } from './repo-slug-cache' const slugResolutionInFlight = new Map>() -const MAX_SLUG_RESOLUTION_GENERATIONS = 1024 // Why: an invalidation (repo removed, remote changed) can land while a // resolution is in-flight — before it ever wrote to `slugByRepoId`. Deleting @@ -43,24 +43,11 @@ const MAX_SLUG_RESOLUTION_GENERATIONS = 1024 // stale slug would repopulate the cache after invalidation. Bump the key's // generation on every invalidation and commit a result only if the generation // it started with is still current. -const slugResolutionGeneration = new Map() -let slugResolutionGenerationSequence = 0 -let evictedSlugResolutionGeneration = 0 +const slugResolutionGenerations = createBoundedGenerationMap(1024) function invalidateSlugResolution(cacheKey: string): void { slugResolutionInFlight.delete(cacheKey) - slugResolutionGeneration.set(cacheKey, ++slugResolutionGenerationSequence) - while (slugResolutionGeneration.size > MAX_SLUG_RESOLUTION_GENERATIONS) { - const oldest = slugResolutionGeneration.keys().next() - if (oldest.done) { - return - } - evictedSlugResolutionGeneration = Math.max( - evictedSlugResolutionGeneration, - slugResolutionGeneration.get(oldest.value) ?? 0 - ) - slugResolutionGeneration.delete(oldest.value) - } + slugResolutionGenerations.advance(cacheKey) } // Why: clear after remove/remote-change so the next index build re-resolves. @@ -98,14 +85,14 @@ async function resolveRepoSlug( if (inFlight) { return inFlight } - const generation = slugResolutionGeneration.get(cacheKey) ?? evictedSlugResolutionGeneration + const generation = slugResolutionGenerations.get(cacheKey) const resolution = (async () => { // Why: only write the resolved value if this key wasn't invalidated // mid-flight; otherwise a stale slug would repopulate the cache. const commit = (value: string | null): string | null => { if ( slugResolutionInFlight.get(cacheKey) === resolution && - (slugResolutionGeneration.get(cacheKey) ?? evictedSlugResolutionGeneration) === generation + slugResolutionGenerations.get(cacheKey) === generation ) { rememberRepoSlug(cacheKey, value) } diff --git a/src/renderer/src/lib/workspace-session-host-persistence.test.ts b/src/renderer/src/lib/workspace-session-host-persistence.test.ts index e6dbc3f352e..41b0be72937 100644 --- a/src/renderer/src/lib/workspace-session-host-persistence.test.ts +++ b/src/renderer/src/lib/workspace-session-host-persistence.test.ts @@ -8,7 +8,6 @@ import { buildWorkspaceSessionHostSnapshots, patchWorkspaceSessionByHost, persistWorkspaceSessionByHost, - persistWorkspaceSessionByHostSync, type HostPersistenceState } from './workspace-session-host-persistence' import { @@ -423,7 +422,7 @@ describe('fetchWorkspaceSessionFromHosts', () => { expect(owner(worktreeWorkspaceKey(worktreeId))).toBe('runtime:env-1') }) - it('builds local-first host snapshots reused by synchronous persistence', () => { + it('builds local-first host snapshots', () => { const localWorktreeId = 'local-repo::C:\\src\\local' const remoteWorktreeId = 'remote-repo::/srv/remote' const makeTab = (id: string, worktreeId: string) => ({ @@ -473,13 +472,6 @@ describe('fetchWorkspaceSessionFromHosts', () => { expect(snapshots[1].state.tabsByWorktree).toEqual({ [remoteWorktreeId]: [expect.objectContaining({ id: 'remote-tab' })] }) - - const setSync = vi.fn() - persistWorkspaceSessionByHostSync({ get: vi.fn(), patch: vi.fn(), setSync }, payload, state) - - expect(setSync.mock.calls).toEqual( - snapshots.map((snapshot) => [snapshot.state, snapshot.hostId]) - ) }) }) @@ -704,7 +696,8 @@ describe('persistWorkspaceSessionByHost', () => { expect(set).toHaveBeenCalledWith( expect.objectContaining({ tabsByWorktree: { [localWorktreeId]: expect.any(Array) } - }) + }), + undefined ) expect(set).toHaveBeenCalledWith( expect.objectContaining({ diff --git a/src/renderer/src/lib/workspace-session-host-persistence.ts b/src/renderer/src/lib/workspace-session-host-persistence.ts index 909acdd4d69..a7603850266 100644 --- a/src/renderer/src/lib/workspace-session-host-persistence.ts +++ b/src/renderer/src/lib/workspace-session-host-persistence.ts @@ -93,13 +93,7 @@ function getFolderWorkspacePartitionHostId( : null const parsed = parseExecutionHostId(workspace?.executionHostId ?? group?.executionHostId) if (parsed) { - // Every non-local kind owns its own partition — the same answer main's - // RuntimeWorkspaceSessionController.getPreferredHostId gives for this key. Answering 'local' - // for an ssh host left the renderer and the runtime writing one folder workspace into two - // stores, which is #12723 unfixed for folder workspaces; and once the renderer began writing - // `ssh:` at all, a save's field-level patch erased the folder rows main had put - // there. Boot reads these partitions from persistence's own census, not the repo catalog, so - // a target whose only workspace is a folder is no longer unenumerated. + // Why: every non-local kind owns its own partition, matching main's getPreferredHostId (#12723). return parsed.id } if (workspace && group) { @@ -135,14 +129,6 @@ function buildRepoHostById( return repoHostById } -/** Map a worktree to the host partition it persists under, plus the host claims behind it. - * - * Why every non-local host and not just `runtime:*`: an SSH worktree's session is already - * read-modify-written into `ssh:` by the main-process runtime, so answering 'local' - * here double-owned the data and left whichever half the readers skipped round-tripping as - * absence (#12721, #12723). The one exception is an id two hosts both publish: it gets a - * deterministic primary so the co-claimant's rows can be parked in the shadow instead of sharing - * one bucket with it. */ /** True only when the catalog positively says `hostId` no longer holds the workspace. An id the * catalog cannot speak for yet keeps its restored partition — the same rule the shadow uses. */ function catalogReattributedAwayFrom( @@ -154,6 +140,14 @@ function catalogReattributedAwayFrom( return Boolean(claimed) && !contestedPartitionHosts(claimed ?? []).includes(hostId) } +/** Map a worktree to the host partition it persists under, plus the host claims behind it. + * + * Why every non-local host and not just `runtime:*`: an SSH worktree's session is already + * read-modify-written into `ssh:` by the main-process runtime, so answering 'local' + * here double-owned the data and left whichever half the readers skipped round-tripping as + * absence (#12721, #12723). The one exception is an id two hosts both publish: it gets a + * deterministic primary so the co-claimant's rows can be parked in the shadow instead of sharing + * one bucket with it. */ export function buildHostSessionRouting(state: HostPersistenceState): HostSessionRouting { const repoHostById = buildRepoHostById(state.repos) const claims = indexWorktreeHostClaims(state.worktreesByRepo, repoHostById) @@ -268,14 +262,9 @@ export async function persistWorkspaceSessionByHost( payload: WorkspaceSessionState, state: HostPersistenceState ): Promise { - // Why 'replace': api.set swaps the whole partition, so parked rows must ride along even for - // fields nothing else routed to this host. - const slices = splitWorkspaceSessionForWrite(payload, state, 'replace') - const writes: Promise[] = [api.set(slices[LOCAL_EXECUTION_HOST_ID] ?? payload)] - for (const [hostId, slice] of nonLocalHostSessionEntries(slices)) { - writes.push(api.set(slice, hostId)) - } - await Promise.all(writes) + await Promise.all( + buildWorkspaceSessionHostSnapshots(payload, state).map((s) => api.set(s.state, s.hostId)) + ) await api.flush() } @@ -294,14 +283,3 @@ export function buildWorkspaceSessionHostSnapshots( })) ] } - -/** Synchronous full-session split for the beforeunload / quit paths. */ -export function persistWorkspaceSessionByHostSync( - api: SessionApi, - payload: WorkspaceSessionState, - state: HostPersistenceState -): void { - for (const snapshot of buildWorkspaceSessionHostSnapshots(payload, state)) { - api.setSync(snapshot.state, snapshot.hostId) - } -} diff --git a/src/renderer/src/runtime/web-session-tabs-sync/state.ts b/src/renderer/src/runtime/web-session-tabs-sync/state.ts index 786b2b04e98..0fc3525e821 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/state.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/state.ts @@ -136,7 +136,6 @@ export const latestReceivedSessionTabsInventoryFrameByEnvironment = new Map() export const trackedSessionTabsWorktreeIdsByEnvironment = new Map>() export const sessionTabsEnvironmentsByWorktree = new Map>() -export const sessionTabsTrackingGenerationByEnvironment = new Map() export const lastHostTerminalTabCountByWorktree = new Map() export const MAX_TRACKED_SESSION_TABS_INVENTORY_OMISSIONS = 512 export type SessionTabsInventoryOmissionObservation = { diff --git a/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts b/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts index 68d9afe6386..a388e19172d 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts @@ -9,7 +9,6 @@ import { sessionTabsRuntimeHistoryByEnvironment, trackedSessionTabsWorktreeIdsByEnvironment, sessionTabsEnvironmentsByWorktree, - sessionTabsTrackingGenerationByEnvironment, lastHostTerminalTabCountByWorktree, sessionTabsInventoryOmissionsByWorktree, hostSessionTabIdByLocalKey, @@ -42,6 +41,7 @@ import { } from '../web-session-terminal-placement' import { clearHostSessionMirrorHydration } from '../host-session-mirror-hydration' import { clearHostMirrorHandleGapVerdictsForEnvironment } from '@/lib/host-mirror-handle-gap-wait' +import { createBoundedGenerationMap } from '@/lib/bounded-generation-map' import { clearHostSessionTabIdMappings } from './tracking-mappings' import { sessionTabsFreshnessKey, @@ -49,26 +49,7 @@ import { removeWebSessionTabsEnvironment } from './tracking' -const MAX_SESSION_TABS_TRACKING_GENERATIONS = 512 -let sessionTabsTrackingGenerationSequence = 0 -let evictedSessionTabsTrackingGeneration = 0 - -function advanceSessionTabsTrackingGeneration(environmentId: string): void { - const next = ++sessionTabsTrackingGenerationSequence - sessionTabsTrackingGenerationByEnvironment.set(environmentId, next) - while (sessionTabsTrackingGenerationByEnvironment.size > MAX_SESSION_TABS_TRACKING_GENERATIONS) { - const oldest = sessionTabsTrackingGenerationByEnvironment.keys().next() - if (oldest.done) { - break - } - const oldestEnvironmentId = oldest.value - evictedSessionTabsTrackingGeneration = Math.max( - evictedSessionTabsTrackingGeneration, - sessionTabsTrackingGenerationByEnvironment.get(oldestEnvironmentId) ?? 0 - ) - sessionTabsTrackingGenerationByEnvironment.delete(oldestEnvironmentId) - } -} +const sessionTabsTrackingGenerations = createBoundedGenerationMap(512) export function getLastKnownHostTerminalTabCount( environmentId: string, @@ -118,9 +99,7 @@ export function resetWebSessionTabsSnapshotFreshnessForTests(): void { hostSessionTabIdByLocalKey.clear() hostSessionTabMappingKeysByEnvironmentAndWorktree.clear() hostWorkingClientBoundaryByPaneKey.clear() - sessionTabsTrackingGenerationByEnvironment.clear() - sessionTabsTrackingGenerationSequence = 0 - evictedSessionTabsTrackingGeneration = 0 + sessionTabsTrackingGenerations.reset() resetWebSessionBrowserPlacementsForTests() } @@ -181,7 +160,7 @@ export function clearWebSessionTabsTrackingForEnvironment(environmentId: string) return } const keyPrefix = `${trimmedEnvironmentId}:` - advanceSessionTabsTrackingGeneration(trimmedEnvironmentId) + sessionTabsTrackingGenerations.advance(trimmedEnvironmentId) for (const key of latestSessionTabsSnapshotByWorktree.keys()) { if (key.startsWith(keyPrefix)) { latestSessionTabsSnapshotByWorktree.delete(key) @@ -245,5 +224,5 @@ export function clearWebSessionTabsTrackingForEnvironment(environmentId: string) export function getWebSessionTabsTrackingGeneration(environmentId: string): number { const key = environmentId.trim() - return sessionTabsTrackingGenerationByEnvironment.get(key) ?? evictedSessionTabsTrackingGeneration + return sessionTabsTrackingGenerations.get(key) } diff --git a/src/renderer/src/store/slices/runtime-environment-ssh.ts b/src/renderer/src/store/slices/runtime-environment-ssh.ts index dfaaedfe7ae..d68ccc64786 100644 --- a/src/renderer/src/store/slices/runtime-environment-ssh.ts +++ b/src/renderer/src/store/slices/runtime-environment-ssh.ts @@ -1,9 +1,13 @@ import type { StateCreator } from 'zustand' import type { AppState } from '../types' import type { SshConnectionState, SshTargetSummary } from '../../../../shared/ssh-types' -import { sanitizeSshTargetGeneration } from '../../../../shared/ssh-target-generation' import { sshConnectionStatesEqual } from './ssh-connection-state-equality' -import { sshTargetLabelsEqual } from './ssh-target-cleanup' +import { + collectSshTargetGenerations, + sshTargetGenerationsEqual, + sshTargetLabelsEqual +} from './ssh-target-cleanup' +import { createBoundedGenerationMap } from '../../lib/bounded-generation-map' export { selectRuntimeAwareSshConnectionGeneration, selectRuntimeAwareSshError, @@ -75,32 +79,8 @@ const EMPTY_BUCKET: RuntimeEnvironmentSshBucket = { targetsHydrated: false } -function collectTargetGenerations(targets: SshTargetSummary[]): Map { - const generations = new Map() - for (const target of targets) { - const generation = sanitizeSshTargetGeneration(target.generation) - if (generation !== undefined) { - generations.set(target.id, generation) - } - } - return generations -} - -function targetGenerationsEqual(current: Map, next: Map): boolean { - return ( - current.size === next.size && - [...next].every(([targetId, generation]) => current.get(targetId) === generation) - ) -} - -const stateGenerationByEnvironment = new Map() -const targetConnectionGenerationByEnvironment = new Map() -const MAX_SSH_STATE_GENERATIONS = 512 -const MAX_SSH_TARGET_GENERATIONS = 4096 -let targetConnectionGenerationSequence = 0 -let evictedTargetConnectionGeneration = 0 -let stateGenerationSequence = 0 -let evictedStateGeneration = 0 +const targetConnectionGenerations = createBoundedGenerationMap(4096) +const stateGenerations = createBoundedGenerationMap(512) function targetGenerationKey(environmentId: string, targetId: string): string { return `${environmentId}\0${targetId}` @@ -110,48 +90,18 @@ export function getEnvironmentSshTargetConnectionGeneration( environmentId: string, targetId: string ): number { - return ( - targetConnectionGenerationByEnvironment.get(targetGenerationKey(environmentId, targetId)) ?? - evictedTargetConnectionGeneration - ) + return targetConnectionGenerations.get(targetGenerationKey(environmentId, targetId)) } function advanceEnvironmentSshTargetConnectionGeneration( environmentId: string, targetId: string ): void { - const key = targetGenerationKey(environmentId, targetId) - targetConnectionGenerationByEnvironment.set(key, ++targetConnectionGenerationSequence) - while (targetConnectionGenerationByEnvironment.size > MAX_SSH_TARGET_GENERATIONS) { - const oldest = targetConnectionGenerationByEnvironment.keys().next() - if (oldest.done) { - break - } - evictedTargetConnectionGeneration = Math.max( - evictedTargetConnectionGeneration, - targetConnectionGenerationByEnvironment.get(oldest.value) ?? 0 - ) - targetConnectionGenerationByEnvironment.delete(oldest.value) - } + targetConnectionGenerations.advance(targetGenerationKey(environmentId, targetId)) } export function getEnvironmentSshStateGeneration(environmentId: string): number { - return stateGenerationByEnvironment.get(environmentId) ?? evictedStateGeneration -} - -function advanceEnvironmentSshStateGeneration(environmentId: string): void { - stateGenerationByEnvironment.set(environmentId, ++stateGenerationSequence) - while (stateGenerationByEnvironment.size > MAX_SSH_STATE_GENERATIONS) { - const oldest = stateGenerationByEnvironment.keys().next() - if (oldest.done) { - break - } - evictedStateGeneration = Math.max( - evictedStateGeneration, - stateGenerationByEnvironment.get(oldest.value) ?? 0 - ) - stateGenerationByEnvironment.delete(oldest.value) - } + return stateGenerations.get(environmentId) } function generationIsCurrent(environmentId: string, generation: number | undefined): boolean { @@ -225,10 +175,10 @@ export const createRuntimeEnvironmentSshSlice: StateCreator< const connectionStates = new Map( Array.from(bucket.connectionStates).filter(([targetId]) => targetIds.has(targetId)) ) - const targetGenerations = collectTargetGenerations(targets) + const targetGenerations = collectSshTargetGenerations(targets) if ( sshTargetLabelsEqual(bucket.targetLabels, targets) && - targetGenerationsEqual(bucket.targetGenerations, targetGenerations) + sshTargetGenerationsEqual(bucket.targetGenerations, targetGenerations) ) { // Why: an unchanged (even empty) list is still a successful load — the // hydration flag must flip on the first fetch of an empty target set. @@ -262,7 +212,7 @@ export const createRuntimeEnvironmentSshSlice: StateCreator< markEnvironmentSshStateStale: (environmentId) => set((s) => { - advanceEnvironmentSshStateGeneration(environmentId) + stateGenerations.advance(environmentId) const bucket = s.sshStateByEnvironment.get(environmentId) if ( !bucket || @@ -285,7 +235,7 @@ export const createRuntimeEnvironmentSshSlice: StateCreator< removeEnvironmentSshState: (environmentId) => set((s) => { - advanceEnvironmentSshStateGeneration(environmentId) + stateGenerations.advance(environmentId) if (!s.sshStateByEnvironment.has(environmentId)) { return s } @@ -301,7 +251,7 @@ export const createRuntimeEnvironmentSshSlice: StateCreator< const next = new Map(s.sshStateByEnvironment) for (const id of next.keys()) { if (!keep.has(id)) { - advanceEnvironmentSshStateGeneration(id) + stateGenerations.advance(id) next.delete(id) changed = true } diff --git a/src/renderer/src/store/slices/ssh-target-cleanup.ts b/src/renderer/src/store/slices/ssh-target-cleanup.ts index 294ad3f098d..515575b8042 100644 --- a/src/renderer/src/store/slices/ssh-target-cleanup.ts +++ b/src/renderer/src/store/slices/ssh-target-cleanup.ts @@ -83,34 +83,59 @@ function isRemovedSshTargetTabSession( return targetTabIds.has(tabId) || isSshTargetSessionId(sessionId, targetId) } +// Why: each returns undefined when nothing was removed, so the patch only carries changed fields. +function filterRecordIfChanged( + record: Record, + keep: (key: string, value: T) => boolean +): Record | undefined { + const entries = Object.entries(record) + const kept = entries.filter(([key, value]) => keep(key, value)) + return kept.length === entries.length ? undefined : Object.fromEntries(kept) +} + +function omitKey(record: Record, key: string): Record | undefined { + if (!Object.hasOwn(record, key)) { + return undefined + } + const { [key]: _removed, ...next } = record + return next +} + +function withoutMapKey(map: Map, key: string): Map | undefined { + const next = new Map(map) + return next.delete(key) ? next : undefined +} + +function withoutSetKey(set: Set, key: string): Set | undefined { + const next = new Set(set) + return next.delete(key) ? next : undefined +} + +function filterIfChanged(items: T[], keep: (item: T) => boolean): T[] | undefined { + const next = items.filter(keep) + return next.length === items.length ? undefined : next +} + function omitRemovedSshTargetTabSessions( sessions: Record, targetId: string, targetTabIds: Set -): { next: Record; removed: boolean } { - const next: Record = {} - let removed = false - for (const [tabId, sessionId] of Object.entries(sessions)) { - if (isRemovedSshTargetTabSession(tabId, sessionId, targetId, targetTabIds)) { - removed = true - continue - } - next[tabId] = sessionId - } - return { next, removed } +): Record | undefined { + return filterRecordIfChanged( + sessions, + (tabId, sessionId) => !isRemovedSshTargetTabSession(tabId, sessionId, targetId, targetTabIds) + ) } function omitRemovedSshTargetRecovery( entries: Record, targetId: string, targetTabIds: ReadonlySet -): { next: Record; removed: boolean } { - const next = Object.fromEntries( - Object.entries(entries).filter( - ([tabId, entry]) => !targetTabIds.has(tabId) && entry.authority.targetId !== targetId - ) +): Record | undefined { + return filterRecordIfChanged( + entries, + (tabId, entry) => !targetTabIds.has(tabId) && entry.authority.targetId !== targetId ) - return { next, removed: Object.keys(next).length !== Object.keys(entries).length } } function clearSshTargetTabPtyState( @@ -127,8 +152,12 @@ function clearSshTargetTabPtyState( > & { changed: boolean } { let nextTabsByWorktree = state.tabsByWorktree const nextPtyIdsByTabId = { ...state.ptyIdsByTabId } - const nextLastKnownRelayPtyIdByTabId = { ...state.lastKnownRelayPtyIdByTabId } - const nextPendingCodexPaneRestartIds = { ...state.pendingCodexPaneRestartIds } + const nextLastKnownRelayPtyIdByTabId = { + ...state.lastKnownRelayPtyIdByTabId + } + const nextPendingCodexPaneRestartIds = { + ...state.pendingCodexPaneRestartIds + } const nextCodexRestartNoticeByPtyId = { ...state.codexRestartNoticeByPtyId } let changed = false @@ -188,118 +217,73 @@ export function buildRemovedSshTargetCleanupPatch( targetId: string ): Partial | null { const targetTabIds = collectSshTargetTerminalTabIds(state, targetId) - const tabPtyState = clearSshTargetTabPtyState(state, targetId, targetTabIds) - const { next: nextDeferredSessions, removed: removedDeferredSession } = + const { changed: tabPtyChanged, ...tabPtyPatch } = clearSshTargetTabPtyState( + state, + targetId, + targetTabIds + ) + const patch: Partial = tabPtyChanged ? tabPtyPatch : {} + const assign = (key: K, value: AppState[K] | undefined): void => { + if (value !== undefined) { + patch[key] = value + } + } + assign( + 'deferredSshSessionIdsByTabId', omitRemovedSshTargetTabSessions(state.deferredSshSessionIdsByTabId, targetId, targetTabIds) + ) // Why: pending-reconnect holds each tab's pre-restart session until reconnect // drains it; if the target is removed first the entry is dead but the orphan // sweep now reads it as liveness, so clear it here too (#9911). - const { next: nextPendingReconnect, removed: removedPendingReconnect } = + assign( + 'pendingReconnectPtyIdByTabId', omitRemovedSshTargetTabSessions(state.pendingReconnectPtyIdByTabId, targetId, targetTabIds) - const { next: nextPaneRetries, removed: removedPaneRetries } = omitRemovedSshTargetRecovery( - state.directSshPaneRetryByTabId, - targetId, - targetTabIds ) - const { next: nextLiveBindings, removed: removedLiveBindings } = omitRemovedSshTargetRecovery( - state.directSshLivePtyBindingByTabId, - targetId, - targetTabIds + assign( + 'directSshPaneRetryByTabId', + omitRemovedSshTargetRecovery(state.directSshPaneRetryByTabId, targetId, targetTabIds) ) - const { next: nextRetryHistory, removed: removedRetryHistory } = omitRemovedSshTargetRecovery( - state.directSshPaneRetryHistoryByTabId, - targetId, - targetTabIds + assign( + 'directSshLivePtyBindingByTabId', + omitRemovedSshTargetRecovery(state.directSshLivePtyBindingByTabId, targetId, targetTabIds) ) - const nextPendingLayoutEdits = Object.fromEntries( - Object.entries(state.pendingDirectSshLayoutEditsByTabId ?? {}).filter( - ([, entry]) => entry.targetId !== targetId + assign( + 'directSshPaneRetryHistoryByTabId', + omitRemovedSshTargetRecovery(state.directSshPaneRetryHistoryByTabId, targetId, targetTabIds) + ) + assign( + 'pendingDirectSshLayoutEditsByTabId', + filterRecordIfChanged( + state.pendingDirectSshLayoutEditsByTabId ?? {}, + (_tabId, entry) => entry.targetId !== targetId ) ) - const removedPendingLayoutEdits = - Object.keys(nextPendingLayoutEdits).length !== - Object.keys(state.pendingDirectSshLayoutEditsByTabId ?? {}).length - - const nextDeferredTargets = state.deferredSshReconnectTargets.filter((id) => id !== targetId) - const nextTransientClearedConnections = { - ...state.transientClearedAgentStatusConnectionIds - } - const removedTransientClearBlock = Object.hasOwn(nextTransientClearedConnections, targetId) - delete nextTransientClearedConnections[targetId] - const nextConnectionStates = new Map(state.sshConnectionStates) - const removedConnectionState = nextConnectionStates.delete(targetId) - const nextLabels = new Map(state.sshTargetLabels) - const removedLabel = nextLabels.delete(targetId) + assign( + 'deferredSshReconnectTargets', + filterIfChanged(state.deferredSshReconnectTargets, (id) => id !== targetId) + ) + assign( + 'sshCredentialQueue', + filterIfChanged(state.sshCredentialQueue, (req) => req.targetId !== targetId) + ) + assign( + 'transientClearedAgentStatusConnectionIds', + omitKey(state.transientClearedAgentStatusConnectionIds, targetId) + ) + assign( + 'remoteWorkspaceSyncStatusByTargetId', + omitKey(state.remoteWorkspaceSyncStatusByTargetId, targetId) + ) + assign('portForwardsByConnection', omitKey(state.portForwardsByConnection, targetId)) + assign('detectedPortsByConnection', omitKey(state.detectedPortsByConnection, targetId)) + assign('sshConnectionStates', withoutMapKey(state.sshConnectionStates, targetId)) + assign('sshTargetLabels', withoutMapKey(state.sshTargetLabels, targetId)) // Why: a lingering generation would keep a deleted registration fenceable, and // the id is reissued fresh on re-add, so the old value can never become right. - const nextGenerations = new Map(state.sshTargetGenerations) - const removedGeneration = nextGenerations.delete(targetId) - const nextHydrated = new Set(state.remoteWorkspaceHydratedTargetIds) - const removedHydrated = nextHydrated.delete(targetId) - const removedSyncStatus = Object.hasOwn(state.remoteWorkspaceSyncStatusByTargetId, targetId) - const removedPortForwards = Object.hasOwn(state.portForwardsByConnection, targetId) - const removedDetectedPorts = Object.hasOwn(state.detectedPortsByConnection, targetId) - const nextSyncStatus = { ...state.remoteWorkspaceSyncStatusByTargetId } - delete nextSyncStatus[targetId] - const nextPortForwards = { ...state.portForwardsByConnection } - delete nextPortForwards[targetId] - const nextDetectedPorts = { ...state.detectedPortsByConnection } - delete nextDetectedPorts[targetId] - const nextCredentialQueue = state.sshCredentialQueue.filter((req) => req.targetId !== targetId) - const removedCredentialRequest = nextCredentialQueue.length !== state.sshCredentialQueue.length - const removedDeferredTarget = - nextDeferredTargets.length !== state.deferredSshReconnectTargets.length - const changed = - removedTransientClearBlock || - removedConnectionState || - removedLabel || - removedGeneration || - removedHydrated || - removedSyncStatus || - removedPortForwards || - removedDetectedPorts || - tabPtyState.changed || - removedCredentialRequest || - removedDeferredTarget || - removedDeferredSession || - removedPendingReconnect || - removedPaneRetries || - removedLiveBindings || - removedRetryHistory || - removedPendingLayoutEdits - if (!changed) { - return null - } - - return { - ...(removedTransientClearBlock - ? { transientClearedAgentStatusConnectionIds: nextTransientClearedConnections } - : {}), - ...(removedConnectionState ? { sshConnectionStates: nextConnectionStates } : {}), - ...(removedLabel ? { sshTargetLabels: nextLabels } : {}), - ...(removedGeneration ? { sshTargetGenerations: nextGenerations } : {}), - ...(removedHydrated ? { remoteWorkspaceHydratedTargetIds: nextHydrated } : {}), - ...(removedSyncStatus ? { remoteWorkspaceSyncStatusByTargetId: nextSyncStatus } : {}), - ...(removedPortForwards ? { portForwardsByConnection: nextPortForwards } : {}), - ...(removedDetectedPorts ? { detectedPortsByConnection: nextDetectedPorts } : {}), - ...(tabPtyState.changed - ? { - tabsByWorktree: tabPtyState.tabsByWorktree, - ptyIdsByTabId: tabPtyState.ptyIdsByTabId, - lastKnownRelayPtyIdByTabId: tabPtyState.lastKnownRelayPtyIdByTabId, - pendingCodexPaneRestartIds: tabPtyState.pendingCodexPaneRestartIds, - codexRestartNoticeByPtyId: tabPtyState.codexRestartNoticeByPtyId - } - : {}), - ...(removedCredentialRequest ? { sshCredentialQueue: nextCredentialQueue } : {}), - ...(removedDeferredTarget ? { deferredSshReconnectTargets: nextDeferredTargets } : {}), - ...(removedDeferredSession ? { deferredSshSessionIdsByTabId: nextDeferredSessions } : {}), - ...(removedPendingReconnect ? { pendingReconnectPtyIdByTabId: nextPendingReconnect } : {}), - ...(removedPaneRetries ? { directSshPaneRetryByTabId: nextPaneRetries } : {}), - ...(removedLiveBindings ? { directSshLivePtyBindingByTabId: nextLiveBindings } : {}), - ...(removedRetryHistory ? { directSshPaneRetryHistoryByTabId: nextRetryHistory } : {}), - ...(removedPendingLayoutEdits - ? { pendingDirectSshLayoutEditsByTabId: nextPendingLayoutEdits } - : {}) - } + assign('sshTargetGenerations', withoutMapKey(state.sshTargetGenerations, targetId)) + assign( + 'remoteWorkspaceHydratedTargetIds', + withoutSetKey(state.remoteWorkspaceHydratedTargetIds, targetId) + ) + return Object.keys(patch).length > 0 ? patch : null } diff --git a/src/renderer/src/store/slices/ssh.ts b/src/renderer/src/store/slices/ssh.ts index e4de2caafb7..c304fedf7cb 100644 --- a/src/renderer/src/store/slices/ssh.ts +++ b/src/renderer/src/store/slices/ssh.ts @@ -13,6 +13,7 @@ import { sshTargetLabelsEqual } from './ssh-target-cleanup' import { sshConnectionStatesEqual } from './ssh-connection-state-equality' +import { createBoundedGenerationMap } from '../../lib/bounded-generation-map' export type RemoteWorkspaceSyncStatus = { phase: 'idle' | 'pulling' | 'pushing' | 'synced' | 'conflict' | 'error' | 'offline' @@ -91,28 +92,10 @@ export type SshSlice = { setDetectedPorts: (targetId: string, ports: EnrichedDetectedPort[]) => void } -const targetConnectionGeneration = new Map() -const MAX_LOCAL_SSH_TARGET_GENERATIONS = 4096 -let targetConnectionGenerationSequence = 0 -let evictedTargetConnectionGeneration = 0 +const localTargetConnectionGenerations = createBoundedGenerationMap(4096) export function getLocalSshTargetConnectionGeneration(targetId: string): number { - return targetConnectionGeneration.get(targetId) ?? evictedTargetConnectionGeneration -} - -function advanceLocalSshTargetConnectionGeneration(targetId: string): void { - targetConnectionGeneration.set(targetId, ++targetConnectionGenerationSequence) - while (targetConnectionGeneration.size > MAX_LOCAL_SSH_TARGET_GENERATIONS) { - const oldest = targetConnectionGeneration.keys().next() - if (oldest.done) { - break - } - evictedTargetConnectionGeneration = Math.max( - evictedTargetConnectionGeneration, - targetConnectionGeneration.get(oldest.value) ?? 0 - ) - targetConnectionGeneration.delete(oldest.value) - } + return localTargetConnectionGenerations.get(targetId) } export const createSshSlice: StateCreator = (set) => ({ @@ -135,7 +118,7 @@ export const createSshSlice: StateCreator = (set) => if (sshConnectionStatesEqual(previous, state)) { return s } - advanceLocalSshTargetConnectionGeneration(targetId) + localTargetConnectionGenerations.advance(targetId) next.set(targetId, state) const didReconnect = previous?.status !== 'connected' && state.status === 'connected' let blockedConnections = s.transientClearedAgentStatusConnectionIds diff --git a/src/renderer/src/store/slices/stale-runtime-host-rows.test.ts b/src/renderer/src/store/slices/stale-runtime-host-rows.test.ts index 767763a9b1a..61b7bd42887 100644 --- a/src/renderer/src/store/slices/stale-runtime-host-rows.test.ts +++ b/src/renderer/src/store/slices/stale-runtime-host-rows.test.ts @@ -59,21 +59,21 @@ describe('dropWorktreeRowsForRemovedRuntimeEnvironments', () => { } const result = dropWorktreeRowsForRemovedRuntimeEnvironments(rowsByRepo, new Set(['env-a'])) expect(result.rowsByRepo.repo1).toEqual([row('w-local', 'local')]) - expect(result.removedWorktreeIds).toEqual(['w-a']) + expect(result.removedRows.map((r) => r.id)).toEqual(['w-a']) }) it('returns the SAME reference with empty ids when nothing matches', () => { const rowsByRepo = { repo1: [row('w-local', 'local'), row('w-b', runtimeB)] } const result = dropWorktreeRowsForRemovedRuntimeEnvironments(rowsByRepo, new Set(['env-a'])) expect(result.rowsByRepo).toBe(rowsByRepo) - expect(result.removedWorktreeIds).toEqual([]) + expect(result.removedRows.map((r) => r.id)).toEqual([]) }) it('returns the SAME reference with empty ids when the removed set is empty', () => { const rowsByRepo = { repo1: [row('w-a', runtimeA)] } const result = dropWorktreeRowsForRemovedRuntimeEnvironments(rowsByRepo, new Set()) expect(result.rowsByRepo).toBe(rowsByRepo) - expect(result.removedWorktreeIds).toEqual([]) + expect(result.removedRows.map((r) => r.id)).toEqual([]) }) it('keeps the repo key even when all of its rows drop', () => { @@ -81,7 +81,7 @@ describe('dropWorktreeRowsForRemovedRuntimeEnvironments', () => { const result = dropWorktreeRowsForRemovedRuntimeEnvironments(rowsByRepo, new Set(['env-a'])) expect(Object.keys(result.rowsByRepo)).toEqual(['repoA']) expect(result.rowsByRepo.repoA).toEqual([]) - expect(result.removedWorktreeIds).toEqual(['w-a1', 'w-a2']) + expect(result.removedRows.map((r) => r.id)).toEqual(['w-a1', 'w-a2']) }) it('gives only the changed repo a new array; unchanged repos keep their reference', () => { @@ -111,6 +111,6 @@ describe('dropWorktreeRowsForRemovedRuntimeEnvironments', () => { expect(result.rowsByRepo.removedRepo).toEqual([]) expect(result.rowsByRepo.ambiguousRepo).toEqual([row('w-ambiguous')]) - expect(result.removedWorktreeIds).toEqual(['w-legacy']) + expect(result.removedRows.map((r) => r.id)).toEqual(['w-legacy']) }) }) diff --git a/src/renderer/src/store/slices/stale-runtime-host-rows.ts b/src/renderer/src/store/slices/stale-runtime-host-rows.ts index 2b12fa5dcde..b0599b7902a 100644 --- a/src/renderer/src/store/slices/stale-runtime-host-rows.ts +++ b/src/renderer/src/store/slices/stale-runtime-host-rows.ts @@ -17,7 +17,7 @@ export function isRemovedRuntimeHostId( export type DropRuntimeRowsResult = { rowsByRepo: Record - removedWorktreeIds: string[] + removedRows: T[] } /** @@ -35,10 +35,10 @@ export function dropWorktreeRowsForRemovedRuntimeEnvironments< repoIdsWithoutSurvivingOwners?: ReadonlySet ): DropRuntimeRowsResult { if (removedEnvironmentIds.size === 0) { - return { rowsByRepo, removedWorktreeIds: [] } + return { rowsByRepo, removedRows: [] } } let changed = false - const removedWorktreeIds: string[] = [] + const removedRows: T[] = [] const next: Record = {} for (const [repoId, rows] of Object.entries(rowsByRepo)) { const survivors = rows.filter((row) => { @@ -48,7 +48,7 @@ export function dropWorktreeRowsForRemovedRuntimeEnvironments< isRemovedRuntimeHostId(row.hostId, removedEnvironmentIds) || (row.hostId === undefined && repoIdsWithoutSurvivingOwners?.has(repoId) === true) ) { - removedWorktreeIds.push(row.id) + removedRows.push(row) return false } return true @@ -58,7 +58,7 @@ export function dropWorktreeRowsForRemovedRuntimeEnvironments< changed = true } } - return changed ? { rowsByRepo: next, removedWorktreeIds } : { rowsByRepo, removedWorktreeIds: [] } + return changed ? { rowsByRepo: next, removedRows } : { rowsByRepo, removedRows: [] } } /** diff --git a/src/renderer/src/store/slices/worktrees/teardown/purge-stale-runtime-host-state.ts b/src/renderer/src/store/slices/worktrees/teardown/purge-stale-runtime-host-state.ts index 98a90486ea5..f5561f31500 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/purge-stale-runtime-host-state.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/purge-stale-runtime-host-state.ts @@ -144,41 +144,6 @@ export function createPurgeStaleRuntimeHostState( seenRemovedWorktreeTargets.add(identity) removedWorktreeTargets.push(hostId ? { id: worktreeId, hostId } : { id: worktreeId }) } - const recordRemovedRowTargets = ( - rowsByRepo: Record< - string, - readonly { - id: string - hostId?: ExecutionHostId - runtimeOwnerEnvironmentId?: string - }[] - > - ): void => { - for (const [repoId, rows] of Object.entries(rowsByRepo)) { - for (const row of rows) { - const removedOwner = - (row.runtimeOwnerEnvironmentId !== undefined && - removed.has(row.runtimeOwnerEnvironmentId)) || - isRemovedRuntimeHostId(row.hostId, removed) || - (row.hostId === undefined && repoIdsWithoutSurvivingOwners.has(repoId)) - if (!removedOwner) { - continue - } - // A row's execution host is the recency owner. Runtime ownership - // is the only host evidence available for legacy rows without it. - addRemovedWorktreeTarget( - row.id, - row.hostId ?? - (row.runtimeOwnerEnvironmentId - ? toRuntimeExecutionHostId(row.runtimeOwnerEnvironmentId) - : undefined) - ) - } - } - } - recordRemovedRowTargets(s.worktreesByRepo) - recordRemovedRowTargets(detectedRows) - const worktreeDrop = dropWorktreeRowsForRemovedRuntimeEnvironments( s.worktreesByRepo, removed, @@ -193,9 +158,20 @@ export function createPurgeStaleRuntimeHostState( const worktreesChanged = worktreeDrop.rowsByRepo !== s.worktreesByRepo const detectedChanged = detectedDrop.rowsByRepo !== detectedRows + const removedRows = [...worktreeDrop.removedRows, ...detectedDrop.removedRows] + for (const row of removedRows) { + // A row's execution host is the recency owner. Runtime ownership + // is the only host evidence available for legacy rows without it. + addRemovedWorktreeTarget( + row.id, + row.hostId ?? + (row.runtimeOwnerEnvironmentId + ? toRuntimeExecutionHostId(row.runtimeOwnerEnvironmentId) + : undefined) + ) + } const removedWorktreeIds = new Set([ - ...worktreeDrop.removedWorktreeIds, - ...detectedDrop.removedWorktreeIds, + ...removedRows.map((row) => row.id), ...sessionWorktreeIdsOwnedByRemovedHosts ]) // Why: terminal tabs hydrate before worktree metadata, so session-only ids for owner-less repos still need purging. @@ -212,14 +188,10 @@ export function createPurgeStaleRuntimeHostState( } } // Why: bare-id state follows an exact survivor unless the restored-session partition proves it belonged to the removed host. - for (const rows of Object.values(worktreeDrop.rowsByRepo)) { - for (const row of rows) { - if (!sessionWorktreeIdsOwnedByRemovedHosts.has(row.id)) { - removedWorktreeIds.delete(row.id) - } - } - } - for (const rows of Object.values(detectedDrop.rowsByRepo)) { + for (const rows of [ + ...Object.values(worktreeDrop.rowsByRepo), + ...Object.values(detectedDrop.rowsByRepo) + ]) { for (const row of rows) { if (!sessionWorktreeIdsOwnedByRemovedHosts.has(row.id)) { removedWorktreeIds.delete(row.id)