Skip to content

Commit 04d3d02

Browse files
arkylinarkylin
authored
fix(web): Replace anti-pattern of assigning role="button" to entire message div (tiann#665)
* fix(web): replace message-level aria-expanded with explicit toggle button Replace the anti-pattern of assigning role=\"button\" and aria-expanded to the entire message content div. Instead, show an explicit \"Show info\" / \"Hide info\" button next to the timestamp. Also add a visible background container to MessageMetadata so users can actually see when metadata expands/collapses. Remove now-unused metadataToggle.ts and its tests. * fix(web): include turnCount in hasMetadata and pass it through codexReview branch
1 parent 9e35067 commit 04d3d02

5 files changed

Lines changed: 58 additions & 242 deletions

File tree

‎web/src/components/AssistantChat/messages/AssistantMessage.tsx‎

Lines changed: 40 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useCallback, useState, type KeyboardEvent, type MouseEvent } from 'react'
1+
import { useState } from 'react'
22
import { MessagePrimitive, useAssistantState } from '@assistant-ui/react'
33
import { MarkdownText } from '@/components/assistant-ui/markdown-text'
44
import { Reasoning, ReasoningGroup } from '@/components/assistant-ui/reasoning'
@@ -10,7 +10,6 @@ import type { HappyChatMessageMetadata } from '@/lib/assistant-runtime'
1010
import { getAssistantCopyText } from '@/components/AssistantChat/messages/assistantCopyText'
1111
import { getConversationMessageAnchorId } from '@/chat/outline'
1212
import { MessageMetadata } from '@/components/AssistantChat/messages/MessageMetadata'
13-
import { isNestedInteractiveEvent } from '@/components/AssistantChat/messages/metadataToggle'
1413
import { CodexReviewCard } from '@/components/AssistantChat/messages/CodexReviewCard'
1514
import { MessageTimestamp } from '@/components/AssistantChat/messages/MessageTimestamp'
1615

@@ -28,10 +27,6 @@ const MESSAGE_PART_COMPONENTS = {
2827
export function HappyAssistantMessage() {
2928
const { copied, copy } = useCopyToClipboard()
3029
const [showMetadata, setShowMetadata] = useState(false)
31-
const toggleMetadata = useCallback((event: MouseEvent<HTMLElement>) => {
32-
if (isNestedInteractiveEvent(event)) return
33-
setShowMetadata((open) => !open)
34-
}, [])
3530
const messageId = useAssistantState(({ message }) => message.id)
3631
const isCliOutput = useAssistantState(({ message }) => {
3732
const custom = message.metadata.custom as Partial<HappyChatMessageMetadata> | undefined
@@ -66,14 +61,7 @@ export function HappyAssistantMessage() {
6661
|| (typeof durationMs === 'number' && durationMs >= 0)
6762
|| usage != null
6863
|| (messageModel != null && messageModel !== '')
69-
70-
const onMetadataKeyDown = useCallback((event: KeyboardEvent<HTMLDivElement>) => {
71-
if (isNestedInteractiveEvent(event)) return
72-
if (event.key === 'Enter' || event.key === ' ') {
73-
event.preventDefault()
74-
setShowMetadata((open) => !open)
75-
}
76-
}, [])
64+
|| (typeof turnCount === 'number' && turnCount >= 2)
7765

7866
const rootClass = toolOnly
7967
? 'py-1 min-w-0 max-w-full overflow-x-hidden'
@@ -95,7 +83,7 @@ export function HappyAssistantMessage() {
9583
aria-expanded={showMetadata}
9684
className="text-[10px] text-[var(--app-hint)] underline-offset-2 hover:text-[var(--app-fg)] hover:underline"
9785
>
98-
{showMetadata ? 'Hide metadata' : 'Show metadata'}
86+
{showMetadata ? 'Hide info' : 'Show info'}
9987
</button>
10088
)}
10189
</div>
@@ -106,7 +94,6 @@ export function HappyAssistantMessage() {
10694
usage={usage}
10795
model={messageModel ?? null}
10896
turnCount={turnCount}
109-
className="mt-1"
11097
/>
11198
)}
11299
</MessagePrimitive.Root>
@@ -120,25 +107,28 @@ export function HappyAssistantMessage() {
120107
className={`${rootClass} ${copyText ? 'group/msg' : ''} scroll-mt-4`}
121108
>
122109
<div className="flex items-start gap-2">
123-
<div
124-
className={hasMetadata ? 'min-w-0 flex-1 cursor-pointer' : 'min-w-0 flex-1'}
125-
onClick={hasMetadata ? toggleMetadata : undefined}
126-
onKeyDown={hasMetadata ? onMetadataKeyDown : undefined}
127-
role={hasMetadata ? 'button' : undefined}
128-
tabIndex={hasMetadata ? 0 : undefined}
129-
aria-expanded={hasMetadata ? showMetadata : undefined}
130-
>
110+
<div className="min-w-0 flex-1">
131111
<CodexReviewCard review={codexReview} />
132-
<div className="mt-1 flex justify-start">
112+
<div className="mt-1 flex items-center gap-2">
133113
<MessageTimestamp className="text-[10px] leading-none text-[var(--app-hint)]" />
114+
{hasMetadata && (
115+
<button
116+
type="button"
117+
onClick={() => setShowMetadata((open) => !open)}
118+
aria-expanded={showMetadata}
119+
className="text-[10px] text-[var(--app-hint)] underline-offset-2 hover:text-[var(--app-fg)] hover:underline"
120+
>
121+
{showMetadata ? 'Hide info' : 'Show info'}
122+
</button>
123+
)}
134124
</div>
135125
{showMetadata && (
136126
<MessageMetadata
137127
invokedAt={invokedAt}
138128
durationMs={durationMs}
139129
usage={usage}
140130
model={messageModel ?? null}
141-
className="mt-1"
131+
turnCount={turnCount}
142132
/>
143133
)}
144134
</div>
@@ -167,17 +157,20 @@ export function HappyAssistantMessage() {
167157
id={getConversationMessageAnchorId(messageId)}
168158
className={`${rootClass} ${copyText ? 'group/msg' : ''} scroll-mt-4`}
169159
>
170-
<div
171-
className={hasMetadata ? 'min-w-0 cursor-pointer' : 'min-w-0'}
172-
onClick={hasMetadata ? toggleMetadata : undefined}
173-
onKeyDown={hasMetadata ? onMetadataKeyDown : undefined}
174-
role={hasMetadata ? 'button' : undefined}
175-
tabIndex={hasMetadata ? 0 : undefined}
176-
aria-expanded={hasMetadata ? showMetadata : undefined}
177-
>
160+
<div className="min-w-0">
178161
<MessagePrimitive.Content components={MESSAGE_PART_COMPONENTS} />
179-
<div className="mt-1 flex justify-start">
162+
<div className="mt-1 flex items-center gap-2">
180163
<MessageTimestamp className="text-[10px] leading-none text-[var(--app-hint)]" />
164+
{hasMetadata && (
165+
<button
166+
type="button"
167+
onClick={() => setShowMetadata((open) => !open)}
168+
aria-expanded={showMetadata}
169+
className="text-[10px] text-[var(--app-hint)] underline-offset-2 hover:text-[var(--app-fg)] hover:underline"
170+
>
171+
{showMetadata ? 'Hide info' : 'Show info'}
172+
</button>
173+
)}
181174
</div>
182175
{showMetadata && (
183176
<MessageMetadata
@@ -186,7 +179,6 @@ export function HappyAssistantMessage() {
186179
usage={usage}
187180
model={messageModel ?? null}
188181
turnCount={turnCount}
189-
className="mt-1"
190182
/>
191183
)}
192184
</div>
@@ -200,17 +192,20 @@ export function HappyAssistantMessage() {
200192
className={`${rootClass} ${copyText ? 'group/msg' : ''} scroll-mt-4`}
201193
>
202194
<div className="flex items-start gap-2">
203-
<div
204-
className={hasMetadata ? 'min-w-0 flex-1 cursor-pointer' : 'min-w-0 flex-1'}
205-
onClick={hasMetadata ? toggleMetadata : undefined}
206-
onKeyDown={hasMetadata ? onMetadataKeyDown : undefined}
207-
role={hasMetadata ? 'button' : undefined}
208-
tabIndex={hasMetadata ? 0 : undefined}
209-
aria-expanded={hasMetadata ? showMetadata : undefined}
210-
>
195+
<div className="min-w-0 flex-1">
211196
<MessagePrimitive.Content components={MESSAGE_PART_COMPONENTS} />
212-
<div className="mt-1 flex justify-start">
197+
<div className="mt-1 flex items-center gap-2">
213198
<MessageTimestamp className="text-[10px] leading-none text-[var(--app-hint)]" />
199+
{hasMetadata && (
200+
<button
201+
type="button"
202+
onClick={() => setShowMetadata((open) => !open)}
203+
aria-expanded={showMetadata}
204+
className="text-[10px] text-[var(--app-hint)] underline-offset-2 hover:text-[var(--app-fg)] hover:underline"
205+
>
206+
{showMetadata ? 'Hide info' : 'Show info'}
207+
</button>
208+
)}
214209
</div>
215210
{showMetadata && (
216211
<MessageMetadata
@@ -219,7 +214,6 @@ export function HappyAssistantMessage() {
219214
usage={usage}
220215
model={messageModel ?? null}
221216
turnCount={turnCount}
222-
className="mt-1"
223217
/>
224218
)}
225219
</div>

‎web/src/components/AssistantChat/messages/MessageMetadata.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ export function MessageMetadata({ invokedAt, durationMs, usage, model, turnCount
7373
if (parts.length === 0) return null
7474

7575
return (
76-
<div className={`text-[10px] text-[var(--app-hint)] flex flex-wrap gap-x-2 gap-y-0.5 mt-0.5 px-0.5 leading-tight opacity-60 ${className || ''}`}>
76+
<div className={`text-[10px] text-[var(--app-hint)] bg-[var(--app-subtle-bg)] rounded px-2 py-1.5 flex flex-wrap gap-x-2 gap-y-0.5 mt-1 leading-tight ${className || ''}`}>
7777
{parts.map((part, i) => (
7878
<span key={i} className="whitespace-nowrap">{part}</span>
7979
))}

‎web/src/components/AssistantChat/messages/UserMessage.tsx‎

Lines changed: 17 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useCallback, useState, type KeyboardEvent, type MouseEvent } from 'react'
1+
import { useState } from 'react'
22
import { MessagePrimitive, useAssistantState } from '@assistant-ui/react'
33
import { useHappyChatContext } from '@/components/AssistantChat/context'
44
import type { HappyChatMessageMetadata } from '@/lib/assistant-runtime'
@@ -10,17 +10,12 @@ import { CopyIcon, CheckIcon } from '@/components/icons'
1010
import { useCopyToClipboard } from '@/hooks/useCopyToClipboard'
1111
import { getConversationMessageAnchorId } from '@/chat/outline'
1212
import { MessageMetadata } from '@/components/AssistantChat/messages/MessageMetadata'
13-
import { isNestedInteractiveEvent } from '@/components/AssistantChat/messages/metadataToggle'
1413
import { MessageTimestamp } from '@/components/AssistantChat/messages/MessageTimestamp'
1514

1615
export function HappyUserMessage() {
1716
const ctx = useHappyChatContext()
1817
const { copied, copy } = useCopyToClipboard()
1918
const [showMetadata, setShowMetadata] = useState(false)
20-
const toggleMetadata = useCallback((event: MouseEvent<HTMLElement>) => {
21-
if (isNestedInteractiveEvent(event)) return
22-
setShowMetadata((open) => !open)
23-
}, [])
2419
const role = useAssistantState(({ message }) => message.role)
2520
const messageId = useAssistantState(({ message }) => message.id)
2621
const text = useAssistantState(({ message }) => {
@@ -55,14 +50,6 @@ export function HappyUserMessage() {
5550

5651
const hasMetadata = invokedAt != null
5752

58-
const onMetadataKeyDown = useCallback((event: KeyboardEvent<HTMLElement>) => {
59-
if (isNestedInteractiveEvent(event)) return
60-
if (event.key === 'Enter' || event.key === ' ') {
61-
event.preventDefault()
62-
setShowMetadata((open) => !open)
63-
}
64-
}, [])
65-
6653
if (role !== 'user') return null
6754
const canRetry = status === 'failed' && typeof localId === 'string' && Boolean(ctx.onRetryMessage)
6855
const onRetry = canRetry ? () => ctx.onRetryMessage!(localId) : undefined
@@ -85,12 +72,12 @@ export function HappyUserMessage() {
8572
aria-expanded={showMetadata}
8673
className="text-[10px] text-[var(--app-hint)] underline-offset-2 hover:text-[var(--app-fg)] hover:underline"
8774
>
88-
{showMetadata ? 'Hide metadata' : 'Show metadata'}
75+
{showMetadata ? 'Hide info' : 'Show info'}
8976
</button>
9077
)}
9178
</div>
9279
{showMetadata && invokedAt != null && (
93-
<MessageMetadata invokedAt={invokedAt} className="mt-1 justify-end" />
80+
<MessageMetadata invokedAt={invokedAt} />
9481
)}
9582
</div>
9683
</MessagePrimitive.Root>
@@ -103,12 +90,7 @@ export function HappyUserMessage() {
10390
return (
10491
<MessagePrimitive.Root
10592
id={getConversationMessageAnchorId(messageId)}
106-
className={`${getUserBubbleClassName(status)} group/msg scroll-mt-4 ${hasMetadata ? 'cursor-pointer' : ''}`}
107-
onClick={hasMetadata ? toggleMetadata : undefined}
108-
onKeyDown={hasMetadata ? onMetadataKeyDown : undefined}
109-
role={hasMetadata ? 'button' : undefined}
110-
tabIndex={hasMetadata ? 0 : undefined}
111-
aria-expanded={hasMetadata ? showMetadata : undefined}
93+
className={`${getUserBubbleClassName(status)} group/msg scroll-mt-4`}
11294
>
11395
<div className="flex flex-col gap-1">
11496
<div className="flex items-start gap-2">
@@ -123,10 +105,7 @@ export function HappyUserMessage() {
123105
type="button"
124106
title="Copy"
125107
className="rounded-md p-0.5 opacity-60 transition-[opacity,background-color] hover:bg-[var(--app-chat-user-chip-bg)] sm:opacity-0 sm:group-hover/msg:opacity-100"
126-
onClick={(event) => {
127-
event.stopPropagation()
128-
copy(text)
129-
}}
108+
onClick={() => copy(text)}
130109
>
131110
{copied
132111
? <CheckIcon className="h-3.5 w-3.5 text-green-500" />
@@ -137,11 +116,21 @@ export function HappyUserMessage() {
137116
</div>
138117
)}
139118
</div>
140-
<div className="flex justify-end">
119+
<div className="flex justify-end items-center gap-2">
141120
<MessageTimestamp className="text-[10px] leading-none text-[var(--app-hint)]" />
121+
{hasMetadata && (
122+
<button
123+
type="button"
124+
onClick={() => setShowMetadata((open) => !open)}
125+
aria-expanded={showMetadata}
126+
className="text-[10px] text-[var(--app-hint)] underline-offset-2 hover:text-[var(--app-fg)] hover:underline"
127+
>
128+
{showMetadata ? 'Hide info' : 'Show info'}
129+
</button>
130+
)}
142131
</div>
143132
{showMetadata && invokedAt != null && (
144-
<MessageMetadata invokedAt={invokedAt} className="justify-end opacity-60" />
133+
<MessageMetadata invokedAt={invokedAt} />
145134
)}
146135
</div>
147136
</MessagePrimitive.Root>

0 commit comments

Comments
 (0)