fix(deepchat): sync queue and stop states#1376
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughConsolidates terminal error block construction into Changes
Sequence DiagramsequenceDiagram
participant Client
participant Presenter
participant MessageStore
participant EventBus
participant Renderer
Client->>Presenter: sendMessage / queuePendingInput
Presenter->>Presenter: assign runId, register in activeGenerations
Presenter->>MessageStore: persist user message (create)
Presenter->>EventBus: emit stream:start (user persisted)
Presenter->>Presenter: runStreamForMessage(runId)
Presenter->>Renderer: stream events (partial tokens)
alt stream produces tokens
Presenter->>MessageStore: append pending blocks
EventBus->>Renderer: stream:append
else stream errors / aborts / empty
Presenter->>Presenter: buildTerminalErrorBlocks(blocks, errorMessage)
Presenter->>MessageStore: updateContentAndStatus(assistantId, blocks, status='error')
Presenter->>EventBus: emit stream:error / stream:end
Presenter->>activeGenerations: clear runId
end
Presenter->>Presenter: unregister runId
Presenter->>EventBus: emit final stream:end / refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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: 3
🧹 Nitpick comments (3)
test/renderer/components/ChatInputToolbar.test.ts (2)
78-93: Consider adding a test case for stop emission.The current tests verify the send path, but there's no explicit test for clicking the button in stop mode and verifying
stopis emitted. This would provide symmetric coverage.📝 Additional test case
it('emits stop when clicked in stop mode', async () => { const ChatInputToolbar = (await import('@/components/chat/ChatInputToolbar.vue')).default const wrapper = mount(ChatInputToolbar, { props: { isGenerating: true, hasInput: false, sendDisabled: false } }) await wrapper.find('[data-icon="lucide:square"]').element.closest('button')?.click() await wrapper.vm.$nextTick() expect(wrapper.emitted('stop')).toEqual([[]]) expect(wrapper.emitted('send')).toBeUndefined() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/ChatInputToolbar.test.ts` around lines 78 - 93, Add a symmetric test that verifies the component emits "stop" (and not "send") when clicking the stop control while isGenerating is true: import ChatInputToolbar, mount it with props isGenerating: true, hasInput: false, sendDisabled: false (same setup as the existing test), locate and click the stop button (the element with data-icon="lucide:square" or its closest 'button'), wait for DOM updates, then assert wrapper.emitted('stop') equals a single empty event and wrapper.emitted('send') is undefined; place this alongside the existing test for send in ChatInputToolbar.test.ts.
88-89: Use a more robust selector to avoid brittle positional index.
findAll('button')[1]relies on DOM order, which will break silently if the template layout changes. Consider locating the button via its unique content or a test attribute.🔧 Suggested approach: Find button by its icon child
await wrapper.setProps({ hasInput: true }) - await wrapper.findAll('button')[1].trigger('click') + const sendIcon = wrapper.find('[data-icon="lucide:arrow-up"]') + await sendIcon.element.closest('button')?.click() + await wrapper.vm.$nextTick()Or add a
data-testidto the primary action button in the component:<Button data-testid="primary-action" ... />Then in the test:
await wrapper.find('[data-testid="primary-action"]').trigger('click')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/ChatInputToolbar.test.ts` around lines 88 - 89, The test uses a brittle positional selector wrapper.findAll('button')[1] in ChatInputToolbar.test.ts; update the test to target the primary action button with a stable selector — either add a data-testid (e.g., data-testid="primary-action") to the Button in the ChatInputToolbar component and use wrapper.find('[data-testid="primary-action"]').trigger('click'), or locate the button by its unique icon/label (e.g., find the button that contains the specific icon component or text) instead of relying on array index.src/main/presenter/deepchatAgentPresenter/pendingInputStore.ts (1)
43-47: Consider narrowing queue-creation state type.Allowing
'consumed'increateQueueInputWithStatemakes queue creation semantically inconsistent. Restricting this API to'pending' | 'claimed'would prevent invalid records.♻️ Suggested type tightening
- createQueueInputWithState( + createQueueInputWithState( sessionId: string, input: string | SendMessageInput, - state: PendingSessionInputState + state: Exclude<PendingSessionInputState, 'consumed'> ): PendingSessionInputRecord {Also applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/deepchatAgentPresenter/pendingInputStore.ts` around lines 43 - 47, createQueueInputWithState currently accepts PendingSessionInputState including 'consumed', which allows creating semantically invalid queue records; change the parameter type on createQueueInputWithState (and the similar overload at lines ~51-52) to a narrower union that excludes 'consumed' (e.g., 'pending' | 'claimed' or create a new PendingQueueInputState type), update the function signature(s) to use that new type, and adjust any callers to satisfy the narrower type (or explicitly convert/validate states before calling) so only valid queue states can be created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/deepchatAgentPresenter/pendingInputStore.ts`:
- Around line 43-60: createQueueInputWithState can assign duplicate queueOrder
because getNextQueueOrder only inspects pending rows; update getNextQueueOrder
so it computes the next queueOrder against all queue-mode rows (regardless of
state, including 'claimed') or use an atomic DB-side MAX(queue_order)+1 to avoid
races, then use that value when inserting in createQueueInputWithState
(deepchatPendingInputsTable.insert). Ensure getNextQueueOrder (or the new
helper) is the single source of truth so claimed inserts don't reuse existing
queue_order values.
In `@src/main/presenter/deepchatAgentPresenter/process.ts`:
- Around line 99-100: Avoid double-terminalization on user-abort paths by making
the finalize call idempotent: before calling finalizeError(...) in the abort
branches (the spots currently passing USER_CANCELED_GENERATION_ERROR), check
whether cancelGeneration already marked the active message as cancelled/errored
(e.g., inspect state.activeMessage?.status or state.activeMessage?.error?.code
for USER_CANCELED_GENERATION_ERROR) and only call finalizeError if that check
shows the message is not already finalized; apply the same guard to the other
abort branches (the other calls around the same logic), or remove the redundant
finalizeError calls so cancelGeneration is the single writer for user-cancelled
flows.
In `@src/renderer/src/components/message/MessageItemAssistant.vue`:
- Around line 31-33: The spinner visibility checks the root message's status but
the displayed text is variant-aware via currentContent/currentMessage; change
the Spinner condition to use currentMessage.status (or fallback to
message.status if currentMessage is null) so the spinner reflects the displayed
variant's pending state — update the v-if that references message.status to
check currentMessage.status (with a safe null-check) alongside
currentContent.length === 0 and keep Spinner and its class unchanged.
---
Nitpick comments:
In `@src/main/presenter/deepchatAgentPresenter/pendingInputStore.ts`:
- Around line 43-47: createQueueInputWithState currently accepts
PendingSessionInputState including 'consumed', which allows creating
semantically invalid queue records; change the parameter type on
createQueueInputWithState (and the similar overload at lines ~51-52) to a
narrower union that excludes 'consumed' (e.g., 'pending' | 'claimed' or create a
new PendingQueueInputState type), update the function signature(s) to use that
new type, and adjust any callers to satisfy the narrower type (or explicitly
convert/validate states before calling) so only valid queue states can be
created.
In `@test/renderer/components/ChatInputToolbar.test.ts`:
- Around line 78-93: Add a symmetric test that verifies the component emits
"stop" (and not "send") when clicking the stop control while isGenerating is
true: import ChatInputToolbar, mount it with props isGenerating: true, hasInput:
false, sendDisabled: false (same setup as the existing test), locate and click
the stop button (the element with data-icon="lucide:square" or its closest
'button'), wait for DOM updates, then assert wrapper.emitted('stop') equals a
single empty event and wrapper.emitted('send') is undefined; place this
alongside the existing test for send in ChatInputToolbar.test.ts.
- Around line 88-89: The test uses a brittle positional selector
wrapper.findAll('button')[1] in ChatInputToolbar.test.ts; update the test to
target the primary action button with a stable selector — either add a
data-testid (e.g., data-testid="primary-action") to the Button in the
ChatInputToolbar component and use
wrapper.find('[data-testid="primary-action"]').trigger('click'), or locate the
button by its unique icon/label (e.g., find the button that contains the
specific icon component or text) instead of relying on array index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3855dde3-4558-4416-8e50-b3070f15cc64
📒 Files selected for processing (13)
src/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/messageStore.tssrc/main/presenter/deepchatAgentPresenter/pendingInputCoordinator.tssrc/main/presenter/deepchatAgentPresenter/pendingInputStore.tssrc/main/presenter/deepchatAgentPresenter/process.tssrc/renderer/src/components/chat/ChatInputToolbar.vuesrc/renderer/src/components/message/MessageItemAssistant.vuetest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/main/presenter/deepchatAgentPresenter/messageStore.test.tstest/main/presenter/newAgentPresenter/integration.test.tstest/renderer/components/ChatInputToolbar.test.tstest/renderer/components/message/MessageItemAssistant.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf459db2f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/main/presenter/deepchatAgentPresenter/process.test.ts (1)
483-492: Tighten the non-emission assertion to avoid false negatives.Line 483 currently only rejects one exact
stream:errorpayload. The test should assert nostream:erroremission for this message at all.Proposed test assertion hardening
- expect(eventBus.sendToRenderer).not.toHaveBeenCalledWith( - 'stream:error', - 'all', - expect.objectContaining({ - conversationId: 's1', - messageId: 'm1', - eventId: 'm1', - error: 'common.error.userCanceledGeneration' - }) - ) + const streamErrorCalls = (eventBus.sendToRenderer as ReturnType<typeof vi.fn>).mock.calls + .filter( + ([eventName, target, payload]) => + eventName === 'stream:error' && + target === 'all' && + payload?.conversationId === 's1' && + payload?.messageId === 'm1' && + payload?.eventId === 'm1' + ) + expect(streamErrorCalls).toHaveLength(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/deepchatAgentPresenter/process.test.ts` around lines 483 - 492, The test currently only rejects a single exact stream:error payload; tighten it so it asserts no stream:error was emitted for this message at all by changing the expectation to ensure eventBus.sendToRenderer was not called with 'stream:error' and a payload containing at least conversationId: 's1' and messageId: 'm1' (use expect.objectContaining({ conversationId: 's1', messageId: 'm1' }) as the third arg), or alternately scan eventBus.sendToRenderer.mock.calls and assert no call exists where the first arg === 'stream:error' and the third arg contains those two fields; reference eventBus.sendToRenderer and the conversationId/messageId values to locate the assertion to change.test/main/presenter/deepchatAgentPresenter/pendingInputStore.test.ts (1)
90-107: Consider adding a test case that distinguishes claimed-only vs all-rows ordering.The test description says "after claimed queue rows" but with only a claimed row present, the test would pass whether the implementation considers all queue rows or only claimed ones. To explicitly verify the claimed-only ordering behavior, consider adding a case with mixed rows where pending has a higher
queueOrderthan claimed:// Example: pending row has higher order than claimed createQueueRow('pending-1', 'session-1', 3, 'pending'), createQueueRow('claimed-1', 'session-1', 1, 'claimed')If
createQueueInputonly considers claimed rows, next order would be 2. If it considers all rows, it would be 4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/deepchatAgentPresenter/pendingInputStore.test.ts` around lines 90 - 107, Add a new test in pendingInputStore.test.ts that verifies claimed-only ordering by creating mixed existing rows (use createQueueRow to add a pending row with higher queueOrder and a claimed row with lower queueOrder), then call store.createQueueInput(sessionId, ...) and assert the returned record.queueOrder is the next after claimed rows (2) and that deepchatPendingInputsTable.insert was called with an objectContaining queueOrder: 2; reference createQueueRow, createStore, store.createQueueInput, and deepchatPendingInputsTable.insert to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/main/presenter/deepchatAgentPresenter/pendingInputStore.test.ts`:
- Around line 90-107: Add a new test in pendingInputStore.test.ts that verifies
claimed-only ordering by creating mixed existing rows (use createQueueRow to add
a pending row with higher queueOrder and a claimed row with lower queueOrder),
then call store.createQueueInput(sessionId, ...) and assert the returned
record.queueOrder is the next after claimed rows (2) and that
deepchatPendingInputsTable.insert was called with an objectContaining
queueOrder: 2; reference createQueueRow, createStore, store.createQueueInput,
and deepchatPendingInputsTable.insert to locate where to add the test.
In `@test/main/presenter/deepchatAgentPresenter/process.test.ts`:
- Around line 483-492: The test currently only rejects a single exact
stream:error payload; tighten it so it asserts no stream:error was emitted for
this message at all by changing the expectation to ensure
eventBus.sendToRenderer was not called with 'stream:error' and a payload
containing at least conversationId: 's1' and messageId: 'm1' (use
expect.objectContaining({ conversationId: 's1', messageId: 'm1' }) as the third
arg), or alternately scan eventBus.sendToRenderer.mock.calls and assert no call
exists where the first arg === 'stream:error' and the third arg contains those
two fields; reference eventBus.sendToRenderer and the conversationId/messageId
values to locate the assertion to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e727668b-d2e1-45ee-9212-e37cb42bef1e
📒 Files selected for processing (6)
src/main/presenter/deepchatAgentPresenter/pendingInputStore.tssrc/main/presenter/deepchatAgentPresenter/process.tssrc/renderer/src/components/message/MessageItemAssistant.vuetest/main/presenter/deepchatAgentPresenter/pendingInputStore.test.tstest/main/presenter/deepchatAgentPresenter/process.test.tstest/renderer/components/message/MessageItemAssistant.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/src/components/message/MessageItemAssistant.vue
- test/renderer/components/message/MessageItemAssistant.test.ts
- src/main/presenter/deepchatAgentPresenter/process.ts
- src/main/presenter/deepchatAgentPresenter/pendingInputStore.ts
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 by CodeRabbit
Bug Fixes
UX Improvements
Tests