feat: support diff functions for some properties #5279
Conversation
dosubot
bot
added
the
size:M
WalkthroughThe changes in this pull request involve modifications to several files related to the presentation and functionality of the Apollo configuration center. Key updates include enhancements to the HTML structure and AngularJS bindings in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js (2)
167-168: LGTM: Good addition of firstClusterKey for reference cluster tracking.The addition of
firstClusterKeyto the syncData structure provides a clear reference point for cluster comparisons, which is essential for the new diff functionality.Consider documenting the significance of
firstClusterKeyin the code comments, as it serves as a crucial reference point for the diff comparison logic.
175-175: Consider adding validation and extracting the key generation logic.While the compositedKey implementation is correct, consider these improvements:
- Validate that env, clusterName, and namespaceName don't contain ':' to prevent parsing issues
- Extract the key generation into a reusable utility function for consistency
+ // Add to a utility service/file + function generateCompositedKey(env, clusterName, namespaceName) { + // Validate components don't contain separator + if ([env, clusterName, namespaceName].some(component => component.includes(':'))) { + throw new Error('Cluster components cannot contain ":"'); + } + return `${env}:${clusterName}:${namespaceName}`; + } - cluster.compositedKey = cluster.env + ':' + cluster.clusterName + ':' + cluster.namespaceName; + cluster.compositedKey = generateCompositedKey(cluster.env, cluster.clusterName, cluster.namespaceName);apollo-portal/src/main/resources/static/config/diff.html (2)
130-134: LGTM! Consider adding ARIA attributes for better accessibility.The table structure changes look good with proper width constraints and consistent class usage.
Consider adding ARIA attributes to improve accessibility:
-<table class="table table-bordered table-striped table-hover table-fixed" ng-show="isTableDiff"> +<table class="table table-bordered table-striped table-hover table-fixed" ng-show="isTableDiff" role="grid" aria-label="Configuration differences">
162-166: Consider adding error handling for text mode.The text mode diff implementation looks good, but consider adding error handling for cases where originTextInfo might be undefined or malformed.
Add error handling:
-<apollodiff ng-show="!isTableDiff" old-str="syncData.syncToNamespaces[0].originTextInfo" +<apollodiff ng-show="!isTableDiff && syncData.syncToNamespaces.length >= 2" old-str="syncData.syncToNamespaces[0].originTextInfo" new-str="syncData.syncToNamespaces[1].originTextInfo" apollo-id="'releaseStrDiff'"> </apollodiff> +<div ng-show="!isTableDiff && syncData.syncToNamespaces.length < 2" class="alert alert-warning"> + {{'Config.Diff.InsufficientClusters' | translate }} +</div>apollo-portal/src/main/resources/static/views/component/release-modal.html (2)
71-72: Consider translating Chinese comments to English for consistencyWhile the added section comments improve code organization, consider translating them to English to maintain consistency with the rest of the codebase and improve accessibility for international contributors:
- "主版本发布" → "Main Version Release"
- "灰度发布" → "Gray Release"
- "灰度推全" → "Full Release from Gray"
Also applies to: 125-125, 177-178
Line range hint
1-300: Well-structured implementation of diff functionalityThe changes effectively implement diff visualization across all three release types (normal, gray, and full release), with consistent patterns and clear separation. This aligns perfectly with the PR objectives of supporting property format comparisons across different scenarios.
A few architectural observations:
- The modal maintains clear separation between different release types
- The diff implementation is consistent across all views
- The UI provides clear visual feedback for configuration changes
apollo-portal/src/main/resources/static/styles/common-style.css (2)
1186-1187: Consider adding width constraints for better table layout control.While the
table-fixedclass correctly sets the table layout to fixed, consider adding default or minimum column widths to prevent extremely narrow columns in the diff view..table-fixed{ table-layout: fixed; + min-width: 600px; /* Ensure minimum table width */ + width: 100%; /* Fill available space */ }
98-108: Architecture feedback: Well-structured CSS changes for diff functionality.The CSS changes form a cohesive set of styles that work together to support property diff views:
- Pre-formatting for preserving whitespace
- Overflow handling for long lines
- Fixed table layout for consistent diff presentation
This layered approach to styling provides good separation of concerns and maintainability.
Also applies to: 1181-1184, 1186-1187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apollo-portal/src/main/resources/static/config/diff.html(2 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js(1 hunks)apollo-portal/src/main/resources/static/styles/common-style.css(2 hunks)apollo-portal/src/main/resources/static/views/component/diff.html(1 hunks)apollo-portal/src/main/resources/static/views/component/release-modal.html(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- apollo-portal/src/main/resources/static/views/component/diff.html
🔇 Additional comments (7)
apollo-portal/src/main/resources/static/config/diff.html (2)
157-157: LGTM! Comment display implementation is consistent.
The comment display logic properly handles undefined values and follows the same pattern as value comparison.
146-153: Verify the compositedKey generation across clusters.
The value comparison implementation looks good with proper fallback handling and diff highlighting.
Let's verify the compositedKey implementation in the controller:
✅ Verification successful
compositedKey implementation is correctly used for cluster comparison
The verification confirms that compositedKey is properly constructed from cluster attributes:
- Composed as
env:clusterName:namespaceNamein DiffConfigController - Used consistently as a unique identifier for cluster comparison in the diff view
- Properly initialized before being used in the sync data structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify compositedKey generation in DiffConfigController
ast-grep --pattern 'compositedKey = $$$' apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js
Length of output: 132
Script:
#!/bin/bash
# Let's try a broader search for compositedKey usage and definition
rg -A 5 "compositedKey" apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js
# Also search for cluster key related code
ast-grep --pattern 'cluster.$_ = $$$'
Length of output: 2487
apollo-portal/src/main/resources/static/views/component/release-modal.html (2)
82-82: LGTM! Good improvements to the diff visualization
The changes enhance the UI by:
- Adding fixed width for better table layout control
- Implementing the apollodiff component for visual comparison between old and new values
Also applies to: 112-113
188-188: Verify apollodiff component implementation
The changes look good and properly implement diff visualization for gray releases. Let's verify the apollodiff component implementation across the codebase.
Also applies to: 213-214
✅ Verification successful
Let me verify the apollodiff component definition itself with another script.
Let me check the actual implementation and template of the apollodiff component.
apollodiff component implementation is correct and properly used
The apollodiff directive is well-implemented with:
- Proper scope isolation with two-way binding for
oldStr,newStr, andapolloId - Correct diff visualization using JsDiff library
- Consistent usage across the codebase with appropriate apollo-id prefixes:
- 'unpublishedDiffPublished:' for unpublished vs published
- 'grayDiffMaster:' for gray vs master version
- 'releaseStrDiffWithMasterValue' for release vs master
- 'releaseStrDiffWithPublishedValue' for release vs published
- 'clusterDiff:' for cluster comparisons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify apollodiff component implementation and usage
# Check apollodiff component definition
echo "Checking apollodiff component definition:"
fd -e js -e ts apollodiff
# Check apollodiff usage patterns
echo -e "\nChecking apollodiff usage patterns:"
rg -A 2 '<apollodiff\s+old-str=' --type html
Length of output: 2798
Script:
#!/bin/bash
# Search for apollodiff directive/component definition
ast-grep --pattern 'directive($_, "apollodiff")'
# Also check for any JS/TS files containing apollodiff implementation
rg -A 5 "apollodiff" --type js --type ts
Length of output: 1214
Script:
#!/bin/bash
# Check the directive implementation
echo "=== Checking diff directive implementation ==="
cat apollo-portal/src/main/resources/static/scripts/directive/diff-directive.js
# Check the diff component template
echo -e "\n=== Checking diff component template ==="
cat apollo-portal/src/main/resources/static/views/component/diff.html
Length of output: 3751
apollo-portal/src/main/resources/static/styles/common-style.css (2)
98-108: LGTM: Pre-formatting styles support property diff views.
The separation between pre and .pre classes allows for flexible handling of whitespace in different contexts, which is essential for properly displaying property file diffs.
1181-1184: LGTM: Overflow handling for long property lines.
The .left-overflow class appropriately handles horizontal scrolling while maintaining left alignment, which is crucial for displaying long property lines in diff views.
apollo-portal/src/main/resources/static/i18n/zh-CN.json (1)
294-294: LGTM! The translation changes improve clarity.
The changes to the translation strings better reflect the actual states:
- "已发布的值" (published value) for PublishedValue is more accurate
- "待发布的值" (value to be published) for NoPublishedValue is more logical
Also applies to: 297-297
| syncData.syncToNamespaces.push(cluster); | ||
| } | ||
| }); | ||
|
|
||
| syncData.firstClusterKey = selectedClusters[0].compositedKey; |
There was a problem hiding this comment.
Add validation for selectedClusters array.
The current implementation assumes selectedClusters[0] exists, which could lead to runtime errors if the array is empty.
- syncData.firstClusterKey = selectedClusters[0].compositedKey;
+ syncData.firstClusterKey = selectedClusters.length > 0 ? selectedClusters[0].compositedKey : "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| syncData.firstClusterKey = selectedClusters[0].compositedKey; | |
| syncData.firstClusterKey = selectedClusters.length > 0 ? selectedClusters[0].compositedKey : ""; |
|
Thanks for adding this great feature. Please find some comments below:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apollo-portal/src/main/resources/static/scripts/directive/diff-directive.js (1)
Line range hint
41-70: Consider enhancing diff visualization for properties formatWhile adding
display: blockimproves the layout, consider these enhancements for better property diff visualization:
- Add specific CSS classes instead of inline styles for better maintainability
- Consider adding indentation for nested properties
- Add special handling for property key-value pairs to highlight changes in keys and values separately
This would better align with the PR's objective of supporting properties format configurations.
Example implementation:
color = part.added ? 'green' : part.removed ? 'red' : 'grey'; span = document.createElement('span'); - span.style.color = color; - span.style.display = 'block'; + span.className = `diff-line diff-${color}`; pre = part.added ? '+' : part.removed ? '-' : ''; - span.appendChild(document.createTextNode(pre + part.value)); + // Special handling for property format + if (isPropertyFormat(part.value)) { + const [key, value] = parseProperty(part.value); + const keySpan = document.createElement('span'); + keySpan.className = 'diff-property-key'; + keySpan.textContent = pre + key; + + const valueSpan = document.createElement('span'); + valueSpan.className = 'diff-property-value'; + valueSpan.textContent = value; + + span.appendChild(keySpan); + span.appendChild(valueSpan); + } else { + span.appendChild(document.createTextNode(pre + part.value)); + }Add corresponding CSS:
.diff-line { display: block; padding: 2px 0; } .diff-green { color: green; } .diff-red { color: red; } .diff-grey { color: grey; } .diff-property-key { font-weight: bold; margin-right: 8px; } .diff-property-value { font-family: monospace; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apollo-portal/src/main/resources/static/i18n/zh-CN.json(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/diff-directive.js(1 hunks)apollo-portal/src/main/resources/static/views/component/release-modal.html(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-portal/src/main/resources/static/i18n/zh-CN.json
- apollo-portal/src/main/resources/static/views/component/release-modal.html
🔇 Additional comments (1)
apollo-portal/src/main/resources/static/scripts/directive/diff-directive.js (1)
63-63:
Add missing semicolon
A semicolon is missing at the end of the statement.
Apply this diff to fix the syntax:
- span.style.display = 'block'
+ span.style.display = 'block';
|
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apollo-portal/src/main/resources/static/config/history.html (1)
191-198: Excellent implementation of visual diff componentThe
apollodiffcomponent implementation effectively addresses the requirement for clearer property diffs. This change directly improves the user experience by providing visual difference comparisons.Consider these enhancements for robustness:
- Add error handling for cases where diff generation might fail
- Enhance accessibility by adding ARIA attributes for screen readers
Add error handling and accessibility improvements:
<td class="cursor-pointer left-overflow" width="35%" ng-click="showText(change.entity.secondEntity.value)"> <span> <apollodiff old-str="change.entity.firstEntity.value" new-str="change.entity.secondEntity.value" - apollo-id="'diff:' + change.entity.firstEntity.key"> + apollo-id="'diff:' + change.entity.firstEntity.key" + role="region" + aria-label="{{'Config.History.DiffViewer' | translate }}" + ng-attr-aria-errormessage="{{ diffError ? 'diff-error-' + $id : undefined }}"> </apollodiff> + <div ng-if="diffError" id="diff-error-{{$id}}" class="alert alert-danger"> + {{'Config.History.DiffError' | translate }} + </div> </span> </td>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apollo-portal/src/main/resources/static/config/history.html(2 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/resources/static/i18n/zh-CN.json
🔇 Additional comments (1)
apollo-portal/src/main/resources/static/config/history.html (1)
160-164: LGTM! Table layout improvements enhance readability
The addition of table-fixed class and explicit column widths provides better control over the table layout, ensuring consistent presentation of configuration changes.
|
@nobodyiam |
What's the purpose of this PR
The diff function doesn't work for properties currently. We'd like to compare the properties in different cluster or branch.
当前 apollo 页面上的 diff 功能不支持 properties 格式的配置，不便于对比配置。 我们希望支持 properties 格式的 diff 高亮功能
Which issue(s) this PR fixes:
Fixes #
Brief changelog
pics:
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes