Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt risk summary to both entity flyouts #5

Merged

Conversation

tiansivive
Copy link

Adapted Risk Summary to both flyouts

@tiansivive tiansivive marked this pull request as ready for review January 1, 2024 18:35
}, [userRiskData]);
const entityData = (() => {
if (!riskData) return;
if (entity === RiskScoreEntity.user) return (riskData as UserRiskScore).user;
Copy link
Owner

@machadoum machadoum Jan 2, 2024

Choose a reason for hiding this comment

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

NIT
You could create a User-Defined Type Guard like here:

function isUserEntity(entity: UserRiskScore | HostRiskScore): entity is UserRiskScore {
  return (entity as UserRiskScore).user !== undefined;
}

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I wasn't too happy about this
I can extract to a function with a type predicate rather than the IIFE, but it's still gonna be brittle 🤔
And its worse here because it's not just one value, the entityData type depends on the entity prop type, so our type predicate would depend on other, additional types on top of the type it is narrowing.

I'm not sure if there's a better solution.

I actually originally just copied and duplicated the component before making it generic, but I also don't think that's acceptable 😅

@tiansivive tiansivive force-pushed the siem-ea-8084-risk-summary branch from 7ce37ee to 9b10b2e Compare January 2, 2024 10:19
@machadoum machadoum merged commit 034cca0 into machadoum:siem-ea-8084 Jan 2, 2024
1 check passed
machadoum pushed a commit that referenced this pull request Oct 15, 2024
Another one bites the dust
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