Skip to content

Resolve #329: CloudKit JS alternate backend + browser-flag defaults#335

Merged
leogdion merged 2 commits into
330-interactive-mistdemofrom
329-cloudkit-js
May 12, 2026
Merged

Resolve #329: CloudKit JS alternate backend + browser-flag defaults#335
leogdion merged 2 commits into
330-interactive-mistdemofrom
329-cloudkit-js

Conversation

@leogdion
Copy link
Copy Markdown
Member

@leogdion leogdion commented May 12, 2026

Summary

  • Integrate CloudKit JS into MistDemo web for MistKit-vs-CloudKit-JS comparison #329 — wire CloudKit JS as the alternate web backend behind the existing mode toggle. The toggle button is now enabled and each CRUD operation (query/create/update/delete) calls container.privateCloudDatabase directly when CloudKit JS mode is active. Same Apple ID session token, same container, same REST surface as MistKit mode — only the SDK shape differs.
  • Browser-flag UXauth-token keeps its default of auto-opening the browser; web flips to not auto-opening (long-running server, often driven from another machine). Both commands accept --browser and --no-browser; --no-browser wins if both are set.

Test plan

  • swift test from Examples/MistDemo — 920 tests pass.
  • swift test from repo root — 483 tests pass.
  • mise exec -- swift-format -i -r Sources/ Tests/ — clean.
  • mise exec -- swiftlint — 0 violations in 487 files.
  • Manual: swift run mistdemo web --api-token "$CLOUDKIT_API_TOKEN" --browser opens browser, both mode toggle buttons work, CRUD operations succeed in both modes against the same container.
  • Manual: swift run mistdemo auth-token --api-token "$CLOUDKIT_API_TOKEN" still opens the browser by default; --no-browser suppresses it.

Out of scope

🤖 Generated with Claude Code


Perform an AI-assisted review on CodePeer.com

leogdion and others added 2 commits May 12, 2026 14:55
Enables the previously-disabled "CloudKit JS (browser)" toggle in the
mistdemo web UI and wires real container.privateCloudDatabase calls
(performQuery / saveRecords / deleteRecords) behind it, so each CRUD
operation can be driven either through MistKit on the server or
CloudKit JS in the browser against the same container with the same
auth token.

- index.html: replace cloudKitJsNotWired() short-circuits with real
  CloudKit JS handlers; add fieldsToCKJS() to wrap the demo's flat
  {key: "value"} shape into CloudKit JS's {key: {value: ...}} form;
  add runCloudKitJsOperation() to surface the hasErrors/errors payload
  pattern that CloudKit JS uses in place of throwing.
- Update mode hint: both backends share the same Apple ID session
  token, container, and REST surface; only the SDK shape differs.
- Move the index-page tests into WebServerTests+Index.swift (matching
  the existing +CRUD extension pattern) and add a smoke test asserting
  the CDN script tag, an enabled toggle, the three call sites, and
  absence of the placeholder.

No server-side changes: the auth-token capture flow already shares
the ckWebAuthToken across both modes via WebAuthTokenStore.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The auth-token command's whole purpose is to capture a token, so the
default of auto-opening the browser is the right UX. The web command
is a long-running server that's often driven from another machine (or
a non-default browser), so auto-launch is the wrong default there.

- Rename AuthTokenConfig.noBrowser → openBrowser (default: true).
- Rename WebConfig.noBrowser → openBrowser (default: false).
- Add BrowserFlagResolver shared between the two configs: --no-browser
  sets no.browser=true (wins if both flags are set), --browser sets
  browser=true, otherwise the per-command default applies.
- Both commands accept --browser and --no-browser; help text states
  the per-command default explicitly.
- Update README options table and the AuthTokenConfig / AuthTokenCommand
  tests; add coverage for the resolver's precedence rule.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 12, 2026

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: aff31031-559b-492f-8ee4-fbf5e0546380

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 329-cloudkit-js

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

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

@leogdion leogdion merged commit 1e8b907 into 330-interactive-mistdemo May 12, 2026
20 checks passed
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

❌ Patch coverage is 84.12698% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.21%. Comparing base (62c7b6c) to head (917221e).
⚠️ Report is 1 commits behind head on 330-interactive-mistdemo.

Files with missing lines Patch % Lines
.../Sources/MistDemoKit/Configuration/WebConfig.swift 0.00% 6 Missing ⚠️
...ources/MistDemoKit/Commands/AuthTokenCommand.swift 0.00% 2 Missing ⚠️
...Demo/Sources/MistDemoKit/Commands/WebCommand.swift 0.00% 1 Missing ⚠️
...istDemoKit/Configuration/BrowserFlagResolver.swift 87.50% 1 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##           330-interactive-mistdemo     #335      +/-   ##
============================================================
- Coverage                     70.23%   70.21%   -0.03%     
============================================================
  Files                           543      545       +2     
  Lines                         14969    14999      +30     
============================================================
+ Hits                          10514    10532      +18     
- Misses                         4455     4467      +12     
Flag Coverage Δ
mistdemo-spm-macos 59.79% <84.12%> (+0.07%) ⬆️
mistdemo-swift-6.2-jammy ?
mistdemo-swift-6.2-noble ?
mistdemo-swift-6.3-jammy ?
mistdemo-swift-6.3-noble 59.80% <84.12%> (+0.01%) ⬆️
spm 51.71% <ø> (+0.03%) ⬆️
swift-6.1-jammy ?
swift-6.1-noble ?
swift-6.2-jammy ?
swift-6.2-noble ?
swift-6.3-jammy ?
swift-6.3-noble 51.31% <ø> (-0.03%) ⬇️

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
Copy link
Copy Markdown

claude Bot commented May 12, 2026

Code Review — PR #335: CloudKit JS alternate backend + browser-flag defaults

Overview

This PR wires up CloudKit JS as the alternate browser-side backend for the MistDemo web UI, and refactors the browser-open UX from a single --no-browser opt-out into a per-command default with separate --browser/--no-browser flags. Both changes are coherent and well-scoped.


What Works Well

  • Semantic rename: noBrowser: BoolopenBrowser: Bool is clearer. The old double-negative (!config.noBrowser) is now a direct config.openBrowser.
  • BrowserFlagResolver centralizes the logic: Both configs use the same resolver instead of duplicating the two-flag precedence rule. The priority order (no.browser wins, then browser, then default) is explicit and documented in the type comment.
  • XSS hygiene maintained: runCloudKitJsOperation uses setStatus which writes to textContent, not innerHTML. Error messages from CloudKit JS cannot inject HTML.
  • fetchRecordChangeTag pre-fetch for update: CloudKit JS requires recordChangeTag for updates. The helper correctly fetches it before calling saveRecords, matching what MistKit mode does server-side.
  • Test organization: Splitting indexReturnsHtml into WebServerTests+Index.swift keeps the main server test file focused on API routes.
  • New regression test: indexExposesCloudKitJsHandlers verifies all four CloudKit JS operations are wired and the stub function is gone — a good guard against accidental regressions.

Issues and Suggestions

1. Missing WebConfig test for "both flags set" scenario

AuthTokenConfigTests has a dedicated test for the --no-browser wins-over---browser case, but there is no equivalent for WebConfig. Since BrowserFlagResolver is shared the logic is tested, but the parsing path through WebConfig.init(configuration:) is not exercised for the conflict case. A mirror test would close this gap.

2. BrowserFlagResolver has no direct unit tests

The resolver is only exercised indirectly through AuthTokenConfigTests. Given it is a pure, stateless function with a clear three-case contract (no-browser wins, browser set, fall back to default), a small dedicated test suite would make intent obvious and isolate future regressions from config-parsing noise:

// BrowserFlagResolverTests.swift
@Test("no.browser=true resolves to false regardless of browser flag")
@Test("browser=true with no.browser unset resolves to true")
@Test("neither flag set falls back to default")

3. JavaScript: fieldsToCKJS only handles scalar values

The current wrapping ({ value: v }) is correct for strings and numbers but will produce malformed payloads for complex field types — REFERENCE expects { value: { recordName, action } }, LOCATION expects { value: { latitude, longitude } }, etc. For a demo with simple textarea input this is acceptable, but a short comment noting the limitation would prevent future confusion:

// Wraps scalar values only. Complex types (Reference, Location, Asset)
// require richer { value: … } shapes — not handled by this demo.

4. JavaScript: redundant guard in runCloudKitJsOperation

if (data && data.hasErrors && data.errors && data.errors.length) {

data.errors && data.errors.length is redundant — CloudKit JS guarantees hasErrors === true implies a non-empty errors array. Simplifying to data?.hasErrors matches the optional-chaining style used elsewhere in the file. Minor, but worth cleaning up.


Summary

The core logic is sound and the implementation is clean. Two test gaps worth addressing before merge:

  1. A WebConfig-specific test for the "both flags set" conflict resolution path.
  2. Direct unit tests for BrowserFlagResolver (small, but it is now shared infrastructure).

The JavaScript changes are correct for the demo scope with the scalar-value caveat noted above. No blocking issues.

🤖 Generated with Claude Code

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.

1 participant