feat(standalone): granular Explore standalone modes and iframe-safe dashboard fullscreen - #44165
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44165 +/- ##
==========================================
+ Coverage 81.08% 81.10% +0.01%
==========================================
Files 2955 2955
Lines 178321 178385 +64
Branches 41304 41336 +32
==========================================
+ Hits 144588 144674 +86
+ Misses 31024 31002 -22
Partials 2709 2709
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #97acfd
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/ExploreChartPanel/index.tsx - 1
- Test regression from strict equality · Line 507-507
Additional Suggestions - 6
-
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx - 1
-
Redundant standalone spread · Line 252-252This spread is dead: `...additionalParam` is spread after it, and `additionalParam` (from `history.location.search`) always contains `standalone` whenever `standalone` is truthy, since `props.standalone` is derived from the same URL via `getUrlParam`. The numeric value is always overridden by the URL string, so mode 2 is already carried by `additionalParam`. Remove the line or move it after `...additionalParam` if the numeric mode must win.
-
-
superset/utils/webdriver.py - 1
-
Dead enum member · Line 141-141`HIDE_NAV_SHOW_CONTROLS = 2` is added to `ChartStandaloneMode` but never referenced in any Python file. The only backend consumer, `superset/utils/screenshots.py:490`, uses `ChartStandaloneMode.HIDE_NAV.value`, and `ReservedUrlParameters.is_standalone_mode()` treats standalone as a boolean. The frontend `ExploreStandaloneMode.HideNavShowControls` is used, but this backend member is dead code. Remove it or consume it.
-
-
superset-frontend/src/explore/constants.ts - 1
-
Unused enum member · Line 229-229`HideNavShowControls` (=2) is never referenced anywhere in the codebase; consumers only special-case `ExploreStandaloneMode.HideNav` (=1), so mode 2 relies on implicit fall-through to the full editor. Either drop the member or branch on it explicitly in `ExploreViewContainer`/`ExploreChartPanel` so mode 2's behavior is intentional.
-
-
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx - 2
-
Magic number in mock · Line 135-135The mock hardcodes `standalone === 1`, but production (`ExploreChartPanel/index.tsx:507`) compares against `ExploreStandaloneMode.HideNav`. If the enum value ever changes, this test silently diverges from real behavior. Import `ExploreStandaloneMode` from 'src/explore/constants' and compare `standalone === ExploreStandaloneMode.HideNav` instead of the literal `1`.
-
Wrong enum in comment · Line 299-299The comment references `ChartStandaloneMode.HIDE_NAV`, but no such enum exists in the repo — the actual enum is `ExploreStandaloneMode.HideNav` (constants.ts:226). This misleads readers about the mode's source. Update the comment to the real symbol.
-
-
superset-frontend/src/dashboard/components/Header/Header.test.tsx - 1
-
Test mock inconsistency · Line 874-880This test simulates "not in standalone mode" via `window.history.pushState('/dashboard')`, but the `useLocation` mock (line 51) still hardcodes `search: '?standalone=1'`. The label logic in `useHeaderActionsDropdownMenu` reads `getUrlParam(URL_PARAMS.standalone)` (i.e. `window.location.search`), so the test passes today — but the mock contradicts the scenario, and a refactor to `location.search` would silently break it.
-
Review Details
-
Files reviewed - 10 · Commit Range:
d2a1dcd..a9416cf- superset-frontend/src/dashboard/components/Header/Header.test.tsx
- superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx
- superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
- superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
- superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
- superset-frontend/src/explore/constants.ts
- superset-frontend/src/explore/exploreUtils/exploreUtils.test.tsx
- superset-frontend/src/explore/exploreUtils/index.ts
- superset-frontend/src/explore/types.ts
- superset/utils/webdriver.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| ); | ||
|
|
||
| if (standalone) { | ||
| if (standalone === ExploreStandaloneMode.HideNav) { |
There was a problem hiding this comment.
standalone is now compared with strict equality (=== ExploreStandaloneMode.HideNav, i.e. === 1). The existing ExploreChartPanel.test.tsx passes standalone: true (boolean), and true === 1 is false, so the standalone branch no longer renders StandaloneDownloadControl and the test at line 232 will fail. Update those tests to pass the numeric mode.
Code Review Run #97acfd
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #192d32Actionable Suggestions - 0Additional Suggestions - 3
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
sadpandajoe
changed the title
d83aa21 to
1e2f806
Compare
Code Review Agent Run #66de0eActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #8fb7bfActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Fair point, this mock doesn't change the assertion. The fullscreen label comes from getUrlParam(URL_PARAMS.standalone), which reads window.location.search, and that is already set by pushState on the line below. It is deliberate though. The shared useLocation mock at the top of the file hardcodes search: '?standalone=1' for every test, so without the override this test would be asserting non standalone behaviour while the router mock still claims standalone. That is confusing to read even if it is inert, and an earlier run of this same bot (#66de0e) flagged exactly that inconsistency as worth fixing, which is why the override was added. Happy to drop it if reviewers prefer the smaller diff. Either way it is test only, zero behavioural impact, and both runs marked it as non actionable. |
Code Review Agent Run #f0739aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #748f9aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Heya @anamitraadhikari, appreciate you sticking with the bot review back-and-forth on this one. Once CI's green I'll give this a proper look, the standalone mode split and the report-vs-embed screenshot distinction seem like the right call so far. |
…in standalone dashboards - Add ExploreStandaloneMode enum: 1=chart-only (backward compat), 2=hide nav but keep editor controls - Hide "Enter fullscreen" menu item when dashboard is already in standalone mode - Update ExploreViewContainer and ExploreChartPanel to use numeric standalone instead of boolean - Add backend ChartStandaloneMode.HIDE_NAV_SHOW_CONTROLS enum value - Add tests for both standalone modes - Fix PropTypes: standalone bool → number
…escapable The granular standalone work in the previous commit switched the Explore frontend to a numeric comparison, but three things it depends on still assumed the old boolean, single-mode world. Explore ignored the mode. The bootstrap payload carries `is_standalone_mode()`, a boolean, so `standalone === HideNav` was never true and `standalone=1` rendered the full editor instead of a bare chart. Mode 2 only appeared to work because the full editor is also its intended output. The mode is now read from the URL with `getUrlParam`, which already maps '1'/'true' to 1 and '2' to 2, so screenshot URLs (`ChartStandaloneMode.HIDE_NAV` sends `standalone=true`) keep working. The redux and bootstrap types go back to `boolean` to match what the backend actually sends. Mode 2 collapsed to mode 1 on any interaction. `mountExploreUrl` wrote `standalone=1` unconditionally, and Explore calls it on `history.replace` after interactions, so the editor turned into a bare chart on the first click. Callers now pass the active mode through `extraSearch` and the util only supplies HideNav when nothing was given. Fullscreen became a one-way door. The dashboard header hid the toggle whenever `standalone` was set, but that param is also what "Enter fullscreen" sets, so the menu item removed its own escape hatch. The guard is now a real iframe check via `isEmbedded()`, which is what the embed protection was reaching for -- the existing `isEmbedded` local is `!dashboardInfo.userId` and does not cover an authenticated user in an iframe. Top-level users keep both directions; iframes get no toggle at all. No Python changes: `is_standalone_mode()` stays boolean and is untouched. Tests: URL-driven Explore cases for modes 1, 2 and 'true'; an iframe case for the dashboard menu; mode-preservation tests for `mountExploreUrl`; and the subdirectory-prefix "Exit fullscreen" test is restored now that the exit path is reachable again.
…hy modes, normalize enum Addresses three review findings. ExploreChartPanel tests were failing. `ExploreChartPanel.test.tsx` passes `standalone` as a boolean, which stopped matching once the component moved to `=== ExploreStandaloneMode.HideNav`, so the standalone branch no longer rendered `StandaloneDownloadControl`. The tests now pass the enum members rather than raw numbers, which also reads better than the magic 1/0. Unrecognized truthy values regressed. The backend still treats anything other than absent/'false'/'0' as standalone, while the frontend had begun trusting only the numeric URL value, so `standalone=3` hid the nav server-side but rendered the full editor, and a non-numeric truthy value was read as mode 0. Mode 2 stays an explicit numeric opt-in from the URL, but everything else now defers to the boolean the backend already computed, so the two sides cannot disagree. Adds a `standalone=3` regression test. `ChartStandaloneMode` mixed str and int. `HIDE_NAV = "true"` alongside `SHOW_NAV = 0` predates this branch, and adding an int made it worse. It is now all ints and consistent with `DashboardStandaloneMode` directly above it. `HIDE_NAV.value` has a single call site, and `standalone=1` behaves identically to `standalone=true` on both the backend truthiness check and the frontend parse, which the existing `standalone=true` test covers.
The shared `useLocation` mock reports `search: '?standalone=1'` for every test in this file. The non-standalone fullscreen test changed only `window.location` via pushState, so the router location and window.location disagreed inside it. The assertion was still correct, since the hook reads the param through `getUrlParam` (window.location.search) and only uses `location.pathname` from the router, but the test would have kept passing on a stale premise had the hook ever switched to `location.search` -- the same boolean/numeric mismatch class this branch already fixed once. Overriding the mock locally keeps both in agreement.
Normalizing `ChartStandaloneMode` to integers changed the value `ChartScreenshot` puts in its URL from `standalone=true` to `standalone=1`. That value is load bearing on the frontend: `isReportScreenshotMode()` in plugin-chart-echarts treats `true` and `3` as automated captures and suppresses animation, while `1` and `2` are live embeds that keep it. Chart report and thumbnail captures therefore stopped suppressing animation and could be taken mid draw. Adds `ChartStandaloneMode.REPORT = 3`, mirroring the existing `DashboardStandaloneMode.REPORT`, and has `ChartScreenshot` request it. The enum stays integer valued, and `3` is already understood everywhere it matters: the backend treats it as standalone, Explore renders chart only for it, and the ECharts check already recognises it. On backwards compatibility for links carrying `standalone=true`: no change was needed, but it was previously untested. `standalone` is declared a number param, so `getUrlParam` maps `true` to 1 and `false` to 0, and `is_standalone_mode()` still accepts anything other than absent, `false` or `0`. That holds for Explore and for dashboards, where `DashboardBuilder` compares against None/HideNavAndTitle/Report and so reads a legacy link as HideNav. Tests: - `urlUtils.test.ts` pins the coercion table for the standalone param, including `true` and `TRUE` to 1, `false` to 0, and unparseable to null. - `screenshot_test.py` pins that chart captures request mode 3 and not 1, that dashboard captures still request 3, and that both enums stay integers. - Explore tests renamed to say what they guard: legacy `true` compatibility, and chart only rendering for report mode 3. - `Echart.tsx` comment corrected, since charts no longer emit `true`.
`explore.standalone` is seeded by hydrateExplore from `getUrlParam(URL_PARAMS.standalone)`, so it holds the coerced numeric mode, or null when the param is absent or unparseable. It is not the backend's boolean `is_standalone_mode()`. Earlier comments on this branch said otherwise, and the type had been set to `boolean` on that mistaken basis. Corrects three things that followed from it: - Types. The state shape is `number | null` in `ExplorePageState`, `ExploreState` and ExploreViewContainer's local `ExploreRootState`. Type checking passed before only because the hydrate boundary is loosely typed, which is the same kind of mismatch that made `standalone=1` render the full editor in the first place. - Comment. mapStateToProps no longer claims to fall back to a backend boolean. Both branches read the one URL-derived value; mode 2 keeps the editor and any other truthy mode renders chart-only, which is what any truthy `standalone` did before granular modes. An unparseable value arrives as null and renders normally, as it did before. - Test that could not fail. The ExploreChartPanel mock keyed only off mode 1, so modes 2 and 0 rendered the same marker and the mode 2 test passed either way. The mock now emits a distinct marker per mode, the mode 2 test asserts it positively, and a mode 0 case pins the other side. Removing the mode 2 branch from mapStateToProps now fails that test, verified locally. The fixture also derives `explore.standalone` from the same search string rather than hardcoding a boolean the app never stores.
468cd4e to
e768c8f
Compare
There was a problem hiding this comment.
Code Review Agent Run #914a01
Actionable Suggestions - 1
-
superset-frontend/src/explore/exploreUtils/index.ts - 1
- Impossible undefined comparison · Line 185-187
Additional Suggestions - 2
-
superset-frontend/src/utils/urlUtils.test.ts - 1
-
misleading enum reference · Line 151-151Comment references `ChartStandaloneMode.REPORT`, but no such enum exists in the repo (search finds it only in comments). The actual enum added is `ExploreStandaloneMode`, which has no `Report` member. Update the comment to reference the real symbol so readers aren't misled.
-
-
tests/unit_tests/utils/screenshot_test.py - 1
-
Missing return type annotations · Line 744-752The four new `TestScreenshotStandaloneMode` tests omit return type annotations, unlike the repo standard for new Python code. Adding `-> None` keeps the file consistent with the typing rules applied elsewhere in this test module. ([.cursor/rules/dev-standard.mdc](.cursor/rules/dev-standard.mdc))
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/explore/constants.ts - 1
- duplicate standalone enum · Line 226-230
Review Details
-
Files reviewed - 17 · Commit Range:
4b05179..e768c8f- superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.test.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx
- superset-frontend/src/dashboard/components/Header/Header.test.tsx
- superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx
- superset-frontend/src/explore/components/ExploreChartPanel/ExploreChartPanel.test.tsx
- superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
- superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
- superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
- superset-frontend/src/explore/constants.ts
- superset-frontend/src/explore/exploreUtils/exploreUtils.test.tsx
- superset-frontend/src/explore/exploreUtils/index.ts
- superset-frontend/src/explore/reducers/exploreReducer.ts
- superset-frontend/src/explore/types.ts
- superset-frontend/src/utils/urlUtils.test.ts
- superset/utils/screenshots.py
- superset/utils/webdriver.py
- tests/unit_tests/utils/screenshot_test.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| if (search.standalone === undefined) { | ||
| search.standalone = DashboardStandaloneMode.HideNav; | ||
| } |
There was a problem hiding this comment.
The guard compares search.standalone (typed string | number by the Record<string, string | number> cast at index.ts:173) against undefined; under strictNullChecks with no noUncheckedIndexedAccess, tsc 5.4.5 raises TS2367 (no overlap), failing the type-check build. The runtime intent is right — default only when absent — so use typeof search.standalone === 'undefined' (or widen the record type to include undefined).
Code Review Run #914a01
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
Sort of, yeah. #44547 was fixing the |
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
Description:
SUMMARY
Adds a second Explore standalone mode and makes the dashboard fullscreen
toggle safe to use inside an iframe.
standaloneon Explore was previously all-or-nothing: any truthy valuerendered a bare chart. Embedding an editable chart in an iframe therefore
meant choosing between "no chart controls at all" and "the full Superset
nav". This adds
ExploreStandaloneMode:1— chart only (unchanged; what thumbnails and screenshots use)2— hide the nav, keep the editor controlsBecause the bootstrap payload only carries
is_standalone_mode(), aboolean, the mode is resolved from the URL via
getUrlParam, which alreadymaps
'1'/'true'to1and'2'to2. Screenshot URLs(
ChartStandaloneMode.HIDE_NAVsendsstandalone=true) keep working.mountExploreUrlnow carries the active mode throughextraSearchinsteadof writing
standalone=1unconditionally, which would otherwise downgrademode 2 to mode 1 on the first
history.replace.On dashboards, the header's fullscreen toggle is hidden when the page is
actually inside an iframe (
isEmbedded()fromdashboard/util/isEmbedded).Exiting fullscreen reloads without the
standaloneparam and restores thefull Superset nav, which breaks an embed. Note this is distinct from the
isEmbeddedlocal in the menu hook, which is!dashboardInfo.userIdandonly covers anonymous guests via the embedded SDK — not an authenticated
user whose dashboard is iframed. Top-level users keep both directions, so
fullscreen is not a one-way door.
No Python behaviour changes:
is_standalone_mode()is untouched and stillreturns a boolean. The only backend edit is one added enum value on
ChartStandaloneMode.BEFORE/AFTER SCREENSHOTS
Screenshot 1:
Explore with
standalone=2(new mode): Superset nav hidden, chart editor controls kept. On master any truthystandalonerenders a bare chart, so this layout was not previously possible.Screenshot 2:
Dashboard inside an iframe with
standalone=1: the ⋮ menu offers no fullscreen toggle. Exiting would reload withoutstandaloneand restore the full Superset nav, breaking the embed. Outside an iframe the menu is unchanged.TESTING INSTRUCTIONS
Explore:
/explore/?slice_id=<id>— full editor with nav/explore/?slice_id=<id>&standalone=1— chart only/explore/?slice_id=<id>&standalone=2— no nav, editor controls kept/explore/?slice_id=<id>&standalone=true— chart only (screenshot path)bare chart
Dashboard:
/superset/dashboard/<id>/— "Enter fullscreen" present in the ⋮ menustandalone=1and "Exit fullscreen" is availableADDITIONAL INFORMATION