WEB-4654 CGM Use - #2000
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (38)
📝 WalkthroughSummary by CodeRabbit
WalkthroughClinic patient filtering was split into dedicated dropdowns and adapter components. ChangesClinic patient filter controls
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ClinicPatientPage
participant FilterDropdown
participant ActiveFiltersList
participant PatientQuery
ClinicPatientPage->>FilterDropdown: render and apply filter selection
FilterDropdown->>ActiveFiltersList: update active filter state
ActiveFiltersList->>PatientQuery: derive query state and filter parameters
PatientQuery->>ClinicPatientPage: refresh patient table and empty state
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.jsParsing error: [BABEL] /tests/unit/app/pages/clinicworkspace/ClinicPatients.test.js: Using __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.jsParsing error: [BABEL] /tests/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js: Using __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.jsParsing error: [BABEL] /tests/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js: Using
🔧 ast-grep (0.45.1)test/unit/pages/ClinicPatients.test.jsast-grep timed out on this file 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 |
|
This PR was previously approved up to commit 187f974 in PR #1991 by @clintonium-119 However, it was accidentally closed and deleted, which prevents it from being merged. I am re-opening this new PR to be able to merge it. I am no seeking re-approval as it was already approved in PR #1991 |
![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
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/ClinicPatients.js (1)
885-895: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the cleanup update atomic.
useLocalStorageevaluates updater functions against the setter’s capturedstoredValue. Update it to apply the updater through React’s functionalsetStoredValue, then use that updater here to preserve unrelated filters.🤖 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 885 - 895, Make useLocalStorage apply updater functions via React’s functional setStoredValue so each update receives the latest stored value. In the ClinicPatients filter-cleanup effect, call setActiveFilters with a functional updater and derive patientTags and clinicSites from the previous filters, preserving all unrelated filter fields while removing invalid values.
🧹 Nitpick comments (20)
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js (2)
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the PropTypes values from a plain value list.
Line 157 calls
getCgmUseFilterOptions(label => label)only to recover the option values. The identity function stands in fort. The intent is not obvious, and the call couples the PropTypes declaration to the label-building signature. Extract the values into a separate constant.♻️ Proposed extraction
-const getCgmUseFilterOptions = (t) => [ - { value: '<0.7', label: t('Less than 70%') }, - { value: '>=0.7', label: t('70% or more') }, -]; +const CGM_USE_FILTER_VALUES = ['<0.7', '>=0.7']; + +const getCgmUseFilterOptions = (t) => [ + { value: '<0.7', label: t('Less than 70%') }, + { value: '>=0.7', label: t('70% or more') }, +];- timeCGMUsePercent: PropTypes.oneOf(getCgmUseFilterOptions(label => label).map(opt => opt.value)), + timeCGMUsePercent: PropTypes.oneOf(CGM_USE_FILTER_VALUES),The guideline requires UPPER_SNAKE_CASE for constants. As per coding guidelines: "Use PascalCase for components, camelCase for utilities, and UPPER_SNAKE_CASE for constants."
Also applies to: 157-157
🤖 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/CGMUseFilterDropdown.js` around lines 19 - 22, Extract the option values into a separate UPPER_SNAKE_CASE constant and have getCgmUseFilterOptions map those values to translated labels. Update the PropTypes declaration to derive its values directly from this constant instead of calling getCgmUseFilterOptions with an identity function.Source: Coding guidelines
1-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup the imports in the required order. All three new dropdowns place the local
trackMetricimport among the third-party imports and place the Lodash import aftertheme-ui. The shared root cause is one deviation from the required import grouping, repeated in each file. The required order is React, PropTypes, Redux, third-party libraries, Lodash imports, theme-ui, then local imports, with a blank line between groups.
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L1-L18: movetrackMetricdown to the local group with the other../../../imports, and movelodash/noopabove thetheme-uiimport.app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js#L1-L18: apply the same two moves.app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js#L1-L19: apply the same two moves.♻️ Example for CGMUseFilterDropdown.js
import React, { useState } from 'react'; import PropTypes from 'prop-types'; import { useSelector } from 'react-redux'; import { useTranslation } from 'react-i18next'; -import { trackMetric } from '../../../core/metricUtils'; import { colors as vizColors } from '`@tidepool/viz`'; - -import { Box, Grid } from 'theme-ui'; import KeyboardArrowDownRoundedIcon from '`@material-ui/icons/KeyboardArrowDownRounded`'; +import { bindPopover, bindTrigger, usePopupState } from 'material-ui-popup-state/hooks'; + import noop from 'lodash/noop'; -import { bindPopover, bindTrigger, usePopupState } from 'material-ui-popup-state/hooks'; +import { Box, Grid } from 'theme-ui'; +import { trackMetric } from '../../../core/metricUtils'; import Button from '../../../components/elements/Button';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/CGMUseFilterDropdown.js` around lines 1 - 18, Reorder imports in CGMUseFilterDropdown.js (lines 1-18), DataRecencyFilterDropdown.js (lines 1-18), and SummaryPeriodFilterDropdown.js (lines 1-19): place lodash/noop before theme-ui, move trackMetric into the local ../../../ import group, and preserve blank lines between the required React, PropTypes, Redux, third-party, Lodash, theme-ui, and local groups.Source: Coding guidelines
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js (1)
12-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the setter directly, or wrap it in
useCallback.
handleChangeforwards its argument tosetActiveSummaryPeriodwithout change. The wrapper adds no behavior, and a new function identity is created on every render.♻️ Proposed simplification
- const handleChange = (summaryPeriod) => { - setActiveSummaryPeriod(summaryPeriod); - }; - return ( <SummaryPeriodFilterDropdown - onChange={handleChange} + onChange={setActiveSummaryPeriod} activeSummaryPeriod={activeSummaryPeriod} /> );The sibling adapters need the wrapper because they merge into
activeFilters. This one does not. As per coding guidelines: "useuseCallbackfor callback 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/clinicPatientsFilters/FilterBySummaryPeriod.js` around lines 12 - 20, Update FilterBySummaryPeriod’s SummaryPeriodFilterDropdown usage to pass setActiveSummaryPeriod directly as onChange, removing the redundant handleChange wrapper; alternatively memoize the wrapper with useCallback if the component requires it.Source: Coding guidelines
app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js (1)
128-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep one fallback for
filterOptions.Line 130 sets a default parameter, and line 180 repeats the fallback with
filterOptions || lastDataFilterOptions. The default parameter coversundefined, and the||covers an explicitnull. Both paths resolve to the same value. Drop the default parameter or the inline fallback so the source of the default is unambiguous.♻️ Proposed simplification
- filterOptions={filterOptions || lastDataFilterOptions} + filterOptions={filterOptions}Also applies to: 176-184
🤖 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/DataRecencyFilterDropdown.js` around lines 128 - 131, Keep only one fallback for filterOptions in the component function and its usage near the filter construction: either remove the default parameter or remove the inline “|| lastDataFilterOptions” fallback. Preserve the existing behavior for undefined and null values while making the default source unambiguous.__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js (3)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the Jest test titles to the required form. The new dropdown test files use behavior-describing titles that do not follow the required
should do X when Yform. The shared root cause is one naming convention applied inconsistently across the new Jest suites.
__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js#L53-L53: rename all fiveittitles in this file, for exampleshould call onChange with the selected ranges when Apply is clicked.__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js#L48-L48: rename all threeittitles in this file, for exampleshould apply the summary period when a different radio is selected.As per coding guidelines: "In Jest tests, use
jest.fn()for mocks, clear mocks inbeforeEachorafterEach, use descriptive names likeshould do X when Y, and test interactions withuserEvent."🤖 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 `@__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js` at line 53, Rename all five Jest test titles in __tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js (anchor 53-53) and all three titles in __tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js (sibling 48-48) to descriptive “should do X when Y” forms, accurately describing each test’s behavior and trigger; make no other changes.Source: Coding guidelines
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the
metricUtilsmock explicitly, and drop the unused mock handle. Both new dropdown test files importtrackMetric as mockTrackMetricand callmockTrackMetric.mockClear()without a localjest.mockfor that module. The shared root cause is an implicit dependency on a global mock configuration.
__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js#L12-L12: this file never asserts onmockTrackMetric. Remove the import and themockClear()call at line 49, or add assertions for the open and close metrics. This file already declaresjest.mock('launchdarkly-react-client-sdk')at line 14, which shows the intended explicit style.__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js#L10-L10: this file asserts onmockTrackMetric, so addjest.mock('@app/core/metricUtils')and import through the@appalias for consistency with line 9.🤖 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 `@__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js` at line 12, In __tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js:12-12, remove the unused trackMetric import and its mockClear call, since this test does not assert metrics. In __tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js:10-10, explicitly mock `@app/core/metricUtils` and import trackMetric through the `@app` alias, preserving its existing metric assertions.
100-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the popup stays open across the
rerendercall.This test opens the dropdown, changes the
useFlagsreturn value, and then callsrerender(ui()). The assertion at line 110 depends onusePopupStatekeeping the open state through the rerender. That holds while React reconciles the same element type and preserves the internal state. The behavior is implicit, so a future change to the popup state library or to the wrapper structure would make this test pass or fail for the wrong reason. Add an explicit assertion that the dropdown is still open after the rerender.🧪 Proposed guard assertion
useFlags.mockReturnValue({ showExtremeHigh: true }); rerender(ui()); + expect(screen.getByTestId('time-in-range-filter-dropdown')).toBeInTheDocument(); expect(screen.getByRole('checkbox', { name: /Extremely High/ })).toBeInTheDocument();🤖 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 `@__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js` around lines 100 - 111, Update the test “shows the highest range option only when the showExtremeHigh flag is set” to explicitly assert that the Time in Range dropdown remains open immediately after rerender(ui()). Keep the existing Extremely High visibility assertion and use the dropdown’s existing open-state indicator or trigger/popup query.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required
should do X when Ytest titles.Both
ittitles state the behavior but do not follow the required form.✏️ Proposed titles
- it('calls setActiveSummaryPeriod with the newly selected period', async () => { + it('should call setActiveSummaryPeriod with the selected period when Apply is clicked', async () => {- it('reflects the active summary period in the trigger label and the pre-selected radio', async () => { + it('should show the active period in the trigger label and pre-check its radio when activeSummaryPeriod is set', async () => {As per coding guidelines: "In Jest tests, use
jest.fn()for mocks, clear mocks inbeforeEachorafterEach, use descriptive names likeshould do X when Y, and test interactions withuserEvent."Also applies to: 64-64
🤖 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 `@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js` at line 46, Rename the affected Jest test titles in the filter summary period tests to follow the required “should do X when Y” format, including the test at the visible setActiveSummaryPeriod case and the additional test at the referenced location. Preserve each test’s existing behavior and assertions.Source: Coding guidelines
__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js (1)
93-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the close-icon metric.
The tests assert the open, apply, and cancel metrics.
SummaryPeriodFilterDropdownalso emits'Clinic - Summary period filter close'fromonClickCloseIcon. No test exercises that path, so a regression in the close handler stays undetected. Add one assertion that clicks the popover close icon and checks the emitted event.🤖 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 `@__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js` around lines 93 - 111, Add coverage in the SummaryPeriodFilterDropdown tests for the popover close-icon path: open the dropdown, click the close icon through its accessible control, and assert mockTrackMetric was called with 'Clinic - Summary period filter close' and the same clinicId/pageName metadata used by the other metric assertions.app/pages/clinicworkspace/components/SiteFilterDropdown.js (2)
204-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse past-tense metric event names.
The new event names use
open,close,apply, andclear. Rename them to the established past-tense form. Update the matching test expectations.
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L204-L218: rename clear and apply events.app/pages/clinicworkspace/components/SiteFilterDropdown.js#L255-L298: rename open and close events.app/pages/clinicworkspace/components/TagFilterDropdown.js#L207-L220: rename clear and apply events.app/pages/clinicworkspace/components/TagFilterDropdown.js#L258-L301: rename open and close events.__tests__/unit/app/pages/clinicworkspace/components/SiteFilterDropdown.test.js#L93-L130: update metric assertions.__tests__/unit/app/pages/clinicworkspace/components/TagFilterDropdown.test.js#L93-L130: update metric assertions.Based on learnings, use past-tense event name strings for
trackMetriccalls.🤖 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/SiteFilterDropdown.js` around lines 204 - 218, Rename the trackMetric event strings to their established past-tense forms for clear/apply in SiteFilterDropdown and TagFilterDropdown, and for open/close in both dropdown components. Update the corresponding metric assertions in app/pages/clinicworkspace/components/SiteFilterDropdown.js lines 204-218 and 255-298, app/pages/clinicworkspace/components/TagFilterDropdown.js lines 207-220 and 258-301, and both matching test files at lines 93-130 to expect the renamed events.Source: Learnings
37-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine PropTypes for the private dropdown components.
EditSitesAction,DropdownContent,EditTagsAction, andDropdownContentaccept props without runtime contracts. Define their callback and filter-array PropTypes.
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L37-L77: add PropTypes forEditSitesActionandDropdownContent.app/pages/clinicworkspace/components/TagFilterDropdown.js#L37-L80: add PropTypes forEditTagsActionandDropdownContent.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/SiteFilterDropdown.js` around lines 37 - 77, Define runtime PropTypes for the private components EditSitesAction and DropdownContent in app/pages/clinicworkspace/components/SiteFilterDropdown.js (lines 37-77), covering the callback props and clinicSites filter array. Apply the same PropTypes definitions to EditTagsAction and DropdownContent in app/pages/clinicworkspace/components/TagFilterDropdown.js (lines 37-80), covering their callbacks and tag filter array.Source: Coding guidelines
app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.js (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
timeCGMUsePercentPropType with the sibling components.
ActiveFiltersTray.js(line 273) andAppliedFiltersList.js(line 123) both declare this field asPropTypes.oneOf(['<0.7', '>=0.7']). This file declaresPropTypes.string. The value is used directly as thecgm.timeCGMUsePercentquery parameter, so the narrower type documents the contract and catches an invalid value at the adapter boundary.♻️ Proposed refactor
FilterByCGMUse.propTypes = { activeFilters: PropTypes.shape({ - timeCGMUsePercent: PropTypes.string, + timeCGMUsePercent: PropTypes.oneOf(['<0.7', '>=0.7']), }), setActiveFilters: PropTypes.func, };🤖 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/clinicPatientsFilters/FilterByCGMUse.js` around lines 25 - 30, Update the `timeCGMUsePercent` declaration in `FilterByCGMUse.propTypes` from `PropTypes.string` to `PropTypes.oneOf(['<0.7', '>=0.7'])`, matching the contract used by `ActiveFiltersTray` and `AppliedFiltersList`.app/pages/clinicworkspace/components/ActiveFiltersTray.js (2)
114-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd PropTypes to
ChipandChipGroup.
ActiveFiltersTraydeclares PropTypes at lines 269-281, butChip(line 114) andChipGroup(line 167) declare none. Both are React components with required props:ChipneedslabelandonRemove, andChipGroupneedschipsandonRemove.♻️ Proposed addition
+Chip.propTypes = { + label: PropTypes.string.isRequired, + onRemove: PropTypes.func.isRequired, +}; + const ChipGroup = ({ prefix, chips, onRemove }) => {+ChipGroup.propTypes = { + prefix: PropTypes.node, + chips: PropTypes.arrayOf(PropTypes.shape({ + type: PropTypes.string.isRequired, + value: PropTypes.string, + label: PropTypes.string, + })), + onRemove: PropTypes.func.isRequired, +}; + const ActiveFiltersTray = ({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/ActiveFiltersTray.js` around lines 114 - 183, Add PropTypes declarations for the Chip and ChipGroup components. Mark Chip.label and Chip.onRemove as required, and mark ChipGroup.chips and ChipGroup.onRemove as required, reusing the file’s existing PropTypes import and conventions.Source: Coding guidelines
68-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared chip logic from
useTagChipsanduseSiteChips.The two hooks are structurally identical. They differ only in the filter key, the clinic collection, the special-state constant, and the empty label. A single parameterized hook removes the duplication and keeps future changes, such as new sorting rules, in one place.
♻️ Proposed refactor
-const useTagChips = (patientTags = []) => { - const { t } = useTranslation(); - const selectedClinicId = useSelector(state => state.blip.selectedClinicId); - const clinic = useSelector(state => state.blip.clinics?.[selectedClinicId]); - - if (isEqual(patientTags, SPECIAL_FILTER_STATES.ZERO_TAGS)) { - return [{ - type: 'patientTags', - value: SPECIAL_FILTER_STATES.ZERO_TAGS[0], - label: t('No tags'), - }]; - } - - return patientTags - .map(id => ({ - type: 'patientTags', - value: id, - label: find(clinic?.patientTags, { id })?.name, - })) - .filter(chip => chip.label) - .toSorted((a, b) => utils.compareLabels(a.label, b.label)); -}; - -const useSiteChips = (clinicSites = []) => { - const { t } = useTranslation(); - const selectedClinicId = useSelector(state => state.blip.selectedClinicId); - const clinic = useSelector(state => state.blip.clinics?.[selectedClinicId]); - - if (isEqual(clinicSites, SPECIAL_FILTER_STATES.ZERO_SITES)) { - return [{ - type: 'clinicSites', - value: SPECIAL_FILTER_STATES.ZERO_SITES[0], - label: t('No clinic sites'), - }]; - } - - return clinicSites - .map(id => ({ - type: 'clinicSites', - value: id, - label: find(clinic?.sites, { id })?.name, - })) - .filter(chip => chip.label) - .toSorted((a, b) => utils.compareLabels(a.label, b.label)); -}; +const useEntityChips = ({ ids = [], type, clinicField, zeroState, zeroLabel }) => { + const selectedClinicId = useSelector(state => state.blip.selectedClinicId); + const clinic = useSelector(state => state.blip.clinics?.[selectedClinicId]); + + if (isEqual(ids, zeroState)) { + return [{ type, value: zeroState[0], label: zeroLabel }]; + } + + return ids + .map(id => ({ type, value: id, label: find(clinic?.[clinicField], { id })?.name })) + .filter(chip => chip.label) + .toSorted((a, b) => utils.compareLabels(a.label, b.label)); +};Then call it from
ActiveFiltersTray, wheretis already available:const primaryChips = usePrimaryChips(filters); - const tagChips = useTagChips(filters.patientTags); - const siteChips = useSiteChips(filters.clinicSites); + const tagChips = useEntityChips({ + ids: filters.patientTags, + type: 'patientTags', + clinicField: 'patientTags', + zeroState: SPECIAL_FILTER_STATES.ZERO_TAGS, + zeroLabel: t('No tags'), + }); + const siteChips = useEntityChips({ + ids: filters.clinicSites, + type: 'clinicSites', + clinicField: 'sites', + zeroState: SPECIAL_FILTER_STATES.ZERO_SITES, + zeroLabel: t('No clinic sites'), + });🤖 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/ActiveFiltersTray.js` around lines 68 - 112, Extract the duplicated logic from useTagChips and useSiteChips into one parameterized chip hook or helper that accepts the filter key, clinic collection, special-state value, and translated empty-state label. Preserve the existing special-state handling, label lookup, filtering, and sorting, then update ActiveFiltersTray to call the shared implementation for tags and sites using the existing t function.app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.js (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
vizUtils.bgdestructure below the import block.Line 27 places a
conststatement between import statements at lines 24-25 and lines 28-29. The code works because ES module imports are hoisted, but it breaks the grouped import block and hides the two local imports that follow it.♻️ Proposed refactor
import { colors } from '../../../themes/baseTheme'; import { MGDL_UNITS } from '../../../core/constants'; - -const { reshapeBgClassesToBgBounds, generateBgRangeLabels } = vizUtils.bg; import useClinicMetricsPageName from '../useClinicMetricsPageName'; import { timeInRangeFilterThresholds } from '../../../core/clinicUtils'; + +const { reshapeBgClassesToBgBounds, generateBgRangeLabels } = vizUtils.bg;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/TimeInRangeFilterDropdown.js` around lines 24 - 29, Move the vizUtils.bg destructuring declaration below the complete import block, keeping useClinicMetricsPageName and timeInRangeFilterThresholds grouped with the other local imports; preserve the existing import ordering and add the required group spacing without changing behavior.Source: Coding guidelines
app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.js (1)
86-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
hasActiveFiltersfromgetPatientQueryStateinstead of duplicating the checks.Lines 86-93 repeat the exact condition already implemented in
getPatientQueryStateat lines 17-24. Two copies of the same rule will drift when a new filter key is added, and the tray would then hide while a filter is still applied.♻️ Proposed refactor
- const hasSearchActive = !!patientListSearchTextInput; - - const hasActiveFilters = !!( - activeFilters.lastData || - activeFilters.lastDataType || - activeFilters.timeCGMUsePercent || - activeFilters.timeInRange?.length > 0 || - activeFilters.patientTags?.length > 0 || - activeFilters.clinicSites?.length > 0 - ); - - const isRendered = hasActiveFilters || hasSearchActive; - - if (!isRendered) return null; - - const patientQueryState = getPatientQueryState(activeFilters, patientListSearchTextInput); + const hasSearchActive = !!patientListSearchTextInput; + const patientQueryState = getPatientQueryState(activeFilters, patientListSearchTextInput); + + if (patientQueryState === PATIENT_QUERY_STATE.NONE) return null;🤖 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/clinicPatientsFilters/AppliedFiltersList.js` around lines 86 - 93, Update hasActiveFilters in AppliedFiltersList to derive its value from the existing getPatientQueryState result instead of duplicating individual activeFilters checks. Reuse that helper’s established active-filter state so newly supported filter keys remain consistent with the tray’s visibility behavior.app/pages/clinicworkspace/components/ClearFilterButtons.js (1)
15-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet an explicit button type on
ClearButton.
styled.buttonproduces a nativebuttonthat defaults totype="submit". If this component is ever rendered inside aform, a click submits the form in addition to calling the handler. Set the type on the styled component so every usage is safe.♻️ Proposed refactor
const ClearButton = styled.button` background: none; color: ${vizColors.indigo30}; border: none; padding: 0; font: inherit; cursor: pointer; text-underline-offset: 4px; text-decoration: underline; `; + +ClearButton.defaultProps = { type: 'button' };An alternative is to pass
type="button"at each of the four call sites.🤖 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/ClearFilterButtons.js` around lines 15 - 24, Update the styled.button definition for ClearButton to set its default native button type to "button", ensuring all usages avoid unintended form submission without changing the call sites.__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js (1)
388-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the six new tests to the
should ... when ...form.All six new test names start with
maps an applied ... filter into the getPatientsForClinic query. The surrounding tests in this file use theshould ...form, for exampleshould allow creating a new site for a workspaceat line 524.♻️ Proposed renames
- it('maps an applied tag filter into the getPatientsForClinic query', async () => { + it('should include tags in the getPatientsForClinic query when a tag filter is applied', async () => {- it('maps an applied summary period filter into the getPatientsForClinic query', async () => { + it('should set the period in the getPatientsForClinic query when a summary period is applied', async () => {- it('maps an applied site filter into the getPatientsForClinic query', async () => { + it('should include sites in the getPatientsForClinic query when a site filter is applied', async () => {- it('maps an applied data recency filter into the getPatientsForClinic query', async () => { + it('should include lastData bounds in the getPatientsForClinic query when a data recency filter is applied', async () => {- it('maps an applied time in range filter into the getPatientsForClinic query', async () => { + it('should include range comparators in the getPatientsForClinic query when a time in range filter is applied', async () => {- it('maps an applied cgm use filter into the getPatientsForClinic query', async () => { + it('should include timeCGMUsePercent in the getPatientsForClinic query when a CGM use filter is applied', async () => {As per coding guidelines: "In Jest tests, use
jest.fn()for mocks, clear mocks inbeforeEachorafterEach, use descriptive names likeshould do X when Y, and test interactions withuserEvent."Also applies to: 408-408, 427-427, 447-447, 475-475, 500-500
🤖 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 `@__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js` at line 388, Rename the six newly added test cases around the getPatientsForClinic query to the repository’s “should ... when ...” naming convention, preserving each test’s existing filter-specific behavior and intent. Update the tests at the applied tag, status, search, pagination, sorting, and other filter scenarios without changing their implementations.Source: Coding guidelines
app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.js (1)
21-21: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHoist
customLastDataFilterOptionsto module scope.lastDataFilterOptionsis an array, andrejectremoves only value7. The current component creates a new options array on each render.🤖 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/clinicPatientsFilters/FilterByDataRecency.js` at line 21, Move the customLastDataFilterOptions definition out of the component and into module scope, alongside lastDataFilterOptions, so reject is evaluated only once while preserving the existing removal of value 7.app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder the import groups.
Place Redux imports before other third-party imports. In the adapters, place the Lodash import before local action and metric imports.
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js#L1-L10: movelodash/noopbefore the local Redux action and metric imports.app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.js#L1-L10: movelodash/noopbefore the local Redux action and metric imports.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js#L1-L9: move Redux imports before Testing Library imports.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js#L1-L9: move Redux imports before Testing Library imports.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js#L1-L13: move Redux imports before Testing Library imports.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js#L1-L13: move Redux imports before Testing Library imports.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTimeInRange.test.js#L1-L10: move Redux imports before Testing Library imports.__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js#L1-L10: move Redux imports before Testing Library imports.__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js#L1-L10: move Redux imports before Testing Library imports.As per coding guidelines: “Group imports in the required order with blank lines between groups.”
🤖 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/clinicPatientsFilters/FilterBySites.js` around lines 1 - 10, Reorder imports into the required groups with blank lines between them. In app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js and FilterByTags.js, place lodash/noop before the local Redux action and metric imports; in each listed test file, place Redux imports before Testing Library imports. Apply the same ordering to all nine specified files and make no other changes.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`:
- Around line 1054-1056: Guard the timeInRangeFilterThresholds lookup in the
activeFilters.timeInRange iteration before destructuring, skipping unknown
filter keys so the fetch-options effect continues rendering. Also remove
unrecognized keys from activeFilters.timeInRange so filter counts and
applied-filter chips match the filters used in the query.
- Line 3157: Update the memoized renderPeopleTable callback dependencies to
include patientListSearchTextInput, handleClearSearch, and handleResetFilters.
Wrap handleClearSearch and handleResetFilters in useCallback with complete
dependencies before adding them, ensuring patientQueryState and clear controls
update when search text or filter actions change.
In `@app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js`:
- Around line 29-32: Update handleClickEditSites and the corresponding tag edit
handler so each fetch dispatch occurs before trackMetric, with tracking called
immediately afterward and using the selected past-tense event name. Update the
metric expectations in
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js:104-109 and
app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.js:106-111 to
match; apply the handler changes in
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js:29-32 and
FilterByTags.js:29-32.
In `@app/pages/clinicworkspace/components/CGMUseFilterDropdown.js`:
- Around line 24-28: Define PropTypes for the inner DropdownContent component in
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js lines 24-28 for
required onClose, onChange, and timeCGMUsePercent props;
app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js lines 20-26
for onClose, onChange, lastData, lastDataType, and filterOptions; and
app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js lines 32-36
for onClose, onChange, and activeSummaryPeriod, using the appropriate existing
PropTypes shapes and requiredness.
In `@app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js`:
- Around line 156-164: Update the iconLabel on the last-data-filter-trigger
Button to use the same “Data Recency” wording as its visible label, and update
any tests or selectors that depend on “Filter by last upload.”
- Line 89: In DataRecencyFilterDropdown.js at lines 89-89 and 150-150, define
and reuse one approved metric-name prefix for every open, close, apply, and
clear event, replacing the inconsistent Last data/Last upload variants while
preserving or migrating existing dashboard names. In CGMUseFilterDropdown.js at
line 117, apply the same consistency to all dropdown events by standardizing CGM
Use versus CGM use under one approved prefix; update each affected trackMetric
call rather than only the cited clear event.
In `@app/pages/clinicworkspace/components/SiteFilterDropdown.js`:
- Around line 260-267: Use react-i18next through the existing component
translation pattern, then pass the translated strings to the trigger iconLabel
props: update SiteFilterDropdown.js lines 260-267 for “Filter by clinic sites”
and TagFilterDropdown.js lines 263-270 for “Filter by patient tags”; ensure both
components obtain the translation function via useTranslation() or
withTranslation().
- Around line 89-92: Replace the unsupported toSorted call in
sortedSiteFilterOptions with sort on the newly mapped array, preserving label
comparison and avoiding mutation of the source sites. Apply the same change in
app/pages/clinicworkspace/components/TagFilterDropdown.js lines 92-95.
---
Outside diff comments:
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 885-895: Make useLocalStorage apply updater functions via React’s
functional setStoredValue so each update receives the latest stored value. In
the ClinicPatients filter-cleanup effect, call setActiveFilters with a
functional updater and derive patientTags and clinicSites from the previous
filters, preserving all unrelated filter fields while removing invalid values.
---
Nitpick comments:
In `@__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js`:
- Line 388: Rename the six newly added test cases around the
getPatientsForClinic query to the repository’s “should ... when ...” naming
convention, preserving each test’s existing filter-specific behavior and intent.
Update the tests at the applied tag, status, search, pagination, sorting, and
other filter scenarios without changing their implementations.
In
`@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js`:
- Line 46: Rename the affected Jest test titles in the filter summary period
tests to follow the required “should do X when Y” format, including the test at
the visible setActiveSummaryPeriod case and the additional test at the
referenced location. Preserve each test’s existing behavior and assertions.
In
`@__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js`:
- Around line 93-111: Add coverage in the SummaryPeriodFilterDropdown tests for
the popover close-icon path: open the dropdown, click the close icon through its
accessible control, and assert mockTrackMetric was called with 'Clinic - Summary
period filter close' and the same clinicId/pageName metadata used by the other
metric assertions.
In
`@__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js`:
- Line 53: Rename all five Jest test titles in
__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js
(anchor 53-53) and all three titles in
__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js
(sibling 48-48) to descriptive “should do X when Y” forms, accurately describing
each test’s behavior and trigger; make no other changes.
- Line 12: In
__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js:12-12,
remove the unused trackMetric import and its mockClear call, since this test
does not assert metrics. In
__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js:10-10,
explicitly mock `@app/core/metricUtils` and import trackMetric through the `@app`
alias, preserving its existing metric assertions.
- Around line 100-111: Update the test “shows the highest range option only when
the showExtremeHigh flag is set” to explicitly assert that the Time in Range
dropdown remains open immediately after rerender(ui()). Keep the existing
Extremely High visibility assertion and use the dropdown’s existing open-state
indicator or trigger/popup query.
In `@app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.js`:
- Around line 86-93: Update hasActiveFilters in AppliedFiltersList to derive its
value from the existing getPatientQueryState result instead of duplicating
individual activeFilters checks. Reuse that helper’s established active-filter
state so newly supported filter keys remain consistent with the tray’s
visibility behavior.
In `@app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.js`:
- Around line 25-30: Update the `timeCGMUsePercent` declaration in
`FilterByCGMUse.propTypes` from `PropTypes.string` to `PropTypes.oneOf(['<0.7',
'>=0.7'])`, matching the contract used by `ActiveFiltersTray` and
`AppliedFiltersList`.
In `@app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.js`:
- Line 21: Move the customLastDataFilterOptions definition out of the component
and into module scope, alongside lastDataFilterOptions, so reject is evaluated
only once while preserving the existing removal of value 7.
In `@app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js`:
- Around line 1-10: Reorder imports into the required groups with blank lines
between them. In
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js and
FilterByTags.js, place lodash/noop before the local Redux action and metric
imports; in each listed test file, place Redux imports before Testing Library
imports. Apply the same ordering to all nine specified files and make no other
changes.
In `@app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js`:
- Around line 12-20: Update FilterBySummaryPeriod’s SummaryPeriodFilterDropdown
usage to pass setActiveSummaryPeriod directly as onChange, removing the
redundant handleChange wrapper; alternatively memoize the wrapper with
useCallback if the component requires it.
In `@app/pages/clinicworkspace/components/ActiveFiltersTray.js`:
- Around line 114-183: Add PropTypes declarations for the Chip and ChipGroup
components. Mark Chip.label and Chip.onRemove as required, and mark
ChipGroup.chips and ChipGroup.onRemove as required, reusing the file’s existing
PropTypes import and conventions.
- Around line 68-112: Extract the duplicated logic from useTagChips and
useSiteChips into one parameterized chip hook or helper that accepts the filter
key, clinic collection, special-state value, and translated empty-state label.
Preserve the existing special-state handling, label lookup, filtering, and
sorting, then update ActiveFiltersTray to call the shared implementation for
tags and sites using the existing t function.
In `@app/pages/clinicworkspace/components/CGMUseFilterDropdown.js`:
- Around line 19-22: Extract the option values into a separate UPPER_SNAKE_CASE
constant and have getCgmUseFilterOptions map those values to translated labels.
Update the PropTypes declaration to derive its values directly from this
constant instead of calling getCgmUseFilterOptions with an identity function.
- Around line 1-18: Reorder imports in CGMUseFilterDropdown.js (lines 1-18),
DataRecencyFilterDropdown.js (lines 1-18), and SummaryPeriodFilterDropdown.js
(lines 1-19): place lodash/noop before theme-ui, move trackMetric into the local
../../../ import group, and preserve blank lines between the required React,
PropTypes, Redux, third-party, Lodash, theme-ui, and local groups.
In `@app/pages/clinicworkspace/components/ClearFilterButtons.js`:
- Around line 15-24: Update the styled.button definition for ClearButton to set
its default native button type to "button", ensuring all usages avoid unintended
form submission without changing the call sites.
In `@app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js`:
- Around line 128-131: Keep only one fallback for filterOptions in the component
function and its usage near the filter construction: either remove the default
parameter or remove the inline “|| lastDataFilterOptions” fallback. Preserve the
existing behavior for undefined and null values while making the default source
unambiguous.
In `@app/pages/clinicworkspace/components/SiteFilterDropdown.js`:
- Around line 204-218: Rename the trackMetric event strings to their established
past-tense forms for clear/apply in SiteFilterDropdown and TagFilterDropdown,
and for open/close in both dropdown components. Update the corresponding metric
assertions in app/pages/clinicworkspace/components/SiteFilterDropdown.js lines
204-218 and 255-298, app/pages/clinicworkspace/components/TagFilterDropdown.js
lines 207-220 and 258-301, and both matching test files at lines 93-130 to
expect the renamed events.
- Around line 37-77: Define runtime PropTypes for the private components
EditSitesAction and DropdownContent in
app/pages/clinicworkspace/components/SiteFilterDropdown.js (lines 37-77),
covering the callback props and clinicSites filter array. Apply the same
PropTypes definitions to EditTagsAction and DropdownContent in
app/pages/clinicworkspace/components/TagFilterDropdown.js (lines 37-80),
covering their callbacks and tag filter array.
In `@app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.js`:
- Around line 24-29: Move the vizUtils.bg destructuring declaration below the
complete import block, keeping useClinicMetricsPageName and
timeInRangeFilterThresholds grouped with the other local imports; preserve the
existing import ordering and add the required group spacing without changing
behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a0148559-4fba-492c-875d-585bb61a3825
⛔ Files ignored due to path filters (1)
app/core/icons/tagIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (38)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTimeInRange.test.js__tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js__tests__/unit/app/pages/clinicworkspace/components/SiteFilterDropdown.test.js__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js__tests__/unit/app/pages/clinicworkspace/components/TagFilterDropdown.test.js__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.jsapp/core/clinicUtils.jsapp/pages/clinicadmin/clinicadmin.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByTimeInRange.jsapp/pages/clinicworkspace/components/ActiveFiltersTray.jsapp/pages/clinicworkspace/components/CGMUseFilterDropdown.jsapp/pages/clinicworkspace/components/ClearFilterButtons.jsapp/pages/clinicworkspace/components/DataRecencyFilterDropdown.jsapp/pages/clinicworkspace/components/SiteFilterDropdown.jsapp/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.jsapp/pages/clinicworkspace/components/TagFilterDropdown.jsapp/pages/clinicworkspace/components/TimeInRangeFilterDropdown.jsapp/pages/clinicworkspace/useClinicMetricsPageName.jsapp/pages/clinicworkspace/useClinicPatientsFilters.jsapp/pages/clinicworkspace/useIsClinicAdmin.jslocales/en/translation.jsontest/unit/pages/ClinicPatients.test.js
| const DropdownContent = ({ | ||
| onClose, | ||
| onChange, | ||
| timeCGMUsePercent, | ||
| }) => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Define PropTypes for each DropdownContent component. All three new dropdowns declare PropTypes for the exported wrapper but omit them for the inner DropdownContent component. The shared root cause is one missing PropTypes declaration per file. DropdownContent receives required callbacks and value props, so a wrong prop type stays silent.
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L24-L28: add PropTypes foronClose,onChange, andtimeCGMUsePercent.app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js#L20-L26: add PropTypes foronClose,onChange,lastData,lastDataType, andfilterOptions.app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js#L32-L36: add PropTypes foronClose,onChange, andactiveSummaryPeriod.
🛡️ Example for CGMUseFilterDropdown.js
+DropdownContent.propTypes = {
+ onClose: PropTypes.func.isRequired,
+ onChange: PropTypes.func.isRequired,
+ timeCGMUsePercent: PropTypes.string,
+};
+
const CGMUseFilterDropdown = ({As per coding guidelines: "Define PropTypes for all component props."
📍 Affects 3 files
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L24-L28(this comment)app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js#L20-L26app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js#L32-L36
🤖 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/CGMUseFilterDropdown.js` around lines 24
- 28, Define PropTypes for the inner DropdownContent component in
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js lines 24-28 for
required onClose, onChange, and timeCGMUsePercent props;
app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js lines 20-26
for onClose, onChange, lastData, lastDataType, and filterOptions; and
app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js lines 32-36
for onClose, onChange, and activeSummaryPeriod, using the appropriate existing
PropTypes shapes and requiredness.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (7)
app/pages/clinicworkspace/ClinicPatients.js (2)
1054-1056: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the
timeInRangeFilterThresholdslookup against unknown filter keys.Line 1055 destructures
timeInRangeFilterThresholds[filter]with no existence check.activeFilters.timeInRangeis persisted state, so it can contain a key that is not present intimeInRangeFilterThresholds. The destructure then throws aTypeErrorinside the fetch-options effect and the patient list fails to render.This is reachable today.
test/unit/pages/ClinicPatients.test.jsline 928 seedstimeInRange: ['timeInLowPercent'], which is not one of the six threshold keys (timeInVeryLowPercent,timeInAnyLowPercent,timeInTargetPercent,timeInAnyHighPercent,timeInVeryHighPercent,timeInExtremeHighPercent). Any user whose stored filters predate a range rename hits the same path.🐛 Proposed fix
forEach(activeFilters.timeInRange, filter => { - let { comparator, value } = timeInRangeFilterThresholds[filter]; + const threshold = timeInRangeFilterThresholds[filter]; + if (!threshold) return; // ignore unrecognized persisted range keys + + let { comparator, value } = threshold; value = value / 100;Consider also dropping unrecognized keys from
activeFilters.timeInRangeso the filter count and the applied-filter chips stay consistent with the query.📝 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.forEach(activeFilters.timeInRange, filter => { const threshold = timeInRangeFilterThresholds[filter]; if (!threshold) return; // ignore unrecognized persisted range keys let { comparator, value } = threshold; value = value / 100;🤖 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 1054 - 1056, Guard the timeInRangeFilterThresholds lookup in the activeFilters.timeInRange iteration before destructuring, skipping unknown filter keys so the fetch-options effect continues rendering. Also remove unrecognized keys from activeFilters.timeInRange so filter counts and applied-filter chips match the filters used in the query.
3157-3157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add
patientListSearchTextInputto therenderPeopleTabledependencies.Line 3157 reads
patientListSearchTextInputto derivepatientQueryState, but the dependency array at lines 3207-3220 omits it.renderPeopleTableis memoized, so when the user types or clears the search text without any other listed dependency changing, the memoized callback keeps the previouspatientQueryState.The user-visible result is a stale empty state and stale clear controls. Example: with a filter applied and no search text, the state is
FILTER_ONLYand only Reset All Filters renders. After the user types a search term that matches nothing, the state should becomeFILTER_AND_SEARCHand Clear Search should appear, but the stale value keeps it hidden.The same array also omits
handleClearSearchandhandleResetFilters. Those are plain function declarations that are recreated on every render, so wrap them inuseCallbackbefore adding them.🐛 Proposed fix
}, [ activeFilters, clinic?.fetchedPatientCount, columns, data, defaultPatientFetchOptions.sort, handlePageChange, handleSortChange, loading, patientFetchOptions, + patientListSearchTextInput, setActiveFilters, showSummaryData, tableStyle, ]);As per coding guidelines: "use
useCallbackfor callback props, useuseMemofor expensive computations, and respectreact-hooks/exhaustive-deps."Also applies to: 3207-3220
🤖 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 3157, Update the memoized renderPeopleTable callback dependencies to include patientListSearchTextInput, handleClearSearch, and handleResetFilters. Wrap handleClearSearch and handleResetFilters in useCallback with complete dependencies before adding them, ensuring patientQueryState and clear controls update when search text or filter actions change.Source: Coding guidelines
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js (1)
29-32: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use the established metric event contract.
Dispatch the fetch action first. Call
trackMetricimmediately after dispatch. Use a past-tense event name. Update the exact metric assertions with the selected event name.
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js#L29-L32: movetrackMetricafterdispatchand rename the event.app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.js#L29-L32: movetrackMetricafterdispatchand rename the event.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js#L104-L109: update the expected event name.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js#L106-L111: update the expected event name.Based on learnings: use past-tense event names and call
trackMetricimmediately after dispatching the related action at request initiation.📍 Affects 4 files
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js#L29-L32(this comment)app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.js#L29-L32__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js#L104-L109__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js#L106-L111🤖 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/clinicPatientsFilters/FilterBySites.js` around lines 29 - 32, Update handleClickEditSites and the corresponding tag edit handler so each fetch dispatch occurs before trackMetric, with tracking called immediately afterward and using the selected past-tense event name. Update the metric expectations in app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js:104-109 and app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.js:106-111 to match; apply the handler changes in app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.js:29-32 and FilterByTags.js:29-32.Source: Learnings
app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js (2)
89-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # List every clinic filter metric event name to check naming consistency. rg -no --type=js "'Clinic - [^']+'" app | sort -uRepository: tidepool-org/blip
Length of output: 10078
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- DataRecencyFilterDropdown.js ---' sed -n '1,210p' app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js printf '%s\n' '--- CGMUseFilterDropdown.js ---' sed -n '1,175p' app/pages/clinicworkspace/components/CGMUseFilterDropdown.js printf '%s\n' '--- Exact event-name references outside the components ---' rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \ -e 'Clinic - Last upload' \ -e 'Clinic - Last data' \ -e 'Clinic - CGM Use' \ -e 'Clinic - CGM use' \ app test tests 2>/dev/null || true printf '%s\n' '--- Metric definitions and analytics consumers ---' rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \ -e 'trackMetric' \ -e 'Last upload' \ -e 'Last data' \ -e 'CGM Use' \ -e 'CGM use' \ . | head -300Repository: tidepool-org/blip
Length of output: 44858
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- metricUtils and bootstrap transport ---' fd -i 'metricUtils' app sed -n '1,180p' app/core/metricUtils.js 2>/dev/null || true sed -n '35,90p' app/bootstrap.js printf '%s\n' '--- Neighboring clinic filter metric implementations ---' sed -n '180,320p' app/pages/clinicworkspace/components/SiteFilterDropdown.js sed -n '185,320p' app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.js printf '%s\n' '--- Read-only event grouping ---' python3 - <<'PY' import pathlib import re from collections import defaultdict files = [ pathlib.Path('app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js'), pathlib.Path('app/pages/clinicworkspace/components/CGMUseFilterDropdown.js'), ] pattern = re.compile(r"trackMetric\(\s*'([^']+)'") for path in files: events = pattern.findall(path.read_text()) print(path) for event in events: print(f' {event}') groups = defaultdict(list) for event in events: words = event.split(' filter ') groups[words[0]].append(event) if len(set(events)) != len(events): print(' duplicate event names: yes') print(' unique event names:', len(set(events))) PYRepository: tidepool-org/blip
Length of output: 10497
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- metricUtils and bootstrap transport ---' fd -i 'metricUtils' app sed -n '1,180p' app/core/metricUtils.js 2>/dev/null || true sed -n '35,90p' app/bootstrap.js printf '%s\n' '--- Neighboring clinic filter metric implementations ---' sed -n '180,320p' app/pages/clinicworkspace/components/SiteFilterDropdown.js sed -n '185,320p' app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.js printf '%s\n' '--- Read-only event grouping ---' python3 - <<'PY' import pathlib import re files = [ pathlib.Path('app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js'), pathlib.Path('app/pages/clinicworkspace/components/CGMUseFilterDropdown.js'), ] pattern = re.compile(r"trackMetric\(\s*'([^']+)'") for path in files: events = pattern.findall(path.read_text()) print(path) for event in events: print(f' {event}') print(' unique event names:', len(set(events))) PYRepository: tidepool-org/blip
Length of output: 10497
Use one metric-name family per dropdown.
trackMetricsends these strings as distinct event names, so the inconsistentLast data/Last uploadandCGM Use/CGM usenames split analytics funnels. Define one approved prefix per component and build all open, close, apply, and clear events from it. Preserve or migrate existing dashboard names before renaming them.📍 Affects 2 files
app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js#L89-L89(this comment)app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js#L150-L150app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L117-L117🤖 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/DataRecencyFilterDropdown.js` at line 89, In DataRecencyFilterDropdown.js at lines 89-89 and 150-150, define and reuse one approved metric-name prefix for every open, close, apply, and clear event, replacing the inconsistent Last data/Last upload variants while preserving or migrating existing dashboard names. In CGMUseFilterDropdown.js at line 117, apply the same consistency to all dropdown events by standardizing CGM Use versus CGM use under one approved prefix; update each affected trackMetric call rather than only the cited clear event.
156-164: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the icon label with the visible trigger label.
The trigger displays
Data Recencyat line 163, buticonLabelat line 160 reads"Filter by last upload". Screen readers announce the combined name, so the spoken name does not match the visible name. Theidat line 156 also useslast-data-filter-triggerwhile the child control ids uselast-upload-*. UseData Recencywording in the icon label.♿ Proposed fix
- iconLabel="Filter by last upload" + iconLabel="Filter by data recency"If a test or an existing selector queries
Filter by last upload, update it in the same change.📝 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.id="last-data-filter-trigger" selected={!!lastData} {...bindTrigger(lastDataPopupFilterState)} icon={KeyboardArrowDownRoundedIcon} iconLabel="Filter by data recency" sx={{ fontSize: 0, lineHeight: 1.3 }} > {t('Data Recency')} </Button>🤖 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/DataRecencyFilterDropdown.js` around lines 156 - 164, Update the iconLabel on the last-data-filter-trigger Button to use the same “Data Recency” wording as its visible label, and update any tests or selectors that depend on “Filter by last upload.”app/pages/clinicworkspace/components/SiteFilterDropdown.js (2)
89-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail fd -HI -a 'package.json|.browserslistrc|babel.config.*|.babelrc*' . rg -n --hidden 'browserslist|toSorted|core-js|polyfill' \ -g 'package.json' -g '.browserslistrc' -g 'babel.config.*' -g '.babelrc*' -g '*.js' . ast-grep run \ --lang javascript \ --pattern '$ARRAY.toSorted($$$)' \ app/pages/clinicworkspace/componentsRepository: tidepool-org/blip
Length of output: 50374
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- tracked configuration files ---' git ls-files \ 'package.json' \ '.browserslistrc' \ 'babel.config.*' \ '.babelrc*' \ 'config/**' \ 'public/**' | sed -n '1,120p' printf '%s\n' '--- package scripts, browserslist, and polyfill-related dependencies ---' python3 - <<'PY' import json from pathlib import Path p = Path('package.json') if p.exists(): data = json.loads(p.read_text()) print('scripts:', json.dumps(data.get('scripts', {}), indent=2)) print('browserslist:', json.dumps(data.get('browserslist'), indent=2)) deps = {**data.get('dependencies', {}), **data.get('devDependencies', {})} for name in sorted(deps): if any(term in name.lower() for term in ('babel', 'core-js', 'polyfill', 'browserslist')): print(f'{name}: {deps[name]}') PY printf '%s\n' '--- tracked browser/polyfill references ---' git grep -n -E 'browserslist|toSorted|core-js|polyfill' -- \ 'package.json' '.browserslistrc' 'babel.config.*' '.babelrc*' \ '*.js' '*.jsx' '*.ts' '*.tsx' 'config/**' 'public/**' \ ':!**/node_modules/**' || true printf '%s\n' '--- tracked toSorted call sites ---' git grep -n -E '\\.toSorted\\s*\\(' -- \ '*.js' '*.jsx' '*.ts' '*.tsx' ':!**/node_modules/**' || trueRepository: tidepool-org/blip
Length of output: 4264
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- Babel configuration ---' cat -n babel.config.js printf '%s\n' '--- webpack polyfill entry configuration ---' sed -n '175,225p' webpack.config.js printf '%s\n' '--- package dependency metadata ---' python3 - <<'PY' import json from pathlib import Path for name in [ 'node_modules/@babel/polyfill/package.json', 'node_modules/core-js/package.json', 'node_modules/@babel/preset-env/package.json', ]: p = Path(name) print(f'--- {name} ---') if p.exists(): data = json.loads(p.read_text()) for key in ('version', 'main', 'dependencies', 'peerDependencies'): if key in data: print(f'{key}: {json.dumps(data[key], indent=2)}') else: print('not present') PY printf '%s\n' '--- polyfill imports and direct core-js usage ---' git grep -n -E \"`@babel/polyfill`|core-js/(stable|features)|core-js/modules|regenerator-runtime\" -- \ ':!**/node_modules/**' || true printf '%s\n' '--- tracked toSorted call sites ---' git grep -n -F '.toSorted(' -- \ '*.js' '*.jsx' '*.ts' '*.tsx' ':!**/node_modules/**' || trueRepository: tidepool-org/blip
Length of output: 7336
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- `@babel/polyfill` implementation ---' cat -n node_modules/@babel/polyfill/lib/index.js 2>/dev/null || true cat -n node_modules/@babel/polyfill/lib/noConflict.js 2>/dev/null || true printf '%s\n' '--- installed core-js package locations and versions ---' find node_modules -path '*/core-js/package.json' -print \ | while read -r p; do printf '%s: ' "$p" python3 -c 'import json,sys; print(json.load(open(sys.argv[1])).get("version"))' "$p" done | sort printf '%s\n' '--- toSorted polyfill availability in installed core-js packages ---' find node_modules -path '*/core-js/features/array/to-sorted.js' -o \ -path '*/core-js/modules/es.array.to-sorted.js' \ -print printf '%s\n' '--- application polyfill and Babel target references ---' git grep -n -E '(`@babel/polyfill`|core-js/(stable|features)|useBuiltIns|targets|browserslist|toSorted)' -- \ package.json babel.config.js webpack.config.js app jest.setup.js test '*.js' '*.jsx' \ ':!**/node_modules/**' || trueRepository: tidepool-org/blip
Length of output: 4040
Replace
toSortedwith a supported sorting implementation.
@babel/polyfillusescore-js2.6.12 and does not provideArray.prototype.toSorted. Babel does not transform this method. In browsers withouttoSorted, either dropdown fails during option computation. Use.sort()on the newly mapped array or add atoSortedpolyfill in both dropdowns.📍 Affects 2 files
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L89-L92(this comment)app/pages/clinicworkspace/components/TagFilterDropdown.js#L92-L95🤖 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/SiteFilterDropdown.js` around lines 89 - 92, Replace the unsupported toSorted call in sortedSiteFilterOptions with sort on the newly mapped array, preserving label comparison and avoiding mutation of the source sites. Apply the same change in app/pages/clinicworkspace/components/TagFilterDropdown.js lines 92-95.
260-267: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the trigger accessible labels.
The hard-coded
iconLabelvalues expose English-only names to assistive technology. Pass both labels throught().
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L260-L267: translateFilter by clinic sites.app/pages/clinicworkspace/components/TagFilterDropdown.js#L263-L270: translateFilter by patient tags.As per coding guidelines, “Use
react-i18nextviauseTranslation()orwithTranslation()for translations.”📍 Affects 2 files
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L260-L267(this comment)app/pages/clinicworkspace/components/TagFilterDropdown.js#L263-L270🤖 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/SiteFilterDropdown.js` around lines 260 - 267, Use react-i18next through the existing component translation pattern, then pass the translated strings to the trigger iconLabel props: update SiteFilterDropdown.js lines 260-267 for “Filter by clinic sites” and TagFilterDropdown.js lines 263-270 for “Filter by patient tags”; ensure both components obtain the translation function via useTranslation() or withTranslation().Source: Coding guidelines
1 participant
No description provided.