WEB-4654 - Tags & Sites - #1985
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| import SiteFilterDropdown from '../components/SiteFilterDropdown'; | ||
| import useIsClinicAdmin from '../useIsClinicAdmin'; | ||
| import useClinicMetricsPageName from '../useClinicMetricsPageName'; | ||
|
|
There was a problem hiding this comment.
Another adapter component, mapping the state setters of ClinicPatients to the API of the Dropdown
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/pages/clinicworkspace/useClinicMetricsPageName.js (1)
7-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExact-path matching will silently report
Unknownas these filters get reused.Any route variant (trailing slash, or reuse on
/dashboard/tide) falls through to'Unknown', degrading metric attribution rather than failing loudly. Consider prefix matching and adding the dashboard route as the components spread.♻️ Sketch
- switch (pathname) { - case '/clinic-workspace': - case '/clinic-workspace/patients': - return 'Population Health'; - - default: - return 'Unknown'; - }; + if (pathname.startsWith('/clinic-workspace')) return 'Population Health'; + + return 'Unknown';🤖 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/useClinicMetricsPageName.js` around lines 7 - 14, Update the pathname handling in the clinic metrics page-name switch to use prefix matching so trailing slashes and nested clinic-workspace routes resolve to the existing “Population Health” label. Add the `/dashboard/tide` route prefix to the same mapping, while preserving “Unknown” for unrelated paths.app/pages/clinicworkspace/components/SiteFilterDropdown.js (1)
75-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThe two dropdowns are a copy/paste pair and are already drifting. Search filtering, pending selection, the zero-state checkbox, clear/apply + metrics, and the empty state are implemented twice with only the entity noun differing; the drift is already visible (the site-specific
classNameonTagFilterDropdown.jsline 179, and the redundantmarginBottomthat exists only in the site version). Extracting one parameterized dropdown (entity label, options selector, special zero-filter value, metric names, empty-state node) would keep future behavior changes in sync.
app/pages/clinicworkspace/components/SiteFilterDropdown.js#L75-L227: extractDropdownContentinto a shared component and have this file supply only site-specific props (label,clinic?.sites,ZERO_SITES, site metric names,NoClinicSites).app/pages/clinicworkspace/components/TagFilterDropdown.js#L78-L230: replace this duplicatedDropdownContentwith the shared component, supplying tag-specific props (clinic?.patientTags,ZERO_TAGS, tag metric names,NoClinicTags).Non-blocking for this PR — reasonable to land the behavior first and follow up.
🤖 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 75 - 227, Extract the duplicated DropdownContent logic into a shared parameterized component covering search, pending selection, zero-state handling, clear/apply actions, metrics, and empty states. Update app/pages/clinicworkspace/components/SiteFilterDropdown.js lines 75-227 to pass site-specific label, clinic?.sites, ZERO_SITES, site metric names, and NoClinicSites props; update app/pages/clinicworkspace/components/TagFilterDropdown.js lines 78-230 to use the shared component with clinic?.patientTags, ZERO_TAGS, tag metric names, and NoClinicTags. Remove the duplicated implementations while preserving existing behavior.app/core/metricUtils.js (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
useLocationimport fromapp/core/metricUtils.js.The module does not call it, and keeping a router hook import in a standalone util unnecessarily ties
trackMetricconsumers toreact-router-dom.🤖 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/core/metricUtils.js` at line 2, Remove the unused useLocation import from metricUtils.js, leaving trackMetric and the rest of the standalone utility unchanged.
🤖 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/components/SiteFilterDropdown.test.js`:
- Line 98: Update the implementation’s metric names for the clinic site filter
so the apply and clear actions use the same singular/plural wording, then adjust
the expectation around mockTrackMetric if needed to match the unified name.
In `@app/pages/clinicworkspace/components/TagFilterDropdown.js`:
- Line 179: Update the zero-tags row in the tag filter dropdown to use the same
tag-option className and corresponding key convention as the surrounding tag
rows, replacing the copied clinic-site-specific identifiers on the Box element.
Preserve the row’s layout and zero-tags behavior.
---
Nitpick comments:
In `@app/core/metricUtils.js`:
- Line 2: Remove the unused useLocation import from metricUtils.js, leaving
trackMetric and the rest of the standalone utility unchanged.
In `@app/pages/clinicworkspace/components/SiteFilterDropdown.js`:
- Around line 75-227: Extract the duplicated DropdownContent logic into a shared
parameterized component covering search, pending selection, zero-state handling,
clear/apply actions, metrics, and empty states. Update
app/pages/clinicworkspace/components/SiteFilterDropdown.js lines 75-227 to pass
site-specific label, clinic?.sites, ZERO_SITES, site metric names, and
NoClinicSites props; update
app/pages/clinicworkspace/components/TagFilterDropdown.js lines 78-230 to use
the shared component with clinic?.patientTags, ZERO_TAGS, tag metric names, and
NoClinicTags. Remove the duplicated implementations while preserving existing
behavior.
In `@app/pages/clinicworkspace/useClinicMetricsPageName.js`:
- Around line 7-14: Update the pathname handling in the clinic metrics page-name
switch to use prefix matching so trailing slashes and nested clinic-workspace
routes resolve to the existing “Population Health” label. Add the
`/dashboard/tide` route prefix to the same mapping, while preserving “Unknown”
for unrelated paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 506fab81-2b7c-4605-85e1-4c0c50e518ee
⛔ Files ignored due to path filters (1)
app/core/icons/tagIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
__tests__/unit/app/pages/clinicworkspace/ClinicPatients.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/SiteFilterDropdown.test.js__tests__/unit/app/pages/clinicworkspace/components/TagFilterDropdown.test.jsapp/core/metricUtils.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterBySites.jsapp/pages/clinicworkspace/clinicPatientsFilters/FilterByTags.jsapp/pages/clinicworkspace/components/SiteFilterDropdown.jsapp/pages/clinicworkspace/components/TagFilterDropdown.jsapp/pages/clinicworkspace/useClinicMetricsPageName.jsapp/pages/clinicworkspace/useIsClinicAdmin.js
clintonium-119
left a comment
clintonium-119
left a comment
There was a problem hiding this comment.
Great job splitting this all apart. No major issues - just a bunch of small changes requested in the comments
| @@ -1,3 +1,4 @@ | |||
| import appContext from '../bootstrap'; | |||
| import { useLocation } from 'react-router-dom'; | |||
There was a problem hiding this comment.
| import { useLocation } from 'react-router-dom'; |
Suggest removing this unused import.
| switch (pathname) { | ||
| case '/clinic-workspace': | ||
| case '/clinic-workspace/patients': | ||
| return 'Population Health'; | ||
|
|
||
| default: | ||
| return 'Unknown'; | ||
| }; |
There was a problem hiding this comment.
| switch (pathname) { | |
| case '/clinic-workspace': | |
| case '/clinic-workspace/patients': | |
| return 'Population Health'; | |
| default: | |
| return 'Unknown'; | |
| }; | |
| if (pathname.startsWith('/clinic-workspace')) return 'Population Health'; | |
| return 'Unknown'; |
Consider prefix matching to avoid any variants, like trailing slashes falling through to the 'Unknown' case.
There was a problem hiding this comment.
updated to match your suggested implementation: 909ff85
| import get from 'lodash/get'; | ||
| import includes from 'lodash/includes'; | ||
|
|
||
| const useIsClinicAdmin = () => { |
There was a problem hiding this comment.
Nice reuseable hook :)
There is an isClinicAdmin in clinicadmin.js that may be able to leverage this as well.
There was a problem hiding this comment.
Oops, that was the intention - to replace all access points. I missed this spot. 909ff85
The logic to calculate isClinicAdmin diverges between the two implementations. I double checked with claude which seems to think that they arrive at the same value. So I think we're good.
| /> | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Suggest adding propTypes for exported components.
| /> | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Suggest adding propTypes for exported components.
|
|
||
| describe('SiteFilterDropdown', () => { | ||
| let store; | ||
| let wrapper; |
There was a problem hiding this comment.
| let wrapper; |
Remove unused import.
| const setActiveFilters = jest.fn(); | ||
| const setShowClinicPatientTagsDialog = jest.fn(); |
There was a problem hiding this comment.
| const setActiveFilters = jest.fn(); | |
| const setShowClinicPatientTagsDialog = jest.fn(); |
Remove unused imports.
| import { thunk } from 'redux-thunk'; | ||
| import { MemoryRouter } from 'react-router-dom'; | ||
|
|
||
| import * as actions from '@app/redux/actions'; |
There was a problem hiding this comment.
| import * as actions from '@app/redux/actions'; |
Remove unused imports.
|
|
||
| describe('TagFilterDropdown', () => { | ||
| let store; | ||
| let wrapper; |
There was a problem hiding this comment.
| let wrapper; |
Remove unused imports.
| const setActiveFilters = jest.fn(); | ||
| const setShowClinicPatientTagsDialog = jest.fn(); |
There was a problem hiding this comment.
| const setActiveFilters = jest.fn(); | |
| const setShowClinicPatientTagsDialog = jest.fn(); |
Remove unused imports.
2 participants
WEB-4654