From 3db3b929926bd5bfc903fd15cea9a0e5cf862a3c Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Tue, 12 May 2026 12:52:29 +0530 Subject: [PATCH] fix(webview-accounts): typed error wrap for openWebviewAccount (#1472) (#1521) Co-authored-by: Claude Opus 4.7 (1M context) --- app/src/components/accounts/WebviewHost.tsx | 12 +- .../webviewAccountService.openErrors.test.ts | 204 ++++++++++++++++++ app/src/services/webviewAccountService.ts | 94 +++++++- 3 files changed, 304 insertions(+), 6 deletions(-) create mode 100644 app/src/services/__tests__/webviewAccountService.openErrors.test.ts diff --git a/app/src/components/accounts/WebviewHost.tsx b/app/src/components/accounts/WebviewHost.tsx index 33257bff1..bb0cadf55 100644 --- a/app/src/components/accounts/WebviewHost.tsx +++ b/app/src/components/accounts/WebviewHost.tsx @@ -153,7 +153,11 @@ const WebviewHost = ({ accountId, provider }: WebviewHostProps) => { if (!openedRef.current) { openedRef.current = true; log('opening account=%s at %o', accountId, bounds); - void openWebviewAccount({ accountId, provider, bounds }); + openWebviewAccount({ accountId, provider, bounds }).catch(() => { + // Service-layer dispatched `setAccountStatus({ status: 'error', lastError })` + // and emitted a Sentry breadcrumb already; swallowing here prevents the + // rejection from reaching `onunhandledrejection` (OPENHUMAN-REACT-K). + }); } else { void setWebviewAccountBounds(accountId, bounds); } @@ -247,7 +251,11 @@ const WebviewHost = ({ accountId, provider }: WebviewHostProps) => { type="button" onClick={() => { log('retry clicked account=%s provider=%s', accountId, provider); - void retryWebviewAccountLoad(accountId, provider); + retryWebviewAccountLoad(accountId, provider).catch(() => { + // Same contract as the initial open (OPENHUMAN-REACT-K): + // service-layer dispatched error status + breadcrumb; absorbing + // the rejection keeps onunhandledrejection clean. + }); }} className="rounded-md bg-primary-600 px-3 py-1.5 text-xs font-semibold text-white transition-colors hover:bg-primary-700"> Retry loading diff --git a/app/src/services/__tests__/webviewAccountService.openErrors.test.ts b/app/src/services/__tests__/webviewAccountService.openErrors.test.ts new file mode 100644 index 000000000..cceaac255 --- /dev/null +++ b/app/src/services/__tests__/webviewAccountService.openErrors.test.ts @@ -0,0 +1,204 @@ +import * as Sentry from '@sentry/react'; +import { invoke } from '@tauri-apps/api/core'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { store } from '../../store'; +import { addAccount, resetAccountsState } from '../../store/accountsSlice'; +import type { AccountProvider } from '../../types/accounts'; +import { + classifyWebviewAccountError, + openWebviewAccount, + retryWebviewAccountLoad, + setWebviewAccountBounds, + WebviewAccountError, +} from '../webviewAccountService'; + +vi.mock('@tauri-apps/api/core', () => ({ + invoke: vi.fn(), + isTauri: vi.fn().mockReturnValue(true), +})); + +vi.mock('@tauri-apps/api/event', () => ({ listen: vi.fn().mockResolvedValue(() => undefined) })); + +vi.mock('@sentry/react', () => ({ addBreadcrumb: vi.fn() })); + +// Heavy unrelated deps stubbed for the same reason as the listener test. +vi.mock('../api/threadApi', () => ({ threadApi: { createNewThread: vi.fn() } })); +vi.mock('../chatService', () => ({ chatSend: vi.fn() })); +vi.mock('../coreRpcClient', () => ({ callCoreRpc: vi.fn() })); +vi.mock('../notificationService', () => ({ ingestNotification: vi.fn() })); + +const ACCOUNT_ID = 'acct-err-1'; +const BOUNDS = { x: 0, y: 0, width: 800, height: 600 }; + +/** + * Configure the Tauri `invoke` mock so that calls to `webview_account_open` + * reject with `rejection`, while every other invoke (notification permission + * probes, reveal, hide, bounds) resolves successfully. Without this the + * `mockRejectedValueOnce` is consumed by `ensureNotificationPermission`'s + * preflight invoke before openWebviewAccount ever calls the Rust opener. + */ +function rejectOpenWith(rejection: unknown): void { + vi.mocked(invoke).mockImplementation(async (cmd: string) => { + if (cmd === 'webview_account_open') { + return Promise.reject(rejection); + } + return undefined; + }); +} + +function seedAccount(): void { + store.dispatch(resetAccountsState()); + store.dispatch( + addAccount({ + id: ACCOUNT_ID, + provider: 'telegram', + label: 'Test', + createdAt: new Date().toISOString(), + status: 'closed', + }) + ); +} + +describe('classifyWebviewAccountError', () => { + it.each([ + ['unknown provider: gmail', { kind: 'unknown_provider' as const, providerName: 'gmail' }], + [ + 'unknown provider: my-custom_provider.v2', + { kind: 'unknown_provider' as const, providerName: 'my-custom_provider.v2' }, + ], + ['no url for provider: foo', { kind: 'no_url' as const, providerName: 'foo' }], + [ + 'invalid provider url https://x: relative URL without a base', + { kind: 'invalid_url' as const }, + ], + ['something unrelated', { kind: 'unknown' as const }], + ])('classifies %j', (message, expected) => { + expect(classifyWebviewAccountError(message)).toEqual(expected); + }); +}); + +describe('openWebviewAccount error handling', () => { + beforeEach(() => { + vi.clearAllMocks(); + // `clearAllMocks` resets call history but NOT `mockImplementation`, so a + // previous test's `rejectOpenWith(...)` would still apply here. Reset the + // invoke implementation back to a benign default before every test. + vi.mocked(invoke).mockReset(); + vi.mocked(invoke).mockResolvedValue(undefined); + seedAccount(); + }); + + it('wraps a raw string rejection from Tauri invoke into a typed WebviewAccountError', async () => { + rejectOpenWith('unknown provider: gmail'); + + await expect( + openWebviewAccount({ accountId: ACCOUNT_ID, provider: 'telegram', bounds: BOUNDS }) + ).rejects.toBeInstanceOf(WebviewAccountError); + + // The rejection is no longer the bare string that Sentry's + // `onunhandledrejection` handler captures as "Non-Error promise rejection". + expect(store.getState().accounts.accounts[ACCOUNT_ID]?.status).toBe('error'); + // `lastError` must NOT carry the raw rejection text — that string can + // include a user-supplied provider literal (debug-mode custom URL) so the + // store keeps a fixed per-kind summary instead. Original message stays + // attached to the thrown WebviewAccountError for internal control flow. + expect(store.getState().accounts.accounts[ACCOUNT_ID]?.lastError).toBe( + 'Provider not supported' + ); + expect(store.getState().accounts.accounts[ACCOUNT_ID]?.lastError).not.toContain( + 'unknown provider:' + ); + expect(store.getState().accounts.accounts[ACCOUNT_ID]?.lastError).not.toContain('gmail'); + }); + + it('exposes kind + providerName on the wrapped error', async () => { + rejectOpenWith('unknown provider: slack'); + + try { + await openWebviewAccount({ accountId: ACCOUNT_ID, provider: 'slack', bounds: BOUNDS }); + throw new Error('should have rejected'); + } catch (err) { + expect(err).toBeInstanceOf(WebviewAccountError); + const wae = err as WebviewAccountError; + expect(wae.kind).toBe('unknown_provider'); + expect(wae.providerName).toBe('slack'); + } + }); + + it('emits a Sentry breadcrumb with classifier output and no PII', async () => { + rejectOpenWith('unknown provider: discord'); + + await expect( + openWebviewAccount({ accountId: ACCOUNT_ID, provider: 'discord', bounds: BOUNDS }) + ).rejects.toBeInstanceOf(WebviewAccountError); + + expect(Sentry.addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'webview-account', + level: 'warning', + message: 'webview_account_open rejected', + data: expect.objectContaining({ kind: 'unknown_provider', provider: 'discord' }), + }) + ); + + // Breadcrumb must NOT carry the account id or the raw rejection text — + // both could leak workspace identifiers / user state. + const call = vi.mocked(Sentry.addBreadcrumb).mock.calls[0]?.[0]; + expect(JSON.stringify(call)).not.toContain(ACCOUNT_ID); + expect(JSON.stringify(call)).not.toContain('unknown provider:'); + }); + + it('classifies an unknown error string as kind="unknown" with error-level breadcrumb', async () => { + rejectOpenWith('whatever the rust shell threw'); + + await expect( + openWebviewAccount({ accountId: ACCOUNT_ID, provider: 'telegram', bounds: BOUNDS }) + ).rejects.toMatchObject({ kind: 'unknown', providerName: undefined }); + + expect(Sentry.addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + level: 'error', + data: expect.objectContaining({ kind: 'unknown' }), + }) + ); + }); +}); + +describe('retryWebviewAccountLoad error handling', () => { + beforeEach(() => { + vi.clearAllMocks(); + // `clearAllMocks` resets call history but NOT `mockImplementation`, so a + // previous test's `rejectOpenWith(...)` would still apply here. Reset the + // invoke implementation back to a benign default before every test. + vi.mocked(invoke).mockReset(); + vi.mocked(invoke).mockResolvedValue(undefined); + seedAccount(); + }); + + it('propagates a typed WebviewAccountError when invoke rejects', async () => { + // Seed cached bounds via a successful open first (default mock resolves). + await openWebviewAccount({ accountId: ACCOUNT_ID, provider: 'telegram', bounds: BOUNDS }); + + // Now make the retry-time `webview_account_open` invoke fail. + rejectOpenWith('unknown provider: gmail'); + + await expect( + retryWebviewAccountLoad(ACCOUNT_ID, 'gmail' as unknown as AccountProvider) + ).rejects.toBeInstanceOf(WebviewAccountError); + }); + + it('no-ops silently when bounds were never cached', async () => { + await expect( + retryWebviewAccountLoad('never-opened', 'gmail' as unknown as AccountProvider) + ).resolves.toBeUndefined(); + expect(invoke).not.toHaveBeenCalled(); + }); + + // Defensive guard against the regression the parent ticket caught: a bare + // `setWebviewAccountBounds(...)` invocation on an unknown account must not + // surface as an unhandled rejection. + it('setWebviewAccountBounds on a stale account does not throw', async () => { + await expect(setWebviewAccountBounds('stale-id', BOUNDS)).resolves.toBeUndefined(); + }); +}); diff --git a/app/src/services/webviewAccountService.ts b/app/src/services/webviewAccountService.ts index af0660097..a326b9ccd 100644 --- a/app/src/services/webviewAccountService.ts +++ b/app/src/services/webviewAccountService.ts @@ -1,3 +1,4 @@ +import * as Sentry from '@sentry/react'; import { invoke, isTauri } from '@tauri-apps/api/core'; import { listen, type UnlistenFn } from '@tauri-apps/api/event'; import debug from 'debug'; @@ -25,6 +26,82 @@ const errLog = debug('webview-accounts:error'); export { isTauri }; +/** + * Stable classification of a `webview_account_*` Tauri IPC failure. The Rust + * shell rejects with raw `String` values (e.g. `"unknown provider: gmail"`, + * `"no url for provider: foo"`, `"invalid provider url ..."`); without a typed + * wrapper the rejection bubbles as a bare string up to `onunhandledrejection`, + * which Sentry captures as `Non-Error promise rejection` with no stack trace. + * Callers should branch on `kind` instead of re-parsing the message. + */ +export type WebviewAccountErrorKind = 'unknown_provider' | 'invalid_url' | 'no_url' | 'unknown'; + +export class WebviewAccountError extends Error { + readonly kind: WebviewAccountErrorKind; + readonly providerName?: string; + constructor(message: string, kind: WebviewAccountErrorKind, providerName?: string) { + super(message); + this.name = 'WebviewAccountError'; + this.kind = kind; + this.providerName = providerName; + } +} + +/** + * Classify a `webview_account_*` rejection by its surfaced string. Patterns + * map to the Rust-side `format!` sites in + * `app/src-tauri/src/webview_accounts/mod.rs` — keep in sync when those + * error strings change. + */ +export function classifyWebviewAccountError(message: string): { + kind: WebviewAccountErrorKind; + providerName?: string; +} { + const unknownProvider = /^unknown provider:\s*([\w.-]+)/i.exec(message); + if (unknownProvider) { + return { kind: 'unknown_provider', providerName: unknownProvider[1] }; + } + const noUrl = /^no url for provider:\s*([\w.-]+)/i.exec(message); + if (noUrl) { + return { kind: 'no_url', providerName: noUrl[1] }; + } + if (/^invalid provider url\b/i.test(message)) { + return { kind: 'invalid_url' }; + } + return { kind: 'unknown' }; +} + +function toWebviewAccountError(err: unknown): WebviewAccountError { + if (err instanceof WebviewAccountError) return err; + const message = err instanceof Error ? err.message : String(err); + const { kind, providerName } = classifyWebviewAccountError(message); + return new WebviewAccountError(message, kind, providerName); +} + +/** + * Map a `WebviewAccountErrorKind` to a fixed, user-safe summary string used + * for `errLog` output and `setAccountStatus({ lastError })`. The raw Rust + * rejection text can still carry the originally requested provider literal — + * which a custom-URL debug override could route to anything — so anything + * surfaced into Redux (read by the retry overlay UI) or written to the + * `debug('webview-accounts:error')` channel must come from this table, not + * `wrapped.message`. The original message is preserved on the thrown + * `WebviewAccountError` for callers that need internal control flow. + */ +function summaryForKind(kind: WebviewAccountErrorKind): string { + switch (kind) { + case 'unknown_provider': + return 'Provider not supported'; + case 'no_url': + return 'Missing URL for provider'; + case 'invalid_url': + return 'Invalid provider URL'; + case 'unknown': + default: + return 'Failed to open account'; + } +} + interface RecipeEventPayload { account_id: string; provider: string; @@ -993,13 +1070,22 @@ export async function openWebviewAccount(args: OpenAccountArgs): Promise { store.dispatch(setAccountStatus({ accountId: args.accountId, status: 'loading' })); void setFocusedAccount(args.accountId); } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - errLog('open failed: %s', msg); + const wrapped = toWebviewAccountError(err); + const summary = summaryForKind(wrapped.kind); + // Redact: never log or persist `wrapped.message` — the Rust shell can + // include user-supplied provider/url overrides in the rejection text. + errLog('open failed: kind=%s provider=%s', wrapped.kind, wrapped.providerName ?? args.provider); loadingAccounts.delete(args.accountId); store.dispatch( - setAccountStatus({ accountId: args.accountId, status: 'error', lastError: msg }) + setAccountStatus({ accountId: args.accountId, status: 'error', lastError: summary }) ); - throw err; + Sentry.addBreadcrumb({ + category: 'webview-account', + level: wrapped.kind === 'unknown' ? 'error' : 'warning', + message: 'webview_account_open rejected', + data: { kind: wrapped.kind, provider: wrapped.providerName ?? args.provider }, + }); + throw wrapped; } }