fix(chat): keep inference alive across tab switches#583
Closed
M3gA-Mind wants to merge 3 commits intotinyhumansai:mainfrom
Closed
fix(chat): keep inference alive across tab switches#583M3gA-Mind wants to merge 3 commits intotinyhumansai:mainfrom
M3gA-Mind wants to merge 3 commits intotinyhumansai:mainfrom
Conversation
Closes tinyhumansai#577 Made-with: Cursor
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR centralizes chat event listeners from Changes
Sequence DiagramsequenceDiagram
participant SocketProvider
participant ChatEventManager as ChatEventManager<br/>(Singleton)
participant ChatService
participant Redux as Redux Store
participant Conversations
Note over SocketProvider,Conversations: Socket Connection Lifecycle
SocketProvider->>ChatEventManager: init() on sessionToken change
ChatEventManager->>ChatService: subscribeChatEvents(listeners)
ChatService-->>ChatEventManager: return unsubscribe cleanup
Note over SocketProvider,Conversations: Real-time Event Handling
ChatService->>ChatEventManager: onSegment(event)
ChatEventManager->>ChatEventManager: Check deduplication (seenChatEvents)
ChatEventManager->>Redux: dispatch(setToolTimelineForThread)
ChatEventManager->>Redux: dispatch(upsertStreamingForThread)
ChatService->>ChatEventManager: onDone(event)
ChatEventManager->>Redux: dispatch(clearInferenceStatusForThread)
ChatEventManager->>Redux: dispatch(clearStreamingForThread)
ChatEventManager->>Redux: dispatch(setActiveThread(null))
Note over SocketProvider,Conversations: Component Reads Redux State
Redux-->>Conversations: inferenceState.sendingByThread[threadId]
Redux-->>Conversations: inferenceState.toolTimelineByThread[threadId]
Redux-->>Conversations: inferenceState.streamingAssistantByThread[threadId]
Note over SocketProvider,Conversations: Disconnect Lifecycle
SocketProvider->>ChatEventManager: teardown() on sessionToken → null
ChatEventManager->>ChatService: call unsubscribe cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (2)
app/test/e2e/specs/chat-tab-switch.spec.ts (1)
101-109: Add diagnostics around request/response wait failures.If Line 101 or Line 108 fails, triage is still expensive. Capture request-log snapshots and accessibility tree in those failure paths too (not only when input is missing).
🛠️ Suggested refactor
const chatReq = await waitForRequest('POST', '/openai/v1/chat/completions', 30_000); + if (!chatReq) { + stepLog('chat completion request not observed', getRequestLog().slice(-20)); + const tree = await dumpAccessibilityTree(); + stepLog('accessibility snapshot on missing request', tree.slice(0, 4000)); + } expect(chatReq).toBeDefined(); @@ await navigateToConversations(); - await waitForText('Hello from e2e mock agent', 30_000); + try { + await waitForText('Hello from e2e mock agent', 30_000); + } catch (error) { + stepLog('assistant response not visible after tab return', getRequestLog().slice(-20)); + const tree = await dumpAccessibilityTree(); + stepLog('accessibility snapshot on missing response', tree.slice(0, 4000)); + throw error; + } expect(await textExists('Type a message...')).toBe(true);As per coding guidelines: "Add failure diagnostics (request logs, dumpAccessibilityTree()) for faster debugging by agents in E2E specs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/chat-tab-switch.spec.ts` around lines 101 - 109, Wrap the critical waits (waitForRequest('POST', '/openai/v1/chat/completions'), waitForText('Hello from e2e mock agent'), and the textExists('Type a message...') assertion) in try/catch (or add .catch handlers) and on any failure capture diagnostics: save request logs/ snapshots for the failing endpoint and call dumpAccessibilityTree() before rethrowing; include context (which wait failed and relevant request/response payloads) so failures in waitForRequest, waitForText, or textExists produce request-log snapshots and an accessibility tree for faster triage.app/src/services/chatEventManager.ts (1)
77-428: Add event-level debug logs around the new recovery path.The manager logs
init/teardown, but not which events were applied, deduped, or used to clear thread state. That makes tab-switch recovery regressions hard to trace when a thread gets stuck or an event is dropped.As per coding guidelines: "Add substantial, development-oriented logs ... log critical checkpoints including entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error handling paths."
Also applies to: 431-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/chatEventManager.ts` around lines 77 - 428, Add development-level debug logs at the start and at key branch points inside each event handler to trace which events are applied, deduped, skipped, or cause state clears; specifically, instrument setInferenceStart, setIterationStart, setToolCall, setToolResult, setSubagentSpawned, setSubagentDone, setSegment, setTextDelta, setThinkingDelta, setToolArgsDelta, setDone, and setError to log the incoming event (thread_id, request_id, round, tool_name, tool_call_id, success/error_type), the result of markChatEventSeen checks, decisions taken (e.g., existingIdx found, merged/added entry, changed=false/true), and state-clear actions (clearInferenceStatusForThread/clearStreamingForThread/setActiveThread); use the project's debug logger (or console.debug if none) and keep messages concise and consistent so developers can trace entry/exit and branch outcomes for the recovery path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/pages/Conversations.tsx`:
- Around line 427-439: The timeout/failure handler only resets sending and
activeThread (sendingTimeoutRef, dispatch(setThreadSending...), setSendError,
chatEventManager.clearPendingReaction, dispatch(setActiveThread(null))) but
leaves per-thread runtime state like inferenceStatusByThread,
streamingAssistantByThread and any running tool entries, which causes stale
“Thinking…” UI; update these failure branches (the timeout block and the similar
branch at 453-461) to also clear/remove the thread's entries from
inferenceStatusByThread and streamingAssistantByThread and to cancel/clear any
running tools for sendingThreadId (via your chat/tool manager), by dispatching
the appropriate Redux actions or calling the existing chatEventManager/tool
cleanup methods for that thread so all per-thread runtime state is fully
cleared.
- Around line 321-324: The send-watchdog timeout is currently stored in the
component-scoped sendingTimeoutRef and cleared on unmount, which loses the
fallback that flips sendingByThread back to false; instead move the timeout
storage out of the component lifecycle (e.g., a module-level Map or WeakMap
keyed by threadId) so timeouts survive route/tab switches, set the timeout when
marking a thread as sending, and only clear that module-level timeout when a
chat_done or chat_error handler runs or when the timeout fires to reset
sendingByThread; do not clear the module-level timeout in the Conversations
component cleanup—update references from sendingTimeoutRef to the new
module-level storage and adjust the chat_done/chat_error handlers to clear the
corresponding entry.
In `@app/src/services/__tests__/chatEventManager.test.ts`:
- Around line 47-69: This test initializes the module-scoped singleton
chatEventManager but never tears it down, risking cross-test state leakage; add
a teardown at the end of this test that calls a cleanup method on the singleton
(e.g., chatEventManager.destroy() or chatEventManager.teardown()) to unsubscribe
its listeners and reset internal state, and if such a method doesn't exist add a
test-only cleanup method on chatEventManager to remove subscriptions (matching
how mockSubscribeChatEvents registers listeners) so mockSubscribeChatEvents and
setActiveThread interactions are isolated between tests.
In `@app/src/services/chatEventManager.ts`:
- Around line 228-249: The handler setSubagentDone currently updates every
timeline entry whose name equals `🤖 ${event.tool_name}` and status ===
'running'; change it to update only the single matching spawned entry
(preferably by stable ID if the entries have one, e.g., entry.id or
entry.stableId, otherwise pick the latest running entry). In setSubagentDone
locate the target entry index in
store.getState().inference.toolTimelineByThread[event.thread_id] by matching
entry.id === event.subagent_id (or fallback: find last index where entry.name
=== `🤖 ${event.tool_name}` && entry.status === 'running'), then call
setToolTimelineForThread with entries mapped to update only that index (leave
all others unchanged). Keep the inference status update
(setInferenceStatusForThread) as-is after this change.
---
Nitpick comments:
In `@app/src/services/chatEventManager.ts`:
- Around line 77-428: Add development-level debug logs at the start and at key
branch points inside each event handler to trace which events are applied,
deduped, skipped, or cause state clears; specifically, instrument
setInferenceStart, setIterationStart, setToolCall, setToolResult,
setSubagentSpawned, setSubagentDone, setSegment, setTextDelta, setThinkingDelta,
setToolArgsDelta, setDone, and setError to log the incoming event (thread_id,
request_id, round, tool_name, tool_call_id, success/error_type), the result of
markChatEventSeen checks, decisions taken (e.g., existingIdx found, merged/added
entry, changed=false/true), and state-clear actions
(clearInferenceStatusForThread/clearStreamingForThread/setActiveThread); use the
project's debug logger (or console.debug if none) and keep messages concise and
consistent so developers can trace entry/exit and branch outcomes for the
recovery path.
In `@app/test/e2e/specs/chat-tab-switch.spec.ts`:
- Around line 101-109: Wrap the critical waits (waitForRequest('POST',
'/openai/v1/chat/completions'), waitForText('Hello from e2e mock agent'), and
the textExists('Type a message...') assertion) in try/catch (or add .catch
handlers) and on any failure capture diagnostics: save request logs/ snapshots
for the failing endpoint and call dumpAccessibilityTree() before rethrowing;
include context (which wait failed and relevant request/response payloads) so
failures in waitForRequest, waitForText, or textExists produce request-log
snapshots and an accessibility tree for faster triage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26b20ae4-7954-412d-9548-3f3e86b3e1ab
📒 Files selected for processing (11)
.claude/memory.mdapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.remount.test.tsxapp/src/providers/SocketProvider.tsxapp/src/services/__tests__/chatEventManager.test.tsapp/src/services/chatEventManager.tsapp/src/store/__tests__/inferenceSlice.test.tsapp/src/store/index.tsapp/src/store/inferenceSlice.tsapp/test/e2e/helpers/element-helpers.tsapp/test/e2e/specs/chat-tab-switch.spec.ts
Address CodeRabbit feedback for watchdog lifecycle, runtime cleanup, subagent timeline matching, and E2E diagnostics. Made-with: Cursor
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.
Summary
Conversationsinto a globalchatEventManagerso in-flight agent runs continue across route/tab switchesinferenceSliceruntime Redux state for per-thread sending, streaming, inference status, and tool timeline so remounting chat rehydrates correctlyTest plan
yarn workspace openhuman-app compileyarn workspace openhuman-app lintyarn workspace openhuman-app format:checkyarn workspace openhuman-app buildyarn vitest run src/pages/__tests__/Conversations.remount.test.tsx src/store/__tests__/inferenceSlice.test.ts src/services/__tests__/chatEventManager.test.tsbash app/scripts/e2e-run-spec.sh test/e2e/specs/chat-tab-switch.spec.ts chat-tab-switch(currently flaky on macOS Appium session/auth state)Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests