mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 20:24:21 +00:00
393742016e
* ⚡ 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> |
||
|---|---|---|
| .. | ||
| navigation.perf.spec.ts | ||
| payload.ts | ||
| README.md | ||
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: 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:
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.