[backport core/1.45] [bugfix] Use Desktop2 bridge for missing model downloads#12738
Merged
Conversation
## Summary Fixes the Desktop2 missing-model download path so the frontend calls the Desktop2 download bridge directly when it is available, instead of relying on the browser `<a download>` fallback that Desktop2 currently has to intercept indirectly. This addresses Linear FE-956, where missing-model downloads on Windows could open the OS Save As dialog. The issue was reproducible when the frontend language was not English: switching the UI language back to English made the download succeed again. ## Root Cause Desktop2 currently has compatibility logic that watches/intercepts the frontend missing-model download flow from outside the FE code. That interception depends on FE-rendered DOM details, including localized accessible labels such as the missing-model download button `aria-label`. In English, Desktop2 could find the expected download controls and cache the missing-model metadata before the FE-created `<a>` download was clicked. In non-English locales, the localized label no longer matched Desktop2's selector, so the Desktop2 interception path missed the download. The FE then continued down the browser download path, which Electron surfaced as a native Save As dialog on Windows. ## Changes - Adds a narrow Desktop2 runtime bridge check in `missingModelDownload.ts`: - if `window.__comfyDesktop2.downloadModel` exists - and `window.__comfyDesktop2Remote` is not set - then FE calls `window.__comfyDesktop2.downloadModel(model.url, model.name, model.directory)` directly and returns early. - Keeps remote Desktop2 sessions on the existing browser fallback path by preserving the `__comfyDesktop2Remote` guard. - Leaves the existing OSS browser fallback and legacy desktop `isDesktop` download-store path intact. - Logs Desktop2 bridge failures so rejected promises or synchronous bridge throws do not become unhandled errors. - Adds regression coverage for: - Desktop2 bridge path taking priority over browser and legacy desktop fallbacks. - rejected Desktop2 bridge calls being logged without falling back to browser download. - synchronously thrown Desktop2 bridge failures being logged without crashing or falling back to browser download. - remote Desktop2 sessions continuing to use browser fallback. ## User Impact Desktop2 users should no longer depend on localized FE DOM text for missing-model downloads. In particular, non-English UI locales should route missing-model downloads through Desktop2's managed downloader instead of opening the OS Save As dialog. ## Validation - Manually verified the issue is fixed in Desktop2 using a locally built FE dist served through ComfyUI with `--front-end-root`. - Verified Korean locale no longer triggers the Save As dialog and the missing-model download succeeds through Desktop2. - Verified the new regression test fails when the production bridge fix is reverted. - Covered the FE-side contract with unit tests because a true end-to-end assertion of the Windows native Save As dialog is not currently practical in the FE browser-test infrastructure. The FE tests can verify that clicking missing-model download routes into `window.__comfyDesktop2.downloadModel`; they cannot directly prove Electron/Windows native dialog behavior. That full native-dialog regression belongs in Desktop2/Electron integration coverage. - Ran: - `pnpm exec oxfmt --check src/platform/missingModel/missingModelDownload.ts src/platform/missingModel/missingModelDownload.test.ts` - `pnpm lint:unstaged` - `pnpm exec vitest run src/platform/missingModel/missingModelDownload.test.ts` - `pnpm typecheck` - `pnpm build` - Pre-commit hook passed: `oxfmt`, `oxlint`, `eslint`, `typecheck`. - Pre-push hook passed: `knip --cache` completed with existing tag hints only. - Ran a 3-round local Claude review loop; final verdict was approve with no Blocker/Major findings. ## Follow-up Work - Define and document the FE/Desktop2 bridge contract explicitly, including the expected semantics of `downloadModel` resolving `false` versus rejecting. - Add a shared or canonical TypeScript declaration for `window.__comfyDesktop2` and `window.__comfyDesktop2Remote` if more FE code starts depending on these globals. - Remove Desktop2's DOM/aria/class-based missing-model download interception after a sufficient FE compatibility window, so Desktop2 no longer depends on FE DOM structure or localized labels. - Add Desktop2 integration/e2e coverage for missing-model downloads in non-English locales, ideally including Windows where the Save As dialog was observed. This is the right layer for a true native Save As regression test. - Optionally add a lighter FE browser E2E that injects a fake `window.__comfyDesktop2.downloadModel` and verifies the missing-model UI calls that bridge. This would validate the FE contract, but it would still not replace Desktop2/Electron coverage for native dialog behavior. - Decide on user-facing failure UX for Desktop2 bridge download failures once Desktop2 defines whether failures, cancellations, and already-queued downloads are represented by rejection or by `false`. ## Notes This intentionally does not fall back to browser download when the Desktop2 bridge resolves `false`. Falling back there could reintroduce the exact Save As dialog behavior this PR fixes, and the meaning of `false` should be clarified in the Desktop2 bridge contract before FE invents user-facing behavior for it. A true E2E test for this bug would need to exercise Desktop2/Electron on Windows and assert that the native Save As dialog is not opened. The current FE browser-test infrastructure cannot observe that native Desktop2 behavior directly, so this PR uses focused unit regression coverage for the FE routing contract plus manual Desktop2 verification.
dosubot
Bot
added
the
size:M
🎨 Storybook: ✅ Built — View Storybook |
jaeone94
approved these changes
Jun 9, 2026
🎭 Playwright: ✅ 1645 passed, 0 failed · 2 flaky📊 Browser Reports
|
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## core/1.45 #12738 +/- ##
=============================================
+ Coverage 60.49% 60.50% +0.01%
=============================================
Files 1418 1418
Lines 72870 72878 +8
Branches 19356 19357 +1
=============================================
+ Hits 44081 44096 +15
+ Misses 28315 28308 -7
Partials 474 474
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Backport of #12710 to
core/1.45Automatically created by backport workflow.