refactor(plugin): optimize plugin entry, config, and supporting services#15
Merged
Merged
Conversation
- Extract shared system prompt templates in auto-capture.ts and user-memory-learning.ts to eliminate 200+ line duplication between opencode and manual provider paths - Cache 20 prepared statements in UserPromptManager constructor for all hot-path queries (save, get, delete, mark captured, claim, user learning, cleanup, search, link) - Use module-level LOCAL_HOSTS Set and SENSITIVE_KEYS Set in web-server.ts to avoid recreating on every request - Optimize formatMemoriesForCompaction to use array join instead of string concatenation - Add fast paths for single-item batch operations in user-prompt-manager
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR extracts prompt-template helpers, centralizes web-server loopback/PII constants, precompiles SQL statements in UserPromptManager and updates its methods to use them, and makes minor index help-text and memory-formatting refactors. ChangesCode Refactoring and Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
🧹 Nitpick comments (1)
src/services/user-prompt/user-prompt-manager.ts (1)
83-83: ⚡ Quick winThree prepared statements duplicate already-declared ones — reuse the existing statements.
The refactor introduces three unnecessary duplicate prepared statements:
New (duplicate) Identical to markMultipleCaptured(line 83)markCaptured(line 67)markMultipleUserLearning(lines 96-98)markUserLearningCaptured(lines 93-95)getByIds(line 118)getById(line 105)Each is only ever used in the single-item fast paths of
markMultipleAsCaptured,markMultipleAsUserLearningCaptured, andgetPromptsByIds. Removing the duplicates and pointing the fast paths at the already-existing statements reduces both thestmtssurface area and the number of prepared statement handles held against the DB connection.♻️ Proposed fix
Remove the three duplicate declarations from the
stmtstype and initializer:- markMultipleCaptured: ReturnType<DatabaseType["prepare"]>; markMultipleUserLearning: ReturnType<DatabaseType["prepare"]>; ... - getByIds: ReturnType<DatabaseType["prepare"]>;- markMultipleCaptured: this.db.prepare(`UPDATE user_prompts SET captured = 1 WHERE id = ?`), ... - markMultipleUserLearning: this.db.prepare( - `UPDATE user_prompts SET user_learning_captured = 1 WHERE id = ?` - ), ... - getByIds: this.db.prepare(`SELECT * FROM user_prompts WHERE id = ?`),Then update the three fast-path call sites:
// markMultipleAsCaptured - this.stmts.markMultipleCaptured.run(promptIds[0]); + this.stmts.markCaptured.run(promptIds[0]); // markMultipleAsUserLearningCaptured - this.stmts.markMultipleUserLearning.run(promptIds[0]); + this.stmts.markUserLearningCaptured.run(promptIds[0]); // getPromptsByIds - const row = this.stmts.getByIds.get(ids[0]) as any; + const row = this.stmts.getById.get(ids[0]) as any;Also applies to: 96-98, 118-118
🤖 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/user-prompt/user-prompt-manager.ts` at line 83, Remove the three duplicate prepared-statement declarations and reuse the existing ones: delete the stmts entries markMultipleCaptured, markMultipleUserLearning, and getByIds and stop initializing them; then update the fast-path call sites in markMultipleAsCaptured to call the existing markCaptured statement, in markMultipleAsUserLearningCaptured to call markUserLearningCaptured, and in getPromptsByIds to call getById for the single-id fast path so the code uses the already-declared prepared statements instead of creating duplicates.
🤖 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/user-prompt/user-prompt-manager.ts`:
- Around line 264-271: The searchPrompts method builds a LIKE pattern without
escaping user-supplied metacharacters, so queries with % or _ behave as
wildcards; modify searchPrompts to escape backslashes, % and _ in the query
(e.g. replace \ with \\ then % with \% and _ with \_) before surrounding with
%...% and passing to the statements, and update the prepared SQL for
stmts.searchPrompts and stmts.searchPromptsByProject to include "ESCAPE '\\'" so
the DB treats the backslash as the escape character; keep mapping via
rowToPrompt unchanged.
---
Nitpick comments:
In `@src/services/user-prompt/user-prompt-manager.ts`:
- Line 83: Remove the three duplicate prepared-statement declarations and reuse
the existing ones: delete the stmts entries markMultipleCaptured,
markMultipleUserLearning, and getByIds and stop initializing them; then update
the fast-path call sites in markMultipleAsCaptured to call the existing
markCaptured statement, in markMultipleAsUserLearningCaptured to call
markUserLearningCaptured, and in getPromptsByIds to call getById for the
single-id fast path so the code uses the already-declared prepared statements
instead of creating duplicates.
🪄 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: 0e431ef0-b27c-4803-89be-3460027ec2c1
📒 Files selected for processing (5)
src/index.tssrc/services/auto-capture.tssrc/services/user-memory-learning.tssrc/services/user-prompt/user-prompt-manager.tssrc/services/web-server.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/user-prompt/user-prompt-manager.ts (1)
239-245: ⚡ Quick winMake linked-ID collection and deletion atomic.
These two statements can observe different row sets, so
linkedMemoryIdscan drift fromdeletedif matching prompts are modified between the select and delete. Wrapping them in one transaction would keep the returned IDs consistent with the rows actually removed.🤖 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/user-prompt/user-prompt-manager.ts` around lines 239 - 245, The select and delete in deleteOldPrompts currently run separately and can see different row sets; wrap both this.stmts.getLinkedMemoryIds.all(cutoffTime) and this.stmts.deleteOldPrompts.run(cutoffTime) inside a single database transaction so the select and delete observe the same snapshot, capture the linkedMemoryIds from the select and the result.changes from the delete within that transaction, and return those values; ensure the transaction helper for your DB (e.g., db.transaction or similar) is used so failures rollback and errors are rethrown.
🤖 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/user-prompt/user-prompt-manager.ts`:
- Around line 239-245: The select and delete in deleteOldPrompts currently run
separately and can see different row sets; wrap both
this.stmts.getLinkedMemoryIds.all(cutoffTime) and
this.stmts.deleteOldPrompts.run(cutoffTime) inside a single database transaction
so the select and delete observe the same snapshot, capture the linkedMemoryIds
from the select and the result.changes from the delete within that transaction,
and return those values; ensure the transaction helper for your DB (e.g.,
db.transaction or similar) is used so failures rollback and errors are rethrown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e790b45a-45f3-412f-b6a1-fbb7c70dd72b
📒 Files selected for processing (1)
src/services/user-prompt/user-prompt-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.
Summary
Optimization pass over plugin entry point (
index.ts), configuration (config.ts), auto-capture, user memory learning, user prompt manager, and web server modules.Changes
src/services/auto-capture.tsAUTO_CAPTURE_SYSTEM_PROMPT_TEMPLATEandAUTO_CAPTURE_ANALYSIS_PROMPTconstants to eliminate ~100 lines of duplicated prompt text between the opencode provider path and the manual API provider pathsrc/services/user-memory-learning.tsUSER_PROFILE_SYSTEM_PROMPTconstant to eliminate ~10 lines of duplicated prompt text between the two provider pathssrc/services/user-prompt/user-prompt-manager.tssrc/services/web-server.tslocalHostsSet to module-levelLOCAL_HOSTSconstant to avoid recreating on every HTTP requestSENSITIVE_KEYSSet for O(1) PII redaction lookups instead of O(n)Array#includessrc/index.tsformatMemoriesForCompactionto use arrayjoin()instead of repeated string concatenation in a loopVerification
Summary by CodeRabbit
Refactor
Bug Fixes