feat(assets): add website field to asset data mapping#271
Conversation
…Dto and update service logic refactor(api): change DataPayloadResult.raw type to object and remove unused report interfaces
…ng vulnerabilities by severity levels
Summary of ChangesHello @l1ttps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience in the console by introducing a robust multi-select severity filter for vulnerabilities, enabling more granular control over displayed data. It also adds a convenient feature to asset technology badges, making them interactive by linking directly to the technology's website. Underlying these UI enhancements are corresponding API and data model updates, alongside a substantial cleanup of outdated security report functionalities in the generated API client. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces two main features: it adds a clickable website link to technology assets and implements a severity filter on the vulnerabilities page. The backend changes correctly add the website field to the data mapping and update the vulnerabilities endpoint to support filtering by severity. The frontend changes integrate the new severity filter and make the technology tooltips clickable.
The code is generally well-written, but I've identified a couple of areas for improvement in the frontend components, mainly concerning code duplication, imperative DOM manipulation, and accessibility. My comments provide suggestions to address these points by adopting more declarative patterns and improving accessibility.
| onError={(e) => { | ||
| // Fallback to globe icon if image fails to load | ||
| const target = e.target as HTMLImageElement; | ||
| target.style.display = 'none'; | ||
| const parent = target.parentElement; | ||
| if (parent) { | ||
| const globeIcon = document.createElement('div'); | ||
| globeIcon.innerHTML = | ||
| '<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round" class="lucide lucide-globe"><circle cx="12" cy="12" r="10"/><path d="M12 2a14.5 14.5 0 0 0 0 20 14.5 14.5 0 0 0 0-20"/><path d="M2 12h20"/></svg>'; | ||
| parent.appendChild(globeIcon); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The onError handler uses direct DOM manipulation (createElement, appendChild) and innerHTML, which is not idiomatic in React and can be a security risk. This logic is also duplicated for the image in the tooltip content (lines 69-80).
A better approach is to use a state variable to track the image loading error and declaratively render a fallback component. This makes the code cleaner, safer, and more aligned with React principles.
Here's an example of how you could refactor this:
const [triggerImgError, setTriggerImgError] = useState(false);
// ... in your JSX
{tech?.iconUrl && !triggerImgError ? (
<img
src={tech.iconUrl}
alt={tech.name}
className="size-4"
onError={() => setTriggerImgError(true)}
/>
) : (
<Boxes className="size-4" />
)}You would apply a similar pattern for the image inside TooltipContent.
| <div | ||
| key={option.value} | ||
| className={cn( | ||
| 'flex items-center space-x-2 rounded-sm px-2 py-1.5 cursor-pointer hover:bg-accent', | ||
| isSelected && 'bg-accent/50', | ||
| )} | ||
| onClick={() => toggleSeverity(option.value)} | ||
| role="button" | ||
| tabIndex={0} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| toggleSeverity(option.value); | ||
| } | ||
| }} | ||
| > | ||
| <Checkbox | ||
| checked={isSelected} | ||
| onCheckedChange={() => toggleSeverity(option.value)} | ||
| className="border-muted-foreground" | ||
| /> | ||
| <div className="flex items-center gap-2 flex-1"> | ||
| <span | ||
| className={cn('w-2 h-2 rounded-full', option.bgColor)} | ||
| aria-hidden="true" | ||
| /> | ||
| <span className={cn('text-sm', option.color)}> | ||
| {option.label} | ||
| </span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
This implementation has some accessibility and event handling issues. Using a div with role="button" and an onClick handler that contains another interactive element (Checkbox) can be confusing for screen reader users and keyboard navigation. Additionally, you have redundant event handlers (onClick on the div and onCheckedChange on the Checkbox) that both call toggleSeverity.
A better pattern is to use a <label> element associated with the Checkbox. This makes the entire row clickable and is more accessible, while also simplifying the event handling.
<label
key={option.value}
htmlFor={`severity-${option.value}`}
className={cn(
'flex items-center space-x-2 rounded-sm px-2 py-1.5 cursor-pointer hover:bg-accent',
isSelected && 'bg-accent/50',
)}
>
<Checkbox
id={`severity-${option.value}`}
checked={isSelected}
onCheckedChange={() => toggleSeverity(option.value)}
className="border-muted-foreground"
/>
<div className="flex items-center gap-2 flex-1">
<span
className={cn('w-2 h-2 rounded-full', option.bgColor)}
aria-hidden="true"
/>
<span className={cn('text-sm', option.color)}>
{option.label}
</span>
</div>
</label>
No description provided.