Skip to content

Commit 14f70fd

Browse files
christianalfoniCompuIves
christianalfoni
authored and
CompuIves
committed
Improve error handling (codesandbox#3089)
* Improve error handling * use title and message for custom and specific error message combined * Update packages/app/src/app/overmind/effects/executor.ts * Update packages/app/src/app/overmind/factories.ts Co-Authored-By: Ives van Hoorne <Ives.v.h@gmail.com> * Update packages/app/src/app/overmind/internalActions.ts Co-Authored-By: Ives van Hoorne <Ives.v.h@gmail.com> * Update packages/app/src/app/overmind/namespaces/editor/actions.ts Co-Authored-By: Ives van Hoorne <Ives.v.h@gmail.com> * Update packages/app/src/app/overmind/namespaces/editor/internalActions.ts Co-Authored-By: Ives van Hoorne <Ives.v.h@gmail.com> * Update packages/app/src/app/overmind/namespaces/explore/actions.ts Co-Authored-By: Ives van Hoorne <Ives.v.h@gmail.com> * Update packages/app/src/app/overmind/namespaces/explore/actions.ts Co-Authored-By: Ives van Hoorne <Ives.v.h@gmail.com> * Apply suggestions from code review * Apply suggestions from code review * Update packages/app/src/app/overmind/effects/executor.ts * Apply suggestions from code review * add sandbox info error handling * fix comment
1 parent 2b50894 commit 14f70fd

File tree

13 files changed

+309
-239
lines changed

13 files changed

+309
-239
lines changed

‎packages/app/src/app/overmind/actions.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,10 @@ export const signInZeitClicked: AsyncAction = async ({
149149
state.user = await api.createZeitIntegration(data.code);
150150
await actions.deployment.internal.getZeitUserDetails();
151151
} catch (error) {
152-
notificationToast.error('Could not authorize with ZEIT');
152+
actions.internal.handleError({
153+
message: 'Could not authorize with ZEIT',
154+
error,
155+
});
153156
}
154157
} else {
155158
notificationToast.error('Could not authorize with ZEIT');

‎packages/app/src/app/overmind/effects/api/apiFactory.ts

Lines changed: 6 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ export type Api = {
2828
export type ApiConfig = {
2929
provideJwtToken: () => string;
3030
getParsedConfigurations: () => any;
31-
onError: (error: ApiError) => void;
3231
};
3332

3433
export default (config: ApiConfig) => {
@@ -46,56 +45,36 @@ export default (config: ApiConfig) => {
4645
params,
4746
headers: createHeaders(config.provideJwtToken()),
4847
})
49-
.then(response => handleResponse(response, options))
50-
.catch(e => {
51-
config.onError(e);
52-
return Promise.reject(e);
53-
});
48+
.then(response => handleResponse(response, options));
5449
},
5550
post(path, body, options) {
5651
return axios
5752
.post(API_ROOT + path, decamelizeKeys(body), {
5853
headers: createHeaders(config.provideJwtToken()),
5954
})
60-
.then(response => handleResponse(response, options))
61-
.catch(e => {
62-
config.onError(e);
63-
return Promise.reject(e);
64-
});
55+
.then(response => handleResponse(response, options));
6556
},
6657
patch(path, body, options) {
6758
return axios
6859
.patch(API_ROOT + path, decamelizeKeys(body), {
6960
headers: createHeaders(config.provideJwtToken()),
7061
})
71-
.then(response => handleResponse(response, options))
72-
.catch(e => {
73-
config.onError(e);
74-
return Promise.reject(e);
75-
});
62+
.then(response => handleResponse(response, options));
7663
},
7764
put(path, body, options) {
7865
return axios
7966
.put(API_ROOT + path, decamelizeKeys(body), {
8067
headers: createHeaders(config.provideJwtToken()),
8168
})
82-
.then(response => handleResponse(response, options))
83-
.catch(e => {
84-
config.onError(e);
85-
return Promise.reject(e);
86-
});
69+
.then(response => handleResponse(response, options));
8770
},
8871
delete(path, params, options) {
8972
return axios
9073
.delete(API_ROOT + path, {
9174
params,
9275
headers: createHeaders(config.provideJwtToken()),
9376
})
94-
.then(response => handleResponse(response, options))
95-
.catch(e => {
96-
config.onError(e);
97-
return Promise.reject(e);
98-
});
77+
.then(response => handleResponse(response, options));
9978
},
10079
request(requestConfig, options) {
10180
return axios
@@ -106,11 +85,7 @@ export default (config: ApiConfig) => {
10685
headers: createHeaders(config.provideJwtToken()),
10786
})
10887
)
109-
.then(response => handleResponse(response, options))
110-
.catch(e => {
111-
config.onError(e);
112-
return Promise.reject(e);
113-
});
88+
.then(response => handleResponse(response, options));
11489
},
11590
};
11691

‎packages/app/src/app/overmind/effects/live/index.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -192,11 +192,6 @@ export default {
192192
clients.get(moduleShortid).applyClient(operation);
193193
} catch (e) {
194194
// Something went wrong, probably a sync mismatch. Request new version
195-
console.error(
196-
'Something went wrong with applying OT operation',
197-
moduleShortid,
198-
operation
199-
);
200195
this.send('live:module_state', {});
201196
}
202197
},

‎packages/app/src/app/overmind/factories.ts

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
11
import { Contributor } from '@codesandbox/common/lib/types';
2-
import { notificationState } from '@codesandbox/common/lib/utils/notifications';
3-
import { NotificationStatus } from '@codesandbox/notifications';
4-
import { AxiosError } from 'axios';
52
import { IDerive, IState, json } from 'overmind';
63

74
import { AsyncAction } from '.';
@@ -41,31 +38,10 @@ export const withLoadApp = <T>(
4138
actions.userNotifications.internal.initialize();
4239
effects.api.preloadTemplates();
4340
} catch (error) {
44-
if (error.isAxiosError && (error as AxiosError).response.status === 401) {
45-
// Reset existing sign in info
46-
effects.jwt.reset();
47-
effects.analytics.setAnonymousId();
48-
49-
// Allow user to sign in again in notification
50-
notificationState.addNotification({
51-
message: 'Your session seems to be expired, please log in again...',
52-
status: NotificationStatus.ERROR,
53-
actions: {
54-
primary: [
55-
{
56-
label: 'Sign in',
57-
run: () => {
58-
actions.signInClicked({ useExtraScopes: false });
59-
},
60-
},
61-
],
62-
},
63-
});
64-
} else {
65-
effects.notificationToast.error(
66-
"We weren't able to sign you in, this could be due to a flaky connection or something on our server. Please try again in a minute."
67-
);
68-
}
41+
actions.internal.handleError({
42+
message: 'We had trouble with signing you in',
43+
error,
44+
});
6945
}
7046
} else {
7147
effects.jwt.reset();

‎packages/app/src/app/overmind/internalActions.ts

Lines changed: 94 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@ export const signIn: AsyncAction<{ useExtraScopes?: boolean }> = async (
3737
actions.userNotifications.internal.initialize(); // Seemed a bit different originally?
3838
actions.refetchSandboxInfo();
3939
} catch (error) {
40-
actions.internal.addNotification({
41-
title: 'Github Authentication Error',
42-
type: 'error',
40+
actions.internal.handleError({
41+
message: 'Could not authenticate with Github',
42+
error,
4343
});
4444
}
4545
};
@@ -206,10 +206,11 @@ export const setCurrentSandbox: AsyncAction<Sandbox> = async (
206206
currentModuleShortid = resolvedModule
207207
? resolvedModule.shortid
208208
: currentModuleShortid;
209-
} catch (err) {
210-
effects.notificationToast.warning(
211-
`Could not find the module ${sandboxOptions.currentModule}`
212-
);
209+
} catch (error) {
210+
actions.internal.handleError({
211+
message: `Could not find module ${sandboxOptions.currentModule}`,
212+
error,
213+
});
213214
}
214215
}
215216

@@ -275,7 +276,11 @@ export const updateCurrentSandbox: AsyncAction<Sandbox> = async (
275276
state.editor.currentSandbox.title = sandbox.title;
276277
};
277278

278-
export const ensurePackageJSON: AsyncAction = async ({ state, effects }) => {
279+
export const ensurePackageJSON: AsyncAction = async ({
280+
state,
281+
actions,
282+
effects,
283+
}) => {
279284
const sandbox = state.editor.currentSandbox;
280285
const existingPackageJson = sandbox.modules.find(
281286
module => module.directoryShortid == null && module.title === 'package.json'
@@ -310,6 +315,10 @@ export const ensurePackageJSON: AsyncAction = async ({ state, effects }) => {
310315
} catch (error) {
311316
sandbox.modules.splice(sandbox.modules.indexOf(module), 1);
312317
state.editor.modulesByPath = effects.vscode.sandboxFsSync.create(sandbox);
318+
actions.internal.handleError({
319+
message: 'Could not add package.json file',
320+
error,
321+
});
313322
}
314323
}
315324
};
@@ -330,21 +339,55 @@ export const closeTabByIndex: Action<number> = ({ state }, tabIndex) => {
330339
state.editor.tabs.splice(tabIndex, 1);
331340
};
332341

333-
export const onApiError: Action<ApiError> = (
334-
{ state, actions, effects },
335-
error
336-
) => {
337-
const { response } = error;
342+
export const handleError: Action<{
343+
/*
344+
The message that will show as title of the notification
345+
*/
346+
message: string;
347+
error: ApiError | Error;
348+
}> = ({ actions, effects }, { message, error }) => {
349+
const isGenericError = !('response' in error) || error.response.status >= 500;
338350

339-
if (response.status === 401) {
340-
// We need to implement a blocking modal to either sign in or refresh the browser to
341-
// continue in an anonymous state
351+
if (isGenericError) {
352+
effects.analytics.logError(error);
353+
effects.notificationToast.add({
354+
title: message,
355+
message: error.message,
356+
status: NotificationStatus.ERROR,
357+
});
358+
359+
return;
342360
}
343361

344-
if (!response || response.status >= 500) {
345-
effects.analytics.logError(error);
362+
const { response } = error as ApiError;
363+
364+
if (response.status === 401) {
365+
// Reset existing sign in info
366+
effects.jwt.reset();
367+
effects.analytics.setAnonymousId();
368+
369+
// Allow user to sign in again in notification
370+
effects.notificationToast.add({
371+
message: 'Your session seems to be expired, please log in again...',
372+
status: NotificationStatus.ERROR,
373+
actions: {
374+
primary: [
375+
{
376+
label: 'Sign in',
377+
run: () => {
378+
actions.signInClicked({ useExtraScopes: false });
379+
},
380+
},
381+
],
382+
},
383+
});
384+
385+
return;
346386
}
347387

388+
/*
389+
Update error message with what is coming from the server
390+
*/
348391
const result = response.data;
349392

350393
if (result) {
@@ -367,43 +410,31 @@ export const onApiError: Action<ApiError> = (
367410
}
368411
}
369412

413+
const notificationActions = {
414+
primary: [],
415+
};
416+
370417
if (error.message.startsWith('You need to sign in to create more than')) {
371418
// Error for "You need to sign in to create more than 10 sandboxes"
372419
effects.analytics.track('Anonymous Sandbox Limit Reached', {
373420
errorMessage: error.message,
374421
});
375422

376-
effects.notificationToast.add({
377-
message: error.message,
378-
status: NotificationStatus.ERROR,
379-
actions: {
380-
primary: [
381-
{
382-
label: 'Sign in',
383-
run: () => {
384-
actions.internal.signIn({});
385-
},
386-
},
387-
],
423+
notificationActions.primary.push({
424+
label: 'Sign in',
425+
run: () => {
426+
actions.internal.signIn({});
388427
},
389428
});
390429
} else if (error.message.startsWith('You reached the maximum of')) {
391430
effects.analytics.track('Non-Patron Sandbox Limit Reached', {
392431
errorMessage: error.message,
393432
});
394433

395-
effects.notificationToast.add({
396-
message: error.message,
397-
status: NotificationStatus.ERROR,
398-
actions: {
399-
primary: [
400-
{
401-
label: 'Open Patron Page',
402-
run: () => {
403-
window.open(patronUrl(), '_blank');
404-
},
405-
},
406-
],
434+
notificationActions.primary.push({
435+
label: 'Open Patron Page',
436+
run: () => {
437+
window.open(patronUrl(), '_blank');
407438
},
408439
});
409440
} else if (
@@ -415,31 +446,28 @@ export const onApiError: Action<ApiError> = (
415446
errorMessage: error.message,
416447
});
417448

418-
effects.notificationToast.add({
419-
message: error.message,
420-
status: NotificationStatus.ERROR,
421-
actions: {
422-
primary: [
423-
{
424-
label: 'Open Patron Page',
425-
run: () => {
426-
window.open(patronUrl(), '_blank');
427-
},
428-
},
429-
],
449+
notificationActions.primary.push({
450+
label: 'Open Patron Page',
451+
run: () => {
452+
window.open(patronUrl(), '_blank');
430453
},
431454
});
432-
} else {
433-
if (
434-
error.message.startsWith(
435-
'You reached the limit of server sandboxes, we will increase the limit in the future. Please contact hello@codesandbox.io for more server sandboxes.'
436-
)
437-
) {
438-
effects.analytics.track('Patron Server Sandbox Limit Reached', {
439-
errorMessage: error.message,
440-
});
441-
}
442-
443-
effects.notificationToast.error(error.message);
455+
} else if (
456+
error.message.startsWith(
457+
'You reached the limit of server sandboxes, we will increase the limit in the future. Please contact hello@codesandbox.io for more server sandboxes.'
458+
)
459+
) {
460+
effects.analytics.track('Patron Server Sandbox Limit Reached', {
461+
errorMessage: error.message,
462+
});
444463
}
464+
465+
effects.notificationToast.add({
466+
title: message,
467+
message: error.message,
468+
status: NotificationStatus.ERROR,
469+
...(notificationActions.primary.length
470+
? { actions: notificationActions }
471+
: {}),
472+
});
445473
};

0 commit comments

Comments
 (0)