fix: update uicore version, adjust query functions to unify urijs use #749
Conversation
Signed-off-by: Tomás Castillo <[email protected]>
📝 WalkthroughWalkthroughRefactors URL/query construction across multiple action modules to use URI.js, sets Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (2)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/actions/event-actions.js (1)
1570-1627: Bug:queryAllCompaniesduplicatesspeakerEndpointand never queriescompanyEndpoint.
urlsincludesspeakerEndpointtwice (Lines 1601–1603) and omitscompanyEndpoint, so you’re likely missing/api/v1/companiesresults entirely (and doing redundant work).Proposed fix
const urls = [ speakerEndpoint.toString(), - speakerEndpoint.toString(), - submitterEndpoint.toString() + submitterEndpoint.toString(), + companyEndpoint.toString() ];src/actions/sponsorship-actions.js (1)
177-192: Missing lodash import for_.debounce.The function uses
_.debouncebut lodash is not imported in this file, which will cause a runtime error:_ is not defined.🐛 Proposed fix - add lodash import
import URI from "urijs"; +import _ from "lodash"; import history from "../history";
🧹 Nitpick comments (7)
src/actions/badge-actions.js (2)
892-912: Prefer explicit string URL; verifyfiltersemantics.Consider
fetch(endpoint.toString())and double-check the API expectsfilter(vsfilter[]) for/badge-feature-typesfiltering.
31-46: Consolidate URI configuration to a centralized init module to avoid global state mutation in each action file.
URI.escapeQuerySpace = falseis repeated in 11 action files (email, track-chair, sponsorship, company, selection-plan, media-upload, filter-criteria, event, sponsor, attendee, badge). The setting is safe—%20 encoding is RFC-recommended for URL query parameters—but should be configured once on module initialization rather than mutated in each file.src/actions/track-chair-actions.js (2)
25-39: Avoid repeatingURI.escapeQuerySpaceglobal mutation across modules.Recommend moving URI.js configuration to a single initialization point.
286-306: Confirm query filtering contract (filtervsfilter[]) and considerendpoint.toString().
querySummitProgressFlagsusesendpoint.addQuery("filter", ...)(Line 295) and passes a URI.js object directly tofetch(Line 297). Suggest verifying the API contract and usingfetch(endpoint.toString())for consistency.src/actions/event-actions.js (1)
36-60: URI.js adoption: good direction; centralize global config.The URI-based construction is cleaner than string concatenation; I’d still centralize
URI.escapeQuerySpace = falseinstead of repeating it here.src/actions/company-actions.js (1)
30-41: CentralizeURI.escapeQuerySpace = falseto app initialization instead of repeating per module.The pattern is set identically across 11 action files (company, email, badge, attendee, event, sponsor, track-chair, sponsorship, selection-plan, filter-criteria, media-upload). URI.js documentation recommends per-instance configuration via
.escapeQuerySpace()on individual URI objects rather than global mutation, which is fragile and can cause inconsistent behavior if modules load in unexpected order. At minimum, move this to a single initialization point (e.g.,src/app.js) so the intent is explicit and maintainable.src/actions/selection-plan-actions.js (1)
29-41: Confirmed:URI.escapeQuerySpaceconfiguration is repeated across 11 action files—centralize at app bootstrap.The
DEBOUNCE_WAITreuse is good. However,URI.escapeQuerySpace = falseis set individually in each action file (selection-plan-actions.js, sponsorship-actions.js, track-chair-actions.js, sponsor-actions.js, media-upload-actions.js, filter-criteria-actions.js, event-actions.js, email-actions.js, company-actions.js, attendee-actions.js, and badge-actions.js). Moving this to app initialization (e.g.,src/index.js) would eliminate the duplication and ensure consistent behavior globally without repeating it in every action module.Regarding URI.js: Multiple
addQuery("filter[]", value)calls do correctly serialize as repeated query parameters (filter[]=a&filter[]=b), which is the expected format for most backends.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.jsonsrc/actions/attendee-actions.jssrc/actions/badge-actions.jssrc/actions/company-actions.jssrc/actions/email-actions.jssrc/actions/event-actions.jssrc/actions/filter-criteria-actions.jssrc/actions/media-upload-actions.jssrc/actions/selection-plan-actions.jssrc/actions/sponsor-actions.jssrc/actions/sponsorship-actions.jssrc/actions/track-chair-actions.js
🧰 Additional context used
🧬 Code graph analysis (6)
src/actions/selection-plan-actions.js (3)
src/actions/company-actions.js (2)
endpoint(274-274)accessToken(273-273)src/utils/methods.js (2)
escapeFilterValue(137-141)escapeFilterValue(137-141)src/utils/constants.js (2)
DEBOUNCE_WAIT(130-130)DEBOUNCE_WAIT(130-130)
src/actions/event-actions.js (6)
src/actions/attendee-actions.js (2)
endpoint(784-786)accessToken(782-782)src/actions/company-actions.js (2)
endpoint(274-274)accessToken(273-273)src/actions/email-actions.js (2)
endpoint(235-235)accessToken(233-233)src/utils/methods.js (2)
escapeFilterValue(137-141)escapeFilterValue(137-141)src/actions/filter-criteria-actions.js (1)
accessToken(117-117)src/utils/constants.js (2)
FIVE_PER_PAGE(88-88)FIVE_PER_PAGE(88-88)
src/actions/sponsor-actions.js (4)
src/actions/company-actions.js (2)
endpoint(274-274)accessToken(273-273)src/actions/sponsorship-actions.js (4)
endpoint(179-179)endpoint(197-199)accessToken(178-178)accessToken(196-196)src/utils/methods.js (2)
escapeFilterValue(137-141)escapeFilterValue(137-141)src/actions/filter-criteria-actions.js (1)
accessToken(117-117)
src/actions/sponsorship-actions.js (1)
src/actions/sponsor-actions.js (7)
endpoint(181-183)endpoint(2262-2264)endpoint(2285-2287)accessToken(180-180)accessToken(2261-2261)accessToken(2284-2284)accessToken(2305-2305)
src/actions/track-chair-actions.js (3)
src/actions/company-actions.js (1)
endpoint(274-274)src/utils/methods.js (4)
escapeFilterValue(137-141)escapeFilterValue(137-141)fetchErrorHandler(151-171)fetchErrorHandler(151-171)src/utils/constants.js (2)
DEBOUNCE_WAIT(130-130)DEBOUNCE_WAIT(130-130)
src/actions/badge-actions.js (5)
src/actions/company-actions.js (2)
endpoint(274-274)accessToken(273-273)src/actions/event-actions.js (9)
endpoint(1488-1490)endpoint(1508-1510)endpoint(1529-1531)endpoint(1550-1552)accessToken(749-749)accessToken(764-764)accessToken(1487-1487)accessToken(1507-1507)accessToken(1573-1573)src/utils/methods.js (2)
escapeFilterValue(137-141)escapeFilterValue(137-141)src/actions/filter-criteria-actions.js (1)
accessToken(117-117)src/actions/media-upload-actions.js (1)
accessToken(121-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (21)
src/actions/email-actions.js (4)
339-340: LGTM: Function reformatted for consistency.The refactoring to arrow function syntax maintains the same behavior while improving code consistency.
396-416: LGTM: Function reformatted for consistency.The refactoring to arrow function syntax maintains the same behavior while improving code consistency.
428-436: LGTM: Template literal spacing fixed.The extra spaces in the template literals have been correctly removed, improving error message formatting.
42-42: This global URI configuration change is coordinated across the codebase.The setting
URI.escapeQuerySpace = falseis consistently applied across all 11 action modules (attendee-actions, track-chair-actions, sponsorship-actions, sponsor-actions, selection-plan-actions, filter-criteria-actions, media-upload-actions, email-actions, company-actions, badge-actions, and event-actions), indicating this is a deliberate, coordinated change. The query construction patterns throughout these modules show extensive use ofaddQuery()with various parameter values, confirming that all API endpoints in this module and others use the same space encoding behavior.src/actions/attendee-actions.js (1)
53-54: Global URI configuration applied consistently.This file applies the same
URI.escapeQuerySpace = falsesetting as other action modules in this PR. The consistency is good, but ensure that this module's API interactions are compatible with the+encoding for spaces in query parameters.Based on learnings from the PR pattern, this configuration should align with the backend's expectations. However, verify that attendee-related endpoints handle space encoding correctly.
src/actions/filter-criteria-actions.js (1)
37-38: LGTM: Global URI configuration applied consistently.This file applies the same
URI.escapeQuerySpace = falsesetting as other action modules in this PR, ensuring consistent query parameter encoding across the application.src/actions/media-upload-actions.js (1)
34-35: LGTM: Global URI configuration applied consistently.This file applies the same
URI.escapeQuerySpace = falsesetting as other action modules in this PR, ensuring consistent query parameter encoding across the application.package.json (1)
92-92: Verify the package version update for compatibility.The
openstack-uicore-foundationdependency has been updated from version4.2.21to4.2.22.Confirmed:
- Version
4.2.22is available on the npm registry as the latest release- No known security vulnerabilities detected for this version
Still requires manual verification:
- Check the OpenDev/GitHub repository for the CHANGELOG or release tags to confirm no breaking changes were introduced in
4.2.22compared to4.2.21, as detailed release notes are not publicly indexed in standard package trackerssrc/actions/company-actions.js (1)
272-289: Verifyfiltervsfilter[]and avoid relying on implicitfetch()coercion.
queryCompaniesusesendpoint.addQuery("filter", ...), while other codepaths useparams["filter[]"](Line 81). If the backend only honorsfilter[], this is a behavior change.- Consider
fetch(endpoint.toString())for consistency across environments.- Also: this file calls
_.debounce(...)(Line 272) but nolodashimport is shown in the provided snippet—please confirm_is guaranteed in this module’s runtime.Proposed adjustment (if backend expects
filter[]and you want explicit string URLs)export const queryCompanies = _.debounce(async (input, callback) => { const accessToken = await getAccessTokenSafely(); const endpoint = URI(`${window.API_BASE_URL}/api/v1/companies`); input = escapeFilterValue(input); endpoint.addQuery("access_token", accessToken); endpoint.addQuery("fields", "id,name"); endpoint.addQuery("relations", "none"); if (input) { - endpoint.addQuery("filter", `name=@${input}`); + endpoint.addQuery("filter[]", `name=@${input}`); } - fetch(endpoint) + fetch(endpoint.toString()) .then(fetchResponseHandler) .then((json) => { const options = [...json.data]; callback(options); }) .catch(fetchErrorHandler); }, DEBOUNCE_WAIT);src/actions/selection-plan-actions.js (1)
699-719: Confirm backend acceptsfilter(notfilter[]) for this endpoint; considerfetch(endpoint.toString()).
querySelectionPlanExtraQuestionsusesendpoint.addQuery("filter", ...)(Line 708). If the API expects repeatedfilter[], this may silently stop filtering. Also, passingendpoint.toString()tofetchavoids any RequestInfo edge cases.src/actions/badge-actions.js (1)
47-53: NewBADGE_TYPE_CHANGEDaction: confirm reducer coverage.Please verify any reducers/sagas listening for badge-related actions also handle
BADGE_TYPE_CHANGED, otherwise this may be a no-op action in the UI state flow.src/actions/track-chair-actions.js (1)
40-45: New track chair action constants: verify end-to-end wiring.
TRACK_CHAIR_ADDED/TRACK_CHAIR_DELETEDare now exported; please confirm reducers/middleware consume them (and nothing else depends on the previous action types for those flows).src/actions/event-actions.js (2)
1486-1526:filter[]multi-add: verify URI.js serialization for repeated keys.In
queryEventsWithPrivateRSVP, you addfilter[]twice (Lines 1513–1516). Please confirm URI.js serializes this as repeatedfilter[]=parameters (not a comma-joined single value), since the backend likely relies on repetition. Also considerfetch(endpoint.toString())for consistency.
1528-1569: Verifyfilter[]usage against public endpoints and consider explicittoString().Both
querySpeakerCompanyandquerySubmitterCompanynow usefilter[](Lines 1535–1536, 1556–1557). Please confirm these public endpoints accept that shape (some acceptfilternotfilter[]).src/actions/sponsor-actions.js (5)
33-48: LGTM - URI import and global configuration.The URI import and
URI.escapeQuerySpace = falseconfiguration aligns with the project-wide pattern to standardize URI handling and disable space escaping in query parameters. This is consistent with changes in other action files.
179-202: LGTM - querySponsors refactored to URI-based construction.The refactoring correctly builds the endpoint using URI and conditionally adds the filter parameter only when
escapedInputis truthy. The fetch API accepts URI objects since they coerce to strings.
2259-2280: LGTM - querySummitSponsorships refactored consistently.The URI-based endpoint construction follows the same pattern established in
querySponsors, with conditional filter addition when input is present.
2282-2297: LGTM - querySummitAddons refactored to URI-based construction.The refactoring is correct. Note that the
inputparameter is unused, but this appears to be pre-existing behavior (the function fetches metadata, not filtered data).
2299-2335: LGTM - querySponsorAddons refactored to URI-based construction.The refactoring correctly builds URI endpoints for each sponsorship ID within the Promise.all loop. The query parameters are properly added using the fluent
addQueryAPI.src/actions/sponsorship-actions.js (2)
30-39: LGTM - URI import and configuration.The imports and global
URI.escapeQuerySpace = falseconfiguration are consistent with the project-wide pattern established in other action files likesponsor-actions.js.
194-218: LGTM - querySponsorshipsBySummit refactored to URI-based construction.The refactoring correctly builds the endpoint using URI with proper query parameters including
page,per_page(usingMAX_PER_PAGE),expand, andorder. The conditional filter addition follows the established pattern.Note: The same missing lodash import issue mentioned above affects this function as well.
| "moment-timezone": "^0.5.33", | ||
| "node-sass": "^7.0.1", | ||
| "openstack-uicore-foundation": "4.2.21", | ||
| "openstack-uicore-foundation": "4.2.22", |
There was a problem hiding this comment.
update urijs version to latest one 1.19.11
Signed-off-by: Tomás Castillo <[email protected]>
ref: https://app.clickup.com/t/86b7yv7y1
Signed-off-by: Tomás Castillo [email protected]
Summary by CodeRabbit
Chores
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.