ui improvements + issues fixes + new options + mocked mode + other

[nnillni]

• [ ] Tests written and linted ℹ︎
• [x] Commit history is tidy & follows Contributing guidelines ℹ︎

What this does

As per the issue https://github.com/snyk-tech-services/backstage-plugin-snyk/issues/161 and some bugs and UI issues I had, I decided to work a little bit over the weekend with this plugin.

Notable Plugin Changes:

• Added / Changed options of the snyk configuration
  • appHost: This is the old AppHost, renamed with yaml/backstage best practices (camelCase). Defaults to app.snyk.io
  • apiVersion: You can now change the API version via configuration. Default 2023-11-06
  • mocked: You can now mock the plugin so you don't need a Snyk account to test/show it off. Defaults to false
  • showResolvedInGraphs: This flag is to maintain old functionality, where graphs showed the issues resolved too. Defaults to false.
• Improved some UI colors / animations when loading. This is personal preference so feel free to give feedback 👍🏻
  • Resolved issues are now strikethrough and on the bottom of the list
  • Issues are ordered by Status (open > ignored > resolved) then (critical > high > medium > low)
  • If 0 vulnerabilities there's a gradiant of greens now
  • Decreased size to the Snyk Overview from 6 to 3 by default (it was too big)
  • Increased size from 3 to 4 to the graphs in the Snyk tab
  • Changed some labels
  • Fixed the size of the Overview box being different when loading/error than loaded
• Added DEVELOPER_GUIDE.md, as it was hard to set up the environment.
• Updated all dependencies I could

Notes for the reviewer

• I suppose the breaking changes is just the AppHost -> appHost change and also ignoring the Resolved issues by default in the graphs. There's a lot of UI changes as well so it might be considered breaking for some adopters.
• Changed a lot of internal namings / casing to use best practices for classes / React Components
• Added mocks in the utils folder to simulate the API
• I still need to update the docs with pictures, but overall the work is done
• No tests were done, as I don't see any from before either and I am not too versed in them either

I haven't tested with a real Snyk account, as I am in my personal computer now, but I might try next week to add the fork on our developer portal 👍🏻

Screenshots

UPDATE1: Changed back apiHost to appHost (still breaking because camelCase now)

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[aarlaud]

Wow, this is awesome ! Thanks so much for the contribution here, really really appreciate it. I'll take some time to review this asap (might be next week, travelling quite a bit this week).

One note about app hostname, it's different from api. api is for the api calls, app host is for links to the project/issues page in the Snyk UI, which can be a bit tricky depending on which environment someone may be using. It's not 100% consistent across the board to be able to infer if from the api one iirc, but I'll take a look to see what I can do there.

thanks again !

[nnillni]

Oh, I didn't catch that one!

I will check that part as well and see if it can be unified in a single host. If it cannot be unified, then we can just create appHost and apiHost or something of the likes instead 👍🏻 I thought it was used for the API :P

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[aarlaud]

I don't know why, but the configApi doesn't work for me. It says I have to change the visibility but I can see it set to frontend correctly in the interface. Do you happen to know what I'm missing by any chance?

[nnillni]

Hmmm, I had some issues too with the visibility in the Frontend for some variables, and I don't remember how I solved it but maybe rebuilding the plugin or the backstage fixed it?

I don't think I have changed much about the configApi though, I will try to test it today see if I can make it work.

Which version of Backstage are you using? I think I tested all of this on 1.19.6, maybe something might've changed on 1.20

[aarlaud]

Hmmm, I had some issues too with the visibility in the Frontend for some variables, and I don't remember how I solved it but maybe rebuilding the plugin or the backstage fixed it?

I don't think I have changed much about the configApi though, I will try to test it today see if I can make it work.

Which version of Backstage are you using? I think I tested all of this on 1.19.6, maybe something might've changed on 1.20

Brand new install from last night

[nnillni]

Is this the error you get? I am testing it locally with a brand new instance too. Looking into it 👀

[aarlaud]

Is this the error you get? I am testing it locally with a brand new instance too. Looking into it 👀

No, I'm not getting anything there but the console in the browser essentially returns "undefined" and the warning mentions the visibility matter.

[nnillni]

Alright, disregard the error I reported, it seems it was because when doing npx @backstage/create-app it installed the 1.10 and the dependencies were wrong.

Then I followed my developer guide and found the same visibility issue. After looking for it for hours, I "fixed" it with the following steps:

• Do yarn link on the plugin root directory
• Do yarn link backstage-plugin-snyk on the packages/app folder in backstage repo
• Do yarn --cwd packages/app add backstage-plugin-snyk on the backstage repo too

It seems after linking only it will fail the visibility stuff, but apparently if you add the package it works (?). It is ugly and a workaround because both versions are installed (one on the root node_modules and one on the packages/app folder) but atleast it works.

It seems to me that it isn't properly loading the config.d.ts file unless you add the real pkg to backstage. If you know a way to do so or to fix it so it actually loads the one from development without installing the package let me know, hope that made sense 👍🏻

[aarlaud]

Alright, practically everything seems to work, except one thing for me: we need to keep the default api version to 2023-06-19~experimental. There is a lot of variations on how folks want to resolve the projects they wanna see, ranging from project slugs, target name or ID, etc, and the current targets api supporting this logic requires this version I mention. The other endpoints work as well in that version. We need to keep that version for now. One may choose to customize it in their app config if they need a different api version for any reason.

Still for backward compatibility, at least for now and to avoid more breaking changes after the last upgrade, I think I'd like to do this

 getSnykAppHost() {
    const appHost = this.configApi.getOptionalString("snyk.AppHost") ?? this.configApi.getOptionalString("snyk.appHost")
    return appHost ?? "app.snyk.io";
  }

and keep this interface

export interface Config {
  /** Optional configurations for the Snyk plugin */
  snyk?: {
    /**
     * @visibility frontend
     */
    appHost?: string;
    /**
     * @visibility frontend
     */
    AppHost?: string; // kept for backward compatibility
    /**
     * @visibility frontend
     */
    apiVersion?: string;
    /**
     * @visibility frontend
     */
    mocked?: boolean;
    /**
     * @visibility frontend
     */
    showResolvedInGraphs?: boolean;
  };
}

eventually we'll drop it as part of another breaking change whenever we get to do something that will require more breaking changes. Not the cleanest but feels a satisfactory middleground. WDYT?

Otherwise, I'm seeing a bit of a weird behavior with the routing as well in the tabbed cards when swapping between tabs (the url doesn't update but just adds the same chunk of paths at the end of the existing one, losing its sense.... But since that part hasn't changed much in your PR, so I suspect it's the result of our double yarn add thing, or the package linking...

[nnillni]

Sounds good to me! I can look into these changes when I have some time.

Confirming that the double-linking when using the tab happens on localhost, but I tried on our deployed environment and works properly 👍🏻

I will check too if there is a way to not have both installed so this can be tested properly in dev and update the developer guide

[sbarrypoppulo]

I believe the duplicate URL paths is related to this issue (now resolved) - https://github.com/backstage/backstage/issues/21487#issuecomment-1840158811

[cla-bot[bot]]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nnillni on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[nnillni]

@aarlaud I think the changes work with old and new versions of Backstage. I tried updating locally to React 18 as new versions of backstage use it but didn't quite make it work for some reason.

What would be the best path forward here? Should we release this version for now when ready and make a v3 for the upgrade to react 18 later on?

[aarlaud]

@aarlaud I think the changes work with old and new versions of Backstage. I tried updating locally to React 18 as new versions of backstage use it but didn't quite make it work for some reason.

What would be the best path forward here? Should we release this version for now when ready and make a v3 for the upgrade to react 18 later on?

Probably would be a good move yes. I'll do another round of testing before pushing this out.

[aarlaud]

@nnillni please rebase the clabot conflict since I added another contributor in the meantime. I think it's the only conflict basically. Will merge from there if you are all done.

[nnillni]

There you go!

[aarlaud]

:tada: This PR is included in version 2.2.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: