Add focus requesters for search bar and TV navigation rail#1
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Review Summary by QodoAdd bidirectional focus navigation between search and TV rail
WalkthroughsDescription• Add focus navigation support between search bar and TV navigation rail • Implement left focus requester in search input field for TV navigation • Add content focus requester to TV navigation rail for right navigation • Initialize TV rail focus on component mount using LaunchedEffect Diagramflowchart LR
TVRail["TV Navigation Rail"]
SearchBar["Search Bar"]
MainActivity["MainActivity"]
MainActivity -- "firstItemFocusRequester" --> TVRail
MainActivity -- "contentFocusRequester" --> SearchBar
TVRail -- "focusProperties right" --> SearchBar
SearchBar -- "focusProperties left" --> TVRail
TVRail -- "LaunchedEffect requestFocus" --> TVRail
File Changes1. app/src/main/kotlin/moe/koiverse/archivetune/MainActivity.kt
|
Code Review by Qodo
1. Rail right focus dead-end
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
The implementation adds bidirectional focus navigation between the TV navigation rail and search bar. The code:
No runtime errors, logic bugs, or security issues identified. Reviewed by minimax-m2.5-20260211 · 68,897 tokens |
There was a problem hiding this comment.
Code Review
This pull request implements explicit focus navigation between the TV navigation rail and the search bar by passing focus requesters and utilizing focusProperties. Feedback indicates that a redundant LaunchedEffect in TvNavigationRail should be removed to avoid focus management conflicts with existing logic in MainActivity. Additionally, the modifier chains in both SearchBar and TvNavigationRail should be refactored to use conditional logic within the focusProperties block for more idiomatic and readable Compose code.
| ) { | ||
| LaunchedEffect(firstItemFocusRequester) { | ||
| firstItemFocusRequester?.requestFocus() | ||
| } | ||
|
|
There was a problem hiding this comment.
This LaunchedEffect should be removed. MainActivity.kt already manages the initial focus for the navigation rail (lines 990-1001) with specific logic to ensure it doesn't steal focus when the search bar is active. Removing this ensures that focus is managed centrally and avoids potential race conditions or focus-stealing during recomposition.
) {
| .then( | ||
| if (leftFocusRequester != null) { | ||
| Modifier.focusProperties { | ||
| left = leftFocusRequester | ||
| } | ||
| } else { | ||
| Modifier | ||
| } | ||
| ) |
There was a problem hiding this comment.
The modifier chain can be simplified by using focusProperties directly with a conditional check inside the block, rather than using .then(if (...)). This is cleaner and more idiomatic in Compose.
.focusProperties {
leftFocusRequester?.let { left = it }
}| modifier = Modifier | ||
| .then( | ||
| if (index == 0 && firstItemFocusRequester != null) { | ||
| Modifier.focusRequester(firstItemFocusRequester) | ||
| } else { | ||
| Modifier | ||
| } | ||
| ) | ||
| .then( | ||
| if (contentFocusRequester != null) { | ||
| Modifier.focusProperties { | ||
| right = contentFocusRequester | ||
| } | ||
| } else { | ||
| Modifier | ||
| } | ||
| ), |
There was a problem hiding this comment.
The modifier chain for TvNavigationRailItem can be simplified. Instead of using multiple .then(if (...)) blocks, you can call focusProperties directly and use a conditional check inside the block. This improves readability and maintainability.
modifier = Modifier
.then(
if (index == 0 && firstItemFocusRequester != null) {
Modifier.focusRequester(firstItemFocusRequester)
} else {
Modifier
}
)
.focusProperties {
contentFocusRequester?.let { right = it }
},| modifier = Modifier | ||
| .then( | ||
| if (index == 0 && firstItemFocusRequester != null) { | ||
| Modifier.focusRequester(firstItemFocusRequester) | ||
| } else { | ||
| Modifier | ||
| } | ||
| ) | ||
| .then( | ||
| if (contentFocusRequester != null) { | ||
| Modifier.focusProperties { | ||
| right = contentFocusRequester | ||
| } | ||
| } else { | ||
| Modifier | ||
| } | ||
| ), |
There was a problem hiding this comment.
1. Rail right focus dead-end 🐞 Bug ☼ Reliability
TvNavigationRail forces DPAD-right to contentFocusRequester, but MainActivity wires it to searchBarFocusRequester while TopSearch is not composed when active is false. This can trap focus on the rail or trigger FocusRequester errors on TV when navigating right from the rail on non-search screens.
Agent Prompt
## Issue description
`TvNavigationRail` routes DPAD-right to a `contentFocusRequester` that is currently wired to `searchBarFocusRequester`, but `TopSearch` is not composed in the common state where the rail is visible (`!active`). This can cause focus to get stuck on the rail or crash when focus traversal attempts to focus an unattached FocusRequester.
## Issue Context
- Rail is visible only when `shouldShowNavigationBar` which requires `!active`.
- `TopSearch` is only composed when `active` (or when route starts with `search/`).
- So when the rail is visible, `searchBarFocusRequester` often has no attached node.
## Fix Focus Areas
- app/src/main/kotlin/moe/koiverse/archivetune/MainActivity.kt[762-767]
- app/src/main/kotlin/moe/koiverse/archivetune/MainActivity.kt[1169-1186]
- app/src/main/kotlin/moe/koiverse/archivetune/MainActivity.kt[1354-1359]
- app/src/main/kotlin/moe/koiverse/archivetune/ui/component/TvNavigationRail.kt[74-96]
## Suggested fix
- Only set `right = contentFocusRequester` when the target is guaranteed to be composed (otherwise pass `null` / skip the focusProperties override).
- Alternatively, pass a real "content" FocusRequester that is always attached when the rail is visible (e.g., the first focusable item in the current screen content), instead of using `searchBarFocusRequester`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
No description provided.