From 66d0cd85ffa92127cde2dbfdba6b28c1ae9bba3f Mon Sep 17 00:00:00 2001 From: medic-bot Date: Mon, 9 Mar 2026 21:02:24 -0400 Subject: [PATCH] fix: address reviewer feedback (#23) - Extract reorderRows into shared utility (src/utils/reorderRows.ts) to deduplicate identical function in BudgetTable and BudgetVsActualTable - Restore alphabetical sorting of children in budgetService.ts - Fix styling for intermediate parent rows at depth 2+ (was only handling depth 0-1) - Reduce pie chart size (height 220->180, radii reduced) and padding to give more space to the table Co-Authored-By: Claude Opus 4.6 --- src/components/budget/BudgetTable.tsx | 47 +++---------------- src/components/dashboard/CategoryPieChart.tsx | 12 ++--- .../reports/BudgetVsActualTable.tsx | 44 ++--------------- src/services/budgetService.ts | 8 ++-- src/utils/reorderRows.ts | 38 +++++++++++++++ 5 files changed, 61 insertions(+), 88 deletions(-) create mode 100644 src/utils/reorderRows.ts diff --git a/src/components/budget/BudgetTable.tsx b/src/components/budget/BudgetTable.tsx index 60c72d1..aa92e4f 100644 --- a/src/components/budget/BudgetTable.tsx +++ b/src/components/budget/BudgetTable.tsx @@ -2,6 +2,7 @@ import { useState, useRef, useEffect, Fragment } from "react"; import { useTranslation } from "react-i18next"; import { AlertTriangle, ArrowUpDown } from "lucide-react"; import type { BudgetYearRow } from "../../shared/types"; +import { reorderRows } from "../../utils/reorderRows"; const fmt = new Intl.NumberFormat("en-CA", { style: "currency", @@ -18,42 +19,6 @@ const MONTH_KEYS = [ const STORAGE_KEY = "subtotals-position"; -function reorderRows( - rows: T[], - subtotalsOnTop: boolean, -): T[] { - if (subtotalsOnTop) return rows; - - // 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 = []; - } - currentParent = row; - } else if (currentParent) { - currentChildren.push(row); - } else { - result.push(row); - } - } - // Flush last group - if (currentParent) { - result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent); - } - return result; - } - - return reorderGroup(rows, 0); -} - interface BudgetTableProps { rows: BudgetYearRow[]; onUpdatePlanned: (categoryId: number, month: number, amount: number) => void; @@ -214,13 +179,15 @@ export default function BudgetTable({ rows, onUpdatePlanned, onSplitEvenly }: Bu if (row.is_parent) { // Parent subtotal row: read-only, bold, distinct background const parentDepth = row.depth ?? 0; - const isIntermediateParent = parentDepth === 1; + const isTopParent = parentDepth === 0; + const isIntermediateParent = parentDepth >= 1; + const parentPaddingClass = parentDepth >= 3 ? "pl-20 pr-3" : parentDepth === 2 ? "pl-14 pr-3" : parentDepth === 1 ? "pl-8 pr-3" : "px-3"; return ( - +
{/* Category name - sticky */} - + = 3 ? "pl-20 pr-3" : depth === 2 ? "pl-14 pr-3" : depth === 1 ? "pl-8 pr-3" : "px-3"}`}>
-

{t("dashboard.noData")}

+
+

{t("dashboard.noData")}

); } return ( -
+
{hiddenCategories.size > 0 && (
{t("charts.hiddenCategories")}: @@ -72,7 +72,7 @@ export default function CategoryPieChart({ onMouseEnter={() => setIsChartHovered(true)} onMouseLeave={() => setIsChartHovered(false)} > - + {visibleData.map((item, index) => ( diff --git a/src/components/reports/BudgetVsActualTable.tsx b/src/components/reports/BudgetVsActualTable.tsx index 2162f05..a7ef69f 100644 --- a/src/components/reports/BudgetVsActualTable.tsx +++ b/src/components/reports/BudgetVsActualTable.tsx @@ -2,6 +2,7 @@ import { Fragment, useState } from "react"; import { useTranslation } from "react-i18next"; import { ArrowUpDown } from "lucide-react"; import type { BudgetVsActualRow } from "../../shared/types"; +import { reorderRows } from "../../utils/reorderRows"; const cadFormatter = (value: number) => new Intl.NumberFormat("en-CA", { @@ -25,42 +26,6 @@ interface BudgetVsActualTableProps { const STORAGE_KEY = "subtotals-position"; -function reorderRows( - rows: T[], - subtotalsOnTop: boolean, -): T[] { - if (subtotalsOnTop) return rows; - - // 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 = []; - } - currentParent = row; - } else if (currentParent) { - currentChildren.push(row); - } else { - result.push(row); - } - } - // Flush last group - if (currentParent) { - result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent); - } - return result; - } - - return reorderGroup(rows, 0); -} - export default function BudgetVsActualTable({ data }: BudgetVsActualTableProps) { const { t } = useTranslation(); const [subtotalsOnTop, setSubtotalsOnTop] = useState(() => { @@ -201,17 +166,18 @@ export default function BudgetVsActualTable({ data }: BudgetVsActualTableProps) {reorderRows(section.rows, subtotalsOnTop).map((row) => { const isParent = row.is_parent; const depth = row.depth ?? (row.parent_id !== null && !row.is_parent ? 1 : 0); - const isIntermediateParent = isParent && depth === 1; + const isTopParent = isParent && depth === 0; + const isIntermediateParent = isParent && depth >= 1; const paddingClass = depth >= 3 ? "pl-20" : depth === 2 ? "pl-14" : depth === 1 ? "pl-8" : "px-3"; return ( - + a.name.localeCompare(b.name)); + for (const child of sortedSubChildren) { const grandchildren = childrenByParent.get(child.id) || []; if (grandchildren.length > 0) { const subRows = buildSubGroup(child, cat.id, depth + 1); @@ -381,8 +382,9 @@ export async function getBudgetVsActualData( if (!isRowAllZero(direct)) allChildRows.push(direct); } - // Process all children in sort order (preserves tree structure) - for (const child of children) { + // Process children in alphabetical order + const sortedChildren = [...children].sort((a, b) => a.name.localeCompare(b.name)); + for (const child of sortedChildren) { const grandchildren = childrenByParent.get(child.id) || []; if (grandchildren.length > 0) { const subRows = buildSubGroup(child, cat.id, 1); diff --git a/src/utils/reorderRows.ts b/src/utils/reorderRows.ts new file mode 100644 index 0000000..4ac0181 --- /dev/null +++ b/src/utils/reorderRows.ts @@ -0,0 +1,38 @@ +/** + * Shared utility for reordering budget table rows. + * Recursively moves subtotal (parent) rows below their children + * at every depth level when "subtotals on bottom" is enabled. + */ +export function reorderRows< + T extends { is_parent: boolean; parent_id: number | null; category_id: number; depth?: number }, +>(rows: T[], subtotalsOnTop: boolean): T[] { + if (subtotalsOnTop) return rows; + + 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 = []; + } + currentParent = row; + } else if (currentParent) { + currentChildren.push(row); + } else { + result.push(row); + } + } + // Flush last group + if (currentParent) { + result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent); + } + return result; + } + + return reorderGroup(rows, 0); +}