Implement keyboard navigation shortcuts and refactor app navigation#324
Merged
Conversation
- Introduce `KeyboardNavigation` and `KeyboardNavigationEvent` to handle desktop-specific keyboard shortcuts. - Add support for `Ctrl+F` (or `Meta+F`) to navigate to the Search screen from anywhere in the application. - Rename `navBackStack` to `navController` in `Main.kt` and integrate `ObserveAsEvents` to handle keyboard navigation events. - Update `DesktopApp.kt` to capture key events and dispatch them through `KeyboardNavigation`. - Localize the application title in the desktop window using `Res.string.app_name`. - Refactor `RateLimitDialog` logic for better null safety and cleaner state handling. - Reorder `initKoin()` and deep link handling in the desktop entry point to ensure proper initialization. - Perform minor code formatting and organization in `AppNavigation.kt`.
- Relocate the app icon from `composeApp` to the `core:presentation` module. - Rename the resource file from `app-icon.png` to `app_icon.png`. - Update the app icon image asset.
Contributor
WalkthroughThe pull request introduces a keyboard navigation system for the desktop app, enabling users to press Ctrl+F to navigate to the search screen. It refactors the navigation controller usage, integrates event-driven architecture for keyboard events, and improves resource localization in the desktop entry point. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DesktopApp
participant KeyboardNavigation
participant Main
participant NavController
participant SearchScreen
User->>DesktopApp: Press Ctrl+F
DesktopApp->>KeyboardNavigation: onKeyClicked(OnCtrlFClick)
KeyboardNavigation->>KeyboardNavigation: emit event to Flow
Main->>KeyboardNavigation: collect from events
Main->>Main: ObserveAsEvents listens
alt Not already on SearchScreen
Main->>NavController: navigate to SearchScreen
NavController->>SearchScreen: show screen
SearchScreen-->>NavController: ready
else Already on SearchScreen
Main->>Main: skip navigation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/desktop/KeyboardNavigation.kt (1)
7-12: Best practice: Add explicit buffering or checktrySendresult to prevent potential event loss.
Channel<KeyboardNavigationEvent>()is unbuffered andtrySendfailure is not checked. While the current usage pattern (subscription established before user interaction) mitigates real-world risk, this code pattern is fragile and could silently drop events if the composition lifecycle changes.Suggested improvement
object KeyboardNavigation { - private val _events = Channel<KeyboardNavigationEvent>() + private val _events = Channel<KeyboardNavigationEvent>(capacity = Channel.BUFFERED) val events = _events.receiveAsFlow() fun onKeyClicked(event: KeyboardNavigationEvent) { - _events.trySend(event) + _events.trySend(event).onFailure { + // Log dropped event if needed + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/desktop/KeyboardNavigation.kt` around lines 7 - 12, The _events Channel is unbuffered and onKeyClicked uses trySend without checking the result, risking silent event loss; fix by creating _events with an explicit buffer (e.g., Channel.BUFFERED or Channel(capacity = N)) instead of Channel<KeyboardNavigationEvent>(), and/or check the ChannelResult returned by _events.trySend(event) inside onKeyClicked (use isSuccess/isFailure or onFailure to log or handle the dropped event) so events are not silently dropped; update references to _events, events = _events.receiveAsFlow(), and the onKeyClicked(event: KeyboardNavigationEvent) function accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/desktop/KeyboardNavigation.kt`:
- Around line 7-12: The _events Channel is unbuffered and onKeyClicked uses
trySend without checking the result, risking silent event loss; fix by creating
_events with an explicit buffer (e.g., Channel.BUFFERED or Channel(capacity =
N)) instead of Channel<KeyboardNavigationEvent>(), and/or check the
ChannelResult returned by _events.trySend(event) inside onKeyClicked (use
isSuccess/isFailure or onFailure to log or handle the dropped event) so events
are not silently dropped; update references to _events, events =
_events.receiveAsFlow(), and the onKeyClicked(event: KeyboardNavigationEvent)
function accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: acf5a88d-3ac8-4639-b3b4-16af56356646
⛔ Files ignored due to path filters (2)
composeApp/src/commonMain/composeResources/drawable/app-icon.pngis excluded by!**/*.pngcore/presentation/src/commonMain/composeResources/drawable/app_icon.pngis excluded by!**/*.png
📒 Files selected for processing (5)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/Main.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/desktop/KeyboardNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/desktop/KeyboardNavigationEvent.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt
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
1 participant
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.
KeyboardNavigationandKeyboardNavigationEventto handle desktop-specific keyboard shortcuts.Ctrl+F(orMeta+F) to navigate to the Search screen from anywhere in the application.navBackStacktonavControllerinMain.ktand integrateObserveAsEventsto handle keyboard navigation events.DesktopApp.ktto capture key events and dispatch them throughKeyboardNavigation.Res.string.app_name.RateLimitDialoglogic for better null safety and cleaner state handling.initKoin()and deep link handling in the desktop entry point to ensure proper initialization.AppNavigation.kt.Summary by CodeRabbit
Release Notes
New Features
Improvements