Skip to content

fix(control-ui): disable refresh during active runs#74690

Merged
BunsDev merged 2 commits intomainfrom
fix/control-ui-refresh-active-run
Apr 30, 2026
Merged

fix(control-ui): disable refresh during active runs#74690
BunsDev merged 2 commits intomainfrom
fix/control-ui-refresh-active-run

Conversation

@BunsDev
Copy link
Copy Markdown
Member

@BunsDev BunsDev commented Apr 30, 2026

Summary

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: the refresh button only checked chatLoading and connection state, so active turn states were not reflected in native button disabling.
  • Missing detection / guardrail: browser helper tests did not cover refresh disabled state across send/run/stream phases.
  • Contributing context (if known): session.message reloads are already deferred while chatRunId is active because history reloads can race live streaming state.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: ui/src/ui/app-render.helpers.browser.test.ts
  • Scenario the test should lock in: refresh stays enabled while connected and idle, and disables for chatLoading, chatSending, active chatRunId, non-null chatStream, and disconnected state.
  • Why this is the smallest reliable guardrail: the regression is a Lit template disabled binding, so focused browser render coverage catches the visible control state without adding gateway protocol behavior.
  • Existing test that already covers this (if any): none for the active-turn refresh disabled states.
  • If no new test is added, why not: N/A.

User-visible / Behavior Changes

The Control UI refresh button is disabled during active chat turn lifecycle states and re-enabled when idle.

Diagram (if applicable)

Before:
[active chat turn] -> [refresh button enabled] -> [manual history refresh can race stream]

After:
[active chat turn] -> [refresh button disabled] -> [idle refresh behavior unchanged]

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

  • OS: macOS local checkout; Testbox Linux CI-parity runner
  • Runtime/container: Node 22 / pnpm workspace
  • Model/provider: N/A
  • Integration/channel (if any): Control UI chat controls
  • Relevant config (redacted): N/A

Steps

  1. Render chat header controls while connected and idle.
  2. Render the same controls with chatLoading, chatSending, active chatRunId, non-null chatStream, and disconnected state.
  3. Inspect the refresh button's native disabled state.

Expected

  • Refresh is enabled only when connected and idle.
  • Refresh is disabled during loading, send, active run, active stream, and disconnected states.

Actual

  • Before this fix, send/run/stream states left refresh enabled.
  • After this fix, all expected busy/disconnected states disable refresh.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

#66395 has a maintainer request-changes review because it mixes this valid UI fix with unrelated Slack lifecycle edits that have a blocking regression. BingqingLyu#2487 mirrors that same mixed-scope branch.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: stale active-run state could leave refresh disabled until the chat lifecycle clears it.
    • Mitigation: this reuses the same active-state signals already used by nearby Control UI controls, and the test locks the intended disabled-state matrix.

Validation

  • pnpm test ui/src/ui/app-render.helpers.browser.test.ts passed: 9 tests.
  • pnpm exec oxfmt --check --threads=1 ui/src/ui/app-render.helpers.ts ui/src/ui/app-render.helpers.browser.test.ts passed.
  • git diff --check origin/main...HEAD passed.
  • Testbox pnpm check:changed on tbx_01kqdv9hf4mbc1aqgwh2m738d6 selected expected lanes (core, coreTests) and passed conflict markers, changelog attributions, wildcard export checks, duplicate coverage, core typecheck, and core test typecheck, then failed in unrelated packages/sdk/src/index.test.ts on typescript-eslint(no-redundant-type-constituents). That file is unchanged by this PR and the same offending lines are present on origin/main.

AI-assisted: yes.

@BunsDev BunsDev added the codex label Apr 30, 2026
@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui size: XS maintainer Maintainer-authored PR labels Apr 30, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 30, 2026 •
edited
Loading

Codex review: needs maintainer review before merge.

What this changes:

This PR adds a refreshDisabled guard to the Control UI chat controls and browser-render coverage for idle, loading, sending, active-run, active-stream, and disconnected refresh-button states.

Maintainer follow-up before merge:

The PR is already a narrow implementation candidate, but MEMBER authorship and the protected maintainer label make the remaining action explicit maintainer review and merge/CI judgment, not an automated repair lane.

Security review:

Security review cleared: The diff is limited to a Lit disabled binding and focused browser tests; it does not touch dependencies, workflows, permissions, secrets, network calls, package metadata, or command execution paths.

Review details

Best possible solution:

Land the narrow UI guard if maintainers accept the touched-surface proof, let it close #65522 after merge, and keep #73836 as the separate thread for broader gateway/channel/media responsiveness work.

Do we have a high-confidence way to reproduce the issue?

Yes. A focused render of renderChatControls on current main with chatSending, chatRunId, or non-null chatStream reproduces the gap because the button only checks loading/disconnected state; the PR's browser matrix is the right regression path.

Is this the best way to solve the issue?

Yes. A native disabled-state guard in renderChatControls is the narrowest maintainable fix for this UI footgun, and it reuses the busy-state signals already used by adjacent Control UI controls.

Acceptance criteria:

  • pnpm test ui/src/ui/app-render.helpers.browser.test.ts
  • pnpm exec oxfmt --check --threads=1 ui/src/ui/app-render.helpers.ts ui/src/ui/app-render.helpers.browser.test.ts
  • git diff --check origin/main...HEAD
  • Confirm exact-head GitHub checks or rerun the changed gate in Testbox before merge.

What I checked:

  • Protected maintainer handling: Provided PR metadata shows author association MEMBER and the maintainer label, so this requires explicit maintainer review rather than automated cleanup closure. (1511a086614a)
  • Current main gap: On current main, the refresh button is disabled only for state.chatLoading || !state.connected; active send/run/stream states are not included. (ui/src/ui/app-render.helpers.ts:278, e9d4cb2bb6dc)
  • Latest release has same gap: The latest release tag still shows the refresh button disabled only by state.chatLoading || !state.connected, so this is not already shipped. (ui/src/ui/app-render.helpers.ts:258, cbc2ba093146)
  • PR implementation scope: The provided PR diff adds a single refreshDisabled expression covering disconnected, loading, sending, active run id, and non-null stream state, then binds the refresh button to it. (ui/src/ui/app-render.helpers.ts:222, a2c237228d79)
  • PR regression coverage: The provided PR diff adds browser-render coverage asserting refresh is enabled only when connected and idle, and disabled for loading, sending, active run, active stream, and disconnected states. (ui/src/ui/app-render.helpers.browser.test.ts:90, a2c237228d79)
  • Existing busy-state contract: Current main already uses the same loading/sending/run/stream busy expression for chat model and thinking controls, so the PR aligns refresh behavior with nearby controls. (ui/src/ui/chat/session-controls.ts:90, e9d4cb2bb6dc)

Likely related people:

  • steipete: The PR discussion's prior ClawSweeper review routes this surface to steipete based on recent current-main maintenance of Control UI chat lifecycle, session controls, and reconciliation paths adjacent to refresh behavior. (role: recent maintainer and adjacent owner; confidence: medium; commits: 1fad8efa12cf, 1a8a6f8fba04, 07877d71cd17; files: ui/src/ui/app-render.helpers.ts, ui/src/ui/chat/session-controls.ts, ui/src/ui/controllers/chat.ts)
  • BunsDev: Beyond authoring this PR, the PR discussion records recent current-main Control UI commits by BunsDev in the same render/helper and chat surfaces, and the current PR was split into the narrow UI-only path. (role: recent Control UI maintainer; confidence: medium; commits: b1c515270eb5, d0c83777fb58, ca414735b9ee; files: ui/src/ui/app-render.helpers.ts, ui/src/ui/app-render.ts, ui/src/ui/app-chat.ts)
  • vincentkoc: The PR discussion links vincentkoc to recent merged work on active-run and final-reload reconciliation in the same chat controller/lifecycle area that this refresh guard protects. (role: recent chat reconciliation maintainer; confidence: medium; commits: 02908db62b30, cff991c88d04, 6b6dcafcee9c; files: ui/src/ui/controllers/chat.ts, ui/src/ui/app-render.ts, ui/src/ui/app-render.helpers.ts)

Remaining risk / open question:

Codex review notes: model gpt-5.5, reasoning high; reviewed against e9d4cb2bb6dc.

@BunsDev
Copy link
Copy Markdown
Member Author

BunsDev commented Apr 30, 2026

CI triage for exact head a2c237228d792835aced2208612db3062993e566:

  • PR diff is limited to ui/src/ui/app-render.helpers.ts and ui/src/ui/app-render.helpers.browser.test.ts.
  • Relevant UI shard checks-node-core-ui passed.
  • build-artifacts passed including Control UI build.
  • Failing checks are outside this PR's touched surface:
    • check-lint: packages/sdk/src/index.test.ts has typescript-eslint(no-redundant-type-constituents) on the existing unknown union at lines 12 and 17; those lines are present on origin/main.
    • checks-node-agentic-commands-models: stale forward-compat expectations in src/commands/models/list.list-command.forward-compat.test.ts now receive workspaceDir / loadAvailability fields.
    • checks-node-core-support-boundary / aggregate checks-node-core / aggregate check: wrapper failures from the same unrelated SDK/model-test issues.

Local and Testbox proof for the touched surface remains green:

  • pnpm test ui/src/ui/app-render.helpers.browser.test.ts passed 9 tests.
  • pnpm exec oxfmt --check --threads=1 ui/src/ui/app-render.helpers.ts ui/src/ui/app-render.helpers.browser.test.ts passed.
  • git diff --check origin/main...HEAD passed.
  • Testbox pnpm check:changed selected core, coreTests, passed preflight/typecheck, then hit the same unrelated SDK lint issue above.

This was referenced Apr 30, 2026
Closed
Closed
@Angfr95
Copy link
Copy Markdown
Contributor

Angfr95 commented Apr 30, 2026

This PR was opened after #66395, which closed the same issue with an equivalent fix plus browser render test coverage.
The key differences:

  • My PR extracts refreshDisabled as a named variable (more readable/maintainable)
  • My PR adds 6 parametrized browser tests locking in the disabled-state matrix across all lifecycle phases (idle, loading, sending, active run, active stream, disconnected)
  • This PR inlines the condition and adds no tests

@clawsweeper clawsweeper Bot mentioned this pull request Apr 30, 2026
Closed
2 tasks
@BunsDev
Copy link
Copy Markdown
Member Author

BunsDev commented Apr 30, 2026 •
edited
Loading

Thanks for the follow-up. This looks like it is mixing up the related PRs.

Current #74690 is the canonical path for #65522:

  • it extracts refreshDisabled as a named guard in ui/src/ui/app-render.helpers.ts
  • it adds browser render coverage in ui/src/ui/app-render.helpers.browser.test.ts for idle, loading, sending, active run, active stream, and disconnected states
  • it stays UI-only and does not include the Slack lifecycle changes that blocked the earlier mixed-scope branch

#66395 and #74819 are the superseded contributor branches. Their latest shared head only changes ui/src/ui/app-render.helpers.ts; #74690 is the maintainer PR (aka mine) that adds the missing focused browser coverage and should remain the canonical thread for closing #65522.

@BunsDev BunsDev merged commit c34ed90 into main Apr 30, 2026
76 checks passed
@BunsDev BunsDev deleted the fix/control-ui-refresh-active-run branch April 30, 2026 06:02
@BunsDev BunsDev self-assigned this Apr 30, 2026
@Angfr95 Angfr95 mentioned this pull request Apr 30, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Apr 30, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@BunsDev BunsDev

Labels

app: web-ui App: web-ui codex maintainer Maintainer-authored PR size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Control UI: refreshing chat data during an active turn can stall the run and wedge the page/gateway

2 participants

@BunsDev @Angfr95