more navi deeplinking changes#238
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds deep link support to the Eatery app by implementing an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/cornellappdev/android/eatery/MainActivity.kt`:
- Around line 159-162: The onNewIntent override currently only calls
setIntent(intent) and doesn't forward deep-link intents to the NavHost; update
the flow so new intents are passed to NavController.handleDeepLink(intent).
Expose the NavHostController created in NavigationSetup (e.g., return or hoist a
remember { NavHostController(...) } or store it in a mutableState/holder that
MainActivity can access) or alternatively have NavigationSetup observe a shared
state/channel for incoming intents; then, in MainActivity.onNewIntent(intent)
call that shared holder or directly call navController.handleDeepLink(intent) to
ensure deep-link navigation is processed on subsequent intents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f4164db-a589-49d4-a655-ce58cf957368
📒 Files selected for processing (2)
app/src/main/java/com/cornellappdev/android/eatery/MainActivity.ktapp/src/main/java/com/cornellappdev/android/eatery/ui/navigation/MainTabbedNavigation.kt
caleb-bit
left a comment
caleb-bit
left a comment
There was a problem hiding this comment.
Looks good! I left a minor nit about concurrency stuff, but I don't think it's a huge bug.
| override fun onNewIntent(intent: Intent) { | ||
| super.onNewIntent(intent) | ||
| setIntent(intent) | ||
| navController?.handleDeepLink(intent) |
There was a problem hiding this comment.
Nit: I think setContent schedules the composition of the content that it's passed, so there's no guarantee of sequential execution, meaning the navController assignment might actually happen after this line is called. You could consider storing the pending intent and handling it if/when navController is initialized. Nonetheless, this is likely a really rare race condition, so it's probably not too consequential. @AndrewCheung360 can weigh in.
There was a problem hiding this comment.
Agreed. Ideally, we would want to make sure that the navcontroller has been initialized first since there isn't a guarantee, so pending intents would make sense
| authTokenRepository.getTokens() | ||
| } | ||
|
|
||
| override fun onNewIntent(intent: Intent) { |
There was a problem hiding this comment.
Since handleDeepLink is only called in onNewIntent and not in onCreate, would there be any issues with it not being handled if Navi tries to open the app but it is closed and not running the background? Not sure if this is the case, but you can ignore if it has been tested and works.
There was a problem hiding this comment.
Yes in that case only onCreate is called, so I think we might need to do something like this.
There was a problem hiding this comment.
Approved, but left some comments. I would probably do some testing by having both the dev builds of eatery and navi installed on your emulator and seeing if you can open eatery from navi if you haven't already. Would recommend adding screen recordings to the PRs for better verification.
3 participants
Overview
Added deeplinking changes for navhost
Changes Made
added
Summary by CodeRabbit
Release Notes
New Features
Improvements