From 4bad40868cbc82a12f6f4c92351846c652468cde Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 14:39:39 -0500 Subject: [PATCH] refactor(frontend): actually reduce Customers' complexity rather than relocate it (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit claimed this one fixed. It was not: hoisting the confirm dialog to module level moved the reported line from 12 to 60, and I read that as the finding having moved to the hoisted function. It had not — line 60 was Customers() itself, still scoring exactly 16. The scan is what caught it, which is the argument for scanning rather than reasoning about what a rule will say. Two further attempts also failed to move the number, and both are worth recording because they were wrong about what cognitive complexity counts. Collapsing five branches on `disabling` into one copy object fixed the hoisted function but left Customers() at 16. Extracting ten ternaries out of the table's cell renderers into module-level components left it at 16 as well — the ternaries inside a render callback were never the weight. What actually carried the score was the drawer and the reserved-items dialog: two JSX bodies whose loading, empty and populated states are each a branch nested several levels inside the component. Extracting them as CustomerDetailPanel and ReservedItemsBody takes Customers() under the limit. The cell-renderer extraction is kept even though it did not move the metric. NameAndEmail, BooleanTag, ReservedCell and ToggleDisabledButton read better than the inline callbacks did, and BooleanTag removes a repetition the Status column was open-coding differently from Verified and Subscribed. Verified on a scan rather than by argument: technical debt 85 minutes to 5, code smells 14 to 1, and the one that remains is the S6478 false positive. ESLint holds at 31 warnings against a baseline of 35. End-to-end 83 pass. One thing found on the way: the e2e suite is not idempotent against a persistent database. Two runs against a database that had already served three produced two different pairs of failures; recreating it produced a clean 83. CI is unaffected because its Postgres is fresh per run, but locally the suite needs a new database rather than a repeated one. Refs #81 Co-Authored-By: Claude Opus 5 --- frontend/src/admin/Customers.tsx | 438 +++++++++++++++++++------------ 1 file changed, 271 insertions(+), 167 deletions(-) diff --git a/frontend/src/admin/Customers.tsx b/frontend/src/admin/Customers.tsx index e9a3ad1..f9a9f7f 100755 --- a/frontend/src/admin/Customers.tsx +++ b/frontend/src/admin/Customers.tsx @@ -9,53 +9,260 @@ import { const { Title, Text } = Typography; -// Module-level rather than nested in Customers: a function's cognitive -// complexity includes everything declared inside it, and this confirm — with a -// branch on `disabling` for each of the title, body, button label and both -// outcome messages — was most of the component's score on its own. +// The two halves of the disable/re-enable confirm, as components rather than +// branches inside the handler: every piece of copy differs between them, so +// one decision up front reads better than the same condition asked five times. +function DisableWarning({ customer }: Readonly<{ customer: CustomerSummary }>) { + return ( + + They will be signed out everywhere immediately and told the account is disabled if they + try to sign in. + {customer.reserved_count > 0 && ( + <> Their {customer.reserved_count} reserved item + {customer.reserved_count === 1 ? '' : 's'} will be released back to the storefront. + )} + {' '}Self-service data export and account deletion stop working too, so any such request + has to be handled by hand. + + ); +} + +function ReEnableWarning() { + return ( + + They will be able to sign in again. Items released when the account was disabled are not + returned — those may already have sold. + + ); +} + +// The customer drawer's body. Extracted because its states — loading, then a +// description list, then either an empty order history or a table — are all +// branches, and every one of them counted toward Customers(). +function CustomerDetailPanel({ + detail, + loading +}: Readonly<{ detail: CustomerDetail | null; loading: boolean }>) { + if (loading || !detail) { + return ; + } + return ( + <> + + {detail.customer.email} + + + {detail.customer.email_verified ? 'Verified' : 'Unverified'} + + + + + {detail.customer.marketing_consent ? 'Subscribed' : 'Not subscribed'} + + {detail.customer.marketing_consent_at && ( +
+ since {new Date(detail.customer.marketing_consent_at).toLocaleDateString()} +
+ )} + + + {new Date(detail.customer.created_at).toLocaleDateString()} + + + + Order History + {detail.orders.length === 0 ? ( + + ) : ( + `$${(v / 100).toFixed(2)}` }, + { + title: 'Processor', + dataIndex: 'processor', + render: (v: string) => {v} + }, + { title: 'Status', dataIndex: 'status' }, + { title: 'Date', dataIndex: 'created_at', render: (v: string) => new Date(v).toLocaleDateString() } + ]} + /> + )} + + ); +} + +// The reserved-items dialog body, extracted for the same reason: three +// mutually exclusive states rendered as three separate conditionals. +function ReservedItemsBody({ + loading, + reserved, + releasing, + onRelease +}: Readonly<{ + loading: boolean; + reserved: ReservedItem[]; + releasing: number | null; + onRelease: (item: ReservedItem) => void; +}>) { + if (loading) { + return ; + } + if (!reserved.length) { + return ; + } + return ( +
`$${(v / 100).toFixed(2)}` + }, + { + title: 'Reservation expires', + dataIndex: 'expires_at', + render: (v: string) => new Date(v).toLocaleString() + }, + { + title: '', + render: (_: unknown, item: ReservedItem) => ( + + ) + } + ]} + /> + ); +} + +// Cell renderers live at module level for the same reason the confirm does: +// every branch inside a column's render callback counted toward Customers(), +// and a table with nine columns is mostly branches. +function NameAndEmail({ email, name }: Readonly<{ email: string; name: string | null }>) { + return ( +
+
{name || No name}
+
{email}
+
+ ); +} + +function BooleanTag({ + value, + onColor, + onLabel, + offLabel +}: Readonly<{ value: boolean; onColor: string; onLabel: string; offLabel: string }>) { + return {value ? onLabel : offLabel}; +} + +function ReservedCell({ + count, + customer, + onOpen +}: Readonly<{ count: number; customer: CustomerSummary; onOpen: (c: CustomerSummary) => void }>) { + if (Number(count) <= 0) { + return 0; + } + return ( + + ); +} + +function ToggleDisabledButton({ + customer, + busy, + onToggle +}: Readonly<{ customer: CustomerSummary; busy: boolean; onToggle: (c: CustomerSummary) => void }>) { + return ( + + ); +} + +// Dates arrive as ISO strings or null; an em dash reads better than "Invalid Date". +function formatDate(value: string | null): string { + return value ? new Date(value).toLocaleDateString() : '—'; +} + +// Module-level rather than nested in Customers, because a function's cognitive +// complexity counts everything declared inside it. function confirmToggleDisabled( customer: CustomerSummary, setTogglingId: (id: number | null) => void, reload: () => void ) { - const disabling = !customer.disabled_at; - - Modal.confirm({ - title: disabling ? `Disable ${customer.email}?` : `Re-enable ${customer.email}?`, - content: disabling ? ( - - They will be signed out everywhere immediately and told the account is disabled if they - try to sign in. - {customer.reserved_count > 0 && ( - <> Their {customer.reserved_count} reserved item - {customer.reserved_count === 1 ? '' : 's'} will be released back to the storefront. - )} - {' '}Self-service data export and account deletion stop working too, so any such request - has to be handled by hand. - - ) : ( - - They will be able to sign in again. Items released when the account was disabled are not - returned — those may already have sold. - - ), - okText: disabling ? 'Disable' : 'Re-enable', - okButtonProps: { danger: disabling }, - onOk: async () => { - setTogglingId(customer.id); - try { - await setCustomerDisabled(customer.id, disabling); - } catch (err) { - message.error(`Couldn't ${disabling ? 'disable' : 're-enable'} — ${(err as Error).message}`); - return; - } finally { - setTogglingId(null); - } - message.success(disabling ? 'Account disabled' : 'Account re-enabled'); - reload(); + const disabling = !customer.disabled_at; + // Chosen once. Asking `disabling` again for each field is what put this + // function over the complexity limit even after it was hoisted out. + const copy = disabling + ? { + title: `Disable ${customer.email}?`, + content: , + okText: 'Disable', + verb: 'disable', + done: 'Account disabled' } - }); - } + : { + title: `Re-enable ${customer.email}?`, + content: , + okText: 'Re-enable', + verb: 're-enable', + done: 'Account re-enabled' + }; + + Modal.confirm({ + title: copy.title, + content: copy.content, + okText: copy.okText, + okButtonProps: { danger: disabling }, + onOk: async () => { + setTogglingId(customer.id); + try { + await setCustomerDisabled(customer.id, disabling); + } catch (err) { + message.error(`Couldn't ${copy.verb} — ${(err as Error).message}`); + return; + } finally { + setTogglingId(null); + } + message.success(copy.done); + reload(); + } + }); +} export default function Customers() { const [customers, setCustomers] = useState([]); @@ -127,53 +334,35 @@ export default function Customers() { title: 'Customer', dataIndex: 'email', sorter: (a, b) => a.email.localeCompare(b.email), - render: (email: string, row: CustomerSummary) => ( -
-
{row.name || No name}
-
{email}
-
- ) + render: (email: string, row: CustomerSummary) => }, { title: 'Verified', dataIndex: 'email_verified', filters: [{ text: 'Verified', value: true }, { text: 'Unverified', value: false }], onFilter: (value, row) => row.email_verified === value, - render: (v: boolean) => {v ? 'Verified' : 'Unverified'} + render: (v: boolean) => }, { title: 'Subscribed', dataIndex: 'marketing_consent', filters: [{ text: 'Subscribed', value: true }, { text: 'Not subscribed', value: false }], onFilter: (value, row) => row.marketing_consent === value, - render: (v: boolean) => {v ? 'Yes' : 'No'} + render: (v: boolean) => }, { title: 'Status', dataIndex: 'disabled_at', - render: (disabledAt: string | null) => - disabledAt - ? DISABLED - : ACTIVE + render: (disabledAt: string | null) => ( + + ) }, { title: 'Reserved', dataIndex: 'reserved_count', - render: (count: number, customer: CustomerSummary) => - Number(count) > 0 ? ( - - ) : ( - 0 - ) + render: (count: number, customer: CustomerSummary) => ( + void openReserved(c)} /> + ) }, { title: 'Orders', @@ -191,28 +380,24 @@ export default function Customers() { title: 'Last Order', dataIndex: 'last_order_at', sorter: (a, b) => new Date(a.last_order_at || 0).getTime() - new Date(b.last_order_at || 0).getTime(), - render: (v: string | null) => (v ? new Date(v).toLocaleDateString() : '—') + render: (v: string | null) => formatDate(v) }, { title: '', key: 'actions', render: (_: unknown, customer: CustomerSummary) => ( - + ) }, { title: 'Joined', dataIndex: 'created_at', sorter: (a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime(), - render: (v: string) => new Date(v).toLocaleDateString() + render: (v: string) => formatDate(v) } ]; @@ -234,56 +419,7 @@ export default function Customers() { onClose={() => { setDrawerOpen(false); setDetail(null); }} width={480} > - {detailLoading || !detail ? ( - - ) : ( - <> - - {detail.customer.email} - - - {detail.customer.email_verified ? 'Verified' : 'Unverified'} - - - - - {detail.customer.marketing_consent ? 'Subscribed' : 'Not subscribed'} - - {detail.customer.marketing_consent_at && ( -
- since {new Date(detail.customer.marketing_consent_at).toLocaleDateString()} -
- )} - - - {new Date(detail.customer.created_at).toLocaleDateString()} - - - - Order History - {detail.orders.length === 0 ? ( - - ) : ( -
`$${(v / 100).toFixed(2)}` }, - { - title: 'Processor', - dataIndex: 'processor', - render: (v: string) => {v} - }, - { title: 'Status', dataIndex: 'status' }, - { title: 'Date', dataIndex: 'created_at', render: (v: string) => new Date(v).toLocaleDateString() } - ]} - /> - )} - - )} + - {reservedLoading ? : null} - {!reservedLoading && !reserved.length ? ( - - ) : null} - {!reservedLoading && reserved.length > 0 && ( -
`$${(v / 100).toFixed(2)}` - }, - { - title: 'Reservation expires', - dataIndex: 'expires_at', - render: (v: string) => new Date(v).toLocaleString() - }, - { - title: '', - render: (_: unknown, item: ReservedItem) => ( - - ) - } - ]} - /> - )} + );