Skip to content

Add analyzed repositories component#1544

Merged
koesie10 merged 1 commit intomainfrom
koesie10/scanned-repos-tab
Sep 29, 2022
Merged

Add analyzed repositories component#1544
koesie10 merged 1 commit intomainfrom
koesie10/scanned-repos-tab

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Sep 27, 2022

This adds the analyzed repositories component for showing within the "Analyzed" tab. I wasn't completely sure whether there should be a difference between "Pending" and "In progress", but pending will now not show an icon, while in progress will show a spinner.

For the collapsible items, it does not reuse the CollapsibleItem component because that component is tightly coupled with the styles of the remote queries component.

Screenshot 2022-09-27 at 15 03 45

Screenshot 2022-09-27 at 15 03 57

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.

This adds the analyzed repositories component for showing within the
"Analyzed" tab. I wasn't completely sure whether there should be a
difference between "Pending" and "In progress", but pending will now not
show an icon, while in progress will show a spinner.

For the collapsible items, it does not reuse the `CollapsibleItem`
component because that component is tightly coupled with the styles
of the remote queries component.
Base automatically changed from koesie10/outcome-panel to main September 27, 2022 13:20
@koesie10 koesie10 marked this pull request as ready for review September 27, 2022 13:21
@koesie10 koesie10 requested review from a team as code owners September 27, 2022 13:21
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Looks really nice 👍

I did also wonder about whether some of the warnings and other boxes should always extend to be full width. I notice in the screenshots they're a bit short and I think having things be different widths doesn't look great. But when I look at it in storybook everything is a consistent width, so I guess that changed since the screenshots were taken?

Comment on lines +82 to +87
export interface VariantAnalysisScannedRepositoryResult {
repositoryId: number;
interpretedResults?: AnalysisAlert[];
rawResults?: AnalysisRawResults;
}

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.

Very take-it-or-leave-it suggestion, but instead of having two optional fields, could we encode in the type system that we expect precisely one of them to be defined? Maybe something like the following:

Suggested change
export interface VariantAnalysisScannedRepositoryResult {
repositoryId: number;
interpretedResults?: AnalysisAlert[];
rawResults?: AnalysisRawResults;
}
export type VariantAnalysisScannedRepositoryResult = { repositoryId: number } & (VariantAnalysisInterpretedScannedRepositoryResult | VariantAnalysisRawScannedRepositoryResult);
export interface VariantAnalysisInterpretedScannedRepositoryResult {
resultsType: 'interpreted';
interpretedResults: AnalysisAlert[];
}
export interface VariantAnalysisRawScannedRepositoryResult {
resultsType: 'raw';
rawResults: AnalysisRawResults;
}

Then later on you can do things like if (results?.resultsType === 'interpreted') { // use results.interpretedResults for something }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, that would make it more explicit. I don't think we even need the resultsType to make it explicit that only one of the two is defined.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On second thought, now that I try implementing it, I think it might make it harder to pass down props. For example, it would make this much harder:

interpretedResults={results?.interpretedResults}
rawResults={results?.rawResults}

I would ideally like to keep those components independent from the exact structure of the VariantAnalysisScannedRepositoryResult, so I think I'll leave it like this for now.

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.

When I tried it out I changed those props to instead take a VariantAnalysisScannedRepositoryResult, but I agree perhaps that's making them a bit too tied to that type. So keeping things as they are is perfectly fine 👍

}
`;

const Visibility = styled.span`
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.

I also introduce one of these in #1549. For these two PRs I wouldn't worry about it but once both are done I suggest we make a shared component for this.

@koesie10 koesie10 merged commit 177688d into main Sep 29, 2022
@koesie10 koesie10 deleted the koesie10/scanned-repos-tab branch September 29, 2022 14:05
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