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

feat: refactor the HTML to use html/template and to inject styling for other IDEs [IDE-326] #564

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

teodora-sandu
Copy link
Contributor

@teodora-sandu teodora-sandu commented Jun 26, 2024

Description

This PR refactors the HTML template used for the OSS Sugestion Panel so that it's more secure and uses the html/template library. This also refactors the template so that it injects icons directly in LS and so other IDEs don't need to know what icons to inject. It adds a ${ideStyle} variable so that in both VSCode and IntelliJ we can inject the IDE specific styling, like we do for Snyk Code.

To test this, need to use snyk/vscode-extension#482.

Checklist

  • Tests added and all succeed
  • Linted
  • README.md updated, if user-facing
  • License file updated, if new 3rd-party dependency is introduced

🚨After having merged, please update the CLI go.mod to pull in latest language server.

@teodora-sandu teodora-sandu changed the title Feat/html oss intellij feat: refactor the HTML to use html/template and to inject styling for other IDEs [IDE-326] Jun 26, 2024
@teodora-sandu teodora-sandu force-pushed the feat/html-oss-intellij branch from 0ec4893 to cfd2bac Compare June 26, 2024 16:08
@teodora-sandu teodora-sandu force-pushed the feat/html-oss-intellij branch from cfd2bac to 760d766 Compare June 26, 2024 16:08
@teodora-sandu teodora-sandu marked this pull request as ready for review June 26, 2024 16:09
@teodora-sandu teodora-sandu requested a review from a team as a code owner June 26, 2024 16:09
return ``
}
}

func getGitHubIconSvg() template.HTML {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering, if caching/memory will become an issue with 1000s of issues if we embed the svg like that. Do you know how Chrome handles this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean we might run into trouble ebecause we would be using a lot of memory if we cache the same SVG multiple times?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking about this and at the very beginning of this migration, when we were just supporting IntelliJ and writing the HTML and CSS from scratch, we embedded everything in one single HTML file, trading DRY for velocity.

Now that the project has grown and VSCode and IntelliJ are loading CSS rules anyway, I think we could start considering removing the CSS and SVGs from the Language Server and moving them to the IDEs as separate assets.

Benefits:

  1. Reduced Redundancy: Each vulnerability doesn't need to embed the same CSS and SVGs.
  2. Improved Performance: Smaller HTML payloads and better caching of static assets.

WDYT @teodora-sandu @bastiandoetsch

"IssueOverview": html.MarkdownToHTML(string(overview)),
"CVEs": additionalData.Identifiers.CVE,
"CWEs": additionalData.Identifiers.CWE,
"CvssScore": fmt.Sprintf("%.1f", additionalData.CvssScore),
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we're at it - should we just update it to CVSSv4? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! But I didn't change it in the LS message type, since that would affect our IDEs being able to read this value

@teodora-sandu
Copy link
Contributor Author

snyk/vscode-extension#482 should be merged first before this.

Comment on lines 102 to 108
func getExploitMaturity(issue snyk.OssIssueData) string {
if len(issue.Exploit) > 0 {
return fmt.Sprintf("<div class='summary-item maturity'><div class='label font-light'>Exploit maturity</div>"+
"<div class='content'>%s</div></div>", issue.Exploit)
return issue.Exploit
} else {
return ""
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove the else?

func getExploitMaturity(issue snyk.OssIssueData) string {
	if len(issue.Exploit) > 0 {
		return issue.Exploit
	}
	return ""
}

@teodora-sandu teodora-sandu merged commit ed077be into main Jul 8, 2024
16 checks passed
@teodora-sandu teodora-sandu deleted the feat/html-oss-intellij branch July 8, 2024 10:36
ShawkyZ pushed a commit that referenced this pull request Jul 10, 2024
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