Fix: use different query for add-on ddl - #303
Conversation
📝 WalkthroughWalkthroughThe pull request renames summit-specific addon selectors to addon type selectors, updates option mapping to use addon names, changes the query endpoint, and updates Formik integration, exports, Webpack entries, package version, and tests. ChangesAddon type selector
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/mui/__tests__/addon-type-select.test.js (1)
52-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the option mapping in this test.
The test named
"renders options returned by querySummitAddons"only checks that the query was called. It would pass ifr.namewere ignored or mapped to the wrong value. Open the select and assert thatAddon AlphaandAddon Betarender as the option labels and values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/__tests__/addon-type-select.test.js` around lines 52 - 60, Update the test “renders options returned by querySummitAddons” to open the rendered Select after the query resolves, then assert that options labeled and valued “Addon Alpha” and “Addon Beta” are present. Keep the existing querySummitAddons invocation assertion while verifying the returned addon name mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/mui/__tests__/addon-type-select.test.js`:
- Around line 52-60: Update the test “renders options returned by
querySummitAddons” to open the rendered Select after the query resolves, then
assert that options labeled and valued “Addon Alpha” and “Addon Beta” are
present. Keep the existing querySummitAddons invocation assertion while
verifying the returned addon name mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c1ceee9-2896-428b-a4be-7cf66312bcb6
📒 Files selected for processing (7)
src/components/index.jssrc/components/mui/__tests__/addon-type-select.test.jssrc/components/mui/__tests__/mui-formik-addon-type-select.test.jssrc/components/mui/addon-type-select.jssrc/components/mui/formik-inputs/mui-formik-addon-type-select.jssrc/utils/query-actions.jswebpack.common.js
2ad9b5d to
026ed56
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the add-on dropdown to use the add-on types endpoint and renames the MUI selector entry points accordingly, including Formik integration and updated exports/versioning.
Changes:
- Switched add-on data retrieval from summit metadata to the global add-on types endpoint.
- Renamed/replaced the summit add-on select components with generalized add-on type select components (including Formik wrapper) and updated exports/webpack entries.
- Updated/addjusted unit tests and bumped the package version.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.common.js | Updates published bundle entry points to the new add-on type select modules. |
| src/utils/query-actions.js | Changes the query endpoint used for fetching add-on options (now add-on types). |
| src/components/mui/formik-inputs/mui-formik-addon-type-select.js | Renames and rewires the Formik wrapper to use the new add-on type selector. |
| src/components/mui/addon-type-select.js | Updates the selector component to normalize and display add-on type names. |
| src/components/mui/tests/mui-formik-addon-type-select.test.js | Updates Formik wrapper tests to mock and assert against the renamed selector. |
| src/components/mui/tests/addon-type-select.test.js | Updates selector tests to reflect the new API response shape and component name. |
| src/components/index.js | Updates public exports to expose the new selector components. |
| package.json | Bumps the package version for the release containing these changes. |
Suppressed comments (4)
src/components/mui/tests/addon-type-select.test.js:24
- This test file uses
fireEvent-style interaction in other MUI Select tests in the repo, butfireEventisn't imported here. Importing it will allow the options-rendering test to actually open the Select and assert on the menu items.
This issue also appears on line 52 of the same file.
src/components/mui/tests/addon-type-select.test.js:56
- The "renders options" test currently only asserts that the query function was called, but it doesn't verify that the option labels are actually rendered. This can miss regressions in the normalization logic (e.g., using the wrong field for labels).
src/components/mui/addon-type-select.js:33 - The add-on types endpoint returns objects (tests mock
{id, name}), but the component currently usesnameas the option value/key. Usingidas the option value is more stable (avoids collisions if names repeat) and matches the pattern used bySponsorAddonSelect.
src/components/mui/addon-type-select.js:72 - If the Select option values are numeric IDs, the
valueprop type should benumberto match and avoid type mismatches in consumers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@santipalenque please review
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/components/mui/addon-type-select.js:36
- The effect fetches options using
summitIdbut the dependency array is empty, so the list will not refresh ifsummitIdchanges (and this also tripsreact-hooks/exhaustive-deps). IncludesummitIdin the dependency array or explicitly justify why it must only run once.
This issue also appears on line 67 of the same file.
src/components/mui/addon-type-select.js:70
AddonTypeSelectstill requiressummitId, but the underlying query now hits the/summits/all/add-on-typesendpoint wheresummitIdno longer affects results. Making this prop optional avoids forcing callers to pass a dummy value for a generalized selector.
src/components/mui/tests/addon-type-select.test.js:56- The test "renders options returned by querySummitAddons" doesn’t actually assert that the option labels render, so it won’t catch regressions in the new
{id,name}mapping. Open the select and assert the expected option text is present.
src/components/mui/formik-inputs/mui-formik-addon-type-select.js:46 MuiFormikAddonTypeSelectstill requiressummitId, but the underlying selector now queries the global add-on types endpoint. MakingsummitIdoptional here avoids forcing Formik forms to provide a summit id that no longer affects the results.
src/utils/query-actions.js:636summitIdis now unused inquerySummitAddonssince the endpoint is/summits/all/add-on-types. Renaming the parameter to_summitIdmakes the intent clear and avoids confusion for future maintainers.
export const querySummitAddons = async (
summitId,
callback
) => {
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/mui/addon-type-select.js`:
- Around line 29-31: Update the Formik contract used by MuiFormikAddonTypeSelect
so addon values are consistently represented by names across initial values,
persisted data, and submit payloads, or normalize legacy addon objects at the
Formik boundary before selection. Ensure existing object-shaped values still
resolve to the matching MenuItem, and add a regression test covering the legacy
value shape.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95968de1-4f20-4b54-8ad2-387876f9f9cf
📒 Files selected for processing (8)
package.jsonsrc/components/index.jssrc/components/mui/__tests__/addon-type-select.test.jssrc/components/mui/__tests__/mui-formik-addon-type-select.test.jssrc/components/mui/addon-type-select.jssrc/components/mui/formik-inputs/mui-formik-addon-type-select.jssrc/utils/query-actions.jswebpack.common.js
🚧 Files skipped from review as they are similar to previous changes (5)
- webpack.common.js
- src/components/mui/formik-inputs/mui-formik-addon-type-select.js
- src/components/mui/tests/addon-type-select.test.js
- src/utils/query-actions.js
- src/components/mui/tests/mui-formik-addon-type-select.test.js
| const normalized = results.map((r) => ({ | ||
| value: r, | ||
| label: r | ||
| value: r.name, | ||
| label: r.name |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'\b(MuiFormikAddonTypeSelect|MuiSummitAddonSelect|MuiFormikSummitAddonSelect|AddonTypeSelect)\b' \
--glob '*.{js,jsx,mjs,cjs}' . || true
rg -n -C 5 \
'\b(initialValues|setFieldValue|setValue|addon)\b' \
--glob '*.{js,jsx,mjs,cjs}' . || trueRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 50397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
find . -path './node_modules' -prune -o -path './dist' -prune -o -path './build' -prune -o \( -type f \( -name '*.js' -o -name '*.jsx' -o -name '*.mjs' -o -name '*.cjs' \) \) -print | sed 's#^\./##' | sort
echo "== component slices =="
sed -n '1,120p' src/components/mui/addon-type-select.js
echo "--- formik wrapper ---"
sed -n '1,80p' src/components/mui/formik-inputs/mui-formik-addon-type-select.js
echo "--- mui index slice ---"
sed -n '60,105p' src/components/index.js
echo "--- direct imports/usages (focused) =="
rg -n --glob '*.{js,jsx,mjs,cjs}' 'from ["'\''].*addon-type-select["'\'']|<MuiFormikAddonTypeSelect|<MuiAddonTypeSelect' . || true
echo "== addon field occurrences (focused) =="
rg -n --glob '*.{js,jsx,mjs,cjs}' '\badd[on]s?\b' . | head -n 220Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 25887
Migrate the Formik value contract before using addon names.
value: r.name changes the selector value from the full addon object to a string. MuiFormikAddonTypeSelect forwards field.value unchanged and stores the selected value directly. If an existing form still initializes addon with the previous object shape, no MenuItem matches it.
Use addon names for existing Formik initial values, persisted values, and submit payloads, or normalize legacy values at the Formik boundary. Add a regression test for the old value shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/mui/addon-type-select.js` around lines 29 - 31, Update the
Formik contract used by MuiFormikAddonTypeSelect so addon values are
consistently represented by names across initial values, persisted data, and
submit payloads, or normalize legacy addon objects at the Formik boundary before
selection. Ensure existing object-shaped values still resolve to the matching
MenuItem, and add a regression test covering the legacy value shape.
as part of https://app.clickup.com/t/9014802374/86bat8h0n
Summary by CodeRabbit
New Features
Breaking Changes