Repository navigation
feat(semantic-layer): guide semantic layer setup - #45649
mikebridge wants to merge 10 commits into
Conversation
github-actions
Bot
added
i18n
✅ 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 #45649 +/- ##
==========================================
+ Coverage 84.70% 84.72% +0.01%
==========================================
Files 3069 3070 +1
Lines 205610 205784 +174
Branches 48330 48406 +76
==========================================
+ Hits 174157 174342 +185
+ Misses 28160 28149 -11
Partials 3293 3293
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| {semanticSetup === 'layer' && ( | ||
| <SemanticLayerModal | ||
| show | ||
| onHide={() => setSemanticSetup('view')} |
There was a problem hiding this comment.
Suggestion: When creation succeeds after Cancel, its POST has committed but unmounting ignores the response; if the picker refreshed first, it stays empty until reopened.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Race condition
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/DatasetList/index.tsx
**Line:** 1667:1667
**Comment:**
*Race Condition: When creation succeeds after Cancel, its POST has committed but unmounting ignores the response; if the picker refreshed first, it stays empty until reopened.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Agreed—if the user cancels while the create POST is in flight, the unmounted modal drops the response, so the freshly mounted view picker can load its list before the layer exists and stay empty until the dialog is reopened. Should the create completion be reported back to the parent (or the picker refetched on a later signal) even after cancel?
There was a problem hiding this comment.
Yes. Cancel currently only closes/unmounts the modal; it doesn’t guarantee the POST was canceled or rolled back. If that POST later succeeds, the parent should receive a creation-completed signal even though the modal is stale, and use it only to invalidate/refetch the semantic-layer list.
Keep that signal separate from onHide: a late completion must not switch dialogs, close a newer session, or show a stale success toast. A parent-owned refresh signal can trigger a refetch if the view picker is already open, while its normal initial fetch handles the case where creation finishes before the picker remounts. This closes the race without letting the late response take over the UI.
There was a problem hiding this comment.
Agreed, and fixed in 33fe0325be. The create response now signals the parent even if the layer dialog was cancelled, so the view picker refetches its layer list. A stale response can't reopen a dialog or show a success toast. A delayed-POST regression test failed before the fix and passes now.
| from .utils import bootstrap_user_data, get_config_value | ||
|
|
||
| FRONTEND_CONF_KEYS = ( | ||
| "SEMANTIC_LAYER_DOCUMENTATION_URL", |
There was a problem hiding this comment.
Suggestion: This sends the raw configured URL to every user's bootstrap payload, exposing embedded credentials even though the UI hides links containing them.
Assessment: 🔴 Critical · 🔁 Occurrence: Rarely · 🏷️ Security
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 84:84
**Comment:**
*Security: This sends the raw configured URL to every user's bootstrap payload, exposing embedded credentials even though the UI hides links containing them.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Agreed—SEMANTIC_LAYER_DOCUMENTATION_URL is added to the common bootstrap conf unchanged, so a URL with embedded credentials is delivered to every user's page even though the dialog only hides the link; the docs say such URLs are hidden. Should the server drop or sanitize credential-bearing URLs before exposing them?
There was a problem hiding this comment.
Yes. The concern is valid: client-side hiding does not prevent the raw value from appearing in the bootstrap payload. The server should validate SEMANTIC_LAYER_DOCUMENTATION_URL before adding it to conf and return None for URLs with credentials (and, ideally, other invalid URLs). Rejecting the whole URL is safer than stripping credentials, which could silently change its destination. Keep the frontend check as defense in depth, and add a bootstrap test confirming credential-bearing URLs are not exposed.
There was a problem hiding this comment.
Agreed, and fixed in 33fe0325be. The server now leaves credential-bearing, non-HTTP(S) and malformed setup URLs out of the common bootstrap (they become None) and logs one warning, without the URL. The bootstrap tests failed before the filter and pass now, and the configuration docs describe the behavior.
|
| > | ||
| {t('Reload metadata')} | ||
| </Button> | ||
| {/* COPY-TBD: semantic setup guidance, pending product copy. */} |
There was a problem hiding this comment.
IIUC the description still calls the copy a placeholder pending product review. Is it final now, or would it make sense to keep this in draft until it is, so the COPY-TBD notes don't land?
There was a problem hiding this comment.
Not final yet. I've asked product to review the copy. I'll update the text and remove the COPY-TBD markers before this merges, and it won't land with them in place.
| # Optional setup guide shown in the semantic layer and semantic view dialogs. | ||
| # Use an absolute HTTP(S) URL; unset deployments show no setup-guide link. | ||
| SEMANTIC_LAYER_DOCUMENTATION_URL: str | None = None |
There was a problem hiding this comment.
nit: this lands between DOCUMENTATION_URL and DOCUMENTATION_TEXT/DOCUMENTATION_ICON, which the navbar reads together, so DOCUMENTATION_TEXT now reads like the label for the setup guide. Would it make sense to move it below DOCUMENTATION_ICON?
There was a problem hiding this comment.
Moved in 537733d. SEMANTIC_LAYER_DOCUMENTATION_URL now sits below DOCUMENTATION_ICON, so the navbar documentation settings stay together.
…layer-setup-guidance
| /> | ||
| <SemanticLayerModal | ||
| show={!!slCurrentlyEditing} | ||
| onUpdated={refreshData} |
There was a problem hiding this comment.
If the Edit flow commits its PUT after Cancel, DatabaseList only gets the Connections refresh from onUpdated={refreshData} here, and the page tests cover that wiring only for create. Dropping this prop would leave the list stale after a late edit without failing any test, because the modal test passes its own callback. Could you add a DatabaseList test that opens Edit, defers the PUT, cancels, waits for the cancel-triggered refresh, resolves the PUT, and asserts a further Connections request?
There was a problem hiding this comment.
Added a layer updated after Cancel refreshes the Connections list again in DatabaseList.semanticLayerDelete.test.tsx. It opens Edit, holds the PUT pending, cancels, waits for the cancel-triggered Connections refresh, then resolves the PUT and verifies a further Connections request. Removing onUpdated={refreshData} makes the test fail at that final assertion; with the callback restored, all 19 tests in the file pass.
The semantic view and layer dialogs now explain empty and unavailable choices and provide a next step. Users who can create layers can open layer setup directly from an empty view picker; other users see guidance to contact an administrator.
SUMMARY
Distinguish loading, empty results and request failures, with an explicit retry. Display the selected provider's existing description during setup, and return to a refreshed view picker after layer creation or cancellation. Ignore late responses from closed dialogs.
Add optional
SEMANTIC_LAYER_DOCUMENTATION_URLconfiguration for a setup guide in both dialogs. It defaults toNone; invalid URLs and URLs containing credentials stay hidden. The main documentation menu is unchanged. This uses existing provider metadata and permissions.Provider descriptions use a runtime
t(description)lookup with source-language fallback. Dynamic provider text is not automatically extracted into the host translation catalog; this does not add a new metadata field.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: empty selectors offer no setup guidance. After: the empty view picker explains the next step, and the layer picker shows provider setup information or a distinct loading/error/empty state.
Screenshots and a real-browser accessibility walkthrough are still pending. This is a draft with placeholder copy pending product review.
TESTING INSTRUCTIONS
longdoublefailures in untouched dataframe/numeric-contract tests. No other failures.Manual checks with
SEMANTIC_LAYERSenabled:SEMANTIC_LAYER_DOCUMENTATION_URLto an absolute HTTP(S) setup-guide URL. Confirm it remains available during loading and opens in a new tab. Unset it and confirm the link is hidden.Copy is placeholder pending product review; no production documentation URL is supplied or activated.
ADDITIONAL INFORMATION
Companion provider-description change: https://github.com/preset-io/superset-shell/pull/5640. This host change remains compatible with providers that omit a description.
SEMANTIC_LAYERS(existing)