Add feedback module to databunny.#111
Conversation
|
@sbansal1999 is attempting to deploy a commit to the Databuddy Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds voting and feedback to assistant messages. Expands useChat to expose handleVote and handleFeedbackComment using a new TRPC mutation. MessageBubble and ChatSection now wire these handlers, show upvote/downvote, copy for last message, and an inline feedback input after downvote. RPC input makes vote type optional. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CS as ChatSection
participant MB as MessageBubble
participant HC as useChat (hook)
participant RPC as TRPC addFeedback
participant S as Server/DB
U->>MB: Click 👍 / 👎
MB->>HC: handleVote(messageId, type)
HC->>RPC: mutate({ messageId, type })
RPC->>S: upsert vote
S-->>RPC: result
RPC-->>HC: success/error
HC-->>MB: resolve (UI may update)
MB-->>U: Vote state reflected
Note over U,MB: If 👎, feedback textarea appears
U->>MB: Submit feedback comment
MB->>HC: handleFeedbackComment(messageId, comment)
HC->>RPC: mutate({ messageId, comment })
RPC->>S: store comment (no vote change)
S-->>RPC: result
RPC-->>HC: success (toast) / error
HC-->>MB: resolve
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
packages/rpc/src/routers/assistant.ts (1)
189-191: Explicit else-if for 'downvote' is fine; consider idempotency and per-user vote constraintsThe explicit
else if (input.type === 'downvote')is clearer than a genericelse. However, the endpoint currently allows unlimited voting by the same user on the same message. Consider aassistantMessageVotestable keyed by(messageId, userId)to enforce one vote per user and support toggling.I can sketch the table and router changes if helpful.
apps/dashboard/app/(main)/websites/[id]/assistant/hooks/use-chat.ts (3)
47-53: Centralize feedback UX by using mutateAsync; keep error handling consistentYou already surface errors here. To make callers (MessageBubble) able to react (e.g., only disable UI on success), prefer
mutateAsyncand return a boolean from the handlers below.No diff here; see concrete diffs in the handler comments next.
319-325: Make vote submission awaitable and resilient; return success booleanCurrently, UI disables the vote immediately and cannot recover on failure. Return a promise so the component can update state only on success.
Apply this diff:
- const handleVote = useCallback( - (messageId: string, type: Vote) => { - addFeedback.mutate({ messageId, type }); - }, - [addFeedback] - ); + const handleVote = useCallback( + async (messageId: string, type: Vote): Promise<boolean> => { + try { + await addFeedback.mutateAsync({ messageId, type }); + return true; + } catch { + toast.error('Failed to submit vote'); + return false; + } + }, + [addFeedback] + );
326-341: Keep feedback input open on failure; return awaitable resultRight now the input closes immediately in the component. Returning a promise lets the component close only on success and preserve text on failures.
Apply this diff:
- const handleFeedbackComment = useCallback( - (messageId: string, comment: string) => { - addFeedback.mutate( - { messageId, comment }, - { - onSuccess: () => { - toast.success('Feedback submitted'); - }, - onError: () => { - toast.error('Failed to submit feedback'); - }, - } - ); - }, - [addFeedback] - ); + const handleFeedbackComment = useCallback( + async (messageId: string, comment: string): Promise<boolean> => { + try { + await addFeedback.mutateAsync({ messageId, comment }); + toast.success('Feedback submitted'); + return true; + } catch { + toast.error('Failed to submit feedback'); + return false; + } + }, + [addFeedback] + );apps/dashboard/app/(main)/websites/[id]/assistant/components/chat-section.tsx (1)
195-203: Passing isLastMessage via index works; consider robustness if system messages are appendedIndex-based last-message detection is fine today. If system/service messages get appended later, consider a helper that finds the last assistant/user message by type to keep the UX consistent.
No change required now.
apps/dashboard/app/(main)/websites/[id]/assistant/components/message-bubble.tsx (5)
28-36: Reduce type coupling with useChat; declare minimal function types for propsReferencing
ReturnType<typeof useChat>creates unnecessary coupling. Prefer explicit function signatures for better stability and compile perf.Apply this diff:
-import type { useChat, Vote } from '../hooks/use-chat'; +import type { Vote } from '../hooks/use-chat'; @@ -interface MessageBubbleProps { +type HandleVote = (messageId: string, type: Vote) => Promise<boolean> | void; +type HandleFeedbackComment = ( + messageId: string, + comment: string +) => Promise<boolean> | void; + +interface MessageBubbleProps { message: Message; - handleVote: ReturnType<typeof useChat>['handleVote']; - handleFeedbackComment: ReturnType<typeof useChat>['handleFeedbackComment']; + handleVote: HandleVote; + handleFeedbackComment: HandleFeedbackComment; isLastMessage: boolean; }This pairs with the optional
mutateAsyncrefactor suggested in the hook.
104-161: Don’t close the feedback box until the comment is savedCurrently
onSubmitimmediately callsonCancel(), so failures discard the user’s text. Await the submission result and close only on success.Apply this diff:
-function FeedbackInput({ - onSubmit, - onCancel, -}: { - onSubmit: (feedbackText: string) => void; - onCancel: () => void; -}) { +function FeedbackInput({ + onSubmit, + onCancel, +}: { + onSubmit: (feedbackText: string) => Promise<boolean> | boolean | void; + onCancel: () => void; +}) { const [feedbackText, setFeedbackText] = useState(''); - const handleSubmit = () => { + const handleSubmit = async () => { if (!feedbackText.trim()) { return; } - onSubmit(feedbackText.trim()); - onCancel(); + const ok = await Promise.resolve(onSubmit(feedbackText.trim())); + if (ok !== false) { + onCancel(); + setFeedbackText(''); + } };Optionally disable the submit button while awaiting to prevent double submits.
171-199: Set vote state only after a successful mutation; handle failure gracefullyIf the network fails, the UI remains in a “voted” state with no way to retry. Await the handler and only set
voteTypeupon success.Apply this diff (pairs with the
mutateAsyncchange in the hook):-const handleFeedbackButtonClick = (type: Vote) => { - if (type === 'downvote') { - setShowFeedbackInput(true); - } - - setVoteType(type); - handleVote(message.id, type); -}; +const handleFeedbackButtonClick = async (type: Vote) => { + if (type === 'downvote') { + setShowFeedbackInput(true); + } + const ok = await Promise.resolve(handleVote(message.id, type)); + if (ok !== false) { + setVoteType(type); + } +};
186-190: Close feedback input only on successSame rationale as above—avoid discarding the user’s text on failure.
Apply this diff:
-const handleSubmitFeedback = (feedbackText: string) => { - handleFeedbackComment(message.id, feedbackText); - setShowFeedbackInput(false); -}; +const handleSubmitFeedback = async (feedbackText: string) => { + const ok = await Promise.resolve( + handleFeedbackComment(message.id, feedbackText) + ); + if (ok !== false) { + setShowFeedbackInput(false); + } +};
248-257: Optional UX: show a toast on “Copy message” and handle clipboard errors
navigator.clipboard.writeTextis async and can fail in some contexts. Consider try/catch with a toast to confirm or report errors.Example:
try { await navigator.clipboard.writeText(message.content); // toast.success('Copied to clipboard'); } catch { // toast.error('Copy failed'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/dashboard/app/(main)/websites/[id]/assistant/components/chat-section.tsx(2 hunks)apps/dashboard/app/(main)/websites/[id]/assistant/components/message-bubble.tsx(3 hunks)apps/dashboard/app/(main)/websites/[id]/assistant/hooks/use-chat.ts(5 hunks)packages/rpc/src/routers/assistant.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:37:28.419Z
Learnt from: CR
PR: databuddy-analytics/Databuddy#0
File: .cursor/rules/01-MUST-DO.mdc:0-0
Timestamp: 2025-08-11T10:37:28.419Z
Learning: Applies to **/*.{tsx,jsx,ts,js} : When using Phosphor React icons, append 'Icon' to component names (e.g., CaretIcon, not Caret)
Applied to files:
apps/dashboard/app/(main)/websites/[id]/assistant/components/message-bubble.tsx
🔇 Additional comments (5)
apps/dashboard/app/(main)/websites/[id]/assistant/hooks/use-chat.ts (3)
4-6: LGTM on wiring trpc + toastImports look correct and localized. No action needed.
30-31: Vote type export is clear and narrowGood use of a discriminated union for votes.
353-355: Good: expose handlers in the hook’s public APIThis keeps
ChatSectionandMessageBubblelean.apps/dashboard/app/(main)/websites/[id]/assistant/components/chat-section.tsx (1)
95-97: Wiring new handlers is correctDestructuring
handleVoteandhandleFeedbackCommentfromuseChat()is straightforward and keeps props localized.apps/dashboard/app/(main)/websites/[id]/assistant/components/message-bubble.tsx (1)
1-14: Icon naming conforms to our conventionThanks for following the “append ‘Icon’” rule for Phosphor React icons. This matches our repo guideline.
| const updates: Partial<typeof assistantMessages.$inferInsert> = {}; | ||
| if (input.type === 'upvote') { | ||
| updates.upvotes = message.upvotes + 1; | ||
| } else { | ||
| } else if (input.type === 'downvote') { | ||
| updates.downvotes = message.downvotes + 1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Defend against null counters and empty updates; add minimal guards
- If
upvotes/downvotescan be null in existing rows,message.upvotes + 1yieldsNaN. Use?? 0. - If neither
typenorcommentis provided (client bug or tampering), short-circuit with BAD_REQUEST before executing the update.
Apply this diff:
- if (input.type === 'upvote') {
- updates.upvotes = message.upvotes + 1;
- } else if (input.type === 'downvote') {
- updates.downvotes = message.downvotes + 1;
- }
+ if (input.type === 'upvote') {
+ updates.upvotes = (message.upvotes ?? 0) + 1;
+ } else if (input.type === 'downvote') {
+ updates.downvotes = (message.downvotes ?? 0) + 1;
+ } // Add comment if provided
if (input.comment) {
@@
}
+ if (Object.keys(updates).length === 0) {
+ throw new TRPCError({
+ code: 'BAD_REQUEST',
+ message: 'No feedback payload provided (vote or comment required).',
+ });
+ }
+
await db
.update(assistantMessages)
.set(updates)
.where(eq(assistantMessages.id, input.messageId));Also applies to: 248-251
💡 Verification agent
🧩 Analysis chain
Avoid lost updates on vote counters (race condition) by using atomic SQL increments
Two clients upvoting simultaneously can read the same message.upvotes and overwrite each other, undercounting votes. Prefer an atomic SET upvotes = upvotes + 1/SET downvotes = downvotes + 1 in a single UPDATE. This eliminates the read-modify-write race.
Example (requires import { sql } from 'drizzle-orm'):
// inside addFeedback, instead of computing `updates` from a prior read
if (input.type === 'upvote') {
await db
.update(assistantMessages)
.set({ upvotes: sql`${assistantMessages.upvotes} + 1` })
.where(eq(assistantMessages.id, input.messageId));
} else if (input.type === 'downvote') {
await db
.update(assistantMessages)
.set({ downvotes: sql`${assistantMessages.downvotes} + 1` })
.where(eq(assistantMessages.id, input.messageId));
}If you keep the comment append in the same request, wrap both statements in a transaction to ensure consistency.
Run this to confirm schema defaults (ideally upvotes/downvotes default to 0 and feedbackComments defaults to []):
🏁 Script executed:
#!/bin/bash
# Verify assistantMessages schema has safe defaults for counters & comments
rg -nP -C3 '(assistantMessages|assistant_messages)\s*=\s*(pgTable|mysqlTable|sqliteTable)\('
rg -nP -C3 '\b(upvotes|downvotes|feedbackComments)\b'Length of output: 3437
Refactor vote updates to use atomic SQL increments
The current implementation in packages/rpc/src/routers/assistant.ts reads message.upvotes/message.downvotes, increments in JavaScript, then writes back—introducing a race condition when multiple clients vote concurrently and potentially overwrite each other’s updates.
Suggested changes:
- Replace the read-modify-write pattern with an atomic SQL increment via Drizzle’s
sqlhelper:
import { sql } from 'drizzle-orm'
import { eq } from 'drizzle-orm'
import { assistantMessages } from 'packages/db/src/drizzle/schema'
if (input.type === 'upvote') {
await db
.update(assistantMessages)
.set({ upvotes: sql`${assistantMessages.upvotes} + 1` })
.where(eq(assistantMessages.id, input.messageId))
} else if (input.type === 'downvote') {
await db
.update(assistantMessages)
.set({ downvotes: sql`${assistantMessages.downvotes} + 1` })
.where(eq(assistantMessages.id, input.messageId))
}- If you’re also appending to
feedbackCommentsin the same request, wrap both operations in a transaction to ensure consistency:
await db.transaction(async (tx) => {
// atomic vote increment (as above) using tx instead of db…
// then comment append via tx.update(...)
})Schema verification confirms:
upvotesanddownvotesare defined asinteger(...).default(0).notNull(), preventing nulls and initializing counts at 0.feedbackCommentsis a nullablejsonbcolumn (no default). Your code’s|| []fallback correctly handles this. If you prefer a DB-level default of[], consider chaining.default([]).notNull()on that column inschema.ts, though it’s optional given the current fallback.
This refactor is essential to eliminate lost updates under concurrent load and ensure vote counters remain accurate.
🤖 Prompt for AI Agents
In packages/rpc/src/routers/assistant.ts around lines 223-228 the code does a
read-modify-write by reading message.upvotes/message.downvotes and incrementing
in JS which causes race conditions; replace that with an atomic SQL increment
using Drizzle's sql helper and eq predicate (import sql and eq and
assistantMessages), e.g. perform db.update(assistantMessages).set({ upvotes:
sql`${assistantMessages.upvotes} + 1` }).where(eq(assistantMessages.id,
input.messageId)) (or downvotes similarly) instead of writing back computed
counts, and if you also append to feedbackComments in the same request wrap both
operations in db.transaction and use the transaction object (tx) for both
updates to ensure consistency.
There was a problem hiding this comment.
If upvotes/downvotes can be null in existing rows, message.upvotes + 1 yields NaN. Use ?? 0.
the DB has default value of upVotes and downVotes as 0, so the null thing won't be an issue.
There was a problem hiding this comment.
@sbansal1999, you're absolutely right about the database defaults—I can see from the schema that upvotes and downvotes are defined as integer(...).default(0).notNull(), so null values won't be an issue.
However, the more critical concern remains: race conditions during concurrent voting. When multiple users vote simultaneously, they can read the same counter value, increment it in JavaScript, and overwrite each other's updates, leading to lost votes.
The atomic SQL increment approach I suggested eliminates this race condition entirely:
if (input.type === 'upvote') {
await db
.update(assistantMessages)
.set({ upvotes: sql`${assistantMessages.upvotes} + 1` })
.where(eq(assistantMessages.id, input.messageId));
}This ensures vote counts remain accurate under concurrent load, which is essential for a feedback system.
✏️ Learnings added
Learnt from: sbansal1999
PR: databuddy-analytics/Databuddy#111
File: packages/rpc/src/routers/assistant.ts:228-233
Timestamp: 2025-08-24T08:00:03.800Z
Learning: In the Databuddy assistant messages schema, upvotes and downvotes columns are defined with integer(...).default(0).notNull(), so they cannot be null and will always default to 0.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
If neither type nor comment is provided (client bug or tampering), short-circuit with BAD_REQUEST before executing the update.
Added zod validation to prevent that.
There was a problem hiding this comment.
@sbansal1999, you're absolutely correct! The Zod validation with .refine((v) => v.type || v.comment, { message: 'Either type or comment must be provided' }) already prevents empty updates by ensuring at least one field is provided before the mutation logic executes. This makes my suggestion about the runtime Object.keys(updates).length === 0 check redundant.
However, the race condition concern with concurrent voting still remains the primary issue—when multiple users vote simultaneously on the same message, the current read-modify-write pattern can lead to lost votes. The atomic SQL increment approach would still be valuable to ensure vote accuracy under concurrent load.
✏️ Learnings added
Learnt from: sbansal1999
PR: databuddy-analytics/Databuddy#111
File: packages/rpc/src/routers/assistant.ts:228-233
Timestamp: 2025-08-24T08:00:27.729Z
Learning: The addFeedback mutation in packages/rpc/src/routers/assistant.ts uses Zod validation with .refine((v) => v.type || v.comment) to ensure either a vote type or comment is provided, preventing empty updates from reaching the database operation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sbansal1999
PR: databuddy-analytics/Databuddy#111
File: packages/rpc/src/routers/assistant.ts:228-233
Timestamp: 2025-08-24T08:00:03.800Z
Learning: In the Databuddy assistant messages schema, upvotes and downvotes columns are defined with integer(...).default(0).notNull(), so they cannot be null and will always default to 0.
8843a02 to
cc6f4c4
Compare
cc6f4c4 to
0605af0
Compare
2 participants
Summary by CodeRabbit