fix(ui): mount only the active session chat iframe (#2816)
* fix(ui): mount only the active session chat iframe * repro(ui): mount-all-persisted session-chat iframes (#2815) Adds a regression-style reproduction for issue #2815: ContextPanel renders one full-application iframe for every chat tab (inactive tabs are only hidden via the Tailwind 'hidden' class, never unmounted), so a reload restores all persisted session-chat tabs from the ui-store and mounts N embedded OpenChamber apps in one browser tab. The test reads the real ContextPanel.tsx render block, drives the real useUIStore with the issue's persisted scenario (11 tabs, 8 read-only session-chat tabs), and models the render block with the real buildEmbeddedSessionChatURL helper, showing 8 live src iframes (7 hidden but loaded). * test(ui): adapt issue 2815 reproduction for active chat * fix(ui): unmount session chat when panel closes --------- Co-authored-by: ChangeHow <23733347+ChangeHow@users.noreply.github.com>
This commit is contained in:
Andrea V
committed by
GitHub
GitHub
co-authored by
ChangeHow
ChangeHow
parent
b55152db6f
commit
454119ac25
@@ -45,6 +45,7 @@ import { invokeDesktopCommand } from '@/lib/desktopNative';
|
||||
import {
|
||||
EMBEDDED_RUNTIME_BOOTSTRAP_REQUEST,
|
||||
EMBEDDED_RUNTIME_BOOTSTRAP_RESPONSE,
|
||||
getActiveEmbeddedSessionChatTab,
|
||||
getOrCreateEmbeddedSessionChatURL,
|
||||
type EmbeddedSessionChatURLCacheEntry,
|
||||
type EmbeddedSessionRuntimeBootstrap,
|
||||
@@ -2483,8 +2484,13 @@ export const ContextPanel: React.FC = () => {
|
||||
|
||||
}, [activeTab, directoryKey, setSelectedFilePath]);
|
||||
|
||||
const activeChatTabID = activeTab?.mode === 'chat' ? activeTab.id : null;
|
||||
const activeChatSessionID = activeTab?.mode === 'chat' ? getSessionIDFromDedupeKey(activeTab.dedupeKey) : null;
|
||||
const chatTabs = React.useMemo(
|
||||
() => tabs.filter((tab) => tab.mode === 'chat'),
|
||||
[tabs],
|
||||
);
|
||||
const activeChatTabID = isOpen && activeTab?.mode === 'chat' ? activeTab.id : null;
|
||||
const activeChatSessionID = isOpen && activeTab?.mode === 'chat' ? getSessionIDFromDedupeKey(activeTab.dedupeKey) : null;
|
||||
const activeChatTab = getActiveEmbeddedSessionChatTab(chatTabs, activeChatTabID);
|
||||
|
||||
React.useEffect(() => {
|
||||
if (!isOpen || !directoryKey || !activeChatSessionID || typeof window === 'undefined') {
|
||||
@@ -2525,6 +2531,10 @@ export const ContextPanel: React.FC = () => {
|
||||
});
|
||||
}, [currentTheme, darkThemeId, directoryKey, lightThemeId, themeMode]);
|
||||
|
||||
const activeChatSrc = activeChatTab && activeChatSessionID
|
||||
? getEmbeddedChatSrc(activeChatTab.id, activeChatSessionID, activeChatTab.readOnly)
|
||||
: null;
|
||||
|
||||
React.useEffect(() => {
|
||||
const liveTabIDs = new Set(tabs.map((tab) => tab.id));
|
||||
for (const tabID of chatFrameSrcByTabIDRef.current.keys()) {
|
||||
@@ -2721,10 +2731,6 @@ export const ContextPanel: React.FC = () => {
|
||||
</div>
|
||||
);
|
||||
|
||||
const chatTabs = React.useMemo(
|
||||
() => tabs.filter((tab) => tab.mode === 'chat'),
|
||||
[tabs],
|
||||
);
|
||||
const browserTabs = React.useMemo(
|
||||
() => tabs.filter((tab) => tab.mode === 'browser'),
|
||||
[tabs],
|
||||
@@ -2934,41 +2940,26 @@ export const ContextPanel: React.FC = () => {
|
||||
<EditorTreeColumn visible={contextEditorTreeVisible} />
|
||||
</div>
|
||||
) : null}
|
||||
{chatTabs.map((tab) => {
|
||||
const sessionID = getSessionIDFromDedupeKey(tab.dedupeKey);
|
||||
if (!sessionID) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const src = getEmbeddedChatSrc(tab.id, sessionID, tab.readOnly);
|
||||
if (!src) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return (
|
||||
<iframe
|
||||
key={tab.id}
|
||||
ref={(node) => {
|
||||
if (!node) {
|
||||
chatFrameRefs.current.delete(tab.id);
|
||||
return;
|
||||
}
|
||||
chatFrameRefs.current.set(tab.id, node);
|
||||
}}
|
||||
src={src}
|
||||
title={t('contextPanel.iframe.sessionChatTitle', { sessionID })}
|
||||
className={cn(
|
||||
'absolute inset-0 h-full w-full border-0 bg-background',
|
||||
activeChatTabID === tab.id ? 'block' : 'hidden'
|
||||
)}
|
||||
onLoad={() => {
|
||||
postThemeSyncToEmbeddedChat();
|
||||
postChatSettingsSyncToEmbeddedChat();
|
||||
postEmbeddedVisibilityToChats();
|
||||
}}
|
||||
/>
|
||||
);
|
||||
})}
|
||||
{activeChatTab && activeChatSessionID && activeChatSrc ? (
|
||||
<iframe
|
||||
key={activeChatTab.id}
|
||||
ref={(node) => {
|
||||
if (!node) {
|
||||
chatFrameRefs.current.delete(activeChatTab.id);
|
||||
return;
|
||||
}
|
||||
chatFrameRefs.current.set(activeChatTab.id, node);
|
||||
}}
|
||||
src={activeChatSrc}
|
||||
title={t('contextPanel.iframe.sessionChatTitle', { sessionID: activeChatSessionID })}
|
||||
className="absolute inset-0 h-full w-full border-0 bg-background"
|
||||
onLoad={() => {
|
||||
postThemeSyncToEmbeddedChat();
|
||||
postChatSettingsSyncToEmbeddedChat();
|
||||
postEmbeddedVisibilityToChats();
|
||||
}}
|
||||
/>
|
||||
) : null}
|
||||
{browserTabs.map((tab) => (
|
||||
<div
|
||||
key={tab.id}
|
||||
|
||||
+183
@@ -0,0 +1,183 @@
|
||||
/**
|
||||
* Regression coverage for https://github.com/openchamber/openchamber/issues/2815
|
||||
*
|
||||
* A full ContextPanel mount is not available in bun test because its import
|
||||
* graph includes a Vite worker URL. This test follows the source-level guard
|
||||
* pattern in contextPanelEscapeClosesTerminal.test.ts and uses the real store.
|
||||
*/
|
||||
import { afterAll, beforeEach, describe, expect, test } from 'bun:test';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
||||
import { getDefaultTheme } from '@/lib/theme/themes';
|
||||
import { useUIStore } from '@/stores/useUIStore';
|
||||
import {
|
||||
buildEmbeddedSessionChatURL,
|
||||
getActiveEmbeddedSessionChatTab,
|
||||
resetEmbeddedSessionChatCache,
|
||||
} from '../contextPanelEmbeddedChat';
|
||||
|
||||
const __dirname = dirname(fileURLToPath(import.meta.url));
|
||||
const contextPanelSource = readFileSync(join(__dirname, '..', 'ContextPanel.tsx'), 'utf-8');
|
||||
|
||||
type FixtureTab = {
|
||||
id: string;
|
||||
mode: 'chat' | 'git' | 'diff' | 'plan';
|
||||
targetPath: string | null;
|
||||
dedupeKey: string;
|
||||
label: string | null;
|
||||
sessionTitleFallback: string | null;
|
||||
readOnly: boolean;
|
||||
stagedDiff: boolean;
|
||||
diffScope: 'working';
|
||||
touchedAt: number;
|
||||
};
|
||||
|
||||
const DIRECTORY = '/path/to/repository';
|
||||
const originalWindow = globalThis.window;
|
||||
|
||||
const installWindowLocation = () => {
|
||||
const url = new URL('http://127.0.0.1:3000/');
|
||||
Object.defineProperty(globalThis, 'window', {
|
||||
configurable: true,
|
||||
value: {
|
||||
location: {
|
||||
href: url.toString(),
|
||||
origin: url.origin,
|
||||
pathname: url.pathname,
|
||||
search: url.search,
|
||||
},
|
||||
},
|
||||
});
|
||||
};
|
||||
|
||||
const buildTab = (mode: FixtureTab['mode'], id: string): FixtureTab => ({
|
||||
id: mode === 'chat' ? `chat:session:${id}` : id,
|
||||
mode,
|
||||
targetPath: null,
|
||||
dedupeKey: mode === 'chat' ? `session:${id}` : id,
|
||||
label: mode === 'chat' ? `Session ${id}` : null,
|
||||
sessionTitleFallback: null,
|
||||
readOnly: mode === 'chat',
|
||||
stagedDiff: false,
|
||||
diffScope: 'working',
|
||||
touchedAt: Date.now(),
|
||||
});
|
||||
|
||||
const sessionChatTabs = Array.from({ length: 8 }, (_, index) => buildTab('chat', `ses_${index + 1}`));
|
||||
const issueScenarioTabs = [
|
||||
...sessionChatTabs,
|
||||
buildTab('git', 'git'),
|
||||
buildTab('diff', 'diff'),
|
||||
buildTab('plan', 'plan'),
|
||||
];
|
||||
|
||||
const installIssueScenario = () => {
|
||||
useUIStore.setState({
|
||||
contextPanelByDirectory: {
|
||||
[DIRECTORY]: {
|
||||
isOpen: true,
|
||||
expanded: false,
|
||||
tabs: issueScenarioTabs,
|
||||
activeTabId: sessionChatTabs[0].id,
|
||||
widthByMode: {},
|
||||
touchedAt: Date.now(),
|
||||
},
|
||||
} as never,
|
||||
});
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
installWindowLocation();
|
||||
resetEmbeddedSessionChatCache();
|
||||
useUIStore.setState({ contextPanelByDirectory: {}, contextRailOrder: [] });
|
||||
installIssueScenario();
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
Object.defineProperty(globalThis, 'window', {
|
||||
configurable: true,
|
||||
value: originalWindow,
|
||||
});
|
||||
});
|
||||
|
||||
describe('issue #2815 active-only chat iframe source guard', () => {
|
||||
test('does not map persisted chat tabs to iframe elements', () => {
|
||||
expect(contextPanelSource).not.toContain('{chatTabs.map((tab) => {');
|
||||
});
|
||||
|
||||
test('renders the iframe only when an active chat has a session and URL', () => {
|
||||
const start = contextPanelSource.indexOf('{activeChatTab && activeChatSessionID && activeChatSrc ? (');
|
||||
expect(start).toBeGreaterThan(-1);
|
||||
const end = contextPanelSource.indexOf(') : null}', start);
|
||||
expect(end).toBeGreaterThan(start);
|
||||
const block = contextPanelSource.slice(start, end);
|
||||
|
||||
expect(block).toContain('<iframe');
|
||||
expect(block).toContain('key={activeChatTab.id}');
|
||||
expect(block).toContain('src={activeChatSrc}');
|
||||
expect(block).not.toContain("'block' : 'hidden'");
|
||||
});
|
||||
|
||||
test('does not select a chat iframe when the context panel is closed', () => {
|
||||
expect(contextPanelSource).toContain(
|
||||
"const activeChatTabID = isOpen && activeTab?.mode === 'chat' ? activeTab.id : null;",
|
||||
);
|
||||
expect(contextPanelSource).toContain(
|
||||
"const activeChatSessionID = isOpen && activeTab?.mode === 'chat'",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('issue #2815 persisted scenario', () => {
|
||||
test('keeps all tab records but selects one chat for mounting', () => {
|
||||
const panel = useUIStore.getState().contextPanelByDirectory[DIRECTORY];
|
||||
const chatTabs = panel.tabs.filter((tab) => tab.mode === 'chat');
|
||||
const activeTab = getActiveEmbeddedSessionChatTab(chatTabs, panel.activeTabId);
|
||||
|
||||
expect(panel.tabs).toHaveLength(11);
|
||||
expect(chatTabs).toHaveLength(8);
|
||||
expect(activeTab?.id).toBe(sessionChatTabs[0].id);
|
||||
});
|
||||
|
||||
test('produces one live embedded URL for eight persisted chats', () => {
|
||||
const panel = useUIStore.getState().contextPanelByDirectory[DIRECTORY];
|
||||
const chatTabs = panel.tabs.filter((tab) => tab.mode === 'chat');
|
||||
const activeTab = getActiveEmbeddedSessionChatTab(chatTabs, panel.activeTabId);
|
||||
const frames = activeTab ? [buildEmbeddedSessionChatURL('ses_1', DIRECTORY, activeTab.readOnly, {
|
||||
mode: 'system',
|
||||
lightThemeId: 'light',
|
||||
darkThemeId: 'dark',
|
||||
currentTheme: getDefaultTheme(true),
|
||||
})] : [];
|
||||
|
||||
expect(frames).toHaveLength(1);
|
||||
const url = new URL(frames[0]);
|
||||
expect(url.searchParams.get('ocPanel')).toBe('session-chat');
|
||||
expect(url.searchParams.get('sessionId')).toBe('ses_1');
|
||||
expect(url.searchParams.get('readOnly')).toBe('1');
|
||||
});
|
||||
|
||||
test('selects another single chat after a tab switch', () => {
|
||||
useUIStore.getState().setActiveContextPanelTab(DIRECTORY, sessionChatTabs[6].id);
|
||||
|
||||
const panel = useUIStore.getState().contextPanelByDirectory[DIRECTORY];
|
||||
const chatTabs = panel.tabs.filter((tab) => tab.mode === 'chat');
|
||||
const activeTab = getActiveEmbeddedSessionChatTab(chatTabs, panel.activeTabId);
|
||||
|
||||
expect(activeTab?.id).toBe(sessionChatTabs[6].id);
|
||||
expect(chatTabs.filter((tab) => tab.id === activeTab?.id)).toHaveLength(1);
|
||||
});
|
||||
|
||||
test('selects no chat after the panel closes', () => {
|
||||
useUIStore.getState().closeContextPanel(DIRECTORY);
|
||||
|
||||
const panel = useUIStore.getState().contextPanelByDirectory[DIRECTORY];
|
||||
const chatTabs = panel.tabs.filter((tab) => tab.mode === 'chat');
|
||||
const activeTabID = panel.isOpen ? panel.activeTabId : null;
|
||||
|
||||
expect(panel.isOpen).toBe(false);
|
||||
expect(getActiveEmbeddedSessionChatTab(chatTabs, activeTabID)).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
EMBEDDED_RUNTIME_BOOTSTRAP_REQUEST,
|
||||
EMBEDDED_RUNTIME_BOOTSTRAP_RESPONSE,
|
||||
getOrCreateEmbeddedSessionChatURL,
|
||||
getActiveEmbeddedSessionChatTab,
|
||||
getEmbeddedSessionChatOriginSessionId,
|
||||
isEmbeddedSessionChat,
|
||||
requestEmbeddedSessionRuntimeBootstrap,
|
||||
@@ -145,6 +146,22 @@ describe('embedded session chat URL', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('active embedded session chat', () => {
|
||||
const tabs = Array.from({ length: 8 }, (_, index) => ({
|
||||
id: `chat-${index + 1}`,
|
||||
sessionID: `ses_${index + 1}`,
|
||||
}));
|
||||
|
||||
test('selects one tab from persisted chat tabs', () => {
|
||||
expect(getActiveEmbeddedSessionChatTab(tabs, 'chat-5')).toEqual(tabs[4]);
|
||||
});
|
||||
|
||||
test('selects no tab when a chat is not active', () => {
|
||||
expect(getActiveEmbeddedSessionChatTab(tabs, null)).toBeNull();
|
||||
expect(getActiveEmbeddedSessionChatTab(tabs, 'missing-chat')).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('isEmbeddedSessionChat', () => {
|
||||
test('is true only for the session-chat panel search param', () => {
|
||||
installWindowLocation('http://127.0.0.1:5173/app?ocPanel=session-chat&sessionId=ses_1');
|
||||
|
||||
@@ -158,6 +158,17 @@ export const getOrCreateEmbeddedSessionChatURL = (
|
||||
return src;
|
||||
};
|
||||
|
||||
export const getActiveEmbeddedSessionChatTab = <T extends { id: string }>(
|
||||
tabs: T[],
|
||||
activeTabID: string | null,
|
||||
): T | null => {
|
||||
if (!activeTabID) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return tabs.find((tab) => tab.id === activeTabID) ?? null;
|
||||
};
|
||||
|
||||
/**
|
||||
* True when the current document is the embedded session-chat iframe
|
||||
* (`?ocPanel=session-chat`). Used to distinguish the embedded iframe from
|
||||
|
||||
@@ -44,11 +44,13 @@ the `openContext*` actions in `useUIStore`.
|
||||
|
||||
- Opening a surface must never require a control outside the rail, the
|
||||
command palette, or an in-content link.
|
||||
- Multi-instance and session-holding surfaces (file/editor, chat, diff,
|
||||
browser, terminal) are keep-alive panes in `ContextPanel.tsx`: switching
|
||||
- Multi-instance and session-holding surfaces (file/editor, diff, browser,
|
||||
terminal) are keep-alive panes in `ContextPanel.tsx`. Switching these
|
||||
surfaces must not reset their state (open tabs, xterm session, scroll
|
||||
positions). Singleton surfaces (git, pr, notes, plan, context) and preview
|
||||
tabs intentionally remount on switch and must restore themselves from
|
||||
their stores/snapshots instead.
|
||||
positions). Chat tab records stay open, but only the active chat iframe is
|
||||
mounted while the panel is open. A selected chat restores its state from
|
||||
the session stores. A closed panel mounts no chat iframe.
|
||||
Singleton surfaces (git, pr, notes, plan, context) and preview tabs remount
|
||||
on switch. These surfaces must restore their state from stores or snapshots.
|
||||
- Runtime scope: desktop/web `MainLayout` only. VS Code and the dedicated
|
||||
mobile shell have their own layouts and do not consume this registry.
|
||||
|
||||
Reference in New Issue
Block a user