Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/SEP-1760.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
React UI: keep the unsaved-changes prompt after a blocked submit, stop an enable/disable toggle from clearing a scheduled task's arguments, reject silently-truncated numeric input, keep step-less task log lines, retry a failed executor-host lookup, bound execution-event stream reconnects, and no longer render a white table in dark mode.
8 changes: 7 additions & 1 deletion frontend/packages/api/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,13 @@ export function refreshAccessToken(): Promise<string | null> {
refreshInFlight = null;
});
}
_onRefreshed(data.access_token, data.expires_in);
try {
_onRefreshed(data.access_token, data.expires_in);
} catch {
// A throwing auth-layer handler must not invalidate a cookie rotation
// that already succeeded on the backend: it would reject the shared
// promise and force-logout every awaiting caller.
}
return data.access_token;
})();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/

import { useEffect, useRef, type ReactNode } from 'react';
import { useFormContext } from 'react-hook-form';
import { get, useFormContext } from 'react-hook-form';
import { AutoCompleteInput } from '@percona/percona-ui';
import { useSnackbar } from 'notistack';
import { useHosts, type HostOption } from '../../hooks/useHosts';
Expand Down Expand Up @@ -213,7 +213,9 @@ export function HostSelector({
const { data, isLoading, isError, error, refetch } = useHosts();
const hosts = data ?? EMPTY_HOSTS;
const empty = !isLoading && !isError && hosts.length === 0;
const fieldError = errors[name]?.message as string | undefined;
// Path-aware: a one-of branch field carries a dotted name (`source.host`),
// which `errors[name]` would never resolve.
const fieldError = get(errors, name)?.message as string | undefined;
const freeSolo = Boolean(allowCustom);
const inputDisabled = Boolean(disabled) || isError;
const showError = isError || Boolean(fieldError);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ describe('StandaloneHostSelector', () => {
expect(handleChange).toHaveBeenCalledWith('');
});

it('disables the input and shows error text when the endpoint rejects', async () => {
it('shows error text but stays enabled when the endpoint rejects, so opening retries', async () => {
mocked.get.mockRejectedValueOnce(
new ApiError({ kind: 'http', status: 502, message: 'network error' }),
);
Expand All @@ -111,7 +111,18 @@ describe('StandaloneHostSelector', () => {
);

await screen.findByText('network error');
expect(screen.getByLabelText('Execution Host')).toBeDisabled();
const input = screen.getByLabelText('Execution Host');
// `onOpen` holds the only retry trigger, so a disabled input would wedge the
// control until the page remounts.
expect(input).not.toBeDisabled();

mocked.get.mockResolvedValueOnce(
makeResponse([{ id: 'nomad-1', name: 'db-mysql-prod-01', address: '10.0.0.1' }]),
);
const user = userEvent.setup();
await user.click(input);

expect(await screen.findByRole('option', { name: 'db-mysql-prod-01' })).toBeInTheDocument();
});

it('surfaces upstream Tasks-API failure via snackbar', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,10 @@ export function StandaloneHostSelector({
loading={isLoading}
loadingText="Loading hosts…"
noOptionsText="No hosts available"
disabled={disabled || isError}
// Stays enabled on a failed hosts query: `onOpen` holds the only retry
// trigger, and a disabled Autocomplete never opens — one failure would
// otherwise wedge the control until the page remounts.
disabled={disabled}
renderInput={(params) => (
<TextField
{...params}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,17 @@ export function ScheduledTasksPanel({

const handleToggleEnabled = async (task: PeriodicTaskResponse, nextEnabled: boolean) => {
// PeriodicTaskUpdate requires `kwargs` and `description`, but
// PeriodicTaskResponse exposes only `description`. Tasks created elsewhere
// with non-default kwargs will have them reset to '{}' on toggle. Tracked
// upstream as a backend schema gap.
// PeriodicTaskResponse declares only `description`. Preserve `kwargs` when
// the response happens to carry it so a plain enable/disable toggle does
// not silently wipe a task's arguments; '{}' stays the last-resort
// fallback. Tracked upstream as a backend schema gap.
const rawKwargs = (task as { kwargs?: unknown }).kwargs;
const body: PeriodicTaskUpdate = {
name: task.name,
task: task.task,
enabled: nextEnabled,
description: task.description,
kwargs: '{}',
kwargs: typeof rawKwargs === 'string' && rawKwargs ? rawKwargs : '{}',
start_time: task.start_time,
interval: task.interval ?? null,
crontab: task.crontab ?? null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {
useAppTask,
useAppTasks,
type DetailSection,
type ListView,
type SepComponents,
type AppEntitySchema,
type AppSchema,
Expand Down Expand Up @@ -249,6 +250,9 @@ const BASELINE_OVERVIEW_HIDDEN_FIELDS = [
'anonymized_entities',
] as const;

/** Stable empty column set so a schema without a `list_view` never re-memoizes. */
const EMPTY_LIST_COLUMNS: ListView['columns'] = [];

function formatLabel(key: string): string {
return key.replace(/_/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase());
}
Expand Down Expand Up @@ -406,7 +410,10 @@ function OverviewTab({
const navState = (location.state ?? null) as { connectivityWarning?: unknown } | null;
const connectivityWarning = task.connectivity_warning ?? navState?.connectivityWarning;
const taskName = typeof task.name === 'string' && task.name.trim() ? task.name.trim() : undefined;
const columns = schema.list_view!.columns;
// `list_view` is optional: an entity schema reached through an unresolved
// detail route has no top-level list view, so fall back to the task's own
// fields (rendered below as `extraEntries`) rather than crashing.
const columns = schema.list_view?.columns ?? EMPTY_LIST_COLUMNS;
const schemaHiddenFields = schema.list_view?.overview_hidden_fields;

const suppressedFields = useMemo(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,14 @@ export function AppListPage({
: rows.filter((row) => RUNNING_STATUSES.has(row.status as TaskHistoryStatus)).length,
[multi, rows],
);
const listView = multi ? entitySchema!.list_view : schema.list_view!;
// `list_view` is optional on the top-level schema, and an unresolved entity
// route (unknown `entityName` on an entity schema) falls back to it, so it can
// be absent here — guarded below once every hook has run.
const listView = multi ? entitySchema!.list_view : schema.list_view;
const title = multi ? entitySchema!.display_name : schema.display_name;
const description = multi ? entitySchema?.description : schema.description;

const hasActionsColumn = listView.columns.some((c) => c.format === 'actions');
const hasActionsColumn = listView?.columns.some((c) => c.format === 'actions') ?? false;
const deleteEntity = useDeleteAppEntity(
pluginName,
entityName ?? '',
Expand Down Expand Up @@ -203,6 +206,14 @@ export function AppListPage({
});
};

if (!listView) {
return (
<Box sx={{ py: 2 }}>
<Typography variant="h5">Not found</Typography>
</Box>
);
}

return (
<Box>
{multi && schema.entities && !hideEntityTabs && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -335,4 +335,45 @@ describe('normalizeChoiceDefaults', () => {
const result = normalizeChoiceDefaults({ upload: ['RSYNC'] }, sections);
expect(result.upload).toEqual(['RSYNC']);
});

it('normalizes a one-of branch field stored at its dotted path', () => {
const oneOfSections = [
{
title: 'Source',
fields: [
{
type: 'one_of' as const,
name: 'source',
label: 'Source',
discriminator: 'source.mode',
branches: [
{
value: 'rsync',
label: 'Rsync',
fields: [
{
type: 'choice' as const,
name: 'source.transport',
label: 'Transport',
required: false,
choices: [
{ value: 'SSH', label: 'SSH' },
{ value: 'DAEMON', label: 'Daemon' },
],
},
],
},
],
},
],
},
];
const form = { source: { mode: 'rsync', transport: 'ssh' } };

const result = normalizeChoiceDefaults(form, oneOfSections);

expect(result.source).toEqual({ mode: 'rsync', transport: 'SSH' });
// The input is not mutated: `setAtPath` clones the intermediates it walks.
expect(form.source.transport).toBe('ssh');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ import Typography from '@mui/material/Typography';
import ArrowBackIcon from '@mui/icons-material/ArrowBack';
import { useSnackbar } from 'notistack';
import { useAppTask, useUpdateAppTask, type AppSchema } from '@sep/api';
import { SchemaFormRenderer, coerceFormValues, flattenSectionFields } from '../SchemaFormRenderer';
import {
SchemaFormRenderer,
coerceFormValues,
flattenSectionFields,
getAtPath,
setAtPath,
} from '../SchemaFormRenderer';
import type { FormSection, RenderFieldOverride } from '../SchemaFormRenderer/types';
import type { RenderFormSlot } from './types';
import { getStoredForm } from './storedForm';
Expand All @@ -51,16 +57,24 @@ export function normalizeChoiceDefaults(
continue;
}
const choiceMap = new Map(field.choices.map((c) => [c.value.toLowerCase(), c.value]));
const raw = out[field.name];
// Path-aware: `flattenSectionFields` also returns one-of branch fields,
// whose names are dotted paths (`source.mode`) stored nested in the form.
// `setAtPath` shallow-clones the intermediates it walks, so the shallow
// copy above is enough to leave `form` untouched.
const raw = getAtPath(out, field.name);
if (field.type === 'multi_choice' && Array.isArray(raw)) {
out[field.name] = raw.map((v) => {
const canonical = typeof v === 'string' ? choiceMap.get(v.toLowerCase()) : undefined;
return canonical ?? v;
});
setAtPath(
out,
field.name,
raw.map((v) => {
const canonical = typeof v === 'string' ? choiceMap.get(v.toLowerCase()) : undefined;
return canonical ?? v;
}),
);
} else if (field.type === 'choice' && typeof raw === 'string') {
const canonical = choiceMap.get(raw.toLowerCase());
if (canonical !== undefined) {
out[field.name] = canonical;
setAtPath(out, field.name, canonical);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,10 @@ function AppEditPage({
onSubmit: handleSubmit,
loading: updateEntity.isPending,
defaultValues,
capabilities: schema.capabilities,
renderField,
submitError,
fieldErrors,
}) ?? (
<SchemaFormRenderer
sections={sections}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,10 @@ describe('SchemaFormRenderer — field rendering', () => {
expect(screen.getByLabelText(/Notes/)).toBeInTheDocument();
expect(screen.getByLabelText(/When/)).toBeInTheDocument();
expect(screen.getByLabelText(/Config/)).toBeInTheDocument();
expect(screen.getByLabelText(/Upload/)).toBeInTheDocument();
// Exact match: the file picker button carries its own accessible name
// ("Select file for Upload"), which a /Upload/ pattern would also match.
expect(screen.getByLabelText('Upload')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Select file for Upload' })).toBeInTheDocument();
// ServiceSelector / SchemaSelector / TableSelector / HostSelector use
// percona-ui's AutoCompleteInput which renders a TextField — assert by label.
expect(screen.getByLabelText('Table')).toBeInTheDocument();
Expand Down Expand Up @@ -1126,6 +1129,39 @@ describe('SchemaFormRenderer — cardinality_rules', () => {
expect(onSubmit).not.toHaveBeenCalled();
});

it('keeps the unsaved-changes guard armed when a violation blocks submission', async () => {
const user = userEvent.setup();
const removeEventListener = vi.spyOn(window, 'removeEventListener');
renderWithProviders(
<SchemaFormRenderer
sections={[
{
title: 'Source',
cardinality_rules: [{ fields: ['a', 'b'], min: 1, max: 1, message: 'Exactly one.' }],
fields: [
{ type: 'string', name: 'a', label: 'A' },
{ type: 'string', name: 'b', label: 'B' },
],
},
]}
onSubmit={vi.fn()}
/>,
);

// Dirty the form, then trip the max=1 violation so the submit is blocked.
await user.type(screen.getByLabelText('A'), 'one');
await user.type(screen.getByLabelText('B'), 'two');
removeEventListener.mockClear();

await user.click(screen.getByRole('button', { name: /Run/ }));

// A blocked submit must not read as a successful one: react-hook-form would
// set isSubmitSuccessful, `useUnsavedChangesGuard` would go false, and its
// cleanup would drop the beforeunload listener while the form is still dirty.
expect(removeEventListener).not.toHaveBeenCalledWith('beforeunload', expect.any(Function));
removeEventListener.mockRestore();
});

it('allows submission when cardinality is satisfied', async () => {
const user = userEvent.setup();
const onSubmit = vi.fn();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,6 @@ function SchemaFormBody({
const hasInlineErrors = Object.keys(formState.errors).length > 0;

const handleFormSubmit: SubmitHandler<Record<string, unknown>> = (values) => {
if (hasSectionViolations) {
return;
}
onSubmit(coerceFormValues(values, allFields));
};

Expand All @@ -336,9 +333,17 @@ function SchemaFormBody({
// gets a setError entry that no input can ever clear; handleSubmit refuses to
// call onValid while any error remains, which would wedge resubmission.
// Those errors stay visible in the persistent banner regardless. Defined
// inline (not memoized) so it always wraps the latest handleFormSubmit, which
// closes over the current cardinality / fail_when violation state.
// inline (not memoized) so it always sees the current cardinality /
// fail_when violation state and the latest handleFormSubmit.
const handleSubmitEvent = (event: FormEvent<HTMLFormElement>) => {
if (hasSectionViolations) {
// Section-level rules render their own inline Alerts. Stop before
// react-hook-form runs, so it never flags the submit as successful —
// that would disarm `useUnsavedChangesGuard` (isDirty && !isSubmitSuccessful)
// for good on this path, since no submitError ever arrives to re-arm it.
event.preventDefault();
return;
}
if (appliedServerErrorPaths.current.length > 0) {
clearErrors(appliedServerErrorPaths.current);
appliedServerErrorPaths.current = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,12 @@ export function FileField({ field }: FileFieldProps) {
readOnly: true,
endAdornment: (
<InputAdornment position="end">
<IconButton component="label" htmlFor={inputId} edge="end">
<IconButton
aria-label={`Select file for ${field.label}`}
component="label"
htmlFor={inputId}
edge="end"
>
<AttachFileIcon fontSize="small" />
<input
id={inputId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ interface UseCascadingFieldArgs {
interface UseCascadingFieldResult<T> {
/** Current value of the upstream field, or undefined if no dependency is declared. */
upstreamValue: T | undefined;
/** True once the upstream value is set (or if there is no dependency). */
/**
* True once the upstream value is set (or if there is no dependency). An
* upstream selector with nothing selected holds `''`, which is not ready.
*/
ready: boolean;
}

Expand Down Expand Up @@ -62,13 +65,18 @@ export function useCascadingField<T = unknown>({
return;
}
if (previousRef.current !== upstreamValue) {
setValue(fieldName, undefined, { shouldValidate: false, shouldDirty: false });
// `''` is the form's empty-value sentinel for these selector types (see
// `buildFormDefaults`). `undefined` would also flip a bound MUI input
// from controlled to uncontrolled and keep the stale selection visible.
setValue(fieldName, '', { shouldValidate: false, shouldDirty: false });
previousRef.current = upstreamValue;
}
}, [upstreamValue, fieldName, dependsOn, setValue]);

const upstream = upstreamValue as unknown;

return {
upstreamValue: dependsOn ? upstreamValue : undefined,
ready: dependsOn ? upstreamValue !== undefined && upstreamValue !== null : true,
ready: dependsOn ? upstream !== undefined && upstream !== null && upstream !== '' : true,
};
}
Loading