fix: Set Model after Provider is deleted#1935
Open
mfortman11 wants to merge 7 commits into
Open
Conversation
github-actions
Bot
added
frontend
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughBackend helper functions ChangesDefault Model Fallback on Provider Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
github-actions
Bot
added
bug
![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: 4
🧹 Nitpick comments (1)
tests/unit/test_settings_provider_removal_defaults.py (1)
250-255: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test for “no embedding-capable fallback” removal.
This simulator only validates happy-path fallback. Add a case where only Anthropic remains configured and embedding provider is removed, then assert the flow rejects fallback (or raises) instead of silently selecting an unconfigured provider.
🤖 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 `@tests/unit/test_settings_provider_removal_defaults.py` around lines 250 - 255, The `_simulate_embedding_removal` method in the test currently only validates the happy-path scenario where a valid fallback embedding provider exists. Add a new test case that configures only Anthropic as the embedding provider (which is not embedding-capable), then calls `_simulate_embedding_removal` to simulate removing that provider, and assert that the method either raises an exception or explicitly rejects the fallback instead of silently accepting an unconfigured provider. This regression test ensures the code does not silently select invalid fallback providers when no embedding-capable alternatives remain.
🤖 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 `@frontend/app/settings/_components/agent-settings-section.tsx`:
- Around line 136-144: The useEffect that handles auto-selecting a default LLM
model is re-triggering multiple times because allLlmOptions is included in the
dependency array but is recreated on every render (since groupedLlmModels isn't
memoized). When model data loads incrementally, allLlmOptions changes and the
effect re-runs, and since the initial mutation is still pending and
settings.agent?.llm_model is still undefined, handleModelChange fires again
causing duplicate mutations. Fix this by either memoizing groupedLlmModels using
useMemo so it only changes when its actual dependencies change, or by using a
useRef to track whether the auto-select has already been initiated for the
current undefined llm_model state and skip re-running the effect if it has
already been triggered.
In `@frontend/app/settings/_components/ingest-settings-section.tsx`:
- Around line 127-139: The useEffect hook that sets the fallback embedding model
can execute multiple times because allEmbeddingOptions is recreated on every
render and included in the dependency array, causing handleEmbeddingModelChange
to be called repeatedly while settings.knowledge?.embedding_model is empty. Add
a one-shot guard using useRef to track whether the fallback has already been
applied, and only allow the fallback assignment to occur once. Check the ref at
the beginning of the effect to skip execution if the fallback has already been
set, and set the ref to true after the first successful call to
handleEmbeddingModelChange.
In `@src/api/settings/endpoints.py`:
- Around line 643-645: The `_first_configured_embedding_provider` function calls
at multiple locations (around lines 643, 675, and 732) can still return "openai"
as a fallback even when OpenAI is not configured or has been removed, resulting
in an invalid embedding_provider/credentials combination. Fix this by ensuring
that before assigning the result of `_first_configured_embedding_provider` to
`current_config.knowledge.embedding_provider`, you validate that the returned
provider is actually configured in the current configuration. If the returned
provider is not configured, either skip the assignment, select a different valid
provider, or raise an appropriate error to prevent persisting an invalid
configuration state.
In `@tests/unit/test_settings_provider_removal_defaults.py`:
- Line 8: Remove the unused import statement `import pytest` from the file
tests/unit/test_settings_provider_removal_defaults.py. This import is not
referenced anywhere in the test file and is causing a Ruff F401 linting error
that blocks the lint job from passing.
---
Nitpick comments:
In `@tests/unit/test_settings_provider_removal_defaults.py`:
- Around line 250-255: The `_simulate_embedding_removal` method in the test
currently only validates the happy-path scenario where a valid fallback
embedding provider exists. Add a new test case that configures only Anthropic as
the embedding provider (which is not embedding-capable), then calls
`_simulate_embedding_removal` to simulate removing that provider, and assert
that the method either raises an exception or explicitly rejects the fallback
instead of silently accepting an unconfigured provider. This regression test
ensures the code does not silently select invalid fallback providers when no
embedding-capable alternatives remain.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c73dbc05-5338-411d-9112-ca55705c5338
📒 Files selected for processing (6)
frontend/app/settings/_components/agent-settings-section.tsxfrontend/app/settings/_components/ingest-settings-section.tsxfrontend/lib/constants.tssrc/api/settings/endpoints.pysrc/api/settings/helpers.pytests/unit/test_settings_provider_removal_defaults.py
Comment on lines
+127
to
+139
| const allEmbeddingOptions = groupedEmbeddingModels.flatMap((g) => g.options); | ||
|
|
||
| useEffect(() => { | ||
| if ( | ||
| !settings.knowledge?.embedding_model && | ||
| allEmbeddingOptions.length > 0 | ||
| ) { | ||
| const fallback = | ||
| allEmbeddingOptions.find((o) => o.default) || allEmbeddingOptions[0]; | ||
| handleEmbeddingModelChange(fallback.value, fallback.provider); | ||
| } | ||
| }, [settings.knowledge?.embedding_model, allEmbeddingOptions]); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/app/settings/_components/ingest-settings-section.tsx | head -200Repository: langflow-ai/openrag
Length of output: 8541
🏁 Script executed:
cat -n frontend/app/settings/_components/ingest-settings-section.tsx | tail -n +200 | head -100Repository: langflow-ai/openrag
Length of output: 4069
🏁 Script executed:
rg "isPending" frontend/app/settings/_components/ -A 2 -B 2Repository: langflow-ai/openrag
Length of output: 10244
🏁 Script executed:
rg "useUpdateSettingsMutation" -A 20 frontend/app/api/mutations/Repository: langflow-ai/openrag
Length of output: 1847
🏁 Script executed:
rg "useEffect.*allEmbeddingOptions|const allEmbeddingOptions" frontend/app/settings/ -B 3 -A 5Repository: langflow-ai/openrag
Length of output: 828
🏁 Script executed:
rg "lastAutoSelected|auto.*select.*fallback|flatMap.*options" frontend/ --type tsx --type tsRepository: langflow-ai/openrag
Length of output: 91
🏁 Script executed:
rg "auto.*select|fallback.*effect|lastAutoSelected" frontend/app/settings/ -A 3 -B 2Repository: langflow-ai/openrag
Length of output: 950
🏁 Script executed:
cat -n frontend/app/settings/_components/agent-settings-section.tsx | head -150Repository: langflow-ai/openrag
Length of output: 6303
Add a one-shot guard to prevent repeated embedding fallback writes.
The fallback effect can repeatedly call handleEmbeddingModelChange(...) while settings.knowledge?.embedding_model is empty because allEmbeddingOptions (line 127) is recreated on every render and included in the dependency array (line 138). While the mutation is pending, the condition remains true and the effect can re-run, creating duplicate API calls and toasts.
💡 Suggested fix
+ import { useEffect, useRef, useState } from "react";
- import { useEffect, useState } from "react";+ const lastAutoSelectedEmbeddingRef = useRef<string | null>(null);
const allEmbeddingOptions = groupedEmbeddingModels.flatMap((g) => g.options);
useEffect(() => {
+ if (settings.knowledge?.embedding_model) {
+ lastAutoSelectedEmbeddingRef.current = null;
+ return;
+ }
+ if (allEmbeddingOptions.length === 0 || updateSettingsMutation.isPending) return;
+
const fallback =
allEmbeddingOptions.find((o) => o.default) || allEmbeddingOptions[0];
- if (
- !settings.knowledge?.embedding_model &&
- allEmbeddingOptions.length > 0
- ) {
- handleEmbeddingModelChange(fallback.value, fallback.provider);
- }
+ const fallbackKey = `${fallback.provider}:${fallback.value}`;
+ if (lastAutoSelectedEmbeddingRef.current === fallbackKey) return;
+
+ lastAutoSelectedEmbeddingRef.current = fallbackKey;
+ handleEmbeddingModelChange(fallback.value, fallback.provider);
}, [
settings.knowledge?.embedding_model,
allEmbeddingOptions,
+ updateSettingsMutation.isPending,
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const allEmbeddingOptions = groupedEmbeddingModels.flatMap((g) => g.options); | |
| useEffect(() => { | |
| if ( | |
| !settings.knowledge?.embedding_model && | |
| allEmbeddingOptions.length > 0 | |
| ) { | |
| const fallback = | |
| allEmbeddingOptions.find((o) => o.default) || allEmbeddingOptions[0]; | |
| handleEmbeddingModelChange(fallback.value, fallback.provider); | |
| } | |
| }, [settings.knowledge?.embedding_model, allEmbeddingOptions]); | |
| import { useEffect, useRef, useState } from "react"; | |
| const lastAutoSelectedEmbeddingRef = useRef<string | null>(null); | |
| const allEmbeddingOptions = groupedEmbeddingModels.flatMap((g) => g.options); | |
| useEffect(() => { | |
| if (settings.knowledge?.embedding_model) { | |
| lastAutoSelectedEmbeddingRef.current = null; | |
| return; | |
| } | |
| if (allEmbeddingOptions.length === 0 || updateSettingsMutation.isPending) return; | |
| const fallback = | |
| allEmbeddingOptions.find((o) => o.default) || allEmbeddingOptions[0]; | |
| const fallbackKey = `${fallback.provider}:${fallback.value}`; | |
| if (lastAutoSelectedEmbeddingRef.current === fallbackKey) return; | |
| lastAutoSelectedEmbeddingRef.current = fallbackKey; | |
| handleEmbeddingModelChange(fallback.value, fallback.provider); | |
| }, [ | |
| settings.knowledge?.embedding_model, | |
| allEmbeddingOptions, | |
| updateSettingsMutation.isPending, | |
| ]); |
🤖 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 `@frontend/app/settings/_components/ingest-settings-section.tsx` around lines
127 - 139, The useEffect hook that sets the fallback embedding model can execute
multiple times because allEmbeddingOptions is recreated on every render and
included in the dependency array, causing handleEmbeddingModelChange to be
called repeatedly while settings.knowledge?.embedding_model is empty. Add a
one-shot guard using useRef to track whether the fallback has already been
applied, and only allow the fallback assignment to occur once. Check the ref at
the beginning of the effect to skip execution if the fallback has already been
set, and set the ref to true after the first successful call to
handleEmbeddingModelChange.
Comment on lines
+643
to
+645
| fb = _first_configured_embedding_provider(current_config, "ollama") | ||
| current_config.knowledge.embedding_provider = fb | ||
| current_config.knowledge.embedding_model = _default_embedding_model(fb) |
Contributor
There was a problem hiding this comment.
Prevent embedding fallback from selecting an unconfigured provider.
At Line 643, Line 675, and Line 732, fallback embedding provider selection can still resolve to "openai" even when OpenAI was just removed (or never configured). That can persist an invalid embedding_provider/credentials combination in config.
Suggested fix
# remove_ollama_config branch
if current_config.knowledge.embedding_provider == "ollama":
fb = _first_configured_embedding_provider(current_config, "ollama")
+ if not getattr(current_config.providers, fb).configured:
+ return JSONResponse(
+ {"error": "Cannot remove Ollama configuration: configure another embedding provider first."},
+ status_code=400,
+ )
current_config.knowledge.embedding_provider = fb
current_config.knowledge.embedding_model = _default_embedding_model(fb)
# remove_openai_config branch
if current_config.knowledge.embedding_provider == "openai":
fb = _first_configured_embedding_provider(current_config, "openai")
+ if not getattr(current_config.providers, fb).configured:
+ return JSONResponse(
+ {"error": "Cannot remove OpenAI configuration: configure another embedding provider first."},
+ status_code=400,
+ )
current_config.knowledge.embedding_provider = fb
current_config.knowledge.embedding_model = _default_embedding_model(fb)
# remove_watsonx_config branch
if current_config.knowledge.embedding_provider == "watsonx":
fb = _first_configured_embedding_provider(current_config, "watsonx")
+ if not getattr(current_config.providers, fb).configured:
+ return JSONResponse(
+ {"error": "Cannot remove IBM watsonx.ai configuration: configure another embedding provider first."},
+ status_code=400,
+ )
current_config.knowledge.embedding_provider = fb
current_config.knowledge.embedding_model = _default_embedding_model(fb)Also applies to: 675-677, 732-734
🤖 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 `@src/api/settings/endpoints.py` around lines 643 - 645, The
`_first_configured_embedding_provider` function calls at multiple locations
(around lines 643, 675, and 732) can still return "openai" as a fallback even
when OpenAI is not configured or has been removed, resulting in an invalid
embedding_provider/credentials combination. Fix this by ensuring that before
assigning the result of `_first_configured_embedding_provider` to
`current_config.knowledge.embedding_provider`, you validate that the returned
provider is actually configured in the current configuration. If the returned
provider is not configured, either skip the assignment, select a different valid
provider, or raise an appropriate error to prevent persisting an invalid
configuration state.
…-ai/openrag into model-sel-after-provider-delete
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Collaborator
Functional Review 1
|
mpawlow
approved these changes
Jun 23, 2026
mpawlow
left a comment
mpawlow
left a comment
Collaborator
There was a problem hiding this comment.
Code Review 1
- ✅ Approved / LGTM 🚀
- See PR comment (1a) for potential / optional consideration
| [groupedLlmModels], | ||
| ); | ||
|
|
||
| useEffect(() => { |
Collaborator
There was a problem hiding this comment.
(1a) [Minor] Fallback useEffect can fire duplicate save mutations during incremental model loading
Problem
- The new effects run
handleModelChange/handleEmbeddingModelChangewhenever!llm_model && options.length > 0. allLlmOptions/allEmbeddingOptionsare memoized off the per-provider query results. Those queries resolve at different times (OpenAI, Anthropic, Ollama, watsonx), so the memo identity changes several times during initial load.- Each identity change re-runs the effect. Until the in-flight
updateSettingsMutationcompletes andsettingsis refetched,settings.agent?.llm_modelis still empty, so the guard passes again and a second/third mutation fires. - Net effect: multiple redundant
PATCH /settingscalls (and multiple "Settings updated successfully" toasts) for a single intended default-selection.
Background Information
- CodeRabbit flagged this; its stated premise ("
groupedLlmModelsisn't memoized") is now outdated since the PR memoizes both lists — but the residual race remains because the memo inputs themselves change as queries resolve.
Code References
frontend/app/settings/_components/ingest-settings-section.tsx:127-139(approx.)
Potential Solution
- Add a one-shot
useRefguard so the auto-select fires at most once per empty-model state:
const autoSelectedLlm = useRef(false);
useEffect(() => {
if (autoSelectedLlm.current) return;
if (!settings.agent?.llm_model && allLlmOptions.length > 0) {
autoSelectedLlm.current = true;
const fallback = allLlmOptions.find((o) => o.default) || allLlmOptions[0];
handleModelChange(fallback.value, fallback.provider);
}
}, [settings.agent?.llm_model, allLlmOptions]);Alternative Solutions
- Gate the effect on
!isLoadingAnyLlmModelsso it only runs once all provider queries have settled (single, stableallLlmOptions). - Track
updateSettingsMutation.isPendingand skip while a mutation is in flight.
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.
How to replicate and test
Summary by CodeRabbit
Release Notes
New Features
Updates
Bug Fixes
Tests