Skip to content

[WEB-4663] Consolidate clinic Settings tab with combined save#5

Open
clintonium-119 wants to merge 2 commits into
feat/device-names-and-settings-historyfrom
feat/updated-clinic-settings-design
Open

[WEB-4663] Consolidate clinic Settings tab with combined save#5
clintonium-119 wants to merge 2 commits into
feat/device-names-and-settings-historyfrom
feat/updated-clinic-settings-design

Conversation

@clintonium-119

@clintonium-119 clintonium-119 commented Jul 17, 2026

Copy link
Copy Markdown
Member

WEB-4663

I noticed that this element got missed when I did the theme updates. It necessitates a bit of a logic shift on submission, since the UI groups a bunch of fields into one form that hit disparate endpoints. But the UI is much better, so I think it's worth the code shuffle.

Replace the five per-setting panels (each with its own inline
save/cancel) with one "Clinic Settings" panel of hairline setting rows
— Tier, Patient Limit, Timezone, Require MRN, Unique MRN — and a single
Reset / Save Changes footer, mirroring UserActions' one-wrapper pattern.
The Delete Clinic danger panel and confirmation modal stay separate; the
share code stays in the profile header.

Add a presentational SettingRow primitive for the hairline rows.

Add a combined updateClinicSettings route action that persists only the
changed fields (aggregating per-field partial failures instead of
reporting blanket success), and replace the four per-setting on*Update
callbacks and their now-dead server branches with one
onSaveClinicSettings
contract fired once on save.

Patient Limit: collapse the old "Limit Applied" toggle into the input's
empty state (empty = no limit), add a "Set default (250)" button, and
gate the control on US-based tier0100 clinics only — keyed off the
staged
tier and clinic country so a limit is never set for an ineligible
clinic.
Tolerate the legacy hardLimit.patientCount field on read (renamed to
plan), mirroring blip; only ever write plan.

Show the clinic address as a single-line field in the profile header
details.

Add tests for SettingRow and the consolidated Settings tab.
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: consolidating the clinic Settings tab with a combined save flow.
Description check ✅ Passed The description is related to the settings-tab refactor and combined submission flow, even if brief.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/updated-clinic-settings-design

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/components/Clinic/ClinicProfile.test.tsx`:
- Around line 122-163: Extend the ClinicProfile patient-limit tests around the
existing “Set default” and legacy patientCount cases to verify that clearing the
Maximum patients field submits null. Add coverage showing decimal,
exponent-notation, and negative inputs cannot be saved as truncated limits or
interpreted as removals, using the existing save interaction and request
assertions.

In `@app/routes/clinics`.$clinicId.tsx:
- Around line 443-448: Update the MRN settings handling in the record callback
around MrnSettingsSchema.parse so missing fields do not become false: require
both mrnRequired and mrnUnique to be present or retrieve the existing value for
any missing field, and reject submissions whose values are not the literal
strings "true" or "false".
- Around line 456-472: The patient-limit flow must consistently reject malformed
values and prevent writes for ineligible clinics or failed dependent tier
updates. In app/routes/clinics.$clinicId.tsx lines 456-472, validate that the
effective clinic is US-based tier0100, require an exact non-negative integer or
intentional blank, and skip the patient-limit write when the tier change fails.
In app/components/Clinic/ClinicProfile.tsx lines 310-315, convert only blank
input to null and reject decimal, negative, exponent-form, or otherwise
malformed values. In app/components/Clinic/ClinicProfile.test.tsx lines 122-163,
add regression coverage for blank removal, malformed numbers, and ineligible
clinics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d5ef1675-48e1-446f-a13b-55c4984e3a3f

📥 Commits

Reviewing files that changed from the base of the PR and between 6bceb50 and be06892.

📒 Files selected for processing (6)
  • app/components/Clinic/ClinicProfile.test.tsx
  • app/components/Clinic/ClinicProfile.tsx
  • app/components/Clinic/types.ts
  • app/components/ui/SettingRow.test.tsx
  • app/components/ui/SettingRow.tsx
  • app/routes/clinics.$clinicId.tsx

Comment thread app/components/Clinic/ClinicProfile.test.tsx
Comment thread app/routes/clinics.$clinicId.tsx
Comment thread app/routes/clinics.$clinicId.tsx
Address data-integrity findings on the combined clinic Settings save:

- MRN settings: require both mrnRequired and mrnUnique and accept only
  the literal 'true'/'false' at the action boundary, so a partial
  submit can no longer coerce an omitted flag to false and silently
  clear it (the endpoint replaces the whole settings object).
- Patient limit (UI): treat only a blank value as a removal; block the
  save and show an inline error for decimal, negative, exponent, or
  otherwise malformed input instead of truncating via parseInt or
  degrading invalid input to a null removal.
- Patient limit (action): reject non-blank values that aren't an exact
  non-negative integer, and skip the write when a tier change in the
  same submit failed, so a limit is never applied to a clinic that kept
  an ineligible tier.

Add regression tests for blank removal, valid integer save, and
decimal/negative inputs blocking the save.
@clintonium-119
clintonium-119 requested a review from krystophv July 17, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant