From a08b8643a60be4b70c57ea2471150b2d9c8a43b1 Mon Sep 17 00:00:00 2001 From: karthikmudunuri <102793643+karthikmudunuri@users.noreply.github.com> Date: Tue, 7 Apr 2026 23:22:12 +0530 Subject: [PATCH] fix(ui): prevent sidebar instance reordering and new conversation flicker Two separate bugs caused a jarring experience when switching between instances or creating new conversations in the chat sidebar: 1. Sidebar instances reordered on every interaction because the old sortAgentGroupsForFocus() moved the focused agent to the top and fetchAgents re-ran on every URL change, replacing state with a potentially different order. Fixed by always sorting alphabetically in the Sidebar component and stabilising fetchAgents with refs so it only runs on mount. 2. Creating a new conversation caused a visible flicker because React Router had two separate Route elements for the chat page (c/:name? and c/:name/:conversationId). Navigating from one to the other unmounted and remounted ChatPage, resetting all state. Fixed by collapsing both routes into a single catch-all (c/*) and parsing the name/conversationId from the wildcard param. Additional fixes: - handleNewConversation now updates local state directly instead of calling fetchAgents, eliminating a full re-fetch cycle. - applyConversationUpdate only updates selectedConversation if it matches the currently viewed conversation, preventing stale WebSocket events from overwriting the selection. --- ui/src/App.test.tsx | 7 +- ui/src/App.tsx | 5 +- ui/src/components/acp/sidebar.test.tsx | 49 ++++++- ui/src/components/acp/sidebar.tsx | 14 +- ui/src/lib/urls.ts | 5 + ui/src/pages/chat.test.tsx | 174 +++++++++++++++++++++++-- ui/src/pages/chat.tsx | 75 +++++++---- 7 files changed, 274 insertions(+), 55 deletions(-) diff --git a/ui/src/App.test.tsx b/ui/src/App.test.tsx index b95fa682..c4fcd020 100644 --- a/ui/src/App.test.tsx +++ b/ui/src/App.test.tsx @@ -30,7 +30,7 @@ function renderAtRoute(path: string) { Chat Page} /> Create Page} /> Terminal Page} /> - Chat Page} /> + Chat Page} /> @@ -54,6 +54,11 @@ describe('App routing', () => { expect(screen.getByTestId('chat-page')).toBeDefined(); }); + it('renders ChatPage at /c/some-name/some-conversation (same route, no remount)', () => { + renderAtRoute('/c/some-name/some-conversation'); + expect(screen.getByTestId('chat-page')).toBeDefined(); + }); + it('renders TerminalPage at /terminal/my-spritz', () => { renderAtRoute('/terminal/my-spritz'); expect(screen.getByTestId('terminal-page')).toBeDefined(); diff --git a/ui/src/App.tsx b/ui/src/App.tsx index 92fdd2a1..e7bf49a1 100644 --- a/ui/src/App.tsx +++ b/ui/src/App.tsx @@ -6,7 +6,7 @@ import { Layout } from '@/components/layout'; import { ChatPage } from '@/pages/chat'; import { CreatePage } from '@/pages/create'; import { TerminalPage } from '@/pages/terminal'; -import { chatConversationRoutePath, chatRoutePath } from '@/lib/urls'; +import { chatCatchAllRoutePath } from '@/lib/urls'; export function App() { return ( @@ -19,8 +19,7 @@ export function App() { } /> } /> } /> - } /> - } /> + } /> diff --git a/ui/src/components/acp/sidebar.test.tsx b/ui/src/components/acp/sidebar.test.tsx index f20beea7..0450e690 100644 --- a/ui/src/components/acp/sidebar.test.tsx +++ b/ui/src/components/acp/sidebar.test.tsx @@ -86,18 +86,18 @@ describe('Sidebar', () => { ); }); - it('moves the focused agent to the top, highlights it, and collapses other agents', () => { + it('keeps agents in alphabetical order regardless of focus and collapses non-focused agents', () => { renderWithProviders( { const agentHeaders = screen.getAllByRole('button', { name: / conversations$/i, }); - expect(agentHeaders[0]?.getAttribute('aria-label')).toBe('beta conversations'); + // Alpha comes first alphabetically, even though beta is focused + expect(agentHeaders[0]?.getAttribute('aria-label')).toBe('alpha conversations'); + expect(agentHeaders[1]?.getAttribute('aria-label')).toBe('beta conversations'); expect( screen .getByRole('button', { name: 'beta conversations' }) @@ -131,6 +133,41 @@ describe('Sidebar', () => { ).toBe('false'); }); + it('sorts agents alphabetically even when passed in reverse order', () => { + renderWithProviders( + , + ); + + const agentHeaders = screen.getAllByRole('button', { + name: / conversations$/i, + }); + expect(agentHeaders[0]?.getAttribute('aria-label')).toBe('alpha conversations'); + expect(agentHeaders[1]?.getAttribute('aria-label')).toBe('mike conversations'); + expect(agentHeaders[2]?.getAttribute('aria-label')).toBe('zulu conversations'); + }); + it('shows a selected optimistic provisioning conversation for a focused route before the agent is discoverable', () => { renderWithProviders( void; } -function sortAgentGroupsForFocus(groups: AgentGroup[], focusedSpritzName?: string | null): AgentGroup[] { - if (!focusedSpritzName) return groups; - return [...groups].sort((left, right) => { - const leftFocused = left.spritz.metadata.name === focusedSpritzName; - const rightFocused = right.spritz.metadata.name === focusedSpritzName; - if (leftFocused === rightFocused) return 0; - return leftFocused ? -1 : 1; - }); -} - export function Sidebar({ agents, selectedConversationId, @@ -72,7 +62,9 @@ export function Sidebar({ mobileOpen, onCloseMobile, }: SidebarProps) { - const orderedAgents = sortAgentGroupsForFocus(agents, focusedSpritzName); + const orderedAgents = [...agents].sort((a, b) => + a.spritz.metadata.name.localeCompare(b.spritz.metadata.name), + ); const firstAgentName = orderedAgents.length > 0 ? orderedAgents[0].spritz.metadata.name : null; const focusMode = Boolean(focusedSpritzName); const focusedAgentInList = Boolean( diff --git a/ui/src/lib/urls.ts b/ui/src/lib/urls.ts index e9fb0f09..cfabc63e 100644 --- a/ui/src/lib/urls.ts +++ b/ui/src/lib/urls.ts @@ -134,6 +134,11 @@ export function chatConversationRoutePath(): string { return `${chatRoutePrefixSegment()}/:name/:conversationId`; } +/** Returns a single catch-all React Router path for all chat sub-routes. */ +export function chatCatchAllRoutePath(): string { + return `${chatRoutePrefixSegment()}/*`; +} + export const hideRepoInputs = parseBoolean(config.repoDefaults.hideInputs, false); export const defaultRepoUrl = String(config.repoDefaults.url || '').trim(); export const defaultRepoBranch = String(config.repoDefaults.branch || '').trim(); diff --git a/ui/src/pages/chat.test.tsx b/ui/src/pages/chat.test.tsx index fe640ff7..1ee48b9d 100644 --- a/ui/src/pages/chat.test.tsx +++ b/ui/src/pages/chat.test.tsx @@ -454,8 +454,7 @@ function renderChatPage(route: string, rawConfig?: RawSpritzConfig) { - } /> - } /> + } /> } /> @@ -614,7 +613,7 @@ describe('ChatPage draft persistence', () => { - } /> + } /> @@ -665,7 +664,7 @@ describe('ChatPage draft persistence', () => { - } /> + } /> @@ -750,7 +749,7 @@ describe('ChatPage draft persistence', () => { - } /> + } /> @@ -785,7 +784,7 @@ describe('ChatPage draft persistence', () => { - } /> + } /> @@ -1104,7 +1103,7 @@ describe('ChatPage draft persistence', () => { - } /> + } /> @@ -1130,7 +1129,7 @@ describe('ChatPage draft persistence', () => { - } /> + } /> @@ -1338,3 +1337,162 @@ describe('ChatPage draft persistence', () => { ); }); }); + +describe('ChatPage instance ordering', () => { + beforeEach(() => { + vi.useRealTimers(); + Object.defineProperty(globalThis, 'localStorage', { value: createMockStorage(), writable: true }); + Object.defineProperty(globalThis, 'sessionStorage', { value: createMockStorage(), writable: true }); + Object.defineProperty(window.HTMLElement.prototype, 'scrollIntoView', { + value: vi.fn(), + writable: true, + }); + requestMock.mockReset(); + sendPromptMock.mockReset(); + showNoticeMock.mockReset(); + refreshAuthTokenForWebSocketMock.mockClear(); + resetACPMockState(); + sendPromptMock.mockResolvedValue({}); + }); + + it('sorts agents alphabetically in sidebar regardless of API return order', async () => { + const spritzZulu = createSpritz({ metadata: { name: 'zulu-instance' } }); + const spritzAlpha = createSpritz({ metadata: { name: 'alpha-instance' } }); + const convZulu = createConversation({ + metadata: { name: 'conv-z' }, + spec: { sessionId: 'sz', title: 'Zulu conv', spritzName: 'zulu-instance' }, + }); + const convAlpha = createConversation({ + metadata: { name: 'conv-a' }, + spec: { sessionId: 'sa', title: 'Alpha conv', spritzName: 'alpha-instance' }, + }); + + requestMock.mockImplementation((path: string, options?: { method?: string }) => { + if (path === '/spritzes') { + // Return in reverse alphabetical order + return Promise.resolve({ items: [spritzZulu, spritzAlpha] }); + } + if (path === '/acp/conversations?spritz=zulu-instance') { + return Promise.resolve({ items: [convZulu] }); + } + if (path === '/acp/conversations?spritz=alpha-instance') { + return Promise.resolve({ items: [convAlpha] }); + } + if (path.endsWith('/connect-ticket') && options?.method === 'POST') { + return Promise.resolve({ + type: 'connect-ticket', + ticket: 'ticket-123', + expiresAt: '2026-03-30T12:34:56Z', + protocol: 'spritz-acp.v1', + connectPath: '/api/acp/conversations/conv-a/connect', + }); + } + return Promise.resolve({}); + }); + + renderChatPage('/c/alpha-instance/conv-a'); + + await waitFor(() => { + const order = screen.getByTestId('sidebar-agent-order'); + expect(order.textContent).toBe('alpha-instance,zulu-instance'); + }); + }); + + it('keeps agent order stable when selecting a conversation from a different instance', async () => { + const spritzA = createSpritz({ metadata: { name: 'alpha-instance' } }); + const spritzZ = createSpritz({ metadata: { name: 'zulu-instance' } }); + const convA = createConversation({ + metadata: { name: 'conv-a' }, + spec: { sessionId: 'sa', title: 'Alpha conv', spritzName: 'alpha-instance' }, + }); + const convZ = createConversation({ + metadata: { name: 'conv-z' }, + spec: { sessionId: 'sz', title: 'Zulu conv', spritzName: 'zulu-instance' }, + }); + + requestMock.mockImplementation((path: string, options?: { method?: string }) => { + if (path === '/spritzes') { + return Promise.resolve({ items: [spritzZ, spritzA] }); + } + if (path === '/acp/conversations?spritz=alpha-instance') { + return Promise.resolve({ items: [convA] }); + } + if (path === '/acp/conversations?spritz=zulu-instance') { + return Promise.resolve({ items: [convZ] }); + } + if (path.endsWith('/connect-ticket') && options?.method === 'POST') { + return Promise.resolve({ + type: 'connect-ticket', + ticket: 'ticket-123', + expiresAt: '2026-03-30T12:34:56Z', + protocol: 'spritz-acp.v1', + connectPath: '/api/acp/conversations/conv-a/connect', + }); + } + return Promise.resolve({}); + }); + + renderChatPage('/c/alpha-instance/conv-a'); + + await waitFor(() => { + expect(screen.getByTestId('sidebar-agent-order').textContent).toBe( + 'alpha-instance,zulu-instance', + ); + }); + + // Click a conversation from zulu-instance + await act(async () => { + fireEvent.click(screen.getByText('Zulu conv')); + }); + + // Agent order must remain alpha first, zulu second + expect(screen.getByTestId('sidebar-agent-order').textContent).toBe( + 'alpha-instance,zulu-instance', + ); + }); + + it('does not flicker selected conversation when creating a new one', async () => { + const spritz = createSpritz({ metadata: { name: 'covo' } }); + const existingConv = createConversation({ + metadata: { name: 'conv-existing' }, + spec: { sessionId: 'se', title: 'Existing conv', spritzName: 'covo' }, + }); + const newConv = createConversation({ + metadata: { name: 'conv-new' }, + spec: { sessionId: 'sn', title: 'New conv', spritzName: 'covo' }, + }); + + requestMock.mockImplementation((path: string, options?: { method?: string }) => { + if (path === '/spritzes') { + return Promise.resolve({ items: [spritz] }); + } + if (path === '/acp/conversations?spritz=covo') { + return Promise.resolve({ items: [existingConv] }); + } + if (path === '/acp/conversations' && options?.method === 'POST') { + return Promise.resolve(newConv); + } + if (path.endsWith('/connect-ticket') && options?.method === 'POST') { + return Promise.resolve({ + type: 'connect-ticket', + ticket: 'ticket-123', + expiresAt: '2026-03-30T12:34:56Z', + protocol: 'spritz-acp.v1', + connectPath: '/api/acp/conversations/conv-existing/connect', + }); + } + return Promise.resolve({}); + }); + + renderChatPage('/c/covo/conv-existing'); + + await waitFor(() => { + expect(screen.getByTestId('selected-conversation').textContent).toBe('conv-existing'); + }); + + // The selected conversation should never revert to conv-existing after switching + // (regression test: previously fetchAgents would re-run with stale URL params + // and reset selectedConversation back to the old one) + expect(screen.getByTestId('selected-conversation').textContent).toBe('conv-existing'); + }); +}); diff --git a/ui/src/pages/chat.tsx b/ui/src/pages/chat.tsx index e2835554..05c35d5a 100644 --- a/ui/src/pages/chat.tsx +++ b/ui/src/pages/chat.tsx @@ -58,7 +58,10 @@ function getLatestConversation(conversations: ConversationInfo[]): ConversationI } export function ChatPage() { - const { name, conversationId: urlConversationId } = useParams<{ name: string; conversationId: string }>(); + const { '*': splat } = useParams(); + const splatSegments = (splat || '').split('/').filter(Boolean); + const name = splatSegments[0] ? decodeURIComponent(splatSegments[0]) : undefined; + const urlConversationId = splatSegments[1] ? decodeURIComponent(splatSegments[1]) : undefined; const navigate = useNavigate(); const config = useConfig(); const { showNotice } = useNotice(); @@ -76,7 +79,11 @@ export function ChatPage() { const composerRef = useRef(null); const selectedConversationRef = useRef(null); const autoCreatingConversationForRef = useRef(null); + const nameRef = useRef(name); + const urlConversationIdRef = useRef(urlConversationId); + nameRef.current = name; + urlConversationIdRef.current = urlConversationId; selectedConversationRef.current = selectedConversation; const selectedSpritzName = selectedConversation?.spec?.spritzName || name || ''; @@ -89,15 +96,18 @@ export function ChatPage() { : null; const provisioningStatusLine = getProvisioningStatusLine(provisioningSpritz); - // Fetch agents and conversations + // Fetch agents and conversations. Uses refs for URL params so + // the callback identity is stable across navigations. const fetchAgents = useCallback(async (): Promise => { + const currentName = nameRef.current; + const currentUrlConversationId = urlConversationIdRef.current; try { const spritzList = await request<{ items: Spritz[] }>('/spritzes'); let items = spritzList?.items || []; - if (name && !items.some((spritz) => spritz.metadata.name === name)) { + if (currentName && !items.some((spritz) => spritz.metadata.name === currentName)) { try { - const routeSpritz = await request(`/spritzes/${encodeURIComponent(name)}`); - if (routeSpritz?.metadata?.name === name) { + const routeSpritz = await request(`/spritzes/${encodeURIComponent(currentName)}`); + if (routeSpritz?.metadata?.name === currentName) { items = [routeSpritz, ...items]; } } catch { @@ -123,13 +133,14 @@ export function ChatPage() { }), ); + groups.sort((a, b) => a.spritz.metadata.name.localeCompare(b.spritz.metadata.name)); setAgents(groups); - if (!name) { + if (!currentName) { return false; } - const routeSpritz = items.find((spritz) => spritz.metadata.name === name); + const routeSpritz = items.find((spritz) => spritz.metadata.name === currentName); if (!routeSpritz) { setSelectedConversation(null); return true; @@ -139,14 +150,14 @@ export function ChatPage() { return true; } - const group = groups.find((entry) => entry.spritz.metadata.name === name); + const group = groups.find((entry) => entry.spritz.metadata.name === currentName); if (!group) { setSelectedConversation(null); return false; } - if (urlConversationId) { - const match = group.conversations.find((conversation) => conversation.metadata.name === urlConversationId); + if (currentUrlConversationId) { + const match = group.conversations.find((conversation) => conversation.metadata.name === currentUrlConversationId); if (match) { setSelectedConversation(match); return false; @@ -156,28 +167,28 @@ export function ChatPage() { const latestConversation = getLatestConversation(group.conversations); if (latestConversation) { setSelectedConversation(latestConversation); - if (urlConversationId !== latestConversation.metadata.name) { - navigate(chatConversationPath(name, latestConversation.metadata.name), { replace: true }); + if (currentUrlConversationId !== latestConversation.metadata.name) { + navigate(chatConversationPath(currentName, latestConversation.metadata.name), { replace: true }); } return false; } - if (autoCreatingConversationForRef.current === name) { + if (autoCreatingConversationForRef.current === currentName) { return false; } - autoCreatingConversationForRef.current = name; - setCreatingConversationFor(name); + autoCreatingConversationForRef.current = currentName; + setCreatingConversationFor(currentName); try { const conv = await request('/acp/conversations', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ spritzName: name }), + body: JSON.stringify({ spritzName: currentName }), }); if (conv) { setAgents((currentGroups) => currentGroups.map((currentGroup) => - currentGroup.spritz.metadata.name === name + currentGroup.spritz.metadata.name === currentName ? { ...currentGroup, conversations: sortConversationsByRecency([...currentGroup.conversations, conv]), @@ -186,13 +197,13 @@ export function ChatPage() { ), ); setSelectedConversation(conv); - navigate(chatConversationPath(name, conv.metadata.name), { replace: true }); + navigate(chatConversationPath(currentName, conv.metadata.name), { replace: true }); } } catch (err) { showNotice(err instanceof Error ? err.message : 'Failed to start a conversation.'); } finally { autoCreatingConversationForRef.current = null; - setCreatingConversationFor((current) => (current === name ? null : current)); + setCreatingConversationFor((current) => (current === currentName ? null : current)); } return false; } catch (err) { @@ -201,11 +212,13 @@ export function ChatPage() { } finally { setLoading(false); } - }, [name, urlConversationId, navigate, showNotice]); + }, [navigate, showNotice]); + // Initial fetch — runs once on mount (fetchAgents is now stable). useEffect(() => { fetchAgents(); - }, [fetchAgents]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); useEffect(() => { if (!provisioningSpritz) { @@ -235,7 +248,7 @@ export function ChatPage() { window.clearTimeout(timerId); } }; - }, [fetchAgents, provisioningSpritz?.metadata.name]); + }, [fetchAgents, provisioningSpritz?.metadata.name]); // fetchAgents is stable (refs for URL params) const applyConversationTitle = useCallback((conversationId: string, title?: string | null) => { const normalized = String(title || '').trim(); @@ -258,7 +271,9 @@ export function ChatPage() { }, []); const applyConversationUpdate = useCallback((conversation: ConversationInfo) => { - setSelectedConversation(conversation); + setSelectedConversation((prev) => + prev && prev.metadata.name === conversation.metadata.name ? conversation : prev, + ); setAgents((prev) => prev.map((group) => { const sameSpritz = group.spritz.metadata.name === (conversation.spec?.spritzName || ''); @@ -404,9 +419,17 @@ export function ChatPage() { }); if (conv) { setSelectedConversation(conv); + setAgents((currentGroups) => + currentGroups.map((group) => + group.spritz.metadata.name === normalizedSpritzName + ? { + ...group, + conversations: sortConversationsByRecency([...group.conversations, conv]), + } + : group, + ), + ); navigate(chatConversationPath(normalizedSpritzName, conv.metadata.name), { replace: true }); - // Refresh agent list - fetchAgents(); } } catch (err) { toast.error(err instanceof Error ? err.message : 'Failed to create conversation.'); @@ -414,7 +437,7 @@ export function ChatPage() { setCreatingConversationFor((current) => (current === normalizedSpritzName ? null : current)); } }, - [creatingConversationFor, fetchAgents, navigate], + [creatingConversationFor, navigate], ); if (loading) {