Feature - Filter Enhancements - #1984
Conversation
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughClinic patient filtering now uses shared controls for site, tag, data recency, summary period, time in range, and CGM use. The change adds applied-filter chips, query-state handling, clinic-admin detection, localized count messages, and focused tests. ChangesClinic patient filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The filter enhancements introduce only localized cleanup items around imports, metric naming, and prop validation; no actionable merge-blocking risk remains after normal review and checks. Sequence Diagram(s)sequenceDiagram
participant ClinicPatients
participant FilterControls
participant AppliedFiltersList
participant ActiveFiltersTray
participant PatientAPI
ClinicPatients->>FilterControls: render shared filter controls
FilterControls-->>ClinicPatients: update active filter values
ClinicPatients->>AppliedFiltersList: derive query state and applied filters
AppliedFiltersList->>ActiveFiltersTray: render removable chips and counts
ActiveFiltersTray-->>AppliedFiltersList: request filter removal
AppliedFiltersList-->>ClinicPatients: update active filters
ClinicPatients->>PatientAPI: request patients with shared query state
Possibly related PRs
Suggested reviewers: 🚥 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/FilterByCGMUse.test.jsParsing error: [BABEL] /tests/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js: Using __tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.jsParsing error: [BABEL] /tests/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js: Using
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 |
WEB-4654 - Tags & Sites
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
app/pages/clinicworkspace/ClinicPatients.js (1)
215-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd PropTypes for
EmptyContentNode.The component is new in this change and takes
patientQueryStateandchildren. Declare PropTypes for both.♻️ Proposed addition after the component
+EmptyContentNode.propTypes = { + patientQueryState: PropTypes.oneOf(Object.values(PATIENT_QUERY_STATE)), + children: PropTypes.node, +};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/ClinicPatients.js` around lines 215 - 217, Add PropTypes declarations for the patientQueryState and children props accepted by EmptyContentNode, using the appropriate existing PropTypes import and matching the component’s expected value types.Source: Coding guidelines
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js (1)
388-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the two tests to the
should ... when ...form.The surrounding tests in this file use that form, for example "should allow creating a new site for a workspace".
♻️ Proposed renames
- it('maps an applied tag filter into the getPatientsForClinic query', async () => { + it('should map selected tags into the getPatientsForClinic query when the tag filter is applied', async () => {- it('maps an applied site filter into the getPatientsForClinic query', async () => { + it('should map selected sites into the getPatientsForClinic query when the site filter is applied', async () => {As per coding guidelines: "use descriptive names like
should do X when Y".Also applies to: 408-408
🤖 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 two tests around the applied tag filter and the test at the referenced nearby location to follow the “should ... when ...” convention, using descriptive behavior and condition wording consistent with the surrounding tests in the file.Source: Coding guidelines
__tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js (1)
95-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest zero-assignment chips.
Add cases for
SPECIAL_FILTER_STATES.ZERO_TAGSandSPECIAL_FILTER_STATES.ZERO_SITES. Verify the tray rendersNo tagsandNo clinic sites. Verify each remove action passes its filter key and'_'value.Based on learnings from the supplied filter-state contract, zero-assignment sentinels are supported filter values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js` around lines 95 - 123, Add tests in the ActiveFiltersTray tag and site chip coverage for SPECIAL_FILTER_STATES.ZERO_TAGS and SPECIAL_FILTER_STATES.ZERO_SITES, asserting the tray displays “No tags” and “No clinic sites” respectively. For each zero-assignment chip, click its remove control and verify onRemoveFilter receives the corresponding filter key (patientTags or clinicSites) and '_' as the value.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js (1)
73-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required Jest test conventions.
Rename each test to the
should do X when Yformat. Clear mocks inbeforeEachorafterEach.
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js#L73-L162: Rename the test cases and add lifecycle mock cleanup.__tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js#L59-L125: Rename the test cases and add lifecycle mock cleanup.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/clinicPatientsFilters/AppliedFiltersList.test.js` around lines 73 - 162, In __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js lines 73-162 and __tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js lines 59-125, rename every test case to the “should do X when Y” convention and add beforeEach or afterEach mock cleanup for the Jest mocks used by the tests. Preserve the existing userEvent interactions and assertions; no direct production-code changes are needed.Source: Coding guidelines
app/pages/clinicworkspace/components/ActiveFiltersTray.js (1)
1-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace imports in the required groups.
These files place
theme-uior local imports before imports that must precede them. Keep blank lines between groups.
app/pages/clinicworkspace/components/ActiveFiltersTray.js#L1-L19: Move all third-party and Lodash imports beforetheme-ui. MoveTagIconwith the local imports.app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.js#L1-L9: PlacePropTypesbefore Redux. MoveBoxafter Lodash and before local imports.app/pages/clinicworkspace/components/ClearFilterButtons.js#L1-L6: MoveBoxafter the third-party imports.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/ActiveFiltersTray.js` around lines 1 - 19, Reorder imports into the required grouped sequence with blank lines: React, PropTypes, Redux, third-party libraries, Lodash, theme-ui, and local imports. In app/pages/clinicworkspace/components/ActiveFiltersTray.js lines 1-19, place third-party and Lodash imports before theme-ui and move TagIcon into local imports; in app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.js lines 1-9, place PropTypes before Redux and Box after Lodash before local imports; in app/pages/clinicworkspace/components/ClearFilterButtons.js lines 1-6, move Box after third-party imports.Source: Coding guidelines
app/pages/clinicworkspace/components/SiteFilterDropdown.js (3)
12-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder the import groups.
The lodash group appears after the local
utilsandmetricUtilsimports, and@emotion/styledappears after the local component imports. The required order is React, PropTypes, Redux, third-party libraries, Lodash specific imports, theme-ui, then local imports.TagFilterDropdown.jslines 12-35 use the same ordering, so apply the change there too.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/SiteFilterDropdown.js` around lines 12 - 35, Reorder the imports in SiteFilterDropdown.js and the corresponding TagFilterDropdown.js file into the required groups: React, PropTypes, Redux, third-party libraries, Lodash imports, theme-ui, and local imports, with blank lines between groups. Keep all existing imports unchanged while moving the lodash and `@emotion/styled` imports ahead of the local modules.Source: Coding guidelines
248-248: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBoth outer dropdown components subscribe to
clinicwithout using it. TheDropdownContentchild already selectsclinicwhere it is needed. The outer subscription only adds re-renders of the trigger button on unrelated clinic state changes.
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L248-L248: delete theclinicselector fromSiteFilterDropdown.app/pages/clinicworkspace/components/TagFilterDropdown.js#L251-L251: delete theclinicselector fromTagFilterDropdown.🤖 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` at line 248, Remove the unused clinic selector from the outer SiteFilterDropdown component in app/pages/clinicworkspace/components/SiteFilterDropdown.js:248-248 and from the outer TagFilterDropdown component in app/pages/clinicworkspace/components/TagFilterDropdown.js:251-251; retain the clinic selection inside each DropdownContent child where it is required.
72-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThe two dropdown bodies are copy-paste duplicates. Both
DropdownContentimplementations share the same structure: search input, checkbox list, zero-assignment checkbox, empty-state message, and Clear/Apply buttons. Only the identifiers, copy strings, sentinel, and metric names differ. Every future fix has to be applied twice.
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L72-L225: extract a sharedFilterDropdownContentthat accepts the option list, the zero-assignment sentinel, the id prefix, the copy strings, and the metric names, then render it here with the site configuration.app/pages/clinicworkspace/components/TagFilterDropdown.js#L75-L228: render the same shared component with the tag configuration, and keep the ALL-of semantics wording in the tag copy.🤖 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 72 - 225, Extract the duplicated DropdownContent implementations into a shared FilterDropdownContent component, parameterized by option list, zero-assignment sentinel, ID prefix, copy strings, and metric names. Update app/pages/clinicworkspace/components/SiteFilterDropdown.js lines 72-225 to render it with site configuration, and app/pages/clinicworkspace/components/TagFilterDropdown.js lines 75-228 to render it with tag configuration while preserving the tag ALL-of semantics wording; remove the duplicated dropdown body logic from both files.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js (1)
54-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth filter test files share one scaffold with the same mock-reset gap. Each file sets
useIsClinicAdmin.mockReturnValue(true)once in the describe body, mutates it inside a test, and never clearsmockTrackMetric. Later tests then depend on state left by earlier tests. Both files also carry the same wrong inline comment before the admin mock is set back totrue.
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js#L54-L64: addjest.clearAllMocks()anduseIsClinicAdmin.mockReturnValue(true)tobeforeEach, remove line 37 and the individualmockClear()calls, and change the comment at line 92 to "Visible if admin".__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js#L56-L66: apply the samebeforeEachchange, remove line 39 and the individualmockClear()calls, and change the comment at line 94 to "Visible if admin".As per coding guidelines: "In Jest tests, use
jest.fn()for mocks, clear mocks inbeforeEachorafterEach".🤖 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/FilterBySites.test.js` around lines 54 - 64, Reset shared Jest mock state in the beforeEach scaffolds for __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js#L54-L64 and __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js#L56-L66 by adding jest.clearAllMocks() and restoring useIsClinicAdmin.mockReturnValue(true); remove the describe-level admin setup and individual mockClear calls in both files. Update the relevant inline comments in both files to “Visible if admin”.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
`@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js`:
- Around line 92-93: Update the inline comment above
useIsClinicAdmin.mockReturnValue(true) to accurately describe the admin=true
case, replacing the incorrect “Visible if not admin” wording without changing
the test behavior.
In
`@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js`:
- Around line 94-95: Update the inline comment above
useIsClinicAdmin.mockReturnValue(true) to accurately describe the behavior when
the user is an admin.
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Line 3756: Add patientListSearchTextInput to the dependency array of the
memoized renderPeopleTable callback, including the related dependency usage
around the EmptyContentNode and ClearFilterButtons branches, so
getPatientQueryState receives current search text and
react-hooks/exhaustive-deps is satisfied.
- Around line 1798-1810: Update the Data Recency and CGM Use Apply handlers in
the ClinicPatients component to merge their pending values into the current
activeFilters instead of passing pendingFilters as a complete replacement.
Preserve unrelated filters, including patientTags and sites, while applying only
the fields controlled by each handler.
In `@app/pages/clinicworkspace/components/ActiveFiltersTray.js`:
- Around line 157-162: Update the remove-filter Icon in ActiveFiltersTray to use
variant="button" and type="button" so it has button semantics and keyboard
activation while preserving onRemove. Add or update a keyboard interaction test
verifying Enter activates removal of the applied filter.
In `@app/pages/clinicworkspace/components/SiteFilterDropdown.js`:
- Around line 89-92: Replace the ES2023 toSorted usage in the
sortedSiteFilterOptions logic of SiteFilterDropdown.js and the corresponding
sorted tag options logic in TagFilterDropdown.js (lines 89-92 and 92-95) with a
non-mutating sort approach. Preserve the existing label comparison and avoid
mutating the mapped option arrays or relying on unsupported polyfills.
---
Nitpick comments:
In `@__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js`:
- Line 388: Rename the two tests around the applied tag filter and the test at
the referenced nearby location to follow the “should ... when ...” convention,
using descriptive behavior and condition wording consistent with the surrounding
tests in the file.
In
`@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js`:
- Around line 73-162: In
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js
lines 73-162 and
__tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js
lines 59-125, rename every test case to the “should do X when Y” convention and
add beforeEach or afterEach mock cleanup for the Jest mocks used by the tests.
Preserve the existing userEvent interactions and assertions; no direct
production-code changes are needed.
In
`@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js`:
- Around line 54-64: Reset shared Jest mock state in the beforeEach scaffolds
for
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js#L54-L64
and
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js#L56-L66
by adding jest.clearAllMocks() and restoring
useIsClinicAdmin.mockReturnValue(true); remove the describe-level admin setup
and individual mockClear calls in both files. Update the relevant inline
comments in both files to “Visible if admin”.
In
`@__tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js`:
- Around line 95-123: Add tests in the ActiveFiltersTray tag and site chip
coverage for SPECIAL_FILTER_STATES.ZERO_TAGS and
SPECIAL_FILTER_STATES.ZERO_SITES, asserting the tray displays “No tags” and “No
clinic sites” respectively. For each zero-assignment chip, click its remove
control and verify onRemoveFilter receives the corresponding filter key
(patientTags or clinicSites) and '_' as the value.
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 215-217: Add PropTypes declarations for the patientQueryState and
children props accepted by EmptyContentNode, using the appropriate existing
PropTypes import and matching the component’s expected value types.
In `@app/pages/clinicworkspace/components/ActiveFiltersTray.js`:
- Around line 1-19: Reorder imports into the required grouped sequence with
blank lines: React, PropTypes, Redux, third-party libraries, Lodash, theme-ui,
and local imports. In app/pages/clinicworkspace/components/ActiveFiltersTray.js
lines 1-19, place third-party and Lodash imports before theme-ui and move
TagIcon into local imports; in
app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.js lines 1-9,
place PropTypes before Redux and Box after Lodash before local imports; in
app/pages/clinicworkspace/components/ClearFilterButtons.js lines 1-6, move Box
after third-party imports.
In `@app/pages/clinicworkspace/components/SiteFilterDropdown.js`:
- Around line 12-35: Reorder the imports in SiteFilterDropdown.js and the
corresponding TagFilterDropdown.js file into the required groups: React,
PropTypes, Redux, third-party libraries, Lodash imports, theme-ui, and local
imports, with blank lines between groups. Keep all existing imports unchanged
while moving the lodash and `@emotion/styled` imports ahead of the local modules.
- Line 248: Remove the unused clinic selector from the outer SiteFilterDropdown
component in app/pages/clinicworkspace/components/SiteFilterDropdown.js:248-248
and from the outer TagFilterDropdown component in
app/pages/clinicworkspace/components/TagFilterDropdown.js:251-251; retain the
clinic selection inside each DropdownContent child where it is required.
- Around line 72-225: Extract the duplicated DropdownContent implementations
into a shared FilterDropdownContent component, parameterized by option list,
zero-assignment sentinel, ID prefix, copy strings, and metric names. Update
app/pages/clinicworkspace/components/SiteFilterDropdown.js lines 72-225 to
render it with site configuration, and
app/pages/clinicworkspace/components/TagFilterDropdown.js lines 75-228 to render
it with tag configuration while preserving the tag ALL-of semantics wording;
remove the duplicated dropdown body logic from both files.
🪄 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: 308ccb4b-9229-4198-86da-cb97ff284c67
⛔ Files ignored due to path filters (1)
app/core/icons/tagIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.test.js__tests__/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js__tests__/unit/app/pages/clinicworkspace/components/SiteFilterDropdown.test.js__tests__/unit/app/pages/clinicworkspace/components/TagFilterDropdown.test.jsapp/pages/clinicadmin/clinicadmin.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.jsapp/pages/clinicworkspace/components/ActiveFiltersTray.jsapp/pages/clinicworkspace/components/ClearFilterButtons.jsapp/pages/clinicworkspace/components/SiteFilterDropdown.jsapp/pages/clinicworkspace/components/TagFilterDropdown.jsapp/pages/clinicworkspace/useClinicMetricsPageName.jsapp/pages/clinicworkspace/useClinicPatientsFilters.jsapp/pages/clinicworkspace/useIsClinicAdmin.jslocales/en/translation.jsontest/unit/pages/ClinicPatients.test.js
💤 Files with no reviewable changes (1)
- test/unit/pages/ClinicPatients.test.js
| // Visible if not admin | ||
| useIsClinicAdmin.mockReturnValue(true); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the inline comment.
The comment says "Visible if not admin". The code sets the admin mock to true.
📝 Proposed wording
- // Visible if not admin
+ // Visible if admin
useIsClinicAdmin.mockReturnValue(true);📝 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.
| // Visible if not admin | |
| useIsClinicAdmin.mockReturnValue(true); | |
| // Visible if admin | |
| useIsClinicAdmin.mockReturnValue(true); |
🤖 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/FilterBySites.test.js`
around lines 92 - 93, Update the inline comment above
useIsClinicAdmin.mockReturnValue(true) to accurately describe the admin=true
case, replacing the incorrect “Visible if not admin” wording without changing
the test behavior.
| // Visible if not admin | ||
| useIsClinicAdmin.mockReturnValue(true); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the inline comment.
The comment says "Visible if not admin". The code sets the admin mock to true.
📝 Proposed wording
- // Visible if not admin
+ // Visible if admin
useIsClinicAdmin.mockReturnValue(true);📝 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.
| // Visible if not admin | |
| useIsClinicAdmin.mockReturnValue(true); | |
| // Visible if admin | |
| useIsClinicAdmin.mockReturnValue(true); |
🤖 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/FilterByTags.test.js`
around lines 94 - 95, Update the inline comment above
useIsClinicAdmin.mockReturnValue(true) to accurately describe the behavior when
the user is an admin.
|
|
||
| // Show the Filter Reset Bar only if data exists and any filters/search are applied | ||
| const showFilterResetBar = (data?.length > 0) && patientListQueryState !== PATIENT_LIST_QUERY_STATE.NONE; | ||
| const patientQueryState = getPatientQueryState(activeFilters, patientListSearchTextInput); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add patientListSearchTextInput to the renderPeopleTable dependency array.
Line 3756 reads patientListSearchTextInput inside the memoized callback, but the dependency array does not list it. When only the search text changes, the callback keeps a stale patientQueryState, so EmptyContentNode and ClearFilterButtons can render the wrong variant. Other dependencies such as loading and patientFetchOptions currently mask this by chance.
♻️ Proposed dependency fix
loading,
patientFetchOptions,
+ patientListSearchTextInput,
setActiveFilters,As per coding guidelines: "respect react-hooks/exhaustive-deps".
Also applies to: 3806-3819
🤖 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 3756, Add
patientListSearchTextInput to the dependency array of the memoized
renderPeopleTable callback, including the related dependency usage around the
EmptyContentNode and ClearFilterButtons branches, so getPatientQueryState
receives current search text and react-hooks/exhaustive-deps is satisfied.
Source: Coding guidelines
There was a problem hiding this comment.
@henry-tp, noted. The planned removal of pendingFilters does not affect this finding.
renderPeopleTable still reads patientListSearchTextInput, so its dependency array must include patientListSearchTextInput. The finding remains valid.
🐇
You are interacting with an AI system.
| const sortedSiteFilterOptions = useMemo(() => { | ||
| return map(clinic?.sites, ({ id, name }) => ({ id, label: name })) | ||
| .toSorted((a, b) => utils.compareLabels(a.label, b.label)); | ||
| }, [clinic?.sites]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check browserslist targets, Babel/core-js polyfill config, and the Jest/Node version.
set -euo pipefail
fd -H -t f -a '^(package.json|.browserslistrc|babel.config.js|.babelrc|.babelrc.js|.nvmrc|jest.config.js)$' | while IFS= read -r f; do
echo "=== $f ==="
cat -n "$f"
done
# Existing usages and polyfill entry points.
rg -n --type=js '\.toSorted\(|\.toSpliced\(|core-js' -g '!node_modules/**' | head -50Repository: tidepool-org/blip
Length of output: 14416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== browserlist/webpack/runtime config candidates ==="
fd -H -t f -a '^(webpack|webpack\.config|buildapp|buildconfig|babel\.config\.js|.browserslistrc|package.json|.nvmrc)$' | while IFS= read -r f; do
echo "=== $f ==="
cat -n "$f"
done
echo "=== toSorted / core-js usage ==="
rg -n --type=js '\.toSorted\(|\.toSpliced\(|core-js' -g '!node_modules/**' || true
echo "=== relevant dropdowns ==="
for f in app/pages/clinicworkspace/components/SiteFilterDropdown.js app/pages/clinicworkspace/components/TagFilterDropdown.js; do
if [ -f "$f" ]; then
echo "=== $f imports and relevant useMemo ==="
sed -n '1,35p;80,100p' "$f" | cat -n
fi
doneRepository: tidepool-org/blip
Length of output: 10535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== all .browserslistrc/package.json/browserlist config files ==="
fd -H -a '^(\.browserslistrc|package\.json|package-lock\.json|yarn\.lock|browserslistrc\.config)$' . | while IFS= read -r f; do
echo "--- $f ---"
case "$f" in
package.json) sed -n '200,260p' "$f" ;;
*) cat "$f" ;;
esac
done
echo "=== core-js imports/loads in tracked files ==="
rg -n 'import .*core-js|require\(["'\''].*core-js|core-js' --glob '!node_modules/**' --glob '!*.lock' --glob '!package-lock.json' . || true
echo "=== all Array mutations in app pages/components tests (context) ==="
rg -n --type=js '\b(Array|utils)\.|\.sort\(|\.toSorted\(|sortBy\(' app __tests__ -C 2 | head -200Repository: tidepool-org/blip
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== exact core-js imports/loads ==="
rg -n 'import ["\x27]core-js|import {.*} from ["\x27]core-js|require\(["\x27]core-js|core-js/(.*)' --glob '!node_modules/**' --glob '!*.lock' --glob '!package-lock.json' --glob '!yarn.lock' . | head -200 || true
echo "=== core-js dependency entries ==="
python3 - <<'PY'
import json
p = json.load(open('package.json'))
for section in ('dependencies','devDependencies'):
deps = p.get(section, {})
if 'core-js' in deps:
print(section, deps['core-js'])
PY
echo "=== toSorted usages ==="
rg -n --type=js '\.toSorted\s*\(' --glob '!node_modules/**' --glob '!*.lock' . || true
echo "=== package version metadata for babel-preset-react-app core-js target ==="
python3 - <<'PY'
import json, re, zipfile, sys, os
try:
pkg = json.load(open('package.json'))
except Exception as e:
sys.exit(f'package.json: {e}')
core_version = pkg.get('devDependencies', {}).get('core-js') or pkg.get('dependencies', {}).get('core-js')
react_app_version = pkg.get('devDependencies', {}).get('babel-preset-react-app')
print({'core-js': core_version, 'babel-preset-react-app': react_app_version})
PYRepository: tidepool-org/blip
Length of output: 1907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== package.json browserslist fields ==="
python3 - <<'PY'
import json
p=json.load(open('package.json'))
for k in ('scripts','dependencies','devDependencies','peerDependencies','browserslist'):
print(k, 'present' if k in p else 'absent')
if k=='scripts' and isinstance(p.get(k), dict):
for kk,vv in p[k].items():
if 'browserslist' in str(vv):
print(' ', kk, vv)
PY
echo "=== Babel preset-env target/source maps references ==="
rg -n 'useBuiltIns|core-js|targets|sourceType|useBuiltIns|forceBuiltIns|exclude' --glob '!node_modules/**' --glob '!*.lock' --glob '!package-lock.json' --glob '!yarn.lock' | head -200 || true
echo "=== webpack config snippets ==="
for f in $(fd -H -t f 'webpack|buildapp|buildconfig' .); do
echo "=== $f ==="
rg -n --type=js 'browserslist|sourceMap|devtool|core-js|`@babel/preset-env`|useBuiltIns|Array\.prototype\.toSorted|toSorted' "$f" -C 2 || true
doneRepository: tidepool-org/blip
Length of output: 502
🌐 Web query:
Babel 7 preset-env default targets without browserslist includes Array.prototype.toSorted polyfill core-js 3
💡 Result:
In Babel 7, when @babel/preset-env is used without a configured browserslist and without an explicit targets option, it does not default to the browserslist defaults query [1][2][3]. Instead, it effectively assumes no targets are specified, which historically has led Babel to transform code for the oldest possible browsers (or, depending on the specific version and configuration context, no transformation at all) [2][3]. Regarding polyfill inclusion: 1. Automatic Polyfilling is Not Default: @babel/preset-env does not include any core-js polyfills by default [4]. To enable automatic polyfill injection, you must explicitly set the useBuiltIns option (e.g., to "usage" or "entry") and specify the corejs version [5][6][7]. 2. Array.prototype.toSorted: This method is part of the "Change Array by copy" proposal, which is not a stable ECMAScript feature [8]. Because @babel/preset-env only includes polyfills for stable features by default, Array.prototype.toSorted will not be polyfilled automatically even if useBuiltIns is configured [5][6]. 3. Enabling Proposals: To polyfill Array.prototype.toSorted, you must explicitly enable proposal support [5][6]. This is done by configuring the corejs option as an object, such as corejs: { version: "3.48", proposals: true } (adjusting the version to match your installed core-js version) [5][6][7]. Using just "3" as the version is often insufficient as it defaults to "3.0" and will miss newer features [5][9]. If you rely on automatic polyfilling for specific features like toSorted, ensure your Babel configuration explicitly includes the corejs version and enables proposals [5][6]. As noted in community discussions, if the polyfill is still missing, explicitly specifying the precise minor version of core-js (e.g., 3.38 or newer) is often required to ensure the correct modules are targeted [10].
Citations:
- 1: https://babeljs.io/docs/babel-preset-env
- 2: https://stackoverflow.com/questions/71241440/using-defaults-in-browserslistrc-with-babel-preset-env
- 3: @babel/preset-env doesn't read .browserslistrc file (still works using targets option) babel/babel#12721
- 4: babel preset env doesn't do what it claims to do... babel/babel#7619
- 5: https://github.com/babel/website/blob/main/docs/preset-env.md
- 6: https://core-js.io/v4/docs/
- 7: https://core-js.io/docs/usage
- 8: https://core-js.io/v3/docs/features/proposals/change-array-by-copy
- 9: https://github.com/babel/website/blob/master/docs/preset-env.md
- 10: [Bug]: toSorted was not added babel/babel#16745
Do not use Array.prototype.toSorted for stable releases. These option lists call ES2023-only toSorted, and the app does not provide a Babel/core-js target/polyfill path for it. Use a non-mutating sort instead, while the existing core-js dependency remains configured only to the stable features set.
📍 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 ES2023 toSorted usage in the sortedSiteFilterOptions logic of
SiteFilterDropdown.js and the corresponding sorted tag options logic in
TagFilterDropdown.js (lines 89-92 and 92-95) with a non-mutating sort approach.
Preserve the existing label comparison and avoid mutating the mapped option
arrays or relying on unsupported polyfills.
WEB-4654 - Data Recency
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 (2)
app/pages/clinicworkspace/ClinicPatients.js (2)
3682-3695: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
patientListSearchTextInputto therenderPeopleTabledependency array.Line 3632 reads
patientListSearchTextInput, but the dependency array omits it. When only the search text changes, the memoized callback keeps a stalepatientQueryState.EmptyContentNodeandClearFilterButtonsthen render the wrong variant.♻️ Proposed dependency fix
loading, patientFetchOptions, + patientListSearchTextInput, setActiveFilters,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 3682 - 3695, Update the dependency array for the renderPeopleTable callback to include patientListSearchTextInput, which is read within the callback and currently causes stale patientQueryState when search text changes. Preserve the existing dependencies and behavior for all other inputs.Source: Coding guidelines
1941-1949: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThe CGM Use Apply handler still discards filter values owned by the extracted components.
FilterByTags,FilterBySites, and the newFilterByDataRecencywrite only toactiveFilters.pendingFiltersresyncs only in the CGM Use popoveronCloseat Line 1898. Line 1947 callssetActiveFilters(pendingFilters), which replaces the whole filter object. The first CGM Use Apply after a tag, site, or data-recency change therefore restores stalepatientTags,clinicSites,lastData, andlastDataTypevalues.Merge only
timeCGMUsePercentinto the currentactiveFilters.🐛 Proposed fix for the CGM Use Apply handler
- setActiveFilters(pendingFilters); + setActiveFilters({ + ...activeFilters, + timeCGMUsePercent: pendingFilters.timeCGMUsePercent, + }); cgmUsePopupFilterState.close();🤖 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 1941 - 1949, Update the CGM Use Apply handler to merge only pendingFilters.timeCGMUsePercent into the current activeFilters, preserving patientTags, clinicSites, lastData, lastDataType, and other values managed by FilterByTags, FilterBySites, and FilterByDataRecency. Replace the whole-object setActiveFilters(pendingFilters) call in the onClick handler while keeping the metric tracking and popover close behavior unchanged.Source: Learnings
🧹 Nitpick comments (8)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js (1)
443-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe comment does not match the assertion.
The comment states that the test asserts the "14-day span". The assertion only checks that both date bounds are strings. Either correct the comment or assert the span from the captured call arguments.
♻️ Proposed span assertion
- // The from/to date bounds are derived from the current date, so assert their - // presence and 14-day span rather than exact ISO timestamps. + // The from/to date bounds are derived from the current date, so assert the + // 14-day span rather than exact ISO timestamps. expect(defaultProps.api.clinics.getPatientsForClinic).toHaveBeenLastCalledWith( 'clinicID123', expect.objectContaining({ 'cgm.lastDataFrom': expect.any(String), 'cgm.lastDataTo': expect.any(String), limit: 50, offset: 0, period: '14d', sortType: 'cgm', sort: '-lastData', }), expect.any(Function), ); + + const [, appliedQuery] = defaultProps.api.clinics.getPatientsForClinic.mock.calls.at(-1); + expect(moment(appliedQuery['cgm.lastDataTo']).diff(moment(appliedQuery['cgm.lastDataFrom']), 'days')).toBe(14);🤖 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` around lines 443 - 453, Update the assertion around getPatientsForClinic in the ClinicPatients test so it verifies that cgm.lastDataFrom and cgm.lastDataTo from the captured call represent a 14-day span, rather than only checking that both are strings; keep the existing request parameters and callback assertions unchanged.app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js (4)
154-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLocalize the icon label.
Pass
iconLabel={t('Filter by last upload')}. The current hard-coded label is exposed to users outside the translation system.As per coding guidelines, use
react-i18nextfor translations.🤖 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 154 - 163, Update the Button in DataRecencyFilterDropdown to pass the translated value from t to iconLabel instead of the hard-coded “Filter by last upload” string, while preserving the existing trigger and filter behavior.Source: Coding guidelines
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine PropTypes for
DropdownContent.Add PropTypes for
onClose,onChange,lastData,lastDataType, andfilterOptions. The internal component currently accepts unvalidated props.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/DataRecencyFilterDropdown.js` around lines 20 - 26, Define PropTypes for every prop accepted by DropdownContent: onClose, onChange, lastData, lastDataType, and filterOptions. Add the appropriate prop-type declarations after the component, matching each prop’s actual usage and existing project conventions.Source: Coding guidelines
1-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegroup the imports in the required order.
Place Redux imports after React and PropTypes. Place third-party imports next. Place Lodash imports after third-party imports. Place theme-ui imports after Lodash imports. Place all local imports last.
As per coding guidelines, imports must follow the required grouped order.
🤖 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 1 - 18, Regroup the imports in DataRecencyFilterDropdown so React and PropTypes come first, followed by Redux and other third-party imports, then Lodash, theme-ui, and finally all local imports. Preserve every existing import and its usage while applying this ordering consistently.Source: Coding guidelines
88-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse past-tense metric event names.
Rename the clear, apply, open, and close events so each action word is past tense. Update the metric assertions in
__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js.Based on learnings, use past-tense metric event names.
Also applies to: 149-173
🤖 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 88 - 113, Rename the DataRecencyFilterDropdown metric event names for clear, apply, open, and close actions to past-tense wording, updating each corresponding trackMetric call. Revise the metric assertions in the DataRecencyFilterDropdown tests to expect the renamed events, while preserving their existing payloads and behavior.Source: Learnings
app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.js (1)
15-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMemoize callbacks that pass to child components.
app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.js#L15-L17: WraphandleChangewithuseCallback.app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js#L142-L144: WraphandleCloseDropdownwithuseCallback.As per coding guidelines, "use
useCallbackfor 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/FilterByDataRecency.js` around lines 15 - 17, Memoize callback props with useCallback: wrap handleChange in app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.js lines 15-17, preserving activeFilters dependencies, and wrap handleCloseDropdown in app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js lines 142-144 with its required dependencies.Source: Coding guidelines
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js (2)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required test import order.
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js#L1-L9: Move Redux imports before third-party testing-library imports.__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js#L1-L10: Move Redux imports before third-party testing-library imports.As per coding guidelines, imports must follow the required grouped order.
🤖 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/FilterByDataRecency.test.js` around lines 1 - 9, Reorder the imports in __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js (lines 1-9) and __tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js (lines 1-10) so Redux-related imports appear before third-party Testing Library imports, preserving the required grouped import order.Source: Coding guidelines
46-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required Jest test description format.
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js#L46-L73: Rename both tests to useshould do X when Y.__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js#L55-L110: Rename all tests to useshould do X when Y.As per coding guidelines, Jest tests must use descriptive names such as
should do X when Y.🤖 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/FilterByDataRecency.test.js` around lines 46 - 73, Rename both Jest tests in __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js lines 46-73 to descriptive “should do X when Y” names. Rename all tests in __tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js lines 55-110 using the same format; change descriptions only and preserve test behavior.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/clinicPatientsFilters/FilterByDataRecency.js`:
- Around line 19-28: Normalize legacy activeFilters.lastData value 7 before
passing it to DataRecencyFilterDropdown, or retain value 7 in
customLastDataFilterOptions until query-state migration is complete. Ensure the
rendered dropdown never receives a pending value that is absent from its visible
options.
---
Outside diff comments:
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 3682-3695: Update the dependency array for the renderPeopleTable
callback to include patientListSearchTextInput, which is read within the
callback and currently causes stale patientQueryState when search text changes.
Preserve the existing dependencies and behavior for all other inputs.
- Around line 1941-1949: Update the CGM Use Apply handler to merge only
pendingFilters.timeCGMUsePercent into the current activeFilters, preserving
patientTags, clinicSites, lastData, lastDataType, and other values managed by
FilterByTags, FilterBySites, and FilterByDataRecency. Replace the whole-object
setActiveFilters(pendingFilters) call in the onClick handler while keeping the
metric tracking and popover close behavior unchanged.
---
Nitpick comments:
In `@__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js`:
- Around line 443-453: Update the assertion around getPatientsForClinic in the
ClinicPatients test so it verifies that cgm.lastDataFrom and cgm.lastDataTo from
the captured call represent a 14-day span, rather than only checking that both
are strings; keep the existing request parameters and callback assertions
unchanged.
In
`@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js`:
- Around line 1-9: Reorder the imports in
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js
(lines 1-9) and
__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js
(lines 1-10) so Redux-related imports appear before third-party Testing Library
imports, preserving the required grouped import order.
- Around line 46-73: Rename both Jest tests in
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js
lines 46-73 to descriptive “should do X when Y” names. Rename all tests in
__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.js
lines 55-110 using the same format; change descriptions only and preserve test
behavior.
In `@app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.js`:
- Around line 15-17: Memoize callback props with useCallback: wrap handleChange
in app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.js lines
15-17, preserving activeFilters dependencies, and wrap handleCloseDropdown in
app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js lines 142-144
with its required dependencies.
In `@app/pages/clinicworkspace/components/DataRecencyFilterDropdown.js`:
- Around line 154-163: Update the Button in DataRecencyFilterDropdown to pass
the translated value from t to iconLabel instead of the hard-coded “Filter by
last upload” string, while preserving the existing trigger and filter behavior.
- Around line 20-26: Define PropTypes for every prop accepted by
DropdownContent: onClose, onChange, lastData, lastDataType, and filterOptions.
Add the appropriate prop-type declarations after the component, matching each
prop’s actual usage and existing project conventions.
- Around line 1-18: Regroup the imports in DataRecencyFilterDropdown so React
and PropTypes come first, followed by Redux and other third-party imports, then
Lodash, theme-ui, and finally all local imports. Preserve every existing import
and its usage while applying this ordering consistently.
- Around line 88-113: Rename the DataRecencyFilterDropdown metric event names
for clear, apply, open, and close actions to past-tense wording, updating each
corresponding trackMetric call. Revise the metric assertions in the
DataRecencyFilterDropdown tests to expect the renamed events, while preserving
their existing payloads and 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: 85127441-529e-4eee-af6a-004e81cfd6ae
📒 Files selected for processing (7)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.test.js__tests__/unit/app/pages/clinicworkspace/components/DataRecencyFilterDropdown.test.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByDataRecency.jsapp/pages/clinicworkspace/components/DataRecencyFilterDropdown.jstest/unit/pages/ClinicPatients.test.js
💤 Files with no reviewable changes (1)
- test/unit/pages/ClinicPatients.test.js
| const { lastData, lastDataType } = activeFilters; | ||
|
|
||
| const customLastDataFilterOptions = reject(lastDataFilterOptions, { value: 7 }); | ||
|
|
||
| return ( | ||
| <DataRecencyFilterDropdown | ||
| onChange={handleChange} | ||
| lastData={lastData} | ||
| lastDataType={lastDataType} | ||
| filterOptions={customLastDataFilterOptions} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'lastData|lastDataType|lastDataFilterOptions' \
app/pages/clinicworkspace \
app/core/clinicUtils.jsRepository: tidepool-org/blip
Length of output: 39675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files mentioning filterState / defaultFilterState / patientListFilters:\n'
rg -n -C 3 'filterState|defaultFilterState|patientListFilters|setPatientListFilters|useClinicPatientsFilters|getPatientQueryState|save.*filter|query.*filter|url.*filter|lastDataTo|lastDataFrom|summary\.lastData|data recency' app \
--glob '!**/*.test.*' --glob '!**/*.spec.*' | head -n 300
printf '\nCandidate file sizes:\n'
for f in app/pages/clinicworkspace/ClinicPatients.js \
app/pages/clinicworkspace/useClinicPatientsFilters.js \
app/pages/clinicworkspace/AppliedFiltersList.js \
app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.js \
app/core/clinicUtils.js; do
if [ -f "$f" ]; then
wc -l "$f"
fi
doneRepository: tidepool-org/blip
Length of output: 26170
Normalize legacy lastData values before rendering.
activeFilters.lastData is restored from localStorage, 7 is still a schema-supported value, and DataRecencyFilterDropdown renders the dropdown with that pending value. If 7 is removed from the visible options, the trigger can stay selected while no radio is selected. Normalize legacy 7 values on restore or keep 7 in the options until query-state migration removes it.
🤖 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`
around lines 19 - 28, Normalize legacy activeFilters.lastData value 7 before
passing it to DataRecencyFilterDropdown, or retain value 7 in
customLastDataFilterOptions until query-state migration is complete. Ensure the
rendered dropdown never receives a pending value that is absent from its visible
options.
WEB-4654 - Summary Period
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)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js (1)
462-469: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the 14-day window, not only string types.
The comment at Line 462 says the test checks a 14-day span. Lines 464-469 only require both values to be strings. An invalid or incorrectly sized range can pass.
Parse both values, verify valid timestamps, and assert the interval required by the
FilterByDataRecencycontract.Proposed assertion
expect(defaultProps.api.clinics.getPatientsForClinic).toHaveBeenLastCalledWith( 'clinicID123', expect.objectContaining({ 'cgm.lastDataFrom': expect.any(String), 'cgm.lastDataTo': expect.any(String), limit: 50, offset: 0, period: '14d', sortType: 'cgm', sort: '-lastData', }), expect.any(Function), ); + const lastCall = defaultProps.api.clinics.getPatientsForClinic.mock.calls[ + defaultProps.api.clinics.getPatientsForClinic.mock.calls.length - 1 + ]; + const query = lastCall[1]; + const from = moment(query['cgm.lastDataFrom']); + const to = moment(query['cgm.lastDataTo']); + + expect(from.isValid()).toBe(true); + expect(to.isValid()).toBe(true); + expect(to.diff(from, 'days')).toBe(14);🤖 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` around lines 462 - 469, Update the `ClinicPatients` test assertion for `cgm.lastDataFrom` and `cgm.lastDataTo` to parse both values as timestamps, verify they are valid dates, and assert their interval matches the 14-day window required by `FilterByDataRecency`; retain the existing request parameter assertions.
🧹 Nitpick comments (5)
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMemoize callbacks that are passed to child components.
Both components create callback props on every render. Use
useCallbackwith complete dependency arrays.
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js#L12-L18: memoizehandleChangebefore passing it toSummaryPeriodFilterDropdown.app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js#L117-L155: memoizehandleCloseDropdownbefore passing it toPopoverandDropdownContent.As per coding guidelines, “use
useCallbackfor 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 - 18, Memoize the callback props with useCallback and complete dependency arrays: update handleChange in app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js lines 12-18 before passing it to SummaryPeriodFilterDropdown, and update handleCloseDropdown in app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js lines 117-155 before passing it to Popover and DropdownContent.Source: Coding guidelines
__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js (1)
48-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse condition-based test descriptions.
The test descriptions state actions but omit the condition. Rename them to the
should do X when Yform.
__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js#L48-L111: rename the apply, disabled-state, and cancel tests with explicit conditions.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js#L46-L76: rename the callback and active-period tests with explicit conditions.As per coding guidelines, “use descriptive names like
should do X when Y.”🤖 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 48 - 111, Rename the three tests in __tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js (lines 48-111) and the callback and active-period tests in __tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js (lines 46-76) to descriptive “should do X when Y” names that state the relevant condition, while leaving test behavior unchanged.Source: Coding guidelines
app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js (3)
72-147: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse past-tense metric event names.
The new event names use present-tense verbs. Rename them consistently and update the metric assertions.
app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js#L72-L147: renamecancel,apply,open, andcloseevent names to past-tense equivalents.__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js#L60-L104: update expected metric names to match the renamed events.Based on learnings, use past-tense event name strings and retain the existing metric call timing.
🤖 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/SummaryPeriodFilterDropdown.js` around lines 72 - 147, Rename the SummaryPeriodFilterDropdown metric event strings for cancel, apply, open, and close to their past-tense equivalents while preserving the existing metric call timing. Update the expected metric names in __tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js lines 60-104 to match; both affected sites require changes.Source: Learnings
32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd PropTypes for
DropdownContent.
DropdownContentacceptsonClose,onChange, andactiveSummaryPeriodwithout runtime prop validation.Proposed fix
const DropdownContent = ({ onClose, onChange, activeSummaryPeriod, }) => { @@ }; + +DropdownContent.propTypes = { + onClose: PropTypes.func.isRequired, + onChange: PropTypes.func.isRequired, + activeSummaryPeriod: PropTypes.oneOf(summaryPeriodOptions.map(opt => opt.value)).isRequired, +};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/SummaryPeriodFilterDropdown.js` around lines 32 - 36, Define runtime PropTypes for the onClose, onChange, and activeSummaryPeriod props accepted by DropdownContent, using the appropriate existing PropTypes patterns and requiredness for each value.Source: Coding guidelines
1-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder the import groups in the summary-period files.
The imports do not follow the required group order or blank-line separation.
app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js#L1-L19: order React, PropTypes, Redux, third-party, Lodash, theme-ui, then local imports.app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js#L1-L6: separate the React, PropTypes, Lodash, and local groups.__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js#L1-L10: place Redux imports before other third-party test utilities.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js#L1-L9: place Redux imports before other third-party test utilities.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/SummaryPeriodFilterDropdown.js` around lines 1 - 19, Reorder and separate imports into the required groups. In app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js (lines 1-19), order React, PropTypes, Redux, third-party libraries, Lodash, theme-ui, and local imports with blank lines; apply the corresponding React, PropTypes, Lodash, and local grouping in app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js (lines 1-6). In both listed SummaryPeriodFilterDropdown.test.js (lines 1-10) and FilterBySummaryPeriod.test.js (lines 1-9), place Redux imports before other third-party test utilities and preserve blank-line separation.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/components/SummaryPeriodFilterDropdown.js`:
- Line 133: Translate the iconLabel value in SummaryPeriodFilterDropdown using
react-i18next via the component’s useTranslation() or withTranslation()
integration, ensuring the accessible control name is localized instead of
hardcoded English.
---
Outside diff comments:
In `@__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js`:
- Around line 462-469: Update the `ClinicPatients` test assertion for
`cgm.lastDataFrom` and `cgm.lastDataTo` to parse both values as timestamps,
verify they are valid dates, and assert their interval matches the 14-day window
required by `FilterByDataRecency`; retain the existing request parameter
assertions.
---
Nitpick comments:
In
`@__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js`:
- Around line 48-111: Rename the three tests in
__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js
(lines 48-111) and the callback and active-period tests in
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js
(lines 46-76) to descriptive “should do X when Y” names that state the relevant
condition, while leaving test behavior unchanged.
In `@app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js`:
- Around line 12-18: Memoize the callback props with useCallback and complete
dependency arrays: update handleChange in
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js lines
12-18 before passing it to SummaryPeriodFilterDropdown, and update
handleCloseDropdown in
app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js lines
117-155 before passing it to Popover and DropdownContent.
In `@app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js`:
- Around line 72-147: Rename the SummaryPeriodFilterDropdown metric event
strings for cancel, apply, open, and close to their past-tense equivalents while
preserving the existing metric call timing. Update the expected metric names in
__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.js
lines 60-104 to match; both affected sites require changes.
- Around line 32-36: Define runtime PropTypes for the onClose, onChange, and
activeSummaryPeriod props accepted by DropdownContent, using the appropriate
existing PropTypes patterns and requiredness for each value.
- Around line 1-19: Reorder and separate imports into the required groups. In
app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.js (lines
1-19), order React, PropTypes, Redux, third-party libraries, Lodash, theme-ui,
and local imports with blank lines; apply the corresponding React, PropTypes,
Lodash, and local grouping in
app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.js (lines
1-6). In both listed SummaryPeriodFilterDropdown.test.js (lines 1-10) and
FilterBySummaryPeriod.test.js (lines 1-9), place Redux imports before other
third-party test utilities and preserve blank-line separation.
🪄 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: e27f3846-8151-4a1b-8be1-6c88c8d47df6
📒 Files selected for processing (7)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.test.js__tests__/unit/app/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.test.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterBySummaryPeriod.jsapp/pages/clinicworkspace/components/SummaryPeriodFilterDropdown.jstest/unit/pages/ClinicPatients.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/pages/ClinicPatients.test.js
- app/pages/clinicworkspace/ClinicPatients.js
| id="summary-period-filter-trigger" | ||
| {...bindTrigger(summaryPeriodPopupFilterState)} | ||
| icon={KeyboardArrowDownRoundedIcon} | ||
| iconLabel="Filter by summary period duration" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the icon label.
iconLabel is exposed to assistive technology. Pass it through t() so localized users do not receive an English-only control name.
- iconLabel="Filter by summary period duration"
+ iconLabel={t('Filter by summary period duration')}As per coding guidelines, “Use react-i18next via useTranslation() or withTranslation() for translations.”
🤖 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/SummaryPeriodFilterDropdown.js` at line
133, Translate the iconLabel value in SummaryPeriodFilterDropdown using
react-i18next via the component’s useTranslation() or withTranslation()
integration, ensuring the accessible control name is localized instead of
hardcoded English.
Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (7)
app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.js (3)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder
getTimeInRangeFilterOptionsparameters.The signature declares a default for
showExtremeHighbefore the requiredtparameter. The default can never apply, because a caller must passt. Put the required parameter first.♻️ Proposed signature change
-const getTimeInRangeFilterOptions = (showExtremeHigh = false, t) => [ +const getTimeInRangeFilterOptions = (t, showExtremeHigh = false) => [Update the call site on line 109:
- const filterOptions = getTimeInRangeFilterOptions(showExtremeHigh, t); + const filterOptions = getTimeInRangeFilterOptions(t, showExtremeHigh);🤖 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` at line 43, Update getTimeInRangeFilterOptions so the required t parameter comes before optional showExtremeHigh, then adjust its call site accordingly to preserve the existing translation and filter-option behavior.
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
vizUtils.bgdestructuring below the import block.Line 27 places an executable statement between local imports on lines 19-25 and lines 28-29. This splits the local import group. Move the destructuring after all imports.
♻️ Proposed reordering
-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 27 - 29, Move the vizUtils.bg destructuring statement below the complete import block, keeping all React, library, and local imports grouped according to the project’s ordering guidelines.Source: Coding guidelines
83-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd PropTypes for
DropdownContent.
DropdownContentis a React component with three props, but it declares nopropTypes. The coding guidelines require PropTypes for all component props.♻️ Proposed addition after the component definition
+DropdownContent.propTypes = { + onClose: PropTypes.func, + onChange: PropTypes.func, + timeInRange: PropTypes.arrayOf(PropTypes.string), +}; + const TimeInRangeFilterDropdown = ({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/TimeInRangeFilterDropdown.js` around lines 83 - 87, Add PropTypes for the DropdownContent component covering onClose, onChange, and timeInRange, matching their defaults and usage; place the declaration after the component definition and preserve the existing behavior.Source: Coding guidelines
__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js (3)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
constforonChange.
onChangeis never reassigned. The coding guidelines requireconst/letovervarand prefer immutable bindings.♻️ Proposed change
- let onChange = jest.fn(); + const onChange = jest.fn();As per coding guidelines: "Use ES6 features:
const/letinstead ofvar, arrow functions, and destructuring."🤖 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 23, Change the onChange binding in the test to const, since it is never reassigned, while preserving its existing jest.fn() initialization and usage.Source: Coding guidelines
52-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd assertions for the tracked metrics.
The suite imports
mockTrackMetricand clears it, but no test asserts the emitted events. The component emitsClinic - Time in range filter open,Clinic - Time in range clear filter,Clinic - Time in range apply filter, andClinic - Time in range filter close. The apply event also carries per-range booleans. Add assertions for at least the apply and clear events, so the metric payload contract stays covered.🤖 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 52 - 169, Extend the filtering tests around the apply and clear interactions to assert mockTrackMetric receives the corresponding “Clinic - Time in range apply filter” and “Clinic - Time in range clear filter” events. For the apply case, verify the payload includes the expected per-range boolean values for the selected Very High and Very Low options; keep the existing onChange and dropdown assertions intact.
12-12: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse the
@appalias formetricUtilsfor consistency with the neighboring imports. The Jest configuration maps both imports to__mocks__/metricUtils.js, somockTrackMetric.mockClear()is valid.🤖 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, Update the metricUtils import in the test to use the configured `@app` alias, keeping the existing trackMetric alias mockTrackMetric and mockClear usage unchanged.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTimeInRange.test.js (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfigure the
useFlagsmock insidebeforeEach.Line 23 sets the mock return value once, during describe-body evaluation. If Jest config enables
resetMocksorclearMocks, the return value is discarded before each test,useFlags()returnsundefined, and the destructuring inDropdownContentthrows. The sibling testTimeInRangeFilterDropdown.test.jssets this mock insidebeforeEachon line 47. Align both files.♻️ Proposed change
const setActiveFilters = jest.fn(); - useFlags.mockReturnValue({ showExtremeHigh: false }); - const ui = (props = {}) => (beforeEach(() => { store = mockStore({ blip: { selectedClinicId, clinics: { [selectedClinicId]: { id: selectedClinicId } }, }, }); + useFlags.mockReturnValue({ showExtremeHigh: false }); setActiveFilters.mockClear(); });As per coding guidelines: "In Jest tests, use
jest.fn()for mocks, clear mocks inbeforeEachorafterEach".🤖 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/FilterByTimeInRange.test.js` around lines 21 - 23, Move the useFlags.mockReturnValue({ showExtremeHigh: false }) setup into the test suite’s beforeEach, alongside the existing mock reset/clear setup, so every test receives the required return value. Keep the setActiveFilters mock unchanged and align the setup with TimeInRangeFilterDropdown.test.js.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.
Nitpick comments:
In
`@__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByTimeInRange.test.js`:
- Around line 21-23: Move the useFlags.mockReturnValue({ showExtremeHigh: false
}) setup into the test suite’s beforeEach, alongside the existing mock
reset/clear setup, so every test receives the required return value. Keep the
setActiveFilters mock unchanged and align the setup with
TimeInRangeFilterDropdown.test.js.
In
`@__tests__/unit/app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.test.js`:
- Line 23: Change the onChange binding in the test to const, since it is never
reassigned, while preserving its existing jest.fn() initialization and usage.
- Around line 52-169: Extend the filtering tests around the apply and clear
interactions to assert mockTrackMetric receives the corresponding “Clinic - Time
in range apply filter” and “Clinic - Time in range clear filter” events. For the
apply case, verify the payload includes the expected per-range boolean values
for the selected Very High and Very Low options; keep the existing onChange and
dropdown assertions intact.
- Line 12: Update the metricUtils import in the test to use the configured `@app`
alias, keeping the existing trackMetric alias mockTrackMetric and mockClear
usage unchanged.
In `@app/pages/clinicworkspace/components/TimeInRangeFilterDropdown.js`:
- Line 43: Update getTimeInRangeFilterOptions so the required t parameter comes
before optional showExtremeHigh, then adjust its call site accordingly to
preserve the existing translation and filter-option behavior.
- Around line 27-29: Move the vizUtils.bg destructuring statement below the
complete import block, keeping all React, library, and local imports grouped
according to the project’s ordering guidelines.
- Around line 83-87: Add PropTypes for the DropdownContent component covering
onClose, onChange, and timeInRange, matching their defaults and usage; place the
declaration after the component definition and preserve the existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d3a8be5-a020-4d4a-b0c5-f9ac10cacd82
📒 Files selected for processing (12)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.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/TimeInRangeFilterDropdown.test.jsapp/core/clinicUtils.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByTimeInRange.jsapp/pages/clinicworkspace/components/ActiveFiltersTray.jsapp/pages/clinicworkspace/components/ClearFilterButtons.jsapp/pages/clinicworkspace/components/TimeInRangeFilterDropdown.jstest/unit/pages/ClinicPatients.test.js
💤 Files with no reviewable changes (1)
- test/unit/pages/ClinicPatients.test.js
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/app/pages/clinicworkspace/components/ActiveFiltersTray.test.js
- app/pages/clinicworkspace/components/ClearFilterButtons.js
- tests/unit/app/pages/clinicworkspace/ClinicPatients.test.js
- tests/unit/app/pages/clinicworkspace/clinicPatientsFilters/AppliedFiltersList.test.js
- app/pages/clinicworkspace/components/ActiveFiltersTray.js
- app/pages/clinicworkspace/ClinicPatients.js
WEB-4654 CGM Use
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/pages/clinicworkspace/components/CGMUseFilterDropdown.js`:
- Around line 1-18: Reorder imports into the required groups: React, PropTypes,
Redux, third-party libraries, Lodash, theme-ui, and local imports, with blank
lines between groups. In
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js lines 1-18, place
Redux before other third-party imports, Lodash before theme-ui, and local
imports last. In
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js
lines 1-9 and
__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js
lines 1-10, move Provider before Testing Library and other third-party imports;
keep local imports last in the latter.
- Line 65: Rename the metric event strings in CGMUseFilterDropdown.js at lines
65, 80, 117, and 139 from clear/apply/open/close to past-tense forms, and update
the expected apply event assertion in
__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js
lines 66-70 to match.
- Around line 24-28: Add a PropTypes declaration for the DropdownContent
component covering onClose, onChange, and timeCGMUsePercent, placing it before
the component export and matching the expected types used by the component.
🪄 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: 4f10ab31-9498-471e-b7a2-87ba9eda0dbc
📒 Files selected for processing (8)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.test.js__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.jsapp/pages/clinicadmin/clinicadmin.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.jsapp/pages/clinicworkspace/components/CGMUseFilterDropdown.jstest/unit/pages/ClinicPatients.test.js
💤 Files with no reviewable changes (1)
- test/unit/pages/ClinicPatients.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/app/pages/clinicworkspace/ClinicPatients.test.js
- app/pages/clinicadmin/clinicadmin.js
- app/pages/clinicworkspace/ClinicPatients.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 noop from 'lodash/noop'; | ||
|
|
||
| import { bindPopover, bindTrigger, usePopupState } from 'material-ui-popup-state/hooks'; | ||
|
|
||
| import Button from '../../../components/elements/Button'; | ||
| import Popover from '../../../components/elements/Popover'; | ||
| import RadioGroup from '../../../components/elements/RadioGroup'; | ||
| import useClinicMetricsPageName from '../useClinicMetricsPageName'; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reorder imports into the required groups.
These files do not keep Redux imports before other third-party imports, or they place third-party and local imports after later groups.
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L1-L18: place Redux before other third-party imports, Lodash before theme-ui, and all local imports last.__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js#L1-L9: moveProviderbefore Testing Library and other third-party imports.__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js#L1-L10: moveProviderbefore Testing Library and other third-party imports, and keep local imports last.
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.”
📍 Affects 3 files
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L1-L18(this comment)__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js#L1-L9__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js#L1-L10
🤖 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 into the required groups: React, PropTypes, Redux,
third-party libraries, Lodash, theme-ui, and local imports, with blank lines
between groups. In app/pages/clinicworkspace/components/CGMUseFilterDropdown.js
lines 1-18, place Redux before other third-party imports, Lodash before
theme-ui, and local imports last. In
__tests__/unit/app/pages/clinicworkspace/clinicPatientsFilters/FilterByCGMUse.test.js
lines 1-9 and
__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js
lines 1-10, move Provider before Testing Library and other third-party imports;
keep local imports last in the latter.
Source: Coding guidelines
| const DropdownContent = ({ | ||
| onClose, | ||
| onChange, | ||
| timeCGMUsePercent, | ||
| }) => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add PropTypes for DropdownContent.
DropdownContent receives onClose, onChange, and timeCGMUsePercent, but it has no PropTypes declaration. Add the declaration before export.
Proposed fix
+DropdownContent.propTypes = {
+ onClose: PropTypes.func.isRequired,
+ onChange: PropTypes.func.isRequired,
+ timeCGMUsePercent: PropTypes.oneOf(['<0.7', '>=0.7']),
+};
+
const CGMUseFilterDropdown = ({As per coding guidelines, “Define PropTypes for all component props.”
📝 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.
| const DropdownContent = ({ | |
| onClose, | |
| onChange, | |
| timeCGMUsePercent, | |
| }) => { | |
| const DropdownContent = ({ | |
| onClose, | |
| onChange, | |
| timeCGMUsePercent, | |
| }) => { | |
| // existing component body | |
| }; | |
| DropdownContent.propTypes = { | |
| onClose: PropTypes.func.isRequired, | |
| onChange: PropTypes.func.isRequired, | |
| timeCGMUsePercent: PropTypes.oneOf(['<0.7', '>=0.7']), | |
| }; | |
| const CGMUseFilterDropdown = ({ |
🤖 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, Add a PropTypes declaration for the DropdownContent component covering
onClose, onChange, and timeCGMUsePercent, placing it before the component export
and matching the expected types used by the component.
Source: Coding guidelines
| sx={{ fontSize: 1 }} | ||
| variant="secondary" | ||
| onClick={() => { | ||
| trackMetric('Clinic - CGM use clear filter', { clinicId: selectedClinicId, pageName }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use past-tense metric event names.
The new event names use clear, apply, open, and close. Rename them to past-tense forms and update the assertion.
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L65-L65: rename the clear event to a past-tense name.app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L80-L80: rename the apply event to a past-tense name.app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L117-L117: rename the open event to a past-tense name.app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L139-L139: rename the close event to a past-tense name.__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js#L66-L70: update the expected apply event name.
Based on learnings, “use past-tense event name strings.”
📍 Affects 2 files
app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L65-L65(this comment)app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L80-L80app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L117-L117app/pages/clinicworkspace/components/CGMUseFilterDropdown.js#L139-L139__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js#L66-L70
🤖 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` at line 65,
Rename the metric event strings in CGMUseFilterDropdown.js at lines 65, 80, 117,
and 139 from clear/apply/open/close to past-tense forms, and update the expected
apply event assertion in
__tests__/unit/app/pages/clinicworkspace/components/CGMUseFilterDropdown.test.js
lines 66-70 to match.
Source: Learnings
1 participant
No description provided.