refactor(frontend): 修复刷新页面后流式消息丢失的问题，修复会话页面偶发白屏的问题#654
Open
Conversation
TaskStateMachine:
- Add BUFFERING_STATES constant and isBufferingState() method to eliminate
duplicate status checks (used in 3 places)
- Add createAiMessage() factory method to unify message creation logic in
handleChatStartEvent() and applyPendingStarts()
TaskStateManager:
- Convert subscribeInit and getInitialized from arrow functions to regular
class methods
useUnifiedMessages:
- Add cleanupRecoveredTasks() to prevent unbounded growth of recoveredTasksRef
(keeps only last 50 task IDs)
MessageBubble:
- Extract copy cleanup logic into useCopyCleanup() hook with cleanCopyText()
utility function
ChatArea:
- Extract complex hasMessages useMemo into useHasMessages() hook
New files:
- frontend/src/features/tasks/hooks/useCopyCleanup.ts
- frontend/src/features/tasks/hooks/useHasMessages.ts
Contributor
📝 WalkthroughWalkthroughAdds request cancellation and configurable GET options, buffers streaming chat events for recovery, extracts message-visibility into a hook, adds clipboard-cleanup on copy, introduces an error boundary and skeleton loader, and makes initialization reactive via subscribe/getInitialized for task state. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Component
participant TaskCtx as TaskContextProvider
participant API as APIClient / Tasks API
participant StateMachine as TaskStateMachine
participant Recovery as useUnifiedMessages
participant UI as Message UI
Client->>TaskCtx: switchTask(taskId) / refreshSelectedTaskDetail()
TaskCtx->>TaskCtx: abort previous controller (if exists)
TaskCtx->>TaskCtx: create new AbortController -> signal
TaskCtx->>API: getTaskDetail(taskId, { signal })
Note right of API: may resolve or be aborted
alt API returns detail
API->>Recovery: load cached messages / dispatch sync events
Recovery->>StateMachine: SYNC_START / apply pending events
StateMachine->>StateMachine: buffer or apply chat:start / chat:chunk
StateMachine->>UI: emit reconciled messages (including applied pending starts)
else API request aborted
API->>TaskCtx: throws AbortError
TaskCtx->>StateMachine: clear selectedTaskDetail / treat as switched
StateMachine->>UI: ensure no stale detail shown
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
frontend/src/app/(tasks)/chat/page.tsx (1)
25-27: Add ARIA semantics to the spinner for better accessibility.The visual loader works, but adding
role="status"and an accessible label would improve screen-reader feedback.♿ Suggested tweak
function ChatPageSkeleton() { return ( - <div className="flex h-full w-full items-center justify-center bg-base"> - <div className="h-6 w-6 animate-spin rounded-full border-2 border-border border-t-primary" /> + <div className="flex h-full w-full items-center justify-center bg-base" role="status" aria-live="polite"> + <div + className="h-6 w-6 animate-spin rounded-full border-2 border-border border-t-primary" + aria-label="Loading chat page" + /> </div> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/`(tasks)/chat/page.tsx around lines 25 - 27, The spinner div (the element with className "h-6 w-6 animate-spin rounded-full border-2 border-border border-t-primary") needs ARIA semantics: add role="status" to that element and provide an accessible label by including a visually-hidden text node (e.g., a span with screen-reader-only styling containing "Loading…" or similar) as a child so screen readers announce the loading state; ensure the decorative borders remain aria-hidden if you add additional decorative elements.frontend/src/features/tasks/components/message/MessageBubble.tsx (1)
1307-1310: ScopeonCopyto message content blocks to improve maintainability and reduce event handler surface area.The
onCopyhandler attached at the bubble root (line 1309) successfully usespreventDefault()to prevent interference, and nested copy flows (BubbleTools, InlineMessageEdit) operate independently via explicit clipboard APIs or conditional rendering. However, narrowing scope to only the content rendering block—such as wrapping it on the<MixedContentView>parent or content render branch (around line 1397)—would reduce coupling and make future modifications safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/tasks/components/message/MessageBubble.tsx` around lines 1307 - 1310, Move the onCopy handler off the bubble root and attach it only to the actual message content renderer (e.g. the MixedContentView or the content render branch) to limit scope: remove onCopy={handleCopy} from the bubble root (the JSX that currently has className={`${bubbleBaseClasses} ${bubbleTypeClasses}`} and data-message-content="true"), then add onCopy={handleCopy} to the MixedContentView (or the content wrapper used inside the renderContent branch) so handleCopy still runs with preventDefault() for content-only copy events while BubbleTools and InlineMessageEdit keep their independent clipboard logic; keep handleTextSelection on the root and retain data-message-content on the content wrapper as needed.frontend/src/features/tasks/state/TaskStateMachine.ts (1)
668-678: Recovered block patching may temporarily reorder mixed content.This rebuild keeps only non-text blocks, then appends one synthetic text block. In mixed tool/text streams, chronology can shift until
CHAT_DONEarrives. Consider preserving original relative order while patching text recovery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/tasks/state/TaskStateMachine.ts` around lines 668 - 678, The current recovery builds recoveredBlocks by filtering out all text blocks and appending a synthetic text block (see existingBlocks, nonTextBlocks, recoveredBlocks, streamingInfo.subtask_id, recoveredMsg), which can temporarily reorder mixed tool/text content; instead, iterate existingBlocks and produce a new array that preserves original order but replaces or patches the first/streaming text block(s) in-place with the recovered content (or inserts the synthetic text at the original position of the removed text block), leaving non-text blocks untouched until CHAT_DONE; update the logic that constructs recoveredBlocks to use that ordered map/replace approach so chronology is preserved while streaming.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/apis/client.ts`:
- Around line 123-124: The get<T> method currently calls request<T>(endpoint, {
method: 'GET', ...options }) which lets callers override the HTTP method via
options.method; change the merge so the explicit GET wins (e.g., pass options
first and then method or set method after spreading) so request<T> always
receives method: 'GET' regardless of options; update the get<T> implementation
(the get function and its call to request<T>) to enforce method immutability.
In `@frontend/src/app/`(tasks)/chat/error.tsx:
- Around line 33-35: The retry Button used in the error UI doesn't enforce the
44×44 mobile tap target; update the Button element (the one with props
variant="primary" and onClick={reset} rendering {t('actions.retry')}) to include
the recommended sizing utility classes (e.g., h-11 and min-w-[44px]) so it meets
the mobile minimum tap target requirement.
In `@frontend/src/features/tasks/components/chat/ChatArea.tsx`:
- Around line 411-417: The UI mismatch comes from render using hasMessages (from
useHasMessages which includes streaming/pending) while useScrollManagement and
useFloatingInput still consume hasMessagesForHooks (only selectedTaskDetail +
message map); unify them by creating and using a single visibility source (e.g.,
hasMessagesVisible) and pass that same value to both render and hooks. Modify
the call sites so useHasMessages (or a small adapter) returns the canonical
visibility boolean and replace uses of hasMessagesForHooks in
useScrollManagement and useFloatingInput calls with that single
hasMessagesVisible (or update useScrollManagement/useFloatingInput to accept an
explicit visibility flag derived from useHasMessages), ensuring references to
selectedTask, selectedTaskDetail, taskState, streamHandlers and message map are
preserved when computing the unified value.
In `@frontend/src/features/tasks/contexts/taskContext.tsx`:
- Around line 737-739: When selectedTask becomes falsy you currently call
setSelectedTaskDetail(null) without aborting any ongoing detail fetch, allowing
a prior fetch to later overwrite cleared state; fix this by calling abort() on
the AbortController used for task detail fetches (the controller/ref used in the
task detail effect — e.g., detailAbortController or abortControllerRef) before
clearing state and also aborting the previous controller in the task-switch
path, then create a new controller before starting any new fetch so stale
responses cannot overwrite setSelectedTaskDetail; apply these changes around the
code that references selectedTask and setSelectedTaskDetail.
In `@frontend/src/features/tasks/hooks/useCopyCleanup.ts`:
- Around line 12-14: cleanCopyText currently collapses all multiple newlines and
trims both ends, which removes meaningful paragraph/code spacing; change it so
it only strips accidental wrapper noise (leading/trailing blank lines and
surrounding whitespace) while preserving internal multi-newline spacing. Update
the cleanCopyText implementation to remove leading and trailing blank lines
(e.g., via a regex that targets /^\s*\n+|\n+\s*$/) and then optionally trim only
the overall ends, but do not collapse internal consecutive newlines so
paragraphs and code blocks remain intact.
In `@frontend/src/features/tasks/hooks/useTaskStateMachine.ts`:
- Around line 40-43: The code passes unbound instance methods
taskStateManager.subscribeInit and taskStateManager.getInitialized into
useSyncExternalStore, causing `this` to be lost; fix by passing bound functions
(e.g., bind subscribeInit and getInitialized to taskStateManager or wrap them in
arrow functions that call the methods on taskStateManager) so that
useSyncExternalStore receives properly-bound callbacks (ensure the same change
is applied for the subscriber, getter, and the SSR snapshot argument if
applicable) and reference the methods by name (subscribeInit, getInitialized)
when making the change.
In `@frontend/src/features/tasks/hooks/useUnifiedMessages.ts`:
- Around line 239-246: The code marks a task as recovered
(recoveredTasksRef.current.add(effectiveTaskId)) before calling recover(), so if
recover() rejects the task will be incorrectly considered recovered; change the
flow in the effect so you call recover() first and only add effectiveTaskId to
recoveredTasksRef (and call cleanupRecoveredTasks()) after recover() resolves
successfully, and ensure errors from recover() are caught (e.g., try/catch or
.catch) so failed recoveries do not add the id; update references in this block
(needsInitialRecovery, recoveredTasksRef, cleanupRecoveredTasks, recover,
effectiveTaskId) accordingly.
In `@frontend/src/features/tasks/state/TaskStateMachine.ts`:
- Around line 705-707: applyPendingStarts() can leave the state inconsistent by
creating a streaming AI message while leaving status === 'ready' and
streamingSubtaskId === null; after calling this.applyPendingStarts() (and in the
other spots that update this.messages), inspect the updated this.messages for
any active streaming AI message and set this.status = 'streaming' and
this.streamingSubtaskId = <that message's subtaskId>, or clear
streamingSubtaskId and set status to 'ready' if none found; adjust the
message-update branch that assigns this.messages to perform this consistency fix
so status and streamingSubtaskId always reflect the actual messages.
- Line 220: BUFFERING_STATES currently includes 'idle', which causes chunks to
be buffered before recover() populates cached_content and later duplicated by
applyPendingChunks(); to fix, remove 'idle' from the
TaskStateMachine.BUFFERING_STATES array so chunks are not queued while waiting
for recovery, and also add a guard in applyPendingChunks() to skip appending
pendingChunks whose content is already present in cached_content (or clear
pendingChunks after recover() completes) — reference BUFFERING_STATES,
recover(), cached_content, pendingChunks, and applyPendingChunks() when making
the change.
In `@frontend/src/features/tasks/state/TaskStateManager.ts`:
- Around line 35-37: The forEach call that invokes init listeners uses an
expression-bodied arrow (this.initListeners.forEach(listener => listener()))
which triggers the lint rule; change it to a block-bodied callback so the return
value isn't used (e.g., this.initListeners.forEach(listener => { listener();
})), updating the invocation site in TaskStateManager where initListeners and
its forEach are defined/used.
---
Nitpick comments:
In `@frontend/src/app/`(tasks)/chat/page.tsx:
- Around line 25-27: The spinner div (the element with className "h-6 w-6
animate-spin rounded-full border-2 border-border border-t-primary") needs ARIA
semantics: add role="status" to that element and provide an accessible label by
including a visually-hidden text node (e.g., a span with screen-reader-only
styling containing "Loading…" or similar) as a child so screen readers announce
the loading state; ensure the decorative borders remain aria-hidden if you add
additional decorative elements.
In `@frontend/src/features/tasks/components/message/MessageBubble.tsx`:
- Around line 1307-1310: Move the onCopy handler off the bubble root and attach
it only to the actual message content renderer (e.g. the MixedContentView or the
content render branch) to limit scope: remove onCopy={handleCopy} from the
bubble root (the JSX that currently has className={`${bubbleBaseClasses}
${bubbleTypeClasses}`} and data-message-content="true"), then add
onCopy={handleCopy} to the MixedContentView (or the content wrapper used inside
the renderContent branch) so handleCopy still runs with preventDefault() for
content-only copy events while BubbleTools and InlineMessageEdit keep their
independent clipboard logic; keep handleTextSelection on the root and retain
data-message-content on the content wrapper as needed.
In `@frontend/src/features/tasks/state/TaskStateMachine.ts`:
- Around line 668-678: The current recovery builds recoveredBlocks by filtering
out all text blocks and appending a synthetic text block (see existingBlocks,
nonTextBlocks, recoveredBlocks, streamingInfo.subtask_id, recoveredMsg), which
can temporarily reorder mixed tool/text content; instead, iterate existingBlocks
and produce a new array that preserves original order but replaces or patches
the first/streaming text block(s) in-place with the recovered content (or
inserts the synthetic text at the original position of the removed text block),
leaving non-text blocks untouched until CHAT_DONE; update the logic that
constructs recoveredBlocks to use that ordered map/replace approach so
chronology is preserved while streaming.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/src/apis/client.tsfrontend/src/apis/tasks.tsfrontend/src/app/(tasks)/chat/error.tsxfrontend/src/app/(tasks)/chat/page.tsxfrontend/src/features/tasks/components/chat/ChatArea.tsxfrontend/src/features/tasks/components/message/MessageBubble.tsxfrontend/src/features/tasks/contexts/taskContext.tsxfrontend/src/features/tasks/hooks/useCopyCleanup.tsfrontend/src/features/tasks/hooks/useHasMessages.tsfrontend/src/features/tasks/hooks/useTaskStateMachine.tsfrontend/src/features/tasks/hooks/useUnifiedMessages.tsfrontend/src/features/tasks/state/TaskStateMachine.tsfrontend/src/features/tasks/state/TaskStateManager.ts
Comment on lines
+123
to
+124
| async get<T>(endpoint: string, options?: RequestInit): Promise<T> { | ||
| return this.request<T>(endpoint, { method: 'GET', ...options }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the actual current state of the get() method at lines 123-124
cat -n frontend/src/apis/client.ts | sed -n '120,130p'Repository: wecode-ai/Wegent
Length of output: 468
🏁 Script executed:
# Search for all callsites of apiClient.get with method in options
rg --type=ts --type=tsx 'apiClient\.get\s*\(' -A 3 | head -100Repository: wecode-ai/Wegent
Length of output: 87
🏁 Script executed:
# Also check for direct usage of this.request and get patterns
rg --type=ts 'method.*GET' frontend/src/apis/client.ts -B 2 -A 2Repository: wecode-ai/Wegent
Length of output: 203
🏁 Script executed:
# Search for all get() calls in the codebase
rg 'apiClient\.get\(' --type=ts -B 1 -A 3 | head -150Repository: wecode-ai/Wegent
Length of output: 8059
🏁 Script executed:
# Also search for imports and usage patterns of the APIClient class
rg 'new APIClient\|import.*APIClient' --type=ts -B 1 -A 3Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
# Check if there are any tests for the get method
fd -e test.ts -e spec.ts -e test.js -e spec.js | xargs rg -l "\.get\(" | head -5Repository: wecode-ai/Wegent
Length of output: 258
🏁 Script executed:
# Verify the actual behavior of spread ordering in JavaScript objects to be thorough
python3 << 'EOF'
# Demonstrate that last property definition wins in object spread
test1 = {'method': 'GET'}
test2 = {'signal': 'abort_signal'}
merged = {**test1, **test2}
print(f"Test 1 (method first): {merged}")
merged2 = {**test2, **test1}
print(f"Test 2 (method last): {merged2}")
# Simulate the vulnerability
request_init = {'method': 'GET'}
options = {'method': 'POST', 'signal': 'abort_signal'}
result = {**request_init, **options}
print(f"Vulnerability test (options override): {result}")
EOFRepository: wecode-ai/Wegent
Length of output: 276
Prevent get() callers from overriding the HTTP method.
At line 124, { method: 'GET', ...options } allows options.method to silently change this into a non-GET request. While no current callsite exploits this, it breaks the semantic contract of the method.
🔧 Proposed fix
- async get<T>(endpoint: string, options?: RequestInit): Promise<T> {
- return this.request<T>(endpoint, { method: 'GET', ...options })
+ async get<T>(endpoint: string, options?: Omit<RequestInit, 'method' | 'body'>): Promise<T> {
+ return this.request<T>(endpoint, { ...options, method: 'GET' })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/apis/client.ts` around lines 123 - 124, The get<T> method
currently calls request<T>(endpoint, { method: 'GET', ...options }) which lets
callers override the HTTP method via options.method; change the merge so the
explicit GET wins (e.g., pass options first and then method or set method after
spreading) so request<T> always receives method: 'GET' regardless of options;
update the get<T> implementation (the get function and its call to request<T>)
to enforce method immutability.
Comment on lines
+33
to
+35
| <Button variant="primary" onClick={reset}> | ||
| {t('actions.retry')} | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Ensure retry button meets mobile tap-target size requirement.
At Line [33], the button does not explicitly enforce the required 44×44 minimum target size.
📱 Suggested fix
- <Button variant="primary" onClick={reset}>
+ <Button variant="primary" onClick={reset} className="h-11 min-w-[44px]">
{t('actions.retry')}
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button variant="primary" onClick={reset}> | |
| {t('actions.retry')} | |
| </Button> | |
| <Button variant="primary" onClick={reset} className="h-11 min-w-[44px]"> | |
| {t('actions.retry')} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/`(tasks)/chat/error.tsx around lines 33 - 35, The retry
Button used in the error UI doesn't enforce the 44×44 mobile tap target; update
the Button element (the one with props variant="primary" and onClick={reset}
rendering {t('actions.retry')}) to include the recommended sizing utility
classes (e.g., h-11 and min-w-[44px]) so it meets the mobile minimum tap target
requirement.
Comment on lines
+411
to
+417
| // Determine if there are messages to display | ||
| const hasMessages = useHasMessages({ | ||
| selectedTask, | ||
| selectedTaskDetail, | ||
| streamHandlers.hasPendingUserMessage, | ||
| streamHandlers.isStreaming, | ||
| streamHandlers.pendingTaskId, | ||
| streamHandlers.localPendingMessage, | ||
| taskState?.messages, | ||
| ]) | ||
| taskState, | ||
| streamHandlers, | ||
| }) |
Contributor
There was a problem hiding this comment.
hasMessages now diverges from the visibility value used by layout/scroll hooks.
useHasMessages includes streaming/pending states, but useScrollManagement and useFloatingInput still rely on hasMessagesForHooks (selected task detail + message map only). This can produce UI state mismatch (render says “has messages”, hooks behave as “no messages”) during early stream/pending phases.
Please make render + hooks consume one consistent visibility source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/components/chat/ChatArea.tsx` around lines 411 -
417, The UI mismatch comes from render using hasMessages (from useHasMessages
which includes streaming/pending) while useScrollManagement and useFloatingInput
still consume hasMessagesForHooks (only selectedTaskDetail + message map); unify
them by creating and using a single visibility source (e.g., hasMessagesVisible)
and pass that same value to both render and hooks. Modify the call sites so
useHasMessages (or a small adapter) returns the canonical visibility boolean and
replace uses of hasMessagesForHooks in useScrollManagement and useFloatingInput
calls with that single hasMessagesVisible (or update
useScrollManagement/useFloatingInput to accept an explicit visibility flag
derived from useHasMessages), ensuring references to selectedTask,
selectedTaskDetail, taskState, streamHandlers and message map are preserved when
computing the unified value.
Comment on lines
+737
to
739
| // Clear stale detail immediately so downstream hooks don't operate on wrong task data | ||
| setSelectedTaskDetail(null) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get overall file size and structure
wc -l frontend/src/features/tasks/contexts/taskContext.tsx
echo "---"
# Find all abort and setSelectedTaskDetail patterns
rg -n 'abortControllerRef|setSelectedTaskDetail' frontend/src/features/tasks/contexts/taskContext.tsxRepository: wecode-ai/Wegent
Length of output: 670
Abort in-flight detail fetch when deselecting task or clearing selection.
When selectedTask becomes null (or falsy), the else branch executes setSelectedTaskDetail(null) at line 738 without aborting any in-flight request from the previous task. An earlier fetch can still resolve after this line and overwrite the cleared state with stale data.
Abort the controller before clearing detail in both the task-switch path and the deselect-to-null path:
🔧 Proposed fix
if (previousTaskId !== null && previousTaskId !== currentTaskId) {
+ if (abortControllerRef.current) {
+ abortControllerRef.current.abort()
+ abortControllerRef.current = null
+ }
leaveTask(previousTaskId)
// Clear stale detail immediately so downstream hooks don't operate on wrong task data
setSelectedTaskDetail(null)
}
@@
if (selectedTask) {
@@
} else {
+ if (abortControllerRef.current) {
+ abortControllerRef.current.abort()
+ abortControllerRef.current = null
+ }
setSelectedTaskDetail(null)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/contexts/taskContext.tsx` around lines 737 - 739,
When selectedTask becomes falsy you currently call setSelectedTaskDetail(null)
without aborting any ongoing detail fetch, allowing a prior fetch to later
overwrite cleared state; fix this by calling abort() on the AbortController used
for task detail fetches (the controller/ref used in the task detail effect —
e.g., detailAbortController or abortControllerRef) before clearing state and
also aborting the previous controller in the task-switch path, then create a new
controller before starting any new fetch so stale responses cannot overwrite
setSelectedTaskDetail; apply these changes around the code that references
selectedTask and setSelectedTaskDetail.
Comment on lines
+12
to
+14
| export function cleanCopyText(text: string): string { | ||
| return text.replace(/\n{2,}/g, '\n').trim() | ||
| } |
Contributor
There was a problem hiding this comment.
cleanCopyText currently removes meaningful formatting.
Line 13 collapses all multi-newline spacing and trims both ends, which can degrade copied markdown/code readability. Keep intentional paragraph/code spacing while still removing accidental wrapper noise.
Proposed fix
export function cleanCopyText(text: string): string {
- return text.replace(/\n{2,}/g, '\n').trim()
+ return text
+ .replace(/\r\n/g, '\n')
+ // Keep one blank line between blocks; only collapse excessive gaps
+ .replace(/\n{3,}/g, '\n\n')
+ // Remove only boundary newlines introduced by wrapper elements
+ .replace(/^\n+|\n+$/g, '')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/hooks/useCopyCleanup.ts` around lines 12 - 14,
cleanCopyText currently collapses all multiple newlines and trims both ends,
which removes meaningful paragraph/code spacing; change it so it only strips
accidental wrapper noise (leading/trailing blank lines and surrounding
whitespace) while preserving internal multi-newline spacing. Update the
cleanCopyText implementation to remove leading and trailing blank lines (e.g.,
via a regex that targets /^\s*\n+|\n+\s*$/) and then optionally trim only the
overall ends, but do not collapse internal consecutive newlines so paragraphs
and code blocks remain intact.
Comment on lines
+40
to
+43
| const isInitialized = useSyncExternalStore( | ||
| taskStateManager.subscribeInit, | ||
| taskStateManager.getInitialized, | ||
| // SSR snapshot: always false on server |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1. Find and read the TaskStateManager.ts file to see method definitions
echo "=== TaskStateManager.ts method definitions ==="
rg -nP --type=ts -A5 '(subscribeInit|getInitialized)' frontend/src/features/tasks/state/TaskStateManager.ts | head -50
# 2. Verify the actual callsite in useTaskStateMachine.ts
echo ""
echo "=== useTaskStateMachine.ts useSyncExternalStore usage ==="
rg -nP --type=ts -B2 -A6 'useSyncExternalStore' frontend/src/features/tasks/hooks/useTaskStateMachine.ts
# 3. Check if TaskStateManager is a class and how subscribeInit/getInitialized are defined
echo ""
echo "=== TaskStateManager class structure ==="
rg -nP --type=ts -B5 'class TaskStateManager' frontend/src/features/tasks/state/TaskStateManager.ts | head -20Repository: wecode-ai/Wegent
Length of output: 1372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for other usages of these methods to see if this is the only problem site
echo "=== Other usages of taskStateManager.subscribeInit and getInitialized ==="
rg -nP --type=ts 'taskStateManager\.(subscribeInit|getInitialized)' frontend/src/features/tasks
echo ""
echo "=== Verify taskStateManager is an instance (singleton) ==="
rg -nP --type=ts 'export.*taskStateManager' frontend/src/features/tasks/stateRepository: wecode-ai/Wegent
Length of output: 603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that subscribeInit actually uses 'this' context
echo "=== Full subscribeInit method body ==="
rg -nP --type=ts -A8 'subscribeInit\(listener' frontend/src/features/tasks/state/TaskStateManager.ts
echo ""
echo "=== Check if any of these methods are arrow functions ==="
rg -nP --type=ts '(subscribeInit|getInitialized)\s*=' frontend/src/features/tasks/state/TaskStateManager.ts
echo ""
echo "=== Verify this.initListeners and this.deps exist ==="
rg -nP --type=ts '(this\.initListeners|this\.deps)\s*' frontend/src/features/tasks/state/TaskStateManager.ts | head -10Repository: wecode-ai/Wegent
Length of output: 417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full method signatures and their dependencies on 'this'
echo "=== TaskStateManager class fields ==="
rg -nP --type=ts -B2 'class TaskStateManagerImpl' frontend/src/features/tasks/state/TaskStateManager.ts | head -30
echo ""
echo "=== getInitialized full method ==="
rg -nP --type=ts -A3 'getInitialized\(\)' frontend/src/features/tasks/state/TaskStateManager.ts
echo ""
echo "=== Check where initListeners and deps are initialized ==="
rg -nP --type=ts '(this\.initListeners|this\.deps)\s*=' frontend/src/features/tasks/state/TaskStateManager.ts | head -5Repository: wecode-ai/Wegent
Length of output: 411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's verify how useSyncExternalStore calls these functions to understand the binding issue
echo "=== React useSyncExternalStore API check ==="
echo "useSyncExternalStore signature: subscribe(listener), getSnapshot(), getServerSnapshot()"
echo ""
echo "The issue is that when you pass taskStateManager.subscribeInit directly,"
echo "React will call it as: subscribeInit(listener) without 'this' context"
echo ""
echo "Let's verify the method needs 'this' by checking all 'this' usages:"
rg -nP --type=ts 'this\.(initListeners|deps)' frontend/src/features/tasks/state/TaskStateManager.tsRepository: wecode-ai/Wegent
Length of output: 726
Bind TaskStateManager methods before passing to useSyncExternalStore
Lines 41–42 pass unbound instance methods. Both subscribeInit and getInitialized depend on this context (this.initListeners and this.deps). When React invokes them without the instance, this becomes undefined, causing a runtime error.
Proposed fix
const isInitialized = useSyncExternalStore(
- taskStateManager.subscribeInit,
- taskStateManager.getInitialized,
+ listener => taskStateManager.subscribeInit(listener),
+ () => taskStateManager.getInitialized(),
// SSR snapshot: always false on server
() => false
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isInitialized = useSyncExternalStore( | |
| taskStateManager.subscribeInit, | |
| taskStateManager.getInitialized, | |
| // SSR snapshot: always false on server | |
| const isInitialized = useSyncExternalStore( | |
| listener => taskStateManager.subscribeInit(listener), | |
| () => taskStateManager.getInitialized(), | |
| // SSR snapshot: always false on server |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/hooks/useTaskStateMachine.ts` around lines 40 -
43, The code passes unbound instance methods taskStateManager.subscribeInit and
taskStateManager.getInitialized into useSyncExternalStore, causing `this` to be
lost; fix by passing bound functions (e.g., bind subscribeInit and
getInitialized to taskStateManager or wrap them in arrow functions that call the
methods on taskStateManager) so that useSyncExternalStore receives
properly-bound callbacks (ensure the same change is applied for the subscriber,
getter, and the SSR snapshot argument if applicable) and reference the methods
by name (subscribeInit, getInitialized) when making the change.
Comment on lines
+239
to
246
| const needsInitialRecovery = !recoveredTasksRef.current.has(effectiveTaskId) | ||
| if (needsInitialRecovery || !isStreaming) { | ||
| recoveredTasksRef.current.add(effectiveTaskId) | ||
| cleanupRecoveredTasks() | ||
| recover() | ||
| } | ||
| } | ||
| }, [effectiveTaskId, isInitialized, recover, isStreaming]) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the code section in question
nl -ba frontend/src/features/tasks/hooks/useUnifiedMessages.ts | sed -n '235,250p'Repository: wecode-ai/Wegent
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Look for the recover function definition and its return type
rg -A 10 "const recover\s*=" frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the useEffect dependency array and surrounding context
nl -ba frontend/src/features/tasks/hooks/useUnifiedMessages.ts | sed -n '220,260p'Repository: wecode-ai/Wegent
Length of output: 100
🏁 Script executed:
#!/bin/bash
# View the file around the problematic lines using cat and sed
sed -n '235,250p' frontend/src/features/tasks/hooks/useUnifiedMessages.ts | cat -nRepository: wecode-ai/Wegent
Length of output: 811
🏁 Script executed:
#!/bin/bash
# Find and display the recover function definition
rg -B 2 -A 15 "const recover\s*=|function recover" frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for where cleanupRecoveredTasks is defined
rg -B 2 -A 5 "const cleanupRecoveredTasks" frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 512
🏁 Script executed:
#!/bin/bash
# Search for recover function more broadly
rg "recover\s*=" frontend/src/features/tasks/hooks/useUnifiedMessages.ts | head -20Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look at the entire useEffect hook context
sed -n '220,250p' frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 1449
🏁 Script executed:
#!/bin/bash
# Check if cleanupRecoveredTasks is used in dependency arrays elsewhere
rg "cleanupRecoveredTasks" frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Find recover function - check the full file for its definition
rg -B 5 -A 20 "recover.*=.*useCallback|recover.*=.*async|const recover" frontend/src/features/tasks/hooks/useUnifiedMessages.ts | head -50Repository: wecode-ai/Wegent
Length of output: 1283
🏁 Script executed:
#!/bin/bash
# Check the entire file structure to understand where recover comes from
rg -n "recover" frontend/src/features/tasks/hooks/useUnifiedMessages.ts | grep -v "recoveredTasks"Repository: wecode-ai/Wegent
Length of output: 1085
🏁 Script executed:
#!/bin/bash
# View more of the file to see if recover is imported or defined as a parameter
head -100 frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 4013
🏁 Script executed:
#!/bin/bash
# Look at useTaskStateMachine to understand recover() return type
rg -B 5 -A 20 "recover\s*:" frontend/src/features/tasks/hooks/useTaskStateMachine.ts | head -60Repository: wecode-ai/Wegent
Length of output: 920
🏁 Script executed:
#!/bin/bash
# Search for recover function definition in useTaskStateMachine
rg -B 3 -A 10 "const recover\s*=|recover\s*=" frontend/src/features/tasks/hooks/useTaskStateMachine.ts | head -80Repository: wecode-ai/Wegent
Length of output: 466
🏁 Script executed:
#!/bin/bash
# Check if recover is async or returns a promise
rg -B 2 -A 15 "recover.*async|return.*promise|Promise<" frontend/src/features/tasks/hooks/useTaskStateMachine.ts | head -60Repository: wecode-ai/Wegent
Length of output: 689
🏁 Script executed:
#!/bin/bash
# Check for other usages of recoveredTasksRef to ensure nothing depends on immediate marking
rg "recoveredTasksRef" frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 379
🏁 Script executed:
#!/bin/bash
# Search for tests related to this recover functionality
find frontend -name "*.test.*" -o -name "*.spec.*" | xargs rg -l "useUnifiedMessages|recover" 2>/dev/null | head -10Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check what happens on recover() rejection - is there error handling elsewhere?
rg -B 5 -A 5 "recover\(\)" frontend/src/features/tasks/hooks/useUnifiedMessages.tsRepository: wecode-ai/Wegent
Length of output: 1565
Mark task as recovered only after recover() succeeds.
Task is marked recovered on line 242 before the recover() call on line 244 completes. If recover() rejects, the task remains marked in recoveredTasksRef and won't be retried on subsequent effect runs, breaking the intended recovery retry mechanism during streaming.
Suggested fix
- if (needsInitialRecovery || !isStreaming) {
- recoveredTasksRef.current.add(effectiveTaskId)
- cleanupRecoveredTasks()
- recover()
- }
+ if (needsInitialRecovery || !isStreaming) {
+ void recover()
+ .then(() => {
+ recoveredTasksRef.current.add(effectiveTaskId)
+ cleanupRecoveredTasks()
+ })
+ .catch(() => undefined)
+ }
}
- }, [effectiveTaskId, isInitialized, recover, isStreaming])
+ }, [effectiveTaskId, isInitialized, recover, isStreaming, cleanupRecoveredTasks])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const needsInitialRecovery = !recoveredTasksRef.current.has(effectiveTaskId) | |
| if (needsInitialRecovery || !isStreaming) { | |
| recoveredTasksRef.current.add(effectiveTaskId) | |
| cleanupRecoveredTasks() | |
| recover() | |
| } | |
| } | |
| }, [effectiveTaskId, isInitialized, recover, isStreaming]) | |
| const needsInitialRecovery = !recoveredTasksRef.current.has(effectiveTaskId) | |
| if (needsInitialRecovery || !isStreaming) { | |
| void recover() | |
| .then(() => { | |
| recoveredTasksRef.current.add(effectiveTaskId) | |
| cleanupRecoveredTasks() | |
| }) | |
| .catch(() => undefined) | |
| } | |
| } | |
| }, [effectiveTaskId, isInitialized, recover, isStreaming, cleanupRecoveredTasks]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/hooks/useUnifiedMessages.ts` around lines 239 -
246, The code marks a task as recovered
(recoveredTasksRef.current.add(effectiveTaskId)) before calling recover(), so if
recover() rejects the task will be incorrectly considered recovered; change the
flow in the effect so you call recover() first and only add effectiveTaskId to
recoveredTasksRef (and call cleanupRecoveredTasks()) after recover() resolves
successfully, and ensure errors from recover() are caught (e.g., try/catch or
.catch) so failed recoveries do not add the id; update references in this block
(needsInitialRecovery, recoveredTasksRef, cleanupRecoveredTasks, recover,
effectiveTaskId) accordingly.
| private pendingStarts: Array<{ subtaskId: number; shellType?: string; messageId?: number }> = [] | ||
|
|
||
| // States where events should be queued instead of processed immediately | ||
| private static readonly BUFFERING_STATES: readonly TaskStatus[] = ['idle', 'joining', 'syncing'] |
Contributor
There was a problem hiding this comment.
Buffering chunks in idle can duplicate recovered stream content.
Line 220 includes idle in buffering states, so Line 1137 queues chunks before recovery. Later, applyPendingChunks() appends them unconditionally (Line 763), even when cached_content from recover() already includes those tokens.
💡 Suggested fix
- private static readonly BUFFERING_STATES: readonly TaskStatus[] = ['idle', 'joining', 'syncing']
+ private static readonly START_BUFFERING_STATES: readonly TaskStatus[] = ['idle', 'joining', 'syncing']
+ private static readonly CHUNK_BUFFERING_STATES: readonly TaskStatus[] = ['joining', 'syncing']
- private isBufferingState(): boolean {
- return TaskStateMachine.BUFFERING_STATES.includes(this.state.status)
+ private isStartBufferingState(): boolean {
+ return TaskStateMachine.START_BUFFERING_STATES.includes(this.state.status)
+ }
+
+ private isChunkBufferingState(): boolean {
+ return TaskStateMachine.CHUNK_BUFFERING_STATES.includes(this.state.status)
}
- if (this.isBufferingState()) {
+ if (this.isStartBufferingState()) {
// queue CHAT_START
}
- if (this.isBufferingState()) {
+ if (this.isChunkBufferingState()) {
// queue CHAT_CHUNK
}Also applies to: 1133-1145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/state/TaskStateMachine.ts` at line 220,
BUFFERING_STATES currently includes 'idle', which causes chunks to be buffered
before recover() populates cached_content and later duplicated by
applyPendingChunks(); to fix, remove 'idle' from the
TaskStateMachine.BUFFERING_STATES array so chunks are not queued while waiting
for recovery, and also add a guard in applyPendingChunks() to skip appending
pendingChunks whose content is already present in cached_content (or clear
pendingChunks after recover() completes) — reference BUFFERING_STATES,
recover(), cached_content, pendingChunks, and applyPendingChunks() when making
the change.
Comment on lines
+705
to
+707
| // Apply pending start events and chunks that were queued during sync | ||
| // This ensures events received during joining/syncing are not lost | ||
| this.applyPendingStarts() |
Contributor
There was a problem hiding this comment.
applyPendingStarts() can leave an active stream in ready state.
After Line 700/Line 702, Line 732 only updates messages. If pending starts create a new streaming AI message, status can remain ready with streamingSubtaskId = null, which makes state inconsistent.
💡 Suggested fix
private applyPendingStarts(): void {
if (this.pendingStarts.length === 0) return
const newMessages = new Map(this.state.messages)
+ let lastStartedSubtaskId: number | null = null
for (const start of this.pendingStarts) {
const aiMessageId = generateMessageId('ai', start.subtaskId)
if (newMessages.has(aiMessageId)) {
// Message already exists from buildMessages/cached_content, skip
continue
}
newMessages.set(aiMessageId, this.createAiMessage(start))
+ lastStartedSubtaskId = start.subtaskId
}
- this.state = { ...this.state, messages: newMessages }
+ this.state = {
+ ...this.state,
+ messages: newMessages,
+ status: lastStartedSubtaskId !== null ? 'streaming' : this.state.status,
+ streamingSubtaskId:
+ lastStartedSubtaskId !== null ? lastStartedSubtaskId : this.state.streamingSubtaskId,
+ }
this.pendingStarts = []
}Also applies to: 720-734
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/state/TaskStateMachine.ts` around lines 705 -
707, applyPendingStarts() can leave the state inconsistent by creating a
streaming AI message while leaving status === 'ready' and streamingSubtaskId ===
null; after calling this.applyPendingStarts() (and in the other spots that
update this.messages), inspect the updated this.messages for any active
streaming AI message and set this.status = 'streaming' and
this.streamingSubtaskId = <that message's subtaskId>, or clear
streamingSubtaskId and set status to 'ready' if none found; adjust the
message-update branch that assigns this.messages to perform this consistency fix
so status and streamingSubtaskId always reflect the actual messages.
Comment on lines
+35
to
37
| // Notify init listeners so useSyncExternalStore picks up the change | ||
| this.initListeners.forEach(listener => listener()) | ||
| } |
Contributor
There was a problem hiding this comment.
Fix Biome lint error in init-listener iteration
Line 36 uses an expression-bodied callback inside forEach, which triggers lint/suspicious/useIterableCallbackReturn. Switch to a block-bodied callback.
Proposed fix
- this.initListeners.forEach(listener => listener())
+ this.initListeners.forEach(listener => {
+ listener()
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Notify init listeners so useSyncExternalStore picks up the change | |
| this.initListeners.forEach(listener => listener()) | |
| } | |
| // Notify init listeners so useSyncExternalStore picks up the change | |
| this.initListeners.forEach(listener => { | |
| listener() | |
| }) | |
| } |
🧰 Tools
🪛 Biome (2.4.4)
[error] 36-36: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/state/TaskStateManager.ts` around lines 35 - 37,
The forEach call that invokes init listeners uses an expression-bodied arrow
(this.initListeners.forEach(listener => listener())) which triggers the lint
rule; change it to a block-bodied callback so the return value isn't used (e.g.,
this.initListeners.forEach(listener => { listener(); })), updating the
invocation site in TaskStateManager where initListeners and its forEach are
defined/used.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
frontend/src/features/tasks/hooks/useUnifiedMessages.ts (1)
245-253:⚠️ Potential issue | 🟠 MajorMark the task as recovered only after
recover()succeeds.Line 248 records the task as recovered before Line 250 finishes. If
recover()rejects, this task will be skipped on later retries even though recovery never completed.Suggested fix
if (effectiveTaskId > 0) { const needsInitialRecovery = !recoveredTasksRef.current.has(effectiveTaskId) if (needsInitialRecovery || !isStreaming) { - recoveredTasksRef.current.add(effectiveTaskId) - cleanupRecoveredTasks() - recover() + void recover() + .then(() => { + recoveredTasksRef.current.add(effectiveTaskId) + cleanupRecoveredTasks() + }) + .catch(() => undefined) } } - }, [effectiveTaskId, isInitialized, recover, isStreaming]) + }, [effectiveTaskId, isInitialized, recover, isStreaming, cleanupRecoveredTasks])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/tasks/hooks/useUnifiedMessages.ts` around lines 245 - 253, Currently the code calls recoveredTasksRef.current.add(effectiveTaskId) before calling recover(), which means a failed recover() will still mark the task as recovered; change the flow in useUnifiedMessages so you await recover() inside a try/catch and only call recoveredTasksRef.current.add(effectiveTaskId) after recover() resolves successfully (call cleanupRecoveredTasks() as appropriate either before or after the successful mark), and handle/rethrow or log errors in the catch so failed recoveries aren't marked; reference recoveredTasksRef, effectiveTaskId, recover, cleanupRecoveredTasks, and isStreaming when making the change.frontend/src/features/tasks/state/TaskStateMachine.ts (2)
727-741:⚠️ Potential issue | 🟠 Major
applyPendingStarts()can leave state inconsistent with streaming message.After
SYNC_DONEsetsstatus: 'ready'andstreamingSubtaskId: null(line 467),applyPendingStarts()creates streaming AI messages but only updatesmessageswithout transitioning state to'streaming'. This results in an inconsistent state where a streaming message exists butstatus === 'ready'.🔧 Proposed fix
private applyPendingStarts(): void { if (this.pendingStarts.length === 0) return const newMessages = new Map(this.state.messages) + let lastStartedSubtaskId: number | null = null for (const start of this.pendingStarts) { const aiMessageId = generateMessageId('ai', start.subtaskId) if (newMessages.has(aiMessageId)) { // Message already exists from buildMessages/cached_content, skip continue } newMessages.set(aiMessageId, this.createAiMessage(start)) + lastStartedSubtaskId = start.subtaskId } - this.state = { ...this.state, messages: newMessages } + this.state = { + ...this.state, + messages: newMessages, + status: lastStartedSubtaskId !== null ? 'streaming' : this.state.status, + streamingSubtaskId: lastStartedSubtaskId ?? this.state.streamingSubtaskId, + } this.pendingStarts = [] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/tasks/state/TaskStateMachine.ts` around lines 727 - 741, applyPendingStarts() currently inserts AI streaming messages into this.state.messages but doesn't update this.state.status or this.state.streamingSubtaskId, leaving status === 'ready' while streaming messages exist; modify applyPendingStarts (and reuse createAiMessage) so that when you add at least one pending start you also set this.state.status = 'streaming' and this.state.streamingSubtaskId = the corresponding start.subtaskId (use the first start you actually insert), then commit the combined state update (messages, status, streamingSubtaskId) and finally clear this.pendingStarts.
226-227:⚠️ Potential issue | 🟠 MajorBuffering chunks in
idlestate can duplicate recovered content.
BUFFERING_STATESincludes'idle', soCHAT_CHUNKevents arriving beforerecover()is called are queued inpendingChunks. Whenrecover()fetchescached_contentfrom Redis (which already contains those tokens), and thenapplyPendingChunks()appends the queued chunks, the content is duplicated.Consider excluding
'idle'from chunk buffering while keeping it for start event buffering:🔧 Proposed fix
- private static readonly BUFFERING_STATES: readonly TaskStatus[] = ['idle', 'joining', 'syncing'] + private static readonly START_BUFFERING_STATES: readonly TaskStatus[] = ['idle', 'joining', 'syncing'] + private static readonly CHUNK_BUFFERING_STATES: readonly TaskStatus[] = ['joining', 'syncing'] - private isBufferingState(): boolean { - return TaskStateMachine.BUFFERING_STATES.includes(this.state.status) + private isStartBufferingState(): boolean { + return TaskStateMachine.START_BUFFERING_STATES.includes(this.state.status) + } + + private isChunkBufferingState(): boolean { + return TaskStateMachine.CHUNK_BUFFERING_STATES.includes(this.state.status) }Then update
handleChatStartEventto useisStartBufferingState()andhandleChatChunkEventto useisChunkBufferingState().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/tasks/state/TaskStateMachine.ts` around lines 226 - 227, BUFFERING_STATES currently includes 'idle', causing CHAT_CHUNK events to be queued (pendingChunks) before recover() runs and then duplicated when cached_content is merged; remove 'idle' from the chunk-buffering set and split buffering logic into two helpers so start events still buffer: create/adjust isStartBufferingState() to return true for states that should buffer starts (including 'idle') and isChunkBufferingState() to return true for states that should buffer chunks (exclude 'idle'), update BUFFERING_STATES or introduce separate constants accordingly, and change handleChatStartEvent to call isStartBufferingState() and handleChatChunkEvent to call isChunkBufferingState(); ensure pendingChunks/applyPendingChunks flow uses the new chunk-buffering check.frontend/src/features/tasks/contexts/taskContext.tsx (2)
735-739:⚠️ Potential issue | 🟠 MajorAbort in-flight request when switching tasks.
When switching to a different task (lines 735-739),
leaveTaskis called andselectedTaskDetailis cleared, but the abort controller is not cancelled. An in-flightgetTaskDetailrequest from the previous task could still resolve and overwrite state after the task switch.🔧 Proposed fix
if (previousTaskId !== null && previousTaskId !== currentTaskId) { + if (abortControllerRef.current) { + abortControllerRef.current.abort() + abortControllerRef.current = null + } leaveTask(previousTaskId) // Clear stale detail immediately so downstream hooks don't operate on wrong task data setSelectedTaskDetail(null) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/tasks/contexts/taskContext.tsx` around lines 735 - 739, When switching tasks in the branch where previousTaskId !== null && previousTaskId !== currentTaskId, make sure to cancel any in-flight getTaskDetail request by calling abort() on the AbortController used for task detail fetch (the same controller that getTaskDetail uses); update the logic around leaveTask, setSelectedTaskDetail(null) to first abort the existing controller (guarding for undefined), then clear state, and ensure a fresh AbortController is created for the next getTaskDetail call so stale responses cannot overwrite state.
752-754:⚠️ Potential issue | 🟠 MajorAbort in-flight request when clearing task selection.
When
selectedTaskbecomesnull, the code setsselectedTaskDetail(null)without aborting any in-flightgetTaskDetailrequest. A pending fetch from the previous task can resolve after this line and overwrite the cleared state with stale data.🔧 Proposed fix
} else { + if (abortControllerRef.current) { + abortControllerRef.current.abort() + abortControllerRef.current = null + } setSelectedTaskDetail(null) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/tasks/contexts/taskContext.tsx` around lines 752 - 754, When selectedTask becomes null we currently call setSelectedTaskDetail(null) but do not abort any ongoing getTaskDetail fetch, allowing a stale response to overwrite the cleared state; modify the fetch flow in the code that calls getTaskDetail (the logic around selectedTask, setSelectedTaskDetail and getTaskDetail) to create and store an AbortController for each started request, pass controller.signal into getTaskDetail, and on the branch where selectedTask is set to null call controller.abort() before setSelectedTaskDetail(null); additionally ensure the getTaskDetail response handler ignores aborted requests (or verifies selectedTask id still matches the fetched task) before calling setSelectedTaskDetail to prevent stale overwrites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/features/tasks/hooks/useUnifiedMessages.ts`:
- Around line 227-233: The forEach callback in cleanupRecoveredTasks currently
uses a concise arrow that returns the result of
recoveredTasksRef.current.delete(id), which Biome flags; change the callback to
a statement body (or use a for...of) so the delete is invoked without returning
a value — e.g., in cleanupRecoveredTasks, after computing toRemove, iterate over
toRemove and call recoveredTasksRef.current.delete(id) inside a block callback
(or a plain loop) to remove items without returning anything; keep references to
recoveredTasksRef, MAX_RECOVERED_TASKS and toRemove to locate the code.
---
Duplicate comments:
In `@frontend/src/features/tasks/contexts/taskContext.tsx`:
- Around line 735-739: When switching tasks in the branch where previousTaskId
!== null && previousTaskId !== currentTaskId, make sure to cancel any in-flight
getTaskDetail request by calling abort() on the AbortController used for task
detail fetch (the same controller that getTaskDetail uses); update the logic
around leaveTask, setSelectedTaskDetail(null) to first abort the existing
controller (guarding for undefined), then clear state, and ensure a fresh
AbortController is created for the next getTaskDetail call so stale responses
cannot overwrite state.
- Around line 752-754: When selectedTask becomes null we currently call
setSelectedTaskDetail(null) but do not abort any ongoing getTaskDetail fetch,
allowing a stale response to overwrite the cleared state; modify the fetch flow
in the code that calls getTaskDetail (the logic around selectedTask,
setSelectedTaskDetail and getTaskDetail) to create and store an AbortController
for each started request, pass controller.signal into getTaskDetail, and on the
branch where selectedTask is set to null call controller.abort() before
setSelectedTaskDetail(null); additionally ensure the getTaskDetail response
handler ignores aborted requests (or verifies selectedTask id still matches the
fetched task) before calling setSelectedTaskDetail to prevent stale overwrites.
In `@frontend/src/features/tasks/hooks/useUnifiedMessages.ts`:
- Around line 245-253: Currently the code calls
recoveredTasksRef.current.add(effectiveTaskId) before calling recover(), which
means a failed recover() will still mark the task as recovered; change the flow
in useUnifiedMessages so you await recover() inside a try/catch and only call
recoveredTasksRef.current.add(effectiveTaskId) after recover() resolves
successfully (call cleanupRecoveredTasks() as appropriate either before or after
the successful mark), and handle/rethrow or log errors in the catch so failed
recoveries aren't marked; reference recoveredTasksRef, effectiveTaskId, recover,
cleanupRecoveredTasks, and isStreaming when making the change.
In `@frontend/src/features/tasks/state/TaskStateMachine.ts`:
- Around line 727-741: applyPendingStarts() currently inserts AI streaming
messages into this.state.messages but doesn't update this.state.status or
this.state.streamingSubtaskId, leaving status === 'ready' while streaming
messages exist; modify applyPendingStarts (and reuse createAiMessage) so that
when you add at least one pending start you also set this.state.status =
'streaming' and this.state.streamingSubtaskId = the corresponding
start.subtaskId (use the first start you actually insert), then commit the
combined state update (messages, status, streamingSubtaskId) and finally clear
this.pendingStarts.
- Around line 226-227: BUFFERING_STATES currently includes 'idle', causing
CHAT_CHUNK events to be queued (pendingChunks) before recover() runs and then
duplicated when cached_content is merged; remove 'idle' from the chunk-buffering
set and split buffering logic into two helpers so start events still buffer:
create/adjust isStartBufferingState() to return true for states that should
buffer starts (including 'idle') and isChunkBufferingState() to return true for
states that should buffer chunks (exclude 'idle'), update BUFFERING_STATES or
introduce separate constants accordingly, and change handleChatStartEvent to
call isStartBufferingState() and handleChatChunkEvent to call
isChunkBufferingState(); ensure pendingChunks/applyPendingChunks flow uses the
new chunk-buffering check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56791e2a-7df5-45a3-9252-b45bacaef562
📒 Files selected for processing (6)
frontend/src/apis/tasks.tsfrontend/src/features/tasks/components/chat/ChatArea.tsxfrontend/src/features/tasks/components/message/MessageBubble.tsxfrontend/src/features/tasks/contexts/taskContext.tsxfrontend/src/features/tasks/hooks/useUnifiedMessages.tsfrontend/src/features/tasks/state/TaskStateMachine.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/features/tasks/components/message/MessageBubble.tsx
Comment on lines
+227
to
+233
| const cleanupRecoveredTasks = useCallback(() => { | ||
| const MAX_RECOVERED_TASKS = 50 | ||
| const tasks = Array.from(recoveredTasksRef.current) | ||
| if (tasks.length > MAX_RECOVERED_TASKS) { | ||
| const toRemove = tasks.slice(0, tasks.length - MAX_RECOVERED_TASKS) | ||
| toRemove.forEach(id => recoveredTasksRef.current.delete(id)) | ||
| } |
Contributor
There was a problem hiding this comment.
Fix the forEach callback so Biome stops flagging it.
Line 232 uses a concise arrow that returns Set.delete(...). forEach ignores that value, but Biome treats it as suspicious and will keep failing here.
Suggested fix
- toRemove.forEach(id => recoveredTasksRef.current.delete(id))
+ toRemove.forEach(id => {
+ recoveredTasksRef.current.delete(id)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const cleanupRecoveredTasks = useCallback(() => { | |
| const MAX_RECOVERED_TASKS = 50 | |
| const tasks = Array.from(recoveredTasksRef.current) | |
| if (tasks.length > MAX_RECOVERED_TASKS) { | |
| const toRemove = tasks.slice(0, tasks.length - MAX_RECOVERED_TASKS) | |
| toRemove.forEach(id => recoveredTasksRef.current.delete(id)) | |
| } | |
| const cleanupRecoveredTasks = useCallback(() => { | |
| const MAX_RECOVERED_TASKS = 50 | |
| const tasks = Array.from(recoveredTasksRef.current) | |
| if (tasks.length > MAX_RECOVERED_TASKS) { | |
| const toRemove = tasks.slice(0, tasks.length - MAX_RECOVERED_TASKS) | |
| toRemove.forEach(id => { | |
| recoveredTasksRef.current.delete(id) | |
| }) | |
| } |
🧰 Tools
🪛 Biome (2.4.4)
[error] 232-232: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/tasks/hooks/useUnifiedMessages.ts` around lines 227 -
233, The forEach callback in cleanupRecoveredTasks currently uses a concise
arrow that returns the result of recoveredTasksRef.current.delete(id), which
Biome flags; change the callback to a statement body (or use a for...of) so the
delete is invoked without returning a value — e.g., in cleanupRecoveredTasks,
after computing toRemove, iterate over toRemove and call
recoveredTasksRef.current.delete(id) inside a block callback (or a plain loop)
to remove items without returning anything; keep references to
recoveredTasksRef, MAX_RECOVERED_TASKS and toRemove to locate the code.
This was referenced Mar 10, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TaskStateMachine:
TaskStateManager:
useUnifiedMessages:
MessageBubble:
ChatArea:
New files:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements