Skip to content

Conversation

@szymonlesisz
Copy link
Contributor

@szymonlesisz szymonlesisz commented Nov 18, 2025

Running bluetoothctl hungs forever if system service is disabled causing suite loading/handshake issue.

Steps to reproduce:

  1. in terminal systemctl stop bluetooth
  2. run suite app or in terminal bluetoothctl show

🔍🖥️ Suite web test results: View in Currents

🔍🖥️ Suite desktop test results: View in Currents

@szymonlesisz szymonlesisz added the no-project This label is used to specify that PR doesn't need to be added to a project label Nov 18, 2025
@szymonlesisz
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

Walkthrough

The changes modify Bluetooth adapter information retrieval in a Linux-specific context. A new helper function checks Bluetooth service status via systemctl, and the adapter info function now conditionally invokes bluetoothctl based on service availability. Error handling is restructured using a match statement that evaluates outcomes from bluetoothctl and hciconfig commands, prioritizing bluetoothctl results when available and falling back to hciconfig on failure. Platform-specific conditional compilation is added for Linux while maintaining existing macOS and Windows implementations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • The new is_bluetooth_service_running() function implementation requires verification of systemctl invocation syntax and reliability
  • The match statement logic across all four combinations of (bluetoothctl, hciconfig) success/failure states needs careful validation for correctness
  • Platform-specific conditional compilation guards (#[cfg(target_os = "linux")]) should be verified for proper scoping
  • Error propagation priority (bluetoothctl error propagated when both fail) requires confirmation against intended behavior
  • Edge cases in the fallback pattern (hciconfig used only when bluetoothctl fails) should be evaluated

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description explains the issue and provides reproduction steps, but omits required template sections like QA notes and related issue links. Add missing sections: specify QA testing focus areas, link the related issue, and clarify the fix's scope and impact on the bluetooth service check logic.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: fixing a Linux-specific issue in the Bluetooth server's get_info method when systemctl is disabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/bt-server-get-info

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/transport-bluetooth/src/server/methods/get_info.rs (2)

22-29: Good fix for the hang issue.

The conditional execution of bluetoothctl based on service status effectively prevents the indefinite hang. The logic is clear and the error handling is appropriate.

Minor suggestion: Consider making the error message more descriptive, e.g., "Bluetooth service is not running" instead of "Service disabled" for better clarity when debugging.


34-39: Consider simplifying the match arms.

The match logic correctly prioritizes bluetoothctl output over hciconfig and handles all cases appropriately. However, lines 35-36 can be combined since both return the same value when bluetoothctl succeeds, regardless of hciconfig's result.

Optional simplification:

 match (bluetoothctl, hciconfig) {
-    (Ok(info), Ok(_)) => Ok(info),
-    (Ok(info), Err(_)) => Ok(info),
+    (Ok(info), _) => Ok(info),
     (Err(_), Ok(info)) => Ok(String::from_utf8_lossy(&info.stdout).to_string()),
     (Err(error), Err(_)) => Err(error.into()),
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b3a70c3 and 37d62ae.

📒 Files selected for processing (1)
  • packages/transport-bluetooth/src/server/methods/get_info.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: szymonlesisz
Repo: trezor/trezor-suite PR: 18224
File: packages/suite/src/actions/bluetooth/initBluetoothThunk.ts:19-35
Timestamp: 2025-04-09T09:15:31.193Z
Learning: The Bluetooth feature is being implemented incrementally across multiple PRs. The "TO BE DONE" comments in Bluetooth event handlers are intentional placeholders, with implementations planned for subsequent PRs after the core BluetoothProcess infrastructure is in place.
Learnt from: szymonlesisz
Repo: trezor/trezor-suite PR: 21135
File: packages/transport-bluetooth/src/server/platform/linux.rs:34-45
Timestamp: 2025-09-01T10:19:22.902Z
Learning: In the Linux Bluetooth implementation for Trezor Suite, both `connect_with_timeout` and `discover_services` functions automatically handle connecting to the peripheral if it's not already connected, so redundant connection calls are handled gracefully by the function implementations.
Learnt from: szymonlesisz
Repo: trezor/trezor-suite PR: 18126
File: packages/transport-bluetooth/src/server/methods/get_info.rs:33-33
Timestamp: 2025-04-07T18:10:35.860Z
Learning: For the trezor-bluetooth project, Windows-specific Bluetooth adapter information will be implemented using a Windows crate rather than PowerShell commands.
Learnt from: Lemonexe
Repo: trezor/trezor-suite PR: 17002
File: packages/suite/src/views/settings/SettingsDebug/Bluetooth.tsx:26-34
Timestamp: 2025-02-18T10:05:29.381Z
Learning: Error handling for Bluetooth operations in the Bluetooth settings component is planned for future implementation and should not be included in the initial implementation.
📚 Learning: 2025-04-07T18:10:35.860Z
Learnt from: szymonlesisz
Repo: trezor/trezor-suite PR: 18126
File: packages/transport-bluetooth/src/server/methods/get_info.rs:33-33
Timestamp: 2025-04-07T18:10:35.860Z
Learning: For the trezor-bluetooth project, Windows-specific Bluetooth adapter information will be implemented using a Windows crate rather than PowerShell commands.

Applied to files:

  • packages/transport-bluetooth/src/server/methods/get_info.rs
📚 Learning: 2025-07-22T10:14:40.261Z
Learnt from: szymonlesisz
Repo: trezor/trezor-suite PR: 20314
File: packages/transport-bluetooth/src/server/platform/linux.rs:1-37
Timestamp: 2025-07-22T10:14:40.261Z
Learning: In the btleplug Rust library on Linux with BlueZ backend, `peripheral.id()` returns the device path in the format "hciX/dev_..." which is suitable for constructing BlueZ D-Bus object paths, while `peripheral.address()` returns the Bluetooth address in the format "XX:XX:XX:XX:XX:XX".

Applied to files:

  • packages/transport-bluetooth/src/server/methods/get_info.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Linting and formatting
  • GitHub Check: Other Checks
  • GitHub Check: Check for new Circular Imports
  • GitHub Check: Build libs for publishing
  • GitHub Check: Unit Tests
  • GitHub Check: Type Checking
  • GitHub Check: build-app
  • GitHub Check: build-web
🔇 Additional comments (1)
packages/transport-bluetooth/src/server/methods/get_info.rs (1)

9-18: Excellent approach to prevent the bluetoothctl hang.

The helper function correctly checks if the bluetooth service is running before attempting to execute bluetoothctl. Using systemctl is-active --quiet with the status check is the right approach, and the unwrap_or(false) properly handles cases where systemctl itself isn't available or fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-project This label is used to specify that PR doesn't need to be added to a project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants