Skip to content

refactor(ChatView): consolidate button state into useReducer#11454

Open
hannesrudolph wants to merge 1 commit intomainfrom
refactor/chatview-button-state-reducer
Open

refactor(ChatView): consolidate button state into useReducer#11454
hannesrudolph wants to merge 1 commit intomainfrom
refactor/chatview-button-state-reducer

Conversation

@hannesrudolph
Copy link
Collaborator

Summary

Replace 5 independent useState calls (clineAsk, enableButtons, sendingDisabled, primaryButtonText, secondaryButtonText) with a single useReducer to eliminate partial-update race windows.

Problem

When an api_req_started message arrives, button state is cleared via 5 separate setState calls. React can render between updates, creating a window where clineAsk is undefined but primaryButtonText still shows "Start New Task". Button clicks during this window are silently swallowed.

A secondary issue: after api_req_failed, the isStreaming heuristic can remain stale-true (because the last api_req_started has no cost), causing "Start New Task" clicks to route to cancelTask instead of clearTask.

Solution

  • useReducer with 4 action types replaces the 5 useState calls:
    • SET_ASK_STATE: atomic update of all 5 fields
    • SET_SENDING_DISABLED: partial update for retry delay / condensing
    • SET_BUTTON_TEXT: partial update for subtask resume
    • RESET_WITHOUT_BUTTON_TEXT: resets without clearing button text (handleChatReset)
  • Secondary button fix: api_req_failed / mistake_limit_reached / resume_task checks moved above the isStreaming guard in handleSecondaryButtonClick, so authoritative clineAsk state always wins over the heuristic
  • 2 new tests covering both edge cases

What was removed

  • The default fallback bandaid in handlePrimaryButtonClick that checked primaryButtonText as a string proxy — no longer needed since atomic updates prevent the desync

Verification

  • ✅ TypeScript: clean
  • ✅ Lint: clean (0 warnings)
  • ✅ Tests: 26/26 ChatView tests pass, 5496/5496 full suite pass

Replace 5 independent useState calls (clineAsk, enableButtons,
sendingDisabled, primaryButtonText, secondaryButtonText) with a single
useReducer to eliminate partial-update race windows.

The race condition: when an api_req_started message clears state via 5
separate setState calls, React can render between updates, leaving
clineAsk as undefined while button text still shows 'Start New Task'.
Clicks during this window are silently swallowed.

With useReducer, all 5 fields update atomically in a single dispatch,
making the race structurally impossible.

Also preserves the fix for stale isStreaming on secondary button clicks:
api_req_failed/mistake_limit_reached/resume_task cases are checked
before the isStreaming guard in handleSecondaryButtonClick.

4 action types:
- SET_ASK_STATE: atomic update of all 5 fields
- SET_SENDING_DISABLED: partial update for retry delay/condensing
- SET_BUTTON_TEXT: partial update for subtask resume
- RESET_WITHOUT_BUTTON_TEXT: resets without clearing button text

Tests: 26/26 pass, tsc clean, lint clean
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. bug Something isn't working labels Feb 13, 2026
@roomote
Copy link
Contributor

roomote bot commented Feb 13, 2026

Rooviewer Clock   See task

Clean refactor. The useReducer approach eliminates the partial-update race window correctly and the secondary button fix is sound. One minor comment left on a stale test comment.

  • Stale test comment references a removed default case fallback in handlePrimaryButtonClick (ChatView.spec.tsx:1366-1370)

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

Comment on lines +1366 to +1370
// Click the primary button. With clineAsk = "completion_result", the
// handler's case "completion_result" calls startNewTask(). The default
// case fallback also checks primaryButtonText against
// t("chat:startNewTask.title") for safety when clineAsk becomes undefined
// due to race conditions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment references a default case fallback in handlePrimaryButtonClick that was removed in this PR. The only reason this test passes is the completion_result case, not any default fallback. Leaving this comment in could mislead someone into thinking there is a safety net for undefined clineAsk when there isn't.

Suggested change
// Click the primary button. With clineAsk = "completion_result", the
// handler's case "completion_result" calls startNewTask(). The default
// case fallback also checks primaryButtonText against
// t("chat:startNewTask.title") for safety when clineAsk becomes undefined
// due to race conditions.
// Click the primary button. With clineAsk = "completion_result", the
// handler's case "completion_result" calls startNewTask().

Fix it with Roo Code or mention @roomote and request a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant