Feature/new grid filter event list poc#984
Open
santipalenque wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughRefactors ChangesSelectFilterCriteria styling and prop updates
Event filter API simplification
Import modal components
SummitEventListPage directory rebuild
Test state and i18n updates
Dependencies and stylesheet cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 8
🧹 Nitpick comments (1)
src/components/filters/select-filter-criteria/index.js (1)
1-12: 💤 Low valueConsider restoring JSDoc comment format.
The comment header was changed from JSDoc format (
/**...*/) to a regular block comment (/* *...* */). JSDoc comments are recognized by documentation tools and IDEs for auto-documentation and hover tooltips. This change appears unintentional.📝 Restore JSDoc format
-/* * +/** * Copyright 2019 OpenStack Foundation * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * http://www.apache.org/licenses/LICENSE-2.0 * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. - * */ + */🤖 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/filters/select-filter-criteria/index.js` around lines 1 - 12, The file header comment at the beginning of index.js has been unintentionally changed from JSDoc format to a regular block comment. Restore the proper JSDoc format by changing the opening from `/* *` to `/**` so that documentation tools and IDEs can properly recognize and display auto-documentation and hover tooltips for the file.
🤖 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/filters/select-filter-criteria/index.js`:
- Line 97: The AsyncSelect component at line 97 has an unnecessary isLoading
prop that is not used by react-select 2.x since it manages loading state
internally based on pending loadOptions requests. Remove the isLoading prop from
the AsyncSelect component invocation to eliminate this dead code.
In `@src/pages/events/summit-event-list-page/components/ImportModal/index.jsx`:
- Around line 25-26: The Modal component on line 25 uses onHide={onClose}, but
the handleClose function is responsible for clearing local state. When users
close the modal via the close button or backdrop, onClose is called instead of
handleClose, causing the modal to reopen with stale values. Change the onHide
prop to call handleClose instead of onClose to ensure local state is reset on
all modal close paths.
In `@src/pages/events/summit-event-list-page/components/ImportMUXModal/index.jsx`:
- Around line 61-66: The Input component with id "mux_token_secret" is
displaying the token secret as plain text, exposing sensitive credentials. Add a
type="password" attribute to the Input component to mask the input characters,
preventing the sensitive token secret from being visible on screen or in shared
sessions.
- Around line 26-27: The Modal component on line 26 has onHide={onClose} which
doesn't clear the form state when the modal is dismissed via the close button or
backdrop click. Create a new handler function that resets the tokenId,
tokenSecret, and emailTo state variables before calling onClose, then pass this
new handler to the Modal's onHide prop instead of onClose directly. This ensures
stale form data is cleared whenever the modal is closed.
In `@src/pages/events/summit-event-list-page/index.js`:
- Around line 842-844: The filter configuration object with key
"submission_status" has an incorrect label "Submitter Company" that doesn't
match what the filter actually does. Find the filter object with key
"submission_status" and change its label property to accurate text that
correctly describes the submission status field being filtered, such as
"Submission Status" or similar descriptive text that matches the actual
filtering behavior.
- Around line 958-968: The handleFilterCriteriaChange function builds
newEventFilters but does not apply or persist this value, so when the useEffect
at line 958 refetches with parsedFilter, it still uses the old filter state
instead of the newly selected saved criteria. To fix this, ensure that
handleFilterCriteriaChange actually sets or applies the newEventFilters (likely
by updating filter state or calling a state setter) so that the subsequent
getEvents call in the useEffect uses the updated filter criteria. The same issue
applies at the other affected location around lines 1111-1121 where the filter
change is similarly not being persisted before the fetch occurs.
- Around line 1441-1448: The mapStateToProps function is attempting to access
the media_uploads property of mediaUploadListState without checking if
mediaUploadListState exists first, causing a TypeError when it is undefined. Add
a guard condition to safely access mediaUploadListState.media_uploads by either
using optional chaining or a conditional check to provide a fallback value (such
as an empty array) when mediaUploadListState is not defined.
- Around line 1291-1296: The editable flag assignment uses the wrong property to
check against the editableColumns array. In the object creation where columnKey,
value, sortable, and customStyle are assigned from f2, the editable property is
being set by checking if f2.editable is included in editableColumns. However, f2
entries are keyed by columnKey, not editable. Replace the reference to
f2.editable with f2.columnKey when calling editableColumns.includes() so that
the editability check correctly matches against the column identifiers rather
than the editable property value.
---
Nitpick comments:
In `@src/components/filters/select-filter-criteria/index.js`:
- Around line 1-12: The file header comment at the beginning of index.js has
been unintentionally changed from JSDoc format to a regular block comment.
Restore the proper JSDoc format by changing the opening from `/* *` to `/**` so
that documentation tools and IDEs can properly recognize and display
auto-documentation and hover tooltips for the file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1210b4c2-fd6f-43c2-87ce-9b4e3e20809b
📒 Files selected for processing (6)
src/components/filters/select-filter-criteria/index.jssrc/components/filters/select-filter-criteria/index.module.lesssrc/pages/events/summit-event-list-page.jssrc/pages/events/summit-event-list-page/components/ImportMUXModal/index.jsxsrc/pages/events/summit-event-list-page/components/ImportModal/index.jsxsrc/pages/events/summit-event-list-page/index.js
💤 Files with no reviewable changes (2)
- src/components/filters/select-filter-criteria/index.module.less
- src/pages/events/summit-event-list-page.js
| defaultOptions={defaultOptions} | ||
| onMenuOpen={handleMenuOpen} | ||
| isLoading={true} | ||
| isLoading |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does react-select 2.x AsyncSelect manage loading state internally, or does it require an explicit isLoading prop?
💡 Result:
React-select 2.x AsyncSelect does manage its own loading state internally based on pending loadOptions requests, and it does not reliably use an explicit isLoading prop. In react-select v2, there was an acknowledged behavior/bug: “The isLoading prop gets completely ignored by the AsyncSelect” [1]. The fix to allow manually controlling the loading indicator was added later (PR #3690), described as enabling the developer to explicitly set the loading property while keeping the existing behavior intact [2]. So, for react-select 2.x specifically: you should not depend on passing isLoading to drive the spinner; it’s controlled internally by the async request lifecycle. (For context, current react-select code/docs include an isLoading prop intended to “cause the select to be displayed in the loading state, even if the Async select is not currently waiting for loadOptions to resolve” [3], and the Async docs page covers the async loading approach [4]. That reflects behavior after the v2-era fix.)
Citations:
- 1: [v2] No way to manually set isLoading on async select box JedWatson/react-select#2986
- 2: Manually set 'isLoading' state for async selects JedWatson/react-select#3690
- 3: https://github.com/JedWatson/react-select/blob/master/packages/react-select/src/useAsync.ts
- 4: https://react-select.com/async
🏁 Script executed:
# Check the package.json to find react-select version
fd -t f "package.json" | head -5 | xargs -I {} bash -c 'echo "=== {} ===" && cat "{}"' | grep -A 50 "react-select"Repository: fntechgit/summit-admin
Length of output: 1711
🏁 Script executed:
# Read the select-filter-criteria file around line 97
cat -n src/components/filters/select-filter-criteria/index.js | sed -n '85,110p'Repository: fntechgit/summit-admin
Length of output: 997
Remove unused isLoading prop from AsyncSelect.
The isLoading prop at line 97 is unnecessary and ignored by react-select 2.x. AsyncSelect manages loading state internally based on pending loadOptions requests, so this explicit prop has no effect and should be removed as dead code.
🔧 Remove the unnecessary prop
defaultOptions={defaultOptions}
onMenuOpen={handleMenuOpen}
- isLoading
{...rest}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| isLoading | |
| defaultOptions={defaultOptions} | |
| onMenuOpen={handleMenuOpen} | |
| {...rest} |
🤖 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/filters/select-filter-criteria/index.js` at line 97, The
AsyncSelect component at line 97 has an unnecessary isLoading prop that is not
used by react-select 2.x since it manages loading state internally based on
pending loadOptions requests. Remove the isLoading prop from the AsyncSelect
component invocation to eliminate this dead code.
Comment on lines
+25
to
+26
| <Modal show={show} onHide={onClose}> | ||
| <Modal.Header closeButton> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "index.jsx" | grep -i "importmodal"Repository: fntechgit/summit-admin
Length of output: 141
🏁 Script executed:
cat -n ./src/pages/events/summit-event-list-page/components/ImportModal/index.jsxRepository: fntechgit/summit-admin
Length of output: 3677
Reset state on all modal close paths.
handleClose clears local state, but Line 25 uses onHide={onClose}; closing with the modal close button or backdrop skips the reset and reopens with stale values. Change onHide to call handleClose instead.
Suggested patch
- <Modal show={show} onHide={onClose}>
+ <Modal show={show} onHide={handleClose}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Modal show={show} onHide={onClose}> | |
| <Modal.Header closeButton> | |
| <Modal show={show} onHide={handleClose}> | |
| <Modal.Header closeButton> |
🤖 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/pages/events/summit-event-list-page/components/ImportModal/index.jsx`
around lines 25 - 26, The Modal component on line 25 uses onHide={onClose}, but
the handleClose function is responsible for clearing local state. When users
close the modal via the close button or backdrop, onClose is called instead of
handleClose, causing the modal to reopen with stale values. Change the onHide
prop to call handleClose instead of onClose to ensure local state is reset on
all modal close paths.
Comment on lines
+26
to
+27
| <Modal show={show} onHide={onClose}> | ||
| <Modal.Header closeButton> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/pages/events/summit-event-list-page/components/ImportMUXModal/index.jsx | head -80Repository: fntechgit/summit-admin
Length of output: 3081
🏁 Script executed:
cat -n src/pages/events/summit-event-list-page/components/ImportMUXModal/index.jsx | tail -20Repository: fntechgit/summit-admin
Length of output: 653
Use reset-aware close handler for modal hide.
Line 26 routes onHide directly to onClose (the prop), so modal dismissal via close button or backdrop does not clear tokenId, tokenSecret, and emailTo state. This leaves stale form data visible when the modal reopens.
Suggested fix
- <Modal show={show} onHide={onClose}>
+ <Modal show={show} onHide={handleClose}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Modal show={show} onHide={onClose}> | |
| <Modal.Header closeButton> | |
| <Modal show={show} onHide={handleClose}> | |
| <Modal.Header closeButton> |
🤖 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/pages/events/summit-event-list-page/components/ImportMUXModal/index.jsx`
around lines 26 - 27, The Modal component on line 26 has onHide={onClose} which
doesn't clear the form state when the modal is dismissed via the close button or
backdrop click. Create a new handler function that resets the tokenId,
tokenSecret, and emailTo state variables before calling onClose, then pass this
new handler to the Modal's onHide prop instead of onClose directly. This ensures
stale form data is cleared whenever the modal is closed.
Comment on lines
+61
to
+66
| <Input | ||
| id="mux_token_secret" | ||
| value={tokenSecret} | ||
| onChange={(ev) => setTokenSecret(ev.target.value)} | ||
| className="form-control" | ||
| /> |
There was a problem hiding this comment.
Mask MUX token secret input.
The token secret is currently visible as plain text. This exposes sensitive credentials on screen and in shared-session scenarios.
🔒 Suggested patch
<Input
id="mux_token_secret"
+ type="password"
value={tokenSecret}
onChange={(ev) => setTokenSecret(ev.target.value)}
className="form-control"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Input | |
| id="mux_token_secret" | |
| value={tokenSecret} | |
| onChange={(ev) => setTokenSecret(ev.target.value)} | |
| className="form-control" | |
| /> | |
| <Input | |
| id="mux_token_secret" | |
| type="password" | |
| value={tokenSecret} | |
| onChange={(ev) => setTokenSecret(ev.target.value)} | |
| className="form-control" | |
| /> |
🤖 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/pages/events/summit-event-list-page/components/ImportMUXModal/index.jsx`
around lines 61 - 66, The Input component with id "mux_token_secret" is
displaying the token secret as plain text, exposing sensitive credentials. Add a
type="password" attribute to the Input component to mask the input characters,
preventing the sensitive token secret from being visible on screen or in shared
sessions.
Comment on lines
+842
to
+844
| key: "submission_status", | ||
| label: "Submitter Company", | ||
| operators: [OPERATORS.IS], |
There was a problem hiding this comment.
Fix incorrect filter label text for submission_status.
key: "submission_status" is labeled "Submitter Company"; this is mismatched UI text and makes the filter confusing.
🤖 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/pages/events/summit-event-list-page/index.js` around lines 842 - 844, The
filter configuration object with key "submission_status" has an incorrect label
"Submitter Company" that doesn't match what the filter actually does. Find the
filter object with key "submission_status" and change its label property to
accurate text that correctly describes the submission status field being
filtered, such as "Submission Status" or similar descriptive text that matches
the actual filtering behavior.
Comment on lines
+1291
to
+1296
| let c = { | ||
| columnKey: f2.columnKey, | ||
| value: T.translate(`event_list.${f2.value}`), | ||
| sortable: f2.sortable, | ||
| editable: !!editableColumns.includes(f2.editable), | ||
| customStyle: f2.customStyle |
There was a problem hiding this comment.
Editable flag uses the wrong property key.
Line 1295 checks editableColumns.includes(f2.editable), but f2 entries are keyed by columnKey/editableField. This makes dynamic column editability resolve incorrectly.
💡 Suggested patch
let c = {
columnKey: f2.columnKey,
value: T.translate(`event_list.${f2.value}`),
sortable: f2.sortable,
- editable: !!editableColumns.includes(f2.editable),
+ editable: editableColumns.includes(f2.columnKey),
customStyle: f2.customStyle
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let c = { | |
| columnKey: f2.columnKey, | |
| value: T.translate(`event_list.${f2.value}`), | |
| sortable: f2.sortable, | |
| editable: !!editableColumns.includes(f2.editable), | |
| customStyle: f2.customStyle | |
| let c = { | |
| columnKey: f2.columnKey, | |
| value: T.translate(`event_list.${f2.value}`), | |
| sortable: f2.sortable, | |
| editable: editableColumns.includes(f2.columnKey), | |
| customStyle: f2.customStyle |
🤖 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/pages/events/summit-event-list-page/index.js` around lines 1291 - 1296,
The editable flag assignment uses the wrong property to check against the
editableColumns array. In the object creation where columnKey, value, sortable,
and customStyle are assigned from f2, the editable property is being set by
checking if f2.editable is included in editableColumns. However, f2 entries are
keyed by columnKey, not editable. Replace the reference to f2.editable with
f2.columnKey when calling editableColumns.includes() so that the editability
check correctly matches against the column identifiers rather than the editable
property value.
Comment on lines
+1441
to
+1448
| const mapStateToProps = ({ | ||
| currentSummitState, | ||
| currentEventListState, | ||
| mediaUploadListState | ||
| }) => ({ | ||
| currentSummit: currentSummitState.currentSummit, | ||
| mediaUploadTypes: mediaUploadListState.media_uploads, | ||
| ...currentEventListState |
There was a problem hiding this comment.
Guard mediaUploadListState in mapStateToProps to prevent crash.
CI is failing with TypeError: Cannot read properties of undefined (reading 'media_uploads') at Line 1447. mediaUploadListState is not guaranteed to exist.
💡 Suggested patch
const mapStateToProps = ({
currentSummitState,
currentEventListState,
mediaUploadListState
}) => ({
currentSummit: currentSummitState.currentSummit,
- mediaUploadTypes: mediaUploadListState.media_uploads,
+ mediaUploadTypes: mediaUploadListState?.media_uploads ?? [],
...currentEventListState
});🧰 Tools
🪛 GitHub Actions: jest / 0_build.txt
[error] 1447-1447: TypeError: Cannot read properties of undefined (reading 'media_uploads'). In mapToProps: mediaUploadTypes is set from mediaUploadListState.media_uploads, but mediaUploadListState is undefined.
🪛 GitHub Actions: jest / build
[error] 1447-1447: TypeError: Cannot read properties of undefined (reading 'media_uploads') in mapToProps. Accessing mediaUploadListState.media_uploads while mediaUploadListState is undefined.
🤖 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/pages/events/summit-event-list-page/index.js` around lines 1441 - 1448,
The mapStateToProps function is attempting to access the media_uploads property
of mediaUploadListState without checking if mediaUploadListState exists first,
causing a TypeError when it is undefined. Add a guard condition to safely access
mediaUploadListState.media_uploads by either using optional chaining or a
conditional check to provide a fallback value (such as an empty array) when
mediaUploadListState is not defined.
Source: Pipeline failures
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/actions/event-actions.js`:
- Around line 889-901: The dispatch(startLoading()) function is called twice in
the same code block - once at the beginning and again after initializing the
filter variable. Remove the duplicate dispatch(startLoading()) call that appears
after the filter initialization to prevent redundant loading state updates,
keeping only the first call at the start of the function.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30eaaa34-83c6-4df4-9adc-35216db93d1f
📒 Files selected for processing (5)
src/actions/__tests__/event-actions.test.jssrc/actions/event-actions.jssrc/i18n/en.jsonsrc/pages/events/__tests__/summit-event-list-page.test.jssrc/pages/events/summit-event-list-page/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/events/summit-event-list-page/index.js
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@package.json`:
- Line 95: Before merging the openstack-uicore-foundation version bump to
5.0.36-beta.1, verify that the beta version still exports the `.filters-row` and
`.event-list-date-picker` CSS classes. If the beta version does not include
these styles, add local CSS definitions for both classes to ensure layout
doesn't break in the files that depend on them: summit-attendees-list-page.js,
ticket-type-list-page.js, purchase-order-list-page.js, email-log-list-page.js,
and audit-logs/index.js. Alternatively, consider using a stable version of the
dependency if the beta introduces breaking changes to these styles.
In `@src/pages/events/summit-event-list-page/index.js`:
- Around line 956-979: In the _getEvents function, replace the extraColumns
parameter being passed to the getEvents call with selectedColumns instead. This
ensures that when users change field selections, those selections persist
through pagination, sorting, and search operations, rather than reverting to
stale Redux prop values that could reset the column selector.
- Around line 981-994: The two useEffect hooks need guards to prevent stale API
calls during summit switches and filter changes. In the first useEffect
(triggered by currentSummit?.id), verify that currentSummit has a valid id
before calling getMediaUploads and getEvents, and ensure resetFilters completes
before these calls execute. In the second useEffect (triggered by
parsedFilter.join(",")), add a guard to skip the initial _getEvents call that
fires immediately after resetFilters in the first effect, and only call
_getEvents when filters genuinely change after a summit is fully loaded.
Additionally, when filters do change, ensure the pagination resets to page 1 by
passing 1 as the page parameter to the data fetch function in _getEvents.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b97854d-e004-409f-a67b-ef8f62f7c270
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.jsonsrc/actions/event-actions.jssrc/pages/events/__tests__/summit-event-list-page.test.jssrc/pages/events/summit-event-list-page/index.jssrc/styles/summit-event-list-page.less
💤 Files with no reviewable changes (2)
- src/styles/summit-event-list-page.less
- src/pages/events/tests/summit-event-list-page.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/actions/event-actions.js
Comment on lines
+981
to
+994
| useEffect(() => { | ||
| if (currentSummit) { | ||
| getMediaUploads("", 1, MAX_PER_PAGE, "name", 1); | ||
| getEvents(); | ||
| // GridFilter persists criteria to localStorage under a summit-agnostic | ||
| // FILTER_ID, so it survives a summit switch unless cleared explicitly here. | ||
| setSelectedFilterCriteria(null); | ||
| resetFilters(); | ||
| } | ||
| }, [currentSummit?.id]); | ||
|
|
||
| useEffect(() => { | ||
| _getEvents(); | ||
| }, [parsedFilter.join(",")]); |
There was a problem hiding this comment.
Gate filter reloads after summit switches.
Line 993 can dispatch with a pre-reset persisted filter during a summit switch, and Line 982 can dispatch before a usable summit id exists. Guard the effects, skip the stale pass after resetFilters(), and reload page 1 when filters actually change.
Proposed fix
-import React, { useEffect, useState } from "react";
+import React, { useEffect, useRef, useState } from "react";
@@
const [selectedColumns, setSelectedColumns] = useState([]);
const [selectedFilterCriteria, setSelectedFilterCriteria] = useState(null);
+ const skipNextFilterFetch = useRef(false);
@@
useEffect(() => {
- if (currentSummit) {
+ if (currentSummit?.id) {
+ skipNextFilterFetch.current = true;
getMediaUploads("", 1, MAX_PER_PAGE, "name", 1);
getEvents();
@@
useEffect(() => {
- _getEvents();
- }, [parsedFilter.join(",")]);
+ if (!currentSummit?.id) return;
+ if (skipNextFilterFetch.current) {
+ skipNextFilterFetch.current = false;
+ return;
+ }
+ _getEvents({ page: DEFAULT_CURRENT_PAGE });
+ }, [currentSummit?.id, parsedFilter.join(",")]);🤖 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/pages/events/summit-event-list-page/index.js` around lines 981 - 994, The
two useEffect hooks need guards to prevent stale API calls during summit
switches and filter changes. In the first useEffect (triggered by
currentSummit?.id), verify that currentSummit has a valid id before calling
getMediaUploads and getEvents, and ensure resetFilters completes before these
calls execute. In the second useEffect (triggered by parsedFilter.join(",")),
add a guard to skip the initial _getEvents call that fires immediately after
resetFilters in the first effect, and only call _getEvents when filters
genuinely change after a summit is fully loaded. Additionally, when filters do
change, ensure the pagination resets to page 1 by passing 1 as the page
parameter to the data fetch function in _getEvents.
santipalenque
force-pushed
the
feature/new-grid-filter-event-list-poc
branch
from
8491aae to
f287f19
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@package.json`:
- Line 95: The openstack-uicore-foundation dependency is pinned to beta version
5.0.36-beta.1 in package.json, which is unsuitable for production as it may
break CSS classes used across the application. Multiple files including
ticket-type-list-page.js, purchase-order-list-page.js, email-log-list-page.js,
summit-attendees-list-page.js, and audit-logs/index.js depend on the
.filters-row and .event-list-date-picker CSS classes exported by this package.
Downgrade the openstack-uicore-foundation dependency from the beta version to
the stable version 5.0.34 in the package.json file to ensure stability and
prevent filtering and date picker functionality from breaking.
In `@src/pages/events/summit-event-list-page/index.js`:
- Around line 310-337: The Dropdown component in the editableField function for
the allow_feedback column is receiving the entire extraProps object for its
value prop instead of extracting the actual boolean value. Change the value prop
in the Dropdown from value={extraProps} to value={extraProps.value} to pass the
correct boolean value. Apply the same fix to the to_record column's
editableField function as well to ensure both boolean dropdown fields work
correctly.
- Around line 915-925: The filter with key "submission_source" has an inaccurate
label of "Submitter Status" that doesn't reflect what it actually filters on
(the source or origin of the submission, such as Admin vs Submission). Update
the label property to accurately describe that the filter is for submission
source, such as "Submission Source" or similar terminology that clearly
indicates it filters by where the submission came from.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e3042f0-b69e-4064-8fd8-92fc9734cc86
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.jsonsrc/actions/__tests__/event-actions.test.jssrc/actions/event-actions.jssrc/components/filters/select-filter-criteria/index.jssrc/components/filters/select-filter-criteria/index.module.lesssrc/i18n/en.jsonsrc/pages/events/__tests__/summit-event-list-page.test.jssrc/pages/events/summit-event-list-page.jssrc/pages/events/summit-event-list-page/components/ImportMUXModal/index.jsxsrc/pages/events/summit-event-list-page/components/ImportModal/index.jsxsrc/pages/events/summit-event-list-page/index.jssrc/styles/summit-event-list-page.less
💤 Files with no reviewable changes (3)
- src/components/filters/select-filter-criteria/index.module.less
- src/styles/summit-event-list-page.less
- src/pages/events/summit-event-list-page.js
✅ Files skipped from review due to trivial changes (1)
- src/actions/tests/event-actions.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/events/tests/summit-event-list-page.test.js
- src/components/filters/select-filter-criteria/index.js
- src/i18n/en.json
- src/actions/event-actions.js
| "moment-duration-format": "^2.3.2", | ||
| "moment-timezone": "^0.5.33", | ||
| "openstack-uicore-foundation": "5.0.34", | ||
| "openstack-uicore-foundation": "5.0.36-beta.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the beta version's package contents and changelog for breaking changes
# Check if the beta version exists and review its published contents
npm view openstack-uicore-foundation@5.0.36-beta.1 dist.tarball
npm view openstack-uicore-foundation@5.0.36-beta.1 version
# Check for latest stable version
npm view openstack-uicore-foundation version
# Look for any references to filters-row or event-list-date-picker in the codebase
echo "== Files using .filters-row or .event-list-date-picker =="
rg -n --type js --type jsx 'className=.*["\'].*\b(filters-row|event-list-date-picker)\b'Repository: fntechgit/summit-admin
Length of output: 444
🏁 Script executed:
#!/bin/bash
# Fix syntax: use proper escaping for the rg pattern
echo "== Files using .filters-row or .event-list-date-picker in className =="
rg -n '(filters-row|event-list-date-picker)' --type js --type jsx
echo ""
echo "== Package.json content (focusing on dependency versions) =="
sed -n '80,110p' package.json
echo ""
echo "== Check latest stable versions of openstack-uicore-foundation =="
npm view openstack-uicore-foundation versions --json | tail -20Repository: fntechgit/summit-admin
Length of output: 1656
🏁 Script executed:
#!/bin/bash
# Broader search for CSS classes - search all file types
echo "== Search for .filters-row and .event-list-date-picker in all files =="
rg '(filters-row|event-list-date-picker)' --type-list 2>&1 | grep -i "css\|scss\|less"
echo ""
echo "== Actually search CSS/SCSS/LESS files =="
rg '(filters-row|event-list-date-picker)' --type css --type scss --type less 2>&1 | head -20
echo ""
echo "== Broader text search (no type restrictions) =="
rg '(filters-row|event-list-date-picker)' 2>&1 | head -30
echo ""
echo "== Check full package.json for react-redux and redux versions =="
cat package.json | grep -A 2 -B 2 '"react-redux"\|"redux"'Repository: fntechgit/summit-admin
Length of output: 1982
🏁 Script executed:
#!/bin/bash
# Check beta version package details and try to find changelog/breaking changes
echo "== Beta version info =="
npm view openstack-uicore-foundation@5.0.36-beta.1 2>&1 | head -50
echo ""
echo "== Check if there's a changelog or breaking changes doc =="
npm view openstack-uicore-foundation@5.0.36-beta.1 repository.url 2>&1
echo ""
echo "== Compare with stable version =="
npm view openstack-uicore-foundation@5.0.34 2>&1 | head -20Repository: fntechgit/summit-admin
Length of output: 1643
Do not use beta version of openstack-uicore-foundation in production code.
Line 95 specifies openstack-uicore-foundation@5.0.36-beta.1, a beta version published recently with no stable release yet. Beta versions are unstable and unsuitable for production.
Additionally, 5 files across the codebase depend on CSS classes exported by this dependency:
src/pages/tickets/ticket-type-list-page.jssrc/pages/orders/purchase-order-list-page.jssrc/pages/emails/email-log-list-page.jssrc/pages/attendees/summit-attendees-list-page.jssrc/components/audit-logs/index.js
All these files use .filters-row and .event-list-date-picker classes. If the beta version removes or breaks these styles, list filtering and date picker functionality will break across the application.
Downgrade to the latest stable version 5.0.34.
🤖 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 `@package.json` at line 95, The openstack-uicore-foundation dependency is
pinned to beta version 5.0.36-beta.1 in package.json, which is unsuitable for
production as it may break CSS classes used across the application. Multiple
files including ticket-type-list-page.js, purchase-order-list-page.js,
email-log-list-page.js, summit-attendees-list-page.js, and audit-logs/index.js
depend on the .filters-row and .event-list-date-picker CSS classes exported by
this package. Downgrade the openstack-uicore-foundation dependency from the beta
version to the stable version 5.0.34 in the package.json file to ensure
stability and prevent filtering and date picker functionality from breaking.
Comment on lines
+310
to
+337
| { | ||
| columnKey: "allow_feedback", | ||
| value: "allow_feedback", | ||
| sortable: false, | ||
| render: (field) => | ||
| field === undefined ? "N/A" : field === true ? "Yes" : "No", | ||
| editableField: (extraProps) => ( | ||
| <Dropdown | ||
| id="allow_feedback" | ||
| value={extraProps} | ||
| options={[ | ||
| { label: "Yes", value: true }, | ||
| { label: "No", value: false } | ||
| ]} | ||
| menuPortalTarget={document.body} | ||
| menuPosition="fixed" | ||
| styles={{ | ||
| menuPortal: (base) => ({ ...base, zIndex: 9999 }), | ||
| control: (base, state) => ({ | ||
| ...base, | ||
| zIndex: state.menuIsOpen ? HIGH_Z_INDEX : DEFAULT_Z_INDEX | ||
| }) | ||
| }} | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...extraProps} | ||
| /> | ||
| ) | ||
| }, |
There was a problem hiding this comment.
Incorrect value prop in boolean field dropdowns.
Lines 319 and 347 pass the entire extraProps object to value instead of extraProps.value. This will cause the Dropdown to receive an object rather than the boolean value, breaking edit functionality for allow_feedback and to_record columns.
editableField: (extraProps) => (
<Dropdown
id="allow_feedback"
- value={extraProps}
+ value={extraProps.value}
options={[
{ label: "Yes", value: true },
{ label: "No", value: false }
]} editableField: (extraProps) => (
<Dropdown
id="to_record"
- value={extraProps}
+ value={extraProps.value}
options={[
{ label: "Yes", value: true },
{ label: "No", value: false }
]}Also applies to: 338-365
🤖 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/pages/events/summit-event-list-page/index.js` around lines 310 - 337, The
Dropdown component in the editableField function for the allow_feedback column
is receiving the entire extraProps object for its value prop instead of
extracting the actual boolean value. Change the value prop in the Dropdown from
value={extraProps} to value={extraProps.value} to pass the correct boolean
value. Apply the same fix to the to_record column's editableField function as
well to ensure both boolean dropdown fields work correctly.
Comment on lines
+915
to
+925
| { | ||
| key: "submission_source", | ||
| label: "Submitter Status", | ||
| operators: [OPERATORS.IS], | ||
| values: { | ||
| type: "select", | ||
| props: { | ||
| options: submissionSourceOptions | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent filter label for submission_source.
The filter with key "submission_source" has label "Submitter Status" but it filters by submission source (Admin vs Submission). Consider renaming for clarity.
{
key: "submission_source",
- label: "Submitter Status",
+ label: "Submission Source",
operators: [OPERATORS.IS],🤖 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/pages/events/summit-event-list-page/index.js` around lines 915 - 925, The
filter with key "submission_source" has an inaccurate label of "Submitter
Status" that doesn't reflect what it actually filters on (the source or origin
of the submission, such as Admin vs Submission). Update the label property to
accurately describe that the filter is for submission source, such as
"Submission Source" or similar terminology that clearly indicates it filters by
where the submission came from.
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
1 participant
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.
https://app.clickup.com/t/86badvda6
Summary by CodeRabbit