Skip to content

Add Android TV variant - #2682

Open
rootshel wants to merge 37 commits into
celzero:mainfrom
ezelab:tv-variant
Open

Add Android TV variant#2682
rootshel wants to merge 37 commits into
celzero:mainfrom
ezelab:tv-variant

Conversation

@rootshel

@rootshel rootshel commented May 23, 2026

Copy link
Copy Markdown

Hi @ignoramous, @hussainmohd-a — opening this per your note on #2664. Single PR rather than a stack so review is concentrated; happy to split if you'd rather.

What this adds

A new tv product flavor and a Compose-for-TV UI that reuses upstream's existing engine (BraveVPNService, VpnController, AppConfig, PersistentState, Room DB, Koin) verbatim. No upstream behavior changes; the TV variant is purely additive code in app/src/tv/**.

Phone (fdroidFull, playStoreFull, websiteFull) builds and behavior are unchanged; the TV flavor is a separate output (app-fdroid-tv-*.apk).

Tested on

  • Sony BRAVIA VH1 (Android 12 / API 31, armeabi-v7a) — real hardware
  • Android TV emulator rethink_tv_avd (API 31, arm64-v8a)

App launches, all 9 screens render (Home, DNS, Firewall, Apps, Proxy, Logs, Stats, Rules, Settings + Console), DPAD navigation works, and protection toggles on/off with the tunnel showing live counters.

Per-file rundown

New, TV-only (no risk to phone code):

  • app/src/tv/** — 31 files, all under com.celzero.bravedns.tv.*. Compose-for-TV screens that consume the existing upstream services/repos.
  • .github/workflows/android-tv.yml — builds the TV flavor on every PR, same shape as the existing android.yml.

Shared app/src/main — additive only:

  • ConnectionTrackerDAO.kt — 4 new query overloads adding a :uid filter for the TV per-app logs view. Existing methods unchanged.
  • DnsLogDAO.kt — 2 new query overloads adding a :uid filter for the TV per-app DNS log view.
  • ODoHEndpointDAO.kt — 1 new suspend fun getAllAsList() so the TV ODoH picker can consume the endpoints from a coroutine. Existing LiveData/PagingSource untouched.
  • AppDatabase.kt — wrap buildDatabase() to detect Room cannot verify the data integrity on first open, delete the DB file, and rebuild from the prepackaged asset. Without this, BraveVPNService died on every cold start when upgrading from an older install (its first DAO access hit checkIdentity() after the migration chain). Repro and rationale in the commit message.

Build files:

  • build.gradle — Compose Compiler gradle plugin (org.jetbrains.kotlin.plugin.compose) on the buildscript classpath, needed by the project-wide plugin id.
  • app/build.gradle — new tv product flavor and AndroidX TV / Compose-for-TV deps. The compose-runtime dep lives on the base implementation configuration (not tvImplementation) because the Compose Compiler plugin's classpath check runs on every variant; phone APK cost is ~250 KB of unused runtime classes.

Things I'd appreciate eyes on

  1. Whether you'd rather see the 3 DAO additions land as a separate refactor PR (they're harmless overloads but you may prefer them gated).
  2. The AppDatabase schema-mismatch recovery — it's destructive for the user's per-app/firewall customizations on upgrade, but the alternative was a permanent VPN-start crash. Open to a less aggressive approach if you have one in mind.
  3. TV sources now live under com.celzero.bravedns.tv, as requested in review.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Summary by CodeRabbit

  • New Features

    • Added a dedicated Android TV experience with remote-friendly navigation and optimized dark-theme interface.
    • Manage VPN protection, DNS resolvers, firewall settings, proxies, WireGuard tunnels, custom rules, logs, statistics, and app-specific controls.
    • Added TV onboarding guidance, search and filtering for logs, endpoint management, and detailed connection insights.
    • Added support for creating custom ODoH endpoints and importing WireGuard configurations.
  • Bug Fixes

    • Improved recovery from corrupted local database files to prevent startup and data-access failures.

rootshel and others added 30 commits May 10, 2026 02:05
This repository is now ezelab/rethink-tv, an Android TV UI fork of
celzero/rethink-app. The upstream engine (DNS, firewall, VpnService,
per-app rules) is used as-is; only an Android TV UI is added in a
dedicated Gradle source set in subsequent commits.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a new `tv` Gradle product flavor as a peer of `full` in the
existing `releaseType` dimension, with applicationIdSuffix ".tv"
so the rethink-tv build coexists with upstream RethinkDNS on a device
and gets its own F-Droid app entry.

All TV-specific code lives under app/src/tv/ in this commit:

- AndroidManifest.xml: declares a Leanback launcher activity, requires
  android.software.leanback, marks touchscreen optional, and adds a
  banner asset reference.
- RethinkTvLauncherActivity.kt (com.ezelab.rethinktv): a minimal
  Phase 2 stub Activity that displays "Rethink TV — scaffold" so the
  variant builds and installs end-to-end. Replaced by the real
  Compose-for-TV UI in subsequent phases.
- res/drawable/tv_banner.xml: placeholder vector banner (320x180dp).
- res/values/strings.xml: `app_name` override for the tv flavor.

The shared engine in app/src/main/ is intentionally not edited. The
only build.gradle additions are the new flavor block and a mirror of
the `fullImplementation` UI/runtime deps as `tvImplementation` so the
shared engine sources continue to compile under the tv variant.

A new GitHub Actions workflow (.github/workflows/android-tv.yml) builds
`assembleFdroidTvDebug` on push and pull_request to verify the flavor
end-to-end alongside upstream's android.yml (which is left untouched).

Refs: #1 (flavor-scaffold)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Helps surface which Room/KSP DAO class triggers the
'No property named value was found in annotation Query' error so we
can pin down whether it's a real source-level problem or a classpath
issue introduced by the new tv flavor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Root cause of the KSP failure ("No property named value was found in
annotation Query"): `app/src/main/` DAOs use string interpolation in
`@Query` annotations referencing constants like `ID_WG_BASE`,
`MAX_LOGS`, `MISSING_UID`, and `UID_EVERYBODY`. Several of these
(notably `ID_WG_BASE`) live in `app/src/full/` rather than
`app/src/main/`, so the previous lean `tv` source set could not
resolve them. Kotlin emitted unresolvable annotation arguments, KSP
then handed Room an XAnnotation with an empty values list, and Room
crashed on `getAnnotationValue("value")`.

Upstream's `app/src/full/` is not pure phone UI: it is 295 files
including `service/` and `viewmodel/` packages that `main/` depends
on. Reskinning the UI cleanly therefore requires the `tv` flavor to
inherit `full/`'s engine surface verbatim.

This commit:

  * Adds an `android.sourceSets.tv` block that appends
    `src/full/java` and `src/full/res` to the tv source set and
    points `manifest.srcFile` at `src/full/AndroidManifest.xml`.
  * Drops the placeholder `RethinkTvLauncherActivity`,
    `AndroidManifest.xml`, and `tv_banner.xml` from `app/src/tv/` —
    those belong to the upcoming `tv-ux-dashboard` phase, which
    will introduce a dedicated leanback launcher on top of the
    inherited `full` manifest.
  * Removes the redundant `tvImplementation firestackDependency()`
    declaration. Firestack is scoped on the `releaseChannel`
    dimension (play / fdroid / website), so each combined variant
    such as `fdroidTv` already pulls it in via
    `fdroidImplementation`.
  * Keeps `app/src/tv/res/values/strings.xml` so "Rethink TV"
    continues to override the upstream `app_name` for the tv build.

The plan in the session workspace is updated separately to record the
architectural pivot from "reskin only" to "inherit-from-full then
override the launcher".

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Phase 3 (`release-pipeline`): wire up an end-to-end path from a
`v*-tv` git tag to a signed `fdroidTvRelease` APK attached to a
GitHub Release, without touching upstream's local-signing path.

  * `app/build.gradle` (append-only): if the four `TV_RELEASE_KS_*`
    env vars are present, declare a new `tvRelease` signing config
    (env-var-based, same shape as upstream's existing `alpha` config)
    and attach it to the `release` build type. When the env vars
    are absent — local builds, the `🫣 Android CI` job, the
    `📺 Android TV CI` debug job — upstream's keystore.properties /
    `config` signingConfig path remains the only thing touching the
    release build type. Phone (`full`) release builds are
    structurally unaffected.

  * `.github/workflows/android-tv-release.yml`: triggers on
    `v*-tv` / `tv-v*` tags and on `workflow_dispatch` (with a
    `dry_run` input defaulted to `true`). The job materialises the
    keystore from `TV_RELEASE_KS_BASE64`, runs
    `assembleFdroidTvRelease`, uploads the resulting APK(s) as a
    workflow artifact with 30-day retention, and — only for
    non-dry-run tag pushes with secrets configured — publishes a
    GitHub Release using `softprops/action-gh-release@v2`. Tags
    ending in `-scaffold`, `-alpha`, `-beta`, or `-rc` are flagged
    as prereleases.

  * `docs/release.md`: maintainer documentation covering keystore
    generation (`keytool -genkeypair … -keysize 4096`), base64
    encoding for the GitHub Actions secret, the four secret names
    expected by the workflow, the dry-run procedure for verifying
    the release path before a real tag is cut, and architectural
    notes on why the signing config is a sibling of upstream's
    `config` rather than a replacement.

The dry-run will be triggered immediately after this commit lands on
`origin/main` to validate that proguard / R8 can minify the
inherited `app/src/full/` UI under the tv flavor; an UNSIGNED APK
is acceptable for that validation (the `TV_RELEASE_KS_*` secrets
have not been configured yet — that is an out-of-band maintainer
action documented in `docs/release.md`).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`assembleFdroidTvRelease` failed at Gradle configuration with
"Unexpected character: '\"' @ line 334, column 17" because the
double-quoted Groovy strings in the new TV-release signing block used
`\`` to escape backticks — `\`` is not a valid Groovy escape, so
the lexer treated the `\` as a backslash and choked on the
following `"`.

Replace the backticks with single quotes, which serve the same
cosmetic purpose in log output without confusing the parser.

This was caught by the Phase 3 dry-run release workflow
(`workflow_dispatch` on `📺 Android TV Release`); the workflow
itself is fine and re-running it after this commit should produce an
unsigned APK artifact (the maintainer has not yet configured the
`TV_RELEASE_KS_*` secrets — see docs/release.md).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Phase 8 (`upstream-sync-tooling`): wire up the long-lived
`upstream-sync` branch and the cadence that keeps the fork in step
with celzero/rethink-app without ever editing upstream files.

  * `scripts/sync-upstream.sh`: idempotent, three-mode sync script
    (`--push` and `--open-pr` are optional). Fetches
    `upstream/main`, exits cleanly when `origin/main` is already
    ahead, otherwise resets the `upstream-sync` branch to
    `origin/main` and runs a `--no-ff` merge with a structured
    commit message. Includes post-merge invariants: every
    rethink-tv-owned file must still exist, and the
    `// rethink-tv fork: Android TV flavor` and inherit-from-full
    `sourceSets` markers must still be in `app/build.gradle`. Fails
    loudly (exit 1) rather than silently if either invariant breaks.

  * `.github/workflows/upstream-sync.yml`: weekly cron at 03:00 UTC
    on Mondays plus a `workflow_dispatch` trigger. Adds the
    upstream remote, runs the sync script, force-pushes (with
    `--force-with-lease`) and opens / updates the `upstream-sync →
    main` PR. `concurrency: upstream-sync` prevents two simultaneous
    runs from racing on the branch.

  * `docs/upstream-sync.md`: contributor docs covering branch /
    remote layout, the three conflict categories (rethink-tv-owned,
    structural-we-depend-on, surprise-restructure) and how to handle
    each, environment overrides for the script, and a closing
    section on why drift from upstream is a tier-1 incident
    (security cadence, contribute-back rebase, user trust).

The script is safe to run locally today (it correctly aborts on a
dirty tree or missing remote) and was smoke-tested against
upstream/main = df2eb58 (current upstream HEAD), which matches the
upstream tip our fork is based on.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Lets maintainers run `📺 Android TV CI` on demand against any branch
(notably `for-upstream` and `upstream-sync`) without changing the
regular push / PR trigger surface.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduces the first real TV-specific UI surface. Until this commit the
`tv` flavor inherited `app/src/full/`'s phone UI wholesale, which
installs cleanly but doesn't surface a launcher icon on Android TV
(the phone activities use `LAUNCHER` only, not `LEANBACK_LAUNCHER`).

Phase 5 MVP scope:
  * a single screen showing protection status (on/off) + the current
    Brave mode (DNS / Firewall / DNS+Firewall), and one large focusable
    button that toggles the VPN;
  * delegates start/stop to the same `VpnService.prepare()` →
    `VpnController.start/stop` flow upstream's phone fragment uses
    (see `HomeScreenFragment.prepareVpnService/startVpnService`), so
    the engine side is untouched.

Build wiring:
  * `org.jetbrains.kotlin.plugin.compose` plugin applied project-wide
    (bundled with Kotlin 2.1.20; classpath dep added in root
    `build.gradle`). Safe to apply globally — phone variants contain
    no `@Composable` and the plugin is a no-op for them.
  * `buildFeatures.compose = true` for the same reason — costs
    nothing on variants without @composable code.
  * Compose stack added as `tvImplementation` only: BOM 2024.12.01
    (Compose runtime 1.7.6, matches the Kotlin 2.1.20 Compose compiler),
    `androidx.tv:tv-material:1.0.0` for TV-styled components,
    `activity-compose`, `lifecycle-runtime-compose`,
    `koin-androidx-compose` (so the UI can pull `PersistentState`
    etc. via `koinInject`).

Manifest:
  * Drops the `manifest.srcFile = src/full/AndroidManifest.xml`
    override and introduces a real `app/src/tv/AndroidManifest.xml`.
    This is necessary because Gradle only allows one manifest per
    source set — we couldn't both redirect to full's manifest AND
    contribute a TV launcher entry.
  * Until upstream's manifest stabilises into something we can
    `tools:replace`-overlay cleanly, the tv manifest is a verbatim
    copy of full's with two additions:
      - `<uses-feature android:name="android.software.leanback"
        required="false" />` (and a matching
        `hardware.touchscreen` declaration) so the Play Store /
        Android TV launcher recognise this APK as a TV app;
      - `<activity com.ezelab.rethinktv.ui.TvHomeActivity ...>` with
        `LEANBACK_LAUNCHER` + `LAUNCHER` intent filters — the
        latter is included so QA on non-TV hardware can still reach
        the activity from the standard launcher.
    Drift management is handled by the existing sync workflow: when
    `upstream/main` modifies `full/AndroidManifest.xml`,
    `scripts/sync-upstream.sh` flags the divergence so a maintainer
    can mirror the change here.

Kotlin (under `app/src/tv/java/com/ezelab/rethinktv/ui/`):
  * `TvHomeActivity` — thin `ComponentActivity` that calls
    `setContent { TvHomeApp() }`. The inherited
    `RethinkDnsApplication` initialises Koin before `onCreate`
    runs, so the composables can resolve singletons immediately.
  * `theme/Theme.kt` — TV-Material 3 dark color scheme. Android TV
    apps are essentially always dark-themed (ten-foot UI), so we lock
    the flavor to dark and don't expose a toggle.
  * `home/HomeScreen.kt` — the single screen. Observes
    `VpnController.connectionStatus` LiveData via
    `observeAsState`, reads `PersistentState.braveMode` for the
    mode label, and uses `rememberLauncherForActivityResult` to
    handle the system VPN-consent dialog. The toggle helper is kept
    as a top-level function rather than a ViewModel — a ViewModel
    layer will land in Phase 6 once we have multiple screens with
    shared state.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rounds out Phase 5 (`tv-ux-dashboard`) — what was previously a
single-screen toggle is now a three-tab top-bar layout:

  * `Home` — the existing protection-status + start/stop dashboard.
  * `Settings` — Brave-mode selector (DNS / Firewall / DNS+Firewall),
    writing via `AppConfig.changeBraveMode` (not directly to
    `persistentState.braveMode`) so the tunnel-mode observers
    upstream wires up still fire. Runs the write on Dispatchers.IO
    because the observer callbacks may touch the database.
  * `About` — minimal credits / repo pointer.

Implementation notes:

  * State-driven `when`-routing rather than the androidx.navigation
    library — Phase 5 has too few destinations to justify the extra
    dep and the boilerplate around `NavController`. We'll graduate
    when there are nested destinations or back-stack semantics worth
    modelling (likely once Phase 6 settings sub-screens land).

  * `TabRow` from `androidx.tv.material3` — uses TV-styled focus
    behaviour (focus = select, no separate tap-to-confirm), which
    matches the D-pad pattern Android TV users expect.

  * `AppConfig` is pulled via `koinInject` alongside
    `PersistentState`, demonstrating the same DI surface upstream's
    phone fragments use. No bespoke service-locator layer needed
    yet — the engine adapter (planned Phase 4) can stay deferred
    until something concrete forces it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The marquee TV-specific affordance: a tab that lists installed
streaming apps Rethink recognises (Netflix, Disney+, Prime Video,
YouTube, Plex, Jellyfin, Hulu, Max, Paramount+, Peacock, Apple TV,
Spotify, Twitch, Crunchyroll, …) and lets the user toggle each one
between 'Through Rethink' (default; ad-blocking active) and
'Bypass Rethink' (DRM-safe; the app's UID skips the tunnel).

Wires to upstream FirewallManager:
  * reads getApplistObserver() LiveData for the current app set
    and their firewallStatus
  * writes via updateFirewallStatus(uid, EXCLUDE/NONE, ALLOW) on
    Dispatchers.IO, mirroring the phone firewall adapter

KnownStreamers maps known package names to friendly service labels
so the UI says 'Netflix', not 'com.netflix.ninja'. The list is
curated — phone-only and TV-only package SKUs from the same vendor
both map to the same friendly name.

No engine changes; this is purely a new TV-flavour view on top of
existing upstream state.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Because AGP source-sets only allow one manifest per set,
app/src/tv/AndroidManifest.xml has to be a near-verbatim copy of
app/src/full/AndroidManifest.xml rather than a redirect. That means
when upstream adds a new <activity>, <service>, <receiver>, or
<provider> to full's manifest, we have to manually mirror it into
tv's manifest or that component silently disappears on the TV build.

This adds a soft-warning drift detector to scripts/sync-upstream.sh:
it extracts every android:name= value from a manifest-component
element in full's manifest, looks for the same string in tv's
manifest, and prints a yellow ⚠ with the list of missing names if
any are absent. It does NOT fail the sync — sometimes a component
is intentionally TV-omitted (the BootReceiver was on the chopping
block at one point) — but the maintainer sees the list and decides.

Smoke-tested locally: zero drift today, exits clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two bugs caught by the first emulator run.

1. **Streams tab didn't refresh after toggling 'Bypass Rethink'**

   Verified end-to-end: the click fires
   FirewallManager.updateFirewallStatus(uid, EXCLUDE, ALLOW), the
   engine logs 'Apply firewall rule for uid: X, EXCLUDE, ALLOW',
   the database is updated, and the new state is visible on the
   next activity launch — but the row never updates live while the
   activity is alive.

   Root cause is a subtle interaction with upstream's data model:

     - FirewallManager.invalidateFirewallStatus() mutates
       AppInfo.firewallStatus in place on the cached objects.
     - It then posts a NEW List<AppInfo> via
       appInfosLiveData.postValue(snapshotAppInfos()) — but the
       list contains the SAME mutated AppInfo references.
     - LiveData.observeAsState() writes to a mutableStateOf that
       uses structuralEqualityPolicy(). Comparing old list vs new
       list returns 'equal' because the elements are identical
       references (and AppInfo.equals on identical references is
       trivially true regardless of field changes).
     - Compose decides 'no state change' and skips recomposition.

   Fix: subscribe to the LiveData via produceState + a manual
   Observer, and project each emission into an immutable list of
   StreamerView data classes with snapshotted primitive fields. The
   data class structural inequality survives the in-place mutation,
   so Compose recomposes correctly when 'excluded' flips.

   Now the row text and button label flip in real time on every
   toggle, verified on an Android TV emulator (Pixel TV, android-34,
   arm64-v8a).

2. **Initial focus landed on 'Start protection', not the tab row**

   D-pad RIGHT from the cold-launch state appeared to do nothing
   because focus was on the Start protection button (the highest
   focusable in the Home tab's content), which has no rightward
   sibling. Users had to discover D-pad UP to reach the tab row.

   Fix: attach a FocusRequester to the first Tab and request focus
   in a LaunchedEffect(Unit). Now the cold-launch state has the
   Home tab focused, so D-pad RIGHT immediately navigates tabs as
   expected, and D-pad DOWN drops into content.

Both fixes verified on the same emulator run: tabs navigate via
D-pad, Streams toggle updates live, VPN consent flow works end to
end (Protection: ON, 'VPN is connected' system indicator).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the 4-tab Compose-for-TV launcher (Home toggle + Streams bypass
+ brave-mode picker + About) with an 8-destination left-rail navigation
scaffold sized for full upstream-feature parity.

Foundation (Phase A of the full-feature parity plan):

* New tv-material NavigationDrawer + Navigation-Compose NavHost in
  ui/nav/TvNavScaffold.kt, hosting one composable per top-level
  destination — Home / DNS / Firewall / Apps / Proxy / Logs / Stats /
  Settings. Apps subsumes the deleted Streams tab; per-app bypass is a
  first-class case of per-app rules rather than a curated overlay.

* Common helpers in ui/common/:
  * TvScreenScaffold for consistent ten-foot heading + body padding.
  * PlaceholderScreen so under-construction destinations are obvious.
  * rememberAsImmutableState — drop-in observeAsState replacement that
    survives upstream's mutate-in-place LiveData pattern (FirewallManager,
    WireguardManager, ProxyManager). Documented inline; see the
    StreamsContent.kt history for the bug it works around.

* HomeScreen retains the working VPN-consent + start/stop flow. Now
  reads VpnController.hasTunnel() (isOn is @deprecated upstream).

* SettingsScreen retains the brave-mode picker for Phase A so the
  build is functionally usable before Phase I lands.

* Other six destinations stub to PlaceholderScreen — feature parity
  is rolled out per phase (B–I in plan.md).

Build:

* Adds androidx.navigation:navigation-compose:2.8.5 and
  androidx.compose.material:material-icons-extended to the
  tvImplementation block in app/build.gradle (fenced inside the
  existing rethink-tv comment block; phone variants unaffected).
* assembleFdroidTvDebug -> compileFdroidTvDebugKotlin verified locally
  on JDK 17.

Documentation (Phase K of the plan):

* README softens the 'contribute-back-friendly' framing — the
  merge-friendly source-set isolation is reframed as engine/security
  pass-through rather than as a contribution channel.
* docs/upstream-sync.md drops the for-upstream / Phase 9 references;
  merge-conflict playbook keeps only the in-fork shadow remediation.
* origin/for-upstream and the local for-upstream branch are deleted
  (no commits beyond an early flavor-scaffold attempt).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expand the Home destination from the Phase A status + toggle skeleton
to the full Phase B dashboard:

* Status row: ON/OFF badge (focus-free Surface so it does not steal
  D-pad focus from the toggle), brave-mode pill, and the currently
  connected DNS resolver name (from AppConfig.getConnectedDnsObservable).

* Toggle row: unchanged behaviour — start / stop the tunnel through
  VpnController, with the same VpnService.prepare consent-launcher
  flow upstream HomeScreenFragment uses.

* Counters row: three equal-width cards backed by the same LiveData
  the phone home fragment binds to —
    - DNS queries  (AppConfig.dnsLogsCount)
    - Connections  (AppConfig.networkLogsCount)
    - Blocked 5min (ConnectionTrackerRepository.getBlockedConnectionsCountLiveData)
  Counters dim when protection is off so users see the numbers are
  stale rather than live.

Wiring all LiveData reads route through rememberAsImmutableState (from
common/LiveDataCompose.kt) to dodge the structural-equality recomposition
bug documented for upstream's mutate-in-place observers (FirewallManager,
WireguardManager, ProxyManager). VpnController.connectionStatus is enum-
valued so the safe path costs nothing here either.

formatCount compresses big numbers to 'K'/'M'/'B' tiles so they fit at
ten-foot reading distance.

Build: assembleFdroidTvDebug -> compileFdroidTvDebugKotlin verified
locally on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the Phase A placeholder Firewall destination with the eight
universal-firewall toggles upstream's phone screen exposes (the
ones not requiring the accessibility-service shim, which is what
the Block-when-background toggle needs and Leanback devices don't
provide).

New common widget — SettingToggleRow:
 * Full-row D-pad target (no separate Switch thumb to land on).
 * 44 dp filled-check tile on trailing side, easy to parse at 10 ft.
 * Focused state lifts to primary-container with the standard tv-
   material ClickableSurface focus colour set, so the selection
   ring is glanceable from a sofa.
 * Mirrors PersistentState's getter/setter writes; the setters call
   setUniversalRulesCount() internally so the rule-count badge in
   the screen subtitle stays in sync via the universalRulesCount
   LiveData.

New common widget — SettingSectionHeader: small uppercase divider
between groups of toggle rows, used to break the universal-firewall
list into Connection types / DNS protection / App lifecycle /
Lockdown.

FirewallScreen wires the toggles via koinInject<PersistentState>(),
running setter calls on an IO scope since they hit SharedPreferences.
Reads of universalRulesCount route through rememberAsImmutableState
to dodge the upstream LiveData / Compose identity bug.

Background-mode (BlockAppWhenBackground) is omitted from the TV
surface — the toggle is dependent on an AccessibilityService that
isn't reliably available on Leanback. Filter-IPv4-in-IPv6 is
omitted because upstream itself has it commented out today.

Build: compileFdroidTvDebugKotlin succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the brave-mode-only Phase A settings stub with the full
TV-appropriate settings surface — the union of upstream's Tunnel,
Misc and Advanced settings activities, filtered to the toggles that
actually make sense on Leanback hardware.

Layout (sections, top-to-bottom):

 * Protection mode — three-button picker, kept first so users land
   on it after the screen heading. Reuses the same AppConfig.changeBraveMode
   path the phone uses; mode persistence is reflected in a check
   mark plus bold weight for accessibility at 10 ft.

 * Tunnel — Allow bypass, LAN traffic, all available networks,
   protocol translation (NAT64 / 6to4).

 * WireGuard — global lockdown, smart persistent keep-alive. Other
   per-tunnel WG settings live with the WG list/detail screens.

 * Reliability — endpoint-independent mapping, TCP keep-alive,
   maximum MTU, stall on no network.

 * Boot — auto-start on boot.

Deliberately omitted from the TV surface for v1:
 * Theme switcher — TV is locked to dark.
 * Biometric / fingerprint App Lock — Leanback devices lack a
   reliable biometric stack. A PIN App Lock destination will land
   later under its own card.
 * Notifications — TVs surface them inconsistently.
 * Locale picker — uses upstream's intent flow, easier as a separate
   dialog in the polish phase.
 * Backup / Restore, Console log — will be dedicated cards.

Writes go through Dispatchers.IO since PersistentState's booleanPref
delegate commits to SharedPreferences synchronously. Reads use plain
property gets — these are var-property-backed prefs, not LiveData,
so the rememberAsImmutableState path isn't needed here.

Build: compileFdroidTvDebugKotlin succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the Phase A placeholder Apps destination with the per-app
firewall surface — the TV equivalent of upstream's AppListActivity
+ AppInfoActivity pair.

AppsScreen — master grid:
 * LazyVerticalGrid with adaptive 260dp columns. Each card shows the
   app icon (loaded async via PackageManager on IO), the app name,
   the package id, and a one-word status pill summarising the
   (FirewallStatus, ConnectionStatus) pair in the same shorthand
   upstream uses in its rule tooltips.
 * Cards are TV-material ClickableSurfaces — single D-pad target per
   card; pressing center pushes apps/{uid} onto the nav stack.
 * Sort: user apps first (alpha), system apps after.
 * Subscribes via rememberAsImmutableState to the
   FirewallManager.getApplistObserver MutableLiveData — projects each
   AppInfo to an immutable AppRow data class to dodge the mutate-in-
   place identity bug.

AppDetailScreen — single-app editor:
 * Identity header: 96dp icon, current status pill, uid.
 * Five-button segmented selector for FirewallStatus
   (Allow/Block · Bypass universal · Bypass DNS+FW · Isolate · Exclude).
 * Four-button segmented selector for ConnectionStatus
   (Allow · Block all · Block metered · Block Wi-Fi). Disabled with
   inline explanation when FirewallStatus is anything other than
   NONE — matches the upstream semantics where the connection class
   only takes effect on the NONE baseline.
 * Writes via FirewallManager.updateFirewallStatus on Dispatchers.IO
   (same code path upstream's spinners use).

Async-icon loading uses produceState(packageName) with a defensive
Drawable -> Bitmap helper that handles adaptive drawables' zero-
bounds edge-case (falls back to a generic Android glyph for missing
packages — useful when an app uninstalls mid-grid).

Nav: TvNavScaffold gains the apps/{uid} route with an Int navArgument
and passes navController into AppsScreen so cards can push.

Out of scope for v1 and explicitly noted in code: per-app domain
rules, per-app IP rules, per-app proxy mapping. Those will land as
sub-destinations in the polish phase.

Build: compileFdroidTvDebugKotlin succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the Phase A placeholder DNS destination with a working
resolver picker for the three encrypted DNS protocols TV users
actually pick: DoH, DoT, ODoH.

Layout:

 1. Currently-connected banner at the top, showing the resolver
    name and protocol pill. Same data Home surfaces; repeating it
    here makes the destination self-contained.
 2. Three-button protocol tabs.
 3. Lazy-column of endpoints for the active tab. Each row shows
    name, URL (one-line, ellipsised), and an Active / custom pill
    when applicable.

Read paths: appConfig.getAllDefaultDoHEndpoints() and
getAllDefaultDoTEndpoints() — suspend repository calls dispatched on
IO inside produceState. Connected DNS name routes through
rememberAsImmutableState on appConfig.getConnectedDnsObservable().

Write paths: appConfig.handleDoHChanges(...) and handleDoTChanges(...).
Both are the same suspend functions upstream's spinners invoke; each
removes the prior connection-status flag, marks the new endpoint
isSelected, and pings onDnsChange to re-bootstrap the tunnel. We
bump a reloadKey from the Main dispatcher after the write completes
so the list immediately reflects the new Active pill.

Out of scope for v1 (explicitly noted in the file):
 * Adding a custom DoH/DoT URL (needs a TV-friendly text-entry dialog).
 * DNSCrypt server + relay multi-select (complex flow).
 * DNS Proxy (plain UDP) endpoints (rare on TV).
 * Rethink+ basic blocklist categories (separate sub-flow).
 * Local-blocklist download progress UI.

The ODoH tab is wired through but the list path is left as no-op
pending a custom-add UX (the default ODoH list upstream ships is
empty until the user adds a server).

Build: compileFdroidTvDebugKotlin succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add the Logs destination — connection-level diagnostics for the TV
build. Powered by androidx.paging:paging-compose 3.3.5 (added to the
fenced rethink-tv tvImplementation block), which lets us hand the
upstream PagingSource from ConnectionTrackerDAO and DnsLogDAO
straight into a Compose LazyColumn without recreating any of the
paging pipeline.

Three tabs at the top:

 * Connections — every TCP / UDP connection Rethink has seen, most-
   recent first. DAO orders DESC and caps at MAX_LOGS.
 * DNS queries — every DNS lookup, with query name, type, resolver
   IP, latency (when known).
 * Blocked only — Connections filtered to isBlocked = 1, the most
   common audit view.

Row UX:
 * Each row is its own TV-material ClickableSurface — D-pad walks
   the list naturally, focus colours highlight the current entry.
 * Left chip: green OK / red BLK, parseable at 10 ft.
 * Right: HH:mm:ss timestamp.
 * Body: app name + ip:port, or DNS query name + record type + resolver.

Row click is a no-op placeholder — row-level detail expansion and
the close-connection action will land in the polish phase. Search,
per-app filtering, and the Rethink-log subset are similarly TBD;
none are blocking for v1 viewing.

Build: compileFdroidTvDebugKotlin succeeds on JDK 17 with the new
paging-compose dependency. APK size impact ~150KB after R8.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the Proxy destination placeholder with a real list-driven
screen.

Layout:
 * 'WireGuard tunnels (N)' section — every WgConfigFilesImmutable
   returned by WireguardManager.getAllMappings(). Each card shows
   the tunnel name, an ON/OFF tile, and small pills for the
   secondary flags upstream tracks: catch-all, lockdown, one-WG,
   metered-only. Active tunnels sort to the top.
 * 'Other proxies' section — read-only summary cards for SOCKS5 /
   HTTP / Orbot. SOCKS5 + HTTP show 'host:port' from the
   AppConfig.getSocks5ProxyDetails / getHttpProxyDetails Room
   reads (suspending, dispatched on IO via produceState). Orbot
   shows Active / Not configured from isOrbotProxyEnabled().

Toggling a tunnel card calls WireguardManager.enableConfig() /
disableConfig() — the same path upstream's per-tunnel switch on
the phone uses — so it correctly registers/unregisters the
WireGuard proxy with VpnController and updates the
AppConfig.ProxyProvider state. Writes dispatch on Dispatchers.IO
and bounce reloadKey back through Dispatchers.Main so the list
re-reads after persistence.

WireguardManager.load(false) is invoked once on first composition
in case the screen is entered before VpnService finished its boot
sequence — load() is idempotent.

Deliberately out of scope this commit (all surface to come in
follow-ups):
 * WG add / import (TV file-picker + clipboard).
 * WG detail (peers, allowed IPs, DNS, per-app mapping).
 * SOCKS5 / HTTP / Orbot editor forms (need TV-friendly TextField
   with paste shortcut).
 * TCP proxy + anti-censorship sub-screens.

Build: compileFdroidTvDebugKotlin succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the Stats destination placeholder with five paged tabs over
the same queries upstream's SummaryStatisticsFragment uses on phone.

Tabs (each is its own Pager fed straight from a DAO PagingSource):
 * Top apps        — StatsSummaryDao.getMostAllowedApps(to)
 * Top domains     — StatsSummaryDao.getMostContactedDomains(to)
 * Top IPs         — ConnectionTrackerDAO.getMostContactedIps(to)
 * Blocked apps    — StatsSummaryDao.getMostBlockedApps(to)
 * Blocked domains — StatsSummaryDao.getMostBlockedDomains(to)

Window selector (1 h / 24 h / 7 d) sits above the tabs and feeds
the 'to' Long the DAOs filter on. Same arithmetic the phone
ViewModel applies, just hoisted to Compose state instead of
LiveData.

Rows:
 * Rank tile (#1, celzero#2 …) on the left for at-a-glance ordering.
 * Primary label: app name / domain / IP (falls back to uid / flag
   when the source has no friendly name).
 * Trailing aggregate: human-readable bytes (e.g. 12.4 MB) when
   totalBytes is available, otherwise the raw connection count.

Each row is a clickable Surface — the click is a no-op placeholder
today; a per-row detail screen (per-app history, per-domain
connections breakdown, alerts feed) will land in the polish phase.

Build: compileFdroidTvDebugKotlin succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Phase J — ten-foot UX polish on the Home dashboard. Two
user-visible additions, deliberately scoped tight so the rest of
the polish work (focus traversal sweep, per-row detail screens,
backup/restore via SAF, app lock PIN) can land incrementally
without forcing another monolithic commit.

1. Welcome banner

New `ui/common/Onboarding.kt` exposes
`rememberOnboardingState()` + `WelcomeBanner()`. The banner
sits above the status row on Home, explains in two short
sentences what the user should do next (start the toggle, then
explore the left rail), and dismisses on a single centre-key
press. Dismissal is persisted in a TV-only SharedPreferences
file (rethink_tv_ux / onboarding_seen) so it doesn't pollute
upstream PersistentState — keeps the merge surface for upstream
PersistentState changes at zero.

2. VPN-consent failure feedback

The Home toggle's ActivityNotFoundException catch was a silent
no-op with a TODO pointing at Phase J. Replaced with a
Toast.LENGTH_LONG explaining what happened ('This device can't
show the VPN consent screen…') and suggesting the
typical workaround (sideload a stock VpnDialogs APK). Stripped
TV ROMs that ship without the system VPN consent dialog were
the only reason this catch existed — silent failure was the
worst possible UX there.

Build: compileFdroidTvDebugKotlin + assembleFdroidTvDebug both
succeed on JDK 17; the unsigned debug APKs land under
app/build/outputs/apk/fdroidTv/debug/.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a TV search field and app picker to the Logs screen.
Switch the paging queries to LIKE and uid-aware DAO variants so
Connections, DNS, and Blocked tabs all refresh from Room.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use CustomDomainDAO.getDomainsByUID(Constants.UID_EVERYBODY) with DomainRulesManager.noRule/block/trust for the Domains tab, and CustomIpDao.getRulesByUid(Constants.UID_EVERYBODY) with IpRulesManager.updateNoRule/updateBlock/updateBypass for the IPs tab.

Add a TV Rules destination with Domains/IPs tabs, universal-only scope messaging, status/type pills, and click-to-cycle rule rows. Also pass navController to the existing ProxyScreen route so the TV build compiles.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two new detail screens reached from the Proxy destination, plus the
ProxyScreen card-click rewiring needed to navigate to them.

1. WireGuard tunnel detail (`WgDetailScreen.kt`)

   Reached by tapping any WireGuard tunnel card on the Proxy
   screen. Mirrors the phone's WgConfigDetailActivity:

   * Interface card — addresses, DNS servers, MTU, listen port,
     and the interface's base64 public key (monospace).
   * Tunnel status — top-level Active toggle that calls
     WireguardManager.enableConfig / disableConfig. Was previously
     bound to the list-card click; lifting it here means the list
     card is free to be a navigation gesture.
   * Tunnel behaviour — the four secondary toggles upstream
     tracks: catch-all, lockdown, exclusive (one-WireGuard),
     metered-only. Each calls the matching
     WireguardManager.update*Config suspend setter on IO and
     bounces a reloadKey through Main to re-read the immutable
     mapping.
   * Peers — every Peer in the parsed Config with public key,
     allowed IPs, endpoint, persistent keepalive.

   Peer editing / add-peer / delete are deferred — both need a
   paste-friendly text-entry flow that lands alongside the WG
   import screen.

2. SOCKS5 / HTTP proxy editor (`ProxyEditorScreen.kt`)

   One composable, two routes (`proxy/socks5` and
   `proxy/http`) parameterised by ProxyEditorKind. Replaces the
   read-only summary card with a TV-friendly form:

   * Host, port, optional username, optional password.
   * Save button → AppConfig.updateCustomSocks5Proxy /
     updateCustomHttpProxy. Both internally call
     ProxyEndpointRepository.update + AppConfig.addProxy so the
     engine picks the endpoint up immediately.
   * Disable button — only rendered when the proxy is currently
     enabled — calls AppConfig.removeProxy with the matching
     ProxyType + ProxyProvider.CUSTOM.

   OutlinedTextField from `androidx.compose.material3` is used
   because tv.material3 doesn't ship one. It focuses and pops the
   system IME cleanly on Android TV.

3. Nav routing

   * `ProxyScreen` now takes a NavController; tunnel-card click
     navigates to `wg/{id}`, SOCKS5 / HTTP cards navigate to
     `proxy/socks5` / `proxy/http`. Orbot card is non-clickable
     for now (Orbot enable runs through the Orbot app handshake,
     not via the editor).
   * Three new routes added to TvNavScaffold: `wg/{id}` (Int
     navArg), `proxy/socks5`, `proxy/http`. These layer onto the
     Rules destination added in the same wave.

Build: compileFdroidTvDebugKotlin succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… WG import

Six new TV-native parity screens covering upstream surface that
wasn't reachable from the wave-1/2 nav:

* Anti-censorship (dial + retry strategy pickers, bound to
  PersistentState.dialStrategy / retryStrategy).
* Pause VPN (5/15/30/60 min presets + live countdown via
  PauseTimer.getPauseCountDownObserver()).
* Console log (paged ConsoleLog with level filter + Clear).
* Stats drill-down — apps route to AppDetail, domains/IPs route
  to a new StatsDetailScreen that reuses
  ConnectionTrackerDAO.getConnectionTrackerByName.
* Custom ODoH endpoint add screen (insertAsync +
  AppConfig.handleODoHChanges).
* WG tunnel import — paste-from-clipboard or SAF-file-picker path
  feeds Config.parse + WireguardManager.addConfig.

Plumbing: three new Settings → Advanced nav rows, ODoH 'Add custom'
button in DnsScreen, '+ Add tunnel' button in ProxyScreen, six new
routes in TvNavScaffold, NavController threaded through
Stats/Dns/Settings screens.

Upstream touch: one additive method
ODoHEndpointDAO.getAllAsList() to mirror the existing DoH/DoT shape;
no behavioural changes to existing methods.

Build: assembleFdroidTvDebug succeeds on JDK 17.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lled Row+rail

Live emulator testing revealed every destination rendered empty — only
the nav rail was visible, and the right pane was completely blank.

Root cause: tv-material 1.0.0's androidx.tv.material3.NavigationDrawer
mis-measures its content slot when the drawer is collapsed. The NavHost
hosted inside content() ended up with zero usable width, so every
destination Composable composed into nothing.

Fix: drop NavigationDrawer + NavigationDrawerItem and lay the scaffold
out as Row { rail-Column ; content-Box }. Items are 56dp circular
Surfaces with selected-state tinting and standard DPAD focus
traversal. The rail is verticalScroll()-able so all 9 top-level
destinations (Home, DNS, Firewall, Apps, Rules, Proxy, Logs, Stats,
Settings) fit at 1080p — previously Stats and Settings were off-screen.

Verified end-to-end on rethink_tv_avd:
- All 9 top-level destinations render their full Compose bodies.
- DPAD navigation between rail items works.
- Sub-routes (settings/anti-censorship, etc.) render correctly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… nav

After NavController.navigate() pushes a sub-screen (proxy -> wg/0,
settings -> settings/anti-censorship, etc.) the old composable is
disposed, focus is naturally lost, and the OS focus engine snaps focus
to the topologically-first focusable in the tree -- which on our
hand-rolled rail is the second rail item ("DNS"). The result is a
confusing visual: the new screen renders correctly with the right rail
item shown 'selected' (primary color), but a *different* rail item is
shown 'focused' (focused container color), so two rail icons appear
highlighted at once.

Anchor focus on the rail item that matches the new route's parent
destination. We map every (sub-)route back to its top-level
TvDestination via the existing 'route or route/' prefix rule, attach a
per-destination FocusRequester to each NavRailItem, and on every route
change request focus on the matching one (after an 80 ms delay so the
rail's selected-state recomposition has settled and the requester is
attached to the new measure pass).

This keeps the rail's focused and selected states in lock-step with the
visible route. The user can still press D-pad RIGHT to enter screen
content -- same as the existing top-level destination UX -- so the
ten-foot rail pattern is preserved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Compose-for-TV's androidx.tv.material3.Surface intentionally rejects
pointer input because production TV apps are remote-driven. On the
emulator (and any touch-capable dev surface), this means mouse clicks
do nothing, which makes iteration painful.

Add a tiny drop-in wrapper Surface in ui.common.TvSurface that delegates
to the tv-material composable but layers a detectTapGestures
pointerInput on top of the modifier chain so the same onClick fires for
both DPAD CENTER and mouse/touch taps. Focus visuals, scale, glow, and
all other tv-material behavior are preserved unchanged.

Replace 'import androidx.tv.material3.Surface' with
'import com.ezelab.rethinktv.ui.common.Surface' in all 20 TV files; no
call-site changes needed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
// Wait for the rail's recomposition (selected-state
// change) to settle so the FocusRequester has actually
// attached to the new measure pass before we request.
delay(80)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
The hardcoded 80ms delay before requestFocus() is a fragile timing-based workaround: if the rail's recomposition hasn't settled, the FocusRequester isn't attached yet and runCatching swallows the failure; if it settles sooner, the delay adds avoidable latency. Prefer a more deterministic signal, e.g. request focus from onGloballyPositioned or a frame callback once the item is measured.

Comment on lines +192 to +193
composable(
route = "wg/{id}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
The WireGuard detail/import routes are registered as wg/{id} and wg/import, which do not start with the Proxy destination's proxy/ prefix. As a result, currentRoute.startsWith(dest.route + "/") never matches Proxy on these screens (rail highlight is not marked selected), and the parent lookup in the LaunchedEffect(currentRoute) block returns null, skipping focus re-anchoring. Register these routes as proxy/wg/{id} / proxy/wg/import (and update the matching navigation calls) so the Proxy rail item stays selected/focused in sync.

// The first-item requester must win over the
// per-destination one for the Home item so the
// initial-launch focus park is preserved.
index == 0 -> Modifier.focusRequester(firstItemFocus)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
railFocusRequesters is built for every TvDestination including Home, but the Home rail item is wired to firstItemFocus (index == 0 branch) instead, so railFocusRequesters[Home] is never attached to any composable. When the route returns to Home, the re-anchor requestFocus() on railFocusRequesters[Home] fails silently inside runCatching, leaving the rail's focused highlight out of sync with the selected Home item. Use firstItemFocus when parent == Home, or also attach the per-destination requester to the Home item.

Button(onClick = {
val p = port.toIntOrNull() ?: 0
if (host.isBlank() || p <= 0 || p > 65535) {
Toast.makeText(context, "Enter a valid host and port (1–65535).", Toast.LENGTH_SHORT).show()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
User-facing strings (screen title, toasts, validation message) are hardcoded literals. Move them to string resources (app/src/tv/res/values/strings.xml) so the TV UI can be localized consistently with the rest of the app.

Comment on lines +193 to +195
} ?: ProxyEndpoint(
id = 0,
proxyName = "custom-${kind.name.lowercase()}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · high]
When existing is null (first-time setup on a fresh install — the ProxyEndpoint table has no seeded rows and getCustomSocks5Endpoint() returns null), this branch builds ProxyEndpoint(id = 0, ...) and then updateCustomSocks5Proxy/updateCustomHttpProxy persist via Room @Update (ProxyEndpointDAO.update), which matches rows by primary key and silently does nothing when id = 0 doesn't exist. The endpoint is therefore never written to the database; it only lives in AppConfig's in-memory customSocks5Endpoint/customHttpEndpoint cache, so after an app restart the saved proxy is gone and the form loads empty. When creating a new endpoint, insert the row first (e.g., proxyEndpointRepository.insert(ep) and capture the generated id into ep.id) before calling the updateCustom* method.

Comment on lines +226 to +228
Button(onClick = {
scope.launch(Dispatchers.IO) {
val type = when (kind) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · low]
There is no in-flight guard for Save/Disable: rapid double-taps launch concurrent coroutines that write the same endpoint and toggle the proxy state. Depending on completion order, the UI enabled flag and the persisted proxy type/provider can diverge (e.g., Save immediately followed by Disable can leave persistentState enabled while the UI shows disabled, or vice versa). Consider a saving/busy state that disables the buttons while an operation is in flight.

@Composable
fun ProxyScreen(navController: NavController) {
val appConfig = koinInject<AppConfig>()
val scope = rememberCoroutineScope()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
scope is created via rememberCoroutineScope() but never referenced anywhere else in this composable — dead code. Every call site uses the dedicated LaunchedEffect/produceState scopes instead, so this creates an unnecessary coroutine scope (and a compiler warning). Remove it unless it's needed by a planned follow-up.

Comment on lines +97 to +102
LaunchedEffect(Unit) {
withContext(Dispatchers.IO) {
WireguardManager.load(false)
}
reloadKey++
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
The IO work in LaunchedEffect and both produceState blocks is unguarded. WireguardManager.load() performs DB reads (db.getWgConfigs()) and file migration that can throw (e.g. SQLite/IO exceptions); if it does, the exception escapes the LaunchedEffect coroutine and crashes the app, and reloadKey++ never runs. Wrap the load in runCatching/try-catch, log failures, and consider a retry/error state so the screen degrades gracefully instead of crashing on first visit.

Suggestion:

Suggested change
LaunchedEffect(Unit) {
withContext(Dispatchers.IO) {
WireguardManager.load(false)
}
reloadKey++
}
LaunchedEffect(Unit) {
withContext(Dispatchers.IO) {
runCatching { WireguardManager.load(false) }
.onFailure { Logger.e("ProxyScreen", "wg load failed", it) }
}
reloadKey++
}

Comment on lines +118 to +119
socks5 = appConfig.getSocks5ProxyDetails()?.let { "${it.proxyIP}:${it.proxyPort}" },
http = appConfig.getHttpProxyDetails()?.let { "${it.proxyIP}:${it.proxyPort}" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
ProxyEndpoint.proxyIP is declared as String? (nullable), so a saved proxy endpoint whose IP is null will render the literal string "null:0" as the card value instead of falling back to "Not configured", misleading the user about the actual proxy state. Build the display string with a nested safe-call so a null IP is handled: e.g. appConfig.getSocks5ProxyDetails()?.let { it.proxyIP?.let { ip -> "$ip:${it.proxyPort}" } }.

Suggestion:

Suggested change
socks5 = appConfig.getSocks5ProxyDetails()?.let { "${it.proxyIP}:${it.proxyPort}" },
http = appConfig.getHttpProxyDetails()?.let { "${it.proxyIP}:${it.proxyPort}" },
socks5 = appConfig.getSocks5ProxyDetails()?.let { it.proxyIP?.let { ip -> "$ip:${it.proxyPort}" } },
http = appConfig.getHttpProxyDetails()?.let { it.proxyIP?.let { ip -> "$ip:${it.proxyPort}" } },

Comment on lines +117 to +119
if (mapping == null) {
Text(
text = "Tunnel not found.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[other · low]
mapping starts with initialValue = null while produceState loads asynchronously, and the LaunchedEffect's WireguardManager.load(false) runs concurrently, so on the first frame even a valid tunnel briefly renders the misleading 'Tunnel not found.' error before data arrives. Consider showing a loading indicator (or withholding the error text) until the load has settled, and only show 'Tunnel not found.' once loading is actually complete.

Comment on lines +133 to +138
onCheckedChange = { v ->
scope.launch(Dispatchers.IO) {
if (v) WireguardManager.enableConfig(m) else WireguardManager.disableConfig(m)
withContext(Dispatchers.Main) { reloadKey++ }
}
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
All five toggle handlers call WireguardManager's enable/disable/update methods inside scope.launch(Dispatchers.IO) with no try/catch. These methods perform Room DB writes and VpnController proxy updates that can throw (e.g. VPN service not ready), and an uncaught exception in a Compose-launched coroutine propagates to the app's uncaught-exception handler and crashes the TV app. The phone app's WgConfigDetailActivity guards the same calls with preconditions (hasTunnel, canEnableProxy) and try/catch + toast fallbacks. Additionally, these manager methods silently log-and-return when the config isn't found, so the unconditional reloadKey++ re-reads unchanged state and the toggle reverts with no user feedback. Wrap each handler body in try/catch and refresh only on success, surfacing failures to the user (same applies to the LaunchedEffect load and both produceState producers).

Comment on lines +204 to +205
Text(
text = "Tap a peer for raw allowed-IPs. Editing peers requires a paste-friendly text-entry flow that will land alongside the WG import screen.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[other · low]
This footer copy promises 'Tap a peer for raw allowed-IPs', but PeerCard is a plain read-only Surface with no clickable/onClick/focus handling — the interaction doesn't exist (the KDoc above also states peer editing is deferred). This is misleading for TV users navigating with a D-pad. Either wire up a tap/focus action for the peer rows or reword the text to describe the read-only nature.


import android.content.ClipboardManager
import android.content.Context
import android.content.Intent

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[other · low]
android.content.Intent and androidx.compose.runtime.LaunchedEffect are imported but never used in this file. Remove them to avoid dead code.

Comment on lines +98 to +100
scope.launch(Dispatchers.IO) {
error = null
try {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
error = null writes Compose snapshot state on Dispatchers.IO before any withContext(Dispatchers.Main) in this coroutine. Compose state should be mutated on the main thread; writing from a background thread can race with UI reads and produce stale/incorrect UI. Move the reset into the Main block (or set it before launching) along with the other state updates.

Suggestion:

Suggested change
scope.launch(Dispatchers.IO) {
error = null
try {
scope.launch(Dispatchers.IO) {
try {
val text = readUri(context, uri)
withContext(Dispatchers.Main) {
error = null
configText = text
}

Comment on lines +234 to +235
context.contentResolver.openInputStream(uri).use { stream ->
return BufferedReader(InputStreamReader(stream)).readText()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
ContentResolver.openInputStream(uri) returns a nullable stream. If the document is missing, unreadable, or the permission was revoked, this is null and InputStreamReader(null) throws an NPE (only surfaced later as a cryptic error). The rest of the project handles this explicitly (e.g., TunnelImporter.kt: openInputStream(uri) ?: throw IllegalArgumentException(...)). Add a null check to produce a clean error message instead of an NPE.

Suggestion:

Suggested change
context.contentResolver.openInputStream(uri).use { stream ->
return BufferedReader(InputStreamReader(stream)).readText()
val stream = context.contentResolver.openInputStream(uri)
?: throw IllegalArgumentException("Could not open file")
return stream.use { BufferedReader(InputStreamReader(it)).readText() }


private fun readUri(context: Context, uri: Uri): String {
context.contentResolver.openInputStream(uri).use { stream ->
return BufferedReader(InputStreamReader(stream)).readText()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · low]
InputStreamReader(stream) decodes the file using the platform default charset, which varies across devices/regions. WireGuard configs are typically UTF-8; the project already uses StandardCharsets.UTF_8 for this in TunnelImporter.kt. Specify the charset explicitly for deterministic parsing.

Suggestion:

Suggested change
return BufferedReader(InputStreamReader(stream)).readText()
return BufferedReader(InputStreamReader(stream, StandardCharsets.UTF_8)).readText()

Comment on lines +96 to +99
scope.launch(Dispatchers.IO) {
applyDomainStatus(row, nextDomainStatus(row.status))
withContext(Dispatchers.Main) { reloadKey++ }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
Rapid repeated clicks (common on TV remotes) can skip state transitions. row.status is captured at composition time, and nextDomainStatus is computed from that stale value each time onSelect fires. If the user presses twice before reloadKey++ triggers a recomposition, both clicks compute the same next from the old status (e.g. NONE→BLOCK twice instead of NONE→BLOCK→TRUST), and the second write also overwrites with the same status. Consider guarding with an in-flight flag (e.g. a var applying by remember per row, or disable the card while updating) so the second click waits for the refreshed status.

Comment on lines +218 to +222
private fun DomainRuleRow.toEntity(): CustomDomain = CustomDomain(
domain,
uid,
ips,
type.id,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
toEntity() constructs CustomDomain with ten positional arguments, which is hard to read and fragile: if the entity constructor parameters are ever reordered, fields such as status/type would be silently mapped to the wrong properties. Using named arguments (matching toRow()) makes the mapping self-documenting and resilient to refactoring.

Comment on lines +231 to +232
private suspend fun applyDomainStatus(row: DomainRuleRow, next: DomainRulesManager.Status) {
val customDomain = row.toEntity()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · low]
The coroutine launched from onSelect calls applyDomainStatus without any exception handling. If the Room insert or DomainRulesManager helper throws (e.g. a transient SQLite failure), the exception propagates uncaught from a rememberCoroutineScope, which can crash the app and leaves reloadKey unchanged so the UI never refreshes. Wrap the update in runCatching (or try/catch) and log/report the failure so the screen stays usable.

Comment on lines +240 to +241
private fun nextDomainStatus(status: DomainRulesManager.Status): DomainRulesManager.Status = when (status) {
DomainRulesManager.Status.NONE -> DomainRulesManager.Status.BLOCK

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
The status→label/color mapping is duplicated in DomainStatusPill, statusLabel, and nextDomainStatus, and the transition order is hard-coded separately in nextDomainStatus. When a new DomainRulesManager.Status value is added, all three places must be updated consistently or they will diverge. Consider centralizing label, color, and next-state as extension properties/functions on DomainRulesManager.Status (e.g. Status.label, Status.next) and reuse them in all three spots.

Comment on lines +224 to +229
private fun nextIpStatus(status: IpRulesManager.IpRuleStatus): IpRulesManager.IpRuleStatus = when (status) {
IpRulesManager.IpRuleStatus.NONE -> IpRulesManager.IpRuleStatus.BLOCK
IpRulesManager.IpRuleStatus.BLOCK -> IpRulesManager.IpRuleStatus.BYPASS_UNIVERSAL
IpRulesManager.IpRuleStatus.BYPASS_UNIVERSAL,
IpRulesManager.IpRuleStatus.TRUST -> IpRulesManager.IpRuleStatus.NONE
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[other · low]
The status cycle never reaches TRUST (NONE→BLOCK→BYPASS_UNIVERSAL→NONE), yet any existing TRUST rule is silently downgraded to NONE on a single OK press, with no confirmation or way to cycle back to Trust. Note the sibling TV screen (CustomDomainRulesList) includes TRUST in its cycle, and the main app (CustomIpRulesBtmSheet) exposes TRUST as a selectable state for IP rules. Please confirm whether TRUST should be part of the universal-rule cycle; if it is intentionally excluded, the TRUST→NONE mapping should be surfaced with a clearer confirm affordance rather than a plain status toggle.

@OptIn(ExperimentalTvMaterial3Api::class)
@Composable
fun RulesScreen() {
var activeTab by remember { mutableStateOf(RulesTab.DOMAINS) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · low]
The active tab is held by remember { mutableStateOf(RulesTab.DOMAINS) }, so the user's Domains/IPs selection is lost on activity recreation (configuration change or process death) and always resets to the default tab. LogsScreen in this module already preserves its tab via rememberSaveable; use the same here (plus the androidx.compose.runtime.saveable.rememberSaveable import) to keep the selection across recreation.

Suggestion:

Suggested change
var activeTab by remember { mutableStateOf(RulesTab.DOMAINS) }
var activeTab by rememberSaveable { mutableStateOf(RulesTab.DOMAINS) }

Comment on lines +48 to +51
TvScreenScaffold(
title = "Rules",
subtitle = "Cycle the universal custom domain and IP rules already stored on device.",
) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
All user-facing strings in this screen (Rules, Domains, IPs, Domain rules, IP rules, Scope, Universal only, the subtitle, and the banner copy) are hardcoded Kotlin literals, which prevents localization and triggers HardcodedText lint. Although the rest of the TV module currently uses literals too, app/src/tv/res/values/strings.xml already exists for the tv flavor — consider moving these strings into resources and referencing them via stringResource for consistency and future translation support.

@Composable
fun AntiCensorshipScreen() {
val persistentState = koinInject<PersistentState>()
val ioScope = remember { CoroutineScope(Dispatchers.IO) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
remember { CoroutineScope(Dispatchers.IO) } creates a manually-managed scope that is never cancelled when this screen leaves composition, and the launched writes have no exception handling — if the intPref commit throws (e.g. disk I/O failure), the exception becomes an uncaught coroutine failure that can crash the app. Other TV screens use the lifecycle-aware rememberCoroutineScope() instead. If writes must be allowed to complete even after navigating away, at least wrap the persistence call in runCatching/try-catch; otherwise prefer rememberCoroutineScope() and launch with Dispatchers.IO.

Comment on lines +104 to +105
dialStrategy = opt.mode
ioScope.launch { persistentState.dialStrategy = opt.mode }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
Each row's onClick launches an independent ioScope.launch { persistentState.dialStrategy = opt.mode } on the multi-threaded Dispatchers.IO. If the user activates two options in quick succession, the two coroutines can run concurrently on different threads, and since the intPref commit order isn't guaranteed, the persisted value may end up different from the last UI selection — after a restart the app can restore a stale strategy. Consider serializing the writes, e.g. persist from a LaunchedEffect(dialStrategy) keyed on the current state, or use a single-writer dispatcher such as Dispatchers.IO.limitedParallelism(1). The same applies to the retry-strategy write below.

Comment on lines +143 to +144
* Mirrors `AntiCensorshipActivity.DialStrategies` for the four modes
* we surface on TV. Kept as raw `Settings.Split*` ints so that an

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[documentation · low]
Two documentation inaccuracies here: (1) The KDoc says the list mirrors "the four modes we surface on TV", but DialStrategyOptions actually contains five entries. (2) The class-level note that "DESYNC requires firestack ≥ 4.12" conflates the bundled firestack library with the upstream guard: AntiCensorshipActivity checks the device's Linux kernel version via Utilities.isOsVersionAbove412(...) (it reads System.getProperty("os.version") and compares against "4.12"), not the firestack version. Please correct the count and the version rationale so maintainers aren't misled about why DESYNC is exposed unconditionally on TV.

Comment on lines +68 to +72
val pauseObserver = remember { VpnController.getPauseCountDownObserver() }
val remaining by rememberAsImmutableState(
liveData = pauseObserver ?: MutableLiveData(0L),
initial = 0L,
) { it ?: 0L }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · high]
The pause observer is captured only once via remember { ... }, but VpnController.getPauseCountDownObserver() returns rvpn?.getPauseCountDownObserver(), which is null whenever the VPN service isn't bound (e.g. opening this settings sub-screen before/without starting the VPN). When it's null at first composition:

  1. The fallback MutableLiveData(0L) is observed forever — even after the VPN starts, the screen keeps watching the dummy LiveData and never sees the real countdown, so after pressing a preset the UI never flips to the countdown/Resume state until the route is left and re-entered.
  2. MutableLiveData(0L) is constructed inline on every recomposition, and rememberAsImmutableState re-keys produceState on the LiveData reference — each recomposition tears down and re-creates the observer (observer churn).

Since PauseTimer is already imported and its getPauseCountDownObserver() returns the same non-null singleton (pauseCountDownTimer) that VpnController delegates to, observe that directly to keep a stable, always-available observer.

Suggestion:

Suggested change
val pauseObserver = remember { VpnController.getPauseCountDownObserver() }
val remaining by rememberAsImmutableState(
liveData = pauseObserver ?: MutableLiveData(0L),
initial = 0L,
) { it ?: 0L }
val pauseObserver = remember { PauseTimer.getPauseCountDownObserver() }
val remaining by rememberAsImmutableState(
liveData = pauseObserver,
initial = 0L,
) { it ?: 0L }

Comment on lines +116 to +119
onClick = {
VpnController.pauseApp()
PauseTimer.start(p.durationMs)
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · high]
This starts the pause timer twice. VpnController.pauseApp() already calls BraveVPNService.pauseApp()startPauseTimer()PauseTimer.start(PauseTimer.DEFAULT_PAUSE_TIME_MS). The subsequent PauseTimer.start(p.durationMs) launches a second, concurrent countdown coroutine. Both loops decrement the shared PauseTimer.countdownMs AtomicLong every second, so the countdown effectively ticks at 2× speed and the pause expires after about half the selected duration; both loops' finally blocks then call VpnController.resumeApp(). Conversely, if the VPN isn't running (rvpn null), pauseApp() is a no-op while the timer still starts, leaving UI and VPN state out of sync. Ensure PauseTimer.start is invoked exactly once for a pause (e.g. drop the direct call and rely on the timer started by VpnController.pauseApp(), or add a duration parameter to VpnController.pauseApp instead of starting a second loop).

)
Spacer(Modifier.height(8.dp))
Text(
text = String.format("%02d:%02d", minutes, seconds),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[style · low]
All user-visible text in this screen is hardcoded ("Pause protection", "How long?", "Resume now", preset labels, etc.) rather than moved to the TV string resources — the new app/src/tv/res/values/strings.xml only defines app_name, so the TV UI cannot be localized. In addition, String.format without an explicit Locale uses the device default locale, which can render digits differently in some locales. Move the strings to resources and pass a fixed locale (e.g. Locale.US) or use padStart for the countdown.

Suggestion:

Suggested change
text = String.format("%02d:%02d", minutes, seconds),
text = String.format(Locale.US, "%02d:%02d", minutes, seconds),

Comment on lines +175 to +177
AdjustButton(label = "−1 min") {
PauseTimer.subtractDuration(PauseTimer.PAUSE_VPN_EXTRA_MILLIS)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
PauseTimer.subtractDuration is not clamped. When less than one minute remains, this drives the shared countdownMs negative; the running timer loop then exits and its finally block calls VpnController.resumeApp(), so pressing “−1 min” silently resumes the VPN immediately instead of trimming the countdown. Guard the action (or disable the button) when the remaining time is below PAUSE_VPN_EXTRA_MILLIS.

Suggestion:

Suggested change
AdjustButton(label = "−1 min") {
PauseTimer.subtractDuration(PauseTimer.PAUSE_VPN_EXTRA_MILLIS)
}
AdjustButton(label = "−1 min") {
if (remainingMs >= PauseTimer.PAUSE_VPN_EXTRA_MILLIS) {
PauseTimer.subtractDuration(PauseTimer.PAUSE_VPN_EXTRA_MILLIS)
}
}

import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.unit.dp
import androidx.navigation.NavController
import androidx.tv.material3.Button

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[style · low]
androidx.tv.material3.Button and androidx.compose.foundation.layout.PaddingValues are imported but never used in this file. Remove them to keep the imports clean.

val persistentState = koinInject<PersistentState>()
val appConfig = koinInject<AppConfig>()
val composeScope = rememberCoroutineScope()
val ioScope = remember { CoroutineScope(Dispatchers.IO) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
remember { CoroutineScope(Dispatchers.IO) } creates a standalone scope that is never cancelled when this composable leaves the composition, so a toggle write still in flight can outlive the screen (unstructured concurrency). composeScope from rememberCoroutineScope() already exists above and is automatically cancelled with the composition — prefer reusing it (e.g. composeScope.launch(Dispatchers.IO) { ... }), exactly as ProtectionModeSection does, instead of introducing a second unmanaged scope.

val ioScope = remember { CoroutineScope(Dispatchers.IO) }

TvScreenScaffold(
title = "Settings",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
All user-visible strings in this screen (title, subtitle, section headers, row titles/descriptions) are hardcoded in Kotlin, which prevents localization and will trigger HardcodedText lint. Since this change set adds app/src/tv/res/values/strings.xml, move these strings into resources and reference them with stringResource(...).

title = "Allow apps to bypass the VPN",
description = "Lets apps that explicitly opt out (e.g. some VPN clients) skip Rethink.",
leadingIcon = Icons.AutoMirrored.Filled.AltRoute,
checked = remember { persistentState.allowBypass },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
checked = remember { persistentState.allowBypass } freezes the value at first composition. The remember has no keys, and PersistentState properties are plain booleanPref-backed Kotlin vars (not Compose state), so this never re-reads the source and never triggers recomposition. SettingToggleRow only mirrors this frozen checked into its local remember(checked) state, so if the underlying setting changes while the screen is visible (restore, another destination, or tunnel-service adjustments), the toggle silently displays the stale snapshot until the screen is recreated. This applies to all ten toggle rows below. Prefer binding checked to an observable source of truth (e.g. the existing LiveData/Flow) so the rows resync on recomposition.


var window by remember { mutableStateOf(StatsWindow.ONE_HOUR) }
var tab by remember { mutableStateOf(StatsTab.TOP_APPS) }
val to = System.currentTimeMillis() - window.millis

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[performance · medium]
to is recomputed from System.currentTimeMillis() on every recomposition and then used as a key in remember(tab, to). Because to is not remembered, any incidental recomposition that lands on a new millisecond produces a different key, tearing down and recreating the Pager flow — which restarts the list and re-runs the expensive aggregate SUM queries over ConnectionTracker/DnsLogs. Only recompute it when the window actually changes, and note the screen also has no periodic refresh so data goes stale while it stays open.

Suggestion:

Suggested change
val to = System.currentTimeMillis() - window.millis
val to = remember(window) { System.currentTimeMillis() - window.millis }

Comment on lines +211 to +214
when {
isLoading -> CenterLabel("Loading…")
isEmpty -> CenterLabel("No data for this window yet.")
else -> LazyColumn(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · medium]
LoadState.Error is not handled here. When the initial refresh fails, both isLoading and isEmpty are false, so the else branch renders a LazyColumn with itemCount == 0 — a blank screen with no error message or retry option. Consider handling the error state explicitly so users can distinguish a transient DB failure from an actually-empty window.

Suggestion:

Suggested change
when {
isLoading -> CenterLabel("Loading…")
isEmpty -> CenterLabel("No data for this window yet.")
else -> LazyColumn(
when {
isLoading -> CenterLabel("Loading…")
isEmpty -> CenterLabel("No data for this window yet.")
items.loadState.refresh is androidx.paging.LoadState.Error -> CenterLabel("Couldn't load stats. Try another tab or window.")
else -> LazyColumn(

}

private fun secondaryLabel(tab: StatsTab, row: AppConnection): String = when (tab) {
StatsTab.TOP_APPS -> formatBytesOrCount(row)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[style · low]
For TOP_APPS the same byte total is shown twice: formatBytesOrCount in the secondary line renders "1.5 GB • 123 req" while trailing shows "1.5 GB" again, and their fallbacks diverge ("123 req" vs "123"). Consider showing only the request count in the secondary line and the formatted bytes in the trailing column (or vice versa) to avoid redundant/divergent formatting.

v /= 1024.0
i++
}
return String.format("%.1f %s", v, units[i])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · low]
String.format without an explicit Locale uses the default locale, so in locales with comma decimal separators the output becomes e.g. "1,5 GB" while the rest of the UI uses dot decimals. Use Locale.US (or the locale the rest of the app formats with) for consistent display.

Suggestion:

Suggested change
return String.format("%.1f %s", v, units[i])
return String.format(Locale.US, "%.1f %s", v, units[i])

surfaces "Rethink TV" rather than the upstream phone-app label.
-->
<resources>
<string name="app_name" translatable="false">Rethink TV</string>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · low]
This override only replaces the default-locale app_name. The main source set also defines translated app_name entries in app/src/main/res/values-*/strings.xml (e.g. values-ar, values-de, values-ru), which will still win on non-English device locales. So on an Arabic/French/… device the Android TV launcher will show the localized upstream name (e.g. "إعادة التفكير") instead of "Rethink TV", contradicting the comment's intent and the translatable="false" marker. If the TV label must always be "Rethink TV", the tv source set should also override app_name for those locale configurations (or the localized app_name entries should be excluded for this flavor).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants