Fix double input in terminal by disposing previous onData listeners#3
Fix double input in terminal by disposing previous onData listeners#3shreehari-lyzr wants to merge 1 commit into
Conversation
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Good fix — tracking the onData disposable in ContainerManager and calling dispose() before each new registration is the correct, idiomatic approach for xterm.js.
A few observations:
Overlap with PR #10
PR #10 (by tsj2003) solves the same problem from a different layer: it registers a single listener in TerminalManager's constructor and swaps out the callback via a stored property. Both approaches work, but they should not both merge — pick one. This PR's approach (dispose at the call site in ContainerManager) is more explicit about ownership and leaves TerminalManager as a thin wrapper, which is arguably cleaner. PR #10's approach moves the logic into TerminalManager and hides it from callers, which is also defensible. The team should align on one and close the other.
Three separate fix points
The disposable pattern is applied to three separate call sites (startGitclaw, startShell, startAgent). If a fourth process type is added later, it is easy to forget to apply the same pattern. Consider extracting the register-with-dispose logic into a helper on ContainerManager to make the pattern self-enforcing.
terminal.ts change
The return type change on TerminalManager.onData (now returns { dispose(): void }) is a good API improvement regardless of which PR wins — the original void return was the root enabler of this bug. That change is worth keeping.
This PR is otherwise clean and minimal. Once the overlap with PR #10 is resolved, it should be ready to merge.
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Good approach — tracking the disposable and calling dispose() before each new registration cleanly prevents listener accumulation. This is the idiomatic xterm.js pattern for this problem.
A few observations:
Single shared disposable for three code paths: onDataDisposable is a single field, but startGitclaw, startShell, and startAgent (lines ~370, ~420, ~596) all write to it. If any two of those can be active simultaneously — e.g., a shell and an agent running concurrently — the second call will dispose the first shell's listener. Worth confirming whether these three paths are mutually exclusive at runtime. If they are, this is fine; if not, you'll need a disposable per terminal/process.
terminal.onData return type in terminal.ts: The change correctly makes onData() return { dispose(): void } by proxying through to xterm.onData. This is the right signature. Just note that this tightens the contract for any other call sites that currently ignore the return value — make sure nothing else calls terminal.onData() and discards the result expecting the old void return.
PR #10 overlap: There is a parallel fix (PR #10) addressing the same underlying issue from the TerminalManager side. These two PRs touch overlapping files (terminal.ts) with different approaches. Merging both may produce a double-layer fix or a conflict — the team should decide which approach owns this and close or rebase the other before merging.
When startGitclaw/startShell/startAgent restarted (e.g. after process
exit), a new xterm onData handler was registered without removing the
previous one. Each restart stacked another listener, causing every
keystroke to be written to stdin multiple times ("hi" → "hhii").
Track the onData disposable in ContainerManager and dispose it before
each new registration.