fix(terminal): prevent duplicate input on shell restart#10
Conversation
Fixes open-gitagent#2 - When the agent shell restarted (e.g., after gitclaw exited), user keystrokes were duplicated because onData handlers accumulated. Typing 'hi' appeared as 'hhii'. Changes: - Register xterm.onData listener ONCE in constructor - Store callback in dataHandler property - onData() now replaces handler instead of stacking new ones
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Clean fix for the duplicate-input problem. Registering the listener once in the constructor and routing through a nullable dataHandler property is the right pattern — avoids listener stacking entirely.
A couple of things worth checking before merge:
- Initialization gap: Between the constructor completing and the first
onData()call,dataHandlerisnull. Any keystrokes during that window are silently dropped. In practice this is unlikely, but it is worth a comment or an assertion to make it explicit. - Interaction with PR #3: PR #3 (
bugfix/user-inputby shreehari-lyzr) fixes the same issue at theContainerManagerlayer by disposing thexterm.onDatadisposable. Both PRs touchterminal.tsand address the same root cause from different levels. Whichever merges first will conflict with the other — the team should decide on one approach and close the duplicate.
Overall the logic is correct and the change is minimal. Recommend merging after resolving the overlap with PR #3.
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Clean and focused fix. The approach of registering onData once in the constructor and routing through a replaceable dataHandler property is the right pattern here — it eliminates the stacking problem entirely.
A couple of things worth considering:
dataHandler starts as null: If onData() is never called before user input arrives (e.g., terminal mounted but shell not yet started), the constructor-registered listener will silently drop keystrokes. That is probably fine as a no-op, but it may be worth documenting the expected lifecycle — specifically, that onData() must be called before input is meaningful.
Single handler assumption: The public onData() API now silently replaces any previously registered handler without warning. If a future caller registers two handlers expecting both to fire (e.g., a logger alongside the shell writer), the second registration will silently swallow the first. A brief JSDoc note on the replacement semantics would prevent that surprise.
Neither of these is blocking. The fix is correct, the diff is minimal, and it directly addresses the root cause reported in #2. Good work.
Fixes #2 - When the agent shell restarted (e.g., after gitclaw exited), user keystrokes were duplicated because onData handlers accumulated. Typing 'hi' appeared as 'hhii'.
Changes: