CCN beckn referral - #16676
Conversation
Entire-Checkpoint: bc6a6d40b35a
… a scale Replaced 90 arbitrary font sizes (53 of them sub-pixel: 12.5px, 13.5px, 10.5px, 14.5px, 11.5px, 9.5px) with Tailwind scale classes. A single screen rendered 8-11 distinct sizes with no ratio between them. Scale is now 10 / xs(12) / sm(14) / base(16) / lg(18) plus three display sizes. Max visual change is 1.5px. Entire-Checkpoint: ee2aa8184c79
The patient portal is mobile-first but several primary controls were well under the 44px tap minimum: 'See all' at 20px (below even the WCAG 2.2 AA 24px floor), Records/Visits header tabs at 34px, filter chips at 33px, the patient switcher chip at 42px, and the logo home link at 28px. All now min-h-11. Filter chips lose their vertical padding in favour of the min-height so the pill keeps its proportions. Entire-Checkpoint: fd291f30c9fd
View Details, Reschedule and Cancel Appointment used Button size="sm" (h-8, 32px). These are the primary actions on the upcoming appointment card, so they get a min-h-11 floor. Same for the cancel dialog's confirm/dismiss pair. Entire-Checkpoint: 007b8d19f32c
Entire-Checkpoint: c316d7900a98
Entire-Checkpoint: 40edbd0d886e
Entire-Checkpoint: ed0463f3ed2f
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Grumpy Review 🔥
71 files, 6,884 added lines, and a description that says DO NOT REVIEW/MERGE. So naturally, here I am. Reviewing it.
Credit where it is grudgingly due: the Beckn parsers are genuinely defensive, the phase state machine is readable, and someone actually wrote comments explaining why rather than restating the code. That is rarer than it should be.
What bothers me, roughly in order of how much:
- Unbounded slice polling — a 1.2s interval with no deadline and no
awaitinggate. One silent BPP and the browser politely DDoSes your own API forever. - Sticky slice cache — first payload per action wins permanently, so retries render stale catalogs and slots.
- Index-aligned offer↔resource pairing with a fallback to
resources[0]. In an appointment booking flow. Guessing is not a matching strategy. - A round-trip verification that fabricates its own value when the real one is missing. That is not verification, that is optimism with extra steps.
- Assorted smaller sins: unpaginated
limit: 100, an i18n key masquerading as a user-facing string,crypto.randomUUID()with no secure-context guard.
Also: .env now points at develop-api. I assume that is deliberate for the preview branch, but do not let it ride to develop.
None of this is unfixable, and for a WIP preview branch it is a lot better than I expected. Which I resent slightly.> Generated by Grumpy PR Reviewer for #16676 · opus50 · 86.5 AIC · ⌖ 2.17 AIC · ⊞ 8.7K
|
|
||
| // Stop polling once the awaited callback (matching `ON_*`) or a terminal error | ||
| // arrives — it's now the FE's turn (or the flow has failed). | ||
| useEffect(() => { |
There was a problem hiding this comment.
This slice poller has no off switch. refetchInterval fires every 1.2s forever while ready is false, and unlike the status query it ignores awaiting — so a BPP that never sends the slice payload leaves the browser hammering the API until the tab is closed. Add a max attempt/deadline, or gate it on awaiting like the status query.
| const data = sliceQuery.data; | ||
| if (data?.ready && sliceAction) { | ||
| setSlices((prev) => | ||
| prev[sliceAction] ? prev : { ...prev, [sliceAction]: data }, |
There was a problem hiding this comment.
First slice wins, forever. prev[sliceAction] ? prev : ... means a re-run of the same action (start over, re-select, retry) keeps the stale payload and silently renders yesterday's catalog/slots. If the intent is "never overwrite", it needs to be keyed per attempt, not per action name.
| queryKey: ["ccn-resource-list", facilityId], | ||
| queryFn: query(resourceRequestApi.list, { | ||
| // Only requests this facility is receiving (assigned to). | ||
| queryParams: { limit: 100, assigned_facility: facilityId }, |
There was a problem hiding this comment.
limit: 100 with no pagination. Coordination desk gets busy, request 101 vanishes into the void, and nobody knows why. Either paginate or at least surface that the list is truncated.
|
|
||
| iterable.forEach((rawOffer, oi) => { | ||
| const offer = asRecord(rawOffer); | ||
| const resource = asRecord(resources[oi] ?? resources[0]); |
There was a problem hiding this comment.
resources[oi] ?? resources[0] — index-aligning offers to resources and then shrugging to the first one is a coin flip. If the arrays ever disagree you will happily book an appointment against the wrong resource, and nothing will complain. Match on an actual id.
| toast.success(t("send_otp_success")); | ||
| }, | ||
| onError: (error: unknown) => { | ||
| setPhoneError(extractOtpErrorMessage(error, "send_otp_error")); |
There was a problem hiding this comment.
"send_otp_error" is a translation key being passed as a fallback message string, and extractOtpErrorMessage returns raw backend text otherwise. So the user sees either a raw API string or the literal token send_otp_error. Pick one: translate the fallback with t(), or make the whole thing key-based.
| const referral = asRecord(attrs?.referral); | ||
| return { | ||
| contractId: asString(referral?.id) ?? asString(contract?.id), | ||
| coordinationId: asString(attrs?.coordinationId) ?? asString(contract?.id), |
There was a problem hiding this comment.
coordinationId: asString(attrs?.coordinationId) ?? asString(contract?.id) — falling back to the contract id defeats the entire stated purpose of this field, which the comment says is to verify the FE-generated coordinationId round-tripped. A verification that manufactures a plausible value when the real one is missing verifies nothing. Leave it undefined.
| ): string { | ||
| const hasMin = min !== null && min !== undefined; | ||
| const hasMax = max !== null && max !== undefined; | ||
| if (hasMin && hasMax) return `${min} - ${max}`; |
There was a problem hiding this comment.
formatRangeBounds interpolates raw values, so a min of 0 works but an empty-string min sneaks past the != null check and renders "> ". toNumber already handles "" — reuse that instead of hand-rolling a second, laxer emptiness rule in the same file.
| const doSubmit = useCallback(() => { | ||
| if (!option || !transactionId) return; | ||
| if (isConsultation) { | ||
| const newCoordinationId = crypto.randomUUID(); |
There was a problem hiding this comment.
crypto.randomUUID() is only available on secure contexts. Fine in prod, but any (redacted) dev/staging host will throw here mid-flow with no error handling around it. A guarded fallback costs three lines.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 71 changed files in this pull request and generated no new comments.
Suppressed comments (6)
.env:12
- Changing the committed
.envdefaultREACT_CARE_API_URLfromhttps://careapi.ohc.networktohttps://develop-api.ohc.networkchanges the out-of-the-box backend target for anyone running the repo, and diverges from the documented fallback in README/.example.env. If this is only for a branch preview, consider reverting this change and using.env.localorREACT_CARE_URL_MAPinstead.
# Care API URL without the /api prefix
REACT_CARE_API_URL=https://develop-api.ohc.network
src/components/Resource/CcnConsole.tsx:95
- The list view shows the raw
item.statusvalue, while the detail view uses the localizedresource_request_status__…key. This makes the sidebar inconsistent and potentially non-localized. Use the same translation key here too.
<Badge
variant="secondary"
className="shrink-0 text-[10px]"
>
{item.status}
</Badge>
src/pages/PublicAppointments/PatientRegistration.tsx:194
reasoncomes from query params and can be undefined; callingreason.trim()will throw and break patient registration/booking. Trim after defaulting to an empty string instead.
src/Utils/observationRange.ts:88formatRangeBoundstreats empty-string bounds as present (because it only checks null/undefined), which can render>/<even though the docstring says it should return an empty string when neither bound is present. This also contradicts the earlier defensive handling that treats "" as missing.
const hasMin = min !== null && min !== undefined;
const hasMax = max !== null && max !== undefined;
if (hasMin && hasMax) return `${min} - ${max}`;
if (hasMin) return `> ${min}`;
if (hasMax) return `< ${max}`;
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:38
- The logo is an interactive navigation control but is implemented as an
<img>withonClick, which is not keyboard-focusable and won’t be announced as a control by assistive tech. Wrap it in a<button>(or a link) with an accessible label.
src/hooks/useBecknTransaction.ts:167 - If
callApi(becknApi.action, …)throws (network error, 5xx, etc.),awaitingis never cleared (it’s set before the call, and only cleared on certainres.resultvalues). This can leave the UI stuck in a perpetual "busy" state with polling paused (especially fordiscover, before atransactionIdexists). Add acatchto set an error and clearawaiting.
There was a problem hiding this comment.
Ah yes, 71 files, +6919/-2460, labelled "DO NOT REVIEW/MERGE", and here I am anyway. Fine.
Credit where it's grudgingly due: the Beckn layer is genuinely well structured — defensive parsers instead of blind schema trust, a single hook owning the transaction lifecycle, and actual doc comments explaining why. That's more than most of what crosses my desk.
What still bothers me:
.envnow ships a dev API URL as the committed default. Preview branch or not, that's the kind of thing that survives a rebase and ruins someone's afternoon.- Polling has no brakes. The slice query loops every 1.2s until
ready, forever, with no terminal/NACK guard or attempt cap. Networks fail; your poller doesn't know that. resume()is half-finished — new transactionId, stale slices, no poller. It will do the wrong thing the first time anyone uses it.crypto.randomUUID()throws in insecure contexts. Hospital LANs are not famous for their TLS discipline.discoverobject prop defeatsuseCallbackmemoization; an infinite-discover loop is prevented only by a ref. That's a trap, not a design.- Missing error states on the CCN console query and an unpaginated
limit: 100.
None of it is merge-blocking today since this is explicitly a preview branch, but every one of these will bite in production. Commenting, not blocking.
— filed reluctantly, from behind reading glasses> Generated by Grumpy PR Reviewer for #16676 · opus50 · 94.4 AIC · ⌖ 2.14 AIC · ⊞ 8.7K
|
|
||
| # Care API URL without the /api prefix | ||
| REACT_CARE_API_URL=https://careapi.ohc.network | ||
| REACT_CARE_API_URL=https://develop-api.ohc.network |
There was a problem hiding this comment.
Pointing the committed default API URL at develop-api.ohc.network in .env? Congratulations, every fresh clone now silently talks to a dev backend. Even for a preview branch this belongs in a local/preview-only override, not the checked-in default.
| const startDiscover = useCallback(() => { | ||
| void act("discover", buildDiscoverBody({ serviceType, ...discover })); | ||
| }, [act, serviceType, discover]); | ||
|
|
There was a problem hiding this comment.
discover is an object literal prop, so it's a brand-new reference on every parent render — startDiscover never memoizes, and the effect below depends on it. It's only saved from an infinite discover loop by startedRef. Depend on the primitive fields (discover.textSearch, discover.healthServiceType) instead of relying on a ref as a guardrail.
| if (!option || !transactionId) return; | ||
| if (isConsultation) { | ||
| const newCoordinationId = crypto.randomUUID(); | ||
| setCoordinationId(newCoordinationId); |
There was a problem hiding this comment.
crypto.randomUUID() is undefined in non-secure contexts (plain http on a LAN IP, which hospital deployments love). This will throw mid-submit with no error handling. Use a guarded fallback, or better, let the backend mint the coordination id.
| }), | ||
| enabled: !!transactionId && !!sliceAction, | ||
| refetchInterval: (q) => (q.state.data?.ready ? false : SLICE_POLL_MS), | ||
| }); |
There was a problem hiding this comment.
This slice poll has no stop condition other than ready. If the BE never flips ready (NACK, dropped callback, whatever), you poll every 1.2s forever until the user navigates away. Add a NACK/ERROR/terminal guard or an attempt cap.
| if (data?.ready && sliceAction) { | ||
| setSlices((prev) => | ||
| prev[sliceAction] ? prev : { ...prev, [sliceAction]: data }, | ||
| ); |
There was a problem hiding this comment.
prev[sliceAction] ? prev : ... means a slice is cached once and never refreshed. Fine for a one-shot flow, but if the same action fires twice (retry, start-over without reset) the UI silently shows the stale catalog/slots. At minimum key this by transactionId so a new txn can't inherit the old one's slices.
| // console can fire `confirm` and poll a transaction it did not discover. | ||
| const resume = useCallback((existingTransactionId: string) => { | ||
| setTransactionId(existingTransactionId); | ||
| setError(undefined); |
There was a problem hiding this comment.
resume sets a new transactionId but leaves slices from the previous transaction untouched, and doesn't set awaiting, so the status poller never starts. Resuming into a stale catalog with no polling is a fun bug to debug at 2am. Clear slices here.
| queryFn: query(resourceRequestApi.list, { | ||
| // Only requests this facility is receiving (assigned to). | ||
| queryParams: { limit: 100, assigned_facility: facilityId }, | ||
| }), |
There was a problem hiding this comment.
limit: 100 with no pagination and no error state. What happens on request 101, or when this query fails? The user stares at an empty sidebar and assumes there's nothing to do. Add pagination (or at least a "showing first 100" hint) and handle isError.
| <div className="grid grid-cols-1 gap-2 sm:grid-cols-2"> | ||
| {slots.map((slot, index) => { | ||
| const key = slot.id ?? String(index); | ||
| const active = value === key; |
There was a problem hiding this comment.
slot.id ?? String(index) as the React key and the selection identity: if the BPP returns slots without ids, the selected slot silently shifts when the list re-orders. Also, these are semantically radio buttons — plain <button>s give screen readers nothing about selected state. Add aria-pressed/role="radio".
| import resourceRequestApi from "@/types/resourceRequest/resourceRequestApi"; | ||
|
|
||
| // Health service types the coordinator can book an appointment for. | ||
|
|
There was a problem hiding this comment.
A dangling comment with nothing under it. Either it documents HEALTH_SERVICE_TYPES (which lives in another file) or it's leftover — delete it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 71 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/components/Resource/CcnConsole.tsx:95
- The request status in the list view is rendered as the raw enum value (
item.status), but the detail view correctly uses the i18n keyresource_request_status__*. This makes the list inconsistent and potentially non-localized.
<Badge
variant="secondary"
className="shrink-0 text-[10px]"
>
{item.status}
</Badge>
src/components/Resource/beckn/SlotPicker.tsx:38
- These slot buttons behave like a single-select control but don’t expose state to assistive tech. Add
aria-pressed(oraria-selectedwith appropriate roles) so screen readers can announce which slot is currently selected.
<button
key={key}
type="button"
disabled={disabled}
onClick={() => onChange(slot, key)}
className={[
.env:12
- Committing a different default
REACT_CARE_API_URLin the tracked.envchanges the out-of-the-box backend target for everyone pulling this branch, which can be surprising and impact local dev/CI that relies on the previous default.
REACT_CARE_API_URL=https://develop-api.ohc.network
src/pages/PublicAppointments/PatientRegistration.tsx:194
reasoncomes fromuseQueryParams()and can beundefined; callingreason.trim()will throw at runtime. Also,trim()always returns a string so?? ""is redundant.
src/components/Patient/AppointmentTokenPass.tsx:57- This label uses
t(appointment.resource_type, { count: 1 }), which doesn’t match the established translation key pattern used elsewhere for schedulable resources (schedulable_resource__${resource_type}), e.g.src/pages/Appointments/AppointmentDetail.tsx:731. Using a different key risks missing translations.
<Field label={t(appointment.resource_type, { count: 1 })}>
src/hooks/useBecknTransaction.ts:69
- This introduces a fairly complex async polling workflow (Beckn transaction orchestration) without any accompanying automated tests. Given the number of states (awaiting, slice readiness, terminal error statuses), adding Playwright coverage (or at least unit tests around the hook’s state transitions) would help prevent regressions.
Select was carrying appointmentWindowStart/End on contractAttributes. Per the network's reference payload the requested window belongs in performance[], alongside the health service type, and the contract keeps only the service type itself. This is also the shape select gets back: on_select answers with the concrete bookable slots in performance[], which extractSlots already reads. Request and response now speak the same structure. Adds the HealthPerformance JSON-LD context to the select schemaContext -- onyx validates every *Attributes object and NACKs without it -- and splits the per-flow bodies into early returns, matching buildConfirmBody, rather than running a third parallel ternary through a shared literal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 71 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/components/Resource/CcnConsole.tsx:94
item.statusis rendered as a raw enum value in the list, while the detail view correctly uses the localizedresource_request_status__…key. This makes the UI inconsistent and bypasses i18n.
<Badge
variant="secondary"
className="shrink-0 text-[10px]"
>
{item.status}
src/pages/PublicAppointments/PatientRegistration.tsx:194
reasoncomes from query params and can be undefined; callingtrim()will throw and break registration/booking when noreasonis present. Use optional chaining (or default to an empty string) before trimming.
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:38- The logo is rendered as a clickable
, which is not keyboard-accessible and doesn’t convey button semantics to assistive tech. Wrap it in a (or a link) and move the click handler there.
There was a problem hiding this comment.
Grumpy Review 🔥
71 files, ~7k added lines, marked WIP and DO NOT MERGE. Fine — I reviewed the changed lines anyway, because that is apparently my life now.
The credit where it is due: the doc comments on useBecknTransaction and BecknFlow actually explain why, not just what. observationRange.ts defensively coerces API strings to numbers instead of trusting the type declaration. Someone here has been burned before. Good.
The parts that will page someone at 3am:
.envnow points the default build atdevelop-api.ohc.network. Preview branch or not, that is a merge accident waiting to happen.- Both Beckn pollers run unbounded. No timeout, no attempt cap. A BPP that never calls back = infinite request loop plus a permanent spinner with no escape hatch, in a referral booking flow.
crypto.randomUUID()with no fallback, called mid-transaction.CcnConsolefetcheslimit: 100with no pagination — referral 101 just does not exist as far as the coordinator is concerned.usePatientOtpLoginleaks a raw i18n key into user-facing error text.PatientUserProvidersplits auth routing across two effects and a render-time null. Redirect loops love that pattern.
Also noted, and not commented on individually: zero tests for a 7k-line feature touching authentication and clinical referral flows. The merge checklist boxes are all unticked, so I assume you already know.
Non-blocking, since you told me not to review it. Fix the polling and the .env before this goes anywhere near develop.> Generated by Grumpy PR Reviewer for #16676 · opus50 · 103.2 AIC · ⌖ 2.23 AIC · ⊞ 8.7K
|
|
||
| # Care API URL without the /api prefix | ||
| REACT_CARE_API_URL=https://careapi.ohc.network | ||
| REACT_CARE_API_URL=https://develop-api.ohc.network |
There was a problem hiding this comment.
Committing REACT_CARE_API_URL=(developapi.ohc.network/redacted) to the checked-in .env points every default build at the develop API. Even for a preview branch, this is exactly the kind of change that survives a careless merge and quietly repoints production tooling. Use a preview-specific env override instead.
| const startDiscover = useCallback(() => { | ||
| void act("discover", buildDiscoverBody({ serviceType, ...discover })); | ||
| }, [act, serviceType, discover]); | ||
|
|
There was a problem hiding this comment.
discover is an object prop, so this useCallback dep is a fresh reference on every parent render. startDiscover therefore changes every render, which re-runs the autoStart effect below — the only thing saving you from a request storm is the startedRef guard. Depend on the primitive fields (discover.textSearch, discover.healthServiceType) instead of the object.
| if (!option || !transactionId) return; | ||
| if (isConsultation) { | ||
| const newCoordinationId = crypto.randomUUID(); | ||
| setCoordinationId(newCoordinationId); |
There was a problem hiding this comment.
crypto.randomUUID() is only available in secure contexts. Any non-HTTPS deployment (or an older Safari) throws right here, in the middle of placing a referral, with no fallback and no try/catch. Guard it or generate the id server-side.
| }), | ||
| enabled: !!transactionId, | ||
| refetchInterval: awaiting ? POLL_MS : false, | ||
| }); |
There was a problem hiding this comment.
Polling every 1.5s with no timeout and no attempt cap. If the BPP never sends the on_* callback, this hammers the status endpoint forever while the user stares at a spinner. Add a max-attempts / deadline that flips the flow into the error state.
| }), | ||
| enabled: !!transactionId && !!sliceAction, | ||
| refetchInterval: (q) => (q.state.data?.ready ? false : SLICE_POLL_MS), | ||
| }); |
There was a problem hiding this comment.
Same unbounded-poll problem here, plus refetchInterval keeps firing whenever ready is falsy — including when the response is malformed. A never-ready slice means an infinite 1.2s request loop for the lifetime of the page.
| queryFn: query(resourceRequestApi.list, { | ||
| // Only requests this facility is receiving (assigned to). | ||
| queryParams: { limit: 100, assigned_facility: facilityId }, | ||
| }), |
There was a problem hiding this comment.
limit: 100 with no pagination and no status filter. The moment a busy facility has 101 incoming referrals, request 101 silently vanishes from the coordinator's console. Silent data loss in a clinical queue is not a nitpick.
| } | ||
| return response; | ||
| }, | ||
| onSuccess: (response: LoginByOtpResponse) => { |
There was a problem hiding this comment.
setPhoneError(extractOtpErrorMessage(error, "send_otp_error")) passes a raw i18n key as the fallback, so on an unrecognised error shape the user gets the literal string send_otp_error in the UI. It should be t("send_otp_error"). Also: this happily renders backend-supplied strings straight into the form — fine for text, but do not let that habit spread.
| enabled: !!tokenData?.token, | ||
| }); | ||
|
|
||
| const patients = useMemo(() => userData?.results ?? [], [userData]); |
There was a problem hiding this comment.
Two navigation effects plus a render-time return null when !tokenData: the redirect effect never runs on the first render path that matters, and the second effect can fire a select-profile redirect on a transient empty patients list. Consolidate the auth/profile routing into one guarded effect — split navigation logic is how redirect loops are born.
| // than the raw status — means the gaps between submit and callback show a | ||
| // loader instead of a stale interactive state. | ||
| const busy = flow.acting || !!flow.awaiting; | ||
| const showPicker = !busy && phase === "discovered"; |
There was a problem hiding this comment.
busy collapses "POST in flight" and "awaiting callback" into one boolean that hides the entire UI behind a spinner. With no polling timeout upstream, a dropped callback leaves the user with a permanent spinner and no way out — not even the Start Over button, since it is rendered only in the error branch.
Reverts 228c53a, which carried the window on CatalogOption. The option is a flat descriptor of what the user picked; the window is now read back out of the on_discover slice on demand, keyed by the option's catalog-offer index, so nothing about the selection has to be memoised for it. The format was also wrong. The BPP publishes availabilitySchedule times as bare times of day ("22:00"), and the old helper only appended the offset, producing "22:00+05:30" — not a timestamp, and not parseable as one. They are now stamped with today's date in IST to give "2026-07-02T10:00:00+05:30", per the network's reference payload. An overnight window rolls its end to the following day, values that already carry a date keep it, and anything unrecognised passes through untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Entire-Checkpoint: 25765807dacf
…ork/care_fe into amjithtitus09-ccn-beckn-referral
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 72 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/pages/PublicAppointments/PatientRegistration.tsx:194
reasoncomes from query params and can be undefined; callingreason.trim()will throw at runtime and block booking flow after patient creation. Use optional chaining (or default to an empty string) before trimming.
src/pages/PublicAppointments/Success.tsx:96navigator.clipboard.writeTextcan throw (e.g. permission denied / insecure context), which would result in an unhandled rejection and no user feedback. Wrap clipboard copy in try/catch and surface a fallback toast on failure.
src/pages/Encounters/tabs/overview/summary-panel-actions.tab.tsx:61- This action can navigate to a URL containing "undefined" if
selectedEncounteris temporarily null/undefined (e.g. while loading), because the template string interpolates optional-chained values. Guard the click handler so it no-ops until the encounter is available.
src/Utils/observationRange.ts:89 formatRangeBoundstreats empty-string bounds as present, so the UI can render "> " / "< " / " - " when the backend sends "" for missing min/max (which this module already anticipates elsewhere). Normalize "" to undefined before deciding which format to render.
const hasMin = min !== null && min !== undefined;
const hasMax = max !== null && max !== undefined;
if (hasMin && hasMax) return `${min} - ${max}`;
if (hasMin) return `> ${min}`;
if (hasMax) return `< ${max}`;
return "";
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 72 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
src/components/Resource/CcnConsole.tsx:161
- The request status badge renders the raw enum value (
item.status) instead of using the existing i18n mapping used elsewhere in this file (resource_request_status__*). This will show untranslated values in the UI.
<Badge
variant="secondary"
className="shrink-0 text-[10px]"
>
{item.status}
</Badge>
src/components/Resource/CcnConsole.tsx:86
- The query that is documented as “everything assigned to this facility” is filtering by
origin_facility, which will not return the set of requests routed/assigned here (and makes the incoming/outgoing split incorrect).
// One fetch for both tabs — everything assigned to this facility.
const { data: list, isLoading: listLoading } = useQuery({
queryKey: ["ccn-resource-list", facilityId],
queryFn: query(resourceRequestApi.list, {
queryParams: { limit: 100, origin_facility: facilityId },
}),
});
src/Utils/observationRange.ts:88
formatRangeBoundstreats empty-string bounds as present, but earlier in this moduletoNumberdocuments that the API can return bounds as strings (including""). Withmin === ""this currently renders"> ", which is incorrect.
const hasMin = min !== null && min !== undefined;
const hasMax = max !== null && max !== undefined;
if (hasMin && hasMax) return `${min} - ${max}`;
if (hasMin) return `> ${min}`;
if (hasMax) return `< ${max}`;
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:38
- The logo is an
<img>with anonClick, which is not keyboard-accessible and lacks proper interactive semantics. Use a<button>or a<Link>so keyboard and assistive tech users can activate it.
src/Routers/PatientRouter.tsx:27 - This PR introduces/changes substantial patient-OTP portal routing and booking flows, but also deletes an existing Playwright spec (
tests/facility/patient/patientDetails/request/requestCreate.spec.ts) without a replacement. This reduces E2E coverage for critical patient/resource-request workflows; add/restore Playwright specs covering the new patient login + booking path and CCN referral flow.
/** Routes available to a patient signed in with an OTP session. */
const DashboardRoutes = {
"/nearby_facilities": () => <BookFacility />,
"/facility/:facilityId/appointments/:appointmentId/success": ({
| createAppointment({ | ||
| patient: data.id, | ||
| note: reason ?? "", | ||
| note: reason.trim() ?? "", |
Entire-Checkpoint: e83f3cbb144c
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 73 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/pages/PublicAppointments/PatientRegistration.tsx:194
reasoncan be undefined (query param absent), so callingreason.trim()can throw at runtime. Use optional chaining and fall back to an empty note.
src/pages/PublicAppointments/auth/PatientAuthLayout.tsx:37- Clickable
<img>isn’t keyboard-accessible, so the “go home” affordance can’t be used by keyboard/screen-reader users. Wrap the logo in a real<button>(or<Link>) and keep the image purely presentational inside it.
src/pages/PublicAppointments/Success.tsx:95 navigator.clipboard.writeTextcan reject (no permission / insecure context), which would surface as an unhandled rejection. Wrap it in try/catch and show a failure toast.
There was a problem hiding this comment.
Adjusts glasses, sighs.
Right. 73 files, 8.5k added lines, marked DO NOT REVIEW — so of course I get handed it anyway. Credit where it is due: the Beckn parsers are genuinely defensive, the useBecknTransaction state machine is documented better than most production code I have suffered through, and the epoch guard against stale in-flight responses shows someone actually thought about race conditions. I am mildly impressed. Do not let it go to your head.
The things that actually matter:
- Unbounded polling — both the status and slice queries poll forever with no cap. A BPP that never returns
on_selectmeans an infinite request loop. This is the top issue. .envpoints atdevelop-api— do not let that near develop.- Silent data loss —
limit: 100with client-side tab splitting in a referral console. Requests past 100 vanish. resources[oi] ?? resources[0]— guessing at offer/resource pairing means potentially booking the wrong resource.crypto.randomUUID()— throws on non-secure origins, unguarded.coordinationRef={undefined}— the referral link the whole flow exists for is not wired.
Also, ResourceForm just deleted reason, emergency, contacts, assigned facility and priority. I assume that is deliberate for the CCN flow, but that is a lot of data that quietly stopped being collected.
One more that did not fit in my comment budget: usePatientOtpLogin.ts pipes the raw backend error message straight into the UI on an unauthenticated login endpoint. Map it to i18n keys.
Non-blocking, since you told me not to review it. Fix the polling before this is real.> Generated by Grumpy PR Reviewer for #16676 · opus50 · 121.7 AIC · ⌖ 2.16 AIC · ⊞ 8.7K
|
|
||
| # Care API URL without the /api prefix | ||
| REACT_CARE_API_URL=https://careapi.ohc.network | ||
| REACT_CARE_API_URL=https://develop-api.ohc.network |
There was a problem hiding this comment.
Committing develop-api.ohc.network as the default API URL in .env? That is a shared default for everyone, not your preview branch scratchpad. Use a .env.local or a preview-specific env var before this ever gets near develop.
| pathParams: { transactionId: transactionId as string }, | ||
| }), | ||
| enabled: !!transactionId, | ||
| refetchInterval: awaiting ? POLL_MS : false, |
There was a problem hiding this comment.
So we poll every 1.5s forever if the callback never lands? No max attempts, no timeout, no backoff. One BPP that silently drops on_select and this tab hammers the BE until the user gives up and closes it. Add an attempt cap that flips to an error state.
| queryParams: { action: sliceAction as string }, | ||
| }), | ||
| enabled: !!transactionId && !!sliceAction, | ||
| refetchInterval: (q) => (q.state.data?.ready ? false : SLICE_POLL_MS), |
There was a problem hiding this comment.
Same unbounded-poll problem, second verse: a slice that never becomes ready refetches every 1.2s until the heat death of the component. Bound it.
| now: Date = new Date(), | ||
| ): AvailabilityWindow { | ||
| if (!option) return {}; | ||
| const [catalogIndex, offerIndex] = option.key.split("-").map(Number); |
There was a problem hiding this comment.
Encoding a composite key as "0-1" and then reverse-parsing it with split("-") is exactly the kind of stringly-typed cleverness that bites later. Carry {catalogIndex, offerIndex} on CatalogOption and skip the parsing round-trip entirely.
| const IST_OFFSET_MINUTES = 330; | ||
|
|
||
| /** `YYYY-MM-DD` for "today" in IST, whatever zone the browser is in. */ | ||
| function istToday(now: Date): string { |
There was a problem hiding this comment.
istToday adds 330 minutes then calls .toISOString() — that only works because you re-read the UTC date off a deliberately-wrong instant. It works, but it is a landmine for the next reader. dayjs is already a dependency and does .tz("Asia/Kolkata") without the hand-rolled arithmetic.
|
|
||
| iterable.forEach((rawOffer, oi) => { | ||
| const offer = asRecord(rawOffer); | ||
| const resource = asRecord(resources[oi] ?? resources[0]); |
There was a problem hiding this comment.
resources[oi] ?? resources[0] — silently pairing offer N with resource 0 when lengths disagree means we can send resourceIds for a resource the offer has nothing to do with. Booking the wrong resource is not a defect I want to debug in production; skip or flag the mismatch instead of guessing.
| const { data: list, isLoading: listLoading } = useQuery({ | ||
| queryKey: ["ccn-resource-list", facilityId], | ||
| queryFn: query(resourceRequestApi.list, { | ||
| queryParams: { limit: 100, origin_facility: facilityId }, |
There was a problem hiding this comment.
limit: 100 with no pagination, then filtered client-side into two tabs. Facility 101 requests and the coordinator quietly never sees them. Either paginate or filter server-side — an invisible truncation in a referral console is a patient-safety issue, not a UI nit.
| } as const; | ||
|
|
||
| /** Map a Care resource category to the Beckn `healthServiceType` code. */ | ||
| export function healthServiceTypeForCategory( |
There was a problem hiding this comment.
A regex over a free-text category to decide LAB_TEST vs PHYSICAL_CONSULTATION? ResourceRequestCategory is an enum sitting right there. Any new category containing "lab" silently routes to diagnostics. Map the enum explicitly.
| const doSubmit = useCallback(() => { | ||
| if (!option || !transactionId) return; | ||
| if (isConsultation) { | ||
| const newCoordinationId = crypto.randomUUID(); |
There was a problem hiding this comment.
crypto.randomUUID() is undefined on non-secure origins (plain http staging/LAN deployments). This throws and kills the whole submit path with no error handling. Guard it or use a uuid helper.
| // coordinationRef links the booking to the originating referral. The RR | ||
| // record does not currently expose the Beckn coordinationId; wire it | ||
| // here once the BE surfaces it. | ||
| coordinationRef={undefined} |
There was a problem hiding this comment.
coordinationRef={undefined} with a TODO comment — so the booking is never actually linked back to the referral it came from. That is the entire point of the coordination flow. Ship it wired, or this console books orphan appointments.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
DO NOT REVIEW/MERGE
This is used only to expose a branch preview for testing CCN flow
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Updates