Skip to content

perf(runtime): reduce lifecycle and media churn#575

Open
JustYannicc wants to merge 12 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-lifecycle-media-wave2
Open

perf(runtime): reduce lifecycle and media churn#575
JustYannicc wants to merge 12 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-lifecycle-media-wave2

Conversation

@JustYannicc

Copy link
Copy Markdown
Collaborator

What changed

  • Consolidates the wave-2 lifecycle/media performance work for script command timeout cleanup, native menubar update caching, Whisper upload streaming, Gemini prompt streaming, extension one-shot timer pruning, and lazy STT file reads.
  • Adds focused harness coverage for the consolidated runtime/media paths and a minimal TypeScript import test helper stub path for clean-base harnessing.

Why

  • Reduces long-running runtime churn from stale timeout handles, repeated native tray work, retained extension one-shot timers, eager STT file buffers, and avoidable upload/response buffering.
  • Keeps the wave-2 fixes together on a clean origin/main base so upstream can review one lifecycle/media PR instead of six overlapping branches.

Compatibility impact

  • Preserves script command execution behavior, including output-limit failure handling and timeout cleanup.
  • Preserves MenuBarExtra fallback title/tooltip/icon behavior while skipping unchanged native tray work.
  • Preserves Whisper/STT provider routing: whisper.cpp uses file-path transcription without reading a Node buffer; buffer-backed cloud/local providers still read once and clean up temp files.
  • Preserves Gemini prompt output/error semantics while using streamGenerateContent SSE.
  • Preserves extension no-view/background lifecycle cleanup by scoping timers/listeners per extension evaluation and pruning fired one-shots.

How tested

  • node --test scripts/test-script-command-runner.mjs scripts/test-menubar-native-update-cache.mjs scripts/test-ai-provider-whisper-upload.mjs scripts/test-ai-provider-gemini-prompt-streaming.mjs scripts/test-extension-lifecycle-sandbox.mjs scripts/test-whisper-file-buffer-read.mjs
  • ./node_modules/.bin/tsc -p tsconfig.main.json --noEmit --pretty false
  • ./node_modules/.bin/tsc -p tsconfig.renderer.json --noEmit --pretty false (run because ExtensionView.tsx changed; fails on existing non-ExtensionView.tsx renderer errors in App, CameraExtension, LiquidGlassSurface, launcher hooks, raycast-api helpers, settings, and quicklink icons)
  • Codex LSP diagnostics with severity=error for src/main/ai-provider.ts, src/main/main.ts, src/main/menubar-native-update-cache.ts, src/main/script-command-runner.ts, src/main/whisper-transcribe-file.ts, and src/renderer/src/ExtensionView.tsx: no diagnostics found
  • git diff --check

Performance evidence

  • Script output-limit paths clear the active timeout handle after stdout/stderr overflow (before=1 after=0 cleared=1).
  • Menubar stable title ticks reduce native menu rebuild/context-menu work from 60 calls to 1 call.
  • Whisper multipart upload writes 7 buffers and reuses the 8 MiB audio buffer directly, avoiding a full multipart Buffer.concat copy.
  • Gemini prompt streaming yields after the first 66 bytes of a 179-byte SSE response, before response end; legacy buffered response was 75 bytes before first yield.
  • Extension lifecycle harness prunes 1000 fired timeouts and 1000 fired RAFs to 0 tracked one-shot handles and clears scoped listeners/timers on unmount/re-evaluation.
  • whisper.cpp file transcription avoids the legacy eager 70 MiB Node buffer read (readCalls=0, nodeBufferBytes=0), while buffer-backed providers still read once.

Stack validation

  • Built from clean origin/main on codex/consolidate-lifecycle-media-wave2.
  • Cherry-picked/adapted only the wave-2 lifecycle/media commits, with clean-base harness drift limited to scripts/lib/ts-import.mjs stub support and script-runner assertions unrelated to this wave.
  • No coordinator notes, prompt files, local playbooks, pnpm lockfile, or generated package-manager metadata are included.

Replaces

@shobhit99

Copy link
Copy Markdown
Contributor

Reviewed the full diff. Five of the six consolidated changes are clean, behavior-preserving wins; the sixth (the largest file, ExtensionView.tsx) ships ~341 lines of production-unreachable code, so its headline benefit is not actually delivered.

Strengths

  • script-command-runner.ts — moving clearTimeout into finalize() (L533, L543-546) is a genuine fix: the output-limit overflow path calls finalize() directly, so the timeout handle is now cleared there too instead of relying solely on the close/error listeners. No regression (clearing an already-fired timer is a no-op).
  • ai-provider.ts Gemini — switching streamGemini to :streamGenerateContent?alt=sse and routing both prompt + chat through the shared streamGeminiSSE (with throwOnNoText) preserves the no-text/finishReason error semantics while streaming. readResponseBody is fully removed with no remaining references. Real time-to-first-byte win.
  • ai-provider.ts transcribeAudio — writing multipart parts individually via writeBufferedRequestParts and reusing the audio Buffer directly instead of Buffer.concat-ing an 8 MiB+ body is byte-identical, keeps the same Content-Length, and avoids a large copy. Nice.
  • main.ts menubar cache — dedupes title/tooltip/menu/icon native calls; the native menu is only rebuilt when the serialized items JSON changes; file-backed icons always refresh (hasFileBackedMenuBarNativeIcon), preserving correctness; the "never clear a non-empty tooltip" semantics are preserved. Title logic correctly reads the cached updateState.lastResolvedTrayIconOk on icon-skipped ticks. Solid.
  • whisper-transcribe-file.ts — clean DI extraction; the lazy shouldReadWhisperFileAudioBuffer read genuinely avoids buffering the audio file for whisper.cpp/native, and native early-returns '' before the API-key checks, matching the original resolution-chain behavior.

Concerns / potential bugs

  • ExtensionView.tsx — the new lifecycle scope is dead code in production (blocking). The ExtensionView diff is a single hunk (@@ -3362,29 +3362,370 @@) that only redefines TimerRegistry/createTimerRegistry/clearTimerRegistry and adds createExtensionLifecycleScope + createScopedHostProxy + trackEventListener/untrackEventListener + the one-shot-pruning trackTimeout/trackRaf. None of these new functions are called by production code. loadExtensionExport and its wrapper invocation (ExtensionView.tsx ~L3900-4008 on base) are unchanged: the extension still receives the real globalThis (no scopedWindow/scopedDocument), and still uses the old inline trackTimeout/trackRaf (~L3927-3944) that neither prune fired one-shots nor populate timeoutClearers/rafClearers. A repo-wide grep finds zero references to createExtensionLifecycleScope/scopedWindow/trackEventListener outside the definitions and test-extension-lifecycle-sandbox.mjs.
    • Consequence 1: extensions still call the real window.addEventListener directly, so registry.eventListeners is always empty and the new removeEventListener loop in clearTimerRegistry is a no-op — the advertised "clears scoped listeners on unmount" does not happen at runtime.
    • Consequence 2: fired one-shot timeouts/RAFs still accumulate in registry.timeouts/registry.rafs for the extension's lifetime (the exact "churn" the PR claims to fix) because the production trackTimeout isn't the pruning one.
    • The passing test-extension-lifecycle-sandbox.mjs exercises createExtensionLifecycleScope in isolation, which gives false confidence that the fix is wired. Net: no functional regression (fallback clearers keep existing unmount cleanup working), but the PR's headline extension lifecycle/media benefit is not delivered, and 341 lines of unreachable code + misleading test coverage would ship.
    • Fix: either wire createExtensionLifecycleScope into loadExtensionExport (pass scopedWindow/scopedDocument into the fn(...) call and swap the inline timer trackers for the scope's), or drop the ExtensionView portion from this consolidation until the wiring lands.

Suggestions

🔴 Needs changes before merge — the five main-process changes are safe and worth landing, but the ExtensionView.tsx lifecycle scope must be wired into loadExtensionExport or removed; as-is it is inert and its tests imply a fix that does not take effect.

🤖 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 the inert ExtensionView lifecycle scope.

What changed:

  • Wired createExtensionLifecycleScope into production loadExtensionExport.
  • The bundle wrapper now receives scopedWindow for global/globalThis/window/self, scopedDocument for document, and the lifecycle scope's timer APIs for bare setTimeout/setInterval/requestAnimationFrame calls.
  • Removed the old inline production timer wrappers that did not prune fired one-shots.
  • Hardened timer/RAF callback this binding and string timeout evaluation so they stay in the scoped window/document context.
  • Updated scripts/test-extension-lifecycle-sandbox.mjs so it asserts production wrapper wiring in addition to isolated lifecycle-scope behavior.

Review-thread state checked via GraphQL: PR #575 has 0 reviewThreads, 0 reviews, and 1 top-level maintainer comment; this addresses that actionable comment. The menubar iconEmoji suggestion was checked too: iconEmoji is still used in src/main/main.ts.

Verification:

  • node --test scripts/test-ai-provider-gemini-prompt-streaming.mjs scripts/test-ai-provider-whisper-upload.mjs scripts/test-extension-lifecycle-sandbox.mjs scripts/test-menubar-native-update-cache.mjs scripts/test-script-command-runner.mjs scripts/test-whisper-file-buffer-read.mjs ✅ 35 passed
  • git diff --check

@JustYannicc

Copy link
Copy Markdown
Collaborator Author

Follow-up system attribution and amendment (2026-07-10): a full-quit A/B on the affected Mac showed WindowServer staying around 68–73% CPU and MenuBarAgent around 18–22% with SuperCmd completely absent. After relaunch and startup settlement, SuperCmd itself was 0.0–0.1% while the compositor/menu-bar load persisted. This does not support SuperCmd as the cause of the system-wide volume/menu-bar stutter. However, the standalone PR still had one avoidable SuperCmd-side path: file-backed tray icons were decoded/resized and sent through setImage on every accepted title tick. Commit e529dcb adds file identity tracking (path + mtime + size), skips unchanged native image refreshes, and still refreshes when the icon file or image inputs change. Validation: focused menubar test 8/8, full PR test command 64/64, main TypeScript check passed, and git diff check 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.

2 participants