sbom dependency tree enhancements #1539
Conversation
WalkthroughThis update modifies multiple SBOM components. In component details, a new NotificationService is injected to handle error notifications while outdated getters are removed. The summary and status components now leverage new computed properties with semver-based comparisons. UI templates for empty-loading views, component trees, and skeleton loaders have been restructured. The SBOM component model provides cleaned version strings and the factory produces a reformatted component reference. Tests and translation files have also been updated, and a new semver dependency was added to the devDependencies. Changes
Suggested reviewers
Poem
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 warn config production Use ✨ Finishing Touches
🪧 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 (
|
Deploying irenestaging with
|
| Latest commit: |
5722ba2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a4861a37.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1702-sbom-dependency-tree.irenestaging.pages.dev |
Irene
|
||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1702-sbom-dependency-tree-enhancements
|
| Run status |
|
| Run duration | 05m 15s |
| Commit |
|
| Committer | Avi Shah |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
32
|
| View all changes introduced in this branch ↗︎ | |
avzz-19
force-pushed
the
PD-1702-sbom-dependency-tree-enhancements
branch
from
f723015 to
609e536
Compare
avzz-19
force-pushed
the
PD-1702-sbom-dependency-tree-enhancements
branch
from
609e536 to
47be6f4
Compare
avzz-19
force-pushed
the
PD-1702-sbom-dependency-tree-enhancements
branch
from
47be6f4 to
3f07a35
Compare
![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
🧹 Nitpick comments (2)
app/components/sbom/empty-loading-view/index.hbs (1)
4-6: Potential whitespace issue in class names.The multi-line format with whitespace at the end of lines could result in class names with extra spaces. While Handlebars typically handles this, it may cause issues if CSS selectors expect exact class name matches.
- local-class='empty-loading-container - {{if @skeleton "skeleton"}} - {{if this.tree "" "list"}}' + local-class='empty-loading-container + {{if @skeleton " skeleton"}} + {{if this.tree "" " list"}}'app/components/sbom/scan-details/skeleton-loader-list/index.ts (1)
27-29: Added mock data for skeleton loader.Creating an array of empty objects for loading states is a simple but effective approach. Consider adding a constant for the number of skeleton rows instead of hardcoding the value.
- get loadingMockData() { - return new Array(8).fill({}); - } + readonly SKELETON_ROWS_COUNT = 8; + + get loadingMockData() { + return new Array(this.SKELETON_ROWS_COUNT).fill({}); + }
📜 Review details
Configuration used: CodeRabbit 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 (21)
app/components/sbom/component-details/overview/index.ts(2 hunks)app/components/sbom/component-details/summary/index.ts(2 hunks)app/components/sbom/component-status/index.ts(3 hunks)app/components/sbom/empty-loading-view/index.hbs(2 hunks)app/components/sbom/empty-loading-view/index.scss(1 hunks)app/components/sbom/scan-details/component-tree/index.hbs(5 hunks)app/components/sbom/scan-details/component-tree/index.ts(9 hunks)app/components/sbom/scan-details/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader-list/index.hbs(1 hunks)app/components/sbom/scan-details/skeleton-loader-list/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.hbs(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.scss(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader/index.hbs(1 hunks)app/models/sbom-component.ts(1 hunks)mirage/factories/sbom-component.ts(1 hunks)package.json(1 hunks)tests/acceptance/sbom-test.js(3 hunks)tests/integration/components/sbom/scan-details/component-tree-test.js(1 hunks)translations/en.json(2 hunks)translations/ja.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/sbom/scan-details/skeleton-loader-tree/index.scss
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (53)
package.json (1)
185-185: Appropriate addition of semver library for version comparison operations.Adding the semver library is a good choice for implementing the new version comparison features mentioned in the summary (isLatestVersionLess and isOutdated computed properties).
app/components/sbom/scan-details/skeleton-loader/index.hbs (1)
27-27:❓ Verification inconclusive
Setting SVG height to 0px effectively hides the loading SVG.
Changing the loading SVG height from what was likely a larger value to 0px will hide the SVG while preserving the component structure. Consider whether a more semantic approach (like a boolean flag to show/hide the SVG) would be more maintainable in the long term.
🏁 Script executed:
#!/bin/bash # Check if there are other components that might use this pattern or if there's a hide property rg -A 2 -B 2 "@loadingSvgHeight" --type hbsLength of output: 2083
Attention: Review the approach for toggling the SVG visibility in the skeleton loader component.
- In
app/components/sbom/scan-details/skeleton-loader/index.hbs, the attribute@loadingSvgHeightis set to'0px', which effectively hides the loading SVG.- A comparison with other components (e.g.,
app/components/sbom/app-scan/list/index.hbsandapp/components/sbom/component-details/skeleton-loader/index.hbs) shows that a visible loading SVG normally has non-zero heights like'200px'or a fallback value such as'40%'.- To improve code clarity and maintainability, consider refactoring this approach by using a dedicated boolean flag to conditionally render or hide the SVG instead of relying on a height value of
0px.app/components/sbom/empty-loading-view/index.scss (1)
12-15: Added "list" variant for the empty-loading-container.The new
.listclass is properly aligned with the conditional class logic added in the template file.app/components/sbom/empty-loading-view/index.hbs (1)
26-26: Reduced the default height of the loading SVG.Changing the SVG height from 50% to 40% creates a more compact loading visualization. This change is consistent with the overall UI refinements being made to the loading components.
tests/integration/components/sbom/scan-details/component-tree-test.js (2)
168-173: Well-structured PURL generation from component dataThe code creates a standardized Package URL (PURL) representation for SBOM components, which follows the format
pkg:<type>/<namespace>/<name>@<version>. This provides better identification and standardization of components in the tree view.
175-175: Updated assertion to match new component display formatThe test now correctly verifies that the node label displays the PURL format instead of just the component name, aligning with the implementation changes in the component tree.
translations/ja.json (2)
608-608: Added helpful tooltip for highlighted nodeGood addition of tooltip text that helps users understand which component they're currently viewing in the tree.
1321-1321: Added error message for missing parent componentThis translation handles the edge case when a parent component cannot be found, improving the user experience by providing clear error messaging.
app/components/sbom/scan-details/skeleton-loader-list/index.hbs (1)
1-10: Improved skeleton loader with more semantic markupThe skeleton loader has been refactored from a stack-based layout to a table-based layout, which better represents the data structure it's placeholder for. This change also likely makes the loading state more consistent with the loaded state's appearance.
Note that this implementation relies on properties defined in the component class (
this.columnsandthis.loadingMockData), which should be properly implemented in the component's JavaScript/TypeScript file.translations/en.json (2)
608-608: Good addition of descriptive tooltip text.The new tooltip text clearly explains the highlighted node's significance to users, improving the user experience by providing contextual information.
1321-1321: Good error message for parent component lookup failures.This error message provides clear feedback when a parent component reference cannot be found, which will help with debugging and user understanding.
app/models/sbom-component.ts (2)
61-63: Good addition of version string cleanup.The
cleanVersiongetter appropriately handles string cleaning by trimming whitespace and removing surrounding double quotes, which will help with consistent version comparisons.
65-67:✅ Verification successful
Good implementation of consistent version string cleaning.
The
cleanLatestVersiongetter follows the same pattern ascleanVersion, ensuring consistency in how version strings are processed before comparison. This approach improves the reliability of version comparison operations.
🏁 Script executed:
#!/bin/bash # Search for where these new methods are used echo "Checking where the new cleanVersion methods are being used..." rg -A 2 "cleanVersion|cleanLatestVersion" --type tsLength of output: 1959
Consistent Version String Cleaning Confirmed
The implementation of the
cleanLatestVersiongetter inapp/models/sbom-component.tsis in line with the established pattern used incleanVersion. Our verification shows that both methods are consistently utilized across multiple components (e.g., in the component summary and status checks), ensuring reliable version string processing for comparisons. Great work maintaining uniformity across the codebase.app/components/sbom/scan-details/skeleton-loader-tree/index.ts (1)
9-22: Enhanced skeleton loader with improved tree structure representation.The additions to the
SbomScanDetailsSkeletonLoaderTreeComponentclass improve the skeleton loader by better mirroring the structure of the actual component it's substituting for during loading.Notable improvements:
- The
noop()method serves as a placeholder for event handlers- The
expandedproperty initializes with a default expanded node- The
skeletonTreeDataproperty dynamically generates appropriate placeholder data based on whether a filtered tree is being displayedThese changes will create a more consistent user experience during component loading.
app/components/sbom/component-details/overview/index.ts (1)
123-124: Good addition of user feedbackThe error notification provides better feedback to users when parent component verification fails, improving the overall user experience.
app/components/sbom/component-status/index.ts (3)
5-5: Good choice using semver for version comparisonThe semver library is the standard solution for comparing semantic versions, making this implementation more reliable than string comparison.
21-39: Robust implementation of isOutdated computed propertyThe implementation correctly handles edge cases with appropriate null checks and invalid version handling before performing the semver comparison.
58-58: Proper usage of the new computed propertyReplacing the direct access to component.isOutdated with the new computed property ensures consistent version comparison logic.
mirage/factories/sbom-component.ts (2)
33-34: Good addition of 'nuget' to supported namespacesAdding 'nuget' improves the coverage of package ecosystems in the test factory.
35-38:✅ Verification successful
Format change in bom_ref generation
The component reference format has changed from a version-based approach to a group-based format, which aligns with updates in the component model.
🏁 Script executed:
#!/bin/bash # Verify that the new reference format is consistently used across the codebase rg -A 3 "bom_ref" --type ts --glob "!mirage/factories/sbom-component.ts"Length of output: 393
Confirmed: New Component Reference Format in
mirage/factories/sbom-component.tsThe updated group-based format (
${namespace}::${group}:${name}) is correctly implemented. Our verification showed that the only otherbom_refusage (inmirage/factories/sbom-vulnerability-audit.ts) remains contextually appropriate. No inconsistencies were found.app/components/sbom/scan-details/skeleton-loader-tree/index.hbs (3)
4-5: Good improvement to container widthSetting the stack to full width will ensure proper alignment with the parent container.
8-15: Enhanced skeleton loader structureThe new nested stack with appropriate spacing and alignment provides a cleaner skeleton structure matching the actual content layout.
17-28:✅ Verification successful
Better tree representation with AkTree component
Using the AkTree component for the skeleton loader provides a more accurate representation of the actual component tree structure, improving the loading state UX.
🏁 Script executed:
#!/bin/bash # Verify that the skeletonTreeData and noop methods are defined in the component rg -A 10 "skeletonTreeData|noop" --type ts --glob "app/components/sbom/scan-details/skeleton-loader-tree/index.ts"Length of output: 1360
Verified and Approved: Improved Skeleton Loader Implementation
The usage of the AkTree component in the skeleton loader now accurately reflects the final tree structure during loading. The properties used—skeletonTreeData (which generates placeholder nodes) and noop (serving as a no-op event handler)—have been confirmed in the component’s TypeScript file, ensuring the intended behavior.
- File:
app/components/sbom/scan-details/skeleton-loader-tree/index.hbs(Lines 17–28)- Key Points:
- The skeleton tree data is generated correctly.
- The no-op handler is properly defined.
- The revised structure provides a more faithful representation of the component’s state during loading.
app/components/sbom/component-details/summary/index.ts (4)
6-6: Added semver dependency for version comparison.The addition of the semver library is appropriate for implementing proper semantic version comparison.
19-32: Well-implemented version comparison logic.The new
isLatestVersionLesscomputed property correctly compares versions using semver with appropriate null and validity checks. This approach safely handles edge cases where the latest version might be lower than the current version.
48-48: Improved version handling in component summary.The component now uses cleaned version strings and conditional logic to handle the display of version information, which improves data quality in the UI.
Also applies to: 52-54
12-12:✅ Verification successful
Type change for sbomComponent argument.
Changing the argument type from
SbomComponentModel | nullto non-nullableSbomComponentModelenforces that this component always receives a valid SBOM component. Ensure all parent components pass a non-null value.
🏁 Script executed:
#!/bin/bash # Search for places that pass sbomComponent to this component rg -A 3 "Sbom::ComponentDetails::Summary" | grep -B 2 -A 2 "sbomComponent"Length of output: 718
Non-null SBOM component enforced successfully
The verification confirms that all usages of the
Sbom::ComponentDetails::Summarycomponent pass a valid, non-nullsbomComponentvalue:
- In
app/components/sbom/component-details/overview/index.hbs, the component is invoked with@sbomComponent={{@sbomComponent}}.- The integration test in
tests/integration/components/sbom/component-details/summary-test.jslikewise provides a definedsbomComponent.These usages align with the enforced non-nullable type (
SbomComponentModel), ensuring that this component always receives a valid SBOM component. No further action is required.app/components/sbom/scan-details/skeleton-loader-list/index.ts (2)
1-8: Added internationalization support.Good addition of the IntlService to support localization of column headers in the skeleton loader.
9-25: Well-structured columns configuration.The columns getter provides a clear structure for the table headers with appropriate i18n support. The width specification for the component name column improves layout consistency.
app/components/sbom/scan-details/component-tree/index.hbs (5)
66-69: Simplified tree node class condition.The condition for the 'last-node' class has been simplified to only check
node.dataObject.hasNextSibling, removing the dependency onthis.hasMoreComponents. This makes the code clearer and more maintainable.
136-136: Simplified node label display.Removed the
bomReffrom the node label display, showing only the component name. This makes the component tree more readable.
145-157: Enhanced UX with tooltip for highlighted node.Replacing the simple AkStack with AkTooltip improves the user experience by providing contextual information for the highlighted component.
170-170: Updated event handler for loading more children.Now passing the entire parent node object instead of just the key. This provides more context to the handler function.
180-180: Simplified node scrollability check.The condition for checking if a node is scrollable has been simplified to only check
node.parent, aligning with updates to the TreeNodeDataObject interface elsewhere in the codebase.tests/acceptance/sbom-test.js (5)
41-59: Added NotificationsStub service for testing.Good implementation of a stub service for testing notifications. This allows tests to verify that the correct error messages are displayed to users.
157-157: Registered NotificationsStub service.Properly registered the NotificationsStub service in the test setup.
471-550: Comprehensive test for parent component loading.This test thoroughly verifies the component tree functionality with parent components, including error handling for invalid parent IDs. Good coverage of both success and error paths.
552-636: Test coverage for multi-level component hierarchy.Excellent test for verifying that the component tree correctly handles third-level dependencies and their expansion/collapse functionality.
638-711: Test for pagination in component children.Well-structured test for the "View More" functionality, ensuring that additional children can be loaded and displayed properly in the component tree.
app/components/sbom/scan-details/component-tree/index.ts (14)
109-109: Confirm removal ofhasMoreComponentscheck
By removing thehasMoreComponentscondition, this logic now applies margin ifdepthis 1 and there is no next sibling, regardless of any remaining components. Please ensure this is intentional, as it can affect layout when there are more components to load.
163-168: Stricter early return conditions
This change provides a robust safeguard by checking for a valid node, dependency status, and parent reference before proceeding. It helps prevent errors and ensures correct logic flow.
185-185: ChainingisLastLoadedChildwithhasMoreChildren
This conditional appears consistent and logically sound for determining when to show more child elements. No concerns here.
189-189: Refined child-loading flow
Replacing the direct child-appending approach withupdateNodeInTreeensures proper merging of new children into an existing node. This structure is more flexible and maintains clarity.Also applies to: 193-193, 195-200, 203-203
208-221: Ensurecountis updated indataObject
SincehasMoreChildrenrelies onnode.dataObject.count, it's important that this count accurately reflects the total number of children to be loaded. Otherwise, partial data might be assumed complete.
225-226: Check for potential undefinednode.children
This code assumesnode.childrenis defined. Consider initializingchildrento an empty array when building nodes or adding a quick guard to avoid possible type errors.Also applies to: 230-231
448-459: Potential parse risk forparentComponentId
If the node key’s second-to-last segment is not a valid number,Number(...)will yieldNaN. It may be prudent to validate the segment or handle the parse error gracefully.
485-490: Loading children for root node
This block effectively loads children when the top-level node key exists, neatly chaining further lookups. No immediate issues noted.Also applies to: 492-492
495-507: Highlighting only direct children
Currently, this approach searches and highlights only one level of children. If the target child is nested deeper, it may be missed. Confirm that highlighting deeper descendants is unnecessary.
511-511: Empty array fallback
Settingnodes[0]!.children = []ensures no references to non-existent children. This is a straightforward and safe default.
514-516: Expanding the newly loaded root node
Automatically expanding the first node after loading is a convenient approach for immediate visibility. Looks good.
522-522: Loading children concurrency approach
By appendingparentKeytoloadingChildrenKeys, we prevent multiple concurrent loads of the same node. This is a clean way to manage concurrent tasks.Also applies to: 524-527
542-542: Proper chaining of composite keys
PassingparentKeyandmeta.countintotransformApiDataToTreeFormatkeeps the node structure consistent with deeper composite keys. No concerns here.Also applies to: 545-546
551-551: Cleanup fromloadingChildrenKeys
Removing theparentKeyfromloadingChildrenKeyswhether loading succeeds or fails ensures consistent state management.Also applies to: 557-557
avzz-19
force-pushed
the
PD-1702-sbom-dependency-tree-enhancements
branch
from
3f07a35 to
6e2750a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
app/models/sbom-component.ts (2)
61-63: Handle null or undefined version values gracefully.Currently,
this.versionis assumed to be a valid string. Ifversioncan ever benullorundefined, callingtrim()andreplace()will throw an error. Consider providing a fallback to prevent runtime issues.You could wrap it as follows:
get cleanVersion() { - return this.version.trim().replace(/(^")|("$)/g, ''); + const v = this.version ?? ''; + return v.trim().replace(/(^")|("$)/g, ''); }
65-67: Duplicate cleanup logic suggests extracting a helper.
cleanLatestVersionrepeats the same pattern ascleanVersion. Consider extracting the logic to a shared utility or helper function to adhere to DRY (Don't Repeat Yourself).You could do something like:
+function removeSurroundingQuotes(str: string): string { + return str.trim().replace(/(^")|("$)/g, ''); +} get cleanLatestVersion() { - return this.latestVersion.trim().replace(/(^")|("$)/g, ''); + return removeSurroundingQuotes(this.latestVersion); }app/components/sbom/scan-details/skeleton-loader-tree/index.ts (1)
9-22: Consider removing unusednoopand typing the component fields.
- The
noop()method is unused. If there's no plan to utilize it, removing it could reduce clutter.- For better maintainability, add explicit type annotations to
expandedandskeletonTreeDataif you want to leverage TypeScript’s benefits more fully.export default class SbomScanDetailsSkeletonLoaderTreeComponent extends Component<SbomScanDetailsSkeletonLoaderTreeSignature> { - noop() {} - expanded = ['placeholder-1']; + expanded: string[] = ['placeholder-1']; skeletonTreeData = Array.from( { length: this.args.isFilteredTree ? 3 : 12 }, (_, index) => ({ key: `placeholder-${index + 1}`, title: '', children: [], }) ); }app/components/sbom/component-status/index.ts (1)
21-39: Edge case considerations for invalid or partial semver.
- Currently, the logic returns false (not outdated) when versions are invalid or missing, which may hide actual outdated cases if the data is incomplete.
- If pre-release versions or semver-like patterns (e.g., build metadata) are expected, consider additional checks (e.g.,
semver.prerelease) orsemver.satisfies.You might do:
if (!component?.latestVersion || !component?.version) { return false; } else if ( - !semver.valid(component.cleanVersion) || - !semver.valid(component.cleanLatestVersion) + !semver.validRange(component.cleanVersion) || + !semver.validRange(component.cleanLatestVersion) ) { return false; } ...app/components/sbom/scan-details/skeleton-loader-tree/index.hbs (1)
8-15: Consider using fluid or responsive widths.
The fixed@width='500px'may limit responsiveness on smaller screens or larger layouts. If your design permits, consider a responsive approach (e.g., percentages or breakpoints) to ensure a more flexible layout.app/components/sbom/component-details/summary/index.ts (1)
19-32: Method name may be confusing.
The logic inisLatestVersionLessis sound. However, consider a more descriptive name likeisNewerThanLatestVersionif you intend to indicate that the current version exceeds the official latest.app/components/sbom/scan-details/skeleton-loader-list/index.ts (2)
6-25: Consider flexible or dynamic column widths.
Hardcoding specific column widths (e.g.,width: 150) might hinder adaptability on smaller or larger screens. Evaluating a responsive or percentage-based width may improve layout consistency.
27-29: Clarify usage of shared object references.
Usingnew Array(8).fill({})repeats the same object reference. Although acceptable for skeleton data, be aware that direct mutations on these placeholders could propagate unexpectedly if you were to alter them.app/components/sbom/scan-details/component-tree/index.ts (3)
225-230: Avoid magic numbers for child threshold.
Using9as a cutoff can be confusing; consider extracting this into a named constant or configuration setting for clarity.
485-519: Consider adding error handling for child-loading failures.
While loading and highlighting logic is implemented, catching and surfacing errors (e.g., via a user notification) would improve resiliency.
525-566: Error handling silently swallows exceptions.
Currently, the logic returns an empty array on errors. Consider raising a user-facing notification or logging to inform the user.
📜 Review details
Configuration used: CodeRabbit 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 (21)
app/components/sbom/component-details/overview/index.ts(2 hunks)app/components/sbom/component-details/summary/index.ts(2 hunks)app/components/sbom/component-status/index.ts(3 hunks)app/components/sbom/empty-loading-view/index.hbs(2 hunks)app/components/sbom/empty-loading-view/index.scss(1 hunks)app/components/sbom/scan-details/component-tree/index.hbs(5 hunks)app/components/sbom/scan-details/component-tree/index.ts(9 hunks)app/components/sbom/scan-details/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader-list/index.hbs(1 hunks)app/components/sbom/scan-details/skeleton-loader-list/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.hbs(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.scss(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader/index.hbs(1 hunks)app/models/sbom-component.ts(1 hunks)mirage/factories/sbom-component.ts(1 hunks)package.json(1 hunks)tests/acceptance/sbom-test.js(3 hunks)tests/integration/components/sbom/scan-details/component-tree-test.js(1 hunks)translations/en.json(2 hunks)translations/ja.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- app/components/sbom/scan-details/skeleton-loader/index.hbs
- app/components/sbom/empty-loading-view/index.scss
- app/components/sbom/scan-details/skeleton-loader-tree/index.scss
- package.json
- app/components/sbom/scan-details/index.ts
- app/components/sbom/scan-details/skeleton-loader-list/index.hbs
- app/components/sbom/empty-loading-view/index.hbs
- translations/ja.json
- translations/en.json
- tests/integration/components/sbom/scan-details/component-tree-test.js
- app/components/sbom/component-details/overview/index.ts
- mirage/factories/sbom-component.ts
- app/components/sbom/scan-details/component-tree/index.hbs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (24)
tests/acceptance/sbom-test.js (5)
41-59: Well-structured NotificationsStub implementation.The NotificationsStub class is well-designed for testing purposes, with proper methods to capture notifications for later assertion in tests. It follows the same pattern as other service stubs in the file.
157-157: LGTM! Service registration is properly implemented.The NotificationsStub is correctly registered as a service, consistent with other stub services in the test setup.
471-550: Good test for parent component loading and error handling.This test comprehensively verifies the behavior of the component tree when loading parent components from the API, including proper error handling when an invalid parent ID is provided. The test checks both the UI rendering and the error notification when things go wrong.
552-636: Well-structured hierarchical component tree test.The test effectively verifies the multi-level tree structure functionality by:
- Setting up a three-level component hierarchy
- Verifying initial rendering shows only the root
- Confirming level 2 components appear after expanding the root
- Ensuring level 3 components display after expanding a level 2 component
The conditional response handling in the mock API is particularly well-implemented.
638-711: Excellent pagination test for component children.This test thoroughly verifies the "View More" functionality for child components, including:
- Proper batch loading with the initial 15 components
- Correct rendering of the "View More" button when more items are available
- Loading of additional items when the button is clicked
- Accurate count verification at each step
The pagination implementation with offset and limit parameters is well-tested.
app/components/sbom/component-status/index.ts (2)
58-58: Keep consistency in naming across your codebase.You introduced
isOutdatedin place of a removedisOutdatedgetter on the model level. Ensure any usage referencing the old property is updated. In some references, the variable or method might remaincomponent.isOutdated. Confirm everything is consistent to avoid confusion.
5-5:✅ Verification successful
Ensure semver dependency is declared.
Confirm that
semveris listed in your dependencies (not just devDependencies) if it's used in production code paths. Otherwise, this may cause deployment or runtime issues.
🏁 Script executed:
#!/bin/bash # Description: Verify semver is in the correct dependency list in package.json rg '"semver"' package.json -A5Length of output: 226
Semver dependency is correctly declared
After verifying the package.json, the grep output confirms that
semverwith version "^7.7.1" is present in the dependencies section. No further action is required since the production code import inapp/components/sbom/component-status/index.tscorrectly maps to this dependency.app/components/sbom/scan-details/skeleton-loader-tree/index.hbs (2)
4-4: Looks good – flexible width is appropriate.
No major issues here. The@width='full'attribute aligns well with a responsive layout.
17-28: Tree structure integration is fine.
The new<AkTree>usage withthis.skeletonTreeData,@expanded, and noop handlers appears correct for a skeleton loader scenario.app/components/sbom/component-details/summary/index.ts (4)
6-6: Import usage is valid.
Importing the entiresemverlibrary is common and acceptable.
12-12: Confirm non-null usage of sbomComponent.
Changing from nullable to non-null might be a breaking change for callers. Ensure that all calling sites now provide a validsbomComponent.
48-48: Usage of cleanVersion is appropriate.
This clarifies version strings and avoids potential parsing issues.
52-54: Verify intended fallback behavior.
Hiding the "latestVersion" field whenisLatestVersionLessis true may be correct, but confirm that setting it tonullmeets the desired UI expectations.app/components/sbom/scan-details/skeleton-loader-list/index.ts (2)
1-1: Service import is good.
Importingservicefrom@ember/serviceis standard and correct here.
4-4: Intl import looks fine.
Bringing inIntlServiceensures straightforward i18n usage within the component.app/components/sbom/scan-details/component-tree/index.ts (9)
109-109: Preserve clarity in margin logic.
The simplified condition (removinghasMoreComponents) seems valid; just ensure it aligns with intended UI behavior for first-level nodes without siblings.
163-168: Conditionally show "View More" for dependencies only.
This added guard effectively checks if the node exists, is marked as a dependency, and has a parent before proceeding, which is logically sound.
185-185: Verify consistency with partial loading.
Relying onisLastLoadedChild && this.hasMoreChildren(...)is cleaner than checking global flags. Ensure that parent-child relationships remain accurate even if data is loaded in segments.
189-205: Streamlined approach by passing the entire node.
Switching from passing a node key to anAkTreeNodePropsobject is more intuitive and reduces redundant lookups, making the code more maintainable.
208-223: Check for potential partial-load edge cases.
hasMoreChildrencompares the parent's current child count tonode.dataObject.count. Verify that these values remain in sync when loading children incrementally.
357-388: ValidatehasNextSiblingwith paginated loading.
SettinghasNextSiblingbased onindex < components.length - 1may underestimate siblings if not all data has been fetched. Confirm that sibling logic is accurate in partial-load scenarios.
410-410: Using the newly expanded key is appropriate.
The code ensures the correct node is expanded for additional loading. No further issues found.
448-460: Extracting immediate parent ID from node key.
The approach of using the second-to-last segment in the key is sound for this chain-based scheme. Good job handling multi-level nesting logically.
588-590: Reference to total count for incremental loads.
This usage ofmeta.countis consistent with other methods; see previous partial-load comments to ensure the logic stays aligned with actual fetch pagination.
avzz-19
force-pushed
the
PD-1702-sbom-dependency-tree-enhancements
branch
from
6e2750a to
5722ba2
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
translations/ja.json (2)
608-608: Assess Language Consistency for Tooltip TextThe new translation key
"highlightedNodeTooltip"is added with the English text:
"This is the component from the tree that you are currently viewing"While several entries in this file are localized in Japanese, some remain in English. Please verify whether this entry should be translated for Japanese users or intentionally left in English.
1321-1321: Validate the Parent Component Not Found MessageThe new key
"parentComponentNotFound"is introduced with the value:
"Parent component not found"Confirm that this message meets your localization guidelines for the Japanese audience—if consistency is desired, consider providing a Japanese translation.
app/components/sbom/component-details/summary/index.ts (1)
52-54: Consider adding an explanatory comment for version handling logic.The conditional prevents displaying a latest version when it's less than the current version (an unusual edge case). Consider adding a comment explaining this decision to help future developers understand the logic.
{ label: this.intl.t('sbomModule.latestVersion'), + // Don't show latest version if it's less than the current version (potential data inconsistency) value: this.isLatestVersionLess ? null : this.args.sbomComponent?.cleanLatestVersion, },
📜 Review details
Configuration used: CodeRabbit 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 (21)
app/components/sbom/component-details/overview/index.ts(2 hunks)app/components/sbom/component-details/summary/index.ts(2 hunks)app/components/sbom/component-status/index.ts(3 hunks)app/components/sbom/empty-loading-view/index.hbs(2 hunks)app/components/sbom/empty-loading-view/index.scss(1 hunks)app/components/sbom/scan-details/component-tree/index.hbs(5 hunks)app/components/sbom/scan-details/component-tree/index.ts(9 hunks)app/components/sbom/scan-details/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader-list/index.hbs(1 hunks)app/components/sbom/scan-details/skeleton-loader-list/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.hbs(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.scss(1 hunks)app/components/sbom/scan-details/skeleton-loader-tree/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader/index.hbs(1 hunks)app/models/sbom-component.ts(1 hunks)mirage/factories/sbom-component.ts(1 hunks)package.json(1 hunks)tests/acceptance/sbom-test.js(3 hunks)tests/integration/components/sbom/scan-details/component-tree-test.js(1 hunks)translations/en.json(2 hunks)translations/ja.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- app/components/sbom/scan-details/skeleton-loader-list/index.hbs
- app/components/sbom/scan-details/index.ts
- app/components/sbom/scan-details/skeleton-loader/index.hbs
- app/components/sbom/empty-loading-view/index.hbs
- mirage/factories/sbom-component.ts
- app/components/sbom/empty-loading-view/index.scss
- app/components/sbom/scan-details/skeleton-loader-tree/index.scss
- app/components/sbom/component-details/overview/index.ts
- package.json
- tests/integration/components/sbom/scan-details/component-tree-test.js
- app/models/sbom-component.ts
- app/components/sbom/component-status/index.ts
- app/components/sbom/scan-details/skeleton-loader-list/index.ts
- app/components/sbom/scan-details/component-tree/index.hbs
- translations/en.json
- app/components/sbom/scan-details/component-tree/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (16)
app/components/sbom/scan-details/skeleton-loader-tree/index.ts (3)
9-12: Good addition of the noop handler.The
noop()method works well as an empty callback for the tree component's required event handlers.
12-13: Expanded state initialization looks good.The default expanded state is properly initialized with a placeholder key that corresponds to the first item in your skeleton data.
14-21: Well-structured skeleton data generation.The dynamic skeleton data generation based on the filter state is a good approach. Using Array.from with a factory function is an elegant solution to create placeholder items.
app/components/sbom/scan-details/skeleton-loader-tree/index.hbs (3)
1-7: Good stack configuration update.Changing from padding/spacing to full width improves the layout structure for the skeleton loader.
8-15: Clean implementation of the package name container.The nested stack with proper alignment and styling for the package name skeleton provides a consistent representation of the loading state.
17-28: Excellent tree skeleton implementation.The use of AkTree with the skeleton data creates a more accurate representation of the loading state that matches the actual component tree structure. The tree connectors and consistent heights provide a seamless transition between loading and loaded states.
This implementation addresses the PR objective of implementing a proper skeleton loader for the tree view.
tests/acceptance/sbom-test.js (5)
41-59: Well-structured NotificationsStub implementation.The NotificationsStub class provides a clean implementation for testing notification interactions, which is essential for verifying error messages mentioned in the PR objectives. It correctly implements storage and setting of error, success, and info messages.
157-157: LGTM: Service registration for notifications.Correctly registers the NotificationsStub as a service to enable testing of notification behavior throughout the test suite.
471-550: Great new test case for parent component loading and error handling.This test effectively verifies the parent component API loading and error handling, which directly addresses the PR objective of "adding an error message for incorrect parent URLs on the overview page." The test appropriately:
- Sets up test data with realistic PURL format
- Verifies proper component tree rendering
- Tests error handling for incorrect parent IDs
- Confirms appropriate redirection behavior
552-636: Thorough test for multi-level component hierarchy.This test properly validates the functionality of third-level children in the component tree, which is one of the key enhancements mentioned in the PR objectives. The test effectively:
- Creates a complete three-level hierarchy of components
- Verifies expansion of multiple levels
- Confirms correct count of nodes at each level
638-711: Comprehensive test for "View More" functionality.This test directly addresses the PR objective of "fixing the 'view more' option for children displaying incorrect dependency counts." The implementation:
- Correctly simulates pagination with initial and additional batches
- Verifies the component counts before and after loading more
- Tests the UI interaction for loading additional dependencies
The test structure is clear and the assertions properly validate the expected behavior.
app/components/sbom/component-details/summary/index.ts (5)
6-6: Good addition of semver for version comparisons.Adding the semver library is appropriate for properly handling semantic versioning comparisons, which is critical for accurately determining version relationships.
48-48: Good use of cleanVersion property.Using the cleanVersion property improves display consistency and better handles potential version formatting issues.
12-12:✅ Verification successful
Verify non-null assumption for sbomComponent.
The type has been changed from allowing null to requiring a non-null SbomComponentModel. While this improves type safety, ensure that all call sites provide a valid sbomComponent to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Search for places where this component is used to verify the non-null assumption rg -A 3 -B 3 "ComponentDetails::Summary" --glob "*.hbs"Length of output: 628
Non-Null Assumption Confirmed
The search output from
overview/index.hbsshows that theSbom::ComponentDetails::Summarycomponent is consistently invoked with a non-null@sbomComponent(i.e.@sbomComponent={{@sbomComponent}}). This confirms that the change from a nullable to a non-null type is valid, as all call sites appear to provide a proper value.
19-32:❓ Verification inconclusive
Verify semver version handling for non-standard version strings.
The
semver.valid()check will reject non-standard version formats. Ensure this will not cause problems with any existing data containing non-standard version strings, or add additional handling for such cases.Also applies to: 52-54
🏁 Script executed:
#!/bin/bash # Look for examples of version formats in the system to verify compatibility with semver rg "cleanVersion|cleanLatestVersion" --glob "*.ts" -A 2 -B 2Length of output: 2585
SemVer Comparison: Confirm Handling for Non-Standard Versions
The current implementation uses
semver.valid()in both the summary and component-status components to filter out non-standard version strings. Please verify that this approach is acceptable for all existing data. If legacy or non-strict semver versions are present in your dataset, consider adding additional fallback or conversion logic.
- Locations to review:
app/components/sbom/component-details/summary/index.ts(Lines 19–32 and 52–54)app/components/sbom/component-status/index.ts- Version cleaning in
app/models/sbom-component.ts
19-32:❓ Verification inconclusive
Type inconsistency in isLatestVersionLess implementation.
The implementation uses optional chaining (
?.) when accessing sbomComponent properties, suggesting it might be undefined. However, the type definition on line 12 indicates sbomComponent is required and non-nullable.This creates a disconnect between the type definition and implementation. Either:
- The optional chaining is unnecessary if sbomComponent is guaranteed to exist
- The type definition should allow for null/undefined values
🏁 Script executed:
#!/bin/bash # Check if there are places in the codebase where this component is used with a null/undefined sbomComponent rg -A 5 -B 5 "sbom/component-details/summary" --glob "*.hbs"Length of output: 60
Action Required: Align Optional Chaining with sbomComponent's Type
The getter currently uses optional chaining on
sbomComponent(i.e.this.args.sbomComponent?.cleanLatestVersionand similar), which implies it might be undefined. However, the type definition on line 12 suggests thatsbomComponentis required and non-nullable. To maintain consistency, please either:
- Remove the Optional Chaining: If
sbomComponentis always provided (as the limited search in associated Handlebars templates seems to suggest), remove the?.operators.- Update the Type Definition: If there exists a scenario where
sbomComponentcan indeed be null/undefined, adjust the type accordingly to reflect that possibility.Please manually verify that there are no cases (especially outside of the HBS files) where
sbomComponentmight be missing to confirm the appropriate approach.
Fixes: