[chore] Split AgentConversation into hooks and views - #5569
Conversation
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesAgent chat refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
dosubot
Bot
added
Frontend
refactoring
AgentConversation.tsx: 2692 -> 679 lines, across 22 modules.
The seam that made this possible: four mutable scroll refs (stick,
armBottom, animateBottom, programmaticScroll) were written from five call
sites and read by two sets of effects. useScrollIntent owns them behind
three verbs — armGlide() on submit, armJump() on restore/adopt, follow()
for a send that leaves now — so producers never touch raw refs and the two
scroll engines can be lifted out whole.
assets/ conversationLayout, runError, messageParts, scrollGeometry
hooks/ useScrollIntent, useTranscriptScroll, useVirtuosoTranscript,
useAgentChatSession, useSessionHydration, useComposerDraft,
useComposerAttachments, useVoiceComposer, useTurnInspector,
useOnboardingChat, useFirstRunSeed
components/ MessageRow, TurnActivity, AgentTurn, AgentTranscript,
TranscriptPlaceholder, AgentComposerDock
AgentTurn is memoized and turn numbers are precomputed once per message
set — previously every settled turn re-rendered on each streamed token and
the turn lookup was an O(n^2) scan.
Split against current content so the voice-input and file-upload features
survive: useVoiceComposer is a new module for the recorder state (it has no
home in the pre-feature structure), and useComposerAttachments owns
uploadsEnabled plus bindDropTarget(isBlocked) — a predicate read at event
time, since the blocked-drop guard depends on the recorder which depends on
addFiles. Verified line-by-line against the upstream diff.
fc14b8f to
c3dc179
Compare
Railway Preview Environment
Updated at 2026-07-31T19:19:11.202Z |
The agent chat's session history, open-tab list, active tab and cached transcripts are all `atomWithStorage` atoms. jotai's default storage subscribes to the window `storage` event, so every write in one browser tab replaced those records live in every other tab of the same origin. The open-tab list drives the antd `Tabs` items, so an incoming replacement could remove a tab and unmount its `AgentConversation`. That orphans the `useChat` stream mid-turn, and since a transcript is only persisted once its stream settles, the in-flight turn was lost. Switching browser tabs made it worse: both session queries revalidate on window focus, and the reconcile that follows writes to all three keys. These four stores now use localStorage with `subscribe` removed, so a tab owns its own view. Cross-tab awareness already has a correct channel: the server reconcile on focus, plus renames pushed to the durable stream header. The localStorage push was a redundant second channel that replaced whole records instead of merging them. Storage stays shared, so a reload still picks up the last write.
…004] A browser that opened a session while another browser was mid-turn cached a partial transcript, and no amount of refreshing ever fixed it. The durable record log had the complete turn the whole time. The adoption guard asked "does the server have more MESSAGES than I do?". `transcriptToMessages` only closes a message on a `done` record and folds a paused turn into its resume, so a turn that grows in place (tool results landing, an approval round-trip completing) keeps the same message count from start to finish. The guard concluded the server was not ahead and kept the partial copy, on every subsequent reload. The record log is append-only and ordered, so record count is an exact test. `loadSessionMessages` now returns the count it built the transcript from, and that watermark is persisted next to the cached messages. The guard adopts when the log grew past the watermark, with a message-count floor so ingest lag can never trade a longer local transcript for a shorter server one. Both guards now share one rule (`shouldAdoptServerTranscript`): the cache-miss hydration path carried the same blind spot and would have stayed broken if only the revalidate path were fixed. The old "local tail paused, server tail complete" special case is deleted rather than extended, since a watermark sees that growth directly. The watermark is cleared when a turn goes live locally (we cannot know what the runner logged for it), so the next open re-syncs from the log. That clearing effect must stay declared above the persist effect; effects fire in declaration order, and that is what stops a locally-extended transcript being filed under a server watermark. Messages and watermarks can only move together: one writer sets both, and a single `dropSessionMessages` helper is now the only deletion path, covering the quota-eviction loop that previously shed transcripts with no way to shed their watermarks. Caches written before this change have no watermark, which reads as 0, so they adopt the server copy once and are correct from then on.
A session being driven from another browser or tab gave no sign anything was happening. It looked identical to an idle one, so the only feedback was the transcript silently not changing. There is no push channel to browsers today: the runner publishes every event to Redis, but the only consumer is the ingest worker that writes them to the DB. True live following would need an SSE endpoint with per-session auth, which is out of scope here. Instead the conversation now polls the durable record log while the backend reports a live run that this browser is not driving, and the adoption guard decides whether anything changed. A strip above the composer says so, reusing the session bar's `running` dot so the two read as one signal. The poll chains its timeouts rather than using setInterval, since the log is large and backend-slow and a slow fetch must not stack requests on itself. It backs off from 15s to 60s while the log is quiet and resets on real growth: `is_running` is Redis-TTL'd at an hour and the runner only clears it at turn end, so a runner that dies mid-turn would otherwise leave a browser refetching the full log every 15s for that hour. Backing off rather than stopping keeps a genuinely long turn followed, since a slow tool call emits no records until it returns. Background adoptions do not jump to the live edge unless the reader is already there, so a catch-up cannot yank someone who scrolled up to read.
…session-multi-tab
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
web/oss/src/components/AgentChatSlice/AgentConversation.tsx (1)
417-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider assigning
handleSubmitReffrom an effect (or memoizinghandleSubmit).Line 427 mutates the ref during render. It happens to be safe here (consumers read it inside effects/events, which run after commit), but a render-phase side effect is fragile if a future consumer reads it during render.
useEffect(() => { handleSubmitRef.current = handleSubmit })makes the contract explicit.web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.ts (3)
202-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
markLiveGatehere instead of writingliveGateInteractionRefdirectly.Line 204 duplicates the only other write path (
markLiveGate, line 194); routing both through one setter keeps the "live gate" invariant in one place.
307-333: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull transcript scan on every commit.
This effect re-walks every message and part on each throttled commit (~20×/s while streaming).
data-committed-revisiononly ever arrives on the streaming tail, so scanning the last message (or messages after a cursor index) keeps the hot path O(1) in transcript length.
344-367: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSwallowed control-plane failures leave no trace.
Both
killSession(line 359) andcommandSessionStream(line 366) discard errors silently, so a failed cancel — where the runner keeps running and billing — is invisible. Aconsole.warnin the catch keeps the UX unchanged while making the failure diagnosable.🛠️ Proposed change
- .catch(() => {}) + .catch((err) => console.warn("[AgentChat] killSession failed:", err)) return } - commandSessionStream({sessionId, projectId}).catch(() => {}) + commandSessionStream({sessionId, projectId}).catch((err) => + console.warn("[AgentChat] cancel command failed:", err), + )web/oss/src/components/AgentChatSlice/components/AgentComposerDock.tsx (3)
259-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive one blocked flag instead of re-deriving it per control.
composerDisabledis documented as "IDE hand-off / no model key", which is exactly whatonboardingActive ? ideHandoffActive : modelBlockedrecomputes at Lines 259 and 303-305. Two sources of truth for the same state will drift; prefer passing (and using) the single flag for input, mic and attach.Also applies to: 303-305, 327-327
250-396: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider memoizing the
prefix/header/trailingJSX slots.These element trees are rebuilt on every dock render and handed to the lazily-loaded Lexical input as props, defeating any memoization inside it.
useMemoper slot (plus a stableonSubmit/onPasteFileviauseCallback) keeps the heavy editor from re-rendering on unrelated state changes such asbusyor queue updates.As per coding guidelines, "Memoize inline arrays containing objects or JSX when passing them as props to avoid unnecessary rerenders."
Source: Coding guidelines
37-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the narrative comment blocks.
Several of these blocks are multi-paragraph design rationale rather than surprising-constraint notes (the layout-shift and lazy-ref ordering notes do qualify; the "Owner call" / strip-rhythm prose does not).
As per coding guidelines, "Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements."
Also applies to: 169-177, 219-224
Source: Coding guidelines
web/oss/src/components/AgentChatSlice/hooks/useComposerDraft.ts (2)
51-62: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUnmount capture also fires on
sessionIdchange.The effect is keyed on
sessionId, so a session switch within a mounted pane runs this cleanup with the previoussessionIdwhile reading the new editor content — persisting text under the wrong key. If panes are strictly one-per-session (as the doc comment implies) this is unreachable today; asessionIdref snapshot would make it order-independent.
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
RefObject<boolean>overReact.MutableRefObject.In React 19's type definitions, ref types are unified under
RefObject, makingMutableRefObjecta deprecated shorthand. UsingRefObjectalso matchesrichInputRefand removes an unimportedReact.namespace reference.♻️ Proposed change
@@ - revealPlayedRef: React.MutableRefObject<boolean> + revealPlayedRef: RefObject<boolean>web/oss/src/components/AgentChatSlice/hooks/useVoiceComposer.ts (2)
29-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc block is attached to the wrong declaration.
The send-vs-attach rationale sits above
voiceEnabled(Line 40), whose own one-liner then follows it; move it ontovoiceWillSend/startVoiceMessagewhere the decision actually lives.
46-73: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueWrap
startVoiceMessage/dismissMicErrorinuseCallback.Both are handed down as props (
onStartAudio,onDismiss) fromAgentComposerDock; new identities each render block any memoization of the voice controls.As per coding guidelines, "Minimize React re-renders with
useMemo,useCallback, andReact.memowhere appropriate; avoid unstable inline functions and objects".Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 763bfad5-e5ff-4f7d-99b0-4081a1fe4d77
📒 Files selected for processing (22)
web/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/assets/conversationLayout.tsweb/oss/src/components/AgentChatSlice/assets/messageParts.tsweb/oss/src/components/AgentChatSlice/assets/runError.tsweb/oss/src/components/AgentChatSlice/assets/scrollGeometry.tsweb/oss/src/components/AgentChatSlice/components/AgentComposerDock.tsxweb/oss/src/components/AgentChatSlice/components/AgentTranscript.tsxweb/oss/src/components/AgentChatSlice/components/AgentTurn.tsxweb/oss/src/components/AgentChatSlice/components/MessageRow.tsxweb/oss/src/components/AgentChatSlice/components/TranscriptPlaceholder.tsxweb/oss/src/components/AgentChatSlice/components/TurnActivity.tsxweb/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.tsweb/oss/src/components/AgentChatSlice/hooks/useComposerAttachments.tsweb/oss/src/components/AgentChatSlice/hooks/useComposerDraft.tsweb/oss/src/components/AgentChatSlice/hooks/useFirstRunSeed.tsweb/oss/src/components/AgentChatSlice/hooks/useOnboardingChat.tsweb/oss/src/components/AgentChatSlice/hooks/useScrollIntent.tsweb/oss/src/components/AgentChatSlice/hooks/useSessionHydration.tsweb/oss/src/components/AgentChatSlice/hooks/useTranscriptScroll.tsweb/oss/src/components/AgentChatSlice/hooks/useTurnInspector.tsweb/oss/src/components/AgentChatSlice/hooks/useVirtuosoTranscript.tsxweb/oss/src/components/AgentChatSlice/hooks/useVoiceComposer.ts
| useEffect(() => { | ||
| if (!error) return | ||
| const parsed = parseAgentRunError(error) | ||
| setMessages((prev) => { | ||
| const last = prev.length > 0 ? prev[prev.length - 1] : undefined | ||
| const existing = (last?.metadata as {runError?: {message?: string}} | undefined) | ||
| ?.runError | ||
| if (last?.role === "assistant") { | ||
| if (existing?.message === parsed.message) return prev // already stamped | ||
| const next = [...prev] | ||
| next[next.length - 1] = { | ||
| ...last, | ||
| metadata: {...(last.metadata as object | undefined), runError: parsed}, | ||
| } | ||
| return next | ||
| } | ||
| // No trailing assistant turn (failed before one existed) — add a minimal carrier. | ||
| return [ | ||
| ...prev, | ||
| { | ||
| id: `run-error-${generateId()}`, | ||
| role: "assistant", | ||
| parts: [], | ||
| metadata: {runError: parsed}, | ||
| } as (typeof prev)[number], | ||
| ] | ||
| }) | ||
| }, [error, setMessages]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@ai-sdk/react useChat error reset on new sendMessage
💡 Result:
In the AI SDK (specifically starting from newer versions), the error state returned by the useChat hook does not automatically reset when you call sendMessage [1][2][3]. If an error occurs, it persists in the state, often requiring explicit handling to clear it so that subsequent interactions can proceed smoothly [1][2][4]. To manage this, you have a few options: 1. Use clearError: Most recent versions of the @ai-sdk/react package provide a clearError function directly from the useChat hook [3][5][6]. You can call this method before or during your submission logic to reset the error state [1][3]. const { sendMessage, error, clearError } = useChat; const handleSubmit = (e) => { e.preventDefault; if (error) clearError; // Clear the error before sending a new message sendMessage({ text: input }); }; 2. Manual State Reset: If you are on an older version that lacks clearError, or if you need custom behavior, you can use the setMessages function to manipulate the message array. A common pattern is to remove the last (failed) message if the assistant stream was interrupted before sending a new request [2][4]. 3. Error Handling Callback: You can also use the onError callback provided in the useChat options to handle side effects when an error occurs, though this does not automatically clear the state for the UI [2][4][7]. If you find that your UI remains stuck or disabled due to the error, ensure that you are checking the error state in your conditional rendering (e.g., to enable/disable input fields or show/hide retry buttons) and explicitly invoking a reset mechanism [2][4].
Citations:
- 1: useChat: add a clearError handler vercel/ai#4742
- 2: https://ai-sdk.dev/docs/ai-sdk-ui/error-handling
- 3: https://ai-sdk.dev/docs/reference/ai-sdk-ui/use-chat
- 4: https://ai-sdk.dev/v5/docs/ai-sdk-ui/error-handling
- 5: https://ai-sdk.dev/v5/docs/reference/ai-sdk-ui/use-chat
- 6: https://ai-play.vercel.app/docs/reference/ai-sdk-ui/use-chat
- 7: https://ai-play.vercel.app/docs/ai-sdk-ui/error-handling
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file and package info =="
git ls-files | rg '(^|/)useAgentChatSession\.ts$|(^|/)package\.json$|pnpm-lock\.yaml$|package-lock\.json$|yarn\.lock$' || true
echo
echo "== useAgentChatSession outline/sections =="
if [ -f web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.ts ]; then
wc -l web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.ts
sed -n '1,340p' web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.ts | cat -n
fi
echo
echo "== dependency declarations for `@ai-sdk/react` ai package =="
rg -n '"(`@ai-sdk/react`|ai)"|(`@ai-sdk/react`|ai)\@' package.json web/package.json pnpm-lock.yaml web/pnpm-lock.yaml . 2>/dev/null | head -120Repository: Agenta-AI/agenta
Length of output: 36704
🌐 Web query:
vercel ai useChat clearError reset error new submission @ai-sdk/react 3
💡 Result:
In the Vercel AI SDK (specifically @ai-sdk/react), you can manage error states in the useChat hook using the built-in clearError function [1][2]. The clearError function is available on the object returned by useChat and allows you to programmatically clear any existing error state without needing to trigger a reload or perform a new submission [1][3]. Usage: const { error, clearError } = useChat; // To clear the error: clearError; This is particularly useful when you want to reset the UI or allow the user to try again after a failed attempt without leaving the error message permanently displayed [4]. If you are using an older version of the SDK where clearError is not available, common workarounds include using the regenerate function to attempt a retry, or re-initializing the chat state if necessary [5][6]. For full control, you can also process errors via the onError callback option provided when initializing useChat [5][6].
Citations:
- 1: https://ai-sdk.dev/docs/reference/ai-sdk-ui/use-chat
- 2: https://ai-sdk.dev/v5/docs/reference/ai-sdk-ui/use-chat
- 3: https://ai-play.vercel.app/docs/reference/ai-sdk-ui/use-chat
- 4: useChat: add a clearError handler vercel/ai#4742
- 5: https://ai-sdk.dev/v5/docs/ai-sdk-ui/error-handling
- 6: https://ai-play.vercel.app/docs/ai-sdk-ui/error-handling
Clear the previous error before submitting again, or track the stamped error separately.
useChat keeps error until it is cleared explicitly, and the tail-based dedupe only prevents overwriting the same message. If the last turn changes to user before error and messages update, this effect can append another run-error-* assistant carrier; call clearError(error) before sendMessage, or store the parsed error identity in a ref and avoid appending while it remains stamped.
| autoStartedSeedRef.current = true | ||
| handleSubmitRef.current(firstRunPrompt) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unhandled rejection from the seeded auto-send.
handleSubmitRef.current is typed void | Promise<void> and the concrete handleSubmit in AgentConversation.tsx is async (it awaits filesToParts). A rejection here escapes as an unhandled promise rejection — the same case the codebase already handles via ignoreStreamRejection.
🛡️ Proposed fix
- handleSubmitRef.current(firstRunPrompt)
+ void Promise.resolve(handleSubmitRef.current(firstRunPrompt)).catch((err) =>
+ console.warn("[AgentChat] first-run seed send failed:", err),
+ )📝 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.
| autoStartedSeedRef.current = true | |
| handleSubmitRef.current(firstRunPrompt) | |
| autoStartedSeedRef.current = true | |
| void Promise.resolve(handleSubmitRef.current(firstRunPrompt)).catch((err) => | |
| console.warn("[AgentChat] first-run seed send failed:", err), | |
| ) |
Review pass on the AgentConversation split. All five predate the split; the move only made them legible. - Attachment `uid` was derived from name+mtime+size, so the same file attached twice produced two tray rows with one identity — and `uid` is the React key, the preview-URL key, and the remove/view/retry handle. Mint it once and enqueue the minted value instead of re-deriving the string. - `filesToParts` used `Promise.all`, so one unreadable file rejected the whole batch. Every one of `handleSubmit`'s four call sites drops its promise, so that surfaced as an unhandled rejection and a send that silently did nothing. Report failures as data (`allSettled`), hold the send, and name the file through the inline rejection channel the other refusals already use. - The IDE-bubble typewriter kept one timer handle, so a second run (Start over mid-animation) stranded the first chain past every cleanup. One cancel, called on start, on Start over, and on unmount. - Session hydration adopted the initial result unguarded while its refresh sibling was guarded. Route both through one adopter that tracks what it has already adopted, so the guard no longer depends on React commit timing. - SC-4 follow-release bound listeners to whatever scroll node existed at mount. Toggling virtualization off remounts that container, leaving the handlers on a detached node for the rest of the session. Track the node as state and key the node-bound effects on it; `attachScroll` is now the only way to attach it.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/oss/src/components/AgentChatSlice/components/AgentTranscript.tsx (2)
40-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRoute the jump button through the mounted transcript engine.
useVirtuosoderives only fromvirt.enabled, but the Virtuoso branch renders only whenmessages.length > 0. When the native empty-state fallback mounts,virt.jumpToLateststill has no mounted Virtuoso scroller to scroll, and the native handler is skipped becauseuseVirtuosois true. Derive the handler and any engine-specific logic from the same mounted-engine condition, or clearshowJumpbefore the fallback mounts.
40-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd live-region semantics to the virtualized transcript.
The
<Virtuoso>branch replaces the native container that hasrole="log",aria-live="polite", andaria-label.virt.components.Scrolleris a no-op placeholder and does not add those attributes. Implement the customScrollerwith the equivalentrole="log",aria-live="polite", andaria-label="Agent conversation"so the virtualized transcript exposes the same live-region behavior.web/oss/src/components/AgentChatSlice/hooks/useTranscriptScroll.ts (1)
95-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCancel interrupted pins instead of settling them as successful.
animatePinToregistersscrollendand a 700 ms fallback listener, then callsonSettlefrom them. A laterscrollendfrom user input can run after the pin was overridden by the user, which setsstickRef.current = trueand jumps back to the bottom. The fixed timer can also settle a long native glide before it reaches the target. Cancel on all user-scroll inputs and settle only after confirming the target position; keep the timer as a true fallback whenscrollendis unavailable.Also applies to lines 182-195 and 261-285.
🧹 Nitpick comments (1)
web/oss/src/components/AgentChatSlice/components/AgentTranscript.tsx (1)
44-99: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStabilize Virtuoso props during streaming.
itemContentat Line 73 and theincreaseViewportBy,style, andcontextobjects at Lines 49-97 are recreated wheneverAgentTranscriptrenders.AgentConversationsupplies a livemessagesstream, so this path renders frequently.Memoize callbacks and stable objects with their true dependencies. Isolate the changing active footer so it does not invalidate unrelated Virtuoso props. Verify that active footer updates remain visible.
As per coding guidelines: “Minimize React re-renders with
useMemo,useCallback, andReact.memowhere appropriate; avoid unstable inline functions and objects, especially in lists.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6593aa16-1176-41a0-8581-9c646adad3f7
📒 Files selected for processing (7)
web/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/oss/src/components/AgentChatSlice/assets/files.tsweb/oss/src/components/AgentChatSlice/components/AgentTranscript.tsxweb/oss/src/components/AgentChatSlice/hooks/useComposerAttachments.tsweb/oss/src/components/AgentChatSlice/hooks/useOnboardingChat.tsweb/oss/src/components/AgentChatSlice/hooks/useSessionHydration.tsweb/oss/src/components/AgentChatSlice/hooks/useTranscriptScroll.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/oss/src/components/AgentChatSlice/hooks/useSessionHydration.ts
- web/oss/src/components/AgentChatSlice/hooks/useComposerAttachments.ts
- web/oss/src/components/AgentChatSlice/hooks/useOnboardingChat.ts
…remounts The scroll container can be replaced within one session (virtualization toggled, or an empty conversation rendering the plain node under Virtuoso). Three pieces of state are measured against whichever node was live and none of them were reset on the swap: - the scroll baseline, so the first scroll-down-to-edge on the new node compared against the old node's scrollTop and could fail `scrollTop > prevTop` — follow then stayed off at the bottom until the next scroll, - the SC-3 anchor, whose offset was taken in the old node's coordinate space, - an in-flight glide, still holding listeners on the node that went away. Clear all three where the node changes. The anchor is dropped rather than recaptured: its offset has no meaning in the new container, and the next scroll or pointer-down re-anchors.
|
@coderabbitai review |
✅ Action performedReview finished.
|
| // old node's coordinate space), and any glide still animating the node that just went away. | ||
| // Runs before the SC-1/SC-2 pin below, which is declared later. | ||
| useLayoutEffect(() => { | ||
| pinCleanupRef.current?.() |
There was a problem hiding this comment.
🤖 The AI agent says:
[P2] Release the scroll guard when cancelling a node pin
When the plain scroll node is replaced while animatePinTo is active, such as when virtualization is toggled during a submit or jump glide, this cleanup removes the pin listeners and timer but leaves programmaticScrollRef.current set to true. If the plain engine mounts again, its scroll handler, resize observer, and follow effect continue treating every scroll as programmatic, disabling normal autoscroll and anchoring until another pin happens to settle.
Clear the guard or re-arm the pin against the replacement node when cancelling for a node swap.
…session-multi-tab Resolves the useSessionHydration.ts conflict between the watermark-based adoption rework (#5589) and the latent-defect review pass (01f11b7 on #5569) so both intents survive: - Keep #5589's single `adoptServerTranscript` adopter (record-count watermark via `shouldAdoptServerTranscript`) for the hydration path, the SWR revalidation path, and the remote-run poll. - Port 01f11b7's hydration fix into that structure: a local `adopted` flag so a stale empty first result cannot set the "history unavailable" notice after the background refetch already adopted real history. - The rest of 01f11b7's commit-timing guard (local high-water mark / adopted snapshot) is subsumed by the watermark ref, which the adopter updates synchronously before any React commit; documented that invariant on the write so it is not lost. - The count-based guard and paused-tail special case from the old SWR effect are intentionally gone — #5589 replaced them with the watermark ("the special case is gone rather than extended").
`useScrollIntent` returns a fresh object every render, and `intent` sat in `adoptServerTranscript`'s dependency list — so the callback was recreated on every render, and the remote-run poll effect (keyed on it) tore down and re-armed its timer each time. The liveness query alone re-renders the hook ~every 15s while a run is live elsewhere, i.e. on the same cadence as the poll it was cancelling: the record-log catch-up could be starved for the whole run and its backoff never accumulated. Two layers, so the timer's stability is true by construction, not by audit: depend on `intent.armJump` / `intent.stickRef` (both stable) instead of the per-render `intent` object, and have the poll effect read the current adopter through a ref, keyed only on `runningElsewhere` + `sessionId`.
… skipped When even the active session alone overflows the localStorage quota, `writeMessagesWithQuotaGuard` skips the transcript write — but `persistSessionMessagesAtom` still wrote the NEW `recordCount`. After a reload that pairs the OLD (or missing) cached messages with the newer watermark, so `shouldAdoptServerTranscript` rejects the complete server log as "not newer" on every open and the stale cache is frozen until the log happens to outgrow the bogus number. Surface `persisted` from the quota guard and drop the session's watermark when the write was skipped, enforcing the invariant already documented on `dropSessionMessages`: the transcript store and the watermark store must never diverge.
[fix] Release the scroll guard when a transcript node swap cancels a pin
[fix] Stop agent sessions breaking when open in two places [AGE-4004]
2 participants
Context
AgentConversation.tsxhad grown to 2692 lines. One component body held the chat stream, two scroll engines, history hydration, onboarding, the composer, attachments, voice input, and the whole transcript render. Touching any one of those meant reading all of them, and the file was still growing (two feature branches landed in it this cycle).Changes
The blocker was never the length. It was four mutable scroll refs (
stick,armBottom,animateBottom,programmaticScroll) written from five call sites and read by two separate sets of effects. Nothing could be lifted out while those refs were shared ambient state.useScrollIntentnow owns them behind three verbs, one per call-site shape:Producers state intent. They never touch a ref. With that seam the rest moves out whole:
AgentConversation.tsxis 679 lines and reads as a composition root.Two changes are not pure moves, both deliberate:
AgentTurnis memoized. PreviouslyrenderMessagebuilt every row inline, so each streamed token re-rendered every settled turn above the active one. Turn numbers are also precomputed once per message set. The oldturnOfAssistantrescanned the message list per rendered row, which made the transcript O(n^2) on every commit.useComposerAttachmentsreturnsbindDropTarget(isBlocked)rather than a static handler object. The blocked-drop guard depends on the recorder, which depends onaddFiles, which would be circular. The predicate is read at event time, so no ref crosses the boundary.Tests
pnpm --filter @agenta/oss exec tsc --noEmitpasses with zero errors, as do@agenta/entity-uiand@agenta/ee.pnpm lint-fixacross the repo makes no changes. Prettier clean.useStatelazy initializers moved verbatim, comments included. Several of those comments document real races and StrictMode hazards, so they were not rewritten.release/v0.106.1rather than rebased, because the voice input and file upload features landed inside the regions this refactor deletes. Verified line by line: of 357 lines those features added, every behavioural one is present.NEXT_PUBLIC_AGENT_VOICE_INPUTandNEXT_PUBLIC_AGENT_FILE_UPLOADSboth still reach live code.Not runtime verified. The gates above are static.
What to QA
NEXT_PUBLIC_AGENT_VOICE_INPUTon. Empty composer sends it. Composer with text attaches it instead.