feat: open gated model access pages in the Comfy session#1275
Conversation
📝 WalkthroughWalkthroughChangesThe change adds a validated Hugging Face model access window, connects it through a new IPC handler and preload bridge method, and tests URL validation, session wiring, security controls, lifecycle behavior, loading, and rejection paths. Model access page
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant comfyPreload
participant registerDownloadHandlers
participant openModelAccessPageWindow
participant BrowserWindow
Renderer->>comfyPreload: openModelAccessPage(url)
comfyPreload->>registerDownloadHandlers: invoke desktop2-open-model-access-page
registerDownloadHandlers->>openModelAccessPageWindow: pass sender and url
openModelAccessPageWindow->>BrowserWindow: create or reuse session-linked window
openModelAccessPageWindow->>BrowserWindow: loadURL(url)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/main/lib/modelAccessPage.ts`:
- Around line 22-53: Update openModelAccessPageWindow to track the active access
BrowserWindow in a shared activeAccessWindow reference; if it exists and is not
destroyed, focus it and return without creating another window. Clear the
reference when the window closes or fails to load, while preserving the existing
creation and URL validation behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc8e6d05-9a8c-485b-ac62-f242760756cb
📒 Files selected for processing (4)
src/main/lib/ipc/registerDownloadHandlers.tssrc/main/lib/modelAccessPage.test.tssrc/main/lib/modelAccessPage.tssrc/preload/comfyPreload.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/main/lib/modelAccessPage.ts`:
- Around line 27-30: Update modelRepositoryPath to allow only valid Hugging Face
model repository routes, rejecting reserved namespaces such as datasets, login,
and settings before returning a trusted path. Preserve the existing
owner/repository validation, and add table-driven tests covering accepted model
paths and rejected reserved or non-model routes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac96aa32-d8fd-47c2-9ce9-092b772a47d8
📒 Files selected for processing (4)
src/main/lib/ipc/registerDownloadHandlers.test.tssrc/main/lib/modelAccessPage.test.tssrc/main/lib/modelAccessPage.tssrc/preload/comfyPreload.test.ts
|
Hold on a second actually |
New finding in happy path that I would actually say is blocking
There was a problem hiding this comment.
Below verified manually
Bug: closing the Hugging Face window throws TypeError: Object has been destroyed in the main process
The closed handler's first line accesses accessWindow.webContents, but the window is already destroyed when closed fires — verified on Electron 40.4.1 for both close() and destroy(). Every close path hits it, including the happy path (sign in, accept terms, close the window).
The app only registers uncaughtExceptionMonitor, so the exception goes unhandled and packaged builds show Electron's native "A JavaScript error occurred in the main process" dialog on every close. The throw also skips the rest of the handler (stale accessWindows entry, leaked parent closed listener). Tests miss it because the mocked webContents never throws after destroy.
Fix — capture the reference while the window is alive:
const accessContents = accessWindow.webContents
accessWindow.once('closed', () => {
accessPageContents.delete(accessContents)
...
})
|
@benceruleanlu Thanks for catching and manually verifying this. Fixed in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/main/lib/modelAccessPage.test.ts`:
- Line 179: Update the test assertion around accessWindowListeners.get('closed')
to remove optional chaining, so the test invokes the required closed listener
directly and fails when it is missing rather than silently skipping the
assertion.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4ecca556-c1a1-4cec-94f5-b70ab083f606
📒 Files selected for processing (2)
src/main/lib/modelAccessPage.test.tssrc/main/lib/modelAccessPage.ts
2 participants
Summary
This is the Desktop half of the gated Hugging Face model access flow introduced in Comfy-Org/ComfyUI_frontend#13742.
The frontend PR adds a lock action beside missing models that appear to require Hugging Face authorization while leaving the normal Download action intact. This paired PR adds an explicit
openModelAccessPage()Desktop bridge implementation so that action opens the repository in a Hugging Face-only Desktop window using the same Electron session as the originating ComfyUI view.Sharing the session is the important part: after the user signs in and accepts the repository terms, the existing Desktop model download manager uses that same session for
session.downloadURL(). The user can return to ComfyUI, click Download again, and complete the download without a separate OAuth or token flow.Changes
openModelAccessPage(url)from the ComfyUI preload bridge.desktop2-open-model-access-pageIPC handler.BrowserWindowconfigured with the sender's session, no preload,nodeIntegration: false,contextIsolation: true, andsandbox: true.https://huggingface.co/<owner>/<repository>model repository URLs on the default HTTPS port as bridge entry points.https://huggingface.coorigin after the trusted repository window opens. This keeps Hugging Face home, search, settings, sign-in, sign-out, license, Spaces, and other repository flows usable.webContentswhile the window is alive so theclosedhandler can finish cleanup without dereferencing a destroyedBrowserWindow.ERR_ABORTEDnavigation as accepted, while returningfalsefor genuine initial load failures so the frontend can run its browser fallback.Paired Frontend Contract
The frontend contract is defined by Comfy-Org/ComfyUI_frontend#13742 as an optional bridge method:
The method resolves
truewhen Desktop takes ownership of the trusted access-page request. It resolvesfalsewhen the URL is not trusted, the originating ComfyUI host is unavailable, or the initial page load genuinely fails. Afalseresult allows the frontend to fall back to opening the page in the normal browser.Desktop currently consumes the published bridge-types package at
0.1.2, while the frontend PR advances that package to0.1.4. The preload uses a small local intersection type so both PRs can be reviewed and tested in parallel. Once0.1.4is published, Desktop can bump the package and remove that temporary local type without changing runtime behavior.User Flow
Intended Scope and Limitations
Review Focus
startModelDownload()use the originating ComfyUI sender session.BrowserWindowcreation and cannot be bypassed with HTTP, a lookalike hostname, a subdomain, a reserved route, or a non-repository path.https://huggingface.coorigin while all other origins remain blocked.falsewhile superseded navigation remains open.Validation
pnpm run typecheckpnpm run lintpnpm run buildpnpm run test(190files,2,778passed,1skipped)pnpm run test:integration(25passed)pnpm exec vitest run src/main/lib/modelAccessPage.test.ts(25passed, including destroyed-window cleanup)ERR_ABORTED (-3)in Electron and verified that the access window reaches the login page while the main process remains responsive.An automated E2E test is not included because this flow requires a real third-party Hugging Face account, a gated repository access grant, and persistent cookies in a running Electron session. Unit tests cover the deterministic bridge, trust boundary, lifecycle, and session wiring; the authenticated provider flow was verified manually.
Screenshots
2026-07-17.2.52.20.mov