feat: Add diff view to Cloud Config parameter dialog for better conflict handling#3239
Merged
mtrezza merged 12 commits intoparse-community:alphafrom Feb 27, 2026
Merged
Conversation
|
🚀 Thanks for opening this pull request! |
Contributor
✅ 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 an inline conflict handling flow and a unified diff viewer for config edits: introduces ConfigConflictDiff component and styles, adds the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ConfigDialog as "ConfigDialog"
participant Config as "Config (container)"
participant Server as "Server"
participant Diff as "ConfigConflictDiff"
User->>ConfigDialog: Edit param & Save
ConfigDialog->>Config: onConfirm(name,type,value)
Config->>Server: saveParam(name,value)
Server-->>Config: responds with differing value (conflict)
Config->>Config: set modalConflict = true
Config->>ConfigDialog: update props (conflict=true, serverValue)
ConfigDialog->>Diff: render(serverValue, userValue)
User->>ConfigDialog: review diff, toggle Confirm Override
User->>ConfigDialog: Save (with confirmOverride)
ConfigDialog->>Config: onConfirm(..., override=true)
Config->>Server: saveParam(name,value,override=true)
Server-->>Config: save success
Config->>Config: clear modalConflict & modalOpen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
src/lib/tests/ConfigConflictDiff.test.js (1)
31-97: Strengthen assertions to validate diff semantics, not just render presence.Most tests only check truthiness, so row/prefix regressions can slip through. Add a couple of assertions for added/removed row markers or class names.
✅ Example assertion upgrade
it('renders a diff for changed string values', () => { @@ const tree = component.toJSON(); expect(tree).toBeTruthy(); - // Should have table child - expect(tree.children.length).toBe(1); + const serialized = JSON.stringify(tree); + expect(serialized).toContain('lineRemoved'); + expect(serialized).toContain('lineAdded'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tests/ConfigConflictDiff.test.js` around lines 31 - 97, Update the tests in ConfigConflictDiff.test.js to assert diff semantics instead of only truthiness: for the ConfigConflictDiff cases (e.g. the "changed string values" test that passes paramName="greeting", serverValue="hello", userValue="world"), inspect component.toJSON() (tree) to assert the rendered table/row includes the paramName and both server and user values and that the changed cells are marked as added/removed (either by checking the presence of expected class names like "diff-added" / "diff-removed" or visible +/- prefixes in the cell text); apply the same pattern to the object, boolean and number tests (use their paramName/serverValue/userValue) so each test verifies which side is treated as removed vs added rather than only checking tree truthiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dashboard/Data/Config/Config.react.js`:
- Around line 519-523: The current override re-check uses a reference comparison
(serverValueChanged = this.state.modalValue !== conflictServerValue) which can
falsely detect changes for object-like values (e.g., GeoPoint/File); replace
that strict inequality with a deep-equality check (e.g., use an existing
deepEquals utility or import one) when computing serverValueChanged in the
override branch so modalValue and conflictServerValue are compared by value;
update the logic around serverValueChanged (the override branch where
this.setState is called) to use deepEquals(this.state.modalValue,
conflictServerValue) instead of !==.
In `@src/dashboard/Data/Config/ConfigConflictDiff.react.js`:
- Around line 199-201: The single-line if returns violate the `curly` rule;
update the conditional block that checks `row.type` so each `if` uses braces and
explicit return statements (i.e., wrap the bodies for the checks of `row.type
=== 'removed'` and `row.type === 'added'` in { ... } and keep the final `return
styles.lineContext;`), referencing the `row.type` checks and the style values
`styles.lineRemoved`, `styles.lineAdded`, and `styles.lineContext`.
In `@src/dashboard/Data/Config/ConfigConflictDiff.scss`:
- Line 14: The font-family declaration containing 'SFMono-Regular' is quoted and
triggers stylelint's font-family-name-quotes rule; update the font-family value
used in the ConfigConflictDiff.scss font-family declaration (the line with
font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace;)
by removing the quotes around SFMono-Regular (and ensure other font names follow
project quoting rules) so the rule becomes unquoted for SFMono-Regular.
---
Nitpick comments:
In `@src/lib/tests/ConfigConflictDiff.test.js`:
- Around line 31-97: Update the tests in ConfigConflictDiff.test.js to assert
diff semantics instead of only truthiness: for the ConfigConflictDiff cases
(e.g. the "changed string values" test that passes paramName="greeting",
serverValue="hello", userValue="world"), inspect component.toJSON() (tree) to
assert the rendered table/row includes the paramName and both server and user
values and that the changed cells are marked as added/removed (either by
checking the presence of expected class names like "diff-added" / "diff-removed"
or visible +/- prefixes in the cell text); apply the same pattern to the object,
boolean and number tests (use their paramName/serverValue/userValue) so each
test verifies which side is treated as removed vs added rather than only
checking tree truthiness.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/components/JsonEditor/JsonEditor.scsssrc/dashboard/Data/Config/Config.react.jssrc/dashboard/Data/Config/ConfigConflictDiff.react.jssrc/dashboard/Data/Config/ConfigConflictDiff.scsssrc/dashboard/Data/Config/ConfigDialog.react.jssrc/lib/tests/ConfigConflictDiff.test.jstesting/preprocessor.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/dashboard/Data/Config/ConfigConflictDiff.scss (1)
21-21:⚠️ Potential issue | 🟡 MinorUnquote
SFMono-Regularto satisfy stylelint.Line 21 still uses quotes around
SFMono-Regular, which violatesfont-family-name-quotesand can fail lint.🛠️ Suggested fix
- font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace; + font-family: SFMono-Regular, Consolas, 'Liberation Mono', Menlo, monospace;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dashboard/Data/Config/ConfigConflictDiff.scss` at line 21, Remove the unnecessary quotes around SFMono-Regular in the font-family declaration in ConfigConflictDiff.scss: locate the font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace; line and change it so SFMono-Regular is unquoted (leave other family names as-is) to satisfy the font-family-name-quotes stylelint rule.
🧹 Nitpick comments (2)
src/lib/tests/ConfigConflictDiff.test.js (2)
31-43: Strengthen assertions for changed-string rendering.This case currently passes on very weak checks (
truthy+ child count). Please assert diff-specific output (e.g.,-/+rows or rendered changed content) so regressions in row construction are caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tests/ConfigConflictDiff.test.js` around lines 31 - 43, Update the test for ConfigConflictDiff to make assertions that verify the actual diff output rather than only truthiness and child count: render <ConfigConflictDiff serverValue="hello" userValue="world" type="String" /> and assert that the output includes the old/server value and new/user value (e.g., contains "hello" and "world") and that diff indicators or row markers are present (for example a "-" row for serverValue and a "+" row for userValue) so the test fails if the component stops rendering the expected diff rows; reference the ConfigConflictDiff component and the serverValue/userValue props to locate where to update the assertions.
45-55: Object/Boolean/Number cases need behavioral assertions, not just existence checks.These tests only verify that something rendered. Add assertions on expected rendered values/prefixes/row count per type to validate serialization + diff behavior, and consider a table-driven test to remove duplication.
Also applies to: 70-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tests/ConfigConflictDiff.test.js` around lines 45 - 55, The tests in ConfigConflictDiff.test.js currently only assert rendering succeeded; update the specs for the ConfigConflictDiff component (cases where type is "Object", "Boolean", "Number") to assert specific rendered behavior: verify the serialized userValue/serverValue strings appear (e.g., JSON string for objects), check diff prefixes or markers (old vs new) and exact row counts for the diff output, and replace duplicated cases (lines ~45-55 and 70-92) with a table-driven (data-driven) test that iterates over cases describing type, serverValue, userValue, expectedRenderedStrings, expectedPrefix(es), and expectedRowCount, using renderer.create(...).toJSON() and jest matchers (toContain, toMatch, length checks) to validate output for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/dashboard/Data/Config/ConfigConflictDiff.scss`:
- Line 21: Remove the unnecessary quotes around SFMono-Regular in the
font-family declaration in ConfigConflictDiff.scss: locate the font-family:
'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace; line and change
it so SFMono-Regular is unquoted (leave other family names as-is) to satisfy the
font-family-name-quotes stylelint rule.
---
Nitpick comments:
In `@src/lib/tests/ConfigConflictDiff.test.js`:
- Around line 31-43: Update the test for ConfigConflictDiff to make assertions
that verify the actual diff output rather than only truthiness and child count:
render <ConfigConflictDiff serverValue="hello" userValue="world" type="String"
/> and assert that the output includes the old/server value and new/user value
(e.g., contains "hello" and "world") and that diff indicators or row markers are
present (for example a "-" row for serverValue and a "+" row for userValue) so
the test fails if the component stops rendering the expected diff rows;
reference the ConfigConflictDiff component and the serverValue/userValue props
to locate where to update the assertions.
- Around line 45-55: The tests in ConfigConflictDiff.test.js currently only
assert rendering succeeded; update the specs for the ConfigConflictDiff
component (cases where type is "Object", "Boolean", "Number") to assert specific
rendered behavior: verify the serialized userValue/serverValue strings appear
(e.g., JSON string for objects), check diff prefixes or markers (old vs new) and
exact row counts for the diff output, and replace duplicated cases (lines ~45-55
and 70-92) with a table-driven (data-driven) test that iterates over cases
describing type, serverValue, userValue, expectedRenderedStrings,
expectedPrefix(es), and expectedRowCount, using renderer.create(...).toJSON()
and jest matchers (toContain, toMatch, length checks) to validate output for
each case.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Data/Config/ConfigConflictDiff.scsssrc/lib/tests/ConfigConflictDiff.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/dashboard/Data/Config/ConfigDialog.react.js (2)
538-539: Minor redundancy:isExistingParamduplicates invertednewParamlogic.
newParamis already defined on line 393 as!this.props.param. You could reuse it instead of redefining the same condition.✨ Suggested simplification
const isJsonType = this.state.type === 'Object' || this.state.type === 'Array'; const isDiffableType = isJsonType || this.state.type === 'String'; - const isExistingParam = this.props.param && this.props.param.length > 0; + const isExistingParam = !newParam;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dashboard/Data/Config/ConfigDialog.react.js` around lines 538 - 539, The variable isExistingParam duplicates the inverse of newParam (newParam is set to !this.props.param in the component), so remove the redundant isExistingParam declaration and replace its usages with the negation of newParam (or directly use newParam where appropriate); update any references to isExistingParam in methods/JSX to use !newParam or this.props.param so the code uses the single source of truth (newParam) instead of defining isExistingParam.
476-480: Consider extracting the inline IIFE for better readability.The JSON parsing logic in the
serverValueprop is functional but could be clearer as a computed variable.✨ Suggested refactor
+ {(() => { + let serverValueForDiff = this.props.value; + if (this.state.type === 'Object' || this.state.type === 'Array') { + try { + serverValueForDiff = JSON.parse(this.props.value); + } catch { + // Keep raw value on parse error + } + } + return ( <ConfigConflictDiff - serverValue={ - (this.state.type === 'Object' || this.state.type === 'Array') - ? (() => { try { return JSON.parse(this.props.value); } catch { return this.props.value; } })() - : this.props.value - } + serverValue={serverValueForDiff} userValue={this.state.value} type={this.state.type} /> + ); + })()}Alternatively, compute
serverValueForDiffbefore the JSX and reference it directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dashboard/Data/Config/ConfigDialog.react.js` around lines 476 - 480, Extract the inline IIFE used in the serverValue prop into a computed variable above the JSX (e.g., compute serverValueForDiff or serverValue) so the JSX reads serverValue={serverValueForDiff}; implement logic: if this.state.type is 'Object' or 'Array' attempt JSON.parse(this.props.value) in a try/catch and fall back to this.props.value on parse errors, otherwise use this.props.value directly; update references in the JSX to use the new variable and remove the IIFE to improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dashboard/Data/Config/ConfigDialog.react.js`:
- Around line 538-539: The variable isExistingParam duplicates the inverse of
newParam (newParam is set to !this.props.param in the component), so remove the
redundant isExistingParam declaration and replace its usages with the negation
of newParam (or directly use newParam where appropriate); update any references
to isExistingParam in methods/JSX to use !newParam or this.props.param so the
code uses the single source of truth (newParam) instead of defining
isExistingParam.
- Around line 476-480: Extract the inline IIFE used in the serverValue prop into
a computed variable above the JSX (e.g., compute serverValueForDiff or
serverValue) so the JSX reads serverValue={serverValueForDiff}; implement logic:
if this.state.type is 'Object' or 'Array' attempt JSON.parse(this.props.value)
in a try/catch and fall back to this.props.value on parse errors, otherwise use
this.props.value directly; update references in the JSX to use the new variable
and remove the IIFE to improve readability.
parseplatformorg
pushed a commit
that referenced
this pull request
Feb 27, 2026
# [9.1.0-alpha.1](9.0.1-alpha.7...9.1.0-alpha.1) (2026-02-27) ### Features * Add diff view to Cloud Config parameter dialog for better conflict handling ([#3239](#3239)) ([f007a68](f007a68))
Contributor
|
🎉 This change has been released in version 9.1.0-alpha.1 |
2 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
2 participants
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.
Pull Request
Issue
Add diff view to Cloud Config parameter dialog for better conflict handling. The diff view can be enabled via a toggle at any time. It is especially useful when the server value has changed while editing a parameter value.
Tasks
Summary by CodeRabbit
New Features
Style