[ORCA-76] Bound the clinic-scoped recent-entity cookie key count and size - #7
Merged
clintonium-119 merged 7 commits intoJul 31, 2026
Merged
Conversation
The __patients_session, __clinicians_session and __prescriptions_session cookies are keyed per clinic and nothing ever removed a key, so they grew with every clinic whose patient, clinician or prescription pages were opened. React Router rejects any single cookie over 4096 bytes by throwing, which surfaces as a 500 from the affected loader; that is reachable at roughly three clinics' worth of entries. Prune keys for clinics that have fallen out of the __clinics cookie. Only the three child loaders create these keys, so pruning in those loaders is sufficient and avoids two loaders writing the same cookie in one request. The keep set matches the clinic list action.recent-entities already iterates, so only data that was already unreachable is dropped.
📝 WalkthroughSummary by CodeRabbit
WalkthroughShared helpers centralize clinic-scoped recent patients, clinicians, and prescriptions with bounded retention and cookie-size fallback. Clinic routes use the shared session flow, recent clinician data is narrowed, and clinic settings updates are consolidated into one action. ChangesClinic route unification
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ClinicEntityRoute
participant RecentEntityHelpers
participant RecentEntitySession
ClinicEntityRoute->>RecentEntityHelpers: readClinicScopedList(session, prefix, clinicId)
RecentEntityHelpers->>RecentEntitySession: read prefix-clinicId value
RecentEntitySession-->>ClinicEntityRoute: recent entities
ClinicEntityRoute->>RecentEntityHelpers: writeClinicScopedList(session, prefix, clinicId, entries)
ClinicEntityRoute->>RecentEntityHelpers: commitClinicScopedSession(...)
RecentEntityHelpers-->>ClinicEntityRoute: committed cookie
Possibly related PRs
🚥 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
|
Unassigned for review. I'm going to make a few more changes before re-assigning this one for review. |
Viewing ten clinicians in one clinic and then a second 500s the clinician details page until the user clears cookies. React Router throws rather than truncating past 4096 bytes, and the loader does not catch it. Cut what each entry costs. RecentClinician carried roles and lastViewedAt, which nothing renders, and the clinician and prescription lists were stored as JSON strings inside a JSON session, doubling every escape. Storing trimmed arrays takes one clinic's ten clinicians from 2769 to 1605 bytes, so ten across two clinics now fits where it prev Prune keys for clinics no longmmit time, and shed further if the commit still will not fit. Pruning alone is not enough: the limitell before recentClinicsMax binds. action.recent-entities only reads clinics still in __clinics, soble weight on every request, patient names and emails included. readClinicScopedList replaces four hand-rolled readers so all three child routes read and write th Recents written in the previourefill through normal use; these cookies expire after three days anyway.
clintonium-119
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/routes/clinics`.$clinicId.clinicians.$clinicianId.tsx:
- Around line 131-146: Replace the hardcoded `recentClinicians-${clinicId}` key
in the session update within the route handler with the existing
`clinicScopedPrefixes.clinicians` value, preserving the same clinic-specific
session behavior and keeping it consistent with `readClinicScopedList` and
`commitClinicScopedSession`.
In `@app/routes/clinics`.$clinicId.patients.$patientId.tsx:
- Around line 388-399: Replace the hardcoded `patients-${clinicId}` key in the
`recentlyViewed.set` call with the imported `clinicScopedPrefixes.patients`
constant, preserving the existing clinic-scoped key construction and response
behavior.
In `@app/routes/clinics`.$clinicId.prescriptions.$prescriptionId.tsx:
- Line 97: Update the session write in the prescription route to build its key
with the imported clinicScopedPrefixes.prescriptions constant instead of the
duplicated “recentPrescriptions” literal, matching the existing read and commit
key construction.
In `@app/utils/recentEntities.server.ts`:
- Around line 90-108: Update commitClinicScopedSession so its catch block only
falls through to the next pruning attempt when the error matches the pinned
React Router cookie-size message using the specified regular-expression check;
rethrow all other errors immediately so signing, configuration, and storage
failures propagate without further pruning.
🪄 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: 476d8c94-f60c-4b2b-8e3c-00b330565806
📒 Files selected for processing (9)
app/components/Clinic/RecentItemsContext.test.tsxapp/components/Clinic/types.tsapp/routes/action.recent-entities.tsapp/routes/clinics.$clinicId.clinicians.$clinicianId.tsxapp/routes/clinics.$clinicId.patients.$patientId.tsxapp/routes/clinics.$clinicId.prescriptions.$prescriptionId.tsxapp/routes/clinics.$clinicId.tsxapp/utils/recentEntities.server.test.tsapp/utils/recentEntities.server.ts
💤 Files with no reviewable changes (1)
- app/components/Clinic/RecentItemsContext.test.tsx
clintonium-119
requested review from
krystophv
and removed request for
krystophv
July 28, 2026 21:40
Three loaders wrote `${prefix}-${clinicId}` as a literal while
importing clinicScopedPrefixes for their read and commit calls.
Add writeClinicScopedList alongside the reader, both keyed by a
single private clinicScopedKey, so the key shape lives in one
place.
Narrow the commitClinicScopedSession catch to React Router's
cookie-size error. Every exception previously read as "too large",
so a signing or config failure ran all three shed attempts and
discarded other clinics' recents before rethrowing the same error
anyway.
commitSession is cookie.serialize plus a length check, so the shed ladder can serialize a candidate and measure it directly rather than committing and classifying the thrown error. Matching on React Router's "Cookie length will exceed browser maximum" message is gone, along with the try/catch. Build the three recents cookies with createCookie and export them so the ladder has something to serialize; createCookieSessionStorage accepts a Cookie in place of an options object, so the storages are byte-identical. Where shedding everything still won't fit, return the oversized value rather than throwing: the browser drops the cookie and the page renders, where the throw would restore the 500 this path exists to prevent.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/sessions.server.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: extract a shared factory for the three clinic-scoped cookie/session pairs.
The three blocks are structurally identical apart from names. A small helper would remove the repetition and make adding future clinic-scoped entity types (or changing the shared cookie options) a one-line change.
♻️ Example refactor
+function createClinicScopedCookieSession(name: string) { + const cookie = createCookie(name, dataCookieOptions); + return { cookie, session: createCookieSessionStorage({ cookie }) }; +} + -export const patientsCookie = createCookie( - '__patients_session', - dataCookieOptions, -); -export const patientsSession = createCookieSessionStorage({ - cookie: patientsCookie, -}); - -export const cliniciansCookie = createCookie( - '__clinicians_session', - dataCookieOptions, -); -export const cliniciansSession = createCookieSessionStorage({ - cookie: cliniciansCookie, -}); - -export const prescriptionsCookie = createCookie( - '__prescriptions_session', - dataCookieOptions, -); -export const prescriptionsSession = createCookieSessionStorage({ - cookie: prescriptionsCookie, -}); +export const { cookie: patientsCookie, session: patientsSession } = + createClinicScopedCookieSession('__patients_session'); +export const { cookie: cliniciansCookie, session: cliniciansSession } = + createClinicScopedCookieSession('__clinicians_session'); +export const { cookie: prescriptionsCookie, session: prescriptionsSession } = + createClinicScopedCookieSession('__prescriptions_session');Also applies to: 55-80
🤖 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/sessions.server.ts` at line 1, Optionally extract a shared factory near the clinic-scoped session setup that creates each cookie/session pair from the entity-specific name while applying the common cookie options. Replace the three duplicated blocks with calls to this factory, preserving their existing exported names and behavior so future clinic-scoped pairs can be added through the same helper.
🤖 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/sessions.server.ts`:
- Line 1: Optionally extract a shared factory near the clinic-scoped session
setup that creates each cookie/session pair from the entity-specific name while
applying the common cookie options. Replace the three duplicated blocks with
calls to this factory, preserving their existing exported names and behavior so
future clinic-scoped pairs can be added through the same helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a27184e-6f0e-44da-b22f-b418c5f70169
📒 Files selected for processing (6)
app/routes/clinics.$clinicId.clinicians.$clinicianId.tsxapp/routes/clinics.$clinicId.patients.$patientId.tsxapp/routes/clinics.$clinicId.prescriptions.$prescriptionId.tsxapp/sessions.server.tsapp/utils/recentEntities.server.test.tsapp/utils/recentEntities.server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/routes/clinics.$clinicId.prescriptions.$prescriptionId.tsx
- app/routes/clinics.$clinicId.clinicians.$clinicianId.tsx
- app/routes/clinics.$clinicId.patients.$patientId.tsx
krystophv
reviewed
Jul 29, 2026
| } | ||
|
|
||
| /** Browsers reject cookies past this; React Router throws rather than truncate. */ | ||
| export const maxCookieBytes = 4096; |
Member
There was a problem hiding this comment.
nit: this is defined/exported but not consistently used in the tests (which redefine the 'magic' constant)
Member
Author
There was a problem hiding this comment.
nit: this is defined/exported but not consistently used in the tests (which redefine the 'magic' constant)
Good call. Update in commit 2d3e10e
- Replace hardcoded 4096 byte limit with recentEntities.maxCookieBytes constant in cookie size assertions - Simplify clinicScopedPrefixes.clinicians reference to direct string 'recentClinicians' for clearer test setup - Update test description to be agnostic of specific byte limit, improving maintainability as implementation details evolve - Format multi-line assertions for readability
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/clinics.$clinicId.tsx (1)
54-59: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winImport
ValidationErrorandgetErrorMessagebefore using them.
updateClinicSettingscallsgetErrorMessage(error)and throwsnew ValidationError(...)inClinic::$clinicId.tsx, but the file only importserrorResponseandAPIErrorfrom~/utils/errors. Add these to the import so the action compiles and handles invalid input.🐛 Proposed fix
-import { errorResponse, APIError } from '~/utils/errors'; +import { + errorResponse, + APIError, + ValidationError, + getErrorMessage, +} from '~/utils/errors';🤖 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/routes/clinics`.$clinicId.tsx around lines 54 - 59, Update the errors import in updateClinicSettings to also include ValidationError and getErrorMessage from ~/utils/errors, preserving the existing errorResponse and APIError imports so the action compiles and handles invalid input.
🧹 Nitpick comments (3)
app/components/Clinic/types.ts (1)
43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
PatientCountLimitinstead of re-declaring its shape in the loader.
app/routes/clinics.$clinicId.tsxLines 217-220 inline{ plan?: number; patientCount?: number }forhardLimit/softLimit, duplicating this type. Casting to{ hardLimit?: PatientCountLimit; softLimit?: PatientCountLimit }keeps the legacy-field contract in one place.🤖 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/Clinic/types.ts` around lines 43 - 46, Update the hardLimit/softLimit typing in the clinic loader to use the existing PatientCountLimit type for both fields instead of the inline `{ plan?: number; patientCount?: number }` shape, preserving the legacy patientCount fallback contract defined by PatientCountLimit.app/utils/recentEntities.server.test.ts (1)
298-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
recentEntities.clinicScopedPrefixes.cliniciansover the literal.Hardcoding
'recentClinicians'here means a prefix rename keeps this test green while silently exercising a different keyspace. This test previously used the exported constant.🤖 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/utils/recentEntities.server.test.ts` at line 298, Update the test case around the 'recentClinicians' key to use recentEntities.clinicScopedPrefixes.clinicians instead of the hardcoded literal, preserving the test’s existing behavior while ensuring it exercises the exported prefix.app/routes/clinics.$clinicId.tsx (1)
496-505: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPartial failures are reported as a blanket 500, hiding what actually succeeded.
If tier saves but timezone fails, the client only sees
Failed to update: timezonewith status 500, and the toast handler treats it purely as an error — the user has no signal that tier was applied. Validation failures (malformed patient limit, missing MRN flag) also surface as 500 rather than 400.Consider returning the per-field
resultsarray (and a 400 when all failures areValidationError) so the UI can render mixed outcomes.🤖 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/routes/clinics`.$clinicId.tsx around lines 496 - 505, Update the response handling around the per-field results array so partial updates return the results for every field, allowing the client to distinguish successful and failed saves instead of receiving only a blanket error. Return HTTP 400 when all failed results are ValidationError instances, while preserving an appropriate server-error status for non-validation failures and the existing success response when no fields fail.
🤖 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.
Outside diff comments:
In `@app/routes/clinics`.$clinicId.tsx:
- Around line 54-59: Update the errors import in updateClinicSettings to also
include ValidationError and getErrorMessage from ~/utils/errors, preserving the
existing errorResponse and APIError imports so the action compiles and handles
invalid input.
---
Nitpick comments:
In `@app/components/Clinic/types.ts`:
- Around line 43-46: Update the hardLimit/softLimit typing in the clinic loader
to use the existing PatientCountLimit type for both fields instead of the inline
`{ plan?: number; patientCount?: number }` shape, preserving the legacy
patientCount fallback contract defined by PatientCountLimit.
In `@app/routes/clinics`.$clinicId.tsx:
- Around line 496-505: Update the response handling around the per-field results
array so partial updates return the results for every field, allowing the client
to distinguish successful and failed saves instead of receiving only a blanket
error. Return HTTP 400 when all failed results are ValidationError instances,
while preserving an appropriate server-error status for non-validation failures
and the existing success response when no fields fail.
In `@app/utils/recentEntities.server.test.ts`:
- Line 298: Update the test case around the 'recentClinicians' key to use
recentEntities.clinicScopedPrefixes.clinicians instead of the hardcoded literal,
preserving the test’s existing behavior while ensuring it exercises the exported
prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 720a56db-1996-4401-947c-b117fc1ee7ed
📒 Files selected for processing (3)
app/components/Clinic/types.tsapp/routes/clinics.$clinicId.tsxapp/utils/recentEntities.server.test.ts
This was referenced Jul 31, 2026
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
2 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.
ORCA-76
Fixes a reproducible 500: view 10 clinicians in one clinic, then a second clinic, and the clinician details pages eventually start to return 500s until cookies are cleared. React Router throws rather than truncating past 4096 bytes per cookie, and the loader doesn't catch it.
Fixes applied:
Past three clinics the cookie still exceeds the limit — unavoidable at this volume — but it no longer 500s. The commit sheds other clinics' entries and keeps the clinic being viewed, so the cost is a shorter recents list instead of a broken page.
Size alone wasn't enough, and pruning alone wouldn't have helped either: the byte limit is reached at 2–3 clinics, well before
recentClinicsMax(10) binds, so a prune-only change would never engage.