Release 1.101.0 to master - #1998
Open
clintonium-119 wants to merge 85 commits into
Open
Conversation
Functional rewrite of app/pages/userprofile (theme-ui, useSelector, useTranslation). Adds personUtils.isSSOAccount and a stub selectMfaStatus selector that reads allUsersMap[loggedInUserId] .profile.clinic.mfa with a normalized disabled-baseline fallback. Flow-trigger buttons (Edit / Update Password / Set up 2FA / Disable 2FA / Regenerate Codes) fire trackMetric only; real flows land in WEB-4567/4568/4569/4570/4571. Deletes userprofile.less and the matching @import in app/style.less. Rewrites userprofile.test.js as RTL coverage of every branch (non-SSO 2FA disabled / enabled / at-threshold, SSO clinician, personal user, button-stub trackMetric).
Each row's outer Flex now uses flexDirection: ['column', 'row'] with width: ['100%', 'auto'] on the action button, so iPhone SE (<512px breakpoint) stacks the button below the content instead of cramping it inline. Desktop layout (>=512px) unchanged.
The mock2FA flag and its hash-override block were added during WEB-4566 design preview, before real backend MFA data was wired. Removing them lets the page (and CSV export) reflect the actual mfa.enabled / mfa.enabledTime fields, fixing the 3 long-failing clinicadmin.test.js CSV-export tests.
`<Select>` was spreading `id` from formik onto both the outer styled-div wrapper AND the inner native `<select>`, so `<Label htmlFor>` resolved to a non-labellable div and `getByLabelText` failed. Destructure `id` and bind it only to the native select via `inputId = id || name`. Also pin `process.env.TZ = 'UTC'` in `jest.config.js` so direct `npx jest` invocations match `yarn test`'s behavior — three suites with timezone-dependent assertions were silently passing only when invoked through the npm script.
Adds EditPersonalDetailsDialog (name, email-with-SSO-branched Update Email, optional clinic role) and wires it to UserProfile's Edit button. Save Changes uses `fieldsAreValid` for the disabled gate; clinician role list reuses `clinicUtils.roles` (NPI dropped per DEC-0011). 27 tests across both suites.
…B-4571) Add redirectToKeycloakAction() helper to keycloak.js for handling Keycloak's kc_action flows (UPDATE_EMAIL, UPDATE_PASSWORD, etc.). Wire handleUpdateEmail in EditPersonalDetailsDialog to redirect to UPDATE_EMAIL action. Include test coverage for the new helper and dialog behavior.
…eturn-side toasts
(WEB-4570)
Wire ManagePasswordRow's Update Password click to
redirectToKeycloakAction
('UPDATE_PASSWORD', ${origin}/profile). On /profile mount, read
kc_action_status
from both window.location.hash (Keycloak's default
response_mode=fragment
delivery channel) and window.location.search; render the danger-variant
'Password reset cancelled.' toast on cancelled/error and the
success-variant
'Password reset successful. You can now log in using your new password.'
toast
on success. Strip kc_action and kc_action_status from whichever channel
carried
them, preserving OAuth params (state, session_state, iss, code) for
keycloak-js.
Unit-test coverage in test/unit/pages/userprofile.test.js for button
wiring,
all three statuses across hash and search, OAuth-param preservation, and
hash-vs-search priority.
Stop the synthetic `dataSources: [{ state: 'pending' }]` write that the
clinician-initiated invite flow sent through updateClinicPatient. Derive
pending / pendingReconnect / pendingExpired UI states from
patient.connectionRequests[providerName] joined with patient.dataSources
via a new shared resolveConnectState helper, per the BACK-4414 contract.
- DataConnections: drop patientUpdates from send/resend handlers; add
resolveConnectState
(pending/pendingReconnect/pendingExpired/connected/
error/disconnected) and route the UI through it.
- TideDashboard, ClinicPatients: read connection status via the new
helper instead of inspecting dataSources state strings directly.
- clinicUtils: remove orphaned dataSources + connectDexcom validation
from patientSchema (no caller produces those fields).
- Tests + storybook fixtures updated to the connectionRequests shape;
legacy pending-state assertions retired.
…ctions dialog Add SetupTwoFactorInstructionsDialog (pre-setup advisories); "I understand" redirects to Keycloak CONFIGURE_TOTP. Add the /profile return-toast branch and normalize cancelled toasts to the info variant across all kc_action flows.
…ith Figma Introduces the team's first RTK Query endpoint as a pilot for the broader thunk -> RTK migration. The /profile 2FA section's MFA status read now flows through app/redux/features/mfaStatus/mfaStatusApi.js -- getMfaStatus is injected onto the shared RTKQueryApi using a custom queryFn that wraps the existing fetchKeycloakCredentials() helper (Keycloak host + bearer auth differs from the platform baseQuery, so transformResponse is intentionally not used -- the credentials -> mfaStatus map runs inside the queryFn). Adds tagTypes: ['MfaStatus'] + providesTags now so a future MFA mutation can invalidate the read with one line. The component consumes the generated useGetMfaStatusQuery hook directly: selectMfaStatus, working.fetchingMfaStatus, and the fetchMfaStatus thunk are all removed, so this branch ships no new legacy redux. Visual updates to /profile, matching the current Figma: - Row headings (name, Manage password, Two-factor authentication, Recovery Codes) render bold black instead of Tidepool primary blue - 2FA pill reads "2FA Enabled" with blueGreyDark text on the green bg - 2FA + Recovery Codes merge into a single card; each section now uses an inset lightestGrey panel with three responsive columns (label/value cells + action button), stacking on mobile - Disable 2FA uses the danger button variant - Recovery Codes heading gets an info icon backed by PopoverLabel; the hover popover renders the Figma copy + an underlined indigo link to URL_SUPPORT_RECOVERY_CODES (placeholder constant pointing at the generic account-settings article until a recovery-codes-specific article is published) Tests: rewrites test/unit/pages/userprofile.test.js onto a real configureStore (RTKQueryApi reducer + middleware + a static blip reducer) with the keycloak collaborators stubbed to control MFA state per case. Adds __tests__/unit/app/redux/features/mfaStatus/mfaStatusApi.test.js covering the endpoint via setupStore + Provider + MSW. Removes the two unmerged thunk-era tests under test/unit/redux/. Full redux regression green (1777 tests).
…disable-2fa [WEB-4568] Disable 2FA on Account Settings
…nfirm dialog The Disable 2FA button on Account Settings now opens a destructive-confirm dialog (three advisories: device removal, 2FA turned off, recovery codes deleted) whose "I understand" redirects to Keycloak's delete_credential:<id> flow. mapKeycloakCredentialsToMfaStatus surfaces device.id; the kc_action_status reader has a delete_credential branch (success/danger/info variants) with a tolerant matcher accepting the suffixed form Keycloak may echo.
…AUTHN_CODES via confirm dialog Adds a new RegenerateRecoveryCodesConfirmDialog (three advisories: codes invalidated, identity re-verification, save immediately) opened from the Recovery Codes panel's "Regenerate Codes" button; "I understand" redirects to Keycloak's realm-level CONFIGURE_RECOVERY_AUTHN_CODES application-initiated action. Adds a parallel branch in the /profile kc_action_status reader for success/error/cancelled return toasts (danger variant on error, overriding the Figma info-style design). 78/78 affected-suite tests green.
Replace the provisional three-advisory placeholder with the finalized single-advisory design: new title "Generating new recovery codes?" (no warning icon), one security advisory, Cancel / I understand footer. Realign userprofile dialog-heading test assertions.
Replace the placeholder mock2FA logic with the real clinician
securityProfile.{mfaEnabled,mfaEnabledTime} fields in the Clinic Admin
member table and CSV export. Remove the mock2FA flag and its hash-based
fabrication, and make the 2FA badge's icon label translatable.
Update clinicadmin tests to the securityProfile clinician shape.
…column [WEB-4566] Clinic admin mfa column
Functional rewrite of app/pages/userprofile (theme-ui, useSelector, useTranslation). Adds personUtils.isSSOAccount and a stub selectMfaStatus selector that reads allUsersMap[loggedInUserId] .profile.clinic.mfa with a normalized disabled-baseline fallback. Flow-trigger buttons (Edit / Update Password / Set up 2FA / Disable 2FA / Regenerate Codes) fire trackMetric only; real flows land in WEB-4567/4568/4569/4570/4571. Deletes userprofile.less and the matching @import in app/style.less. Rewrites userprofile.test.js as RTL coverage of every branch (non-SSO 2FA disabled / enabled / at-threshold, SSO clinician, personal user, button-stub trackMetric).
The 2FA "Personal device name" cell rendered the full name on one line with no overflow handling, so a long name (256-char limit) spilled across the Created column and the panel edge. Add an opt-in truncate + title prop to InsetCell and enable it on the device-name cell, keeping the device-name and Created cells at an even split so the name truncates with an ellipsis at ~half the row and the row aligns with the Recovery Codes row below. The full name is available on hover.
Truncate long 2FA device name on Account Settings
After accepting a clinician invite, getClinicsForClinician seeds clinic.clinicians with only the viewer's own record. An admin viewer therefore looked like the sole admin, so the banner's sole-admin exemption suppressed the "Set Up 2FA" banner until a navigation round-trip reloaded the full roster. Fetch the full clinician roster via a skip-gated RTK Query hook when the partial data makes the viewer appear to be the sole admin, and count admins from the fetched roster. The exemption is then decided on complete data and the banner renders on first load. The gate reads the partial count from state.blip, not the query cache, so resolving the query does not flip skip and cannot loop. Add the cliniciansApi mock to the suites that render the app-wide provider.
On narrow viewports the Edit Personal Details Email well now stacks the Update Email button full-width beneath the email address instead of floating it right, where its label wrapped. Responsive flexDirection and button width keyed to the 768px breakpoint; desktop layout unchanged.
[WEB-4656] Design review updates to mobile 2fa dialogs
[WEB-4629] Show 2FA banner on first workspace load after accepting invite
Release 1.100.0 to develop
[WEB-4565,WEB-4566] Account settings and clinic Admin 2FA management
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces legacy profile updates with RTK Query, adds a functional account-security page with Keycloak MFA actions, introduces MFA enrollment banners, and exposes clinician MFA data in clinic-admin views and CSV exports. Tests cover the new APIs, dialogs, profile flows, banners, and reporting. ChangesAccount security and MFA APIs
MFA presentation and reporting
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 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 |
Member
Author
|
/deploy qa2 |
Collaborator
|
clintonium-119 updated values.yaml file in qa2 |
Collaborator
|
clintonium-119 updated flux policies file in qa2 |
Collaborator
|
clintonium-119 deployed blip release-1.101.0 branch to qa2 namespace |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (12)
test/unit/pages/clinicdetails.test.js (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the legacy Sinon mock pattern in these
test/files.Both files use the legacy Mocha, Chai, and Sinon test style. The new Jest module mocks and
jest.fn()mocks mix test frameworks and bypass the required stub reset pattern.
test/unit/pages/clinicdetails.test.js#L10-L12: replace the Jest module mock with the legacy-compatible hook stub setup.test/unit/pages/clinicdetails.test.js#L197-L204: usesinon.stub()and reset the stub inbeforeEach.test/unit/pages/patientnew.test.js#L8-L10: replace the Jest module mock with the legacy-compatible hook stub setup.test/unit/pages/patientnew.test.js#L78-L87: usesinon.stub()and reset the stub inbeforeEach.As per coding guidelines,
test/**/*.jsmust usesinon.stub()for mocks and reset stubs inbeforeEach.🤖 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 `@test/unit/pages/clinicdetails.test.js` around lines 10 - 12, Replace the Jest module mocks and jest.fn usage for useUpdateUserProfileMutation with legacy Sinon stubs in test/unit/pages/clinicdetails.test.js (lines 10-12 and 197-204) and test/unit/pages/patientnew.test.js (lines 8-10 and 78-87). Configure each hook stub with sinon.stub() and reset or restore it in beforeEach, preserving the existing test behavior and legacy Mocha/Chai/Sinon style.Source: Coding guidelines
app/pages/clinicadmin/clinicadmin.js (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the required import-group order.
Line 21 adds a third-party Material UI import after the Lodash and theme-ui groups. Move the Material UI imports into the third-party group before the Lodash imports.
As per coding guidelines: "
**/*.{js,jsx,ts,tsx}: 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/clinicadmin/clinicadmin.js` at line 21, Reorder the imports in the clinic admin module so the Material UI import, including VerifiedUserRoundedIcon, appears in the third-party group before the Lodash imports. Preserve blank-line separation and the required group order through the theme-ui and local imports.Source: Coding guidelines
app/pages/userprofile/userprofile.js (3)
909-918: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the conflict between
isRequiredand the default value.Line 910 declares
trackMetricasPropTypes.func.isRequired, and Line 917 suppliesnoopas its default. React appliesdefaultPropsbefore PropTypes validation, so the required check can never fail and callers receive no warning when they omittrackMetric. Choose one: dropisRequired, or drop the default.♻️ Proposed refactor
UserProfile.propTypes = { - trackMetric: PropTypes.func.isRequired, + trackMetric: PropTypes.func, history: PropTypes.object, api: PropTypes.object.isRequired, location: PropTypes.object, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/pages/userprofile/userprofile.js` around lines 909 - 918, Resolve the trackMetric contract in UserProfile.propTypes and UserProfile.defaultProps by either removing isRequired to keep the noop fallback, or removing the trackMetric default to require callers to provide it; ensure the declaration and fallback consistently represent the chosen behavior.
486-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInterpolate the code total instead of hardcoding it in the translated string.
The string embeds
12whileRECOVERY_CODES_TOTALholds the same value at Line 56. If the total ever changes, this copy silently becomes wrong, and translators receive a literal number they cannot adjust.♻️ Proposed refactor
- {t('You’ll receive 12 single-use codes. Each code works once and then expires. Generating a new set will deactivate previous codes.')} + {t('You’ll receive {{total}} single-use codes. Each code works once and then expires. Generating a new set will deactivate previous codes.', { total: RECOVERY_CODES_TOTAL })}🤖 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/userprofile/userprofile.js` at line 486, Update the recovery-code explanatory text near RECOVERY_CODES_TOTAL to interpolate that constant into the translated string instead of hardcoding 12. Pass the total through the existing translation call so the displayed count and translator message stay synchronized when RECOVERY_CODES_TOTAL changes.
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse specific Lodash imports and reuse
RECOVERY_CODES_TOTAL.Two small alignment items:
- Line 21 imports the full
lodashnamespace while Line 22 already uses the per-method form. The file uses onlygetandfind.- Line 65 hardcodes
12althoughRECOVERY_CODES_TOTALis defined at Line 56.As per coding guidelines: "Use specific Lodash imports such as
import get from 'lodash/get'instead of importing from fulllodash."♻️ Proposed refactor
-import _ from 'lodash'; +import find from 'lodash/find'; +import get from 'lodash/get'; import noop from 'lodash/noop';- recoveryCodes: { used: 0, total: 12, generatedTime: null }, + recoveryCodes: { used: 0, total: RECOVERY_CODES_TOTAL, generatedTime: null },Then replace
_.get(...)withget(...)and_.find(...)withfind(...)throughout the file.Also applies to: 56-66
🤖 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/userprofile/userprofile.js` around lines 21 - 22, Replace the full Lodash namespace import in the user profile module with specific imports for get and find, then update all _.get and _.find usages to call those imports directly. In the recovery-code logic near RECOVERY_CODES_TOTAL, replace the hardcoded 12 with the existing RECOVERY_CODES_TOTAL constant.Source: Coding guidelines
test/unit/pages/userprofile.test.js (1)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the MFA fixture timestamps with the real mapper output.
mapKeycloakCredentialsToMfaStatusreturns KeycloakcreatedDatevalues, which are epoch milliseconds. This fixture uses ISO strings. The tests still pass becausemomentaccepts both, so a regression in numeric-timestamp handling stays undetected. Use epoch milliseconds, astest/unit/keycloak.test.jsdoes.🤖 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 `@test/unit/pages/userprofile.test.js` around lines 42 - 49, Update the enabledMfaStatus fixture timestamps, including enabledTime, device.registeredTime, and recoveryCodes.generatedTime, to epoch-millisecond numbers matching mapKeycloakCredentialsToMfaStatus and the established test/unit/keycloak.test.js fixture format; preserve the existing timestamp values’ intended dates.test/unit/pages/EditPersonalDetailsDialog.test.js (3)
141-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the mutation failure path and the processing state.
The suite covers the happy path, validation, and cancel. Two behaviors stay untested: the danger toast when
unwrap()rejects, and theprocessingstate whenisLoadingis true. Both are user-visible.💚 Proposed test additions
+ describe('Save Changes — failure path', () => { + it('shows a danger toast and keeps the dialog open when the mutation rejects', async () => { + mockMutate.mockReturnValue({ + unwrap: jest.fn().mockRejectedValue({ data: 'Something went wrong updating your account.' }), + }); + const { onClose } = renderWith(buildState(clinicianUser)); + fireEvent.click(screen.getByRole('button', { name: 'Save Changes' })); + expect(await screen.findByText('Something went wrong updating your account.')).to.exist; + expect(onClose.called).to.be.false; + }); + });🤖 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 `@test/unit/pages/EditPersonalDetailsDialog.test.js` around lines 141 - 158, Extend the EditPersonalDetailsDialog tests with coverage for the mutation failure path by making unwrap() reject and asserting the danger toast, and cover the processing state by rendering with isLoading true and asserting the save UI reflects processing. Reuse the existing renderWith, buildState, and mockMutate setup.
77-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two
beforeEachblocks.Lines 77-79 and 83-86 both run before every test, and the
let mockMutatedeclaration sits between them. Combine them into one block placed after the declaration for readability.🤖 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 `@test/unit/pages/EditPersonalDetailsDialog.test.js` around lines 77 - 90, Merge the two beforeEach hooks surrounding mockMutate into a single beforeEach placed after the let mockMutate declaration; retain both redirectToKeycloakAction.mockClear() and the mockMutate/useUpdateUserProfileMutation setup in that combined hook.
1-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace this new Jest suite under
__tests__/.
EditPersonalDetailsDialogis a new component with no legacy Karma/Mocha test file. This suite uses Jest and@testing-library/react, so it belongs in__tests__/. Keeping it intest/expands the legacy suite and mixeschai/sinonwith Jest mocks in one file.Move the file to
__tests__/unit/app/pages/userprofile/EditPersonalDetailsDialog.test.jsand switchchai.expect/sinon.stub()toexpect/jest.fn().As per path instructions: "Write new tests in
__tests__/using Jest and@testing-library/react" and "Maintain legacy Karma/Mocha tests intest/and do not expand that suite."🤖 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 `@test/unit/pages/EditPersonalDetailsDialog.test.js` around lines 1 - 26, Move the EditPersonalDetailsDialog test suite to __tests__/unit/app/pages/userprofile/EditPersonalDetailsDialog.test.js, keeping its Jest and `@testing-library/react` setup. Replace the legacy chai.expect and sinon.stub usage with Jest’s expect and jest.fn, including the global declarations and any affected assertions or mocks.Source: Path instructions
app/redux/features/userProfile/userProfileApi.js (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent
catch {}hides real dispatch failures.The empty catch swallows both the expected
queryFulfilledrejection and any error thrown bygetStateordispatch. Bind the error and log it, so a broken sync path is observable.♻️ Proposed change
- } catch {} + } catch (err) { + // The mutation error is surfaced to the caller via unwrap(); log unexpected sync failures. + if (process.env.NODE_ENV !== 'production') console.error(err); + }🤖 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/redux/features/userProfile/userProfileApi.js` around lines 45 - 51, Update the onQueryStarted handler to bind the caught error and log it with the existing logging mechanism, preserving the current query rejection handling while making failures from getState or sync.updateUserSuccess observable.test/unit/keycloak.test.js (1)
475-549: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for malformed
credentialData.The mapper documents a fallback when recovery
credentialDatais absent or invalid, but no test covers it. Add two cases: unparsable JSON, and JSON that omitstotalCodes/remainingCodes. The second case currently producesNaNforrecoveryCodes.used, which is the defect raised onapp/keycloak.jslines 277-299.💚 Proposed test additions
it('returns passwordUpdatedTime null when there is no password credential', () => { const result = mapKeycloakCredentialsToMfaStatus([ { type: 'otp', userCredentialMetadatas: [{ credential: { id: 'otp-1', createdDate: 1779895908703 } }] }, ]); expect(result.passwordUpdatedTime).to.be.null; }); + + it('falls back to the default recovery values when credentialData is not valid JSON', () => { + const result = mapKeycloakCredentialsToMfaStatus([ + { type: 'recovery-authn-codes', userCredentialMetadatas: [{ credential: { id: 'rec-1', credentialData: 'not-json' } }] }, + ]); + expect(result.recoveryCodes.used).to.equal(0); + expect(result.recoveryCodes.total).to.equal(12); + }); + + it('falls back to the default recovery values when credentialData omits the code counts', () => { + const result = mapKeycloakCredentialsToMfaStatus([ + { type: 'recovery-authn-codes', userCredentialMetadatas: [{ credential: { id: 'rec-1', credentialData: '{"algorithm":"SHA-512"}' } }] }, + ]); + expect(result.recoveryCodes.used).to.equal(0); + expect(result.recoveryCodes.total).to.equal(12); + });As per path instructions: "When updating existing code that only has Karma/Mocha tests, add tests to the existing test file instead of creating a new Jest file."
🤖 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 `@test/unit/keycloak.test.js` around lines 475 - 549, Add two tests under mapKeycloakCredentialsToMfaStatus covering recovery credentials with unparsable credentialData and valid JSON missing totalCodes/remainingCodes. Assert the mapper uses its documented fallback values and, for the missing-fields case, recoveryCodes.used is not NaN while preserving the expected default total.Source: Path instructions
app/pages/userprofile/EditPersonalDetailsDialog.js (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse specific Lodash imports.
Replace the full
lodashimport with per-method imports. The file usesgetandnooponly.♻️ Proposed change
-import _ from 'lodash'; +import get from 'lodash/get'; +import noop from 'lodash/noop';Then update the call sites:
_.get(user, ...)→get(user, ...), and_.noop→noop.As per coding guidelines: "Use specific Lodash imports such as
import get from 'lodash/get'instead of importing from fulllodash."🤖 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/userprofile/EditPersonalDetailsDialog.js` at line 6, Replace the full Lodash import in EditPersonalDetailsDialog with specific get and noop imports from their individual lodash modules, then update the corresponding _.get and _.noop call sites to use get and noop directly.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/components/elements/Dialog.js`:
- Around line 191-192: Move the paperWidth* max-width cap rules out of the base
Dialog styles and into a min-width media query, so the mobile width: 100% and
max-width: 100% remain effective below that breakpoint. Update the later
.MuiDialog-paperWidth* rules while preserving their existing caps for wider
viewports.
In `@app/keycloak.js`:
- Around line 277-299: Update the recovery-code handling in the surrounding
function to import and use the shared RECOVERY_CODES_TOTAL constant from the
user profile module instead of the literal 12. Before calculating
recoveryCodes.used, validate that recoveryData.totalCodes and
recoveryData.remainingCodes are finite numbers; otherwise use 0, while
preserving the existing total and generatedTime behavior.
- Around line 245-262: Update fetchKeycloakCredentials to await
keycloak.updateToken(...) before constructing the Authorization header, then add
an AbortController timeout signal to the fetch request so stalled credential
requests terminate and propagate the existing error handling.
In `@app/pages/clinicadmin/clinicadmin.js`:
- Around line 102-107: Update the MFA value preparation used by the clinician
sorting flow around toSortKey and the securityProfile mapping so missing
mfaEnabled values are normalized to a sortable string before utils.compareLabels
receives them. Preserve existing boolean handling and ensure clinicians without
a securityProfile, including pending invites, sort without passing undefined.
- Line 105: Replace Array.prototype.toSorted() in the clinician sorting flow
with a compatibility-safe copied sort, preserving the original clinicianArray
order while producing the same sortedArray result; do not rely on a toSorted
polyfill.
In `@app/pages/clinicdetails/clinicdetails.js`:
- Around line 355-356: Update the branch containing
updateUserProfile(profileUpdates) to await its unwrap result, catch rejection,
handle the error, and return before dispatching the clinic action or proceeding
with migration. Add a rejection test verifying api.clinics.update is not called
when the clinician profile mutation fails.
In `@app/pages/userprofile/DisableTwoFactorConfirmDialog.js`:
- Around line 17-20: Update handleConfirm in
app/pages/userprofile/DisableTwoFactorConfirmDialog.js (lines 17-20),
handleUpdatePassword in app/pages/userprofile/userprofile.js (lines 199-203),
handleConfirm in app/pages/userprofile/SetupTwoFactorInstructionsDialog.js
(lines 17-19), and handleConfirm in
app/pages/userprofile/RegenerateRecoveryCodesConfirmDialog.js (lines 14-16) to
be async, await redirectToKeycloakAction, and catch failures by showing a danger
toast through each component’s existing toast mechanism. Preserve the existing
action identifiers and redirect targets, and use a shared helper if appropriate
to apply the same error-handling pattern consistently.
In `@app/pages/userprofile/EditPersonalDetailsDialog.js`:
- Around line 172-180: Update the trackMetric declaration in
EditPersonalDetailsDialog.propTypes to make it optional, matching the _.noop
fallback defined in EditPersonalDetailsDialog.defaultProps; leave the other
required props unchanged.
In `@app/pages/userprofile/userprofile.js`:
- Around line 606-609: Coerce the open2faSetupOnMount expression to an explicit
boolean before passing it to useState, while preserving the existing isClinician
and !isSSO conditions so setup2faOpen always satisfies
SetupTwoFactorInstructionsDialog’s required boolean open prop.
- Around line 895-899: Guard the credentialId prop in
DisableTwoFactorConfirmDialog by replacing the direct mfaStatus.device.id access
with the existing safe nested-access pattern used elsewhere in the file, such as
_.get, so missing device data does not throw during render.
- Around line 644-702: Replace every translated message in the status branches
of the password, email, TOTP, credential-deletion, and recovery-code handlers
with complete status-specific sentences instead of interpolating the raw
Keycloak status token. Update the cancelled and error branches in the visible
action handling while preserving their existing toast variants, and keep the
existing success messages unchanged.
- Around line 597-625: Update the MFA query and related error/retry handling in
the user profile component to skip MFA only for SSO users, not all
non-clinicians, so personal users retain password metadata. Restrict the MFA
error toast and retry state to non-SSO clinicians, while preserving the query
for personal users and their existing password “Last updated” value; use the
existing isClinician, isSSO, mfaStatus, and MFA control state symbols.
In `@app/redux/features/clinicians/cliniciansApi.js`:
- Around line 5-8: Update getCliniciansForClinic so its implementation matches
the documented 404 behavior by transforming a not-found response to an empty
array, or revise the comment to accurately describe the existing error behavior.
Also encode clinicId before interpolating it into the request path, while
preserving the existing endpoint parameters and treating the SQL-injection
warning as irrelevant.
- Around line 6-7: Update getCliniciansForClinic to support fetching the
complete paginated clinician roster instead of hard-coding limit=1000 and
offset=0. Use the API’s pagination mechanism to request subsequent pages and
aggregate their results before returning them, preserving the response shape
expected by AppBannerProvider so clinic admins beyond the first page are
included.
In `@app/redux/features/mfaStatus/__mocks__/mfaStatusApi.js`:
- Line 5: Update the useGetMfaStatusQuery mock to return all fields destructured
by the user profile consumer: data, boolean isLoading, boolean isFetching,
boolean isError, and a callable refetch function. Use safe default values so
retry interactions do not throw and required boolean props receive valid values.
In `@app/redux/features/userProfile/userProfileApi.js`:
- Around line 11-28: Guard the loggedInUser lookup in queryFn before
constructing newUser, and when it is missing, return the normalized error shape
expected by this mutation instead of dereferencing profile or allowing a
TypeError to escape. Preserve the existing merge and update behavior when
loggedInUser exists.
In `@package.json`:
- Line 7: Update the package version declaration from the prerelease value to
the stable release version 1.101.0, ensuring the release artifact is not marked
as an RC.
In `@test/unit/app/providers/AppBanner/AppBannerProvider.test.js`:
- Around line 32-35: Replace the Jest module mocks and mock-return configuration
for the RTK Query hooks with Sinon stubs, preserving the existing test behavior.
In test/unit/app/providers/AppBanner/AppBannerProvider.test.js at lines 32-35
and 106-110, stub useGetMfaStatusQuery and useGetCliniciansForClinicQuery and
reset/configure them in beforeEach; make the corresponding Jest-to-Sinon
replacements in test/unit/app/app.test.js lines 16-17 and
test/unit/app/providers/AppBanner/AppBanner.test.js lines 24-29, following the
legacy test style.
---
Nitpick comments:
In `@app/pages/clinicadmin/clinicadmin.js`:
- Line 21: Reorder the imports in the clinic admin module so the Material UI
import, including VerifiedUserRoundedIcon, appears in the third-party group
before the Lodash imports. Preserve blank-line separation and the required group
order through the theme-ui and local imports.
In `@app/pages/userprofile/EditPersonalDetailsDialog.js`:
- Line 6: Replace the full Lodash import in EditPersonalDetailsDialog with
specific get and noop imports from their individual lodash modules, then update
the corresponding _.get and _.noop call sites to use get and noop directly.
In `@app/pages/userprofile/userprofile.js`:
- Around line 909-918: Resolve the trackMetric contract in UserProfile.propTypes
and UserProfile.defaultProps by either removing isRequired to keep the noop
fallback, or removing the trackMetric default to require callers to provide it;
ensure the declaration and fallback consistently represent the chosen behavior.
- Line 486: Update the recovery-code explanatory text near RECOVERY_CODES_TOTAL
to interpolate that constant into the translated string instead of hardcoding
12. Pass the total through the existing translation call so the displayed count
and translator message stay synchronized when RECOVERY_CODES_TOTAL changes.
- Around line 21-22: Replace the full Lodash namespace import in the user
profile module with specific imports for get and find, then update all _.get and
_.find usages to call those imports directly. In the recovery-code logic near
RECOVERY_CODES_TOTAL, replace the hardcoded 12 with the existing
RECOVERY_CODES_TOTAL constant.
In `@app/redux/features/userProfile/userProfileApi.js`:
- Around line 45-51: Update the onQueryStarted handler to bind the caught error
and log it with the existing logging mechanism, preserving the current query
rejection handling while making failures from getState or sync.updateUserSuccess
observable.
In `@test/unit/keycloak.test.js`:
- Around line 475-549: Add two tests under mapKeycloakCredentialsToMfaStatus
covering recovery credentials with unparsable credentialData and valid JSON
missing totalCodes/remainingCodes. Assert the mapper uses its documented
fallback values and, for the missing-fields case, recoveryCodes.used is not NaN
while preserving the expected default total.
In `@test/unit/pages/clinicdetails.test.js`:
- Around line 10-12: Replace the Jest module mocks and jest.fn usage for
useUpdateUserProfileMutation with legacy Sinon stubs in
test/unit/pages/clinicdetails.test.js (lines 10-12 and 197-204) and
test/unit/pages/patientnew.test.js (lines 8-10 and 78-87). Configure each hook
stub with sinon.stub() and reset or restore it in beforeEach, preserving the
existing test behavior and legacy Mocha/Chai/Sinon style.
In `@test/unit/pages/EditPersonalDetailsDialog.test.js`:
- Around line 141-158: Extend the EditPersonalDetailsDialog tests with coverage
for the mutation failure path by making unwrap() reject and asserting the danger
toast, and cover the processing state by rendering with isLoading true and
asserting the save UI reflects processing. Reuse the existing renderWith,
buildState, and mockMutate setup.
- Around line 77-90: Merge the two beforeEach hooks surrounding mockMutate into
a single beforeEach placed after the let mockMutate declaration; retain both
redirectToKeycloakAction.mockClear() and the
mockMutate/useUpdateUserProfileMutation setup in that combined hook.
- Around line 1-26: Move the EditPersonalDetailsDialog test suite to
__tests__/unit/app/pages/userprofile/EditPersonalDetailsDialog.test.js, keeping
its Jest and `@testing-library/react` setup. Replace the legacy chai.expect and
sinon.stub usage with Jest’s expect and jest.fn, including the global
declarations and any affected assertions or mocks.
In `@test/unit/pages/userprofile.test.js`:
- Around line 42-49: Update the enabledMfaStatus fixture timestamps, including
enabledTime, device.registeredTime, and recoveryCodes.generatedTime, to
epoch-millisecond numbers matching mapKeycloakCredentialsToMfaStatus and the
established test/unit/keycloak.test.js fixture format; preserve the existing
timestamp values’ intended dates.
🪄 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: 08043fbf-bff4-4af0-8968-064e5588d952
📒 Files selected for processing (56)
.eslintrc.gitignore__tests__/unit/app/redux/features/mfaStatus/mfaStatusApi.test.jsapp/components/elements/Dialog.jsapp/components/elements/Select.jsapp/core/constants.jsapp/core/personutils.jsapp/core/selectors.jsapp/keycloak.jsapp/pages/clinicadmin/clinicadmin.jsapp/pages/clinicdetails/clinicdetails.jsapp/pages/patientnew/patientnew.jsapp/pages/userprofile/DisableTwoFactorConfirmDialog.jsapp/pages/userprofile/EditPersonalDetailsDialog.jsapp/pages/userprofile/RegenerateRecoveryCodesConfirmDialog.jsapp/pages/userprofile/SetupTwoFactorInstructionsDialog.jsapp/pages/userprofile/userprofile.jsapp/pages/userprofile/userprofile.lessapp/providers/AppBanner/AppBanner.jsapp/providers/AppBanner/AppBannerProvider.jsapp/providers/AppBanner/appBanners.jsapp/redux/actions/async.jsapp/redux/actions/sync.jsapp/redux/api/baseApi.jsapp/redux/constants/actionTypes.jsapp/redux/constants/actionWorkingMap.jsapp/redux/features/clinicians/__mocks__/cliniciansApi.jsapp/redux/features/clinicians/cliniciansApi.jsapp/redux/features/mfaStatus/__mocks__/mfaStatusApi.jsapp/redux/features/mfaStatus/mfaStatusApi.jsapp/redux/features/userProfile/userProfileApi.jsapp/redux/reducers/initialState.jsapp/redux/reducers/misc.jsapp/redux/reducers/working.jsapp/style.lessjest.config.jspackage.jsontest/unit/app/app.test.jstest/unit/app/providers/AppBanner/AppBanner.test.jstest/unit/app/providers/AppBanner/AppBannerProvider.test.jstest/unit/app/providers/AppBanner/appBanners.test.jstest/unit/components/useProviderConnectionPopup.test.jstest/unit/keycloak.test.jstest/unit/pages/ClinicianPatients.test.jstest/unit/pages/EditPersonalDetailsDialog.test.jstest/unit/pages/TideDashboard.test.jstest/unit/pages/clinicadmin.test.jstest/unit/pages/clinicdetails.test.jstest/unit/pages/patientnew.test.jstest/unit/pages/userprofile.test.jstest/unit/redux/actions/async.test.jstest/unit/redux/actions/sync.test.jstest/unit/redux/features/userProfile/userProfileApi.test.jstest/unit/redux/reducers/notification.test.jstest/unit/redux/reducers/working.test.jstest/unit/utils/personutils.test.js
💤 Files with no reviewable changes (13)
- app/redux/reducers/initialState.js
- app/pages/userprofile/userprofile.less
- test/unit/redux/reducers/notification.test.js
- test/unit/redux/actions/sync.test.js
- app/style.less
- app/redux/constants/actionWorkingMap.js
- app/redux/actions/async.js
- app/redux/constants/actionTypes.js
- app/redux/reducers/working.js
- app/redux/reducers/misc.js
- test/unit/redux/reducers/working.test.js
- test/unit/redux/actions/async.test.js
- app/redux/actions/sync.js
Comment on lines
+191
to
+192
| width: 100%; | ||
| max-width: 100%; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect MOBILE_BREAKPOINT and theme breakpoints used by Dialog.js.
fd -t f 'Dialog.js' -p 'app/components/elements' --exec rg -n 'MOBILE_BREAKPOINT|breakpoints|import' {}
rg -n 'MOBILE_BREAKPOINT\s*=' --glob '*.js' -g '!node_modules'
rg -n 'export const breakpoints' --glob '*.js' -g '!node_modules'Repository: tidepool-org/blip
Length of output: 1271
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Dialog.js imports and styles ---'
sed -n '1,235p' app/components/elements/Dialog.js
printf '%s\n' '--- breakpoints declaration and related theme files ---'
rg -n -C 4 'breakpoints\s*[:=]|export .*breakpoints|const breakpoints' app --glob '*.js' --glob '*.ts' --glob '*.jsx' --glob '*.tsx' | head -200Repository: tidepool-org/blip
Length of output: 6138
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
text = Path('app/components/elements/Dialog.js').read_text()
paper = text[text.index(' .MuiDialog-paper {'):text.index('\n`;', text.index(' .MuiDialog-paper {'))]
rules = re.findall(r'(?m)^\s*(?:`@media`[^{]+)?|^\s*(?:max-width|width):[^;]+;', paper)
print('Dialog paper declarations in source order:')
for line in paper.splitlines():
if 'width:' in line or 'max-width:' in line:
print(line.strip())
print('\nBreakpoint values:')
theme = Path('app/themes/baseTheme.js').read_text()
match = re.search(r'export const breakpoints = \[([^\]]+)\]', theme)
print(match.group(1))
print('\nRelevant mobile cases:')
for width, cap in [(600, 512), (700, 512), (700, 768), (500, 512)]:
mobile = width <= 768
effective = min(width, cap) if mobile else min(width, cap)
print(f'viewport={width}px, paperWidth cap={cap}px, mobile={mobile}: effective max width={effective}px')
PYRepository: tidepool-org/blip
Length of output: 791
Move the paperWidth* caps into a min-width media query.
The later .MuiDialog-paperWidth* rules override the mobile max-width: 100%. This prevents full width for paperWidth="sm" on mobile viewports wider than 512px.
🤖 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/components/elements/Dialog.js` around lines 191 - 192, Move the
paperWidth* max-width cap rules out of the base Dialog styles and into a
min-width media query, so the mobile width: 100% and max-width: 100% remain
effective below that breakpoint. Update the later .MuiDialog-paperWidth* rules
while preserving their existing caps for wider viewports.
Comment on lines
+245
to
+262
| export async function fetchKeycloakCredentials() { | ||
| const url = new URL(`${keycloak.authServerUrl}/realms/${keycloak.realm}/account/credentials`); | ||
| const response = await fetch(url.toString(), { | ||
| method: 'GET', | ||
| headers: { | ||
| Accept: 'application/json', | ||
| Authorization: `Bearer ${keycloak.token}`, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const error = new Error(`Failed to fetch Keycloak credentials (${response.status})`); | ||
| error.status = response.status; | ||
| throw error; | ||
| } | ||
|
|
||
| return response.json(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the existing keycloak module refreshes tokens before authenticated requests.
rg -n -C 6 'updateToken|isTokenExpired|keycloak\.token' app/keycloak.js
rg -n -C 4 'updateToken' --type=js -g '!node_modules' -g '!**/*.test.js'Repository: tidepool-org/blip
Length of output: 2377
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- app/keycloak.js relevant sections ---'
cat -n app/keycloak.js | sed -n '1,180p;220,285p'
printf '%s\n' '--- fetchKeycloakCredentials callers ---'
rg -n -C 5 'fetchKeycloakCredentials' .
printf '%s\n' '--- project runtime and browser support ---'
rg -n 'AbortSignal\.timeout|AbortController|browserslist|target|webpack|babel' package.json .browserslistrc 2>/dev/null || trueRepository: tidepool-org/blip
Length of output: 20926
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- MFA query and loading/error handling ---'
cat -n app/redux/features/mfaStatus/mfaStatusApi.js
rg -n -C 6 'getMfaStatus|isLoading|Failed to load|mfaStatus' app/pages test/unit/pages __tests__/unit/app 2>/dev/null | head -240
printf '%s\n' '--- timeout and browser-target configuration ---'
rg -n -C 3 'AbortSignal|AbortController|timeout\(' app test __tests__ package.json webpack* config* . 2>/dev/null | head -240
git ls-files | rg '(^|/)(package\.json|.*browserslist.*|babel\.config.*|\.babelrc.*|webpack.*|\.nvmrc)$'Repository: tidepool-org/blip
Length of output: 19135
Refresh the token and add a request deadline.
setTokenRefresh schedules refreshes, but fetchKeycloakCredentials sends keycloak.token without awaiting keycloak.updateToken(...). Refresh the token before the request. A stale token causes the MFA query to show Failed to load.
Add an AbortController deadline. A stalled response leaves getMfaStatus pending and the 2FA row in its loading state.
🤖 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/keycloak.js` around lines 245 - 262, Update fetchKeycloakCredentials to
await keycloak.updateToken(...) before constructing the Authorization header,
then add an AbortController timeout signal to the fetch request so stalled
credential requests terminate and propagate the existing error handling.
Comment on lines
+277
to
+299
| let recoveryData = null; | ||
| if (recoveryCred?.credentialData) { | ||
| try { | ||
| recoveryData = JSON.parse(recoveryCred.credentialData); | ||
| } catch (e) { | ||
| recoveryData = null; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| enabled: !!otpCred, | ||
| enabledTime: otpCred?.createdDate ?? null, | ||
| passwordUpdatedTime: passwordCred?.createdDate ?? null, | ||
| device: { | ||
| id: otpCred?.id ?? null, | ||
| name: otpCred?.userLabel ?? null, | ||
| registeredTime: otpCred?.createdDate ?? null, | ||
| }, | ||
| recoveryCodes: { | ||
| used: recoveryData ? recoveryData.totalCodes - recoveryData.remainingCodes : 0, | ||
| total: recoveryData?.totalCodes ?? 12, | ||
| generatedTime: recoveryCred?.createdDate ?? null, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the recovery-code arithmetic and reuse the shared total constant.
Two points:
- If
credentialDataparses to JSON withouttotalCodesorremainingCodes,usedbecomesNaN. The UI then renders "NaN/12 used". Validate both values are finite numbers before the subtraction. 12is duplicated here.app/pages/userprofile/userprofile.jsalready usesRECOVERY_CODES_TOTALfor the same default. Import the shared constant instead.
🛡️ Proposed fix
let recoveryData = null;
if (recoveryCred?.credentialData) {
try {
recoveryData = JSON.parse(recoveryCred.credentialData);
} catch (e) {
recoveryData = null;
}
}
+ const totalCodes = Number.isFinite(recoveryData?.totalCodes) ? recoveryData.totalCodes : null;
+ const remainingCodes = Number.isFinite(recoveryData?.remainingCodes) ? recoveryData.remainingCodes : null;
+
return {
enabled: !!otpCred,
enabledTime: otpCred?.createdDate ?? null,
passwordUpdatedTime: passwordCred?.createdDate ?? null,
device: {
id: otpCred?.id ?? null,
name: otpCred?.userLabel ?? null,
registeredTime: otpCred?.createdDate ?? null,
},
recoveryCodes: {
- used: recoveryData ? recoveryData.totalCodes - recoveryData.remainingCodes : 0,
- total: recoveryData?.totalCodes ?? 12,
+ used: totalCodes !== null && remainingCodes !== null ? totalCodes - remainingCodes : 0,
+ total: totalCodes ?? RECOVERY_CODES_TOTAL,
generatedTime: recoveryCred?.createdDate ?? null,
},
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/keycloak.js` around lines 277 - 299, Update the recovery-code handling in
the surrounding function to import and use the shared RECOVERY_CODES_TOTAL
constant from the user profile module instead of the literal 12. Before
calculating recoveryCodes.used, validate that recoveryData.totalCodes and
recoveryData.remainingCodes are finite numbers; otherwise use 0, while
preserving the existing total and generatedTime behavior.
Comment on lines
+102
to
+107
| // compareLabels uses localeCompare, which throws on a boolean — coerce booleans to strings. | ||
| const toSortKey = (val) => (typeof val === 'boolean' ? String(val) : val); | ||
|
|
||
| const sortedArray = clinicianArray.toSorted((a, b) => { | ||
| return ( | ||
| utils.compareLabels(a[orderBy], b[orderBy]) || // group by designated column | ||
| utils.compareLabels(toSortKey(a[orderBy]), toSortKey(b[orderBy])) || // group by designated column |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize missing MFA values before sorting.
Line 283 leaves mfaEnabled as undefined when securityProfile is absent. Line 103 preserves that value. The Security sort then passes it to utils.compareLabels, whose localeCompare path cannot compare undefined. Sorting a clinic that has a pending invite can crash the table.
Proposed fix
- const toSortKey = (val) => (typeof val === 'boolean' ? String(val) : val);
+ const toSortKey = (val) => String(val ?? '');📝 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.
Suggested change
| // compareLabels uses localeCompare, which throws on a boolean — coerce booleans to strings. | |
| const toSortKey = (val) => (typeof val === 'boolean' ? String(val) : val); | |
| const sortedArray = clinicianArray.toSorted((a, b) => { | |
| return ( | |
| utils.compareLabels(a[orderBy], b[orderBy]) || // group by designated column | |
| utils.compareLabels(toSortKey(a[orderBy]), toSortKey(b[orderBy])) || // group by designated column | |
| // compareLabels uses localeCompare, which throws on a boolean — coerce booleans to strings. | |
| const toSortKey = (val) => String(val ?? ''); | |
| const sortedArray = clinicianArray.toSorted((a, b) => { | |
| return ( | |
| utils.compareLabels(toSortKey(a[orderBy]), toSortKey(b[orderBy])) || // group by designated column |
🤖 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/clinicadmin/clinicadmin.js` around lines 102 - 107, Update the MFA
value preparation used by the clinician sorting flow around toSortKey and the
securityProfile mapping so missing mfaEnabled values are normalized to a
sortable string before utils.compareLabels receives them. Preserve existing
boolean handling and ensure clinicians without a securityProfile, including
pending invites, sort without passing undefined.
| // compareLabels uses localeCompare, which throws on a boolean — coerce booleans to strings. | ||
| const toSortKey = (val) => (typeof val === 'boolean' ? String(val) : val); | ||
|
|
||
| const sortedArray = clinicianArray.toSorted((a, b) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect declared JavaScript and browser targets.
fd -HI -t f '^(package\.json|\.browserslistrc|\.babelrc|\.babelrc\.json|babel\.config\.(js|cjs|json))$' . -0 |
xargs -0 -r -n1 sh -c '
echo "===== $1 ====="
sed -n "1,240p" "$1"
' sh
# Locate all runtime uses for a compatibility decision.
rg -n --hidden \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' \
'\btoSorted\s*\(' .Repository: tidepool-org/blip
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '===== tracked target/config files ====='
git ls-files | grep -E '(^|/)(package\.json|\.browserslistrc|\.babelrc(\.json)?|babel\.config\.(js|cjs|json)|webpack.*\.(js|cjs|json))$' |
while IFS= read -r file; do
echo "===== $file ====="
sed -n '1,260p' "$file"
done
echo '===== tracked toSorted uses ====='
git grep -n -E '\btoSorted[[:space:]]*\(' -- ':!node_modules' ':!dist' ':!build' || true
echo '===== root tracked files relevant to CI/browser support ====='
git ls-files '.github/**' 'Dockerfile*' '*browserslist*' '*babel*' | sed -n '1,200p'Repository: tidepool-org/blip
Length of output: 18149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '===== polyfill declarations ====='
git grep -n -E '`@babel/polyfill`|core-js|browserslist|targets' -- \
'package.json' 'yarn.lock' 'babel.config.js' 'webpack.config.js' \
'.browserslistrc' 'babel.config.*' 2>/dev/null || true
echo '===== relevant lockfile entries ====='
rg -n -A18 -B3 '^("?`@babel/polyfill`|"?core-js(@|")|"?core-js-pure)' yarn.lock | sed -n '1,240p'Repository: tidepool-org/blip
Length of output: 6341
Replace toSorted() with a compatibility-safe sort
The application loads @babel/polyfill backed by core-js 2.6.12, which does not polyfill Array.prototype.toSorted(). Browser targets are also undeclared. Use a copied sort() call or load a toSorted polyfill.
🤖 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/clinicadmin/clinicadmin.js` at line 105, Replace
Array.prototype.toSorted() in the clinician sorting flow with a
compatibility-safe copied sort, preserving the original clinicianArray order
while producing the same sortedArray result; do not rely on a toSorted polyfill.
Comment on lines
+6
to
+7
| getCliniciansForClinic: builder.query({ | ||
| query: (clinicId) => `clinics/${clinicId}/clinicians?limit=1000&offset=0`, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for other clinician-roster fetches and any existing pagination handling.
rg -n -C 4 'clinicians\?limit|fetchCliniciansFromClinic|limit=1000' --type=js -g '!node_modules'Repository: tidepool-org/blip
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n app/redux/features/clinicians/cliniciansApi.js
printf '%s\n' '--- clinic admin references ---'
rg -n -C 8 'Security|CSV|clinician|Clinician' app/pages/clinicadmin/clinicadmin.js
printf '%s\n' '--- API and pagination references ---'
rg -n -C 5 'getCliniciansForClinic|clinicians|pagination|offset|limit' app/redux app/pages/clinicadmin --glob '*.{js,jsx,ts,tsx}'Repository: tidepool-org/blip
Length of output: 49080
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- RTK hook usage and endpoint tests ---'
rg -n -C 5 'useGetCliniciansForClinicQuery|getCliniciansForClinic' app test tests __tests__ 2>/dev/null || true
printf '%s\n' '--- clinician and clinic size limits ---'
rg -ni -C 4 'clinician.{0,30}(limit|max|cap)|(?:limit|max|cap).{0,30}clinician|clinic.{0,30}(limit|max|cap)|(?:limit|max|cap).{0,30}clinic' --glob '*.{js,jsx,json,md,yml,yaml}' --glob '!node_modules' . || true
printf '%s\n' '--- API client implementation and tests ---'
rg -n -C 8 'getCliniciansFromClinic|clinics/.+clinicians|FETCH_CLINICIANS_FROM_CLINIC' app tests test __tests__ 2>/dev/null || true
printf '%s\n' '--- relevant tracked files ---'
git ls-files | rg 'clinician|clinicadmin|api|package.json|README|test' | head -200Repository: tidepool-org/blip
Length of output: 50374
🌐 Web query:
Tidepool platform clinic maximum number of clinicians clinic clinician roster API limit=1000
💡 Result:
The Tidepool platform does not document a specific, hard-coded API limit of 1000 clinicians per clinic roster. While Tidepool APIs utilize pagination parameters (such as 'limit' and 'offset') for retrieving lists of resources like clinics, patients, and staff members [1][2], these parameters are typically used to manage the size of individual API responses rather than enforcing a rigid cap on the total number of clinicians a clinic can host. Technical documentation for the Tidepool Clinic service indicates that client applications must handle pagination when querying clinic members (administrators and clinicians) using the clinic ID [1]. Developers are encouraged to use the limit and offset parameters to fetch roster data in pages [1]. While Tidepool does impose specific limits in other areas, such as a patient count limit of 250 for the no-cost Tidepool+ Base offering [3], there is no public technical evidence specifying a maximum roster size for clinician accounts. For production integration needs, Tidepool recommends that developers contact them directly or join their public open-source Slack channel for clarification on API constraints and best practices [4][5].
Citations:
- 1: https://tidepool.redocly.app/docs/clinic-integration
- 2: tidepool-org/platform-client@79f692a
- 3: https://support.tidepool.org/hc/en-us/articles/23771490686228-Tidepool-Patient-Limits
- 4: https://tidepool.redocly.app/
- 5: https://dev-docs.tidepool.org/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact target consumers ---'
rg -n -C 6 'useGetCliniciansForClinicQuery|cliniciansApi' app test/unit/app/providers/AppBanner test/unit/app/providers/AppBanner 2>/dev/null
printf '%s\n' '--- legacy clinic-admin fetch path ---'
sed -n '1878,1912p' app/redux/actions/async.js
sed -n '724,754p' app/redux/reducers/misc.js
sed -n '590,675p' test/unit/pages/clinicadmin.test.js
printf '%s\n' '--- client source locations ---'
git ls-files | rg '(^|/)(platform-client|api).*(clinic|clinician)|clinics.*\\.js$|package\\.json$' | head -100
rg -n -C 4 'getCliniciansFromClinic' --glob '!node_modules' --glob '!dist' --glob '!build' . | head -120Repository: tidepool-org/blip
Length of output: 34763
Paginate the clinician roster query.
AppBannerProvider treats the response as complete when it counts clinic admins. The platform does not define a 1,000-clinician cap and requires clients to handle pagination. Otherwise, an admin beyond the first 1,000 records can be missed and the sole-admin exemption can be applied incorrectly.
🧰 Tools
🪛 ast-grep (0.45.1)
[error] 5-7: Avoid SQL injection
Context: builder.query({
query: (clinicId) => clinics/${clinicId}/clinicians?limit=1000&offset=0,
})
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-javascript)
🤖 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/redux/features/clinicians/cliniciansApi.js` around lines 6 - 7, Update
getCliniciansForClinic to support fetching the complete paginated clinician
roster instead of hard-coding limit=1000 and offset=0. Use the API’s pagination
mechanism to request subsequent pages and aggregate their results before
returning them, preserving the response shape expected by AppBannerProvider so
clinic admins beyond the first page are included.
|
|
||
| // Stubs the RTK Query hook for suites on redux-mock-store (no api middleware). Defaults to no | ||
| // data; override per-test with useGetMfaStatusQuery.mockReturnValue({ data }). | ||
| export const useGetMfaStatusQuery = jest.fn(() => ({ data: undefined })); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Return a fully shaped query result from the mock.
The consumer at app/pages/userprofile/userprofile.js Line 597 destructures data, isLoading, isFetching, isError, and refetch. This mock supplies only data. Two effects follow in any suite that renders the profile page with this mock:
refetchisundefined, sohandleRetryMfaStatusthrows a TypeError when a test clicks "Re-fetch 2FA Status".loadinganderrorareundefined, which violates thePropTypes.bool.isRequireddeclarations onTwoFactorRowand emits PropTypes warnings.
🛠️ Proposed fix
-export const useGetMfaStatusQuery = jest.fn(() => ({ data: undefined }));
+export const useGetMfaStatusQuery = jest.fn(() => ({
+ data: undefined,
+ isLoading: false,
+ isFetching: false,
+ isError: false,
+ refetch: jest.fn(),
+}));📝 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.
Suggested change
| export const useGetMfaStatusQuery = jest.fn(() => ({ data: undefined })); | |
| export const useGetMfaStatusQuery = jest.fn(() => ({ | |
| data: undefined, | |
| isLoading: false, | |
| isFetching: false, | |
| isError: false, | |
| refetch: jest.fn(), | |
| })); |
🤖 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/redux/features/mfaStatus/__mocks__/mfaStatusApi.js` at line 5, Update the
useGetMfaStatusQuery mock to return all fields destructured by the user profile
consumer: data, boolean isLoading, boolean isFetching, boolean isError, and a
callable refetch function. Use safe default values so retry interactions do not
throw and required boolean props receive valid values.
Comment on lines
+11
to
+28
| queryFn: async (formValues, { getState }) => { | ||
| const { blip: { loggedInUserId, allUsersMap } } = getState(); | ||
| const loggedInUser = allUsersMap[loggedInUserId]; | ||
|
|
||
| const newUser = assign( | ||
| {}, | ||
| omit(loggedInUser, ['profile', 'preferences']), | ||
| omit(formValues, ['profile', 'preferences']), | ||
| { | ||
| profile: assign({}, loggedInUser.profile, formValues.profile), | ||
| preferences: assign({}, loggedInUser.preferences, formValues.preferences), | ||
| } | ||
| ); | ||
|
|
||
| let userUpdates = cloneDeep(newUser); | ||
| if (userUpdates.username === loggedInUser.username) { | ||
| userUpdates = omit(userUpdates, 'username', 'emails'); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against a missing logged-in user before the merge.
allUsersMap[loggedInUserId] can be undefined — for example when the session ends while the dialog is open, or when loggedInUserId is not yet populated. Line 20 then dereferences loggedInUser.profile and throws a TypeError inside queryFn. The throw escapes the try block at line 30, so the mutation rejects with a non-normalized error and the dialog's catch shows no useful message.
Return a normalized error instead.
🛡️ Proposed fix
const { blip: { loggedInUserId, allUsersMap } } = getState();
const loggedInUser = allUsersMap[loggedInUserId];
+ if (!loggedInUser) {
+ return { error: { status: 'CUSTOM_ERROR', data: ErrorMessages.ERR_UPDATING_USER } };
+ }
+
const newUser = assign(📝 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.
Suggested change
| queryFn: async (formValues, { getState }) => { | |
| const { blip: { loggedInUserId, allUsersMap } } = getState(); | |
| const loggedInUser = allUsersMap[loggedInUserId]; | |
| const newUser = assign( | |
| {}, | |
| omit(loggedInUser, ['profile', 'preferences']), | |
| omit(formValues, ['profile', 'preferences']), | |
| { | |
| profile: assign({}, loggedInUser.profile, formValues.profile), | |
| preferences: assign({}, loggedInUser.preferences, formValues.preferences), | |
| } | |
| ); | |
| let userUpdates = cloneDeep(newUser); | |
| if (userUpdates.username === loggedInUser.username) { | |
| userUpdates = omit(userUpdates, 'username', 'emails'); | |
| } | |
| queryFn: async (formValues, { getState }) => { | |
| const { blip: { loggedInUserId, allUsersMap } } = getState(); | |
| const loggedInUser = allUsersMap[loggedInUserId]; | |
| if (!loggedInUser) { | |
| return { error: { status: 'CUSTOM_ERROR', data: ErrorMessages.ERR_UPDATING_USER } }; | |
| } | |
| const newUser = assign( | |
| {}, | |
| omit(loggedInUser, ['profile', 'preferences']), | |
| omit(formValues, ['profile', 'preferences']), | |
| { | |
| profile: assign({}, loggedInUser.profile, formValues.profile), | |
| preferences: assign({}, loggedInUser.preferences, formValues.preferences), | |
| } | |
| ); | |
| let userUpdates = cloneDeep(newUser); | |
| if (userUpdates.username === loggedInUser.username) { | |
| userUpdates = omit(userUpdates, 'username', 'emails'); | |
| } |
🤖 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/redux/features/userProfile/userProfileApi.js` around lines 11 - 28, Guard
the loggedInUser lookup in queryFn before constructing newUser, and when it is
missing, return the normalized error shape expected by this mutation instead of
dereferencing profile or allowing a TypeError to escape. Preserve the existing
merge and update behavior when loggedInUser exists.
| }, | ||
| "packageManager": "yarn@3.6.4", | ||
| "version": "1.100.0", | ||
| "version": "1.101.0-rc.1", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the stable release version.
The PR objective is to release 1.101.0 to master, but package.json declares 1.101.0-rc.1. This produces a prerelease artifact. Set the version to 1.101.0 before the final release, or update the release objective if this is intentionally an RC.
🤖 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 `@package.json` at line 7, Update the package version declaration from the
prerelease value to the stable release version 1.101.0, ensuring the release
artifact is not marked as an RC.
Comment on lines
+32
to
+35
| jest.mock('../../../../../app/redux/features/mfaStatus/mfaStatusApi'); | ||
| import { useGetMfaStatusQuery } from '../../../../../app/redux/features/mfaStatus/mfaStatusApi'; | ||
| jest.mock('../../../../../app/redux/features/clinicians/cliniciansApi'); | ||
| import { useGetCliniciansForClinicQuery } from '../../../../../app/redux/features/clinicians/cliniciansApi'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the legacy test mocking style.
Replace the new jest.mock() and Jest mock-return configuration with sinon.stub() calls. Reset each stub in beforeEach.
test/unit/app/providers/AppBanner/AppBannerProvider.test.js#L32-L35: Stub the RTK Query hook exports with Sinon.test/unit/app/providers/AppBanner/AppBannerProvider.test.js#L106-L110: Configure and reset the Sinon stubs inbeforeEach.test/unit/app/app.test.js#L16-L17: Replace the Jest module mocks with Sinon stubs.test/unit/app/providers/AppBanner/AppBanner.test.js#L24-L29: Replace the Jest module mocks with Sinon stubs.
As per coding guidelines: “In Karma/Mocha tests, use sinon.stub() for mocks, reset stubs in beforeEach, and follow the legacy testing style already used in test/.”
📍 Affects 3 files
test/unit/app/providers/AppBanner/AppBannerProvider.test.js#L32-L35(this comment)test/unit/app/providers/AppBanner/AppBannerProvider.test.js#L106-L110test/unit/app/app.test.js#L16-L17test/unit/app/providers/AppBanner/AppBanner.test.js#L24-L29
🤖 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 `@test/unit/app/providers/AppBanner/AppBannerProvider.test.js` around lines 32
- 35, Replace the Jest module mocks and mock-return configuration for the RTK
Query hooks with Sinon stubs, preserving the existing test behavior. In
test/unit/app/providers/AppBanner/AppBannerProvider.test.js at lines 32-35 and
106-110, stub useGetMfaStatusQuery and useGetCliniciansForClinicQuery and
reset/configure them in beforeEach; make the corresponding Jest-to-Sinon
replacements in test/unit/app/app.test.js lines 16-17 and
test/unit/app/providers/AppBanner/AppBanner.test.js lines 24-29, following the
legacy test style.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release 1.101.0 to master