Repository navigation
Conversation
…e runs #5256 added a send-admission gate in `useChat.send` keyed on `!targetThreadId` and the *viewed* thread id, intending to block a duplicate send into a thread that already has a run in flight. It over-blocked: any send with no explicit target (starting a new chat) or one addressed to a different thread was silently dropped (`return null`) whenever any run was active anywhere — the "cannot start a new chat while an existing chat is in progress" / "doesn't accept from front end" regression. Re-key the gate on the actual destination thread (`targetThreadId || threadId`, the same value the send already resolves): block only when *that* thread has a run in flight. New chats and parallel sends to other threads now go through; a duplicate send into the busy thread stays blocked. Add functional regression tests that drive `useChat.send` directly (no DOM): a new chat and a parallel second-thread send both reach `sendMessage` while a run on another thread is active, and a duplicate into the busy thread is still rejected. The first two fail against the #5256 code and pass with this fix; the third passes either way (guards against over-correcting). Rebuilt the committed `static/dist/app.js` esbuild bundle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesParallel-thread send admission
Sequence Diagram(s)sequenceDiagram
participant "useChat.send" as UseChatSend
participant "createThreadRequest" as CreateThreadRequest
participant "sendMessage" as SendMessage
participant "submitBusyRef" as SubmitBusyRef
participant "onRunSettled" as OnRunSettled
UseChatSend->>UseChatSend: resolve sendTargetThreadId = targetThreadId || threadId
UseChatSend->>CreateThreadRequest: create destination thread when threadId is missing
UseChatSend->>SendMessage: POST the message for the resolved destination
SendMessage-->>UseChatSend: settle
UseChatSend->>SubmitBusyRef: clear in finally
OnRunSettled-->>UseChatSend: later SSE settlement updates run state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
github-actions
Bot
added
size: XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the send gate logic in useChat.js to block duplicate sends only when the destination thread already has an active run in flight. This resolves an issue where parallel threads and starting a new chat were incorrectly blocked. Additionally, comprehensive unit tests have been added in useChat-send.test.mjs to verify these parallel-thread send scenarios and prevent regressions. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Add a complement to the parallel-send test: viewing thread-a while the active run is on thread-b, a send addressed to thread-b is blocked. Together with the parallel-send case (viewed busy, different target -> allowed) this pins the admission block on the destination thread alone, not the currently-viewed one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BenKurrek
enabled auto-merge
June 26, 2026 14:31
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js`:
- Around line 403-417: The send flow in useChat.js is still blocked by the
global isProcessingRef.current early return even after rekeying activeRun.
Update the send guard around sendMessage so it only blocks when the in-flight
run belongs to the same destination thread, using sendTargetThreadId and
activeRunRef/current thread matching instead of the viewed-thread-wide
processing flag. Keep the existing activeRunBlocksSend logic and remove or
narrow the isProcessing check so cross-thread sends and new chats can proceed
while another thread is running.
In
`@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs`:
- Around line 2584-2671: The new parallel-send fixture in
createParallelSendContext only seeds activeRun, but the real busy-thread state
also has isProcessing set, so the test does not reproduce the early-return path
in send. Update the fixture and the affected parallel-send tests to model a true
in-flight thread by setting both activeRun and isProcessing together, using the
existing React state slot setup in createParallelSendContext so the send logic
exercises the same branch as production.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e96a96d6-312e-4c24-b25a-c04890cf6694
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
…hread Address review (CodeRabbit) on #5336: rekeying only `activeRun` left the `isProcessingRef.current` early-return blocking any send while the *viewed* thread was busy. A running thread carries both flags, so a parallel send to another thread — or a new chat from a busy view — still returned null before sendMessage. Narrow the processing guard to the destination thread too (`isProcessingRef.current && Boolean(sendTargetThreadId) && sendTargetThreadId === threadId`); the added Boolean guard keeps a new chat (both ids null) from being blocked, which the literal `=== threadId` suggestion would have. Tests: seed `isProcessing` alongside `activeRun` for the viewed-running-thread fixtures so they reproduce the real busy state. Verified the parallel-send test fails against the wide isProcessing gate and passes once it is narrowed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Thanks @coderabbitai — both valid, fixed in 894d528.
|
|
Tip For best results, initiate chat on the files or code changes.
(•ᴗ•)ノ🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
think-in-universe
previously approved these changes
Jun 26, 2026
BenKurrek
disabled auto-merge
June 26, 2026 14:53
…ment The real regression behind "can't start a new chat while one is in progress". #5256 added a `submitBusyRef` re-entrancy guard that is set on send but cleared ONLY in `onRunSettled` — delivered over the *open* thread's SSE. When the user starts a run and then opens a new chat before it settles, that thread's SSE is torn down, its settle event never reaches the hook, the guard stays `true`, and every later send is silently dropped (`send` returns null — "doesn't accept from front end"). It presented as flaky because it only bites when the first run is still in flight at navigation time. Release the guard in the send `finally` instead — its job is to serialize the in-flight POST, nothing more. Blocking a resubmit into a still-running thread is already handled by the per-destination `activeRunBlocksSend` guard, so the same-thread protection (covered by "accepted run blocks another submit until settlement") is preserved. This is distinct from the earlier activeRun/isProcessing gate fix on this branch; both over-block mechanisms came from #5256. Add a regression test driving the actual sequence (send a run that never settles, then send to a different thread) — verified it fails without the finally reset and passes with it. Browser-level repro (Chrome via CDP) confirms the new-chat-while-running send now succeeds 4/4 on the previously-flaky timing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#5256) Add a Playwright scenario to the Reborn v2 smoke suite that reproduces the `submitBusyRef` deadlock in a real browser: send a slow-response turn in thread A, then use the in-app "+ New" button (client-side navigation, NOT a reload, so the hook instance and the leaked ref survive) and send in the new chat while thread A is still running. Asserts the new-chat message actually posts and renders. This is the lifecycle the unit tests structurally cannot reach (navigation + SSE teardown), and it's the test that would have caught the original bug from day one. Verified red/green against the live binary: PASS with the finally guard-release, FAIL (new-chat bubble never renders) without it. Supporting changes: - Add a stable `data-testid="new-chat"` to the sidebar "+ New" button and a `SEL_V2["new_chat"]` selector (matching the existing data-testid convention). - Refresh the stale `tests/e2e/CLAUDE.md`: it implied the harness was legacy-gateway-only and listed 11 of ~65 scenarios. Document that the suite also drives the Reborn `ironclaw-reborn serve` v2 SPA, add the `ironclaw_reborn_binary` / `reborn_v2_server` / `reborn_v2_browser` / `reborn_v2_page` fixtures, and group the scenario table by surface. - Rebuilt the committed `dist/app.js` for the new data-testid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BenKurrek
enabled auto-merge
June 26, 2026 16:27
think-in-universe
approved these changes
Jun 26, 2026
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 (2)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js (1)
403-426: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDestination-thread gating still loses busy state for offscreen targets.
Lines 413-426 key the block to
sendTargetThreadId, but the only busy sources areactiveRunRefandisProcessingRef. Those refs are current-thread scoped here: Lines 221-255 reset them on thread switches, and Lines 489-515 only repopulate them whenshouldRenderInCurrentThreadis true. So after a send toopts.threadId !== threadId, Line 614 releasessubmitBusyRef, but nothing records that destination as busy. A second explicit send to the same offscreen thread can then slip through and create a duplicate run, which misses the PR contract of blocking duplicates for the busy destination. This needs per-destination busy tracking rather than reusing the viewed-thread refs.🤖 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 `@crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js` around lines 403 - 426, The send gating in useChat.js is still relying on viewed-thread-scoped refs, so offscreen destination threads lose their busy state and can accept duplicate sends. Update the send path around sendTargetThreadId, activeRunRef, and isProcessingRef so busy status is tracked per destination thread rather than only for the currently rendered thread. Ensure the logic that resets on thread switches and the code that repopulates state after send both preserve busy tracking for opts.threadId targets even when shouldRenderInCurrentThread is false, so repeated sends to the same offscreen thread remain blocked.crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs (1)
2681-2866: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe busy-destination regression test still seeds an unreachable hook state.
Lines 2845-2866 preload
activeRun.threadId === "thread-b"while the hook is mounted onthread-a. ButuseChatclears transient run state on thread switches incrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jsLines 221-255, and it only writesactiveRunfor sends that render in the current thread at Lines 508-515. So this fixture can stay green while the real bug remains: send to offscreenthread-b, let the POST settle, then send tothread-bagain without navigating. That reachable two-send flow is the one that needs coverage here.🤖 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 `@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs` around lines 2681 - 2866, The busy-destination test is seeding an unreachable state by mounting useChat on thread-a with activeRun pointing at thread-b, which useChat clears on thread switches and never maintains for offscreen sends. Update the fixture and assertions in the parallel-send tests to exercise the reachable flow: send to thread-b, let that request settle, then immediately send to thread-b again without changing the viewed thread. Use the existing useChat, send, and createParallelSendContext helpers to make sure the second send is blocked only when the destination thread is actually busy.
🤖 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 `@tests/e2e/CLAUDE.md`:
- Around line 108-112: The fixture-location docs are inconsistent: the table
entry for reborn_v2_server correctly points to test_reborn_webui_v2_smoke.py,
but the earlier note still claims all fixtures live in tests/e2e/conftest.py.
Update the docs in CLAUDE.md to narrow that claim or explicitly list the v2
fixture exceptions, so the fixture names reborn_v2_server and reborn_v2_browser
are documented in the right place.
---
Outside diff comments:
In `@crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js`:
- Around line 403-426: The send gating in useChat.js is still relying on
viewed-thread-scoped refs, so offscreen destination threads lose their busy
state and can accept duplicate sends. Update the send path around
sendTargetThreadId, activeRunRef, and isProcessingRef so busy status is tracked
per destination thread rather than only for the currently rendered thread.
Ensure the logic that resets on thread switches and the code that repopulates
state after send both preserve busy tracking for opts.threadId targets even when
shouldRenderInCurrentThread is false, so repeated sends to the same offscreen
thread remain blocked.
In
`@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs`:
- Around line 2681-2866: The busy-destination test is seeding an unreachable
state by mounting useChat on thread-a with activeRun pointing at thread-b, which
useChat clears on thread switches and never maintains for offscreen sends.
Update the fixture and assertions in the parallel-send tests to exercise the
reachable flow: send to thread-b, let that request settle, then immediately send
to thread-b again without changing the viewed thread. Use the existing useChat,
send, and createParallelSendContext helpers to make sure the second send is
blocked only when the destination thread is actually busy.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 435feb7e-a4ed-4b11-a73f-8ac3720096e9
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (6)
crates/ironclaw_webui_v2_static/static/js/components/sidebar-nav.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjstests/e2e/CLAUDE.mdtests/e2e/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
Collaborator
Author
|
Superseded by #5352 — re-opened from a branch in |
auto-merge was automatically disabled
June 26, 2026 16:32
Pull request was closed
personal-upstream-sync Bot
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 26, 2026
…e runs (nearai#5352) * fix(reborn): unblock parallel-thread sends and new chats during active runs nearai#5256 added a send-admission gate in `useChat.send` keyed on `!targetThreadId` and the *viewed* thread id, intending to block a duplicate send into a thread that already has a run in flight. It over-blocked: any send with no explicit target (starting a new chat) or one addressed to a different thread was silently dropped (`return null`) whenever any run was active anywhere — the "cannot start a new chat while an existing chat is in progress" / "doesn't accept from front end" regression. Re-key the gate on the actual destination thread (`targetThreadId || threadId`, the same value the send already resolves): block only when *that* thread has a run in flight. New chats and parallel sends to other threads now go through; a duplicate send into the busy thread stays blocked. Add functional regression tests that drive `useChat.send` directly (no DOM): a new chat and a parallel second-thread send both reach `sendMessage` while a run on another thread is active, and a duplicate into the busy thread is still rejected. The first two fail against the nearai#5256 code and pass with this fix; the third passes either way (guards against over-correcting). Rebuilt the committed `static/dist/app.js` esbuild bundle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): pin send block on destination thread identity Add a complement to the parallel-send test: viewing thread-a while the active run is on thread-b, a send addressed to thread-b is blocked. Together with the parallel-send case (viewed busy, different target -> allowed) this pins the admission block on the destination thread alone, not the currently-viewed one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): also key the isProcessing send gate on the destination thread Address review (CodeRabbit) on nearai#5336: rekeying only `activeRun` left the `isProcessingRef.current` early-return blocking any send while the *viewed* thread was busy. A running thread carries both flags, so a parallel send to another thread — or a new chat from a busy view — still returned null before sendMessage. Narrow the processing guard to the destination thread too (`isProcessingRef.current && Boolean(sendTargetThreadId) && sendTargetThreadId === threadId`); the added Boolean guard keeps a new chat (both ids null) from being blocked, which the literal `=== threadId` suggestion would have. Tests: seed `isProcessing` alongside `activeRun` for the viewed-running-thread fixtures so they reproduce the real busy state. Verified the parallel-send test fails against the wide isProcessing gate and passes once it is narrowed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): release submitBusyRef on send completion, not run settlement The real regression behind "can't start a new chat while one is in progress". nearai#5256 added a `submitBusyRef` re-entrancy guard that is set on send but cleared ONLY in `onRunSettled` — delivered over the *open* thread's SSE. When the user starts a run and then opens a new chat before it settles, that thread's SSE is torn down, its settle event never reaches the hook, the guard stays `true`, and every later send is silently dropped (`send` returns null — "doesn't accept from front end"). It presented as flaky because it only bites when the first run is still in flight at navigation time. Release the guard in the send `finally` instead — its job is to serialize the in-flight POST, nothing more. Blocking a resubmit into a still-running thread is already handled by the per-destination `activeRunBlocksSend` guard, so the same-thread protection (covered by "accepted run blocks another submit until settlement") is preserved. This is distinct from the earlier activeRun/isProcessing gate fix on this branch; both over-block mechanisms came from nearai#5256. Add a regression test driving the actual sequence (send a run that never settles, then send to a different thread) — verified it fails without the finally reset and passes with it. Browser-level repro (Chrome via CDP) confirms the new-chat-while-running send now succeeds 4/4 on the previously-flaky timing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(e2e): browser regression for the new-chat-while-running deadlock (nearai#5256) Add a Playwright scenario to the Reborn v2 smoke suite that reproduces the `submitBusyRef` deadlock in a real browser: send a slow-response turn in thread A, then use the in-app "+ New" button (client-side navigation, NOT a reload, so the hook instance and the leaked ref survive) and send in the new chat while thread A is still running. Asserts the new-chat message actually posts and renders. This is the lifecycle the unit tests structurally cannot reach (navigation + SSE teardown), and it's the test that would have caught the original bug from day one. Verified red/green against the live binary: PASS with the finally guard-release, FAIL (new-chat bubble never renders) without it. Supporting changes: - Add a stable `data-testid="new-chat"` to the sidebar "+ New" button and a `SEL_V2["new_chat"]` selector (matching the existing data-testid convention). - Refresh the stale `tests/e2e/CLAUDE.md`: it implied the harness was legacy-gateway-only and listed 11 of ~65 scenarios. Document that the suite also drives the Reborn `ironclaw-reborn serve` v2 SPA, add the `ironclaw_reborn_binary` / `reborn_v2_server` / `reborn_v2_browser` / `reborn_v2_page` fixtures, and group the scenario table by surface. - Rebuilt the committed `dist/app.js` for the new data-testid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): make finally the sole owner of submitBusyRef release Address review on nearai#5352: drop the now-redundant submitBusyRef release in onRunSettled. The send() finally clears it on every POST completion, so the run-settle path clearing it too is the wrong layer for a POST re-entrancy guard. Single, correctly-scoped release point; the 'accepted run blocks another submit until settlement' test still passes (it relies on finally + activeRun, not the onRunSettled clear). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * build(reborn): rebuild dist/app.js against merged-main source The branch's committed bundle was built before the latest main merge, so it missed main's webui JS changes. Rebuild so dist matches source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Regression
Users reported (Slack): "we cannot run parallel threads all of a sudden. it just doesn't accept from front end — cannot start a new chat while an existing chat is in progress."
Bisected to #5256 (
feat(reborn): expose user-scoped tool settings, commit 9ce47c4), which bundled severalblock … sends during active runcommits. Those added a new send-admission gate inuseChat.send:The intent was to block a duplicate send into a thread that already has a run in flight. But the predicate is keyed wrong:
!targetThreadIdblocks any send with no explicit target.chat.js#handleSendpassesthreadId: activeThreadId, which isundefinedon the new-chat/landing screen — so starting a new chat is dropped whenever any run is active.activeRunForSend.threadId === threadIdblocks a send addressed to a different thread just because the currently viewed thread is running — i.e. it kills parallel threads.Fix
Re-key the gate on the actual destination thread —
targetThreadId || threadId, the same valuesendalready resolves a few lines down — and block only when that thread has a run in flight:This is purely frontend. Backend admission caps (
max_concurrent_runs_per_userdefault 3, conversation capNone) don't reject a second thread, and backendThreadBusyis per-thread, so a brand-new thread id never trips it.Tests
Added three functional regression tests in
useChat-send.test.mjsthat drive the realuseChat.sendcaller — no DOM, markup, or class names, they assert whether the request actually reachessendMessage/createThread:starts a new chat while another thread's run is active— new chat creates the thread and sends.addresses a second thread in parallel while viewing a running thread— reachessendMessagefor the other thread.still blocks a duplicate send into the already-running thread— preserved guard.Verified the tests catch the regression: tests 1 & 2 fail against the #5256 code (
createThreadnever called; message never reachessendMessage) and pass with this fix; test 3 passes either way (so the fix isn't just removing the protection).Rebuilt the committed
static/dist/app.jsesbuild bundle so the fix ships at runtime.🤖 Generated with Claude Code