Skip to content

Commit 27ceea3

Browse files
authored
šŸ› Fixed label mutation errors being silently swallowed in the label picker (#28510)
fixes https://linear.app/ghost/issue/BER-3493/label-createedit-mutation-errors-are-silently-swallowed - label create, edit and delete failures in the posts label picker gave no feedback: the client-side duplicate check only saw locally-loaded labels, rejections were silently swallowed, and renaming a label to an existing name returned an unhandled 500 instead of a validation error - the API is now the single source of truth for label name validation: unique constraint violations map to a 422 on both add and edit - error toasts never rendered in any shade-based admin app (Posts, members, Analytics) because the global error handler only emitted to the toast outlet mounted in Settings; it now routes each toast to whichever outlet the current screen has mounted, showing exactly one even when both exist - the picker maps API outcomes to user intent: edit failures render inline, creating a name that already exists adopts and selects the existing label, and deleting an already-gone label counts as deleted - review hardening: creation now selects the label against live selection state so a concurrent create cannot deselect it, and stays disabled until the duplicate-adoption lookup settles; adoption keys on the 422 status so permission errors surface instead of masquerading as success; NQL string escaping no longer doubles backslashes (NQL has no \\ escape) and is consolidated into one shared escapeNqlString helper; useFetchApi returns a stable callback
1 parent cb39915 commit 27ceea3

32 files changed

Lines changed: 1272 additions & 142 deletions

File tree

ā€Žapps/admin-x-design-system/src/providers/design-system-provider.tsxā€Ž

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ const DesignSystemProvider: React.FC<DesignSystemProviderProps> = ({fetchKoenigL
4545
return (
4646
<DesignSystemContext.Provider value={{isAnyTextFieldFocused, setFocusState, fetchKoenigLexical, darkMode}}>
4747
<GlobalDirtyStateProvider>
48-
<Toaster />
48+
<Toaster containerClassName="toast-outlet-react-hot-toast" />
4949
<NiceModal.Provider>
5050
{children}
5151
</NiceModal.Provider>

ā€Žapps/admin-x-framework/package.jsonā€Ž

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@
4141
"require": "./dist/utils/post-utils.cjs",
4242
"types": "./types/utils/post-utils.d.ts"
4343
},
44+
"./utils/nql": {
45+
"import": "./dist/utils/nql.js",
46+
"require": "./dist/utils/nql.cjs",
47+
"types": "./types/utils/nql.d.ts"
48+
},
4449
"./vite": {
4550
"import": "./dist/vite.js",
4651
"require": "./dist/vite.cjs",
@@ -107,6 +112,7 @@
107112
"react": "catalog:",
108113
"react-dom": "catalog:",
109114
"react-hot-toast": "catalog:",
115+
"sonner": "catalog:",
110116
"react-router": "catalog:"
111117
},
112118
"peerDependencies": {

ā€Žapps/admin-x-framework/src/api/labels.tsā€Ž

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import {InfiniteData} from '@tanstack/react-query';
22
import {Meta, createInfiniteQuery, createMutation, createQuery, createQueryWithId} from '../utils/api/hooks';
3+
import {apiUrl, useFetchApi} from '../utils/api/fetch-api';
4+
import {escapeNqlString} from '../utils/nql';
5+
import {useCallback} from 'react';
6+
import {useQueryClient} from '@tanstack/react-query';
37

48
export type Label = {
59
id: string;
@@ -61,6 +65,23 @@ export const useCreateLabel = createMutation<LabelsResponseType, Pick<Label, 'na
6165
}
6266
});
6367

68+
export const useFindLabelByName = () => {
69+
const fetchApi = useFetchApi();
70+
71+
return useCallback(async (name: string): Promise<Label | undefined> => {
72+
const data = await fetchApi<LabelsResponseType>(apiUrl('/labels/', {filter: `name:${escapeNqlString(name)}`, limit: '1'}));
73+
return data.labels?.[0];
74+
}, [fetchApi]);
75+
};
76+
77+
export const useInvalidateLabels = () => {
78+
const queryClient = useQueryClient();
79+
80+
return useCallback(() => {
81+
queryClient.invalidateQueries([dataType]);
82+
}, [queryClient]);
83+
};
84+
6485
export const useEditLabel = createMutation<LabelsResponseType, Pick<Label, 'id' | 'name'>>({
6586
method: 'PUT',
6687
path: label => `/labels/${label.id}/`,

ā€Žapps/admin-x-framework/src/hooks/use-filterable-api.tsā€Ž

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {useRef} from 'react';
22
import {apiUrl, useFetchApi} from '../utils/api/fetch-api';
33
import {Meta} from '../utils/api/hooks';
4+
import {escapeNqlString} from '../utils/nql';
45

56
const filterData = <
67
Data extends {[k in FilterKey]: string},
@@ -12,10 +13,6 @@ const filterData = <
1213
return data.filter(item => item[filterKey]?.toLowerCase().includes(input.toLowerCase()));
1314
};
1415

15-
const escapeNqlString = (value: string) => {
16-
return '\'' + value.replace(/'/g, '\\\'') + '\'';
17-
};
18-
1916
const useFilterableApi = <
2017
Data extends {id: string} & {[k in FilterKey]: string} & {[k: string]: unknown},
2118
ResponseKey extends string = string,

ā€Žapps/admin-x-framework/src/hooks/use-handle-error.tsā€Ž

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,32 @@
11
import * as Sentry from '@sentry/react';
22
import {showToast} from '@tryghost/admin-x-design-system';
3+
import {toast as sonnerToast} from 'sonner';
34
import {useCallback} from 'react';
45
import toast from 'react-hot-toast';
56
import {useFramework} from '../providers/framework-provider';
6-
import {APIError, ValidationError} from '../utils/errors';
7+
import {APIError, getErrorMessage} from '../utils/errors';
8+
9+
// Stale toasts can cover UI and block clicks, especially in tests
10+
function dismissToasts() {
11+
toast.remove();
12+
sonnerToast.dismiss();
13+
}
14+
15+
// There are two toast outlets: admin-x-design-system's DesignSystemProvider
16+
// renders react-hot-toast (marked with this class), shade's ShadeProvider
17+
// renders sonner - and the React shell can mount both at once. The marker's
18+
// presence in the DOM picks the library to emit to.
19+
function showErrorToast(message: React.ReactNode) {
20+
dismissToasts();
21+
if (document.querySelector('.toast-outlet-react-hot-toast')) {
22+
showToast({
23+
message,
24+
type: 'error'
25+
});
26+
} else {
27+
sonnerToast.error(message);
28+
}
29+
}
730

831
/**
932
* Generic error handling for API calls. This is enabled by default for queries (can be disabled by
@@ -38,26 +61,15 @@ const useHandleError = () => {
3861
return;
3962
}
4063

41-
toast.remove();
42-
4364
if (error instanceof APIError && error.response?.status === 418) {
4465
// We use this status in tests to indicate the API request was not mocked -
45-
// don't show a toast because it may block clicking things in the test
46-
} else if (error instanceof ValidationError && error.data?.errors[0]) {
47-
showToast({
48-
message: error.data.errors[0].context || error.data.errors[0].message,
49-
type: 'error'
50-
});
66+
// don't show a toast because it may block clicking things in the test,
67+
// but still clear lingering toasts that would block clicks the same way
68+
dismissToasts();
5169
} else if (error instanceof APIError) {
52-
showToast({
53-
message: error.message,
54-
type: 'error'
55-
});
70+
showErrorToast(getErrorMessage(error, error.message));
5671
} else {
57-
showToast({
58-
message: 'Something went wrong, please try again.',
59-
type: 'error'
60-
});
72+
showErrorToast('Something went wrong, please try again.');
6173
}
6274
}, [sentryDSN]);
6375

ā€Žapps/admin-x-framework/src/utils/api/fetch-api.tsā€Ž

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import * as Sentry from '@sentry/react';
2+
import {useCallback} from 'react';
23
import {useFramework} from '../../providers/framework-provider';
34
import {APIError, MaintenanceError, ServerUnreachableError, TimeoutError} from '../errors';
45
import {getGhostPaths} from '../helpers';
@@ -120,8 +121,9 @@ const fetchWithXhr = (
120121
export const useFetchApi = () => {
121122
const {ghostVersion, sentryDSN} = useFramework();
122123

124+
// Memoized so hooks that depend on fetchApi can cache
123125
// eslint-disable-next-line @typescript-eslint/no-explicit-any
124-
return async <ResponseData = any>(
126+
return useCallback(async <ResponseData = any>(
125127
endpoint: string | URL,
126128
{
127129
method = 'GET',
@@ -222,7 +224,7 @@ export const useFetchApi = () => {
222224
// this can't happen, but TS isn't smart enough to undeerstand that the loop will never exit without an error or return
223225
// because of retry + attempts usage combination
224226
return undefined as never;
225-
};
227+
}, [ghostVersion, sentryDSN]);
226228
};
227229

228230
const {apiRoot, activityPubRoot} = getGhostPaths();

ā€Žapps/admin-x-framework/src/utils/errors.tsā€Ž

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,16 @@ export class ValidationError extends JSONError {
110110

111111
export const errorsWithMessage = [ValidationError, ThemeValidationError, HostLimitError, EmailError];
112112

113+
// The API error serializer puts the human-readable text in `context`;
114+
// `message` is only a generic summary
115+
export function getErrorMessage(error: unknown, fallback: string): string {
116+
if (error instanceof ValidationError && error.data?.errors[0]) {
117+
return error.data.errors[0].context || error.data.errors[0].message;
118+
}
119+
120+
return fallback;
121+
}
122+
113123
// Frontend errors
114124

115125
export class AlreadyExistsError extends Error {
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// NQL's only escape sequences are \' and \" - there is no \\, so a backslash
2+
// is a literal character and doubling it would query a different value.
3+
// Escaping just quotes is still injection-safe: because \\ is never an escape
4+
// pair, the backslash emitted before a quote always parses as the \' escape,
5+
// so no input (including trailing or adjacent backslashes) can turn an
6+
// escaped quote back into a string terminator.
7+
export function escapeNqlString(value: string): string {
8+
return `'${value.replace(/'/g, '\\\'')}'`;
9+
}
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
import {act} from '@testing-library/react';
2+
import {beforeEach, describe, expect, it, vi} from 'vitest';
3+
import {ValidationError} from '../../../src/utils/errors';
4+
import {renderHookWithProviders} from '../../../src/test/test-utils';
5+
import {useCreateLabel, useEditLabel, useFindLabelByName} from '../../../src/api/labels';
6+
import {withMockFetch} from '../../utils/mock-fetch';
7+
8+
const {mockShowToast, mockSonnerError} = vi.hoisted(() => ({
9+
mockShowToast: vi.fn(),
10+
mockSonnerError: vi.fn()
11+
}));
12+
13+
vi.mock('@tryghost/admin-x-design-system', async (importOriginal) => {
14+
const original = await importOriginal<typeof import('@tryghost/admin-x-design-system')>();
15+
return {...original, showToast: mockShowToast};
16+
});
17+
18+
vi.mock('sonner', () => ({
19+
toast: {
20+
error: mockSonnerError,
21+
dismiss: vi.fn()
22+
}
23+
}));
24+
25+
const duplicateLabelResponse = {
26+
errors: [{
27+
code: 'VALIDATION',
28+
context: 'Label already exists',
29+
details: null,
30+
ghostErrorCode: null,
31+
help: null,
32+
id: 'label-error-id',
33+
message: 'Validation error, cannot save label.',
34+
property: null,
35+
type: 'ValidationError'
36+
}]
37+
};
38+
39+
const existingLabel = {
40+
id: 'label-1',
41+
name: 'Existing label',
42+
slug: 'existing-label',
43+
created_at: '',
44+
updated_at: ''
45+
};
46+
47+
const mockErrorFetch = {
48+
json: duplicateLabelResponse,
49+
headers: {'content-type': 'application/json'},
50+
ok: false,
51+
status: 422
52+
};
53+
54+
describe('labels api', () => {
55+
beforeEach(() => {
56+
vi.clearAllMocks();
57+
});
58+
59+
it('rejects duplicate creates without reporting - the picker owns recovery', async () => {
60+
await withMockFetch(mockErrorFetch, async () => {
61+
const {result} = renderHookWithProviders(() => useCreateLabel());
62+
63+
await act(async () => {
64+
await expect(result.current.mutateAsync({name: 'Existing label'})).rejects.toBeInstanceOf(ValidationError);
65+
});
66+
67+
expect(mockShowToast).not.toHaveBeenCalled();
68+
expect(mockSonnerError).not.toHaveBeenCalled();
69+
});
70+
});
71+
72+
it('finds a label by name', async () => {
73+
await withMockFetch({
74+
json: {labels: [existingLabel]},
75+
headers: {'content-type': 'application/json'},
76+
ok: true,
77+
status: 200
78+
}, async (mock) => {
79+
const {result} = renderHookWithProviders(() => useFindLabelByName());
80+
81+
await act(async () => {
82+
await expect(result.current(`O'Existing label`)).resolves.toEqual(existingLabel);
83+
});
84+
85+
const url = new URL(mock.calls[0][0] as string);
86+
expect(url.searchParams.get('filter')).toBe(String.raw`name:'O\'Existing label'`);
87+
expect(url.searchParams.get('limit')).toBe('1');
88+
});
89+
});
90+
91+
it('does not escape backslashes in the lookup filter', async () => {
92+
await withMockFetch({
93+
json: {labels: []},
94+
headers: {'content-type': 'application/json'},
95+
ok: true,
96+
status: 200
97+
}, async (mock) => {
98+
const {result} = renderHookWithProviders(() => useFindLabelByName());
99+
100+
await act(async () => {
101+
await result.current('trailing\\');
102+
});
103+
104+
// NQL keeps lone backslashes literal and only unescapes \' and \",
105+
// so a doubled backslash would query a two-backslash name
106+
const url = new URL(mock.calls[0][0] as string);
107+
expect(url.searchParams.get('filter')).toBe(String.raw`name:'trailing\'`);
108+
});
109+
});
110+
111+
it('resolves undefined when no label matches the name', async () => {
112+
await withMockFetch({
113+
json: {labels: []},
114+
headers: {'content-type': 'application/json'},
115+
ok: true,
116+
status: 200
117+
}, async () => {
118+
const {result} = renderHookWithProviders(() => useFindLabelByName());
119+
120+
await act(async () => {
121+
await expect(result.current('Missing label')).resolves.toBeUndefined();
122+
});
123+
});
124+
});
125+
126+
it('rejects duplicate edits without reporting - the edit row surfaces them', async () => {
127+
await withMockFetch(mockErrorFetch, async () => {
128+
const {result} = renderHookWithProviders(() => useEditLabel());
129+
130+
await act(async () => {
131+
await expect(result.current.mutateAsync({id: 'label-1', name: 'Existing label'})).rejects.toBeInstanceOf(ValidationError);
132+
});
133+
134+
expect(mockShowToast).not.toHaveBeenCalled();
135+
expect(mockSonnerError).not.toHaveBeenCalled();
136+
});
137+
});
138+
});

0 commit comments

Comments
Ā (0)