feat(assistant): make the thumbs actually report something (#1236)
* feat(assistant): make the thumbs actually report something The thumbs up/down under an assistant answer shipped wired to nothing. They lit up, the vote died in component state, and the code said so in a comment nobody reading the UI could see. An affordance that looks like it reports something and does not is worse than no affordance: it spends the user's goodwill once, and silently. They now post to a new /api/agent/feedback, which emits the SAME agent.feedback event the gnubok_feedback MCP tool emits, with actorType 'user'. The product team already queries event_log for that type, so chat votes land in the backlog they read rather than in a second place someone has to remember to look at. event_log takes the payload as jsonb and already treats agent.* as telemetry, so there is no migration. The conversation id is caller-supplied, so it gets the same ownership check /api/agent/invoke got: without it a member could file feedback against a colleague's thread and the backlog would carry conversations the reporter never saw. Mutation-checked, three tests fail when the guard is removed. The pressed state is set only after the server accepts the vote, so the button never claims a report that never arrived, and a vote does not toggle off: it is append-only telemetry, and offering an undo we cannot honour would be a control that lies. Changing your mind sends the other sentiment instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(assistant): drop the unused free-text field from the feedback route compliance-swarm flagged the `comment` field as an undocumented PII exposure: free text embedded verbatim into the agent.feedback payload and written to event_log under telemetry retention, with no data-classification decision, next to a PostHog policy that treats the same class of data differently. The finding is right, and the field was worse than it looked: no caller ever sent one. The UI posts sentiment and a turn index. So this is dead API surface whose only effect was to accept whatever a user might type, in an accounting product, into a 180-day log: client names, personnummer, case details. Removed rather than documented. A comment box is a reasonable thing to want, but it needs its own classification and redaction decision made with the UI in front of it, not inherited from an unused parameter. The test now asserts the property instead of the field's absence: a caller that posts a comment anyway must not get it stored anywhere in the payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Opus 5
Claude Opus 5
parent
2da96c0be2
commit
1dce9227a4
@@ -1,6 +1,6 @@
|
||||
'use client'
|
||||
|
||||
import { useEffect, useLayoutEffect, useRef, useState } from 'react'
|
||||
import { useEffect, useLayoutEffect, useRef, useState, useCallback } from 'react'
|
||||
import Link from 'next/link'
|
||||
import {
|
||||
Send,
|
||||
@@ -25,6 +25,7 @@ import ApprovalCard from './ApprovalCard'
|
||||
import { getErrorMessage as getUserErrorMessage } from '@/lib/errors/get-error-message'
|
||||
import type { StoredStagedOperation } from '@/types'
|
||||
import type { AgentStatusEvent } from './agent-status'
|
||||
import { sendFeedback, type FeedbackSentiment } from './feedback-client'
|
||||
|
||||
// Markdown parser loads separately from the chat surface: react-markdown +
|
||||
// remark-gfm pull in the whole unified/remark tree.
|
||||
@@ -738,6 +739,18 @@ export default function AgentChat({
|
||||
}
|
||||
}
|
||||
|
||||
// A vote needs the thread it belongs to. Without a conversation id there is
|
||||
// nothing to attach the report to, so the buttons stay inert rather than
|
||||
// posting a vote the backlog cannot trace to an answer.
|
||||
const handleVote = useCallback(
|
||||
async (sentiment: FeedbackSentiment) => {
|
||||
const id = conversationIdRef.current
|
||||
if (!id) return false
|
||||
return sendFeedback({ conversationId: id, sentiment })
|
||||
},
|
||||
[],
|
||||
)
|
||||
|
||||
return (
|
||||
<div className="relative flex flex-col h-full min-h-0">
|
||||
{/* The chat had no live region at all, so a screen-reader user got no
|
||||
@@ -774,6 +787,7 @@ export default function AgentChat({
|
||||
}
|
||||
onRegenerate={handleRegenerate}
|
||||
onCorrection={handleCorrection}
|
||||
onVote={handleVote}
|
||||
/>
|
||||
</div>
|
||||
))}
|
||||
@@ -869,12 +883,14 @@ function MessageBubble({
|
||||
showRegenerate,
|
||||
onRegenerate,
|
||||
onCorrection,
|
||||
onVote,
|
||||
}: {
|
||||
message: ChatMessage
|
||||
streamingTail: boolean
|
||||
showRegenerate?: boolean
|
||||
onRegenerate?: () => void
|
||||
onCorrection?: (message: string) => void
|
||||
onVote?: (sentiment: FeedbackSentiment) => Promise<boolean>
|
||||
}) {
|
||||
const isUser = message.role === 'user'
|
||||
// An assistant turn that contains only tool calls (no text, no streaming
|
||||
@@ -992,6 +1008,7 @@ function MessageBubble({
|
||||
<MessageActions
|
||||
text={message.text}
|
||||
onRegenerate={showRegenerate ? onRegenerate : undefined}
|
||||
onVote={onVote}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
@@ -1001,20 +1018,36 @@ function MessageBubble({
|
||||
/**
|
||||
* Hover row under a finished assistant answer: copy, feedback, regenerate.
|
||||
*
|
||||
* Feedback is deliberately fire-and-forget and local-only for now: the point of
|
||||
* this row is that the affordances exist where users look for them. Wiring the
|
||||
* thumbs to gnubok_feedback is a follow-up, and a failed vote must never
|
||||
* interrupt reading an answer.
|
||||
* The thumbs used to be local-only: they lit up and the vote died in component
|
||||
* state. They now report to /api/agent/feedback, and the pressed state is set
|
||||
* only once the server has accepted the vote, so the button never claims a
|
||||
* report that did not happen.
|
||||
*
|
||||
* A vote does not toggle off. It emits an append-only telemetry event, and
|
||||
* there is no un-emitting one, so offering an undo would be a control that
|
||||
* lies. Changing your mind sends the other sentiment, which is a thing the
|
||||
* backlog can actually see.
|
||||
*/
|
||||
function MessageActions({
|
||||
text,
|
||||
onRegenerate,
|
||||
onVote,
|
||||
}: {
|
||||
text: string
|
||||
onRegenerate?: () => void
|
||||
onVote?: (sentiment: FeedbackSentiment) => Promise<boolean>
|
||||
}) {
|
||||
const [copied, setCopied] = useState(false)
|
||||
const [vote, setVote] = useState<'up' | 'down' | null>(null)
|
||||
const [voting, setVoting] = useState(false)
|
||||
|
||||
async function handleVote(next: 'up' | 'down') {
|
||||
if (voting || vote === next || !onVote) return
|
||||
setVoting(true)
|
||||
const ok = await onVote(next === 'up' ? 'positive' : 'negative')
|
||||
setVoting(false)
|
||||
if (ok) setVote(next)
|
||||
}
|
||||
|
||||
useEffect(() => {
|
||||
if (!copied) return
|
||||
@@ -1043,8 +1076,9 @@ function MessageActions({
|
||||
</button>
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => setVote(vote === 'up' ? null : 'up')}
|
||||
className={cn(btn, vote === 'up' && 'text-foreground')}
|
||||
onClick={() => handleVote('up')}
|
||||
disabled={voting || !onVote}
|
||||
className={cn(btn, vote === 'up' && 'text-foreground', voting && 'opacity-60')}
|
||||
title="Bra svar"
|
||||
aria-pressed={vote === 'up'}
|
||||
>
|
||||
@@ -1053,8 +1087,9 @@ function MessageActions({
|
||||
</button>
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => setVote(vote === 'down' ? null : 'down')}
|
||||
className={cn(btn, vote === 'down' && 'text-foreground')}
|
||||
onClick={() => handleVote('down')}
|
||||
disabled={voting || !onVote}
|
||||
className={cn(btn, vote === 'down' && 'text-foreground', voting && 'opacity-60')}
|
||||
title="Dåligt svar"
|
||||
aria-pressed={vote === 'down'}
|
||||
>
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
import { describe, it, expect, vi } from 'vitest'
|
||||
import { sendFeedback } from '../feedback-client'
|
||||
|
||||
/**
|
||||
* The thumbs shipped wired to nothing: they lit up and the vote died in
|
||||
* component state. These pin the contract the button now depends on, in
|
||||
* particular that a failed send reports false, because the pressed state is
|
||||
* set from this return value and must never claim a report that never arrived.
|
||||
*/
|
||||
|
||||
describe('sendFeedback', () => {
|
||||
it('posts the vote to the feedback endpoint', async () => {
|
||||
const fetchImpl = vi.fn().mockResolvedValue({ ok: true })
|
||||
|
||||
const ok = await sendFeedback({
|
||||
conversationId: 'c1',
|
||||
sentiment: 'negative',
|
||||
messageIndex: 3,
|
||||
fetchImpl: fetchImpl as unknown as typeof fetch,
|
||||
})
|
||||
|
||||
expect(ok).toBe(true)
|
||||
const [url, init] = fetchImpl.mock.calls[0]!
|
||||
expect(url).toBe('/api/agent/feedback')
|
||||
expect(init.method).toBe('POST')
|
||||
expect(JSON.parse(init.body)).toEqual({
|
||||
conversation_id: 'c1',
|
||||
sentiment: 'negative',
|
||||
message_index: 3,
|
||||
})
|
||||
})
|
||||
|
||||
it('omits the message index rather than sending null for it', async () => {
|
||||
// The route validates message_index as an integer when present; sending
|
||||
// null would fail validation and lose the vote for no reason.
|
||||
const fetchImpl = vi.fn().mockResolvedValue({ ok: true })
|
||||
await sendFeedback({
|
||||
conversationId: 'c1',
|
||||
sentiment: 'positive',
|
||||
fetchImpl: fetchImpl as unknown as typeof fetch,
|
||||
})
|
||||
expect(JSON.parse(fetchImpl.mock.calls[0]![1].body)).toEqual({
|
||||
conversation_id: 'c1',
|
||||
sentiment: 'positive',
|
||||
})
|
||||
})
|
||||
|
||||
it('reports failure for a non-2xx instead of assuming it landed', async () => {
|
||||
const fetchImpl = vi.fn().mockResolvedValue({ ok: false, status: 404 })
|
||||
expect(
|
||||
await sendFeedback({
|
||||
conversationId: 'c1',
|
||||
sentiment: 'positive',
|
||||
fetchImpl: fetchImpl as unknown as typeof fetch,
|
||||
}),
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('reports failure instead of throwing when the request dies', async () => {
|
||||
// Offline, or the request cut off by a navigation. An unhandled rejection
|
||||
// here would surface as an error in the middle of reading an answer.
|
||||
const fetchImpl = vi.fn().mockRejectedValue(new TypeError('Failed to fetch'))
|
||||
await expect(
|
||||
sendFeedback({
|
||||
conversationId: 'c1',
|
||||
sentiment: 'negative',
|
||||
fetchImpl: fetchImpl as unknown as typeof fetch,
|
||||
}),
|
||||
).resolves.toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,35 @@
|
||||
/**
|
||||
* Send one thumbs vote on an assistant answer.
|
||||
*
|
||||
* Kept out of the component so it is testable without a React harness (this
|
||||
* repo's unit project is node-only), and so the failure contract is stated in
|
||||
* one place: this resolves false rather than throwing, because a vote that
|
||||
* cannot be sent must never interrupt reading the answer it was about.
|
||||
*/
|
||||
export type FeedbackSentiment = 'positive' | 'negative'
|
||||
|
||||
export async function sendFeedback(args: {
|
||||
conversationId: string
|
||||
sentiment: FeedbackSentiment
|
||||
messageIndex?: number
|
||||
fetchImpl?: typeof fetch
|
||||
}): Promise<boolean> {
|
||||
const { conversationId, sentiment, messageIndex, fetchImpl = fetch } = args
|
||||
try {
|
||||
const res = await fetchImpl('/api/agent/feedback', {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({
|
||||
conversation_id: conversationId,
|
||||
sentiment,
|
||||
...(messageIndex === undefined ? {} : { message_index: messageIndex }),
|
||||
}),
|
||||
})
|
||||
return res.ok
|
||||
} catch {
|
||||
// Offline, or the request was cut off by a navigation. Reported as a
|
||||
// failed vote so the button can go back to unpressed rather than claiming
|
||||
// a report that never arrived.
|
||||
return false
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user