Skip to content

feat(ingest): change asset list #14073

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 35 commits into from
Jul 22, 2025
Merged

feat(ingest): change asset list #14073

merged 35 commits into from
Jul 22, 2025

Conversation

anshbansal
Copy link
Collaborator

@anshbansal anshbansal commented Jul 14, 2025 •
edited
Loading

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Jul 14, 2025
@alwaysmeticulous alwaysmeticulous
Copy link

alwaysmeticulous bot commented Jul 14, 2025 •
edited
Loading

🔴 Meticulous spotted visual differences in 195 of 1548 screens tested: view and approve differences detected.

Meticulous evaluated ~9 hours of user flows against your PR.

Last updated for commit def23d7. This comment will update as new commits are pushed.

@codecov Codecov
Copy link

codecov bot commented Jul 14, 2025 •
edited
Loading

Bundle Report

Changes will increase total bundle size by 6.49kB (0.03%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
datahub-react-web-esm 22.24MB 6.49kB (0.03%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: datahub-react-web-esm

Assets Changed:

Asset Name Size Change Total Size Change (%)
assets/index-*.js 6.49kB 18.6MB 0.03%

Files in assets/index-*.js:

  • ./src/app/ingestV2/executions/components/ExecutionDetailsModal.tsx → Total Size: 2.47kB

  • ./src/app/ingestV2/source/IngestedAssets.tsx → Total Size: 8.66kB

  • ./src/app/ingestV2/executions/components/reporting/StructuredReportItemList.tsx → Total Size: 839 bytes

  • ./src/app/ingestV2/source/utils.ts → Total Size: 12.57kB

  • ./src/alchemy-components/components/Modal/Modal.tsx → Total Size: 2.52kB

  • ./src/app/ingestV2/executions/components/BaseTab.tsx → Total Size: 200 bytes

  • ./src/app/ingestV2/executions/components/LogsTab.tsx → Total Size: 1.3kB

  • ./src/app/ingestV2/executions/types.ts → Total Size: 185 bytes

  • ./src/app/ingestV2/ManageIngestionPage.tsx → Total Size: 6.08kB

  • ./src/app/ingestV2/executions/components/RecipeTab.tsx → Total Size: 1.2kB

  • ./src/app/ingestV2/executions/components/reporting/StructuredReportItem.tsx → Total Size: 1.67kB

  • ./src/alchemy-components/components/Tabs/Tabs.tsx → Total Size: 4.92kB

  • ./src/app/ingestV2/executions/components/SummaryTab.tsx → Total Size: 5.69kB

  • ./src/app/ingestV2/executions/components/reporting/StructuredReport.tsx → Total Size: 1.57kB

v-tarasevich-blitz-brain
v-tarasevich-blitz-brain reviewed Jul 14, 2025
View reviewed changes
@anshbansal anshbansal force-pushed the ab-2025-jul-14-edit-ingest-assets-page branch from 9580144 to f1975fa Compare July 14, 2025 14:24
@codecov Codecov
Copy link

codecov bot commented Jul 14, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 48.39286% with 289 lines in your changes missing coverage. Please review.

❌ Unsupported file format

Upload processing failed due to unsupported file format. Please review the parser error message: 
Error parsing JUnit XML in /home/runner/work/datahub/datahub/metadata-io/build/test-results/test/TEST-com.linkedin.metadata.graph.search.opensearch.SearchGraphServiceOpenSearchTest.xml at 683:1051

Caused by:

RuntimeError: Error converting computed name to ValidatedString


Caused by:
    string is too long</code></pre>

For more help, visit our troubleshooting guide.

Files with missing lines Patch % Lines
.../app/ingestV2/executions/components/SummaryTab.tsx 36.79% 67 Missing ⚠️
...lchemy-components/components/Tabs/Tabs.stories.tsx 0.00% 63 Missing ⚠️
...b-react/src/app/ingestV2/source/IngestedAssets.tsx 48.95% 49 Missing ⚠️
...V2/executions/components/ExecutionDetailsModal.tsx 16.07% 47 Missing ⚠️
...c/app/ingestV2/executions/components/RecipeTab.tsx 48.27% 15 Missing ⚠️
...src/app/ingestV2/executions/components/LogsTab.tsx 48.14% 14 Missing ⚠️
...ct/src/alchemy-components/components/Tabs/Tabs.tsx 76.59% 11 Missing ⚠️
...ecutions/components/reporting/StructuredReport.tsx 9.09% 10 Missing ⚠️
...web-react/src/app/ingestV2/ManageIngestionPage.tsx 10.00% 9 Missing ⚠️
datahub-web-react/src/app/ingestV2/source/utils.ts 97.93% 2 Missing ⚠️
... and 2 more

:x: Your patch status has failed because the patch coverage (48.39%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

:loudspeaker: Thoughts on this report? Let us know!

gabe-lyons
gabe-lyons reviewed Jul 14, 2025
View reviewed changes
datahub-web-react/src/alchemy-components/components/Tabs/Tabs.tsx Outdated
Comment on lines 195 to 204
<div
ref={tabsContainerRef}
style={{
maxHeight,
overflowY: 'auto' as const,
height: '100%',
position: 'relative',
}}
>
{tabsContent}
Copy link
Contributor

Choose a reason for hiding this comment

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

my initial reaction is that adding in refs and scrollTo actions into the Tabs component is not ideal. Generally using refs and scrollTo is a break-glass last approach. If anything, I would have the alchemy component export its ref and do the scrollTo outside unless we're sure this is something we want to bake into all tabs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I take this out then I have to break the abstraction of Tabs as finding the correct element to scroll from inside the component was proving to be quite difficult. I did that earlier but then as it was breaking abstraction I opted to do this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Imo this is ok to do in the component, especially since it's flagged

gabe-lyons
gabe-lyons reviewed Jul 14, 2025
View reviewed changes
gabe-lyons
gabe-lyons reviewed Jul 14, 2025
View reviewed changes
@datahub-cyborg datahub-cyborg bot added the pending-submitter-response Issue/request has been reviewed but requires a response from the submitter label Jul 16, 2025
@datahub-cyborg datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Jul 16, 2025
@anshbansal
Copy link
Collaborator Author

anshbansal commented Jul 16, 2025 •
edited
Loading

@gabe-lyons it will look like this in case of more than few types
image

Loom showing the changes https://www.loom.com/share/ed028af28e6a40608fe6edf7348545bd

@datahub-cyborg datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Jul 16, 2025
asikowitz
asikowitz reviewed Jul 20, 2025
View reviewed changes
Copy link
Collaborator

@asikowitz asikowitz left a comment

Choose a reason for hiding this comment

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

Really nice job on your first frontend PR (I think? At least when formally on a product team)! I have one request around handling type unsafety that I'd like to discuss before approval, and a bunch of non-blocking style nits

datahub-web-react/src/alchemy-components/components/Tabs/Tabs.tsx Outdated
Comment on lines 195 to 204
<div
ref={tabsContainerRef}
style={{
maxHeight,
overflowY: 'auto' as const,
height: '100%',
position: 'relative',
}}
>
{tabsContent}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Imo this is ok to do in the component, especially since it's flagged

datahub-web-react/src/app/ingestV2/executions/components/RecipeTab.tsx Outdated
`;

const RecipeSection = styled.div`
border-top: 1px solid ${ANTD_GRAY[4]};
Copy link
Collaborator

Choose a reason for hiding this comment

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

ant colors are deprecated, please use one in foundations/colors.ts if possible.

datahub-web-react/src/app/ingestV2/executions/components/LogsTab.tsx Outdated

import { GetIngestionExecutionRequestQuery } from '@graphql/ingestion.generated';

const SectionHeader = styled(Typography.Title)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is reused across each tab and we likely want to keep them consistent across tabs. Perhaps we can consolidate these in a components.tsx or similar. @chriscollins3456 do we have a convention here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to a common file

@datahub-cyborg datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Jul 20, 2025
asikowitz
asikowitz approved these changes Jul 21, 2025
View reviewed changes
Copy link
Collaborator

@asikowitz asikowitz left a comment

Choose a reason for hiding this comment

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

I'd still prefer to wrap getIngestionContents and getOtherIngestionContents in a try / catch, but approving to unblock. Nice job!

@datahub-cyborg datahub-cyborg bot added pending-submitter-merge and removed needs-review Label for PRs that need review from a maintainer. labels Jul 21, 2025
gabe-lyons
gabe-lyons reviewed Jul 22, 2025
View reviewed changes
datahub-web-react/src/app/ingestV2/source/utils.ts
if (!structuredReportObject) {
return null;
}
const aspectsBySubtypes = structuredReportObject.source.report.aspects_by_subtypes;
Copy link
Contributor

Choose a reason for hiding this comment

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

could this ever throw an error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#14167

gabe-lyons
gabe-lyons reviewed Jul 22, 2025
View reviewed changes
datahub-web-react/src/app/ingestV2/executions/components/SummaryTab.tsx
Comment on lines +57 to +59
const RecipeSection = styled(SectionBase)``;

const LogsSection = styled(SectionBase)``;
Copy link
Contributor

Choose a reason for hiding this comment

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

why create an empty styled component like this?

Copy link
Collaborator Author

@anshbansal anshbansal Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was refactoring, got feedback, cleanup, repeat and this happened. Taken care of in #14167

@anshbansal anshbansal merged commit e539e33 into master Jul 22, 2025
36 of 38 checks passed
@anshbansal anshbansal deleted the ab-2025-jul-14-edit-ingest-assets-page branch July 22, 2025 09:11
kartikey-visa pushed a commit to kartikey-visa/datahub that referenced this pull request Jul 23, 2025
@anshbansal @kartikey-visa
feat(ingest): change asset list (datahub-project#14073)
060057d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabe-lyons gabe-lyons gabe-lyons left review comments

@v-tarasevich-blitz-brain v-tarasevich-blitz-brain v-tarasevich-blitz-brain left review comments

@asikowitz asikowitz asikowitz approved these changes

Assignees

No one assigned

Labels

pending-submitter-merge product PR or Issue related to the DataHub UI/UX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anshbansal @gabe-lyons @asikowitz @v-tarasevich-blitz-brain