Skip to content

Commit 51acdec

Browse files
brandonkachenbrandonkachen
committed
refactor(cli): improve send-message hook architecture and error handling
- Extract use-message-execution.ts hook for SDK execution - Extract use-run-state-persistence.ts hook for state management - Extract agent-resolution.ts utilities (resolveAgent, buildPromptWithContext) - Add try/catch around client.run() with structured error results - Create handleExecutionFailure helper for unified error cleanup - Refactor ExecuteMessageParams into semantic groups (MessageData, StreamingContext, ExecutionContext) - Rename clearMessages to resetRunState for clarity - Fix blank AI message on failure by using updater.setError()
1 parent 198b0a4 commit 51acdec

File tree

10 files changed

+530
-165
lines changed

10 files changed

+530
-165
lines changed

‎REFACTORING_PLAN.md‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ This document outlines a prioritized refactoring plan for the 51 issues identifi
1515

1616
## Progress Tracker
1717

18-
> **Last Updated:** Wave 1 Complete
19-
> **Current Status:** Ready for Wave 2 (Track A critical path)
18+
> **Last Updated:** 2025-01-20 (Commit 2.1 Complete)
19+
> **Current Status:** Ready for Commit 2.2 (block utils + think tags)
2020
2121
### Phase 1 Progress
2222
| Commit | Description | Status | Completed By |
@@ -30,7 +30,7 @@ This document outlines a prioritized refactoring plan for the 51 issues identifi
3030
### Phase 2 Progress
3131
| Commit | Description | Status | Completed By |
3232
|--------|-------------|--------|-------------|
33-
| 2.1 | Refactor use-send-message.ts | ⬜ Not Started | - |
33+
| 2.1 | Refactor use-send-message.ts | ✅ Complete | Codebuff |
3434
| 2.2 | Consolidate block utils + think tags | ⬜ Not Started | - |
3535
| 2.3 | Refactor loopAgentSteps | ⬜ Not Started | - |
3636
| 2.4 | Consolidate billing duplication | ⬜ Not Started | - |
@@ -182,22 +182,32 @@ This document outlines a prioritized refactoring plan for the 51 issues identifi
182182

183183
> **Note:** Commit 1.5 (run-agent-step.ts) moved to Phase 2 to let chat.tsx patterns establish first.
184184
185-
### Commit 2.1: Refactor `use-send-message.ts`
185+
### Commit 2.1: Refactor `use-send-message.ts` ✅ COMPLETE
186186
**Files:** `cli/src/hooks/use-send-message.ts`
187187
**Est. Time:** 4-5 hours
188-
**Est. LOC Changed:** ~400-500
189-
190-
| Task | Description |
191-
|------|-------------|
192-
| Extract `useBashHandler` hook | Bash command handling |
193-
| Extract `useAttachmentHandler` hook | File attachment processing |
194-
| Extract `useMessageExecution` hook | Core execution logic |
195-
| Extract `useMessageErrors` hook | Error handling |
196-
| Compose in main hook | Wire up extracted hooks |
188+
**Actual Time:** ~6 hours (included additional improvements from review feedback)
189+
**Est. LOC Changed:** ~400-500
190+
**Actual LOC Changed:** 506 insertions, 151 deletions
191+
192+
| Task | Description | Status |
193+
|------|-------------|--------|
194+
| Extract `useMessageExecution` hook | SDK execution logic (client.run(), agent resolution) | ✅ |
195+
| Extract `useRunStatePersistence` hook | Run state loading/saving, chat continuation | ✅ |
196+
| Extract `agent-resolution.ts` utilities | `resolveAgent`, `buildPromptWithContext` | ✅ |
197+
| Refactor `ExecuteMessageParams` | Grouped into MessageData, StreamingContext, ExecutionContext | ✅ |
198+
| Add unified error handling | try/catch around client.run(), `handleExecutionFailure` helper | ✅ |
199+
| Rename `clearMessages` → `resetRunState` | Clearer naming | ✅ |
200+
| Fix blank AI message on failure | Use `updater.setError()` instead of separate error message | ✅ |
201+
202+
**New Files Created:**
203+
- `cli/src/hooks/use-message-execution.ts`
204+
- `cli/src/hooks/use-run-state-persistence.ts`
205+
- `cli/src/utils/agent-resolution.ts`
197206

198207
**Dependencies:** Commits 1.1a, 1.1b (chat.tsx patterns)
199208
**Risk:** Medium
200-
**Rollback:** Revert single commit
209+
**Rollback:** Revert single commit
210+
**Commit:** `e93ee30e9`
201211

202212
---
203213

‎cli/src/chat.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,7 @@ export const Chat = ({
402402
}
403403
}, [isStreaming, pendingBashMessages, setMessages])
404404

405-
const { sendMessage, clearMessages } = useSendMessage({
405+
const { sendMessage, resetRunState } = useSendMessage({
406406
inputRef,
407407
activeSubagentsRef,
408408
isChainInProgressRef,
@@ -466,7 +466,7 @@ export const Chat = ({
466466
logoutMutation,
467467
streamMessageIdRef,
468468
addToQueue,
469-
clearMessages,
469+
resetRunState,
470470
saveToHistory,
471471
scrollToLatest,
472472
sendMessage,

‎cli/src/commands/__tests__/bash-command.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ describe('bash command', () => {
3333
logoutMutation: {} as any,
3434
streamMessageIdRef: { current: null },
3535
addToQueue: mock(() => {}),
36-
clearMessages: mock(() => {}),
36+
resetRunState: mock(() => {}),
3737
saveToHistory: mock(() => {}),
3838
scrollToLatest: mock(() => {}),
3939
sendMessage: mock(async () => {}),
@@ -301,7 +301,7 @@ describe('bash command', () => {
301301
logoutMutation: {} as any,
302302
streamMessageIdRef: { current: null },
303303
addToQueue: mock(() => {}),
304-
clearMessages: mock(() => {}),
304+
resetRunState: mock(() => {}),
305305
saveToHistory: mock(() => {}),
306306
scrollToLatest: mock(() => {}),
307307
sendMessage: mock(async () => {}),

‎cli/src/commands/__tests__/command-args.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ describe('command factory pattern', () => {
3030
logoutMutation: {} as RouterParams['logoutMutation'],
3131
streamMessageIdRef: { current: null },
3232
addToQueue: mock(() => {}),
33-
clearMessages: mock(() => {}),
33+
resetRunState: mock(() => {}),
3434
saveToHistory: mock(() => {}),
3535
scrollToLatest: mock(() => {}),
3636
sendMessage: mock(async () => {}),
@@ -214,22 +214,22 @@ describe('command factory pattern', () => {
214214

215215
const sendMessage = mock(async () => {})
216216
const setMessages = mock(() => {})
217-
const clearMessages = mock(() => {})
217+
const resetRunState = mock(() => {})
218218
const setCanProcessQueue = mock(() => {})
219219

220220
const params = createMockParams({
221221
inputValue: '/new hello world',
222222
sendMessage,
223223
setMessages,
224-
clearMessages,
224+
resetRunState,
225225
setCanProcessQueue,
226226
})
227227

228228
newCmd!.handler(params, 'hello world')
229229

230230
// Should clear messages
231231
expect(setMessages).toHaveBeenCalled()
232-
expect(clearMessages).toHaveBeenCalled()
232+
expect(resetRunState).toHaveBeenCalled()
233233

234234
// Should re-enable queue and send message
235235
expect(setCanProcessQueue).toHaveBeenCalledWith(true)
@@ -245,22 +245,22 @@ describe('command factory pattern', () => {
245245

246246
const sendMessage = mock(async () => {})
247247
const setMessages = mock(() => {})
248-
const clearMessages = mock(() => {})
248+
const resetRunState = mock(() => {})
249249
const setCanProcessQueue = mock(() => {})
250250

251251
const params = createMockParams({
252252
inputValue: '/new',
253253
sendMessage,
254254
setMessages,
255-
clearMessages,
255+
resetRunState,
256256
setCanProcessQueue,
257257
})
258258

259259
newCmd!.handler(params, '')
260260

261261
// Should clear messages
262262
expect(setMessages).toHaveBeenCalled()
263-
expect(clearMessages).toHaveBeenCalled()
263+
expect(resetRunState).toHaveBeenCalled()
264264

265265
// Should disable queue and NOT send message
266266
expect(setCanProcessQueue).toHaveBeenCalledWith(false)

‎cli/src/commands/command-registry.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export type RouterParams = {
3434
logoutMutation: UseMutationResult<boolean, Error, void, unknown>
3535
streamMessageIdRef: React.MutableRefObject<string | null>
3636
addToQueue: (message: string, attachments?: PendingAttachment[]) => void
37-
clearMessages: () => void
37+
resetRunState: () => void
3838
saveToHistory: (message: string) => void
3939
scrollToLatest: () => void
4040
sendMessage: SendMessageFn
@@ -315,7 +315,7 @@ export const COMMAND_REGISTRY: CommandDefinition[] = [
315315

316316
// Clear the conversation
317317
params.setMessages(() => [])
318-
params.clearMessages()
318+
params.resetRunState()
319319
params.saveToHistory(params.inputValue.trim())
320320
clearInput(params)
321321
params.stopStreaming()

‎cli/src/hooks/helpers/send-message.ts‎

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -341,8 +341,8 @@ export const handleRunCompletion = (params: {
341341
})
342342
}
343343

344-
export const handleRunError = (params: {
345-
error: unknown
344+
export type HandleExecutionFailureParams = {
345+
errorMessage: string
346346
timerController: SendMessageTimerController
347347
updater: BatchedMessageUpdater
348348
setIsRetrying: (value: boolean) => void
@@ -351,9 +351,17 @@ export const handleRunError = (params: {
351351
updateChainInProgress: (value: boolean) => void
352352
isProcessingQueueRef?: MutableRefObject<boolean>
353353
isQueuePausedRef?: MutableRefObject<boolean>
354-
}) => {
354+
}
355+
356+
/**
357+
* Handles execution failures from executeMessage returning { success: false }.
358+
* Marks the AI message with an error, finalizes queue state, and stops the timer.
359+
*/
360+
export const handleExecutionFailure = (
361+
params: HandleExecutionFailureParams,
362+
): void => {
355363
const {
356-
error,
364+
errorMessage,
357365
timerController,
358366
updater,
359367
setIsRetrying,
@@ -364,9 +372,6 @@ export const handleRunError = (params: {
364372
isQueuePausedRef,
365373
} = params
366374

367-
const errorInfo = getErrorObject(error, { includeRawError: true })
368-
369-
logger.error({ error: errorInfo }, 'SDK client.run() failed')
370375
setIsRetrying(false)
371376
finalizeQueueState({
372377
setStreamStatus,
@@ -376,15 +381,63 @@ export const handleRunError = (params: {
376381
isQueuePausedRef,
377382
})
378383
timerController.stop('error')
384+
updater.setError(errorMessage)
385+
}
386+
387+
export const handleRunError = (params: {
388+
error: unknown
389+
timerController: SendMessageTimerController
390+
updater: BatchedMessageUpdater
391+
setIsRetrying: (value: boolean) => void
392+
setStreamStatus: (status: StreamStatus) => void
393+
setCanProcessQueue: (can: boolean) => void
394+
updateChainInProgress: (value: boolean) => void
395+
isProcessingQueueRef?: MutableRefObject<boolean>
396+
isQueuePausedRef?: MutableRefObject<boolean>
397+
}) => {
398+
const {
399+
error,
400+
timerController,
401+
updater,
402+
setIsRetrying,
403+
setStreamStatus,
404+
setCanProcessQueue,
405+
updateChainInProgress,
406+
isProcessingQueueRef,
407+
isQueuePausedRef,
408+
} = params
409+
410+
const errorInfo = getErrorObject(error, { includeRawError: true })
411+
412+
logger.error({ error: errorInfo }, 'SDK client.run() failed')
379413

380414
if (isOutOfCreditsError(error)) {
381-
updater.setError(OUT_OF_CREDITS_MESSAGE)
415+
handleExecutionFailure({
416+
errorMessage: OUT_OF_CREDITS_MESSAGE,
417+
timerController,
418+
updater,
419+
setIsRetrying,
420+
setStreamStatus,
421+
setCanProcessQueue,
422+
updateChainInProgress,
423+
isProcessingQueueRef,
424+
isQueuePausedRef,
425+
})
382426
useChatStore.getState().setInputMode('outOfCredits')
383427
invalidateActivityQuery(usageQueryKeys.current())
384428
return
385429
}
386430

387-
// Use setError for all errors so they display in UserErrorBanner consistently
388431
const errorMessage = errorInfo.message || 'An unexpected error occurred'
389-
updater.setError(errorMessage)
432+
handleExecutionFailure({
433+
errorMessage,
434+
timerController,
435+
updater,
436+
setIsRetrying,
437+
setStreamStatus,
438+
setCanProcessQueue,
439+
updateChainInProgress,
440+
isProcessingQueueRef,
441+
isQueuePausedRef,
442+
})
390443
}

0 commit comments

Comments
 (0)