OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

refactor(frontend): Terminal

Open amanape opened this issue 1 year ago • 1 comments

Summary

I have created a React hook (useXTerm) to act as a wrapper around the @xterm/xterm Terminal class so it is easier (and reusable) to use in the application. It exposes a ref that can be used on any div element.

READ

@iFurySt @yimothysu There was a custom plugin JsonWebsocketAddon which handled messages via Socket but I was not able to properly traceback its use (limited time today).

I did find that the terminal did output commands via the cmd state, and assumed that this is the way it is supposed to output data as I found consistent results during my manual tests.

So, this PR removes the JsonWebsocketAddon based on the above assumption. If this was a mistake, let me know. It should be a quick change as the new hook is built to accept any addons passed.

Fixes

  • Screen no longer "extends" downwards when there is output in the terminal.

Improvements

  • Replace the deprecated xterm-addon-fit with the new @xterm/addon-fit (see here)
  • Specify that it is read-only
  • Improved fitting and resizing
  • Reusable

Issues

  • Because of the nature of the used libraries, I did not build this via TDD and was unable to write tests after finishing the hook and component. I've added describe.todo()'s to come later when I have more time to dive in-depth in properly mocking the utilized API 😵

amanape avatar Apr 23 '24 18:04 amanape

LGTM

iFurySt avatar Apr 24 '24 01:04 iFurySt

@amanape JsonWebSocket was originally added in https://github.com/OpenDevin/OpenDevin/pull/71 to resolve https://github.com/OpenDevin/OpenDevin/issues/64. Can you double check there's no regression? Besides that, LGTM.

yimothysu avatar Apr 24 '24 02:04 yimothysu

I think we've since enforced the terminal to be read-only, so will proceed.

yimothysu avatar Apr 24 '24 02:04 yimothysu