fix(chat): restore queue slots and allow 10 - #2236
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe pending-input capacity increases from 5 to 10. Claimed inputs no longer count toward the active queue limit. Renderer limits and integration tests reflect the updated behavior. ChangesPending input capacity
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change raises the waiting-message limit to ten and releases waiting capacity when messages are claimed while retaining them for execution and recovery. The updated backend, UI limits, and regression coverage align with that behavior, with no current merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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
left a comment
There was a problem hiding this comment.
Review: Approve ✅
(Comment review — GitHub does not allow approving one's own PR. Reviewed as if approving.)
Verified against f886d6e9b (merge-base with dev):
Core fix — correct. countActiveQueue now excludes claimed rows, which aligns the JS admission count with the pre-existing SQL semantics in countActiveBySession (AND NOT (mode = 'queue' AND state = 'claimed'), deepchatPendingInputs.ts:208) and with the renderer's activeCount (derived from listPendingInputs, which already filtered claimed). This resolves the UI-shows-4/5-but-Enter-fails mismatch described in the PR.
Claimed inputs remain tracked. Still returned by getInput/listActiveInputs (restart recovery), hasClaimedInput (drain gating), and getClaimedInput; release paths (releaseClaimedQueueInputTo, blockClaimedInput) correctly re-occupy a slot.
Races — none. ensureWithinLimit + insert run synchronously on the single-threaded main process; the async gap in sendQueuedMessage is re-checked authoritatively at the final synchronous admission point. Delete/move are transactional.
No stale assumptions of 5. Constants, error strings, i18n queueCount/limitReached (all 20 locales use {count}/{max}), and existing tests checked — no leftover 5 semantics. Off-by-one correct on all three gates (main throw, renderer isAtCapacity, lane hint).
Test — appropriate. The single new it covers the persistence contract (reject at 11, refill after claim with ordering, refill after delete while claimed stays active) via the public SessionPendingInputs API. Smallest durable coverage, nothing implementation-coupled.
Non-blocking notes (follow-up only)
- P3 — The limit now lives in three hand-synchronized places:
MAX_ACTIVE_PENDING_INPUTS(pendingInputs.ts:12),MAX_PENDING_INPUTS(pendingInput.ts:6), and the prop default in PendingInputLane.vue:295. All three were updated consistently here, but a future limit change must again touch all three or the UI counter will disagree with the admission gate. Consider a shared constant insrc/sharedas a follow-up. - P3 —
countActiveQueuematerializes rows and filters in JS while its siblingcountActivedelegates to a SQLCOUNT. A SQL predicate would match the sibling's pattern and avoid row materialization on every enqueue. Not required in this PR given the smallest-change rule.
yyhhyyyyyy
left a comment
There was a problem hiding this comment.
Three inline notes. The first is a small regression, the other two are minor.
| return this.database.deepchatPendingInputsTable | ||
| .listActiveBySession(sessionId) | ||
| .filter((row) => row.mode === 'queue').length | ||
| .filter((row) => row.mode === 'queue' && row.state !== 'claimed').length |
There was a problem hiding this comment.
Excluding claimed here fixes the 4/5 mismatch, but the release paths (releaseClaimedQueueInputTo, releaseClaimedInput, restart recovery) move the item back to pending / retry_required with no capacity check. With 10 waiting + 1 claimed, a pre-user-fact rollback, release-for-mutation, or a restart release leaves 11 waiting items and the lane shows 11/10. Reproduced locally; before this change the same flow saturated at 5. Probably want admission to keep one slot reserved while an item is claimed, or cap the displayed count.
| import type { SessionTranscript } from './transcript' | ||
|
|
||
| const MAX_ACTIVE_PENDING_INPUTS = 5 | ||
| const MAX_ACTIVE_PENDING_INPUTS = 10 |
There was a problem hiding this comment.
Nit: the limit now lives in three places (this constant, renderer MAX_PENDING_INPUTS, and the activeLimit default in PendingInputLane.vue, which ChatPage never passes). A single shared constant would stop them drifting again.
| }) | ||
| }) | ||
|
|
||
| describeIfNativeSqlite('SessionPendingInputs queue capacity', () => { |
There was a problem hiding this comment.
FYI this suite is describe.skip under plain Node (17 skipped locally), and the test-main CI job runs plain Node, so this regression test won't execute in CI unless the file is added to the DEEPCHAT_REQUIRE_NATIVE_SQLITE=1 list.
Allow up to 10 waiting messages per session and free a waiting slot as soon as a queued message is claimed. Previously, the UI hid the running message while the main process still counted it toward the five-message limit, so Enter and Queue could fail even with a visible count of 4/5.
The main admission limit, renderer submit gating, and waiting-lane counter/full hint now use 10. Claimed messages remain tracked for execution and recovery, but do not occupy waiting slots. A SQLite regression covers rejecting an eleventh waiting message, refilling after a claim, and refilling after deletion while the claimed message remains active.
Validation:
pnpm run format:check,pnpm run i18n,pnpm run lint, andpnpm run typecheck.ELECTRON_RUN_AS_NODE=1 DEEPCHAT_REQUIRE_NATIVE_SQLITE=1 pnpm exec electron node_modules/vitest/vitest.mjs run --project main test/main/session/data/tables/deepchatPendingInputsTable.test.ts -t 'SessionPendingInputs queue capacity'.The full native table suite has four unrelated existing failures in migration/transaction tests, reproduced with the unchanged test file. They involve mocked temporary-directory creation and obsolete direct table access on
MainDatabase.Summary by CodeRabbit
New Features
Bug Fixes