No task waits on a Codex review; the redesign tasks are confirmed on the product owner's Z Fold7; TASK-016's leftovers are on hold; no biometric reauthentication. Agent: main (opus) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
160 lines
11 KiB
Markdown
160 lines
11 KiB
Markdown
# TASK-038: Settings screen with an opt-in auth-retry toggle and a favorite-push on/off toggle
|
|
|
|
Status: implemented; all required checks pass; live-confirmed on the human product owner's device
|
|
Owner: Claude
|
|
Reviewer: Codex
|
|
|
|
## Outcome
|
|
|
|
Two features the human product owner explicitly asked for together, both gated behind a new
|
|
Settings screen so neither one silently changes existing behavior for everyone:
|
|
|
|
- **A — reactive auth retry (opt-in, default off)**: when a live 401 happens despite proactive
|
|
refresh already having judged the token not yet due (e.g. a server-side revocation, not ordinary
|
|
expiry), the app now optionally retries once with a forcibly-refreshed token instead of going
|
|
straight to the re-login screen. Off by default; the human product owner explicitly asked for
|
|
this to be something "설정화면에서 설정 해두면 동작하고 안해두면 동작 안하게."
|
|
- **B — favorite push notifications (on/off, default on)**: TASK-036's background push checker can
|
|
now be turned off entirely from Settings, closing that task's own "no in-app way to disable this"
|
|
known limitation.
|
|
|
|
## Scope
|
|
|
|
- Included: new `core/settings/` package (`AppSettingsStore` + `SharedPreferencesAppSettingsStore`,
|
|
`AppSettingsSession`), new `feature/settings/` package (`SettingsViewModel`, `SettingsScreen`),
|
|
`Route.Settings` + `RouteStackSaver` entry, a "설정" button on the repository list's top bar,
|
|
`FavoritePushCheckWorker` gated on B, a new `ReactiveTokenProvider` interface + `RefreshingTokenProvider.forceRefresh()`
|
|
+ shared `callAuthenticated` retry helper wired into all four Gitea API classes for A.
|
|
- Excluded: No per-repository granularity for B (a single app-wide toggle, not one per favorited
|
|
repository — see Design notes). No UI indication of *why* a request succeeded on retry (it's
|
|
silent by design — the whole point is the human product owner never sees anything when it works).
|
|
No change to the *proactive* refresh TASK-029 already does unconditionally — A only adds a
|
|
reactive fallback on top of it, opt-in.
|
|
|
|
## Design notes
|
|
|
|
- **`ReactiveTokenProvider` is a separate interface from `BearerTokenProvider`, not an addition to
|
|
it**: `BearerTokenProvider` is a `fun interface` (exactly one abstract method) specifically so
|
|
every existing test fake written as a plain `BearerTokenProvider { "token" }` lambda keeps
|
|
compiling — adding a second method to that interface would have broken every one of those across
|
|
the whole test suite. `ReactiveTokenProvider : BearerTokenProvider` adds the one extra capability
|
|
(`forceRefresh()`) reactive retry needs; only `RefreshingTokenProvider` (the one production
|
|
implementation) also implements it, and the shared retry helper checks
|
|
`tokenProvider as? ReactiveTokenProvider` before relying on it, falling back to the old
|
|
single-attempt behavior (a plain test fake, or the setting disabled) otherwise.
|
|
- **One shared `callAuthenticated` helper, not four separate copies of the same retry logic**: all
|
|
four Gitea API classes (`GiteaRepositoryBrowserApi`, `GiteaHistoryApi`, `GiteaWorkItemsApi`,
|
|
`GiteaNotificationsApi`) had near-identical `authenticated`/`get` wrappers already; each now
|
|
delegates the "get a token, call, retry once reactively if warranted" orchestration to one
|
|
function (`data/repositories/GiteaRepositoryBrowserApi.kt`, alongside `BearerTokenProvider` and
|
|
`ReactiveTokenProvider`, which already lived there and are already imported by the other three
|
|
API classes — not a new architectural pattern, just one more shared thing in that same spot).
|
|
- **`forceRefresh()` always attempts a refresh, unlike `accessToken()`'s proactive check**: the
|
|
whole reason the reactive path exists is that a 401 already happened despite the token looking
|
|
fine by the clock — calling `accessToken()` again reactively would just return the same
|
|
still-time-valid-looking token and retry identically. `forceRefresh()` skips that time check
|
|
entirely, but reuses the exact same mutex and refresh-or-fall-back-to-stale logic `accessToken()`
|
|
already had (factored into a shared private `refreshOrFallBack`), so a reactive retry racing an
|
|
in-flight proactive refresh (or another reactive retry) still shares one attempt at the single-use
|
|
rotating refresh token rather than each independently trying to consume it.
|
|
- **Every new constructor parameter defaults to preserving old behavior exactly**: `reactiveAuthRetryEnabled: () -> Boolean = { false }`
|
|
on all four API classes means every existing test/call site that doesn't pass it keeps compiling
|
|
and behaving identically; production wiring (each feature's `ViewModel.Factory`) passes
|
|
`{ AppSettingsSession.store(application).isReactiveAuthRetryEnabled() }` instead. A lambda, not
|
|
the whole `AppSettingsStore` interface, so a test can flip retry on/off with a plain closure
|
|
without needing a fake store.
|
|
- **B is one app-wide toggle, not per-repository**: a per-repository toggle would need its own list
|
|
UI (every favorited repository, each with its own switch) and its own storage shape — real,
|
|
separable scope beyond what was asked for in this combined request. `FavoritePushCheckWorker`
|
|
checks the single toggle first, before touching the network at all, so turning it off also stops
|
|
the periodic check from doing any work, not just from posting.
|
|
- **Settings has no loading state**: `SettingsViewModel` reads `AppSettingsStore`'s synchronous
|
|
`SharedPreferences`-backed values directly at construction — unlike every other ViewModel in this
|
|
app, there's no network round trip to wait on, so the screen renders immediately.
|
|
|
|
## Acceptance criteria
|
|
|
|
- [x] A new "설정" button opens a Settings screen from the repository list's top bar.
|
|
- [x] Reactive auth retry defaults off; toggling it on/off persists across navigation and app
|
|
restarts (`SharedPreferences`).
|
|
- [x] Favorite push notifications defaults on (matching TASK-036's prior unconditional behavior);
|
|
toggling it off makes `FavoritePushCheckWorker` a no-op before any network call.
|
|
- [x] With reactive auth retry enabled and the token provider supporting it, a 401 triggers exactly
|
|
one retry with a forcibly-refreshed token; the original failure propagates if that retry also
|
|
fails, if the setting is off, or if the token provider doesn't support it.
|
|
- [x] Every existing API-class test and call site keeps compiling and passing unmodified (all new
|
|
constructor parameters are defaulted).
|
|
- [x] Build, unit tests, and lint pass.
|
|
- [x] Live-confirmed on the human product owner's device: the Settings screen opens, both switches
|
|
visibly toggle and correctly persist across back-navigation and re-entry, no crash.
|
|
|
|
## Security and privacy
|
|
|
|
- `forceRefresh()` performs the exact same refresh-token exchange TASK-029's proactive path already
|
|
does (same gateway call, same Keystore-backed `TokenVault`, same mutex) — this task adds a second
|
|
*trigger* for that existing, already-reviewed flow, not a new credential-handling code path.
|
|
- No new data is stored beyond two booleans in a new `SharedPreferences` file
|
|
(`ubgitea_app_settings`) — no tokens, no personal data.
|
|
- Reactive retry is opt-in and defaults off specifically because it changes how *every* API call in
|
|
the app behaves on a 401 — a security-adjacent code path was judged worth requiring an explicit,
|
|
visible choice rather than silently changing default behavior for everyone.
|
|
|
|
## Required checks
|
|
|
|
```text
|
|
./gradlew compileDebugKotlin compileDebugUnitTestKotlin
|
|
./gradlew testDebugUnitTest
|
|
./gradlew assembleDebug lintDebug
|
|
```
|
|
|
|
## Evidence
|
|
|
|
- `core/settings/AppSettingsStore.kt` (+ `SharedPreferencesAppSettingsStore`), `AppSettingsSession.kt`:
|
|
the two-toggle store and its shared-singleton accessor, mirroring `AuthSession`'s own pattern.
|
|
- `data/repositories/GiteaRepositoryBrowserApi.kt`: new `ReactiveTokenProvider` interface, new
|
|
shared `internal suspend fun callAuthenticated(...)`.
|
|
- `core/auth/android/RefreshingTokenProvider.kt`: implements `ReactiveTokenProvider`; new
|
|
`forceRefresh()`; `accessToken()`'s refresh/fallback logic factored into a shared private
|
|
`refreshOrFallBack` both methods now call.
|
|
- `GiteaRepositoryBrowserApi.kt`/`GiteaHistoryApi.kt`/`GiteaWorkItemsApi.kt`/`GiteaNotificationsApi.kt`:
|
|
each gained a defaulted `reactiveAuthRetryEnabled: () -> Boolean = { false }` constructor
|
|
parameter; each class's `authenticated`/`get` now delegates to `callAuthenticated`.
|
|
- `RepositoryListViewModel.kt`/`HistoryViewModel.kt`/`WorkItemsViewModel.kt`/`NotificationsViewModel.kt`/`CodeBrowserViewModel.kt`
|
|
(all five `Factory`s that construct one of the four API classes): now pass
|
|
`reactiveAuthRetryEnabled = { AppSettingsSession.store(application).isReactiveAuthRetryEnabled() }`.
|
|
- `core/notifications/FavoritePushCheckWorker.kt`: checks
|
|
`AppSettingsSession.store(application).isFavoritePushNotificationsEnabled()` first, before the
|
|
session check, before any network call.
|
|
- `feature/settings/SettingsViewModel.kt`, `SettingsScreen.kt`: the new screen and its ViewModel.
|
|
- `ui/navigation/Route.kt`, `RouteStackSaver.kt`: new `Route.Settings` + its encode/decode case.
|
|
- `feature/repositories/RepositoryListScreen.kt`, `MainActivity.kt`: the "설정" button and its
|
|
wiring into the route stack.
|
|
- New tests: `RefreshingTokenProviderTest.kt` gained `forceRefresh()` coverage (refreshes regardless
|
|
of stated expiry, falls back to stale on no-refresh-token/failed-refresh, empty when nothing
|
|
stored); new `CallAuthenticatedTest.kt` (disabled setting doesn't retry, an unsupporting token
|
|
provider doesn't retry, a successful retry uses the refreshed token, a failed retry propagates the
|
|
original failure, a blank forced-refresh result doesn't retry, a non-auth failure is never
|
|
retried); `RouteEncodingTest.kt` gained a `Route.Settings` round-trip case.
|
|
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL.
|
|
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing (every pre-existing test
|
|
across all four API classes and `RefreshingTokenProvider` unmodified and still green).
|
|
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors.
|
|
- Live device: installed over wireless `adb`. Opened Settings from the new button; both switches
|
|
visibly toggled (screenshot-confirmed each state change); backed out and re-entered Settings —
|
|
both toggles still reflected exactly what was set, confirming `SharedPreferences` persistence;
|
|
restored both to their defaults afterward; `logcat` clean of `FATAL EXCEPTION`/`AndroidRuntime`
|
|
throughout.
|
|
|
|
## Known limitations
|
|
|
|
- Reactive auth retry's actual effect (recovering from a live 401 without forcing re-login) hasn't
|
|
been observed end-to-end live — that requires a genuine 401 to occur with the setting on, which
|
|
is by nature a rare, hard-to-trigger event; covered instead by direct unit tests of the retry
|
|
orchestration (`CallAuthenticatedTest`) and of `forceRefresh()` itself.
|
|
- B remains one app-wide toggle, not per-repository (see Design notes) — deliberate, not an
|
|
oversight.
|
|
|
|
## Review findings
|
|
|
|
None yet — no Codex review pending (Codex retired, 2026-09-29).
|