Add custom Capacitor plugin-socket for raw TCP support #4471
Conversation
WalkthroughAdds an Android TCP transport: new Capacitor Android plugin (BetaflightTcpPlugin), registers it in MainActivity (plugin registrations moved before super.onCreate), adds a CapacitorTcp JS wrapper, updates Android protocol selection to include TCP, and relaxes some serial backend reconnect logic. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as App UI
participant SerialMgr as serial.js
participant CapacitorTcp as CapacitorTcp (JS)
participant Plugin as BetaflightTcpPlugin (Android)
participant Socket as TCP Socket
participant Reader as Plugin Reader Thread
UI->>SerialMgr: request connect via TCP
SerialMgr->>CapacitorTcp: connect(address)
CapacitorTcp->>Plugin: connect(host, port)
Plugin->>Plugin: set CONNECTING, open socket (with timeout)
Plugin->>Socket: connect
Socket-->>Plugin: connected
Plugin->>Plugin: init streams, set CONNECTED, start Reader
Plugin-->>CapacitorTcp: resolve connect
CapacitorTcp-->>SerialMgr: emit connect
SerialMgr-->>UI: notify connected
UI->>SerialMgr: send(data)
SerialMgr->>CapacitorTcp: send(data)
CapacitorTcp->>Plugin: send(base64)
Plugin->>Plugin: decode, writer lock, write to socket
Plugin-->>CapacitorTcp: send success
CapacitorTcp-->>SerialMgr: notify send complete
Reader->>Socket: read bytes
Socket-->>Reader: data chunk
Reader->>Plugin: emit dataReceived(base64)
Plugin-->>CapacitorTcp: event dataReceived
CapacitorTcp-->>SerialMgr: forward data
SerialMgr-->>UI: data available
UI->>SerialMgr: disconnect()
SerialMgr->>CapacitorTcp: disconnect()
CapacitorTcp->>Plugin: disconnect()
Plugin->>Plugin: set DISCONNECTING, close resources
Plugin-->>CapacitorTcp: emit connectionClosed / resolve
CapacitorTcp-->>SerialMgr: emit disconnect
SerialMgr-->>UI: disconnected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 7
🧹 Nitpick comments (6)
capacitor-plugin-socket/src/definitions.ts (2)
1-6: Consider supporting binary data payloads.
Raw TCP sockets often exchange binary data. Currentlysend/receiveusestringfordata. You may want to useArrayBufferorUint8Array(or overloads) in the interface to handle binary payloads.
1-6: Add JSDoc comments for improved DX.
Annotating each method (connect,send,receive,disconnect) with JSDoc will surface signatures and parameter docs in editors.capacitor-plugin-socket/src/index.ts (2)
1-1: Specify the plugin interface generic.
Use the generic onregisterPluginfor full TypeScript type inference:const SocketPlugin = registerPlugin<SocketPlugin>('SocketPlugin', { … });
3-5: Add explicit iOS fallback.
Currently onlywebis defined. To avoid runtime errors on unsupported platforms, add aniosstub to import your web implementation:const SocketPlugin = registerPlugin<SocketPlugin>('SocketPlugin', { web: () => import('./web').then(m => new m.SocketPluginWeb()), ios: () => import('./web').then(m => new m.SocketPluginWeb()), });capacitor-plugin-socket/src/web.ts (1)
5-23: Use the WebPlugin’s unimplemented helper.
Rather thanconsole.log+ default returns, callthis.unimplemented()for a standardized unimplemented error:async connect(...) { - console.log('Web implementation does not support raw TCP sockets.', options); - return { success: false }; + return this.unimplemented('Raw TCP sockets are not supported on web.'); }Apply similarly for
send,receive, anddisconnect.android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
1-72: Add thread safety and consider using AsyncTask or ExecutorService.The current implementation performs network operations on the main thread and lacks thread safety:
- Network operations on the main UI thread can cause ANR issues
- No proper handling of concurrent method calls
- Consider using background threads for socket operations
While Capacitor's
executeAsynchelps with background execution, you may want to implement a more robust threading strategy. Consider:
- Using Android's ExecutorService for managing a thread pool
- Implementing proper synchronization for shared resources
- Adding cancellation support for long-running operations
Additionally, consider implementing timeout handling for all socket operations and add proper documentation for users of this plugin, explaining:
- When to use SSL vs. raw sockets
- How to handle timeouts and connection failures
- Best practices for error handling
🧰 Tools
🪛 ast-grep (0.31.1)
[info] 23-23: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
android/app/src/main/java/betaflight/configurator/MainActivity.java(1 hunks)android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)capacitor-plugin-socket/package.json(1 hunks)capacitor-plugin-socket/src/definitions.ts(1 hunks)capacitor-plugin-socket/src/index.ts(1 hunks)capacitor-plugin-socket/src/web.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
android/app/src/main/java/betaflight/configurator/MainActivity.java (1)
capacitor-plugin-socket/src/definitions.ts (1)
SocketPlugin(1-6)
capacitor-plugin-socket/src/index.ts (1)
capacitor-plugin-socket/src/definitions.ts (1)
SocketPlugin(1-6)
capacitor-plugin-socket/src/web.ts (1)
capacitor-plugin-socket/src/definitions.ts (1)
SocketPlugin(1-6)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
capacitor-plugin-socket/src/definitions.ts (1)
SocketPlugin(1-6)capacitor-plugin-socket/src/web.ts (4)
connect(5-8)send(10-13)receive(15-18)disconnect(20-23)
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 23-23: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
android/app/src/main/java/betaflight/configurator/MainActivity.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
haslinghuis
force-pushed
the
capacitor-plugin-socket
branch
from
2881743 to
507f32c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (7)
19-33: Improve security and robustness inconnectmethod.The connect method has several issues that could lead to application crashes or security vulnerabilities:
- No input validation
- No check for existing connections
- Using an unencrypted socket (security concern)
- Operations run on the main thread (may freeze UI)
- No socket timeout
- No resource cleanup on failure
🧰 Tools
🪛 ast-grep (0.31.1)
[info] 23-23: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
35-47: Add state validation to send method.The send method should check if there's an active connection before attempting to write data. It also runs on the main thread and lacks input validation.
49-59: Potential blocking operation in receive method.The
readLine()method blocks indefinitely until data with a line terminator is available or the connection is closed, which can cause the app to freeze.
61-72: Improve resource cleanup in disconnect method.The disconnect method only closes the socket but doesn't properly clean up all resources:
- Reader and writer aren't closed
- Class fields aren't reset
- No state tracking
1-72: 🛠️ Refactor suggestionImplement a helper method for resource cleanup.
The class needs a helper method for closing resources consistently across all methods to avoid resource leaks and maintain proper state.
@PluginMethod public void disconnect(PluginCall call) { try { - if (socket != null) socket.close(); + closeResources(); + isConnected = false; JSObject ret = new JSObject(); ret.put("success", true); call.resolve(ret); } catch (Exception e) { call.reject("Disconnect failed: " + e.getMessage()); } } +/** + * Helper method to close all resources and clean up state + */ +private void closeResources() { + try { + if (reader != null) { + reader.close(); + reader = null; + } + if (writer != null) { + writer.close(); + writer = null; + } + if (socket != null) { + socket.close(); + socket = null; + } + } catch (IOException e) { + // Log but continue cleanup + Log.e("SocketPlugin", "Error closing resources", e); + } +}🧰 Tools
🪛 ast-grep (0.31.1)
[info] 23-23: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
13-18: 🛠️ Refactor suggestionAdd connection state tracking.
The class lacks state management to track when a socket is connected. This is important for preventing operations on closed sockets and proper cleanup.
@CapacitorPlugin(name = "SocketPlugin") public class SocketPlugin extends Plugin { private Socket socket; private BufferedReader reader; private BufferedWriter writer; + private boolean isConnected = false;
51-52: 🛠️ Refactor suggestionMove network I/O off the main thread.
Reading from the socket on the main thread can cause UI freezes (ANR - Application Not Responding). Move this operation to a background thread.
@PluginMethod public void receive(PluginCall call) { + // Verify connection state first + if (socket == null || !isConnected) { + call.reject("Not connected to any server"); + return; + } + + // Use executeAsync to move this work off the main thread + executeAsync(() -> { try { String data = reader.readLine(); + if (data == null) { + // Connection closed by server + closeResources(); + isConnected = false; + call.reject("Connection closed by server"); + return; + } JSObject ret = new JSObject(); ret.put("data", data); call.resolve(ret); } catch (Exception e) { + closeResources(); + isConnected = false; call.reject("Receive failed: " + e.getMessage()); } + }); }
🧹 Nitpick comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
1-12: Remove unused import and add class documentation.The
android.util.Logimport on line 3 is not used in the code. Also, the class would benefit from proper documentation explaining its purpose and usage.package betaflight.configurator.plugin; -import android.util.Log; import com.getcapacitor.JSObject; import com.getcapacitor.Plugin; import com.getcapacitor.PluginCall; import com.getcapacitor.annotation.CapacitorPlugin; import java.io.*; import java.net.Socket; +/** + * Capacitor plugin that provides raw TCP socket functionality. + * Implements methods to connect, send, receive, and disconnect from TCP servers. + */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
android/app/src/main/java/betaflight/configurator/MainActivity.java(1 hunks)android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)capacitor-plugin-socket/package.json(1 hunks)capacitor-plugin-socket/src/definitions.ts(1 hunks)capacitor-plugin-socket/src/index.ts(1 hunks)capacitor-plugin-socket/src/web.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- android/app/src/main/java/betaflight/configurator/MainActivity.java
- capacitor-plugin-socket/src/web.ts
- capacitor-plugin-socket/package.json
- capacitor-plugin-socket/src/index.ts
- capacitor-plugin-socket/src/definitions.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
capacitor-plugin-socket/src/definitions.ts (1)
SocketPlugin(1-6)capacitor-plugin-socket/src/web.ts (4)
connect(5-8)send(10-13)receive(15-18)disconnect(20-23)
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 23-23: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
23-23:❓ Verification inconclusive
Consider using encrypted sockets for sensitive data.
The current implementation uses unencrypted sockets which could expose transmitted data to network eavesdropping. For sensitive information, consider using SSL/TLS sockets.
🏁 Script executed:
#!/bin/bash # Check if the application is transmitting sensitive information through this socket rg -i "password|credential|token|secret|key" --type java -C 3 android/app/src/main | grep -i "socket"Length of output: 101
🏁 Script executed:
#!/bin/bash # Inspect SocketPlugin.java for socket initialization patterns echo "=== new Socket usage ===" grep -R "new Socket" -n android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java || echo "none found" echo "=== SSLSocket usage ===" grep -R "SSLSocket" -n android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java || echo "none found" echo "=== SSLContext usage ===" grep -R "SSLContext" -n android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java || echo "none found"Length of output: 580
Evaluate encryption for SocketPlugin.java
The
SocketPlugincurrently opens an unencrypted TCP socket (java.net.Socket) on line 24 without any SSL/TLS layers. If this channel ever carries sensitive data (credentials, tokens, personal info), it would be exposed to eavesdropping.• File:
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
– Line 24:socket = new Socket(ip, port);
– No use ofSSLSocketorSSLContextdetectedPlease verify whether any sensitive information is sent over this socket. If so, replace
Socketwith anSSLSocket(or configure anSSLContext) to enable TLS encryption.🧰 Tools
🪛 ast-grep (0.31.1)
[info] 23-23: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
haslinghuis
force-pushed
the
capacitor-plugin-socket
branch
from
1864ae5 to
b94e7a6
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (3)
capacitor-plugin-socket/package.json (3)
12-13: Replace placeholder author information before publishing
The"author": "Your Name"placeholder will be published to the registry as-is.- "author": "Your Name", + "author": "Betaflight Configurator Team <[email protected]>",
1-20: Add repository / bugs URLs for better traceability
Users of the plugin will not see where to report issues or view the source."license": "MIT", + "repository": { + "type": "git", + "url": "https://github.com/betaflight-configurator/capacitor-plugin-socket" + }, + "bugs": { + "url": "https://github.com/betaflight-configurator/capacitor-plugin-socket/issues" + },
8-10: Include a clean step and ensure output goes todist/
tscwill emit files next to sources unlessoutDiris configured. Consider:"scripts": { - "build": "tsc" + "clean": "rimraf dist", + "build": "npm run clean && tsc -p tsconfig.json" },Also add
rimraftodevDependenciesif you adopt this.
🛑 Comments failed to post (5)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (5)
88-96: 🛠️ Refactor suggestion
receive()performs a blocking read on the UI thread
reader.readLine()can block indefinitely, risking ANR. Off-load to a background thread:executeAsync(() -> { try { String data = reader.readLine(); … } catch (…) { … } });Also verify connection state (
socket == null || !isConnected), and handlenullresults (server closed).🤖 Prompt for AI Agents
In android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java around lines 88 to 96, the receive() method performs a blocking read on the UI thread using reader.readLine(), which can cause ANR issues. To fix this, wrap the readLine() call and subsequent logic inside an executeAsync() runnable to offload it to a background thread. Additionally, before reading, check if the socket is null or not connected and reject the call early if so. Also handle the case where readLine() returns null, indicating the server closed the connection, and reject the call accordingly.
68-72: 🛠️ Refactor suggestion
send()still executed on main thread
Large writes or network stalls will freeze the UI. Similar toreceive(), wrap the write inexecuteAsync(() -> { … }).🤖 Prompt for AI Agents
In android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java around lines 68 to 72, the send() method is currently executed on the main thread, which can cause UI freezes during large writes or network delays. To fix this, wrap the send operation inside an executeAsync(() -> { ... }) block to run it asynchronously off the main thread, similar to how receive() is handled. This will prevent UI blocking during socket writes.
44-53:
⚠️ Potential issue
isConnectednever set totrueafter successful connect
Because the flag remainsfalse, everysend()call will immediately reject.writer = new BufferedWriter(new OutputStreamWriter(socket.getOutputStream())); + isConnected = true;📝 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.try { socket = new Socket(ip, port); socket.setSoTimeout(30_000); // 30s timeout reader = new BufferedReader(new InputStreamReader(socket.getInputStream())); writer = new BufferedWriter(new OutputStreamWriter(socket.getOutputStream())); isConnected = true; JSObject ret = new JSObject(); ret.put("success", true); call.resolve(ret); } catch (Exception e) { closeResources();🧰 Tools
🪛 ast-grep (0.31.1)
[info] 44-44: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
🤖 Prompt for AI Agents
In android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java around lines 44 to 53, the isConnected flag is not set to true after a successful socket connection, causing all send() calls to reject immediately. To fix this, set isConnected to true right after the socket and streams are successfully initialized and before resolving the call, ensuring the flag accurately reflects the connection state.
24-35: 🛠️ Refactor suggestion
Potential NPE from auto-unboxing
Integerport value
call.getInt("port")returnsInteger; if absent it becomesnulland unboxing intointthrows.- int port = call.getInt("port"); + Integer port = call.getInt("port");Then adjust subsequent comparisons (
port == null || port < 1 …).📝 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.String ip = call.getString("ip"); - int port = call.getInt("port"); + Integer port = call.getInt("port"); // Validate inputs if (ip == null || ip.isEmpty()) { call.reject("IP address is required"); return; } if (port <= 0 || port > 65535) { call.reject("Invalid port number"); return; }🤖 Prompt for AI Agents
In android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java around lines 24 to 35, the code uses call.getInt("port") which returns an Integer that may be null, causing a NullPointerException when auto-unboxed to int. Change the port variable to Integer and check if it is null before performing numeric comparisons. Update the validation condition to verify port is not null and within the valid range before proceeding.
129-133:
⚠️ Potential issueMissing
Logimport – compilation will fail
Log.eis referenced butandroid.util.Logis not imported.+import android.util.Log;📝 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.// at the top of android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java package betaflight.configurator.plugin; import android.util.Log; import com.getcapacitor.JSObject; import com.getcapacitor.Plugin; import com.getcapacitor.PluginCall; import com.getcapacitor.annotation.CapacitorPlugin; // … rest of imports and class definition🤖 Prompt for AI Agents
In android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java around lines 129 to 133, the code uses Log.e for error logging but does not import android.util.Log, causing a compilation error. Add the import statement for android.util.Log at the top of the file with the other imports to fix this issue.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
44-56: 🛠️ Refactor suggestionBlocking network I/O is still executed on the UI thread
new Socket(...)and stream construction are potentially slow and will freeze the UI, causing ANRs on older devices.
A previous review already pointed this out; consider wrapping the whole block inexecuteAsync(() -> { … }).🧰 Tools
🪛 ast-grep (0.31.1)
[info] 44-44: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
87-97:⚠️ Potential issue
receiveis blocking & lacks state checksAll earlier concerns remain:
- No
socket != null && isConnectedcheck.readLine()blocks indefinitely on the UI thread.data == null(server closed) is not handled.Please apply the earlier suggestion: guard state, use
executeAsync, and handle EOF.
🧹 Nitpick comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
112-134: UI-thread hopping for a simple log is unnecessary &getContext()may not exist
getContext().getActivity()is not part of the publicPluginAPI; usegetActivity()directly, or just:- // Log but continue cleanup - getContext().getActivity().runOnUiThread(() -> - Log.e("SocketPlugin", "Error closing resources", e)); + // Log but continue cleanup + Log.e("SocketPlugin", "Error closing resources", e);This also eliminates the need to obtain the main-thread handler.
44-46: Consider encrypted transport (SSL/TLS) when handling credentials or PIIStatic analysis flags the clear-text
Socket; if the plugin will ever carry sensitive data, plan anSSLSocketoption or document the security trade-offs.🧰 Tools
🪛 ast-grep (0.31.1)
[info] 44-44: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
android/app/src/main/java/betaflight/configurator/MainActivity.java(1 hunks)android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)capacitor-plugin-socket/package.json(1 hunks)capacitor-plugin-socket/src/definitions.ts(1 hunks)capacitor-plugin-socket/src/index.ts(1 hunks)capacitor-plugin-socket/src/web.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- android/app/src/main/java/betaflight/configurator/MainActivity.java
- capacitor-plugin-socket/src/definitions.ts
- capacitor-plugin-socket/src/web.ts
- capacitor-plugin-socket/src/index.ts
- capacitor-plugin-socket/package.json
🧰 Additional context used
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 44-44: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
haslinghuis
force-pushed
the
capacitor-plugin-socket
branch
2 times, most recently
from
84f4673 to
acfa06d
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
38-56: 🛠️ Refactor suggestion
⚠️ Potential issue
connectnever setsisConnectedand still blocks the main thread
isConnectedis never flipped totrue; every latersend/receivecall will be rejected.- All network I/O is executed on the UI thread → risk of ANR.
- try { - socket = new Socket(ip, port); + executeAsync(() -> { + try { + socket = new Socket(ip, port); socket.setSoTimeout(30_000); // 30 s timeout reader = new BufferedReader(new InputStreamReader(socket.getInputStream())); writer = new BufferedWriter(new OutputStreamWriter(socket.getOutputStream())); + isConnected = true; // 🟢 mark connected JSObject ret = new JSObject(); ret.put("success", true); - call.resolve(ret); - } catch (Exception e) { - closeResources(); - call.reject("Connection failed: " + e.getMessage()); - } + call.resolve(ret); + } catch (Exception e) { + closeResources(); + call.reject("Connection failed: " + e.getMessage()); + } + });🧰 Tools
🪛 ast-grep (0.31.1)
[info] 44-44: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
58-85: 🛠️ Refactor suggestion
sendstill runs on UI thread and lacks framing / synchronizationLarge writes can stall the UI and concurrent calls may corrupt the stream.
Suggested adjustments:
- Wrap the logic in
executeAsync.synchronized(writer)to prevent concurrent writes.- Append a newline or convert to bytes if the protocol is line-oriented/binary.
- try { - writer.write(data); - writer.flush(); + executeAsync(() -> { + try { + synchronized (writer) { + writer.write(data); + writer.flush(); + } + JSObject ret = new JSObject(); + ret.put("success", true); + call.resolve(ret); + } catch (Exception e) { + closeResources(); + isConnected = false; + call.reject("Send failed: " + e.getMessage()); + } + }); - JSObject ret = new JSObject(); - ret.put("success", true); - call.resolve(ret); - } catch (Exception e) { - closeResources(); - isConnected = false; - call.reject("Send failed: " + e.getMessage()); - }
🧹 Nitpick comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
116-134: Potential NPE whengetActivity()is null & missing error logging strategy
getContext().getActivity()may returnnullwhen the plugin is used without an Activity (e.g. in background). Consider usingLogdirectly or gating the UI-thread call:if (getContext() != null && getContext().getActivity() != null) { getContext().getActivity().runOnUiThread( () -> Log.e("SocketPlugin", "Error closing resources", e)); } else { Log.e("SocketPlugin", "Error closing resources", e); }Also consider switching wildcard
java.io.*import to explicit classes for clarity.
44-46: Unencrypted socket — evaluate need for TLSStatic analysis flags clear-text transmission (
new Socket(ip, port)). If the data is sensitive, migrate toSSLSocketFactoryor document why plain TCP is acceptable.🧰 Tools
🪛 ast-grep (0.31.1)
[info] 44-44: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
🛑 Comments failed to post (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
87-97: 🛠️ Refactor suggestion
⚠️ Potential issue
receivecan freeze the UI and lacks connection/state checks
readLine()blocks indefinitely on the main thread and there is no guard for disconnected sockets; on failure resources stay open.Key fixes:
- Check
socket != null && !socket.isClosed() && isConnected.- Run the blocking read inside
executeAsync.- Handle
nullreturn (peer closed) and callcloseResources().🤖 Prompt for AI Agents
In android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java around lines 87 to 97, the receive method blocks the main thread by calling readLine() directly and does not check socket connection state, risking UI freeze and resource leaks. Fix this by first verifying socket is not null, not closed, and connected before reading. Move the blocking readLine() call inside an executeAsync block to avoid blocking the UI thread. Also, handle the case where readLine() returns null (indicating the peer closed the connection) by calling closeResources() to clean up properly.
3-10:
⚠️ Potential issueMissing imports lead to compilation failure
@PluginMethodandLogare referenced but not imported, so the file will not compile.import com.getcapacitor.annotation.CapacitorPlugin; +import com.getcapacitor.annotation.PluginMethod; // annotation in use + +import android.util.Log; // used in closeResources📝 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.import com.getcapacitor.JSObject; import com.getcapacitor.Plugin; import com.getcapacitor.PluginCall; import com.getcapacitor.annotation.CapacitorPlugin; import com.getcapacitor.annotation.PluginMethod; import android.util.Log; import java.io.*; import java.net.Socket;🤖 Prompt for AI Agents
In android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java around lines 3 to 10, the imports for @PluginMethod and Log are missing, causing compilation errors. Add the import statements for com.getcapacitor.PluginMethod and android.util.Log to resolve the missing references and ensure the file compiles successfully.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (6)
1-23: Refine imports and class‐level documentation
The wildcard importjava.io.*can be replaced with explicit imports (BufferedReader,InputStreamReader,BufferedWriter,OutputStreamWriter,IOException) to improve readability and avoid accidental namespace collisions. Also, consider movingimport android.util.Log;next to other Android imports for consistency.
24-34: Enhance parameter validation feedback
Current checks reject on missing/empty IP or invalid port, but don’t include the invalid value in the error. Consider:- if (ip == null || ip.isEmpty()) { - call.reject("IP address is required"); + if (ip == null || ip.isEmpty()) { + call.reject("IP address is required: received '" + ip + "'"); return; } - if (port <= 0 || port > 65535) { - call.reject("Invalid port number"); + if (port <= 0 || port > 65535) { + call.reject("Invalid port number: " + port); return; }
40-47: Prevent double‐registration race
The duplicate‐connection guard checkssocket.isClosed(), but doesn’t account for a connection in progress. Consider synchronizing access tosocketor using an explicit lock to avoid a race condition when twoconnectcalls happen in quick succession.🧰 Tools
🪛 ast-grep (0.31.1)
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
46-55: Handle encrypted connections or document trust boundary
Static analysis flagged an unencryptedSocket. If this plugin may transport sensitive data, either switch toSSLSocketFactory(SSLSocket) or clearly document that it’s intended for plain‐text debug use only.🧰 Tools
🪛 ast-grep (0.31.1)
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
61-69: Early‐return input validation is good—align formatting
The@PluginMethodannotation onsendis flush‐left; indent it to match other methods for consistency.
115-137: Simplify resource cleanup logging
You post theLog.ecall back on the UI thread; logging can occur on the current thread safely. You can simplify:- getContext().getActivity().runOnUiThread(() -> - Log.e("SocketPlugin", "Error closing resources", e)); + Log.e("SocketPlugin", "Error closing resources", e);This avoids unnecessary UI thread marshaling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
android/app/src/main/java/betaflight/configurator/MainActivity.java(1 hunks)android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)capacitor-plugin-socket/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/src/main/java/betaflight/configurator/MainActivity.java
- capacitor-plugin-socket/package.json
🧰 Additional context used
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
102-113: Safe disconnect & state reset is correct
Thedisconnectmethod cleanly closes resources and flipsisConnected. Good use of the helper.
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
100-115: Consider non-blocking read alternatives.The
readLine()call will block indefinitely if no newline character is received, which could lead to resource leaks if the connection stays idle.Consider implementing a timeout mechanism or alternative read approach:
getBridge().getExecutor().execute(() -> { try { - String data = reader.readLine(); + // Option 1: Use socket timeout already set + String data = reader.readLine(); + if (data == null) { + call.reject("Connection closed by server"); + closeResources(); + isConnected = false; + return; + } // Option 2: Alternative - read with fixed buffer size // char[] buffer = new char[8192]; // int length = reader.read(buffer, 0, buffer.length); // String data = new String(buffer, 0, length); JSObject ret = new JSObject(); ret.put("data", data); call.resolve(ret); } catch (Exception e) { closeResources(); isConnected = false; call.reject("Receive failed: " + e.getMessage()); } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 48-48: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (6)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (6)
1-16: Well-structured package and imports with clear documentation.The package structure and imports are appropriate for a Capacitor plugin. The class documentation clearly explains the plugin's purpose, which is good practice.
17-22: Good class structure with proper field initialization.The class is correctly annotated with
@CapacitorPluginand extendsPluginas required. The instance variables for socket, reader, writer, and connection state are appropriately defined.
64-98: Well-designed send method with thread safety.The send method includes proper input validation, connection state checking, and runs write operations on a background thread. The synchronization on the writer prevents concurrent modification issues, and appending a newline for framing is appropriate for text-based protocols.
117-128: Well-implemented disconnect method.The disconnect method properly cleans up resources and maintains state consistency. The error handling is appropriate.
130-152: Well-designed resource cleanup helper.The
closeResourcesmethod thoroughly cleans up all open resources in the correct order, nullifies references, and handles exceptions properly. Using the UI thread for logging is appropriate.
24-62:❓ Verification inconclusive
Well-implemented connect method with security considerations.
The connect method has proper input validation, duplicate connection prevention, and runs network operations on a background thread. Setting a timeout is a good practice.
However, the socket communication is not encrypted, which could be a security concern if sensitive data is transmitted.
Consider using an
SSLSocketfor secure communication if sensitive data will be transmitted. If encryption is not required for your specific use case, add a comment explaining why.
🏁 Script executed:
#!/bin/bash # Check if this app handles sensitive data elsewhere that might require encryption rg -A 2 "password|token|credential|api_key" --type javaLength of output: 57
Verify encryption requirements for socket communication
No references to passwords, tokens, credentials, or API keys were found in the Java codebase. If you are confident that only non-sensitive data is sent over this socket, please add a comment in SocketPlugin.java explaining why unencrypted communication is acceptable. If there is any chance you’ll transmit credentials or other sensitive information in the future, switch to an SSLSocket to ensure data is encrypted in transit.
• File to update:
- android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (in the connect method)
🧰 Tools
🪛 ast-grep (0.31.1)
[info] 48-48: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
110-118: 🛠️ Refactor suggestionInconsistent error handling in receive method.
Unlike the
sendmethod, thereceivemethod doesn't clean up resources or update connection state when an error occurs. This could lead to resource leaks and inconsistent state.} catch (Exception e) { + closeResources(); + isConnected = false; call.reject("Receive failed: " + e.getMessage()); }
🧹 Nitpick comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
3-3: Unused import that should be removed.The
android.util.Logimport is currently only used in thecloseResourcesmethod for error logging. If that's the only usage, it's fine to keep, but if you're not going to use it elsewhere, consider using a more Capacitor-specific logging approach.-import android.util.Log;
122-132: Consider running disconnect on a background thread.Although disconnect operations are typically quick, they can occasionally block. For consistency with other methods and to avoid potential UI freezes, consider running the disconnect operation on a background thread.
@PluginMethod public void disconnect(PluginCall call) { - try { - closeResources(); - isConnected = false; - JSObject ret = new JSObject(); - ret.put("success", true); - call.resolve(ret); - } catch (Exception e) { - call.reject("Disconnect failed: " + e.getMessage()); - } + getBridge().getExecutor().execute(() -> { + try { + closeResources(); + isConnected = false; + JSObject ret = new JSObject(); + ret.put("success", true); + call.resolve(ret); + } catch (Exception e) { + call.reject("Disconnect failed: " + e.getMessage()); + } + }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 48-48: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
1-157: Overall implementation looks solid with good error handling and background threading.The plugin follows Capacitor's conventions correctly and implements proper validation, background threading, and resource management. The synchronization for writes is a good practice to prevent race conditions. The code has addressed previous review comments effectively.
🧰 Tools
🪛 ast-grep (0.31.1)
[info] 48-48: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
46-61:❓ Verification inconclusive
Consider using encrypted socket for sensitive communications.
The current implementation uses a plain
Socketwhich transmits data in cleartext. If this plugin will handle sensitive information, consider usingSSLSocketinstead.
🏁 Script executed:
#!/bin/bash # Check if this app transmits sensitive information that would require encryption # Look for any existing SSL/TLS usage in the codebase rg -A 2 "SSLSocket|SSLSocketFactory|TLS|SSL" --type javaLength of output: 58
Evaluate data sensitivity and add TLS if needed
I ran a search across the Java code and didn’t find any existing use of SSLSocket, SSLSocketFactory, TLS, or SSL. The current implementation in
SocketPlugin.java(lines 46–61) uses a plainSocket, which sends everything in cleartext.If this plugin ever handles sensitive or security-critical data (e.g. credentials, configuration parameters that must be protected, or commands over untrusted networks), please switch to an encrypted socket (for example, via
SSLSocketFactory/SSLContext) and validate certificates to ensure confidentiality and integrity.• Affected file:
• android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (lines 46–61)🧰 Tools
🪛 ast-grep (0.31.1)
[info] 48-48: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
122-124: 🛠️ Refactor suggestionEnsure resources are cleaned up on receive failure
Mirror thesenderror path by closing resources and resettingisConnectedwhenreceivethrows an exception:- } catch (Exception e) { - call.reject("Receive failed: " + e.getMessage()); - } + } catch (Exception e) { + closeResources(); + isConnected = false; + call.reject("Receive failed: " + e.getMessage()); + }
🧹 Nitpick comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
49-50: Consider supporting encrypted connections
The plugin currently uses an unencryptedSocketfor raw TCP. For production or sensitive data, consider allowing anSSLSocketviaSSLSocketFactory, or clearly document that this is intended only for trusted networks.
159-163: Use a static TAG constant for logging and guard against null activity
Extract the log tag intoprivate static final String TAG = "SocketPlugin"and checkgetContext()/getActivity()beforerunOnUiThreadto avoid possible NPEs:+ private static final String TAG = "SocketPlugin"; ... - getContext().getActivity().runOnUiThread(() -> - Log.e("SocketPlugin", "Error closing resources", e)); + if (getContext() != null && getContext().getActivity() != null) { + getContext().getActivity().runOnUiThread(() -> + Log.e(TAG, "Error closing resources", e)); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 48-48: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (4)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (4)
53-60: Verify thread‐safety of call.resolve()/call.reject()
Resolving or rejectingPluginCallinstances from a background executor may have threading implications within the Capacitor runtime. Confirm these calls are safe off the main thread, or alternatively dispatch them back to the UI thread:getBridge().getExecutor().execute(() -> { // ... call.resolve(ret); // or getActivity().runOnUiThread(() -> call.resolve(ret)); });
65-72: Solid input validation insend
The null-check fordatacorrectly guards against missing payloads before attempting to write. Nice work.
80-88: Good use of background thread and synchronization insend
Offloading the write and flush to the executor and synchronizing onwriterprevents UI blocking and race conditions.
129-136: Effective cleanup and state reset indisconnect
CallingcloseResources()and resetting theisConnectedflag ensures no sockets or streams linger after disconnect.
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
1-166: 💡 Verification agent🧩 Analysis chain
Consider using encrypted socket communication for sensitive data.
The current implementation uses an unencrypted Socket, which could allow traffic interception by malicious actors.
🌐 Web query:
When should I use SSLSocket instead of Socket in Java?💡 Result:
You should use SSLSocket instead of Socket in Java when transferring sensitive data over insecure communication channels. The SSLSocket class provides critical security protections that regular Socket does not offer, making it essential for secure communications.
Security Protections Provided by SSLSocket
SSLSocket extends the basic Socket class but adds several important security layers through protocols like Secure Sockets Layer (SSL) and Transport Layer Security (TLS)[1][2]. These additional protections include:
Integrity Protection: SSL prevents modification of messages by active attackers, ensuring that data cannot be tampered with during transmission[1].
Authentication: SSLSocket provides peer authentication capabilities. In most implementations, servers are always authenticated, and clients can be authenticated when requested by servers[1][4].
Confidentiality: SSLSocket encrypts data being sent between client and server, protecting sensitive information such as financial data or personal information from eavesdropping by passive wiretappers[1].
Specific Use Cases
You should use SSLSocket rather than Socket in the following scenarios:
When handling sensitive data: Any application that deals with personal information, financial data, login credentials, or other confidential information[1].
For secure Remote Method Invocation (RMI): When using Java RMI, which depends on object serialization, serialized data must be protected in transit[1].
When communicating across untrusted networks: Any data transmitted over public networks like the internet should use secure sockets[1][5].
For compliance requirements: Many regulatory standards require secure communications for sensitive data transfers[1].
Implementation Comparison
Implementing SSLSocket involves a few more steps than using a regular Socket, but the security benefits are substantial. A basic SSLSocket implementation requires:
- Initializing an SSLContext
- Creating an SSLSocketFactory from the context
- Using the factory to create an SSLSocket
- Starting the SSL handshake process[5][6]
For example, client-side implementation requires:
// Initialize SSLContext SSLContext sslContext = SSLContext.getInstance("TLS"); sslContext.init(null, trustManagerFactory.getTrustManagers(), null); // Create SSLSocket SSLSocketFactory sslSocketFactory = sslContext.getSocketFactory(); SSLSocket sslSocket = (SSLSocket) sslSocketFactory.createSocket("localhost", 8443); // Start SSL handshake sslSocket.startHandshake();While SSLSocket adds security, it's worth noting that it may introduce some performance overhead compared to plain Socket connections[3]. However, this trade-off is necessary and justified when security is a requirement.
Citations:
- 1: https://wiki.sei.cmu.edu/confluence/display/java/MSC00-J.+Use+SSLSocket+rather+than+Socket+for+secure+data+exchange
- 2: https://docs.oracle.com/cd/E17802_01/j2se/j2se/1.5.0/jcp/beta1/apidiffs/javax/net/ssl/SSLSocket.html
- 3: https://forums.oracle.com/ords/apexds/post/ssl-socket-slowness-3572
- 4: https://learn.microsoft.com/en-us/dotnet/api/javax.net.ssl.sslsocket?view=net-android-34.0
- 5: https://qwiet.ai/appsec-resources/secure-your-java-applications-with-jsse-a-comprehensive-guide-to-java-secure-socket-extension/
- 6: https://snyk.io/blog/implementing-tls-in-java/
- 7: https://discourse.igniterealtime.org/t/rfe-upgrade-socket-to-starttls-with-custom-sslsocketfactory-as-alternative/82089
- 8: https://coderanch.com/t/739044/java/Socket-Programming-Java-TLS
- 9: https://matthewdavis111.com/java/poke-ssl-test-java-certs/
- 10: https://coderanch.com/t/479423/java/Plain-socket-SSL
Switch to SSLSocket for secure communications
The plugin currently opens a plain
Socket, which sends all data—including potentially sensitive payloads—in clear text over the network. To prevent eavesdropping, tampering, and meet common compliance requirements, you should use Java’sSSLSocket(TLS) instead:• File: android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
• Method: connect(PluginCall)Example refactor for
connect(...):- socket = new Socket(ip, port); + // Initialize SSL context (e.g. TLSv1.2+) + SSLContext sslContext = SSLContext.getInstance("TLS"); + sslContext.init(null, /* custom TrustManagers if needed */ null, null); + SSLSocketFactory factory = sslContext.getSocketFactory(); + SSLSocket sslSocket = (SSLSocket) factory.createSocket(ip, port); + sslSocket.startHandshake(); + socket = sslSocket;Next steps:
- Configure a TrustManager/KeyManager if you need custom certificates or mutual auth.
- Verify hostname on the server certificate to avoid MITM attacks.
- Adjust timeouts and cipher suites as required by your security policy.
🧰 Tools
🪛 ast-grep (0.31.1)
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
🧹 Nitpick comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
86-88: Consider making newline behavior configurable.The current implementation always appends a newline character after the data, which assumes a line-based protocol. This might not be appropriate for all TCP protocols.
synchronized (writer) { - // Append newline for framing; adjust as needed for your protocol - writer.write(data); - writer.newLine(); + // Allow configurable newline behavior + boolean appendNewline = call.getBoolean("appendNewline", true); + writer.write(data); + if (appendNewline) { + writer.newLine(); + } writer.flush(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (3)
131-142: Consider executing disconnect on a background thread.While the disconnect implementation correctly cleans up resources, it runs on the calling thread which is likely the UI thread. Socket closure can be blocking.
@PluginMethod public void disconnect(PluginCall call) { - try { - closeResources(); - isConnected = false; - JSObject ret = new JSObject(); - ret.put("success", true); - call.resolve(ret); - } catch (Exception e) { - call.reject("Disconnect failed: " + e.getMessage()); - } + getBridge().getExecutor().execute(() -> { + try { + closeResources(); + isConnected = false; + JSObject ret = new JSObject(); + ret.put("success", true); + call.resolve(ret); + } catch (Exception e) { + call.reject("Disconnect failed: " + e.getMessage()); + } + }); }
144-167: Thorough resource cleanup implementation.The closeResources method properly:
- Closes all resources in the correct order
- Nullifies references to prevent memory leaks
- Updates connection state on failures
- Handles exceptions gracefully with logging
- Runs logging on the UI thread
Consider adding a writer.flush() before closing to ensure all buffered data is sent.
if (writer != null) { + writer.flush(); // Ensure all buffered data is sent writer.close(); writer = null; }
24-63:❓ Verification inconclusive
Consider using encrypted socket for sensitive data.
The implementation correctly validates inputs, prevents duplicate connections, and moves network operations to a background thread. The timeout setting is a good practice.
However, the code uses an unencrypted socket which could expose transmitted data to interception.
If your application transmits sensitive data, consider using SSLSocket instead:
- socket = new Socket(ip, port); + SSLSocketFactory factory = (SSLSocketFactory) SSLSocketFactory.getDefault(); + socket = (SSLSocket) factory.createSocket(ip, port);This would require adding these imports:
import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory;
🏁 Script executed:
#!/bin/bash # Description: Check if secure communications are required elsewhere in the app # Check for SSL/TLS usage rg -A 2 "SSLSocket|SSLSocketFactory|TrustManager|KeyStore" --type java # Check for sensitive data patterns rg -A 2 "password|credential|token|key|secret|auth" --type javaLength of output: 138
Verify encryption requirements for Socket connections
File: android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Lines: 24-63Ripgrep across the Java codebase didn’t detect any existing SSL/TLS usage (SSLSocketFactory, TrustManager, KeyStore, etc.) or common sensitive-data patterns (password, token, key, secret). Absence of evidence isn’t evidence of absence—if this plugin ever carries credentials or other sensitive information, you should consider switching to an SSLSocket:
- socket = new Socket(ip, port); + SSLSocketFactory factory = (SSLSocketFactory) SSLSocketFactory.getDefault(); + socket = (SSLSocket) factory.createSocket(ip, port);And add these imports:
import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory;Please verify whether encrypted communication is required for your use case.
🧰 Tools
🪛 ast-grep (0.31.1)
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (4)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (4)
1-16: Well-structured package declarations and class documentation.The class is properly organized with appropriate imports and clear documentation that explains the plugin's purpose. The Capacitor plugin annotation is correctly set up.
19-22: Good field declarations with proper connection state tracking.The class fields are well-defined, including the boolean flag to track connection state. This will help prevent operations on closed sockets.
65-99: Thread-safe send implementation with proper validation.The send method implementation is robust with:
- Input validation
- Connection state verification
- Background thread execution
- Synchronized writer access
- Protocol framing with newline
- Proper error handling and resource cleanup
This addresses all the concerns from previous reviews.
101-129: Excellent non-blocking receive implementation with peer disconnection handling.The receive method correctly:
- Validates connection state before proceeding
- Executes read operations on a background thread
- Handles the case when a peer closes the connection (null readLine return)
- Properly manages resources on exceptions
- Returns data in the correct format
This addresses all previous concerns about blocking reads and error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (3)
49-54: Raw TCP is sent in cleartext – evaluate TLS or document threat modelStatic analysis flags the plain
new Socket(ip, port)instantiation. If credentials or other sensitive data traverse this channel, consider upgrading toSSLSocketFactoryor at least documenting why encryption is unnecessary in your context (e.g. LAN-only diagnostics).🧰 Tools
🪛 ast-grep (0.38.1)
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
150-160:flush()in cleanup can block – usually unnecessary beforeclose()
BufferedWriter.close()already performs an implicit flush. Callingflush()first can add latency or block during error paths (e.g. lost network). You can safely drop it and rely onclose().
86-88: Assumed protocol framing – maybe make configurableAppending
newLine()hard-codes a text-based framing scheme. If binary protocols or CRLF expectations arise, consider exposing a boolean/enum option (e.g.terminator) so callers can pick the delimiter, or document the newline requirement clearly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
30-40: Input validation looks solidThe guard clauses for IP emptiness and port range prevent common misuse and avoid
NullPointerExceptionfrom unboxing. Nicely done.
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Outdated
Show resolved
Hide resolved
haslinghuis
force-pushed
the
capacitor-plugin-socket
branch
from
30e43e2 to
6e8146c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (3)
25-63: Consider encrypted transport or document security implications
Theconnectmethod creates a rawSocket, which is unencrypted by default (static analysis flagged CWE-319). If you intend to transmit sensitive information, integrateSSLSocketFactoryto enable TLS or clearly document that this plugin provides unauthenticated plain-text TCP. Providing an option to configure SSL/TLS will make this plugin more versatile and secure.🧰 Tools
🪛 ast-grep (0.38.1)
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures(unencrypted-socket-java)
105-133: Capturereaderlocally to avoid race conditions
You checkreader != nullbefore scheduling the background task, but you don't capture it inside the executor. Ifdisconnectraces in before the task runs,readermay be null and trigger an NPE. Mirror thesendapproach by doing:BufferedReader localReader = reader; if (localReader == null) { call.reject("Connection lost"); return; } String data = localReader.readLine();
153-174:closeResourcescleanup looks robust
The helper neatly closes streams, nullifies references, logs errors on the UI thread, and resetsisConnected. As a minor nitpick, you can remove the explicitwriter.flush()—close()will flush internally—or separate its exception handling if you want to log flush-specific failures.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket(ip, port)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (2)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (2)
65-103:sendmethod correctly offloads work and guards state
Great use ofgetBridge().getExecutor()to avoid ANRs, input validation, capture ofwriter, synchronized writes, and cleanup on error. Just verify in your Capacitor version that invokingcall.resolve/call.rejectfrom a background thread is safe.
135-147: Verify background-thread context fordisconnectcallbacks
disconnectoffloads cleanup to the executor, which is good, but now thecall.resolvefires off the background thread. Ensure that downstream JavaScript consumers don’t rely on callbacks coming from the UI thread or expect immediate synchronous cleanup.
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
Show resolved
Hide resolved
|
@coderabbitio We are stuck in an endless review loop here. All changes made are made on your requests, but keep getting more requests after that for the same file. Please provide a full refactor and working solution for android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java. |
|
Tip For best results, initiate chat on the files or code changes. |
|
A summary of the edits CodeRabbit can apply:
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets): ✏️ android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.javaReplace the entire contents of SocketPlugin.java with the following improved implementation: package betaflight.configurator.plugin;
import android.util.Log;
import com.getcapacitor.JSObject;
import com.getcapacitor.Plugin;
import com.getcapacitor.PluginCall;
import com.getcapacitor.PluginMethod;
import com.getcapacitor.annotation.CapacitorPlugin;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.ReentrantLock;
/**
* Capacitor plugin that provides raw TCP socket functionality with thread safety,
* robust resource management, and comprehensive error handling.
*/
@CapacitorPlugin(name = "SocketPlugin")
public class SocketPlugin extends Plugin {
private static final String TAG = "SocketPlugin";
// Error messages
private static final String ERROR_IP_REQUIRED = "IP address is required";
private static final String ERROR_INVALID_PORT = "Invalid port number";
private static final String ERROR_ALREADY_CONNECTED = "Already connected; please disconnect first";
private static final String ERROR_NOT_CONNECTED = "Not connected to any server";
private static final String ERROR_DATA_REQUIRED = "Data is required";
private static final String ERROR_CONNECTION_LOST = "Connection lost";
private static final String ERROR_CONNECTION_CLOSED = "Connection closed by peer";
// Connection settings
private static final int DEFAULT_TIMEOUT_MS = 30_000;
private static final int MIN_PORT = 1;
private static final int MAX_PORT = 65535;
private enum ConnectionState {
DISCONNECTED,
CONNECTING,
CONNECTED,
DISCONNECTING,
ERROR
}
// Thread-safe state and locks
private final AtomicReference<ConnectionState> state = new AtomicReference<>(ConnectionState.DISCONNECTED);
private final ReentrantLock socketLock = new ReentrantLock();
private final ReentrantLock writerLock = new ReentrantLock();
private Socket socket;
private BufferedReader reader;
private BufferedWriter writer;
@PluginMethod
public void connect(final PluginCall call) {
call.setKeepAlive(true);
String ip = call.getString("ip");
int port = call.getInt("port", -1);
if (ip == null || ip.isEmpty()) {
call.reject(ERROR_IP_REQUIRED);
call.setKeepAlive(false);
return;
}
if (port < MIN_PORT || port > MAX_PORT) {
call.reject(ERROR_INVALID_PORT);
call.setKeepAlive(false);
return;
}
if (!compareAndSetState(ConnectionState.DISCONNECTED, ConnectionState.CONNECTING)) {
call.reject(ERROR_ALREADY_CONNECTED);
call.setKeepAlive(false);
return;
}
getBridge().getExecutor().execute(() -> {
socketLock.lock();
try {
socket = new Socket();
socket.connect(new InetSocketAddress(ip, port), DEFAULT_TIMEOUT_MS);
socket.setSoTimeout(DEFAULT_TIMEOUT_MS);
reader = new BufferedReader(new InputStreamReader(socket.getInputStream()));
writer = new BufferedWriter(new OutputStreamWriter(socket.getOutputStream()));
state.set(ConnectionState.CONNECTED);
JSObject result = new JSObject();
result.put("success", true);
call.resolve(result);
Log.d(TAG, "Connected to " + ip + ":" + port);
} catch (Exception e) {
state.set(ConnectionState.ERROR);
closeResourcesInternal();
call.reject("Connection failed: " + e.getMessage());
Log.e(TAG, "Connection failed", e);
} finally {
socketLock.unlock();
call.setKeepAlive(false);
}
});
}
@PluginMethod
public void send(final PluginCall call) {
String data = call.getString("data");
if (data == null || data.isEmpty()) {
call.reject(ERROR_DATA_REQUIRED);
return;
}
if (state.get() != ConnectionState.CONNECTED) {
call.reject(ERROR_NOT_CONNECTED);
return;
}
call.setKeepAlive(true);
getBridge().getExecutor().execute(() -> {
writerLock.lock();
try {
if (writer == null || state.get() != ConnectionState.CONNECTED) {
call.reject(ERROR_CONNECTION_LOST);
return;
}
writer.write(data);
writer.newLine();
writer.flush();
JSObject result = new JSObject();
result.put("success", true);
call.resolve(result);
Log.d(TAG, "Sent data: " + truncateForLog(data));
} catch (Exception e) {
handleCommunicationError(e, "Send failed", call);
} finally {
writerLock.unlock();
call.setKeepAlive(false);
}
});
}
@PluginMethod
public void receive(final PluginCall call) {
if (state.get() != ConnectionState.CONNECTED || reader == null) {
call.reject(ERROR_NOT_CONNECTED);
return;
}
call.setKeepAlive(true);
getBridge().getExecutor().execute(() -> {
try {
String data = reader.readLine();
if (data == null) {
handleCommunicationError(new IOException("End of stream"), ERROR_CONNECTION_CLOSED, call);
return;
}
JSObject result = new JSObject();
result.put("data", data);
call.resolve(result);
Log.d(TAG, "Received data: " + truncateForLog(data));
} catch (Exception e) {
handleCommunicationError(e, "Receive failed", call);
} finally {
call.setKeepAlive(false);
}
});
}
@PluginMethod
public void disconnect(final PluginCall call) {
ConnectionState current = state.get();
if (current == ConnectionState.DISCONNECTED) {
JSObject result = new JSObject();
result.put("success", true);
call.resolve(result);
return;
}
if (!compareAndSetState(current, ConnectionState.DISCONNECTING)) {
call.reject("Invalid state for disconnect: " + current);
return;
}
call.setKeepAlive(true);
getBridge().getExecutor().execute(() -> {
socketLock.lock();
try {
closeResourcesInternal();
state.set(ConnectionState.DISCONNECTED);
JSObject result = new JSObject();
result.put("success", true);
call.resolve(result);
Log.d(TAG, "Disconnected successfully");
} catch (Exception e) {
state.set(ConnectionState.ERROR);
call.reject("Disconnect failed: " + e.getMessage());
Log.e(TAG, "Disconnect failed", e);
} finally {
socketLock.unlock();
call.setKeepAlive(false);
}
});
}
@Override
protected void handleOnDestroy() {
socketLock.lock();
try {
state.set(ConnectionState.DISCONNECTING);
closeResourcesInternal();
state.set(ConnectionState.DISCONNECTED);
} catch (Exception e) {
Log.e(TAG, "Error cleaning up resources on destroy", e);
} finally {
socketLock.unlock();
}
super.handleOnDestroy();
}
private void closeResourcesInternal() {
if (reader != null) {
try { reader.close(); } catch (IOException e) { Log.e(TAG, "Error closing reader", e);} finally { reader = null; }
}
if (writer != null) {
try { writer.flush(); writer.close(); } catch (IOException e) { Log.e(TAG, "Error closing writer", e);} finally { writer = null; }
}
if (socket != null) {
try { socket.close(); } catch (IOException e) { Log.e(TAG, "Error closing socket", e);} finally { socket = null; }
}
}
private void handleCommunicationError(Exception error, String message, PluginCall call) {
socketLock.lock();
try {
state.set(ConnectionState.ERROR);
closeResourcesInternal();
state.set(ConnectionState.DISCONNECTED);
call.reject(message + ": " + error.getMessage());
Log.e(TAG, message, error);
} finally {
socketLock.unlock();
}
}
private boolean compareAndSetState(ConnectionState expected, ConnectionState newState) {
return state.compareAndSet(expected, newState);
}
private String truncateForLog(String data) {
if (data == null) return "null";
final int maxLen = 100;
if (data.length() <= maxLen) return data;
return data.substring(0, maxLen) + "... (" + data.length() + " chars)";
}
}Build the Android project, deploy the Capacitor app to a device or emulator, and exercise the SocketPlugin by calling its connect, send, receive, and disconnect methods. Verify that operations succeed, errors are handled gracefully, logs are produced as expected, and resources are cleaned up on disconnect or app termination. A summary of the context that CodeRabbit is considering across the codebase:
|
|
Caution CodeRabbit has failed to create a local commit with your edits. |
1 similar comment
|
Caution CodeRabbit has failed to create a local commit with your edits. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (1)
221-231: Consider separating resource closing into individual methods.While the current implementation works well, having individual methods for closing each resource type (reader, writer, socket) could improve readability and maintainability.
- private void closeResourcesInternal() { - if (reader != null) { - try { reader.close(); } catch (IOException e) { Log.e(TAG, "Error closing reader", e);} finally { reader = null; } - } - if (writer != null) { - try { writer.flush(); writer.close(); } catch (IOException e) { Log.e(TAG, "Error closing writer", e);} finally { writer = null; } - } - if (socket != null) { - try { socket.close(); } catch (IOException e) { Log.e(TAG, "Error closing socket", e);} finally { socket = null; } - } - } + private void closeResourcesInternal() { + closeReader(); + closeWriter(); + closeSocket(); + } + + private void closeReader() { + if (reader != null) { + try { + reader.close(); + } catch (IOException e) { + Log.e(TAG, "Error closing reader", e); + } finally { + reader = null; + } + } + } + + private void closeWriter() { + if (writer != null) { + try { + writer.flush(); + writer.close(); + } catch (IOException e) { + Log.e(TAG, "Error closing writer", e); + } finally { + writer = null; + } + } + } + + private void closeSocket() { + if (socket != null) { + try { + socket.close(); + } catch (IOException e) { + Log.e(TAG, "Error closing socket", e); + } finally { + socket = null; + } + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.1)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java
[info] 82-82: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (9)
android/app/src/main/java/betaflight/configurator/plugin/SocketPlugin.java (9)
19-24: Well-designed Capacitor plugin structure with clear documentation.The class is properly annotated with
@CapacitorPluginand has clear documentation explaining its purpose and features. The plugin name matches the class name, making it consistent and easy to understand.
27-40: Excellent use of constants for error messages and settings.Using string constants for error messages enhances maintainability and consistency throughout the codebase. The connection settings constants are also well-defined with appropriate names and values.
41-56: Good use of enum and thread-safe fields.The
ConnectionStateenum provides a clear state model, and the thread-safety approach usingAtomicReferenceandReentrantLockis appropriate for a multi-threaded environment where socket operations run on background threads.
107-142: Good implementation of asynchronous send operation with thread safety.The method correctly validates input, checks connection state, and performs I/O operations asynchronously on a background thread with proper synchronization and error handling. The use of
truncateForLogis a nice touch to prevent log pollution.
144-169: Appropriately handling null response in receive method.The receive method correctly detects when the connection is closed by the peer (when
readLine()returns null) and handles this case properly by transitioning to an error state and cleaning up resources.
171-204: Robust disconnect implementation with proper state management.The disconnect method is well-implemented with state validation, thread safety, and proper resource cleanup. It also handles the case where the socket is already disconnected, avoiding unnecessary operations.
206-219: Excellent resource cleanup in handleOnDestroy.The override of
handleOnDestroyensures that resources are properly cleaned up when the plugin is destroyed, preventing resource leaks. The use of locks ensures that cleanup operations don't conflict with ongoing operations.
233-244: Good centralized error handling approach.The
handleCommunicationErrormethod centralizes error handling logic, making the code more maintainable and consistent. It appropriately transitions state, cleans up resources, and logs errors.
246-255: Well-implemented helper methods.The helper methods for state comparison and log truncation are well-implemented. The
truncateForLogmethod is a particularly nice touch that prevents log pollution when dealing with large data payloads.
android/app/src/main/java/betaflight/configurator/protocols/tcp/BetaflightTcpPlugin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/js/protocols/CapacitorTcp.js (3)
130-130: Optional: Consider using TypeError for consistency.While
new Error()is acceptable for invalid port parsing, usingnew TypeError()would be more semantically accurate for type/format validation errors. This is a stylistic preference and not critical.
150-165: Remove redundant state assignment.Line 154 sets
this.connected = false, and line 162 sets it again unconditionally. The duplicate assignment is unnecessary.Apply this diff to consolidate state updates:
async disconnect() { try { const res = await this.plugin.disconnect(); - if (res.success) { - this.connected = false; - } this.dispatchEvent(new CustomEvent("disconnect", { detail: true })); } catch (e) { console.error(`${this.logHead}Failed to close connection: ${e}`); this.dispatchEvent(new CustomEvent("disconnect", { detail: false })); } this.connected = false; this.bytesReceived = 0; this.bytesSent = 0; }
197-199: Clarify or remove the inconsistent return value.The method returns
{ bytesSent: 0 }after both successful sends (where bytes were actually sent) and when not connected. This is inconsistent with the callback-based reporting (lines 177-180) and may confuse callers.If the return value is unused (callers rely on the callback), consider removing it or documenting the behavior. Otherwise, return accurate values:
async send(data, cb) { if (this.connected) { const bytes = normalizeToUint8Array(data); + let sentBytes = 0; try { const payload = uint8ArrayToBase64(bytes); const res = await this.plugin.send({ data: payload }); if (res.success) { - this.bytesSent += bytes.byteLength; + sentBytes = bytes.byteLength; + this.bytesSent += sentBytes; if (cb) { cb({ error: null, - bytesSent: bytes.byteLength, + bytesSent: sentBytes, }); } } else { throw new Error("Send failed"); } } catch (e) { console.error(`${this.logHead}Failed to send data e: ${e}`); if (cb) { cb({ error: e, bytesSent: 0, }); } } + return { bytesSent: sentBytes }; } return { bytesSent: 0, }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/CapacitorTcp.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: In betaflight-configurator’s CapacitorBluetooth.js, native plugin listeners (notification, services, connectionState) must be attached exactly once per app lifetime. Re-attaching them during writes/connect cycles leads to duplicate handlers, increased JS workload, MSP queue pressure, and can prevent the MSP_EEPROM_WRITE callback from firing (so reinitializeConnection is not reached). Implement an idempotent _attachNativeListenersOnce() guard.
Applied to files:
src/js/protocols/CapacitorTcp.js
🪛 GitHub Check: SonarCloud Code Analysis
src/js/protocols/CapacitorTcp.js
[warning] 143-143: Complete the task associated to this "TODO" comment.
[warning] 21-21: Prefer String.fromCodePoint() over String.fromCharCode().
[warning] 130-130: new Error() is too unspecific for a type check. Use new TypeError() instead.
[warning] 13-13: Prefer String#codePointAt() over String#charCodeAt().
⏰ 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). (1)
- GitHub Check: deploy
🔇 Additional comments (6)
src/js/protocols/CapacitorTcp.js (6)
1-3: LGTM!The plugin initialization with optional chaining is a safe pattern.
5-16: LGTM! Static analysis warning is a false positive.The use of
charCodeAtis correct for binary data. The static analysis suggestion to usecodePointAtwould introduce data corruption, ascodePointAtcan return values greater than 255, which would overflow Uint8Array slots.
18-24: LGTM! Static analysis warning is a false positive.The use of
String.fromCharCodeis correct for binary data encoding. The static analysis suggestion to usefromCodePointwould be inappropriate for byte-level binary data.
26-40: LGTM!The normalization logic comprehensively handles various data types and throws an appropriate TypeError for unsupported inputs.
86-92: LGTM!The handler methods are simple and correctly update state or delegate to appropriate methods.
94-117: LGTM!The port management methods appropriately handle TCP connection metadata. Returning an empty array from
getDevicesis correct since TCP doesn't enumerate available devices.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/js/protocols/CapacitorTcp.js (1)
114-114: Use TypeError for invalid port validation.Static analysis correctly identifies that parameter validation errors should throw
TypeErrorrather than genericErrorfor better semantic clarity.Apply this diff:
- if (Number.isNaN(fallbackPort)) { - throw new Error(`Invalid port in path: ${path}`); - } + if (Number.isNaN(fallbackPort)) { + throw new TypeError(`Invalid port in path: ${path}`); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/CapacitorTcp.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: In betaflight-configurator’s CapacitorBluetooth.js, native plugin listeners (notification, services, connectionState) must be attached exactly once per app lifetime. Re-attaching them during writes/connect cycles leads to duplicate handlers, increased JS workload, MSP queue pressure, and can prevent the MSP_EEPROM_WRITE callback from firing (so reinitializeConnection is not reached). Implement an idempotent _attachNativeListenersOnce() guard.
Applied to files:
src/js/protocols/CapacitorTcp.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/CapacitorTcp.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/CapacitorTcp.js
📚 Learning: 2025-11-22T21:18:08.814Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:11-46
Timestamp: 2025-11-22T21:18:08.814Z
Learning: In betaflight-configurator, protocol implementations (WebSerial, CapacitorSerial, WebBluetooth, etc.) are instantiated once in the Serial class constructor and stored as singletons in the _protocols array for the entire application lifetime. They are never destroyed or recreated, so cleanup methods to remove event listeners are not needed.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-24T15:07:25.227Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:47-47
Timestamp: 2025-11-24T15:07:25.227Z
Learning: In betaflight-configurator's CapacitorSerial protocol (src/js/protocols/CapacitorSerial.js), the fire-and-forget this.loadDevices() call in the constructor is intentional background pre-loading. The actual critical device list loading for UI population is orchestrated by port_handler.js via serial.getDevices("serial"), which properly awaits CapacitorSerial.getDevices() → loadDevices() in a complete async chain. The constructor's async call does not cause race conditions.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: CapacitorBluetooth.connect should only dispatch the 'open' event after GATT services are resolved and notifications are started; announcing open earlier causes the first MSP write to fail with “Unable to write characteristic” and contributes to early queue load.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:41:59.374Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:135-143
Timestamp: 2025-11-28T22:41:59.374Z
Learning: In betaflight-configurator, WebSerial.js uses a constant path "serial" for all devices, and CapacitorBluetooth.js uses constant path "bluetooth", consistent with the single-device-at-a-time workflow. WebBluetooth.js uses counter-based paths (bluetooth_${counter}), but that's specific to its implementation. The path strategy varies by protocol based on their specific requirements.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:51:11.691Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:176-206
Timestamp: 2025-11-28T22:51:11.691Z
Learning: In betaflight-configurator, Web Bluetooth API's `services` option filters scan results to only devices advertising those UUIDs, while `optionalServices` requests permission to access services after connection without filtering. PWA (WebBluetooth.js) uses `{ acceptAllDevices: true, optionalServices: uuids }` to show all devices while requesting post-connection service access. CapacitorBluetooth.js should match this pattern rather than using `services` as a scan filter.
Applied to files:
src/js/protocols/CapacitorTcp.js
🪛 GitHub Check: SonarCloud Code Analysis
src/js/protocols/CapacitorTcp.js
[warning] 114-114: new Error() is too unspecific for a type check. Use new TypeError() instead.
[warning] 21-21: Prefer String.fromCodePoint() over String.fromCharCode().
[warning] 13-13: Prefer String#codePointAt() over String#charCodeAt().
[warning] 127-127: Complete the task associated to this "TODO" comment.
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/js/protocols/CapacitorTcp.js (3)
5-24: LGTM: Base64 conversion correctly handles binary data.The use of
charCodeAtandfromCharCodeis correct for binary data (byte values 0–255). The static analysis warnings suggestingcodePointAt/fromCodePointare false positives—those methods are designed for Unicode code points and would corrupt binary data. The inline comment at lines 10-12 explicitly documents this design choice.
26-68: LGTM: Constructor and event listeners correctly implemented.The constructor initializes state and attaches native plugin listeners once per application lifetime, consistent with the singleton pattern used for all protocol implementations in betaflight-configurator. The past review concern about duplicate handlers has been confirmed as resolved.
134-149: LGTM: Disconnect ensures cleanup regardless of errors.The method defensively resets state (lines 146-148) after the try-catch block, ensuring consistent cleanup even if the native plugin call fails. While line 138 sets
connected = falseon success and line 146 sets it again unconditionally, this redundancy is acceptable for robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/protocols/CapacitorTcp.js (1)
160-167: Critical: Update cumulativebytesSentcounter.The
sendmethod updatesactualBytesSentfor the return value but fails to update the cumulativethis.bytesSentcounter (initialized at line 34). This breaks statistics tracking—this.bytesSentwill remain 0 regardless of data sent, diverging from other protocol implementations (WebSerial, CapacitorSerial, CapacitorBluetooth) that maintain cumulative counters for telemetry.Apply this diff to update the cumulative counter:
if (res.success) { + this.bytesSent += bytes.byteLength; actualBytesSent = bytes.byteLength; if (cb) {
🧹 Nitpick comments (1)
src/js/protocols/CapacitorTcp.js (1)
114-114: UseTypeErrorfor type validation errors.Line 114 throws a generic
Errorfor an invalid port number, but since this is a type/value validation failure,TypeErrorwould be more semantically correct and improve error categorization.Apply this diff:
- throw new Error(`Invalid port in path: ${path}`); + throw new TypeError(`Invalid port in path: ${path}`);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/CapacitorTcp.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: In betaflight-configurator’s CapacitorBluetooth.js, native plugin listeners (notification, services, connectionState) must be attached exactly once per app lifetime. Re-attaching them during writes/connect cycles leads to duplicate handlers, increased JS workload, MSP queue pressure, and can prevent the MSP_EEPROM_WRITE callback from firing (so reinitializeConnection is not reached). Implement an idempotent _attachNativeListenersOnce() guard.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-24T15:07:25.227Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:47-47
Timestamp: 2025-11-24T15:07:25.227Z
Learning: In betaflight-configurator's CapacitorSerial protocol (src/js/protocols/CapacitorSerial.js), the fire-and-forget this.loadDevices() call in the constructor is intentional background pre-loading. The actual critical device list loading for UI population is orchestrated by port_handler.js via serial.getDevices("serial"), which properly awaits CapacitorSerial.getDevices() → loadDevices() in a complete async chain. The constructor's async call does not cause race conditions.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:41:59.374Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:135-143
Timestamp: 2025-11-28T22:41:59.374Z
Learning: In betaflight-configurator, WebSerial.js uses a constant path "serial" for all devices, and CapacitorBluetooth.js uses constant path "bluetooth", consistent with the single-device-at-a-time workflow. WebBluetooth.js uses counter-based paths (bluetooth_${counter}), but that's specific to its implementation. The path strategy varies by protocol based on their specific requirements.
Applied to files:
src/js/protocols/CapacitorTcp.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/CapacitorTcp.js
📚 Learning: 2025-11-22T21:18:08.814Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:11-46
Timestamp: 2025-11-22T21:18:08.814Z
Learning: In betaflight-configurator, protocol implementations (WebSerial, CapacitorSerial, WebBluetooth, etc.) are instantiated once in the Serial class constructor and stored as singletons in the _protocols array for the entire application lifetime. They are never destroyed or recreated, so cleanup methods to remove event listeners are not needed.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: CapacitorBluetooth.connect should only dispatch the 'open' event after GATT services are resolved and notifications are started; announcing open earlier causes the first MSP write to fail with “Unable to write characteristic” and contributes to early queue load.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:51:11.691Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:176-206
Timestamp: 2025-11-28T22:51:11.691Z
Learning: In betaflight-configurator, Web Bluetooth API's `services` option filters scan results to only devices advertising those UUIDs, while `optionalServices` requests permission to access services after connection without filtering. PWA (WebBluetooth.js) uses `{ acceptAllDevices: true, optionalServices: uuids }` to show all devices while requesting post-connection service access. CapacitorBluetooth.js should match this pattern rather than using `services` as a scan filter.
Applied to files:
src/js/protocols/CapacitorTcp.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/CapacitorTcp.js
🪛 GitHub Check: SonarCloud Code Analysis
src/js/protocols/CapacitorTcp.js
[warning] 127-127: Complete the task associated to this "TODO" comment.
[warning] 13-13: Prefer String#codePointAt() over String#charCodeAt().
[warning] 21-21: Prefer String.fromCodePoint() over String.fromCharCode().
[warning] 114-114: new Error() is too unspecific for a type check. Use new TypeError() instead.
⏰ 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). (1)
- GitHub Check: deploy
🔇 Additional comments (2)
src/js/protocols/CapacitorTcp.js (2)
5-16: LGTM: Correct use ofcharCodeAtfor binary data.The implementation correctly uses
charCodeAtto extract byte values (0-255) from the binary string returned byatob. The inline comment accurately explains whycodePointAtwould be incorrect here (it can return values > 255 for Unicode code points, corrupting the byte array). The SonarCloud warning suggestingcodePointAtis a false positive.
18-24: LGTM: Correct use offromCharCodefor binary data.The implementation correctly uses
String.fromCharCodeto build a binary string from byte values, which is then encoded withbtoa. UsingString.fromCodePoint(as suggested by SonarCloud) is unnecessary for byte values (0-255) and would add overhead without benefit. The SonarCloud warning is a false positive.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/protocols/CapacitorTcp.js (1)
143-177: Update the cumulative bytesSent counter on successful send.The
sendmethod correctly setsactualBytesSentand returns it, but fails to update the instance-levelthis.bytesSentcounter. This counter is initialized in the constructor (line 34) and is intended for cumulative statistics tracking, consistent with howthis.bytesReceivedis maintained (line 71). Without updating it, monitoring and statistics features will show incorrect transmission totals.Apply this diff:
if (res.success) { + this.bytesSent += bytes.byteLength; actualBytesSent = bytes.byteLength; if (cb) { cb({ error: null, bytesSent: actualBytesSent, }); }
🧹 Nitpick comments (2)
src/js/protocols/CapacitorTcp.js (2)
107-107: Consider simplifying the port parsing logic.The check
url.port.length ? parseInt(url.port, 10) : 80works correctly but is unconventional. A more idiomatic approach would beparseInt(url.port, 10) || 80, which handles both empty strings and non-numeric values gracefully.Apply this diff:
- const port = url.port.length ? parseInt(url.port, 10) : 80; + const port = parseInt(url.port, 10) || 80;
126-141: Minor redundancy in state reset logic.The
disconnectmethod resetsthis.connectedconditionally at line 130 whenres.successis true, but then unconditionally resets it again at line 138. The unconditional reset makes the conditional one redundant.This doesn't cause incorrect behavior, but streamlining would improve clarity.
Consider this refactor:
async disconnect() { try { - const res = await this.plugin.disconnect(); - if (res.success) { - this.connected = false; - } + await this.plugin.disconnect(); this.dispatchEvent(new CustomEvent("disconnect", { detail: true })); } catch (e) { console.error(`${this.logHead}Failed to close connection: ${e}`); this.dispatchEvent(new CustomEvent("disconnect", { detail: false })); } this.connected = false; this.bytesReceived = 0; this.bytesSent = 0; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/CapacitorTcp.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: In betaflight-configurator’s CapacitorBluetooth.js, native plugin listeners (notification, services, connectionState) must be attached exactly once per app lifetime. Re-attaching them during writes/connect cycles leads to duplicate handlers, increased JS workload, MSP queue pressure, and can prevent the MSP_EEPROM_WRITE callback from firing (so reinitializeConnection is not reached). Implement an idempotent _attachNativeListenersOnce() guard.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-24T15:07:25.227Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:47-47
Timestamp: 2025-11-24T15:07:25.227Z
Learning: In betaflight-configurator's CapacitorSerial protocol (src/js/protocols/CapacitorSerial.js), the fire-and-forget this.loadDevices() call in the constructor is intentional background pre-loading. The actual critical device list loading for UI population is orchestrated by port_handler.js via serial.getDevices("serial"), which properly awaits CapacitorSerial.getDevices() → loadDevices() in a complete async chain. The constructor's async call does not cause race conditions.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:41:59.374Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:135-143
Timestamp: 2025-11-28T22:41:59.374Z
Learning: In betaflight-configurator, WebSerial.js uses a constant path "serial" for all devices, and CapacitorBluetooth.js uses constant path "bluetooth", consistent with the single-device-at-a-time workflow. WebBluetooth.js uses counter-based paths (bluetooth_${counter}), but that's specific to its implementation. The path strategy varies by protocol based on their specific requirements.
Applied to files:
src/js/protocols/CapacitorTcp.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/CapacitorTcp.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/CapacitorTcp.js
📚 Learning: 2025-11-22T21:18:08.814Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:11-46
Timestamp: 2025-11-22T21:18:08.814Z
Learning: In betaflight-configurator, protocol implementations (WebSerial, CapacitorSerial, WebBluetooth, etc.) are instantiated once in the Serial class constructor and stored as singletons in the _protocols array for the entire application lifetime. They are never destroyed or recreated, so cleanup methods to remove event listeners are not needed.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: CapacitorBluetooth.connect should only dispatch the 'open' event after GATT services are resolved and notifications are started; announcing open earlier causes the first MSP write to fail with “Unable to write characteristic” and contributes to early queue load.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:51:11.691Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:176-206
Timestamp: 2025-11-28T22:51:11.691Z
Learning: In betaflight-configurator, Web Bluetooth API's `services` option filters scan results to only devices advertising those UUIDs, while `optionalServices` requests permission to access services after connection without filtering. PWA (WebBluetooth.js) uses `{ acceptAllDevices: true, optionalServices: uuids }` to show all devices while requesting post-connection service access. CapacitorBluetooth.js should match this pattern rather than using `services` as a scan filter.
Applied to files:
src/js/protocols/CapacitorTcp.js
🔇 Additional comments (3)
src/js/protocols/CapacitorTcp.js (3)
5-24: LGTM!The base64 encoding/decoding utilities are correctly implemented. The explicit comment explaining the use of
charCodeAtovercodePointAtdemonstrates awareness of byte-level data handling requirements.
27-68: LGTM!The constructor correctly initializes state and attaches plugin listeners. Per the established pattern in this codebase,
CapacitorTcpis instantiated once as a singleton in theSerialclass, so listeners are attached exactly once per application lifetime. The implementation is consistent with other protocol implementations likeCapacitorSerialandCapacitorBluetooth.
78-101: LGTM!The port management methods follow the established protocol interface pattern. Returning an empty array from
getDevices()is appropriate for TCP, which doesn't enumerate devices like serial ports do.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/js/protocols/CapacitorTcp.js (1)
103-108: Optional: switch toNumber.parseIntto align with Sonar style guidance.
parseInt(url.port, 10)is perfectly valid here; if you want to satisfy the Sonar warning, you can make this purely stylistic change:- const port = url.port.length ? parseInt(url.port, 10) : 5761; + const port = url.port.length ? Number.parseInt(url.port, 10) : 5761;No behavioral change, just lints happier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/CapacitorTcp.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: In betaflight-configurator’s CapacitorBluetooth.js, native plugin listeners (notification, services, connectionState) must be attached exactly once per app lifetime. Re-attaching them during writes/connect cycles leads to duplicate handlers, increased JS workload, MSP queue pressure, and can prevent the MSP_EEPROM_WRITE callback from firing (so reinitializeConnection is not reached). Implement an idempotent _attachNativeListenersOnce() guard.
Applied to files:
src/js/protocols/CapacitorTcp.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/CapacitorTcp.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/CapacitorTcp.js
📚 Learning: 2025-11-22T21:18:08.814Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:11-46
Timestamp: 2025-11-22T21:18:08.814Z
Learning: In betaflight-configurator, protocol implementations (WebSerial, CapacitorSerial, WebBluetooth, etc.) are instantiated once in the Serial class constructor and stored as singletons in the _protocols array for the entire application lifetime. They are never destroyed or recreated, so cleanup methods to remove event listeners are not needed.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-24T15:07:25.227Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:47-47
Timestamp: 2025-11-24T15:07:25.227Z
Learning: In betaflight-configurator's CapacitorSerial protocol (src/js/protocols/CapacitorSerial.js), the fire-and-forget this.loadDevices() call in the constructor is intentional background pre-loading. The actual critical device list loading for UI population is orchestrated by port_handler.js via serial.getDevices("serial"), which properly awaits CapacitorSerial.getDevices() → loadDevices() in a complete async chain. The constructor's async call does not cause race conditions.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: CapacitorBluetooth.connect should only dispatch the 'open' event after GATT services are resolved and notifications are started; announcing open earlier causes the first MSP write to fail with “Unable to write characteristic” and contributes to early queue load.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:51:11.691Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:176-206
Timestamp: 2025-11-28T22:51:11.691Z
Learning: In betaflight-configurator, Web Bluetooth API's `services` option filters scan results to only devices advertising those UUIDs, while `optionalServices` requests permission to access services after connection without filtering. PWA (WebBluetooth.js) uses `{ acceptAllDevices: true, optionalServices: uuids }` to show all devices while requesting post-connection service access. CapacitorBluetooth.js should match this pattern rather than using `services` as a scan filter.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:41:59.374Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:135-143
Timestamp: 2025-11-28T22:41:59.374Z
Learning: In betaflight-configurator, WebSerial.js uses a constant path "serial" for all devices, and CapacitorBluetooth.js uses constant path "bluetooth", consistent with the single-device-at-a-time workflow. WebBluetooth.js uses counter-based paths (bluetooth_${counter}), but that's specific to its implementation. The path strategy varies by protocol based on their specific requirements.
Applied to files:
src/js/protocols/CapacitorTcp.js
🪛 GitHub Check: SonarCloud Code Analysis
src/js/protocols/CapacitorTcp.js
[warning] 21-21: Prefer String.fromCodePoint() over String.fromCharCode().
[warning] 107-107: Prefer Number.parseInt over parseInt.
[warning] 118-118: Complete the task associated to this "TODO" comment.
[warning] 13-13: Prefer String#codePointAt() over String#charCodeAt().
⏰ 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). (1)
- GitHub Check: deploy
🔇 Additional comments (2)
src/js/protocols/CapacitorTcp.js (2)
5-24: Binary Base64 helpers correctly usecharCodeAt/fromCharCode(Sonar warning is a false positive).For
atob/btoabinary strings,charCodeAtandString.fromCharCodeare the correct byte-level APIs.codePointAt/fromCodePointwork on Unicode code points and can yield values >255, which would corrupt binary data when writing into aUint8Arrayor encoding to Base64. I’d keep the current implementation and mark the Sonar warnings on lines 13 and 21 as false positives rather than changing this.
26-68: Constructor and plugin listener wiring look correct for the singleton protocol pattern.State initialization, plugin availability check, and mapping of
dataReceived/dataReceivedError/connectionClosedinto internal counters andreceive/disconnect flows are consistent and side‑effect free. GivenCapacitorTcpis instantiated once and stored inSerial._protocols, listener attachment in the constructor won’t accumulate duplicates over time. Based on learnings, this matches the existing CapacitorSerial/CapacitorBluetooth patterns.
|
|
Preview URL: https://pr4471.betaflight-app-preview.pages.dev |
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 (1)
src/js/protocols/CapacitorTcp.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: In betaflight-configurator’s CapacitorBluetooth.js, native plugin listeners (notification, services, connectionState) must be attached exactly once per app lifetime. Re-attaching them during writes/connect cycles leads to duplicate handlers, increased JS workload, MSP queue pressure, and can prevent the MSP_EEPROM_WRITE callback from firing (so reinitializeConnection is not reached). Implement an idempotent _attachNativeListenersOnce() guard.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-24T15:07:25.227Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:47-47
Timestamp: 2025-11-24T15:07:25.227Z
Learning: In betaflight-configurator's CapacitorSerial protocol (src/js/protocols/CapacitorSerial.js), the fire-and-forget this.loadDevices() call in the constructor is intentional background pre-loading. The actual critical device list loading for UI population is orchestrated by port_handler.js via serial.getDevices("serial"), which properly awaits CapacitorSerial.getDevices() → loadDevices() in a complete async chain. The constructor's async call does not cause race conditions.
Applied to files:
src/js/protocols/CapacitorTcp.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/CapacitorTcp.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/CapacitorTcp.js
📚 Learning: 2025-11-22T21:18:08.814Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:11-46
Timestamp: 2025-11-22T21:18:08.814Z
Learning: In betaflight-configurator, protocol implementations (WebSerial, CapacitorSerial, WebBluetooth, etc.) are instantiated once in the Serial class constructor and stored as singletons in the _protocols array for the entire application lifetime. They are never destroyed or recreated, so cleanup methods to remove event listeners are not needed.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-12-01T17:09:13.412Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-01T17:09:13.412Z
Learning: CapacitorBluetooth.connect should only dispatch the 'open' event after GATT services are resolved and notifications are started; announcing open earlier causes the first MSP write to fail with “Unable to write characteristic” and contributes to early queue load.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:41:59.374Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:135-143
Timestamp: 2025-11-28T22:41:59.374Z
Learning: In betaflight-configurator, WebSerial.js uses a constant path "serial" for all devices, and CapacitorBluetooth.js uses constant path "bluetooth", consistent with the single-device-at-a-time workflow. WebBluetooth.js uses counter-based paths (bluetooth_${counter}), but that's specific to its implementation. The path strategy varies by protocol based on their specific requirements.
Applied to files:
src/js/protocols/CapacitorTcp.js
📚 Learning: 2025-11-28T22:51:11.691Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:176-206
Timestamp: 2025-11-28T22:51:11.691Z
Learning: In betaflight-configurator, Web Bluetooth API's `services` option filters scan results to only devices advertising those UUIDs, while `optionalServices` requests permission to access services after connection without filtering. PWA (WebBluetooth.js) uses `{ acceptAllDevices: true, optionalServices: uuids }` to show all devices while requesting post-connection service access. CapacitorBluetooth.js should match this pattern rather than using `services` as a scan filter.
Applied to files:
src/js/protocols/CapacitorTcp.js
🪛 GitHub Check: SonarCloud Code Analysis
src/js/protocols/CapacitorTcp.js
[warning] 13-13: Prefer String#codePointAt() over String#charCodeAt().
[warning] 21-21: Prefer String.fromCodePoint() over String.fromCharCode().
⏰ 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). (1)
- GitHub Check: deploy
🔇 Additional comments (3)
src/js/protocols/CapacitorTcp.js (3)
5-24: Binary Base64 helpers are correct as writtenThe
base64ToUint8Array/uint8ArrayToBase64pair correctly implements binary-safe atob/btoa conversion usingcharCodeAt/fromCharCode, which is appropriate for 0–255 byte values produced byatob. I would keep this as-is even though some linters prefer the code‑point APIs.
26-67: Constructor and plugin listener wiring look solidState initialization, plugin availability check, and wiring of
dataReceived,dataReceivedError, andconnectionClosedintohandleReceiveBytes+receive/handleDisconnectare consistent with the other protocols and the singleton instantiation pattern inSerial. No changes needed here.
141-176:sendsemantics and counters now look consistentThe
sendmethod correctly tracksactualBytesSent, incrementsthis.bytesSent, and returns the actual byte count while invoking the optional callback with matching values. Error handling via the catch block and logging is also reasonable for this path.
|
Latest artifact download URL: https://github.com/betaflight/betaflight-configurator/actions/runs/19944243105/artifacts/4770004811 |
2 participants
Status
References
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.