feat(settings): multi-select model deletion, regex search, dual-area model list#467
feat(settings): multi-select model deletion, regex search, dual-area model list#467Jacobinwwey wants to merge 10 commits into
Conversation
…models Add is_selected column to models table (default 1) to distinguish provider-supported models from user-selected models. Add deleteModels batch delete and updateModelsSelected batch update queries. New models fetched via refresh default to is_selected=0 (support area only). Signed-off-by: Jacob <jacob@example.com> Signed-off-by: Jacobinwwey <jacob.hxx.cn@outlook.com>
Add zh-CN and en-US translation keys for: multi-select mode toggle, batch delete confirmation/success, regex search toggle and validation, model support/selection area titles and descriptions, add-to-selection and remove-from-selection actions. Signed-off-by: Jacob <jacob@example.com> Signed-off-by: Jacobinwwey <jacob.hxx.cn@outlook.com>
…rea model list ModelList now splits into: - Model Selection Area: models the user has explicitly added (is_selected=1) - Model Support Area: all provider-supported models (is_selected=0) Features added: - Multi-select mode with checkbox selection, select-all, and batch delete - Regex search toggle (.* button in search bar) with error validation - Add-to-selection / remove-from-selection buttons on ModelCard - Batch delete with confirmation dialog blocks default model deletion ModelCard gains multiSelectMode/isSelected/area props and toggle-select, add-to-selection, remove-from-selection events. ModelGroup passes through multi-select state with group-level select/deselect. AiServices orchestrator wires batch-delete, add-to-selection, and remove-from-selection events to the new deleteModels and updateModelsSelected database queries. Signed-off-by: Jacob <jacob@example.com> Signed-off-by: Jacobinwwey <jacob.hxx.cn@outlook.com>
|
|
github-actions
Bot
added
area:frontend
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an ChangesModel Multi-Select Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 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 |
|
Thanks for your first pull request to TouchAI. 感谢你向 TouchAI 提交第一条 Pull Request。 A few quick checks before review:
Helpful links: Thanks for contributing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/desktop/src/database/drizzle/0004_model_is_selected.sql`:
- Line 1: Add a DB-level constraint and tighten the TypeScript types/callers:
change the migration that adds is_selected so the column is a 1-byte integer and
has a CHECK constraint limiting values to (0,1) (e.g. make it TINYINT/INTEGER
NOT NULL DEFAULT 1 and add CHECK(is_selected IN (0,1))); then update the code
types and caller to only allow 0|1: in the DB model types narrow
is_selected/isSelected to the union type 0 | 1 (ModelEntity, ModelCreateData,
ModelWithProvider) and adjust the updateModelsSelected function
signature/implementation to accept and persist only 0 or 1. Ensure any runtime
code that sets isSelected validates/coerces to 0 or 1 before persisting.
In
`@apps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vue`:
- Around line 159-163: The component keeps multiSelectMode and selectedModelIds
across provider switches causing hidden selections to persist; add a watcher on
the providerId prop inside ModelList (or the setup where
regexMode/multiSelectMode/selectedModelIds are declared) that resets
multiSelectMode.value = false and selectedModelIds.value = new Set() whenever
providerId changes (alternatively force a remount keyed by providerId), and
apply the same reset in the other place where those refs are declared so
selections are cleared when the parent switches providers.
In `@apps/desktop/src/views/SettingsView/components/AiServices/index.vue`:
- Around line 674-682: The handler handleRemoveFromSelection currently allows
clearing is_selected for any model id, including the protected defaultModelId;
change it to first check whether id === defaultModelId (or compare against the
current default model variable) and if so abort with a user-facing error (e.g.,
alert.error with a message asking to choose a new default first) without calling
updateModelsSelected/patchCachedModel/broadcastModelsUpdated; otherwise proceed
as before calling updateModelsSelected([id], 0), patchCachedModel(id, {
is_selected: 0 }), and broadcastModelsUpdated(). Ensure the check uses the same
defaultModelId reference used elsewhere so behavior matches the batch-delete
protection.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 079ba0ad-6c35-44fa-b45f-499b2c72462a
📒 Files selected for processing (10)
apps/desktop/src/database/drizzle/0004_model_is_selected.sqlapps/desktop/src/database/drizzle/meta/_journal.jsonapps/desktop/src/database/queries/models.tsapps/desktop/src/database/schema.tsapps/desktop/src/database/types/index.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/views/SettingsView/components/AiServices/components/ModelCard.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelGroup.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vueapps/desktop/src/views/SettingsView/components/AiServices/index.vue
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: CodeQL (rust)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
🔇 Additional comments (5)
apps/desktop/src/database/drizzle/meta/_journal.json (1)
32-38: LGTM!apps/desktop/src/database/schema.ts (1)
271-271: LGTM!apps/desktop/src/database/queries/models.ts (1)
3-3: LGTM!Also applies to: 40-40, 191-218
apps/desktop/src/database/types/index.ts (1)
228-228: LGTM!Also applies to: 250-250, 285-285
apps/desktop/src/i18n/messages.ts (1)
1417-1419: Usetp()(nott()) for pipe-delimited plural keys
apps/desktop/src/i18n/messages.tsdefinessettings.ai.confirmBatchDeleteandsettings.ai.batchDeleteSucceededas pipe-delimited plural strings (en-US), butModelList.vueandAiServices/index.vuecallt(..., { count: ids.length })for those keys. This repo provides a plural-count helpertp()inapps/desktop/src/i18n/index.tsand uses it for other count-based plural messages, so these call sites should switch totp()(or make the message definitions single-form).
Jacobinwwey
force-pushed
the
feat/model-list-multi-select-regex-search
branch
from
6892df1 to
7d11a8f
Compare
- Add CHECK(is_selected IN (0,1)) constraint to migration - Revert is_selected from 0|1 to number (matching Drizzle inference) - Reset multiSelectMode and selectedModelIds when providerId changes - Block remove-from-selection for the default model - Switch confirmBatchDelete/batchDeleteSucceeded to tp() for plurals - Update ModelCard test for new button order (remove-from-selection first) - Add is_selected field to test model factories Signed-off-by: Jacob <jacob@example.com> Signed-off-by: Jacobinwwey <jacob.hxx.cn@outlook.com>
Jacobinwwey
force-pushed
the
feat/model-list-multi-select-regex-search
branch
from
7d11a8f to
384249e
Compare
Add is_selected: 1 to all ModelWithProvider factory functions in test files. This field was added in migration 0004 but test mocks were not updated, causing test:typecheck to fail in CI.
|
Thank you for your contribution; this is a very useful feature. However, in manual testing I found that even though it has not been added to the selection area by the provider, it still appears in the model selection popup, which seems inconsistent with expectations. |
|
And another bug: the model of supporting areas can be set as default directly, but they won’t automatically transition into the selection area. |
There was a problem hiding this comment.
This database migration should be automatically generated by drizzle-kit rather than manually added, as manual additions often miss snapshot files belike now.
| <span class="text-xs text-neutral-400">({{ selectionModels.length }})</span> | ||
| </div> | ||
| <p class="text-xs text-neutral-400"> | ||
| {{ t('settings.ai.modelSelectionAreaDescription') }} |
There was a problem hiding this comment.
I don’t think this description is necessary.
| 'settings.ai.regexSearch': '正则搜索', | ||
| 'settings.ai.regexSearchExit': '退出正则', | ||
| 'settings.ai.invalidRegex': '正则表达式无效：{error}', | ||
| 'settings.ai.modelSupportArea': '模型支持区域', |
There was a problem hiding this comment.
Wouldn't it be more appropriate to change it to “已选模型” / “全部模型”? and "添加" rather than "添加到选择区域"
|
Additionally, we're discussing a question that should have been addressed during the issue phase: since this PR adds too many buttons, would it be more appropriate to switch to icons? What do you think? |
Improvements based on @hiqiancheng's review: 1. **i18n improvements**: Simplified area labels and action text - "模型支持区域" → "全部模型" (Model support area → All models) - "模型选择区域" → "已选模型" (Model selection area → Selected models) - "添加到选择区域" → "添加" (Add to selection → Add) - "从选择区域移除" → "移除" (Remove from selection → Remove) - Removed unnecessary description text below area headers 2. **Migration metadata**: Added missing snapshot file for 0004 - Generated 0004_snapshot.json using drizzle-kit - Ensures migration metadata is complete and matches drizzle-kit expectations All previously addressed CodeRabbit feedback remains fixed: - CHECK constraint in migration (is_selected IN (0,1)) - providerId watcher resets multi-select state - Default model guard in handleRemoveFromSelection
…text Update test expectation from "Remove from selection" to "Remove" to match the simplified i18n text requested by reviewer.
✅ All Reviewer Feedback AddressedThank you @coderabbitai and @hiqiancheng for the thorough reviews! I've addressed all feedback points: 📝 @hiqiancheng's Feedback1. ✅ Migration snapshot file (commit c4febbf)
2. ✅ Simplified i18n labels (commits c4febbf, 40fd833)
3. ✅ Removed unnecessary descriptions (commit c4febbf)
🤖 @coderabbitai's Feedback (Previously Addressed)All three major issues were already fixed in commit 6892df1: 1. ✅ Database CHECK constraint
2. ✅ Multi-select state reset on provider switch
3. ✅ Default model protection in single remove
🧪 Test ResultsAll 976 tests pass locally and in CI:
📦 Final Commits
Ready for re-review! 🚀 |
|
Triggering a fresh review now. (◕‿◕✿) ✅ Action performedReview finished.
|
Jacobinwwey
force-pushed
the
feat/model-list-multi-select-regex-search
branch
from
40fd833 to
bff5abd
Compare
This commit fixes two functional bugs identified by @hiqiancheng: 1. **Bug: Unselected models appear in SearchView model picker** - Root cause: findModelsWithProvider() didn't filter by is_selected - Fix: Added WHERE is_selected = 1 to the query - Impact: SearchView model dropdown now only shows models in "selection area" 2. **Bug: Setting support-area model as default doesn't move it to selection area** - Root cause: handleSetDefaultModel() only updated is_default, not is_selected - Fix: Auto-call updateModelsSelected([id], 1) before setting default - Impact: Setting any model as default now ensures it's in selection area Note: Icon buttons were already implemented in the original PR (commit 3506f1b), so no changes needed for the "too many buttons" design concern. All 976 tests pass.
Jacobinwwey
force-pushed
the
feat/model-list-multi-select-regex-search
branch
from
e5cdf74 to
3e96871
Compare
|
@hiqiancheng Thank you for the thorough review! I've addressed all three of your comments: 1. Icon Buttons vs Text ButtonsAlready implemented in the original PR (commit 3506f1b). All action buttons use
2. Bug: Unselected models appear in SearchView dropdownFixed in commit 3e96871. The issue was in // Before: no filtering
const whereConditions = [];
if (providerId !== undefined) {
whereConditions.push(eq(models.provider_id, providerId));
}
// After: filter out unselected models
const whereConditions = [eq(models.is_selected, 1)];
if (providerId !== undefined) {
whereConditions.push(eq(models.provider_id, providerId));
}3. Bug: Setting default from support area doesn't auto-selectFixed in commit 3e96871. Added auto-selection logic in const handleSetDefaultModel = async (id: number) => {
try {
const nextDefaultModel = findCachedModel(id);
// If the model is not in selection area, add it first
if (nextDefaultModel && nextDefaultModel.is_selected === 0) {
await updateModelsSelected([id], 1);
}
await setDefaultModel({ modelId: id });
// ... rest of the logic
}
}All 976 tests pass. Ready for re-review! |
|
@Jacobinwwey Yes, but I’m referring to those buttons that are placed side by side with the search bar. Since several buttons have been added, the original space has been squeezed — using icons seems like a better choice? I’d like to discuss this with you; please don’t have the agent start coding right away. What do you think? |
@hiqiancheng |
…ti-select-regex-search # Conflicts: # apps/desktop/tests/services/AgentService/catalog/providers.test.ts
3 participants
Summary
1. Context & Motivation (Context)
is_selectedcolumn to themodelstable (schema migration required). This is a lightweight boolean flag approach rather than a separate "wishlist" table — chosen for simplicity and to avoid JOIN overhead. New models from provider refresh default tois_selected=0, requiring an explicit user action to add them to the selection area.2. Implementation Details (Implementation)
is_selectedinteger column (default 1) tomodelstable withCHECK(is_selected IN (0,1))constraint. Existing models retainis_selected=1to preserve backward compatibility.deleteModels(ids[])batch delete andupdateModelsSelected(ids[], isSelected)batch update. Both accept optionalDatabaseExecutorfor transaction participation.multiSelectMode,isSelected,areaprops. In multi-select mode: checkbox replaces radio, card click toggles selection. In support area: shows "+" (add-to-selection). In selection area: shows "−" (remove-from-selection).is_selected=1) and Support Area (is_selected=0). Regex toggle (".*" button) with validation. Multi-select state resets on provider switch viawatch(providerId). Batch delete blocks default model.showEditDialogintoshowEditProviderDialog.3. Testing Strategies (Validation)
vue-tsc --noEmit)is_selected=1Related issue or RFC
Related to #467 - User-requested feature for better model management workflow
AI assistance disclosure
This PR was developed with assistance from Claude Code (Claude Opus 4.8).
Testing evidence
Risk notes
is_selectedcolumn with CHECK constraintis_selected=1for backward compatibilityis_selected=0Screenshots or recordings
N/A - Backend/logic feature with UI state changes visible in existing Settings view
Checklist
🤖 Generated with Claude Code