-
Notifications
You must be signed in to change notification settings - Fork 94
feat: thumbs up/down on ai assistant messages #18803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ErlingHauan
wants to merge
26
commits into
main
Choose a base branch
from
feat-ai-chat-feedback-buttons
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
b11f16c
first iteration
ErlingHauan 2c3bf8f
move feedback endpoint out of agent route
ErlingHauan 88c85b6
remove unused X-API-KEY logic from AltinityAgentClient
ErlingHauan 5b8bc3f
fix dialog text and styling
ErlingHauan ac24cc0
refactor MessageFeedback and tests
ErlingHauan f2fab70
split bundled chat-types imports into per-file imports
ErlingHauan 24cc893
use centralized mock texts in MessageFeedback.test.tsx
ErlingHauan 930e405
extract UserFeedback type; simplify onMessageFeedback handlers
ErlingHauan 43467a2
remove unnecessary error logging
ErlingHauan 9cfa604
extract decorateMessagesWithTraceIds to utils file
ErlingHauan ad008e6
validate developer owns trace before writing feedback to it
ErlingHauan f31a524
reset feedback states when closing dialog
ErlingHauan f97a73c
fix typing to pass strict null checks
ErlingHauan b02142b
use state for dialog
ErlingHauan 1621480
use StudioFormGroup in MessageFeedback
ErlingHauan 8077f75
feedback endpoint returns 403 without message
ErlingHauan fbe5e42
simplify designer backend code
ErlingHauan 4e467b9
add cancel button to dialog
ErlingHauan 4b409b8
make useChatFeedbackMutation take org, app as params
ErlingHauan 7dc71aa
add tests to Messages
ErlingHauan a9d6817
make feedback endpoint idempotent
ErlingHauan cdc4560
use PUT/upsert so repeat requests overwrites previous feedback on tha…
ErlingHauan b96728d
update texts based on feedback
ErlingHauan 91c6202
add cancellation token support
ErlingHauan db81a9f
small fixes
ErlingHauan dece4a1
add required prop messages to test setup
ErlingHauan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| """API routes module""" | ||
| from .websocket import register_websocket_routes | ||
| from .agent import router as agent_router | ||
| from .feedback import router as feedback_router | ||
|
|
||
| __all__ = ['register_websocket_routes', 'agent_router'] | ||
| __all__ = ['register_websocket_routes', 'agent_router', 'feedback_router'] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| """User feedback API routes""" | ||
|
|
||
| from typing import Optional | ||
|
|
||
| from fastapi import APIRouter, HTTPException, Request, Response | ||
| from pydantic import BaseModel, field_validator | ||
|
|
||
| from shared.utils.langfuse_utils import get_trace_developer, score_validation | ||
| from shared.utils.logging_utils import get_logger | ||
|
|
||
| router = APIRouter() | ||
| log = get_logger(__name__) | ||
|
|
||
| FEEDBACK_SCORE_NAME = "user_feedback" | ||
| FEEDBACK_COMMENT_MAX_LENGTH = 10000 | ||
| DEVELOPER_HEADER = "X-Developer" | ||
|
|
||
|
|
||
| class FeedbackReq(BaseModel): | ||
| """User feedback (thumbs up/down) on an assistant message, recorded as a Langfuse score.""" | ||
|
|
||
| trace_id: str | ||
| thumbs_up: bool | ||
| comment: Optional[str] = None | ||
|
|
||
| @field_validator("trace_id") | ||
| @classmethod | ||
| def _validate_trace_id(cls, v: str) -> str: | ||
| if not v.strip(): | ||
| raise ValueError("trace_id must be non-empty") | ||
| return v | ||
|
|
||
| @field_validator("comment") | ||
| @classmethod | ||
| def _validate_comment(cls, v: Optional[str]) -> Optional[str]: | ||
| if v is None: | ||
| return v | ||
| if len(v) > FEEDBACK_COMMENT_MAX_LENGTH: | ||
| raise ValueError( | ||
| f"comment must not exceed {FEEDBACK_COMMENT_MAX_LENGTH} characters" | ||
| ) | ||
| return v | ||
|
|
||
|
|
||
| @router.post("/api/feedback", status_code=204) | ||
| async def submit_feedback(req: FeedbackReq, request: Request): | ||
| """Records user feedback as a Langfuse score on the given trace.""" | ||
| caller = request.headers.get(DEVELOPER_HEADER) | ||
| if not caller: | ||
| raise HTTPException( | ||
| status_code=400, detail=f"Missing {DEVELOPER_HEADER} header" | ||
| ) | ||
|
|
||
| trace_owner = get_trace_developer(req.trace_id) | ||
| if trace_owner != caller: | ||
| raise HTTPException(status_code=403) | ||
|
|
||
| score_validation( | ||
| name=FEEDBACK_SCORE_NAME, | ||
| passed=req.thumbs_up, | ||
| trace_id=req.trace_id, | ||
| comment=req.comment, | ||
| ) | ||
| return Response(status_code=204) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| from unittest.mock import patch | ||
|
|
||
| from fastapi.testclient import TestClient | ||
|
|
||
| from api.main import app | ||
|
|
||
| FEEDBACK_PATH = "/api/feedback" | ||
| VALID_TRACE_ID = "trace-abc-123" | ||
| DEVELOPER = "ola" | ||
| OTHER_DEVELOPER = "kari" | ||
| DEVELOPER_HEADER = {"X-Developer": DEVELOPER} | ||
|
|
||
|
|
||
| def _post_feedback(payload, headers=None): | ||
| return TestClient(app).post(FEEDBACK_PATH, json=payload, headers=headers) | ||
|
|
||
|
|
||
| class TestFeedbackEndpoint: | ||
| def test_thumbs_up_writes_score_and_returns_204(self): | ||
| with ( | ||
| patch("api.routes.feedback.get_trace_developer", return_value=DEVELOPER), | ||
| patch("api.routes.feedback.score_validation") as mock_score, | ||
| ): | ||
| response = _post_feedback( | ||
| {"trace_id": VALID_TRACE_ID, "thumbs_up": True}, | ||
| headers=DEVELOPER_HEADER, | ||
| ) | ||
|
|
||
| assert response.status_code == 204 | ||
| mock_score.assert_called_once_with( | ||
| name="user_feedback", | ||
| passed=True, | ||
| trace_id=VALID_TRACE_ID, | ||
| comment=None, | ||
| ) | ||
|
|
||
| def test_thumbs_down_with_comment_is_forwarded(self): | ||
| with ( | ||
| patch("api.routes.feedback.get_trace_developer", return_value=DEVELOPER), | ||
| patch("api.routes.feedback.score_validation") as mock_score, | ||
| ): | ||
| response = _post_feedback( | ||
| { | ||
| "trace_id": VALID_TRACE_ID, | ||
| "thumbs_up": False, | ||
| "comment": "Svaret var ikke nyttig.", | ||
| }, | ||
| headers=DEVELOPER_HEADER, | ||
| ) | ||
|
|
||
| assert response.status_code == 204 | ||
| mock_score.assert_called_once_with( | ||
| name="user_feedback", | ||
| passed=False, | ||
| trace_id=VALID_TRACE_ID, | ||
| comment="Svaret var ikke nyttig.", | ||
| ) | ||
|
|
||
| def test_missing_developer_header_returns_400(self): | ||
| with patch("api.routes.feedback.score_validation") as mock_score: | ||
| response = _post_feedback({"trace_id": VALID_TRACE_ID, "thumbs_up": True}) | ||
|
|
||
| assert response.status_code == 400 | ||
| mock_score.assert_not_called() | ||
|
|
||
| def test_unknown_trace_owner_returns_403(self): | ||
| with ( | ||
| patch("api.routes.feedback.get_trace_developer", return_value=None), | ||
| patch("api.routes.feedback.score_validation") as mock_score, | ||
| ): | ||
| response = _post_feedback( | ||
| {"trace_id": VALID_TRACE_ID, "thumbs_up": True}, | ||
| headers=DEVELOPER_HEADER, | ||
| ) | ||
|
|
||
| assert response.status_code == 403 | ||
| mock_score.assert_not_called() | ||
|
|
||
| def test_owner_mismatch_returns_403(self): | ||
| with ( | ||
| patch( | ||
| "api.routes.feedback.get_trace_developer", return_value=OTHER_DEVELOPER | ||
| ), | ||
| patch("api.routes.feedback.score_validation") as mock_score, | ||
| ): | ||
| response = _post_feedback( | ||
| {"trace_id": VALID_TRACE_ID, "thumbs_up": True}, | ||
| headers=DEVELOPER_HEADER, | ||
| ) | ||
|
|
||
| assert response.status_code == 403 | ||
| mock_score.assert_not_called() | ||
|
|
||
| def test_empty_trace_id_returns_422(self): | ||
| with patch("api.routes.feedback.score_validation") as mock_score: | ||
| response = _post_feedback( | ||
| {"trace_id": "", "thumbs_up": True}, | ||
| headers=DEVELOPER_HEADER, | ||
| ) | ||
|
|
||
| assert response.status_code == 422 | ||
| mock_score.assert_not_called() | ||
|
|
||
| def test_comment_over_max_length_returns_422(self): | ||
| with patch("api.routes.feedback.score_validation") as mock_score: | ||
| response = _post_feedback( | ||
| { | ||
| "trace_id": VALID_TRACE_ID, | ||
| "thumbs_up": True, | ||
| "comment": "x" * 10001, | ||
| }, | ||
| headers=DEVELOPER_HEADER, | ||
| ) | ||
|
|
||
| assert response.status_code == 422 | ||
| mock_score.assert_not_called() |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.