WEB-4677 - Improve Pop Health Add/Edit Patient Modal Performance - #1995
WEB-4677 - Improve Pop Health Add/Edit Patient Modal Performance#1995henry-tp wants to merge 13 commits into
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 creation and editing now use dedicated dialog components. Validation state moves into the dialogs and ChangesClinic patient dialogs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClinicPatients
participant AddPatientDialog
participant PatientForm
participant api
ClinicPatients->>AddPatientDialog: Render active add dialog
AddPatientDialog->>PatientForm: Render and validate patient form
ClinicPatients->>AddPatientDialog: Confirm patient creation
AddPatientDialog->>api: Submit patient form
api-->>AddPatientDialog: Return processing state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js (3)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the dialog prop contracts.
Both exported dialogs accept props but have no
propTypes. DeclareapiandonClosefor both dialogs. DeclarepatientforEditPatientDialog. DeclaresearchDebounceMswhen it is forwarded.
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L19-L23: AddEditPatientDialog.propTypes.app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L18-L21: AddAddPatientDialog.propTypes.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/clinicPatientsDialogs/editPatientDialog.js` around lines 19 - 23, Declare propTypes for both EditPatientDialog and AddPatientDialog, covering api and onClose, plus patient on EditPatientDialog and searchDebounceMs wherever it is forwarded. Apply the corresponding changes in app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js at lines 19-23 and app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js at lines 18-21.Source: Coding guidelines
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required import groups and component terminators.
trackMetricis a local import but appears before Lodash imports. Move it into the local-import group after Lodash. Terminate bothconst ...Dialogassignments with;.
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L4-L10: Reorder and separate import groups.app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L101-L101: Change}to};.app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L4-L9: Reorder and separate import groups.app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L81-L81: Change}to};.As per coding guidelines, “Group imports in the required order with blank lines between groups” and “Use single-quoted strings and required semicolons.”
🤖 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/clinicPatientsDialogs/editPatientDialog.js` around lines 4 - 10, Reorder imports in app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js lines 4-10 and app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js lines 4-9 so Lodash imports precede local imports, with required blank-separated groups; terminate the const editPatientDialog assignment at line 101 and const addPatientDialog assignment at line 81 with semicolons.Source: Coding guidelines
40-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMemoize callbacks passed to child components.
Both dialogs create
onFormChange,onClose, and confirmation callback props on every render. Wrap named handlers inuseCallback. Replace the inline edit close and cancel handlers with named memoized handlers. Verify complete dependency arrays withreact-hooks/exhaustive-deps.
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L40-L54: Memoize named form, close, and confirm handlers.app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L63-L66: Replace the inline title-close callback.app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L82-L85: Replace the inline cancel callback.app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L35-L42: Memoize named form, close, and confirm handlers.As per coding guidelines, “use
useCallbackfor callback props” and “respectreact-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/clinicPatientsDialogs/editPatientDialog.js` around lines 40 - 54, Memoize callback props with useCallback and complete exhaustive-deps dependencies: update handleFormChange, handleClose, and handleConfirm in app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L40-L54; replace the inline title-close and cancel callbacks at `#L63-L66` and `#L82-L85` with named memoized handlers; and apply the same treatment to the named form, close, and confirm handlers in app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L35-L42.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 3036-3049: Forward searchDebounceMs through both extracted patient
dialogs so caller-provided values reach PatientForm instead of being replaced by
the fixed default. In app/pages/clinicworkspace/ClinicPatients.js:3036-3049,
pass searchDebounceMs to AddPatientDialog and EditPatientDialog and include it
in both useCallback dependency arrays. In
app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js:18-21 and
:60, accept it with SEARCH_DEBOUNCE_MS as the default and pass it to
PatientForm. Apply the same changes in
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js:19-23 and
:76.
---
Nitpick comments:
In `@app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js`:
- Around line 19-23: Declare propTypes for both EditPatientDialog and
AddPatientDialog, covering api and onClose, plus patient on EditPatientDialog
and searchDebounceMs wherever it is forwarded. Apply the corresponding changes
in app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js at lines
19-23 and app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js at
lines 18-21.
- Around line 4-10: Reorder imports in
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js lines 4-10
and app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js lines
4-9 so Lodash imports precede local imports, with required blank-separated
groups; terminate the const editPatientDialog assignment at line 101 and const
addPatientDialog assignment at line 81 with semicolons.
- Around line 40-54: Memoize callback props with useCallback and complete
exhaustive-deps dependencies: update handleFormChange, handleClose, and
handleConfirm in
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L40-L54;
replace the inline title-close and cancel callbacks at `#L63-L66` and `#L82-L85`
with named memoized handlers; and apply the same treatment to the named form,
close, and confirm handlers in
app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L35-L42.
🪄 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: 24b25fbf-b1d2-485a-bdcc-b4345aa720d3
📒 Files selected for processing (3)
app/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.jsapp/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js
| if (!showAddPatientDialog) return null; | ||
|
|
||
| <DialogContent> | ||
| <PatientForm api={api} trackMetric={trackMetric} onFormChange={handlePatientFormChange} searchDebounceMs={searchDebounceMs} action="create" /> | ||
| </DialogContent> | ||
|
|
||
| <DialogActions> | ||
| <Button id="addPatientCancel" variant="secondary" onClick={handleCloseOverlays}> | ||
| {t('Cancel')} | ||
| </Button> | ||
| <Button | ||
| id="addPatientConfirm" | ||
| variant="primary" | ||
| onClick={handleAddPatientConfirm} | ||
| processing={creatingClinicCustodialAccount.inProgress} | ||
| disabled={!fieldsAreValid(keys(patientFormContext?.values), validationSchema({mrnSettings, existingMRNs}), patientFormContext?.values)} | ||
| > | ||
| {t('Add Patient')} | ||
| </Button> | ||
| </DialogActions> | ||
| </Dialog> | ||
| return ( | ||
| <AddPatientDialog api={api} onClose={handleCloseOverlays} /> | ||
| ); | ||
| }, [ | ||
| api, | ||
| creatingClinicCustodialAccount.inProgress, | ||
| handleAddPatientConfirm, | ||
| mrnSettings, | ||
| existingMRNs, | ||
| patientFormContext?.values, | ||
| showAddPatientDialog, | ||
| t, | ||
| trackMetric | ||
| ]); | ||
| }, [api, showAddPatientDialog, handleCloseOverlays]); | ||
|
|
||
| const renderEditPatientDialog = useCallback(() => { | ||
| return ( | ||
| <Dialog | ||
| id="editPatient" | ||
| aria-labelledby="dialog-title" | ||
| open={showEditPatientDialog} | ||
| onClose={handleCloseOverlays} | ||
| > | ||
| <DialogTitle onClose={() => { | ||
| trackMetric('Clinic - Edit patient close', { clinicId: selectedClinicId }); | ||
| handleCloseOverlays() | ||
| }}> | ||
| <MediumTitle id="dialog-title">{t('Edit Patient Details')}</MediumTitle> | ||
| </DialogTitle> | ||
|
|
||
| <DialogContent> | ||
| <PatientForm | ||
| api={api} | ||
| trackMetric={trackMetric} | ||
| onFormChange={handlePatientFormChange} | ||
| patient={selectedPatient} | ||
| searchDebounceMs={searchDebounceMs} | ||
| action="edit" | ||
| /> | ||
| </DialogContent> | ||
|
|
||
| <DialogActions> | ||
| <Button id="editPatientCancel" variant="secondary" onClick={() => { | ||
| trackMetric('Clinic - Edit patient cancel', { clinicId: selectedClinicId, source: 'Patients list' }); | ||
| handleCloseOverlays() | ||
| }}> | ||
| {t('Cancel')} | ||
| </Button> | ||
| if (!showEditPatientDialog || !selectedPatient) return null; | ||
|
|
||
| <Button | ||
| id="editPatientConfirm" | ||
| variant="primary" | ||
| onClick={handleEditPatientConfirm} | ||
| processing={updatingClinicPatient.inProgress} | ||
| disabled={!fieldsAreValid(keys(patientFormContext?.values), validationSchema({mrnSettings, existingMRNs}), patientFormContext?.values)} | ||
| > | ||
| {t('Save Changes')} | ||
| </Button> | ||
| </DialogActions> | ||
| </Dialog> | ||
| return ( | ||
| <EditPatientDialog api={api} patient={selectedPatient} onClose={handleCloseOverlays} /> | ||
| ); | ||
| }, [ | ||
| api, | ||
| handleEditPatientConfirm, | ||
| mrnSettings, | ||
| existingMRNs, | ||
| handleCloseOverlays, | ||
| patientFormContext, | ||
| searchDebounceMs, | ||
| selectedClinicId, | ||
| selectedPatient, | ||
| showEditPatientDialog, | ||
| t, | ||
| trackMetric, | ||
| updatingClinicPatient.inProgress | ||
| ]); | ||
| }, [api, selectedPatient, handleCloseOverlays]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward searchDebounceMs through the extracted dialogs.
The inline dialogs forwarded searchDebounceMs to PatientForm, and PatientForm uses it to debounce MRN lookups. The extracted dialogs replace that input with fixed 1000, so a caller-provided non-default value has no effect. (raw.githubusercontent.com)
app/pages/clinicworkspace/ClinicPatients.js#L3036-L3049: PasssearchDebounceMsto both dialog components and add it to bothuseCallbackdependency arrays.app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L18-L21: AcceptsearchDebounceMs, withSEARCH_DEBOUNCE_MSas its default.app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L60-L60: Pass the received value toPatientForm.app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L19-L23: AcceptsearchDebounceMs, withSEARCH_DEBOUNCE_MSas its default.app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L76-L76: Pass the received value toPatientForm.
Proposed fix
-<AddPatientDialog api={api} onClose={handleCloseOverlays} />
+<AddPatientDialog
+ api={api}
+ onClose={handleCloseOverlays}
+ searchDebounceMs={searchDebounceMs}
+/>
-<EditPatientDialog api={api} patient={selectedPatient} onClose={handleCloseOverlays} />
+<EditPatientDialog
+ api={api}
+ patient={selectedPatient}
+ onClose={handleCloseOverlays}
+ searchDebounceMs={searchDebounceMs}
+/>
const AddPatientDialog = ({
api,
onClose = noop,
+ searchDebounceMs = SEARCH_DEBOUNCE_MS,
}) => {
- ...
- searchDebounceMs={SEARCH_DEBOUNCE_MS}
+ ...
+ searchDebounceMs={searchDebounceMs}
const EditPatientDialog = ({
api,
patient,
onClose = noop,
+ searchDebounceMs = SEARCH_DEBOUNCE_MS,
}) => {
- ...
- searchDebounceMs={SEARCH_DEBOUNCE_MS}
+ ...
+ searchDebounceMs={searchDebounceMs}📍 Affects 3 files
app/pages/clinicworkspace/ClinicPatients.js#L3036-L3049(this comment)app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L18-L21app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js#L60-L60app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L19-L23app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js#L76-L76
🤖 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 3036 - 3049,
Forward searchDebounceMs through both extracted patient dialogs so
caller-provided values reach PatientForm instead of being replaced by the fixed
default. In app/pages/clinicworkspace/ClinicPatients.js:3036-3049, pass
searchDebounceMs to AddPatientDialog and EditPatientDialog and include it in
both useCallback dependency arrays. In
app/pages/clinicworkspace/clinicPatientsDialogs/addPatientDialog.js:18-21 and
:60, accept it with SEARCH_DEBOUNCE_MS as the default and pass it to
PatientForm. Apply the same changes in
app/pages/clinicworkspace/clinicPatientsDialogs/editPatientDialog.js:19-23 and
:76.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/clinicworkspace/ClinicPatients.js (1)
3043-3049: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
showEditPatientDialogto the dependency array.
renderEditPatientDialogreadsshowEditPatientDialog, but the current dependency list omits it. Add it to avoid a stale condition and to keepreact-hooks/exhaustive-depssatisfied.🤖 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 3043 - 3049, Add showEditPatientDialog to the dependency array of renderEditPatientDialog, preserving the existing callback logic and ensuring its visibility condition updates with current state.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 123-124: Update the AddPatientDialog and EditPatientDialog imports
in ClinicPatients.js to match the tracked lowercase filenames
addPatientDialog.js and editPatientDialog.js, or consistently rename the files
and references so module resolution works on case-sensitive systems.
---
Outside diff comments:
In `@app/pages/clinicworkspace/ClinicPatients.js`:
- Around line 3043-3049: Add showEditPatientDialog to the dependency array of
renderEditPatientDialog, preserving the existing callback logic and ensuring its
visibility condition updates with current state.
🪄 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: 85d97839-5bed-4b8f-9e62-e06f21040a7e
📒 Files selected for processing (1)
app/pages/clinicworkspace/ClinicPatients.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.js (2)
1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup imports in the required order.
Keep React and Redux imports first. Keep third-party imports before Lodash imports. Move all local
../../../...imports to the final group.
app/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.js#L1-L15: move local imports after the Lodash group.app/pages/clinicworkspace/clinicPatientsDialogs/EditPatientDialog.js#L1-L15: move local imports after the Lodash group.As per coding guidelines, group imports in the required order with blank lines between groups.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.js` around lines 1 - 15, Reorder imports in AddPatientDialog.js and EditPatientDialog.js: keep React/Redux imports first, third-party imports next, Lodash imports after them, and all local ../../../ imports last, with blank lines separating each group.Source: Coding guidelines
18-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the dialog component declarations.
Define PropTypes for every dialog prop. Terminate each component declaration with a semicolon.
app/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.js#L18-L83: define PropTypes forapiandonClose, then add the final semicolon.app/pages/clinicworkspace/clinicPatientsDialogs/EditPatientDialog.js#L19-L103: define PropTypes forapi,patient, andonClose, then add the final semicolon.As per coding guidelines, define PropTypes for all component props and use required semicolons.
🤖 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/clinicPatientsDialogs/AddPatientDialog.js` around lines 18 - 83, Complete the declarations for AddPatientDialog in app/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.js (lines 18-83) and EditPatientDialog in app/pages/clinicworkspace/clinicPatientsDialogs/EditPatientDialog.js (lines 19-103) by adding PropTypes for every listed prop: api and onClose for AddPatientDialog, and api, patient, and onClose for EditPatientDialog. Terminate both component declarations with semicolons.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 `@app/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.js`:
- Around line 1-15: Reorder imports in AddPatientDialog.js and
EditPatientDialog.js: keep React/Redux imports first, third-party imports next,
Lodash imports after them, and all local ../../../ imports last, with blank
lines separating each group.
- Around line 18-83: Complete the declarations for AddPatientDialog in
app/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.js (lines
18-83) and EditPatientDialog in
app/pages/clinicworkspace/clinicPatientsDialogs/EditPatientDialog.js (lines
19-103) by adding PropTypes for every listed prop: api and onClose for
AddPatientDialog, and api, patient, and onClose for EditPatientDialog. Terminate
both component declarations with semicolons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16e9de9d-08d7-49a7-887b-ed93c75fa75e
📒 Files selected for processing (4)
app/components/clinic/PatientForm/PatientForm.jsapp/pages/clinicworkspace/ClinicPatients.jsapp/pages/clinicworkspace/clinicPatientsDialogs/AddPatientDialog.jsapp/pages/clinicworkspace/clinicPatientsDialogs/EditPatientDialog.js
| const deletingClinicSite = useSelector((state) => state.blip.working.deletingClinicSite); | ||
| const deletingClinicPatientTag = useSelector((state) => state.blip.working.deletingClinicPatientTag); | ||
| const fetchingTideDashboardPatients = useSelector((state) => state.blip.working.fetchingTideDashboardPatients); | ||
| const fetchingRpmReportPatients = useSelector((state) => state.blip.working.fetchingRpmReportPatients); |
There was a problem hiding this comment.
By calling the non-specific useSelector(state => state.blip.working) which returns the entire working state, we cause the component to re-render every time any part of state.blip.working changes, regardless of whether it is a slice we are interested in. So we may reduce re-rendering by returning specific slices instead of state.blip.working as a whole. It may be marginally helpful.
WEB-4677
I've made a few modifications to hopefully improve performance.
The main issue really is that the
formikContextis held in state at the top level of the page component forClinicPatients. So for every keystroke, when theformikContextgets updated, that updates the state at the top ofClinicPatients, so everything downstream must get re-rendered and re-calculated.By abstracting out the modal contents and scoping the
formikContextto the modal, we ensure that changes to theformikContextonly force re-rendering for the modal and contents, so the rest of the page doesn't need to get re-calculated.