fitzee commented on code in PR #45524:
URL: https://github.com/apache/superset/pull/45524#discussion_r4330827191
##########
superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx:
##########
@@ -610,47 +623,65 @@ export default function AddSemanticViewModal({
configuration: runtimeData,
}));
- const { json } = await SupersetClient.post({
- endpoint: '/api/v1/semantic_view/',
- jsonPayload: { views: viewsToCreate },
- });
-
- const created = Array.isArray(json?.result?.created)
- ? json.result.created
- : [];
- const errors = Array.isArray(json?.result?.errors)
- ? json.result.errors
- : [];
+ let body: unknown;
+ try {
+ const { json } = await SupersetClient.post({
+ endpoint: '/api/v1/semantic_view/',
+ jsonPayload: { views: viewsToCreate },
+ });
+ body = json;
+ } catch (error) {
+ // SupersetClient rejects non-2xx responses before parsing their body.
+ if (
+ !isRecord(error) ||
+ error.status !== 422 ||
+ typeof error.json !== 'function'
+ ) {
+ throw error;
+ }
+ body = await error.json();
+ }
+ if (gen !== fetchGenRef.current) return;
Review Comment:
non-blocking: Cancel/X stay enabled during save (`StandardModal` only
disables the primary button), and closing bumps `fetchGenRef`, so this early
return drops a response whose views were already committed server-side. Before
this change the base code still called `onSuccess()`/toast after close; now the
dataset list is not refreshed and the user gets no confirmation that anything
was created (the new "closed and reopened" test asserts exactly this). Suggest
skipping only the local state updates on a stale generation, but still calling
`onSuccess()` (and the success toast) when `created.length > 0`.
##########
superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx:
##########
@@ -726,12 +757,32 @@ export default function AddSemanticViewModal({
saveLoading={saving}
>
<ModalContent>
+ {creationErrors.length > 0 && (
Review Comment:
non-blocking: `creationErrors` is cleared on layer change, close, and save,
but not in `handleRuntimeFormChange`. After a partial failure, changing the
source location (e.g. a different schema) replaces the views list while the
alert still reports failures for views from the previous configuration.
Clearing it when runtime data changes (or when the views list is refetched)
avoids stale errors.
##########
superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx:
##########
@@ -726,12 +757,32 @@ export default function AddSemanticViewModal({
saveLoading={saving}
>
<ModalContent>
+ {creationErrors.length > 0 && (
+ <Alert
+ type="error"
+ showIcon
+ message={t('Some semantic views could not be added')}
+ description={
+ <List
Review Comment:
nit: no height bound. With a large batch failing (e.g. layer unreachable,
100 selected views) the alert pushes the form far down the modal, and the toast
concatenates all 100 names. A `max-height` + `overflow-y: auto` on the list,
and truncating the toast name list ("a, b, c and 97 more"), would keep it
usable.
##########
superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx:
##########
@@ -610,47 +623,65 @@ export default function AddSemanticViewModal({
configuration: runtimeData,
}));
- const { json } = await SupersetClient.post({
- endpoint: '/api/v1/semantic_view/',
- jsonPayload: { views: viewsToCreate },
- });
-
- const created = Array.isArray(json?.result?.created)
- ? json.result.created
- : [];
- const errors = Array.isArray(json?.result?.errors)
- ? json.result.errors
- : [];
+ let body: unknown;
+ try {
+ const { json } = await SupersetClient.post({
+ endpoint: '/api/v1/semantic_view/',
+ jsonPayload: { views: viewsToCreate },
+ });
+ body = json;
+ } catch (error) {
+ // SupersetClient rejects non-2xx responses before parsing their body.
+ if (
+ !isRecord(error) ||
+ error.status !== 422 ||
+ typeof error.json !== 'function'
+ ) {
+ throw error;
+ }
+ body = await error.json();
+ }
+ if (gen !== fetchGenRef.current) return;
+ const { created, errors } = parseCreationResult(body);
+ setCreationErrors(errors);
if (created.length > 0) {
+ setAvailableViews(previous =>
+ previous.map(view =>
+ created.includes(view.name)
+ ? { ...view, already_added: true }
+ : view,
+ ),
+ );
+ setSelectedViewNames(previous =>
+ previous.filter(name => !created.includes(name)),
+ );
addSuccessToast(t('%s semantic view(s) added', created.length));
+ onSuccess();
}
if (errors.length > 0) {
- const failedNames = errors
- .map((error: { name?: string }) => error?.name)
- .filter((name: string | undefined): name is string => !!name);
+ const failedNames = errors.map(error => error.name).filter(Boolean);
addDangerToast(
failedNames.length > 0
? t(
'%s semantic view(s) failed to add: %s',
- errors.length,
+ failedNames.length,
Review Comment:
nit: switching from `errors.length` to `failedNames.length` undercounts when
the server returns an error without a name (the API emits
`view_data.get("name")`, which is `None` for a schema-invalid entry): two
failures render as "1 semantic view(s) failed to add: customers". Counting
server errors separately from the synthetic generic entry keeps the number
accurate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
