Skip to content

fix(validation): restore project checks#532

Open
JustYannicc wants to merge 9 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-validation-checks
Open

fix(validation): restore project checks#532
JustYannicc wants to merge 9 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-validation-checks

Conversation

@JustYannicc

@JustYannicc JustYannicc commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

What changed

Consolidates the validation/build fixes from these source PRs into one upstream PR:

The combined change adds a root Project Checks workflow, upgrades the test TypeScript import helper, makes the renderer project typecheck clean, skips Claude review on fork PRs that cannot receive OIDC/secrets, restores strict i18n parity with the missing Italian keys, and approves the native pnpm build scripts needed for local validation.

Why

The individual validation PRs were fixing related gaps in the project safety net: missing root project checks, unreliable TypeScript import coverage in tests, renderer typecheck noise, fork PR review-check failures, non-strict locale drift, and pnpm 11 native-build approvals. Keeping them together gives upstream one coherent PR that restores the expected validation path for follow-on work.

Compatibility impact

Most changes are CI, test, type, or install metadata only. The runtime-facing changes are behavior-preserving type alignments and added Italian translation strings. Same-repository PRs still run Claude review; fork PRs skip that job because the required credentials are not available to pull_request runs. npm installs remain unchanged, while pnpm installs may now run the explicitly approved native build scripts.

How tested

  • pnpm install passed and ran the approved native postinstall/rebuild path.
  • Codex LSP diagnostics: renderer source scan reported 0 errors; focused raycast-api and hooks scans also reported 0 errors.
  • pnpm exec tsc -p tsconfig.renderer.json --noEmit --pretty false exited 0.
  • ./node_modules/.bin/tsc -p tsconfig.renderer.json --noEmit --pretty false exited 0 with no diagnostics.
  • pnpm exec tsc -p tsconfig.main.json --noEmit --pretty false exited 0.
  • ./node_modules/.bin/tsc -p tsconfig.main.json --noEmit --pretty false exited 0 with no diagnostics.
  • pnpm run check:i18n passed for de, es, fr, it, ja, ko, ru, zh-Hans, and zh-Hant.
  • pnpm test passed: 28 tests, 0 failures.
  • pnpm run build:main passed.
  • pnpm run build:renderer passed; Vite emitted its existing CJS Node API deprecation warning.
  • git diff --check origin/main...HEAD passed.

Performance evidence

This is a validation-only PR that restores project checks, typecheck, i18n parity, build coverage, and workflow behavior. It has no runtime performance benchmark.

Stack validation

The branch was created cleanly from origin/main and contains only the intended source PR changes listed above. This consolidation is not a runtime performance benchmark task, so validation evidence is the restored typecheck, i18n, test, build, LSP, and workflow-diff checks rather than before/after timing data.

Replaces

@shobhit99

Copy link
Copy Markdown
Contributor

Summary: A broad Codex consolidation of six validation/type PRs into one. The core "restore project checks" work (i18n parity, a new CI workflow, an upgraded test helper, renderer type-cleanups) is correct and I verified the load-bearing pieces end-to-end. A few non-blocking gaps below.

Strengths

  • i18n restoration is complete and correct. scripts/check-i18n.mjs now defaults strict-mode to all locales (was only ko), matching the project i18n policy. I confirmed on main that every locale is already at parity except it, which is missing exactly 14 keys (settings.ai.whisper.vocabulary.* ×4, settings.extensions.appSearchScope.* ×10) — and it.json adds precisely those 14. So the stricter gate passes with no false failures. Italian copy preserves the {count} placeholder and brand terms (Whisper, SuperCmd, Kotlin/Electron/Raycast).
  • Test helper upgrade is sound. scripts/test-browser-search-lag-fix.mjs moves from grep-only assertions to actually executing the real module via the new esbuild-bundling importTs. I verified the new assertions match reality: resolveBrowserInput('example.com')https://example.com/ / host example.com, and toSearch(...)https://www.google.com/search?q=browser%20search%20lag (browser-input-resolver.ts:120-123). No false CI failure.
  • CI install flags are correct. project-checks.yml uses npm ci --ignore-scripts --force. @esbuild/linux-x64 is present in package-lock.json (optional dep), so esbuild works on the Ubuntu runner for both npm test and vite build; --force correctly bypasses the os:darwin-pinned @esbuild/darwin-* direct deps. --ignore-scripts safely skips the electron/native postinstall (guarded by ELECTRON_SKIP_BINARY_DOWNLOAD/SUPERCMD_SKIP_ELECTRON_TESTS).
  • A real latent bug is fixed: CameraExtension.tsx:301 previously assigned to setCapturePreviewClearTimerRef.current — a non-existent identifier that would throw ReferenceError inside the setTimeout callback — now correctly capturePreviewClearTimerRef.current.
  • Remaining type changes are behavior-preserving: addHexAlpha Deps type now matches the real (color, alphaHex) => string | undefined (icon-runtime-assets.tsx:236); the OAuth getPersonalAccessToken() getter avoids reaching a private field; the Toast.Style enum/static dedup keeps the same runtime value.

Concerns / potential bugs

  • The new workflow does not gate the renderer typecheckproject-checks.yml:77-82 runs build:renderer (= vite build, esbuild only, no type checking) and only build:main runs tsc. The whole "make renderer typecheck clean" effort (source PR fix(types): make renderer typecheck clean #518) is therefore not enforced by CI, so future renderer type regressions won't fail. Recommend adding a tsc -p tsconfig.renderer.json --noEmit step. (Not a correctness bug, but it undercuts the PR's stated goal.)
  • Overlaps with sibling PR fix(validation): consolidate project checks #600 ("fix(validation): consolidate project checks", branch codex/consolidate-validation-final), which consolidates the same source PRs. Both add project-checks.yml + pnpm-workspace.yaml, both modify check-i18n.mjs, both add the Italian keys — they will conflict. Only one should land; please dedupe before merge.
  • Minor — type narrowing at hooks/useLauncherCommandModel.ts:190 drops | string from browserId?: BrowserSearchSource. Safe as long as no untyped value (IPC/JSON) is ever assigned to it; typecheck passing suggests all call sites are typed, just flagging the narrowed contract.

Suggestions

  • Consider splitting the pure type/behavior renderer changes from the CI/i18n changes in future — the fix(validation) title undersells edits to OAuth, Toast, list/detail runtime, camera, and settings, which makes regressions harder to bisect. (All disclosed in the description, so this is process-only.)

Verdict: 🟡 Approve with minor comments

🤖 Automated review by Claude Code (Opus 4.8).

@JustYannicc

JustYannicc commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the maintainer feedback on this PR.

Changes made:

  • Added an explicit renderer typecheck gate to .github/workflows/project-checks.yml before the renderer Vite build:
    npm exec -- tsc -p tsconfig.renderer.json --noEmit
  • Widened launcher/browser profile ID typing from BrowserSearchSource to BrowserSearchSource | string at the launcher profile boundary and renderer CommandInfo browser profile fields, so IPC/JSON-provided string IDs remain type-safe instead of relying on the narrower enum-only contract.
  • Re-checked PR review state via GraphQL: there are no inline review threads on fix(validation): restore project checks #532; the actionable feedback was all in the top-level comment.
  • Confirmed PR fix(validation): consolidate project checks #600 is still open and overlaps this consolidation work. I did not make a code change for that here because it is merge coordination between the two PRs, but calling it out explicitly so only one consolidation path is landed/deduped.

Verification:

  • node --test scripts/test-renderer-crash-recovery.mjs scripts/test-renderer-error-boundary.mjs scripts/test-root-search-ranking.mjs ✅ 14 passed
  • npm exec -- tsc -p tsconfig.main.json --noEmit
  • git diff --check

An independent verifier agent reviewed the final diff and found no blockers.

@JustYannicc JustYannicc self-assigned this Jul 12, 2026
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