fix: add VT100 response feedback to SwingTerminal and WebTerminal#1824
Conversation
#1797) SwingTerminal and WebTerminal never fed VT100 responses (cursor position reports, device attributes) back to the LineDisciplineTerminal after writing to the ScreenTerminal. DisplayTest and Tmux do this correctly via ScreenTerminal.read() + processInputBytes(). Add feedbackVt100Response() to both output streams so that VT100 query-response sequences (e.g. DSR, DA) work correctly.
📝 WalkthroughWalkthroughOutput streams for SwingTerminal and WebTerminal were changed to perform an immediate read/feedback loop after writing decoded output: they call a feedback hook that reads pending bytes from the terminal component and routes any non-empty bytes back into the terminal's input processor for immediate processing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OutputStream
participant TerminalComponent
participant Terminal
Client->>OutputStream: write(bytes)
OutputStream->>OutputStream: decode UTF-8
OutputStream->>TerminalComponent: write(decoded output)
OutputStream->>OutputStream: feedbackVt100Response()
OutputStream->>TerminalComponent: read()
TerminalComponent-->>OutputStream: pending bytes
alt pending bytes non-empty
OutputStream->>Terminal: processInputBytes(pending bytes)
Terminal->>Terminal: process VT100 response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
builtins/src/main/java/org/jline/builtins/WebTerminal.java (1)
500-508:⚠️ Potential issue | 🟠 MajorSame concurrency concern as
SwingTerminalOutputStream.feedbackVt100Response.
TerminalAjaxHandler.handle(line 330) callsprocessInputByteson the HTTP handler thread to deliver user keystrokes. Output generated by another thread (e.g., a running command writing tomasterOutput) now also callsprocessInputBytesvia this feedback hook. SinceLineDisciplineTerminal.processInputBytesis unsynchronized, a VT100 response and a concurrent keystroke can interleave byte-for-byte inslaveInputPipe. Please verify whether serialization is needed here; the same remediation would apply toSwingTerminal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builtins/src/main/java/org/jline/builtins/WebTerminal.java` around lines 500 - 508, feedbackVt100Response can race with TerminalAjaxHandler.handle because both call LineDisciplineTerminal.processInputBytes (via webTerminal.processInputBytes), which is unsynchronized and can interleave bytes into slaveInputPipe; serialize calls to processInputBytes by guarding them with the same lock used elsewhere (or add a dedicated lock) so both TerminalAjaxHandler.handle and WebTerminal.feedbackVt100Response synchronize on that lock before invoking webTerminal.processInputBytes; update WebTerminal.feedbackVt100Response to acquire the lock (or call through a single-threaded executor) prior to calling processInputBytes, and apply the same pattern to SwingTerminalOutputStream.feedbackVt100Response to ensure consistent serialization.
🧹 Nitpick comments (2)
builtins/src/main/java/org/jline/builtins/SwingTerminal.java (2)
872-874: LGTM — consider a brief Javadoc for consistency.Other public methods in
TerminalComponent(write,dump,isDirty,pollInput,takeInput) are documented; adding a one-liner describing that this drains pending VT100 output from the underlyingScreenTerminalwould keep the surface consistent.📝 Proposed Javadoc
+ /** + * Drains any pending VT100 response output produced by the underlying {`@link` ScreenTerminal}. + * + * `@return` the accumulated VT100 output, or an empty string if none + */ public String read() { return screenTerminal.read(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java` around lines 872 - 874, Add a one-line Javadoc to the public method read() in SwingTerminal describing its behavior and consistency with other TerminalComponent methods: state that read() drains and returns pending VT100 output from the underlying ScreenTerminal (screenTerminal.read()), keeping the surface consistent with write, dump, isDirty, pollInput, and takeInput documentation; place the Javadoc immediately above the public String read() method in class SwingTerminal.
310-314: Minor: feedback is skipped on the buffer-full decode path.
decodeAndOutput(false)on line 312 (and line 450 inside it) can write decoded characters tocomponentwhen the 16-byteinputBufferfills up, but does not callfeedbackVt100Response(). In practice the VT100 response stays queued invt100_outand is picked up on the next write, so it's not a correctness bug — just an inconsistency worth a follow-up if you want uniform drain behavior.Also applies to: 449-452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java` around lines 310 - 314, When inputBuffer fills and you call decodeAndOutput(false) (in the buffer-full branches around inputBuffer.hasRemaining() and the similar path at lines ~449-452), the code writes decoded chars to component but does not call feedbackVt100Response(), leaving vt100_out queued; update those buffer-full paths to invoke feedbackVt100Response() after decodeAndOutput(false) so VT100 feedback is drained immediately and behavior matches the normal decode path. Ensure you reference the same flow used elsewhere (where decodeAndOutput(true) triggers feedbackVt100Response()) and call feedbackVt100Response() only after decodeAndOutput(false) completes to preserve ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java`:
- Around line 270-278: feedbackVt100Response currently calls
terminal.processInputBytes(...) from arbitrary threads (EDT/HTTP handlers) while
inputThread and other handlers also call processInputBytes, allowing byte-level
interleaving; fix by serializing all calls to
LineDisciplineTerminal.processInputBytes: introduce a dedicated lock or marshal
VT100 responses onto the same input thread used by SwingTerminal.inputThread (or
WebTerminal's input dispatcher) and use that lock when invoking
terminal.processInputBytes in SwingTerminal.feedbackVt100Response and
WebTerminal.WebTerminalOutputStream.feedbackVt100Response so writes to the
LineDisciplineTerminal/slaveInputPipe are single-threaded and ordered.
---
Duplicate comments:
In `@builtins/src/main/java/org/jline/builtins/WebTerminal.java`:
- Around line 500-508: feedbackVt100Response can race with
TerminalAjaxHandler.handle because both call
LineDisciplineTerminal.processInputBytes (via webTerminal.processInputBytes),
which is unsynchronized and can interleave bytes into slaveInputPipe; serialize
calls to processInputBytes by guarding them with the same lock used elsewhere
(or add a dedicated lock) so both TerminalAjaxHandler.handle and
WebTerminal.feedbackVt100Response synchronize on that lock before invoking
webTerminal.processInputBytes; update WebTerminal.feedbackVt100Response to
acquire the lock (or call through a single-threaded executor) prior to calling
processInputBytes, and apply the same pattern to
SwingTerminalOutputStream.feedbackVt100Response to ensure consistent
serialization.
---
Nitpick comments:
In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java`:
- Around line 872-874: Add a one-line Javadoc to the public method read() in
SwingTerminal describing its behavior and consistency with other
TerminalComponent methods: state that read() drains and returns pending VT100
output from the underlying ScreenTerminal (screenTerminal.read()), keeping the
surface consistent with write, dump, isDirty, pollInput, and takeInput
documentation; place the Javadoc immediately above the public String read()
method in class SwingTerminal.
- Around line 310-314: When inputBuffer fills and you call
decodeAndOutput(false) (in the buffer-full branches around
inputBuffer.hasRemaining() and the similar path at lines ~449-452), the code
writes decoded chars to component but does not call feedbackVt100Response(),
leaving vt100_out queued; update those buffer-full paths to invoke
feedbackVt100Response() after decodeAndOutput(false) so VT100 feedback is
drained immediately and behavior matches the normal decode path. Ensure you
reference the same flow used elsewhere (where decodeAndOutput(true) triggers
feedbackVt100Response()) and call feedbackVt100Response() only after
decodeAndOutput(false) completes to preserve ordering.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4d09a9e-70d4-42cd-8145-1f9f432aeb0b
📒 Files selected for processing (2)
builtins/src/main/java/org/jline/builtins/SwingTerminal.javabuiltins/src/main/java/org/jline/builtins/WebTerminal.java
| private void feedbackVt100Response() throws IOException { | ||
| if (component == null || terminal == null) { | ||
| return; | ||
| } | ||
| String response = component.read(); | ||
| if (!response.isEmpty()) { | ||
| terminal.processInputBytes(response.getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm LineDisciplineTerminal.processInputBytes has no synchronization and inspect slaveInputPipe writes.
fd -e java LineDisciplineTerminal.java --exec cat {}Repository: jline/jline3
Length of output: 14331
🏁 Script executed:
fd -e java SwingTerminal.java --exec cat {} | head -300Repository: jline/jline3
Length of output: 10183
🏁 Script executed:
fd -e java WebTerminal.java --exec cat {}Repository: jline/jline3
Length of output: 27247
Concurrent calls to processInputBytes can cause byte-level interleaving of VT100 responses with keystrokes.
feedbackVt100Response() invokes terminal.processInputBytes(...) from whichever thread invokes the output stream's write methods (typically EDT in SwingTerminal, or HTTP handler threads in WebTerminal). Meanwhile, SwingTerminal's inputThread (line 96) and WebTerminal's HTTP handlers also call processInputBytes for user input. Per LineDisciplineTerminal.processInputBytes, byte iteration writes to slaveInputPipe without synchronization, allowing a VT100 response (e.g. \e[5;10R) to interleave at the byte level with concurrent keystrokes, producing malformed sequences downstream.
Serialize feedback on the input thread or guard processInputBytes with a lock. The same concern applies to WebTerminal.WebTerminalOutputStream.feedbackVt100Response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builtins/src/main/java/org/jline/builtins/SwingTerminal.java` around lines
270 - 278, feedbackVt100Response currently calls terminal.processInputBytes(...)
from arbitrary threads (EDT/HTTP handlers) while inputThread and other handlers
also call processInputBytes, allowing byte-level interleaving; fix by
serializing all calls to LineDisciplineTerminal.processInputBytes: introduce a
dedicated lock or marshal VT100 responses onto the same input thread used by
SwingTerminal.inputThread (or WebTerminal's input dispatcher) and use that lock
when invoking terminal.processInputBytes in SwingTerminal.feedbackVt100Response
and WebTerminal.WebTerminalOutputStream.feedbackVt100Response so writes to the
LineDisciplineTerminal/slaveInputPipe are single-threaded and ordered.
…inal Verify that DSR (cursor position report) and DA (device attributes) responses from ScreenTerminal are correctly fed back as terminal input via processInputBytes(). Also tests that cursor position reports reflect actual cursor movement.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
builtins/src/test/java/org/jline/builtins/Vt100ResponseFeedbackTest.java (2)
42-122: Optional: extract common setup to reduce duplication across the 5 tests.Each test repeats: construct terminal → fetch attributes → disable
ECHO→ set attributes → write query → assert response. A small helper parameterized on aTerminalsupplier and the query/expected response would keep the test bodies focused on what they verify. Not required — purely a readability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builtins/src/test/java/org/jline/builtins/Vt100ResponseFeedbackTest.java` around lines 42 - 122, Multiple tests in Vt100ResponseFeedbackTest duplicate the same setup steps (creating a Terminal, getting Attributes, disabling Attributes.LocalFlag.ECHO, setting attributes, writing a query and asserting the response); extract a small helper method (e.g., runTerminalQuery or performTerminalQuery) that accepts a Supplier<Terminal> (or Terminal factory), the query bytes/String, and the expected response, and call this helper from webTerminalFeedsDsrResponse, webTerminalFeedsDeviceAttributesResponse, webTerminalDsrReflectsCursorPosition, swingTerminalFeedsDsrResponse, and swingTerminalFeedsDeviceAttributesResponse to centralize creating the WebTerminal/SwingTerminal, applying attrs.setLocalFlag(..., false) via terminal.getAttributes()/terminal.setAttributes(...), writing the query (terminal.output().write(...); terminal.output().flush()), draining input with readAvailable when needed, and asserting the response with readAvailable(terminal.input(), timeout); keep test-specific differences (query and expected response) as parameters so tests remain concise.
27-40:readAvailablemay drop late-arriving bytes and artificially extends drain duration.The second loop exits as soon as
available()transiently returns 0, so any bytes delivered after the initial burst (even within the timeout) will be missed. This is tolerable because the new feedback path is inline withwrite()/flush(), but it makes the helper brittle if feedback ever becomes asynchronous.Also, the drain at line 82 (
readAvailable(..., 100)) will always wait the full 100 ms because writing "Hello" is not expected to produce any response, soavailable()stays 0 for the whole deadline. Consider an early-return “no data expected” drain, or keep reading until the deadline (not until the first empty poll).♻️ Proposed refactor
private String readAvailable(InputStream in, long timeoutMs) throws Exception { long deadline = System.currentTimeMillis() + timeoutMs; - while (in.available() == 0 && System.currentTimeMillis() < deadline) { - Thread.sleep(10); - } byte[] buf = new byte[256]; int total = 0; - while (in.available() > 0 && total < buf.length) { - int n = in.read(buf, total, buf.length - total); - if (n <= 0) break; - total += n; + while (System.currentTimeMillis() < deadline && total < buf.length) { + if (in.available() > 0) { + int n = in.read(buf, total, buf.length - total); + if (n <= 0) break; + total += n; + } else if (total > 0) { + // We already got some bytes and nothing more is pending; assume response is complete. + break; + } else { + Thread.sleep(10); + } } return new String(buf, 0, total, StandardCharsets.UTF_8); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builtins/src/test/java/org/jline/builtins/Vt100ResponseFeedbackTest.java` around lines 27 - 40, The helper readAvailable currently stops reading when available() transiently returns 0 and the drain call readAvailable(..., 100) will always block the full timeout; change readAvailable to keep polling until the deadline and accumulate bytes as they arrive (do not break the read loop on a single available()==0), and add an optional flag/overload (e.g. readAvailable(InputStream in, long timeoutMs, boolean returnImmediatelyIfNoData) or readAvailableNoWait) so callers that expect no response (the drain call) can return immediately if no bytes were ever observed; update callers to use the new flag where an early-return is desired and ensure the reading loop only exits on deadline or when buffer is full.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builtins/src/test/java/org/jline/builtins/Vt100ResponseFeedbackTest.java`:
- Around line 42-122: Multiple tests in Vt100ResponseFeedbackTest duplicate the
same setup steps (creating a Terminal, getting Attributes, disabling
Attributes.LocalFlag.ECHO, setting attributes, writing a query and asserting the
response); extract a small helper method (e.g., runTerminalQuery or
performTerminalQuery) that accepts a Supplier<Terminal> (or Terminal factory),
the query bytes/String, and the expected response, and call this helper from
webTerminalFeedsDsrResponse, webTerminalFeedsDeviceAttributesResponse,
webTerminalDsrReflectsCursorPosition, swingTerminalFeedsDsrResponse, and
swingTerminalFeedsDeviceAttributesResponse to centralize creating the
WebTerminal/SwingTerminal, applying attrs.setLocalFlag(..., false) via
terminal.getAttributes()/terminal.setAttributes(...), writing the query
(terminal.output().write(...); terminal.output().flush()), draining input with
readAvailable when needed, and asserting the response with
readAvailable(terminal.input(), timeout); keep test-specific differences (query
and expected response) as parameters so tests remain concise.
- Around line 27-40: The helper readAvailable currently stops reading when
available() transiently returns 0 and the drain call readAvailable(..., 100)
will always block the full timeout; change readAvailable to keep polling until
the deadline and accumulate bytes as they arrive (do not break the read loop on
a single available()==0), and add an optional flag/overload (e.g.
readAvailable(InputStream in, long timeoutMs, boolean returnImmediatelyIfNoData)
or readAvailableNoWait) so callers that expect no response (the drain call) can
return immediately if no bytes were ever observed; update callers to use the new
flag where an early-return is desired and ensure the reading loop only exits on
deadline or when buffer is full.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c1fe9aa-2b56-4ae3-823c-0b16509f90a6
📒 Files selected for processing (1)
builtins/src/test/java/org/jline/builtins/Vt100ResponseFeedbackTest.java
1 participant
Summary
Fixes #1797 (partial — bug fix only, refactoring in a follow-up PR).
SwingTerminal and WebTerminal never fed VT100 responses back to the
LineDisciplineTerminalafter writing to theScreenTerminal. This means terminal query-response sequences like cursor position reports (DSR) and device attributes (DA) were silently lost.DisplayTest and Tmux already do this correctly: after
ScreenTerminal.write(), they callread()to collect any VT100 output and feed it back viaprocessInputBytes().Changes
read()toTerminalComponent, addfeedbackVt100Response()toSwingTerminalOutputStreamcalled after eachcomponent.write()webTerminalreference toWebTerminalOutputStream, addfeedbackVt100Response()called after eachcomponent.write()Test plan
./mvx mvn test -pl builtins— 1071 tests pass./mvx mvn test -pl terminal -Dtest=DisplayTest— 2 tests pass./mvx mvn spotless:apply -pl builtins— no formatting changesSummary by CodeRabbit
Improvements
Tests