fix(desktop): align tray, shortcuts, and macOS chrome with current CMP desktop guidance#6205
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:
📝 WalkthroughWalkthroughDesktop window visibility now persists across tray transitions, keyboard shortcuts use platform-appropriate modifiers, macOS JVM configuration enables system title-bar appearance, and offline Flatpak verification targets Flathub with OpenJDK 25 and Gradle 9.6.1. ChangesDesktop behavior and Flatpak verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TrayState
participant MeshtasticWindow
participant ComposeWindow
TrayState->>MeshtasticWindow: provide visibility state
MeshtasticWindow->>ComposeWindow: show or hide window
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
desktopApp/src/main/kotlin/org/meshtastic/desktop/Main.kt (1)
361-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a type alias to avoid repeated FQN for
KeyEvent.The fully-qualified
androidx.compose.ui.input.key.KeyEventreceiver/parameter type (Lines 364, 369) suggests a naming clash with anotherKeyEventin this file's imports. Atypealias ComposeKeyEvent = androidx.compose.ui.input.key.KeyEventat the top of the file would improve readability without behavior change. The modifier-detection logic itself (Cmd on macOS, Ctrl elsewhere) is correct.♻️ Optional refactor
+private typealias ComposeKeyEvent = androidx.compose.ui.input.key.KeyEvent + private val isMacOS = DesktopOS.current() == DesktopOS.MacOS /** Platform-conventional shortcut modifier: Cmd on macOS, Ctrl on Windows/Linux. */ -private val androidx.compose.ui.input.key.KeyEvent.isShortcutModifierPressed: Boolean +private val ComposeKeyEvent.isShortcutModifierPressed: Boolean get() = if (isMacOS) isMetaPressed else isCtrlPressed /** Handles Cmd/Ctrl-key shortcuts. Returns `true` if the event was consumed. */ private fun handleKeyboardShortcut( - event: androidx.compose.ui.input.key.KeyEvent, + event: ComposeKeyEvent, multiBackstack: MultiBackstack, exitApplication: () -> Unit, ): Boolean {🤖 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 `@desktopApp/src/main/kotlin/org/meshtastic/desktop/Main.kt` around lines 361 - 373, Replace the repeated fully qualified KeyEvent references in the isShortcutModifierPressed extension and handleKeyboardShortcut with a file-level typealias such as ComposeKeyEvent, preserving the existing shortcut behavior and avoiding conflicts with the other KeyEvent import.
🤖 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 `@desktopApp/src/main/kotlin/org/meshtastic/desktop/Main.kt`:
- Around line 361-373: Replace the repeated fully qualified KeyEvent references
in the isShortcutModifierPressed extension and handleKeyboardShortcut with a
file-level typealias such as ComposeKeyEvent, preserving the existing shortcut
behavior and avoiding conflicts with the other KeyEvent import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e65b15fb-9226-4b24-8500-f5798399076d
📒 Files selected for processing (2)
desktopApp/build.gradle.ktsdesktopApp/src/main/kotlin/org/meshtastic/desktop/Main.kt
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDesktop window visibility now persists across tray transitions, keyboard shortcuts use platform-appropriate modifiers, and macOS JVM configuration enables system title-bar appearance. ChangesDesktop behavior
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/verify-flatpak/verify.sh (1)
58-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
VID_REPOto reflect the Flathub repository.The variable now points to Flathub, so the stale
VID_REPOname can mislead future maintenance and cause incorrect repository updates.-VID_REPO="https://github.com/flathub/org.meshtastic.MeshtasticDesktop.git" +FLATHUB_REPO="https://github.com/flathub/org.meshtastic.MeshtasticDesktop.git"Update all references accordingly. As per coding guidelines, keep naming aligned with
AGENTS.mdand the Flatpak verification guidance inscripts/verify-flatpak/README.md.🤖 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 `@scripts/verify-flatpak/verify.sh` around lines 58 - 60, Rename the VID_REPO variable to a name that clearly identifies the Flathub repository, following conventions in AGENTS.md and scripts/verify-flatpak/README.md, and update every reference to the variable throughout scripts/verify-flatpak/verify.sh.Source: Coding guidelines
.github/workflows/verify-flatpak.yml (1)
77-80: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin the clone to the same branch used by the local verifier.
This clone follows GitHub’s default branch, while
scripts/verify-flatpak/verify.sh:99-100explicitly resets toorigin/master. If the Flathub default changes, CI and local verification can build different packaging revisions.Canonical fix
git clone --depth 1 --recurse-submodules \ + --branch master \ https://github.com/flathub/org.meshtastic.MeshtasticDesktop.git \As per coding guidelines, keep this workflow aligned with
AGENTS.mdand the CI verification playbook in.skills/testing-ci/SKILL.md.🤖 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 @.github/workflows/verify-flatpak.yml around lines 77 - 80, Update the “Clone the Flathub packaging repo” step to explicitly clone the master branch, matching the origin/master reset performed by verify.sh and keeping CI aligned with local verification.Source: Coding guidelines
🤖 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 @.github/workflows/verify-flatpak.yml:
- Around line 77-80: Update the “Clone the Flathub packaging repo” step to
explicitly clone the master branch, matching the origin/master reset performed
by verify.sh and keeping CI aligned with local verification.
In `@scripts/verify-flatpak/verify.sh`:
- Around line 58-60: Rename the VID_REPO variable to a name that clearly
identifies the Flathub repository, following conventions in AGENTS.md and
scripts/verify-flatpak/README.md, and update every reference to the variable
throughout scripts/verify-flatpak/verify.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd2f09e6-6598-49c9-b319-84ef7a65acce
📒 Files selected for processing (4)
.github/workflows/verify-flatpak.ymlscripts/verify-flatpak/README.mdscripts/verify-flatpak/desktop-offline.yamlscripts/verify-flatpak/verify.sh
✅ Files skipped from review due to trivial changes (1)
- scripts/verify-flatpak/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/verify-flatpak/desktop-offline.yaml
42003af to
8453071
Compare
|
Trimmed the 4 Flatpak/JDK-25 commits off this branch — they now live (identical patches) in #6208, which owns the JDK 25 story. This PR is back to purely the CMP desktop fixes. Heads-up on CI: verify-flatpak's offline job will fail here until #6208 merges — main's offline manifest still only ships |
…P desktop guidance Audit of the desktop shell against JetBrains' official Compose Multiplatform desktop docs (CMP 1.11.1, our pin) surfaced three gaps: - Hide-to-tray dropped the Window from composition, destroying the navigation backstack and scroll positions; use the documented Window(visible=) pattern so the UI tree survives a tray round trip - Keyboard shortcuts checked only the meta key, leaving them dead on the Windows/Linux distributions; use the platform-conventional modifier (Cmd on macOS, Ctrl elsewhere) - Add -Dapple.awt.application.appearance=system so the macOS title bar follows the system dark theme Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows reports AltGr as Ctrl+Alt, so typing AltGr characters (e.g. @ on German layouts = AltGr+Q) would have fired shortcuts — including quit. Require Alt up on the Ctrl path. Also import KeyEvent instead of repeating the FQN (no clash exists in this file). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8453071 to
d9f87f3
Compare
1 participant
Audited the desktop shell against JetBrains' official Compose Multiplatform desktop docs (
kotlin-multiplatform-dev-docs@ HEAD; newest stable CMP = 1.11.1, which is our pin). The shell already matches current guidance almost everywhere — this PR fixes the three real gaps the audit surfaced, plus a JDK-25 gap in the offline Flatpak manifest that this PR's CI run exposed.🐛 Bug Fixes
Windowfrom composition, destroying the navigation backstack and scroll positions; restoring from the tray restarted the UI from scratch. Now uses the documented minimize-to-tray pattern (Window(visible = …)) so the compose tree survives the round trip.@on German layouts = AltGr+Q) would fire shortcuts, including quit.-Dapple.awt.application.appearance=systemjvmArg; previously the window chrome stayed light while the app content was dark.desktopApptolanguageVersion=25, but the offline manifest still shipped only theopenjdk21SDK extension; with toolchain auto-provisioning disabled (the build is offline by design),compileJavafailed on both arches. The manifest now ships dual JDKs —openjdk21for the Gradle daemon (gradle-daemon-jvm.propertiespinstoolchainVersion=21) andopenjdk25for the desktop toolchain — with the coupling documented. Also realigned the manifest's bundled Gradle dist with the 9.6.1 wrapper (it was silently passing on 9.6.0). Surfaced here rather than on chore(desktop): bump Compose Desktop runtime from JBR 21 to JBR 25 #6203 becauseverify-flatpakonly runs on PRs touching desktop paths.🧹 Chores
scripts/verify-flatpak/verify.shcloneflathub/org.meshtastic.MeshtasticDesktop(what Flathub actually builds, seeded from vidplace7's dev repo on 2026-07-08) instead of the dev fork, including themaster-vs-maindefault-branch fix in the script's refresh path.🧹 Audit notes (no change needed)
androidx.compose.ui.window.Trayremains the only documented tray API through CMP 1.12 — nothing to migrate.onExternalDrag,ClipboardManager, desktop-specific@Preview,Key.Home, explicit hot-reload plugin pin); UI tests are already on ComposeUiTest v2 (refactor: leverage CMP 1.11 + Lifecycle 2.11 — v2 test API, Json privacy, dropUnlessResumed nav guards #5112).version.set("7.9.1")); branch is rebased onto that JBR 25 bump.Testing Performed
Full baseline green on the JBR 25 toolchain (re-run after the AltGr guard):
spotlessApply spotlessCheck detekt assembleDebug test allTests, 0 failures. The Flatpak manifest fix is exercised by this PR's ownverify-flatpakjobs.🤖 Generated with Claude Code
Summary by CodeRabbit