fix(chat): unfreeze the turn timer and surface background jobs - #1041
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughChangesBackground job transcript
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
4633-4639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister the chat-info host in a dedicated effect.
The registration shares the listener effect, whose deps include
selectedSubagentSnapshots. That memo changes whenever new subagent snapshots arrive, so the pane releases and re-registers the host repeatedly during an active turn and notifies every subscriber twice per cycle. The listener itself must keep those deps; the registration does not depend on them.Move the registration to a mount-only effect. The host advertisement then has one lifetime, and the pane is a host for exactly as long as it is mounted.
♻️ Proposed split
window.addEventListener("ade:chat:open-info", handler); - // Advertise the listener so transcript affordances that open this pane can - // hide themselves on hosts that have no actions pane (PersonalChatsPage). - const releaseHost = registerChatInfoHost(); - return () => { - releaseHost(); - window.removeEventListener("ade:chat:open-info", handler); - }; + return () => { + window.removeEventListener("ade:chat:open-info", handler); + }; }, [selectedSessionId, selectedSubagentSnapshots]); + + // Advertise the listener so transcript affordances that open this pane can + // hide themselves on hosts that have no actions pane (PersonalChatsPage). + // Mount-scoped: the effect above re-runs on snapshot churn, which must not + // momentarily retract the host. + useEffect(() => registerChatInfoHost(), []);🤖 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 `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 4633 - 4639, Separate the registerChatInfoHost lifecycle from the effect that installs the ade:chat:open-info listener in AgentChatPane. Keep the listener effect’s existing selectedSubagentSnapshots dependencies, but move host registration into its own mount-only effect with a cleanup that calls releaseHost, so the advertisement lasts exactly while the pane is mounted.
🤖 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 `@apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts`:
- Around line 1508-1521: Apply the real-subagent provisional job-line removal in
the terminal branch handling backgroundShell, before upsertBackgroundJobLine
adds or updates the terminal entry. Reuse the existing removal logic and
real-subagent guard from the subagent_started/subagent_progress path, so a first
subagent_result event removes any background-chip:<taskId> line opened by
scheduled-work updates before rendering the spawn anchor and result card.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 4633-4639: Separate the registerChatInfoHost lifecycle from the
effect that installs the ade:chat:open-info listener in AgentChatPane. Keep the
listener effect’s existing selectedSubagentSnapshots dependencies, but move host
registration into its own mount-only effect with a cleanup that calls
releaseHost, so the advertisement lasts exactly while the pane is mounted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 36dcdb2d-a386-4962-b713-0cc25794cfe5
⛔ Files ignored due to path filters (6)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (10)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/SubagentActivityCards.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/lib/terminalAttention.test.tsapps/desktop/src/shared/sessionStatusPresentation.tsapps/ios/ADE/Views/Work/WorkTimelineHelpers.swift
|
@codex review Addressed the CodeRabbit finding on Hoisted the guard above the event-type split so it covers every lifecycle path, rather than duplicating the loop into the terminal branch as suggested — one predicate, one place to change. Regression test Also corrected two "never splices rows" invariant comments that the change made wrong, and documented that the two guards are counterparts rather than complements ( |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca08d594ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function useChatInfoHostAvailable(): boolean { | ||
| return useSyncExternalStore( | ||
| subscribeChatInfoHost, | ||
| () => chatInfoHostCount > 0, | ||
| () => false, |
There was a problem hiding this comment.
When Personal Chats is open after the Work tab has mounted, App.tsx keeps the project surface and its AgentChatPane mounted but inactive, so those hidden panes still increment this module-level count. That makes every PersonalChatsPage transcript think a chat-info host is available and render the new background-job open button, but the dispatched event is then ignored for a different sessionId or opens an invisible actions pane, recreating the dead affordance this registry is meant to prevent. This should be scoped to the message list's owning host rather than any window listener.
Useful? React with 👍 / 👎.
|
@codex review Good catch, and it invalidated my own fix rather than the original code — confirmed against Replaced the registry with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed17428a8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <BackgroundJobLine | ||
| event={event} | ||
| sessionEnded={options?.sessionEnded} |
There was a problem hiding this comment.
When this new BackgroundJobLine is rendered from PersonalChatsPage, that host never passes sessionEnded to AgentChatMessageList (it renders the list at PersonalChatsPage.tsx around the selected events, while only deriving turnActive from selectedSession?.status). For an ended personal chat that still has a persisted background_task row stuck in running (the orphan/restart case this component explicitly handles), options?.sessionEnded is therefore false and the line keeps a live interval/ticker instead of freezing and dropping the bogus duration. Please pass selectedSession?.status === "ended" from the personal chat host as well.
Useful? React with 👍 / 👎.
|
@codex review Confirmed and fixed. Now mirrors Note the earlier CodeRabbit thread on |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Three bugs that combined to make a working turn look hung.
The "working for" timer froze at 0s. The ticker captured the timer <span>
once, but the status line swaps from a bare <span> into an expander
<button> when the turn's first tool entry lands, so React remounted the
element and the ticker kept writing into a detached node. It now paints
through a callback ref and re-reads the ref each tick, so the counter
follows whichever node is mounted. This affected every desktop platform
and the web client, not just Windows.
A session whose turn had ended but still had a live background job read
as a bare "Working" with no duration -- indistinguishable from a stalled
turn. It now says "Background work" (or "Background work xN") and shows
its elapsed.
Background shell commands had no in-thread presence while running. The
row was pushed only on the terminal event, and -- more importantly -- it
was driven only by legacy subagent lifecycle events. The live Claude
runtime reports a backgrounded shell ONLY as
scheduled_work_update {kind:"background_task"}, which the transcript
dropped outright, so the row never appeared for a job you actually
started. Both producers now feed one background_job_line on one row key,
pushed at the job's first sighting and mutated in place at exit: a
settled row is never reopened by a late progress tick, a late agentType
cannot reclassify a job mid-flight, and a real subagent reported through
the background stream never gets a line beside its cards. It renders as
a quiet divider one-liner, not a card, with a live elapsed that freezes
on an ended session and an "open" affordance shown only where a host
listens for it.
iOS still drops the background_task stream; that divergence is now
documented as unported work rather than deliberate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The real-subagent guard that removes a background job line opened by the scheduled-work producer lived only on the spawn path, so a task whose only lifecycle event is subagent_result skipped it -- reachable from a truncated or replayed transcript -- and the agent rendered as a job line next to its card pair. Hoisted above the event-type split so it covers every lifecycle path, rather than duplicating the loop into the terminal branch. Also corrects two now-wrong "never splices rows" invariant comments and records that the two guards are counterparts, not complements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The module-level registry was the wrong shape. App renders every ProjectSurface and only toggles `active`, so each AgentChatPane stays MOUNTED while Personal Chats is open. A global "is any host alive" count therefore read true on exactly the surface that has no actions pane: PersonalChatsPage rendered the background-job `open` button, and clicking dispatched into a hidden pane that drops the event on its sessionId guard -- recreating the dead affordance the registry was added to prevent. Replaced with ChatInfoHostContext, provided by AgentChatPane around the transcript it owns. Only the owning subtree can answer this, and the whole registry (plus its useSyncExternalStore subscription) goes away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PersonalChatsPage derived turnActive and nothing else, so it never told the transcript the session had ended. A background job left `running` by a restart therefore kept a live interval in an archived personal chat -- the exact orphan case the freeze was added for -- while the same chat in the Work tab froze correctly. Mirrors AgentChatPane's derivation exactly (`status === "ended"`). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d0354e3 to
d18d375
Compare
|
@codex review Rebased onto Re-verified on the new base: typecheck clean, 1317 tests across 56 files, docs validation green. Both prior findings remain fixed (guard hoisted above the event-type split; |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Three bugs that combined to make a working turn look hung. Reported from a live session showing
working for 0s · taking longer than usual— a contradiction that is only reachable through the first bug.The turn timer froze at
0sWorkingIndicatorticks by mutatingtextContenton a ref'd<span>, deliberately avoiding a per-second React commit. The effect captured that element once. But the status line changes shape mid-turn —hasToolActivity ? <button>{status}</button> : status— so when the turn's first tool call lands, React sees a different element type at that position, unmounts the subtree, and mounts a fresh one.timerRef.currentrepointed to the new span; the effect never re-ran, sotick()kept writing into the detached node.setLongRunningis React state, so it survived the remount — which is exactly why the indicator could claim0sandtaking longer than usualat the same time.Now painted through a callback ref (repaints in the same commit as the remount, so no flash back to
0s) plus a ref re-read each tick.Not Windows-specific — pure reconciliation, identical on macOS/Linux and in the web client. iOS was never affected; its
TimelineView(.periodic)has no captured node to strand.A finished turn with a live background job read as "Working"
sessionStatusPresentation's(ready|idle) && activeBackgroundTaskCount > 0branch returned a bare"Working"withshowsElapsed: false. A row claiming the model is thinking, with no duration to judge it by, is indistinguishable from a stalled turn. NowBackground work/Background work ×Nwith its elapsed showing.A running background job was invisible in the thread
The in-thread row was pushed only on the terminal event — and, more importantly, was driven only by legacy subagent lifecycle events. The live Claude runtime reports a backgrounded shell only as
scheduled_work_update {kind:"background_task"}(emitClaudeBackgroundTaskUpdateemits no subagent events for these), and the transcript dropped that outright. The row therefore never appeared for a job you actually started — it worked only when replaying transcripts from an older build.Both producers now feed one
background_job_lineon one row key, pushed at the job's first sighting and mutated in place at exit. Invariants covered by tests:agentTypecannot reclassify a job mid-flight;It renders as a quiet centered rule-line — the scheduled-wake/spawn-return divider idiom, deliberately not a card, since background jobs are frequent and rarely the point of the turn. Live elapsed that freezes on an ended session (an archived job stranded
runningwould otherwise tick forever), and anopenaffordance that reveals the actions pane, rendered only where a host actually listens for it.Notes
background-chip:<agentKey>is deliberately unchanged — the virtualizer'smeasuredHeightsare keyed by it.⚙/✓/✗, which resolve to Segoe UI Emoji on Windows and render as heavier colour glyphs off the baseline.mainand surface as a broken build mid-release. The divergence is documented inWorkTimelineHelpers.swiftandios-companion.mdas unported work. Net iOS diff is 13 comment-only lines.Verification
tsc --noEmitclean for desktop-owned code.ade-clitypecheck clean against the modified desktop sources.🤖 Generated with Claude Code
Summary by CodeRabbit