Skip to content

Commit 5be53a4

Browse files
Ajit-MehrotraAjit-Mehrotra
authored
fix(onboarding): separate apply and completion flows (#1948)
## Summary - stop the Summary step from marking onboarding complete and keep it focused on applying settings - move persistent onboarding completion to the Next Steps screen through one shared code path used by both Go to Dashboard and Reboot - make the final Next Steps screen close explicitly after completion so force-opening onboarding starts from Overview instead of re-persisting `CONFIGURE_SETTINGS` - keep bypass and the top-right close behavior unchanged - address valid CodeRabbit feedback by tightening Summary logging, making the async completion error accessible, using a semantic failure assertion in tests, and fixing stale onboarding docs - keep the new modal regression coverage type-check-safe so the branch stays green under `vue-tsc` ## Validation - confirmed on the latest main-based branch that Summary still called `completeOnboarding()` before this fix - confirmed the top-right X still uses the backend close path and bypass still only hides onboarding without completion - confirmed Go to Dashboard and Reboot did not previously share a completion path - traced the regression back to the original onboarding-flow introduction commit `15bd7477` (`feat(onboarding): add new onboarding flows for Unraid OS`), so this was not a recent regression from this PR series - confirmed the force-open bug happened because `Go to Dashboard` cleared the draft before calling the modal's generic next-step handler, which then fell back to Overview and immediately re-saved step 2; Reboot did not share that bug because it never calls the modal advance path after cleanup ## Testing - `pnpm --filter @unraid/web lint` - `pnpm --filter @unraid/web type-check` - `pnpm --filter @unraid/web test` - latest full verification completed successfully after the final follow-up commit: 63 files, 596 tests passed, 6 skipped ## Notes - bypass behavior was intentionally left unchanged
1 parent 8f4ea98 commit 5be53a4

File tree

8 files changed

+193
-121
lines changed

8 files changed

+193
-121
lines changed

‎web/__test__/components/Onboarding/OnboardingModal.test.ts‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,21 @@ vi.mock('~/components/Onboarding/stepRegistry', () => ({
118118
ADD_PLUGINS: { template: '<div data-testid="plugins-step" />' },
119119
ACTIVATE_LICENSE: { template: '<div data-testid="license-step" />' },
120120
SUMMARY: { template: '<div data-testid="summary-step" />' },
121-
NEXT_STEPS: { template: '<div data-testid="next-step" />' },
121+
NEXT_STEPS: {
122+
props: ['onComplete'],
123+
setup(props: { onComplete: () => void }) {
124+
const handleClick = () => {
125+
cleanupOnboardingStorageMock();
126+
props.onComplete();
127+
};
128+
129+
return {
130+
handleClick,
131+
};
132+
},
133+
template:
134+
'<div data-testid="next-step"><button data-testid="next-step-complete" @click="handleClick">finish</button></div>',
135+
},
122136
},
123137
}));
124138

@@ -313,6 +327,22 @@ describe('OnboardingModal.vue', () => {
313327
expect(cleanupOnboardingStorageMock).toHaveBeenCalledWith();
314328
});
315329

330+
it('closes the modal from next steps after draft cleanup instead of persisting step 2 again', async () => {
331+
onboardingDraftStore.currentStepId.value = 'NEXT_STEPS';
332+
cleanupOnboardingStorageMock.mockImplementation(() => {
333+
onboardingDraftStore.currentStepId.value = null;
334+
});
335+
336+
const wrapper = mountComponent();
337+
338+
await wrapper.find('[data-testid="next-step-complete"]').trigger('click');
339+
await flushPromises();
340+
341+
expect(onboardingModalStoreState.closeModal).toHaveBeenCalledTimes(1);
342+
expect(onboardingDraftStore.setCurrentStep).not.toHaveBeenCalledWith('CONFIGURE_SETTINGS');
343+
expect(onboardingDraftStore.currentStepId.value).toBeNull();
344+
});
345+
316346
it('shows a loading state while exit confirmation is closing the modal', async () => {
317347
let closeModalDeferred:
318348
| {

‎web/__test__/components/Onboarding/OnboardingNextStepsStep.test.ts‎

Lines changed: 66 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { flushPromises, mount } from '@vue/test-utils';
33

44
import { beforeEach, describe, expect, it, vi } from 'vitest';
55

6+
import { COMPLETE_ONBOARDING_MUTATION } from '~/components/Onboarding/graphql/completeUpgradeStep.mutation';
67
import OnboardingNextStepsStep from '~/components/Onboarding/steps/OnboardingNextStepsStep.vue';
78
import { createTestI18n } from '../../utils/i18n';
89

@@ -11,6 +12,9 @@ const {
1112
activationCodeDataStore,
1213
submitInternalBootRebootMock,
1314
cleanupOnboardingStorageMock,
15+
completeOnboardingMock,
16+
refetchOnboardingMock,
17+
useMutationMock,
1418
} = vi.hoisted(() => ({
1519
draftStore: {
1620
internalBootApplySucceeded: false,
@@ -32,6 +36,9 @@ const {
3236
},
3337
submitInternalBootRebootMock: vi.fn(),
3438
cleanupOnboardingStorageMock: vi.fn(),
39+
completeOnboardingMock: vi.fn().mockResolvedValue({}),
40+
refetchOnboardingMock: vi.fn().mockResolvedValue({}),
41+
useMutationMock: vi.fn(),
3542
}));
3643

3744
vi.mock('@unraid/ui', () => ({
@@ -55,6 +62,12 @@ vi.mock('~/components/Onboarding/store/activationCodeData', () => ({
5562
useActivationCodeDataStore: () => reactive(activationCodeDataStore),
5663
}));
5764

65+
vi.mock('~/components/Onboarding/store/onboardingStatus', () => ({
66+
useOnboardingStore: () => ({
67+
refetchOnboarding: refetchOnboardingMock,
68+
}),
69+
}));
70+
5871
vi.mock('~/components/Onboarding/composables/internalBoot', () => ({
5972
submitInternalBootReboot: submitInternalBootRebootMock,
6073
}));
@@ -63,10 +76,27 @@ vi.mock('~/components/Onboarding/store/onboardingStorageCleanup', () => ({
6376
cleanupOnboardingStorage: cleanupOnboardingStorageMock,
6477
}));
6578

79+
vi.mock('@vue/apollo-composable', async () => {
80+
const actual =
81+
await vi.importActual<typeof import('@vue/apollo-composable')>('@vue/apollo-composable');
82+
return {
83+
...actual,
84+
useMutation: useMutationMock,
85+
};
86+
});
87+
6688
describe('OnboardingNextStepsStep', () => {
6789
beforeEach(() => {
6890
vi.clearAllMocks();
6991
draftStore.internalBootApplySucceeded = false;
92+
completeOnboardingMock.mockResolvedValue({});
93+
refetchOnboardingMock.mockResolvedValue({});
94+
useMutationMock.mockImplementation((doc: unknown) => {
95+
if (doc === COMPLETE_ONBOARDING_MUTATION) {
96+
return { mutate: completeOnboardingMock };
97+
}
98+
return { mutate: vi.fn() };
99+
});
70100
});
71101

72102
const mountComponent = () => {
@@ -84,18 +114,21 @@ describe('OnboardingNextStepsStep', () => {
84114
return { wrapper, onComplete };
85115
};
86116

87-
it('continues to dashboard when reboot is not required', async () => {
117+
it('continues to dashboard through the shared completion path when reboot is not required', async () => {
88118
const { wrapper, onComplete } = mountComponent();
89119

90120
const button = wrapper.find('[data-testid="brand-button"]');
91121
await button.trigger('click');
92122
await flushPromises();
93123

124+
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
125+
expect(refetchOnboardingMock).toHaveBeenCalledTimes(1);
126+
expect(cleanupOnboardingStorageMock).toHaveBeenCalledWith();
94127
expect(onComplete).toHaveBeenCalledTimes(1);
95128
expect(submitInternalBootRebootMock).not.toHaveBeenCalled();
96129
});
97130

98-
it('shows reboot warning dialog and waits for confirmation', async () => {
131+
it('marks onboarding complete through the same path before rebooting', async () => {
99132
draftStore.internalBootApplySucceeded = true;
100133
const { wrapper, onComplete } = mountComponent();
101134

@@ -115,8 +148,39 @@ describe('OnboardingNextStepsStep', () => {
115148
await confirmButton!.trigger('click');
116149
await flushPromises();
117150

151+
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
152+
expect(refetchOnboardingMock).toHaveBeenCalledTimes(1);
118153
expect(cleanupOnboardingStorageMock).toHaveBeenCalledWith();
119154
expect(submitInternalBootRebootMock).toHaveBeenCalledTimes(1);
120155
expect(onComplete).not.toHaveBeenCalled();
121156
});
157+
158+
it('continues when the completion refresh fails after marking onboarding complete', async () => {
159+
refetchOnboardingMock.mockRejectedValueOnce(new Error('refresh failed'));
160+
const { wrapper, onComplete } = mountComponent();
161+
162+
const button = wrapper.find('[data-testid="brand-button"]');
163+
await button.trigger('click');
164+
await flushPromises();
165+
166+
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
167+
expect(refetchOnboardingMock).toHaveBeenCalledTimes(1);
168+
expect(cleanupOnboardingStorageMock).toHaveBeenCalledWith();
169+
expect(onComplete).toHaveBeenCalledTimes(1);
170+
});
171+
172+
it('shows an error and stays on the page when completion fails', async () => {
173+
completeOnboardingMock.mockRejectedValueOnce(new Error('offline'));
174+
const { wrapper, onComplete } = mountComponent();
175+
176+
const button = wrapper.find('[data-testid="brand-button"]');
177+
await button.trigger('click');
178+
await flushPromises();
179+
180+
expect(wrapper.find('[role="alert"]').exists()).toBe(true);
181+
expect(cleanupOnboardingStorageMock).not.toHaveBeenCalled();
182+
expect(refetchOnboardingMock).not.toHaveBeenCalled();
183+
expect(onComplete).not.toHaveBeenCalled();
184+
expect(submitInternalBootRebootMock).not.toHaveBeenCalled();
185+
});
122186
});

‎web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts‎

Lines changed: 18 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,7 @@ describe('OnboardingSummaryStep', () => {
639639
await vi.runAllTimersAsync();
640640
await flushPromises();
641641

642-
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
642+
expect(completeOnboardingMock).not.toHaveBeenCalled();
643643
});
644644

645645
it('skips core setting mutations when baseline is loaded and nothing changed', async () => {
@@ -651,7 +651,7 @@ describe('OnboardingSummaryStep', () => {
651651
expect(setThemeMock).not.toHaveBeenCalled();
652652
expect(setLocaleMock).not.toHaveBeenCalled();
653653
expect(updateSshSettingsMock).not.toHaveBeenCalled();
654-
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
654+
expect(completeOnboardingMock).not.toHaveBeenCalled();
655655
});
656656

657657
it('keeps custom baseline server identity when draft mirrors baseline values', async () => {
@@ -854,56 +854,34 @@ describe('OnboardingSummaryStep', () => {
854854

855855
it.each([
856856
{
857-
caseName: 'baseline available + completion/refetch succeed',
857+
caseName: 'baseline available + apply succeeds',
858858
apply: () => {},
859859
assertExpected: (wrapper: ReturnType<typeof mountComponent>['wrapper']) => {
860-
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
861-
expect(refetchOnboardingMock).toHaveBeenCalledTimes(1);
860+
expect(completeOnboardingMock).not.toHaveBeenCalled();
861+
expect(refetchOnboardingMock).not.toHaveBeenCalled();
862862
expect(wrapper.text()).toContain('Setup Applied');
863863
expect(wrapper.text()).not.toContain('Setup Saved in Best-Effort Mode');
864+
expect(wrapper.text()).toContain(
865+
'Settings applied. Continue to the final step to finish onboarding.'
866+
);
864867
},
865868
},
866869
{
867-
caseName: 'baseline available + onboarding refetch fails',
868-
apply: () => {
869-
refetchOnboardingMock.mockRejectedValue(new Error('refresh failed'));
870-
},
871-
assertExpected: (wrapper: ReturnType<typeof mountComponent>['wrapper']) => {
872-
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
873-
expect(refetchOnboardingMock).toHaveBeenCalledTimes(1);
874-
expect(wrapper.text()).toContain('Could not refresh onboarding state right now. Continuing.');
875-
expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode');
876-
},
877-
},
878-
{
879-
caseName: 'baseline unavailable + completion succeeds',
870+
caseName: 'baseline unavailable + apply succeeds',
880871
apply: () => {
881872
coreSettingsResult.value = null;
882873
coreSettingsError.value = new Error('Graphql is offline.');
883874
},
884875
assertExpected: (wrapper: ReturnType<typeof mountComponent>['wrapper']) => {
885-
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
886-
expect(refetchOnboardingMock).not.toHaveBeenCalled();
887-
expect(wrapper.text()).toContain('Skipping onboarding state refresh while API is unavailable.');
888-
expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode');
889-
},
890-
},
891-
{
892-
caseName: 'completion mutation fails',
893-
apply: () => {
894-
completeOnboardingMock.mockRejectedValue(new Error('offline'));
895-
},
896-
assertExpected: (wrapper: ReturnType<typeof mountComponent>['wrapper']) => {
897-
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
876+
expect(completeOnboardingMock).not.toHaveBeenCalled();
898877
expect(refetchOnboardingMock).not.toHaveBeenCalled();
899-
expect(cleanupOnboardingStorageMock).not.toHaveBeenCalled();
900878
expect(wrapper.text()).toContain(
901-
'Could not mark onboarding complete right now (API may be offline): offline'
879+
'Baseline settings unavailable. Continuing in best-effort mode.'
902880
);
903881
expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode');
904882
},
905883
},
906-
])('follows completion endpoint decision matrix ($caseName)', async (scenario) => {
884+
])('keeps summary apply-only behavior ($caseName)', async (scenario) => {
907885
scenario.apply();
908886

909887
const { wrapper } = mountComponent();
@@ -917,27 +895,23 @@ describe('OnboardingSummaryStep', () => {
917895

918896
await clickApply(wrapper);
919897

920-
expect(completeOnboardingMock).toHaveBeenCalledTimes(1);
898+
expect(completeOnboardingMock).not.toHaveBeenCalled();
921899
expect(cleanupOnboardingStorageMock).not.toHaveBeenCalled();
922900
});
923901

924-
it('retries completeOnboarding after transient network errors when SSH changed', async () => {
902+
it('does not attempt onboarding completion after applying SSH changes', async () => {
925903
draftStore.useSsh = true;
926904
updateSshSettingsMock.mockResolvedValue({
927905
data: {
928906
updateSshSettings: { id: 'vars', useSsh: true, portssh: 22 },
929907
},
930908
});
931-
completeOnboardingMock
932-
.mockRejectedValueOnce(new Error('NetworkError when attempting to fetch resource.'))
933-
.mockResolvedValueOnce({});
934909

935910
const { wrapper } = mountComponent();
936911
await clickApply(wrapper);
937912

938-
expect(completeOnboardingMock).toHaveBeenCalledTimes(2);
913+
expect(completeOnboardingMock).not.toHaveBeenCalled();
939914
expect(wrapper.text()).toContain('Setup Applied');
940-
expect(wrapper.text()).not.toContain('Could not mark onboarding complete right now');
941915
});
942916

943917
it('retries final identity update after transient network errors when SSH changed', async () => {
@@ -961,7 +935,7 @@ describe('OnboardingSummaryStep', () => {
961935
expect(wrapper.text()).not.toContain('Server identity request returned an error, continuing');
962936
});
963937

964-
it('prefers best-effort result over timeout classification when completion fails', async () => {
938+
it('prefers warnings over success when plugin installation times out', async () => {
965939
draftStore.selectedPlugins = new Set(['community-apps']);
966940
const timeoutError = new Error(
967941
'Timed out waiting for install operation plugin-op to finish'
@@ -970,13 +944,12 @@ describe('OnboardingSummaryStep', () => {
970944
};
971945
timeoutError.code = 'INSTALL_OPERATION_TIMEOUT';
972946
installPluginMock.mockRejectedValue(timeoutError);
973-
completeOnboardingMock.mockRejectedValue(new Error('offline'));
974947

975948
const { wrapper } = mountComponent();
976949
await clickApply(wrapper);
977950

978-
expect(wrapper.text()).toContain('Setup Saved in Best-Effort Mode');
979-
expect(wrapper.text()).not.toContain('Setup Continued After Timeout');
951+
expect(completeOnboardingMock).not.toHaveBeenCalled();
952+
expect(wrapper.text()).toContain('Setup Continued After Timeout');
980953
});
981954

982955
it('prefers timeout result over warning classification when completion succeeds', async () => {
@@ -1001,7 +974,6 @@ describe('OnboardingSummaryStep', () => {
1001974
it('shows completion dialog in offline mode and advances only after OK', async () => {
1002975
coreSettingsResult.value = null;
1003976
coreSettingsError.value = new Error('Graphql is offline.');
1004-
completeOnboardingMock.mockRejectedValue(new Error('offline'));
1005977

1006978
const { wrapper, onComplete } = mountComponent();
1007979
await clickApply(wrapper);

‎web/src/components/Onboarding/OnboardingModal.vue‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,12 @@ const currentStepProps = computed<Record<string, unknown>>(() => {
432432
showActivationCodeHint: hasActivationCode.value,
433433
};
434434
435+
case 'NEXT_STEPS':
436+
return {
437+
...baseProps,
438+
onComplete: () => closeModal({ reload: true }),
439+
};
440+
435441
default:
436442
return baseProps;
437443
}

0 commit comments

Comments
 (0)