[Refactor] simplify conversation services#866
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 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 |
![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: 43b6fffa28
ℹ️ 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".
| ...options, | ||
| mode: 'context' | ||
| }) | ||
| ...(lookupMode === 'target' ? { permission: 'manage' } : {}), | ||
| mode: lookupMode | ||
| } | ||
| const resolved = await this.resolveConversation(session, lookupOptions) |
There was a problem hiding this comment.
updateConversationUsage() now calls resolveConversation() in 'active' mode before assertManageAllowed(), and active resolution can create a new conversation when none exists (see resolveActiveMode create path). That means a user who is blocked by manageMode: 'admin' can still mutate state (new conversation row) by running chatluna.use.*, then receive an authorization error afterward. The previous flow checked permissions before any active-mode side effects.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the conversation service and its middlewares to improve code structure, error handling, and performance. Key changes include the introduction of custom error classes, extraction of reusable logic into helper functions, and optimization of conversation resolution flows. Feedback suggests improving the readability of the parseLockOption function with a switch statement and optimizing database queries in collectAclConversationIds by using the $in operator to reduce the number of calls.
| function parseLockOption( | ||
| raw: string | undefined | ||
| ): boolean | null | 'toggle' | undefined { | ||
| if (raw === 'reset') return null | ||
| if (raw === 'true' || raw === 'on' || raw === 'lock') return true | ||
| if (raw === 'false' || raw === 'off' || raw === 'unlock') return false | ||
| if (raw === 'toggle') return 'toggle' | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
The chain of if statements in parseLockOption can be replaced with a switch statement for improved readability and structure. This makes the different cases clearer at a glance.
function parseLockOption(
raw: string | undefined
): boolean | null | 'toggle' | undefined {
switch (raw) {
case 'reset':
return null
case 'true':
case 'on':
case 'lock':
return true
case 'false':
case 'off':
case 'unlock':
return false
case 'toggle':
return 'toggle'
default:
return undefined
}
}| private async collectAclConversationIds( | ||
| session: Session, | ||
| required: ConstraintPermission | ||
| ) { | ||
| const acl: ACLRecord[] = [] | ||
| const fetch = async ( | ||
| principalType: 'user' | 'guild', | ||
| principalId: string, | ||
| permission: ConstraintPermission | ||
| ) => { | ||
| acl.push( | ||
| ...((await this.ctx.database.get('chatluna_acl', { | ||
| principalType: 'guild', | ||
| principalId: guildId, | ||
| permission: 'manage' | ||
| principalType, | ||
| principalId, | ||
| permission | ||
| })) as ACLRecord[]) | ||
| ) | ||
|
|
||
| if (required === 'view') { | ||
| acl.push( | ||
| ...((await this.ctx.database.get('chatluna_acl', { | ||
| principalType: 'guild', | ||
| principalId: guildId, | ||
| permission: 'view' | ||
| })) as ACLRecord[]) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| const conversationIds = Array.from( | ||
| new Set(acl.map((item) => item.conversationId)) | ||
| ) | ||
|
|
||
| const matches: ConversationRecord[] = [] | ||
|
|
||
| for (let i = 0; i < conversationIds.length; i += 200) { | ||
| const ids = conversationIds.slice(i, i + 200) | ||
| const conversations = (await this.ctx.database.get( | ||
| 'chatluna_conversation', | ||
| { | ||
| id: { | ||
| $in: ids | ||
| } | ||
| } | ||
| )) as ConversationRecord[] | ||
| await fetch('user', session.userId, 'manage') | ||
| if (required === 'view') { | ||
| await fetch('user', session.userId, 'view') | ||
| } | ||
|
|
||
| matches.push(...conversations.filter(matched)) | ||
| const guildId = session.guildId ?? session.channelId | ||
| if (guildId != null) { | ||
| await fetch('guild', guildId, 'manage') | ||
| if (required === 'view') { | ||
| await fetch('guild', guildId, 'view') | ||
| } | ||
| } | ||
|
|
||
| return matches | ||
| return Array.from(new Set(acl.map((item) => item.conversationId))) | ||
| } |
There was a problem hiding this comment.
The collectAclConversationIds function currently makes up to four separate database calls to fetch ACLs. This can be optimized by using the $in operator to fetch permissions for 'view' and 'manage' in a single query for each principal type ('user' and 'guild'), reducing the number of database calls to a maximum of two.
private async collectAclConversationIds(
session: Session,
required: ConstraintPermission
) {
const acl: ACLRecord[] = []
const permissions = required === 'view' ? ['view', 'manage'] : ['manage']
const userAcls = await this.ctx.database.get('chatluna_acl', {
principalType: 'user',
principalId: session.userId,
permission: { $in: permissions }
})
acl.push(...(userAcls as ACLRecord[]))
const guildId = session.guildId ?? session.channelId
if (guildId != null) {
const guildAcls = await this.ctx.database.get('chatluna_acl', {
principalType: 'guild',
principalId: guildId,
permission: { $in: permissions }
})
acl.push(...(guildAcls as ACLRecord[]))
}
return Array.from(new Set(acl.map((item) => item.conversationId)))
}1 participant
This pr refactors core conversation services to simplify conversation resolution, management, runtime request handling, and typed constraint errors.
New Features
None
Bug fixes
Other Changes
ConversationServiceto consolidate managed constraint lookup, model fallback selection, ACL lookup, and target conversation matching.ConversationRuntimeto extract request key building, platform validation, abort signal linking, human message construction, and reply formatting.conversation_types.ts.