Skip to content

fix(adopt): fail clearly when Legacy Desktop adoption can't source ComfyUI#942

Open
Kosinkadink wants to merge 1 commit into
mainfrom
fix/legacy-desktop-adopt-source-missing-clear-failure
Open

fix(adopt): fail clearly when Legacy Desktop adoption can't source ComfyUI#942
Kosinkadink wants to merge 1 commit into
mainfrom
fix/legacy-desktop-adopt-source-missing-clear-failure

Conversation

@Kosinkadink

Copy link
Copy Markdown
Member

Summary

When in-place adoption of a Legacy Desktop install could not obtain the ComfyUI source (no pre-swap staged copy and the git clone failed), the source-missing dialog offered a "Switch to managed env" button. Choosing it threw source-missing-switch-to-managed, which the migrate dispatcher mapped to { ok: true, navigate: 'new-install' }.

But navigate: 'new-install' is unhandled in the renderer (only 'list'/'detail' are), so the operation reported success while doing nothing — leaving the user back where they started (the legacy detail view on the dashboard-tile path, or the chooser on the first-time-migration path). The button label was also misleading: it sounded like a lightweight env swap but really pointed at a fresh install that abandons the legacy data and launch args.

Fix

Replace the fake-success escape with a clear failure (per #917):

  • Drop the "Switch to managed env" option from the source-missing prompt; keep Retry (transient network/git failures) and Cancel.
  • On any non-retry choice, throw a source-missing: error that the dispatcher surfaces as { ok: false, message } with a user-friendly message that suggests doing a new install.
  • This fixes both the dashboard-tile and first-time-migration surfaces at once, since both flow through the same backend + ProgressModal error banner.

Also fixes a latent bug: an unexpected prompt choice previously break-ed the source loop silently, leaving sourceMode null and adoption continuing without a source.

Changes

  • desktopAdopt.ts — drop 'switch-to-managed' from UserChoice; source loop throws a clear error on anything but explicit retry.
  • migrate.ts — remove the "Switch to managed env" button and the fake-success remap; source failures return { ok: false, message } with a friendly message.
  • locales/en.json — remove adoptPromptSwitchToManaged, clarify the prompt message, add adoptSourceMissingFailed.
  • errorBucket.ts — update stale comment (source_missing bucketing still works).
  • migrate.test.ts — assert fail-clearly behavior.

Verification

  • pnpm typecheck / lint / build pass.
  • migrate.test.ts, desktopAdopt.test.ts, telemetry.test.ts pass.

Closes #917

…mfyUI

When in-place adoption of a Legacy Desktop install could not obtain the
ComfyUI source (no pre-swap staged copy and the git clone failed), the
source-missing dialog offered a 'Switch to managed env' button. Choosing
it threw 'source-missing-switch-to-managed', which the migrate dispatcher
mapped to { ok: true, navigate: 'new-install' }. But navigate:'new-install'
is unhandled in the renderer, so the operation reported success while
doing nothing — leaving the user back where they started (detail view on
the dashboard-tile path, or the chooser on the first-time-migration path).

Replace the fake-success escape with a clear failure: drop the
'Switch to managed env' option, keep Retry, and on any non-retry choice
throw a source-missing error that the dispatcher surfaces as
{ ok: false, message } with a user-friendly message suggesting a new
install. This fixes both the dashboard-tile and first-use migration
surfaces at once, since both flow through the same backend + ProgressModal
error banner.

Also fixes a latent bug where an unexpected prompt choice would silently
break the source loop, leaving sourceMode null and adoption continuing
without a source.

Closes #917

Amp-Thread-ID: https://ampcode.com/threads/T-019e96a8-4f9f-709a-920e-b7659a9140a7
Co-authored-by: Amp <amp@ampcode.com>
@Kosinkadink

Copy link
Copy Markdown
Member Author

Manual testing still pending

Automated checks pass (typecheck, lint, build, and the migrate / desktopAdopt unit tests), but the Legacy Desktop adoption happy path has not yet been verified end‑to‑end on a real machine. Recording what's left so it isn't missed before merge.

What still needs a manual run

  1. Dashboard tile → adopt (happy path). From the dashboard, click the ComfyUI Legacy Desktop tile and confirm it now runs the adopt flow cleanly — no "Switch to managed env" option, and a new adopted install is registered on success.
  2. Clear failure + retry on source-missing. Force the source‑missing condition (no staged copy + clone unavailable) and confirm the user now gets a clear error with a Retry option, and that cancelling does not silently fall through to a fake "new install" success.
  3. No user data loss on failure. Confirm that an aborted/failed adoption removes only the new adopted install dir under ComfyUI-Installs\ and never touches the legacy base dir (user/, input/, output/).
  4. Startup‑arg porting. Confirm the adopted install's launchArgs reflect the legacy Desktop settings. With an empty legacy Comfy.Server.LaunchArgs the synthesized default should be --listen 127.0.0.1 --port 8000 --enable-manager. A richer check (setting a server‑config UI toggle in legacy Desktop first, then confirming it carries into launchArgs) is still worth doing.

Already confirmed by code inspection

  • Dashboard tile for sourceId === 'desktop' routes to adoptDesktopInstall (adoption is the migration method, not standalone snapshot migration).
  • On failure, migrate.ts removes the adopted record + rms the adopted installPath only — legacy base dir is untouched.
  • The old fallback (source-missing → fake { ok: true, navigate: 'new-install' }) was a no‑op that the renderer ignored; this PR replaces it with a clear failure message.

A local test environment has been staged (clean fresh‑adopt state, marker cleared, full backup taken); just need to drive the UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migration - do we really have a fallback that automatically tries to do a new install when the adoption fails?

1 participant