Comfy-Org / Comfy-Org/Comfy-Desktop

docs: legacy-desktop-adopt-migration.md documents a decision that #998 reversed (one-shot ComfyUI checkout)

Open
#1,434 0 comments 0 reactions 1 assignee Claimed by @Kosinkadink View on GitHub
documentation tech-debt
Dominant language
TypeScript
Stars
458
Forks
59
Avg merge
22h 18m
Merged PRs (30d)
45

Description

`docs/legacy-desktop-adopt-migration.md` is the design doc for Legacy Desktop → Desktop adoption. One of its load-bearing decisions was **reversed in code three months ago** and the doc still states the old one as current, along with three citations that no longer resolve.

A plan-audit sweep across the repo's docs found this; the other docs checked clean (see "Also checked" below).

Verified against `origin/main` @ `f26ea50` (2026-08-20).

## 1. The one-shot `git checkout ` no longer happens — the doc says it does

The doc's **Adoption contract** section states:

> - One-shot `git checkout ` of the ComfyUI source tree. Adopting users are coming in through what they perceive as a ComfyUI update, so they expect a fresh server.

`src/main/lib/desktopAdopt.ts` (right after the `comfy.desktop.adopt.source` step) now carries a comment saying the **opposite**:

```
// Adoption preserves the user's existing ComfyUI checkout as-is — it is
// not auto-updated to latest stable. A "frozen" install must stay on
// whatever version the user was running; ComfyUI updates are opt-in per
// install (`autoUpdateComfyUI` stays `false` on the record) and can be
// triggered manually from the Update tab.
```

The reversal landed in #998 (`fix: keep frozen ComfyUI installs frozen on Load Snapshot and Track Existing (#986)`, 2026-06-09), which removed the checkout but did not touch any `.md` file. The doc has been edited twice since (#1281, #1376) and this paragraph survived both.

This is the expensive kind of stale: the doc still carries the *rationale* ("they expect a fresh server") for a behavior that was deliberately reversed for a correctness reason. Anyone planning follow-up work off this doc would build against the wrong contract.

## 2. `adoptedComfyTagAtMigration` is documented as a record field but is never written

The doc's **Per-install record fields** block lists:

```
adoptedComfyTagAtMigration?: tag chosen by the one-shot checkout
```

Nothing in production code sets it. The only references on `origin/main` are:

- `src/main/lib/desktopAdopt.test.ts:1307` — `expect(record).not.toHaveProperty('adoptedComfyTagAtMigration')`, i.e. the test asserts it is **absent**
- `src/main/lib/ipc/shared.ts:941` — destructured out so it is dropped on copy

It is also listed under "What this design intentionally does NOT do" adjacent text as a live field, which compounds the confusion.

## 3. Telemetry table cites a step event that does not exist, and omits three that do

The doc's telemetry table lists the step events as:

> `comfy.desktop.adopt.backup`, `tcc`, `validate_venv`, `snapshot`, `source`, **`comfy_update`**, `requirements`, `requirements_reconcile`, `carry_settings`, `register`

The actual set emitted from `src/main` is:

```
allocate backup carry_settings detect find_existing register
requirements requirements_reconcile snapshot source started
succeeded tcc validate_venv
```

- **`comfy_update` does not exist** — it was the step wrapping the removed checkout.
- **`detect`, `find_existing`, `allocate` are missing from the doc.**

Related: the doc lists `adopted_comfy_tag_at_migration` as a property of `comfy.desktop.adopt.succeeded`. It is still emitted, but hardcoded:

```ts
adopted_comfy_tag_at_migration: null,
```

so it is a dead column in PostHog rather than a real property. Worth either populating it or dropping it from both the doc and the payload.

## 4. "Open scope" lists an item that is already done

The doc's closing **Open scope (not yet on this branch)** section lists two pending items. The second:

> - Wire `comfy.desktop.adopt.*` telemetry properties so the migration funnel is visible end-to-end in PostHog.

This shipped. `adoptDesktopInstall` wraps every phase in `telemetry.trackedStep`, `comfy.desktop.adopt.succeeded` carries the full property set, and `telemetry.captureInstallCompleted({ method: 'adopt' })` fires the funnel event. The doc's own telemetry table documents all of it — the doc contradicts itself.

The first item (auto-trigger adoption from the first-launch takeover) **is** genuinely still open — no `autoAdopt` / auto-adopt path exists — so that one should stay.

## Suggested fix

One docs PR:

- Replace the one-shot-checkout bullet with the frozen-checkout behavior and its rationale from #998.
- Drop `adoptedComfyTagAtMigration` from the record-fields block, or note it as reserved-and-unset.
- Correct the step-event list; decide whether `adopted_comfy_tag_at_migration` is populated or removed.
- Delete the completed telemetry item from "Open scope", keep the auto-trigger item.

## Also checked (clean — recording so the next pass can skip them)

- `docs/instance-navigation-926-handoff.md` — all 14 cited paths and every symbol (`decideNavigation`, `navClass`, `pickInstallFromPicker`, `openInstallInNewWindow`, `confirmSwitch`, `allowDuplicate`, …) resolve; the 24-row behavior matrix still matches `navDecision.ts` + the caret filter in `ComfyUISettingsContent.vue`. **One stale word**: the Tests section says `useInstanceActions.test.ts` covers "cloud-capacity + kill-confirm gates" — the cloud-capacity gate was deleted wholesale in #1374 (`capacity` has zero occurrences under `src/` and `e2e/`). One-line fix, not worth its own issue.
- `docs/torch-index-stacks.md` — schema, `kind` rules, `indexTag` validation, the AMD multi-arch trust boundary, and the default-deny semantics all match `torchIndexManifest.ts`. Gap, not drift: the manifest also accepts an optional top-level **`series`** map (`TorchSeriesInfo`, `noteKey`/`note`/`minDriver`) that the doc's schema section does not mention, so a publisher following the doc would silently fall back to the in-app `INDEX_SERIES` defaults.
- `e2e/README.md` — all 40 cited spec filenames exist.
- `TESTING.md` — the four-category taxonomy, the `src/**/*.integration.test.ts` split, and the `ci.yml` / `lifecycle.yml` mapping all hold.
- Issue #900 (the only open plan-shaped issue) — commented separately; one row of its hydration matrix went stale a day after it was filed.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.