Skip to content

fix: allow scheduled Apple Health medication mirrors - #1007

Merged
MBombeck merged 4 commits into
MBombeck:mainfrom
muhdusama:medfix/apple-health-scheduled-mirror
Sep 20, 2026
Merged

MBombeck merged 4 commits into
MBombeck:mainfrom
muhdusama:medfix/apple-health-scheduled-mirror

Conversation

@muhdusama

@muhdusama muhdusama commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Allow Apple Health medication mirrors with asNeeded: false to be created without requiring a HealthLog-owned schedule.

The iOS client maps HealthKit medications that have an Apple schedule to asNeeded: false, but does not send HealthLog schedules. The server previously rejected those mirrors with HTTP 422 (Mindestens ein Zeitfenster).

This change exempts externalSource: APPLE_HEALTH from the local schedule requirement while preserving the existing validation for native HealthLog medications.

Tests

  • src/app/api/medications/__tests__/route.test.ts: 33/33 passed on current main
  • Added regression coverage for a scheduled Apple Health mirror without a HealthLog-owned schedule
  • End-to-end sync validation confirmed the previously rejected request shape now returns HTTP 201 and remains as_needed=false

@MBombeck

Copy link
Copy Markdown
Owner

@muhdusama thanks for this, and for bringing the failing shape with it. The diagnosis matches what the client does, and the 422 is real, so the direction is right.

Two things before I take it, one narrow and one that decides whether the line is enough.

The bypass is wider than the case. externalSource is an ordinary request field. The both-or-neither refine means a caller has to send an externalId alongside it, which is a hurdle, but nothing stops any client from labelling a plain medication as an Apple Health mirror and skipping the schedule requirement that way. Gating the exemption on the pair rather than on the source alone keeps it to actual mirrors:

(b.externalSource === "APPLE_HEALTH" && b.externalId !== undefined) ||
b.asNeeded === true ||
(!!b.schedules && b.schedules.length >= 1)

What happens to the medication afterwards is the part I want to be sure about. A medication with asNeeded: false and zero schedules is a shape nothing else in the system has had to deal with. The reminder job selects exactly { active: true, asNeeded: false }, and adherence is computed from expected doses per schedule. If a mirror with no schedules lands in the adherence denominator as "nothing taken", it shows up as a red compliance chip and drags the health score, which would be a worse bug than the 422 it fixes.

Could you check that path, or tell me if you already did? A quick look at the per-medication compliance read and the dashboard chip for such a mirror would settle it. If it turns out the mirror needs excluding there too, that belongs in this PR rather than in a follow-up, and I am happy to help with that part.

One smaller question while you are in there: does the update path need the same exemption, or does an existing mirror never get patched with asNeeded: false and no schedules?

@muhdusama

Copy link
Copy Markdown
Contributor Author

@MBombeck thanks for the detailed review. I checked all three points against the current main (0195c59).

1. Narrowing the exemption

Agreed. I think the exemption should be scoped to the provenance pair exactly as you suggested:

(b.externalSource === "APPLE_HEALTH" && b.externalId !== undefined) ||
b.asNeeded === true ||
(!!b.schedules && b.schedules.length >= 1)

The existing both-or-neither refine already requires externalSource and externalId together, but keeping the schedule exemption itself pair-scoped makes the intended boundary explicit and prevents it becoming wider if that earlier refine ever changes. I will keep regression coverage for the valid Apple Health mirror and the normal scheduled-medication requirement.

2. Zero schedules, compliance, reminders, and the dashboard

I traced the asNeeded: false + zero-schedule shape through the current code.

  • The reminder worker does fetch { active: true, asNeeded: false }, but reminder generation iterates the medication's schedules. With zero schedules there are no reminder slots and no missed-dose rows minted.
  • Current main now has the shared expectsDoses() guard (!asNeeded && schedules.length > 0) and uses it in the medication-compliance insight, BP-status path, comprehensive insights, and doctor report, so this shape is excluded from those adherence calculations.
  • The current composite Health Score itself does not have a medication/adherence pillar. Its pillars are blood pressure, glycaemia, activity, sleep, adiposity, wellbeing, and lipids, so this mirror cannot lower the current Health Score.
  • The dashboard MED_COMPLIANCE ring is today's taken/scheduled progress from medsToday; a zero-schedule mirror contributes no scheduled dose, and the ring self-gates when there are no scheduled doses.

I did find one remaining issue worth fixing in this PR: the batched GET /api/medications/compliance route filters only on asNeeded: false, and the per-medication compliance route also builds a payload without an expectsDoses guard. calculateCompliance() deliberately returns 100% when there are zero expected schedules, so a zero-schedule mirror can currently surface as a misleading perfect compliance value on the medication card/detail view.

I agree that this should be handled here rather than left as a follow-up. Reusing expectsDoses() at those compliance endpoints, with regression coverage for an Apple Health mirror, looks like the cleanest way to keep "no expected doses" from being presented as adherence.

3. Update path

On the server side, the current Apple Health mirror replay path is POST/idempotent: re-posting the same (externalSource, externalId) pair returns the existing medication with HTTP 200 and performs no update.

updateMedicationSchema does not accept externalSource or externalId. An ordinary PUT that omits schedules can preserve an existing zero-schedule mirror, while an explicit schedules: [] on a scheduled/non-PRN update is currently rejected.

So for the current mirror replay path I do not see the same create-time exemption being required on update. I will pin that assumption with coverage as well; if the client has a PUT path that explicitly sends an empty schedule list for a mirror, that would be the case that needs separate handling.

Thanks again for calling these out. The downstream check was useful, especially the card/detail compliance case.

@MBombeck

Copy link
Copy Markdown
Owner

That is exactly the trace I was hoping for, thank you. I checked the two load-bearing claims myself rather than take them on trust, and both hold:

expectsDoses is !asNeeded && schedules.length > 0 in src/lib/analytics/compliance/display.ts, and it filters the comprehensive insights, the doctor report, the blood-pressure status path and the medication-compliance insight. A mirror with no schedules drops out of every adherence denominator, so it cannot show up as a red chip or a missed dose. The score has no medication pillar to drag either. Good.

Your reading of the update path matches mine: the replay is idempotent on POST, and updateMedicationSchema does not carry the provenance pair, so there is nothing to exempt there. Pinning that assumption with a test is the right instinct, because the day the client does send an empty schedule list on a PUT, the test is what tells us instead of a user.

The diff still carries the wide form of the refine, so the pair-scoped version has not landed yet. Push that, keep the two regression cases you mentioned, and I will merge as soon as CI is green.

Thanks for the care you put into this, both in the original report and in tracing it afterwards. It saved me a considerable amount of work.

@muhdusama

Copy link
Copy Markdown
Contributor Author

@MBombeck the requested pair-scoped follow-up is pushed now.

Pushed

Commit: c223b6d

The create refine now requires the Apple Health provenance pair for the schedule exemption:

(b.externalSource === "APPLE_HEALTH" && b.externalId !== undefined) ||
b.asNeeded === true ||
(!!b.schedules && b.schedules.length >= 1)

The existing regressions remain in place:

  • a normal scheduled medication with no schedule still returns 422;
  • a scheduled Apple Health mirror with both externalSource + externalId and no HealthLog-owned schedule is accepted.

I also added the update-path regression we discussed: an existing zero-schedule scheduled mirror can receive an ordinary PUT when schedules is omitted, while the existing update invariants still reject an explicit empty schedule list on the scheduled/non-PRN path.

Local validation

  • POST medication route suite: 33/33 passed
  • PUT medication route suite: 27/27 passed
  • focused total: 60/60 passed
  • Prettier: pass
  • git diff --check: clean
  • repository gitleaks pre-commit hook: pass

The new upstream workflow runs were created for this head, but all nine are currently action_required, so they are waiting for fork-workflow authorisation rather than having executed yet.

Thanks again. Once those are authorised, I will keep an eye on the results.

@muhdusama

Copy link
Copy Markdown
Contributor Author

@MBombeck looks like everything is green now. Thanks again for approving the workflow runs.

All of the required checks have passed, including Integration, e2e, Security & Quality, CodeQL, Knip and the Docker builds, and GitHub is now showing the PR as clean/mergeable.

Appreciate you taking the time to review this and point me in the right direction on the pair-scoped check. I will leave it with you for the merge.

@MBombeck

Copy link
Copy Markdown
Owner

Thanks for the follow-up. All required checks are green, including integration and e2e, and both Docker builds passed. The provenance-pair check and the update regression cover the changes I asked for.

My previous “merge as soon as CI is green” reply left out the remaining compliance issue you had already identified. That was an incomplete merge criterion on my side. Your downstream trace was useful, and I want to carry that finding through before merging.

I checked c223b6d: both compliance endpoints still build a payload for asNeeded: false with zero schedules. The shared bundle returns rate: 100 for that empty schedule set, and the card, table and detail overview render it as adherence. The current tests pass without covering this case.

A bounded way to finish this:

  • Use the existing expectsDoses() distinction at the compliance read/presentation boundary. The relevant server files are src/app/api/medications/compliance/route.ts and src/app/api/medications/[id]/compliance/route.ts, which share src/lib/medications/compliance-payload.ts. A mirror without a local schedule needs an explicit “not applicable” outcome rather than a displayed percentage. Keep asNeeded: false; it still describes the source medication correctly.
  • Carry that outcome through the card, table and detail overview. Simply omitting the batched entry leaves the card and table showing loading skeletons. Their current guards only check asNeeded. The presentation points are card-parts/medication-card-body.tsx, medication-table.tsx and detail/medication-detail-tabs.tsx under src/components/medications/; the GLP-1 card also uses the shared card body.
  • Add coverage for both compliance endpoints and the visible empty state: a scheduled Apple Health mirror with no local schedules must show neither a percentage nor a permanent loading state. Keep an ordinary scheduled medication as a positive control and preserve PRN behavior. The route test suites already sit beside both endpoints.

Please preserve the existing arithmetic for scheduled medications with no doses due within a particular window. This fix concerns a medication with no local schedule at all. If the API response shape changes, include the OpenAPI schema and generated contract in the same change.

Would you like to take this final piece? If you would prefer help with the server or UI part, I can contribute that portion to this PR. The original sync fix and your investigation are valuable; this closes the path from accepting the mirror to displaying it correctly.

@muhdusama

Copy link
Copy Markdown
Contributor Author

@MBombeck thanks for spelling out the remaining path. I have taken this final piece through as well and pushed it in 41d47c3.

I kept the change bounded to the zero-local-schedule case you described. Both compliance endpoints now return an explicit not-applicable state with NO_LOCAL_SCHEDULE, and the percentage and display fields are null for that case. A scheduled medication that has a local schedule still follows the existing compliance arithmetic, including periods where no dose happens to be due. PRN behaviour remains unchanged.

I also carried that state through the medication card, table and detail overview. A mirrored medication with no HealthLog-owned schedule now shows a settled “Adherence not applicable” state rather than 100% or a loading skeleton. The table sort treats it as having no compliance value rather than as 0%.

The OpenAPI schema and generated contract are updated in the same commit, and I added coverage for both endpoints and the visible states you called out.

Local validation is green:

  • focused compliance and presentation tests: 88/88
  • full medication component suite: 367/367
  • TypeScript: pass
  • focused ESLint: pass
  • Prettier and git diff checks: pass
  • OpenAPI generation and drift check: pass
  • repository gitleaks hook: pass

The fresh upstream workflow runs are waiting for approval again at the moment, so they have not executed yet.

Thanks for catching the remaining presentation path. I think this now closes the mirror flow cleanly from accepting it through to displaying it correctly.

@MBombeck MBombeck left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for carrying this through both endpoints and the web views. The explicit not-applicable state, table sorting and shared card handling address the presentation gap I raised. I have approved the fresh workflow runs; they are running now.

I found a compatibility issue when checking this against the released iOS 1.0.3 models. I missed that client in my previous review, so the incomplete acceptance criteria are mine. Your implementation follows the direction I gave you.

The released app requires non-null compliance7 and compliance30 objects in both MedicationCompliancePayload and MedicationComplianceSummaryEntry. I compiled that unchanged model file and decoded fixtures matching this PR's responses:

Scheduled detail: decoded
Scheduled batch: decoded
Mirror detail: valueNotFound at compliance7
Mixed batch: valueNotFound at Index 1.compliance7

One mirror therefore makes the entire batch fail to decode, including otherwise valid scheduled entries. The store falls back to individual requests, so this does not crash the medication list, but it loses batching and the mirror's individual response still fails. Updating OpenAPI or a future app build does not protect installations already in use.

Please preserve a response that released clients can decode. An explicit opt-in for the nullable representation is one option; an additive compatible representation is another. The updated web views should still render the explicit not-applicable state without a percentage. Please include a mixed scheduled/mirror batch regression against the legacy non-null contract; if the response varies by capability, cover requests in both orders through the shared cache too.

I am holding the merge for this compatibility issue. The source-mirror fix and the web presentation work remain useful; this concerns the rollout boundary with the app already in the App Store.

applicable: false,
notApplicableReason: "NO_LOCAL_SCHEDULE",
compliance7: null,
compliance30: null,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[P2] Preserve the released iOS response contract. Both compliance models in iOS 1.0.3 require these objects. Returning null rejects the per-medication payload and the entire batch array whenever it contains this row. I reproduced both failures with the released Swift model; details and the compatibility boundary are in the review summary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you and good catch. Let me work on these tomorrow and revert back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@MBombeck thank you for catching this. I have pushed the compatibility fix in 24ff031.

I kept the additive applicable=false / NO_LOCAL_SCHEDULE signal for clients that understand the new state, but restored non-null compliance7 and compliance30 objects for the not-applicable case so the released iOS 1.0.3 models can still decode both the detail response and a mixed batch. Those fields are all-zero compatibility placeholders rather than the previous vacuous 100%, and complianceDisplay remains null. The web continues to branch on applicable, so it renders the explicit not-applicable state without a percentage.

I also added a mixed scheduled/mirror batch regression against the legacy non-null contract, updated the OpenAPI schema/generated spec, and fixed the missing Polish locale keys that caused the previous Security & Quality unit-test failure.

Focused local validation is green:

  • compliance route tests: 18/18
  • presentation and locale-integrity tests: 101/101
  • TypeScript: pass
  • ESLint: pass
  • Prettier: pass
  • OpenAPI generation/drift check: pass
  • gitleaks pre-commit scan: pass

The new head is pushed and should now go through the upstream checks.

@MBombeck MBombeck left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rechecked against the released client contract. The compatibility placeholders preserve the required non-null fields, while applicable carries the new semantic state. The mixed-batch regression covers the rollout boundary.

@MBombeck
MBombeck merged commit aa58495 into MBombeck:main Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants