feat: add advisor preview #127

sangress · 2022-01-05T08:51:34Z

Add advisor score to vscode extension

Score will show for packages with vulnerabilities, as shown in the following image:

To see this feature in action, use a package.json file with a package that known to have a score lower then 70, then hover with your mouse over the Advisor Score text with the mouse to see the advisor score details.
Example dependencies:

    "axios": "^0.20.0",
    "express": "^4.17.1",
    "firebase": "^5.8.2",
    "lodash": "^4.17.11",
    "numeral": "^2.0.6", # lower then 70
    "vue": "^2.5.21",
    "vue-router": "^3.0.6",
    "chino": "0.0.0", # lower then 70
    "vuejs": "3.0.1" # lower then 70

CLAassistant · 2022-01-05T08:51:41Z

All committers have signed the CLA.

michelkaporin · 2022-01-10T08:42:30Z

src/snyk/advisor/AdvisorTypes.ts

@@ -0,0 +1,13 @@
+export type AdvisorScoreLabel = {


Names of the files should follow camel casing - see other files.

I did see other files before. For example: https://github.com/snyk/vscode-extension/blob/feat/advisor_score/src/snyk/common/commands/types.ts#L10

I mean files, not types :)

michelkaporin · 2022-01-10T11:07:49Z

src/snyk/advisor/editor/EditorDecorator.ts

@@ -0,0 +1,75 @@
+import * as vscode from 'vscode';


We should rely on vscode abstractions instead, for testing purposes.

Please eleborate.

When you write unit tests, "vscode" module cannot be resolved since tests run not within VS Code. To overcome this, we invert our dependency on vscode module through dependency inversion mechanisms. You can see inversions in src/snyk/common/vscode folder.

michelkaporin · 2022-01-10T11:26:59Z

src/snyk/common/vscode/types.ts

@@ -25,3 +25,28 @@ export type DecorationOptions = vscode.DecorationOptions;
 export type ThemeColor = vscode.ThemeColor;
 export type ThemableDecorationInstanceRenderOptions = vscode.ThemableDecorationInstanceRenderOptions;
 export type CodeActionProviderMetadata = vscode.CodeActionProviderMetadata;
+export enum Language {


This file contains mappings between local types and vscode API ones.
The following types are unrelated to vscode, thus shouldn't be part of /vscode/types.ts.

michelkaporin · 2022-01-10T13:31:14Z

src/snyk/advisor/editor/EditorDecorator.ts

+      hoverMessageMarkdown.appendMarkdown(`| ${label}: | | | ${score?.labels[label]} |`);
+      hoverMessageMarkdown.appendMarkdown('\n');
+    });
+    hoverMessageMarkdown.appendMarkdown(`[More Details](http://snyk.io/advisor/npm-package/${score.name})`);


Can we make use of existing configuration variables instead of hardcoding the link here? E.g. defaultAuthHost from configuration.ts

michelkaporin · 2022-02-03T15:23:47Z

src/snyk/advisor/services/AdvisorDisposable.ts

+      if (supportedLanguage) {
+        const modules = getModules(fileName, document.getText(), supportedLanguage).filter(isValidModuleName);
+        const promises = modules
+          .map(module => this.vulnerabilityCountProvider.getVulnerabilityCount(module, supportedLanguage))


Why do you need this map? Vulnerability Count doesn't relate to advisor.

Because we only show advisor score for packages that has vulnerabilities.

Is it the case in IntelliJ Advisor integration as well? I didn't see the feature plan and tbh this wouldn't be my expectation 🤔

That's the requirements that I got from Oren - @snyk/advisor

Please verify if we're aligned on that requirement with IntelliJ plugin.

michelkaporin · 2022-02-03T15:33:05Z

src/snyk/advisor/services/AdvisorService.ts

+      }
+
+      const result = new CliError(err, '');
+      console.error('Failed to get scores', result.error.toString());


Errors should be reported via Logger class. See logger injection in other classes.

src/snyk/common/configuration/configuration.ts

michelkaporin · 2022-02-03T15:39:40Z

src/snyk/common/api/apiСlient.ts

+  post<T = unknown, R = AxiosResponse<T>>(url: string, data?: unknown, config?: AxiosRequestConfig): Promise<R> {
+    this.http.interceptors.request.use(req => {
+      if (req.method === 'post') {
+        req.baseURL = this.configuration.baseApiUrl;


This class should work with the same base Url for API. If there's a need to work with different API url, you either create another class or introduce type parameter here indicating what type of Snyk Api we want to POST to.

You want to create a new class just for a single method? I think it's a bit overkill and duplication of classes.

It is important to follow single responsibility principle. Use another suggested option then, if you don't like another class. But first determine what's the difference between these two APIs and only then make a decision please.

I understand and I agree on that, it looks like a duplication now that I look at it. But I think this is not the case because apiClient is common and generic.
Another solution can be that we'll use a helper function/class that will return the right url according to the request method (in this case). What do you think about it?

If you can keep single responsibility principle with a new function/class, then sure :) Sometimes it's better to duplicate rather than making a thing less comprehensible when glancing at the code.

michelkaporin · 2022-02-03T15:40:12Z

src/snyk/common/vscode/nativeModules.ts

@@ -0,0 +1,44 @@
+export default [


Please don't duplicate the code.

I moved it to common, how is that a duplication?

snyk/vscode-extension/src/snyk/snykOss/constants/nativeModules.ts

src/snyk/common/vscode/languageConsts.ts

michelkaporin · 2022-02-03T15:42:38Z

Issues found from quick glance:

When opening and closing tab, advisor score is not getting reported inline. It seems to show the score only if the extension is started when manifest file is opened.
Score is prefixed with pipe when no vuln count is shown.

3. Hover doesn't show where does the Advisor info come from. User can be misled and not know that it's actually Snyk. Please run this by UX.

package.json

src/snyk/advisor/editor/editorDecorator.ts

src/snyk/common/vscode/editorDecorator.ts

src/snyk/common/configuration/configuration.ts

michelkaporin

It's quite many comments I've left but it's much better after the last iteration 👍. Let's have a call when you will go through them. There are few bits that I still want to discuss.

Something you shouldn't also forget is testing the implemented bits 😉

src/snyk/advisor/services/AdvisorDisposable.ts

src/snyk/advisor/services/AdvisorApiClient.ts

src/snyk/advisor/services/AdvisorDisposable.ts

src/snyk/common/vscode/parsing.ts

src/snyk/common/vscode/types.ts

src/snyk/common/vscode/languageConsts.ts

src/snyk/advisor/messages.ts

src/snyk/advisor/services/AdvisorApiClient.ts

src/snyk/base/modules/snykLib.ts

src/snyk/common/editor/types.ts

src/snyk/advisor/services/advisorApiClient.ts

src/snyk/common/configuration/configuration.ts

src/snyk/common/vscode/parsing.ts

src/snyk/extension.ts

src/snyk/advisor/editor/editorDecorator.ts

src/snyk/advisor/services/advisorService.ts

michelkaporin · 2022-03-28T06:24:42Z

src/snyk/base/modules/snykLib.ts

@@ -141,7 +142,8 @@ export default class SnykLib extends BaseSnykModule implements ISnykLib {

    try {
      const oldResult = this.ossService.getResult();
-      const result = await this.ossService.test(manual, reportTriggeredEvent);
+      const result: OssResult = await this.ossService.test(manual, reportTriggeredEvent);


Can you revert file to the original? There's no need to write type for the variable, let's have changes clean for git blame to work fine when needed.

src/snyk/common/messages/advisorMessages.ts

src/snyk/common/vscode/parsing.ts

src/snyk/snykOss/services/vulnerabilityCount/parsers/packageJsonParser.ts

src/test/unit/advisor/services/advisorService.test.ts

src/snyk/advisor/services/advisorService.ts

michelkaporin · 2022-04-01T12:54:09Z

As discussed offline, I'll be taking this PR over and merging it afterwards.

michelkaporin

Bless me for self approving it.

michelkaporin · 2022-04-06T14:53:35Z

Thank you for your work on it @sangress 👏

sangress · 2022-06-08T15:02:39Z

Thank you for your work on it @sangress 👏

Thanks @michelkaporin for mentioning.
It's good to see it live; it was my longest PR ever, but I did enjoy working on it.

Good luck with future features :)

sangress requested a review from michelkaporin January 5, 2022 08:51

sangress requested a review from a team as a code owner January 5, 2022 08:51

sangress marked this pull request as draft January 27, 2022 08:56

sangress marked this pull request as ready for review February 2, 2022 15:06

michelkaporin reviewed Feb 3, 2022

View reviewed changes

michelkaporin reviewed Mar 11, 2022

View reviewed changes

package.json Show resolved Hide resolved

src/snyk/advisor/editor/editorDecorator.ts Outdated Show resolved Hide resolved

src/snyk/common/vscode/editorDecorator.ts Outdated Show resolved Hide resolved

src/snyk/common/configuration/configuration.ts Show resolved Hide resolved

michelkaporin reviewed Mar 11, 2022

View reviewed changes

michelkaporin reviewed Mar 16, 2022

View reviewed changes

michelkaporin reviewed Mar 17, 2022

View reviewed changes

src/snyk/extension.ts Outdated Show resolved Hide resolved

michelkaporin reviewed Mar 28, 2022

View reviewed changes

michelkaporin reviewed Mar 29, 2022

View reviewed changes

src/snyk/advisor/services/advisorService.ts Outdated Show resolved Hide resolved

michelkaporin reviewed Mar 29, 2022

View reviewed changes

src/snyk/advisor/services/advisorService.ts Outdated Show resolved Hide resolved

michelkaporin changed the title ~~Feat/advisor score~~ feat: add advisor preview Apr 6, 2022

michelkaporin previously approved these changes Apr 6, 2022

View reviewed changes

feat: add advisor preview

1b06383

michelkaporin dismissed their stale review via 1b06383 April 6, 2022 14:46

michelkaporin force-pushed the feat/advisor_score branch from e955749 to 1b06383 Compare April 6, 2022 14:46

michelkaporin approved these changes Apr 6, 2022

View reviewed changes

michelkaporin merged commit 213c781 into main Apr 6, 2022

michelkaporin deleted the feat/advisor_score branch April 6, 2022 14:53

This was referenced Apr 21, 2022

[Snyk] Upgrade @snyk/code-client from 4.5.0 to 4.9.0 NOUIY/vscode-extension#15

Open

[Snyk] Upgrade @snyk/code-client from 4.5.0 to 4.10.0 NOUIY/vscode-extension#18

Open

snyk-bot mentioned this pull request May 17, 2022

[Snyk] Upgrade @snyk/code-client from 4.5.0 to 4.11.0 NOUIY/vscode-extension#25

Open

cat2608 mentioned this pull request Dec 4, 2023

fix: security upgrade axios from 0.27.2 to 1.6.0 [HEAD-1036] #387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add advisor preview #127

feat: add advisor preview #127

sangress commented Jan 5, 2022 •

edited

Loading

CLAassistant commented Jan 5, 2022 •

edited

Loading

michelkaporin Jan 10, 2022

sangress Feb 7, 2022

michelkaporin Feb 7, 2022

sangress Feb 7, 2022

michelkaporin Jan 10, 2022

sangress Feb 7, 2022

michelkaporin Feb 7, 2022

michelkaporin Jan 10, 2022

michelkaporin Jan 10, 2022

michelkaporin Feb 3, 2022

sangress Feb 7, 2022

michelkaporin Feb 7, 2022

sangress Feb 7, 2022

michelkaporin Feb 7, 2022

michelkaporin Feb 3, 2022

michelkaporin Feb 3, 2022

sangress Feb 7, 2022

michelkaporin Feb 7, 2022

sangress Feb 7, 2022

michelkaporin Feb 14, 2022

michelkaporin Feb 3, 2022

sangress Feb 7, 2022

michelkaporin Feb 7, 2022

michelkaporin commented Feb 3, 2022

michelkaporin left a comment

michelkaporin Mar 28, 2022

michelkaporin commented Apr 1, 2022

michelkaporin left a comment

michelkaporin commented Apr 6, 2022

sangress commented Jun 8, 2022

feat: add advisor preview #127

feat: add advisor preview #127

Conversation

sangress commented Jan 5, 2022 • edited Loading

Add advisor score to vscode extension

CLAassistant commented Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelkaporin commented Feb 3, 2022

michelkaporin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelkaporin commented Apr 1, 2022

michelkaporin left a comment

Choose a reason for hiding this comment

michelkaporin commented Apr 6, 2022

sangress commented Jun 8, 2022

sangress commented Jan 5, 2022 •

edited

Loading

CLAassistant commented Jan 5, 2022 •

edited

Loading