Fix conversation auto-scroll and yolo mode tool approvals#190
Merged
georpar merged 6 commits intoPiebald-AI:mainfrom Mar 8, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an isYoloEnabled flag propagated into conversation event handling (auto-approves tool calls when enabled) and removes the shared Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Hook as useConversationEvents
participant Backend
User->>Frontend: Trigger action causing tool_call
Frontend->>Hook: Emit tool_call event (hook configured with isYoloEnabled)
alt YOLO enabled
Hook->>Hook: Select approval outcome (prefer allow_always/allow_once or proceed_always)
Hook->>Backend: api.send_tool_call_confirmation_response(auto-approve)
Backend-->>Hook: Ack
Hook-->>Frontend: Continue processing (no user prompt)
else YOLO disabled
Hook->>Hook: Store confirmation request in Map
Hook-->>Frontend: Update confirmation requests state
Frontend->>User: Render confirmation prompt
User->>Frontend: Submit choice
Frontend->>Hook: Deliver user's confirmation
Hook->>Backend: api.send_tool_call_confirmation_response(user_choice)
Backend-->>Hook: Ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/App.tsx`:
- Around line 454-491: The context currently includes messagesContainerRef even
though no consumer uses it; remove messagesContainerRef from the object returned
by the useMemo (contextValue) and from its dependency array in App.tsx, and also
remove it from the corresponding Context type/interface and any provider prop
definitions (so things like messagesContainerRef are not exported from the app
context). Ensure no other code references messagesContainerRef after removal; if
any remaining usages exist, switch them to localContainerRef (as used in
HomeDashboard) or update those callers accordingly.
In `@frontend/src/hooks/useConversationEvents.ts`:
- Around line 273-274: The callback setupEventListenerForConversation currently
includes isYoloEnabled in its dependency array which causes a full
teardown/re-register of listeners when YOLO toggles; change isYoloEnabled to a
ref: create isYoloEnabledRef via useRef<boolean>(), update that ref wherever the
toggle changes, replace direct reads of isYoloEnabled inside
setupEventListenerForConversation with isYoloEnabledRef.current, and remove
isYoloEnabled from the useCallback dependency array so the callback identity
remains stable and doesn't trigger the App.tsx effect to re-register all
listeners.
In `@frontend/src/pages/HomeDashboard.tsx`:
- Around line 70-84: The auto-scroll effect in HomeDashboard (the
React.useEffect that reads localContainerRef,
currentConversation?.messages.length and currentConversation?.isStreaming) won't
run for appended text chunks because messages.length doesn't change; update the
effect to also depend on the tail message's dynamic content (e.g., include
currentConversation?.messages?.[currentConversation.messages.length-1]?.parts?.length
or a computed lastMessageContentLength) or attach a
MutationObserver/ResizeObserver to localContainerRef to trigger scrolling when
the container's scrollHeight changes; ensure you still check isNearBottomRef
before setting container.scrollTop.
- Around line 42-43: Remove the leftover debug console.log in HomeDashboard.tsx
that prints currentConversation on every render; locate the statement
referencing currentConversation (console.log("🏠 HomeDashboard -
currentConversation:", currentConversation)) inside the HomeDashboard component
and delete it (or replace it with a conditional debug logger that only runs in
development, e.g., gated by process.env.NODE_ENV === "development"). Ensure no
other stray debug logs remain in the HomeDashboard component.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx`: - Around line 454-491: The context currently includes messagesContainerRef even though no consumer uses it; remove messagesContainerRef from the object returned by the useMemo (contextValue) and from its dependency array in App.tsx, and also remove it from the corresponding Context type/interface and any provider prop definitions (so things like messagesContainerRef are not exported from the app context). Ensure no other code references messagesContainerRef after removal; if any remaining usages exist, switch them to localContainerRef (as used in HomeDashboard) or update those callers accordingly. In `@frontend/src/pages/HomeDashboard.tsx`: - Around line 42-43: Remove the leftover debug console.log in HomeDashboard.tsx that prints currentConversation on every render; locate the statement referencing currentConversation (console.log("🏠 HomeDashboard - currentConversation:", currentConversation)) inside the HomeDashboard component and delete it (or replace it with a conditional debug logger that only runs in development, e.g., gated by process.env.NODE_ENV === "development"). Ensure no other stray debug logs remain in the HomeDashboard component.frontend/src/App.tsx (1)
454-491:messagesContainerRefis still provided in context but no longer consumed.HomeDashboard now uses
localContainerRefinstead ofmessagesContainerReffrom context. If nothing else consumesmessagesContainerRef, it's dead code in the context value (lines 461, 479). Consider removing it from the context to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 454 - 491, The context currently includes messagesContainerRef even though no consumer uses it; remove messagesContainerRef from the object returned by the useMemo (contextValue) and from its dependency array in App.tsx, and also remove it from the corresponding Context type/interface and any provider prop definitions (so things like messagesContainerRef are not exported from the app context). Ensure no other code references messagesContainerRef after removal; if any remaining usages exist, switch them to localContainerRef (as used in HomeDashboard) or update those callers accordingly.frontend/src/pages/HomeDashboard.tsx (1)
42-43: Remove debugconsole.logleft in production code.Line 43 logs
currentConversationon every render, which is noisy in production.🧹 Proposed fix
- // Debug logging for currentConversation - console.log("🏠 HomeDashboard - currentConversation:", currentConversation);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/HomeDashboard.tsx` around lines 42 - 43, Remove the leftover debug console.log in HomeDashboard.tsx that prints currentConversation on every render; locate the statement referencing currentConversation (console.log("🏠 HomeDashboard - currentConversation:", currentConversation)) inside the HomeDashboard component and delete it (or replace it with a conditional debug logger that only runs in development, e.g., gated by process.env.NODE_ENV === "development"). Ensure no other stray debug logs remain in the HomeDashboard component.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useConversationEvents.ts`:
- Around line 923-958: The YOLO auto-approve logic currently picks the first
matching option (so it prefers allow_once) and on API failure it never stores
the confirmation request, leaving the call stuck; update the block that handles
isYoloEnabledRef.current to (1) prefer an option with kind === "allow_always",
then fall back to "allow_once", then to "proceed_always" for outcome selection
(use legacyConfirmationRequest.options to find those), and (2) when calling
api.send_tool_call_confirmation_response (the call using sessionId, requestId,
toolCallId, outcome), catch errors and in the catch handler store the
legacyConfirmationRequest into the confirmationRequests Map via
setConfirmationRequests (same logic as the non-YOLO branch) so the UI can
present the dialog and retry; keep error logging in the catch path.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/HomeDashboard.tsx (1)
231-233:⚠️ Potential issue | 🟡 MinorAvoid
Date.now()in React keys — causes unnecessary remounts.Including
Date.now()in the key forces React to treat the component as new on every render, unmounting and remounting it. This defeats React's reconciliation and can cause lost state, performance issues, and visual flicker.🐛 Proposed fix
return ( <ToolCallDisplay - key={`${ - msgPart.toolCall.id - }-${hasConfirmation}-${Date.now()}`} + key={`${msgPart.toolCall.id}-${hasConfirmation}`} toolCall={msgPart.toolCall}If the intent is to force re-render when confirmation state changes,
hasConfirmationalready provides that differentiation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/HomeDashboard.tsx` around lines 231 - 233, The key currently uses Date.now() which forces remounts; change the key to a stable value derived from the message part and confirmation state instead (e.g., combine msgPart.toolCall.id with hasConfirmation) so React can reconcile correctly—update the key expression that references msgPart.toolCall.id and hasConfirmation to remove Date.now() and use a stable unique identifier (or include an existing stable field like a part id/index if needed).
♻️ Duplicate comments (1)
frontend/src/hooks/useConversationEvents.ts (1)
923-943:⚠️ Potential issue | 🟠 MajorYOLO auto-approve still picks
allow_onceoverallow_alwaysand lacks error recovery.Two concerns from the previous review remain:
Option priority:
find()returns the first match. PergetOptionKind()(lines 250-262), options are ordered[allow_once, allow_always, ...]. This always picksallow_oncewhen both exist. For YOLO mode,allow_alwaysbetter matches the intent and reduces future prompts.Silent failure: If the API call fails, the error is logged but the confirmation request isn't stored in the Map. The UI won't show a dialog, and the tool call remains stuck with no recovery path.
🐛 Proposed fix
// If yolo mode is enabled, auto-approve the tool call if (isYoloEnabledRef.current) { console.log(`🤖 [YOLO] Auto-approving tool call: ${toolCallId}`); - // Find the "allow_always" or "allow_once" option and use it - const autoApproveOption = legacyConfirmationRequest.options?.find( - (opt) => - opt.kind === "allow_always" || opt.kind === "allow_once" + // Prefer "allow_always" for YOLO mode, fall back to "allow_once" + const autoApproveOption = + legacyConfirmationRequest.options?.find( + (opt) => opt.kind === "allow_always" + ) ?? + legacyConfirmationRequest.options?.find( + (opt) => opt.kind === "allow_once" ); const outcome = autoApproveOption?.optionId || "proceed_always"; // Send approval response to backend api .send_tool_call_confirmation_response({ sessionId: conversationId, requestId: legacyConfirmationRequest.requestId, toolCallId: toolCallId, outcome: outcome, }) .catch((err) => { console.error("Failed to send auto-approve response:", err); + // Fallback: store in confirmation requests so user can manually approve + setConfirmationRequests((prev) => { + const newMap = new Map(prev); + newMap.set(toolCallId, legacyConfirmationRequest); + return newMap; + }); }); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useConversationEvents.ts` around lines 923 - 943, The YOLO auto-approve currently picks the first matching option (which ends up being allow_once) and silently drops the confirmation on API failure; update the selection to prefer "allow_always" over "allow_once" (e.g., check for allow_always first, then allow_once) when computing outcome for legacyConfirmationRequest, and in the send_tool_call_confirmation_response.catch handler, persist the legacyConfirmationRequest into the confirmationRequests Map (the same map used to surface confirmation dialogs) keyed by requestId or toolCallId so the UI can show the dialog and the tool call can be recovered.
🧹 Nitpick comments (1)
frontend/src/App.tsx (1)
160-166: EnsureisYoloEnabledis strictly boolean to avoid passingundefined.The
yolofield is optional (yolo?: boolean) in bothGeminiConfigandQwenConfig. When the config exists butyolois not set, this expression evaluates toundefinedrather thanfalse. WhileuseConversationEventshandles this with?? false, explicitly coercing to boolean here improves type safety and clarity.♻️ Proposed fix
// Get yolo mode status from backend config const isYoloEnabled = backendState.selectedBackend === "gemini" - ? backendState.configs.gemini.yolo + ? backendState.configs.gemini.yolo ?? false : backendState.selectedBackend === "qwen" - ? backendState.configs.qwen.yolo + ? backendState.configs.qwen.yolo ?? false : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 160 - 166, The computed isYoloEnabled can be undefined because GeminiConfig.yolo and QwenConfig.yolo are optional; update the expression that derives isYoloEnabled (the const isYoloEnabled using backendState.selectedBackend and backendState.configs.gemini.yolo / backendState.configs.qwen.yolo) to coerce the result to a strict boolean (e.g., use the nullish coalescing operator ?? false or a Boolean/!! cast) so it always yields true or false before being passed into useConversationEvents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/pages/HomeDashboard.tsx`:
- Around line 231-233: The key currently uses Date.now() which forces remounts;
change the key to a stable value derived from the message part and confirmation
state instead (e.g., combine msgPart.toolCall.id with hasConfirmation) so React
can reconcile correctly—update the key expression that references
msgPart.toolCall.id and hasConfirmation to remove Date.now() and use a stable
unique identifier (or include an existing stable field like a part id/index if
needed).
---
Duplicate comments:
In `@frontend/src/hooks/useConversationEvents.ts`:
- Around line 923-943: The YOLO auto-approve currently picks the first matching
option (which ends up being allow_once) and silently drops the confirmation on
API failure; update the selection to prefer "allow_always" over "allow_once"
(e.g., check for allow_always first, then allow_once) when computing outcome for
legacyConfirmationRequest, and in the send_tool_call_confirmation_response.catch
handler, persist the legacyConfirmationRequest into the confirmationRequests Map
(the same map used to surface confirmation dialogs) keyed by requestId or
toolCallId so the UI can show the dialog and the tool call can be recovered.
---
Nitpick comments:
In `@frontend/src/App.tsx`:
- Around line 160-166: The computed isYoloEnabled can be undefined because
GeminiConfig.yolo and QwenConfig.yolo are optional; update the expression that
derives isYoloEnabled (the const isYoloEnabled using
backendState.selectedBackend and backendState.configs.gemini.yolo /
backendState.configs.qwen.yolo) to coerce the result to a strict boolean (e.g.,
use the nullish coalescing operator ?? false or a Boolean/!! cast) so it always
yields true or false before being passed into useConversationEvents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9d4ec41-a6a8-49e4-ab6e-03348cbde576
📒 Files selected for processing (3)
frontend/src/App.tsxfrontend/src/hooks/useConversationEvents.tsfrontend/src/pages/HomeDashboard.tsx
Member
|
Excellent contribution. Thank you! |
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
2 participants
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.
Changes
1. Fix Conversation Auto-Scroll
The conversation window now auto-scrolls to new messages, but only when the user is near the bottom. This prevents annoying jumps when scrolling up to read previous messages.
2. Fix Yolo Mode Auto-Approve
When yolo mode is enabled, tool calls are now automatically approved without requiring manual confirmation.
Testing
Summary by CodeRabbit
New Features
Refactor
Documentation