refactor: no derived state#1886
Open
mfortman11 wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughAcross 13 frontend files, ChangesuseEffect → Render-time prev-state Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
![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: 2
🤖 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/onboarding/_components/anthropic-onboarding.tsx`:
- Around line 33-36: The synchronization block that updates state when
hasEnvApiKey changes is missing the cleanup logic that handleGetFromEnvChange
applies. Instead of directly calling setGetFromEnv(hasEnvApiKey), invoke
handleGetFromEnvChange(hasEnvApiKey) to ensure that when the environment API key
availability changes, any stale manual API keys or selected models are properly
cleaned up alongside the getFromEnv state change, preventing UI inconsistencies.
In `@frontend/app/onboarding/_hooks/useModelSelection.ts`:
- Around line 11-38: The default model selection in useModelSelection.ts (lines
11-38, the prevModelsData state and conditions checking languageModel and
embeddingModel) only runs when modelsData reference changes, missing cached data
on first render. Initialize prevModelsData to a sentinel value (like undefined
or a Symbol) instead of modelsData so the default selection logic runs
immediately when cached data is available. Additionally, in onboarding-card.tsx
(lines 88-129), initialize the auto-selection marker to a sentinel value and add
isEmbedding and isCloudBrand to the dependency array of any effect that tracks
these inputs, ensuring cached settings and context changes properly recompute
the provider selection.
🪄 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: 32154f3a-9bf6-424a-a410-e0dd027f3ceb
📒 Files selected for processing (13)
frontend/app/chat/_components/function-calls/container.tsxfrontend/app/onboarding/_components/animated-provider-steps.tsxfrontend/app/onboarding/_components/anthropic-onboarding.tsxfrontend/app/onboarding/_components/model-selector.tsxfrontend/app/onboarding/_components/onboarding-card.tsxfrontend/app/onboarding/_hooks/useModelSelection.tsfrontend/app/settings/_components/s3-settings-dialog.tsxfrontend/components/chat-renderer.tsxfrontend/components/knowledge-search-bar.tsxfrontend/components/knowledge-search-input.tsxfrontend/components/navigation.tsxfrontend/components/task-dialog/use-task-dialog.tsfrontend/contexts/knowledge-filter-context.tsx
Comment on lines
+33
to
+36
| if (hasEnvApiKey !== prevHasEnvApiKey) { | ||
| setPrevHasEnvApiKey(hasEnvApiKey); | ||
| setGetFromEnv(hasEnvApiKey); | ||
| } |
Contributor
There was a problem hiding this comment.
Apply the same cleanup when hasEnvApiKey changes.
This render-time sync flips getFromEnv without the cleanup in handleGetFromEnvChange, so a prop-driven switch can retain a stale manual key or selected models while the UI says it is using the environment key.
Proposed fix
- if (hasEnvApiKey !== prevHasEnvApiKey) {
- setPrevHasEnvApiKey(hasEnvApiKey);
- setGetFromEnv(hasEnvApiKey);
- }
const debouncedApiKey = useDebouncedValue(apiKey, 500);
// Fetch models from API when API key is provided
@@
embeddingModels,
} = useModelSelection(modelsData, isEmbedding);
+
+ if (hasEnvApiKey !== prevHasEnvApiKey) {
+ setPrevHasEnvApiKey(hasEnvApiKey);
+ setGetFromEnv(hasEnvApiKey);
+ if (hasEnvApiKey) {
+ setApiKey("");
+ }
+ setEmbeddingModel?.("");
+ setLanguageModel?.("");
+ }
const handleGetFromEnvChange = (fromEnv: boolean) => {🤖 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/onboarding/_components/anthropic-onboarding.tsx` around lines 33
- 36, The synchronization block that updates state when hasEnvApiKey changes is
missing the cleanup logic that handleGetFromEnvChange applies. Instead of
directly calling setGetFromEnv(hasEnvApiKey), invoke
handleGetFromEnvChange(hasEnvApiKey) to ensure that when the environment API key
availability changes, any stale manual API keys or selected models are properly
cleaned up alongside the getFromEnv state change, preventing UI inconsistencies.
Comment on lines
+11
to
+38
| // Set default selections when models first load (render-time adjustment) | ||
| const [prevModelsData, setPrevModelsData] = useState(modelsData); | ||
| if (modelsData !== prevModelsData) { | ||
| setPrevModelsData(modelsData); | ||
| if (modelsData) { | ||
| const defaultLangModel = isEmbedding | ||
| ? undefined | ||
| : modelsData.language_models.find((m) => m.default); | ||
| const defaultEmbedModel = isEmbedding | ||
| ? modelsData.embedding_models.find((m) => m.default) | ||
| : undefined; | ||
|
|
||
| // Set language model: prefer default, fallback to first available | ||
| if (!languageModel && !isEmbedding) { | ||
| const defaultLangModel = modelsData.language_models.find( | ||
| (m) => m.default, | ||
| ); | ||
| if (defaultLangModel) { | ||
| setLanguageModel(defaultLangModel.value); | ||
| } else if (modelsData.language_models.length > 0) { | ||
| setLanguageModel(modelsData.language_models[0].value); | ||
| } | ||
| } | ||
|
|
||
| // Set embedding model: prefer default, fallback to first available | ||
| if (!embeddingModel && isEmbedding) { | ||
| const defaultEmbedModel = modelsData.embedding_models.find( | ||
| (m) => m.default, | ||
| ); | ||
| if (defaultEmbedModel) { | ||
| setEmbeddingModel(defaultEmbedModel.value); | ||
| } else if (modelsData.embedding_models.length > 0) { | ||
| setEmbeddingModel(modelsData.embedding_models[0].value); | ||
| } | ||
| } | ||
| } | ||
| }, [modelsData, languageModel, embeddingModel, isEmbedding]); | ||
| } |
Contributor
There was a problem hiding this comment.
The new prev-state guards skip initial cached React Query data. Initializing the “previous” value from the current upstream value means the render-time adjustment only runs after a later reference change, not when data is already available on first render.
frontend/app/onboarding/_hooks/useModelSelection.ts#L11-L38: guard default selection by “selection is empty and a default exists,” or initialize the previous-data marker to a sentinel so cachedmodelsDatastill selects defaults.frontend/app/onboarding/_components/onboarding-card.tsx#L88-L129: initialize the auto-selection marker to a sentinel and includeisEmbedding/isCloudBrandin the tracked inputs so cached settings and context changes recompute provider selection.
📍 Affects 2 files
frontend/app/onboarding/_hooks/useModelSelection.ts#L11-L38(this comment)frontend/app/onboarding/_components/onboarding-card.tsx#L88-L129
🤖 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/onboarding/_hooks/useModelSelection.ts` around lines 11 - 38,
The default model selection in useModelSelection.ts (lines 11-38, the
prevModelsData state and conditions checking languageModel and embeddingModel)
only runs when modelsData reference changes, missing cached data on first
render. Initialize prevModelsData to a sentinel value (like undefined or a
Symbol) instead of modelsData so the default selection logic runs immediately
when cached data is available. Additionally, in onboarding-card.tsx (lines
88-129), initialize the auto-selection marker to a sentinel value and add
isEmbedding and isCloudBrand to the dependency array of any effect that tracks
these inputs, ensuring cached settings and context changes properly recompute
the provider selection.
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.
Every fix follows the same React-recommended technique: adjust state during render instead of in a useEffect. Compare the previous value of the upstream data; if it changed, call setState synchronously during render. React batches these updates and re-invokes the component before committing to the DOM, so the stale frame is never painted.
Summary by CodeRabbit
Release Notes
Refactor
Bug Fixes