Skip to content

Orchestrator 1.9 backport#2863

Merged
karthikjeeyar merged 3 commits intoredhat-developer:orchestrator-1.9-backportfrom
lokanandaprabhu:orchestrator-1.9-backport
Apr 22, 2026
Merged

Orchestrator 1.9 backport#2863
karthikjeeyar merged 3 commits intoredhat-developer:orchestrator-1.9-backportfrom
lokanandaprabhu:orchestrator-1.9-backport

Conversation

@lokanandaprabhu
Copy link
Copy Markdown
Member

Cherrypick of below PR's

#2834
#2808
#2818

karthikjeeyar and others added 3 commits April 22, 2026 11:14
…loper#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
@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge Bot commented Apr 22, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. Unhandled rejections in loop 🐞 Bug ☼ Reliability
Description
useGetExtraErrors builds an array of already-started Promises and then awaits them one-by-one; any
Promise that rejects before it is awaited can surface as an unhandled rejection. The code also still
starts all async validations concurrently despite the comment claiming sequential execution.
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

⭐⭐ Medium

Concern is plausible, but same sequential-await pattern was just merged for this file without review
pushback.

PR-#2818

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
walkThrough invokes the async callback immediately while building the array (so all Promises begin
execution right away), and the subsequent for-loop awaits them sequentially, delaying attachment of
rejection handlers for later Promises.

workspaces/orchestrator/plugins/orchestrator-form-widgets/src/utils/useGetExtraErrors.ts[32-66]
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` constructs an array of Promises (which start running immediately) and then awaits them one-by-one. This can produce unhandled promise rejection events for promises that reject before they are awaited, and it does not actually enforce sequential execution.

### Issue Context
The PR comment indicates an intent to run validations sequentially to avoid interleaving mutations on the shared `errors` object, but the current structure (`walkThrough` returning started Promises) does not provide true serialization.

### 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 approach:
1) If serialization is *not* required (likely now that `safeSet` is fixed): revert to `await Promise.all(promises)` to ensure all rejections are handled.
2) If true serialization is required: change `walkThrough` to return an array of thunks (`() => Promise<void>`) or collected `{path, uiSchemaProperty}` items, then `for...of` invoke/await each one; this ensures only one async callback runs at a time and avoids unhandled rejections.

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



Remediation recommended

2. Non-canonical types import 🐞 Bug ⚙ Maintainability
Description
safeSet.test.ts imports JsonObject from the '@backstage/types/index' subpath while the rest of the
workspace imports from '@backstage/types'. This inconsistent import path makes tooling/refactors
riskier and can complicate module resolution across environments.
Code

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

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

⭐⭐⭐ High

Repo previously replaced '@backstage/types/index' with '@backstage/types' to fix TS issues;
canonical import preferred.

PR-#1990

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test uses a different module path than the production code and other tests in the same
package, creating inconsistency within the workspace.

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

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

### Issue description
The test imports `JsonObject` from `@backstage/types/index` instead of `@backstage/types`, which is inconsistent with the rest of the codebase.

### Issue Context
Keeping imports consistent avoids brittle module-resolution behavior and makes automated refactors (and lint rules) more reliable.

### 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


Grey Divider

Qodo Logo

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@karthikjeeyar karthikjeeyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@rhdh-qodo-merge
Copy link
Copy Markdown

Review Summary by Qodo

Fix orchestrator form validation errors and UI styling issues

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Fix multi-step form validation errors dropping or misplacing nested field paths
• Preserve async validation errors by running callbacks sequentially instead of concurrently
• Handle empty form data in review step to prevent crashes with display-only fields
• Fix UI styling issues in workflow results page with Material-UI component imports
Diagram
flowchart LR
  A["Multi-step Forms"] --> B["safeSet Deep Paths"]
  A --> C["Sequential Async Validation"]
  A --> D["toRootExtraErrors Wrapper"]
  B --> E["Fixed Error Structure"]
  C --> E
  D --> E
  F["Review Step"] --> G["generateReviewTableData"]
  F --> H["NestedReviewTable Guards"]
  G --> I["Empty Form Data Handling"]
  H --> I
  J["Workflow Results UI"] --> K["Material-UI Imports"]
  J --> L["Divider Styling"]
  K --> M["Fixed UI Rendering"]
  L --> M
Loading

Grey Divider

File Changes

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

Fix deep nested path handling in error structure

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 comprehensive tests for deep path recursion

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 race conditions

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 error 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 form 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 in review table rendering

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 handling

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


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

Fix dialog title layout with flexbox styling

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


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

Fix Material-UI imports and divider styling 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 code block styling with proper CSS selector specificity

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 fix for empty form data in review step

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 multi-step form validation error handling fixes

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


Grey Divider

Qodo Logo

@rhdh-qodo-merge rhdh-qodo-merge Bot added documentation Improvements or additions to documentation enhancement New feature or request Tests Bug fix labels Apr 22, 2026
@karthikjeeyar karthikjeeyar merged commit 4ec3c3f into redhat-developer:orchestrator-1.9-backport Apr 22, 2026
11 checks passed
karthikjeeyar added a commit that referenced this pull request Apr 22, 2026
* 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: Lokananda Prabhu <102503482+lokanandaprabhu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug fix documentation Improvements or additions to documentation enhancement New feature or request lgtm Tests workspace/orchestrator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants