Fix/disable refresh during active turn#66395
Conversation
Greptile SummaryThis PR contains two separate fixes: (1) the advertised UI fix that adds The UI fix is correct and minimal. The Slack changes are sound (the Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/slack/src/monitor/provider.gateway-crash.test.ts
Line: 33-44
Comment:
**Test coverage for max-reconnect paths**
The new test validates only the non-recoverable auth error branch (`isNonRecoverableSlackAuthError` → `return`). The two max-reconnect-attempts branches converted from `throw` to `return` in `provider.ts` — one in the `app.start()` catch block and one in the post-disconnect block — have no test coverage. If the `runtime.error?.()` call were removed or the `return` were accidentally reverted to `throw` in either of those paths, no test would catch it.
Consider adding two more cases to this describe block:
```ts
it("resolves instead of rejecting after exhausting reconnect attempts on start", async () => {
// mock start() to throw a non-auth error N times (N = maxAttempts)
...
await expect(monitorSlackProvider({ ... })).resolves.toBeUndefined();
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(ui): disable chat refresh button dur..." | Re-trigger Greptile |
|
|
||
| describe("monitorSlackProvider - gateway crash prevention", () => { | ||
| it("resolves instead of rejecting on non-recoverable auth error", async () => { | ||
| await expect( | ||
| monitorSlackProvider({ | ||
| botToken: "xoxb-test", | ||
| appToken: "xapp-test", | ||
| accountId: "default", | ||
| }), | ||
| ).resolves.toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new test validates only the non-recoverable auth error branch (isNonRecoverableSlackAuthError → return). The two max-reconnect-attempts branches converted from throw to return in provider.ts — one in the app.start() catch block and one in the post-disconnect block — have no test coverage. If the runtime.error?.() call were removed or the return were accidentally reverted to throw in either of those paths, no test would catch it.
Consider adding two more cases to this describe block:
it("resolves instead of rejecting after exhausting reconnect attempts on start", async () => {
// mock start() to throw a non-auth error N times (N = maxAttempts)
...
await expect(monitorSlackProvider({ ... })).resolves.toBeUndefined();
});Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/slack/src/monitor/provider.gateway-crash.test.ts
Line: 33-44
Comment:
**Test coverage for max-reconnect paths**
The new test validates only the non-recoverable auth error branch (`isNonRecoverableSlackAuthError` → `return`). The two max-reconnect-attempts branches converted from `throw` to `return` in `provider.ts` — one in the `app.start()` catch block and one in the post-disconnect block — have no test coverage. If the `runtime.error?.()` call were removed or the `return` were accidentally reverted to `throw` in either of those paths, no test would catch it.
Consider adding two more cases to this describe block:
```ts
it("resolves instead of rejecting after exhausting reconnect attempts on start", async () => {
// mock start() to throw a non-auth error N times (N = maxAttempts)
...
await expect(monitorSlackProvider({ ... })).resolves.toBeUndefined();
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07c1dde3b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -593,14 +593,17 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { | |||
| runtime.error?.( | |||
| `slack socket mode failed to start due to non-recoverable auth error — skipping channel (${formatUnknownError(err)})`, | |||
| ); | |||
| throw err; | |||
| return; | |||
There was a problem hiding this comment.
Preserve fatal startup errors by rejecting monitor task
Returning here makes monitorSlackProvider resolve on non-recoverable startup failures, but src/gateway/server-channels.ts only records lastError when startAccount rejects (the catch path around lines 432-435). In the auth-failure case this now leaves runtime status with lastError: null, so Slack appears stopped without an actionable reason in channel status/UI even though startup failed due to invalid credentials.
Useful? React with 👍 / 👎.
07c1dde to
28b4306
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28b43061af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| error = vi.fn(); | ||
| } | ||
| class HTTPReceiver {requestListener = vi.fn();} | ||
| return { default: App, App, HTTPReceiver }; |
There was a problem hiding this comment.
Provide SocketModeReceiver in mocked @slack/bolt export
This mock returns only App and HTTPReceiver, but monitorSlackProvider runs in socket mode (config.mode: "socket") and resolveSlackBoltInterop requires SocketModeReceiver as well. The test will fail early with Unable to resolve @slack/bolt App/HTTPReceiver exports before it reaches the auth-error path, so it neither validates the intended regression nor remains stable in CI.
Useful? React with 👍 / 👎.
| runtime.error?.( | ||
| `slack socket mode failed to start after ${reconnectAttempts} attempts — stopping channel (${formatUnknownError(err)})`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Propagate max-attempt startup failures to channel runtime
Returning when socket startup retries are exhausted treats a hard startup failure as a successful completion. In src/gateway/server-channels.ts, the channel runtime only records lastError in the startAccount rejection path, so this branch now stops/restarts without surfacing the failure reason to status/UI. This should reject (or explicitly set an error status) so operators can diagnose persistent startup failures.
Useful? React with 👍 / 👎.
28b4306 to
9f537a9
Compare
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the latest contributor branch is now the intended UI-only guard and appears functionally correct, but an open maintainer PR, #74690, explicitly supersedes this UI-only portion, adds browser coverage for the disabled-state matrix, and is the canonical path for closing the underlying issue. So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Use #74690 as the canonical implementation path because it contains the same narrow Control UI guard plus focused browser coverage, then close #65522 when that maintainer PR lands and ships. Security review: Security review cleared: The latest diff only changes a Control UI button disabled binding and does not touch dependencies, workflows, permissions, secrets, network calls, or code execution paths. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 1fb096f0e607. |
BunsDev
left a comment
There was a problem hiding this comment.
Thanks for the Control UI fix. I am requesting changes because this PR is mixed-scope: the UI disabled-state guard is valid, but the Slack provider lifecycle edits introduce a blocking regression by treating permanent startup failures as clean resolution, which can hide failure status and allow restart loops. Please split this into a UI-only PR for #65522, or remove the Slack changes and add focused Control UI coverage. I am superseding the UI fix with a clean local maintainer patch.
9f537a9 to
4cb375e
Compare
@BunsDev Slack changes removed — PR is now UI-only, single commit touching only ui/src/ui/app-render.helpers.ts. Ready for re-review. |
The refresh button was only guarded by chatLoading and connected state, allowing clicks during active agent turns. This could stall the run, reset chat state, and wedge the gateway. Add chatSending, chatRunId, and chatStream guards to match the existing busy-state pattern used by the model and thinking selects. Fixes openclaw#65522
4cb375e to
ec35ffa
Compare
Summary
chatLoadingor disconnected, remaining clickable during active agent turns.chatSending,chatRunId, andchatStreamguards to the refresh button'sdisabledcondition.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
disabledbinding was missing the busy-state guards (chatSending,chatRunId,chatStream) that are already used by the model select and thinking select controls in the same file.busyvariable combining these four flags. The refresh button only had two of them.Regression Test Plan (if applicable)
ui/src/ui/app-render.helpers.tschatSending,chatRunId, andchatStream?disabled), so it's evaluated on every render cycle.User-visible / Behavior Changes
Refresh button is now grayed out and non-clickable while the agent is sending, running, or streaming a response.
Diagram (if applicable)
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Refresh button should be disabled (grayed out) during the entire agent turn lifecycle.
Actual (before fix)
Refresh button remained clickable. Clicking it mid-turn reset the chat and stalled the run.
Evidence
Before (refresh button clickable during active turn)
After (refresh button disabled during active turn)
Human Verification (required)
Review Conversations
Compatibility / Migration
Risks and Mitigations
chatRunIdorchatStreamare set but the turn is actually stale, leaving the button permanently disabled.resetChatStateForSessionSwitch) clears all three flags.