Skip to content

perf(startup): stop queueing window creation behind the proxy apply and i18n - #18436

Merged
nwparker merged 5 commits into
mainfrom
perf/startup-parallel-awaits
Sep 4, 2026
Merged

perf(startup): stop queueing window creation behind the proxy apply and i18n#18436
nwparker merged 5 commits into
mainfrom
perf/startup-parallel-awaits

Conversation

@nwparker

@nwparker nwparker commented Sep 3, 2026

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​442 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​436
Prod 10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​102 $\color{#cf222e}{\Huge{\mathbf{−}}}$​19 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​83

ELI5

When Orca launches, the main process does a short list of chores before it is allowed to open the window you actually look at. Two of those chores did not need to be in that line. One was waiting for the network-proxy setting to be written into Chromium; the other was loading the translated app menu. Neither is something the window needs in order to exist, so the window was just standing behind them.

This moves both out of the way. The proxy is still applied at exactly the same moment and still finishes before anything on the network happens — a separate gate already holds every request until it lands — but the window no longer waits for the confirmation. The menu is built at the same time the window is created instead of before it.

One smaller thing rides along: a 166 KB emoji dataset that only gets read when you name a workspace was being baked into the main bundle and JSON-parsed on every single launch; it is now read from disk the first time it is actually needed.

Why it matters

Measured with ORCA_STARTUP_DIAGNOSTICS, best warm iteration on this machine:

  • f-claude-pty-seeded -> f-proxy-applied = 24 ms, with nothing between the two markers but the await on applyElectronProxySettings. That 24 ms sat entirely in front of open-main-window-start.
  • initializeMainProcessI18nAndMenu = 8 ms for English. Non-English adds a lazy locale chunk (src/main/i18n/main-i18n.ts:26-30, 380-616 KB), 2.6-5.5 ms to read and compile plus i18next ingestion — call it 15-20 ms for ja/ko/es/zh.
  • Whole-phase context from the same trace: app-ready -> open-main-window-start = 91 ms; open-main-window-start -> window-created = 119 ms; window-created -> did-finish-load = 273 ms.

So this is roughly 32 ms (English) to 44 ms (non-English) removed from in front of window creation, plus the 166 KiB main-bundle reduction below, against a 1.6-2.0 s time-to-workspace-ready. It is a real but small fraction. Several of these are two-line reorders, which is why they are worth taking.

Measurement

Bundle bytes (electron-vite production build, before/after on this branch)

before after delta
out/main/index.js 7,210,071 B 7,040,147 B -169,924 B (-166.0 KiB)

Renderer bytes are unchanged: the lazy-dialog item is no longer on this branch (see "What I did not ship"), so verify-renderer-boot-graph reports the same 342 chunks / 4368.4 KB as main.

Confirming the emoji dataset really left the main bundle (water_buffalo is a shortcode only that JSON carries):

$ grep -c water_buffalo out/main/index.js     # before
1
$ grep -c water_buffalo out/main/index.js     # after
0
$ grep -rl water_buffalo out/main/            # after: not in a split chunk either
(no output)

Microbenchmark: the parse that used to run on every launch

Re-measured with process.cpuUsage() deltas rather than wall clock (this box runs ~24 agents;
wall-clock numbers on it are noise):

dataset bytes 170452
parse cpu 1.247 ms (keys 3979)
parse cpu 0.74 ms  (keys 3979)
parse cpu 1.408 ms (keys 3979)
parse cpu 0.711 ms (keys 3979)
parse cpu 1.273 ms (keys 3979)

That ran at main-bundle module-evaluation time on every start. It now runs only when a worktree name is first sanitized.

Failing-test proof

The new/changed guards were run against origin/main's sources (production files stashed, tests kept):

 FAIL  src/shared/emoji-shortcode-catalog.lazy.test.ts > emoji shortcode catalog laziness > keeps the 166 KB dataset off every module main statically imports
AssertionError: expected 'import emojiShortcodes from \'emojiba…' not to match /\bfrom '[^']*emojibase-data[^']*'/

 FAIL  src/shared/emoji-shortcode-catalog.lazy.test.ts > emoji shortcode catalog laziness > loads the main-side dataset synchronously into an identical catalog

 FAIL  src/main/startup/main-process-ready-phase-ordering.test.ts > ready-phase concurrency > creates the window without waiting for i18n and the native menu
AssertionError: expected [ 'foundation', …(2) ] to deeply equal [ 'foundation', …(4) ]

 FAIL  src/main/startup/main-process-ready-phase-ordering.test.ts > initial proxy application ordering > parks the default-session proxy apply instead of blocking window creation on it
AssertionError: expected 'import { app, session } from \'electr…' to contain 'state.initialProxyApplicationReady = …'

 FAIL  src/main/startup/main-process-ready-phase-ordering.test.ts > initial proxy application ordering > awaits the proxy after the window opens and before the desktop relay starts
AssertionError: expected -1 to be greater than 662

 FAIL  src/main/startup/main-process-ready-phase-ordering.test.ts > initial proxy application ordering > keeps headless serve strictly ordered behind the proxy apply
AssertionError: expected -1 to be greater than or equal to 0

 Test Files  2 failed (2)
      Tests  9 failed | 2 passed (11)

With the change:

 Test Files  2 passed (2)
      Tests  11 passed (11)

(The "creates the window without waiting for i18n" case is the load-bearing one for item 2: on main the i18n phase is awaited first, so with a deliberately-never-resolving i18n mock the launch phase never starts and the recorded event list stops at i18n-start. The two source-anchored proxy cases follow the existing desktop-startup-ordering.test.ts pattern, which is how this repo pins startup ordering.)

src/main/proxy-guarded-fetch-call-site-audit.test.ts (new, see Correctness) was verified failing by
introducing each violation it is meant to catch into src/main/updater-nudge.ts and reverting:

# adding `session: session.fromPartition('orca-nudge')` to the existing net.fetch options
 FAIL  proxy-guarded fetch call-site audit (main) > keeps every net.fetch/net.request on the guarded default session
AssertionError: This request names its own session/partition, so it is not covered by
installElectronProxyRequestGuard(session.defaultSession) and startup never applies the persisted
proxy to it. [...]: expected [ 'main/updater-nudge.ts:12' ] to deeply equal []

# routing the same call through `session.fromPartition('orca-nudge').fetch(...)` instead
 FAIL  proxy-guarded fetch call-site audit (main) > keeps every non-default-session fetcher audited with its expected count
AssertionError: A session.fromPartition(...) session is not covered by
installElectronProxyRequestGuard(session.defaultSession) [...]: expected
[ 'main/updater-nudge.ts: found 1 call(s)' ] to deeply equal []

99fe1997400 then closed two holes the first version of those rules had, verified the same way with a
scratch file carrying one call site per shape (each is caught, and the file is deleted again):

# net.request({ url, session }) and net.request({ url, partition })  -- object shorthand
 FAIL  ... > keeps every net.fetch/net.request on the guarded default session
expected [ 'main/tmp-violation-probe.ts:12', 'main/tmp-violation-probe.ts:6',
           'main/tmp-violation-probe.ts:9' ] to deeply equal []

# session.fromPartition('x').fetch(url), holder.s.fetch(url), session2.fetch(url)
 FAIL  ... > keeps every non-default-session fetcher audited with its expected count
expected [ 'main/tmp-violation-probe.ts: found 3 call(s)' ] to deeply equal []

Before that commit the shorthand and the two non-identifier receivers (....fetch(, a.b.fetch()
all passed silently; only line 6 and the bare session2.fetch( were caught. The audited counts did
not move (2/2/1), so no real call site changed classification.

Performance is unchanged by the audit commits

The audit is test-only, so none of the numbers above move. Verified deterministically rather than by
timing:

$ git diff bc1307d8bac..HEAD --stat -- . ':!*.test.ts' ':!*.test.mjs'
(no output)

i.e. the two audit commits (46131460e72, 99fe1997400) add and then tighten one .test.ts file and
change zero production lines -- the diff above is empty at both. The 24 ms proxy hoist,
the 8 ms i18n overlap and the 166 KiB main-bundle drop are all byte-for-byte the same code as the
head that was measured.

Correctness

Item 1 — the proxy await. The safety claim is not "no fetcher runs in that window". It does: src/main/claude-accounts/oauth-refresh.ts:142 issues net.fetch(OAUTH_TOKEN_URL), reached synchronously from new ClaudeRuntimeAuthService(store) -> void this.safeSyncForCurrentSelection() -> runtime-auth-sync.ts:250 -> refreshManagedAccountTokenIfNeeded, during initializeReadyRuntimeServices. The updater nudge/prerelease feed and the rate-limit fetchers follow at the ready-to-show / +1000 ms boundary.

The reason the hoist is safe is that installElectronProxyRequestGuard (src/main/network/electron-proxy-request-guard.ts, installed at main-process-ready-foundation.ts:56 and again at :143) is what enforces the ordering, not the await. It hooks defaultSession.webRequest.onBeforeRequest and awaits getProxySessionApplicationReadiness — it holds the request until the newest proxy transition settles rather than cancelling it (see the existing electron-proxy-request-guard.test.ts case "holds renderer requests until a delayed proxy transition settles"). Every fetcher above uses net.fetch/net.request with no explicit session, so they all land on defaultSession and are all held. Verified empirically on this repo's Electron 43 (standalone probe app, main-process net.fetch against a loopback server): the defaultSession onBeforeRequest listener sees the request URL, cb({cancel:true}) fails it with net::ERR_BLOCKED_BY_CLIENT, and a listener that defers its callback by 500 ms delays the net.fetch by the same 500 ms. So a main-process net.fetch really is held by the guard, not merely observed by it. The await in initializeReadyFoundation was therefore redundant for ordering and only queued openMainWindow behind a setProxy round trip.

The apply itself is unchanged: applyElectronProxySettings(store.getSettings()) is still called at the same line, before the guard is (re-)installed, so the guard still observes pending readiness from the same instant. The invalid-settings and failure console.warns were moved onto the parked promise's settle handlers, so they fire at the same moment they did before, not later. Attaching the handlers synchronously also closes a small unhandled-rejection window that existed while the promise sat unawaited for ~90 lines.

The phase postcondition is preserved: initializeMainProcessReady still does not resolve until the proxy has settled, because launchDesktopMode awaits it (after the window, before new DesktopRelayService) and launchServeMode awaits it at its top. Headless serve therefore keeps the strict "proxy before anything" ordering it had, since it has no window to unblock. Rejection behaviour is identical: the old try/catch swallowed failures and so does the new rejection handler, so await state.initialProxyApplicationReady can never throw.

applyBrowserSessionProxies is unaffected: it operates on session.fromPartition(...) objects with their own per-session state in sessionProxyApplications (a WeakMap keyed by session), so it was never ordered against the default session's apply — only against the resolver registration, which still precedes it.

Item 2 — i18n/menu concurrency. Nothing on the window-creation path imports main-i18n: translateMain consumers are system-tray.ts, main-window-close-lifecycle.ts, editable-context-menu.ts, runtime-rpc-startup-failure.ts, register-app-menu.ts, settings.ts and the i18n/menu phase itself — none of which createMainWindow, main-window-controller.ts, main-window-core-services.ts or main-window-service-readiness.ts reach. The tray is created on ready-to-show, which is after did-finish-load. Menu ordering is safe because Electron's Menu.setApplicationMenu applies to already-created windows (it iterates BrowserWindow.getAllWindows() on Windows/Linux and is global on macOS), and createMainWindow sets autoHideMenuBar: true and never calls window.setMenu, so there is nothing for a late registerAppMenu to race. Promise.all (rather than a bare parallel start) keeps both settled before the phase resolves and attaches handlers to both, so neither can become an unhandled rejection.

Item 4 — the emoji dataset. src/shared/emoji-shortcode-catalog.ts no longer imports the dataset itself; it takes a synchronous loader. Both consumers register one at module scope of the only module that reaches the catalog in their bundle, so the loader is always set before the first call:

  • renderer: workspace-emoji-shortcodes.ts keeps its eager static import and registers () => emojiShortcodes. Byte-for-byte the same renderer behaviour — no dynamic import, no await, no window in which the transform can return an empty catalog. config/scripts/renderer-boot-graph.mjs:44-48 documents why that must not change, and this PR does not change it.
  • main: deferred-emoji-shortcode-dataset.ts registers a createRequire(__filename) loader, the same pattern already used by src/main/linear/linear-sdk.ts. require() is synchronous, so loadCatalog() keeps its exact contract; a new test asserts the deferred path produces an toEqual-identical entry list and an identical replaceKnownEmojiWithShortcodes output with no await between registration and first use.

Packaging (corrected in bc1307d8bac). The original claim here — that emojibase-data ships inside app.asar because it is a production dependency — was wrong on both counts, and it was a P0. app.asar carries zero node_modules entries (verified against the shipped 1.4.197 build: asar.listPackage(...).filter(e => e.includes('node_modules')).length === 0), so every bare require from packaged main resolves out of Resources/node_modules, which is the explicit allowlist in config/packaged-runtime-node-modules.cjs (PACKAGED_RUNTIME_PACKAGE_ROOTS). That is how @linear/sdk works. emojibase-data is a devDependency (package.json:237) and was in neither place — find /Applications/Orca.app -name 'emojibase*' returns nothing — so requireEmojiShortcodeDataset() would have thrown MODULE_NOT_FOUND in every packaged build, taking sanitizeWorktreeName and therefore all workspace creation down with it. Dev builds resolve from the repo checkout, which is why local testing did not catch it.

verifyPackagedMainRuntimeDeps does not catch this either: it regexes out/main/index.js for literal require("x"), and the bundler renames the createRequire binding (grep 'require("@linear/sdk")' on the shipped bundle: no match).

The fix copies the single 166 KB dataset file — not the 49 MB package root — into Resources/node_modules via commonExtraResources, so the bare specifier resolves unchanged and the bundle/parse win is kept in full. emojibase-data has no exports field, so the one JSON file is sufficient for subpath resolution. Two new gates in config/scripts/electron-builder-runtime-resources.test.mjs: one resolves the dataset through a simulated Resources/app.asar/out/mainResources/node_modules walk, and one asserts every createRequire'd bare specifier under src/main is covered by the packaged resource plan on all three platforms (Windows-native loaders excepted). Both fail on the pre-fix config.

Edge cases.

  • SSH / folder workspaces: nothing here branches on workspace kind or execution host. sanitizeWorktreeName is unchanged in behaviour and still synchronous, so remote and folder-workspace naming are untouched.
  • Headless orca serve: gets its proxy await at the top of launchServeMode, ahead of the WSL barrier, headless PTY runtime, runtimeRpc.start(), the CLI install and printServeReady — strictly stronger ordering than the desktop path, and equivalent to today.
  • Windows: the early-window path (startWindowsDesktopBeforeShellPathReady) is untouched; it already opens the window before shellPathReady, and it flows into the same launchDesktopMode await.
  • Empty/missing state: mainProcessState.initialProxyApplicationReady defaults to Promise.resolve(), so any path that never runs the foundation (tests, orca CLI entry) awaits a no-op, exactly like shellPathReady and managedWslCliStartupBarrierReady.
  • Wire compatibility: no RPC params, stream frames or published content change.

The guard's precondition is now enforced by CI, not by review. "Every app-owned request lands on defaultSession, where the guard holds it" is the one invariant the hoist depends on. It held when this was written (all 18 net.fetch/net.request sites under src/main pass no session option), but nothing stopped a future fetcher from quietly routing around it. src/main/proxy-guarded-fetch-call-site-audit.test.ts (new, modelled on the existing src/main/global-fetch-call-site-audit.test.ts) makes both escape routes fail loudly:

  1. no net.fetch( / net.request( under src/main may name a session: neither a session: / partition: key nor the { url, session } object shorthand that both net.request overloads accept. The test balances parentheses from the call site to read the real argument text (skipping string bodies), so it is not fooled by line wrapping;
  2. every .fetch( call site whose receiver is not a literal net / globalThis / global is counted per file against an allowlist, so a new non-default-session fetcher fails until it is audited — including the receivers with no bare identifier to key off, session.fromPartition(...).fetch(...) and ctx.session.fetch(...). The allowlist is opencode-go-usage-fetcher.ts (2 — its session is proxied by createOpenCodeRequestSession), minimax-request-context.ts (2 — its session is not proxied, see below), and jira/authenticated-request.ts (1 — an injected HttpClient, not a session, which resolves to net.fetch on defaultSession). globalThis.fetch/global.fetch are excluded because global-fetch-call-site-audit.test.ts already owns them.

Both rules were verified failing (transcript above). This is worth noting because the same audit also covers the identical assumption on origin/main's post-startup proxy transitions (ipc/settings.ts:203-215 and every ensureElectronProxyFromEnvironment caller), where the guard has always been the sole ordering authority.

Pre-existing gaps the audit surfaces but does not close (for separate tickets), neither introduced nor worsened here:

  • electron-updater issues its own requests on session.fromPartition('electron-updater', {cache:false}) (node_modules/electron-updater/out/electronHttpExecutor.js:8,52), which neither the request guard nor applyElectronProxySettings touches. Third-party code, so out of the audit's reach; noted in the test's header comment.
  • main/rate-limits/minimax-request-context.ts fetches on session.fromPartition('orca-minimax-rate-limit-fetch') and never applies a proxy to it, unlike its opencode-go sibling. applyElectronProxySettings has only ever configured defaultSession, so the await this PR removes never covered that partition either — the behaviour is identical before and after. It is now listed in the audit allowlist with that gap spelled out, so it stays visible instead of being rediscovered.

Negative user-facing trade-offs

Not "None" — one, and it is a log-ordering detail:

  1. A console.warn for an invalid or failed proxy apply is no longer guaranteed to be printed before openMainWindow runs. It fires at the same wall-clock moment as before (the handlers are attached synchronously to the same promise), but its position relative to other startup logs can shift. Nothing branches on it.

Removed since the last review: the "first open of a lazy dialog waits on a chunk fetch" trade-off is gone, because the lazy-dialog item is no longer on this branch at all — NewWorkspaceComposerCard.tsx and SidebarSettingsHelpMenu.tsx are byte-identical to main. There is no longer any first-open latency cost, and no renderer behaviour change of any kind in this PR.

Explicitly not traded away: no new cap, cadence, debounce, threshold, sampling window or reduced coverage; the proxy is applied at the identical moment and no request can escape it — and, as of 46131460e72 / 99fe1997400, that last clause is a CI-enforced assertion rather than a claim.

What I did not ship from the brief

Item 3 (lazy renderer chunks) in full. Two of the seven chunks fit lazy-with-retry cleanly and were on an earlier revision of this branch, but they are no longer here: this PR is now main-process startup ordering plus the emoji dataset only, and it changes no renderer component. That also removed the only user-visible trade-off the PR had. The other five were never viable:

  • worktree-creation-flow (19.0 KB) and delete-worktree-flow (13.4 KB) are plain function modules, not components — lazy-with-retry does not apply, and they have 2 and 12 synchronous call sites respectively. Deferring them means turning those call sites async, which is the same class of change as the reverted emoji dynamic import.
  • SkillFreshnessUpdateDialog (7.2 KB) must stay mounted: it is the subscriber to skill-freshness-update-dialog's request store (subscribeSkillFreshnessUpdateDialog), so an unmounted dialog would miss requests. Deferring it safely needs the subscription hoisted into AppRootSurfaces, which is a bigger change than 7 KB justifies.
  • SshHostAdvancedFields (8.3 KB) owns its own "Advanced" disclosure trigger (open/onOpenChange are its props), so lazying it removes the click target — exactly what the brief says not to do.
  • CliSkillRuntimeSetup (6.8 KB) is a shared helper module with ~20 importers, most of them type-only. Not a dialog.

Test plan

nice -n 10 node_modules/.bin/vitest run --config config/vitest.config.ts \
  src/main/startup src/main/network src/shared/emoji-shortcode-catalog.lazy.test.ts \
  src/renderer/src/lib/workspace-emoji-shortcodes.lazy.test.ts src/main/ipc/worktree-logic.test.ts
# 55 passed | 1 skipped (56 files), 586 passed | 6 skipped

nice -n 10 node_modules/.bin/vitest run --config config/vitest.config.ts src/main/ipc/worktree
# 65 files, 805 passed

nice -n 10 node_modules/.bin/vitest run --config config/vitest.config.ts \
  src/main/proxy-guarded-fetch-call-site-audit.test.ts src/main/global-fetch-call-site-audit.test.ts \
  src/main/startup/main-process-ready-phase-ordering.test.ts \
  src/main/network/proxy-settings.test.ts src/main/network/proxy-settings-session.test.ts \
  src/main/network/electron-proxy-request-guard.test.ts \
  src/shared/emoji-shortcode-catalog.lazy.test.ts
# 7 files, 52 passed (re-run at 99fe1997400)

node_modules/.bin/tsc --noEmit -p config/tsconfig.node.json --composite false   # clean
node_modules/.bin/tsc --noEmit -p config/tsconfig.tc.cli.json --composite false # clean
node_modules/.bin/oxlint <changed files>                                        # clean
node config/scripts/run-electron-vite-build.mjs                                 # boot-graph gate passes

Manual check worth doing on review: create a workspace with an emoji in the name and confirm it still slugifies to the shortcode (that is the only user-reachable path the emoji-dataset change touches).

Changes since review (2ed433c58cb)

Rebased onto main (picks up the fr.json catalog fix from #18550, which is what static analysis was red on). Three follow-ups on top:

  1. Typecheck (the real failure). TS6307 in two directions:
    • tsconfig.tc.web.json lists src/main/ipc/worktree-logic.ts, which now imports src/main/ipc/deferred-emoji-shortcode-dataset.ts — that file is now listed too.
    • tsconfig.tc.cli.json includes src/shared/**/*, and emoji-shortcode-catalog.lazy.test.ts had one case importing ../main/ipc/deferred-emoji-shortcode-dataset.js. Rather than drag a createRequire main module into the CLI project, that case moved to src/main/ipc/deferred-emoji-shortcode-dataset.test.ts — which restores the boundary the shared test's own comment asserts. All three projects clean.
  2. The i18n race is now closed, not just documented. mainProcessState.mainProcessI18nReady is published synchronously in main-process-ready.ts before the launch phase is invoked, and launchDesktopMode chains the runtime-RPC failure dialog off it. Still void, not await: the wait is on i18n, never on the modal, and it is failure-only. desktop-startup-ordering.test.ts pins the new shape and now also asserts no await ... showRuntimeRpcStartupFailureDialog(.
  3. Audit whitespace hole. net.fetch (url, { session }) and x .fetch(url) slipped both rules because each regex required ( immediately after the member name; both now use \s*\(. Verified failing against a scratch probe carrying one call per shape (deleted again); audited counts unchanged at 2 / 2 / 1.

Win is intact. The whole production diff of this commit is 14 added / 5 removed lines across main-process-ready.ts, main-process-runtime-launch.ts and main-process-state.ts — an error-path chain and one Promise.resolve() field. Nothing on the emoji path, the proxy hoist or the bundle changed, and main-process-state was already imported by all three sibling phase modules, so no module was added to main's evaluation graph. The deterministic guard for the overlap — "creates the window without waiting for i18n and the native menu", which records the phase event list against a never-resolving i18n mock — still passes.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 335eedce-3249-4f66-8714-d026ec41b614

📥 Commits

Reviewing files that changed from the base of the PR and between 7ed86a9 and 2ed433c.

📒 Files selected for processing (16)
  • config/electron-builder.config.cjs
  • config/scripts/electron-builder-runtime-resources.test.mjs
  • config/tsconfig.tc.web.json
  • src/main/ipc/deferred-emoji-shortcode-dataset.test.ts
  • src/main/ipc/deferred-emoji-shortcode-dataset.ts
  • src/main/ipc/worktree-logic.ts
  • src/main/proxy-guarded-fetch-call-site-audit.test.ts
  • src/main/startup/desktop-startup-ordering.test.ts
  • src/main/startup/main-process-ready-foundation.ts
  • src/main/startup/main-process-ready-phase-ordering.test.ts
  • src/main/startup/main-process-ready.ts
  • src/main/startup/main-process-runtime-launch.ts
  • src/main/startup/main-process-state.ts
  • src/renderer/src/lib/workspace-emoji-shortcodes.ts
  • src/shared/emoji-shortcode-catalog.lazy.test.ts
  • src/shared/emoji-shortcode-catalog.ts
🚧 Files skipped from review as they are similar to previous changes (10)
  • src/main/startup/main-process-ready-foundation.ts
  • src/main/ipc/deferred-emoji-shortcode-dataset.ts
  • config/scripts/electron-builder-runtime-resources.test.mjs
  • src/main/ipc/worktree-logic.ts
  • src/main/startup/main-process-ready.ts
  • src/main/startup/main-process-runtime-launch.ts
  • src/renderer/src/lib/workspace-emoji-shortcodes.ts
  • src/shared/emoji-shortcode-catalog.ts
  • src/shared/emoji-shortcode-catalog.lazy.test.ts
  • config/electron-builder.config.cjs

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The PR replaces the static emoji dataset import with configurable synchronous and deferred loaders. Electron packages copy the dataset into Resources/node_modules, with tests verifying lazy require coverage and packaged resolution. Ready-phase initialization now runs concurrently with runtime launch. Initial proxy application uses a shared promise barrier before serve-mode fetchers and desktop relay setup. A static audit checks proxy-sensitive network call sites.

Merge Risk: ⚪ Minimal · up to 2ed43

Startup work now runs concurrently without allowing the runtime failure dialog to race locale initialization, while emoji shortcodes load on first main-process use from the packaged resource. The remaining reviewed changes have no actionable merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 15 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary startup performance change: window creation no longer waits for proxy application and i18n initialization.
Description check ✅ Passed The description is detailed, focused, and documents the motivation, implementation, measurements, correctness reasoning, trade-offs, compatibility considerations, and testing. It does not include the …
Full details: Docstring Coverage

Explanation

Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 15 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nwparker

nwparker commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up note for the Correctness section, spotted after opening: with i18n now concurrent, there is one error path that can read a translated string before initializeMainProcessI18nAndMenu settles — showRuntimeRpcStartupFailureDialog (src/main/runtime/runtime-rpc-startup-failure.ts:50,56) calls translateMain. It only fires when runtimeRpc.start() rejects, which is gated behind shellPathReady and therefore normally lands well after i18n's ~8 ms. If it ever did win the race, translateMain falls back to its English defaultValue rather than failing, so the worst case is an English error dialog for a non-English user on an already-broken launch. Flagging it rather than leaving it undocumented.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

The emoji-dataset deferral will throw at runtime in packaged builds: emojibase-data is a devDependency, so it is not shipped into the packaged app and the main-process require cannot resolve it.

Reviewed changes

  • Proxy apply moved off the window-creation pathapplyElectronProxySettings is parked on mainProcessState.initialProxyApplicationReady with its warn handlers attached, and awaited in launchDesktopMode/launchServeMode instead of the foundation phase.
  • i18n/menu parallelized with window creationinitializeMainProcessReady runs initializeMainProcessI18nAndMenu and initializeMainProcessRuntimeLaunch under Promise.all.
  • Emoji dataset behind an injected loaderemoji-shortcode-catalog.ts no longer statically imports the dataset; main registers a createRequire-based loader and the renderer keeps its eager import.
  • New ordering + laziness tests — a source-anchored phase-ordering test and an emoji-catalog parity test.

🚨 Main-process emoji require cannot resolve in packaged builds

emojibase-data is a devDependency (devDependencies.emojibase-data = 17.0.0, absent from dependencies), and it is not in PACKAGED_RUNTIME_PACKAGE_ROOTS. electron-builder does not ship devDependencies into app.asar, so requireFromMain('emojibase-data/en/shortcodes/emojibase.json') will throw Cannot find module the first time replaceKnownEmojiWithShortcodes runs — which is on every worktree name, not just emoji-bearing ones, because sanitizeWorktreeName calls it unconditionally. The @linear/sdk precedent cited in the comment does not transfer: @linear/sdk is a production dependency (^82.1.0), whereas emojibase-data is not.

Technical details
# emojibase-data is not shipped to packaged builds

## Affected sites
- src/main/ipc/deferred-emoji-shortcode-dataset.ts:11 — bare runtime `requireFromMain('emojibase-data/en/shortcodes/emojibase.json')`.
- src/main/ipc/worktree-logic.ts:41 — `replaceKnownEmojiWithShortcodes(input)` runs unconditionally in `sanitizeWorktreeName`, so worktree create/naming fails wholesale, not only for emoji.

## Why it is missed by the safety net
- `verifyPackagedMainRuntimeDeps` (config/packaged-runtime-node-modules.cjs:240) regex-matches literal `require("…")` in the main bundle; `requireFromMain('…')` via `createRequire` does not match, so packaging CI won't flag it.
- electron-builder only packages `dependencies`, and this repo additionally curates a `PACKAGED_RUNTIME_PACKAGE_ROOTS` closure for bare `require()` calls — `emojibase-data` is in neither.

## Required outcome
- The main-process emoji dataset must resolve in a packaged build, or the deferral must not throw when it can't (fail closed to a safe fallback rather than aborting worktree naming).

## Suggested approach
- Move `emojibase-data` (and its peer `emojibase`) into `dependencies`, and add `emojibase-data` to `PACKAGED_RUNTIME_PACKAGE_ROOTS` so the same closure rule that already covers `@linear/sdk` applies. Then extend `verifyPackagedMainRuntimeDeps` (or a contract test) to cover `createRequire`-style runtime requires so this class of regression fails CI.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ipc/deferred-emoji-shortcode-dataset.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 11157100-672a-48c7-a9ac-183542a04390

📥 Commits

Reviewing files that changed from the base of the PR and between 3eec77c and bc1307d.

📒 Files selected for processing (12)
  • config/electron-builder.config.cjs
  • config/scripts/electron-builder-runtime-resources.test.mjs
  • src/main/ipc/deferred-emoji-shortcode-dataset.ts
  • src/main/ipc/worktree-logic.ts
  • src/main/startup/main-process-ready-foundation.ts
  • src/main/startup/main-process-ready-phase-ordering.test.ts
  • src/main/startup/main-process-ready.ts
  • src/main/startup/main-process-runtime-launch.ts
  • src/main/startup/main-process-state.ts
  • src/renderer/src/lib/workspace-emoji-shortcodes.ts
  • src/shared/emoji-shortcode-catalog.lazy.test.ts
  • src/shared/emoji-shortcode-catalog.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Comment thread src/main/ipc/worktree-logic.ts
Comment thread src/main/startup/main-process-ready.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Packaging fix for the lazy emoji dataset require — copies the single 166 KB emojibase-data JSON into Resources/node_modules via commonExtraResources, so main's createRequire'd specifier resolves out of Resources/node_modules in packaged builds and resolves the prior MODULE_NOT_FOUND crash on every worktree name.
  • Two new packaging contract gates — one scans src/main for every bare createRequire specifier and asserts it is covered by the packaged resource plan on all three platforms (with a Windows-only corridor for native loaders), the other resolves the copied dataset through a simulated app.asarResources/node_modules walk.
  • Comment correctiondeferred-emoji-shortcode-dataset.ts now documents the actual resolution path (Resources/node_modules, not app.asar) and the 49 MB package-root rationale.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 7bd8e126-8019-4f4d-8e3e-34480570227d

📥 Commits

Reviewing files that changed from the base of the PR and between bc1307d and 4613146.

📒 Files selected for processing (1)
  • src/main/proxy-guarded-fetch-call-site-audit.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Comment thread src/main/proxy-guarded-fetch-call-site-audit.test.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor suggestion inline about the audit's regex coverage.

Reviewed changes

  • New proxy-guard call-site audit test — added src/main/proxy-guarded-fetch-call-site-audit.test.ts, a static source scan that fails CI when a net.fetch/net.request under src/main passes a session:/partition: option, or when a non-net .fetch( call site appears outside a three-entry allowlist.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/proxy-guarded-fetch-call-site-audit.test.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Closed the fetch-audit regex blind spots — rule 2 now classifies each .fetch( receiver by extracting the trailing identifier (FETCH_CALL + RECEIVER_IDENTIFIER against a DEFAULT_SESSION_RECEIVERS set), so the inline chained session.fromPartition(...).fetch(, the optional-chained s?.fetch(, and member ctx.session.fetch( forms are counted instead of slipping past both rules. Rule 1's SESSION_SCOPED_OPTION additionally matches the { url, session } object shorthand both net.request overloads accept.

This directly resolves the one open finding from the prior review (the NON_NET_FETCH_CALL blind spot). The change is test-only: git diff bc1307d8bac..HEAD --stat -- . ':!*.test.ts' ':!*.test.mjs' is empty, so no production behavior moved. Verified locally that the audit test passes and that the allowlist counts (2/2/1) still match the real call sites.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

…nd i18n

Three independent, measured startup wins, all free:

1. Park the initial Chromium proxy apply on `mainProcessState` instead of
   awaiting it mid-`initializeReadyFoundation`. `setProxy` still starts at the
   identical moment; the default-session request guard (which holds, not
   cancels) is what actually fences fetchers on it, so only window creation
   stops waiting. Runtime launch still awaits it before the desktop relay and
   before every headless-serve fetcher.
2. Run `initializeMainProcessI18nAndMenu` concurrently with
   `initializeMainProcessRuntimeLaunch`. Nothing in window creation reads a
   translated string or the native menu.
3. Load `emojibase-data` in main through `createRequire` on first use instead
   of a static import, keeping 166 KB of JSON off `out/main/index.js` and its
   ~2 ms parse off every launch. The renderer keeps its eager copy unchanged.

out/main/index.js 7,210,071 -> 7,040,147 bytes. No renderer behaviour changes.
app.asar carries no node_modules, so main's bare requires resolve only out of
Resources/node_modules. emojibase-data is a devDependency and is not in the
packaged runtime allowlist, so the new createRequire in
deferred-emoji-shortcode-dataset.ts threw MODULE_NOT_FOUND in every packaged
build — breaking sanitizeWorktreeName, and with it workspace creation.

Copy the single 166 KB dataset (not the 49 MB package root) into
Resources/node_modules, and gate every createRequire'd bare specifier in
src/main against the packaged resource plan. verifyPackagedMainRuntimeDeps
cannot catch these: the bundler renames the require binding.
…session guard

The hoist relies on installElectronProxyRequestGuard(session.defaultSession) holding every app-owned request until the persisted proxy lands. Nothing enforced that every fetcher actually lands on defaultSession. Two source-anchored rules do now: no net.fetch/net.request may name a session/partition, and every non-net .fetch( call site is counted against an allowlist.
…tch call-site audit

The audit caught `net.request({ session: x })` and `ident.fetch(`, but not the two
shapes a real regression is just as likely to take: the `{ url, session }` shorthand
that both `net.request` overloads accept, and a receiver with no bare identifier
(`session.fromPartition(...).fetch(`, `ctx.session.fetch(`). Rule 1 now also matches
the shorthand key; rule 2 scans every `.fetch(` and excludes only a literal
`net`/`globalThis`/`global` receiver. Audited counts are unchanged (2/2/1).
…n it

TS6307: the composite web project lists src/main/ipc/worktree-logic.ts, which
now imports the deferred dataset loader, and the shared lazy test reached into
src/main from a project that has no src/main files. Add the loader to
tsconfig.tc.web.json and move the cross-project case into a src/main test.

Also close the last two review gaps: gate the runtime-RPC startup failure
dialog (the only launch-phase translateMain reader) on a published i18n
barrier so a concurrent i18n phase cannot leave a non-English user with the
English fallback, and let the fetch call-site audit match `net.fetch (url)`.
@nwparker
nwparker force-pushed the perf/startup-parallel-awaits branch from 99fe199 to 2ed433c Compare September 4, 2026 03:46
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Closed the i18n race rather than just documenting itmainProcessState.mainProcessI18nReady is now published synchronously in main-process-ready.ts (as initializeMainProcessI18nAndMenu().catch(() => {})) before the launch phase starts, and launchDesktopMode chains the runtime-RPC startup failure dialog off it with void state.mainProcessI18nReady.then(...). The wait is on i18n only, never on the modal, and remains failure-only. The uncatched i18nAndMenuReady is still what Promise.all awaits, so an i18n failure still surfaces through initializeMainProcessReady while the dialog still fires with fallback strings.
  • Scoped the deferred emoji loader to the projects that own itconfig/tsconfig.tc.web.json now lists deferred-emoji-shortcode-dataset.ts (it is imported by worktree-logic.ts, which tc.web already includes), and the main-side parity case moved out of the shared emoji-shortcode-catalog.lazy.test.ts into a dedicated src/main/ipc/deferred-emoji-shortcode-dataset.test.ts, restoring the shared project's no-src/main boundary.
  • Tightened the fetch-audit regexesFETCH_CALL and NET_REQUEST_CALL now allow whitespace before (, closing the net.fetch (…) / x .fetch(…) hole.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit 1c4c6b7 into main Sep 4, 2026
33 checks passed
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.

1 participant