feat(frontend):Display path of file ops and cmd in headline
- [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 />
}}
/>
Link of any specific issues this addresses.
https://github.com/happyherp/OpenHands/issues/4
@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?
@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
titleattribute as welle.g.
Read pyproject.toml, and then if you hover overpyproject.tomlyou can see the full path (thanks to thetitleattribute)For directories, I wonder if we can say
Read directoryvsRead 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 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?
I did manage to at least just display filename always, with path on hover:
Excited to have this in! Thanks for contributing