Repository navigation
feat(chat): add MessageParts to render a message's parts - #129
pratikbuilds wants to merge 5 commits into
Conversation
7437dff to
8eff9c5
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Automated multi-agent review: independent review + second-agent validation. Independent re-derivation confirms MessageParts is a composition layer, not a slot-fragmenter: renderMarkdown delegates to MessageContent's single render prop (src/ui/message-parts.tsx:333), the hard-cutover files are untouched, and the PR removes ~195 lines of hand-rolled part rendering from the stories — the opposite of a second rendering path. Nothing blocking; all items below are doc gaps or static-verified a11y edges.
- minor
src/ui/message-parts.tsx:331-333—renderTextsilently takes precedence overrenderMarkdownfor text parts; a host passing both gets a deadrenderMarkdownwith no warning. One line in therenderTextJSDoc ("Takes precedence overrenderMarkdown.") closes it. - minor a11y (static, HIGH)
message-parts.tsx:110, 132-139— the documented focus guarantee for customrenderApproval/renderBatchimplicitly requires that markup to contain a plain<button>(or mirrordata-slot="tool-approval"/data-status="pending"), becausefocusTargetfinds the next target by those selectors. State the DOM contract in the prop docs. - minor a11y (static trace)
message-parts.tsx:264-285+src/ui/tool-approval.tsx:116-122— when a waiting call stops waiting without a user decision (approval resolves totimeout/expired, which the contract explicitly allows), the card folds into its run and the focused buttons unmount with no focus recovery:MessageParts' effect only acts after a userdecide, andToolApproval's recovery effect can't fire on the unmounting commit. A regression test that focuses Approve and then resolves the record server-side would pin the expected behavior. - latent edge
message-parts.tsx:267-283— if the host removes the decided call frompartsentirely (instead of recording a decided status), focus recovery early-returns and focus stays on<body>. Fall back to the group the card sat under, or document that decided tools must remain inparts. - minor doc
README.md:345— the 0.3 upgrade row still tells upgraders to hand-mapgroupActivity(message.parts)ontoActivity/MessageContent/… — the exact mapping this PR automates. Point the row atMessageParts. - nit
message-parts.tsx:363— the adapter lambda names its first parameterimage, but perGeneratedImages' prop type that value is the image's index; rename toindex. (Same misreading exists infull-chat.stories.tsx:470— PR #127.)
8eff9c5 to
ddd6235
Compare
ddd6235 to
d364440
Compare
b20074d to
adc5a10
Compare
d364440 to
1f81635
Compare
|
Thanks for the review. Here's what happened to each point. Fixed1. Focus fell to the page when a waiting call timed out or expired (minor a11y)
2. Focus was lost if the host removed the decided call from
3. Doc gaps (minor)
4. The image adapter's argument was misnamed (nit)
Also in this PR
Commits: |
1f81635 to
5f79929
Compare
adc5a10 to
59a7b3f
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
Adds MessageParts, which renders each part of a message and moves focus when a waiting tool call lapses or is dropped.
Findings
-
src/ui/message-parts.tsx:181— Dropping a waiting call from the middle of a batch focuses the first remaining card, not the next one.focusTargetlooks the id up in the batch after it is gone, sofindIndexis-1.waiting.findIndex((index) => index > decided)is then0, andMath.max(0, …)keeps that. The next Enter approves the wrong call. The previous groups are already onleaving; use that call's old index and focus the next card that is still waiting after it. -
src/ui/message-parts.tsx:379— Removing an earlier part drops focus to the page while a later call is still waiting.groupActivitykeys parts by index (conversation-activity.ts:68), so the later card's key changes and it remounts (message-parts.tsx:520). The layout effect then returns because that call is stillisAwaitingApproval, and it never restores focus. Key the card bytoolCallId, or restore focus when the focused control has disconnected even though the call is still waiting.
5f79929 to
cccd2d7
Compare
31d704f to
831f99f
Compare
cccd2d7 to
492128c
Compare
831f99f to
c5f1b9e
Compare
492128c to
8c2f88c
Compare
Every host mapped ConversationPart to components by hand and re-implemented the focus handoff when a decided call folds into the run before it. MessageParts groups parts with groupActivity and renders each kind as its component (Activity, ToolApproval, ToolGroup, MessageContent, Attachments tiles, GeneratedImages, Sources, ConversationNotice), with a render prop per kind to replace any of them. After a decision folds a call away, focus moves to the run's trigger, or to the next waiting call in a batch, instead of falling to the page. The full chat and activity stories and the e2e fixtures use it in place of their own mappings; the e2e specs check that focus never sits on <body> for a frame after a keyboard decision.
MessageParts moved focus only after a decision made through its own decide. When the server resolved a call to timeout or expired while focus was on its buttons, the call folded into its run, the buttons unmounted, and focus fell to the page. The same happened when a host recorded a decision by removing the call from parts. Track which waiting call's card holds focus from the root's focus and blur events, and give a call that stops waiting under focus the same handoff as a decided one: the next waiting call in a batch, else the run's trigger. For a call that is gone from parts, fall back to the group it stood in, then the group before it, then the first control in the message. Document the markup custom renderBatch and renderActivity must keep for the handoff, and that renderText takes precedence over renderMarkdown. Point the 0.3 upgrade rows at MessageParts, and name the image adapter's argument index.
The consumer smoke checks one export per chat module so a dropped export * line fails CI. MessageParts arrived after that list was written; add it.
Dropping a waiting call from the middle of a batch moved focus to the first waiting card, not the next one. The handoff now skips every call up to and including the one that left, so focus lands on the call that waited after it. Removing a part before a waiting call shifted the index keys of every group after it, so React remounted the waiting card and focus fell to the page. A tool trace or batch is now keyed by its call id, and the handoff finds a group by that key. As in ToolGroup, the handoff focuses the next call's card rather than its Approve, so a held Enter cannot decide a call nobody chose; a host card without the card slot gets its first button. The batch e2e specs expect the card.
MessageParts watches two calls whose buttons may go away under focus: the one last decided and the waiting one whose card holds focus. It merged them into one, preferring the decided call, and cleared both once that one stopped waiting. In a batch, a decision moves focus to the next card, which starts the watch on that call, and recording the decision then ended it too: when the next call timed out or expired, focus fell to the page. A decision the host never records kept the merged watch stuck on that call, so a different call lapsing under focus later also dropped focus to the page. Each watch now ends only when its own call stops waiting. When both end in one update, the handoff starts from the focused call, since that is where focus was.
8c2f88c to
45c2783
Compare
c5f1b9e to
177ed17
Compare
Round 3 fixesA fresh review pass over the stack turned up the points below; each is fixed in a new commit at the end of this branch, and the stack is rebased onto it.
|
Stacked on #128: review only this commit. Part of #94.
Why
Every host maps
ConversationPartto components by hand, and each one re-implements the approval focus handoff. WhengroupActivityfolds a decided call into the run before it, the call's buttons unmount, and without that handoff focus drops to<body>. The oldPartsRendereronly knew the part type that #128 removes.What
MessageParts(src/ui/message-parts.tsx, in the root barrel) groups a message's parts withgroupActivityand renders each kind as its component:ActivityToolApprovalToolGroupMessageContent, usingrenderMarkdownas its render propAttachmentstilesGeneratedImages, withonOpenImageSourcesConversationNoticerenderText,renderActivity,renderStep,renderApproval,renderBatch,renderBlock,renderFiles,renderEvent,renderImages,renderSources. An override can returnundefinedto keep the default ornullto render nothing. A block'sdatabelongs to the host, so blocks render nothing withoutrenderBlock.ActivityTrigger. In a batch that still has waiting calls, it moves to the next waiting call's buttons, wrapping to the first. The handoff waits until the host has recorded the decision inparts, and it never takes focus from an element the person has moved to. A customrenderApprovalorrenderBatchgets the same behaviour through thedecidecallback it receives.streamingdefaults to the enclosingMessage's status.stories/chat/full-chat.stories.tsx(its focus code is removed),stories/chat/activity.stories.tsx, ande2e/fixtures.tsx(the tool-approval and thread scenarios, plus a newtool-batch-approvalscenario).Tests
src/ui/message-parts.test.tsx(16 tests): every part kind, overrides, streaming, image open, and focus after a decision:renderApprovalThe focus tests fail when the handoff is disabled.
Playwright:
e2e/focus.tschecksdocument.activeElementon every frame. The tool-approval, batch-approval and full-chat specs assert that focus is never on<body>for even one frame after a keyboard decision.bun run check,build,dep-guardand the fulltest:e2epass: 126 passed, including the Storybook axe sweep. The known list is unchanged.Closes #112