feat(apollo-vertex): ai-chat message actions — copy, feedback, edit, regenerate [3/5]#630
feat(apollo-vertex): ai-chat message actions — copy, feedback, edit, regenerate [3/5]#630petervachon wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
e315e1b to
cc1b25a
Compare
28f05e0 to
e6653c8
Compare
Dependency License Review
License distribution
Excluded packages
|
cc1b25a to
1cd03f4
Compare
e6653c8 to
7817feb
Compare
1cd03f4 to
c39dd48
Compare
7b409a5 to
2c7e90f
Compare
ccb80ab to
cf9efc2
Compare
d332c03 to
6a9cb61
Compare
cf9efc2 to
b3871a0
Compare
0xr3ngar
left a comment
There was a problem hiding this comment.
@petervachon since other PRs mentioned here were closed, can we close this PR as well?
b3871a0 to
fffc189
Compare
0xr3ngar
left a comment
There was a problem hiding this comment.
@pieman1313 hey Razvan I feel like this is going in the wrong direction, the component and data structure seem super off. Can you take a deeper look into this and let me know if I'm going crazy or something, because the component structure feels super off, and we have to patch things with effects or contexts that I do not think should be there.
| {showCopy && ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| void handleCopy(); | ||
| }} | ||
| className="size-7 inline-flex items-center justify-center rounded-md hover:bg-ai-chat-muted transition-colors" | ||
| aria-label={copyLabel} | ||
| > | ||
| {copied ? ( | ||
| <Check className="size-3.5 text-success" aria-hidden="true" /> | ||
| ) : ( | ||
| <Copy | ||
| className="size-3.5 text-ai-chat-muted-foreground" | ||
| aria-hidden="true" | ||
| /> | ||
| )} | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>{copyLabel}</TooltipContent> | ||
| </Tooltip> | ||
| )} |
There was a problem hiding this comment.
I would extract this copy state handling into a seperate component. This way the state const [copied, setCopied] = useState(false); and the handler handleCopy don't live in this component.
Extract into a generic component
import { useEffect, useState } from 'react';
import { useTranslation } from 'react-i18next';
type CopyToClipboardButtonProps = {
copyValue: string;
};
export const CopyToClipboardButton = ({
copyValue,
}: CopyToClipboardButtonProps) => {
const [hasCopied, setHasCopied] = useState(false);
const { t } = useTranslation();
const onClickHandler = async () => {
await navigator.clipboard
.writeText(copyValue)
.then(() => setHasCopied(true))
.catch(() => setHasCopied(false));
};
useEffect(() => {
let timeout: NodeJS.Timeout;
if (hasCopied) {
timeout = setTimeout(() => {
setHasCopied(false);
}, 1500);
}
return () => clearTimeout(timeout);
}, [hasCopied]);
return (
<Button
testId={TestId.copyToClipboard}
size="small"
variant="text"
color={hasCopied ? 'success' : 'primary'}
onClick={async () => {
await onClickHandler();
}}
startIcon={hasCopied ? <CheckIcon /> : <FileCopyIcon />}
>
{hasCopied ? t('copied') : t('copy')}
</Button>
);
};| {showCopy && ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button |
There was a problem hiding this comment.
also why are we using and not the component from the lib?
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={() => onFeedback("down")} | ||
| className="size-7 inline-flex items-center justify-center rounded-md hover:bg-ai-chat-muted transition-colors" | ||
| aria-label={t("bad_response")} | ||
| > | ||
| <ThumbsDown | ||
| className="size-3.5 text-ai-chat-muted-foreground" | ||
| aria-hidden="true" | ||
| /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>{t("bad_response")}</TooltipContent> | ||
| </Tooltip> | ||
| </> | ||
| )} | ||
|
|
||
| {messageRole === "assistant" && onRegenerate && ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={onRegenerate} | ||
| className="size-7 inline-flex items-center justify-center rounded-md hover:bg-ai-chat-muted transition-colors" | ||
| aria-label={t("try_again")} | ||
| > | ||
| <RefreshCw | ||
| className="size-3.5 text-ai-chat-muted-foreground" | ||
| aria-hidden="true" | ||
| /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>{t("try_again")}</TooltipContent> | ||
| </Tooltip> | ||
| )} | ||
|
|
||
| {messageRole === "user" && onEdit && ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={onEdit} | ||
| className="size-7 inline-flex items-center justify-center rounded-md hover:bg-ai-chat-muted transition-colors" | ||
| aria-label={t("edit")} | ||
| > | ||
| <Pencil | ||
| className="size-3.5 text-ai-chat-muted-foreground" | ||
| aria-hidden="true" | ||
| /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>{t("edit")}</TooltipContent> | ||
| </Tooltip> | ||
| )} |
There was a problem hiding this comment.
all these Buttons are literally the same with the only difference being the handler + icon + text. Extract into a shared Component and just pass the handler + text + icon
| @@ -0,0 +1,28 @@ | |||
| import type React from "react"; | |||
|
|
|||
| export type MessageFeedbackType = "up" | "down"; | |||
There was a problem hiding this comment.
I would call it positive | negative
| /** Show hover action toolbar on messages */ | ||
| showMessageActions: boolean; | ||
| /** Show copy button in message actions */ | ||
| showCopyButton: boolean; | ||
| /** Whether the chat is currently loading */ | ||
| isLoading: boolean; |
There was a problem hiding this comment.
why are these in the AiChatConfig type? these booleans make no sense to me. Why would a config have the isLoading boolean? Also the showCopyButton is sus? shouldn't these always be shown, same thing goes for showMessageActions
| > | ||
| {t("cancel")} | ||
| </button> | ||
| <button |
| transition={ENTRANCE_TRANSITION} | ||
| > | ||
| <div className="flex flex-col items-end gap-2 w-[80%]"> | ||
| <textarea |
There was a problem hiding this comment.
dont we also jhave a textarea component in the registry?
| // Streaming state — explicit prop wins, otherwise derive from chat-level isLoading | ||
| // and whether this is the latest assistant message currently being generated. | ||
| const isStreaming = | ||
| isStreamingProp ?? | ||
| (config.isLoading && | ||
| !isUser && | ||
| config.latestAssistantMessageId === message.id); |
There was a problem hiding this comment.
this is super sus, this feels like a code smell. Why is the isStreaming based on the loading and if there is no user and if the last msg id is the current message id
| // Auto-focus, select all, and scroll into view when entering edit mode. | ||
| // rAF defers the scroll until after React has committed the new layout. | ||
| useEffect(() => { | ||
| if (isEditing && editTextareaRef.current) { | ||
| editTextareaRef.current.focus(); | ||
| editTextareaRef.current.select(); | ||
| const el = editTextareaRef.current; | ||
| requestAnimationFrame(() => { | ||
| el.scrollIntoView({ behavior: "smooth", block: "center" }); | ||
| }); | ||
| } | ||
| }, [isEditing]); |
There was a problem hiding this comment.
what is this? cant we create an authofocus ref and attach it to the button or textarea?
const autoFocus = (container: HTMLDivElement | null) => {
if (!container) return;
const focusable = container.querySelector(FOCUSABLE_SELECTOR);
focusable?.focus();
};
export const useAutoFocusFirst = () => {
return autoFocus;
};
const ref = useAutofucusFirst();
| // Keep editValue in sync if message content changes externally (e.g. regenerate) | ||
| useEffect(() => { | ||
| if (!isEditing) setEditValue(displayContent); | ||
| }, [displayContent, isEditing]); |
There was a problem hiding this comment.
this is also super weird side effect that I think should never happen
|
@0xr3ngar valid callouts, @VKravchuk can you have a look? not sure about the provider altogether tbh |
ac1db1f to
8d64feb
Compare
0xr3ngar
left a comment
There was a problem hiding this comment.
OK I started reviewing the code again but I see smells everywhere :panic:
rn the data flow feels pretty shady. We pass messages into <AiChat />, but then the consumer also maps over messages and renders <AiChatMessage /> manually. On top of that, getAiChatMessageProps re-derives streaming/latest/action state, AiChatMessage re-derives display text/content visibility, and the template owns tool rendering as children
So the same source of truth is being interpreted in a bunch of different places
AiChathandles empty state, loading, header, copy conversation, scroll, etc..getAiChatMessagePropshandles latest message/streaming/action visibilityAiChatMessagehandles display text and whether the message is renderable- the template handles message mapping and tool rendering
that feels like a pretty big code smell, we’ve kind of split ownership in the worst middle-ground way- AiChat both owns and doesn’t own messages at the same time :/
It also creates a bunch of perf smells. During streaming, we’re constantly re-rendering/recomputing across multiple layers: the template maps every message again, tool parts are checked/rendered again, message props are re-derived, display text is rebuilt, and AiChat also walks messages for shell-level state like copy conversation/loading
I don’t think the current pattern/architecture is healthy at all. I think we need to go back to the drawing board and create a better architecture where message state is derived in one place instead of being re-interpreted and re-computed constantly across multiple components
882f789 to
67f3652
Compare
60ffe60 to
8ddf50c
Compare
8ddf50c to
9f766ba
Compare
What this does
Adds the per-message action layer — the controls that let users interact with individual messages rather than just reading them.
onFeedbackcallback so the host app can record quality signalsAiChatProvidercontext so all message components can read it without prop-drillingTest plan
onFeedbackwith the message ID and typeonRegenerate🤖 Generated with Claude Code