Repository navigation
improvement: Autosave settings on change - #544
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughInput-level autosave is added across Project and User settings; a shared ChangesInput-level Autosave Settings Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/app/pages/UserSettings/UserSettings.tsx (1)
576-617: 💤 Low valueConsider reverting local state on autosave failure.
When
profileAutosaveFetcherreturns an error (lines 605-613), the local state (e.g.,timezone,reportFrequency,form.timeFormat) remains set to the new value while the server retains the old value. The user sees an error toast but the UI remains inconsistent with server state.This is acceptable for a "chill" review since users can manually re-select the correct value, but for a more robust UX, you could track the previous value and revert on error.
🤖 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 `@web/app/pages/UserSettings/UserSettings.tsx` around lines 576 - 617, When profile autosave fails, the UI remains optimistically updated; capture and restore previous local values by saving the prior snapshot before applying optimistic changes and reverting it when profileAutosaveFetcher.data.error is truthy. Specifically, in the flow that triggers autosave (where activeProfileAutosave, pendingProfileAutosave, and flushProfileAutosave are used), store previous values for the mutated fields (e.g., timezone, reportFrequency, form.timeFormat) keyed to the autosave intent, then in the useEffect branch handling profileAutosaveFetcher.data.error restore those saved values (or call mergeUser with the server-side user if present) and clear activeProfileAutosave/pendingProfileAutosave as currently done; ensure lastHandledProfileAutosaveData is still updated to avoid double-handling.
🤖 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.
Nitpick comments:
In `@web/app/pages/UserSettings/UserSettings.tsx`:
- Around line 576-617: When profile autosave fails, the UI remains
optimistically updated; capture and restore previous local values by saving the
prior snapshot before applying optimistic changes and reverting it when
profileAutosaveFetcher.data.error is truthy. Specifically, in the flow that
triggers autosave (where activeProfileAutosave, pendingProfileAutosave, and
flushProfileAutosave are used), store previous values for the mutated fields
(e.g., timezone, reportFrequency, form.timeFormat) keyed to the autosave intent,
then in the useEffect branch handling profileAutosaveFetcher.data.error restore
those saved values (or call mergeUser with the server-side user if present) and
clear activeProfileAutosave/pendingProfileAutosave as currently done; ensure
lastHandledProfileAutosaveData is still updated to avoid double-handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9dc1dd0-cfcb-4304-b7b8-bbcfd10a683b
📒 Files selected for processing (9)
web/app/components/Header/index.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/tabs/AccessSettings.tsxweb/app/pages/Project/Settings/tabs/DangerZone.tsxweb/app/pages/Project/Settings/tabs/General.tsxweb/app/pages/Project/Settings/tabs/Revenue.tsxweb/app/pages/Project/Settings/tabs/Shields.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/public/locales/en.json
💤 Files with no reviewable changes (1)
- web/app/pages/Project/Settings/tabs/DangerZone.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/app/pages/Project/Settings/ProjectSettings.tsx (2)
1168-1176:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRollback failed autosaves for non-text settings.
handleFieldAutosavecommits the new value to local state immediately, but the autosave error path only reports the failure. When a request is rejected, controls likepublic,active, or CAPTCHA difficulty can stay in the new position even though the server kept the old value.Also applies to: 1208-1218
1558-1569:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t keep the new GSC property selected after a failed save.
This writes
gscPropertyUriinto local state beforegsc-set-propertysucceeds, and the error path never restores the previous value. A failed request will therefore leave the dropdown showing a property that is not actually connected.🤖 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 `@web/app/pages/Project/Settings/ProjectSettings.tsx` around lines 1558 - 1569, The onSelect handler currently writes gscPropertyUri into local state before the POST completes, so a failed gsc-set-property leaves the dropdown showing an unsaved value; change the flow so you do NOT call setForm immediately in the onSelect; instead call gscFetcher.submit({ intent: 'gsc-set-property', propertyUri: item.key }, { method: 'post' }) first, and update form.gscPropertyUri only after you observe a successful response (e.g. in an effect watching gscFetcher.data or gscFetcher.state for the gsc-set-property result); leave the previous gscPropertyUri intact on failure (no state change) so the dropdown reflects the actual persisted value.
🧹 Nitpick comments (1)
web/app/pages/Organisations/Settings/index.tsx (1)
68-68: 🏗️ Heavy liftExtract the fetcher-response dedupe into a shared hook.
This
useRef+ early-return pattern now exists across several settings screens. Centralizing it would make future autosave-side-effect fixes easier to apply consistently and reduce drift between pages.Also applies to: 92-94
🤖 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 `@web/app/pages/Organisations/Settings/index.tsx` at line 68, The repeated useRef-based dedupe (see lastHandledData: useRef<OrganisationSettingsActionData | null>) and early-return pattern used to ignore duplicate fetcher responses should be extracted into a reusable hook (e.g., useDeduplicateFetcherResponse) and used across settings pages; implement a hook that accepts the fetcher (or fetcher.data) and a comparator/key extractor, keeps the last handled value internally (via ref), returns a boolean or the new data only when it differs from the last value, then replace the inline lastHandledData checks in Organisation Settings (and the other screens referenced) with calls to this new hook to centralize the dedupe logic and avoid duplicated early-return logic.
🤖 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 `@web/app/pages/Project/Settings/tabs/Revenue.tsx`:
- Around line 148-156: When handling the 'update-revenue-currency' autosave
failure, restore the UI dropdown to the prior value instead of only nulling
pendingCurrency: on the error branch where you currently do
pendingCurrency.current = null and toast.error(error), also reset
selectedCurrency (or the state that drives the dropdown) back to the previous
value stored in pendingCurrency.current before it was cleared, so the displayed
currency matches saved status; apply the same fix to the analogous error
handling block at the other occurrence (lines ~354-357) to ensure both autosave
failures revert the UI to the previous currency.
---
Outside diff comments:
In `@web/app/pages/Project/Settings/ProjectSettings.tsx`:
- Around line 1558-1569: The onSelect handler currently writes gscPropertyUri
into local state before the POST completes, so a failed gsc-set-property leaves
the dropdown showing an unsaved value; change the flow so you do NOT call
setForm immediately in the onSelect; instead call gscFetcher.submit({ intent:
'gsc-set-property', propertyUri: item.key }, { method: 'post' }) first, and
update form.gscPropertyUri only after you observe a successful response (e.g. in
an effect watching gscFetcher.data or gscFetcher.state for the gsc-set-property
result); leave the previous gscPropertyUri intact on failure (no state change)
so the dropdown reflects the actual persisted value.
---
Nitpick comments:
In `@web/app/pages/Organisations/Settings/index.tsx`:
- Line 68: The repeated useRef-based dedupe (see lastHandledData:
useRef<OrganisationSettingsActionData | null>) and early-return pattern used to
ignore duplicate fetcher responses should be extracted into a reusable hook
(e.g., useDeduplicateFetcherResponse) and used across settings pages; implement
a hook that accepts the fetcher (or fetcher.data) and a comparator/key
extractor, keeps the last handled value internally (via ref), returns a boolean
or the new data only when it differs from the last value, then replace the
inline lastHandledData checks in Organisation Settings (and the other screens
referenced) with calls to this new hook to centralize the dedupe logic and avoid
duplicated early-return logic.
🪄 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: 6a7ca194-9886-4d26-8131-c7f72aebf34d
📒 Files selected for processing (11)
web/app/pages/Organisations/Settings/People.tsxweb/app/pages/Organisations/Settings/Projects.tsxweb/app/pages/Organisations/Settings/index.tsxweb/app/pages/Project/Settings/Emails/Emails.tsxweb/app/pages/Project/Settings/People/People.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/tabs/Revenue.tsxweb/app/pages/UserSettings/components/Organisations.tsxweb/app/pages/UserSettings/components/ProjectList.tsxweb/app/pages/UserSettings/components/Socialisations.tsxweb/app/pages/UserSettings/components/TwoFA.tsx
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 (2)
web/app/pages/Project/Settings/tabs/Revenue.tsx (1)
206-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent overlapping currency autosaves from corrupting rollback state.
pendingCurrencytracks only one{previous,next}pair. If the user changes currency again before the first request resolves, this ref gets overwritten and success/error handlers can commit or rollback to the wrong value.💡 Minimal fix
const handleCurrencyChange = ( currency: RevenueCurrency, previousCurrency: RevenueCurrency, ) => { + if (fetcher.state !== 'idle' || pendingCurrency.current) return pendingCurrency.current = { previous: previousCurrency, next: currency } fetcher.submit( { intent: 'update-revenue-currency', currency }, { method: 'POST', action: `/projects/settings/${projectId}` }, ) } ... <Select<CurrencyOption> ... onSelect={(item) => { + if (fetcher.state !== 'idle' || pendingCurrency.current) return if (item.code === selectedCurrency) return const previousCurrency = isRevenueCurrency(status?.currency) ? status.currency : selectedCurrency setSelectedCurrency(item.code) handleCurrencyChange(item.code, previousCurrency) }} />Also applies to: 370-378
🤖 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 `@web/app/pages/Project/Settings/tabs/Revenue.tsx` around lines 206 - 214, pendingCurrency currently holds a single {previous,next} and gets overwritten if the user changes currency before a prior request resolves, causing handlers to commit/rollback the wrong value; fix by associating each submission with a unique request id (e.g., const requestId = Symbol() or a counter) inside handleCurrencyChange, store that id alongside the {previous,next} in pendingCurrency.current before calling fetcher.submit, and then in the success and error handlers only apply commit/rollback when the handler's captured requestId matches pendingCurrency.current.requestId (otherwise ignore), so stale responses cannot corrupt the rollback state; update any other currency-change call sites (lines similar to 370-378) to use the same pattern.web/app/pages/UserSettings/UserSettings.tsx (1)
502-523:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t silently swallow server-side
fieldErrors.This branch handles
fieldErrors, but if there is no top-levelerrorit returns on Line 523 without showing any feedback. That makes backend validation failures look like a no-op to the user.At minimum, surface a fallback toast from the first field error before returning; ideally, plumb these errors into the relevant inline form state.
🤖 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 `@web/app/pages/UserSettings/UserSettings.tsx` around lines 502 - 523, The branch that handles fetcher.data?.fieldErrors currently returns early when there's no top-level fetcher.data.error, silently swallowing validation feedback; update the block that checks fetcher.data?.error || fetcher.data?.fieldErrors so that when fetcher.data.fieldErrors exists (even if fetcher.data.error is falsy) you surface user feedback — e.g., extract the first field error from fetcher.data.fieldErrors and show a fallback toast or map the field errors into the form's inline error state before returning; keep the existing rollback logic using pendingToggles.current, mergeUser, pendingPasswordChangeRef/passwordChangedRef and setIsPasswordChangeModalOpened, but do not return without first displaying the field error(s).
🤖 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 `@web/app/pages/Organisations/Settings/People.tsx`:
- Around line 286-291: The removal logic is racey because it falls back to
memberToRemove (cleared when the modal closes); change the fallback to a stable
source by capturing the id before the modal closes or reading it from the
fetcher submission instead of memberToRemove: use fetcher.data.memberId first,
then fallback to fetcher.submission?.formData.get('memberId') or a captured
variable (e.g., memberIdToRemove) that you set when opening the modal; update
the block around removedMemberId, setMembers, and the modal close flow so
memberToRemove is not relied on post-submit.
In `@web/app/pages/Organisations/Settings/Projects.tsx`:
- Around line 241-255: The success path currently reads the UI state variable
selectedProject when appending to setProjects, which can drift if the selection
changes while the request is in flight; capture the project you actually
submitted into a local const (e.g., submittedProject) before starting the async
request and then use submittedProject inside the success branch instead of
selectedProject (update the block that calls setProjects/current.some(...) and
the object literal to reference submittedProject.id, submittedProject.name, and
submittedProject.admin) so the list reflects the submitted item regardless of
later UI changes.
---
Outside diff comments:
In `@web/app/pages/Project/Settings/tabs/Revenue.tsx`:
- Around line 206-214: pendingCurrency currently holds a single {previous,next}
and gets overwritten if the user changes currency before a prior request
resolves, causing handlers to commit/rollback the wrong value; fix by
associating each submission with a unique request id (e.g., const requestId =
Symbol() or a counter) inside handleCurrencyChange, store that id alongside the
{previous,next} in pendingCurrency.current before calling fetcher.submit, and
then in the success and error handlers only apply commit/rollback when the
handler's captured requestId matches pendingCurrency.current.requestId
(otherwise ignore), so stale responses cannot corrupt the rollback state; update
any other currency-change call sites (lines similar to 370-378) to use the same
pattern.
In `@web/app/pages/UserSettings/UserSettings.tsx`:
- Around line 502-523: The branch that handles fetcher.data?.fieldErrors
currently returns early when there's no top-level fetcher.data.error, silently
swallowing validation feedback; update the block that checks fetcher.data?.error
|| fetcher.data?.fieldErrors so that when fetcher.data.fieldErrors exists (even
if fetcher.data.error is falsy) you surface user feedback — e.g., extract the
first field error from fetcher.data.fieldErrors and show a fallback toast or map
the field errors into the form's inline error state before returning; keep the
existing rollback logic using pendingToggles.current, mergeUser,
pendingPasswordChangeRef/passwordChangedRef and setIsPasswordChangeModalOpened,
but do not return without first displaying the field error(s).
🪄 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: 42ed434e-a3a0-47d3-82c5-5f4b9860fa78
📒 Files selected for processing (20)
web/app/components/NotificationChannels/EnableWebPushButton.tsxweb/app/components/NotificationChannels/NotificationChannels.tsxweb/app/hooks/useDeduplicateFetcherResponse.tsweb/app/pages/Organisations/Settings/People.tsxweb/app/pages/Organisations/Settings/Projects.tsxweb/app/pages/Organisations/Settings/index.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsSettings.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsxweb/app/pages/Project/Settings/Annotations.tsxweb/app/pages/Project/Settings/Emails/Emails.tsxweb/app/pages/Project/Settings/People/People.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/tabs/Revenue.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/pages/UserSettings/components/Organisations.tsxweb/app/pages/UserSettings/components/ProjectList.tsxweb/app/pages/UserSettings/components/Socialisations.tsxweb/app/pages/UserSettings/components/TwoFA.tsxweb/app/routes/organisations.$id.tsxweb/app/routes/projects.settings.$id.tsx
Merged
3 of 6 tasks
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.
Changes
todo:
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Chores