Skip to content

[release-1.9] fix(orchestrator): orchestrator 1.9 backport#2876

Merged
karthikjeeyar merged 1 commit intoworkspace/orchestratorfrom
orchestrator-1.9-backport
Apr 22, 2026
Merged

[release-1.9] fix(orchestrator): orchestrator 1.9 backport#2876
karthikjeeyar merged 1 commit intoworkspace/orchestratorfrom
orchestrator-1.9-backport

Conversation

@karthikjeeyar
Copy link
Copy Markdown
Member

Cherrypick of below PR's


Hey, I just made a Pull Request!

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

* fix(orchestrator): preserve nested extraErrors and async validation errors (#2818)

* fix(orchestrator-form): Review step with empty form data (#2834)

Empty pruned form data (e.g. display-only ActiveText) made generateReviewTableData return undefined and NestedReviewTable throw. Return {} from generateReviewTableData and guard NestedReviewTable. Add regression test and changeset.

Made-with: Cursor

* fix(orchestrator-form-react): fix ui styling issues in workflow results page (#2808)

---------

Co-authored-by: Karthik Jeeyar <karthik@redhat.com>
@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge Bot commented Apr 22, 2026

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Invalid types import path 🐞 Bug ≡ Correctness
Description
safeSet.test.ts imports JsonObject from @backstage/types/index, which can break under package
exports restrictions and fail tests/builds. Other code in this repo consistently imports from
@backstage/types.
Code

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts[R19-21]

+import { safeSet } from './safeSet';
+import { JsonObject } from '@backstage/types/index';
+
Relevance

⭐⭐⭐ High

Repo generally avoids brittle deep import paths; root @backstage/types is the stable pattern.

PR-#2818

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test file imports a subpath (@backstage/types/index) that may not be exported by the
package, while production code uses the root import (@backstage/types), indicating the intended
stable import path.

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts[17-21]
workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[17-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A test imports from `@backstage/types/index`, which may not be allowed by the package export map.

### Issue Context
The rest of the codebase imports Backstage types from `@backstage/types`.

### Fix Focus Areas
- workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts[17-21]

### Suggested fix
Change:
```ts
import { JsonObject } from '@backstage/types/index';
```
To:
```ts
import { JsonObject } from '@backstage/types';
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Unhandled rejection risk 🐞 Bug ☼ Reliability
Description
useGetExtraErrors creates all async-validation promises up-front and then awaits them one-by-one;
a promise can reject before its turn is awaited, which can emit unhandledrejection warnings/events
and still doesn't truly serialize execution. If serialization is needed, tasks must be executed
lazily; otherwise revert to Promise.all/Promise.allSettled to attach handlers immediately.
Code

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[R146-151]

+    // 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;
+    }
Relevance

⭐⭐⭐ High

Eagerly-started promises awaited later can trigger unhandledrejection; team likely wants safer
async handling.

PR-#2818

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
walkThrough eagerly calls the async callback and returns already-started promises, while
callback performs multiple awaited operations that can throw/reject (template evaluation,
request-init building, fetch, json parsing). Sequentially awaiting these already-started promises
leaves a window where later rejections have no handler attached yet, and also does not prevent
concurrent execution since the promises have already begun.

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[32-66]
workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[77-134]
workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[139-153]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`useGetExtraErrors` builds an array of already-started promises via `walkThrough(...)` and then awaits them sequentially. This does not actually serialize the async work (promises start immediately) and it can allow a later promise to reject before any handler is attached, causing `unhandledrejection` warnings/events.

### Issue Context
The goal (per comment) is to avoid interleaving mutations on the shared `errors` object. If serialization is truly required, you must *not* start all async callbacks at once.

### Fix Focus Areas
- workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[32-66]
- workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[139-153]

### Suggested fix
Choose one:
1) If you don't need strict serialization anymore (since `safeSet` is fixed), revert to `await Promise.all(promises)` or `await Promise.allSettled(promises)`.
2) If you do need serialization, refactor `walkThrough` to return a list of *thunks* (`() => Promise<void>`) or to be an `async` walker that `await`s `callback(...)` as it traverses, so each async validation is started only when it's being awaited.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. Mixed MUI Alert versions 🐞 Bug ⚙ Maintainability
Description
WorkflowResult now imports Alert/AlertTitle from @material-ui/lab (MUI v4) while the
component otherwise uses @mui/material (MUI v5), increasing the risk of theme/styling
inconsistencies and adding a new runtime dependency. Prefer using the MUI v5 @mui/material Alert
like the rest of the plugin.
Code

workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowResult.tsx[R28-31]

+import { Alert, AlertTitle } from '@material-ui/lab';
import Box from '@mui/material/Box';
import Button from '@mui/material/Button';
import CircularProgress from '@mui/material/CircularProgress';
Relevance

⭐ Low

Team intentionally switched to @material-ui/lab Alert to fix theme conflicts; mixing v4/v5 is
accepted here.

PR-#2808

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Only WorkflowResult uses @material-ui/lab for Alert; other orchestrator components use
@mui/material Alert/AlertTitle. The PR also adds @material-ui/lab to the plugin dependencies,
widening the MUI v4/v5 mix within the same frontend plugin.

workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowResult.tsx[19-36]
workspaces/orchestrator/plugins/orchestrator/src/components/ExecuteWorkflowPage/MissingSchemaNotice.tsx[17-22]
workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowInstancePage.tsx[34-52]
workspaces/orchestrator/plugins/orchestrator/package.json[55-69]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`WorkflowResult` switched to `@material-ui/lab` (MUI v4) Alert components inside a file that otherwise uses `@mui/material` (MUI v5). This increases maintenance burden and can cause styling/theming inconsistencies.

### Issue Context
Other orchestrator components already use `@mui/material/Alert` and `@mui/material/AlertTitle`.

### Fix Focus Areas
- workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowResult.tsx[25-36]
- workspaces/orchestrator/plugins/orchestrator/package.json[62-67]

### Suggested fix
Prefer:
```ts
import Alert from '@mui/material/Alert';
import AlertTitle from '@mui/material/AlertTitle';
```
Then remove `@material-ui/lab` from `dependencies` if it was introduced only for this usage.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@sonarqubecloud
Copy link
Copy Markdown

@karthikjeeyar karthikjeeyar merged commit 1d7ac98 into workspace/orchestrator Apr 22, 2026
11 checks passed
@rhdh-qodo-merge
Copy link
Copy Markdown

Review Summary by Qodo

Fix orchestrator 1.9 backport: nested errors, empty forms, and UI styling

🐞 Bug fix 🧪 Tests

Grey Divider

Walkthroughs

Description
• Fix nested extraErrors and async validation errors in multi-step forms
  - safeSet now correctly handles deep paths (e.g., step.x.y) by recursing on remainder instead of
  truncating
  - useGetExtraErrors runs async callbacks sequentially to prevent lost updates on shared error object
  - toRootExtraErrors wraps active step errors for RJSF root schema compatibility
• Fix Review step crash with empty form data
  - generateReviewTableData returns {} instead of undefined for empty data
  - NestedReviewTable guards against undefined data parameter
• Fix UI styling issues in workflow results page
  - Update Alert/AlertTitle imports from @material-ui/lab for consistency
  - Fix Divider sx prop specificity with && selector for proper margin application
  - Add flexbox layout to WorkflowDescriptionModal title for better alignment
Diagram
flowchart LR
  A["Multi-step Form Validation"] --> B["safeSet Deep Paths"]
  A --> C["Sequential Async Callbacks"]
  A --> D["toRootExtraErrors Wrapper"]
  B --> E["Preserve Nested Field Errors"]
  C --> E
  D --> E
  F["Review Step"] --> G["generateReviewTableData"]
  G --> H["Handle Empty Form Data"]
  H --> I["NestedReviewTable Guard"]
  J["UI Components"] --> K["Import Updates"]
  J --> L["Divider Styling Fix"]
  K --> M["Consistent Material-UI Usage"]
  L --> M
Loading

Grey Divider

File Changes

1. workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.ts 🐞 Bug fix +8/-6

Fix deep path recursion in error object building

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.ts


2. workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts 🧪 Tests +68/-0

Add tests for deep path and sibling field merging

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/safeSet.test.ts


3. workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts 🐞 Bug fix +6/-1

Run async validation callbacks sequentially to prevent lost updates

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts


View more (14)
4. workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.test.tsx 🧪 Tests +120/-0

Test async validation error aggregation for nested fields

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.test.tsx


5. workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.ts ✨ Enhancement +41/-0

Wrap extraErrors with active step key for RJSF compatibility

workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.ts


6. workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.test.ts 🧪 Tests +53/-0

Test extraErrors wrapping for multi-step form validation

workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/toRootExtraErrors.test.ts


7. workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.ts 🐞 Bug fix +2/-1

Return empty object instead of undefined for empty data

workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.ts


8. workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.test.ts 🧪 Tests +17/-0

Add regression test for empty form data handling

workspaces/orchestrator/plugins/orchestrator-form-react/src/utils/generateReviewTableData.test.ts


9. workspaces/orchestrator/plugins/orchestrator-form-react/src/components/NestedReviewTable.tsx 🐞 Bug fix +2/-1

Guard against undefined data parameter with safe default

workspaces/orchestrator/plugins/orchestrator-form-react/src/components/NestedReviewTable.tsx


10. workspaces/orchestrator/plugins/orchestrator-form-react/src/components/OrchestratorFormWrapper.tsx ✨ Enhancement +2/-9

Use toRootExtraErrors utility for error wrapping logic

workspaces/orchestrator/plugins/orchestrator-form-react/src/components/OrchestratorFormWrapper.tsx


11. workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowDescriptionModal.tsx 🐞 Bug fix +8/-1

Add flexbox layout to dialog title for proper alignment

workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowDescriptionModal.tsx


12. workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowResult.tsx 🐞 Bug fix +3/-4

Fix Alert imports and Divider styling specificity issues

workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowInstancePage/WorkflowResult.tsx


13. workspaces/orchestrator/plugins/orchestrator/src/components/ui/TextCodeBlock.tsx 🐞 Bug fix +4/-1

Fix root element styling specificity with && selector

workspaces/orchestrator/plugins/orchestrator/src/components/ui/TextCodeBlock.tsx


14. workspaces/orchestrator/plugins/orchestrator/package.json Dependencies +1/-0

Add @material-ui/lab dependency for Alert components

workspaces/orchestrator/plugins/orchestrator/package.json


15. workspaces/orchestrator/.changeset/fix-review-empty-formdata.md 📝 Documentation +5/-0

Document Review step empty form data fix

workspaces/orchestrator/.changeset/fix-review-empty-formdata.md


16. workspaces/orchestrator/.changeset/new-books-matter.md 📝 Documentation +5/-0

Document workflow results page UI styling fixes

workspaces/orchestrator/.changeset/new-books-matter.md


17. workspaces/orchestrator/.changeset/sharp-dots-sing.md 📝 Documentation +10/-0

Document async validation and deep path error fixes

workspaces/orchestrator/.changeset/sharp-dots-sing.md


Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants