fix(dashboard): hide Edit button in embedded dashboards#40687
Conversation
Embedded (Embedded SDK) dashboards authenticate with a guest token and have no userId, so the header already derives `isEmbedded = !dashboardInfo?.userId`. The Edit button was gated only on `userCanEdit`, so it appeared in embedded dashboards; entering edit mode there and discarding leaves the iframe stuck on a perpetual loading screen (#30893). Gate the button on `!isEmbedded` as well. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #d43495Actionable 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 |
dosubot
Bot
added
dashboard
There was a problem hiding this comment.
Pull request overview
This PR prevents a broken editing flow in Embedded SDK dashboards by hiding the Edit dashboard button when the dashboard is rendered in an embedded (guest token) context, while preserving existing behavior for normal logged-in dashboards.
Changes:
- Gate rendering of the Edit dashboard button on
userCanEdit && !isEmbedded. - Add unit coverage ensuring the button is not shown when
dashboardInfo.userIdis missing (embedded context).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| superset-frontend/src/dashboard/components/Header/index.tsx | Hide the Edit button in embedded dashboards by adding an !isEmbedded guard and updating memo deps accordingly. |
| superset-frontend/src/dashboard/components/Header/Header.test.tsx | Add a unit test asserting the Edit button is absent when userId is undefined even if edit permission is present. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Review
Head SHA verified: 8d13ce2
19-Scan Coverage
All 19 Python-specific scans N/A — TypeScript-only PR (2 files, +17/-1).
Key Checks
Rule 26 (behavioral test): Verified. testIdAttribute: 'data-test' is configured in spec/helpers/setup.ts, so queryByTestId('edit-dashboard-button') correctly maps to the button's data-test attribute. Removing the !isEmbedded guard would cause this test to fail — not a false positive.
Rule 26 (positive case): Covered by the existing "should toggle the edit mode" test (line 600), which uses dash_edit_perm: true + userId: '1' (non-embedded) and asserts the button is present.
useMemo deps: isEmbedded correctly added to the dependency array at line 779 — prevents stale closure renders.
Entry-point completeness: The Edit dashboard button is the only call site for handleEnterEditMode. No keyboard shortcut for edit mode exists in the component. The actions dropdown (useHeaderActionsDropdownMenu.tsx) already gated the fullscreen toggle with !isEmbedded at line 228.
Findings
BLOCKER: None · HIGH: None · MEDIUM: None · NIT: None
PRAISE
- Minimal fix — 1 condition, 1 dep entry, 15-line test.
- Reuses the established
isEmbedded = !dashboardInfo?.userIdpattern already applied to the metadata bar (line 652) — no new logic. - Test comment correctly documents the non-obvious guest-token → no
userId→isEmbeddedchain.
CI: CodeQL, pre-commit, playwright, cypress, frontend-build all pass. Jest/lint queued but no failures.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40687 +/- ##
=======================================
Coverage 63.96% 63.96%
=======================================
Files 2658 2658
Lines 143095 143095
Branches 32907 32907
=======================================
Hits 91537 91537
Misses 49991 49991
Partials 1567 1567
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:
|
SUMMARY
Embedded dashboards (rendered via the Embedded SDK) authenticate with a guest token, so they have no logged-in
userId. The dashboard header already derives embedded state from this (isEmbedded = !dashboardInfo?.userId, used elsewhere e.g. to hide the metadata bar), but the Edit dashboard button was gated only onuserCanEdit. As a result it rendered inside embedded dashboards, where editing isn't a supported flow.Per #30893, clicking Edit dashboard → Discard in an embedded dashboard leaves the iframe stuck on a perpetual loading screen (the discard navigation can't resolve in the embedded context). Hiding the Edit button removes that broken entry point.
This gates the Edit button on
!isEmbeddedin addition touserCanEdit. No behavior change for normal (logged-in) dashboards.Closes #30893
BEFORE/AFTER
Before: embedded dashboards with edit permission show the Edit dashboard button → entering edit mode and discarding hangs the iframe.
After: the Edit dashboard button is not rendered in embedded dashboards.
TESTING INSTRUCTIONS
Unit test added:
Header.test.tsx→ "should NOT render the Edit dashboard button when embedded" (asserts the button is absent whenuserIdis undefined even withdash_edit_perm: true).ADDITIONAL INFORMATION
🤖 Generated with Claude Code