[codex] Remove chat-triggered Slack connect flow - #5463
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ 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
WalkthroughRemoves the Slack channel-connect UI and command path in the web UI, updates the associated tests and fixtures, and changes Slack triggered-run delivery to run inline in the composition runtime. ChangesChannel-connect removal
Slack triggered-run delivery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
There was a problem hiding this comment.
Code Review
This pull request removes the channel connect slash command functionality, including the ChannelConnectCard component, the channelConnectAction state, and its resolution logic from the chat page and the useChat hook. The associated tests have been updated to reflect these removals and the resulting shift in state indices. The review feedback highlights a bug in the test suite where an assertion still queries index === 6 instead of the shifted index === 5 for busyGateNotice, resulting in a silent loss of test coverage, and suggests updating an outdated comment describing the state slot order.
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.
| assert.equal(stateSlots.get(4).value, pendingGate); | ||
| assert.equal(renderedMessages.length, 0); | ||
| assert.deepEqual( | ||
| stateUpdates.filter((call) => call.index === 6).map((call) => call.value?.content), |
There was a problem hiding this comment.
Since the channelConnectAction state variable (previously index 3) has been removed, the indices of all subsequent state variables in useChat have shifted down by 1. Specifically, busyGateNotice is now index 5 (previously index 6). Filtering stateUpdates by index === 6 actually targets stateThreadId (now index 6) instead of busyGateNotice, which causes the assertion to check the wrong state variable and silently lose test coverage. This should be updated to index === 5.
| stateUpdates.filter((call) => call.index === 6).map((call) => call.value?.content), | |
| stateUpdates.filter((call) => call.index === 5).map((call) => call.value?.content), |
| // State slot order: cooldownUntil(0), now(1), activeRun(2), | ||
| // channelConnectAction(3), isProcessing(4), pendingGate(5), | ||
| // isProcessing(3), pendingGate(4), | ||
| // busyGateNotice(6), stateThreadId(7). |
There was a problem hiding this comment.
The comment describing the state slot order is outdated. Since channelConnectAction was removed, busyGateNotice is now index 5 and stateThreadId is index 6. Let's update the comment to reflect the correct indices.
| // State slot order: cooldownUntil(0), now(1), activeRun(2), | |
| // channelConnectAction(3), isProcessing(4), pendingGate(5), | |
| // isProcessing(3), pendingGate(4), | |
| // busyGateNotice(6), stateThreadId(7). | |
| // State slot order: cooldownUntil(0), now(1), activeRun(2), | |
| // isProcessing(3), pendingGate(4), | |
| // busyGateNotice(5), stateThreadId(6). |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs (4)
1093-1096: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLikely wrong index for the busy-notice "no content" check.
busyGateNoticeobjects carry a.contentfield (see line 807); per the file's mapping that's index 5, not 6 (index 6 isstateThreadId, a plain string with no.content). This filter is effectively a no-op and doesn't exercise the intended guarantee.🐛 Proposed fix
assert.deepEqual( - stateUpdates.filter((call) => call.index === 6).map((call) => call.value?.content), + stateUpdates.filter((call) => call.index === 5).map((call) => call.value?.content), [], );🤖 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 1093 - 1096, The busy-notice “no content” assertion is checking the wrong state update index in useChat-send.test.mjs, so it never validates busyGateNotice.content. Update the filter in the relevant test to target the state update slot that actually holds the busyGateNotice object (the one mapped to content in this test file, not stateThreadId), and keep the assertion that its content collection is empty so the intended guarantee is exercised.
2267-2334: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTest name promises "does not fetch" but the mock allows the fetch to succeed.
Unlike every other "should not fetch connectable channels" test in this file (e.g. lines 109-111, 337-339, 1776-1779), this one's
queryClient.fetchQueryis permissive (async ({ queryFn }) => queryFn()) instead of throwing. If a future regression reintroduces a pre-sendfetchQuerycall, this test would still pass — it provides no actual signal for the behavior it's named after.🐛 Proposed fix
queryClient: { - fetchQuery: async ({ queryFn }) => queryFn(), + fetchQuery: async () => { + throw new Error("chat send should not fetch connectable channels"); + }, invalidateQueries: () => {}, },🤖 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 2267 - 2334, The test useChat.send: does not fetch connectable channels before submitting chat is too permissive because queryClient.fetchQuery currently executes the queryFn instead of failing, so it would not catch a regression. Update the test setup in useChat-send.test.mjs so fetchQuery throws if called, matching the other “should not fetch connectable channels” tests, and keep the assertions around createThreadRequest, sendMessage, and loggedErrors to verify send() never triggers a pre-send channel fetch.
811-914: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWrong state-slot index: busy-notice check reads
stateThreadId, notbusyGateNotice.
index === 6isstateThreadIdper the established mapping in this same file (see the "thread switch render" test, lines 1262-1328, andcreateResolveGateContext's comment at 2345-2346: pendingGate=4, busyGateNotice=5, stateThreadId=6). The sibling test just above (lines 798-808) correctly checksindex === 5for the busy notice. As written, this assertion can't detect a busy notice incorrectly written into the deactivated thread.🐛 Proposed fix
assert.deepEqual( - stateUpdates.filter((call) => call.index === 6).map((call) => call.value), + stateUpdates.filter((call) => call.index === 5).map((call) => call.value), [], "a busy gate notice must not be written into a thread that became active later", );🤖 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 811 - 914, The assertion in the busy-seeds test is checking the wrong state slot index, so it cannot detect a misplaced busy notice. Update the check in the `useChat.send: rejected busy seeds notice when active thread changed in flight` test to target the `busyGateNotice` slot used elsewhere in this file, matching the established mapping from `createResolveGateContext` and the sibling busy-notice test. Keep the rest of the test logic the same, but make sure the `stateUpdates` filter verifies the busy notice slot rather than `stateThreadId`.
916-1002: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTwo wrong state-slot indices in this test.
stateSlots.get(5)should bestateSlots.get(4)(pendingGate — see lines 708-712/803 wheresetPendingGateFromEventsconsistently writes index 4), and theindex === 6filter at the end should beindex === 5(busyGateNotice, per the file-wide mapping). As written, neither assertion checks the slot its message claims to verify.🐛 Proposed fix
- assert.equal(stateSlots.get(5).value, null); + assert.equal(stateSlots.get(4).value, null); assert.equal(renderedMessages.length, 2); assert.equal(renderedMessages[0].status, "error"); assert.equal(renderedMessages[1].role, "system"); assert.equal(renderedMessages[1].content, "Thread is busy, please try again."); assert.deepEqual( - stateUpdates.filter((call) => call.index === 6).map((call) => call.value), + stateUpdates.filter((call) => call.index === 5).map((call) => call.value), [], "a resolved gate should not get a lingering card-level busy notice", );🤖 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 916 - 1002, The test in useChat.send is asserting the wrong state slots for pendingGate and busyGateNotice. Update the assertion that reads stateSlots.get(5) to use the pendingGate slot consistent with setPendingGateFromEvents and the existing slot mapping, and change the stateUpdates filter from index === 6 to the busyGateNotice slot used throughout this file. Keep the test names and expectations aligned with the actual slot indices referenced by useChat and useChatEvents.
🤖 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 2543-2546: The state slot order comment in the fixture is stale
and mislabels busyGateNotice and stateThreadId by one index. Update the mapping
in the test setup around initialByIndex to match the established slot order used
elsewhere in useChat-send.test.mjs, keeping busyGateNotice and stateThreadId
aligned with their actual indices so the comment remains accurate if this
fixture is extended.
---
Outside diff comments:
In
`@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs`:
- Around line 1093-1096: The busy-notice “no content” assertion is checking the
wrong state update index in useChat-send.test.mjs, so it never validates
busyGateNotice.content. Update the filter in the relevant test to target the
state update slot that actually holds the busyGateNotice object (the one mapped
to content in this test file, not stateThreadId), and keep the assertion that
its content collection is empty so the intended guarantee is exercised.
- Around line 2267-2334: The test useChat.send: does not fetch connectable
channels before submitting chat is too permissive because queryClient.fetchQuery
currently executes the queryFn instead of failing, so it would not catch a
regression. Update the test setup in useChat-send.test.mjs so fetchQuery throws
if called, matching the other “should not fetch connectable channels” tests, and
keep the assertions around createThreadRequest, sendMessage, and loggedErrors to
verify send() never triggers a pre-send channel fetch.
- Around line 811-914: The assertion in the busy-seeds test is checking the
wrong state slot index, so it cannot detect a misplaced busy notice. Update the
check in the `useChat.send: rejected busy seeds notice when active thread
changed in flight` test to target the `busyGateNotice` slot used elsewhere in
this file, matching the established mapping from `createResolveGateContext` and
the sibling busy-notice test. Keep the rest of the test logic the same, but make
sure the `stateUpdates` filter verifies the busy notice slot rather than
`stateThreadId`.
- Around line 916-1002: The test in useChat.send is asserting the wrong state
slots for pendingGate and busyGateNotice. Update the assertion that reads
stateSlots.get(5) to use the pendingGate slot consistent with
setPendingGateFromEvents and the existing slot mapping, and change the
stateUpdates filter from index === 6 to the busyGateNotice slot used throughout
this file. Keep the test names and expectations aligned with the actual slot
indices referenced by useChat and useChatEvents.
🪄 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: e84e7b2e-6e7b-4763-8adf-55408365a54b
📒 Files selected for processing (9)
crates/ironclaw_webui_v2_static/src/assets.rscrates/ironclaw_webui_v2_static/static/js/lib/channel-connect.jscrates/ironclaw_webui_v2_static/static/js/lib/channel-connect.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/channel-connect-card.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/channel-connect-card.test.mjscrates/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
💤 Files with no reviewable changes (6)
- crates/ironclaw_webui_v2_static/static/js/lib/channel-connect.test.mjs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/components/channel-connect-card.test.mjs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/components/channel-connect-card.js
- crates/ironclaw_webui_v2_static/static/js/lib/channel-connect.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjs
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/lib/useChat-send.test.mjs (1)
2219-2287: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest name claims no connectable-channels fetch but the stub doesn't enforce it.
This test asserts behavior "without fetching connectable channels," yet
queryClient.fetchQuery(line 2245) just executesqueryFn()instead of throwing like every sibling test does (e.g. lines 2004-2007, 2158-2161). If a regression reintroduced a pre-send connectable-channels fetch, this test would still pass.🧪 Proposed fix to actually enforce the claim
queryClient: { - fetchQuery: async ({ queryFn }) => queryFn(), + fetchQuery: async () => { + throw new Error("slash-connect prompts should not fetch connectable channels"); + }, invalidateQueries: () => {}, },🤖 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 2219 - 2287, The test “useChat.send: slash connect text submits to the model without fetching connectable channels” does not actually enforce that no connectable-channels lookup happens. Update the `queryClient.fetchQuery` stub in this test to match the sibling tests by throwing if it is called, so any unexpected pre-send fetch fails the test. Keep the existing assertions on `useChat`, `send`, and `createThreadRequest` behavior, but make the no-fetch expectation explicit through the stub.
🤖 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/lib/useChat-send.test.mjs`:
- Around line 2219-2287: The test “useChat.send: slash connect text submits to
the model without fetching connectable channels” does not actually enforce that
no connectable-channels lookup happens. Update the `queryClient.fetchQuery` stub
in this test to match the sibling tests by throwing if it is called, so any
unexpected pre-send fetch fails the test. Keep the existing assertions on
`useChat`, `send`, and `createThreadRequest` behavior, but make the no-fetch
expectation explicit through the stub.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27d4c93a-7cea-420b-9a3e-58dea6680b94
📒 Files selected for processing (2)
crates/ironclaw_webui_v2_static/src/assets.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
3c9dfe4 to
96e5b75
Compare
|
🚅 Deployed to the ironclaw-pr-5463 environment in ironclaw-ci-preview
|
This branch was successfully deployed
1 participant
Summary
Removes the Reborn WebUI v2 chat-triggered Slack/channel connection flow entirely. Chat no longer tries to classify user text or slash-style input as a channel connection command.
Slack setup and pairing remain available through the dedicated Extensions/channel UI. The shared
channel-connect.jsmodule now only exposeslistConnectableChannels()for that UI.Why
The chat path had brittle text matching around Slack connection activation. Even after narrowing it, chat-owned connection shortcuts still made normal messages like Slack setup or regex questions vulnerable to being treated as UI activation commands. The safer product boundary is explicit UI setup only.
Behavior
/connect slacksubmits as normal chat text.connect my Slack accountsubmits as normal chat text.setup a Slack regex for chat routingsubmits as normal chat text.Validation
node --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjsnode --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjscargo test -p ironclaw_webui_v2_staticgit diff --checkNote: the first
cargo test -p ironclaw_webui_v2_staticattempt hitNo space left on devicewhile writing Rust incremental cache undertarget/; after clearing generated incremental cache, the same test command passed.