Add config options for default sorting/filtering values in variant analysis results view by shati-patel · Pull Request #2392 · github/vscode-codeql

shati-patel · 2023-04-28T14:23:58Z

Adds new settings for configuring the default display of variant analysis results:

This also changes the default display to sort by number of results (instead of by name).

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
- See internal issue.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.
- Not needed: We have a general page about customizing settings here, and we've listed the specific new settings in the changelog.

…alysis results view Co-authored-by: Robert <robertbrignull@github.com>

shati-patel · 2023-04-28T16:06:36Z

extensions/ql-vscode/src/view/variant-analysis/__tests__/VariantAnalysis.spec.tsx

+    // Without this await, `getByDisplayValue` could not find any selected dropdown option.
+    await new Promise((resolve) => setTimeout(resolve, 0));
+
+    expect(screen.getByDisplayValue("With results")).toBeInTheDocument();
+    expect(screen.getByDisplayValue("Number of results")).toBeInTheDocument();


For some reason, await new Promise((resolve) => setTimeout(resolve, 0)); was necessary to get these "default" expectations to pass 🤔

Not sure why, but it works 😅 If anyone knows more about this, let us know!

I'm not sure why this is happening (it could be that some of the VS Code UI toolkit shadow DOM is only rendered on the next tick or something like that), but a slightly more reliable way to solve this is by waiting for the element to start existing:

await waitFor(() => screen.getByText("With results"));

This way we don't depend on the exact tick and it will still fail if it doesn't find the element after some time.

This is perfect! Thanks ✨

The default filterSortState is no longer to show "all" repositories, so I've created an explicit "all-inclusive" filterSortState for the purpose of these tests.

extensions/ql-vscode/package.json

koesie10 · 2023-05-01T12:47:05Z

extensions/ql-vscode/src/view/variant-analysis/__tests__/VariantAnalysis.spec.tsx

+    // Without this await, `getByDisplayValue` could not find any selected dropdown option.
+    await new Promise((resolve) => setTimeout(resolve, 0));
+
+    expect(screen.getByDisplayValue("With results")).toBeInTheDocument();
+    expect(screen.getByDisplayValue("Number of results")).toBeInTheDocument();


I'm not sure why this is happening (it could be that some of the VS Code UI toolkit shadow DOM is only rendered on the next tick or something like that), but a slightly more reliable way to solve this is by waiting for the element to start existing:

await waitFor(() => screen.getByText("With results"));

This way we don't depend on the exact tick and it will still fail if it doesn't find the element after some time.

koesie10 · 2023-05-01T12:47:32Z

extensions/ql-vscode/src/view/variant-analysis/__tests__/VariantAnalysis.spec.tsx

+  const postMessage = async (msg: ToVariantAnalysisMessage) => {
+    await act(async () => {
+      // window.postMessage doesn't set the origin correctly, see
+      // https://github.com/jsdom/jsdom/issues/2745
+      window.dispatchEvent(
+        new MessageEvent("message", {
+          source: window,
+          origin: window.location.origin,
+          data: msg,
+        }),
+      );
+
+      // The event is dispatched asynchronously, so we need to wait for it to be handled.
+      await new Promise((resolve) => setTimeout(resolve, 0));
+    });
+  };


Now that we're using this in at least two tests, do you think it would make sense to move this to a separate file and export the function?

Good idea! I wasn't sure where best to put the shared file. Currently in src/view/common/post-message.ts but I'm open to better suggestions 😅

shati-patel · 2023-05-01T16:28:46Z

Thank you for the helpful comments, @koesie10 😍

robertbrignull · 2023-05-02T09:54:26Z

When pairing on this on Friday, we decided to add the sort/filter information to the setVariantAnalysis message. This was for ease and because we're always already sending one of those messages at the time we want the sort/filter information.

Over the weekend I've thought more and I think it's still unclear what the best approach is, but I'm leaning more in favour of having a new message type and just sending two separate messages one after the other. That way the naming of the messages is more clear, and we don't have the sort/filter state being optional in the message.

What do you think?

These details of the webview messages are purely internal and there's no backwards compatibility concerns, so we could also do this as a followup PR after this one.

shati-patel · 2023-05-02T15:08:52Z

When pairing on this on Friday, we decided to add the sort/filter information to the setVariantAnalysis message. This was for ease and because we're always already sending one of those messages at the time we want the sort/filter information.

Over the weekend I've thought more and I think it's still unclear what the best approach is, but I'm leaning more in favour of having a new message type and just sending two separate messages one after the other. That way the naming of the messages is more clear, and we don't have the sort/filter state being optional in the message.

What do you think?

These details of the webview messages are purely internal and there's no backwards compatibility concerns, so we could also do this as a followup PR after this one.

Thanks, @robertbrignull! That seems like a reasonable suggestion. Doesn't sounds too big, so I'll change that in this PR 👍🏽

robertbrignull

LGTM from my point of view. @koesie10 do you want to take another look / have any comments or are you happy?

shati-patel · 2023-05-03T10:12:55Z

Thanks for the reviews! I'll merge/fix conflicts once the 1.8.4 release is complete 🚀

koesie10

LGTM, I just found one minor inconsistency

extensions/ql-vscode/src/pure/interface-types.ts

Co-authored-by: Koen Vlaswinkel <koesie10@users.noreply.github.com>

shati-patel · 2023-05-03T13:34:59Z

LGTM, I just found one minor inconsistency

Woops! Thanks, @koesie10. Fixed.

shati-patel · 2023-05-03T13:35:39Z

Slight niggle (just discussed with @robertbrignull offline) is that I haven't yet implemented showing "in progress" repos.

If we only display With results by default, then the "in progress" repos are hidden from view when you kick off a variant analysis so it's harder to tell what is being analyzed. If no repos return results, then the view remains empty. See internal linked issue(s)!

Instead of fixing that in this PR, I'm going to change the default back to All here and merge. Then we can think a bit more about the "in progress" filtering in a follow-up.

Sorry for the noise! 🙉

shati-patel · 2023-05-03T14:08:35Z

Made All the default again in 006dd7e. There are other changes, like the permissiveFilterSortState in tests, which are no longer strictly necessary. But I'll keep that in, since I think it makes sense to decouple the default filter sort state from those tests.

@robertbrignull, would you mind giving this another look to make sure it matches what we discussed? 🙇🏽‍♀️

robertbrignull

Yep, this matches what I thought we discussed.

I also tried running it and it works well. I'd be happy to ship this.

shati-patel and others added 4 commits April 28, 2023 15:12

Add config options for default sorting/filtering values in variant an…

e40cf13

…alysis results view Co-authored-by: Robert <robertbrignull@github.com>

Update changelog

a8b17cf

Change defaultFilterSortState to match the default config value

d6cd73d

Add test for filtering and sorting in results view

e4da4c5

shati-patel commented Apr 28, 2023

View reviewed changes

shati-patel added 2 commits April 28, 2023 17:29

Fix test to use a more permissive filterSortState

d8d4e43

The default filterSortState is no longer to show "all" repositories, so I've created an explicit "all-inclusive" filterSortState for the purpose of these tests.

Fix more tests

1970af9

shati-patel marked this pull request as ready for review May 1, 2023 12:06

shati-patel requested review from a team as code owners May 1, 2023 12:06

koesie10 reviewed May 1, 2023

View reviewed changes

shati-patel added 3 commits May 1, 2023 16:45

Rename config option to say "default"

798714c

Move "postMessage" helper into separate file

462eaf4

Use "waitFor" in test to get expected text

f120b8e

Set the filterSortState in a separate message

21f4fbc

robertbrignull approved these changes May 2, 2023

View reviewed changes

koesie10 approved these changes May 3, 2023

View reviewed changes

extensions/ql-vscode/src/pure/interface-types.ts Outdated Show resolved Hide resolved

extensions/ql-vscode/src/pure/interface-types.ts Outdated Show resolved Hide resolved

Fix capitalisation of "setFilterSortStateMessage"

db29deb

Co-authored-by: Koen Vlaswinkel <koesie10@users.noreply.github.com>

shati-patel marked this pull request as draft May 3, 2023 13:35

shati-patel added 2 commits May 3, 2023 14:55

Merge branch 'main' into shati-patel/sorting-config

5ca5593

Make All the default filter value

006dd7e

shati-patel marked this pull request as ready for review May 3, 2023 14:08

robertbrignull approved these changes May 3, 2023

View reviewed changes

shati-patel enabled auto-merge (squash) May 3, 2023 14:48

shati-patel merged commit 71611e0 into main May 3, 2023

shati-patel deleted the shati-patel/sorting-config branch May 3, 2023 14:52

shati-patel mentioned this pull request May 3, 2023

Update screenshots for changes to variant analysis results view github/codeql#13021

Merged

Conversation

shati-patel commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

shati-patel Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

koesie10 May 1, 2023

Choose a reason for hiding this comment

Uh oh!

shati-patel May 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

koesie10 May 1, 2023

Choose a reason for hiding this comment

Uh oh!

koesie10 May 1, 2023

Choose a reason for hiding this comment

Uh oh!

shati-patel May 1, 2023

Choose a reason for hiding this comment

Uh oh!

shati-patel commented May 1, 2023

Uh oh!

robertbrignull commented May 2, 2023

Uh oh!

shati-patel commented May 2, 2023

Uh oh!

robertbrignull left a comment

Choose a reason for hiding this comment

Uh oh!

shati-patel commented May 3, 2023

Uh oh!

koesie10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shati-patel commented May 3, 2023

Uh oh!

shati-patel commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shati-patel commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertbrignull left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shati-patel commented Apr 28, 2023 •

edited

Loading

shati-patel commented May 3, 2023 •

edited

Loading

shati-patel commented May 3, 2023 •

edited

Loading