From b84ee23eec0887cf1818ec7bdcb293a50e8d4350 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Fri, 4 Sep 2026 16:33:03 -0400 Subject: [PATCH] fix(desktop): drop the dead tabList override and close the hosted-bundle CI gap - Revert CdpBridge.tabList to main's delegation: CdpBridge has no production constructor, browser.tabList is served by AgentBrowserBridgeTabs, and the override published canGoBack/canGoForward that BrowserTabInfo never declared. The hybrid shell reads navigation state from the screencast 'navigation' event, not from tabList, so nothing regresses. - pr-code-change-scope: mobile/host-web-app and mobile/app/h import broadly across mobile/src, so a mobile/src edit changes out/mobile-web-rnw, which ships in every desktop installer. Add mobile/src/ to the carve-out and keep mobile test files desktop-irrelevant so the cost stays bounded. - Move @noble/hashes to devDependencies: its only root consumers are src/mobile-web/src, which Metro inlines at packaging time, matching buffer. - files.unwatch awaits teardown again on the connection-scoped path via cleanupIfOwnedByConnectionAndWait, so a rewatch cannot hold two watchers. - readGuestNavigationState warns once instead of silently greying out Back/Forward when navigationHistory is missing. - One exported isRecord in src/shared and one byte-length encoder in src/mobile-web/src; mobile/ copies left to a later pass. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- config/scripts/pr-code-change-scope.mjs | 15 ++-- config/scripts/pr-code-change-scope.test.mjs | 32 +++++++-- config/tsconfig.mobile-web.json | 1 + package.json | 2 +- pnpm-lock.yaml | 6 +- .../browser/browser-guest-navigation-state.ts | 17 ++++- src/main/browser/cdp-bridge.ts | 24 +------ .../methods/files-unwatch-ownership.test.ts | 70 +++++++++++++++---- src/main/runtime/rpc/methods/files.ts | 2 +- .../runtime-service-command-surface.ts | 3 + .../runtime/runtime-subscription-registry.ts | 38 +++++++--- .../mobile-web-subscription-setup-error.ts | 14 +--- src/shared/is-record.ts | 3 + .../mobile-web/bridge-message-parser.ts | 5 +- 14 files changed, 158 insertions(+), 74 deletions(-) create mode 100644 src/shared/is-record.ts diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index cbaf869fc1c..19a62276bff 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -183,6 +183,9 @@ const SHARED_PACKAGE_PREFIXES = [ // sources remain mobile-only. const DESKTOP_RELEVANT_MOBILE_PREFIXES = [ 'mobile/app/h/', + // Why: mobile/host-web-app and mobile/app/h import broadly across mobile/src, so a + // mobile/src edit changes out/mobile-web-rnw, which ships in every desktop installer. + 'mobile/src/', 'mobile/package.json', 'mobile/pnpm-lock.yaml', 'mobile/pnpm-workspace.yaml', @@ -360,10 +363,14 @@ function isTestFile(file) { } function isDesktopIrrelevantPath(file) { - return ( - matchesPrefix(file, DESKTOP_IRRELEVANT_PREFIXES) && - !matchesPrefix(file, DESKTOP_RELEVANT_MOBILE_PREFIXES) - ) + return matchesPrefix(file, DESKTOP_IRRELEVANT_PREFIXES) && !isDesktopRelevantMobilePath(file) +} + +// Why: a mobile test file cannot reach out/mobile-web-rnw, so keep test-only mobile diffs +// desktop-irrelevant — otherwise the mobile/src carve-out drags the full desktop matrix +// onto every mobile PR. +function isDesktopRelevantMobilePath(file) { + return !isTestFile(file) && matchesPrefix(file, DESKTOP_RELEVANT_MOBILE_PREFIXES) } function isNativeCacheInputPath(file) { diff --git a/config/scripts/pr-code-change-scope.test.mjs b/config/scripts/pr-code-change-scope.test.mjs index 0c0162c4f59..404503ed22a 100644 --- a/config/scripts/pr-code-change-scope.test.mjs +++ b/config/scripts/pr-code-change-scope.test.mjs @@ -98,7 +98,14 @@ describe('docs-only path classification', () => { }) it('does not start desktop PR Checks for mobile-only diffs', () => { - expect(shouldRunPrChecks(['mobile/src/App.tsx'])).toBe(false) + // Native-only routes never enter the hosted export, whose router root is host-web-app. + expect(shouldRunPrChecks(['mobile/app/settings.tsx'])).toBe(false) + expect(shouldRunPrChecks(['mobile/ios/Podfile'])).toBe(false) + }) + + it('keeps mobile test-only diffs off the desktop matrix', () => { + expect(shouldRunPrChecks(['mobile/src/session/mobile-session-surface.test.tsx'])).toBe(false) + expect(shouldRunPrChecks(['mobile/app/h/[hostId]/tasks.test.tsx'])).toBe(false) }) it('runs desktop packaging when a shared host route changes', () => { @@ -108,6 +115,20 @@ describe('docs-only path classification', () => { }) }) + // Why: mobile/host-web-app and mobile/app/h import broadly across mobile/src, and the + // export they feed becomes out/mobile-web-rnw, which every desktop installer carries. A + // mobile/src edit must reach the bundle's own budget and afterPack verification gates. + it('runs desktop packaging when hosted-page sources under mobile/src change', () => { + for (const file of [ + 'mobile/src/session/MobileSessionSurface.tsx', + 'mobile/src/tasks/use-mobile-tasks-host-operations.ts', + 'mobile/src/transport/types.ts', + 'mobile/src/mobile-web/mobile-web-session-snapshot.ts' + ]) { + expectClassification([file], { package: true, package_windows: true }) + } + }) + it('does not start desktop PR Checks for cloud-only diffs', () => { expect( shouldRunPrChecks([ @@ -370,9 +391,12 @@ describe('per-job path classification', () => { ).toBe(true) // Why false: a mobile-only diff skips every desktop job, so the install step's own // job never runs and claiming the install is needed contradicts should_run. - expect(classifyPrJobs(['mobile/src/a.ts']).mobile_dependencies).toBe(false) - expect(classifyPrJobs(['mobile/src/a.ts']).should_run).toBe(false) - expect(classifyPrJobs(['README.md', 'mobile/src/a.ts']).mobile_dependencies).toBe(false) + expect(classifyPrJobs(['mobile/app/settings.tsx']).mobile_dependencies).toBe(false) + expect(classifyPrJobs(['mobile/app/settings.tsx']).should_run).toBe(false) + expect(classifyPrJobs(['README.md', 'mobile/app/settings.tsx']).mobile_dependencies).toBe(false) + // Hosted-page sources keep desktop jobs on, so the install they lint with is needed. + expect(classifyPrJobs(['mobile/src/a.ts']).should_run).toBe(true) + expect(classifyPrJobs(['mobile/src/a.ts']).mobile_dependencies).toBe(true) // The hosted page's dependency manifest is packaged by the desktop runtime, so // its diff keeps desktop jobs (and the install they lint with) enabled. expect(classifyPrJobs(['mobile/package.json']).should_run).toBe(true) diff --git a/config/tsconfig.mobile-web.json b/config/tsconfig.mobile-web.json index 95222a2fd9d..08d6ef5d176 100644 --- a/config/tsconfig.mobile-web.json +++ b/config/tsconfig.mobile-web.json @@ -6,6 +6,7 @@ "../src/shared/clipboard-text.ts", "../src/shared/event-loop-yield.ts", "../src/shared/file-link-location.ts", + "../src/shared/is-record.ts", "../src/shared/mobile-markdown-document.ts", "../src/shared/mobile-web/**/*", "../src/shared/terminal-file-link-matcher.ts", diff --git a/package.json b/package.json index 7929be6fc2e..f6f264556c9 100644 --- a/package.json +++ b/package.json @@ -165,7 +165,6 @@ "@electron-toolkit/utils": "^4.0.0", "@floating-ui/dom": "1.7.6", "@linear/sdk": "^82.1.0", - "@noble/hashes": "1.8.0", "@parcel/watcher": "^2.5.6", "@xterm/addon-serialize": "0.15.0-beta.300", "@xterm/headless": "6.1.0-beta.302", @@ -193,6 +192,7 @@ "@electron-toolkit/tsconfig": "^2.0.0", "@electron/rebuild": "^4.2.0", "@monaco-editor/react": "^4.7.0", + "@noble/hashes": "1.8.0", "@playwright/test": "^1.59.1", "@sanity/diff-match-patch": "^3.2.0", "@stablyai/playwright-test": "^2.1.14", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7d9166f5c84..5a63b5a9377 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -134,9 +134,6 @@ importers: '@linear/sdk': specifier: ^82.1.0 version: 82.1.0(graphql@16.14.2) - '@noble/hashes': - specifier: 1.8.0 - version: 1.8.0 '@parcel/watcher': specifier: ^2.5.6 version: 2.5.6 @@ -213,6 +210,9 @@ importers: '@monaco-editor/react': specifier: ^4.7.0 version: 4.7.0(monaco-editor@0.55.1)(react-dom@19.2.8(react@19.2.8))(react@19.2.8) + '@noble/hashes': + specifier: 1.8.0 + version: 1.8.0 '@playwright/test': specifier: ^1.59.1 version: 1.59.1 diff --git a/src/main/browser/browser-guest-navigation-state.ts b/src/main/browser/browser-guest-navigation-state.ts index 1911ed9f634..92e85116e90 100644 --- a/src/main/browser/browser-guest-navigation-state.ts +++ b/src/main/browser/browser-guest-navigation-state.ts @@ -1,11 +1,24 @@ +let warnedMissingNavigationHistory = false + export function readGuestNavigationState(guest: Electron.WebContents): { canGoBack: boolean canGoForward: boolean } { const history = guest.navigationHistory + // Why: Electron always provides navigationHistory, so `false` here is an API break, not a + // page with no history — say so once instead of silently greying out Back/Forward forever. + if (typeof history?.canGoBack !== 'function' || typeof history?.canGoForward !== 'function') { + if (!warnedMissingNavigationHistory) { + warnedMissingNavigationHistory = true + console.warn( + '[browser-guest] webContents.navigationHistory is unavailable; Back/Forward stay disabled' + ) + } + return { canGoBack: false, canGoForward: false } + } return { - canGoBack: history?.canGoBack?.() ?? false, - canGoForward: history?.canGoForward?.() ?? false + canGoBack: history.canGoBack(), + canGoForward: history.canGoForward() } } diff --git a/src/main/browser/cdp-bridge.ts b/src/main/browser/cdp-bridge.ts index 476b4e4aba3..77aba3ba6d5 100644 --- a/src/main/browser/cdp-bridge.ts +++ b/src/main/browser/cdp-bridge.ts @@ -27,15 +27,12 @@ import type { BrowserSelectResult, BrowserSnapshotResult, BrowserTabListResult, - BrowserTabInfo, BrowserTabSwitchResult, BrowserTypeResult, BrowserUploadResult, BrowserViewportResult, BrowserWaitResult } from '../../shared/runtime-types' -import { readGuestNavigationState } from './browser-guest-navigation-state' -import { webContents } from 'electron' import type { BrowserManager } from './browser-manager' import type { CdpAuxiliaryCommands, CdpTabState } from './cdp-auxiliary-commands' import { CdpBridgeCommandSet } from './cdp-bridge-command-set' @@ -259,26 +256,7 @@ export class CdpBridge { } tabList(): BrowserTabListResult { - const tabs: BrowserTabInfo[] = [] - let index = 0 - - for (const [tabId, wcId] of this.getRegisteredTabs()) { - const guest = webContents.fromId(wcId) - if (!guest || guest.isDestroyed()) { - continue - } - tabs.push({ - browserPageId: tabId, - index, - url: guest.getURL(), - title: guest.getTitle(), - active: wcId === this.activeWebContentsId, - ...readGuestNavigationState(guest) - }) - index++ - } - - return { tabs } + return this.commands.tabs.tabList() } async tabSwitch(index: number): Promise { diff --git a/src/main/runtime/rpc/methods/files-unwatch-ownership.test.ts b/src/main/runtime/rpc/methods/files-unwatch-ownership.test.ts index 958af93edc2..015d5c21ff9 100644 --- a/src/main/runtime/rpc/methods/files-unwatch-ownership.test.ts +++ b/src/main/runtime/rpc/methods/files-unwatch-ownership.test.ts @@ -2,35 +2,81 @@ import { describe, expect, it, vi } from 'vitest' import type { OrcaRuntimeService } from '../../orca-runtime' import { RpcDispatcher } from '../dispatcher' import type { RpcRequest } from '../core' +import { RuntimeSubscriptionRegistry } from '../../runtime-subscription-registry' import { FILE_METHODS } from './files' +function unwatchRequest(subscriptionId: string): RpcRequest { + return { + id: 'req-1', + authToken: 'tok', + method: 'files.unwatch', + params: { subscriptionId } + } +} + describe('files.unwatch ownership', () => { it('refuses teardown when the socket does not own the subscription', async () => { - const cleanupSubscriptionIfOwnedByConnection = vi.fn().mockReturnValue(false) + const cleanupSubscriptionIfOwnedByConnectionAndWait = vi.fn().mockResolvedValue(false) const cleanupSubscriptionAndWait = vi.fn() const runtime = { getRuntimeId: () => 'test-runtime', - cleanupSubscriptionIfOwnedByConnection, + cleanupSubscriptionIfOwnedByConnectionAndWait, cleanupSubscriptionAndWait } as unknown as OrcaRuntimeService const dispatcher = new RpcDispatcher({ runtime, methods: FILE_METHODS }) - const request: RpcRequest = { - id: 'req-1', - authToken: 'tok', - method: 'files.unwatch', - params: { subscriptionId: 'files-watch-conn-owner-1' } - } const replies: unknown[] = [] - await dispatcher.dispatchStreaming(request, (reply) => replies.push(JSON.parse(reply)), { - connectionId: 'conn-attacker' - }) + await dispatcher.dispatchStreaming( + unwatchRequest('files-watch-conn-owner-1'), + (reply) => replies.push(JSON.parse(reply)), + { connectionId: 'conn-attacker' } + ) - expect(cleanupSubscriptionIfOwnedByConnection).toHaveBeenCalledWith( + expect(cleanupSubscriptionIfOwnedByConnectionAndWait).toHaveBeenCalledWith( 'files-watch-conn-owner-1', 'conn-attacker' ) expect(cleanupSubscriptionAndWait).not.toHaveBeenCalled() expect(replies).toEqual([expect.objectContaining({ result: { unsubscribed: false } })]) }) + + // Why: returning before @parcel/watcher is released lets a rewatch hold two watchers. + it('does not reply until the owning connection teardown settles', async () => { + const subscriptions = new RuntimeSubscriptionRegistry() + let releaseWatcher: (() => void) | undefined + let released = false + subscriptions.register( + 'files-watch-slow-1', + () => + new Promise((resolve) => { + releaseWatcher = () => { + released = true + resolve() + } + }), + 'conn-owner' + ) + const runtime = { + getRuntimeId: () => 'test-runtime', + cleanupSubscriptionIfOwnedByConnectionAndWait: + subscriptions.cleanupIfOwnedByConnectionAndWait.bind(subscriptions) + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: FILE_METHODS }) + + let settled = false + const pending = dispatcher + .dispatch(unwatchRequest('files-watch-slow-1'), { connectionId: 'conn-owner' }) + .then((response) => { + settled = true + return response + }) + + await vi.waitFor(() => expect(releaseWatcher).toBeDefined()) + expect(settled).toBe(false) + expect(released).toBe(false) + + releaseWatcher?.() + await expect(pending).resolves.toMatchObject({ ok: true, result: { unsubscribed: true } }) + expect(released).toBe(true) + }) }) diff --git a/src/main/runtime/rpc/methods/files.ts b/src/main/runtime/rpc/methods/files.ts index d757bdc7742..7e9d9a3cf23 100644 --- a/src/main/runtime/rpc/methods/files.ts +++ b/src/main/runtime/rpc/methods/files.ts @@ -280,7 +280,7 @@ export const FILE_METHODS: RpcAnyMethod[] = [ handler: async (params, { runtime, connectionId }) => { if (connectionId) { return { - unsubscribed: runtime.cleanupSubscriptionIfOwnedByConnection( + unsubscribed: await runtime.cleanupSubscriptionIfOwnedByConnectionAndWait( params.subscriptionId, connectionId ) diff --git a/src/main/runtime/runtime-service-command-surface.ts b/src/main/runtime/runtime-service-command-surface.ts index 19545cc76e6..8acaed830a3 100644 --- a/src/main/runtime/runtime-service-command-surface.ts +++ b/src/main/runtime/runtime-service-command-surface.ts @@ -23,6 +23,7 @@ export type RuntimeServiceCommandSurface = { cleanupSubscriptionsByPrefix: RuntimeSubscriptionRegistry['cleanupByPrefix'] cleanupSubscriptionsForConnection: RuntimeSubscriptionRegistry['cleanupForConnection'] cleanupSubscriptionIfOwnedByConnection: RuntimeSubscriptionRegistry['cleanupIfOwnedByConnection'] + cleanupSubscriptionIfOwnedByConnectionAndWait: RuntimeSubscriptionRegistry['cleanupIfOwnedByConnectionAndWait'] onNotificationDispatched: RuntimeMobileNotificationController['onDispatched'] getMobileNotificationListenerCount: RuntimeMobileNotificationController['getListenerCount'] dispatchMobileNotification: RuntimeMobileNotificationController['dispatch'] @@ -103,6 +104,8 @@ export function installRuntimeServiceCommandSurface( cleanupSubscriptionsForConnection: subscriptions.cleanupForConnection.bind(subscriptions), cleanupSubscriptionIfOwnedByConnection: subscriptions.cleanupIfOwnedByConnection.bind(subscriptions), + cleanupSubscriptionIfOwnedByConnectionAndWait: + subscriptions.cleanupIfOwnedByConnectionAndWait.bind(subscriptions), onNotificationDispatched: notifications.onDispatched.bind(notifications), getMobileNotificationListenerCount: notifications.getListenerCount.bind(notifications), dispatchMobileNotification: notifications.dispatch.bind(notifications), diff --git a/src/main/runtime/runtime-subscription-registry.ts b/src/main/runtime/runtime-subscription-registry.ts index febab3f85ff..198874bb495 100644 --- a/src/main/runtime/runtime-subscription-registry.ts +++ b/src/main/runtime/runtime-subscription-registry.ts @@ -42,18 +42,40 @@ export class RuntimeSubscriptionRegistry { } cleanupIfOwnedByConnection(subscriptionId: string, connectionId?: string): boolean { - if (!connectionId) { + const verdict = this.resolveConnectionOwnership(subscriptionId, connectionId) + if (verdict === 'cleanup') { this.cleanup(subscriptionId) - return true } + return verdict !== 'foreign' + } + + // Why: an unwatch that returns before the underlying watcher is released lets a fast + // rewatch hold two watchers on one path and double-deliver change events. + async cleanupIfOwnedByConnectionAndWait( + subscriptionId: string, + connectionId?: string + ): Promise { + const verdict = this.resolveConnectionOwnership(subscriptionId, connectionId) + if (verdict === 'cleanup') { + await this.cleanupAndWait(subscriptionId) + } + return verdict !== 'foreign' + } + + private resolveConnectionOwnership( + subscriptionId: string, + connectionId?: string + ): 'cleanup' | 'absent' | 'foreign' { + if (!connectionId) { + return 'cleanup' + } + // An unregistered id is already gone, not refused. if (!this.cleanups.has(subscriptionId)) { - return true + return 'absent' } - if (this.connectionBySubscription.get(subscriptionId) !== connectionId) { - return false - } - this.cleanup(subscriptionId) - return true + return this.connectionBySubscription.get(subscriptionId) === connectionId + ? 'cleanup' + : 'foreign' } cleanup(subscriptionId: string): void { diff --git a/src/mobile-web/src/mobile-web-subscription-setup-error.ts b/src/mobile-web/src/mobile-web-subscription-setup-error.ts index 22a5fb2c190..d88a9a02c7d 100644 --- a/src/mobile-web/src/mobile-web-subscription-setup-error.ts +++ b/src/mobile-web/src/mobile-web-subscription-setup-error.ts @@ -3,6 +3,7 @@ import { MOBILE_WEB_BRIDGE_MAX_SUBSCRIPTIONS, type MobileWebBridgeShellMessage } from '../../shared/mobile-web/bridge-contract' +import { encodedMobileWebBridgeValueByteLength } from './mobile-web-bridge-request-encoding' import { MobileWebBridgeClientError } from './mobile-web-bridge-client-error' type OperationGrant = Extract['grants'][number] @@ -25,7 +26,7 @@ export function mobileWebSubscriptionSetupError(args: { if (!args.payloadValid) { return new MobileWebBridgeClientError('invalid_request', false) } - if (encodedByteLength(args.payload) > args.grant.limits.maxRequestBytes) { + if (encodedMobileWebBridgeValueByteLength(args.payload) > args.grant.limits.maxRequestBytes) { return new MobileWebBridgeClientError('too_large', false) } if ( @@ -36,14 +37,3 @@ export function mobileWebSubscriptionSetupError(args: { } return null } - -function encodedByteLength(value: unknown): number { - try { - const encoded = JSON.stringify(value) - return encoded === undefined - ? Number.POSITIVE_INFINITY - : new TextEncoder().encode(encoded).byteLength - } catch { - return Number.POSITIVE_INFINITY - } -} diff --git a/src/shared/is-record.ts b/src/shared/is-record.ts new file mode 100644 index 00000000000..52dc4abbb65 --- /dev/null +++ b/src/shared/is-record.ts @@ -0,0 +1,3 @@ +export function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} diff --git a/src/shared/mobile-web/bridge-message-parser.ts b/src/shared/mobile-web/bridge-message-parser.ts index 01c1cfc538b..68e153b2e0e 100644 --- a/src/shared/mobile-web/bridge-message-parser.ts +++ b/src/shared/mobile-web/bridge-message-parser.ts @@ -1,4 +1,5 @@ import type { z } from 'zod' +import { isRecord } from '../is-record' import { MOBILE_WEB_BRIDGE_MAX_MESSAGE_BYTES } from './bridge-limits' import { MOBILE_WEB_BRIDGE_PROTOCOL_VERSION } from './bridge-protocol-version' import { isExactMobileWebJsonDocument } from './exact-json-document' @@ -62,7 +63,3 @@ export function parseMobileWebBridgeMessageDocument( } return { ok: true, value: parsed.data } } - -function isRecord(value: unknown): value is Record { - return typeof value === 'object' && value !== null && !Array.isArray(value) -}