Repository navigation
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Enabling the compile cache at the tail of runMainProcessPreflight means the main process's own module graph is never cached — the main-process half of the stated goal isn't achieved by the current placement. See the inline note.
Reviewed changes
Clears up how this PR's two mechanisms actually behave at runtime:
- Main-process compile cache —
enableMainProcessCompileCache()callsmodule.enableCompileCache(dir), then re-exports the resolved directory viaprocess.env.NODE_COMPILE_CACHEso child processes inherit it. - Session code-cache path —
configureSessionCodeCache()pinssession.defaultSession's V8 code cache location to<userData>/Code Cache. - Tests — cover the happy path, the env-var round-trip, and graceful failure, plus a
setCodeCachePathmock assertion.
ℹ️ Nitpicks
app.getPath('userData')is a persistent, non-cleared directory, but Node's docs recommend a directory underos.tmpdir()for the compile cache to avoid filling the disk with stale entries. Since the cache is invalidated per Node version (not per app version), entries for modules that move or disappear across app updates will accumulate there. Consider whether a temp dir is a better default, or accept the growth and document it.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe main bootstrap inserts a compile-cache banner before startup diagnostics. The banner calls Node’s compile-cache API when available and sets Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The banner is configured to run before the main bundle, addressing the warm-start concern. The remaining risk is a bounded test gap: the fallback test may pass after earlier cache initialization without checking fallback selection, so a regression could go undetected. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: ed09f6d8-f424-4f2f-ad2f-c6dddf29132e
📒 Files selected for processing (4)
src/main/startup/main-process-preflight.tssrc/main/startup/main-process-ready-foundation.tssrc/main/startup/native-code-cache.test.tssrc/main/startup/native-code-cache.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| recordBreadcrumb: (data) => recordDurableCrashBreadcrumb('gpu_crash_hardware', data) | ||
| }) | ||
| : null | ||
| enableMainProcessCompileCache() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 37238
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- changed file context ---'
sed -n '1,80p;270,325p' src/main/startup/main-process-preflight.ts
printf '%s\n' '--- helper definition and usages ---'
rg -n -C 8 'enableMainProcessCompileCache' .
printf '%s\n' '--- main entry candidates and imports ---'
rg -n -C 4 'main-process-preflight|bootstrap|startup' src/main package.json 2>/dev/null | head -240Repository: stablyai/orca
Length of output: 38689
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- compile-cache implementation ---'
cat -n src/main/startup/native-code-cache.ts
printf '%s\n' '--- preflight declaration and call sites ---'
rg -n -C 12 'export (async )?function|function .*preflight|runMainProcessPreflight|main-process-preflight' src/main --glob '*.ts' --glob '!*.test.ts'
printf '%s\n' '--- package entry configuration ---'
rg -n -C 8 '"main"|electron|src/main' package.json electron-builder.yml* forge.config.* config src --glob '*.json' --glob '*.yml' --glob '*.yaml' --glob '*.ts' --glob '*.mjs' 2>/dev/null | head -260Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- compile-cache implementation ---'
cat -n src/main/startup/native-code-cache.ts
printf '%s\n' '--- exact preflight references ---'
rg -n --glob '*.ts' --glob '*.tsx' --glob '*.mjs' --glob '*.js' 'main-process-preflight' . | head -80
printf '%s\n' '--- package main fields ---'
rg -n -C 3 '"main"|"build"|"files"' package.json
printf '%s\n' '--- likely main entry files ---'
git ls-files 'src/main/*' | head -80Repository: stablyai/orca
Length of output: 10888
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- main entry ---'
cat -n src/main/index.ts | sed -n '1,90p'
printf '%s\n' '--- preflight declaration and relevant beginning ---'
rg -n -C 10 'runMainProcessPreflight|export const|export function|export async function' src/main/startup/main-process-preflight.ts | head -120Repository: stablyai/orca
Length of output: 5647
🌐 Web query:
Node.js module.enableCompileCache modules compiled after enabled official documentation v24
💡 Result:
In Node.js v24, the module compile cache is a feature that allows Node.js to persist V8 code cache on disk for CommonJS, ECMAScript, and TypeScript modules to speed up subsequent compilations [1][2]. Once enabled, it improves the performance of module loading by reducing repeated compilation work, though it may introduce a slight overhead during the initial load of a module graph [1][3]. To enable this feature, you can use the module.enableCompileCache method or the NODE_COMPILE_CACHE environment variable [1][4]. Method Usage: module.enableCompileCache([options]) The method accepts an optional object: - directory (string, optional): The location to store the compile cache. If not provided, Node.js defaults to the path specified by the NODE_COMPILE_CACHE environment variable, or path.join(os.tmpdir, 'node-compile-cache') if that variable is unset [1][3]. - portable (boolean, optional): If set to true, this enables portable compile cache, allowing the cache to be reused even if the project directory is moved (a best-effort feature) [1][2]. Recommendations and Behavior: - For general use, it is recommended to call module.enableCompileCache without arguments to allow the directory to be overridden by the NODE_COMPILE_CACHE environment variable when necessary [1][4]. - Enabling the cache only affects the current Node.js instance. To extend it to child worker threads, you must either call the method within those threads or set the NODE_COMPILE_CACHE environment variable in the parent process, which can be inherited by the children [1][4][2]. - The API is designed not to throw exceptions; it is intended to be a quiet optimization. The method returns an object containing information such as the status (one of module.constants.compileCacheStatus) and the directory being used [1][5]. - To locate the compile cache directory used by a running instance, you can use the module.getCompileCacheDir method [4][2]. - You can clear the cache at any time by deleting the directory specified for storage; it will be recreated automatically the next time it is needed [1][6].
Citations:
- 1: https://nodejs.org/docs/latest-v24.x/api/module.html
- 2: https://nodejs-api-docs-tooling.vercel.app/module.html
- 3: https://nodejs.org/api/module.md
- 4: https://nodejs.org/download/nightly/v24.0.0-nightly2024122967b647edc7/docs/api/module.html
- 5: https://nodejs.org/download/release/v22.13.1/docs/api/module.html
- 6: nodejs/node@4d77fd2c46
Enable Node’s compile cache before importing the main startup graph.
src/main/index.ts imports runMainProcessPreflight, which imports enableMainProcessCompileCache. Line 305 enables the cache only after this graph has loaded, so warm launches cannot use it for the main startup modules. Initialize the cache in an entry shim before importing the bootstrap modules.
| }) | ||
|
|
||
| it('enables compile cache and sets process.env.NODE_COMPILE_CACHE', () => { | ||
| const result = enableMainProcessCompileCache('/tmp/mock-cache') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 20402
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test file ---'
cat -n src/main/startup/native-code-cache.test.ts
printf '%s\n' '--- implementation and direct references ---'
rg -n -C 8 'enableMainProcessCompileCache|configureSessionCodeCache|NODE_COMPILE_CACHE|ALREADY_ENABLED|compile-cache' src/mainRepository: stablyai/orca
Length of output: 17044
🌐 Web query:
Node.js v24 module.enableCompileCache ALREADY_ENABLED directory documentation
💡 Result:
In Node.js, ALREADY_ENABLED is a status value returned by the module.enableCompileCache function [1][2][3]. It indicates that the module compile cache has already been initialized in the current process [1][4][5]. When module.enableCompileCache returns a status of ALREADY_ENABLED, the returned object also includes a directory property [1][6][5]. This property specifies the absolute path to the directory currently being used to store the compile cache [1][7][5]. The compile cache is considered already enabled if it was previously activated by: 1. A prior call to module.enableCompileCache within the same process [1][7][4]. 2. The presence of the NODE_COMPILE_CACHE environment variable [1][2][7]. The compile cache feature is designed as a non-critical optimization [4][3][5]. Therefore, calling module.enableCompileCache when it is ALREADY_ENABLED does not throw an exception; instead, it provides the existing configuration details through the returned object to assist with debugging or verification [7][4][3].
Citations:
- 1: http://nodejs.org/docs/latest/api/module.html
- 2: https://nodejs.org/docs/latest-v22.x/api/module.html
- 3: https://beta.docs.nodejs.org/module.html
- 4: https://bun.sh/reference/node/module
- 5: https://nodejs.org/docs/latest-v26.x/api/module.html
- 6: https://nodejs.org/download/release/v22.13.1/docs/api/module.html
- 7: https://docs.deno.com/api/node/module/
Isolate the compile-cache tests from process-global state.
The first test enables Node’s process-global compile cache at /tmp/mock-cache. The fallback test can then receive ALREADY_ENABLED and reuse that directory instead of testing the userData/compile-cache fallback. Mock node:module or run these tests in an isolated process, then assert the exact fallback directory.
| it('returns false gracefully if setCodeCachePath throws or is missing', () => { | ||
| expect(configureSessionCodeCache({} as Pick<Session, 'setCodeCachePath'>)).toBe(false) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the throwing branch.
The test passes an object without setCodeCachePath. It exits through the guard. The catch for a throwing setCodeCachePath remains untested. Add a session mock whose method throws.
Proposed test addition
it('returns false gracefully if setCodeCachePath throws or is missing', () => {
expect(configureSessionCodeCache({} as Pick<Session, 'setCodeCachePath'>)).toBe(false)
+ expect(
+ configureSessionCodeCache({
+ setCodeCachePath: () => {
+ throw new Error('cache path unavailable')
+ }
+ })
+ ).toBe(false)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('returns false gracefully if setCodeCachePath throws or is missing', () => { | |
| expect(configureSessionCodeCache({} as Pick<Session, 'setCodeCachePath'>)).toBe(false) | |
| }) | |
| it('returns false gracefully if setCodeCachePath throws or is missing', () => { | |
| expect(configureSessionCodeCache({} as Pick<Session, 'setCodeCachePath'>)).toBe(false) | |
| expect( | |
| configureSessionCodeCache({ | |
| setCodeCachePath: () => { | |
| throw new Error('cache path unavailable') | |
| } | |
| }) | |
| ).toBe(false) | |
| }) |
|
Pushed 4490ea0 addressing the placement blocker and the other two threads: Placement (the blocker) — you're right that the preflight call was too late. The enable now lives in a build-time banner prepended to
Test isolation — rewritten. The old fallback test could never fail; new tests cover the wrapper's actual contract (idempotent repeat-enable keeps |
There was a problem hiding this comment.
ℹ️ No critical issues — one minor cross-platform nitpick inline.
Reviewed changes
- Added a build-time
createCompileCacheBanner()thatcreateMainBootstrapPlugin()prepends toout/main/index.js, callingmodule.enableCompileCache()before the main module graph evaluates and pinningNODE_COMPILE_CACHEfor forked children — resolving the prior "enabled too late" finding. - Removed
configureSessionCodeCache()and itsmain-process-ready-foundation.tscall site (a runtime no-op). - Rewrote
native-code-cache.test.tsto assert the wrapper contract (idempotency + result shape) instead of pretending to exercise the process-global one-shot fallback.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Portable test paths pushed ( |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Test path portability — replaced the hardcoded
/tmp/mock-cachecache dir withjoin(tmpdir(), 'orca-compile-cache-test')in the compile-cache tests, resolving the prior cross-platform nitpick (the Unix-only path broke on Windows where it resolved to<drive>:\tmp\mock-cache).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…uates CodeRabbit blocker: module.enableCompileCache() is not retroactive — the call at the tail of runMainProcessPreflight only saw modules compiled after it, so the main process's own startup graph was never cached. Move the enable to a build-time banner prepended to out/main/index.js (same generateBundle mechanism as the bootstrap-fatal-exit and startup- diagnostics banners). It runs before Rollup's chunk requires, so the entire main graph is cached from the first launch. The no-arg call uses Node's default cache dir outside userData, which keeps it correct across the dev/E2E userData redirect that happens later in preflight; chunk cache entries are keyed by content hash and shared across profiles. The runtime enableMainProcessCompileCache() stays as an idempotent fallback that pins NODE_COMPILE_CACHE for forked children (daemon, plugin-host, sidecars); the banner sets the same env var on the normal path. Drop configureSessionCodeCache — setCodeCachePath to the default 'Code Cache' location was a runtime no-op.
Hardcoded /tmp/mock-cache resolves to a drive root on Windows where mkdirSync can fail; derive from os.tmpdir() instead.
829da71 to
8e6efd4
Compare
This branch has not been deployed
1 participant
ELI5
Node re-parses the same JavaScript on every app launch. This PR turns on Node's on-disk compile cache so warm starts skip parsing/compiling for the main process and its child processes.
What
Enables
module.enableCompileCache()for the main process and its child processes at startup.Why (and why the original placement was wrong)
CodeRabbit correctly flagged that the first version called
enableCompileCache()at the tail of preflight — afterindex.ts's entire static import graph had already evaluated. Since the API is not retroactive, the main process's own startup (the headline claim) was never cached. 4490ea0 re-architects:createCompileCacheBanner()is prepended toout/main/index.jsbycreateMainBootstrapPlugin()— the samegenerateBundlemechanism already used for the bootstrap-fatal-exit and startup-diagnostics banners. It executes before Rollup's chunk requires, so the whole main graph is cached from the first launch. Verified in the built bundle: banner at byte ~3.3k, module graph begins at ~8.1k.userData) is used deliberately — preflight redirectsuserDatafor dev/E2E only after this banner runs; a no-arg enable is immune to that ordering, and chunk cache entries are keyed by content hash so they are safely shared across profiles.process.env.NODE_COMPILE_CACHEto the resolved directory so forked children (daemon, plugin-host, sidecars) reuse the cache; the runtimeenableMainProcessCompileCache()remains as an idempotent fallback for banner-less entry paths.configureSessionCodeCache—session.setCodeCachePathto Electron's own defaultCode Cachelocation was a runtime no-op (h/t review).Visual proof
N/A — no UI change; startup timing only.
Testing
pnpm vitest run src/main/startup/native-code-cache.test.ts— 3 passing (idempotency, env pinning, fallback contract)pnpm tsc --noEmit -p config/tsconfig.node.json— cleanpnpm build:electron-vite— banner verified in built output before the module graph; banner IIFE executed standalone setsNODE_COMPILE_CACHEto the default cache dirPlatform coverage
Compatibility
enableCompileCacheis a no-op guarded by a feature check on Node versions without itAI disclosure
Authored with AI assistance (Claude); human-reviewed.
Review notes
Checklist