Skip to content

Data Insights: fetch total_data_assets once on the Data Assets tab - #36452

Merged
ShaileshParmar11 merged 10 commits into
mainfrom
fix/data-insights-total-assets-once
Oct 29, 2026
Merged

ShaileshParmar11 merged 10 commits into
mainfrom
fix/data-insights-total-assets-once

Conversation

@ShaileshParmar11

@ShaileshParmar11 ShaileshParmar11 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Describe your changes:

No linked issue; this fixes a performance problem Sentry reported in production (COLLATE-FRONTEND-2A, "N+1 API Call" on /data-insights/data-assets). Labelled skip-pr-checks.

On the Data Insights → Data Assets tab, every chart card fetched the total_data_assets series on its own, with identical start, end and filter. The tab sent that request 5 times per load and 5 more on every filter change. Each card also waited for it before fetching its own chart, one request after another.

  • DataAssetsTab fetches total_data_assets once per filter change and passes it to the cards as totalAssets. A response that a newer filter has superseded is ignored.
  • The Total Data Assets card draws that shared series and makes no request of its own.
  • The four percentage cards fetch their own chart and their absolute-values chart in parallel (Promise.all). They stay in the loading state until the shared totals arrive, so they never show a 0% change computed from an empty series. If the totals request fails, they show the empty state, as they did before when the request inside each card failed.
  • The changeInValue memo listed only chartData.results as a dependency. It also reads totalAssets and absoluteValues, which now arrive separately, so they are added.
  • DataInsightProvider and its interface move from pages/DataInsightPage/ to components/DataInsight/DataInsightProvider/, so the Data Insight components no longer import a page (no-lower-layer-page-imports). openmetadata-collate imports the provider from the old path in DashboardChart.tsx and DashboardDetails.tsx, so pages/DataInsightPage/DataInsightProvider.tsx stays as a re-export. The provider's code is unchanged.
  • .github/playwright/impact-map.json maps components/DataInsight/** and pages/DataInsightPage/** to DataInsight.spec.ts; these files were unmapped and would have run the full suite.

Requests on load go from 15 (largely sequential) to 11, with no duplicates. The tier card is unchanged.

Type of change:

  • Bug fix

High-level design:

N/A — small change.

Tests:

Use cases covered

  • Data Assets tab requests total_data_assets once and hands the same object to the five cards that read it.
  • The tab hands the cards null when the totals request fails, and does not fetch while the KPIs are loading.
  • The Total Data Assets card renders from the shared series without a request.
  • A percentage card sends its own chart and absolute-values requests together, and never requests total_data_assets.
  • A percentage card stays loading until the shared totals arrive, and shows the empty state (no change value) when they failed.

Unit tests

  • I added unit tests for the new/changed logic.
  • Files added/updated:
    • src/components/DataInsight/DataAssetsTab/DataAssetsTab.test.tsx (new, 3 tests)
    • src/components/DataInsight/DataInsightChartCard.test.tsx (4 new tests)
  • yarn test src/components/DataInsight: 10 suites, 38 tests passed.
  • Coverage: DataAssetsTab.component.tsx 100% lines; DataInsightChartCard.tsx 85% lines. The uncovered lines are pre-existing (computeTotalDataAssetsByTierChange, the targetValue/totalValue switch, the legend mouse handlers).
  • All 6 new tests fail when run against the previous code (3 against main, 3 against the first commit of this PR).
  • tsc --noEmit: no errors in the changed files. ESLint: 0 errors. Prettier: clean.

Backend integration tests

  • Not applicable (no backend API changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • No new spec: there is no new UI behaviour to drive. The existing DataInsight.spec.ts covers the tab rendering its graphs.
  • Ran playwright/e2e/Pages/DataInsight.spec.ts (project Data Insight) locally on b8b58e2b97b, against the Vite dev server for this branch and a local OSS backend (PLAYWRIGHT_IS_OSS=true): 14 passed (setup, entity data, Data Insight app run, 10 @data-insight tests, teardown) in 1.5m.
  • Not run: the 21 canary and smoke specs make playwright_affected also lists. None of them touches Data Insight.

Manual testing performed

On the same local stack, after the Data Insight app had run (80 data assets):

  1. Signed in as admin and opened Insights → Data Assets.
  2. All cards rendered: Total Data Assets 80, Data Assets with Description 47.5%, the KPI section and the Total Data Assets chart with its per-type breakdown.
  3. Recorded the chart requests on page load. Per mount: total_data_assets 1 (was 5), 10 system/charts/name/*/data requests plus 1 listChartData. The dev build renders under React.StrictMode, so every request appears twice there; a production build mounts once.

UI screen recording:

Not applicable — performance bug fix with no visual change.

Checklist:

  • I have read the CONTRIBUTING document.

  • My PR title is Fixes <issue-number>: <short explanation> — no issue; Sentry-reported fix, labelled skip-pr-checks.

  • My PR is linked to a GitHub issue via Fixes #<issue-number> above — see above.

  • I have commented on my code, particularly in hard-to-understand areas.

  • For JSON Schema changes: not applicable.

  • For a UI feature or feature task: not applicable (bug fix).

  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

  • Self-review: I reviewed the final diff against REVIEW.md (the pr-checklist skill runs it) and fixed every 🔴 finding, or answered it on the line below.

    • Reviewed 8adda704e30; test-only commit add32800eb2 follows. The first pass found one 🔴: percentage cards showed a 0% change while the totals were loading or after they failed. Fixed in 8adda704e30, with tests. The second pass found no 🔴.
    • Left as follow-ups (🟡): the card's own fetch has no stale-request guard (pre-existing); totalAssets is optional rather than required per card type (one caller today); no test asserts the computed changeInValue value.
  • Decision check: no durable decision; nothing to record.

  • I have added a test that covers the exact scenario we are fixing.

🤖 Generated with Claude Code

Each chart card fetched the total_data_assets series on its own, so the tab
sent the same request five times per load and per filter change, and each
card waited on it before its own chart. The tab now fetches it once and
passes it to the cards; each card fetches its own chart and absolute values
in parallel.
A percentage card computed its change from an empty totals series while the
shared total_data_assets request was in flight, or after it failed, and showed
0%. It now waits for the totals and shows the empty state if they failed.
@ShaileshParmar11 ShaileshParmar11 added the skip-pr-checks Bypass PR metadata validation check label Oct 28, 2026
@ShaileshParmar11
ShaileshParmar11 requested a review from a team as a code owner October 28, 2026 12:24
@ShaileshParmar11
ShaileshParmar11 requested review from karthicksharan and a balanced review from Copilot October 28, 2026 12:24

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@github-actions github-actions Bot added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Oct 28, 2026
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 80%
79.96% (131645/164628) 66.45% (46448/69890) 69.5% (26380/37952)

Copilot AI balanced review requested due to automatic review settings October 29, 2026 06:36

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@ShaileshParmar11 ShaileshParmar11 added the To release Will cherry-pick this PR into the release branch label Oct 29, 2026
Copilot AI balanced review requested due to automatic review settings October 29, 2026 08:47

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 6 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 6 warning(s) across 2 changed file(s).

Count Rule
5 react-hooks/exhaustive-deps
1 openmetadata-imports/no-api-calls-in-iteration
All findings
Location Rule Message
🟡 src/components/DataInsight/DataInsightProvider/DataInsightProvider.tsx:284:5 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'selectedDaysFilter' and 'tier'. Either include them or remove the dependency array.
🟡 src/components/DataInsight/DataInsightProvider/DataInsightProvider.tsx:307:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'fetchDefaultTeamOptions'. Either include it or remove the dependency array.
🟡 src/components/DataInsight/DataInsightSummary.tsx:72:5 react-hooks/exhaustive-deps React Hook useMemo has an unnecessary dependency: 'chartFilter'. Either exclude it or remove the dependency array.
🟡 src/components/DataInsight/DataInsightSummary.tsx:77:5 react-hooks/exhaustive-deps React Hook useMemo has an unnecessary dependency: 'chartFilter'. Either exclude it or remove the dependency array.
🟡 src/components/DataInsight/DataInsightSummary.tsx:148:16 openmetadata-imports/no-api-calls-in-iteration Avoid issuing one API request per item. Fetch at the data owner, use a bulk endpoint, or use useQueries with an intentional concurrency policy.
🟡 src/components/DataInsight/DataInsightSummary.tsx:179:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'fetchEntitiesChartData', 'fetchMostActiveUser', and 'fetchWebChartData'. Either include them or remove the depen

Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

Copilot AI balanced review requested due to automatic review settings October 29, 2026 09:24

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

…nts stop importing a page

openmetadata-collate still imports it from pages/DataInsightPage/DataInsightProvider,
so that path stays as a re-export.
Copilot AI balanced review requested due to automatic review settings October 29, 2026 09:34

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot AI balanced review requested due to automatic review settings October 29, 2026 09:47

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@gitar-bot

gitar-bot Bot commented Oct 29, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

🟡 Medium risk · Sharing filtered totals across Data Assets cards changes chart loading and calculations; stale or failed fetch handling could show incorrect or empty insights.

Fetches total_data_assets once per filter change in DataAssetsTab and passes it to the chart cards, cutting duplicate requests on the Data Assets tab from 15 to 11. The percentage cards stay loading until the shared totals arrive, and the changeInValue memo now includes its full dependencies. No issues found.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Not enabled

Set up

For more verification details, comment gitar display:verbose.

Options

Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@sonarqubecloud

Copy link
Copy Markdown

@ShaileshParmar11
ShaileshParmar11 added this pull request to the merge queue Oct 29, 2026
Merged via the queue into main with commit 7d46ff4 Oct 29, 2026
72 checks passed
@ShaileshParmar11
ShaileshParmar11 deleted the fix/data-insights-total-assets-once branch October 29, 2026 15:33
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Backport to 2.0 is under way. It is built from openmetadata-collate, which holds the credentials needed to open a PR here, and it will comment again with the result — nothing is needed from you unless it reports otherwise.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Backport to 2.1 is under way. It is built from openmetadata-collate, which holds the credentials needed to open a PR here, and it will comment again with the result — nothing is needed from you unless it reports otherwise.

@gh-bot-collate

Copy link
Copy Markdown

🤖 Backport to 2.0 failed. The conflict could not be resolved, so this change is not on that branch. Resolver run.

@gh-bot-collate

Copy link
Copy Markdown

🤖 Backport to 2.1 opened as #36571 (clean cherry-pick). It merges itself once the checks pass.

ShaileshParmar11 added a commit that referenced this pull request Oct 30, 2026
…36452) (#36571)

* ui: fetch total_data_assets once on the Data Insights Data Assets tab

Each chart card fetched the total_data_assets series on its own, so the tab
sent the same request five times per load and per filter change, and each
card waited on it before its own chart. The tab now fetches it once and
passes it to the cards; each card fetches its own chart and absolute values
in parallel.

* ui: keep percentage cards loading until the shared totals arrive

A percentage card computed its change from an empty totals series while the
shared total_data_assets request was in flight, or after it failed, and showed
0%. It now waits for the totals and shows the empty state if they failed.

* ci: map the Data Insight components to DataInsight.spec.ts

* ui: assert the empty and loading states in the chart card tests

* ui: list the chart card fetch effect's dependencies in full

* ui: fetch the shared totals in an async function with try/catch

* ui: mark the fire-and-forget fetches in the Data Insight effects as void

* ui: drive the DataAssetsTab test's provider mock without importing the page

* ui: move DataInsightProvider out of pages so the Data Insight components stop importing a page

openmetadata-collate still imports it from pages/DataInsightPage/DataInsightProvider,
so that path stays as a re-export.

* ui: sort the moved Data Insight provider's imports

(cherry picked from commit 7d46ff4)

Co-authored-by: Shailesh Parmar <shailesh.parmar.webdev@gmail.com>
ShaileshParmar11 added a commit that referenced this pull request Oct 30, 2026
…36452) (#36651)

* ui: fetch total_data_assets once on the Data Insights Data Assets tab

Each chart card fetched the total_data_assets series on its own, so the tab
sent the same request five times per load and per filter change, and each
card waited on it before its own chart. The tab now fetches it once and
passes it to the cards; each card fetches its own chart and absolute values
in parallel.

* ui: keep percentage cards loading until the shared totals arrive

A percentage card computed its change from an empty totals series while the
shared total_data_assets request was in flight, or after it failed, and showed
0%. It now waits for the totals and shows the empty state if they failed.

* ci: map the Data Insight components to DataInsight.spec.ts

* ui: assert the empty and loading states in the chart card tests

* ui: list the chart card fetch effect's dependencies in full

* ui: fetch the shared totals in an async function with try/catch

* ui: mark the fire-and-forget fetches in the Data Insight effects as void

* ui: drive the DataAssetsTab test's provider mock without importing the page

* ui: move DataInsightProvider out of pages so the Data Insight components stop importing a page

openmetadata-collate still imports it from pages/DataInsightPage/DataInsightProvider,
so that path stays as a re-export.

* ui: sort the moved Data Insight provider's imports

(cherry picked from commit 7d46ff4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants