fix(embedded): show already-added allowed domains in embed modal (closes #35328)#41399
fix(embedded): show already-added allowed domains in embed modal (closes #35328)#41399rusackas wants to merge 1 commit into
Conversation
The embed dashboard modal wraps the allowed-domains Input in a FormItem
that carries name="allowed-domains". With an antd v5 Form, a named
FormItem is taken over by the Form store: antd clones the child and
injects its own value/onChange, ignoring the controlled value={allowedDomains}
passed to the Input. As a result, domains loaded from the GET /embedded
response never render when the modal is reopened.
Swap name= for htmlFor= on the FormItem so it acts purely as a label
wrapper, restoring the controlled Input while preserving the a11y label
association (matching the Input's id="allowed-domains"). Adds a
regression test asserting the input is populated from the API response.
Adopts anandraiin3#15 (authored via Devin AI). Credit to
@anandraiin3 for the original fix.
Closes apache#35328
Co-Authored-By: Devin AI <devin-ai-integration[bot]@users.noreply.github.com>
Code Review Agent Run #abb568Actionable 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41399 +/- ##
=======================================
Coverage 64.44% 64.44%
=======================================
Files 2655 2655
Lines 145473 145473
Branches 33575 33575
=======================================
Hits 93747 93747
Misses 50027 50027
Partials 1699 1699
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:
|
SUMMARY
Fixes the dashboard "Embed dashboard" modal so that previously saved allowed domains are displayed in the input when the modal is reopened.
This adopts anandraiin3/superset#15 (originally authored via Devin AI). Full credit to @anandraiin3 for the original fix — this PR brings it onto current
masterwith the regression test intact.Root cause
The Ant Design v5 overhaul (#31590) restructured this form to use the
@superset-ui/core/componentsForm/FormItemwrappers and movedname="allowed-domains"from the<Input>element onto the<FormItem>wrapper:When
FormItemhas aname, Ant Design takes over the field via its internalFormstore: it clones the child and injects its ownvalue/onChange, ignoring the explicitvalue={allowedDomains}passed to theInput. As a result, programmatic state updates from React (i.e.setAllowedDomains(result.allowed_domains.join(', '))after theGET /embeddedcall resolves) never propagate to the rendered input.This is why:
embedded.allowed_domainsis populated. This also matches the issue symptom where the input detects a duplicate domain and keeps Save disabled (theisDirtycheck uses React state, which is populated correctly).Fix
Replace
name="allowed-domains"on<FormItem>withhtmlFor="allowed-domains". TheFormItemis now used purely as a label/layout wrapper (not a controlled antd field), so the<Input>'s explicitvalue/onChangeare honored as a normal controlled component. ThehtmlForkeeps the label associated with the input for accessibility (matching the existingid="allowed-domains"on the<Input>).This is a minimal, focused change that restores the pre-AntD-v5 behavior without changing how data is loaded, saved, or validated.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — pure UI state-binding fix; no visual styling changes. Verified via the new unit test described below.
TESTING INSTRUCTIONS
Automated:
A new test,
populates the allowed-domains input from the API response, asserts that when the GET/api/v1/dashboard/<id>/embeddedendpoint returns{ allowed_domains: ['example.com'] }, the input element'svalueis"example.com". This test fails onmasterand passes with this fix.Manual (mirrors the issue reproduction):
EMBEDDED_SUPERSETfeature flag.https://mytestdomain.comand click Save changes.https://mytestdomain.com(previously it was empty).ADDITIONAL INFORMATION
Adopted from anandraiin3#15 (originally authored via Devin AI). Credit to @anandraiin3.