fix(ios-readplace): remove hard-coded server internals from the client#919
fix(ios-readplace): remove hard-coded server internals from the client#919FagnerMartinsBrack wants to merge 4 commits into
Conversation
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.
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 8m 9s —— View job Review: remove hard-coded server internals from the iOS clientThis is a coherent, well-reasoned PR that does what it says: each item removes a real coupling and is independently shippable. I traced every change end-to-end. The B1 fix is correct ( One genuine gap, and a few low-priority observations. High Priority Issues (Must Fix)None found Medium Priority Issues1. The headline C3/C1 behaviors ship without testsThe two most novel guarantees in this PR have no test pinning them, and the iOS suite has no coverage gate (
Suggested additions (all pure decode/unit tests, no view host needed): // C3 — item links become row controls, without double-rendering read
func testRowControlsSurfaceANonStructuralItemLinkAndNeverTheReadLink() throws {
let json = """
{ "properties": { "id": "x", "url": "https://example.com/x" },
"links": [
{ "rel": ["read"], "href": "/queue/x/view" },
{ "rel": ["share"], "href": "/queue/x/share", "title": "Share" },
{ "rel": ["item"], "href": "/queue/x" },
{ "rel": ["alternate", "next"], "href": "/queue?page=2" }
] }
"""
let article = try XCTUnwrap(Article(entity: try decodeEntity(json)))
XCTAssertEqual(article.rowControls.compactMap(\.link?.rel.first), ["share"],
"only the non-structural, non-read item link surfaces as a control")
XCTAssertEqual(article.readHref, "/queue/x/view", "read stays the primary tap, not a duplicate control")
}
// C1 — a code-less warning still shows; a malformed one degrades without blanking the page
func testWarningMissingCodeStillSurfacesItsMessage() throws { /* warning: { "message": "…" } → warning?.message set, code nil */ }
func testMalformedWarningDegradesToNoBannerButKeepsTheCollection() throws { /* warning: { "code": "x" } (no message) → warning == nil, articles still decode */ }Low Priority Suggestions1.
|
Review: remove hard-coded server internals from the iOS clientThis is a coherent, well-reasoned PR that does what it says: each item removes a real coupling and is independently shippable. I traced every change end-to-end. The B1 fix is correct ( High Priority Issues (Must Fix)None found Medium Priority Issues1. The headline C3/C1 behaviors ship without testsThe two most novel guarantees have no test pinning them, and the iOS suite has no coverage gate (
Low Priority Suggestions1.
|
|
@claude Fix the review issues. Use your Read tool to read the local file Context
Review to AddressSee the review comment above this one for the issues to fix. |
|
Claude finished @FagnerMartinsBrack's task in 17m 23s —— View job Fixed the medium-priority review issuePushed
What was fixedThe only medium issue was the test-coverage gap on the PR's headline C3/C1 behaviors (no high issues; the 4 low-priority items were intentionally left per the auto-apply scope). Added 3 XCTest cases to
Approach & why
|
…haviors Addresses the medium-priority gap from the PR #919 review: the C3 row-control surfacing (isSemanticControlLink / rowControls) and the C1 warning degradation paths shipped without tests, and the iOS suite has no coverage gate to catch a regression. - C3: a row surfaces a non-structural item link (share) as a control, keeps read as the primary tap (readHref) without double-surfacing it, and excludes a structural link (item) and a multi-rel link carrying a structural rel (["alternate","next"]). - C1: a warning missing code still surfaces its message; a malformed warning (missing the required message) degrades to no banner while the rest of the collection still decodes. Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 8m 56s —— View job Review complete — PR #919 (commit
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found The prior review's sole medium — the C3/C1 test-coverage gap — is resolved by commit
Assertions are positive and non-vacuous (the malformed-warning test pairs Low Priority Suggestions1.
|
|
@claude Please address the following low-priority review suggestion. It's a clean, in-scope precision fix — no design decision needed. Tighten the cookie-host match in The sign-out scrub matches cookies with where cookie.domain == host || cookie.domain.hasSuffix("." + host)This still matches the exact host and every legitimate subdomain ( |
|
Claude finished @FagnerMartinsBrack's task in 11m 37s —— View job Tightened the cookie-host match in
|
removeReaderWebStoreData matched the reader's WebKit cookies with cookie.domain.contains(host), which also matches any domain that merely contains the host as a substring (e.g. readplace.com.attacker.example). Match the exact host or a dot-prefixed subdomain instead, so the sign-out wipe still covers the host, the leading-dot domain cookie (.readplace.com), and legitimate subdomains (app.readplace.com) without reaching a spoofed foreign origin. On sign-out the old predicate only ever over-deleted, so this narrows the scrub to exactly the app's own origins. Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 4m 47s —— View job Review — PR #919 (commit
|
Review — PR #919 (commit
|
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 11m 47s —— View job CI failure: the new coverage gate, not a test failureAll 212 tests pass (
Root cause: a brand-new gate calibrated off a local runThe gate (
Why I'm not auto-applyingBoth Recommended fix (reply to approve)Calibrate the new gate to the CI environment — 2 of the 3 files aren't even in this PR:
Reply Alternative — don't loosen |
1 participant
Purpose
The iOS app is meant to be a generic hypermedia client: it knows only the entry point, the Siren media type, and the action names / link rels it supports — everything else (URLs, methods, field values, presentation) it must discover from the server or tolerate. A review found a cluster of places where the client instead hard-coded server internals or duplicated server domain logic, so a routine server-side change would break the app or force an App Store release.
This PR fixes the independent, client-only items from that review (the ones that need no coordinated server change). Each removes a concrete coupling and is individually shippable.
What changed and why it matters
A4 — Inject all bootstrap cookies; scrub by host, not by name
Problem:
bootstrapSession()selected the reader session cookie by the hard-coded namehutch_sid, and sign-out deleted it by name. If the server ever renamed its session cookie (a server-internal change by definition), cookie injection would return nothing and every in-app reader open would fail until an App Store release.Fix: the client now forwards every cookie the
/auth/sessionresponse set into the reader web view, and scrubs sign-out cookies by the app's own server host.AppConfig.sessionCookieNameis gone. The client no longer needs to know the cookie's name.B1 — Re-read the collection on a reader status change instead of inferring "read"
Problem: the reader bridge fired on the presence of a
statusfield and the client unconditionally treated it as "marked read", dropping the row. Butupdate-statusis a server toggle (value: "read"or"unread"), so a toggle back to unread wrongly removed the item.Fix: on a reader status change the client re-reads the collection and adopts the server's truth (a shallow list) rather than inferring a direction. The row leaves only if the server's unread-only collection no longer lists it. (A deep-scrolled list holds its position and reconciles on the next pull-to-refresh.) The durable server-owned signal is a follow-up PR; this removes the incorrect inference now.
C1 — Tolerate an evolving/malformed collection warning
Problem:
SirenWarning.codewas a required decode key, decoded atomically, even though the client reads onlywarning.message. Dropping or renamingcodeserver-side — a property no client reads — would throw and blank the entire collection. The non-fatal warning channel could take down the page.Fix:
codeis now optional and the warning decodes leniently, so an evolving or malformed warning degrades to no banner, never a blank page.C2 — Exclude a link from controls when any rel is structural
Problem: the client keyed structural-link detection on
rel.firstonly. A multi-rel link like["alternate", "next"]keyed asalternate, escaped the structural filter, and would render as a tappable control while also being followed for pagination.Fix: a link carrying any structural rel (
self/root/prev/next/item) is never a control, mirroring the browser extension's walker.C3 — Surface an item's other semantic links as controls
Problem: each item reduced its links to a single
readpick; every other semantic entity link (a futureshare,summary, …) was discarded and could never render — the exact failure the affordance loop exists to prevent.Fix: the row now surfaces every non-structural, non-
readentity link as a control (keepingreadas the primary tap), so a newly-advertised item link renders with no client change.D — Drop comments that reintroduce URL coupling in prose
Removed comments naming concrete server routes/methods/symbols (
/queue,/auth/session,/oauth/token,SESSION_COOKIE_NAME,isFile,/login,/signup). The code discovers these from the response; a route baked into a comment rots silently and re-teaches the coupling the contract removes.Not in this PR (need coordinated server changes)
Split out because they can't be done client-only without regressing test-pinned behaviour:
POST /auth/session."read"status literal in two places, both pinned by explicit tests encoding intended dual-signal/toggle behaviour).Testing
Full XCTest suite green locally (207 tests, 0 failures) plus the STAGING smoke pass, run exactly as CI runs
make test.