Patch manifest #4687
Patch manifest #4687
Conversation
WalkthroughRefactors Tauri startup into a library-run model, replaces tauri_plugin_shell with tauri_plugin_opener and tauri_plugin_serialplugin, removes legacy Android serial module, adds Android MainActivity and USB patch/build scripts, migrates JS serial protocol to tauri-plugin-serialplugin-api, updates CI to Java 21 and runs Android manifest patching. Changes
Sequence Diagram(s)sequenceDiagram
participant Web as JS Frontend
participant TauriJS as TauriSerial (JS)
participant PluginAPI as tauri-plugin-serialplugin-api
participant RustLib as src-tauri::run
participant Android as MainActivity / OS
Note over Web,TauriJS: Discovery
Web->>TauriJS: loadDevices()
TauriJS->>PluginAPI: available_ports()
PluginAPI->>RustLib: enumerate ports
alt Android needs permission
RustLib->>Android: check/notify devices
Android->>RustLib: permission events
RustLib->>PluginAPI: grant/handle permission
end
PluginAPI-->>TauriJS: port list
TauriJS-->>Web: addedDevice events
Note over Web,TauriJS: Connect & I/O
Web->>TauriJS: connect(path, opts)
TauriJS->>PluginAPI: open(path, opts)
PluginAPI->>RustLib: open_port
PluginAPI-->>TauriJS: connected
loop read loop
TauriJS->>PluginAPI: readBinary()
PluginAPI-->>TauriJS: data
TauriJS-->>Web: receive event
end
Web->>TauriJS: send(data)
TauriJS->>PluginAPI: writeBinary(data)
PluginAPI-->>TauriJS: bytes_written
Web->>TauriJS: disconnect()
TauriJS->>PluginAPI: cancelListen(), close()
PluginAPI-->>TauriJS: closed
TauriJS-->>Web: disconnect event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/patch-android-manifest.sh (2)
38-82: Device filter creation is solid, but consider future maintainability.The device_filter.xml generation is well-structured and covers major Betaflight-compatible devices. The heredoc approach is clean and readable.
Consider documenting (in ANDROID_USB.md) how users can add additional USB devices if needed, since the device list is currently hardcoded in the script.
29-37: Sed approach works for current Tauri-generated manifests but could be more robust.The concern about multiple activities does not apply here: Tauri-generated Android manifests contain a single
MainActivitywith the launcher intent-filter. The sed regex0,/<\/activity>/scorrectly targets this only activity.However, the sed approach remains fragile for XML manipulation. Using
xmlstarletinstead would be more robust, preserve XML formatting, and scale better if the manifest structure ever changes. Consider this as an optional refactoring to improve maintainability and portability.ANDROID_USB.md (1)
56-77: Expand troubleshooting and backup restoration guidance.The documentation is comprehensive, but could be enhanced:
- Backup restoration: Line 62 mentions a backup is created, but doesn't explain where it's stored (
$MANIFEST_PATH.bak) or how users can restore it if needed.- Error recovery: The troubleshooting section could mention what to do if the patch script fails or produces an invalid manifest (hint: restore from the backup).
Consider adding a brief "Restoring from Backup" section that explains the
.bakfile location and how to recover if something goes wrong.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ANDROID_USB.md(1 hunks)scripts/patch-android-manifest.sh(1 hunks)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
🔇 Additional comments (2)
scripts/patch-android-manifest.sh (1)
1-20: LGTM!Proper validation, helpful error messages, and backup creation. The manifest path and error handling are appropriate for the Tauri Android structure.
ANDROID_USB.md (1)
1-20: LGTM!Clear problem statement with good explanation of the Android rebuild issue and the patch-based solution. The required permissions list is accurate and helps readers understand what the script adds.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/patch-android-manifest.sh (1)
22-38: ** sed portability issues already flagged in past review.**The sed portability concerns for lines 22-29 and similar issues for lines 31-38 (USB intent filter insertion) have been covered in the past review comment. Both sed operations suffer from the same macOS incompatibility and lack of XML validation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(2 hunks)scripts/patch-android-manifest.sh(1 hunks)src-tauri/Cargo.toml(1 hunks)src-tauri/src/main.rs(1 hunks)src-tauri/src/serial_android.rs(0 hunks)
💤 Files with no reviewable changes (1)
- src-tauri/src/serial_android.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). (2)
- GitHub Check: Tauri Android preview (APK)
- GitHub Check: Tauri Linux preview (deb/appimage)
🔇 Additional comments (5)
src-tauri/Cargo.toml (1)
23-24: LGTM: Unified serial plugin simplifies cross-platform support.The replacement of platform-specific serial dependencies with a single
tauri-plugin-serialplugindependency streamlines the build configuration and aligns with the simplified plugin initialization inmain.rs..github/workflows/ci.yml (2)
129-133: LGTM: Java 21 upgrade appropriate for modern Android development.
154-156: LGTM: Manifest patching integrated into build workflow.The patch step is correctly positioned after Android project initialization and before the build.
src-tauri/src/main.rs (1)
7-9: LGTM: Platform-agnostic plugin initialization.The removal of conditional compilation directives and platform-specific branching simplifies the codebase and aligns with the unified
tauri-plugin-serialplugindependency.scripts/patch-android-manifest.sh (1)
43-77: Document the source and compatibility of hardcoded USB device IDs.The device filter includes numerous vendor-id/product-id pairs without documenting their sources or verification status. For maintainability and security, please verify and document:
- The source of each vendor/product ID pair (manufacturer specs, community testing, etc.)
- Which Betaflight firmware versions or hardware these correspond to
- Whether all listed devices have been tested with this implementation
Consider adding inline comments for each device group with references to official documentation or issue numbers where these IDs were verified.
scripts/patch-android-manifest.sh
Outdated
| if [ -f "$APP_BUILD_GRADLE" ]; then | ||
| echo "Adding USB serial library dependency..." | ||
| if ! grep -q "usb-serial-for-android" "$APP_BUILD_GRADLE"; then | ||
| cat >> "$APP_BUILD_GRADLE" << 'EOF' | ||
| dependencies { | ||
| // USB Serial library for Android | ||
| implementation("com.github.mik3y:usb-serial-for-android:3.8.0") | ||
| } | ||
| EOF | ||
| echo "✓ USB serial library dependency added!" | ||
| else | ||
| echo "USB serial library dependency already present" | ||
| fi | ||
| else | ||
| echo "Warning: $APP_BUILD_GRADLE not found, skipping dependency addition" | ||
| fi |
There was a problem hiding this comment.
Risk of duplicate dependencies blocks in build.gradle.kts.
The script appends a dependencies block if "usb-serial-for-android" is not found (line 86), but doesn't check whether a dependencies block already exists in the file. Gradle build files typically have an existing dependencies block, so this logic will likely append a second block, potentially causing build errors or unexpected behavior.
Modify the script to insert the dependency within an existing dependencies block, or fail with a clear error message if the structure is unexpected:
if [ -f "$APP_BUILD_GRADLE" ]; then
echo "Adding USB serial library dependency..."
if ! grep -q "usb-serial-for-android" "$APP_BUILD_GRADLE"; then
- cat >> "$APP_BUILD_GRADLE" << 'EOF'
-
-dependencies {
- // USB Serial library for Android
- implementation("com.github.mik3y:usb-serial-for-android:3.8.0")
-}
-EOF
+ if grep -q "^dependencies {" "$APP_BUILD_GRADLE"; then
+ # Insert within existing dependencies block
+ sed -i '/^dependencies {/a \ // USB Serial library for Android\n implementation("com.github.mik3y:usb-serial-for-android:3.8.0")' "$APP_BUILD_GRADLE"
+ else
+ echo "Error: No dependencies block found in $APP_BUILD_GRADLE"
+ exit 1
+ fi
echo "✓ USB serial library dependency added!"Note: This suggestion also inherits the sed portability issues flagged elsewhere in this review.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/patch-android-manifest.sh (2)
35-37: Complex multi-line sed substitution is fragile and non-portable.Line 35 uses a non-standard sed pattern (
0,/<\/activity>/s|...|) with extensive embedded newlines that will fail or corrupt output on macOS and other systems with BSD sed.The same platform-detection or xmlstarlet fallback from the previous comment applies here. A portable alternative using xmlstarlet:
- sed -i '0,/<\/activity>/s|</activity>|\n <!-- Intent filter for USB device attach -->\n <intent-filter>\n <action android:name="android.hardware.usb.action.USB_DEVICE_ATTACHED" />\n </intent-filter>\n\n <!-- USB device filter metadata -->\n <meta-data\n android:name="android.hardware.usb.action.USB_DEVICE_ATTACHED"\n android:resource="@xml/device_filter" />\n </activity>|' "$MANIFEST_PATH" + xmlstarlet ed -L \ + -s '//activity' -t elem -n 'intent-filter' \ + -s '//activity/intent-filter' -t elem -n 'action' \ + -a android:name -v 'android.hardware.usb.action.USB_DEVICE_ATTACHED' \ + -s '//activity' -t elem -n 'meta-data' \ + -a android:name -v 'android.hardware.usb.action.USB_DEVICE_ATTACHED' \ + -a android:resource -v '@xml/device_filter' \ + "$MANIFEST_PATH"
26-28: Fix sed portability for macOS—BSD sed incompatible with current multi-line insertion syntax.The script uses GNU-specific sed syntax at lines 27, 36, and 91 that fails on macOS (BSD sed):
- Line 27: Escaped newlines (
\n) ini\command are treated as literal characters by BSD sed, corrupting the XML- Line 36: The
0address (first occurrence) is GNU-only; BSD sed doesn't support it- Line 91: Escaped newlines in
a\command have the same BSD incompatibilitySince Tauri's Android tooling supports cross-platform development including macOS, this script must work on both GNU and BSD sed variants.
Implement platform detection and use portable syntax:
+# Detect OS and set sed options +if [[ "$OSTYPE" == "darwin"* ]]; then + # BSD sed requires empty string after -i + sed_i=(-i '') +else + sed_i=(-i) +fi + # Check if USB permissions already added if grep -q "android.permission.USB_PERMISSION" "$MANIFEST_PATH"; then echo "USB permissions already present in manifest" else # Add USB permissions before </manifest> - sed -i '/<\/manifest>/i \ <!-- USB permissions for serial communication -->\n <uses-permission android:name="android.permission.USB_PERMISSION" />\n <uses-feature android:name="android.hardware.usb.host" android:required="false" />' "$MANIFEST_PATH" + sed "${sed_i[@]}" '/<\/manifest>/i\ + <!-- USB permissions for serial communication -->\ + <uses-permission android:name="android.permission.USB_PERMISSION" />\ + <uses-feature android:name="android.hardware.usb.host" android:required="false" />' "$MANIFEST_PATH" echo "Added USB permissions to manifest" fiApply the same pattern (actual line continuations instead of
\nescapes, and platform-aware sed invocation) to lines 36 and 91.
🧹 Nitpick comments (1)
scripts/patch-android-manifest.sh (1)
20-38: Consider adding XML validation after manifest modifications.The script creates a backup (good), but does not validate that the modified manifest is still valid XML. If a sed command partially fails or corrupts the file, the error will go undetected until the build step.
Add a post-modification validation step:
# After all manifest modifications: if command -v xmllint &> /dev/null; then if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then echo "Error: Modified manifest is invalid XML. Restoring from backup." cp "$MANIFEST_PATH.bak" "$MANIFEST_PATH" exit 1 fi fiThis provides early detection of corruption and a automatic recovery path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(2 hunks)scripts/patch-android-manifest.sh(1 hunks)src-tauri/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- src-tauri/Cargo.toml
⏰ 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). (2)
- GitHub Check: Tauri Android preview (APK)
- GitHub Check: Tauri Linux preview (deb/appimage)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
157-171: Verified: Fix required to prevent duplicateallprojectsblocks in generatedbuild.gradle.kts.The generated
src-tauri/gen/android/build.gradle.ktswill contain anallprojectsblock (matching the committed./android/build.gradlestructure). The current logic checks only for "jitpack.io" absence and blindly appends a newallprojectsblock, which creates duplicate or malformed Gradle configuration and breaks the build.The fix must detect an existing
allprojectsblock and insert the maven repository into it, rather than appending a new block:- if [ -f "$FILE" ]; then - echo "Ensuring JitPack repository in $FILE..." - if ! grep -q "jitpack.io" "$FILE"; then - echo "Adding JitPack repository..." - printf '\nallprojects {\n repositories {\n maven(url = "https://jitpack.io")\n }\n}\n' >> "$FILE" - fi + if [ -f "$FILE" ]; then + echo "Ensuring JitPack repository in $FILE..." + if ! grep -q "jitpack.io" "$FILE"; then + if grep -q "^allprojects {" "$FILE"; then + echo "allprojects block exists; inserting JitPack into existing block..." + sed -i '/^allprojects {/a\ repositories {\n maven(url = "https://jitpack.io")\n }' "$FILE" + else + echo "Adding new allprojects block with JitPack..." + printf '\nallprojects {\n repositories {\n maven(url = "https://jitpack.io")\n }\n}\n' >> "$FILE" + fi + fiNote: The sed approach has portability concerns; consider awk for better cross-platform compatibility.
🧹 Nitpick comments (1)
scripts/patch-android-manifest.sh (1)
88-99: Excellent use of awk for portable dependency insertion.The shift from sed to awk on lines 91–97 makes this logic portable to macOS and Linux. However, the regex
^dependencies {on line 88 assumes the dependencies block starts at the beginning of the line. In typical Kotlin build files, this block is not indented, so this should work; however, consider usinggrep -E "dependencies\s*\{"(without anchors) for robustness if the build file is reformatted.This is a minor concern that can be addressed in a follow-up if needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)scripts/patch-android-manifest.sh(1 hunks)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
- Created test workflow to verify manifest patching is applied correctly - Updated ANDROID_USB.md with current status and known issues - Added ANDROID_USB_DEBUGGING.md with investigation steps and troubleshooting guide
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test-android-patch.yml(1 hunks)ANDROID_USB.md(1 hunks)ANDROID_USB_DEBUGGING.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
ANDROID_USB_DEBUGGING.md
[style] ~16-~16: ‘FIRST PRIORITY’ might be wordy. Consider a shorter alternative.
Context: ...Steps ### 1. Verify Manifest Patching (FIRST PRIORITY) Run the test workflow: ```bash # Push...
(EN_WORDINESS_PREMIUM_FIRST_PRIORITY)
🪛 markdownlint-cli2 (0.18.1)
ANDROID_USB.md
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
🔇 Additional comments (5)
ANDROID_USB_DEBUGGING.md (1)
1-121: Comprehensive debugging guide looks good overall.The document is well-structured and provides clear investigation steps, code examples, and troubleshooting guidance. A few minor refinements:
Simplify emphasis on line 16 — "FIRST PRIORITY" is slightly wordy. Consider "PRIMARY STEP" or "STEP 1: Verify Manifest Patching" as a cleaner heading style.
Documentation is well-aligned with the workflow — The instructions properly reference the
test-android-patch.ymlworkflow and match the patch script's expected behavior described elsewhere in the PR.ANDROID_USB.md (2)
34-67: Usage section is clear and well-structured.The build workflow instructions are clear and progressively layered (initialize → patch → build). The automated workflow section (lines 56–67) nicely summarizes the steps for users who prefer a combined command.
69-89: Documentation aligns well with companion script and debugging guide.The "What the Script Does" section accurately describes the expected behavior of
scripts/patch-android-manifest.shand clearly maps to the verification steps inANDROID_USB_DEBUGGING.md. The troubleshooting section addresses sequencing pitfalls (patch after init, before build) that are common sources of user error..github/workflows/test-android-patch.yml (2)
1-103: Workflow structure is sound and verified.The Tauri CLI supports the
--ciflag for non-interactive Android initialization, confirming line 45 is correct.The workflow follows good CI/CD practices:
- Proper environment setup sequence (Java → Android SDK → Rust → targets)
- Pre/post-patch verification with grep checks
- Conditional JitPack repository addition (line 85) prevents duplicates
- Artifact upload (7 days) enables inspection of generated files
75-92: I need to check the repository for any gradle template references or check what tauri versions might be using.The review comment appears to be based on outdated Gradle patterns and may be incorrect.
Modern Android projects use dependencyResolutionManagement with repositories in settings.gradle.kts, not allprojects blocks in build.gradle.kts. Tauri-generated build.gradle.kts files include plugins {}, android {}, and configuration blocks following the modern Kotlin DSL pattern.
The workflow code appends a Groovy-style
allprojects { repositories { ... } }block to a Kotlin DSL file. The Kotlin DSL uses build.gradle.kts syntax instead of the older Groovy build.gradle, so this pattern mixing may not work as intended. Additionally, repository configuration in modern Android projects should be in settings.gradle.kts viadependencyResolutionManagement, not in build.gradle.kts viaallprojects.The concern about duplicate
allprojectsblocks is likely unfounded because modern Tauri-generated projects do not use this deprecated pattern.
…ibility The sed -i command was failing silently due to different behavior on macOS vs Linux. Rewrote manifest patching to use awk which is more portable and reliable. This should fix the issue where the USB intent filter wasn't being added, causing the app to not prompt for USB permission when devices are attached.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ANDROID_USB_DEBUGGING.md (3)
21-21: Simplify phrasing in section heading."FIRST PRIORITY" is slightly verbose; consider shortening to "HIGH PRIORITY" or "PRIORITY" for conciseness.
47-47: Add language identifier to fenced code block.The code block at line 47 should specify
bashfor syntax highlighting and proper rendering.-``` +```bash lsusb
96-96: Wrap bare URL in markdown link syntax.Bare URLs should be wrapped in markdown link format for better documentation rendering.
-4. **Review plugin source code** at https://github.com/s00d/tauri-plugin-serialplugin/tree/main/android +4. **Review plugin source code** at [tauri-plugin-serialplugin Android source](https://github.com/s00d/tauri-plugin-serialplugin/tree/main/android)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ANDROID_USB_DEBUGGING.md(1 hunks)scripts/patch-android-manifest.sh(1 hunks)
🧰 Additional context used
🪛 LanguageTool
ANDROID_USB_DEBUGGING.md
[style] ~21-~21: ‘FIRST PRIORITY’ might be wordy. Consider a shorter alternative.
Context: ...Steps ### 1. Verify Manifest Patching (FIRST PRIORITY) Run the test workflow: ```bash # Push...
(EN_WORDINESS_PREMIUM_FIRST_PRIORITY)
🪛 markdownlint-cli2 (0.18.1)
ANDROID_USB_DEBUGGING.md
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
scripts/patch-android-manifest.sh (3)
27-61: ✅ Good use of awk for cross-platform portability.The script correctly uses awk for XML insertions (lines 27–61) instead of the previous sed commands, ensuring the script works on both macOS and Linux. The
foundvariable in the second awk block (line 46) properly limits the intent filter insertion to the first</activity>match.
66-101: Verify decimal VID/PID values match expected USB devices.The device_filter.xml uses decimal format for IDs, which is correct per Android documentation, though IDs are often specified in hex format by manufacturers. The conversions appear reasonable (e.g., 1027 = 0x0403 for FTDI), but verify that the product IDs (e.g., 12886 for "STM32 in HID mode") match your target devices. Users may need to add their specific device VID/PID combinations if they're not in this list.
114-138: Good defensive Gradle dependency insertion logic.The script correctly checks for both the existence of
dependenciesblock and the library presence before inserting. The awk-based insertion (lines 114–120) is portable and the fallback to appending a new block (lines 124–130) handles edge cases gracefully.
| # Using awk for portability across macOS and Linux | ||
| awk ' | ||
| /<\/manifest>/ { | ||
| print " <!-- USB permissions for serial communication -->" | ||
| print " <uses-permission android:name=\"android.permission.USB_PERMISSION\" />" | ||
| print " <uses-feature android:name=\"android.hardware.usb.host\" android:required=\"false\" />" | ||
| } | ||
| { print } | ||
| ' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" | ||
| echo "Added USB permissions to manifest" | ||
| fi | ||
|
|
||
| # Check if USB intent filter already added | ||
| if grep -q "USB_DEVICE_ATTACHED" "$MANIFEST_PATH"; then | ||
| echo "USB intent filter already present in manifest" | ||
| else | ||
| # Add USB device intent filter and metadata before </activity> | ||
| # Using awk for portability across macOS and Linux | ||
| awk ' | ||
| /<\/activity>/ && !found { | ||
| print " <!-- Intent filter for USB device attach -->" | ||
| print " <intent-filter>" | ||
| print " <action android:name=\"android.hardware.usb.action.USB_DEVICE_ATTACHED\" />" | ||
| print " </intent-filter>" | ||
| print "" | ||
| print " <!-- USB device filter metadata -->" | ||
| print " <meta-data" | ||
| print " android:name=\"android.hardware.usb.action.USB_DEVICE_ATTACHED\"" | ||
| print " android:resource=\"@xml/device_filter\" />" | ||
| found=1 | ||
| } | ||
| { print } | ||
| ' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" |
There was a problem hiding this comment.
Add XML validation after manifest modifications.
The script modifies the manifest and creates device_filter.xml but never validates the resulting XML is well-formed. If modifications are incorrect or incomplete, the build will fail later during the Gradle phase. Consider adding a quick validation check after both awk operations:
# After the manifest modifications (around line 61)
if command -v xmllint &> /dev/null; then
xmllint --noout "$MANIFEST_PATH" || { echo "❌ Manifest XML validation failed"; exit 1; }
else
echo "⚠️ Warning: xmllint not found, skipping XML validation"
fiThis addresses a concern flagged in the previous review.
Do you want me to generate the complete updated script with XML validation integrated?
🤖 Prompt for AI Agents
In scripts/patch-android-manifest.sh around lines 27 to 59, the script updates
AndroidManifest.xml but does not validate the resulting XML; add an xmllint
validation step after each awk modification (i.e., immediately after the first
mv that writes MANIFEST_PATH and immediately after the second mv that writes
MANIFEST_PATH) so the script runs `xmllint --noout "$MANIFEST_PATH"` when
xmllint is available, exits with a non-zero status and a clear error message on
validation failure, and prints a warning and continues (or documents skipping)
if xmllint is not installed.
The tauri-plugin-serialplugin doesn't automatically request USB permissions on Android. Created a custom MainActivity that: - Registers broadcast receivers for USB attach/detach events - Automatically requests permission when USB devices are detected - Checks for already connected devices on app launch - Handles permission dialog responses - Includes extensive logging for debugging The patch script now copies this MainActivity into the generated Android project, replacing the default TauriActivity. This should fix the issue where the app didn't prompt for USB permission and couldn't detect any serial ports.
Added comprehensive testing guide including: - How to monitor logcat for USB events - Three test scenarios (pre-connected, hot plug, app selector) - Common issues and fixes - What to look for in logs (good vs bad signs)
The MainActivity.kt was missing the import for TauriActivity from app.tauri package, causing Kotlin compilation errors. Added the required import statement.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
.github/workflows/test-android-patch.yml (2)
58-74: Assert the patch outcome, don’t just printCurrently this step only cats/greps loosely. Add explicit assertions for usb.host feature and the usb-serial dependency so the job fails if patching regresses.
Apply this diff to harden the checks:
- name: Show files after patch run: | echo "=== AndroidManifest.xml AFTER patch ===" cat src-tauri/gen/android/app/src/main/AndroidManifest.xml echo "" echo "=== Checking for USB permissions ===" - grep -i "usb" src-tauri/gen/android/app/src/main/AndroidManifest.xml || echo "No USB permissions found!" + set -e + grep -qi 'android.hardware.usb.host' src-tauri/gen/android/app/src/main/AndroidManifest.xml || (echo "Missing usb.host feature!" && exit 1) + grep -qi 'intent-filter' src-tauri/gen/android/app/src/main/AndroidManifest.xml || (echo "Missing intent-filter!" && exit 1) echo "" echo "=== device_filter.xml ===" cat src-tauri/gen/android/app/src/main/res/xml/device_filter.xml || echo "device_filter.xml not found!" echo "" echo "=== MainActivity.kt ===" cat src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt || echo "MainActivity.kt not found!" echo "" echo "=== app/build.gradle.kts dependencies ===" - grep -A 10 "dependencies {" src-tauri/gen/android/app/build.gradle.kts || echo "No dependencies block found!" + grep -A 10 "dependencies {" src-tauri/gen/android/app/build.gradle.kts || (echo "No dependencies block found!" && exit 1) + grep -qi 'com.github.mik3y:usb-serial-for-android' src-tauri/gen/android/app/build.gradle.kts || (echo "usb-serial dependency missing!" && exit 1)
55-57: Make the patch step fail verbosely on any errorEnable xtrace and pipefail when running the patch script for easier debugging and to avoid silent failures.
- - name: Patch Android manifest for USB support - run: bash scripts/patch-android-manifest.sh + - name: Patch Android manifest for USB support + run: bash -euxo pipefail scripts/patch-android-manifest.shscripts/MainActivity.kt (3)
16-18: Make constants static and explicitPrefer companion object + const for TAG and action string. Minor perf/readability win.
-class MainActivity : TauriActivity() { - private val TAG = "BetaflightUSB" - private val ACTION_USB_PERMISSION = "com.betaflight.configurator.USB_PERMISSION" +class MainActivity : TauriActivity() { + companion object { + private const val TAG = "BetaflightUSB" + private const val ACTION_USB_PERMISSION = "com.betaflight.configurator.USB_PERMISSION" + }
23-40: Drop unnecessary synchronized block in onReceiveonReceive runs on the main thread for this dynamic receiver; no shared mutable state here. The lock adds overhead without benefit.
- ACTION_USB_PERMISSION -> { - synchronized(this) { - val device: UsbDevice? = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { - intent.getParcelableExtra(UsbManager.EXTRA_DEVICE, UsbDevice::class.java) - } else { - @Suppress("DEPRECATION") - intent.getParcelableExtra(UsbManager.EXTRA_DEVICE) - } - - if (intent.getBooleanExtra(UsbManager.EXTRA_PERMISSION_GRANTED, false)) { - device?.let { - Log.d(TAG, "Permission granted for device ${it.deviceName}") - // Device is ready to use - } - } else { - Log.d(TAG, "Permission denied for device ${device?.deviceName}") - } - } - } + ACTION_USB_PERMISSION -> { + val device: UsbDevice? = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + intent.getParcelableExtra(UsbManager.EXTRA_DEVICE, UsbDevice::class.java) + } else { + @Suppress("DEPRECATION") + intent.getParcelableExtra(UsbManager.EXTRA_DEVICE) + } + if (intent.getBooleanExtra(UsbManager.EXTRA_PERMISSION_GRANTED, false)) { + device?.let { Log.d(TAG, "Permission granted for device ${it.deviceName}") } + } else { + Log.d(TAG, "Permission denied for device ${device?.deviceName}") + } + }
100-103: Guard receiver unregister to avoid IllegalArgumentExceptionDefensive guard prevents crashes if lifecycle changes later.
- unregisterReceiver(usbReceiver) + try { + unregisterReceiver(usbReceiver) + } catch (e: IllegalArgumentException) { + Log.w(TAG, "usbReceiver already unregistered", e) + }ANDROID_USB_DEBUGGING.md (2)
56-72: Use headings instead of bold for scenario titlesSatisfy MD036 and improve structure.
-**Test A: Device Already Connected** +#### Test A: Device Already Connected ... -**Test B: Hot Plug** +#### Test B: Hot Plug ... -**Test C: App Selector** +#### Test C: App Selector
89-99: Add languages to fenced code blocksSatisfy MD040 and improve readability.
-``` +```text Bus 001 Device 005: ID 0483:5740 STMicroelectronics Virtual COM Port ^^^^ ^^^^ VID PID-
+text
BetaflightUSB: Checking connected USB devices: 1 found
BetaflightUSB: Device: /dev/bus/usb/001/005, VID: 1155, PID: 22336
BetaflightUSB: Requesting USB permission for /dev/bus/usb/001/005
BetaflightUSB: Permission granted for device /dev/bus/usb/001/005-``` +```text BetaflightUSB: Checking connected USB devices: 0 found # ^ Device not detected by Android at all BetaflightUSB: Permission denied for device /dev/bus/usb/001/005 # ^ User denied permission or dialog didn't work # No logs at all # ^ MainActivity not installed or crashed-
+text
=== MainActivity.kt ===
package com.betaflight.configurator
...Also applies to: 107-113, 115-125, 134-139
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test-android-patch.yml(1 hunks)ANDROID_USB.md(1 hunks)ANDROID_USB_DEBUGGING.md(1 hunks)scripts/MainActivity.kt(1 hunks)scripts/patch-android-manifest.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/patch-android-manifest.sh
- ANDROID_USB.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
ANDROID_USB_DEBUGGING.md
56-56: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
64-64: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
72-72: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
134-134: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Tauri Android preview (APK)
- GitHub Check: Tauri Linux preview (deb/appimage)
🔇 Additional comments (1)
scripts/MainActivity.kt (1)
148-152: Correct PendingIntent flags for S+ USB permission flowUse of FLAG_MUTABLE for S+ is correct because the system fills extras. No changes needed.
If you want to ensure no collisions across simultaneous requests, consider using a unique requestCode:
- 0, + device.deviceId,
| - name: Ensure JitPack repository | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| FILE="src-tauri/gen/android/build.gradle.kts" | ||
| if [ -f "$FILE" ]; then | ||
| echo "=== ROOT BUILD.GRADLE.KTS BEFORE JITPACK ===" | ||
| cat "$FILE" | ||
| echo "" | ||
| echo "Ensuring JitPack repository in $FILE..." | ||
| if ! grep -q "jitpack.io" "$FILE"; then | ||
| echo "Adding JitPack repository..." | ||
| printf '\nallprojects {\n repositories {\n maven(url = "https://jitpack.io")\n }\n}\n' >> "$FILE" | ||
| fi | ||
| echo "" | ||
| echo "=== ROOT BUILD.GRADLE.KTS AFTER JITPACK ===" | ||
| cat "$FILE" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
Fix JitPack repository injection: target settings.gradle.kts and Kotlin DSL syntax
Appending an allprojects block to build.gradle.kts is discouraged in Gradle 8+ and often ignored when repositoriesMode FAIL_ON_PROJECT_REPOS is enabled. Also, maven(url = "...") is not idiomatic Kotlin DSL. Patch settings.gradle.kts under dependencyResolutionManagement instead.
Apply this diff to replace the step and use Kotlin DSL:
- - name: Ensure JitPack repository
- shell: bash
- run: |
- set -euo pipefail
- FILE="src-tauri/gen/android/build.gradle.kts"
- if [ -f "$FILE" ]; then
- echo "=== ROOT BUILD.GRADLE.KTS BEFORE JITPACK ==="
- cat "$FILE"
- echo ""
- echo "Ensuring JitPack repository in $FILE..."
- if ! grep -q "jitpack.io" "$FILE"; then
- echo "Adding JitPack repository..."
- printf '\nallprojects {\n repositories {\n maven(url = "https://jitpack.io")\n }\n}\n' >> "$FILE"
- fi
- echo ""
- echo "=== ROOT BUILD.GRADLE.KTS AFTER JITPACK ==="
- cat "$FILE"
- fi
+ - name: Ensure JitPack repository (settings.gradle.kts)
+ shell: bash
+ run: |
+ set -euo pipefail
+ FILE="src-tauri/gen/android/settings.gradle.kts"
+ if [ -f "$FILE" ]; then
+ echo "=== settings.gradle.kts BEFORE JITPACK ==="
+ cat "$FILE"; echo ""
+ if grep -q 'dependencyResolutionManagement' "$FILE"; then
+ if ! grep -q 'https://jitpack.io' "$FILE"; then
+ echo "Adding JitPack to dependencyResolutionManagement.repositories..."
+ awk '
+ /dependencyResolutionManagement/ {print; inBlock=1; next}
+ inBlock && /repositories\s*\{/ && !added {print; print " maven(\"https://jitpack.io\")"; added=1; next}
+ {print}
+ ' "$FILE" > "$FILE.tmp" && mv "$FILE.tmp" "$FILE"
+ fi
+ else
+ printf '\ndependencyResolutionManagement {\n repositories {\n mavenCentral()\n google()\n maven("https://jitpack.io")\n }\n}\n' >> "$FILE"
+ fi
+ echo "=== settings.gradle.kts AFTER JITPACK ==="
+ cat "$FILE"
+ fiRun to confirm where repositories are sourced:
🏁 Script executed:
#!/bin/bash
rg -n "dependencyResolutionManagement|jitpack|repositoriesMode" -S src-tauri/gen/android 2>/dev/null || trueLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if src-tauri/gen/android directory exists and list contents
if [ -d "src-tauri/gen/android" ]; then
echo "=== src-tauri/gen/android exists ==="
find "src-tauri/gen/android" -type f \( -name "*.gradle*" -o -name "*.kts" \) 2>/dev/null | head -20
else
echo "src-tauri/gen/android does not exist"
fi
# Search for any gradle or kts files in src-tauri
echo ""
echo "=== Searching for gradle/kts files in src-tauri ==="
find src-tauri -type f \( -name "*.gradle.kts" -o -name "*.gradle" \) 2>/dev/null | head -20
# Check if settings.gradle.kts or build.gradle.kts exist elsewhere
echo ""
echo "=== Checking entire repo for build.gradle.kts or settings.gradle.kts ==="
find . -maxdepth 5 -type f \( -name "build.gradle.kts" -o -name "settings.gradle.kts" \) 2>/dev/nullLength of output: 163
🏁 Script executed:
#!/bin/bash
# View the entire test-android-patch.yml workflow to understand context
cat .github/workflows/test-android-patch.ymlLength of output: 3674
🌐 Web query:
Tauri android init gradle structure settings.gradle.kts
💡 Result:
Short answer — what tauri android init produces and what goes in settings.gradle.kts
-
The Android project is generated at src-tauri/gen/android as a Gradle multi-project (root + modules).
settings.gradle.ktslives at that generated android root and declares the included modules and repository resolution rules. [1][2] -
Typical responsibilities of settings.gradle.kts in a Tauri-generated project:
- include the app module (usually ":app") and the Tauri Android library/project (names vary; older templates used
:tauri-android/:tauri). - set dependencyResolutionManagement / repositoriesMode and repositories so subprojects don’t each declare their own repos.
- (optionally) pluginManagement or version catalogs if the template uses them. [1][2]
- include the app module (usually ":app") and the Tauri Android library/project (names vary; older templates used
-
Minimal example (Kotlin DSL) you can expect or copy into the generated android root:
- include(":app")
- include(":tauri-android") // or the module name your template created
- dependencyResolutionManagement {
repositoriesMode.set(RepositoriesMode.FAIL_ON_PROJECT_REPOS)
repositories {
google()
mavenCentral()
}
}
(Adapt module names to whateversrc-tauri/gen/androidcontains.) [1][2]
-
Notes:
- Tauri’s Android templates have been updated to recent Gradle / Android Gradle Plugin versions (Gradle 8.x / AGP 8.x in current templates), so you may need recent Android Studio / JDK versions. Check the Tauri CLI/release notes for the exact template versions you have. [3]
Sources
- Gradle: Organizing multi-project builds / settings.gradle.kts (explains settings file role and dependencyResolutionManagement). [1]
- Community Tauri Android setup guides showing the generated layout and where files live (src-tauri/gen/android, app/build.gradle.kts). [2]
- Tauri CLI / template release notes mentioning Gradle/AGP updates. [3]
🌐 Web query:
Kotlin DSL maven repository syntax gradle
💡 Result:
Short examples for declaring Maven repositories in Gradle Kotlin DSL (build.gradle.kts or settings.gradle.kts):
-
common built-ins
repositories {
mavenCentral()
mavenLocal()
google()
}
[1] -
custom Maven URL (build.gradle.kts)
repositories {
maven {
url = uri("https://my.repo.example/maven")
}
}
[1][2] -
with name + credentials (username/password)
repositories {
maven {
name = "MyRepo"
url = uri("https://my.repo.example/maven")
credentials(PasswordCredentials::class) {
username = findProperty("repoUser") as String?
password = findProperty("repoPassword") as String?
}
}
}
[2] -
token/HTTP-header auth example (GitLab style)
val gitLabToken: String by project
repositories {
maven {
url = uri("https://gitlab.com/api/v4/projects/12345/packages/maven")
name = "GitLab"
credentials(org.gradle.plugin.auth.HttpHeaderCredentials::class) {
name = "Private-Token"
value = gitLabToken
}
authentication { create<org.gradle.plugin.auth.HttpHeaderAuthentication>("header") }
}
}
[3] -
centralizing in settings.gradle.kts (dependencyResolutionManagement)
dependencyResolutionManagement {
repositories {
mavenCentral()
maven { url = uri("https://my.repo.example/maven") }
}
}
[1][3]
References:
- Gradle — Declaring repositories (Kotlin DSL examples). [1]
- MavenArtifactRepository / properties (credentials, url). [2]
- Kotlin DSL examples including GitLab token / StackOverflow translation. [3]
Fix JitPack repository injection: target settings.gradle.kts with correct Kotlin DSL syntax
The original approach appends allprojects to build.gradle.kts using Groovy syntax, which is incompatible with Kotlin DSL (.kts) files and ignored in Gradle 8+ when repositoriesMode.FAIL_ON_PROJECT_REPOS is enabled. The correct target is settings.gradle.kts under dependencyResolutionManagement.
However, the proposed fix also contains incorrect Kotlin DSL syntax. In Kotlin DSL, the correct syntax is maven { url = uri("https://jitpack.io") }, not maven("url").
Replace the step with:
- name: Ensure JitPack repository (settings.gradle.kts)
shell: bash
run: |
set -euo pipefail
FILE="src-tauri/gen/android/settings.gradle.kts"
if [ -f "$FILE" ]; then
echo "=== settings.gradle.kts BEFORE JITPACK ==="
cat "$FILE"; echo ""
if grep -q 'dependencyResolutionManagement' "$FILE"; then
if ! grep -q 'https://jitpack.io' "$FILE"; then
echo "Adding JitPack to dependencyResolutionManagement.repositories..."
sed -i '/repositories\s*{/a\ maven { url = uri("https://jitpack.io") }' "$FILE"
fi
else
printf '\ndependencyResolutionManagement {\n repositories {\n mavenCentral()\n google()\n maven { url = uri("https://jitpack.io") }\n }\n}\n' >> "$FILE"
fi
echo "=== settings.gradle.kts AFTER JITPACK ==="
cat "$FILE"
fi🤖 Prompt for AI Agents
.github/workflows/test-android-patch.yml around lines 75 to 92: the workflow
currently appends a Groovy-style allprojects block into
src-tauri/gen/android/build.gradle.kts which is incompatible with Kotlin DSL and
ignored under Gradle 8+ with repositoriesMode.FAIL_ON_PROJECT_REPOS; instead
modify the step to target settings.gradle.kts and inject a Kotlin DSL
dependencyResolutionManagement block (or merge into existing one) that adds the
JitPack repository using correct Kotlin DSL syntax: repositories { maven { url =
uri("https://jitpack.io") } }; ensure you check for an existing JitPack uri to
avoid duplication and preserve file integrity rather than appending Groovy code
to build.gradle.kts.
ANDROID_USB_DEBUGGING.md
Outdated
| adb install -r gen/android/app/build/outputs/apk/universal/release/app-universal-release-unsigned.apk | ||
| ``` |
There was a problem hiding this comment.
Install the debug variant or use Gradle install task
Unsigned release APK won’t install. Recommend installing debug or using Gradle.
-adb install -r gen/android/app/build/outputs/apk/universal/release/app-universal-release-unsigned.apk
+./gradlew -p gen/android :app:installDebug
+# or:
+# adb install -r gen/android/app/build/outputs/apk/universal/debug/app-universal-debug.apk📝 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.
| adb install -r gen/android/app/build/outputs/apk/universal/release/app-universal-release-unsigned.apk | |
| ``` | |
| ./gradlew -p gen/android :app:installDebug | |
| # or: | |
| # adb install -r gen/android/app/build/outputs/apk/universal/debug/app-universal-debug.apk |
🤖 Prompt for AI Agents
In ANDROID_USB_DEBUGGING.md around lines 35-36, the example shows installing an
unsigned release APK which won’t install on devices; update the instructions to
either use the debug APK path (e.g.,
gen/android/app/build/outputs/apk/debug/app-debug.apk) or recommend running the
Gradle install task (./gradlew installDebug) — alternatively note that a release
APK must be properly signed before installation and show the signed APK path or
signing/signingConfig step.
… DSL
Changed from incorrectly appending Groovy-style allprojects block to
build.gradle.kts (which is ignored in Gradle 8+ with FAIL_ON_PROJECT_REPOS)
to properly injecting JitPack repository into settings.gradle.kts using
correct Kotlin DSL syntax: maven { url = uri("https://jitpack.io") }
This inserts the repository into the existing dependencyResolutionManagement
repositories block, which is the correct location for Gradle 8+.
The CI workflow step was running too early and the awk pattern wasn't matching correctly. Moved the JitPack repository injection into the patch-android-manifest.sh script so it runs at the right time and uses the same proven awk approach as the other patches. This ensures the JitPack repository is added to settings.gradle.kts so that usb-serial-for-android:3.9.0 can be resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/patch-android-manifest.sh (2)
117-149: Gradle dependency insertion handles both existing and missingdependenciesblocks.The script correctly checks for an existing
dependenciesblock (line 122) and inserts the USB library into it using awk. If no block exists, it appends one. However, the grep pattern^dependencies {assumes the block starts at the beginning of a line, which could fail if the build.gradle.kts uses indentation or different formatting.Consider making the pattern more flexible to handle indented
dependenciesblocks:- if grep -q "^dependencies {" "$APP_BUILD_GRADLE"; then + if grep -q "^\s*dependencies\s*{" "$APP_BUILD_GRADLE"; then echo "Inserting into existing dependencies block..." - awk '/^dependencies \{/ { + awk '/^\s*dependencies\s*\{/ { print print " // USB Serial library for Android" print " implementation(\"com.github.mik3y:usb-serial-for-android:3.9.0\")"This addresses potential formatting variations in generated Gradle files.
23-62: Add XML validation after manifest modifications.The script modifies the manifest via awk insertions (lines 27–36 and 46–60) but never validates the resulting XML is well-formed. If awk produces malformed XML due to an unexpected manifest structure, the error propagates silently until the Gradle build fails. This issue was flagged in previous reviews but remains unaddressed.
Add XML validation after both manifest modifications:
# After line 38 (USB permissions insertion): if command -v xmllint &> /dev/null; then if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then echo "❌ Manifest XML validation failed after USB permissions insertion" exit 1 fi fi # After line 62 (intent filter insertion): if command -v xmllint &> /dev/null; then if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then echo "❌ Manifest XML validation failed after intent filter insertion" exit 1 fi fiOr consolidate into a helper function and call it once after all modifications complete.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)scripts/patch-android-manifest.sh(1 hunks)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
🔇 Additional comments (6)
scripts/patch-android-manifest.sh (4)
27-38: ✓ Awk-based USB permissions insertion is platform-portable.The migration from sed to awk for USB permissions insertion addresses the macOS/BSD sed compatibility issues flagged in previous reviews. The conditional check (line 24) prevents duplicate insertions, and the awk pattern correctly targets the closing
</manifest>tag.
40-62: ✓ Awk-based intent filter insertion with!foundguard prevents duplicates.The USB device intent-filter insertion correctly uses the
!foundflag to ensure the block is inserted only once, even if the script runs multiple times. The indentation and escaping are properly handled.
64-102: ✓ Device filter XML is well-formed and comprehensive.The hardcoded USB device list covers common Betaflight-compatible microcontroller families (STM32, CP210x, GD32, AT32, APM32, Raspberry Pi Pico, etc.) and is properly formatted. The use of a heredoc with proper XML escaping is safe.
155-176: ✓ JitPack repository insertion is correctly guarded and portable.The awk implementation for inserting the JitPack maven entry into the existing
repositoriesblock uses the!foundflag to prevent duplicates and is portable across platforms..github/workflows/ci.yml (2)
129-133: ✓ Java version upgrade to 21 is straightforward.The upgrade from Java 17 to 21 in the tauri-android-preview job is a simple version bump with no other changes. Ensure downstream code (Gradle, Tauri, etc.) is compatible with Java 21.
154-155: ✓ Patch step is correctly positioned in the workflow.The new step "Patch Android manifest for USB support" runs at the right point: after "Initialize Tauri Android project" (which generates the required files) and before "Build Tauri Android APK". This ensures the manifest and Gradle configuration are patched before the build consumes them.
…ttings.gradle.kts - Target the correct repositories (dependency resolution, not pluginManagement) - Robust awk block tracking to avoid inserting in wrong block - Append full dependencyResolutionManagement block if missing - Log repository lines for verification
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/patch-android-manifest.sh (2)
155-200: Consider improving robustness of JitPack repository insertion.The awk logic for inserting the JitPack repository (lines 162–176) relies on brace tracking and assumes a specific Gradle file structure (e.g., braces on the same line,
repositories {on its own line). If the Gradle file format varies, the insertion could fail silently or insert in an incorrect location. The fallback logic helps (lines 179–190), but the primary insertion path is brittle.Consider one of the following:
- Add a post-insertion validation step to confirm the repository was added correctly.
- Use a more lenient pattern match that tolerates formatting variations (e.g.,
repositories\s*\{).- Document that settings.gradle.kts must follow a specific format for this insertion to work.
1-22: Add error handling and validation for awk operations.The script uses
set -eto exit on error, but doesn't validate that awk operations succeeded or produced valid output. If awk fails silently or produces a truncated file, the corruption won't be caught. After each awk operation, verify the output exists and is non-empty before moving it:' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" + if [ ! -s "$MANIFEST_PATH" ]; then + echo "Error: Manifest modification resulted in empty file" + cp "$MANIFEST_PATH.bak" "$MANIFEST_PATH" + exit 1 + fiApply this check after each awk-to-tmp-file-and-mv sequence (lines 36, 60, 131, 176).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/patch-android-manifest.sh(1 hunks)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
| # Add USB permissions before </manifest> | ||
| # Using awk for portability across macOS and Linux | ||
| awk ' | ||
| /<\/manifest>/ { | ||
| print " <!-- USB permissions for serial communication -->" | ||
| print " <uses-permission android:name=\"android.permission.USB_PERMISSION\" />" | ||
| print " <uses-feature android:name=\"android.hardware.usb.host\" android:required=\"false\" />" | ||
| } | ||
| { print } | ||
| ' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" | ||
| echo "Added USB permissions to manifest" | ||
| fi | ||
|
|
||
| # Check if USB intent filter already added | ||
| if grep -q "USB_DEVICE_ATTACHED" "$MANIFEST_PATH"; then | ||
| echo "USB intent filter already present in manifest" | ||
| else | ||
| # Add USB device intent filter and metadata before </activity> | ||
| # Using awk for portability across macOS and Linux | ||
| awk ' | ||
| /<\/activity>/ && !found { | ||
| print " <!-- Intent filter for USB device attach -->" | ||
| print " <intent-filter>" | ||
| print " <action android:name=\"android.hardware.usb.action.USB_DEVICE_ATTACHED\" />" | ||
| print " </intent-filter>" | ||
| print "" | ||
| print " <!-- USB device filter metadata -->" | ||
| print " <meta-data" | ||
| print " android:name=\"android.hardware.usb.action.USB_DEVICE_ATTACHED\"" | ||
| print " android:resource=\"@xml/device_filter\" />" | ||
| found=1 | ||
| } | ||
| { print } | ||
| ' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" | ||
| echo "Added USB intent filter to manifest" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
Add XML validation after manifest modifications.
Past reviews flagged the lack of XML validation after manifest changes. The script modifies the manifest with awk operations but never validates the resulting XML is well-formed. If any awk insertion is incorrect or incomplete, the XML could be corrupted, and this error would only surface later during the Gradle build phase.
Add validation after both awk modifications:
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH"
echo "Added USB permissions to manifest"
+
+ # Validate XML is well-formed
+ if command -v xmllint &> /dev/null; then
+ xmllint --noout "$MANIFEST_PATH" || { echo "❌ Manifest XML validation failed"; exit 1; }
+ fi
fiApply the same validation after the intent-filter insertion (around line 62).
I'll verify the review comment by examining the actual file and validating the claims made.
I need to verify the claims made in the review comment by examining the actual file. Let me gather the necessary information.
🏁 Script executed:
#!/bin/bash
# Examine the patch-android-manifest.sh file to validate review comment claims
echo "=== File info ==="
wc -l scripts/patch-android-manifest.sh 2>/dev/null || echo "File not found"
echo -e "\n=== Lines 27-62 (USB permissions and intent filter sections) ==="
sed -n '27,62p' scripts/patch-android-manifest.sh
echo -e "\n=== Lines 155-200 (JitPack and line 197 grep command) ==="
sed -n '155,200p' scripts/patch-android-manifest.sh
echo -e "\n=== Search for xmllint or XML validation ==="
grep -n "xmllint\|xml.*valid" scripts/patch-android-manifest.sh || echo "No XML validation found"
echo -e "\n=== Check for sed usage (to verify sed portability issues claim) ==="
grep -n "sed" scripts/patch-android-manifest.sh || echo "No sed commands found"Length of output: 4213
🏁 Script executed:
#!/bin/bash
# Examine the patch-android-manifest.sh file to validate review comment claims
echo "=== File existence and total lines ==="
if [ -f scripts/patch-android-manifest.sh ]; then
wc -l scripts/patch-android-manifest.sh
else
echo "File not found at scripts/patch-android-manifest.sh"
find . -name "patch-android-manifest.sh" 2>/dev/null
fi
echo -e "\n=== Lines 27-62 (USB permissions and intent filter sections) ==="
sed -n '27,62p' scripts/patch-android-manifest.sh 2>/dev/null
echo -e "\n=== Lines 155-200 (JitPack section with line 197 grep) ==="
sed -n '155,200p' scripts/patch-android-manifest.sh 2>/dev/null
echo -e "\n=== Search for XML validation (xmllint, xml validation) ==="
grep -n "xmllint\|xml.*validat" scripts/patch-android-manifest.sh 2>/dev/null || echo "No xmllint validation found"
echo -e "\n=== Check for sed usage in entire file ==="
grep -n "^[^#]*sed" scripts/patch-android-manifest.sh 2>/dev/null || echo "No sed commands found"
echo -e "\n=== Complete file structure (first 30 lines) ==="
head -30 scripts/patch-android-manifest.sh 2>/dev/nullLength of output: 5384
I need to verify the exact line number of the grep error mentioned in the review.
🏁 Script executed:
#!/bin/bash
# Find exact line number of grep commands, especially the one with -n flag issue
echo "=== All grep commands in the file with line numbers ==="
grep -n "grep" scripts/patch-android-manifest.sh
echo -e "\n=== Line 191 specifically ==="
sed -n '191p' scripts/patch-android-manifest.sh
echo -e "\n=== Lines 189-195 (context around line 191) ==="
sed -n '189,195p' scripts/patch-android-manifest.sh
echo -e "\n=== Search for double -n flags in grep ==="
grep -n -- '-n.*-n' scripts/patch-android-manifest.sh || echo "No double -n found via direct search"
echo -e "\n=== Analyze the grep command syntax ==="
sed -n '191p' scripts/patch-android-manifest.sh | cat -ALength of output: 1253
🏁 Script executed:
#!/bin/bash
# Find exact line number of grep commands and verify the syntax error
echo "=== All grep commands with line numbers ==="
grep -n "grep" scripts/patch-android-manifest.sh
echo -e "\n=== Lines 185-200 to see full grep context ==="
sed -n '185,200p' scripts/patch-android-manifest.sh
echo -e "\n=== Search for patterns with multiple flags ==="
grep -nE '\-n.*\-n|repositories' scripts/patch-android-manifest.sh | tail -5Length of output: 1575
Add XML validation after manifest modifications and fix grep syntax error.
The script modifies the Android manifest with awk but never validates the XML remains well-formed. Corrupted XML will only surface during the Gradle build phase.
Additionally, line 197 has a grep syntax error: the -n flag appears twice, and the pipe characters in the pattern won't work as regex alternation without -E flag.
Line 38 area — Add XML validation after USB permissions insertion:
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH"
echo "Added USB permissions to manifest"
+ if command -v xmllint &> /dev/null; then
+ xmllint --noout "$MANIFEST_PATH" || { echo "❌ Manifest XML validation failed"; exit 1; }
+ fi
fiLine 62 area — Add XML validation after intent-filter insertion:
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH"
echo "Added USB intent filter to manifest"
+ if command -v xmllint &> /dev/null; then
+ xmllint --noout "$MANIFEST_PATH" || { echo "❌ Manifest XML validation failed"; exit 1; }
+ fi
fiLine 197 — Fix grep command (remove duplicate -n, add -E for extended regex):
- grep -n "dependencyResolutionManagement\|repositories \{|jitpack.io" -n "$SETTINGS_GRADLE" || true
+ grep -nE "dependencyResolutionManagement|repositories \{|jitpack.io" "$SETTINGS_GRADLE" || true- Switch dependency back to 3.8.0 (known available on JitPack) - Inject JitPack into both dependencyResolutionManagement and pluginManagement - Add log preview of repositories blocks to verify in CI
Use `grep -En` with extended regex alternation for clearer CI logging:
`dependencyResolutionManagement|pluginManagement|repositories { |jitpack.io`.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/tauri-patch-android.sh (3)
10-10: Remove or document the unused MAINACTIVITY_PATH variable.Line 10 defines
MAINACTIVITY_PATHbut it's never used. Lines 107–109 explicitly skip custom MainActivity implementation. Either remove this variable or add an inline comment explaining its intended future use and why it's currently unused.Apply this diff to document the intent:
-MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt" +# MAINACTIVITY_PATH intentionally unused—custom MainActivity not needed for USB serial permissions. +# (See lines 107–109.) Kept as reference for potential future enhancements.Alternatively, remove the variable entirely and the referencing comment.
29-62: Add XML validation after manifest modifications.The script modifies
AndroidManifest.xmlwith awk but never validates the resulting XML is well-formed. If the awk transformation produces malformed XML, the error will only surface later during the Gradle build phase, making debugging harder.Add validation after both awk modifications:
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" echo "Added USB permissions to manifest" + if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then + echo "❌ Manifest XML validation failed after USB permissions insertion" + mv "$MANIFEST_PATH.bak" "$MANIFEST_PATH" + exit 1 + fi + fi fiApply the same validation after line 60 (intent-filter insertion):
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" echo "Added USB intent filter to manifest" + if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then + echo "❌ Manifest XML validation failed after intent-filter insertion" + mv "$MANIFEST_PATH.bak" "$MANIFEST_PATH" + exit 1 + fi + fi fiThis approach restores the backup if validation fails, ensuring the script fails fast rather than producing a broken build.
67-102: Add optional validation for device_filter.xml creation.The
device_filter.xmlis created fresh via a here-doc, which is reliable. However, adding a quick XML validation check would catch any issues early:cat > "$DEVICE_FILTER_PATH" << 'EOF' <?xml version="1.0" encoding="utf-8"?> <resources> <!-- ... USB device filters ... --> </resources> EOF + +if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$DEVICE_FILTER_PATH" 2>/dev/null; then + echo "❌ Device filter XML validation failed" + exit 1 + fi +fiThis ensures the file is well-formed before the build attempts to consume it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tauri-patch-android.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
🪛 Shellcheck (0.11.0)
scripts/tauri-patch-android.sh
[warning] 10-10: MAINACTIVITY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/tauri-patch-android.sh (3)
189-210: Fix sed portability for pluginManagement patching—fails on macOS.The sed commands on lines 191–208 use GNU sed syntax that fails on macOS (BSD sed). Same issue as lines 157–171.
Apply the same portable awk-based solution here, or simplify by validating that the file-creation path is sufficient.
157-171: Fix sed portability for dependencyResolutionManagement patching—fails on macOS.The sed commands on lines 158–170 use GNU sed syntax (
/a\with backslash continuations) that fails on macOS (BSD sed). While the file-creation path (lines 119–138) is correct, the patching logic will silently fail or corrupt settings.gradle.kts for macOS developers usingscripts/build-android-local.sh.Replace with a portable awk-based approach:
# Check and add missing repositories in dependencyResolutionManagement - if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "google()"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - google() -}' "$SETTINGS_GRADLE" - fi - if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "mavenCentral()"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - mavenCentral() -}' "$SETTINGS_GRADLE" - fi - if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "jitpack.io"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - maven { url = uri("https://jitpack.io") } -}' "$SETTINGS_GRADLE" - fi + # Regenerate the entire file with correct repositories instead of patching + # This ensures portability and correctness across macOS and Linux + awk ' + /dependencyResolutionManagement/ { found_drm=1 } + /pluginManagement/ && found_drm && !done_repos { + # Insert missing repositories before pluginManagement + print "dependencyResolutionManagement {" + print " repositories {" + print " google()" + print " mavenCentral()" + print " maven { url = uri(\"https://jitpack.io\") }" + print " }" + print "}" + print "" + done_repos=1 + } + { print } + ' "$SETTINGS_GRADLE" > "$SETTINGS_GRADLE.tmp" && mv "$SETTINGS_GRADLE.tmp" "$SETTINGS_GRADLE"Alternatively, validate that the generated file in lines 119–138 is always correct and skip the entire patching section (lines 156–172) with a warning if the file already exists.
221-225: Fix sed portability for dependency injection—fails on macOS.The sed command on lines 223–225 uses GNU sed syntax (
/a\continuation) that fails on macOS (BSD sed). Use the portable awk approach or simple text insertion:- if grep -q "^dependencies {" "$APP_BUILD_GRADLE"; then - # Insert after the opening dependencies { line - sed -i '/^dependencies {/a\ - implementation("com.github.mik3y:usb-serial-for-android:3.8.0") -' "$APP_BUILD_GRADLE" + if grep -q "^dependencies {" "$APP_BUILD_GRADLE"; then + # Insert after the opening dependencies { line using portable awk + awk ' + /^dependencies \{/ && !added { + print $0 + print " implementation(\"com.github.mik3y:usb-serial-for-android:3.8.0\")" + added=1 + next + } + { print } + ' "$APP_BUILD_GRADLE" > "$APP_BUILD_GRADLE.tmp" && mv "$APP_BUILD_GRADLE.tmp" "$APP_BUILD_GRADLE"
🧹 Nitpick comments (4)
scripts/tauri-patch-android.sh (4)
10-10: Remove or document unused variable MAINACTIVITY_PATH.This variable is defined but never referenced. Either remove it or add a comment explaining its intended purpose (as noted in lines 107–109 where custom MainActivity is skipped).
-MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt" +# MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt" +# Custom MainActivity implementation skipped; USB permissions handled by serial plugin (see line 108)
29-36: Good—portable awk-based manifest patching with idempotency.The awk-based approach is cross-platform compatible and correctly handles atomic file replacement. However, consider adding XML validation after modification to catch corruption early.
Add XML validation after line 37:
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" echo "Added USB permissions to manifest" + if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then + echo "❌ Manifest XML validation failed after patching" + exit 1 + fi + fi fi
46-60: Good—portable awk-based intent filter insertion.The awk implementation is cross-platform compatible with proper idempotency. Add XML validation here too (after line 61) to ensure manifest integrity.
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" echo "Added USB intent filter to manifest" + if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then + echo "❌ Manifest XML validation failed after intent filter insertion" + exit 1 + fi + fi fi
67-102: Add validation for device_filter.xml.The here-document is well-formed, but add a post-write XML validation check to catch any file system issues or truncation.
<usb-device vendor-id="11914" product-id="15" /> <!-- Raspberry Pi Pico in Bootloader mode --> </resources> EOF + +# Validate device_filter.xml is well-formed +if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$DEVICE_FILTER_PATH" 2>/dev/null; then + echo "❌ Device filter XML validation failed" + exit 1 + fi +fi echo "✓ Android manifest patched successfully!"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
scripts/tauri-patch-android.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
scripts/tauri-patch-android.sh
[warning] 10-10: MAINACTIVITY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/tauri-patch-android.sh (2)
217-225: Fix sed portability for app/build.gradle.kts dependency injection—will fail on macOS.Lines 223–225 use the same GNU sed syntax that fails on macOS. Replace with awk for portability:
if grep -q "^dependencies {" "$APP_BUILD_GRADLE"; then # Insert after the opening dependencies { line - sed -i '/^dependencies {/a\ - implementation("com.github.mik3y:usb-serial-for-android:3.9.0") -' "$APP_BUILD_GRADLE" + # Use awk for cross-platform compatibility + awk ' + /^dependencies \{/ && !added { + print $0 + print " implementation(\"com.github.mik3y:usb-serial-for-android:3.9.0\")" + added=1 + next + } + { print } + ' "$APP_BUILD_GRADLE" > "$APP_BUILD_GRADLE.tmp" && mv "$APP_BUILD_GRADLE.tmp" "$APP_BUILD_GRADLE" else
157-172: Fix sed portability for settings.gradle.kts—will fail on macOS.Lines 158–172 use GNU sed syntax (
sed -iwith backslash-escaped newlines) that fails on macOS (which uses BSD sed). These commands will silently fail or corrupt the Gradle file for macOS developers usingscripts/build-android-local.sh.Replace the complex sed logic with the same awk-based approach used for manifest modifications (lines 29–36, 46–60), or use sed with a backup suffix that works across platforms:
- if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "google()"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - google() -}' "$SETTINGS_GRADLE" + if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "google()"; then + # Use awk for cross-platform compatibility (macOS + Linux) + awk ' + /dependencyResolutionManagement/ { in_drm=1 } + in_drm && /repositories \{/ { print; print " google()"; next } + { print } + ' "$SETTINGS_GRADLE" > "$SETTINGS_GRADLE.tmp" && mv "$SETTINGS_GRADLE.tmp" "$SETTINGS_GRADLE" fiApply the same fix to lines 162–165, 167–170 (remaining repositories in dependencyResolutionManagement), and repeat for pluginManagement block (lines 190–209).
🧹 Nitpick comments (1)
scripts/tauri-patch-android.sh (1)
10-10: Remove or document the unusedMAINACTIVITY_PATHvariable.This variable is defined but never used in the script. While lines 107–109 explain why custom MainActivity implementation is skipped, the variable declaration itself adds unnecessary clutter.
Either remove line 10 entirely or replace it with a clarifying comment.
-MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt" +# Custom MainActivity not needed; USB permissions handled by the serial plugin and manifest intent filters
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tauri-patch-android.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
🪛 Shellcheck (0.11.0)
scripts/tauri-patch-android.sh
[warning] 10-10: MAINACTIVITY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
🔇 Additional comments (2)
scripts/tauri-patch-android.sh (2)
27-38: Add XML validation after manifest modifications.The manifest is modified via awk but never validated for well-formedness. If the awk insertion produces invalid XML, the error surfaces only during the Gradle build phase. Add validation immediately after line 36 to fail fast.
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" echo "Added USB permissions to manifest" + + # Validate manifest XML is well-formed + if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then + echo "❌ Manifest XML validation failed after USB permissions insertion" + exit 1 + fi + fi fiApply the same validation pattern after the intent-filter insertion at line 62.
64-102: Add XML validation for created device_filter.xml.The device_filter.xml is written fresh from a heredoc and never validated. While lower risk than manifest modification, validation should be added to catch any templating errors.
cat > "$DEVICE_FILTER_PATH" << 'EOF' <?xml version="1.0" encoding="utf-8"?> <resources> ...usb-device entries... </resources> EOF + +# Validate device filter XML is well-formed +if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$DEVICE_FILTER_PATH" 2>/dev/null; then + echo "❌ Device filter XML validation failed" + exit 1 + fi +fi
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/tauri-patch-android.sh (2)
223-225: Fix sed portability for app/build.gradle.kts dependency—fails on macOS.Lines 223–225 use GNU sed syntax that fails on macOS (BSD sed). Replace with the awk-based approach for consistency:
- sed -i '/^dependencies {/a\ - implementation("com.github.mik3y:usb-serial-for-android:3.8.1") -' "$APP_BUILD_GRADLE" + # Use awk for cross-platform compatibility + awk ' + /^dependencies \{/ && !added { + print $0 + print " implementation(\"com.github.mik3y:usb-serial-for-android:3.8.1\")" + added = 1 + next + } + { print } + ' "$APP_BUILD_GRADLE" > "$APP_BUILD_GRADLE.tmp" && mv "$APP_BUILD_GRADLE.tmp" "$APP_BUILD_GRADLE"This ensures the dependency is added correctly on both macOS and Linux.
158-171: Fix sed portability for dependencyResolutionManagement repositories—fails on macOS.The sed commands on lines 158–171 use GNU sed syntax (
sed -iwith/a\multi-line insertions) that fails silently on macOS (BSD sed). The repositories won't be added, leaving the Gradle configuration incomplete.Since the awk-based approach on lines 29–36 works across platforms, apply the same pattern here. Replace the sed blocks with a portable solution:
- if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "google()"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - google() -}' "$SETTINGS_GRADLE" - fi - if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "mavenCentral()"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - mavenCentral() -}' "$SETTINGS_GRADLE" - fi - if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "jitpack.io"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - maven { url = uri("https://jitpack.io") } -}' "$SETTINGS_GRADLE" - fi + # Use awk for cross-platform compatibility (macOS + Linux) + if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "google()"; then + awk ' + /dependencyResolutionManagement \{/,/^\}/ { + if (/repositories \{/ && !added_google) { + print $0 + print " google()" + added_google = 1 + next + } + } + { print } + ' "$SETTINGS_GRADLE" > "$SETTINGS_GRADLE.tmp" && mv "$SETTINGS_GRADLE.tmp" "$SETTINGS_GRADLE" + fi + # Apply similar awk patterns for mavenCentral() and jitpack.io...This ensures repositories are added correctly on both macOS and Linux.
🧹 Nitpick comments (3)
scripts/tauri-patch-android.sh (3)
10-10: Remove or document unused variable MAINACTIVITY_PATH.This variable is defined but never used. The comment at lines 107–109 explains custom MainActivity is skipped, so either remove this line or add an inline comment referencing the skip logic for future readers.
-MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt" +# MAINACTIVITY_PATH unused: custom MainActivity is not needed for USB serial permissions (see lines 107–109)
36-36: Add XML validation after manifest modifications.The manifest is modified via awk insertions, but never validated to ensure well-formedness. If the awk operations produce invalid XML, the build fails later during Gradle compilation with a confusing error message.
Add validation after each manifest modification:
' "$MANIFEST_PATH" > "$MANIFEST_PATH.tmp" && mv "$MANIFEST_PATH.tmp" "$MANIFEST_PATH" echo "Added USB permissions to manifest" + if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$MANIFEST_PATH" 2>/dev/null; then + echo "❌ Manifest XML validation failed after USB permissions insertion" + mv "$MANIFEST_PATH.bak" "$MANIFEST_PATH" + exit 1 + fi + fi fiApply the same validation block after line 60 (intent filter insertion).
Also applies to: 60-60
102-102: Validate device_filter.xml after creation.The device_filter.xml is written but never validated. If the XML is malformed, the Gradle build fails with an unclear error.
Add validation immediately after the here-doc closes (after line 102):
EOF + +if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$DEVICE_FILTER_PATH" 2>/dev/null; then + echo "❌ Device filter XML validation failed" + exit 1 + fi +fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tauri-patch-android.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
🪛 Shellcheck (0.11.0)
scripts/tauri-patch-android.sh
[warning] 10-10: MAINACTIVITY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Tauri Android preview (APK)
- GitHub Check: Tauri Linux preview (deb/appimage)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/tauri-patch-android.sh (2)
158-170: Replace sed with awk for cross-platform portability (dependencyResolutionManagement repositories).Lines 158–170 use GNU sed syntax (
/a\with line continuation) that fails on macOS/BSD sed. The script already uses the correct awk-based approach for pluginManagement repositories (lines 191–244)—apply the same pattern here for consistency and macOS compatibility.# Check and add missing repositories in dependencyResolutionManagement if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "google()"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - google() -}' "$SETTINGS_GRADLE" + # Using awk for portability across macOS and Linux + awk ' + /dependencyResolutionManagement \{/,/\}/ { + if ($0 ~ /repositories \{/ && !found) { + print $0 + print " google()" + found=1 + next + } + } + { print } + ' "$SETTINGS_GRADLE" > "$SETTINGS_GRADLE.tmp" && mv "$SETTINGS_GRADLE.tmp" "$SETTINGS_GRADLE" fi if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "mavenCentral()"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - mavenCentral() -}' "$SETTINGS_GRADLE" + # Using awk for portability across macOS and Linux + awk ' + /dependencyResolutionManagement \{/,/\}/ { + if ($0 ~ /repositories \{/ && !found) { + print $0 + print " mavenCentral()" + found=1 + next + } + } + { print } + ' "$SETTINGS_GRADLE" > "$SETTINGS_GRADLE.tmp" && mv "$SETTINGS_GRADLE.tmp" "$SETTINGS_GRADLE" fi if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "jitpack.io"; then - sed -i '/dependencyResolutionManagement {/,/}/ { /repositories {/a\ - maven { url = uri("https://jitpack.io") } -}' "$SETTINGS_GRADLE" + # Using awk for portability across macOS and Linux + awk ' + /dependencyResolutionManagement \{/,/\}/ { + if ($0 ~ /repositories \{/ && !found) { + print $0 + print " maven { url = uri(\"https://jitpack.io\") }" + found=1 + next + } + } + { print } + ' "$SETTINGS_GRADLE" > "$SETTINGS_GRADLE.tmp" && mv "$SETTINGS_GRADLE.tmp" "$SETTINGS_GRADLE" fi
259-261: Replace sed with awk for cross-platform portability (app/build.gradle.kts dependency injection).Line 259–261 uses GNU sed syntax that fails on macOS/BSD sed. Replace with awk for portability, matching the pattern already used elsewhere in this script.
if grep -q "^dependencies {" "$APP_BUILD_GRADLE"; then # Insert after the opening dependencies { line - sed -i '/^dependencies {/a\ - implementation("com.github.mik3y:usb-serial-for-android:3.8.1") -' "$APP_BUILD_GRADLE" + # Using awk for portability across macOS and Linux + awk ' + /^dependencies \{/ && !found { + print $0 + print " implementation(\"com.github.mik3y:usb-serial-for-android:3.8.1\")" + found=1 + next + } + { print } + ' "$APP_BUILD_GRADLE" > "$APP_BUILD_GRADLE.tmp" && mv "$APP_BUILD_GRADLE.tmp" "$APP_BUILD_GRADLE" else
🧹 Nitpick comments (2)
scripts/tauri-patch-android.sh (2)
10-10: Remove or document unused variable MAINACTIVITY_PATH.This variable is defined but never used; lines 107–109 explicitly skip custom MainActivity implementation. Either remove the unused variable or add a comment explaining why it's defined and not used, referencing the skip logic for future maintainers.
-MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt"Alternatively, add a comment if future custom MainActivity support is planned:
-MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt" +# MAINACTIVITY_PATH="src-tauri/gen/android/app/src/main/java/com/betaflight/configurator/MainActivity.kt" +# Currently unused; custom MainActivity not needed since USB permissions are handled by the serial plugin. +# Defined for future reference if custom activity implementation becomes necessary.
67-102: Add XML validation for device_filter.xml after creation.The device_filter.xml is generated fresh but never validated. While the static XML is unlikely to be malformed, adding a quick validation step follows defensive practices and catches issues before the build consumes the file.
cat > "$DEVICE_FILTER_PATH" << 'EOF' <?xml version="1.0" encoding="utf-8"?> <resources> <!-- USB device filters for Betaflight-compatible devices --> ... </resources> EOF + +# Validate device filter XML is well-formed +if command -v xmllint &> /dev/null; then + if ! xmllint --noout "$DEVICE_FILTER_PATH" 2>/dev/null; then + echo "❌ Device filter XML validation failed" + exit 1 + fi +fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tauri-patch-android.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
🪛 Shellcheck (0.11.0)
scripts/tauri-patch-android.sh
[warning] 10-10: MAINACTIVITY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
…e.kts instead of relying on the project-level settings.gradle.kts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/create-device-filter.sh (1)
12-47: Clean and straightforward script; consider optional XML validation.The device filter generation is well-organized with clear comments for each device category. The XML structure looks correct.
For robustness (optional), you could add a post-generation XML validation step:
# After the cat > command closes (after line 47) if command -v xmllint &> /dev/null; then xmllint --noout "$DEVICE_FILTER_PATH" || { echo "❌ Device filter XML validation failed"; exit 1; } else echo "⚠️ Tip: Install xmllint for XML validation" fiscripts/patch-android-manifest.sh (1)
20-35: Solid portability fix using awk for manifest permissions.Good use of awk to insert USB permissions in a portable way across macOS and Linux. The backup creation (line 18) and duplicate-prevention checks (lines 21, 38) are appropriate.
For enhanced robustness (optional), you could add XML validation after each modification:
# After line 34 (after USB permissions insertion) if command -v xmllint &> /dev/null; then xmllint --noout "$MANIFEST_PATH" || { echo "❌ Manifest validation failed"; exit 1; } fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
scripts/README.md(1 hunks)scripts/create-device-filter.sh(1 hunks)scripts/patch-android-manifest.sh(1 hunks)scripts/patch-app-gradle.sh(1 hunks)scripts/patch-gradle-settings.sh(1 hunks)scripts/tauri-patch-android.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
🔇 Additional comments (1)
scripts/tauri-patch-android.sh (1)
1-39: Clean orchestration script; depends on fixing sed issues in called scripts.The main script is well-structured and calls the patching scripts in logical order. The manifest existence check (line 12) and the explicit skip of custom MainActivity (lines 33–35) are appropriate.
However, this script will fail on macOS if the called scripts (
patch-gradle-settings.shandpatch-app-gradle.sh) still contain sed portability issues. Once those scripts are fixed, this orchestration will work correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/patch-gradle-settings.sh (1)
47-49: Remove stray statements causing syntax error.Lines 47–49 contain floating Kotlin statements (
google(),mavenCentral(), etc.) with no context inside the bash script. These are not part of any bash construct and cause the script to fail parsing. Remove them entirely.else - google() - mavenCentral() - maven { url = uri("https://jitpack.io") } # Portable awk-based insertion for missing repositories in dependencyResolutionManagement if ! grep -A 10 "dependencyResolutionManagement" "$SETTINGS_GRADLE" | grep -q "google()"; then
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/patch-app-gradle.sh(1 hunks)scripts/patch-gradle-settings.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
🪛 Shellcheck (0.11.0)
scripts/patch-app-gradle.sh
[error] 54-54: Couldn't parse this function. Fix to allow more checks.
(SC1073)
[error] 54-54: Trying to declare parameters? Don't. Use () and refer to params as $1, $2..
(SC1065)
[error] 55-55: Expected a { to open the function definition.
(SC1064)
[error] 55-55: Fix any mentioned problems and try again.
(SC1072)
scripts/patch-gradle-settings.sh
[error] 47-47: Couldn't parse this function. Fix to allow more checks.
(SC1073)
[error] 48-48: Expected a { to open the function definition.
(SC1064)
[error] 48-48: Fix any mentioned problems and try again.
(SC1072)
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
@coderabbitai wake up after build completes :) |
|
Tip For best results, initiate chat on the files or code changes. @haslinghuis, I appreciate the heads up! 😊 Unfortunately, I can't automatically monitor build statuses or wake myself up when builds complete. I only respond when you explicitly mention me or when new commits are pushed to the PR. Here are your options:
The rate limit window should have passed by now, so I'm ready to review whenever you'd like! Just give me the word. 👍 |
haslinghuis
force-pushed
the
tauri-test
branch
from
745a9c1 to
4dfd15e
Compare
|
Backtracking on AI suggestions |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/protocols/TauriSerial.js (2)
213-252: Critical: Missing method and property mismatch in device loading.Two critical issues prevent this code from working:
Missing method: Line 237 calls
this._parseAndroidPortsResponse(newPorts)but this method is not defined anywhere in the file.Property mismatch: Line 246 calls
this.createPort(port)whereportcomes fromallPorts(which hasvendorId/productIdproperties from_convertPortsMapToArray), butcreatePortexpectsusbVendorId/usbProductIdproperties (lines 214-216). This will result in undefined values.Apply this diff to fix the property mismatch:
createPort(port) { - const displayName = vendorIdNames[port.usbVendorId] - ? vendorIdNames[port.usbVendorId] - : `VID:${port.usbVendorId} PID:${port.usbProductId}`; + const displayName = vendorIdNames[port.vendorId] + ? vendorIdNames[port.vendorId] + : `VID:${port.vendorId} PID:${port.productId}`; return { path: "tauriserial", displayName: `Betaflight ${displayName}`, - vendorId: port.usbVendorId, - productId: port.usbProductId, + vendorId: port.vendorId, + productId: port.productId, port: port, }; }And add the missing
_parseAndroidPortsResponsemethod or remove the Android string handling if not needed:_parseAndroidPortsResponse(stringResponse) { // TODO: Implement Android ports string parsing // Expected format: "{/dev/bus/usb/002/002={type=USB, vid=1155, ...}}" // For now, return empty object to prevent crash console.warn(`${logHead} Android string parsing not yet implemented`); return {}; }
266-322: Critical: Orphaned catch block causes syntax error.Lines 316-322 contain a
catchblock that is not attached to anytryblock, resulting in invalid JavaScript syntax. Thecatchblock appears to be leftover from refactoring.Apply this diff to fix the syntax error:
this.dispatchEvent(new CustomEvent("connect", { detail: true })); console.log(`${logHead} Connected to ${path}`); return true; + } catch (error) { + console.error(`${logHead} Error connecting:`, error); + this.openRequested = false; + this.dispatchEvent(new CustomEvent("connect", { detail: false })); + return false; } - catch(error) { - console.error(`${logHead} Error connecting:`, error); - - this.openRequested = false; - this.dispatchEvent(new CustomEvent("connect", { detail: false })); - return false; - }The correct structure should wrap the entire connect logic in a single try-catch block.
🧹 Nitpick comments (3)
src/js/protocols/TauriSerial.js (3)
51-52: Remove or restore commented device monitoring code.The commented-out call to
startDeviceMonitoring()should either be removed if device monitoring is no longer needed, or uncommented with a clear explanation of why it was disabled.Apply this diff if device monitoring is intentionally disabled:
- // this.loadDevices().then(() => this.startDeviceMonitoring()); this.loadDevices();Or uncomment if monitoring should be active:
- // this.loadDevices().then(() => this.startDeviceMonitoring()); - this.loadDevices(); + this.loadDevices().then(() => this.startDeviceMonitoring());
148-171: Device filtering correctly implemented.The filter logic properly restricts devices to known VID/PID combinations. The DEBUG flag approach is acceptable, though consider using an environment variable or configuration setting for runtime control.
Optional: Use an environment-based debug flag for runtime control:
- // Set to true to enable debug logs - const DEBUG = false; + const DEBUG = import.meta.env.DEV || false;
300-300: Remove TODO-style comment.The comment "should we add disconnect handler here ?" should be removed or converted to a proper TODO if action is needed.
this.addEventListener("receive", this.handleReceiveBytes); - // should we add disconnect handler here ? this.addEventListener("disconnect", this.handleDisconnect);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src-tauri/Cargo.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
ANDROID.md(0 hunks)package.json(2 hunks)src-tauri/Cargo.toml(1 hunks)src-tauri/capabilities/default.json(1 hunks)src-tauri/src/lib.rs(1 hunks)src-tauri/src/main.rs(1 hunks)src/js/protocols/TauriSerial.js(9 hunks)src/js/protocols/TauriSerial.js.bck(1 hunks)src/js/serial.js(3 hunks)
💤 Files with no reviewable changes (1)
- ANDROID.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/capabilities/default.json
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4576
File: src/js/port_usage.js:17-23
Timestamp: 2025-08-22T16:43:20.901Z
Learning: In betaflight-configurator, the serial facade architecture requires accessing metrics like bitrate, bytesReceived, and bytesSent from serial._protocol rather than the top-level serial object. This change maintains compatibility with existing port utilization calculations that have been stable for over 11 years.
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-05-14T21:51:09.253Z
Learning: On Android Chrome browsers, Web Bluetooth API is supported for BLE devices and Web USB API is supported, but Web Serial API is not supported (except limited Bluetooth serial support added in 2025). The Betaflight Configurator should detect and use available APIs on Android rather than requiring all three.
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:32:21.385Z
Learning: In the betaflight-configurator codebase, port paths use counter prefixes (e.g., "bluetooth1", "bluetooth2", "serial1") rather than direct protocol identifiers. The protocol selection logic correctly uses `portPath.startsWith("bluetooth")` to detect bluetooth ports regardless of the counter suffix, rather than direct string matching against protocol map keys.
📚 Learning: 2025-10-25T21:16:32.474Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
Applied to files:
src/js/serial.jssrc/js/protocols/TauriSerial.js.bcksrc/js/protocols/TauriSerial.js
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
Applied to files:
src/js/serial.jssrc/js/protocols/TauriSerial.js.bcksrc/js/protocols/TauriSerial.js
📚 Learning: 2025-08-22T16:43:20.901Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4576
File: src/js/port_usage.js:17-23
Timestamp: 2025-08-22T16:43:20.901Z
Learning: In betaflight-configurator, the serial facade architecture requires accessing metrics like bitrate, bytesReceived, and bytesSent from serial._protocol rather than the top-level serial object. This change maintains compatibility with existing port utilization calculations that have been stable for over 11 years.
Applied to files:
src/js/serial.jssrc/js/protocols/TauriSerial.js.bck
📚 Learning: 2025-05-14T21:51:09.253Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-05-14T21:51:09.253Z
Learning: On Android Chrome browsers, Web Bluetooth API is supported for BLE devices and Web USB API is supported, but Web Serial API is not supported (except limited Bluetooth serial support added in 2025). The Betaflight Configurator should detect and use available APIs on Android rather than requiring all three.
Applied to files:
src/js/serial.jssrc/js/protocols/TauriSerial.js
📚 Learning: 2025-06-09T00:32:21.385Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:32:21.385Z
Learning: In the betaflight-configurator codebase, port paths use counter prefixes (e.g., "bluetooth1", "bluetooth2", "serial1") rather than direct protocol identifiers. The protocol selection logic correctly uses `portPath.startsWith("bluetooth")` to detect bluetooth ports regardless of the counter suffix, rather than direct string matching against protocol map keys.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-09T00:33:22.959Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:33:22.959Z
Learning: In the betaflight-configurator codebase, port paths use counter suffixes for serial and bluetooth ports (e.g., "serial1", "serial2", "bluetooth1", "bluetooth2") instead of using connectionId, which means simple string matching against protocol map keys won't work for protocol selection.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/protocols/TauriSerial.js
🧬 Code graph analysis (1)
src-tauri/src/main.rs (1)
src-tauri/src/lib.rs (1)
run(2-8)
⏰ 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). (2)
- GitHub Check: Tauri Android preview (APK)
- GitHub Check: Tauri Linux preview (deb/appimage)
🔇 Additional comments (8)
src/js/serial.js (3)
5-5: LGTM: Clean conditional protocol initialization.The conditional initialization correctly separates Tauri and web environments, instantiating only the necessary protocols for each platform.
Also applies to: 22-31
33-34: LGTM: Event forwarding properly initialized.The event forwarding setup is correctly placed after protocol initialization, ensuring events are properly forwarded in both Tauri and web environments.
48-48: LGTM: Explicit parameter improves clarity.Removing the default parameter makes the API more explicit and aligns with the single call site at line 44.
src/js/protocols/TauriSerial.js (5)
59-69: LGTM: Disconnect handling and port accessor improved.The
handleDisconnectmethod properly handles unexpected disconnections, and returningthis.portfromgetConnectedPort()provides more useful information than just the connectionId.
173-211: LGTM: Device change detection implemented correctly.The device monitoring logic properly detects added and removed devices, emits appropriate events, and updates the ports list.
360-437: LGTM: Send and disconnect methods properly implemented.The
send()method correctly converts data to Uint8Array and useswriteBinary(), whiledisconnect()properly cleans up resources withcancelListen()andclose().
439-442: LGTM: Public device retrieval properly delegates to loadDevices.The method correctly maintains the public API by delegating to
loadDevices()and returning the ports list.
129-142: Original review comment is incorrect - stub implementation is intentional.The
requestPermissionDevice()method was deliberately simplified from a previous Tauri invoke pattern. The backup file shows it previously invoked the serial plugin, but this was removed because Tauri + Android native layer automatically handle device discovery and permissions without requiring JS-level invocation. The Android native code (MainActivity.kt) manages USB permissions at the native layer throughusbManager.requestPermission(), so the JS stub correctly returnstrueto indicate no error. This architecture delegates permission handling to the platform layer, bypassing the browser permission model entirely.
| if (isBrokenPipeError(msg)) { | ||
| console.error(`${logHead} Fatal poll error (broken pipe) on ${this.connectionId}:`, error); | ||
| throw error; | ||
| } | ||
| console.warn(`${logHead} Poll error:`, error); | ||
| await new Promise((resolve) => setTimeout(resolve, 5)); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`${logHead} Error in read loop:`, error); | ||
| this.handleFatalSerialError(error); | ||
| } finally { | ||
| console.log(`${logHead} Polling stopped for ${this.connectionId || "<no-port>"}`); | ||
| } | ||
| } | ||
|
|
||
| async send(data, callback) { | ||
| if (!this.connected) { | ||
| console.error(`${logHead} Cannot send: port not connected`); | ||
| const res = { bytesSent: 0 }; | ||
| callback?.(res); | ||
| return res; | ||
| } | ||
|
|
||
| try { | ||
| // Convert data to Uint8Array | ||
| let dataArray; | ||
| if (data instanceof ArrayBuffer) { | ||
| dataArray = new Uint8Array(data); | ||
| } else if (data instanceof Uint8Array) { | ||
| dataArray = data; | ||
| } else if (Array.isArray(data)) { | ||
| dataArray = new Uint8Array(data); | ||
| } else { | ||
| console.error(`${logHead} Unsupported data type:`, data?.constructor?.name); | ||
| const res = { bytesSent: 0 }; | ||
| callback?.(res); | ||
| return res; | ||
| } | ||
|
|
||
| this.transmitting = true; | ||
|
|
||
| const writeChunk = async (chunk) => { | ||
| await invoke("plugin:serialplugin|write_binary", { | ||
| path: this.connectionId, | ||
| value: Array.from(chunk), | ||
| }); | ||
| }; | ||
|
|
||
| if (this.isNeedBatchWrite) { | ||
| // Batch write for macOS AT32 compatibility | ||
| const batchSize = 63; | ||
| for (let offset = 0; offset < dataArray.length; offset += batchSize) { | ||
| const chunk = dataArray.slice(offset, offset + batchSize); | ||
| await writeChunk(chunk); | ||
| } | ||
| } else { | ||
| await writeChunk(dataArray); | ||
| } | ||
|
|
||
| this.transmitting = false; | ||
| this.bytesSent += dataArray.length; | ||
|
|
||
| const res = { bytesSent: dataArray.length }; | ||
| callback?.(res); | ||
| return res; | ||
| } catch (error) { | ||
| console.error(`${logHead} Error sending data:`, error); | ||
| this.transmitting = false; | ||
| if (isBrokenPipeError(error)) { | ||
| // Treat as device removal to trigger reconnect flow | ||
| this.handleFatalSerialError(error); | ||
| } | ||
| const res = { bytesSent: 0 }; |
There was a problem hiding this comment.
Import/define isBrokenPipeError before use.
readLoop() and send() call isBrokenPipeError(...), but this module never defines or imports that helper. As soon as any error path runs you’ll hit ReferenceError: isBrokenPipeError is not defined, which tears down the serial session. Please wire in the helper (import the existing utility or implement it locally) before invoking it.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/js/protocols/TauriSerial.js (3)
266-322: Critical: Syntax error - malformed try-catch block.The
catchblock at line 316 is separated from thetryblock that opens at line 279. JavaScript requirescatchto immediately follow thetryblock, but lines 288-315 contain code outside both blocks, creating invalid syntax.Apply this diff to fix the structure:
const port = { path: /dev/, baudRate: options.baudRate || 115200, }; - try { - console.log(`${logHead} Connecting to ${path} with options:`, port); - this.port = new SerialPort(port); - const openResult = await this.port.open(); - console.log(`${logHead} Port opened successfully!`, openResult); - } catch (error) { - console.error(`${logHead} Error connecting:`, error); - } - - // Connection successful - this.connected = true; - this.connectionId = path; - this.bitrate = port.baudRate; - this.openRequested = false; - - this.connectionInfo = { - connectionId: path, - bitrate: this.bitrate, - }; - - this.addEventListener("receive", this.handleReceiveBytes); - // should we add disconnect handler here ? - this.addEventListener("disconnect", this.handleDisconnect); - - // Start port listening - // await this.port.listen(data => { - // this.dispatchEvent(new CustomEvent("receive", { detail: new Uint8Array.from(data) })); - // }); - - // Start reading - this.reading = true; - this.readLoop(); - - this.dispatchEvent(new CustomEvent("connect", { detail: true })); - console.log(`${logHead} Connected to ${path}`); - return true; - } - catch(error) { - console.error(`${logHead} Error connecting:`, error); - - this.openRequested = false; - this.dispatchEvent(new CustomEvent("connect", { detail: false })); - return false; - } + try { + console.log(`${logHead} Connecting to ${path} with options:`, port); + this.port = new SerialPort(port); + const openResult = await this.port.open(); + console.log(`${logHead} Port opened successfully!`, openResult); + + // Connection successful + this.connected = true; + this.connectionId = path; + this.bitrate = port.baudRate; + this.openRequested = false; + + this.connectionInfo = { + connectionId: path, + bitrate: this.bitrate, + }; + + this.addEventListener("receive", this.handleReceiveBytes); + this.addEventListener("disconnect", this.handleDisconnect); + + // Start reading + this.reading = true; + this.readLoop(); + + this.dispatchEvent(new CustomEvent("connect", { detail: true })); + console.log(`${logHead} Connected to ${path}`); + return true; + } catch (error) { + console.error(`${logHead} Error connecting:`, error); + this.openRequested = false; + this.dispatchEvent(new CustomEvent("connect", { detail: false })); + return false; + } + }
226-252: Critical: Missing_parseAndroidPortsResponsemethod.Line 237 calls
this._parseAndroidPortsResponse(newPorts)to parse Android's string response format, but this method is not defined in the class. This will cause a runtime error when the Android plugin returns a string instead of an object.You need to add the
_parseAndroidPortsResponsemethod to parse Android's response format. Based on the comment at line 235-236, the method should convert a string like"{/dev/bus/usb/002/002={type=USB, vid=1155, ...}}"into a proper JavaScript object/Map.
324-358: Fix readBinary API usage: must be instance method with ReadOptions object.Line 329 incorrectly uses
SerialPort.readBinary(this.port, 1024, 100)as a static method with positional parameters. The correct usage is an instance method that accepts ReadOptions (e.g., timeout in ms and maxBytes):const result = await this.port.readBinary({ timeout: 100, maxBytes: 1024 });The current static method pattern will cause runtime failures. Change to instance method with object parameters.
🧹 Nitpick comments (5)
scripts/MainActivity.kt (3)
22-38: Reconsider the synchronized block.The
synchronized(this)block on line 22 uses the anonymousBroadcastReceiverobject as the lock, which doesn't provide meaningful thread safety here.BroadcastReceiver.onReceive()already executes on the main thread, and there's no shared mutable state being protected within this scope.Consider removing the synchronized block:
ACTION_USB_PERMISSION -> { - synchronized(this) { - val device: UsbDevice? = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { - intent.getParcelableExtra(UsbManager.EXTRA_DEVICE, UsbDevice::class.java) - } else { - @Suppress("DEPRECATION") - intent.getParcelableExtra(UsbManager.EXTRA_DEVICE) - } - - if (intent.getBooleanExtra(UsbManager.EXTRA_PERMISSION_GRANTED, false)) { - device?.let { - Log.d(TAG, "Permission granted for device ${it.deviceName}") - // Device is ready to use - } - } else { - Log.d(TAG, "Permission denied for device ${device?.deviceName}") - } + val device: UsbDevice? = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + intent.getParcelableExtra(UsbManager.EXTRA_DEVICE, UsbDevice::class.java) + } else { + @Suppress("DEPRECATION") + intent.getParcelableExtra(UsbManager.EXTRA_DEVICE) + } + + if (intent.getBooleanExtra(UsbManager.EXTRA_PERMISSION_GRANTED, false)) { + device?.let { + Log.d(TAG, "Permission granted for device ${it.deviceName}") + // Device is ready to use + } + } else { + Log.d(TAG, "Permission denied for device ${device?.deviceName}") } }
18-63: Extract repeated USB device extraction logic.The pattern for extracting
UsbDevicefrom anIntentwith API level checks appears five times throughout the file. Consider extracting this into a helper function to improve maintainability.Add a helper function:
private fun Intent.getUsbDevice(): UsbDevice? { return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { getParcelableExtra(UsbManager.EXTRA_DEVICE, UsbDevice::class.java) } else { @Suppress("DEPRECATION") getParcelableExtra(UsbManager.EXTRA_DEVICE) } }Then simplify each usage:
ACTION_USB_PERMISSION -> { - val device: UsbDevice? = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { - intent.getParcelableExtra(UsbManager.EXTRA_DEVICE, UsbDevice::class.java) - } else { - @Suppress("DEPRECATION") - intent.getParcelableExtra(UsbManager.EXTRA_DEVICE) - } + val device: UsbDevice? = intent.getUsbDevice()Also applies to: 82-88, 109-114
139-164: Remove unnecessary cast in PendingIntent creation.Line 156 contains an explicit cast
this as Contextwhich is unnecessary sinceMainActivityextendsTauriActivity, which extendsActivity, andActivityis already aContext.Apply this diff:
val permissionIntent = PendingIntent.getBroadcast( - this as Context, + this, 0, Intent(ACTION_USB_PERMISSION), flags )src/js/protocols/TauriSerial.js (2)
134-142: Stub implementation - Android USB permission flow pending.The
requestPermissionDevicemethod currently just returnstruewithout implementing actual Android USB permission requests. Based on the past review discussion, the proper permission flow requires MainActivity.kt to handleUsbManager.requestPermission()and the ACTION_USB_PERMISSION broadcast.This is acceptable for the current development state, but the implementation should be completed once the Android project structure issues are resolved.
Based on learnings
51-52: Consider clarifying why device monitoring is not auto-started.Line 51 shows that automatic device monitoring via
startDeviceMonitoring()has been disabled in the constructor. If this is intentional (to allow manual control), consider adding a comment explaining the rationale. If it should be enabled, uncomment line 51.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/MainActivity.kt(1 hunks)src/js/protocols/TauriSerial.js(9 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
📚 Learning: 2025-10-25T21:16:32.474Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-11-06T17:26:50.178Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
Applied to files:
src/js/protocols/TauriSerial.jsscripts/MainActivity.kt
📚 Learning: 2025-06-09T00:32:21.385Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:32:21.385Z
Learning: In the betaflight-configurator codebase, port paths use counter prefixes (e.g., "bluetooth1", "bluetooth2", "serial1") rather than direct protocol identifiers. The protocol selection logic correctly uses `portPath.startsWith("bluetooth")` to detect bluetooth ports regardless of the counter suffix, rather than direct string matching against protocol map keys.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-09T00:33:22.959Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:33:22.959Z
Learning: In the betaflight-configurator codebase, port paths use counter suffixes for serial and bluetooth ports (e.g., "serial1", "serial2", "bluetooth1", "bluetooth2") instead of using connectionId, which means simple string matching against protocol map keys won't work for protocol selection.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-05-14T21:51:09.253Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-05-14T21:51:09.253Z
Learning: On Android Chrome browsers, Web Bluetooth API is supported for BLE devices and Web USB API is supported, but Web Serial API is not supported (except limited Bluetooth serial support added in 2025). The Betaflight Configurator should detect and use available APIs on Android rather than requiring all three.
Applied to files:
src/js/protocols/TauriSerial.js
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
🔇 Additional comments (4)
scripts/MainActivity.kt (2)
65-97: Receiver registration and device enumeration flow looks good.The implementation correctly uses
RECEIVER_NOT_EXPORTEDfor Android 13+ (line 76), which is a security best practice. The fallback to check connected devices when not launched by USB attachment is well-structured.One minor consideration: the receiver is registered before checking the launch intent, which theoretically could lead to the same device being processed twice if a USB event fires between lines 79 and 82. However, the
hasPermissioncheck inrequestUsbPermission(line 142) should prevent duplicate permission requests, making this a non-issue in practice. Consider verifying this behavior during testing if you observe any duplicate permission dialogs.
1-165: Solid USB permission and lifecycle handling implementation.The overall implementation comprehensively handles USB device permissions across multiple Android entry points (activity launch, new intents, and broadcast events). The code demonstrates good practices:
- Proper lifecycle management (register/unregister receiver)
- Backward-compatible API usage for multiple Android versions
- Comprehensive logging for debugging
- Early-return guards to prevent duplicate permission requests
The suggested refactors address code quality and maintainability rather than functional correctness.
As per learnings, ensure this file is applied after
tauri android inithas been run, since the Android project structure must exist before USB support patches can be integrated.src/js/protocols/TauriSerial.js (2)
299-310: Resolved: Duplicate receive events fixed.The duplicate receive event dispatching issue flagged in previous reviews has been addressed by commenting out the
port.listen()callback (lines 304-306) and relying solely onreadLoop()for data reception. This eliminates the data duplication problem.
148-171: Resolved: Debug code properly returns filtered ports.The temporary debug code flagged in previous reviews has been addressed. The method now correctly returns
filteredports (line 170) that match known Betaflight VID/PIDs, rather than returning all ports. The DEBUG flag is properly set tofalsefor production.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/js/protocols/TauriSerial.js (3)
266-322: Critical: Orphaned catch block causes syntax error.Lines 316-322 contain a
catchblock that is not attached to anytryblock. The try-catch on lines 279-286 is already complete, and the code on lines 288-315 is not wrapped in a try block. This orphaned catch block is invalid JavaScript syntax and will prevent the code from loading.Apply this diff to properly wrap the connection setup code in a try-catch block:
async connect(path, options) { if (this.openRequested) { console.log(`${logHead} Connection already requested`); return false; } this.openRequested = true; const port = { path: path, baudRate: options.baudRate || 115200, }; try { console.log(`${logHead} Connecting to ${path} with options:`, port); this.port = new SerialPort(port); const openResult = await this.port.open(); console.log(`${logHead} Port opened successfully!`, openResult); - } catch (error) { - console.error(`${logHead} Error connecting:`, error); - } - - // Connection successful - this.connected = true; - this.connectionId = path; - this.bitrate = port.baudRate; - this.openRequested = false; - - this.connectionInfo = { - connectionId: path, - bitrate: this.bitrate, - }; - - this.addEventListener("receive", this.handleReceiveBytes); - // should we add disconnect handler here ? - this.addEventListener("disconnect", this.handleDisconnect); - - // Start port listening - // await this.port.listen(data => { - // this.dispatchEvent(new CustomEvent("receive", { detail: new Uint8Array.from(data) })); - // }); - - // Start reading - this.reading = true; - this.readLoop(); - - this.dispatchEvent(new CustomEvent("connect", { detail: true })); - console.log(`${logHead} Connected to ${path}`); - return true; - } - catch(error) { + + // Connection successful + this.connected = true; + this.connectionId = path; + this.bitrate = port.baudRate; + this.openRequested = false; + + this.connectionInfo = { + connectionId: path, + bitrate: this.bitrate, + }; + + this.addEventListener("receive", this.handleReceiveBytes); + + // Start reading + this.reading = true; + this.readLoop(); + + this.dispatchEvent(new CustomEvent("connect", { detail: true })); + console.log(`${logHead} Connected to ${path}`); + return true; + } catch (error) { console.error(`${logHead} Error connecting:`, error); - this.openRequested = false; this.dispatchEvent(new CustomEvent("connect", { detail: false })); return false; + } }Note: I also removed the problematic disconnect event listener registration on line 301, which is addressed in a separate comment below.
360-389: Incorrect return value: Returns cumulative bytes instead of current send operation.Lines 372-375 accumulate the total bytes sent and return the cumulative total, but the callback and return value should contain the bytes sent in this specific operation, not the running total.
Apply this diff to return the correct value:
try { this.transmitting = true; const dataArray = data instanceof ArrayBuffer ? new Uint8Array(data) : data; - this.bytesSent += await this.port.writeBinary(dataArray); + const bytesSent = await this.port.writeBinary(dataArray); + this.bytesSent += bytesSent; this.transmitting = false; - const res = { bytesSent: this.bytesSent }; + const res = { bytesSent }; callback?.(res); return res;
324-358: SerialPort.read() must be called as instance method on this.port.Line 329 incorrectly calls
SerialPort.read()as a static method. In the tauri-plugin-serialplugin-api, read is an instance method — you call it on a SerialPort object, consistent with howthis.port.writeBinary()is called at line 372. The instance must be used to read from the connected port.Apply this fix:
try { // Non-blocking read with short timeout - const result = await SerialPort.read({ timeout: 100 }); + const result = await this.port.read({ timeout: 100 }); if (result && result.length > 0) { this.dispatchEvent(new CustomEvent("receive", { detail: new Uint8Array(result) }));
🧹 Nitpick comments (1)
src/js/protocols/TauriSerial.js (1)
415-419: Remove unnecessarycancelListen()call.Line 416 calls
this.port.cancelListen()but the correspondingport.listen()is commented out on lines 303-306. Since the listening mechanism is not being used (the code usesreadLoop()with polling instead), thecancelListen()call is unnecessary.Apply this diff to remove the unnecessary call:
// Close the port if (this.port) { - await this.port.cancelListen(); await this.port.close(); console.log(`${logHead} Port closed`); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/TauriSerial.js(9 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
📚 Learning: 2025-10-25T21:16:32.474Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-11-06T17:26:50.178Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-09T00:32:21.385Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:32:21.385Z
Learning: In the betaflight-configurator codebase, port paths use counter prefixes (e.g., "bluetooth1", "bluetooth2", "serial1") rather than direct protocol identifiers. The protocol selection logic correctly uses `portPath.startsWith("bluetooth")` to detect bluetooth ports regardless of the counter suffix, rather than direct string matching against protocol map keys.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-09T00:33:22.959Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:33:22.959Z
Learning: In the betaflight-configurator codebase, port paths use counter suffixes for serial and bluetooth ports (e.g., "serial1", "serial2", "bluetooth1", "bluetooth2") instead of using connectionId, which means simple string matching against protocol map keys won't work for protocol selection.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-05-14T21:51:09.253Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-05-14T21:51:09.253Z
Learning: On Android Chrome browsers, Web Bluetooth API is supported for BLE devices and Web USB API is supported, but Web Serial API is not supported (except limited Bluetooth serial support added in 2025). The Betaflight Configurator should detect and use available APIs on Android rather than requiring all three.
Applied to files:
src/js/protocols/TauriSerial.js
⏰ 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). (2)
- GitHub Check: Tauri Android preview (APK)
- GitHub Check: Tauri Linux preview (deb/appimage)
🔇 Additional comments (5)
src/js/protocols/TauriSerial.js (5)
148-171: LGTM - Filter properly returns filtered devices.The device filtering logic correctly returns the filtered array of known Betaflight-compatible devices. The debug logging is properly gated behind a
DEBUGflag (set tofalse), and the previous issue where all ports were returned has been addressed.
213-224: LGTM - Property names and path correctly fixed.The
createPortmethod now correctly usesport.vendorIdandport.productIdproperties that match the input from_convertPortsMapToArray, and uses the actual device pathport.pathinstead of a hardcoded value. Both previously flagged issues have been properly addressed.
274-277: LGTM - Port path correctly uses function parameter.The port configuration object now correctly uses the
pathparameter from the function signature instead of the invalid regex literal/dev/. The previous issue has been properly addressed.
129-142: Incomplete Android permission implementation.The
requestPermissionDevice()method is a stub that only logs and returnstruewithout actually requesting USB permissions. Based on the learnings, Android requires explicit USB permission flow viaUsbManager.requestPermissionfor device access and enumeration to work.The actual implementation should involve:
- Calling the Tauri plugin's Android-side permission request
- Handling the async permission dialog result
- Returning the actual permission status
Would you like me to help generate the proper implementation using the tauri-plugin-serialplugin Android permission API?
⛔ Skipped due to learnings
Learnt from: haslinghuis Repo: betaflight/betaflight-configurator PR: 4379 File: src/js/protocols/TauriSerial.js:203-259 Timestamp: 2025-10-25T21:16:32.474Z Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.Learnt from: blckmn Repo: betaflight/betaflight-configurator PR: 4521 File: src/js/protocols/WebSerial.js:148-151 Timestamp: 2025-06-19T22:13:09.136Z Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.Learnt from: haslinghuis Repo: betaflight/betaflight-configurator PR: 4687 File: src/js/protocols/TauriSerial.js:299-310 Timestamp: 2025-11-06T17:26:50.178Z Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
231-243: Critical: Missing_parseAndroidPortsResponsemethod.Line 237 calls
this._parseAndroidPortsResponse(newPorts)to handle Android's string-formatted port response, but this method is not defined anywhere in the file. This will cause a runtime error when Android returns port data as a string.The comment on line 235 indicates the expected format:
"{/dev/bus/usb/002/002={type=USB, vid=1155, ...}}". You need to implement this parser to convert the Android string format to a proper ports map object.Apply this diff to add the missing method:
+ /** + * Parse Android's string-formatted ports response into a proper object. + * Android returns: "{/dev/bus/usb/002/002={type=USB, vid=1155, pid=22336, ...}}" + * @private + */ + _parseAndroidPortsResponse(responseString) { + try { + // Remove outer braces and split by port entries + const cleaned = responseString.trim().replace(/^{|}$/g, ''); + const portsMap = {}; + + // Match each port entry: /path={key=value, ...} + const portRegex = /(\/[^=]+)=\{([^}]+)\}/g; + let match; + + while ((match = portRegex.exec(cleaned)) !== null) { + const [, path, propsStr] = match; + const props = {}; + + // Parse key=value pairs + const propRegex = /(\w+)=([^,}]+)/g; + let propMatch; + while ((propMatch = propRegex.exec(propsStr)) !== null) { + const [, key, value] = propMatch; + props[key] = value.trim(); + } + + portsMap[path] = props; + } + + return portsMap; + } catch (error) { + console.error(`${logHead} Failed to parse Android ports response:`, error); + throw error; + } + } + async loadDevices() {⛔ Skipped due to learnings
Learnt from: blckmn Repo: betaflight/betaflight-configurator PR: 4521 File: src/js/protocols/WebSerial.js:148-151 Timestamp: 2025-06-19T22:13:09.136Z Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.Learnt from: haslinghuis Repo: betaflight/betaflight-configurator PR: 4687 File: src/js/protocols/TauriSerial.js:299-310 Timestamp: 2025-11-06T17:26:50.178Z Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
| // this.loadDevices().then(() => this.startDeviceMonitoring()); | ||
| this.loadDevices(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify device monitoring is started elsewhere.
The device monitoring setup has been removed from the constructor. Line 51 previously called startDeviceMonitoring() after loading devices, but this is now commented out. Without this, the automatic device add/remove detection won't work unless startDeviceMonitoring() is explicitly called elsewhere in the initialization flow.
Run the following script to check if device monitoring is started elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for startDeviceMonitoring calls in the codebase
rg -n 'startDeviceMonitoring\(' --type=js -C3Length of output: 798
Restore device monitoring in the constructor.
Device monitoring is not started anywhere in the codebase. The constructor previously called startDeviceMonitoring() after loading devices (line 51, now commented out), but this is never called elsewhere. According to the auto-detection design, the 1-second polling interval for device add/remove detection won't function without this. Uncomment and properly chain the call: this.loadDevices().then(() => this.startDeviceMonitoring());
🤖 Prompt for AI Agents
In src/js/protocols/TauriSerial.js around lines 51 to 52, the constructor calls
this.loadDevices() but no longer starts device monitoring; restore the original
behavior by chaining startDeviceMonitoring() after loadDevices resolves: call
this.loadDevices().then(() => this.startDeviceMonitoring()); so the 1-second
polling for device add/remove auto-detection is re-enabled.
| this.addEventListener("receive", this.handleReceiveBytes); | ||
| // should we add disconnect handler here ? | ||
| this.addEventListener("disconnect", this.handleDisconnect); |
There was a problem hiding this comment.
Major: Incorrect event listener causes disconnect loop.
Line 301 registers handleDisconnect to listen for disconnect events, but this creates a problematic flow:
disconnect()is called (line 391)- It dispatches a "disconnect" event (line 421)
handleDisconnectreceives the event and callsdisconnect()again (line 63)- This creates redundant disconnect calls or potential infinite loop
The handleDisconnect method should listen to device removal events (e.g., removedDevice from line 190), not the disconnect event.
Apply this diff to fix the event listener registration:
this.addEventListener("receive", this.handleReceiveBytes);
- // should we add disconnect handler here ?
- this.addEventListener("disconnect", this.handleDisconnect);Then register it to listen for device removal during device monitoring setup instead:
// In the connect method, after successful connection
// Listen for this specific device being removed
const connectedPath = path;
const deviceRemovedHandler = (event) => {
if (event.detail.path === connectedPath) {
this.handleDisconnect();
}
};
this.addEventListener("removedDevice", deviceRemovedHandler);🤖 Prompt for AI Agents
In src/js/protocols/TauriSerial.js around lines 299–301, the code registers
handleDisconnect for the "disconnect" event which causes a recursive disconnect
loop; remove that addEventListener("disconnect", this.handleDisconnect)
registration and instead, in the connect method after a successful connection,
register a "removedDevice" listener scoped to the connected device path that
calls this.handleDisconnect() only when event.detail.path matches the connected
device path (store connectedPath and create a deviceRemovedHandler that compares
event.detail.path === connectedPath and calls this.handleDisconnect()); ensure
the original erroneous listener is removed and the new deviceRemoved listener is
added during device monitoring setup.
There was a problem hiding this comment.
Is removedDevice event supported in underlying code ?
If not suggest we need the 1s device polling
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if startDeviceMonitoring is called anywhere in the codebase
rg -n "startDeviceMonitoring" --type js -C3
Length of output: 798
🏁 Script executed:
#!/bin/bash
# Check the constructor and device monitoring setup in TauriSerial.js
cat src/js/protocols/TauriSerial.js | grep -A5 -B5 "startDeviceMonitoring\|loadDevices()"
Length of output: 1028
@haslinghuis You're absolutely right! The removedDevice event is implemented in the code (dispatched by checkDeviceChanges() at line 190), but it relies on the 1-second device polling to function.
However, I see the issue: device monitoring is currently disabled. In the constructor at line 51, the call to startDeviceMonitoring() is commented out:
// this.loadDevices().then(() => this.startDeviceMonitoring());
this.loadDevices();So my previous suggestion to listen for removedDevice events won't work unless device monitoring is active.
You have two options:
- Enable device monitoring (uncomment line 51) to make the
removedDeviceevent work, then apply my disconnect handler fix - Keep device monitoring disabled and remove the problematic
disconnectevent listener entirely (just delete lines 299-301)
If device monitoring stays disabled, there won't be automatic detection of device removal, so the handleDisconnect listener serves no purpose anyway. The disconnect loop issue would still exist if someone manually calls disconnect(), so it's safer to remove it.
Which approach would you prefer?
🧠 Learnings used
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/protocols/TauriSerial.js (2)
266-318: Critical: Invalid JavaScript syntax - orphaned catch block.The
connectmethod has a syntax error. Thetryblock ends at line 286, but lines 288-311 contain connection success handling code that is outside any try/catch, followed by an orphanedcatchblock (lines 312-318) with no associatedtry. This will cause a syntax error at runtime.Apply this diff to fix the structure:
async connect(path, options) { if (this.openRequested) { console.log(`${logHead} Connection already requested`); return false; } this.openRequested = true; const port = { path: path, baudRate: options.baudRate || 115200, }; try { console.log(`${logHead} Connecting to ${path} with options:`, port); this.port = new SerialPort(port); const openResult = await this.port.open(); console.log(`${logHead} Port opened successfully!`, openResult); - } catch (error) { - console.error(`${logHead} Error connecting:`, error); - } - - // Connection successful - this.connected = true; - this.connectionId = path; - this.bitrate = port.baudRate; - this.openRequested = false; - - this.connectionInfo = { - connectionId: path, - bitrate: this.bitrate, - }; - - this.addEventListener("receive", this.handleReceiveBytes); - // should we add disconnect handler here ? - this.addEventListener("disconnect", this.handleDisconnect); - - // On mobile platforms, listen() events may not work reliably - // Use active polling with read() instead - this.reading = true; - this.readLoop(); - - this.dispatchEvent(new CustomEvent("connect", { detail: true })); - console.log(`${logHead} Connected to ${path}`); - return true; - } - catch(error) { - console.error(`${logHead} Error connecting:`, error); - - this.openRequested = false; - this.dispatchEvent(new CustomEvent("connect", { detail: false })); - return false; + + // Connection successful + this.connected = true; + this.connectionId = path; + this.bitrate = port.baudRate; + this.openRequested = false; + + this.connectionInfo = { + connectionId: path, + bitrate: this.bitrate, + }; + + this.addEventListener("receive", this.handleReceiveBytes); + // should we add disconnect handler here ? + this.addEventListener("disconnect", this.handleDisconnect); + + // On mobile platforms, listen() events may not work reliably + // Use active polling with read() instead + this.reading = true; + this.readLoop(); + + this.dispatchEvent(new CustomEvent("connect", { detail: true })); + console.log(`${logHead} Connected to ${path}`); + return true; + } catch (error) { + console.error(`${logHead} Error connecting:`, error); + this.openRequested = false; + this.dispatchEvent(new CustomEvent("connect", { detail: false })); + return false; + } }
226-252: Implement missing_parseAndroidPortsResponsemethod.The method called at line 237 is not defined in the codebase. This will cause a runtime error whenever Android returns a string-formatted port response. You need to implement this method to parse the string format described in the comment (e.g.,
"{/dev/bus/usb/002/002={type=USB, vid=1155, ...}}") into a proper ports map object for processing by_convertPortsMapToArray.
♻️ Duplicate comments (1)
src/js/protocols/TauriSerial.js (1)
299-301: Major: Disconnect event listener still causes recursion.Line 301 registers
handleDisconnectfor the"disconnect"event, which creates a recursion issue:
disconnect()is called (line 374)- It dispatches a "disconnect" event (line 408)
handleDisconnectreceives the event and callsdisconnect()again (line 63)This was flagged in previous reviews. Since device monitoring is currently disabled (line 51), the
removedDeviceevents won't fire, making this listener ineffective for detecting physical disconnections anyway.Apply this diff to remove the problematic listener:
this.addEventListener("receive", this.handleReceiveBytes); - // should we add disconnect handler here ? - this.addEventListener("disconnect", this.handleDisconnect);Note: If you re-enable device monitoring (line 51), consider using
removedDeviceevents instead to detect physical disconnections without recursion.
🧹 Nitpick comments (2)
src/js/protocols/TauriSerial.js (2)
51-52: Device auto-detection is disabled.Line 51 has the
startDeviceMonitoring()call commented out, which disables automatic detection of device connections and disconnections. Based on your PR comment about "Backtracking on AI suggestions," this may be intentional.However, be aware that without device monitoring:
- The 1-second polling interval won't run
addedDeviceandremovedDeviceevents won't fire- Users must manually refresh the device list via
getDevices()orloadDevices()If auto-detection is not needed for your use case, consider removing the entire
startDeviceMonitoring(),stopDeviceMonitoring(), andcheckDeviceChanges()methods to reduce code complexity. Otherwise, if you plan to re-enable it later, add a TODO comment explaining why it's currently disabled.
320-338: Consider gating verbose read logging.Line 327 logs every successful read with byte count. During active serial communication, this will generate significant console spam. Consider either removing this log in production or gating it behind a debug flag (similar to the approach used in
_filterToKnownDevicesat lines 149-150).Apply this diff to gate the verbose logging:
+ const DEBUG_READS = false; console.log(`${logHead} Starting read loop`); while (this.reading) { try { const result = await this.port.read({ timeout: 100, size: 1024 }); if (result && Array.isArray(result) && result.length > 0) { - console.log(`${logHead} Read ${result.length} bytes`); + if (DEBUG_READS) console.log(`${logHead} Read ${result.length} bytes`); this.dispatchEvent(new CustomEvent("receive", { detail: new Uint8Array(result) })); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/TauriSerial.js(9 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
📚 Learning: 2025-10-25T21:16:32.474Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-11-06T17:26:50.178Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-09T00:32:21.385Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:32:21.385Z
Learning: In the betaflight-configurator codebase, port paths use counter prefixes (e.g., "bluetooth1", "bluetooth2", "serial1") rather than direct protocol identifiers. The protocol selection logic correctly uses `portPath.startsWith("bluetooth")` to detect bluetooth ports regardless of the counter suffix, rather than direct string matching against protocol map keys.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-06-09T00:33:22.959Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-06-09T00:33:22.959Z
Learning: In the betaflight-configurator codebase, port paths use counter suffixes for serial and bluetooth ports (e.g., "serial1", "serial2", "bluetooth1", "bluetooth2") instead of using connectionId, which means simple string matching against protocol map keys won't work for protocol selection.
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/protocols/TauriSerial.js
📚 Learning: 2025-05-14T21:51:09.253Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-05-14T21:51:09.253Z
Learning: On Android Chrome browsers, Web Bluetooth API is supported for BLE devices and Web USB API is supported, but Web Serial API is not supported (except limited Bluetooth serial support added in 2025). The Betaflight Configurator should detect and use available APIs on Android rather than requiring all three.
Applied to files:
src/js/protocols/TauriSerial.js
⏰ 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). (2)
- GitHub Check: Tauri Linux preview (deb/appimage)
- GitHub Check: Tauri Android preview (APK)
🔇 Additional comments (1)
src/js/protocols/TauriSerial.js (1)
134-141: Verify Android permission implementation.The
requestPermissionDevice()method is documented to "trigger the permission dialog" (line 131), but the current implementation only logs and returnstruewithout actually requesting any USB permissions.Since you mentioned solving the Android permission issues in earlier comments, the permission flow may be handled elsewhere (e.g., in MainActivity.kt or via the Tauri plugin's native layer). However, if this method is intended to be called from JavaScript to trigger permission requests, it needs actual implementation.
Confirm whether:
- Android permissions are automatically handled by MainActivity.kt/plugin (making this stub correct)
- This method should invoke a Tauri command to trigger the native permission flow
- This is a placeholder for future implementation
Based on learnings: In tauri-plugin-serialplugin Android development, the Android project must be initialized via
tauri android initbefore the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/tauri-patch-android.sh (3)
22-22: Add validation for subordinate script existence before execution.The script calls four subordinate scripts (patch-android-manifest.sh, create-device-filter.sh, patch-gradle-settings.sh, patch-app-gradle.sh) without verifying they exist or are executable. If any subordinate script is missing or corrupted, the error message will be cryptic.
Add an existence check before each invocation:
+# Verify subordinate scripts exist before running them +for script in patch-android-manifest.sh create-device-filter.sh patch-gradle-settings.sh patch-app-gradle.sh; do + if [ ! -f "$SCRIPT_DIR/$script" ]; then + echo "Error: Required script not found: $SCRIPT_DIR/$script" + exit 1 + fi +done + echo "1. Patching Android manifest..." bash "$SCRIPT_DIR/patch-android-manifest.sh"Alternatively, add a check at the top after
SCRIPT_DIRis set:+# Verify all subordinate scripts exist +required_scripts=("patch-android-manifest.sh" "create-device-filter.sh" "patch-gradle-settings.sh" "patch-app-gradle.sh") +for script in "${required_scripts[@]}"; do + [ -f "$SCRIPT_DIR/$script" ] || { echo "Error: Missing required script: $script"; exit 1; } +doneAlso applies to: 25-25, 28-28, 31-31
40-46: Add validation that custom MainActivity copy succeeds.The script copies the custom MainActivity but doesn't verify the copy succeeded or validate the destination file. If the copy fails silently (e.g., due to permission issues), the build may proceed with incorrect or missing MainActivity.
if [ -f "$CUSTOM_MAIN_ACTIVITY_SRC" ]; then mkdir -p "$CUSTOM_MAIN_ACTIVITY_DST_DIR" - cp "$CUSTOM_MAIN_ACTIVITY_SRC" "$CUSTOM_MAIN_ACTIVITY_DST" - echo " - Custom MainActivity applied to $CUSTOM_MAIN_ACTIVITY_DST" + if cp "$CUSTOM_MAIN_ACTIVITY_SRC" "$CUSTOM_MAIN_ACTIVITY_DST"; then + echo " - Custom MainActivity applied to $CUSTOM_MAIN_ACTIVITY_DST" + else + echo "Error: Failed to copy custom MainActivity" + exit 1 + fi else echo " - No custom MainActivity found at $CUSTOM_MAIN_ACTIVITY_SRC; skipping" fi
37-46: Hardcoded package path is currently correct but fragile—consider extracting it dynamically.The script hardcodes the MainActivity destination to
com.betaflight.app, which matches theidentifierinsrc-tauri/tauri.conf.json. However, this creates a maintenance risk: if someone changes the identifier in the configuration file without updating the script, the custom MainActivity will be silently placed in the wrong directory and never executed.Extract the package name from the generated AndroidManifest.xml at runtime, or document this hardcoded assumption prominently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tauri-patch-android.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
📚 Learning: 2025-11-06T17:26:50.178Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4687
File: src/js/protocols/TauriSerial.js:299-310
Timestamp: 2025-11-06T17:26:50.178Z
Learning: In tauri-plugin-serialplugin Android development, the Android project must be initialized via `tauri android init` before the USB support patches (AndroidManifest.xml permissions, device_filter.xml, MainActivity.kt) can be applied. Without initialization, the plugin returns empty results from available_ports_direct() because the generated Android project structure and USB permissions don't exist yet.
Applied to files:
scripts/tauri-patch-android.sh
⏰ 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). (2)
- GitHub Check: Tauri Android preview (APK)
- GitHub Check: Tauri Linux preview (deb/appimage)
2 participants
Using
Issue with Tauri Android is the
Summary by CodeRabbit
New Features
Infrastructure & Build
Documentation