ACM-33136 Review step when clicking yaml highlight button, if yaml doesn't exist nothing happens#6046
Conversation
📝 WalkthroughWalkthroughThis PR refactors input path serialization from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-33136-Review-step-when-clicking-yaml-highlight-button-if-yaml-doesn-t-exist-nothing-happens Signed-off-by: John Swanke <jswanke@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
frontend/src/wizards/Argo/ArgoWizard.tsx (1)
698-716: Helpers duplicated verbatim withfrontend/packages/react-form-wizard/wizards/Argo/ArgoWizard.tsx(lines 742-759).These two functions are byte-identical across both files and must drift in lock-step forever, which is a silent-drift hazard. Consider extracting to a shared module (e.g. under
frontend/packages/react-form-wizard/src) and importing from both call sites.Also, a minor robustness note on the fallback branch: for non-boolean/non-string
value(e.g.undefined/null), it produces#["PruneLast=undefined"], which still matches the hash-bracket regex consumer. Not reachable via the current wiring (pathValueToInputValuealways yields boolean/string), but worth a guard if this helper is reused elsewhere.🤖 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 698 - 716, Two helpers inputValueToPathString and prunePropagationPolicyCheckboxToPathString are duplicated verbatim and have a weak fallback for non-boolean/non-string values; extract both functions into a single shared utility module and import them from both ArgoWizard locations so they stay in sync, and update prunePropagationPolicyCheckboxToPathString (and the fallback branch of inputValueToPathString) to defensively handle null/undefined/other types (e.g., coerce to empty string or skip adding the key) to avoid producing "#[\"PruneLast=undefined\"]" while keeping the existing boolean/string behavior intact.frontend/packages/react-form-wizard/src/inputs/Input.ts (1)
161-174: Suffix concatenation is correct; one subtle consequence worth noting.
registrationPathis used both as the review-registration path (line 180) and — in non-test/non-Cypress mode — as the DOMid(line 174). SinceinputValueToPathString(value)is value-dependent, theid/registration key will change every time the user toggles the control (e.g.…syncOptions#["PruneLast=true"]↔…#["PruneLast=false"]). That causes theuseLayoutEffectat line 176 tounregisterthe old id andregistera new one on every value change, plus abumpReviewDomTree().For the handful of Argo sync-policy checkboxes this is fine, but if this pattern is reused for high-frequency inputs (e.g. text inputs using
inputValueToPathString) it will churn the registry on every keystroke. Worth a brief comment near line 166 warning future callers to only use this for low-cardinality/toggled inputs, or consider decoupling the registration key from the value (keepidstable, store the value-derived suffix only inside the registered meta).🤖 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/Input.ts` around lines 161 - 174, The current code appends inputValueToPathString(value) to registrationPath which makes the DOM id (id) change on every value change and churn the registry in useLayoutEffect; fix by decoupling the DOM id from the value-derived suffix: keep buildReviewInputRegistrationPath and the value-derived suffix only for the review registration key, but compute a stable id for the DOM (use convertId(props) or strip the inputValueToPathString part when assigning id), and add a brief comment by the buildReviewInputRegistrationPath/id assignment explaining that inputValueToPathString should only be used for low-cardinality toggles or for registration metadata to avoid frequent unregister/register cycles (referencing buildReviewInputRegistrationPath, inputValueToPathString, registrationPath, convertId, id, and the useLayoutEffect unregister/register flow).frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx (2)
125-147: Asymmetric kind handling for single vs. array item.When
itemis an array, you explicitly find a resource whosekind === kindHeadand bail if none. Whenitemis a single object, you skip that check and rely onget-valuereturningundefinedfor a wrong-kind prefix. That works, but consider guarding symmetrically for clarity and to avoid false positives if the singleitemhappens to have a property named the same as the path's kind segment:🔧 Symmetric kind validation
let target: unknown = item if (Array.isArray(item)) { target = item.find((res) => { if (!res || typeof res !== 'object') return false const rk = (res as { kind?: unknown }).kind return rk != null && String(rk) !== '' && String(rk) === kindHead }) if (!target) return false + } else if (item && typeof item === 'object') { + const rk = (item as { kind?: unknown }).kind + if (rk != null && String(rk) !== '' && String(rk) !== kindHead) return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 125 - 147, The function yamlPathBelongsToItem has asymmetric kind handling: when item is an array you check for a resource whose kind matches kindHead but when item is a single object you don't; add the same guard for single objects by extracting kindHead (already computed as kindHead) and, before calling yamlPathValueBelongsToTarget, verify that (item as { kind?: unknown }).kind is non-null and String(kind) === kindHead — if it does not match return false; place this check in the branch where target is an object (inside yamlPathBelongsToItem) so single-object items are validated the same way as array elements.
95-113: Path-escape round-trip relies onget-value's key-with-dot fallback.
normalized(from line 128) already replaced\.with., so whenbasePathincludes a YAML key that contains dots (e.g. annotations likeapp.kubernetes.io/name), the lookup depends onget-value's built-in rejoin-on-miss behavior to find the real key. That does work with the defaultjoinChar: '.', but if the key separator in your objects is ever something other than.(e.g. after Monaco-side transformation), this will silently miss.Also:
m[1]at line 107 is typedstring | undefinedundernoUncheckedIndexedAccess; add a guard or non-null assertion consistent with the rest of the file (e.g.m[1]!).🔧 Minor guard for capture group
if (pathWasAppended && Array.isArray(got)) { - const key = bracketAnnotationKey(m[1]) + const key = bracketAnnotationKey(m[1] ?? '') if (key === '') return false return got.some((el) => typeof el === 'string' && arrayStringStartsWithKeyAtNonAlphanumericBoundary(el, key)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx` around lines 95 - 113, The lookup currently relies on get-value's join-on-miss behavior and also uses m[1] without a non-null guard; update yamlPathValueBelongsToTarget to first try get(target, resourcePath) (preserving any original key tokens) before falling back to get(target, basePath) so we don't silently depend on get-value's dot-rejoin behavior, and change m[1] to a non-null assertion (m[1]!) or add a guard consistent with the file; keep the existing array-handling logic but reference m[1]! when computing the bracket key.frontend/packages/react-form-wizard/src/review/utils.ts (1)
149-174:simplifyLabelWordSlashesis a blanket transform — verify edge cases.Concerns with applying this to every label in the review tree:
- A word ending in
/collapses to an empty string ("foo/".slice(4) === ""), turning"foo/ bar"into" bar".- Any user-authored label containing
/(e.g."CPU/Memory","Read/Write") loses its prefix. The transform appears targeted at Kubernetes-style keys likeapp.kubernetes.io/name → name, but it runs on all labels unconditionally.- A leading-slash word like
"/path"becomes"path".If the intent is specifically k8s-prefixed label keys, consider scoping (e.g. only when prefix matches a known domain pattern) or at minimum guarding against the empty-word case.
🔧 Optional guard for empty-tail words
function simplifyLabelWordSlashes(label: string): string { return label.replace(/\S+/g, (word) => { const i = word.indexOf('/') if (i === -1) return word - return word.slice(i + 1) + const tail = word.slice(i + 1) + return tail === '' ? word : tail }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/react-form-wizard/src/review/utils.ts` around lines 149 - 174, simplifyLabelWordSlashes currently strips everything before the first '/' on any whitespace-delimited word, which drops trailing-slash words and non-k8s labels; update simplifyLabelWordSlashes to only strip when (1) a '/' exists, (2) the suffix after the slash is non-empty, and (3) the prefix looks like a k8s domain-style prefix (e.g. contains a '.' or matches known domains like "kubernetes.io"); otherwise return the original word. Modify the function referenced by simplifyLabels and simplifyLabelsInNode so empty-tail cases preserve the original word and only k8s-like prefixes are removed.
🤖 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/packages/react-form-wizard/src/inputs/Input.ts`:
- Around line 161-174: The current code appends inputValueToPathString(value) to
registrationPath which makes the DOM id (id) change on every value change and
churn the registry in useLayoutEffect; fix by decoupling the DOM id from the
value-derived suffix: keep buildReviewInputRegistrationPath and the
value-derived suffix only for the review registration key, but compute a stable
id for the DOM (use convertId(props) or strip the inputValueToPathString part
when assigning id), and add a brief comment by the
buildReviewInputRegistrationPath/id assignment explaining that
inputValueToPathString should only be used for low-cardinality toggles or for
registration metadata to avoid frequent unregister/register cycles (referencing
buildReviewInputRegistrationPath, inputValueToPathString, registrationPath,
convertId, id, and the useLayoutEffect unregister/register flow).
In `@frontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsx`:
- Around line 125-147: The function yamlPathBelongsToItem has asymmetric kind
handling: when item is an array you check for a resource whose kind matches
kindHead but when item is a single object you don't; add the same guard for
single objects by extracting kindHead (already computed as kindHead) and, before
calling yamlPathValueBelongsToTarget, verify that (item as { kind?: unknown
}).kind is non-null and String(kind) === kindHead — if it does not match return
false; place this check in the branch where target is an object (inside
yamlPathBelongsToItem) so single-object items are validated the same way as
array elements.
- Around line 95-113: The lookup currently relies on get-value's join-on-miss
behavior and also uses m[1] without a non-null guard; update
yamlPathValueBelongsToTarget to first try get(target, resourcePath) (preserving
any original key tokens) before falling back to get(target, basePath) so we
don't silently depend on get-value's dot-rejoin behavior, and change m[1] to a
non-null assertion (m[1]!) or add a guard consistent with the file; keep the
existing array-handling logic but reference m[1]! when computing the bracket
key.
In `@frontend/packages/react-form-wizard/src/review/utils.ts`:
- Around line 149-174: simplifyLabelWordSlashes currently strips everything
before the first '/' on any whitespace-delimited word, which drops
trailing-slash words and non-k8s labels; update simplifyLabelWordSlashes to only
strip when (1) a '/' exists, (2) the suffix after the slash is non-empty, and
(3) the prefix looks like a k8s domain-style prefix (e.g. contains a '.' or
matches known domains like "kubernetes.io"); otherwise return the original word.
Modify the function referenced by simplifyLabels and simplifyLabelsInNode so
empty-tail cases preserve the original word and only k8s-like prefixes are
removed.
In `@frontend/src/wizards/Argo/ArgoWizard.tsx`:
- Around line 698-716: Two helpers inputValueToPathString and
prunePropagationPolicyCheckboxToPathString are duplicated verbatim and have a
weak fallback for non-boolean/non-string values; extract both functions into a
single shared utility module and import them from both ArgoWizard locations so
they stay in sync, and update prunePropagationPolicyCheckboxToPathString (and
the fallback branch of inputValueToPathString) to defensively handle
null/undefined/other types (e.g., coerce to empty string or skip adding the key)
to avoid producing "#[\"PruneLast=undefined\"]" while keeping the existing
boolean/string behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8cb23769-e991-4258-9dd4-82a1c86e9a6c
📒 Files selected for processing (9)
frontend/packages/react-form-wizard/src/inputs/Input.tsfrontend/packages/react-form-wizard/src/inputs/WizCustomWrapper.tsxfrontend/packages/react-form-wizard/src/review/ReviewStep.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepContexts.tsxfrontend/packages/react-form-wizard/src/review/ReviewStepNavigation.tsxfrontend/packages/react-form-wizard/src/review/utils.tsfrontend/packages/react-form-wizard/wizards/Argo/ArgoWizard.tsxfrontend/src/components/SyncEditor/decorate.tsfrontend/src/wizards/Argo/ArgoWizard.tsx
💤 Files with no reviewable changes (1)
- frontend/packages/react-form-wizard/src/inputs/WizCustomWrapper.tsx
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, jeswanke 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 |
2 participants
📝 Summary
when clicking on an arrow, if the key didn't exist in the yaml yet, nothing happened
NOW: it opens the form input instead of doing nothing
Also in Argo wizard, if repo hadn't been set this wouldn't have an error and its edit button would have no effect
Ticket Summary (Title):
ACM-33136 Review step when clicking yaml highlight button, if yaml doesn't exist nothing happens
Ticket Link:
https://redhat.atlassian.net/browse/ACM-33136
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
New Features
Improvements