From 1fa70bf34db92eb98f1ffe8f909e9768437b8a86 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:46:24 -0700 Subject: [PATCH] refactor(mobile-sync): share the published-active-group rule with the structured projection Review follow-up. `resolvePublishedActiveGroupId` was a second copy of the rule already in `projectLocalStructuredSessionTabs`; extract it once and call it from both, so the structured projection cannot drift from the publisher. Ablating the shared rule left the structured projection's own tests green, so pin it there too: a snapshot whose active group holds no visible structured tab must not keep pointing at it, and must fall to null when nothing survives. Also repairs a cross-file comment reference that dangled after the relocation. --- ...local-structured-session-tabs-sync.test.ts | 59 +++++++++++++++++++ .../local-structured-session-tabs-sync.ts | 7 +-- .../mobile-session-published-active-group.ts | 17 ++++++ ...e-graph-published-group-references.test.ts | 5 +- .../src/runtime/sync-runtime-graph.ts | 25 +++----- 5 files changed, 89 insertions(+), 24 deletions(-) create mode 100644 src/renderer/src/runtime/mobile-session-published-active-group.ts diff --git a/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts b/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts index 4bd9a2e6d45..f4c76dd3ad1 100644 --- a/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts +++ b/src/renderer/src/runtime/local-structured-session-tabs-sync.test.ts @@ -208,6 +208,65 @@ describe('local structured session tab projection', () => { tabGroupLayout: undefined, tabs: [expect.objectContaining({ type: 'agent-session', agent: 'codex' })] }) + // The surviving group is still the active one, so the pointer passes through untouched. + expect(projectLocalStructuredSessionTabs(snapshot).activeGroupId).toBe('structured-group') + }) + + // Same invariant as the mobile snapshot publisher: this projection drops every group with no + // visible structured tab, so an active-group pointer at a dropped one would name a group the + // caller never receives. + it('does not keep an active-group pointer at a group it dropped', () => { + const base = { + worktree: 'workspace-1', + publicationEpoch: 'epoch-1', + snapshotVersion: 1, + activeGroupId: 'terminal-group', + activeTabId: 'terminal-1', + activeTabType: 'terminal', + tabGroups: [ + { id: 'terminal-group', activeTabId: 'terminal-1', tabOrder: ['terminal-1'] }, + { + id: 'structured-group', + activeTabId: 'agent-session:codex-1', + tabOrder: ['agent-session:codex-1'] + } + ], + tabs: [ + { + type: 'terminal', + id: 'terminal-1', + parentTabId: 'terminal-1', + leafId: 'leaf-1', + title: 'Terminal', + status: 'ready', + terminal: 'term-1', + ptyId: 'pty-1', + isActive: true + }, + { + type: 'agent-session', + id: 'agent-session:codex-1', + title: 'Codex Chat', + sessionId: 'codex-1', + agent: 'codex', + isActive: false + } + ] + } satisfies RuntimeMobileSessionTabsResult + + const projected = projectLocalStructuredSessionTabs(base) + + // The presence half: the structured group does survive, so an empty projection fails here. + expect(projected.tabGroups?.map((group) => group.id)).toEqual(['structured-group']) + expect(projected.activeGroupId).toBe('structured-group') + + // And with nothing left to point at, the pointer goes to null rather than a dropped group. + const structuredDropped = projectLocalStructuredSessionTabs({ + ...base, + tabs: base.tabs.filter((tab) => tab.type !== 'agent-session') + } satisfies RuntimeMobileSessionTabsResult) + expect(structuredDropped.tabGroups).toEqual([]) + expect(structuredDropped.activeGroupId).toBeNull() }) it('preserves the exact local split through apply, persistence, and hydration', () => { diff --git a/src/renderer/src/runtime/local-structured-session-tabs-sync.ts b/src/renderer/src/runtime/local-structured-session-tabs-sync.ts index 7d8ee6aa626..9de6102c872 100644 --- a/src/renderer/src/runtime/local-structured-session-tabs-sync.ts +++ b/src/renderer/src/runtime/local-structured-session-tabs-sync.ts @@ -10,6 +10,7 @@ import { decideWebSessionTabsSnapshot } from './web-session-tabs-sync' import type { WebSessionTabsSyncState } from './web-session-tabs-sync' +import { resolvePublishedActiveGroupId } from './mobile-session-published-active-group' export const LOCAL_STRUCTURED_SESSION_OWNER = 'local-structured-session' let localStructuredSessionTabsRestorePromise: Promise | null = null @@ -42,11 +43,7 @@ export function projectLocalStructuredSessionTabs( activeTabId: visibleIds.has(snapshot.activeTabId ?? '') ? snapshot.activeTabId : null, activeTabType: snapshot.activeTabId && visibleIds.has(snapshot.activeTabId) ? snapshot.activeTabType : null, - activeGroupId: - snapshot.activeGroupId && - projectedTabGroups?.some((group) => group.id === snapshot.activeGroupId) - ? snapshot.activeGroupId - : (projectedTabGroups?.[0]?.id ?? null), + activeGroupId: resolvePublishedActiveGroupId(snapshot.activeGroupId, projectedTabGroups), tabs: snapshot.tabs.filter((tab) => visibleIds.has(tab.id)), tabGroups: projectedTabGroups, // Why: group membership locates chats; the renderer's split tree remains locally authoritative. diff --git a/src/renderer/src/runtime/mobile-session-published-active-group.ts b/src/renderer/src/runtime/mobile-session-published-active-group.ts new file mode 100644 index 00000000000..fdbfc76b150 --- /dev/null +++ b/src/renderer/src/runtime/mobile-session-published-active-group.ts @@ -0,0 +1,17 @@ +import type { RuntimeMobileSessionTabGroup } from '../../../shared/runtime-session-contracts' + +/** + * The active-group pointer a mobile snapshot is allowed to publish. + * + * Why: a group whose every tab was held back never reaches the client, so naming it would point at + * something the client was never sent. Falls back to the first group that did publish, then null. + */ +export function resolvePublishedActiveGroupId( + activeGroupId: string | null, + publishedGroups: readonly RuntimeMobileSessionTabGroup[] | undefined +): string | null { + if (activeGroupId && publishedGroups?.some((group) => group.id === activeGroupId)) { + return activeGroupId + } + return publishedGroups?.[0]?.id ?? null +} diff --git a/src/renderer/src/runtime/sync-runtime-graph-published-group-references.test.ts b/src/renderer/src/runtime/sync-runtime-graph-published-group-references.test.ts index d495c9fce1d..813d98f436c 100644 --- a/src/renderer/src/runtime/sync-runtime-graph-published-group-references.test.ts +++ b/src/renderer/src/runtime/sync-runtime-graph-published-group-references.test.ts @@ -35,8 +35,9 @@ function danglingGroupRefs( } describe('buildMobileSessionTabSnapshots published group references', () => { - // Why the same claim is driven again through tab groups: the test above takes the legacy nav - // order, where the only thing that has to hold the document back is the per-tab guard. With + // Why groups get their own fixture: 'omits a browser tab located by a workspace document from + // mobile snapshots' (in sync-runtime-graph-editor-diff-tabs.test.ts) drives the same document + // through the legacy nav order, where the per-tab guard is the only thing holding it back. With // groups the projection runs first and publishes group metadata of its own, so a document reaching // it leaves a group whose tabOrder and activeTabId name a tab the phone is never sent. it('keeps a workspace document out of published tab groups as well as the tab list', () => { diff --git a/src/renderer/src/runtime/sync-runtime-graph.ts b/src/renderer/src/runtime/sync-runtime-graph.ts index 06927bfbb94..c9d937684be 100644 --- a/src/renderer/src/runtime/sync-runtime-graph.ts +++ b/src/renderer/src/runtime/sync-runtime-graph.ts @@ -29,6 +29,7 @@ import type { import { isTerminalLeafId, makePaneKey, parsePaneKey } from '../../../shared/stable-pane-id' import { isWebTerminalSurfaceTabId } from '../../../shared/terminal-surface-id' import { isClaudeManagementTitle } from '../../../shared/agent-detection' +import { resolvePublishedActiveGroupId } from './mobile-session-published-active-group' import { parseWorkspaceKey } from '../../../shared/workspace-scope' import type { Tab, TabGroup, TabGroupLayoutNode } from '../../../shared/tab-types' import type { @@ -1420,18 +1421,18 @@ export function buildMobileSessionTabSnapshots( fallbackEditorTabs, active?.id ?? null ) - const tabGroupLayout = - tabGroups && tabGroups.length > 0 - ? pruneTabGroupLayout(inputs.tabGroupLayout, new Set(tabGroups.map((group) => group.id))) - : groupProjection.tabGroupLayout const publishedGroups = tabGroups && tabGroups.length > 0 ? tabGroups : undefined + const tabGroupLayout = publishedGroups + ? pruneTabGroupLayout( + inputs.tabGroupLayout, + new Set(publishedGroups.map((group) => group.id)) + ) + : groupProjection.tabGroupLayout const content = { - // Why not the desktop's own active group: a group whose every tab was held back never reaches - // the client, so naming it here points at something the client was never sent (STA-5724). activeGroupId: resolvePublishedActiveGroupId(activeGroupId, publishedGroups), activeTabId: active?.id ?? null, activeTabType: active?.type ?? null, - ...(tabGroups && tabGroups.length > 0 ? { tabGroups } : {}), + ...(publishedGroups ? { tabGroups: publishedGroups } : {}), ...(tabGroupLayout ? { tabGroupLayout } : {}), tabs } @@ -1502,16 +1503,6 @@ function applyUnifiedEditorTabIdsToLegacyOrder( }) } -function resolvePublishedActiveGroupId( - activeGroupId: string | null, - publishedGroups: readonly RuntimeMobileSessionTabGroup[] | undefined -): string | null { - if (activeGroupId && publishedGroups?.some((group) => group.id === activeGroupId)) { - return activeGroupId - } - return publishedGroups?.[0]?.id ?? null -} - function appendFallbackEditorTabsToGroups( tabGroups: RuntimeMobileSessionTabGroup[] | undefined, sourceGroups: readonly TabGroup[],