Skip to content

Navigation 3, dependency injection, and module structure for the phone client - #884

Draft
moehamade wants to merge 15 commits into
permissionlesstech:mainfrom
moehamade:feat/nav3-di-groundwork
Draft

Navigation 3, dependency injection, and module structure for the phone client#884
moehamade wants to merge 15 commits into
permissionlesstech:mainfrom
moehamade:feat/nav3-di-groundwork

Conversation

@moehamade

@moehamade moehamade commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Description

Draft — opened early for direction, not for merge. It is one branch so the pieces can be read together; if the direction is agreed I will split it into the smaller PRs listed at the bottom.

Introduces dependency injection, Gradle convention plugins, a first shared module, and a Navigation 3 skeleton to the phone client. Design doc is in docs/navigation3-modularization-plan.md, including the parts of it that turned out to be wrong.

The nine chat overlays are deliberately not converted yet. That is the large, risky part and it belongs in its own reviewable steps.

Why

The phone client has no navigation layer (navigation-compose is declared but unused — nothing calls NavHost or rememberNavController), no DI, and 273 files in one module. Navigation today is a when (onboardingState) over 8 states, nine overlays driven by independent booleans, and a hand-written back priority chain.

What is here

  • Hilt + KSP, with the four ViewModels that are actually obtained from a ViewModelProvider injected, and the hand-rolled ViewModelProvider.Factory deleted.
  • build-logic convention plugins (bitchat.android.library, .library.compose, .hilt).
  • :core:domain — the two model files the UI actually imports. A library module cannot depend on an application module, so these have to leave :app before any feature module can compile.
  • :core:navigation — Navigator, an @ActivityRetainedScoped back stack, the EntryProviderInstaller multibinding, and the NavDisplay host.
  • Onboarding and chat as routes, replacing the when and the OnBackPressedCallback that was being registered inside a composable body on every recomposition.

Three things worth a maintainer's eye

1. Reproducibility. Adding KSP means codegen inside the byte-for-byte pipeline. Two :app:bundleRelease runs under the flags from build-release.sh produced identical bytes locally, so the processors do not introduce ordering nondeterminism. That is a proxy, not the guarantee — the canonical check pins JDK 21.0.11+10 and Linux SDK archives and only runs in the container, so CI is the real test. Verification metadata gains components in three reviewed batches and removes none.

2. :wear. syncSharedAppSources gains a second from(...) block reusing the same include and exclude lists, so the files moved into :core:domain keep being mirrored. AGENTS.md is updated to match. Verified :wear's sharedSrc still resolves all nine model files and still excludes FileSharingManager.

This also caps the DI work: 10 of the stateful singletons are compiled into :wear, which has no Hilt and no KSP, so they can be provided into the graph but never converted to @Inject constructor classes.

3. gradle.properties metaspace 512m → 1g. KSP and the Dagger processors exhaust the old ceiling. The failure does not name its cause — it surfaces as ClassLoader.defineClass OOM in an unrelated task, and if it lands during --write-verification-metadata it can truncate gradle/verification-metadata.xml to zero bytes. JVM args are not build inputs, so this does not affect reproducibility.

Two design calls that differ from the doc

Onboarding is one destination, not eight. Its steps are driven by permission results and adapter state changes, not by the user navigating, and going back a step has never been supported. Per-step routes would invent a history that does not exist. Only the crossing into chat is a navigation event, and it uses resetTo so Back cannot re-enter onboarding.

MeshService stays in :app. It looked like a cheap extraction — it is already an interface — but it returns noise.NoiseSession.NoiseSessionState and references three types from its own package, so moving it would pull the Noise/crypto layer into :core:domain. Feature modules should not depend on the transport API anyway.

Verification

:app:assembleDebug, :app:assembleRelease (R8), :wear:assembleDebug, 591 unit tests, and manual checks on an emulator: launch renders onboarding, granting permissions advances without touching the back stack, chat is reached, Back from chat exits rather than re-entering onboarding, and rotation preserves the stack.

The emulator pass was not optional — seeding the back stack from a LaunchedEffect compiled and passed every test while crashing on launch, because NavDisplay rejects an empty back stack and effects do not run until after the first composition.

If the direction is agreed, this splits into

  1. build: KSP + Hilt, lock state and verification metadata
  2. build: metaspace
  3. feat(di): inject the ViewModels
  4. refactor: convention plugins + :core:domain + the :wear sync change
  5. feat(nav): :core:navigation
  6. feat(nav): onboarding and chat as routes
  7. refactor(di): bridge the singletons that injectable code reached statically

Checklist

  • I have read the contribution guidelines
  • I have performed a self-review of my code
  • I have mentioned the corresponding issue — no issue yet. Opened as a draft to agree direction first; happy to open a tracking issue if you would rather have one.
  • If it is a core feature, I have added automated tests — no behaviour is added, and the existing 591 tests cover the refactored paths. Back-order tests are owed when the overlays become routes.

moehamade and others added 15 commits August 15, 2026 01:23
@moehamade @claude
docs: design for Navigation 3, dependency injection, and module struc…
8eb8c6b 
…ture

Documents the current state of the phone client (no navigation layer, no DI,
273 files in one module), the target module layout, the route model, and a
sequenced set of changes to get there.

Records two findings that shape the approach: ChatState rather than
ChatViewModel is the coupling point, since the ten managers already exist as
separate collaborators sharing one state object; and wear/build.gradle.kts
file-copies source out of app/src/main/java by path glob, which constrains
what can move out of :app.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
build: add KSP and Hilt to the phone client
a97675b 
Adds the KSP and Hilt Gradle plugins and the Hilt runtime/compiler
dependencies to :app, with the lock state and verification metadata
regenerated per docs/reproducible-builds.md. No source changes; nothing
is injected yet.

KSP versions independently of Kotlin as of 2.3.0. Earlier releases used a
<kotlin>-<ksp> scheme requiring an exact match to the Kotlin version, so
the catalog records the reason the two versions no longer track.

Verification metadata gains 36 components and removes none. Reviewed:

- androidx.compose.runtime:runtime:1.8.2 and
  androidx.annotation:annotation-experimental:1.3.1 are .module metadata
  only, with no jar or aar. They are resolution metadata, not artifacts,
  and do not downgrade what the Compose BOM selects.
- kotlin-stdlib:2.3.20 carries a jar but the lock state confines it to
  kspPluginClasspath, so it never reaches a runtime classpath.
- com.google.dagger:dagger was already on the runtime classpath at 2.59
  transitively; it moves to 2.60.1.
- The remainder are the Dagger compiler's own build-time dependencies
  (guava, kotlinpoet, javapoet, google-java-format, error_prone).

wear/gradle.lockfile and settings-gradle.lockfile are unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
feat(di): inject the phone client's ViewModels with Hilt
33d6b36 
Replaces the hand-rolled ViewModelProvider.Factory in MainActivity with
constructor injection. No singleton is converted and no behaviour changes;
this only moves ViewModel construction into the graph so feature modules
can obtain their own ViewModels later.

MeshModule bridges the existing process-wide mesh instances into the graph.
Its bindings are deliberately unscoped: MeshServiceHolder is the source of
truth, it is shared with the foreground service, and it supports replacing
the mesh service after a panic clear. A @singleton binding would pin the
first instance for the process lifetime and hand out a stale service after
a replacement.

ChatViewModel still resolves to a single activity-scoped instance, so the
unifiedMeshService.delegate assignment continues to reference the same
object it did before.

ApkDownloadViewModel is injected through its secondary constructor. Its
collaborators are internal types, and routing those through Dagger's
generated Java buys nothing; the primary constructor stays available so
tests can keep substituting fakes.

Verified: :app:assembleDebug, :wear:assembleDebug, and 591 unit tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
build: raise Gradle metaspace to 1g
92fc397 
KSP and the Dagger/Hilt annotation processors load enough additional classes
into the daemon that a combined testDebugUnitTest + lintDebug run exhausts the
previous 512m ceiling.

The failure is worth describing because it does not name its cause: it surfaces
as an OutOfMemoryError inside ClassLoader.defineClass in whichever task happens
to be loading classes at the time, including lint on an unrelated module. When
it lands during a --write-verification-metadata run it can also truncate
gradle/verification-metadata.xml to zero bytes, which then fails the next build
with "Unable to read dependency verification metadata: Premature end of file".

JVM arguments are not build inputs, so this does not affect reproducibility.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
refactor: add Gradle convention plugins and extract :core:domain
2bac618 
Introduces the module infrastructure the feature modules will need. Gradle
forbids a library module depending on an application module, so the domain
types the UI layer uses have to leave :app before any :feature:* module can
compile against them.

build-logic is an included build providing bitchat.android.library and
bitchat.android.library.compose. Two notes for anyone extending them:

- AGP 9 applies the Kotlin Android plugin itself, so the convention plugins
  must not apply org.jetbrains.kotlin.android. That is why the version catalog
  still has no entry for it.
- AGP 9's CommonExtension is no longer generic and exposes getters only. The
  block-syntax overloads (defaultConfig, compileOptions, lint, buildFeatures)
  are declared on the concrete LibraryExtension, so the plugins configure that.

The compose convention plugin sets includeComposeMappingFile to false to match
:app and :wear. Kotlin 2.4.10's optional Compose group-key mapping depends on
unspecified class-file iteration order, and a library omitting it would
reintroduce the nondeterminism the release pipeline byte-compares against.

:core:domain holds BitchatMessage.kt and BitchatFilePacket.kt, which between
them declare every model type the UI imports and depend only on Parcelable,
Gson and java.nio. The design called for moving model/** plus the MeshService
interface; neither turned out to be possible:

- Four of model/'s ten files reach into protocol, sync or features.file, so
  moving the package entire would drag protocol/ and sync/ out of :app.
- MeshService references MeshDelegate, PeerInfo and PrivateMediaPreparation
  from its own package and returns noise.NoiseSession.NoiseSessionState, so
  extracting it would pull the Noise/crypto layer along. It stays in :app,
  which is the better boundary regardless: feature modules should not depend
  on the transport API.

wear/build.gradle.kts gains a second from(...) block reusing the same include
and exclude lists, so the moved files keep being mirrored into sharedSrc and
sharedSourceIncludes keeps its shape. Verified: wear's sharedSrc still contains
all nine model files and still excludes FileSharingManager.

Moving BitchatMessage across a module boundary broke six smart casts, because
Kotlin will not smart-cast a property declared in another module. Each is fixed
by reading into a local val rather than asserting non-null, which preserves the
null-safety the smart cast provided.

Verification metadata gains 39 build-time components and removes none, all from
the kotlin-dsl plugin and the Kotlin Gradle plugin used to compile build-logic.
app/gradle.lockfile is unchanged, since :core:domain is a project dependency.

Verified: :app:assembleDebug, :wear:assembleDebug, and 591 unit tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
style: group the new dagger imports with their neighbours
12b4b2a 
The Hilt annotations landed mid-block in two files, splitting the androidx and
kotlinx groups those files otherwise keep. No behaviour change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
docs: record what the design got wrong once it met the build
2ca497b 
Updates the plan against what actually landed. The substantive corrections:

- model/** is not extractable wholesale, and MeshService cannot move to
  :core:domain without dragging the Noise/crypto layer with it.
- :core:designsystem is deferred, with the resource and visibility costs
  written down so the decision can be re-taken rather than rediscovered.
- The Hilt/KSP toolchain risk and the :wear regression risk are closed.
- Codegen does not break build determinism: two :app:bundleRelease runs under
  the flags from build-release.sh produced identical bytes. Recorded as the
  proxy it is, since the canonical check pins a JDK and Linux SDK archives and
  only runs in the container.

Adds a toolchain section for the remaining modules covering the AGP 9 DSL
changes, Kotlin's refusal to smart-cast across a module boundary, the metaspace
ceiling and how its failure can truncate the verification metadata, and the
Compose mapping-file setting new library modules must inherit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
feat(nav): add :core:navigation with a single-stack Navigator
350825a 
Introduces the Navigation 3 foundation: a Navigator interface, an
@ActivityRetainedScoped AppNavigator over a SnapshotStateList<NavKey>, the
EntryProviderInstaller multibinding type, and the NavDisplay host. Nothing
navigates yet; :app is unchanged.

Uses navigation3 1.1.6 rather than the 1.2.0 alpha line. 1.1.6 floors Compose
at 1.10.0, below the 1.11.4 the BOM already resolves, so it does not drag
Compose forward.

The back stack survives configuration changes because the navigator is held by
ActivityRetainedComponent, not because of rememberNavBackStack. That is a
deliberate trade: rememberNavBackStack would require every NavKey to be
@serializable and pull in kotlinx-serialization, and the only thing it buys
over the retained scope is restoration across process death, which nothing
needs yet — onboarding re-derives its state on launch. Keys are kept as plain
data objects so annotating them later is trivial.

Single-stack by design. The multi-back-stack pattern exists to serve bottom
navigation; this app has no tabs, so it would reduce to a map holding one key.

Also adds a bitchat.android.hilt convention plugin, applied alongside a module's
chosen base rather than including one, so a module picks plain-library or
Compose-library and adds injection to it.

API note for anyone following the published recipes: on 1.1.6 the entry scope
is EntryProviderScope, not EntryProviderBuilder, and the saveable decorator is
rememberSaveableStateHolderNavEntryDecorator. Both were confirmed against the
artifacts rather than the docs.

Verification metadata gains 41 components and removes none, all navigation3,
navigationevent, and transitive floor metadata.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
feat(nav): put onboarding and chat behind NavDisplay
4e52647 
Replaces the when (onboardingState) in MainActivity and the ad-hoc back
handling with a Navigation 3 back stack.

Onboarding is modelled as one destination, not eight. Its steps are driven by
permission results and adapter state changes rather than by the user
navigating, and the app has never supported going back from one step to the
previous one. Giving each step an entry would invent a history that does not
exist, so the state machine stays and only the crossing between onboarding and
chat is a navigation event. That crossing uses resetTo, so Back after
onboarding exits rather than re-entering it.

Removes the OnBackPressedCallback that was constructed and registered inside a
composable body, which added a new callback on every recomposition of that
branch. NavDisplay owns back now. It drives predictive back through
androidx.navigationevent, and adding a second handler over the same gesture can
detach the navigationevent input mid-gesture, so there must not be another one.

Chat still manages its overlays with booleans, so back has to consult
ChatViewModel first. That is the interceptBack parameter, which exists only
until those overlays become routes.

The back stack is seeded before setContent rather than from a LaunchedEffect.
Verified the hard way: NavDisplay rejects an empty back stack, and an effect
does not run until after the first composition, so the app crashed on launch
with "NavDisplay backstack cannot be empty".

Verified on an emulator: launch renders onboarding; granting permissions
advances to the battery step without touching the back stack; skipping reaches
chat; Back from chat exits to the launcher instead of returning to onboarding;
rotation preserves the stack through the retained scope with no empty-backstack
error. Plus :app:assembleDebug, :app:assembleRelease, :wear:assembleDebug and
591 unit tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
docs: correct the onboarding route model
15238b7 
The plan specified OnboardingRoute(step) over the 8 OnboardingState values.
That is wrong: onboarding is driven by permission results and adapter state
changes rather than user navigation, and the app has never supported going back
a step, so per-step entries would invent a history that does not exist. It is
one destination.

Records the two runtime constraints this surfaced — NavDisplay rejects an empty
back stack so it cannot be seeded from a LaunchedEffect, and seeding must be
idempotent because the retained scope already holds the stack across
configuration changes.

Also notes that removing the unused navigation-compose dependency is not a free
deletion, since :wear still uses Navigation 2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
docs: note the navigation skeleton in the status header
e610aae 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
refactor(di): inject the singletons that injectable code was reaching…
3304e00 
… statically

Bridges four process-wide singletons into the graph and removes the
getInstance(...) calls from ChatViewModel, MainActivity and
GeohashPickerActivity.

These stay getInstance(context) singletons rather than becoming
@Inject constructor classes. They cannot become classes: SeenMessageStore is
compiled into :wear through the shared-source sync, and :wear has no Hilt and
no KSP. Providing them keeps the object exactly as :wear sees it while letting
:app depend on the type instead of the static accessor, which is the part that
makes the consuming code testable.

Scope is deliberately "singletons that injectable code already reached
statically" rather than the whole inventory. Providing bindings nothing
consumes would be speculative, and the remaining call sites are in objects that
are not injectable themselves — converting those cascades.

Three were considered and left alone:

- MessageRouter takes the mesh service, which ChatViewModel replaces after a
  panic clear, so a cached binding would go stale.
- ContactDirectory is accessed through static functions rather than an
  instance, so there is nothing to inject without converting it first.
- DebugSettingsManager.getInstance() throws when uninitialised, which is why
  its call site wraps it in try/catch. A binding would move that failure into
  graph creation.

ConversationListPreferences is excluded for a different reason: it is an
internal class, and a @provides returning an internal Kotlin type runs into
name mangling in Dagger's generated Java.

Injected as dagger.Lazy because each was previously resolved at the point of
use. Resolving eagerly would construct them during ViewModel creation, earlier
than before.

GeohashPickerActivity becomes @androidentrypoint.

Verified on an emulator, since compile-time graph validation does not prove the
@androidentrypoint transform or field timing: the app launches, the location
channels sheet opens (so ChatViewModel constructs with the new bindings), and
the geohash picker opens from it and resumes with no injection failure. Note
its manifest sets exported=false, so `am start` is denied and only the in-app
path actually exercises it. Plus :app:assembleDebug, :app:assembleRelease,
:wear:assembleDebug and 591 unit tests.

No dependency changes, so the lock state and verification metadata are
untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
refactor(di): resolve MessageRouter through a Provider
54e56cc 
MessageRouter.getInstance does two things: it returns the process-wide router
and it re-points that router at the current mesh, refreshing both `mesh` and
the sender peer ID. The refresh lives inside the accessor.

So a cached binding would not hand back a stale object — it would skip the
refresh, leaving the router pointing at the mesh service that
recreateMeshServiceAfterPanic() replaced. The binding is therefore unscoped and
injected as Provider<MessageRouter>, so every use re-resolves and re-runs the
refresh, which is exactly what the getInstance calls did before.

This works because the MeshService binding is also unscoped and re-reads
MeshServiceHolder, and the panic path clears and repopulates that holder rather
than only updating ChatViewModel's own fields.

tryGetInstance() in the panic path is left alone on purpose: it clears the
outbox only if a router already exists, and injecting would construct one.

The deeper fix is for the mesh service to keep a stable identity and swap its
internals on panic instead of being replaced wholesale. That would also remove
the repeated delegate reassignments. It reaches into the mesh layer, which is
shared with :wear, so it is out of scope here and noted in the module.

Verified: build, 591 unit tests, :wear, and a launch on device with no missing
binding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
refactor(di): inject ConversationListPreferences and DebugSettingsMan…
fd58b35 
…ager

Both were excluded from the previous change. Re-examined, both were fixable and
one of the stated reasons was simply wrong.

DebugSettingsManager.getInstance() does not throw when uninitialised, contrary
to what the earlier commit message said. It lazily constructs a no-argument
instance under a lock, so there is no initialise-first contract and a plain
@singleton binding is safe. The try/catch wrapping it in ChatViewModel was
vestigial; the field it guarded turned out to have no remaining usages at all
and is deleted.

ConversationListPreferences was excluded because a @provides returning an
internal Kotlin type runs into name mangling in Dagger's generated Java. Making
the class public resolves that and costs nothing: :app is an application module
with no consumers, so internal and public have the same reach there.

Verified: build, 591 unit tests, :wear, and a launch on device.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
@moehamade @claude
build: pin the Groovy and Ant artifacts that IDE sync resolves
4c5cfdc 
Adding build-logic made Android Studio fail sync with "Dependency verification
failed for org.apache.groovy:groovy:4.0.32". Syncing a composite build
materialises localGroovy()/gradleApi() to resolve Kotlin script definitions,
and those artifacts were never pinned.

Regenerating does not fix this. --write-verification-metadata records only what
that invocation resolves, and no command-line invocation resolves these:
compileClasspath, runtimeClasspath, testRuntimeClasspath, kotlinScriptDef and a
forced clean compile of build-logic were all checked, and a full regeneration
run produced an empty diff. Only IDE sync reaches them, so the entries have to
be written by hand.

Pinned rather than trusted. The file already trusts IDE-only artifacts by rule
for javadoc and sources, and these would arguably qualify, but pinning a
verified checksum is strictly stronger than trusting a pattern, and relaxing
verification on this project should be a deliberate decision rather than a fix
for a sync error.

Scoped to the Groovy modules the Gradle 9.6.1 distribution actually ships
(eleven plus the BOM) rather than everything published at 4.0.32, plus the Ant
artifacts groovy-ant declares. Ant is pinned at both the version groovy-ant
declares and the version the distribution carries, since resolution can reach
either. Ant publishes no Gradle module metadata, so those entries are POM only.

Every checksum was fetched from repo1.maven.org. The one artifact that had
already been downloaded, groovy-4.0.32.module, matched the local cache
byte-for-byte, which is what makes this the benign case: a new component that
was never pinned, not a changed checksum on a pinned one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaZivRfLu7FdKjzVwwRjcc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@moehamade