[ACM-30639] General wizard UX improvements for placement#6010
Merged
openshift-merge-bot[bot] merged 6 commits intomainfrom Apr 22, 2026
Merged
[ACM-30639] General wizard UX improvements for placement#6010openshift-merge-bot[bot] merged 6 commits intomainfrom
openshift-merge-bot[bot] merged 6 commits intomainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new WizLabelSelect input and re-exports it; integrates it into match-expression UI; adds tolerations editor and ensures default tolerations are included when creating Placements; refactors predicate/layout styling; and updates multiple tests to account for duplicate “Action” buttons and assert toleration YAML. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Placement UI / MatchExpression / WizLabelSelect
participant Hook as useInput / Local State
participant API as Kubernetes API
User->>UI: Choose "New placement" / open match expression
UI->>Hook: initialize fields (predicates, default tolerations)
Hook-->>UI: provide value, setValue handlers
User->>UI: Open WizLabelSelect / type or pick option
UI->>Hook: filter options / set selected value
Hook-->>UI: current stored value (label↔value)
User->>UI: Submit placement
UI->>API: POST Placement (includes spec.tolerations, numberOfClusters)
API-->>UI: 201 Created
UI-->>User: show success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx (1)
57-57: Using index-based selection for multiple action buttons.Using
getAllByRole(...)[0]handles the multiple-button scenario but is order-dependent. If button ordering changes, this test could break silently or click the wrong element. Consider adding a more specific selector or a comment documenting which button is expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx` at line 57, The test currently clicks the first match of screen.getAllByRole('button', { name: /action/i }) which is order-dependent; update the test to target the specific button more robustly by selecting it via a unique accessible name or container scope (e.g., use getByRole with the explicit button label, use within(<parentElement>).getByRole(...), or use a dedicated data-testid/aria-label on the intended action button) and replace the index-based access with that selector; if adding a test-only identifier isn’t possible, at minimum add a short comment explaining why the first button is guaranteed to be the correct one.frontend/src/wizards/Argo/ArgoWizard.test.tsx (1)
221-226: Selector strategy is order-dependent and can become flakyUsing
clickByRole('button', { name: 'Action' }, 0)plusclickByText('equals any of')depends on render order and duplicate text assumptions. Prefer scoping queries to the specific predicate row/container to keep these tests stable.As per coding guidelines "Ensure tests actually test the described behavior".
Also applies to: 332-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Argo/ArgoWizard.test.tsx` around lines 221 - 226, The test uses global queries (clickByRole and clickByText) that rely on render order and duplicate text; update the interactions to scope queries to the specific predicate row/container (e.g., locate the predicate row element first and then use within(row).getByRole / within(row).getByText) so you click the correct "Action" button and the correct "equals any of" / "does not equal any of" option; replace the top-level clickByRole('button', { name: 'Action' }, 0) and clickByText('equals any of') calls with scoped queries against that predicate container and apply the same scoped change to the other occurrence referenced in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx`:
- Around line 96-98: The "no results" placeholder (set via
setFilteredOptions([noResults])) is currently selectable and onSelect will fall
back to setting noResults as the value; update the component so the placeholder
cannot become a valid selection by either (a) rendering the noResults entry as a
disabled/non-selectable option (e.g., mark the item representing noResults as
disabled in the list rendering) or (b) adding an explicit guard in the onSelect
handler (the onSelect function and normalizedOptions lookup) that ignores clicks
where the chosen label/value equals the noResults token and does not call the
value setter. Reference the noResults variable, setFilteredOptions, onSelect,
and normalizedOptions when making this change.
In `@frontend/src/wizards/Argo/ArgoWizard.tsx`:
- Around line 916-925: When re-creating the Placement.spec for the "New
placement" flow in ArgoWizard.tsx, the new object currently omits
numberOfClusters which causes selection behavior to change; update the two
places that rebuild Placement.spec (the blocks that set tolerations and other
placement fields when switching to "New placement"—the re-creation that follows
the tolerations array and the other similar recreation further down) to include
numberOfClusters: 1 in the produced spec object so the default matches the
initial Placement.spec (ensure the property is added alongside existing fields
like tolerations and matchExpressions).
In `@frontend/src/wizards/Placement/MatchExpression.css`:
- Around line 1-9: The calc() expression in the .match-expression-field
.pf-v6-c-menu-toggle:not(.pf-m-typeahead) rule is split across lines causing a
stylelint operator-newline error; reformat the calc() so the multiplication
operations remain on a single line (combine the font-size * line-height and the
2 * spacer terms onto one line each or the whole expression onto one line) while
preserving the same values and surrounding whitespace, leaving other
declarations (padding-block, display, align-items) unchanged.
---
Nitpick comments:
In `@frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx`:
- Line 57: The test currently clicks the first match of
screen.getAllByRole('button', { name: /action/i }) which is order-dependent;
update the test to target the specific button more robustly by selecting it via
a unique accessible name or container scope (e.g., use getByRole with the
explicit button label, use within(<parentElement>).getByRole(...), or use a
dedicated data-testid/aria-label on the intended action button) and replace the
index-based access with that selector; if adding a test-only identifier isn’t
possible, at minimum add a short comment explaining why the first button is
guaranteed to be the correct one.
In `@frontend/src/wizards/Argo/ArgoWizard.test.tsx`:
- Around line 221-226: The test uses global queries (clickByRole and
clickByText) that rely on render order and duplicate text; update the
interactions to scope queries to the specific predicate row/container (e.g.,
locate the predicate row element first and then use within(row).getByRole /
within(row).getByText) so you click the correct "Action" button and the correct
"equals any of" / "does not equal any of" option; replace the top-level
clickByRole('button', { name: 'Action' }, 0) and clickByText('equals any of')
calls with scoped queries against that predicate container and apply the same
scoped change to the other occurrence referenced in the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bd055cbe-df41-428b-90f8-dfb844c77f7f
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (11)
frontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/src/routes/Governance/policies/CreatePolicy.test.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Governance/Policy/policyWizard.test.tsxfrontend/src/wizards/Placement/MatchExpression.cssfrontend/src/wizards/Placement/MatchExpression.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
Comment on lines
+1
to
+9
| .match-expression-field .pf-v6-c-menu-toggle:not(.pf-m-typeahead) { | ||
| height: calc( | ||
| var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * | ||
| var(--pf-t--global--spacer--control--vertical--default) | ||
| ); | ||
| padding-block: 0; | ||
| display: flex; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
Stylelint error: newline after operator in calc().
Static analysis flags scss/operator-no-newline-after on line 3. Reformat to keep the multiplication on a single line.
🔧 Proposed fix
.match-expression-field .pf-v6-c-menu-toggle:not(.pf-m-typeahead) {
- height: calc(
- var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 *
- var(--pf-t--global--spacer--control--vertical--default)
- );
+ height: calc(var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * var(--pf-t--global--spacer--control--vertical--default));
padding-block: 0;
display: flex;
align-items: center;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .match-expression-field .pf-v6-c-menu-toggle:not(.pf-m-typeahead) { | |
| height: calc( | |
| var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * | |
| var(--pf-t--global--spacer--control--vertical--default) | |
| ); | |
| padding-block: 0; | |
| display: flex; | |
| align-items: center; | |
| } | |
| .match-expression-field .pf-v6-c-menu-toggle:not(.pf-m-typeahead) { | |
| height: calc(var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * var(--pf-t--global--spacer--control--vertical--default)); | |
| padding-block: 0; | |
| display: flex; | |
| align-items: center; | |
| } |
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 3-3: Unexpected newline after "*" (scss/operator-no-newline-after)
(scss/operator-no-newline-after)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/wizards/Placement/MatchExpression.css` around lines 1 - 9, The
calc() expression in the .match-expression-field
.pf-v6-c-menu-toggle:not(.pf-m-typeahead) rule is split across lines causing a
stylelint operator-newline error; reformat the calc() so the multiplication
operations remain on a single line (combine the font-size * line-height and the
2 * spacer terms onto one line each or the whole expression onto one line) while
preserving the same values and surrounding whitespace, leaving other
declarations (padding-block, display, align-items) unchanged.
Contributor
Author
|
/test check |
Randy424
force-pushed
the
ACM-30639-general-wizard-updates
branch
from
38fc5b1 to
884b578
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
frontend/src/wizards/Placement/MatchExpression.css (1)
2-4:⚠️ Potential issue | 🟡 MinorStylelint calc operator-newline issue remains unresolved.
The
calc()expression still breaks right after*, which violatesscss/operator-no-newline-afterand can fail lint.Proposed lint-safe formatting
.match-expression-field .pf-v6-c-menu-toggle:not(.pf-m-typeahead) { - height: calc( - var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * - var(--pf-t--global--spacer--control--vertical--default) - ); + height: calc(var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * var(--pf-t--global--spacer--control--vertical--default)); padding-block: 0; display: flex; align-items: center; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/MatchExpression.css` around lines 2 - 4, The calc() in MatchExpression.css (the height property) currently places the '*' operator at the end of a line which violates scss/operator-no-newline-after; fix by reformatting the calc expression so arithmetic operators start each continued line (or put the entire calc on one line) — e.g., move each '*' to the beginning of the following line in the height declaration to ensure no newline immediately follows an operator.frontend/src/wizards/Argo/ArgoWizard.tsx (1)
916-959:⚠️ Potential issue | 🟠 MajorRe-created
Placement.specstill dropsnumberOfClustersdefaultOn Line 916 and Line 948, the recreated
Placement.specincludes tolerations but omitsnumberOfClusters: 1, while initial defaults include it (Line 352 and Line 428). Toggling to “Existing placement” and back to “New placement” can change selection behavior.Suggested fix
newResources.push( isPullModel ? ({ apiVersion: PlacementApiVersion, kind: PlacementKind, metadata: { name: '', namespace: '' }, spec: { tolerations: [ { key: 'cluster.open-cluster-management.io/unreachable', operator: 'Exists', }, { key: 'cluster.open-cluster-management.io/unavailable', operator: 'Exists', }, ], + numberOfClusters: 1, predicates: [ { requiredClusterSelector: { labelSelector: { matchExpressions: [ { key: 'local-cluster', operator: 'NotIn', values: ['true'], }, ], }, }, }, ], }, } as IResource) : ({ apiVersion: PlacementApiVersion, kind: PlacementKind, metadata: { name: '', namespace: '' }, spec: { tolerations: [ { key: 'cluster.open-cluster-management.io/unreachable', operator: 'Exists', }, { key: 'cluster.open-cluster-management.io/unavailable', operator: 'Exists', }, ], + numberOfClusters: 1, }, } as IResource) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Argo/ArgoWizard.tsx` around lines 916 - 959, The reproduced Placement.spec objects in ArgoWizard.tsx (the two places building a Placement IResource in the ternary branches around the predicates/tolerations blocks) are missing the default numberOfClusters: 1; update both constructed spec objects (the one cast as IResource and the alternate Placement object with apiVersion/ kind/ metadata) to include numberOfClusters: 1 alongside tolerations (and predicates where present) so toggling between “Existing placement” and “New placement” preserves the original default selection behavior.
🧹 Nitpick comments (2)
frontend/src/wizards/Placement/MatchExpression.tsx (1)
24-44: Consider removing duplicated inline max-width styling.
style={{ maxWidth: 200 }}is duplicated across both field wrappers; moving this into a shared CSS rule would keep layout styling centralized and easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/MatchExpression.tsx` around lines 24 - 44, Remove the duplicated inline style={{ maxWidth: 200 }} from the two divs in MatchExpression.tsx and centralize the layout styling by adding a CSS rule for the existing class selector "match-expression-field" (e.g., .match-expression-field { max-width: 200px; }) in the component's stylesheet or CSS module; update the JSX to rely on the className only (no inline style) so both wrappers (the divs wrapping WizLabelSelect and WizTextInput) inherit the shared max-width.frontend/src/wizards/Placement/PlacementSection.tsx (1)
364-376: Consider centralizing default reachability tolerationsThe same toleration array is now duplicated across placement creation paths. Extracting a shared constant/helper would reduce drift risk when defaults evolve.
Refactor sketch
+const defaultReachabilityTolerations = [ + { key: 'cluster.open-cluster-management.io/unreachable', operator: 'Exists' }, + { key: 'cluster.open-cluster-management.io/unavailable', operator: 'Exists' }, +] ... newResources.push({ apiVersion: PlacementApiVersion, kind: PlacementKind, metadata: { name: placementName, namespace }, spec: { - tolerations: [ - { - key: 'cluster.open-cluster-management.io/unreachable', - operator: 'Exists', - }, - { - key: 'cluster.open-cluster-management.io/unavailable', - operator: 'Exists', - }, - ], + tolerations: defaultReachabilityTolerations, ...props.defaultPlacementSpec, }, } as IResource)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/PlacementSection.tsx` around lines 364 - 376, Duplicate reachability tolerations are being inlined into placement specs; extract them into a single exported constant or helper (e.g., DEFAULT_REACHABILITY_TOLERATIONS) and use it wherever placements are created instead of repeating the array. Update PlacementSection.tsx to replace the inline tolerations under spec with a spread or reference to that constant and adjust other placement creation paths to import and reuse the same symbol so defaults remain centralized and easy to update; keep the existing spread of props.defaultPlacementSpec but move the literal tolerations into the shared constant/helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/wizards/Argo/ArgoWizard.tsx`:
- Around line 916-959: The reproduced Placement.spec objects in ArgoWizard.tsx
(the two places building a Placement IResource in the ternary branches around
the predicates/tolerations blocks) are missing the default numberOfClusters: 1;
update both constructed spec objects (the one cast as IResource and the
alternate Placement object with apiVersion/ kind/ metadata) to include
numberOfClusters: 1 alongside tolerations (and predicates where present) so
toggling between “Existing placement” and “New placement” preserves the original
default selection behavior.
In `@frontend/src/wizards/Placement/MatchExpression.css`:
- Around line 2-4: The calc() in MatchExpression.css (the height property)
currently places the '*' operator at the end of a line which violates
scss/operator-no-newline-after; fix by reformatting the calc expression so
arithmetic operators start each continued line (or put the entire calc on one
line) — e.g., move each '*' to the beginning of the following line in the height
declaration to ensure no newline immediately follows an operator.
---
Nitpick comments:
In `@frontend/src/wizards/Placement/MatchExpression.tsx`:
- Around line 24-44: Remove the duplicated inline style={{ maxWidth: 200 }} from
the two divs in MatchExpression.tsx and centralize the layout styling by adding
a CSS rule for the existing class selector "match-expression-field" (e.g.,
.match-expression-field { max-width: 200px; }) in the component's stylesheet or
CSS module; update the JSX to rely on the className only (no inline style) so
both wrappers (the divs wrapping WizLabelSelect and WizTextInput) inherit the
shared max-width.
In `@frontend/src/wizards/Placement/PlacementSection.tsx`:
- Around line 364-376: Duplicate reachability tolerations are being inlined into
placement specs; extract them into a single exported constant or helper (e.g.,
DEFAULT_REACHABILITY_TOLERATIONS) and use it wherever placements are created
instead of repeating the array. Update PlacementSection.tsx to replace the
inline tolerations under spec with a spread or reference to that constant and
adjust other placement creation paths to import and reuse the same symbol so
defaults remain centralized and easy to update; keep the existing spread of
props.defaultPlacementSpec but move the literal tolerations into the shared
constant/helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3613771d-eb3a-44b6-bf72-b8ba986fdd87
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (11)
frontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/src/routes/Governance/policies/CreatePolicy.test.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Governance/Policy/policyWizard.test.tsxfrontend/src/wizards/Placement/MatchExpression.cssfrontend/src/wizards/Placement/MatchExpression.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/routes/Governance/policies/CreatePolicy.test.tsx
- frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx
- frontend/src/wizards/Placement/Placement.tsx
Randy424
force-pushed
the
ACM-30639-general-wizard-updates
branch
from
884b578 to
b845e08
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/wizards/Placement/MatchExpression.css (1)
2-4:⚠️ Potential issue | 🟡 MinorFix stylelint operator-newline violation in
calc()formatting.Line 3 breaks after
*, which triggersscss/operator-no-newline-after. Keep multiplication terms on one line.Suggested fix
.match-expression-field .pf-v6-c-menu-toggle:not(.pf-m-typeahead) { - height: calc( - var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * - var(--pf-t--global--spacer--control--vertical--default) - ); + height: calc(var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * var(--pf-t--global--spacer--control--vertical--default)); padding-block: 0; display: flex; align-items: center; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/MatchExpression.css` around lines 2 - 4, The calc() expression for the height property currently breaks the line after the `*`, triggering the stylelint operator-newline rule; update the `height` declaration so the multiplication terms in the `calc(...)` are kept on the same line (i.e., reformat the `calc()` in the `height` rule so no `*` is followed by a newline) to satisfy `scss/operator-no-newline-after`.
🧹 Nitpick comments (2)
frontend/src/wizards/Argo/ArgoWizard.test.tsx (1)
221-226: Prefer scoped queries over index-based “Action” selectionUsing positional selection (
..., 0) and generic text matching ('equals any of') is fragile when layout/order changes. Prefer scoping withwithin(...)on the specific expression row/container to make these assertions stable.As per coding guidelines, "Check that tests actually test the described behavior".
Also applies to: 332-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Argo/ArgoWizard.test.tsx` around lines 221 - 226, Replace the fragile index-based and global queries in ArgoWizard.test.tsx (e.g., clickByRole('button', { name: 'Action' }, 0) and clickByText('equals any of')) by first locating the specific expression row/container (use within(container) or a scoped query on the expression element) and then calling scoped queries like within(expressionRow).getByRole('button', { name: 'Action' }) and within(expressionRow).getByRole('combobox', { name: 'Select the label' }) and within(expressionRow).getByText(/does not equal any of/i) so the test targets the correct row without relying on positional indices; update the similar block around lines 332-337 the same way.frontend/src/wizards/Governance/Policy/policyWizard.test.tsx (1)
294-320: Assert parsed YAML structure, not only text fragmentsThese checks validate presence of strings, but they don’t guarantee the tolerations are in the expected resource path (
Placement.spec.tolerations) or that both entries are structurally correct. Consider parsing the YAML and asserting the actual object shape/values.As per coding guidelines, "Verify test coverage is meaningful, not just for metrics" and "Check that tests actually test the described behavior".
Also applies to: 322-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx` around lines 294 - 320, The test "default tolerations are set when creating new placement" currently asserts text fragments in the Monaco editor; instead grab the editor value from the textbox role named "monaco" (the same element obtained via screen.getByRole in the test), parse it as YAML/JSON, and assert the parsed object has Placement.spec.tolerations as an array with two entries containing keys "cluster.open-cluster-management.io/unreachable" and "cluster.open-cluster-management.io/unavailable" and operator "Exists"; update the assertions to check the actual object shape/values rather than using expect(...).toHaveTextContent so the test verifies Placement.spec.tolerations structure and contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 183-196: The WizTextInput for the toleration value
("toleration-value", path "value") is only hidden based on operator but not
validated; update the component so that when toleration.operator === 'Equal' the
field is marked required (e.g., add a required prop or validation rule tied to
path "value") so the form validation prevents submission with operator 'Equal'
and an empty value; keep the existing hidden logic but augment it with a
conditional required/validator that references the same operator check used in
the hidden callback.
In `@frontend/src/wizards/Placement/PlacementSection.tsx`:
- Around line 364-376: The spec object currently spreads
props.defaultPlacementSpec after adding two baseline tolerations which allows
caller-provided tolerations to overwrite them; update PlacementSection (the spec
construction surrounding props.defaultPlacementSpec and tolerations) to merge
tolerations explicitly instead of overwriting: build a new tolerations array
that starts with the two baseline tolerations
(cluster.open-cluster-management.io/unreachable and .../unavailable) and then
appends props.defaultPlacementSpec.tolerations (if any), deduplicating by
key+operator, and finally spread the rest of props.defaultPlacementSpec
(excluding its tolerations) into spec so all other fields remain unchanged and
the baseline tolerations are preserved.
---
Duplicate comments:
In `@frontend/src/wizards/Placement/MatchExpression.css`:
- Around line 2-4: The calc() expression for the height property currently
breaks the line after the `*`, triggering the stylelint operator-newline rule;
update the `height` declaration so the multiplication terms in the `calc(...)`
are kept on the same line (i.e., reformat the `calc()` in the `height` rule so
no `*` is followed by a newline) to satisfy `scss/operator-no-newline-after`.
---
Nitpick comments:
In `@frontend/src/wizards/Argo/ArgoWizard.test.tsx`:
- Around line 221-226: Replace the fragile index-based and global queries in
ArgoWizard.test.tsx (e.g., clickByRole('button', { name: 'Action' }, 0) and
clickByText('equals any of')) by first locating the specific expression
row/container (use within(container) or a scoped query on the expression
element) and then calling scoped queries like
within(expressionRow).getByRole('button', { name: 'Action' }) and
within(expressionRow).getByRole('combobox', { name: 'Select the label' }) and
within(expressionRow).getByText(/does not equal any of/i) so the test targets
the correct row without relying on positional indices; update the similar block
around lines 332-337 the same way.
In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx`:
- Around line 294-320: The test "default tolerations are set when creating new
placement" currently asserts text fragments in the Monaco editor; instead grab
the editor value from the textbox role named "monaco" (the same element obtained
via screen.getByRole in the test), parse it as YAML/JSON, and assert the parsed
object has Placement.spec.tolerations as an array with two entries containing
keys "cluster.open-cluster-management.io/unreachable" and
"cluster.open-cluster-management.io/unavailable" and operator "Exists"; update
the assertions to check the actual object shape/values rather than using
expect(...).toHaveTextContent so the test verifies Placement.spec.tolerations
structure and contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fad63aa-be54-43a5-ac68-9a256050f276
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (11)
frontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/src/routes/Governance/policies/CreatePolicy.test.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Governance/Policy/policyWizard.test.tsxfrontend/src/wizards/Placement/MatchExpression.cssfrontend/src/wizards/Placement/MatchExpression.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/react-form-wizard/src/index.ts
- frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/routes/Governance/policies/CreatePolicy.test.tsx
- frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx
- frontend/src/wizards/Argo/ArgoWizard.tsx
- frontend/src/wizards/Placement/MatchExpression.tsx
Randy424
force-pushed
the
ACM-30639-general-wizard-updates
branch
from
b845e08 to
45a5da1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/wizards/Placement/MatchExpression.css (1)
2-4:⚠️ Potential issue | 🟡 MinorFix stylelint operator-newline violation in
calc().Line 3 still has a newline after
*, which violatesscss/operator-no-newline-after. Keep multiplication terms on the same line.Suggested fix
.match-expression-field .pf-v6-c-menu-toggle:not(.pf-m-typeahead) { - height: calc( - var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * - var(--pf-t--global--spacer--control--vertical--default) - ); + height: calc(var(--pf-t--global--font--size--body--default) * var(--pf-t--global--font--line-height--body) + 2 * var(--pf-t--global--spacer--control--vertical--default)); padding-block: 0; display: flex; align-items: center; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/MatchExpression.css` around lines 2 - 4, The height property's calc() expression in MatchExpression.css breaks the scss/operator-no-newline-after rule because a multiplication operator is followed by a newline; update the calc() value so multiplication terms stay on the same line (e.g., make the entire calc(...) or each multiplication segment contiguous) — target the height declaration's calc() expression and remove the newline after the '*' so the multiplication terms are on one line.
🧹 Nitpick comments (1)
frontend/src/wizards/Governance/Policy/policyWizard.test.tsx (1)
317-320: Assert the toleration list shape, not just presence.These checks still pass if the defaults are duplicated after the existing→new placement switch. That regression is plausible here because
frontend/src/wizards/Placement/PlacementSection.tsx:355-375prepends the default tolerations whenever a newPlacementis created. Please assert the YAML contains exactly the expected two tolerations, not just these substrings.As per coding guidelines, "Verify test coverage is meaningful, not just for metrics" and "Check that tests actually test the described behavior."
Also applies to: 351-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx` around lines 317 - 320, The test currently only checks for substrings and can pass if defaults are duplicated; update the assertions in policyWizard.test.tsx (the expects around lines 351-353 and 317-320) to parse the rendered YAML/tolerations value and assert the exact array shape and length (exactly two tolerations with the expected keys/operators and no duplicates) rather than using toHaveTextContent on substrings; locate the tolerations output (the same element referenced as input) and verify its parsed structure (length === 2 and both objects match the expected {key: 'cluster.open-cluster-management.io/unreachable', operator: 'Exists'} and {key: 'cluster.open-cluster-management.io/unavailable', operator: 'Exists'}) to prevent the PlacementSection.tsx default-prepend regression from slipping through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx`:
- Line 105: The condition uses a non-existent enum member "DisplayMode.Details";
update the check to use a valid member from the DisplayMode enum (e.g., replace
"DisplayMode.Details" with "DisplayMode.StepsHidden" or "DisplayMode.Step" as
appropriate for the intended behavior) in the if statement that reads "if (mode
=== DisplayMode.Details)" in WizLabelSelect.tsx so it compiles against the
defined DisplayMode enum.
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 175-202: The operator/effect options are rendered as raw enum
strings; update WizSingleSelect (id="toleration-operator") and WizLabelSelect
(id="toleration-effect") to supply options as { label: t('...'), value:
'EnumLiteral' } objects (use t('Exists'), t('Equal'), t('NoSelect'),
t('PreferNoSelect'), t('NoSelectIfNew') for labels) so UI strings go through
translation while stored values remain the API enums; keep the WizTextInput
hidden/validation logic unchanged (it should still compare the selected value to
'Equal') so behavior is preserved.
---
Duplicate comments:
In `@frontend/src/wizards/Placement/MatchExpression.css`:
- Around line 2-4: The height property's calc() expression in
MatchExpression.css breaks the scss/operator-no-newline-after rule because a
multiplication operator is followed by a newline; update the calc() value so
multiplication terms stay on the same line (e.g., make the entire calc(...) or
each multiplication segment contiguous) — target the height declaration's calc()
expression and remove the newline after the '*' so the multiplication terms are
on one line.
---
Nitpick comments:
In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx`:
- Around line 317-320: The test currently only checks for substrings and can
pass if defaults are duplicated; update the assertions in policyWizard.test.tsx
(the expects around lines 351-353 and 317-320) to parse the rendered
YAML/tolerations value and assert the exact array shape and length (exactly two
tolerations with the expected keys/operators and no duplicates) rather than
using toHaveTextContent on substrings; locate the tolerations output (the same
element referenced as input) and verify its parsed structure (length === 2 and
both objects match the expected {key:
'cluster.open-cluster-management.io/unreachable', operator: 'Exists'} and {key:
'cluster.open-cluster-management.io/unavailable', operator: 'Exists'}) to
prevent the PlacementSection.tsx default-prepend regression from slipping
through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac36c67d-0bf2-49d9-b233-3f7841021b92
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (11)
frontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/src/routes/Governance/policies/CreatePolicy.test.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Governance/Policy/policyWizard.test.tsxfrontend/src/wizards/Placement/MatchExpression.cssfrontend/src/wizards/Placement/MatchExpression.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/src/routes/Governance/policies/CreatePolicy.test.tsx
- frontend/src/wizards/Placement/MatchExpression.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/packages/react-form-wizard/src/index.ts
- frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx
- frontend/src/wizards/Placement/PlacementSection.tsx
- frontend/src/wizards/Argo/ArgoWizard.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx`:
- Around line 45-46: The code builds stringOptions from normalizedOptions
(stringOptions = useMemo(() => normalizedOptions.map(opt => opt.label), ...))
and later resolves selections by using find on label, which makes options with
duplicate labels ambiguous; update the logic to detect duplicates at build time
and fail fast (throw or console.error) when duplicate labels exist, and change
the selection resolution to use the option's unique identifier (opt.value)
rather than label; specifically modify the normalizedOptions -> stringOptions
creation to check for duplicate labels (e.g., build a Set or Map and error if a
label already exists) and update the code that currently uses find(opt =>
opt.label === selectedLabel) to resolve by value (find(opt => opt.value ===
selectedValue) or use the Map from label->value only if no duplicates) so
lookups are deterministic and duplicates are rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17eed498-dd08-4920-b17c-2b4ad0b63314
📒 Files selected for processing (2)
frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/wizards/Placement/PlacementSection.tsx
Contributor
|
Thanks @Randy424 ! The latest changes look great! Thanks for fixing the tolerations being removed bug! I'll put this on hold in case Kevin has anything. /hold |
6 tasks
Add expandable sections for label expressions and tolerations, default tolerations when creating new placements, WizLabelSelect component, and updated copy/styling across placement wizard steps. Includes tests verifying default tolerations persist across placement mode switches. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com>
…e order Add validation requiring value when toleration operator is Equal. Fix toleration merge order in PlacementSection to preserve baseline defaults when callers provide defaultPlacementSpec. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DisplayMode.Details was removed from the enum on main. Remove the unreachable details rendering path and unused imports. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wrap operator and effect option labels in t() for i18n compliance. Replace find-based label lookup with Map for deterministic resolution. Remove unused WizSingleSelect import from Placement.tsx. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevent duplicate tolerations when defaultPlacementSpec already includes baseline entries. Add exact count assertions to toleration tests. Detect duplicate labels in WizLabelSelect at dev time. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Randy424
force-pushed
the
ACM-30639-general-wizard-updates
branch
from
d25b866 to
cd592cb
Compare
Contributor
Author
|
Merge conflicts resolved. |
KevinFCormier
approved these changes
Apr 22, 2026
Contributor
|
/unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, KevinFCormier, Randy424 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add expandable sections for label expressions and tolerations, default tolerations when creating new placements, WizLabelSelect component, and updated copy/styling across placement wizard steps. Includes tests verifying default tolerations persist across placement mode switches.
📝 Summary
Ticket Summary (Title):
Ticket Link:
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
New Features
Improvements
Tests
Style
Preview: