Skip to content

perf(renderer): consolidate Raycast runtime hot paths#599

Open
JustYannicc wants to merge 10 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-renderer-raycast-wave4
Open

perf(renderer): consolidate Raycast runtime hot paths#599
JustYannicc wants to merge 10 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-renderer-raycast-wave4

Conversation

@JustYannicc

Copy link
Copy Markdown
Collaborator

What changed

  • Lazy-load non-English renderer locale catalogs while keeping English available as the fallback catalog.
  • Scope Raycast frecency storage by extension context and migrate legacy rankings.
  • Stream canvas sc-asset://canvas-lib/* files through the protocol helper and move scene/thumbnail reads to async filesystem paths.
  • Reduce Raycast grid selection churn by avoiding unnecessary cell work on selection changes.
  • Clear retained form runtime snapshots on unmount.
  • Guard action/list/grid/menubar registry microtasks so queued updates do not call state after unmount.

Why

These are the clean wave4 renderer/Raycast hot-path fixes from the granular PRs, consolidated onto current main without the inherited broad-branch noise, pnpm artifacts, coordinator notes, prompt/playbook files, or generated locks.

Compatibility impact

Raycast API behavior is preserved. Storage changes keep legacy frecency data via migration, locale loading still falls back to English while async catalogs load, canvas asset URLs remain under the existing sc-asset scheme, and the registry/form cleanup paths only affect teardown behavior.

How tested

  • git diff --check
  • node --test scripts/test-i18n-locale-loading.mjs scripts/test-use-frecency-sorting.mjs scripts/test-canvas-asset-serving.mjs scripts/test-form-runtime-snapshot-cleanup.mjs scripts/test-registry-microtask-lifecycle.mjs
  • ./node_modules/.bin/vite build
  • LSP diagnostics on all touched TS/TSX files: no errors

npm run typecheck:renderer could not run through this machine's npm shim: npm is intentionally disabled and the pnpm fallback stops on ignored native build scripts before script dispatch. The package also has no typecheck:renderer entry in package.json. I ran the renderer compiler equivalent directly with ./node_modules/.bin/tsc -p tsconfig.renderer.json --noEmit; it still fails on existing repo-wide diagnostics outside this wave4 branch, including src/renderer/src/App.tsx, CameraExtension.tsx, components/LiquidGlassSurface.tsx, launcher browser search model/control types, Raycast API index/toast/type issues, oauth access-token types, settings voice types, and quicklink icon option typing. Touched files are clean under LSP.

Performance evidence

  • Canvas asset event-loop measurement, 32 MB x 8 iterations: new stream HTTP serving p95 delay 1.311ms vs old buffered HTTP serving p95 delay 2.785ms; new file stream response p95 delay 1.356ms vs old buffered readFile response p95 delay 2.265ms.
  • Raycast grid churn measurement, 112 visible cells, 4096 items, 1200 selection steps, 15 iterations: visible cell render median 28.188ms, selection scroll median 0.657ms, selection render churn median 650.574ms with 2400 cell renders.

Stack validation

This branch is origin/main plus seven commits: the six selected wave4 implementation commits and one finisher validation commit. It intentionally does not include pnpm workspace/lock artifacts or the broader historical renderer stacks. PRs #533, #555, and #572 are not claimed as replaced because they cover broader work not fully included here.

Replaces

@shobhit99

Copy link
Copy Markdown
Contributor

Reviewed the full diff. This is a well-scoped consolidation of six wave4 hot-path fixes (canvas async I/O + streamed sc-asset://canvas-lib, lazy locale catalogs, frecency scoping, form-snapshot cleanup, grid memoization, and registry unmount guards). Despite the ~2460 added lines, ~1900 are .mjs test/measurement scripts; the actual product surface is a modest ~550 lines across 13 files, which keeps bisect risk low. Per the PR body it supersedes the granular #581#598 PRs and explicitly does not claim to replace #533/#555/#572 (broader work) — that framing looks accurate.

Strengths

  • Grid React.memo comparator is correct. areGridItemRendererPropsEqual (grid-runtime-items.tsx:~127) compares title/subtitle/content/isSelected/dataIdx and omits onSelect/onActivate/onContextAction. I verified in grid-runtime.tsx:243-261 that those handlers only close over globalIdx (passed as dataIdx, which is compared) plus stable setters/refs, and that the render body (grid-runtime-items.tsx:159-203) derives its entire visual output from the compared props (data-idx={dataIdx}, swatchColor/renderableContent from content). So skipping handler comparison cannot produce stale visuals or stale closures. Hoisting the helper fns to module scope preserves behavior (same resolveIconSrc threaded through).
  • Frecency scoping + migration is safe. readScopedJsonState (storage-scope.ts) copies legacy→scoped without deleting the legacy key, so multiple extensions sharing the old global sc-frecency-<ns> key each migrate independently with no data loss. Bonus correctness fix: snapshotting now once per sort (use-frecency-sorting.ts:65-80) removes the Date.now()-per-comparison inconsistency that could violate sort transitivity.
  • Form snapshot ownership (form-runtime-context.tsx / form-runtime.tsx) correctly prevents an unmounting form from clobbering a freshly-mounted form's global snapshot (symbol owner guard), and clears the retained module-level snapshot on unmount. Submit reads happen synchronously before the pop/unmount cleanup, so getFormValues()/getFormErrors() are unaffected.
  • Lazy i18n falls back to English gracefully; translateMessage null-guards both catalogs, and no non-React caller relies on synchronous catalog availability (only consumer is useI18n().t). No en.json strings changed, so the locale-parity rule isn't triggered.
  • Canvas async + sc-asset-protocol.ts is a clean extraction and adds real path-traversal protection (path.relative escape check → 404). net.fetch(file://…) is already an established pattern in main.ts (13699/13720), getCanvasLibDir() returns the same userData/canvas-lib path the inline code used, and streaming upstream.body avoids the old blocking readFileSync.

Concerns / potential bugs

  • Registry unmount guards are redundant with the microtask guard and add StrictMode fragility (dev-only, likely self-healing). action-runtime-registry.tsx:386/402, grid-runtime-hooks.ts:53/65, list-runtime-hooks.ts:96/117, menubar-runtime-parent.tsx add if (!mountedRef.current) return; inside register/set/unregister/delete and registryRef.current.clear() in a passive-useEffect cleanup. The only state-after-unmount risk is setRegistryVersion inside the queued microtask — and that path is already guarded by the new if (!mountedRef.current) check inside queueMicrotask. The per-call guards + clear() are therefore unnecessary, and they interact awkwardly with React.StrictMode (enabled in main.tsx:184): items register via useLayoutEffect (grid-runtime-items.tsx:48, list-runtime-renderers.tsx:59) or useEffect (menubar-runtime-items.tsx:28) that run before the container's passive mount effect re-sets mountedRef=true. During StrictMode's dev double-invoke the container's simulated passive unmount (mountedRef=false + registry.clear()) can wipe/drop the re-registrations. In practice this almost certainly self-heals on the next container re-render — the item registration effects depend on [props, …] and props is a fresh object each render, so they re-run once mountedRef is true again — and production builds never double-invoke. Net user-facing impact is probably nil-to-transient, but it's fragile. Suggest keeping only the microtask guard and dropping the per-call guards + registryRef.current.clear() (clearing a ref on true unmount buys nothing since the whole component is GC'd anyway), or at minimum smoke-testing that a static List/Grid/MenuBarExtra with no post-mount re-render still populates in dev under StrictMode.

Suggestions (optional)

  • The lazy-locale change means non-English users briefly see English on first paint until the dynamic import() resolves. Acceptable and documented, but if any locale is the persisted default it may be worth pre-warming loadAppLocale(locale) before first paint to avoid the flash.

Verdict: 🟡 Approve with minor comments — the core API-behavior-preservation is solid (grid memo, frecency, form snapshot, i18n fallback all verified correct); the only open item is the redundant/fragile registry unmount guards, which are dev-only and likely self-healing but worth trimming or verifying under StrictMode.

🤖 Automated review by Claude Code (Opus 4.8).

@JustYannicc

Copy link
Copy Markdown
Collaborator Author

Addressed the registry teardown feedback from the review.

What changed:

  • Removed the redundant per-call mountedRef guards from the action/list/grid/menubar registry mutation APIs.
  • Removed passive cleanup registryRef.current.clear() calls from those registries.
  • Kept the important protection at the actual risk point: the queued microtask still checks mountedRef.current before calling state.
  • Expanded scripts/test-registry-microtask-lifecycle.mjs with a StrictMode-style passive cleanup replay smoke test for static Action/List/Grid/MenuBarExtra registrations. The test registers while the parent mounted ref is temporarily false, replays passive setup, and verifies the registries republish without post-unmount state updates.

Proof/tests run locally:

  • git diff --check
  • rg -n "if \\(!mountedRef\\.current\\) return|registryRef\\.current\\.clear\\(\\)|!mountedRef\\.current \\|\\| pendingRef\\.current" src/renderer/src/raycast-api/action-runtime-registry.tsx src/renderer/src/raycast-api/grid-runtime-hooks.ts src/renderer/src/raycast-api/list-runtime-hooks.ts src/renderer/src/raycast-api/menubar-runtime-parent.tsx ✅ no matches
  • node --test scripts/test-registry-microtask-lifecycle.mjs ✅ 3/3 tests pass, including registry-strictmode-passive-replay
  • node --test scripts/test-i18n-locale-loading.mjs scripts/test-use-frecency-sorting.mjs scripts/test-canvas-asset-serving.mjs scripts/test-form-runtime-snapshot-cleanup.mjs scripts/test-registry-microtask-lifecycle.mjs ✅ 17/17 pass
  • ./node_modules/.bin/vite build

Also re-ran ./node_modules/.bin/tsc -p tsconfig.renderer.json --noEmit; it still fails on existing repo-wide renderer diagnostics outside this fix, consistent with the PR body. None of the new failures are in the edited registry/test files.

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