[#617][Part 1/3] Migrate to navigation 3 - Template compose project#630
Open
[#617][Part 1/3] Migrate to navigation 3 - Template compose project#630
Conversation
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Failure
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the BaseDestination/NavHost approach with a Navigator/back-stack system, adds Navigation3 and Kotlinx serialization, introduces deep-link parsing/decoding utilities, an in-composition ResultEventBus, DI changes supplying Navigator, MainActivity deep-link handling and animations, and related test and dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as MainActivity
participant Matcher as DeepLinkMatcher
participant Decoder as KeyDecoder
participant Navigator as Navigator
participant BackStack as SnapshotStateList
Activity->>Matcher: Matcher.match(DeepLinkRequest, DeepLinkPattern)
Matcher-->>Activity: DeepLinkMatchResult(serializer, args)
Activity->>Decoder: KeyDecoder.decode(args, serializer)
Decoder-->>Activity: typedDestination
Activity->>Navigator: navigator.goTo(typedDestination)
Navigator->>BackStack: append(destination)
sequenceDiagram
participant Screen as HomeScreen
participant ViewModel as HomeViewModel
participant Navigator as Navigator
participant BackStack as SnapshotStateList
Screen->>ViewModel: user action -> emit navigation intent
ViewModel->>Navigator: navigator.goTo(destination)
Navigator->>BackStack: append(destination)
BackStack-->>Screen: recomposition with new top destination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
eeeeaa
changed the base branch from
chore/#617-migrate-to-navigation-3
to
develop
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
changed the title
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3-template-compose
branch
from
c8e2d19 to
d43951a
Compare
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
changed the title
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
changed the title
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Failure
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3-template-compose
branch
from
d43951a to
b9181b3
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Kover report for template-compose:🧛 Template - Compose Unit Tests Code Coverage:
|
| File | Coverage |
|---|---|
AppNavigation.kt |
90.91% |
BaseViewModel.kt |
85.71% |
ComponentActivityExt.kt |
100.00% |
DeepLinkMatcher.kt |
0.00% |
DeepLinkPattern.kt |
0.00% |
DeepLinkRequest.kt |
0.00% |
Exceptions.kt |
100.00% |
HomeScreen.kt |
92.00% |
HomeScreenNavEntry.kt |
100.00% |
KeyDecoder.kt |
0.00% |
NavigatorImpl.kt |
25.00% |
ResponseMapping.kt |
95.00% |
ResultEffect.kt |
0.00% |
ResultEventBus.kt |
22.22% |
Modified Files Not Found In Coverage Report:
FakeNavigator.kt
HomeScreenTest.kt
MainActivity.kt
MainActivityModule.kt
Navigator.kt
ResponseMappingTest.kt
build.gradle.kts
gradle-wrapper.properties
libs.versions.toml
Codebase cunningly covered by count Shroud 🧛
Generated by 🚫 Danger
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3-template-compose
branch
from
b9181b3 to
4a9adbc
Compare
eeeeaa
requested review from
hoangnguyen92dn,
sleepylee,
thawzintoe-ptut,
toby-thanathip and
win-than-htike
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
template-compose/app/build.gradle.kts (1)
146-146: Consider depending onkotlinx-serialization-coreunlesstemplate-compose/appusesJsondirectly.The deeplink helpers in
template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.ktandKeyDecoder.ktonly use core serialization types (KSerializer,SerialDescriptor,AbstractDecoder). No usage ofJson,encodeToString, ordecodeFromStringexists in the app module, so the narrowerkotlinx-serialization-coredependency is sufficient.♻️ Suggested change
- implementation(libs.kotlinx.serialization.json) + implementation(libs.kotlinx.serialization.core)Also add a catalog alias:
kotlinx-serialization-core = { group = "org.jetbrains.kotlinx", name = "kotlinx-serialization-core", version.ref = "kotlinxSerialization" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/build.gradle.kts` at line 146, The project currently adds implementation(libs.kotlinx.serialization.json) but the app module only uses core serialization types (see DeepLinkPattern.kt and KeyDecoder.kt which reference KSerializer, SerialDescriptor, AbstractDecoder), so replace the json artifact with the core artifact: update the dependency in build.gradle.kts to use the kotlinx-serialization-core catalog alias (i.e., change the implementation(...) entry referencing kotlinx.serialization.json to the core alias) and add the corresponding catalog alias entry kotlinx-serialization-core = { group="org.jetbrains.kotlinx", name="kotlinx-serialization-core", version.ref="kotlinxSerialization" } to the version catalog.template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kt (1)
21-25: Null-to-empty-string substitution may cause unintended parse failures.When a query parameter is present but has no value (e.g.,
?fooor?foo=),getQueryParameterreturnsnullor empty string. The current code maps both to"", which will then fail parsing for non-string types inDeepLinkMatcher(e.g.,"".toInt()throws).While
matchQueryArgscatches this and returnsfalse, the semantics are unclear: is "present but null" different from "present but empty"?Consider excluding null values from the map so they aren't attempted to parse:
♻️ Proposed refinement
val queries = buildMap { uri.queryParameterNames.forEach { argName -> - this[argName] = uri.getQueryParameter(argName) ?: "" + uri.getQueryParameter(argName)?.let { value -> + this[argName] = value + } } }This way, only query parameters with actual values are included in the map, and
DeepLinkMatcherwon't attempt to parse absent values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kt` around lines 21 - 25, The current buildMap in DeepLinkRequest.kt unconditionally substitutes null query parameter values to "" which causes parsing errors downstream in DeepLinkMatcher; change the map population to only insert entries when uri.getQueryParameter(argName) returns a non-null value (i.e., skip nulls) so that "present but no value" parameters are excluded from queries and DeepLinkMatcher.matchQueryArgs won't attempt to parse empty placeholders.template-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kt (1)
49-53: Potential inconsistency betweencontainscheck and null handling.Line 49 uses
arguments.contains(currentName)which returns true even if the value isnull. However, if a null value is present,decodeValue()will throw at Line 63. The serialization flow should calldecodeNotNullMark()first, but this contract depends on the serializer implementation.Consider aligning the check to only process non-null entries:
♻️ Safer alternative
- if (arguments.contains(currentName)) { + if (arguments[currentName] != null) {This ensures only non-null arguments trigger decoding, making the behavior consistent with
decodeNotNullMark().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kt` around lines 49 - 53, The check arguments.contains(currentName) in KeyDecoder (the block that sets elementIndex/elementName and returns elementIndex) can accept null values which later causes decodeValue() to throw; change the logic to only treat an entry as present when it is non-null — e.g., verify the argument value for currentName is not null (or call the decoder's decodeNotNullMark()/equivalent null-check) before assigning elementIndex/elementName and returning; update the code paths around decodeValue() (the method invoking decodeNotNullMark() and decodeValue()) so presence detection and null handling are consistent and avoid attempting to decode null entries.template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt (1)
8-9: Make the fake start from the same initial destination as production.
MainActivityModuleprovidesNavigatorImpl(startDestination = Home), but this fake starts with an empty stack. That lets tests pass against states the app can never reach and can hide back-stack regressions. Seed it with the real start destination, or accept one via constructor so tests can mirror production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt` around lines 8 - 9, FakeNavigator currently initializes backStack empty which diverges from production where MainActivityModule constructs NavigatorImpl(startDestination = Home); change FakeNavigator to accept a start destination (e.g., a parameter like startDestination: Any) or seed its backStack with the production start (Home) so tests mirror real app state; update the FakeNavigator class constructor and its override val backStack: SnapshotStateList<Any> = mutableStateListOf(startDestination) (or push Home when no arg) and adjust tests to pass the same startDestination used by NavigatorImpl.template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt (2)
65-65: Consider makingchannelMapprivate.Exposing the mutable map as
public valbreaks encapsulation and allows external code to directly manipulate the internal state, potentially bypassing the intended API and causing inconsistent behavior.♻️ Proposed fix
- val channelMap: MutableMap<String, Channel<Any?>> = mutableMapOf() + private val channelMap: MutableMap<String, Channel<Any?>> = mutableMapOf()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt` at line 65, The public mutable map channelMap on ResultEventBus breaks encapsulation; make channelMap private (e.g., private val channelMap: MutableMap<String, Channel<Any?>>) and expose only the necessary API from ResultEventBus (public methods such as register/obtain/closeChannel or a read-only view like getChannel(key): Channel<Any?>?) so external code cannot mutate the internal map directly; update all usages of channelMap inside the class to the new private property and call the public methods instead.
86-89: Potential race betweenclose()andremove()in concurrent scenarios.If another coroutine calls
sendResultorgetResultFlowfor the same key betweenclose()andremove(), it could interact with a closed channel or recreate it unexpectedly. If thread-safety is a concern, consider using synchronized access orConcurrentHashMap.remove().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt` around lines 86 - 89, The current removeResult closes the channel then removes it, which can race with sendResult/getResultFlow creating a new channel between those calls; change removeResult to atomically remove first and then close the removed channel (e.g. val ch = channelMap.remove(resultKey); ch?.close()) and ensure channelMap is a thread-safe map (e.g. ConcurrentHashMap) so operations from sendResult/getResultFlow and removeResult are safe; reference removeResult, channelMap, sendResult and getResultFlow when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kt`:
- Around line 101-117: In handleNewIntent, wrap the deep-link deserialization
and navigation in a try-catch: surround the
KeyDecoder(match.args).decodeSerializableValue(match.serializer) and subsequent
navigator.goTo(deepLinkNavKey) call with a try block and catch
serialization/deserialization exceptions (e.g., Exception or a more specific
serialization exception), and in the catch clear intent.data (set to null) and
skip navigation so a bad/malformed intent does not crash the activity; keep the
existing logic to clear intent.data after successful navigation as well.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt`:
- Around line 37-42: The catch logging in DeepLinkMatcher (inside the
candidateSegment.typeParser invocation) currently logs the raw requestedSegment;
update the error logging to avoid printing raw deep-link values and instead log
safe identifiers (e.g., the argument name or pattern from candidateSegment) and
context (e.g., route pattern) — do the same for the query parsing block around
the query.value handling (lines referencing query.value) so no raw URI values or
PII are emitted; locate the code in DeepLinkMatcher.kt where
candidateSegment.typeParser is invoked and where query.value is logged and
replace the message to include only argument names/patterns and the exception
details, not the raw input.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt`:
- Around line 107-113: The Boolean and Char parsers in DeepLinkPattern.kt use
lenient functions (String::toBoolean and a lambda using it.first()) that either
silently coerce values or throw the wrong exception; replace the
PrimitiveKind.BOOLEAN and PrimitiveKind.CHAR branches with strict validators
that explicitly check the input string: for BOOLEAN only accept "true" or
"false" (return corresponding Boolean or throw IllegalArgumentException on any
other value) and for CHAR ensure the string length is exactly 1 (return the
single Char or throw IllegalArgumentException otherwise) so DeepLinkMatcher can
catch malformed deep links properly.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kt`:
- Around line 40-42: LaunchedEffect is depending on
resultEventBus.channelMap[resultKey] which is a plain MutableMap entry and
getResultFlow(resultKey) can return null until sendResult creates the channel,
so the effect may never restart and miss results; fix by ensuring the
channel/flow is created before collecting—either call a new or existing eager
factory on ResultEventBus (e.g., add/invoke ensureChannelOrGetFlow(resultKey) or
change getResultFlow to create the channel if absent) so that
LaunchedEffect(resultKey, ...) always observes a non-null flow, then collect
from resultEventBus.getResultFlow<T>(resultKey) safely (or use the returned
non-null flow) instead of relying on a map entry change; reference symbols:
LaunchedEffect, resultKey, resultEventBus.channelMap, getResultFlow, sendResult,
channelMap.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt`:
- Around line 76-81: sendResult has a race between channelMap.contains +
creation and uses trySend with BufferOverflow.SUSPEND which is contradictory;
fix by making channelMap access atomic (use channelMap.getOrPut(resultKey) or
switch channelMap to a thread-safe ConcurrentHashMap) to ensure only one Channel
is created for a key, and replace trySend(result) with a suspending send (e.g.,
launch/coroutineScope { channel.send(result) }) if you want suspension on full
buffer, or change the Channel creation policy to a non-suspending overflow
(DROP_OLDEST/DROP_LATEST) if silent drops are acceptable; update the code paths
referencing sendResult and channelMap to match the chosen approach (sendResult,
channelMap, Channel).
In
`@template-compose/data/src/test/java/co/nimblehq/template/compose/data/extensions/ResponseMappingTest.kt`:
- Line 37: The test currently uses value equality ("it shouldBe
NoConnectivityException()") which checks reference/equality rather than type;
update the assertion in ResponseMappingTest to a type-based check by replacing
the shouldBe NoConnectivityException() call with a Kotest type assertion such as
shouldBeInstanceOf<NoConnectivityException>() (or
shouldBeInstanceOf(NoConnectivityException::class)) so the test verifies the
exception type rather than instance equality.
- Line 27: The test uses reference equality against a class instance
(NoConnectivityException()) which will fail; change the assertion to a
type-based check by replacing the equality assertion with a Kotest type matcher
(use shouldBeInstanceOf for the exception under test) and add the import
io.kotest.matchers.types.shouldBeInstanceOf so the assertion verifies the value
is an instance of NoConnectivityException rather than comparing object
references.
---
Nitpick comments:
In `@template-compose/app/build.gradle.kts`:
- Line 146: The project currently adds
implementation(libs.kotlinx.serialization.json) but the app module only uses
core serialization types (see DeepLinkPattern.kt and KeyDecoder.kt which
reference KSerializer, SerialDescriptor, AbstractDecoder), so replace the json
artifact with the core artifact: update the dependency in build.gradle.kts to
use the kotlinx-serialization-core catalog alias (i.e., change the
implementation(...) entry referencing kotlinx.serialization.json to the core
alias) and add the corresponding catalog alias entry kotlinx-serialization-core
= { group="org.jetbrains.kotlinx", name="kotlinx-serialization-core",
version.ref="kotlinxSerialization" } to the version catalog.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kt`:
- Around line 21-25: The current buildMap in DeepLinkRequest.kt unconditionally
substitutes null query parameter values to "" which causes parsing errors
downstream in DeepLinkMatcher; change the map population to only insert entries
when uri.getQueryParameter(argName) returns a non-null value (i.e., skip nulls)
so that "present but no value" parameters are excluded from queries and
DeepLinkMatcher.matchQueryArgs won't attempt to parse empty placeholders.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kt`:
- Around line 49-53: The check arguments.contains(currentName) in KeyDecoder
(the block that sets elementIndex/elementName and returns elementIndex) can
accept null values which later causes decodeValue() to throw; change the logic
to only treat an entry as present when it is non-null — e.g., verify the
argument value for currentName is not null (or call the decoder's
decodeNotNullMark()/equivalent null-check) before assigning
elementIndex/elementName and returning; update the code paths around
decodeValue() (the method invoking decodeNotNullMark() and decodeValue()) so
presence detection and null handling are consistent and avoid attempting to
decode null entries.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt`:
- Line 65: The public mutable map channelMap on ResultEventBus breaks
encapsulation; make channelMap private (e.g., private val channelMap:
MutableMap<String, Channel<Any?>>) and expose only the necessary API from
ResultEventBus (public methods such as register/obtain/closeChannel or a
read-only view like getChannel(key): Channel<Any?>?) so external code cannot
mutate the internal map directly; update all usages of channelMap inside the
class to the new private property and call the public methods instead.
- Around line 86-89: The current removeResult closes the channel then removes
it, which can race with sendResult/getResultFlow creating a new channel between
those calls; change removeResult to atomically remove first and then close the
removed channel (e.g. val ch = channelMap.remove(resultKey); ch?.close()) and
ensure channelMap is a thread-safe map (e.g. ConcurrentHashMap) so operations
from sendResult/getResultFlow and removeResult are safe; reference removeResult,
channelMap, sendResult and getResultFlow when making the change.
In
`@template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt`:
- Around line 8-9: FakeNavigator currently initializes backStack empty which
diverges from production where MainActivityModule constructs
NavigatorImpl(startDestination = Home); change FakeNavigator to accept a start
destination (e.g., a parameter like startDestination: Any) or seed its backStack
with the production start (Home) so tests mirror real app state; update the
FakeNavigator class constructor and its override val backStack:
SnapshotStateList<Any> = mutableStateListOf(startDestination) (or push Home when
no arg) and adjust tests to pass the same startDestination used by
NavigatorImpl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff5e6f5-184f-4025-9079-6ea56d0e87eb
📒 Files selected for processing (27)
template-compose/app/build.gradle.ktstemplate-compose/app/src/main/java/co/nimblehq/template/compose/di/modules/main/MainActivityModule.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/extensions/ComponentActivityExt.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/extensions/SavedStateHandleExt.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/navigation/Navigator.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/navigation/NavigatorImpl.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/AppDestination.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/AppNavGraph.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/base/BaseDestination.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/base/BaseViewModel.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/MainDestination.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/MainNavGraph.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kttemplate-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kttemplate-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreenTest.kttemplate-compose/data/src/main/java/co/nimblehq/template/compose/data/extensions/ResponseMapping.kttemplate-compose/data/src/test/java/co/nimblehq/template/compose/data/extensions/ResponseMappingTest.kttemplate-compose/domain/src/main/java/co/nimblehq/template/compose/domain/exceptions/Exceptions.kttemplate-compose/gradle/libs.versions.tomltemplate-compose/gradle/wrapper/gradle-wrapper.properties
💤 Files with no reviewable changes (6)
- template-compose/app/src/main/java/co/nimblehq/template/compose/ui/base/BaseDestination.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/extensions/SavedStateHandleExt.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/ui/AppNavGraph.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/ui/AppDestination.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/MainNavGraph.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/MainDestination.kt
template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kt
Outdated
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kt
Outdated
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt
Show resolved
Hide resolved
...mpose/data/src/test/java/co/nimblehq/template/compose/data/extensions/ResponseMappingTest.kt
Outdated
Show resolved
Hide resolved
...mpose/data/src/test/java/co/nimblehq/template/compose/data/extensions/ResponseMappingTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt (1)
67-67: Consider makingchannelMapprivate.The
channelMapproperty is currentlypublicwhich exposes the internal implementation details of the event bus. This allows external code to directly manipulate the map, bypassing the provided API methods (sendResult,ensureChannelAndGetFlow,removeResult).♻️ Proposed fix to encapsulate internal state
- val channelMap = ConcurrentHashMap<String, Channel<Any?>>() + private val channelMap = ConcurrentHashMap<String, Channel<Any?>>()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt` at line 67, The channelMap field in ResultEventBus is public and exposes internal state; make channelMap private to encapsulate the implementation and prevent external mutation. Update the declaration of channelMap to be private (retain type ConcurrentHashMap<String, Channel<Any?>>) and ensure existing methods sendResult, ensureChannelAndGetFlow, and removeResult continue to access it internally; run tests/compile to confirm no external usages rely on the public map and replace any external usages with new API methods if found.template-compose/gradle/libs.versions.toml (1)
68-69: Minor formatting: missing blank line before section comment.For consistency with other sections in this file, consider adding a blank line between the
kotlinx-serialization-coredeclaration and the# Logcomment.📝 Suggested formatting fix
kotlinx-serialization-core = { group = "org.jetbrains.kotlinx", name = "kotlinx-serialization-core", version.ref = "kotlinxSerialization" } + # Log🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/gradle/libs.versions.toml` around lines 68 - 69, Add a blank line between the kotlinx-serialization-core dependency declaration (kotlinx-serialization-core = { group = "org.jetbrains.kotlinx", name = "kotlinx-serialization-core", version.ref = "kotlinxSerialization" }) and the following section comment (“# Log”) so the file matches the other section formatting and keeps a clear separation between dependency entries and section headers.template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt (1)
1-7: Consider relocating to mirror source directory structure.Per coding guidelines, test files should mirror source directory structure.
Navigatoris innavigation/, butFakeNavigatoris inui/screens/. Consider moving totest/.../navigation/FakeNavigator.ktfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt` around lines 1 - 7, The FakeNavigator test utility is placed under ui/screens which doesn't mirror the production package where Navigator lives (navigation); move the FakeNavigator class file into the test package that parallels the source navigation package (e.g., test/.../navigation/) so its package path matches Navigator's structure; update the Kotlin package declaration inside the FakeNavigator file to the new navigation test package and adjust any imports/usages referencing FakeNavigator to the new package to ensure tests compile (look for the FakeNavigator type and Navigator interface references).template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kt (1)
72-89:popTransitionSpecandpredictivePopTransitionSpecare identical — consider extracting.Both transition specs have the same implementation. Extract to a shared variable to reduce duplication.
♻️ Suggested refactor
+ val popSpec: NavDisplay.TransitionSpec = { + slideInHorizontally( + initialOffsetX = { -it }, + animationSpec = tween(TWEEN_DURATION_IN_MILLIS) + ) togetherWith slideOutHorizontally( + targetOffsetX = { it }, + animationSpec = tween(TWEEN_DURATION_IN_MILLIS) + ) + } // ... in NavDisplay call: - popTransitionSpec = { - slideInHorizontally( - initialOffsetX = { -it }, - animationSpec = tween(TWEEN_DURATION_IN_MILLIS) - ) togetherWith slideOutHorizontally( - targetOffsetX = { it }, - animationSpec = tween(TWEEN_DURATION_IN_MILLIS) - ) - }, - predictivePopTransitionSpec = { - slideInHorizontally( - initialOffsetX = { -it }, - animationSpec = tween(TWEEN_DURATION_IN_MILLIS) - ) togetherWith slideOutHorizontally( - targetOffsetX = { it }, - animationSpec = tween(TWEEN_DURATION_IN_MILLIS) - ) - } + popTransitionSpec = popSpec, + predictivePopTransitionSpec = popSpec🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kt` around lines 72 - 89, popTransitionSpec and predictivePopTransitionSpec in MainActivity.kt duplicate the same slideInHorizontally/slideOutHorizontally logic using TWEEN_DURATION_IN_MILLIS; extract the shared AnimationSpec into a single val (e.g., horizontalPopTransitionSpec or POP_TRANSITION_SPEC) and replace both popTransitionSpec and predictivePopTransitionSpec with that variable to remove duplication and keep behavior identical. Ensure the new val is scoped appropriately (top-level or inside the MainActivity composable) and references TWEEN_DURATION_IN_MILLIS just like the originals.template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt (1)
17-22: Consider handling URI comparison case-sensitivity.The exact match check at line 20 compares
request.uri == deepLinkPattern.uriPatterndirectly. If the incoming URI has different casing in the scheme or host, this may fail to match even when semantically equivalent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt` around lines 17 - 22, The exact-match check inside DeepLinkMatcher.match() currently does a direct string comparison of request.uri and deepLinkPattern.uriPattern, which is case-sensitive; update the exact-match logic to perform a case-insensitive comparison for scheme and host (while keeping path/authority/queries compared as appropriate) by parsing both strings into URIs (e.g., java.net.URI) and comparing uri.scheme.lowercase() and uri.host.lowercase() plus the normalized path/query components before returning DeepLinkMatchResult(deepLinkPattern.serializer, mapOf()); adjust the condition that presently checks request.uri == deepLinkPattern.uriPattern so semantically equivalent URIs with different casing still match.template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt (1)
82-88: Same potential crash for query parameter names.The same issue applies here:
getElementIndex(paramName)will throw if a query parameter in the URI pattern doesn't match any field in the serializer's descriptor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt` around lines 82 - 88, The loop building queryValueParsers can throw when uriPattern.queryParameterNames contains a name not present in the serializer descriptor; guard against that by skipping unknown query names before calling serializer.descriptor.getElementIndex and getElementDescriptor (e.g. check paramName is in serializer.descriptor.elementNames or safely attempt getElementIndex inside a try/catch), and only call getTypeParser and put an entry into queryValueParsers for valid descriptor elements referenced by getElementDescriptor; update the code around queryValueParsers, getElementIndex, serializer.descriptor.getElementDescriptor, and getTypeParser accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kt`:
- Line 42: deepLinkPatterns is initialized empty, so handleNewIntent will never
match deep links (firstNotNullOfOrNull returns null); either add a TODO comment
indicating deep link support is pending or populate deepLinkPatterns with actual
DeepLinkPattern instances (e.g., provide them via Hilt multibinding similar to
entryProviderScopes) and update usages in handleNewIntent to iterate those
patterns; specifically modify the deepLinkPatterns declaration and the code that
resolves patterns in handleNewIntent/firstNotNullOfOrNull to use the
injected/multibound list of DeepLinkPattern objects.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt`:
- Around line 64-69: During DeepLinkPattern construction, validate the result of
serializer.descriptor.getElementIndex(argName) before calling
getElementDescriptor: check that elementIndex >= 0 and if not throw/raise an
error that clearly names the missing argument (argName) and the serializable
type context (serializer.descriptor) so callers know which argument is absent;
update the block that currently calls getElementIndex -> getElementDescriptor ->
add(PathSegment(...)) to perform this guard and produce a descriptive error
instead of allowing getElementDescriptor to fail.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt`:
- Around line 80-81: Remove the unused helper method getResultFlow (inline fun
<reified T> getResultFlow(resultKey: String = T::class.toString()) =
channelMap[resultKey]?.receiveAsFlow()) from ResultEventBus; ensure callers use
the existing ensureChannelAndGetFlow API (used in ResultEffect.kt) which
guarantees a channel exists before returning the flow, and run project-wide
search to confirm no references remain before committing.
---
Nitpick comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kt`:
- Around line 72-89: popTransitionSpec and predictivePopTransitionSpec in
MainActivity.kt duplicate the same slideInHorizontally/slideOutHorizontally
logic using TWEEN_DURATION_IN_MILLIS; extract the shared AnimationSpec into a
single val (e.g., horizontalPopTransitionSpec or POP_TRANSITION_SPEC) and
replace both popTransitionSpec and predictivePopTransitionSpec with that
variable to remove duplication and keep behavior identical. Ensure the new val
is scoped appropriately (top-level or inside the MainActivity composable) and
references TWEEN_DURATION_IN_MILLIS just like the originals.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt`:
- Around line 17-22: The exact-match check inside DeepLinkMatcher.match()
currently does a direct string comparison of request.uri and
deepLinkPattern.uriPattern, which is case-sensitive; update the exact-match
logic to perform a case-insensitive comparison for scheme and host (while
keeping path/authority/queries compared as appropriate) by parsing both strings
into URIs (e.g., java.net.URI) and comparing uri.scheme.lowercase() and
uri.host.lowercase() plus the normalized path/query components before returning
DeepLinkMatchResult(deepLinkPattern.serializer, mapOf()); adjust the condition
that presently checks request.uri == deepLinkPattern.uriPattern so semantically
equivalent URIs with different casing still match.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt`:
- Around line 82-88: The loop building queryValueParsers can throw when
uriPattern.queryParameterNames contains a name not present in the serializer
descriptor; guard against that by skipping unknown query names before calling
serializer.descriptor.getElementIndex and getElementDescriptor (e.g. check
paramName is in serializer.descriptor.elementNames or safely attempt
getElementIndex inside a try/catch), and only call getTypeParser and put an
entry into queryValueParsers for valid descriptor elements referenced by
getElementDescriptor; update the code around queryValueParsers, getElementIndex,
serializer.descriptor.getElementDescriptor, and getTypeParser accordingly.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt`:
- Line 67: The channelMap field in ResultEventBus is public and exposes internal
state; make channelMap private to encapsulate the implementation and prevent
external mutation. Update the declaration of channelMap to be private (retain
type ConcurrentHashMap<String, Channel<Any?>>) and ensure existing methods
sendResult, ensureChannelAndGetFlow, and removeResult continue to access it
internally; run tests/compile to confirm no external usages rely on the public
map and replace any external usages with new API methods if found.
In
`@template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt`:
- Around line 1-7: The FakeNavigator test utility is placed under ui/screens
which doesn't mirror the production package where Navigator lives (navigation);
move the FakeNavigator class file into the test package that parallels the
source navigation package (e.g., test/.../navigation/) so its package path
matches Navigator's structure; update the Kotlin package declaration inside the
FakeNavigator file to the new navigation test package and adjust any
imports/usages referencing FakeNavigator to the new package to ensure tests
compile (look for the FakeNavigator type and Navigator interface references).
In `@template-compose/gradle/libs.versions.toml`:
- Around line 68-69: Add a blank line between the kotlinx-serialization-core
dependency declaration (kotlinx-serialization-core = { group =
"org.jetbrains.kotlinx", name = "kotlinx-serialization-core", version.ref =
"kotlinxSerialization" }) and the following section comment (“# Log”) so the
file matches the other section formatting and keeps a clear separation between
dependency entries and section headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08b81067-8fc5-4319-8b59-71e7bc27085a
📒 Files selected for processing (11)
template-compose/app/build.gradle.ktstemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kttemplate-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kttemplate-compose/data/src/test/java/co/nimblehq/template/compose/data/extensions/ResponseMappingTest.kttemplate-compose/gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- template-compose/app/build.gradle.kts
- template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kt
template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kt
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt
Show resolved
Hide resolved
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3-template-compose
branch
from
46cd22d to
82ed328
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt (1)
68-71:⚠️ Potential issue | 🟠 MajorValidate placeholder names before reading descriptor indices.
getElementIndex()can return an unknown-name sentinel when a path or query placeholder is not a field onT. Passing that value intogetElementDescriptor()crashesDeepLinkPatternconstruction, so one typo in the URI template takes the app down instead of failing with a clear configuration error.Suggested fix
- val elementIndex = serializer.descriptor.getElementIndex(argName) - val elementDescriptor = serializer.descriptor.getElementDescriptor(elementIndex) + val elementIndex = serializer.descriptor.getElementIndex(argName) + require(elementIndex >= 0) { + "Unknown deep link argument '$argName' for ${serializer.descriptor.serialName}" + } + val elementDescriptor = serializer.descriptor.getElementDescriptor(elementIndex) // finally, add the arg name and its respective type parser to the map add(PathSegment(argName, true, getTypeParser(elementDescriptor.kind))) @@ - val elementIndex = serializer.descriptor.getElementIndex(paramName) - val elementDescriptor = serializer.descriptor.getElementDescriptor(elementIndex) + val elementIndex = serializer.descriptor.getElementIndex(paramName) + require(elementIndex >= 0) { + "Unknown deep link query '$paramName' for ${serializer.descriptor.serialName}" + } + val elementDescriptor = serializer.descriptor.getElementDescriptor(elementIndex) this[paramName] = getTypeParser(elementDescriptor.kind)For kotlinx.serialization SerialDescriptor, what does getElementIndex(name) return when the name is missing, and is that return value valid to pass into getElementDescriptor(index)?Also applies to: 87-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt` around lines 68 - 71, DeepLinkPattern construction currently calls serializer.descriptor.getElementIndex(argName) and immediately passes the result into getElementDescriptor, which will crash if getElementIndex returns the "unknown-name" sentinel; instead, after computing elementIndex (from serializer.descriptor.getElementIndex(argName)), validate that it is not the sentinel (e.g., negative/SerialDescriptor.UNKNOWN_NAME index) and if it is, throw or return a clear configuration error mentioning the invalid placeholder name and the DeepLinkPattern template; apply the same check for the other occurrence around the code that handles query/path placeholders (the similar block at lines 87-89) so typos in URI templates produce readable errors rather than runtime crashes.
🧹 Nitpick comments (1)
template-compose/gradle/libs.versions.toml (1)
104-106: Keep Navigation 3 / Compose integrations out of the generic bundles.Adding
navigation3-*tobundles.androidxandhilt-lifecycle-viewmodel-composetobundles.hiltmakes those broad aliases navigation-aware. A dedicatednavigation3bundle, and optionally ahilt-composebundle, will keep unrelated modules from picking these up accidentally.Suggested catalog split
androidx = [ "androidx-core", "androidx-lifecycle-runtime", "androidx-lifecycle-compose", - "androidx-lifecycle-viewmodel-navigation3", - "androidx-navigation3-runtime", - "androidx-navigation3-ui", ] +navigation3 = [ + "androidx-lifecycle-viewmodel-navigation3", + "androidx-navigation3-runtime", + "androidx-navigation3-ui", +] hilt = [ "hilt-android", - "hilt-lifecycle-viewmodel-compose", ] +hilt-compose = [ + "hilt-lifecycle-viewmodel-compose", +]Also applies to: 115-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/gradle/libs.versions.toml` around lines 104 - 106, The androidx navigation3 entries ("androidx-lifecycle-viewmodel-navigation3", "androidx-navigation3-runtime", "androidx-navigation3-ui") and the hilt-compose alias (hilt-lifecycle-viewmodel-compose) were added into broad bundles (bundles.androidx and bundles.hilt); move these items out of those generic bundles and instead create a dedicated navigation3 bundle (e.g., bundles.navigation3) that contains the three navigation3 artifacts, and optionally a bundles.hilt-compose (or hilt-compose) bundle that contains hilt-lifecycle-viewmodel-compose so that unrelated modules using bundles.androidx or bundles.hilt do not become navigation- or compose-aware inadvertently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt`:
- Around line 28-63: The match() function currently only checks path/query shape
and can succeed for URIs with the wrong host/scheme; add an early guard that
rejects mismatched scheme/authority before any path parsing. Specifically, in
match() compare the request URI's scheme and authority (or host+port) to the
deepLinkPattern's expected scheme/authority (derived from
deepLinkPattern.uriPattern or parsed Uri) and return null immediately if they
differ, then continue to existing path and matchQueryArgs() logic so only URIs
for the intended domain proceed to argument parsing and returning a
DeepLinkMatchResult.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt`:
- Around line 104-105: removeResult currently closes and removes the channel
(channelMap) which completes the active LaunchedEffect collector in ResultEffect
so it never re-subscribes when sendResult creates a new channel; change
removeResult (and its use of channelMap) to stop closing and removing the
existing channel—leave the channel instance intact so the existing
LaunchedEffect continues collecting; if you need explicit cleanup, add a
dedicated lifecycle cleanup invoked when the composable is disposed rather than
closing the channel inside removeResult.
---
Duplicate comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt`:
- Around line 68-71: DeepLinkPattern construction currently calls
serializer.descriptor.getElementIndex(argName) and immediately passes the result
into getElementDescriptor, which will crash if getElementIndex returns the
"unknown-name" sentinel; instead, after computing elementIndex (from
serializer.descriptor.getElementIndex(argName)), validate that it is not the
sentinel (e.g., negative/SerialDescriptor.UNKNOWN_NAME index) and if it is,
throw or return a clear configuration error mentioning the invalid placeholder
name and the DeepLinkPattern template; apply the same check for the other
occurrence around the code that handles query/path placeholders (the similar
block at lines 87-89) so typos in URI templates produce readable errors rather
than runtime crashes.
---
Nitpick comments:
In `@template-compose/gradle/libs.versions.toml`:
- Around line 104-106: The androidx navigation3 entries
("androidx-lifecycle-viewmodel-navigation3", "androidx-navigation3-runtime",
"androidx-navigation3-ui") and the hilt-compose alias
(hilt-lifecycle-viewmodel-compose) were added into broad bundles
(bundles.androidx and bundles.hilt); move these items out of those generic
bundles and instead create a dedicated navigation3 bundle (e.g.,
bundles.navigation3) that contains the three navigation3 artifacts, and
optionally a bundles.hilt-compose (or hilt-compose) bundle that contains
hilt-lifecycle-viewmodel-compose so that unrelated modules using
bundles.androidx or bundles.hilt do not become navigation- or compose-aware
inadvertently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d55a8592-bbe4-4ad1-8d0a-2fdc879c3dbd
📒 Files selected for processing (11)
template-compose/app/build.gradle.ktstemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kttemplate-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kttemplate-compose/data/src/test/java/co/nimblehq/template/compose/data/extensions/ResponseMappingTest.kttemplate-compose/gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt
- template-compose/app/build.gradle.kts
template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt
Outdated
Show resolved
Hide resolved
|
Hi @eeeeaa 👋 In real projects, navigation logic can grow quickly. With Nav3's entry-based system, would it make sense to organize entries into a dedicated For example, the package co.nimblehq.template.compose.navigation.entries
import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel
import androidx.navigation3.runtime.EntryProviderScope
import androidx.navigation3.runtime.NavKey
import co.nimblehq.template.compose.navigation.Navigator
import co.nimblehq.template.compose.ui.screens.main.home.HomeScreen
data object Home : NavKey
fun EntryProviderScope<Any>.homeScreenEntry(
navigator: Navigator,
) {
entry<Home> {
HomeScreen(
viewModel = hiltViewModel(),
navigator = navigator
)
}
}Additionally, we can also have a dedicated package co.nimblehq.template.compose.navigation
import androidx.compose.animation.ContentTransform
import androidx.compose.animation.core.tween
import androidx.compose.animation.slideInHorizontally
import androidx.compose.animation.slideOutHorizontally
import androidx.compose.animation.togetherWith
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.unit.IntOffset
import androidx.lifecycle.viewmodel.navigation3.rememberViewModelStoreNavEntryDecorator
import androidx.navigation3.runtime.entryProvider
import androidx.navigation3.runtime.rememberSaveableStateHolderNavEntryDecorator
import androidx.navigation3.ui.NavDisplay
import co.nimblehq.template.compose.navigation.entries.homeScreenEntry
import co.nimblehq.template.compose.util.LocalResultEventBus
import co.nimblehq.template.compose.util.ResultEventBus
private const val TWEEN_DURATION_IN_MILLIS = 500
@Composable
fun AppNavigation(
navigator: Navigator,
modifier: Modifier = Modifier,
) {
val eventBus = remember { ResultEventBus() }
CompositionLocalProvider(LocalResultEventBus.provides(eventBus)) {
NavDisplay(
backStack = navigator.backStack,
onBack = { navigator.goBack() },
entryDecorators = listOf(
rememberSaveableStateHolderNavEntryDecorator(),
rememberViewModelStoreNavEntryDecorator()
),
transitionSpec = { horizontalSlideTransition(isPush = true) },
popTransitionSpec = { horizontalSlideTransition(isPush = false) },
predictivePopTransitionSpec = { horizontalSlideTransition(isPush = false) },
modifier = modifier,
entryProvider = entryProvider {
homeScreenEntry(navigator)
accountScreenEntry(navigator,profile)
cartScreenEntry(navigator,cart)
}
)
}
}
/**
* Creates a horizontal slide transition. If [isPush] is `true`, slides from right to left (Forward).
* Otherwise, slides from left to right (Backward/Pop).
*/
private fun horizontalSlideTransition(isPush: Boolean): ContentTransform {
val direction = if (isPush) 1 else -1
val spec = tween<IntOffset>(TWEEN_DURATION_IN_MILLIS)
return slideInHorizontally(
initialOffsetX = { it * direction },
animationSpec = spec
) togetherWith slideOutHorizontally(
targetOffsetX = { -it * direction },
animationSpec = spec
)
} |
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3-template-compose
branch
from
82ed328 to
318069d
Compare
…t separate navEntry class
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3-template-compose
branch
from
318069d to
0e6b26d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kt (1)
64-64: Consider removing debug log in production code.The
Timber.d("Result : $uiModels")line appears to be debug logging. Consider removing it or guarding it for debug builds only to avoid unnecessary logging in release builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kt` at line 64, Remove the leftover debug log call Timber.d("Result : $uiModels") from the HomeScreen composable (or guard it behind a debug-only check); either delete the Timber.d invocation in HomeScreen.kt or wrap it with a BuildConfig.DEBUG / debug-only guard so the message is not emitted in release builds.template-compose/app/src/main/java/co/nimblehq/template/compose/navigation/entries/HomeScreenNavEntry.kt (1)
11-11: Consider adding@Serializableannotation toHomefor deep link support.The
HomeNavKey should be annotated with@Serializableif it will be used with the deep link system. Looking atMainActivity.kt, theDeepLinkPatternexample usesHome.serializer(), which requires the@Serializableannotation.♻️ Proposed fix
+import kotlinx.serialization.Serializable + +@Serializable data object Home : NavKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/main/java/co/nimblehq/template/compose/navigation/entries/HomeScreenNavEntry.kt` at line 11, The Home NavKey data object lacks the `@Serializable` annotation required by the deep link system; annotate the data object Home (data object Home : NavKey) with `@Serializable` and add the necessary kotlinx.serialization import so calls like Home.serializer() in MainActivity.kt / DeepLinkPattern compile and work correctly.template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt (1)
1-37: Consider movingFakeNavigatorto mirror source directory structure.As per coding guidelines, test files should mirror the source directory structure. Since
Navigatoris located atnavigation/Navigator.kt, consider movingFakeNavigatortosrc/test/java/.../navigation/FakeNavigator.ktfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt` around lines 1 - 37, The test helper FakeNavigator currently lives under ui.screens but should mirror the production package for consistency; move the FakeNavigator class so its package matches the Navigator source package (e.g., package co.nimblehq.template.compose.navigation) and place the file under the test tree in the corresponding navigation directory (src/test/java/.../navigation/FakeNavigator.kt), ensuring imports and references to Navigator, Home, and KClass remain correct and tests compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kt`:
- Around line 44-47: LaunchedEffect currently only keys on resultKey and
force-casts Flow<Any?> elements to T, which can throw ClassCastException and
fails to restart when the CompositionLocal-bound resultEventBus changes; update
the LaunchedEffect to key on both resultKey and resultEventBus (e.g.,
LaunchedEffect(resultKey, resultEventBus)) and guard the payload before casting
by using a safe cast (use result as? T) and only call onResult(result) when the
cast succeeds (otherwise ignore or log the unexpected type) — change references
around resultEventBus.ensureChannelAndGetFlow(...) and onResult to implement
these two fixes.
- Around line 40-45: The LaunchedEffect currently uses only resultKey as its key
so it won't restart when a new ResultEventBus instance is passed; update the
LaunchedEffect call in the function that takes resultEventBus (defaulting to
LocalResultEventBus.current) to include resultEventBus alongside resultKey as
keys (e.g., LaunchedEffect(resultKey, resultEventBus)) so the coroutine that
collects from resultEventBus is restarted when the bus instance changes; ensure
the rest of the block (including the `@Suppress`("UNCHECKED_CAST") and onResult
collection) remains unchanged.
---
Nitpick comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/navigation/entries/HomeScreenNavEntry.kt`:
- Line 11: The Home NavKey data object lacks the `@Serializable` annotation
required by the deep link system; annotate the data object Home (data object
Home : NavKey) with `@Serializable` and add the necessary kotlinx.serialization
import so calls like Home.serializer() in MainActivity.kt / DeepLinkPattern
compile and work correctly.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kt`:
- Line 64: Remove the leftover debug log call Timber.d("Result : $uiModels")
from the HomeScreen composable (or guard it behind a debug-only check); either
delete the Timber.d invocation in HomeScreen.kt or wrap it with a
BuildConfig.DEBUG / debug-only guard so the message is not emitted in release
builds.
In
`@template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt`:
- Around line 1-37: The test helper FakeNavigator currently lives under
ui.screens but should mirror the production package for consistency; move the
FakeNavigator class so its package matches the Navigator source package (e.g.,
package co.nimblehq.template.compose.navigation) and place the file under the
test tree in the corresponding navigation directory
(src/test/java/.../navigation/FakeNavigator.kt), ensuring imports and references
to Navigator, Home, and KClass remain correct and tests compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66223557-3b14-4109-b9be-dff645dbf508
📒 Files selected for processing (15)
template-compose/app/build.gradle.ktstemplate-compose/app/src/main/java/co/nimblehq/template/compose/di/modules/main/MainActivityModule.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/navigation/AppNavigation.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/navigation/entries/HomeScreenNavEntry.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kttemplate-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kttemplate-compose/data/src/test/java/co/nimblehq/template/compose/data/extensions/ResponseMappingTest.kttemplate-compose/gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkRequest.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkMatcher.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEventBus.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/util/DeepLinkPattern.kt
- template-compose/app/build.gradle.kts
- template-compose/app/src/main/java/co/nimblehq/template/compose/util/KeyDecoder.kt
template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kt
Show resolved
Hide resolved
template-compose/app/src/main/java/co/nimblehq/template/compose/util/ResultEffect.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/di/modules/main/MainActivityModule.kt`:
- Around line 16-21: The current provideNavigator binds Navigator to an
in-memory NavigatorImpl (uses SnapshotStateList) under ActivityRetainedComponent
so the back stack is lost on process death; update
provideNavigator/NavigatorImpl to persist the stack keys (not just per-entry
state handled by AppNavigation) by either (A) injecting a
SavedStateHandle/SavedStateRegistryOwner into NavigatorImpl and saving/restoring
the list of route/entry keys to saved state (serialize keys, restore into
SnapshotStateList on construction), or (B) replace the custom stack with Nav3’s
serializable back stack primitives (NavBackStack / savedState via NavController)
so the stack structure is saved across process death; adjust the Hilt provider
(provideNavigator) to accept the saved-state dependency or move the provider to
a component that can access SavedStateRegistry and ensure NavigatorImpl exposes
restore/save hooks called from onCreate/onSaveInstanceState.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61334500-7ce5-4a6d-83f7-d1cabd182e94
📒 Files selected for processing (6)
template-compose/app/src/main/java/co/nimblehq/template/compose/di/modules/main/MainActivityModule.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/navigation/AppNavigation.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/navigation/entries/HomeScreenNavEntry.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/MainActivity.kttemplate-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kttemplate-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- template-compose/app/src/test/java/co/nimblehq/template/compose/ui/screens/FakeNavigator.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/main/home/HomeScreen.kt
- template-compose/app/src/main/java/co/nimblehq/template/compose/navigation/entries/HomeScreenNavEntry.kt
Comment on lines
+16
to
+21
| @InstallIn(ActivityRetainedComponent::class) | ||
| object MainActivityModule { | ||
|
|
||
| @Provides | ||
| @ActivityRetainedScoped | ||
| fun provideNavigator(): Navigator = NavigatorImpl(startDestination = Home) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the key files mentioned
find . -name "NavigatorImpl.kt" -o -name "AppNavigation.kt" -o -name "MainActivityModule.kt" | head -20Repository: nimblehq/android-templates
Length of output: 671
🏁 Script executed:
# Search for the actual files in the template-compose project
fd -t f "NavigatorImpl.kt|AppNavigation.kt|MainActivityModule.kt" template-compose/Repository: nimblehq/android-templates
Length of output: 355
🌐 Web query:
Hilt ActivityRetainedComponent lifecycle process death configuration changes
💡 Result:
ActivityRetainedComponent in Hilt: configuration change vs. process death
What it is
ActivityRetainedComponent is a Hilt component whose lifetime matches a configuration-surviving Activity (i.e., the logical Activity across rotations). It’s the backing scope for @ActivityRetainedScoped. [1], [2]
Configuration changes (rotation, locale, UI mode, etc.)
- The component is created the first time the Activity is created (first
Activity.onCreate()in that logical Activity lifetime). [1], [3] - It survives configuration changes, so a new Activity instance after rotation will reuse the same
ActivityRetainedComponentinstance. [2], [3] - It’s destroyed only when the Activity is actually finished, i.e., at the last
Activity.onDestroy()(not the intermediateonDestroy()triggered by rotation). [1], [3]
Process death (app process killed, then later recreated)
ActivityRetainedComponentis in-memory only. If the OS kills your app process, the component is gone.- When the app is recreated after process death, Hilt builds a brand-new component graph, including a new
ActivityRetainedComponent. (Nothing “retained” survives because the prior process memory is wiped.) This matches the general rule thatViewModelsurvives configuration changes but does not survive process death unless you persist/restore state separately. [4], [5]
Practical implication
- Use
@ActivityRetainedScopedfor objects you want to reuse across rotations (common examples: “repo-like” instances tied to a screen flow). - For process-death survival, you must persist/restore state (e.g.,
SavedStateHandlefor small UI state, or database/files for durable data); don’t rely on retained scopes for that. [5]
Sources: Hilt component lifetimes and note about creation/destruction across config changes [1], [3]; definition of ActivityRetainedComponent as configuration-surviving [2]; ViewModel/process-death + state restoration guidance (SavedStateHandle) [4], [5].
🌐 Web query:
Android Navigation 3 save state process death back stack persistence
💡 Result:
To survive process death with Jetpack Navigation 3, you must persist your back stack (because Nav3 makes the back stack app-owned). The intended path is:
-
Use a saveable back stack primitive
NavBackStackis@Serializable, so it can be saved/restored via saved-state automatically when used with the provided “remember + serialization” helpers. This is specifically called out as enabling restore across process death + config change. [1]- The official Nav3 guidance is to create navigation state in a way that’s saveable (i.e., stored in Android saved state) rather than keeping it only in memory. [2][3]
-
Ensure everything on the back stack is serializable
- Because the back stack is serialized, your destination “keys” (routes/entries) must be serializable (typically via
kotlinx.serialization). [1][3]
- Because the back stack is serialized, your destination “keys” (routes/entries) must be serializable (typically via
-
If you keep per-destination state in ViewModels, scope it to the back stack entry
- Nav3 provides an add-on (
androidx.lifecycle:lifecycle-viewmodel-navigation3) with aNavEntryDecoratorto scope/clearViewModelstate only while thatNavEntryremains on the back stack, and clear it when popped—while still allowing restoration after process recreation when backed by saved state. [2]
- Nav3 provides an add-on (
Key takeaway: in Nav3, back stack persistence across process death happens by serializing NavBackStack into saved state (rather than NavController doing it implicitly like Nav2). [1][2]
Sources:
[1] AndroidX Navigation3 release notes (NavBackStack serializable; process-death restore)
[2] Android Developers: “Save and manage navigation state” (Navigation 3)
[3] Android Developers: Navigation 3 recipes showing saveable back stack patterns
🏁 Script executed:
cat -n template-compose/app/src/main/java/co/nimblehq/template/compose/navigation/NavigatorImpl.ktRepository: nimblehq/android-templates
Length of output: 1128
🏁 Script executed:
cat -n template-compose/app/src/main/java/co/nimblehq/template/compose/navigation/AppNavigation.ktRepository: nimblehq/android-templates
Length of output: 2644
🏁 Script executed:
cat -n template-compose/app/src/main/java/co/nimblehq/template/compose/di/modules/main/MainActivityModule.ktRepository: nimblehq/android-templates
Length of output: 1231
This navigator loses the back stack after process death.
provideNavigator() binds Navigator to NavigatorImpl, which stores the back stack only in memory using SnapshotStateList. ActivityRetainedComponent survives configuration changes but not process death. When the app is killed and recreated, a new NavigatorImpl is instantiated with only Home as the initial entry. Navigation 3 requires back stacks to be persisted explicitly (via serializable NavBackStack and saved state) to survive process death. The decorators in AppNavigation (lines 32–33) handle per-entry state but do not persist the stack structure itself. Either persist the back stack keys via saved state or switch to Nav3's built-in serializable back stack primitives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@template-compose/app/src/main/java/co/nimblehq/template/compose/di/modules/main/MainActivityModule.kt`
around lines 16 - 21, The current provideNavigator binds Navigator to an
in-memory NavigatorImpl (uses SnapshotStateList) under ActivityRetainedComponent
so the back stack is lost on process death; update
provideNavigator/NavigatorImpl to persist the stack keys (not just per-entry
state handled by AppNavigation) by either (A) injecting a
SavedStateHandle/SavedStateRegistryOwner into NavigatorImpl and saving/restoring
the list of route/entry keys to saved state (serialize keys, restore into
SnapshotStateList on construction), or (B) replace the custom stack with Nav3’s
serializable back stack primitives (NavBackStack / savedState via NavController)
so the stack structure is saved across process death; adjust the Hilt provider
(provideNavigator) to accept the saved-state dependency or move the provider to
a component that can access SavedStateRegistry and ensure NavigatorImpl exposes
restore/save hooks called from onCreate/onSaveInstanceState.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#617
What happened 👀
Note
Split migration into 3 Parts
Part 1 - migrate template compose
Part 2 - migrate sample compose
Part 3 - refactor sample compose examples
Insight 📝
Reference:
Proof Of Work 📹
App launch successfully and behavior is the same
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Bug Fixes