-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Architectural Revamp #99
Conversation
This commit refactors the initialization of viewState in components by extracting it from the `onStart` method and assigning it directly to the `viewState` property. This change aims to improve clarity and maintainability of the code. Additionally, a new `whenStarted` extension function is introduced in the BaseComponent class. This function allows components to perform actions when their lifecycle starts while providing a StateFlow representation of their viewState. This further simplifies the initialization process and improves readability.
…avEntry interfaces These changes simplify the codebase by removing unnecessary abstractions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
shared/feature/src/androidMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostDefaults.android.kt (1)
3-15
: Implementation aligns well with Android navigation patternsThe Android-specific implementation correctly follows the platform's lazy loading pattern for bottom navigation tabs. The documentation clearly explains the behavioral differences between platforms, which is crucial for maintainability.
Consider adding a code comment explaining that this implementation supports Android's memory-efficient approach by avoiding unnecessary initialization of inactive bottom navigation destinations.
shared/feature/src/iosMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostDefaults.ios.kt (1)
3-18
: iOS implementation follows platform-specific navigation patternsThe implementation correctly:
- Eagerly initializes all bottom navigation tabs as required by iOS
- Maintains the complete navigation stack
- Places the initial config at the end of the list, making it the default selected tab
This approach aligns well with iOS navigation patterns and the PR's goal of platform-specific optimizations. Consider documenting the rationale for maintaining the complete navigation stack on iOS, perhaps in a shared documentation file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
shared/feature/src/androidMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostDefaults.android.kt
(1 hunks)shared/feature/src/iosMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostDefaults.ios.kt
(1 hunks)
🔇 Additional comments (1)
shared/feature/src/iosMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostDefaults.ios.kt (1)
14-17
: Verify handling of all SignedInConfig cases
The when
expression should handle all possible SignedInConfig
cases. If new navigation destinations are added in the future, this implementation needs to be updated.
...ure/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt
Outdated
Show resolved
Hide resolved
...ature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/thirdScreen/ThirdComponent.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt (1)
46-56
: Ensurewhen
expression is exhaustiveConsider making the
when
expression exhaustive by adding anelse
branch or handling all possible cases ofSecondScreen.PickerType
. This will enhance future maintainability and prevent potential runtime exceptions if new picker types are introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt
(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/thirdScreen/ThirdComponent.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/thirdScreen/ThirdComponent.kt
🔇 Additional comments (1)
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt (1)
1-66
: SecondComponent
implementation aligns with the new architectural design
The implementation correctly adopts the new component-based architecture, integrating retained components and streamlined navigation actions. This enhances modularity and simplifies state management.
...tekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/ext/DecomposeValueExt.kt
Outdated
Show resolved
Hide resolved
Implemented suggestion from #99: New `ValueStateFlow<T>` wrapper that implements `StateFlow<T>` interface. The `Value<T>.asStateFlow(): StateFlow<T>` extension no longer requires `CoroutineContext`.
@@ -0,0 +1,10 @@ | |||
package app.futured.arkitekt.decompose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arkitect-decompose
and arkitekt-cr-usecases
packages has to be added to init_template.kts as packages to be renamed in fun renamePackagesInShared
☝️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we should want to rename the app.futured.arkitekt.*
package to the application one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor asStateFlow() extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/coroutines/ValueStateFlow.kt (1)
21-21
: Consider caching the replay list.The current implementation creates a new list on each access. Consider storing it as a property to avoid unnecessary object creation.
- override val replayCache: List<T> = listOf(value) + override val replayCache: List<T> by lazy { listOf(value) }shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/home/HomeNavHostComponent.kt (2)
33-36
: Add KDoc to document navigation actionsThe navigation actions would benefit from documentation explaining:
- The expected format and constraints of the newStack parameter
- Any validation or error handling that occurs
- Examples of typical usage
override val actions: HomeNavHost.Actions = object : HomeNavHost.Actions { + /** + * Updates the navigation stack with a new list of configurations. + * @param newStack The new navigation stack to replace the current one. + * Must not be empty and must contain valid configurations. + */ override fun navigate(newStack: List<Child<HomeConfig, HomeChild>>) = homeNavigator.navigate { newStack.map { it.configuration } } + /** + * Removes the top configuration from the navigation stack. + * No-op if the stack only contains one item. + */ override fun pop() = homeNavigator.pop() }
1-77
: Strong architectural implementation with room for enhancementThe implementation successfully achieves the PR objectives:
- Components are retained and independent
- Navigation is simplified using lambdas
- Dependency injection is properly utilized
- Clear separation of concerns
Consider these architectural improvements:
- Add error handling for navigation edge cases (empty stack, invalid configurations)
- Consider implementing a navigation event tracking mechanism for analytics
- Document the navigation contract between components in the interface file
Would you like assistance in implementing any of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/coroutines/ValueStateFlow.kt
(1 hunks)shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/ext/DecomposeValueExt.kt
(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/home/HomeNavHostComponent.kt
(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/profile/ProfileNavHostComponent.kt
(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/root/RootNavHostComponent.kt
(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostComponent.kt
(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/profile/ProfileNavHostComponent.kt
- shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostComponent.kt
🔇 Additional comments (9)
shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/ext/DecomposeValueExt.kt (1)
9-13
: LGTM! Documentation and implementation are clear and concise.
The KDoc documentation clearly describes the purpose and return type of the extension function. The implementation follows the suggested approach from the previous review, removing the unnecessary CoroutineScope dependency.
shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/coroutines/ValueStateFlow.kt (1)
23-32
: LGTM! Clean implementation with proper resource management.
The collect implementation:
- Properly manages resources with try-finally
- Correctly handles value propagation
- Ensures subscription cancellation
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/home/HomeNavHostComponent.kt (3)
44-75
: Well-structured navigation configuration
The navigation setup for each screen is clean and follows a consistent pattern:
- Clear separation of concerns between screens
- Unidirectional navigation flow
- Type-safe navigation using sealed configurations
- Proper dependency injection of navigation callbacks
This implementation aligns well with the PR objectives of simplifying navigation and making components more independent.
26-29
: Consider using a more specific output type instead of Nothing
Using Nothing
as the output type means this component can never emit any events to its parent. Consider if this is intentional or if there might be cases where the home navigation needs to communicate upwards (e.g., sign out events, global navigation events).
Let's check if other nav hosts follow the same pattern:
✅ Verification successful
Using Nothing as output type is consistent with the navigation pattern
Looking at the codebase, using Nothing
as the output type for navigation components is a deliberate architectural decision consistently applied across all navigation components:
RootNavHostComponent
:AppComponent<RootNavHostViewState, Nothing>
SignedInNavHostComponent
:AppComponent<SignedInNavHostViewState, Nothing>
ProfileNavHostComponent
:AppComponent<Unit, Nothing>
HomeNavHostComponent
:AppComponent<Unit, Nothing>
This pattern indicates that navigation components are designed to be self-contained, handling all navigation logic internally without needing to emit events to parent components. Navigation between screens is managed through dedicated navigation classes (e.g., HomeNavHostNavigation
, ProfileNavHostNavigation
) rather than through component events.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other NavHost components to verify the pattern
ast-grep --pattern 'class $_NavHostComponent$_($_) : AppComponent<$_, $_>($_)'
Length of output: 80
Script:
#!/bin/bash
# Let's try a broader search for NavHost-related components and classes
rg -l "NavHost" --type kotlin
# Then let's look for any AppComponent usage patterns
rg "AppComponent" -A 2 --type kotlin
Length of output: 30253
38-76
: Verify component lifecycle management
The stack configuration looks clean and follows good practices. However, we should verify that child components are properly disposed when removed from the stack to prevent memory leaks.
Let's check the component disposal implementation:
✅ Verification successful
Based on the search results, I can now generate the final response:
Component lifecycle is properly managed
The codebase implements proper component lifecycle management through Decompose's built-in mechanisms:
- Components inherit from
BaseComponent
which sets up acomponentCoroutineScope
that is automatically cancelled on component disposal:
protected val componentCoroutineScope = MainScope().also { scope ->
componentContext.lifecycle.doOnDestroy { scope.cancel() }
}
- The
AppComponentContext
properly delegates lifecycle management to Decompose'sComponentContext
- Child components are created through
AppComponentFactory
which ensures proper lifecycle integration - Platform-specific implementations (iOS/Android) correctly handle lifecycle events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for component disposal handling in AppComponent
ast-grep --pattern 'class AppComponent {
$$$
onDispose() {
$$$
}
$$$
}'
Length of output: 85
Script:
#!/bin/bash
# Let's try different approaches to find component lifecycle management
# Search for any class containing onDispose method
ast-grep --pattern 'class $_ {
$$$
onDispose($$$) {
$$$
}
$$$
}'
# Search for disposal-related patterns in the codebase
rg -i "dispose|ondispose|lifecycle" -A 3
# Look for Decompose's ComponentContext usage which handles lifecycle
rg "ComponentContext" -A 3
# Search for any interface or class related to component lifecycle
ast-grep --pattern 'interface $_ {
$$$
onDispose($$$)
$$$
}'
Length of output: 54815
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/root/RootNavHostComponent.kt (3)
87-95
: Potential issue: 'pendingDeepLink' is not cleared after consumption
In the consumeDeepLink()
method, pendingDeepLink
is not set to null
after the deep link has been processed. This could result in the same deep link being processed multiple times if consumeDeepLink()
is called again.
Apply this diff to clear pendingDeepLink
after consumption:
87 private fun consumeDeepLink(): Boolean {
88 val deepLink = pendingDeepLink ?: return false
89 when (deepLink) {
90 DeepLinkDestination.HomeTab -> slotNavigator.activate(RootConfig.deepLinkHome())
91 DeepLinkDestination.ProfileTab -> slotNavigator.activate(RootConfig.deepLinkProfile())
92 DeepLinkDestination.SecondScreen -> slotNavigator.activate(RootConfig.deepLinkSecondScreen())
93 is DeepLinkDestination.ThirdScreen -> slotNavigator.activate(
94 RootConfig.deepLinkThirdScreen(ThirdScreenArgs(deepLink.argument))
95 )
+ }
+ pendingDeepLink = null
96 return true
97 }
26-28
: Constructor parameters are well-defined and promote dependency injection
The use of @InjectedParam
for componentContext
and constructor injection for deepLinkResolver
enhances testability and maintainability of the component.
66-71
: Initialization logic appropriately handles deep links
The init
block effectively checks for any pending deep links upon creation and activates the corresponding screen, defaulting to the Login
screen if none are present.
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt (1)
21-66
: Implementation of SecondComponent
is Well-Structured and Correct
The SecondComponent
class is correctly implemented, effectively utilizing SlotNavigation
and childSlot
for managing navigation and state. The use of StateFlow
for viewState
and picker
provides efficient and reactive state management. The navigation actions are properly defined, and the dependency injection with @InjectedParam
annotations aligns with best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
init_template.kts (1)
Line range hint
238-253
: Correct the regular expression inupdateFastfileEnvVariables
The regular expression pattern in
updateFastfileEnvVariables
may not correctly escape special characters, potentially leading to incorrect matches. Use a raw string to define the regex pattern for clarity and correctness.Apply this diff to correct the regex pattern:
fun updateFastfileEnvVariables(file: File, varName: String, newValue: String) { if (!file.exists()) { println("File does not exist: $file") return } var content = file.readText() // Look for the pattern ENV['VAR_NAME'] = 'var_value' and change 'var_value' - val envVarPattern = "ENV\\['$varName'] = '(.*)'".toRegex() + val envVarPattern = """ENV\['$varName'\] = '(.*)'""".toRegex() if (envVarPattern.containsMatchIn(content)) { // The variable is found in the file - replace the old value with the new value content = content.replace(envVarPattern, "ENV['$varName'] = '$newValue'") file.writeText(content) } else { println("Variable $varName was not found in Fastfile.") } }
🧹 Nitpick comments (1)
init_template.kts (1)
Line range hint
257-277
: Enhance user prompts inreadInput
functionIn the
readInput
function, consider providing clearer prompts to guide the user input. This improves user experience by specifying the required format for inputs, especially for the package name.Apply this diff to enhance the prompts:
print("Project name: ") val appName: String = readLine() ?.takeIf { it.isNotBlank() } ?.replace(" ", "_") ?: error("You need to enter name") -print("Package name (e.g. com.example.test): ") +print("Package name (e.g., com.example.app): ") val packageName = readLine() ?.takeIf { it.isNotBlank() } ?: error("You need to enter package name") print("Framework name: (default 'shared'): ")
File("iosApp/iosAppTests/iosAppTests.swift").toPath(), | ||
File("iosApp/iosAppTests/${appName}Tests.swift").toPath(), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential exceptions when moving files
When using Files.move
to rename files, exceptions may occur if the source files do not exist or are not accessible. To prevent the script from crashing, add checks to ensure that the source files exist before attempting to move them.
Apply this diff to add existence checks:
+if (Files.exists(File("iosApp/iosAppTests/iosAppTests.swift").toPath())) {
Files.move(
File("iosApp/iosAppTests/iosAppTests.swift").toPath(),
File("iosApp/iosAppTests/${appName}Tests.swift").toPath(),
)
+} else {
+ println("Source file does not exist: iosAppTests.swift")
+}
+if (Files.exists(File("iosApp/iosAppUITests/iosAppUITestsLaunchTests.swift").toPath())) {
Files.move(
File("iosApp/iosAppUITests/iosAppUITestsLaunchTests.swift").toPath(),
File("iosApp/iosAppUITests/${appName}UITestsLaunchTests.swift").toPath(),
)
+} else {
+ println("Source file does not exist: iosAppUITestsLaunchTests.swift")
+}
Also applies to: 155-157
fun moveFileTree(parent: Path, fromPath: Path, toPath: Path) { | ||
if (parent.exists().not()) { | ||
return | ||
} | ||
val sourcePath = parent.resolve(fromPath) | ||
val targetPath = parent.resolve(toPath) | ||
|
||
Files.walk(parent.resolve(fromPath)) | ||
.filter { path -> path.isRegularFile() } | ||
.forEach { source -> | ||
val target = targetPath.resolve(sourcePath.relativize(source)) | ||
Files.createDirectories(target.parent) | ||
Files.move(source, target, StandardCopyOption.REPLACE_EXISTING) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add existence check for sourcePath
to prevent exceptions
In the moveFileTree
function, if the sourcePath
does not exist, Files.walk(sourcePath)
will throw an exception. Adding a check to verify that sourcePath
exists before proceeding will make the function more robust.
Apply this diff to add the existence check:
fun moveFileTree(parent: Path, fromPath: Path, toPath: Path) {
if (parent.exists().not()) {
return
}
val sourcePath = parent.resolve(fromPath)
val targetPath = parent.resolve(toPath)
+ if (sourcePath.exists().not()) {
+ return
+ }
Files.walk(sourcePath)
.filter { path -> path.isRegularFile() }
.forEach { source ->
val target = targetPath.resolve(sourcePath.relativize(source))
Files.createDirectories(target.parent)
Files.move(source, target, StandardCopyOption.REPLACE_EXISTING)
}
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fun moveFileTree(parent: Path, fromPath: Path, toPath: Path) { | |
if (parent.exists().not()) { | |
return | |
} | |
val sourcePath = parent.resolve(fromPath) | |
val targetPath = parent.resolve(toPath) | |
Files.walk(parent.resolve(fromPath)) | |
.filter { path -> path.isRegularFile() } | |
.forEach { source -> | |
val target = targetPath.resolve(sourcePath.relativize(source)) | |
Files.createDirectories(target.parent) | |
Files.move(source, target, StandardCopyOption.REPLACE_EXISTING) | |
} | |
} | |
fun moveFileTree(parent: Path, fromPath: Path, toPath: Path) { | |
if (parent.exists().not()) { | |
return | |
} | |
val sourcePath = parent.resolve(fromPath) | |
val targetPath = parent.resolve(toPath) | |
if (sourcePath.exists().not()) { | |
return | |
} | |
Files.walk(sourcePath) | |
.filter { path -> path.isRegularFile() } | |
.forEach { source -> | |
val target = targetPath.resolve(sourcePath.relativize(source)) | |
Files.createDirectories(target.parent) | |
Files.move(source, target, StandardCopyOption.REPLACE_EXISTING) | |
} | |
} |
# Conflicts: # init_template.kts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
init_template.kts (2)
145-148
:⚠️ Potential issueHandle potential exceptions when moving files
When using
Files.move
to rename files, exceptions may occur if the source files do not exist or are not accessible.Apply this diff to add existence checks:
+val sourceFile = File("iosApp/iosAppTests/iosAppTests.swift").toPath() +if (Files.exists(sourceFile)) { Files.move( - File("iosApp/iosAppTests/iosAppTests.swift").toPath(), + sourceFile, File("iosApp/iosAppTests/${appName}Tests.swift").toPath(), ) +} else { + println("Source file does not exist: iosAppTests.swift") +}Also applies to: 154-157
229-243
:⚠️ Potential issueAdd existence check for sourcePath to prevent exceptions
The
moveFileTree
function should verify the existence ofsourcePath
before attempting to walk through it.Apply this diff to add the existence check:
fun moveFileTree(parent: Path, fromPath: Path, toPath: Path) { if (parent.exists().not()) { return } val sourcePath = parent.resolve(fromPath) val targetPath = parent.resolve(toPath) + if (sourcePath.exists().not()) { + println("Source path does not exist: $sourcePath") + return + } Files.walk(sourcePath) .filter { path -> path.isRegularFile() } .forEach { source -> val target = targetPath.resolve(sourcePath.relativize(source)) Files.createDirectories(target.parent) Files.move(source, target, StandardCopyOption.REPLACE_EXISTING) } }
🧹 Nitpick comments (4)
init_template.kts (4)
18-19
: Consider deriving templatePackagePath from templatePackageNameInstead of hardcoding the path, consider deriving it from
templatePackageName
:-val templatePackagePath: Path = Path.of("app/futured/kmptemplate") +val templatePackagePath: Path = Path.of(templatePackageName.replace('.', '/'))
25-34
: Add logging for subproject renaming operationsConsider adding logging to track the progress of renaming operations, which would help in debugging if issues occur.
+println("Renaming Gradle subprojects...") renameGradleSubproject("buildSrc", appPackageName) +println("✓ Renamed buildSrc") renameGradleSubproject("baselineprofile", appPackageName) +println("✓ Renamed baselineprofile") // ... (similar for other subprojects)
168-169
: Add error handling for file deletion operationsThe file deletion operations should handle potential errors and provide feedback.
-File("LICENSE").delete() -File("init_template.kts").delete() +listOf("LICENSE", "init_template.kts").forEach { filename -> + val file = File(filename) + if (file.exists()) { + if (file.delete()) { + println("✓ Deleted $filename") + } else { + println("Failed to delete $filename") + } + } else { + println("File not found: $filename") + } +}
Line range hint
264-283
: Enhance input validation in readInput functionThe input validation could be more robust with pattern matching and better error messages.
fun readInput(): Triple<String, String, String> { print("Project name: ") val appName: String = readLine() ?.takeIf { it.isNotBlank() } ?.replace(" ", "_") - ?: error("You need to enter name") + ?.takeIf { it.matches("[a-zA-Z][a-zA-Z0-9_]*".toRegex()) } + ?: error("Invalid project name. Must start with a letter and contain only letters, numbers, and underscores") print("Package name (e.g. com.example.test): ") val packageName = readLine() ?.takeIf { it.isNotBlank() } + ?.takeIf { it.matches("[a-z][a-z0-9_]*(\\.[a-z][a-z0-9_]*)*".toRegex()) } - ?: error("You need to enter package name") + ?: error("Invalid package name. Must be in format: com.example.test") print("Framework name: (default 'shared'): ") val frameworkName = readLine() ?.takeIf { it.isNotBlank() } ?.replace(" ", "_") + ?.takeIf { it.matches("[a-zA-Z][a-zA-Z0-9_]*".toRegex()) } ?: "shared" return Triple(appName, packageName, frameworkName) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/android_google_play_release.yml
(3 hunks)init_template.kts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/android_google_play_release.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Android PR check
- GitHub Check: test / test
Generated by 🚫 Danger |
Architectural Revamp
This is a proposal for the new Futured KMP architecture.
History
During our KMP journey over last few years, we've iterated over several architectural and navigation patterns using Decompose.
First, we started using Components as
InstanceKeeper
wrappers over ViewModels - which held presentation logic and survived configuration changes. For performing navigation, we used events emitted from ViewModel into parent Components using Flows. This resulted in rather large amount of boilerplate code especially in multi-level Component trees. The groundwork was there, we needed to simplify the rather messy navigation logic.We then switched over to the concept of Navigators. These singleton-scoped objects, one for each Component that held navigation holder (such as Stack or Slot), were responsible for performing navigation actions. They could be injected directly into ViewModels to perform navigation actions basically from anywhere. They served us well, but for the cost of direct coupling between Component and Component tree it was placed in. This made us unable to really "decompose" the app into reusable Components - each screen Component was simply tied to it's navigation stack inside app and could not be used anywhere else. Also, passing results between components was complicated.
This brings us to this proposal...
What's new
This list summarizes the most notable changes.
BaseComponent
)Made changes to initialisation logic of Components. When one wants to launch some logic as a side-effect of Component initialisation, this should not be done inThe sample app now prefersinit {}
block, but by usingfun Flow<ViewState>.whenStarted(onStarted)
extension function on Component state, which would callonStarted
lambda after View subscribes to ViewState updates. This way we can control Component initialisation execution (in tests), as opposed toinit {}
, over which we do not have any control.Lifecycle.doOnCreate()
extension overinit {}
block to perform initialisation side-effects.@Factory
-scoped and are created usingAppComponentFactory
object which implementKoinComponent
interface. This approach needs to be discussed further - I can imagine having either KSP-generated injection factories tailored to each component, or to simply write factories manually. (The AppComponentFactory currently feels like magic and does not explicitly let user know what additional@InjectedParam
parameters need to be passed to the factory function).What needs to be done
AppComponentFactory
arkitekt-decompose
subproject into https://github.com/futuredapp/arkitektSummary by CodeRabbit
Release Notes
New Features
HomeNavHost
,ProfileNavHost
, andRootNavHost
for improved user navigation experience.Picker
UI component for selecting items, enhancing user interaction.DeepLink
handling for seamless navigation via URLs.SignedInNavHost
andSignedInNavHostComponent
for managing signed-in user navigation.FruitPickerComponent
andVegetablePickerComponent
for improved item selection.Bug Fixes
Documentation
Refactor
Tests
DeepLinkResolver
to ensure reliable deep link handling.Chores