Three concrete failures from the first CI run on 0e98228aa8, addressed:
1. Jest hoisting (sharded-jest-tests shard 3): the Layer 5 mock factory
referenced `APPLICATION_ROOT_MOCK` from outer scope. Jest hoists
`jest.mock()` above all top-level statements, so the variable was
undefined when the factory ran, producing
"module factory of jest.mock() is not allowed to reference any
out-of-scope variables". Renamed to `mockApplicationRoot` — Jest
carves out an exception for variables prefixed with `mock`. Comment
added so the next contributor doesn't lose ten minutes to the
rename rule.
2. oxlint (pre-commit): two errors in normalizeBackendUrls.ts.
- "walk was used before it was defined": moved the `walk` helper
above its caller `normalizeBackendUrls`. The hoisting was valid JS
but oxlint enforces textual order.
- "Do not use `new Array(singleArgument)`": replaced
`new Array(value.length)` with a `[]` + push pattern. Same
allocation cost, no surprise sparse-array semantics.
3. prettier (pre-commit): line-wrap the React type imports in
navigationUtils.ts and tighten the conditional layout in
normalizeBackendUrls.ts to match prettier's expected output.
Outstanding: the `playwright-tests (chromium, /app/prefix)` failures
look like infrastructure flakiness — the failing tests (bulk export
dashboards, create dataset wizard, duplicate dataset) all hit
`page.goto: Test timeout of 30000ms exceeded` and
`apiRequestContext.post: socket hang up`, and don't exercise the one
production code path this PR touches (SliceHeaderControls Cmd-click).
Watching the next run before treating it as a real failure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Skeleton commit for the subdirectory deployment refactor. Adds the test
framework and one example test per layer; the helpers themselves are
stubbed so the suite is meaningfully red until the green commit lands.
Frameworks
- spec/helpers/withApplicationRoot.ts: fixture that rewrites #app data
and resets the module cache so getBootstrapData() returns the requested
application root inside the callback. Replaces the inline ritual that
pathUtils.test.ts currently repeats per test.
- spec/helpers/sourceTreeScanner.ts: line-by-line regex scanner over the
source tree with allow-list support. Backs the static-invariant tests
in Layer 2 with workspace-relative file:line locations on failure.
Stubs
- src/utils/navigationUtils.ts: openInNewTab, redirect, redirectReplace,
getShareableUrl, AppLink. Each throws a "not implemented" error with a
doc comment describing the channel rule it enforces. Existing
navigateTo / navigateWithState are kept untouched and called out as
legacy multi-mode helpers scheduled for replacement.
- packages/superset-ui-core/src/connection/normalizeBackendUrls.ts:
conservative URL field normaliser. Ships the curated NORMALIZED_URL_FIELDS
set (initially empty pending per-endpoint audit) and a documented
NORMALIZER_EXCLUSIONS list explaining why bug_report_url, thumbnail_url,
user_login_url, etc. are deliberately not normalised.
Layered tests (one example each; full suite expands per layer in
subsequent commits on this PR)
- Layer 1 unit: navigationUtils.test.ts exercises openInNewTab under
empty / single / nested application roots, plus absolute-URL and
mailto passthrough. Red until the helper is implemented.
- Layer 2 invariant: navigationUtils.invariants.test.ts asserts that
ensureAppRoot / makeUrl are not imported outside navigationUtils.ts.
Allow-list seeded with the 19 current call sites so the test is GREEN
on day one; migration commits delete entries from the list.
- Layer 3 normaliser: normalizeBackendUrls.test.ts pairs a positive
strip case with negative passthrough cases (non-allow-listed field,
absolute URL, similar-but-different prefix segment, empty root).
Red until the normaliser is implemented.
- Layer 4 contract: SupersetClientAppRootContract.test.ts pins the
channel-2 invariant (root applied exactly once, never doubled).
Documents the double-prefix symptom in a regression assertion.
- Layer 5 regression: SliceHeaderControls.subdirectory.test.tsx
asserts Cmd-click "Edit chart" opens a prefixed URL when the app
is deployed under a subdirectory. Red until index.tsx:266 is
migrated to openInNewTab.
Strategy: each subsequent commit on this PR fans out one layer to its
full coverage and migrates the corresponding call sites, shrinking the
Layer 2 allow-list in lockstep.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
