fix(provider): pin auto-enabled providers to top#16750
Open
ousugo wants to merge 3 commits into
Open
Conversation
74c5277 to
d535f22
Compare
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.
What this PR does
Before this PR:
Providers that were automatically enabled after entering an API key could remain in their previous list position when the providers cache was not ready, because the reorder call could silently no-op.
After this PR:
Auto-enabled providers are moved to the top only after the provider list is ready for reordering. The shared enablement flow now asserts reorder readiness before enabling, uses a common move-to-first hook, and covers manual provider enablement as well.
Fixes # None
Why we need it and why it was done in this way
The following tradeoffs were made:
The provider is not enabled when the provider list is not ready to reorder. This avoids the inconsistent state where the provider is enabled but not pinned to the top.
The following alternatives were considered:
Allowing the enable operation to succeed and retrying reorder later was considered, but that would preserve a transient inconsistent UI state and require additional retry orchestration. Keeping the enable and reorder precondition in one flow is simpler and makes failure explicit.
Links to places where the discussion took place: N/A
Breaking changes
None
If this PR introduces breaking changes, please describe the changes and the impact on users.
Special notes for your reviewer
This PR intentionally leaves broader connection-check races and stale closure edge cases for a separate follow-up. The focused fix is to remove the silent cache-not-ready no-op from the provider auto-enable and manual enable paths.
Validation:
pnpm lintpassed.useProviderEnable,useMoveProviderToFirst,useProviderAutoModelSync,useProviderConnectionCheck,providerEnablement, and model pull reconcile tests.pnpm testran after retrying the local sqlite/better-sqlite3 issue, but the overall suite still has unrelated Windows path/mock failures in main-process tests.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note