Skip to content

Commit 0f001a6

Browse files
christian-byrneConnor Byrne
andauthored
fix: stop transient ineligibility from permanently completing the first-run tour (Comfy-Org#15173)
## Summary `handleStartupOutcome` treated "not a tour candidate on this boot" as "already onboarded", permanently writing `Comfy.TutorialCompleted` for users who were only transiently ineligible. ## Changes - **What**: `isFirstRunCandidate()` returns false for five reasons, only one of which is stable. `handleStartupOutcome` marked the tutorial completed for all five. `Comfy.TutorialCompleted` is persisted server-side against the user record and is only ever set to `true`, so a single ineligible boot removed onboarding from that account forever. Replaced the boolean with a three-way decision: `complete` (mark + template browser), `defer` (template browser only, flag untouched), `getting-started`. `isFirstRunCandidate()` is now derived from it, so `handleUrlWorkflow` is unchanged. Classification: | condition | decision | why | | -------------------------- | -------- | --------------------------------------------------------------------------------- | | `!isCloud` | complete | build-time `__DISTRIBUTION__`; cannot change for this deployment | | `isNewUser() === false` | complete | a determined returning user does not become new again | | `!onboardingTourEnabled` | defer | server flag, defaults false before `/features` resolves, and flips during rollout | | `!isDesktopWidth` | defer | viewport | | `!isSubscriptionEnabled()` | defer | reads `window.__CONFIG__`, written by the same `/features` fetch | | `isNewUser() === null` | defer | undetermined is not a determination | Two reachable cases this fixes. Signing up on a phone and returning on a laptop is ordinary behaviour, and today it costs the user onboarding permanently — raised by @MaanilVerma on Comfy-Org#15091. The broader variant, found while verifying that one: every boot taken while `onboarding_tour_enabled` is off marks _every_ new user completed, so the entire cohort onboarded before the rollout can never see the tour once it is turned on. ## Review Focus `isSubscriptionEnabled()` is `isCloud && window.__CONFIG__?.subscription_required` — deployment config, not per-user subscription state, so "permanent" is arguable. It is classified transient because `window.__CONFIG__` is populated by the async `/features` fetch in `refreshRemoteConfig`, which sets it to `{}` on timeout (5s), on 401/403, and on any network error. A wedged features endpoint would otherwise burn the tour for every user who booted during the outage. The cost of being wrong is asymmetric: a wrong `defer` reopens the template browser one extra time, a wrong `complete` is unrecoverable. The `url-intent` branch still marks completed, unchanged. That user got a real first-run experience — their workflow loaded, and `handleUrlWorkflow` offers the tour over it — so it is a genuine completion, not an environmental miss. Candidacy is still read once per startup: `decideFirstRun()` is called a single time in `handleStartupOutcome`, so a later resize, flag refresh, or subscription change cannot unmount the Getting Started screen mid-interaction. Covered by the existing test. Non-candidates still get `Comfy.BrowseTemplates` on both paths. ## Tests Existing disqualifier table split into permanent and transient. Four new transient cases assert the flag is not written, plus an end-to-end case that runs two boots through a `setSetting` mock that actually persists: narrow viewport, then desktop. All five fail on `main` (`expected "vi.fn()" to not be called at all, but actually been called 1 times`, and `expected false to be true` for the two-boot case). Co-authored-by: Connor Byrne <c.byrne@comfy.org>
1 parent 0bc18c4 commit 0f001a6

2 files changed

Lines changed: 78 additions & 12 deletions

File tree

src/renderer/extensions/firstRunTour/gettingStarted/firstRunEntry.test.ts

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ describe('useFirstRunEntry', () => {
8585
mocks.isNewUser = true
8686
mocks.tourFlag = true
8787
mocks.settings = {}
88+
mocks.setSetting.mockImplementation((key: string, value: unknown) => {
89+
mocks.settings[key] = value
90+
})
8891
})
8992

9093
describe('what a fresh user sees', () => {
@@ -97,16 +100,19 @@ describe('useFirstRunEntry', () => {
97100
expect(mocks.execute).not.toHaveBeenCalled()
98101
})
99102

100-
const disqualifiers: [string, () => void][] = [
103+
const permanentDisqualifiers: [string, () => void][] = [
101104
['not on cloud', () => void (mocks.isCloud = false)],
105+
['a returning user', () => void (mocks.isNewUser = false)]
106+
]
107+
108+
const transientDisqualifiers: [string, () => void][] = [
102109
['below the md breakpoint', () => void (mocks.isDesktopWidth = false)],
103110
['subscription disabled', () => void (mocks.subscriptionEnabled = false)],
104-
['a returning user', () => void (mocks.isNewUser = false)],
105111
['new-user state undetermined', () => void (mocks.isNewUser = null)],
106112
['the tour flag off', () => void (mocks.tourFlag = false)]
107113
]
108114

109-
it.for(disqualifiers)(
115+
it.for([...permanentDisqualifiers, ...transientDisqualifiers])(
110116
'opens the template browser instead, with %s',
111117
async ([, disqualify]) => {
112118
disqualify()
@@ -119,12 +125,53 @@ describe('useFirstRunEntry', () => {
119125
'A non-candidate must keep the existing template-browser flow'
120126
).toBe(false)
121127
expect(mocks.execute).toHaveBeenCalledWith('Comfy.BrowseTemplates')
128+
}
129+
)
130+
131+
it.for(permanentDisqualifiers)(
132+
'marks the tutorial completed for %s, which no later boot can lift',
133+
async ([, disqualify]) => {
134+
disqualify()
135+
const entry = await freshEntry()
136+
137+
await entry.handleStartupOutcome('fresh')
138+
122139
expect(
123140
mocks.setSetting,
124141
'Without this the browser reopens on every launch, forever'
125142
).toHaveBeenCalledWith('Comfy.TutorialCompleted', true)
126143
}
127144
)
145+
146+
it.for(transientDisqualifiers)(
147+
'leaves the tutorial unmarked for %s, so a later boot can still onboard',
148+
async ([, disqualify]) => {
149+
disqualify()
150+
const entry = await freshEntry()
151+
152+
await entry.handleStartupOutcome('fresh')
153+
154+
expect(
155+
mocks.setSetting,
156+
'Comfy.TutorialCompleted is write-once and server-side; setting it here burns the tour for an account that was only ineligible this boot'
157+
).not.toHaveBeenCalled()
158+
}
159+
)
160+
161+
it('onboards a user whose earlier boot was only transiently ineligible', async () => {
162+
mocks.isDesktopWidth = false
163+
const phone = await freshEntry()
164+
await phone.handleStartupOutcome('fresh')
165+
166+
mocks.isDesktopWidth = true
167+
const laptop = await freshEntry()
168+
await laptop.handleStartupOutcome('fresh')
169+
170+
expect(
171+
laptop.gettingStartedVisible.value,
172+
'signing up on a phone and returning on a laptop is ordinary behaviour'
173+
).toBe(true)
174+
})
128175
})
129176

130177
it('marks a url-intent startup completed without taking over the screen', async () => {
@@ -167,8 +214,9 @@ describe('useFirstRunEntry', () => {
167214

168215
expect(
169216
entry.gettingStartedVisible.value ||
170-
mocks.setSetting.mock.calls.length > 0,
171-
'a startup that opened a blank canvas must either offer onboarding or record that it is done; anything else strands the user with no screen and no flag'
217+
mocks.setSetting.mock.calls.length > 0 ||
218+
mocks.execute.mock.calls.length > 0,
219+
'a startup that opened a blank canvas must offer onboarding, record that it is done, or open the template browser; anything else strands the user with an empty screen'
172220
).toBe(true)
173221
}
174222
)

src/renderer/extensions/firstRunTour/gettingStarted/firstRunEntry.ts

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,29 @@ export const useFirstRunEntry = createSharedComposable(() => {
2727
const isDesktopWidth =
2828
useBreakpoints(breakpointsTailwind).greaterOrEqual('md')
2929

30+
/**
31+
* `defer` is ineligibility a later boot can lift — the tour flag, the
32+
* viewport, remote config that has not arrived. `Comfy.TutorialCompleted` is
33+
* write-once and server-side, so only `complete` may set it.
34+
*/
35+
type FirstRunDecision = 'getting-started' | 'defer' | 'complete'
36+
37+
function decideFirstRun(): FirstRunDecision {
38+
if (!isCloud) return 'complete'
39+
40+
const isNewUser = useNewUserService().isNewUser()
41+
if (isNewUser === false) return 'complete'
42+
43+
if (!useFeatureFlags().flags.onboardingTourEnabled) return 'defer'
44+
if (!isDesktopWidth.value) return 'defer'
45+
if (!useSubscription().isSubscriptionEnabled()) return 'defer'
46+
if (isNewUser === null) return 'defer'
47+
48+
return 'getting-started'
49+
}
50+
3051
function isFirstRunCandidate(): boolean {
31-
if (!useFeatureFlags().flags.onboardingTourEnabled) return false
32-
if (!isCloud) return false
33-
if (!isDesktopWidth.value) return false
34-
if (!useSubscription().isSubscriptionEnabled()) return false
35-
return useNewUserService().isNewUser() === true
52+
return decideFirstRun() === 'getting-started'
3653
}
3754

3855
/**
@@ -51,12 +68,13 @@ export const useFirstRunEntry = createSharedComposable(() => {
5168
return
5269
}
5370

54-
if (isFirstRunCandidate()) {
71+
const decision = decideFirstRun()
72+
if (decision === 'getting-started') {
5573
gettingStartedVisible.value = true
5674
return
5775
}
5876

59-
await markTutorialCompleted()
77+
if (decision === 'complete') await markTutorialCompleted()
6078
await useCommandStore().execute('Comfy.BrowseTemplates')
6179
}
6280

0 commit comments

Comments
 (0)