Repository navigation
fix(ssh): terminal ux - #82
Merged
Merged
Conversation
- One character per cell, so the grid stays monospaced where no mono font exists (Android) - Scrollback with touch drag; output no longer lost when the keyboard resizes the view - Long-press selection with handles, copy/copy screen/paste menu, Ctrl+V / Ctrl+Shift+C - "Keyboard on tap" toggle; shell opens at the settled width; SSH.NET shell read fixes
…the screen edge - Paste drops the trailing line break; a multi-line paste asks: one line / run all / cancel - Bracketed paste (CSI ?2004h) is honoured when the shell asks for it - Copy joins rows the terminal soft-wrapped, so a long command copies as one line - Edge auto-scroll ran the wrong way and per pointer event; bottom-row handle was ungrabbable
- targetSdk 36 is edge-to-edge, so AdjustResize is ignored and the IME overlays the window - SoftKeyboardInset pads MainView and overlay dialogs by max(safe area, keyboard) - Focused field is scrolled into view; rotation re-applies the inset - In the SSH terminal the prompt stays above the keyboard
PR Summary by QodoFix SSH terminal selection, paste, scrollback, and mobile keyboard layout
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Code Review by Qodo
1. Failed shell writes keep piling up
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+802
to
+805
| var pasteCombo = | ||
| (e.KeyModifiers.HasFlag(KeyModifiers.Control) && e.Key == Key.V) || | ||
| (e.KeyModifiers.HasFlag(KeyModifiers.Shift) && e.Key == Key.Insert); | ||
| if (pasteCombo) |
There was a problem hiding this comment.
4. Tablet shortcuts send control keys 🐞 Bug ≡ Correctness
TerminalView.OnKeyDown recognizes the new copy and paste shortcuts, but the mobile terminal keeps keyboard focus on ImeProxy, whose OnProxyKeyDown still routes every Ctrl+letter combination through SendInput. With an external keyboard on mobile, Ctrl+V sends a control byte instead of pasting, and Ctrl+Shift+C sends the interrupt byte instead of copying.
Agent Prompt
## Issue description
Mobile hardware-keyboard events go to the IME proxy, bypassing the terminal's new copy and paste shortcut handling.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/Views/Dialogs/SshTerminalContent.axaml.cs[379-403]
- src/OpenIPC.Viewer.App/Controls/TerminalView.cs[798-824]
## Recommended Fix
Recognize copy and paste combinations in the proxy before its generic Ctrl+letter branch, and route them to the same clipboard actions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+203
to
+204
| var top = history - _scrollOffset; | ||
| var selection = NormalizedSelection(); |
There was a problem hiding this comment.
5. New output moves the scrollback view 🐞 Bug ≡ Correctness
TerminalView.Render calculates the first displayed history row as `ScrollbackRows - _scrollOffset, while _scrollOffset` stays unchanged as output arrives. When a command continues printing after the user scrolls back, each appended history row moves the visible window forward instead of keeping the rows being read in place.
Agent Prompt
## Issue description
A fixed offset from the newest output makes a scrolled-back viewport advance whenever the shell prints another row.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/Controls/TerminalView.cs[197-210]
- src/OpenIPC.Viewer.App/Controls/TerminalView.cs[297-305]
- src/OpenIPC.Viewer.Core/Ssh/Terminal/TerminalEmulator.cs[363-373]
## Recommended Fix
Track the displayed history position while scrolled back, adjusting the offset when history grows and accounting for evictions at the history cap.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+132
to
+134
| Array.Copy(source, next[r], Math.Min(columns, source.Length)); | ||
| if (IsWrapped(source)) | ||
| MarkWrapped(next[r], true); |
There was a problem hiding this comment.
6. Widening the terminal adds copied spaces 🐞 Bug ≡ Correctness
TerminalEmulator.Resize preserves a source row's wrap mark after copying it into a wider blank row, and LineJoiner.Add retains that row's trailing cells when joining wrapped text. If a four-column abcdef wraps into abcd and ef before the view widens to eight columns, copying the rows produces abcd ef rather than abcdef.
Agent Prompt
## Issue description
Resize transfers wrap metadata to a wider row, causing copy to treat newly padded cells as spaces from the original wrapped line.
## Fix Focus Areas
- src/OpenIPC.Viewer.Core/Ssh/Terminal/TerminalEmulator.cs[118-135]
- src/OpenIPC.Viewer.App/Controls/TerminalView.cs[423-429]
## Recommended Fix
Preserve the original wrap boundary or reflow wrapped text on width changes, so joining a copied line does not include padding introduced by resize.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+96
to
+97
| private (int Row, int Col)? _selAnchor; | ||
| private (int Row, int Col)? _selFocus; |
There was a problem hiding this comment.
7. Selected history can copy another row 🐞 Bug ≡ Correctness
TerminalView retains a selection as numeric history-row positions without adjusting them when the emulator evicts its oldest scrollback row. If output reaches the 1000-row history cap while a selection remains, the highlight and copied text shift to newer rows rather than staying on the selected content.
Agent Prompt
## Issue description
Evicting the oldest scrollback row shifts every numeric selection coordinate without moving the stored selection endpoints.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/Controls/TerminalView.cs[91-97]
- src/OpenIPC.Viewer.App/Controls/TerminalView.cs[373-388]
- src/OpenIPC.Viewer.Core/Ssh/Terminal/TerminalEmulator.cs[363-373]
## Recommended Fix
Expose history evictions to the view and adjust or clear affected selection endpoints, or identify selected rows independently of their current list index.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+137
to
+140
| catch (Exception ex) | ||
| { | ||
| _logger.LogDebug(ex, "SSH shell write loop ended"); | ||
| } |
There was a problem hiding this comment.
1. Failed shell writes keep piling up 🐞 Bug ☼ Reliability
WriteLoopAsync catches a write exception and exits without completing or faulting _outgoing, while SendAsync continues to accept strings into its unbounded channel. After the connection fails during a send, subsequent terminal input has no consumer, receives no failure from SendAsync, and accumulates in memory.
Agent Prompt
## Issue description
The writer can terminate while the unbounded outgoing channel remains writable, silently retaining subsequent terminal input.
## Fix Focus Areas
- src/OpenIPC.Viewer.Infrastructure/Ssh/SshNetShell.cs[34-38]
- src/OpenIPC.Viewer.Infrastructure/Ssh/SshNetShell.cs[120-148]
## Recommended Fix
Complete or fault the outgoing channel when its writer fails, reject later sends, and propagate the write failure to callers rather than reporting queue admission as delivery.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+166
to
169
| _outgoing.Writer.TryComplete(); | ||
| _stopping.Cancel(); | ||
| // Disposing the stream is what unblocks the parked read. | ||
| _stream.Dispose(); |
There was a problem hiding this comment.
8. Closing a terminal can discard sent input 🐞 Bug ☼ Reliability
SendAsync now completes when it enqueues text, but DisposeAsync immediately cancels the writer and disposes its stream without draining queued entries. If the user closes the terminal just after sending a command or choosing to run a paste, bytes that SendAsync already reported as sent can remain in the queue and never reach the camera.
Agent Prompt
## Issue description
Shell shutdown cancels the writer immediately even though prior sends only guaranteed queue admission, not delivery.
## Fix Focus Areas
- src/OpenIPC.Viewer.Infrastructure/Ssh/SshNetShell.cs[120-148]
- src/OpenIPC.Viewer.Infrastructure/Ssh/SshNetShell.cs[156-173]
## Recommended Fix
Give queued writes a bounded opportunity to finish before cancelling and disposing the stream, and make send completion reflect delivery or a reported failure.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+78
to
+80
| // Bounded: nobody is coming for this if the terminal never attached. | ||
| if (_pending is { Count: < 64 }) | ||
| _pending.Add(data); |
There was a problem hiding this comment.
9. Early shell output can disappear 🐞 Bug ☼ Reliability
SshNetShell.Publish stops buffering receive chunks after _pending reaches 64 entries, although the reader starts before the terminal subscribes. If a shell prints enough output before SshTerminalViewModel attaches its handler, later chunks in that interval are discarded and cannot be recovered from the stream.
Agent Prompt
## Issue description
The read loop consumes shell output before a listener attaches, but its pending buffer silently drops chunks after 64 entries.
## Fix Focus Areas
- src/OpenIPC.Viewer.Infrastructure/Ssh/SshNetShell.cs[41-81]
- src/OpenIPC.Viewer.Infrastructure/Ssh/SshNetShell.cs[84-107]
- src/OpenIPC.Viewer.App/ViewModels/SshTerminalViewModel.cs[68-80]
## Recommended Fix
Attach the listener before starting reads or apply backpressure until it attaches; do not consume and discard unread terminal output.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+30
to
+32
| var paste = TerminalPaste.Normalize("curl -s \\n http://cam/x\n\n | head\n"); | ||
| Assert.Equal(4, TerminalPaste.LineCount(paste)); | ||
| Assert.Equal("curl -s http://cam/x | head", TerminalPaste.AsOneLine(paste)); |
There was a problem hiding this comment.
2. Paste unit test fails and misses its target case 🐞 Bug ≡ Correctness
AsOneLine_JoinsTrimsAndDropsContinuationBackslash uses \\n, producing a literal backslash and n rather than a backslash followed by a newline. The input therefore has three lines instead of the asserted four, causing both the line-count and joined-command assertions to fail without exercising continuation-backslash stripping.
Agent Prompt
## Issue description
The continuation-backslash test uses `\\n`, which C# reads as a literal backslash followed by `n`, rather than a backslash followed by a newline. Its line-count and joined-text assertions fail, and it does not test continuation-backslash stripping.
## Fix Focus Areas
- tests/OpenIPC.Viewer.Core.Tests/Ssh/TerminalPasteTests.cs[28-33]
## Recommended Fix
Change the input to `"curl -s \\\n http://cam/x\n\n | head\n"` so the continuation backslash is immediately followed by a newline. Then verify that both the asserted line count and joined command match the corrected input.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Losing rows takes them off the TOP, the way a real terminal does it: the interesting end | ||
| // of a shell session is the bottom one. Keeping the top instead would drop the prompt every | ||
| // time the view got shorter — which on a phone is every time the soft keyboard opens. | ||
| var drop = had ? Math.Max(0, Rows - rows) : 0; |
There was a problem hiding this comment.
3. Opening the keyboard can hide the shell prompt 🐞 Bug ≡ Correctness
Resize always moves Rows - rows lines from the top into scrollback, wherever the cursor is. If the cursor is above the new bottom edge, as in a fresh session or after clear, the prompt scrolls out of view and the cursor is clamped to row 0 on a blank line, so typed text echoes away from the prompt and the line stays split after the keyboard closes.
Agent Prompt
## Issue description
When the screen gets shorter, `Resize` pushes rows from the top into scrollback even when the cursor sits near the top. The prompt is then pushed off screen and the cursor lands on a blank row.
## Fix Focus Areas
- src/OpenIPC.Viewer.Core/Ssh/Terminal/TerminalEmulator.cs[96-145]
## Recommended Fix
Set `drop = Math.Clamp(_cursorRow - (rows - 1), 0, Rows - rows)` so only enough top rows go to scrollback to keep the cursor on screen. Remove any remaining excess rows from the empty bottom of the screen instead. Add a test where the cursor is near the top when the screen shrinks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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
Related
Type
Checklist
TreatWarningsAsErrors=true).dotnet test); new Core logic has unit tests.AppreferencesCoreonly (Infrastructure / Video / Devices wired via DI in a head).Platforms tested
Screenshots / notes