Fix: Location-Switcher text overflow#16228
Fix: Location-Switcher text overflow#16228pranavrutagi-egov wants to merge 37 commits intoohcnetwork:developfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
WalkthroughLocationSwitcher UI updated to fix text overflow in constrained widths: container uses Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
🚀 Preview Deployment Ready!🔗 Preview URL: https://pr-16228.care-preview-a7w.pages.dev 📱 Mobile Access: This preview will be automatically updated when you push new commits to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/sidebar/facility/location/location-switcher.tsx`:
- Around line 91-100: The TooltipComponent is being rendered with
content={location?.name} which can be undefined and causes an empty tooltip;
update the JSX in the LocationSwitcher component to only render TooltipComponent
when location?.name is truthy (e.g., wrap the existing TooltipComponent block in
a conditional like location?.name ? <TooltipComponent ...> : <div ...>),
otherwise render the same inner div without the TooltipComponent (or use a
native title attribute on the inner div) so no empty tooltip appears while
useCurrentLocation is resolving.
- Line 32: The import for TooltipComponent is out of order; move the line
importing TooltipComponent so it sits with the other "@/components/ui/" imports
(the UI group) instead of after "@/common/constants". Locate the import
statement for TooltipComponent and place it alongside the other UI imports in
this file (e.g., near other imports from "@/components/ui/") to follow the
project's import ordering convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdf93067-0e8f-48ec-a4cb-08faa6fb3ba7
📒 Files selected for processing (1)
src/components/ui/sidebar/facility/location/location-switcher.tsx
|
|
||
| import { RESULTS_PER_PAGE_LIMIT } from "@/common/constants"; | ||
|
|
||
| import { TooltipComponent } from "@/components/ui/tooltip"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Import order: move this with other @/components/ui/ imports.
The TooltipComponent import should be grouped with the other @/components/ui/ imports (lines 10-26), not placed after @/common/constants.
🧹 Suggested reordering
Move the import to be with the other UI imports:
import { Separator } from "@/components/ui/separator";
import { useSidebar } from "@/components/ui/sidebar";
+import { TooltipComponent } from "@/components/ui/tooltip";
import PaginationComponent from "@/components/Common/Pagination";
import { RESULTS_PER_PAGE_LIMIT } from "@/common/constants";
-import { TooltipComponent } from "@/components/ui/tooltip";
import useCurrentLocation from "@/pages/Facility/locations/utils/useCurrentLocation";As per coding guidelines: "Order imports: 3rd-party → library → CAREUI → UI → components → hooks → utils → relative."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/sidebar/facility/location/location-switcher.tsx` at line
32, The import for TooltipComponent is out of order; move the line importing
TooltipComponent so it sits with the other "@/components/ui/" imports (the UI
group) instead of after "@/common/constants". Locate the import statement for
TooltipComponent and place it alongside the other UI imports in this file (e.g.,
near other imports from "@/components/ui/") to follow the project's import
ordering convention.
| <TooltipComponent content={location?.name}> | ||
| <div className="flex min-w-0 flex-col items-start"> | ||
| <span className="text-xs text-gray-500"> | ||
| {t("current_location")} | ||
| </span> | ||
| <span className="w-full truncate text-left text-sm font-medium text-gray-900"> | ||
| {location?.name} | ||
| </span> | ||
| </div> | ||
| </TooltipComponent> |
There was a problem hiding this comment.
Handle undefined location to avoid rendering an empty tooltip.
When location is undefined (initial state before useCurrentLocation resolves), location?.name will be undefined. Based on the TooltipComponent implementation in src/components/ui/tooltip.tsx, this will still render the tooltip wrapper with empty content, potentially showing an empty tooltip on hover.
Consider conditionally rendering the tooltip only when the location name is available:
🛠️ Suggested fix
<div className="min-w-0 flex-1">
- <TooltipComponent content={location?.name}>
- <div className="flex min-w-0 flex-col items-start">
- <span className="text-xs text-gray-500">
- {t("current_location")}
- </span>
- <span className="w-full truncate text-left text-sm font-medium text-gray-900">
- {location?.name}
- </span>
- </div>
- </TooltipComponent>
+ <div className="flex min-w-0 flex-col items-start">
+ <span className="text-xs text-gray-500">
+ {t("current_location")}
+ </span>
+ <span
+ className="w-full truncate text-left text-sm font-medium text-gray-900"
+ title={location?.name}
+ >
+ {location?.name}
+ </span>
+ </div>
</div>Alternatively, keep TooltipComponent but guard rendering:
{location?.name ? (
<TooltipComponent content={location.name}>
...
</TooltipComponent>
) : (
<div className="flex min-w-0 flex-col items-start">...</div>
)}Based on learnings: "In the care_fe repository, prefer the native HTML title attribute for simple tooltips over using shadcn/ui Tooltip components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/sidebar/facility/location/location-switcher.tsx` around
lines 91 - 100, The TooltipComponent is being rendered with
content={location?.name} which can be undefined and causes an empty tooltip;
update the JSX in the LocationSwitcher component to only render TooltipComponent
when location?.name is truthy (e.g., wrap the existing TooltipComponent block in
a conditional like location?.name ? <TooltipComponent ...> : <div ...>),
otherwise render the same inner div without the TooltipComponent (or use a
native title attribute on the inner div) so no empty tooltip appears while
useCurrentLocation is resolving.
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #8141 |
|
Hi @gigincg, kindly review this PR |
Jacobjeevan
left a comment
Jacobjeevan
left a comment
There was a problem hiding this comment.
Please do ensure you are checking relevant/completed items from the merge checklist.
…ft empty (ohcnetwork#16244) Co-authored-by: nihal467 <nihalcct@gmail.com>\
…cnetwork#16073) - while request is still pending
…rk#16045) Co-authored-by: Mohammed Nihal <57055998+nihal467@users.noreply.github.com>
… TagFilterDropdown (ohcnetwork#15984)
…ESTIONNAIRE) print (ohcnetwork#16193)
…on (ohcnetwork#16136) Co-authored-by: Mohammed Nihal <57055998+nihal467@users.noreply.github.com>
Co-authored-by: Yaswanth Kumar Bethu <yash.topaz@gmail.com>
…ork#16110) Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Mohammed Nihal <57055998+nihal467@users.noreply.github.com>
…network#16032) Co-authored-by: Ishu Agrawal <ishuagrawal@Ishus-MacBook-Air.local>
|
@nihal467 this looks fine |
…k#16094) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ion and fixed search (ohcnetwork#15894)
nihal467
left a comment
nihal467
left a comment
There was a problem hiding this comment.
- dont show tooltip in the mobile view
- fix the responsive view in the location selector popup as well
nihal467
added
Changes Required
and removed
needs testing
reviewed
|
Hi @nihal467, as mentioned in your comment, I have made the following changes:
Here are supporting screenshots: |
There was a problem hiding this comment.
Actionable comments posted: 45
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/components/Common/ProductionWarningBanner.tsx (1)
6-7: 🧹 Nitpick | 🔵 TrivialImport ordering:
ravigershould be grouped with external packages.The
ravigerimport on line 7 should be placed with other 3rd-party imports (lines 1-4), before the internal@/atoms/import. As per coding guidelines: "Order imports: 3rd-party → library → CAREUI → UI → components → hooks → utils → relative."Proposed fix
import { useAtom } from "jotai"; import { AlertTriangleIcon } from "lucide-react"; import { useEffect } from "react"; import { useTranslation } from "react-i18next"; +import { useQueryParams } from "raviger"; import { developerModeAtom } from "@/atoms/developerMode"; -import { useQueryParams } from "raviger";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Common/ProductionWarningBanner.tsx` around lines 6 - 7, Move the third-party import for useQueryParams from "raviger" to the top group of external/3rd-party imports so it sits with other external packages and before the internal import developerModeAtom from "@/atoms/developerMode"; update the import order in ProductionWarningBanner.tsx to follow the project's convention (3rd-party → library → CAREUI → UI → components → hooks → utils → relative) so useQueryParams is grouped with other external imports.src/components/Auth/Login.tsx (1)
112-122: 🧹 Nitpick | 🔵 TrivialConsider using
setTimeoutinstead ofsetIntervalfor cleaner timer logic.With
[resendOtpCountdown]as a dependency, the effect re-runs on every countdown tick, clearing and re-creating the interval each second. While functionally correct, usingsetTimeoutis a cleaner pattern for this use case:♻️ Proposed refactor using setTimeout
useEffect(() => { if (resendOtpCountdown <= 0) { return; } - const timer = setInterval(() => { + const timer = setTimeout(() => { setResendOtpCountdown((prevTime) => prevTime - 1); }, 1000); - return () => clearInterval(timer); + return () => clearTimeout(timer); }, [resendOtpCountdown]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Auth/Login.tsx` around lines 112 - 122, The effect in Login.tsx re-creates an interval every tick because it depends on resendOtpCountdown; replace the setInterval pattern with a single setTimeout that runs when resendOtpCountdown > 0 to decrement once (call setResendOtpCountdown(prev => prev - 1)), and return a cleanup that clears the timeout; keep resendOtpCountdown in the dependency array so the timeout schedules the next tick until it reaches 0 and ensure you reference the same state updater (setResendOtpCountdown) and the resendOtpCountdown variable in the effect.src/pages/Facility/billing/PaymentReconciliationSheet.tsx (1)
240-253: 🧹 Nitpick | 🔵 TrivialConsider clearing
reference_numberonly when switching to cash.The
form.setValue("reference_number", "")call runs on every dependency change whenisCashPaymentis true, including amount/tenderedAmount changes. While React Hook Form handles this efficiently, you could optimize by only clearing when the payment method actually changes to cash.♻️ Optional refactor to clear only on method switch
+ // Clear reference number when switching to cash payment + useEffect(() => { + if (isCashPayment) { + form.setValue("reference_number", ""); + } + }, [isCashPayment, form]); + // Calculate returned amount when tender amount, amount or payment method changes useEffect(() => { if (isCashPayment) { // For cash payments, calculate change to return form.setValue( "returned_amount", round(Decimal.max(0, tenderedAmount || "0").minus(amount || "0")), ); - form.setValue("reference_number", ""); } else { // For non-cash payments, tendered amount equals payment amount and returned is 0 form.setValue("tendered_amount", amount || "0"); form.setValue("returned_amount", "0"); } }, [tenderedAmount, amount, isCashPayment, form]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/billing/PaymentReconciliationSheet.tsx` around lines 240 - 253, The effect currently clears reference_number on every change when isCashPayment is true; instead track the previous isCashPayment state (e.g., with a useRef like prevIsCashPayment) and only call form.setValue("reference_number", "") when isCashPayment transitions from false to true. Keep the existing calculations for tendered_amount and returned_amount in useEffect (or split into two effects) but only perform the reference_number clear on the method switch by checking prevIsCashPayment.current before clearing and then updating prevIsCashPayment.current = isCashPayment. Reference symbols: useEffect, isCashPayment, form.setValue("reference_number", ""), and prevIsCashPayment (or equivalent ref).src/pages/Appointments/components/PractitionerSelector.tsx (3)
197-199:⚠️ Potential issue | 🟡 Minor
handleSelectAllcan introduce duplicate selections.The function appends all users without checking for duplicates. If some users in the organization are already selected, they'll be added again.
🐛 Filter out already-selected users
const handleSelectAll = (users: UserReadMinimal[]) => { - onSelect([...selected, ...users]); + const newUsers = users.filter((u) => !selected.some((s) => s.id === u.id)); + onSelect([...selected, ...newUsers]); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Appointments/components/PractitionerSelector.tsx` around lines 197 - 199, handleSelectAll currently appends the incoming users to selected without deduplication, which can create duplicate entries; update handleSelectAll to merge selected and users by unique user identifier (e.g., user.id) and pass the deduplicated array to onSelect (use selected, users, onSelect and the user id key to locate and remove duplicates before calling onSelect).
529-555:⚠️ Potential issue | 🟡 MinorClickable element lacks keyboard accessibility.
The clickable
divfor child organization navigation hasonClickbut lacks:
role="button"for screen reader identificationtabIndex={0}for keyboard focusonKeyDownhandler for Enter/Space activationThis prevents keyboard-only users from navigating child organizations.
🛠️ Add accessibility attributes
<div key={organization.id} className="flex items-center gap-3 p-3 rounded-lg hover:bg-gray-50 cursor-pointer" + role="button" + tabIndex={0} onClick={() => handleChildOrganizationClick(organization)} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleChildOrganizationClick(organization); + } + }} >As per coding guidelines: "Ensure keyboard navigation works properly in page components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Appointments/components/PractitionerSelector.tsx` around lines 529 - 555, The clickable organization card using the div with key={organization.id} should be made keyboard-accessible: add role="button" and tabIndex={0} to the div and implement an onKeyDown handler that listens for Enter and Space keys and calls handleChildOrganizationClick(organization) (same action as the existing onClick); keep existing onClick as-is and ensure the handler uses the organization variable passed to the component so keyboard activation mirrors mouse activation.
149-167: 🧹 Nitpick | 🔵 TrivialInefficient caching pattern:
searchQueryin queryKey causes redundant API calls.The
searchQueryis included inqueryKeybut not used inqueryFn. This creates separate cache entries for each unique search term, even though the API returns identical data. Every keystroke with a new character triggers a fresh API request.A more efficient approach would be to cache the full dataset once and filter client-side using
useMemo:♻️ Suggested refactor
// Fetch all practitioners for search functionality const { data: allPractitionersData } = useQuery({ - queryKey: ["allPractitioners", facilityId, searchQuery, showAllOrgs], + queryKey: ["allPractitioners", facilityId, showAllOrgs], queryFn: query(scheduleApi.appointments.availableUsers, { pathParams: { facilityId }, queryParams: { ...(showAllOrgs ? {} : { organization_ids: organizations.map((org) => org.id).join(","), }), }, }), - select: (data: { users: UserReadMinimal[] }) => - data.users.filter((user) => - formatName(user).toLowerCase().includes(searchQuery.toLowerCase()), - ), - enabled: open && !!searchQuery && searchQuery.length > 0, + enabled: open && searchQuery.trim().length > 0, }); + + const allPractitioners = useMemo( + () => + allPractitionersData?.users?.filter((user) => + formatName(user).toLowerCase().includes(searchQuery.toLowerCase()), + ) ?? [], + [allPractitionersData, searchQuery], + );Note: Also added
.trim()to the enabled check to avoid triggering queries for whitespace-only searches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Appointments/components/PractitionerSelector.tsx` around lines 149 - 167, The query includes searchQuery in the queryKey causing redundant cached entries and API calls; remove searchQuery from the queryKey in the useQuery call (keep facilityId and showAllOrgs), change the select to return the raw data.users (no filtering) or omit select, and perform client-side filtering in a useMemo that derives the filtered practitioners from allPractitioners using formatName(...).toLowerCase().includes(searchQuery.toLowerCase()). Also tighten the enabled flag to use open && !!searchQuery.trim() so whitespace-only input doesn't trigger requests; update references to allPractitioners, the useQuery call, queryKey array, queryFn (scheduleApi.appointments.availableUsers), select, and enabled accordingly.src/pages/Facility/services/inventory/externalSupply/deliveryOrder/PrintDeliveryOrder.tsx (1)
126-133:⚠️ Potential issue | 🟠 MajorGuard the template preview on facility readiness.
useCurrentFacility()can still be loading here, butPrintPreviewis mounted immediately with the newfacilityprop. That means this document can render or auto-print without facility branding on a cold load. Please add the sameisFacilityLoading || !facilityguard used inPrintInvoice.As per coding guidelines, "Implement proper loading states and error boundaries in page components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/services/inventory/externalSupply/deliveryOrder/PrintDeliveryOrder.tsx` around lines 126 - 133, The PrintPreview is being mounted before facility data is ready; update PrintDeliveryOrder to mirror PrintInvoice by reading isFacilityLoading from useCurrentFacility and only render PrintPreview when facility is ready (i.e., guard with !isFacilityLoading && facility), otherwise render a loading/placeholder (or keep disabled) to prevent rendering/auto-print without facility branding; locate the useCurrentFacility call and the PrintPreview JSX in PrintDeliveryOrder.tsx and apply this conditional guard.src/pages/Facility/services/pharmacy/AllMedicationBillForm.tsx (1)
254-259:⚠️ Potential issue | 🟠 MajorEditing still collapses multi-instruction regimens to the first entry.
Line 257 only seeds
AddMedicationSheetwith the first dosage instruction, but the save handler writes its returned array back to the fullitems.${editingItemIndex}.dosageInstructionsfield. If a medication already has multiple instructions, opening edit mode here drops every instruction except the first and recalculates lot quantities from that truncated regimen. Either edit a specific instruction in place or block this sheet for multi-instruction rows.As per coding guidelines, "Implement proper data validation for medical information in page components".
Also applies to: 263-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/services/pharmacy/AllMedicationBillForm.tsx` around lines 254 - 259, The code currently passes only the first instruction to AddMedicationSheet via existingDosageInstructions when editing (using editingItemIndex and form.watch(`items.${editingItemIndex}.dosageInstructions`)?.[0]), which truncates multi-instruction regimens on save; update the logic so AddMedicationSheet receives the full dosageInstructions array and the specific instruction index (pass form.watch(`items.${editingItemIndex}.dosageInstructions`) and editingInstructionIndex) and modify the save handler to replace only the edited index in items.${editingItemIndex}.dosageInstructions instead of overwriting the whole array, or alternatively prevent opening the sheet when the array length > 1 and surface a validation message—apply the same change for the related block at lines 263-296.src/pages/Facility/services/pharmacy/PrintMedicationReturn.tsx (1)
124-131:⚠️ Potential issue | 🟠 MajorWait for
facilitybefore mounting the template preview.This preview now depends on
facility, but it never waits foruseCurrentFacility()to resolve. On a cold load the document can render or auto-print without facility branding.As per coding guidelines, "Implement proper loading states and error boundaries in page components".Suggested guard
- const { facility } = useCurrentFacility(); + const { facility, isFacilityLoading } = useCurrentFacility(); + + if (isFacilityLoading || !facility) { + return <Loading />; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/services/pharmacy/PrintMedicationReturn.tsx` around lines 124 - 131, The PrintPreview is mounted before useCurrentFacility() resolves, so guard rendering on facility: in the component wrapping the PrintPreview (where useCurrentFacility() is called) check for facility being defined and render a loading state or null until facility is available; specifically, delay returning the PrintPreview (with props title, disabled, facility, templateSlug) until facility is truthy so branding is present, and ensure any auto-print behavior is suppressed while facility is undefined.src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx (1)
128-165:⚠️ Potential issue | 🟠 MajorDo not merge PRN and scheduled requests for the same product.
isPRNis only captured when the group is first created. If the same medication has both scheduled and PRN requests, later requests are merged into whichever section was seen first, so the chart can render scheduled orders under the PRN table or vice versa.One way to split the groups
- const productId = - med.requested_product?.id || med.medication?.code || med.id; const productName = med.requested_product?.name || med.medication?.display || "Unknown Medication"; const isPRN = med.dosage_instruction.some((di) => di.as_needed_boolean); + const productId = + med.requested_product?.id || med.medication?.code || med.id; + const groupKey = `${productId}:${isPRN ? "prn" : "regular"}`; - if (!groups.has(productId)) { - groups.set(productId, { + if (!groups.has(groupKey)) { + groups.set(groupKey, { productId, productName, requests: [], isPRN, hasActiveRequests: false, }); } - const group = groups.get(productId)!; + const group = groups.get(groupKey)!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx` around lines 128 - 165, The grouping currently uses productId alone (in medicationRequests.results.forEach and the groups Map) and captures isPRN only when the group is created, causing PRN and scheduled requests for the same product to be merged incorrectly; update the grouping key to include PRN state (e.g., use `${productId}:${isPRN ? 'prn' : 'reg'}`) or maintain separate groups per productId+isPRN when creating entries and when retrieving group with groups.get(...), ensure productName still derives from the first item but requests are pushed into the correct PRN-specific group so later filtering (regular/prn) uses the group's isPRN flag reliably.src/pages/Facility/billing/account/components/PrintChargeItems.tsx (1)
136-141:⚠️ Potential issue | 🟠 MajorWait for facility data before rendering the template header.
Line 420 now makes the print header depend on
facility, but this component still never waits foruseCurrentFacility()to resolve. On a cold load the preview can render with missing facility branding before the context is ready.PrintInvoicesalready guards the same template-backed path.🛠️ Suggested fix
- const { facility } = useCurrentFacility(); + const { facility, isFacilityLoading } = useCurrentFacility(); ... + if (isFacilityLoading || !facility) { + return <TableSkeleton count={3} />; + } ... - disabled={isLoading || isLoadingPayments || isLoadingAccount} + disabled={ + isLoading || + isLoadingPayments || + isLoadingAccount || + isFacilityLoading + }Also applies to: 416-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Facility/billing/account/components/PrintChargeItems.tsx` around lines 136 - 141, The component PrintChargeItems currently renders the template header before useCurrentFacility() resolves, causing blank/missing branding; update PrintChargeItems to wait for the facility data (from useCurrentFacility()) before rendering the template-backed header—use the facility value or an isLoadingFacility flag to short-circuit rendering (or return a loader) until facility is available, similar to how PrintInvoices guards the template path; locate the facility usage in PrintChargeItems and wrap the header/template render with a guard that checks facility (or isLoading) to ensure branding is present..github/workflows/playwright.yaml (1)
257-297:⚠️ Potential issue | 🟠 MajorDon't report
✅ Passedwhen a shard failed before writing JSON.This block derives
statusonly from parsedtest-results.json. If a shard fails earlier (backend startup, setup project, browser install, etc.), it contributes zero counts and the PR comment can still show success. Foldneeds.test.resultor missing-shard detection into the status calculation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/playwright.yaml around lines 257 - 297, The merge step (id: merge) currently computes status only from existing test-results.json files (TOTAL_TESTS, PASSED_TESTS, FAILED_TESTS, SKIPPED_TESTS) so if a shard failed before writing JSON the overall status can be erroneously marked Passed; update the merge logic to detect missing or incomplete shards by checking for missing all-results/playwright-results-shard-*/test-results.json files or a failed-shard marker (e.g., a needs.test.result flag or sentinel file produced on early failure) and treat any missing shard or failure marker as a test failure (set FAILED_TESTS>0 or directly set status to Failed) before emitting status to GITHUB_OUTPUT so the final status reflects shards that never wrote test-results.json.src/components/ui/multi-filter/tagFilter.tsx (2)
62-84:⚠️ Potential issue | 🟠 MajorNested tag groups are pruned before users can open them.
isRootLevelnow means “has children”, not “level 0”. For any second-level group, the query is disabled until expansion,loadingChildrenisfalse,hasActiveChildrenisfalse, and this returnsnullimmediately. That makes grouped tags below the first level unreachable.🛠️ Suggested fix
- const isRootLevel = tag.has_children; + const isGroupTag = tag.has_children; + const isRootLevel = level === 0 && isGroupTag; ... - disabled={isRootLevel && allChildrenSelected} + disabled={isGroupTag && allChildrenSelected} ... - if (isRootLevel) { + if (isGroupTag) { setExpanded(!expanded); } else { onTagToggle(tag); } ... - {isRootLevel ? ( + {isGroupTag ? ( <Component className="h-4 w-4 text-black/80" strokeWidth={1.25} /> ) : ( <Checkbox checked={isSelected} className="h-4 w-4" /> )} ... - {isRootLevel && !allChildrenSelected && ( + {isGroupTag && !allChildrenSelected && ( <ChevronRight className={cn( "h-3 w-3 transition-transform", expanded && "rotate-90", )} /> )} ... - {expanded && isRootLevel && ( + {expanded && isGroupTag && ( <div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/multi-filter/tagFilter.tsx` around lines 62 - 84, The component incorrectly uses isRootLevel = tag.has_children to decide pruning, causing non-root groups to be returned null; change the pruning condition to only skip rendering for actual root level groups by replacing the if check with one that uses level (e.g., if (level === 0 && !loadingChildren && !hasActiveChildren) return null) and/or rename isRootLevel to hasChildren to avoid confusion; update references to isRootLevel in this file (tagFilter.tsx) so only level === 0 is used for the early-return logic while leaving the query.enabled logic (tag.has_children && (level === 0 || expanded)) unchanged.
63-74:⚠️ Potential issue | 🟠 MajorAdd
facilityIdto the child-tag query key.The root-level query includes
facilityIdin its key (["tags", resource, search, facilityId]), but this child query does not. Since both queries passfacility: facilityIdin their params, the cache key should be consistent. Without it, switching facilities could cause React Query to serve cached results from the previous facility while the root and sibling queries refetch under different keys.Suggested fix
- queryKey: ["tags", resource, "parent", tag.id], + queryKey: ["tags", resource, "parent", tag.id, facilityId],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/multi-filter/tagFilter.tsx` around lines 63 - 74, The child-tag useQuery currently builds queryKey as ["tags", resource, "parent", tag.id] but passes facility: facilityId in query params; update the queryKey in the useQuery call (the one invoking tagConfigApi.list) to include facilityId (e.g. append facilityId to the array) so the child query cache is namespaced per facility and stays consistent with the root query; keep the same enabled condition (tag.has_children && (level === 0 || expanded)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5e73a20-e795-41e5-80ed-ad7c67826c00
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (107)
.github/workflows/playwright.yaml.gitignoreCLAUDE.mdpackage.jsonplaywright.config.tspublic/locale/en.jsonscripts/playwright-db.shsrc/CAREUI/misc/PrintPreview.tsxsrc/Utils/inventory.tssrc/Utils/schema/computedField.tssrc/Utils/schema/types.tssrc/components/Auth/Login.tsxsrc/components/Billing/ChargeItems/ChargeItemsSection.tsxsrc/components/Common/PrintTable.tsxsrc/components/Common/ProductionWarningBanner.tsxsrc/components/Common/RequirementsSelector.tsxsrc/components/Encounter/EncounterInfoCard.tsxsrc/components/Facility/ConsultationDetails/PrintAllQuestionnaireResponses.tsxsrc/components/Facility/ConsultationDetails/PrintQuestionnaireResponse.tsxsrc/components/Location/BedListing.tsxsrc/components/Location/LocationNavigation.tsxsrc/components/Location/hooks/useLocationNavigation.tssrc/components/Medicine/DosageInstructionList.tsxsrc/components/Medicine/MedicationAdministration/GroupedMedicationRow.tsxsrc/components/Medicine/MedicationAdministration/MedicineAdminForm.tsxsrc/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsxsrc/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsxsrc/components/Medicine/MedicationAdministration/utils.tssrc/components/Medicine/MedicationRequestTable/DispenseHistory.tsxsrc/components/Medicine/MedicationsTable.tsxsrc/components/Medicine/utils.tssrc/components/Patient/TreatmentSummary.tsxsrc/components/Prescription/PrescriptionPreview.tsxsrc/components/Questionnaire/ManageResponseTemplatesSheet.tsxsrc/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsxsrc/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsxsrc/components/Questionnaire/QuestionnaireEditor.tsxsrc/components/Resource/PrintResourceLetter.tsxsrc/components/Users/UserForm.tsxsrc/components/ui/multi-filter/dateFilter.tsxsrc/components/ui/multi-filter/departmentFilter.tsxsrc/components/ui/multi-filter/filterConfigs.tsxsrc/components/ui/multi-filter/tagFilter.tsxsrc/components/ui/sidebar/facility/location/location-switcher.tsxsrc/hooks/useExtensions.tsxsrc/pages/Admin/Roles/RoleForm.tsxsrc/pages/Admin/organizations/components/AdminOrganizationFormSheet.tsxsrc/pages/Appointments/AppointmentPrint.tsxsrc/pages/Appointments/components/PractitionerSelector.tsxsrc/pages/Appointments/components/PrintAppointments.tsxsrc/pages/Facility/billing/PaymentReconciliationSheet.tsxsrc/pages/Facility/billing/account/AccountShow.tsxsrc/pages/Facility/billing/account/components/BedChargeItemsTable.tsxsrc/pages/Facility/billing/account/components/ChargeItemsTable.tsxsrc/pages/Facility/billing/account/components/PrintChargeItems.tsxsrc/pages/Facility/billing/account/utils.tssrc/pages/Facility/billing/invoice/InvoiceShow.tsxsrc/pages/Facility/billing/invoice/PrintInvoice.tsxsrc/pages/Facility/billing/invoice/PrintInvoices.tsxsrc/pages/Facility/billing/paymentReconciliation/PrintPaymentReconciliation.tsxsrc/pages/Facility/locations/LocationContent.tsxsrc/pages/Facility/queues/ManageQueue.tsxsrc/pages/Facility/queues/ManageQueueOngoingTab.tsxsrc/pages/Facility/queues/ServicePointsDropDown.tsxsrc/pages/Facility/services/diagnosticReports/DiagnosticReportPrint.tsxsrc/pages/Facility/services/inventory/InventoryList.tsxsrc/pages/Facility/services/inventory/StockLotSelector.tsxsrc/pages/Facility/services/inventory/SupplyDeliveryTable.tsxsrc/pages/Facility/services/inventory/ToDispatch.tsxsrc/pages/Facility/services/inventory/ToReceive.tsxsrc/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsxsrc/pages/Facility/services/inventory/externalSupply/deliveryOrder/PrintDeliveryOrder.tsxsrc/pages/Facility/services/inventory/externalSupply/requestOrder/PrintRequestOrder.tsxsrc/pages/Facility/services/inventory/externalSupply/requestOrder/RequestOrderShow.tsxsrc/pages/Facility/services/inventory/externalSupply/utils/inventoryUtils.tssrc/pages/Facility/services/pharmacy/AllMedicationBillForm.tsxsrc/pages/Facility/services/pharmacy/DispensedMedicationList.tsxsrc/pages/Facility/services/pharmacy/MedicationBillForm.tsxsrc/pages/Facility/services/pharmacy/MedicationDispenseList.tsxsrc/pages/Facility/services/pharmacy/PrintDispenseOrder.tsxsrc/pages/Facility/services/pharmacy/PrintMedicationReturn.tsxsrc/pages/Facility/services/pharmacy/components/DispensedItemsSheet.tsxsrc/pages/Facility/services/pharmacy/components/MedicationBillRow.tsxsrc/pages/Facility/services/serviceRequests/components/AddMultipleChargeItemsSheet.tsxsrc/pages/Facility/settings/activityDefinition/ActivityDefinitionForm.tsxsrc/pages/Facility/settings/observationDefinition/ObservationDefinitionForm.tsxsrc/pages/Facility/settings/organizations/FacilityOrganizationList.tsxsrc/style/index.csssrc/types/emr/medicationRequest/medicationRequest.tssrc/types/facility/printTemplate.tssrc/types/location/utils.tssrc/types/scheduling/schedule.tstests/PLAYWRIGHT_GUIDE.mdtests/admin/roles/role.spec.tstests/facility/patient/encounter/careTeam/linkCareTeam.spec.tstests/facility/patient/encounter/files/drawings/encounterDrawing.spec.tstests/facility/patient/encounter/medicine/prescriptionEdit.spec.tstests/facility/patient/encounter/structuredQuestions/medicationRequest.spec.tstests/facility/settings/activityDefinition/categoryList.spec.tstests/facility/settings/billing/discount/discountCodeCreate.spec.tstests/facility/settings/chargeItemDefinition/chargeItemDefinitionsCreate.spec.tstests/facility/settings/chargeItemDefinition/chargeItemDefinitionsDelete.spec.tstests/facility/settings/chargeItemDefinition/chargeItemDefinitionsEdit.spec.tstests/facility/settings/departments/departmentUserCreate.spec.tstests/facility/settings/departments/departmentUserManage.spec.tstests/globalSetup.tstests/organization/user/userCreation.spec.ts
💤 Files with no reviewable changes (1)
- src/pages/Facility/locations/LocationContent.tsx
| /* CI workers are controlled per-phase in the workflow (setup=1, chromium=4). | ||
| * Locally, use all available cores. */ | ||
| workers: undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Setup specs:"
rg --files tests -g '*.setup.ts'
echo
echo "Generic Playwright entrypoints:"
rg -nP '(npx\s+)?playwright test(?!.*--project=)' package.json .github/workflows
echo
echo "Relevant config block:"
sed -n '28,70p' playwright.config.tsRepository: ohcnetwork/care_fe
Length of output: 2334
Config removes setup→chromium ordering on CI, breaking ad-hoc test runs.
fullyParallel: false ensures setup specs run serially among themselves, but does not prevent chromium tests from starting in parallel. The dependencies: process.env.CI ? [] : ["setup"] removes the cross-project dependency entirely when CI=true, making npm run playwright:test unsafe on CI. While the workflow mitigates this by running --project=setup separately before chromium, the config itself provides no guarantees for generic entrypoints like "playwright:test": "playwright test" or "playwright:test:headed".
Setup specs have documented ordering dependencies (e.g., patient.setup.ts depends on facility.setup.ts for facilityId), yet CI environments can bypass this if setup ordering is not enforced by the config. Consider restoring the setup dependency for CI environments or documenting that playwright test must never be invoked directly on CI without explicit project sequencing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright.config.ts` around lines 33 - 35, The config removes cross-project
ordering on CI by setting dependencies: process.env.CI ? [] : ["setup"], which
allows chromium projects to start before setup finishes; change the project
dependency so the chromium projects always depend on the "setup" project (i.e.,
set dependencies: ["setup"] for the chromium project(s) instead of an empty
array when process.env.CI is true), keep fullyParallel: false for intra-project
serial setup, and retain workers behavior as intended; update the dependencies
property in playwright.config.ts (and preserve the "setup" project name) so CI
runs cannot bypass setup ordering.
| "add_diagnosis_one": "Add Diagnosis", | ||
| "add_diagnosis_other": "Add another Diagnosis", | ||
| "add_discount": "Add Discount", | ||
| "add_dosage_step": "Add Dosage Step", |
There was a problem hiding this comment.
Append new en.json keys at the end of the file
These new translation keys are inserted throughout the file instead of being appended at the bottom of public/locale/en.json. Please move newly introduced keys to the end to match repository i18n workflow.
As per coding guidelines: "Append missing i18n keys directly to the end of the public/locale/en.json file without reading i18n files".
Also applies to: 512-513, 731-731, 1418-1418, 1804-1804, 1970-1970, 2431-2431, 3321-3321, 3460-3460, 4758-4758, 4886-4886, 5317-5317, 6425-6425, 6457-6457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/locale/en.json` at line 432, New translation keys (e.g.,
"add_dosage_step" and the other newly introduced keys mentioned in the review)
were inserted mid-file instead of being appended; move each newly added key
string to the end of public/locale/en.json so all new i18n keys are appended
only, ensure the JSON remains valid (commas, no duplicate keys), remove the
mid-file entries after appending, and run any i18n/lint checks to verify no
duplicates or formatting errors remain.
| const [imagesReady, setImagesReady] = useState(false); | ||
| const printSectionRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| useEffect(() => { | ||
| setImagesReady(false); | ||
| const node = printSectionRef.current; | ||
| if (!node || props.disabled) return; | ||
|
|
||
| let cancelled = false; | ||
|
|
||
| const waitForImages = async () => { | ||
| const images = Array.from(node.querySelectorAll("img")); | ||
| await Promise.all( | ||
| images.map((img) => | ||
| img.complete | ||
| ? Promise.resolve() | ||
| : img.decode().catch(() => undefined), | ||
| ), | ||
| ); | ||
| if (!cancelled) setImagesReady(true); | ||
| }; | ||
|
|
||
| waitForImages(); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [props.disabled, props.facility]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding content dependency to image loading effect.
The useEffect that waits for images to load depends on [props.disabled, props.facility]. If the children content changes dynamically (e.g., data loads asynchronously), new images may be added that aren't detected. Consider adding a content-related dependency or using a MutationObserver for more robust detection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CAREUI/misc/PrintPreview.tsx` around lines 54 - 81, The effect that waits
for images in PrintPreview (useEffect using printSectionRef, imagesReady and
waitForImages) currently only reruns on props.disabled and props.facility so
dynamically added/changed children/images are missed; update the effect to
either include the content-related dependency (e.g., props.children or whatever
prop supplies the preview HTML) in the dependency array so waitForImages reruns
when content changes, or replace/augment the logic with a MutationObserver
attached to printSectionRef.current that calls waitForImages on
childList/subtree mutations and is properly disconnected in the cleanup; ensure
the observer and cancelled flag respect props.disabled and that you still
setImagesReady(true) only when not cancelled.
| <div className="pointer-events-auto group relative animate-pulse"> | ||
| <div className="absolute -inset-1 rounded-full bg-red-500 opacity-50 blur-sm" /> | ||
| <div className="relative flex items-center gap-2 rounded-full bg-red-600 px-4 py-2 text-white sm:shadow-lg ring-2 ring-red-400 transition-all hover:scale-105 hover:shadow-xl"> | ||
| <AlertTriangleIcon className="h-4 w-4 shrink-0 animate-pulse" /> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant animate-pulse and missing motion preference handling.
-
animate-pulseis applied to both the parent (line 27) and the icon (line 30), creating overlapping animations. Consider removing one. -
For WCAG compliance, respect user motion preferences using Tailwind's
motion-reduce:variant, especially important in healthcare contexts where users may have vestibular disorders.
Proposed fix
- <div className="pointer-events-auto group relative animate-pulse">
+ <div className="pointer-events-auto group relative motion-safe:animate-pulse">
<div className="absolute -inset-1 rounded-full bg-red-500 opacity-50 blur-sm" />
<div className="relative flex items-center gap-2 rounded-full bg-red-600 px-4 py-2 text-white sm:shadow-lg ring-2 ring-red-400 transition-all hover:scale-105 hover:shadow-xl">
- <AlertTriangleIcon className="h-4 w-4 shrink-0 animate-pulse" />
+ <AlertTriangleIcon className="h-4 w-4 shrink-0" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Common/ProductionWarningBanner.tsx` around lines 27 - 30, The
ProductionWarningBanner applies animate-pulse twice (on the parent div and on
AlertTriangleIcon), causing redundant overlapping animation; remove one of the
animate-pulse classes (keep it either on the parent container or the
AlertTriangleIcon) and add Tailwind's motion-reduce handling (e.g., use
motion-reduce:animate-none on whichever element keeps the animation) so the
component respects user prefers-reduced-motion; update the className strings on
the parent div and/or AlertTriangleIcon accordingly.
| <Button variant="link" aria-label={t("patient_home")} asChild> | ||
| <Link | ||
| basePath="/" | ||
| href={`/facility/${facilityId}/patients/home?${new URLSearchParams({ | ||
| phone_number: encounter.patient.phone_number, | ||
| year_of_birth: encounter.patient.year_of_birth?.toString() || "", | ||
| partial_id: encounter.patient.id.slice(0, 5), | ||
| }).toString()}`} | ||
| className="text-gray-700 underline hover:text-gray-900 text-sm font-medium" | ||
| > | ||
| {t("patient_home")} | ||
| </Link> |
There was a problem hiding this comment.
Don't put patient identifiers into this link.
Serializing phone_number, year_of_birth, and partial_id into the query string leaks PHI into browser history, referrer headers, proxy logs, and screenshots. Use an internal identifier or navigation state instead of demographics in the URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Encounter/EncounterInfoCard.tsx` around lines 184 - 195, The
link currently serializes PHI (encounter.patient.phone_number, year_of_birth,
partial_id) into the query string; replace those query params with a non‑PHI
identifier (e.g., encounter.patient.id or a server-issued
internalPatientId/token) or move the data into navigation state/session so no
demographics appear in the URL. Update the Button/Link usage that constructs
href (the Link component and its URLSearchParams call) to only include the safe
internal identifier (or remove href and perform a programmatic navigation that
passes state) and ensure any backend routes expect the internal id instead of
PHI.
| const prescriptionDate = page | ||
| .getByText(/^\d{2}\/\d{2}\/\d{4} \d{2}:\d{2} (AM|PM)$/) | ||
| .first() | ||
| .click(); | ||
| .first(); | ||
| await expect(prescriptionDate).toBeVisible({ timeout: 10000 }); | ||
| await prescriptionDate.click(); |
There was a problem hiding this comment.
Scope the date click to the medication row to avoid flaky mis-clicks.
Line 96 and Line 139 use a global date regex with .first(). If multiple prescriptions are listed, this can open the wrong record and intermittently fail assertions downstream. Anchor the date locator to the row containing medicineName.
🔧 Suggested fix
- const prescriptionDate = page
- .getByText(/^\d{2}\/\d{2}\/\d{4} \d{2}:\d{2} (AM|PM)$/)
- .first();
+ const medicationRow = page.getByRole("row").filter({ hasText: medicineName }).first();
+ const prescriptionDate = medicationRow.getByText(
+ /^\d{2}\/\d{2}\/\d{4} \d{2}:\d{2} (AM|PM)$/,
+ );
await expect(prescriptionDate).toBeVisible({ timeout: 10000 });
await prescriptionDate.click();Also applies to: 139-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/facility/patient/encounter/medicine/prescriptionEdit.spec.ts` around
lines 96 - 100, The date locator is too global and uses .first(), causing flaky
clicks; update the logic that defines prescriptionDate to first locate the
medication row containing the specific medicineName text (anchor by the
medicineName locator), then within that row find the date element matching the
/^\d{2}\/\d{2}\/\d{4 \d{2}:\d{2} (AM|PM)$/ regex and click it; replace both
occurrences that set prescriptionDate (the one at the top and the one around
lines 139-143) so they scope the date lookup to the row containing medicineName
rather than using .first().
| durationData = faker.helpers.arrayElement(DURATION_UNITS); | ||
| durationLabel = durationData.label; | ||
| durationUnit = durationData.value; | ||
| durationUnit = faker.helpers.arrayElement(DURATION_UNITS); |
There was a problem hiding this comment.
Plural-only duration unit handling can make this test flaky.
At Line 115, unit values are plural-only (e.g., "days"), while duration can be 1. This leads to assertions like "1 days" at Lines 160/163 and 277/280, which can fail if the UI renders singular labels ("1 day").
💡 Suggested fix
const DURATION_UNITS = ["days", "hours", "weeks", "months", "years"];
@@
let durationUnit: string;
+ let durationDisplayUnit: string;
let duration: number;
@@
durationUnit = faker.helpers.arrayElement(DURATION_UNITS);
duration = faker.number.int({ min: 1, max: INT_MAX });
+ durationDisplayUnit = duration === 1 ? durationUnit.replace(/s$/, "") : durationUnit;
@@
await page
.getByPlaceholder("Type eg. 5 days, 2 weeks")
- .fill(`${duration} ${durationUnit}`);
+ .fill(`${duration} ${durationDisplayUnit}`);
@@
await page
- .getByRole("option", { name: `${duration} ${durationUnit}` })
+ .getByRole("option", { name: `${duration} ${durationDisplayUnit}` })
.click();
@@
await page
.getByPlaceholder("Type eg. 5 days, 2 weeks")
- .fill(`${duration} ${durationUnit}`);
+ .fill(`${duration} ${durationDisplayUnit}`);
@@
await page
- .getByRole("option", { name: `${duration} ${durationUnit}` })
+ .getByRole("option", { name: `${duration} ${durationDisplayUnit}` })
.click();Also applies to: 160-163, 277-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/facility/patient/encounter/structuredQuestions/medicationRequest.spec.ts`
at line 115, The test picks durationUnit from DURATION_UNITS and can produce
plural-only units like "days" while duration may be 1, causing assertions like
"1 days" to fail; fix by making durationUnit agree with duration — update the
test's setup where durationUnit is assigned (variable durationUnit and array
DURATION_UNITS) to choose a singular form when duration === 1 (or normalize the
final displayed string by pluralizing/singularizing based on duration) so
assertions that compose `${duration} ${durationUnit}` match the UI rendering.
| await expect(page).toHaveURL( | ||
| `/facility/${facilityId}/settings/activity_definitions/categories/f-${facilityId}-${testData.slug}`, | ||
| `/facility/${facilityId}/settings/activity_definitions/categories/f-${facilityId}-${testData.slug}?status=active`, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "categoryList.spec.ts" tests/Repository: ohcnetwork/care_fe
Length of output: 125
🏁 Script executed:
cat -n tests/facility/settings/activityDefinition/categoryList.spec.ts | head -100Repository: ohcnetwork/care_fe
Length of output: 3864
🏁 Script executed:
cat -n tests/facility/settings/activityDefinition/categoryList.spec.ts | sed -n '80,200p'Repository: ohcnetwork/care_fe
Length of output: 4920
🏁 Script executed:
fd -t f "categories" src/pages/facility/settings/ | head -20Repository: ohcnetwork/care_fe
Length of output: 177
🏁 Script executed:
rg "activity_definitions/categories" src/ -t ts -t tsx -A 5 | head -50Repository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
fd -t f "*.spec.ts" tests/facility/overview/ | head -5Repository: ohcnetwork/care_fe
Length of output: 173
🏁 Script executed:
cat -n tests/facility/overview/overview.spec.ts | sed -n '1,50p'Repository: ohcnetwork/care_fe
Length of output: 134
🏁 Script executed:
git ls-files --others --exclude-standard | head -5Repository: ohcnetwork/care_fe
Length of output: 139
🏁 Script executed:
find tests -name "*.spec.ts" -type f | head -20Repository: ohcnetwork/care_fe
Length of output: 1079
🏁 Script executed:
find src -type d -name "*activity*" | head -10Repository: ohcnetwork/care_fe
Length of output: 141
🏁 Script executed:
rg "activity_definitions/categories" src/ -A 3 -B 3 | head -60Repository: ohcnetwork/care_fe
Length of output: 3851
🏁 Script executed:
cat -n src/pages/Facility/settings/activityDefinition/CategoryList.tsx | head -150Repository: ohcnetwork/care_fe
Length of output: 157
🏁 Script executed:
rg "status=active" src/ --no-heading | head -20Repository: ohcnetwork/care_fe
Length of output: 452
🏁 Script executed:
find tests -name "*overview*" -type fRepository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
ls -la src/pages/Facility/settings/activityDefinition/Repository: ohcnetwork/care_fe
Length of output: 498
🏁 Script executed:
rg "status.*active" src/pages/Facility/settings/ --no-headingRepository: ohcnetwork/care_fe
Length of output: 3702
🏁 Script executed:
cat -n src/pages/Facility/settings/activityDefinition/ResourceCategoryList.tsx 2>/dev/null | head -200Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
fd "ResourceCategory" src/pages/Facility/settings/Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
cat -n src/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsx | sed -n '1,100p'Repository: ohcnetwork/care_fe
Length of output: 4120
🏁 Script executed:
rg "navigate.*categories" src/pages/Facility/settings/activityDefinition/ -B 3 -A 3Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
cat -n src/pages/Facility/settings/activityDefinition/ActivityDefinitionList.tsxRepository: ohcnetwork/care_fe
Length of output: 2946
🏁 Script executed:
rg "ResourceCategoryList" src/components/Common/ -lRepository: ohcnetwork/care_fe
Length of output: 108
🏁 Script executed:
cat -n src/components/Common/ResourceCategoryList.tsx | head -200Repository: ohcnetwork/care_fe
Length of output: 7838
🏁 Script executed:
cat -n src/components/Common/ResourceCategoryList.tsx | sed -n '200,400p'Repository: ohcnetwork/care_fe
Length of output: 7426
🏁 Script executed:
rg "status=active" src/components/Common/ResourceCategoryList.tsx -B 5 -A 5Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
cat -n src/pages/Facility/settings/layout.tsx | grep -A 20 "activity_definitions/categories"Repository: ohcnetwork/care_fe
Length of output: 1068
🏁 Script executed:
rg "useQuery|useFilters" src/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsx -B 2 -A 8 | head -100Repository: ohcnetwork/care_fe
Length of output: 1551
🏁 Script executed:
cat -n src/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsx | sed -n '160,250p'Repository: ohcnetwork/care_fe
Length of output: 3345
🏁 Script executed:
rg "navigate.*status" src/pages/Facility/settings/activityDefinition/ -B 3 -A 3Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
rg "defaultQueryParams" src/pages/Facility/settings/activityDefinition/ -B 5 -A 5Repository: ohcnetwork/care_fe
Length of output: 1274
🏁 Script executed:
cat -n src/hooks/useFilters.ts | head -100Repository: ohcnetwork/care_fe
Length of output: 117
🏁 Script executed:
rg "useFilters" src/hooks/useFilters.ts -A 30 | head -60Repository: ohcnetwork/care_fe
Length of output: 125
🏁 Script executed:
fd "useFilters" src/ -type fRepository: ohcnetwork/care_fe
Length of output: 232
🏁 Script executed:
rg "export.*useFilters" src/ -B 2 -A 20 | head -80Repository: ohcnetwork/care_fe
Length of output: 1261
🏁 Script executed:
cat -n src/hooks/useFilters.tsx | sed -n '1,100p'Repository: ohcnetwork/care_fe
Length of output: 3823
🏁 Script executed:
cat -n src/pages/Facility/settings/activityDefinition/ActivityDefinitionList.tsx | sed -n '15,35p'Repository: ohcnetwork/care_fe
Length of output: 734
🏁 Script executed:
cat -n src/hooks/useFilters.tsx | sed -n '78,120p'Repository: ohcnetwork/care_fe
Length of output: 1575
🏁 Script executed:
rg "qParams\." src/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsx | head -20Repository: ohcnetwork/care_fe
Length of output: 452
🏁 Script executed:
cat -n src/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsx | sed -n '180,205p'Repository: ohcnetwork/care_fe
Length of output: 1049
🏁 Script executed:
rg "updateQuery|qParams" src/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsx | head -30Repository: ohcnetwork/care_fe
Length of output: 951
🏁 Script executed:
rg "toContain" tests/ --type ts | head -10Repository: ohcnetwork/care_fe
Length of output: 1111
🏁 Script executed:
rg "\.toHaveURL" tests/ --type ts | head -10Repository: ohcnetwork/care_fe
Length of output: 1048
Use contains-based URL assertions for dynamic query parameters.
The activity definition category pages dynamically add query parameters via the useFilters hook (e.g., status=active, page=1). Exact toHaveURL() assertions will become brittle when filter state changes. Use expect(page.url()).toContain(...) to verify the path and essential params without requiring an exact match.
Applies to lines: 85-87, 123-125, 192-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/facility/settings/activityDefinition/categoryList.spec.ts` around lines
85 - 87, The tests use exact toHaveURL assertions that break when dynamic query
params change; update each assertion that checks the category URL (the ones
referencing page, facilityId and testData.slug) to use a contains-based check
against page.url() instead of toHaveURL(), e.g. assert page.url() contains the
path
`/facility/${facilityId}/settings/activity_definitions/categories/f-${facilityId}-${testData.slug}`
and separately assert it contains the essential query param like `status=active`
if needed; replace the three occurrences that currently call toHaveURL(...) with
these contains-based assertions to avoid brittleness.
| const codeField = page.getByRole("textbox", { name: /code/i }); | ||
| const codeFieldContainer = page.locator("div").filter({ has: codeField }); | ||
|
|
||
| await expect(codeFieldContainer.getByText(/^code$/i)).toBeVisible(); | ||
| await expect(codeField).toHaveAttribute("aria-invalid", "true"); | ||
| await expect(codeFieldContainer.getByText(/required/i)).toBeVisible(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using the shared field error helper for consistency.
The test helpers tests/helper/error provide getFieldErrorMessage for validating form field errors. Using it would align with the patterns documented in tests/PLAYWRIGHT_GUIDE.md.
♻️ Alternative using helper
+import { getFieldErrorMessage } from "tests/helper/error";
+
// In test:
- const codeField = page.getByRole("textbox", { name: /code/i });
- const codeFieldContainer = page.locator("div").filter({ has: codeField });
-
- await expect(codeFieldContainer.getByText(/^code$/i)).toBeVisible();
- await expect(codeField).toHaveAttribute("aria-invalid", "true");
- await expect(codeFieldContainer.getByText(/required/i)).toBeVisible();
+ const codeField = page.getByRole("textbox", { name: /code/i });
+ await expect(codeField).toHaveAttribute("aria-invalid", "true");
+ await expect(getFieldErrorMessage(codeField)).toContainText("required");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/facility/settings/billing/discount/discountCodeCreate.spec.ts` around
lines 69 - 74, Replace the ad-hoc assertions using codeField and
codeFieldContainer with the shared helper: instead of manually checking
visibility and aria-invalid on codeField and its container, call the test helper
getFieldErrorMessage(...) from tests/helper/error to assert the field label and
the "required" error message and verify the aria-invalid state; update the test
around the codeField/codeFieldContainer locators and use getFieldErrorMessage
with the same field identifier to keep consistency with PLAYWRIGHT_GUIDE.md
patterns.
| await page | ||
| .getByRole("combobox") | ||
| .filter({ hasText: "Select organization" }) | ||
| .click(); | ||
| await page.getByPlaceholder("Search organization").fill(userType); | ||
| await page.getByRole("option", { name: userType }).click(); | ||
| await page | ||
| .getByRole("combobox") | ||
| .filter({ hasText: "Select designation" }) | ||
| .click(); | ||
| await page.getByPlaceholder("Search Roles").fill("Member"); | ||
| await page.getByRole("option", { name: "Member" }).click(); |
There was a problem hiding this comment.
Don't feed the new responsibilities picker with the old USER_TYPES set.
This flow no longer targets the old user-type field, but USER_TYPES still includes values like Volunteer and Administrator while the downstream org-link step only uses the seeded ROLES set. That makes the new dropdowns depend on options/designations that may not exist and turns this spec flaky. Reuse the same known-valid role for both steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/facility/settings/departments/departmentUserCreate.spec.ts` around
lines 56 - 67, The test uses the old USER_TYPES value when filling the
organization/designation pickers which makes the new responsibilities picker
flaky; update the spec so the same known-valid seeded role (the ROLES value used
later) is used for both steps instead of USER_TYPES — locate the combobox
interactions that use getByPlaceholder("Search organization") and
getByPlaceholder("Search Roles") and replace the userType input with the seeded
role variable (the same value passed to the downstream org-link step) so both
dropdowns target an option that definitely exists.
| DJANGO_SETTINGS_MODULE=config.settings.local DJANGO_READ_DOT_ENV_FILE=true .venv/bin/python manage.py migrate --verbosity=0 | ||
| DJANGO_SETTINGS_MODULE=config.settings.local DJANGO_READ_DOT_ENV_FILE=true .venv/bin/python manage.py load_fixtures |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded venv path assumes standard setup.
The script assumes the backend uses .venv/bin/python for the virtual environment. If developers use different venv paths (e.g., venv/, env/, or system Python), this will fail.
♻️ Consider making venv path configurable
+PYTHON="${CARE_PYTHON:-$CARE_DIR/.venv/bin/python}"
+
# Run migrations and load fixtures
cd "$CARE_DIR"
-DJANGO_SETTINGS_MODULE=config.settings.local DJANGO_READ_DOT_ENV_FILE=true .venv/bin/python manage.py migrate --verbosity=0
-DJANGO_SETTINGS_MODULE=config.settings.local DJANGO_READ_DOT_ENV_FILE=true .venv/bin/python manage.py load_fixtures
+DJANGO_SETTINGS_MODULE=config.settings.local DJANGO_READ_DOT_ENV_FILE=true "$PYTHON" manage.py migrate --verbosity=0
+DJANGO_SETTINGS_MODULE=config.settings.local DJANGO_READ_DOT_ENV_FILE=true "$PYTHON" manage.py load_fixtures🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/playwright-db.sh` around lines 75 - 76, The script currently
hardcodes the Python interpreter to ".venv/bin/python" for the
DJANGO_SETTINGS_MODULE commands (the manage.py migrate and load_fixtures calls);
make the venv path configurable by introducing a VENV_PYTHON (or similar)
variable that first checks $VENV_PYTHON, then $VIRTUAL_ENV/bin/python, then
common fallbacks like .venv/bin/python and venv/bin/python, and finally falls
back to "python3" from PATH, and use that variable in place of the hardcoded
".venv/bin/python" in the DJANGO_SETTINGS_MODULE ... manage.py migrate and ...
manage.py load_fixtures invocations so developers with different venv layouts or
system Python will work.
| <DropdownMenuItem | ||
| onSelect={(e) => { | ||
| e.preventDefault(); | ||
| if (hasChildren) { | ||
| setExpanded(!expanded); | ||
| } | ||
| onOrgToggle(org); | ||
| }} | ||
| className="flex items-center gap-2 px-2 py-1 cursor-pointer" | ||
| style={{ paddingLeft: `${level * 16 + 8}px` }} | ||
| > | ||
| <div className="flex items-center gap-2 flex-1"> | ||
| <Checkbox checked={isSelected} className="h-4 w-4" /> | ||
| <div className="flex items-center gap-2 flex-1 min-w-0"> | ||
| <Checkbox | ||
| checked={isSelected} | ||
| className="data-[state=checked]:border-primary-700 text-white shrink-0" | ||
| /> | ||
| <div | ||
| className={cn( | ||
| "h-3 w-3 rounded-full shrink-0 border", | ||
| getColorForOrg(org.id, 0), | ||
| )} | ||
| className={cn("size-3 rounded-full border", getColorForOrg(org.id))} | ||
| /> | ||
| <span className="text-sm truncate flex-1">{org.name}</span> | ||
| {hasChildren && ( | ||
| <ChevronRight | ||
| className={cn( | ||
| "h-3 w-3 transition-transform", | ||
| expanded && "rotate-90", | ||
| )} | ||
| /> | ||
| <Button | ||
| variant="ghost" | ||
| size="xs" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setExpanded(!expanded); | ||
| }} | ||
| className="w-12 justify-center -mr-2" | ||
| > | ||
| <ChevronRight | ||
| className={cn("transition-transform", expanded && "rotate-90")} | ||
| /> | ||
| </Button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f departmentFilter.tsxRepository: ohcnetwork/care_fe
Length of output: 113
🏁 Script executed:
cat -n src/components/ui/multi-filter/departmentFilter.tsx | head -150Repository: ohcnetwork/care_fe
Length of output: 5695
🏁 Script executed:
rg -A 5 -B 5 "onOrgToggle\|setExpanded" src/components/ui/multi-filter/departmentFilter.tsxRepository: ohcnetwork/care_fe
Length of output: 44
Expansion requires keyboard handler on the menu item.
The nested chevron Button handles expand/collapse only via mouse click. Keyboard users can select departments but cannot expand child departments from the focused row. Add a keyboard handler (e.g., ArrowRight key) to DropdownMenuItem to toggle expansion, or switch to a submenu pattern that exposes expansion to keyboard focus.
Current implementation (lines 67-97)
<DropdownMenuItem
onSelect={(e) => {
e.preventDefault();
onOrgToggle(org);
}}
className="flex items-center gap-2 px-2 py-1 cursor-pointer"
style={{ paddingLeft: `${level * 16 + 8}px` }}
>
<div className="flex items-center gap-2 flex-1 min-w-0">
<Checkbox
checked={isSelected}
className="data-[state=checked]:border-primary-700 text-white shrink-0"
/>
<div
className={cn("size-3 rounded-full border", getColorForOrg(org.id))}
/>
<span className="text-sm truncate flex-1">{org.name}</span>
{hasChildren && (
<Button
variant="ghost"
size="xs"
onClick={(e) => {
e.stopPropagation();
setExpanded(!expanded);
}}
className="w-12 justify-center -mr-2"
>
<ChevronRight
className={cn("transition-transform", expanded && "rotate-90")}
/>
</Button>
)}
</div>
</DropdownMenuItem>
Per coding guidelines, ensure full keyboard navigation in components for clinical environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/multi-filter/departmentFilter.tsx` around lines 67 - 97,
The DropdownMenuItem currently only toggles expansion via the child Button
click; add an onKeyDown handler on DropdownMenuItem (next to the existing
onSelect) to listen for ArrowRight (to setExpanded(true)) and ArrowLeft (to
setExpanded(false)) and ensure the handler calls
e.preventDefault()/e.stopPropagation() as needed so it doesn't trigger the
onSelect toggle; keep the existing Button click behavior but remove reliance on
it for keyboard users, and ensure Enter/Space still trigger onOrgToggle as
before while Arrow keys only control expansion state via setExpanded and update
the ChevronRight rotation accordingly.
| } = useMultiFilterState(filterConfigs, onFilterUpdate, { | ||
| ...qParams, | ||
| date: | ||
| qParams.date_after || qParams.date_before | ||
| ? { | ||
| from: qParams.date_after ? new Date(qParams.date_after) : undefined, | ||
| to: qParams.date_before ? new Date(qParams.date_before) : undefined, | ||
| } | ||
| : undefined, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 200 src/pages/Facility/services/inventory/ToDispatch.tsx | tail -n 40Repository: ohcnetwork/care_fe
Length of output: 1157
🏁 Script executed:
sed -n '203,210p' src/pages/Facility/services/inventory/ToDispatch.tsxRepository: ohcnetwork/care_fe
Length of output: 335
🏁 Script executed:
sed -n '315,355p' src/pages/Facility/services/inventory/ToDispatch.tsxRepository: ohcnetwork/care_fe
Length of output: 1449
🏁 Script executed:
rg "dateTimeQueryString|parseISO" src/pages/Facility/services/inventory/ToDispatch.tsx -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 843
🏁 Script executed:
rg "dateTimeQueryString" src/ -l --type ts --type tsxRepository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
rg "dateTimeQueryString" src/Utils/ -lRepository: ohcnetwork/care_fe
Length of output: 80
🏁 Script executed:
sed -n '1,50p' src/Utils/utils.ts | head -n 50Repository: ohcnetwork/care_fe
Length of output: 1380
🏁 Script executed:
rg "export.*dateTimeQueryString" src/Utils/ -A 10Repository: ohcnetwork/care_fe
Length of output: 581
🏁 Script executed:
grep -n "function dateTimeQueryString\|const dateTimeQueryString\|export.*dateTimeQueryString" src/Utils/utils.tsRepository: ohcnetwork/care_fe
Length of output: 140
🏁 Script executed:
rg "from \"date-fns\"" src/pages/Facility/services/inventory/ToDispatch.tsxRepository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
rg "parseISO" src/ --type ts --type tsxRepository: ohcnetwork/care_fe
Length of output: 89
🏁 Script executed:
sed -n '86,100p' src/Utils/utils.tsRepository: ohcnetwork/care_fe
Length of output: 524
🏁 Script executed:
cat src/Utils/dayjs.tsRepository: ohcnetwork/care_fe
Length of output: 434
🏁 Script executed:
rg "new Date\(" src/pages/Facility/services/inventory/ToDispatch.tsx -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 1251
🏁 Script executed:
rg "dayjs\(" src/Utils/utils.ts -A 1 | head -n 30Repository: ohcnetwork/care_fe
Length of output: 614
🏁 Script executed:
rg "parseISO\|startOfDay\|endOfDay" src/Utils/ --type tsRepository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
grep -n "import.*dayjs\|import.*date-fns" src/pages/Facility/services/inventory/ToDispatch.tsxRepository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
rg "dayjs\(.*\).toDate\(\)" src/Utils/utils.ts -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 272
🏁 Script executed:
python3 << 'EOF'
# Test to understand the timezone parsing behavior
# In the dateTimeQueryString function, it does: dayjs(date).toDate()
# The issue is what happens when you pass new Date("2026-04-07") to dayjs
print("JavaScript new Date parsing behavior:")
print("new Date('2026-04-07') parses date string as UTC")
print("In negative UTC offset timezones (e.g., EST UTC-5), this becomes previous day")
print()
print("dayjs parsing behavior:")
print("dayjs('2026-04-07') should parse as local date by default")
print()
print("The fix suggestion uses parseISO from date-fns")
print("But parseISO is stricter and may have similar UTC semantics")
print()
print("Better approach: Use dayjs() which is already in the codebase")
print("Instead of: new Date(qParams.date_after)")
print("Use: dayjs(qParams.date_after).toDate()")
EOFRepository: ohcnetwork/care_fe
Length of output: 561
🏁 Script executed:
rg "dateQueryString\|dateTimeQueryString" src/Utils/utils.ts -B 5 -A 15 | head -n 80Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
head -n 1 src/pages/Facility/services/inventory/ToDispatch.tsx && grep -n "^import" src/pages/Facility/services/inventory/ToDispatch.tsx | head -n 20Repository: ohcnetwork/care_fe
Length of output: 1386
🏁 Script executed:
rg "type DateLike\|interface DateLike" src/Utils/Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
sed -n '70,95p' src/Utils/utils.tsRepository: ohcnetwork/care_fe
Length of output: 717
🏁 Script executed:
python3 << 'EOF'
# Verify parseISO behavior with ISO strings
# parseISO("2026-04-07") will parse as UTC midnight and return a Date object
# This is the SAME behavior as new Date("2026-04-07")
# The correct solution is to use dayjs which is already imported in utils.ts
# dayjs("2026-04-07") intelligently handles date-only strings in local time
print("Issue Analysis:")
print("=" * 60)
print()
print("Current code: new Date(qParams.date_after)")
print("Problem: Parses '2026-04-07' as UTC → shifts date in negative offset zones")
print()
print("Review suggestion: parseISO(qParams.date_after)")
print("Issue: parseISO also parses as UTC for ISO strings")
print(" This will NOT fix the timezone problem!")
print()
print("Correct approach: dayjs(qParams.date_after).toDate()")
print("Why: dayjs intelligently parses date-only strings in local timezone")
print()
print("Note: dateTimeQueryString already uses dayjs internally!")
print("So just passing the string directly is cleaner than wrapping with new Date()")
EOFRepository: ohcnetwork/care_fe
Length of output: 661
🏁 Script executed:
rg "export const dateQueryString" src/Utils/utils.ts -A 5Repository: ohcnetwork/care_fe
Length of output: 287
🏁 Script executed:
python3 << 'EOF'
# Verify parseISO behavior
# According to date-fns documentation, parseISO parses ISO 8601 strings
# For a date-only string "2026-04-07", parseISO will:
# - Treat it as an ISO date string
# - Parse it as UTC (same as new Date!)
import subprocess
result = subprocess.run([
'rg', 'import.*parseISO', 'src/',
], capture_output=True, text=True)
print("Usage of parseISO in codebase:")
print(result.stdout if result.stdout else "No imports of parseISO found")
print()
print("Conclusion: parseISO is NOT used in the codebase")
print("date-fns is imported in utils.ts but used minimally")
print("The project primarily uses dayjs for date handling")
EOFRepository: ohcnetwork/care_fe
Length of output: 434
🏁 Script executed:
rg "import.*from.*date-fns" src/ --max-count=10Repository: ohcnetwork/care_fe
Length of output: 9253
🏁 Script executed:
rg "DateLike.*=" src/Utils/utils.ts -B 2Repository: ohcnetwork/care_fe
Length of output: 628
🏁 Script executed:
python3 << 'EOF'
import json
# Document the timezone issue and proposed solutions
analysis = {
"issue": "new Date('2026-04-07') in negative UTC zones shifts date backward",
"current_code": "new Date(qParams.date_after)",
"review_suggestion": "parseISO(qParams.date_after) from date-fns",
"review_suggestion_flaw": "parseISO also parses as UTC - does NOT fix the issue",
"available_options": {
"option_1": {
"code": "dayjs(qParams.date_after).toDate()",
"description": "Uses existing dayjs which intelligently handles local dates",
"already_in_codebase": True
},
"option_2": {
"code": "dateTimeQueryString(qParams.date_after)",
"description": "Pass string directly to dateTimeQueryString which uses dayjs",
"already_in_codebase": True,
"most_elegant": True
},
"option_3": {
"code": "parseISO(qParams.date_after)",
"description": "From date-fns, but has same UTC parsing issue",
"fixes_issue": False
}
}
}
print(json.dumps(analysis, indent=2))
EOFRepository: ohcnetwork/care_fe
Length of output: 968
🏁 Script executed:
sed -n '165,190p' src/pages/Facility/services/inventory/ToDispatch.tsxRepository: ohcnetwork/care_fe
Length of output: 706
🏁 Script executed:
sed -n '1,20p' src/pages/Facility/services/inventory/ToDispatch.tsxRepository: ohcnetwork/care_fe
Length of output: 797
🏁 Script executed:
rg "import dayjs\|from.*dayjs" src/pages/Facility/services/inventory/ToDispatch.tsxRepository: ohcnetwork/care_fe
Length of output: 44
Replace new Date() with dayjs() for date-only query parameters.
new Date("2026-04-07") parses date strings as UTC, shifting the date backward by one day in negative-offset time zones. Both filter rehydration and API query bounds are affected. Use dayjs() instead, which intelligently handles local dates:
For filter rehydration (lines 175-184, 315-328):
- from: qParams.date_after ? new Date(qParams.date_after) : undefined,
+ from: qParams.date_after ? dayjs(qParams.date_after).toDate() : undefined,
- to: qParams.date_before ? new Date(qParams.date_before) : undefined,
+ to: qParams.date_before ? dayjs(qParams.date_before).toDate() : undefined,For API query construction (lines 203-208, 347-352), pass the string directly to dateTimeQueryString which uses dayjs internally:
- date_after: qParams.date_after ? dateTimeQueryString(new Date(qParams.date_after)) : undefined,
+ date_after: qParams.date_after ? dateTimeQueryString(qParams.date_after) : undefined,
- date_before: qParams.date_before ? dateTimeQueryString(new Date(qParams.date_before), true) : undefined,
+ date_before: qParams.date_before ? dateTimeQueryString(qParams.date_before, true) : undefined,Apply the same changes to created_date_* blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/services/inventory/ToDispatch.tsx` around lines 175 - 184,
The filter rehydration currently constructs Date objects via new Date(...)
(inside the useMultiFilterState call and the created_date_* blocks) which causes
UTC shifts; replace those with dayjs(...) to create local-date-aware values
(e.g., use dayjs(qParams.date_after) / dayjs(qParams.date_before) instead of new
Date(...)) and update any other rehydration sites for created_date_*
accordingly; for API query construction (where dateTimeQueryString is invoked in
ToDispatch), stop converting to Date and instead pass the original date string
(or dayjs instance string) into dateTimeQueryString so it can parse with dayjs
internally (update calls around dateTimeQueryString and any created_date_* query
builders to accept the string/dayjs input).
| await page.goto( | ||
| `/facility/${facilityId}/settings/billing/discount_configuration`, | ||
| ); | ||
| await page.waitForLoadState("networkidle"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider avoiding waitForLoadState("networkidle").
Using networkidle can be flaky because it waits for no network activity for 500ms, which may not reliably indicate the page is ready. Consider waiting for specific elements or using waitForLoadState("domcontentloaded") instead, or better yet, rely on Playwright's auto-waiting when interacting with elements.
♻️ Suggested alternative
- await page.waitForLoadState("networkidle");
+ // Remove networkidle waits - Playwright's auto-wait on element interactions is preferred
+ // The subsequent expect/click calls will auto-wait for elementsOr if you need to ensure page stability:
await page.waitForLoadState("domcontentloaded");Also applies to: 41-41, 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/facility/settings/billing/discount/discountCodeCreate.spec.ts` at line
16, Replace flaky uses of page.waitForLoadState("networkidle") in the tests with
a more reliable wait: either use page.waitForLoadState("domcontentloaded") or,
preferably, wait for specific elements/locators to be ready (e.g.,
page.waitForSelector or locator.waitFor for the button, form, or input you
interact with) or rely on Playwright's built-in auto-waiting when performing
actions; update each occurrence of page.waitForLoadState("networkidle") in
discountCodeCreate.spec.ts (and the other occurrences noted) to one of these
approaches so the test waits for a concrete element/state rather than network
idle.
|
|
||
| ## Authentication | ||
|
|
||
| Use one of these storage states depending on the role needed: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to code block.
The code block at line 54 is missing a language specifier, which affects syntax highlighting and triggers a markdownlint warning.
📝 Fix markdown formatting
```typescript
// Most tests use admin
test.use({ storageState: "tests/.auth/user.json" });
-```
+```
+
// Nurse-specific testsAlso add blank lines before/after code blocks at lines 16, 27, and 102 to satisfy markdownlint MD031.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 54-54: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/PLAYWRIGHT_GUIDE.md` around lines 52 - 55, Add a language specifier and
fix surrounding blank lines for the Playwright code block containing the line
test.use({ storageState: "tests/.auth/user.json" });: replace the plain code
fence with ```typescript before that block (so syntax highlighting applies) and
ensure there is a blank line before the opening fence and a blank line after the
closing fence; also add blank lines before/after the other code blocks
referenced near the comments "Most tests use admin", the "Nurse-specific tests"
block, and the block around line 102 to satisfy markdownlint MD031.
Proposed Changes
Fixes #15877
Relevant Screenshots:
Truncated name:
Tootip:
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores