fix: prevent quick menu tab debounce when controller active#996
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:
📝 WalkthroughWalkthroughMade QuickMenu state and focus behavior conditional on visibility, removed two LaunchedEffect blocks, added a dedicated scroll state for the Screen Effects tab and passed it into ScreenEffectsTabContent, and updated the remaining focus-retry to call the HUD content’s first-item focus requester. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
xXJSONDeruloXx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/component/QuickMenu.kt (1)
517-532: LGTM — consolidating focus logic with tab-aware targeting looks correct.The change appropriately:
- Preserves the selected tab across menu close/reopen cycles (via
rememberSaveableat line 264)- Requests focus on the correct tab based on current
selectedTabvalue- Keeps
selectedTabout of the effect keys intentionally, since re-focusing should only happen on visibility transitionsOptional: The catch block swallows all
Exceptiontypes. SinceFocusRequester.requestFocus()specifically throwsIllegalStateExceptionwhen the requester isn't attached, catching that explicitly would be more precise:♻️ Optional refinement
- } catch (_: Exception) { + } catch (_: IllegalStateException) {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/component/QuickMenu.kt` around lines 517 - 532, The catch currently swallows all Exception types in the LaunchedEffect(isVisible) retry loop; change it to catch IllegalStateException specifically so only the focus-requester-not-attached case is handled while other exceptions bubble: update the try/catch inside the LaunchedEffect that calls .requestFocus() for the when(selectedTab) branch (referencing hudTabFocusRequester, effectsTabFocusRequester, controllerTabFocusRequester and selectedTab/QuickMenuTab) to catch IllegalStateException instead of Exception and keep the delay(80) retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/component/QuickMenu.kt`:
- Around line 517-532: The catch currently swallows all Exception types in the
LaunchedEffect(isVisible) retry loop; change it to catch IllegalStateException
specifically so only the focus-requester-not-attached case is handled while
other exceptions bubble: update the try/catch inside the LaunchedEffect that
calls .requestFocus() for the when(selectedTab) branch (referencing
hudTabFocusRequester, effectsTabFocusRequester, controllerTabFocusRequester and
selectedTab/QuickMenuTab) to catch IllegalStateException instead of Exception
and keep the delay(80) retry behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b41cc4b9-e41f-47ef-b3e1-569f61755d36
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/component/QuickMenu.kt
xXJSONDeruloXx
force-pushed
the
fix/quick-menu-touch-tab-double-tap
branch
from
b526cf5 to
00f8b0c
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/component/QuickMenu.kt">
<violation number="1">
P2: Effects tab scroll position persistence was regressed by removing hoisted scroll state, causing reset when tab content is recomposed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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/app/gamenative/ui/component/QuickMenu.kt`:
- Line 264: The selectedTab reset is caused by using rememberSaveable(isVisible)
as a key and focus restoration always targets hudItemFocusRequester; change the
state to rememberSaveable() without isVisible so selectedTab persists across
visibility toggles (use var selectedTab by rememberSaveable {
mutableIntStateOf(QuickMenuTab.HUD) }) and update the LaunchedEffect that
restores focus to choose the correct FocusRequester based on the current
selectedTab (inspect QuickMenuTab values and dispatch focus to the corresponding
requester such as hudItemFocusRequester, weaponsItemFocusRequester,
inventoryItemFocusRequester, etc., instead of always calling
hudItemFocusRequester). Ensure the LaunchedEffect's key includes selectedTab (or
no key if you trigger manually) so the right element receives focus when the UI
becomes visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1056710f-3343-4484-bc06-84ed6ecb8961
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/ui/component/QuickMenu.ktapp/src/main/java/app/gamenative/ui/component/ScreenEffectsPanel.kt
| } | ||
|
|
||
| var selectedTab by rememberSaveable { mutableIntStateOf(QuickMenuTab.HUD) } | ||
| var selectedTab by rememberSaveable(isVisible) { mutableIntStateOf(QuickMenuTab.HUD) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 '\bQuickMenu\s*\(' --type=ktRepository: utkarshdalal/GameNative
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Find QuickMenu.kt file
fd QuickMenu.kt --type fRepository: utkarshdalal/GameNative
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Search for QuickMenu instantiations (without type restriction)
rg -n -C4 '\bQuickMenu\s*\(' --type kotlin 2>/dev/null || rg -n -C4 '\bQuickMenu\s*\('Repository: utkarshdalal/GameNative
Length of output: 3523
🏁 Script executed:
#!/bin/bash
# Get the QuickMenu.kt file and examine key lines
# First find the file
find . -name "QuickMenu.kt" -type f | head -1 | xargs cat -n | sed -n '250,280p;510,540p'Repository: utkarshdalal/GameNative
Length of output: 2552
🏁 Script executed:
#!/bin/bash
# Look at QuickMenu usage in XServerScreen
sed -n '1940,1965p' app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktRepository: utkarshdalal/GameNative
Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Check if there are other QuickMenu call sites besides XServerScreen
rg -l 'QuickMenu\(' --type kotlin app/src/main/java/Repository: utkarshdalal/GameNative
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Check if selectedTab is ever saved/restored elsewhere (e.g., SavedStateHandle, DataStore)
rg -n 'selectedTab' app/src/main/java/app/gamenative/ui/component/QuickMenu.ktRepository: utkarshdalal/GameNative
Length of output: 898
🏁 Script executed:
#!/bin/bash
# Check if there's any mechanism in the broader codebase to persist QuickMenu selectedTab
rg -n 'QuickMenuTab' app/src/main/java/app/gamenative/ | grep -E '(DataStore|SavedStateHandle|remembered|saved)'Repository: utkarshdalal/GameNative
Length of output: 49
Fix tab reset and focus targeting for selected tab restoration.
The component resets selectedTab to HUD on each visibility toggle because rememberSaveable(isVisible) uses visibility as a reset key. Additionally, the LaunchedEffect always targets hudItemFocusRequester regardless of which tab is actually selected, so even if the tab state were preserved, focus would still go to the wrong UI element. This prevents the "restore selected tab" feature from working.
Suggested fix
- var selectedTab by rememberSaveable(isVisible) { mutableIntStateOf(QuickMenuTab.HUD) }
+ var selectedTab by rememberSaveable { mutableIntStateOf(QuickMenuTab.HUD) }
- LaunchedEffect(isVisible) {
+ LaunchedEffect(isVisible, selectedTab, renderer) {
if (isVisible) {
+ val targetRequester = when (selectedTab) {
+ QuickMenuTab.HUD -> hudItemFocusRequester
+ QuickMenuTab.EFFECTS -> if (renderer != null) effectsItemFocusRequester else effectsTabFocusRequester
+ QuickMenuTab.CONTROLLER -> controllerItemFocusRequester
+ else -> hudItemFocusRequester
+ }
repeat(3) {
try {
- hudItemFocusRequester.requestFocus()
+ targetRequester.requestFocus()
return@LaunchedEffect
} catch (_: Exception) {
delay(80)
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var selectedTab by rememberSaveable(isVisible) { mutableIntStateOf(QuickMenuTab.HUD) } | |
| var selectedTab by rememberSaveable { mutableIntStateOf(QuickMenuTab.HUD) } | |
| LaunchedEffect(isVisible, selectedTab, renderer) { | |
| if (isVisible) { | |
| val targetRequester = when (selectedTab) { | |
| QuickMenuTab.HUD -> hudItemFocusRequester | |
| QuickMenuTab.EFFECTS -> if (renderer != null) effectsItemFocusRequester else effectsTabFocusRequester | |
| QuickMenuTab.CONTROLLER -> controllerItemFocusRequester | |
| else -> hudItemFocusRequester | |
| } | |
| repeat(3) { | |
| try { | |
| targetRequester.requestFocus() | |
| return@LaunchedEffect | |
| } catch (_: Exception) { | |
| delay(80) | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/ui/component/QuickMenu.kt` at line 264, The
selectedTab reset is caused by using rememberSaveable(isVisible) as a key and
focus restoration always targets hudItemFocusRequester; change the state to
rememberSaveable() without isVisible so selectedTab persists across visibility
toggles (use var selectedTab by rememberSaveable {
mutableIntStateOf(QuickMenuTab.HUD) }) and update the LaunchedEffect that
restores focus to choose the correct FocusRequester based on the current
selectedTab (inspect QuickMenuTab values and dispatch focus to the corresponding
requester such as hudItemFocusRequester, weaponsItemFocusRequester,
inventoryItemFocusRequester, etc., instead of always calling
hudItemFocusRequester). Ensure the LaunchedEffect's key includes selectedTab (or
no key if you trigger manually) so the right element receives focus when the UI
becomes visible.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Just so I understand this, you're persisting the scroll-state through injection rather than of its own accord? |
|
@phobos665 yeah intentionally, so the state is restored if you go to a diff tab and come back, since it's conditionally composed. This way it survives changing tabs and close/open of qam |
3 participants
follow up from #976
handles focus debounce in case where invoked with controller but tab touched on touch screen, rather than back with touch swipe then tab change on touch without controller active to render border focusable.
also restores selected tab and scroll position so you are not bumped to top of menu every invoke
Summary by CodeRabbit