From a04a75f7f3fb4895c569039c72d0ba37254281e7 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 01:23:27 -0700 Subject: [PATCH 1/3] test(preload): per-pane IPC channels must share one ipcRenderer listener #1015. Against a real EventEmitter as ipcRenderer, 12 goal-loop:changed or dictation:stream-transcript subscribers must create one listener, deliver every event to all of them, remove the listener with the last unsubscribe, and emit no MaxListeners warning. A subscriber that throws must not starve the others. Co-Authored-By: Claude Opus 5 (1M context) --- src/preload/api/sharedListeners.test.ts | 56 +++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 src/preload/api/sharedListeners.test.ts diff --git a/src/preload/api/sharedListeners.test.ts b/src/preload/api/sharedListeners.test.ts new file mode 100644 index 000000000..b1797da4e --- /dev/null +++ b/src/preload/api/sharedListeners.test.ts @@ -0,0 +1,56 @@ +import { EventEmitter } from 'node:events' +import { afterEach, describe, expect, it, vi } from 'vitest' + +// #1015: at startup the dev app warned "11 goal-loop:changed listeners added +// to [IpcRenderer]. MaxListeners is 10", and the same for +// dictation:stream-transcript. It is not a leak: every mounted pane +// subscribes once (GoalLoopPane, the composer's dictation hook) and cleans up. +// But each subscription was its own ipcRenderer listener, so 11 panes crossed +// Node's warning threshold, and that noise hides the next real leak. +// +// ipcRenderer is a real EventEmitter here, so listenerCount and the +// MaxListeners warning behave exactly as in Electron's renderer. +const ipc = vi.hoisted(() => ({ renderer: null as unknown as EventEmitter })) +vi.mock('electron', async () => { + const { EventEmitter: Emitter } = await import('node:events') + ipc.renderer = new Emitter() + return { ipcRenderer: ipc.renderer } +}) + +const { goalLoopApi } = await import('./goalLoop.js') +const { dictationApi } = await import('./dictation.js') + +afterEach(() => { vi.restoreAllMocks() }) + +describe.each([ + ['goal-loop:changed', (cb: (payload: unknown) => void) => goalLoopApi.onGoalLoopChanged(() => cb(undefined))], + ['dictation:stream-transcript', (cb: (payload: unknown) => void) => dictationApi.onDictationStreamTranscript(cb as never)], +])('%s', (channel, subscribe) => { + it('many panes share ONE ipcRenderer listener, every pane still hears every event, and the last unsubscribe removes it', () => { + const warning = vi.fn() + process.on('warning', warning) + const received = Array.from({ length: 12 }, () => vi.fn()) + const unsubscribes = received.map(cb => subscribe(cb)) + expect(ipc.renderer.listenerCount(channel)).toBe(1) + + ipc.renderer.emit(channel, {}, { sessionId: 's1' }) + for (const cb of received) expect(cb).toHaveBeenCalledTimes(1) + + unsubscribes.slice(0, 11).forEach(unsubscribe => unsubscribe()) + expect(ipc.renderer.listenerCount(channel)).toBe(1) + unsubscribes[11]!() + expect(ipc.renderer.listenerCount(channel)).toBe(0) + process.off('warning', warning) + expect(warning).not.toHaveBeenCalled() + }) + + it('one subscriber that throws does not starve the others', () => { + vi.spyOn(console, 'error').mockImplementation(() => undefined) + const after = vi.fn() + const off1 = subscribe(() => { throw new Error('pane crashed') }) + const off2 = subscribe(after) + ipc.renderer.emit(channel, {}, {}) + expect(after).toHaveBeenCalledTimes(1) + off1(); off2() + }) +}) From 59f49c5b1db83b79ba6e10ba4c072c05e7b0fc97 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 01:23:28 -0700 Subject: [PATCH 2/3] fix(preload): one shared ipcRenderer listener for per-pane channels #1015. Every mounted GoalLoopPane and composer subscribed its own ipcRenderer listener, so eleven panes crossed Node's MaxListeners of 10 and the dev app warned "Possible EventEmitter memory leak" at every startup. It was not a leak, but a permanent false alarm hides the next real one, and raising the limit would silence that alarm. subscribeShared keeps one relay listener per channel, fanned out to a set of subscribers. The relay is removed with the last subscriber, and each subscriber is isolated from another's throw. goal-loop:changed and dictation:stream-transcript use it. LSP diagnostics keeps its permanent listener on purpose (see lsp.ts: code blocks mount and unmount while scrolling). Closes #1015 Co-Authored-By: Claude Opus 5 (1M context) --- src/preload/api/dictation.ts | 5 +-- src/preload/api/goalLoop.ts | 9 +++--- src/preload/api/ipc.ts | 61 +++++++++++++++++++++++++++++++++--- 3 files changed, 64 insertions(+), 11 deletions(-) diff --git a/src/preload/api/dictation.ts b/src/preload/api/dictation.ts index 99511ef6f..be7e3d39d 100644 --- a/src/preload/api/dictation.ts +++ b/src/preload/api/dictation.ts @@ -1,6 +1,6 @@ import { ipcRenderer } from 'electron' -import { subscribe } from '@preload/api/ipc.js' +import { subscribe, subscribeShared } from '@preload/api/ipc.js' import type { DictationApiKeyStatus, DictationApiKeySetResult, @@ -31,8 +31,9 @@ export const dictationApi = { onDictationHotkeyUp: (handler: (payload: { binding: string }) => void): Unsub => subscribe('dictation:hotkey-up', handler), + // Shared (#1015): every mounted composer subscribes. onDictationStreamTranscript: (handler: (payload: DictationStreamTranscriptEvent) => void): Unsub => - subscribe('dictation:stream-transcript', handler), + subscribeShared('dictation:stream-transcript', handler), startDictationStream: (params: { provider: DictationProvider diff --git a/src/preload/api/goalLoop.ts b/src/preload/api/goalLoop.ts index 1d9bbe3e0..b90d9e85e 100644 --- a/src/preload/api/goalLoop.ts +++ b/src/preload/api/goalLoop.ts @@ -1,4 +1,5 @@ import { ipcRenderer } from 'electron' +import { subscribeShared } from '@preload/api/ipc.js' import type { GoalLoopControlAction, GoalLoopState } from '@shared/types/goalLoop.js' export type GoalLoopControlRequest = { @@ -12,9 +13,7 @@ export const goalLoopApi = { ipcRenderer.invoke('goal-loop:read', sessionIds), controlGoalLoop: (request: GoalLoopControlRequest): Promise => ipcRenderer.invoke('goal-loop:control', request), - onGoalLoopChanged: (listener: () => void): (() => void) => { - const handler = () => listener() - ipcRenderer.on('goal-loop:changed', handler) - return () => { ipcRenderer.removeListener('goal-loop:changed', handler) } - }, + // Shared (#1015): every mounted GoalLoopPane subscribes. + onGoalLoopChanged: (listener: () => void): (() => void) => + subscribeShared('goal-loop:changed', () => listener()), } diff --git a/src/preload/api/ipc.ts b/src/preload/api/ipc.ts index 7ccfd3516..5b4f21961 100644 --- a/src/preload/api/ipc.ts +++ b/src/preload/api/ipc.ts @@ -8,16 +8,69 @@ import type { Unsub } from '@preload/api/types.js' // and returns the resulting Unsub. This keeps domain modules tiny — // they don't each reimplement "add a listener, return a remover." // -// Why one listener per caller (not multiplexed): +// Why one listener per caller (not multiplexed) by default: // Most onX consumers subscribe once at app mount with a single // callback that dispatches by sessionId. ipcRenderer.on fans the // event out to every registered listener cheaply; we don't need a -// dedupe layer here. The ONE case that needs multiplexing is LSP -// diagnostics (see ./lsp.ts) — a set-of-subscribers pattern that -// survives hot-module reloads without leaking N IPC listeners. +// dedupe layer for those. Channels that every mounted PANE subscribes +// to use subscribeShared below instead. export function subscribe(channel: string, cb: (payload: T) => void): Unsub { const listener = (_evt: unknown, payload: T) => cb(payload) ipcRenderer.on(channel, listener) return () => ipcRenderer.removeListener(channel, listener) } + +/** + * One ipcRenderer listener per channel, fanned out to every subscriber. + * + * WHY (#1015): some channels are subscribed once per mounted pane + * (goal-loop:changed from GoalLoopPane, dictation:stream-transcript from each + * composer). Eleven panes crossed Node's default of 10 listeners, and the dev + * app warned "Possible EventEmitter memory leak" at every startup. It was not + * a leak (every pane unsubscribes), but a permanent false alarm hides the + * next real one. Raising MaxListeners would silence exactly that alarm. + * LSP diagnostics (./lsp.ts) solved the same problem with its own Set; this is + * that pattern made reusable. + * + * The relay is removed with the last subscriber, so no listener outlives its + * users. Each subscriber is isolated: one pane that throws must not stop the + * others from hearing the event, which separate ipcRenderer listeners did not + * guarantee either (EventEmitter stops at the first throw). + */ +type SharedChannel = { subscribers: Set<(payload: unknown) => void>; relay: (event: unknown, payload: unknown) => void } +const sharedChannels = new Map() + +export function subscribeShared(channel: string, cb: (payload: T) => void): Unsub { + let shared = sharedChannels.get(channel) + if (!shared) { + const subscribers = new Set<(payload: unknown) => void>() + const relay = (_event: unknown, payload: unknown) => { + // Copied first: a subscriber may unsubscribe (a pane unmounting in + // response to the event) while the loop runs. + for (const subscriber of [...subscribers]) { + try { + subscriber(payload) + } catch (error) { + console.error(`[ipc] a ${channel} subscriber threw:`, error) + } + } + } + ipcRenderer.on(channel, relay) + shared = { subscribers, relay } + sharedChannels.set(channel, shared) + } + // A fresh wrapper per call: the same callback subscribed twice is two + // subscriptions, each removed by its own Unsub, as with ipcRenderer.on. + const subscription = (payload: unknown) => cb(payload as T) + shared.subscribers.add(subscription) + return () => { + const current = sharedChannels.get(channel) + if (!current || !current.subscribers.delete(subscription)) return + if (current.subscribers.size === 0) { + ipcRenderer.removeListener(channel, current.relay) + sharedChannels.delete(channel) + } + } +} + From ac0878c678a839266e70dc3aaacca53b92798d16 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 01:54:29 -0700 Subject: [PATCH 3/3] fix(preload): share the other per-pane channels too; make the warning check real From the #1039 review (CHANGES REQUESTED): - MEDIUM: record-session:started and record-session:stopping are also subscribed per rendered pane (every Feed mounts a RenderShapeCaptureProvider), so 11 panes still warned at startup. tldr:changed and goal:changed do the same while a peek is up. All four use subscribeShared, and the test covers all six channels. - LOW: Node emits MaxListenersExceededWarning on process.nextTick, so the test's warning spy must outlive a tick. Otherwise the check could never fail. - NIT: the comment now says why LSP diagnostics keeps its own permanent listener. Co-Authored-By: Claude Opus 5 (1M context) --- src/preload/api/devDebug.ts | 19 +++++-------------- src/preload/api/ipc.ts | 7 ++++--- src/preload/api/sharedListeners.test.ts | 11 ++++++++++- src/preload/api/tldr.ts | 17 +++++++---------- 4 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/preload/api/devDebug.ts b/src/preload/api/devDebug.ts index da307df8f..36e36691d 100644 --- a/src/preload/api/devDebug.ts +++ b/src/preload/api/devDebug.ts @@ -2,6 +2,7 @@ import { ipcRenderer } from 'electron' import type { DevDebugConfig, PasteDebugSession } from '@preload/api/types.js' import type { RenderShapeAppendResult } from '@shared/types/renderShapes.js' +import { subscribeShared } from '@preload/api/ipc.js' export const devDebugApi = { getDevDebugConfig: (): Promise => @@ -55,16 +56,11 @@ export const devDebugApi = { // event — for an idle restored pane that is whenever the user first // prompts it, unboundedly after Feed mount, so every renderer-side poll // schedule loses the race. Same subscribe shape as lsp:diagnostics. + // Shared (#1015/#1039 review): every Feed mounts a RenderShapeCaptureProvider + // that subscribes, so 11 rendered panes crossed MaxListeners. onSessionRecordingStarted: ( cb: (payload: { sessionId: string; generation: string }) => void, - ): (() => void) => { - const listener = ( - _evt: unknown, - payload: { sessionId: string; generation: string }, - ): void => cb(payload) - ipcRenderer.on('record-session:started', listener) - return () => ipcRenderer.removeListener('record-session:started', listener) - }, + ): (() => void) => subscribeShared('record-session:started', cb), // Natural provider exit is a two-step close: main keeps the recorder open // for a short grace window and asks the renderer to flush its coalesced // shape counters. The renderer acknowledges by calling finish below; main's @@ -76,12 +72,7 @@ export const devDebugApi = { // closing fresh state. onSessionRecordingStopping: ( cb: (payload: { sessionId: string; generation?: string }) => void, - ): (() => void) => { - const listener = (_evt: unknown, payload: { sessionId: string; generation?: string }): void => - cb(payload) - ipcRenderer.on('record-session:stopping', listener) - return () => ipcRenderer.removeListener('record-session:stopping', listener) - }, + ): (() => void) => subscribeShared('record-session:stopping', cb), finishSessionRecordingStop: (sessionId: string, generation?: string): Promise => ipcRenderer.invoke('record-session:finish-stop', sessionId, generation), readRenderShapeSightings: (): Promise<{ diff --git a/src/preload/api/ipc.ts b/src/preload/api/ipc.ts index 5b4f21961..107e624e3 100644 --- a/src/preload/api/ipc.ts +++ b/src/preload/api/ipc.ts @@ -30,8 +30,10 @@ export function subscribe(channel: string, cb: (payload: T) => void): Unsub { * app warned "Possible EventEmitter memory leak" at every startup. It was not * a leak (every pane unsubscribes), but a permanent false alarm hides the * next real one. Raising MaxListeners would silence exactly that alarm. - * LSP diagnostics (./lsp.ts) solved the same problem with its own Set; this is - * that pattern made reusable. + * LSP diagnostics (./lsp.ts) solved the same problem with its own Set and keeps + * it on purpose: its listener stays installed for the renderer's lifetime, + * because code blocks mount and unmount constantly while scrolling. This one + * removes the relay with its last subscriber. * * The relay is removed with the last subscriber, so no listener outlives its * users. Each subscriber is isolated: one pane that throws must not stop the @@ -73,4 +75,3 @@ export function subscribeShared(channel: string, cb: (payload: T) => void): U } } } - diff --git a/src/preload/api/sharedListeners.test.ts b/src/preload/api/sharedListeners.test.ts index b1797da4e..6da45f8f8 100644 --- a/src/preload/api/sharedListeners.test.ts +++ b/src/preload/api/sharedListeners.test.ts @@ -19,14 +19,20 @@ vi.mock('electron', async () => { const { goalLoopApi } = await import('./goalLoop.js') const { dictationApi } = await import('./dictation.js') +const { devDebugApi } = await import('./devDebug.js') +const { tldrApi } = await import('./tldr.js') afterEach(() => { vi.restoreAllMocks() }) describe.each([ ['goal-loop:changed', (cb: (payload: unknown) => void) => goalLoopApi.onGoalLoopChanged(() => cb(undefined))], ['dictation:stream-transcript', (cb: (payload: unknown) => void) => dictationApi.onDictationStreamTranscript(cb as never)], + ['record-session:started', (cb: (payload: unknown) => void) => devDebugApi.onSessionRecordingStarted(cb as never)], + ['record-session:stopping', (cb: (payload: unknown) => void) => devDebugApi.onSessionRecordingStopping(cb as never)], + ['tldr:changed', (cb: (payload: unknown) => void) => tldrApi.onTldrChanged(cb as never)], + ['goal:changed', (cb: (payload: unknown) => void) => tldrApi.onGoalChanged(cb as never)], ])('%s', (channel, subscribe) => { - it('many panes share ONE ipcRenderer listener, every pane still hears every event, and the last unsubscribe removes it', () => { + it('many panes share ONE ipcRenderer listener, every pane still hears every event, and the last unsubscribe removes it', async () => { const warning = vi.fn() process.on('warning', warning) const received = Array.from({ length: 12 }, () => vi.fn()) @@ -40,6 +46,9 @@ describe.each([ expect(ipc.renderer.listenerCount(channel)).toBe(1) unsubscribes[11]!() expect(ipc.renderer.listenerCount(channel)).toBe(0) + // Node emits MaxListenersExceededWarning on process.nextTick, so the spy + // must outlive a tick or this check can never fail (#1039 review). + await new Promise(resolve => setImmediate(resolve)) process.off('warning', warning) expect(warning).not.toHaveBeenCalled() }) diff --git a/src/preload/api/tldr.ts b/src/preload/api/tldr.ts index 21d6c029d..fc46a3cb9 100644 --- a/src/preload/api/tldr.ts +++ b/src/preload/api/tldr.ts @@ -1,4 +1,5 @@ import { ipcRenderer } from 'electron' +import { subscribeShared } from '@preload/api/ipc.js' import type { TldrEnforcementStatus, TldrHistoryEntry, TldrRecord, TldrUpdate } from '@shared/types/tldr.js' export const tldrApi = { @@ -13,15 +14,11 @@ export const tldrApi = { readTldrHistory: (identity: string): Promise => ipcRenderer.invoke('tldr:history', identity), readGoals: (identities: string[]): Promise> => ipcRenderer.invoke('goal:read', identities), readGoalHistory: (identity: string): Promise => ipcRenderer.invoke('goal:history', identity), - onGoalChanged: (listener: (update: TldrUpdate) => void): (() => void) => { - const handler = (_event: Electron.IpcRendererEvent, update: TldrUpdate) => listener(update) - ipcRenderer.on('goal:changed', handler) - return () => { ipcRenderer.removeListener('goal:changed', handler) } - }, + // Shared (#1039 review): every visible pane's peek subscribes while a + // TLDR/Goal peek is up. + onGoalChanged: (listener: (update: TldrUpdate) => void): (() => void) => + subscribeShared('goal:changed', listener), readTldrEnforcement: (identities: string[]): Promise> => ipcRenderer.invoke('tldr:enforcement', identities), - onTldrChanged: (listener: (update: TldrUpdate) => void): (() => void) => { - const handler = (_event: Electron.IpcRendererEvent, update: TldrUpdate) => listener(update) - ipcRenderer.on('tldr:changed', handler) - return () => { ipcRenderer.removeListener('tldr:changed', handler) } - }, + onTldrChanged: (listener: (update: TldrUpdate) => void): (() => void) => + subscribeShared('tldr:changed', listener), }