From 7aee4fb7bdbf21bc1bf788c5caa3514083ee0dc3 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 7 May 2026 10:38:23 -0700 Subject: [PATCH] fix(subdirectory): unblock CI on subdirectory-helpers PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/connection/normalizeBackendUrls.ts | 36 +++++++++---------- .../SliceHeaderControls.subdirectory.test.tsx | 13 ++++--- .../src/utils/navigationUtils.ts | 6 +++- 3 files changed, 31 insertions(+), 24 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 8375955d653..f717553ec59 100644 --- a/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts +++ b/superset-frontend/packages/superset-ui-core/src/connection/normalizeBackendUrls.ts @@ -159,27 +159,15 @@ export function normalizeBackendUrlString( 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); + const out: unknown[] = []; 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; + out.push(next); } return changed ? out : value; } @@ -190,10 +178,7 @@ function walk(value: unknown, root: string): unknown { for (const key of Object.keys(value)) { const fieldValue = value[key]; let nextValue: unknown; - if ( - NORMALIZED_URL_FIELDS.has(key) && - typeof fieldValue === 'string' - ) { + if (NORMALIZED_URL_FIELDS.has(key) && typeof fieldValue === 'string') { nextValue = normalizeBackendUrlString(fieldValue, { applicationRoot: root, }); @@ -208,3 +193,18 @@ function walk(value: unknown, root: string): unknown { 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; +} 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 e24356f1b65..9a82d5bad64 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.subdirectory.test.tsx @@ -44,10 +44,13 @@ import SliceHeaderControls, { SliceHeaderControlsProps } from '.'; // goes green. // ============================================================================= -const APPLICATION_ROOT_MOCK = jest.fn(() => ''); +// Variable name must start with `mock` so Jest's hoisted `jest.mock()` +// factory can reference it. Renaming this prefix breaks the suite with +// "module factory is not allowed to reference any out-of-scope variables". +const mockApplicationRoot = jest.fn(() => ''); jest.mock('src/utils/getBootstrapData', () => ({ - applicationRoot: () => APPLICATION_ROOT_MOCK(), + applicationRoot: () => mockApplicationRoot(), })); const SLICE_ID = 371; @@ -124,7 +127,7 @@ describe('SliceHeaderControls — Cmd-click "Edit chart" under subdirectory depl let openSpy: jest.SpyInstance; beforeEach(() => { - APPLICATION_ROOT_MOCK.mockReturnValue(''); + mockApplicationRoot.mockReturnValue(''); openSpy = jest.spyOn(window, 'open').mockImplementation(() => null); }); @@ -133,7 +136,7 @@ describe('SliceHeaderControls — Cmd-click "Edit chart" under subdirectory depl }); test('opens the unprefixed exploreUrl when application root is empty', async () => { - APPLICATION_ROOT_MOCK.mockReturnValue(''); + mockApplicationRoot.mockReturnValue(''); renderControls(); userEvent.click(screen.getByRole('button', { name: 'More Options' })); @@ -148,7 +151,7 @@ describe('SliceHeaderControls — Cmd-click "Edit chart" under subdirectory depl }); test('opens the prefixed exploreUrl when deployed under a subdirectory', async () => { - APPLICATION_ROOT_MOCK.mockReturnValue('/superset'); + mockApplicationRoot.mockReturnValue('/superset'); renderControls(); userEvent.click(screen.getByRole('button', { name: 'More Options' })); diff --git a/superset-frontend/src/utils/navigationUtils.ts b/superset-frontend/src/utils/navigationUtils.ts index 657686de6c5..4faa7f2709c 100644 --- a/superset-frontend/src/utils/navigationUtils.ts +++ b/superset-frontend/src/utils/navigationUtils.ts @@ -16,7 +16,11 @@ * specific language governing permissions and limitations * under the License. */ -import { createElement, type AnchorHTMLAttributes, type ReactElement } from 'react'; +import { + createElement, + type AnchorHTMLAttributes, + type ReactElement, +} from 'react'; import { ensureAppRoot } from './pathUtils'; // =============================================================================