Add commands for navigating the steps on a path #175
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcreedcmu
previously approved these changes
Nov 25, 2019
| export function getResult(sarif: sarif.Log, key: Result): sarif.Result | undefined { | ||
| if (sarif.runs.length === 0) return undefined; | ||
| if (sarif.runs[0].results === undefined) return undefined; | ||
| let results = sarif.runs[0].results; |
There was a problem hiding this comment.
This can be const, yeah?
| @@ -72,7 +74,8 @@ export function getPathRelativeToSourceLocationPrefix(sourceLocationPrefix: stri | |||
| export class PathTable extends React.Component<PathTableProps, PathTableState> { | |||
| constructor(props: PathTableProps) { | |||
| super(props); | |||
| this.state = { expanded: {} }; | |||
| this.state = { expanded: {}, selectedPathNode: undefined }; | |||
| this.handleNavigationEvent = this.handleNavigationEvent.bind(this); | |||
There was a problem hiding this comment.
I almost prefer to have the .bind(this)s at the callsite, or else give the property the .bind()ed method a different name from the original method name. Do you have a strong opinion here? I haven't seen this pattern before but am willing to defer if you feel like it's idiomatic.
There was a problem hiding this comment.
I'd say it's quite common. It's recommended on reactjs.org, with the other alternative being to make the method into a field instead.
We generally recommend binding in the constructor or using the class fields syntax, to avoid this sort of performance problem.
jcreedcmu
approved these changes
Nov 25, 2019
Merged
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds the command
CodeQL: Show Next Step on PathandCodeQL: Show Previous Step on Pathfor navigating the steps on the currently shown path.The idea is that you can add keybindings for these commands to navigate the path without the mouse. Doing this with the mouse required quite a bit of zig-zag eye tracking between the code view and result view.
You still have to click one of the steps on a path to "choose" that path. There are currently no commands for switching between results or paths. It would be nice to have a fully keyboard-controllable result view, but I think I'll leave that for a future PR.
It's a bit of a hassle to make the result view remember which path node was previously clicked. For this we need a way to reference a specific part of the result set, that is, a result index, path index, and a path step index. I introduced the types
Keys.Result,Keys.Path, andKeys.PathNodeto represent such indices.Commit-by-commit review recommended.