Skip to content

Feat/starter templates#1084

Open
MaanilVerma wants to merge 31 commits into
Comfy-Org:mainfrom
MaanilVerma:feat/starter-templates
Open

Feat/starter templates#1084
MaanilVerma wants to merge 31 commits into
Comfy-Org:mainfrom
MaanilVerma:feat/starter-templates

Conversation

@MaanilVerma

@MaanilVerma MaanilVerma commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a starter-template picker to the standalone install flow: pick a workflow (Image / Video / Audio / 3D) at install time and land on a ready-to-run canvas — the template auto-opens on first launch and its required models were pre-downloaded in the background — instead of a blank canvas with "missing model" errors. Picking nothing (Skip & Install) preserves today's blank-canvas behaviour exactly. The picker is gated behind a PostHog experiment (treatment shows it, control keeps the legacy flow), with funnel telemetry on each step.

Changes

What

  • bundledTemplates.ts — curated one-per-modality picks (flux_schnell, text_to_video_wan, audio_stable_audio_example, 3d_hunyuan3d_image_to_model); metadata copied verbatim from the live template index. Adds pure shouldWarnVram() and buildTemplateDeeplink() helpers.
  • TemplatePickerStep.vue — compact selectable-row picker (thumbnail + title + modality · ~size · VRAM ~x, description expands in the selected row), full keyboard nav (Arrow/Home/End move+select; Enter/Space via native button), live disk-block + VRAM-warning alerts, modality-glyph fallback on thumbnail load error. Thumbnails are bundled in-repo ('self', CSP-safe) since a fresh user has no local template package.
  • InstallWizardModal.vue — dedicated template step after Configure; Skip & Install vs Install; skipTemplatePickerStep setting + "Don't show again" (only when ≥1 local install). The step is A/B-gated (treatment only) and auto-skipped when free disk can't fit even the cheapest template. The wizard resets to Configure when the source changes.
  • installHelpers.ts — one shared disk decision: minTemplateModelBytes (cheapest of N) → templateDiskRequiredBytes (×1.1 headroom) → isTemplateDiskBlockedcheckTemplateDiskOrBlock. Drives the skip-the-step gate, the picker alert, the Install guard, and the save-time hard gate from one source so they can't drift.
  • templateModels.ts — resolves required models dynamically from the template's workflow JSON (site-packages first, GitHub-raw fallback), scans top-level/node-level/subgraph models[], whitelists hosts (HTTPS-only), sanitizes paths (no traversal), dedupes.
  • templateDownloadTask.ts + templateDownloadCore.ts — background download starting at install-begin; three-layer split (stateful task → pure unit-tested core → display reader). Bounded-concurrency pool (cap 3), 2× per-file retry, Windows MAX_PATH guard, gated-repo (401/403) messaging, surfaced disk + per-file errors.
  • launch.ts + launchPhases.ts — model download shown as the last launch step; serverUp-gated reader keeps the bar honest through real phases; launch gate holds the ComfyUI reveal until the download settles, with a footer "Skip & open ComfyUI" and, on failure, a "retry in-app" line + 3·2·1 countdown before opening.
  • attach.ts — first-launch ?template=<id>&source=default deeplink (zero frontend change; useTemplateUrlLoader opens it), one-shot cleared after.
  • comfyDownloadManager.ts — Skip hands the resume-capable task off to the title-bar downloads tray (no restart); the mirror is read-only. Template rows now also surface in the "All Downloads" modal (live progress + dismiss + clear-all), via the existing model-download-progress channel. Finished mirror rows are purged on dismiss / clear-finished so they don't linger.
  • index.ts + titlePopup.tsbefore-quit confirm when template downloads are still in flight; the downloads tray auto-opens ~2.5 s after the ComfyUI reveal when a template download is still running, so the user notices it.

Breaking

  • None. No frontend changes (deeplink only). New InstallationRecord fields (bundledTemplateId, pendingTemplateOpen, downloadTemplateModels) are all optional → existing installs load unchanged. IPC channels, specs, and telemetry are preserved (the All-Downloads integration reuses model-download-progress; no new channels).

Review Focus

  • Launch gate (launch.ts waitForTemplateDownloadGate + the serverUp reader) is the crux: it must show the download as the last step and hold the reveal without falsely marking earlier phases done, then redirect on done/skip/error. The settle condition is one pure primitive (awaitTemplateDownloadSettleddone | error | cancelled | skipped | aborted | absent).
  • Shared disk decision — the skip-the-step gate, the picker alert, the Install guard, and the save-time gate all call one isTemplateDiskBlocked() so they can't drift. Storage-too-small now skips the picker when nothing fits; otherwise the selected pick is blocked and clicking the (dimmed-but-clickable) Install shakes the error, mirroring the first-use consent nudge — no dead-end disabled button.
  • A/B gatingtreatment shows the picker, control keeps the legacy flow; fails closed to control on a slow/failed flag fetch; exposure + variant recorded once per session.
  • Cross-OS — disk (statfs), VRAM (nvidia-smi / Apple os.totalmem() / systeminformation fallback, never false-warns), path.join throughout, Windows-only MAX_PATH. Verified handled on all three OSes.
  • Scope boundariesInstallWizardModal.vue / ProgressModal.vue are already oversized; this PR adds to them rather than extracting composables. That refactor is a deliberate follow-up, not in scope here.

Testing

Unit + component coverage by design: the branchy logic is pure and Electron-free, so it's pinned without a window; the picker is component-tested; only the live launch flow (real ComfyUI server + multi-GB download) is manual, since the e2e harness never boots a real server. pnpm typecheck (node + web + e2e + integration), pnpm lint, and npx vitest run (2,286 tests) all green.

  • templateDownloadCore.test.ts — cumulative math, runPool concurrency/abort, withRetry budget + fatal-cancel, truncateForMaxPath, describeDownloadFailure (401/403 vs generic, no false-match), tray-entry mapping.
  • templateDownloadGate.test.tsawaitTemplateDownloadSettled resolves done / error (incl. disk pre-flight) / cancelled / skipped / aborted / absent; clears the stale skip flag on settle.
  • templateModels.test.tssanitizeModelPath (rejects traversal) + resolveTemplateModels URL/host (HTTPS-only) guards.
  • bundledTemplates.test.tsshouldWarnVram branches; buildTemplateDeeplink round-trip + malformed-URL fallback.
  • index.test.tsbuildInstallation: Skip (no template) builds no download; a real pick sets the id + one-shot + download flag.
  • installations.test.tsclearPendingTemplateOpen fires once, no-ops on a legacy record with no template fields, returns false when the install is gone.
  • comfyDownloadManager.test.ts — tray-mirror cleanup on dismiss/clear-finished; mirror rows appear in getAllDownloads() + fan out a model-download-progress broadcast per row.
  • TemplatePickerStep.test.ts — disk-block / VRAM alerts, Recommended tag, select + keyboard nav, Enter/Space native activation, thumbnail @error → glyph, nudgeDiskError() shakes when blocked / no-ops when not.
  • installHelpers.test.tstemplateDiskRequiredBytes headroom, isTemplateDiskBlocked, minTemplateModelBytes (none / smallest / ignores-zero) feeding the skip-the-picker gate.

Manual verification (pnpm dev) — every edge case:

# Scenario Steps Expected
1 Template picker appears New Install → Standalone → Configure → Continue Template step: 4 rows (Image "Recommended" + pre-selected, Video, Audio, 3D); thumbnail + title + modality · ~size · VRAM ~x
2 Thumbnails Look at all rows Real previews (bundled, offline-safe); no broken-image glyph, no title bleed
3 Select + keyboard Click rows; ↑/↓/Home/End; Enter/Space Yellow ring + check; description expands; keyboard moves and selects
4 Install a template Pick Image → Install "Downloading template models" is the last launch step; ComfyUI opens with the template on canvas
5 No stepper jump Watch the loader bar Advances smoothly through real phases; model step activates only at the end
6 Skip & Install Picker → Skip & Install No template, no download ([templateDownload] absent), blank canvas
7 Slow → gate hold → Skip → tray Heavy template; watch launch Holds on the last step (server up, download running) + "Skip & open ComfyUI"; click → opens; tray auto-opens ~2.5 s in and shows the download continuing (not restarted); the row also appears in "All Downloads"
8 VRAM warning GPU below a pick's recommendation (or lower recommendedVramBytes) Amber "may run slowly"; Install stays enabled; silent on AMD/Intel/unknown VRAM
9 Disk block + wiggle Heavy template on a small disk (free disk between the cheapest and this pick) Red disk error; Install reads disabled (dimmed) but is clickable — clicking it shakes the error (no install); a pick that fits clears it; Skip & Install always works
9b Picker auto-skipped (no fit) Volume too small for even the cheapest template; Configure → Continue Template step skipped entirely — Continue installs with no template (no picker, no error)
10 Don't-show-again With ≥1 install → wizard → template step Checkbox appears (absent for first-ever user); tick + Install → next New Install skips the step
11 Quit mid-download Heavy install, ⌘Q while downloading Confirm dialog ("Quit Anyway / Keep Downloading", default Keep)
12 Download error → countdown Go offline mid-download, or a gated repo Substatus red + bold + X; gate shows "failed — retry in-app" + 3·2·1 → ComfyUI opens automatically
13 Gated repo (401/403) Point a model at a license-gated HF repo Clear "requires a login or license — open in ComfyUI to sign in" log line; non-fatal, counts toward retry
14 Source change resets step Reach template step → Back → switch to a non-standalone source Returns to Configure; picker not shown for the new source
15 A/B gate Toggle the PostHog flag (or VITE_FORCE_TEMPLATE_PICKER=1) treatment shows the picker; control skips straight to install; exposure recorded once
16 Resize Wizard narrow → wide Rows reflow cleanly; no overflow/overlap; footer reachable
17 Cross-OS smoke Run on macOS / Windows / Linux Disk pre-check, VRAM detect, and model paths behave per-OS

…st launch

- Add bundledTemplate card field to the standalone source (wizard Advanced)
- Resolve a template's required models dynamically from its workflow JSON
  (site-packages first, GitHub-raw fallback; scans top/node/subgraph models[])
- Persist bundledTemplateId + one-shot pendingTemplateOpen on the install record
- Append ?template=<id>&source=default to the comfy URL on first launch
  (attach.ts), consumed once via clearPendingTemplateOpen; zero frontend changes
- Consent checkbox + downloadTemplateModels flag; starter-template i18n (en/zh)
…stall

- New templateDownloadTask: fire-and-forget at install-begin so bytes overlap
  env setup; own AbortController; per-file failures non-fatal
- New templateDownloadCore (pure, unit-tested): runPool (bounded concurrency 3),
  summarizeTemplateState (cumulative math), formatTemplateSubStatus
- Hot-path chunk callback is O(1) counters only; reader formats off-band
- Export getModelsBaseDir so install-time + in-window downloads agree on the dir
- Kick off from registerInstallationHandlers; abort on install cancel/window close
- Remove the old blocking postInstall download path
…h status + logs

- Splice a synthetic template-models phase into the launch stepper after the
  security scan; a 500 ms reader drives its substatus from shared state
- Rich substatus: speed, ETA, current file (N of M), cumulative X/Y GB
- Stream per-file logs into View logs; seed the launch op's terminal from the
  durable log ring buffer (new logs-snapshot IPC) so install-leg lines survive
- Fix the progress-bar leap: phase weight 0.05 + report indeterminate when the
  download was already complete, real percent (capped 99) while it runs
- What's done (deeplink + background download), decision matrix, files touched
- Phase 2 decided scope + edge-case dispositions + living build checklist
- Notes the live constants are the Phase-1 test set, not the Phase-2 picks
- Per-file 2× auto-retry around download() via a pure withRetry helper;
  a user cancel is fatal (no retry), and .dl-meta resume means a retry
  continues the partial rather than restarting it.
- Windows MAX_PATH guard (truncateForMaxPath) before each write; a name
  too long to fit becomes a per-file skip, not a task failure.
- Surface download failures in the substatus: red + bold + X icon, wired
  end-to-end (ProgressData.error → phaseErrors → ProgressStepVM.isError →
  BrandProgressView .is-error).
- In-task disk pre-check now a surfaced hard error with a disk-specific
  message (templateModelsNoSpace) instead of a silent skip.
- Unit tests for withRetry, truncateForMaxPath, and the disk-error branch.
- detectGPU() now returns vramBytes on any OS: nvidia-smi memory.total
  (authoritative) → os.totalmem() for Apple Silicon → systeminformation
  si.graphics() fallback for AMD/Intel/discrete. Reuses the graphics probe
  the app already runs for telemetry rather than hand-rolling native probes.
  Undefined only when no real number is readable, so the picker never
  false-warns. Exposed via the existing detect-gpu IPC + GPUInfo.vramBytes.
- shouldWarnVram(detected, recommended) pure helper in bundledTemplates:
  warns only when a real detected figure is below the template's
  recommendation; silent on undefined or no recommendation.
- BundledTemplate gains optional recommendedVramBytes (populated in group A).
- Unit tests for shouldWarnVram.
- checkTemplateDiskOrBlock (installHelpers): free disk vs the selected
  template's model size × 1.1 headroom, with NO continue-anyway — a block
  alert instead, so the user frees space or picks a lighter template rather
  than ending up with a half-downloaded model set + a confusing error row.
- Wired into InstallWizardModal.handleSave, after the existing install-bundle
  disk warn; only fires when a template-with-models is chosen and consented.
- Pure templateDiskRequiredBytes extracted for the threshold math + tested.
- en/zh copy: diskSpace.templateBlock{Title,Message}.
When the trailing template-models phase is still running, surface a centered
"Skip model download" button. Clicking hands the resume-capable background
task off to the title-bar downloads tray so the user can enter ComfyUI
immediately — no restart.

- comfyDownloadManager: a separate mirror registry (setTemplateTrayMirror /
  clearTemplateTrayMirror) merged into getDownloadsTrayState's active/recent.
  Kept out of pendingDownloads so the real-download DownloadItem lifecycle
  (cancel/retry/temp-rename) is untouched; the renderer needs no changes.
- templateDownloadTask: mirrorTemplateDownloadToTray polls the shared state
  every 500ms and reflects it via the pure templateStateToTrayEntries mapper
  until terminal; stopTemplateTrayMirror tears it down on window close.
- skip-template-download IPC + preload + ProgressModal button (gated on
  active=template-models, not-errored, <100%).
- Fix: guard op.phaseErrors access with optional chaining (partial op mocks).
- Tests for templateStateToTrayEntries.
- bundledTemplates.ts: one verified showcase per modality, metadata copied
  verbatim from the live workflow_templates index (title/description/size/vram):
  Image flux_schnell, Video text_to_video_wan, Audio audio_stable_audio_example,
  3D 3d_hunyuan3d_image_to_model. Each confirmed to embed a downloadable
  models[]. Dropped image_z_image_turbo (no embedded models → nothing
  pre-downloads) and the doc's stable-audio-3/triposplat ids (absent from the
  current index). Added modality + thumbnailUrl (/templates/<id>-1.webp) +
  recommendedVramBytes, passed through getFieldOptions data.
- Fix: the templates package was split into per-modality media sub-packages
  (…_media_image/_video/_other/…). loadTemplateJson only checked the legacy
  single package, so local resolution always missed and fell back to the slow,
  hang-prone remote path. Now probes all media packages first.
A full-screen template picker after Configure, before install — a modality
grid of showcase cards (thumbnail + size + modality chip) plus a "blank
canvas" option, with the image template pre-selected.

- TemplatePickerStep.vue renders the bundledTemplate FieldOptions the wizard
  already loads, so selecting drives selections.bundledTemplate. Warns (never
  blocks) when detected VRAM < the template's recommended VRAM; silent when
  VRAM is unknown. Hosts the model-download consent + "Don't show again".
- InstallWizardModal: step machine (configure → template); CTAs
  "Skip & Install" / "Install"; Back returns to Configure. The bundledTemplate
  Advanced card is hidden while the picker step is active (shown as the
  fallback when the picker is gated off).
- Gating: skipTemplatePickerStep setting; "Don't show again" only when
  getInstallationsSummary().localCount > 0; opted-out returning users
  auto-skip the step. Express install installs with no template by
  construction (picks the recommended "None" option).
- Dashboard "Add New Instance" inherits the step (same modal).
- en/zh copy + nested standalone.modality.* labels.
before-quit now checks hasActiveTemplateDownloads() and, when a template-model
download is still in flight, shows a synchronous confirm ("Quit Anyway" /
"Keep Downloading", default = keep) before tearing down. Quitting drops the
download (no resume), so the user gets one chance to back out. Only gates a
real user quit, not an in-progress relaunch/update quit.
All 26 checklist items across groups A–F landed; full suite (2138 tests),
all four typechecks, and eslint are green. Only live pnpm dev runtime
verification (G) remains — interactive, left for the engineer.
- Redesign the picker as a compact selectable row list (reuses
  brand-variant-list): thumbnail + title + meta (modality · size · VRAM),
  description expands inside the selected row, full keyboard nav. Drop the
  card grid, the "None" tile, and the consent toggle ("Skip & Install" is the
  blank-canvas / no-download path).
- Bundle the 4 modality thumbnails in-repo (public/images/templates/*.webp,
  downscaled) and point bundledTemplates at them; remote raw.githubusercontent
  previews were blocked by the renderer CSP (img-src 'self' data:).
- Live disk-block in the picker: selecting a too-large template shows a red
  error and disables Install. Extract the shared pure decision
  isTemplateDiskBlocked used by the picker, the wizard, and the save-time gate.
- Drop dead consent-sync in handleSave (buildInstallation derives
  downloadTemplateModels from the template id) and the unused defineExpose.
…eveal

- Move the template-models phase to the end of the launch stepper. Its 500ms
  reader stays silent until the server is up (serverUp flag) so the main
  tracker drives the real phases honestly; only then does the download become
  the active last row (no faked-done / 99% jump).
- Gate the ComfyUI reveal: at port-ready, if the download is still running,
  hold and show "Skip & open ComfyUI" instead of flashing past. Resolve on
  done / skip / cancel; on error (after 2x retry) show a failure line + 3-2-1
  countdown, then open. awaitTemplateDownloadSettled is the single settle
  primitive; requestSkipTemplateDownload releases the gate + mirrors to tray.
- Surface a clearer "requires login or license" line for a gated repo (401/403)
  via describeDownloadFailure; counts toward retry, non-fatal.
- Picker UI (compact rows, bundled thumbnails, no None/consent, live disk-block).
- Stepper: download-last + serverUp-gated reader + waitForTemplateDownloadGate
  (hold reveal until settle/skip; error countdown). Drop the reverted
  reachedLastRealPhase notes.
- 13-row human-reviewer test matrix.
- gate: awaitTemplateDownloadSettled resolves done/error/cancelled/
  skipped/aborted/absent + clears the stale skip flag on settle
- picker: TemplatePickerStep disk-block, VRAM warn, recommended tag,
  select/keyboard nav, thumbnail-fail glyph
- build: "Skip & Install" (none) builds no model download; a real
  pick sets bundledTemplateId + pendingTemplateOpen + downloadTemplateModels
- docs: rewrite handoff as a durable reference (mental model, decisions,
  edge cases, coverage map, 13-row review matrix); verified file links
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds starter-template selection, resolves template manifests, runs bounded/retryable background downloads with path truncation and tray mirroring, gates launch to await settlement with skip/abort, updates renderer install/progress UX and IPC/types, adds i18n, tests, and a handoff doc.

Changes

Starter Templates Feature

Layer / File(s) Summary
Template data model and install contract
src/main/sources/standalone/bundledTemplates.ts, src/main/sources/standalone/index.ts, src/main/installations.ts
Adds template types/catalog, bundledTemplate installer field, persists bundledTemplateId/pendingTemplateOpen/downloadTemplateModels in buildInstallation, and provides clearPendingTemplateOpen helper and skipTemplatePickerStep setting.
GPU VRAM detection & formatting
src/main/lib/gpu.ts, src/renderer/src/lib/formatting.ts
Adds optional vramBytes to GPU info with platform probes (nvidia-smi, os.totalmem, systeminformation) and a coarse bytes formatter for UI metadata.
Template JSON resolution
src/main/sources/standalone/templateModels.ts
Loads template JSON from local packages or GitHub, extracts models from top-level and nested nodes, validates allowlisted URLs and extensions, normalizes filenames, and deduplicates downloads.
Download core utilities
src/main/sources/standalone/templateDownloadCore.ts
Pure helpers: withRetry, runPool, truncateForMaxPath, summarizeTemplateState, templateStateToTrayEntries, formatTemplateSubStatus, and disk-space error handling.
Background download task & tray mirroring
src/main/sources/standalone/templateDownloadTask.ts, src/main/lib/comfyDownloadManager.ts
Per-install task state, start/abort/skip APIs, runTask pipeline (resolve, disk pre-check, bounded downloads with retries, progress/throttled logs), 500ms tray mirroring, and mirrored-row setters in comfyDownloadManager.
Gate behavior and tests
src/main/sources/standalone/templateDownloadGate.test.ts, src/main/sources/standalone/templateDownloadCore.test.ts
Tests for awaitTemplateDownloadSettled outcomes, core helper behaviors, path truncation, retry semantics, tray mapping, and substatus formatting.
Install handler & IPC wiring
src/main/lib/ipc/registerInstallationHandlers.ts, src/main/lib/ipc/registerLogsHandlers.ts, src/preload/api.ts
Starts fire-and-forget template downloads during install when selected, streams output to renderer and logs, aborts on install failure, registers skip-template-download and logs-snapshot IPC handlers, and bridges logsSnapshot and skipTemplateDownload to window.api.
Launch gating and attach/quit lifecycle
src/main/lib/launchPhases.ts, src/main/lib/ipc/sessionActions/launch.ts, src/main/host/attach.ts, src/main/index.ts
Adds synthetic template-models phase, readers that emit progress only after server reachable, waits for template settlement (3→2→1 on error), first-launch template deeplink open with pending flag clear, aborts task on install cleanup, and blocks quit with a synchronous warning when downloads are active.
Renderer picker & install wizard flow
src/renderer/src/components/TemplatePickerStep.vue, src/renderer/src/views/InstallWizardModal.vue, src/renderer/src/lib/installHelpers.ts
New TemplatePicker SFC and two-step wizard (Configure → Template → Install), disk hard-block calculation (model + 10% headroom), VRAM warnings, keyboard navigation, thumbnail fallback, pre-install disk check, and opt-out persistence.
Progress UI, errors, and skip CTA
src/renderer/src/lib/progressViewModel.ts, src/renderer/src/stores/progressStore.ts, src/renderer/src/components/BrandProgressView.vue, src/renderer/src/views/ProgressModal.vue
Adds phase-level error flag in progress data and op.phaseErrors tracking, seeds terminalOutput via logsSnapshot, ProgressStepVM.isError for active-row styling, BrandProgressView renders error icon, and ProgressModal exposes “Skip model download” action that calls skipTemplateDownload.
IPC types and settings
src/types/ipc.ts, src/main/settings.ts
Adds ProgressData.error, GPUInfo.vramBytes, ElectronApi.logsSnapshot and skipTemplateDownload signatures, and KnownSettings.skipTemplatePickerStep + SETTINGS_SCHEMA entry.
i18n and docs
locales/en.json, locales/zh.json, docs/starter-templates-handoff.md
Adds English/Chinese strings for picker, download progress, quit prompt, disk-space block, skip actions, lifecycle phase label, and a detailed engineering handoff document describing architecture, flows, tests, and manual validation scenarios.
  • Suggested reviewers:
    • deepme987 (for the deep-dive into downloads — no rabbit holes, I promise!)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

@coderabbitai coderabbitai Bot requested a review from deepme987 June 12, 2026 18:04

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

🤖 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 `@docs/starter-templates-handoff.md`:
- Around line 123-128: Remove the brittle numeric test-line counts from the
table and replace them with descriptive scope/category text; e.g., for
templateDownloadCore.test.ts mention covered areas like "core utilities: runPool
concurrency/abort/isolation, withRetry budget+fatal-cancel, truncateForMaxPath,
templateStateToTrayEntries, formatTemplateSubStatus, describeDownloadFailure
(401/403 vs generic)"; for templateDownloadGate.test.ts mention "launch-gate
behaviors: awaitTemplateDownloadSettled resolves correct reasons
(done/error/cancelled/skipped/aborted/absent) and skip-flag clearing"; for
index.test.ts note "skip-path build: buildInstallation with template none vs
real pick"; for TemplatePickerStep.test.ts list UI checks including disk-block
alert and VRAM warning; for installHelpers.test.ts list
templateDiskRequiredBytes and isTemplateDiskBlocked; ensure all filenames
(templateDownloadCore.test.ts, templateDownloadGate.test.ts, index.test.ts,
TemplatePickerStep.test.ts, installHelpers.test.ts) and key symbols
(describeDownloadFailure, awaitTemplateDownloadSettled, buildInstallation,
templateDiskRequiredBytes, isTemplateDiskBlocked) are referenced so readers can
find tests without fixed line counts.

In `@src/main/lib/comfyDownloadManager.ts`:
- Around line 155-174: setTemplateTrayMirror and clearTemplateTrayMirror operate
on the global templateTrayMirror/createdAtByUrl maps which allows concurrent
installs to clobber each other; make the tray mirror install-scoped by adding an
install identifier (e.g., installId) parameter to setTemplateTrayMirror and
clearTemplateTrayMirror and change storage to a per-install map (e.g.,
templateTrayMirrorByInstall: Map<installId, Map<url, DownloadProgress>> and
createdAtByInstall: Map<installId, Map<url, number>) so setTemplateTrayMirror
only replaces entries for that installId and clearTemplateTrayMirror only
removes that install’s entries; ensure createdAt lookups/sets use the
install-scoped createdAt map and keep/emits
downloadEvents.emit('tray-state-changed') but include or derive the installId as
needed so other installs aren’t affected.

In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 379-425: The polling loop started in the IIFE uses an
AbortController named abort and only stops when abort.signal.aborted is true,
but some early return/exit paths don't call abort.abort(), leaving the 500ms
timer spinning; update every launch exit path (including the other return sites
around the referenced ranges) to call abort.abort() before returning so the
tick/while loop (and its Promise timer/onAbort handlers) deterministically stop;
locate symbols like the IIFE that defines tick, the while
(!abort.signal.aborted) loop, and the abort AbortController and ensure each code
path that currently returns or exits invokes abort.abort() (and keep existing
cleanup of listeners/timers intact).

In `@src/main/sources/standalone/index.test.ts`:
- Around line 127-130: The test hardcodes the sentinel string 'none' when
building the template; replace that literal with the shared constant
NO_TEMPLATE_VALUE to avoid drift—use template(NO_TEMPLATE_VALUE) in the test
that calls standalone.buildInstallation and ensure NO_TEMPLATE_VALUE is imported
from its module (or referenced from the same source as production code) so the
test and production contract share the same sentinel value.

In `@src/main/sources/standalone/index.ts`:
- Around line 189-191: The bundledTemplate value is taken directly from
selections.bundledTemplate?.value into tplValue and persisted as
bundledTemplateId without validation, allowing stale/forged IDs; add a guard
that verifies tplValue is a valid known template ID before assigning
bundledTemplateId (e.g., check against the application's template registry/list
or a isValidTemplateId util) and only set bundledTemplateId when tplValue !==
NO_TEMPLATE_VALUE and passes that validation, otherwise set bundledTemplateId to
undefined and optionally log or ignore the invalid value.

In `@src/main/sources/standalone/templateModels.ts`:
- Around line 94-95: The URL validation currently allows both parsed.protocol
=== 'http:' and 'https:' which permits insecure model downloads; update the
protocol check in the validation logic in templateModels.ts so that only
parsed.protocol === 'https:' is accepted (reject 'http:'), leaving the rest of
the hostname handling (host = parsed.hostname.toLowerCase()) unchanged and
returning false for any non-HTTPS URLs.
- Around line 158-166: Reject template-provided path traversal by
validating/sanitizing m.name and m.directory before using them: ensure m.name
and m.directory are strings and do not contain ".." or any path separators ("/"
or "\\" or path.sep) and use a safe basename/normalization for filename (replace
current stripQueryParams usage with or follow by path.basename or explicit
reject on separators) and validate directory contains only a single allowed
folder name (no separators); if validation fails, continue. Apply this check
where m.name/m.directory are read (around variables filename, key, seen, result)
so malicious names cannot escape the intended model folder when building
destination paths.

In `@src/renderer/src/components/BrandProgressView.test.ts`:
- Around line 6-12: The test helper step now accepts isError but no test covers
the error rendering path; add a regression test in BrandProgressView.test.ts
that creates a step via step(phase, 'failed' | appropriateStatus, detail, true)
and renders BrandProgressView with that step, then assert the DOM shows the
error indicator (e.g. presence of the error icon element or an error CSS class
like the component's error modifier) to verify the isError branch in
ProgressStepVM is rendered. Include assertions for both the icon element and the
error class to be safe.

In `@src/renderer/src/components/TemplatePickerStep.test.ts`:
- Around line 115-121: The test "never blocks the model-free none-equivalent (no
recommended bytes)" wrongly selects IMAGE.value; update the test to select the
none-sentinel instead so it exercises the none-equivalent path. In the test that
calls mountPicker, replace selectedValue: IMAGE.value with the appropriate none
constant (e.g. NONE.value or the repo's none sentinel) so the assertion on
.tps__alert--error matches the intended branch; adjust imports if needed to
reference the NONE symbol used by the TemplatePickerStep tests.

In `@src/renderer/src/stores/progressStore.test.ts`:
- Line 35: Add a focused unit test in progressStore.test.ts that stubs
logsSnapshot with vi.fn().mockResolvedValue('<SNAP>') and then triggers the
codepath that seeds logs for launch-chain operations (exercise the progress
store initialization or the specific method that handles launch-chain start),
asserting that logsSnapshot was called and that the resulting store logs array
begins with/prepends '<SNAP>' (or the mocked snapshot text) before other log
entries; reference the logsSnapshot mock and the progress store
initialization/launch-chain handler to locate where to trigger the behavior.

In `@src/renderer/src/views/InstallWizardModal.vue`:
- Around line 330-341: Reset the ephemeral state (pickerEnabled,
hasLocalInstall, detectedVramBytes) to their default values immediately when the
modal opens, capture a local generation token/counter for this open, then in
each async callback (the promises from window.api.getSetting,
window.api.getInstallationsSummary, window.api.detectGPU) check that the token
still matches before assigning to pickerEnabled.value, hasLocalInstall.value, or
detectedVramBytes.value; if the generation has changed (modal reopened/closed),
ignore the result. This prevents racey stale assignments across modal reopens
while keeping the same async calls and identifiers.

In `@src/renderer/src/views/ProgressModal.vue`:
- Around line 60-68: templateSkipped is sticky across operations and
canSkipTemplateDownload doesn’t check whether the current operation is finished;
update logic so templateSkipped resets when currentOp changes and add an
op.finished guard in canSkipTemplateDownload. Specifically, tie the
templateSkipped state to the current operation (e.g., reset templateSkipped when
currentOp.value changes) and change the computed canSkipTemplateDownload to
return false if !op or op.finished, in addition to the existing checks (keep
using currentOp, templateSkipped, globalProgress, and op.phaseErrors); apply the
same fix to the similar logic referenced around the other occurrence (lines
~276-279).
- Around line 70-79: The handler handleSkipTemplateDownload currently sets
templateSkipped.value = true before awaiting window.api.skipTemplateDownload,
which hides retry opportunities; move the mutation so templateSkipped.value is
only set after the await succeeds (or revert it in the catch), and keep the
emitTelemetryAction call as-is (or call it on success) so that the skip is
marked consumed only when window.api.skipTemplateDownload(id) completes
successfully; update references in handleSkipTemplateDownload (displayId,
templateSkipped, window.api.skipTemplateDownload, emitTelemetryAction)
accordingly.

In `@src/types/ipc.ts`:
- Around line 487-490: Update the GPUInfo.vramBytes JSDoc to reflect that the
main GPU detector now attempts to determine VRAM for AMD and Intel as well (not
just NVIDIA/Apple Silicon); remove the stale claim that it is undefined for
AMD/Intel/unknown and instead state that vramBytes is optional and, when present
for any vendor, the template picker will warn if it is below a template's
recommended VRAM (and not warn when undefined). Refer to the GPUInfo type and
the vramBytes property when making this change.
🪄 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

Run ID: 77cb7369-8871-44f2-be3f-9f506c5902b3

📥 Commits

Reviewing files that changed from the base of the PR and between e4db94d and 5824812.

📒 Files selected for processing (41)
  • docs/starter-templates-handoff.md
  • locales/en.json
  • locales/zh.json
  • src/main/host/attach.ts
  • src/main/index.ts
  • src/main/installations.ts
  • src/main/lib/comfyDownloadManager.ts
  • src/main/lib/gpu.ts
  • src/main/lib/ipc/registerInstallationHandlers.ts
  • src/main/lib/ipc/registerLogsHandlers.ts
  • src/main/lib/ipc/sessionActions/launch.ts
  • src/main/lib/launchPhases.ts
  • src/main/settings.ts
  • src/main/sources/standalone/bundledTemplates.test.ts
  • src/main/sources/standalone/bundledTemplates.ts
  • src/main/sources/standalone/index.test.ts
  • src/main/sources/standalone/index.ts
  • src/main/sources/standalone/install.ts
  • src/main/sources/standalone/templateDownloadCore.test.ts
  • src/main/sources/standalone/templateDownloadCore.ts
  • src/main/sources/standalone/templateDownloadGate.test.ts
  • src/main/sources/standalone/templateDownloadTask.ts
  • src/main/sources/standalone/templateModels.ts
  • src/preload/api.ts
  • src/renderer/public/images/templates/3d_hunyuan3d_image_to_model.webp
  • src/renderer/public/images/templates/audio_stable_audio_example.webp
  • src/renderer/public/images/templates/flux_schnell.webp
  • src/renderer/public/images/templates/text_to_video_wan.webp
  • src/renderer/src/components/BrandProgressView.test.ts
  • src/renderer/src/components/BrandProgressView.vue
  • src/renderer/src/components/TemplatePickerStep.test.ts
  • src/renderer/src/components/TemplatePickerStep.vue
  • src/renderer/src/lib/formatting.ts
  • src/renderer/src/lib/installHelpers.test.ts
  • src/renderer/src/lib/installHelpers.ts
  • src/renderer/src/lib/progressViewModel.ts
  • src/renderer/src/stores/progressStore.test.ts
  • src/renderer/src/stores/progressStore.ts
  • src/renderer/src/views/InstallWizardModal.vue
  • src/renderer/src/views/ProgressModal.vue
  • src/types/ipc.ts

Comment thread docs/starter-templates-handoff.md Outdated
Comment thread src/main/lib/comfyDownloadManager.ts Outdated
Comment thread src/main/lib/ipc/sessionActions/launch.ts
Comment thread src/main/sources/standalone/index.test.ts Outdated
Comment thread src/main/sources/standalone/index.ts Outdated
Comment thread src/renderer/src/stores/progressStore.test.ts
Comment thread src/renderer/src/views/InstallWizardModal.vue Outdated
Comment thread src/renderer/src/views/ProgressModal.vue
Comment thread src/renderer/src/views/ProgressModal.vue
Comment thread src/types/ipc.ts Outdated
…ests

- surface the silent disk-probe failure in the task log instead of
  swallowing it
- launch gate: emit the failure immediately when the download already
  errored before server-up (closes the ≤500ms blind window)
- extract pure buildTemplateDeeplink; attach.ts uses it + logs a failed
  clearPendingTemplateOpen instead of dropping the error
- prune stale template URLs from the tray-mirror createdAt map (no leak)
- reset the wizard to the Configure step when the source changes
- strip history-narrating comments; convert kept intent to JSDoc
- tests: deeplink round-trip, clearPendingTemplateOpen (incl. legacy
  no-template-fields record), picker Enter/Space native activation

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
docs/starter-templates-handoff.md (1)

126-132: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Remove brittle numeric line counts from the test coverage table — descriptions are already there.

The numeric counts (29, 8, 3, 14, 6) will drift when tests are refactored, reformatted, or new ones are added. Since the "What it pins" column already crisply documents each test area's scope (e.g., "runPool concurrency/abort/isolation, withRetry budget + fatal-cancel" for core utilities), strip the counts and let the descriptions stand alone — far less likely to line-count itself into a corner. 🧌

✏️ Proposed fix: Remove numeric line counts from test-coverage filenames
- | Download core       | [templateDownloadCore.test.ts](../src/main/sources/standalone/templateDownloadCore.test.ts) (29) | cumulative math, `runPool` concurrency/abort/isolation, `withRetry` budget + fatal-cancel, `truncateForMaxPath`, `templateStateToTrayEntries`, `formatTemplateSubStatus`, `**describeDownloadFailure`** (401/403 vs generic, no-false-match) |
- | **Launch gate**     | [templateDownloadGate.test.ts](../src/main/sources/standalone/templateDownloadGate.test.ts) (8)  | `awaitTemplateDownloadSettled` resolves the right reason for done / error (incl. disk-space pre-flight) / cancelled / skipped / aborted / absent; skip flag cleared on settle (no stale pre-skip)                                            |
- | **Skip-path build** | [index.test.ts](../src/main/sources/standalone/index.test.ts) → `starter template` (3)           | `buildInstallation` with template = `none` (or absent) builds **no** `downloadTemplateModels`; a real pick sets `bundledTemplateId` + `pendingTemplateOpen` + `downloadTemplateModels: true`                                                 |
- | **Picker UI**       | [TemplatePickerStep.test.ts](../src/renderer/src/components/TemplatePickerStep.test.ts) (14)     | rows render (none excluded), Recommended on first only, aria-checked, select emit, ArrowDown nav, meta line, **disk-block alert** (loading/below/above/model-free), **VRAM warning** (below/meets/undetected), thumbnail `@error` → glyph    |
- | Disk helpers        | [installHelpers.test.ts](../src/renderer/src/lib/installHelpers.test.ts) (6)                     | `templateDiskRequiredBytes` headroom, `isTemplateDiskBlocked` (unknown/model-free/below/above)                                                                                                                                               |
+ | Download core       | [templateDownloadCore.test.ts](../src/main/sources/standalone/templateDownloadCore.test.ts) | cumulative math, `runPool` concurrency/abort/isolation, `withRetry` budget + fatal-cancel, `truncateForMaxPath`, `templateStateToTrayEntries`, `formatTemplateSubStatus`, `**describeDownloadFailure`** (401/403 vs generic, no-false-match) |
+ | **Launch gate**     | [templateDownloadGate.test.ts](../src/main/sources/standalone/templateDownloadGate.test.ts) | `awaitTemplateDownloadSettled` resolves the right reason for done / error (incl. disk-space pre-flight) / cancelled / skipped / aborted / absent; skip flag cleared on settle (no stale pre-skip)                                            |
+ | **Skip-path build** | [index.test.ts](../src/main/sources/standalone/index.test.ts) → `starter template`          | `buildInstallation` with template = `none` (or absent) builds **no** `downloadTemplateModels`; a real pick sets `bundledTemplateId` + `pendingTemplateOpen` + `downloadTemplateModels: true`                                                 |
+ | **Picker UI**       | [TemplatePickerStep.test.ts](../src/renderer/src/components/TemplatePickerStep.test.ts) | rows render (none excluded), Recommended on first only, aria-checked, select emit, ArrowDown nav, meta line, **disk-block alert** (loading/below/above/model-free), **VRAM warning** (below/meets/undetected), thumbnail `@error` → glyph    |
+ | Disk helpers        | [installHelpers.test.ts](../src/renderer/src/lib/installHelpers.test.ts) | `templateDiskRequiredBytes` headroom, `isTemplateDiskBlocked` (unknown/model-free/below/above)                                                                                                                                               |
🤖 Prompt for 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.

In `@docs/starter-templates-handoff.md` around lines 126 - 132, The table in
docs/starter-templates-handoff.md contains brittle numeric line counts next to
test file links (e.g., the "(29)" after templateDownloadCore.test.ts, "(8)"
after templateDownloadGate.test.ts, "(3)" after index.test.ts, "(14)" after
TemplatePickerStep.test.ts, "(6)" after installHelpers.test.ts); remove these
parenthesized numeric counts from each "Where" cell so the rows read only the
file link and keep the existing "What it pins" descriptions unchanged. Ensure
links like templateDownloadCore.test.ts, templateDownloadGate.test.ts,
index.test.ts, TemplatePickerStep.test.ts, and installHelpers.test.ts remain
intact and only the trailing numeric annotations are deleted.

Source: Coding guidelines

🤖 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/installations.test.ts`:
- Around line 623-631: The test creates an InstallationRecord using a double
cast through Partial which hides type mismatches; remove the unnecessary cast so
the call to installations.add(...) uses a value typed directly as
InstallationRecord (or adjust the object to satisfy InstallationRecord) instead
of casting with "as Partial<InstallationRecord> as InstallationRecord" — update
the object literal passed to installations.add to conform to InstallationRecord
and drop the double-cast to let the compiler catch contract slips.

---

Duplicate comments:
In `@docs/starter-templates-handoff.md`:
- Around line 126-132: The table in docs/starter-templates-handoff.md contains
brittle numeric line counts next to test file links (e.g., the "(29)" after
templateDownloadCore.test.ts, "(8)" after templateDownloadGate.test.ts, "(3)"
after index.test.ts, "(14)" after TemplatePickerStep.test.ts, "(6)" after
installHelpers.test.ts); remove these parenthesized numeric counts from each
"Where" cell so the rows read only the file link and keep the existing "What it
pins" descriptions unchanged. Ensure links like templateDownloadCore.test.ts,
templateDownloadGate.test.ts, index.test.ts, TemplatePickerStep.test.ts, and
installHelpers.test.ts remain intact and only the trailing numeric annotations
are deleted.
🪄 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

Run ID: 7e8bbdb0-5426-4a5b-8864-7e4b195c7661

📥 Commits

Reviewing files that changed from the base of the PR and between 5824812 and 5562274.

📒 Files selected for processing (12)
  • docs/starter-templates-handoff.md
  • locales/en.json
  • locales/zh.json
  • src/main/host/attach.ts
  • src/main/installations.test.ts
  • src/main/lib/comfyDownloadManager.ts
  • src/main/lib/ipc/sessionActions/launch.ts
  • src/main/sources/standalone/bundledTemplates.test.ts
  • src/main/sources/standalone/bundledTemplates.ts
  • src/main/sources/standalone/templateDownloadTask.ts
  • src/renderer/src/components/TemplatePickerStep.test.ts
  • src/renderer/src/views/InstallWizardModal.vue
💤 Files with no reviewable changes (2)
  • locales/zh.json
  • locales/en.json

Comment thread src/main/installations.test.ts Outdated
… add funnel telemetry

Wraps the new install-wizard template step in the
`desktop-starter-templates-picker` A/B flag (control: legacy single-screen
Configure, treatment: picker step), reuses the cache-first
`telemetryGetExperimentFlag` plumbing used by FirstUseTakeover, and pins the
variant as a person property so any downstream activation event can be sliced
by arm.

Adds `comfy.desktop.template.picker_shown / .selected / .install_confirmed /
.skipped` so the picker funnel is measurable end-to-end against the
existing `comfy.desktop.template.download.skipped` event.

Defaults to control on flag miss / network failure so the legacy flow ships
when the experiment can't be resolved.
@deepme987

Copy link
Copy Markdown
Collaborator

Wired the starter-template picker into a PostHog experiment + funnel telemetry in bf7024eb so we can ship at 50% and read activation-funnel deltas.

Experiment

  • Flag key: desktop-starter-templates-picker
  • Variants: control (legacy single-screen Configure) vs treatment (picker step shown).
  • Lookup uses the cache-first telemetryGetExperimentFlag plumbing (the same path FirstUseTakeover uses for desktop-fork-default). Boot-time fetch lands on disk; the running modal reads from cache, so the wizard never blocks on the network.
  • Exposure event (comfy.desktop.experiment.exposed) is fired once per session main-side.
  • Variant is also pinned as a person property (experiment_desktop-starter-templates-picker = control|treatment) so any downstream activation event (boot success, first workflow exec) can be sliced by arm without joining back through exposure.
  • Defaults to control on flag miss / network failure → legacy flow ships when the experiment can't be resolved.

The treatment arm still respects the user's skipTemplatePickerStep opt-out and the existing standalone-source gate, so an opted-out returning user in treatment still won't see the step.

Funnel events (all comfy.desktop.template.*)

Event When Props
picker_shown step transitions to template template_count, has_local_install, default_template_id, variant
selected user picks a real (non-None) template, dedup'd on value change template_id, size_bucket, variant
install_confirmed "Install" clicked from picker template_id, size_bucket, has_models, dont_show_again, variant
skipped "Skip & Install" clicked had_template_selected, candidate_template_id, dont_show_again, variant

These compose with the existing comfy.desktop.template.download.skipped (ProgressModal) so we have shown → selected → install_confirmed → download_skipped/completed → first_workflow_exec.

PostHog setup (before turning the experiment on)

  1. Create a multivariate feature flag in Prod Comfy (Homepage, Cloud, Hub) (project 204330):
    • Key: desktop-starter-templates-picker
    • Variants: control (50%), treatment (50%)
    • Rollout condition: app_name = comfyui-desktop (or id_class is set) so it only targets desktop installs, not website / cloud users on the same project.
  2. Create the experiment in PostHog Experiments using that flag, with primary metric = comfy.desktop.template.install_confirmed (north-star: pick + install a starter template) and a secondary metric on comfy.desktop.comfyui.boot_success / first workflow execution.
  3. The renderer reads the flag at modal open; cached value applies on the next install, refreshed value lands on disk for the boot after that, per the experiments.ts contract (one-boot-of-lag is intentional).

Scope still open (not in this commit)

  • Main-side download lifecycle (template.download_started / .download_completed / .download_failed) and template.auto_opened from attach.ts. Happy to add in a follow-up; kept this commit focused on the install-time funnel so the experiment can ship.
  • "Don't show again" toggle event is intentionally folded into install_confirmed / skipped rather than a standalone event (the value is already on both terminal events).

Pre-commit ran typecheck + lint + the 15 TemplatePickerStep unit tests all green.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/renderer/src/views/InstallWizardModal.vue (2)

353-353: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Guard the awaited open() path with the same generation token.

loadGeneration is incremented, but this awaited branch never checks it before mutating wizard state or calling selectSourceCard(). If an older open() resumes after a newer one starts, it can stomp instPath, hardwareValidation, and currentSource on the live modal — stale state with extra weight, not a spell I’d celebrate.

Suggested fix
 async function open(opts: OpenOpts = {}): Promise<void> {
   loadGeneration++
+  const gen = loadGeneration
   instName.value = ''
@@
   try {
     const [, installDir] = await Promise.all([loadSources(), installDirPromise])
+    if (gen !== loadGeneration) return
 
     defaultInstPath.value = installDir ?? ''
     instPath.value = defaultInstPath.value
 
     // Pre-select Standalone (the recommended method); other sources are reachable via the Advanced method-picker chips.
     hardwareValidation = await window.api.validateHardware()
+    if (gen !== loadGeneration) return
     const standalone = sources.value.find((s) => s.id === 'standalone')
     if (standalone && hardwareValidation.supported) {
       await selectSourceCard(standalone)
     } else if (standalone) {
       detectedGpu.value = hardwareValidation.error || t('newInstall.noGpuDetected')
     }
   } finally {
-    initializing.value = false
+    if (gen === loadGeneration) {
+      initializing.value = false
+    }
   }
 }

Also applies to: 417-433

🤖 Prompt for 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.

In `@src/renderer/src/views/InstallWizardModal.vue` at line 353, The awaitable
open() flow increments loadGeneration but does not verify the token before
mutating wizard state; update open() to capture a local generation token (e.g.,
const myGen = loadGeneration) immediately after incrementing and before any
awaits, then after each await check if myGen === loadGeneration and return early
if not; apply the same guard before mutating instPath, hardwareValidation,
currentSource and before calling selectSourceCard() (also add the same
generation-checks around the secondary awaited branch that covers the logic
currently around the selectSourceCard() / state updates referenced in the later
block).

375-390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reset the experiment arm on each open().

starterTemplatesVariant keeps its previous value across modal reopens, so after one treatment run a later open can still show the picker before telemetryGetExperimentFlag() resolves. That breaks the documented fail-closed-to-control behavior; a tiny imp today, a skewed funnel tomorrow.

Suggested fix
 async function open(opts: OpenOpts = {}): Promise<void> {
   loadGeneration++
   instName.value = ''
   cameFromLocalBranch.value = opts.cameFromLocalBranch === true
@@
   initializing.value = true
   step.value = 'configure'
   dontShowTemplatePicker.value = false
+  starterTemplatesVariant.value = 'control'
 
   // Resolve picker gating + VRAM in the background — needed only by the time the
🤖 Prompt for 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.

In `@src/renderer/src/views/InstallWizardModal.vue` around lines 375 - 390, Reset
the experiment arm state at the start of the modal open path so previous runs
don't leak into subsequent opens: inside the open() method clear or set
starterTemplatesVariant to null (or the sentinel that indicates "unresolved")
and also ensure any dependent UI gating (e.g., pickerEnabled or
dontShowTemplatePicker.value) relies on the resolved value from
telemetryGetExperimentFlag() rather than a stale starterTemplatesVariant; then
let telemetryGetExperimentFlag(STARTER_TEMPLATES_EXPERIMENT_KEY) set
starterTemplatesVariant when it resolves.
🤖 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.

Outside diff comments:
In `@src/renderer/src/views/InstallWizardModal.vue`:
- Line 353: The awaitable open() flow increments loadGeneration but does not
verify the token before mutating wizard state; update open() to capture a local
generation token (e.g., const myGen = loadGeneration) immediately after
incrementing and before any awaits, then after each await check if myGen ===
loadGeneration and return early if not; apply the same guard before mutating
instPath, hardwareValidation, currentSource and before calling
selectSourceCard() (also add the same generation-checks around the secondary
awaited branch that covers the logic currently around the selectSourceCard() /
state updates referenced in the later block).
- Around line 375-390: Reset the experiment arm state at the start of the modal
open path so previous runs don't leak into subsequent opens: inside the open()
method clear or set starterTemplatesVariant to null (or the sentinel that
indicates "unresolved") and also ensure any dependent UI gating (e.g.,
pickerEnabled or dontShowTemplatePicker.value) relies on the resolved value from
telemetryGetExperimentFlag() rather than a stale starterTemplatesVariant; then
let telemetryGetExperimentFlag(STARTER_TEMPLATES_EXPERIMENT_KEY) set
starterTemplatesVariant when it resolves.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a6ff7c04-1880-4efe-9a16-31331f88f050

📥 Commits

Reviewing files that changed from the base of the PR and between 5562274 and bf7024e.

📒 Files selected for processing (1)
  • src/renderer/src/views/InstallWizardModal.vue

@deepme987

Copy link
Copy Markdown
Collaborator

PostHog side is fully wired (project 204330 / Prod Comfy). Nothing left for the team to click before merge — flag is in draft so it doesn't fire until the experiment is launched from the UI.

Links

Metrics on the experiment

Slot Metric Type
Primary exposed → comfy.desktop.execution.first_completed funnel
Secondary picker_shownselectedinstall_confirmed funnel
Secondary install.method.selectedcomfyui.boot_started funnel
Secondary Workflow executions per user (execution.completed, total) mean
Secondary Picker skip rate (template.skipped, total) mean — goal: decrease
Secondary boot_startedfirst_completed funnel

Exposure event is set to comfy.desktop.experiment.exposed filtered to experiment_key = desktop-starter-templates-picker, so the denominator is "users who opened the install wizard while enrolled," not "every desktop boot." Tighter exposure window than $feature_flag_called.

Dashboard tiles

  1. Activation funnel — exposed → first execution success, broken down by variant (primary north-star)
  2. First-execution conversion rate — same numerator/denominator, rendered as a side-by-side bar with B/A*100
  3. Picker funnel — shown → selected → install_confirmed (treatment-only; control fires zero by construction)
  4. Picker engagement timeline — shown / install / skipped daily DAU
  5. Template selection distribution — install_confirmed broken down by template_id, bar chart
  6. Install completion funnel — method.selected (standalone) → boot_started → first_completed, by variant
  7. Model-download skip ratetemplate.download.skipped / template.install_confirmed * 100
  8. Workflow executions per user — weekly_active broken down by variant (retention check)
  9. Exposure population (sanity) — daily exposed DAU broken down by variant, to catch flag-targeting drift

Test accounts filtered out everywhere.

Launch flow when ready

  1. Merge PR Feat/starter templates #1084 → events ship.
  2. Wait for comfy.desktop.experiment.exposed to start landing in PostHog (open the wizard once on a build to verify).
  3. Hit Launch on the experiment page (https://us.posthog.com/project/204330/experiments/376772).
  4. The dashboard starts populating as users come through the wizard.

The flag stays draft until launched, so no risk of accidentally serving variants before merge.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
locales/zh.json (1)

10-10: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix the broken migration CTA copy.

migrateBannerAction has a stray 否w, so the Chinese banner button will render malformed text. Tiny typo, big oops.

🛠️ Proposed fix
-    "migrateBannerAction": "迁移 否w",
+    "migrateBannerAction": "立即迁移",
🤖 Prompt for 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.

In `@locales/zh.json` at line 10, The translation value for the key
migrateBannerAction contains a stray "否w" and should be corrected; locate the
migrateBannerAction entry and replace the malformed string "迁移 否w" with the
intended Chinese CTA (e.g., "迁移") so the banner button renders properly.
🤖 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.

Outside diff comments:
In `@locales/zh.json`:
- Line 10: The translation value for the key migrateBannerAction contains a
stray "否w" and should be corrected; locate the migrateBannerAction entry and
replace the malformed string "迁移 否w" with the intended Chinese CTA (e.g., "迁移")
so the banner button renders properly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9295d30e-c2e4-40e2-9f36-b54fc044afb5

📥 Commits

Reviewing files that changed from the base of the PR and between bf7024e and 32d400e.

📒 Files selected for processing (2)
  • locales/en.json
  • locales/zh.json

Security:
- reject non-HTTPS model URLs and sanitize template-provided model
  name/directory against path traversal (new templateModels.test.ts)

Correctness:
- scope the tray mirror per install so concurrent installs don't clobber
  each other's rows
- abort the template-models launch reader on success/failure so its 500ms
  timer can't leak after a skip
- validate the chosen template id against the known set before persisting
- generation-guard the wizard's open() async setters against reopen races
- ProgressModal: gate skip on op.finished, reset per op, and mark consumed
  only after the skip IPC succeeds

Docs/tests:
- correct the stale GPUInfo.vramBytes JSDoc (AMD/Intel now probed)
- use NO_TEMPLATE_VALUE, fix the none-equivalent picker test, drop a
  double cast, add isError-render and logsSnapshot-seeding coverage
Was a corrupted string ('迁移 否w'); now reads '立即迁移'.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
locales/zh.json (1)

779-780: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep standalone.label in English.

This key is translated to “独立版”, but the locale note says product names should remain English, so the UI will drift from the shared naming contract. As per coding guidelines, standalone.label and portable.label are product names and should remain English.

🔧 Suggested fix
-    "label": "独立版",
+    "label": "Standalone",
🤖 Prompt for 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.

In `@locales/zh.json` around lines 779 - 780, The translation for the key
standalone.label is incorrectly localized; per the locale note product names
must stay in English, so update the value of standalone.label from "独立版" to the
English product name (e.g., "Standalone") so the UI matches the shared naming
contract; verify portable.label likewise remains English if present.

Source: Coding guidelines

src/renderer/src/views/InstallWizardModal.vue (1)

379-420: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset starterTemplatesVariant at modal open to keep fail-closed behavior truly closed.

Line 379 resets picker state but not the experiment variant, so control can stroll into treatment on a later flag-miss open due to stale state.

Suggested fix
   step.value = 'configure'
   dontShowTemplatePicker.value = false
+  starterTemplatesVariant.value = 'control'
   // Reset to defaults synchronously so a slow prior-open response can't leave
   // stale gating/VRAM on this open; the guarded callbacks below then refill them.
   pickerEnabled.value = true
🤖 Prompt for 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.

In `@src/renderer/src/views/InstallWizardModal.vue` around lines 379 - 420, Reset
the experiment variant when the modal opens by setting
starterTemplatesVariant.value = 'control' alongside the other synchronous resets
(near pickerEnabled.value, hasLocalInstall.value, detectedVramBytes.value) so a
stale treatment value can't persist across opens; leave the later
telemetryGetExperimentFlag promise to overwrite this when a fresh value arrives
and keep the existing generation guard (gen !== loadGeneration).
🤖 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/comfyDownloadManager.ts`:
- Around line 212-220: The mirror entries produced from
templateTrayMirrorByInstall are added to recent but never removed, so terminal
mirrored rows linger; update the cleanup paths to remove those mirrored entries
when a user dismisses or clears finished downloads by (1) modifying
dismissRecentDownload() to also remove any matching entries from
templateTrayMirrorByInstall (lookup by the same id/key used when mirroring) and
(2) modifying clearFinishedDownloads() to iterate templateTrayMirrorByInstall
and purge all terminal entries (or clear entire buckets that contain only
finished items) in addition to clearing recentDownloads; use the existing helper
isTerminalStatus() and the same entry identity used when pushing into recent to
find and delete the mirrored rows so the tray no longer retains stale template
mirror entries.

---

Outside diff comments:
In `@locales/zh.json`:
- Around line 779-780: The translation for the key standalone.label is
incorrectly localized; per the locale note product names must stay in English,
so update the value of standalone.label from "独立版" to the English product name
(e.g., "Standalone") so the UI matches the shared naming contract; verify
portable.label likewise remains English if present.

In `@src/renderer/src/views/InstallWizardModal.vue`:
- Around line 379-420: Reset the experiment variant when the modal opens by
setting starterTemplatesVariant.value = 'control' alongside the other
synchronous resets (near pickerEnabled.value, hasLocalInstall.value,
detectedVramBytes.value) so a stale treatment value can't persist across opens;
leave the later telemetryGetExperimentFlag promise to overwrite this when a
fresh value arrives and keep the existing generation guard (gen !==
loadGeneration).
🪄 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

Run ID: 33ac29df-a5c4-4be5-affa-d2b79f07aa71

📥 Commits

Reviewing files that changed from the base of the PR and between 32d400e and 89756e5.

📒 Files selected for processing (15)
  • locales/zh.json
  • src/main/installations.test.ts
  • src/main/lib/comfyDownloadManager.ts
  • src/main/lib/ipc/sessionActions/launch.ts
  • src/main/sources/standalone/index.test.ts
  • src/main/sources/standalone/index.ts
  • src/main/sources/standalone/templateDownloadTask.ts
  • src/main/sources/standalone/templateModels.test.ts
  • src/main/sources/standalone/templateModels.ts
  • src/renderer/src/components/BrandProgressView.test.ts
  • src/renderer/src/components/TemplatePickerStep.test.ts
  • src/renderer/src/stores/progressStore.test.ts
  • src/renderer/src/views/InstallWizardModal.vue
  • src/renderer/src/views/ProgressModal.vue
  • src/types/ipc.ts

Comment thread src/main/lib/comfyDownloadManager.ts
Mirrored template-model rows live in templateTrayMirrorByInstall, not
recentDownloads, so dismissRecentDownload / clearFinishedDownloads left
finished ones lingering in the tray forever.

- dismissRecentDownload also drops a matching mirror row (pruning empty buckets)
- clearFinishedDownloads purges terminal mirror rows, keeping in-flight ones
- test: both cleanup paths drop finished mirror rows; a downloading row survives
…modal + auto-open tray

- getAllDownloads() now includes template-mirror rows so the All-Downloads
  modal lists them, matching the tray popup
- setTemplateTrayMirror broadcasts each row via model-download-progress so the
  modal tracks live progress like any real download
- auto-open the downloads tray ~2.5s after ComfyUI reveal when a template
  download is still running; wired into all three onLaunch reveal paths
- tests: getAllDownloads includes mirror rows + per-row broadcast fires
- skip the template step entirely when free disk can't fit even the
  cheapest model-bearing template (new minTemplateModelBytes feeds
  diskTooSmallForAnyTemplate → shouldShowPickerStep); fails open while
  disk space is still loading
- Install button stays clickable but reads disabled when blocked; clicking
  it shakes the disk-error alert instead of installing, mirroring the
  first-use consent nudge (nudgeDiskError exposed by the picker)
- tests: minTemplateModelBytes + the skip-gate threshold; nudgeDiskError
  shakes when blocked / no-ops when not
- docs: both behaviors in gating, decisions, edge cases, coverage, matrix
- fix the cross-ref label + stale #picker-ui anchor to #template-picker-ui
- rename the test-coverage row and matrix scenario labels to match
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.

3 participants