Fix stale Pear message panes after idle#86
Conversation
|
CodeAnt AI is reviewing your PR. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughAdds broker-backed message reconciliation: shared IPC types and handlers, session-scoped event-stream rebind/diagnostics, Relay message normalization and reconcile API, renderer debounced reconciler merging messages into the agent store, tests, preload/App wiring, and a CLI repro plus config updates. ChangesMessage Reconciliation Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
codeant-ai
Bot
added
the
size:XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a message reconciliation mechanism and event stream rebinding for the agent-relay broker, allowing the renderer to fetch and merge canonical chat messages upon window focus, visibility changes, or broker status updates. The feedback highlights several opportunities to prevent potential runtime crashes by adding defensive guards around message.from and reaction.agents in the main process, as well as guarding window.pear and window.pear.broker references in the renderer hook.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function senderNameFromRelayMessage(message: RelayMessage): string { | ||
| return message.from.name || message.from.id || 'unknown' | ||
| } |
There was a problem hiding this comment.
Guard against message.from being null or undefined to prevent runtime TypeError crashes during message reconciliation.
function senderNameFromRelayMessage(message: RelayMessage): string {
return message.from?.name || message.from?.id || 'unknown'
}There was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/main/broker.ts:512. Sender fallback now uses optional chaining on message.from before reading name/id.
| reactions: message.reactions?.map((reaction) => ({ | ||
| emoji: reaction.emoji, | ||
| count: reaction.count, | ||
| reactedByHuman: reaction.agents.some(isHumanSenderName) | ||
| })) |
There was a problem hiding this comment.
Guard against reaction.agents being undefined or not an array. If reaction.agents is missing, calling .some() will throw a runtime error.
reactions: message.reactions?.map((reaction) => ({
emoji: reaction.emoji,
count: reaction.count,
reactedByHuman: Array.isArray(reaction.agents) && reaction.agents.some(isHumanSenderName)
}))There was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/main/broker.ts:567. Reaction agent checks now guard reaction.agents with Array.isArray(...) before .some(...).
| function refreshEventStream(reason: string): void { | ||
| const projectId = useProjectStore.getState().activeProjectId || undefined | ||
| const broker = window.pear.broker as PearAPI['broker'] & BrokerWithMessageReconciliation | ||
| void broker.refreshEventStream?.(projectId, reason).catch(() => undefined) | ||
| } |
There was a problem hiding this comment.
In Electron renderers, window.pear or window.pear.broker might be undefined during early initialization or in testing environments. Guarding against this prevents potential runtime crashes.
| function refreshEventStream(reason: string): void { | |
| const projectId = useProjectStore.getState().activeProjectId || undefined | |
| const broker = window.pear.broker as PearAPI['broker'] & BrokerWithMessageReconciliation | |
| void broker.refreshEventStream?.(projectId, reason).catch(() => undefined) | |
| } | |
| function refreshEventStream(reason: string): void { | |
| const projectId = useProjectStore.getState().activeProjectId || undefined | |
| const broker = window.pear?.broker as (PearAPI['broker'] & BrokerWithMessageReconciliation) | undefined | |
| void broker?.refreshEventStream?.(projectId, reason).catch(() => undefined) | |
| } |
There was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/renderer/src/hooks/use-message-reconciliation.ts:200. Refresh now uses window.pear?.broker?.refreshEventStream?.(...) and safely ignores missing preload.
| reconcileMessages: (input) => | ||
| (window.pear.broker as PearAPI['broker'] & BrokerWithMessageReconciliation).reconcileMessages(input), |
There was a problem hiding this comment.
Add a defensive guard for window.pear?.broker to prevent TypeError crashes if the preload API is not fully loaded or mocked.
| reconcileMessages: (input) => | |
| (window.pear.broker as PearAPI['broker'] & BrokerWithMessageReconciliation).reconcileMessages(input), | |
| reconcileMessages: (input) => { | |
| const broker = window.pear?.broker as (PearAPI['broker'] & BrokerWithMessageReconciliation) | undefined | |
| return broker ? broker.reconcileMessages(input) : Promise.resolve([]) | |
| }, |
There was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/renderer/src/hooks/use-message-reconciliation.ts:226. Reconcile now reads window.pear?.broker?.reconcileMessages and falls back to an empty resolved list when unavailable.
| useEffect(() => { | ||
| return window.pear.broker.onStatus((status) => { | ||
| if (BROKER_CONNECTED_STATUSES.has(status.status)) { | ||
| refreshEventStream(`broker:${status.status}`) | ||
| reconciler.schedule(`broker:${status.status}`) | ||
| } | ||
| }) | ||
| }, [reconciler]) |
There was a problem hiding this comment.
Guard the onStatus subscription against an undefined window.pear?.broker to ensure the hook does not crash on mount.
| useEffect(() => { | |
| return window.pear.broker.onStatus((status) => { | |
| if (BROKER_CONNECTED_STATUSES.has(status.status)) { | |
| refreshEventStream(`broker:${status.status}`) | |
| reconciler.schedule(`broker:${status.status}`) | |
| } | |
| }) | |
| }, [reconciler]) | |
| useEffect(() => { | |
| return window.pear?.broker?.onStatus((status) => { | |
| if (BROKER_CONNECTED_STATUSES.has(status.status)) { | |
| refreshEventStream(`broker:${status.status}`) | |
| reconciler.schedule(`broker:${status.status}`) | |
| } | |
| }) | |
| }, [reconciler]) |
There was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/renderer/src/hooks/use-message-reconciliation.ts:276. Status subscription now uses optional chaining around window.pear?.broker?.onStatus?.(...).
| useEffect(() => { | ||
| const broker = window.pear.broker as PearAPI['broker'] & BrokerWithMessageReconciliation | ||
| if (!broker.onEventStreamDiagnostic) return | ||
| return broker.onEventStreamDiagnostic((event) => { | ||
| if (EVENT_STREAM_RECONCILED_STATUSES.has(event.status)) { | ||
| reconciler.schedule(`event-stream:${event.status}`) | ||
| } | ||
| }) | ||
| }, [reconciler]) |
There was a problem hiding this comment.
Guard the onEventStreamDiagnostic subscription against an undefined window.pear?.broker to prevent crashes.
| useEffect(() => { | |
| const broker = window.pear.broker as PearAPI['broker'] & BrokerWithMessageReconciliation | |
| if (!broker.onEventStreamDiagnostic) return | |
| return broker.onEventStreamDiagnostic((event) => { | |
| if (EVENT_STREAM_RECONCILED_STATUSES.has(event.status)) { | |
| reconciler.schedule(`event-stream:${event.status}`) | |
| } | |
| }) | |
| }, [reconciler]) | |
| useEffect(() => { | |
| const broker = window.pear?.broker as (PearAPI['broker'] & BrokerWithMessageReconciliation) | undefined | |
| if (!broker?.onEventStreamDiagnostic) return | |
| return broker.onEventStreamDiagnostic((event) => { | |
| if (EVENT_STREAM_RECONCILED_STATUSES.has(event.status)) { | |
| reconciler.schedule(`event-stream:${event.status}`) | |
| } | |
| }) | |
| }, [reconciler]) |
There was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/renderer/src/hooks/use-message-reconciliation.ts:285. Event-stream diagnostic subscription now guards window.pear?.broker and onEventStreamDiagnostic before subscribing.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (inFlight) { | ||
| debug({ kind: 'skipped', reason }) | ||
| return inFlight | ||
| } |
There was a problem hiding this comment.
Suggestion: Reconciliation triggers are dropped while a fetch is already running: when runNow sees an inFlight promise it returns early and does not queue a follow-up run. If a new trigger (like tab switch/focus/status) happens during a slow fetch, the later trigger can be lost and the UI may remain stale until another external trigger occurs. Keep a "pending rerun" flag or reschedule once inFlight completes. [race condition]
Severity Level: Critical 🚨
- ❌ Active chat tab can show stale message history.
- ⚠️ Tab switch during slow fetch may not refresh content.
- ⚠️ Reconnect/focus triggers can be dropped under load.
- ⚠️ Debug logs show skipped runs without follow-up reconcile.Steps of Reproduction ✅
1. Open the renderer app so `App` mounts, which calls `useMessageReconciliation()` at
`src/renderer/src/App.tsx:14-30` (line 30).
2. `useMessageReconciliation()` constructs a reconciler via `createMessageReconciler` at
`src/renderer/src/hooks/use-message-reconciliation.ts:109-181`, wiring `reconcileMessages`
to `window.pear.broker.reconcileMessages` and `mergeMessages` to `mergeReconciledMessages`
(lines 227-241).
3. With an active channel/DM tab selected, a trigger such as `activeRoomKey` change,
window focus, or broker status causes `reconciler.schedule(...)` to be called from the
effects at `use-message-reconciliation.ts:246-248`, `251-254`, `258-270`, `285-289`, or
`293-299`; after the debounce delay, `schedule()` calls `runNow(reason)` (lines 158-167).
4. Assume the first reconciliation run is slow (e.g., high latency in
`deps.reconcileMessages(request)` at `use-message-reconciliation.ts:136`) so `inFlight` is
set non-null at line 133 and remains pending; a second trigger (tab switch, focus, broker
reconnect) fires while this promise is still in-flight, causing `schedule()` to enqueue
another run.
5. When the second scheduled run fires, `runNow()` executes and hits the `if (inFlight)`
check at `use-message-reconciliation.ts:122-125`, logs a `skipped` debug event, and
`return inFlight` without performing a fresh `getRequest()`/`reconcileMessages()`; the
timer was already cleared in `schedule()` at line 165 and no new timer is set.
6. After the original in-flight reconciliation resolves, no additional run is triggered
for the later reason (new active room, refreshed broker status, etc.), so the currently
active chat room's messages can remain stale until some future external trigger causes
another `reconciler.schedule(...)` call.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/src/hooks/use-message-reconciliation.ts
**Line:** 122:125
**Comment:**
*Race Condition: Reconciliation triggers are dropped while a fetch is already running: when `runNow` sees an `inFlight` promise it returns early and does not queue a follow-up run. If a new trigger (like tab switch/focus/status) happens during a slow fetch, the later trigger can be lost and the UI may remain stale until another external trigger occurs. Keep a "pending rerun" flag or reschedule once `inFlight` completes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/renderer/src/hooks/use-message-reconciliation.ts:119 and src/renderer/src/hooks/use-message-reconciliation.ts:155. In-flight triggers now set pendingRerun and run again after the current fetch; regression covered in src/renderer/src/hooks/use-message-reconciliation.test.ts:145.
| if (previous) { | ||
| byId.set(next.id, { | ||
| ...previous, | ||
| ...next, | ||
| threadReplies: next.threadReplies || previous.threadReplies, | ||
| reactions: next.reactions || previous.reactions | ||
| }) | ||
| changed = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
Suggestion: The merge path marks state as changed for every existing message id even when incoming content is identical, so every reconciliation call rebuilds/sorts the full message array and triggers unnecessary Zustand updates/rerenders. Only mark changed when at least one field actually differs before replacing the existing entry. [performance]
Severity Level: Major ⚠️
- ⚠️ Chat message list rerenders on every reconciliation call.
- ⚠️ Direct-message room derivation recomputes unnecessarily.
- ⚠️ More React work during frequent focus/reconnect events.
- ⚠️ Potential UI jank with large message histories.Steps of Reproduction ✅
1. Mount the renderer app so `App` runs `useMessageReconciliation()` at
`src/renderer/src/App.tsx:20-30`, which wires reconciliation to the broker and message
store.
2. A reconciliation trigger (e.g., window focus or broker status) calls
`reconciler.schedule(...)` from
`src/renderer/src/hooks/use-message-reconciliation.ts:246-248,251-254,258-270,285-289,293-299`,
eventually running `deps.reconcileMessages(request)` and passing the results into
`mergeReconciledMessages` at `use-message-reconciliation.ts:133-141,183-201`.
3. `mergeReconciledMessages()` calls
`useAgentStore.getState().reconcileMessages(messages)` at `agent-store.ts:183-187`, which
in turn uses `reconcileChatMessages(state.messages, messages)` inside the Zustand `set`
callback at `agent-store.ts:632-645`.
4. In `reconcileChatMessages` at `agent-store.ts:331-365`, for each incoming message that
matches an existing `id`, the merge path at lines 345-355 (`const previous =
byId.get(next.id); if (previous) { byId.set(next.id, { ...previous, ...next, ... });
changed = true; }`) unconditionally sets `changed = true` and replaces the map entry with
a new object, even when all fields (`body`, `timestamp`, `reactions`, `threadReplies`,
etc.) are identical to `previous`.
5. Because `changed` is set whenever any incoming message is seen, the function always
returns a new array instance via `Array.from(byId.values()).sort(...)` at
`agent-store.ts:362-365` instead of returning the original `existingMessages` reference.
6. The store's `reconcileMessages` then sees `nextMessages !== state.messages` and returns
`{ messages: nextMessages }` at `agent-store.ts:632-644`, forcing a Zustand state update
and downstream rerenders on every reconciliation call, even when the broker returns only
messages that are bitwise-identical to what is already in the UI.
7. Features that derive direct message rooms from `messages`, such as
`deriveDirectMessageRooms` in `src/renderer/src/lib/direct-messages.ts:11-48`, will
repeatedly recompute and rerender under frequent reconciliations, incurring avoidable CPU
and rendering work.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/renderer/src/stores/agent-store.ts
**Line:** 346:355
**Comment:**
*Performance: The merge path marks state as changed for every existing message id even when incoming content is identical, so every reconciliation call rebuilds/sorts the full message array and triggers unnecessary Zustand updates/rerenders. Only mark changed when at least one field actually differs before replacing the existing entry.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/renderer/src/stores/agent-store.ts:393 and src/renderer/src/hooks/use-message-reconciliation.test.ts:233. Existing-id merges now use field-scoped equality before marking changed. Note: the equality check is field-scoped (body, timestamp, reactions, threadReplies, plus displayed routing fields); reverting just the equality check restores the over-marking behavior and makes the new no-op identical re-merge assertion go RED. Codex-2 earlier non-vacuity for genuine updates is preserved.
Review of #86 — Fix stale Pear message panes after idleVerdict: APPROVE ✅ The PR cleanly addresses every clause of the Phase 3 acceptance bar in Acceptance bar — line-by-line
Non-vacuity (verified empirically)I reverted each fix in isolation on the PR branch and re-ran tests:
Locally re-ran
CI checks on the PR are also green; CodeRabbit pass. Code quality notesStrengths
Minor follow-ups (non-blocking, don't need to land in this PR)
What I'd ask forNothing required. Ship it. The follow-ups above are genuinely optional — drop them or schedule them separately. The PR meets every clause of the contract and has empirical non-vacuity for the load-bearing behavior. |
| if (win && !win.isDestroyed()) { | ||
| session.window = win | ||
| } |
There was a problem hiding this comment.
Suggestion: Rebinding unconditionally overwrites the session window when a win is passed, and this runs for each session being refreshed. When refresh is invoked without a specific project, one renderer can steal event routing ownership for other sessions/windows; only update session.window for explicitly targeted sessions or when session ownership is validated. [stale reference]
Severity Level: Major ⚠️
- ❌ Broker events for a project routed to wrong window.
- ⚠️ PTY terminal output may appear in incorrect project tab.
- ⚠️ Event-stream diagnostics misattribute project to wrong renderer.Steps of Reproduction ✅
1. Start Pear and open at least two projects so that `useProjectStore.ensureBroker()`
(src/renderer/src/stores/project-store.ts:136–156) causes `pear.broker.start()` to run for
each, creating multiple `BrokerSession` entries in `BrokerManager.sessions`
(src/main/broker.ts:1077–1087, 1121–1180).
2. In any renderer window, call the public preload API
`window.pear.broker.refreshEventStream(undefined, 'manual-debug')`, which forwards via
`invoke('broker:refresh-event-stream', projectId, reason)` in `preload/index.ts`
(src/preload/index.ts:252–253).
3. The IPC handler `ipcMain.handle('broker:refresh-event-stream', ...)`
(src/main/ipc-handlers.ts:266–269) receives `projectId` as `undefined`, resolves `win`
from `BrowserWindow.fromWebContents(event.sender)`, and calls
`brokerManager.refreshEventStream(projectId, reason || 'renderer-request', win ||
undefined)`.
4. In `BrokerManager.refreshEventStream()` (src/main/broker.ts:1720–1726), because
`projectId` is falsy, it builds `sessions = Array.from(this.sessions.values())` (all
projects) and for each session calls `rebindSessionEventStream(sessionKeyFor(session),
session, reason, win)` with the same `win`.
5. Inside `rebindSessionEventStream()` (src/main/broker.ts:1730–1739), the guard `if (win
&& !win.isDestroyed()) { session.window = win }` overwrites `session.window` for every
session, so all projects' sessions now point to the calling window's `BrowserWindow`
regardless of which window originally owned them.
6. Subsequent broker events use `windowForSession()` and `publishBrokerEvent()`
(src/main/broker.ts:1677–1681, 1927–1943) to choose the target renderer based on
`session.window`; as a result, events for other projects (including `broker:event`,
`broker:event-stream-diagnostic`, and `broker:pty-chunk`) are routed only to the stealing
window, leaving the original project windows with stale terminals and message panes.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/broker.ts
**Line:** 1737:1739
**Comment:**
*Stale Reference: Rebinding unconditionally overwrites the session window when a `win` is passed, and this runs for each session being refreshed. When refresh is invoked without a specific project, one renderer can steal event routing ownership for other sessions/windows; only update `session.window` for explicitly targeted sessions or when session ownership is validated.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/main/broker.ts:1720 and src/main/broker.ts:1725. Global refreshes no longer pass a caller window into session rebind; only project-scoped refreshes can update session.window.
| session.unsubEvent() | ||
| session.client.disconnectEvents() | ||
| session.unsubEvent = this.attachClient(sessionKey, session.client, session.window) | ||
| session.client.connectEvents(session.lastEventSeq) |
There was a problem hiding this comment.
Suggestion: The rebind flow tears down the existing listener before ensuring a new listener is successfully attached and connected. If attachClient or connectEvents throws, the session is left unsubscribed and stops receiving broker events until another successful rebind; keep the old subscription until reattach succeeds, or restore it in the catch path. [logic error]
Severity Level: Critical 🚨
- ❌ Broker session can permanently lose event-stream updates.
- ❌ Chat and terminal panes freeze after rebind failure.
- ⚠️ Message reconciliation stops reflecting latest relay messages.Steps of Reproduction ✅
1. Launch Pear so `App` mounts and `useProjectStore.load()` runs
(src/renderer/src/App.tsx:51–53), which in turn calls `ensureBroker()`
(src/renderer/src/stores/project-store.ts:136–156) and invokes `pear.broker.start(...)` to
create a `BrokerSession` with an active `client.onEvent` subscription via
`BrokerManager.attachClient()` (src/main/broker.ts:1844–1889).
2. In a renderer, `useMessageReconciliation()` triggers an event-stream refresh by calling
`broker.refreshEventStream(projectId, reason)`
(src/renderer/src/hooks/use-message-reconciliation.ts:209–213), which flows through the
IPC handler `broker:refresh-event-stream` (src/main/ipc-handlers.ts:266–269) to
`BrokerManager.refreshEventStream(projectId, reason, win)` (src/main/broker.ts:1720–1726).
3. For the target session, `refreshEventStream()` calls
`rebindSessionEventStream(sessionKey, session, reason, win)`
(src/main/broker.ts:1725–1727), which executes the rebind sequence: `session.unsubEvent()`
and `session.client.disconnectEvents()` (src/main/broker.ts:1768–1769) to tear down the
existing listener, then `session.unsubEvent = this.attachClient(sessionKey,
session.client, session.window)` and `session.client.connectEvents(session.lastEventSeq)`
(src/main/broker.ts:1770–1771) to attach a new one.
4. If either `attachClient(...)` or `session.client.connectEvents(...)` throws—for example
due to a transient broker error or harness-driver failure—the control jumps to the `catch`
block (src/main/broker.ts:1785–1793), which only emits a `BrokerEventStreamDiagnostic`
with `status: 'rebind-error'` and rethrows, without restoring the previous `onEvent`
subscription or reconnecting events.
5. After such a failure, the `BrokerSession` has already run `session.unsubEvent()` and
`disconnectEvents()`, but no replacement listener is attached, leaving the session with no
active `client.onEvent` handler; downstream, `useBrokerEvents()`
(src/renderer/src/hooks/use-broker-events.ts:64–76, 80–92) no longer receives
`broker:event`, `broker:status`, or PTY chunk updates for that project, so chat and
terminal UIs stop updating until a later successful rebind or app restart.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/broker.ts
**Line:** 1768:1771
**Comment:**
*Logic Error: The rebind flow tears down the existing listener before ensuring a new listener is successfully attached and connected. If `attachClient` or `connectEvents` throws, the session is left unsubscribed and stops receiving broker events until another successful rebind; keep the old subscription until reattach succeeds, or restore it in the catch path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/main/broker.ts:1769 and src/main/broker.ts:1799. Rebind now keeps the previous listener available while wiring the next listener, and restores the previous subscription if attach/connect fails; regression covered in src/main/broker.test.ts:455.
| ipcMain.handle('broker:refresh-event-stream', async (event, projectId?: string, reason?: string) => { | ||
| const win = BrowserWindow.fromWebContents(event.sender) | ||
| await brokerManager.refreshEventStream(projectId, reason || 'renderer-request', win || undefined) | ||
| }) |
There was a problem hiding this comment.
Suggestion: The IPC handler accepts an optional projectId and forwards it directly, so renderer calls without a project end up refreshing every broker session. Focus/online/status-triggered refreshes can therefore disconnect and reconnect unrelated projects, creating avoidable churn and cross-project side effects; require a concrete project id (or scope to sender-bound project) before calling refresh. [logic error]
Severity Level: Major ⚠️
- ⚠️ Global refresh from one window rebinds all broker sessions.
- ⚠️ Unrelated projects see unnecessary event-stream disconnects.
- ⚠️ Rebind failures in other projects affect uninvolved windows.Steps of Reproduction ✅
1. Run Pear and open multiple projects so that `useProjectStore.ensureBroker()` ultimately
calls `brokerManager.start(projectId, ...)` via `ipcMain.handle('broker:start', ...)`
(src/main/ipc-handlers.ts:175–183), creating multiple `BrokerSession`s in
`BrokerManager.sessions` (src/main/broker.ts:1077–1087, 1121–1180).
2. From any renderer window, invoke the public preload API without a project scope, e.g.
in DevTools run `window.pear.broker.refreshEventStream(undefined, 'manual-global')`, which
calls `invoke('broker:refresh-event-stream', projectId, reason)`
(src/preload/index.ts:252–253) with `projectId` as `undefined`.
3. The IPC handler `ipcMain.handle('broker:refresh-event-stream', ...)`
(src/main/ipc-handlers.ts:266–269) receives the request, derives `win` from
`BrowserWindow.fromWebContents(event.sender)`, and passes `projectId` (undefined), `reason
|| 'renderer-request'`, and `win` to `brokerManager.refreshEventStream(...)`.
4. Inside `BrokerManager.refreshEventStream()` (src/main/broker.ts:1720–1724), because
`projectId` is falsy, it computes `sessions = Array.from(this.sessions.values())` and
iterates all broker sessions—local and cloud—calling
`rebindSessionEventStream(sessionKeyFor(session), session, reason, win)` for each, even
for projects unrelated to the calling window.
5. Each unrelated session undergoes a disconnect/reconnect cycle (`session.unsubEvent()`,
`disconnectEvents()`, then `connectEvents()` in src/main/broker.ts:1768–1772), exposing
them to the failure mode described in broker.ts:1785–1793 (rebind error leaves them
unsubscribed) and causing unnecessary event-stream churn across projects when the caller
likely intended to refresh only its own active project.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/ipc-handlers.ts
**Line:** 266:269
**Comment:**
*Logic Error: The IPC handler accepts an optional `projectId` and forwards it directly, so renderer calls without a project end up refreshing every broker session. Focus/online/status-triggered refreshes can therefore disconnect and reconnect unrelated projects, creating avoidable churn and cross-project side effects; require a concrete project id (or scope to sender-bound project) before calling refresh.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Checked at HEAD 2e94a15. Fixed — see src/main/ipc-handlers.ts:266. IPC now trims and requires a non-empty project id before calling refreshEventStream, preventing renderer-triggered global rebinds.
|
CodeAnt AI finished reviewing your PR. |
1 participant
User description
Summary
pear.broker.reconcileMessages(input)backed by SDK message list APIsValidation
git diff --checknpx vitest run src/main/broker.test.ts src/renderer/src/hooks/use-message-reconciliation.test.tsnpm testnpm run buildNotes:
npm run buildstill prints existing Vite dynamic/static import warnings forauth.ts,broker.ts, andintegrations.ts, but completes successfully.CodeAnt-AI Description
Keep chat panes in sync after idle, tab switches, and reconnects
What Changed
Impact
✅ Fewer stale chat panes after idle✅ Fewer duplicate messages after reconnect✅ Messages stay in the active window after tab or window changes🔄 Retrigger CodeAnt AI Review
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.