From 4a5b5fb5fe3e5ca4d03ae934226b1e31c64a75b6 Mon Sep 17 00:00:00 2001
From: medic-bot <medic-bot@lacompagniemaximus.com>
Date: Mon, 9 Mar 2026 20:46:00 -0400
Subject: [PATCH] fix: address reviewer feedback (#23)

- Make reorderRows() recursive to support subtotals toggle at all depth
  levels (not just depth 0-1)
- Restore pie chart legend (show on hover only to save space)
- Give more horizontal space to budget table (3/4 grid vs 2/3)
---
 src/components/budget/BudgetTable.tsx         | 64 +++++++------------
 src/components/dashboard/CategoryPieChart.tsx | 34 +++++++++-
 .../reports/BudgetVsActualTable.tsx           | 61 +++++++-----------
 src/pages/DashboardPage.tsx                   |  4 +-
 4 files changed, 82 insertions(+), 81 deletions(-)

diff --git a/src/components/budget/BudgetTable.tsx b/src/components/budget/BudgetTable.tsx
index 6ed81b7..60c72d1 100644
--- a/src/components/budget/BudgetTable.tsx
+++ b/src/components/budget/BudgetTable.tsx
@@ -23,51 +23,35 @@ function reorderRows<T extends { is_parent: boolean; parent_id: number | null; c
   subtotalsOnTop: boolean,
 ): T[] {
   if (subtotalsOnTop) return rows;
-  // Group depth-0 parents with all their descendants, then move subtotals to bottom
-  const groups: { parent: T | null; children: T[] }[] = [];
-  let current: { parent: T | null; children: T[] } | null = null;
-  for (const row of rows) {
-    if (row.is_parent && (row.depth ?? 0) === 0) {
-      if (current) groups.push(current);
-      current = { parent: row, children: [] };
-    } else if (current) {
-      current.children.push(row);
-    } else {
-      if (current) groups.push(current);
-      current = { parent: null, children: [row] };
-    }
-  }
-  if (current) groups.push(current);
-  return groups.flatMap(({ parent, children }) => {
-    if (!parent) return children;
-    // Also move intermediate subtotals (depth-1 parents) to bottom of their sub-groups
-    const reorderedChildren: T[] = [];
-    let subParent: T | null = null;
-    const subChildren: T[] = [];
-    for (const child of children) {
-      if (child.is_parent && (child.depth ?? 0) === 1) {
-        // Flush previous sub-group
-        if (subParent) {
-          reorderedChildren.push(...subChildren, subParent);
-          subChildren.length = 0;
+
+  // Recursively move subtotal (parent) rows below their children at every depth level
+  function reorderGroup(groupRows: T[], parentDepth: number): T[] {
+    const result: T[] = [];
+    let currentParent: T | null = null;
+    let currentChildren: T[] = [];
+
+    for (const row of groupRows) {
+      if (row.is_parent && (row.depth ?? 0) === parentDepth) {
+        // Flush previous group
+        if (currentParent) {
+          result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent);
+          currentChildren = [];
         }
-        subParent = child;
-      } else if (subParent && child.parent_id === subParent.category_id) {
-        subChildren.push(child);
+        currentParent = row;
+      } else if (currentParent) {
+        currentChildren.push(row);
       } else {
-        if (subParent) {
-          reorderedChildren.push(...subChildren, subParent);
-          subParent = null;
-          subChildren.length = 0;
-        }
-        reorderedChildren.push(child);
+        result.push(row);
       }
     }
-    if (subParent) {
-      reorderedChildren.push(...subChildren, subParent);
+    // Flush last group
+    if (currentParent) {
+      result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent);
     }
-    return [...reorderedChildren, parent];
-  });
+    return result;
+  }
+
+  return reorderGroup(rows, 0);
 }
 
 interface BudgetTableProps {
diff --git a/src/components/dashboard/CategoryPieChart.tsx b/src/components/dashboard/CategoryPieChart.tsx
index 328f78f..44e7fbd 100644
--- a/src/components/dashboard/CategoryPieChart.tsx
+++ b/src/components/dashboard/CategoryPieChart.tsx
@@ -3,7 +3,7 @@ import { useTranslation } from "react-i18next";
 import { PieChart, Pie, Cell, ResponsiveContainer, Tooltip } from "recharts";
 import { Eye } from "lucide-react";
 import type { CategoryBreakdownItem } from "../../shared/types";
-import { ChartPatternDefs, getPatternFill } from "../../utils/chartPatterns";
+import { ChartPatternDefs, getPatternFill, PatternSwatch } from "../../utils/chartPatterns";
 import ChartContextMenu from "../shared/ChartContextMenu";
 
 interface CategoryPieChartProps {
@@ -23,6 +23,7 @@ export default function CategoryPieChart({
 }: CategoryPieChartProps) {
   const { t } = useTranslation();
   const hoveredRef = useRef<CategoryBreakdownItem | null>(null);
+  const [isChartHovered, setIsChartHovered] = useState(false);
   const [contextMenu, setContextMenu] = useState<{ x: number; y: number; item: CategoryBreakdownItem } | null>(null);
 
   const visibleData = data.filter((d) => !hiddenCategories.has(d.category_name));
@@ -66,7 +67,11 @@ export default function CategoryPieChart({
         </div>
       )}
 
-      <div onContextMenu={handleContextMenu}>
+      <div
+        onContextMenu={handleContextMenu}
+        onMouseEnter={() => setIsChartHovered(true)}
+        onMouseLeave={() => setIsChartHovered(false)}
+      >
         <ResponsiveContainer width="100%" height={220}>
           <PieChart>
             <ChartPatternDefs
@@ -112,6 +117,31 @@ export default function CategoryPieChart({
         </ResponsiveContainer>
       </div>
 
+      <div
+        className={`flex flex-wrap gap-x-4 gap-y-1 mt-2 transition-opacity duration-200 ${isChartHovered ? "opacity-100" : "opacity-0 pointer-events-none"}`}
+      >
+        {data.map((item, index) => {
+          const isHidden = hiddenCategories.has(item.category_name);
+          return (
+            <button
+              key={index}
+              className={`flex items-center gap-1.5 text-sm ${isHidden ? "opacity-40" : ""}`}
+              onContextMenu={(e) => {
+                e.preventDefault();
+                setContextMenu({ x: e.clientX, y: e.clientY, item });
+              }}
+              onClick={() => isHidden ? onToggleHidden(item.category_name) : undefined}
+              title={isHidden ? t("charts.clickToShow") : undefined}
+            >
+              <PatternSwatch index={index} color={item.category_color} prefix="cat-pie" />
+              <span className="text-[var(--muted-foreground)]">
+                {item.category_name} {total > 0 && !isHidden ? `${Math.round((item.total / total) * 100)}%` : ""}
+              </span>
+            </button>
+          );
+        })}
+      </div>
+
       {contextMenu && (
         <ChartContextMenu
           x={contextMenu.x}
diff --git a/src/components/reports/BudgetVsActualTable.tsx b/src/components/reports/BudgetVsActualTable.tsx
index 8bec299..2162f05 100644
--- a/src/components/reports/BudgetVsActualTable.tsx
+++ b/src/components/reports/BudgetVsActualTable.tsx
@@ -30,48 +30,35 @@ function reorderRows<T extends { is_parent: boolean; parent_id: number | null; c
   subtotalsOnTop: boolean,
 ): T[] {
   if (subtotalsOnTop) return rows;
-  const groups: { parent: T | null; children: T[] }[] = [];
-  let current: { parent: T | null; children: T[] } | null = null;
-  for (const row of rows) {
-    if (row.is_parent && (row.depth ?? 0) === 0) {
-      if (current) groups.push(current);
-      current = { parent: row, children: [] };
-    } else if (current) {
-      current.children.push(row);
-    } else {
-      if (current) groups.push(current);
-      current = { parent: null, children: [row] };
-    }
-  }
-  if (current) groups.push(current);
-  return groups.flatMap(({ parent, children }) => {
-    if (!parent) return children;
-    const reorderedChildren: T[] = [];
-    let subParent: T | null = null;
-    const subChildren: T[] = [];
-    for (const child of children) {
-      if (child.is_parent && (child.depth ?? 0) === 1) {
-        if (subParent) {
-          reorderedChildren.push(...subChildren, subParent);
-          subChildren.length = 0;
+
+  // Recursively move subtotal (parent) rows below their children at every depth level
+  function reorderGroup(groupRows: T[], parentDepth: number): T[] {
+    const result: T[] = [];
+    let currentParent: T | null = null;
+    let currentChildren: T[] = [];
+
+    for (const row of groupRows) {
+      if (row.is_parent && (row.depth ?? 0) === parentDepth) {
+        // Flush previous group
+        if (currentParent) {
+          result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent);
+          currentChildren = [];
         }
-        subParent = child;
-      } else if (subParent && child.parent_id === subParent.category_id) {
-        subChildren.push(child);
+        currentParent = row;
+      } else if (currentParent) {
+        currentChildren.push(row);
       } else {
-        if (subParent) {
-          reorderedChildren.push(...subChildren, subParent);
-          subParent = null;
-          subChildren.length = 0;
-        }
-        reorderedChildren.push(child);
+        result.push(row);
       }
     }
-    if (subParent) {
-      reorderedChildren.push(...subChildren, subParent);
+    // Flush last group
+    if (currentParent) {
+      result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent);
     }
-    return [...reorderedChildren, parent];
-  });
+    return result;
+  }
+
+  return reorderGroup(rows, 0);
 }
 
 export default function BudgetVsActualTable({ data }: BudgetVsActualTableProps) {
diff --git a/src/pages/DashboardPage.tsx b/src/pages/DashboardPage.tsx
index 89e00e9..d3b1fd0 100644
--- a/src/pages/DashboardPage.tsx
+++ b/src/pages/DashboardPage.tsx
@@ -126,7 +126,7 @@ export default function DashboardPage() {
         ))}
       </div>
 
-      <div className="grid grid-cols-1 lg:grid-cols-3 gap-4 mb-6">
+      <div className="grid grid-cols-1 lg:grid-cols-4 gap-4 mb-6">
         <div className="lg:col-span-1">
           <h2 className="text-lg font-semibold mb-3">{t("dashboard.expensesByCategory")}</h2>
           <CategoryPieChart
@@ -137,7 +137,7 @@ export default function DashboardPage() {
             onViewDetails={viewDetails}
           />
         </div>
-        <div className="lg:col-span-2">
+        <div className="lg:col-span-3">
           <h2 className="text-lg font-semibold mb-3">{t("dashboard.budgetVsActual")}</h2>
           <BudgetVsActualTable data={budgetVsActual} />
         </div>