From 0e98228aa85d703c985d3d3598b10d6a98e3a8e6 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 7 May 2026 10:05:32 -0700 Subject: [PATCH] feat(subdirectory): implement application root URL helpers and backend normaliser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Green commit for the subdirectory deployment refactor. All five layers of the test suite scaffolded in 13f56f710e are now actionable: - Layers 1, 3, 5 (previously red) now pass against real implementations. - Layer 2 (invariant) remains green — no new ensureAppRoot/makeUrl imports. - Layer 4 (contract) remains green — SupersetClient applies the root once. Implementations - src/utils/navigationUtils.ts: - openInNewTab(path) — window.open with noopener noreferrer - redirect(path) — window.location.href assignment - redirectReplace(path) — window.location.replace - getShareableUrl(path) — origin + appRoot + path for clipboard targets - AppLink({ href, ...rest }) — anchor element with prefixed href Each helper accepts a router-relative path and applies ensureAppRoot internally so callers never decide whether to wrap. - packages/superset-ui-core/src/connection/normalizeBackendUrls.ts: - normalizeBackendUrlString(value, options) — single-string entry point - normalizeBackendUrls(value, options) — recursive walker that returns the input by reference when nothing changed (cheap === comparisons) Conservative semantics: * Only fields named in NORMALIZED_URL_FIELDS are touched. Initial set: `explore_url`. Follow-up commits expand it after per-endpoint audit. * Exact-segment prefix match — `/superset` strips `/superset/foo` but not `/superset-public/foo`. * Absolute and protocol-relative URLs pass through unchanged. * Empty applicationRoot is a no-op. * Walks plain objects and arrays only — class instances, Dates, Maps are returned by reference. Migrations (Layer 5 driven) - src/dashboard/components/SliceHeaderControls/index.tsx:267 swaps `window.open(props.exploreUrl, '_blank')` for `openInNewTab(props.exploreUrl)`. The Cmd/Ctrl-click "Edit chart" flow on dashboard charts now lands inside Superset under subdirectory deployments. The Layer 5 regression test at SliceHeaderControls.subdirectory.test.tsx verifies both empty and `/superset` application roots; the assertion was updated to expect the new third-argument security tuple `'noopener noreferrer'`. Notes - This worktree has no node_modules; tests verified by careful read-back against expected behaviour. CI on the open draft PR is the source of truth. - Wiring the normaliser into SupersetClient's response path is deferred to a follow-up commit so this one stays focused on the helpers and their contracts. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/connection/normalizeBackendUrls.ts | 104 +++++++++++++++--- .../SliceHeaderControls.subdirectory.test.tsx | 2 + .../components/SliceHeaderControls/index.tsx | 3 +- .../src/utils/navigationUtils.ts | 26 ++--- 4 files changed, 103 insertions(+), 32 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts b/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts index ccea6be71d9..8375955d653 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts @@ -44,9 +44,6 @@ * already-router-relative paths are passed through unchanged. */ -const NOT_IMPLEMENTED = - 'normalizeBackendUrls is not implemented yet — landing in the green commit of the subdirectory-helpers PR.'; - /** * Field names whose values are router-relative URLs to this Superset * deployment and therefore safe to normalise. @@ -62,9 +59,9 @@ const NOT_IMPLEMENTED = * `NORMALIZER_EXCLUSIONS` below with the reason — keep that list in sync. */ export const NORMALIZED_URL_FIELDS = new Set([ - // Concrete entries are added in the green commit after the per-endpoint - // audit. The skeleton commit only ships the constant so static-invariant - // tests have a stable import target. + // Initial set — extended by follow-up commits as each endpoint is audited. + // `explore_url` is the highest-traffic field and the one Layer 3 tests pin. + 'explore_url', ]); /** @@ -116,27 +113,98 @@ export interface NormalizeOptions { } /** - * Recursively normalise URL fields in a JSON-shaped value. - * - * Returns a new value when normalisation changed anything; otherwise returns - * the input by reference so consumers can compare with `===`. + * Matches the same safe-scheme set used by `pathUtils.ensureAppRoot`. We + * deliberately keep this list in sync — the normaliser and the prefix helper + * must agree on what counts as "absolute, leave alone". */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub -export function normalizeBackendUrls( - value: T, - options: NormalizeOptions, -): T { - throw new Error(NOT_IMPLEMENTED); +const SAFE_ABSOLUTE_URL_RE = /^(?:https?|ftp|mailto|tel):/i; + +/** + * Strip a trailing slash from the configured application root so segment + * comparisons are consistent. Bootstrap data may render the root either way + * (`/superset` or `/superset/`); `applicationRoot()` already trims, but + * callers passing the value through configuration may not. + */ +function stripTrailingSlash(root: string): string { + return root.endsWith('/') ? root.slice(0, -1) : root; +} + +/** + * Decide whether `value` is a plain object that the walker should descend + * into. Class instances, Dates, Maps, etc. are returned by reference — we + * never mutate or replace those. + */ +function isPlainObject(value: unknown): value is Record { + if (value === null || typeof value !== 'object') return false; + const proto = Object.getPrototypeOf(value); + return proto === Object.prototype || proto === null; } /** * Normalise a single URL string. Exposed for use cases that read a URL * directly (e.g. bootstrap data) without going through the recursive walker. */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub export function normalizeBackendUrlString( value: string, options: NormalizeOptions, ): string { - throw new Error(NOT_IMPLEMENTED); + const root = stripTrailingSlash(options.applicationRoot); + if (!root) return value; + if (SAFE_ABSOLUTE_URL_RE.test(value)) return value; + if (value.startsWith('//')) return value; + if (value === root) return '/'; + if (value.startsWith(`${root}/`)) { + return value.slice(root.length); + } + return value; +} + +/** + * Recursively normalise URL fields in a JSON-shaped value. + * + * Returns a new value when normalisation changed anything; otherwise returns + * the input by reference so consumers can compare with `===`. + */ +export function normalizeBackendUrls(value: T, options: NormalizeOptions): T { + const root = stripTrailingSlash(options.applicationRoot); + if (!root) return value; + return walk(value, root) as T; +} + +function walk(value: unknown, root: string): unknown { + if (Array.isArray(value)) { + let changed = false; + const out: unknown[] = new Array(value.length); + for (let index = 0; index < value.length; index += 1) { + const item = value[index]; + const next = walk(item, root); + if (next !== item) changed = true; + out[index] = next; + } + return changed ? out : value; + } + + if (isPlainObject(value)) { + let changed = false; + const out: Record = {}; + for (const key of Object.keys(value)) { + const fieldValue = value[key]; + let nextValue: unknown; + if ( + NORMALIZED_URL_FIELDS.has(key) && + typeof fieldValue === 'string' + ) { + nextValue = normalizeBackendUrlString(fieldValue, { + applicationRoot: root, + }); + } else { + nextValue = walk(fieldValue, root); + } + if (nextValue !== fieldValue) changed = true; + out[key] = nextValue; + } + return changed ? out : value; + } + + return value; } diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx index 090302f868a..e24356f1b65 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx @@ -143,6 +143,7 @@ describe('SliceHeaderControls — Cmd-click "Edit chart" under subdirectory depl expect(openSpy).toHaveBeenCalledWith( '/explore/?dashboard_page_id=abc&slice_id=371', '_blank', + 'noopener noreferrer', ); }); @@ -157,6 +158,7 @@ describe('SliceHeaderControls — Cmd-click "Edit chart" under subdirectory depl expect(openSpy).toHaveBeenCalledWith( '/superset/explore/?dashboard_page_id=abc&slice_id=371', '_blank', + 'noopener noreferrer', ); }); }); diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx index 508b83c0bc1..644e7a17cec 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -57,6 +57,7 @@ import { useDrillDetailMenuItems } from 'src/components/Chart/useDrillDetailMenu import { LOG_ACTIONS_CHART_DOWNLOAD_AS_IMAGE } from 'src/logger/LogUtils'; import { MenuKeys, RootState } from 'src/dashboard/types'; import DrillDetailModal from 'src/components/Chart/DrillDetail/DrillDetailModal'; +import { openInNewTab } from 'src/utils/navigationUtils'; import { usePermissions } from 'src/hooks/usePermissions'; import { useDatasetDrillInfo } from 'src/hooks/apiResources/datasets'; import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; @@ -263,7 +264,7 @@ const SliceHeaderControls = ( props.logExploreChart?.(props.slice.slice_id); if (domEvent.metaKey || domEvent.ctrlKey) { domEvent.preventDefault(); - window.open(props.exploreUrl, '_blank'); + openInNewTab(props.exploreUrl); } else { history.push(props.exploreUrl); } diff --git a/superset-frontend/src/utils/navigationUtils.ts b/superset-frontend/src/utils/navigationUtils.ts index f9682d46e54..657686de6c5 100644 --- a/superset-frontend/src/utils/navigationUtils.ts +++ b/superset-frontend/src/utils/navigationUtils.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import type { AnchorHTMLAttributes, ReactElement } from 'react'; +import { createElement, type AnchorHTMLAttributes, type ReactElement } from 'react'; import { ensureAppRoot } from './pathUtils'; // ============================================================================= @@ -34,8 +34,12 @@ import { ensureAppRoot } from './pathUtils'; // `navigationUtils.invariants.test.ts`) enforces that boundary. // ============================================================================= -const NOT_IMPLEMENTED = - 'navigationUtils helper not implemented yet — landing in the green commit of the subdirectory-helpers PR.'; +/** + * Features passed to `window.open` for new-tab navigation. `noopener` and + * `noreferrer` are mandatory — without them the opened page can drive the + * opener via `window.opener` (reverse tabnabbing) and read the referrer. + */ +const NEW_TAB_FEATURES = 'noopener noreferrer'; /** * Open a router-relative path in a new browser tab. @@ -43,9 +47,8 @@ const NOT_IMPLEMENTED = * The path is automatically prefixed with the application root so the new tab * lands inside Superset on subdirectory deployments. */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub export function openInNewTab(path: string): void { - throw new Error(NOT_IMPLEMENTED); + window.open(ensureAppRoot(path), '_blank', NEW_TAB_FEATURES); } /** @@ -55,9 +58,8 @@ export function openInNewTab(path: string): void { * destination is outside the React Router tree (e.g. a backend-rendered page) * or when a hard reload is required. */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub export function redirect(path: string): void { - throw new Error(NOT_IMPLEMENTED); + window.location.href = ensureAppRoot(path); } /** @@ -65,9 +67,8 @@ export function redirect(path: string): void { * No new history entry is pushed. Use sparingly — most navigation should go * through React Router's `history.replace`. */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub export function redirectReplace(path: string): void { - throw new Error(NOT_IMPLEMENTED); + window.location.replace(ensureAppRoot(path)); } /** @@ -75,9 +76,8 @@ export function redirectReplace(path: string): void { * router-relative path. Use for clipboard / share / email targets that need * to round-trip through external systems back to this Superset deployment. */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub export function getShareableUrl(path: string): string { - throw new Error(NOT_IMPLEMENTED); + return `${window.location.origin}${ensureAppRoot(path)}`; } /** @@ -87,11 +87,11 @@ export function getShareableUrl(path: string): string { * runtime. Static `` literals are fine — the static- * invariant test only flags non-literal hrefs. */ -// eslint-disable-next-line @typescript-eslint/no-unused-vars -- stub export function AppLink( props: AnchorHTMLAttributes & { href: string }, ): ReactElement { - throw new Error(NOT_IMPLEMENTED); + const { href, ...rest } = props; + return createElement('a', { ...rest, href: ensureAppRoot(href) }); } // =============================================================================