feat(ambient-ui): chat tab, action bar, persistent chat sidebar#1636
Conversation
- Replace hardcoded hex colors with semantic Tailwind tokens for dark mode - Add react-markdown + remark-gfm for assistant message rendering - Add @tailwindcss/typography for prose styling - Fix tool payload parsing to handle operational event format - Filter out empty assistant message bubbles - Strip wrapping quotes from tool result display - Hide empty arguments section when tool has no args - Spec: add Persistent Chat Sidebar requirement, update Chat tab scenario Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Chat tab: - Markdown rendering for user and assistant messages (react-markdown + remark-gfm) - Tool call/result grouping into single collapsible blocks - Extract last_assistant_message from system events when assistant payload is empty - Dark mode support (semantic Tailwind tokens, no hardcoded hex) - Scroll-to-top/bottom buttons with tooltips, matching old frontend pattern - Input hint text below textarea, removed redundant phase badge from input Action bar: - Stop/Restart as primary buttons, Export/Delete in 3-dot dropdown menu - Delete with confirmation dialog, navigates to fleet on success - AlertDialog shadcn component added Persistent chat sidebar: - Pop-out button on Chat tab opens right-edge sidebar - Sidebar persists across page navigation (context at dashboard layout level) - Resizable by dragging, collapsible to narrow strip - Session name in header links to session detail page - Chat icon on fleet table rows opens sidebar - Only one sidebar at a time, replacing on new session Runner: - Accumulate tool args from TOOL_CALL_ARGS events, push on TOOL_CALL_END - Tool use messages now include full input arguments - Updated tests for accumulation flow (22 passing) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Main content area clips overflow so sticky header can't bleed into sidebar - Sidebar gets z-20 to always render above main content - Sidebar session ID persisted in URL (?chat=<id>) — survives reload - Active tab persisted in URL (?tab=logs) — survives reload - Sidebar goes full-screen on mobile widths (max-md breakpoint) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
|
Warning Review limit reached
More reviews will be available in 10 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements a persistent chat sidebar and chat tab UI, message parsing/grouping and send hooks, session delete/export confirmations, Radix AlertDialog wrappers, Tailwind typography and markdown deps, and runner-side tool-call arg accumulation with updated tests and spec changes. ChangesChat UI & Session Management
Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Review — Request Changes
Well-executed feature overall: clean context/provider split, good a11y (role="log", aria-label, keyboard resize), proper event listener cleanup, and the runner tool-arg accumulation change (TOOL_CALL_START → accumulate, emit on TOOL_CALL_END) is the right design. Three issues need fixing before merge.
Major
1. session-header.tsx:262 — useSessionMessages called unconditionally for export
const { data: messages } = useSessionMessages(session.id)This fires on every render of the session detail page — every user who opens any session now fetches the full message history, even if they never touch the export button. Message history can be large. The fetch should be deferred: either lazy-load on dropdown open, or move export into a separate component that mounts only when the menu is open.
2. chat-messages.tsx:750-759 — Orphan tool_result fallback is non-deterministic with concurrent tools
} else {
const lastPending = Array.from(pendingToolUses.values()).pop()
if (lastPending && !lastPending.toolResult) {
lastPending.toolResult = msg
} else {
items.push({ kind: 'message', message: msg })
}
}Claude Code issues parallel tool calls frequently. When toolCallId matching fails, falling back to "last pending" by insertion order will attach the result to the wrong tool call. The Map preserves insertion order, so under concurrent execution pop() is essentially random from the user's perspective. The safest fix is to always surface unmatched results as standalone messages (items.push(...)) rather than guessing which tool they belong to.
3. chat-sidebar.tsx:882-884 — Queries fire with empty string ID when sidebar is closed
const { data: session } = useSession(openSessionId ?? '', undefined)
const { data: messagesData } = useSessionMessages(openSessionId ?? '')When openSessionId is null (sidebar closed — the default state), these hooks receive '' and fire unless the underlying queries have guards. The sidebar is closed on most page renders. Both calls need enabled: !!openSessionId:
const { data: session } = useSession(openSessionId ?? '', { enabled: !!openSessionId })
const { data: messagesData } = useSessionMessages(openSessionId ?? '', { enabled: !!openSessionId })Minor
4. operational_events.py — TOOL_CALL_START and TOOL_CALL_ARGS in _SKIP_TYPES is dead code
Both are caught by explicit early returns in consume() before the _SKIP_TYPES check is reached. Remove them from the frozenset or add a comment explaining the defensive intent.
5. use-send-message.ts:1221 — Adapter re-instantiated on every render
const adapter = port ?? createSessionMessagesAdapterWithFetch()Wrap in useMemo or useRef (the same pattern used in the session queries).
6. enrichMessages silent drop needs a comment
Empty assistant messages that have no nearby system event are silently dropped (the continue applies even when no extraction succeeded). This is probably intentional, but a one-liner explaining why would prevent future confusion.
Fix 1–3 and this is ready to ship. The UI architecture (context + layout-level sidebar + URL state) is solid.
— amber
- Defer message fetch for export to on-click (was eager on every render) - Remove non-deterministic orphan tool_result fallback (always standalone) - Replace useSearchParams with window.location to fix SSG build failure - Memoize adapter in useSendMessage via useRef - Remove dead TOOL_CALL_START/ARGS from _SKIP_TYPES - Add comment explaining empty assistant message drop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsx:
- Around line 59-66: The dialog is being closed immediately in
handleConfirmDelete after calling deleteSession.mutate, which hides errors if
the deletion fails; move the setDeleteDialogOpen(false) call into the mutate
onSuccess callback (alongside the router.push) so the dialog only closes when
deleteSession.mutate succeeds, keeping deleteSession, mutate, onSuccess,
setDeleteDialogOpen and router.push as the referenced symbols to locate and
update.
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/fleet/[sessionId]/page.tsx:
- Around line 14-22: The Tabs component is currently uncontrolled (using
defaultValue) so it won't update when searchParams changes; switch to controlled
mode by passing value={activeTab} (derived from useSearchParams()) instead of
defaultValue, and update handleTabChange to use the framework router replace
(e.g., router.replace()) to update the URL search param 'tab' so back/forward
navigation syncs; update references in this file for activeTab, handleTabChange,
and useSearchParams to ensure the Tabs reads activeTab and reacts to search
param changes.
In `@components/ambient-ui/src/components/chat-messages.tsx`:
- Around line 348-420: The component currently ignores the incoming disabled
prop in the actual send path: update ChatInput so the disabled prop is honored
everywhere — add disabled checks to handleSend (include "!disabled" in the
early-return condition alongside !isRunning and sendMessage.isPending), include
disabled in the Textarea disabled prop (change disabled={!isRunning ||
sendMessage.isPending} to disabled={!isRunning || sendMessage.isPending ||
disabled}), and guard handleKeyDown (return early if disabled) and/or the
Textarea onChange so typing/Enter cannot submit when disabled; reference
functions/variables: ChatInput, handleSend, handleKeyDown, canSend, textareaRef,
and the Textarea component.
- Around line 342-420: ChatInput currently receives the phase prop but never
renders the session status; update the ChatInput component to display a small
phase badge using the existing phase value (and/or isRunning) next to the input
area (for example above or to the left of the Textarea inside the outer div).
Use the phase string to set the badge text and optionally apply different styles
for 'Running' vs other phases; ensure the badge is rendered regardless of
disabled/sendMessage state so the status is always visible.
In `@components/ambient-ui/src/components/chat-sidebar-context.tsx`:
- Around line 23-46: The ChatSidebarProvider currently seeds openSessionId from
useSearchParams but never re-syncs on browser back/forward and updateChatParam
uses window.history.replaceState; change updateChatParam to use Next.js
router.replace for shallow updates, and in ChatSidebarProvider add a useEffect
that watches the router or searchParams (e.g., router.asPath or
searchParams.get('chat')) and calls setOpenSessionId(searchParams.get('chat') ??
null) to keep state in sync; update openSidebar and closeSidebar to call the new
updateChatParam (which should accept router) so all URL changes go through
Next.js router.replace with shallow: true to avoid routing inconsistencies.
In `@components/ambient-ui/src/components/chat-sidebar.tsx`:
- Around line 77-105: handleMouseDown currently registers document
mousemove/mouseup handlers that are only removed inside handleMouseUp, which
leaks listeners and body style overrides if the component unmounts mid-drag; fix
by promoting the handler functions (handleMouseMove, handleMouseUp) and their
references (e.g., refs like isDragging, startX, startWidth) so they can be
removed from document in a useEffect cleanup: store the handlers in stable refs
or module-level consts so handleMouseDown adds them and both handleMouseUp and
the useEffect return cleanup remove them and restore
document.body.style.cursor/userSelect; update handleMouseDown to set the refs
and add listeners, keep the existing removal in handleMouseUp, and add a
useEffect that on unmount checks/removes the same listeners and resets body
styles to ensure no leak.
In
`@components/runners/ambient-runner/ambient_runner/bridges/claude/operational_events.py`:
- Around line 66-70: The payload["input"] assignment must always produce an
object/dict for downstream consumers; update the block that parses args (the
payload["input"] = json.loads(args) / fallback) to normalize non-dict results
into an object: try to json.loads(args), if result is a dict use it as-is,
otherwise wrap the parsed value into a dict (e.g., {"value": parsed}) and on
JSONDecodeError/TypeError set payload["input"] = {"text": str(args)}; adjust the
logic around json.loads, args, and payload["input"] accordingly so
tool_use.input is always an object.
🪄 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: Enterprise
Run ID: 78806701-a7ed-4f41-b906-e0078635976c
⛔ Files ignored due to path filters (1)
components/ambient-ui/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (20)
components/ambient-ui/package.jsoncomponents/ambient-ui/src/adapters/sdk-sessions.tscomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/chat-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/page.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-columns.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/_components/fleet-table.tsxcomponents/ambient-ui/src/app/(dashboard)/layout.tsxcomponents/ambient-ui/src/app/globals.csscomponents/ambient-ui/src/components/chat-messages.tsxcomponents/ambient-ui/src/components/chat-sidebar-context.tsxcomponents/ambient-ui/src/components/chat-sidebar.tsxcomponents/ambient-ui/src/components/ui/alert-dialog.tsxcomponents/ambient-ui/src/ports/sessions.tscomponents/ambient-ui/src/queries/index.tscomponents/ambient-ui/src/queries/use-send-message.tscomponents/ambient-ui/src/queries/use-sessions.tscomponents/runners/ambient-runner/ambient_runner/bridges/claude/operational_events.pycomponents/runners/ambient-runner/tests/test_operational_events.pyspecs/ambient-ui/ambient-ui.spec.md
| const searchParams = useSearchParams() | ||
| const activeTab = searchParams.get('tab') ?? 'phase' | ||
| const { data: session, isLoading, error } = useSession(sessionId) | ||
|
|
||
| const handleTabChange = (value: string) => { | ||
| const url = new URL(window.location.href) | ||
| url.searchParams.set('tab', value) | ||
| window.history.replaceState({}, '', url.toString()) | ||
| } |
There was a problem hiding this comment.
Tabs won't sync with browser back/forward.
Using defaultValue makes the Tabs component uncontrolled—it only reads the prop on mount. When the user navigates back/forward, activeTab updates but the selected tab doesn't change. Use value={activeTab} for controlled behavior, and preferably router.replace() instead of window.history.replaceState.
🔄 Recommended fix for controlled tabs
+'use client'
+
+import { useParams, useSearchParams, useRouter, usePathname } from 'next/navigation'
-import { useParams, useSearchParams } from 'next/navigation'
export default function SessionDetailPage() {
const { sessionId } = useParams<{ projectId: string; sessionId: string }>()
+ const router = useRouter()
+ const pathname = usePathname()
const searchParams = useSearchParams()
const activeTab = searchParams.get('tab') ?? 'phase'
const handleTabChange = (value: string) => {
- const url = new URL(window.location.href)
- url.searchParams.set('tab', value)
- window.history.replaceState({}, '', url.toString())
+ const params = new URLSearchParams(searchParams)
+ params.set('tab', value)
+ router.replace(`${pathname}?${params.toString()}`)
}
// ...
return (
<div className="space-y-6">
<SessionHeader session={session} />
- <Tabs defaultValue={activeTab} onValueChange={handleTabChange}>
+ <Tabs value={activeTab} onValueChange={handleTabChange}>Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/fleet/[sessionId]/page.tsx
around lines 14 - 22, The Tabs component is currently uncontrolled (using
defaultValue) so it won't update when searchParams changes; switch to controlled
mode by passing value={activeTab} (derived from useSearchParams()) instead of
defaultValue, and update handleTabChange to use the framework router replace
(e.g., router.replace()) to update the URL search param 'tab' so back/forward
navigation syncs; update references in this file for activeTab, handleTabChange,
and useSearchParams to ensure the Tabs reads activeTab and reacts to search
param changes.
| type ChatInputProps = { | ||
| sessionId: string | ||
| phase: string | ||
| disabled: boolean | ||
| } | ||
|
|
||
| export function ChatInput({ sessionId, phase, disabled }: ChatInputProps) { | ||
| const [input, setInput] = useState('') | ||
| const textareaRef = useRef<HTMLTextAreaElement | null>(null) | ||
| const sendMessage = useSendMessage(sessionId) | ||
| const isRunning = phase === 'Running' | ||
| const canSend = isRunning && !disabled && input.trim().length > 0 && !sendMessage.isPending | ||
|
|
||
| const handleSend = useCallback(() => { | ||
| const trimmed = input.trim() | ||
| if (!trimmed || !isRunning || sendMessage.isPending) return | ||
| sendMessage.mutate(trimmed, { | ||
| onSuccess: () => { | ||
| setInput('') | ||
| textareaRef.current?.focus() | ||
| }, | ||
| }) | ||
| }, [input, isRunning, sendMessage]) | ||
|
|
||
| const handleKeyDown = useCallback( | ||
| (e: React.KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| handleSend() | ||
| } | ||
| }, | ||
| [handleSend], | ||
| ) | ||
|
|
||
| return ( | ||
| <div className="border-t bg-background px-4 py-3"> | ||
| {!isRunning && ( | ||
| <div className="flex items-center gap-2 mb-2"> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Send is disabled while the session is not running. | ||
| </span> | ||
| </div> | ||
| )} | ||
| {sendMessage.isError && ( | ||
| <div className="mb-2"> | ||
| <span className="text-xs text-destructive"> | ||
| Failed to send message. Please try again. | ||
| </span> | ||
| </div> | ||
| )} | ||
| <div className="flex gap-2"> | ||
| <Textarea | ||
| ref={textareaRef} | ||
| value={input} | ||
| onChange={e => setInput(e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder={isRunning ? 'Send a message...' : 'Session is not running'} | ||
| disabled={!isRunning || sendMessage.isPending} | ||
| className="min-h-[40px] max-h-[120px] resize-none" | ||
| aria-label="Chat message input" | ||
| rows={1} | ||
| /> | ||
| <Button | ||
| onClick={handleSend} | ||
| disabled={!canSend} | ||
| size="icon" | ||
| aria-label="Send message" | ||
| className="shrink-0 self-end" | ||
| > | ||
| <Send className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| {isRunning && ( | ||
| <p className="text-[10px] text-muted-foreground mt-1"> | ||
| Enter to send · Shift+Enter for new line | ||
| </p> | ||
| )} | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
Render the phase badge where this component already receives phase.
The supplied spec for this PR says the current status should appear near the input, but ChatInput never renders it. Right now phase is only used to gate sending, so the inline chat composer misses the promised status indicator.
Suggested fix
export function ChatInput({ sessionId, phase, disabled }: ChatInputProps) {
@@
return (
<div className="border-t bg-background px-4 py-3">
+ <div className="mb-2 flex items-center justify-between gap-2">
+ <span className="text-xs text-muted-foreground">
+ {isRunning ? 'Ready to send' : 'Send is disabled while the session is not running.'}
+ </span>
+ <PhaseIndicator phase={phase} />
+ </div>
- {!isRunning && (
- <div className="flex items-center gap-2 mb-2">
- <span className="text-xs text-muted-foreground">
- Send is disabled while the session is not running.
- </span>
- </div>
- )}
{sendMessage.isError && (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-ui/src/components/chat-messages.tsx` around lines 342 -
420, ChatInput currently receives the phase prop but never renders the session
status; update the ChatInput component to display a small phase badge using the
existing phase value (and/or isRunning) next to the input area (for example
above or to the left of the Textarea inside the outer div). Use the phase string
to set the badge text and optionally apply different styles for 'Running' vs
other phases; ensure the badge is rendered regardless of disabled/sendMessage
state so the status is always visible.
| function updateChatParam(sessionId: string | null) { | ||
| const url = new URL(window.location.href) | ||
| if (sessionId) { | ||
| url.searchParams.set('chat', sessionId) | ||
| } else { | ||
| url.searchParams.delete('chat') | ||
| } | ||
| window.history.replaceState({}, '', url.toString()) | ||
| } | ||
|
|
||
| export function ChatSidebarProvider({ children }: { children: ReactNode }) { | ||
| const searchParams = useSearchParams() | ||
| const initialChat = searchParams.get('chat') | ||
| const [openSessionId, setOpenSessionId] = useState<string | null>(initialChat) | ||
|
|
||
| const openSidebar = useCallback((sessionId: string) => { | ||
| setOpenSessionId(sessionId) | ||
| updateChatParam(sessionId) | ||
| }, []) | ||
|
|
||
| const closeSidebar = useCallback(() => { | ||
| setOpenSessionId(null) | ||
| updateChatParam(null) | ||
| }, []) |
There was a problem hiding this comment.
State desync on browser back/forward navigation.
The provider initializes state from the URL on mount but doesn't re-sync when the URL changes via browser navigation. Since this provider lives in a persistent layout, browser back/forward will update searchParams but leave openSessionId stale. Additionally, using window.history.replaceState bypasses Next.js's router, which can cause routing inconsistencies.
Use the Next.js router for controlled URL state:
🔄 Recommended fix using router.replace()
-import { useSearchParams, useRouter, usePathname } from 'next/navigation'
+import { useSearchParams, useRouter, usePathname } from 'next/navigation'
-function updateChatParam(sessionId: string | null) {
- const url = new URL(window.location.href)
- if (sessionId) {
- url.searchParams.set('chat', sessionId)
- } else {
- url.searchParams.delete('chat')
- }
- window.history.replaceState({}, '', url.toString())
-}
export function ChatSidebarProvider({ children }: { children: ReactNode }) {
+ const router = useRouter()
+ const pathname = usePathname()
const searchParams = useSearchParams()
- const initialChat = searchParams.get('chat')
- const [openSessionId, setOpenSessionId] = useState<string | null>(initialChat)
+ const openSessionId = searchParams.get('chat')
const openSidebar = useCallback((sessionId: string) => {
- setOpenSessionId(sessionId)
- updateChatParam(sessionId)
+ const params = new URLSearchParams(searchParams)
+ params.set('chat', sessionId)
+ router.replace(`${pathname}?${params.toString()}`)
- }, [])
+ }, [router, pathname, searchParams])
const closeSidebar = useCallback(() => {
- setOpenSessionId(null)
- updateChatParam(null)
+ const params = new URLSearchParams(searchParams)
+ params.delete('chat')
+ router.replace(`${pathname}?${params.toString()}`)
- }, [])
+ }, [router, pathname, searchParams])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function updateChatParam(sessionId: string | null) { | |
| const url = new URL(window.location.href) | |
| if (sessionId) { | |
| url.searchParams.set('chat', sessionId) | |
| } else { | |
| url.searchParams.delete('chat') | |
| } | |
| window.history.replaceState({}, '', url.toString()) | |
| } | |
| export function ChatSidebarProvider({ children }: { children: ReactNode }) { | |
| const searchParams = useSearchParams() | |
| const initialChat = searchParams.get('chat') | |
| const [openSessionId, setOpenSessionId] = useState<string | null>(initialChat) | |
| const openSidebar = useCallback((sessionId: string) => { | |
| setOpenSessionId(sessionId) | |
| updateChatParam(sessionId) | |
| }, []) | |
| const closeSidebar = useCallback(() => { | |
| setOpenSessionId(null) | |
| updateChatParam(null) | |
| }, []) | |
| export function ChatSidebarProvider({ children }: { children: ReactNode }) { | |
| const router = useRouter() | |
| const pathname = usePathname() | |
| const searchParams = useSearchParams() | |
| const openSessionId = searchParams.get('chat') | |
| const openSidebar = useCallback((sessionId: string) => { | |
| const params = new URLSearchParams(searchParams) | |
| params.set('chat', sessionId) | |
| router.replace(`${pathname}?${params.toString()}`) | |
| }, [router, pathname, searchParams]) | |
| const closeSidebar = useCallback(() => { | |
| const params = new URLSearchParams(searchParams) | |
| params.delete('chat') | |
| router.replace(`${pathname}?${params.toString()}`) | |
| }, [router, pathname, searchParams]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-ui/src/components/chat-sidebar-context.tsx` around lines
23 - 46, The ChatSidebarProvider currently seeds openSessionId from
useSearchParams but never re-syncs on browser back/forward and updateChatParam
uses window.history.replaceState; change updateChatParam to use Next.js
router.replace for shallow updates, and in ChatSidebarProvider add a useEffect
that watches the router or searchParams (e.g., router.asPath or
searchParams.get('chat')) and calls setOpenSessionId(searchParams.get('chat') ??
null) to keep state in sync; update openSidebar and closeSidebar to call the new
updateChatParam (which should accept router) so all URL changes go through
Next.js router.replace with shallow: true to avoid routing inconsistencies.
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Review — LGTM ✅
All issues from the prior round addressed in 813d5be.
- Export deferred:
useSessionMessagesremoved fromsession-header; export does a dynamic import +adapter.list()on click only. - Orphan tool_result:
pop()fallback gone; unmatched results always surface as standalone messages. - Empty-string queries:
chat-sidebar.tsxunchanged but bothuseSessionanduseSessionMessagescarryenabled: !!sessionId—''is falsy, no queries fire. Pre-existing guard covered it. - Minors:
TOOL_CALL_START/TOOL_CALL_ARGSremoved from_SKIP_TYPES, adapter memoized viauseRef, drop-comment added toenrichMessages.
— amber
- Dialog closes after mutation settles, not before (shows errors) - Send path honors disabled prop (blocks Enter key when loading) - Sidebar syncs with browser back/forward via popstate listener - Drag listeners cleaned up on unmount (no leaked event handlers) - Normalize tool_use.input to always be an object Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/ambient-ui/src/components/chat-sidebar-context.tsx (1)
21-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winState desync on browser back/forward persists; add popstate listener to sync.
Initializing
openSessionIdfromreadChatParam()on mount (line 37) captures the URL once, but browser back/forward won't update React state. Since this provider lives in a persistent layout, the sidebar will display stale session data after navigation.Given the SSG build constraint (why
useSearchParamswas removed), add apopstatelistener to re-sync state:🔄 Proposed fix with popstate listener
export function ChatSidebarProvider({ children }: { children: ReactNode }) { const [openSessionId, setOpenSessionId] = useState<string | null>(readChatParam) + + useEffect(() => { + const handlePopState = () => { + setOpenSessionId(readChatParam()) + } + window.addEventListener('popstate', handlePopState) + return () => window.removeEventListener('popstate', handlePopState) + }, []) const openSidebar = useCallback((sessionId: string) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ambient-ui/src/components/chat-sidebar-context.tsx` around lines 21 - 47, The ChatSidebarProvider initializes openSessionId from readChatParam but never updates when the browser history changes; add a window "popstate" listener inside a useEffect in ChatSidebarProvider that calls readChatParam and calls setOpenSessionId(...) to resync state (and clean up the listener on unmount), ensuring updateChatParam still updates URL when openSidebar/closeSidebar run; reference readChatParam, updateChatParam, ChatSidebarProvider, openSidebar and closeSidebar when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@components/ambient-ui/src/components/chat-sidebar-context.tsx`:
- Around line 21-47: The ChatSidebarProvider initializes openSessionId from
readChatParam but never updates when the browser history changes; add a window
"popstate" listener inside a useEffect in ChatSidebarProvider that calls
readChatParam and calls setOpenSessionId(...) to resync state (and clean up the
listener on unmount), ensuring updateChatParam still updates URL when
openSidebar/closeSidebar run; reference readChatParam, updateChatParam,
ChatSidebarProvider, openSidebar and closeSidebar when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3abe0fdc-cd52-49a5-86fc-79300b96cfe6
📒 Files selected for processing (6)
components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/page.tsxcomponents/ambient-ui/src/components/chat-messages.tsxcomponents/ambient-ui/src/components/chat-sidebar-context.tsxcomponents/ambient-ui/src/queries/use-send-message.tscomponents/runners/ambient-runner/ambient_runner/bridges/claude/operational_events.py
🚧 Files skipped from review as they are similar to previous changes (4)
- components/ambient-ui/src/queries/use-send-message.ts
- components/runners/ambient-runner/ambient_runner/bridges/claude/operational_events.py
- components/ambient-ui/src/components/chat-messages.tsx
- components/ambient-ui/src/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/session-header.tsx
Covers tryParseToolPayload, tryParseToolResult, extractLastAssistantMessage, enrichMessages, groupChatItems, and buildChatItems integration flow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Global QueryCache/MutationCache error handler detects 401 responses and redirects to /api/auth/sso/login with returnTo preserving the current URL. Prevents users from seeing stale 401 errors after token expiry — they get seamlessly re-authenticated instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merge Queue Status
This pull request spent 1 minute 44 seconds in the queue, including 28 seconds running CI. Required conditions to merge |
Amber Review — PR #1636 (post-merge)
SummarySolid feature delivery — chat tab, persistent sidebar, action bar, and session export/delete all work correctly at the structural level. TypeScript passes, mutations are wired right, phase-gating on Stop/Restart is correct, and the tool-call grouping logic in FindingsCriticalC1 — Raw <p className="text-sm text-destructive">
Failed to load session: {error.message}
</p>Internal error detail (potentially stack frames, API URLs, auth hints) rendered directly in the UI. Fix: C2 — Raw <p className="text-sm text-destructive">
Failed to load messages: {error.message}
</p>Same violation. Fix: MajorM1 — <DropdownMenuItem onClick={handleExport}>
Fix: const [isExporting, setIsExporting] = useState(false)
const handleExport = useCallback(async () => {
setIsExporting(true)
try {
// ... existing logic
} catch {
// show toast or inline error
} finally {
setIsExporting(false)
}
}, [session])M2 — refetchInterval: 3000, // polls regardless of session phaseThis was flagged as unfixed in my PR #1633 review. This PR adds two new call sites:
A Completed session's sidebar will poll Fix (same as flagged for #1633): accept Minorm1 — import { useLiveTail, LiveIndicator } from '@/app/(dashboard)/[projectId]/fleet/[sessionId]/_components/live-tail-indicator'A global layout component ( Score: 3/5 checks passed (C1, C2, M1 need follow-up; M2 carries from #1633; m1 is architectural hygiene)Suggested follow-up PRs:
Amber — code review agent |
Re-Review: PR #1636 — Updated VerdictChecked current main @ All five findings from my original review remain unfixed. Finding Status
C1 —
|
2 participants
Summary
?tab=logs) and sidebar session (?chat=<id>) persisted in URL — survives reloadlast_assistant_messagefrom system events when assistant payload is emptySpec changes
Test plan
npx tsc --noEmit— zero type errors🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores