perf: use pre-fetched advisory severity summary on SBOM list#1127
Open
ctron wants to merge 2 commits into
Open
perf: use pre-fetched advisory severity summary on SBOM list#1127ctron wants to merge 2 commits into
ctron wants to merge 2 commits into
Conversation
Contributor
Reviewer's GuideFrontend now consumes pre-fetched advisory severity summaries from the SBOM list API (via the new Sequence diagram for SBOM list advisories prefetchsequenceDiagram
actor User
participant SbomListPage
participant useFetchSBOMs
participant Backend as Backend_v3_sbom_API
participant SbomTable
participant SBOMVulnerabilities
participant VulnerabilityGallery
User ->> SbomListPage: Open SBOM list page
SbomListPage ->> useFetchSBOMs: useFetchSBOMs(params)
useFetchSBOMs ->> Backend: GET /v3/sbom?advisories=true
Backend -->> useFetchSBOMs: SBOM list with advisories
useFetchSBOMs -->> SbomListPage: items[{ id, advisories, ... }]
SbomListPage ->> SbomTable: Render with items
SbomTable ->> SBOMVulnerabilities: SBOMVulnerabilities(advisories)
SBOMVulnerabilities ->> VulnerabilityGallery: VulnerabilityGallery(severities)
Note over SBOMVulnerabilities,VulnerabilityGallery: No per-row GET /sbom/{id}/advisory requests
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider explicitly handling the case where
item.advisoriesisundefinedornullinSBOMVulnerabilities(e.g., via a fallback UI or prop type refinement) so the table cell behavior is clear when advisory data is missing. - The
severitiesobject inSBOMVulnerabilitiesis recreated on every render via object spread; if this component is used in a large table, memoizing or building it earlier (e.g., in the parent or viauseMemo) could avoid unnecessary re-renders ofVulnerabilityGallery.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider explicitly handling the case where `item.advisories` is `undefined` or `null` in `SBOMVulnerabilities` (e.g., via a fallback UI or prop type refinement) so the table cell behavior is clear when advisory data is missing.
- The `severities` object in `SBOMVulnerabilities` is recreated on every render via object spread; if this component is used in a large table, memoizing or building it earlier (e.g., in the parent or via `useMemo`) could avoid unnecessary re-renders of `VulnerabilityGallery`.
## Individual Comments
### Comment 1
<location path="client/src/app/pages/sbom-list/components/SbomVulnerabilities.tsx" line_range="23" />
<code_context>
+ advisories,
}) => {
- const { data, isFetching, fetchError } = useVulnerabilitiesOfSbom(sbomId);
+ const severities = { ...DEFAULT_SEVERITIES, ...advisories };
- return (
</code_context>
<issue_to_address>
**issue (bug_risk):** Spreading a possibly null/undefined `advisories` object can throw at runtime.
Because `advisories` is now optional (`advisories?: SbomAdvisorySummary | null`), spreading it directly in `{ ...DEFAULT_SEVERITIES, ...advisories }` will throw if it’s `null` or `undefined`. Please guard or default it, e.g. `const severities = { ...DEFAULT_SEVERITIES, ...(advisories ?? {}) };`, or handle the missing data before rendering `VulnerabilityGallery`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1127 +/- ##
==========================================
- Coverage 53.30% 53.27% -0.04%
==========================================
Files 270 270
Lines 5879 5877 -2
Branches 1842 1845 +3
==========================================
- Hits 3134 3131 -3
- Misses 2443 2444 +1
Partials 302 302
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ctron
force-pushed
the
feature/sbom-advisory-summary
branch
2 times, most recently
from
503dd23 to
51635f1
Compare
Consume the new `?advisories=true` query parameter on the SBOM list
endpoint to display severity counts per row, eliminating N+1 requests
to `GET /sbom/{id}/advisory` (~4s each).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ctron
force-pushed
the
feature/sbom-advisory-summary
branch
from
51635f1 to
c07c1d9
Compare
|
|
||
| import { LoadingWrapper } from "@app/components/LoadingWrapper"; | ||
| import { TableCellError } from "@app/components/TableCellError"; | ||
| import type { SbomAdvisorySummary } from "@app/client"; |
Collaborator
There was a problem hiding this comment.
I don't see the SbomAdvisorySummary type in openapi spec, there are only SbomAdvisory (that was used in getSbomAdvisories before) or a new type RequestedField_HashMap_HashMap (as was added in this commit ).
Comment on lines
+11
to
+23
| const DEFAULT_SEVERITIES: { [key in ExtendedSeverity]: number } = { | ||
| unknown: 0, | ||
| none: 0, | ||
| low: 0, | ||
| medium: 0, | ||
| high: 0, | ||
| critical: 0, | ||
| }; | ||
|
|
||
| export const SBOMVulnerabilities: React.FC<SBOMVulnerabilitiesProps> = ({ | ||
| sbomId, | ||
| advisories, | ||
| }) => { | ||
| const { data, isFetching, fetchError } = useVulnerabilitiesOfSbom(sbomId); | ||
| const severities = { ...DEFAULT_SEVERITIES, ...advisories }; |
Collaborator
There was a problem hiding this comment.
I believe the better pattern would be not to add const DEFAULT_SEVERITIES where everything is equals to zero but to change VulnerabilityGalleryProps from this:
interface VulnerabilityGalleryProps {
severities: { [key in ExtendedSeverity]: number };
}
To this:
interface VulnerabilityGalleryProps {
severities: Partial<{ [key in ExtendedSeverity]: number }>;
}
This would let us keep SBOMVulnerabilities component clean and just use it like that:
export const SBOMVulnerabilities: React.FC<SBOMVulnerabilitiesProps> = ({ advisories }) => (
<VulnerabilityGallery severities={advisories ?? {}} />
);
|
|
||
| import type { SbomHead, SourceDocument } from "@app/client"; | ||
| import type { | ||
| SbomAdvisorySummary, |
Collaborator
There was a problem hiding this comment.
Same thing here as in the previous comment
I don't see the SbomAdvisorySummary type in openapi spec, there are only SbomAdvisory (that was used in getSbomAdvisories before) or a new type RequestedField_HashMap_HashMap
ctron
added
the
backport release/0.5.z
Use generated RequestedFieldHashMapHashMap type instead of non-existent SbomAdvisorySummary, make VulnerabilityGallery accept partial severities, and simplify SBOMVulnerabilities. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@stanislavsemeniuk tried to convince claude to fix the review comments. Maybe you can have another look. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
?advisories=truequery parameter on the SBOM list endpoint to display severity counts per rowGET /sbom/{id}/advisory(~4s each) that were made per table rowDependencies
Depends on backend PR: guacsec/trustify#2437 which adds the
?advisories=truequery parameter toGET /v3/sbom.Changes
SbomVulnerabilities.tsx- rewritten to accept pre-fetched advisory summary instead of fetching per rowsboms.ts- passadvisories: truein the list querysbom-table.tsx- passitem.advisoriesto the component instead ofitem.idsbom-context.ts- addadvisoriesfield to the item typetypes.gen.ts- not committed (gitignored), will be regenerated from the updated OpenAPI specTest plan
GET /sbom/{id}/advisoryresponses🤖 Generated with Claude Code
Summary by Sourcery
Optimize SBOM list vulnerability display by consuming pre-fetched advisory summaries instead of per-row advisory API calls.
New Features:
Enhancements: