Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt (1)
52-62: Consider exposing timestamp errors to the UI.The
onErrorhandler only logs the error silently. While this may be acceptable for a demo timestamp feature, per coding guidelines errors should typically be modeled as results. Consider adding an error state toSecondViewStateif timestamp failures should be visible to users, or document why silent failure is intentional here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt` around lines 52 - 62, The onError in runTimestamp currently only logs errors; update the UI state to surface timestamp failures by adding an error field to SecondViewState (e.g., timestampError: Throwable? or timestampErrorMessage: String?) and set it in componentState from the onError handler of timeStampUseCase; specifically, modify SecondViewState to include the error property, update any constructors/defaults, and in runTimestamp's onError block call componentState.update { it.copy(timestampError = theError) } (and optionally clear timestampError on successful onNext), or if silent failure is intentional, add a brief code comment in runTimestamp explaining that omission.shared/ui/src/commonMain/kotlin/app/futured/kmptemplate/ui/screen/FirstScreenUi.kt (1)
94-95: Consecutive identical Spacers - possible cleanup.Two
Spacer(modifier = Modifier.height(4.dp))in a row (lines 94-95) appear to be a leftover from removing thecreatedAtdisplay. Consider consolidating into a singleSpacerwith 8.dp height, or removing the duplicate if it was unintentional.♻️ Proposed consolidation
Text(text = viewState.counterText.localized()) - Spacer(modifier = Modifier.height(4.dp)) - Spacer(modifier = Modifier.height(4.dp)) + Spacer(modifier = Modifier.height(8.dp)) AnimatedVisibility(viewState.randomPersonText != null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/ui/src/commonMain/kotlin/app/futured/kmptemplate/ui/screen/FirstScreenUi.kt` around lines 94 - 95, In FirstScreenUi.kt inside the composable that renders the list item (the block containing two consecutive Spacer(modifier = Modifier.height(4.dp))), remove the duplicate Spacer and either replace the pair with a single Spacer(modifier = Modifier.height(8.dp)) or keep a single 4.dp Spacer if less space was intended; update the Spacer adjacent to where the removed createdAt text used to be so layout spacing is correct (look for the composable function that contains those Spacer calls to locate the change).shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/TimeStampUseCase.kt (1)
15-20: Consider clarifying the emission behavior.The loop variable
iis unused, and the loop runs exactly 1001 times before terminating. If the intent is a finite demo, consider usingrepeat(1001)for clarity. If continuous emission is intended (until collection stops), considerwhile(true)orwhile(currentCoroutineContext().isActive)for consistency withCounterUseCaseImpl.♻️ Option A: Finite emissions with `repeat`
override fun build(args: Unit): Flow<Instant> = flow { - for (i in 0..1000) { + repeat(1001) { emit(Clock.System.now()) delay(1000) } }♻️ Option B: Infinite emissions (matching CounterUseCaseImpl pattern)
+import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.isActive + `@Factory` internal class TimeStampUseCaseImpl : TimeStampUseCase { override fun build(args: Unit): Flow<Instant> = flow { - for (i in 0..1000) { + while (currentCoroutineContext().isActive) { emit(Clock.System.now()) delay(1000) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/TimeStampUseCase.kt` around lines 15 - 20, The loop in TimeStampUseCase.build currently iterates with an unused index and emits 1001 times; decide desired behavior and update accordingly: if you want a finite demo, replace the for-loop with repeat(1001) to clearly emit 1001 timestamps; if you want continuous emission matching CounterUseCaseImpl, replace the loop with while (true) or while (currentCoroutineContext().isActive) to emit indefinitely until collection cancels. Update the implementation in TimeStampUseCase.build (and ensure import of currentCoroutineContext() if used) to reflect the chosen pattern.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondViewState.kt (1)
13-14: String resource key references "first_screen" but is used in SecondViewState.
MR.strings.first_screen_created_atis used here for the second screen. If this is intentional reuse, consider renaming the string resource to something generic (e.g.,created_at) for clarity. Otherwise, create asecond_screen_created_atresource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondViewState.kt` around lines 13 - 14, The code in SecondViewState uses MR.strings.first_screen_created_at via the createdAtText property which mislabels the resource for the second screen; update the usage to a properly named resource (e.g., MR.strings.second_screen_created_at) or rename the string resource to a generic key (e.g., created_at) and then reference that from createdAtText (symbol: createdAtText, MR.strings.first_screen_created_at) so the resource name matches the screen intent.shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenTest.kt (2)
27-27: File name doesn't match class name.The file is named
FirstScreenTest.ktbut contains classFirstComponentTest. For consistency with Kotlin conventions, consider renaming the file toFirstComponentTest.kt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenTest.kt` at line 27, The class declaration uses the name FirstComponentTest while the file is named FirstScreenTest.kt; rename the file to FirstComponentTest.kt so it matches the Kotlin class name FirstComponentTest (ensuring the filename and class name are consistent with Kotlin conventions).
197-224: Use Long literals in counter assertions for consistency.The counter assertions use Int literals (
0,29,30, etc.) butcounterisLong. While this works due to implicit conversion, using0L,29L,30L, etc. would be more consistent with other tests in this file (lines 124-129).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenTest.kt` around lines 197 - 224, The assertions in FirstScreenTest use Int literals for the Long-typed counter; update the literal values in the block using state.awaitItem() (and the events.assertion) to use Long literals (e.g., 0L, 29L, 30L, 31L, 40L) so they match the counter's type and the style used elsewhere in this test file; look for the assertions referencing counter inside FirstScreenTest (the state.awaitItem() runs and the FirstUiEvent.Notify assertion) and replace the numeric literals with their Long forms.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/ext/LifecycleUseCaseExecution.kt (1)
64-71:flowOnplacement may not achieve intended threading for use case work.The
flowOnoperator is applied afterflatMapLatest, meaning it affects where theflatMapLatestswitching logic runs, not necessarily wheretargetFlow's upstream work executes. Ifbuild(args)returns a flow that performs work without its ownflowOn, that work may not run on the worker dispatcher as intended.Consider applying
flowOnto thetargetFlowdirectly inside theflatMapLatest:♻️ Proposed refactor
val targetFlow = build(args) coroutineScopeOwner.useCaseJobPool[this] = lifecycleState .flatMapLatest { active -> - if (active) targetFlow else emptyFlow() + if (active) targetFlow.flowOn(coroutineScopeOwner.getWorkerDispatcher()) else emptyFlow() } - .flowOn(coroutineScopeOwner.getWorkerDispatcher()) .onStart { flowUseCaseConfig.onStart() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/ext/LifecycleUseCaseExecution.kt` around lines 64 - 71, The current placement of flowOn after lifecycleState.flatMapLatest means the worker dispatcher may only affect the flatMapLatest operator, not the upstream work in targetFlow created by build(args); instead, inside the flatMapLatest branch where you return the active flow, apply flowOn(coroutineScopeOwner.getWorkerDispatcher()) directly to targetFlow (e.g., return targetFlow.flowOn(...)) so that the upstream work of the flow built by build(args) runs on the worker dispatcher, and keep onStart invoked as before via .onStart { flowUseCaseConfig.onStart() } on the resulting flow placed after flatMapLatest.shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCaseExecution.kt (2)
83-105: Consider adding a convenience overload forUseCase<Unit, T>.The non-suspending
executehas a convenience overload forUseCase<Unit, T>(lines 20-22). For API consistency, you might want to add a similar overload for the suspending version.➕ Optional overload to mirror the non-suspend API
/** * Synchronously executes use case and saves it's Deferred. By default all previous * pending executions are canceled, this can be changed by the [cancelPrevious]. * This version is used for use cases without initial arguments. * * `@return` [Result] that encapsulates either a successful result or a failed result. */ context(coroutineScopeOwner: CoroutineScopeOwner) suspend fun <T : Any?> UseCase<Unit, T>.execute( cancelPrevious: Boolean = true, ): Result<T> = execute(Unit, cancelPrevious)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCaseExecution.kt` around lines 83 - 105, Add a convenience overload for the suspending UseCase.execute to mirror the non-suspending API: declare a context(coroutineScopeOwner: CoroutineScopeOwner) suspend fun <T : Any?> UseCase<Unit, T>.execute(cancelPrevious: Boolean = true): Result<T> that simply calls the existing suspending UseCase<ARGS, T>.execute with Unit (i.e., return execute(Unit, cancelPrevious)); this keeps API consistency with the non-suspending overload and reuses the current coroutineScopeOwner and cancellation behavior.
79-79: Minor: KDoc references incorrect type names.
[Success]and[Error]are not valid KDoc references for Kotlin'sResultclass. Consider updating to match the actual API.📝 Suggested documentation fix
- * `@return` [Result] that encapsulates either a successful result with [Success] or a failed result with [Error] + * `@return` [Result] that encapsulates either a successful result with [Result.success] or a failed result with [Result.failure]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCaseExecution.kt` at line 79, KDoc incorrectly references non-existent `[Success]` and `[Error]` types; update the comment in UseCaseExecution.kt (the function returning Result) to reference Kotlin's Result API correctly—e.g., say it returns a Result encapsulating either a successful value (via Result.success) or a failure with a Throwable (via Result.failure / exception), or simply "a successful result or a failed result with a Throwable" instead of `[Success]` and `[Error]`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@iosApp/iosApp/Views/Screen/First/FirstViewModel.swift`:
- Around line 51-54: showToast(event: FirstUiEventNotify) currently ignores the
passed-in event and sets alertText to a hardcoded "todo"; change it to use the
data on FirstUiEventNotify (e.g. event.counter or event.message) to compose the
user-facing string and assign that to alertText, then set alertVisible = true as
before (update the function showToast(event:) and the alertText assignment so
the alert shows the actual event value).
In
`@shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCaseExecution.kt`:
- Around line 99-103: The current pipeline uses
flowUseCaseConfig.onMap?.invoke(it) inside mapNotNull which drops all emissions
when onMap is null; fix by making onMap required for mapped executions: in
FlowUseCaseConfig.Builder enforce requireNotNull(onMap) with a clear message for
executeMapped, and then in FlowUseCaseExecution change mapNotNull {
flowUseCaseConfig.onMap?.invoke(it) } to a non-null invocation (e.g., map {
flowUseCaseConfig.onMap.invoke(it) }) relying on the builder validation;
alternatively, if you prefer an identity fallback, document and implement a
default onMap that returns the input for matching types.
In
`@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstViewState.kt`:
- Around line 21-26: The computed property randomPersonText currently uses a
hardcoded "Failed to fetch" string; replace that with a localized resource by
adding a new string resource (e.g., first_screen_random_person_error) and use
MR.strings.first_screen_random_person_error.format() in the randomPersonText
getter where randomPersonError != null, keeping the existing
MR.strings.first_screen_random_person.format(randomPerson.name.orEmpty()) branch
intact.
In
`@shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenTest.kt`:
- Line 187: The test passes an Int Flow to counterUseCase which expects Longs;
change the lambda providing counterUseCase in FirstScreenTest (the entry with
counterUseCase = { flowOf(29, 30, 31, 40) }) to emit Long literals (e.g. 29L,
30L, 31L, 40L) so the returned Flow type matches CounterUseCase's Long emission
type and avoids type-mismatch compilation errors.
---
Nitpick comments:
In
`@shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCaseExecution.kt`:
- Around line 83-105: Add a convenience overload for the suspending
UseCase.execute to mirror the non-suspending API: declare a
context(coroutineScopeOwner: CoroutineScopeOwner) suspend fun <T : Any?>
UseCase<Unit, T>.execute(cancelPrevious: Boolean = true): Result<T> that simply
calls the existing suspending UseCase<ARGS, T>.execute with Unit (i.e., return
execute(Unit, cancelPrevious)); this keeps API consistency with the
non-suspending overload and reuses the current coroutineScopeOwner and
cancellation behavior.
- Line 79: KDoc incorrectly references non-existent `[Success]` and `[Error]`
types; update the comment in UseCaseExecution.kt (the function returning Result)
to reference Kotlin's Result API correctly—e.g., say it returns a Result
encapsulating either a successful value (via Result.success) or a failure with a
Throwable (via Result.failure / exception), or simply "a successful result or a
failed result with a Throwable" instead of `[Success]` and `[Error]`.
In
`@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/ext/LifecycleUseCaseExecution.kt`:
- Around line 64-71: The current placement of flowOn after
lifecycleState.flatMapLatest means the worker dispatcher may only affect the
flatMapLatest operator, not the upstream work in targetFlow created by
build(args); instead, inside the flatMapLatest branch where you return the
active flow, apply flowOn(coroutineScopeOwner.getWorkerDispatcher()) directly to
targetFlow (e.g., return targetFlow.flowOn(...)) so that the upstream work of
the flow built by build(args) runs on the worker dispatcher, and keep onStart
invoked as before via .onStart { flowUseCaseConfig.onStart() } on the resulting
flow placed after flatMapLatest.
In
`@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/TimeStampUseCase.kt`:
- Around line 15-20: The loop in TimeStampUseCase.build currently iterates with
an unused index and emits 1001 times; decide desired behavior and update
accordingly: if you want a finite demo, replace the for-loop with repeat(1001)
to clearly emit 1001 timestamps; if you want continuous emission matching
CounterUseCaseImpl, replace the loop with while (true) or while
(currentCoroutineContext().isActive) to emit indefinitely until collection
cancels. Update the implementation in TimeStampUseCase.build (and ensure import
of currentCoroutineContext() if used) to reflect the chosen pattern.
In
`@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.kt`:
- Around line 52-62: The onError in runTimestamp currently only logs errors;
update the UI state to surface timestamp failures by adding an error field to
SecondViewState (e.g., timestampError: Throwable? or timestampErrorMessage:
String?) and set it in componentState from the onError handler of
timeStampUseCase; specifically, modify SecondViewState to include the error
property, update any constructors/defaults, and in runTimestamp's onError block
call componentState.update { it.copy(timestampError = theError) } (and
optionally clear timestampError on successful onNext), or if silent failure is
intentional, add a brief code comment in runTimestamp explaining that omission.
In
`@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondViewState.kt`:
- Around line 13-14: The code in SecondViewState uses
MR.strings.first_screen_created_at via the createdAtText property which
mislabels the resource for the second screen; update the usage to a properly
named resource (e.g., MR.strings.second_screen_created_at) or rename the string
resource to a generic key (e.g., created_at) and then reference that from
createdAtText (symbol: createdAtText, MR.strings.first_screen_created_at) so the
resource name matches the screen intent.
In
`@shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenTest.kt`:
- Line 27: The class declaration uses the name FirstComponentTest while the file
is named FirstScreenTest.kt; rename the file to FirstComponentTest.kt so it
matches the Kotlin class name FirstComponentTest (ensuring the filename and
class name are consistent with Kotlin conventions).
- Around line 197-224: The assertions in FirstScreenTest use Int literals for
the Long-typed counter; update the literal values in the block using
state.awaitItem() (and the events.assertion) to use Long literals (e.g., 0L,
29L, 30L, 31L, 40L) so they match the counter's type and the style used
elsewhere in this test file; look for the assertions referencing counter inside
FirstScreenTest (the state.awaitItem() runs and the FirstUiEvent.Notify
assertion) and replace the numeric literals with their Long forms.
In
`@shared/ui/src/commonMain/kotlin/app/futured/kmptemplate/ui/screen/FirstScreenUi.kt`:
- Around line 94-95: In FirstScreenUi.kt inside the composable that renders the
list item (the block containing two consecutive Spacer(modifier =
Modifier.height(4.dp))), remove the duplicate Spacer and either replace the pair
with a single Spacer(modifier = Modifier.height(8.dp)) or keep a single 4.dp
Spacer if less space was intended; update the Spacer adjacent to where the
removed createdAt text used to be so layout spacing is correct (look for the
composable function that contains those Spacer calls to locate the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff6ccb3a-92e2-4c85-b91a-2460107a47fb
📒 Files selected for processing (46)
gradle/libs.versions.tomliosApp/iosApp/Views/Screen/First/FirstView.swiftiosApp/iosApp/Views/Screen/First/FirstViewModel.swiftiosApp/iosApp/Views/Screen/Second/SecondView.swiftiosApp/iosApp/Views/Screen/Second/SecondViewModel.swiftshared/arkitekt-cr-usecases/build.gradle.ktsshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCase.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCaseExecution.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCase.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCaseExecution.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/CoroutineScopeOwner.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseConfig.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/SingleUseCaseExecutionScope.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/UseCaseConfig.ktshared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/UseCaseExecutionScope.ktshared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/UseCaseExecutionScopeTest.ktshared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/base/BaseUseCaseExecutionScopeTest.ktshared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFailureFlowUseCase.ktshared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFailureUseCase.ktshared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFlowUseCase.ktshared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestUseCase.ktshared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/presentation/BaseComponent.ktshared/feature/build.gradle.ktsshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/CounterUseCase.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/FetchDataUseCase.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/IsUserLoggedInUseCase.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/SetUserLoggedInUseCase.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/TimeStampUseCase.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/ext/LifecycleUseCaseExecution.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/root/RootNavHostComponent.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostComponent.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/base/BaseComponents.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponent.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstEvent.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenPreviews.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstViewState.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/loginScreen/LoginComponent.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/profileScreen/ProfileComponent.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondComponent.ktshared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/secondScreen/SecondViewState.ktshared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/base/ComponentTest.ktshared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/base/ComponentTestPreparation.ktshared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenTest.ktshared/ui/src/commonMain/kotlin/app/futured/kmptemplate/ui/screen/FirstScreenUi.ktshared/ui/src/commonMain/kotlin/app/futured/kmptemplate/ui/screen/SecondScreenUi.kt
💤 Files with no reviewable changes (3)
- shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/UseCaseExecutionScope.kt
- shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/SingleUseCaseExecutionScope.kt
- shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.kt
Comment on lines
+51
to
54
| func showToast(event: FirstUiEventNotify) { | ||
| alertText = "todo" | ||
| alertVisible = true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what properties FirstUiEventNotify exposes
rg -n "class FirstUiEvent" --type swift -A 10
rg -n "FirstUiEvent" shared/feature/src/commonMain -A 5 --type kotlinRepository: futuredapp/kmp-futured-template
Length of output: 3706
🏁 Script executed:
#!/bin/bash
# Search for FirstUiEventNotify definition and properties
find . -type f -name "*.swift" | xargs grep -l "FirstUiEventNotify" | head -5
rg "FirstUiEventNotify" -A 5 -B 2 --type swiftRepository: futuredapp/kmp-futured-template
Length of output: 1322
🏁 Script executed:
#!/bin/bash
# Check how showToast is being called and what data flows to it
rg "showToast" -B 5 -A 5 --type swiftRepository: futuredapp/kmp-futured-template
Length of output: 2766
🏁 Script executed:
#!/bin/bash
# Check FirstViewState to understand what data might be available
rg "FirstViewState\|FirstViewModel" -A 10 --type swift | head -50Repository: futuredapp/kmp-futured-template
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for FirstUiEvent definition on Kotlin side to understand the data structure
cat iosApp/iosApp/Views/Screen/First/FirstViewModel.swift | head -60Repository: futuredapp/kmp-futured-template
Length of output: 1465
🏁 Script executed:
#!/bin/bash
# Find and examine FirstEvent.kt more carefully to see the Notify class structure
cat shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstEvent.ktRepository: futuredapp/kmp-futured-template
Length of output: 541
🏁 Script executed:
#!/bin/bash
# Search for any SKIE generated files or bindings that might show FirstUiEventNotify properties
find . -type f \( -name "*.kt" -o -name "*.swift" \) | xargs grep -l "FirstUiEventNotify\|FirstEvent" | grep -E "(generated|Skie)" | head -10
rg "class Notify\|data class Notify" -A 3 --type kotlinRepository: futuredapp/kmp-futured-template
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Search for any Swift code that accesses properties of FirstUiEventNotify or similar event types
rg "FirstUiEventNotify\." --type swift
rg "\.notify\|\.count\|\.text" iosApp/iosApp/Views/Screen/First --type swift -B 2 -A 2Repository: futuredapp/kmp-futured-template
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check if there's generated Swift code from SKIE or any bindings documentation
find . -type f -name "*.swift" | xargs grep -l "SKIE\|KotlinNative" | head -5
# Also check the Kotlin side to understand the event better
rg "sendEvent.*Notify" --type kotlin -B 2 -A 2Repository: futuredapp/kmp-futured-template
Length of output: 786
Use event data instead of hardcoded placeholder.
The showToast(event:) method receives a FirstUiEventNotify parameter containing the counter value, but ignores it and hardcodes alertText = "todo". The event should be used to provide the actual alert message to the user.
🔧 Suggested fix
func showToast(event: FirstUiEventNotify) {
- alertText = "todo"
+ alertText = String(event.count) // or access appropriate property from the event
alertVisible = true
}📝 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
| func showToast(event: FirstUiEventNotify) { | |
| alertText = "todo" | |
| alertVisible = true | |
| } | |
| func showToast(event: FirstUiEventNotify) { | |
| alertText = String(event.count) // or access appropriate property from the event | |
| alertVisible = true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iosApp/iosApp/Views/Screen/First/FirstViewModel.swift` around lines 51 - 54,
showToast(event: FirstUiEventNotify) currently ignores the passed-in event and
sets alertText to a hardcoded "todo"; change it to use the data on
FirstUiEventNotify (e.g. event.counter or event.message) to compose the
user-facing string and assign that to alertText, then set alertVisible = true as
before (update the function showToast(event:) and the alertText assignment so
the alert shows the actual event value).
Comment on lines
+99
to
+103
| coroutineScopeOwner.useCaseJobPool[this] = build(args) | ||
| .flowOn(coroutineScopeOwner.getWorkerDispatcher()) | ||
| .onStart { flowUseCaseConfig.onStart() } | ||
| .mapNotNull { flowUseCaseConfig.onMap?.invoke(it) } | ||
| .onEach { flowUseCaseConfig.onNext(it) } |
There was a problem hiding this comment.
Silent emission loss when onMap is not configured.
When onMap is null, the expression flowUseCaseConfig.onMap?.invoke(it) returns null for every emission, causing mapNotNull to filter out all values. This means onNext will never be called if the caller forgets to set onMap.
Consider making onMap required for executeMapped, or use a different approach that doesn't silently drop emissions.
🔧 Proposed fix: require onMap or use identity fallback
Option 1: Use identity function as fallback (if T and M can be the same):
- .mapNotNull { flowUseCaseConfig.onMap?.invoke(it) }
+ .mapNotNull { flowUseCaseConfig.onMap?.invoke(it) ?: `@Suppress`("UNCHECKED_CAST") (it as? M) }Option 2: Require onMap in the FlowUseCaseConfig.Builder for mapped variants by making it non-nullable in the config, or validate at build time:
// In FlowUseCaseConfig.Builder
fun build(): FlowUseCaseConfig<T, M> {
requireNotNull(onMap) { "onMap must be provided for executeMapped" }
// ...
}📝 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
| coroutineScopeOwner.useCaseJobPool[this] = build(args) | |
| .flowOn(coroutineScopeOwner.getWorkerDispatcher()) | |
| .onStart { flowUseCaseConfig.onStart() } | |
| .mapNotNull { flowUseCaseConfig.onMap?.invoke(it) } | |
| .onEach { flowUseCaseConfig.onNext(it) } | |
| coroutineScopeOwner.useCaseJobPool[this] = build(args) | |
| .flowOn(coroutineScopeOwner.getWorkerDispatcher()) | |
| .onStart { flowUseCaseConfig.onStart() } | |
| .mapNotNull { flowUseCaseConfig.onMap?.invoke(it) ?: `@Suppress`("UNCHECKED_CAST") (it as? M) } | |
| .onEach { flowUseCaseConfig.onNext(it) } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCaseExecution.kt`
around lines 99 - 103, The current pipeline uses
flowUseCaseConfig.onMap?.invoke(it) inside mapNotNull which drops all emissions
when onMap is null; fix by making onMap required for mapped executions: in
FlowUseCaseConfig.Builder enforce requireNotNull(onMap) with a clear message for
executeMapped, and then in FlowUseCaseExecution change mapNotNull {
flowUseCaseConfig.onMap?.invoke(it) } to a non-null invocation (e.g., map {
flowUseCaseConfig.onMap.invoke(it) }) relying on the builder validation;
alternatively, if you prefer an identity fallback, document and implement a
default onMap that returns the input for matching types.
Comment on lines
+21
to
+26
| val randomPersonText: StringDesc? | ||
| get() = when { | ||
| randomPersonError != null -> MR.strings.first_screen_random_person.format("Failed to fetch") | ||
| randomPerson != null -> MR.strings.first_screen_random_person.format(randomPerson.name.orEmpty()) | ||
| else -> null | ||
| } |
There was a problem hiding this comment.
Hardcoded string "Failed to fetch" should use a localized resource.
The error message bypasses the localization system used elsewhere in this file. This will cause i18n issues if the app supports multiple languages.
Proposed fix
val randomPersonText: StringDesc?
get() = when {
- randomPersonError != null -> MR.strings.first_screen_random_person.format("Failed to fetch")
+ randomPersonError != null -> MR.strings.first_screen_random_person_error
randomPerson != null -> MR.strings.first_screen_random_person.format(randomPerson.name.orEmpty())
else -> null
}You'll need to add first_screen_random_person_error to your string resources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstViewState.kt`
around lines 21 - 26, The computed property randomPersonText currently uses a
hardcoded "Failed to fetch" string; replace that with a localized resource by
adding a new string resource (e.g., first_screen_random_person_error) and use
MR.strings.first_screen_random_person_error.format() in the randomPersonText
getter where randomPersonError != null, keeping the existing
MR.strings.first_screen_random_person.format(randomPerson.name.orEmpty()) branch
intact.
| val expectedPerson = Person("Darth Vader") | ||
| val component = createComponent( | ||
| fetchDataUseCase = { expectedPerson }, | ||
| counterUseCase = { flowOf(29, 30, 31, 40) }, |
There was a problem hiding this comment.
Use Long literals for type consistency.
flowOf(29, 30, 31, 40) infers Flow<Int>, but CounterUseCase should emit Long values (as used in other tests like line 124-129 with 1L, 2L, etc.). This may cause a type mismatch.
Proposed fix
- counterUseCase = { flowOf(29, 30, 31, 40) },
+ counterUseCase = { flowOf(29L, 30L, 31L, 40L) },📝 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
| counterUseCase = { flowOf(29, 30, 31, 40) }, | |
| counterUseCase = { flowOf(29L, 30L, 31L, 40L) }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstScreenTest.kt`
at line 187, The test passes an Int Flow to counterUseCase which expects Longs;
change the lambda providing counterUseCase in FirstScreenTest (the entry with
counterUseCase = { flowOf(29, 30, 31, 40) }) to emit Long literals (e.g. 29L,
30L, 31L, 40L) so the returned Flow type matches CounterUseCase's Long emission
type and avoids type-mismatch compilation errors.
Member
|
Why is this needed if the similar work is being done in futuredapp/arkitekt#178 ? Local arkitekt modules will be eventually deleted and replaced by new Arkitekt lib. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
executeWithLifecycleSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests