Skip to content

fix(validation): consolidate project checks#600

Open
JustYannicc wants to merge 14 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-validation-final
Open

fix(validation): consolidate project checks#600
JustYannicc wants to merge 14 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-validation-final

Conversation

@JustYannicc

Copy link
Copy Markdown
Collaborator

What changed

  • Consolidates the validation/project-checks stack onto one final branch without pnpm artifacts or generated locks.
  • Keeps npm validation authoritative via package-manager parity checks and routes npm test through the non-perf test runner while npm run test:perf:ci owns browser/file/root perf harnesses.
  • Adds explicit perf-harness test access exports for browser search and file search internals so the consolidated perf checks run from a clean npm install.
  • Slightly relaxes the 50k browser-search perf CI guard to avoid boundary-only failures while keeping the indexed path budgeted.

Why

The superseded validation PRs overlapped and left local validation vulnerable to duplicate perf execution, missing harness exports, and overly tight perf thresholds on this machine. This branch keeps the consolidated project checks fast, npm-based, and reproducible while preserving renderer typecheck, i18n, build, and perf coverage.

Compatibility impact

Validation/tooling only. Runtime search behavior is unchanged; the new exports are test harness accessors for existing internal helpers. npm test now runs non-perf Node tests, with perf coverage retained in npm run test:perf:ci.

How tested

  • /usr/bin/env npm_config_ignore_scripts=true /opt/homebrew/opt/node@22/bin/npm ci
  • /opt/homebrew/opt/node@22/bin/npm test
  • /opt/homebrew/opt/node@22/bin/npm run test:perf:ci
  • /opt/homebrew/opt/node@22/bin/npm run check:i18n
  • /opt/homebrew/opt/node@22/bin/npm run build:main
  • /opt/homebrew/opt/node@22/bin/npm run typecheck:renderer
  • /opt/homebrew/opt/node@22/bin/npm run build:renderer
  • git diff --check

Performance evidence

  • Browser perf CI 50k dataset: index 20.47ms / 7000ms, ranked max avg 265.35ms / 300ms, ordered max avg 262.35ms / 300ms, ranked event-loop delay 7601.26ms / 8000ms, ordered event-loop delay 7535.74ms / 8000ms.
  • File perf CI large dataset: 33,964 indexed entries, initial index 806.34ms / 60000ms, normal query p95 5.207ms / 3000ms, path query p95 27.405ms / 3000ms, event-loop p95 250.324ms / 5000ms.
  • Root search perf CI: optimized median 591.10ms / 900ms, indexed median 83.11ms / 350ms, compile median 61.87ms / 500ms, indexed speedup 22.82x.

Stack validation

  • Final branch: JustYannicc:codex/consolidate-validation-final, commit 0635377 on top of the previous consolidation output 8d5dc12.
  • Renderer TypeScript is repo-wide clean via npm run typecheck:renderer.
  • Main TypeScript/build is clean via npm run build:main; renderer production build is clean apart from existing Vite CJS/Browserslist warnings.
  • Codex LSP: TypeScript server installed; diagnostics reported no errors for src/main/file-search-index.ts, src/renderer/src/hooks/useBrowserSearch.ts, and scripts/test-browser-search-performance.mjs; renderer directory scan reported 0 errors across the LSP 50-file cap. Biome was configured but not installed for package.json, so JSON LSP diagnostics were unavailable.
  • No pnpm-lock.yaml, pnpm-workspace.yaml, .pnpm-store, coordinator notes, prompt/playbook markdown, or generated lock/artifact files are included.

Replaces

Intentionally leaves #532 open because its older bundled TypeScript import-helper/test consolidation is not fully represented in this final branch.

@shobhit99

Copy link
Copy Markdown
Contributor

This PR is nominally a fix(validation) "consolidate project checks", but it's a large grab-bag (46 files, +2490/-138): a new project-checks.yml CI workflow, a package-manager parity gate, three perf harnesses, an i18n strictness change, native-build incremental caching, dependency reshuffling, plus a root-search ranking hot-path refactor and ~20 one-line type-safety fixes. Per the description it explicitly does not supersede #532 (it replaces #578/#589/#591), so #532 is correctly left alone.

Strengths

  • i18n change is well-executed and verified. check-i18n.mjs now defaults every non-en locale to strict (fail on any missing/extra/invalid key), which enforces the repo's stated i18n parity policy. Crucially it also brings the one lagging locale into line: base it.json was missing exactly 14 keys, and the PR adds exactly those 14 (stt.vocabulary.* ×4, appSearchScope.* ×10). I verified post-PR all 9 locales are at 0 missing / 0 extra, so the now-strict npm run check:i18n passes.
  • Fixes real drift/bugs: package-lock 1.0.25-PMH1.0.26 to match package.json (they were out of sync on main) and adds the packageManager field the parity check expects; CameraExtension.tsx setCapturePreviewClearTimerRef.currentcapturePreviewClearTimerRef.current (the old identifier would throw a TypeError when the 5.3s timer fired); StoreTab shortcut null-key guard; Toast.Style namespace enumtype (removes the value-level collision with the static Style member while keeping Toast.Style.Animated working at runtime); phosphor import via package name instead of a hardcoded ../../../../node_modules/... path.
  • Package-manager parity check + its unit tests are solid, with sensible pnpm-runtime/lockfile guards. Native-build stamping is CI-safe (the Linux CI never builds native).
  • The ranking refactor ships an equivalence test.

Concerns / potential bugs

  1. The "hot path" refactor isn't wired into the app and adds wasted work (contradicts the PR's stated purpose). useLauncherCommandModel.ts:529 still calls the non-indexed rankCommands(...), which internally does createRootCommandScoreIndex(...) — re-precompiling every field of every command — on every keystroke. The cached fast-path rankCommandsWithIndex (the thing the 22x-speedup perf report measures) is used only by scripts/test-root-search-perf.mjs, never by the renderer. Worse: rankCommandsWithIndex now computes matchKind/matchScore via a second scoring pass (scoringFields, alias 1.08 / keyword 0.68), but the consumer destructures only { command } at useLauncherCommandModel.ts:530 and then re-derives those same values with its own scoreRootSearchFields(...) call at lines 532-537. So the real hot path now runs an extra scoring pass per matched command whose output is discarded — net neutral-to-slightly-slower for users, with the advertised speedup unrealized. To actually deliver it: memoize the index across keystrokes, switch the consumer to rankCommandsWithIndex, and drop the redundant second scoreRootSearchFields.
    • Minor: the equivalence test's signature() sorts before comparing, so it checks the set of (id, matchKind, matchScore) but not sort order. Order looks preserved by inspection (sort score uses the same rankingFields weights as the old code), but an ordering regression wouldn't be caught.
  2. The parity gate can false-positive on routine local commands. check-package-manager-parity.mjs runs as a pre-hook for build:main, typecheck:renderer, build:renderer, check:i18n, and test, and collectInstalledVersionFailures hard-fails if any installed node_modules version differs from package-lock.json. On a dev machine with a slightly stale/deduped tree (common after an iterative npm install x), everyday npm run typecheck:renderer/build:main will now abort with a parity error instead of doing the work. Also EXPECTED_PACKAGE_MANAGER = "npm@11.12.1" is hardcoded, so any later bump to the packageManager field breaks all gated commands until the constant is updated. Consider scoping the strict installed-version check to CI (env flag) and reading the expected manager from package.json.
  3. Absolute perf budgets on shared CI runners are flake-prone. project-checks.yml runs npm run test:perf:ci on ubuntu-latest with wall-clock thresholds (index 7000ms, query avg 300ms, event-loop 8000ms, root median 900ms). Shared runners have highly variable CPU, so these will intermittently red-build unrelated PRs. Consider making perf informational/non-blocking or asserting only relative speedups.

Suggestions (non-blocking)

  • liquid-glass-react (imported by src/renderer/src/components/LiquidGlassSurface.tsx) is still absent from package.json/lock and not installed; the new liquid-glass-react.d.ts only satisfies tsc. This builds today only because LiquidGlassSurface is unimported (dead code) so Vite never resolves it — if it's ever wired in, build:renderer breaks in CI. The real fix is declaring the dep or removing the component.
  • The title undersells the scope: a fix(validation) PR also ships product-code behavior changes (the ranking path). Worth a heads-up for future consolidation PRs.

Verdict: 🟡 Approve with minor comments — the CI/i18n/build/dependency machinery is sound, nothing is a correctness regression, and CI should stay green (i18n parity verified, version drift fixed). But the centerpiece perf refactor doesn't deliver its claimed hot-path win (not wired into the renderer + a redundant scoring pass), and the parity gate's scope can break routine local commands — both worth addressing given perf consolidation is the PR's stated goal.

🤖 Automated review by Claude Code (Opus 4.8).

@JustYannicc

JustYannicc commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the actionable review feedback from the July 8 maintainer comment.

What changed:

  • Wired the root-search command hot path into the real renderer flow: useLauncherCommandModel now memoizes a createRootCommandScoreIndex(...) across query changes, calls rankCommandsWithIndex(...), and reuses the returned matchKind/matchScore instead of doing a second discarded scoring pass.
  • Added blocking order coverage for indexed command ranking in scripts/test-root-command-ranking.mjs, and tightened the perf harness equivalence check so it compares ordered signatures instead of sorted set membership.
  • Updated package-manager parity so the expected npm manager is read from package.json, while installed node_modules version checks run only in CI or with SUPERCMD_STRICT_PACKAGE_MANAGER_PARITY=1. Local lifecycle hooks still catch lock/spec drift and pnpm usage without false-positiveing on stale local installs.
  • Made the shared-runner perf workflow step informational with continue-on-error: true, and removed unused absolute root-search budget env vars from test:perf:ci while keeping the relative indexed-speedup gate.
  • Fixed the local npm/npx compatibility wrappers so npm-shaped commands run through pnpm, including npm-declared projects.

Verification:

  • npm ci --ignore-scripts
  • npm exec -- node -e 'console.log("npm-exec-through-pnpm-ok")'
  • npx --no-install node -e 'console.log("npx-no-install-ok")'
  • node --test scripts/test-package-manager-parity.mjs scripts/test-renderer-crash-recovery.mjs scripts/test-renderer-error-boundary.mjs scripts/test-root-command-ranking.mjs scripts/test-root-search-ranking.mjs ✅ 23 passed
  • npm exec -- tsc -p tsconfig.main.json --noEmit
  • git diff --check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants