refactor(api): move the Android clients to API v2 - #307
Merged
Merged
Conversation
Adds scripts/sync-apiv2-fixtures.sh, which copies the pilot-operation and generic problem fixtures plus fixtures.schema.json and a filtered index.json from a silo-server checkout into shared/src/commonTest/resources/api/v2/fixtures, recording the exact server commit in SOURCE (key=value convention). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
New lenient @serializable models under network/apiv2 for the four Android pilot operations plus Problem and SystemInfo. Enums are string-backed value classes whose unknown values stay observable (known == null) instead of collapsing to a default under coerceInputValues. PATCH bodies use a Patch wrapper so omitted members are absent and cleared members are literal null despite explicitNulls=false. ApiV2ContractTest decodes every vendored fixture with the production SiloJson and pins null/absence semantics, defaults, unknown fields and enum values, and the encoded PATCH shape. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ApiV2Probe GETs /api/v2/system/info once per established connection and returns V2(info) / UpdateServer / Failure(kind). Only a 404 with a text/plain Content-Type (the legacy listener's http.NotFound) is UpdateServer; HTML or problem+json 404s, HTML 200s, malformed JSON, 401/403, 429, 5xx, timeouts and connect/TLS exceptions each stay their own failure kind. ServerEntry gains a ServerContract state (UNKNOWN / V2 / UPDATE_REQUIRED) persisted through the registry; both server-setup view models probe the candidate on connect and show the update-server message. ApiV2Gate blocks pilot v2 calls in the UPDATE_REQUIRED state without ever redirecting to a v1 path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… to v2
getSetupStatus, getMe, listProgress, and updateProfile now call the v2
endpoints only (GET /api/v2/system/setup, GET /api/v2/account/me,
GET /api/v2/progress walked by cursor, PATCH /api/v2/profiles/{id}) with no
v1 fallback and no replay of a failed mutation. The API layer adapts the v2
models back to the v1-shaped User/Profile/ProgressListResponse so repositories
and screens keep compiling; User.id and impersonator_user_id become opaque
strings. DI wires ApiV2Gate and ApiV2Probe. ApiV2NoFallbackTest proves a failed
PATCH is not retried against /api/v1 and that UPDATE_REQUIRED blocks the call
before any request.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… refresh The v2 contract probe only ran inside AuthRepository.setServerUrl, so servers saved before this build stayed ServerContract.UNKNOWN forever: nothing probed when the app restored its active server on launch, when the user picked a server from the server list, when device pairing switched servers, or when the server identity was refreshed. A v1-only server could never reach the update-server state on those paths (F5). AuthRepository now mirrors Apple's AuthService: refreshActiveServerName() (re)establishes the contract verdict before reading the branding name, with an optional knownContract so the connect path records the verdict it already has instead of probing twice. A new switchToServer(id) is the single entry point for activating a registered server (registry switch + token scope + refresh); ServerListViewModel, TvServerListViewModel, and the pairing wrong-server screen in AppNavigation use it. RegistryPairingAuthPort cannot reach the repository during its restore-on-failure switch, so it calls refreshServerContract() after the switch. Both MainActivity variants refresh once when the launch path restores an active server. Recording is guarded against a switch racing the probe: the active server id is captured before the request and the verdict is dropped if it changed, so a slow answer never overwrites the newer server's state. Failures still record nothing (UNKNOWN is non-blocking). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RegistryPairingAuthPort re-probes the v2 contract after restoring the previous server on a failed account replacement, but every rollback test built the port without an AuthRepository, so that line never ran. Fail the token manager's post-commit hook so the registry has already switched to the new server when the error surfaces, then assert that a supplied AuthRepository issues exactly one GET /api/v2/system/info and records the verdict against the restored server. A second test pins the no-repository path: rollback still restores the old identity and leaves the contract UNKNOWN. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The server pilot branch gained schema examples, which changed the contract digest carried in get_system_info_ok.json. Re-run the sync script against that commit; SOURCE now pins 17380cbca59dff44d8e1454fd07ece37efc26afe. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rver AuthApi.getSetupStatus(serverUrl) targets a candidate server the app is not connected to, but it was wrapped in the ACTIVE entry's ApiV2Gate. A user whose active server is UPDATE_REQUIRED therefore could not add a different, supported server (both Add Server flows probe the candidate first), and could not reconnect to the same server after upgrading it. Explicit-server (absolute URL) v2 overloads now use ApiV2Gate.Unrestricted; the candidate's own gate is the contract probe ServerSetupViewModel runs before switching. The relative form still targets the active server and stays gated. getSetupStatus(serverUrl) is the only explicit-server v2 overload today; the other absolute-URL calls in network/api are v1. Test: ApiV2NoFallbackTest.explicitServerSetupStatusBypassesTheActiveServersUpdateRequiredVerdict Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RegistryPairingAuthPort refreshed the contract verdict only in the rollback branch. After a successful replaceAccountSession the newly paired server stayed at UNKNOWN (or a stale UPDATE_REQUIRED saved by an older build), so gated startup consumers acted on a verdict that was never established for it. Pairing is a server switch like every other path: after the session commits and before the port reports SignedIn, run refreshServerContract() for the now-active server. The rollback call is kept. The probe never throws on a failed request, so this cannot undo a committed session. The override now declares `: Unit` so the added tail expression does not change the port's return type. Test: RegistryPairingAuthPortTest.successfulPairingProbesTheNewServerExactlyOnce Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…aunch The launch probe in resolveStartDestination was fire-and-forget, so gated startup consumers (ProfileSelectionViewModel's admin lookup, and anything else behind ApiV2Gate) could run against a stale UPDATE_REQUIRED verdict saved before the server was upgraded, and fail without a request. Only UPDATE_REQUIRED is harmful: UNKNOWN and V2 both pass the gate. So resolveStartDestination (already a suspend fun inside a LaunchedEffect on both phone and TV) now calls the new AuthRepository.awaitContractRefreshIfUpdateRequired(), which probes and waits at most 3 s only when the stored verdict is UPDATE_REQUIRED, and passes the result to refreshActiveServerName(knownContract) so the background identity refresh does not probe twice. No runBlocking on the main thread; the wait runs under Dispatchers.IO. Consumers are made robust as well: AuthRepository exposes activeServerContractFlow (from registry.activeEntry), and ProfileSelectionViewModel collects it once in init and re-runs loadProfiles() when the contract moves from UPDATE_REQUIRED to V2. Tests: - AuthRepositoryContractTest: awaitContractRefreshIfUpdateRequired replaces a stale UPDATE_REQUIRED verdict / does not probe when the gate would pass - ProfileSelectionViewModelContractTest.v2VerdictArrivingAfterUpdateRequiredRetriggersTheGatedLoad Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Removing the ACTIVE server makes the registry promote the next-MRU entry, but neither server-list view model probed it: TV only called tokenManager.switchActiveServer(promotedId), and the phone did nothing after remove(). The promoted server kept whatever contract verdict an older build had stored (or UNKNOWN), and its fetched name was never refreshed. TV: replace the bare token switch with authRepository.switchToServer (registry switch + token switch + refreshActiveServerName). It is idempotent when the registry already points at promotedId — AndroidServerRegistry.switchTo just bumps lastUsedAt and re-applies the same active id — and the navigation logic after it is unchanged. Phone: mirror the TV wasActive/promoted logic and call authRepository.refreshActiveServerName() when the active server was removed and another one was promoted. Test: AuthRepositoryContractTest `switchToServer on the already-active id still probes` (no ServerList view-model tests exist). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…test ProfileSelectionViewModelContractTest asserted on a plain list the mock engine appended to from its own thread, so under the full unit-test run the first assertion could observe an empty list before the v1 request had landed. Requests now go through a channel and each expected request is awaited on a real clock (bounded), so the test observes network arrivals instead of assuming them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The server pilot branch documented updateProfile's real statuses and media type (contract digest moved) and switched listProgress to keyset cursors (list_progress_ok.json next_cursor changed). SOURCE now pins 74fe2b4ac97349167d1bb85892d7f0566bd43d49. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The probe mapped any 404 with a text/plain Content-Type to UpdateServer,
so a reverse proxy's own plain-text "Not Found" would tell the user to
update a server that may not be a Silo at all.
Decide on the body instead: only Go's http.NotFound text
("404 page not found", tolerating the single trailing newline it writes)
is UpdateServer. Leading whitespace, extra newlines, or any other body
stays Failure(UNEXPECTED_STATUS, 404). This matches the Apple client's
isLegacyNotFound rule so both clients reach the same verdict.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…efresh Both server-list view models await AuthRepository.switchToServer behind a spinner. It ran refreshActiveServerName, which awaits the contract probe plus the branding and health requests, so a saved server that accepts the TCP connection but never answers held the spinner for the full client timeouts (about 72 s). switchToServer now switches the registry and token scope, awaits only the contract probe bounded by the same 3 s used on the launch path (recording V2/UPDATE_REQUIRED, leaving UNKNOWN alone), and launches the display-name refresh on an injected process-lifetime backgroundScope without awaiting it. The scope is optional; without one the name refresh runs inline after the probe (single-server hosts, tests). The active-id guard still drops a late name for a server that is no longer active. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
listProgress promises the full progress list, but the cursor walk stopped at PROGRESS_MAX_PAGES and returned ApiResult.Success with only the prefix, so continue-watching consumers treated older entries as absent without any signal. The page size stays at the server maximum (200). PROGRESS_MAX_PAGES is now a runaway guard only (100 pages, 20,000 entries): reaching it with has_more still true returns ApiResult.Error(code = 0, error = "progress_incomplete") instead of Success. The function now documents that the result is complete or an error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
185d1e0 made the probe accept only Go's exact "404 page not found" text, so the two pairing tests that mocked a bare "not found" started recording UNKNOWN instead of UPDATE_REQUIRED. Mock the real legacy body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The server pilot branch constrained the profile PIN schema (contract digest moved) and bound listProgress cursors to the viewer's access policy (list_progress_ok.json carries a new next_cursor). SOURCE now pins c9e5a4e376f919a90e76aa8408892ba925d5661e. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… lifts When an upgraded server still holds a stored UPDATE_REQUIRED verdict and the launch probe outlasts its 3 s bound, MainTvActivity routes on the stale verdict and the background refresh records V2 afterwards. The TV profile picker called getCurrentUser() exactly once in loadProfiles(), so the gated admin lookup failed locally and the admin lost the profile-management controls for the ViewModel's lifetime. Mirror the phone ProfileSelectionViewModel: collect activeServerContractFlow in init and re-run loadProfiles() only when the verdict moves from UPDATE_REQUIRED to V2. The initial emission is skipped, so a fresh V2 or UNKNOWN start does not double-load. TvProfileSelectionViewModelContractTest proves a V2 verdict arriving after an UPDATE_REQUIRED first load re-triggers the load and lets the v2 admin lookup reach the network. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…time After a successful replaceAccountSession the pairing port awaited refreshServerContract() unbounded. The client request and socket timeouts are ~60 s while the companion waits only 30 s for PairingMessage.ServerResult, so a stalled /api/v2/system/info made the phone report failure after the credentials were already committed. Add AuthRepository.refreshServerContractBounded(timeoutMs = 3_000L) as the single home for the probe bound, and route switchToServer, awaitContractRefreshIfUpdateRequired, and both PairingAuthPort probe sites (success and rollback) through it. A timeout leaves the verdict UNKNOWN, which passes the gate and is re-probed on the next switch or launch. RegistryPairingAuthPortTest gains successfulPairingReportsSignedInWhenTheProbeNeverAnswers: a MockEngine that suspends until cancelled still lets the port return within the bound on runTest's virtual clock, with the session intact and no verdict recorded. The two existing probe-counting tests now run the port on a real-clock dispatcher, because under the test scheduler the new virtual-time bound fires before the engine thread can answer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…imes out switchToServer bounds the contract probe to SWITCH_PROBE_TIMEOUT_MS so the switch spinner stays responsive. When the target entry held a stale UPDATE_REQUIRED verdict and /api/v2/system/info answered after that bound, withTimeoutOrNull cancelled the only contract refresh; the background task afterwards refreshed just the display name, so the gate kept rejecting v2 calls for the switched session until the next launch even though the upgraded server would have answered moments later. Keep the switch bounded, but when the bounded probe returns null and a backgroundScope is wired in, launch a replacement unbounded probe (the client's own timeouts still apply) pinned to the target serverId. The existing active-id guard in recordServerContract drops the result if the user switched again meanwhile. Without a scope (tests, single-server hosts) behavior is unchanged. The launch path in MainActivity and MainTvActivity already follows awaitContractRefreshIfUpdateRequired with a fire-and-forget refreshActiveServerName(knownContract = null) that runs a full unbounded probe, so it needs no replacement. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
refreshServerContractBounded mapped a transient probe Failure (connection error, 5xx) to the non-null ServerContract.UNKNOWN. refreshServerContract correctly recorded nothing for it, but both callers read any non-null result as "verdict obtained": the launch path passed knownContract = UNKNOWN into refreshActiveServerName, which then skipped its own probe, and switchToServer skipped its background replacement probe. With a stored UPDATE_REQUIRED and a server briefly unreachable at launch or switch, the gate kept rejecting pilot calls for the whole session. Make "no verdict" uniform: the bounded probe now returns null unless the probe produced a real V2 or UPDATE_REQUIRED verdict, so a failure is handled exactly like a timeout. The launch path (MainActivity, MainTvActivity) already falls back to refreshActiveServerName(null), which re-probes unbounded on lifecycleScope; switchToServer now launches its background replacement for failures too. recordServerContract keeps ignoring UNKNOWN. PairingAuthPort discards the return value and needs no change. Tests cover a 503 answer returning null and switchToServer's replacement recording V2 once the server recovers, a connection failure on the launch guard returning null, and real V2 / UPDATE_REQUIRED answers still returning their verdict without a replacement probe. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…talls Both pairing probe sites (the newly paired server and the rollback restore) awaited refreshServerContractBounded() and discarded the result, so a timeout or transient failure while re-pairing a server that carried a stale UPDATE_REQUIRED left the session gated with no replacement probe — unlike switchToServer, which already handed the retry to the background scope. Extract that "bounded probe, then background replacement on no verdict" step into AuthRepository.refreshServerContractWithFallback (serverId) and use it from switchToServer and both pairing sites, so every path that activates a server shares one fallback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…failed The connect path mapped a transient candidate-probe Failure through toServerContract() into a non-null UNKNOWN and passed that to setServerUrl once /api/v2/system/setup succeeded. refreshActiveServerName neither records UNKNOWN nor re-probes when handed a known contract, so a stale UPDATE_REQUIRED on the registry entry survived a successful v2 connection and kept gating the session. A successful v2 setup answer is proof of v2 on its own, so both setup view models now pass CONTRACT_PROVEN_BY_V2_RESPONSE (ServerContract.V2) on the success path and keep the probe only for the UpdateServer early return. The shared value is documented in AuthRepository.kt so the reasoning lives next to toServerContract(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
probeServerContract ran the candidate probe unbounded, so a server that accepted the socket but stalled held the setup spinner for the client's ~60 s timeout per candidate (HTTPS then HTTP for a bare host) before the setup call waited again. Bound it with withTimeoutOrNull(SWITCH_PROBE_TIMEOUT_MS) behind a timeoutMs parameter. A timeout returns null, which the connect path already treats as "no verdict"; getSetupStatus carries the client's own timeout and decides reachability, so no second bound is added there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
On the first launch after upgrading the app, restored server entries carry ServerContract.UNKNOWN, which passes ApiV2Gate. Authenticated startup consumers (getCurrentUser and friends) then raced the background probe and could receive raw v2 404s from a saved v1-only server before UPDATE_REQUIRED was recorded; those one-shot loads do not retry. Generalize awaitContractRefreshIfUpdateRequired into awaitContractRefreshIfUnsettled: only a stored V2 skips the bounded wait; UNKNOWN and UPDATE_REQUIRED both probe before routing. Both activities keep passing the result as knownContract to the fire-and-forget refreshActiveServerName, so a null (V2, timeout, or failure) still re-probes off the critical path. No other callers used the old name, so it is renamed rather than aliased. Tests: UNKNOWN awaits and records UPDATE_REQUIRED from a v1-only server; UNKNOWN awaits and records V2; V2 returns null with zero requests; existing UPDATE_REQUIRED cases unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Removing the ACTIVE server on the phone promotes the next-MRU entry inside the registry, and ServerListViewModel.onRemove re-probed it via refreshActiveServerName in viewModelScope. Popping the server list cancels that scope, so the promoted entry could keep UNKNOWN or a stale UPDATE_REQUIRED for the rest of the session. Route the promotion through AuthRepository.switchToServer(promotedId) like the TV client already does. The registry already points at the promoted id, so the switch is idempotent there: EncryptedTokenManagerImpl.switchActiveServer returns early when the id is unchanged (no credential-epoch bump, no cache reload), and the bounded probe hands its replacement to the repository's process-lifetime backgroundScope with the display-name refresh off the critical path. Test: promoted server re-probe survives cancellation of the calling scope (AuthRepositoryContractTest). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The server pilot branch paginated listAdminUsers, declared the statuses the listener really emits, relaxed the profile quality/subtitle enums, and bound listProgress cursors to the library-restriction flag (contract digest and list_progress_ok.json next_cursor moved). SOURCE now pins 7bd3af1430962a8b6662510caffe49e2954c4055. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
listProgress let every page's client.get resolve the active auth scope afresh, so a server or profile switch mid-walk sent later pages under a different identity while `entries` kept the earlier pages. The server binds the cursor to user + profile and answers 400 invalid_cursor, so mixed results could not surface today, but the walk relied on that server behavior and reported a confusing cursor error. The walk now captures one AuthScopeSnapshot up front (or takes an explicit `scope`, mirroring addFavorite/syncProgress) and pins every page request with authScope(), so SiloAuthPlugin sends the same server, profile, and credential slot on each page. Before each later page the captured scope is compared with the live one via the new AuthScopeSnapshot.isSameIdentityAs (extracted from ProfileRepository.identityScopeUnchanged); a generation or epoch change aborts with ApiResult.Error(error = "identity_changed") instead of continuing. PersonalDataApi now takes the TokenManager from Koin; the default null keeps single-scope test constructions unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ncel it refreshServerContractWithFallback awaited the bounded probe in the caller's coroutine and only then launched the unbounded replacement on backgroundScope. A server-list viewModelScope cancelled by Back during the bound propagated CancellationException out of the bounded call, so the replacement never started and the switched-to server stayed UNKNOWN or a stale UPDATE_REQUIRED for the session. With a backgroundScope and probe wired in, the whole bounded-then-unbounded sequence now runs in a job launched on backgroundScope before the caller waits; the caller only awaits the bounded verdict through a CompletableDeferred under its own withTimeoutOrNull. Cancelling the caller cancels that await, not the probe. The active-id guard stays inside the background job, and recordServerContract still drops verdicts for a server that is no longer active. Without a scope the inline behavior is unchanged. Tests cover a caller cancelled mid-bound whose late answer is still recorded, and a caller cancelled with the server switched meanwhile that records nothing. Three existing tests moved their background scope to a real-clock dispatcher because the probe no longer runs in the caller. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Quick104 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedToo many files! This PR contains 539 files, which is 439 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (539)
You can disable this status message by setting the 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. Comment |
|
Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting). This review would cost an estimated $10.90, which exceeds your per-review limit of $10.00. The top 3 files driving up this estimate:
Tip To get this pull request reviewed, you can:
|
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a 539-file API migration that substantially changes production networking, identity scoping, playback recovery, downloads, persistence migrations, notifications, and UI behavior. Its broad runtime, schema, and authentication-sensitive impact is beyond a low-risk refactor or bounded additive change. Not approved because:
Review your spending limits in Billing settings, or comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01a6a92896
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`main` replaced season and episode detail pages in place inside the browse deck (#302) while this branch removed the deck and made item detail an ordinary full-screen page. Keep the full-screen page and carry the in-place series redirect onto it: - The route holds the resolved series id, season, and episode id, feeds a keyed `ItemDetailViewModel` through `DEFAULT_ARGS_KEY`, and publishes the displayed identity so external links still compare against what is on screen rather than the original route argument. - A pending redirect selects the loading branch of the detail crossfade, so the season or episode detail on its way out never dissolves into view. - Drop `videoCastRequest`; `main` removed its only caller with the detail-page "Play on device" action. The overlay remote-control button this branch added keeps its own picker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are unbounded-retention defects found by Codex on this PR. `ActiveProfileStore` kept a cached profile across an identity change. The session-expiry observer routes to Login without resetting this singleton, so the next account to sign in and fail its first profile fetch drew the previous user's name and avatar in the header. The cache now belongs to the profile it was loaded for and is dropped the moment the active profile moves. `SequencedPlayback` never pruned settled attempts. The journal is serialized whole on every write and progress writes land every ten seconds, so each finished playback made every later write larger than the last — retained replan responses carry full subtitle inventories. Settled attempts are now compacted to tombstones that keep only what still answers attempt-id fencing and `owns`, capped at the eight most recent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DownloadWorker derived a client with followRedirects = false and closed it
in finally. Ktor propagates manageEngine through config {}, so that close
shut down the app-wide OkHttp engine and every later request failed until
restart. Use the shared client directly; redirects on the file route are
followed again and Ktor already strips Authorization across origins.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ocking the item A failed or lost v2 personal write left a personal_uncertain outbox row that nothing cleared, so every later watched or rating write for that item was refused. These are idempotent desired-state writes: abandon the row on any non-success and let a newer write supersede a stale v2 row. Legacy v1 rows still block as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…mutex ProxyAuxiliaryRequestHeaders.isCurrent took SequencedPlayback's mutex, which start, replan, progress and stop hold across their HTTP calls, and it runs under runBlocking on a Media3 loader thread. Read volatile snapshots of the auxiliary generation and live attempts instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A single 5xx or network error ended the flow with PollUncertain even though the pending authorization was still valid. Retry on the poll interval; a consumed code already terminates through the 404 branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rly return Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he inbox Every non-close event ran a full refresh (unread count, first page, up to 20 sync pages). Fold created/read/readAll locally through applyEvent and reread only on the connection snapshot. markRead no longer rereads either. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 045bfe0059
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…URLs Two tests still asserted the IllegalArgumentException that 1c5a9bf removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
updateProfile was the only profile mutation that bypassed the captured-scope exchange helper, so a PATCH overlapping a profile, account or server switch could be sent under the new identity and reported as saved for the old one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ownership check An identity change between completeWrite and the sidecar write left the original owner's sidecar at downloading with a staged URI that had already moved, and the terminal failure meant WorkManager never retried. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ce7be0988
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The stub clients for catalog, downloads, reader, recommendations and metadata ran on MockEngine's default dispatcher, so a response could land on a real thread after Dispatchers.resetMain() and fail the test with a DispatchException on CI. Pin them to the scheduler and cancel the view models before Main is reset. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…etry The v2 file route sends one bare conflict problem for preparing, cancelled, failed and revoked downloads alike, so the client cannot distinguish them. Rewrite the comment that still described the v1 download_inactive code and remove the unused v1 error-code extractor and its tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Preserve the server-wide unread count during event folds
When unreadCount comes from /unread-count, it can exceed the 25 notifications loaded into rows. Folding a Created or Read event recomputes the count from only those visible rows, so an account with 100 unread notifications can suddenly show at most 26 after one realtime event or local mark-read action. Apply an event delta to the existing total, or refresh the authoritative count, rather than replacing it with the partial-page count.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Content ids may contain reserved path characters. The watch route already encoded them; the catalog item, series, version, episode, favorites and watchlist routes interpolated the raw id and split it into extra segments. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tion A cross-device mark-all-read arrives as Invalidate because its signed cutoff cannot be folded locally. Treat it like the connection snapshot and refresh. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The mock engine evaluates handlers on its own threads, so a request issued just before an identity transition could be checked after it and fail the owner assertion on CI. A request that carries an older generation is expected to name the old owner. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Android phone and TV clients talked to the server over
/api/v1/. Theserver has since redesigned those routes as
/api/v2/with different shapes,stricter identity scoping, and — for playback — a genuinely different start
contract. Until the clients moved, every new server capability had to be
back-ported into v1 shapes, and a v1-only client could not express things the
v2 API makes explicit: which identity a read belongs to, which session owns a
playback mutation, and whether the server is new enough to serve the call at
all.
Solution
Every HTTP call the Android clients make now goes to
/api/v2/, with threedocumented exceptions.
docs/api-v2/android-migration-status.mdis the ledgerand lists each endpoint family and its status; it is verifiable against the
source with the
greprecorded at the top of that file.The substantive pieces:
ApiV2Gateblocks v2 calls when the activeserver reports
UPDATE_REQUIRED. The contract is probed at launch, on serverswitch, on identity refresh, and after pairing, with bounded waits so a stalled
probe cannot hold up sign-in.
issued under, so a profile or server change mid-flight cannot land a write
under the wrong identity or publish a stale read.
PlaybackDecisionshape. Start was redesignedserver-side. Sessions are owner-bound, terminal decisions that allocate no
session settle correctly, and recovery is durable across process death.
notification/section/recommendation/catalog remnants, the bound audiobook
timeline, and the owner-loss recovery union.
scripts/sync-apiv2-fixtures.sh, so the contracttests check against real server responses rather than hand-written shapes.
This branch also carries unrelated UI work
Flagging this plainly rather than burying it: the last several commits are phone
and TV UI changes with no connection to the API migration — the fullscreen
player HUD insets, a flattened playback settings menu, the item detail page
becoming an ordinary full-screen page, server addresses in the TV setup sheet,
and season/episode numbers on TV Home cards. By the repository's own
one-concern-per-PR rule these belong in their own PR; they are here at the
author's direction.
Validation
./gradlew :androidApp:testDebugUnitTest :androidTvApp:testDebugUnitTest—green (1154 TV tests).
the emulator: Pixel 11 Pro (phone) and a Google TV Streamer (
kirkwood,Android 14, 32-bit
armeabi-v7a). Launched clean, no crashes inlogcat -b crash, TV cold start 679–705 ms across three runs with the baseline profileinstalling.
device. Before/after screenshots are attached below.
Risks
single unit in one sitting; the per-commit history is the useful granularity.
proguard-rules.proover areflection-heavy stack. R8 breakage is runtime-only, so the release smoke
tests above are load-bearing.
/api/v2/. Olderservers are handled by
UPDATE_REQUIRED, but that path is worth exercisingdeliberately before release.
TvNextUpSelectionHandoffTest.profileOrServerChangeClearsPendingNextUpHandofffailed once and passed on re-run with no change to it or its subject. It is a
coroutine/ktor-mock timing test and looks order-dependent. Not introduced
here, not fixed here, worth its own issue.
Follow-up
calendar, detail rail, and inbox. Only the Home screen was unified here.
:baselineprofile-tvlast generated; regenerating it against the current UI is the next real
performance lever.
AI disclosure
Written with AI assistance.
claude-fable-5-1(Claude Fable 5.1) authored the bulk of themigration — 69 commits carry its
Co-Authored-Bytrailer.claude-opus-5[1m](Claude Opus 5, 1M context) authored 4 commits: theplayer HUD fix, the playback settings menu, the TV setup sheet addresses, and
the TV Home episode numbers.
TvMediaRow.ktis attributed to Codex fromearlier work on this repository.
All changes were reviewed, built, and tested on real devices by a human-directed
process; the validation evidence above was produced by running the commands and
installs listed, not inferred.
Note
Move Android and TV clients from API v1 to API v2 with identity-fenced operations
ApiV2Gatebacked by aServerContractprobe; v2 operations are blocked with anupdate_servererror when the active server is v1-only, while unauthenticated probes (branding, setup status) bypass the gateAuthScopeSnapshotownership checks andOwnerPolicyguards on v2 calls so captured identity (server, profile, credential epoch) is validated before and after each request; stale results returnidentity_changedinstead of publishingMembershipPortoutbox (MembershipOutbox,MembershipRuntime,RoomMembershipPort) that records commands, dispatches single-attempt mutations, and reconciles failures; watched/rating writes go through aPersonalWritejournal with the same ownership fencingSequencedPlaybackwith a durablePlaybackJournalStore, v2 control tickets for realtime WebSocket connections, and proxy auxiliary header capture scoped to issued subtitle URLsSettingsCapabilitiesResult,safeStatusCall, legacy personal-data operations,getReaderConfig/saveReaderConfig,updateAnnotation,getConversionCapability); any out-of-tree callers of these will break.PersonalDataApino longer exposes favorites, watchlist, progress sync, ratings, or watched-state methods.EbookReaderApino longer exposes config or annotation operations.SettingsApino longer exposes per-setting CRUD or subtitle-appearance methods. User/request/notification/suggestion identifiers changed from numeric to string on the wire. Download and push responses now require additional fields (generation, server-device ID).Macroscope summarized a22c27b.