Skip to content

chore(lint): convert ChartDataProvider and StatefulChart to function components#39456

Open
rusackas wants to merge 3 commits intochore/lint-cleanup-function-componentsfrom
chore/fc-05-chart-data-provider
Open

chore(lint): convert ChartDataProvider and StatefulChart to function components#39456
rusackas wants to merge 3 commits intochore/lint-cleanup-function-componentsfrom
chore/fc-05-chart-data-provider

Conversation

@rusackas
Copy link
Copy Markdown
Member

Summary

  • Converts ChartDataProvider and StatefulChart in superset-ui-core from class components to function components

Part of the broader class→function component lint cleanup tracked in #37902.

Test plan

  • CI passes on the target feature branch

Additional information

This is sub-PR 5 of ~10, each targeting chore/lint-cleanup-function-components.

🤖 Generated with Claude Code

…components

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review bot commented Apr 17, 2026

Bito Automatic Review Skipped - Branch Excluded

Bito didn't auto-review because the source or target branch is excluded from automatic reviews.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change the branch exclusion settings here, or contact your Bito workspace admin at evan@preset.io.

@dosubot dosubot bot added change:frontend Requires changing the frontend packages labels Apr 17, 2026
Comment on lines +154 to +155
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [formData, sliceId]);
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.

Suggestion: The effect only depends on formData and sliceId, so changes to other fetch-driving props (client, loadDatasource, and request option props) do not trigger a refetch and the component can render stale or incomplete data. Make the effect depend on handleFetchData so it reruns whenever any input used by the fetch logic changes. [logic error]

Severity Level: Major ⚠️
- ❌ loadDatasource toggle ignored; datasource metadata never fetched afterwards.
- ⚠️ Updated request options ignored until IDs also change.
Suggested change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [formData, sliceId]);
}, [handleFetchData]);
Steps of Reproduction ✅
1. In
`superset-frontend/packages/superset-ui-core/test/chart/components/ChartDataProvider.test.tsx:69-82`,
use the existing `setup` helper to render `<ChartDataProvider {...props}
loadDatasource={false} />`, which mounts `ChartDataProvider` from
`src/chart/components/ChartDataProvider.tsx`.

2. After the initial load completes (where `handleFetchData` at
`ChartDataProvider.tsx:109-146` has run once and `mockLoadDatasource` has not been called
because `loadDatasource` was `false`), call `rerender` with the same `formData`/`sliceId`
but `loadDatasource` flipped to `true`, e.g. `rerender(<ChartDataProvider {...props}
loadDatasource />);` inside the same test file.

3. With the current implementation of the effect at `ChartDataProvider.tsx:148-155`
(`useEffect(() => { handleFetchData(); }, [formData, sliceId]);`), React compares the
dependency array, sees that `formData` and `sliceId` are unchanged, and does not re-run
the effect, so `handleFetchData` is not invoked a second time.

4. Because `handleFetchData` is the only place that calls
`chartClient.loadDatasource(...)`, the mocked `mockLoadDatasource` from
`ChartDataProvider.test.tsx:46-56` remains at zero calls even after enabling
`loadDatasource`, and the children never receive a payload with `datasource` populated;
the same behavior occurs if only `formDataRequestOptions`, `datasourceRequestOptions`, or
`queryRequestOptions` change while `formData`/`sliceId` stay the same, so updated fetch
configuration is ignored until IDs change.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx
**Line:** 154:155
**Comment:**
	*Logic Error: The effect only depends on `formData` and `sliceId`, so changes to other fetch-driving props (`client`, `loadDatasource`, and request option props) do not trigger a refetch and the component can render stale or incomplete data. Make the effect depend on `handleFetchData` so it reruns whenever any input used by the fetch logic changes.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

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.

Pre-existing behavior — the effect dependency list matches the componentDidUpdate semantics from the original class component. Not introduced by this PR.

@netlify
Copy link
Copy Markdown

netlify bot commented Apr 17, 2026

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit a5f0c31
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/69e26f3cafde260008c823be
😎 Deploy Preview https://deploy-preview-39456--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

rusackas and others added 2 commits April 17, 2026 10:56
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.45%. Comparing base (5106afb) to head (d820f16).

Additional details and impacted files
@@                            Coverage Diff                             @@
##           chore/lint-cleanup-function-components   #39456      +/-   ##
==========================================================================
- Coverage                                   64.45%   64.45%   -0.01%     
==========================================================================
  Files                                        2541     2541              
  Lines                                      131662   131654       -8     
  Branches                                    30517    30516       -1     
==========================================================================
- Hits                                        84863    84855       -8     
  Misses                                      45333    45333              
  Partials                                     1466     1466              
Flag Coverage Δ
javascript 66.05% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend packages size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant