From 10c513f4d5a0d9fc557d6696e8ee3921d268ba32 Mon Sep 17 00:00:00 2001
From: openhands <openhands@all-hands.dev>
Date: Tue, 14 Jan 2025 09:12:00 +0000
Subject: [PATCH 1/2] Fix issue #6262: [Bug]: "Success/failure" behavior is not
 consistent for file reading and editing

---
 .../components/file-operations.test.tsx       | 77 +++++++++++++++++++
 .../features/chat/expandable-message.tsx      | 54 ++++++-------
 frontend/src/state/chat-slice.ts              |  8 ++
 3 files changed, 112 insertions(+), 27 deletions(-)
 create mode 100644 frontend/__tests__/components/file-operations.test.tsx

diff --git a/frontend/__tests__/components/file-operations.test.tsx b/frontend/__tests__/components/file-operations.test.tsx
new file mode 100644
index 000000000000..8a0a3189f20c
--- /dev/null
+++ b/frontend/__tests__/components/file-operations.test.tsx
@@ -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");
+  });
+});
diff --git a/frontend/src/components/features/chat/expandable-message.tsx b/frontend/src/components/features/chat/expandable-message.tsx
index f04c9c428354..8ead346dd9ec 100644
--- a/frontend/src/components/features/chat/expandable-message.tsx
+++ b/frontend/src/components/features/chat/expandable-message.tsx
@@ -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)}
@@ -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={{
diff --git a/frontend/src/state/chat-slice.ts b/frontend/src/state/chat-slice.ts
index 5bfffb62d4b3..b0f29f494bfc 100644
--- a/frontend/src/state/chat-slice.ts
+++ b/frontend/src/state/chat-slice.ts
@@ -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 (
@@ -154,6 +158,10 @@ 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") {

From f33afe929f4731f5ba1eaa79dfd21d0e44d070c1 Mon Sep 17 00:00:00 2001
From: openhands <openhands@all-hands.dev>
Date: Tue, 14 Jan 2025 09:18:58 +0000
Subject: [PATCH 2/2] Fix pr #6263: Fix issue #6262: [Bug]: "Success/failure"
 behavior is not consistent for file reading and editing

---
 frontend/src/state/chat-slice.ts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/frontend/src/state/chat-slice.ts b/frontend/src/state/chat-slice.ts
index b0f29f494bfc..094f04d82f0f 100644
--- a/frontend/src/state/chat-slice.ts
+++ b/frontend/src/state/chat-slice.ts
@@ -160,7 +160,8 @@ export const chatSlice = createSlice({
           .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 &&
+        causeMessage.success =
+          observation.payload.content.length > 0 &&
           !observation.payload.content.toLowerCase().includes("error:");
       }