fix(RHOAIENG-57510): store raw input value in MultiSelection isCreatable option name#7198
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMultiSelection.tsx now always sets Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes IssuesCWE-20: Improper Input Validation — Removing the Missing state discriminator — The change decouples display formatting from the canonical option value without introducing explicit metadata (flag/type) to convey "created" vs "existing". Relying solely on Actionable steps
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/components/MultiSelection.tsx (2)
137-161: Add regression coverage for the raw-name/display-label split.This branch changes persisted selection semantics, not just dropdown text. Add a test for: create a new option, verify the chip stores the raw value, remove it, type the same value again, and verify only one create row is offered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MultiSelection.tsx` around lines 137 - 161, The test suite needs a regression test for the raw-name vs display-label creation flow: write a test that mounts the MultiSelection component (using the same props that enable isCreatable and supply createOptionMessage), type a new value into inputValue to trigger createOption, confirm selecting it adds a chip whose stored value equals the raw input (not the display label), remove that chip, type the exact same raw value again and assert that only one create row is offered (i.e., createOption is present once and no duplicate rows exist). Use component identifiers tied to the MultiSelection props/state (inputValue, isCreatable, createOption, createOptionMessage, and allValues) to drive interactions and assertions.
154-161: RemoveReact.useMemoaroundcreateOptionDisplayNameand compute the string inline.Deriving a short display string from current props is trivial enough that memoization adds overhead without benefit. React 19's official guidance and compiler-based optimization handle expensive derivations automatically; manual memoization here violates the coding guideline: "avoid unnecessary useCallback/useMemo/useRef — React is performant by default."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MultiSelection.tsx` around lines 154 - 161, Remove the React.useMemo wrapper around createOptionDisplayName and compute the display string inline (use the values createOption and createOptionMessage directly where createOptionDisplayName is referenced); replace the memoized constant createOptionDisplayName with a simple conditional expression that returns undefined if !createOption, otherwise returns typeof createOptionMessage === 'string' ? createOptionMessage : createOptionMessage(createOption.name), and remove the dependency array since memoization is being eliminated.
🤖 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/pages/groupSettings/GroupSettings.tsx`:
- Around line 45-48: The processGroup function is writing opt.name back
unchanged which preserves legacy creatable labels like 'Define new group:
"foo"'; update processGroup (SelectionOptions -> GroupStatus) to normalize
opt.name before returning: detect the legacy pattern (e.g. /^Define new
group:\s*"?(.+?)"?$/) and extract the inner label to use as name, otherwise fall
back to opt.name (or opt.label if appropriate), keeping id as String(opt.id) and
enabled from opt.selected; this ensures old persisted display labels are cleaned
on write while still handling normal options.
---
Nitpick comments:
In `@frontend/src/components/MultiSelection.tsx`:
- Around line 137-161: The test suite needs a regression test for the raw-name
vs display-label creation flow: write a test that mounts the MultiSelection
component (using the same props that enable isCreatable and supply
createOptionMessage), type a new value into inputValue to trigger createOption,
confirm selecting it adds a chip whose stored value equals the raw input (not
the display label), remove that chip, type the exact same raw value again and
assert that only one create row is offered (i.e., createOption is present once
and no duplicate rows exist). Use component identifiers tied to the
MultiSelection props/state (inputValue, isCreatable, createOption,
createOptionMessage, and allValues) to drive interactions and assertions.
- Around line 154-161: Remove the React.useMemo wrapper around
createOptionDisplayName and compute the display string inline (use the values
createOption and createOptionMessage directly where createOptionDisplayName is
referenced); replace the memoized constant createOptionDisplayName with a simple
conditional expression that returns undefined if !createOption, otherwise
returns typeof createOptionMessage === 'string' ? createOptionMessage :
createOptionMessage(createOption.name), and remove the dependency array since
memoization is being eliminated.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bcd2de3d-adcb-415a-a821-bf1912473125
📒 Files selected for processing (2)
frontend/src/components/MultiSelection.tsxfrontend/src/pages/groupSettings/GroupSettings.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7198 +/- ##
==========================================
- Coverage 64.81% 63.64% -1.18%
==========================================
Files 2441 2502 +61
Lines 75996 77594 +1598
Branches 19158 19717 +559
==========================================
+ Hits 49257 49382 +125
- Misses 26739 28212 +1473
... and 71 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…ble option name The createOption was storing the createOptionMessage display text (e.g. `Define new group: "foo"`) as the option name instead of the raw input value, causing the duplicate check to fail on subsequent lookups. Separate the display text into createOptionDisplayName, used only for dropdown rendering, and remove the now-unnecessary prefix-stripping workaround in GroupSettings. Also fixes a typo where `Option.name` referenced the global constructor instead of `createOption.name`.
The previous code had a typo (Option.name instead of createOption.name) that made the create option data-testid always resolve to "Option". Update the test selector to match the corrected test ID. Also remove unnecessary useMemo around createOptionDisplayName.
dpanshug
force-pushed
the
rhoaieng-57510-fix-multiselection-creatable
branch
from
2bc14de to
e481c85
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Griffin-Sullivan 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 |
openshift-merge-bot
Bot
merged commit abd09cb
into
opendatahub-io:main
2 participants
https://redhat.atlassian.net/browse/RHOAIENG-57510
Description
The createOption was storing the createOptionMessage display text (e.g.
Define new group: "foo") as the option name instead of the raw input value, causing the duplicate check to fail on subsequent lookups. Separate the display text into createOptionDisplayName, used only for dropdown rendering, and remove the now-unnecessary prefix-stripping workaround in GroupSettings.@Griffin-Sullivan i have fixed directly the main component, rather than your proposed solution in JIRA. This benefits all current and future consumers of this component
Fix in Create subscription form
Screen.Recording.2026-04-13.at.2.56.15.PM.mov
Fix in User Management page
Screen.Recording.2026-04-13.at.2.51.43.PM.mov
How Has This Been Tested?
test) and select the create optionAdd group "test"and the selected chip showstesttestagainAdd group "test"entry appears (no duplicates)Test Impact
None
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
Bug Fixes
Refactor
Tests