fix: Fix controller focus not being acquired on screen entry#1634
fix: Fix controller focus not being acquired on screen entry#1634VinceBT wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reusable ChangesInitial Focus Navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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}
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}
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
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
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
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
left a comment
phobos665
left a comment
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.
|
@VinceBT - could you please add a recording after merging latest master? |
|
Sorry I closed it by mistake - please provide a recording after merging in latest master |
VinceBT
force-pushed
the
feature/initial-focus
branch
from
8277361 to
b5a875f
Compare
|
@VinceBT - can you provide a recording please |
VinceBT
changed the title
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
b5a875f to
96c5de7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/ui/screen/login/TwoFactorAuthScreen.kt (1)
134-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace inlined retry with
requestInitialFocushelper.The retry loop here duplicates
Modifier.requestInitialFocusfromFocusModifiers.kt. SincefocusRequesteris already applied via.focusRequester(focusRequester)on line 156, you can replace both theLaunchedEffectblock and that modifier call with a singlerequestInitialFocus(focusRequester)on the text field modifier, consistent withRecommendedGameScreen.kt.♻️ Proposed refactor
val focusRequester = remember { FocusRequester() } val keyboardController = LocalSoftwareKeyboardController.current - LaunchedEffect(Unit) { - // Node may not be placed on the first frame; retry until requestFocus() takes. - repeat(5) { - try { - if (focusRequester.requestFocus()) return@LaunchedEffect - } catch (_: IllegalStateException) { - } - delay(32) - } - } Column(modifier = Modifier.fillMaxWidth()) {And on the text field modifier:
NoExtractOutlinedTextField( modifier = Modifier .fillMaxWidth() - .focusRequester(focusRequester) + .requestInitialFocus(focusRequester) .dpadFocusEscape(This also removes the need for the
kotlinx.coroutines.delayimport.🤖 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/TwoFactorAuthScreen.kt` around lines 134 - 142, Replace the inline LaunchedEffect retry loop in TwoFactorAuthScreen with the existing requestInitialFocus(focusRequester) modifier on the text field, removing the separate focusRequester modifier call. Remove the now-unused kotlinx.coroutines.delay import, while preserving the text field’s initial-focus behavior consistent with RecommendedGameScreen.app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (1)
598-607: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFocus retry loop is correct but duplicates
requestInitialFocushelper logic.The retry pattern here (5 attempts,
IllegalStateExceptioncatch,delay(32)) is identical to theModifier.requestInitialFocushelper introduced inFocusModifiers.kt. However, sinceplayButtonFocusRequesteris passed as a parameter toPrimaryActionButtonrather than applied via a modifier chain,requestInitialFocuscan't be used directly without refactoringPrimaryActionButtonto accept a modifier.Consider extracting just the retry portion into a reusable
suspendhelper (e.g.,suspend fun FocusRequester.requestFocusWithRetry()) that bothrequestInitialFocusand this call site can share, eliminating the duplicated loop body.🤖 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/LibraryAppScreen.kt` around lines 598 - 607, Extract the shared five-attempt focus retry behavior from the LaunchedEffect block and Modifier.requestInitialFocus into a suspend FocusRequester helper, such as requestFocusWithRetry(), preserving the IllegalStateException handling and 32ms delay. Update both callers to use the helper, including the playButtonFocusRequester flow, while keeping the existing focus-triggering behavior unchanged.
🤖 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/library/LibraryAppScreen.kt`:
- Around line 598-607: Extract the shared five-attempt focus retry behavior from
the LaunchedEffect block and Modifier.requestInitialFocus into a suspend
FocusRequester helper, such as requestFocusWithRetry(), preserving the
IllegalStateException handling and 32ms delay. Update both callers to use the
helper, including the playButtonFocusRequester flow, while keeping the existing
focus-triggering behavior unchanged.
In `@app/src/main/java/app/gamenative/ui/screen/login/TwoFactorAuthScreen.kt`:
- Around line 134-142: Replace the inline LaunchedEffect retry loop in
TwoFactorAuthScreen with the existing requestInitialFocus(focusRequester)
modifier on the text field, removing the separate focusRequester modifier call.
Remove the now-unused kotlinx.coroutines.delay import, while preserving the text
field’s initial-focus behavior consistent with RecommendedGameScreen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29d80392-ade3-4844-ae15-2dc8c38fcc85
📒 Files selected for processing (8)
app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.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/TwoFactorAuthScreen.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
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt
- app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt
- app/src/main/java/app/gamenative/ui/util/FocusModifiers.kt
- app/src/main/java/app/gamenative/ui/screen/settings/SettingsScreen.kt
- app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt
3 participants
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 96c5de7. Summary will update on new commits.
Summary by CodeRabbit