fix composer lag in long conversations (#236) by daniilperkin · Pull Request #274 · SprintStartProject/sprintstart-frontend

daniilperkin · 2026-09-28T08:03:59Z

What this fixes

Closes #236 — typing in the buddy composer gets sluggish as a conversation grows (20–50+ messages with markdown, tool lines and action proposals). In a long thread each keystroke re-rendered the whole conversation and re-parsed every reply's markdown.

Three causes, all present in dev:

The draft lived inside the session. useBuddyConversation owned both draft and messages and publishes its entire return value through one context, so every setDraft handed every reader a new session value — BuddyPage and the whole thread with it, plus BuddyDock and BuddyWidget.
The transcript was unmemoised and its props were fresh on every render. BuddyThread/BuddyMessage had no React.memo, and callers passed inline arrows for renderReplyAction, renderQuestionAction and lastMessageFooter — so any re-render of a caller re-parsed every reply.
Tokens compounded it. Each streamed token changed messages, re-rendering the whole thread per token.

How it is fixed — six commits, three passes

Pass 1 — the page's keystroke path (dcdcf6e4, 46a6a48a).

BuddyDraftProvider owns draft/setDraft/handleSubmit and sits below BuddyProvider, so the page, the dock and the widget are children and a keystroke re-renders only the box. It publishes two contexts: the value (read by the composer alone) and a write-only setDraft whose identity never changes — which keeps the suggestion chips off the per-keystroke path as well. useBuddyConversation keeps only draftResetToken, because a fresh visit or a project switch must still empty the box. handleSubmit's contract is unchanged: egg phrases play their effect, are swallowed, and return false so the composer keeps the caret (the caret tests still pin it).

Then BuddyThread is memoised and each turn became a memoised BuddyThreadRow (the shape MessageRow in the chat already uses); BuddyMarkdown is memoised on content; and every callback the thread compares is held in one identity by its callers, with the contract documented on the props. A token now re-renders one row — its own.

Pass 2 — after a deep review of this PR (62bc4a82, 0ff67a26, a5c17656, a05f518c).

The review found one real leak Pass 1 had left, one long-standing bug in the code this PR touches, and cruft in the same corner. All of it is fixed here:

Finding	What it was	Fix
The newest reply's bot never slept	The thread asked its rows "is this the newest message?" where it meant "is this the turn receiving tokens right now". `BuddyMessage` hands that flag to `SleepyBot` as `canSleep={!isStreaming}`, so the newest reply was flagged as streaming for the rest of the conversation. On `dev` too: the buddy's row was forked from the chat's `MessageRow` without the null that `streamingMessageId` has when idle	`Boolean(isStreaming && …)`, plus `BuddyThreadStreaming` — which fails 2/2 on the parent commit
The widget still re-rendered per keystroke	It read the draft through `useBuddy()`, and the widget mounts on every page: a keystroke in the dock re-rendered the widget, the dock, its header and every bubble in it — and on `/buddy` it re-rendered on every character while drawing nothing	`useBuddy()` no longer reads the draft (the "Draft with AI" seeding effect takes the write-only half). Measured over eighteen keystrokes: 51 floating-surface renders before, 0 after; on `/buddy`, 17 → 0
The history-state hand-off was a second mechanism for nothing	`goToPage` passed the words in `location.state` and `useHandedOffDraft` applied them on arrival — but `BuddyDraftProvider` sits above the router, so both surfaces already read one box. It was also the reason `goToPage` depended on the draft, and so the root of the leak above	`useHandedOffDraft` retired; the hand-off is `navigate("/buddy")`; `buddyDraftAcrossRoutes` pins it, including that clearing the box survives a back-and-forward that used to re-seed it from the history entry
`dismissAction` searched the transcript by id	Which is what forced the `messagesRef` (and the question of whether a ref can be a render behind a click) to keep that callback's identity stable for the memoised rows. `confirmAction` has always taken the action object from the card that drew it	`dismissAction(messageId, action)`; the lookup, the ref and its effect are gone
Dead `before` prop	Declared in two prop types and threaded through one render; no caller has passed it since the rail took over "what came back from the PM"	Removed from both

On top of that, the dock and its header control are memoised: every prop the dock takes is a plain value, a callback the widget holds in one identity, or a motion value, so a render of the widget that changes none of them (a resize while it is open, a drag) no longer walks the panel.

Verification (real output — `npm run try` on `a05f518c`)

Step	Result
`format:check`	All matched files use Prettier code style
`build` (`tsc -b && vite build`)	✓ built in 1.44 s
`lint` (`eslint .`)	clean
unit (`vitest run --exclude tests/unit/a11y`)	330 files / 3199 tests passed
a11y (`vitest run tests/unit/a11y/`)	55 files / 69 tests passed

Every commit also builds on its own (npx tsc -b after each), so the branch is green at every step rather than only at the tip.

New tests, each one failing on the commit before it:

BuddyComposerIsolation — /buddy with a 32-message transcript under the real provider, the markdown renderer stubbed to count parses: typing an 18-character question → 0 parses; one streamed token → exactly 1 (its own reply). It also mounts the widget/dock for the floating half: typing in the dock → 0 renders of the widget/dock and 0 parses.
BuddyThreadStreaming — the bot's flag per row: nothing at rest, the live turn while it runs, nothing again once it lands.
buddyDraftAcrossRoutes — the words cross because one box lives above the router, and clearing them on the page survives back-and-forward.

Overlap with open PRs (measured, not estimated)

git merge-tree --write-tree against #267's branch (feature/311-buddy-onboarding-tutor) reports one content conflict — BuddyComposer.tsx — with every other file auto-merging. The two branches touch three files in common (BuddyComposer, BuddyMarkdown, useBuddyConversation); the rest of #267's 72 changed files are elsewhere (board path cards, a deleted card-blueprint service, the onboarding path). It does not delete BuddyComposerCaret.test.tsx, and it does not touch the dino props. Whichever of the two lands second rebases one file.

#271 (hotfix/233-…) landed on dev while this branch was open. Its source changes (board-save, query invalidation) merged without a conflict, but the tests it added were written against useBuddy() still owning the draft — the coupling this PR removes — so its three new render sites were ported to the harness's helper (see below). #272 is stacked on #267 and inherits the same small rebase. #266 (citations, queue, stop) plans its work in useBuddyConversation and asked for this fix first; it now builds on a smaller render surface.

Deliberately out of scope

No content-visibility or virtualisation of the transcript: with parsing isolated, rows are cheap to render, and a virtual list changes scroll and search behaviour enough to deserve its own evaluation.
No memoised SleepyBot/Framer Motion internals: a re-render no longer reaches them per keystroke, so it would be churn without a measured win.

Third pass — the review of the fix

A second read (of the fix itself, not the original) found one overlap that looked reachable, one
memo boundary that was only covered by luck, and two nits. All four are addressed in aaf14043,
ff2e1161 and 5655e86e:

The opener suggestion vs a reply's own proposals. The footer follows the newest reply now,
which includes a proposal-only one — so a reply carrying a card would have drawn both the card and
the greeting's suggestion under one bubble. It cannot happen: streamOpenBuddy has no
action_proposal case and never writes actions onto the greeting, so a text-less
greeting-with-proposal is not buildable by this client. The line is held anyway
(!message.actions?.length), and BuddyThreadFooter pins it — its second test fails on the parent
commit with exactly that overlap. The same file pins the other rule: the suggestion follows the
newest reply, proposal-only or not, instead of falling back onto an older bubble.
BuddyConversation joined the memo boundary (the thread, its rows and the dock already had it),
together with the one prop that would have silently defeated it: the page's aboveComposer chips
element was built inline in the render and is now held in a useMemo. Its props list is written
down as the contract it is.
The "adjust state during render" pattern is named once. It appears in eight files with eight
slightly different explanations — enough for a reviewer to read the two this PR touches as
independent inventions. The pattern and its three rules (own state only, behind an inequality
check, never a ref) now live in CODING_STANDARDS.md § 3, and the two comments here keep only
their local why, pointing there for the how.

Verification at the tip: npm run try exit 0 — prettier clean, build 1.46 s, lint clean, unit
331 files / 3,201 tests, a11y 55 files / 69 tests; npx tsc -b clean after each of the nine
commits.

`dev` moved mid-flight

PR #271 merged into dev (7b481d63) while this PR was open. The source merge was clean — no
conflicts — and the honest signal came from CI: the pull-request run lints the merge with dev,
and #271's new board-sync tests in useBuddy.test.tsx call useBuddy() + setDraft() /
handleSubmit(), exactly the API this PR splits out. Its three new render sites go through the
harness's useBuddyWithDraft() now, like every other sending test in the file; the merge
(b9c21766) and the port (9cae8c6a) are on the branch, and the merged buddy + board suites run
63 files / 600 tests. Nothing else from #271 needed a decision: its useBuddyConversation work
merged beside the dismissAction change without a conflict, and messagesRef stays gone.

#236) Typing in the buddy composer grew sluggish as a conversation grew: in a 50-message thread the box lagged for tens to hundreds of milliseconds per character. The composer was not the cost — everything the words were attached to was. Both the draft and the transcript were owned by `useBuddyConversation`, whose entire return value is published through one context. `setDraft` on every keystroke therefore produced a new session value, and React re-rendered every reader of it: on `/buddy` the page itself, and with it the whole thread — each reply re-run through `ReactMarkdown` with `remarkGfm` (a full AST parse per reply per character), each bubble re-running its animation hooks, plus the dock and the widget for the same session. This commit takes the draft out of that path: * `BuddyDraftProvider` (new) owns the words, and sits *below* the session provider. The page, the dock and the widget are its `children`, whose elements never change identity, so a keystroke now re-renders the box and nothing else. * Two contexts, not one, because the halves age differently. The value (`draft`/`setDraft`/`handleSubmit`) changes per keystroke and only the composer reads it; the write-only `BuddyDraftActions` never changes identity, so the surfaces that merely *fill* the box — the suggestion chips, the dock's hand-off to `/buddy` — stay off the per-keystroke path entirely. * `handleSubmit` keeps its exact contract — an egg phrase plays its effect, is swallowed, and returns `false` so the composer keeps the caret — only its home moved. The caret tests pin that. * `useBuddyConversation` no longer owns `draft`, `setDraft` or `handleSubmit`. What it still owes the composer is the news that the thread was replaced: a fresh visit and a project switch bump `draftResetToken`, and the provider clears the box in the same pass that notices it (the documented "adjust state when a prop changes" pattern rather than an effect, which would paint one frame of a draft belonging to a conversation that is gone, and pay a second render to correct it). * `useBuddy` merges the session with the draft again, so the widget and the tests that type-then-submit keep one API. * `dismissAction` reads the transcript through a ref instead of through `messages` state — same behaviour, but the callback stays the same function for the session's lifetime, which is what the memoisation in the next commit compares. The props the transcript will be compared on are also held in one identity across `BuddyPage`, `BuddyDock` and `BuddyWidget` (`renderReplyAction`, `renderQuestionAction`, `lastMessageFooter`, `onRetryOpen`, `onStartFreshVisit`, `onHideSuggestions`): a memo can only skip a re-render it is not asked for. Nothing else changes: the same words, the same submit contract, the same clearing on a fresh visit, the same one composer shared by the dock and the page.

The split took the keystroke off the conversation's re-render path; this closes the half a streamed token walks. Every token lands in the same `messages` array the thread renders from, so the whole thread re-rendered per token — every reply re-parsed, every bubble's animation hooks re-evaluated — and with the thread memoised but its props unstable, every row would still have re-rendered in turn. * `BuddyThread` is memoised, and each turn is now its own memoised `BuddyThreadRow` — the shape `MessageRow` in the chat already uses. A token changes one message object, so one row re-renders and the others bail out on referential equality, Markdown and all. * `BuddyMarkdown` is memoised on `content`. A reply's text never changes once written, so a render that did not change it skips the parse entirely. (The row is the first line of defence; this is the second.) * The row contract is written where it will be read: the render callbacks must be stable functions and `lastMessageFooter` must come from one `useMemo`, because either rebuilt per render hands every row a new prop and undoes the memo. `renderQuestionAction`/`renderReplyAction` now say so, and both callers do it. * The footer's gate — "is this the latest reply, and has the buddy stopped writing" — moved up into the thread, so a row never has to know about its neighbours. `BuddyComposerIsolation` (new) is the guard that this stays true. It mounts `/buddy` with a 32-message transcript under the real provider and counts how many times a reply is parsed (the Markdown renderer is stubbed to count): typing must cost zero parses, and each streamed token exactly one — its own reply, and no other. It is the assertion the rest of the suite cannot make, because every other buddy test looks at a single surface and the lag lived in the wiring between them.

The thread asked its rows "is this the newest message?" where it meant "is this the turn receiving tokens right now". The two only agree while a turn is running, so the newest reply was flagged as streaming for the rest of the conversation — and `BuddyMessage` hands that flag straight to `SleepyBot` as `canSleep={!isStreaming}`. A bot that may never sleep never does: the newest reply was the one bubble that stayed visibly working, forever. `Boolean(isStreaming && ...)` releases the flag the moment the turn ends. It is also a prop the memoised rows compare, so getting it exactly right is what keeps a finished turn from re-rendering the thread. The chat has drawn this line correctly from the start — `MessageRow` sets `canSleep={message.id !== streamingMessageId}` where `streamingMessageId` is null whenever nothing is streaming — and the buddy's row was forked from it without that null. This is the same line, in the buddy's shape. Two smaller things in the same corner, from the same reading of the file: - `lastAssistantId`'s note still described the escalation offer it used to carry. Escalation lives under the hire's own questions now; what the footer under a reply carries is the greeting's one suggested next step, and the note says so. - A reply that is only a proposal counts as a reply. The empty message the send loop appends is the one worth skipping, and skipping it should mean "nothing written yet, and nothing offered" — not "no prose, so look further back for something to hang the footer under". The new test renders the thread with the bubble stood in for and reads the flag off each row: nothing is flagged at rest, the last row while the turn runs, and nothing again once it lands. On the parent commit both assertions fail.

The page stopped re-rendering per keystroke in the first two commits. The widget had not: it read the draft through `useBuddy()`, and the widget is the one buddy surface mounted on every page — so a keystroke in the dock re-rendered the widget, and with it the dock, its header and every bubble in it. On `/buddy`, where the widget draws nothing, it still re-rendered on every character. Measured by the test added here, on the parent commit: eighteen keystrokes in the dock cost the floating surfaces 51 renders, and the same typing on `/buddy` cost 17. `useBuddy()` no longer reads the draft. It keeps the one thing it actually needed the setter for — the aiBuddyBus seeding effect ("Draft with AI"), which now takes the write-only half — and any surface that needs the value itself takes `useBuddyDraft()`. That was only possible because the hand-off stopped carrying the draft, which was the other half of the same leak. `goToPage` passed the words in history state and `useHandedOffDraft` applied them on arrival; but `BuddyDraftProvider` sits above the router, so the dock and the page have been reading one box since the first commit of this branch. The copy was a second mechanism for something the architecture already gave, and it was the reason `goToPage` depended on the draft. `useHandedOffDraft` is gone; the hand-off is `navigate("/buddy")`. On top of that, the dock and its header control are memoised. Every prop the dock takes is a plain value, a callback the widget holds in one identity, or a motion value — so a render of the widget that changes none of them (a resize while the dock is open, a drag across the screen) no longer walks the whole panel. `headerControl` is held in one identity for the same reason: built inline it would have been the one prop the memo always found changed. Tests: - The isolation file gains the floating half of the issue — a keystroke must cost zero renders of the widget/dock *and* zero markdown parses, with the render count proven wired before it is trusted, so a spy that never fires cannot pass vacuously. - `buddyDraftAcrossRoutes` (replacing the retired hook's tests) holds the replacement in place: the words cross because both surfaces read one box above the router, and clearing them on the page survives a back-and-forward that used to re-seed them out of the history entry. - The two tests that drive the session *through the composer* now use a test-only paired hook, `useBuddyWithDraft` — the pairing is what a test needs and no production surface does, so it lives in the harness.

`dismissAction` took a message id and an action id and searched the transcript for the action object, because only that object says whether the dismissal has a stored proposal to decline at the backend. That lookup is what forced the `messagesRef` next to it: reading `messages` state directly would have rebuilt the callback on every token, and every memoised row would then have re-rendered for a token that belongs to one of them. `confirmAction` has taken the action object all along, handed over by the card that drew it. `dismissAction` does now too: `BuddyActionProposals` passes what it rendered, the lookup goes away, and the ref and its effect go with it. The callback no longer depends on anything a token can change, and the question of whether a ref like that could be a render behind a click does not arise. The two assertions in the card's tests and the four calls in the team-mode tests follow the new shape.

`BuddyConversation` and `BuddyThread` both declared `before?: ReactNode` — "what came back from the hire's PM" — and nothing has passed it since the rail took that job: the page renders the rail's own panel, and the dock passes nothing at all. A prop no caller supplies is a promise the component cannot keep, and it was still being threaded through two prop types and one render. Removed from both, along with the pass-through between them.

kiranfin

Review

Clean, well-scoped refactor: the draft/session context split, the per-row thread memoisation, and the isStreaming fix (a reply no longer stays "awake" forever once its turn has finished) are all wired correctly, and the referential-stability contract the new memoisation relies on is honestly kept everywhere I checked — BuddyDraftProvider sits above the router so the hand-off to /buddy genuinely needs no history-state payload anymore, and dismissAction taking the action object instead of an id lookup removes a real staleness risk. Typecheck, lint, and the PR-relevant Vitest suites all pass in an isolated worktree; the unrelated localStorage failures in tests/unit/features/buddy/* are pre-existing on dev as well.

🟡 Worth a look

lastAssistantId now also matches an assistant reply that carries only action proposals and no text — src/features/buddy/components/BuddyThread.tsx:223-230. That's the intended fix for a proposal-only reply losing its footer, but it also means that if the buddy's opening/greeting message ever ships with an action proposal and no prose, lastMessageFooter (the "suggested next step" button from openerAction) and BuddyActionProposals' confirm/dismiss controls would render in the same footer at once. Probably not reachable with how greetings are generated today, but worth a quick confirmation that a text-less greeting-with-proposal can't happen, or a guard/test if it can.
BuddyConversation itself isn't wrapped in memo while BuddyThread and BuddyDock are — harmless today since BuddyThread's own memoisation still bails correctly, but worth a one-line note (or the wrap) so a future prop added to BuddyConversation doesn't quietly reintroduce a re-render path that the rest of this PR just closed.

🟢 Nits / cleanup

No test pins the new lastAssistantId behavior for an actions-only reply (point 1 above) — a small regression test would make the intent explicit and catch the footer-overlap case if it ever becomes reachable.
BuddyDraftProvider's reset-token effect and AppContent's peekMode reset in App.tsx both use the same "adjust state during render" pattern — consider naming it once in a shared comment/reference so it doesn't read as three independently-invented conventions.

Overall this is ready to merge from a correctness standpoint; the two 🟡 points are worth a quick look but neither blocks.

The opener footer is the greeting's "suggested next step", shown under the newest reply while nobody has spoken. A reply that arrived with a proposal of its own drew both — the card and the suggestion, stacked under one bubble — which reads as two competing offers rather than one. Not reachable today, and worth saying why: `streamOpenBuddy` has no `action_proposal` case (proposals only ever arrive on the message stream), and the greeting's placeholder is only given `content` or `error`, never `actions`. A text-less greeting-with-proposal cannot be built by the current client. Held anyway, because the alternative is that the day it becomes representable, the page shows two offers under one bubble and nothing fails. `BuddyThreadFooter` pins both rules that decide where the suggestion goes: it follows the *newest* reply (a proposal-only turn is still a turn — the reason `lastAssistantId` counts it), and it steps aside for a reply that brings a next step of its own, without falling back onto an older bubble. Stash the guard and the second test fails with exactly the overlap it describes.

…one identity The thread, every row and the dock were memoised in the earlier passes; the conversation wrapping them was not. So a page re-render the conversation has no interest in — the rail opening, a toast landing, the visit divider moving — still walked the whole column, and the memo inside absorbed the cost instead of the work never starting. It is `memo` now, with the props list written down as the contract it actually is: plain values, motion values, callbacks the page holds in one identity, and elements it builds once. That last part needed a real fix rather than a comment: `aboveComposer` was a JSX block built inline in the page's render, which would have made it the one prop the memo always found changed — silently, and only in the profiler. It is a `useMemo` above the return now, and the chips' reasoning moved with it, to where the element is built instead of sitting in an attribute list.

"Adjust state during render" appears in eight files, each comment explaining it a little differently — enough that a reviewer read the two this PR touches as independently invented conventions rather than one pattern used twice. It is one pattern, with three rules, and they are now named once in `CODING_STANDARDS.md` § 3 (workspace root, deliberately unversioned): only the component's own state, only behind an inequality check, and never a ref — a ref write during render belongs in an effect, which `ChatProvider` also notes. The two comments here keep their own local why — what is being corrected, and what an effect would paint in the meantime — and point at the standard for the how.

…dy-lag

PR #271 landed on `dev` while this branch was open, and the tests it added — the board synchronisation suite at the end of `useBuddy.test.tsx` — were written against `useBuddy()` still owning the draft: `result.current.setDraft(...)` and `handleSubmit(...)` taken off the same hook the always-mounted widget reads. That coupling is exactly what this branch removes, so the merge resolved the source files cleanly and left three render sites under the new blocks calling a hook that no longer carries the composer. CI runs the PR against the merge with `dev`, which is why the push run was green and the pull-request run was not. They go through the harness's `useBuddyWithDraft()` like every other sending test in the file — same providers (`Wrapper` is `QueryClientProvider` wrapping `BuddyProviderWithStubs`, draft provider included), same assertions, same turns. `tsc -b` and the type-aware eslint run recover, and the merged buddy + board suites are 63 files / 600 tests. Nothing else from #271 needed touching: the salvaged `useBuddyConversation` merged with the `dismissAction` change without a conflict, and `messagesRef` stays gone.

daniilperkin added 2 commits September 28, 2026 10:03

daniilperkin linked an issue Sep 28, 2026 that may be closed by this pull request

Prevent Buddy composer lag in long conversations #236

Closed

daniilperkin added 4 commits September 28, 2026 11:24

daniilperkin marked this pull request as ready for review September 28, 2026 09:44

daniilperkin requested review from BabuPlk, DavidLeuter and kiranfin and removed request for DavidLeuter September 28, 2026 09:44

daniilperkin changed the title ~~perf(buddy): fix composer lag in long conversations (#236)~~ fix composer lag in long conversations (#236) Sep 28, 2026

kiranfin self-assigned this Sep 28, 2026

kiranfin approved these changes Sep 28, 2026

View reviewed changes

daniilperkin added 5 commits September 28, 2026 13:18

Merge remote-tracking branch 'origin/dev' into hotfix/236-prevent-bud…

b9c2176

…dy-lag

daniilperkin merged commit fc2ced1 into dev Sep 28, 2026
4 checks passed

daniilperkin deleted the hotfix/236-prevent-buddy-lag branch September 28, 2026 11:41

Navigation Menu

fix composer lag in long conversations (#236) - #274

fix composer lag in long conversations (#236)#274
daniilperkin merged 11 commits into
devfrom
hotfix/236-prevent-buddy-lag

daniilperkin commented Sep 28, 2026 •

edited

Loading

Uh oh!

kiranfin left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

daniilperkin commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this fixes

How it is fixed — six commits, three passes

Verification (real output — npm run try on a05f518c)

Overlap with open PRs (measured, not estimated)

Deliberately out of scope

Third pass — the review of the fix

dev moved mid-flight

Uh oh!

kiranfin left a comment

Choose a reason for hiding this comment

Review

🟡 Worth a look

🟢 Nits / cleanup

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daniilperkin commented Sep 28, 2026 •

edited

Loading

Verification (real output — `npm run try` on `a05f518c`)

`dev` moved mid-flight