feat: Add confirmation dialog before saving a Cloud Config parameter that has been modified since editing it #2770
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 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.✅ security/snyk check is complete. No issues have been found. (View Details) |
Uffizzi Ephemeral Environment
|
|
Caution Review failedThe pull request is closed. 📝 Walkthrough""" WalkthroughThe changes introduce concurrency conflict detection when saving configuration parameters in the dashboard. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigComponent
participant Server
participant Modal
User->>ConfigComponent: Initiate saveParam(name, value)
ConfigComponent->>Server: Fetch latest config data
Server-->>ConfigComponent: Return latest config data
ConfigComponent->>ConfigComponent: Compare cached vs latest value
alt Values differ and not override
ConfigComponent->>Modal: Show confirmation dialog
Modal->>User: Prompt to confirm overwrite
User->>Modal: Click "Continue"
Modal->>ConfigComponent: Confirm overwrite
ConfigComponent->>ConfigComponent: Call saveParam with override
ConfigComponent->>Server: Save parameter (override)
else Values same or override
ConfigComponent->>Server: Save parameter
end
Server-->>ConfigComponent: Save response
Assessment against linked issues
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
src/dashboard/Data/Browser/Browser.scss (1)
275-276: Fix the spelling error in the class nameThere's a typo in the class name
.confimConfig- it's missing the letter "r" and should be.confirmConfig.-.confimConfig { +.confirmConfig { padding: 10px 20px; }src/dashboard/Data/Config/Config.react.js (3)
108-130: Fix the class name reference to match the CSSThe implementation of the confirmation modal looks good, but there's a typo in the class name. The CSS class is misspelled as
.confimConfig(missing an "r"), and the same typo is reflected here.- <div className={[browserStyles.confimConfig]}> + <div className={[browserStyles.confirmConfig]}>This change should be made after fixing the class name in the CSS file.
274-294: Great implementation of concurrency conflict detectionThe async saveParam implementation with concurrency checking is a valuable improvement that prevents accidental overwrites when multiple users edit the same parameter.
Consider the following improvements:
- Add error handling for the fetch operation
- Clean up
this.confirmDataafter it's used to prevent memory leaksasync saveParam({ name, value, type, masterKeyOnly, override }) { const cachedParams = this.cacheData.get('params'); const cachedValue = cachedParams.get(name); - await this.props.config.dispatch(ActionTypes.FETCH); + try { + await this.props.config.dispatch(ActionTypes.FETCH); + } catch (error) { + // Handle fetch error gracefully + console.error('Failed to fetch latest config data:', error); + // Continue with save operation as we can't verify concurrency + } const fetchedParams = this.props.config.data.get('params'); if (cachedValue !== fetchedParams.get(name) && !override) { this.setState({ confirmModalOpen: true, modalOpen: false, }); this.confirmData = { name, value, type, masterKeyOnly, }; return; }And in the onConfirm handler for the modal:
onConfirm={() => { this.setState({ confirmModalOpen: false }); this.saveParam({ ...this.confirmData, override: true, }); + // Clean up confirmData after use + this.confirmData = null; }}
117-123: Clean up confirmData after useTo prevent potential memory leaks, clean up the
confirmDataproperty after it's used.onConfirm={() => { this.setState({ confirmModalOpen: false }); this.saveParam({ ...this.confirmData, override: true, }); + // Clean up confirmData after use + this.confirmData = null; }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Data/Browser/Browser.scss(1 hunks)src/dashboard/Data/Config/Config.react.js(5 hunks)
🔇 Additional comments (4)
src/dashboard/Data/Browser/Browser.scss (1)
271-273: Good fix for the missing closing braceAdding the missing closing brace for the
.noScrollclass fixes a CSS syntax error that could have caused styling issues.src/dashboard/Data/Config/Config.react.js (3)
42-42: LGTM: Added state variable for modal controlGood addition of the
confirmModalOpenstate to control the visibility of the confirmation modal.
63-63: LGTM: Caching data for concurrency checksGood approach to cache the config data for later comparison when saving parameters.
281-293:❌ Incorrect review comment
Verify behavior when the parameter is being created
For a new parameter that doesn't exist yet, the
cachedValueandfetchedParams.get(name)might both be undefined, potentially bypassing the conflict detection. Consider adding a check to handle this case.
🏁 Script executed:
#!/bin/bash # Check if there's specific handling for new parameters in the codebase echo "Checking how new parameters are created and saved..." rg -A 5 -B 5 'createParameter|Adding.*parameter' --type js echo "Looking for other saveParam calls to understand parameter creation flow..." rg 'saveParam\(' --type js echo "Examining whether the equality check handles undefined values correctly..." rg 'cachedValue.*!==.*fetchedParams' --type jsLength of output: 3825
🏁 Script executed:
#!/bin/bash sed -n '200,360p' src/dashboard/Data/Config/Config.react.jsLength of output: 4998
🏁 Script executed:
#!/bin/bash rg -n 'cacheData' src/dashboard/Data/Config/Config.react.jsLength of output: 177
No change needed for new parameter creation
The conflict check (
cachedValue !== fetchedParams.get(name)) intentionally allows both values to beundefined, so creating a brand-new parameter proceeds without triggering the confirmation modal. Special-casing the “new parameter” scenario isn’t necessary.Likely an incorrect or invalid review comment.
Signed-off-by: Manuel <[email protected]>
mtrezza
changed the title
mtrezza
changed the title
Uffizzi Ephemeral Environment
|
There was a problem hiding this comment.
- Shows dialog when editing in same dashboard instance:
https://github.com/user-attachments/assets/c3a48989-4e78-4aa8-937b-1d00350499a4 - The most likely user behavior when seeing the dialog will be to cancel, reload the page, investigate the current value and perhaps redo the edit. It's rather unlikely that a user will just continue, without even knowing what will be overwritten. Therefore I think we should fetch the parameter from the server right before showing the edit dialog. The issue is more likely to occur the longer the dashboard page has been open, e.g. in a separate browser tab. Reloading the param right before editing will mitigate the issue in the first place.
- How does the UI handle long loading fetch - how does the user experience while waiting?
- How is failed fetch handled? I assume a failed save shows an error message, does this also invoke that error message?
Signed-off-by: Manuel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/dashboard/Data/Config/Config.react.js (1)
278-278: Remove trailing whitespace.There are trailing spaces on lines 278, 316, and 321 that should be removed to comply with the linting rules.
- this.setState({ loading: true }); - + this.setState({ loading: true }); - this.setState({ modalOpen: false }); - + this.setState({ modalOpen: false }); - let transformedValue = value; - + let transformedValue = value;Also applies to: 316-316, 321-321
🧰 Tools
🪛 GitHub Check: Lint
[failure] 278-278:
Trailing spaces not allowed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Data/Config/Config.react.js(6 hunks)src/dashboard/Data/Config/ConfigDialog.react.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/dashboard/Data/Config/ConfigDialog.react.js (2)
src/components/Modal/Modal.react.js (1)
Modal(24-106)src/components/LoaderContainer/LoaderContainer.react.js (1)
LoaderContainer(16-29)
src/dashboard/Data/Config/Config.react.js (1)
src/components/Modal/Modal.react.js (1)
Modal(24-106)
🪛 GitHub Check: Lint
src/dashboard/Data/Config/Config.react.js
[failure] 321-321:
Trailing spaces not allowed
[failure] 316-316:
Trailing spaces not allowed
[failure] 278-278:
Trailing spaces not allowed
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (12)
src/dashboard/Data/Config/ConfigDialog.react.js (3)
24-24: Good addition of LoaderContainer component.Adding the LoaderContainer component allows for providing visual feedback to users during asynchronous operations, which improves the user experience.
226-299: Good code restructuring.Extracting the dialog content into a separate variable improves code readability and maintainability while preparing for the LoaderContainer implementation.
301-317: Well implemented loading state in the modal.The changes effectively integrate the loading state with the modal's disabled property and wrap the content with LoaderContainer. This provides users with visual feedback during the parameter saving process and concurrent modification checks.
src/dashboard/Data/Config/Config.react.js (9)
24-24: Appropriate Modal component import.The Modal component import supports the new confirmation dialog functionality for handling concurrency conflicts.
42-42: Good addition of confirmModalOpen state.This state variable properly manages the visibility of the confirmation dialog when concurrent modifications are detected.
63-63: Effective data caching implementation.Caching the configuration data enables the detection of concurrent modifications by comparing cached values with freshly fetched ones.
96-96: Good addition of loading prop.Passing the loading state to ConfigDialog enables proper visual feedback in the UI, complementing the LoaderContainer changes in ConfigDialog.react.js.
109-131: Well-implemented confirmation modal for concurrent modifications.The modal provides clear information to users about the concurrency conflict and offers appropriate options to proceed or cancel. This prevents accidental overwrites of changes made by other users.
275-298: Excellent concurrency conflict detection implementation.The updated saveParam method now properly checks for concurrent modifications by fetching the latest data and comparing it with the original value. This prevents unintentional overwrites and improves data integrity.
🧰 Tools
🪛 GitHub Check: Lint
[failure] 278-278:
Trailing spaces not allowed
300-314: Good implementation of data update after save.The code properly updates both the server and the local cache after a successful save operation, maintaining data consistency.
317-355: Well-implemented config history management.The code effectively manages the configuration history in localStorage, including proper handling of different value types (Date, File) and respecting the configurable history limit.
🧰 Tools
🪛 GitHub Check: Lint
[failure] 321-321:
Trailing spaces not allowed
356-362: Good error handling and loading state management.The try-catch-finally block properly handles errors during the save operation and ensures the loading state is reset regardless of the outcome.
Uffizzi Ephemeral Environment
|
mtrezza
changed the title
# [7.2.0-alpha.5](7.2.0-alpha.4...7.2.0-alpha.5) (2025-05-24) ### Features * Add confirmation dialog before saving a Cloud Config parameter that has been modified since editing it ([#2770](#2770)) ([adb9b5c](adb9b5c))
|
🎉 This change has been released in version 7.2.0-alpha.5 |
# [7.2.0](7.1.0...7.2.0) (2025-06-01) ### Bug Fixes * Data browser not scrolling to top when changing filter while cell selected ([#2821](#2821)) ([c2527dc](c2527dc)) * Data browser table shows loading indicator when info panel is loading ([#2782](#2782)) ([da57e5e](da57e5e)) * Improperly aligned unfolding sub-items in context menu in data browser ([#2726](#2726)) ([3fed292](3fed292)) * Notifications fade out erratically when executing a script on large number of rows ([#2822](#2822)) ([3891381](3891381)) * Pagination does not reset to page 1 when clicking on class or filter ([#2798](#2798)) ([29d1447](29d1447)) * Saving new filter in data browser overwrites filters added in other dashboard instances ([#2769](#2769)) ([46bc154](46bc154)) * Selecting a saved filter in data browser may highlight a different filter ([#2783](#2783)) ([4c6e853](4c6e853)) ### Features * Add confirmation dialog before saving a Cloud Config parameter that has been modified since editing it ([#2770](#2770)) ([adb9b5c](adb9b5c)) * Add custom CSS styling for info panel items ([#2788](#2788)) ([f031e5d](f031e5d)) * Add relative date filter in data browser for date constraints relative to when the query is run ([#2736](#2736)) ([d9dfd69](d9dfd69)) * Add script execution on parallel batches with option `script.executionBatchSize` ([#2828](#2828)) ([cee8b8d](cee8b8d)) * Keyboard Enter key can be used to select item in data browser filter dialog field dropdown ([#2771](#2771)) ([dc14710](dc14710))
|
🎉 This change has been released in version 7.2.0 |
New Pull Request Checklist
Issue Description
Closes: #2567
Approach
TODOs before merging
Summary by CodeRabbit
New Features
Bug Fixes
.noScrollCSS class for correct styling.Style
.confirmConfigCSS class to improve modal layout.