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: display Priority Score in Snyk Code suggestions [IDE-30] #422

Merged
merged 10 commits into from
Jan 12, 2024

Conversation

cat2608
Copy link
Contributor

@cat2608 cat2608 commented Jan 4, 2024

Description

Priority Score is rendered dynamically within the issue details.

The Priority Score will become available once the Language Server propagates it, as detailed in this PR: snyk/snyk-ls#425

Checklist

  • Tests added and all succeed
  • Linted
  • CHANGELOG.md updated
  • README.md updated, if user-facing

Screenshots / GIFs

Before After
missing-priority-score priority-score
priority-score.mp4

Priority Score is rendered dynamically within the issue details.

Co-authored-by: Jason Luong <[email protected]>
@cat2608 cat2608 changed the title feat: display Priority Score in Snyk Code suggestions feat: display Priority Score in Snyk Code suggestions [HEAD-1187] Jan 4, 2024
@cat2608 cat2608 force-pushed the feat/HEAD-938-add-priority-score branch from 011f8a0 to 527a185 Compare January 5, 2024 20:26
`showSuggestionMeta` handles the dynamic generation of
issue type, CWE links, issue position, and priority score.
Updated `severity` property in the `Suggestion` from a generic `string`
to a specific union type: `'Low' | 'Medium' | 'High'`.
This ensures that severity can only be one of these three explicit values.
The `toggleSeverityIcons` function updates the visibility of severity icons based on the `currentSeverity`.
If `currentSeverity` is undefined, all icons are hidden.
@cat2608 cat2608 force-pushed the feat/HEAD-938-add-priority-score branch from 9c7b118 to debe8bc Compare January 7, 2024 14:33
…gestion`

This change aims to make clear that these variables are references to DOM elements,
not values or other types of objects.
This change aims to avoid overwriting existing classes list by directly manipulating `className`.
Comment on lines 163 to 167

function appendIdentifierSpan(identifiers: Element, id: string, link?: string) {
const delimiter = document.createElement('span');
delimiter.innerText = ' | ';
delimiter.className = 'delimiter';
identifiers.appendChild(delimiter);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When testing https://github.com/dexidp/dex I realised that we had an extra delimiter in Snyk Configuration Issue panel

Before After
Before After

Comment on lines +402 to 408
// Append the priority score if available.
if (suggestion.priorityScore !== undefined) {
const priorityScoreElement = document.createElement('span');
priorityScoreElement.className = 'suggestion-meta';
priorityScoreElement.textContent = `Priority Score: ${suggestion.priorityScore}`;
metaElem.appendChild(priorityScoreElement);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not available it the element is not added to the DOM
Screenshot 2024-01-07 at 19 52 46

/* eslint-disable @typescript-eslint/no-unsafe-assignment */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have decided to enable this rule with the idea that little by little we will add the missing types. I'm happy to revert it if the team finds it so.

Comment on lines +197 to +212
const severityMap = {
Low: 1,
Medium: 2,
High: 3,
};
return suggestion
? {
value: stringMap[suggestion.severity],
text: suggestion.severity,
}
: undefined;

const severityAllowedValues = Object.keys(severityMap);
if (!severityAllowedValues.includes(severity)) {
return undefined;
}

return {
value: severityMap[severity],
text: severity,
};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this function since the check suggestion ? {} : undefined didn't guarantee that suggestion contained a severity property or that severity contained valid values to match severityMap[suggestion.severity].

Comment on lines +225 to 240
function toggleSeverityIcons(severity: HTMLElement, currentSeverity: CurrentSeverity | undefined) {
const severityIcons = severity.querySelectorAll('img');
const validIconsIds: { [key: number]: string } = { 1: 'sev1', 2: 'sev2', 3: 'sev3' };

if (!currentSeverity) {
severityIcons.forEach(icon => (icon.className = 'icon hidden'));
return;
}

severity.setAttribute('title', currentSeverity.text);

severityIcons.forEach(icon => {
const currentIconId = validIconsIds[currentSeverity.value];
icon.className = icon.id === currentIconId ? 'icon' : 'icon hidden';
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opted for strict comparison icon.id === currentIconId instead of testing it with contains: n.id.includes(currentSeverity.value) to ensure matching

@cat2608 cat2608 marked this pull request as ready for review January 7, 2024 20:18
@cat2608 cat2608 requested a review from a team as a code owner January 7, 2024 20:18
@cat2608 cat2608 changed the title feat: display Priority Score in Snyk Code suggestions [HEAD-1187] feat: display Priority Score in Snyk Code suggestions [IDE-30] Jan 7, 2024
Copy link
Contributor

@acke acke left a comment

Choose a reason for hiding this comment

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

Well done @cat2608, for amazing refactoring and implementation!

- Adjusted colors for 'added' and 'removed' elements to improve visibility in high contrast mode.
- Ensured consistent background color for 'code' elements within 'added' and 'removed' sections.

This commit addresses visual issues in the high contrast theme of VSCode.
@bastiandoetsch bastiandoetsch merged commit c9f170e into main Jan 12, 2024
6 checks passed
@bastiandoetsch bastiandoetsch deleted the feat/HEAD-938-add-priority-score branch January 12, 2024 14:28
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