diff --git a/workspaces/orchestrator/.changeset/fix-review-empty-formdata.md b/workspaces/orchestrator/.changeset/fix-review-empty-formdata.md new file mode 100644 index 0000000000..e5bed1af89 --- /dev/null +++ b/workspaces/orchestrator/.changeset/fix-review-empty-formdata.md @@ -0,0 +1,5 @@ +--- +'@red-hat-developer-hub/backstage-plugin-orchestrator-form-react': patch +--- + +fix: avoid Review step crash when form data is empty (for example display-only `ActiveText` with no submitted values). `generateReviewTableData` returns `{}` instead of undefined, and `NestedReviewTable` handles missing data. diff --git a/workspaces/orchestrator/.changeset/new-books-matter.md b/workspaces/orchestrator/.changeset/new-books-matter.md new file mode 100644 index 0000000000..dbef34c04d --- /dev/null +++ b/workspaces/orchestrator/.changeset/new-books-matter.md @@ -0,0 +1,5 @@ +--- +'@red-hat-developer-hub/backstage-plugin-orchestrator': patch +--- + +Fix UI styling issues in worfkflow results page diff --git a/workspaces/orchestrator/.changeset/sharp-dots-sing.md b/workspaces/orchestrator/.changeset/sharp-dots-sing.md new file mode 100644 index 0000000000..6e2106e1a3 --- /dev/null +++ b/workspaces/orchestrator/.changeset/sharp-dots-sing.md @@ -0,0 +1,10 @@ +--- +'@red-hat-developer-hub/backstage-plugin-orchestrator-form-widgets': patch +'@red-hat-developer-hub/backstage-plugin-orchestrator-form-react': patch +--- + +Fix multi-step workflow forms dropping or misplacing async validation errors and deep nested field paths. + +- @red-hat-developer-hub/backstage-plugin-orchestrator-form-widgets: Fix safeSet deep paths and sequential async validation in getExtraErrors. + +- @red-hat-developer-hub/backstage-plugin-orchestrator-form-react: Wrap extraErrors with the active step key so RJSF matches the root schema. diff --git a/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/NestedReviewTable.tsx b/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/NestedReviewTable.tsx index e56f5666c9..25a0d25dde 100644 --- a/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/NestedReviewTable.tsx +++ b/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/NestedReviewTable.tsx @@ -112,9 +112,10 @@ export const NestedReviewTable: React.FC = ({ data, }) => { const { classes } = useStyles(); + const safeData = data ?? {}; // Group top-level sections - const sections: [string, JsonValue][] = Object.entries(data).filter( + const sections: [string, JsonValue][] = Object.entries(safeData).filter( ([_, value]) => value !== undefined, ) as [string, JsonValue][]; diff --git a/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/OrchestratorFormWrapper.tsx b/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/OrchestratorFormWrapper.tsx index 313d8b58ec..a2c5d08d46 100644 --- a/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/OrchestratorFormWrapper.tsx +++ b/workspaces/orchestrator/plugins/orchestrator-form-react/src/components/OrchestratorFormWrapper.tsx @@ -35,6 +35,7 @@ import { import { useTranslation } from '../hooks/useTranslation'; import { getActiveStepKey } from '../utils/getSortedStepEntries'; import { useStepperContext } from '../utils/StepperContext'; +import { toRootExtraErrors } from '../utils/toRootExtraErrors'; import useValidator from '../utils/useValidator'; import { AuthRequester } from './AuthRequester'; import { createHiddenFieldTemplate } from './HiddenFieldTemplate'; @@ -108,15 +109,7 @@ const FormComponent = (decoratorProps: FormDecoratorProps) => { extraErrorsUiSchema, ); - if (activeKey) { - setExtraErrors( - _extraErrors?.[activeKey] - ? (_extraErrors[activeKey] as ErrorSchema) - : undefined, - ); - } else { - setExtraErrors(_extraErrors); - } + setExtraErrors(toRootExtraErrors(activeKey, _extraErrors)); } catch (err) { _validationError = err as Error; } finally { diff --git a/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.test.ts b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.test.ts index 5541bda745..a32f07a71d 100644 --- a/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.test.ts +++ b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.test.ts @@ -351,4 +351,21 @@ describe('mapSchemaToData', () => { const result = generateReviewTableData(schema, data); expect(result).toEqual(expectedResult); }); + + it('returns an empty object when form data is empty (e.g. display-only ActiveText)', () => { + const schema: JSONSchema7 = { + type: 'object', + title: 'Access request', + properties: { + infoMessage: { + type: 'string', + title: 'Info', + 'ui:widget': 'ActiveText', + } as JSONSchema7, + }, + }; + + const result = generateReviewTableData(schema, {}); + expect(result).toEqual({}); + }); }); diff --git a/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.ts b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.ts index 1be463c850..0d74880f3e 100644 --- a/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.ts +++ b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.ts @@ -101,7 +101,8 @@ function generateReviewTableData( data, options?.includeHiddenFields ?? false, ); - return result[''] as JsonObject; + const root = result[''] as JsonObject | undefined; + return root ?? {}; } export default generateReviewTableData; diff --git a/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.test.ts b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.test.ts new file mode 100644 index 0000000000..be4da620f4 --- /dev/null +++ b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.test.ts @@ -0,0 +1,53 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ERRORS_KEY } from '@rjsf/utils'; + +import { toRootExtraErrors } from './toRootExtraErrors'; + +describe('toRootExtraErrors', () => { + it('returns undefined when extraErrors is undefined', () => { + expect(toRootExtraErrors('my-step', undefined)).toBeUndefined(); + }); + + it('returns extraErrors unchanged when activeKey is undefined', () => { + const tree = { a: { [ERRORS_KEY]: ['x'] } }; + expect(toRootExtraErrors(undefined, tree)).toBe(tree); + }); + + it('wraps the active step slice so RJSF receives root-shaped errors', () => { + const inner = { + xParams: { + fieldA: { [ERRORS_KEY]: ['invalid A'] }, + fieldB: { [ERRORS_KEY]: ['invalid B'] }, + }, + }; + const fromGetExtraErrors = { + 'my-solution': inner, + }; + expect(toRootExtraErrors('my-solution', fromGetExtraErrors)).toEqual({ + 'my-solution': inner, + }); + }); + + it('returns undefined when the active step has no errors in the tree', () => { + expect( + toRootExtraErrors('missing-step', { + 'other-step': { [ERRORS_KEY]: ['e'] }, + }), + ).toBeUndefined(); + }); +}); diff --git a/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.ts b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.ts new file mode 100644 index 0000000000..dc4187c28f --- /dev/null +++ b/workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.ts @@ -0,0 +1,41 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { JsonObject } from '@backstage/types'; + +import { ErrorSchema } from '@rjsf/utils'; + +/** + * RJSF `extraErrors` must match the root `schema.properties` shape. For + * multi-step forms, `getExtraErrors` returns a tree keyed by step name; pass + * `activeKey` so the slice is wrapped again for the root form. + */ +export function toRootExtraErrors( + activeKey: string | undefined, + extraErrors: ErrorSchema | undefined, +): ErrorSchema | undefined { + if (!extraErrors) { + return undefined; + } + if (!activeKey) { + return extraErrors; + } + const slice = extraErrors[activeKey]; + if (!slice) { + return undefined; + } + return { [activeKey]: slice } as ErrorSchema; +} diff --git a/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts new file mode 100644 index 0000000000..739aa7ecc7 --- /dev/null +++ b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts @@ -0,0 +1,68 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ERRORS_KEY } from '@rjsf/utils'; + +import { safeSet } from './safeSet'; +import { JsonObject } from '@backstage/types/index'; + +describe('safeSet', () => { + it('splits only on the first dot so deep paths (e.g. step.x.y) are not truncated', () => { + // `String.prototype.split(".", 2)` truncates remainder (e.g. "a.b.c" -> ["a","b"]); + // paths with three or more segments must recurse on `a` + `b.c`, not `a` + `b` only. + const errors: JsonObject = {}; + safeSet(errors, 'a.b.c.d', { [ERRORS_KEY]: ['deep'] }); + expect(errors).toEqual({ + a: { + b: { + c: { + d: { + [ERRORS_KEY]: ['deep'], + }, + }, + }, + }, + }); + }); + + it('sets a single-segment path', () => { + const errors: JsonObject = {}; + safeSet(errors, 'onlyKey', { [ERRORS_KEY]: ['msg'] }); + expect(errors).toEqual({ onlyKey: { [ERRORS_KEY]: ['msg'] } }); + }); + + it('merges sibling leaf paths under a shared prefix when applied sequentially', () => { + const errors: JsonObject = {}; + safeSet(errors, 'my-solution.xParams.fieldA', { + [ERRORS_KEY]: ['a'], + }); + safeSet(errors, 'my-solution.xParams.fieldB', { + [ERRORS_KEY]: ['b'], + }); + safeSet(errors, 'my-solution.xParams.fieldC', { + [ERRORS_KEY]: ['c'], + }); + expect(errors).toEqual({ + 'my-solution': { + xParams: { + fieldA: { [ERRORS_KEY]: ['a'] }, + fieldB: { [ERRORS_KEY]: ['b'] }, + fieldC: { [ERRORS_KEY]: ['c'] }, + }, + }, + }); + }); +}); diff --git a/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.ts b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.ts index 85b8859dac..1b71d062c4 100644 --- a/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.ts +++ b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.ts @@ -21,12 +21,14 @@ export const safeSet: ( path: string, value: JsonValue, ) => void = (errors, path, value) => { - const steps = path.split('.', 2); - if (steps.length === 1) { - errors[steps[0]] = value; + const dot = path.indexOf('.'); + if (dot === -1) { + errors[path] = value; } else { - const safeObject = (errors[steps[0]] ?? {}) as JsonObject; - errors[steps[0]] = safeObject; - safeSet(safeObject, steps[1], value); + const head = path.slice(0, dot); + const rest = path.slice(dot + 1); + const safeObject = (errors[head] ?? {}) as JsonObject; + errors[head] = safeObject; + safeSet(safeObject, rest, value); } }; diff --git a/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.test.tsx b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.test.tsx new file mode 100644 index 0000000000..17cd455240 --- /dev/null +++ b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.test.tsx @@ -0,0 +1,120 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { renderHook } from '@testing-library/react'; + +import { JsonObject } from '@backstage/types'; +import { ERRORS_KEY } from '@rjsf/utils'; + +import { useGetExtraErrors } from './useGetExtraErrors'; + +jest.mock('./evaluateTemplate', () => ({ + evaluateTemplateString: jest + .fn() + .mockResolvedValue('https://example.com/validate'), +})); + +jest.mock('./useRequestInit', () => ({ + getRequestInit: jest.fn().mockResolvedValue({}), +})); + +jest.mock('./useTemplateUnitEvaluator', () => ({ + useTemplateUnitEvaluator: jest.fn(() => async () => undefined), +})); + +const mockFetch = jest.fn(); + +jest.mock('@backstage/core-plugin-api', () => { + const actual = jest.requireActual('@backstage/core-plugin-api'); + return { + ...actual, + useApi: (ref: unknown) => { + if (ref === actual.fetchApiRef) { + return { fetch: mockFetch }; + } + throw new Error( + `useGetExtraErrors.test: unmocked api ref: ${String(ref)}`, + ); + }, + }; +}); + +describe('useGetExtraErrors', () => { + beforeEach(() => { + mockFetch.mockReset(); + }); + + it('aggregates async validation errors for all nested fields under a shared path prefix', async () => { + let fetchCall = 0; + mockFetch.mockImplementation(async () => { + const n = ++fetchCall; + return { + status: 422, + json: async () => ({ issues: [`validation failed for request ${n}`] }), + }; + }); + + const { result } = renderHook(() => useGetExtraErrors()); + + const uiSchema: JsonObject = { + 'my-solution': { + xParams: { + fieldA: { + 'ui:widget': 'ActiveTextInput', + 'ui:props': { 'validate:url': 'https://example.com/validate' }, + }, + fieldB: { + 'ui:widget': 'ActiveTextInput', + 'ui:props': { 'validate:url': 'https://example.com/validate' }, + }, + fieldC: { + 'ui:widget': 'ActiveTextInput', + 'ui:props': { 'validate:url': 'https://example.com/validate' }, + }, + }, + }, + }; + + const formData: JsonObject = { + 'my-solution': { + xParams: { + fieldA: '', + fieldB: '', + fieldC: '', + }, + }, + }; + + const errors = await result.current(formData, uiSchema); + + expect(mockFetch).toHaveBeenCalledTimes(3); + expect(errors).toEqual({ + 'my-solution': { + xParams: { + fieldA: { + [ERRORS_KEY]: ['validation failed for request 1'], + }, + fieldB: { + [ERRORS_KEY]: ['validation failed for request 2'], + }, + fieldC: { + [ERRORS_KEY]: ['validation failed for request 3'], + }, + }, + }, + }); + }); +}); diff --git a/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts index 2b1b9a6c4d..2ed6ca88b9 100644 --- a/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts +++ b/workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts @@ -143,7 +143,12 @@ export const useGetExtraErrors = () => { '', ); - await Promise.all(promises); + // Run sequentially so concurrent async callbacks cannot interleave + // `safeSet` mutations on the shared `errors` object (lost updates under + // the same path prefix, e.g. `step.xParams.fieldA` vs `fieldB`). + for (const p of promises) { + await p; + } return errors; }; }; diff --git a/workspaces/orchestrator/plugins/orchestrator/package.json b/workspaces/orchestrator/plugins/orchestrator/package.json index 648a9c08df..66c7dd7d34 100644 --- a/workspaces/orchestrator/plugins/orchestrator/package.json +++ b/workspaces/orchestrator/plugins/orchestrator/package.json @@ -62,6 +62,7 @@ "@backstage/plugin-permission-react": "^0.4.38", "@backstage/types": "^1.2.2", "@material-ui/core": "^4.12.4", + "@material-ui/lab": "^4.0.0-alpha.61", "@red-hat-developer-hub/backstage-plugin-orchestrator-common": "workspace:^", "@red-hat-developer-hub/backstage-plugin-orchestrator-form-api": "workspace:^", "@red-hat-developer-hub/backstage-plugin-orchestrator-form-react": "workspace:^", diff --git a/workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowDescriptionModal.tsx b/workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowDescriptionModal.tsx index 1547d02f30..6a42047143 100644 --- a/workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowDescriptionModal.tsx +++ b/workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowDescriptionModal.tsx @@ -118,7 +118,14 @@ export const RefForwardingWorkflowDescriptionModal: ForwardRefRenderFunction< fullWidth > - + {workflow.name} - + {logsEnabled && ( <> @@ -452,7 +451,7 @@ export const WorkflowResult: React.FC<{ {t('run.logs.viewLogs')} - + )} diff --git a/workspaces/orchestrator/plugins/orchestrator/src/components/ui/TextCodeBlock.tsx b/workspaces/orchestrator/plugins/orchestrator/src/components/ui/TextCodeBlock.tsx index 1f0e24473a..6ad7f85b0b 100644 --- a/workspaces/orchestrator/plugins/orchestrator/src/components/ui/TextCodeBlock.tsx +++ b/workspaces/orchestrator/plugins/orchestrator/src/components/ui/TextCodeBlock.tsx @@ -29,9 +29,12 @@ const useStyles = makeStyles<{ isDarkMode: boolean; maxHeight?: number }>()( position: 'relative', paddingTop: theme.spacing(2), paddingBottom: theme.spacing(2), - backgroundColor: isDarkMode ? '#151515' : '#F0F0F0', maxWidth: 600, marginTop: '0.6rem', + '&&': { + outline: 'unset', + backgroundColor: isDarkMode ? '#151515' : '#F0F0F0', + }, }, iconButton: { position: 'absolute', diff --git a/workspaces/orchestrator/yarn.lock b/workspaces/orchestrator/yarn.lock index 8862f14654..ec79cd7c04 100644 --- a/workspaces/orchestrator/yarn.lock +++ b/workspaces/orchestrator/yarn.lock @@ -12613,6 +12613,7 @@ __metadata: "@backstage/types": ^1.2.2 "@janus-idp/cli": 3.6.1 "@material-ui/core": ^4.12.4 + "@material-ui/lab": ^4.0.0-alpha.61 "@mui/icons-material": ^5.17.1 "@mui/material": ^5.17.1 "@red-hat-developer-hub/backstage-plugin-orchestrator-common": "workspace:^"