feat: add comfy:// deep link protocol handler#1651
Open
MillerMedia wants to merge 4 commits intomainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comfy:// deep-link support: protocol registration and debug config entry, new deep-link utilities, a DEEP_LINK_OPEN IPC channel and preload handler, startup capture of OS/CLI URLs, DesktopApp pending-URL plumbing, and AppWindow.handleDeepLink to validate, emit, and focus on open. Changes
Sequence DiagramsequenceDiagram
participant OS as OS / CLI
participant Main as Main Process
participant Utils as Deep Link Utils
participant AppWin as AppWindow
participant IPC as IPC Channel
participant Renderer as Renderer
OS->>Main: deep link via CLI args or open-url event
Main->>Utils: findDeepLinkUrl(commandLine)
Utils-->>Main: comfy:// URL or undefined
Main->>Main: store pendingDeepLinkUrl (if any)
Main->>AppWin: construct DesktopApp(..., pendingDeepLinkUrl)
AppWin->>AppWin: load frontend / ready
AppWin->>Utils: parseDeepLinkUrl(url)
Utils-->>AppWin: { action, filePath } or null
AppWin->>AppWin: fs.access(filePath)
AppWin->>IPC: send DEEP_LINK_OPEN(filePath)
IPC->>Renderer: onDeepLinkOpen callback invoked
Renderer->>Renderer: open file / handle path
AppWin->>AppWin: restore & focus window
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main-process/appWindow.ts`:
- Around line 387-389: The new open-url listener can double-handle a URL if the
early startup listener already set pendingDeepLinkUrl; change the listener added
in setupAppEvents()/app.on('open-url', ...) so it first checks the shared
pendingDeepLinkUrl (or a new boolean flag like earlyDeepLinkCaptured) and
returns without calling handleDeepLink(url) when that pending value exists
(meaning the early listener already captured the deep link). Concretely: inside
the app.on('open-url', (_event, url) => { ... }) handler in appWindow.ts, if
pendingDeepLinkUrl === url (or earlyDeepLinkCaptured === true) then skip calling
this.handleDeepLink(url); keep the early listener behavior of preventDefault()
and storing pendingDeepLinkUrl so DesktopApp.start() still performs the single
canonical handleDeepLink(pendingDeepLinkUrl) call. Ensure the check references
pendingDeepLinkUrl, setupAppEvents(), handleDeepLink, and DesktopApp.start.
In `@src/utils/deepLink.ts`:
- Around line 48-49: findDeepLinkUrl currently only matches args that start with
a lowercase PROTOCOL_NAME, so inputs like "COMFY://..." are missed; update
findDeepLinkUrl to perform case-insensitive matching (e.g. compare
arg.toLowerCase() to `${PROTOCOL_NAME}://` or use a case-insensitive regex)
while still returning the original arg string if it matches; change the
predicate inside findDeepLinkUrl to use a case-insensitive check so
parseDeepLinkUrl can receive and parse the original URL.
In `@tests/unit/main-process/appWindow.test.ts`:
- Around line 120-209: The test file uses Jasmine/Mocha-style it(...) blocks
which violate the project standard; update all unit test definitions in this
file (the describe('AppWindow.handleDeepLink') suite) to use test(...) instead
of it(...), i.e., replace every occurrence of it('should ...', () => { ... })
with test('should ...', () => { ... }) so tests like the ones exercising
AppWindow.handleDeepLink (and assertions on sendSpy, fs.existsSync, mockFocus,
mockRestore, mockIsMinimized) follow the required test(...) form.
In `@tests/unit/utils/deepLink.test.ts`:
- Around line 1-80: The tests in deepLink.test.ts use "it" but the project
convention requires "test"; update the import to { describe, expect, test } from
'vitest' and replace every "it(" usage with "test(" in both describe blocks (the
tests referencing parseDeepLinkUrl and findDeepLinkUrl) so all 16 test cases use
test(...) while keeping the same descriptions and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a4614a2-fcba-4c31-bed4-0edee3f17ee0
📒 Files selected for processing (10)
builder-debug.config.tssrc/constants.tssrc/desktopApp.tssrc/infrastructure/ipcChannels.tssrc/main-process/appWindow.tssrc/main.tssrc/preload.tssrc/utils/deepLink.tstests/unit/main-process/appWindow.test.tstests/unit/utils/deepLink.test.ts
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main-process/appWindow.ts (1)
390-393:⚠️ Potential issue | 🟠 MajorGuard the macOS
open-urlpath against startup double-delivery.This still calls
handleDeepLink(url)unconditionally, so the earlyopen-urllistener insrc/main.tscan capture the same URL andDesktopApp.start()will replay it later. Add adefaultPreventedor pending-URL guard here so the deep link is handled once.Possible fix
- app.on('open-url', (event, url) => { - event.preventDefault(); - void this.handleDeepLink(url); + app.on('open-url', (event, url) => { + if (event.defaultPrevented) return; + event.preventDefault(); + void this.handleDeepLink(url); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main-process/appWindow.ts` around lines 390 - 393, The macOS 'open-url' handler currently always calls this.handleDeepLink(url), causing duplicate processing when an early listener in startup already captured the URL; update the app.on('open-url', ...) handler to first check event.defaultPrevented and a class-level pending/handled flag (e.g., this._pendingDeepLink or this._deepLinkHandled) and only call this.handleDeepLink(url) when the event is not defaultPrevented and the URL hasn't already been queued/processed; set or clear the flag appropriately so the deep link is handled exactly once and avoid double-delivery with DesktopApp.start() replay logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main-process/appWindow.ts`:
- Around line 179-194: The logs currently print sensitive deep-link targets (the
full comfy:// URL and local file paths) in the parseDeepLinkUrl handling; update
the handlers around parseDeepLinkUrl, the result checks, and the fs.access catch
(references: parseDeepLinkUrl, result, log.warn, log.error, fs.access,
dialog.showErrorBox) to avoid logging full paths/URLs — instead log
non-sensitive info such as result.action and a redacted target (e.g.,
path.basename(result.filePath) or replace the home directory with "~" or a
literal "<redacted>") so logs never contain usernames/home directories, while
keeping dialog.showErrorBox behavior unchanged if you want users to see the full
path.
- Around line 198-201: The deep-link handler currently restores only minimized
windows (this.isMinimized() → this.restore()) but doesn't unhide windows that
are hidden to tray; update the flow around the IPC send
(IPC_CHANNELS.DEEP_LINK_OPEN) to ensure the BrowserWindow is shown before
focusing by checking visibility (e.g., this.isVisible() or equivalent) and
calling this.show() when the window is not visible, then restore if minimized
and finally call this.focus(); modify the methods around send, isMinimized,
restore, show, and focus to ensure hidden-to-tray windows are revealed before
focus.
---
Duplicate comments:
In `@src/main-process/appWindow.ts`:
- Around line 390-393: The macOS 'open-url' handler currently always calls
this.handleDeepLink(url), causing duplicate processing when an early listener in
startup already captured the URL; update the app.on('open-url', ...) handler to
first check event.defaultPrevented and a class-level pending/handled flag (e.g.,
this._pendingDeepLink or this._deepLinkHandled) and only call
this.handleDeepLink(url) when the event is not defaultPrevented and the URL
hasn't already been queued/processed; set or clear the flag appropriately so the
deep link is handled exactly once and avoid double-delivery with
DesktopApp.start() replay logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7600b0ca-2ce0-46b5-a6cc-e74d4ab1bc87
📒 Files selected for processing (3)
src/desktopApp.tssrc/main-process/appWindow.tstests/unit/main-process/appWindow.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/desktopApp.ts
- tests/unit/main-process/appWindow.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main-process/appWindow.ts (2)
198-201:⚠️ Potential issue | 🟠 MajorShow the window before focusing a deep link.
If the app is hidden to tray,
restore()/focus()will not make it visible, so the workflow opens in the background with no visible UI. Callshow()before restore/focus.Possible fix
+ if (!this.window.isVisible()) this.show(); if (this.isMinimized()) this.restore(); this.focus();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main-process/appWindow.ts` around lines 198 - 201, The deep-link handling currently calls this.restore() and this.focus() after sending IPC_CHANNELS.DEEP_LINK_OPEN but does not ensure the window is visible; update the sequence in the same handler so you call this.show() before attempting to restore/focus (e.g., if the window is hidden to tray). Locate the block that calls this.send(IPC_CHANNELS.DEEP_LINK_OPEN, ...), then insert a this.show() call prior to the existing this.isMinimized()/this.restore() and this.focus() calls so the window becomes visible when a deep link is opened.
179-198:⚠️ Potential issue | 🟠 MajorDeep-link targets still leak into logs.
The full
comfy://URL and absolute file path are still logged here, and ifrendererReadyisfalsethe queuedDEEP_LINK_OPENpayload will also be logged later bysendQueuedEventsOnReady(). Please redact or suppress deep-link URLs/paths on both paths so usernames and home-directory details never end up in desktop logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main-process/appWindow.ts` around lines 179 - 198, The deep-link URL and absolute file path are being logged in parseDeepLinkUrl handling and later when sent/queued; change the logging to redact sensitive parts (e.g., replace the URL/filePath with a sanitized version such as masking the home directory and username or showing only the filename using path.basename) instead of printing the full value, and ensure the payload sent via this.send(IPC_CHANNELS.DEEP_LINK_OPEN, ...) and any logging inside sendQueuedEventsOnReady() also uses the same sanitized/stripped file identifier (or omits the path entirely) so no absolute paths or comfy:// URLs are written to logs; update the logic around parseDeepLinkUrl result handling, the fs.access error logging, and send/sendQueuedEventsOnReady to use the sanitized value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main-process/appWindow.ts`:
- Around line 190-195: The catch block that wraps fs.access(...) currently
treats every failure as "File Not Found"; change it to inspect the caught error
(e.g., the catch parameter) and branch on error.code: if error.code === 'ENOENT'
keep the existing log.error and dialog.showErrorBox message for missing file
using result.filePath, otherwise log the full error via log.error with the error
details and show a different dialog.showErrorBox describing an access/permission
error (include error.message) so permission and I/O errors are not misreported
as missing files.
---
Duplicate comments:
In `@src/main-process/appWindow.ts`:
- Around line 198-201: The deep-link handling currently calls this.restore() and
this.focus() after sending IPC_CHANNELS.DEEP_LINK_OPEN but does not ensure the
window is visible; update the sequence in the same handler so you call
this.show() before attempting to restore/focus (e.g., if the window is hidden to
tray). Locate the block that calls this.send(IPC_CHANNELS.DEEP_LINK_OPEN, ...),
then insert a this.show() call prior to the existing
this.isMinimized()/this.restore() and this.focus() calls so the window becomes
visible when a deep link is opened.
- Around line 179-198: The deep-link URL and absolute file path are being logged
in parseDeepLinkUrl handling and later when sent/queued; change the logging to
redact sensitive parts (e.g., replace the URL/filePath with a sanitized version
such as masking the home directory and username or showing only the filename
using path.basename) instead of printing the full value, and ensure the payload
sent via this.send(IPC_CHANNELS.DEEP_LINK_OPEN, ...) and any logging inside
sendQueuedEventsOnReady() also uses the same sanitized/stripped file identifier
(or omits the path entirely) so no absolute paths or comfy:// URLs are written
to logs; update the logic around parseDeepLinkUrl result handling, the fs.access
error logging, and send/sendQueuedEventsOnReady to use the sanitized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d564339-baff-4eb6-a01a-10fc4906550d
📒 Files selected for processing (3)
src/main-process/appWindow.tssrc/utils/deepLink.tstests/unit/utils/deepLink.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/deepLink.ts
- tests/unit/utils/deepLink.test.ts
Register a custom `comfy://` protocol so that URLs like `comfy://open?file=/path/to/workflow.json` open local workflow files in the app — whether it's already running or launched fresh. - Add URL parser utility (src/utils/deepLink.ts) for parsing comfy:// URLs - Register protocol via app.setAsDefaultProtocolClient on startup - Handle macOS open-url events (including pre-app-ready via early listener) - Handle Windows/Linux deep links via process.argv and second-instance - Add DEEP_LINK_OPEN IPC channel with type-safe definitions - Expose onDeepLinkOpen in preload with unsubscribe support - Queue pending deep links during startup, deliver after frontend loads - Clear pending URL after consumption to prevent replay on restart - Add electron-builder protocols config for packaged builds - Add 22 unit tests covering URL parsing and AppWindow.handleDeepLink Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… files Address code review findings: - Switch from sync fs.existsSync to async fs.access to avoid blocking the main process event loop on slow filesystems - Show an error dialog when a deep link points to a missing file, so the user gets feedback instead of silent failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cheme matching Address CodeRabbit review findings: - Add event.defaultPrevented check in AppWindow open-url handler to prevent duplicate deep link processing during startup listener overlap - Make findDeepLinkUrl case-insensitive so COMFY:// URLs are matched Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MillerMedia
force-pushed
the
feat/deep-link-protocol-handler
branch
from
a6c17bc to
6c1e468
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/main-process/appWindow.test.ts (1)
164-214: Use platform-neutral file paths in deep-link tests.These tests currently hard-code POSIX paths (e.g.,
/path/to/workflow.json). Preferpath.join/path.normalize/path.sepso assertions remain explicitly cross-platform.♻️ Proposed refactor
+import path from 'node:path';- it('should send DEEP_LINK_OPEN IPC for a valid comfy://open URL with existing file', async () => { + it('should send DEEP_LINK_OPEN IPC for a valid comfy://open URL with existing file', async () => { vi.mocked(fs.access).mockResolvedValue(); + const workflowPath = path.normalize(path.join(path.sep, 'path', 'to', 'workflow.json')); - await appWindow.handleDeepLink('comfy://open?file=/path/to/workflow.json'); + await appWindow.handleDeepLink(`comfy://open?file=${encodeURIComponent(workflowPath)}`); - expect(sendSpy).toHaveBeenCalledWith(IPC_CHANNELS.DEEP_LINK_OPEN, '/path/to/workflow.json'); + expect(sendSpy).toHaveBeenCalledWith(IPC_CHANNELS.DEEP_LINK_OPEN, workflowPath); }); it('should not send IPC when file does not exist', async () => { vi.mocked(fs.access).mockRejectedValue(new Error('ENOENT')); + const missingPath = path.normalize(path.join(path.sep, 'nonexistent', 'file.json')); - await appWindow.handleDeepLink('comfy://open?file=/nonexistent/file.json'); + await appWindow.handleDeepLink(`comfy://open?file=${encodeURIComponent(missingPath)}`); expect(sendSpy).not.toHaveBeenCalled(); expect(dialog.showErrorBox).toHaveBeenCalledWith( 'File Not Found', - expect.stringContaining('/nonexistent/file.json') + expect.stringContaining(missingPath) ); });As per coding guidelines: "Tests should be cross-platform compatible; ensure proper use of
path.normalize,path.join, andpath.sepfor Windows, macOS, and Linux compatibility."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/main-process/appWindow.test.ts` around lines 164 - 214, Replace hard-coded POSIX file path literals in the deep-link unit tests with platform-neutral paths using Node's path utilities: use path.join or path.normalize to build the file paths passed into appWindow.handleDeepLink and use the normalized value in assertions (e.g., for sendSpy expect and for dialog.showErrorBox expect.stringContaining). Update the test cases that call appWindow.handleDeepLink('comfy://open?file=...') and the corresponding expect(sendSpy) and expect(dialog.showErrorBox) checks to reference the normalized path variable instead of raw '/path/...' or '/nonexistent/...'; keep mocks (vi.mocked(fs.access)), and window helpers (mockIsMinimized, mockRestore, mockFocus) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/main-process/appWindow.test.ts`:
- Around line 41-44: The test accesses a non-public member appWindow.url causing
type errors; update the code to use a public API instead by either (A) changing
the test to call a public accessor (e.g., appWindow.getUrl()) or (B) adding a
small public getter on the AppWindow class (e.g., public getUrl(): string {
return this.internalUrl; }) that returns the internal URL, then update the test
to assert on appWindow.getUrl() being defined; reference the AppWindow
constructor and the new or existing getUrl/getter when making the change.
---
Nitpick comments:
In `@tests/unit/main-process/appWindow.test.ts`:
- Around line 164-214: Replace hard-coded POSIX file path literals in the
deep-link unit tests with platform-neutral paths using Node's path utilities:
use path.join or path.normalize to build the file paths passed into
appWindow.handleDeepLink and use the normalized value in assertions (e.g., for
sendSpy expect and for dialog.showErrorBox expect.stringContaining). Update the
test cases that call appWindow.handleDeepLink('comfy://open?file=...') and the
corresponding expect(sendSpy) and expect(dialog.showErrorBox) checks to
reference the normalized path variable instead of raw '/path/...' or
'/nonexistent/...'; keep mocks (vi.mocked(fs.access)), and window helpers
(mockIsMinimized, mockRestore, mockFocus) unchanged.
🪄 Autofix (Beta)
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
Run ID: e86bd139-497c-4a95-b782-80a398544121
📒 Files selected for processing (10)
builder-debug.config.tssrc/constants.tssrc/desktopApp.tssrc/infrastructure/ipcChannels.tssrc/main-process/appWindow.tssrc/main.tssrc/preload.tssrc/utils/deepLink.tstests/unit/main-process/appWindow.test.tstests/unit/utils/deepLink.test.ts
✅ Files skipped from review due to trivial changes (3)
- builder-debug.config.ts
- src/utils/deepLink.ts
- src/main.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/constants.ts
- src/infrastructure/ipcChannels.ts
- tests/unit/utils/deepLink.test.ts
- src/main-process/appWindow.ts
- Remove test referencing non-existent AppWindow.url property (TS2339) - Show hidden-to-tray windows before focusing on deep link open
Author
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main-process/appWindow.ts (1)
178-196:⚠️ Potential issue | 🟡 MinorRedact sensitive paths from log output and differentiate
fs.access()error types.Two unresolved concerns from prior reviews:
Sensitive data in logs: Lines 181, 186, and 193 write full
comfy://URLs and local file paths (potentially containing usernames/home directories) to logs.Error type conflation: The
catchblock (lines 192-195) treats allfs.access()failures as "File Not Found", butEACCES(permission denied) and other I/O errors should be reported differently.Suggested fix
public async handleDeepLink(url: string): Promise<void> { const result = parseDeepLinkUrl(url); if (!result) { - log.warn(`Invalid deep link URL: ${url}`); + log.warn('Invalid deep link URL received'); return; } if (result.action !== 'open') { log.warn(`Unsupported deep link action: ${result.action}`); return; } try { await fs.access(result.filePath); - } catch { - log.error(`Deep link file not found: ${result.filePath}`); - dialog.showErrorBox('File Not Found', `The workflow file could not be found:\n\n${result.filePath}`); + } catch (error: unknown) { + const code = error instanceof Error && 'code' in error ? (error as { code: string }).code : undefined; + if (code === 'ENOENT') { + log.error('Deep link file not found'); + dialog.showErrorBox('File Not Found', `The workflow file could not be found:\n\n${result.filePath}`); + } else { + log.error('Deep link file access error', error); + dialog.showErrorBox('File Access Error', `Could not access the workflow file:\n\n${result.filePath}`); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main-process/appWindow.ts` around lines 178 - 196, In handleDeepLink, avoid logging full sensitive paths/URLs from parseDeepLinkUrl by creating a redactedPath (e.g., show only filename or last N segments or replace home dir with ~) and use that in log.warn/log.error and dialog.showErrorBox instead of result.filePath or the raw URL; also refine the fs.access() catch: inspect the thrown error.code and handle ENOENT by logging "File Not Found" and showing the redactedPath, handle EACCES by logging "Permission denied accessing" and show a permission error dialog, and for any other error log and show a generic I/O error mentioning the redactedPath and include the actual error.code or message in the internal log only (not in user-facing dialogs); reference the functions/identifiers handleDeepLink, parseDeepLinkUrl, fs.access, log.warn, log.error, and dialog.showErrorBox when making the changes.
🧹 Nitpick comments (2)
tests/unit/main-process/appWindow.test.ts (2)
193-209: Consider adding a test for showing hidden windows.The test suite covers minimized windows but doesn't test the
isVisible() → show()path that was added to handle windows hidden to tray. This was a specific fix from prior review feedback.Suggested test to add
it('should show hidden window when handling a valid deep link', async () => { vi.mocked(fs.access).mockResolvedValue(); const mockIsVisible = vi.fn(() => false); const mockShow = vi.fn(); // Update the BrowserWindow mock to include isVisible and show vi.mocked(BrowserWindow).mockImplementation( () => ({ // ... existing mocks ... isVisible: mockIsVisible, show: mockShow, isMinimized: vi.fn(() => false), restore: vi.fn(), focus: vi.fn(), }) as unknown as BrowserWindow ); const hiddenAppWindow = new AppWindow(undefined, undefined, false); vi.spyOn(hiddenAppWindow, 'send').mockImplementation(vi.fn()); await hiddenAppWindow.handleDeepLink('comfy://open?file=/path/to/workflow.json'); expect(mockShow).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/main-process/appWindow.test.ts` around lines 193 - 209, Add a unit test that verifies the isVisible() → show() path when handling deep links: mock fs.access to resolve, extend the BrowserWindow mock to include isVisible returning false and a mock show(), construct a new AppWindow instance (via new AppWindow(...)), spy on its send method, call handleDeepLink('comfy://open?file=/path/to/workflow.json') on that instance, and assert that show() was called; reference BrowserWindow mock, isVisible, show, AppWindow, and handleDeepLink to locate where to add this test.
112-115: Consider moving thefsmock to the top of the file with other mocks.While
vi.mockcalls are hoisted in Vitest, placing thenode:fs/promisesmock between test suites (lines 112-115) reduces readability. Grouping all mocks together at the top improves maintainability.Suggested refactor
vi.mock('electron', () => ({ // ... electron mock })); +vi.mock('node:fs/promises', () => ({ + default: { access: vi.fn() }, + access: vi.fn(), +})); + describe('AppWindow', () => { // ... }); describe('AppWindow popup handler', () => { // ... }); -vi.mock('node:fs/promises', () => ({ - default: { access: vi.fn() }, - access: vi.fn(), -})); - describe('AppWindow.handleDeepLink', () => { // ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/main-process/appWindow.test.ts` around lines 112 - 115, The vi.mock call for 'node:fs/promises' is placed mid-file reducing readability; move the vi.mock('node:fs/promises', ...) declaration up with the other mocks near the top of the test file so all mocks are grouped together; update references to the mocked symbols (the default export mock and access mock created by vi.fn()) to remain unchanged (keep the same mock shape) and ensure no test-specific setup relies on its current position (e.g., in appWindow.test.ts, relocate the vi.mock('node:fs/promises', () => ({ default: { access: vi.fn() }, access: vi.fn() })) block to the top alongside other vi.mock calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/main-process/appWindow.test.ts`:
- Around line 134-152: The BrowserWindow test fixture used in appWindow.test.ts
is missing a mock for isVisible which handleDeepLink calls; update the mocked
BrowserWindow implementation (the vi.mocked(BrowserWindow).mockImplementation
block) to include an isVisible property (e.g., vi.fn(() => true) or configurable
mock) so tests exercising handleDeepLink and this.window.isVisible() behave
correctly and can assert visibility-dependent behavior.
- Around line 8-17: The electron mock in
tests/unit/main-process/appWindow.test.ts is missing several exports used by
appWindow.ts and its dependency ipcChannels.ts; update the vi.mock('electron',
...) to include ipcRenderer (with methods like on/send/invoke as needed) plus
Menu, Tray, app, nativeTheme, and shell stubs in addition to the existing
BrowserWindow, ipcMain, dialog, and screen.getPrimaryDisplay; ensure the mock
provides the specific methods your code calls (e.g., ipcRenderer.invoke/on,
app.whenReady/isReady or event handlers, Menu.buildFromTemplate, Tray
constructor, nativeTheme.shouldUseDarkColors, shell.openExternal) so imports in
ipcChannels.ts and appWindow.ts resolve and CI passes.
---
Duplicate comments:
In `@src/main-process/appWindow.ts`:
- Around line 178-196: In handleDeepLink, avoid logging full sensitive
paths/URLs from parseDeepLinkUrl by creating a redactedPath (e.g., show only
filename or last N segments or replace home dir with ~) and use that in
log.warn/log.error and dialog.showErrorBox instead of result.filePath or the raw
URL; also refine the fs.access() catch: inspect the thrown error.code and handle
ENOENT by logging "File Not Found" and showing the redactedPath, handle EACCES
by logging "Permission denied accessing" and show a permission error dialog, and
for any other error log and show a generic I/O error mentioning the redactedPath
and include the actual error.code or message in the internal log only (not in
user-facing dialogs); reference the functions/identifiers handleDeepLink,
parseDeepLinkUrl, fs.access, log.warn, log.error, and dialog.showErrorBox when
making the changes.
---
Nitpick comments:
In `@tests/unit/main-process/appWindow.test.ts`:
- Around line 193-209: Add a unit test that verifies the isVisible() → show()
path when handling deep links: mock fs.access to resolve, extend the
BrowserWindow mock to include isVisible returning false and a mock show(),
construct a new AppWindow instance (via new AppWindow(...)), spy on its send
method, call handleDeepLink('comfy://open?file=/path/to/workflow.json') on that
instance, and assert that show() was called; reference BrowserWindow mock,
isVisible, show, AppWindow, and handleDeepLink to locate where to add this test.
- Around line 112-115: The vi.mock call for 'node:fs/promises' is placed
mid-file reducing readability; move the vi.mock('node:fs/promises', ...)
declaration up with the other mocks near the top of the test file so all mocks
are grouped together; update references to the mocked symbols (the default
export mock and access mock created by vi.fn()) to remain unchanged (keep the
same mock shape) and ensure no test-specific setup relies on its current
position (e.g., in appWindow.test.ts, relocate the vi.mock('node:fs/promises',
() => ({ default: { access: vi.fn() }, access: vi.fn() })) block to the top
alongside other vi.mock calls).
🪄 Autofix (Beta)
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
Run ID: 5fe4dc77-82cf-40d8-832c-1cbb1bf30a48
📒 Files selected for processing (2)
src/main-process/appWindow.tstests/unit/main-process/appWindow.test.ts
✅ Actions performedComments resolved. Approval is disabled; enable |
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
2 participants
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
comfy://custom protocol handler so thatcomfy://open?file=/path/to/workflow.jsonopens local workflow files in the appopen-urlevent), Windows/Linux (process.argv+second-instance)DEEP_LINK_OPENIPC channel with message queuing for startup timingChanges
New files
src/utils/deepLink.ts— Pure utility for parsingcomfy://URLs. Extracts action and file path. No Electron imports, reusable by future MCP integration.tests/unit/utils/deepLink.test.ts— 16 tests covering valid URLs, encoded paths, Windows paths, missing params, malformed URLs, case-insensitive actions.Modified files
src/main.ts— Registers protocol viasetAsDefaultProtocolClient, adds earlyopen-urllistener for macOS pre-ready URLs, scansprocess.argvfor Windows/Linux fresh launch.src/main-process/appWindow.ts— AddshandleDeepLink(url)method (parse, validate file exists, send IPC, focus window). ExtendssetupAppEvents()withsecond-instancedeep link extraction andopen-urlhandler.src/desktopApp.ts— Accepts optionalpendingDeepLinkUrlin constructor. Consumes and clears it after frontend loads to prevent replay on troubleshooting restart.src/preload.ts— ExposesonDeepLinkOpenwith unsubscribe return function for renderer to listen for incoming workflow files.src/constants.ts— AddsDEEP_LINK_OPENIPC channel name.src/infrastructure/ipcChannels.ts— Adds type definition for the new IPC channel.builder-debug.config.ts— Addsprotocolsconfig for electron-builder local builds.tests/unit/main-process/appWindow.test.ts— 6 new tests forhandleDeepLink: valid URL sends IPC, invalid/unsupported/missing file no-ops, window focus/restore behavior.Design decisions
AppWindow.send()which queues messages when the renderer isn't ready yet, so deep links received during startup are delivered once the frontend loads.parseDeepLinkUrlhas no Electron dependencies, making it reusable for future MCP integration.open-urlfires beforeapp.ready, so an early listener captures it, then is removed onceAppWindowtakes over to avoid duplicate handlers.fileparam is decoded but never used to fetch remote resources.Frontend integration (out of scope)
The ComfyUI frontend needs to implement a handler for
window.electronAPI.onDeepLinkOpen(filePath => ...)to load the workflow from the given file path. This PR delivers the Electron-side plumbing only.Test plan
second-instancedeep link delivery via dev mode test (URL parsed, file validated,handleDeepLinkinvoked)open comfy://open?file=/path/to/workflow.json(requires packaged app withInfo.plistprotocol registration)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
┆Issue is synchronized with this Notion page by Unito