fix(reborn): keep approval gates visible on busy sends#5241
Conversation
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesApproval gate survives busy sends
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 451-456: The send flow in useChat.js is restoring a stale
render-time gate snapshot after async send, which can overwrite a gate that was
resolved, cleared, or replaced while the request was in flight. Update the
sendMessage path around gateBeforeSend, setPendingGate, and setIsProcessing so
busy/error paths leave the current gate untouched, and on non-busy success only
clear the pending gate if gateBeforeSend is still the active one. Apply the same
guard to the other gate-handling block mentioned in the diff as well.
In
`@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs`:
- Around line 388-392: The busy-path test around send should assert the
invariant that the approval gate remains visible without depending on restoring
the old pendingGate snapshot. Update the expectation in useChat send tests to
verify the gate is not cleared during a busy rejection, and add a race scenario
in send where the gate changes before the response so the test confirms a
resolved or replaced gate is not resurrected by send.
In `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py`:
- Around line 398-456: The Ruff warnings in the smoke test need to be cleaned up
by adding explicit `-> None` return annotations to the private async route
handlers in this scenario, including fulfill_json, handle_session,
handle_threads, handle_timeline, and handle_send, so their signatures are
consistent and type-safe. Also update the compound assertion near the end of the
test into separate assertions so the check is clearer and satisfies the style
rule; use the surrounding test function and these helper names to locate the
affected block.
- Around line 356-363: The browser test in
test_reborn_v2_busy_rejection_keeps_approval_gate_visible is creating its own
context and page instead of using the mandated function-scoped page fixture.
Update this scenario to accept and use page directly, then add the init script
and any route setup on that page before navigating to the /v2/chat route, and
remove the manual context creation/cleanup; apply the same fixture-based pattern
to the related test referenced in the comment as well.
🪄 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: 6a481aff-8792-4879-9219-8d61cb80465d
📒 Files selected for processing (4)
crates/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/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
|
🚅 Deployed to the ironclaw-pr-5241 environment in ironclaw-ci-preview
|
|
@claude review |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 490-492: The busyGateNotice update in useChat is using the stale
gateBeforeSend snapshot, which can leave the banner stranded after the gate
resolves. Update the post-await notice assignment so it only calls
setBusyGateNotice when the live gate state still matches the triggering gate,
using the current pending gate ref/state rather than the captured gateBeforeSend
value. Keep the check alongside the existing noticeKey and
shouldRenderInCurrentThread logic so the notice is only shown when the gate is
still current.
🪄 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: 76573d54-9752-4220-bbde-1e244443ef88
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (5)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.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/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
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 490-495: The busy notice handling in useChat is now skipping the
notice entirely when the gate snapshot is stale, which leaves failed attempts
with no server feedback. Update the busy-notice path around busyNoticeKey,
pendingGateRef.current, and setBusyGateNotice so the notice is still surfaced in
the conversation flow even when the current gate has been replaced or resolved,
instead of falling through to the empty false branch. Preserve the existing
check for the active gate, but add a fallback that keeps or attaches the busy
notice to the attempted message/thread rather than dropping it.
In
`@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs`:
- Around line 870-970: The busy-rejection handling in useChat.send is currently
allowing the server notice to disappear when a gate changes or resolves during
the request. Update the logic around the send flow and the related test
expectations in useChat-send.test.mjs so the rejected busy outcome still
surfaces feedback: keep the stale-gate guard, but when the current gate is gone,
fall back to appending a system notice; when a gate still exists, attach the
notice to that gate instead. Use the existing useChat, send, and pending gate
state handling to locate the path.
🪄 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: 2c0ead00-1688-4236-93bc-f676bb4fe664
⛔ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/scenarios/test_reborn_webui_v2_smoke.py (1)
525-525: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the standard
pagefixture for this scenario.This new browser test still depends on
reborn_v2_page, so it keeps bypassing the repo’s required browser-test harness instead of using the function-scopedpagefixture. As per coding guidelines,tests/e2e/scenarios/test_*.py: "Use thepagefixture for browser tests; it provides a fresh browser context per test...". As per path instructions,tests/e2e/CLAUDE.md: "Browser tests should use thepagefixture (fresh context per test)...".🤖 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 `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py` at line 525, The test `test_reborn_v2_composer_accepts_draft_while_run_is_processing` is still using the shared `reborn_v2_page` fixture instead of the required standard `page` fixture. Update this browser test to accept and use `page` directly, and remove the dependency on `reborn_v2_page` so it runs in a fresh browser context like the rest of the `test_*` browser scenarios.Sources: Coding guidelines, Path instructions
🤖 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/scenarios/test_reborn_webui_v2_smoke.py`:
- Around line 542-543: The current assertion in the second-send flow can
false-pass because `to_have_count(1)` may already be satisfied before the extra
`Enter` is fully processed. Update the check around `composer.press("Enter")`
and `expect(reborn_v2_page.locator(SEL_V2["msg_user"]))` to wait for a short
settle period after the keypress, then verify no second user bubble appears, or
alternatively assert that the draft/input state remains present after the second
press.
---
Duplicate comments:
In `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py`:
- Line 525: The test
`test_reborn_v2_composer_accepts_draft_while_run_is_processing` is still using
the shared `reborn_v2_page` fixture instead of the required standard `page`
fixture. Update this browser test to accept and use `page` directly, and remove
the dependency on `reborn_v2_page` so it runs in a fresh browser context like
the rest of the `test_*` browser scenarios.
🪄 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: 714f0a88-243e-48cc-adff-0a3736372f40
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (5)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.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/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/scenarios/test_reborn_webui_v2_smoke.py (1)
525-525: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the standard
pagefixture for this scenario.This new browser test still depends on
reborn_v2_page, so it keeps bypassing the repo’s required browser-test harness instead of using the function-scopedpagefixture. As per coding guidelines,tests/e2e/scenarios/test_*.py: "Use thepagefixture for browser tests; it provides a fresh browser context per test...". As per path instructions,tests/e2e/CLAUDE.md: "Browser tests should use thepagefixture (fresh context per test)...".🤖 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 `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py` at line 525, The test `test_reborn_v2_composer_accepts_draft_while_run_is_processing` is still using the shared `reborn_v2_page` fixture instead of the required standard `page` fixture. Update this browser test to accept and use `page` directly, and remove the dependency on `reborn_v2_page` so it runs in a fresh browser context like the rest of the `test_*` browser scenarios.Sources: Coding guidelines, Path instructions
🤖 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/scenarios/test_reborn_webui_v2_smoke.py`:
- Around line 542-543: The current assertion in the second-send flow can
false-pass because `to_have_count(1)` may already be satisfied before the extra
`Enter` is fully processed. Update the check around `composer.press("Enter")`
and `expect(reborn_v2_page.locator(SEL_V2["msg_user"]))` to wait for a short
settle period after the keypress, then verify no second user bubble appears, or
alternatively assert that the draft/input state remains present after the second
press.
---
Duplicate comments:
In `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py`:
- Line 525: The test
`test_reborn_v2_composer_accepts_draft_while_run_is_processing` is still using
the shared `reborn_v2_page` fixture instead of the required standard `page`
fixture. Update this browser test to accept and use `page` directly, and remove
the dependency on `reborn_v2_page` so it runs in a fresh browser context like
the rest of the `test_*` browser scenarios.
🪄 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: 714f0a88-243e-48cc-adff-0a3736372f40
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (5)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.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/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
🛑 Comments failed to post (1)
tests/e2e/scenarios/test_reborn_webui_v2_smoke.py (1)
542-543: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Line 543 can false-pass before a blocked second send shows up.
to_have_count(1)is already true before the secondEnter, so this assertion can return immediately and miss a delayed extra user bubble. Re-check after a short settle window, or assert that the draft is still present after the second keypress.Suggested tightening
await composer.press("Enter") - await expect(reborn_v2_page.locator(SEL_V2["msg_user"])).to_have_count(1, timeout=1000) + await reborn_v2_page.wait_for_timeout(1000) + await expect(reborn_v2_page.locator(SEL_V2["msg_user"])).to_have_count(1) + await expect(composer).to_have_value("draft while the reply is still running")📝 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.await composer.press("Enter") await reborn_v2_page.wait_for_timeout(1000) await expect(reborn_v2_page.locator(SEL_V2["msg_user"])).to_have_count(1) await expect(composer).to_have_value("draft while the reply is still running")🤖 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 `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py` around lines 542 - 543, The current assertion in the second-send flow can false-pass because `to_have_count(1)` may already be satisfied before the extra `Enter` is fully processed. Update the check around `composer.press("Enter")` and `expect(reborn_v2_page.locator(SEL_V2["msg_user"]))` to wait for a short settle period after the keypress, then verify no second user bubble appears, or alternatively assert that the draft/input state remains present after the second press.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/useChat-send.test.mjs`:
- Around line 644-666: The test in useChat-send.test.mjs is over-constraining
gated send attempts by expecting the attempted message to disappear from
renderedMessages. Update the assertions around chat.send("another request") to
verify the user’s attempted message remains in the conversation timeline with
failed/retry and approval/busy feedback, rather than asserting an empty
renderedMessages array. Keep the short-circuit behavior for the
backend/sendCalls if needed, but align the checks with the intended flow in the
send path and approval gate handling used by chat.send and the state update
assertions.
🪄 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: 6fd46930-403a-4e6b-b3f9-739b65de1524
📒 Files selected for processing (4)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
143313b to
687a043
Compare
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 519-530: The busy banner update in useChat’s sendMessage response
handling is using a stale shouldRenderInCurrentThread value, so it can write
setBusyGateNotice for the wrong active thread after a thread switch. Update this
branch to re-check the live active thread before calling setBusyGateNotice,
using the current thread state alongside sendThreadId/pendingGateRef, and fall
back to appendSystemNotice() when the response no longer belongs to the active
thread.
- Around line 242-250: The pre-send approval gate guard in useChat should rely
on committed state instead of pendingGateRef.current, because the ref can still
point to the previous thread immediately after the thread-reset effect runs.
Update the send/chat submission path that performs the pre-await gate check to
use pendingGate for the initial validation, keep pendingGateRef.current only for
the post-await recheck, and adjust the callback dependencies so the guard stays
aligned with the current thread state.
🪄 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: fed8c7f6-26fd-4d22-9ad3-ca163eec6664
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (8)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/approval-card.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/suggestion-chips.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjstests/e2e/helpers.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js (1)
526-537: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the live render decision when appending the fallback notice.
When a target-thread send starts off-screen (
shouldRenderInCurrentThread === false) and the user switches into that target before therejected_busyresponse,liveShouldRenderInCurrentThreadis true, but Line 536 callsappendSystemNotice()with the stale default, so the notice is only seeded and not rendered in the now-active thread.Suggested fix
if (liveShouldRenderInCurrentThread) { const currentNoticeKey = busyNoticeKey(sendThreadId, pendingGateRef.current); if (currentNoticeKey) { setBusyGateNotice({ gateKey: currentNoticeKey, content: response.notice, }); } else { - appendSystemNotice(); + appendSystemNotice(true); } } else { appendSystemNotice(false); }🤖 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 526 - 537, The fallback busy notice path in useChat should use the live thread visibility decision instead of the stale default when handling rejected_busy. In the liveShouldRenderInCurrentThread block, update the appendSystemNotice flow so it renders based on the current target thread state, not the original shouldRenderInCurrentThread value; use the existing liveShouldRenderInCurrentThread, sendThreadId, and appendSystemNotice logic together so a thread that becomes active before the response still shows the notice immediately.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js`:
- Around line 526-537: The fallback busy notice path in useChat should use the
live thread visibility decision instead of the stale default when handling
rejected_busy. In the liveShouldRenderInCurrentThread block, update the
appendSystemNotice flow so it renders based on the current target thread state,
not the original shouldRenderInCurrentThread value; use the existing
liveShouldRenderInCurrentThread, sendThreadId, and appendSystemNotice logic
together so a thread that becomes active before the response still shows the
notice immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e079fd97-2d36-4cbc-8b81-2e771cbbd627
⛔ 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
|
[PASS] Reviewed latest head: passed end-to-end smoke/JS tests with no new CI failures, and mergeability is healthy. No blocking correctness findings in first-pass diff scan. Residual risk: no additional behavioral test beyond existing JS coverage was added in this pass. Guidance for human follow-up:
|
Summary
rejected_busysend path and a browser E2E regression for the v2 SPA approval-gate UI.Linked Issue
Closes #5210
Validation
node --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjspython -m py_compile tests/e2e/helpers.py tests/e2e/scenarios/test_reborn_webui_v2_smoke.pygit diff --checkpython -m pytest tests/e2e/scenarios/test_reborn_webui_v2_smoke.py::test_reborn_v2_busy_rejection_keeps_approval_gate_visible -q- not run locally because this environment does not have pytest installed:No module named pytestSecurity Impact
No. This changes browser-side Reborn WebUI v2 chat state handling only; approval resolution semantics and backend authorization are unchanged.
Database Impact
No schema or migration changes.
Blast Radius
Limited to Reborn WebUI v2 chat send behavior while an approval gate is pending, plus targeted unit and E2E regression coverage.
Rollback Plan
Revert this PR to restore the previous behavior where a new send attempt clears the local pending gate state until history/SSE rehydrates it.
Review track: C