You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Check for required components (tests, UI, documentation)
Provide comprehensive review feedback
Review Summary
This PR adds a feedback system for capturing user feedback on agent conversations and messages. While the core implementation is solid, there are several critical missing components required by the repository guidelines.
⚠️ Critical Issues
1. Missing Database Migrations 🚨
The PR adds a new feedback table to the runtime schema but no migration files were generated. This will break the database when deployed.
Action required:
pnpm db:generate # Generate migration for the feedback table
pnpm db:migrate # Apply the migration locally to test
2. Missing Unit Tests 🚨
Per CLAUDE.md requirements, all new features MUST include comprehensive unit tests. This PR has zero tests for:
API route handlers (agents-api/src/domains/manage/routes/__tests__/feedback.test.ts)
Data access functions (packages/agents-core/src/data-access/runtime/__tests__/feedback.test.ts)
3. Missing UI Components 🚨
The PR only includes a single feedback-dialog.tsx component but lacks:
Feedback management UI for viewing/filtering feedback
Integration into the agent builder pages
Form validation schemas in the UI
4. Missing Documentation 🚨
No documentation was added to /agents-docs/ explaining:
How to use the feedback API
Integration guide for developers
API reference for the new endpoints
5. Missing Changeset 🚨
This change affects published packages (agents-api, agents-core) but lacks a changeset entry. Required for version management.
Action required:
pnpm bump minor --pkg agents-core --pkg agents-api "Add feedback system for capturing user feedback on conversations and messages"
✅ What's Good
Database Schema
Well-structured feedback table with proper foreign key constraints
Includes cascade delete for conversation cleanup
Good indexing on conversation_id and message_id
Flexible properties JSONB field for extensibility
API Implementation
Proper use of createProtectedRoute with correct permissions
CRUD operations follow existing patterns
Appropriate authorization checks (view for reads, edit for writes)
Good pagination support
Data Access Layer
Clean implementation with proper scoping
Efficient query patterns
Follows established patterns in the codebase
🔍 Additional Observations
Schema Design Considerations
The reasons field is typed as string[] | null - consider standardizing common feedback reasons
The userType distinguishes between 'user' and 'api_key' which is good for tracking feedback sources
No unique constraints on (conversationId, messageId, userId) - allows multiple feedback entries from same user
Security & Authorization
Routes correctly use project-scoped permissions
Tenant isolation properly enforced
Consider adding rate limiting for feedback creation to prevent spam
API Design
RESTful endpoints are well-structured
Consider adding bulk operations for analytics use cases
Missing endpoint to get feedback stats/analytics
📝 Recommendations
Immediate Actions Required (Blocking):
Generate and apply database migrations
Add comprehensive unit tests (minimum 80% coverage)
Create feedback management UI in agents-manage-ui
Add documentation to agents-docs
Create a changeset entry
Future Enhancements (Non-blocking):
Add analytics endpoints for feedback aggregation
Consider webhook events for feedback creation
Add feedback export functionality
Implement feedback sentiment analysis
Sample Test Structure
// agents-api/src/domains/manage/routes/__tests__/feedback.test.tsimport{describe,it,expect,beforeEach}from'vitest';describe('Feedback API Routes',()=>{describe('POST /feedback',()=>{it('should create feedback with valid data',async()=>{// Test implementation});it('should validate required fields',async()=>{// Test validation});it('should enforce project permissions',async()=>{// Test authorization});});// More test cases...});
Verdict: This PR cannot be merged in its current state due to missing mandatory components per CLAUDE.md requirements. Please address the critical issues above before proceeding.
The Feedback CRUD API introduced in PR #2419 was tested across create, read, update, delete operations, filtering, pagination, input validation, and security scenarios. All core CRUD operations, data validation (enum types, ID patterns, required fields), authentication enforcement, tenant isolation, SQL injection protection, and cascade delete behavior work correctly. One issue was found: the API returns 500 Internal Server Error instead of 400/415 when requests are sent with an incorrect Content-Type header.
✅ Passed (32)
Test Case
Summary
Timestamp
Screenshot
ROUTE-1
GET feedback list returned 200 with 2 items and correct pagination
6:08
ROUTE-2
GET feedback by ID returned 200 with correct data
5:59
ROUTE-3
POST feedback with all fields returned 201 with correct data
5:35
ROUTE-4
POST feedback with custom ID returned 201 with matching ID
5:49
ROUTE-5
PATCH feedback returned 200 with updated type/details and changed updatedAt
6:20
ROUTE-6
DELETE feedback returned 200 success:true, GET confirms 404
6:34
ROUTE-7
GET feedback filtered by messageId returned exactly 2 items with correct filter
View-only user can GET (200) but POST/PATCH/DELETE return 404 (permission denied)
24:52
❌ Failed (1)
Test Case
Summary
Timestamp
Screenshot
ADV-8
Wrong Content-Type returns 500 instead of expected 400/415
28:20
Wrong Content-Type header returns 500 instead of 400/415 – Failed
Where: POST /manage/tenants/{tenantId}/projects/{projectId}/feedback/
Steps to reproduce:
Send POST request to the feedback endpoint
Set Content-Type header to application/x-www-form-urlencoded or text/plain
Include valid feedback data in the body
What failed: The API returns 500 Internal Server Error instead of the expected 400 Bad Request or 415 Unsupported Media Type. When Content-Type is not application/json, the request body is not parsed, causing all fields to be null/undefined. This triggers a database FK constraint violation on the null conversationId rather than proper content-type validation.
Code analysis: Examined agents-api/src/domains/manage/routes/feedback.ts. The route definition at lines 131-136 specifies that it only accepts application/json content type, but there is no middleware to validate Content-Type before body parsing. When a non-JSON Content-Type is sent, the body parsing silently fails and fields become null, which then causes a FK violation at the database level.
app.openapi(createProtectedRoute({method: 'post',path: '/',summary: 'Create Feedback',description: 'Create a new feedback entry for a conversation or message',operationId: 'create-feedback',tags: ['Feedback'],permission: requireProjectPermission('edit'),request: {params: TenantProjectParamsSchema,body: {content: {'application/json': {schema: FeedbackApiInsertSchema,},},},},// ...}),async(c)=>{const{ tenantId, projectId }=c.req.valid('param');constbody=c.req.valid('json');// Returns undefined/null when Content-Type is wrongconstcreated=awaitcreateFeedback(runDbClient)({
...body,// Spreads null valuesid: body.id||generateId(),
tenantId,
projectId,});// FK violation occurs here due to null conversationId// ...});
Why this is likely a bug: The route explicitly declares it only accepts application/json content type, but when a different Content-Type is sent, instead of returning a 400 or 415 error, the body parsing fails silently and the request proceeds with null values. This causes a database-level FK constraint error (500) instead of proper HTTP content-type validation (400/415). The error message exposes internal database details rather than providing a user-friendly validation error.
Introduced by this PR: Yes – this PR added the feedback route (agents-api/src/domains/manage/routes/feedback.ts is a new file introduced in this PR).
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
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.