diff --git a/client/src/components/Chat/Messages/Content/ActivityPhaseGroup.tsx b/client/src/components/Chat/Messages/Content/ActivityPhaseGroup.tsx index 15118d14f1..315dbb886e 100644 --- a/client/src/components/Chat/Messages/Content/ActivityPhaseGroup.tsx +++ b/client/src/components/Chat/Messages/Content/ActivityPhaseGroup.tsx @@ -299,15 +299,13 @@ function LivePhaseHeader({ const sandboxStarting = useAtomValue( sandboxStartingByToolCallId(activity.pendingToolCallId ?? ''), ); - const text = - sandboxStarting && activity.pendingToolCallId != null - ? localize('com_ui_sandbox_starting') - : activity.text; + const showSandboxStartup = sandboxStarting && activity.pendingToolCallId != null; + const text = showSandboxStartup ? localize('com_ui_sandbox_starting') : activity.text; + /** Startup describes this call, not the repeated tool. Throttle its text + * and suppressed count together so neither can paint with the old value. */ + const comboCount = showSandboxStartup ? 1 : activity.comboCount; const { source } = activity; - const line = useMemo( - () => ({ text, source, comboCount: activity.comboCount }), - [text, source, activity.comboCount], - ); + const line = useMemo(() => ({ text, source, comboCount }), [text, source, comboCount]); const previewRef = useRef(null); /** The full width the line may occupy, which is the flex track rather than * the label box: the box shrinks to its text whenever the multiplier rides diff --git a/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx index 202e4980f8..4e38738b86 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx @@ -1621,7 +1621,9 @@ describe('ContentParts — live activity fold', () => { expect(screen.getByTestId('activity-phase-announcer')).toHaveTextContent( 'Reading the lens file', ); - expect(liveHeader()).toHaveAccessibleName('Querying ×2'); + /** No multiplier: both calls named their own work, so the line is a + * sentence about the newest one rather than the tool's label. */ + expect(liveHeader()).toHaveAccessibleName('Querying'); }); it('leaves a span of legacy Assistants calls, which the header cannot name, unfolded', () => { diff --git a/client/src/components/Chat/Messages/Content/__tests__/LiveParity.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/LiveParity.test.tsx index 255644c95a..f40763a1b2 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/LiveParity.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/LiveParity.test.tsx @@ -201,9 +201,18 @@ describe('live combo aggregation', () => { for (const intent of ['Checking', 'Checking the', 'Checking the last file']) { parts[parts.length - 1] = toPart({ name: 'lookup', args: { intent } }, 'tail'); readFirst.mockClear(); - expect(activity(parts).comboCount).toBe(1024); + /** A tail that names its own work hides the count, but the span pass + * still reaches the head, and must do so exactly once per delta. */ + expect(activity(parts).comboCount).toBe(1); expect(readFirst).toHaveBeenCalledTimes(1); } + + /** The same 1,024 parts under a generic tail: the whole suffix counts, on + * the same single prefix read. */ + parts[parts.length - 1] = toPart({ name: 'lookup', output: 'ok' }, 'tail'); + readFirst.mockClear(); + expect(activity(parts).comboCount).toBe(1024); + expect(readFirst).toHaveBeenCalledTimes(1); }); it('counts only the suffix, ignoring descriptive metadata and sparse slots', () => { @@ -394,14 +403,10 @@ describe('live fold parity with the cards it hides', () => { it('changes the multiplier with the throttled status line', () => { jest.useFakeTimers(); - const first = toPart( - { name: 'create_file', args: '{"intent":"Creating the first file"}', output: '' }, - 'first', - ); - const second = toPart( - { name: 'create_file', args: '{"intent":"Creating the second file"}', output: '' }, - 'second', - ); + /** Generic lines, because only those carry a count: the multiplier has to + * arrive with the line it belongs to, not a paint ahead of it. */ + const first = toPart({ name: 'create_file', output: 'created' }, 'first'); + const second = toPart({ name: 'create_file', output: '' }, 'second'); const view = mount([first], undefined, true); view.rerender( @@ -421,11 +426,59 @@ describe('live fold parity with the cards it hides', () => { ); const header = within(screen.getByTestId('activity-phase-card')).getAllByRole('button')[0]; - expect(header).toHaveAccessibleName('Creating the first file'); + expect(header).toHaveAccessibleName('Ran Create File'); act(() => { jest.advanceTimersByTime(500); }); - expect(header).toHaveAccessibleName('Creating the second file ×2'); + expect(header).toHaveAccessibleName('Running Create File ×2'); + }); + + it('drops the multiplier as soon as the call names its own work', () => { + /** A count modifies the tool's name. Once the line is a sentence about + * this call, `×2` reads as a claim about the sentence. */ + const view = mount( + [ + toPart({ name: 'create_file', output: 'created' }, 'first'), + toPart({ name: 'create_file', args: '{"intent":"Creating the second file"}' }, 'second'), + ], + undefined, + true, + ); + const header = within(screen.getByTestId('activity-phase-card')).getAllByRole('button')[0]; + + expect(header).toHaveAccessibleName('Creating the second file'); + expect(screen.queryByTestId('live-phase-combo')).toBeNull(); + + /** The same pair without the intent still counts, so the suppression is + * the line's doing and not a lost count. */ + view.unmount(); + mount( + [ + toPart({ name: 'create_file', output: 'created' }, 'first'), + toPart({ name: 'create_file', output: '' }, 'second'), + ], + undefined, + true, + ); + expect(screen.getByTestId('live-phase-combo')).toHaveTextContent('×2'); + }); + + it('drops the multiplier on a line that reports how the call ended', () => { + mount( + [ + toPart({ name: 'lookup', output: 'rows' }, 'first'), + toPart({ name: 'lookup', output: 'rows', runStepStatus: 'failed' }, 'second'), + ], + undefined, + true, + ); + const header = within(screen.getByTestId('activity-phase-card')).getAllByRole('button')[0]; + + /** The span's verdict still counts the failure; the line does not count + * the tool, because "Failed lookup ×2" would blame both calls. */ + expect(header).toHaveAccessibleName(/^Failed: lookup/); + expect(screen.queryByTestId('live-phase-combo')).toBeNull(); + expect(screen.getByTestId('live-phase-outcome')).toHaveTextContent('1 failed'); }); it('resets the multiplier across an agent handoff', () => { @@ -933,7 +986,12 @@ describe('live activity hardening transitions', () => { function SandboxEvent() { const setStarting = useSetAtom(sandboxStartingByToolCallId('sandbox-call')); - return ; + return ( + <> + + + + ); } it.each([ @@ -948,20 +1006,83 @@ describe('live activity hardening transitions', () => { * reads the same sandbox signal instead, and stays one card throughout. */ jest.useFakeTimers(); const call = { name, args, output: '' }; - const view = render(frame([toPart(call, 'sandbox-call')], )); + const earlier = toPart({ ...call, output: 'ok' }, 'earlier'); + const view = render(frame([earlier, toPart(call, 'sandbox-call')], )); const card = screen.getByTestId('activity-phase-card'); + const header = within(card).getByRole('button'); expect(screen.queryByTestId('tool-call')).toBeNull(); + expect(header).toHaveAccessibleName(/×2$/); + expect(screen.getByTestId('live-phase-combo')).toHaveTextContent('×2'); + + fireEvent.click(screen.getByRole('button', { name: 'Start sandbox' })); + expect(header).toHaveAccessibleName(/×2$/); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(header).toHaveAccessibleName('Starting sandbox environment'); + expect(screen.queryByTestId('live-phase-combo')).toBeNull(); + + fireEvent.click(screen.getByRole('button', { name: 'Clear sandbox startup' })); + expect(header).toHaveAccessibleName('Starting sandbox environment'); + expect(screen.queryByTestId('live-phase-combo')).toBeNull(); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(header).toHaveAccessibleName(/×2$/); + expect(screen.getByTestId('live-phase-combo')).toHaveTextContent('×2'); fireEvent.click(screen.getByRole('button', { name: 'Start sandbox' })); act(() => { jest.advanceTimersByTime(500); }); - expect(card).toHaveTextContent('Starting sandbox'); + expect(header).toHaveAccessibleName('Starting sandbox environment'); + /** Output can arrive before the transient startup flag is cleared. */ + view.rerender( + frame([ + earlier, + toPart({ ...call, output: 'ok', runStepStatus: 'completed' }, 'sandbox-call'), + ]), + ); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(screen.getByTestId('activity-phase-card')).toBe(card); + expect(header).toHaveAccessibleName(/^Ran .* ×2$/); + expect(screen.getByTestId('live-phase-combo')).toHaveTextContent('×2'); + }); + + it('keeps startup and intent labels uncounted, then counts a new generic call', () => { + jest.useFakeTimers(); + const earlier = toPart({ name: Tools.execute_code, output: 'ok' }, 'earlier'); + const pending = toPart({ name: Tools.execute_code, output: '' }, 'sandbox-call'); + const view = render(frame([earlier, pending], )); + const header = within(screen.getByTestId('activity-phase-card')).getByRole('button'); + fireEvent.click(screen.getByRole('button', { name: 'Start sandbox' })); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(header).toHaveAccessibleName('Starting sandbox environment'); + expect(screen.queryByTestId('live-phase-combo')).toBeNull(); + + const named = toPart( + { name: Tools.execute_code, args: '{"intent":"Checking the data', output: '' }, + 'sandbox-call', + ); + view.rerender(frame([earlier, named])); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(header).toHaveAccessibleName('Checking the data'); + expect(screen.queryByTestId('live-phase-combo')).toBeNull(); view.rerender( - frame([toPart({ ...call, output: 'ok', runStepStatus: 'completed' }, 'sandbox-call')]), + frame([earlier, named, toPart({ name: Tools.execute_code, output: '' }, 'next-call')]), ); - expect(screen.getByTestId('activity-phase-card')).toBe(card); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(header).toHaveAccessibleName(/^Running .* ×3$/); + expect(screen.getByTestId('live-phase-combo')).toHaveTextContent('×3'); }); it('holds one card across a run of code calls whose intent is not the first key', () => { @@ -1084,7 +1205,8 @@ describe('live activity hardening transitions', () => { jest.advanceTimersByTime(500); }); const header = screen.getByRole('button'); - expect(header).toHaveAccessibleName('Checking the next file ×2'); + expect(header).toHaveAccessibleName('Checking the next file'); + expect(screen.queryByTestId('live-phase-combo')).toBeNull(); expect(header.querySelector('.absolute[aria-hidden="true"]')).toBeNull(); }); diff --git a/client/src/components/Chat/Messages/Content/live.ts b/client/src/components/Chat/Messages/Content/live.ts index 73fc3e5aa5..c79852a07c 100644 --- a/client/src/components/Chat/Messages/Content/live.ts +++ b/client/src/components/Chat/Messages/Content/live.ts @@ -34,7 +34,9 @@ export type LiveActivity = { * the content array; the header reads the same signal for this call rather * than unfolding the span to let the card say it. */ pendingToolCallId?: string; - /** Consecutive uses of the newest tool, including the current call. */ + /** Consecutive uses of the newest tool, including the current call. Above 1 + * only while the line is the tool's own generic label, which is the only + * thing a count of that tool can modify. */ comboCount: number; /** Failed and stopped calls anywhere in the span, not just the newest line. */ outcome: SpanOutcome; @@ -84,13 +86,22 @@ export function needsReader(part: TMessageContentParts | undefined): boolean { return Array.isArray(toolCall.subagent_content) && toolCall.subagent_content.some(needsReader); } +/** + * A tool's line, and whether it is the generic label rather than a line about + * this one call. `generic` is what a repeat count may modify: `Running Code ×3` + * counts runs of Code, while `Checking the PR head ×3` would claim that + * sentence happened three times — a call that names its own work already says + * which work it is doing, so the count only confuses it. + */ +type ToolLine = { text: string; generic: boolean }; + function toolCallLine( part: TMessageContentParts, toolCall: LiveToolCall, localize: Localize, serverNames: readonly string[], span: SpanSummary, -): string { +): ToolLine { const intent = getToolCallIntent(toolCall.args); const label = getToolDisplayLabel(toolCall.name ?? '', localize, serverNames); /** The verdict comes from the resolver the group header uses, so a collapsed @@ -98,30 +109,39 @@ function toolCallLine( * or a stop — whichever channel reported it. */ const meta = span.metaOf(part); if (meta?.cancelled === true) { - return localize('com_ui_cancelled'); + return { text: localize('com_ui_cancelled'), generic: false }; } if (meta?.failed === true) { /** Reads as the hidden card does: `ToolCall` uses the same template. */ const subject = intent ?? label; - return subject ? localize('com_ui_failed_subject', { 0: subject }) : localize('com_ui_failed'); + return { + text: subject ? localize('com_ui_failed_subject', { 0: subject }) : localize('com_ui_failed'), + generic: false, + }; } /** Ahead of the intent, as on `BashCall`/`ExecuteCode`: a returned handle * is not a result, and "Ran …" would turn ongoing work into a success. */ if (meta?.background != null) { - return localize( - meta.background === 'running' ? 'com_ui_background_running' : 'com_ui_background_finished', - ); + return { + text: localize( + meta.background === 'running' ? 'com_ui_background_running' : 'com_ui_background_finished', + ), + generic: false, + }; } if (intent != null) { - return intent; + return { text: intent, generic: false }; } if (!label) { - return localize('com_assistants_running_action'); + return { text: localize('com_assistants_running_action'), generic: true }; } - return localize( - meta?.hasOutput === true ? 'com_assistants_completed_function' : 'com_assistants_running_var', - { 0: label }, - ); + return { + text: localize( + meta?.hasOutput === true ? 'com_assistants_completed_function' : 'com_assistants_running_var', + { 0: label }, + ), + generic: true, + }; } /** Bounds the sentence scan on long reasoning, like the streaming peek. */ @@ -280,13 +300,17 @@ function newestLine( } const toolCall = getStandardToolCall(part); if (toolCall != null) { + const line = toolCallLine(part, toolCall, localize, serverNames, span); return { - text: toolCallLine(part, toolCall, localize, serverNames, span), + text: line.text, /** Provider ids repeat across batches, so the position is part of the * identity: a second call reusing an id is a new line, not the first * one still growing. */ source: `tool:${toolCall.id ?? ''}:${position}`, - comboCount: Math.max(1, span.trailingToolCount), + /** Counted for the generic label alone: as soon as the call names its + * own work, or reports how it ended, the count has nothing left to + * multiply and reads as a claim about that sentence. */ + comboCount: line.generic ? Math.max(1, span.trailingToolCount) : 1, ...(isAwaitingStartup(part, toolCall, span) && { pendingToolCallId: toolCall.id }), }; } @@ -298,7 +322,8 @@ function newestLine( * The newest nameable activity in a span: the last tool call's own line (its * streamed intent, else the generic text its card would show), a filled batch * label once one lands after it, or the thought streaming after both. Later parts win, so the - * header always reads as the bottom line of the list it stands for. + * header always reads as the bottom line of the list it stands for. A repeat + * count rides the generic label only, never a line that names one call. * * Runs on every streamed delta. Outcomes and the tool combo share one full-span * pass so late failures cannot disappear; the line stops at the newest nameable diff --git a/e2e/client-build.test.mjs b/e2e/client-build.test.mjs index b79d4829e6..2a7286bd01 100644 --- a/e2e/client-build.test.mjs +++ b/e2e/client-build.test.mjs @@ -14,7 +14,8 @@ const root = fileURLToPath(new URL('../', import.meta.url)); // without requiring a database or identity provider. It does not simulate an active model run. test( 'an old tab retains its bundle identity and draft across a worker update', - { timeout: 30000 }, + /** Allow setup plus both activations; individual browser waits remain bounded. */ + { timeout: 60000 }, async () => { const temporary = await mkdtemp(path.join(tmpdir(), 'librechat-builds-')); const appHtml = await readFile(path.join(root, 'client/index.html'), 'utf8'); @@ -55,7 +56,10 @@ test( window.__lcRumPush('before-bootstrap'); installRumBootstrap(window); window.fixtureVersion = ${JSON.stringify(version)}; - navigator.serviceWorker.register('/sw.js');`, + navigator.serviceWorker.register('/sw.js').then( + registration => { window.fixtureRegistration = registration; }, + error => { window.fixtureWorkerError = String(error); }, + );`, ); await build({ root: fixture, @@ -71,14 +75,25 @@ test( ); } await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); - browser = await chromium.launch({ headless: true, channel: process.env.PLAYWRIGHT_CHANNEL }); + browser = await chromium.launch({ + headless: true, + channel: process.env.PLAYWRIGHT_CHANNEL, + timeout: 10000, + }); const page = await browser.newPage(); page.setDefaultTimeout(10000); const url = `http://127.0.0.1:${server.address().port}`; await page.goto(`${url}/c/example`); await page.waitForFunction(() => window.fixtureVersion === 'A'); - await page.evaluate(() => navigator.serviceWorker.ready); - await page.waitForFunction(() => !!navigator.serviceWorker.controller); + /** `ready` has no timeout and a controller can still be activating. Finish + * A's handshake before clearing its events or requesting another worker. */ + await page.waitForFunction( + () => + window.fixtureWorkerError || + (window.fixtureRegistration?.active?.state === 'activated' && + navigator.serviceWorker.controller?.state === 'activated'), + ); + assert.equal(await page.evaluate(() => window.fixtureWorkerError), undefined); await page.getByLabel('Draft').fill('Keep my unsent text'); const firstId = await page.evaluate(() => window.__lcRumQueue[0].attributes.clientBuildId); assert.match(firstId, /^index\..+\.js$/); @@ -93,15 +108,30 @@ test( await page.evaluate(() => { window.__lcRumQueue.length = 0; + window.fixturePreviousController = navigator.serviceWorker.controller; }); serving = 'B'; - await page.evaluate(async () => { - const registration = await navigator.serviceWorker.getRegistration(); - await registration.update(); + /** `evaluate` does not bound an awaited update promise. Observe its result + * through the timed wait, and require B's activation, not a late A ping. */ + await page.evaluate(() => { + window.fixtureRegistration.update().then( + () => { + window.fixtureUpdateFinished = true; + }, + (error) => { + window.fixtureWorkerError = String(error); + }, + ); }); - await page.waitForFunction(() => - window.__lcRumQueue.some((event) => event.type === 'sw-ping'), + await page.waitForFunction( + () => + window.fixtureWorkerError || + (window.fixtureUpdateFinished && + navigator.serviceWorker.controller !== window.fixturePreviousController && + navigator.serviceWorker.controller?.state === 'activated' && + window.__lcRumQueue.some((event) => event.type === 'sw-ping')), ); + assert.equal(await page.evaluate(() => window.fixtureWorkerError), undefined); // Outlive the worker's unresponsive-client deadline to catch an unwanted navigation. await page.waitForTimeout(2000); assert.equal(await page.getByLabel('Draft').inputValue(), 'Keep my unsent text');