Skip to content

fix(terminal): prevent duplicate input on shell restart#10

Open
tsj2003 wants to merge 1 commit into
open-gitagent:mainfrom
tsj2003:main
Open

fix(terminal): prevent duplicate input on shell restart#10
tsj2003 wants to merge 1 commit into
open-gitagent:mainfrom
tsj2003:main

Conversation

@tsj2003
Copy link
Copy Markdown

@tsj2003 tsj2003 commented Apr 3, 2026

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:

  • Register xterm.onData listener ONCE in constructor
  • Store callback in dataHandler property
  • onData() now replaces handler instead of stacking new ones

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
Copy link
Copy Markdown
Contributor

@shreyas-lyzr shreyas-lyzr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Initialization gap: Between the constructor completing and the first onData() call, dataHandler is null. 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.
  2. Interaction with PR #3: PR #3 (bugfix/user-input by shreehari-lyzr) fixes the same issue at the ContainerManager layer by disposing the xterm.onData disposable. Both PRs touch terminal.ts and 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.

Copy link
Copy Markdown
Contributor

@shreyas-lyzr shreyas-lyzr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User's input message inputs double characters

2 participants