Skip to content

Implement setVariantAnalysis message#1562

Merged
koesie10 merged 7 commits intomainfrom
koesie10/set-variant-analysis-message
Oct 5, 2022
Merged

Implement setVariantAnalysis message#1562
koesie10 merged 7 commits intomainfrom
koesie10/set-variant-analysis-message

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Oct 3, 2022

It's probably easiest to review this PR commit-by-commit.

This implements the setVariantAnalysis message, both in the extension host and in the webview. This will only send this message when the monitor retrieves a new variant analysis summary, but this should be extended in the future once we have a central place for managing variant analyses. Then, the view could receive the variant analysis message when the view is loaded (i.e. https://github.com/github/vscode-codeql/pull/1550/files#diff-29b0e9f4687addd3c5aaa5c980a9d079de9f0e1e804cd79aacf81f3239fcd2d1R50-R53).

I was able to test that this (merged in with main to open the view automatically) will open the view, show the incorrect data until the first poll returns data. When the polling has properly started, it will show the correct data.

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.

@koesie10 koesie10 added the secexp label Oct 3, 2022
@koesie10 koesie10 requested review from a team as code owners October 3, 2022 13:41
@elenatanasoiu elenatanasoiu force-pushed the elenatanasoiu/download-variant-analysis-results branch 2 times, most recently from 77c20cf to f56f017 Compare October 4, 2022 16:07
Base automatically changed from elenatanasoiu/download-variant-analysis-results to main October 5, 2022 10:43
To be able to send messages to the open view for a variant analysis, we
need to have a reference to the view. This is done by keeping track of
all open views in a dictionary indexed by their variant analysis ID.

We currently only allow one view per variant analysis, but do allow
multiple variant analysis views to be open at a time. In the future, we
may want to allow multiple views per variant analysis (such that e.g.
"Split right" works), but this is not supported yet.

The reason for the indirection through the interfaces is to prevent
circular dependencies between the variant analysis view and the manager.
This will ensure that when we return a new variant analysis summary from
the API, the variant analysis object will be updated.
All fields in the variant analysis skipped repositories are optional,
but this was not properly defined in the API types. This will correct
the types and the functions processing the data such that they handle
non-existing fields.
This will store the variant analysis in the React state and replace it
when the `setVariantAnalysis` message is received.
@koesie10 koesie10 force-pushed the koesie10/set-variant-analysis-message branch from 3a96162 to 6941584 Compare October 5, 2022 10:54
Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu left a comment

Choose a reason for hiding this comment

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

Left a small question mostly for my own sake, but it's clear enough to follow the commits in order to understand how this is being set up.

Comment thread extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts
@koesie10 koesie10 merged commit 9002313 into main Oct 5, 2022
@koesie10 koesie10 deleted the koesie10/set-variant-analysis-message branch October 5, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants