feat: support non-ascii characters in io text #128
Conversation
WalkthroughModified the Changes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
devices/android.go (1)
657-687: ASCII vs DeviceKit paths look solid; consider richer errors and always-clearing clipboardThe overall control flow (ASCII via
input text+ escaping, non-ASCII via DeviceKit clipboard + paste, explicit error otherwise) aligns well with the PR goal. A couple of small, non-blocking improvements:
Include adb output in new error paths
Forset clipboardandpaste, you currently drop the command output, which can make debugging hard compared to other methods in this file that include it.For example:
- _, err := d.runAdbCommand("shell", "am", "broadcast", "-a", "devicekit.clipboard.set", "-e", "encoding", "base64", "-e", "text", base64Text, "-n", "com.mobilenext.devicekit/.ClipboardBroadcastReceiver") - if err != nil { - return fmt.Errorf("failed to set clipboard: %w", err) - } + output, err := d.runAdbCommand("shell", "am", "broadcast", "-a", "devicekit.clipboard.set", "-e", "encoding", "base64", "-e", "text", base64Text, "-n", "com.mobilenext.devicekit/.ClipboardBroadcastReceiver") + if err != nil { + return fmt.Errorf("failed to set clipboard: %v\nOutput: %s", err, string(output)) + }And similarly for the
KEYCODE_PASTEcall.
- Always attempt to clear the clipboard (even on paste failures)
Right now, ifKEYCODE_PASTEfails, the function returns without attempting the clear broadcast, potentially leaving sensitive text on the clipboard. You could make the clear adeferso it always runs best-effort:- // try sending over clipboard if DeviceKit is installed - if d.isDeviceKitInstalled() { + // try sending over clipboard if DeviceKit is installed + if d.isDeviceKitInstalled() { + defer func() { + _, _ = d.runAdbCommand("shell", "am", "broadcast", "-a", "devicekit.clipboard.clear", "-n", "com.mobilenext.devicekit/.ClipboardBroadcastReceiver") + }() @@ - // clear clipboard when we're done - _, _ = d.runAdbCommand("shell", "am", "broadcast", "-a", "devicekit.clipboard.clear", "-n", "com.mobilenext.devicekit/.ClipboardBroadcastReceiver") - - return nil + return nil }These changes are optional, but they improve debuggability and make the clipboard behavior more robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
devices/android.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (3)
devices/android.go (3)
5-5: Base64 import fits the new clipboard flowImporting
encoding/base64is appropriate for the DeviceKit clipboard path and looks correct.
612-641: Tighten helpers: simplify DeviceKit check and improve shell-escaping helperTwo small cleanups here:
isDeviceKitInstalled:
- Given
GetAppPathcurrently never surfaces an error (it returns("", nil)on failure), theerr == nilpart is effectively redundant.- You can simplify without behavior change:
-func (d *AndroidDevice) isDeviceKitInstalled() bool { - appPath, err := d.GetAppPath("com.mobilenext.devicekit") - return err == nil && appPath != "" -} +func (d *AndroidDevice) isDeviceKitInstalled() bool { + appPath, _ := d.GetAppPath("com.mobilenext.devicekit") + return appPath != "" +}
escapeShellText:
- The current
specialCharsconstruction is a bit hard to read and introduces an extra newline into the set.- Repeated string concatenation inside the loop is O(n²) in the worst case.
- A small refactor with a named const and
strings.Builderkeeps it clearer and linear:-// escapeShellText escapes shell special characters -func escapeShellText(text string) string { - // escape all shell special characters that could be used for injection - specialChars := `\'"`+ "`" + ` -|&;()<>{}[]$*?` - result := "" - for _, char := range text { - if strings.ContainsRune(specialChars, char) { - result += "\\" - } - result += string(char) - } - return result -} +const shellSpecialChars = "\\'\"`|&;()<>{}[]$*?" + +// escapeShellText escapes shell special characters +func escapeShellText(text string) string { + var b strings.Builder + b.Grow(len(text) * 2) // worst case: every char escaped + for _, char := range text { + if strings.ContainsRune(shellSpecialChars, char) { + b.WriteByte('\\') + } + b.WriteRune(char) + } + return b.String() +}[ suggest_optional_refactor ]
644-648: Early return on empty text is a good guardShort-circuiting on
text == ""avoids invokingadb shell inputwith an empty payload and matches the “simple submit” comment; no issues here.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.