From b8116d32ebb36f57c086c2da4663ad3f65e9d9e0 Mon Sep 17 00:00:00 2001
From: Dave <david.els1@outlook.com>
Date: Thu, 1 Oct 2026 08:32:04 +0200
Subject: [PATCH] fix(giellie): pin Approve/Reject above a scrolling list on
 long approvals

A long batch (e.g. 16 stock_items.create) made the approval card taller than
the page; it sits outside the message scroller, so the Approve button was
clipped and unreachable. The card now has a bounded height (60dvh), a pinned
header holding Approve/Reject, and only the list of changes scrolls.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
---
 src/components/giellie/approval-card.test.tsx | 24 +++++++++++++++
 src/components/giellie/approval-card.tsx      | 30 +++++++++++--------
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/src/components/giellie/approval-card.test.tsx b/src/components/giellie/approval-card.test.tsx
index 77a3194..529ed67 100644
--- a/src/components/giellie/approval-card.test.tsx
+++ b/src/components/giellie/approval-card.test.tsx
@@ -64,6 +64,30 @@ describe("ApprovalCard", () => {
     expect(onDecide).toHaveBeenNthCalledWith(2, false);
   });
 
+  it("keeps Approve/Reject pinned outside a scrolling list on a long batch (16 changes)", () => {
+    const many: PendingAction = {
+      tool_uses: Array.from({ length: 16 }, (_, i) => ({
+        id: String(i),
+        name: "stock_items.create",
+        input: { name: `ITEM ${i}`, cost_price: 5.8, markup: 80 },
+        confirmAlways: false,
+      })),
+      completed: [],
+      order: Array.from({ length: 16 }, (_, i) => String(i)),
+    };
+    render(<ApprovalCard pending={many} busy={false} onDecide={() => {}} />);
+    const list = screen.getByTestId("approval-list");
+    const approve = screen.getByRole("button", { name: /approve/i });
+    const reject = screen.getByRole("button", { name: /reject/i });
+    // The list is the only scroller, and the buttons are not inside it …
+    expect(list.className).toContain("overflow-y-auto");
+    expect(list.contains(approve)).toBe(false);
+    expect(list.contains(reject)).toBe(false);
+    // … and they come BEFORE it, so a tall list can never push them off-screen.
+    expect(approve.compareDocumentPosition(list) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy();
+    expect(screen.getByText("16 changes", { exact: false })).toBeTruthy();
+  });
+
   it("disables both buttons while busy", () => {
     render(<ApprovalCard pending={pending} busy onDecide={() => {}} />);
     expect((screen.getByRole("button", { name: /approve/i }) as HTMLButtonElement).disabled).toBe(true);
diff --git a/src/components/giellie/approval-card.tsx b/src/components/giellie/approval-card.tsx
index dadbb78..7e7fcf8 100644
--- a/src/components/giellie/approval-card.tsx
+++ b/src/components/giellie/approval-card.tsx
@@ -12,11 +12,23 @@ interface Props {
 
 export function ApprovalCard({ pending, busy, onDecide }: Props) {
   return (
-    <div className="rounded-lg border border-amber-400/60 bg-amber-50 dark:bg-amber-950/30 p-3 space-y-3">
-      <p className="text-sm font-medium">
-        Giellie wants to make {pending.tool_uses.length === 1 ? "this change" : `${pending.tool_uses.length} changes`}:
-      </p>
-      <ul className="space-y-2">
+    // Bounded height + pinned header: with a long batch (e.g. a 16-item price
+    // list) the Approve/Reject buttons must stay on screen while only the list
+    // of changes scrolls. The card sits outside the message scroller, so an
+    // unbounded card used to push its own buttons out of the clipped page.
+    <div className="flex max-h-[60dvh] flex-col rounded-lg border border-amber-400/60 bg-amber-50 dark:bg-amber-950/30">
+      <div className="flex shrink-0 flex-wrap items-center gap-2 border-b border-amber-400/40 p-3">
+        <p className="mr-auto text-sm font-medium">
+          Giellie wants to make {pending.tool_uses.length === 1 ? "this change" : `${pending.tool_uses.length} changes`}:
+        </p>
+        <Button size="sm" disabled={busy} onClick={() => onDecide(true)}>
+          <Check className="mr-1 h-4 w-4" /> Approve
+        </Button>
+        <Button size="sm" variant="outline" disabled={busy} onClick={() => onDecide(false)}>
+          <X className="mr-1 h-4 w-4" /> Reject
+        </Button>
+      </div>
+      <ul data-testid="approval-list" className="min-h-0 flex-1 space-y-2 overflow-y-auto p-3">
         {pending.tool_uses.map((tu) => (
           <li key={tu.id} className="rounded-md border bg-background p-2">
             <div className="flex items-center gap-2 text-sm font-mono font-semibold">
@@ -33,14 +45,6 @@ export function ApprovalCard({ pending, busy, onDecide }: Props) {
           </li>
         ))}
       </ul>
-      <div className="flex gap-2">
-        <Button size="sm" disabled={busy} onClick={() => onDecide(true)}>
-          <Check className="mr-1 h-4 w-4" /> Approve
-        </Button>
-        <Button size="sm" variant="outline" disabled={busy} onClick={() => onDecide(false)}>
-          <X className="mr-1 h-4 w-4" /> Reject
-        </Button>
-      </div>
     </div>
   );
 }