[Feature] delete conversations by sequence range#894
Open
dingyi222666 wants to merge 1 commit into
Open
Conversation
Contributor
通读此 PR 在 ChatLuna 对话管理系统中实现会话批量删除功能。用户现可通过范围格式（如 变更会话批量删除功能
🎯 2 (Simple) | ⏱️ ~12 分钟
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to delete conversations by specifying ranges or lists of sequences (e.g., '1..2' or '1,3'). It adds sequence parsing logic, updates the conversation management middleware to handle batch deletions, and includes corresponding unit tests. The review feedback highlights two important improvements: first, a critical security concern regarding a potential Denial of Service (DoS) vulnerability in parseSeqs when handling extremely large ranges, which should be mitigated by enforcing a maximum sequence limit; second, a performance optimization in deleteBySeqs to use a Set for sequence lookups, reducing the filtering complexity from O(N * M) to O(N + M).
![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: 1d3f6bbd40
ℹ️ 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".
Comment on lines
+430
to
+435
| for ( | ||
| let seq = Math.min(start, end); | ||
| seq <= Math.max(start, end); | ||
| seq += 1 | ||
| ) { | ||
| seqs.add(seq) |
There was a problem hiding this comment.
When a user runs a selector with a very large range, e.g. chatluna.delete 1..1000000000, this loop expands every integer into a Set before the middleware ever checks which conversations exist. Because chatluna.delete is available at authority 1, a short command can block the bot's event loop and/or exhaust memory; validate the range size or defer expansion until after listing the available entries.
Useful? React with 👍 / 👎.
![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: 1
🧹 Nitpick comments (2)
packages/core/src/commands/conversation.ts (1)
243-244: ⚖️ Poor tradeoff建议内联 parseSeqs 函数
根据代码规范："a function is justified only when called from multiple distinct call sites with non-trivial logic"。
parseSeqs仅在 244 行被调用一次，建议直接内联到调用处以减少不必要的抽象层级。♻️ 内联后的建议实现
const presetLane = options.preset?.trim() || undefined - const target = conversation?.trim() || undefined - const seqs = target == null ? undefined : parseSeqs(target) + const input = conversation?.trim() || undefined + let seqs: number[] | undefined + if ( + input != null && + (input.includes(',') || input.includes('..')) && + /^\d+(?:\.\.\d+)?(?:,\d+(?:\.\.\d+)?)*$/.test(input) + ) { + const set = new Set<number>() + for (const part of input.split(',')) { + const [start, end = start] = part.split('..').map(Number) + for ( + let seq = Math.min(start, end); + seq <= Math.max(start, end); + seq += 1 + ) { + set.add(seq) + } + } + seqs = [...set] + } const includeArchived = options.archived === true || options.all === true await chain.receiveCommand( session, 'conversation_delete', { allPresetLanes: presetLane == null, conversation_manage: { - targetConversation: seqs == null ? target : undefined, + targetConversation: seqs == null ? input : undefined, targetConversationSeqs: seqs, presetLane, includeArchived } }, ctx ) }) -function parseSeqs(input: string) { - if (!input.includes(',') && !input.includes('..')) return undefined - if (!/^\d+(?:\.\.\d+)?(?:,\d+(?:\.\.\d+)?)*$/.test(input)) { - return undefined - } - - const seqs = new Set<number>() - for (const part of input.split(',')) { - const [start, end = start] = part.split('..').map(Number) - for ( - let seq = Math.min(start, end); - seq <= Math.max(start, end); - seq += 1 - ) { - seqs.add(seq) - } - } - - return [...seqs] -} - declare module '../chains/chain' {根据代码规范："Prefer inline code over extracting into functions; a function is justified only when called from multiple distinct call sites with non-trivial logic"。
Also applies to: 421-440
🤖 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 `@packages/core/src/commands/conversation.ts` around lines 243 - 244, The helper function parseSeqs should be inlined where it's only used: replace the call sites that assign seqs (e.g., const target = conversation?.trim() || undefined; const seqs = target == null ? undefined : parseSeqs(target)) by moving parseSeqs's logic directly into that expression so seqs is computed without an extra function; do the same for the other occurrence around the 421–440 block—remove the parseSeqs declaration and update callers (reference symbols: parseSeqs, target, seqs) so there is no separate function call and behavior remains identical.packages/core/src/middlewares/system/conversation_manage.ts (1)
283-293: ⚖️ Poor tradeoff建议内联 deleteBySeqs 函数
deleteBySeqs函数仅在 287 行被调用一次，根据代码规范应该内联到调用处。♻️ 内联后的建议实现
middleware('conversation_delete', async (session, context) => { try { const { presetLane, includeArchived, allPresetLanes } = getManageOptions(context) const seqs = context.options.conversation_manage?.targetConversationSeqs if (seqs != null) { - context.message = await deleteBySeqs(ctx, session, seqs, { - presetLane, - includeArchived, - allPresetLanes - }) + const entries = await ctx.chatluna.conversation.listConversationEntries( + session, + { presetLane, includeArchived, allPresetLanes } + ) + const targets = entries.filter((item) => seqs.includes(item.displaySeq)) + if (targets.length !== seqs.length) { + context.message = session.text('chatluna.conversation.messages.target_not_found') + } else { + const deleted: ConversationRecord[] = [] + for (const target of targets) { + deleted.push( + await ctx.chatluna.conversation.deleteConversation(session, { + conversationId: target.conversation.id, + presetLane, + includeArchived, + allPresetLanes + }) + ) + } + context.message = session.text('chatluna.conversation.messages.delete_success', [ + deleted.map((item) => item.title).join('\n') + ]) + } return ChainMiddlewareRunStatus.STOP } const conversation = await ctx.chatluna.conversation.deleteConversation(session, { conversationId: resolvedConversationId(context), presetLane, includeArchived, allPresetLanes }) context.message = session.text( 'chatluna.conversation.messages.delete_success', conversationSummary(conversation) ) } catch (error) { context.message = session.text( 'chatluna.conversation.messages.delete_failed', [formatConversationError(session, error, 'delete')] ) } return ChainMiddlewareRunStatus.STOP }) -async function deleteBySeqs( - ctx: Context, - session: Session, - seqs: number[], - opts: ReturnType<typeof getManageOptions> -) { - const entries = await ctx.chatluna.conversation.listConversationEntries( - session, - opts - ) - const targets = entries.filter((item) => seqs.includes(item.displaySeq)) - if (targets.length !== seqs.length) { - return session.text('chatluna.conversation.messages.target_not_found') - } - - const deleted: ConversationRecord[] = [] - for (const target of targets) { - deleted.push( - await ctx.chatluna.conversation.deleteConversation(session, { - conversationId: target.conversation.id, - ...opts - }) - ) - } - - return session.text('chatluna.conversation.messages.delete_success', [ - deleted.map((item) => item.title).join('\n') - ]) -}根据代码规范："a function is justified only when called from multiple distinct call sites with non-trivial logic"。
Also applies to: 780-808
🤖 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 `@packages/core/src/middlewares/system/conversation_manage.ts` around lines 283 - 293, The helper function deleteBySeqs is only invoked once at the call site guarded by context.options.conversation_manage?.targetConversationSeqs (the block that sets context.message and returns ChainMiddlewareRunStatus.STOP), so inline its logic directly into that block and remove the now-unnecessary deleteBySeqs function; locate the other similar single-use helper at the region referenced (around the other occurrence between lines 780-808) and apply the same inlining there, ensuring you preserve parameters used (ctx, session, seqs, presetLane, includeArchived, allPresetLanes) and the exact behavior and return value when assigning context.message.
🤖 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 `@packages/core/src/middlewares/system/conversation_manage.ts`:
- Around line 805-807: The bulk-delete branch uses
session.text('chatluna.conversation.messages.delete_success', [...]) with a
single parameter (deleted.map(...).join('\n')) while the single-delete flow
passes three parameters via conversationSummary ([title, seq, id]); unify these
so the i18n placeholders align by either (A) using a dedicated i18n key for bulk
deletes (e.g. 'chatluna.conversation.messages.delete_success_multi') and call
session.text with that key and the single joined title string, or (B) keep the
existing 'delete_success' key and pass the same three-argument shape from the
bulk branch (for example [joinedTitles, '', ''] or a concatenated seq/id string)
so the parameter count matches; update the call site (the session.text
invocation in the deleted.map(...).join(...) line) accordingly and add the new
i18n entry if you choose option A.
---
Nitpick comments:
In `@packages/core/src/commands/conversation.ts`:
- Around line 243-244: The helper function parseSeqs should be inlined where
it's only used: replace the call sites that assign seqs (e.g., const target =
conversation?.trim() || undefined; const seqs = target == null ? undefined :
parseSeqs(target)) by moving parseSeqs's logic directly into that expression so
seqs is computed without an extra function; do the same for the other occurrence
around the 421–440 block—remove the parseSeqs declaration and update callers
(reference symbols: parseSeqs, target, seqs) so there is no separate function
call and behavior remains identical.
In `@packages/core/src/middlewares/system/conversation_manage.ts`:
- Around line 283-293: The helper function deleteBySeqs is only invoked once at
the call site guarded by
context.options.conversation_manage?.targetConversationSeqs (the block that sets
context.message and returns ChainMiddlewareRunStatus.STOP), so inline its logic
directly into that block and remove the now-unnecessary deleteBySeqs function;
locate the other similar single-use helper at the region referenced (around the
other occurrence between lines 780-808) and apply the same inlining there,
ensuring you preserve parameters used (ctx, session, seqs, presetLane,
includeArchived, allPresetLanes) and the exact behavior and return value when
assigning context.message.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 83c3b84e-0a56-4955-96bb-b9daaf600ef9
📒 Files selected for processing (3)
packages/core/src/commands/conversation.tspackages/core/src/middlewares/system/conversation_manage.tspackages/core/tests/conversation-resolve.spec.ts
Comment on lines
+805
to
+807
| return session.text('chatluna.conversation.messages.delete_success', [ | ||
| deleted.map((item) => item.title).join('\n') | ||
| ]) |
Contributor
There was a problem hiding this comment.
i18n 消息参数不一致
批量删除和单个删除使用同一个 i18n 键 delete_success，但传递的参数不一致：
- 单个删除（303-305 行）：通过
conversationSummary传递[title, seq, id]三个参数 - 批量删除（805-807 行）：仅传递
[joined_titles]一个参数
如果 i18n 模板期望三个占位符（如 {0}, {1}, {2}），批量删除时访问 {1} 或 {2} 可能导致显示错误或 undefined。
建议统一参数格式，或为批量删除使用独立的 i18n 键。
🤖 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 `@packages/core/src/middlewares/system/conversation_manage.ts` around lines 805
- 807, The bulk-delete branch uses
session.text('chatluna.conversation.messages.delete_success', [...]) with a
single parameter (deleted.map(...).join('\n')) while the single-delete flow
passes three parameters via conversationSummary ([title, seq, id]); unify these
so the i18n placeholders align by either (A) using a dedicated i18n key for bulk
deletes (e.g. 'chatluna.conversation.messages.delete_success_multi') and call
session.text with that key and the single joined title string, or (B) keep the
existing 'delete_success' key and pass the same three-argument shape from the
bulk branch (for example [joinedTitles, '', ''] or a concatenated seq/id string)
so the parameter count matches; update the call site (the session.text
invocation in the deleted.map(...).join(...) line) accordingly and add the new
i18n entry if you choose option A.
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.
This pr adds batch sequence selectors to the
chatluna.deletecommand.New Features
1..3.1,3,5.Bug fixes
Other Changes
yarn lint-fixcompleted with no errors. Existing max-len warnings remain inread_chat_message.tsandextension-agent/src/sub-agent/builtin.ts.