refactor: Fix ui flashes from on prop changes#1812
Open
mfortman11 wants to merge 5 commits into
Open
Conversation
github-actions
Bot
added
community
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFive frontend components refactor state management from useEffect-based initialization and effects to render-time conditionals and local state tracking. AnimatedProviderSteps detects completion transitions to set elapsed time, OnboardingStep synchronizes text/visibility/completion state during render, UnifiedCloudPicker inlines OneDrive URL detection, and TaskDialogFileList and useTaskDialog hook consolidate state resets into conditional blocks instead of effect watchers. ChangesRender-time state synchronization refactor
Possibly related PRs
Suggested reviewers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/cloud-picker/unified-cloud-picker.tsx (1)
31-31:⚠️ Potential issue | 🟠 MajorFix stale
effectiveBaseUrlcaused by persistedautoBaseUrlwhen switching providers
unified-cloud-picker.tsxstores OneDrive’s fallback in component state (autoBaseUrl) and deriveseffectiveBaseUrlasbaseUrl || autoBaseUrl. Since there’s no reset whenproviderchanges away from"onedrive",createProviderHandler(provider, ..., effectiveBaseUrl)can receive the OneDrive picker URL for subsequent providers whilebaseUrlis still unset.Suggested fix
- const [autoBaseUrl, setAutoBaseUrl] = useState<string | undefined>(undefined); - - const effectiveBaseUrl = baseUrl || autoBaseUrl; - - if (provider === "onedrive" && !baseUrl && accessToken && !autoBaseUrl) { - setAutoBaseUrl("https://onedrive.live.com/picker"); - } + const effectiveBaseUrl = + baseUrl ?? + (provider === "onedrive" && accessToken + ? "https://onedrive.live.com/picker" + : undefined);🤖 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/components/cloud-picker/unified-cloud-picker.tsx` at line 31, The stale OneDrive URL comes from storing OneDrive’s fallback in state (autoBaseUrl) and using effectiveBaseUrl = baseUrl || autoBaseUrl even after provider changes; update the component so autoBaseUrl is cleared or ignored when provider !== "onedrive" — e.g., add a useEffect that watches provider and sets setAutoBaseUrl(undefined) whenever provider changes away from "onedrive", or only pass a OneDrive-specific effectiveBaseUrl into createProviderHandler(provider, ..., effectiveBaseUrl) by computing effectiveBaseUrl conditionally (use baseUrl || autoBaseUrl only if provider === "onedrive"); ensure references to autoBaseUrl, effectiveBaseUrl and createProviderHandler are updated accordingly.
🤖 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/animated-provider-steps.tsx`:
- Around line 30-38: The component seeds prevIsCompleted incorrectly and never
handles the true->false transition; change initialization of prevIsCompleted so
it does not start from the incoming isCompleted prop (initialize false) and
replace the inline if-block with a useEffect that watches isCompleted and
processingStartTime: when isCompleted becomes true && prevIsCompleted is false,
setElapsedTime(Date.now() - processingStartTime) (guarding for undefined
processingStartTime) and setPrevIsCompleted(true); when isCompleted becomes
false, setPrevIsCompleted(false) so subsequent completions recompute
elapsedTime; update references to setPrevIsCompleted and setElapsedTime inside
that effect.
In `@frontend/app/onboarding/_components/onboarding-step.tsx`:
- Around line 38-40: On mount OnboardingStep currently initializes displayedText
and showChildren to empty/false which causes blank render when isVisible &&
isCompleted && !isMarkdown; change the initial state setup so displayedText
initializes to text and showChildren to true when props.isVisible &&
props.isCompleted && !props.isMarkdown (keep prev as before), and ensure the
typing useEffect still early-returns for completed cases—this guarantees
immediate full text and children on first render for the non-markdown completed
case.
---
Outside diff comments:
In `@frontend/components/cloud-picker/unified-cloud-picker.tsx`:
- Line 31: The stale OneDrive URL comes from storing OneDrive’s fallback in
state (autoBaseUrl) and using effectiveBaseUrl = baseUrl || autoBaseUrl even
after provider changes; update the component so autoBaseUrl is cleared or
ignored when provider !== "onedrive" — e.g., add a useEffect that watches
provider and sets setAutoBaseUrl(undefined) whenever provider changes away from
"onedrive", or only pass a OneDrive-specific effectiveBaseUrl into
createProviderHandler(provider, ..., effectiveBaseUrl) by computing
effectiveBaseUrl conditionally (use baseUrl || autoBaseUrl only if provider ===
"onedrive"); ensure references to autoBaseUrl, effectiveBaseUrl and
createProviderHandler are updated accordingly.
🪄 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: dc95ce7a-6ed6-40c3-8cad-df6606d2a5ee
📒 Files selected for processing (5)
frontend/app/onboarding/_components/animated-provider-steps.tsxfrontend/app/onboarding/_components/onboarding-step.tsxfrontend/components/cloud-picker/unified-cloud-picker.tsxfrontend/components/task-dialog/file-list.tsxfrontend/components/task-dialog/use-task-dialog.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
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.
Eliminate stale UI flashes from state-on-prop-change effects
Fixes 10 instances of no-adjust-state-on-prop-change across 5 files. These useEffect calls synced state to prop changes, causing React to render once with stale values before the effect corrected them — users
briefly saw wrong data.
Changes:
file-list.tsx— Tab reset on retry tab disappearing moved to render-time checkuse-task-dialog.ts— Selection clear on dialog close and status category fallback moved to prev-prop patternanimated-provider-steps.tsx— Removed redundant startTime state (was just a copy of processingStartTime prop); elapsed time now captured during render on completion transitiononboarding-step.tsx— Split monolithic effect into render-time prop adjustments + a slimmer effect for the typewriter animation interval onlyunified-cloud-picker.tsx— Replaced fake-async effect with render-time check (the "async" function was synchronous); removed dead isLoadingBaseUrl state and unreachable loading UISummary by CodeRabbit