fix(nav): Desktop crash on SearchScreen enum nav arg#606
Conversation
There was a problem hiding this comment.
rainxchzed has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
WalkthroughNavigation route serialization for ChangesNavigation Serialization
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.kt (1)
117-123: 💤 Low valueConsider logging when enum parsing fails.
The
runCatching { valueOf(name) }.getOrNull()approach silently discards invalid platform names. While defaulting tonullfor an optional navigation argument is reasonable, adding a log statement when parsing fails could help debug unexpected navigation state or catch typos in future call sites.💡 Optional: Add logging on parse failure
val initialPlatform = args.initialPlatform?.let { name -> runCatching { zed.rainxch.search.presentation.model.SearchPlatformUi .valueOf(name) - }.getOrNull() + }.onFailure { + // Log: "Invalid platform name in nav args: $name" + }.getOrNull() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.kt` around lines 117 - 123, The current silent parsing of args.initialPlatform via runCatching { SearchPlatformUi.valueOf(name) }.getOrNull() should log failures so invalid platform names are visible; change the expression to attach an onFailure handler that logs the invalid name and the throwable (e.g., using the app logger or println) before returning null—refer to the initialPlatform variable, args.initialPlatform, and zed.rainxch.search.presentation.model.SearchPlatformUi.valueOf to locate and update the parsing block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.kt`:
- Around line 117-123: The current silent parsing of args.initialPlatform via
runCatching { SearchPlatformUi.valueOf(name) }.getOrNull() should log failures
so invalid platform names are visible; change the expression to attach an
onFailure handler that logs the invalid name and the throwable (e.g., using the
app logger or println) before returning null—refer to the initialPlatform
variable, args.initialPlatform, and
zed.rainxch.search.presentation.model.SearchPlatformUi.valueOf to locate and
update the parsing block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66d4bdd1-a328-40bd-a86e-d8e91e706446
📒 Files selected for processing (2)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.kt
Hotfix. Regression from #597 (Task 12 platform chips).
SearchScreen(val initialPlatform: SearchPlatformUi? = null)works on Android but crashes Desktop:```
IllegalArgumentException: Route ...SearchScreen could not find any NavType
for argument initialPlatform of type SearchPlatformUi? - typeMap received was {}
at RouteSerializerKt.forEachIndexedKType (RouteSerializer.kt:190)
at NavDestinationBuilder. (nonAndroid.kt:49)
```
Compose Navigation's non-Android serializer has no built-in NavType for enums. Cheapest fix: switch nav arg to
String?(enum name), convert atcomposable<>boundary viaSearchPlatformUi.valueOf(name).Test plan
Source: user-reported crash on desktop launch.
Summary by CodeRabbit