feat: add held back-button routing#138
Conversation
📝 WalkthroughWalkthroughThe PR introduces a hold-to-action state machine for the Escape key: pressing Escape arms a 1-second timer; releasing before timeout triggers ChangesCancel-hold feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 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 `@src/ui/app/Main.qml`:
- Around line 1599-1608: The timer currently only sets root._cancelHoldReady but
tests expect navigation immediately when the hold threshold passes; update the
Timer (cancelHoldTimer) onTriggered handler to not only set
root._cancelHoldReady but also invoke the same action/path used on key release
to perform navigation (i.e., call the existing cancel/release handler that the
key-release path uses) when root._cancelPending is true so navigation occurs
while the key is still held.
In `@tests/ui/tst_navigation.qml`:
- Around line 135-152: The tests expect the cancel-hold behavior to execute when
the cancelHoldTimer fires, so update the cancelHoldTimer handler in Main.qml to
perform the actual navigation/modal open (same logic currently in
handleKeyRelease) instead of only setting _cancelHoldReady; specifically, call
the code that routes to screenHub or sets quitConfirmModalVisible, clear/disable
_cancelHoldPending/_cancelHoldReady and stop the timer, and ensure
handleKeyRelease checks those flags and returns early (no-op) if the timer
already executed to avoid double actions.
🪄 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: d6ac5eb4-b87d-420e-ac84-bc77d2bc3dbe
📒 Files selected for processing (3)
rust/zaparoo-core/src/input_actions.rssrc/ui/app/Main.qmltests/ui/tst_navigation.qml
| Timer { | ||
| id: cancelHoldTimer | ||
| interval: root._cancelHoldMs | ||
| repeat: false | ||
| onTriggered: { | ||
| if (!root._cancelPending) | ||
| return; | ||
| root._cancelHoldReady = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Timer only sets flag—tests expect navigation on timer fire, not on release.
The timer's onTriggered sets _cancelHoldReady = true but doesn't dispatch the action. The tests (test_cancel_hold_from_games_routes_directly_to_hub, test_cancel_hold_on_hub_opens_quit_confirm) use tryCompare before calling handleKeyRelease, expecting navigation to occur when the timer fires. With the current implementation, navigation only happens on key release.
If the intended UX is "navigate when hold threshold passes (key still held)," the timer should dispatch the action:
Proposed fix for timer-triggered dispatch
Timer {
id: cancelHoldTimer
interval: root._cancelHoldMs
repeat: false
onTriggered: {
if (!root._cancelPending)
return;
root._cancelHoldReady = true;
+ root._stopCancelHold();
+ root.handleAction("cancel_hold");
}
}Alternatively, if the intended UX is "navigate on release after holding 1 second," the tests need restructuring to call handleKeyRelease and use a wait() or tryCompare on a different property.
🤖 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 `@src/ui/app/Main.qml` around lines 1599 - 1608, The timer currently only sets
root._cancelHoldReady but tests expect navigation immediately when the hold
threshold passes; update the Timer (cancelHoldTimer) onTriggered handler to not
only set root._cancelHoldReady but also invoke the same action/path used on key
release to perform navigation (i.e., call the existing cancel/release handler
that the key-release path uses) when root._cancelPending is true so navigation
occurs while the key is still held.
| function test_cancel_hold_from_games_routes_directly_to_hub(): void { | ||
| main.activeScreen = main.screenGames; | ||
| main.handleKey(Qt.Key_Escape); | ||
| tryCompare(main, "activeScreen", main.screenHub); | ||
| compare(main._cancelHoldPending, false, "Hold timer must clear after firing"); | ||
| main.handleKeyRelease(Qt.Key_Escape); | ||
| compare(main.activeScreen, main.screenHub, "Release after long-hold must not fire a second cancel"); | ||
| } | ||
|
|
||
| function test_cancel_hold_on_hub_opens_quit_confirm(): void { | ||
| main.activeScreen = main.screenHub; | ||
| main.handleKey(Qt.Key_Escape); | ||
| tryCompare(main, "quitConfirmModalVisible", true); | ||
| compare(main.activeScreen, main.screenHub, "Long-hold Back on Hub must stay on Hub and ask for quit"); | ||
| compare(main._cancelHoldPending, false, "Hold timer must clear after opening quit confirm"); | ||
| main.handleKeyRelease(Qt.Key_Escape); | ||
| compare(main.quitConfirmModalVisible, true, "Release after long-hold must not close the quit confirm"); | ||
| } |
There was a problem hiding this comment.
Tests expect timer-triggered navigation but implementation dispatches on release.
Both test_cancel_hold_from_games_routes_directly_to_hub and test_cancel_hold_on_hub_opens_quit_confirm call tryCompare immediately after handleKey(Qt.Key_Escape), before any handleKeyRelease. This expects the timer's onTriggered to navigate/open the modal. However, the current cancelHoldTimer only sets _cancelHoldReady = true—the actual action dispatch happens in handleKeyRelease.
These tests will timeout because the screen/modal state never changes until release.
If the fix is applied to cancelHoldTimer (dispatching on timer fire), these tests are correct. If the intended behavior is release-triggered, restructure:
Alternative test structure for release-triggered behavior
function test_cancel_hold_from_games_routes_directly_to_hub(): void {
main.activeScreen = main.screenGames;
main.handleKey(Qt.Key_Escape);
+ wait(root._cancelHoldMs + 50); // Wait past hold threshold
+ compare(main.activeScreen, main.screenGames, "Must not navigate while key is still held");
+ main.handleKeyRelease(Qt.Key_Escape);
- tryCompare(main, "activeScreen", main.screenHub);
+ compare(main.activeScreen, main.screenHub);
compare(main._cancelHoldPending, false, "Hold timer must clear after firing");
- main.handleKeyRelease(Qt.Key_Escape);
- compare(main.activeScreen, main.screenHub, "Release after long-hold must not fire a second cancel");
}This comment is related to the cancelHoldTimer issue flagged in Main.qml.
🤖 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 `@tests/ui/tst_navigation.qml` around lines 135 - 152, The tests expect the
cancel-hold behavior to execute when the cancelHoldTimer fires, so update the
cancelHoldTimer handler in Main.qml to perform the actual navigation/modal open
(same logic currently in handleKeyRelease) instead of only setting
_cancelHoldReady; specifically, call the code that routes to screenHub or sets
quitConfirmModalVisible, clear/disable _cancelHoldPending/_cancelHoldReady and
stop the timer, and ensure handleKeyRelease checks those flags and returns early
(no-op) if the timer already executed to avoid double actions.
1 participant
Summary
Testing
Summary by CodeRabbit
Release Notes