Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: HTML report tinkering #1561

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

another-rex
Copy link
Collaborator

@another-rex another-rex commented Feb 3, 2025

Made a series of changes to resolve the issues identified in #1528

Hosted an example here: https://another-rex.github.io/TestPages/Vulnerability%20Scan%20Report.html

To make it easier to see the changes, when reviewing, use this link:
https://github.com/google/osv-scanner/pull/1561/files/bd8d5211e77612b1a0a68a4b00db1d40535fe400..8223593ef84cda34da84a57aecd12d3258ea1463
which select the diffs Excluding the first commit (use shift to select multiple commits). That moves the files around which breaks all of git's diffing. No change other than moving the files and reindenting is done in that first commit.

HTML:

  • Move to actual .js and .css file rather than .html files.
  • Alias and groupid tooltips now put each ID on a new line.
  • Can now click on the entire filter box to change it, not just on the text part.

CSS:

  • Remove max-height in the inner tables, this was making it impossible to have tooltips that escape the container (at least I haven't figured out how to have both).
  • Tooltip box sizing is now dynamic with max-width
  • Tooltips now display upwards instead of downwards
  • Highlight source path better
  • Minor refactor to how the search box is laid out
  • Remove unused css lines.
  • Make iframe bg color black instead of white to avoid flash banging people.

JS:

  • Remove all style edits in javascript, state changes are made with classes now. (TIL classList.toggle() function)
  • Basic linter pass (e.g. use const on variables, define all variables...etc)
  • Run showAllVulns() function at page load.

@another-rex another-rex requested a review from hogo6002 February 3, 2025 02:06
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.31%. Comparing base (a5d0e2b) to head (6014e21).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1561   +/-   ##
=======================================
  Coverage   69.31%   69.31%           
=======================================
  Files         200      200           
  Lines       19038    19038           
=======================================
  Hits        13196    13196           
  Misses       5135     5135           
  Partials      707      707           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hogo6002
Copy link
Contributor

hogo6002 commented Feb 3, 2025

Thanks! can you host one sample HTML on Github

@another-rex
Copy link
Collaborator Author

Copy link
Contributor

@hogo6002 hogo6002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning up the JS file with best practice!

text-align: left;
line-height: 1.5;

max-width: 300px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting max width is not good for all use cases:
Screenshot 2025-02-03 at 5 17 26 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, are there any reasons for placing the tooltip above the text? I feel it's more common to position it below.

@@ -15,7 +15,11 @@
{{ else }}
<div class="tooltip">
<div class="clickable" onclick="openVulnInNewTab('{{ $element.ID }}')">{{ $element.ID }}</div>
<span class="tooltiptext">Group IDs: {{ join $element.GroupIDs ", " }}</span>
<span class="tooltiptext">Group IDs: <br>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change also needs to be added to the "uncalled vulnerability" section below. Ideally, we should only have one template, but "called" and "uncalled" vulnerabilities are displayed slightly differently, so I've kept two sections for now. Perhaps we can just merge them.

padding-top: 20px;
padding-bottom: 30px;
border-bottom: 1px solid rgba(255, 255, 255, 0.12);
max-height: 800px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should keep the max height and make the package details a scrollable window. This would be a better user experience for packages with many vulnerabilities. I see the tooltip issue, but I feel it's not too much problem, the aliases/group ID tooltip window can still be viewed by scrolling down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants