From ef998ca16c0e22034da186e5c24fc0ce5e896919 Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Tue, 12 May 2026 00:37:42 +0530 Subject: [PATCH] fix(openUrl): fall back to window.open when CEF IPC handle not ready (#1472) (#1491) --- app/src/components/settings/SettingsHome.tsx | 2 +- .../settings/__tests__/SettingsHome.test.tsx | 2 +- app/src/utils/openUrl.test.ts | 75 ++++++++++++++++--- app/src/utils/openUrl.ts | 54 +++++++++++-- 4 files changed, 115 insertions(+), 18 deletions(-) diff --git a/app/src/components/settings/SettingsHome.tsx b/app/src/components/settings/SettingsHome.tsx index 51635f0b4..12b9c9bd9 100644 --- a/app/src/components/settings/SettingsHome.tsx +++ b/app/src/components/settings/SettingsHome.tsx @@ -209,7 +209,7 @@ const SettingsHome = () => { ), onClick: () => { - void openUrl(BILLING_DASHBOARD_URL); + openUrl(BILLING_DASHBOARD_URL).catch(() => {}); }, }, { diff --git a/app/src/components/settings/__tests__/SettingsHome.test.tsx b/app/src/components/settings/__tests__/SettingsHome.test.tsx index 560e57203..2add00ed5 100644 --- a/app/src/components/settings/__tests__/SettingsHome.test.tsx +++ b/app/src/components/settings/__tests__/SettingsHome.test.tsx @@ -32,7 +32,7 @@ vi.mock('../../../store', () => ({ persistor: { purge: vi.fn().mockResolvedValue vi.mock('../../../utils/links', () => ({ BILLING_DASHBOARD_URL: 'https://billing.example.com' })); -vi.mock('../../../utils/openUrl', () => ({ openUrl: vi.fn() })); +vi.mock('../../../utils/openUrl', () => ({ openUrl: vi.fn().mockResolvedValue(undefined) })); vi.mock('../../../utils/tauriCommands', () => ({ resetOpenHumanDataAndRestartCore: vi.fn().mockResolvedValue(undefined), diff --git a/app/src/utils/openUrl.test.ts b/app/src/utils/openUrl.test.ts index 24927eb54..0cd927eb5 100644 --- a/app/src/utils/openUrl.test.ts +++ b/app/src/utils/openUrl.test.ts @@ -1,17 +1,24 @@ /** * Unit tests for `openUrl`. The Tauri path is exercised in callers' - * integration tests; here we focus on the browser fallback so the - * non-Tauri branch (used by dev preview builds) doesn't regress. + * integration tests; here we focus on the browser fallback and the + * CEF-IPC-not-ready recovery so the non-Tauri branch (used by dev + * preview builds) and the CEF gap window (#1472 / REACT-T/S/R) do + * not regress. */ import { afterEach, beforeEach, describe, expect, it, type Mock, vi } from 'vitest'; const isTauriMock = vi.fn(); const tauriOpenUrlMock = vi.fn(); +const addBreadcrumbMock = vi.fn(); vi.mock('@tauri-apps/api/core', () => ({ isTauri: () => isTauriMock() })); vi.mock('@tauri-apps/plugin-opener', () => ({ openUrl: (url: string) => tauriOpenUrlMock(url) })); +vi.mock('@sentry/react', () => ({ + addBreadcrumb: (...args: unknown[]) => addBreadcrumbMock(...args), +})); + describe('openUrl', () => { let originalWindowOpen: typeof window.open; let windowOpenMock: Mock; @@ -35,9 +42,9 @@ describe('openUrl', () => { await openUrl('https://example.com/page'); expect(tauriOpenUrlMock).toHaveBeenCalledWith('https://example.com/page'); - // Browser fallback must NOT fire under Tauri — it would spawn a - // new webview window with no useful behaviour for custom schemes. + // Browser fallback must NOT fire when the Tauri call succeeded. expect(windowOpenMock).not.toHaveBeenCalled(); + expect(addBreadcrumbMock).not.toHaveBeenCalled(); }); it('falls back to window.open in a browser context (non-Tauri)', async () => { @@ -52,17 +59,67 @@ describe('openUrl', () => { 'noopener,noreferrer' ); expect(tauriOpenUrlMock).not.toHaveBeenCalled(); + expect(addBreadcrumbMock).not.toHaveBeenCalled(); }); - it('propagates Tauri opener errors to the caller (no silent fallback)', async () => { - // Regression guard: the previous implementation swallowed the - // error and called window.open, which spawned a useless webview - // window for unhandled custom schemes (`obsidian://...`). + it('propagates Tauri opener errors for non-http schemes (no silent fallback)', async () => { + // Regression guard: `window.open` cannot launch custom-scheme + // URLs (`obsidian://`, `mailto:`, …) — it spawns a useless Tauri + // webview window. For those we MUST propagate the error to the + // caller, even when the failure is the CEF IPC race. isTauriMock.mockReturnValue(true); tauriOpenUrlMock.mockRejectedValue(new Error('scheme not allowed')); const { openUrl } = await import('./openUrl'); - await expect(openUrl('obsidian://open?path=/x')).rejects.toThrow('scheme not allowed'); + await expect(openUrl('obsidian://open?path=/Users/me/Vault')).rejects.toThrow( + 'scheme not allowed' + ); expect(windowOpenMock).not.toHaveBeenCalled(); + // Non-http schemes log only the protocol — the rest of the URL (here the + // vault path) is the payload itself and must not leak to Sentry. + expect(addBreadcrumbMock).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'ipc', + level: 'warning', + message: 'tauriOpenUrl failed; evaluating fallback', + data: expect.objectContaining({ url: 'obsidian:' }), + }) + ); + const call = addBreadcrumbMock.mock.calls[0]?.[0] as { data?: { url?: string } } | undefined; + expect(call?.data?.url).not.toContain('Vault'); + expect(call?.data?.url).not.toContain('/Users/me'); + }); + + it('falls back to window.open when tauriOpenUrl rejects on an http URL (CEF IPC race recovery, #1472)', async () => { + // Concrete repro for OPENHUMAN-REACT-T/S/R: CEF embedder + // injects `window.ipc.postMessage` after `on_after_created`. A + // click landing in that gap causes `tauriOpenUrl` to reject with + // a TypeError. For http(s) URLs the safe recovery is to hand off + // to `window.open` so the Billing dashboard still opens. + isTauriMock.mockReturnValue(true); + const ipcError = new TypeError("Cannot read properties of undefined (reading 'postMessage')"); + tauriOpenUrlMock.mockRejectedValue(ipcError); + + const { openUrl } = await import('./openUrl'); + await openUrl('https://tinyhumans.ai/dashboard?token=secret-redact-me'); + + expect(windowOpenMock).toHaveBeenCalledWith( + 'https://tinyhumans.ai/dashboard?token=secret-redact-me', + '_blank', + 'noopener,noreferrer' + ); + // Breadcrumb keeps only origin for http(s) — pathname + query (which may + // carry tokens / emails / vault paths) must not be sent to Sentry. + expect(addBreadcrumbMock).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'ipc', + level: 'warning', + message: 'tauriOpenUrl failed; evaluating fallback', + data: expect.objectContaining({ url: 'https://tinyhumans.ai' }), + }) + ); + const call = addBreadcrumbMock.mock.calls[0]?.[0] as { data?: { url?: string } } | undefined; + expect(call?.data?.url).not.toContain('secret-redact-me'); + expect(call?.data?.url).not.toContain('/dashboard'); }); }); diff --git a/app/src/utils/openUrl.ts b/app/src/utils/openUrl.ts index 7dd65f5da..286d97c8f 100644 --- a/app/src/utils/openUrl.ts +++ b/app/src/utils/openUrl.ts @@ -1,6 +1,29 @@ +import * as Sentry from '@sentry/react'; import { isTauri } from '@tauri-apps/api/core'; import { openUrl as tauriOpenUrl } from '@tauri-apps/plugin-opener'; +const isHttpUrl = (url: string): boolean => /^https?:\/\//i.test(url); + +/** + * Returns a low-PII representation of `url` for telemetry breadcrumbs. + * For http(s) we keep only the origin so the host is identifiable but the + * pathname/query/fragment (which may carry tokens, emails, or local paths) + * never leave the device. For other schemes (`mailto:`, `obsidian://`, …) + * we keep only the protocol — the rest of the URL is the payload itself + * (the email address, the vault path) and must not be logged. + */ +const getTelemetryUrl = (url: string): string => { + try { + const parsed = new URL(url); + if (parsed.protocol === 'http:' || parsed.protocol === 'https:') { + return parsed.origin; + } + return parsed.protocol; + } catch { + return 'invalid-url'; + } +}; + /** * Opens a URL using the host OS's default handler. * @@ -10,19 +33,36 @@ import { openUrl as tauriOpenUrl } from '@tauri-apps/plugin-opener'; * registered application instead of staying inside the embedded * webview. * - * On the Tauri side errors propagate to the caller — we deliberately - * do NOT fall back to `window.open` for desktop. The fallback would - * spawn a Tauri webview window that has no useful behaviour for - * custom schemes (Obsidian, mailto, etc.) and the call would appear - * to "open in a new window" instead of handing off to the OS. + * CEF embedder note: the IPC bridge (`window.ipc.postMessage`) is + * injected on the renderer-side after `on_after_created` fires. + * A click landing in that gap causes the plugin's `invoke()` glue + * to reject with `TypeError: Cannot read properties of undefined + * (reading 'postMessage')`. For http(s) URLs we recover by falling + * back to `window.open` so the user-facing flow still works. For + * non-http schemes we re-throw — `window.open` would spawn a Tauri + * webview window that cannot handle custom schemes, which is worse + * UX than a propagated error the caller can surface. * * In a browser context (no Tauri) we keep the `window.open` path so * `https://` / `mailto:` links still work for dev/preview builds. */ export const openUrl = async (url: string): Promise => { if (isTauri()) { - await tauriOpenUrl(url); - return; + try { + await tauriOpenUrl(url); + return; + } catch (err) { + Sentry.addBreadcrumb({ + category: 'ipc', + level: 'warning', + message: 'tauriOpenUrl failed; evaluating fallback', + data: { url: getTelemetryUrl(url), error: String(err) }, + }); + if (!isHttpUrl(url)) { + throw err; + } + // http(s) URL — safe to fall back to window.open. + } } window.open(url, '_blank', 'noopener,noreferrer'); };