chore: Upgrade AG Grid to use tree shaking #32334

kgabryje · 2025-02-20T18:33:20Z

SUMMARY

Upgrade AG Grid to version 33. This version improved modules architecture, which lets us specify exactly which parts of the library we use and tree-shake the rest, thus improving the bundle size.
Additionally, I wrapped some props passed to AG Grid in useMemo to avoid rerenders

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No visual changes

TESTING INSTRUCTIONS

The AG Grid table in SQL Lab should work like before

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

kgabryje · 2025-02-20T18:34:28Z

@justinpark could you please verify if I included all modules that we use in the table in SQL Lab? There's a tool that helps selecting the correct modules here

korbit-ai

I've completed my review and didn't find any issues.

Files scanned

File Path	Reviewed
superset-frontend/src/setup/setupAGGridModules.ts	✅
superset-frontend/src/views/App.tsx	✅
superset-frontend/src/components/GridTable/index.tsx	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

Setting Value

Review Schedule Automatic excluding drafts

Max Issue Count 10

Automatic PR Descriptions ❌

Issue Categories

Category Enabled

Documentation ✅

Logging ✅

Error Handling ✅

Readability ✅

Design ✅

Performance ✅

Security ✅

Functionality ✅

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ [email protected]

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

github-actions · 2025-02-21T08:50:18Z

@kgabryje Processing your ephemeral environment request here. Action: up.

github-actions · 2025-02-21T08:59:20Z

@kgabryje Ephemeral environment spinning up at http://52.12.1.165:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

github-actions · 2025-02-21T14:27:40Z

@kgabryje Processing your ephemeral environment request here. Action: up.

github-actions · 2025-02-21T14:38:17Z

@kgabryje Ephemeral environment spinning up at http://52.12.1.165:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

villebro

LGTM

michael-s-molina · 2025-02-21T17:00:00Z

@kgabryje I pinged @justinpark and he'll take a look. Please don't merge it just yet 😉

kgabryje · 2025-02-21T17:22:26Z

superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx

@@ -363,7 +368,7 @@ describe('ResultSet', () => {
      );
    });
    const { getByRole } = setup(mockedProps, mockStore(initialState));
-    expect(getByRole('treegrid')).toBeInTheDocument();
+    expect(getByRole('grid')).toBeInTheDocument();


For some reason, in jest runtime, the ag grid root had a role "grid", not "treegrid"

justinpark

LGTM! Thanks for upgrade

justinpark

wait. it looks like the header menu has gone in this version. Could you check what is missing?

justinpark · 2025-02-21T18:03:54Z

superset-frontend/src/setup/setupAGGridModules.ts

+  RowApiModule,
+  CellApiModule,
+  RenderApiModule,
+  ClientSideRowModelModule,


Could you check if there's an api for column header?

There isn't, the closest thing is ColumnApiModule which we include here

CustomFilterModule might be the one.

kgabryje · 2025-02-21T18:19:28Z

wait. it looks like the header menu has gone in this version. Could you check what is missing?

Which header menu? I'm comparing before/after side by side and I can't see a difference. Also nothing happens on right-click in both versions.

In the recording, I'm running master on localhost, and the other tab is the ephemeral env for this PR.

Screen.Recording.2025-02-21.at.19.18.25.mov

justinpark · 2025-02-21T23:10:40Z

superset-frontend/src/components/GridTable/index.tsx

@@ -216,21 +231,16 @@ function GridTable<RecordType extends object>({
        `}
      >
        <AgGridReact
+          // TODO: migrate to Theme API - https://www.ag-grid.com/react-data-grid/theming-migration/
+          theme="legacy"


with this theme value, the existing ag-theme-quartz styles are broken.

Please migrate to theme api first to apply the theme name here.

justinpark · 2025-02-21T23:12:38Z

wait. it looks like the header menu has gone in this version. Could you check what is missing?

Which header menu? I'm comparing before/after side by side and I can't see a difference. Also nothing happens on right-click in both versions.

In the recording, I'm running master on localhost, and the other tab is the ephemeral env for this PR.

Screen.Recording.2025-02-21.at.19.18.25.mov

I found that it's not related to this change but this ant dropdown migration.

I will fix the dropdown menu issue separately.

chore: Upgrade AG Grid to use tree shaking

3eefefd

kgabryje requested review from justinpark, geido and michael-s-molina February 20, 2025 18:33

pull-request-size bot added the size/L label Feb 20, 2025

github-actions bot added the dependencies:npm label Feb 20, 2025

dosubot bot added change:frontend Requires changing the frontend sqllab Namespace | Anything related to the SQL Lab labels Feb 20, 2025

korbit-ai bot reviewed Feb 20, 2025

View reviewed changes

kgabryje added the testenv-up label Feb 21, 2025

kgabryje added 2 commits February 21, 2025 10:09

Fix tests, use legacy theme

7f38133

Fix tests

dbcc4d6

kgabryje added testenv-up and removed testenv-up labels Feb 21, 2025

apache deleted a comment from github-actions bot Feb 21, 2025

villebro approved these changes Feb 21, 2025

View reviewed changes

kgabryje commented Feb 21, 2025

View reviewed changes

justinpark approved these changes Feb 21, 2025

View reviewed changes

justinpark requested changes Feb 21, 2025

View reviewed changes

justinpark reviewed Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Upgrade AG Grid to use tree shaking #32334

chore: Upgrade AG Grid to use tree shaking #32334

kgabryje commented Feb 20, 2025 •

edited

Loading

kgabryje commented Feb 20, 2025

korbit-ai bot left a comment •

edited

Loading

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

villebro left a comment

michael-s-molina commented Feb 21, 2025

kgabryje Feb 21, 2025

justinpark left a comment

justinpark left a comment

justinpark Feb 21, 2025

kgabryje Feb 21, 2025

justinpark Feb 21, 2025

kgabryje commented Feb 21, 2025

justinpark Feb 21, 2025

justinpark commented Feb 21, 2025

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Category	Enabled
Documentation	✅
Logging	✅
Error Handling	✅
Readability	✅
Design	✅
Performance	✅
Security	✅
Functionality	✅

chore: Upgrade AG Grid to use tree shaking #32334

Are you sure you want to change the base?

chore: Upgrade AG Grid to use tree shaking #32334

Conversation

kgabryje commented Feb 20, 2025 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

kgabryje commented Feb 20, 2025

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Interacting with Korbit

Customizing Korbit

Current Korbit Configuration

Feedback and Support

Looking to add Korbit to your team? Get started with a free 2 week trial here

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025

villebro left a comment

Choose a reason for hiding this comment

michael-s-molina commented Feb 21, 2025

kgabryje Feb 21, 2025

Choose a reason for hiding this comment

justinpark left a comment

Choose a reason for hiding this comment

justinpark left a comment

Choose a reason for hiding this comment

justinpark Feb 21, 2025

Choose a reason for hiding this comment

kgabryje Feb 21, 2025

Choose a reason for hiding this comment

justinpark Feb 21, 2025

Choose a reason for hiding this comment

kgabryje commented Feb 21, 2025

justinpark Feb 21, 2025

Choose a reason for hiding this comment

justinpark commented Feb 21, 2025

kgabryje commented Feb 20, 2025 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading