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 branch selection in the tree node [IDE-446] #489

Merged
merged 11 commits into from
Jul 17, 2024

Conversation

acke
Copy link
Contributor

@acke acke commented Jul 12, 2024

Description

This pr covers the UI parts of selecting a base branch for delta findings. No actually calls to language server for available local branches, or to set the choose branch is a part of this pr.

This PR handles tree view for Security Code and Code Quality depending on the Delta Findings feature flag. When enabled, the title for the tree node shows

  • Code Security - New Issues
  • Code Quality - New Issues

Checklist

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

Screenshots / GIFs

code-and-quality-with-deltas.mp4

@acke acke requested a review from a team as a code owner July 12, 2024 14:13
@acke acke self-assigned this Jul 12, 2024
@acke acke requested a review from ShawkyZ July 12, 2024 14:13
@acke acke force-pushed the feat/choose_netnet_base_branch branch from 2c2bdc7 to d608822 Compare July 12, 2024 14:19
@acke acke changed the title Feat/choose netnet base branch Feat/choose netnet base branch WIP Jul 12, 2024
@cat2608 cat2608 changed the title Feat/choose netnet base branch WIP feat: display branch selection in the tree node [IDE-446] Jul 15, 2024
@cat2608 cat2608 force-pushed the feat/choose_netnet_base_branch branch 3 times, most recently from 8ed8df4 to fc3a11c Compare July 15, 2024 12:57
@cat2608 cat2608 force-pushed the feat/choose_netnet_base_branch branch from fc3a11c to 4307560 Compare July 15, 2024 13:21
@cat2608 cat2608 force-pushed the feat/choose_netnet_base_branch branch from 0d1d525 to 0a7438b Compare July 15, 2024 15:49
Comment on lines 64 to 66
async setBaseBranch(): Promise<void> {
await this.executeCommand(SNYK_SET_BASE_BRANCH_COMMAND, this.authService.setBaseBranch.bind(this.authService));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this will probably not stay like this, but use a setting

Copy link
Contributor

@bastiandoetsch bastiandoetsch left a comment

Choose a reason for hiding this comment

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

I think this needs to wait until the LS side is implemented for correct integration. Correct me, if there's another story that handles the LS integration.

@cat2608 cat2608 force-pushed the feat/choose_netnet_base_branch branch 2 times, most recently from 591ecee to 731d15a Compare July 16, 2024 16:55
@cat2608 cat2608 force-pushed the feat/choose_netnet_base_branch branch from 731d15a to e82fd27 Compare July 16, 2024 17:08
Comment on lines +297 to +300
if (!nIssues) {
return '✅ Congrats! No issues found!';
}
return `Snyk found ${nIssues} issue${nIssues === 1 ? '' : 's'}`;
Copy link
Contributor

@cat2608 cat2608 Jul 16, 2024

Choose a reason for hiding this comment

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

Small refactor for consistency. Agreed with Arvid and Neil 🙌

Before After
before after

Comment on lines -313 to +328
"when": "view == 'snyk.views.analysis.code.security' || view == 'snyk.views.analysis.code.quality' || view == 'snyk.views.analysis.oss' || view == 'snyk.views.analysis.configuration'",
"when": "view == 'snyk.views.analysis.code.security' || view == 'snyk.views.analysis.code.security.delta' || view == 'snyk.views.analysis.code.quality' || view == 'snyk.views.analysis.code.quality.delta' || view == 'snyk.views.analysis.oss' || view == 'snyk.views.analysis.configuration'",
"group": "navigation"
},
{
"command": "snyk.settings",
"when": "view == 'snyk.views.analysis.code.security' || view == 'snyk.views.analysis.code.quality' || view == 'snyk.views.analysis.oss' || view == 'snyk.views.welcome' || view == 'snyk.views.analysis.configuration'",
"when": "view == 'snyk.views.analysis.code.security' || view == 'snyk.views.analysis.code.security.delta' || view == 'snyk.views.analysis.code.quality' || view == 'snyk.views.analysis.code.quality.delta' || view == 'snyk.views.analysis.oss' || view == 'snyk.views.welcome' || view == 'snyk.views.analysis.configuration'",
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds the gear ⚙️ and play ▶️ buttons on the right of the title

@@ -42,18 +42,20 @@ export default class CodeSecurityIssueTreeProvider extends IssueTreeProvider {
protected getIssueFoundText(nIssues: number, ignoredIssueCount: number): string {
if (nIssues > 0) {
let text;

if (nIssues === 1) {
text = `${nIssues} vulnerability found by Snyk`;
Copy link
Contributor

@bastiandoetsch bastiandoetsch Jul 17, 2024

Choose a reason for hiding this comment

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

Shouldn't this be issues, too?

Suggested change
text = `${nIssues} vulnerability found by Snyk`;
text = `${nIssues} issue found by Snyk`;

@@ -42,18 +42,20 @@ export default class CodeSecurityIssueTreeProvider extends IssueTreeProvider {
protected getIssueFoundText(nIssues: number, ignoredIssueCount: number): string {
if (nIssues > 0) {
let text;

if (nIssues === 1) {
text = `${nIssues} vulnerability found by Snyk`;
} else {
text = `✋ ${nIssues} vulnerabilities found by Snyk`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
text = `✋ ${nIssues} vulnerabilities found by Snyk`;
text = `✋ ${nIssues} issues found by Snyk`;

getBaseBranch(): TreeNode | null {
const deltaFindingsEnabled = this.configuration.getDeltaFindingsEnabled();

//TODO: get the actual base branch from Snyk Language Server
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just ask a config setting, what's the base branch for the folder. LS sends a folderConfig notification for all known workspace folders when they are registered at LS.

@cat2608 cat2608 force-pushed the feat/choose_netnet_base_branch branch from e82fd27 to ac09779 Compare July 17, 2024 12:15
@cat2608 cat2608 merged commit f10f698 into main Jul 17, 2024
14 checks passed
@cat2608 cat2608 deleted the feat/choose_netnet_base_branch branch July 17, 2024 12:46
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.

4 participants