feat: highlight diffs for properties #5282
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe pull request introduces significant updates to various HTML, JavaScript, and localization files within the Apollo portal. Key changes include enhancements to the display of differences in configuration and release history, with updates to AngularJS bindings and the introduction of new properties for handling text differences. The CSS file sees the addition of new classes for improved layout, while localization files are updated for clarity in translations. Overall, these modifications refine the user interface and improve the functionality of the diff display and release history components. Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)CHANGES.md (1)
The entry follows the established format and clearly describes the new feature for highlighting diffs for properties. It properly links to the PR and is placed in the correct location within the changelog. 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 All contributors have signed the CLA ✍️ ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (16)
apollo-portal/src/main/resources/static/views/component/show-text-modal.html (1)
26-32: Consider enhancing the diff mode configuration.The current implementation provides a good foundation for text diff functionality. To make it more robust and maintainable, consider:
- Adding configuration options for diff visualization (e.g., inline vs side-by-side).
- Implementing a loading state for large diffs.
- Adding error boundaries for graceful fallback.
apollo-portal/src/main/resources/static/scripts/directive/diff-directive.js (2)
63-63: Consider moving styles to CSS classesWhile the change to make diff lines block-level elements improves readability, applying styles directly in JavaScript reduces maintainability. Consider moving this to a CSS class.
-span.style.display = 'block'; +span.classList.add('diff-line');Then add to your CSS:
.diff-line { display: block; }
Line range hint
63-67: Consider handling long lines gracefullyThe block display might cause issues with very long lines. Consider adding word-wrap handling for better user experience.
-span.style.display = 'block'; +span.classList.add('diff-line');Then in CSS:
.diff-line { display: block; word-wrap: break-word; white-space: pre-wrap; }apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js (1)
77-80: Consider adding property validation and defaultsWhile the new properties enhance the directive's capabilities for text diffing, consider:
- Adding type validation for
oldStrandnewStr- Setting a default value for
enableTextDiffscope: { text: '=', oldStr: '=', newStr: '=', - enableTextDiff: '=' + enableTextDiff: '=?' }, +link: function (scope) { + // Initialize with default value + scope.enableTextDiff = angular.isDefined(scope.enableTextDiff) ? scope.enableTextDiff : false;apollo-portal/src/main/resources/static/scripts/directive/release-modal-directive.js (1)
174-176: Consider adding type validation and default case handling.While the current implementation correctly sets the comparison states, it could be more robust with:
- Type parameter validation
- Default case handling to prevent all flags being false
Consider this improvement:
function switchReleaseChangeViewType(type) { scope.releaseChangeViewType = type; + const validTypes = ['compareWithMasterValue', 'compareWithPublishedValue', 'release']; + if (!validTypes.includes(type)) { + type = 'compareWithPublishedValue'; // default to published comparison + } scope.isCompareMaster = type === 'compareWithMasterValue'; scope.isComparePublished = type === 'compareWithPublishedValue'; scope.isNoCompare = type === 'release'; }apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js (2)
175-175: Consider adding input validation for compositedKey components.The composite key construction is correct and consistent with existing usage. However, consider adding validation to ensure none of the components (env, clusterName, namespaceName) are undefined or contain the separator character ':'.
- cluster.compositedKey = cluster.env + ':' + cluster.clusterName + ':' + cluster.namespaceName; + cluster.compositedKey = [cluster.env, cluster.clusterName, cluster.namespaceName].every(Boolean) + ? [cluster.env, cluster.clusterName, cluster.namespaceName].join(':') + : (() => { throw new Error('Invalid cluster components for compositedKey'); })();
198-201: Consider adding input validation.The function implementation is clean and follows SRP. However, consider adding validation for the input parameters.
function showTextDiff(oldStr, newStr) { + if (typeof oldStr !== 'string' || typeof newStr !== 'string') { + throw new TypeError('showTextDiff expects string parameters'); + } $scope.oldStr = oldStr; $scope.newStr = newStr; AppUtil.showModal('#showTextModal'); }apollo-portal/src/main/resources/static/config/diff.html (2)
146-157: Enhance UX for clickable diff cellsWhile the functionality is correct, consider these improvements for better UX and maintainability:
- Add a tooltip to indicate the cell is clickable
- Simplify the nested expressions
- Add visual feedback for the diff comparison
<td ng-repeat="cluster in syncData.syncToNamespaces.slice(1)" class="cursor-pointer" - ng-click="showTextDiff((itemsKeyedByCluster[syncData.firstClusterKey] || {}).value, (itemsKeyedByCluster[cluster.compositedKey] || {}).value)"> + ng-click="showTextDiff((itemsKeyedByCluster[syncData.firstClusterKey] || {}).value, (itemsKeyedByCluster[cluster.compositedKey] || {}).value)" + title="{{'Config.Diff.ClickToCompare' | translate }}" + ng-class="{'diff-cell': (itemsKeyedByCluster[syncData.firstClusterKey] || {}).value !== (itemsKeyedByCluster[cluster.compositedKey] || {}).value}">Add this CSS class:
.diff-cell { background-color: rgba(255, 238, 240, 0.5); }
161-164: Consider supporting multi-cluster text diffThe current implementation only supports comparing two clusters in text mode. As the system grows, users might need to compare multiple clusters simultaneously.
Consider enhancing the apollodiff component to support multiple text sources, similar to how the table view handles multiple clusters. This would provide consistency across both diff modes.
apollo-portal/src/main/resources/static/views/component/release-modal.html (4)
Line range hint
70-114: Consider adjusting column widths for better usabilityThe current width distribution might not be optimal:
- Key column at 12% might be too narrow for long configuration keys
- New value column at 50% might be unnecessarily wide
Consider a more balanced distribution:
- <th width="12%"> + <th width="20%"> {{'Component.Publish.Key' | translate }} </th> <th> {{'Component.Publish.PublishedValue' | translate }} </th> - <th width="50%"> + <th width="40%"> {{'Component.Publish.NoPublishedValue' | translate }} </th>
Line range hint
121-168: Consider simplifying the column visibility logicThe table uses multiple conditions (isCompareMaster, isComparePublished, isNoCompare) for column visibility, which increases complexity and might make maintenance difficult.
Consider using a single enum/state variable to control the comparison mode instead of multiple boolean flags:
enum ComparisonMode { NO_COMPARE, COMPARE_MASTER, COMPARE_PUBLISHED }This would simplify the template logic to:
- <th ng-if="isCompareMaster || isNoCompare"> + <th ng-if="comparisonMode !== 'COMPARE_PUBLISHED'"> {{'Component.Publish.MasterValue' | translate }} </th> - <th ng-if="isComparePublished || isNoCompare"> + <th ng-if="comparisonMode !== 'COMPARE_MASTER'"> {{'Component.Publish.GrayPublishedValue' | translate }} </th>
Line range hint
175-208: Maintain consistent column widths across tablesFor better UI consistency, the column widths should match across all tables in the modal.
Apply the same width adjustments suggested for the normal release table:
- <th width="12%"> + <th width="20%"> {{'Component.Publish.Key' | translate }} </th> - <th width="50%"> + <th width="40%"> {{'Component.Publish.GrayValue' | translate }} </th>
269-271: Reconsider textarea height reductionThe textarea height has been reduced from 4 rows to 2, which might be too restrictive for detailed release notes.
Consider keeping the original height or making it auto-expandable:
- <textarea rows="2" name="comment" class="form-control" + <textarea rows="4" name="comment" class="form-control" style="resize: vertical;" ng-model="releaseComment" placeholder="Add an optional extended description..."></textarea>apollo-portal/src/main/resources/static/styles/common-style.css (2)
1182-1184: Consider adding a comment about column width management.While the
.table-fixedclass correctly implements fixed table layout, it's important to note that this requires explicit column width definitions for optimal results.Add a comment to help other developers:
.table-fixed{ + /* Note: When using this class, define explicit column widths for optimal layout */ table-layout: fixed; }
1186-1188: Consider using a more specific class name.The
.blockclass name is very generic and could lead to naming conflicts or confusion. Consider using a more descriptive name that indicates its intended use case.Examples of more specific names:
-.block { +.force-block-display { display: block; }or
-.block { +.block-container { display: block; }apollo-portal/src/main/resources/static/config/history.html (1)
160-160: LGTM! Consider standardizing column width declarations.The table layout improvements with
table-fixedclass and explicit column widths will help maintain consistent rendering. However, there's an inconsistency in width declarations:
- Line 163:
width="10%"matches the TD element (line 171:width="10%")- Line 164:
width="12%"doesn't match its TD element (line 178:width="20%")- <th width="12%">{{'Config.History.ChangeKey' | translate }}</th> + <th width="20%">{{'Config.History.ChangeKey' | translate }}</th>Also applies to: 163-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
apollo-portal/src/main/resources/static/config/diff.html(2 hunks)apollo-portal/src/main/resources/static/config/history.html(3 hunks)apollo-portal/src/main/resources/static/i18n/en.json(1 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(3 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js(3 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ReleaseHistoryController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/diff-directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/directive/release-modal-directive.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.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)apollo-portal/src/main/resources/static/views/component/show-text-modal.html(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apollo-portal/src/main/resources/static/i18n/en.json
- apollo-portal/src/main/resources/static/views/component/diff.html
🔇 Additional comments (18)
apollo-portal/src/main/resources/static/views/component/show-text-modal.html (2)
26-29: LGTM! Clean implementation of conditional rendering.
The wrapper div correctly preserves the original functionality while enabling the new text diff feature toggle.
30-32: Verify apollodiff component implementation and error handling.
Please ensure:
- The
apollodiffcomponent handles edge cases gracefully (empty strings, null values, large diffs). - The hardcoded
apollo-id='textDiff'won't cause conflicts if multiple diffs are shown simultaneously.
Let's verify the component implementation and its usage:
✅ Verification successful
Let's gather more information about the apollodiff implementation and its error handling.
The apollodiff implementation is robust and the ID usage is appropriate
The verification reveals that:
-
The component handles edge cases gracefully:
- Undefined values are converted to empty strings
- Carriage returns are properly sanitized
- The component uses JsDiff for reliable diff generation
- Empty or missing display areas are handled safely
-
The hardcoded
textDiffID is not problematic because:- Other instances of apollodiff use unique, context-specific IDs (e.g.,
diffWithPublished:${key},grayDiffWithMaster:${key}) - The
textDiffID is only used in the modal context which is not rendered simultaneously with other diffs
- Other instances of apollodiff use unique, context-specific IDs (e.g.,
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check apollodiff component implementation and its usage across the codebase
# Find the apollodiff component definition
echo "Searching for apollodiff component definition..."
fd -e js -e ts | xargs rg -l "apollodiff|apolloDiff"
# Check for other instances of hardcoded 'textDiff' id
echo "Checking for potential id conflicts..."
rg "textDiff"
# Look for error handling in the component
echo "Checking error handling..."
ast-grep --pattern 'catch ($_) {
$$$
}'
Length of output: 781
Script:
#!/bin/bash
# Check the implementation of the apollodiff directive
echo "Checking apollodiff directive implementation..."
cat apollo-portal/src/main/resources/static/scripts/directive/diff-directive.js
# Check DiffConfigController for context
echo "Checking DiffConfigController implementation..."
cat apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js
# Search for any other modals or components using apollodiff
echo "Checking other usages of apollodiff..."
rg -A 5 -B 5 "apollodiff"
Length of output: 27233
apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js (1)
77-80: Verify JSON processing for diff functionality
The new diff-related properties (oldStr, newStr) might need the same JSON processing as the text property. Consider:
- Adding watchers for
oldStrandnewStr - Applying the
parseBigIntprocessing to diff content when needed
apollo-portal/src/main/resources/static/scripts/directive/release-modal-directive.js (2)
37-37: LGTM! The new scope variable is well-placed and properly initialized.
The initialization of isComparePublished to true aligns with the default releaseChangeViewType value and follows the existing naming conventions.
174-176: Verify template integration for comparison states.
The new comparison states (isCompareMaster, isComparePublished, isNoCompare) should be properly handled in the template file.
✅ Verification successful
Template integration for comparison states is properly implemented
The template file correctly uses all three comparison states:
isCompareMastercontrols the display of master value comparisonisComparePublishedcontrols the display of published value comparisonisNoComparecontrols the display of release values
The states are used in appropriate ng-if directives and table column visibility logic, particularly in the gray release section of the template.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the comparison states in the template
rg -l "isCompareMaster|isComparePublished|isNoCompare" --type html
# Check the structure of comparison logic in the template
ast-grep --pattern 'ng-if="$_"' --lang html
Length of output: 189
Script:
#!/bin/bash
# Check the content of the template file to verify the integration
cat apollo-portal/src/main/resources/static/views/component/release-modal.html
# Also check for any related controller files that might handle these states
rg -l "ReleaseModalController|DiffConfigController" --type js
Length of output: 19425
apollo-portal/src/main/resources/static/scripts/controller/config/ReleaseHistoryController.js (2)
56-56: LGTM: Proper scope assignment
The showTextDiff function is correctly exposed to the view through the controller's scope.
211-214: LGTM: Proper state management
The addition of enableTextDiff = false ensures clear state management when displaying plain text.
Let's verify that this flag is properly handled in the view template:
✅ Verification successful
Proper flag usage confirmed in the view template
The enableTextDiff flag is correctly implemented in show-text-modal.html:
- When
false, it shows plain text content viang-show="!enableTextDiff" - When
true, it shows diff view using theapollodiffcomponent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of enableTextDiff in HTML templates
rg -t html 'enableTextDiff' apollo-portal/src/main/resources/static/
Length of output: 490
Script:
#!/bin/bash
# Let's check the content of show-text-modal.html to understand how enableTextDiff is used
cat apollo-portal/src/main/resources/static/views/component/show-text-modal.html
Length of output: 1766
apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js (2)
41-41: LGTM: Function binding is correct.
The renamed function is properly bound to the Angular scope.
167-168: Verify the usage of firstClusterKey.
The new property is properly initialized. Let's verify its usage across the codebase.
✅ Verification successful
Property initialization and usage verified
The firstClusterKey property is properly initialized and used within the DiffConfigController.js. It stores the composite key (env:clusterName:namespaceName) of the first selected cluster during the diff operation. The property is set after collecting the sync namespaces and is used as a reference point for the diff comparison.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of firstClusterKey to ensure it's properly utilized
rg "firstClusterKey" --type js
Length of output: 329
Script:
#!/bin/bash
# Get more context around the usage of firstClusterKey
rg "firstClusterKey" -B 5 -A 5 --type js
Length of output: 2927
Script:
#!/bin/bash
# Search for where this syncData object is used
rg "syncData" -B 2 -A 2 --type js apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js
Length of output: 1684
apollo-portal/src/main/resources/static/config/diff.html (2)
130-140: LGTM! Improved table header semantics
The change from td to th elements improves semantic correctness and accessibility. The header structure clearly shows environment and cluster context.
172-172: Verify text diff modal integration
The modal now supports text diffing functionality. Let's verify the integration with the controller.
✅ Verification successful
Text diff modal integration is properly implemented
The integration is correctly implemented with all necessary components in place:
- The directive
show-text-modal-directive.jsproperly handles the text diff attributes:- Defines
enableTextDiff,oldStr, andnewStrin its scope - Implements proper text processing and JSON handling
- Defines
- The
DiffConfigController.jscorrectly implements theshowTextDifffunction that passes the old and new text values to the modal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the showTextModal directive and controller integration
# Check if the controller properly initializes the text diff
rg -A 5 "showTextDiff.*function.*oldStr.*newStr" apollo-portal/src/main/resources/static/scripts/controller/config/
# Check if the modal directive handles the enable-text-diff flag
rg -A 5 "enable-text-diff.*scope" apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js
Length of output: 240
Script:
#!/bin/bash
# Let's try a broader search to find the modal directive and its implementation
# Search for the showTextModal directive definition
rg -l "directive.*showTextModal" apollo-portal/src/main/resources/static/scripts/directive/
# Find the modal template usage
rg -l "showtextmodal" apollo-portal/src/main/resources/static/
# Look for any text diff related functionality
rg -A 5 "text-diff|textDiff" apollo-portal/src/main/resources/static/scripts/
Length of output: 1616
Script:
#!/bin/bash
# Let's examine the show-text-modal-directive.js and DiffConfigController.js
# Check the show-text-modal-directive implementation
cat apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js
# Check the DiffConfigController implementation around the text diff
cat apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js
Length of output: 14797
apollo-portal/src/main/resources/static/views/component/release-modal.html (1)
Line range hint 42-59: Verify the button group visibility logic
The complex condition for button group visibility might have edge cases. Please verify that the buttons are correctly shown/hidden in all scenarios:
- Non-properties format
- Branch without merge and publish
- Properties format with branch
✅ Verification successful
Let me gather more context about the release modal directive to understand the visibility logic better.
Let me check how these properties are determined by looking at the namespace initialization and the modal directive's full implementation.
Based on the comprehensive code review, I can now provide a final response about the button group visibility logic:
Button group visibility logic is correctly implemented
The visibility condition !toReleaseNamespace.isPropertiesFormat || (toReleaseNamespace.isBranch && !toReleaseNamespace.mergeAndPublish) is properly handled because:
isPropertiesFormatis set based on namespace format (namespace.format == 'properties')isBranchandmergeAndPublishflags are properly managed in the release flow:- For normal releases: neither flag is set
- For gray releases:
isBranch = true,mergeAndPublish = false - For merge and publish: both flags are set
- The button group's visibility aligns with the three distinct release workflows (normal, gray, and merge) as evidenced in the
release()function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related controller logic handling these visibility conditions
rg -A 5 "toReleaseNamespace\.isPropertiesFormat|toReleaseNamespace\.isBranch|toReleaseNamespace\.mergeAndPublish" --type js
Length of output: 1082
Script:
#!/bin/bash
# Search for the initialization of toReleaseNamespace and its properties
ast-grep --pattern 'scope.toReleaseNamespace = $_'
# Also search for any function that sets these properties
rg "scope\.toReleaseNamespace\.(isPropertiesFormat|isBranch|mergeAndPublish)\s*=" --type js
# Look for the switchReleaseChangeViewType function implementation
ast-grep --pattern 'switchReleaseChangeViewType = function($_) {
$$$
}'
Length of output: 684
Script:
#!/bin/bash
# Get the full content of the release modal directive
cat apollo-portal/src/main/resources/static/scripts/directive/release-modal-directive.js
# Look for namespace property initialization in the controller
rg "namespace\.(isPropertiesFormat|isBranch|mergeAndPublish)" --type js -A 2
Length of output: 11989
apollo-portal/src/main/resources/static/styles/common-style.css (2)
1177-1180: LGTM!
The .left-overflow class is well-named and correctly implements horizontal scrolling with left-aligned text.
Line range hint 98-104: Verify the selector change impact.
The selector change from .pre to pre .pre increases specificity and only targets .pre elements within pre tags. This could affect existing elements using the .pre class that are not within pre tags.
✅ Verification successful
Based on the search results, I can now provide a final response:
The selector change is safe to proceed.
The search results show that the .pre class is used in a specific context within pre elements in the codebase:
- In
show-text-modal.html, it's used within<pre class="modal-body no-radius">elements - In
rollback-modal.html, it's used as part of a compound classpre-scrollable
The change from .pre to pre .pre is actually improving the specificity to match the actual usage in the HTML templates, where the class is applied to elements within pre tags. This change better reflects the existing DOM structure and won't break any existing styling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of .pre class to verify the impact
# Look for .pre class usage in HTML/JSX files
rg -t html -t jsx 'class="[^"]*\bpre\b[^"]*"' -A 2 -B 2
# Look for .pre class usage in JavaScript files
rg -t js 'className="[^"]*\bpre\b[^"]*"' -A 2 -B 2
Length of output: 138
Script:
#!/bin/bash
# Search for .pre class usage with broader patterns and file types
rg -l "class.*pre" --type html --type js --type ts --type tsx
# Search for specific HTML/JSX patterns of .pre usage
rg "<[^>]*class=\"[^\"]*pre[^\"]*\"[^>]*>" --type html --type js --type ts --type tsx -A 2 -B 2
# Search for React className patterns
rg "className=\"[^\"]*pre[^\"]*\"" --type js --type ts --type tsx -A 2 -B 2
# Look for style references to .pre
rg "\.pre\s*{" --type css --type scss --type less -A 5 -B 1
Length of output: 459
Script:
#!/bin/bash
# Search for .pre class usage with simpler patterns
rg -t html -t js "class.*pre"
# Look for style references to .pre in CSS files
rg "\.pre" -t css
# Search for files that might contain the pre class usage
fd -e html -e js -e jsx -e css | xargs grep -l "pre"
Length of output: 100379
apollo-portal/src/main/resources/static/config/history.html (2)
312-312: Verify the modal directive and consider adding documentation.
The modal component has been enhanced with new attributes to support text diff functionality. Let's verify the implementation and suggest documentation improvements.
Consider adding JSDoc comments to document the new attributes in the directive file:
/**
* @directive
* @name showtextmodal
* @description Shows a modal dialog with text comparison capabilities
* @param {string} text - The text to display
* @param {string} newStr - The new version of the text
* @param {string} oldStr - The old version of the text
* @param {boolean} enableTextDiff - Whether to enable text diff visualization
*/✅ Verification successful
Modal directive implementation is complete and functional
The directive implementation properly supports all the new attributes:
text: For displaying plain text or JSON contentoldStrandnewStr: For text comparison via the apollodiff componentenableTextDiff: Controls whether to show diff view or regular text view
The implementation includes:
- JSON parsing with BigInt support
- Text diff visualization using apollodiff component
- Proper template with conditional rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the showtextmodal directive implementation
# Check the directive implementation
rg -A 10 "showtextmodal.*directive" apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js
# Check if there's any documentation
fd README.md -x rg -l "showtextmodal"
Length of output: 162
Script:
#!/bin/bash
# Let's try different approaches to locate the directive implementation
# Search for the directive file
fd "show-text-modal-directive.js"
# Search for any file containing 'showtextmodal'
rg -i "showtextmodal" --type js
# Search for modal-related directives
rg "directive\(.*modal" --type js
# Search for files with 'modal' in their name
fd -e js modal
Length of output: 4458
Script:
#!/bin/bash
# Let's examine the show-text-modal-directive.js implementation
cat apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js
Length of output: 4613
Script:
#!/bin/bash
# Let's check the template file referenced in the directive
cat apollo-portal/src/main/resources/static/views/component/show-text-modal.html
Length of output: 1766
192-194: Verify the implementation of showTextDiff in the controller.
The change from showText to showTextDiff improves the UX by showing differences between old and new values. However, we should verify the controller implementation.
✅ Verification successful
Implementation of showTextDiff is properly implemented and functional
The showTextDiff function in ReleaseHistoryController.js is correctly implemented to handle text diff comparison:
- It takes two parameters (
oldStr,newStr) as required by the HTML change - It sets up the diff view by enabling text diff mode:
$scope.enableTextDiff = true - It stores both old and new values in the scope for comparison
- It displays the modal using
AppUtil.showModal('#showTextModal')
The implementation aligns with the UI change and follows a clean pattern for modal-based diff display.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the showTextDiff implementation in ReleaseHistoryController.js
# Check if the function is properly implemented
rg -A 10 "showTextDiff.*function.*\(" apollo-portal/src/main/resources/static/scripts/controller/config/ReleaseHistoryController.js
# Check for any related test cases
fd -e spec.js -e test.js -x rg -l "showTextDiff"
Length of output: 182
Script:
#!/bin/bash
# Let's try different approaches to locate the implementation
# 1. Search for the file first to ensure it exists
fd "ReleaseHistoryController.js$"
# 2. Try a broader search for showTextDiff in all JS files
rg "showTextDiff" --type js
# 3. Look for any modal or dialog related code that might handle text diff
rg "showText.*function|function.*showText" --type js
# 4. Search for diff-related functionality
rg "diff.*function|function.*diff" --type js
Length of output: 69370
Script:
#!/bin/bash
# Let's look at the specific implementation in ReleaseHistoryController.js
cat apollo-portal/src/main/resources/static/scripts/controller/config/ReleaseHistoryController.js
Length of output: 7858
apollo-portal/src/main/resources/static/i18n/zh-CN.json (2)
294-295: LGTM! Publish-related translations are accurate and consistent.
The translations for published value ("已发布的值"), changes ("改动"), unpublished value ("待发布的值"), and modification record ("修改记录") are clear and appropriate for the Chinese locale.
Also applies to: 297-297, 300-300
735-739: LGTM! History-related translations are clear and consistent.
The translations for change type ("类型"), key, value ("值"), old value ("旧值"), and new value ("新值") follow standard Chinese terminology for version control systems.
| function showTextDiff(oldStr, newStr) { | ||
| $scope.enableTextDiff = true; | ||
| $scope.oldStr = oldStr; | ||
| $scope.newStr = newStr; | ||
| AppUtil.showModal('#showTextModal'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling
The new text diff functionality needs additional safeguards for robustness:
- Input validation for
oldStrandnewStr - Error handling for modal operations
- Cleanup of scope variables when modal is closed
Consider applying these improvements:
function showTextDiff(oldStr, newStr) {
+ // Input validation
+ if (oldStr === undefined || newStr === undefined) {
+ AppUtil.showErrorMsg($translate.instant('Config.Text.InvalidInput'));
+ return;
+ }
+
$scope.enableTextDiff = true;
$scope.oldStr = oldStr;
$scope.newStr = newStr;
- AppUtil.showModal('#showTextModal');
+
+ try {
+ AppUtil.showModal('#showTextModal');
+
+ // Clean up when modal is closed
+ $('#showTextModal').on('hidden.bs.modal', function () {
+ $scope.$apply(function() {
+ $scope.enableTextDiff = false;
+ $scope.oldStr = null;
+ $scope.newStr = null;
+ });
+ });
+ } catch (error) {
+ AppUtil.showErrorMsg($translate.instant('Config.Text.ModalError'));
+ }
}📝 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.
| function showTextDiff(oldStr, newStr) { | |
| $scope.enableTextDiff = true; | |
| $scope.oldStr = oldStr; | |
| $scope.newStr = newStr; | |
| AppUtil.showModal('#showTextModal'); | |
| } | |
| function showTextDiff(oldStr, newStr) { | |
| // Input validation | |
| if (oldStr === undefined || newStr === undefined) { | |
| AppUtil.showErrorMsg($translate.instant('Config.Text.InvalidInput')); | |
| return; | |
| } | |
| $scope.enableTextDiff = true; | |
| $scope.oldStr = oldStr; | |
| $scope.newStr = newStr; | |
| try { | |
| AppUtil.showModal('#showTextModal'); | |
| // Clean up when modal is closed | |
| $('#showTextModal').on('hidden.bs.modal', function () { | |
| $scope.$apply(function() { | |
| $scope.enableTextDiff = false; | |
| $scope.oldStr = null; | |
| $scope.newStr = null; | |
| }); | |
| }); | |
| } catch (error) { | |
| AppUtil.showErrorMsg($translate.instant('Config.Text.ModalError')); | |
| } | |
| } |
| syncData.syncToNamespaces.push(cluster); | ||
| } | ||
| }); | ||
|
|
||
| syncData.firstClusterKey = selectedClusters[0].compositedKey; |
There was a problem hiding this comment.
Add null check for selectedClusters array.
The code 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 : ''; |
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/diff.html (1)
146-151: Consider extracting value access logic into a helper functionThe nested object access with fallbacks
(itemsKeyedByCluster[cluster.compositedKey] || {}).valueis repeated multiple times. Consider extracting this into a helper function for improved readability and maintainability.+// Add to controller +$scope.getItemValue = function(items, clusterKey) { + return (items[clusterKey] || {}).value; +}; -<td ng-bind="(itemsKeyedByCluster[syncData.firstClusterKey] || {}).value"></td> +<td ng-bind="getItemValue(itemsKeyedByCluster, syncData.firstClusterKey)"></td> -<span ng-if="itemsKeyedByCluster[cluster.compositedKey]" - ng-bind="(itemsKeyedByCluster[cluster.compositedKey] || {}).value"> +<span ng-if="itemsKeyedByCluster[cluster.compositedKey]" + ng-bind="getItemValue(itemsKeyedByCluster, cluster.compositedKey)">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apollo-portal/src/main/resources/static/config/diff.html(2 hunks)
🔇 Additional comments (3)
apollo-portal/src/main/resources/static/config/diff.html (3)
130-140: LGTM! Well-structured table header with proper semantic HTML
The table header implementation follows best practices with:
- Semantic use of
<th>elements - Clear column organization
- Proper i18n support
172-172: Verify showtextmodal directive implementation
The modal component has new attributes for text diffing. Let's ensure the directive properly implements these features.
Let's check the directive implementation:
✅ Verification successful
Directive implementation properly handles text diffing attributes
The directive implementation in show-text-modal-directive.js correctly defines all the required attributes in its scope:
enableTextDiff: '='oldStr: '='newStr: '='
Additionally, from the previous search results, we found that these attributes are properly utilized in the controllers (DiffConfigController.js and ReleaseHistoryController.js) and the diff functionality is implemented in diff-directive.js which watches and processes the old and new string values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the showtextmodal directive implementation
ast-grep --pattern 'directive("showtextmodal", function() {
$$$
scope: {
$$$
enableTextDiff: $$$,
oldStr: $$$,
newStr: $$$
$$$
},
$$$
}'
Length of output: 166
Script:
#!/bin/bash
# Let's try a broader search for the directive implementation
rg -l "directive.*showtextmodal" --type js
# Then search for scope definitions
rg "scope.*enableTextDiff|scope.*oldStr|scope.*newStr" --type js -A 5 -B 5
Length of output: 6453
Script:
#!/bin/bash
# Let's examine the show-text-modal-directive.js file since we found it
cat apollo-portal/src/main/resources/static/scripts/directive/show-text-modal-directive.js
Length of output: 4613
161-164: Add error handling for text content
The text diff component should handle cases where the text content might be undefined or invalid.
Let's verify the error handling in the apollodiff directive:
There was a problem hiding this comment.
Copilot reviewed 5 out of 13 changed files in this pull request and generated no suggestions.
Files not reviewed (8)
- apollo-portal/src/main/resources/static/config/diff.html: Language not supported
- apollo-portal/src/main/resources/static/config/history.html: Language not supported
- apollo-portal/src/main/resources/static/i18n/en.json: Language not supported
- apollo-portal/src/main/resources/static/i18n/zh-CN.json: Language not supported
- apollo-portal/src/main/resources/static/styles/common-style.css: Language not supported
- apollo-portal/src/main/resources/static/views/component/diff.html: Language not supported
- apollo-portal/src/main/resources/static/views/component/release-modal.html: Language not supported
- apollo-portal/src/main/resources/static/views/component/show-text-modal.html: Language not supported
|
I have read the CLA Document and I hereby sign the CLA |
What's the purpose of this PR
Show diffs for properties for the following pages:
Which issue(s) this PR fixes:
Fixes #
Brief changelog
showTextwithshowTextDiffshowTextwithshowTextDiffshowTextDiffFollow 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
Bug Fixes
Style
Documentation