feat(first-run-tour): offer templates once the tour ends - #14144
Merged
Conversation
- add blank canvas (demo) and app mode (wip) tours - overlay with target highlight, landing card, step state handling, step card - v-coachmark directive updating registry for mount/unmount - frame settling watcher for animated targets (dialogs) - focus trap for the target plus the coachmark element - add telemetry for each step
- move tour trigger to linear controls section
- onboardingTours: use 'auto' placement for assets panel so the card follows the sidebar side - TourOverlay: clamp no-target card left to the viewport margin so it never goes off-screen - useCoachmarkTour: catch onPrimary action failures, surface a toast, and only advance on success - useCoachmarkTour: claim the single-instance guard synchronously so a duplicate mount stays inert - useFocusTrap: send Shift+Tab from outside the trap to the last item instead of skipping it - telemetry: drop dead run-button imports left over from the rebase (superseded by getRunButtonTelemetryProperties)
- Open the tour only via the help button or ?coach= param (no auto-open when entering app mode) - Drop the loadTemplate step tech and the demo card image — the tour runs on the user's existing app - Skip the assets-button step at tour start when the assets panel is already open; step indicator counts 4 instead of 5
- Open the app-mode tour when entering an app with linear controls visible (mode === 'app' && hasOutputs), including when the overlay mounts into one already (immediate watch) - Respect the seen-flag, so it won't reopen once completed or skipped; skip the empty/welcome state and arrange (builder) mode - Move test target DOM cleanup into afterEach so appended nodes are removed even if a test fails early
- Track the spotlight target with VueUse useElementBounding instead of a hand-rolled measure/RAF/ResizeObserver loop; keep a capture-phase scroll listener so it still follows targets inside scrollable panels - Fold the spotlight ring into the dim element (CSS outline + box-shadow), dropping the separate SVG; the idle pulse now animates outline-color - Remove the unused CoachStep `elevated` field and its plumbing
Reka's built-in Escape dismissal proved unreliable for this dialog in e2e (Skip, which sets open=false directly, works); close via the same model path on Escape so the welcome landing reliably dismisses.
Trimming the graph-mode anchors earlier removed the templates-button click that loadTemplate relied on to open the browser; open it via the Comfy.BrowseTemplates command instead.
Extract TourSpotlight, rendered only while a spotlight step is shown, so the geometry/scroll, focus-trap and click-to-advance listeners (plus z-index and the stall pulse) register for the active step rather than the whole graph-view session. useCoachmarkTour slims to the state machine; the registry-query helpers (targetMounted/waitForTarget) move to coachmarkRegistry and useCoachmarkTarget becomes geometry-only and self-measuring. Also from review feedback: - read the ?coach= force in setup, before the immediate auto-open watcher, so an already-populated app no longer drops a ?coach=any replay - harden isEntryPath against prototype keys (Object.hasOwn) - decouple each tour's auto-open condition into useTourTriggers, keeping the engine tour-agnostic
Conflict in LinearControls.vue: main refactored the run-button test id into a named constant (linearRunButtonTestId, same 'linear-run-button' value), while this branch added the v-coachmark anchor and a static test id to the same sections. Kept the coachmark directive and adopted main's :data-testid binding, dropping the now-redundant static id.
Raise useCoachmarkTour.ts to full line coverage and TourSpotlight.vue escape handling with behavioral tests.
- Replace the hand-rolled card-placement math (resolvePlacement /
cardCorner / clampCardPosition) and the manual target tracking (scroll
listener + useElementBounding + rAF settle loop) with @floating-ui/vue
(offset/flip/shift + autoUpdate); promote it from a transitive to a
direct dependency.
- Keep vertically-centred (leftCenter) cards on-screen with
shift({ crossAxis: true }), and centre the card when its target hasn't
laid out, so a step never renders off-screen or invisible.
- Make targetMounted/waitForTarget layout-aware (poll per frame) so a
deferred target that registers before it sizes resolves only once
measurable, matching what the spotlight actually displays.
- Shrink the spotlight glow (pad 8->4) so it no longer spills onto an
adjacent control the user might click.
- Label the spotlight dialog with aria-labelledby pointing at its heading
instead of a duplicated aria-label.
- Drop the unused isActive option from useFocusTrap.
- Add an e2e guard that walks every app-mode spotlight step and asserts
each card sits fully within the viewport; add unit coverage for
last-step Skip hiding, modal z-index reclaim, and all four onboarding
telemetry stages.
- Drive Floating UI autoUpdate manually so animationFrame polling runs only while a deferred target is still moving, then fall back to scroll/resize listeners instead of polling every frame for the whole step - Set aria-modal to false on interaction steps where the user must click outside the card - Ignore unrecognized ?coach= values; keep `any` as the replay keyword - Align the spotlight scrim with the landing backdrop (0.62 -> 0.6) to avoid a dim shift on landing -> spotlight - Export COACH_IDS and import it in the drift guard instead of a hardcoded in-sync list - Clarify why the landing needs an explicit Escape listener (Reka's DismissableLayer doesn't fire update:open here) - Test that the started telemetry event omits step_index/coach_id while per-step events include them Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Report the timed-out deferred step in skip telemetry by advancing the step index before ending the tour, instead of logging the prior step - Make the ?coach= override one-shot: named paths force-start only via the delayed timer (respecting START_DELAY_MS) and re-entry honors the seen-flag again, rather than bypassing it all session - Resolve waitForTarget to false immediately when the signal is already aborted, so it can't hang until timeout - Reuse the cached dialog locator in Tour.cardForStep
- Drop the ?coach= query param: remove the forced-entry/replay-any override and delayed force-start; tours now start only via auto-open or an explicit request. E2E replays via the in-app help button after entering app mode - Flatten coachmarkController to plain requestTour/onTourRequested exports instead of a useCoachmarkController composable - Derive the top-bar safe inset from the --comfy-topbar-height token plus CARD_GAP instead of hardcoding 56 - Remove the landing Start button's fixed width - Document the real cause of the landing Escape workaround (global keybinding preventDefaults Escape before Reka's DismissableLayer dismisses) - Trim verbose comments in coachmarkRegistry and TourOverlay tests
- Show public/assets/images/app-mode-landing.png on the welcome landing's left panel
…otes - clear the spotlight's previous ZIndex entry before re-raising, so step changes stop leaking entries into the shared modal stacking sequence - report tour telemetry as the user-visible numbering: 1-based step_number within counted spotlight steps, omitted for the landing - seed the e2e seen-tours setting from TOURS so future tours can't auto-open under unrelated suites
- derive step translation keys from a step `name` (onboardingCoachmarks.<tour>.<name>.*), with te()-checked primary/skip label overrides falling back to Next/Done/Skip - move all tour i18n into the engine; TourOverlay and TourSpotlight now receive translated title/body strings - replace the landing's required open model with a skip emit and drop the landingOpen writable computed - simplify TourOverlay.test.ts mocks with fromPartial - replace SCRIM_COLOR with Tailwind classes (bg-black/60, spotlight spread shadow) - narrow the app-run-button anchor so the spotlight excludes the run error warning - key the e2e tour fixture's replay button by tour name - drop the tour spec's template loading; the test server's default workflow already populates the graph (locally: pnpm dev:test) - prune comments that restated code
- tour step keys are built as onboardingCoachmarks.<tour>.<name>.*, so the literal-key scan can't see them
- a single sync() replaces the per-hook register/unregister logic; mounted/updated/unmounted all reconcile desired vs current id - drops the oldValue bookkeeping and its explanatory comment
This was referenced Aug 4, 2026
pull Bot
pushed a commit
to Penguinjanator/ComfyUI_frontend
that referenced
this pull request
Aug 4, 2026
…14143) > **Stack — 4 of 6.** Rebuilt from scratch after the review on Comfy-Org#13725, split into short PRs. > Based on Comfy-Org#13290 (coachmark engine); each PR stacks on the one before it. > > 1. Comfy-Org#14140 — engine: canvas targets for the coachmark tour > 2. Comfy-Org#14141 — Getting Started screen + entry gating > 3. Comfy-Org#14142 — role pins + live drift guard > 4. Comfy-Org#14143 — the tour itself + e2e walk ← this PR > 5. Comfy-Org#14144 — post-tour nudge > 6. Comfy-Org#14145 — unpinned workflows (share / template links) ## Summary The tour a new user actually walks: upload, prompt, run, result. Steps come from the resolved roles, targets are canvas nodes registered with the engine, and the engine renders and advances them. The parallel overlay stack from the previous attempt is conspicuously absent. ## Changes - `TOURS.firstRun` builds real `CoachStep`s with `coachId`s and registered canvas targets, so the engine's spotlight, positioning, copy resolution and telemetry all apply. - Camera framing is a glide then a fit against a known duration. No settle detection, no watchdog — `animateToBounds` is wall-clock, so the end time is already known. Reduced motion frames instantly rather than skipping. - The Run step gates on the paywall, consuming the click so the underlying handler never queues a refused run. - Run outcomes are matched to the tour's own workflow through the execution store, so a job queued elsewhere can't speak for the tour. - The tour turns Nodes 2.0 on when it starts: every step names a `[data-node-id]`, and a new user has no `Comfy.InstalledVersion`, so the versioned default never applies and the setting reads off. If no tour then starts, the switch is undone. - A Playwright walk: fresh profile → Getting Started → pick a template → steps → Run → result. ### From the review round (`c504c6a`, `48358e6`) - A rejecting tour resolver used to escape `begin()` and leave the engine stuck at `resolving`, refusing every later start. It now resolves to no steps. - `beginTour` consulted the engine only after persisting the renderer switch, mutating the tour registry and waiting out the preview. It bails first, and undoes only a switch it made itself. - The Run-button interceptor keyed on `step.name === 'run'`, so renaming a translation key changed control flow. It reads `selfAdvancing`. - A registered tour can now carry the condition it runs under, so the first-run tour is torn down when the layout its coachmarks need goes away — same path as a trigger's, without the seen flag. - The Result step's spinner is gone. Its predicate was the same expression as the copy beside it, and it kept spinning when a run ended without a recorded status. ## Review focus No feature-layer RAF, no second overlay, no second i18n namespace, no inline telemetry — the engine owns all of it. Worth knowing: onboarding is one-shot by design. A run that fails still ends the tour; the user sees a "didn't finish" card and moves on. --------- Co-authored-by: pythongosssss <125205205+pythongosssss@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: bymyself <cbyrne@comfy.org>
pull Bot
pushed a commit
to Penguinjanator/ComfyUI_frontend
that referenced
this pull request
Aug 4, 2026
…g#14145) > **Stack — 6 of 6.** Rebuilt from scratch after the review on Comfy-Org#13725, split into short PRs. > Based on Comfy-Org#13290 (coachmark engine); each PR stacks on the one before it. > > 1. Comfy-Org#14140 — engine: canvas targets for the coachmark tour > 2. Comfy-Org#14141 — Getting Started screen + entry gating > 3. Comfy-Org#14142 — role pins + live drift guard > 4. Comfy-Org#14143 — the tour itself + e2e walk > 5. Comfy-Org#14144 — post-tour nudge > 6. Comfy-Org#14145 — unpinned workflows (share / template links) ← this PR ## Summary Pins cover the eight templates the grid can show. This covers everything else — a shared workflow, or a template URL nobody curated — by reading the roles off the graph itself. Simon asked that any-workflow generality land as its own stacked PR with adversarial tests rather than riding along untested; this is that PR. ## Changes - `heuristicRoles` finds the source, prompt and sink on an unpinned graph. Pins still win where they exist; anything else falls through to here, so the heuristics are the default path rather than dead code. - Prompt selection is ranked, not matched: labels are normalised for case and separators, wiring to a `positive` input outranks a label saying so, and `negative` / `system` disqualify. A tie returns nothing rather than guessing. - `?share=` and `?template=` links offer the tour once their workflow is actually on canvas. ## Review focus The tests are adversarial by design — negative-before-positive ports, a node carrying both boxes, disagreeing sinks, bypassed nodes, ties, a 50-node graph — all built on real `LGraph` instances rather than copied templates. That was the condition for bringing heuristics back at all. --------- Co-authored-by: pythongosssss <125205205+pythongosssss@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: Maanil Verma <maanil@comfy.org> Co-authored-by: bymyself <cbyrne@comfy.org>
This was referenced Aug 4, 2026
christian-byrne
added
needs-backport
Member
|
@MaanilVerma Successfully backported to #14662 |
Member
|
@MaanilVerma Successfully backported to #14663 |
github-actions
Bot
removed
the
needs-backport
comfy-pr-bot
added a commit
that referenced
this pull request
Aug 4, 2026
…our ends (#14662) Backport of #14144 to `core/1.49` Automatically created by backport workflow. Co-authored-by: Maanil Verma <vermaMaanil97@gmail.com> Co-authored-by: pythongosssss <125205205+pythongosssss@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: bymyself <cbyrne@comfy.org>
comfy-pr-bot
added a commit
that referenced
this pull request
Aug 4, 2026
…tour ends (#14663) Backport of #14144 to `cloud/1.49` Automatically created by backport workflow. Co-authored-by: Maanil Verma <vermaMaanil97@gmail.com> Co-authored-by: pythongosssss <125205205+pythongosssss@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: bymyself <cbyrne@comfy.org>
This was referenced Aug 4, 2026
This was referenced Aug 4, 2026
pull Bot
pushed a commit
to Penguinjanator/ComfyUI_frontend
that referenced
this pull request
Aug 5, 2026
…ded (Comfy-Org#14677) ## Summary The first-run nudge picked its copy from whether a tour _started_, so a user who skipped on step 1 — or one whose tour was torn down by a target that never mounted — was congratulated for a first result they never made. This makes the copy follow how the tour actually ended. **The nudge's visibility is unchanged: every ending still shows it.** ## The defect `FirstRunTourNudge.vue` chose its copy with `` `…nudge.${tourWasShown ? 'ran' : 'noTour'}` ``, and `tourWasShown` was assigned from `started` — the return of `engine.startTour()` in `useFirstRunTourController.ts` — which means _a tour began_, not _the user completed one_: | ending | copy before | copy now | | -------------------------------- | ------------------------------------------------ | -------- | | completed | `ran` | `ran` | | never appeared | `noTour` ✓ | `noTour` | | **skipped on step 1** | **`ran`** — "You just made your first" | `noTour` | | **abandoned (`target_timeout`)** | **`ran`**, shown alongside the `loadError` toast | `noTour` | | postponed (paywall) | `ran` | `noTour` | | `trigger_lost` | `ran` | `noTour` | The abandoned case is the sharp one: the same event raises an error toast and a congratulation. ## Changes - **What**: `finish()` in `onboardingTourStore.ts` already computes `outcome` and `skipReason`; the store now hands that on as `lastEnding` (a discriminated union, cleared when a new run is requested so the previous ending cannot speak for it). `useFirstRunTourController` derives `tourWasCompleted` from it in place of `tourWasShown`, and the nudge reserves the `ran` copy for `completed`. Every other ending gets `noTour`, which reads as "here's where to go next" for a skipper or an abandoner. - **Breaking**: none. `lastEnding` is additive; `tourWasShown` had exactly one consumer (the nudge). ## Visibility is deliberately unchanged Every ending still arms the nudge, including the ones that now read as `noTour`. That is @MaanilVerma's product decision in Comfy-Org#14144: _"a user who saw no tour is the one who most needs somewhere to go next, so suppressing it there removes the way forward."_ Comfy-Org#14672 tried to suppress the nudge on the abandoned ending and was closed for exactly this reason — this PR treats the copy, not the visibility, as the defect. The regression guard for that is `still offers the nudge after a tour $named` in `useFirstRunTourController.test.ts`, which asserts `nudgeArmed` for all five endings, plus a sixth test for the never-started case and a component-level check that the nudge renders whether or not the tour finished. ## Tests New coverage, 19 tests: - `onboardingTourStore.test.ts` (6) — the ending recorded for a user skip, a `target_timeout` teardown, a `trigger_lost` teardown, a walked-to-the-end tour; that a running tour has no ending; that a new run drops the last one. - `useFirstRunTourController.test.ts` (10, plus an existing never-started test extended to assert visibility) — `completed` → congratulation; `user` / `target_timeout` / `postponed` / `trigger_lost` → none; **all five endings still arm the nudge**; never-started arms it too, with no congratulation. - `FirstRunTourNudge.test.ts` (3) — completed → `ran`, unfinished → `noTour`, and the nudge appears either way. ### Mutation evidence - Revert the rule to the old semantics (`tourWasCompleted = ending?.tour === 'firstRun'`, i.e. "a tour ran"): **4 failed | 45 passed**. The four are the skipped/abandoned/postponed/lost-context copy cases. The five visibility tests stayed green, confirming they are not coupled to the copy rule. - Pin the component's copy key to `ran`: **1 failed | 10 passed**. Pin it to `noTour`: **1 failed | 10 passed**. - Restored: **374 passed (23 files)** across `src/renderer/extensions/firstRunTour` and `src/platform/onboarding`. `pnpm install --frozen-lockfile`, `format:check`, `lint`, `typecheck` and `knip` all clean. ## Review Focus - `lastEnding` carries `skipReason` although no production code reads it today — the tests name each ending, so the reason has to be observable, and it is the shape `finish()` already computes. Say the word if you would rather it be trimmed to `{ tour, outcome }`. - The controller latches `tourWasCompleted` when the nudge arms rather than deriving it live, so a later tour ending cannot rewrite the copy of a nudge already on screen. - Not verified: no e2e run. `gettingStartedTour.spec.ts` → "leaves the nudge until the upgrade dialog closes" asserts visibility only (postponed still arms), and no e2e or unit test elsewhere asserts the nudge's copy strings — checked by grep, not by running Playwright. - Telemetry is untouched: `nudge_shown` and `explore_templates_clicked` still report only `{ tour: 'firstRun' }`, so the funnel still cannot tell a toured user from a tour-less one. That is a real gap, and deliberately out of scope here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
pull Bot
pushed a commit
to Penguinjanator/ComfyUI_frontend
that referenced
this pull request
Aug 12, 2026
…15107) @MaanilVerma — **your call entirely; close it if you'd rather not.** I owe you context on why it's arriving as a PR eight days late rather than as a question. ## The gap `OnboardingTourNudgeMetadata` is `{ tour }` only, so `nudge_shown` and `explore_templates_clicked` cannot be split by how the tour ended. That matters more here than it would elsewhere, because **every ending arms the nudge** — deliberately, per your call in Comfy-Org#14144: *"a user who saw no tour is the one who most needs somewhere to go next."* So the nudge's audience is deliberately a mix of *finished the tour* and *never saw one*, and right now the funnel cannot tell those two apart. A conversion from someone who completed the walkthrough and one from someone the tour never started for land in the same bucket. ## The change `tourWasCompleted` is already in scope in the component — it picks the copy one line above the telemetry call. This carries the same value onto both events: ```ts telemetry?.trackOnboardingTour('nudge_shown', { tour: 'firstRun', tour_completed: tourWasCompleted.value }) ``` Optional field, no new event, **no visibility rule changed**, nothing added to any other event. ## Verification `FirstRunTourNudge.test.ts` **12/12**. The two existing assertions pinned the exact metadata shape, so they failed until updated — which is the suite working. Added one case asserting both events carry `false` when the tour did not complete. Mutation-checked: hard-coding `tour_completed: true` fails exactly that new case, 11 others still pass. `typecheck` and `format:check` clean. ## Why it's late I drafted this as part of a Slack reply to you on **2026-08-04** and never sent it, so you have never actually seen the offer. That's the failure, not the telemetry. It was the one item in that draft still worth acting on — the rest of it has since been overtaken (the nudge copy landed in Comfy-Org#14677, and you've since settled the OG image on Comfy-Org#14957). If you'd rather have the *full* ending instead of a boolean — `completed` vs `skipped` with its `skipReason` — say so and I'll rework it; the controller has `engine.lastEnding` and only exposes the boolean today. I went with the boolean because it needed no change to the controller's surface. Co-authored-by: t <t@t.t>
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
6 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.
Summary
A card that slides up after the tour ends, pointing the user at the rest of the template library. Deliberately outcome-blind: it shows whether the run succeeded, failed, or the user skipped — whoever just finished needs somewhere to go next either way.
Changes
FirstRunTourNudge, armed when the tour deactivates and shown after a short delay.immediate: true. Mounting when the nudge is already due still shows it — the previous attempt waited for an edge and could strand it permanently.Review focus
The reconcile-at-setup shape is the fix worth checking. Both regression tests mount with pre-populated store state, which is the case that used to fail.