From 53af61160a8f2074cde9851c470b1c991f4479e4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:20:05 -0700 Subject: [PATCH 1/4] fix(settings): keep Shortcuts rows visible on pane-title-only search (#9097) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: 조재중 <126754298+m-a-king@users.noreply.github.com> --- .../settings/ShortcutCommandBlock.tsx | 3 ++ .../settings/ShortcutFilterRail.test.ts | 35 +++++++++++++++ .../settings/ShortcutFilterRail.tsx | 13 +++++- .../src/components/settings/ShortcutsPane.tsx | 5 ++- .../settings-search-shortcuts-pane.spec.ts | 43 +++++++++++++++++++ 5 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 tests/e2e/settings-search-shortcuts-pane.spec.ts diff --git a/src/renderer/src/components/settings/ShortcutCommandBlock.tsx b/src/renderer/src/components/settings/ShortcutCommandBlock.tsx index 853e6ae3c62..c4eaa9cbe84 100644 --- a/src/renderer/src/components/settings/ShortcutCommandBlock.tsx +++ b/src/renderer/src/components/settings/ShortcutCommandBlock.tsx @@ -106,6 +106,8 @@ export function ShortcutCommandBlock({ /> ) + // Why: ShortcutsPane already filters rows against the global query; + // forceVisible skips the re-match here that would hide rows it kept. return (
diff --git a/src/renderer/src/components/settings/ShortcutFilterRail.test.ts b/src/renderer/src/components/settings/ShortcutFilterRail.test.ts index d979415798a..8dd6a039010 100644 --- a/src/renderer/src/components/settings/ShortcutFilterRail.test.ts +++ b/src/renderer/src/components/settings/ShortcutFilterRail.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import type { KeybindingActionId } from '../../../../shared/keybindings' import { SHORTCUT_LOCAL_SEARCH_QUERY_MAX_BYTES, + buildShortcutGlobalSearchMatcher, isShortcutLocalSearchQueryTooLarge, matchesShortcutLocalSearch, normalizeShortcutLocalSearchQuery, @@ -52,6 +53,40 @@ describe('ShortcutFilterRail search helpers', () => { expect(normalizeShortcutLocalSearchQuery(query)).toBeNull() }) + it('narrows rows when the global settings query matches specific rows', () => { + const settingsRow = createShortcutRow() + const worktreeRow: ShortcutRowModel = { + ...createShortcutRow(), + item: { + ...createShortcutRow().item, + id: 'worktree.create' as KeybindingActionId, + title: 'Create worktree', + searchKeywords: ['worktree', 'create'] + } + } + + const matcher = buildShortcutGlobalSearchMatcher([settingsRow, worktreeRow], 'worktree') + + expect(matcher(settingsRow)).toBe(false) + expect(matcher(worktreeRow)).toBe(true) + }) + + it('keeps every row visible on a pane-title-only global query', () => { + const rows = [createShortcutRow()] + + // Mirrors sidebar search selecting the pane with a query that matches only + // pane-level metadata (e.g. a localized pane title) and no row metadata. + const matcher = buildShortcutGlobalSearchMatcher(rows, '단축키') + + expect(matcher(rows[0])).toBe(true) + }) + + it('keeps every row visible when the global query is empty', () => { + const rows = [createShortcutRow()] + + expect(buildShortcutGlobalSearchMatcher(rows, '')(rows[0])).toBe(true) + }) + it('rejects oversized pasted shortcut searches before reading row metadata', () => { const oversizedQuery = 'secret-shortcut-search'.repeat(SHORTCUT_LOCAL_SEARCH_QUERY_MAX_BYTES) const row = { diff --git a/src/renderer/src/components/settings/ShortcutFilterRail.tsx b/src/renderer/src/components/settings/ShortcutFilterRail.tsx index 76445a67e9a..222127f88ed 100644 --- a/src/renderer/src/components/settings/ShortcutFilterRail.tsx +++ b/src/renderer/src/components/settings/ShortcutFilterRail.tsx @@ -6,7 +6,7 @@ import { cn } from '../../lib/utils' import { Button } from '../ui/button' import { Input } from '../ui/input' import type { ShortcutTerminalStatus } from './shortcut-terminal-status' -import type { SettingsSearchEntry } from './settings-search' +import { matchesSettingsSearch, type SettingsSearchEntry } from './settings-search' import { translate } from '@/i18n/i18n' export type ShortcutFilter = 'all' | 'modified' | 'unassigned' | 'conflicts' @@ -60,6 +60,17 @@ export function getShortcutSearchEntry(row: ShortcutRowModel): SettingsSearchEnt } } +// Why: a sidebar query can select this pane by title alone while matching zero +// rows; that would blank the list, so zero-row queries keep every row visible. +export function buildShortcutGlobalSearchMatcher( + rows: readonly ShortcutRowModel[], + searchQuery: string +): (row: ShortcutRowModel) => boolean { + const rowMatches = (row: ShortcutRowModel): boolean => + matchesSettingsSearch(searchQuery, getShortcutSearchEntry(row)) + return rows.some(rowMatches) ? rowMatches : () => true +} + export function matchesShortcutFilter(row: ShortcutRowModel, filter: ShortcutFilter): boolean { switch (filter) { case 'modified': diff --git a/src/renderer/src/components/settings/ShortcutsPane.tsx b/src/renderer/src/components/settings/ShortcutsPane.tsx index b6fe5b3556c..29f9661d196 100644 --- a/src/renderer/src/components/settings/ShortcutsPane.tsx +++ b/src/renderer/src/components/settings/ShortcutsPane.tsx @@ -25,7 +25,7 @@ import { sameBindings } from './keybinding-override-edits' import { - getShortcutSearchEntry, + buildShortcutGlobalSearchMatcher, matchesShortcutFilter, matchesShortcutLocalSearch, normalizeShortcutLocalSearchQuery, @@ -138,9 +138,10 @@ export function ShortcutsPane(): React.JSX.Element { ) const shortcutSearchQuery = normalizeShortcutLocalSearchQuery(shortcutQuery) const shortcutRows = shortcutGroups.flatMap((group) => group.rows) + const matchesShortcutGlobalSearch = buildShortcutGlobalSearchMatcher(shortcutRows, searchQuery) const matchesShortcutSearch = (row: ShortcutRowsByGroup['rows'][number]): boolean => shortcutSearchQuery !== null && - matchesSettingsSearch(searchQuery, getShortcutSearchEntry(row)) && + matchesShortcutGlobalSearch(row) && matchesShortcutLocalSearch(row, shortcutSearchQuery, platform) const baseVisibleRows = shortcutRows.filter((row) => matchesShortcutSearch(row)) const filterCounts: Record = { diff --git a/tests/e2e/settings-search-shortcuts-pane.spec.ts b/tests/e2e/settings-search-shortcuts-pane.spec.ts new file mode 100644 index 00000000000..17e8097083b --- /dev/null +++ b/tests/e2e/settings-search-shortcuts-pane.spec.ts @@ -0,0 +1,43 @@ +import { test, expect } from './helpers/orca-app' +import { waitForSessionReady } from './helpers/store' + +test.describe('Settings sidebar search on the Shortcuts pane', () => { + test('pane-title-only query keeps rows visible and local search usable', async ({ orcaPage }) => { + await waitForSessionReady(orcaPage) + + await orcaPage.evaluate(async () => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + // Why: the spec asserts on English strings; the host machine may run a + // non-English system locale, which 'system' would follow. + await store.getState().updateSettings({ uiLanguage: 'en' }) + store.getState().openSettingsPage() + }) + + const searchInput = orcaPage.getByPlaceholder('Search settings') + await expect(searchInput).toBeVisible() + await searchInput.fill('shortcuts') + + // The query matches the pane title, so the Shortcuts pane auto-activates. + await expect(orcaPage.getByRole('heading', { name: 'Shortcuts', exact: true })).toBeVisible() + + // Regression: a pane-title-only query used to blank the whole list (0/112). + await expect(orcaPage.getByText('Go to File', { exact: true })).toBeVisible() + await expect(orcaPage.getByText('No shortcuts match those filters.')).not.toBeVisible() + + // Regression: the pane's own search was dead while the global query was + // active, because it intersected with an already-empty base list. + const localSearch = orcaPage.getByPlaceholder('Search command or keys') + await localSearch.fill('go to') + await expect(orcaPage.getByText('Go to File', { exact: true })).toBeVisible() + await expect(orcaPage.getByText('Force Reload', { exact: true })).not.toBeVisible() + + // A row-matching global query still narrows the list as before. + await localSearch.clear() + await searchInput.fill('worktree') + await expect(orcaPage.getByText('Create worktree', { exact: true })).toBeVisible() + await expect(orcaPage.getByText('Go to File', { exact: true })).not.toBeVisible() + }) +}) From a7c40cd89b2fde220c04abbc24289bb38456da3b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:21:09 -0700 Subject: [PATCH 2/4] fix(terminal): stop gluing the next line onto a modifier-clicked URL (#9100) Fixes #8832. HTTP hit-testing no longer consumes path hard-wrap candidates that poisoned mid-row URLs with next-line labels. Uses soft-wrap, framed hard-wrap HTTP, and dedicated edge-wrap reconstruction instead. Supersedes #8852 (edge-wrap approach credit: @xianjianlf2). --- .../edge-wrapped-terminal-http-links.test.ts | 425 ++++++++++++++++++ .../edge-wrapped-terminal-http-links.ts | 183 ++++++++ .../repro-8832-url-next-line.test.ts | 203 +++++++++ .../terminal-url-link-click.test.ts | 38 ++ .../terminal-url-link-hit-testing.ts | 33 +- 5 files changed, 866 insertions(+), 16 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.test.ts create mode 100644 src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.ts create mode 100644 src/renderer/src/components/terminal-pane/repro-8832-url-next-line.test.ts diff --git a/src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.test.ts b/src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.test.ts new file mode 100644 index 00000000000..9289f67b7b2 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.test.ts @@ -0,0 +1,425 @@ +import type { IBufferLine } from '@xterm/xterm' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { registerHttpLinkStoreAccessor } from '@/lib/http-link-routing' +import { openHttpLinkAtBufferPosition } from './terminal-url-link-hit-testing' + +const COLS = 40 +const WIDE_GLYPH = '界' +const openUrlMock = vi.fn() + +function makeBufferLine(content: string, cols = COLS): IBufferLine { + return makeBufferLineFromCells( + Array.from(content, (text) => ({ text, width: text === WIDE_GLYPH ? 2 : 1 })), + cols + ) +} + +function makeBufferLineFromCells( + cells: { text: string; width: number }[], + cols = COLS +): IBufferLine { + const columns: number[] = [] + let column = 0 + let text = '' + for (const cell of cells) { + text += cell.text + for (let index = 0; index < cell.text.length; index++) { + columns.push(column) + } + column += cell.width + } + while (column < cols) { + text += ' ' + columns.push(column) + column++ + } + columns.push(column) + + return { + isWrapped: false, + length: cols, + translateToString: ( + _trimRight?: boolean, + startColumn = 0, + endColumn = text.length, + outColumns?: number[] + ) => { + outColumns?.splice(0, outColumns.length, ...columns.slice(startColumn, endColumn + 1)) + return text.slice(startColumn, endColumn) + } + } as IBufferLine +} + +describe('edge-wrapped terminal HTTP links', () => { + beforeEach(() => { + vi.stubGlobal('window', { api: { shell: { openUrl: openUrlMock } } }) + registerHttpLinkStoreAccessor(() => ({ + settings: { openLinksInApp: false }, + setActiveWorktree: vi.fn(), + createBrowserTab: vi.fn() + })) + openUrlMock.mockReset() + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('opens a URL when a wide glyph starts the continuation row', () => { + const prefix = 'https://example.com/wide/' + const firstRow = `${prefix}${'c'.repeat(COLS - 1 - prefix.length)}` + const continuationRow = `${WIDE_GLYPH}tail` + const expectedUrl = new URL(`${firstRow}${continuationRow}`).toString() + const rows = [makeBufferLine(firstRow), makeBufferLine(continuationRow)] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 3, y: 2 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + }) + + it('opens a URL when a multi-code-point wide cell starts the continuation row', () => { + const wideCell = '👩‍💻' + const prefix = 'https://example.com/wide/' + const firstRow = `${prefix}${'c'.repeat(COLS - 1 - prefix.length)}` + const continuationRow = `${wideCell}tail` + const expectedUrl = new URL(`${firstRow}${continuationRow}`).toString() + const rows = [ + makeBufferLine(firstRow), + makeBufferLineFromCells([ + { text: wideCell, width: 2 }, + ...Array.from('tail', (text) => ({ text, width: 1 })) + ]) + ] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 1, y: 2 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + }) + + it('does not use the wide-glyph tolerance for an ASCII continuation row', () => { + const firstRow = `https://example.com/${'a'.repeat(COLS - 1 - 'https://example.com/'.length)}` + const rows = [makeBufferLine(firstRow), makeBufferLine('next')] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(firstRow) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 2, y: 2 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(false) + expect(openUrlMock).not.toHaveBeenCalled() + }) + + it.each(['Description: 123', 'Description:', '404: Not Found', 'HTTP/2: 200', 'HTTP/2:200'])( + 'does not append a %s label when a complete URL ends at the terminal edge', + (labelRow) => { + const firstRow = `https://example.com/${'a'.repeat(COLS - 1 - 'https://example.com/'.length)}/` + const rows = [makeBufferLine(firstRow), makeBufferLine(labelRow)] + const buffer = { getLine: (y: number) => rows[y] } + + expect(firstRow).toHaveLength(COLS) + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(firstRow) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 2, y: 2 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(false) + expect(openUrlMock).not.toHaveBeenCalled() + } + ) + + it('does not append an adjacent URL that starts on the next row', () => { + const firstRow = `https://example.com/${'a'.repeat(COLS - 'https://example.com/'.length)}` + const secondRow = 'https://two.test/path' + const rows = [makeBufferLine(firstRow), makeBufferLine(secondRow)] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(firstRow) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 2 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(secondRow) + }) + + it('keeps a full-width continuation row that ends in a colon', () => { + const prefix = 'https://example.com/' + const firstRow = `${prefix}${'a'.repeat(COLS - prefix.length)}` + const secondRow = `${'b'.repeat(COLS - 1)}:` + const thirdRow = 'tail' + const expectedUrl = new URL(`${firstRow}${secondRow}${thirdRow}`).toString() + const rows = [makeBufferLine(firstRow), makeBufferLine(secondRow), makeBufferLine(thirdRow)] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 20, y: 2 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + }) + + it('keeps a short colon-bearing URL continuation', () => { + const firstRow = `https://example.com/${'a'.repeat(COLS - 'https://example.com/'.length)}` + const secondRow = 'urn:abc' + const expectedUrl = `${firstRow}${secondRow}` + const rows = [makeBufferLine(firstRow), makeBufferLine(secondRow)] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: 4, y: 2 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + }) + + it('does not count text before the URL against the URL length limit', () => { + const cols = 200 + const prefix = `${'P'.repeat(179)} ` + const url = `https://example.com/${'a'.repeat(1_880)}` + const displayed = `${prefix}${url}` + const rowTexts = Array.from({ length: Math.ceil(displayed.length / cols) }, (_value, index) => + displayed.slice(index * cols, (index + 1) * cols) + ) + const rows = rowTexts.map((row) => makeBufferLine(row, cols)) + const buffer = { getLine: (y: number) => rows[y] } + + expect(url).toHaveLength(1_900) + expect(displayed.length).toBeGreaterThan(2_048) + expect( + openHttpLinkAtBufferPosition(buffer, { x: 191, y: 1 }, cols, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(url) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 10, y: rows.length }, cols, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(url) + }) + + it('reconstructs a narrow-terminal URL spanning more than 20 rows from either end', () => { + const url = `https://example.com/${'a'.repeat(1_000)}` + const rowTexts = Array.from({ length: Math.ceil(url.length / COLS) }, (_value, index) => + url.slice(index * COLS, (index + 1) * COLS) + ) + const rows = rowTexts.map((row) => makeBufferLine(row)) + const buffer = { getLine: (y: number) => rows[y] } + + expect(url.length).toBeLessThanOrEqual(2_048) + expect(rowTexts.length).toBeGreaterThan(20) + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(url) + + openUrlMock.mockReset() + expect( + openHttpLinkAtBufferPosition(buffer, { x: 5, y: rowTexts.length }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(url) + }) + + it('reconstructs a wide-glyph URL whose row span exceeds the single-width column bound', () => { + const wideCount = 1_100 + const url = `https://example.com/${WIDE_GLYPH.repeat(wideCount)}` + const rowTexts: string[] = [] + let row = '' + let width = 0 + for (const char of url) { + const charWidth = char === WIDE_GLYPH ? 2 : 1 + if (width + charWidth > COLS) { + rowTexts.push(row) + row = '' + width = 0 + } + row += char + width += charWidth + } + if (row.length > 0) { + rowTexts.push(row) + } + const rows = rowTexts.map((rowText) => makeBufferLine(rowText)) + const buffer = { getLine: (y: number) => rows[y] } + + expect(url.length).toBeLessThanOrEqual(2_048) + expect(rowTexts.length).toBeGreaterThan(Math.ceil(2_048 / COLS) + 1) + expect( + openHttpLinkAtBufferPosition(buffer, { x: 5, y: rowTexts.length }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(new URL(url).toString()) + }) + + it('joins a real multi-line URL whose continuation is a Chinese path segment', () => { + const prefix = 'https://example.com/' + const firstRow = `${prefix}${'a'.repeat(COLS - prefix.length)}` + const continuation = `${WIDE_GLYPH}文档/路径` + const expectedUrl = new URL(`${firstRow}${continuation}`).toString() + const rows = [makeBufferLine(firstRow), makeBufferLine(continuation)] + const buffer = { getLine: (y: number) => rows[y] } + + expect(firstRow).toHaveLength(COLS) + expect( + openHttpLinkAtBufferPosition(buffer, { x: 11, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(expectedUrl) + }) + + it('does not glue a mid-row URL to a next-line Windows path', () => { + const firstRow = 'Repo: https://example.com/repo/' + const secondRow = 'C:\\Users\\demo\\project\\file.ts' + const rows = [makeBufferLine(firstRow), makeBufferLine(secondRow)] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: firstRow.indexOf('https') + 8, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith('https://example.com/repo/') + }) + + it('does not create an edge candidate for a scheme embedded in a word', () => { + const embeddedUrl = `abchttps://example.com/${'a'.repeat( + COLS - 'abchttps://example.com/'.length + )}` + const rows = [makeBufferLine(embeddedUrl), makeBufferLine('tail')] + const buffer = { getLine: (y: number) => rows[y] } + + expect( + openHttpLinkAtBufferPosition(buffer, { x: 10, y: 1 }, COLS, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(false) + expect(openUrlMock).not.toHaveBeenCalled() + }) + + it('keeps a URL under the length limit when its final row also contains a label', () => { + const cols = 200 + const url = `https://example.com/${'a'.repeat(2_020)}` + const displayed = `${url} ${'L'.repeat(159)}` + const rowTexts = Array.from({ length: Math.ceil(displayed.length / cols) }, (_value, index) => + displayed.slice(index * cols, (index + 1) * cols) + ) + const rows = rowTexts.map((row) => makeBufferLine(row, cols)) + const buffer = { getLine: (y: number) => rows[y] } + + expect(url).toHaveLength(2_040) + expect(rowTexts).toHaveLength(11) + expect(rowTexts.at(-1)).toHaveLength(cols) + expect( + openHttpLinkAtBufferPosition(buffer, { x: 10, y: 1 }, cols, { + worktreeId: 'wt-1', + forceSystemBrowser: true + }) + ).toBe(true) + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(url) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.ts b/src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.ts new file mode 100644 index 00000000000..48e10fabedd --- /dev/null +++ b/src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.ts @@ -0,0 +1,183 @@ +import type { IBufferLine } from '@xterm/xterm' +import { TERMINAL_HTTP_URL_MAX_LENGTH } from './terminal-http-link-limits' +import { translateLineWithColumns, type WrappedLogicalLine } from './wrapped-terminal-link-ranges' + +const HTTP_SCHEME_PATTERN = /https?:\/\//i +const HTTP_SCHEME_START_PATTERN = /^https?:\/\//i +const HTTP_STATUS_ROW_PATTERN = /^HTTP\/\d(?:\.\d)?:\d{3}(?:\s|$)/i +const VERTICAL_LAYOUT_FRAME_PATTERN = /[│┃║╎╏┆┇┊┋|]/ +const EMPTY_LABEL_ROW_PATTERN = /^[^\s:][^:]*:$/ +const LABEL_WITH_SPACING_ROW_PATTERN = /^[^\s:][^:]*:\s/ + +function maxEdgeWrappedHttpRows(lineLength: number): number { + // Why: every continued row reaches the terminal edge, so even all-width-2 + // cells carry at least floor(columns / 2) characters per row; the URL length + // limit then bounds the scan without truncating URLs on narrow terminals. + // The +2 covers the start and tail rows, which may hold only a fragment. + const minCharsPerFullRow = Math.max(1, Math.floor(lineLength / 2)) + return Math.ceil(TERMINAL_HTTP_URL_MAX_LENGTH / minCharsPerFullRow) + 2 +} + +type TrimmedTranslatedLine = { + text: string + sourceText: string + columns: number[] + lineLength: number +} + +function trimRightTranslatedLine(line: IBufferLine): TrimmedTranslatedLine | null { + const translated = translateLineWithColumns(line) + let endIndex = translated.text.length + while (endIndex > 0 && /\s/.test(translated.text[endIndex - 1])) { + endIndex-- + } + if (endIndex === 0) { + return null + } + return { + text: translated.text.slice(0, endIndex), + sourceText: translated.text, + columns: translated.columns.slice(0, endIndex + 1), + lineLength: line.length + } +} + +function firstCellWidth(row: TrimmedTranslatedLine): number { + const firstColumn = row.columns[0] + if (firstColumn === undefined) { + return 0 + } + const nextColumn = row.columns.find((column) => column > firstColumn) + return nextColumn === undefined ? 0 : nextColumn - firstColumn +} + +function isAsciiWordCode(code: number): boolean { + return ( + (code >= 48 && code <= 57) || + (code >= 65 && code <= 90) || + code === 95 || + (code >= 97 && code <= 122) + ) +} + +function findHttpSchemeIndex(text: string): number { + let searchStart = 0 + while (searchStart < text.length) { + const relativeIndex = text.slice(searchStart).search(HTTP_SCHEME_PATTERN) + if (relativeIndex === -1) { + return -1 + } + const schemeIndex = searchStart + relativeIndex + if (schemeIndex === 0 || !isAsciiWordCode(text.charCodeAt(schemeIndex - 1))) { + return schemeIndex + } + searchStart = schemeIndex + 1 + } + return -1 +} + +function rowsCanContinueAtTerminalEdge( + row: WrappedLogicalLine['rows'][number], + nextRow: TrimmedTranslatedLine +): boolean { + // Why: a cursor-positioned label is not URL continuation evidence even when + // the preceding URL happens to end at the terminal edge (#8832). + const nextColumnAfterText = nextRow.columns.at(-1) + const nextRowReachesEdge = + nextColumnAfterText !== undefined && nextColumnAfterText >= nextRow.lineLength + const isLabelRow = + LABEL_WITH_SPACING_ROW_PATTERN.test(nextRow.text) || + ((EMPTY_LABEL_ROW_PATTERN.test(nextRow.text) || HTTP_STATUS_ROW_PATTERN.test(nextRow.text)) && + !nextRowReachesEdge) + if (isLabelRow || HTTP_SCHEME_START_PATTERN.test(nextRow.text)) { + return false + } + const columnAfterText = row.columns.at(-1) + if (columnAfterText === undefined || columnAfterText < row.lineLength - 1) { + return false + } + return columnAfterText >= row.lineLength || firstCellWidth(nextRow) > 1 +} + +export function buildEdgeWrappedHttpLogicalLineCandidates( + buffer: { getLine(y: number): IBufferLine | undefined }, + bufferLineNumber: number +): WrappedLogicalLine[] { + // Why: cursor-positioned output lacks xterm wrap metadata, but an HTTP URL + // may still continue when each earlier row reaches the terminal edge. + const currentY = bufferLineNumber - 1 + const currentLine = buffer.getLine(currentY) + if (!currentLine) { + return [] + } + const maxRows = maxEdgeWrappedHttpRows(currentLine.length) + + const candidates: WrappedLogicalLine[] = [] + const translatedLines = new Map() + const getTranslatedLine = (rowY: number): TrimmedTranslatedLine | null => { + if (translatedLines.has(rowY)) { + return translatedLines.get(rowY) ?? null + } + const line = buffer.getLine(rowY) + const translated = line ? trimRightTranslatedLine(line) : null + translatedLines.set(rowY, translated) + return translated + } + const minY = Math.max(0, currentY - maxRows + 1) + for (let startY = currentY; startY >= minY; startY--) { + const start = getTranslatedLine(startY) + const schemeIndex = start ? findHttpSchemeIndex(start.text) : -1 + if (!start || schemeIndex === -1 || VERTICAL_LAYOUT_FRAME_PATTERN.test(start.text)) { + continue + } + + let text = '' + const rows: WrappedLogicalLine['rows'] = [] + for (let rowY = startY; rowY < startY + maxRows; rowY++) { + const line = buffer.getLine(rowY) + const translated = getTranslatedLine(rowY) + if (!line || !translated) { + break + } + if (VERTICAL_LAYOUT_FRAME_PATTERN.test(translated.text)) { + break + } + if ( + rows.length > 0 && + !line.isWrapped && + !rowsCanContinueAtTerminalEdge(rows.at(-1)!, translated) + ) { + break + } + const fragmentStartIndex = rowY === startY ? schemeIndex : 0 + const fragment = translated.text.slice(fragmentStartIndex) + const whitespaceIndex = fragment.search(/\s/) + const possibleUrlFragmentLength = whitespaceIndex === -1 ? fragment.length : whitespaceIndex + if (text.length + possibleUrlFragmentLength > TERMINAL_HTTP_URL_MAX_LENGTH) { + break + } + + rows.push({ + y: rowY, + text: fragment, + sourceText: translated.sourceText, + columns: translated.columns.slice(fragmentStartIndex), + startIndex: text.length, + isWrapped: line.isWrapped, + lineLength: line.length + }) + text += fragment + if (rows.length > 1 && rowY >= currentY) { + candidates.push({ + text, + rows: [...rows], + fingerprint: `edge-http:${rows.map((row) => `${row.y}:${row.sourceText}`).join('\0')}` + }) + } + } + } + + return candidates.sort( + (left, right) => right.rows.length - left.rows.length || right.text.length - left.text.length + ) +} diff --git a/src/renderer/src/components/terminal-pane/repro-8832-url-next-line.test.ts b/src/renderer/src/components/terminal-pane/repro-8832-url-next-line.test.ts new file mode 100644 index 00000000000..14fbe14aa81 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/repro-8832-url-next-line.test.ts @@ -0,0 +1,203 @@ +/** + * Issue #8832 — Cmd-click URL must not glue the next logical line. + * + * Root cause: path hard-wrap reconstruction (from #8339) still joins a URL + * suffix ending in `/` with the next row's path-like prefix. HTTP hit-testing + * must not consume those candidates; it uses soft-wrap, framed hard-wrap HTTP, + * and edge-wrap reconstruction only. + * + * Re-run: + * pnpm exec vitest run --config config/vitest.config.ts \ + * src/renderer/src/components/terminal-pane/repro-8832-url-next-line.test.ts + */ +import type { IBufferLine } from '@xterm/xterm' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { registerHttpLinkStoreAccessor } from '@/lib/http-link-routing' +import { buildCandidateLogicalLinesForBufferPosition } from './terminal-file-link-hit-testing' +import { + extractTerminalHttpLinks, + openHttpLinkAtBufferPosition +} from './terminal-url-link-hit-testing' +import { buildHardWrappedPathLogicalLineCandidates } from './wrapped-terminal-link-ranges' + +const LINE_1 = 'Repo: https://github.com/stablyai/orca/' +const LINE_2 = 'Description: 123' +const EXPECTED_URL = 'https://github.com/stablyai/orca/' +const BUGGY_URL = 'https://github.com/stablyai/orca/Description' + +const openUrlMock = vi.fn() + +function makeBufferLine( + text: string, + options: { cols?: number; isWrapped?: boolean } = {} +): IBufferLine { + const cols = options.cols ?? Math.max(text.length, 80) + const padded = text.padEnd(cols) + return { + isWrapped: options.isWrapped ?? false, + length: cols, + getCell: () => undefined, + translateToString: ( + _trimRight?: boolean, + startColumn = 0, + endColumn = padded.length, + outColumns?: number[] + ) => { + if (outColumns) { + outColumns.length = 0 + for (let index = startColumn; index <= endColumn; index++) { + outColumns.push(index) + } + } + return padded.slice(startColumn, endColumn) + } + } as IBufferLine +} + +function twoRowBuffer( + row0: string, + row1: string, + options: { cols?: number; softWrapped?: boolean } = {} +): { getLine(y: number): IBufferLine | undefined } { + const cols = options.cols ?? 120 + const rows = [ + makeBufferLine(row0, { cols, isWrapped: false }), + makeBufferLine(row1, { cols, isWrapped: options.softWrapped === true }) + ] + return { getLine: (y: number) => rows[y] } +} + +function issueBuffer(): { getLine(y: number): IBufferLine | undefined } { + return twoRowBuffer(LINE_1, LINE_2, { cols: 120 }) +} + +function openUrlAt( + buffer: { getLine(y: number): IBufferLine | undefined }, + x: number, + y: number, + cols = 120 +) { + openUrlMock.mockReset() + const opened = openHttpLinkAtBufferPosition(buffer, { x, y }, cols, { + worktreeId: 'wt-repro-8832', + forceSystemBrowser: true + }) + return { opened, url: openUrlMock.mock.calls[0]?.[0] as string | undefined } +} + +describe('#8832 hard-wrapped path candidates glue next-line text into URLs', () => { + beforeEach(() => { + vi.stubGlobal('navigator', { userAgent: 'Macintosh' }) + vi.stubGlobal('window', { api: { shell: { openUrl: openUrlMock } } }) + registerHttpLinkStoreAccessor(() => ({ + settings: { openLinksInApp: false }, + setActiveWorktree: vi.fn(), + createBrowserTab: vi.fn() + })) + openUrlMock.mockReset() + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('still builds a multi-row path candidate that concatenates Description onto the URL', () => { + // Why: documents the path-reconstruction poison that HTTP hit-testing must + // not consume. File links remain protected by existence checks. + const buffer = issueBuffer() + const candidates = buildHardWrappedPathLogicalLineCandidates(buffer, 1) + const multiRow = candidates.filter((candidate) => candidate.rows.length > 1) + + expect(multiRow.some((candidate) => candidate.text.includes('Description'))).toBe(true) + + const glued = multiRow.find((candidate) => candidate.text.includes('https://')) + expect(glued).toBeDefined() + expect(glued!.text).toContain('https://github.com/stablyai/orca/Description') + }) + + it('HTTP extraction on a path logical line still yields the glued URL', () => { + const buffer = issueBuffer() + const candidates = buildCandidateLogicalLinesForBufferPosition(buffer, 1) + const extracted = candidates.flatMap((line) => extractTerminalHttpLinks(line.text)) + + expect(extracted.map((link) => link.url)).toContain(BUGGY_URL) + const singleLineUrls = extractTerminalHttpLinks(LINE_1).map((link) => link.url) + expect(singleLineUrls).toEqual([EXPECTED_URL]) + }) + + it('openHttpLinkAtBufferPosition opens the unglued URL when Cmd-clicking the URL row', () => { + const buffer = issueBuffer() + const urlStart = LINE_1.indexOf('https://') + const { opened, url } = openUrlAt(buffer, urlStart + 10, 1) + + expect(opened).toBe(true) + expect(url).toBe(EXPECTED_URL) + expect(url).not.toBe(BUGGY_URL) + }) + + it.each([ + ['Chinese label', '说明: 中文路径/文件.ts'], + ['Windows path', 'C:\\Users\\demo\\project\\README.md'], + ['POSIX path', '/usr/local/bin/orca'], + ['relative path', './src/main/index.ts'], + ['UNC-ish path', '\\\\server\\share\\file.txt'] + ])('does not glue a mid-row URL to a next-line %s', (_label, nextLine) => { + const row0 = 'See https://example.com/docs/' + const buffer = twoRowBuffer(row0, nextLine, { cols: 100 }) + const urlStart = row0.indexOf('https://') + const { opened, url } = openUrlAt(buffer, urlStart + 8, 1) + + expect(opened).toBe(true) + expect(url).toBe('https://example.com/docs/') + expect(url).not.toMatch(/说明|Users|usr|src|server/) + }) + + it('still joins a soft-wrapped multi-line URL via native wrap metadata', () => { + const cols = 40 + const url = 'https://example.com/very/long/path/segments/that/continue/more' + const row0 = url.slice(0, cols) + const row1 = url.slice(cols) + const buffer = twoRowBuffer(row0, row1, { cols, softWrapped: true }) + const first = openUrlAt(buffer, 11, 1, cols) + const second = openUrlAt(buffer, 5, 2, cols) + + expect(first.opened).toBe(true) + expect(first.url).toBe(url) + expect(second.opened).toBe(true) + expect(second.url).toBe(url) + }) + + it('still joins an edge-wrapped multi-line URL without wrap metadata', () => { + const cols = 40 + const url = 'https://example.com/very/long/path/segments/that/continue/more' + const row0 = url.slice(0, cols) + const row1 = url.slice(cols) + expect(row0).toHaveLength(cols) + const buffer = twoRowBuffer(row0, row1, { cols, softWrapped: false }) + const first = openUrlAt(buffer, 11, 1, cols) + const second = openUrlAt(buffer, 5, 2, cols) + + expect(first.opened).toBe(true) + expect(first.url).toBe(url) + expect(second.opened).toBe(true) + expect(second.url).toBe(url) + }) + + it('does not treat file:// as an HTTP link (file routing stays separate)', () => { + const row0 = 'open file:///Users/demo/project/README.md' + const buffer = twoRowBuffer(row0, 'next: line', { cols: 100 }) + const { opened, url } = openUrlAt(buffer, row0.indexOf('file://') + 4, 1) + + expect(opened).toBe(false) + expect(url).toBeUndefined() + }) + + it('opens only the HTTP URL when a Windows path shares the same row', () => { + const row0 = 'https://example.com/a C:\\Users\\demo\\file.txt' + const buffer = twoRowBuffer(row0, 'Description: more', { cols: 100 }) + const { opened, url } = openUrlAt(buffer, 8, 1) + + expect(opened).toBe(true) + expect(url).toBe('https://example.com/a') + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-url-link-click.test.ts b/src/renderer/src/components/terminal-pane/terminal-url-link-click.test.ts index c8e85950da3..36b38c09a5e 100644 --- a/src/renderer/src/components/terminal-pane/terminal-url-link-click.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-url-link-click.test.ts @@ -379,6 +379,44 @@ describe('hard-wrapped terminal HTTP clicks', () => { expect(openUrlMock).toHaveBeenCalledWith('http://example.com/') }) + it('does not glue the next logical line onto a URL that ends mid-row (#8832)', () => { + const { terminal, registrations } = makeTerminal({ + cols: 80, + urlRows: ['Repo: https://github.com/stablyai/orca/', 'Description: 123'], + softWrapped: false + }) + const disposable = installHttpLinkClickFallback(terminal, { worktreeId: 'wt-1' }) + + const fallback = registrations.find( + ([name, _listener, options]) => name === 'mouseup' && options === undefined + )?.[1] + fallback!(mouseEventForRow(0)) + + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith('https://github.com/stablyai/orca/') + disposable.dispose() + }) + + it('still joins a URL hard-wrapped at the row edge without native wrap metadata', () => { + const cols = 40 + const url = 'https://example.com/very/long/path/segments/that/continue/more' + const { terminal, registrations } = makeTerminal({ + cols, + urlRows: [url.slice(0, cols), url.slice(cols)], + softWrapped: false + }) + const disposable = installHttpLinkClickFallback(terminal, { worktreeId: 'wt-1' }) + + const fallback = registrations.find( + ([name, _listener, options]) => name === 'mouseup' && options === undefined + )?.[1] + fallback!(mouseEventForRow(0)) + + expect(openUrlMock).toHaveBeenCalledOnce() + expect(openUrlMock).toHaveBeenCalledWith(url) + disposable.dispose() + }) + it('does not suppress a modifier-click when the buffer position is not an HTTP link', () => { const { terminal, registrations } = makeTerminal({ urlRows: ['not-a-link'] }) const disposable = installHttpLinkClickFallback(terminal, { worktreeId: 'wt-1' }) diff --git a/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts b/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts index bfbbe29f723..3a6e2c1f274 100644 --- a/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts +++ b/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts @@ -1,10 +1,8 @@ import type { IBufferLine, IBufferRange, IDisposable, Terminal } from '@xterm/xterm' import { openHttpLink } from '@/lib/http-link-routing' -import { - buildCandidateLogicalLinesForBufferPosition, - dedupeLogicalLines -} from './terminal-file-link-hit-testing' +import { buildEdgeWrappedHttpLogicalLineCandidates } from './edge-wrapped-terminal-http-links' import { buildHardWrappedHttpLogicalLineCandidates } from './hard-wrapped-terminal-http-links' +import { dedupeLogicalLines } from './terminal-file-link-hit-testing' import { isTerminalHttpLinkActivation } from './terminal-http-link-activation' import { installTerminalLinkPtyMouseSuppression } from './terminal-link-pty-mouse-suppression' import { getTerminalBufferPositionForMouseEvent } from './terminal-mouse-buffer-position' @@ -271,7 +269,10 @@ function findHttpLinkAtBufferPosition( ? [nativeWrappedLogicalLine] : []), ...buildHardWrappedHttpLogicalLineCandidates(buffer, position.y), - ...buildCandidateLogicalLinesForBufferPosition(buffer, position.y) + ...buildEdgeWrappedHttpLogicalLineCandidates(buffer, position.y), + ...(nativeWrappedLogicalLine && nativeWrappedLogicalLine.rows.length === 1 + ? [nativeWrappedLogicalLine] + : []) ]) if (logicalLines.length === 0) { return null @@ -290,6 +291,17 @@ function findHttpLinkAtBufferPosition( return null } +function rangeContainsBufferPosition( + range: IBufferRange, + position: { x: number; y: number }, + terminalColumns: number +): boolean { + const lower = range.start.y * terminalColumns + range.start.x + const upper = range.end.y * terminalColumns + range.end.x + const current = position.y * terminalColumns + position.x + return lower <= current && current <= upper +} + export function openTerminalHttpLink(url: string, deps: UrlLinkHitTestDeps): void { if (deps.forceSystemBrowser) { openHttpLink(url, { worktreeId: deps.worktreeId, forceSystemBrowser: true }) @@ -316,14 +328,3 @@ export function openTerminalHttpLink(url: string, deps: UrlLinkHitTestDeps): voi openHttpLink(url, { worktreeId: deps.worktreeId, forceSystemBrowser: true }) }) } - -function rangeContainsBufferPosition( - range: IBufferRange, - position: { x: number; y: number }, - terminalColumns: number -): boolean { - const lower = range.start.y * terminalColumns + range.start.x - const upper = range.end.y * terminalColumns + range.end.x - const current = position.y * terminalColumns + position.x - return lower <= current && current <= upper -} From 539e0601d5f14b350a8ca20c408326058efa05b3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:24:10 -0700 Subject: [PATCH 3/4] fix(github): render GitHub Enterprise PR avatars via API avatar_url (#9107) Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: davkim1030 --- src/main/github/client.ts | 35 +++-- src/main/github/work-item-details.test.ts | 112 ++++++++++++++ src/main/github/work-item-details.ts | 138 ++++++++++++++++-- .../src/components/GitHubItemDialog.tsx | 13 +- .../src/components/PullRequestPage.tsx | 23 +-- src/renderer/src/components/TaskPage.tsx | 18 +-- .../github/github-user-avatar.test.tsx | 107 ++++++++++++++ .../components/github/github-user-avatar.tsx | 104 +++++++++++++ .../repro-8784-ghe-avatar-fallback.test.ts | 55 +++++++ src/shared/types.ts | 5 + 10 files changed, 547 insertions(+), 63 deletions(-) create mode 100644 src/renderer/src/components/github/github-user-avatar.test.tsx create mode 100644 src/renderer/src/components/github/github-user-avatar.tsx create mode 100644 src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 54572bd406f..ac85669836a 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -485,16 +485,32 @@ function mapIssueWorkItem(item: Record): MainWorkItem { .filter(Boolean) : [], updatedAt: String(item.updated_at ?? item.updatedAt ?? ''), - author: - typeof item.user === 'object' && item.user !== null && 'login' in item.user - ? String((item.user as { login?: unknown }).login ?? '') - : typeof item.author === 'object' && item.author !== null && 'login' in item.author - ? String((item.author as { login?: unknown }).login ?? '') - : null, + ...authorFieldsFromUnknown(item), ...(item.assignees !== undefined ? { assignees: usersFromUnknown(item.assignees) } : {}) } } +/** + * Derive both the author login and its API avatar_url in one place so GHE + * avatars render (the login-only `github.com/{login}.png` URL 404s on GHE). + * + * REST exposes `user.avatar_url`; gh/GraphQL expose `author.avatarUrl`. `gh pr + * view` omits the avatar entirely, so authorAvatarUrl is left undefined and the + * UI falls back to the login URL then a placeholder. See #8784. + */ +function authorFieldsFromUnknown( + item: Record +): Pick { + const user = userFromUnknown(item.user ?? item.author) + if (!user) { + return { author: null } + } + return { + author: user.login, + ...(user.avatarUrl ? { authorAvatarUrl: user.avatarUrl } : {}) + } +} + function extractHeadOwnerLogin(item: Record): string | null { // gh CLI `pr list --json headRepositoryOwner` shape: { login } if (typeof item.headRepositoryOwner === 'object' && item.headRepositoryOwner !== null) { @@ -746,12 +762,7 @@ function mapPullRequestWorkItem( .filter(Boolean) : [], updatedAt: String(item.updated_at ?? item.updatedAt ?? ''), - author: - typeof item.user === 'object' && item.user !== null && 'login' in item.user - ? String((item.user as { login?: unknown }).login ?? '') - : typeof item.author === 'object' && item.author !== null && 'login' in item.author - ? String((item.author as { login?: unknown }).login ?? '') - : null, + ...authorFieldsFromUnknown(item), branchName: typeof item.head === 'object' && item.head !== null && 'ref' in item.head ? String((item.head as { ref?: unknown }).ref ?? '') diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index 6d3d14c99b7..4fd21131c23 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -216,6 +216,56 @@ describe('getWorkItemDetails', () => { expect(details?.participants?.[0]?.login).toBe('issue-author') }) + it('enriches a non-participating assignee avatar from the GraphQL assignees connection', async () => { + // Why: a GHE assignee who never commented is absent from `participants`, so + // the enrichment must also draw avatars from the `assignees` connection or + // item.assignees keeps the blank avatar `gh` returns. + getWorkItemMock.mockResolvedValueOnce({ + id: 'issue:924', + type: 'issue', + number: 924, + title: 'Assignee avatar', + state: 'open', + url: 'https://github.com/acme/widgets/issues/924', + labels: [], + updatedAt: '2026-04-01T00:00:00Z', + author: 'issue-author', + assignees: [{ login: 'ghe-assignee', name: 'GHE Assignee', avatarUrl: '' }] + }) + getIssueOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + data: { + repository: { + issue: { + body: 'Issue body', + assignees: { + nodes: [ + { + login: 'ghe-assignee', + name: 'GHE Assignee', + avatarUrl: 'https://ghe.example.com/avatars/ghe-assignee' + } + ] + }, + participants: { nodes: [{ login: 'issue-author', avatarUrl: 'https://x/y' }] }, + comments: { nodes: [] } + } + } + } + }) + }) + .mockResolvedValueOnce({ stdout: '' }) + + const details = await getWorkItemDetails('/repo-root', 924, 'issue') + + expect(details?.item.assignees?.[0]).toMatchObject({ + login: 'ghe-assignee', + avatarUrl: 'https://ghe.example.com/avatars/ghe-assignee' + }) + }) + it('caps issue timeline pagination by supported activity items', async () => { getWorkItemMock.mockResolvedValueOnce({ id: 'issue:923', @@ -568,4 +618,66 @@ describe('getWorkItemDetails', () => { expect(details?.filesUnavailable).toBe(false) expect(details?.files).toEqual([]) }) + + // Why: `gh pr view` omits avatar_url, so the login-based github.com URL 404s on + // GHE. getWorkItemDetails must resolve author/reviewer/assignee avatars via the + // GraphQL user(login:) batch and stamp them onto the returned item. See #8784. + it('enriches PR author, reviewer, and assignee avatars from the GraphQL user lookup', async () => { + getWorkItemMock.mockResolvedValueOnce({ + id: 'pr:1102', + type: 'pr', + number: 1102, + title: 'Enterprise PR', + state: 'open', + url: 'https://ghe.example.com/acme/widgets/pull/1102', + labels: [], + updatedAt: '2026-07-11T00:00:00Z', + author: 'seah', + reviewRequests: [{ login: 'ludi', name: null, avatarUrl: '' }], + latestReviews: [{ login: 'inho', state: 'APPROVED', avatarUrl: '' }], + // A default `u/0` placeholder must be replaced by the resolved avatar. + assignees: [{ login: 'seah', name: 'Seah', avatarUrl: 'https://avatars.example.com/u/0?v=4' }] + }) + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getPRCommentsMock.mockResolvedValue([]) + getPRChecksMock.mockResolvedValue([]) + const avatars: Record = { + seah: 'https://avatars.example.com/u/1?v=4', + ludi: 'https://avatars.example.com/u/2?v=4', + inho: 'https://avatars.example.com/u/3?v=4' + } + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const target = args.at(-1) + if (target === 'repos/acme/widgets/pulls/1102') { + return { + stdout: JSON.stringify({ body: 'PR body', head: { sha: 'h' }, base: { sha: 'b' } }) + } + } + if (target === 'repos/acme/widgets/pulls/1102/files?per_page=100') { + return { stdout: '[]' } + } + const query = args.find((arg) => arg.startsWith('query=')) ?? '' + if (query.includes('user(login:')) { + // Return the aliased users the batch asked for, keyed by their login. + const data: Record = {} + let index = 0 + for (const login of Object.keys(avatars)) { + if (query.includes(`user(login: "${login}")`)) { + data[`u${index}`] = { login, name: null, avatarUrl: avatars[login] } + index += 1 + } + } + return { stdout: JSON.stringify({ data }) } + } + return { stdout: JSON.stringify({ data: {} }) } + }) + + const details = await getWorkItemDetails('/repo-root', 1102, 'pr') + + expect(details?.item.authorAvatarUrl).toBe(avatars.seah) + expect(details?.item.reviewRequests?.[0]?.avatarUrl).toBe(avatars.ludi) + expect(details?.item.latestReviews?.[0]?.avatarUrl).toBe(avatars.inho) + // The default u/0 placeholder is overridden by the resolved avatar. + expect(details?.item.assignees?.[0]?.avatarUrl).toBe(avatars.seah) + }) }) diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 9aa2de2b26b..bd126641fcb 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -85,7 +85,7 @@ const ISSUE_DETAILS_QUERY = `query($owner: String!, $repo: String!, $number: Int repository(owner: $owner, name: $repo) { issue(number: $number) { body - assignees(first: 50) { nodes { login } } + assignees(first: 50) { nodes { login avatarUrl(size: 48) ... on User { name } } } participants(first: 100) { nodes { login avatarUrl(size: 48) ... on User { name } } } @@ -111,7 +111,7 @@ type GraphQLIssueDetailsResponse = { repository?: { issue?: { body?: string | null - assignees?: { nodes?: { login?: string }[] } + assignees?: { nodes?: { login?: string; avatarUrl?: string; name?: string | null }[] } participants?: { nodes?: GitHubAssignableUser[] } comments?: { nodes?: { @@ -323,6 +323,12 @@ async function getIssueTimelineItems( } } +/** + * Fetch an issue's body, comments, assignees, participants, and timeline in a + * single GraphQL round-trip. Returns null on any partial error so the caller + * falls back to the strict REST path. Assignee/participant avatars are resolved + * here so GitHub Enterprise users don't render blank. + */ async function getIssueDetailsViaGraphQL( repoPath: string, issueNumber: number, @@ -332,6 +338,10 @@ async function getIssueDetailsViaGraphQL( body: string comments: PRComment[] assignees: string[] + // Avatar-bearing assignees, kept separate from the `assignees` login list so + // callers that only need logins are unaffected; used to enrich assignee + // avatars that `gh` leaves blank (notably GitHub Enterprise users). + assigneeUsers: GitHubAssignableUser[] participants: GitHubAssignableUser[] timelineItems: GitHubIssueTimelineItem[] } | null> { @@ -386,9 +396,16 @@ async function getIssueDetailsViaGraphQL( url: c.url ?? '', isBot: c.author?.__typename === 'Bot' })) - const assignees = (issue.assignees?.nodes ?? []) - .map((a) => a.login) - .filter((login): login is string => Boolean(login)) + const assigneeUsers: GitHubAssignableUser[] = (issue.assignees?.nodes ?? []) + .filter((a): a is { login: string; avatarUrl?: string; name?: string | null } => + Boolean(a.login) + ) + .map((a) => ({ + login: a.login, + name: a.name ?? null, + avatarUrl: a.avatarUrl ?? '' + })) + const assignees = assigneeUsers.map((a) => a.login) const participants: GitHubAssignableUser[] = (issue.participants?.nodes ?? []) .filter((u) => Boolean(u.login)) .map((u) => ({ @@ -401,6 +418,7 @@ async function getIssueDetailsViaGraphQL( body: issue.body ?? '', comments, assignees, + assigneeUsers, participants, timelineItems } @@ -877,6 +895,11 @@ async function getGitHubUsersByLogin( return [] } if (rateLimitGuard('graphql').blocked) { + // Why: surface the skip. Callers degrade silently to login-based avatar URLs + // (blank on GHE), so without this log "why are avatars missing" is untraceable. + console.warn( + `getGitHubUsersByLogin skipped: GraphQL rate-limit budget exhausted (${uniqueLogins.length} logins unresolved)` + ) return [] } const fields = uniqueLogins @@ -921,19 +944,100 @@ async function getGitHubUsersByLogin( } } +/** + * Stamp GraphQL-resolved avatars onto a work item's author, reviewers, review + * requests, latest reviews, and assignees. + * + * The work item's users come from `gh pr view`, which omits avatar_url. On + * GitHub Enterprise the login-based `github.com/{login}.png` URL 404s, so those + * avatars render blank while comment avatars (fetched via GraphQL) work. + * `knownUsers` (participants + the mention-author batch, which already includes + * the item's reviewers/assignees) carries the resolved avatars, so we apply + * them without any extra round-trip. See #8784. + */ +function enrichItemDisplayAvatars( + item: Omit, + knownUsers: GitHubAssignableUser[] +): Omit { + const avatarByLogin = new Map() + for (const user of knownUsers) { + if (user.login && user.avatarUrl) { + avatarByLogin.set(user.login.toLowerCase(), user.avatarUrl) + } + } + if (avatarByLogin.size === 0) { + return item + } + // Why: prefer the GraphQL-resolved avatar over whatever `gh` returned. `gh pr + // view` often yields an empty avatar or the default `u/0` placeholder for + // enterprise users; the resolved avatar is the authoritative one for that + // login. Fall back to the original only when the lookup found nothing. + const avatarFor = (login: string): string | undefined => avatarByLogin.get(login.toLowerCase()) + const resolvedAvatar = (login: string, existing?: string | null): string | undefined => + avatarFor(login) || existing || undefined + // Callers coalesce a missing result to the field's "no avatar" sentinel ('' + // for GitHubAssignableUser, null for GitHubPRReviewSummary); GitHubUserAvatar + // treats both as falsy and falls back to the login URL then initials. + const authorAvatarUrl = (item.author ? avatarFor(item.author) : undefined) || item.authorAvatarUrl + return { + ...item, + ...(authorAvatarUrl ? { authorAvatarUrl } : {}), + ...(item.reviewRequests + ? { + reviewRequests: item.reviewRequests.map((user) => ({ + ...user, + avatarUrl: resolvedAvatar(user.login, user.avatarUrl) ?? '' + })) + } + : {}), + ...(item.latestReviews + ? { + latestReviews: item.latestReviews.map((review) => ({ + ...review, + avatarUrl: resolvedAvatar(review.login, review.avatarUrl) ?? null + })) + } + : {}), + ...(item.assignees + ? { + assignees: item.assignees.map((user) => ({ + ...user, + avatarUrl: resolvedAvatar(user.login, user.avatarUrl) ?? '' + })) + } + : {}) + } +} + async function getMentionParticipants( repoPath: string, - item: Pick, + item: Pick< + GitHubWorkItem, + 'author' | 'number' | 'type' | 'reviewRequests' | 'latestReviews' | 'assignees' + >, comments: PRComment[], participants: GitHubAssignableUser[], connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const visibleLogins = [item.author ?? '', ...comments.map((comment) => comment.author)] - // Why: one aliased GraphQL query returns login/name/avatarUrl for every - // mentioned author in a single round-trip. The previous REST fan-out - // (/users/) returned the same fields but cost one rate-limit point - // per user. + // Why: resolve mention authors AND the item's reviewers/assignees in ONE + // aliased GraphQL round-trip. Folding the display users in here means the + // caller can reuse this result to stamp avatars (see enrichItemDisplayAvatars) + // without a second, separately rate-limited lookup — which is what made GHE + // reviewer/assignee avatars resolve intermittently. See #8784. + // Why: order the always-visible display users (author, reviewers, assignees) + // before comment authors so the 40-login cap in getGitHubUsersByLogin never + // drops a reviewer/assignee avatar on a PR with many commenters. On a PR with + // 40+ distinct participants the trailing comment authors are dropped from this + // batch and lose only their @mention-suggestion avatar; rendered comment + // avatars are unaffected (they carry their own avatarUrl from the comment). + const visibleLogins = [ + item.author ?? '', + ...(item.reviewRequests ?? []).map((user) => user.login), + ...(item.latestReviews ?? []).map((review) => review.login), + ...(item.assignees ?? []).map((user) => user.login), + ...comments.map((comment) => comment.author) + ] const graphQlUsers = await getGitHubUsersByLogin( repoPath, visibleLogins, @@ -1007,7 +1111,13 @@ export async function getWorkItemDetails( ) if (collapsed) { return { - item, + // Include assigneeUsers: a non-participating assignee is absent from + // the participants connection, so without them item.assignees keeps a + // blank/placeholder avatar (common for GitHub Enterprise users). + item: enrichItemDisplayAvatars(item, [ + ...collapsed.participants, + ...collapsed.assigneeUsers + ]), body: collapsed.body, comments: collapsed.comments, assignees: collapsed.assignees, @@ -1030,7 +1140,7 @@ export async function getWorkItemDetails( localGitOptions ) return { - item, + item: enrichItemDisplayAvatars(item, mentionParticipants), body, comments, assignees, @@ -1064,7 +1174,7 @@ export async function getWorkItemDetails( ]) return { - item, + item: enrichItemDisplayAvatars(item, mentionParticipants), body, comments, headSha: shas?.headSha, diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index d076f0b325d..6f6b7fb2424 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -181,7 +181,7 @@ import { filterGitHubPRReviewerCandidates, getGitHubPRReviewerQueryState } from '@/components/github/github-pr-reviewer-candidate-filter' -import { githubAvatarUrl } from '@/components/github/github-issue-comment-helpers' +import { GitHubUserAvatar } from '@/components/github/github-user-avatar' import { presentGitHubPRMergeState } from '@/components/github-pr-merge-state' import { GITHUB_PR_MERGE_METHOD_LABELS, @@ -421,16 +421,7 @@ function ReviewerAvatar({ login: string avatarUrl: string }): React.JSX.Element { - return ( - - ) + return } function mergeReviewerSuggestions( diff --git a/src/renderer/src/components/PullRequestPage.tsx b/src/renderer/src/components/PullRequestPage.tsx index d807dacf89f..b53ba354618 100644 --- a/src/renderer/src/components/PullRequestPage.tsx +++ b/src/renderer/src/components/PullRequestPage.tsx @@ -157,7 +157,7 @@ import { filterGitHubPRReviewerCandidates, getGitHubPRReviewerQueryState } from '@/components/github/github-pr-reviewer-candidate-filter' -import { githubAvatarUrl } from '@/components/github/github-issue-comment-helpers' +import { GitHubUserAvatar } from '@/components/github/github-user-avatar' import { filterGitHubMentionOptions } from '@/components/github/github-mention-option-filter' import { getCommentBodySubmitState, @@ -468,16 +468,7 @@ function ReviewerAvatar({ login: string avatarUrl: string }): React.JSX.Element { - return ( - - ) + return } function mergeReviewerSuggestions( @@ -7148,12 +7139,10 @@ export default function PullRequestPage({ {workItem.author ? ( - ) : null} diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index af47d26736d..e43cc6a8089 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -133,6 +133,7 @@ import { } from '../../../shared/linear-links' import PRFilterDropdowns, { type PRFilterChange } from '@/components/github/PRFilterDropdowns' import { GitHubMarkdownComposer } from '@/components/github/GitHubMarkdownComposer' +import { GitHubUserAvatar } from '@/components/github/github-user-avatar' import { buildGitHubRepoUrl, parseGitHubIssueOrPRLink } from '@/lib/github-links' import { findGithubWorkItemWorkspaceAttachment, @@ -1520,17 +1521,16 @@ function ReviewChipAvatar({ reviewer: GitHubPRPrimaryReviewer | null }): React.JSX.Element { if (reviewer?.login) { - // Why: `gh pr list --json reviewRequests` can return only logins; GitHub's - // public avatar endpoint keeps the list visual aligned with assignee cells. - const avatarUrl = reviewer.avatarUrl || `https://github.com/${reviewer.login}.png?size=40` + // Why: `gh pr list --json reviewRequests` can return only logins. Prefer the + // API avatar_url so GHE renders; GitHubUserAvatar falls back to the login URL + // and then an initials placeholder when no avatar loads. See #8784. return ( - ) } diff --git a/src/renderer/src/components/github/github-user-avatar.test.tsx b/src/renderer/src/components/github/github-user-avatar.test.tsx new file mode 100644 index 00000000000..5eeed156426 --- /dev/null +++ b/src/renderer/src/components/github/github-user-avatar.test.tsx @@ -0,0 +1,107 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, describe, expect, it } from 'vitest' +import { GitHubUserAvatar, resolveGitHubUserAvatarSrc } from './github-user-avatar' + +function render(node: React.JSX.Element): { root: Root; container: HTMLDivElement } { + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + act(() => { + root.render(node) + }) + return { root, container } +} + +describe('GitHubUserAvatar', () => { + let root: Root | null = null + let container: HTMLDivElement | null = null + + afterEach(() => { + act(() => root?.unmount()) + container?.remove() + root = null + container = null + }) + + it('renders the API avatar_url when provided (works for github.com and GHE)', () => { + const url = 'https://avatars.example.com/u/42?u=hash&v=4' + ;({ root, container } = render()) + const img = container.querySelector('img') + expect(img?.getAttribute('src')).toBe(url) + }) + + it('falls back to the login URL when no avatar_url is available', () => { + ;({ root, container } = render()) + const img = container.querySelector('img') + expect(img?.getAttribute('src')).toBe('https://github.com/octocat.png?size=64') + }) + + it('degrades to an initials placeholder when the image fails to load (GHE 404)', () => { + ;({ root, container } = render( + + )) + const img = container.querySelector('img') + expect(img).not.toBeNull() + act(() => { + img?.dispatchEvent(new Event('error')) + }) + expect(container.querySelector('img')).toBeNull() + expect(container.textContent).toBe('AL') + }) + + it('skips leading non-BMP chars so initials never render a broken surrogate', () => { + ;({ root, container } = render()) + act(() => { + container?.querySelector('img')?.dispatchEvent(new Event('error')) + }) + // Emoji word is skipped entirely rather than split into a broken half. + expect(container.textContent).toBe('D') + }) + + it('derives initials from the login when no name is given', () => { + ;({ root, container } = render()) + act(() => { + container?.querySelector('img')?.dispatchEvent(new Event('error')) + }) + expect(container.textContent).toBe('OC') + }) + + it('retries when avatarUrl changes after an earlier src failed (late-arriving avatar)', () => { + // Why: PR detail enrichment supplies avatar_url after first paint. A row that + // 404s on the early login-based URL must recover once the real URL lands. + ;({ root, container } = render()) + // First src is the login fallback; simulate the GHE 404. + act(() => { + container?.querySelector('img')?.dispatchEvent(new Event('error')) + }) + expect(container.querySelector('img')).toBeNull() + // Enriched avatar_url arrives → component must render the new image, not the + // latched placeholder. + const url = 'https://avatars.example.com/u/7?v=4' + act(() => { + root?.render() + }) + expect(container.querySelector('img')?.getAttribute('src')).toBe(url) + }) + + it('shows initials immediately when login and avatarUrl are both empty', () => { + ;({ root, container } = render()) + expect(container.querySelector('img')).toBeNull() + expect(container.textContent).toBe('AD') + }) +}) + +describe('resolveGitHubUserAvatarSrc', () => { + it('prefers trimmed API avatar over login.png', () => { + expect(resolveGitHubUserAvatarSrc('u', ' https://avatars.example.com/u/1?v=4 ')).toBe( + 'https://avatars.example.com/u/1?v=4' + ) + }) + + it('skips empty login so no github.com/.png request is built', () => { + expect(resolveGitHubUserAvatarSrc('', null)).toBeNull() + }) +}) diff --git a/src/renderer/src/components/github/github-user-avatar.tsx b/src/renderer/src/components/github/github-user-avatar.tsx new file mode 100644 index 00000000000..385c7feda93 --- /dev/null +++ b/src/renderer/src/components/github/github-user-avatar.tsx @@ -0,0 +1,104 @@ +import { useState } from 'react' +import { cn } from '@/lib/utils' +import { githubAvatarUrl } from '@/components/github/github-issue-comment-helpers' + +/** + * Build a 1-2 character initials placeholder from a display name or login, + * used when no avatar image is available or the image fails to load. + */ +function initialsFor(login: string, name?: string | null): string { + const source = (name?.trim() || login).trim() + if (!source) { + return '?' + } + // Iterate by code point (not UTF-16 unit) and keep only letters/digits, so a + // display name that leads with an emoji or other non-BMP char yields clean + // initials instead of a broken surrogate half. Words that are all symbols are + // skipped; an all-symbol name falls back to '?'. + const alnum = (word: string): string[] => [...word].filter((ch) => /[\p{L}\p{N}]/u.test(ch)) + const parts = source.split(/\s+/).filter(Boolean) + const letters = + parts.length >= 2 + ? parts + .map((word) => alnum(word)[0]) + .filter(Boolean) + .slice(0, 2) + .join('') + : alnum(source).slice(0, 2).join('') + return letters.toUpperCase() || '?' +} + +/** + * Prefer the API `avatar_url`, then the public login.png URL. Empty/whitespace + * API values and empty logins never produce a bogus request. See #8784. + */ +export function resolveGitHubUserAvatarSrc( + login: string, + avatarUrl?: string | null +): string | null { + const fromApi = avatarUrl?.trim() + if (fromApi) { + return fromApi + } + const trimmedLogin = login.trim() + if (!trimmedLogin) { + return null + } + return githubAvatarUrl(trimmedLogin) +} + +/** + * Avatar for a GitHub user that renders correctly on github.com and GitHub + * Enterprise. GHE logins don't exist on github.com, so the login-based + * `github.com/{login}.png` URL 404s. We prefer the API-provided `avatarUrl` + * and, when it (or the login fallback) fails to load, degrade to an initials + * placeholder instead of a broken image. See #8784. + */ +export function GitHubUserAvatar({ + login, + name, + avatarUrl, + title, + className +}: { + login: string + name?: string | null + avatarUrl?: string | null + title?: string + className?: string +}): React.JSX.Element { + // Why: track the specific src that failed rather than a boolean. Avatar data + // arrives after the first paint (PR detail enrichment), so a row that 404s on + // the early login-based URL must retry once the real avatar_url lands — a + // latched boolean would keep showing the placeholder forever. See #8784. + const [failedSrc, setFailedSrc] = useState(null) + const src = resolveGitHubUserAvatarSrc(login, avatarUrl) + if (src && failedSrc !== src) { + return ( + setFailedSrc(src)} + className={cn( + 'shrink-0 rounded-full border border-border/50 bg-muted object-cover', + className + )} + /> + ) + } + return ( + + {initialsFor(login, name)} + + ) +} diff --git a/src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts b/src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts new file mode 100644 index 00000000000..7b831270910 --- /dev/null +++ b/src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts @@ -0,0 +1,55 @@ +/** + * Issue #8784 — GHE PR avatars must prefer API avatar_url over github.com/{login}.png. + * + * Regression guard (was a repro that documented the broken path). After the fix: + * - resolveGitHubUserAvatarSrc prefers API URLs + * - PullRequestPage author uses authorAvatarUrl via GitHubUserAvatar + * - TaskPage ReviewChipAvatar no longer hardcodes github.com login.png + * + * Re-run: + * pnpm exec vitest run --config config/vitest.config.ts \ + * src/renderer/src/components/github/repro-8784-ghe-avatar-fallback.test.ts + */ +import { describe, expect, it } from 'vitest' +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { githubAvatarUrl } from './github-issue-comment-helpers' +import { resolveGitHubUserAvatarSrc } from './github-user-avatar' + +describe('issue #8784 GHE avatar fallback (regression)', () => { + it('prefers API avatar_url over login.png (GHE healthy path)', () => { + const api = 'https://ghe.example.com/avatars/u/42?v=4' + expect(resolveGitHubUserAvatarSrc('enterprise-only-user', api)).toBe(api) + }) + + it('trims whitespace-only avatarUrl and falls back to login.png for github.com', () => { + expect(resolveGitHubUserAvatarSrc('octocat', ' ')).toBe( + 'https://github.com/octocat.png?size=64' + ) + expect(resolveGitHubUserAvatarSrc('octocat', null)).toBe(githubAvatarUrl('octocat')) + }) + + it('returns null when neither avatarUrl nor login is usable (no bogus request)', () => { + expect(resolveGitHubUserAvatarSrc('', '')).toBeNull() + expect(resolveGitHubUserAvatarSrc(' ', null)).toBeNull() + }) + + it('login-only fallback still hardcodes public github.com png (github.com path)', () => { + // Why: github.com users without avatar_url still use this; GHE relies on + // enrichment + image onError → initials when this 404s. + expect(githubAvatarUrl('corp-user')).toBe('https://github.com/corp-user.png?size=64') + }) + + it('source routes PR author/reviewer avatars through GitHubUserAvatar + authorAvatarUrl', () => { + const prPage = readFileSync(join(__dirname, '../PullRequestPage.tsx'), 'utf8') + expect(prPage).toMatch(/GitHubUserAvatar/) + expect(prPage).toMatch(/authorAvatarUrl/) + // Why: author chip must not ignore API avatar_url and only pass login. + expect(prPage).not.toMatch(/githubAvatarUrl\(workItem\.author\)/) + + const taskPage = readFileSync(join(__dirname, '../TaskPage.tsx'), 'utf8') + expect(taskPage).toMatch(/GitHubUserAvatar/) + // Why: list chip must not hardcode github.com/{login}.png. + expect(taskPage).not.toMatch(/github\.com\/\$\{reviewer\.login\}\.png/) + }) +}) diff --git a/src/shared/types.ts b/src/shared/types.ts index ff46520c44d..36297481741 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -1491,6 +1491,11 @@ export type GitHubWorkItem = { labels: string[] updatedAt: string author: string | null + // Why: GHE user logins don't exist on github.com, so the github.com/{login}.png + // fallback 404s. Carry the API-provided avatar_url so github.com + Enterprise + // both render; absent on the gh-pr-view path (gh omits avatar), then the UI + // falls back to the login URL and finally an initials placeholder. See #8784. + authorAvatarUrl?: string branchName?: string baseRefName?: string // Why: PR checks are keyed by head commit; carrying this lets task rows use From e01f3f474c39ce1a35c83d07c2adf36a380c48d3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:38:13 -0700 Subject: [PATCH 4/4] fix(sidebar): preserve Project Group members under Hide sleeping (#9101) * fix(sidebar): preserve grouped projects under sleep filter * test(sidebar): harden #8865 project-group sleep-filter coverage Add adversarial cases for explicit project filters, multi-member groups, ungrouped neighbors, and buildRows membership/count preservation. Co-authored-by: bbingz --------- Co-authored-by: bbingz --- .../src/components/sidebar/WorktreeList.tsx | 3 +- .../empty-project-placeholder-repos.test.ts | 105 ++++++++++++++++++ .../empty-project-placeholder-repos.ts | 8 +- .../sidebar/worktree-list-groups.test.ts | 67 +++++++++++ 4 files changed, 181 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index 81671c20616..948228984d8 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -5711,9 +5711,10 @@ const WorktreeList = React.memo(function WorktreeList({ groupBy, repos: visibleReposForRows, worktreesByRepo, + visibleWorktrees, filterRepoIds }) - }, [filterRepoIds, groupBy, visibleReposForRows, worktreesByRepo]) + }, [filterRepoIds, groupBy, visibleReposForRows, visibleWorktrees, worktreesByRepo]) const allRepoIds = useMemo(() => repos.map((r) => r.id), [repos]) // Why: buildRows only needs which creates exist and their repo. Subscribe on a diff --git a/src/renderer/src/components/sidebar/empty-project-placeholder-repos.test.ts b/src/renderer/src/components/sidebar/empty-project-placeholder-repos.test.ts index aaa3be238e9..8a33151f73a 100644 --- a/src/renderer/src/components/sidebar/empty-project-placeholder-repos.test.ts +++ b/src/renderer/src/components/sidebar/empty-project-placeholder-repos.test.ts @@ -38,6 +38,7 @@ describe('getEmptyProjectPlaceholderRepoIds', () => { groupBy: 'repo', repos: [repo], worktreesByRepo: { [repo.id]: [] }, + visibleWorktrees: [], filterRepoIds: [] }) ) @@ -51,6 +52,7 @@ describe('getEmptyProjectPlaceholderRepoIds', () => { groupBy: 'repo', repos: [repo], worktreesByRepo: {}, + visibleWorktrees: [], filterRepoIds: [] }) ) @@ -67,6 +69,7 @@ describe('getEmptyProjectPlaceholderRepoIds', () => { groupBy: 'repo', repos: [selectedRepo, hiddenRepo], worktreesByRepo: { [selectedRepo.id]: [], [hiddenRepo.id]: [] }, + visibleWorktrees: [], filterRepoIds: [selectedRepo.id] }) ) @@ -79,6 +82,7 @@ describe('getEmptyProjectPlaceholderRepoIds', () => { groupBy: 'none', repos: [repo], worktreesByRepo: { [repo.id]: [] }, + visibleWorktrees: [], filterRepoIds: [] }).size ).toBe(0) @@ -90,8 +94,109 @@ describe('getEmptyProjectPlaceholderRepoIds', () => { groupBy: 'repo', repos: [repo], worktreesByRepo: { [repo.id]: [worktree] }, + visibleWorktrees: [], filterRepoIds: [] }).size ).toBe(0) }) + + it('keeps grouped repos visible when workspace filters hide all of their rows', () => { + const groupedRepo: Repo = { ...repo, projectGroupId: 'group-1' } + const groupedWorktree: Worktree = { ...worktree, repoId: groupedRepo.id } + + expect( + Array.from( + getEmptyProjectPlaceholderRepoIds({ + groupBy: 'repo', + repos: [groupedRepo], + worktreesByRepo: { [groupedRepo.id]: [groupedWorktree] }, + visibleWorktrees: [], + filterRepoIds: [] + }) + ) + ).toEqual([groupedRepo.id]) + }) + + it('does not create a grouped repo placeholder when one of its workspaces is visible', () => { + const groupedRepo: Repo = { ...repo, projectGroupId: 'group-1' } + const groupedWorktree: Worktree = { ...worktree, repoId: groupedRepo.id } + + expect( + getEmptyProjectPlaceholderRepoIds({ + groupBy: 'repo', + repos: [groupedRepo], + worktreesByRepo: { [groupedRepo.id]: [groupedWorktree] }, + visibleWorktrees: [groupedWorktree], + filterRepoIds: [] + }).size + ).toBe(0) + }) + + it('still respects explicit project filters for sleep-filtered grouped members', () => { + const selected: Repo = { ...repo, id: 'repo-selected', projectGroupId: 'group-1' } + const filteredOut: Repo = { ...repo, id: 'repo-hidden', projectGroupId: 'group-1' } + const selectedWt: Worktree = { ...worktree, id: 'wt-selected', repoId: selected.id } + const hiddenWt: Worktree = { ...worktree, id: 'wt-hidden', repoId: filteredOut.id } + + expect( + Array.from( + getEmptyProjectPlaceholderRepoIds({ + groupBy: 'repo', + repos: [selected, filteredOut], + worktreesByRepo: { + [selected.id]: [selectedWt], + [filteredOut.id]: [hiddenWt] + }, + // Why: simulate Hide sleeping removing every card while the project + // filter still intentionally excludes `filteredOut`. + visibleWorktrees: [], + filterRepoIds: [selected.id] + }) + ) + ).toEqual([selected.id]) + }) + + it('placeholders only the fully-filtered members of a multi-project group', () => { + const sleeping: Repo = { ...repo, id: 'repo-sleeping', projectGroupId: 'group-1' } + const awake: Repo = { ...repo, id: 'repo-awake', projectGroupId: 'group-1' } + const sleepingWt: Worktree = { ...worktree, id: 'wt-sleeping', repoId: sleeping.id } + const awakeWt: Worktree = { ...worktree, id: 'wt-awake', repoId: awake.id } + + expect( + Array.from( + getEmptyProjectPlaceholderRepoIds({ + groupBy: 'repo', + repos: [sleeping, awake], + worktreesByRepo: { + [sleeping.id]: [sleepingWt], + [awake.id]: [awakeWt] + }, + visibleWorktrees: [awakeWt], + filterRepoIds: [] + }) + ) + ).toEqual([sleeping.id]) + }) + + it('does not placeholder ungrouped neighbors of a filtered grouped member', () => { + const grouped: Repo = { ...repo, id: 'repo-grouped', projectGroupId: 'group-1' } + const ungrouped: Repo = { ...repo, id: 'repo-ungrouped' } + const groupedWt: Worktree = { ...worktree, id: 'wt-grouped', repoId: grouped.id } + const ungroupedWt: Worktree = { ...worktree, id: 'wt-ungrouped', repoId: ungrouped.id } + + expect( + Array.from( + getEmptyProjectPlaceholderRepoIds({ + groupBy: 'repo', + repos: [grouped, ungrouped], + worktreesByRepo: { + [grouped.id]: [groupedWt], + [ungrouped.id]: [ungroupedWt] + }, + visibleWorktrees: [], + filterRepoIds: [] + }) + ) + ).toEqual([grouped.id]) + }) }) diff --git a/src/renderer/src/components/sidebar/empty-project-placeholder-repos.ts b/src/renderer/src/components/sidebar/empty-project-placeholder-repos.ts index 26c248a2a65..57aee59c1bd 100644 --- a/src/renderer/src/components/sidebar/empty-project-placeholder-repos.ts +++ b/src/renderer/src/components/sidebar/empty-project-placeholder-repos.ts @@ -5,6 +5,7 @@ export function getEmptyProjectPlaceholderRepoIds(args: { groupBy: WorktreeGroupBy repos: readonly Repo[] worktreesByRepo: Readonly> + visibleWorktrees: readonly Worktree[] filterRepoIds: readonly string[] }): Set { if (args.groupBy !== 'repo') { @@ -12,12 +13,17 @@ export function getEmptyProjectPlaceholderRepoIds(args: { } const filterSet = args.filterRepoIds.length > 0 ? new Set(args.filterRepoIds) : null + const visibleRepoIds = new Set(args.visibleWorktrees.map((worktree) => worktree.repoId)) const placeholderRepoIds = new Set() for (const repo of args.repos) { if (filterSet && !filterSet.has(repo.id)) { continue } - if ((args.worktreesByRepo[repo.id]?.length ?? 0) === 0) { + const hasNoWorktrees = (args.worktreesByRepo[repo.id]?.length ?? 0) === 0 + // Why: workspace filters hide cards, but must not rewrite the visible + // membership of a persisted Project Group. #8865 + const isFilteredProjectGroupMember = repo.projectGroupId != null && !visibleRepoIds.has(repo.id) + if (hasNoWorktrees || isFilteredProjectGroupMember) { placeholderRepoIds.add(repo.id) } } diff --git a/src/renderer/src/components/sidebar/worktree-list-groups.test.ts b/src/renderer/src/components/sidebar/worktree-list-groups.test.ts index b967c4cd356..4b4012f8c7c 100644 --- a/src/renderer/src/components/sidebar/worktree-list-groups.test.ts +++ b/src/renderer/src/components/sidebar/worktree-list-groups.test.ts @@ -1933,6 +1933,73 @@ describe('project groups', () => { expect(rows[0]).toMatchObject({ label: 'Platform' }) }) + it('keeps sleep-filtered Project Group members as empty project headers', () => { + // Why: #8865 — Hide sleeping removes workspace cards; membership placeholders + // must still project the grouped project header so the group count stays honest. + const group: ProjectGroup = { + id: 'group-1', + name: 'Platform', + parentPath: '/platform', + parentGroupId: null, + createdFrom: 'folder-scan', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 + } + const sleepingRepo: Repo = { + ...repo, + id: 'repo-sleeping', + displayName: 'sleeping-project', + projectGroupId: group.id + } + const awakeRepo: Repo = { + ...repo, + id: 'repo-awake', + displayName: 'awake-project', + projectGroupId: group.id + } + const awakeWorktree: Worktree = { + ...worktree, + id: 'wt-awake', + repoId: awakeRepo.id, + path: '/tmp/awake' + } + + const rows = buildRows( + 'repo', + [awakeWorktree], + new Map([ + [sleepingRepo.id, sleepingRepo], + [awakeRepo.id, awakeRepo] + ]), + null, + new Set(), + undefined, + undefined, + undefined, + {}, + new Map([[awakeWorktree.id, awakeWorktree]]), + false, + undefined, + [group], + new Set([sleepingRepo.id]) + ) + + expect(rows[0]).toMatchObject({ + type: 'header', + key: 'project-group:group-1', + count: 2 + }) + // Why: empty/placeholder projects sort after projects with visible activity. + expect(rows.filter((row) => row.type === 'header').map((row) => row.key)).toEqual([ + 'project-group:group-1', + `repo:${awakeRepo.id}`, + `repo:${sleepingRepo.id}` + ]) + }) + it('renders ungrouped repos as top-level repo rows when Project Groups exist', () => { const group: ProjectGroup = { id: 'group-1',