onboarding: prevent user from skipping login - #6181
Conversation
wesrupert
commented
Sep 14, 2026
- Prevent user from skipping login during onboarding
- Add some additional checks to AI pages when a user is not signed in
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate issues remain in onboarding test flows and Vision/AI authentication lifecycle.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prevents skipping login during onboarding and adds signed-out protections for Streamlabs AI.
Changes:
- Updates onboarding flow, translations, tests, and helpers.
- Gates AI settings and landing-page controls by authentication.
- Prevents or manages Vision startup based on login state.
File summaries
| File | Summary |
|---|---|
test/regular/onboarding.ts |
Critical (3 votes): Anonymous flow remains on Login while the test proceeds to hardware setup. |
test/regular/obs-importer.ts |
Updates onboarding selectors. |
test/helpers/modules/onboarding.ts |
Critical (3 votes): skipOnboarding cannot skip the logged-out Login step, causing default suites to fail. |
app/services/vision/index.ts |
Moderate (2 votes): Vision may not retry startup after login. Critical (2 votes): Vision remains active after logout. |
app/services/onboarding/onboarding-v2.ts |
Moderate (1 vote): Recording mode can still show Continue on the logged-out login step. |
app/i18n/en-US/onboarding.json |
Adds onboarding translation updates. |
app/i18n/en-US/ai.json |
Adds AI authentication messaging. |
app/components-react/windows/settings/AISettings.tsx |
Gates AI settings for signed-out users. |
app/components-react/pages/AILanding.tsx |
Moderate (2 votes): The page may not rerender when authentication changes, leaving stale AI controls and guards. |
app/components-react/modals/onboarding/Splash.tsx |
Simplifies the onboarding entry point. |
Review details
Suppressed comments (2)
app/services/onboarding/onboarding-v2.ts:156
isSkippableonly controls the Skip link.RecordingLoginis not inNO_BUTTON_STEPS(app/components-react/modals/onboarding/Onboarding.tsx:15), so when recording mode is enabled and the user is logged out the footer still renders Continue;takeStep()can then advance past login (and complete if OBS is absent). Hide Continue forRecordingLoginor require authentication before advancing.
isSkippable: modifiers.loggedIn,
app/services/vision/index.ts:229
- This early return does not reconcile Vision with auth transitions. Because the enabled state is persisted, a logged-out startup with AI enabled returns here and never retries after a later login; conversely, logging out while Vision is running does not call
stop(), so the process/EventSource can continue for a signed-out user. Tie startup/shutdown to the UserService login/logout lifecycle (or reset the persisted enabled state) so the login requirement holds in both directions.
if (!this.userService.isLoggedIn) {
this.log('Vision is not supported for logged-out users.');
return;
}
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
BundleMonFiles updated (1)
Unchanged files (3)
Total files change +143B 0% Final result: ✅ View report in BundleMon website ➡️ |
3a9262b to
cf41c57
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical test-runner mismatch and unresolved onboarding, Vision, and signed-out AI issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
app/services/onboarding/onboarding-v2.ts:270
- When this test-only bypass is active, returning here also bypasses the
appService.setOnboarded(true)call immediately below. The old helper completed onboarding and established that state; with this path enabled,appService.state.onboardedremains false and changes menu/dismissable behavior in tests. Mark the app as onboarded before returning.
if (Utils.env.SLD_TESTS_SKIP_ONBOARDING) {
return;
}
app/services/vision/index.ts:240
- This authentication check only runs before the asynchronous update/start work. If logout occurs while
checkNeedsUpdate()ordownloadAndInstall()is pending, the logout handler can finishstop()before a child process exists, after which this invocation proceeds to start Vision while the user is logged out. Startup must be cancelled or serialized with logout, with the auth state checked through the final process start.
if (!this.userService.isLoggedIn) {
this.log('Vision is not supported for logged-out users.');
return;
}
test/regular/onboarding.ts:123
- When both flags are false, this branch has just navigated from Splash to Login, but Login is non-skippable for a logged-out user. Therefore the later
clickIfDisplayed('button=Skip')is a no-op andfn()runs while the login screen is still shown; the activeLogin new user after onboarding skippedtest then waits for the setup screen and fails. Update or remove that now-invalid logged-out scenario instead of invoking the callback here.
if (newUser || login) {
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
| <SwitchInput | ||
| label={$t('Turn On AI')} | ||
| disabled={visionState.isStarting} | ||
| disabled={!isLoggedIn || visionState.isStarting} |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in authentication guards, Vision startup, and onboarding test flows.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
test/helpers/webdriver/index.ts:348
- The explicit
skipOnboarding()call was removed, but theskipOnboardingimport near the top of this file is still present and is now unused. This will fail the repository's unused-variable lint rule; remove the stale import along with the call-site change.
app/components-react/pages/AILanding.tsx:271
- This only disables the toggle for logged-out users.
VisionEnabledStateis persisted and the logout handler stops the process without clearingisEnabled, so a user who enabled AI before logging out can still use the feature cards below (disabled={!enabled}). In particular,WidgetsService.createWidget()returnsundefinedwhen logged out, butonGamePulseWidgetClicklater dereferenceswidget.getSource(), causing an exception. Gate these actions onisLoggedInas well, or reset the persisted enabled state on logout.
disabled={!isLoggedIn || visionState.isStarting}
app/services/vision/index.ts:240
- This login check runs only before the asynchronous update/install work. If logout happens after this check but before
ensureStarted(), the logout handler'sstop()can finish before this in-flight call and the runner will still be started and subscribed for a logged-out user. Re-check authentication immediately before starting, and cancel or invalidate stale startup work on logout.
if (!this.userService.isLoggedIn) {
this.log('Vision is not supported for logged-out users.');
return;
}
test/regular/obs-importer.ts:54
- After this click, the path creates a non-skippable Login step while the test user is still logged out.
testingFakeAuth()logs the user in directly and does not advance or recompute that step, so the laterclickIfDisplayed('button=Skip')is a no-op and the test remains on Login while waiting forConnect Platforms. Re-enter the flow after authentication (as the onboarding tests do) or otherwise recompute the step's skippability before continuing.
await clickWhenDisplayed('button=Get Started', { timeout: 15000 });
test/regular/onboarding.ts:125
- The
newUser === false && login === falsepath still tries to skip afterGet Started, but the new onboarding path marks the Login step non-skippable for a logged-out user.clickIfDisplayed('button=Skip')therefore does nothing and theLogin new user after onboarding skippedtest later waits for the hardware step while still on Login, timing out. This test/helper path needs to be redesigned for the new mandatory-login flow rather than retaining the old skip assumption.
if (newUser || login) {
await isDisplayed('button=Twitch');
const user = await logIn(t, 'twitch', { prime: false }, false, true, newUser);
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
| runnerEnv: { | ||
| SLD_TESTS_SKIP_ONBOARDING: options.skipOnboarding ? 'true' : '', | ||
| }, |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved onboarding, authentication-gating, and test-environment issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
app/components-react/modals/onboarding/Splash.tsx:13
- Removing the recording-mode entry from this splash makes the V2
RecordingLoginpath unreachable during onboarding: the only onboarding code that enabledRecordingModeServicewas the deleted handler. Users can no longer choose recording-only onboarding, even though the service andRecordingLoginstep remain implemented; retain that entry and make the recording login step non-skippable while logged out instead.
function getStarted() {
OnboardingV2Service.actions.takeStep();
app/components-react/pages/AILanding.tsx:271
- Disabling only the main switch does not disable the feature actions below it.
VisionServicepreservesenabledState.isEnabledon logout, so a user who logs out while AI is enabled still gets enabled Game Pulse/Sidekick/overlay buttons (disabled={!enabled}); in particular,createWidget()returnsundefinedfor logged-out users and this page then dereferenceswidget.getSource(). Gate these feature actions onisLoggedInas well, or clear the enabled state on logout.
disabled={!isLoggedIn || visionState.isStarting}
app/services/onboarding/onboarding-v2.ts:270
- This early return skips the existing
this.appService.setOnboarded(true)call below, so every test run that uses the new skip flag leavesAppService.state.onboardedfalse. Code that gates menus and dismissables on that state will continue behaving as if onboarding is incomplete; set the app as onboarded before returning (or complete the same state transition as normal onboarding).
if (Utils.env.SLD_TESTS_SKIP_ONBOARDING) {
this.appService.setOnboarded(true);
return;
app/services/onboarding/onboarding-v2.ts:157
- Making the Login step non-skippable only removes the footer button; the platform-login path still calls
OnboardingV2Service.actions.takeStep()afterstartAuthregardless of its result. A cancelled or failed platform auth can therefore still advance past Login without a signed-in user. Advance only forEPlatformCallResult.Success(and handle the two-factor result separately), as the Streamlabs ID path already does.
return {
name: modifiers.recordingMode ? EOnboardingSteps.RecordingLogin : EOnboardingSteps.Login,
isSkippable: modifiers.loggedIn,
};
app/services/vision/index.ts:166
- Stopping on logout does not cancel an in-flight
ensureRunning(): that method can be awaiting an update or process start, and it only checksisLoggedInbefore those awaits. If logout happens during that window, this stop can complete andensureRunning()can then start Vision for the logged-out user. Serialize start/stop or invalidate/recheck the start immediately before and after the awaited startup work.
this.userService.userLogout.subscribe(() => {
this.log('Vision is not supported for logged-out users, stopping.');
void this.stop();
});
app/services/vision/index.ts:240
- This check only prevents the runner from starting;
setIsEnabled(true)has already persisted the enabled state, and the source/widget/automation hooks callsetIsEnabled(true)without an authentication check. A logged-out user can therefore leave AI enabled while no process runs, which also re-enables the landing-page actions. Enforce the login requirement when enabling insetIsEnabled, not only inensureRunning().
if (!this.userService.isLoggedIn) {
this.log('Vision is not supported for logged-out users.');
return;
}
test/helpers/webdriver/index.ts:279
runnerEnvis only placed on the options passed towebdriverio.remote, butApplication.start()has already spawned Chromedriver with an explicit environment and never applies this field to it or to Electron. ConsequentlySLD_TESTS_SKIP_ONBOARDINGis absent from the app process, so the defaultskipOnboarding: truepath no longer skips onboarding and most regular tests will start on the welcome screen. Pass this variable through theChildProcess.spawnenvironment instead.
SLD_TESTS_SKIP_ONBOARDING: options.skipOnboarding ? 'true' : '',
test/regular/onboarding.ts:125
- The unauthenticated branch still calls
clickIfDisplayed('button=Skip'), butSplash -> Loginnow creates a non-skippable Login step when the user is logged out. The activeLogin new user after onboarding skippedtest invokes this helper with both flags false, so it remains on the Login page andfinishOnboarding()immediately fails; update or remove that obsolete no-login scenario to match the new required-login flow.
if (newUser || login) {
await isDisplayed('button=Twitch');
const user = await logIn(t, 'twitch', { prime: false }, false, true, newUser);
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
| return; | ||
| } | ||
| await clickWhenDisplayed('a=Log In', { timeout: 15000 }); | ||
| await clickWhenDisplayed('button=Get Started', { timeout: 15000 }); |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved authentication, Vision startup, and onboarding test issues must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
app/components-react/pages/AILanding.tsx:271
- Disabling only the switch does not protect the rest of this page:
VisionEnabledStateis persisted, and logout does not necessarily clear it when no Vision process is running. A logged-out session can therefore still haveenabled === true, leaving the Reactive Overlays, Game Pulse, and Sidekick actions enabled because they only usedisabled={!enabled}. Gate those feature actions onisLoggedInas well (or derive an effective enabled value).
disabled={!isLoggedIn || visionState.isStarting}
app/components-react/windows/settings/AISettings.tsx:122
- The authentication guard is applied to the process/game selectors, but not to the Events Log and Display Frame buttons above. Because logout stops Vision asynchronously, those buttons can remain usable while
isRunningis still stale; include the login state in their disabled conditions as well.
disabled={!isLoggedIn || !enabled || !isRunning}
app/services/vision/index.ts:237
- This login check only runs before the asynchronous update/install work. If the user logs out while
checkNeedsUpdate(),ensureUpdated(), or another awaited operation is in progress, the logout handler can finishstop()and the pending call will still reachvisionRunner.ensureStarted()and start Vision for a logged-out user. Re-check login state or cancel/invalidate in-flight startup immediately before starting the runner.
if (!this.userService.isLoggedIn) {
app/services/vision/index.ts:166
- This logout path stops the process but leaves the persisted
VisionEnabledState.isEnabledset totrue.AILandinguses that flag to enable its feature cards, so a user who logs out while AI is enabled can still click Reactive Overlays, Game Pulse, or Sidekick actions even though they are signed out. Clear the enabled state on logout or addisLoggedInguards to every AI action.
this.userService.userLogout.subscribe(() => {
this.log('Vision is not supported for logged-out users, stopping.');
void this.stop();
});
test/helpers/webdriver/index.ts:279
- This does not propagate the flag to the Electron process.
Application.start()spawns chromedriver with an explicit environment containing onlyNODE_ENVandSLOBS_CACHE_DIR(lines 126-131), andremote()only connects to that already-running driver; nothing in this runner consumesrunnerEnv. As a result, the default tests no longer skip onboarding after the explicitskipOnboarding()call was removed and will start on the onboarding screen. PassSLD_TESTS_SKIP_ONBOARDINGthrough the spawned process environment (or another Electron startup mechanism) before removing the old helper.
SLD_TESTS_SKIP_ONBOARDING: options.skipOnboarding ? 'true' : '',
test/regular/onboarding.ts:123
- When both flags are false, this branch still invokes
fnwithout advancing past Login. Since the new Login step is non-skippable while logged out, the activeLogin new user after onboarding skippedtest reaches this branch and then waits for the setup step while the login screen is still displayed. Update or remove that scenario, or have the helper complete the now-required login before invokingfn.
if (newUser || login) {
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
3b71ca3 to
68e1a4f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect logged-out AI/Vision behavior and onboarding test correctness.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
app/components-react/pages/AILanding.tsx:271
- This only disables the master switch.
VisionEnabledStateis persisted and the logout handler stops Vision without clearing it, so after a user logs out while AI was enabled, the threeAIFeatureactions remain enabled because they only usedisabled={!enabled}. The user can still invoke Game Pulse or Sidekick actions from the AI page while logged out; gate those actions onisLoggedInas well (or explicitly reset the enabled state on logout).
disabled={!isLoggedIn || visionState.isStarting}
app/services/vision/index.ts:240
- The login check only runs when
ensureRunning()starts. If logout occurs while the update/check awaits below are in progress, the new logout handler can stop the current process, then this in-flight call continues tovisionRunner.ensureStarted()and starts Vision for a logged-out user. Revalidate authentication or cancel/invalidate in-flight startup before starting the runner.
if (!this.userService.isLoggedIn) {
this.log('Vision is not supported for logged-out users.');
return;
}
test/helpers/modules/onboarding.ts:30
- This default changes the meaning of every existing caller:
test/regular/onboarding.tsandtest/regular/obs-importer.tspreviously calledlogIn(..., isNewUser=false), but this helper now passestruetotestingFakeAuth. As a result, the old-user onboarding case no longer tests an old user, and the OBS importer test takes the new-user scene-collection path. Keep the default false and pass true explicitly only from the new-user onboarding case.
export async function advancePastOnboardingLogin(t: TExecutionContext, newUser = true) {
test/helpers/webdriver/index.ts:280
runnerEnvis only attached to the object passed towebdriverio.remote(), butApplication.start()launches chromedriver with an explicit environment containing onlyNODE_ENVandSLOBS_CACHE_DIR. This arbitrary remote option is not propagated to the Electron process, soUtils.env.SLD_TESTS_SKIP_ONBOARDINGis never set and tests using the defaultskipOnboarding: truewill still open onboarding instead of skipping it. Pass the variable through the spawned process environment (and type the custom option), or retain the UI skip path.
runnerEnv: {
SLD_TESTS_SKIP_ONBOARDING: options.skipOnboarding ? 'true' : '',
},
test/regular/onboarding.ts:245
- The active “as a new user” case is documented as the no-theme scenario and still calls
confirmDefaultSourceswithHasDefaultSources, but this change installs a theme. That makes the test exercise a different flow and conflicts with its expected source assertion; the separate skipped case already covers theme installation.
await finishOnboarding(true);
test/regular/onboarding.ts:225
- Because the Login step is now created with
isSkippable: modifiers.loggedIn, a logged-out user has no Skip button afterGet Started. This skipped test's no-login path therefore cannot reachConnect Platforms, so its “functional” note is no longer accurate; update/rewrite or remove this case before re-enabling it.
await clickIfDisplayed('button=Skip');
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
| } | ||
| await clickWhenDisplayed('a=Log In', { timeout: 15000 }); | ||
| await waitForDisplayed('button=Twitch'); | ||
| await advancePastOnboardingLogin(t); |
| { timeout: 15000 }, | ||
| ); | ||
| await waitForDisplayed('span=100%'); | ||
| await sleep(1000); |
68e1a4f to
a04e310
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in onboarding, authentication, Vision startup, and test flows.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
app/components-react/modals/onboarding/Splash.tsx:30
- Replacing the Splash actions with only
Get Startedremoves the only fresh-onboarding entry point that enables recording mode, even though the v2 path still documents and implements a separateRecordingLoginflow. This is an unrelated user-facing regression to the stated login requirement; preserve an equivalent recording-mode entry while making its login step non-skippable, or explicitly remove the now-dead recording onboarding path.
<Button onClick={getStarted} type="primary" className={styles.bigButton}>
{$t('Get Started')}
app/components-react/pages/AILanding.tsx:271
- The new guard disables only the toggle.
enabledState.isEnabledis not synchronously cleared byuserLogout(it is only reset from the runner-exit callback), so after logout or during shutdown the feature buttons below can remain enabled wheneverenabledis true. In that stateWidgetsService.createWidgetreturnsundefinedfor a logged-out user andonGamePulseWidgetClickdereferenceswidget.getSource(). Include auth in the feature-action disabled predicates and/or guard those callbacks.
disabled={!isLoggedIn || visionState.isStarting}
app/services/vision/index.ts:166
- The logout handler starts
stop()but does not cancel or invalidate anensureRunning()already in progress. That method can pass its login check, await update/start operations, and then callensureStarted()after logout, recreating Vision and its EventSource for a logged-out user. Add a cancellation/generation check around the asynchronous startup so logout wins the race.
this.userService.userLogout.subscribe(() => {
this.log('Vision is not supported for logged-out users, stopping.');
void this.stop();
});
test/helpers/webdriver/index.ts:280
runnerEnvis only added to the WebdriverIORemoteOptions;Application.start()launches chromedriver with an explicit environment containing onlyNODE_ENVandSLOBS_CACHE_DIR, and does not forward this field to chromedriver or Electron. As a result,SLD_TESTS_SKIP_ONBOARDINGnever reachesUtils.env, so the defaultskipOnboarding: trueno longer skips onboarding and the regular test suite starts behind the modal. Forward this value through the spawned process environment (or keep the old skip helper).
runnerEnv: {
SLD_TESTS_SKIP_ONBOARDING: options.skipOnboarding ? 'true' : '',
},
test/regular/obs-importer.ts:47
- The old OBS Importer flow passed
isNewUser=false, but this helper defaultsnewUsertotrue. This now marks an existing-user import as a first login and changes downstream scene-collection/default-source behavior; passfalsehere while keeping the new-user onboarding callers explicit.
await advancePastOnboardingLogin(t);
test/regular/onboarding.ts:137
- A fixed one-second pause does not wait for
installOverlayor the onboarding transition to finish. A slow download can leave the test on the installing view, while the laterisDisplayedcall is neither a wait nor an assertion, making this test race the installation. Wait for the final Sources view (with a suitable timeout) instead.
await sleep(1000);
test/regular/onboarding.ts:225
- This skipped test still models onboarding without authentication: after Get Started it clicks Skip on the Login step. The new flow deliberately makes that step non-skippable while logged out, so if the test is re-enabled it cannot reach the theme/source assertions. Update or remove this obsolete case rather than leaving it as a nonfunctional test.
await clickIfDisplayed('button=Skip');
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
| return { | ||
| name: modifiers.recordingMode ? EOnboardingSteps.RecordingLogin : EOnboardingSteps.Login, | ||
| isSkippable: modifiers.loggedIn, | ||
| }; |
| await goThroughOnboarding(t, login, newUser, async () => { | ||
| await finishOnboarding(installTheme); | ||
| await goThroughOnboarding(t, async () => { | ||
| await finishOnboarding(true); |