refactor(ChatView): consolidate button state into useReducer#11452
refactor(ChatView): consolidate button state into useReducer#11452hannesrudolph wants to merge 1 commit intomainfrom
Conversation
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
Clean refactoring. The
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
| // 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. |
There was a problem hiding this comment.
This comment references a "default case fallback" in handlePrimaryButtonClick that no longer exists -- the PR description explicitly lists its removal under "What was removed." The test itself is correct (it exercises the completion_result case), but the comment is misleading. Consider updating it to reflect the actual code path.
| // 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.
|
Closing — opened on the wrong branch. Will re-open from the correct branch. |
Summary
Replace 5 independent
useStatecalls (clineAsk,enableButtons,sendingDisabled,primaryButtonText,secondaryButtonText) with a singleuseReducerto eliminate partial-update race windows.Problem
When an
api_req_startedmessage arrives, button state is cleared via 5 separatesetStatecalls. React can render between updates, creating a window whereclineAskisundefinedbutprimaryButtonTextstill shows "Start New Task". Button clicks during this window are silently swallowed.A secondary issue: after
api_req_failed, theisStreamingheuristic can remain stale-true (because the lastapi_req_startedhas nocost), causing "Start New Task" clicks to route tocancelTaskinstead ofclearTask.Solution
useReducerwith 4 action types replaces the 5useStatecalls:SET_ASK_STATE: atomic update of all 5 fieldsSET_SENDING_DISABLED: partial update for retry delay / condensingSET_BUTTON_TEXT: partial update for subtask resumeRESET_WITHOUT_BUTTON_TEXT: resets without clearing button text (handleChatReset)api_req_failed/mistake_limit_reached/resume_taskchecks moved above theisStreamingguard inhandleSecondaryButtonClick, so authoritativeclineAskstate always wins over the heuristicWhat was removed
defaultfallback bandaid inhandlePrimaryButtonClickthat checkedprimaryButtonTextas a string proxy — no longer needed since atomic updates prevent the desyncVerification