Smurching/add thumbs up down feedback#87
Open
smurching wants to merge 26 commits intodatabricks:mainfrom
Open
Conversation
This commit adds user feedback capabilities to the chatbot interface, allowing users to rate assistant responses with thumbs up/down reactions. Feedback is stored in the database and optionally sent to MLflow for experiment tracking. Changes include: - Database schema: New Feedback table with foreign keys to Message, Chat, and User - Database queries: CRUD operations for feedback (create, read, update, delete) - Server route: /api/feedback endpoint with MLflow integration as side effect - UI updates: Thumbs up/down buttons in message actions for assistant messages - MLflow client: Async submission to MLflow assessments API - Migration: SQL migration to add Feedback table - Tests: Unit tests for feedback route endpoints - Documentation: Environment variables for MLFLOW_EXPERIMENT_ID in .env.example and app.yaml The MLflow integration is optional and configured via MLFLOW_EXPERIMENT_ID environment variable. If not set, feedback is saved to database only. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Multiple fixes to get thumbs up/down feedback working: 1. Fix Message table reference - Updated schema.ts to use 'Message_v2' instead of 'Message' - Database has both tables; Message_v2 has parts/attachments columns 2. Fix foreign key constraints - Updated Feedback.messageId FK to point to Message_v2 - Script: scripts/fix-feedback-fk.ts 3. Fix feedback POST route middleware - Removed requireChatAccess (expects :id param that doesn't exist) - Added manual checkChatAccess call in handler 4. Add automatic user creation - Created ensureUserExists() function - Called before creating feedback to satisfy FK constraint - Fixes issue where users weren't auto-created on first use 5. Load existing feedback on page load - Added GET /api/feedback/chat/:chatId endpoint - Extended useChatData hook to fetch feedback - Passes feedback state through component tree to MessageActions Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1. Fix React.memo bug in Messages component
- Add feedback prop comparison
- Fix return value (true when props equal, not false)
- This prevents constant re-renders that caused UI to disappear
2. Fix missing feedback field in useChatData hook
- Add feedback: {} to 404 error path
- Ensures consistent data structure
3. Add AGENTS.md with comprehensive UI development guide
- React component patterns and memoization
- API endpoint testing with curl examples
- Common debugging patterns and solutions
- Performance optimization techniques
Testing:
- Created test script verifying memo logic
- Confirmed all memo comparison cases work correctly
- Verified API data flow with curl
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Creates automated test script that validates the complete flow: - Session verification - Chat creation with streaming messages - Chat metadata retrieval - Message history fetching - Feedback submission (thumbs up/down) - Feedback updates - Chat history listing Features: - Colored output for better readability - Automatic test user generation - Error handling and validation - Browser URL generation for manual verification - Follows patterns from AGENTS.md Usage: ./scripts/test-e2e-flow.sh [user-id] [email] Test results: ✅ All 7 tests passed successfully - Created chat with streaming response - Submitted and updated feedback - Verified data persistence This script can be used to validate changes before committing and ensures the feedback feature works end-to-end. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Documents comprehensive testing results for feedback feature: - All 7 test scenarios passed successfully - Bug fixes applied (React.memo and data structure) - API endpoints validated - Database state verified - Test artifacts and examples included Provides clear record of: - What was tested and how - Issues found and resolved - Remaining tasks - How to run tests Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Problem: - Page went blank when clicking thumbs up/down - React error: "Rendered more hooks than during the previous render" - Caused by early return (if isLoading) BEFORE all hooks were called Root Cause: The component had this structure: 1. Call 4 hooks 2. Early return if isLoading (line 52) 3. Call useCallback hook (line 70) When isLoading=true: 4 hooks called When isLoading=false: 5 hooks called This violated React's Rules of Hooks. Fix: - Moved ALL hooks to the top (before any early returns) - Converted handleCopy to useCallback for consistency - Added useMemo for textFromParts to avoid recalculation - Added initialFeedback comparison to memo function - Added comment explaining the requirement Hook call order is now consistent across all renders: 1. useCopyToClipboard 2. useState (feedback) 3. useState (isSubmittingFeedback) 4. useMemo (textFromParts) 5. useEffect 6. useCallback (handleFeedback) 7. useCallback (handleCopy) 8. Early return (after all hooks) Testing: - Frontend hot-reload should apply changes automatically - No more hooks violation errors - Feedback buttons should work without blanking page Related: https://reactjs.org/link/rules-of-hooks Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Users get visual feedback from the button color change (green/red), so the toast notification is redundant. Keeping error toast for failed submissions since that's important feedback when something goes wrong. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Problem: When reloading the page on an existing chat, saw this error: TypeError: Cannot read properties of undefined (reading 'state') at Chat.makeRequest (@ai-sdk_react.js:2252:35) at async AbstractChat.resumeStream Root Cause: On page reload: 1. lastPartRef.current is undefined (no stream parts received yet) 2. onFinish callback is triggered (from AI SDK's automatic resume) 3. Logic sees lastPartRef === undefined and tries to resumeStream() 4. But there's no active stream to resume - it's just a page load 5. AI SDK's internal state is undefined, causing the error Fix: Check if streamCursor === 0 before attempting resume. If we haven't received any stream parts in this session, there's nothing to resume. This only affects actual stream interruptions, not page reloads. Flow now: - Page reload: streamCursor=0 → skip resume → fetch history - Stream disconnect: streamCursor>0 → attempt resume - Stream complete: lastPart.type='finish' → skip resume → fetch history Testing: 1. Reload page on existing chat → no error 2. Submit feedback → feedback works, no error 3. Network disconnect during stream → resume works Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Problem: - Network tab shows streaming happening token-by-token - But UI doesn't show anything until entire message completes - Then shows everything at once with feedback widget Root Cause: Same memo bug as before in message.tsx line 395: - Memo function always returned false - This disabled memoization and caused unnecessary re-renders - Should return true when props are equal (skip re-render) Also missing: - initialFeedback comparison in memo function Fix: 1. Changed final return from false to true 2. Added initialFeedback.feedbackType comparison 3. Now properly skips re-renders when props haven't changed 4. Allows re-renders when message.parts changes (streaming) Expected behavior after fix: ✅ Tokens stream and display incrementally ✅ No feedback widget while streaming (isLoading=true hides it) ✅ Feedback widget appears when stream completes (isLoading=false) This matches the behavior in Messages component we fixed earlier. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added logging at three levels to track streaming: 1. ChatTransport.onStreamPart - logs when stream parts arrive 2. Chat component - logs when messages array updates 3. PreviewMessage - logs when message component renders This will help identify where the bottleneck is: - If only transport logs appear: AI SDK not updating messages - If transport + chat logs appear: PreviewMessage not re-rendering - If all logs appear frequently: Display/CSS issue User reported only seeing one PreviewMessage log with 0 chars even though streaming takes time, suggesting messages array is not being updated during streaming. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Error: Cannot access 'messages' before initialization Cause: useEffect referenced messages/status before useChat declared them Fix: Moved useEffect to after useChat hook declaration. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This should make token-by-token streaming more responsive in the UI. Lower throttle = more frequent UI updates = smoother streaming display. Debug logs added in previous commits will help identify if: - Stream parts are arriving ([ChatTransport] logs) - Messages array is updating ([Chat] logs) - Component is re-rendering ([PreviewMessage] logs) These logs appear in browser console, not server logs. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Modified PreviewMessage memo to always re-render during streaming - Removed debug logging from chat.tsx and message.tsx - Updated TESTING_SUMMARY.md with streaming fix documentation The issue was that the memo function blocked re-renders when isLoading=true, preventing incremental token display. Now tokens stream smoothly and feedback widget appears only after message completion.
Root cause: Messages component memo was blocking re-renders during streaming. The AI SDK mutates the messages array in place, so deep equality checks don't detect changes. This prevented PreviewMessage from receiving updated props during streaming. Changes: - Modified Messages memo to always re-render when status is 'streaming' - Added E2E testing documentation to AGENTS.md - Created STREAMING_DEBUG_INVESTIGATION.md documenting the investigation This ensures tokens appear incrementally during message generation while maintaining performance optimization for static messages.
Resolved conflicts: - client/src/components/chat.tsx: Kept both OAuth error check and streamCursor check - client/tsconfig.json: Removed obsolete /tools path mapping - client/src/components/messages.tsx: Removed duplicate sendMessage prop Integrated upstream changes including OAuth error handling and package restructuring.
Removed TESTING_SUMMARY.md and STREAMING_DEBUG_INVESTIGATION.md to keep PR focused on feature implementation. Core documentation in AGENTS.md with E2E testing instructions is preserved.
…bricks_options.return_trace: true for serving endpoints in AI SDK provider Signed-off-by: Sid Murching <sid.murching@databricks.com>
- Add feedback table to DB schema with feedbackType, mlflowAssessmentId
- Capture trace IDs from Databricks Responses API via databricksFetch layer
(injects databricks_options.return_trace=true) and onChunk callback
- Store trace IDs in DB (traceId column on message) and in-memory fallback
(message-meta-store.ts) for ephemeral mode
- POST feedback as MLflow assessment (api/3.0/mlflow/traces/{id}/assessments)
with boolean feedback.value; skip gracefully if no trace ID
- Export getWorkspaceHostname() from ai-sdk-providers for use in feedback route
- Add MSW mock for MLflow assessments endpoint with body validation
- Add route tests: text streaming, trace ID capture, and feedback submission
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…source_id
- Extend message-meta-store.ts to store assessmentId per (messageId, userId)
for ephemeral-mode deduplication
- Hoist existingFeedback lookup before MLflow call so DB-mode assessment ID
is available for deduplication check
- PATCH /api/3.0/mlflow/traces/{id}/assessments/{assessmentId} when an
assessment already exists; POST only for first submission
- Use session.user.email (falls back to id) as MLflow source_id
- Add MSW PATCH mock for MLflow assessments endpoint
- Add test: second feedback submission uses PATCH instead of POST
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bricks/ai-sdk-provider Instead of injecting databricks_options.return_trace in the custom databricksFetch interceptor, forward it via providerOptions so the upstream provider handles it natively. Capture the resulting trace_id via includeRawChunks/onChunk from the raw SSE events, removing the global traceIdMap state and fallback retrieval logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…m git tracking This directory was an embedded copy of the chatbot app template that should not be tracked separately. The canonical copy lives in e2e-chatbot-app-next/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The MLflow assessments PATCH API requires an update_mask field specifying which fields to update. Without it the request fails with INVALID_PARAMETER_VALUE. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When API_PROXY is set, sends x-mlflow-return-trace-id: true as a
request header. The MLflow AgentServer appends a standalone SSE event
{ trace_id: "tr-..." } at the end of the stream, which onChunk now
captures alongside the existing Databricks serving endpoint path.
Also adds:
- routes-api-proxy Playwright project + second webServer on port 3003
for testing the full API_PROXY trace-ID → feedback path
- Configurable Vite port/backend via PORT and BACKEND_URL env vars,
enabling multiple local server pairs to run simultaneously
- 2 integration tests (trace-id-capture.api-proxy.test.ts) covering
happy-path capture and PATCH deduplication
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Schema The AI SDK's generateId produces 16-char alphanumeric IDs (nanoid), not UUIDs. Assistant response messages receive nanoid IDs server-side, so sending them back as previousMessages failed z.string().uuid() validation. Relax the check to z.string() for previous message IDs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s/ai-sdk-provider The upstream package was rebuilt (OpenResponses streaming support) and dropped the extractApprovalStatus export. Remove the import and the MCP denial short-circuit that depended on it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.