Decomposed process.ts, Audited and fixed 3 small pre-existing bugs - #1355
Merged
Conversation
📝 WalkthroughWalkthroughThe monolithic preload process bridge is split into core and per-domain IPC factories. New remote APIs cover process, tabs, commands, automation, sessions, groups, context, settings, and related operations. Tests and web-server callbacks validate the extracted APIs and response-channel behavior. ChangesPreload IPC modularization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR decomposes the large process preload bridge into focused per-domain factories while preserving the existing
Confidence Score: 5/5The PR appears safe to merge, with the decomposed preload API retaining its existing contracts and the three targeted fixes addressing reachable pre-existing failures. All prior process API properties remain represented exactly once with matching IPC channels and argument shapes, while the callback changes safely fix timer ordering, response-channel collisions, and settings snapshot drift. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Composer["createProcessApi()"] --> Core["Process core"]
Composer --> Commands["Command and queue remotes"]
Composer --> Tabs["Tab and browser remotes"]
Composer --> Automation["Auto Run, Cue, and playbook remotes"]
Composer --> Data["Session, group, git, settings, and context remotes"]
Composer --> Cadenza["Cadenza and movement remotes"]
Core --> IPC["ipcRenderer channels"]
Commands --> IPC
Tabs --> IPC
Automation --> IPC
Data --> IPC
Cadenza --> IPC
IPC --> Main["Electron main process"]
Main --> Web["Web server callbacks"]
Reviews (1): Last reviewed commit: "Decomposed process.ts, Audited and fixed..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
src/main/preload/process/sessionCrudRemote.ts (1)
61-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one request-response registration helper for the domain factories. Each factory hand-rolls the same
ipcRenderer.on+ forward +removeListenershape, and the direct-forward handlers send no ack when the callback throws or rejects. The caller then waits for its 5000 ms timeout instead of receiving an immediate error. A single helper that wraps the callback inPromise.resolve(...), acks a caller-supplied fallback, and rethrows would fix all sites at once and match the pattern already used inautoRunControlRemote.tsandplaybookRemote.ts.
src/main/preload/process/sessionCrudRemote.ts#L61-L72: routeonRemoteCreateSessionthrough the shared helper with a{ success: false, error }fallback, then apply it to lines 97-104, 119-126, 143-158, and 176-191.src/main/preload/process/contextOpsRemote.ts#L12-L20: routeonRemoteMergeContextthrough the shared helper with afalsefallback, then apply it to lines 36-44 and 60-64.src/main/preload/process/groupCrudRemote.ts#L17-L25: routeonRemoteCreateGroupthrough the shared helper with anullfallback, then apply it to lines 47-48 and 76-81.src/main/preload/process/groupChatRemote.ts#L29-L37: routeonRemoteStartGroupChatthrough the shared helper with anullfallback, then apply it to lines 10, 56-57, 76-77, and 96-97.Check
docs/agent-guides/IPC-PATTERNS.mdfirst. If a canonical helper already exists there, extend it instead of adding a new one.Based on learnings from the coding guidelines: "Before creating a new utility, helper, hook, component, type, or constant, check the relevant guide in
docs/agent-guides/and reuse or extend the canonical implementation instead of duplicating it."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/preload/process/sessionCrudRemote.ts` around lines 61 - 72, Extract or extend the canonical request-response registration helper described in docs/agent-guides/IPC-PATTERNS.md so it wraps callbacks with Promise.resolve, acknowledges failures using the supplied fallback, and rethrows errors. Apply it to sessionCrudRemote.ts lines 61-72, 97-104, 119-126, 143-158, and 176-191; contextOpsRemote.ts lines 12-20, 36-44, and 60-64; groupCrudRemote.ts lines 17-25, 47-48, and 76-81; and groupChatRemote.ts lines 29-37, 10, 56-57, 76-77, and 96-97, using fallbacks { success: false, error }, false, null, and null respectively.Source: Coding guidelines
src/main/preload/process/tabRemote.ts (1)
17-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider acking a failure when the new-tab callback throws.
onRemoteNewTabuses the request-response pattern withresponseChannel. If the callback throws synchronously, nothing is ever sent on that channel, so the caller waits for its full response timeout.browserTabRemote.tslines 14-21 solve the same problem by sending a failure value first and then rethrowing for Sentry.This behavior is unchanged from the previous monolithic module. Aligning it makes the request-response factories consistent.
♻️ Proposed change
const handler = (_: unknown, sessionId: string, responseChannel: string) => - callback(sessionId, responseChannel); + { + try { + callback(sessionId, responseChannel); + } catch (error) { + ipcRenderer.send(responseChannel, null); + throw error; + } + }; ipcRenderer.on('remote:newTab', handler);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/preload/process/tabRemote.ts` around lines 17 - 24, Update onRemoteNewTab so its IPC handler catches synchronous errors from callback, sends a failure response through responseChannel before rethrowing the error for Sentry, and preserves the existing success callback behavior and listener cleanup.src/__tests__/main/preload/process/commandRemote.test.ts (1)
30-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the remaining factory methods and the error path.
createCommandRemoteApiexposes four methods. These tests cover onlyonRemoteCommand. Consider adding cases for:
- The returned unsubscribe function. Assert that it calls
removeListenerwith the same channel and handler reference.- The catch path at
commandRemote.tslines 49-59. If the rethrow fix in the other comment is applied, a test here locks in that behavior.onRemoteSwitchMode,onRemoteInterrupt, andonRemoteSelectSession.The
mockOn.mockImplementationblock is repeated in both tests. A smallcaptureHandler(channel)helper would remove that duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/main/preload/process/commandRemote.test.ts` around lines 30 - 97, Expand coverage for createCommandRemoteApi beyond onRemoteCommand: add tests for the unsubscribe function verifying removeListener receives the same channel and handler, the commandRemote.ts catch path preserving the rethrow behavior, and the onRemoteSwitchMode, onRemoteInterrupt, and onRemoteSelectSession methods. Extract the repeated mockOn handler-capture setup into a small captureHandler(channel) helper and reuse it in the existing tests.src/main/preload/process/commandRemote.ts (1)
40-48: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider dropping the command preview from the debug log.
commandPreviewforwards the first 50 characters of the user prompt to the main-process logger. Prompt text can contain credentials, tokens, or personal data. The log is written to the persisted log buffer throughlogger:log. Logging the command length instead of the content keeps the diagnostic value without retaining user content.This behavior is carried over from the previous monolithic module, so it is not a regression in this PR.
🔒 Proposed change
log('Received remote:executeCommand IPC', { sessionId, - commandPreview: command?.substring(0, 50), + commandLength: command?.length ?? 0, inputMode, tabId, force, imageCount: images?.length ?? 0, background, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/preload/process/commandRemote.ts` around lines 40 - 48, Update the remote:executeCommand debug log to remove commandPreview and log only the command’s length instead. Preserve the existing sessionId, inputMode, tabId, force, imageCount, and background fields while ensuring no user prompt content is passed to log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/agent-guides/IPC-PATTERNS.md`:
- Line 38: Update the process row in the IPC patterns table to identify the
main-process handler file consistently with the other rows, rather than listing
preload modules in the Handler File column. Correct the preload factory count to
18, and move the `core.ts` plus 17 `*Remote.ts` module breakdown into the
explanatory text below the table or a separate column.
In `@src/__tests__/main/web-server/web-server-factory.test.ts`:
- Around line 2106-2107: Update the test around registerTabCallbacks so both
callback promises returned by callback('session-1') are retained and settled
before the test completes. Invoke the corresponding response handlers, or use
fake timers to trigger the five-second fallback cleanup, ensuring both IPC
listeners and timeout resources are released.
In `@src/main/preload/process/autoRunConfigRemote.ts`:
- Around line 20-35: Update the error handlers in onRemoteConfigureAutoRun,
onRemoteGetAutoRunDocs, onRemoteGetAutoRunDocContent, and onRemoteSaveAutoRunDoc
to rethrow the caught error after sending their fallback acknowledgement,
matching the existing pattern in onRemoteSetAutoRunFolder. Preserve each
handler’s current fallback response while ensuring the original error reaches
the global error handler.
In `@src/main/preload/process/commandRemote.ts`:
- Around line 49-59: Update the catch block surrounding the remote command
callback invocation to rethrow the captured error after logging it. Preserve the
existing logging behavior and do not add an acknowledgement, since
remote:executeCommand has no response channel.
In `@src/main/preload/process/cueRemote.ts`:
- Around line 23-33: Handle only known recoverable errors in the Cue and Git
remote callbacks. In src/main/preload/process/cueRemote.ts:23-33, preserve the
failure acknowledgment, report unexpected callback failures through the project
Sentry utility, and rethrow them; in src/main/preload/process/gitRemote.ts:13-19
and 45-51, replace empty status/diff fallbacks for unexpected failures with
Sentry reporting and rethrowing, while retaining existing behavior for known
recoverable errors.
In `@src/main/preload/process/gistRemote.ts`:
- Around line 26-34: Update the callback invocation in the remote gist process
to handle both synchronous throws and rejected Promise results by wrapping the
returned value in a Promise chain. On failure, send the unsuccessful IPC
response, call captureException with context "remoteCreateGist", and rethrow
unexpected errors.
In `@src/main/preload/process/queueRemote.ts`:
- Around line 32-37: Update the callback invocation in onRemoteEnqueueCommand,
onRemoteListQueue, and onRemoteRemoveQueueItem to use Promise.resolve(...)
within the existing try/catch so rejected promises reach the fallback
ipcRenderer.send(responseChannel, { success: false }) acknowledgement. Preserve
rethrowing the error after sending the acknowledgement and match the established
sibling-factory pattern.
In `@src/main/preload/process/sessionCrudRemote.ts`:
- Around line 20-31: Add throw error after each fallback ipcRenderer.send in the
Promise rejection and synchronous catch branches of the session CRUD callback
flow, preserving the failure acknowledgement while rethrowing the original
exception for Sentry capture. Match the behavior used by
autoRunControlRemote.ts.
---
Nitpick comments:
In `@src/__tests__/main/preload/process/commandRemote.test.ts`:
- Around line 30-97: Expand coverage for createCommandRemoteApi beyond
onRemoteCommand: add tests for the unsubscribe function verifying removeListener
receives the same channel and handler, the commandRemote.ts catch path
preserving the rethrow behavior, and the onRemoteSwitchMode, onRemoteInterrupt,
and onRemoteSelectSession methods. Extract the repeated mockOn handler-capture
setup into a small captureHandler(channel) helper and reuse it in the existing
tests.
In `@src/main/preload/process/commandRemote.ts`:
- Around line 40-48: Update the remote:executeCommand debug log to remove
commandPreview and log only the command’s length instead. Preserve the existing
sessionId, inputMode, tabId, force, imageCount, and background fields while
ensuring no user prompt content is passed to log.
In `@src/main/preload/process/sessionCrudRemote.ts`:
- Around line 61-72: Extract or extend the canonical request-response
registration helper described in docs/agent-guides/IPC-PATTERNS.md so it wraps
callbacks with Promise.resolve, acknowledges failures using the supplied
fallback, and rethrows errors. Apply it to sessionCrudRemote.ts lines 61-72,
97-104, 119-126, 143-158, and 176-191; contextOpsRemote.ts lines 12-20, 36-44,
and 60-64; groupCrudRemote.ts lines 17-25, 47-48, and 76-81; and
groupChatRemote.ts lines 29-37, 10, 56-57, 76-77, and 96-97, using fallbacks {
success: false, error }, false, null, and null respectively.
In `@src/main/preload/process/tabRemote.ts`:
- Around line 17-24: Update onRemoteNewTab so its IPC handler catches
synchronous errors from callback, sends a failure response through
responseChannel before rethrowing the error for Sentry, and preserves the
existing success callback behavior and listener cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e943c177-5156-4d6b-b074-9f2fb6272c64
📒 Files selected for processing (29)
docs/agent-guides/IPC-PATTERNS.mdsrc/__tests__/main/preload/process/cadenzaMovementRemote.test.tssrc/__tests__/main/preload/process/commandRemote.test.tssrc/__tests__/main/preload/process/core.test.tssrc/__tests__/main/preload/process/groupCrudRemote.test.tssrc/__tests__/main/preload/process/tabRemote.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/main/preload/process.tssrc/main/preload/process/autoRunConfigRemote.tssrc/main/preload/process/autoRunControlRemote.tssrc/main/preload/process/browserTabRemote.tssrc/main/preload/process/cadenzaMovementRemote.tssrc/main/preload/process/commandRemote.tssrc/main/preload/process/contextOpsRemote.tssrc/main/preload/process/core.tssrc/main/preload/process/cueRemote.tssrc/main/preload/process/gistRemote.tssrc/main/preload/process/gitRemote.tssrc/main/preload/process/groupChatRemote.tssrc/main/preload/process/groupCrudRemote.tssrc/main/preload/process/notificationRemote.tssrc/main/preload/process/playbookRemote.tssrc/main/preload/process/queueRemote.tssrc/main/preload/process/sessionCrudRemote.tssrc/main/preload/process/settingsRemote.tssrc/main/preload/process/tabRemote.tssrc/main/web-server/callbacks/cadenzaMovementCallbacks.tssrc/main/web-server/callbacks/settingsCallbacks.tssrc/main/web-server/callbacks/tabCallbacks.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation