Skip to content

Commit 8b155d1

Browse files
authored
fix: handle race condition between guid loading and license check (#1847)
On errors, a `console.error` message should be emitted from the browser console, tagged `[ReplaceCheck.check]`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added retry capability for license eligibility checks with a contextual "Retry" button that appears in error states. * **Bug Fixes** * Fixed license status initialization to correctly default to ready state. * Enhanced error messaging with specific messages for different failure scenarios (missing credentials, access denied, server errors). * Improved status display handling to prevent potential runtime errors. * **Localization** * Added "Retry" text translation. * **Tests** * Updated and added tests for reset functionality and error handling. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent d13a1f6 commit 8b155d1

File tree

4 files changed

+118
-14
lines changed

4 files changed

+118
-14
lines changed

web/__test__/store/replaceRenew.test.ts

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ import { useServerStore } from '~/store/server';
1414

1515
vi.mock('@unraid/shared-callbacks', () => ({}));
1616

17+
vi.mock('@unraid/ui', () => ({
18+
BrandLoading: {},
19+
}));
20+
1721
vi.mock('~/composables/services/keyServer', () => ({
1822
validateGuid: vi.fn(),
1923
}));
@@ -62,7 +66,7 @@ describe('ReplaceRenew Store', () => {
6266
expect(store.replaceStatus).toBe('ready');
6367
});
6468

65-
it('should initialize with error state when guid is missing', () => {
69+
it('should initialize with ready state even when guid is missing', () => {
6670
vi.mocked(useServerStore).mockReturnValueOnce({
6771
guid: undefined,
6872
keyfile: mockKeyfile,
@@ -72,7 +76,8 @@ describe('ReplaceRenew Store', () => {
7276

7377
const newStore = useReplaceRenewStore();
7478

75-
expect(newStore.replaceStatus).toBe('error');
79+
// Store now always initializes as 'ready' - errors are set when check() is called
80+
expect(newStore.replaceStatus).toBe('ready');
7681
});
7782
});
7883

@@ -138,6 +143,18 @@ describe('ReplaceRenew Store', () => {
138143
expect(store.renewStatus).toBe('installing');
139144
});
140145

146+
it('should reset all states with reset action', () => {
147+
store.setReplaceStatus('error');
148+
store.keyLinkedStatus = 'error';
149+
store.error = { name: 'Error', message: 'Test error' };
150+
151+
store.reset();
152+
153+
expect(store.replaceStatus).toBe('ready');
154+
expect(store.keyLinkedStatus).toBe('ready');
155+
expect(store.error).toBeNull();
156+
});
157+
141158
describe('check action', () => {
142159
const mockResponse = {
143160
hasNewerKeyfile: false,
@@ -326,8 +343,59 @@ describe('ReplaceRenew Store', () => {
326343
await store.check();
327344

328345
expect(store.replaceStatus).toBe('error');
346+
expect(store.keyLinkedStatus).toBe('error');
329347
expect(console.error).toHaveBeenCalledWith('[ReplaceCheck.check]', testError);
330-
expect(store.error).toEqual(testError);
348+
expect(store.error).toEqual({ name: 'Error', message: 'Test error' });
349+
});
350+
351+
it('should set error when guid is missing during check', async () => {
352+
vi.mocked(useServerStore).mockReturnValue({
353+
guid: '',
354+
keyfile: mockKeyfile,
355+
} as unknown as ReturnType<typeof useServerStore>);
356+
357+
setActivePinia(createPinia());
358+
const testStore = useReplaceRenewStore();
359+
360+
await testStore.check();
361+
362+
expect(testStore.replaceStatus).toBe('error');
363+
expect(testStore.keyLinkedStatus).toBe('error');
364+
expect(testStore.error?.message).toBe('Flash GUID required to check replacement status');
365+
});
366+
367+
it('should set error when keyfile is missing during check', async () => {
368+
vi.mocked(useServerStore).mockReturnValue({
369+
guid: mockGuid,
370+
keyfile: '',
371+
} as unknown as ReturnType<typeof useServerStore>);
372+
373+
setActivePinia(createPinia());
374+
const testStore = useReplaceRenewStore();
375+
376+
await testStore.check();
377+
378+
expect(testStore.replaceStatus).toBe('error');
379+
expect(testStore.keyLinkedStatus).toBe('error');
380+
expect(testStore.error?.message).toBe('Keyfile required to check replacement status');
381+
});
382+
383+
it('should provide descriptive error for 403 status', async () => {
384+
const error403 = { response: { status: 403 }, message: 'Forbidden' };
385+
vi.mocked(validateGuid).mockRejectedValueOnce(error403);
386+
387+
await store.check();
388+
389+
expect(store.error?.message).toBe('Access denied - license may be linked to another account');
390+
});
391+
392+
it('should provide descriptive error for 500+ status', async () => {
393+
const error500 = { response: { status: 500 }, message: 'Server Error' };
394+
vi.mocked(validateGuid).mockRejectedValueOnce(error500);
395+
396+
await store.check();
397+
398+
expect(store.error?.message).toBe('Key server temporarily unavailable - please try again later');
331399
});
332400
});
333401
});

web/src/components/Registration/ReplaceCheck.vue

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
<script setup lang="ts">
2+
import { computed } from 'vue';
23
import { useI18n } from 'vue-i18n';
34
import { storeToRefs } from 'pinia';
45
5-
import { ArrowTopRightOnSquareIcon, KeyIcon } from '@heroicons/vue/24/solid';
6+
import { ArrowPathIcon, ArrowTopRightOnSquareIcon, KeyIcon } from '@heroicons/vue/24/solid';
67
import { Badge, BrandButton } from '@unraid/ui';
78
import { DOCS_REGISTRATION_REPLACE_KEY } from '~/helpers/urls';
89
@@ -11,20 +12,30 @@ import { useReplaceRenewStore } from '~/store/replaceRenew';
1112
const { t } = useI18n();
1213
const replaceRenewStore = useReplaceRenewStore();
1314
const { replaceStatusOutput } = storeToRefs(replaceRenewStore);
15+
16+
const isError = computed(() => replaceStatusOutput.value?.variant === 'red');
17+
const showButton = computed(() => !replaceStatusOutput.value || isError.value);
18+
19+
const handleCheck = () => {
20+
if (isError.value) {
21+
replaceRenewStore.reset();
22+
}
23+
replaceRenewStore.check(true);
24+
};
1425
</script>
1526

1627
<template>
1728
<div class="flex flex-wrap items-center justify-between gap-2">
1829
<BrandButton
19-
v-if="!replaceStatusOutput"
20-
:icon="KeyIcon"
21-
:text="t('registration.replaceCheck.checkEligibility')"
30+
v-if="showButton"
31+
:icon="isError ? ArrowPathIcon : KeyIcon"
32+
:text="isError ? t('common.retry') : t('registration.replaceCheck.checkEligibility')"
2233
class="grow"
23-
@click="replaceRenewStore.check"
34+
@click="handleCheck"
2435
/>
2536

26-
<Badge v-else :variant="replaceStatusOutput.variant" :icon="replaceStatusOutput.icon" size="md">
27-
{{ t(replaceStatusOutput.text ?? 'Unknown') }}
37+
<Badge v-else :variant="replaceStatusOutput?.variant" :icon="replaceStatusOutput?.icon" size="md">
38+
{{ t(replaceStatusOutput?.text ?? 'Unknown') }}
2839
</Badge>
2940

3041
<span class="inline-flex flex-wrap items-center justify-end gap-2">

web/src/locales/en.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
"common.installed": "Installed",
3131
"common.installing": "Installing",
3232
"common.learnMore": "Learn More",
33+
"common.retry": "Retry",
3334
"common.loading2": "Loading…",
3435
"common.success": "Success!",
3536
"common.unknown": "Unknown",

web/src/store/replaceRenew.ts

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,7 @@ export const useReplaceRenewStore = defineStore('replaceRenewCheck', () => {
9595
renewStatus.value = status;
9696
};
9797

98-
const replaceStatus = ref<'checking' | 'eligible' | 'error' | 'ineligible' | 'ready'>(
99-
guid.value ? 'ready' : 'error'
100-
);
98+
const replaceStatus = ref<'checking' | 'eligible' | 'error' | 'ineligible' | 'ready'>('ready');
10199
const setReplaceStatus = (status: typeof replaceStatus.value) => {
102100
replaceStatus.value = status;
103101
};
@@ -169,11 +167,15 @@ export const useReplaceRenewStore = defineStore('replaceRenewCheck', () => {
169167
const check = async (skipCache: boolean = false) => {
170168
if (!guid.value) {
171169
setReplaceStatus('error');
170+
setKeyLinked('error');
172171
error.value = { name: 'Error', message: 'Flash GUID required to check replacement status' };
172+
return;
173173
}
174174
if (!keyfile.value) {
175175
setReplaceStatus('error');
176+
setKeyLinked('error');
176177
error.value = { name: 'Error', message: 'Keyfile required to check replacement status' };
178+
return;
177179
}
178180

179181
try {
@@ -240,11 +242,32 @@ export const useReplaceRenewStore = defineStore('replaceRenewCheck', () => {
240242
} catch (err) {
241243
const catchError = err as WretchError;
242244
setReplaceStatus('error');
243-
error.value = catchError?.message ? catchError : { name: 'Error', message: 'Unknown error' };
245+
setKeyLinked('error');
246+
247+
let errorMessage = 'Unknown error';
248+
if (catchError?.response?.status === 401) {
249+
errorMessage = 'Authentication failed - please sign in again';
250+
} else if (catchError?.response?.status === 403) {
251+
errorMessage = 'Access denied - license may be linked to another account';
252+
} else if (catchError?.response?.status && catchError.response.status >= 500) {
253+
errorMessage = 'Key server temporarily unavailable - please try again later';
254+
} else if (catchError?.message) {
255+
errorMessage = catchError.message;
256+
} else if (typeof navigator !== 'undefined' && !navigator.onLine) {
257+
errorMessage = 'No internet connection';
258+
}
259+
260+
error.value = { name: 'Error', message: errorMessage };
244261
console.error('[ReplaceCheck.check]', catchError);
245262
}
246263
};
247264

265+
const reset = () => {
266+
replaceStatus.value = 'ready';
267+
keyLinkedStatus.value = 'ready';
268+
error.value = null;
269+
};
270+
248271
return {
249272
// state
250273
keyLinkedStatus,
@@ -255,6 +278,7 @@ export const useReplaceRenewStore = defineStore('replaceRenewCheck', () => {
255278
// actions
256279
check,
257280
purgeValidationResponse,
281+
reset,
258282
setReplaceStatus,
259283
setRenewStatus,
260284
error,

0 commit comments

Comments
 (0)