Skip to content

feat(feedback): add thumbs up/down buttons and persistence - #138

Open
Karn2898 wants to merge 2 commits into
extra-org:mainfrom
Karn2898:feat/message-feedback
Open

Karn2898 wants to merge 2 commits into
extra-org:mainfrom
Karn2898:feat/message-feedback

Conversation

@Karn2898

@Karn2898 Karn2898 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Adds 👍/👎 feedback buttons on assistant messages in the widget, persisted alongside the conversation in agent_manager.

What changed

  • src/agent_manager/domain/models.py — adds feedback: str | None = None to ConversationMessage.
  • src/agent_manager/domain/repository.py — adds update_message_feedback to the Repository port.
  • src/agent_manager/infrastructure/persistence/sql_repository.py — implements update_message_feedback by writing {"feedback": "thumbs_up"|"thumbs_down"} into the message's metadata_json column.
  • src/agent_manager/infrastructure/persistence/memory_repository.py — same for the in-memory adapter.
  • src/agent_manager/application/conversation_service.py — adds set_message_feedback(conversation_id, message_id, feedback, principal) which authorizes the caller and delegates to the repository.
  • src/agent_manager/api/schemas.py — adds MessageFeedbackRequest / MessageFeedbackResponse and adds feedback to MessageOut.
  • src/agent_manager/api/routes/conversations.py — adds POST /conversations/{conversation_id}/messages/{message_id}/feedback.
  • src/agent_manager/api/static/widget/types.ts — adds feedback?: "thumbs_up" | "thumbs_down" to MessageEntry and ChatMessage.
  • src/agent_manager/api/static/widget/api/AgentChatClient.ts — adds setMessageFeedback.
  • src/agent_manager/api/static/widget/react/useConversation.ts — exposes setMessageFeedback on the Conversation hook.
  • src/agent_manager/api/static/widget/react/AgentChatApp.tsx — adds 👍/👎 buttons to MessageActions, rendered only on assistant messages, wired to conversation.setMessageFeedback.

Tests

  • tests/agent_manager/test_service.py — 3 tests: persist+return, missing message returns None, authorization enforced.
  • tests/agent_manager/test_api.py — 3 tests: happy-path 200, 404 for missing message, 403 for wrong caller.

All 955 tests pass, lint clean, typecheck clean.

@Asaf-prog

Copy link
Copy Markdown
Collaborator

Thanks — the overall direction looks good, but I still see a few blockers before approving.

  1. Feedback update is not scoped to the authorized conversation

The service currently authorizes the caller against conversation_id:

await self._authorize(conversation_id, principal)

but the repository update is performed only by message_id:

update_message_feedback(message_id, feedback)

and in SQL:

session.get(ConversationMessageRow, message_id)

That creates a broken object-level authorization gap.

A caller who owns conversation A could potentially submit a message ID that belongs to conversation B, pass authorization on A, and still update the message from B.

Please scope the update to both:

conversation_id
message_id

at the repository boundary itself, rather than authorizing one object and mutating another by a global identifier.

  1. The assistant-only invariant is only enforced by the UI

The feature is described as feedback on assistant messages, but the backend currently allows feedback to be written to any message.

The repository/service contract should enforce that the target message is:

role == ASSISTANT

This should be a domain/application invariant, not something we rely on the widget to enforce.

  1. Fresh streamed assistant messages do not appear to have a persisted message ID

The feedback actions are rendered only when:

messageId && conversationId && onFeedback

History-loaded messages have message_id, but the newly streamed assistant entry is built from runtime stream events and does not appear to receive the persisted assistant message ID.

So the likely behavior is:

user gets a fresh answer
→ no feedback buttons

reload/open history
→ message_id is loaded
→ feedback buttons appear

The user should be able to rate the answer immediately after receiving it.

Please propagate the persisted assistant message ID through the final stream contract, or otherwise reconcile the newly streamed entry with the persisted message before rendering feedback actions.

  1. The widget does not update local feedback state after voting

setMessageFeedback() currently performs the API request, but I don't see the corresponding MessageEntry being updated locally.

That means:

click thumbs up
→ backend persists it
→ local `entry.feedback` stays unchanged
→ `aria-pressed` / active styling stays stale

until the conversation is reloaded.

Please update the local entry state after a successful response, or use an optimistic update with rollback on failure.

  1. Feedback should have a typed domain contract

Right now most layers use:

feedback: str

and the allowed values are only enforced at the API schema using a regex.

I would prefer a shared domain type / enum such as:

thumbs_up
thumbs_down

so invalid feedback values cannot flow through the application or repository layers.

This is less important than the authorization/UI issues, but it would make the contract cleaner.

  1. CI is currently not green

The backend quality gate passes, but the UI job fails on:

Rebuild widget and fail if the committed bundle is stale

Because of that, the widget unit tests and Playwright tests are skipped.

Please rebuild/commit the widget bundle and add regression coverage for the new feedback behavior.

At minimum I would expect UI coverage for:

  • feedback controls appear on a freshly completed assistant response;
  • thumbs up/down updates the selected state;
  • switching from up to down works correctly;
  • feedback does not render for user messages;
  • persisted feedback is restored from history.

Once the mutation is correctly scoped, the assistant-only invariant is enforced server-side, and the live widget state is wired correctly, I think this will be in a much stronger state.

@Asaf-prog

Copy link
Copy Markdown
Collaborator

I welcome this initiative @Karn2898

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants