feat: Add confirmation dialog before saving a Cloud Config parameter that has been modified since loading 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
|
📝 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
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
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 <5673677+mtrezza@users.noreply.github.com>
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.