feat: Add confirmation dialog to handle conflicts when migrating settings to server #3092
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughAdds conflict-detection and optional overwrite handling to migration flows for views, filters, and scripts; DashboardSettings now aggregates conflicts, displays a modal for user resolution, and can retry migrations with overwrite consent. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DashboardSettings as DashboardSettings(UI)
participant ViewMgr as ViewPreferencesManager
participant FilterMgr as FilterPreferencesManager
participant ScriptMgr as ScriptManager
participant Server
User->>DashboardSettings: Click "Migrate to Server"
activate DashboardSettings
par Parallel: Check Views/Filters/Scripts
DashboardSettings->>ViewMgr: migrateToServer(appId, false)
activate ViewMgr
ViewMgr->>Server: Load existing views
Server-->>ViewMgr: Server views
ViewMgr->>ViewMgr: Detect conflicts
alt Conflicts
ViewMgr-->>DashboardSettings: {success:false, conflicts}
else No Conflicts
ViewMgr->>ViewMgr: _migrateViewsToServer()
ViewMgr->>Server: Save views
Server-->>ViewMgr: OK
ViewMgr-->>DashboardSettings: {success:true, viewCount}
end
deactivate ViewMgr
DashboardSettings->>FilterMgr: migrateToServer(appId, false)
activate FilterMgr
FilterMgr->>Server: Load existing filters
Server-->>FilterMgr: Server filters
FilterMgr->>FilterMgr: Detect conflicts
alt Conflicts
FilterMgr-->>DashboardSettings: {success:false, conflicts}
else No Conflicts
FilterMgr->>FilterMgr: _migrateFiltersToServer()
FilterMgr->>Server: Save filters
Server-->>FilterMgr: OK
FilterMgr-->>DashboardSettings: {success:true, filterCount}
end
deactivate FilterMgr
DashboardSettings->>ScriptMgr: migrateToServer(appId, false)
activate ScriptMgr
ScriptMgr->>Server: Load existing scripts
Server-->>ScriptMgr: Server scripts
ScriptMgr->>ScriptMgr: Detect conflicts
alt Conflicts
ScriptMgr-->>DashboardSettings: {success:false, conflicts}
else No Conflicts
ScriptMgr->>ScriptMgr: _migrateScriptsToServer()
ScriptMgr->>Server: Save scripts
Server-->>ScriptMgr: OK
ScriptMgr-->>DashboardSettings: {success:true, scriptCount}
end
deactivate ScriptMgr
end
alt Any Conflicts Returned
rect rgba(255,215,155,0.6)
DashboardSettings->>DashboardSettings: Aggregate conflicts
DashboardSettings->>User: renderConflictModal() (showConflictModal=true)
end
User->>DashboardSettings: Choose Overwrite or Cancel
alt Overwrite
DashboardSettings->>ViewMgr: migrateToServer(appId, true)
DashboardSettings->>FilterMgr: migrateToServer(appId, true)
DashboardSettings->>ScriptMgr: migrateToServer(appId, true)
else Cancel
DashboardSettings->>DashboardSettings: handleConflictCancel()
end
end
deactivate DashboardSettings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/lib/ViewPreferencesManager.js (2)
106-139: Duplicate server fetch can be optimized.
getConfigsByPrefixis called once here (line 108) for conflict detection, and then again inside_migrateViewsToServer(line 195). Consider passingexistingViewConfigsandexistingViewIdsto_migrateViewsToServerto avoid the redundant network call.🔎 Suggested optimization
- await this._migrateViewsToServer(appId, localViews, overwriteConflicts); + await this._migrateViewsToServer(appId, localViews, existingViewIds, overwriteConflicts);Then update
_migrateViewsToServerto acceptexistingViewIdsas a parameter instead of fetching again.
200-229: Consider extracting view config cleanup into a helper.The logic for cleaning a view config (removing ID, null/undefined values, stringifying query) is duplicated across
_migrateViewsToServer,_saveViewsToServer(lines 303-319), and_saveViewToServer(lines 341-354). Extracting this into a private_prepareViewConfig(view)method would reduce duplication and ensure consistency.src/dashboard/Settings/DashboardSettings/DashboardSettings.react.js (2)
617-671: Consider moving inline styles to SCSS.The conflict list uses extensive inline styles. For consistency with the rest of the component and easier maintenance, consider moving these to
DashboardSettings.scss.
169-173: ClearmigrationConflictsafter overwrite to avoid stale state.When the user confirms overwrite,
migrationConflictsremains populated with the old conflict data. While it won't cause visible issues (the modal is hidden), clearing it maintains cleaner state.🔎 Suggested fix
handleConflictOverwrite() { - this.setState({ showConflictModal: false }); + this.setState({ showConflictModal: false, migrationConflicts: [] }); // User chose to overwrite - retry migration with overwrite flag this.migrateToServer(true); }src/lib/FilterPreferencesManager.js (1)
228-259: Consider extracting filter config cleanup into a helper.Similar to ViewPreferencesManager, the logic for preparing a filter config (removing ID, adding className, removing null/undefined values, stringifying filter) is duplicated across
_migrateFiltersToServer,_saveFiltersToServer(lines 348-367), and_saveFilterToServer(lines 388-405). A shared_prepareFilterConfig(filter, className)helper would reduce duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/dashboard/Settings/DashboardSettings/DashboardSettings.react.jssrc/lib/FilterPreferencesManager.jssrc/lib/ViewPreferencesManager.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/FilterPreferencesManager.js (2)
src/lib/ClassPreferences.js (2)
getAllPreferences(54-75)className(62-62)src/lib/generatePath.js (2)
filters(8-8)filter(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (3)
src/dashboard/Settings/DashboardSettings/DashboardSettings.react.js (1)
119-136: LGTM - conflict aggregation logic is sound.The approach of collecting conflicts from both views and filters, then conditionally showing the dialog, correctly handles the migration flow. The early return after showing the conflict dialog properly prevents partial migration.
src/lib/FilterPreferencesManager.js (2)
221-267: LGTM - the helper correctly handles conditional saves.The
_migrateFiltersToServermethod properly checksoverwriteConflicts || !existingFilterIds.includes(filterId)before saving, ensuring non-conflicting filters are migrated while conflicts are skipped when not overwriting.
153-157: Filters in classes with conflicts are skipped entirely, preventing migration of non-conflicting filters.When
conflictingIds.length > 0and!overwriteConflicts, line 153's condition fails and the entire class's migration is skipped (lines 153–157). This includes non-conflicting filters from that class. The underlying_migrateFiltersToServermethod already handles per-filter conflict detection (line 250), so class-level gating creates an all-or-nothing behavior that differs from ViewPreferencesManager, which migrates items individually and only returns conflicts when requested.If non-conflicting filters should always be migrated while collecting conflicts for user decision, remove the class-level gate and rely on the per-filter check at line 250.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dashboard/Settings/DashboardSettings/DashboardSettings.react.js (1)
101-171: Set loading state when retrying migration after conflict resolution.The migration flow correctly aggregates conflicts from all three sources (views, filters, scripts) and displays the conflict dialog. However, when the user chooses to overwrite (line 184),
this.migrateToServer(true)is called without settingmigrationLoadingback totrue. This could result in a brief moment where the UI doesn't show a loading indicator during the retry.🔧 Proposed fix to ensure loading state during retry
handleConflictOverwrite() { - this.setState({ showConflictModal: false }); + this.setState({ showConflictModal: false, migrationLoading: true }); // User chose to overwrite - retry migration with overwrite flag this.migrateToServer(true); }Otherwise, the conflict detection, aggregation, and success messaging logic is sound and correctly handles all three resource types.
🧹 Nitpick comments (2)
src/lib/ScriptManager.js (2)
195-249: Approve the conflict detection logic with a minor optimization opportunity.The conflict detection and migration flow is well-structured. The logic correctly:
- Detects conflicts by comparing local and server script IDs
- Returns conflicts for user decision when
overwriteConflictsis false- Delegates to the helper method to perform the actual migration
However, there's a minor inefficiency: existing scripts are fetched twice—once here (lines 212-215) and again in
_migrateScriptsToServer(lines 308-311).💡 Optional optimization to avoid duplicate fetch
Consider passing the already-fetched
existingScriptConfigsto the helper method:// Proceed with migration (merge mode) - await this._migrateScriptsToServer(appId, localScripts, overwriteConflicts); + await this._migrateScriptsToServer(appId, localScripts, overwriteConflicts, existingScriptConfigs); return { success: true, scriptCount: localScripts.length };Then update the helper signature to accept and use the cached configs instead of re-fetching.
301-345: Clarify the comment about merge mode.The implementation correctly handles both merge and overwrite scenarios. However, the comment on line 242 says "Proceed with migration (merge mode)" which could be misleading since this method is called in both modes (merge when no conflicts, overwrite when conflicts exist and user approved).
📝 Suggested comment clarification
- // Proceed with migration (merge mode) + // Proceed with migration (merge or overwrite mode based on overwriteConflicts flag) await this._migrateScriptsToServer(appId, localScripts, overwriteConflicts);The merge/preserve behavior on lines 328-335 and the comment on line 339 correctly document that server scripts not in local storage are preserved during migration, which is the right behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Settings/DashboardSettings/DashboardSettings.react.jssrc/lib/ScriptManager.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-27T12:09:47.644Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Applied to files:
src/dashboard/Settings/DashboardSettings/DashboardSettings.react.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docker linux/amd64
- GitHub Check: Node 18
🔇 Additional comments (5)
src/dashboard/Settings/DashboardSettings/DashboardSettings.react.js (5)
18-18: LGTM! Clean state and import additions.The Modal import and new state properties (
showConflictModal,migrationConflicts) are properly added to support the conflict handling flow.Also applies to: 51-52
173-190: LGTM! Clear conflict handling flow.The three new methods provide a clean separation of concerns:
showConflictDialog: prepares and displays the modalhandleConflictOverwrite: retries with overwrite permissionhandleConflictCancel: cancels and notifies the userThe user feedback on cancellation (line 189) is helpful.
582-582: LGTM! Improved user-facing description.The updated description text clearly informs users about the conflict detection and resolution flow, which improves transparency and sets proper expectations.
621-725: LGTM! Well-structured conflict modal with clear presentation.The
renderConflictModalmethod effectively:
- Groups conflicts by type (views, filters, scripts)
- Displays each conflict with a meaningful name and ID
- Provides scrolling for long conflict lists
- Uses appropriate Modal type (DANGER) and clear action buttons
The conditional formatting on lines 644, 663 handles spacing between sections gracefully, and the filter display (lines 668-671) includes both the filter name and class name for context.
553-553: No change needed to the condition.The three managers (
viewPreferencesManager,filterPreferencesManager, andscriptManager) are always initialized together in the constructor (lines 72-74), so checking two of them is sufficient to guarantee the third is initialized. The current condition at line 553 is safe as written.Likely an incorrect or invalid review comment.
mtrezza
changed the title
# [8.2.0-alpha.13](8.2.0-alpha.12...8.2.0-alpha.13) (2025-12-23) ### Features * Add confirmation dialog to handle conflicts when migrating settings to server ([#3092](#3092)) ([ae50b8d](ae50b8d))
|
🎉 This change has been released in version 8.2.0-alpha.13 |
2 participants
New Pull Request Checklist
Issue Description
When migrating settings from local to server storage, any conflicting server-stored setting is overwritten and any non-conflicting server-stored setting is deleted.
Approach
Show conflict dialog to let user handle migration:
This also fixes an issue where JS console scripts are not migrated to the server.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.