sadpandajoe opened a new pull request, #39925:
URL: https://github.com/apache/superset/pull/39925
### 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.ts` — `openInNewTab`, `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/`:
```bash
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](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
