[ACM-33148] Add placement preview to standalone placement wizard#6024
Merged
openshift-merge-bot[bot] merged 1 commit intostolostron:mainfrom Apr 23, 2026
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
frontend/src/routes/Governance/policies/CreatePolicy.test.tsx (1)
98-98: Avoid positional selection for the “action” buttonLine 98 relies on the first match from a non-unique role/name query, which can become flaky as UI composition changes. Prefer a scoped query tied to the placement control being exercised, so the test remains intent-specific.
As per coding guidelines, "Verify test coverage is meaningful, not just for metrics".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Governance/policies/CreatePolicy.test.tsx` at line 98, Replace the positional selection screen.getAllByRole('button', { name: /action/i })[0].click() with a scoped query so the test targets the specific placement control: first locate the placement control container (e.g., via getByLabelText, getByTestId or another nearby unique query for the control in CreatePolicy.test.tsx), then use within(container).getByRole('button', { name: /action/i }).click(); this removes the [0] index and ties the action button lookup to the relevant placement element (use within from `@testing-library/react`).frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx (1)
57-57: Use a deterministic selector instead of index-based button pickingLine 57 uses
[0]on multiple “action” buttons, which makes this step order-dependent and brittle. Please scope the query to the placement/label-expression section (e.g., withwithin(...)or a stable test id) so the test always clicks the intended control.As per coding guidelines, "Check that tests actually test the described behavior".
🤖 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 uses the brittle expression screen.getAllByRole('button', { name: /action/i })[0] which depends on DOM order; instead scope the query to the specific placement/label-expression section by using within(...) on the container element for that section (e.g., const section = within(getByTestId('placement-label-expression') or within(getByText('Placement / Label Expression').closest(...))) and then call section.getByRole('button', { name: /action/i }) to click the correct button, or add and use a stable data-testid on the intended button and replace the index-based selector with getByTestId.backend/src/routes/placementDebug.ts (1)
90-96: Verify: Null check onresponsemay be unnecessary.The callback to
request()receivesIncomingMessagewhich should always be defined when the callback fires. Theif (!response)guard seems defensive but may be unreachable. Consider whether this is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/routes/placementDebug.ts` around lines 90 - 96, The null-check on response inside the request callback is unnecessary because the callback receives an IncomingMessage when invoked; remove the if (!response) return respondInternalServerError(req, res) guard in the upstream = request(options, (response) => { ... }) callback, and instead ensure proper error handling by registering an error handler on upstream (upstream.on('error', err => respondInternalServerError(req, res))) so failures in the request path are handled there; keep the existing res.writeHead(response.statusCode ?? 500, ...) and response.pipe(res as unknown as NodeJS.WritableStream) unchanged.frontend/src/wizards/Placement/MatchedClustersModal.tsx (2)
43-47: Consider extracting inline styles to CSS.The
rowStyleobject is recreated on each render. Since these styles are static, consider moving them to a CSS file or memoizing withuseMemoto avoid object recreation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/MatchedClustersModal.tsx` around lines 43 - 47, The inline style object rowStyle in MatchedClustersModal.tsx is recreated on every render; move these static styles into a CSS/SCSS class (e.g., .matched-clusters-row) and apply that class to the element using className, or if you prefer to keep styles in JS, wrap rowStyle with useMemo inside the MatchedClustersModal component to memoize the object (e.g., useMemo(() => ({ padding: ..., backgroundColor: ..., borderBottom: ... }), [])) so the object reference is stable across renders.
35-41: Consider:totalClustersunused whenhasLimitis false.When
notMatchedClustersis empty, the title shows{{count}} clusters matchedusingmatchedClusters.length. ThetotalClustersprop is only used in thehasLimitcase. Verify this is intentional - if all clusters match,matchedClusters.length === totalClustersanyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/MatchedClustersModal.tsx` around lines 35 - 41, The title logic in MatchedClustersModal uses totalClusters only when hasLimit is true, leaving the false branch to show matchedClusters.length; update the title so the non-limit branch uses props.totalClusters (falling back to props.matchedClusters.length if undefined) for consistency and clarity, or add an explicit check that props.matchedClusters.length === props.totalClusters and document/assert it; adjust the title computation that references hasLimit, props.matchedClusters, and props.totalClusters accordingly.backend/test/routes/placementDebug.test.ts (1)
12-61: Assert mocked upstream/auth calls were actually consumed.These cases assert status codes, but they don’t explicitly verify that expected
nockinterceptors were hit. Addscope.isDone()checks (or anafterEachwithnock.isDone()) to prevent false positives if proxy/auth calls are skipped.As per coding guidelines,
**/*.test.{ts,tsx}: "Verify test coverage is meaningful, not just for metrics" and "Check that tests actually test the described behavior."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/routes/placementDebug.test.ts` around lines 12 - 61, Tests in placementDebug.test.ts currently assert status codes but don't verify that the nock interceptors were actually consumed, which can mask skipped proxy/auth calls; update each test (or add an afterEach) to capture the nock scope returned by nock(...) and/or nockAuth() and call scope.isDone() (or use nock.isDone()) to assert mocks were hit after the request; reference the nock calls in each test (the nock(upstreamHost).post('/debug/placements/') and nock('https://localhost:9443').post('/debug/placements/') scopes and any scope returned/used by nockAuth()) and fail the test if isDone() is false so the tests verify the expected upstream/auth calls actually occurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/routes/placementDebug.ts`:
- Around line 51-53: The conditional check after await getAuthenticatedToken in
placementDebug is unreachable because getAuthenticatedToken throws on auth
failure; wrap the call to getAuthenticatedToken(req, res) inside a try-catch in
placementDebug, assign the result to token in the try block, and in the catch
send an appropriate response (e.g., res.statusCode = 401; res.end(...)) or
rethrow; then remove the dead if (!token) return check. This targets the
placementDebug function and the getAuthenticatedToken call.
In `@frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx`:
- Around line 47-55: labelToValue currently keeps only the first value for a
repeated label which makes selection ambiguous; update the logic that derives
labelToValue (and the similar valueToLabel block around lines 67-75) to detect
duplicate labels in normalizedOptions and fail fast: scan normalizedOptions for
any duplicate opt.label and throw a clear Error (or call a provided validation
handler) that includes the offending label and the conflicting opt.value entries
so authors must provide unique labels (alternatively, if you prefer value-driven
selection instead, change the selection/keying code to use opt.value end-to-end
rather than opt.label). Ensure the checks reference the normalizedOptions array
and the labelToValue / valueToLabel symbols so duplicates are caught before UI
selection occurs.
In `@frontend/src/wizards/Placement/MatchExpression.css`:
- Around line 2-5: The SCSS lint error comes from a newline after the `*`
operator in the height calc expression inside MatchExpression.css; open the rule
that defines height (the calc(...) expression) and move the multiplication
operand(s) so the `*` operator and its right-hand operand appear on the same
line (e.g., keep "2 * var(--pf-t--global--spacer--control--vertical--default)"
on one line) to satisfy the scss/operator-no-newline-after rule and preserve the
existing spacing and semantics.
In `@frontend/src/wizards/Placement/Placement.test.tsx`:
- Around line 257-264: Update the failing test in Placement.test.tsx to use the
current number-input label rendered by the Placement component: replace the
stale data-testid selector "number-input-Limit the number of clusters selected"
with the new label-based selector that matches label={t('Number of clusters')}
(e.g., use the data-testid "number-input-Number of clusters" or switch to
getByLabelText('Number of clusters')) so the expectation targets the actual
rendered input in the Placement component.
- Around line 163-181: The tests still assert the removed
alertTitle/alertContent props on the Placement component; update them to assert
the new cluster-set guidance that the component renders via
WizMultiSelect.helperText instead. Locate the Placement component tests (the two
failing cases and the similar block at 243-249) and replace assertions that look
for alertTitle/alertContent text with assertions that query for the helperText
rendered by the WizMultiSelect instance (the guidance produced in the Placement
component's WizMultiSelect.helperText branch). Ensure the test renders Placement
with the same namespaceClusterSetNames/clusters inputs used in the component
logic and assert presence/absence of the helperText string rather than the old
alert text.
- Around line 58-78: The mock for '@patternfly-labs/react-form-wizard' renders
elements with id attributes but the tests query by data-testid; update the
mocked components inside the jest.mock block (WizArrayInput, WizCheckbox,
WizCustomWrapper, WizKeyValue, WizMultiSelect, WizNumberInput, WizTextInput) to
include matching data-testid attributes (e.g.,
data-testid={`array-input-${label}`}, data-testid={`checkbox-${label}`},
data-testid={`custom-wrapper-${label}`}, and add data-testid to the inner value
span such as data-testid="custom-wrapper-value") so test queries using
getByTestId(...) will find the nodes; keep existing id if needed but ensure
data-testid values mirror the same naming convention used in tests.
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 155-180: The review value must be serializable: instead of passing
a React node in the error branch to WizCustomWrapper's value, pass a plain
string (e.g. error.message || t('An unknown error occurred.')) and set
alertVariant to 'warning' when error is present; remove the Tooltip/Alert node
from the value prop so ReviewStep.tsx will not fallback to formatReviewValue but
will render the warning row correctly (update the conditional for alertVariant
and the value passed to WizCustomWrapper in Placement.tsx accordingly).
In `@frontend/src/wizards/Placement/PlacementSection.test.tsx`:
- Around line 53-89: The tests use screen.getByTestId/queryByTestId but the
mocks render id attributes, so update the mocks in PlacementSection.test.tsx:
change the mock return markup for Placement, Placements, PlacementBindings,
PlacementRule and the PatternFly mocks (Section, WizItemSelector,
WizSingleSelect) to include data-testid attributes (e.g.,
data-testid="placement-component", "placements-component", "placement-bindings",
"placement-rule", "section-<label>", "item-selector", "single-select-<label>")
or add matching data-testid alongside the existing id attributes so the test
queries find the mocked nodes; keep the same prop usage (props.useFeatureFlag,
label) so behavior is unchanged.
In `@frontend/src/wizards/Placement/PlacementSection.tsx`:
- Around line 445-446: Replace the hardcoded seconds string with a localized
string using the useTranslation() API: instead of rendering
{`${toleration.tolerationSeconds}s`} inside <Label>, call t(...) with an
interpolation key (e.g. t('{{count}}s', { count: toleration.tolerationSeconds })
or use <Trans> for more complex markup) so the unit "s" and number are
localizable; update the render in PlacementSection (the Label around
toleration.tolerationSeconds) to use that t()/<Trans> call.
- Around line 630-638: The deduplicateTolerations helper currently only keys on
t.key and t.operator, which incorrectly drops distinct tolerations that differ
by value, effect, or tolerationSeconds; update deduplicateTolerations to dedupe
by the full meaningful identity (e.g., include t.value, t.effect and
t.tolerationSeconds in the dedupe key or serialize the normalized toleration
object via JSON.stringify) so that distinct tolerations are preserved when
merging into defaultPlacementSpec.tolerations.
In `@frontend/src/wizards/Placement/usePlacementDebug.ts`:
- Around line 94-107: The catch block for postPlacementDebug currently treats
errors like successful responses by setting cachedSpecKey and cachedState;
change this so failed lookups are not cached: in the catch handler for
postPlacementDebug (the code that builds errorState using EMPTY_STATE and
error), call setState(errorState) but do NOT assign cachedSpecKey = fetchKey or
cachedState = errorState; keep the cache assignments only in the successful
response path (where the debug result is stored), and verify
clearPlacementDebugCache still clears cachedSpecKey/cachedState as expected.
---
Nitpick comments:
In `@backend/src/routes/placementDebug.ts`:
- Around line 90-96: The null-check on response inside the request callback is
unnecessary because the callback receives an IncomingMessage when invoked;
remove the if (!response) return respondInternalServerError(req, res) guard in
the upstream = request(options, (response) => { ... }) callback, and instead
ensure proper error handling by registering an error handler on upstream
(upstream.on('error', err => respondInternalServerError(req, res))) so failures
in the request path are handled there; keep the existing
res.writeHead(response.statusCode ?? 500, ...) and response.pipe(res as unknown
as NodeJS.WritableStream) unchanged.
In `@backend/test/routes/placementDebug.test.ts`:
- Around line 12-61: Tests in placementDebug.test.ts currently assert status
codes but don't verify that the nock interceptors were actually consumed, which
can mask skipped proxy/auth calls; update each test (or add an afterEach) to
capture the nock scope returned by nock(...) and/or nockAuth() and call
scope.isDone() (or use nock.isDone()) to assert mocks were hit after the
request; reference the nock calls in each test (the
nock(upstreamHost).post('/debug/placements/') and
nock('https://localhost:9443').post('/debug/placements/') scopes and any scope
returned/used by nockAuth()) and fail the test if isDone() is false so the tests
verify the expected upstream/auth calls actually occurred.
In `@frontend/src/routes/Governance/policies/CreatePolicy.test.tsx`:
- Line 98: Replace the positional selection screen.getAllByRole('button', {
name: /action/i })[0].click() with a scoped query so the test targets the
specific placement control: first locate the placement control container (e.g.,
via getByLabelText, getByTestId or another nearby unique query for the control
in CreatePolicy.test.tsx), then use within(container).getByRole('button', {
name: /action/i }).click(); this removes the [0] index and ties the action
button lookup to the relevant placement element (use within from
`@testing-library/react`).
In `@frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx`:
- Line 57: The test currently uses the brittle expression
screen.getAllByRole('button', { name: /action/i })[0] which depends on DOM
order; instead scope the query to the specific placement/label-expression
section by using within(...) on the container element for that section (e.g.,
const section = within(getByTestId('placement-label-expression') or
within(getByText('Placement / Label Expression').closest(...))) and then call
section.getByRole('button', { name: /action/i }) to click the correct button, or
add and use a stable data-testid on the intended button and replace the
index-based selector with getByTestId.
In `@frontend/src/wizards/Placement/MatchedClustersModal.tsx`:
- Around line 43-47: The inline style object rowStyle in
MatchedClustersModal.tsx is recreated on every render; move these static styles
into a CSS/SCSS class (e.g., .matched-clusters-row) and apply that class to the
element using className, or if you prefer to keep styles in JS, wrap rowStyle
with useMemo inside the MatchedClustersModal component to memoize the object
(e.g., useMemo(() => ({ padding: ..., backgroundColor: ..., borderBottom: ...
}), [])) so the object reference is stable across renders.
- Around line 35-41: The title logic in MatchedClustersModal uses totalClusters
only when hasLimit is true, leaving the false branch to show
matchedClusters.length; update the title so the non-limit branch uses
props.totalClusters (falling back to props.matchedClusters.length if undefined)
for consistency and clarity, or add an explicit check that
props.matchedClusters.length === props.totalClusters and document/assert it;
adjust the title computation that references hasLimit, props.matchedClusters,
and props.totalClusters accordingly.
🪄 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: c403d42a-7217-460f-aa41-b34bb935abc9
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (30)
backend/src/app.tsbackend/src/routes/placementDebug.tsbackend/test/routes/placementDebug.test.tsfrontend/packages/react-form-wizard/src/Wizard.tsxfrontend/packages/react-form-wizard/src/contexts/FooterContentProvider.tsxfrontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/WizCustomWrapper.tsxfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsxfrontend/src/resources/managed-cluster.tsfrontend/src/resources/placement-debug.test.tsfrontend/src/resources/placement-debug.tsfrontend/src/routes/Governance/policies/CreatePolicy.test.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/CreatePlacement/PlacementWizard.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Governance/Policy/policyWizard.test.tsxfrontend/src/wizards/Governance/PolicySet/PolicySetWizard.test.tsxfrontend/src/wizards/Placement/MatchExpression.cssfrontend/src/wizards/Placement/MatchExpression.tsxfrontend/src/wizards/Placement/MatchedClustersModal.test.tsxfrontend/src/wizards/Placement/MatchedClustersModal.tsxfrontend/src/wizards/Placement/Placement.test.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.test.tsxfrontend/src/wizards/Placement/PlacementSection.tsxfrontend/src/wizards/Placement/usePlacementDebug.test.tsfrontend/src/wizards/Placement/usePlacementDebug.ts
Randy424
force-pushed
the
ACM-33148-standalone-placement-preview
branch
from
138bad3 to
dca3c7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/wizards/Placement/PlacementSection.tsx (2)
372-373:⚠️ Potential issue | 🟡 MinorLocalize the toleration-seconds label.
{${toleration.tolerationSeconds}s}bypassest(), so this user-facing text cannot be translated.As per coding guidelines, "Ensure all user-facing strings use useTranslation() or from src/lib/acm-i18next."
🤖 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 372 - 373, The label for toleration seconds currently concatenates a raw "s" suffix using `{`${toleration.tolerationSeconds}s`}`, bypassing localization; update the rendering inside PlacementSection (the Label around toleration.tolerationSeconds) to use the i18n helper (e.g., the t() function from useTranslation or a <Trans />) so the unit/suffix is translated—call t with a key like 'seconds' and pass toleration.tolerationSeconds as a variable (or use a translatable template) and render the localized string in the Label component.
537-545:⚠️ Potential issue | 🟠 MajorDeduping on
key+operatordrops valid tolerations.Two tolerations can share
keyandoperatorwhile differing invalue,effect, ortolerationSeconds. Consider including all meaningful fields in the dedupe key.Suggested fix
function deduplicateTolerations(tolerations: Toleration[]): Toleration[] { const seen = new Set<string>() return tolerations.filter((t) => { - const key = `${t.key}::${t.operator ?? ''}` + const key = [ + t.key ?? '', + t.operator ?? '', + t.value ?? '', + t.effect ?? '', + t.tolerationSeconds ?? '', + ].join('::') if (seen.has(key)) return false seen.add(key) return true }) }🤖 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 537 - 545, The deduplication in deduplicateTolerations currently only uses t.key and t.operator which can incorrectly drop distinct tolerations; update dedupe logic in deduplicateTolerations to build the uniqueness key from all meaningful fields (e.g., t.key, t.operator, t.value, t.effect, and t.tolerationSeconds) so that tolerations differing by value/effect/seconds are preserved, and continue using a Set<string> to filter by that composite key.
🧹 Nitpick comments (1)
frontend/src/wizards/Governance/Policy/policyWizard.test.tsx (1)
315-327: Extract repeated YAML toleration assertions into a helper.The same YAML-read + assertions block appears twice; consolidating it will reduce maintenance friction.
♻️ Proposed refactor
+function expectDefaultPlacementTolerationsInYaml() { + const input = screen.getByRole('textbox', { name: /monaco/i }) as HTMLTextAreaElement + const yamlContent = input.textContent ?? '' + expect(yamlContent).toContain('key: cluster.open-cluster-management.io/unreachable') + expect(yamlContent).toContain('key: cluster.open-cluster-management.io/unavailable') + expect(yamlContent).toContain('operator: Exists') + const tolerationMatches = yamlContent.match(/- key: cluster\.open-cluster-management\.io\//g) + expect(tolerationMatches).toHaveLength(2) +} ... - const input = screen.getByRole('textbox', { name: /monaco/i }) as HTMLTextAreaElement - const yamlContent = input.textContent ?? '' - expect(yamlContent).toContain('key: cluster.open-cluster-management.io/unreachable') - expect(yamlContent).toContain('key: cluster.open-cluster-management.io/unavailable') - expect(yamlContent).toContain('operator: Exists') - const tolerationMatches = yamlContent.match(/- key: cluster\.open-cluster-management\.io\//g) - expect(tolerationMatches).toHaveLength(2) + expectDefaultPlacementTolerationsInYaml() ... - const input = screen.getByRole('textbox', { name: /monaco/i }) as HTMLTextAreaElement - const yamlContent = input.textContent ?? '' - expect(yamlContent).toContain('key: cluster.open-cluster-management.io/unreachable') - expect(yamlContent).toContain('key: cluster.open-cluster-management.io/unavailable') - expect(yamlContent).toContain('operator: Exists') - const tolerationMatches = yamlContent.match(/- key: cluster\.open-cluster-management\.io\//g) - expect(tolerationMatches).toHaveLength(2) + expectDefaultPlacementTolerationsInYaml()Also applies to: 352-364
🤖 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 315 - 327, Extract the repeated YAML-read and assertions into two small helpers: getMonacoYaml() to locate the Monaco textbox (using screen.getByRole('textbox', { name: /monaco/i })) and return its textContent (or ''), and assertTolerationsInYaml(yaml: string) which runs the three expect(...) checks (contains both toleration keys, contains 'operator: Exists', and the regex match count equals 2). Replace both duplicated blocks in the test with const yaml = getMonacoYaml(); assertTolerationsInYaml(yaml); so the assertions are centralized and reusable.
🤖 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/review/ReviewStep.tsx`:
- Around line 917-932: The Alert rendering branch in ReviewStep (the block
checking n.alertVariant) can produce an empty title when n.value === true
because formatReviewValue(true) returns an empty string; update that branch to
compute title by preferring a non-empty string from n.value (if string), then
formatReviewValue(n.value), and if that result is empty, fall back to n.label or
n.path so Alert (component <Alert />) always receives a meaningful title; keep
the existing isInline and style props and preserve precedingDlGroup handling.
---
Duplicate comments:
In `@frontend/src/wizards/Placement/PlacementSection.tsx`:
- Around line 372-373: The label for toleration seconds currently concatenates a
raw "s" suffix using `{`${toleration.tolerationSeconds}s`}`, bypassing
localization; update the rendering inside PlacementSection (the Label around
toleration.tolerationSeconds) to use the i18n helper (e.g., the t() function
from useTranslation or a <Trans />) so the unit/suffix is translated—call t with
a key like 'seconds' and pass toleration.tolerationSeconds as a variable (or use
a translatable template) and render the localized string in the Label component.
- Around line 537-545: The deduplication in deduplicateTolerations currently
only uses t.key and t.operator which can incorrectly drop distinct tolerations;
update dedupe logic in deduplicateTolerations to build the uniqueness key from
all meaningful fields (e.g., t.key, t.operator, t.value, t.effect, and
t.tolerationSeconds) so that tolerations differing by value/effect/seconds are
preserved, and continue using a Set<string> to filter by that composite key.
---
Nitpick comments:
In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx`:
- Around line 315-327: Extract the repeated YAML-read and assertions into two
small helpers: getMonacoYaml() to locate the Monaco textbox (using
screen.getByRole('textbox', { name: /monaco/i })) and return its textContent (or
''), and assertTolerationsInYaml(yaml: string) which runs the three expect(...)
checks (contains both toleration keys, contains 'operator: Exists', and the
regex match count equals 2). Replace both duplicated blocks in the test with
const yaml = getMonacoYaml(); assertTolerationsInYaml(yaml); so the assertions
are centralized and reusable.
🪄 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: 209841d1-41e1-45a9-81e1-a2ac7efd7901
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (30)
backend/src/app.tsbackend/src/routes/placementDebug.tsbackend/test/routes/placementDebug.test.tsfrontend/packages/react-form-wizard/src/Wizard.tsxfrontend/packages/react-form-wizard/src/contexts/FooterContentProvider.tsxfrontend/packages/react-form-wizard/src/index.tsfrontend/packages/react-form-wizard/src/inputs/WizCustomWrapper.tsxfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsxfrontend/src/resources/managed-cluster.tsfrontend/src/resources/placement-debug.test.tsfrontend/src/resources/placement-debug.tsfrontend/src/routes/Governance/policies/CreatePolicy.test.tsxfrontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsxfrontend/src/routes/Infrastructure/Clusters/Placements/CreatePlacement/PlacementWizard.tsxfrontend/src/wizards/Argo/ArgoWizard.test.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Governance/Policy/policyWizard.test.tsxfrontend/src/wizards/Governance/PolicySet/PolicySetWizard.test.tsxfrontend/src/wizards/Placement/MatchExpression.cssfrontend/src/wizards/Placement/MatchExpression.tsxfrontend/src/wizards/Placement/MatchedClustersModal.test.tsxfrontend/src/wizards/Placement/MatchedClustersModal.tsxfrontend/src/wizards/Placement/Placement.test.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.test.tsxfrontend/src/wizards/Placement/PlacementSection.tsxfrontend/src/wizards/Placement/usePlacementDebug.test.tsfrontend/src/wizards/Placement/usePlacementDebug.ts
✅ Files skipped from review due to trivial changes (7)
- backend/src/app.ts
- frontend/packages/react-form-wizard/src/index.ts
- frontend/src/wizards/Governance/PolicySet/PolicySetWizard.test.tsx
- frontend/src/resources/placement-debug.test.ts
- frontend/src/resources/managed-cluster.ts
- frontend/src/wizards/Placement/Placement.test.tsx
- frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx
- frontend/src/routes/Infrastructure/Clusters/Placements/CreatePlacement/PlacementWizard.tsx
- frontend/packages/react-form-wizard/src/inputs/WizCustomWrapper.tsx
- frontend/src/routes/Governance/policies/CreatePolicy.test.tsx
- backend/test/routes/placementDebug.test.ts
- frontend/packages/react-form-wizard/src/contexts/FooterContentProvider.tsx
- frontend/src/resources/placement-debug.ts
- backend/src/routes/placementDebug.ts
- frontend/src/wizards/Placement/MatchExpression.tsx
- frontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsx
- frontend/src/wizards/Placement/MatchedClustersModal.test.tsx
- frontend/src/wizards/Placement/MatchedClustersModal.tsx
- frontend/src/wizards/Placement/usePlacementDebug.ts
- frontend/src/wizards/Placement/PlacementSection.test.tsx
- frontend/src/wizards/Placement/usePlacementDebug.test.ts
Randy424
changed the title
Wire the enhancedPlacement feature flag into the standalone placement wizard so it shows the matched clusters preview UI (footer status, modal, and review step alert), achieving feature parity with the policy and application wizards. 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-33148-standalone-placement-preview
branch
from
dca3c7f to
a7c9148
Compare
Randy424
changed the title
Contributor
Author
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, 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
2 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.
Summary
enhancedPlacementfeature flag andusePlacementDebughook into the standalone placement wizard (PlacementWizard.tsx) so it shows the matched clusters preview UIACM-30639-wizard-placement-enhancements-v2(placement preview backend + UI) andACM-30639-general-wizard-updates(general wizard UX improvements)isClusterSetvariable after mergeContext
The placement preview feature (matched cluster counts, footer link, clusters modal) was only wired up in the policy/policy-set wizards via
PlacementSection. The standalone placement wizard at Infrastructure > Clusters > Placements rendered thePlacementcomponent withoutuseFeatureFlagorplacementDebugState, so the preview never appeared.Jira: ACM-33148
Depends on: #5995 (placement preview) and ACM-30639-general-wizard-updates
Test plan
enhancedPlacementin console settings🤖 Generated with Claude Code
Summary by CodeRabbit