Skip to content

test(subdirectory): scaffold red/green tests for application root URL helpers#39925

Draft
sadpandajoe wants to merge 2 commits intomasterfrom
claude/subdirectory-helpers-tdd
Draft

test(subdirectory): scaffold red/green tests for application root URL helpers#39925
sadpandajoe wants to merge 2 commits intomasterfrom
claude/subdirectory-helpers-tdd

Conversation

@sadpandajoe
Copy link
Copy Markdown
Member

SUMMARY

Skeleton commit for a TDD-driven refactor that makes subdirectory deployments (SUPERSET_APP_ROOT) decision-free for frontend developers. After the full PR lands, every URL in the codebase is router-relative; the channel-3 helpers (openInNewTab, redirect, getShareableUrl, <AppLink>) wrap internally, and a backend response normaliser strips the application root on the way in so frontend code only ever speaks one shape.

This PR is intentionally just the scaffolding so the framing can be reviewed before ~30-60 call sites get migrated.

What's in this PR

Frameworks (implemented)

  • spec/helpers/withApplicationRoot.ts — fixture that sets application_root in the bootstrap data, resets the module cache, and tears down on completion. Replaces an inline ritual that pathUtils.test.ts repeats per test.
  • spec/helpers/sourceTreeScanner.ts — line-by-line regex scanner over the source tree with allow-list support. Reports workspace-relative file:line for each violation.

Stubs (throw "not implemented")

  • src/utils/navigationUtils.tsopenInNewTab, redirect, redirectReplace, getShareableUrl, AppLink. Each documents the channel rule it enforces. Existing navigateTo / navigateWithState stay as legacy multi-mode helpers and are scheduled for replacement.
  • packages/superset-ui-core/src/connection/normalizeBackendUrls.ts — conservative URL field normaliser. Ships a curated NORMALIZED_URL_FIELDS set (initially empty pending per-endpoint audit) and a documented NORMALIZER_EXCLUSIONS list explaining why fields like bug_report_url, thumbnail_url, and user_login_url are deliberately not normalised.

Layered tests (one example each; expanded in follow-up commits on this PR)

Layer What it covers Example file State on day 1
1 Per-helper unit behaviour under empty / single / nested app roots navigationUtils.test.ts (openInNewTab) 🔴 red — helper throws
2 Static invariant: no ensureAppRoot / makeUrl import outside navigationUtils.ts navigationUtils.invariants.test.ts 🟢 green — allow-list seeded with the 19 current call sites
3 Backend URL normaliser: positive strip + negative passthrough cases normalizeBackendUrls.test.ts 🔴 red — normaliser throws
4 SupersetClient × applicationRoot contract: root applied exactly once SupersetClientAppRootContract.test.ts 🟢 green — formalises existing behaviour
5 Per-site regression for SliceHeaderControls Cmd-click "Edit chart" SliceHeaderControls.subdirectory.test.tsx 🔴 red — index.tsx:266 calls window.open(props.exploreUrl) without prefixing

Why this approach

The codebase has three URL channels — React Router, SupersetClient, and browser-direct sinks (window.open, window.location.href, <a href>). Today each has its own convention for whether the caller pre-applies the application root, and bugs come from mixing them up:

  • Double-prefix: passing an ensureAppRoot(...) value into a router or SupersetClient channel that already applies the prefix internally.
  • Missing-prefix: passing a raw /path to a browser-direct sink that does not.

The fix makes router-relative the canonical shape — the one developers always write — and pushes the wrapping decision into helpers so individual call sites never need to think about it. Combined with the response normaliser, backend-supplied URLs (explore_url, permalink, etc.) speak the same shape, and ensureAppRoot / makeUrl become module-private inside navigationUtils.ts.

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. Final commit lands an ESLint rule that complements the static-invariant tests.

When the in-flight subdirectory branches (origin/subdirectory-bugs, origin/subdirectory-export) merge to master, this branch rebases onto them and adapts.

TESTING INSTRUCTIONS

Run the test suite from superset-frontend/:

npm run test -- spec/helpers/withApplicationRoot \
                src/utils/navigationUtils.invariants.test.ts \
                packages/superset-ui-core/test/connection/SupersetClientAppRootContract.test.ts

Layers 2 and 4 should pass on day one. Layers 1, 3, and 5 are intentionally red — they describe the behaviour the green commit will deliver.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration
  • Introduces new feature or API
  • Removes existing feature or API

🤖 Generated with Claude Code

sadpandajoe and others added 2 commits May 6, 2026 17:30
… helpers

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>
Pure formatting follow-up to 13f56f7. No behaviour change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant