From 96d77b37c5371f15f8dc9263f70c6d4e47be221f Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 16 Sep 2026 00:46:46 -0400 Subject: [PATCH] perf: always show project names and remove notification scans (#20931) * perf: avoid repeated agent scans when labeling notifications * perf: always label notifications and remove project counting * fix: qualify the notification project group by its folder's host Folder notifications resolved the folder host-aware, then looked its project group up by bare ID. The owner index fails a bare ID closed when two hosts publish the same group ID, so a remote folder lost the project name the catalog already had. Also drops the identity rescans that recovered display fields: the catalog finders now return the caller's row type, matching findIndexedRepoOwnerForHost. Updates the idle-arbitration expectation that still asserted the removed hasMultipleActiveRepos flag. --- src/main/ipc/notification-options.ts | 2 +- .../notifications-message-formatting.test.ts | 77 +++++---- ...y-connection-hook-idle-arbitration.test.ts | 1 - .../terminal-notification-state.test.ts | 160 ++++++++++++++++++ .../terminal-notification-state.ts | 103 ++++------- .../use-notification-dispatch.ts | 14 +- .../src/lib/worktree-runtime-owner-index.ts | 18 +- src/shared/notification-settings-types.ts | 1 + 8 files changed, 246 insertions(+), 130 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/terminal-notification-state.test.ts diff --git a/src/main/ipc/notification-options.ts b/src/main/ipc/notification-options.ts index deb0b93fd2f..a19f6044a46 100644 --- a/src/main/ipc/notification-options.ts +++ b/src/main/ipc/notification-options.ts @@ -87,7 +87,7 @@ function formatNotificationWorktreeContext(args: NotificationDispatchRequest): s NOTIFICATION_TITLE_CONTEXT_MAX_LENGTH ) const repoLabel = normalizeNotificationText(args.repoLabel, NOTIFICATION_TITLE_CONTEXT_MAX_LENGTH) - if (args.hasMultipleActiveRepos && repoLabel && worktreeLabel) { + if (repoLabel && worktreeLabel) { return normalizeNotificationText( `${repoLabel} / ${worktreeLabel}`, NOTIFICATION_TITLE_CONTEXT_MAX_LENGTH diff --git a/src/main/ipc/notifications-message-formatting.test.ts b/src/main/ipc/notifications-message-formatting.test.ts index 677c3131203..abf41bfae3c 100644 --- a/src/main/ipc/notifications-message-formatting.test.ts +++ b/src/main/ipc/notifications-message-formatting.test.ts @@ -96,43 +96,6 @@ describe('registerNotificationHandlers', () => { ) ).toEqual({ delivered: true }) - expect(notificationCtorMock).toHaveBeenCalledWith( - expectedNativeNotificationOptions({ - title: 'feat/notis - Codex finished', - body: 'Updated the notification body.' - }) - ) - }) - - it('includes the repo name when multiple repos are active', async () => { - registerNotificationHandlers({ - getSettings: () => ({ - notifications: { - enabled: true, - agentTaskComplete: true, - terminalBell: false, - suppressWhenFocused: true - } - }) - } as never) - - const handler = getDispatchHandler() - expect( - await handler( - {}, - { - source: 'agent-task-complete', - worktreeId: 'repo::wt1', - worktreeLabel: 'feat/notis', - repoLabel: 'orca', - hasMultipleActiveRepos: true, - agentType: 'codex', - agentState: 'done', - agentLastAssistantMessage: 'Updated the notification body.' - } - ) - ).toEqual({ delivered: true }) - expect(notificationCtorMock).toHaveBeenCalledWith( expectedNativeNotificationOptions({ title: 'orca / feat/notis - Codex finished', @@ -141,6 +104,46 @@ describe('registerNotificationHandlers', () => { ) }) + it.each([true, false, undefined])( + 'includes the repo name regardless of the legacy multiple-repo flag (%s)', + async (hasMultipleActiveRepos) => { + registerNotificationHandlers({ + getSettings: () => ({ + notifications: { + enabled: true, + agentTaskComplete: true, + terminalBell: false, + suppressWhenFocused: true + } + }) + } as never) + + const handler = getDispatchHandler() + expect( + await handler( + {}, + { + source: 'agent-task-complete', + worktreeId: 'repo::wt1', + worktreeLabel: 'feat/notis', + repoLabel: 'orca', + hasMultipleActiveRepos, + agentType: 'codex', + agentState: 'done', + agentLastAssistantMessage: 'Updated the notification body.' + } + ) + ).toEqual({ delivered: true }) + + expect(notificationCtorMock).toHaveBeenCalledWith( + expectedNativeNotificationOptions({ + title: 'orca / feat/notis - Codex finished', + body: 'Updated the notification body.' + }) + ) + } + ) + it('keeps a readable body when no assistant response was captured', async () => { registerNotificationHandlers({ getSettings: () => ({ diff --git a/src/renderer/src/components/terminal-pane/pty-connection-hook-idle-arbitration.test.ts b/src/renderer/src/components/terminal-pane/pty-connection-hook-idle-arbitration.test.ts index ce5834f100a..1b6cfd4f049 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection-hook-idle-arbitration.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection-hook-idle-arbitration.test.ts @@ -218,7 +218,6 @@ describe('connectPanePty', () => { worktreeId: 'wt-1', repoLabel: 'orca', worktreeLabel: 'feat/notis', - hasMultipleActiveRepos: true, terminalTitle: '* Claude done', agentType: 'claude', agentState: 'done', diff --git a/src/renderer/src/components/terminal-pane/terminal-notification-state.test.ts b/src/renderer/src/components/terminal-pane/terminal-notification-state.test.ts new file mode 100644 index 00000000000..e09f6dade3a --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-notification-state.test.ts @@ -0,0 +1,160 @@ +// @vitest-environment happy-dom +import { describe, expect, it } from 'vitest' +import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants' +import { useAppStore } from '@/store' +import { makeFolderWorkspace, makeWorktree } from '@/store/slices/worktrees-slice-test-fixtures' +import { getNotificationWorkspaceLabels } from './terminal-notification-state' + +function stateWithWorkspace() { + return { + ...useAppStore.getInitialState(), + worktreesByRepo: { repo: [makeWorktree({ id: 'wt', repoId: 'repo', displayName: 'Feature' })] }, + repos: [ + { + id: 'repo', + displayName: 'Orca', + path: '/orca', + connectionId: null, + badgeColor: 'blue', + addedAt: 0 + } + ] + } +} + +describe('notification workspace labels', () => { + it('includes the only project without reading agent inventories', () => { + const state = stateWithWorkspace() + Object.defineProperty(state, 'agentStatusByPaneKey', { + get() { + throw new Error('agent scan') + } + }) + Object.defineProperty(state, 'retainedAgentsByPaneKey', { + get() { + throw new Error('retained scan') + } + }) + expect(getNotificationWorkspaceLabels(state, 'wt')).toEqual({ + repoLabel: 'Orca', + worktreeLabel: 'Feature' + }) + expect(getNotificationWorkspaceLabels(state, 'worktree:wt')).toEqual({ + repoLabel: 'Orca', + worktreeLabel: 'Feature' + }) + }) + + it('keeps labels for remote Git workspaces', () => { + const state = stateWithWorkspace() + state.worktreesByRepo.repo = [ + makeWorktree({ + id: 'remote', + repoId: 'repo', + hostId: 'ssh:server', + displayName: 'Remote feature' + }) + ] + expect(getNotificationWorkspaceLabels(state, 'remote')).toEqual({ + repoLabel: 'Orca', + worktreeLabel: 'Remote feature' + }) + }) + + it.each([undefined, 'ssh:server'] as const)( + 'resolves folder and project names on host %s', + (executionHostId) => { + const state = stateWithWorkspace() + state.folderWorkspaces = [ + makeFolderWorkspace({ + id: 'folder-id', + projectGroupId: 'group', + name: 'Website', + executionHostId + }) + ] + state.projectGroups = [ + { + id: 'group', + name: 'Personal', + executionHostId, + parentPath: null, + parentGroupId: null, + createdFrom: 'manual', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 0, + updatedAt: 0 + } + ] + expect(getNotificationWorkspaceLabels(state, 'folder:folder-id')).toEqual({ + repoLabel: 'Personal', + worktreeLabel: 'Website' + }) + state.projectGroups = [] + expect(getNotificationWorkspaceLabels(state, 'folder:folder-id')).toEqual({ + repoLabel: undefined, + worktreeLabel: 'Website' + }) + } + ) + + it.each([false, true])( + 'qualifies project groups by the folder host (legacy SSH: %s)', + (legacy) => { + const state = stateWithWorkspace() + state.folderWorkspaces = [ + makeFolderWorkspace({ + id: 'remote-folder', + name: 'Remote folder', + projectGroupId: 'shared', + ...(legacy ? { connectionId: 'server' } : { executionHostId: 'ssh:server' as const }) + }) + ] + state.projectGroups = (['local', 'ssh:server'] as const).map((executionHostId) => ({ + id: 'shared', + name: executionHostId === 'local' ? 'Local group' : 'Remote group', + executionHostId, + parentPath: null, + parentGroupId: null, + createdFrom: 'manual' as const, + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 0, + updatedAt: 0 + })) + expect(getNotificationWorkspaceLabels(state, 'folder:remote-folder')).toEqual({ + repoLabel: 'Remote group', + worktreeLabel: 'Remote folder' + }) + } + ) + + it('does not pick an arbitrary folder when hosts have conflicting records', () => { + const state = stateWithWorkspace() + state.folderWorkspaces = (['ssh:a', 'ssh:b'] as const).map((executionHostId) => + makeFolderWorkspace({ id: 'duplicate', name: executionHostId, executionHostId }) + ) + expect(getNotificationWorkspaceLabels(state, 'folder:duplicate', 'Terminal')).toEqual({ + repoLabel: undefined, + worktreeLabel: 'Terminal' + }) + }) + + it.each(['folder:missing', 'missing-worktree', FLOATING_TERMINAL_WORKTREE_ID])( + 'uses readable fallbacks for %s', + (id) => { + const state = stateWithWorkspace() + expect(getNotificationWorkspaceLabels(state, id, 'My terminal')).toEqual({ + repoLabel: undefined, + worktreeLabel: 'My terminal' + }) + expect(getNotificationWorkspaceLabels(state, id, ' ')).toEqual({ + repoLabel: undefined, + worktreeLabel: 'workspace' + }) + } + ) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-notification-state.ts b/src/renderer/src/components/terminal-pane/terminal-notification-state.ts index ac2d1c6d227..db24696115c 100644 --- a/src/renderer/src/components/terminal-pane/terminal-notification-state.ts +++ b/src/renderer/src/components/terminal-pane/terminal-notification-state.ts @@ -1,7 +1,11 @@ -import { isExplicitAgentStatusFresh } from '@/lib/agent-status' import type { useAppStore } from '@/store' -import { getWorktreeMapFromState } from '@/store/selectors' -import { AGENT_STATUS_STALE_AFTER_MS } from '../../../../shared/agent-status-types' +import { getRepoMapFromState, getWorktreeMapFromState } from '@/store/selectors' +import { + findIndexedFolderWorkspaceOwner, + findIndexedProjectGroupOwner, + getCatalogOwnerHostId +} from '@/lib/worktree-runtime-owner-index' +import { parseWorkspaceKey } from '../../../../shared/workspace-scope' import { parsePaneKey } from '../../../../shared/stable-pane-id' import type { TerminalPaneLayoutNode } from '../../../../shared/terminal-tab-types' @@ -144,74 +148,31 @@ export function isCurrentKnownPaneKey( return ptyHints.length === 0 || ptyHints.some((ptyId) => !isSuppressedPtyHint(state, ptyId)) } -function hasActiveWorktreeState(state: StoreSnapshot, worktreeId: string): boolean { - if (hasLivePtyForWorktree(state, worktreeId)) { - return true +export function getNotificationWorkspaceLabels( + state: StoreSnapshot, + workspaceId: string, + terminalTitle?: string +): { repoLabel?: string; worktreeLabel: string } { + const scope = parseWorkspaceKey(workspaceId) + const fallback = terminalTitle?.trim() || 'workspace' + if (scope?.type === 'folder') { + const folder = findIndexedFolderWorkspaceOwner(state.folderWorkspaces, scope.folderWorkspaceId) + // The group ID is only unique per host, so qualify it with the folder's own host. + const group = + folder && + findIndexedProjectGroupOwner( + state.projectGroups, + folder.projectGroupId, + getCatalogOwnerHostId(folder) + ) + return { repoLabel: group?.name, worktreeLabel: folder?.name || fallback } } - - if ((state.browserTabsByWorktree?.[worktreeId] ?? []).length > 0) { - return true + const worktree = getWorktreeMapFromState(state).get( + scope?.type === 'worktree' ? scope.worktreeId : workspaceId + ) + const repo = worktree ? getRepoMapFromState(state).get(worktree.repoId) : undefined + return { + repoLabel: repo?.displayName, + worktreeLabel: worktree?.displayName || worktree?.branch || fallback } - - const worktree = getWorktreeMapFromState(state).get(worktreeId) - if (worktree?.workspaceStatus === 'in-progress') { - return true - } - - if ( - Object.values(state.retainedAgentsByPaneKey ?? {}).some( - (agent) => agent.worktreeId === worktreeId - ) - ) { - return true - } - - const tabs = state.tabsByWorktree[worktreeId] ?? [] - const tabIds = new Set(tabs.map((tab) => tab.id)) - if (tabIds.size === 0) { - return false - } - - const now = Date.now() - return Object.values(state.agentStatusByPaneKey ?? {}).some((entry) => { - const tabId = getPaneKeyTabId(entry.paneKey) - return ( - tabId !== null && - tabIds.has(tabId) && - isExplicitAgentStatusFresh(entry, now, AGENT_STATUS_STALE_AFTER_MS) - ) - }) -} - -function countReposWithWorktrees(state: StoreSnapshot): number { - let count = 0 - for (const worktrees of Object.values(state.worktreesByRepo)) { - if (worktrees.length > 0) { - count += 1 - } - } - return count -} - -export function countReposNeedingNotificationDisambiguation(state: StoreSnapshot): number { - const activeRepoIds = new Set() - const worktreeMap = getWorktreeMapFromState(state) - for (const worktreeId of Object.keys(state.tabsByWorktree)) { - if (!hasActiveWorktreeState(state, worktreeId)) { - continue - } - const repoId = worktreeMap.get(worktreeId)?.repoId - if (repoId) { - activeRepoIds.add(repoId) - } - } - for (const [repoId, worktrees] of Object.entries(state.worktreesByRepo)) { - if (activeRepoIds.has(repoId)) { - continue - } - if (worktrees.some((worktree) => hasActiveWorktreeState(state, worktree.id))) { - activeRepoIds.add(repoId) - } - } - return Math.max(activeRepoIds.size, countReposWithWorktrees(state)) } diff --git a/src/renderer/src/components/terminal-pane/use-notification-dispatch.ts b/src/renderer/src/components/terminal-pane/use-notification-dispatch.ts index 3c4070b44aa..f15c5067925 100644 --- a/src/renderer/src/components/terminal-pane/use-notification-dispatch.ts +++ b/src/renderer/src/components/terminal-pane/use-notification-dispatch.ts @@ -1,7 +1,6 @@ import { useCallback } from 'react' import { useAppStore } from '@/store' import { resolveCommittedTitleAgentType } from '@/lib/pane-agent-evidence' -import { getRepoMapFromState, getWorktreeMapFromState } from '@/store/selectors' import { playDesktopNotificationSound } from '@/lib/desktop-notification-sound' import { showBlockedNotificationFallbackToast } from '@/lib/blocked-notification-fallback' import { buildAgentNotificationId } from '../../../../shared/agent-notification-id' @@ -15,7 +14,7 @@ import type { AgentCompletionDispatchMeta, AgentCompletionStatusSnapshot } from './agent-completion-coordinator-types' -import { countReposNeedingNotificationDisambiguation } from './terminal-notification-state' +import { getNotificationWorkspaceLabels } from './terminal-notification-state' import { createTerminalAttentionSurface } from './terminal-attention-surface' import { applyAgentAttention, @@ -134,13 +133,6 @@ export function dispatchTerminalNotification( // Desktop settings are applied in main after independent mobile delivery. - // Why: prefer worktree.repoId over string-parsing the worktreeId. The - // `${repoId}::${path}` format is an implementation detail of id - // construction; coupling the notification dispatcher to it would silently - // drop the repo label if that format ever changes. The worktree object - // itself is the source of truth for its owning repo. - const worktree = getWorktreeMapFromState(state).get(worktreeId) - const repo = worktree ? getRepoMapFromState(state).get(worktree.repoId) : null const customSoundId = state.settings?.notifications?.customSoundId ?? 'system' const customSoundVolume = state.settings?.notifications?.customSoundVolume ?? null // Why: pane keys are reused across turns. A rich OS notification must not @@ -175,9 +167,7 @@ export function dispatchTerminalNotification( ...(notificationId ? { notificationId } : {}), worktreeId: request.workspaceId, paneKey: request.subjectKey ?? undefined, - repoLabel: repo?.displayName, - worktreeLabel: worktree?.displayName || worktree?.branch || worktreeId, - hasMultipleActiveRepos: countReposNeedingNotificationDisambiguation(state) > 1, + ...getNotificationWorkspaceLabels(state, request.workspaceId, event.terminalTitle), terminalTitle: event.terminalTitle, isActiveWorktree: request.workspaceIsActive, ...agentSnapshot diff --git a/src/renderer/src/lib/worktree-runtime-owner-index.ts b/src/renderer/src/lib/worktree-runtime-owner-index.ts index cddb3b81d90..df5aa8551eb 100644 --- a/src/renderer/src/lib/worktree-runtime-owner-index.ts +++ b/src/renderer/src/lib/worktree-runtime-owner-index.ts @@ -279,11 +279,11 @@ export function findIndexedRepoOwnerForHost( return resolution?.kind === 'resolved' ? (resolution.owner as T) : null } -export function findIndexedFolderWorkspaceOwner( - folderWorkspaces: readonly FolderWorkspaceOwnerRecord[] | undefined, +export function findIndexedFolderWorkspaceOwner( + folderWorkspaces: readonly T[] | undefined, folderWorkspaceId: string, executionHostId?: ExecutionHostId -): FolderWorkspaceOwnerRecord | null { +): T | null { if (!folderWorkspaces) { return null } @@ -295,14 +295,15 @@ export function findIndexedFolderWorkspaceOwner( const resolution = index.get( executionHostId ? `${folderWorkspaceId}\0${executionHostId}` : folderWorkspaceId ) - return resolution?.kind === 'resolved' ? resolution.owner : null + // The cache is keyed by this exact array, so its owner retains the caller's row type. + return resolution?.kind === 'resolved' ? (resolution.owner as T) : null } -export function findIndexedProjectGroupOwner( - projectGroups: readonly ProjectGroupOwnerRecord[] | undefined, +export function findIndexedProjectGroupOwner( + projectGroups: readonly T[] | undefined, projectGroupId: string, executionHostId?: ExecutionHostId -): ProjectGroupOwnerRecord | null { +): T | null { if (!projectGroups) { return null } @@ -314,5 +315,6 @@ export function findIndexedProjectGroupOwner( const resolution = index.get( executionHostId ? `${projectGroupId}\0${executionHostId}` : projectGroupId ) - return resolution?.kind === 'resolved' ? resolution.owner : null + // The cache is keyed by this exact array, so its owner retains the caller's row type. + return resolution?.kind === 'resolved' ? (resolution.owner as T) : null } diff --git a/src/shared/notification-settings-types.ts b/src/shared/notification-settings-types.ts index fe4ef74015c..1c06b90d8db 100644 --- a/src/shared/notification-settings-types.ts +++ b/src/shared/notification-settings-types.ts @@ -33,6 +33,7 @@ export type NotificationDispatchRequest = { paneKey?: string repoLabel?: string worktreeLabel?: string + /** Legacy senders may still provide this; project labels are now always shown. */ hasMultipleActiveRepos?: boolean terminalTitle?: string isActiveWorktree?: boolean