Offline support gsoc phase1 caching #14901
Conversation
…gsoc_phase1_caching
…gsoc_phase1_caching
|
WalkthroughThis PR adds comprehensive offline support to a React Query-based application. Changes include IndexedDB caching infrastructure (Dexie), offline write queuing system, conflict detection, dependency resolution, network status monitoring, and synchronization workflows. App-wide persistence is enabled via React Query configuration and persister factory. Multiple components are updated to support offline/online flows with queued mutations. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 117
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (25)
src/pages/Organization/components/OrganizationLayout.tsx (1)
63-70: AddnetworkMode: "online"to align with the offline support pattern.The addition of
meta: { persist: true }correctly enables persistence for organization data. However, according to the PR objectives, queries should also explicitly setnetworkMode: "online"to prevent background retries when offline.🔎 Proposed fix
const { data: org, isLoading } = useQuery({ queryKey: ["organization", id], queryFn: query(organizationApi.get, { pathParams: { id }, }), meta: { persist: true }, + networkMode: "online", enabled: !!id, });src/pages/Organization/components/GovtOrganizationSelector.tsx (1)
142-172: Critical: Race condition with async state update invalidates requiredDepth check.Line 158 checks
selectedLevels.length >= requiredDepth, butselectedLevelsstill holds the OLD state becausesetSelectedLevels(lines 148-152) queues an async update. The check runs before the state update completes, causing incorrect validation.Example failure:
- User selects first level (index=0) when
requiredDepth=1- Lines 148-152 queue state update to length 1
- Line 158 checks OLD length (0) >= 1 → FALSE
- Incorrectly calls
onChange("")andsetSelectedOrganization(null)Fix: Use the computed new length
index + 1instead of the stale state length.🔎 Proposed fix
if (!required || (required && !organization.has_children)) { onChange(organization.id); setSelectedOrganization?.(organization); } else if ( requiredDepth != null && - selectedLevels.length >= requiredDepth + index + 1 >= requiredDepth ) { onChange(organization.id); setSelectedOrganization?.(organization); } else {src/pages/Appointments/components/PractitionerSelector.tsx (5)
92-109: AddnetworkMode: "online"to prevent background retries when offline.Per the PR description, queries with
meta: { persist: true }should also setnetworkMode: "online"to avoid background retries when offline. This query will attempt to refetch in the background when offline without this configuration.🔎 Proposed fix
const { data: organizationsResponse } = useQuery({ queryKey: ["facilityOrganizations", facilityId, showAllOrgs], queryFn: query( showAllOrgs ? facilityOrganizationApi.list : facilityOrganizationApi.listMine, { pathParams: { facilityId }, queryParams: { parent: "", active: true, limit: 100, }, }, ), meta: { persist: true }, + networkMode: "online", enabled: open, });
117-148: AddnetworkMode: "online"to prevent background retries when offline.This query also needs
networkMode: "online"to align with the PR's offline strategy and prevent unnecessary retry attempts when offline.🔎 Proposed fix
const { data: organizationUsers, isLoading: isLoadingOrganizationUsers } = useQuery({ queryKey: ["organizationUsers", facilityId, currentOrganizationId], queryFn: async ({ signal }) => { // Try availableUsers API with organization filter first try { const response = await query( scheduleApi.appointments.availableUsers, { pathParams: { facilityId }, queryParams: { organization_ids: currentOrganizationId! }, }, )({ signal }); return response; } catch (_error) { // Fallback to facilityOrganizationApi.listUsers if availableUsers doesn't support organization filter const response = await query(facilityOrganizationApi.listUsers, { pathParams: { facilityId, organizationId: currentOrganizationId!, }, queryParams: { limit: 1000 }, })({ signal }); // Transform the response to match availableUsers format return { users: response.results.map((userRole) => userRole.user), }; } }, meta: { persist: true }, + networkMode: "online", enabled: !!currentOrganizationId, });
151-159: AddnetworkMode: "online"to prevent background retries when offline.This search query also requires
networkMode: "online"to complete the offline support pattern.🔎 Proposed fix
const { data: allPractitioners } = useQuery({ queryKey: ["allPractitioners", facilityId, searchQuery], queryFn: query(scheduleApi.appointments.availableUsers, { pathParams: { facilityId }, queryParams: { limit: 10 }, }), meta: { persist: true }, + networkMode: "online", enabled: open && !!searchQuery && searchQuery.length > 0, });
162-173: AddnetworkMode: "online"to prevent background retries when offline.This child organizations query also needs
networkMode: "online"to prevent background retries when offline.🔎 Proposed fix
const { data: childOrganizations } = useQuery({ queryKey: ["childOrganizations", facilityId, currentOrganizationId], queryFn: query(facilityOrganizationApi.list, { pathParams: { facilityId }, queryParams: { parent: currentOrganizationId!, active: true, }, }), meta: { persist: true }, + networkMode: "online", enabled: !!currentOrganizationId, });
92-173: ConfigurestaleTimefor organizational and user data to prevent stale practitioner information.These queries persist organizational structure and practitioner lists without any staleness configuration. In a healthcare setting, cached data showing removed practitioners, reassigned users, or outdated department structures could lead to incorrect appointment bookings.
Consider adding
staleTimeconfiguration to ensure cached data is revalidated appropriately, and implement a cache invalidation strategy when organizational changes occur (e.g., practitioner removed, department restructured).Example configuration approach:
const { data: organizationsResponse } = useQuery({ queryKey: ["facilityOrganizations", facilityId, showAllOrgs], queryFn: query(/* ... */), meta: { persist: true }, networkMode: "online", staleTime: 1000 * 60 * 15, // 15 minutes enabled: open, });Additionally, consider implementing cache invalidation when:
- A practitioner is added/removed from the system
- Organizational structure changes
- User returns online after offline period (revalidate all persisted data)
src/components/Patient/LinkDepartmentsSheet.tsx (3)
158-163: Add fallback error handling for consistency.The error handling in
DeleteOrganizationButtondirectly displays API error messages without a fallback, unlike the batch mutation error handler (lines 218-242) which now includes an i18n fallback. Consider adding similar fallback error handling for consistency.🔎 Suggested improvement
onError: (error) => { - const errorData = error.cause as { errors: { msg: string }[] }; - errorData.errors.forEach((er) => { - toast.error(er.msg); - }); + try { + const errorData = error.cause as { errors?: { msg: string }[] }; + if (errorData.errors?.length) { + errorData.errors.forEach((er) => { + toast.error(er.msg); + }); + } else { + toast.error(t("an_unexpected_error_occurred")); + } + } catch { + toast.error(t("an_unexpected_error_occurred")); + } },
218-242: Error handling is functional but could benefit from better type safety.The error extraction logic handles multiple error response structures, which is good for robustness. However, the type assertion
error.cause as { results?: ... }is fragile and could lead to runtime issues if the error structure changes.Consider defining a dedicated type for the batch error response structure:
interface BatchErrorResponse { results?: Array<{ data?: { detail?: string; errors?: Array<{ msg: string }>; }; }>; }Then use it in the error handler:
const errorData = error.cause as BatchErrorResponse;This makes the expected error structure explicit and easier to maintain.
255-276: Manual path parameter replacement is fragile and error-prone.The manual string replacement for path parameters (lines 257-260) bypasses type-safe parameter handling and could break if route templates change. This pattern differs from the standard mutation calls elsewhere (e.g., line 142-145).
Consider investigating whether the batch API or mutation utilities provide a helper for constructing batch requests with proper path parameter resolution. If not available, consider creating a reusable utility function that:
- Accepts the route definition and path parameters
- Returns the resolved URL string
- Handles all entity types consistently
This would reduce duplication and make the code more maintainable.
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx (3)
450-463: MissingnetworkMode: "online"configuration.Similar to the
activeMedicationsquery, this query should also explicitly setnetworkMode: "online"per PR objectives to prevent background retries when offline.🔎 Suggested addition of networkMode
const { data: stoppedMedications } = useQuery({ queryKey: ["medication_requests_stopped", patientId, encounterId], queryFn: query(medicationRequestApi.list, { pathParams: { patientId }, queryParams: { encounter: encounterId, limit: 100, status: INACTIVE_MEDICATION_STATUSES.join(","), facility: facilityId, }, }), + networkMode: "online", meta: { persist: true }, enabled: !!patientId && canAccess, });
465-497: Potential cache proliferation with dynamicvisibleSlotsin query key.Including
visibleSlots(line 469) in the query key combined withmeta: { persist: true }will create a separate persisted cache entry for each unique time slot combination as users navigate. This can lead to IndexedDB storage bloat with many small cached ranges.Consider either:
- Fetching a wider date range and filtering client-side to reduce cache entries
- Using a more stable query key (e.g., date range only) that doesn't change with every navigation action
Additionally, this query is also missing
networkMode: "online"configuration per PR objectives.🔎 Suggested addition of networkMode
const { data: administrations } = useQuery({ queryKey: [ "medication_administrations", patientId, visibleSlots, encounterId, ], queryFn: query(medicationAdministrationApi.list, { pathParams: { patientId }, queryParams: { encounter: encounterId, limit: 100, ...(visibleSlots.length > 0 && { occurrence_period_start_after: (() => { const firstSlot = visibleSlots[0]; const [startHour] = firstSlot.start.split(":").map(Number); const date = new Date(firstSlot.date); date.setHours(startHour, 0, 0, 0); return date.toISOString(); })(), occurrence_period_start_before: (() => { const lastSlot = visibleSlots[visibleSlots.length - 1]; const [endHour] = lastSlot.end.split(":").map(Number); const date = new Date(lastSlot.date); date.setHours(endHour, 0, 0, 0); return date.toISOString(); })(), }), }, }), + networkMode: "online", meta: { persist: true }, enabled: !!patientId && !!visibleSlots?.length && canAccess, });
435-448: Medication data persisted to IndexedDB requires encryption for HIPAA compliance.The
meta: { persist: true }configuration caches medication request data (patient ID, encounter ID, facility ID, medication details) to IndexedDB without encryption. HIPAA Security Rule requires regulated entities to implement reasonable and appropriate administrative, physical, and technical safeguards for protecting ePHI and ensure the confidentiality, integrity, and availability of all ePHI. If PHI is properly encrypted and the keys are protected separately, loss or theft of that stored data may qualify for encryption safe harbor—meaning the incident may not be a reportable breach under HIPAA. Encryption and key management are not just security choices; they materially affect your regulatory exposure when stored data is compromised.Verify that encryption is implemented for the persisted query cache, particularly for medication administration data which contains patient health information.
src/components/Patient/allergy/list.tsx (1)
76-99: Align queryKey with conditional encounter filtering—cache mismatch causes stale data.The
queryKeyincludes onlypatientId, but thequeryFnconditionally filters byencounterIdbased onencounterStatus(lines 81–85). WhenencounterStatusorencounterIdchanges, React Query won't recognize this as a different query and will serve cached data from the previous encounter context.Per the learning, allergies are patient-specific and should not be filtered by encounter. Remove the conditional
encounterparameter from the query (lines 81–85) to fetch all patient allergies consistently:queryParams: { - encounter: - encounterStatus && - completedEncounterStatus.includes(encounterStatus) - ? encounterId - : undefined, limit: LIMIT,Also add
networkMode: "online"to align with the configuration inAllergyQuestion.tsx(line 586), ensuring consistent offline behavior for persisted medical data.src/components/Questionnaire/data/StructuredFormData.tsx (1)
283-287: Remove duplicate appointment entry.This appointment entry is a duplicate of the one already added at lines 267-271. Having the same entry twice in
STRUCTURED_QUESTIONSwill cause duplicate items in UI dropdowns/lists that consume this array.🔎 Proposed fix
{ value: "charge_item", label: "Charge Item", questionnaire: charge_item_questionnaire, }, - { - value: "appointment", - label: "Appointment", - questionnaire: appointment_questionnaire, - }, ] as const;src/components/HistoricalRecordSelector/index.tsx (1)
165-194: Contradictory configuration: persistence with staleTime: 0.Setting
meta: { persist: true }(line 190) combined withstaleTime: 0(line 193) creates contradictory behavior:
persist: truecaches data across sessions for offline accessstaleTime: 0marks data as immediately stale, triggering refetch on every useFor historical medical records, either:
- Remove
staleTime: 0to allow persisted data to be used, OR- Remove
meta: { persist: true }if fresh data is always requiredGiven this is historical data that rarely changes, consider removing
staleTime: 0or setting a reasonable stale window (e.g., 5-10 minutes).🔎 Recommended fix
queryFn: async () => { // ... queryFn implementation }, meta: { persist: true }, enabled: isOpen && !disableAPI, - staleTime: 0, + staleTime: 5 * 60 * 1000, // 5 minutes });src/components/Files/FileSubTab.tsx (1)
97-116: Ensure persisted file metadata is properly invalidated and doesn't exceed appropriate TTL.The
meta: { persist: true }flag enables offline access to file metadata. Since persisted data is stored independently from in-memory cache, verify that:
- File mutations (archive, delete, edit) call
queryClient.invalidateQueries({ queryKey: ["files"] })inonSuccessto mark persisted data as stale- The global persistence config has a
maxAgeappropriate for medical data (typically should be shorter than the default 24 hours)- Permission checks are re-evaluated when persisted data is restored, not just when the query is initially enabled
Note:
invalidateQueriesmarks queries as stale but doesn't remove data from localStorage; ensure your mutation strategy accounts for this distinction for sensitive medical records.src/components/Resource/ResourceList.tsx (1)
80-94: Add cache invalidation to resource mutations and document emergency data handling.Resource mutations (create/update) must call
queryClient.invalidateQueries({ queryKey: ["resources"] })in theonSuccesscallback to ensure the ResourceList reflects changes immediately. The 2-daymaxAgefor persistence is configured but doesn't account for emergency resource flags (line 282)—consider implementing a shorter TTL (e.g., 5–15 minutes) for queries with emergency-flagged resources or adding visual indicators when displaying persisted data in critical workflows.src/pages/Appointments/BookAppointment/AppointmentDateSelection.tsx (1)
106-126: Add explicit staleTime to appointment slots query for time-sensitive medical data.The default staleTime of 0 means data is immediately marked stale; while it will still read from cache, a background refetch can happen under certain conditions. The
slotsTodayQueryusesmeta: { persist: true }for offline support but lacks an explicitstaleTimeconfiguration. While the query key resets daily viadateQueryString(new Date()), within a single day appointment slots expire as time passes. TheisPastfiltering at line 134 only runs when rendering today's date with available data—if a refetch fails or the component unmounts, persisted stale slots could be reused on the next render.Set an explicit
staleTime(e.g., 5–15 minutes) on this query to ensure React Query refetches expired slots automatically and prevents users from selecting or viewing time-sensitive expired appointment slots.src/hooks/useFileManager.tsx (1)
95-111: Address signed URL expiration in persisted file queries.The
retrieveUploadquery caches file metadata containing signed URLs withmeta: { persist: true }, but signed URLs include digitally-signed expiration dates. If a user accesses cached file data after the URL expires, they will receive a 403 error. Consider:
- Setting a
staleTimeon the file query that aligns with your signed URL TTL- Validating signed URL expiration before using cached responses
- Implementing fallback logic to refetch when offline cache is detected as stale
- Documenting the signed URL validity window and cache strategy
src/pages/Facility/settings/organizations/components/FacilityOrganizationSelector.tsx (1)
79-98: Query key includes search term — may bloat persisted cacheIncluding
facilityOrgSearchin the query key (line 83-84) means each unique search string creates a separate cached entry withmeta: { persist: true }. This could lead to significant IndexedDB bloat over time as users search different terms.Consider either:
- Removing the search term from cached queries (only cache root/unexpanded results)
- Implementing cache eviction based on entry count or age
src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx (1)
397-426: MissingupdateQuestionnaireResponseCBin dependency array.The
useEffectcallsupdateQuestionnaireResponseCBbut omits it from dependencies. This can cause stale closures if the callback reference changes.🔎 Proposed fix
- }, [patientDiagnoses, editMode, offlineEntry]); + }, [patientDiagnoses, editMode, offlineEntry, updateQuestionnaireResponseCB, questionnaireResponse.question_id]);src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx (1)
683-711: Bug: MissingofflineEntryandupdateQuestionnaireResponseCBin dependency array.The effect uses
offlineEntry(line 684) but it's not in the dependency array. This means the effect won't re-run ifofflineEntrychanges, potentially displaying stale offline data.🔎 Proposed fix
- }, [patientSymptoms, editMode]); + }, [patientSymptoms, editMode, offlineEntry, updateQuestionnaireResponseCB, questionnaireResponse.question_id]);src/Providers/AuthUserProvider.tsx (1)
172-199: Remove duplicate cleanup calls insignOut.
queryClient.clear()anduserPersister.removeClient()are called twice - at lines 175-176 and again at lines 194-195. The second set of calls is redundant.🔎 Proposed fix - remove first duplicate set
const signOut = useCallback(async () => { const accessToken = localStorage.getItem(LocalStorageKeys.accessToken); const refreshToken = localStorage.getItem(LocalStorageKeys.refreshToken); - queryClient.clear(); - await userPersister.removeClient(); if (accessToken && refreshToken) { try { await mutate(authApi.logout)({src/components/Resource/ResourceForm.tsx (1)
688-697: Consider simplifying the disabled logic.The nested ternary is hard to read. Extract to a computed variable for clarity.
🔎 Proposed simplification
+ const isSubmitDisabled = (() => { + if (offlineEntryId) return false; // Always enable for offline entries + if (id) return isUpdatePending || !form.formState.isDirty; + return isPending; + })(); + <Button type="submit" variant="primary" - disabled={ - offlineEntryId - ? false // Always enable if editing an offline entry - : id - ? isUpdatePending || !form.formState.isDirty - : isPending - } + disabled={isSubmitDisabled} >
|
|
||
|
|
||
| # tanstack query garbage collection time (in milliseconds) (default set to: 2 days) | ||
| VITE_QUERY_GC_TIME= | ||
|
|
||
| # tanstack query persist max age (in milliseconds) (default set to 2days )(maxage<=gcTime) | ||
| VITE_QUERY_PERSIST_MAX_AGE= | ||
|
|
||
|
|
||
|
|
||
|
|
||
| # Time after which successfully synced offline records can be cleaned up | ||
| REACT_OFFLINE_CLEANUP_AFTER_SECONDS= | ||
|
|
||
| # Ping url to checknetwrk status | ||
| REACT_PING_URL="" |
There was a problem hiding this comment.
Fix typo and reduce excessive blank lines.
There's a typo on line 90: "checknetwrk" should be "check network". Also, the multiple consecutive blank lines (lines 76-77, 84-86) are inconsistent with the rest of the file formatting.
🔎 Proposed fix
-
-
# tanstack query garbage collection time (in milliseconds) (default set to: 2 days)
VITE_QUERY_GC_TIME=
# tanstack query persist max age (in milliseconds) (default set to 2days )(maxage<=gcTime)
VITE_QUERY_PERSIST_MAX_AGE=
-
-
-
# Time after which successfully synced offline records can be cleaned up
REACT_OFFLINE_CLEANUP_AFTER_SECONDS=
-# Ping url to checknetwrk status
+# Ping URL to check network status
REACT_PING_URL=""📝 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.
| # tanstack query garbage collection time (in milliseconds) (default set to: 2 days) | |
| VITE_QUERY_GC_TIME= | |
| # tanstack query persist max age (in milliseconds) (default set to 2days )(maxage<=gcTime) | |
| VITE_QUERY_PERSIST_MAX_AGE= | |
| # Time after which successfully synced offline records can be cleaned up | |
| REACT_OFFLINE_CLEANUP_AFTER_SECONDS= | |
| # Ping url to checknetwrk status | |
| REACT_PING_URL="" | |
| # tanstack query garbage collection time (in milliseconds) (default set to: 2 days) | |
| VITE_QUERY_GC_TIME= | |
| # tanstack query persist max age (in milliseconds) (default set to 2days )(maxage<=gcTime) | |
| VITE_QUERY_PERSIST_MAX_AGE= | |
| # Time after which successfully synced offline records can be cleaned up | |
| REACT_OFFLINE_CLEANUP_AFTER_SECONDS= | |
| # Ping URL to check network status | |
| REACT_PING_URL="" |
🤖 Prompt for AI Agents
.example.env around lines 76 to 91: fix the typo "checknetwrk" to "check
network" on the REACT_PING_URL comment and remove the excessive blank lines so
spacing matches the rest of the file (keep single blank lines between logical
groups); ensure comments remain aligned and preserve existing variable entries
and blank line separation conventions.
| // ===== Enforce max_age <= gctime ===== | ||
| const queryGcTime = import.meta.env.VITE_QUERY_GC_TIME | ||
| ? Number(import.meta.env.VITE_QUERY_GC_TIME) | ||
| : 1000 * 60 * 60 * 24 * 2; // 2 days | ||
|
|
||
| const queryPersistMaxAge = import.meta.env.VITE_QUERY_PERSIST_MAX_AGE | ||
| ? Number(import.meta.env.VITE_QUERY_PERSIST_MAX_AGE) | ||
| : 1000 * 60 * 60 * 24 * 2; // 2 days | ||
|
|
||
| if (queryPersistMaxAge > queryGcTime) { | ||
| throw new Error( | ||
| "queryPersistMaxAge must be less than or equal to queryGcTime", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add NaN validation for environment variable parsing.
If VITE_QUERY_GC_TIME or VITE_QUERY_PERSIST_MAX_AGE contains an invalid number string, Number() returns NaN, which could cause unexpected behavior. The comparison queryPersistMaxAge > queryGcTime with NaN always returns false, bypassing the validation.
🔎 Proposed fix with NaN handling
-const queryGcTime = import.meta.env.VITE_QUERY_GC_TIME
- ? Number(import.meta.env.VITE_QUERY_GC_TIME)
- : 1000 * 60 * 60 * 24 * 2; // 2 days
+const DEFAULT_CACHE_DURATION = 1000 * 60 * 60 * 24 * 2; // 2 days
+
+const parseEnvNumber = (value: string | undefined, fallback: number): number => {
+ if (!value) return fallback;
+ const parsed = Number(value);
+ if (Number.isNaN(parsed)) {
+ console.warn(`Invalid number in env: ${value}, using fallback: ${fallback}`);
+ return fallback;
+ }
+ return parsed;
+};
+
+const queryGcTime = parseEnvNumber(
+ import.meta.env.VITE_QUERY_GC_TIME,
+ DEFAULT_CACHE_DURATION,
+);
-const queryPersistMaxAge = import.meta.env.VITE_QUERY_PERSIST_MAX_AGE
- ? Number(import.meta.env.VITE_QUERY_PERSIST_MAX_AGE)
- : 1000 * 60 * 60 * 24 * 2; // 2 days
+const queryPersistMaxAge = parseEnvNumber(
+ import.meta.env.VITE_QUERY_PERSIST_MAX_AGE,
+ DEFAULT_CACHE_DURATION,
+);📝 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.
| // ===== Enforce max_age <= gctime ===== | |
| const queryGcTime = import.meta.env.VITE_QUERY_GC_TIME | |
| ? Number(import.meta.env.VITE_QUERY_GC_TIME) | |
| : 1000 * 60 * 60 * 24 * 2; // 2 days | |
| const queryPersistMaxAge = import.meta.env.VITE_QUERY_PERSIST_MAX_AGE | |
| ? Number(import.meta.env.VITE_QUERY_PERSIST_MAX_AGE) | |
| : 1000 * 60 * 60 * 24 * 2; // 2 days | |
| if (queryPersistMaxAge > queryGcTime) { | |
| throw new Error( | |
| "queryPersistMaxAge must be less than or equal to queryGcTime", | |
| ); | |
| } | |
| // ===== Enforce max_age <= gctime ===== | |
| const DEFAULT_CACHE_DURATION = 1000 * 60 * 60 * 24 * 2; // 2 days | |
| const parseEnvNumber = (value: string | undefined, fallback: number): number => { | |
| if (!value) return fallback; | |
| const parsed = Number(value); | |
| if (Number.isNaN(parsed)) { | |
| console.warn(`Invalid number in env: ${value}, using fallback: ${fallback}`); | |
| return fallback; | |
| } | |
| return parsed; | |
| }; | |
| const queryGcTime = parseEnvNumber( | |
| import.meta.env.VITE_QUERY_GC_TIME, | |
| DEFAULT_CACHE_DURATION, | |
| ); | |
| const queryPersistMaxAge = parseEnvNumber( | |
| import.meta.env.VITE_QUERY_PERSIST_MAX_AGE, | |
| DEFAULT_CACHE_DURATION, | |
| ); | |
| if (queryPersistMaxAge > queryGcTime) { | |
| throw new Error( | |
| "queryPersistMaxAge must be less than or equal to queryGcTime", | |
| ); | |
| } |
🤖 Prompt for AI Agents
In care.config.ts around lines 30 to 43, the numeric parsing of
VITE_QUERY_GC_TIME and VITE_QUERY_PERSIST_MAX_AGE does not validate for NaN so
an invalid env value will bypass the max_age <= gctime check; update the parsing
to validate each parsed number (e.g., use Number() then check Number.isFinite or
!Number.isNaN) and if parsing fails either throw a clear Error indicating the
specific invalid environment variable or fall back to the intended default, then
perform the existing comparison and throw the existing error when
queryPersistMaxAge > queryGcTime.
| "cross-env": "^7.0.3", | ||
| "date-fns": "^4.0.0", | ||
| "dayjs": "^1.11.13", | ||
| "dexie": "^4.0.11", |
There was a problem hiding this comment.
Update Dexie to the latest stable version.
The current latest stable version of Dexie is 4.2.1, whereas the specified version ^4.0.11 only ensures updates within the 4.0.x branch. The prototype pollution vulnerability that affected earlier versions (before 4.0.0-alpha.3) has been resolved, so version 4.0.11 is secure. However, updating to 4.2.1 or using ^4.2.1 would provide access to newer features and improvements while maintaining compatibility.
🤖 Prompt for AI Agents
In package.json around line 100, the dependency "dexie": "^4.0.11" is pinned to
the 4.0.x branch; update it to the latest stable release by changing the version
string to "^4.2.1" (or simply "4.2.1") in the dependencies section so the
project can consume Dexie 4.2.1 and its fixes/features, then run npm install /
yarn install and verify the build and tests pass.
| "invalid_entry_type_encounter": "Invalid entry type for encounter editing", | ||
| "invalid_entry_type_questionnaire": "Invalid entry type for structured questionnaire editing", | ||
| "invalid_entry_type_structured_questionnaire": "Invalid entry type for structured questionnaire editing", | ||
| "invalid_entry_type_time_of_death": "Invalid entry type for time of death editing", | ||
| "invalid_ip_address": "Invalid IP Address", |
There was a problem hiding this comment.
Clarify invalid_entry_type_* messages (structured vs non‑structured)
These two keys currently share effectively the same message:
- Line 2620:
"invalid_entry_type_questionnaire": "Invalid entry type for structured questionnaire editing" - Line 2621:
"invalid_entry_type_structured_questionnaire": "Invalid entry type for structured questionnaire editing"
This makes it impossible for users to distinguish “generic questionnaire” vs “structured questionnaire” cases, and suggests at least one message is mislabeled.
Consider updating along these lines:
invalid_entry_type_questionnaire: “Invalid entry type for questionnaire editing”invalid_entry_type_structured_questionnaire: keep as is for the structured case.
This keeps semantics clear and helps debugging when surfaced in toasts/logs.
🤖 Prompt for AI Agents
In public/locale/en.json around lines 2619 to 2623, the two keys for
questionnaire errors are identical and ambiguous; update
"invalid_entry_type_questionnaire" to read "Invalid entry type for questionnaire
editing" (non-structured) and leave
"invalid_entry_type_structured_questionnaire" as "Invalid entry type for
structured questionnaire editing" so the messages clearly distinguish generic
questionnaire vs structured questionnaire cases.
| "no_encounter_data_offline": "No encounter data found in offline entry", | ||
| "no_encounter_history": "No encounter history available", |
There was a problem hiding this comment.
Align encounter‑offline “no data found” messages
"no_encounter_data_offline": "No encounter data found in offline entry" overlaps conceptually with "encounter_info_not_found_offline": "Encounter information not found offline" (Line 2002).
If both are used, it would be good to standardize phrasing so users don’t see two almost‑identical messages in different parts of the UI. For example, make one explicitly about “entry payload missing” vs the other “encounter not cached for offline use”.
🤖 Prompt for AI Agents
In public/locale/en.json around lines 3290-3291 (and reference line ~2002), the
two offline encounter messages are phrasing-overlapping; update them to be
distinct and consistent: change one to explicitly indicate a missing entry
payload (e.g., "Offline entry payload missing") and the other to indicate the
encounter is not cached for offline use (e.g., "Encounter not cached for offline
use"), and ensure both follow the project's capitalization and punctuation
conventions so UI shows clear, non-redundant messages.
| queryClient.setQueryDefaults(["patientPermissions"], { | ||
| meta: { persist: true }, | ||
| networkMode: "online", | ||
| }); | ||
|
|
||
| queryClient.setQueryDefaults(["encounterPermissions"], { | ||
| meta: { persist: true }, | ||
| networkMode: "online", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move setQueryDefaults calls into a useEffect to avoid side effects during render.
Calling setQueryDefaults directly in the component body executes on every render, which is inefficient and violates React's pure render principle. These defaults should be set once when the component mounts.
🔎 Proposed fix
const queryClient = useQueryClient();
- queryClient.setQueryDefaults(["patientPermissions"], {
- meta: { persist: true },
- networkMode: "online",
- });
-
- queryClient.setQueryDefaults(["encounterPermissions"], {
- meta: { persist: true },
- networkMode: "online",
- });
+ useEffect(() => {
+ queryClient.setQueryDefaults(["patientPermissions"], {
+ meta: { persist: true },
+ networkMode: "online",
+ });
+
+ queryClient.setQueryDefaults(["encounterPermissions"], {
+ meta: { persist: true },
+ networkMode: "online",
+ });
+ }, [queryClient]);🤖 Prompt for AI Agents
In src/pages/Encounters/EncounterShow.tsx around lines 91 to 99,
setQueryDefaults is being called during render which causes side effects on
every render; move those calls into a useEffect so they run once on mount. Wrap
both queryClient.setQueryDefaults calls in a useEffect(() => { ... }, []),
ensure queryClient is available from context/hook inside the effect, and keep
the same options (meta: { persist: true }, networkMode: "online") so defaults
are applied only once when the component mounts.
| useEffect(() => { | ||
| if (patient?.permissions && facilityId) { | ||
| queryClient.setQueryData( | ||
| ["patientPermissions", facilityId], | ||
| patient.permissions, | ||
| ); | ||
| } | ||
| }, [patient?.permissions, facilityId]); | ||
|
|
||
| useEffect(() => { | ||
| if (primaryEncounter?.permissions && facilityId) { | ||
| queryClient.setQueryData( | ||
| ["encounterPermissions", facilityId], | ||
| primaryEncounter.permissions, | ||
| ); | ||
| } | ||
| }, [primaryEncounter?.permissions, facilityId]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add queryClient to dependency arrays.
While queryClient is typically stable, omitting it from dependencies can cause ESLint warnings and potential issues if the client instance ever changes. Adding it ensures correctness and silences linter warnings.
🔎 Proposed fix
useEffect(() => {
if (patient?.permissions && facilityId) {
queryClient.setQueryData(
["patientPermissions", facilityId],
patient.permissions,
);
}
- }, [patient?.permissions, facilityId]);
+ }, [patient?.permissions, facilityId, queryClient]);
useEffect(() => {
if (primaryEncounter?.permissions && facilityId) {
queryClient.setQueryData(
["encounterPermissions", facilityId],
primaryEncounter.permissions,
);
}
- }, [primaryEncounter?.permissions, facilityId]);
+ }, [primaryEncounter?.permissions, facilityId, queryClient]);🤖 Prompt for AI Agents
In src/pages/Encounters/EncounterShow.tsx around lines 101 to 117, both
useEffect hooks update queryClient.setQueryData but do not include queryClient
in their dependency arrays; add queryClient to each dependency array (i.e.,
[patient?.permissions, facilityId, queryClient] and
[primaryEncounter?.permissions, facilityId, queryClient]) so the effects remain
correct if the client instance changes and to silence ESLint warnings.
| silent: true, | ||
| }), | ||
|
|
||
| meta: { persist: true }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find persister-related files
fd -t f "Persister\|persister" src/ --extension ts --extension tsx
# Look for OfflineSupport directory structure
find src -type d -name "*Offline*" -o -type d -name "*offline*"Repository: ohcnetwork/care_fe
Length of output: 80
🏁 Script executed:
# List files in OfflineSupport directory
ls -la src/OfflineSupport/
# Check for persister file
find src/OfflineSupport -type f -name "*.ts" -o -name "*.tsx"Repository: ohcnetwork/care_fe
Length of output: 1666
🏁 Script executed:
# Examine the createUserPersister.ts file
cat -n src/OfflineSupport/createUserPersister.ts
# Check for encryption/crypto imports in the project
rg -n "encrypt|crypto" src/OfflineSupport/ --type ts --type tsx
# Check logout/session cleanup in auth-related files
find src -type f -name "*auth*" -o -name "*logout*" -o -name "*session*" | head -20Repository: ohcnetwork/care_fe
Length of output: 1770
🏁 Script executed:
# Examine AppCacheDB to see if encryption is implemented
cat -n src/OfflineSupport/AppcacheDB.ts
# Check for logout handlers
rg -n "logout|removeClient|clear" src/types/auth/ --type ts -A 3
# Check auth API for session cleanup
cat -n src/types/auth/auth.ts | head -100Repository: ohcnetwork/care_fe
Length of output: 3969
🏁 Script executed:
# Find logout handler implementation
rg -n "logout\|signout" src/ --type ts -B 2 -A 5 | head -80
# Check if removeClient is called during logout
rg -n "removeClient\|persistQueryClient" src/ --type ts -B 2 -A 2
# Check for audit logging/context
cat -n src/context/AuditContext.tsx 2>/dev/null || echo "AuditContext not found"
# Search for any encryption usage in the entire codebase
rg -n "encrypt\|crypto\|cipher" src/ --type ts | head -20Repository: ohcnetwork/care_fe
Length of output: 84
🏁 Script executed:
# Search for logout handler more broadly
rg -n "logout" src/ --type ts -B 3 -A 5 | head -100
# Check for cache/session clearing on logout
rg -n "clearQueryData\|clear()\|removeClient" src/ --type ts -B 2 -A 2
# Check auth context or state management
find src/context -type f -name "*.ts" -o -name "*.tsx" | head -20
# Look for Dexie encryption middleware usage
rg -n "applyEncryption\|Encrypted" src/ --type tsRepository: ohcnetwork/care_fe
Length of output: 2970
🏁 Script executed:
# Examine the full logout implementation
cat -n src/Providers/AuthUserProvider.tsx | head -200
# Check if there are any access controls for cached data
rg -n "permission\|access\|authorized" src/OfflineSupport/ --type ts -B 2 -A 2
# Check SyncContext for any security measures
cat -n src/context/SyncContext.tsx | head -100
# Check PermissionContext for PHI access controls
cat -n src/context/PermissionContext.tsx | head -150Repository: ohcnetwork/care_fe
Length of output: 12868
🏁 Script executed:
# Check if there's any encryption middleware applied to Dexie
rg -n "Dexie\|applyEncryption" src/OfflineSupport/ --type ts -A 3
# Verify patient data is being persisted unencrypted
rg -n "meta.*persist\|persist.*true" src/pages/Patient/ --type ts | head -20
# Check what specific data is cached in PatientHomeEncounters
cat -n src/pages/Patient/home/PatientHomeEncounters.tsx | sed -n '40,60p'Repository: ohcnetwork/care_fe
Length of output: 665
Critical: Encounter data persisted unencrypted—implement PHI encryption and access controls.
Patient encounter data in PatientHomeEncounters.tsx is persisted to IndexedDB without encryption. Verification confirms:
- No encryption at rest:
createUserPersister.tsstores data unencrypted in Dexie IndexedDB - No access controls for cached data: Runtime permission checks exist but don't protect persisted data
- No audit logging: No audit context found for PHI access tracking
- Data cleanup works: Logout properly clears persisted data via
removeClient()andqueryClient.clear()
Implement encryption middleware for the AppCacheDB (using Dexie-encrypted or equivalent), add audit logging for PHI access, and establish session timeout guards for offline-persisted data to meet HIPAA compliance requirements.
🤖 Prompt for AI Agents
In src/pages/Patient/home/PatientHomeEncounters.tsx around line 48 the persister
is configured with meta: { persist: true } which causes PHI to be stored
unencrypted in IndexedDB; replace this by wiring AppCacheDB to an encryption
layer (e.g., Dexie-encrypted or equivalent) so data at rest is encrypted with
keys stored in a secure keystore (OS keychain or a KMS-backed secret), enforce
access controls so all runtime permission checks validate before decrypting
cached records, add audit logging hooks that record user id, timestamp,
operation, and reason whenever PHI is read from or written to the DB, and
implement a session timeout/auto-lock that clears or re-encrypts persisted data
after inactivity and on logout; also add migration path and tests to ensure
decryption fails without keys and audit entries are emitted for PHI access.
| useEffect(() => { | ||
| // persist only when success + valid patient returned | ||
| if (status === "success" && patientData?.id) { | ||
| queryClient.ensureQueryData({ | ||
| queryKey: ["patient", patientData.id], | ||
| queryFn: async () => patientData, | ||
| staleTime: Infinity, | ||
| gcTime: Infinity, | ||
| meta: { persist: true }, | ||
| }); | ||
|
|
||
| console.log("Patient cached:", patientData.id); | ||
| } | ||
| console.log("status:", status); | ||
| }, [status, patientData?.id]); // depend only on stable values |
There was a problem hiding this comment.
Remove console.log statements and fix dependency array.
Debug console.log statements (lines 81, 83) should be removed before merging to production. Additionally, queryClient and patientData should be in the dependency array since they're used inside the effect.
🔎 Proposed fix
useEffect(() => {
// persist only when success + valid patient returned
if (status === "success" && patientData?.id) {
queryClient.ensureQueryData({
queryKey: ["patient", patientData.id],
queryFn: async () => patientData,
staleTime: Infinity,
gcTime: Infinity,
meta: { persist: true },
});
-
- console.log("Patient cached:", patientData.id);
}
- console.log("status:", status);
- }, [status, patientData?.id]); // depend only on stable values
+ }, [status, patientData, queryClient]);🤖 Prompt for AI Agents
In src/pages/Patient/VerifyPatient.tsx around lines 70 to 84, remove the two
console.log statements on lines 81 and 83 and update the useEffect dependency
array to include the variables used inside the effect; replace the current
dependency array with [status, patientData, queryClient] so the effect runs
correctly when patientData or queryClient change.
| export const getMonthFromDate = (dateStr: string | Date) => { | ||
| const date = new Date(dateStr); | ||
| return startOfMonth(date); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add JSDoc documentation and error handling.
The new utility function lacks documentation and error handling for invalid dates. As per coding guidelines, utility functions should document their purpose and behavior, especially for date handling functions that should consider timezone implications.
🔎 Proposed improvements
+/**
+ * Returns the start of the month for a given date.
+ *
+ * @param dateStr - Date string or Date object
+ * @returns Date object representing the start of the month (first day at 00:00:00)
+ * @throws Error if the date is invalid
+ *
+ * @example
+ * getMonthFromDate('2024-03-15') // Returns 2024-03-01 00:00:00
+ * getMonthFromDate(new Date()) // Returns first day of current month
+ */
export const getMonthFromDate = (dateStr: string | Date) => {
const date = new Date(dateStr);
+ if (isNaN(date.getTime())) {
+ throw new Error(`Invalid date: ${dateStr}`);
+ }
return startOfMonth(date);
};As per coding guidelines, document date utility functions and handle timezone considerations properly.
🤖 Prompt for AI Agents
In src/Utils/utils.ts around lines 238 to 241, add JSDoc describing the
function’s purpose, accepted inputs (string|Date), return type and explicit note
about timezone behavior (whether it returns the start of month in local time or
UTC), and implement input validation/error handling: parse the input into a
Date, check validity (e.g. using isNaN(date.getTime()) or a date-fns isValid
call) and throw a clear TypeError on invalid input; keep using startOfMonth but
document the timezone semantics so callers know how dates are normalized.
| import { SyncProvider } from "@/context/SyncContext"; | ||
|
|
||
| import { createUserPersister } from "./OfflineSupport/createUserPersister"; | ||
|
|
||
| import { ShortcutProvider } from "@/context/ShortcutContext"; | ||
|
|
||
| import { toast } from "sonner"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Import order inconsistency.
Imports are not following the prescribed order (3rd-party → library → CAREUI → UI → components → hooks → utils → relative). SyncProvider from context should come before the relative import of createUserPersister, and toast should be grouped with other 3rd-party imports.
🤖 Prompt for AI Agents
In src/App.tsx lines 22-28, the import order is inconsistent: move third-party
imports like `toast` (from "sonner") to the top grouped with other 3rd-party
imports, then place library/CAREUI imports as applicable, then context imports
(`SyncProvider`, `ShortcutProvider` from "@/context/...") before any relative
imports; finally keep the relative import `createUserPersister` from
"./OfflineSupport/createUserPersister" last—reorder the import statements to
follow the prescribed sequence (3rd-party → library → CAREUI → UI → components →
hooks → utils → relative).
| onlineManager.setEventListener(() => { | ||
| return () => {}; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify intent: empty event listener disables default online/offline detection.
Setting an empty event listener on onlineManager disables React Query's default browser-based online/offline detection. While this appears intentional (to use custom backend ping-based detection instead), a comment explaining this would improve maintainability.
🔎 Suggested improvement
+// Disable React Query's default online/offline detection based on browser events.
+// Custom detection via useNetworkStatus uses backend ping for more reliable status.
onlineManager.setEventListener(() => {
return () => {};
});📝 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.
| onlineManager.setEventListener(() => { | |
| return () => {}; | |
| }); | |
| // Disable React Query's default online/offline detection based on browser events. | |
| // Custom detection via useNetworkStatus uses backend ping for more reliable status. | |
| onlineManager.setEventListener(() => { | |
| return () => {}; | |
| }); |
🤖 Prompt for AI Agents
In src/App.tsx around lines 31 to 33, the call to onlineManager.setEventListener
with an empty function disables React Query's default browser online/offline
detection but lacks an explanatory comment; add a concise comment above this
line stating that disabling the default listener is intentional because the app
uses a custom backend ping-based online detection (include brief note about
where that custom logic lives or points to its module), so future maintainers
understand this is deliberate and not a bug.
| displayCareConsoleArt(); | ||
| }, []); | ||
|
|
||
| toast.success("Welcome to Care!"); |
There was a problem hiding this comment.
Critical bug: Toast fires on every render instead of once on mount.
toast.success("Welcome to Care!") is placed in the component body outside of any hook, causing it to execute on every render. This will spam users with welcome toasts on any state change or re-render.
🔎 Suggested fix
Move the toast inside the existing useEffect:
const App = () => {
useEffect(() => {
displayCareConsoleArt();
+ toast.success("Welcome to Care!");
}, []);
- toast.success("Welcome to Care!");
-
return (📝 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.
| toast.success("Welcome to Care!"); | |
| useEffect(() => { | |
| displayCareConsoleArt(); | |
| toast.success("Welcome to Care!"); | |
| }, []); | |
| return ( |
🤖 Prompt for AI Agents
In src/App.tsx around line 49, the toast.success("Welcome to Care!") call is
currently in the component body and therefore runs on every render; remove that
call from the component body and place it inside the existing useEffect so it
runs once on mount (ensure the useEffect has an appropriate dependency array,
e.g., [] or the minimal dependencies required); this prevents the toast from
firing on every state change or re-render.
| await db.OfflineWrites.where({ | ||
| type: OfflineKeyMap.assign_user_to_patient, | ||
| resourceType: "patient", | ||
| }) | ||
| .and((entry) => { | ||
| const payload = entry.payload as { user: string; role: string }; | ||
| return ( | ||
| entry.mutationPathParams?.patientId === patientId && | ||
| payload?.user === removeUserId | ||
| ); | ||
| }) | ||
| .delete(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider handling edge case where matching assign entry doesn't exist.
The code deletes related assign entries when removing a user (lines 172-183). However, if a user is removed who was assigned online (not offline), this deletion won't find any matching entries. While this is not an error, consider adding a log or comment to clarify this expected behavior for maintainability.
- await db.OfflineWrites.where({
+ const deletedCount = await db.OfflineWrites.where({
type: OfflineKeyMap.assign_user_to_patient,
resourceType: "patient",
})
.and((entry) => {
const payload = entry.payload as { user: string; role: string };
return (
entry.mutationPathParams?.patientId === patientId &&
payload?.user === removeUserId
);
})
.delete();
+
+ // Note: deletedCount will be 0 if user was assigned online, which is expected
+ if (deletedCount > 0) {
+ console.log(`Removed ${deletedCount} pending assign operation(s) for user ${removeUserId}`);
+ }🤖 Prompt for AI Agents
In src/components/Patient/PatientDetailsTab/offlineQueue.ts around lines 172 to
183, the deletion of OfflineWrites for assign_user_to_patient may no-op when the
user was assigned online; update the code to explicitly handle that edge case by
capturing the delete result (number of rows removed) and either log a debug/info
message when zero rows were deleted to document this expected condition or add
an inline comment explaining why no matching offline entry is not an error;
ensure the log message includes patientId and removeUserId for traceability.
| const db = new AppCacheDB(); | ||
| const [partialOfflinePatients, setPartialOfflinePatients] = useState< | ||
| PartialPatientModel[] | [] | ||
| >([]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move AppCacheDB initialization outside component body or use useMemo.
Creating a new AppCacheDB instance on every render is inefficient. Consider initializing it once using useMemo or moving it outside the component if it's safe to share across instances.
+const db = new AppCacheDB(); // Module-level singleton
+
export default function PatientIndex({ facilityId }: { facilityId: string }) {
- const db = new AppCacheDB();
const [partialOfflinePatients, setPartialOfflinePatients] = useState<
PartialPatientModel[] | []
>([]);Or with useMemo:
const db = useMemo(() => new AppCacheDB(), []);🤖 Prompt for AI Agents
In src/components/Patient/PatientIndex.tsx around lines 57 to 60, the code
currently creates a new AppCacheDB instance on every render which is
inefficient; replace that with a single persistent instance by either moving the
new AppCacheDB() call outside the component (if it is safe to share across
component instances) or by memoizing it inside the component with useMemo and an
empty dependency array so the instance is created only once per component
lifecycle; choose the approach appropriate for AppCacheDB (shared singleton vs
per-component) and ensure any cleanup logic is preserved if the DB requires
disposal.
| useEffect(() => { | ||
| if (!isRestoring) updateStatus(); | ||
|
|
||
| // Listen to onlineManager status changes | ||
| const unsubscribe = onlineManager.subscribe((online) => { | ||
| if (!online && !offlinePollInterval.current) { | ||
| startOfflinePolling(); | ||
| } else if (online) { | ||
| stopOfflinePolling(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener("focus", updateStatus); | ||
| return () => { | ||
| window.removeEventListener("focus", updateStatus); | ||
| if (transitionTimeout.current) clearTimeout(transitionTimeout.current); | ||
| stopOfflinePolling(); | ||
| unsubscribe(); | ||
| }; | ||
| }, [isRestoring]); |
There was a problem hiding this comment.
Missing dependencies in useEffect may cause stale closures.
The useEffect references updateStatus, startOfflinePolling, and stopOfflinePolling functions, but these are not included in the dependency array. This can lead to stale closure bugs. Since these functions reference state and refs, they should either be memoized with useCallback or included in the dependency array.
🔎 Suggested fix
Wrap the functions with useCallback:
+ const stopOfflinePolling = useCallback(() => {
+ if (offlinePollInterval.current) {
+ clearInterval(offlinePollInterval.current);
+ offlinePollInterval.current = null;
+ }
+ }, []);
+ const startOfflinePolling = useCallback(() => {
+ if (!offlinePollInterval.current) {
+ offlinePollInterval.current = window.setInterval(async () => {
+ const reachable = await checkBackendReachable();
+ if (reachable) goOnline();
+ }, OFFLINE_POLL_INTERVAL);
+ }
+ }, [goOnline]);
useEffect(() => {
if (!isRestoring) updateStatus();
// ...
return () => {
// cleanup
};
- }, [isRestoring]);
+ }, [isRestoring, updateStatus, startOfflinePolling, stopOfflinePolling]);Committable suggestion skipped: line range outside the PR's diff.
| useEffect(() => { | ||
| if (!onlineManager.isOnline() && !offlinePollInterval.current) { | ||
| startOfflinePolling(); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Second useEffect has empty dependency array but references functions that may change.
This effect calls startOfflinePolling() which depends on goOnline, creating a potential stale closure issue. Consider combining this with the main effect or ensuring proper dependencies.
🤖 Prompt for AI Agents
In src/Utils/networkstatus.ts around lines 154 to 158, the second useEffect has
an empty dependency array but calls startOfflinePolling(), which captures
goOnline and can create a stale closure; fix by either merging this effect into
the main effect that already depends on goOnline (so it always uses the current
functions), or make startOfflinePolling and goOnline stable (wrap them in
useCallback or store current functions in refs) and add them to the effect
dependency array; update the effect to include the appropriate dependencies or
move the call to the main effect to avoid stale references.
| retry: (failureCount, error) => { | ||
| // Only retry network errors or server errors (502, 503, 504) up to 3 times | ||
| if ( | ||
| error.message === "Network Error" || | ||
| (error instanceof HTTPError && [502, 503, 504].includes(error.status)) | ||
| ) { | ||
| if (onlineManager.isOnline()) { | ||
| toast.warning(i18next.t("you_are_offline")); | ||
| } | ||
| onlineManager.setOnline(false); | ||
| return failureCount < 3; | ||
| } | ||
| return false; | ||
| }, |
There was a problem hiding this comment.
Toast spam risk: offline warning shown on every retry attempt.
The toast notification inside the retry callback will fire on each of the 3 retry attempts, potentially showing 3 "you are offline" warnings in quick succession. Move the toast outside the retry logic or add deduplication.
🔎 Suggested fix
retry: (failureCount, error) => {
// Only retry network errors or server errors (502, 503, 504) up to 3 times
if (
error.message === "Network Error" ||
(error instanceof HTTPError && [502, 503, 504].includes(error.status))
) {
- if (onlineManager.isOnline()) {
- toast.warning(i18next.t("you_are_offline"));
- }
onlineManager.setOnline(false);
return failureCount < 3;
}
return false;
},The toast is already shown in onError, so removing it from retry prevents duplicates.
🤖 Prompt for AI Agents
In src/Utils/request/queryClient.ts around lines 46 to 59, the retry callback
currently calls toast.warning(i18next.t("you_are_offline")) on each retry which
causes duplicate toasts; remove the toast call from this retry function and keep
only the onlineManager.setOnline(false) and the retry return logic (failureCount
< 3) so retries still occur for network/server errors, and rely on the existing
onError handler to show the offline toast once.
| onError: async (error) => { | ||
| if (error.message !== "Network Error") { | ||
| handleHttpError(error); | ||
| return; | ||
| } | ||
| if (onlineManager.isOnline()) { | ||
| toast.warning(i18next.t("you_are_offline")); | ||
| } | ||
| onlineManager.setOnline(false); | ||
| await restorePersistedCache(queryClient); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fragile error detection using string comparison.
Checking error.message === "Network Error" is brittle as error messages can vary across browsers, fetch implementations, and locales. Consider creating a dedicated error type or utility function for network error detection.
🔎 Suggested fix
// Add to types.ts or a utility file
export function isNetworkError(error: Error): boolean {
return (
error.message === "Network Error" ||
error.name === "NetworkError" ||
error.message.includes("Failed to fetch") ||
error.message.includes("network")
);
}Also applies to: 80-91
🤖 Prompt for AI Agents
In src/Utils/request/queryClient.ts around lines 67-77 (and similarly 80-91),
the code uses a brittle string comparison error.message === "Network Error";
replace this with a reusable network-error detection utility (e.g.,
isNetworkError) and call that instead. Implement isNetworkError in a shared
utils/types file to check multiple reliable signals (error.name, common
substrings like "Failed to fetch" or "network", and any library-specific flags)
and import/use it in both places; keep existing offline handling behavior and
ensure type narrowing or guards handle non-Error values safely.
| onSuccess: async () => { | ||
| if (!onlineManager.isOnline()) { | ||
| toast.success(i18next.t("welcome_back_you_are_online")); | ||
| } | ||
| onlineManager.setOnline(true); | ||
| }, |
There was a problem hiding this comment.
Overly aggressive online status: any mutation success sets online=true.
Setting onlineManager.setOnline(true) on every successful mutation could interfere with offline queue processing. If offline mutations are processed locally and succeed without network, this would incorrectly mark the app as online. The online status should be determined by actual network reachability checks, not mutation success.
🔎 Suggested fix
Consider removing the onSuccess handler or making it conditional:
mutationCache: new MutationCache({
onError: async (error) => {
// ... existing error handling
},
- onSuccess: async () => {
- if (!onlineManager.isOnline()) {
- toast.success(i18next.t("welcome_back_you_are_online"));
- }
- onlineManager.setOnline(true);
- },
}),Let useNetworkStatus handle online status transitions based on actual backend reachability.
🤖 Prompt for AI Agents
In src/Utils/request/queryClient.ts around lines 92 to 97, the current onSuccess
handler unconditionally calls onlineManager.setOnline(true) after any mutation
success which wrongly flips the app to online when mutations succeed locally;
remove that unconditional setOnline(true) (or remove the onSuccess handler
entirely) and instead rely on a dedicated network reachability mechanism (e.g.,
useNetworkStatus or onlineManager pings) to update online state; if you must
keep a success hook, make it conditional on an explicit network-confirmation
flag returned by the server or call a network check before calling
onlineManager.setOnline(true).
| const fetchAndPopulateForm = async () => { | ||
| try { | ||
| const db = new AppCacheDB(); | ||
| const entry = await db.OfflineWrites.get(offlineEntryId!); | ||
| setOfflineEntryData(entry); | ||
|
|
||
| if (entry && entry.normalizedData) { | ||
| const normalizedData = entry.normalizedData as EncounterRead; | ||
| setOfflineSelectedOrganizations(normalizedData.organizations || []); | ||
| form.reset({ | ||
| status: normalizedData.status as | ||
| | EncounterStatus.PLANNED | ||
| | EncounterStatus.IN_PROGRESS | ||
| | EncounterStatus.ON_HOLD, | ||
| encounter_class: normalizedData.encounter_class, | ||
| priority: normalizedData.priority ?? "routine", | ||
| organizations: | ||
| normalizedData.organizations?.map((org) => org.id) ?? [], | ||
| start_date: normalizedData.period?.start ?? new Date().toISOString(), | ||
| tags: normalizedData.tags?.map((tag) => tag.id) ?? [], | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error fetching offline entry:", error); | ||
| toast.error(t("error_loading_offline_encounter_data")); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add cleanup on component unmount to prevent memory leaks.
The fetchAndPopulateForm function is defined but there's no cleanup if the component unmounts while the async operation is in progress. Consider adding an abort signal or cleanup flag.
🔎 Add cleanup to prevent state updates on unmounted component
+ const isMountedRef = useRef(true);
+
+ useEffect(() => {
+ return () => {
+ isMountedRef.current = false;
+ };
+ }, []);
const fetchAndPopulateForm = async () => {
try {
const db = new AppCacheDB();
const entry = await db.OfflineWrites.get(offlineEntryId!);
+
+ if (!isMountedRef.current) return; // Don't update if unmounted
+
setOfflineEntryData(entry);
if (entry && entry.normalizedData) {
const normalizedData = entry.normalizedData as EncounterRead;
setOfflineSelectedOrganizations(normalizedData.organizations || []);
form.reset({
// ... form data
});
}
} catch (error) {
console.error("Error fetching offline entry:", error);
+ if (!isMountedRef.current) return;
toast.error(t("error_loading_offline_encounter_data"));
}
};🤖 Prompt for AI Agents
In src/components/Encounter/CreateEncounterForm.tsx around lines 220 to 246, the
async fetchAndPopulateForm can update state after the component unmounts; add a
mounted/abort mechanism and check it before any state updates to prevent memory
leaks. Implement a local let mounted = true (or an AbortController if the DB
call supports it) inside the effect that defines fetchAndPopulateForm, and in
the effect cleanup set mounted = false (or call controller.abort()); then, after
awaiting db.OfflineWrites.get(...), guard all state calls (setOfflineEntryData,
setOfflineSelectedOrganizations, form.reset, toast/error logging if desired)
with if (!mounted) return so no updates occur after unmount. Ensure the cleanup
is added to the useEffect that calls fetchAndPopulateForm.
| const patientMedications = queryClient.getQueryData< | ||
| PaginatedResponse<MedicationRequestRead> | ||
| >(["medication_requests", patientId, encounterId]); |
There was a problem hiding this comment.
Replace synchronous getQueryData with reactive useQuery hook.
Using queryClient.getQueryData() reads the cache once at render time and won't trigger re-renders when the cache updates. According to the PR objectives, offline mutations use setQueryData to provide immediate UI feedback. However, this component won't reflect those updates because it doesn't subscribe to cache changes.
This defeats the purpose of React Query's reactivity and the offline sync mechanism.
🔎 Recommended fix using useQuery hook
- const patientMedications = queryClient.getQueryData<
- PaginatedResponse<MedicationRequestRead>
- >(["medication_requests", patientId, encounterId]);
+ const { data: patientMedications } = useQuery({
+ queryKey: ["medication_requests", patientId, encounterId],
+ queryFn: () =>
+ queryClient.getQueryData<PaginatedResponse<MedicationRequestRead>>(
+ ["medication_requests", patientId, encounterId]
+ ) ?? { count: 0, results: [] },
+ enabled: !onlineManager.isOnline(),
+ staleTime: Infinity,
+ });This approach:
- Subscribes to cache updates, ensuring UI reflects offline mutations immediately
- Only runs when offline (via
enabledoption) - Returns empty results if no cached data exists
- Follows the coding guideline to use
useQueryfor data access
Committable suggestion skipped: line range outside the PR's diff.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for medication_requests query key definitions and usage
echo "=== Searching for medication_requests query key patterns ==="
rg -n --type ts --type tsx "medication_requests" -A 3 -B 3 | head -100
echo ""
echo "=== Searching for useQuery with medication_requests ==="
rg -n --type ts --type tsx "useQuery.*medication" -A 5 | head -80
echo ""
echo "=== Looking at the target file ==="
cat -n src/components/Medicine/MedicationRequestTable/index.tsx | head -100Repository: ohcnetwork/care_fe
Length of output: 4585
🏁 Script executed:
#!/bin/bash
# More targeted search for medication request queries
echo "=== Search for medication request API/hooks ==="
find src -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "medication.*request" | head -20
echo ""
echo "=== Search for useQuery with medication ==="
find src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "useQuery" {} \; | xargs grep -l "medication" | head -15
echo ""
echo "=== Look for query key definitions ==="
find src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -n "queryKey.*medication" {} + 2>/dev/null | head -20
echo ""
echo "=== Search for where medication_requests is fetched ==="
find src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "medication_requests" {} \; 2>/dev/nullRepository: ohcnetwork/care_fe
Length of output: 5849
🏁 Script executed:
#!/bin/bash
# Check TreatmentSummary.tsx to see the actual query
echo "=== TreatmentSummary.tsx around line 135 ==="
sed -n '130,150p' src/components/Patient/TreatmentSummary.tsx
echo ""
echo "=== MedicationRequestQuestion.tsx around line 216 ==="
sed -n '210,230p' src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx
echo ""
echo "=== Check if there are other files with medication_requests fetch ==="
grep -n "useQuery.*medication_requests\|queryKey.*medication_requests" src/components/Patient/TreatmentSummary.tsx src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx -A 5Repository: ohcnetwork/care_fe
Length of output: 3522
🏁 Script executed:
#!/bin/bash
# Check where MedicationRequestTable is used
echo "=== Where is MedicationRequestTable imported? ==="
grep -r "MedicationRequestTable" src --include="*.tsx" --include="*.ts" | grep -i "import\|from"
echo ""
echo "=== Check the parent structure of MedicationRequestTable ==="
grep -n "import.*MedicationRequestTable\|from.*MedicationRequestTable" src -r --include="*.tsx"
echo ""
echo "=== Look at full context of how it's used ==="
grep -B 5 -A 10 "MedicationRequestTable" src/pages/Encounters/*.tsx 2>/dev/null | head -40Repository: ohcnetwork/care_fe
Length of output: 1862
🏁 Script executed:
#!/bin/bash
# Check how MedicationRequestTable is used in medicines.tsx
echo "=== src/pages/Encounters/tabs/medicines.tsx ==="
cat src/pages/Encounters/tabs/medicines.tsx
echo ""
echo "=== Check if there are other queries for medication_requests in the same context ==="
grep -n "useQuery\|useEncounter\|medication" src/pages/Encounters/tabs/medicines.tsx | head -30Repository: ohcnetwork/care_fe
Length of output: 364
🏁 Script executed:
#!/bin/bash
# Check the Encounters page context
echo "=== Check Encounters page structure ==="
cat src/pages/Encounters/index.tsx | head -150
echo ""
echo "=== Look for any queries in Encounters folder ==="
grep -r "useQuery.*medication" src/pages/Encounters --include="*.tsx" | head -20Repository: ohcnetwork/care_fe
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Find Encounters structure
echo "=== List Encounters directory ==="
find src/pages/Encounters -type f -name "*.tsx" | head -20
echo ""
echo "=== Look for where EncounterProvider is defined ==="
grep -r "EncounterProvider\|useEncounter" src --include="*.tsx" --include="*.ts" | grep -i "export\|function\|const" | head -15
echo ""
echo "=== Check if there's a parent component that provides the context ==="
cat src/pages/Encounters/utils/EncounterProvider.tsx | head -100Repository: ohcnetwork/care_fe
Length of output: 5717
🏁 Script executed:
#!/bin/bash
# Check if MedicationRequestTable actually fetches data or just reads from cache
echo "=== Full MedicationRequestTable component (focus on data fetching) ==="
sed -n '1,150p' src/components/Medicine/MedicationRequestTable/index.tsx | tail -80
echo ""
echo "=== Check if there are any useQuery hooks in MedicationRequestTable ==="
grep -n "useQuery\|useMutation\|queryClient.fetch" src/components/Medicine/MedicationRequestTable/index.tsx
echo ""
echo "=== Check PrescriptionView which is imported ==="
grep -n "useQuery.*medication" src/components/Medicine/PrescriptionView.tsx | head -5Repository: ohcnetwork/care_fe
Length of output: 3342
🏁 Script executed:
#!/bin/bash
# Critical check: Who is responsible for populating the medication_requests cache?
echo "=== Check PrescriptionListSelector which is used in MedicationRequestTable ==="
grep -n "useQuery\|medication_requests" src/components/Medicine/PrescriptionListSelector.tsx | head -20
echo ""
echo "=== Check if there's any hook that fetches medication_requests globally ==="
find src -name "*use*.ts" -o -name "*use*.tsx" | xargs grep -l "medication_requests" 2>/dev/null
echo ""
echo "=== Look at full EncounterShow to see the hierarchy ==="
find src/pages/Encounters -name "EncounterShow.tsx" -exec cat {} \; | head -80Repository: ohcnetwork/care_fe
Length of output: 3575
🏁 Script executed:
#!/bin/bash
# Check what query key PrescriptionListSelector uses
echo "=== PrescriptionListSelector useQuery ==="
sed -n '35,55p' src/components/Medicine/PrescriptionListSelector.tsx
echo ""
echo "=== Check if there are other components that might fetch medication_requests ==="
grep -rn "medication_requests.*patientId.*encounterId" src/components/Medicine/MedicationRequestTable --include="*.tsx" --include="*.ts"
echo ""
echo "=== Final confirmation: which component actually fetches with ["medication_requests", patientId, encounterId]? ==="
grep -rn "queryKey.*medication_requests.*patientId.*encounterId\|useQuery.*medication_requests" src --include="*.tsx" -A 5 | grep -E "queryKey|useQuery|queryFn" | head -30Repository: ohcnetwork/care_fe
Length of output: 2094
🏁 Script executed:
#!/bin/bash
# Critical finding: Check if TreatmentSummary or MedicationRequestQuestion are used in Encounter tab
echo "=== Is TreatmentSummary used in Encounter flow? ==="
grep -rn "TreatmentSummary" src/pages/Encounters --include="*.tsx"
echo ""
echo "=== Is MedicationRequestQuestion used in Encounter flow? ==="
grep -rn "MedicationRequestQuestion" src/pages/Encounters --include="*.tsx"
echo ""
echo "=== Where is TreatmentSummary actually used? ==="
grep -rn "TreatmentSummary" src --include="*.tsx" | grep -i "import\|from"
echo ""
echo "=== Check what gets rendered in EncounterShow tabs ==="
grep -n "EncounterMedicinesTab\|medicines.tsx" src/pages/Encounters/EncounterShow.tsx -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 957
🏁 Script executed:
#!/bin/bash
# Critical investigation: trace the actual data flow in Encounter medicines tab
echo "=== Full ConsultationRoutes to see when TreatmentSummary is used ==="
cat src/Routers/routes/ConsultationRoutes.tsx
echo ""
echo "=== Check if there's any prefetch or initial setup for medication_requests in Encounter ==="
grep -rn "prefetchQuery.*medication" src/pages/Encounters --include="*.tsx" --include="*.ts"
echo ""
echo "=== Check where medicine data comes from in offline mode ==="
grep -rn "medication.*offline\|offline.*medication" src --include="*.tsx" --include="*.ts" | head -10Repository: ohcnetwork/care_fe
Length of output: 7658
🏁 Script executed:
#!/bin/bash
# Final critical check: trace complete data flow in Encounter medicines tab
echo "=== What components are actually in EncounterMedicinesTab? ==="
cat src/pages/Encounters/tabs/medicines.tsx
echo ""
echo "=== Is there ANYTHING that fetches medication_requests in the Encounter flow? ==="
grep -rn "medication_requests" src/pages/Encounters --include="*.tsx" --include="*.ts" | grep -v "medication_requests_active\|medication_requests_stopped"
echo ""
echo "=== Check if PrescriptionView or PrescriptionListSelector fetch medication_requests ==="
grep -n "useQuery\|medication_requests" src/components/Medicine/PrescriptionView.tsx | head -20
echo ""
echo "=== Summary: what queries are actually used in the medicines tab ==="
echo "1. PrescriptionListSelector queries:"
grep -n "queryKey" src/components/Medicine/PrescriptionListSelector.tsx
echo ""
echo "2. MedicationRequestTable queries:"
grep -n "queryKey\|useQuery\|getQueryData" src/components/Medicine/MedicationRequestTable/index.tsxRepository: ohcnetwork/care_fe
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Final verification: Is MedicationRequestTable used ONLY in EncounterMedicinesTab or also elsewhere?
echo "=== All places where MedicationRequestTable is imported ==="
grep -rn "import.*MedicationRequestTable\|from.*MedicationRequestTable" src --include="*.tsx"
echo ""
echo "=== Check MedicationHistory which also imports from MedicationRequestTable ==="
cat src/pages/Patient/History/MedicationHistory.tsx | head -50Repository: ohcnetwork/care_fe
Length of output: 3223
Query key ["medication_requests", patientId, encounterId] is never populated in Encounter medicines tab flow.
The MedicationRequestTable component attempts to retrieve cached medication requests with this key, but the Encounter medicines tab only fetches prescriptions via PrescriptionListSelector. The medication_requests cache entry remains unpopulated in this context, resulting in missing data for offline access. Either the component should fetch its own data with useQuery or be refactored to accept medication data as props from a parent component that does fetch it.
| {onlineManager.isOnline() ? ( | ||
| <div className="flex flex-1 flex-col lg:flex-row w-full gap-1 h-full"> | ||
| <PrescriptionListSelector | ||
| patientId={patientId} | ||
| encounterId={encounterId} | ||
| facilityId={facilityId} | ||
| selectedPrescriptionId={selectedPrescriptionId} | ||
| onSelectPrescription={(prescription) => { | ||
| setSelectedPrescriptionId(prescription?.id); | ||
| }} | ||
| /> | ||
|
|
||
| {selectedPrescriptionId ? ( | ||
| <div className="flex-1 w-full h-full overflow-auto"> | ||
| <PrescriptionView | ||
| patientId={patientId} | ||
| prescriptionId={selectedPrescriptionId} | ||
| canWrite={canWrite} | ||
| facilityId={facilityId} | ||
| encounterId={encounterId} | ||
| /> | ||
| </div> | ||
| ) : ( | ||
| <div className="w-full flex-1 h-full flex items-center justify-center"> | ||
| <div className="flex flex-col items-center"> | ||
| <ReceiptTextIcon className="text-gray-500" /> | ||
| <h3 className="font-medium"> | ||
| {t("no_prescriptions_found")} | ||
| </h3> | ||
| {canWrite && ( | ||
| <Button | ||
| asChild | ||
| variant="outline" | ||
| className="text-gray-950 hover:text-gray-700 h-9 mt-2" | ||
| > | ||
| <Link href={`questionnaire/medication_request`}> | ||
| <PlusIcon className="mr-2 size-4" /> | ||
| {t("create_prescription")} | ||
| </Link> | ||
| </Button> | ||
| )} | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ) : ( | ||
| <div className="flex flex-1 flex-col lg:flex-row w-full gap-1 h-full"> | ||
| {patientMedications ? ( | ||
| <div className="flex-1 w-full h-full overflow-auto"> | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center gap-2 flex-wrap justify-between px-2 my-2 w-full"> | ||
| <div className="flex items-center gap-2 flex-wrap "> | ||
| {canWrite && ( | ||
| <Button | ||
| asChild | ||
| variant="outline" | ||
| size="sm" | ||
| className="text-gray-950 hover:text-gray-700 h-9" | ||
| data-cy="edit-prescription" | ||
| > | ||
| <Link href={`questionnaire/medication_request`}> | ||
| <> | ||
| <PencilIcon className="mr-2 size-4" /> | ||
| {t("edit")} | ||
| </> | ||
| </Link> | ||
| </Button> | ||
| )} | ||
| </div> | ||
| </div> | ||
| <div className="flex flex-col gap-4 px-2"> | ||
| <MedicationsTable | ||
| medications={patientMedications.results || []} | ||
| showActiveOnly={false} | ||
| /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <div className="w-full flex-1 h-full flex items-center justify-center"> | ||
| <div className="flex flex-col items-center"> | ||
| <ReceiptTextIcon className="text-gray-500" /> | ||
| <h3 className="font-medium"> | ||
| {t("no_prescriptions_found")} | ||
| </h3> | ||
| {canWrite && ( | ||
| <Button | ||
| asChild | ||
| variant="outline" | ||
| className="text-gray-950 hover:text-gray-700 h-9 mt-2" | ||
| data-cy="edit-prescription" | ||
| > | ||
| <Link href={`questionnaire/medication_request`}> | ||
| <PlusIcon className="mr-2 size-4" /> | ||
| {t("create_prescription")} | ||
| </Link> | ||
| </Button> | ||
| )} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Use reactive online status subscription instead of static check.
The onlineManager.isOnline() call on line 116 is evaluated once at render time and doesn't react to network status changes. If the user goes offline (or comes back online) while viewing this component, the UI will show stale online/offline state until the component remounts.
This creates a poor user experience where the UI doesn't reflect the actual network state.
🔎 Recommended fix using useOnlineManager hook
Create a custom hook or use React Query's subscription pattern:
// Add this custom hook at the top of the file or in a utils file
import { useEffect, useState } from "react";
import { onlineManager } from "@tanstack/react-query";
function useIsOnline() {
const [isOnline, setIsOnline] = useState(onlineManager.isOnline());
useEffect(() => {
return onlineManager.subscribe((online) => {
setIsOnline(online);
});
}, []);
return isOnline;
}Then use it in the component:
const [selectedPrescriptionId, setSelectedPrescriptionId] = useState<
string | undefined
>();
+ const isOnline = useIsOnline();
const patientMedications = queryClient.getQueryData<
PaginatedResponse<MedicationRequestRead>
>(["medication_requests", patientId, encounterId]);
return (
<div className="space-y-2 h-full">
<Tabs defaultValue="prescriptions" className="h-full">
{/* ... */}
<TabsContent value="prescriptions" className="flex-1 flex flex-col overflow-hidden">
- {onlineManager.isOnline() ? (
+ {isOnline ? (
<div className="flex flex-1 flex-col lg:flex-row w-full gap-1 h-full">Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Medicine/MedicationRequestTable/index.tsx around lines 116-218
the component uses onlineManager.isOnline() directly which is only evaluated
once and will not update when network status changes; replace that static check
with a reactive subscription: create a useIsOnline hook that initializes state
from onlineManager.isOnline(), subscribes to onlineManager in a useEffect to
update state on changes (and unsubscribes on cleanup), then call useIsOnline()
inside the component and use the returned boolean in place of
onlineManager.isOnline() so the UI re-renders when online status changes.
| <div className="w-full flex-1 h-full flex items-center justify-center"> | ||
| <div className="flex flex-col items-center"> | ||
| <ReceiptTextIcon className="text-gray-500" /> | ||
| <h3 className="font-medium"> | ||
| {t("no_prescriptions_found")} | ||
| </h3> | ||
| {canWrite && ( | ||
| <Button | ||
| asChild | ||
| variant="outline" | ||
| className="text-gray-950 hover:text-gray-700 h-9 mt-2" | ||
| > | ||
| <Link href={`questionnaire/medication_request`}> | ||
| <PlusIcon className="mr-2 size-4" /> | ||
| {t("create_prescription")} | ||
| </Link> | ||
| </Button> | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicated empty state into reusable component.
The online (lines 139-158) and offline (lines 195-215) empty states contain nearly identical JSX with only minor differences (the data-cy attribute). This duplication violates the DRY principle and increases maintenance burden.
🔎 Proposed refactor to eliminate duplication
You already have an EmptyState component defined in this file (lines 31-58) but it uses a different structure. Consider either:
Option 1: Update the existing EmptyState component to support the prescription icon and create button:
interface EmptyStateProps {
searching?: boolean;
searchQuery?: string;
message?: string;
description?: string;
+ showCreateButton?: boolean;
+ createButtonHref?: string;
+ createButtonText?: string;
+ dataCy?: string;
}
export const EmptyState = ({
searching,
searchQuery,
message,
description,
+ showCreateButton,
+ createButtonHref,
+ createButtonText,
+ dataCy,
}: EmptyStateProps) => {
const { t } = useTranslation();
return (
- <div className="flex min-h-[200px] flex-col items-center justify-center gap-4 p-8 text-center">
- <div className="rounded-full bg-secondary/10 p-3">
- <CareIcon icon="l-tablets" className="text-3xl text-gray-500" />
- </div>
+ <div className="w-full flex-1 h-full flex items-center justify-center">
+ <div className="flex flex-col items-center">
+ <ReceiptTextIcon className="text-gray-500" />
<h3 className="font-medium">
{message || (searching ? t("no_matches_found") : t("no_prescriptions"))}
</h3>
- <div className="max-w-[200px] space-y-1">
- <p className="text-sm text-gray-500">
- {description ||
- (searching
- ? t("no_medications_match_query", { searchQuery })
- : t("no_medications_prescribed"))}
- </p>
- </div>
+ {showCreateButton && canWrite && (
+ <Button
+ asChild
+ variant="outline"
+ className="text-gray-950 hover:text-gray-700 h-9 mt-2"
+ data-cy={dataCy}
+ >
+ <Link href={createButtonHref!}>
+ <PlusIcon className="mr-2 size-4" />
+ {createButtonText}
+ </Link>
+ </Button>
+ )}
</div>
</div>
);
};Then use it:
{isOnline ? (
selectedPrescriptionId ? (
// ... PrescriptionView
) : (
<EmptyState
message={t("no_prescriptions_found")}
showCreateButton={true}
createButtonHref="questionnaire/medication_request"
createButtonText={t("create_prescription")}
/>
)
) : (
patientMedications ? (
// ... MedicationsTable
) : (
<EmptyState
message={t("no_prescriptions_found")}
showCreateButton={true}
createButtonHref="questionnaire/medication_request"
createButtonText={t("create_prescription")}
dataCy="edit-prescription"
/>
)
)}Option 2: Create a separate PrescriptionEmptyState component with the prescription-specific UI.
Also applies to: 195-215
🤖 Prompt for AI Agents
In src/components/Medicine/MedicationRequestTable/index.tsx around lines 139-158
(and similarly 195-215), the online and offline empty-state JSX is duplicated;
extract the shared UI into a reusable component by either extending the existing
EmptyState to accept props for icon, message, optional create button (href/text)
and optional data-cy, or by adding a new PrescriptionEmptyState component with
those props; replace both inline blocks with the new component, passing
dataCy="edit-prescription" only for the offline case to preserve the attribute
and ensure translations and canWrite logic are forwarded into the component.
| interface NormalizeAndSetQueryDataParams { | ||
| entry: any; | ||
| patientData: PatientRead | undefined; | ||
| queryClient: QueryClient; | ||
| authUser: CurrentUserRead; | ||
| selectedOrganization: Organization | null; | ||
| existingTags: TagConfig[]; | ||
| identifiers: PatientIdentifier[]; | ||
| permissions: string[] | undefined; | ||
| createdDate?: string; | ||
| modifiedDate?: string; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid any type; use proper type definitions.
The entry parameter in NormalizeAndSetQueryDataParams is typed as any, and patientData is declared but never used in the function body.
As per coding guidelines, avoid explicit any types. Consider using OfflineWritesEntry for type safety.
🔎 Proposed fix
interface NormalizeAndSetQueryDataParams {
- entry: any;
- patientData: PatientRead | undefined;
+ entry: OfflineWritesEntry;
queryClient: QueryClient;
authUser: CurrentUserRead;
selectedOrganization: Organization | null;
existingTags: TagConfig[];
identifiers: PatientIdentifier[];
permissions: string[] | undefined;
createdDate?: string;
modifiedDate?: string;
}Add the import at the top:
import { OfflineWritesEntry } from "@/OfflineSupport/AppcacheDB";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Patient/offlineQueue.ts around lines 52 to 63: the interface
NormalizeAndSetQueryDataParams uses `entry: any` and declares `patientData`
which is unused; replace the any with the specific OfflineWritesEntry type and
either remove the unused patientData property or use it where intended. Add the
import `OfflineWritesEntry` from "@/OfflineSupport/AppcacheDB" at the top of the
file, update the interface to use `entry: OfflineWritesEntry`, and remove the
unused patientData field (or reference it in the function) so the type is
accurate and no unused properties remain.
| try { | ||
| const db = new AppCacheDB(); | ||
| const parentWrite = await db.OfflineWrites.get(parentId); | ||
|
|
||
| if (!parentWrite) { | ||
| return { | ||
| isSynced: false, | ||
| parentId, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Inconsistent parent sync status logic.
When parentWrite is not found (line 1328), the function returns isSynced: false. However, if the parent ID refers to a record that was never offline (i.e., it's a server-created record), the absence of an offline write entry should mean it's already synced. Consider checking if the parentId starts with "offline-" prefix.
🔎 Proposed fix
export async function checkParentSyncStatus(parentId?: string): Promise<{
isSynced: boolean;
parentId?: string;
}> {
if (!parentId) {
return {
isSynced: true,
parentId,
};
}
+ // If parent is not an offline ID, it's already synced (server-created)
+ if (!isOfflineId(parentId)) {
+ return {
+ isSynced: true,
+ parentId,
+ };
+ }
+
try {
const db = new AppCacheDB();
const parentWrite = await db.OfflineWrites.get(parentId);
if (!parentWrite) {
+ // Offline entry was cleaned up after successful sync
return {
- isSynced: false,
+ isSynced: true,
parentId,
};
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/OfflineSupport/offlineWriteHelpers.ts around lines 1324 to 1333, the code
treats a missing parentWrite as unsynced; change the logic to consider whether
parentId is an offline-generated id: if parentWrite is missing and parentId does
not start with the "offline-" prefix, return { isSynced: true, parentId },
otherwise return { isSynced: false, parentId }; implement this by checking
parentId.startsWith("offline-") (guarding for null/undefined) before deciding
the isSynced flag.
| import { onlineManager } from "@tanstack/react-query"; | ||
| import { toast } from "sonner"; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Filename typo: "syncmanger.ts" should be "syncmanager.ts".
The filename contains a typo. Consider renaming the file and updating all imports for better maintainability.
🤖 Prompt for AI Agents
In src/OfflineSupport/syncmanger.ts around lines 1 to 3 the filename contains a
typo ("syncmanger.ts") — rename the file to src/OfflineSupport/syncmanager.ts
and update all project imports and references to use the corrected filename;
update any build/test configs or barrel exports that reference the old path, run
a global search/replace for "syncmanger" -> "syncmanager", and verify imports
compile and tests pass.
| interface SyncManagerOptions { | ||
| userId: string; | ||
| facilityId?: string; | ||
| maxRetries?: number; | ||
| retryDelayMs?: number; | ||
| enableConflictDetection?: boolean; | ||
| onProgress?: (syncedCount: number, totalCount: number) => void; | ||
| onSyncStart?: (totalCount: number) => void; | ||
| onSyncComplete?: () => void; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src -name "syncmanger.ts" -o -name "syncmanager.ts" | head -5Repository: ohcnetwork/care_fe
Length of output: 94
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '50,65p'Repository: ohcnetwork/care_fe
Length of output: 614
🏁 Script executed:
rg "maxRetries" src/OfflineSupport/syncmanger.ts -nRepository: ohcnetwork/care_fe
Length of output: 110
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '400,430p'Repository: ohcnetwork/care_fe
Length of output: 751
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | grep -A 20 "processWrite"Repository: ohcnetwork/care_fe
Length of output: 1711
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '115,180p'Repository: ohcnetwork/care_fe
Length of output: 2309
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '115,140p'Repository: ohcnetwork/care_fe
Length of output: 981
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '1,100p' | tail -50Repository: ohcnetwork/care_fe
Length of output: 1590
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '62,114p'Repository: ohcnetwork/care_fe
Length of output: 1548
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '115,160p'Repository: ohcnetwork/care_fe
Length of output: 1600
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '185,250p'Repository: ohcnetwork/care_fe
Length of output: 2510
🏁 Script executed:
cat -n src/OfflineSupport/syncmanger.ts | sed -n '185,300p'Repository: ohcnetwork/care_fe
Length of output: 4332
🏁 Script executed:
rg "delay\(" src/OfflineSupport/syncmanger.ts -A 2 | head -10Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
rg "private.*delay" src/OfflineSupport/syncmanger.ts -A 2Repository: ohcnetwork/care_fe
Length of output: 44
maxRetries option is defined but not used for automatic retries.
The processWrite method does not implement retry logic using the maxRetries option. Failed writes are caught, logged with an incremented retry counter, and immediately marked as failed without automatic re-attempts. The maxRetries and retryDelayMs options are defined in SyncManagerOptions but are never referenced in the actual write processing logic. Either implement the retry mechanism or remove these unused options.
| for (let i = 0; i < sortedWrites.length; i++) { | ||
| const write = sortedWrites[i]; | ||
|
|
||
| if (this.abortController?.signal.aborted) { | ||
| break; | ||
| } | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
|
|
There was a problem hiding this comment.
Hardcoded delay ignores retryDelayMs option.
Line 128 uses a fixed 500ms delay, but the SyncManagerOptions interface accepts retryDelayMs. The passed option is never used.
🔎 Proposed fix
for (let i = 0; i < sortedWrites.length; i++) {
const write = sortedWrites[i];
if (this.abortController?.signal.aborted) {
break;
}
- await new Promise((resolve) => setTimeout(resolve, 500));
+ await new Promise((resolve) =>
+ setTimeout(resolve, this.options.retryDelayMs ?? 500)
+ );
const writeResult = await this.processWrite(write);🤖 Prompt for AI Agents
In src/OfflineSupport/syncmanger.ts around lines 121 to 129, the loop uses a
hardcoded 500ms delay which ignores the SyncManagerOptions.retryDelayMs; replace
the fixed delay with the configured retryDelayMs (falling back to the existing
default of 500ms if the option is undefined) by reading
this.options.retryDelayMs (or options.retryDelayMs) and waiting that many
milliseconds instead of the constant 500.
| private isPermanentFailure(error: any): boolean { | ||
| if (error instanceof HTTPError) { | ||
| const statusCode = error.status; | ||
|
|
||
| if (statusCode >= 400 && statusCode < 500 && statusCode !== 429) { | ||
| return true; | ||
| } | ||
|
|
||
| if (statusCode >= 500) { | ||
| // Consider 501, 502, 503 as temporary, others as permanent | ||
| return ![501, 502, 503].includes(statusCode); | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
HTTP 500 treated as permanent failure may be incorrect.
The current logic treats HTTP 500 (Internal Server Error) as a permanent failure, but 500 errors are often transient server issues that resolve on retry. Consider including 500 in the temporary failure list alongside 501, 502, 503.
🔎 Proposed fix
if (statusCode >= 500) {
- // Consider 501, 502, 503 as temporary, others as permanent
- return ![501, 502, 503].includes(statusCode);
+ // Consider 500, 501, 502, 503, 504 as temporary (common transient errors)
+ return ![500, 501, 502, 503, 504].includes(statusCode);
}🤖 Prompt for AI Agents
In src/OfflineSupport/syncmanger.ts around lines 306 to 321, the
isPermanentFailure function currently treats HTTP 500 as a permanent failure;
change the logic so 500 is considered temporary like 501/502/503. Update the
status check to treat 500,501,502,503 as temporary (i.e., return false for these
codes) and only return true for other 5xx codes; keep existing 4xx handling
(except 429) unchanged.
adarsh-priydarshi-5646
left a comment
adarsh-priydarshi-5646
left a comment
There was a problem hiding this comment.
@Vikaspal8923, This PR was raised without a linked issue and contains a very large number of commits and changes (128 commits, 122 files, ~10k LOC).
Please do not raise PRs without an approved issue and assignment.
Additionally, such a large PR makes it very difficult to review and raises concerns about whether the changes were properly understood and tested. It would be better to first go through the contribution guidelines and understand the expected workflow.
Please close this PR and raise smaller, well-scoped PRs linked to approved issues after assignment. Also, ensure that the code is written and reviewed carefully before submission, rather than pushing large, LLM-auto-generated changes. This helps avoid unnecessary noise and keeps the review process effective.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
@adarsh-priydarshi-5646 This PR was part of a GSoC project |
4 participants
Offline Support Implemented For:
Offline Caching & Sync Logic Overview:
React Query caching is used with meta: { persist: true } to persist only those queries required for offline workflows selectively. This ensures that only essential query data is stored offline.
networkMode: "online" is set explicitly to avoid unnecessary background retries or inconsistencies when offline.
Mutations are queued manually:
For each supported form, mutation records are stored in a custom OfflineWrites queue. Each entry contains:
A unique ID (e.g., offline-+action)
The required mutation path, type, and payload
User and facility context
For existing records, a serverTimestamp and useQueryPathParams are also stored to allow post-sync reconciliation.
Displaying Offline-Created/Updated Records Immediately
To ensure a smooth user experience, any entity created or updated while offline is shown immediately in the UI — without waiting for backend sync. This is achieved through the following approach:
Normalized the form data: After form submission, the raw payload is normalized to match the API response shape.
React Query setQueryData is then used to:
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.