feedback

[shagun-singh-inkeep]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 31bf91e

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

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
agents-eval-api	Ready	Preview, Comment	Jan 15, 2026 9:36pm
agents-manage-api	Ready	Preview, Comment	Jan 15, 2026 9:36pm
agents-manage-ui	Ready	Preview, Comment	Jan 15, 2026 9:36pm
agents-run-api	Ready	Preview, Comment	Jan 15, 2026 9:36pm

1 Skipped Deployment

Project	Deployment	Review	Updated (UTC)
agents-docs	Skipped		Jan 15, 2026 9:36pm

[claude]

Claude finished @shagun-singh-inkeep's task —— View job

---

TypeScript Staff Engineer / System Architect Review

I've conducted a comprehensive review of this feedback functionality PR. The implementation adds user feedback collection across conversations with solid telemetry integration. Here are my architectural and engineering considerations:

🔧 System Design & Architecture

Strengths

• Clean API Design: The /v1/feedback endpoint follows RESTful conventions with proper validation using Zod schemas
• Telemetry Integration: Excellent use of OpenTelemetry for tracking feedback events - this provides valuable analytics capabilities
• Consistent Data Flow: Feedback flows properly from UI → API → telemetry → analytics backend

Architectural Concerns

1. Error Boundary & Data Consistency ⚠️

Issue: agents-run-api/src/routes/feedback.ts:61-62 - The span is ended immediately and success is returned even if the flushBatchProcessor might fail asynchronously.

// Current implementation
span.end();
await new Promise((resolve) => setImmediate(resolve));
await flushBatchProcessor(); // This could fail after span is ended

Recommendation: Move span completion after successful flush:

await flushBatchProcessor();
span.end(); // Only end after successful flush

2. Missing Data Validation 🔒

Issue: No server-side validation that the conversationId actually exists or belongs to the authenticated tenant/project.

Recommendation: Add validation in feedback route:

// Verify conversation exists and user has access
const conversationExists = await verifyConversationAccess(conversationId, tenantId, projectId);
if (!conversationExists) {
  return c.json({ error: 'Conversation not found or access denied' }, 404);
}

3. Race Condition Potential ⚡

Issue: agents-manage-ui/src/components/agent/playground/chat-widget.tsx:87-136 - The feedback submission doesn't handle concurrent submissions for the same conversation/message.

Current Flow:

const feedbackKey = `${conversationId}-${msgId || 'general'}`;
const existingFeedback = submittedFeedback.get(feedbackKey);
// Race condition: Multiple rapid clicks could submit multiple requests

Recommendation: Add request deduplication:

const [submittingFeedback, setSubmittingFeedback] = useState<Set<string>>(new Set());

const submitFeedback = useCallback(async (feedbackType, msgId) => {
  const feedbackKey = `${conversationId}-${msgId || 'general'}`;
  
  if (submittingFeedback.has(feedbackKey)) {
    return; // Already submitting
  }
  
  setSubmittingFeedback(prev => new Set(prev).add(feedbackKey));
  try {
    // ... existing logic
  } finally {
    setSubmittingFeedback(prev => {
      const next = new Set(prev);
      next.delete(feedbackKey);
      return next;
    });
  }
}, []);

🛡️ Security & Data Access

4. Authentication Context Validation ⚠️

Issue: agents-run-api/src/routes/feedback.ts:36-38 - Fallback to 'unknown' values for missing execution context is concerning for analytics integrity.

const tenantId = executionContext?.tenantId || 'unknown';

Recommendation: Return 401/403 for missing authentication context:

if (!executionContext?.tenantId) {
  return c.json({ error: 'Authentication required' }, 401);
}

5. Input Sanitization ✅

Good: Proper Zod validation with enum constraints for feedback types prevents injection attacks.

🚀 Performance & Scalability

6. Efficient Data Querying ✅

Good: The conversation picker implements proper pagination and search debouncing (conversation-picker.tsx:58-65).

7. Memory Management Concern ⚠️

Issue: chat-widget.tsx:74-76 - submittedFeedback Map could grow indefinitely with conversation lifetime.

Recommendation: Clear old feedback state when conversation ID changes:

useEffect(() => {
  setSubmittedFeedback(new Map()); // Clear on conversation change
}, [conversationId]);

🎯 Customer Experience & API Design

8. Excellent UX Patterns ✅

• Immediate visual feedback with toast notifications
• Clear feedback state management (positive/negative icons)
• Proper loading states and error handling

9. API Consistency ✅

• Follows existing header patterns (x-inkeep-tenant-id, etc.)
• Consistent error response format with other endpoints

🔄 Future Extensibility

10. Feedback Type Extensibility ✅

Good: Using enum for feedback types allows easy expansion:

feedback: z.enum(['positive', 'negative']), // Easy to extend: ['positive', 'negative', 'neutral', 'custom']

11. Missing Bulk Operations 💡

Consideration: For evaluation workflows, bulk feedback operations might be valuable:

// Future API extension
POST /v1/feedback/bulk
{
  "feedback": [
    { "conversationId": "123", "feedback": "positive" },
    { "conversationId": "456", "feedback": "negative" }
  ]
}

🧪 Testing & Observability

12. Telemetry Attributes ✅

Excellent: Comprehensive span attributes provide rich analytics (feedback.type, conversation.id, etc.).

13. Error Tracking ⚠️

Issue: chat-widget.tsx:123 - Generic error logging doesn't provide actionable debugging information.

Recommendation: Add structured error context:

catch (error) {
  console.error('Error submitting feedback:', {
    error: error instanceof Error ? error.message : String(error),
    conversationId,
    feedbackType,
    messageId: msgId,
    timestamp: new Date().toISOString()
  });
}

⚡ Critical Actions Required

1. Fix span completion timing in feedback route (data consistency issue)
2. Add conversation ownership validation (security issue)
3. Implement request deduplication (race condition prevention)
4. Strengthen authentication validation (security hardening)

💡 Recommended Enhancements

1. Add feedback reason/comment field for richer data collection
2. Implement feedback analytics dashboard integration
3. Consider feedback webhook notifications for real-time monitoring
4. Add feedback rate limiting to prevent spam

🎯 Overall Assessment

This is a solid implementation with good architectural foundations. The telemetry integration is excellent and the UX patterns are thoughtful. The critical issues are primarily around data consistency and security validation - addressable with the recommended changes above.

The feedback system provides valuable user insights and integrates well with the existing evaluation workflow. With the suggested improvements, this will be a robust, scalable feature.

---

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 7 days if no further activity occurs.

If this PR is still relevant:

• Rebase it on the latest main branch
• Add a comment explaining its current status
• Request a review if it's ready

Thank you for your contributions!