Fix Navigation 3 scene metadata key checks#927
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies ScenesSnippets.kt by replacing the idiomatic use of the contains extension function with explicit containsKey(Key.toString()) calls for metadata lookups and removing the associated import. The reviewer recommends reverting these changes to use the more idiomatic in operator, which improves readability and maintains consistency with other snippets in the repository. Specific code suggestions were provided to restore the idiomatic syntax for both DetailKey and ListKey checks.
| import androidx.navigation3.runtime.NavEntry | ||
| import androidx.navigation3.runtime.NavKey | ||
| import androidx.navigation3.runtime.NavMetadataKey | ||
| import androidx.navigation3.runtime.contains |
There was a problem hiding this comment.
This import should be retained if you use the idiomatic in operator or contains extension for metadata key checks, as suggested in the comments below.
| entries.lastOrNull()?.takeIf { it.metadata.contains(DetailKey) } ?: return null | ||
| val listEntry = entries.findLast { it.metadata.contains(ListKey) } ?: return null | ||
| entries.lastOrNull()?.takeIf { | ||
| it.metadata.containsKey(DetailKey.toString()) |
There was a problem hiding this comment.
Using containsKey(DetailKey.toString()) is less idiomatic than using the contains extension function (or the in operator) provided by the Navigation 3 runtime. This change also introduces an inconsistency with other snippets in the repository, such as MetadataSnippets.kt, which demonstrate the use of the extension. If the extension is functional, it is preferred for better readability and type safety.
| it.metadata.containsKey(DetailKey.toString()) | |
| DetailKey in it.metadata |
| it.metadata.containsKey(DetailKey.toString()) | ||
| } ?: return null | ||
| val listEntry = | ||
| entries.findLast { it.metadata.containsKey(ListKey.toString()) } ?: return null |
There was a problem hiding this comment.
Consistent with the check for DetailKey, consider using the idiomatic in operator here as well.
| entries.findLast { it.metadata.containsKey(ListKey.toString()) } ?: return null | |
| entries.findLast { ListKey in it.metadata } ?: return null |
nero002
force-pushed
the
dheeraj/fix-navigation3-scene-metadata-keys
branch
from
fe9c912 to
23d8289
Compare
1 participant
This makes the list-detail scene snippet explicit about looking up entries in NavEntry metadata by String key.
Test: ANDROID_HOME=/Users/Dheeraj/Library/Android/sdk ./gradlew :compose:snippets:compileDebugKotlin