fix(profiles, nav): commit identity atomically, and stop navigation losing entries - #175
Merged
Merged
Conversation
Audit of the profile/PIN path found the client could claim one profile while holding another's proof, and could act on a verification the user had already abandoned. - selectProfile now writes id and token together. Previously it set only the id, so phone carried the PREVIOUS profile's token into the new selection and every request went out as X-Profile-Id: B with A's X-Profile-Token. TV escaped it only because its switch path happened to call clearProfile() first. Both real token managers do this in one lock and one preferences edit, so a crash cannot persist a mismatch. - verifyPin no longer touches TokenManager. It used to persist the token into whatever server slot was active when the response landed, so cancelling mid-flight or switching servers could install one server's profile token as another's. It is now a pure query and the caller commits the answer. - Both selection ViewModels gained a generation guard around the verify round trip. Cancel during verification still entered the profile. - Verification is gated on an issued token, not a bare valid=true, so the client cannot enter a protected profile holding nothing to present. - Profile commits are refused outright while a remote-playback overlay owns identity, and the managers refuse to merge into an overlay. - Profile list responses are dropped if the identity they were fetched under has been replaced; a stale grid let the user pick a profile from a session the app no longer holds. - Phone kept the raw PIN in rememberSaveable, which the OS serializes across process death. Now plain remember. Server side was verified against the running prod revision 8bde6f11 (== upstream/main HEAD): canManageHousehold requires admin or an active primary profile, plus a valid X-Profile-Token when that primary has a PIN, so the PIN boundary is enforced server-side and none of the above was a privilege escalation. Known and deliberately left: paired READS still take the lock separately (AuthInterceptorImpl, MediaAuthSession, PlaybackRealtimeClient), the ServerRegistry second durable write, TV profile-picker sign-out not clearing saved credentials during an overlay, and no picker reload on TEMPORARY_SCOPE_END. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
…ntries `launchSingleTop` matches the destination NODE, not its arguments, and every item-detail route shares one node. So navigating from detail A to a related item B reused A's entry instead of pushing, and Back from B skipped A entirely. The comment at the call site asserted the opposite. - Item detail now collapses only an exact repeat (same contentId AND season), which is what that comment claimed launchSingleTop did. Applied to related items, series, seasons, person detail, both collection screens, the shell, and the silo://item deep link. Season paging keeps its deliberate popUpTo replace. - Playback navigation is worse under single-top: AndroidX reuses the entry, so its ViewModelStore survives and the previous title's player ViewModel can live on beside the new one. Home Play had no guard at all while detail Play had one, so a fast double Select stacked two players and two sessions. All playback now goes through one helper that suppresses only when the entry THIS request created is still on top, and otherwise takes over the current player rather than stacking. Weaker keys were tried and rejected: the route alone ignores what is on top, and route+contentId collides when another path puts up the same title with different arguments. - Watch Together's player target now shares that bookkeeping instead of single-topping the current player in place, which preserved the entry id and could make a stale record look current. - Cast launch replaces whichever player is on top, not only the video one; it stacked over an audiobook and Back resurrected it. - Content ids are percent-encoded in routes on both clients, and the phone deep-link parser reads rawPath and decodes exactly one segment instead of interpolating an already-decoded value back into a route — silo://item/ abc%3FseasonNumber%3D9 injected a route argument. playerRouteIntentOrNull decodes back so an already-showing player still matches. - Phone route encoding uses java.net rather than android.net.Uri, which is stubbed under plain JVM tests and silently yields "item/null"; that had already broken two existing suites mid-change. - Duplicate-nav guards added to the remaining argument-free destinations, and two dead launchSingleTop options removed (popUpTo is evaluated first, so they could never match). Seven review rounds; five returned NO-GO on the playback suppression key before it was keyed on back-stack entry identity with arrival confirmation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
The remaining findings from the navigation and profile audits. - Bottom-nav tab switching popped to a hard-coded Home. An offline launch with downloads starts on Downloads, and popping to a route that is not on the stack pops nothing, so every tab stacked and Back walked back through previously visited tabs instead of leaving. The anchor is now read from the live back stack, because neither a hard-coded route nor the graph's declared start survives that tab later disappearing. - A tab that can no longer be shown is now removed rather than left at the bottom of the stack, where Back revealed it and its own effect bounced straight back. This also covers the anchor vanishing while the user is on a different tab, which nothing previously noticed. No saveState on that path: restoreState is evaluated before launchSingleTop, so saving the vanishing tab and restoring on the way out immediately restored the subtree that had just been popped. - Cold device links were the graph's start destination, so a signed-out user got Pair Device at the root; its Sign In pushed Login, and the successful login cleared the stack and lost the pairing request. They are queued as pending external routes instead, so the normal gates run and pairing arrives on an authenticated stack with somewhere to go back to. The PairDevice destination's navDeepLinks are gone — Navigation matched the launch Intent itself when the graph was installed, which was the same bypass. - HTTPS device links carry the origin that issued the pairing request and it was discarded, so the code was looked up against whichever server happened to be active. The origin is now parsed (typed, default ports normalized), checked, and carried on the pending request so it survives the wait through setup and login and is re-checked at delivery. A link naming a different configured server is refused; that refusal is silent, which is recorded as a known gap needing a product decision. - The launch Intent was re-parsed on every Activity recreation, pulling the user back to a link they had already followed. Delivery is now recorded on the Intent for in-process recreation and in saved state for process death. - Profile id and token are read as one identity in the three places that assemble both into a request. The write was already atomic; separate reads could still pair an old id with a new token across a switch. Test doubles delegating TokenManager needed getProfileIdentity overrides: interface delegation forwards a default method to the delegate, so they were silently exercising the wrong identity while staying green. Eleven review rounds. The last five were all one root cause — save/restore state on a tab that can no longer be shown — and each fix exposed the next. Unrelated: androidApp PlayerViewModelLoadOwnershipIntegrationTest > exitDuringDeferredLoadRejectsAndStopsLateReady is a pre-existing flake, timing out at 5s on roughly two runs in three, including on a clean HEAD. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Closes the three items left open after the navigation work. The flaky player test. It was not load — it failed 2 in 6 on an idle machine. Its wait helper hopped to Dispatchers.Default and polled in real time while the work ran on UnconfinedTestDispatcher, which resumes inline or queues on the thread's unconfined event loop; the poller could win that race and time out with the continuation still queued. Every test in the class was exposed. It now uses StandardTestDispatcher, waits on replayable StateFlow signals instead of polling, and keeps a real-time deadline only to turn a hang into a failure. That deadline is 30s, not 5s: five was tight enough that a full-suite run could blow it while the work was merely slow, which looks exactly like the race being removed. Also fixed a test whose final wait was already true when called, so it asserted nothing. Mutation-verified twice; 12 consecutive runs clean. Notifications acted on whoever was signed in when they were tapped, which for a PendingIntent can be days and several profile switches later. They now carry the identity that generated them, established from the fetched row and a scope that held across the fetch, or not at all — a partial identity is worse than none, because a missing component is a wildcard at delivery. Unattributable ones still post so the user knows something happened, but carry no route, no identity and no item text. Delivery independently requires a complete identity, so a notification from an older build or an Intent crafted against the exported Activity cannot navigate. Content deep links are server-local, so they pin to the identity active when the link arrives; arriving signed-out pins nothing, which is what still lets a link opened before login work after it. Invite claims stay unpinned — they carry their own target server and are meant to work pre-auth. A pairing link naming a server other than the active one was refused silently: the user scanned a code and nothing happened. The route now carries its issuing origin and the screen says "This pairing request is for <server>" with a switch action, or offers to add an unknown one. Refusing to look a code up against the wrong server is preserved — the screen owns that check now. The request survives the sign-in the switch may require, on all three paths that can trigger one. Two production bugs found while doing it: snapshotCurrentScope was the one identity read that did not reconcile with the registry first, so straight after a registry-driven switch it described the previous server; and a device-shaped link whose origin could not be read parsed into a route with no origin, which downstream reads as "names no server" and pairs against whatever is active. Known limit, recorded in the code: the push payload carries no issuing server and the FCM token stays registered with previously-active servers, so a push from A arriving while B is active cannot be attributed at all and posts non-navigable. Fixing that needs issuer fields in the push protocol — server-side work. Seven review rounds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change adds atomic profile identity APIs, scope-guarded profile selection, lifecycle-safe external routes, server-aware device pairing, notification attribution checks, and duplicate-safe Android and TV navigation. ChangesIdentity and profile authentication
External routing and navigation
Sequence Diagram(s)sequenceDiagram
participant ExternalIntent
participant MainActivity
participant AppNavigation
participant TokenManager
ExternalIntent->>MainActivity: deliver route
MainActivity->>TokenManager: resolve current identity scope
TokenManager-->>MainActivity: return scope snapshot
MainActivity->>AppNavigation: queue scoped route
AppNavigation->>TokenManager: validate scope after navigation wait
AppNavigation-->>MainActivity: consume or requeue route
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codex reviewed this branch against its rebased base and returned NO-GO on four counts. All four are addressed. Unprotected profile selection was not scope-guarded. Both clients passed expectedScope = null for a profile with no PIN, which deliberately disables the guard — leaving the one path with no PIN round trip to re-establish identity committing unguarded. If another account became active between the grid being accepted and the commit entering the barrier, one account's profile id was written into the other's token slot. The scope the displayed grid was fetched under is now retained and passed for every selection, protected or not. An external tab link could retain a live player session indefinitely. tabSwitchNavOptions saves state so a tab keeps its stack, which is right for a tab — but a saved player entry keeps its ViewModelStore alive, so onCleared never runs and the session it owns is never stopped. The save is keyed to the LOWEST popped destination, so a later clearBackStack on the player route would not even find it. External tab routes now pop the player without saving first, so its ordinary teardown runs. This is the same class of leak the session-ownership work exists to prevent. External item links still used unconditional launchSingleTop, which is the exact defect this branch fixes for in-app navigation: AndroidX matches the destination node, not its arguments, so a notification for item B while item A's detail was showing reused A's entry and Back skipped A. Single-top now requires the arguments to agree. ExternalRouteScope did not carry identityGeneration, so signing out and back into the same account — or A to B back to A — was accepted. Ids alone cannot tell a new session from the old one. The generation is now captured, compared, and constrains only when it was known, the same rule the ids follow. Deliberately not credentialEpoch, which moves on ordinary token writes and would kill legitimate routes after a routine refresh. The PR description claimed this comparison already happened. It did not; that claim was wrong and is now true. Tests added for the generation rule and for argument-aware external item links. Full suite green on all four modules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
…igation # Conflicts: # androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt (1)
32-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose
noOpClientafter the tests run.
HttpClientowns an engine and a coroutine scope. This instance is never closed, so each test instance leaks one client.SiloAuthPluginPinTestin the same module closes its clients. Add an@AfterTestteardown.♻️ Proposed teardown
private val noOpClient = HttpClient(MockEngine { _ -> respond(content = "{}", status = HttpStatusCode.OK, headers = headersOf("Content-Type", "application/json")) }) + + `@AfterTest` + fun tearDown() { + noOpClient.close() + }Add the import:
import kotlin.test.Test +import kotlin.test.AfterTest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt` around lines 32 - 34, Add an `@AfterTest` teardown method to ProfileIdentityCommitTest that closes the noOpClient HttpClient after each test instance, importing the required teardown annotation and preserving the existing client setup.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt (2)
52-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
gridScopeso the@param clearErrorKDoc stays onloadProfiles.The KDoc at lines 52-56 documents
clearError. The newgridScopeproperty was inserted between that block andloadProfiles, so theclearErrordocumentation is now attached togridScope.gridScopehas noclearErrorparameter, and the tag will not resolve.♻️ Proposed reordering
+ /** + * The identity the displayed grid was fetched under. + * + * Every selection is qualified with it, protected or not. Passing null for + * unprotected picks disabled the guard for exactly the case with no PIN + * round trip to re-establish scope — so if another account became active + * between the grid being accepted and the commit entering the barrier, one + * account's profile id was written into the other's token slot. + */ + private var gridScope: AuthScopeSnapshot? = null + /** * `@param` clearError false keeps an existing error banner (e.g. a failed * delete's explanation) visible across the follow-up list refresh, which * would otherwise silently swallow it. */ - /** - * The identity the displayed grid was fetched under. - * - * Every selection is qualified with it, protected or not. Passing null for - * unprotected picks disabled the guard for exactly the case with no PIN - * round trip to re-establish scope — so if another account became active - * between the grid being accepted and the commit entering the barrier, one - * account's profile id was written into the other's token slot. - */ - private var gridScope: AuthScopeSnapshot? = null - fun loadProfiles(clearError: Boolean = true) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt` around lines 52 - 68, Move the gridScope property and its KDoc before the clearError KDoc, keeping the clearError documentation immediately above loadProfiles. Ensure gridScope retains its identity-scope documentation and loadProfiles remains the symbol documented by `@param` clearError.
271-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ScopeChangedempties the grid but keepsgridScopein both ViewModels. Both files state the invariant that a captured scope must not outlive the grid it was captured for, and bothloadProfilesimplementations honor it on the identity-mismatch path. TheScopeChangedhandlers do not. If the follow-uploadProfiles()fails, the empty grid keeps a scope from an identity the app no longer holds.
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt#L271-L289: setgridScope = nullbefore the_uiState.updateblock in theScopeChangedbranch ofselectProfile.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt#L207-L224: setgridScope = nullbefore the_uiState.updateblock in theScopeChangedbranch ofcommitSelection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt` around lines 271 - 289, Clear gridScope whenever ScopeChanged empties the profile grid: in androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt lines 271-289, set gridScope to null before the _uiState.update block in selectProfile; likewise in androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt lines 207-224, set gridScope to null before the _uiState.update block in commitSelection.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt (1)
253-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping
launchSingleTopon the lobby branch.
TvRoute.WatchTogetherLobby.ROUTEcarries a{roomId}argument, solaunchSingleTopmatches the destination node and not the room. If a lobby entry is ever on top when a second room is opened, Navigation reuses that entry and rewrites its arguments. That is the same node-versus-arguments mismatch this PR removes for item detail and playback.I could not reach this state from the supplied context:
TvWatchTogetherLobbyScreenexposes onlyonNavigateToPlayerandonBack, and the other entry points run while Main or ItemDetail is on top. Treat this as consistency hardening rather than a live defect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt` around lines 253 - 255, Remove launchSingleTop from the lobby navigation branch in the surrounding navigation logic, and invoke navigate(destination) normally for TvRoute.WatchTogetherLobby.ROUTE so each roomId creates a distinct lobby entry.androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt (1)
40-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth assertions match the current source; consider a behavioral test for the routing decision.
TvAppNavigation.ktcontainsnavigateToTvWatchTogether(snapshot, lastPlaybackNavigation)at line 905 andtvWatchTogetherDestination(room)at line 245, so these pass.The assertions bind to identifier names inside the production source. Renaming the lambda parameter
snapshot, or thenavigateToTvWatchTogetherparameterroom, fails the test without any behavior change.tvWatchTogetherDestinationis a pure function ofRoomSnapshot, so a direct assertion on its returned route for a lobby snapshot and a player snapshot would pin the same contract without depending on source text. This matches the pattern the newTvItemDetailNavigationTestuses.The rest of this class already uses source-text assertions, so this is a pre-existing pattern rather than something the PR introduces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt` around lines 40 - 45, Replace the identifier-based source-text assertions in TvWatchTogetherSurfaceSourceTest with behavioral coverage of the routing decision: directly verify that tvWatchTogetherDestination returns the expected route for both lobby and player RoomSnapshot inputs, following the approach used by TvItemDetailNavigationTest. Keep coverage focused on the destination contract rather than parameter or implementation names.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt (1)
66-68: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAdd explicit coverage for percent in TV route arguments.
Encoding
contentId,roomId, andtitlewithURLEncoderis appropriate for these route segments, but add a TV route test round-tripping a literal%as%25. The TvAppNavigation matchers already compare decoded arguments, and Navigation Compose 2.9.8 decodes route arguments, so there is no code change needed if that coverage is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt` around lines 66 - 68, Add a TV navigation route test covering a literal percent sign in the encoded contentId, roomId, and title arguments, asserting the route contains %25 and the TvAppNavigation matchers receive the original decoded values. Reuse the existing route-building and matcher test patterns; no production code changes are needed.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt (1)
42-64: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTwo builders produce the
pair_deviceroute with different encoders.Route.PairDeviceencodestoken,code, andserverOriginwithUri.encode, which leaves:and/unescaped and returns null under plain JVM unit tests.buildPairDeviceRouteencodes the same three arguments with aURLEncoder-based helper. Both routes reach the same destination and the samedeviceLoginServerMatchcomparison, so the two encodings must agree.
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt#L42-L64: replace the threeUri.encode(it)calls with the file's newit.routeEncode()helper.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt#L139-L153: keep therouteEncodeencoding, and reuseRoute.PairDevice(token, code, serverOrigin).routeonce both builders share one encoder, so the route exists in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt` around lines 42 - 64, The two pair-device route builders use inconsistent encoders. In androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt#L42-L64, replace the three Uri.encode calls in Route.PairDevice with the file’s routeEncode helper. In androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt#L139-L153, retain routeEncode and reuse Route.PairDevice(token, code, serverOrigin).route instead of constructing the route separately.androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt (1)
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the blank
profileIdcase.This test covers a blank
serverIdand a blankroute. It does not cover a blankprofileId. That guard is the third of the three checks that stop a wildcard identity scope, so pin it as well.💚 Proposed addition
assertNull( notificationExternalRouteOrNull(route = " ", serverId = "server-a", profileId = "kids"), ) + assertNull( + notificationExternalRouteOrNull(route = "item/abc", serverId = "server-a", profileId = " "), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt` around lines 55 - 63, Add a third assertion to the `blank is not an identity` test for `notificationExternalRouteOrNull`, using a valid route and serverId with a whitespace-only profileId, and assert that it returns null. Keep the existing blank serverId and route cases unchanged.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt (1)
118-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared route-segment decode into one helper.
isSameItemDetail(lines 118-125) andplayerRouteIntentOrNull(line 147) use the identical decode expression, including the+preservation step. Both values feed destination-identity comparisons. If one copy changes, the item path and the player path compare differently. Extract a single private helper and call it from both.♻️ Proposed refactor
+private fun String.decodeRouteSegmentOrNull(): String? = + runCatching { + URLDecoder.decode(replace("+", "%2B"), StandardCharsets.UTF_8.name()) + }.getOrNull()?.takeIf(String::isNotBlank)val targetContentId = targetRoute .substringAfter("item/") .substringBefore('?') .takeIf { it.isNotBlank() } // Same decode as the player intent: the route percent-encodes the id, // and an encoded id never equals the decoded one held by the entry. - ?.let { - runCatching { - URLDecoder.decode(it.replace("+", "%2B"), StandardCharsets.UTF_8.name()) - }.getOrNull() - } - ?.takeIf { it.isNotBlank() } + ?.decodeRouteSegmentOrNull() ?: return falseApply the same call at line 147 in
playerRouteIntentOrNull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt` around lines 118 - 124, Extract the shared percent-encoded route-segment decoding, including the plus-sign preservation and failure handling, into a single private helper in ExternalRouteNavigation. Replace the inline decode in isSameItemDetail and the identical expression in playerRouteIntentOrNull with calls to that helper.androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt (1)
414-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the null live generation case.
This test covers a pinned generation against a known live generation. It does not cover a pinned generation against a null live generation. That case occurs in AppNavigation.kt when
tokenManager.snapshotCurrentScope()returns null, andmatchesthen refuses the route. Add the assertion so the fail-closed direction stays pinned.💚 Proposed addition
assertFalse( scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = 8L), ) + // No live scope at all must not satisfy a generation-pinned route. + assertFalse( + scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = null), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt` around lines 414 - 428, The test aScopePinnedToAGenerationDoesNotMatchALaterSession should also assert that a pinned Identity scope does not match when the live identityGeneration is null, preserving the fail-closed behavior used by AppNavigation when snapshotCurrentScope returns null. Add the null-generation matches assertion alongside the existing generation checks.androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt (1)
72-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
kotlin.test.assertNotEqualsinstead of the local helper.The test uses
kotlin 2.1.20and already imports sibling assertions fromkotlin.test; addimport kotlin.test.assertNotEquals, remove the private helper, and keep the call at line 29 asassertNotEquals("item/abc?seasonNumber=9", route)forString?.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt` around lines 72 - 76, In ContentDeepLinkEncodingTest, import kotlin.test.assertNotEquals and remove the local private assertNotEquals helper. Keep the existing assertion call unchanged so it uses the Kotlin test assertion with the nullable String route value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt`:
- Around line 234-241: Update queueExternalRouteFrom so its
pendingExternalRouteRequests write occurs only when the processed Intent is
still the current activity Intent after currentIdentityScope() resumes. Ignore
stale coroutine results when a newer Intent has replaced it, preserving the
latest route in onNewIntent.
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt`:
- Around line 630-639: Update the start method’s requestCount publication so the
value is assigned atomically within the synchronized(pending) block, ensuring
overlapping calls cannot publish a stale lower count after a higher one. Keep
the existing Pending registration and awaitRequestCount behavior unchanged.
In
`@shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt`:
- Around line 271-289: Update setProfileIdentity so its mutex-protected block
first calls ensureCacheMatchesRegistryLocked(), before reading temporaryScope or
activeServerId; then retain the existing temporary-scope guard and profile-pair
persistence using the reconciled server context.
---
Nitpick comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt`:
- Around line 118-124: Extract the shared percent-encoded route-segment
decoding, including the plus-sign preservation and failure handling, into a
single private helper in ExternalRouteNavigation. Replace the inline decode in
isSameItemDetail and the identical expression in playerRouteIntentOrNull with
calls to that helper.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt`:
- Around line 42-64: The two pair-device route builders use inconsistent
encoders. In
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt#L42-L64,
replace the three Uri.encode calls in Route.PairDevice with the file’s
routeEncode helper. In
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt#L139-L153,
retain routeEncode and reuse Route.PairDevice(token, code, serverOrigin).route
instead of constructing the route separately.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt`:
- Around line 52-68: Move the gridScope property and its KDoc before the
clearError KDoc, keeping the clearError documentation immediately above
loadProfiles. Ensure gridScope retains its identity-scope documentation and
loadProfiles remains the symbol documented by `@param` clearError.
- Around line 271-289: Clear gridScope whenever ScopeChanged empties the profile
grid: in
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt
lines 271-289, set gridScope to null before the _uiState.update block in
selectProfile; likewise in
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt
lines 207-224, set gridScope to null before the _uiState.update block in
commitSelection.
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt`:
- Around line 72-76: In ContentDeepLinkEncodingTest, import
kotlin.test.assertNotEquals and remove the local private assertNotEquals helper.
Keep the existing assertion call unchanged so it uses the Kotlin test assertion
with the nullable String route value.
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt`:
- Around line 414-428: The test
aScopePinnedToAGenerationDoesNotMatchALaterSession should also assert that a
pinned Identity scope does not match when the live identityGeneration is null,
preserving the fail-closed behavior used by AppNavigation when
snapshotCurrentScope returns null. Add the null-generation matches assertion
alongside the existing generation checks.
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt`:
- Around line 55-63: Add a third assertion to the `blank is not an identity`
test for `notificationExternalRouteOrNull`, using a valid route and serverId
with a whitespace-only profileId, and assert that it returns null. Keep the
existing blank serverId and route cases unchanged.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt`:
- Around line 253-255: Remove launchSingleTop from the lobby navigation branch
in the surrounding navigation logic, and invoke navigate(destination) normally
for TvRoute.WatchTogetherLobby.ROUTE so each roomId creates a distinct lobby
entry.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt`:
- Around line 66-68: Add a TV navigation route test covering a literal percent
sign in the encoded contentId, roomId, and title arguments, asserting the route
contains %25 and the TvAppNavigation matchers receive the original decoded
values. Reuse the existing route-building and matcher test patterns; no
production code changes are needed.
In
`@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt`:
- Around line 40-45: Replace the identifier-based source-text assertions in
TvWatchTogetherSurfaceSourceTest with behavioral coverage of the routing
decision: directly verify that tvWatchTogetherDestination returns the expected
route for both lobby and player RoomSnapshot inputs, following the approach used
by TvItemDetailNavigationTest. Keep coverage focused on the destination contract
rather than parameter or implementation names.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt`:
- Around line 32-34: Add an `@AfterTest` teardown method to
ProfileIdentityCommitTest that closes the noOpClient HttpClient after each test
instance, importing the required teardown annotation and preserving the existing
client setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a43d0a32-5670-4777-a04c-99f2e890dbc5
📒 Files selected for processing (39)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/MediaAuthSession.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/network/WatchTogetherRealtimeWebSocketTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushNotificationPresenter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/BottomNavBar.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkRoutes.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/MainScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingWrongServerScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/PINEntryDialog.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.ktshared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.ktshared/src/androidUnitTest/kotlin/org/siloserver/silo/network/EncryptedTokenManagerScopeGenerationTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/PlaybackRealtimeClient.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.ktshared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginPinTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt
Comment on lines
234
to
+241
| override fun onNewIntent(intent: Intent) { | ||
| super.onNewIntent(intent) | ||
| // A genuinely new Intent has not been consumed, whatever the old one | ||
| // carried. | ||
| intent.removeExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED) | ||
| consumedExternalRoute = null | ||
| setIntent(intent) | ||
| val route = deviceLoginPairRouteOrNull(intent.dataString) | ||
| ?: inviteClaimRouteOrNull(intent.dataString) | ||
| ?: notificationRouteOrNull(intent) | ||
| ?: contentDeepLinkRouteOrNull(intent.dataString) | ||
| route?.let { pendingExternalRouteRequests.value = externalRouteRequestFactory.create(it) } | ||
| lifecycleScope.launch { queueExternalRouteFrom(intent) } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A second Intent during the suspension can restore an older route.
queueExternalRouteFrom suspends at currentIdentityScope(). If a second Intent arrives while the first coroutine is suspended, the second coroutine can finish first. The first one then resumes and overwrites pendingExternalRouteRequests with the older route. The user lands on the previous link instead of the one just opened.
Bind the write to the Intent that is still current.
🐛 Proposed fix: drop a result whose Intent is no longer current
- private suspend fun queueExternalRouteFrom(intent: Intent?) {
+ private suspend fun queueExternalRouteFrom(intent: Intent?) {
if (intent?.getBooleanExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED, false) == true) return+ // A newer Intent may have arrived while currentIdentityScope() was
+ // suspended; its coroutine may already have published. Do not clobber
+ // it with this older route.
+ if (intent !== this.intent) return
pendingExternalRouteRequests.value =
externalRouteRequestFactory.create(route = route, scope = scope)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt`
around lines 234 - 241, Update queueExternalRouteFrom so its
pendingExternalRouteRequests write occurs only when the processed Intent is
still the current activity Intent after currentIdentityScope() resumes. Ignore
stale coroutine results when a newer Intent has replaced it, preserving the
latest route in onNewIntent.
Comment on lines
+630
to
639
| /** Replayable so a request that lands before the wait begins is still seen. */ | ||
| private val requestCount = MutableStateFlow(0) | ||
|
|
||
| override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult = | ||
| suspendCoroutine { continuation -> | ||
| synchronized(pending) { | ||
| requestCount.value = synchronized(pending) { | ||
| pending += Pending(request, continuation) | ||
| pending.size | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Publish requestCount inside the lock.
The assignment to requestCount.value runs after synchronized(pending) releases. If two start calls ever overlap, the caller that computed size 1 can assign after the caller that computed size 2, so requestCount regresses from 2 to 1. awaitRequestCount reads the current value through first { it >= count }, so a waiter that begins after the regression blocks until a further request arrives, and the 30-second deadline turns that into a confusing timeout.
The current tests drive loadContent sequentially, so overlapping start calls are not reachable today. The fix is one line and removes the hazard for future tests. stoppedSignal at line 682 already uses update and does not have this problem.
🔒️ Proposed fix to publish the count atomically
override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult =
suspendCoroutine { continuation ->
- requestCount.value = synchronized(pending) {
+ synchronized(pending) {
pending += Pending(request, continuation)
- pending.size
+ requestCount.value = pending.size
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Replayable so a request that lands before the wait begins is still seen. */ | |
| private val requestCount = MutableStateFlow(0) | |
| override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult = | |
| suspendCoroutine { continuation -> | |
| synchronized(pending) { | |
| requestCount.value = synchronized(pending) { | |
| pending += Pending(request, continuation) | |
| pending.size | |
| } | |
| } | |
| /** Replayable so a request that lands before the wait begins is still seen. */ | |
| private val requestCount = MutableStateFlow(0) | |
| override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult = | |
| suspendCoroutine { continuation -> | |
| synchronized(pending) { | |
| pending += Pending(request, continuation) | |
| requestCount.value = pending.size | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt`
around lines 630 - 639, Update the start method’s requestCount publication so
the value is assigned atomically within the synchronized(pending) block,
ensuring overlapping calls cannot publish a stale lower count after a higher
one. Keep the existing Pending registration and awaitRequestCount behavior
unchanged.
Comment on lines
+271
to
+289
| override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { | ||
| mutex.withLock { | ||
| // A temporary overlay owns its own identity for the lifetime of a | ||
| // remote-playback handoff. Merging a profile commit into it is how | ||
| // you get the exact defect this method exists to prevent: writing | ||
| // the new profile id beside the overlay's old token. Leave it | ||
| // alone; the repository rejects the commit outright. | ||
| if (temporaryScope != null) return@withLock | ||
| val serverId = activeServerId ?: return | ||
| if (this.profileId == profileId && this.profileToken == profileToken) return | ||
| this.profileId = profileId | ||
| this.profileToken = profileToken | ||
| val idKey = serverScopedKey(serverId, KEY_PROFILE_ID) | ||
| val tokenKey = serverScopedKey(serverId, KEY_PROFILE_TOKEN) | ||
| prefs.edit().apply { | ||
| if (profileId == null) remove(idKey) else putString(idKey, profileId) | ||
| if (profileToken == null) remove(tokenKey) else putString(tokenKey, profileToken) | ||
| }.apply() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reconcile the active-server cache before writing the profile pair.
After registry.switchTo(B), this method can run before the registry collector updates cached activeServerId. It then writes B's profile identity to A's preference keys and cache. Call ensureCacheMatchesRegistryLocked() before reading temporaryScope or activeServerId.
Proposed fix
override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) {
mutex.withLock {
+ ensureCacheMatchesRegistryLocked()
// A temporary overlay owns its own identity for the lifetime of a🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt`
around lines 271 - 289, Update setProfileIdentity so its mutex-protected block
first calls ensureCacheMatchesRegistryLocked(), before reading temporaryScope or
activeServerId; then retain the existing temporary-scope guard and profile-pair
persistence using the reconciled server context.
RXWatcher
added a commit
to RXWatcher/silo-android
that referenced
this pull request
Aug 6, 2026
Codex reviewed this branch twice and fixed both remaining findings. The return path did not work on this branch at all. Detail-to-related navigation used launchSingleTop, which AndroidX matches on the destination NODE rather than its contentId, so A -> related B reused A's entry and left no A for Back to reveal — the restoration had nothing to restore to. The comment there asserted the opposite. Related details now push, which is the minimum this branch needs; the broader exact-repeat helper stays on Silo-Server#175 rather than being duplicated here. The keyed savers are typed rather than erased. The generic version cast through a type parameter, so `values[1] as T` validated against Any only: a payload with the right owner and the wrong type passed restore and failed later where Compose reads it, crashing during restoration instead of being rejected. Boolean and Int savers now validate surface, slot, length and type, and the return-target saver rejects malformed payloads rather than throwing. Each carries a slot name as well as the surface token, because the owner alone cannot tell one scalar slot from another. The timeout and revocation policy is extracted so it can be tested directly, with coverage for the attachment-timeout fallback and mid-loop revocation — the paths that had none. Full suite green on all four modules with --rerun-tasks, verified here rather than taken on trust: Codex's own sandbox could not run Gradle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Codex reviewed this branch twice and fixed the remaining findings itself. Three of the four problems on the second pass were introduced by my own remediation, and a fourth turned up here. The grid scope now moves only with a grid the app actually accepted. Assigning it before the result meant a reload that FAILED under a new identity left the old grid on screen qualified by the new scope, so a profile from the old session committed as belonging to the new one — worse than the unguarded commit it replaced. Nulling it on a scope mismatch then opened a second hole: a tap dispatched after the grid cleared committed unguarded precisely because the scope was null. Both are closed, and a cleared grid can no longer select at all. External-route single-top is an explicit per-destination policy rather than punctuation matching. My version keyed on whether the route contained '?' or '/', which happened to work for the cases I had in mind and not for others; the policy now names each destination an external request can produce — inbox, item, player, downloads, pair_device, invite_claim — so a second invitation gets its own entry instead of collapsing onto the first. The player pop stays conditional on the player being the current destination. Popping a player that sits below newer history would take that history with it, which is a worse trade than leaving the rarer case saved. Notifications still carry no identity generation, deliberately: that counter restarts at zero every process, so persisting it into a PendingIntent would refuse a legitimate notification tapped after the app was killed. Closing that needs a durable epoch, not a process-local count. The KDoc says so rather than implying coverage it does not have. Tests added for every path above, including the ones that had none: a reload that fails under a new scope, a tap after a cleared grid, the player pop with the player absent, on top, and below other entries, the full single-top decision including invite_claim, and notification extras through to the delivered scope. Full suite green on all four modules, verified here across repeated --rerun-tasks runs rather than taken on trust: Codex's sandbox could not run Gradle and correctly declined to claim it had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
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.
Four fixes across profile identity and navigation. The common thread is state that was written in two steps, or read from a place that could not answer.
Profile identity is committed as one thing
selectProfilewrote the profile id and its token separately. A failure or a concurrent switch between the two left the app holding one profile's id with another's token — an identity that never existed. Both now commit inside the identity transition, under a scope guard that refuses the write if the surrounding scope moved while it was in flight.The guard needed care:
IdentityTransitionBarrier.changing()bumps the generation before running its block, so a naive comparison rejects every protected-profile login. It compares against the generation the transition itself established.verifyPinis now a pure query. It was mutating token state as a side effect of asking a question, which meant a failed PIN entry could leave scope changed.launchSingleTopwas silently replacing entriesAndroidX matches
launchSingleTopon the destination node, not its arguments — so navigating from one item's detail to another's reused the same entry, and the back stack quietly lost the first. The same applied to the player. Both now compare arguments before deciding it is really the same destination.Related AndroidX behaviour this also accounts for:
restoreStateis evaluated beforelaunchSingleTop;saveStateon an inclusive pop keys the saved stack to the lowest popped destination; and an existing saved mapping is retained rather than replaced.Deep links, tab roots and identity reads
Three gaps closed together: a deep link could land on a tab root that had not been created, a tab anchor was read from a stale source rather than the live back stack, and an identity read could run before the identity it described was committed. The vanished-tab case is unified through
clearBackStack.External routes are attributed to the identity that created them
A notification or content link created under one identity could be opened under another — different server, different profile — and silently act on the wrong account's data. Routes now carry an
ExternalRouteScope, and a scoped route only opens under a matching identity.Attribution compares
serverIdandidentityGeneration, deliberately notcredentialEpoch, which moves on ordinary token writes and would have invalidated legitimate routes.Testing
Full suite green on all four modules. Reviewed with Codex across multiple passes; the scope-guard generation bug above was caught that way, and the test double that hid it was rewritten against the real barrier.
TV detail focus restoration is deliberately not in this PR — it is disjoint from these files and goes separately.
🤖 Generated with Claude Code
https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Summary by CodeRabbit