Conversation
Contributor
📝 WalkthroughWalkthroughRefactors QQ Bot delivery state from persisted segments to in-memory per-endpoint tool buffers, adds Changes
Sequence DiagramsequenceDiagram
participant Client as Inbound Message
participant Runtime as QQBotRuntime
participant Buffer as QQBotToolBufferState
participant Delivery as Delivery System
participant Remote as Remote Endpoint
Client->>Runtime: processInboundMessage / deliverConversation
activate Runtime
Note over Runtime: derive sourceMessageId & poll snapshots
Runtime->>Buffer: syncToolBuffer(sourceMessageId, deliverySegments)
Runtime->>Buffer: identify ready pendingProcessSegments
loop flush while batches ready
Runtime->>Buffer: collect pending process texts
Runtime->>Delivery: enforce QQBOT_MAX_PASSIVE_REPLIES
Delivery->>Remote: send combined tool/process message
Remote-->>Delivery: ack
Runtime->>Buffer: mark flushedProcessKeys
end
alt Conversation completes
Runtime->>Delivery: send final answer text
else Timeout/Error
Runtime->>Delivery: flush remaining tool text
Runtime->>Delivery: send QQBOT_TIMEOUT_REPLY / error reply
end
Runtime->>Buffer: clear endpointToolBuffers
deactivate Runtime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
zerob13
changed the title
![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: 2
🧹 Nitpick comments (3)
test/main/presenter/sqlitePresenter.test.ts (1)
721-797: LGTM — solid coverage for the catalog-driventimeout_msrepair path.Test correctly bootstraps a v24 DB with
timeout_msmissing, verifiesdiagnoseSchema()reports the issue,repairSchema()status isrepaired, and post-repair the column exists while the pre-existingreasoning_visibilityvalue is preserved.Minor nit: the seed value
reasoning_visibility = 'auto'is only round-tripped as a raw TEXT here, but if'auto'is not a member ofReasoningVisibility, using a canonical value (e.g., one accepted byisReasoningVisibility) would make the fixture more faithful to production data. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/sqlitePresenter.test.ts` around lines 721 - 797, The test seeds reasoning_visibility with the literal 'auto' which may not match the canonical values; update the fixture in the deepchat_sessions INSERT so the seeded value is a valid production value accepted by isReasoningVisibility (e.g., use a member from the ReasoningVisibility enum or the canonical string the isReasoningVisibility helper expects), then rerun the assertions around presenter.diagnoseSchema() and presenter.repairSchema() to ensure the value is round-tripped unchanged.src/main/presenter/sqlitePresenter/tables/deepchatSessions.ts (1)
164-214: Remove redundanttimeout_mscolumn addition from v23 recovery migration (cleanup only).The migration system explicitly tolerates
ALTER TABLE ADD COLUMNerrors when the column already exists (seeshouldIgnoreMigrationStatementErrorinsrc/main/presenter/sqlitePresenter/index.ts), so v24's unconditionaltimeout_msaddition will not cause a migration failure even if v23 recovery adds it first. However, the redundant attempt is poor code design.Either remove
timeout_msfromgetRecoveryMigrationStatements()(Option A) or make v24 conditional (Option B) to avoid unnecessary error logging and clarify intent. If Option A, also update the test expectation attest/main/presenter/sqlitePresenter/deepchatSessionsTable.test.tsline 111–117.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/sqlitePresenter/tables/deepchatSessions.ts` around lines 164 - 214, The v24 migration unconditionally adds timeout_ms but getRecoveryMigrationStatements (used by getMigrationSQL(version: 23)) already includes timeout_ms, causing a redundant ALTER TABLE; remove the duplicate by either (A) deleting the timeout_ms statement from getRecoveryMigrationStatements so v23 recovery no longer tries to add it, or (B) make the v24 branch in getMigrationSQL conditional (check this.hasColumn('timeout_ms') or call getRecoveryMigrationStatements to avoid adding it) and update the test expectation in test/main/presenter/sqlitePresenter/deepchatSessionsTable.test.ts (lines ~111–117) if you choose option A. Ensure references: getRecoveryMigrationStatements, getMigrationSQL (version 23 and 24), and timeout_ms are adjusted accordingly.test/main/presenter/remoteControlPresenter/qqbotRuntime.test.ts (1)
1-1: Mirror the QQBot source subdirectory in the test path.This suite targets
src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts, but the test sits directly underremoteControlPresenter/. Move it totest/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.test.tsso the test tree mirrors the source tree. As per coding guidelines, “Vitest test suites should mirror the source structure undertest/main/**andtest/renderer/**.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/qqbotRuntime.test.ts` at line 1, Move the test file so the test tree mirrors the source: relocate the current qqbotRuntime.test.ts from test/main/presenter/remoteControlPresenter/ to test/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.test.ts so it sits alongside the source module qqbotRuntime.ts; update any internal import paths in the test (references to qqbotRuntime, imports at top of the file) to reflect the new relative path and ensure vitest imports (afterEach, describe, expect, it, vi) still resolve.
🤖 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/remoteControlPresenter/qqbot/qqbotRuntime.ts`:
- Around line 333-336: The code recomputes sourceMessageId each poll
(getConversationSourceMessageId) and getOrCreateToolBuffer currently clears the
buffer when that id changes, causing flushedProcessKeys to reset and resends
when a conversation's source id evolves (e.g., execution.eventId ->
snapshot.messageId); to fix, change getOrCreateToolBuffer/syncToolBuffer logic
to preserve the original buffer identity once any process batches are observed:
detect when flushedProcessKeys (or any processed segments) exist for a
conversation and thereafter treat new sourceMessageId values as aliases
(migrate/merge them into the existing buffer) instead of deleting/creating a new
buffer, or implement a lock that pins the buffer id for that conversation until
the conversation completes; update getOrCreateToolBuffer to check for an
existing pinned buffer key and perform alias migration/merge of deliverySegments
rather than clearing state when sourceMessageId differs.
- Around line 354-360: The code currently always sends finalText from
getFinalDeliveryText() even when it equals the no-response sentinel ("No
assistant response was produced."), causing a noisy extra message after process
output; update the branch around flushBufferedProcessMessages(endpointKey,
sendContext, { reserveTerminalSlot: Boolean(finalText) }) and
sendText(sendContext, finalText) to skip sending when finalText matches the
shared no-response sentinel and process output was actually flushed. Implement
this by checking the same sentinel constant (or string) used elsewhere and a
flag indicating process output was flushed (e.g., have
flushBufferedProcessMessages return a boolean or inspect sendContext/return
value) and only call sendText(sendContext, finalText) when either finalText is
not the sentinel or no process output was flushed; update the related test to
expect the sentinel to be omitted when process output exists.
---
Nitpick comments:
In `@src/main/presenter/sqlitePresenter/tables/deepchatSessions.ts`:
- Around line 164-214: The v24 migration unconditionally adds timeout_ms but
getRecoveryMigrationStatements (used by getMigrationSQL(version: 23)) already
includes timeout_ms, causing a redundant ALTER TABLE; remove the duplicate by
either (A) deleting the timeout_ms statement from getRecoveryMigrationStatements
so v23 recovery no longer tries to add it, or (B) make the v24 branch in
getMigrationSQL conditional (check this.hasColumn('timeout_ms') or call
getRecoveryMigrationStatements to avoid adding it) and update the test
expectation in test/main/presenter/sqlitePresenter/deepchatSessionsTable.test.ts
(lines ~111–117) if you choose option A. Ensure references:
getRecoveryMigrationStatements, getMigrationSQL (version 23 and 24), and
timeout_ms are adjusted accordingly.
In `@test/main/presenter/remoteControlPresenter/qqbotRuntime.test.ts`:
- Line 1: Move the test file so the test tree mirrors the source: relocate the
current qqbotRuntime.test.ts from test/main/presenter/remoteControlPresenter/ to
test/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.test.ts so it sits
alongside the source module qqbotRuntime.ts; update any internal import paths in
the test (references to qqbotRuntime, imports at top of the file) to reflect the
new relative path and ensure vitest imports (afterEach, describe, expect, it,
vi) still resolve.
In `@test/main/presenter/sqlitePresenter.test.ts`:
- Around line 721-797: The test seeds reasoning_visibility with the literal
'auto' which may not match the canonical values; update the fixture in the
deepchat_sessions INSERT so the seeded value is a valid production value
accepted by isReasoningVisibility (e.g., use a member from the
ReasoningVisibility enum or the canonical string the isReasoningVisibility
helper expects), then rerun the assertions around presenter.diagnoseSchema() and
presenter.repairSchema() to ensure the value is round-tripped unchanged.
🪄 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: 50f509a0-5fb3-4a35-9738-1c617383e025
📒 Files selected for processing (6)
src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.tssrc/main/presenter/sqlitePresenter/schemaCatalog.tssrc/main/presenter/sqlitePresenter/tables/deepchatSessions.tstest/main/presenter/remoteControlPresenter/qqbotRuntime.test.tstest/main/presenter/sqlitePresenter.test.tstest/main/presenter/sqlitePresenter/deepchatSessionsTable.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts (1)
284-289: Optional: extract the repeated tool buffer + delivery state cleanup.The pair
clearToolBuffer(endpointKey)+bindingStore.clearRemoteDeliveryState(endpointKey)recurs in five places (error path here, and indeliverConversationat lines 325-326, 350-351, 368-369, 379-380). A small helper would make it harder to forget one side of the cleanup in future branches.♻️ Proposed refactor
+ private resetEndpointDeliveryState(endpointKey: string): void { + this.clearToolBuffer(endpointKey) + this.deps.bindingStore.clearRemoteDeliveryState(endpointKey) + }Then replace the paired calls with
this.resetEndpointDeliveryState(endpointKey).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts` around lines 284 - 289, Extract the repeated cleanup pair into a single helper on the class (e.g., add a method resetEndpointDeliveryState(endpointKey) that calls this.clearToolBuffer(endpointKey) and this.deps.bindingStore.clearRemoteDeliveryState(endpointKey)); then replace each occurrence of the pair (currently calls to clearToolBuffer(endpointKey) + this.deps.bindingStore.clearRemoteDeliveryState(endpointKey) — seen in the error path around the flush + sendText block and the various deliverConversation branches) with a single call to this.resetEndpointDeliveryState(endpointKey) to ensure both sides are always cleaned together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.ts`:
- Around line 284-289: Extract the repeated cleanup pair into a single helper on
the class (e.g., add a method resetEndpointDeliveryState(endpointKey) that calls
this.clearToolBuffer(endpointKey) and
this.deps.bindingStore.clearRemoteDeliveryState(endpointKey)); then replace each
occurrence of the pair (currently calls to clearToolBuffer(endpointKey) +
this.deps.bindingStore.clearRemoteDeliveryState(endpointKey) — seen in the error
path around the flush + sendText block and the various deliverConversation
branches) with a single call to this.resetEndpointDeliveryState(endpointKey) to
ensure both sides are always cleaned together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 324cdf84-48f5-4ab8-b615-94193197c645
📒 Files selected for processing (2)
src/main/presenter/remoteControlPresenter/qqbot/qqbotRuntime.tstest/main/presenter/remoteControlPresenter/qqbotRuntime.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/main/presenter/remoteControlPresenter/qqbotRuntime.test.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
deepchat_sessions.timeout_msso data migration and recovery paths stay aligned with the actual table definition.Testing
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests