From cfaeb114686e5ef72d09e8a9df8ec0b60b5d7e02 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 7 May 2026 22:08:12 -0700 Subject: [PATCH] fix(subdirectory): reinstate invariants scan, harden scanner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shard-6 hang reproduced on master independently of this PR — earlier diagnostic edits (skipping the invariants scan) are no longer needed. Reinstates the static-invariant scan in `navigationUtils.invariants.test.ts` and keeps the previously seeded PATH_UTILS_IMPORT_ALLOWLIST so the suite is GREEN today and shrinks as each migration commit lands. Also hoists the regex compile out of the per-line loop in `scanSource`. With no `g` flag, `RegExp.exec()` ignores `lastIndex`, so recompiling per line was wasted allocation across ~1.5M lines workspace- wide. If the source pattern includes `g`, the helper now strips it once at the top of the file rather than relying on per-line construction. Adds `jest.setTimeout(20000)` to `navigationUtils.test.ts` as a defence-in-depth safety net — any future hang surfaces a Jest timeout error with the test name rather than running for the workflow's six-hour wallclock limit. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../spec/helpers/sourceTreeScanner.ts | 10 +++-- .../utils/navigationUtils.invariants.test.ts | 43 ++++++++++++------- .../src/utils/navigationUtils.test.ts | 4 ++ 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/superset-frontend/spec/helpers/sourceTreeScanner.ts b/superset-frontend/spec/helpers/sourceTreeScanner.ts index ebc143d8ed5..9f7fb844fb6 100644 --- a/superset-frontend/spec/helpers/sourceTreeScanner.ts +++ b/superset-frontend/spec/helpers/sourceTreeScanner.ts @@ -173,11 +173,15 @@ export function scanSource(options: ScanOptions): ScanHit[] { const contents = readFileSync(absoluteFile, 'utf8'); const lines = contents.split('\n'); + // Reuse a single regex per file. Without the `g` flag, RegExp's + // `lastIndex` is ignored on `.exec()` — recompiling per-line was + // wasted allocation across ~1.5M lines workspace-wide. + const lineRegex = pattern.flags.includes('g') + ? new RegExp(pattern.source, pattern.flags.replace('g', '')) + : pattern; + for (let index = 0; index < lines.length; index += 1) { const lineText = lines[index]; - // Re-create the regex per line so the global flag's lastIndex doesn't - // bleed across iterations. - const lineRegex = new RegExp(pattern.source, pattern.flags); const match = lineRegex.exec(lineText); if (match) { hits.push({ diff --git a/superset-frontend/src/utils/navigationUtils.invariants.test.ts b/superset-frontend/src/utils/navigationUtils.invariants.test.ts index 79736f45499..f31eff59c91 100644 --- a/superset-frontend/src/utils/navigationUtils.invariants.test.ts +++ b/superset-frontend/src/utils/navigationUtils.invariants.test.ts @@ -16,8 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -// `scanSource` and `expectNoHits` are imported by the active scan when it is -// reinstated (see comment block below). +import { expectNoHits, scanSource } from 'spec/helpers/sourceTreeScanner'; // ============================================================================= // Layer 2 example: structural invariant @@ -65,23 +64,35 @@ const PATH_UTILS_IMPORT_ALLOWLIST: string[] = [ 'src/views/CRUD/hooks.ts', ]; -// The full scan (`no file outside navigationUtils.ts imports ensureAppRoot -// or makeUrl from pathUtils`) is temporarily removed while the CI shard-hang -// root cause is being isolated — the scanner walks ~1591 source files and a -// recent shard-6 run hung for 3+ hours without logging PASS for any of our -// new test files. We restore the scan in a follow-up commit once we know -// whether `scanSource` was the cause (we suspect per-line `new RegExp(...)` -// allocation under sync recursion). Keeping the file present with a sentinel -// so the import path stays valid for the helpers + future scans. -// -// The static-invariant pattern is fully implemented in `sourceTreeScanner.ts` -// and `scanSource` is exported for re-use. Reinstatement plan: -// 1. Hoist the regex compile out of the per-line loop in scanSource -// 2. Add a per-test timing log so any future hang surfaces a culprit -// 3. Re-add the scan with the seeded PATH_UTILS_IMPORT_ALLOWLIST test('PATH_UTILS_IMPORT_ALLOWLIST entries are workspace-relative paths', () => { for (const entry of PATH_UTILS_IMPORT_ALLOWLIST) { expect(entry.startsWith('/')).toBe(false); expect(entry.includes('\\')).toBe(false); } }); + +test('no file outside navigationUtils.ts imports ensureAppRoot or makeUrl from pathUtils', () => { + const hits = scanSource({ + pattern: /\b(?:ensureAppRoot|makeUrl)\b/, + allowlist: [ + // The two modules that are *allowed* to know about path prefixing. + // `pathUtils.ts` defines the helpers; `navigationUtils.ts` is the only + // re-export sanctioned for the rest of the codebase to consume. + 'src/utils/pathUtils.ts', + 'src/utils/navigationUtils.ts', + // SupersetClient has its own `appRoot` configuration path — it does + // not import from `pathUtils`. Excluded so a future occurrence of + // the word `appRoot` in connection internals does not trip this scan. + 'packages/superset-ui-core/src/connection/SupersetClientClass.ts', + 'packages/superset-ui-core/src/connection/normalizeBackendUrls.ts', + ...PATH_UTILS_IMPORT_ALLOWLIST, + ], + }); + + expectNoHits( + hits, + 'Found imports of ensureAppRoot / makeUrl outside navigationUtils.ts. ' + + 'Use the focused helpers (openInNewTab, redirect, getShareableUrl, AppLink) ' + + 'instead, or add the file to PATH_UTILS_IMPORT_ALLOWLIST with justification.', + ); +}); diff --git a/superset-frontend/src/utils/navigationUtils.test.ts b/superset-frontend/src/utils/navigationUtils.test.ts index 82c776108de..4bb9d5f810d 100644 --- a/superset-frontend/src/utils/navigationUtils.test.ts +++ b/superset-frontend/src/utils/navigationUtils.test.ts @@ -18,6 +18,10 @@ */ import { withApplicationRoot } from 'spec/helpers/withApplicationRoot'; +// Failsafe so a future hang surfaces a Jest timeout error with the test name +// rather than running for the workflow's 6-hour wallclock limit. +jest.setTimeout(20000); + // ============================================================================= // Layer 1 example: openInNewTab // =============================================================================