From fa449bc0ef5301a64bcd0357605819b5fcfa0256 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:56:26 -0700 Subject: [PATCH] fix(worktree-palette): stop blanked display names from crashing Cmd+J (#11323) * fix(worktree-palette): stop blanked display names from crashing Cmd+J Blanking the "Display Name" field made buildWorktreeMetaUpdates emit `displayName: undefined` as a present key. The store's `{ ...worktree, ...updates }` spread then erased the live name, so the next palette keystroke threw "Cannot read properties of undefined (reading 'toLowerCase')" in searchWorktrees (crash a1f81ea1, build 1.4.159). Fixed at three layers so no single guard is load-bearing: - Producer: persist the blanking intent as '' instead of undefined, and let WorktreeSet accept '' so remote/SSH hosts stop dropping the clear. - Store: applyWorktreeUpdates and applyDetectedWorktreeUpdates drop present-but-undefined keys for fields Worktree declares required. - Readers: resolveWorktreeDisplayName/resolveWorktreeBranchLabel mirror the main-side mergeWorktree fallback (custom -> branch -> folder) for all four Cmd+J searches, the checks/review index, and the render site. Co-authored-by: Orca * test(worktree): assert omitted display name shape --------- Co-authored-by: Orca --- .../rpc/methods/worktree-schemas.test.ts | 25 ++++- .../runtime/rpc/methods/worktree-schemas.ts | 5 +- .../src/components/WorktreeJumpPalette.tsx | 14 ++- .../worktree-checks-review-index.test.ts | 21 ++++ .../cmd-j/worktree-checks-review-index.ts | 6 +- .../sidebar/worktree-meta-updates.test.ts | 13 +++ .../sidebar/worktree-meta-updates.ts | 8 +- .../src/lib/browser-palette-search.test.ts | 50 ++++++++++ .../src/lib/browser-palette-search.ts | 7 +- .../src/lib/simulator-palette-search.test.ts | 45 +++++++++ .../src/lib/simulator-palette-search.ts | 7 +- .../src/lib/workspace-tab-palette-results.ts | 7 +- .../lib/workspace-tab-palette-search.test.ts | 32 +++++++ .../lib/worktree-default-display-name.test.ts | 95 +++++++++++++++++++ .../src/lib/worktree-default-display-name.ts | 34 +++++++ .../src/lib/worktree-palette-search.test.ts | 57 +++++++++++ .../src/lib/worktree-palette-search.ts | 11 ++- .../src/store/slices/worktree-helpers.test.ts | 17 ++++ .../src/store/slices/worktree-helpers.ts | 42 +++++++- src/renderer/src/store/slices/worktrees.ts | 5 +- 20 files changed, 478 insertions(+), 23 deletions(-) create mode 100644 src/renderer/src/lib/worktree-default-display-name.test.ts create mode 100644 src/renderer/src/lib/worktree-default-display-name.ts diff --git a/src/main/runtime/rpc/methods/worktree-schemas.test.ts b/src/main/runtime/rpc/methods/worktree-schemas.test.ts index 975693bb7..219791cb9 100644 --- a/src/main/runtime/rpc/methods/worktree-schemas.test.ts +++ b/src/main/runtime/rpc/methods/worktree-schemas.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { WorktreeActivate, WorktreeCreate } from './worktree-schemas' +import { WorktreeActivate, WorktreeCreate, WorktreeSet } from './worktree-schemas' describe('worktree RPC schemas', () => { it('validates additive navigation intent', () => { @@ -31,4 +31,27 @@ describe('worktree RPC schemas', () => { expect(parsed.success).toBe(false) }) + + it('keeps a blanked display name on remote hosts instead of dropping the clear', () => { + // Blanking sends displayName:'' meaning "fall back to the branch/folder name". + // Coercing it to undefined made updateManagedWorktreeMeta's omitUndefinedProperties + // drop the key, so an SSH/paired-web rename-to-blank silently kept the old name. + const parsed = WorktreeSet.parse({ worktree: 'id:r1::/repos/wt', displayName: '' }) + + expect(parsed.displayName).toBe('') + expect(Object.prototype.hasOwnProperty.call(parsed, 'displayName')).toBe(true) + }) + + it('still omits a display name that was never sent', () => { + const parsed = WorktreeSet.parse({ worktree: 'id:r1::/repos/wt', comment: 'note' }) + + expect(parsed.displayName).toBeUndefined() + expect(Object.prototype.hasOwnProperty.call(parsed, 'displayName')).toBe(false) + }) + + it('ignores a non-string display name rather than persisting it', () => { + const parsed = WorktreeSet.parse({ worktree: 'id:r1::/repos/wt', displayName: 42 }) + + expect(parsed.displayName).toBeUndefined() + }) }) diff --git a/src/main/runtime/rpc/methods/worktree-schemas.ts b/src/main/runtime/rpc/methods/worktree-schemas.ts index b79fb320f..905a58f95 100644 --- a/src/main/runtime/rpc/methods/worktree-schemas.ts +++ b/src/main/runtime/rpc/methods/worktree-schemas.ts @@ -189,7 +189,10 @@ export const WorktreePrefetchCreateBase = z.object({ }) export const WorktreeSet = WorktreeSelector.extend({ - displayName: OptionalString, + // Why: '' is the blanking contract — "fall back to the branch/folder name". + // OptionalString coerced it to undefined, so on remote/SSH hosts clearing the + // name was dropped here and the old name came back on the next refresh. + displayName: OptionalPlainString, // Why: empty comments are meaningful metadata updates, so use the plain // string parser instead of OptionalString's empty-as-undefined behavior. comment: OptionalPlainString, diff --git a/src/renderer/src/components/WorktreeJumpPalette.tsx b/src/renderer/src/components/WorktreeJumpPalette.tsx index 61f3a4eb5..efe91e4e7 100644 --- a/src/renderer/src/components/WorktreeJumpPalette.tsx +++ b/src/renderer/src/components/WorktreeJumpPalette.tsx @@ -23,7 +23,6 @@ import { CommandEmpty, CommandItem } from '@/components/ui/command' -import { branchName } from '@/lib/git-utils' import { parseGitHubIssueOrPRNumber, parseGitHubIssueOrPRLink } from '@/lib/github-links' import { getLinkedWorkItemSuggestedName, getLinkedWorkItemWorkspaceName } from '@/lib/new-workspace' import type { LinkedWorkItemSummary } from '@/lib/new-workspace' @@ -48,6 +47,10 @@ import { type MatchRange, type PaletteSearchResult } from '@/lib/worktree-palette-search' +import { + resolveWorktreeBranchLabel, + resolveWorktreeDisplayName +} from '@/lib/worktree-default-display-name' import { CREATE_WORKTREE_ITEM_ID, createWorktreePaletteRequestGuard, @@ -1794,7 +1797,10 @@ export default function WorktreeJumpPalette(): React.JSX.Element | null { const worktree = entry.worktree const repo = repoMap.get(worktree.repoId) const repoName = repo?.displayName ?? '' - const branch = branchName(worktree.branch) + // Why: both must match searchWorktrees' resolution, or highlight ranges land on + // the wrong text — and a branch-less row would throw here before search ever ran. + const branch = resolveWorktreeBranchLabel(worktree) + const worktreeLabel = resolveWorktreeDisplayName(worktree) const status = getWorktreeStatus( tabsByWorktree[worktree.id] ?? [], browserTabsByWorktree[worktree.id] ?? [], @@ -1862,11 +1868,11 @@ export default function WorktreeJumpPalette(): React.JSX.Element | null { {entry.match.displayNameRange ? ( ) : ( - worktree.displayName + worktreeLabel )} {isCurrentWorktree && ( diff --git a/src/renderer/src/components/cmd-j/worktree-checks-review-index.test.ts b/src/renderer/src/components/cmd-j/worktree-checks-review-index.test.ts index 78889b1d2..e401aa066 100644 --- a/src/renderer/src/components/cmd-j/worktree-checks-review-index.test.ts +++ b/src/renderer/src/components/cmd-j/worktree-checks-review-index.test.ts @@ -177,4 +177,25 @@ describe('buildWorktreeChecksReviewIndex', () => { expect(reviews.has(localWorktree)).toBe(false) expect(reviews.get(sshWorktree)).toMatchObject({ provider: 'github', number: 42 }) }) + + it('skips a branch-less worktree instead of throwing', () => { + // Why: Cmd+J builds this index for every worktree before any query is typed, + // so a folder workspace (empty branch) or a partially hydrated row reaches it. + const branchless: Worktree = { + ...worktree, + id: 'worktree-folder', + branch: undefined as unknown as string, + displayName: undefined as unknown as string + } + + const reviews = buildWorktreeChecksReviewIndex({ + worktrees: [branchless], + repoByHostIdentity: new Map([[getRepoHostIdentity(repo), repo]]), + prCache: {}, + hostedReviewCache: {}, + settings: null + }) + + expect(reviews.has(branchless)).toBe(false) + }) }) diff --git a/src/renderer/src/components/cmd-j/worktree-checks-review-index.ts b/src/renderer/src/components/cmd-j/worktree-checks-review-index.ts index 507eee554..992fab427 100644 --- a/src/renderer/src/components/cmd-j/worktree-checks-review-index.ts +++ b/src/renderer/src/components/cmd-j/worktree-checks-review-index.ts @@ -1,4 +1,4 @@ -import { branchName } from '@/lib/git-utils' +import { resolveWorktreeBranchLabel } from '@/lib/worktree-default-display-name' import { getGitHubPRCacheKey } from '@/store/slices/github-cache-key' import { getHostedReviewCacheKey } from '@/store/slices/hosted-review-cache-identity' import { getRepoHostIdentityForParts } from '@/store/slices/repo-host-identity' @@ -35,7 +35,9 @@ export function buildWorktreeChecksReviewIndex({ if (!repo) { continue } - const branch = branchName(worktree.branch) + // Why: Cmd+J builds this index for every worktree before search runs, so a + // branch-less folder workspace or partially hydrated row must not throw here. + const branch = resolveWorktreeBranchLabel(worktree) const prKey = getGitHubPRCacheKey( repo.path, repo.id, diff --git a/src/renderer/src/components/sidebar/worktree-meta-updates.test.ts b/src/renderer/src/components/sidebar/worktree-meta-updates.test.ts index a53eeae41..e7e81727b 100644 --- a/src/renderer/src/components/sidebar/worktree-meta-updates.test.ts +++ b/src/renderer/src/components/sidebar/worktree-meta-updates.test.ts @@ -49,6 +49,19 @@ describe('buildWorktreeMetaUpdates', () => { }) }) + it('clears a display name with empty string, never a present-undefined key', () => { + const updates = buildWorktreeMetaUpdates({ + displayNameInput: ' ', + currentDisplayName: 'Custom Name', + issueInput: '', + prInput: '', + commentInput: '' + }) + + expect(updates.displayName).toBe('') + expect(Object.values(updates).every((value) => value !== undefined)).toBe(true) + }) + it('rejects PR URLs in the issue input', () => { expect( buildWorktreeMetaUpdates({ diff --git a/src/renderer/src/components/sidebar/worktree-meta-updates.ts b/src/renderer/src/components/sidebar/worktree-meta-updates.ts index 26769c650..538b8de6b 100644 --- a/src/renderer/src/components/sidebar/worktree-meta-updates.ts +++ b/src/renderer/src/components/sidebar/worktree-meta-updates.ts @@ -48,12 +48,14 @@ export function buildWorktreeMetaUpdates(args: { const finalLinkedPR = trimmedPR === '' ? null : linkedPRNumber !== null ? linkedPRNumber : undefined + // Why: blanking the field means "fall back to the branch/folder name", and the + // empty string is how that intent is persisted. Emitting `undefined` instead + // put a present-but-undefined key into the store spread, wiping the live name + // and crashing the worktree palette (crash a1f81ea1). const trimmedDisplayName = args.displayNameInput.trim() const updates: Partial = { comment: args.commentInput.trim(), - ...(trimmedDisplayName !== args.currentDisplayName && { - displayName: trimmedDisplayName || undefined - }) + ...(trimmedDisplayName !== args.currentDisplayName && { displayName: trimmedDisplayName }) } if (finalLinkedIssue !== undefined) { updates.linkedIssue = finalLinkedIssue diff --git a/src/renderer/src/lib/browser-palette-search.test.ts b/src/renderer/src/lib/browser-palette-search.test.ts index 6e76c9ab5..23cf01969 100644 --- a/src/renderer/src/lib/browser-palette-search.test.ts +++ b/src/renderer/src/lib/browser-palette-search.test.ts @@ -331,4 +331,54 @@ describe('browser-palette-search', () => { it('rejects oversized whitespace before trimming', () => { expect(searchBrowserPages([], ' '.repeat(BROWSER_PALETTE_QUERY_MAX_BYTES + 1))).toEqual([]) }) + + it('falls back to the branch label when a cleared display name left it undefined', () => { + // Why: Cmd+J runs this search over the same worktree objects as searchWorktrees, + // so the store-level display-name corruption reaches here too. + const cleared = makeWorktree({ + displayName: undefined as unknown as string, + branch: 'refs/heads/feature/browser-search' + }) + const entries: SearchableBrowserPage[] = [ + { + page: makePage(), + workspace: makeWorkspace(), + worktree: cleared, + repoName: 'orca', + worktreeSortIndex: 0, + isCurrentPage: false, + isCurrentWorktree: false + } + ] + + const results = searchBrowserPages(entries, 'browser-search') + expect(results[0]).toMatchObject({ + worktreeName: 'feature/browser-search', + worktreeRange: { start: 'feature/'.length, end: 'feature/browser-search'.length } + }) + }) + + it('lists a branch-less row on the empty query without throwing', () => { + const cleared = makeWorktree({ + displayName: undefined as unknown as string, + branch: undefined as unknown as string, + path: '/repos/design-review' + }) + const entries: SearchableBrowserPage[] = [ + { + page: makePage(), + workspace: makeWorkspace(), + worktree: cleared, + repoName: 'orca', + worktreeSortIndex: 0, + isCurrentPage: false, + isCurrentWorktree: false + } + ] + + expect(searchBrowserPages(entries, '')[0]).toMatchObject({ + worktreeName: 'design-review', + worktreeRange: null + }) + }) }) diff --git a/src/renderer/src/lib/browser-palette-search.ts b/src/renderer/src/lib/browser-palette-search.ts index 759335a4a..942331640 100644 --- a/src/renderer/src/lib/browser-palette-search.ts +++ b/src/renderer/src/lib/browser-palette-search.ts @@ -1,6 +1,7 @@ import { ORCA_BROWSER_BLANK_URL } from '../../../shared/constants' import type { BrowserPage, BrowserWorkspace, Worktree } from '../../../shared/types' import { isClipboardTextByteLengthOverLimit } from '../../../shared/clipboard-text' +import { resolveWorktreeDisplayName } from './worktree-default-display-name' import type { MatchRange } from './worktree-palette-search' export type SearchableBrowserPage = { @@ -125,6 +126,8 @@ export function searchBrowserPages( const formattedUrl = formatBrowserPaletteUrl(entry.page.url) const title = entry.page.title || formattedUrl const fallbackSecondaryText = formattedUrl + // Why: a cleared display name leaves this undefined at runtime; findRange would throw. + const worktreeName = resolveWorktreeDisplayName(entry.worktree) const baseResult = { pageId: entry.page.id, workspaceId: entry.workspace.id, @@ -132,7 +135,7 @@ export function searchBrowserPages( title, workspaceLabel: entry.workspace.label ?? null, repoName: entry.repoName, - worktreeName: entry.worktree.displayName, + worktreeName, isCurrentPage: entry.isCurrentPage, isCurrentWorktree: entry.isCurrentWorktree } @@ -234,7 +237,7 @@ export function searchBrowserPages( continue } - const worktreeRange = findRange(entry.worktree.displayName, trimmedQuery) + const worktreeRange = findRange(worktreeName, trimmedQuery) if (worktreeRange) { results.push({ ...baseResult, diff --git a/src/renderer/src/lib/simulator-palette-search.test.ts b/src/renderer/src/lib/simulator-palette-search.test.ts index 48b7ea98d..cfc8aa4a7 100644 --- a/src/renderer/src/lib/simulator-palette-search.test.ts +++ b/src/renderer/src/lib/simulator-palette-search.test.ts @@ -174,4 +174,49 @@ describe('simulator-palette-search', () => { it('rejects oversized whitespace before trimming simulator palette queries', () => { expect(searchSimulatorTabs([], ' '.repeat(SIMULATOR_PALETTE_QUERY_MAX_BYTES + 1))).toEqual([]) }) + + it('falls back to the branch label when a cleared display name left it undefined', () => { + // Why: Cmd+J runs this search over the same worktree objects as searchWorktrees, + // so the store-level display-name corruption reaches here too. + const entries = [ + { + tab: makeTab(), + worktree: makeWorktree({ + displayName: undefined as unknown as string, + branch: 'refs/heads/feature/mobile-emulator' + }), + repoName: 'orca', + worktreeSortIndex: 0, + isCurrentTab: false, + isCurrentWorktree: false + } + ] + + expect(searchSimulatorTabs(entries, 'mobile-emulator')[0]).toMatchObject({ + worktreeName: 'feature/mobile-emulator', + worktreeRange: { start: 'feature/'.length, end: 'feature/mobile-emulator'.length } + }) + }) + + it('lists a branch-less row on the empty query without throwing', () => { + const entries = [ + { + tab: makeTab(), + worktree: makeWorktree({ + displayName: undefined as unknown as string, + branch: undefined as unknown as string, + path: '/repos/design-review' + }), + repoName: 'orca', + worktreeSortIndex: 0, + isCurrentTab: false, + isCurrentWorktree: false + } + ] + + expect(searchSimulatorTabs(entries, '')[0]).toMatchObject({ + worktreeName: 'design-review', + worktreeRange: null + }) + }) }) diff --git a/src/renderer/src/lib/simulator-palette-search.ts b/src/renderer/src/lib/simulator-palette-search.ts index 1260700ca..aded93466 100644 --- a/src/renderer/src/lib/simulator-palette-search.ts +++ b/src/renderer/src/lib/simulator-palette-search.ts @@ -1,5 +1,6 @@ import type { Tab, TabGroup, Worktree } from '../../../shared/types' import { isClipboardTextByteLengthOverLimit } from '../../../shared/clipboard-text' +import { resolveWorktreeDisplayName } from './worktree-default-display-name' import type { MatchRange } from './worktree-palette-search' export type SearchableSimulatorTab = { @@ -180,6 +181,8 @@ export function searchSimulatorTabs( for (const entry of entries) { const title = entry.tab.label || 'Mobile Emulator' const secondaryText = 'Mobile Emulator tab' + // Why: a cleared display name leaves this undefined at runtime; findRange would throw. + const worktreeName = resolveWorktreeDisplayName(entry.worktree) const baseResult = { tabId: entry.tab.id, worktreeId: entry.worktree.id, @@ -187,7 +190,7 @@ export function searchSimulatorTabs( title, secondaryText, repoName: entry.repoName, - worktreeName: entry.worktree.displayName, + worktreeName, isCurrentTab: entry.isCurrentTab, isCurrentWorktree: entry.isCurrentWorktree } @@ -253,7 +256,7 @@ export function searchSimulatorTabs( continue } - const worktreeRange = findRange(entry.worktree.displayName, trimmedQuery) + const worktreeRange = findRange(worktreeName, trimmedQuery) if (worktreeRange) { results.push({ ...baseResult, diff --git a/src/renderer/src/lib/workspace-tab-palette-results.ts b/src/renderer/src/lib/workspace-tab-palette-results.ts index 4f1a5798e..9d829355a 100644 --- a/src/renderer/src/lib/workspace-tab-palette-results.ts +++ b/src/renderer/src/lib/workspace-tab-palette-results.ts @@ -1,3 +1,4 @@ +import { resolveWorktreeDisplayName } from './worktree-default-display-name' import type { MatchRange } from './worktree-palette-search' import type { SearchableWorkspaceTab, @@ -114,6 +115,8 @@ export function searchWorkspaceTabs( const results: WorkspaceTabPaletteSearchResult[] = [] for (const entry of entries) { + // Why: a cleared display name leaves this undefined at runtime; findRange would throw. + const worktreeName = resolveWorktreeDisplayName(entry.worktree) const baseResult = { tabId: entry.tab.id, entityId: entry.tab.entityId, @@ -123,7 +126,7 @@ export function searchWorkspaceTabs( title: entry.title, secondaryText: entry.secondaryText, repoName: entry.repoName, - worktreeName: entry.worktree.displayName, + worktreeName, isCurrentTab: entry.isCurrentTab, isCurrentWorktree: entry.isCurrentWorktree } @@ -200,7 +203,7 @@ export function searchWorkspaceTabs( continue } - const worktreeRange = findRange(entry.worktree.displayName, trimmedQuery) + const worktreeRange = findRange(worktreeName, trimmedQuery) if (worktreeRange) { results.push({ ...baseResult, diff --git a/src/renderer/src/lib/workspace-tab-palette-search.test.ts b/src/renderer/src/lib/workspace-tab-palette-search.test.ts index fb02f8c74..03b0f55fa 100644 --- a/src/renderer/src/lib/workspace-tab-palette-search.test.ts +++ b/src/renderer/src/lib/workspace-tab-palette-search.test.ts @@ -438,4 +438,36 @@ describe('workspace-tab-palette-search', () => { 'terminal-other' ]) }) + + it('falls back to the branch label when a cleared display name left it undefined', () => { + // Why: Cmd+J runs this search over the same worktree objects as searchWorktrees, + // so the store-level display-name corruption reaches here too. + const cleared = makeWorktree({ + displayName: undefined as unknown as string, + branch: 'refs/heads/feature/workspace-tab-search' + }) + const entries = buildEntries({ worktrees: [cleared] }) + + expect(searchWorkspaceTabs(entries, 'workspace-tab-search')[0]).toMatchObject({ + worktreeName: 'feature/workspace-tab-search', + worktreeRange: { + start: 'feature/'.length, + end: 'feature/workspace-tab-search'.length + } + }) + }) + + it('lists a branch-less row on the empty query without throwing', () => { + const cleared = makeWorktree({ + displayName: undefined as unknown as string, + branch: undefined as unknown as string, + path: path.join('repos', 'design-review') + }) + const entries = buildEntries({ worktrees: [cleared] }) + + expect(searchWorkspaceTabs(entries, '')[0]).toMatchObject({ + worktreeName: 'design-review', + worktreeRange: null + }) + }) }) diff --git a/src/renderer/src/lib/worktree-default-display-name.test.ts b/src/renderer/src/lib/worktree-default-display-name.test.ts new file mode 100644 index 000000000..89184c421 --- /dev/null +++ b/src/renderer/src/lib/worktree-default-display-name.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it } from 'vitest' +import { + resolveWorktreeBranchLabel, + resolveWorktreeDisplayName +} from './worktree-default-display-name' + +describe('resolveWorktreeBranchLabel', () => { + it('strips refs/heads/ like the raw branchName call it replaces', () => { + expect(resolveWorktreeBranchLabel({ branch: 'refs/heads/feature/jump' })).toBe('feature/jump') + }) + + it('returns empty for a folder workspace, which carries no branch', () => { + expect(resolveWorktreeBranchLabel({ branch: '' })).toBe('') + }) + + it('returns empty instead of throwing when branch is absent at runtime', () => { + // The palette renders every row on an empty query, before any branch search runs, + // so an unguarded branchName() here crashed the whole palette. + expect(resolveWorktreeBranchLabel({ branch: undefined as unknown as string })).toBe('') + }) +}) + +describe('resolveWorktreeDisplayName', () => { + it('prefers the custom name', () => { + expect( + resolveWorktreeDisplayName({ + displayName: 'Design review', + branch: 'refs/heads/feature/jump', + path: '/repos/orca' + }) + ).toBe('Design review') + }) + + it('falls back to the branch when the name was blanked to an empty string', () => { + expect( + resolveWorktreeDisplayName({ + displayName: '', + branch: 'refs/heads/feature/jump', + path: '/repos/orca' + }) + ).toBe('feature/jump') + }) + + it('treats a whitespace-only name as blank', () => { + expect( + resolveWorktreeDisplayName({ + displayName: ' ', + branch: 'refs/heads/main', + path: '/repos/orca' + }) + ).toBe('main') + }) + + it('falls back to the branch when a cleared name left the field undefined', () => { + expect( + resolveWorktreeDisplayName({ + displayName: undefined as unknown as string, + branch: 'refs/heads/main', + path: '/repos/orca' + }) + ).toBe('main') + }) + + it('falls back to the folder name for a branch-less folder workspace', () => { + expect( + resolveWorktreeDisplayName({ displayName: '', branch: '', path: '/repos/design-review' }) + ).toBe('design-review') + }) + + it('resolves the folder name from a Windows path', () => { + expect( + resolveWorktreeDisplayName({ + displayName: '', + branch: '', + path: 'C:\\Users\\alice\\repos\\design-review' + }) + ).toBe('design-review') + }) + + it('keeps emoji and non-ASCII names intact', () => { + expect( + resolveWorktreeDisplayName({ displayName: '🚀 Läufer', branch: '', path: '/repos/x' }) + ).toBe('🚀 Läufer') + }) + + it('returns empty rather than throwing when every source is missing', () => { + expect( + resolveWorktreeDisplayName({ + displayName: undefined as unknown as string, + branch: undefined as unknown as string, + path: undefined as unknown as string + }) + ).toBe('') + }) +}) diff --git a/src/renderer/src/lib/worktree-default-display-name.ts b/src/renderer/src/lib/worktree-default-display-name.ts new file mode 100644 index 000000000..765b6d084 --- /dev/null +++ b/src/renderer/src/lib/worktree-default-display-name.ts @@ -0,0 +1,34 @@ +import { branchName } from '@/lib/git-utils' +import { basename } from '@/lib/path' +import type { Worktree } from '../../../shared/types' + +type WorktreeDisplayNameSource = Pick + +/** + * `branch` is typed non-optional but is absent on folder workspaces and + * partially hydrated rows, and `branchName` throws on undefined. Every render + * and search read of the branch label must go through here. + */ +export function resolveWorktreeBranchLabel(worktree: Pick): string { + return typeof worktree.branch === 'string' ? branchName(worktree.branch) : '' +} + +/** + * Renderer mirror of main-side `mergeWorktree`: a missing or blank custom name + * falls back to the branch, then the folder. `displayName` is typed non-optional + * but arrives undefined at runtime once a custom name is cleared (crash + * a1f81ea1), so every read must go through here instead of dereferencing it. + */ +export function resolveWorktreeDisplayName(worktree: WorktreeDisplayNameSource): string { + const custom = typeof worktree.displayName === 'string' ? worktree.displayName.trim() : '' + if (custom) { + return custom + } + + const branch = resolveWorktreeBranchLabel(worktree).trim() + if (branch) { + return branch + } + + return typeof worktree.path === 'string' ? basename(worktree.path).trim() : '' +} diff --git a/src/renderer/src/lib/worktree-palette-search.test.ts b/src/renderer/src/lib/worktree-palette-search.test.ts index 4cb69b75e..81d549a34 100644 --- a/src/renderer/src/lib/worktree-palette-search.test.ts +++ b/src/renderer/src/lib/worktree-palette-search.test.ts @@ -141,6 +141,63 @@ describe('worktree-palette-search', () => { expect(searchWorktrees([makeWorktree()], query, repoMap, null, null)).toEqual([]) }) + it('falls back to branch text when a cleared display name left it undefined', () => { + const cleared = makeWorktree({ + displayName: undefined as unknown as string, + branch: 'refs/heads/feature/worktree-jump' + }) + + expect(() => searchWorktrees([cleared], 'jump', repoMap, null, null)).not.toThrow() + // Highlight range indexes the branch-derived label the palette actually renders. + expect(searchWorktrees([cleared], 'jump', repoMap, null, null)[0]).toMatchObject({ + worktreeId: 'wt-1', + matchedField: 'displayName', + displayNameRange: { start: 'feature/worktree-'.length, end: 'feature/worktree-jump'.length } + }) + }) + + it('falls back to the folder name when both display name and branch are missing', () => { + const folderWorkspace = makeWorktree({ + displayName: undefined as unknown as string, + branch: '', + path: '/tmp/design-review' + }) + + expect(searchWorktrees([folderWorkspace], 'design', repoMap, null, null)[0]).toMatchObject({ + matchedField: 'displayName', + displayNameRange: { start: 0, end: 6 } + }) + }) + + it('survives a cleared display name on composite repo/branch queries', () => { + const cleared = makeWorktree({ + displayName: undefined as unknown as string, + branch: undefined as unknown as string + }) + + expect(() => searchWorktrees([cleared], 'orca/jump', repoMap, null, null)).not.toThrow() + }) + + it('still lists a branch-less row on the empty query, which renders every row', () => { + // Why: the empty query short-circuits before any branch read, so the row reaches the + // render loop untouched — the label resolution there has to be guarded too. + const cleared = makeWorktree({ + displayName: undefined as unknown as string, + branch: undefined as unknown as string + }) + + expect(searchWorktrees([cleared], '', repoMap, null, null)).toEqual([ + { + worktreeId: 'wt-1', + matchedField: null, + displayNameRange: null, + branchRange: null, + repoRange: null, + supportingText: null + } + ]) + }) + it('returns a truncated comment snippet with the highlighted match range', () => { const results = searchWorktrees( [ diff --git a/src/renderer/src/lib/worktree-palette-search.ts b/src/renderer/src/lib/worktree-palette-search.ts index f8dce9958..074587e8a 100644 --- a/src/renderer/src/lib/worktree-palette-search.ts +++ b/src/renderer/src/lib/worktree-palette-search.ts @@ -1,5 +1,8 @@ -import { branchName } from '@/lib/git-utils' import { issueCacheKey as getIssueCacheKey } from '@/store/slices/github' +import { + resolveWorktreeBranchLabel, + resolveWorktreeDisplayName +} from './worktree-default-display-name' import type { HostedReviewInfo } from '../../../shared/hosted-review' import type { Repo, Worktree } from '../../../shared/types' import { extractWorktreePaletteCommentSnippet } from './worktree-palette-comment-snippet' @@ -101,7 +104,7 @@ export function searchWorktrees( for (const worktree of worktrees) { if (composite) { const repoName = repoMap.get(worktree.repoId)?.displayName ?? '' - const branch = branchName(worktree.branch) + const branch = resolveWorktreeBranchLabel(worktree) const repoIdx = repoName.toLowerCase().indexOf(composite.repoPart) const branchIdx = branch.toLowerCase().indexOf(composite.branchPart) if (repoIdx !== -1 && branchIdx !== -1) { @@ -117,7 +120,7 @@ export function searchWorktrees( // that happens to contain a slash (e.g. "feature/foo") still get hits. } - const nameIndex = worktree.displayName.toLowerCase().indexOf(q) + const nameIndex = resolveWorktreeDisplayName(worktree).toLowerCase().indexOf(q) if (nameIndex !== -1) { results.push( makeResult(worktree.id, 'displayName', { @@ -127,7 +130,7 @@ export function searchWorktrees( continue } - const branch = branchName(worktree.branch) + const branch = resolveWorktreeBranchLabel(worktree) const branchIndex = branch.toLowerCase().indexOf(q) if (branchIndex !== -1) { results.push( diff --git a/src/renderer/src/store/slices/worktree-helpers.test.ts b/src/renderer/src/store/slices/worktree-helpers.test.ts index fbd887504..2750be444 100644 --- a/src/renderer/src/store/slices/worktree-helpers.test.ts +++ b/src/renderer/src/store/slices/worktree-helpers.test.ts @@ -51,4 +51,21 @@ describe('applyWorktreeUpdates', () => { expect(result['repo-b']?.[0]).toBe(samePathDifferentProject) expect(result['repo-b']?.[0]?.displayName).toBe('Project B') }) + + it('never lets a present-but-undefined value clobber existing worktree fields', () => { + const target = makeWorktree({ + id: 'repo-a::/Users/alice/project', + repoId: 'repo-a', + displayName: 'Project A', + comment: 'notes' + }) + + const result = applyWorktreeUpdates({ 'repo-a': [target] }, target.id, { + displayName: undefined, + comment: 'edited' + }) + + expect(result['repo-a']?.[0]?.displayName).toBe('Project A') + expect(result['repo-a']?.[0]?.comment).toBe('edited') + }) }) diff --git a/src/renderer/src/store/slices/worktree-helpers.ts b/src/renderer/src/store/slices/worktree-helpers.ts index fee8e4797..bea4a374b 100644 --- a/src/renderer/src/store/slices/worktree-helpers.ts +++ b/src/renderer/src/store/slices/worktree-helpers.ts @@ -326,11 +326,51 @@ export function findWorktreeById( return undefined } +type RequiredKey = { [K in keyof T]-?: undefined extends T[K] ? never : K }[keyof T] + +// Why: a present-but-undefined key in a spread ERASES the field. That is the +// intended wire signal for clearing optional metadata (pushTarget), but on a +// field Worktree declares required it produced a live `displayName: undefined` +// that crashed the worktree palette (crash a1f81ea1). Typed off Worktree so a +// newly-required field is protected automatically. +const ERASURE_PROTECTED_KEYS: Record, keyof WorktreeMeta>, true> = { + displayName: true, + comment: true, + linkedIssue: true, + linkedPR: true, + linkedLinearIssue: true, + isArchived: true, + isUnread: true, + isPinned: true, + sortOrder: true, + lastActivityAt: true +} + +export function withoutErasedRequiredWorktreeFields( + updates: Partial +): Partial { + const erased = Object.keys(ERASURE_PROTECTED_KEYS).filter( + (key) => + updates[key as keyof WorktreeMeta] === undefined && + Object.prototype.hasOwnProperty.call(updates, key) + ) + if (erased.length === 0) { + return updates + } + + const next = { ...updates } + for (const key of erased) { + delete next[key as keyof WorktreeMeta] + } + return next +} + export function applyWorktreeUpdates( worktreesByRepo: Record, worktreeId: string, - updates: Partial + rawUpdates: Partial ): Record { + const updates = withoutErasedRequiredWorktreeFields(rawUpdates) const repoId = getRepoIdFromWorktreeId(worktreeId) const worktrees = worktreesByRepo[repoId] if (!worktrees) { diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 1d7ae9c5c..8b9f4e34e 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -20,6 +20,7 @@ import type { RuntimeWorktreeListResult } from '../../../../shared/runtime-types import { findWorktreeById, applyWorktreeUpdates, + withoutErasedRequiredWorktreeFields, getRepoIdFromWorktreeId, type DirectSshWorktreeFetchOptions, type WorktreeFetchOptions, @@ -706,8 +707,10 @@ function notifyRuntimeScopeForbiddenIfNeeded(error: unknown): boolean { function applyDetectedWorktreeUpdates( detectedWorktreesByRepo: AppState['detectedWorktreesByRepo'], worktreeId: string, - updates: Partial + rawUpdates: Partial ): AppState['detectedWorktreesByRepo'] { + // Why: mirrors applyWorktreeUpdates — detected rows feed the same palette. + const updates = withoutErasedRequiredWorktreeFields(rawUpdates) let changed = false const nextByRepo: AppState['detectedWorktreesByRepo'] = {}