fix(provider): remove deprecated laoshi provider - #1485
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded detection and removal of deprecated builtin providers and model-selection settings (including preferredModel), migrated legacy builtin agent selections into current presets when appropriate, and removed three builtin providers ( Changes
Sequence Diagram(s)sequenceDiagram
participant AppInit as App Init
participant Config as ConfigPresenter
participant Store as Persisted Store
participant Repo as AgentRepository
participant Bus as EventBus
AppInit->>Config: initialize()
Config->>Store: getProviders(), get model-selection settings
Store-->>Config: providers[], settings
Config->>Config: removeDeprecatedBuiltinProviders()
Config->>Store: setProviders(filteredProviders)
Config->>Config: getDeprecatedProviderModelSelectionKeysToClear(settings)
loop for each clearedKey
Config->>Store: delete clearedKey
Config->>Bus: emit CONFIG_EVENTS.SETTING_CHANGED (clearedKey, undefined)
end
AppInit->>Repo: attach repository
Repo->>Config: setAgentRepository()
Config->>Config: reconcileLegacyBuiltinAgentSelections()
alt legacy selections copied
Config->>Store: set builtin preset values
Config->>Bus: emit CONFIG_EVENTS.SETTING_CHANGED (as needed)
end
Config->>Config: cleanupDeprecatedBuiltinAgentSelections()
Config->>Store: update builtin presets to null if deprecated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ 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)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/main/presenter/configPresenter/deprecatedProviderCleanup.test.ts (1)
80-141: Solid coverage of the happy path — consider adding an idempotency / no-op case.The current suite verifies one full cleanup pass. A small additional case where
getProviders()returns no deprecated entries and all selections already point to live providers would lock in two useful invariants:setProvidersis not called, andeventBus.sendToMainis not fired. Cheap insurance against future regressions in the early-exit branches.💡 Suggested extra case
it('is a no-op when no deprecated providers or selections are present', () => { const selectionStore = new Map<string, unknown>([ ['defaultModel', { providerId: 'openai', modelId: 'gpt-4o' }] ]) const store = { get: vi.fn((key: string) => selectionStore.get(key)), delete: vi.fn() } const getProviders = vi.fn().mockReturnValue([createProvider('openai')]) const setProviders = vi.fn() const presenter = Object.assign(Object.create(ConfigPresenter.prototype), { store, getProviders, setProviders }) ;( presenter as ConfigPresenter & { cleanupDeprecatedBuiltinProviders: () => void } ).cleanupDeprecatedBuiltinProviders() expect(setProviders).not.toHaveBeenCalled() expect(store.delete).not.toHaveBeenCalled() expect(eventBus.sendToMain).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/configPresenter/deprecatedProviderCleanup.test.ts` around lines 80 - 141, Add a new unit test for ConfigPresenter.cleanupDeprecatedBuiltinProviders that asserts idempotency/no-op: create a selectionStore where selections point to live providers (e.g., providerId 'openai'), mock getProviders to return only live providers, and supply spies for setProviders and store.delete; call cleanupDeprecatedBuiltinProviders and assert setProviders.not.toHaveBeenCalled(), store.delete.not.toHaveBeenCalled(), and eventBus.sendToMain.not.toHaveBeenCalled() to ensure early-exit behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 947-966: Existing cleanup only removes deprecated selections from
the flat store in cleanupDeprecatedBuiltinProviders(), but persisted selections
inside the builtin DeepChat agent remain; add a new method (e.g.,
cleanupDeprecatedBuiltinAgentSelections) that calls getBuiltinDeepChatConfig(),
checks defaultModelPreset, assistantModel, and visionModel against
DEPRECATED_BUILTIN_PROVIDER_IDS, builds a partial DeepChatAgentConfig with those
fields nulled when deprecated, and calls updateBuiltinDeepChatConfig(updates) if
any changes are needed; invoke this new cleanup from setAgentRepository()
immediately after
initializeUnifiedAgents()/migrateLegacyDefaultVisionModelToBuiltinAgent() so
agent-stored deprecated selections are sanitized when the repository attaches.
---
Nitpick comments:
In `@test/main/presenter/configPresenter/deprecatedProviderCleanup.test.ts`:
- Around line 80-141: Add a new unit test for
ConfigPresenter.cleanupDeprecatedBuiltinProviders that asserts
idempotency/no-op: create a selectionStore where selections point to live
providers (e.g., providerId 'openai'), mock getProviders to return only live
providers, and supply spies for setProviders and store.delete; call
cleanupDeprecatedBuiltinProviders and assert
setProviders.not.toHaveBeenCalled(), store.delete.not.toHaveBeenCalled(), and
eventBus.sendToMain.not.toHaveBeenCalled() to ensure early-exit behavior is
preserved.
🪄 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
Run ID: d9376aa7-a85c-4f92-886a-b95d6b2a515e
📒 Files selected for processing (5)
src/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/providers.tssrc/shared/providerDeeplink.tstest/main/presenter/configPresenter/deprecatedProviderCleanup.test.tstest/manual/deeplink-playground.html
💤 Files with no reviewable changes (1)
- src/main/presenter/configPresenter/providers.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/presenter/configPresenter/index.ts (1)
262-274: Consider normalizingselection.providerIdfor consistency with sibling helpers.
isDeprecatedBuiltinProviderId(Line 204-207) andisDeprecatedBuiltinModelSelection(Line 209-218) both normalize provider IDs vianormalizeKnownProviderId(lowercases/trims/resolves aliases) before checking againstDEPRECATED_BUILTIN_PROVIDER_IDS. In contrast,getDeprecatedProviderModelSelectionKeysToCleardoes a rawdeprecatedProviderIdSet.has(selection.providerId), so any persisted entry with different casing, surrounding whitespace, or an alias would slip past the flat-store cleanup even though the agent-level cleanup would catch it. Using the same normalization here would make the two cleanup passes symmetric.♻️ Proposed refactor
export const getDeprecatedProviderModelSelectionKeysToClear = ( settings: Partial< Record<ProviderModelSettingKey, { providerId: string; modelId: string } | undefined> >, deprecatedProviderIds: readonly string[] = DEPRECATED_BUILTIN_PROVIDER_IDS ): ProviderModelSettingKey[] => { - const deprecatedProviderIdSet = new Set(deprecatedProviderIds) - return DEPRECATED_PROVIDER_MODEL_SETTING_KEYS.filter((key) => { const selection = settings[key] - return isModelSelection(selection) && deprecatedProviderIdSet.has(selection.providerId) + return isDeprecatedBuiltinModelSelection(selection, deprecatedProviderIds) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 262 - 274, getDeprecatedProviderModelSelectionKeysToClear currently compares selection.providerId raw against deprecatedProviderIdSet, causing mismatches for differing case/whitespace/aliases; update this function to normalize provider IDs the same way as isDeprecatedBuiltinProviderId/isDeprecatedBuiltinModelSelection by calling normalizeKnownProviderId on selection.providerId (and/or pre-normalizing deprecatedProviderIds into the set) before checking membership; reference getDeprecatedProviderModelSelectionKeysToClear, normalizeKnownProviderId, DEPRECATED_PROVIDER_MODEL_SETTING_KEYS, and isModelSelection to locate and make the change so the flat-store cleanup mirrors the agent-level cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 262-274: getDeprecatedProviderModelSelectionKeysToClear currently
compares selection.providerId raw against deprecatedProviderIdSet, causing
mismatches for differing case/whitespace/aliases; update this function to
normalize provider IDs the same way as
isDeprecatedBuiltinProviderId/isDeprecatedBuiltinModelSelection by calling
normalizeKnownProviderId on selection.providerId (and/or pre-normalizing
deprecatedProviderIds into the set) before checking membership; reference
getDeprecatedProviderModelSelectionKeysToClear, normalizeKnownProviderId,
DEPRECATED_PROVIDER_MODEL_SETTING_KEYS, and isModelSelection to locate and make
the change so the flat-store cleanup mirrors the agent-level cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60b812a2-2213-4464-9c69-761216c8d0cc
📒 Files selected for processing (2)
src/main/presenter/configPresenter/index.tstest/main/presenter/configPresenter/deprecatedProviderCleanup.test.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
2 participants
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.
fix(provider): remove deprecated laoshi provider
Summary by CodeRabbit