-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix(ui): pass createdBy to highlight query column #14042
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
Conversation
✅ Meticulous spotted 0 visual differences across 1589 screens tested: view results. Meticulous evaluated ~8 hours of user flows against your PR. Expected differences? Click here. Last updated for commit d84591e. This comment will update as new commits are pushed. |
Bundle ReportChanges will increase total bundle size by 323 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. ❌ Your patch status has failed because the patch coverage (53.84%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one suggestion on how we can avoid type casting and risking white screens of death! this is a tactic i'm going to push for going forward
@@ -17,6 +17,7 @@ export function mapQuery({ queryEntity, entityUrn, siblingUrn, poweredEntity }: | |||
description: queryEntity.properties?.description || undefined, | |||
query: queryEntity.properties?.statement?.value || '', | |||
createdTime: queryEntity?.properties?.created?.time, | |||
createdBy: queryEntity?.properties?.createdOn?.actor as CorpUser, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we're trying to avoid casting types as much as possible going forward because that's what gets us into situations where we get white screens of death.
What i think we can do here that is much safer is instead of having createdBy
being a CorpUser
is map createdBy
into the correct type for what we're fetching for exactly. here's what i mean:
inside of the actor
field in query.graphql
we can request for a new fragment called ActorWithDisplayName
that's simply:
urn
type
...entityDisplayNameFields
and since that's what we request for, we can set createdBy?: ActorWithDisplayNameFragment
(this type gets autogenerated like other types) in ...Queries/types.ts
so then we don't actually have to do any type casting and it is typed accurately to what is being fetched.
i find creating fragments with helpful names we can use as types in the app to be much nicer and obviously safer than type casting.
const photoUrl = | ||
createdBy?.__typename === 'CorpUser' | ||
? (createdBy as any)?.editableProperties?.pictureLink || | ||
(createdBy as any)?.editableInfo?.pictureLink || | ||
undefined | ||
: undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we querying for these fields in entityDisplayNameFields
? otherwise I don't think we'll ever render them hence you having to cast as any
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chriscollins3456 nice catch - would something like this work? i don't want to use corpUser if possible, wondering if there's a better approach
adding fields in query.graphql
fragment ActorWithDisplayName on Entity {
urn
type
...entityDisplayNameFields
... on CorpUser {
editableProperties {
pictureLink
}
editableInfo {
pictureLink
}
}
}
and updating queryColumns.tsx
const photoUrl =
createdBy.__typename === 'CorpUser'
? createdBy?.editableProperties?.pictureLink || createdBy?.editableInfo?.pictureLink || undefined
: undefined;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think that would work! however we could make it even cleaner since we know actor
is a CorpUser
in ResolvedAuditStamp
- we could do fragment ActorWithDisplayName on CorpUser
here and then when we use ActorWithDisplayNameFragment
in the UI i don't think we'll have to have that __typename
check because it should be properly typed and less ambiguous. let me know if that works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
Screen.Recording.2025-07-11.at.3.19.08.PM.mov
picking up changes from this PR partially : https://github.com/acryldata/datahub-fork/commit/dfd617bcafa6aa9b2d687f6650587e65a1fc007d?diff=unified#diff-e04c9c2d80cbfd7aa7e3e0f867248464db0f6497684661132d6ead81ded21856