Conversation
📝 WalkthroughWalkthroughIntegrates PostHog analytics across chat features: session store emits events for message send/start/stop/retry and tool approvals (resolving tool names); chat UI emits events for new chats, cost-disclaimer lifecycle, and history open/load/delete. Adds a retry flag to streaming start and a guard in disclaimer dismiss. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates PostHog analytics across the chat interface, tracking events such as message submission, conversation starts, tool approvals, and history interactions. A logic issue was identified where the chat_conversation_started event could be triggered multiple times if a user retries the first message of a conversation before a trace ID is assigned.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/web_ui/src/lib/chat/chat_session_store.ts`:
- Around line 169-177: beginStreaming currently emits analytics events but is
also called by retryLastRequest, causing retries to be logged as new sends;
update beginStreaming to accept a source parameter (e.g., source: "send" |
"retry"), have sendMessage call beginStreaming("send") and retryLastRequest call
beginStreaming("retry"), then only emit chat_conversation_started when source
=== "send" && !traceId; optionally only emit chat_message_sent when source ===
"send" to keep retries separate from sends. Ensure you update signatures and
callers for beginStreaming and adjust the logic that emits posthog.capture for
chat_message_sent and chat_conversation_started accordingly.
In `@app/web_ui/src/routes/`(app)/chat/chat_history.svelte:
- Around line 51-55: The analytics event is fired before verifying the dialog
actually opened; update the open() function so
posthog.capture("chat_history_opened") is called only after a successful
historyDialog?.show() (i.e., check historyDialog exists and call show() first),
and then trigger posthog.capture; keep void loadSessionList() as-is after
showing. Reference: modify open(), historyDialog?.show(), posthog.capture, and
loadSessionList() so the capture happens post-show and only when historyDialog
is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d818490d-2773-4986-8324-dbb330723bb7
📒 Files selected for processing (4)
app/web_ui/src/lib/chat/chat_session_store.tsapp/web_ui/src/routes/(app)/chat/chat.svelteapp/web_ui/src/routes/(app)/chat/chat_cost_disclaimer.svelteapp/web_ui/src/routes/(app)/chat/chat_history.svelte
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEADNo lines with coverage information in this diff.
|
…re dialog did open before firing
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/web_ui/src/lib/chat/chat_session_store.ts (1)
406-434:toolNameForCallIdresolution is correctly ordered before state mutation.Calling
toolNameForCallId(toolCallId)before thecombined.update(...)/maybeFinishToolApproval()ensures the lookup still seestoolApprovalWaiterpopulated — important sincemaybeFinishToolApprovalcan clear it. Good sequencing.One small nit:
applyToolApprovalRunandapplyToolApprovalSkipare now near-identical aside from the event name and the boolean pick. If you ever add a third decision type, extracting a shared helper (e.g.,applyToolApprovalDecision(id, decision)) would cut the duplication, but it's not worth it at two call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/chat/chat_session_store.ts` around lines 406 - 434, Both applyToolApprovalRun and applyToolApprovalSkip duplicate the same logic; extract a helper applyToolApprovalDecision(toolCallId: string, pick: boolean, eventName: string) that first checks get(combined).toolApprovalWaiter, captures the event via posthog.capture(eventName, { tool_name: toolNameForCallId(toolCallId) }), performs combined.update to set toolApprovalPicks[toolCallId] = pick, and then calls maybeFinishToolApproval(); then implement applyToolApprovalRun and applyToolApprovalSkip as thin wrappers that call applyToolApprovalDecision(toolCallId, true, "chat_tool_approval_run") and applyToolApprovalDecision(toolCallId, false, "chat_tool_approval_skip"), preserving the current ordering (call toolNameForCallId before the state update).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/web_ui/src/lib/chat/chat_session_store.ts`:
- Around line 406-434: Both applyToolApprovalRun and applyToolApprovalSkip
duplicate the same logic; extract a helper applyToolApprovalDecision(toolCallId:
string, pick: boolean, eventName: string) that first checks
get(combined).toolApprovalWaiter, captures the event via
posthog.capture(eventName, { tool_name: toolNameForCallId(toolCallId) }),
performs combined.update to set toolApprovalPicks[toolCallId] = pick, and then
calls maybeFinishToolApproval(); then implement applyToolApprovalRun and
applyToolApprovalSkip as thin wrappers that call
applyToolApprovalDecision(toolCallId, true, "chat_tool_approval_run") and
applyToolApprovalDecision(toolCallId, false, "chat_tool_approval_skip"),
preserving the current ordering (call toolNameForCallId before the state
update).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 555acdc7-6970-45f6-8ad5-a1c7de4e6a01
📒 Files selected for processing (2)
app/web_ui/src/lib/chat/chat_session_store.tsapp/web_ui/src/routes/(app)/chat/chat_history.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- app/web_ui/src/routes/(app)/chat/chat_history.svelte
leonardmq
requested review from
chiang-daniel,
scosman,
sfierro and
tawnymanticore
1 participant
What does this PR do?
Add analytics events to chat.
New events:
chat_message_sentis_new_conversation(boolean),message_length(number),has_app_context_header(boolean),message_count(number)chat_conversation_startedchat_stoppedchat_retrychat_tool_approval_runtool_name(string | undefined)chat_tool_approval_skiptool_name(string | undefined)chat_new_chat_clickedhad_messages(boolean)chat_cost_disclaimer_shownchat_cost_disclaimer_acceptedchat_cost_disclaimer_declinedchat_history_openedchat_history_session_loadedmessage_count(number)chat_history_session_deletedChecklists
Summary by CodeRabbit