Wire landed MistKit endpoints into MistDemo web app (#394)

[leogdion]

Closes #394.

Replaces the nine 501 "pending" stubs in MistKit-server mode (mistdemo web, Hummingbird) with real handlers forwarding to the already-shipped CloudKitService wrappers (#215/#45/#47/#48/#367), restoring parity with CloudKit JS mode (the follow-up to #370's scaffolding).

Endpoints wired

Verb	Route	MistKit method
POST	records/lookup	lookupRecords()
POST	records/changes	fetchRecordChanges()
POST	zones/list	listZones()
POST	zones/lookup	lookupZones()
POST	zones/changes	fetchZoneChanges()
GET	users/caller	fetchCaller()
POST	users/discover	discoverUserIdentities()
POST	users/lookup/email	lookupUsersByEmail()
POST	users/lookup/id	lookupUsersByRecordName()

Changes

• Backend: nine webXxx methods on WebBackend; CloudKitService conformance split into CloudKitService+WebBackend+Reads.swift (records/zones) and +Users.swift for file-length hygiene.
• DTOs: new WebRequests+Records / WebRequests+Users and extended WebRequests+Zones; new WebResponse shapes RecordChanges, ZoneChanges, Caller, Users. User routes carry no database selector — they run on the public DB with web-auth, matching the wrappers.
• records/lookup surfaces per-record failures via try $0.get(), mirroring the webModifySubscriptions precedent.
• De-stub: addUnwiredLandedEndpoints removed; WebServer+Pending now lists only records/resolve (Fetching Record Information (records/resolve) #41 — no wrapper yet).
• Front-end: the browser already branched to these /api/* routes in MistKit mode, so no JS change was needed — wiring the server flips each panel off the "pending Wire landed MistKit API endpoints into the MistDemo web app (MistKit-server mode) #394" banner.

Tests

MockBackend extended with the nine methods + call-capture structs; per route a forwarding test and a 401-unauthorized test (mirroring WebServerTests+Assets). 963 MistDemo tests pass; swift build, swift-format, swiftlint, and header checks clean.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 977b6403-5dea-41fa-8bc7-b4b4feb59677

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 394-mistdemo-web-app

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (v1.0.0-beta.2@1a527f2). Learn more about missing BASE report.

Additional details and impacted files

@@               Coverage Diff                @@
##             v1.0.0-beta.2     #396   +/-   ##
================================================
  Coverage                 ?   71.85%           
================================================
  Files                    ?      168           
  Lines                    ?     3844           
  Branches                 ?        0           
================================================
  Hits                     ?     2762           
  Misses                   ?     1082           
  Partials                 ?        0           

Flag	Coverage Δ
mistdemo-spm-macos	11.42% <ø> (?)
mistdemo-swift-6.2-jammy	11.42% <ø> (?)
mistdemo-swift-6.2-noble	11.42% <ø> (?)
mistdemo-swift-6.3-jammy	11.42% <ø> (?)
mistdemo-swift-6.3-noble	11.42% <ø> (?)
spm	71.57% <ø> (?)
swift-6.1-jammy	71.70% <ø> (?)
swift-6.1-noble	71.76% <ø> (?)
swift-6.2-jammy	71.73% <ø> (?)
swift-6.2-noble	71.65% <ø> (?)
swift-6.3-jammy	71.54% <ø> (?)
swift-6.3-noble	71.84% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review — PR #396: Wire landed MistKit endpoints into MistDemo web app

Overview

This PR replaces nine 501 "pending" stubs in MistDemo's Hummingbird (server) mode with real handlers that forward to existing CloudKitService wrappers. The change is self-contained to Examples/MistDemo/ and follows established patterns precisely. 963 tests pass.

---

✅ What's done well

• Consistent layering. New CloudKitService+WebBackend+Reads.swift and +Users.swift extension files mirror the existing +WebBackend.swift split. The thin-wrapper pattern is correct: WebBackend keeps the MockBackend seam cleanly separated from real network calls.
• Import conventions followed. Every new file uses internal import / public import — no bare import — per the project requirement.
• WebServer+Pending.swift cleanup. Removing PendingVerb and addUnwiredLandedEndpoints is the right call now that users/caller (the only GET stub) is wired. The leftover single-path registerPendingPost name is accurate.
• addZonesModifyEndpoint visibility tightened. Narrowing it from internal to private after introducing the umbrella addZonesEndpoints is good hygiene.
• Auth guard is uniform. Every new handler opens with guard let token = await tokenStore.currentToken — consistent with all existing routes.
• Test coverage matches established pattern. Each route gets a forwarding test and a 401-unauthenticated test, exactly like WebServerTests+Assets. Call-capture structs are Sendable and named clearly.

---

🔍 Issues / observations

1. webLookupRecords — batch-fails on any per-record error (intentional?)

return try results.map { try $0.get() }

If CloudKit returns NOT_FOUND for even one record name, the entire request throws and the caller gets a 500 rather than a partial list. CloudKit's own REST API returns per-record errors in the response body. The PR description says this mirrors webModifySubscriptions, so it appears intentional — but it means a caller requesting ["existing-record", "missing-record"] sees a generic server error instead of one result + one error. Worth a comment or a follow-up issue if the web panel needs finer-grained feedback.

2. webDiscoverUsers accepts emails only — UserIdentityLookupInfo supports more

lookupInfos: emails.map { UserIdentityLookupInfo(emailAddress: $0) }

UserIdentityLookupInfo also supports phoneNumber and userRecordName. The current API contract only exposes email lookup via this route. If the demo or any real client ever needs phone-number discovery, a separate request struct and route would be required. Not a bug, but worth noting in a comment or issue.

3. moreComing is surfaced in the response but never followed up

records/changes and zones/changes return moreComing: Bool in their JSON, but neither the server handler nor the browser client paginates automatically. For a demo with small containers this is fine, but large change sets will be silently truncated. Consider either: (a) using fetchAllRecordChanges / the all-zones equivalent in the server handler, or (b) documenting the truncation clearly so callers know they must re-request with the returned syncToken.

4. Test file naming inconsistency

The zone-reads tests live in WebServerTests+ZoneReads.swift but the analogous files are WebServerTests+Records.swift and WebServerTests+Users.swift (no "Reads" suffix). Minor, but worth aligning (WebServerTests+Zones.swift?) to make ls/grep results predictable.

5. desiredKeys hardcoded to nil in webLookupRecords

desiredKeys: nil,

Returns all fields for every record. No mechanism for the browser to request a projection. Fine for the demo, but if the response payload grows large (assets, large strings), it could be noticeable. A follow-up issue for field projection via the request body would be useful.

6. DiscoverUsers, LookupUsersByEmail, and LookupUsersByRecordName use synthesized Decodable

The three user-request structs in WebRequests+Users.swift rely on synthesized Decodable conformance (no explicit CodingKeys), while all other request structs in the PR define explicit CodingKeys. This is not a bug — synthesized keys match the field names — but it's an inconsistency that could confuse readers who expect CodingKeys everywhere. Adding explicit (even trivial) CodingKeys enums would make the pattern uniform.

---

Summary

The implementation is clean, well-scoped, and test-covered. The only blocking concern I'd flag before merge is item 1 (batch-fail on per-record NOT_FOUND in webLookupRecords) — if the demo's lookup panel is expected to show partial results rather than a red error banner, the behavior needs a change. Items 2–6 are non-blocking and suitable for follow-up issues.

🤖 Generated with Claude Code

[claude]

Code Review: Wire landed MistKit endpoints into MistDemo web app (#394)

Overview

This PR replaces nine 501 "pending" stubs in Hummingbird/MistKit-server mode with real handlers forwarding to existing CloudKitService wrappers. The approach is consistent, the file split is sensible, and each endpoint ships with two tests (forwarding + 401). Overall this is clean, ready-to-merge work. A few minor observations below.

---

What's done well

• Consistent handler shape. Every new route follows the same guard-token → decode-body → runOperation → encode-response pattern already established by the subscription/asset handlers. No surprises.
• addZonesModifyEndpoint tightened to private. Correct encapsulation now that the public entry point is addZonesEndpoints.
• PendingVerb removed. Good cleanup — with only one remaining pending stub (records/resolve, POST-only), the enum was dead weight.
• records/lookup error surfacing. try results.map { try $0.get() } propagates per-record failures (e.g. NOT_FOUND) rather than silently dropping them. The comment explaining this matches the webModifySubscriptions precedent, which is exactly the right reasoning.
• internal import on every import. Follows the project convention correctly.
• Test coverage. 2 tests per endpoint (forwarding + 401), argument capture verified through MockBackend, response shape decoded and asserted. Mirrors WebServerTests+Assets exactly.

---

Minor observations

1. Potentially unnecessary Foundation import in CloudKitService+WebBackend+Users.swift

The file only uses UserInfo, UserIdentity, and UserIdentityLookupInfo — all MistKit types. Foundation doesn't appear to contribute anything here. Worth checking if it can be dropped (though it's harmless if left).

2. Silent empty-array fallback in request DTOs

WebRequests.Lookup and WebRequests.LookupZones both default recordNames/zoneNames to [] when the key is absent:

try container.decodeIfPresent([String].self, forKey: .recordNames) ?? []

A missing field is almost certainly a caller bug, so a hard decode failure would surface problems faster. For a demo this is acceptable — just worth noting in case these DTOs are reused in a stricter context later.

3. users/discover exposes emails only

WebRequests.DiscoverUsers hard-codes emails: [String]. The underlying discoverUserIdentities(lookupInfos:) can also accept phone numbers or record names via UserIdentityLookupInfo. Fine for the demo (the browser only sends emails), but the field name emails makes the constraint implicit rather than explicit.

4. All-or-nothing lookup failure UX

Because webLookupRecords calls try $0.get() for every result, a single NOT_FOUND record causes the entire response to error. The comment acknowledges this as intentional, but a user who passes 5 valid record names and 1 bad one gets no records back. Again, demo-acceptable — just something to document in the UI or revisit if this shapes a future production API.

5. Thin wrapper boilerplate

webFetchCaller() and webListZones() are single-line passthroughs. They exist solely for WebBackend protocol conformance, which is the right reason. No change needed, just noting it's protocol-driven, not accidental.

---

No blocking issues

The implementation is correct, the tests are solid, the cleanup of addUnwiredLandedEndpoints is complete, and the code follows project conventions throughout. Ready to merge.

[claude]

Review posted below - see next message

[claude]

Code Review: PR 396 - Wire landed MistKit endpoints into MistDemo web app ### Overview This PR replaces nine 501 stubs with real Hummingbird route handlers forwarding to already-shipped CloudKitService wrappers. Implementation is clean, consistent with existing patterns, and well-tested. ### Code Quality and Style Strengths - All route handlers follow the established tokenStore/backendFactory capture, auth guard, decode, runOperation pattern without deviation. - CloudKitService+WebBackend+Reads.swift / +Users.swift file split matches the existing +WriteOperations/+ZoneOperations convention. - Removing PendingVerb and registerPending in favour of registerPendingPost is the right cleanup now that the only remaining stub is a POST. - Per-record error surfacing in webLookupRecords is correctly documented, explaining the intentional fail-fast behaviour matching webModifySubscriptions. Minor observations 1. webListZones, webFetchCaller, etc. are pure one-liner pass-throughs - protocol indirection with no logic. Internally consistent with how webModifyZones wraps modifyZones; WebBackend is doing structural separation rather than enforcing behavioural contracts. Not a problem, just worth noting. 2. WebRequests.DiscoverUsers and WebRequests.LookupUsersByEmail are structurally identical (both decode emails array of strings). Separate types are the right call for type safety; worth knowing if this family grows. 3. WebRequests.ListZones uses a custom init(from:) just to call WebRequests.decodeDatabase for its only field. Consistent with existing zone DTO pattern but adds more code than a synthesised conformance would. ### Potential Issues Missing per-record failure test for records/lookup The comment in CloudKitService+WebBackend+Reads.swift calls out that the get() call surfaces per-record CloudKit errors (e.g. NOT_FOUND). There is no test exercising this code path. The consumePendingError() mechanism in MockBackend supports it. Adding a test would confirm the documented behaviour and prevent a future refactor from silently swallowing per-record errors. Auth failure tests omit database field recordsLookupRequiresAuth, zonesLookupRequiresAuth, and zonesChangesRequiresAuth send bodies without a database key. The 401 short-circuits before body decoding, so these are correct today - but if the auth guard were ever reordered the tests would fail for the wrong reason. Sending a fully valid body in 401 tests makes them more robust. ### Security - Email addresses in users/discover and users/lookup/email are forwarded to Apple CloudKit API. No server-side PII logging is introduced; the previous CodeQL-driven debug-level guard on response bodies is unaffected. - Token presence is checked before any backend call or body decode across all nine new routes. No bypass path is visible. ### Summary Clean implementation, correct auth handling, consistent style. The per-record failure test gap is the only functional blind spot worth addressing before merge; everything else is stylistic and non-blocking.

[claude]

Code Review — PR #396: Wire landed MistKit endpoints into MistDemo web app

Overview

This PR replaces nine 501 stubs in MistKit-server mode (mistdemo web) with real Hummingbird handlers that forward to the already-shipped CloudKitService wrappers. The changes are clean, well-scoped, and follow the project's existing conventions faithfully.

---

✅ Strengths

• Consistent patterns: Every new handler follows the tokenStore capture → guard token → runOperation → encode flow already established by WebServer+Assets and WebServer+Subscriptions. No reinvention.
• Correct DB semantics: User-identity routes (users/caller, users/discover, etc.) carry no database field and don't need one — the underlying wrappers hard-wire public+web-auth. The DTO and protocol designs reflect this accurately.
• Clean de-stubbing: addUnwiredLandedEndpoints and PendingVerb are properly removed. The addZonesModifyEndpoint visibility drop from internal to private is a nice tightening.
• Test coverage: 18 tests (9 routes × forwarding + 401) match the WebServerTests+Assets precedent exactly.

---

🐛 Potential Issues

1. webLookupRecords fails on the first per-record error — no partial results

return try results.map { try $0.get() }

If a lookup for ["note-1", "note-2"] returns note-1 = .success(...) and note-2 = .failure(NOT_FOUND), the whole call throws. The caller gets an error instead of the one record that was found. CloudKit's own /records/lookup response returns per-record status objects, so partial results are legitimate.

The comment says this mirrors webModifySubscriptions, but subscriptions are a batch-modify (partial failure there is semantically different than a read). Consider returning Result<RecordInfo, Error> in the response, or at minimum documenting the all-or-nothing behavior in the WebBackend protocol.

2. webDiscoverUsers only maps emails — phone/recordName look-up silently unavailable

internal func webDiscoverUsers(
  emails: [String]
) async throws -> [UserIdentity] {
  try await discoverUserIdentities(
    lookupInfos: emails.map { UserIdentityLookupInfo(emailAddress: $0) }
  )
}

discoverUserIdentities accepts any [UserIdentityLookupInfo], which can hold an email, phone number, or record name. WebRequests.DiscoverUsers and the protocol method only accept emails. If the intent is email-only, that's fine — but it's worth a note that users/lookup/id and users/lookup/email are the intended paths for non-email lookup, and users/discover is intentionally email-only here.

---

🔍 Minor Observations

3. Empty recordNames succeeds silently

WebRequests.Lookup decodes:

self.recordNames = try container.decodeIfPresent([String].self, forKey: .recordNames) ?? []

A POST with no recordNames key returns HTTP 200 with an empty records array. That's probably acceptable, but it's a silent no-op that could mask a client bug. Returning 400 on an empty list is defensible.

4. No test for the per-record error path in webLookupRecords

The mock always returns stub records. The try $0.get() branch is never exercised in the test suite. Given that this behavior differs from a simple passthrough, a test with pendingError or a mock that returns a .failure result would lock in the semantic.

5. Comments describe "what" rather than "why" in the conformance extensions

Per CLAUDE.md: "Only add [a comment] when the WHY is non-obvious." The file-header comments like:

// Read-side `WebBackend` conformance for records and zones: lookup, changes,
// and zone listing. The primary conformance declaration lives in ...

…are navigation aids rather than rationale. The file name already communicates the content. Fine to keep if the convention elsewhere is the same, but worth noting for future files.

---

Summary

The implementation is solid and production-ready for a demo app. The two actionable items worth resolving before merge:

1. Decide and document the all-or-nothing behavior of webLookupRecords — either add a test for the error case or change the behavior to match CloudKit's per-record semantics.
2. Add a note (or rename) webDiscoverUsers so it's clear the email-only constraint is intentional, not an oversight.

Everything else is style / nice-to-have. 963 tests passing and the lint pipeline clean are a good baseline.

[claude]

Code Review — PR #396: Wire landed MistKit endpoints into MistDemo web app

Overview

This PR replaces nine 501 stubs with real Hummingbird route handlers that forward to already-shipped CloudKitService wrappers. The scope is well-defined: backend wiring + DTOs + tests + light frontend cleanup. The architectural decision to skip users/lookup/email and users/lookup/id (Apple-deprecated) in favor of the unified users/discover endpoint is correct.

---

✅ What's working well

• Extension structure follows the established WebServer+Xxx / CloudKitService+WebBackend+Xxx pattern perfectly. Splitting reads and users into separate files is the right call for file length.
• Explicit access modifiers and import qualifiers (internal import, internal func) are consistent throughout.
• #if canImport(Hummingbird) guards are correctly applied on all server-only files.
• addUnwiredLandedEndpoints / PendingVerb dead code is cleanly removed. The rename to registerPendingPost is accurate since the only remaining stub is a POST.
• Test coverage shape matches the existing pattern: a forwarding test + a 401 test per route. Mock stubs are minimal and predictable.
• Auth check before body decode in all handlers — the 401 path never touches the (possibly malformed) body.

---

Issues

Minor — Missing error-propagation test for records/changes

records/lookup has recordsLookupPropagatesBackendError verifying that a backend failure returns 500, but records/changes has no equivalent. Given the all-or-nothing comment in webLookupRecords specifically calls this out, it's worth having the parallel test for the changes endpoint too.

Minor — Empty-array inputs are not guarded

WebRequests.Lookup defaults recordNames to [] via decodeIfPresent ?? []. If a client sends {} or {"recordNames":[]}, webLookupRecords forwards an empty array to lookupRecords, and webLookupZones forwards an empty ZoneID array to lookupZones(zoneIDs:). CloudKit's behavior with an empty lookup payload is undefined from the client side (it may return an empty array, may return an error). A guard with a 400 response would be cheaper to debug than an unexpected CloudKit error propagating as 500.

// Example — WebServer+Records.swift addRecordsLookupEndpoint
guard !body.recordNames.isEmpty else {
    return Response(status: .badRequest)
}

Nit — Name collision between WebRequests.RecordChanges and WebResponse.RecordChanges

Both the request DTO and the response struct are named RecordChanges (in different namespaces, so no compiler issue). A reader scanning WebServer+Records.swift encounters both and has to track which namespace each belongs to. WebRequests.RecordChangesRequest / WebResponse.RecordChangesBody would be unambiguous, though this is a low-priority readability concern.

Nit — Continuation alignment in MockBackend+UserOperations.swift

The webDiscoverUsers stub (line ~1289) aligns the + continuation differently from the rest of the file:

return emails.map { email in
  UserIdentity(lookupInfo: UserIdentityLookupInfo(emailAddress: email))
}
  + userRecordNames.map { name in   // ← extra two-space indent before `+`
    UserIdentity(userRecordName: .recordName(name))
  }

Running swift-format may reformat this. Not a correctness issue, just inconsistent with the rest of the codebase.

---

Suggestions (non-blocking)

• Test for users/discover with only record names: The existing usersDiscoverForwards test sends both emails and record names. A test with only userRecordNames (and the emails key absent) would confirm the decodeIfPresent ?? [] path for that specific input shape.
• webFetchCaller / webListZones thin wrappers: These are one-liner forwards to fetchCaller() / listZones(). The WebBackend protocol seam justifies them for mock-ability, and the symmetry with the other webXxx methods is worth preserving — just noting that if the protocol is ever consolidated, these are candidates for removal.

---

Summary

Solid, focused PR. The two actionable items are the missing error-propagation test for records/changes and the unguarded empty-array inputs. Everything else is convention-compliant and test-covered. Mergeable after those are addressed (or explicitly deferred with a tracking comment).