Leviathan v3 #4
Leviathan v3 #4
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors the DI core to a scoped Dependency API with valueOf/factoryOf/instanceOf, adds Compose and ViewModel injection helpers, overhauls examples/docs, updates tests to new DSL, and migrates build to Android Kotlin Multiplatform with version bumps and new ABI tasks/wrapper upgrades. Public APIs change accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as Composable caller
participant C as Compose runtime
participant S as DIScope (composition)
participant D as Dependency<T>
U->>C: inject(dependency)
rect rgba(200,220,255,0.25)
note right of C: Remember per-composition scope
C->>C: remember { DIScope() }
C->>D: injectedIn(S)
D-->>C: T
C-->>U: T
end
rect rgba(255,230,200,0.25)
note right of C: Dispose composition
C->>S: onDispose -> close()
end
sequenceDiagram
autonumber
actor VM as ViewModel
participant VS as ViewModelDIScope
participant IS as Internal DIScope
participant D as Dependency<T>
VM->>VS: inject(dependency)
alt first call
VS->>VS: create IS if absent
end
VS->>D: injectedIn(IS)
D-->>VM: T
note over VM,VS: On ViewModel.onCleared()
VM->>VS: close()
VS->>IS: close()
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
leviathan-compose/src/commonMain/kotlin/com/composegears/leviathan/compose/LeviathanCompose.kt (1)
35-44: Consider addingdependencyas a key toremember.Currently, the
rememberblock that callsdependency.injectedIn(scope)doesn't includedependencyas a key. If thedependencyparameter changes between recompositions, the remembered value won't update.Apply this diff if
dependencyis expected to change:- return remember { dependency.injectedIn(scope) } + return remember(dependency) { dependency.injectedIn(scope) }However, if
dependencyis guaranteed to be stable (e.g., coming from aModuleobject), the current implementation is fine.leviathan/src/commonMain/kotlin/com/composegears/leviathan/Leviathan.kt (2)
8-25: Consider thread safety for DIScope.The
closeActionslist usesmutableListOf, which is not thread-safe. IfonClose()orclose()can be called from multiple threads concurrently, this could lead to race conditions.If multi-threaded access is expected, consider using a thread-safe collection:
- private val closeActions = mutableListOf<() -> Unit>() + private val closeActions = mutableListOf<() -> Unit>() + private val lock = Any() internal open fun onClose(action: () -> Unit) { + synchronized(lock) { closeActions += action + } } public fun close() { + val actions = synchronized(lock) { + closeActions.toList().also { closeActions.clear() } + } - closeActions.onEach { it() } - closeActions.clear() + actions.forEach { it() } }However, if DIScope is intended for single-threaded use (e.g., within a ViewModel or Composition), the current implementation is acceptable.
46-62: Consider thread safety for FactoryDependency cache.The
scopedCachemap is not thread-safe. IfinjectedIn()is called concurrently for the same scope, it could create duplicate instances or cause data corruption.Consider using
synchronizedor a concurrent map:- private val scopedCache = mutableMapOf<DIScope, T>() + private val scopedCache = mutableMapOf<DIScope, T>() + private val lock = Any() + override fun injectedIn(scope: DIScope): T = if (useCache) { - scopedCache.getOrElse(scope) { - val instance = factory(DependencyInitializationScope(scope)) - scopedCache[scope] = instance - scope.onClose { - scopedCache.remove(scope) - } - instance + synchronized(lock) { + scopedCache.getOrPut(scope) { + val instance = factory(DependencyInitializationScope(scope)) + scope.onClose { + synchronized(lock) { + scopedCache.remove(scope) + } + } + instance + } } } else factory(DependencyInitializationScope(scope))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(2 hunks)build.gradle.kts(2 hunks)gradle/libs.versions.toml(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)gradlew(4 hunks)gradlew.bat(2 hunks)leviathan-compose/api/android/leviathan-compose.api(1 hunks)leviathan-compose/api/jvm/leviathan-compose.api(1 hunks)leviathan-compose/api/leviathan-compose.klib.api(1 hunks)leviathan-compose/build.gradle.kts(2 hunks)leviathan-compose/src/commonMain/kotlin/com/composegears/leviathan/compose/LeviathanCompose.kt(1 hunks)leviathan/build.gradle.kts(2 hunks)leviathan/src/commonMain/kotlin/com/composegears/leviathan/Leviathan.kt(1 hunks)leviathan/src/commonTest/kotlin/com/composegears/leviathan/Test.kt(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
55-55: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
🔇 Additional comments (22)
gradlew.bat (1)
76-76: LGTM! Wrapper invocation updated to direct jar execution.The change from
-classpathwithGradleWrapperMainto-jar gradle-wrapper.jaraligns with Gradle's standard wrapper pattern and simplifies the invocation.gradlew (1)
89-89: LGTM! Wrapper script improvements and direct jar invocation.The changes include:
- Safer path handling with
printf '%s\n' "$PWD"to avoid special character issues.- Direct jar invocation (
-jar gradle-wrapper.jar) consistent with the Windows script and Gradle's standard pattern.- Security improvement:
execreplacesevalfor final command execution, eliminating shell expansion risks.Also applies to: 213-213, 248-248
leviathan-compose/api/leviathan-compose.klib.api (1)
9-11: LGTM! New inject API surface added.The three inject overloads provide ViewModel-scoped DI, composable injection, and lambda-based injection. The declarations are consistent with the PR objectives and align with the new DI surface.
leviathan-compose/api/android/leviathan-compose.api (1)
2-4: LGTM! New inject API surface consistent with klib dump.The three inject overloads replace
leviathanInjectand provide ViewModel-scoped DI, composable injection, and lambda-based injection. The declarations are consistent with the klib.api and align with the PR objectives.leviathan-compose/build.gradle.kts (3)
19-22: LGTM: ABI validation enabled.Enabling ABI validation is essential for maintaining binary compatibility across library versions.
51-51: Correct use ofapi()instead ofimplementation().Since this module exposes Leviathan types in its public API (e.g.,
Dependency<T>in function signatures), usingapi(projects.leviathan)ensures proper transitive dependency exposure.
55-55: LGTM: lifecycle-viewmodel-compose dependency added.This dependency correctly supports the new
ViewModel.inject()extension function introduced in this PR.leviathan-compose/src/commonMain/kotlin/com/composegears/leviathan/compose/LeviathanCompose.kt (2)
12-22: LGTM: ViewModelDIScope implementation.The
ViewModelDIScopewrapper correctly integrates with ViewModel'sAutoCloseablemechanism, ensuring the DIScope is cleaned up when the ViewModel is cleared.
24-31: LGTM: ViewModel.inject extension.The implementation correctly:
- Lazily creates and stores a
ViewModelDIScopeusing the ViewModel's closeable registry- Reuses the scope across multiple inject calls within the same ViewModel
- Ensures cleanup via the ViewModel lifecycle
leviathan/build.gradle.kts (2)
16-23: LGTM: ABI validation and compiler options.The ABI validation is correctly enabled, and the opt-in to
ExperimentalComposeUiApiis appropriate for a library that may expose Compose types.
26-36: LGTM: androidLibrary configuration.The migration from
androidTargettoandroidLibraryis correctly configured with appropriate namespace, SDK versions, and JVM target.leviathan-compose/api/jvm/leviathan-compose.api (1)
1-5: LGTM: API surface updated.The API definition correctly reflects the new
injectoverloads introduced in the implementation, replacing the previousleviathanInjectentry point.README.md (3)
42-53: LGTM: Clear documentation of new DSL.The documentation clearly explains the new dependency declaration functions (
instanceOf,factoryOf,valueOf) with their parameters and behavior. This will help users understand the new API.
72-82: LGTM: Comprehensive module examples.The module examples demonstrate all key patterns:
- Auto-close and keep-alive instances
- Factory with parameters
- Dependency injection via
inject()- Interface-based dependencies
- Constant values
87-113: LGTM: Usage examples cover all integration points.The examples correctly demonstrate:
- ViewModel integration with default parameter injection
- Compose integration with
inject()- Manual scope management for random access
leviathan/src/commonTest/kotlin/com/composegears/leviathan/Test.kt (6)
52-88: LGTM: Comprehensive DIScope lifecycle tests.The tests correctly verify:
- Close actions are executed
- Multiple close actions execute in order
- Close actions only execute once
- GLOBAL scope doesn't execute close actions
90-130: LGTM: Factory behavior tests.The tests thoroughly verify the two factory modes:
cachedFactory(useCache=true) caches within scope, creates new across scopesalwaysNewFactory(useCache=false) always creates new instances
132-206: LGTM: Instance lifecycle tests.The tests comprehensively verify:
autoCloseInstance(keepAlive=false) reuses within scope and nullifies when all scopes closekeepAliveInstance(keepAlive=true) persists across scopes and survives closure
208-238: LGTM: Dependency injection tests.The tests verify that
inject()correctly resolves dependencies during initialization, including nested dependency chains.
240-282: LGTM: Cyclic and external dependency tests.The tests verify:
- Cyclic dependencies resolve correctly via lazy lambdas
- External dependencies from other modules work correctly and maintain independence
284-323: LGTM: Mixed dependency behavior tests.The tests verify that different dependency types behave correctly when used together within the same scope and across different scopes.
leviathan/src/commonMain/kotlin/com/composegears/leviathan/Leviathan.kt (1)
91-116: LGTM: Leviathan DSL implementation.The
valueOf,factoryOf, andinstanceOfhelper functions provide a clean, declarative API for defining dependencies. The use of@JvmStaticon thegetValueoperator correctly enables property delegation on the JVM.
egorikftp
force-pushed
the
v3
branch
2 times, most recently
from
f0b1536 to
94f2863
Compare
Resolve #3 points
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores