diff --git a/src/renderer/src/components/right-sidebar/source-control/commit/discard-all-failure-envelope.test.tsx b/src/renderer/src/components/right-sidebar/source-control/commit/discard-all-failure-envelope.test.tsx new file mode 100644 index 00000000000..050a0cbf9ff --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/commit/discard-all-failure-envelope.test.tsx @@ -0,0 +1,107 @@ +// @vitest-environment happy-dom + +import { act, renderHook } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { DiscardAllArea } from './discard-all-sequence' +import type { SourceControlEntryGroups } from '../listing/section-order' +import { useSourceControlDiscardConfirmation } from './use-discard-confirmation' + +const toastError = vi.fn() +const bulkUnstage = vi.fn() + +vi.mock('sonner', () => ({ toast: { error: (...args: unknown[]) => toastError(...args) } })) +vi.mock('@/i18n/i18n', () => ({ + translate: (_key: string, fallback: string, vars?: Record) => + fallback.replace(/\{\{(\w+)\}\}/g, (_m, name: string) => String(vars?.[name] ?? '')) +})) +vi.mock('@/lib/connection-context', () => ({ getConnectionId: () => null })) +vi.mock('@/runtime/runtime-git-client', () => ({ + bulkUnstageRuntimeGitPaths: (...args: unknown[]) => bulkUnstage(...args) +})) + +// Electron rethrows a rejected `ipcMain.handle` as `Error invoking remote method '': `, +// and `window.api.git.bulkUnstage` / `git.discard` are exactly that. Both discard-all toasts read the +// first retained rejection, so the wrapper reached the description verbatim. +const ENVELOPED = + "Error invoking remote method 'git:bulkUnstage': Error: fatal: pathspec 'src/app.ts' did not match any file(s) known to git" +const REASON = "fatal: pathspec 'src/app.ts' did not match any file(s) known to git" +const ENVELOPE_ONLY = "Error invoking remote method 'git:bulkUnstage': Error" + +function renderDiscard(overrides: { + discardMany?: (paths: string[]) => Promise + discardSingle?: (path: string) => Promise +}) { + return renderHook(() => + useSourceControlDiscardConfirmation({ + activeRepoSettings: {} as never, + activeWorktreeId: 'repo::/w', + worktreePath: '/w', + grouped: {} as SourceControlEntryGroups, + isExecutingBulk: false, + setIsExecutingBulk: () => {}, + clearSelection: () => {}, + discardMany: overrides.discardMany ?? (async () => {}), + discardSingle: overrides.discardSingle ?? (async () => {}), + refreshActiveGitStatusAfterMutation: async () => {} + }) + ) +} + +// Why: the hook exposes the confirmation dialog, not the handler — this is the real user path. +async function confirmDiscardAll( + result: { current: ReturnType }, + area: DiscardAllArea, + paths: readonly string[] +): Promise { + await act(async () => { + result.current.requestDiscardPaths(area, paths) + }) + await act(async () => { + result.current.confirmPendingDiscard() + await Promise.resolve() + }) +} + +describe('the discard-all failure toast', () => { + beforeEach(() => { + toastError.mockReset() + bulkUnstage.mockReset() + }) + + it('shows the reason, not the IPC envelope, when the unstage pre-step rejects', async () => { + bulkUnstage.mockRejectedValue(new Error(ENVELOPED)) + const { result } = renderDiscard({}) + + await confirmDiscardAll(result, 'staged', ['src/app.ts']) + + const description = toastError.mock.calls[0][1].description + expect(description).toBe(REASON) + expect(description).not.toContain('Error invoking remote method') + }) + + it('drops the description when the envelope carried no reason', async () => { + bulkUnstage.mockRejectedValue(new Error(ENVELOPE_ONLY)) + const { result } = renderDiscard({}) + + await confirmDiscardAll(result, 'staged', ['src/app.ts']) + + expect(toastError.mock.calls[0][1].description).toBeUndefined() + }) + + it('shows the reason alongside the failed paths when a per-file discard rejects', async () => { + const { result } = renderDiscard({ + discardMany: async () => { + throw new Error('no bulk discard on this relay') + }, + discardSingle: async () => { + throw new Error(ENVELOPED) + } + }) + + await confirmDiscardAll(result, 'unstaged', ['src/app.ts']) + + const description = toastError.mock.calls[0][1].description + expect(description).toBe(`${REASON} (e.g. src/app.ts)`) + expect(description).not.toContain('Error invoking remote method') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/commit/use-discard-confirmation.ts b/src/renderer/src/components/right-sidebar/source-control/commit/use-discard-confirmation.ts index ab0d5f045a6..c067fbf856d 100644 --- a/src/renderer/src/components/right-sidebar/source-control/commit/use-discard-confirmation.ts +++ b/src/renderer/src/components/right-sidebar/source-control/commit/use-discard-confirmation.ts @@ -1,6 +1,7 @@ import { useCallback, useState } from 'react' import { toast } from 'sonner' import { getConnectionId } from '@/lib/connection-context' +import { stripIpcInvokeEnvelope } from '@/lib/ipc-error' import { bulkUnstageRuntimeGitPaths, type RuntimeGitContext } from '@/runtime/runtime-git-client' import { translate } from '@/i18n/i18n' import type { GitStatusEntry } from '../../../../../../shared/git-status-types' @@ -90,17 +91,22 @@ export function useSourceControlDiscardConfirmation({ console.error('[SourceControl] discard-all failure', error) } }) + // Why: onError retains every rejection in full above; the title already names the operation, + // so an envelope with nothing behind it drops the description rather than printing plumbing. + const firstMsg = + errors[0] instanceof Error + ? (stripIpcInvokeEnvelope(errors[0].message) ?? undefined) + : undefined if (result.aborted) { toast.error( translate( 'auto.components.right.sidebar.SourceControl.a5e5a11090', 'Discard all failed — unable to unstage files before discard' ), - { description: errors[0] instanceof Error ? errors[0].message : undefined } + { description: firstMsg } ) } else if (result.failed.length > 0) { // Why: show only the first error + a sample of failed paths to avoid a huge toast body on bulk failures. - const firstMsg = errors[0] instanceof Error ? errors[0].message : undefined const sample = result.failed.slice(0, 3).join(', ') const more = result.failed.length > 3 ? `, +${result.failed.length - 3} more` : '' toast.error( diff --git a/src/renderer/src/components/task-page/hooks/gitlab-list-error-envelope.test.tsx b/src/renderer/src/components/task-page/hooks/gitlab-list-error-envelope.test.tsx new file mode 100644 index 00000000000..7579daacdee --- /dev/null +++ b/src/renderer/src/components/task-page/hooks/gitlab-list-error-envelope.test.tsx @@ -0,0 +1,93 @@ +// @vitest-environment happy-dom + +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { renderHook, waitFor } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { useTaskPageGitLabFetch } from './use-task-page-gitlab-fetch' +import type { Repo } from '../../../../../shared/repo-types' + +vi.mock('@/i18n/i18n', () => ({ + translate: (_key: string, fallback: string) => fallback +})) +vi.mock('@/components/task-page/source/repo-source-context', () => ({ + getTaskPageRepoSourceContext: () => null +})) + +// `gitlab:listIssues` throws before it returns a payload — `assertRegisteredRepo` rejects with +// "Access denied: unknown repository path" — so the renderer sees Electron's invoke envelope, and +// this banner renders errs[0] verbatim. +const ENVELOPED = + "Error invoking remote method 'gitlab:listIssues': Error: Access denied: unknown repository path" +const REASON = 'Access denied: unknown repository path' +const ENVELOPE_ONLY = "Error invoking remote method 'gitlab:listIssues': Error" +const UNREADABLE_COPY = + 'Could not load GitLab items, and the failure did not include a readable reason.' + +const repo = { id: 'repo-1', path: '/w/project' } as Repo + +function renderFetch(rejection: unknown) { + const listIssues = vi.fn().mockRejectedValue(rejection) + ;(globalThis as unknown as { window: Window }).window.api = { + gl: { listIssues, listMRs: vi.fn() } + } as never + + const setGitlabError = vi.fn() + renderHook(() => + useTaskPageGitLabFetch({ + taskSource: 'gitlab', + gitlabView: 'issues', + activeGitlabFilter: 'assigned-to-me', + gitlabRefreshNonce: 0, + selectedRepos: [repo], + selectedReposKey: 'repo-1', + primaryRepo: repo, + gitlabIssuePage: 0, + setGitlabItems: vi.fn(), + setGitlabLoading: vi.fn(), + setGitlabError, + setGitlabIssuePage: vi.fn(), + setGitlabIssueTotalPages: vi.fn(), + setGitlabIssueLoadingTargetPage: vi.fn(), + setGitlabTodos: vi.fn(), + setGitlabTodosLoading: vi.fn() + }) + ) + return setGitlabError +} + +// Why: the effect clears the banner with setGitlabError(null) before fetching, so the asserted +// value is the last call, not the first. +async function bannerText(setGitlabError: ReturnType): Promise { + await waitFor(() => expect(setGitlabError.mock.calls.length).toBeGreaterThan(1)) + return setGitlabError.mock.calls.at(-1)?.[0] +} + +describe('the GitLab task-list error banner', () => { + beforeEach(() => { + vi.restoreAllMocks() + }) + + // Why: `translate` is mocked to its fallback above, and the real one prefers the catalog whenever + // the key resolves — so without this the asserted copy is a string no user ever sees. + it('reads that copy from the catalog, not just the inline fallback', () => { + const catalog = JSON.parse( + readFileSync(join(__dirname, '..', '..', '..', 'i18n', 'locales', 'en.json'), 'utf8') + ) + + expect(catalog.auto.components.TaskPage.unreadableGitlabListError).toBe(UNREADABLE_COPY) + }) + + it('shows the reason, not the IPC envelope, when a list handler rejects', async () => { + const banner = await bannerText(renderFetch(new Error(ENVELOPED))) + + expect(banner).toBe(REASON) + expect(banner).not.toContain('Error invoking remote method') + }) + + it('shows readable copy when the envelope carried no reason', async () => { + const banner = await bannerText(renderFetch(new Error(ENVELOPE_ONLY))) + + expect(banner).toBe(UNREADABLE_COPY) + }) +}) diff --git a/src/renderer/src/components/task-page/hooks/use-task-page-gitlab-fetch.ts b/src/renderer/src/components/task-page/hooks/use-task-page-gitlab-fetch.ts index d92537ea2bf..c7d41f191b1 100644 --- a/src/renderer/src/components/task-page/hooks/use-task-page-gitlab-fetch.ts +++ b/src/renderer/src/components/task-page/hooks/use-task-page-gitlab-fetch.ts @@ -7,6 +7,8 @@ import { import { resolveGitLabIssuePageState } from '@/components/task-page/gitlab/gitlab-issue-pages' import { getTaskPageRepoSourceContext } from '@/components/task-page/source/repo-source-context' import { withGitLabIpcTimeout } from '@/runtime/gitlab-ipc-timeout' +import { extractIpcErrorMessage } from '@/lib/ipc-error' +import { translate } from '@/i18n/i18n' import type { GitLabIssueFilter, GitLabTaskFilter } from '@/components/task-page-localized-options' import type { GitLabTodo, GitLabWorkItem } from '../../../../../shared/gitlab-types' import type { Repo } from '../../../../../shared/repo-types' @@ -136,7 +138,19 @@ export function useTaskPageGitLabFetch({ const settled: { items: readonly GitLabWorkItem[]; totalPages?: number }[] = [] for (const r of results) { if (r.status !== 'fulfilled') { - errs.push(r.reason instanceof Error ? r.reason.message : String(r.reason)) + // Why: a rejected `gl:` handler arrives wrapped in Electron's invoke envelope, and this + // string is rendered verbatim in the list banner. Kept in errs either way — errs.length + // is the pager's errorCount, so dropping an unreadable one would retreat the page. + console.error('[TaskPage] GitLab list request rejected', r.reason) + errs.push( + extractIpcErrorMessage( + r.reason, + translate( + 'auto.components.TaskPage.unreadableGitlabListError', + 'Could not load GitLab items, and the failure did not include a readable reason.' + ) + ) + ) continue } settled.push(r.value) diff --git a/src/renderer/src/components/terminal-pane/terminal-error-surface-census.test.ts b/src/renderer/src/components/terminal-pane/terminal-error-surface-census.test.ts index bb0a6d617fb..52e69a7dcf2 100644 --- a/src/renderer/src/components/terminal-pane/terminal-error-surface-census.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-error-surface-census.test.ts @@ -16,10 +16,15 @@ import { describe, expect, it } from 'vitest' * what catches the next one: `formatClipboardImagePasteError` was a real envelope leak into this * same surface that bypassed the accumulator entirely, and no stripper-keyed census could see it. * - * What this cannot see, stated rather than implied: it is scoped to one surface. The same shape - * exists elsewhere (`use-task-page-gitlab-fetch.ts` buffers raw rejection messages into `errs` and - * renders `errs[0]`; `use-discard-confirmation.ts` renders `errors[0].message`), and nothing here - * looks at those. It also reads source text, so it sees the call, not the value. + * What this cannot see, stated rather than implied: it is scoped to one surface, and it reads source + * text, so it sees the call, not the value. The two siblings it used to name — `use-task-page-gitlab-fetch.ts` + * and `use-discard-confirmation.ts` — are fixed and now enumerated in `lib/ipc-error-call-site-census.test.ts`. + * + * That does not make the population closed, and the note there says why: enumerating by render sink + * rather than by stripper finds 118 renderer modules that still hand a raw rejection to a user + * surface with an IPC-capable producer behind it. Per-surface censuses like this one do not scale to + * that, and a lint rule on the idiom would need suppressions. The proposal on the table is to strip + * once at the preload boundary instead. */ const SURFACE_DOORS: Readonly> = { 'setTerminalError(null)': 'clears the surface', diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index a42467da811..b567e807db1 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -2211,7 +2211,8 @@ "linearModeHasWorktreeTooltip": "Linear tickets linked to an Orca workspace", "linearEmptyHasWorktree": "No Linear tickets are linked to an Orca workspace yet. Start work from a Linear issue to see it here.", "linearOpenAttachedWorkspace": "Open workspace attached to {{value0}}", - "linearWorktreesColumn": "Workspaces" + "linearWorktreesColumn": "Workspaces", + "unreadableGitlabListError": "Could not load GitLab items, and the failure did not include a readable reason." }, "Terminal": { "73768427cf": "Close", diff --git a/src/renderer/src/lib/ipc-error-call-site-census.test.ts b/src/renderer/src/lib/ipc-error-call-site-census.test.ts index a329c25e76b..bf5f2dc5ac6 100644 --- a/src/renderer/src/lib/ipc-error-call-site-census.test.ts +++ b/src/renderer/src/lib/ipc-error-call-site-census.test.ts @@ -4,7 +4,7 @@ import { describe, expect, it } from 'vitest' /** * Why an enumeration and not a count: the previous note on this reader put its population at 18 - * and said the sites depended on its fail-open behaviour. The real number is 30, and every one + * and said the sites depended on its fail-open behaviour. The real number is 31, and every one * of them renders the result to a user, so "these can keep the looser regex" was never a choice * anybody had made. A count is easy to be wrong about; this produces the list. * @@ -65,6 +65,10 @@ const CALL_SITES: Readonly> = surface: 'dialog error' }, 'src/renderer/src/components/sidebar/useCreateRepo.ts': { calls: 1, surface: 'dialog error' }, + 'src/renderer/src/components/task-page/hooks/use-task-page-gitlab-fetch.ts': { + calls: 1, + surface: 'task list banner, via the errs buffer whose first entry the banner renders' + }, 'src/renderer/src/components/terminal-pane/ipc-pty-connect.ts': { calls: 1, surface: 'terminal error+classifier' @@ -116,7 +120,7 @@ const ENVELOPE_MODULE = 'src/shared/ipc-invoke-envelope.ts' * * A caller that must not fall back to the wrapper text branches on null instead, so it reaches for * the stripper directly and never appears in CALL_SITES above. Counting only `extractIpcErrorMessage` - * reports 30 and reads as a total; it is not one, and the last person to freeze a number here was + * reports 31 and reads as a total; it is not one, and the last person to freeze a number here was * wrong for exactly this reason. Both lists together are the set of modules that open the envelope. */ const DIRECT_STRIPPER_SITES: Readonly> = { @@ -125,6 +129,11 @@ const DIRECT_STRIPPER_SITES: Readonly { * What neither list can see, stated rather than implied: both are keyed on the stripper, so they * enumerate sites that already handle the envelope and can never enumerate one that does not. A * leak is invisible here by construction — the terminal error toast was fed a raw envelope for as - * long as this file has existed. Surfaces that buffer error text before display need a census - * keyed on the surface; `terminal-pane/terminal-error-surface-census.test.ts` is one, for one - * surface. Others (`use-task-page-gitlab-fetch.ts`, `use-discard-confirmation.ts`) have none. + * long as this file has existed. + * + * So the honest scope of this file: it is a change-detector for the sites that already strip, not + * evidence that the leaking population is empty. It is not. Enumerating by surface instead — every + * renderer expression that reads free text off a rejection and passes it to a render sink (a + * `toast.*` argument or a `set*` state setter whose value is rendered) — finds 239 such + * expressions across 162 modules, of which 118 reach a producer that can cross `ipcRenderer.invoke` + * and do not strip. Four sampled at random were all real leaks of this exact shape, including + * `source-control/commit/use-bulk-actions.ts`, which sits beside the file this PR just fixed, and + * `settings/SshPane.tsx`, which renders the caught message as the whole toast title. + * + * No per-call-site gate can close that, because the leaking shape *is* the ordinary idiom: the + * discriminator is whether the value crossed IPC, which is not visible where it is rendered. A + * lint rule keyed on the idiom would fire on hundreds of correct sites and need suppressions. + * + * The narrow fix is the boundary, not the call site. Every envelope in the app is created in one + * place — 730 `ipcRenderer.invoke(` calls live in exactly two files under `src/preload`. A preload + * `invoke` wrapper that rejects with the stripped reason would fix all 118 at once and make this + * census unnecessary, enforced by a ratchet test banning bare `ipcRenderer.invoke` outside it — + * the same shape as the existing `child_process` ratchet. That is a separate change; the cost to + * weigh first is that `TerminalPane.tsx` deliberately keeps the wrapped form in the console, so + * the boundary must strip for display while the log keeps the original. */ // Why: this is the number the freeze note got wrong, so it is asserted rather than described. - it('number 30, and all of them render to a user', () => { + it('number 31, and all of them render to a user', () => { const total = Object.values(CALL_SITES).reduce((sum, { calls }) => sum + calls, 0) - expect(total).toBe(30) + expect(total).toBe(31) expect(Object.values(CALL_SITES).filter(({ surface }) => surface === '')).toEqual([]) expect(Object.values(DIRECT_STRIPPER_SITES).filter(({ surface }) => surface === '')).toEqual([]) })