Skip to content
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

Merged
merged 54 commits into from
Jan 10, 2025
Merged

Architectural Revamp #99

merged 54 commits into from
Jan 10, 2025

Conversation

matejsemancik
Copy link
Member

@matejsemancik matejsemancik commented Nov 26, 2024

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.

  • All components are now retained. This is assumed to be safe in a way we use and structure our architecture, because we never inject Android Activity, or any other dependencies that could potentially leak from Components. This change also enables us to implement features described in next points.
  • Components become "ViewModels". Since the Component itself is retained, it makes ViewModel previously kept in InstanceKeeper, unnecessary. All presentation logic is now moved directly to Component (subclass of BaseComponent)
  • Ditched the concept of Navigators. Instead, we provide a set of NavigationAction lambdas to each component. These actions are specific for each component and describe a set of navigation actions that can be performed by this component. It is responsibility of parent component to handle invocations of these functions by performing necessary mutations to the navigation, or handle them in any other suitable way. This change ultimately makes Components reusable and independent of the navigation tree they are placed in. This approach imposes little bit more of boilerplate as compared to Navigators, however not as much as compared to the first version with events. (In my opinion, this is small price to pay for flexibility).
  • 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 in init {} block, but by using fun Flow<ViewState>.whenStarted(onStarted) extension function on Component state, which would call onStarted lambda after View subscribes to ViewState updates. This way we can control Component initialisation execution (in tests), as opposed to init {}, over which we do not have any control. The sample app now prefers Lifecycle.doOnCreate() extension over init {} block to perform initialisation side-effects.
  • Components can inject dependencies using classic constructor injection. All components are @Factory-scoped and are created using AppComponentFactory object which implement KoinComponent 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).
  • Refactored the sample app and added navigation result example (Picker component)

What needs to be done

  • Document core architectural components
  • Refine API
  • (will be solved separately) Create a developer documentation (using mkdocs) describing common navigation and presentation use-cases for new team members
  • Make decision about AppComponentFactory
    • We will investigate KSP
  • Update README.md
  • (will be solved separately) Have discussion about extracting arkitekt-decompose subproject into https://github.com/futuredapp/arkitekt

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new navigation components: HomeNavHost, ProfileNavHost, and RootNavHost for improved user navigation experience.
    • Implemented a Picker UI component for selecting items, enhancing user interaction.
    • Added DeepLink handling for seamless navigation via URLs.
    • Added new composable functions for home and profile navigation, improving the user experience.
    • Introduced SignedInNavHost and SignedInNavHostComponent for managing signed-in user navigation.
    • Added FruitPickerComponent and VegetablePickerComponent for improved item selection.
  • Bug Fixes

    • Resolved issues related to deep link navigation and state management.
  • Documentation

    • Updated README to reflect architectural changes and new navigation structures.
  • Refactor

    • Streamlined navigation logic across various components for better maintainability.
    • Enhanced state management and error handling in API interactions.
    • Updated various components to use new navigation patterns and state management techniques.
  • Tests

    • Added unit tests for DeepLinkResolver to ensure reliable deep link handling.
    • Introduced tests for new navigation components to validate functionality.
  • Chores

    • Updated dependency versions for improved performance and security.

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.
Copy link

@coderabbitai coderabbitai bot left a 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 patterns

The 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 patterns

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1e619da and 9244cc7.

📒 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.

Syntey
Syntey previously approved these changes Dec 5, 2024
Copy link

@coderabbitai coderabbitai bot left a 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: Ensure when expression is exhaustive

Consider making the when expression exhaustive by adding an else branch or handling all possible cases of SecondScreen.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

📥 Commits

Reviewing files that changed from the base of the PR and between 9244cc7 and b2123eb.

📒 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.

Syntey
Syntey previously approved these changes Dec 5, 2024
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
Copy link
Collaborator

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 ☝️

Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link

@coderabbitai coderabbitai bot left a 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 actions

The 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 enhancement

The 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:

  1. Add error handling for navigation edge cases (empty stack, invalid configurations)
  2. Consider implementing a navigation event tracking mechanism for analytics
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between b2123eb and 730ce55.

📒 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 a componentCoroutineScope 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's ComponentContext
  • 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.

Copy link

@coderabbitai coderabbitai bot left a 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 in updateFastfileEnvVariables

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 in readInput function

In 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'): ")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 730ce55 and b8d3906.

📒 Files selected for processing (1)
  • init_template.kts (3 hunks)

Comment on lines +146 to +148
File("iosApp/iosAppTests/iosAppTests.swift").toPath(),
File("iosApp/iosAppTests/${appName}Tests.swift").toPath(),
)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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

Comment on lines +222 to +236
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)
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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)
}
}

Copy link

@coderabbitai coderabbitai bot left a 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 issue

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.

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 issue

Add existence check for sourcePath to prevent exceptions

The moveFileTree function should verify the existence of sourcePath 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 templatePackageName

Instead 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 operations

Consider 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 operations

The 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 function

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between b8d3906 and a1f18db.

📒 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

Copy link

6 Warnings
⚠️ Feature or fix PR title should include JIRA-ID and short description.
⚠️ Feature or fix PR branch name should include JIRA-ID and short description.
⚠️ This pull request is too big.
⚠️ KLIB resolver: The same 'unique_name=annotation_commonMain' found in more than one library: /Users/runnerm1/actions-runner/_work/kmp-futured-template/kmp-futured-template/shared/feature/build/kotlinTransformedMetadataLibraries/nativeMain/androidx.annotation-annotation-1.7.0-commonMain-kNhS6A.klib, /Users/runnerm1/actions-runner/_work/kmp-futured-template/kmp-futured-template/shared/feature/build/kotlinTransformedMetadataLibraries/nativeMain/org.jetbrains.compose.annotation-internal-annotation-1.7.0-commonMain-BD0V2w.klib
⚠️ KLIB resolver: The same 'unique_name=resources_commonMain' found in more than one library: /Users/runnerm1/actions-runner/_work/kmp-futured-template/kmp-futured-template/shared/feature/build/kotlinTransformedMetadataLibraries/commonMain/dev.icerock.moko-resources-0.24.0-commonMain-3vAcDQ.klib, /Users/runnerm1/actions-runner/_work/kmp-futured-template/kmp-futured-template/shared/resources/build/classes/kotlin/metadata/commonMain
⚠️ KLIB resolver: The same 'unique_name=runtime_commonMain' found in more than one library: /Users/runnerm1/actions-runner/_work/kmp-futured-template/kmp-futured-template/shared/feature/build/kotlinTransformedMetadataLibraries/commonMain/org.jetbrains.compose.runtime-runtime-1.7.0-commonMain-9pDeVQ.klib, /Users/runnerm1/actions-runner/_work/kmp-futured-template/kmp-futured-template/shared/feature/build/kotlinTransformedMetadataLibraries/nativeMain/app.cash.sqldelight-runtime-2.0.1-commonMain-wyRdBg.klib
2 Messages
📖 iosAppTests: Executed 1 test, with 0 failures (0 expected) in 0.001 (0.001) seconds
📖 iosAppUITests: Executed 1 test, with 0 failures (0 expected) in 19.189 (19.189) seconds

Generated by 🚫 Danger

@matejsemancik matejsemancik merged commit e5cc299 into develop Jan 10, 2025
7 checks passed
@matejsemancik matejsemancik deleted the feature/v3 branch January 10, 2025 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants