Skip to content

Commit 05d99f8

Browse files
fix: chat loading-state model placeholder (#8431)
Signed-off-by: sunilkumarvalmiki <g.sunilkumarvalmiki@gmail.com> Co-authored-by: jh-block <jhugo@block.xyz>
1 parent fd61058 commit 05d99f8

5 files changed

Lines changed: 185 additions & 26 deletions

File tree

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
import { describe, it, expect, vi, beforeEach } from 'vitest';
2+
import { render, type RenderOptions, screen } from '@testing-library/react';
3+
import ModelsBottomBar from './ModelsBottomBar';
4+
import { IntlTestWrapper } from '../../../../i18n/test-utils';
5+
6+
const renderWithIntl = (ui: React.ReactElement, options?: RenderOptions) =>
7+
render(ui, { wrapper: IntlTestWrapper, ...options });
8+
9+
const createDropdownRef = (): React.RefObject<HTMLDivElement> =>
10+
({ current: document.createElement('div') }) as React.RefObject<HTMLDivElement>;
11+
12+
let mockCurrentModel: string | null = 'config-model';
13+
let mockCurrentProvider: string | null = 'config-provider';
14+
const mockGetProviders = vi.fn();
15+
const mockOnModelChanged = vi.fn();
16+
17+
vi.mock('../../../ModelAndProviderContext', () => ({
18+
useModelAndProvider: () => ({
19+
currentModel: mockCurrentModel,
20+
currentProvider: mockCurrentProvider,
21+
}),
22+
}));
23+
24+
vi.mock('../../../ConfigContext', () => ({
25+
useConfig: () => ({
26+
getProviders: mockGetProviders,
27+
}),
28+
}));
29+
30+
vi.mock('../modelInterface', () => ({
31+
getProviderMetadata: vi.fn().mockResolvedValue({ display_name: 'Config Provider' }),
32+
}));
33+
34+
vi.mock('../predefinedModelsUtils', () => ({
35+
getModelDisplayName: (model: string) => `Display ${model}`,
36+
}));
37+
38+
vi.mock('../../../bottom_menu/BottomMenuAlertPopover', () => ({
39+
default: () => null,
40+
}));
41+
42+
vi.mock('../../../ui/dropdown-menu', () => ({
43+
DropdownMenu: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
44+
DropdownMenuTrigger: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
45+
DropdownMenuContent: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
46+
DropdownMenuItem: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
47+
}));
48+
49+
vi.mock('../../localInference/ModelSettingsPanel', () => ({
50+
ModelSettingsPanel: () => null,
51+
}));
52+
53+
vi.mock('../../../ui/scroll-area', () => ({
54+
ScrollArea: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
55+
}));
56+
57+
describe('ModelsBottomBar', () => {
58+
beforeEach(() => {
59+
vi.clearAllMocks();
60+
mockCurrentModel = 'config-model';
61+
mockCurrentProvider = 'config-provider';
62+
mockGetProviders.mockResolvedValue([]);
63+
});
64+
65+
it('shows a loading placeholder while the active session model is still loading', async () => {
66+
renderWithIntl(
67+
<ModelsBottomBar
68+
sessionId="session-123"
69+
dropdownRef={createDropdownRef()}
70+
setView={vi.fn()}
71+
onModelChanged={mockOnModelChanged}
72+
sessionLoaded={false}
73+
/>
74+
);
75+
76+
expect(screen.getByTestId('model-loading-state')).toHaveTextContent('Loading model...');
77+
});
78+
79+
it('shows the active session model once the session has loaded', async () => {
80+
renderWithIntl(
81+
<ModelsBottomBar
82+
sessionId="session-123"
83+
dropdownRef={createDropdownRef()}
84+
setView={vi.fn()}
85+
sessionModel="session-model"
86+
sessionProvider="session-provider"
87+
onModelChanged={mockOnModelChanged}
88+
sessionLoaded={true}
89+
/>
90+
);
91+
92+
expect(screen.getByText('session-model')).toBeInTheDocument();
93+
expect(screen.queryByTestId('model-loading-state')).not.toBeInTheDocument();
94+
});
95+
96+
it('shows the configured model when there is no active session', async () => {
97+
renderWithIntl(
98+
<ModelsBottomBar
99+
sessionId={null}
100+
dropdownRef={createDropdownRef()}
101+
setView={vi.fn()}
102+
onModelChanged={mockOnModelChanged}
103+
/>
104+
);
105+
106+
expect(screen.getByText('config-model')).toBeInTheDocument();
107+
expect(screen.queryByTestId('model-loading-state')).not.toBeInTheDocument();
108+
});
109+
});

ui/desktop/src/components/settings/models/bottom_bar/ModelsBottomBar.tsx

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Sliders, Bot, Settings } from 'lucide-react';
1+
import { Sliders, Bot, LoaderCircle, Settings } from 'lucide-react';
22
import React, { useEffect, useState } from 'react';
33
import { useModelAndProvider } from '../../../ModelAndProviderContext';
44
import { SwitchModelModal } from '../subcomponents/SwitchModelModal';
@@ -26,6 +26,10 @@ const i18n = defineMessages({
2626
id: 'modelsBottomBar.currentModel',
2727
defaultMessage: 'Current model',
2828
},
29+
loadingModel: {
30+
id: 'modelsBottomBar.loadingModel',
31+
defaultMessage: 'Loading model...',
32+
},
2933
changeModel: {
3034
id: 'modelsBottomBar.changeModel',
3135
defaultMessage: 'Change Model',
@@ -73,10 +77,13 @@ export default function ModelsBottomBar({
7377
const [isLocalModelSettingsOpen, setIsLocalModelSettingsOpen] = useState(false);
7478
const [providerDefaultModel, setProviderDefaultModel] = useState<string | null>(null);
7579

76-
// Hide label while session data is still being fetched (avoids flashing
77-
// the config default before the session's actual model arrives).
78-
const isModelLoading = sessionId && !sessionLoaded;
80+
// Show a visible loading placeholder while session metadata is still being fetched,
81+
// rather than flashing the config default or leaving the footer blank.
82+
const isModelLoading = Boolean(sessionId && !sessionLoaded);
7983
const displayModel = currentModel || providerDefaultModel || displayModelName;
84+
const loadingModelLabel = intl.formatMessage(i18n.loadingModel);
85+
const triggerLabel = isModelLoading ? loadingModelLabel : displayModel;
86+
const menuModelLabel = isModelLoading ? loadingModelLabel : displayModelName;
8087

8188
useEffect(() => {
8289
if (!currentProvider) return;
@@ -121,16 +128,24 @@ export default function ModelsBottomBar({
121128
<DropdownMenuTrigger className="flex items-center hover:cursor-pointer max-w-[180px] md:max-w-[200px] lg:max-w-[380px] min-w-0 text-text-primary/70 hover:text-text-primary transition-colors">
122129
<div className="flex items-center truncate max-w-[130px] md:max-w-[200px] lg:max-w-[360px] min-w-0">
123130
<Bot className="mr-1 h-4 w-4 flex-shrink-0" />
124-
<span className={`truncate text-xs${isModelLoading ? ' opacity-0' : ''}`}>
125-
{displayModel}
126-
</span>
131+
{isModelLoading ? (
132+
<span
133+
data-testid="model-loading-state"
134+
className="inline-flex items-center gap-1 truncate text-xs"
135+
>
136+
<LoaderCircle className="h-3 w-3 animate-spin flex-shrink-0" />
137+
<span className="truncate">{triggerLabel}</span>
138+
</span>
139+
) : (
140+
<span className="truncate text-xs">{triggerLabel}</span>
141+
)}
127142
</div>
128143
</DropdownMenuTrigger>
129144
<DropdownMenuContent side="top" align="center" className="w-64 text-sm">
130145
<h6 className="text-xs text-text-primary mt-2 ml-2">{intl.formatMessage(i18n.currentModel)}</h6>
131146
<p className="flex items-center justify-between text-sm mx-2 pb-2 border-b mb-2">
132-
{displayModelName}
133-
{displayProvider && ` — ${displayProvider}`}
147+
{menuModelLabel}
148+
{!isModelLoading && displayProvider && ` — ${displayProvider}`}
134149
</p>
135150
<DropdownMenuItem onClick={() => setIsAddModelModalOpen(true)}>
136151
<span>{intl.formatMessage(i18n.changeModel)}</span>

ui/desktop/src/i18n/messages/en.json

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2315,6 +2315,9 @@
23152315
"modelsBottomBar.currentModel": {
23162316
"defaultMessage": "Current model"
23172317
},
2318+
"modelsBottomBar.loadingModel": {
2319+
"defaultMessage": "Loading model..."
2320+
},
23182321
"modelsBottomBar.localModelSettings": {
23192322
"defaultMessage": "Local Model Settings"
23202323
},

ui/desktop/tests/e2e/fixtures.ts

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
import { test as base, Page, Browser, chromium } from '@playwright/test';
2-
import { spawn, ChildProcess } from 'child_process';
2+
import { exec, spawn, ChildProcess } from 'child_process';
33
import { join } from 'path';
44
import { promisify } from 'util';
55

6-
const execAsync = promisify(require('child_process').exec);
6+
const execAsync = promisify(exec);
77

88
type GooseTestFixtures = {
99
goosePage: Page;
@@ -28,7 +28,8 @@ type GooseTestFixtures = {
2828
*/
2929
export const test = base.extend<GooseTestFixtures>({
3030
// Test-scoped fixture: launches a fresh Electron app for each test
31-
goosePage: async ({}, use, testInfo) => {
31+
goosePage: async ({ browserName }, providePage, testInfo) => {
32+
void browserName;
3233
console.log(`Launching fresh Electron app for test: ${testInfo.title}`);
3334

3435
let appProcess: ChildProcess | null = null;
@@ -80,8 +81,9 @@ export const test = base.extend<GooseTestFixtures>({
8081
console.log(`Connected to Electron app on attempt ${attempt} (~${(attempt * retryDelay) / 1000}s)`);
8182
break;
8283
} catch (error) {
84+
const errorMessage = error instanceof Error ? error.message : String(error);
8385
if (attempt === maxRetries) {
84-
throw new Error(`Failed to connect to Electron app after ${maxRetries} attempts (${(maxRetries * retryDelay) / 1000}s). Last error: ${error.message}`);
86+
throw new Error(`Failed to connect to Electron app after ${maxRetries} attempts (${(maxRetries * retryDelay) / 1000}s). Last error: ${errorMessage}`);
8587
}
8688
// Wait before next retry
8789
await new Promise(resolve => setTimeout(resolve, retryDelay));
@@ -92,26 +94,28 @@ export const test = base.extend<GooseTestFixtures>({
9294
throw new Error('Browser connection failed unexpectedly');
9395
}
9496

95-
// Get the electron app context and first page
96-
const contexts = browser.contexts();
97-
if (contexts.length === 0) {
98-
throw new Error('No browser contexts found');
97+
// Wait for Electron to create its first window after the CDP endpoint is up.
98+
let page: Page | null = null;
99+
for (let attempt = 1; attempt <= 100; attempt++) {
100+
const contexts = browser.contexts();
101+
page = contexts.flatMap((context) => context.pages())[0] ?? null;
102+
if (page) {
103+
break;
104+
}
105+
await new Promise((resolve) => setTimeout(resolve, 100));
99106
}
100107

101-
const pages = contexts[0].pages();
102-
if (pages.length === 0) {
108+
if (!page) {
103109
throw new Error('No windows/pages found');
104110
}
105111

106-
const page = pages[0];
107-
108112
// Wait for page to be ready
109113
await page.waitForLoadState('domcontentloaded');
110114

111115
// Try to wait for networkidle
112116
try {
113117
await page.waitForLoadState('networkidle', { timeout: 10000 });
114-
} catch (error) {
118+
} catch {
115119
console.log('NetworkIdle timeout (likely due to MCP activity), continuing...');
116120
}
117121

@@ -124,7 +128,7 @@ export const test = base.extend<GooseTestFixtures>({
124128
console.log('App ready, starting test...');
125129

126130
// Provide the page to the test
127-
await use(page);
131+
await providePage(page);
128132

129133
} finally {
130134
console.log('Cleaning up Electron app for this test...');
@@ -146,19 +150,23 @@ export const test = base.extend<GooseTestFixtures>({
146150
// First try SIGTERM for graceful shutdown
147151
process.kill(-appProcess.pid, 'SIGTERM');
148152
await new Promise(resolve => setTimeout(resolve, 2000));
149-
} catch (e) {
153+
} catch {
150154
// Process might already be dead
151155
}
152156
// Then SIGKILL if still running
153157
try {
154158
process.kill(-appProcess.pid, 'SIGKILL');
155-
} catch (e) {
159+
} catch {
156160
// Process already exited
157161
}
158162
}
159163
console.log('Cleaned up app process');
160164
} catch (error) {
161-
if (error.code !== 'ESRCH' && !error.message?.includes('No such process')) {
165+
if (
166+
error instanceof Error &&
167+
!('code' in error && error.code === 'ESRCH') &&
168+
!error.message.includes('No such process')
169+
) {
162170
console.error('Error killing app process:', error);
163171
}
164172
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { test, expect } from './fixtures';
2+
3+
test.describe('Loading State', () => {
4+
test('shows a model placeholder while creating a new chat session', async ({ goosePage }) => {
5+
await goosePage.waitForSelector('[data-testid="chat-input"]', { timeout: 30000 });
6+
7+
const chatInput = await goosePage.waitForSelector('[data-testid="chat-input"]');
8+
await chatInput.fill('Respond with the single word hello.');
9+
await chatInput.press('Enter');
10+
11+
await goosePage.waitForSelector('[data-testid="loading-indicator"]', {
12+
state: 'visible',
13+
timeout: 10000,
14+
});
15+
16+
const loadingModel = goosePage.locator('[data-testid="model-loading-state"]');
17+
await expect(loadingModel).toHaveText(/loading model/i, { timeout: 10000 });
18+
19+
await goosePage.screenshot({
20+
path: test.info().outputPath('loading-state-fresh-session.png'),
21+
fullPage: true,
22+
});
23+
});
24+
});

0 commit comments

Comments
 (0)