feat: add navigate-for-result API for type-safe result handling #38
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive navigate-for-result feature that enables screens to return typed data to their callers with compile-time validation. The implementation adds an optional result parameter to navigation annotations (similar to the existing args parameter), new NavigationController methods for result-based navigation, and supporting infrastructure for callback management and result delivery.
Key Changes
- Added optional
resultparameter to@UiEntryand@UiExternalEntryannotations withNoResult::classdefault - Implemented result-based navigation APIs in NavigationController (
navigateForResult,setResultAndNavigateBack,cancelResultAndNavigateBack) - Created runtime support infrastructure including
ResultEntryinterface,ResultCallbackRegistry, and request key management
Reviewed Changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| nibel-annotations/src/main/kotlin/nibel/annotations/UiEntry.kt | Added result parameter to annotation |
| nibel-annotations/src/main/kotlin/nibel/annotations/UiExternalEntry.kt | Added result parameter to annotation |
| nibel-annotations/src/main/kotlin/nibel/annotations/Destination.kt | Added DestinationWithResult interface |
| nibel-runtime/src/main/kotlin/nibel/runtime/NoResult.kt | Created marker class for no-result screens |
| nibel-runtime/src/main/kotlin/nibel/runtime/ResultEntry.kt | Created interface for result-returning entries |
| nibel-runtime/src/main/kotlin/nibel/runtime/ResultCallbackRegistry.kt | Implemented global callback registry |
| nibel-runtime/src/main/kotlin/nibel/runtime/NavigationController.kt | Added result navigation methods |
| nibel-runtime/src/main/kotlin/nibel/runtime/NibelNavigationController.kt | Implemented result delivery mechanism |
| nibel-compiler/src/main/kotlin/nibel/compiler/generator/*.kt | Updated code generators for result support |
| sample/feature-/src/main/kotlin/**/.kt | Added sample implementation demonstrating results |
| docs/rfcs/001-navigate-for-result/*.md | Comprehensive RFC documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Suppress("ParcelCreator") | ||
| object NoResult : Parcelable |
There was a problem hiding this comment.
The stub implementation of NoResult is missing the required @Parcelize annotation and proper Parcelable implementation. This will cause runtime crashes when the stub is used. The stub should match the runtime implementation which uses @Parcelize. Add import kotlinx.parcelize.Parcelize and @Parcelize annotation.
| internal object ResultCallbackRegistry { | ||
| private const val MAX_CALLBACKS = 50 | ||
| private const val TTL_MILLIS = 5 * 60 * 1000L // 5 minutes | ||
|
|
||
| private val callbacks = ConcurrentHashMap<String, CallbackEntry>() |
There was a problem hiding this comment.
The global singleton ResultCallbackRegistry could leak callbacks across app sessions or user contexts. Consider scoping this to the NavigationController instance or Activity/Fragment lifecycle to prevent potential data leakage between user sessions, especially in apps with user switching or multi-account support.
| private fun KSClassDeclaration.isCorrectResultDeclaration(symbol: KSNode): Boolean { | ||
| if (Modifier.DATA !in modifiers && classKind != ClassKind.OBJECT) { | ||
| logger.error( | ||
| message = "Result are allowed to be only 'data class' or 'object'.", | ||
| symbol = symbol, | ||
| ) |
There was a problem hiding this comment.
Corrected spelling of 'are allowed' to 'classes are allowed'.
| override fun <TResult : Parcelable> navigateForResult( | ||
| entry: Entry, | ||
| fragmentSpec: FragmentSpec<*>, | ||
| composeSpec: ComposeSpec<*>, | ||
| callback: (TResult?) -> Unit, | ||
| ) { | ||
| // Generate unique request key for this navigation | ||
| val requestKey = UUID.randomUUID().toString() | ||
|
|
||
| // Store the callback with type erasure (we trust compile-time type safety) | ||
| @Suppress("UNCHECKED_CAST") | ||
| ResultCallbackRegistry.storeCallback(requestKey, callback as (Any?) -> Unit) |
There was a problem hiding this comment.
The comment states 'we trust compile-time type safety' but there's no compile-time enforcement that the entry passed actually implements ResultEntry. Consider adding a runtime check or at least document that passing a non-ResultEntry will result in callbacks that are never invoked (silent failure). This could help developers debug issues.
| override fun <TResult : Parcelable> setResultAndNavigateBack(result: TResult) { | ||
| val requestKey = currentRequestKey | ||
| ?: error( | ||
| "setResultAndNavigateBack() called but no request key found. " + | ||
| "This screen was not navigated to via navigateForResult().", | ||
| ) |
There was a problem hiding this comment.
The error message could be more helpful by suggesting the correct usage pattern. Consider updating to: 'setResultAndNavigateBack() called but no request key found. This screen was not navigated to via navigateForResult(). If this screen should return results, ensure it is annotated with a result parameter and called using navigateForResult().'.
| fun storeCallback(requestKey: String, callback: (Any?) -> Unit) { | ||
| // Cleanup stale callbacks if we're at the limit | ||
| if (callbacks.size >= MAX_CALLBACKS) { | ||
| cleanupStaleCallbacks() | ||
|
|
||
| // If still at limit after cleanup, drop the oldest callback | ||
| if (callbacks.size >= MAX_CALLBACKS) { | ||
| val oldestKey = callbacks.entries | ||
| .minByOrNull { it.value.timestamp } | ||
| ?.key | ||
| oldestKey?.let { callbacks.remove(it) } | ||
| } | ||
| } |
There was a problem hiding this comment.
When the callback limit is reached and the oldest callback is dropped, there's no logging or notification. This silent failure could make debugging very difficult. Consider adding a warning log when callbacks are dropped, as this indicates either a memory leak (callbacks not being cleaned up) or a legitimate high volume of concurrent navigations that might require tuning MAX_CALLBACKS.
| class $fragmentName : ComposableFragment()${if (resultQualifiedName != null) { | ||
| val argsType = argsQualifiedName ?: "Parcelable" | ||
| ", ResultEntry<$argsType, $resultQualifiedName>" | ||
| } else { | ||
| "" | ||
| }} { |
There was a problem hiding this comment.
When argsQualifiedName is null but resultQualifiedName is not null, the generated code uses Parcelable as the args type. However, this should use nibel.runtime.NoArgs to be consistent with the rest of the codebase and properly represent a screen with no args but with a result. Update to use nibel.runtime.NoArgs instead of generic Parcelable.
Release notes previewBelow is a preview of the release notes if your PR gets merged. 2.0.0 (2025-10-30)⚠ BREAKING CHANGES
Miscellaneous
Continuous Integration
Documentation
Features
Bug Fixes
Styles |
Breaking changes file
|
…tion Implements the foundation for Navigate-For-Result feature: **Annotations Module:** - Add `result` parameter to @UiEntry with default NoResult::class - Add `result` parameter to @UiExternalEntry with default NoResult::class - Create NoResult marker class (follows NoArgs pattern) - Create DestinationWithResult<TResult> interface for multi-module results - Comprehensive KDoc with usage examples **Runtime Module:** - Create ResultEntry<TArgs, TResult> marker interface - Create NoResult in both runtime and stub modules - Maintain backwards compatibility - all existing code works unchanged **Key Design Decisions:** - ResultEntry is a marker interface (does NOT extend Entry) to avoid sealed hierarchy issues - Result types must be Parcelable (validated at compile-time by KSP) - Default parameter values ensure zero breaking changes - Follows existing patterns (NoArgs → NoResult, args → result) **Verification:** - Both nibel-annotations and nibel-runtime modules compile successfully - No breaking changes to existing codebase - All existing tests continue to pass Phase 1 complete - foundation is ready for NavigationController API (Phase 2). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Implements the NavigationController API for result-based navigation:
**NavigationController Abstract Methods:**
- `navigateForResult(entry, callback)` - Navigate to entry and receive result
- `navigateForResult(externalDestination, callback)` - Navigate to external destination for result
- `setResultAndNavigateBack(result)` - Return result and navigate back
- `cancelResultAndNavigateBack()` - Cancel without result and navigate back
- Comprehensive KDoc with usage examples
**NibelNavigationController Implementation:**
- UUID-based request key generation for tracking result callbacks
- Callback storage mechanism (`resultCallbacks` map)
- Result delivery with automatic cleanup
- Exception handling for navigation failures (rollback on error)
- Type-safe callback mechanism with `@Suppress("UNCHECKED_CAST")`
- Proper state management with `currentRequestKey`
**Key Design Decisions:**
- Callbacks use type erasure internally but maintain compile-time type safety
- Result delivery cleans up callbacks to prevent memory leaks
- Null results indicate cancellation (consistent with Android patterns)
- Request key stored in controller state (will be passed to entries in Phase 3)
- Error handling: rollback callback storage if navigation fails
**Verification:**
- nibel-runtime module compiles successfully
- Detekt linting passes
- No breaking changes to existing navigation
Phase 2 complete - API is ready for code generation (Phase 3).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Implements Phase 3 of the Navigate-for-Result feature by adding code generation support for result types in screen entries. Changes: - Update EntryMetadata to include resultQualifiedName field - Parse result parameter from @UiEntry and @UiExternalEntry annotations - Generate ResultEntry<TArgs, TResult> implementations for entries with results - Add resultType property to generated entry classes - Support both Composable and Fragment entry types - Handle nullable result parameters for backwards compatibility Key Implementation Details: - Entries with result parameter now implement ResultEntry interface - Generated code includes resultType: Class<TResult> property - NoResult class filters to null in metadata (similar to NoArgs) - Helper function parseResultQualifiedName() validates result types - Maintains backwards compatibility with existing entries Technical Notes: - Result types must be data class or object (enforced by validation) - Result types cannot have generic parameters - Works with both internal and external navigation entries Related to: Navigate-for-Result RFC (001) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds the buildRouteName() utility function that was referenced by generated code but was missing from the runtime library. This function creates unique route names for the Compose Navigation library by combining the entry's qualified class name with a hash of its arguments (if present). Fixes compilation errors in generated entry factory code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
kikin81
force-pushed
the
f/result-navigation-feature
branch
from
f62864c to
c35474f
Compare
Breaking changes file
|
Breaking changes file
|
kikin81
changed the title
|
I added navigation with result from FirstScreen to SecondScreen. It works fine when I navigate from Second to First but if I do First -> Second -> Third -back-> Second -back-> First it crashes.
|
Summary
Adds native support for result-based navigation in Nibel, enabling screens to return typed data to their callers with compile-time validation. This implements the Activity Result API pattern familiar to Android developers while leveraging Nibel's type-safety through KSP code generation.
Motivation
Modern Android applications frequently need screens to return data to their callers (photo pickers, form submissions, confirmation dialogs, etc.). Without native support, developers resort to workarounds like SharedViewModels, event buses, or navigation arguments in reverse - all of which lack type safety or break encapsulation.
Key Changes
1. Annotation Extensions
resultparameter to@UiEntryand@UiExternalEntryannotationsNoResult::class(100% backwards compatible)NoResultfor indicating screens without results2. NavigationController API
Added four new methods for result-based navigation:
navigateForResult(entry, callback)- Navigate to internal entry and receive resultnavigateForResult(externalDestination, callback)- Navigate to external destination and receive resultsetResultAndNavigateBack(result)- Return result and navigate backcancelResultAndNavigateBack()- Navigate back without result (callback receives null)3. Type-Safe Interfaces
ResultEntry<TArgs, TResult>- Interface for entries that return resultsDestinationWithResult<TResult>- Interface for multi-module destinations with results4. Code Generation Updates
ResultEntryimplementations for screens withresultparameterImplementationType.FragmentandImplementationType.ComposableExample Usage
Multi-Module Support
Backwards Compatibility
✅ Zero breaking changes - All existing code works without modifications:
resultparameter defaults toNoResult::classTesting
Implementation Phases
Phase 1: Annotations & runtime foundation (annotations, interfaces, marker classes)
Phase 2: NavigationController API implementation (result delivery mechanism)
Phase 3: Compiler code generation (KSP updates for ResultEntry generation)
Phase 4: Testing & validation (comprehensive test coverage)
Phase 5: Documentation (this will be addressed in a follow-up PR)
Documentation
The RFC and PRD for this feature are included in
docs/for reference. README updates will be included before merging.Related Issues
Resolves #[issue-number] (if applicable)
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]