mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 08:03:20 +00:00
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
This commit is contained in:
@@ -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) {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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",
|
||||
|
||||
+1
-1
@@ -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",
|
||||
|
||||
Generated
+3
-3
@@ -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
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<BrowserTabSwitchResult> {
|
||||
|
||||
@@ -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<void>((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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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<boolean> {
|
||||
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 {
|
||||
|
||||
@@ -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<MobileWebBridgeShellMessage, { type: 'init' }>['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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,3 @@
|
||||
export function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === 'object' && value !== null && !Array.isArray(value)
|
||||
}
|
||||
@@ -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<T>(
|
||||
}
|
||||
return { ok: true, value: parsed.data }
|
||||
}
|
||||
|
||||
function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === 'object' && value !== null && !Array.isArray(value)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user