WEB-4654 - Filter Bar - #1980
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds localized active-filter chips with removal handling, special site/tag filter states, reorganized summary-dashboard filters, and unconditional applied-filter rendering in the clinic patient list. ChangesClinic patient filters
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClinicPatients
participant AppliedFiltersAdapter
participant AppliedFilters
participant ReduxPatientListFilters
ClinicPatients->>AppliedFiltersAdapter: render activeFilters and setActiveFilters
AppliedFiltersAdapter->>ReduxPatientListFilters: read patientListSearchTextInput
AppliedFiltersAdapter->>AppliedFilters: pass filters and removal callback
AppliedFilters->>AppliedFiltersAdapter: request filter removal
AppliedFiltersAdapter->>ClinicPatients: update activeFilters
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/pages/clinicworkspace/ClinicPatients.js (1)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove these local imports after third-party imports.
These local modules are inserted before later third-party imports such as
material-ui-popup-state, breaking the required grouping. As per coding guidelines, “Group imports in the required order with blank lines between groups: React, PropTypes, Redux, third-party libraries, Lodash specific imports, theme-ui, then local imports.”🤖 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 `@app/pages/clinicworkspace/ClinicPatients.js` around lines 51 - 52, Reorder the imports in ClinicPatients.js so all third-party dependencies, including material-ui-popup-state, appear before the local useClinicPatientsFilters and ActiveFiltersBar imports; preserve the required grouping and add a blank line before local imports.Source: Coding guidelines
app/pages/clinicworkspace/ActiveFiltersBar.js (1)
8-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the component prop contract.
ActiveFiltersBarimportsPropTypesbut does not define them foractiveFiltersorsetActiveFilters. As per coding guidelines, “Define PropTypes for all component props.”🤖 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 `@app/pages/clinicworkspace/ActiveFiltersBar.js` around lines 8 - 54, Declare PropTypes for both activeFilters and setActiveFilters on the ActiveFiltersBar component, matching the expected filter state shape and callback signature; use the existing PropTypes import and mark required props appropriately.Source: Coding guidelines
app/pages/clinicworkspace/components/AppliedFilters.js (2)
1-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the required import grouping.
Third-party, Lodash, theme-ui, and local imports are interleaved. As per coding guidelines, “Group imports in the required order with blank lines between groups: React, PropTypes, Redux, third-party libraries, Lodash specific imports, theme-ui, then local imports.”
🤖 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 `@app/pages/clinicworkspace/components/AppliedFilters.js` around lines 1 - 17, The imports in AppliedFilters.js are not grouped according to the project convention. Reorder them into separate blank-line groups: React, PropTypes, Redux, third-party libraries, Lodash imports, theme-ui, then local imports, including the existing symbols useSelector, useTranslation, withTranslation, Flex, Text, and local components/themes.Source: Coding guidelines
104-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare PropTypes for the new components.
Chip,ChipGroup, andAppliedFiltersaccept props but define no runtime contracts. As per coding guidelines, “Define PropTypes for all component props.”🤖 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 `@app/pages/clinicworkspace/components/AppliedFilters.js` around lines 104 - 235, Add runtime PropTypes declarations for the new Chip, ChipGroup, and AppliedFilters components, covering translation function, labels, callbacks, prefix values, chip collections, and filters with appropriate required/default constraints. Import PropTypes consistently with the file’s existing conventions and attach the declarations directly to each component.Source: Coding guidelines
🤖 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 `@app/pages/clinicworkspace/ClinicPatients.js`:
- Line 4163: Update the ActiveFiltersBar integration and component logic so
removing a chip computes one next-filter object and applies it to both
activeFilters and pendingFilters via setActiveFilters and setPendingFilters.
Pass setPendingFilters from the ClinicPatients render site into
ActiveFiltersBar, ensuring subsequent popover Apply actions cannot restore
removed filters.
In `@app/pages/clinicworkspace/components/AppliedFilters.js`:
- Around line 135-141: Make the removal control in the AppliedFilters chip
keyboard-accessible: update the Icon using onClick={onRemove} from static
rendering to a focusable interactive variant or button element, preserving its
label and styling, and ensure Enter/Space activate onRemove.
---
Nitpick comments:
In `@app/pages/clinicworkspace/ActiveFiltersBar.js`:
- Around line 8-54: Declare PropTypes for both activeFilters and
setActiveFilters on the ActiveFiltersBar component, matching the expected filter
state shape and callback signature; use the existing PropTypes import and mark
required props appropriately.
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 51-52: Reorder the imports in ClinicPatients.js so all third-party
dependencies, including material-ui-popup-state, appear before the local
useClinicPatientsFilters and ActiveFiltersBar imports; preserve the required
grouping and add a blank line before local imports.
In `@app/pages/clinicworkspace/components/AppliedFilters.js`:
- Around line 1-17: The imports in AppliedFilters.js are not grouped according
to the project convention. Reorder them into separate blank-line groups: React,
PropTypes, Redux, third-party libraries, Lodash imports, theme-ui, then local
imports, including the existing symbols useSelector, useTranslation,
withTranslation, Flex, Text, and local components/themes.
- Around line 104-235: Add runtime PropTypes declarations for the new Chip,
ChipGroup, and AppliedFilters components, covering translation function, labels,
callbacks, prefix values, chip collections, and filters with appropriate
required/default constraints. Import PropTypes consistently with the file’s
existing conventions and attach the declarations directly to each component.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cc3487f-2f42-49ba-a5f7-82d25f59e739
⛔ Files ignored due to path filters (1)
app/core/icons/tagIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
app/pages/clinicworkspace/ActiveFiltersBar.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/components/AppliedFilters.jsapp/pages/clinicworkspace/useClinicPatientsFilters.jslocales/en/translation.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tidepool-org/viz(auto-detected)
| } | ||
| /> | ||
| } | ||
| <ActiveFiltersBar activeFilters={activeFilters} setActiveFilters={setActiveFilters} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize pending filters when removing a chip.
ActiveFiltersBar only updates activeFilters; pendingFilters retains the removed value. Opening the affected popover then clicking Apply restores that supposedly removed filter because the popover reads and applies pendingFilters.
Pass setPendingFilters into ActiveFiltersBar and update both states from the same computed next-filter object.
🤖 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 `@app/pages/clinicworkspace/ClinicPatients.js` at line 4163, Update the
ActiveFiltersBar integration and component logic so removing a chip computes one
next-filter object and applies it to both activeFilters and pendingFilters via
setActiveFilters and setPendingFilters. Pass setPendingFilters from the
ClinicPatients render site into ActiveFiltersBar, ensuring subsequent popover
Apply actions cannot restore removed filters.
There was a problem hiding this comment.
This will not be significant as I will be removing pendingFilters entirely in an upcoming PR.
There was a problem hiding this comment.
@henry-tp, understood. Since pendingFilters is being removed in the upcoming PR, this synchronization change is not worth adding here. Would you like me to open a follow-up issue to track the removal and ensure chip removal cannot be reverted afterward?
| <Icon | ||
| className="remove-filter-icon" | ||
| variant="static" | ||
| icon={CloseRoundedIcon} | ||
| label={t('Remove {{ label }} filter', { label })} | ||
| onClick={onRemove} | ||
| sx={{ fontSize: '14px', color: vizColors.indigo50, flexShrink: 0 }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make chip removal keyboard-accessible.
Line 137 renders Icon as static, which resolves to a non-focusable span; keyboard users cannot reach or activate the removal action on Line 140.
Proposed fix
- '&:hover .remove-filter-icon': {
+ '&:hover .remove-filter-icon, &:focus-within .remove-filter-icon': {
width: '14px',
ml: 1,
mr: 2,
opacity: 1,
},
...
- variant="static"
+ variant="button"📝 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.
| <Icon | |
| className="remove-filter-icon" | |
| variant="static" | |
| icon={CloseRoundedIcon} | |
| label={t('Remove {{ label }} filter', { label })} | |
| onClick={onRemove} | |
| sx={{ fontSize: '14px', color: vizColors.indigo50, flexShrink: 0 }} | |
| '&:hover .remove-filter-icon, &:focus-within .remove-filter-icon': { | |
| width: '14px', | |
| ml: 1, | |
| mr: 2, | |
| opacity: 1, | |
| }, | |
| ... | |
| <Icon | |
| className="remove-filter-icon" | |
| variant="button" | |
| icon={CloseRoundedIcon} | |
| label={t('Remove {{ label }} filter', { label })} | |
| onClick={onRemove} | |
| sx={{ fontSize: '14px', color: vizColors.indigo50, flexShrink: 0 }} |
🤖 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 `@app/pages/clinicworkspace/components/AppliedFilters.js` around lines 135 -
141, Make the removal control in the AppliedFilters chip keyboard-accessible:
update the Icon using onClick={onRemove} from static rendering to a focusable
interactive variant or button element, preserving its label and styling, and
ensure Enter/Space activate onRemove.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/clinicworkspace/AppliedFiltersAdapter.js (1)
9-62: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDefine PropTypes for all component props.
As per coding guidelines, PropTypes must be defined for all component props.
AppliedFiltersAdapteracceptsactiveFiltersandsetActiveFiltersbut declares no PropTypes.♻️ Proposed PropTypes
export default AppliedFiltersAdapter; + +AppliedFiltersAdapter.propTypes = { + activeFilters: PropTypes.shape({ + timeCGMUsePercent: PropTypes.string, + lastData: PropTypes.string, + lastDataType: PropTypes.string, + timeInRange: PropTypes.arrayOf(PropTypes.string), + meetsGlycemicTargets: PropTypes.bool, + patientTags: PropTypes.arrayOf(PropTypes.string), + clinicSites: PropTypes.arrayOf(PropTypes.string), + }).isRequired, + setActiveFilters: PropTypes.func.isRequired, +};🤖 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 `@app/pages/clinicworkspace/AppliedFiltersAdapter.js` around lines 9 - 62, Define PropTypes for the activeFilters and setActiveFilters props accepted by AppliedFiltersAdapter, importing PropTypes as needed. Specify activeFilters as the appropriate object shape and setActiveFilters as a required function, then attach the definitions to AppliedFiltersAdapter.Source: Coding guidelines
🧹 Nitpick comments (1)
app/pages/clinicworkspace/AppliedFiltersAdapter.js (1)
12-49: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winWrap
handleRemoveFilterinuseCallback.As per coding guidelines, callback props should use
useCallback. This prevents unnecessary re-renders ofAppliedFilterswhenAppliedFiltersAdapterre-renders without filter changes.♻️ Proposed useCallback wrapper
+import { useCallback } from 'react'; + const AppliedFiltersAdapter = ({ activeFilters, setActiveFilters }) => { const { patientListSearchTextInput } = useSelector(({ blip }) => blip.patientListFilters); - const handleRemoveFilter = (filterKey, value) => { + const handleRemoveFilter = useCallback((filterKey, value) => { switch (filterKey) { // ... existing switch body ... } - }; + }, [activeFilters, setActiveFilters]);🤖 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 `@app/pages/clinicworkspace/AppliedFiltersAdapter.js` around lines 12 - 49, Wrap handleRemoveFilter in React’s useCallback, preserving its existing switch behavior, and include activeFilters and any other referenced values (such as defaultFilterState and setActiveFilters) in the dependency array. Keep the callback passed to AppliedFilters stable when filter inputs have not changed.Source: Coding guidelines
🤖 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 `@app/pages/clinicworkspace/AppliedFiltersAdapter.js`:
- Around line 1-7: Reorder the imports in AppliedFiltersAdapter so React,
PropTypes, Redux, third-party, Lodash, and local imports appear in the required
order, adding blank lines between each group; specifically move PropTypes before
useSelector and separate the import groups.
---
Outside diff comments:
In `@app/pages/clinicworkspace/AppliedFiltersAdapter.js`:
- Around line 9-62: Define PropTypes for the activeFilters and setActiveFilters
props accepted by AppliedFiltersAdapter, importing PropTypes as needed. Specify
activeFilters as the appropriate object shape and setActiveFilters as a required
function, then attach the definitions to AppliedFiltersAdapter.
---
Nitpick comments:
In `@app/pages/clinicworkspace/AppliedFiltersAdapter.js`:
- Around line 12-49: Wrap handleRemoveFilter in React’s useCallback, preserving
its existing switch behavior, and include activeFilters and any other referenced
values (such as defaultFilterState and setActiveFilters) in the dependency
array. Keep the callback passed to AppliedFilters stable when filter inputs have
not changed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2da8836d-0663-4950-9b4f-12ee2c285409
📒 Files selected for processing (3)
app/pages/clinicworkspace/AppliedFiltersAdapter.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/components/AppliedFilters.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/pages/clinicworkspace/components/AppliedFilters.js
- app/pages/clinicworkspace/ClinicPatients.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/pages/clinicworkspace/ClinicPatients.js (2)
4152-4225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
patientListSearchTextInputdependency causes stale query-state closure.
patientListQueryState(Line 4157) is derived fromactiveFiltersandpatientListSearchTextInput, but theuseCallbackdependency array (Lines 4213-4225) only listsactiveFilters, notpatientListSearchTextInput. As the user types/clears search, this memoized callback won't be recreated, soEmptyContentNode's message and theClearFilterButtonsshown both inline (Lines 4166-4173) and in the empty-state (Lines 4188-4193) can reflect a stale search state.🐛 Proposed fix
}, [ activeFilters, clinic?.fetchedPatientCount, columns, data, defaultPatientFetchOptions.sort, handlePageChange, handleSortChange, loading, patientFetchOptions, + patientListSearchTextInput, setActiveFilters, showSummaryData, tableStyle, ]);As per coding guidelines, "respect
react-hooks/exhaustive-deps."🤖 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 `@app/pages/clinicworkspace/ClinicPatients.js` around lines 4152 - 4225, Add patientListSearchTextInput to the dependency array of renderPeopleTable’s useCallback so patientListQueryState and both ClearFilterButtons instances reflect the current search input.Source: Coding guidelines
4159-4173: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
AppliedFiltersAdapterchip removal doesn't syncpendingFilters.
AppliedFiltersAdapter.handleRemoveFilteronly callssetActiveFilters(per its exported contract), never touchingpendingFilters. Every other filter-mutation path in this file (handleResetFilters, tag/site/lastData/cgmUse/timeInRange clear+apply) keepspendingFiltersandactiveFiltersin sync. Removing a chip breaks that invariant: if a filter popover is opened before itsonCloseresetspendingFilterstoactiveFilters, clicking Apply can resurrect the filter just removed via the chip.This is the same issue previously flagged on
ActiveFiltersBar, now reappearing through the newAppliedFiltersAdapterintegration. Fixing it requiresAppliedFiltersAdapter(a different layer in this PR stack) to accept and updatependingFilterstoo, plus wiringsetPendingFiltersthrough here.🐛 Suggested wiring change here (also requires updating AppliedFiltersAdapter.js)
<AppliedFiltersAdapter activeFilters={activeFilters} setActiveFilters={setActiveFilters} + pendingFilters={pendingFilters} + setPendingFilters={setPendingFilters} rightContent={🤖 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 `@app/pages/clinicworkspace/ClinicPatients.js` around lines 4159 - 4173, Update the AppliedFiltersAdapter integration in ClinicPatients so chip removal keeps pendingFilters synchronized with activeFilters: pass setPendingFilters alongside setActiveFilters, and update AppliedFiltersAdapter’s handleRemoveFilter contract to apply the removal to both states. Preserve existing filter-reset and apply behavior.
🤖 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.
Outside diff comments:
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 4152-4225: Add patientListSearchTextInput to the dependency array
of renderPeopleTable’s useCallback so patientListQueryState and both
ClearFilterButtons instances reflect the current search input.
- Around line 4159-4173: Update the AppliedFiltersAdapter integration in
ClinicPatients so chip removal keeps pendingFilters synchronized with
activeFilters: pass setPendingFilters alongside setActiveFilters, and update
AppliedFiltersAdapter’s handleRemoveFilter contract to apply the removal to both
states. Preserve existing filter-reset and apply behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 148fa9d5-a01e-4b2a-8948-e88db72b7e98
📒 Files selected for processing (3)
app/pages/clinicworkspace/AppliedFiltersAdapter.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/components/AppliedFilters.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/pages/clinicworkspace/AppliedFiltersAdapter.js
- app/pages/clinicworkspace/components/AppliedFilters.js
| import { getPatientQueryState } from '../ClinicPatients'; | ||
| import { defaultFilterState } from '../useClinicPatientsFilters'; | ||
| import { Box } from 'theme-ui'; | ||
|
|
There was a problem hiding this comment.
You may think this component is a weird abstraction, and it kind of is in isolation.
The point of this component is to serve as an adapter betweenClinicPatients the ActiveFiltersTray component. It maps and translates the lower component's callbacks + API to the state and setters above. It also helps keep this logic scoped to this component, since ClinicPatients doesn't have a need for this logic otherwise. I will do this for all of the filters
This strategy will allow us to keep the base component's API decoupled from the page it's on, which necessary because we will be mounting it in 2 other dashboards which have different ways that they hold and set state.
I promise it will make a lot more sense when I get to TIDE Dashboard and Device Issues
| import { Box } from 'theme-ui'; | ||
| import styled from '@emotion/styled'; | ||
| import { colors as vizColors } from '@tidepool/viz'; | ||
|
|
There was a problem hiding this comment.
ripped out of ClinicPatients.js
1 participant
WEB-4654