OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

feat(frontend):Display path of file ops and cmd in headline

Open happyherp opened this issue 9 months ago • 2 comments

  • [x] Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below The paths of edited files are now displayed in the headline. So are the first 80 chars of bash-commands.

End-user friendly description of the problem this fixes or functionality that this introduces. The paths of edited files are now displayed in the headline. So are the first 80 chars of bash-commands.


Give a summary of what the PR does, explaining any non-trivial design decisions.

Changed the translation keys to include the path. "Running <cmd>{{action.payload.args.command}}</cmd> To achieve this, the observation and command objects are passed to expandable-message component and from there to the translation method. Trim action.payload.args.command down to 80 chars. Some commands can be very long. To make the paths/commands bold and to handle an escaping issue, MonoComponent was created. Example:

<Trans
                  i18nKey={translationId}
                  values={translationParams}
                  components={{
                    bold: <strong />,
                    path: <MonoComponent />,
                    cmd: <MonoComponent />
                  }}
                />

image


Link of any specific issues this addresses.

https://github.com/happyherp/OpenHands/issues/4

happyherp avatar Mar 26 '25 17:03 happyherp

@happyherp can we just show the base filename, instead of the whole path? would be great if we can put the full path in the title attribute as well

e.g. Read pyproject.toml, and then if you hover over pyproject.toml you can see the full path (thanks to the title attribute)

For directories, I wonder if we can say Read directory vs Read file?

rbren avatar Mar 27 '25 19:03 rbren

@happyherp can we just show the base filename, instead of the whole path? would be great if we can put the full path in the title attribute as well

e.g. Read pyproject.toml, and then if you hover over pyproject.toml you can see the full path (thanks to the title attribute)

For directories, I wonder if we can say Read directory vs Read file?

That totally makes sense from a feature perspective. But when planning it, I began to think that a refactoring of the messages-component is in order.

It seems, the chat was build assuming that "messages"(observation, action) would be handled pretty uniformly: display the content, translate the type for the headline.

But we are starting to make customizations to these messages that are type dependent. These are all over the place right now. A lot in chat-slize.ts while updating the state:

      let text = "";
      if (actionID === "run") {
        text = `Command:\n\`${action.payload.args.command}\``;
      } else if (actionID === "run_ipython") {
        text = `\`\`\`\n${action.payload.args.code}\n\`\`\``;
      } else if (actionID === "write") {
        let { content } = action.payload.args;
        if (content.length > MAX_CONTENT_LENGTH) {
          content = `${content.slice(0, MAX_CONTENT_LENGTH)}...`;
        }
        text = `${action.payload.args.path}\n${content}`;
      }

some in expandable-message.tsx


 if (
    config?.FEATURE_FLAGS.ENABLE_BILLING &&
    config?.APP_MODE === "saas" &&
    id === "STATUS$ERROR_LLM_OUT_OF_CREDITS"
  ) {
    return (
      <div
        data-testid="out-of-credits"
        className="flex gap-2 items-center justify-start border-l-2 pl-2 my-2 py-2 border-danger"
      >
        <div className="text-sm w-full">
          <div className="font-bold text-danger">
            {t("STATUS$ERROR_LLM_OUT_OF_CREDITS")}
          </div>
          <Link
            className="mt-2 mb-2 w-full h-10 rounded flex items-center justify-center gap-2 bg-primary text-[#0D0F11]"
            to="/settings/billing"
          >
            {t("BILLING$CLICK_TO_TOP_UP")}
          </Link>
        </div>
      </div>
    );
  }

My estimation is, that we will want to make more changes to functionality of messages that are dependent on their type. If would keep going this way, we would end up with a very long list of if (type=="abc") type statements that will get hard to manage.

So I thought I will introduce some separation of concerns.

Make ExpandableMessage content-agnostic: Just a Component that manages a headline with a content that is hidden by default.

Create a class hierarchy of message formatters, with a base class that does the default(translate headline by type, content as is), with extension points(buildTitle, buildContent) that can then be overridden by subclasses.

export class ReadObservationFormatter extends DefaultObservationFormatter {
  constructor(props: ObservationFormatterProps) {
    super(props);
  }

  override _makeContent(): string {
    const { observation } = this.props;
    // For read observations, we format the content as code
    return `\`\`\`\n${observation.payload.content}\n\`\`\``;
  }
  
  /**
   * For read operations, we consider it successful if there's content and no error
   */
  override determineSuccess(): boolean {
    const { observation } = this.props;
    const content = observation.payload.content;
    
    if (observation.payload.extras.impl_source === "oh_aci") {
      return content.length > 0 && !content.startsWith("ERROR:\n");
    } else {
      return content.length > 0 && !content.toLowerCase().includes("error:");
    }
  }

So I began a refactoring branch that turned into a mess. I don't want to look at it for a while.

Can we merge the current version? I maybe somebody else wants to try their hands at this refactoring.

happyherp avatar Mar 29 '25 10:03 happyherp

@happyherp looks like lint and tests just need to get fixed

Just to be clear--the current PR would show the full filename and command, but trimmed to 80 chars?

rbren avatar Apr 07 '25 20:04 rbren

I did manage to at least just display filename always, with path on hover:

image

happyherp avatar Apr 08 '25 07:04 happyherp

Excited to have this in! Thanks for contributing

rbren avatar Apr 08 '25 12:04 rbren