Skip to content

Extract code snippet into stand alone component#1181

Merged
charisk merged 2 commits intomainfrom
charisk/code-snippet-component
Mar 4, 2022
Merged

Extract code snippet into stand alone component#1181
charisk merged 2 commits intomainfrom
charisk/code-snippet-component

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Mar 3, 2022

Extracts components and logic that help us render code snippets out of AnalysisAlertResult and into a new stand-alone component so that it can be re-used when showing code paths for problem path queries.

The code is mostly a copy of what has been checked in but with a few adjustments:

  • Improved styling
  • Improved highlighting for multi-line highlights
  • It no longer receives an alert model
  • The "message" part is been made optional, with the ability to receive more children (this is where we'll plug in the code paths link for problem path queries).

Note that AnalysisAlertResult is really quite thin now and could be incorporated back into RemoteQueries.tsx but I think it'll get a bit more meat to it once we do code paths.

Checklist

N/A:

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

@charisk charisk requested a review from a team as a code owner March 3, 2022 16:00
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Approving, assuming you will fix the typo. :)

My comment about the correctness of the locations can be done in the future.

const isFirstHighlightedLine = lineNumber === highlightedRegion.startLine;
const isLastHighlightedLine = lineNumber === highlightedRegion.endLine;

const highlightStartColumn = isSingleLineHighlight
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Going back to our conversation on the previous PR about handling invalid source locs, there are a number of common errors that I think you can handle right here:

if (highlightStartColumn > highilightEndColumn) {
  ([highlightStartColumn, highilightEndColumn] = [highilightEndColumn, highlightStartColumn]);
}

highlightStartColumn = Math.max(highlightStartColumn, 0);
highilightEndColumn = Math.max(highlightStartColumn, highilightEndColumn);

Or some variant of this. It looks like this code won't cause an error if the locations are invalid, but they might cause missing highlights. Again, this is pretty rare, but it does happen.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the example/code. Lets discuss validation next week!

? highlightedRegion.startColumn
: 0;

const highilightEndColumn = isSingleLineHighlight
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: highilightEndColumn -> highlightEndColumn

@charisk charisk enabled auto-merge (squash) March 4, 2022 07:55
@charisk charisk merged commit b510b85 into main Mar 4, 2022
@charisk charisk deleted the charisk/code-snippet-component branch March 4, 2022 08:06
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.

2 participants