PackageTable: Add Licenses column #740
Open
+147
−58
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR introduces a new licenses column to the package table (showing counts and expanding to full lists), adds a WithPackage HOC for fetching package details, refactors vulnerability hooks/components to accept full package objects, updates the table context to support expansion for licenses, and simplifies query function syntax. Entity relationship diagram for package and licenseserDiagram
PURL_DETAILS {
uuid string PK
licenses License[]
}
LICENSE {
license_name string
}
PURL_DETAILS ||--o{ LICENSE : has
Class diagram for WithPackage and PackageVulnerabilities refactorclassDiagram
class WithPackage {
+packageId: string
+children(pkg, isFetching, fetchError): ReactNode
}
class PackageVulnerabilities {
+pkg: PurlDetails | undefined
}
class useVulnerabilitiesOfPackage {
+pkg: PurlDetails | undefined
+data
}
class useVulnerabilitiesOfPackageId {
+packageId: string
+data
+isFetching
+fetchError
}
WithPackage --> "1" PackageVulnerabilities : provides pkg
PackageVulnerabilities --> "1" useVulnerabilitiesOfPackage : uses
WithPackage --> "1" useFetchPackageById : uses
useVulnerabilitiesOfPackageId --> "1" useFetchPackageById : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Remove the commented-out NavLink and raw license count in PackageTable and consolidate all link/display logic into the new PackageLicenses component for clarity.
- Consider batch-fetching license data alongside the package list instead of using useFetchPackageById for each row to avoid N+1 API calls and improve performance.
- Verify that your generatePath calls use the correct route parameters and adjust the spelling between “License” and “Licence” for consistency in the UI.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove the commented-out NavLink and raw license count in PackageTable and consolidate all link/display logic into the new PackageLicenses component for clarity.
- Consider batch-fetching license data alongside the package list instead of using useFetchPackageById for each row to avoid N+1 API calls and improve performance.
- Verify that your generatePath calls use the correct route parameters and adjust the spelling between “License” and “Licence” for consistency in the UI.
## Individual Comments
### Comment 1
<location> `client/src/app/pages/package-list/components/PackageLicenses.tsx:32` </location>
<code_context>
+ packageId: pkg?.uuid || "",
+ })}
+ >
+ {(pkg?.licenses?.length ?? 0)} {(pkg?.licenses?.length ?? 0) > 1 ? "Licences" : "Licence"}
+ </NavLink>
+ </LoadingWrapper>
</code_context>
<issue_to_address>
**suggestion (typo):** The pluralization of 'Licence' uses 'Licences', which is a non-standard spelling.
Use 'Licenses' instead of 'Licences' to align with standard English and codebase conventions.
```suggestion
{(pkg?.licenses?.length ?? 0)} {(pkg?.licenses?.length ?? 0) > 1 ? "Licenses" : "License"}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
client/src/app/pages/package-list/components/PackageLicenses.tsx
Outdated
Show resolved
Hide resolved
gildub
force-pushed
the
add-licenses-to-packages
branch
3 times, most recently
from
459ae12 to
2ce32ba
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #740 +/- ##
==========================================
- Coverage 60.17% 59.68% -0.49%
==========================================
Files 155 156 +1
Lines 2684 2699 +15
Branches 607 613 +6
==========================================
- Hits 1615 1611 -4
- Misses 840 857 +17
- Partials 229 231 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gildub
changed the title
carlosthe19916
requested changes
Sep 22, 2025
client/src/app/pages/package-list/components/PackageLicenses.tsx
Outdated
Show resolved
Hide resolved
gildub
force-pushed
the
add-licenses-to-packages
branch
2 times, most recently
from
aa13a49 to
da42912
Compare
gildub
changed the title
gildub
force-pushed
the
add-licenses-to-packages
branch
4 times, most recently
from
e7438f4 to
7d79c9c
Compare
carlosthe19916
requested changes
Sep 25, 2025
There was a problem hiding this comment.
@gildub thanks for the PR. Please read my comments below. Hopefully we will be able to address those issues. Let me know if you need me to expand on any of the points made.
Comment on lines
29
to
36
| const renderLicenseWithMappings = ( | ||
| license: string, | ||
| mappings: LicenseRefMapping[], | ||
| ) => { | ||
| return mappings.reduce((prev, { license_id, license_name }) => { | ||
| return prev.replaceAll(license_id, license_name); | ||
| }, `${license}`); | ||
| }; |
There was a problem hiding this comment.
This is the exact same function as https://github.com/guacsec/trustify-ui/blob/main/client/src/app/pages/sbom-details/packages-by-sbom.tsx#L43-L54
Maybe we could move it somewhere and reuse it in both places.
| e.license_name, | ||
| item.licenses_ref_mapping, | ||
| )}{" "} | ||
| <Label isCompact>{e.license_type}</Label> |
There was a problem hiding this comment.
Just like in this PR #743 , UX requested to remove the type from the UI as it has no meaning for users. We can confirm that requirement internally with UX
client/src/app/queries/packages.ts
Outdated
| refetch, | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
I see some potential problems here:
- the file
queries/packageshas 2 functions to fetch Packages.useFetchPackagesanduseFetchPackagesDetails. There should not be 2 different implementations for fetching the same entity only one. - The new function
useFetchPackagesDetailsis calling thelistPurland for each element found, it callsgetPurlusingPromise.all. This is critical IMHO:- For the
useFetchPackagesDetailsto finish, it needs to wait forlistPurland then wait for ALLgetPurlto pass. The UI won't be able to render the table of packages until each singlegetPurlis finished.- Take the Vulnerabilities column from the main branch as an example: the UI is able to render the package table even if the count of vulnerabilities hasn't finished.
useFetchPackagesDetailsis implemented in a way of ALL or NOTHING. What if only one singlegetPurlfails, then the wholeuseFetchPackagesDetailswill fail.
- For the
Let me expand a bit on technical requirements using the image below:
- The UI should be able to render the Package List table without the need to wait for its dependant requests to finish. That means, the table should be rendered without waiting for
getPurlto finish. - If one or many
getPurlfail, the table should still render the table with all packages, and render which columns were not being able to be fetch, rendering an error in the column that was not able to be fetched.- In my image above, all rows in the Vulnerability column fail, but it should be possible to handle cases when only part of them failed and the rest passed. E.g. Vulnerability column in row 1 shows data but Vulnerability column in row 2 shows error
- Production environment proved high latency in many endpoint, forcing a single endpoint to depend on another endpoint should be avoided unless there is no escape from it. In this case, it should be possible not to have dependent requests.
- The nature of
Promise.allworks as, wait for all promises inside the array, and then either success all of them or either fail if any of the promises fail. Under this specific context and use case we should not use it. Each row in the package table is independent and there is no reason to block certain rows just because the other rows are still being fetched. And again, keep in mind production environment are many more times slower. We experience already many times timeout errors. We need to handle data fetch orchestration efficiently.
There was a problem hiding this comment.
@carlosthe19916, you're right regarding the PromiseAll() waiting for all items to fetched (or fail).
No worries, let me refactor.
gildub
force-pushed
the
add-licenses-to-packages
branch
from
7d79c9c to
3d9df17
Compare
|
@gildub - Can you fix the failing tests please and let @carlosthe19916 know when this PR is available for him to review again. Thanks |
gildub
force-pushed
the
add-licenses-to-packages
branch
from
3d9df17 to
ab300f5
Compare
gildub
force-pushed
the
add-licenses-to-packages
branch
from
ab300f5 to
89ee121
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `client/src/app/pages/package-list/components/PackageVulnerabilities.tsx:16-18` </location>
<code_context>
- useVulnerabilitiesOfPackage(packageId);
+ const { data } = useVulnerabilitiesOfPackage(pkg);
+
+ if (!data?.summary?.vulnerabilityStatus?.affected?.severities) {
+ return null;
+ }
</code_context>
<issue_to_address>
**suggestion:** Returning null may lead to empty table cells.
Consider displaying a placeholder like 'None' or a dash instead of returning null to make empty cells more informative.
```suggestion
if (!data?.summary?.vulnerabilityStatus?.affected?.severities) {
return <span>-</span>;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai dismiss |
carlosthe19916
requested changes
Sep 29, 2025
There was a problem hiding this comment.
@gildub thanks for the PR. The code is in a better shape.
Please read my code request changes below. We are almost there!
If you need me to expand on any of the points feel free to add a comment
| children: ( | ||
| pkg: PurlDetails | undefined, | ||
| isFetching: boolean, | ||
| fetchError?: Error, |
There was a problem hiding this comment.
maybe fetchError should be AxiosError and not Error?
Comment on lines
7
to
25
| interface PackageVulnerabilitiesProps { | ||
| packageId: string; | ||
| pkg: PurlDetails | undefined; | ||
| } | ||
|
|
||
| export const PackageVulnerabilities: React.FC<PackageVulnerabilitiesProps> = ({ | ||
| packageId, | ||
| pkg, | ||
| }) => { | ||
| const { data, isFetching, fetchError } = | ||
| useVulnerabilitiesOfPackage(packageId); | ||
| const { data } = useVulnerabilitiesOfPackage(pkg); | ||
|
|
||
| if (!data?.summary?.vulnerabilityStatus?.affected?.severities) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <LoadingWrapper | ||
| isFetching={isFetching} | ||
| fetchError={fetchError} | ||
| isFetchingState={<Skeleton screenreaderText="Loading contents" />} | ||
| fetchErrorState={(error) => <TableCellError error={error} />} | ||
| > | ||
| <VulnerabilityGallery | ||
| severities={data.summary.vulnerabilityStatus.affected.severities} | ||
| /> | ||
| </LoadingWrapper> | ||
| <VulnerabilityGallery | ||
| severities={data.summary.vulnerabilityStatus.affected.severities} | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The fact that now the package is injected as a property should not delete other functionally existing already.
- The main branch is able to render an error in a table cell in case there is a backend error. This PR is removing that functionality.
- The original task was done here ✨ [TC-2434] Add TableCellError component #456
Suggestion:
- Make the
PackageVulnerabilitiesPropsto receive 3 properties.pkg(you already have it)isFetching: booleanfetchError: AxiosError
- Keep the whole original code written in the component.
- The only difference will be that in the main branch the 3 properties above come from
useVulnerabilitiesOfPackagebut in this PR those 3 properties will be injected as properties of the component itself
| modifier="breakWord" | ||
| {...getTdProps({ columnKey: "vulnerabilities" })} | ||
| > | ||
| {item.purl[0] && <PackageVulnerabilities pkg={pkg} />} |
There was a problem hiding this comment.
WithPackageexposes 3 properties already, pass all properties fromWithPackagetoPackageVulnerabilities. MeaningisFetchingandfetchError- The conditional
item.purl[0] &&is not necessary anymore. It should be enough to directly render<PackageVulnerabilities/>. The parent<WithPackage />is already doing the verification.
Comment on lines
113
to
187
| @@ -112,14 +143,48 @@ | |||
| /> | |||
| )} | |||
| </Td> | |||
| <WithPackage packageId={item.uuid}> | |||
| {(pkg) => ( | |||
| <Td | |||
| width={20} | |||
| {...getTdProps({ columnKey: "vulnerabilities" })} | |||
| > | |||
| <PackageVulnerabilities pkg={pkg} /> | |||
| </Td> | |||
| )} | |||
| </WithPackage> | |||
| </TableRowContentWithControls> | |||
| </Tr> | |||
| {isCellExpanded(item) ? ( | |||
| <Tr isExpanded> | |||
| <Td | |||
| width={20} | |||
| {...getTdProps({ columnKey: "vulnerabilities" })} | |||
| {...getExpandedContentTdProps({ | |||
| item, | |||
| })} | |||
| className={spacing.pLg} | |||
| > | |||
| <PackageVulnerabilities packageId={item.uuid} /> | |||
| <ExpandableRowContent> | |||
| <div className={spacing.ptLg}> | |||
| {isCellExpanded(item, "licenses") ? ( | |||
| <WithPackage packageId={item.uuid}> | |||
| {(pkg) => ( | |||
| <List isPlain> | |||
| {pkg?.licenses?.map((license, idx) => ( | |||
| <ListItem | |||
| key={`${license.license_name}-${idx}`} | |||
| > | |||
| {license.license_name} | |||
| </ListItem> | |||
| ))} | |||
| </List> | |||
| )} | |||
| </WithPackage> | |||
| ) : null} | |||
| </div> | |||
| </ExpandableRowContent> | |||
| </Td> | |||
| </TableRowContentWithControls> | |||
| </Tr> | |||
| </Tr> | |||
| ) : null} | |||
There was a problem hiding this comment.
Each time I click in the expanded area for licenses there is a new request for fetching the package details. Let's avoid that.
Suggestion:
- Declare
<WithPackage/>just below<Tbody />so you wrap the whole table body thus you can pass all properties thatWithPackageis exposing to all areas insideTBody. This way there is going to be no additional requests each time the user expands the License area.
Comment on lines
+113
to
+131
| <WithPackage packageId={item.uuid}> | ||
| {(pkg) => ( | ||
| <Td | ||
| width={10} | ||
| modifier="truncate" | ||
| {...getTdProps({ | ||
| columnKey: "licenses", | ||
| isCompoundExpandToggle: true, | ||
| item, | ||
| rowIndex, | ||
| })} | ||
| > | ||
| {pkg?.licenses?.length ?? 0}{" "} | ||
| {(pkg?.licenses?.length ?? 0) > 1 | ||
| ? "Licenses" | ||
| : "License"} | ||
| </Td> | ||
| )} | ||
| </WithPackage> |
There was a problem hiding this comment.
What if there is an error while fetching Package? the current code won't render an error but will render 0 Licenses which is wrong and misleading.
Suggestion:
- Wrap the
<Td>tag with something like
<LoadingWrapper
isFetching={isFetching}
fetchError={fetchError}
isFetchingState={<Skeleton screenreaderText="Loading contents" />}
fetchErrorState={(error) => <TableCellError error={error} />}
>
// here write the same code you have for the `<Td/>`
</LoadingWrapper>this way if there is an error then the error will be rendered
There was a problem hiding this comment.
An additional step, definitely in a separate PR not here, would be to create a reusable component like TdWithError that receives the same properties as <Td/> but handles additionally isFetching and fetchError. But this is something we can do later
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses the addition of the licenses column from the following issue:
Fix https://issues.redhat.com/browse/TC-2834
Summary by Sourcery
Add a Licenses column to the package listings with expandable license details and refactor vulnerability fetching and rendering to use a shared package data provider.
New Features:
Enhancements: