-
Notifications
You must be signed in to change notification settings - Fork 16.5k
feat(Embedding): Embeddable Charts as per SIP #33424
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
base: master
Are you sure you want to change the base?
feat(Embedding): Embeddable Charts as per SIP #33424
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33424 +/- ##
==========================================
+ Coverage 60.48% 65.73% +5.24%
==========================================
Files 1931 554 -1377
Lines 76236 40109 -36127
Branches 8568 0 -8568
==========================================
- Hits 46114 26367 -19747
+ Misses 28017 13742 -14275
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Missing embedded property context ▹ view | 🧠 Not in scope | |
| Missing null check in HYDRATE_EMBEDDED reducer ▹ view | 🧠 Incorrect | |
| Insecure Default Embedding Policy ▹ view | ||
| Inconsistent and Unclear Column Naming ▹ view | 🧠 Not in standard | |
| Unchecked External Data Injection ▹ view | 🧠 Not in scope | |
| Unsafe Type Bypass and Inefficient Object Copy ▹ view | 🧠 Incorrect | |
| Unsafe Type Bypass in Embedded Datasource Hydration ▹ view | 🧠 Not in standard | |
| State overwrite in HYDRATE_EMBEDDED case ▹ view | 🧠 Incorrect | |
| Duplicate Hydration Logic for Embedded Charts ▹ view | ||
| Nullable Audit Timestamps ▹ view | 🧠 Not in scope |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/dashboard/reducers/datasources.ts | ✅ |
| superset-frontend/src/explore/reducers/datasourcesReducer.ts | ✅ |
| superset-frontend/src/dashboard/reducers/dashboardInfo.js | ✅ |
| superset-frontend/src/dashboard/actions/datasources.ts | ✅ |
| superset-frontend/src/dashboard/reducers/sliceEntities.js | ✅ |
| superset/models/embedded_chart.py | ✅ |
| superset/migrations/versions/2025-05-14_03-40_67dd9e3429b2_add_embedded_charts_table.py | ✅ |
| superset/daos/chart.py | ✅ |
| superset/commands/chart/delete.py | ✅ |
| superset-frontend/src/embedded/embeddedChart/useExploreData.ts | ✅ |
| superset-frontend/src/types/bootstrapTypes.ts | ✅ |
| superset/commands/chart/exceptions.py | ✅ |
| superset/embedded/view.py | ✅ |
| superset-frontend/src/dataMask/reducer.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/ui-overrides/types.ts | ✅ |
| superset-frontend/src/components/Chart/chartReducer.ts | ✅ |
| superset-frontend/src/dashboard/reducers/dashboardState.js | ✅ |
| superset-frontend/src/embedded/embeddedChart/hydrateEmbedded.ts | ✅ |
| superset-frontend/src/embedded/index.tsx | ✅ |
| superset-frontend/src/embedded/embeddedChart/index.tsx | ✅ |
| superset-frontend/src/dashboard/components/SliceHeader/index.tsx | ✅ |
| superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx | ✅ |
| superset/models/slice.py | ✅ |
| superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx | ✅ |
| superset-frontend/src/dashboard/components/Header/index.jsx | ✅ |
| superset/charts/api.py | ✅ |
| superset/charts/schemas.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| embedded?: { | ||
| dashboard_id: string; | ||
| chart_id: string; | ||
| resource_type: 'dashboard' | 'chart'; | ||
| }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| [HYDRATE_EMBEDDED]() { | ||
| return { | ||
| ...action.data.sliceEntities, | ||
| }; | ||
| }, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @property | ||
| def allowed_domains(self) -> list[str]: | ||
| """ | ||
| A list of domains which are allowed to embed the chart. | ||
| An empty list means any domain can embed. | ||
| """ |
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.
Insecure Default Embedding Policy 
Tell me more
What is the issue?
The allowed_domains property allows unrestricted embedding from any domain when allow_domain_list is empty, which is an insecure default that enables clickjacking attacks.
Why this matters
Without domain restrictions by default, malicious sites could embed and manipulate the chart content, potentially leading to data theft or user deception through UI redressing attacks.
Suggested change ∙ Feature Preview
Change the behavior to deny all domains by default instead of allow all. For example:
@property
def allowed_domains(self) -> list[str]:
"""A list of domains which are allowed to embed the chart.
An empty list means no domains can embed (secure by default).
"""
return self.allow_domain_list.split(",") if self.allow_domain_list else ["DENY_ALL"]Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| __tablename__ = "embedded_charts" | ||
|
|
||
| uuid = Column(UUIDType(binary=True), default=uuid.uuid4, primary_key=True) | ||
| allow_domain_list = Column(Text) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (action.type === HYDRATE_EMBEDDED) { | ||
| // @ts-ignore | ||
| return { ...action.data.datasources }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // @ts-ignore | ||
| return { ...action.data.datasources }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (action.type === HYDRATE_EMBEDDED) { | ||
| // @ts-ignore | ||
| return { ...action.data.datasources }; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (action.type === HYDRATE_EMBEDDED) { | ||
| return { | ||
| ...action.data.datasources, | ||
| }; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| [HYDRATE_EMBEDDED]() { | ||
| return { ...state, ...action.data.dashboardState }; | ||
| }, |
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.
Duplicate Hydration Logic for Embedded Charts 
Tell me more
What is the issue?
The HYDRATE_EMBEDDED action handler is exactly the same as HYDRATE_DASHBOARD, which could lead to inconsistent state management if embedded charts require different hydration logic.
Why this matters
If embedded charts have unique state requirements or need special handling during hydration, using the same logic as dashboard hydration could cause incorrect behavior or state inconsistencies in embedded contexts.
Suggested change ∙ Feature Preview
Consider whether embedded charts need different hydration logic and modify accordingly. If they truly need the same logic, consider using a shared function:
const hydrateState = data => ({ ...state, ...data.dashboardState });
[HYDRATE_DASHBOARD]() {
return hydrateState(action.data);
},
[HYDRATE_EMBEDDED]() {
return hydrateState(action.data);
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| sa.Column("created_on", sa.DateTime(), nullable=True), | ||
| sa.Column("changed_on", sa.DateTime(), nullable=True), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
I commented on the SIP back in October, #30075 (comment), but why not embedding a single-chart-dashboard? There's a related idea which is allowing to embed parts of the Explore experience, potentaly allowing embedded use cases for light data exploration. Wondering if that's in-scope here (?) The idea would be to allow/limit part of the Explore experience, say I may want to hide/show different panels and/or controls... I'm guessing the idea here is more to simply embed a single chart, without interactions. |
This pr deals with embedding a single chart with normal dashboard interactons What i understood from the previous comment of the sip was #30075 (comment) we need to have chart creation within dashboard ... I have implemented the same in a private fork , we used to call it easy charts 1.User can place a chart container on the grid <- completely empty which will have a button +add chart on it We can connect on this , if easy charts are implemented in a seperate sip , we can bring those anytime here in embedded charts |
Something like this @mistercrunch , Some level of explore controls in dashboard for the chart Or something where its metric centric & the user can select metrics & create instant charts within dashboard like below Or something where the user can just change the query groupby just by hitting a dropdown & selecting from a list of Groupbys like Display by departments like the below Curious to know , whats the idea will fit best with superset so we can implement it |
|
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@mistercrunch Ephemeral environment spinning up at http://35.87.137.37:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
Complex topic. We should probably talk. Some thoughts related to all this:
|
Yes , i somewhat got the idea , we can connect on this , i have created an sip for the same , would include points from the ones mentioned above |
|
This needs a big rebase... do we still want to carry this through? The SIP is already approved, so it just needs to be completed, or the SIP may (eventually) be discarded due to inactivity. |
Add "Embed chart" option to chart header dropdown menu on dashboards. When clicked, opens a modal that generates embed code (iframe + guest token) using the existing permalink + guest token infrastructure. Changes: - Add POST /api/v1/embedded_chart/ endpoint to create embeddable charts - Add EmbeddedChartModal component with allowed domains and TTL settings - Add "Embed chart" menu item to SliceHeaderControls - Add EmbedChart to MenuKeys enum This brings UI parity with PR apache#33424's approach while using our simpler ephemeral permalink-based architecture (no database model needed). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>



SUMMARY
This pr implements Embeddable Charts as per SIP #30075 (comment)
The only takeaway , the solution doesnt use charts in standalone mode , instead it takes the dashboard charts, hydrates them with all the required data & tells them to take full width & height of iframe , this avoids use of standalone mode & gives the same experience as dashboard charts
Chart embedding in action with nextjs app
screen-capture.23.webm
Embed uuid generation just like dashboard

Most of normal chart features of dashboard works
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION