ACM-30639 Integrate PlacementDebugServer API into placement wizard#5995
Merged
openshift-merge-bot[bot] merged 16 commits intoApr 22, 2026
Merged
Conversation
|
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 backend POST /placement-debug proxy and end-to-end frontend placement-debug support: API/types, abortable debounced hook, UI (modal, footer provider), new input component, review metadata for alerts/non-editable inputs, and Placement UX guarded by an enhanced-placement feature flag.
sequenceDiagram
participant User as User
participant Frontend as Frontend (UI / usePlacementDebug)
participant Backend as Backend (/placement-debug)
participant Service as Placement Debug Service
User->>Frontend: Edit placement or open placement UI
Frontend->>Frontend: debounce, compute specKey
Frontend->>Backend: POST /placement-debug (placement payload + token)
Backend->>Service: Proxy POST (authorization header, content-length)
Service-->>Backend: Responds (status + JSON body/stream)
Backend-->>Frontend: Forward status + JSON body
Frontend->>Frontend: parse response -> set matched/notMatched/total state
Frontend->>User: render MatchedClustersModal / footer link / review updates
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 12
🤖 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/app.ts`:
- Line 70: The /placement-debug route buffers the entire request via
collectBody() without limits (vulnerable to memory exhaustion); update the
handler that registers placementDebug (router.post('/placement-debug',
placementDebug)) to enforce a maximum request body size (e.g., 1MB) before
buffering: first check Content-Length and reject >1MB with an appropriate
error/413 response, and/or modify collectBody() to stream and abort if the
accumulated bytes exceed the configured maxSize, returning a 413 and closing the
connection; ensure the token auth logic (already in place around the handler)
remains and return a clear error when refusing oversized requests.
In `@backend/src/routes/placementDebug.ts`:
- Around line 25-38: The handler placementDebug buffers the entire request via
collectBody before authenticating, allowing unauthenticated callers to force
large memory use; move the getToken(req) and unauthorized(req, res) check to run
before calling collectBody(req), and modify collectBody (or create
collectBodyWithLimit) to enforce a configurable max body size while streaming
(check cumulative chunk lengths and reject the promise if exceeded) so oversized
requests are aborted early; update placementDebug to call the size-limited
collector only after token validation and ensure you destroy/close the request
on rejection to avoid resource leaks.
In `@frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx`:
- Around line 44-45: filteredOptions is left empty when reopening a selected
value because once value is set the component renders PfSelect directly and
never repopulates filteredOptions; update the open handling (the state setter
used by open/setOpen) or the PfSelect open path to populate filteredOptions from
the original options list when the menu is opened (e.g., call
setFilteredOptions(options) or run the same filter logic used by InputSelect in
the open=true branch), ensuring SelectListOptions receives a populated array;
look for references to filteredOptions, setFilteredOptions, InputSelect,
PfSelect, SelectListOptions and the value/open state to implement this fix.
In `@frontend/packages/react-form-wizard/src/review/ReviewStep.tsx`:
- Around line 874-875: The collapsed summary badges are still editable because
ReviewCollapsedValueBadge is rendered without checking inputNode.nonEditable;
update the rendering logic so collapsed badges honor read-only fields by only
making them clickable when onReviewEdit != null && !inputNode.nonEditable (or
pass an explicit isEditable prop to ReviewCollapsedValueBadge), and ensure
ReviewPenHoverZone / ReviewCollapsedValueBadge use that flag to disable
click/edit behavior for nonEditable inputNode entries.
In `@frontend/src/wizards/Placement/MatchedClustersModal.tsx`:
- Around line 50-54: The "Learn more" link-style Button in MatchedClustersModal
is a dead focus target; either remove the interactive Button and render plain
text (replace the <Button variant="link" isInline> with a non-interactive <span>
or <Text> containing t('Learn more')), or wire it to the docs action by adding
an onClick handler (e.g., onClick={() => window.open(DOCS_URL, '_blank')} or
using your app's navigation helper) and include appropriate
aria-label/rel="noopener noreferrer" for security; update the Button element
inside MatchedClustersModal accordingly and ensure tests/aria checks reflect the
change.
In `@frontend/src/wizards/Placement/MatchExpression.css`:
- Around line 3-4: Remove the newline immediately after the multiplication
operator in the calc expression so the operator is not followed by a line break
(fixing the scss/operator-no-newline-after error); update the expression that
uses 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) to place the '*'
operators and their operands on the same line (or move the '*' to the end of the
preceding line) so the entire calc math is properly formatted.
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 110-118: usePlacementDebug returns loading and error but the
component only uses matchedCount to derive matchedLabel, so backend/TLS failures
show the same '-' as in-flight; update the rendering logic around
usePlacementDebug(placement, props.useFeatureFlag) and matchedLabel to
explicitly check loading and error (use the loading and error values from
usePlacementDebug) and surface them separately—show a loading state when loading
is true, and render an error alert/message when error is present (include
context like placement id/name) instead of treating both as the empty match
case; ensure the new logic is applied where matchedLabel is computed and in the
related UI block around matchedCount (lines handling matchedLabel / the 141-159
region) so errors are clearly visible to the user.
- Around line 224-227: The helper text in the Placement component shows an
unsupported toleration effect ("effect=NoSchedule"); update the example to use
one of the form's supported effects (e.g., "effect=PreferNoSelect" or
"effect=NoSelectIfNew") so the guidance matches the available options. Locate
the string passed to t(...) in Placement (the paragraph under the tolerations
helper) and replace the example effect value to one of the supported values, or
alternatively add "NoSchedule" to the toleration effect select options if you
prefer to support that behavior in the effect field.
- Around line 102-110: The Placement component currently passes an optional prop
directly into usePlacementDebug and renders footer content regardless of the
flag; fix by normalizing the flag to a strict boolean (e.g., const
featureEnabled = props.useFeatureFlag === true) and pass featureEnabled into
usePlacementDebug(placement, featureEnabled), then wrap any
footer/setFooterContent logic (the code block that sets footer content in step
mode) behind the same featureEnabled check so debug fetches and footer UI are
only active when the flag is true.
- Around line 254-260: The select for Toleration.effect currently includes an
empty string option which can persist effect: '' and violate the
Toleration.effect type; update the WizSingleSelect (id="toleration-effect",
path="effect") to either remove the '' option or provide an explicit "Any
effect" option that maps to undefined (e.g. use options as objects like [{label:
t('Any effect'), value: undefined}, {label: 'NoSelect', value: 'NoSelect'}, ...]
or handle the onChange to set undefined when the empty choice is picked) so the
form never serializes an empty string for effect.
In `@frontend/src/wizards/Placement/PlacementSection.tsx`:
- Around line 409-413: The fallback label 'Exists' is hard-coded in the
toleration render loop and must use translation; update the map in
PlacementSection where currentPlacement.spec.tolerations.map renders Label for
toleration.operator to call the translator instead of the literal string (e.g.,
use useTranslation() from src/lib/acm-i18next and replace the fallback with
t('Exists') or the appropriate i18n key), ensure useTranslation is imported and
invoked in the component and that the Label uses the translated value;
reference: currentPlacement.spec.tolerations.map, toleration.operator, and the
Label rendering.
- Line 188: The hook usePlacementDebug is being invoked unconditionally (it
defaults enabled=true) and thus triggers backend calls even when
settings.enhancedPlacement is off; fix by gating the hook call so it only runs
when the feature is enabled and a placement exists — either pass an explicit
enabled flag into usePlacementDebug (e.g., usePlacementDebug(currentPlacement, {
enabled: settings.enhancedPlacement && Boolean(currentPlacement) }) if the hook
supports it) or only call usePlacementDebug when settings.enhancedPlacement &&
currentPlacement and otherwise set matched, notMatched, matchedCount,
totalClusters to safe default values.
🪄 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: 4a289bd7-c4aa-47b3-b350-8689addd56ea
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (17)
backend/src/app.tsbackend/src/routes/placementDebug.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/WizCustomInputWrapper.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.tsfrontend/src/wizards/Placement/MatchExpression.cssfrontend/src/wizards/Placement/MatchExpression.tsxfrontend/src/wizards/Placement/MatchedClustersModal.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsxfrontend/src/wizards/Placement/usePlacementDebug.ts
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
frontend/src/wizards/Placement/Placement.tsx (1)
116-119:⚠️ Potential issue | 🟠 MajorDifferentiate loading/errors from “no match data.”
usePlacementDebugexposesloadinganderror, but both the footer label and the non-editable summary only branch onmatchedCount. A backend/TLS failure therefore renders the same-/neutral state as an in-flight request.As per coding guidelines, "Check for proper error boundaries and error handling".
Also applies to: 147-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/Placement.tsx` around lines 116 - 119, The footer label and non-editable summary currently only check matchedCount and treat loading/errors the same as "no data"; modify the logic around matchedLabel and the summary rendering to first inspect usePlacementDebug's loading and error flags (from the hook that provides matchedCount and totalClusters) and return distinct strings for those states (e.g., a translated "Loading…" when loading and a translated "Error: {{message}}" when error exists, including error.message if available) before falling back to the matchedCount / totalClusters display; update both the matchedLabel computation and the corresponding non-editable summary branch to use this loading/error-first branching so backend/TLS failures and in-flight requests are clearly differentiated from true "no match" results.
🧹 Nitpick comments (1)
frontend/src/wizards/Placement/Placement.tsx (1)
121-138: Keep footer content owned by one component.
PlacementSectionalready sets footer content for this flow, andFooterContentProviderholds a single shared node. Writing the footer from both parent and child makes the result effect-order dependent and cleanup-sensitive. Consolidating this into one owner would make the debug footer much less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/Placement/Placement.tsx` around lines 121 - 138, The footer is being set in this child component via useSetFooterContent (the setFooterContent call inside the useEffect in Placement.tsx), which conflicts with the parent PlacementSection/FooterContentProvider that already owns the shared footer node; remove this child-side footer mutation and move the matched-by placement UI into the single owner (e.g., PlacementSection) or expose a callback/prop from PlacementSection to render the matchedLabel/button there; specifically, delete the useEffect that calls setFooterContent and instead add a prop or handler on PlacementSection to render the matchedLabel Button and call openMatchedModal (or lift setIsMatchedClustersModalOpen) so only PlacementSection interacts with FooterContentProvider.
🤖 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 129-134: WizLabelSelect currently always attaches an onClose
handler to the Label which calls setValue(undefined), allowing the value to be
cleared even when the component is readonly or disabled; update the code so the
onClose is only attached/active when the input is neither readonly nor disabled
(e.g., conditionally pass onClose or make the handler early-return when
props.readonly || props.disabled), referencing the Label element and setValue in
WizLabelSelect to ensure the value cannot be cleared while readonly/disabled.
In `@frontend/packages/react-form-wizard/src/review/ReviewStep.tsx`:
- Around line 914-924: The alert-variant branch in ReviewStep.tsx currently only
uses n.value as the Alert.title, dropping any registered n.label from
WizCustomInputWrapper.tsx; update the Alert rendering so it preserves the label
(e.g., use n.label as the Alert title when present or combine label + formatted
value) by changing the title assignment for the Alert (key `alert-${n.path}`) to
include n.label if defined while still formatting n.value via formatReviewValue
when needed.
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 34-38: The collapsed badge in TolerationCollapsed renders raw
operator text; update TolerationCollapsed to use useTranslation() from
src/lib/acm-i18next, map the operator value (from useItem() as Toleration, e.g.,
operator?.toLowerCase() ?? 'exists') to a translation key (e.g.,
'toleration.operator.exists' / 'toleration.operator.equal' or similar) and pass
it through t(...) before rendering inside <Label>, so the displayed operator is
localized; ensure you import and call useTranslation() within
TolerationCollapsed and handle unknown operators with a fallback translated
string.
In `@frontend/src/wizards/Placement/PlacementSection.tsx`:
- Around line 371-379: The alert rendering currently only shows when
matchedCount !== undefined && totalClusters > 0, which hides zero-match and
placement-debug error states; update the PlacementSection component to also
render an inline Alert when totalClusters === 0 or matchedCount === 0
(zero-match) and to display an error Alert when the usePlacementDebug hook
indicates a failure (e.g., check the hook's error / status value such as
placementDebugError or placementDebug?.error). Keep the existing success/info
message when matches exist, but add separate branches for the zero-match warning
and for the placement-debug failure so both states are visible in Details mode;
reference matchedCount, totalClusters, and the usePlacementDebug hook when
making the conditional checks.
- Around line 412-418: The Details view currently skips
toleration.tolerationSeconds when it's 0 due to a truthy check; update the
render in the map over currentPlacement.spec.tolerations (inside the
Fragment/Label group) to test for null/undefined instead (e.g.,
toleration.tolerationSeconds != null or
Number.isFinite(toleration.tolerationSeconds)) and then render Label with
`${toleration.tolerationSeconds}s` so that 0 is shown.
---
Duplicate comments:
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 116-119: The footer label and non-editable summary currently only
check matchedCount and treat loading/errors the same as "no data"; modify the
logic around matchedLabel and the summary rendering to first inspect
usePlacementDebug's loading and error flags (from the hook that provides
matchedCount and totalClusters) and return distinct strings for those states
(e.g., a translated "Loading…" when loading and a translated "Error:
{{message}}" when error exists, including error.message if available) before
falling back to the matchedCount / totalClusters display; update both the
matchedLabel computation and the corresponding non-editable summary branch to
use this loading/error-first branching so backend/TLS failures and in-flight
requests are clearly differentiated from true "no match" results.
---
Nitpick comments:
In `@frontend/src/wizards/Placement/Placement.tsx`:
- Around line 121-138: The footer is being set in this child component via
useSetFooterContent (the setFooterContent call inside the useEffect in
Placement.tsx), which conflicts with the parent
PlacementSection/FooterContentProvider that already owns the shared footer node;
remove this child-side footer mutation and move the matched-by placement UI into
the single owner (e.g., PlacementSection) or expose a callback/prop from
PlacementSection to render the matchedLabel/button there; specifically, delete
the useEffect that calls setFooterContent and instead add a prop or handler on
PlacementSection to render the matchedLabel Button and call openMatchedModal (or
lift setIsMatchedClustersModalOpen) so only PlacementSection interacts with
FooterContentProvider.
🪄 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: 30e079fd-9158-4a2b-9a39-d5e42dc45367
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (6)
backend/src/routes/placementDebug.tsfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/src/wizards/Placement/MatchedClustersModal.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/wizards/Placement/MatchedClustersModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/routes/placementDebug.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx (1)
88-101: Potential logic issue inhandleSetOptionsmerge.When
displayLabelis not instringOptions(e.g., a previously selected value that's no longer a valid option), it gets added tofilteredat line 93. This could show stale/invalid options in the dropdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx` around lines 88 - 101, handleSetOptions currently unshifts displayLabel into filtered when displayLabel is absent from stringOptions, which can surface stale/invalid options; modify the logic in handleSetOptions so displayLabel is only included when it is actually present in the incoming options array (o.includes(displayLabel)) rather than merely missing from stringOptions—update the condition around filtered.unshift(displayLabel) and keep the final setFilteredOptions([...new Set([...filtered, ...o])]) behavior to preserve uniqueness.
🤖 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/PlacementSection.tsx`:
- Around line 372-387: The component currently renders zero-match and
match-count alerts but ignores the error returned by usePlacementDebug; update
PlacementSection to check the error from usePlacementDebug and render an inline
Alert (e.g., variant='danger') when error is truthy, showing the error message
(use error.message || String(error)) so API failures are visible to users; keep
the existing matchedCount/totalClusters logic and place the error Alert before
or above the matchedCount Alert to ensure visibility.
---
Nitpick comments:
In `@frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx`:
- Around line 88-101: handleSetOptions currently unshifts displayLabel into
filtered when displayLabel is absent from stringOptions, which can surface
stale/invalid options; modify the logic in handleSetOptions so displayLabel is
only included when it is actually present in the incoming options array
(o.includes(displayLabel)) rather than merely missing from stringOptions—update
the condition around filtered.unshift(displayLabel) and keep the final
setFilteredOptions([...new Set([...filtered, ...o])]) behavior to preserve
uniqueness.
🪄 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: 79212d5a-e215-4894-a517-48f753bad2ad
📒 Files selected for processing (5)
frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/src/wizards/Argo/ArgoWizard.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
ea880a4 to
af4f1f2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/wizards/Governance/Policy/policyWizard.test.tsx (1)
34-50: Consider centralizing test providers to reduce wrapper duplication.The same
RecoilRoot+Routernesting is repeated in 3 helpers. A small shared test-provider wrapper would make future provider changes safer and faster.Suggested refactor
+function TestProviders({ children }: { children: JSX.Element }) { + return ( + <RecoilRoot> + <Router>{children}</Router> + </RecoilRoot> + ) +} function TestPolicyWizard() { return ( - <RecoilRoot> - <Router> - <PolicyWizard ... /> - </Router> - </RecoilRoot> + <TestProviders> + <PolicyWizard ... /> + </TestProviders> ) } function TestPolicyWizardGK() { return ( - <RecoilRoot> - <Router> - <PolicyWizard ... /> - </Router> - </RecoilRoot> + <TestProviders> + <PolicyWizard ... /> + </TestProviders> ) } function TestPolicyWizardOperatorPolicy() { return ( - <RecoilRoot> - <Router> - <PolicyWizard ... /> - </Router> - </RecoilRoot> + <TestProviders> + <PolicyWizard ... /> + </TestProviders> ) }Also applies to: 75-92, 109-127
🤖 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 34 - 50, Multiple tests repeat the same RecoilRoot + Router wrapper around PolicyWizard; create a shared test provider wrapper component (e.g., TestProviders) that renders RecoilRoot and Router once and use it in the three helper blocks instead of duplicating the nesting; update the tests that render <PolicyWizard ...> to wrap it with <TestProviders> so places referencing RecoilRoot and Router (and tests using PolicyWizard, mockPolicy, mockPlacements, mockManagedClusters, mockClusterSet, mockClusterSetBinding) all reuse the centralized provider and remove the repeated wrapper code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/wizards/Governance/Policy/policyWizard.test.tsx`:
- Around line 34-50: Multiple tests repeat the same RecoilRoot + Router wrapper
around PolicyWizard; create a shared test provider wrapper component (e.g.,
TestProviders) that renders RecoilRoot and Router once and use it in the three
helper blocks instead of duplicating the nesting; update the tests that render
<PolicyWizard ...> to wrap it with <TestProviders> so places referencing
RecoilRoot and Router (and tests using PolicyWizard, mockPolicy, mockPlacements,
mockManagedClusters, mockClusterSet, mockClusterSetBinding) all reuse the
centralized provider and remove the repeated wrapper code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c6d190e-f337-4957-a676-b993481d3b9e
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (10)
frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.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/Governance/PolicySet/PolicySetWizard.test.tsxfrontend/src/wizards/Placement/Placement.tsxfrontend/src/wizards/Placement/PlacementSection.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/react-form-wizard/src/review/ReviewStep.tsx
- frontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsx
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
af4f1f2 to
7652f03
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/test/routes/placementDebug.test.ts (1)
5-27: Addnock.cleanAll()inafterEachto prevent interceptor leakage.Without cleanup, leftover interceptors from a failing test can affect subsequent tests. Also consider verifying the response body in the success case to confirm the proxy correctly forwards the payload.
describe(`placementDebug Route`, function () { + afterEach(() => { + nock.cleanAll() + }) + it(`proxies placement debug request to upstream service`, async function () {🤖 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 5 - 27, Tests in placementDebug Route leak nock interceptors across cases; add an afterEach hook that calls nock.cleanAll() (and optionally nock.restore() if you mock global behavior) to clear interceptors after each it block, and in the success test (where request('POST', '/placement-debug', { placement: 'test' }) is used) add an assertion on the response body (e.g., that it contains aggregatedScores: []) to ensure the proxy forwards the payload correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/test/routes/placementDebug.test.ts`:
- Around line 5-27: Tests in placementDebug Route leak nock interceptors across
cases; add an afterEach hook that calls nock.cleanAll() (and optionally
nock.restore() if you mock global behavior) to clear interceptors after each it
block, and in the success test (where request('POST', '/placement-debug', {
placement: 'test' }) is used) add an assertion on the response body (e.g., that
it contains aggregatedScores: []) to ensure the proxy forwards the payload
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bbaa141-c1b3-4b72-ba5f-20a79f5c0940
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (13)
backend/test/routes/placementDebug.test.tsfrontend/packages/react-form-wizard/src/inputs/WizLabelSelect.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.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/Governance/PolicySet/PolicySetWizard.test.tsxfrontend/src/wizards/Placement/Placement.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 (1)
- frontend/src/wizards/Governance/PolicySet/PolicySetWizard.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/routes/Governance/policies/CreatePolicy.test.tsx
- frontend/src/routes/Governance/policy-sets/CreatePolicySet.test.tsx
- frontend/src/wizards/Argo/ArgoWizard.test.tsx
- frontend/src/wizards/Governance/Policy/policyWizard.test.tsx
- frontend/src/wizards/Placement/usePlacementDebug.ts
Contributor
Author
|
/retest |
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
7652f03 to
9f5d336
Compare
Contributor
Author
|
/retest Prow is failing to schedule pods for our test pipeline. Restarting... |
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
2 times, most recently
from
c1f54dc to
363f011
Compare
Randy424
commented
Apr 20, 2026
Comment on lines
+33
to
+49
| function collectBody(req: Http2ServerRequest): Promise<Buffer> { | ||
| return new Promise((resolve, reject) => { | ||
| const chunks: Buffer[] = [] | ||
| let size = 0 | ||
| req.on('data', (chunk: Buffer) => { | ||
| size += chunk.length | ||
| if (size > MAX_BODY_SIZE) { | ||
| req.destroy() | ||
| reject(new Error('Request body too large')) | ||
| return | ||
| } | ||
| chunks.push(chunk) | ||
| }) | ||
| req.on('end', () => resolve(Buffer.concat(chunks))) | ||
| req.on('error', reject) | ||
| }) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Using collectBody here. It's store-and-forward proxy rather than a streaming proxy like search.ts
The pipeline approach (as seen in search.ts) doesn't work with the mock-request test infrastructure. The search.ts tests have // TODO - pipeline is not writing response comments where they can't assert on status codes.
With collectBody, our tests can actually verify status codes (200, 401, 413, 500), which is why all 6 backend tests pass and assert correctly. If we switched to pipeline, we'd lose the ability to test response status codes — the same limitation search.ts has.
Tradeoff: pipeline is more memory-efficient for large payloads (streams directly), while collectBody buffers the full request body (capped at 1MB). For placement debug payloads (small JSON placement specs), the 1MB buffer is fine and gives us better testability.
Contributor
There was a problem hiding this comment.
I'm OK with this for now, but I think we should explore this some more. The Ansible route uses pipeline and seems to have tests that do validate the response code.
fxiang1
reviewed
Apr 21, 2026
| "Match labels": "Match labels", | ||
| "Matched": "Matched", | ||
| "Matched by Placement": "Matched by Placement", | ||
| "Matched by Placement:": "Matched by Placement:", |
Contributor
There was a problem hiding this comment.
I wonder if we can just add the : outside of the translation function so we don't have duplicate this translation text.
Contributor
|
Thanks @Randy424 ! This looks great! I just pointed out some minor nits in the PR comments and concerns in slack thread https://redhat-internal.slack.com/archives/C02684L8JMV/p1776778522354229 |
Contributor
Author
|
Update: this branch will remain dedicated to PlacementDebugServer API integration and placement preview features, but changes dedicated to the general wizard updates will be introduced in a new branch/PR. |
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
1683d33 to
1d0e163
Compare
Contributor
Author
|
Hi @fxiang1, I've added an inline alert in the matched by placement footer result for when errors arise, I've paired it with a tooltip, let me know if that's OK or overkill. Additionally, now that I have rebased this branch on main, a defect from main has appeared in the wizard. I am tracking it here. |
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
12183d4 to
96db472
Compare
Contributor
|
Thanks @Randy424 ! The placement api error fix looks great! It never hurts to provide more info to the user :) I'll put this on hold in case Kevin has anything. /hold |
Contributor
|
@Randy424 I just checked the standalone placement wizard and the preview is not there, but given we're running out of time we should fix that in a bug or something. |
Contributor
Author
|
@fxiang1 I will open a defect and PR for it--thank you for the heads up 😵💫 ...And I guess this should apply to the changes in the wizard enhancement PR too. |
Wire usePlacementDebug hook into Placement and PlacementSection components behind the enhancedPlacement feature flag. Add matched cluster count in wizard footer, review step alert, expandable label expression and toleration sections, and WizLabelSelect integration in MatchExpression. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add 1MB body size limit and HTTP 413 response in placementDebug - Move auth check before body buffering to prevent unauthenticated memory consumption - Initialize and repopulate filteredOptions in WizLabelSelect on menu open - Honor nonEditable flag in collapsed review badges - Remove dead "Learn more" button from MatchedClustersModal - Normalize useFeatureFlag to strict boolean, gate footer content - Fix toleration helper text (NoSchedule → NoSelect), remove empty string from effect options - Gate usePlacementDebug in PlacementSection behind feature flag - Translate 'Exists' fallback label Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Pass enhancedPlacement flag to all Placement consumers - Add RecoilRoot to wizard tests for settingsState dependency - Gate WizLabelSelect onClose when disabled/readonly - Fix tolerationSeconds=0 truthy check - Show zero-match warning in review alert - Use getAuthenticatedToken in backend proxy route - Add MatchedClustersModal, usePlacementDebug, and backend tests Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Cache hook results to avoid refetch on wizard step changes - Remove X button from WizLabelSelect label pills - Remove unused ClusterScore export Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate non-placement-preview changes to separate branch: WizLabelSelect, MatchExpression styling, toleration expandable, label expandable, and copy updates. Address PR feedback by deduplicating translation key and surfacing API error messages. Fix proxy header forwarding order. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve issues introduced during rebase onto main: rename WizCustomInputWrapper to WizCustomWrapper to match main's rename, restore MatchExpression.tsx to main's version (DisplayMode.Details belongs to general-updates branch), replace DisplayMode.Details references with type-safe equivalents, and remove duplicate i18n keys. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Preserve HTTP status code and reason from ResourceError in the error message so users see actionable details (e.g. "500 Internal Server Error: upstream service unavailable"). Render errors as plain inline danger alerts with a tooltip for the full message, replacing the previous text-in-value approach. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move error alert inline next to the label instead of replacing the entire section, so "Matched by Placement:" remains visible with the danger alert and tooltip appearing where the count would be. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch error alerts from danger to warning variant, adjust footer padding to 0 top / 1rem bottom, and reset search term when the matched clusters modal reopens. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add cache-hit tests for usePlacementDebug (initial render and re-render paths), add body-shape test for placement-debug API client, switch error alerts to warning variant, adjust footer spacing, and reset modal search on reopen. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com>
PlacementRule was removed from main; update PlacementSection tests to drop IPlacementRule imports, mocks, and test cases that exercised the now-removed placement rule code paths. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create an OpenShift Route to the cluster-manager-placement service so the placement debug proxy works in standalone dev mode. Fix the backend to use the route hostname for Host header and TLS SNI when PLACEMENT_DEBUG_URL is set, so the OpenShift router can match the request. 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-wizard-placement-enhancements-v2
branch
from
931479a to
e9956e9
Compare
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
e9956e9 to
5bd6fe2
Compare
- Remove getDevAgent() from placementDebug.ts, always use
getServiceAgent() to align with search.ts and other proxy routes
- Simplify host header logic — set unconditionally like other services
- Rename useFeatureFlag prop to showPlacementPreview for clarity
- Fix i18n: use {{matched}} instead of {{count}} to avoid triggering
i18next pluralization; remove stale translation entries
- Add explanatory comment for eslint-disable in usePlacementDebug.ts
Spike ACM-33179 opened for collectBody investigation.
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-wizard-placement-enhancements-v2
branch
from
5bd6fe2 to
c0a17ef
Compare
KevinFCormier
previously approved these changes
Apr 22, 2026
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
e3cadd6 to
049c5ca
Compare
Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com>
Randy424
force-pushed
the
ACM-30639-wizard-placement-enhancements-v2
branch
from
049c5ca to
4055fbb
Compare
KevinFCormier
approved these changes
Apr 22, 2026
|
[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.
Summary
FooterContentProvidercontext,WizLabelSelectcomponent, andnonEditable/alertVariantsupport inWizCustomInputWrapperandReviewStepusePlacementDebughook with debouncing, abort handling, andenhancedPlacementfeature flag gatingusePlacementDebugintoPlacementandPlacementSectioncomponents. Add matched cluster count in wizard footer, review step alert, expandable label/toleration sections, andWizLabelSelectinMatchExpressionAll PlacementDebugServer functionality is gated behind the
enhancedPlacementfeature flag (settings.enhancedPlacement === 'enabled'). When disabled, the wizard behaves identically tomain.TLS Certificate Note
The PlacementDebugServer currently generates self-signed certificates at runtime, which are not signed by OpenShift's service-ca. A separate effort is in progress to add the
service.beta.openshift.io/serving-cert-secret-nameannotation to thecluster-manager-placementService so thatgetServiceAgent()can validate the certificate in production. Until that lands, the backend proxy usesgetServiceAgent()in production (will work once annotation is added) andgetDevAgent()whenPLACEMENT_DEBUG_URLis set (for local development).Local Development Setup
To test the PlacementDebugServer integration locally:
Enable the PlacementDebugServer feature gate on the cluster (if not already enabled):
oc patch PlacementConfiguration cluster --type merge -p '{"spec":{"featureGates":[{"feature":"PlacementDebugServer","mode":"Enable"}]}}'Verify the debug server is running (pods should show 2/2 containers):
Port-forward to the PlacementDebugServer:
Add the override to
setup.shline ~135 worked well for me:Start the console as usual (
npm startornpm run plugins). The port-forward must remain running in a separate terminal.Test plan
enhancedPlacementdisabled: verify wizard behaves identically tomain(no API calls, no footer count, no review alert), with the exception of Toleration expandable and some new copy from the figma.enhancedPlacementenabled + port-forward running: open Policy wizard → Placement stepenhancedPlacementenabled but port-forward NOT running: verify graceful degradation (shows "-" for count, no errors in console)WizLabelSelect,nonEditable,alertVariantrender correctly in non-placement wizard contexts🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI/UX Improvements
When the enhancedPlacement feature flag is enabled, a read-only alert appears at the top of the Placement form showing how many clusters match the current criteria. It updates live as you change cluster sets, label expressions, tolerations, or limits.
Placement step — footer link
A "Matched by Placement: X of Y clusters" link appears in the wizard footer. Clicking it opens the matched clusters modal.
Matched Clusters Modal
Lists matched and not-matched clusters with search filtering. When a numberOfClusters limit is set, clusters are split into "Matched" and "Not matched" sections with tooltip explanations. Without a limit, clusters appear in a flat list.
Review step — placement alert
The review/summary step shows a compact Alert reflecting the match result (same info/warning/danger variants as the form step).
Label expressions — pill style
Label expression dropdowns (operator, effect) now display selected values as outline label pills without an X/close button. Users change the value by reopening the dropdown.
Tests