Add commands for navigation of alerts #1568

asgerf · 2022-10-05T21:00:19Z

Some time ago I added commands for navigating the steps on a path, but didn't add commands for navigating between alerts - this PR does that.

This PR adds the commands:

CodeQL: Navigate Up in Local Result Viewer
CodeQL: Navigate Down in Local Result Viewer
CodeQL: Navigate Left in Local Result Viewer
CodeQL: Navigate Right in Local Result Viewer

The intent is that you can bind keyboard shortcuts to these commands to navigate alerts with the keyboard.

When viewing 'raw' results, such as from quick-eval, the alert-navigation commands move between rows and columns.

When viewing alerts, the left/right commands instead move into or out of nesting levels of tree view:

Alerts
- Paths
  - Path steps

There are no commands for clicking the individual links in an interpreted alert message, but you can sort of achieve this by viewing the #select result set as a raw result table.

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.
[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.

aeisenberg

This is great! Thanks for creating this PR. I think there are several functions in result-keys.ts that can be removed. And I have not tried this myself, but I will soon.

extensions/ql-vscode/src/interface.ts

extensions/ql-vscode/src/pure/result-keys.ts

extensions/ql-vscode/src/view/results/raw-results-table.tsx

asgerf · 2022-10-06T08:45:20Z

Thanks for the quick review @aeisenberg! I wasn't expecting reviews while the PR is in draft state, as I'd like to get some more feedback on the UI, but I'm happy to iterate on it if you're OK with the scope of the PR changing along the way.

In particular, I'm not 100% convinced these commands are actually right. I feels a little off to have two different set of key bindings for moving up/down in different parts of the alert view.

It's possible we want to change the commands to be more general "move left/right/up/down". Like the arrow keys in a common tree-view, with the caveat that moving up/down shouldn't leave the current level (moving down past the last path step should not skip over to the next path - that would be annoying).

RasmusWL · 2022-10-06T13:24:00Z

When viewing 'raw' results, the alert-navigation commands move between rows (raw results cannot have paths, so the path step commands do nothing). This view is a table with an arbitrary number of columns, each of which could be a link to a source location. In principle there should be a way to navigate sideways here, but for now it just remembers which column you clicked last time, and will keep showing you that column when switching rows via the command.

Nice 💪

moving down past the last path step should not skip over to the next path - that would be annoying

When thinking about this, I feel like it would work OK, but without having tried it I guess it's hard to really say. Is your feeling about annoyance based on having tried it and it being bad, or from your intuitive feeling?

asgerf · 2022-10-06T17:22:05Z

I've pushed another commit that changes the commands to left/right/up/down behaviour. On my machine I bound the keys to cmd+shift+{I,J,K,L} in a WASD-like pattern. Personally I like this a bit better, but I'd be very interested in feedback about how this works for others.

The internal command ID for up/down is previousPathStep/nextPathStep so as to not break existing keybindings. I couldn't find a way to deprecate commands in vscode/

We register a handler for the old command ID, but do not mention it in package.json. This seems to be backward compatible without polluting the command palette.

asgerf · 2022-10-11T09:12:40Z

After using this for a few days I noticed some quirky behaviour when stepping down while a node was expanded. I pushed another commit to collapse the previous node when stepping up or down.

Perhaps it's best if I keep using it for a few more days before moving ahead.

RasmusWL · 2022-10-20T08:51:38Z

I've tried out the navigation in this PR, and overall I'm a big fan. Especially for quick evaling predicates, being able to move left/right for columns is amazing!

However, for path-problems I'm not personally 100% on board with the required left/right movement. I was always pressing DOWN when on the path level (I only looked at alerts with a single path), and then got annoyed when it didn't work 😄

Suggestion: When you expand an alert, could you be taken directly to the first path-step of the first path? -- at least if there is only one path.

aeisenberg

That's really neat. It works great for me. I have a handful of small comments.

If we are not going to bind the new commands to key shortcuts by default, then we should update the README with some instructions on how to do it manually. Perhaps it would fit after the ### Running a query section.

extensions/ql-vscode/CHANGELOG.md

aeisenberg · 2022-10-20T18:09:09Z

extensions/ql-vscode/src/pure/result-keys.ts


 /**
 * Looks up a specific result in a result set.
 */
-export function getResult(sarif: sarif.Log, key: Result): sarif.Result | undefined {
+export function getResult(sarif: sarif.Log, key: Result | Path | PathNode): sarif.Result | undefined {
  if (sarif.runs.length === 0) return undefined;


Minor: The body of this function can be replaced with this:

return sarif.runs?.[0]?.results?.[key.resultIndex];

It's really outside the scope of this PR, but it's a minor readability improvement if you want to add it.

extensions/ql-vscode/src/pure/result-keys.ts

extensions/ql-vscode/src/view/results/alert-table.tsx

Document the changes in this PR: github/vscode-codeql#1568

Co-authored-by: Andrew Eisenberg <[email protected]>

aeisenberg

Minor change in command name, and then it looks good.

extensions/ql-vscode/package.json

Co-authored-by: Andrew Eisenberg <[email protected]>

asgerf · 2022-10-24T11:29:46Z

Thanks for the feedback @RasmusWL

However, for path-problems I'm not personally 100% on board with the required left/right movement. I was always pressing DOWN when on the path level (I only looked at alerts with a single path), and then got annoyed when it didn't work 😄

Yeah the same thing happened to me, and I also tried various ways of skipping over the 'path' layer. But it never felt right. It generates that uncanny feeling where the UI tries to be really smart, but then becomes less predictable and less consistent with other UI components. My take is that the current version is something you can get used to, and then it stops being annoying, whereas the other one will always feel a little off.

asgerf · 2022-10-24T11:37:13Z

I updated the PR description to match the current behavior.

RasmusWL · 2022-10-24T11:41:17Z

However, for path-problems I'm not personally 100% on board with the required left/right movement. I was always pressing DOWN when on the path level (I only looked at alerts with a single path), and then got annoyed when it didn't work smile

Yeah the same thing happened to me, and I also tried various ways of skipping over the 'path' layer. But it never felt right. It generates that uncanny feeling where the UI tries to be really smart, but then becomes less predictable and less consistent with other UI components. My take is that the current version is something you can get used to, and then it stops being annoying, whereas the other one will always feel a little off.

I want this feature more than I want to discuss it, so although I'm not 100% convinced I will learn this, I say we ship it 🚢

aeisenberg

I'm feeling good about this. I think this will be a nice change. I still think we can add some more documentation for this. We can either expand the readme, or put it into the codeql extension docs on the codeql microsite.

But, that could be done later. Nice work.

Feel free to merge when you're ready.

asgerf · 2022-10-25T07:50:35Z

Thanks @aeisenberg! It seems I'm not authorized to merge in this repo. Can you trigger the merge?

asgerf added 6 commits October 5, 2022 22:56

Add 'show next/previous alert' commands

ab933fc

Replace 'expanded' with a Set<number>

2949fc3

Replace the expansion index with the result key

bb61b5e

Ensure nodes are expanded

3c4682e

Also show selection in raw result view

20dea5e

Make raw result view respond to navigation events

125f638

aeisenberg reviewed Oct 5, 2022

View reviewed changes

Switch commands to up/down/left/right semantics

88bfd19

asgerf added 3 commits October 7, 2022 09:22

Bugfix in getPathNode

0f6100c

Rename command IDs.

5a69465

We register a handler for the old command ID, but do not mention it in package.json. This seems to be backward compatible without polluting the command palette.

Remove unsed parts of result-keys.ts

f759eed

asgerf marked this pull request as ready for review October 7, 2022 14:45

asgerf requested a review from a team as a code owner October 7, 2022 14:45

asgerf added 2 commits October 10, 2022 15:36

Added change note

4871728

When stepping up or down, collapse the previous node

d08e005

asgerf added 2 commits October 19, 2022 14:09

Reveal panel on navigate, to prevent webview destruction

45b6288

Scroll selected item into view

0e3679d

aeisenberg reviewed Oct 20, 2022

View reviewed changes

asgerf added a commit to asgerf/codeql that referenced this pull request Oct 21, 2022

Docs: Mention new navigation commands

23697db

Document the changes in this PR: github/vscode-codeql#1568

asgerf mentioned this pull request Oct 21, 2022

Docs: Mention new navigation commands github/codeql#10925

Merged

asgerf and others added 5 commits October 21, 2022 13:54

Update extensions/ql-vscode/CHANGELOG.md

ecc07a5

Co-authored-by: Andrew Eisenberg <[email protected]>

Update extensions/ql-vscode/src/view/results/alert-table.tsx

cbf15e6

Co-authored-by: Andrew Eisenberg <[email protected]>

Title-case command names, like other commands

53bb9d7

Use null-aware accessors in getResult

65777b5

Consistently check for undefined rather than nullish

d4a58a6

asgerf added 2 commits October 21, 2022 14:26

Update a new more nullish checks

e1a56dd

Mention keyboard navigation in README

bdf7208

aeisenberg reviewed Oct 21, 2022

View reviewed changes

extensions/ql-vscode/package.json Outdated Show resolved Hide resolved

asgerf and others added 4 commits October 24, 2022 13:13

Update extensions/ql-vscode/package.json

9cb4b9d

Co-authored-by: Andrew Eisenberg <[email protected]>

Fix bad suggestion merge in package.json

0acf9f7

Merge branch 'main' into asgerf/navigate-alerts

ead1fb4

Fix incorrect merge resolution in changelog

b480f8f

aeisenberg approved these changes Oct 24, 2022

View reviewed changes

aeisenberg merged commit 09b30fe into github:main Oct 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commands for navigation of alerts #1568

Add commands for navigation of alerts #1568

asgerf commented Oct 5, 2022 •

edited

Loading

aeisenberg left a comment

asgerf commented Oct 6, 2022

RasmusWL commented Oct 6, 2022

asgerf commented Oct 6, 2022

asgerf commented Oct 11, 2022

RasmusWL commented Oct 20, 2022

aeisenberg left a comment

aeisenberg Oct 20, 2022

aeisenberg left a comment

asgerf commented Oct 24, 2022

asgerf commented Oct 24, 2022 •

edited

Loading

RasmusWL commented Oct 24, 2022

aeisenberg left a comment •

edited

Loading

asgerf commented Oct 25, 2022

Add commands for navigation of alerts #1568

Add commands for navigation of alerts #1568

Conversation

asgerf commented Oct 5, 2022 • edited Loading

Checklist

aeisenberg left a comment

Choose a reason for hiding this comment

asgerf commented Oct 6, 2022

RasmusWL commented Oct 6, 2022

asgerf commented Oct 6, 2022

asgerf commented Oct 11, 2022

RasmusWL commented Oct 20, 2022

aeisenberg left a comment

Choose a reason for hiding this comment

aeisenberg Oct 20, 2022

Choose a reason for hiding this comment

aeisenberg left a comment

Choose a reason for hiding this comment

asgerf commented Oct 24, 2022

asgerf commented Oct 24, 2022 • edited Loading

RasmusWL commented Oct 24, 2022

aeisenberg left a comment • edited Loading

Choose a reason for hiding this comment

asgerf commented Oct 25, 2022

asgerf commented Oct 5, 2022 •

edited

Loading

asgerf commented Oct 24, 2022 •

edited

Loading

aeisenberg left a comment •

edited

Loading