refactor(ai): optimize AI session manager and provider factory#14
Merged
Conversation
…session manager cache
📝 WalkthroughWalkthroughAISessionManager now precompiles and stores SQLite prepared statements on construction and reuses them across methods; AIProviderFactory caches a single AISessionManager instance and supplies it to all provider constructors. ChangesSession Manager Optimization & Caching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/ai/session/ai-session-manager.ts`:
- Around line 204-217: addMessageAtomic currently reads next_seq via
getNextSeqStmt and then inserts with addMessageStmt in separate steps, which
allows races and duplicate sequence values under concurrent writers; wrap the
read-and-insert in a single DB transaction (use your DB's transaction API or a
BEGIN/COMMIT block) so the sequence read and addMessageStmt.run happen
atomically (or instead atomically increment a counter row and use its returned
value), and enforce uniqueness at the schema level by creating the unique index
idx_ai_messages_session_sequence on ai_messages(ai_session_id, sequence); update
addMessageAtomic, getNextSeqStmt usage, and any sequence counter logic to
operate inside that transaction.
🪄 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 Plus
Run ID: 95f97b56-2f3c-41b8-ba6b-92b7654b4f9d
📒 Files selected for processing (2)
src/services/ai/ai-provider-factory.tssrc/services/ai/session/ai-session-manager.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/ai/session/ai-session-manager.ts`:
- Around line 117-119: Before creating the unique index
idx_ai_messages_session_sequence_unique in ai-session-manager.ts, run a
transaction that deletes duplicate rows from the ai_messages table keeping only
the row with the smallest id per (ai_session_id, sequence) pair, then create the
unique index; ensure you execute BEGIN, DELETE (using a self-join or correlated
subquery to keep min(id)), CREATE UNIQUE INDEX IF NOT EXISTS
idx_ai_messages_session_sequence_unique ON ai_messages(ai_session_id, sequence),
and COMMIT, and add error handling that performs ROLLBACK on any failure so the
DB is not left partially migrated—implement this using the same DB handle
(this.db.run/this.db.exec) in the initialization code that currently calls
CREATE UNIQUE INDEX.
🪄 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 Plus
Run ID: 977d3434-dd54-4dfa-b39a-f444d3c6a2ef
📒 Files selected for processing (1)
src/services/ai/session/ai-session-manager.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/services/ai/session/ai-session-manager.ts (2)
155-180: ⚡ Quick win
createSessionis the only write path not using a cached prepared statement.Every other mutation (
addMessage,updateSession,deleteSession,clearMessages,cleanupExpiredSessions) was migrated to a storedStatement, but theINSERT INTO ai_sessionsat line 160 still prepares ad-hoc SQL on every call, directly contradicting the PR's stated goal of eliminating re-preparation overhead.♻️ Proposed refactor
Add a
createSessionStmtfield alongside the others (lines 23–34):+ private createSessionStmt: any;Initialize it in the constructor (after
initDatabase()):+ this.createSessionStmt = this.db.prepare(` + INSERT INTO ai_sessions ( + id, provider, session_id, conversation_id, + metadata, created_at, updated_at, expires_at + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `);Then replace the inline
db.runincreateSession:- this.db.run( - ` - INSERT INTO ai_sessions ( - id, provider, session_id, conversation_id, - metadata, created_at, updated_at, expires_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) - `, - [ - id, - params.provider, - params.sessionId, - params.conversationId || null, - JSON.stringify(params.metadata || {}), - now, - now, - expiresAt, - ] - ); + this.createSessionStmt.run( + id, + params.provider, + params.sessionId, + params.conversationId || null, + JSON.stringify(params.metadata || {}), + now, + now, + expiresAt + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/ai/session/ai-session-manager.ts` around lines 155 - 180, createSession currently uses a raw db.run call instead of the prepared-statement pattern used elsewhere; add a new Statement field createSessionStmt to the class (matching other statement fields), initialize it in the constructor after initDatabase() with the same INSERT INTO ai_sessions SQL, and then replace the inline this.db.run(...) inside createSession with a call to this.createSessionStmt.run(...) binding the same parameters (id, params.provider, params.sessionId, params.conversationId || null, JSON.stringify(params.metadata || {}), now, now, expiresAt), then return getSession as before.
114-116: ⚡ Quick winNon-unique index on
(ai_session_id, sequence)is redundant after the unique index is created.
idx_ai_messages_session(line 114–116) andidx_ai_messages_session_sequence_unique(line 138) cover the exact same column pair. SQLite's query planner will use the unique index for all lookups, making the non-unique one dead weight. Maintaining two indexes on the same columns doubles the write overhead on everyINSERTintoai_messages.Since
ensureUniqueMessageSequencesalways runs immediately after (and throws on failure), remove the non-unique index and let the unique one serve both purposes:♻️ Proposed refactor
- this.db.run( - "CREATE INDEX IF NOT EXISTS idx_ai_messages_session ON ai_messages(ai_session_id, sequence)" - ); - this.ensureUniqueMessageSequences();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/ai/session/ai-session-manager.ts` around lines 114 - 116, Remove the redundant non-unique index creation for idx_ai_messages_session on the ai_messages table: delete the this.db.run(...) call that creates "CREATE INDEX IF NOT EXISTS idx_ai_messages_session ON ai_messages(ai_session_id, sequence)". Rely on the existing unique index idx_ai_messages_session_sequence_unique (created later) to cover lookups and keep ensureUniqueMessageSequences behavior unchanged; this avoids double write overhead on INSERTs into ai_messages and leaves ensureUniqueMessageSequences and the unique index creation intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/ai/session/ai-session-manager.ts`:
- Around line 155-180: createSession currently uses a raw db.run call instead of
the prepared-statement pattern used elsewhere; add a new Statement field
createSessionStmt to the class (matching other statement fields), initialize it
in the constructor after initDatabase() with the same INSERT INTO ai_sessions
SQL, and then replace the inline this.db.run(...) inside createSession with a
call to this.createSessionStmt.run(...) binding the same parameters (id,
params.provider, params.sessionId, params.conversationId || null,
JSON.stringify(params.metadata || {}), now, now, expiresAt), then return
getSession as before.
- Around line 114-116: Remove the redundant non-unique index creation for
idx_ai_messages_session on the ai_messages table: delete the this.db.run(...)
call that creates "CREATE INDEX IF NOT EXISTS idx_ai_messages_session ON
ai_messages(ai_session_id, sequence)". Rely on the existing unique index
idx_ai_messages_session_sequence_unique (created later) to cover lookups and
keep ensureUniqueMessageSequences behavior unchanged; this avoids double write
overhead on INSERTs into ai_messages and leaves ensureUniqueMessageSequences and
the unique index creation intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dc7b565-6ef4-4964-9ff3-fd3c5213e565
📒 Files selected for processing (1)
src/services/ai/session/ai-session-manager.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
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.
Changes
thisbinding bug:getMessagespreviously usedrows.map(this.rowToMessage)which losesthiscontext in strict mode. Fixed torows.map((row) => this.rowToMessage(row)).updateSession: Replaced string-concatenated UPDATE with 3 static prepared statements (conversation_id only, metadata only, both). Removes SQL injection risk from internal code path.sessionManagerstatic cache socreateProvidercallsgetAISessionManager()once instead of 4 times per creation.Impact
Testing
Requesting @coderabbitai full review for additional optimization opportunities in the AI provider layer and across the codebase.
Summary by CodeRabbit