Fix Factory generator - #129
Conversation
…injection The factory generator now uniformly handles all injected parameters via the `@InjectedParam` annotation, removing special handling for `AppComponentContext` and `Navigation`. This simplifies the parameter injection logic. Also, the package name has been updated to `app.futured.arkitekt.factorygenerator.processor`.
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/PoetFactoryComponentGenerator.kt (1)
12-13: Bug: return type breaks for nested classes; use toClassName().Building
ClassName(package, listOf(baseName))fails forOuter.Innercomponents. Use KotlinPoet KSP’stoClassName().Apply this diff:
@@ -import com.squareup.kotlinpoet.ksp.toTypeName +import com.squareup.kotlinpoet.ksp.toTypeName +import com.squareup.kotlinpoet.ksp.toClassName @@ - val returnType = ClassName( - packageName = factoryComponentPackageName, - simpleNames = listOf(baseName), - ) + val returnType = factoryComponent.toClassName()Also applies to: 117-121
🧹 Nitpick comments (4)
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/PoetFactoryComponentGenerator.kt (3)
33-35: KDoc is stale after removing special-cases.It still claims handling of AppComponentContext/Navigation/Arg. Please update to reflect the new uniform @InjectedParam-only behavior.
Apply this diff:
- * 4. **Handles Special Types:** Detects and handles special types like `AppComponentContext`, - * `Navigation`, and `Arg` for specific component needs. + * 4. No special‑casing of types: only constructor parameters annotated with `@InjectedParam` are considered.
77-80: Improve KSP incrementality: mark files as isolating and set origins.This generator produces one file per annotated class; prefer
aggregating = falsewith originating files set.Apply this diff:
- fileSpec.writeTo(codeGenerator, aggregating = true) + fileSpec.writeTo( + codeGenerator = codeGenerator, + aggregating = false, + originatingKSFiles = listOfNotNull(factoryComponent.containingFile), + )
142-148: Remove dead helpers.
findTypeByNameandcontainsTypeNameare unused after dropping special‑cases.Apply this diff:
- private fun List<KSValueParameter>.findTypeByName(name: String): TypeName? = this - .find { it.containsTypeName(name) } - ?.type?.toTypeName() - - private fun KSValueParameter.containsTypeName(name: String): Boolean = - this.type.toTypeName().toString().contains(name)shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessorProvider.kt (1)
8-10: Optional: pass logger/options to the processor if you plan diagnostics later.Not required now, but wiring
environment.loggerandenvironment.optionsgives better error reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessor.kt(1 hunks)shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessorProvider.kt(1 hunks)shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/PoetFactoryComponentGenerator.kt(3 hunks)shared/arkitekt-decompose/processor/src/jvmMain/resources/META-INF/services/com.google.devtools.ksp.processing.SymbolProcessorProvider(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Error handling: Use sealed NetworkError classes and model operations as results (Success/Failure)
Max 25 functions per class
Max 20 functions per file
Files:
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessorProvider.ktshared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessor.ktshared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/PoetFactoryComponentGenerator.kt
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Max line length: 140 characters
Files:
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessorProvider.ktshared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessor.ktshared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/PoetFactoryComponentGenerator.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check-android / Lint & Tests
- GitHub Check: check-ios / Test
🔇 Additional comments (5)
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessor.kt (1)
1-1: Package rename looks correct and consistent.Matches the updated service loader path in resources.
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/PoetFactoryComponentGenerator.kt (2)
125-129: LGTM: parameters only from @InjectedParam.The apply block cleanly limits the signature to injected params.
135-137: LGTM: explicit qualifier=null and parametersOf(...)Works with zero or more params; trailing comma formatting is fine.
shared/arkitekt-decompose/processor/src/jvmMain/kotlin/app/futured/arkitekt/factorygenerator/processor/ComponentFactoryProcessorProvider.kt (1)
1-1: Package rename aligned with source files and service loader.Looks consistent with the resources entry.
shared/arkitekt-decompose/processor/src/jvmMain/resources/META-INF/services/com.google.devtools.ksp.processing.SymbolProcessorProvider (1)
1-1: Service loader path updated correctly.Matches the new package. Please run a sample build to confirm KSP discovery.
| val unInjectedConstructorParams = factoryComponent.primaryConstructor?.parameters | ||
| ?.filter { it.annotations.any { it.shortName.asString() == INJECTED_PARAM_ANNOTATION } } | ||
| ?: error("No @InjectedParam annotation found in $baseName's constructor") | ||
|
|
There was a problem hiding this comment.
Don’t hard‑fail when no constructor or no @InjectedParam; treat as zero params.
The current error(...) aborts codegen for components with no primary constructor or no injected params. Generating createComponent() with parametersOf() is fine.
Apply this diff:
- val unInjectedConstructorParams = factoryComponent.primaryConstructor?.parameters
- ?.filter { it.annotations.any { it.shortName.asString() == INJECTED_PARAM_ANNOTATION } }
- ?: error("No @InjectedParam annotation found in $baseName's constructor")
+ val unInjectedConstructorParams = factoryComponent.primaryConstructor
+ ?.parameters
+ ?.filter { it.annotations.any { it.shortName.asString() == INJECTED_PARAM_ANNOTATION } }
+ .orEmpty()📝 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.
| val unInjectedConstructorParams = factoryComponent.primaryConstructor?.parameters | |
| ?.filter { it.annotations.any { it.shortName.asString() == INJECTED_PARAM_ANNOTATION } } | |
| ?: error("No @InjectedParam annotation found in $baseName's constructor") | |
| val unInjectedConstructorParams = factoryComponent.primaryConstructor | |
| ?.parameters | |
| ?.filter { it.annotations.any { it.shortName.asString() == INJECTED_PARAM_ANNOTATION } } | |
| .orEmpty() |
Generated by 🚫 Danger |
2 participants
The factory generator now uniformly handles all injected parameters via the
@InjectedParamannotation, removing special handling forAppComponentContextandNavigation. This simplifies the parameter injection logic. Also, the package name has been updated toapp.futured.arkitekt.factorygenerator.processor.Summary by CodeRabbit
Refactor
Chores
Impact: Consumers may need to update imports and regenerate code to align with the new createComponent signature.