Android ssh client - #235
Open
pappz wants to merge 34 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request adds SSH connection management, profile-scoped session persistence, peer actions, navigation, and an xterm.js WebView terminal with password prompts, reconnect support, clipboard handling, and responsive resizing. ChangesSSH session lifecycle
Navigation and interface integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
pappz
force-pushed
the
feature/android-client-ssh
branch
2 times, most recently
from
August 10, 2026 12:21
1902b73 to
a9f0074
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/io/netbird/client/CustomTabURLOpener.java (1)
28-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe launch-failure path does not call the registered callback.
The constructor receives
resultCallbackbut does not store it. The failure branch at Lines 80-82 instead checkscontext instanceof OnCustomTabResult.MainActivityimplementsServiceAccessorandStateListenerRegistry, notOnCustomTabResult, so the branch never runs. IfcustomTabLauncher.launch(intent)throws, no caller learns that the surface closed: the login opener never callsmBinder.stopEngine(), and the SSH opener never runs its close handler. The user sees a stalled login with no feedback.Store the callback and invoke it directly.
🐛 Proposed fix
private final ActivityResultLauncher<Intent> customTabLauncher; + private final OnCustomTabResult resultCallback; /** Written from a Go thread, read from the main thread and vice versa. */ private volatile boolean isOpened = false; @@ public CustomTabURLOpener(AppCompatActivity activity, OnCustomTabResult resultCallback) { this.context = activity; + this.resultCallback = resultCallback; this.customTabLauncher = activity.registerForActivityResult( new ActivityResultContracts.StartActivityForResult(), o -> { isOpened = false; resultCallback.onClosed(); } ); } @@ } catch (Exception e) { Log.e(TAG, "Failed to launch CustomTab: " + e.getMessage()); isOpened = false; - if (context instanceof OnCustomTabResult) { - ((OnCustomTabResult) context).onClosed(); - } + if (resultCallback != null) { + resultCallback.onClosed(); + } }Also applies to: 77-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/CustomTabURLOpener.java` around lines 28 - 37, Update CustomTabURLOpener’s constructor to retain the provided OnCustomTabResult callback, then change the launch-failure handling around customTabLauncher.launch to invoke that stored callback directly instead of checking context. Preserve the existing callback behavior for successful activity closure.
🧹 Nitpick comments (13)
app/src/main/res/navigation/mobile_navigation.xml (1)
100-103: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the
passwordnavigation argument and handling.The current dialog passes only
host,port, anduser, butSSHTerminalFragmentstill acceptsARG_PASSWORDandSshSessionretains passwords for reconnects. UsepromptForPassword()and clear the password after authentication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/res/navigation/mobile_navigation.xml` around lines 100 - 103, Remove the password argument from the navigation definition and eliminate related ARG_PASSWORD handling in SSHTerminalFragment and password retention in SshSession reconnect state. Have SSH authentication obtain credentials through promptForPassword(), then clear the password immediately after authentication while preserving host, port, and user arguments.app/src/main/assets/terminal/xterm.css (1)
237-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExclude the vendored stylesheet from Stylelint instead of editing it.
Stylelint reports
declaration-empty-line-beforeon Line 243. This file is the upstream xterm.js stylesheet and keeps its MIT header. Editing it complicates future upgrades.Add
app/src/main/assets/terminal/to.stylelintignoreso the vendored file stays byte-identical to upstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/assets/terminal/xterm.css` around lines 237 - 246, Add app/src/main/assets/terminal/ to .stylelintignore and leave the vendored xterm.css stylesheet unchanged, preserving its upstream byte content.Source: Linters/SAST tools
app/src/main/res/layout/list_item_ssh_session.xml (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a color resource for the indicator placeholder.
#4caf50repeats the literal used inSshSessionsFragment.colorForState. The runtime code overwrites this value on every bind, so the literal here is design-time only. A shared color resource keeps the two in step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/res/layout/list_item_ssh_session.xml` at line 13, Replace the hardcoded android:background value in the SSH session list item with a shared color resource, and update SshSessionsFragment.colorForState to use that same resource so the design-time placeholder and runtime indicator remain consistent.app/src/main/java/io/netbird/client/ui/ssh/SshConnectDialog.java (1)
240-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive feedback when the navigation controller cannot be resolved.
If
resolveNavControllerreturns null,connectreturns false. The dialog stays open and the Connect button appears dead. No message explains the failure.Log the condition, or show a toast, so the state is diagnosable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/ui/ssh/SshConnectDialog.java` around lines 240 - 242, Update the navController-null branch in connect to provide diagnostic feedback before returning false, using the existing logging mechanism or a user-visible toast. Ensure the failure to resolve the navigation controller is clearly reported while preserving the current return behavior.app/src/main/assets/terminal/index.html (2)
195-201: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe 50 ms delay before
onReadyis a timing assumption.If layout is not complete after 50 ms,
applyFitreturns early andonReadyreports the default 80x24 grid. The SSH session then starts with the wrong window size until the firstResizeObservercallback corrects it. On a slow device this produces visible reflow at the start of the session.
requestAnimationFramenested twice, or aResizeObserverfirst-callback trigger, ties the signal to actual layout instead of a fixed delay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/assets/terminal/index.html` around lines 195 - 201, Replace the fixed 50 ms setTimeout around applyFit and bridge.onReady with layout-synchronized scheduling, such as two nested requestAnimationFrame callbacks or the first ResizeObserver callback. Ensure applyFit completes after layout before bridge.onReady reports term.cols and term.rows, while preserving the final term.focus behavior.
110-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReport a resize only when the dimensions change.
applyFitcallsbridge.onResizeon every invocation. TheResizeObserveron Line 138 runsapplyFiton each animation frame while the keyboard animates. Each call crosses the JavaScript bridge and reachessession.resize, which sends a window-change request over SSH.Cache the last reported
cols/rowsand skip the call when they are unchanged.♻️ Proposed refactor
+ var lastCols = 0, lastRows = 0; function applyFit() { try { fit.fit(); } catch (e) { // Layout not ready yet return; } - if (bridge && bridge.onResize) { + if (bridge && bridge.onResize && (term.cols !== lastCols || term.rows !== lastRows)) { + lastCols = term.cols; + lastRows = term.rows; bridge.onResize(term.cols, term.rows); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/assets/terminal/index.html` around lines 110 - 120, Update applyFit to cache the last reported terminal cols and rows, and call bridge.onResize only when either dimension differs from the cached values. After reporting, update both cached dimensions; preserve the existing fit error handling and bridge checks.app/src/main/java/io/netbird/client/ui/ssh/SshSessionsFragment.java (2)
87-93: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
DiffUtilinstead ofnotifyDataSetChanged.
submitrebinds every row on each session snapshot. Session state changes arrive often during connect. The full rebind cancels item animations and can interrupt a touch on a row button.
ListAdapterwith aDiffUtil.ItemCallbackkeyed oninfo.idlimits the update to changed rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/ui/ssh/SshSessionsFragment.java` around lines 87 - 93, Update the adapter’s submit flow to use ListAdapter with a DiffUtil.ItemCallback keyed by SshSession.Info.id, replacing the items.clear/addAll and notifyDataSetChanged calls. Submit the new snapshot through the adapter so only changed session rows rebind while preserving existing row rendering.
74-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the state colors to color resources.
Color.parseColorruns on every bind and parses a string literal each time. The values also bypass the theme and the night-mode palette used elsewhere in this feature.Define the four colors in
res/values/colors.xmland read them withContextCompat.getColor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/ui/ssh/SshSessionsFragment.java` around lines 74 - 81, Move the four state color values out of SshSessionsFragment.colorForState into named color resources in res/values/colors.xml, including the existing default color. Update colorForState to accept or access a Context and return each resource via ContextCompat.getColor, preserving the current state-to-color mapping and enabling theme/night-mode overrides.app/src/main/res/layout/fragment_ssh_terminal.xml (1)
61-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the key labels to string resources.
Esc,Tab,Ctrl,Alt, the control codes, and the symbol keys are hardcoded in the layout.key_copyandkey_pastealready use@stringresources, so the file is inconsistent. Android Lint reportsHardcodedTextfor the literals.Declare the fixed key names in
strings.xmlwithtranslatable="false", and declareEsc,Tab,Ctrl, andAltas translatable entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/res/layout/fragment_ssh_terminal.xml` around lines 61 - 238, Replace the hardcoded labels in the buttons key_esc, key_tab, key_ctrl, key_alt, key_ctrl_c, key_ctrl_d, key_ctrl_z, key_up, key_down, key_left, key_right, key_pipe, key_tilde, key_slash, key_dash, and key_underscore with string-resource references. Add corresponding entries to strings.xml, marking Esc, Tab, Ctrl, and Alt as translatable while marking control-code and symbol labels as translatable="false"; preserve the existing key_copy and key_paste resources.app/src/main/java/io/netbird/client/MainActivity.java (2)
582-587: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
sshUrlOpenerfield to the field block.The field is declared between methods. Every other field in this class is declared at the top. Move it next to
urlOpenerandextendUrlOpenerfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/MainActivity.java` around lines 582 - 587, Move the sshUrlOpener field declaration from between methods to the class’s top-level field block, placing it alongside urlOpener and extendUrlOpener; leave getSSHURLOpener() unchanged.
169-181: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRegister the client factory after
sshUrlOpenerexists.Line 171 installs the
ClientFactory, butsshUrlOpeneris assigned at Line 252. Between those pointsurlOpener()returns null.SshSessionManager.reconnectpasses that value toSshSession.bindClient, which then skipssetURLOpener. The window is short, but the ordering is avoidable: createsshUrlOpenerbeforesetClientFactory.♻️ Proposed reordering
SshSessionManager.get().init(this); syncSshSessionProfile(); + sshUrlOpener = new CustomTabURLOpener(this, () -> { + // Custom Tab closed; SSH device-code polling will time out if not completed. + }); SshSessionManager.get().setClientFactory(new SshSessionManager.ClientFactory() {Then remove the assignment at Lines 252-254.
Also applies to: 252-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/MainActivity.java` around lines 169 - 181, Move the sshUrlOpener initialization before SshSessionManager.get().setClientFactory(...) so the factory’s urlOpener() returns an initialized value whenever used. Remove the later duplicate assignment and preserve the existing getSSHURLOpener() initialization.app/src/main/java/io/netbird/client/ServiceAccessor.java (1)
38-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new methods as
@Nullable.
MainActivity.newSSHClient()returns null when the VPN binder is null, andgetSSHURLOpener()returns thesshUrlOpenerfield, which is null untilonCreateassigns it. The other nullable methods in this interface carry@Nullable. Match that convention so callers get the same static-analysis signal.♻️ Proposed change
+ /** New SSH client, or null while the VPN service is not bound. */ + `@Nullable` SSHClient newSSHClient(); + /** URL opener for SSH SSO flows, or null before the activity is created. */ + `@Nullable` URLOpener getSSHURLOpener();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/ServiceAccessor.java` around lines 38 - 41, Annotate ServiceAccessor.newSSHClient() and ServiceAccessor.getSSHURLOpener() with `@Nullable`, matching the existing nullable method convention and ensuring the required annotation import is present.app/src/main/java/io/netbird/client/ui/ssh/SshSession.java (1)
176-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit null checks for
client.A restored session has
client == nulluntil a reconnect binds one.write,resize,close, anddisconnectdereferenceclientdirectly. Thecatch (Exception e)blocks swallow the resultingNullPointerException, so the code works, but it uses an exception for normal control flow and logs misleading messages such as "close failed: null".
SshSessionManager.setProfilecallscloseAllInternal, andSshSessionManager.editcallsexisting.close(); both can reachclose()on a restored session with no client.♻️ Proposed guard
public void write(byte[] data) { if (data == null || data.length == 0) { return; } + SSHClient target = client; + if (target == null) { + return; + } try { - client.write(data); + target.write(data); } catch (Exception e) { Log.d(LOGTAG, "write failed: " + e.getMessage()); } }public void close() { + SSHClient target = client; + if (target != null) { try { - client.close(); + target.close(); } catch (Exception e) { Log.d(LOGTAG, "close failed: " + e.getMessage()); } + }Also applies to: 237-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/netbird/client/ui/ssh/SshSession.java` around lines 176 - 198, Add explicit client-null guards in SshSession methods write, resize, close, and disconnect before dereferencing client. Preserve write/resize state behavior, return normally when no client is bound, and keep exception handling only for actual client operation failures rather than NullPointerException control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/io/netbird/client/MainActivity.java`:
- Around line 471-472: Update MainActivity’s SSH factory cleanup to be
ownership-aware: store the ClientFactory registered during onCreate in an
sshClientFactory field, and replace the unconditional
SshSessionManager.get().setClientFactory(null) call with conditional clearing
through a clearClientFactory(ClientFactory) method that only clears when the
supplied instance is still registered. Add this synchronized method to
SshSessionManager and use the stored factory during activity destruction.
In `@app/src/main/java/io/netbird/client/ui/ssh/SshConnectDialog.java`:
- Around line 222-227: Validate the parsed SSH port in both saveEdit and
connect, accepting only values from 1 through 65535; treat non-numeric and
out-of-range values as invalid rather than applying the default port. Set an
error on portInput and stop the save/connect operation when validation fails,
while preserving normal handling for valid ports.
In `@app/src/main/java/io/netbird/client/ui/ssh/SshSession.java`:
- Around line 115-156: Reset passwordAttempts to 0 in the successful connection
path of connectAsync, after target.startSession completes and sessionStarted is
set. Leave retryWithPassword and reconnect behavior unchanged.
In `@app/src/main/java/io/netbird/client/ui/ssh/SshSessionManager.java`:
- Around line 219-237: Update SshSessionManager.publish() to synchronize access
to sessions while copying sessions.values() into a separate list, then release
the monitor before calling SshSession.snapshot so mutable session-state reads
remain outside the lock. Preserve the existing postValue behavior and ensure the
map iteration is protected from concurrent create, close, edit, setProfile, and
state-change callbacks.
In `@app/src/main/java/io/netbird/client/ui/ssh/SshSessionStore.java`:
- Around line 99-108: Update the entry validation in SshSessionStore.load to
skip records when either FIELD_HOST or FIELD_ID is empty. Perform the FIELD_ID
check before adding the Entry, preserving the existing behavior for valid IDs
and hosts.
In `@app/src/main/java/io/netbird/client/ui/ssh/SSHTerminalFragment.java`:
- Around line 297-311: Update sendBytes to perform the keyAlt update through
mainHandler, keeping all binding.keyAlt view access on the main thread and
guarding against a destroyed view or null binding before calling
updateModifierStyle. Apply the same main-thread and lifecycle-safe handling to
the keyCtrl update in onInput.
- Around line 577-588: Replace all user-visible SSH status literals with
localized string resources: update SSHTerminalFragment’s status handling,
including CLOSED/ERROR, the connecting message, NetBird/session validation
messages in attachOrCreateSession, and update SshSessionsFragment’s connecting,
connected, password-required, closed, and error statuses. Add the required
formatted entries to strings.xml and remove the default-locale toLowerCase()
call, ensuring both files use resource-based localized text.
- Around line 238-256: Guard the asynchronous callback in copySelection with
isAdded() before calling requireContext(), accessing the clipboard service, or
showing completion toasts. Return immediately when the fragment is detached,
while preserving the existing empty-selection and successful-copy behavior when
it remains attached.
- Around line 334-337: Update SSHTerminalFragment.printStatus to use
JSONObject.quote for JavaScript string encoding, matching pasteClipboard, and
remove the manual backslash/single-quote escaping. Preserve the existing
window.printStatus invocation while ensuring newlines, carriage returns, U+2028,
and other special characters remain valid and intact.
- Around line 435-439: Update attachToSession to detach the existing
sessionListener from the current session before creating and attaching a
replacement listener, while preserving the existing session assignment and
new-listener setup.
- Around line 387-433: Update attachOrCreateSession to retain the ID of the
SshSession returned by manager.create and reuse it on subsequent onReady calls.
Before creating a new session, resolve the retained ID through
SshSessionManager.get, attach to that session, and resize it; only call
manager.create when no retained session exists or the retained session has been
closed.
In `@app/src/main/res/layout/fragment_ssh_terminal.xml`:
- Around line 131-169: Add localized android:contentDescription values to the
arrow buttons key_up, key_down, key_left, and key_right, and to the key_ctrl_c,
key_ctrl_d, and key_ctrl_z buttons. Use descriptive accessible names such as
“Arrow up” and “Control C” rather than relying on their symbol labels, defining
the strings through the project’s existing localization resources.
In `@app/src/main/res/layout/list_item_peer.xml`:
- Around line 90-91: Update the SSH button dimensions in the list_item_peer
layout to at least 48dp for both width and height, and adjust its padding or
equivalent inner spacing so the existing icon size remains unchanged.
In `@app/src/main/res/layout/list_item_ssh_session.xml`:
- Around line 9-16: Update the state_indicator View in the layout so it cannot
resolve to zero height: constrain its height to the label block’s vertical
bounds or assign an appropriate fixed height, while preserving its horizontal
constraints and existing appearance.
In `@netbird`:
- Line 1: Replace the regular SSH connection’s gossh.InsecureIgnoreHostKey()
configuration with a strict known-hosts or equivalent trusted-key callback
before opening the session. Update the SSH connection setup symbol that
constructs the client, while preserving the existing SSH CLI host-key
verification behavior.
---
Outside diff comments:
In `@app/src/main/java/io/netbird/client/CustomTabURLOpener.java`:
- Around line 28-37: Update CustomTabURLOpener’s constructor to retain the
provided OnCustomTabResult callback, then change the launch-failure handling
around customTabLauncher.launch to invoke that stored callback directly instead
of checking context. Preserve the existing callback behavior for successful
activity closure.
---
Nitpick comments:
In `@app/src/main/assets/terminal/index.html`:
- Around line 195-201: Replace the fixed 50 ms setTimeout around applyFit and
bridge.onReady with layout-synchronized scheduling, such as two nested
requestAnimationFrame callbacks or the first ResizeObserver callback. Ensure
applyFit completes after layout before bridge.onReady reports term.cols and
term.rows, while preserving the final term.focus behavior.
- Around line 110-120: Update applyFit to cache the last reported terminal cols
and rows, and call bridge.onResize only when either dimension differs from the
cached values. After reporting, update both cached dimensions; preserve the
existing fit error handling and bridge checks.
In `@app/src/main/assets/terminal/xterm.css`:
- Around line 237-246: Add app/src/main/assets/terminal/ to .stylelintignore and
leave the vendored xterm.css stylesheet unchanged, preserving its upstream byte
content.
In `@app/src/main/java/io/netbird/client/MainActivity.java`:
- Around line 582-587: Move the sshUrlOpener field declaration from between
methods to the class’s top-level field block, placing it alongside urlOpener and
extendUrlOpener; leave getSSHURLOpener() unchanged.
- Around line 169-181: Move the sshUrlOpener initialization before
SshSessionManager.get().setClientFactory(...) so the factory’s urlOpener()
returns an initialized value whenever used. Remove the later duplicate
assignment and preserve the existing getSSHURLOpener() initialization.
In `@app/src/main/java/io/netbird/client/ServiceAccessor.java`:
- Around line 38-41: Annotate ServiceAccessor.newSSHClient() and
ServiceAccessor.getSSHURLOpener() with `@Nullable`, matching the existing nullable
method convention and ensuring the required annotation import is present.
In `@app/src/main/java/io/netbird/client/ui/ssh/SshConnectDialog.java`:
- Around line 240-242: Update the navController-null branch in connect to
provide diagnostic feedback before returning false, using the existing logging
mechanism or a user-visible toast. Ensure the failure to resolve the navigation
controller is clearly reported while preserving the current return behavior.
In `@app/src/main/java/io/netbird/client/ui/ssh/SshSession.java`:
- Around line 176-198: Add explicit client-null guards in SshSession methods
write, resize, close, and disconnect before dereferencing client. Preserve
write/resize state behavior, return normally when no client is bound, and keep
exception handling only for actual client operation failures rather than
NullPointerException control flow.
In `@app/src/main/java/io/netbird/client/ui/ssh/SshSessionsFragment.java`:
- Around line 87-93: Update the adapter’s submit flow to use ListAdapter with a
DiffUtil.ItemCallback keyed by SshSession.Info.id, replacing the
items.clear/addAll and notifyDataSetChanged calls. Submit the new snapshot
through the adapter so only changed session rows rebind while preserving
existing row rendering.
- Around line 74-81: Move the four state color values out of
SshSessionsFragment.colorForState into named color resources in
res/values/colors.xml, including the existing default color. Update
colorForState to accept or access a Context and return each resource via
ContextCompat.getColor, preserving the current state-to-color mapping and
enabling theme/night-mode overrides.
In `@app/src/main/res/layout/fragment_ssh_terminal.xml`:
- Around line 61-238: Replace the hardcoded labels in the buttons key_esc,
key_tab, key_ctrl, key_alt, key_ctrl_c, key_ctrl_d, key_ctrl_z, key_up,
key_down, key_left, key_right, key_pipe, key_tilde, key_slash, key_dash, and
key_underscore with string-resource references. Add corresponding entries to
strings.xml, marking Esc, Tab, Ctrl, and Alt as translatable while marking
control-code and symbol labels as translatable="false"; preserve the existing
key_copy and key_paste resources.
In `@app/src/main/res/layout/list_item_ssh_session.xml`:
- Line 13: Replace the hardcoded android:background value in the SSH session
list item with a shared color resource, and update
SshSessionsFragment.colorForState to use that same resource so the design-time
placeholder and runtime indicator remain consistent.
In `@app/src/main/res/navigation/mobile_navigation.xml`:
- Around line 100-103: Remove the password argument from the navigation
definition and eliminate related ARG_PASSWORD handling in SSHTerminalFragment
and password retention in SshSession reconnect state. Have SSH authentication
obtain credentials through promptForPassword(), then clear the password
immediately after authentication while preserving host, port, and user
arguments.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b4e178e-69c8-4fe2-9ae1-87e86d45d1bf
📒 Files selected for processing (41)
app/src/main/assets/terminal/index.htmlapp/src/main/assets/terminal/xterm-addon-fit.jsapp/src/main/assets/terminal/xterm-addon-webgl.jsapp/src/main/assets/terminal/xterm.cssapp/src/main/assets/terminal/xterm.jsapp/src/main/java/io/netbird/client/CustomTabURLOpener.javaapp/src/main/java/io/netbird/client/MainActivity.javaapp/src/main/java/io/netbird/client/ServiceAccessor.javaapp/src/main/java/io/netbird/client/ui/home/PeersAdapter.javaapp/src/main/java/io/netbird/client/ui/ssh/SSHTerminalFragment.javaapp/src/main/java/io/netbird/client/ui/ssh/SshConnectDialog.javaapp/src/main/java/io/netbird/client/ui/ssh/SshSession.javaapp/src/main/java/io/netbird/client/ui/ssh/SshSessionManager.javaapp/src/main/java/io/netbird/client/ui/ssh/SshSessionStore.javaapp/src/main/java/io/netbird/client/ui/ssh/SshSessionsFragment.javaapp/src/main/res/drawable-night/edit_text_white_focusable.xmlapp/src/main/res/drawable/edit_text_white_focusable.xmlapp/src/main/res/drawable/ic_nav_ssh.xmlapp/src/main/res/drawable/ic_ssh_close.xmlapp/src/main/res/drawable/ic_ssh_disconnect.xmlapp/src/main/res/layout/dialog_simple_edit_text.xmlapp/src/main/res/layout/fragment_ssh_sessions.xmlapp/src/main/res/layout/fragment_ssh_terminal.xmlapp/src/main/res/layout/list_item_peer.xmlapp/src/main/res/layout/list_item_ssh_session.xmlapp/src/main/res/menu/bottom_nav.xmlapp/src/main/res/menu/peer_clipboard_menu.xmlapp/src/main/res/navigation/mobile_navigation.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-hu/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ja/strings.xmlapp/src/main/res/values-pt/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values/strings.xmlnetbirdtool/src/main/java/io/netbird/client/tool/EngineRunner.javatool/src/main/java/io/netbird/client/tool/VPNService.java
pappz
force-pushed
the
feature/android-client-ssh
branch
from
August 11, 2026 12:14
928037b to
c7130bd
Compare
- WebView-hosted xterm.js (5.5.0 + fit-addon) terminal rendered via app/src/main/assets/terminal/. The Go gomobile SSHClient streams PTY output to Java which evaluateJavascripts base64 chunks into xterm. - Auto-detection of server type via NetBird SSH banner: NetBird-JWT triggers the existing Custom-Tabs URL opener for the OAuth 2.0 device-code flow, NetBird-no-JWT uses the NetBird private key, and a regular OpenSSH server falls back to NetBird key then optional password. One unified Connect() in Go covers all three. - Persistent sessions: SshSessionManager (application-scoped singleton) owns SSHClients with a 256 KB scrollback buffer per session, so fragments can detach (e.g. on backgrounding) and re-attach later with the scrollback replayed before live output resumes. - New "SSH" drawer entry → SshSessionsFragment lists active sessions with state indicators and a FAB to open the connect dialog for a free-form host. Peer long-press → SSH continues to work with the IP prefilled. - Connect dialog asks only for host (when not prefilled), username, port, and an optional password used by regular SSH fallback. - ActionBar auto-hides on the terminal destination for maximum screen area; BottomSheetDialogFragments auto-dismiss when navigating away from home so the terminal is not covered. Requires the matching netbird submodule on the android-client-ssh branch which adds the SSHClient gomobile binding.
The long-press menu already offered SSH, but nothing hinted at it, and a row tap opens the peer detail so it could not carry the action either. A dedicated button makes it reachable in one tap. Shown whatever the peer's status: the engine dials on demand, so an idle peer still accepts a connection. The long-press entry stays, and loses its connected-only condition for the same reason.
The NetBird auth paths never use a password, and a regular server is tried with the NetBird key first, so asking up front was wrong more often than not. The terminal now prompts only once the server has actually refused everything else. The default port follows where the connection starts from: a prefilled host is a NetBird peer on 22022, one typed by hand is an ordinary server on 22. The nav argument and the parse fallback follow suit. Enter submits, so the fields carry IME actions. setSingleLine has to precede setInputType, since it resets the type.
Adds a NEEDS_PASSWORD state, which is a pause rather than a failure: the session waits for the terminal to collect a password and retries, as often as the server keeps refusing, matching what a normal ssh client allows. Cancelling ends the session instead of parking it with no way forward. A finished session can be redialled in place from a bar below the terminal, reusing the session so its scrollback stays readable. The screen is cleared only on the very first connect, so the connect chatter does not sit above the prompt while earlier output survives a reconnect. CONNECTING now prints a notice: a reconnect does not go through the create path, so it had none.
The list only lived in memory, so it was lost on restart. Connection details now go to SharedPreferences and come back as closed sessions that reconnect on demand; a live connection cannot outlive the process. Passwords are never stored, and a restored entry prompts again. Keyed by profile, because an overlay IP means a different host under a different profile, so one list must not leak into another. Switching closes whatever is live, since the tunnel goes down with the old profile. Lists belonging to deleted profiles are discarded by comparing against the live profile IDs, as deletion happens elsewhere and reports nothing.
Tapping a finished session reconnects when it left no output behind, and otherwise just opens it, letting the terminal's own bar offer the redial once there is something to read. A disconnect button ends a live session while keeping it listed, distinct from closing it, which also discards the scrollback and so asks for confirmation first. The row's text was constrained to the close button rather than the one beside it, so the label overlapped and hid it. The night theme inherits a Light parent, leaving colorControlNormal and the default text colour dark, so the icons and labels were invisible. Both now use the app's own theme-aware colours, which meant replacing the framework close icon with a tintable one.
Sessions were already keyed by a unique id rather than by host, so parallel connections to one target worked; what was missing was a way to ask for one, and a way to tell the results apart. Long-pressing a row now offers Duplicate, which opens a second session to the same target and connects it. The password is not carried over: it belongs to the session that was asked for it, so a server wanting one prompts again. Sessions sharing a target are numbered, the number leading the label as tmux does, since the target is long enough to be truncated on a narrow row and that would drop the part that disambiguates. A target with a single session stays unnumbered. The label also gains the ellipsize and maxLines the peer rows already use, so a long FQDN cannot wrap and make rows uneven.
The SSH sessions floating action button used the default Material tint and the framework ic_input_add icon, so it looked out of place next to the flat orange FAB on the profiles page. Give it the same drawable, background tint, white icon and zero elevation, switch the fixed 16dp margin to fab_margin so it insets on landscape and tablet layouts, and reuse fab_content_inset for the list's bottom padding.
Replaces the bundled xterm.js 5.5.0 and addon-fit 0.10.0 with 6.0.0 and 0.11.0, taken from the npm tarballs rather than a CDN so the files carry no third-party minification. This matches the version the iOS client already ships, so the two platforms no longer drift apart. Every API index.html relies on is unchanged in 6.0, so the only fix the upgrade needs is for the scrollbar: 6.0 renders its own scrollbar element instead of using the native one, which the existing ::-webkit-scrollbar rule no longer reaches.
Puts the xterm options that were left at their defaults to use: the full 16 colour ANSI palette, since without one the server's colours fall back to the WebView defaults and are close to unreadable on black, a contrast floor for the pairings that stay illegible anyway, and allowProposedApi so the buffer and parser APIs are reachable. Font size goes down rather than up: every point costs about four columns, and wrapped lines cost more than small glyphs. Loads the WebGL renderer as well, dropping it on context loss so a backgrounded app falls back to the DOM renderer instead of showing a blank terminal. Grows the key bar to cover what a phone keyboard makes expensive: ^C, ^D and ^Z as single keys, because arming Ctrl needs the soft keyboard to then deliver a letter and it does not always do so; the punctuation that sits behind a symbol page; and copy and paste, which the terminal had no way to reach at all. Sticky Ctrl and Alt stay for every other combination. The keyboard used to cover the terminal outright. The manifest asks for adjustPan, which slides the window up and carries the key bar off screen, so the fragment switches to adjustResize while it is visible. From API 35 that mode is ignored and the keyboard simply draws over the window, so the IME inset is padded instead. Either way the WebView ends up shorter, which needs .xterm to track its container height, or the row count never shrinks. Also gives the password prompt the dialog theme the rest of the app uses. It was building a bare AlertDialog, so the theme's global text colour made the title white on white; that theme deliberately leaves the window transparent and expects the shared rounded layout to supply the body.
A developer's own login name was baked into four places: the connect dialog's fallback, the terminal fragment's argument default, a string resource and the navigation graph. Anyone else got that name silently substituted whenever the field was left empty, which fails authentication against a remote account that does not exist. There is no sensible default to replace it with, since the login name is the remote account. The dialog now prefills whatever was last connected with, empty on a fresh install, and stores it again on connect. The key is not per profile: the name belongs to whoever holds the phone, and the same account is usually used whichever profile is active. Connecting with an empty host or username now marks the field and leaves the dialog open rather than dismissing it, which is what the substituted default used to paper over.
Picks up the SSH JWT flow calling OnLoginSuccess once it has a token, so the Custom Tab opened for device-code auth closes itself instead of staying in front of the terminal. No app-side change is needed: the SSH URL opener is the same CustomTabURLOpener the login flow uses, and its onLoginSuccess already brings the activity forward. MainActivity is singleTask, so that returns to the existing instance and the terminal fragment is still on the stack.
Bumps the submodule for the SSH JWT flow calling its URL opener in turn rather than from two racing goroutines, which is what left the browser in front of the terminal after the token had arrived. Calling in turn exposed two problems here that the goroutines had been hiding. launch() and startActivity() drive activity machinery and have to run on the main thread, so a synchronous call from a Go thread would raise a wrong-thread error; both are posted now. isOpened is set before that post rather than inside it, because the caller may report success straight after and onLoginSuccess does nothing unless the surface is already marked as opened, and it is volatile since the two threads share it. onLoginSuccess deliberately leaves isOpened set: MainActivity.onStop reads it to keep the service bound while the SSO surface is in front, and the launcher callback clears it when the tab actually goes away.
A session saved with the wrong address or login name could only be closed and recreated from scratch. Long-pressing an entry now offers Edit alongside Duplicate, prefilled with the session's own details. The details are final on a session, so the entry is rebuilt rather than mutated: the old one is closed and replaced under the same id. That keeps its place in the list, since a LinkedHashMap put on an existing key holds the original position, and overwrites the stored entry instead of appending a second one. The scrollback goes with it, having come from a different host. Editing leaves the session disconnected on purpose. Redialling here would connect before the user has seen whether the new details are right, and the list already offers a reconnect. The host field is always shown in the editor, including for a peer session where connecting hides it, because correcting the address is half of what the editor is for.
MainActivity locks portrait on phones, which suits every screen it has. A terminal is the exception: landscape roughly doubles the column count, which is what long command lines and full-screen programs need. The fragment unlocks the orientation while it is on screen and restores the lock on the way out, so nothing else gains a rotation it was not designed for. The session survives the rotation on its own: it belongs to the manager rather than the fragment, onDestroyView only detaches the listener, and attaching replays the scrollback into the recreated view. The arguments needed one fix for this. A fragment opened from the connect dialog carries host details and no session id, so a recreated view took the create path and would have dialled a second session to the same target on every turn of the screen. The id of a session created here is written back into the arguments, and the password dropped from them now that it has been handed to the session.
Regular SSH servers previously connected without any host-key check. Show the presented fingerprint for an untrusted host and, once the user confirms it, reconnect with the key trusted; the Go side then stores it in a per-profile known-hosts file and verifies against it thereafter. The store is per profile, since an overlay IP is a different host under a different profile, and a profile's file is removed with the profile. A host's key is also dropped once no session targets it, so deleting the last session for a host clears its trusted key while a shared host keeps it. Bumps the netbird submodule for the host-key verification changes.
The SSH terminal strings only existed in the base resources, so the whole feature showed in English under de, es, fr, hu, it, ja, pt, ru and zh-rCN. Add the 34 strings to each, port numbers and format placeholders left intact.
pappz
force-pushed
the
feature/android-client-ssh
branch
from
August 12, 2026 09:52
62364ca to
734295a
Compare
Synchronize the session snapshot, keep view writes on the main thread, survive an activity recreate, and stop leaking session listeners.
Name the symbol keys for screen readers, grow the peer SSH button to the 48dp touch target, and give the session state bar the peer list's shape.
lixmal
previously approved these changes
Aug 14, 2026
pappz
force-pushed
the
feature/android-client-ssh
branch
from
August 14, 2026 15:59
c20647b to
4380065
Compare
lixmal
previously approved these changes
Aug 14, 2026
The Go flows now invoke URLOpener.open synchronously from a Go thread, so openers must not do UI work inline. The device-code login opener was the only one still showing the QR dialog and starting the browser on the calling thread; post both to the main thread like the extend opener does, and post onLoginSuccess as well so the dialog field is only touched from the main thread. Bump netbird for the shared OAuth token flow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes