Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #6262: [Bug]: "Success/failure" behavior is not consistent for file reading and editing #6263

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions frontend/__tests__/components/file-operations.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { render, screen } from "@testing-library/react";
import { describe, it, expect } from "vitest";
import { Messages } from "#/components/features/chat/messages";

describe("File Operations Messages", () => {
it("should show success indicator for successful file read operation", () => {
const messages = [
{
type: "action",
translationID: "read_file_contents",
content: "Successfully read file contents",
success: true,
sender: "assistant",
},
];

render(<Messages messages={messages} isAwaitingUserConfirmation={false} />);

const statusIcon = screen.getByTestId("status-icon");
expect(statusIcon).toBeInTheDocument();
expect(statusIcon.closest("svg")).toHaveClass("fill-success");
});

it("should show failure indicator for failed file read operation", () => {
const messages = [
{
type: "action",
translationID: "read_file_contents",
content: "Failed to read file contents",
success: false,
sender: "assistant",
},
];

render(<Messages messages={messages} isAwaitingUserConfirmation={false} />);

const statusIcon = screen.getByTestId("status-icon");
expect(statusIcon).toBeInTheDocument();
expect(statusIcon.closest("svg")).toHaveClass("fill-danger");
});

it("should show success indicator for successful file edit operation", () => {
const messages = [
{
type: "action",
translationID: "edit_file_contents",
content: "Successfully edited file contents",
success: true,
sender: "assistant",
},
];

render(<Messages messages={messages} isAwaitingUserConfirmation={false} />);

const statusIcon = screen.getByTestId("status-icon");
expect(statusIcon).toBeInTheDocument();
expect(statusIcon.closest("svg")).toHaveClass("fill-success");
});

it("should show failure indicator for failed file edit operation", () => {
const messages = [
{
type: "action",
translationID: "edit_file_contents",
content: "Failed to edit file contents",
success: false,
sender: "assistant",
},
];

render(<Messages messages={messages} isAwaitingUserConfirmation={false} />);

const statusIcon = screen.getByTestId("status-icon");
expect(statusIcon).toBeInTheDocument();
expect(statusIcon.closest("svg")).toHaveClass("fill-danger");
});
});
54 changes: 27 additions & 27 deletions frontend/src/components/features/chat/expandable-message.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ export function ExpandableMessage({
)}
>
<div className="text-sm w-full">
{headline && (
<div className="flex flex-row justify-between items-center w-full">
<span
className={cn(
"font-bold",
type === "error" ? "text-danger" : "text-neutral-300",
)}
>
{headline}
<div className="flex flex-row justify-between items-center w-full">
<span
className={cn(
headline ? "font-bold" : "",
type === "error" ? "text-danger" : "text-neutral-300",
)}
>
{headline}
{headline && (
<button
type="button"
onClick={() => setShowDetails(!showDetails)}
Expand All @@ -76,25 +76,25 @@ export function ExpandableMessage({
/>
)}
</button>
</span>
{type === "action" && success !== undefined && (
<span className="flex-shrink-0">
{success ? (
<CheckCircle
data-testid="status-icon"
className={cn(statusIconClasses, "fill-success")}
/>
) : (
<XCircle
data-testid="status-icon"
className={cn(statusIconClasses, "fill-danger")}
/>
)}
</span>
)}
</div>
)}
{showDetails && (
</span>
{type === "action" && success !== undefined && (
<span className="flex-shrink-0">
{success ? (
<CheckCircle
data-testid="status-icon"
className={cn(statusIconClasses, "fill-success")}
/>
) : (
<XCircle
data-testid="status-icon"
className={cn(statusIconClasses, "fill-danger")}
/>
)}
</span>
)}
</div>
{(!headline || showDetails) && (
<Markdown
className="text-sm overflow-auto"
components={{
Expand Down
9 changes: 9 additions & 0 deletions frontend/src/state/chat-slice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ export const chatSlice = createSlice({
text = `${action.payload.args.path}\n${content}`;
} else if (actionID === "browse") {
text = `Browsing ${action.payload.args.url}`;
} else if (actionID === "read") {
text = `Reading file ${action.payload.args.path}`;
} else if (actionID === "edit") {
text = `Editing file ${action.payload.args.path}`;
}
if (actionID === "run" || actionID === "run_ipython") {
if (
Expand Down Expand Up @@ -154,6 +158,11 @@ export const chatSlice = createSlice({
causeMessage.success = !ipythonObs.content
.toLowerCase()
.includes("error:");
} else if (observationID === "read" || observationID === "edit") {
// For read/edit operations, we consider it successful if there's content and no error
causeMessage.success =
observation.payload.content.length > 0 &&
!observation.payload.content.toLowerCase().includes("error:");
}

if (observationID === "run" || observationID === "run_ipython") {
Expand Down
Loading