Fix controller focus not being acquired on screen entry#1634
Open
VinceBT wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a reusable ChangesInitial Focus Navigation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt (1)
586-600: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate this with the existing tab bootstrap effect.
This adds a third entry-focus path, but the
LaunchedEffect(state.currentTab)above still runs on first composition, so loaded tabs can queue two initialrequestContentFocusOrDefer(0)calls at the same time. If this guard is meant to prevent focus fights, it also needs to gate the existing tab bootstrap.🤖 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 `@app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt` around lines 586 - 600, The initial content-focus guard in LibraryScreen adds a third focus path, but the existing LaunchedEffect(state.currentTab) bootstrap still issues its own first-load requestContentFocusOrDefer(0) call. Consolidate the startup focus logic so both the current-tab bootstrap and the new content-loaded effect share the same one-shot guard (for example via didInitialContentFocus) and only one initial focus request can be queued.
🤖 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.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt`:
- Around line 619-623: The username auto-focus on entry is being attempted only
once and `runCatching` can silently fail before the field is attached, leaving
the login screen unfocused. Update the `LaunchedEffect(Unit)` block in
`UserLoginScreen` to use the existing
`requestInitialFocus(usernameFocusRequester)` helper, or copy its retry behavior
here, so focus keeps retrying until `usernameFocusRequester` can actually attach
and receive focus.
In `@app/src/main/java/app/gamenative/ui/util/FocusModifiers.kt`:
- Line 21: The LaunchedEffect in FocusModifiers.kt is only keyed by enabled, so
a new FocusRequester instance will not retrigger the initial focus request when
the requester changes. Update the effect keys in the focus modifier logic
(LaunchedEffect in the FocusRequester-related composable) to include the
requester instance alongside enabled, so the effect restarts whenever a
different FocusRequester is supplied.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt`:
- Around line 586-600: The initial content-focus guard in LibraryScreen adds a
third focus path, but the existing LaunchedEffect(state.currentTab) bootstrap
still issues its own first-load requestContentFocusOrDefer(0) call. Consolidate
the startup focus logic so both the current-tab bootstrap and the new
content-loaded effect share the same one-shot guard (for example via
didInitialContentFocus) and only one initial focus request can be queued.
🪄 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: 74e41970-7c84-44ee-9763-4426317236c3
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/RecommendedGameScreen.ktapp/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsScreen.ktapp/src/main/java/app/gamenative/ui/util/FocusModifiers.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt:370">
P2: New portrait tab focus handling duplicates the reusable `Modifier.requestInitialFocus()` helper introduced by this PR. `DownloadsTabRow` hand-rolls the same `remember` + `FocusRequester` map + retrying `LaunchedEffect` pattern instead of delegating to the shared helper, creating inconsistent retry parameters and multiple places to update if the focus logic changes.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt:434">
P1: Initial-focus retry loop ignores `requestFocus()` return value. `FocusRequester.requestFocus()` returns `Boolean` (`true` on success, `false` if cancelled); a `false` result is not an exception. When focus is denied but no exception is thrown, this code still sets `requestedInitialFocus = true` and exits, so retries are skipped and focus may never actually be acquired.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt (1)
619-629: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the inline retry loop with
requestInitialFocus.This now duplicates the shared retry contract in
FocusModifiers.kt, so the login screen can drift from the other screens the next time that helper changes. It’s simpler to move the behavior onto the username field’s modifier and delete thisLaunchedEffect.Suggested change
- // Focus the username field on entry so the first controller/D-pad press is not wasted. - // Retry: the field may not be attached on the first frame. - LaunchedEffect(Unit) { - repeat(5) { - try { - usernameFocusRequester.requestFocus() - return@LaunchedEffect - } catch (_: IllegalStateException) { - delay(32) - } - } - } ... NoExtractOutlinedTextField( value = username, onValueChange = onUsername, singleLine = true, label = { Text(stringResource(R.string.login_username_hint)) }, modifier = Modifier .fillMaxWidth() - .focusRequester(usernameFocusRequester) + .requestInitialFocus(usernameFocusRequester) .dpadFocusEscape( focusManager = focusManager, keyboardController = keyboardController, onDown = { passwordFocusRequester.requestFocus() }, ),🤖 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 `@app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt` around lines 619 - 629, The username auto-focus logic in UserLoginScreen should use the shared requestInitialFocus helper instead of the local LaunchedEffect retry loop. Move the initial focus behavior onto the username field’s modifier via requestInitialFocus from FocusModifiers.kt, and remove the duplicated repeat/delay retry block so the login screen stays aligned with the other screens.
🤖 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 `@app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt`:
- Around line 619-629: The username auto-focus logic in UserLoginScreen should
use the shared requestInitialFocus helper instead of the local LaunchedEffect
retry loop. Move the initial focus behavior onto the username field’s modifier
via requestInitialFocus from FocusModifiers.kt, and remove the duplicated
repeat/delay retry block so the login screen stays aligned with the other
screens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df4a2110-7506-432c-a508-f3a6680ad206
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.ktapp/src/main/java/app/gamenative/ui/util/FocusModifiers.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/util/FocusModifiers.kt
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt (1)
349-355: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the swallowed exception to the focus-layout case.
These loops currently catch every
Exception, while the shared focus helper only masksIllegalStateException. Keeping the catch broad here can hide unrelated runtime failures and silently fall back to the unfocused state.Suggested change
- } catch (_: Exception) { + } catch (_: IllegalStateException) { }Also applies to: 434-440
🤖 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 `@app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt` around lines 349 - 355, The focus request loop in DownloadsScreen is catching every Exception, which is broader than the shared focus helper behavior and can hide unrelated failures. Narrow the catch in the LaunchedEffect focus attempt around focusRequester.requestFocus() to only the focus-layout case (matching the helper’s IllegalStateException handling), and apply the same fix to the other identical block referenced by the review.
🤖 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 `@app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt`:
- Around line 349-355: The focus request loop in DownloadsScreen is catching
every Exception, which is broader than the shared focus helper behavior and can
hide unrelated failures. Narrow the catch in the LaunchedEffect focus attempt
around focusRequester.requestFocus() to only the focus-layout case (matching the
helper’s IllegalStateException handling), and apply the same fix to the other
identical block referenced by the review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09cc39d7-2a09-42a3-a0d2-333455dfc81c
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.ktapp/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.ktapp/src/main/java/app/gamenative/ui/util/FocusModifiers.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/ui/util/FocusModifiers.kt
- app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt
phobos665
reviewed
Jul 1, 2026
phobos665
left a comment
phobos665
left a comment
Contributor
There was a problem hiding this comment.
@utkarshdalal quite interested in this one.
Basically allow for easier controller focus on certain screens.
The big one is rgarding the FocusModifiers, but seems pretty interesting to me.
Owner
|
@VinceBT - could you please add a recording after merging latest master? |
Owner
|
Sorry I closed it by mistake - please provide a recording after merging in latest master |
On most screens nothing was focused on entry, so the first D-pad/controller
press was consumed just to seed focus ('press A in the void'). Add a reusable
Modifier.requestInitialFocus(focusRequester, enabled) helper that attaches the
requester and requests focus once the node is laid out (with retry), and apply
it on:
- SettingsScreen: the back button
- UserLoginScreen: the username field
- RecommendedGameScreen: the primary action button
- LibraryScreen: content focus once the list loads (one-shot, reusing the
existing requestContentFocusOrDefer with the same overlay guards)
- DownloadsScreen: the portrait tab row (the landscape sidebar already did this)
VinceBT
force-pushed
the
feature/initial-focus
branch
from
8277361 to
b5a875f
Compare
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
3 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.
Description
On most screens, opening them with a controller left nothing focused, so the first D-pad/A press did nothing — you had to press once "into the void" before anything reacted.
This adds a small reusable
Modifier.requestInitialFocus()helper and uses it to focus a sensible first element on entry:The helper retries focus a few times because the node isn't always laid out on the first frame.
Recording
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Fixes missing controller focus on screen entry so the first D-pad/A press works immediately. Adds a reusable
Modifier.requestInitialFocus()that requests focus after layout and retries until it succeeds.Modifier.requestInitialFocus()(attachesFocusRequester; retries untilrequestFocus()returns true; effect keyed on requester/enabled).Written for commit b5a875f. Summary will update on new commits.
Summary by CodeRabbit