[fix] Rebase #5589 onto #5569 and fix the two blocking review findings - #5608
Merged
mmabrouk merged 5 commits intoJul 31, 2026
Merged
Conversation
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.
…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.
…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.
dosubot
Bot
added
the
size:L
|
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:
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 |
Contributor
Railway Preview Environment
Updated at 2026-07-31T19:18:39.505Z |
mmabrouk
merged commit Jul 31, 2026
6febfe2
into
fix/5530-agent-session-multi-tab
39 of 40 checks passed
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.
Rebase and review fixes for #5589, as part of the 0.106.2 release pass. Targets
fix/5530-agent-session-multi-tabso it lands as part of that PR.Arda is away, so this resolves the conflict and takes the two Codex findings that block the feature. The other two findings are filed as issues rather than fixed here, per the reasoning below.
Context
#5589 conflicted with its own base. It was rebased onto
fe-chore/agent-conversation-split, and two more commits then landed on that branch:01f11b7ba5(closes five latent defects in the agent chat slice) and79056cbc4c(drops per-node scroll measurements). One file conflicted,useSessionHydration.ts, in three places.Only
01f11b7ba5was ever involved.79056cbc4ctouches a different file and merged cleanly. Of01f11b7ba5's five fixes, exactly one lands in this file: routing the hydration path's initial result through the same guard as its refetch, with a synchronous high-water mark instead of a ref that lags a React commit.The conflict resolution
Git auto-merged the code sitting between the conflict markers, so the merged file already referenced
adoptedLengthfrom01f11b7ba5. Taking #5589's side wholesale would leave a dangling reference that does not compile, and the obvious way to "fix" that (dropping the condition) would silently revert one of the five defect fixes with no test to catch it.So the resolution keeps #5589's structure and ports the guard into it, using a local
adoptedflag set when the refetch adopts.For the third hunk the two sides differ in mechanism, not goal. It resolves to #5589's watermark, because
adoptServerTranscriptwrites the watermark synchronously before any React commit, so a racing delivery with equal-or-fewer records is rejected regardless of arrival order. That is the property01f11b7ba5was adding, reached a different way. The invariant is now written as a comment on the watermark write, so it does not stay tribal knowledge.The two review fixes
The poll timer reset itself every render.
useScrollIntentreturns a fresh object each render, soadoptServerTranscriptwas rebuilt each render and the polling effect tore down its 15 second timer and re-armed with reset backoff. The hook re-renders on the liveness query's own 15 second beat, so during a remote run the record catch-up raced its own canceller at the same period. When the remote run ends, polling stops for good, so a starved poll leaves the transcript stale until the session is reopened. That is the feature this PR ships.The deps now name stable members (
intent.armJump, auseCallback([]), andintent.stickRef, a ref) rather than the per-render object. The poll effect also reads the adopter through a ref and keys only on[runningElsewhere, sessionId], so the timer's stability no longer depends on auditing every other dependency.The poll was deliberately not rewritten as
atomWithQuery. That section ofweb/AGENTS.mdprescribes it for data fetching; this is a side-effecting adoption loop with growth-reset backoff, and the chained-timeout shape is intentional so a slow fetch cannot stack on itself.A skipped transcript write still saved its watermark. When a session is too large for localStorage the write is skipped, but
persistSessionMessagesAtomfiled the newrecordCountanyway. After a reload the old or missing messages pair with a newer watermark, soshouldAdoptServerTranscriptrejects the complete server log as not newer and the cache stays stale, potentially forever for a session that never runs again. This breaks the invariant the same file documents ondropSessionMessages, that the two stores must never diverge.The quota guard now returns
{evicted, persisted}, and the atom deletes that session's watermark whenpersistedis false. An absent watermark reads as 0, so the next open re-syncs from the durable log.Filed rather than fixed
Two Codex findings are real but were left alone on purpose, because fixing either means a design decision Arda should own. Both are tracked as issues.
The first is that liveness stops polling when its result is empty, so a tab that never regains focus will not notice a run starting elsewhere. The stop is deliberate and Arda's comment cites the request budget. Clicking into the tab heals it and nothing is lost.
The second is that a watermark-less transcript can be overwritten by a lagging server copy with the same message count, during the ingest lag after your own run. It self-heals on the next open. Any real fix adds a completion guard to the shared adoption rule and changes its test contract.
Tests
tsc --noEmiton the oss project: clean.eslinton the three changed files: clean. Worth noting because the poll effect deliberately omits the adopter from its dependency list.@agenta/entitiesunit suite: 931 tests across 65 files pass, includingsession-transcript-adoption.test.ts. The adoption rule itself is untouched.Neither
useSessionHydrationnor the session persistence atoms have direct tests, so these checks prove nothing broke, not that the two fixes work. The quota guard is the testable one if someone wants positive coverage.What to QA