feat(hutch,ios): make the iOS app a generic hypermedia client + coverage gate (consolidated)#927
feat(hutch,ios): make the iOS app a generic hypermedia client + coverage gate (consolidated)#927FagnerMartinsBrack wants to merge 10 commits into
Conversation
FagnerMartinsBrack
changed the title
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude CI has FAILED. Use your Read tool to read the local file Context
Failed Jobs
|
|
Claude finished @FagnerMartinsBrack's task in 9m 54s —— View job CI Failure Fix — PR #927
All 255 tests pass. The job fails only at the coverage gate this PR itself introduces ( Why this is a seeding problem, not a regressionThe gate is new in this PR (
Recommended fixPer CLAUDE.md — "Never lower floors / add excludes without explicit human approval… present the situation and ask" — I'm asking rather than silently weakening the gate you just added: 1. func testDecodeFailureSurfacesAsFailed() async {
// A malformed queue body makes loadQueue throw a non-refusal error; the
// share-save journey must surface it as .failed, not crash.
let store = TestSupport.loggedInStore()
let captor = FakeHTMLCaptor(page: CapturedPage(rawHtml: "<html><body>hi</body></html>", title: "Captured", mediaType: nil))
StubURLProtocol.setHandler { request, _ in
switch request.url?.path {
case "/": return .redirect(to: "/queue")
case "/queue": return .json(200, "[]") // array, not a Siren object → decode error
default: return .json(404, "{}")
}
}
let saver = SaveSharedPage(store: store, api: makeAPI(store: store), captor: captor)
let outcome = await saver.run(url: URL(string: "https://example.com/post")!, fallbackTitle: nil, sharedPdf: nil)
guard case .failed = outcome else { return XCTFail("expected .failed, got \(outcome)") }
}2. 3. Why I haven't pushedThis CI runner is Linux with no Xcode, so I can't run Reply |
|
@claude Resolve conflicts. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 35m 37s —— View job Merge conflicts resolved ✅Merged current
Why merge, not rebaseThis branch already carried a merge commit (the author's stated strategy) plus 4 early commits whose content already landed in Conflicts (3 files, all the reader-reconciliation area)
The PR's B1 — "re-read and adopt the server's truth on a reader status change; never infer 'read' and drop a row" (because
Judgment call for the author to sanity-check: I favored B1 (the PR's documented purpose) over main's optimistic drop, since keeping the latter would re-introduce the toggle-to-unread bug B1 fixes. main's account-deletion feature is untouched — it depends only on Auto-merges verified
Validation
|
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude CI has FAILED. Use your Read tool to read the local file Context
Failed Jobs
|
|
Claude finished @FagnerMartinsBrack's task in 12m 54s —— View job CI Failure Fix — PR #927 (Attempt 2 of 5)
All 262 tests pass ( What changed since attempt 1The first three are the same seeding drift as before (floors were seeded on your local Xcode; CI's Xcode 26.3 measures 1–3% lower on these environment‑sensitive files).
Why I'm not pushing anything this attemptOnly The fix (ready to apply on your
|
Several places had the client hard-coding server internals instead of discovering or tolerating them, so a server-side change would break the app or force an App Store release. This removes them: - Inject every cookie the session-bootstrap response sets into the reader web view instead of selecting one by the hard-coded `hutch_sid` name, and scrub sign-out cookies by server host rather than by name. A server cookie rename no longer breaks the reader or needs an app release. - On a reader status change, re-read the collection and adopt the server's truth instead of inferring "read" and dropping the row locally. A toggle back to unread no longer wrongly removes the item from the list. - Decode a collection warning leniently and make `code` optional, so an evolving or malformed warning degrades to no banner instead of failing the whole collection decode and blanking the page. - Exclude a link from toolbar controls when ANY of its rels is structural, not just the presentation token, so a multi-rel structural link can't render as a tappable control while also driving pagination. - Surface an item's non-structural, non-`read` semantic links as row controls, so a future item link (e.g. `share`) renders instead of being silently discarded. - Drop comments naming concrete server routes/methods/symbols (`/queue`, `/auth/session`, `/oauth/token`, `SESSION_COOKIE_NAME`, `isFile`, `/login`, `/signup`) that rot silently and reintroduce the URL coupling the hypermedia contract removes.
The rest of the repo enforces a 100% coverage gate, but the iOS suite ran with no coverage measurement or threshold at all — a structural blind spot. `make test` now runs with `-enableCodeCoverage` and fails if any source file drops below its recorded floor (`scripts/check-coverage.py`). The gate is a ratchet, not an all-at-once 100%: `scripts/coverage-baseline.json` excludes pure SwiftUI views and WebKit/OS-boundary glue (the Swift analog of the repo's whole-file OS-boundary exclusion) and records a per-file floor for every other file, seeded at today's coverage. A file with no floor must be 100%, so a new logic file cannot land untested. Later PRs raise the floors toward 100% and move ShareURLExtractor out of the exclusions once it is testable. The checker uses only the system python3 + xcrun, so the CI ios-tests job needs no extra toolchain setup.
`ShareViewController` and `ShareURLExtractor` lived only in the extension target, which no test target compiles, so their real logic — the multi-item payload scan, the URL-item type fallbacks, the PDF size ceiling, and the outcome→status mapping — had zero coverage and was structurally unreachable. - Move `ShareURLExtractor` into `Shared/` (it is Foundation-only) so it compiles into the app target `@testable import` can reach, and split `extract` into a context wrapper plus an item-list core a test can drive with fabricated `NSItemProvider`s instead of a live extension context. - Extract the outcome→(message, symbol, tone) decision out of `ShareViewController` into a pure `ShareStatusPresentation` in `Shared/`; the controller keeps only the tone→`UIColor` mapping at the UIKit boundary. - Cover both: `ShareURLExtractor` at 96% (URL / data / string / plain-text extraction, multi-item split, PDF provider + suggested-name fallback, the byte ceiling) and `ShareStatusPresentation` at 100% (every outcome, refusal join, error-vs-warning tone). - Coverage baseline: drop `ShareURLExtractor` from the exclusions and floor it at 96%; pin `ShareStatusPresentation` at 100%.
Cover error and edge paths that had no tests, and raise their coverage floors: - OAuthService (90%→98%): a malformed 200 body and a 200 missing refresh_token both surface .malformedResponse; refresh without a stored token throws .noRefreshToken and makes no request; revoke without a token skips the network but still clears. - AppSession completeSignIn (79%→83%): a callback carrying error= is denied, a callback with no code is .missingCode, and a failed code exchange surfaces as a failure — none exchanging or logging in. - ReadingListViewModel (94%→95%): a 401 whose refresh also fails routes to the logout callback without an error banner; a collection warning populates warningText. - KeychainTokenStorage (57%→98%): a real simulator-keychain round trip through the production path (add / read / update / remove, key isolation) — the test double never exercised it. - TokenStore (85%→99%): extract the embedded-provisioning-profile parsing into a pure `parseAppGroupId(fromProvisioningProfile:)` and cover it with fixtures (valid profile, no plist, no app-group entitlement, empty group array); the `Bundle.main` read stays the only untested line.
Replace assertions that pass for the wrong reason with ones that pin the actual
value, per the test-driven-design conventions:
- SirenDate: assert the parsed instant equals a known UTC time (and that the
fractional variant is exactly 123 ms later) instead of `XCTAssertNotNil`, and
assert the fixture's `savedAt` parses to its exact instant — a parser that
returned the wrong time previously passed.
- PKCE: assert the challenge's exact length (43) and that every character is in
the base64url alphabet, replacing three `contains("+"/"/"/"=")` negatives that
a wrong-length or wrong-alphabet challenge would still satisfy.
- Login/Signup authorize tests: derive the expected host from
`AppConfig.serverBaseURL` instead of the literal "readplace.com", so the
assertion is environment-agnostic (a prerequisite for running the full suite
under the STAGING build).
… tested seams Two behavioural decisions were embedded in SwiftUI/WebKit glue with no unit-test seam, unlike their already-extracted twins (`ToolbarRoute`, `HTMLCaptor.navigationResponseDecision`): - `ItemRoute` lifts the per-item routing out of `ReadingListView.activate` — a link opens, a destructive action confirms, any other action invokes — so the contract-relevant behaviours (a link-only item control opens rather than being dropped; a destructive control never invokes without confirmation) are unit-tested without a view. - `WebResponsePolicy` lifts the "a 4xx/5xx status is a failure" rule out of the `WebPageView` navigation delegate (WKWebView delivers error statuses through `didFinish`, so the web view would otherwise paint the error body), tested at the 399/400 boundary. Both new files are gated at 100%; the views keep only their effect/plumbing.
…ard-coding it The iOS reader mints a browser session cookie from the bearer token before loading the cookie-authenticated reader page. The client hard-coded that route and method (`POST /auth/session`), so a server-side rename would break every article tap until an App Store release — the exact URL coupling the hypermedia contract exists to remove. - Server: advertise a `create-session` action on the collection (href `/auth/session`, method POST). It carries no fields — the route reads the bearer from the Authorization header — and no title, since it is a machine capability a client invokes bespoke rather than a rendered control (like `save-articles`). - Client: discover the action from the loaded collection and follow its href and method to mint the session; map `create-session` to a non-toolbar presentation so it never renders as a control. A server that hasn't advertised the action yet falls back to the fixed path, so an older shipped build keeps working. The action is additive and non-breaking: a client without a handler skips it.
…-coding "read" The iOS client derived an article's read indicator from the status vocabulary (`status == "read" || readAt != nil`) — a domain literal baked into the client that goes stale if the server's status vocabulary evolves (e.g. an `archived` state, or keeping `readAt` after a mark-unread). - Server: emit an explicit `isRead` boolean property on the article entity (already computed for the toggle label), so the read-state is server-authored. - Client: render read-state from the server's `isRead` when present, falling back to the old status/readAt derivation only for an older server that doesn't emit it (App Store back-compat). The property is additive and non-breaking. The optimistic row-removal prediction on the swipe mark-read (`removesItem`) still reads the update-status field value; that path is the client-side optimism that was deliberately kept, and its literal is out of scope here.
…owns htmx The iOS reader detected its mark-read by injecting a script that sniffed the web front-end's htmx internals (the `htmx:beforeSwap` event and `detail.requestConfig` shape). That coupled the app to the server's rendering-engine implementation: swap htmx for plain fetch/forms, or upgrade across an event-shape change, and mark-read silently stops closing the sheet — with no contract between the two sides. - Server: the chromeless (app) reader now posts the `markedRead` message to the WKWebView itself, from a server-authored script guarded on the WKWebView message handler so it is inert in the same reader served to a browser. The htmx detail stays on the server that owns htmx. - Client: stop injecting any script — the app only registers the `readplaceReader` handler and reacts to the `markedRead` message, holding no knowledge of the front-end's event shape. The pure message parser stays unit-tested. `readplaceReader` / `markedRead` is the stable contract between the two sides. This completes the reader fix begun by re-reading the collection on the signal instead of inferring the mark-read direction.
FagnerMartinsBrack
force-pushed
the
ios/server-reader-signal
branch
from
b192a86 to
40298b3
Compare
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
Rebased Conflicts resolved (all reproducing the PR's own authoritative resolution from the pre-rebase merge tip, since
The final rebased Swift sources/tests are byte-identical to the pre-rebase tip. |
|
@claude CI has FAILED. Use your Read tool to read the local file Context
Failed Jobs
|
|
Claude finished @FagnerMartinsBrack's task in 9m 39s —— View job CI Failure Fix — PR #927 (Attempt 3 of 5)
All 262 tests pass ( The important change since attempt 2: 3 of the 4 are fixable without touching the baselineAttempts 1–2 asked you to weaken the gate on all of these. That was the wrong ask. Re-reading the code, three of the four can be fixed the way the repo's "Coverage Over Legibility" rule wants — raise real coverage / relocate the untestable path — needing no approval at all. Only
What I need from you — one lineAdd
Why I'm not pushing anything this attemptCI cannot go green until --- · failed CI run · branch |
…loors The per-file coverage gate's first CI runs failed on four files whose floors were seeded from a different Xcode. All four now clear their floors by raising real coverage; coverage-baseline.json is untouched: - SaveSharedPage 91->98: assert the generic catch surfaces an undecodable queue body as .failed without attempting a save. - ShareURLExtractor 95->100: fold the nil-context guard into the item-list overload and lift the simulator-dependent loadItem coercion into a pure coerceURL seam, pinned shape-by-shape. - WebDialog 47->100: move the UIAlertController presentation glue into ReaderWebView.swift, the WKWebView OS boundary that is its only caller (already excluded); the pure dialog mapping stays measured. - LoginView 88->97: inject the session and the WebAuthFlow factory so startLogin/startSignup are directly testable; pin that Login and Sign up start the correct authorize request and clear a stale auth error, and render both screen states deterministically.
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Resolved without any gate change — CI is green ( The approval the CI fixer was blocked on (attempts 1–3) turned out to be unnecessary: all four files were lifted above their floors by raising real coverage, and
Validated locally with the exact CI sequence ( |
|
Claude finished @FagnerMartinsBrack's task in 7m 27s —— View job PR Review — #927 (generic hypermedia client + coverage gate)
Verdict: no high or medium priority issues. The formal review (with the Highlights from my read:
Two low-priority observations only (details in the review block): a doc/behaviour nuance in Verified |
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found Low Priority Suggestions1.
|
1 participant
Consolidates what were 9 stacked PRs (#919–#927) into one PR targeting
mainso CI runs and the whole thing is independently deployable. They were stacked because the changes share 14 files and form a dependency chain (everything builds on the base client refactor; the coverage work shares one baseline file), so they can't be separated into independentmain-targeting PRs without cumulative diffs — they need coordination, hence one PR. The branch is merged up to currentmain; future drift is left to the conflict-resolver.This makes the iOS app behave as a generic hypermedia client — it knows only the entry point, the Siren media type, and the action names / link rels it supports, and discovers or tolerates everything else — and closes the coverage/testability gaps a review found. Server changes are additive and non-breaking.
1. Client contract fixes — stop hard-coding server internals
hutch_sid; now the client forwards every cookie the/auth/sessionresponse set and scrubs sign-out by server host. A server cookie rename no longer breaks the reader.update-statusis a server toggle; the client used to infer "read" from a status POST and drop the row. It now re-reads the collection and adopts the server's truth, so a toggle back to unread no longer wrongly removes the item.warningdecoded atomically with a requiredcodethe client never reads; dropping/renamingcodewould blank the whole page. Nowcodeis optional and the warning decodes leniently → degrades to no banner.["alternate","next"]can't render as a control while also driving pagination.readentity link as a control, so a future item link (e.g.share) renders instead of being discarded./queue,/auth/session,/oauth/token,SESSION_COOKIE_NAME,isFile,/login,/signup) — they rot silently and reintroduce the URL coupling the contract removes.2. Coverage & testability
make testnow runs with-enableCodeCoverageand fails below a per-file floor (scripts/check-coverage.py+coverage-baseline.json) — the iOS suite previously ran with no coverage measurement at all. It's a ratchet: pure SwiftUI views and WebKit/OS-boundary glue are excluded; every other file must stay at or above its floor; a new logic file must be 100%.ShareURLExtractormoved intoShared/(with an item-list core tests can drive), and the outcome→status mapping extracted into a pureShareStatusPresentation— both previously uncoverable in the extension-only target.parseAppGroupId), plus OAuth malformed/refresh/revoke branches,completeSignInerror paths, and the VM 401→logout route.XCTAssertNotNil/negative assertions on dates, the PKCE challenge, and host pins with assertions that pin the actual value.ItemRoute(per-item routing) andWebResponsePolicy(4xx handling) lifted out of the SwiftUI/WebKit glue into pure, gated seams.3. Server-coordinated hypermedia fixes (server + client, additive)
create-sessionaction on the collection; the client follows its href/method instead of hard-codingPOST /auth/session, falling back to the fixed path for an older server.isReadboolean on the article; the client renders from it instead of hard-coding the"read"status literal, falling back to the status/readAtderivation only when the server omits it.markedReadmessage to the WKWebView itself (guarded so it's inert in a browser); the app injects no script and no longer sniffs the front-end's htmx internals.readplaceReader/markedReadis the stable contract.Deploy notes
All server changes are additive/non-breaking (a client without a handler skips them). A2/B3 deploy in either order with the client; B2's server change should land before the app build that depends on it. Nothing invalidates existing installs.
Testing
make test.collection-siren,article-siren, andqueue.reader-platformsuites pass (incl. the newcreate-session,isRead, and reader-bridge assertions). Fullpnpm checkruns in CI.Supersedes and closes #919–#926.