Merged
Conversation
…unknown-destination # Conflicts: # tiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/ComposableNavigation.kt
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR introduces a new DestinationLoader abstraction to replace function-based destination resolution, adds a TiamatUnsafeApi annotation, brings key-based destination identification, and refactors navigation APIs throughout to use the loader pattern. Also adds graph merging capabilities and a Modifier parameter to TiamatPreview. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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)
tiamat-destinations/tiamat-destinations/src/commonMain/kotlin/com/composegears/tiamat/destinations/TiamatGraph.kt (1)
31-39:⚠️ Potential issue | 🟠 MajorOperator
plusshould not mutate the left operand.Current behavior mutates
thisfor merged graphs, creating inconsistency with the documented contract ("returns a new merged graph") and unexpected side effects in chained operations likea + b + c.♻️ Proposed change
public operator fun plus(other: TiamatGraph): TiamatGraph { - if (this is TiamatMergedGraph) { - this.allDestinations.addAll(other.destinations()) - return this - } else { - val mergedGraph = TiamatMergedGraph() - mergedGraph.allDestinations.addAll(this.destinations()) - mergedGraph.allDestinations.addAll(other.destinations()) - return mergedGraph - } + val mergedGraph = TiamatMergedGraph() + mergedGraph.allDestinations.addAll(this.destinations()) + mergedGraph.allDestinations.addAll(other.destinations()) + return mergedGraph }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiamat-destinations/tiamat-destinations/src/commonMain/kotlin/com/composegears/tiamat/destinations/TiamatGraph.kt` around lines 31 - 39, The operator plus implementation currently mutates the left operand when it is a TiamatMergedGraph; change it so it always returns a new TiamatMergedGraph without mutating this. In the TiamatGraph.operator fun plus(other: TiamatGraph) implementation, create a fresh TiamatMergedGraph instance, addAll(this.destinations()) and addAll(other.destinations()) into that new instance (instead of calling this.allDestinations.addAll), and return the new instance; reference the TiamatMergedGraph class, the plus operator on TiamatGraph, the allDestinations collection, and the destinations() method to locate and update the code.
🧹 Nitpick comments (2)
tiamat-destinations/tiamat-destinations/src/commonMain/kotlin/com/composegears/tiamat/destinations/TiamatGraph.kt (1)
22-22: Prefer an abstractdestinations()contract over runtimeerror().This change weakens API safety by turning a compile-time requirement into a runtime crash path.
♻️ Proposed change
- public fun destinations(): Array<NavDestination<*>> = error("Graph is empty") + public fun destinations(): Array<NavDestination<*>>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiamat-destinations/tiamat-destinations/src/commonMain/kotlin/com/composegears/tiamat/destinations/TiamatGraph.kt` at line 22, The destinations() function currently returns error("Graph is empty"), which introduces a runtime crash; change the API to require implementers to provide destinations at compile time by making destinations() an abstract function (e.g., declare TiamatGraph as an abstract class or convert it to an interface) with signature fun destinations(): Array<NavDestination<*>> so concrete graph implementations must override it; update any classes extending/implementing TiamatGraph to implement destinations() accordingly and remove the runtime error fallback.tiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/DestinationLoader.kt (1)
62-67: Consider using a map for O(1) lookup in FromArray.The current implementation uses
firstOrNullwhich is O(n). For applications with many destinations, this could be optimized with a map-based lookup.♻️ Optional optimization using Map
public class FromArray( private val destinations: Array<NavDestination<*>> ) : DestinationLoader { + private val destinationMap = destinations.associateBy { it.key } + override fun load(key: String): NavDestination<*>? = - destinations.firstOrNull { it.key == key } + destinationMap[key] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/DestinationLoader.kt` around lines 62 - 67, The FromArray class uses destinations.firstOrNull { it.key == key } (load) which is O(n); change it to build an index map (e.g., val destinationByKey = destinations.associateBy { it.key }) once in the class (constructor or init) and have load(key: String) return destinationByKey[key]; update the FromArray class to hold the Map alongside the existing destinations and use it for O(1) lookups while preserving the same load signature and return type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/ComposablePreview.kt`:
- Around line 17-18: The KDoc for the ComposablePreview function references a
non-existent parameter "@param destinationLoader"; update the KDoc to match the
actual function signature in ComposablePreview.kt by removing the "@param
destinationLoader" line (or replace it with the correct parameter name if the
function should accept a loader), ensuring the remaining param tags (e.g.,
"@param modifier") match the symbols in the ComposablePreview function
declaration.
---
Outside diff comments:
In
`@tiamat-destinations/tiamat-destinations/src/commonMain/kotlin/com/composegears/tiamat/destinations/TiamatGraph.kt`:
- Around line 31-39: The operator plus implementation currently mutates the left
operand when it is a TiamatMergedGraph; change it so it always returns a new
TiamatMergedGraph without mutating this. In the TiamatGraph.operator fun
plus(other: TiamatGraph) implementation, create a fresh TiamatMergedGraph
instance, addAll(this.destinations()) and addAll(other.destinations()) into that
new instance (instead of calling this.allDestinations.addAll), and return the
new instance; reference the TiamatMergedGraph class, the plus operator on
TiamatGraph, the allDestinations collection, and the destinations() method to
locate and update the code.
---
Nitpick comments:
In
`@tiamat-destinations/tiamat-destinations/src/commonMain/kotlin/com/composegears/tiamat/destinations/TiamatGraph.kt`:
- Line 22: The destinations() function currently returns error("Graph is
empty"), which introduces a runtime crash; change the API to require
implementers to provide destinations at compile time by making destinations() an
abstract function (e.g., declare TiamatGraph as an abstract class or convert it
to an interface) with signature fun destinations(): Array<NavDestination<*>> so
concrete graph implementations must override it; update any classes
extending/implementing TiamatGraph to implement destinations() accordingly and
remove the runtime error fallback.
In
`@tiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/DestinationLoader.kt`:
- Around line 62-67: The FromArray class uses destinations.firstOrNull { it.key
== key } (load) which is O(n); change it to build an index map (e.g., val
destinationByKey = destinations.associateBy { it.key }) once in the class
(constructor or init) and have load(key: String) return destinationByKey[key];
update the FromArray class to hold the Map alongside the existing destinations
and use it for O(1) lookups while preserving the same load signature and return
type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
gradle/tiamat.tomlsample/shared/src/commonMain/kotlin/composegears/tiamat/sample/App.kttiamat-destinations/tiamat-destinations/src/commonMain/kotlin/com/composegears/tiamat/destinations/TiamatGraph.kttiamat/api/jvm/tiamat.apitiamat/api/tiamat.klib.apitiamat/src/commonMain/kotlin/com/composegears/tiamat/TiamatUnsafeApi.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/ComposableNavController.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/ComposableNavigation.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/ComposablePreview.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/DestinationLoader.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/TransitionController.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/navigation/NavController.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/navigation/NavDestination.kttiamat/src/commonMain/kotlin/com/composegears/tiamat/navigation/NavEntry.kttiamat/src/commonTest/kotlin/com/composegears/tiamat/navigation/NavControllerTests.kttiamat/src/commonTest/kotlin/com/composegears/tiamat/navigation/NavEntryTests.kt
tiamat/src/commonMain/kotlin/com/composegears/tiamat/compose/ComposablePreview.kt
Outdated
Show resolved
Hide resolved
egorikftp
approved these changes
Mar 3, 2026
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.
Summary by CodeRabbit
Release Notes
New Features
+operator for merging navigation graphs.Chores