feat(onboarding): add invite claim and server-driven feature tour - #125
Conversation
playback.audio_language and the profile's subtitle_language are BCP 47
language tags in the server's settings contract. The phone put the display
label on the wire verbatim — "English", not "en" — and the TV did the same
for audio while doing it correctly for subtitles.
That was already broken before the server started enforcing it: the same
string is handed to ExoPlayer as preferredAudioLanguage, and
setPreferredAudioLanguage("English") never matches a track tagged eng, so
choosing an audio language on Android has silently been a no-op. It also
meant Android and Apple wrote different vocabularies to the same key —
Apple has always sent codes, so a language picked on an iPhone read as
"Default" on the phone and vice versa.
Now that the server validates the tag, the flusher's PUT 400s and only
logs, so the setting would stop persisting entirely after a server upgrade.
Replaces the four drifted option lists with one table in shared, so a
language cannot be added to one surface and missed on the others, and
translates values already on devices on read rather than re-sending a
label the server will reject.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Companion to silo-server#501. - silo://invite?server=…&token=… deep link opens InviteClaimScreen with the server and single-use token already bound — no "which server?" step. The invitee sets only a password; their email address is their username. On success the server is registered and tokens persist, identical post-conditions to a manual login. - OnboardingTourScreen renders the server's /onboarding/flow manifest (surface=phone): one step per page, skip always reachable, unknown step kinds dropped at load (the forward-compat contract). Progress and completion post per profile, so finishing here silences web/TV. setting_choice steps write through the existing profile-update path. - Profile selection now routes Home entry through the tour gate, which immediately hands off to Home when state is already done. TV (androidTvApp) intentionally not covered here — the manifest contract already carries surface=tv for a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (2)
📝 WalkthroughWalkthroughThis PR adds emailed-invitation claiming, server-driven onboarding tours with persisted progress, shared BCP 47 language handling across Android and TV, and configurable Aurora layout behavior. ChangesInvitation claim flow
Server-driven onboarding tour
Shared language settings
Aurora layout configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)Invitation claimsequenceDiagram
participant User
participant MainActivity
participant InviteClaimScreen
participant AuthRepository
participant AuthApi
User->>MainActivity: Open silo://invite link
MainActivity->>InviteClaimScreen: Navigate with server and token
InviteClaimScreen->>AuthRepository: Lookup invitation
AuthRepository->>AuthApi: GET invitation
AuthApi-->>InviteClaimScreen: Invitation details
User->>InviteClaimScreen: Submit password
InviteClaimScreen->>AuthRepository: Accept invitation
AuthRepository->>AuthApi: POST acceptance
AuthRepository-->>InviteClaimScreen: Successful login response
Onboarding toursequenceDiagram
participant MainActivity
participant OnboardingTourScreen
participant OnboardingTourViewModel
participant OnboardingRepository
participant OnboardingApi
MainActivity->>OnboardingTourScreen: Navigate when tour is incomplete
OnboardingTourScreen->>OnboardingTourViewModel: load()
OnboardingTourViewModel->>OnboardingRepository: Fetch state and flow
OnboardingRepository->>OnboardingApi: GET onboarding endpoints
OnboardingApi-->>OnboardingTourViewModel: Flow and progress
OnboardingTourScreen->>OnboardingTourViewModel: Advance, skip, or finish
OnboardingTourViewModel->>OnboardingRepository: Post progress
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- Route warm-start invite deep links and keep pre-auth claim navigation - Distinguish unreachable server from dead invite, with retry - Cache tour completion per server+profile and gate warm starts on it - Persist tour setting choices on advance; survive navigation with NonCancellable - Migrate legacy language display labels to wire codes on phone and TV
![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: 56f385b1ff
ℹ️ 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".
| client.post("${serverUrl.trimEnd('/')}/api/v1/invitations/$token/accept") { | ||
| skipSiloAuth() | ||
| contentType(ContentType.Application.Json) | ||
| setBody(AcceptInvitationRequest(password = password)) |
There was a problem hiding this comment.
When an invite targets an unapproved http:// origin, the lookup GET succeeds because read-only skipSiloAuth requests may bypass consent, but this credential-bearing POST is rejected by AuthInterceptorImpl with CleartextOriginNotApprovedException. The exception becomes a generic NetworkError, and the invite screen provides no consent prompt, so first-time invitees on HTTP/LAN servers cannot claim their accounts unless they previously approved the origin through an unrelated setup flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. InviteClaimViewModel checks cleartextConsentStore.requiresApproval(serverUrl) before the claim POST and surfaces pendingCleartextOrigin, so the screen collects the same consent server setup does, then retries.
| if (result is ApiResult.Success) { | ||
| setServerUrl(serverUrl) | ||
| } | ||
| return persistSession(result) |
There was a problem hiding this comment.
When the invite targets a server that is already registered with a signed-in account, setServerUrl() reloads that server slot's existing profile ID and profile token, while persistSession() overwrites only the access and refresh tokens. The newly claimed account therefore reaches profile selection with headers for the previous account's profile still attached by AuthInterceptorImpl, which can make subsequent profile requests fail or operate with mismatched identity state; clear the target slot's profile state before saving the new session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. acceptInvitation clears the target slot's profile id and profile token (and the registry's) after a successful claim, before persistSession, so the new account doesn't inherit the previous one's profile headers.
| if (persistCurrentChoice) { | ||
| persistChoiceIfAny(current.steps.getOrNull(current.currentIndex)) | ||
| } | ||
| markDoneLocally() |
There was a problem hiding this comment.
If connectivity drops when the user taps Done or Skip, this marks the profile permanently done locally before posting progress, and the POST result is ignored. Future launches then short-circuit without consulting the server, so the completion is never retried and other clients continue showing the tour despite this client claiming that completion is synchronized per profile; only mark the cache after a successful response or retain a retryable pending completion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. finish() posts inside withContext(NonCancellable) and only calls localCache.markDone when the result is ApiResult.Success, so a lost POST leaves the cache unset and the next launch re-consults the server.
| val flowDeferred = async { onboardingRepository.getFlow(surface = "phone") } | ||
| when (val state = stateDeferred.await()) { | ||
| is ApiResult.Success -> { | ||
| if (state.data.done) { |
There was a problem hiding this comment.
When a profile has unfinished progress from another device or a process death, the successful state response's lastStep is ignored and applyFlow() always initializes currentIndex to zero. Although every advance posts the newly displayed step, reopening the tour restarts it from the beginning instead of restoring that progress; carry the returned state into flow application and select the matching step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. The state response's lastStep is carried into applyFlow as resumeStep and resolved to startIndex, so the tour resumes at the recorded step. Verified on device: the tour opened mid-flow rather than at step one.
| is ApiResult.Error -> _uiState.update { | ||
| it.copy(isLoadingInvitation = false, invitationInvalid = true) |
There was a problem hiding this comment.
Any HTTP error is treated as proof that the invitation is expired, including transient server failures such as 429, 500, or 503. In those cases the screen shows the permanent-invalid card with no retry action even though the token was never rejected; only terminal invitation statuses should set invitationInvalid, while retryable HTTP responses should use the same retry state as network failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. Only TERMINAL_LOOKUP_CODES (400/404/409/410) set invitationInvalid; other statuses set lookupFailed, which offers retry via onRetryLookup.
| "quality_preference" -> UpdateProfileRequest(qualityPreference = value) | ||
| "subtitle_language" -> UpdateProfileRequest(subtitleLanguage = value) | ||
| "subtitle_mode" -> UpdateProfileRequest(subtitleMode = value) | ||
| "auto_skip_intro" -> UpdateProfileRequest(autoSkipIntro = value.toBoolean()) |
There was a problem hiding this comment.
For an auto_skip_intro tour step, this updates only the profile field, while Android playback reads PlayerSettingsStore.autoSkipIntroFlow and never consults that profile value. Unlike the special-cased quality choice, the selected intro-skip behavior therefore remains unchanged for playback and in the Settings screen during the current session; write it through playerSettingsStore.setAutoSkipIntro() as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. persistChoiceIfAny mirrors auto_skip_intro and auto_skip_credits into playerSettingsStore alongside quality_preference, since Android playback reads the local store rather than the profile field.
| if (spec.key == "quality_preference") { | ||
| playerSettingsStore.setPreferredQuality(value) | ||
| } | ||
| profileRepository.updateActiveProfile(request) |
There was a problem hiding this comment.
The result of the profile update is discarded, so a validation error or transient network failure silently loses the user's setting even though the tour has already advanced and may subsequently be marked complete. This is especially unrecoverable for the final setting step because navigation immediately removes the tour; check the result and retry, retain pending work, or keep the user on the step instead of reporting successful completion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly addressed in bb6b902: the choice is mirrored into playerSettingsStore first, so the setting the tour showed does take effect on this device even when the profile PUT fails. Keeping the user on the step for a failed best-effort PUT would trap them in a tour they cannot leave, which is the worse failure here; Settings re-syncs from the server later.
| viewModelScope.launch { | ||
| when (val result = authRepository.lookupInvitation(serverUrl, token)) { |
There was a problem hiding this comment.
If a second invite deep link replaces the current one while its first lookup is still in flight, load() launches another request without cancelling or generation-checking the previous job. The older response can then arrive last and overwrite the UI with the first invite's email and server name while the stored serverUrl and token point at the second invite, causing the user to submit a password to an account other than the one displayed; cancel the prior lookup or ignore results whose inputs are no longer current.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. load() bumps a lookupGeneration and the in-flight lookup returns early when it no longer matches, so a superseded invite's response can't paint over the current one.
| if (state.isLoading || state.steps.isEmpty()) { | ||
| Box(Modifier.fillMaxSize(), contentAlignment = Alignment.Center) { | ||
| CircularProgressIndicator(color = Color(0xFFF3EFE9)) | ||
| } |
There was a problem hiding this comment.
While either onboarding request is slow, this branch renders only a spinner and provides no Skip action even though the shared HTTP client permits requests to remain pending for up to 60 seconds. Because profile selection clears the previous back stack before opening this gate, a stalled server can block access to Home for the full timeout; render the skip control during loading or apply a much shorter fail-open timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. The loading branch renders Skip above the spinner, and finish() handles a blank tourId by letting the user through with server state left not-done.
| fun label(wire: String?, unsetLabel: String): String = | ||
| TAGS.firstOrNull { it.first == wire }?.second ?: unsetLabel |
There was a problem hiding this comment.
Valid tags outside the ten picker entries are deliberately preserved by migrateLegacyValue()—for example nl, hi, or pt-BR synced from another client—but this formatter labels every such active value as Default for audio or Off for subtitles. Playback can therefore continue applying a preference that both phone and TV settings claim is disabled; expose the stored tag with a fallback label or include it as a selected option rather than mapping it to the unset state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in bb6b902. LanguageOptions.label echoes a preserved out-of-picker tag back as itself and only falls back to the unset label for values that aren't tags at all.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
shared/src/commonMain/kotlin/org/siloserver/silo/network/api/OnboardingApi.kt (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer Ktor's
parameter()over string interpolation for the query string.
surfaceis safe today since callers only pass the literal"phone"/"tv", but building the query string via raw interpolation bypasses Ktor's automatic encoding and is easy to get wrong if this ever takes a dynamic value.♻️ Proposed fix
- suspend fun getFlow(surface: String): ApiResult<OnboardingFlow> = safeApiCall { - client.get("/api/v1/onboarding/flow?surface=$surface") - } + suspend fun getFlow(surface: String): ApiResult<OnboardingFlow> = safeApiCall { + client.get("/api/v1/onboarding/flow") { + url { parameter("surface", surface) } + } + }🤖 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/commonMain/kotlin/org/siloserver/silo/network/api/OnboardingApi.kt` around lines 22 - 24, Update the getFlow method to pass surface through Ktor’s parameter() query API instead of interpolating it into the request URL. Keep the existing endpoint path and safeApiCall behavior unchanged while ensuring the query value is encoded by Ktor.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourViewModel.kt (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused unit tests for
OnboardingTourViewModel. Cover step filtering, default seeding, and complete/skip persistence so this first-run flow doesn’t regress across devices.🤖 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/onboarding/OnboardingTourViewModel.kt` around lines 45 - 51, Add focused unit tests for OnboardingTourViewModel covering step filtering, default step seeding, and persistence when completing or skipping the tour. Mock or fake OnboardingRepository, ProfileRepository, PlayerSettingsStore, TokenManager, and OnboardingTourLocalCache as needed, and verify the expected state and saved values across first-run and repeat-device scenarios.Source: Path instructions
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/InviteClaimRouteParser.kt (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate route-encoding logic vs.
Route.InviteClaim.This hand-rolls the same
"invite_claim?server=...&token=..."string thatRoute.InviteClaim(server, token).route(Routes.kt) already builds viaUri.encode. Two independent encoders for one logical route can silently drift if the route format changes.♻️ Proposed refactor: reuse `Route.InviteClaim`
- val server = params["server"]?.takeIf { it.isNotBlank() } ?: return null - val token = params["token"]?.takeIf { it.isNotBlank() } ?: return null - - return "invite_claim?server=${server.routeEncode()}&token=${token.routeEncode()}" + val server = params["server"]?.takeIf { it.isNotBlank() } ?: return null + val token = params["token"]?.takeIf { it.isNotBlank() } ?: return null + + return Route.InviteClaim(server, token).route } - -private fun String.routeEncode(): String = - URLEncoder.encode(this, Charsets.UTF_8.name()).replace("+", "%20")🤖 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/InviteClaimRouteParser.kt` around lines 41 - 45, Update the invite-claim route construction in InviteClaimRouteParser to reuse Route.InviteClaim(server, token).route instead of manually assembling the query string and calling routeEncode. Remove the redundant routeEncode helper if it is no longer used, preserving the existing parsed server and token values.
🤖 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/ui/screens/onboarding/OnboardingTourViewModel.kt`:
- Around line 102-112: The completion requests in finish() and applyFlow() mark
the tour locally complete without checking the onboardingRepository response.
Inspect the returned ApiResult from complete/skip, only call markDoneLocally()
after confirmed success, and handle or log failures so unsuccessful server
updates remain observable and retryable rather than permanently bypassing future
server checks.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt`:
- Around line 53-54: Update LanguageOptions.label to distinguish blank or
malformed values from nonblank tags absent in TAGS, displaying preserved unknown
tags or an “Other” representation instead of unsetLabel; retain existing labels
for picker entries. Update LanguageOptionsTest unknown-tag expectations and add
coverage for a preserved out-of-picker tag in
shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt
lines 40-46.
- Around line 29-40: Rename the non-constant LanguageOptions.TAGS property to
tags and update all three references in
shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt
at lines 15-16, 25-28, and 33-36. Also rename AudioLanguages and
SubtitleLanguages to camelCase in
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt
at lines 2034-2036, updating their usages consistently.
In `@shared/src/commonMain/kotlin/org/siloserver/silo/network/api/AuthApi.kt`:
- Around line 71-95: Update lookupInvitation and acceptInvitation to append the
invitation token as an encoded URL path segment instead of interpolating it
directly into the request path. Preserve the existing endpoints, authentication
settings, request body, and response handling while ensuring reserved token
characters cannot alter the path or query.
---
Nitpick comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/InviteClaimRouteParser.kt`:
- Around line 41-45: Update the invite-claim route construction in
InviteClaimRouteParser to reuse Route.InviteClaim(server, token).route instead
of manually assembling the query string and calling routeEncode. Remove the
redundant routeEncode helper if it is no longer used, preserving the existing
parsed server and token values.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourViewModel.kt`:
- Around line 45-51: Add focused unit tests for OnboardingTourViewModel covering
step filtering, default step seeding, and persistence when completing or
skipping the tour. Mock or fake OnboardingRepository, ProfileRepository,
PlayerSettingsStore, TokenManager, and OnboardingTourLocalCache as needed, and
verify the expected state and saved values across first-run and repeat-device
scenarios.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/network/api/OnboardingApi.kt`:
- Around line 22-24: Update the getFlow method to pass surface through Ktor’s
parameter() query API instead of interpolating it into the request URL. Keep the
existing endpoint path and safeApiCall behavior unchanged while ensuring the
query value is encoded by Ktor.
🪄 Autofix (Beta)
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: 3d06fc4f-483b-4b83-aaed-f9b75a789ab6
📒 Files selected for processing (27)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/settings/AndroidPlayerSettingsStore.ktandroidApp/src/androidMain/AndroidManifest.xmlandroidApp/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/ui/navigation/AppNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/InviteClaimRouteParser.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/InviteClaimScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/InviteClaimViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourLocalCache.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/PlaybackSettings.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SubtitleSettings.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.ktshared/src/commonMain/kotlin/org/siloserver/silo/di/NetworkModule.ktshared/src/commonMain/kotlin/org/siloserver/silo/di/RepositoryModule.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/auth/InvitationModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/onboarding/OnboardingModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/api/AuthApi.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/api/OnboardingApi.ktshared/src/commonMain/kotlin/org/siloserver/silo/repository/AuthRepository.ktshared/src/commonMain/kotlin/org/siloserver/silo/repository/OnboardingRepository.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt
- Ask for cleartext consent before the credential-bearing claim POST (read-only lookup may pass the gate; the POST would be rejected as an opaque network error on unapproved http:// origins) - Clear the previous account's profile id/token when a claim adopts an already-registered server slot - Cache tour completion locally only after the server acks the POST, so a lost completion is retried instead of silently diverging - Resume the tour from the server-reported last step - Treat only 400/404/409/410 lookup responses as terminal; 429/5xx get the retry card, and stale lookups are generation-checked so a slow response can't paint a superseded invite - Keep Skip reachable while the tour manifest loads (no tour id yet = finish locally without posting) - Mirror auto-skip tour choices into the player settings store the app actually reads, like quality - Swallow malformed percent-encoding in warm deep-link parsers instead of throwing from onNewIntent - Show preserved out-of-table language tags as themselves rather than claiming the preference is off Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/InviteClaimViewModel.kt (1)
115-147: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGate duplicate claim attempts before launching.
Both the IME action and “Create account” call
onClaimClick, butisSubmittingis set only after the coroutine begins and the cleartext check completes. Rapid duplicate actions can therefore issue multiple one-time claim POSTs. Add a synchronous attempt-in-flight guard and clear it on cancellation or every terminal outcome.🤖 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/auth/InviteClaimViewModel.kt` around lines 115 - 147, Add a synchronous attempt-in-flight guard to onClaimClick before launching its coroutine, returning immediately for duplicate actions and setting the guard before launch. Clear the guard on coroutine cancellation and every terminal outcome, including validation failure, cleartext-consent suspension, successful submission, and claim errors; ensure onConfirmCleartext also participates in the same guard so only one claim POST can be active.
🤖 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/ui/screens/auth/InviteClaimViewModel.kt`:
- Around line 62-66: Update the early-return identity check in load to require
both the existing serverUrl and token to match the incoming values, while
retaining the non-null invitation condition. Continue updating serverUrl and
token and performing the lookup when either value differs.
---
Outside diff comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/InviteClaimViewModel.kt`:
- Around line 115-147: Add a synchronous attempt-in-flight guard to onClaimClick
before launching its coroutine, returning immediately for duplicate actions and
setting the guard before launch. Clear the guard on coroutine cancellation and
every terminal outcome, including validation failure, cleartext-consent
suspension, successful submission, and claim errors; ensure onConfirmCleartext
also participates in the same guard so only one claim POST can be active.
🪄 Autofix (Beta)
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: 4a83ba86-8a75-4171-9093-1f908e453584
📒 Files selected for processing (10)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/InviteClaimRouteParser.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/InviteClaimScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/InviteClaimViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourViewModel.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.ktshared/src/commonMain/kotlin/org/siloserver/silo/repository/AuthRepository.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt
🚧 Files skipped from review as they are similar to previous changes (8)
- shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/InviteClaimScreen.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourScreen.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/InviteClaimRouteParser.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/repository/AuthRepository.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/onboarding/OnboardingTourViewModel.kt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb6b9025ac
ℹ️ 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".
|
|
||
| private fun key(serverId: String?, profileId: String?): String? { | ||
| if (serverId.isNullOrBlank() || profileId.isNullOrBlank()) return null | ||
| return "done:$serverId:$profileId" |
There was a problem hiding this comment.
When the server publishes a new tour_id or resets this profile's onboarding state, this permanent server/profile-only key still returns true, so both cold startup and load() skip the server state and flow endpoints forever. Completed users therefore never see later server-driven tours; store the completed tour/version or periodically revalidate the server rather than treating one completion as permanent for the profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, but not folded into this PR — the fix is bigger than it looks. The stored value is a Boolean on installs already in the field, so re-reading that key as a tour id throws ClassCastException without a migration, and the cold-start gate in MainActivity.resolveStartDestination checks the cache before any tour id is known, so it needs a separate answer. Today a new tour id is not permanently suppressed for most users: the cache is only written on a confirmed complete/skip, and the gate still consults the server whenever it is unset. Worth doing deliberately with the migration rather than as a drive-by here.
| when (val result = authRepository.acceptInvitation(serverUrl, token, current.password)) { | ||
| is ApiResult.Success -> { | ||
| _uiState.update { it.copy(isSubmitting = false, claimSuccess = true) } |
There was a problem hiding this comment.
When a second warm invite replaces the route while the first claim POST is still in flight, load() invalidates only lookup responses; this submission still updates the shared UI and active server when it returns. The first response can consequently navigate away from the second invite or race a second submission and leave credentials for the wrong server active, so capture the lookup generation/target when submitting and discard results after it changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 69eb935. submitClaim captures lookupGeneration (plus the server/token it is claiming) and discards its result if the target changed while the POST was in flight.
| ): ApiResult<User> { | ||
| val result = authApi.acceptInvitation(serverUrl, token, password) | ||
| if (result is ApiResult.Success) { | ||
| setServerUrl(serverUrl) |
There was a problem hiding this comment.
After the single-use accept POST succeeds, setServerUrl() synchronously runs the best-effort health/name refresh before persistSession() saves the returned credentials. If that health request stalls for its six-second timeout and the invite screen is popped or the process is killed, the coroutine is cancelled with the invitation already consumed but no new tokens stored, leaving the user on the switched server with an unusable invite; switch scopes and persist the session before performing the optional metadata refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changed. setServerUrl must run before persistSession — persisting writes into the active server's slot, so saving first would put the claimed account's tokens on the previous server. The window is real but narrow, and reordering as suggested trades a rare lost-token case for a wrong-slot one. If the health refresh is the concern, the fix is to make that refresh non-blocking inside setServerUrl, which affects every caller and belongs in its own change.
| if (result is ApiResult.Success) { | ||
| localCache.markDone(tokenManager.getCurrentServerId(), tokenManager.getProfileId()) | ||
| } |
There was a problem hiding this comment.
When the completion or skip POST remains in flight after navigation to Home and the user switches profile or server before it returns, these token-manager lookups resolve the new active identity rather than the profile whose request just succeeded. The new profile can then be marked locally done and bypass its own tour, while the original profile is not cached; capture the server and profile IDs before launching the non-cancellable request and use that snapshot on acknowledgement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 69eb935. Both finish() and applyFlow's empty-steps branch snapshot the server and profile ids before the non-cancellable POST and mark that snapshot on acknowledgement, rather than re-reading the token manager after Home may have switched identity.
The tour rendered its chrome — pips, Skip, Back/Next — over an empty screen. AuroraScreen wraps content in a verticalScroll, which measures children with unbounded height, so the tour's weight(1f) body had no space to divide and collapsed to zero. Verified on an S26 Ultra: the region between the pips and the buttons measured 0px tall. AuroraScreen takes a `scrollable` flag for screens that lay themselves out against the display instead of scrolling as one block, and applies safeDrawing insets so a full-height screen's first and last rows clear the system bars under edge-to-edge. The steps are now a HorizontalPager, so they can be swiped as cards as well as driven by the buttons. Both routes move through one path in the ViewModel, so a step reached by swiping records progress and commits its setting_choice exactly like a tapped one; advancing commits, going back does not. The pager runs full-bleed so the next card peeks in from the edge rather than being clipped at the gutter, and neighbours scale and dim to read as depth. auroraGlass takes an `elevation`, defaulting to the previous 60dp. The tour cards pass 0: their fill is translucent, so at full height the drop shadow's own outline showed through the glass as a faint hard-edged box that tracked the card across a swipe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Percent-encode the invitation token into the lookup and accept paths. It arrives from an emailed link, so a '/' or '?' in it would re-shape the request rather than be carried as a path segment. Send the onboarding flow's `surface` through Ktor's parameter() instead of interpolating it into the query string, so encoding is the client's job rather than the caller's. Rename LanguageOptions.TAGS to `tags`, and the TV settings screen's AudioLanguages/SubtitleLanguages to camelCase, per the repo's convention for non-constant properties. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tour's completion POST outlives the screen: finished=true navigates to Home, where the user can switch profile or server before it returns. Reading the token manager on acknowledgement therefore cached "done" against whichever profile was active by then, letting it skip a tour it had never seen while the profile that actually finished stayed unmarked. Snapshot the server and profile ids before the request and mark those. An invite is identified by server and token together. The claim screen's early-return compared only the token, so a second link carrying the same token on a different server kept the first server and submitted the password there. It also let a superseded claim POST drive the UI on return; the submission now pins the lookup generation and discards its result once the target has moved on. Left alone: scoping the local done-cache to a tour id. Its stored value is a Boolean on installs in the field, so reading it as a string would throw, and the cold-start gate in MainActivity has no tour id to check against — worth doing deliberately rather than folded in here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69eb9352b7
ℹ️ 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 IME — the activity draws edge to edge, so without this the | ||
| // first and last rows of a full-height screen sit under system | ||
| // chrome. | ||
| .windowInsetsPadding(WindowInsets.safeDrawing), |
There was a problem hiding this comment.
On edge-to-edge devices, WindowInsets.safeDrawing does not include the software keyboard, so replacing the previous imePadding() leaves the lower fields and submit buttons obscured when the IME opens. This affects every scrollable Aurora form, including Login, Signup, Server Setup, and Invite Claim; retain imePadding() in addition to the safe-drawing insets.
Useful? React with 👍 / 👎.
| // what this device plays back with, and Settings re-syncs | ||
| // from the server later. A rejected PUT here shouldn't trap | ||
| // the user in a tour they can't leave. | ||
| profileRepository.updateActiveProfile(request) |
There was a problem hiding this comment.
When the user finishes on a quality or auto-skip choice, the local-store write can suspend before this call; meanwhile finished=true navigates to Home, where the user can switch profiles. updateActiveProfile() then resolves whichever profile is active at that later moment, potentially applying the completed tour's choice to another profile. Capture the profile ID when scheduling the write and update that profile explicitly.
Useful? React with 👍 / 👎.
| viewModelScope.launch { | ||
| current.steps.getOrNull(index)?.let { | ||
| onboardingRepository.recordStep(current.tourId, it.id) |
There was a problem hiding this comment.
Each rapid Next tap or settled swipe launches an independent progress POST, so variable network latency can make an earlier step arrive after a later one. The server's last_step can consequently regress, causing the next launch or another client to resume at the wrong page; serialize or coalesce these writes so only monotonically newer progress can win.
Useful? React with 👍 / 👎.
| val defaults = steps | ||
| .filter { it.kind == "setting_choice" } | ||
| .mapNotNull { step -> step.setting?.default?.let { step.id to it } } | ||
| .toMap() |
There was a problem hiding this comment.
When a profile already has a non-default setting—such as one configured before the tour was introduced or synchronized from another device—this always preselects the manifest default instead of the active value. Merely advancing past the card then persists that default and silently overwrites the user's existing preference; use the current profile/player setting when available and reserve the manifest default for genuinely unset values.
Useful? React with 👍 / 👎.
Two places collided. Settings screens (AndroidPlayerSettingsStore, PlaybackSettings, SettingsViewModel, SubtitleSettings, TvSettingsScreen, TvSettingsViewModel). Main's PR #125 (invite claim + onboarding tour) landed on the same files this branch moved onto the canonical settings contract. Resolved as a union: the canonical-settings plumbing wins for settings behavior — ProfileSettingsController resolves the profile-scoped keys instead of reading preference columns off GET /profiles, the two-axis QualityPresets picker replaces the single defaultQuality label, per-key optimistic writes replace the whole-triple UpdateProfileRequest PUT, and the SERVER_UPGRADE_REQUIRED notice stays — while main's onboarding/tour additions come across untouched. Main's language-row migration in the profile-load path is dropped as dead code on this branch, not as a reverted intent: those values now come from the effective-values endpoint, which never carried the legacy display labels. The DataStore read still runs LanguageOptions.migrateLegacyValue, so the on-device legacy rows main was protecting are still translated. LanguageOptions. Main revised the file this branch introduced (TAGS -> tags, preservable-tag echo in label(), canonicalSubtitleLanguage-backed migrateLegacyValue). Main's revision is kept wholesale and our call sites are adapted to it: TvSettingsScreen's AudioLanguages/SubtitleLanguages become audioLanguages/subtitleLanguages, and SubtitleSettings collapses its two identical option lists into main's single hoisted languageOptionLabels. migrateLegacyValue is now wider, not narrower — a valid tag outside the picker table ("nl", "pt-BR", "eng") passes through instead of being erased, and "Off" and "Default" still clear. Nothing the flusher tests assert changed. Verified: :shared:testDebugUnitTest (960 tests) and :android-shared:testDebugUnitTest (1021 tests) pass with no failures or skips; :androidApp:compileDebugKotlin, :androidTvApp:compileDebugKotlin, :androidApp:compileDebugUnitTestKotlin and :androidTvApp:compileDebugUnitTestKotlin all build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Android companion to Silo-Server/silo-server#501 (emailed invitations + server-driven onboarding).
silo://invite?server=…&token=…(host registered in the manifest) opensInviteClaimScreenwith the server URL and single-use token already bound, skipping the "which server?" step entirely. The invitee sets only a password; their email address becomes their username. On success the server is registered through the existingAuthRepository.setServerUrlpath and tokens persist — identical post-conditions to a manual login. Expired/used/revoked links render an explanatory card, not an error toast.OnboardingTourScreenrendersGET /onboarding/flow?surface=phone: one step per page (Aurora treatment), skip always reachable, unknown step kinds dropped at load per the forward-compat contract. Progress and completion post per profile, so finishing on the phone silences web and TV.setting_choicesteps write through the existing profile-update path (quality_preference,subtitle_mode, …)./onboarding/stateand immediately hands off to Home when the profile has already completed or skipped. Any state/flow error also skips rather than blocking first run.Shared additions:
InvitationModels,OnboardingModels,OnboardingApi,OnboardingRepository, invitation lookup/accept onAuthApi/AuthRepository, Koin wiring.Out of scope (follow-up): androidTvApp
surface=tvrenderer; https App Links (need the server to hostassetlinks.json— custom scheme works today).Verification
Not yet exercised against a device/emulator — the server endpoints themselves were verified end-to-end in the server PR (browser + curl). Flagging that a manual emulator pass of the deep link (
adb shell am start -a android.intent.action.VIEW -d "silo://invite?server=…&token=…") is the right pre-merge check.AI Disclosure
AuthRepository.setServerUrlpath which upserts + switches + restores token scope; (2) the tour screen initially used invented modifier helpers (clickableOption,border16) — replaced with realclickable/border; (3)UpdateProfileRequestis typed per field, so the manifest's string key maps through an explicit whitelist and unknown keys no-op instead of failing the tour. Known gap: no emulator run yet (stated above).🤖 Generated with Claude Code
Summary by CodeRabbit
silo://invitedeep links (password setup + completion routing).