mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 04:07:05 +00:00
🔀 perf: Swap the Transcript With the URL on Conversation Switch (#15054)
* ⚡ perf: Swap the Transcript With the URL on Conversation Switch Switching conversations left the PREVIOUS transcript painted under the new URL. Two things on the critical path caused it, both fixed here. `RouterProvider` commits location updates inside `React.startTransition` by default in react-router v7, and a transition keeps the outgoing tree on screen until the incoming one has fully rendered — so every millisecond the next thread took to render was time spent looking at the previous one, and React yields during that render, stretching it well past its CPU cost. Nothing here reads route data through router loaders, so the transition bought no pending UI; conversation state also still lives in Recoil, whose transition-safe reads are gated behind `_TRANSITION_SUPPORT_UNSTABLE` hooks this app does not use. `useTransitions={false}` puts the route change back in the click's own task. `navigateToConvo` also awaited `GET /api/convos/:id` before calling `navigate()`, so the route did not change until a full server round trip completed. The clicked row already carries its conversation, so the route and conversation state now change together and the refetch reconciles afterwards. The row is a list projection, so any previously fetched full record underlays it — prompt prefix, sampling params and files survive the switch, and a send during the reconcile window still carries the real settings. Measured on the built client with a 250ms conversation-fetch latency, switching between two 30-turn conversations: before cold click→url 527ms click→paint 931ms 14 stale frames (297ms) warm click→url 474ms click→paint 838ms 12 stale frames (277ms) after cold click→url ~190ms click→paint ~450ms 0 stale frames warm click→url ~280ms click→paint ~280ms 0 stale frames The warm switch now paints the new transcript in the same commit as the URL. The warm-cache message loading this depends on is untouched. Adds `e2e/benchmarks-navigation`, a react-scan benchmark that guards the result: an in-page sampler records the route and the mounted conversation once per animation frame, so a frame pairing the next URL with the previous transcript is caught directly. The react-scan harness the reasoning benchmark had inlined moves to `e2e/perf/scan.ts` and is now shared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qZJDNkyH5rgCz6KcLjseq * 🎯 fix: Resolve Sidebar Rows by Their Accessible Button in the Nav Benchmark The a11y pass on the sidebar moved the conversation row's `role="button"` and `aria-label` off the `convo-item` container and onto a real `<button>` that `ConvoLink` renders inside it. The benchmark's click helper required a single node carrying both the testid and the label, so after merging dev it found nothing and threw. Match on whichever node inside a row carries the label and let the click bubble to the container's handler, which still owns the navigation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qZJDNkyH5rgCz6KcLjseq * 🛡️ fix: Close Three Navigation Races Found in Review Codex review of the optimistic-navigation path found three real defects. Superseded reconciliations were written unconditionally. Selecting B then C before both records settled let B's response land last and restore B into conversation state while the route and transcript showed C — and sends read from that state, so a user could submit into a conversation they were no longer looking at. Navigations now claim a shared token before any await and late responses are discarded. The token is module state rather than a ref because every sidebar row mounts its own hook instance, so a ref cannot see that a click on a different row superseded this one. The first visit to a conversation installed the sidebar row as active state. That row is a projection without prompt prefix, sampling params, tools or files, so the composer became usable with settings that silently fell back to defaults. Only a conversation whose full record is already cached now takes the instant path; the first visit keeps the previous behavior and moves the route once the record is in hand. Every later switch to it is instant, which is the case this PR set out to fix. A failed record fetch removed the target's message cache even though, after optimistic navigation, that query is already mounted — a transient error could cancel an in-flight history fetch, or discard one that had succeeded, with no route change left to remount it. That removal is now limited to a conversation confirmed gone, and the first-visit path still clears before the route moves, where a fresh mount follows. The benchmark's round-trip assertion was also unfalsifiable: nothing delayed the record request, so an implementation that awaits it still answered inside the threshold. It now holds that request open and asserts the warm switch completes while it is unresolved, which no wall-clock bound can fake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qZJDNkyH5rgCz6KcLjseq * 🧭 fix: Tie Pending Navigation Work to the Route, Not a Token Codex found that the navigation token only tracked calls made through this hook. Every other way out of a conversation — `useNewConvo`, a link, a redirect, the back button — moves the route without touching it, so a record still in flight for the conversation being left passed the guard. On the cached path that overwrote the new route's conversation state; on the first-visit path it was worse, calling `navigate()` and pulling the user back into a chat they had already left. The token was the wrong question. What makes pending work still wanted is not "was this the last conversation clicked" but "is the user still where they were when it started" — and only the browser's own location sees every way that can change. Each async step now captures the route before its request and re-reads it before writing, which subsumes the superseded-click case the token was added for and removes the module state entirely. Reading `window.location` directly rather than `useLocation` keeps this free of subscriptions: every sidebar row mounts this hook, so subscribing would re-render all of them on every navigation — the cost this hook exists to avoid. Comparing pathname against pathname also makes the basename cancel. The tests move from `MemoryRouter` to a real history, since the mechanism is now the browser location itself, and cover both bypass paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qZJDNkyH5rgCz6KcLjseq * 🔢 fix: Keep the Last Click Authoritative Across First-Visit Navigations Codex found that the route guard cannot separate two first-visit clicks from each other. That path deliberately leaves the route where it is until the record arrives, so clicking two uncached conversations in quick succession has both requests capture the same pathname — whichever the network answered first then navigated, and the later click was discarded. Response order decided where the user landed instead of click order. Restores a generation counter alongside the route check. Claiming the last PR's removal of the token as a subsumption was wrong: the two guards answer different questions and neither covers the other. The generation says "a newer intent replaced this one"; the route says "the user left by some means this hook never saw". Both are needed, and both are cheap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qZJDNkyH5rgCz6KcLjseq * 🧹 refactor: Stop Writing Server Snapshots Into User-Owned Conversation State Four review findings on this branch were all the same defect: navigation started a background fetch and wrote its result into the conversation atom. That atom is user-editable — model, endpoint, prompt prefix, sampling params — and the target chat is interactive from the moment the route changes, so a late write races the user and every other writer. Each round added another predicate to the write ("is this still the last click?", "is the user still on this route?"), and each predicate left one more writer uncovered; the last one is a setting picked on the same route by the same navigation, which no ordering or route guard can see. Remove the write instead of guarding it. The warm path refreshes the React Query cache and stops there, so the optimistic merge that lands with the route is the navigation's last word. The refreshed record is consumed by the next switch to that conversation, which is where a cached record is read anyway. This also dissolves the queued-focus finding: `applyConversation` (and its `requestChatFocus`) is now reachable only from paths that navigate, so a focus intent can no longer outlive the navigation that requested it. Scope the synchronous route commit to conversation switches. `useTransitions` on the provider disabled transitions for every route, including the lazily loaded prompts, skills, insights and project screens, where yielding to input during a large first render is worth more than an atomic swap. The opt-out now travels per navigation as `chatNavigation` (`flushSync`), applied in `useNavigateToConvo` and `useNewConvo`. Tests: the four behavioural guards fail against an implementation that restores the background write, including a new case where the user picks a model while the refresh is in flight. * 🎯 fix: Decide Route Commit Once, and Keep Refreshed Settings Refreshed Reverts the per-navigation transition opt-out from the previous commit. Review asked to scope `useTransitions={false}` to conversation switches, and I scoped it by passing an option at the call sites I knew about — then immediately missed one: `finalHandler` promotes `/c/new` to the server-assigned ID and navigates without it, so the atom identifies the real conversation while the route and message query still say `new`. That is not a missed call site, it is the wrong shape. Fourteen call sites across components, chat hooks and SSE handlers navigate into `/c/*`; an opt-out carried by each one is a list that rots as call sites are added, and five of the fourteen were covered. The property is route-shaped, so the decision goes back to the one place that sees every navigation. Answering the original critique on its merits: nothing in the app reads route data through router loaders or renders pending UI from `useNavigation`, so the transition produces no interstitial on any route — it only defers the commit, which on the chat route is the bug this PR exists to fix. Two conversation fixes alongside it: Sidebar rows no longer reinstate settings the background refresh replaced. The row projection carries `endpoint`, `model` and `spec`, and the warm path spreads the row over the cached record — so a row from before an edit made on another device would undo that edit on every switch until the list refetched. The refresh now merges the record into the list cache, which is what made "picked up on the next switch" true rather than merely intended. Starting a new chat now supersedes a pending first visit. "New chat" from `/c/new` lands on `/c/new`, so the pathname is unchanged and the record for a conversation the user just abandoned would land and pull them into it. The navigation counter is exported as `supersedeNavigation` and called from `useNewConvo`. Deliberately not called from the stream recoveries in `useEventHandlers`/`useChatFunctions`: those are the app reacting, not the user changing their mind, and they should not cancel a conversation the user opened. Intent is a closed set; navigation is not. Both new tests fail against the implementation they guard. * 🧷 fix: Keep the Record Refresh Off List State and Off Background Composers Three fixes to the previous two commits, all the same underlying mistake in different places: something that started earlier landing on top of something the user did later. The list-cache write added last commit merged the whole fetched record into every sidebar and pinned row. That response is a snapshot from before the target was interactive, and the list is where renaming, pinning and sharing land — so a rename completing while the request was in flight was silently undone. This is the same stale-snapshot-over-live-state mistake the refresh had just stopped making against the conversation atom, reintroduced one layer down. It now writes `endpoint`, `model` and `spec` only, which is what the staleness it exists to fix is about, and which no list mutation touches. The route comparison ignored the query string. `/c/new?projectId=A` is a different conversation scope than `/c/new`, and the landing chip re-scopes a draft by writing the atom and rewriting search params in place — never through a conversation hook, so neither the pathname nor the recorded intent moved. A pending first-visit record would then land on the draft the user had just re-scoped. The comparison now includes `search`. Superseding moved from `switchToConversation` into `newConversation`, guarded by `keepComposerState`. That flag marks a call that re-renders a composer an earlier call already opened — agent metadata arriving late, for instance. The user asked for nothing there, so it must not cancel a conversation they clicked while it was in flight. `switchToConversation` has no callers outside this hook, so the move loses no coverage. The first two are covered by tests that fail against the implementation they guard. The third is verified by inspection: exercising it needs the whole `useNewConvo` provider tree, which is disproportionate for a one-line guard. --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Danny Avila
GitHub
parent
29f6ec6eae
commit
393742016e
11 changed files with 1515 additions and 216 deletions
|
|
@ -64,7 +64,26 @@ const App = () => {
|
|||
<RadixToast.Provider>
|
||||
<ToastProvider>
|
||||
<DndProvider backend={HTML5Backend}>
|
||||
<RouterProvider router={router} />
|
||||
{/* Location updates commit in the caller's own task instead
|
||||
of React's transition lane. A transition keeps the
|
||||
OUTGOING route painted until the incoming one finishes
|
||||
rendering, so switching conversations left the previous
|
||||
transcript on screen under the new URL for as long as the
|
||||
next thread took to render.
|
||||
|
||||
Set here rather than per navigation because the property
|
||||
is route-shaped, not caller-shaped: fourteen call sites
|
||||
across components, chat hooks and SSE handlers navigate
|
||||
into `/c/*`, and an opt-out passed at each one is a list
|
||||
that silently rots as call sites are added. Nothing in the
|
||||
app reads route data through router loaders or renders
|
||||
pending UI from `useNavigation`, so the transition buys no
|
||||
interstitial on any route — it only defers the commit. And
|
||||
conversation state still lives in Recoil, whose
|
||||
transition-safe reads are gated behind
|
||||
`_TRANSITION_SUPPORT_UNSTABLE` hooks this app does not use.
|
||||
Worth revisiting once that state has moved to Jotai. */}
|
||||
<RouterProvider router={router} useTransitions={false} />
|
||||
<WakeLockManager />
|
||||
<QueryDevtoolsGate />
|
||||
<Toast />
|
||||
|
|
|
|||
|
|
@ -0,0 +1,464 @@
|
|||
import { RecoilRoot, useRecoilValue } from 'recoil';
|
||||
import { QueryKeys } from 'librechat-data-provider';
|
||||
import { render, screen, act, waitFor } from '@testing-library/react';
|
||||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
|
||||
import { BrowserRouter, useLocation, useNavigate } from 'react-router-dom';
|
||||
import type { TConversation, TEndpointsConfig } from 'librechat-data-provider';
|
||||
import useNavigateToConvo, { supersedeNavigation } from '../useNavigateToConvo';
|
||||
import { SetConvoProvider } from '~/Providers';
|
||||
import store from '~/store';
|
||||
|
||||
/** The one thing a test cannot own: the HTTP call. Deferred per conversation
|
||||
* so the assertions can observe the window before a record lands, and can
|
||||
* settle two in-flight requests out of order. */
|
||||
const mockPending = new Map<
|
||||
string,
|
||||
{ resolve: (conversation: TConversation) => void; reject: (error: unknown) => void }
|
||||
>();
|
||||
const mockGetConversationById = jest.fn(
|
||||
(id: string) =>
|
||||
new Promise<TConversation>((resolve, reject) => {
|
||||
mockPending.set(id, { resolve, reject });
|
||||
}),
|
||||
);
|
||||
|
||||
jest.mock('librechat-data-provider', () => ({
|
||||
...jest.requireActual('librechat-data-provider'),
|
||||
dataService: {
|
||||
...jest.requireActual('librechat-data-provider').dataService,
|
||||
getConversationById: (...args: unknown[]) => mockGetConversationById(...(args as [string])),
|
||||
},
|
||||
}));
|
||||
|
||||
const B = 'convo-b';
|
||||
const C = 'convo-c';
|
||||
|
||||
/** What `getConvosByCursor` projects into the sidebar: no prompt prefix, no
|
||||
* sampling params. Navigating from a row must never expose these as absent. */
|
||||
const rowB = {
|
||||
conversationId: B,
|
||||
title: 'Bravo',
|
||||
endpoint: 'openAI',
|
||||
model: 'gpt-4o-mini',
|
||||
} as TConversation;
|
||||
|
||||
const rowC = {
|
||||
conversationId: C,
|
||||
title: 'Charlie',
|
||||
endpoint: 'openAI',
|
||||
model: 'gpt-4o-mini',
|
||||
} as TConversation;
|
||||
|
||||
const recordB = {
|
||||
conversationId: B,
|
||||
title: 'Bravo (stale title)',
|
||||
endpoint: 'openAI',
|
||||
model: 'gpt-4o',
|
||||
promptPrefix: 'You are terse.',
|
||||
temperature: 0.2,
|
||||
} as TConversation;
|
||||
|
||||
const recordC = {
|
||||
conversationId: C,
|
||||
title: 'Charlie (full)',
|
||||
endpoint: 'openAI',
|
||||
model: 'gpt-4o',
|
||||
promptPrefix: 'You are verbose.',
|
||||
temperature: 0.9,
|
||||
} as TConversation;
|
||||
|
||||
const endpointsConfig = { openAI: {} } as unknown as TEndpointsConfig;
|
||||
|
||||
const notFound = () => ({ status: 404, message: 'not found' });
|
||||
|
||||
function Harness() {
|
||||
const { navigateToConvo } = useNavigateToConvo();
|
||||
const conversation = useRecoilValue(store.conversationByIndex(0));
|
||||
const { setConversation } = store.useSetConversationAtom(0);
|
||||
const navigate = useNavigate();
|
||||
const location = useLocation();
|
||||
|
||||
return (
|
||||
<div>
|
||||
<button
|
||||
data-testid="go-b"
|
||||
onClick={() => navigateToConvo(rowB, { currentConvoId: 'convo-a' })}
|
||||
/>
|
||||
<button data-testid="go-c" onClick={() => navigateToConvo(rowC, { currentConvoId: B })} />
|
||||
{/* Every other way out of a conversation — "New chat", a link, a
|
||||
redirect, the back button — moves the route without going through
|
||||
`navigateToConvo`, exactly like `useNewConvo` does. */}
|
||||
<button data-testid="go-elsewhere" onClick={() => navigate('/c/new')} />
|
||||
{/* Picking a model, endpoint or spec reaches the same atom through
|
||||
`newConversation` and stays on the same route — the case no route or
|
||||
ordering guard can see. */}
|
||||
<button
|
||||
data-testid="pick-model"
|
||||
onClick={() =>
|
||||
setConversation({ ...(conversation as TConversation), model: 'user-picked-model' })
|
||||
}
|
||||
/>
|
||||
{/* What `ProjectLandingChip` does: re-scopes the draft and rewrites the
|
||||
query string in place. The pathname never moves. */}
|
||||
<button
|
||||
data-testid="scope-draft"
|
||||
onClick={() => {
|
||||
setConversation({ ...(conversation as TConversation), chatProjectId: 'project-x' });
|
||||
navigate('/c/new?projectId=project-x', { replace: true });
|
||||
}}
|
||||
/>
|
||||
{/* What `useNewConvo` does on "New chat": records that the user wants a
|
||||
different conversation, then navigates — to the SAME pathname when
|
||||
they are already on `/c/new`. */}
|
||||
<button
|
||||
data-testid="new-chat"
|
||||
onClick={() => {
|
||||
supersedeNavigation();
|
||||
navigate('/c/new');
|
||||
}}
|
||||
/>
|
||||
<div data-testid="path">{location.pathname}</div>
|
||||
<div data-testid="convo">{JSON.stringify(conversation ?? null)}</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
/** A real history, not `MemoryRouter`: the hook abandons superseded work by
|
||||
* reading the browser's own location, so the test has to move it for real. */
|
||||
function renderHarness(cached: TConversation[] = []) {
|
||||
window.history.pushState({}, '', '/c/convo-a');
|
||||
|
||||
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
||||
queryClient.setQueryData([QueryKeys.endpoints], endpointsConfig);
|
||||
queryClient.setQueryData([QueryKeys.allConversations], {
|
||||
pages: [{ conversations: [rowB, rowC], nextCursor: null }],
|
||||
pageParams: [undefined],
|
||||
});
|
||||
for (const record of cached) {
|
||||
queryClient.setQueryData([QueryKeys.conversation, record.conversationId], record);
|
||||
}
|
||||
|
||||
render(
|
||||
<BrowserRouter>
|
||||
<RecoilRoot>
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<SetConvoProvider>
|
||||
<Harness />
|
||||
</SetConvoProvider>
|
||||
</QueryClientProvider>
|
||||
</RecoilRoot>
|
||||
</BrowserRouter>,
|
||||
);
|
||||
|
||||
return queryClient;
|
||||
}
|
||||
|
||||
const currentConvo = (): TConversation | null =>
|
||||
JSON.parse(screen.getByTestId('convo').textContent ?? 'null');
|
||||
const currentPath = () => screen.getByTestId('path').textContent;
|
||||
|
||||
const sidebarRow = (queryClient: QueryClient, id: string) =>
|
||||
queryClient
|
||||
.getQueryData<{ pages: { conversations: TConversation[] }[] }>([QueryKeys.allConversations])
|
||||
?.pages[0].conversations.find((c) => c.conversationId === id);
|
||||
|
||||
const click = (testId: string) => {
|
||||
act(() => {
|
||||
screen.getByTestId(testId).click();
|
||||
});
|
||||
};
|
||||
|
||||
const settle = async (id: string, outcome: TConversation | { status: number }) => {
|
||||
const deferred = mockPending.get(id);
|
||||
if (!deferred) {
|
||||
throw new Error(`no in-flight request for ${id}`);
|
||||
}
|
||||
await act(async () => {
|
||||
if ('conversationId' in outcome) {
|
||||
deferred.resolve(outcome as TConversation);
|
||||
} else {
|
||||
deferred.reject(outcome);
|
||||
}
|
||||
});
|
||||
};
|
||||
|
||||
describe('useNavigateToConvo', () => {
|
||||
afterEach(() => {
|
||||
mockGetConversationById.mockClear();
|
||||
mockPending.clear();
|
||||
});
|
||||
|
||||
describe('with the full record already cached', () => {
|
||||
it('changes the route and the conversation together, without waiting on the fetch', () => {
|
||||
renderHarness([recordB]);
|
||||
click('go-b');
|
||||
|
||||
/** The refresh is in flight and deliberately unresolved: the route and
|
||||
* the conversation state must already be on the target. Awaiting the
|
||||
* response here is what left the previous conversation on screen. */
|
||||
expect(mockGetConversationById).toHaveBeenCalledWith(B);
|
||||
expect(currentPath()).toBe(`/c/${B}`);
|
||||
expect(currentConvo()?.conversationId).toBe(B);
|
||||
});
|
||||
|
||||
it('underlays the cached record so the sidebar projection cannot drop settings', () => {
|
||||
renderHarness([recordB]);
|
||||
click('go-b');
|
||||
|
||||
const optimistic = currentConvo();
|
||||
/** Fields only the record carries survive the switch, so a send during
|
||||
* the refresh window still uses this conversation's real settings. */
|
||||
expect(optimistic?.promptPrefix).toBe('You are terse.');
|
||||
expect(optimistic?.temperature).toBe(0.2);
|
||||
/** ...while the row stays authoritative for what it does carry. */
|
||||
expect(optimistic?.title).toBe('Bravo');
|
||||
expect(optimistic?.model).toBe('gpt-4o-mini');
|
||||
});
|
||||
|
||||
it('refreshes the cached record for the next switch', async () => {
|
||||
const queryClient = renderHarness([recordB]);
|
||||
click('go-b');
|
||||
await settle(B, { ...recordB, title: 'Bravo (server)' });
|
||||
|
||||
await waitFor(() =>
|
||||
expect(queryClient.getQueryData<TConversation>([QueryKeys.conversation, B])?.title).toBe(
|
||||
'Bravo (server)',
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
it('does not write the refreshed record into conversation state', async () => {
|
||||
renderHarness([recordB]);
|
||||
click('go-b');
|
||||
await settle(B, { ...recordB, title: 'Bravo (server)' });
|
||||
|
||||
/** Conversation state is the user's — model, endpoint, prompt prefix,
|
||||
* sampling params — and the target is interactive the moment the route
|
||||
* changes. A background write of a server snapshot into it races the
|
||||
* user and every other writer, so this navigation's last write is the
|
||||
* optimistic one. */
|
||||
expect(currentConvo()?.title).toBe('Bravo');
|
||||
expect(currentConvo()?.promptPrefix).toBe('You are terse.');
|
||||
});
|
||||
|
||||
it('updates the sidebar row so it cannot reinstate settings the refresh replaced', async () => {
|
||||
const queryClient = renderHarness([recordB]);
|
||||
click('go-b');
|
||||
/** `endpoint`, `model` and `spec` are in the sidebar projection, so a row
|
||||
* from before a change made on another device would spread back over the
|
||||
* refreshed record on every switch — until the list itself refetched. */
|
||||
await settle(B, { ...recordB, model: 'gpt-4o-elsewhere' });
|
||||
|
||||
await waitFor(() => expect(sidebarRow(queryClient, B)?.model).toBe('gpt-4o-elsewhere'));
|
||||
});
|
||||
|
||||
it('leaves list-owned row state alone when the refresh lands', async () => {
|
||||
const queryClient = renderHarness([recordB]);
|
||||
click('go-b');
|
||||
|
||||
/** Renaming and pinning land in the list while this request is in flight.
|
||||
* The response is a snapshot from before either happened. */
|
||||
queryClient.setQueryData([QueryKeys.allConversations], {
|
||||
pages: [{ conversations: [{ ...rowB, title: 'Renamed', pinned: true }, rowC] }],
|
||||
pageParams: [undefined],
|
||||
});
|
||||
await settle(B, { ...recordB, model: 'gpt-4o-elsewhere' });
|
||||
|
||||
await waitFor(() => expect(sidebarRow(queryClient, B)?.model).toBe('gpt-4o-elsewhere'));
|
||||
expect(sidebarRow(queryClient, B)?.title).toBe('Renamed');
|
||||
expect(sidebarRow(queryClient, B)?.pinned).toBe(true);
|
||||
});
|
||||
|
||||
it('keeps a setting the user picks while the refresh is in flight', async () => {
|
||||
renderHarness([recordB]);
|
||||
click('go-b');
|
||||
|
||||
/** Same conversation, same route, same navigation: no ordering or route
|
||||
* guard can tell this apart from an untouched screen. */
|
||||
click('pick-model');
|
||||
expect(currentConvo()?.model).toBe('user-picked-model');
|
||||
|
||||
await settle(B, recordB);
|
||||
|
||||
/** `recordB.model` is `gpt-4o`. Restoring it here would visibly revert
|
||||
* the pick and make the next send carry settings the user did not
|
||||
* choose. */
|
||||
expect(currentConvo()?.model).toBe('user-picked-model');
|
||||
});
|
||||
});
|
||||
|
||||
describe('on the first visit, with no cached record', () => {
|
||||
it('holds the route until the record that a send needs is in hand', async () => {
|
||||
renderHarness();
|
||||
click('go-b');
|
||||
|
||||
/** The sidebar row alone would expose a usable composer whose sends
|
||||
* silently carry default prompt prefix and sampling params. */
|
||||
expect(mockGetConversationById).toHaveBeenCalledWith(B);
|
||||
expect(currentPath()).toBe('/c/convo-a');
|
||||
|
||||
await settle(B, recordB);
|
||||
|
||||
await waitFor(() => expect(currentPath()).toBe(`/c/${B}`));
|
||||
expect(currentConvo()?.promptPrefix).toBe('You are terse.');
|
||||
expect(currentConvo()?.temperature).toBe(0.2);
|
||||
});
|
||||
|
||||
it('still lands on the conversation when the record fetch fails', async () => {
|
||||
const queryClient = renderHarness();
|
||||
queryClient.setQueryData([QueryKeys.messages, B], [{ messageId: 'stale' }]);
|
||||
click('go-b');
|
||||
await settle(B, notFound());
|
||||
|
||||
await waitFor(() => expect(currentPath()).toBe(`/c/${B}`));
|
||||
expect(currentConvo()?.conversationId).toBe(B);
|
||||
/** Cleared before the route moved, so the target mounts a fresh query. */
|
||||
expect(queryClient.getQueryData([QueryKeys.messages, B])).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('when a later navigation supersedes an in-flight one', () => {
|
||||
it('does not restore a conversation whose refresh resolves after the user moved on', async () => {
|
||||
renderHarness([recordB, recordC]);
|
||||
click('go-b');
|
||||
click('go-c');
|
||||
expect(currentPath()).toBe(`/c/${C}`);
|
||||
|
||||
/** B's refresh lands last. Writing it would restore B into state while
|
||||
* the route and transcript show C — and sends read from state. The
|
||||
* refresh writes only the query cache, so there is nothing to restore. */
|
||||
await settle(B, { ...recordB, title: 'Bravo (late)' });
|
||||
|
||||
expect(currentPath()).toBe(`/c/${C}`);
|
||||
expect(currentConvo()?.conversationId).toBe(C);
|
||||
expect(currentConvo()?.title).toBe('Charlie');
|
||||
});
|
||||
|
||||
it('abandons a refresh when the user left through a path that bypasses this hook', async () => {
|
||||
renderHarness([recordB]);
|
||||
click('go-b');
|
||||
expect(currentPath()).toBe(`/c/${B}`);
|
||||
|
||||
/** `useNewConvo` and every link, redirect or back-button press move the
|
||||
* route without calling `navigateToConvo`, so nothing this hook tracks
|
||||
* for itself would notice — only the browser's own location does. */
|
||||
click('go-elsewhere');
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
|
||||
await settle(B, { ...recordB, title: 'Bravo (late)' });
|
||||
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
expect(currentConvo()?.title).not.toBe('Bravo (late)');
|
||||
});
|
||||
|
||||
it('does not pull the user back when they leave before the first-visit record lands', async () => {
|
||||
renderHarness();
|
||||
click('go-b');
|
||||
/** Uncached, so the route has not moved yet — B is still only pending. */
|
||||
expect(currentPath()).toBe('/c/convo-a');
|
||||
|
||||
click('go-elsewhere');
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
|
||||
await settle(B, recordB);
|
||||
|
||||
/** Completing the navigation here would yank the user out of the chat
|
||||
* they deliberately opened and into one they had already left. */
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
});
|
||||
|
||||
it('does not land a first-visit record after the user starts a new chat', async () => {
|
||||
renderHarness();
|
||||
/** Start where "New chat" also lands, so the pathname genuinely cannot
|
||||
* distinguish before from after. */
|
||||
click('go-elsewhere');
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
|
||||
click('go-b');
|
||||
/** Uncached, so the route has not moved — still `/c/new`. */
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
|
||||
click('new-chat');
|
||||
/** "New chat" from `/c/new` lands on `/c/new`: the pathname is unchanged,
|
||||
* so only the recorded intent can tell that B is no longer wanted. */
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
|
||||
await settle(B, recordB);
|
||||
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
expect(currentConvo()?.conversationId).not.toBe(B);
|
||||
});
|
||||
|
||||
it('does not land a first-visit record after the user re-scopes the draft', async () => {
|
||||
renderHarness();
|
||||
click('go-elsewhere');
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
|
||||
click('go-b');
|
||||
click('scope-draft');
|
||||
/** Only the query string moved, so a pathname-only comparison sees
|
||||
* nothing — and this action never goes through a conversation hook, so
|
||||
* no intent is recorded either. */
|
||||
expect(currentPath()).toBe('/c/new');
|
||||
|
||||
await settle(B, recordB);
|
||||
|
||||
expect(currentConvo()?.chatProjectId).toBe('project-x');
|
||||
expect(currentConvo()?.conversationId).not.toBe(B);
|
||||
});
|
||||
|
||||
it('lands on the last conversation clicked, not the first record to arrive', async () => {
|
||||
renderHarness();
|
||||
click('go-b');
|
||||
click('go-c');
|
||||
/** Neither has moved the route yet — the first-visit path waits for its
|
||||
* record — so both requests started from the same pathname and only
|
||||
* click order can say which one the user actually wants. */
|
||||
expect(currentPath()).toBe('/c/convo-a');
|
||||
|
||||
/** B answers first, but C was clicked last. */
|
||||
await settle(B, recordB);
|
||||
await settle(C, recordC);
|
||||
|
||||
expect(currentPath()).toBe(`/c/${C}`);
|
||||
expect(currentConvo()?.conversationId).toBe(C);
|
||||
});
|
||||
|
||||
it('discards a first-visit navigation that resolves after the user moved on', async () => {
|
||||
renderHarness([recordC]);
|
||||
click('go-b');
|
||||
click('go-c');
|
||||
expect(currentPath()).toBe(`/c/${C}`);
|
||||
|
||||
await settle(B, recordB);
|
||||
|
||||
expect(currentPath()).toBe(`/c/${C}`);
|
||||
expect(currentConvo()?.conversationId).toBe(C);
|
||||
});
|
||||
});
|
||||
|
||||
describe('when the refresh fails after the route already moved', () => {
|
||||
it('keeps the mounted message cache on a transient failure', async () => {
|
||||
const queryClient = renderHarness([recordB]);
|
||||
queryClient.setQueryData([QueryKeys.messages, B], [{ messageId: 'loaded' }]);
|
||||
click('go-b');
|
||||
await settle(B, { status: 500 });
|
||||
|
||||
/** The messages query is already mounted, so dropping it here would
|
||||
* cancel or discard a history fetch with no remount left to retry it. */
|
||||
expect(queryClient.getQueryData([QueryKeys.messages, B])).toEqual([{ messageId: 'loaded' }]);
|
||||
expect(currentPath()).toBe(`/c/${B}`);
|
||||
});
|
||||
|
||||
it('drops the message cache when the conversation is confirmed gone', async () => {
|
||||
const queryClient = renderHarness([recordB]);
|
||||
queryClient.setQueryData([QueryKeys.messages, B], [{ messageId: 'stale' }]);
|
||||
click('go-b');
|
||||
await settle(B, notFound());
|
||||
|
||||
await waitFor(() =>
|
||||
expect(queryClient.getQueryData([QueryKeys.messages, B])).toBeUndefined(),
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -21,12 +21,75 @@ import {
|
|||
getDefaultEndpoint,
|
||||
buildDefaultConvo,
|
||||
requestChatFocus,
|
||||
isNotFoundError,
|
||||
updateConvoInAllQueries,
|
||||
logger,
|
||||
} from '~/utils';
|
||||
import { useApplyModelSpecEffects } from '~/hooks/Agents';
|
||||
import { startupConfigKey } from '~/data-provider';
|
||||
import store from '~/store';
|
||||
|
||||
/**
|
||||
* The route the browser is actually showing, as the browser reports it.
|
||||
*
|
||||
* Only `navigateWithRecord` needs this, and only because it is the one path
|
||||
* that still calls `navigate()` after an await: it captures the route before
|
||||
* its request and re-reads it before moving the user, so a record that lands
|
||||
* for a conversation the user has already left cannot pull them back. The
|
||||
* browser's own location is the only thing that sees EVERY way they can leave
|
||||
* — a different sidebar row, "New chat", a link, a redirect, or the back
|
||||
* button — where any bookkeeping this hook maintained itself would only cover
|
||||
* the navigations that happen to route through it.
|
||||
*
|
||||
* The query string counts: `/c/new?projectId=A` is a different conversation
|
||||
* scope than `/c/new`, and the chip that changes it writes the draft in place
|
||||
* without ever going through a conversation hook — so a pathname-only
|
||||
* comparison would let a pending record land on a draft the user had just
|
||||
* re-scoped.
|
||||
*
|
||||
* Read directly rather than through `useLocation` because each sidebar row
|
||||
* mounts its own `useNavigateToConvo`: subscribing would re-render every row on
|
||||
* every navigation, which is the cost this hook exists to avoid. Comparing a
|
||||
* location against a location also makes the router basename cancel out.
|
||||
*/
|
||||
const currentRoute = () => window.location.pathname + window.location.search;
|
||||
|
||||
/**
|
||||
* Counts navigations this hook starts, so the user's LAST click is the one
|
||||
* that lands.
|
||||
*
|
||||
* The route check alone cannot separate two first-visit clicks from each
|
||||
* other: that path deliberately leaves the route where it is until the record
|
||||
* arrives, so both captures read the same pathname and whichever request the
|
||||
* network answered first would win. The two guards are orthogonal and neither
|
||||
* subsumes the other — the generation says "a newer intent replaced this one",
|
||||
* the route says "the user left by some means this hook never saw".
|
||||
*
|
||||
* Module-scoped for the same reason the route is read from the browser: every
|
||||
* sidebar row mounts its own `useNavigateToConvo`, so a ref would be private
|
||||
* to the row that was clicked and blind to the click that superseded it.
|
||||
*/
|
||||
let navigationGeneration = 0;
|
||||
|
||||
/**
|
||||
* Records that the user has asked for a different conversation, so a first
|
||||
* visit still waiting on its record abandons instead of landing.
|
||||
*
|
||||
* Called by `useNewConvo`, which is the only other place a user action decides
|
||||
* WHICH conversation they want. Starting a new chat from `/c/new` leaves the
|
||||
* pathname exactly where it was, so the route check cannot see it — the same
|
||||
* blind spot two first-visit clicks have, and for the same reason.
|
||||
*
|
||||
* Deliberately not called from every `navigate()` that touches `/c/*`. The
|
||||
* recoveries in `useEventHandlers` and `useChatFunctions` are the app reacting
|
||||
* to a stream, not the user changing their mind, and they should not cancel a
|
||||
* conversation the user deliberately opened. Intent is a closed set; navigation
|
||||
* is not.
|
||||
*/
|
||||
export const supersedeNavigation = () => {
|
||||
navigationGeneration++;
|
||||
};
|
||||
|
||||
const useNavigateToConvo = (index = 0) => {
|
||||
const navigate = useNavigate();
|
||||
const queryClient = useQueryClient();
|
||||
|
|
@ -52,34 +115,108 @@ const useNavigateToConvo = (index = 0) => {
|
|||
[setConvo, queryClient, applyModelSpecEffects],
|
||||
);
|
||||
|
||||
const fetchFreshData = async (conversation?: Partial<TConversation>) => {
|
||||
const conversationId = conversation?.conversationId;
|
||||
const applyConversation = (conversation: TConversation) => {
|
||||
const target = { ...conversation };
|
||||
clearModelForNonEphemeralAgent(target);
|
||||
setConversation(target);
|
||||
requestChatFocus();
|
||||
};
|
||||
|
||||
const fetchConversationRecord = (conversationId: string) =>
|
||||
queryClient.fetchQuery([QueryKeys.conversation, conversationId], () =>
|
||||
dataService.getConversationById(conversationId),
|
||||
);
|
||||
|
||||
/**
|
||||
* Refreshes the cached record AFTER the route has already changed, for a
|
||||
* conversation whose full record was already cached. The cache already
|
||||
* carries everything the sidebar projection omits, so this exists only to
|
||||
* pick up edits made elsewhere — awaiting it before navigating would spend a
|
||||
* round trip with the DEPARTING conversation still on screen.
|
||||
*
|
||||
* It refreshes the CACHE and deliberately writes nothing into conversation
|
||||
* state. That state is the user's: model, endpoint, prompt prefix, sampling
|
||||
* params, and the target is interactive from the moment the route changes,
|
||||
* so writing a server snapshot into it once the response lands races the
|
||||
* user's own selections and every other writer — streamed updates, presets,
|
||||
* mention select. No predicate closes that: "is this still wanted?" gains one
|
||||
* more answer per writer, and a route or ordering guard cannot see a user who
|
||||
* never left. The refreshed record is read by the next switch to this
|
||||
* conversation, which is where a cached record is consumed.
|
||||
*/
|
||||
const refreshConversationRecord = async (conversationId: string) => {
|
||||
try {
|
||||
const data = await fetchConversationRecord(conversationId);
|
||||
logger.log('conversation', 'Refreshed cached conversation record', data);
|
||||
/** The sidebar row overlays this record on the next switch, and the row
|
||||
* projection carries `endpoint`, `model` and `spec` — so leaving the list
|
||||
* untouched would let a row from before an edit made elsewhere reinstate
|
||||
* the old setting on every switch until the list itself refetches, which
|
||||
* is the opposite of what this refresh is for.
|
||||
*
|
||||
* Only those three fields. This response is a snapshot from before the
|
||||
* user could touch anything, and the list is where renaming, pinning and
|
||||
* sharing land — writing it wholesale would undo a rename that completed
|
||||
* while this was in flight, which is the same stale-snapshot-over-live-
|
||||
* state mistake this refresh stopped making against the conversation
|
||||
* atom. No list mutation touches these three. */
|
||||
updateConvoInAllQueries(queryClient, conversationId, (row) => ({
|
||||
...row,
|
||||
endpoint: data.endpoint,
|
||||
model: data.model,
|
||||
spec: data.spec,
|
||||
}));
|
||||
} catch (error) {
|
||||
logger.error('conversation', 'Error refreshing conversation record on navigation', error);
|
||||
/** Only a conversation that is confirmed GONE invalidates what is on
|
||||
* screen. The messages query is already mounted by now, so dropping its
|
||||
* cache on a transient failure would cancel an in-flight history fetch
|
||||
* (or discard one that already succeeded) with no route change left to
|
||||
* remount it, blanking a transcript that was fine. */
|
||||
if (isNotFoundError(error)) {
|
||||
queryClient.removeQueries([QueryKeys.messages, conversationId]);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
/**
|
||||
* First visit to a conversation in this session: nothing has its full record
|
||||
* yet, and the sidebar row is a PROJECTION (see `getConvosByCursor`) without
|
||||
* prompt prefix, sampling params, tools or files. Landing the route on that
|
||||
* would expose a usable composer whose sends silently carry default
|
||||
* settings, so this path keeps the pre-existing behavior and moves the route
|
||||
* once the real record is in hand. Every later switch to this conversation
|
||||
* takes the instant path above.
|
||||
*/
|
||||
const navigateWithRecord = async (conversation: TConversation, generation: number) => {
|
||||
const conversationId = conversation.conversationId;
|
||||
if (!conversationId) {
|
||||
return;
|
||||
}
|
||||
/** The route the user was on when they asked for this one. The route has
|
||||
* NOT moved yet on this path, so "still here" is what makes finishing the
|
||||
* navigation legitimate — leaving it would mean pulling the user back to a
|
||||
* conversation they already navigated away from. Two first-visit clicks in
|
||||
* a row both capture THIS route, which is why the generation is what keeps
|
||||
* them in click order rather than in response order. */
|
||||
const routeAtStart = currentRoute();
|
||||
let record = conversation;
|
||||
try {
|
||||
const data = await queryClient.fetchQuery([QueryKeys.conversation, conversationId], () =>
|
||||
dataService.getConversationById(conversationId),
|
||||
);
|
||||
logger.log('conversation', 'Fetched fresh conversation data', data);
|
||||
|
||||
const convoData = { ...data };
|
||||
clearModelForNonEphemeralAgent(convoData);
|
||||
setConversation(convoData);
|
||||
requestChatFocus();
|
||||
navigate(`/c/${conversationId ?? Constants.NEW_CONVO}`);
|
||||
record = await fetchConversationRecord(conversationId);
|
||||
logger.log('conversation', 'Fetched fresh conversation data', record);
|
||||
} catch (error) {
|
||||
console.error('Error fetching conversation data on navigation', error);
|
||||
if (conversation) {
|
||||
/** The conversation fetch failed (deleted convo, lost access): drop the
|
||||
* warm message cache so stale contents can't render as current when the
|
||||
* background revalidation fails too. */
|
||||
queryClient.removeQueries([QueryKeys.messages, conversationId]);
|
||||
setConversation(conversation as TConversation);
|
||||
requestChatFocus();
|
||||
navigate(`/c/${conversationId}`);
|
||||
}
|
||||
logger.error('conversation', 'Error fetching conversation data on navigation', error);
|
||||
/** Nothing is mounted for this conversation yet, so clearing a warm
|
||||
* cache here still predates the route change: the target mounts a fresh
|
||||
* query rather than rendering contents that may no longer exist. */
|
||||
queryClient.removeQueries([QueryKeys.messages, conversationId]);
|
||||
}
|
||||
if (generation !== navigationGeneration || currentRoute() !== routeAtStart) {
|
||||
logger.log('conversation', 'Discarding superseded navigation', conversationId);
|
||||
return;
|
||||
}
|
||||
applyConversation(record);
|
||||
navigate(`/c/${conversationId}`);
|
||||
};
|
||||
|
||||
const navigateToConvo = (
|
||||
|
|
@ -95,6 +232,9 @@ const useNavigateToConvo = (index = 0) => {
|
|||
const { currentConvoId } = options || {};
|
||||
logger.log('conversation', 'Navigating to conversation', conversation);
|
||||
hasSetConversation.current = true;
|
||||
/** Claim this click's place in the order before any await, so a request
|
||||
* still in flight for an earlier one cannot land on top of it. */
|
||||
const generation = ++navigationGeneration;
|
||||
setSubmission(null);
|
||||
|
||||
let convo = { ...conversation };
|
||||
|
|
@ -149,8 +289,25 @@ const useNavigateToConvo = (index = 0) => {
|
|||
* from a non-chat route (e.g. /projects).
|
||||
*/
|
||||
queryClient.invalidateQueries([QueryKeys.messages, convo.conversationId]);
|
||||
const cachedConvo = queryClient.getQueryData<TConversation>([
|
||||
QueryKeys.conversation,
|
||||
convo.conversationId,
|
||||
]);
|
||||
queryClient.invalidateQueries([QueryKeys.conversation, convo.conversationId]);
|
||||
fetchFreshData(convo);
|
||||
if (!cachedConvo) {
|
||||
navigateWithRecord(convo, generation);
|
||||
return;
|
||||
}
|
||||
/** Route and conversation state change together, in the click's own
|
||||
* task, so the switch commits once instead of straddling a round trip.
|
||||
* The cached record underlays the row, which is a list PROJECTION: the
|
||||
* row's fields are the fresher ones, everything the projection drops
|
||||
* (prompt prefix, sampling params, files) survives, and this is the last
|
||||
* write this navigation makes — what the user sees now is what a send
|
||||
* will carry until they change it themselves. */
|
||||
applyConversation({ ...cachedConvo, ...convo });
|
||||
navigate(`/c/${convo.conversationId}`);
|
||||
refreshConversationRecord(convo.conversationId);
|
||||
} else {
|
||||
setConversation(convo);
|
||||
requestChatFocus();
|
||||
|
|
|
|||
|
|
@ -37,6 +37,7 @@ import {
|
|||
logger,
|
||||
} from '~/utils';
|
||||
import { useDeleteFilesMutation, useGetEndpointsQuery, useGetStartupConfig } from '~/data-provider';
|
||||
import { supersedeNavigation } from './Conversations/useNavigateToConvo';
|
||||
import useGetConversation from './Conversations/useGetConversation';
|
||||
import useAssistantListMap from './Assistants/useAssistantListMap';
|
||||
import { clearUploadRecovery } from './Files/useFileHandling';
|
||||
|
|
@ -409,6 +410,19 @@ const useNewConvo = (index = 0) => {
|
|||
}
|
||||
}
|
||||
|
||||
/** A first visit to another conversation may still be waiting on its
|
||||
* record. Starting a new chat keeps the pathname, so nothing the route
|
||||
* check sees changes — without this, that record lands and pulls the user
|
||||
* into the conversation they just abandoned.
|
||||
*
|
||||
* `keepComposerState` marks a call that re-renders a composer an earlier
|
||||
* call already opened, such as agent metadata arriving late. The user did
|
||||
* not ask for anything there, so it must not cancel a conversation they
|
||||
* clicked while it was in flight. */
|
||||
if (!keepComposerState) {
|
||||
supersedeNavigation();
|
||||
}
|
||||
|
||||
switchToConversation(
|
||||
conversation,
|
||||
preset,
|
||||
|
|
|
|||
65
e2e/benchmarks-navigation/README.md
Normal file
65
e2e/benchmarks-navigation/README.md
Normal file
|
|
@ -0,0 +1,65 @@
|
|||
# Conversation-Navigation Perf Benchmark (react-scan)
|
||||
|
||||
Guards the app's most-used interaction: picking another conversation from the
|
||||
sidebar. The regression it exists to catch is a **stale switch** — the URL
|
||||
becomes `/c/<next>` while the *previous* conversation is still what's painted.
|
||||
|
||||
Two 30-turn (60-row) conversations are seeded straight into Mongo, then the
|
||||
spec switches between them twice: once cold (target not cached) and once warm
|
||||
(both message caches populated — the case users hit constantly when bouncing
|
||||
between two open chats).
|
||||
|
||||
## What it measures
|
||||
|
||||
An in-page sampler records, once per animation frame, the route the browser is
|
||||
showing and which conversation's rows are mounted. From that:
|
||||
|
||||
- **`staleFrames` / `staleAfterUrlMs`** — frames where the URL already named the
|
||||
next conversation while the previous transcript was still on screen. This is
|
||||
the headline metric and the one the assertions are built around. Frames
|
||||
showing *neither* transcript (the cold switch's spinner) are not stale; only
|
||||
the wrong conversation is.
|
||||
- **`clickToUrlMs`** — click to route change. Catches navigation being gated
|
||||
behind a server round trip again.
|
||||
- **`clickToPaintMs`** — click to the next transcript painted.
|
||||
|
||||
Plus, via [react-scan](https://github.com/aidenybai/react-scan): total component
|
||||
renders and main-thread long tasks across each switch.
|
||||
|
||||
## Why this is a real hazard
|
||||
|
||||
`RouterProvider` commits location updates inside `React.startTransition` by
|
||||
default in react-router v7. A transition keeps the OUTGOING tree painted until
|
||||
the incoming one has finished rendering — so any work that makes the incoming
|
||||
conversation slow to render is paid as time the user spends looking at the
|
||||
wrong conversation, under the right URL. `App.jsx` opts out at the provider,
|
||||
so putting the app back on the transition lane is one of the regressions this
|
||||
benchmark catches.
|
||||
|
||||
## Run
|
||||
|
||||
Requires a built client (`client/dist`) like the other mock e2e configs.
|
||||
react-scan is not a repo dependency; provide the bundle path. Baselines were
|
||||
measured with react-scan 0.5.7 — instrumentation overhead and `onRender`
|
||||
semantics are version-dependent, so keep it pinned:
|
||||
|
||||
```bash
|
||||
npm i --no-save react-scan@0.5.7
|
||||
npm run e2e:benchmark:navigation
|
||||
```
|
||||
|
||||
or point `REACT_SCAN_PATH` at an existing
|
||||
`react-scan@0.5.7/dist/auto.global.js`.
|
||||
|
||||
## Getting component names
|
||||
|
||||
This benchmark runs against the built client so its wall-clock budgets mean
|
||||
something, and the production minifier (oxc) strips `displayName`, leaving
|
||||
react-scan's per-component tally mangled (`tn`, `ic`, …). Totals and long tasks
|
||||
are unaffected.
|
||||
|
||||
To attribute renders to components, run the same spec against the vite dev
|
||||
server — point `baseURL` at `http://127.0.0.1:3090` the way
|
||||
`playwright.config.reasoning-perf.ts` does. Expect the wall-clock assertions to
|
||||
fail there: a dev build's render path is far slower than anything a user sees.
|
||||
Use that mode for attribution, this config for budgets.
|
||||
430
e2e/benchmarks-navigation/navigation.perf.spec.ts
Normal file
430
e2e/benchmarks-navigation/navigation.perf.spec.ts
Normal file
|
|
@ -0,0 +1,430 @@
|
|||
import { randomUUID } from 'node:crypto';
|
||||
import { expect, test } from '@playwright/test';
|
||||
import type { Page, TestInfo } from '@playwright/test';
|
||||
import {
|
||||
clearUserConversations,
|
||||
deleteMessagesByConversation,
|
||||
deleteConversations,
|
||||
seedConversations,
|
||||
seedMessages,
|
||||
} from '../specs/mock/db';
|
||||
import { messagesView } from '../specs/mock/helpers';
|
||||
import { getE2EUser } from '../setup/user';
|
||||
import {
|
||||
attachSnapshot,
|
||||
installReactScan,
|
||||
longTaskStats,
|
||||
resetPerf,
|
||||
snapshotPerf,
|
||||
topComponents,
|
||||
totals,
|
||||
} from '../perf/scan';
|
||||
import {
|
||||
ROWS_PER_CONVO,
|
||||
TURNS_PER_CONVO,
|
||||
buildConversationMessages,
|
||||
convoMarker,
|
||||
turnHeading,
|
||||
} from './payload';
|
||||
|
||||
/**
|
||||
* Conversation-switch perf benchmark (react-scan).
|
||||
*
|
||||
* Guards the single most-used navigation in the app: picking another
|
||||
* conversation from the sidebar. The regression this exists to catch is a
|
||||
* *stale* switch — the URL becomes `/c/<next>` while the previous
|
||||
* conversation is still the thing painted on screen.
|
||||
*
|
||||
* Two things made that happen, and this guards both. `RouterProvider` commits
|
||||
* location updates inside `React.startTransition` by default in react-router
|
||||
* v7, which keeps the OUTGOING tree painted until the incoming one has fully
|
||||
* rendered — so every millisecond the next thread takes to render was spent
|
||||
* showing the previous one (`App.jsx` now opts out). And navigation used to
|
||||
* await a conversation refetch before changing the route at all, spending a
|
||||
* server round trip on the departing conversation.
|
||||
*
|
||||
* The in-page sampler measures exactly that window, so either regression —
|
||||
* route updates back on the transition lane, or navigation gated behind a
|
||||
* request — shows up here as frames of the wrong conversation.
|
||||
*/
|
||||
|
||||
type NavSample = { t: number; path: string; marker: string };
|
||||
|
||||
type NavGlobal = {
|
||||
samples: NavSample[];
|
||||
markers: string[];
|
||||
rafId: number;
|
||||
clickedAt: number;
|
||||
begin(markers: string[]): void;
|
||||
mark(): void;
|
||||
end(): NavSample[];
|
||||
};
|
||||
|
||||
declare global {
|
||||
interface Window {
|
||||
__NAV__: NavGlobal;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Samples once per animation frame: the route the browser is showing and the
|
||||
* conversation whose rows are actually mounted. A frame that reports the next
|
||||
* conversation's path alongside the previous conversation's marker is a frame
|
||||
* the user spent looking at stale content.
|
||||
*
|
||||
* Reading only the FIRST `.message-render` row keeps the per-frame cost to one
|
||||
* row's text; every seeded row carries its conversation's marker, so whichever
|
||||
* slice the progressive mount window admitted identifies the tree either way.
|
||||
* When the main thread blocks, frames simply stop firing — the gap is the
|
||||
* stall, and the next sample reports the state the user actually saw next.
|
||||
*/
|
||||
const NAV_SAMPLER = `(() => {
|
||||
const nav = {
|
||||
samples: [],
|
||||
markers: [],
|
||||
rafId: 0,
|
||||
clickedAt: 0,
|
||||
begin(markers) {
|
||||
this.markers = markers;
|
||||
this.samples = [];
|
||||
this.clickedAt = 0;
|
||||
const tick = () => {
|
||||
const row = document.querySelector('.message-render');
|
||||
const text = row ? row.textContent || '' : '';
|
||||
let marker = '';
|
||||
for (const candidate of this.markers) {
|
||||
if (text.indexOf(candidate) !== -1) {
|
||||
marker = candidate;
|
||||
break;
|
||||
}
|
||||
}
|
||||
this.samples.push({ t: performance.now(), path: location.pathname, marker });
|
||||
this.rafId = requestAnimationFrame(tick);
|
||||
};
|
||||
this.rafId = requestAnimationFrame(tick);
|
||||
},
|
||||
mark() {
|
||||
this.clickedAt = performance.now();
|
||||
},
|
||||
end() {
|
||||
cancelAnimationFrame(this.rafId);
|
||||
const clickedAt = this.clickedAt;
|
||||
return this.samples.map((sample) => ({ ...sample, t: sample.t - clickedAt }));
|
||||
},
|
||||
};
|
||||
window.__NAV__ = nav;
|
||||
})();`;
|
||||
|
||||
type SwitchTiming = {
|
||||
/** Click → the address bar showing the next conversation. */
|
||||
clickToUrlMs: number;
|
||||
/** Click → the next conversation's rows painted. */
|
||||
clickToPaintMs: number;
|
||||
/**
|
||||
* How long the PREVIOUS conversation stayed painted after the URL already
|
||||
* named the next one. Frames showing neither transcript (a spinner on a cold
|
||||
* switch) are not stale — only the wrong conversation is.
|
||||
*/
|
||||
staleAfterUrlMs: number;
|
||||
/** Frames observed showing the next path over the previous transcript. */
|
||||
staleFrames: number;
|
||||
samples: number;
|
||||
};
|
||||
|
||||
function firstSampleTime(samples: NavSample[], predicate: (sample: NavSample) => boolean): number {
|
||||
const found = samples.find(predicate);
|
||||
if (!found) {
|
||||
throw new Error('navigation sampler never observed the expected frame');
|
||||
}
|
||||
return found.t;
|
||||
}
|
||||
|
||||
function summarize(samples: NavSample[], nextPath: string, nextMarker: string): SwitchTiming {
|
||||
const clickToUrlMs = firstSampleTime(samples, (sample) => sample.path === nextPath);
|
||||
const clickToPaintMs = firstSampleTime(
|
||||
samples,
|
||||
(sample) => sample.path === nextPath && sample.marker === nextMarker,
|
||||
);
|
||||
const staleSamples = samples.filter(
|
||||
(sample) => sample.path === nextPath && sample.marker !== '' && sample.marker !== nextMarker,
|
||||
);
|
||||
const lastStale = staleSamples[staleSamples.length - 1];
|
||||
return {
|
||||
clickToUrlMs: Math.round(clickToUrlMs),
|
||||
clickToPaintMs: Math.round(clickToPaintMs),
|
||||
staleAfterUrlMs: lastStale ? Math.round(lastStale.t - clickToUrlMs) : 0,
|
||||
staleFrames: staleSamples.length,
|
||||
samples: samples.length,
|
||||
};
|
||||
}
|
||||
|
||||
const userEmail = getE2EUser().email;
|
||||
const CONVO_A = { id: randomUUID(), label: 'A', title: 'Navigation bench alpha' };
|
||||
const CONVO_B = { id: randomUUID(), label: 'B', title: 'Navigation bench bravo' };
|
||||
const CONVOS = [CONVO_A, CONVO_B];
|
||||
|
||||
/** Conversation row in the sidebar; `Convo` labels rows "<title> conversation". */
|
||||
function sidebarRow(page: Page, title: string) {
|
||||
return page.getByRole('button', { name: `${title} conversation`, exact: true });
|
||||
}
|
||||
|
||||
/** A heading only the given conversation renders, used to confirm its paint. */
|
||||
function threadHeading(page: Page, label: string, turn: number) {
|
||||
return messagesView(page)
|
||||
.getByRole('heading', { name: turnHeading(label, turn), exact: true })
|
||||
.first();
|
||||
}
|
||||
|
||||
/**
|
||||
* Clicks the row from inside the page so the click timestamp shares the page's
|
||||
* clock with the sampler — driving it over the wire would fold the Playwright
|
||||
* round trip into every measured interval. Resolving the row by attribute here
|
||||
* rather than through a locator also keeps the click out of Playwright's
|
||||
* element-stability wait, which never settles while the thread is mid-switch.
|
||||
*
|
||||
* The accessible name sits on the inner button `ConvoLink` renders, while the
|
||||
* click handler sits on the `convo-item` container around it — so match on
|
||||
* whichever node inside a row carries the label and let the click bubble.
|
||||
*/
|
||||
async function clickConversation(page: Page, title: string): Promise<void> {
|
||||
await sidebarRow(page, title).waitFor({ state: 'visible', timeout: 30_000 });
|
||||
await page.evaluate((label) => {
|
||||
const rows = document.querySelectorAll('[data-testid="convo-item"]');
|
||||
const row = Array.from(rows)
|
||||
.flatMap((element) => [element, ...Array.from(element.querySelectorAll('[aria-label]'))])
|
||||
.find((element) => element.getAttribute('aria-label') === label);
|
||||
if (!row) {
|
||||
throw new Error(`conversation row not found: ${label}`);
|
||||
}
|
||||
window.__NAV__.mark();
|
||||
(row as HTMLElement).click();
|
||||
}, `${title} conversation`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds `GET /api/convos/:id` open for one conversation and resolves to a
|
||||
* release function returning how many requests were held. Anything that awaits
|
||||
* that record before moving the route therefore cannot complete the switch
|
||||
* while the hold is in place, which is what makes the assertion independent of
|
||||
* how fast the database answers.
|
||||
*/
|
||||
async function holdConversationRecord(page: Page, conversationId: string) {
|
||||
const pattern = `**/api/convos/${conversationId}`;
|
||||
let release: () => void = () => undefined;
|
||||
const held = new Promise<void>((resolve) => {
|
||||
release = resolve;
|
||||
});
|
||||
let heldRequests = 0;
|
||||
|
||||
await page.route(pattern, async (route) => {
|
||||
heldRequests += 1;
|
||||
await held;
|
||||
await route.continue();
|
||||
});
|
||||
|
||||
return async () => {
|
||||
release();
|
||||
await page.unroute(pattern);
|
||||
return heldRequests;
|
||||
};
|
||||
}
|
||||
|
||||
async function switchTo(
|
||||
page: Page,
|
||||
target: { id: string; label: string; title: string },
|
||||
): Promise<SwitchTiming> {
|
||||
await page.evaluate(
|
||||
(markers) => {
|
||||
window.__NAV__.begin(markers);
|
||||
},
|
||||
CONVOS.map((convo) => convoMarker(convo.label)),
|
||||
);
|
||||
await clickConversation(page, target.title);
|
||||
await expect(threadHeading(page, target.label, 1)).toBeAttached({ timeout: 60_000 });
|
||||
const samples = await page.evaluate(() => window.__NAV__.end());
|
||||
return summarize(samples, `/c/${target.id}`, convoMarker(target.label));
|
||||
}
|
||||
|
||||
function reportTiming(name: string, timing: SwitchTiming): void {
|
||||
console.log(
|
||||
`${name.padEnd(22)} click→url=${String(timing.clickToUrlMs).padStart(5)}ms ` +
|
||||
`click→paint=${String(timing.clickToPaintMs).padStart(5)}ms ` +
|
||||
`stale-after-url=${String(timing.staleAfterUrlMs).padStart(5)}ms ` +
|
||||
`stale-frames=${timing.staleFrames}`,
|
||||
);
|
||||
}
|
||||
|
||||
async function attachTiming(testInfo: TestInfo, name: string, timing: SwitchTiming): Promise<void> {
|
||||
await testInfo.attach(name, {
|
||||
body: JSON.stringify(timing, null, 2),
|
||||
contentType: 'application/json',
|
||||
});
|
||||
}
|
||||
|
||||
test.describe('conversation navigation perf (react-scan)', () => {
|
||||
test.beforeAll(async () => {
|
||||
await clearUserConversations(userEmail);
|
||||
await seedConversations(
|
||||
userEmail,
|
||||
CONVOS.map((convo, index) => ({
|
||||
conversationId: convo.id,
|
||||
title: convo.title,
|
||||
updatedAt: new Date(Date.now() - index * 60_000),
|
||||
})),
|
||||
);
|
||||
for (const convo of CONVOS) {
|
||||
await seedMessages(userEmail, convo.id, buildConversationMessages(convo.label));
|
||||
}
|
||||
});
|
||||
|
||||
test.afterAll(async () => {
|
||||
const ids = CONVOS.map((convo) => convo.id);
|
||||
await deleteMessagesByConversation(ids);
|
||||
await deleteConversations(ids);
|
||||
});
|
||||
|
||||
test('switching between long conversations swaps the transcript with the URL', async ({
|
||||
page,
|
||||
}, testInfo) => {
|
||||
test.setTimeout(6 * 60 * 1000);
|
||||
|
||||
await installReactScan(page);
|
||||
await page.addInitScript({ content: NAV_SAMPLER });
|
||||
|
||||
await page.goto(`/c/${CONVO_A.id}`, { timeout: 120_000 });
|
||||
await expect(threadHeading(page, CONVO_A.label, 1)).toBeAttached({ timeout: 120_000 });
|
||||
await expect(sidebarRow(page, CONVO_B.title)).toBeVisible({ timeout: 30_000 });
|
||||
|
||||
/**
|
||||
* The cold switch is the first visit to B: its messages are not cached, so
|
||||
* the incoming tree is a spinner. Frames showing that spinner are not
|
||||
* stale — only frames showing conversation A are.
|
||||
*/
|
||||
await resetPerf(page);
|
||||
const cold = await switchTo(page, CONVO_B);
|
||||
const coldPerf = await snapshotPerf(page);
|
||||
|
||||
/**
|
||||
* The warm switch is the case users hit constantly — bouncing between two
|
||||
* conversations they have both already opened. Both message caches are
|
||||
* populated, so the incoming tree renders a full transcript rather than a
|
||||
* spinner; this is the switch that went stale.
|
||||
*
|
||||
* The conversation record request is held open across it. A wall-clock
|
||||
* bound could not tell the two implementations apart — against a local
|
||||
* Mongo an implementation that awaits the record still answers well inside
|
||||
* any threshold — so this asserts the property directly: the switch
|
||||
* completes while the request is still unresolved.
|
||||
*/
|
||||
await switchTo(page, CONVO_A);
|
||||
await expect(threadHeading(page, CONVO_A.label, 1)).toBeAttached({ timeout: 60_000 });
|
||||
await resetPerf(page);
|
||||
const releaseRecord = await holdConversationRecord(page, CONVO_B.id);
|
||||
const warm = await switchTo(page, CONVO_B);
|
||||
const heldRequests = await releaseRecord();
|
||||
const warmPerf = await snapshotPerf(page);
|
||||
|
||||
const coldTotals = totals(coldPerf);
|
||||
const warmTotals = totals(warmPerf);
|
||||
const coldTasks = longTaskStats(coldPerf);
|
||||
const warmTasks = longTaskStats(warmPerf);
|
||||
|
||||
console.log(
|
||||
`\n=== Conversation switch (${TURNS_PER_CONVO} turns / ${ROWS_PER_CONVO} rows each) ===`,
|
||||
);
|
||||
reportTiming('cold (uncached)', cold);
|
||||
reportTiming('warm (cached)', warm);
|
||||
console.log(
|
||||
`cold renders=${coldTotals.renders} render-time=${coldTotals.time.toFixed(0)}ms ` +
|
||||
`longtask-total=${coldTasks.total.toFixed(0)}ms worst=${coldTasks.worst.toFixed(0)}ms`,
|
||||
);
|
||||
for (const line of topComponents(coldPerf, 12)) {
|
||||
console.log(` ${line}`);
|
||||
}
|
||||
console.log(
|
||||
`warm renders=${warmTotals.renders} render-time=${warmTotals.time.toFixed(0)}ms ` +
|
||||
`longtask-total=${warmTasks.total.toFixed(0)}ms worst=${warmTasks.worst.toFixed(0)}ms`,
|
||||
);
|
||||
for (const line of topComponents(warmPerf, 12)) {
|
||||
console.log(` ${line}`);
|
||||
}
|
||||
|
||||
await attachTiming(testInfo, 'cold-switch.json', cold);
|
||||
await attachTiming(testInfo, 'warm-switch.json', warm);
|
||||
await attachSnapshot(testInfo, 'cold-switch-renders.json', coldPerf, {
|
||||
clickToPaintMs: cold.clickToPaintMs,
|
||||
staleAfterUrlMs: cold.staleAfterUrlMs,
|
||||
});
|
||||
await attachSnapshot(testInfo, 'warm-switch-renders.json', warmPerf, {
|
||||
clickToPaintMs: warm.clickToPaintMs,
|
||||
staleAfterUrlMs: warm.staleAfterUrlMs,
|
||||
});
|
||||
|
||||
/**
|
||||
* The sampler must have actually run: a zero-sample phase would satisfy
|
||||
* every upper bound below without observing anything.
|
||||
*/
|
||||
expect(cold.samples).toBeGreaterThan(1);
|
||||
expect(warm.samples).toBeGreaterThan(1);
|
||||
|
||||
/**
|
||||
* THE core guard: once the URL names the next conversation, the previous
|
||||
* transcript must not still be what is painted. Both fixes this benchmark
|
||||
* was written for converge here — the route now commits in the click's own
|
||||
* task with the conversation state, so the swap is atomic and no frame
|
||||
* shows the wrong pairing.
|
||||
*
|
||||
* Measured against the production build with a 250ms conversation-fetch
|
||||
* latency: before, the outgoing transcript held for 12-14 frames
|
||||
* (~280-300ms) on every switch; after, zero frames. A couple of frames of
|
||||
* slack absorbs scheduler noise; anything more means the swap stopped
|
||||
* being atomic — most likely a route update back on React's transition
|
||||
* lane, which paints the outgoing tree until the incoming one is ready.
|
||||
*
|
||||
* This holds for the cold switch too: waiting for the record there delays
|
||||
* the URL, it does not desynchronise it from the transcript.
|
||||
*/
|
||||
expect(cold.staleFrames).toBeLessThanOrEqual(2);
|
||||
expect(warm.staleFrames).toBeLessThanOrEqual(2);
|
||||
expect(cold.staleAfterUrlMs).toBeLessThan(120);
|
||||
expect(warm.staleAfterUrlMs).toBeLessThan(120);
|
||||
|
||||
/**
|
||||
* A warm switch must not wait on the conversation record: the whole switch
|
||||
* above completed while that request was held open. The hold must have
|
||||
* actually engaged — zero held requests would mean the route pattern
|
||||
* stopped matching and the assertion proved nothing.
|
||||
*
|
||||
* The cold switch is deliberately NOT bounded this way. A conversation
|
||||
* with no cached record still waits for it, because the sidebar row is a
|
||||
* projection without the prompt prefix, sampling params, tools and files a
|
||||
* send needs; landing the route on that would expose a composer whose
|
||||
* sends silently carry defaults.
|
||||
*/
|
||||
expect(heldRequests).toBeGreaterThan(0);
|
||||
expect(warm.clickToUrlMs).toBeLessThan(400);
|
||||
|
||||
/** End to end, both switches stay inside a responsive budget
|
||||
* (measured after: ~250ms warm, ~450ms cold). */
|
||||
expect(cold.clickToPaintMs).toBeLessThan(900);
|
||||
expect(warm.clickToPaintMs).toBeLessThan(900);
|
||||
|
||||
/**
|
||||
* The commit that swaps the transcript is now synchronous, so it must stay
|
||||
* small enough not to read as a freeze — a single stall past this bound
|
||||
* means the incoming thread's first commit stopped being windowed
|
||||
* (measured after: 285-356ms worst).
|
||||
*/
|
||||
expect(warmTasks.worst).toBeLessThan(600);
|
||||
|
||||
/**
|
||||
* Component names are mangled in the built client, so the per-component
|
||||
* tally above is diagnostic only; the TOTAL is still comparable and is
|
||||
* what catches a subscription regression that re-renders the app on every
|
||||
* route change (measured after: ~2.8k warm, ~3.4k cold, dominated by the
|
||||
* per-row hover-button chrome each message mounts).
|
||||
*/
|
||||
expect(warmTotals.renders).toBeGreaterThan(100);
|
||||
expect(warmTotals.renders).toBeLessThan(9000);
|
||||
});
|
||||
});
|
||||
82
e2e/benchmarks-navigation/payload.ts
Normal file
82
e2e/benchmarks-navigation/payload.ts
Normal file
|
|
@ -0,0 +1,82 @@
|
|||
import type { SeedMessage } from '../specs/mock/db';
|
||||
|
||||
/**
|
||||
* Deterministic transcripts for the conversation-navigation perf benchmark.
|
||||
*
|
||||
* Two seeded conversations of the same shape, each long enough that its first
|
||||
* commit is real work (well past `MIN_PROGRESSIVE_ROWS`), so switching between
|
||||
* them measures the navigation path a user actually feels — not a two-message
|
||||
* toy thread that renders in a single frame regardless.
|
||||
*/
|
||||
|
||||
/** Turns per seeded conversation; one turn is a user + assistant pair. */
|
||||
export const TURNS_PER_CONVO = 30;
|
||||
|
||||
/** Rows per conversation — what `useProgressiveRowMount` windows over. */
|
||||
export const ROWS_PER_CONVO = TURNS_PER_CONVO * 2;
|
||||
|
||||
const SENTENCE =
|
||||
'The migration plan sequences every dependent service behind a single feature flag so rollbacks stay one toggle away. ';
|
||||
|
||||
/**
|
||||
* Per-conversation marker carried by EVERY row. The in-page sampler reads it
|
||||
* off whichever row happens to be mounted, so it identifies the painted
|
||||
* transcript without depending on which slice of the thread the progressive
|
||||
* mount window admitted first.
|
||||
*/
|
||||
export function convoMarker(label: string): string {
|
||||
return `NAVBENCH-${label}`;
|
||||
}
|
||||
|
||||
/** Rendered heading text unique to one conversation, for Playwright locators. */
|
||||
export function turnHeading(label: string, turn: number): string {
|
||||
return `${convoMarker(label)} section ${turn}`;
|
||||
}
|
||||
|
||||
function assistantBody(label: string, turn: number): string {
|
||||
const table =
|
||||
'| Service | Requests | Growth |\n| --- | --- | --- |\n' +
|
||||
'| gateway | 120000 | 4% |\n| worker | 135500 | 12% |\n';
|
||||
const code =
|
||||
'```ts\nexport function rollout(stage: number): boolean {\n return stage > 0;\n}\n```\n';
|
||||
let body = `## ${turnHeading(label, turn)}\n\n${SENTENCE}${SENTENCE}\n\n`;
|
||||
body += `- ${convoMarker(label)} point one for turn ${turn}\n`;
|
||||
body += `- ${convoMarker(label)} point two for turn ${turn}\n\n`;
|
||||
if (turn % 3 === 0) {
|
||||
body += `${code}\n`;
|
||||
}
|
||||
if (turn % 4 === 0) {
|
||||
body += `${table}\n`;
|
||||
}
|
||||
return body;
|
||||
}
|
||||
|
||||
/**
|
||||
* Linear thread (no siblings): every message parents the previous one, so the
|
||||
* visible path is the whole conversation and `latestMessageDepth` equals
|
||||
* `ROWS_PER_CONVO - 1`.
|
||||
*/
|
||||
export function buildConversationMessages(label: string): SeedMessage[] {
|
||||
const messages: SeedMessage[] = [];
|
||||
let parentMessageId = '00000000-0000-0000-0000-000000000000';
|
||||
for (let turn = 1; turn <= TURNS_PER_CONVO; turn += 1) {
|
||||
const userMessageId = `${label}-user-${turn}`;
|
||||
messages.push({
|
||||
messageId: userMessageId,
|
||||
parentMessageId,
|
||||
text: `${convoMarker(label)} prompt ${turn}: walk me through the rollout for stage ${turn}.`,
|
||||
isCreatedByUser: true,
|
||||
sender: 'User',
|
||||
});
|
||||
const assistantMessageId = `${label}-assistant-${turn}`;
|
||||
messages.push({
|
||||
messageId: assistantMessageId,
|
||||
parentMessageId: userMessageId,
|
||||
text: assistantBody(label, turn),
|
||||
isCreatedByUser: false,
|
||||
sender: 'Assistant',
|
||||
});
|
||||
parentMessageId = assistantMessageId;
|
||||
}
|
||||
return messages;
|
||||
}
|
||||
|
|
@ -1,6 +1,4 @@
|
|||
import fs from 'node:fs';
|
||||
import { expect, test } from '@playwright/test';
|
||||
import type { Page, TestInfo } from '@playwright/test';
|
||||
import {
|
||||
MOCK_ENDPOINTS,
|
||||
NEW_CHAT_PATH,
|
||||
|
|
@ -15,194 +13,14 @@ import {
|
|||
END_MARKER,
|
||||
SENTENCE,
|
||||
} from './payload';
|
||||
|
||||
type RenderTally = Record<string, { count: number; time: number }>;
|
||||
|
||||
type PerfSnapshot = {
|
||||
renders: RenderTally;
|
||||
longTasks: number[];
|
||||
/** Milliseconds between the phase's anchor render and this snapshot, on
|
||||
* the page's own clock: the first ThinkingContent render (assistant
|
||||
* content = stream start) when one occurred, else the first render after
|
||||
* the reset (typing phase), else the reset itself. Idle request-setup
|
||||
* time before the assistant renders never pads the interval. */
|
||||
elapsedMs: number;
|
||||
};
|
||||
|
||||
type PerfGlobal = {
|
||||
renders: RenderTally;
|
||||
longTasks: number[];
|
||||
startedAt: number;
|
||||
firstRenderAt: number | null;
|
||||
firstStreamRenderAt: number | null;
|
||||
drain(): void;
|
||||
reset(): void;
|
||||
};
|
||||
|
||||
declare global {
|
||||
interface Window {
|
||||
__PERF__: PerfGlobal;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The react-scan bundle is injected from disk so the repo does not need it as
|
||||
* a dependency; point REACT_SCAN_PATH at `react-scan/dist/auto.global.js`.
|
||||
*/
|
||||
function resolveReactScanPath(): string {
|
||||
const fromEnv = process.env.REACT_SCAN_PATH;
|
||||
if (fromEnv && fs.existsSync(fromEnv)) {
|
||||
return fromEnv;
|
||||
}
|
||||
return require.resolve('react-scan/dist/auto.global.js');
|
||||
}
|
||||
|
||||
const TALLY_SETUP = `(() => {
|
||||
const perf = {
|
||||
renders: Object.create(null),
|
||||
longTasks: [],
|
||||
observer: null,
|
||||
startedAt: performance.now(),
|
||||
firstRenderAt: null,
|
||||
firstStreamRenderAt: null,
|
||||
drain() {
|
||||
if (!this.observer) {
|
||||
return;
|
||||
}
|
||||
for (const entry of this.observer.takeRecords()) {
|
||||
this.longTasks.push(entry.duration);
|
||||
}
|
||||
},
|
||||
reset() {
|
||||
this.drain();
|
||||
this.renders = Object.create(null);
|
||||
this.longTasks = [];
|
||||
this.startedAt = performance.now();
|
||||
this.firstRenderAt = null;
|
||||
this.firstStreamRenderAt = null;
|
||||
},
|
||||
};
|
||||
window.__PERF__ = perf;
|
||||
try {
|
||||
perf.observer = new PerformanceObserver((list) => {
|
||||
for (const entry of list.getEntries()) {
|
||||
perf.longTasks.push(entry.duration);
|
||||
}
|
||||
});
|
||||
perf.observer.observe({ type: 'longtask', buffered: true });
|
||||
} catch (_error) {
|
||||
/* longtask unsupported: totals stay empty */
|
||||
}
|
||||
const nameOf = (fiber) => {
|
||||
let type = fiber && fiber.type;
|
||||
for (let depth = 0; depth < 4 && type; depth += 1) {
|
||||
if (typeof type === 'function') {
|
||||
return type.displayName || type.name || null;
|
||||
}
|
||||
if (typeof type === 'object') {
|
||||
if (type.displayName) {
|
||||
return type.displayName;
|
||||
}
|
||||
type = type.type || type.render;
|
||||
continue;
|
||||
}
|
||||
return String(type);
|
||||
}
|
||||
return null;
|
||||
};
|
||||
const configure = () => {
|
||||
if (typeof window.reactScan !== 'function') {
|
||||
return false;
|
||||
}
|
||||
window.reactScan({
|
||||
enabled: true,
|
||||
log: false,
|
||||
showToolbar: false,
|
||||
animationSpeed: 'off',
|
||||
trackUnnecessaryRenders: false,
|
||||
dangerouslyForceRunInProduction: true,
|
||||
onRender: (fiber, renders) => {
|
||||
if (perf.firstRenderAt == null) {
|
||||
perf.firstRenderAt = performance.now();
|
||||
}
|
||||
for (const render of renders) {
|
||||
const name = render.componentName || nameOf(fiber) || 'anonymous';
|
||||
if (perf.firstStreamRenderAt == null && name === 'ThinkingContent') {
|
||||
perf.firstStreamRenderAt = performance.now();
|
||||
}
|
||||
let slot = perf.renders[name];
|
||||
if (!slot) {
|
||||
slot = { count: 0, time: 0 };
|
||||
perf.renders[name] = slot;
|
||||
}
|
||||
slot.count += render.count || 1;
|
||||
slot.time += render.time || 0;
|
||||
}
|
||||
},
|
||||
});
|
||||
return true;
|
||||
};
|
||||
if (!configure()) {
|
||||
const timer = setInterval(() => {
|
||||
if (configure()) {
|
||||
clearInterval(timer);
|
||||
}
|
||||
}, 50);
|
||||
}
|
||||
})();`;
|
||||
|
||||
async function snapshotPerf(page: Page): Promise<PerfSnapshot> {
|
||||
return page.evaluate(() => {
|
||||
window.__PERF__.drain();
|
||||
return {
|
||||
renders: window.__PERF__.renders,
|
||||
longTasks: window.__PERF__.longTasks.slice(),
|
||||
elapsedMs:
|
||||
performance.now() -
|
||||
(window.__PERF__.firstStreamRenderAt ??
|
||||
window.__PERF__.firstRenderAt ??
|
||||
window.__PERF__.startedAt),
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
async function resetPerf(page: Page): Promise<void> {
|
||||
await page.evaluate(() => {
|
||||
window.__PERF__.reset();
|
||||
});
|
||||
}
|
||||
|
||||
function totals(snapshot: PerfSnapshot): { renders: number; time: number } {
|
||||
let renders = 0;
|
||||
let time = 0;
|
||||
for (const slot of Object.values(snapshot.renders)) {
|
||||
renders += slot.count;
|
||||
time += slot.time;
|
||||
}
|
||||
return { renders, time };
|
||||
}
|
||||
|
||||
function topComponents(snapshot: PerfSnapshot, limit: number): string[] {
|
||||
return Object.entries(snapshot.renders)
|
||||
.sort((a, b) => b[1].count - a[1].count)
|
||||
.slice(0, limit)
|
||||
.map(
|
||||
([name, slot]) =>
|
||||
`${name.padEnd(28)} renders=${String(slot.count).padStart(6)} time=${slot.time.toFixed(1)}ms`,
|
||||
);
|
||||
}
|
||||
|
||||
async function attachSnapshot(
|
||||
testInfo: TestInfo,
|
||||
name: string,
|
||||
snapshot: PerfSnapshot,
|
||||
extra: Record<string, number>,
|
||||
): Promise<void> {
|
||||
await testInfo.attach(name, {
|
||||
body: JSON.stringify({ ...extra, ...snapshot }, null, 2),
|
||||
contentType: 'application/json',
|
||||
});
|
||||
}
|
||||
import {
|
||||
attachSnapshot,
|
||||
installReactScan,
|
||||
resetPerf,
|
||||
snapshotPerf,
|
||||
topComponents,
|
||||
totals,
|
||||
} from '../perf/scan';
|
||||
|
||||
test.describe('reasoning stream perf (react-scan)', () => {
|
||||
test('one long unsplit reasoning + markdown reply stays render-bounded', async ({
|
||||
|
|
@ -216,8 +34,10 @@ test.describe('reasoning stream perf (react-scan)', () => {
|
|||
const textChunks = countModelChunks(textSection);
|
||||
const sectionCount = (textSection.match(/## Section /g) ?? []).length;
|
||||
|
||||
await page.addInitScript({ content: fs.readFileSync(resolveReactScanPath(), 'utf8') });
|
||||
await page.addInitScript({ content: TALLY_SETUP });
|
||||
/** The payload always opens with reasoning, so the first ThinkingContent
|
||||
* render is the first assistant-content paint — anchor the measured
|
||||
* interval there. */
|
||||
await installReactScan(page, 'ThinkingContent');
|
||||
/** Stream with the reasoning box EXPANDED — the heavier layout path a
|
||||
* user gets with "Show Thinking" enabled — so the measured interval
|
||||
* covers live paragraph layout inside the box, not just the collapsed
|
||||
|
|
|
|||
220
e2e/perf/scan.ts
Normal file
220
e2e/perf/scan.ts
Normal file
|
|
@ -0,0 +1,220 @@
|
|||
import fs from 'node:fs';
|
||||
import type { Page, TestInfo } from '@playwright/test';
|
||||
|
||||
/**
|
||||
* Shared react-scan instrumentation for the render-perf benchmarks.
|
||||
*
|
||||
* react-scan is injected from disk rather than depended on, so the repo does
|
||||
* not carry it; point `REACT_SCAN_PATH` at `react-scan/dist/auto.global.js`.
|
||||
* Baselines are version-sensitive (instrumentation overhead and `onRender`
|
||||
* semantics both move between releases), so keep the version pinned to the one
|
||||
* each benchmark's README records.
|
||||
*/
|
||||
|
||||
export type RenderTally = Record<string, { count: number; time: number }>;
|
||||
|
||||
export type PerfSnapshot = {
|
||||
renders: RenderTally;
|
||||
longTasks: number[];
|
||||
/** Milliseconds between the phase's anchor render and this snapshot, on the
|
||||
* page's own clock: the first `anchorComponent` render when one occurred,
|
||||
* else the first render after the reset, else the reset itself. Idle setup
|
||||
* time before anything renders never pads the interval. */
|
||||
elapsedMs: number;
|
||||
};
|
||||
|
||||
type PerfGlobal = {
|
||||
renders: RenderTally;
|
||||
longTasks: number[];
|
||||
startedAt: number;
|
||||
firstRenderAt: number | null;
|
||||
firstAnchorRenderAt: number | null;
|
||||
drain(): void;
|
||||
reset(): void;
|
||||
};
|
||||
|
||||
declare global {
|
||||
interface Window {
|
||||
__PERF__: PerfGlobal;
|
||||
}
|
||||
}
|
||||
|
||||
export function resolveReactScanPath(): string {
|
||||
const fromEnv = process.env.REACT_SCAN_PATH;
|
||||
if (fromEnv && fs.existsSync(fromEnv)) {
|
||||
return fromEnv;
|
||||
}
|
||||
return require.resolve('react-scan/dist/auto.global.js');
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the page-side tally script. `anchorComponent` names the component
|
||||
* whose first render starts the measured interval; omit it to anchor on the
|
||||
* first render of any component.
|
||||
*/
|
||||
export function buildTallySetup(anchorComponent?: string): string {
|
||||
const anchor = JSON.stringify(anchorComponent ?? null);
|
||||
return `(() => {
|
||||
const ANCHOR = ${anchor};
|
||||
const perf = {
|
||||
renders: Object.create(null),
|
||||
longTasks: [],
|
||||
observer: null,
|
||||
startedAt: performance.now(),
|
||||
firstRenderAt: null,
|
||||
firstAnchorRenderAt: null,
|
||||
drain() {
|
||||
if (!this.observer) {
|
||||
return;
|
||||
}
|
||||
for (const entry of this.observer.takeRecords()) {
|
||||
this.longTasks.push(entry.duration);
|
||||
}
|
||||
},
|
||||
reset() {
|
||||
this.drain();
|
||||
this.renders = Object.create(null);
|
||||
this.longTasks = [];
|
||||
this.startedAt = performance.now();
|
||||
this.firstRenderAt = null;
|
||||
this.firstAnchorRenderAt = null;
|
||||
},
|
||||
};
|
||||
window.__PERF__ = perf;
|
||||
try {
|
||||
perf.observer = new PerformanceObserver((list) => {
|
||||
for (const entry of list.getEntries()) {
|
||||
perf.longTasks.push(entry.duration);
|
||||
}
|
||||
});
|
||||
perf.observer.observe({ type: 'longtask', buffered: true });
|
||||
} catch (_error) {
|
||||
/* longtask unsupported: totals stay empty */
|
||||
}
|
||||
const nameOf = (fiber) => {
|
||||
let type = fiber && fiber.type;
|
||||
for (let depth = 0; depth < 4 && type; depth += 1) {
|
||||
if (typeof type === 'function') {
|
||||
return type.displayName || type.name || null;
|
||||
}
|
||||
if (typeof type === 'object') {
|
||||
if (type.displayName) {
|
||||
return type.displayName;
|
||||
}
|
||||
type = type.type || type.render;
|
||||
continue;
|
||||
}
|
||||
return String(type);
|
||||
}
|
||||
return null;
|
||||
};
|
||||
const configure = () => {
|
||||
if (typeof window.reactScan !== 'function') {
|
||||
return false;
|
||||
}
|
||||
window.reactScan({
|
||||
enabled: true,
|
||||
log: false,
|
||||
showToolbar: false,
|
||||
animationSpeed: 'off',
|
||||
trackUnnecessaryRenders: false,
|
||||
dangerouslyForceRunInProduction: true,
|
||||
onRender: (fiber, renders) => {
|
||||
if (perf.firstRenderAt == null) {
|
||||
perf.firstRenderAt = performance.now();
|
||||
}
|
||||
for (const render of renders) {
|
||||
const name = render.componentName || nameOf(fiber) || 'anonymous';
|
||||
if (ANCHOR != null && perf.firstAnchorRenderAt == null && name === ANCHOR) {
|
||||
perf.firstAnchorRenderAt = performance.now();
|
||||
}
|
||||
let slot = perf.renders[name];
|
||||
if (!slot) {
|
||||
slot = { count: 0, time: 0 };
|
||||
perf.renders[name] = slot;
|
||||
}
|
||||
slot.count += render.count || 1;
|
||||
slot.time += render.time || 0;
|
||||
}
|
||||
},
|
||||
});
|
||||
return true;
|
||||
};
|
||||
if (!configure()) {
|
||||
const timer = setInterval(() => {
|
||||
if (configure()) {
|
||||
clearInterval(timer);
|
||||
}
|
||||
}, 50);
|
||||
}
|
||||
})();`;
|
||||
}
|
||||
|
||||
/** Injects react-scan plus the tally script before any app code runs. */
|
||||
export async function installReactScan(page: Page, anchorComponent?: string): Promise<void> {
|
||||
await page.addInitScript({ content: fs.readFileSync(resolveReactScanPath(), 'utf8') });
|
||||
await page.addInitScript({ content: buildTallySetup(anchorComponent) });
|
||||
}
|
||||
|
||||
export async function snapshotPerf(page: Page): Promise<PerfSnapshot> {
|
||||
return page.evaluate(() => {
|
||||
window.__PERF__.drain();
|
||||
return {
|
||||
renders: window.__PERF__.renders,
|
||||
longTasks: window.__PERF__.longTasks.slice(),
|
||||
elapsedMs:
|
||||
performance.now() -
|
||||
(window.__PERF__.firstAnchorRenderAt ??
|
||||
window.__PERF__.firstRenderAt ??
|
||||
window.__PERF__.startedAt),
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
export async function resetPerf(page: Page): Promise<void> {
|
||||
await page.evaluate(() => {
|
||||
window.__PERF__.reset();
|
||||
});
|
||||
}
|
||||
|
||||
export function totals(snapshot: PerfSnapshot): { renders: number; time: number } {
|
||||
let renders = 0;
|
||||
let time = 0;
|
||||
for (const slot of Object.values(snapshot.renders)) {
|
||||
renders += slot.count;
|
||||
time += slot.time;
|
||||
}
|
||||
return { renders, time };
|
||||
}
|
||||
|
||||
export function topComponents(snapshot: PerfSnapshot, limit: number): string[] {
|
||||
return Object.entries(snapshot.renders)
|
||||
.sort((a, b) => b[1].count - a[1].count)
|
||||
.slice(0, limit)
|
||||
.map(
|
||||
([name, slot]) =>
|
||||
`${name.padEnd(28)} renders=${String(slot.count).padStart(6)} time=${slot.time.toFixed(1)}ms`,
|
||||
);
|
||||
}
|
||||
|
||||
export function longTaskStats(snapshot: PerfSnapshot): { total: number; worst: number } {
|
||||
let total = 0;
|
||||
let worst = 0;
|
||||
for (const duration of snapshot.longTasks) {
|
||||
total += duration;
|
||||
worst = Math.max(worst, duration);
|
||||
}
|
||||
return { total, worst };
|
||||
}
|
||||
|
||||
export async function attachSnapshot(
|
||||
testInfo: TestInfo,
|
||||
name: string,
|
||||
snapshot: PerfSnapshot,
|
||||
extra: Record<string, number>,
|
||||
): Promise<void> {
|
||||
await testInfo.attach(name, {
|
||||
body: JSON.stringify({ ...extra, ...snapshot }, null, 2),
|
||||
contentType: 'application/json',
|
||||
});
|
||||
}
|
||||
26
e2e/playwright.config.navigation-perf.ts
Normal file
26
e2e/playwright.config.navigation-perf.ts
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
import { defineConfig } from '@playwright/test';
|
||||
import mockConfig from './playwright.config.mock';
|
||||
|
||||
/**
|
||||
* Conversation-navigation perf benchmark config.
|
||||
*
|
||||
* Seeds two long conversations directly in Mongo and switches between them so
|
||||
* react-scan can measure what the sidebar's most-used interaction costs — and,
|
||||
* above all, whether the painted transcript keeps up with the URL.
|
||||
*
|
||||
* Unlike the reasoning-stream benchmark this runs against the BUILT client
|
||||
* (`client/dist`, served by the mock app server) rather than the vite dev
|
||||
* server: the assertions here are wall-clock budgets, and a dev build's module
|
||||
* graph and unminified render path inflate them past anything a user would
|
||||
* see. The tradeoff is that the production minifier (oxc) strips component
|
||||
* names, so react-scan's per-component tally is mangled — total render counts
|
||||
* and long tasks still hold. See the README for getting names back.
|
||||
*/
|
||||
export default defineConfig({
|
||||
...mockConfig,
|
||||
testDir: 'benchmarks-navigation',
|
||||
outputDir: 'benchmarks-navigation/.test-results',
|
||||
timeout: 10 * 60 * 1000,
|
||||
retries: 0,
|
||||
reporter: [['line']],
|
||||
});
|
||||
|
|
@ -69,6 +69,8 @@
|
|||
"e2e:mock:redis": "npm run e2e:prepare && cross-env E2E_STREAM_STORE=redis playwright test --config=e2e/playwright.config.mock.ts",
|
||||
"e2e:mock:redis:transport": "npm run e2e:prepare && cross-env E2E_STREAM_STORE=redis playwright test --config=e2e/playwright.config.redis.ts",
|
||||
"e2e:benchmark:agents": "npm run e2e:prepare && playwright test --config=e2e/playwright.config.benchmark.ts agent-startup.latency.spec.ts",
|
||||
"e2e:benchmark:navigation": "npm run e2e:prepare && playwright test --config=e2e/playwright.config.navigation-perf.ts",
|
||||
"e2e:benchmark:reasoning": "npm run e2e:prepare && playwright test --config=e2e/playwright.config.reasoning-perf.ts",
|
||||
"e2e:bombadil": "npm run e2e:prepare && playwright test --config=e2e/playwright.config.bombadil.ts",
|
||||
"e2e:bombadil:run": "playwright test --config=e2e/playwright.config.bombadil.ts",
|
||||
"e2e:bombadil:branch-reload": "npm run e2e:prepare && cross-env BOMBADIL_SPECIFICATION=branch-reload.specification.ts BOMBADIL_TIME_LIMIT=30s playwright test --config=e2e/playwright.config.bombadil.ts",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue