[#617][Part 2/3] Migrate to navigation 3 - Sample compose project#619
Conversation
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughRefactors navigation from a BaseDestination/NavHost system to a retained-scope Navigator with backStack, adds deep-link parsing/matching, introduces ResultEventBus and ResultEffect for inter-screen results, migrates UiModel to Kotlinx Serialization, updates DI and MainActivity wiring, and bumps dependency versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as MainActivity
participant Nav as Navigator
participant DLM as DeepLinkMatcher
participant REB as ResultEventBus
participant Entry as EntryProvider
Activity->>Nav: injected via DI
Activity->>REB: create & provide LocalResultEventBus
Activity->>Activity: handleNewIntent(intent)
Activity->>DLM: DeepLinkMatcher(DeepLinkRequest, pattern).match()
alt match found
DLM-->>Activity: keyBuilder + args
Activity->>Nav: goTo(keyBuilder(args))
end
Entry->>Nav: compose screen for current destination
Entry->>REB: use LocalResultEventBus for sending/getting results
sequenceDiagram
participant Source as CallingScreen
participant Nav as Navigator
participant Second as SecondScreen
participant REB as ResultEventBus
Source->>Nav: goTo(Second(id))
Nav->>Second: display SecondScreen(id)
Second->>REB: sendResult(key, value)
Second->>Nav: goBack()
Source->>REB: collect ResultEffect(key)
REB-->>Source: result received
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Kover report for template-compose:🧛 Template - Compose Unit Tests Code Coverage:
|
| File | Coverage |
|---|---|
BaseScreen.kt |
100.00% |
BaseViewModel.kt |
85.71% |
ComponentActivityExt.kt |
100.00% |
DeepLinkMatcher.kt |
0.00% |
DeepLinkPattern.kt |
0.00% |
DeepLinkRequest.kt |
0.00% |
HomeScreen.kt |
92.00% |
HomeViewModel.kt |
100.00% |
KeyDecoder.kt |
0.00% |
NavigatorImpl.kt |
25.00% |
ResultEffect.kt |
0.00% |
ResultEventBus.kt |
22.22% |
UiModel.kt |
100.00% |
Modified Files Not Found In Coverage Report:
ComposableUtil.kt
Constants.kt
FakeNavigator.kt
FakeNavigator.kt
HomeScreenTest.kt
HomeScreenTest.kt
HomeViewModelTest.kt
MainActivity.kt
MainActivityModule.kt
Navigator.kt
SecondScreen.kt
ThirdScreen.kt
build.gradle.kts
build.gradle.kts
gradle-wrapper.properties
libs.versions.toml
Codebase cunningly covered by count Shroud 🧛
Generated by 🚫 Danger
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
changed the title
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
changed the base branch from
develop
to
chore/610-part-2-update-navigation-library-and-refactor-to-follow-unidirectional-data-flow
eeeeaa
changed the base branch from
chore/610-part-2-update-navigation-library-and-refactor-to-follow-unidirectional-data-flow
to
develop
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
5d653e5 to
b44c618
Compare
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
changed the title
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
b44c618 to
33c4f7b
Compare
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
33c4f7b to
1491c46
Compare
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sample-compose/gradle/libs.versions.toml (1)
126-129:⚠️ Potential issue | 🔴 CriticalRemove
hilt-navigationfrom the hilt bundle or migrate to the Navigation 3 approach.
androidx.hilt:hilt-navigation-composeis designed for Navigation 2 (Compose) and is incompatible with Navigation 3. Nav3 doesn't useNavController/NavBackStackEntry, so this dependency will pull in the oldnavigation-composeas a transitive dependency without providing any scoping benefit.For Navigation 3, use
androidx.hilt:hilt-lifecycle-viewmodel-compose(Hilt 1.3.0+) combined withandroidx.lifecycle:lifecycle-viewmodel-navigation3decorators to scope ViewModels correctly to individual Nav3 entries. Removehilt-navigation-composefrom the bundle if fully migrating to Nav3.
🤖 Fix all issues with AI agents
In
`@sample-compose/app/src/androidTest/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.kt`:
- Line 77: The assertEquals call has its expected and actual swapped; change the
invocation in HomeScreenTest so the expected value "1" is the first argument and
the actual expression (fakeNavigator.currentScreen() as?
MainDestination.Second)?.id is the second argument; update the assertion that
currently references fakeNavigator.currentScreen(), MainDestination.Second and
id to follow JUnit's assertEquals(expected, actual) ordering.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.kt`:
- Around line 107-121: handleNewIntent currently reads intent.data and navigates
via navigator.goTo(deepLinkNavKey) but never clears the Intent, so the same deep
link can be re-processed on config changes; after successfully decoding and
navigating (i.e., when deepLinkNavKey != null and navigator.goTo is called)
clear the processed deep link by setting intent.data = null (or record the
processed Uri in a short-lived flag) inside handleNewIntent to prevent replay;
update any callers (e.g., onNewIntent) to rely on this cleared intent rather
than re-processing the same Uri.
- Around line 52-56: handleNewIntent(intent) is being invoked inside the
Composable scope created by setContent, causing it to run on every recomposition
and re-trigger navigation (via navigator.goTo). Move the call to
handleNewIntent(intent) out of setContent (before calling setContent) or, if it
must remain inside, wrap it in a side-effect block such as LaunchedEffect(key1 =
intent) so it only runs once per intent; update references to ResultEventBus and
navigator.goTo accordingly to use the pre-composed initialization if you move
the call.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.kt`:
- Line 81: TAG_LOG_ERROR currently holds the leftover string
"Nav3RecipesDeepLink"; update the constant TAG_LOG_ERROR in DeepLinkMatcher.kt
to a value that reflects this project's module or app (e.g.,
"SampleComposeDeepLink" or the app's actual package/module name) or replace the
hardcoded string with a dynamic identifier (such as BuildConfig.APPLICATION_ID)
so logs correctly identify this project/module.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkPattern.kt`:
- Around line 101-116: getTypeParser currently maps PrimitiveKind.CHAR to
String::toCharArray which yields a CharArray and causes a type mismatch; change
the CHAR mapping in getTypeParser to return a single Char (e.g., use
String::first or an equivalent lambda that returns the first character) so the
parser returns Char as expected by the downstream serialization code in
getTypeParser and TypeParser.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkRequest.kt`:
- Around line 21-25: The current buildMap block in DeepLinkRequest.kt forces
non-null with uri.getQueryParameter(argName)!! which crashes for valueless
params; change the mapping so values are handled safely by using
uri.getQueryParameter(argName) ?: "" (or keep the map type as Map<String,
String?> and assign the nullable return directly) when building the queries val,
ensuring you no longer force-unwrap query parameter results from
uri.queryParameterNames.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/KeyDecoder.kt`:
- Around line 10-14: The KDoc for the decoder has a duplicated article ("a a") —
update the comment above the KeyDecoder (the decoder function in KeyDecoder.kt)
to read "Decodes the list of arguments into a back stack key" (remove the extra
"a") so the documentation is correct and typo-free.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/ResultEventBus.kt`:
- Around line 83-85: The removeResult function currently removes the channel
reference from channelMap without closing it, which can leave active
receiveAsFlow() collectors hanging; update removeResult (and the overloads that
use channelMap) to first lookup the Channel instance from channelMap, call its
close() (e.g., close(null) or close(cause) to signal completion), and only then
remove it from channelMap so any collectors complete properly; ensure you
reference channelMap and the Channel type used when retrieving and closing the
channel in removeResult.
In
`@sample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.kt`:
- Line 108: The assertEquals call in HomeScreenTest uses swapped arguments;
change the order so the literal expected value comes first and the dynamic
actual value second — replace the current
assertEquals((fakeNavigator.currentScreen() as? MainDestination.Second)?.id,
"1") with assertEquals("1", (fakeNavigator.currentScreen() as?
MainDestination.Second)?.id) so the failure messages report "expected: [1] but
was: [...]"; locate the call referencing fakeNavigator.currentScreen() and
MainDestination.Second in HomeScreenTest.
🧹 Nitpick comments (12)
sample-compose/domain/build.gradle.kts (1)
7-14: Redundantjavacompatibility block.
kotlin { jvmToolchain(17) }already configures the Java toolchain (including source/target compatibility) for both Kotlin and Java compilation. Thejavablock on lines 7-10 is now redundant.♻️ Proposed simplification
-java { - sourceCompatibility = JavaVersion.VERSION_17 - targetCompatibility = JavaVersion.VERSION_17 -} - kotlin { jvmToolchain(17) }sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/ComposableUtil.kt (1)
9-11:@SuppressLint("ComposableNaming")is no longer needed.Now that the function is correctly named
StatusBarColor(PascalCase), it already follows the Compose naming convention for@Composablefunctions that returnUnit. The suppress annotation was only necessary for the oldsetStatusBarColorname.♻️ Proposed fix
-@SuppressLint("ComposableNaming") `@Composable` fun StatusBarColor(You can also remove the unused
android.annotation.SuppressLintimport on line 3 if nothing else in the file uses it.sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkPattern.kt (1)
54-71:varshould beval—resultis never reassigned.Proposed fix
- var result = regexPatternFillIn.find(segment) + val result = regexPatternFillIn.find(segment)sample-compose/app/src/main/java/co/nimblehq/sample/compose/navigation/Navigator.kt (1)
1-18: This interface design correctly follows Navigation 3's API requirements.
NavDisplaydirectly requires aSnapshotStateListparameter for the back stack (as confirmed in MainActivity.kt), making the mutable exposure necessary rather than optional. The use ofAnyfor destinations is consistent with Navigation 3's design pattern.The optional refactor to split read/write access (exposing a read-only view in the interface while keeping the mutable list internal) remains a valid future improvement if Navigation 3 evolves to support immutable back stack parameters.
sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/ResultEventBus.kt (1)
62-62:channelMapshould not be public — it leaks internal mutable state.Exposing the backing
MutableMapallows any consumer to arbitrarily mutate channels.ResultEffectaccesses it as aLaunchedEffectkey, but that can be addressed with a dedicated accessor. Consider making itinternalorprivateand exposing only what's needed (e.g., a method to check key presence).sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/ResultEffect.kt (1)
40-44: ThechannelMapkey depends on nav3 navigation recomposition; consider usingSnapshotStateMapfor robustness.Using a plain
MutableMapas aLaunchedEffectkey means Compose won't observe when entries are added or removed—changes to the map itself don't trigger recomposition. This code works in the current nav3 navigation context because returning to Home triggers a full recomposition of theentry<MainDestination.Home>block, ensuring the channel is found by then. However, ifResultEffectwere used on the same screen without navigation, or if a result needed to be received before sending, the effect would silently collect nothing.For a more robust solution that doesn't depend on navigation lifecycle, use
SnapshotStateMapinstead, which properly integrates with Compose's recomposition system.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (1)
22-27:@Suppress("UnusedPrivateMember")targets private members, butnavigatorandviewModelare public parameters.The lint warning for unused parameters in a public composable function is typically
UnusedParameteror a different detekt/lint rule.UnusedPrivateMembermay not actually suppress the intended warning here. Consider using the correct suppress target or adding aTODOto wirenavigator/viewModelin a follow-up.sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.kt (2)
42-44: Misleading comment on line 43.The comment says "if it's path arg is not the expected type" but the condition actually checks literal path-segment mismatch (i.e., the non-parameterized segments don't match). Also has a typo: "it's" → "the".
Proposed fix
- } else if (requestedSegment != candidateSegment.stringValue) { - // if it's path arg is not the expected type, its not a match + } else if (requestedSegment != candidateSegment.stringValue) { + // literal path segment mismatch, not a match
47-60: Missing query params from the request are silently ignored.If
deepLinkPattern.queryValueParsersdeclares a required query parameter that the request URL omits, it won't appear inargs. This may cause a deserialization failure downstream inKeyDecoder. Consider validating that all expected query params are present, or document that missing query params are intentional (optional).sample-compose/app/src/main/java/co/nimblehq/sample/compose/di/modules/main/MainActivityModule.kt (1)
39-44:ResultEffect<Boolean>relies onT::class.toString()as the key — fragile if more Boolean-typed results are added.Currently only one result type (
Boolean) is used, but if another screen sends aBooleanresult with a different semantic meaning, the keys will collide. Consider using an explicit, descriptiveresultKeyparameter (e.g.,resultKey = "second_screen_update") to make the contract explicit and avoid future key collisions.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.kt (2)
34-34: Minor: trailing semicolon is not idiomatic Kotlin.Proposed fix
-private const val TWEEN_DURATION_IN_MILLIS = 500; +private const val TWEEN_DURATION_IN_MILLIS = 500
59-96:popTransitionSpecandpredictivePopTransitionSpecare identical — consider extracting to a shared variable.Both blocks define the same slide-in-from-left / slide-out-to-right animation. Extracting a shared transition spec reduces duplication and keeps them in sync.
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
There was a problem hiding this comment.
🧹 Nitpick comments (9)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkPattern.kt (1)
57-57:varcan beval—resultis never reassigned.Proposed fix
- var result = regexPatternFillIn.find(segment) + val result = regexPatternFillIn.find(segment)sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.kt (2)
81-81:TAG_LOG_ERRORis unnecessarily public.This constant is only used internally for logging within this package. Consider making it
internalorprivateto avoid polluting the public API surface.Proposed fix
-const val TAG_LOG_ERROR = "SampleComposeDeepLink" +internal const val TAG_LOG_ERROR = "SampleComposeDeepLink"
42-44: Misleading comment on static segment mismatch.The comment says "if it's path arg is not the expected type" but this branch handles the case where a static path segment doesn't match. The comment should reflect that.
Proposed fix
} else if (requestedSegment != candidateSegment.stringValue) { - // if it's path arg is not the expected type, its not a match + // if the static segment doesn't match, it's not a match return nullsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.kt (2)
34-34: Remove trailing semicolon — not idiomatic Kotlin.Proposed fix
-private const val TWEEN_DURATION_IN_MILLIS = 500; +private const val TWEEN_DURATION_IN_MILLIS = 500
77-94:popTransitionSpecandpredictivePopTransitionSpecare identical — consider extracting.Both specs define the same slide animation. Extracting a shared variable would reduce duplication.
Proposed fix
Define the shared spec at the top of
onCreateor as a file-level value, then reference it for both parameters:+ val popSpec: AnimatedContentTransitionScope<Any>.() -> ContentTransform = { + slideInHorizontally( + initialOffsetX = { -it }, + animationSpec = tween(TWEEN_DURATION_IN_MILLIS) + ) togetherWith slideOutHorizontally( + targetOffsetX = { it }, + animationSpec = tween(TWEEN_DURATION_IN_MILLIS) + ) + } NavDisplay( ... - 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, )sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt (1)
23-37:navigatorparameter is unused — suppressed via annotation.The
@Suppress("UnusedPrivateMember")appears to be covering the unusednavigatorparameter. If this is kept purely for API consistency with other screens (HomeScreen, ThirdScreen), consider adding a brief comment explaining why, so future maintainers don't remove it.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.kt (1)
44-44: TheviewModel.navigatorflow name is misleading.A flow that emits navigation destinations is named
navigator, which can be confused with the actualNavigatorinstance. Consider renaming it in the ViewModel to something likenavigationEventordestinationto clarify intent. This is in the ViewModel (not this file), but worth noting since both are used on the same line.sample-compose/gradle/libs.versions.toml (2)
28-28: Consider updating Navigation 3 to1.0.1.The Navigation3 library is now stable, and version
1.0.0is valid. However, from Navigation3 1.0.1, Navigation3 now depends on NavigationEvent 1.0.2, which "fixes an IllegalStateException caused by using NavDisplay during AndroidStudio Previews." The official get-started guide now recommendsnav3Core = "1.0.1".Suggested fix
-navigation3 = "1.0.0" +navigation3 = "1.0.1"
24-24:kotlinx-serialization-json1.7.3 is significantly outdated for Kotlin 2.1.10.Version 1.7.3 was released on Sep 19, 2024, and was built for Kotlin 2.0.0. With this project using Kotlin 2.1.10, there are much newer releases available: 1.8.1 (Mar 2025), 1.9.0 (Jun 2025). The official Navigation 3 get-started guide recommends
kotlinxSerializationCore = "1.9.0".While 1.7.3 may work due to forward compatibility, using version 1.9.0 would align with Google's own recommendation for Navigation 3 projects and ensure better compatibility with Kotlin 2.1.x.
Suggested fix
-kotlinxSerialization = "1.7.3" +kotlinxSerialization = "1.9.0"
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
e995ee4 to
8d0ceae
Compare
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
|
Also, shouldn't we work on |
|
@kaungkhantsoe I aim to migrate to nav3 step by step else the PR size will be too big, so I will work on template-compose on a separate PR, we could determine the order later when we merge, wdyt? |
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
|
@eeeeaa I am good with merging the template first 🤝 |
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
26dbd65 to
d19f547
Compare
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.kt (1)
79-96: Consider extracting duplicated pop transition specs.The blocks on Line 79-87 and Line 88-96 are identical. A shared helper would reduce drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.kt` around lines 79 - 96, The two identical lambda blocks assigned to popTransitionSpec and predictivePopTransitionSpec should be extracted into a single helper to avoid duplication: create a reusable function or val (e.g., createPopTransitionSpec or popTransitionSpecHelper) that returns the combined slideInHorizontally togetherWith slideOutHorizontally using TWEEN_DURATION_IN_MILLIS, then replace both popTransitionSpec and predictivePopTransitionSpec usages with that helper; reference the existing symbols popTransitionSpec, predictivePopTransitionSpec, slideInHorizontally, slideOutHorizontally and TWEEN_DURATION_IN_MILLIS when implementing the change.sample-compose/app/src/main/java/co/nimblehq/sample/compose/di/modules/main/MainActivityModule.kt (1)
41-46: Use an explicit result key for this update event.On Line 41/45/62, the default
Boolean-typed key is used. This can cause cross-screen collisions when another Boolean result channel is introduced.♻️ Proposed refactor
object MainActivityModule { + private const val RESULT_KEY_SECOND_UPDATED = "result_second_updated" `@Provides` `@ActivityRetainedScoped` fun provideNavigator(): Navigator = NavigatorImpl(startDestination = Home) @@ - ResultEffect<Boolean> { isUpdated -> + ResultEffect<Boolean>(resultKey = RESULT_KEY_SECOND_UPDATED) { isUpdated -> if (isUpdated) { context.showToast(context.getString(R.string.message_updated)) } - eventBus.removeResult<Boolean>() + eventBus.removeResult<Boolean>(resultKey = RESULT_KEY_SECOND_UPDATED) } @@ onUpdateClick = { - eventBus.sendResult<Boolean>(result = true) + eventBus.sendResult<Boolean>( + resultKey = RESULT_KEY_SECOND_UPDATED, + result = true + ) navigator.goBack() } ) }Also applies to: 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-compose/app/src/main/java/co/nimblehq/sample/compose/di/modules/main/MainActivityModule.kt` around lines 41 - 46, The ResultEffect<Boolean> block uses the implicit Boolean-typed default key which can collide across screens; change it to use an explicit result key constant (e.g., val UPDATE_RESULT_KEY = "update_result") and pass that key into ResultEffect<Boolean>(key = UPDATE_RESULT_KEY) and into eventBus.removeResult<Boolean>(UPDATE_RESULT_KEY); update both occurrences (the ResultEffect<Boolean> where context.showToast(context.getString(R.string.message_updated)) is called and the matching eventBus.removeResult<Boolean> calls) so the result channel is uniquely namespaced.sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt (1)
22-23: AlignThirdScreennullability withThirdkey contract.Line 22 defines
Third.modelas non-null, but Line 27/38 accept nullable values. Tightening this keeps the API consistent and avoids accidental"null"rendering.♻️ Proposed refactor
data class Third(val model: UiModel) `@Suppress`("UnusedPrivateMember") `@Composable` fun ThirdScreen( - model: UiModel?, + model: UiModel, navigator: Navigator, viewModel: ThirdViewModel = hiltViewModel(), ) = BaseScreen( @@ private fun ThirdScreenContent( - data: UiModel?, + data: UiModel, modifier: Modifier = Modifier, ) {Also applies to: 27-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt` around lines 22 - 23, ThirdScreen currently accepts nullable models while the Third data class declares model as non-null (data class Third(val model: UiModel)); update the API to be consistent by changing ThirdScreen’s parameters and any call sites to require a non-null Third (or non-null UiModel) instead of nullable types—locate the ThirdScreen composable/function that takes a nullable Third or UiModel and make its parameter non-null, remove null checks or default "null" rendering, and ensure callers always pass a Third (or Third.model) that is non-null to match data class Third(val model: UiModel).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.kt`:
- Around line 44-47: The deep-link key construction currently force-unwraps
args["id"] in the DeepLinkPattern keyBuilder (and keyBuilder is invoked
unguarded elsewhere), which can crash on malformed links; update the
DeepLinkPattern entries in deepLinkPatterns so keyBuilder safely handles a
missing or non-numeric "id" (e.g., parse/validate args["id"] and return a
nullable key or a Result) and then update the place that executes keyBuilder
(the code that calls keyBuilder around lines 110-115) to check the returned
value for null/failure before using it (skip navigation or show an error)
instead of assuming a non-null Second(id = ...); reference DeepLinkPattern,
keyBuilder, Second, and deepLinkPatterns when making the changes.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.kt`:
- Around line 13-16: Validate the incoming URI's scheme and authority/host
before comparing path segments: in DeepLinkMatcher.kt, before the existing
pathSegments.size check and before the argument-matching block (the blocks
around request.pathSegments vs deepLinkPattern.pathSegments and the later 20-35
logic), first compare request.scheme (or request.uri.scheme) and
request.authority/host with deepLinkPattern's expected scheme/authority; if they
differ return null. Apply the same scheme/authority check in both the
exact-match branch that returns DeepLinkMatchResult(deepLinkPattern.keyBuilder,
mapOf()) and in the argument-extraction branch so URIs from other hosts/schemes
with identical path shapes are rejected.
---
Nitpick comments:
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/di/modules/main/MainActivityModule.kt`:
- Around line 41-46: The ResultEffect<Boolean> block uses the implicit
Boolean-typed default key which can collide across screens; change it to use an
explicit result key constant (e.g., val UPDATE_RESULT_KEY = "update_result") and
pass that key into ResultEffect<Boolean>(key = UPDATE_RESULT_KEY) and into
eventBus.removeResult<Boolean>(UPDATE_RESULT_KEY); update both occurrences (the
ResultEffect<Boolean> where
context.showToast(context.getString(R.string.message_updated)) is called and the
matching eventBus.removeResult<Boolean> calls) so the result channel is uniquely
namespaced.
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt`:
- Around line 22-23: ThirdScreen currently accepts nullable models while the
Third data class declares model as non-null (data class Third(val model:
UiModel)); update the API to be consistent by changing ThirdScreen’s parameters
and any call sites to require a non-null Third (or non-null UiModel) instead of
nullable types—locate the ThirdScreen composable/function that takes a nullable
Third or UiModel and make its parameter non-null, remove null checks or default
"null" rendering, and ensure callers always pass a Third (or Third.model) that
is non-null to match data class Third(val model: UiModel).
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.kt`:
- Around line 79-96: The two identical lambda blocks assigned to
popTransitionSpec and predictivePopTransitionSpec should be extracted into a
single helper to avoid duplication: create a reusable function or val (e.g.,
createPopTransitionSpec or popTransitionSpecHelper) that returns the combined
slideInHorizontally togetherWith slideOutHorizontally using
TWEEN_DURATION_IN_MILLIS, then replace both popTransitionSpec and
predictivePopTransitionSpec usages with that helper; reference the existing
symbols popTransitionSpec, predictivePopTransitionSpec, slideInHorizontally,
slideOutHorizontally and TWEEN_DURATION_IN_MILLIS when implementing the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
sample-compose/app/src/androidTest/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/di/modules/main/MainActivityModule.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/MainDestination.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModel.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkPattern.ktsample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.ktsample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModelTest.kt
💤 Files with no reviewable changes (1)
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/MainDestination.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.kt
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModel.kt
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkPattern.kt
- sample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModelTest.kt
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.kt (1)
13-16:⚠️ Potential issue | 🟠 MajorReject cross-host/cross-scheme URIs before path matching.
At Line 13, matching starts with path shape only. A URI from a different scheme/authority but same path pattern can still match and build a key.
🛡️ Proposed fix
fun match(): DeepLinkMatchResult<T>? { + if (request.uri.scheme != deepLinkPattern.uriPattern.scheme) return null + if (request.uri.authority != deepLinkPattern.uriPattern.authority) return null if (request.pathSegments.size != deepLinkPattern.pathSegments.size) return null // exact match (url does not contain any arguments) if (request.uri == deepLinkPattern.uriPattern) return DeepLinkMatchResult(deepLinkPattern.keyBuilder, mapOf())Also applies to: 20-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.kt` around lines 13 - 16, The matcher currently compares only path shape (request.pathSegments vs deepLinkPattern.pathSegments) before constructing a key, allowing URIs with different schemes/hosts to match; update the matching logic in DeepLinkMatcher (the block using request.pathSegments, request.uri and deepLinkPattern.uriPattern and the later matching logic around lines ~20-35) to first compare scheme and authority/host between the incoming request and the deepLinkPattern (reject if scheme or host differ), and only then proceed to path-segment or exact uriPattern comparisons and return DeepLinkMatchResult(keyBuilder, ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@sample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.kt`:
- Around line 13-16: The matcher currently compares only path shape
(request.pathSegments vs deepLinkPattern.pathSegments) before constructing a
key, allowing URIs with different schemes/hosts to match; update the matching
logic in DeepLinkMatcher (the block using request.pathSegments, request.uri and
deepLinkPattern.uriPattern and the later matching logic around lines ~20-35) to
first compare scheme and authority/host between the incoming request and the
deepLinkPattern (reject if scheme or host differ), and only then proceed to
path-segment or exact uriPattern comparisons and return
DeepLinkMatchResult(keyBuilder, ...).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
sample-compose/app/src/androidTest/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/di/modules/main/MainActivityModule.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/MainActivity.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/MainDestination.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModel.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/second/SecondScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkMatcher.ktsample-compose/app/src/main/java/co/nimblehq/sample/compose/util/DeepLinkPattern.ktsample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeScreenTest.ktsample-compose/app/src/test/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModelTest.kt
💤 Files with no reviewable changes (1)
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/MainDestination.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/third/ThirdScreen.kt
- sample-compose/app/src/main/java/co/nimblehq/sample/compose/ui/screens/main/home/HomeViewModel.kt
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
d19f547 to
aba7ac7
Compare
eeeeaa
had a problem deploying
to
template-compose
GitHub Actions
Error
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
aba7ac7 to
7492383
Compare
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
temporarily deployed
to
template-compose
GitHub Actions
Inactive
eeeeaa
force-pushed
the
chore/#617-migrate-to-navigation-3
branch
from
ac0c62b to
2d22049
Compare
3 participants
#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 📹
Sample project work the same as before migration
Screen.Recording.2569-02-11.at.12.04.04.mov
Summary by CodeRabbit
New Features
Refactor
Tests
Chores