refactor(frontend): actually reduce Customers' complexity rather than relocate it (#81)

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 <noreply@anthropic.com>

frontend/src/admin/Customers.tsx

@@ -9,20 +9,11 @@ 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.
-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 ? (
+// 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 (
     <span>
       They will be signed out everywhere immediately and told the account is disabled if they
       try to sign in.
@@ -33,25 +24,241 @@ function confirmToggleDisabled(
       {' '}Self-service data export and account deletion stop working too, so any such request
       has to be handled by hand.
     </span>
-      ) : (
+  );
+}
+
+function ReEnableWarning() {
+  return (
     <span>
       They will be able to sign in again. Items released when the account was disabled are not
       returned — those may already have sold.
     </span>
-      ),
-      okText: disabling ? 'Disable' : 'Re-enable',
+  );
+}
+
+// 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 <Spin />;
+  }
+  return (
+    <>
+      <Descriptions column={1} size="small" bordered>
+        <Descriptions.Item label="Email">{detail.customer.email}</Descriptions.Item>
+        <Descriptions.Item label="Verified">
+          <Tag color={detail.customer.email_verified ? 'green' : 'default'}>
+            {detail.customer.email_verified ? 'Verified' : 'Unverified'}
+          </Tag>
+        </Descriptions.Item>
+        <Descriptions.Item label="Marketing consent">
+          <Tag color={detail.customer.marketing_consent ? 'blue' : 'default'}>
+            {detail.customer.marketing_consent ? 'Subscribed' : 'Not subscribed'}
+          </Tag>
+          {detail.customer.marketing_consent_at && (
+            <div style={{ fontSize: 12, opacity: 0.65, marginTop: 4 }}>
+              since {new Date(detail.customer.marketing_consent_at).toLocaleDateString()}
+            </div>
+          )}
+        </Descriptions.Item>
+        <Descriptions.Item label="Joined">
+          {new Date(detail.customer.created_at).toLocaleDateString()}
+        </Descriptions.Item>
+      </Descriptions>
+
+      <Title level={5} style={{ marginTop: 24 }}>Order History</Title>
+      {detail.orders.length === 0 ? (
+        <Empty description="No orders yet" />
+      ) : (
+        <Table
+          rowKey="id"
+          size="small"
+          dataSource={detail.orders}
+          pagination={false}
+          columns={[
+            { title: 'Item', dataIndex: 'item_name' },
+            { title: 'Amount', dataIndex: 'amount_cents', render: (v: number) => `$${(v / 100).toFixed(2)}` },
+            {
+              title: 'Processor',
+              dataIndex: 'processor',
+              render: (v: string) => <Tag>{v}</Tag>
+            },
+            { 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 <Spin />;
+  }
+  if (!reserved.length) {
+    return <Empty description="This customer isn't holding any items" />;
+  }
+  return (
+    <Table
+      rowKey="item_id"
+      dataSource={reserved}
+      pagination={false}
+      size="small"
+      columns={[
+        { title: 'Item', dataIndex: 'name' },
+        {
+          title: 'Price',
+          dataIndex: 'price_cents',
+          render: (v: number) => `$${(v / 100).toFixed(2)}`
+        },
+        {
+          title: 'Reservation expires',
+          dataIndex: 'expires_at',
+          render: (v: string) => new Date(v).toLocaleString()
+        },
+        {
+          title: '',
+          render: (_: unknown, item: ReservedItem) => (
+            <Button
+              size="small"
+              danger
+              loading={releasing === item.item_id}
+              onClick={() => onRelease(item)}
+            >
+              Release
+            </Button>
+          )
+        }
+      ]}
+    />
+  );
+}
+
+// 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 (
+    <div>
+      <div>{name || <span style={{ opacity: 0.5 }}>No name</span>}</div>
+      <div style={{ fontSize: 12, opacity: 0.65 }}>{email}</div>
+    </div>
+  );
+}
+
+function BooleanTag({
+  value,
+  onColor,
+  onLabel,
+  offLabel
+}: Readonly<{ value: boolean; onColor: string; onLabel: string; offLabel: string }>) {
+  return <Tag color={value ? onColor : 'default'}>{value ? onLabel : offLabel}</Tag>;
+}
+
+function ReservedCell({
+  count,
+  customer,
+  onOpen
+}: Readonly<{ count: number; customer: CustomerSummary; onOpen: (c: CustomerSummary) => void }>) {
+  if (Number(count) <= 0) {
+    return <Text type="secondary">0</Text>;
+  }
+  return (
+    <Button
+      type="link"
+      style={{ padding: 0 }}
+      // The whole row opens the customer drawer, so without this the click
+      // reaches both handlers and the drawer opens behind the reserved-items
+      // dialog.
+      onClick={(event) => { event.stopPropagation(); onOpen(customer); }}
+    >
+      {count} item{Number(count) === 1 ? '' : 's'}
+    </Button>
+  );
+}
+
+function ToggleDisabledButton({
+  customer,
+  busy,
+  onToggle
+}: Readonly<{ customer: CustomerSummary; busy: boolean; onToggle: (c: CustomerSummary) => void }>) {
+  return (
+    <Button
+      size="small"
+      danger={!customer.disabled_at}
+      loading={busy}
+      // The row opens the detail drawer, so this must not bubble.
+      onClick={(event) => { event.stopPropagation(); onToggle(customer); }}
+    >
+      {customer.disabled_at ? 'Re-enable' : 'Disable'}
+    </Button>
+  );
+}
+
+// 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;
+  // 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: <DisableWarning customer={customer} />,
+        okText: 'Disable',
+        verb: 'disable',
+        done: 'Account disabled'
+      }
+    : {
+        title: `Re-enable ${customer.email}?`,
+        content: <ReEnableWarning />,
+        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 ${disabling ? 'disable' : 're-enable'} — ${(err as Error).message}`);
+        message.error(`Couldn't ${copy.verb} — ${(err as Error).message}`);
         return;
       } finally {
         setTogglingId(null);
       }
-        message.success(disabling ? 'Account disabled' : 'Account re-enabled');
+      message.success(copy.done);
       reload();
     }
   });
@@ -127,52 +334,34 @@ export default function Customers() {
       title: 'Customer',
       dataIndex: 'email',
       sorter: (a, b) => a.email.localeCompare(b.email),
-      render: (email: string, row: CustomerSummary) => (
-        <div>
-          <div>{row.name || <span style={{ opacity: 0.5 }}>No name</span>}</div>
-          <div style={{ fontSize: 12, opacity: 0.65 }}>{email}</div>
-        </div>
-      )
+      render: (email: string, row: CustomerSummary) => <NameAndEmail email={email} name={row.name} />
     },
     {
       title: 'Verified',
       dataIndex: 'email_verified',
       filters: [{ text: 'Verified', value: true }, { text: 'Unverified', value: false }],
       onFilter: (value, row) => row.email_verified === value,
-      render: (v: boolean) => <Tag color={v ? 'green' : 'default'}>{v ? 'Verified' : 'Unverified'}</Tag>
+      render: (v: boolean) => <BooleanTag value={v} onColor="green" onLabel="Verified" offLabel="Unverified" />
     },
     {
       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) => <Tag color={v ? 'blue' : 'default'}>{v ? 'Yes' : 'No'}</Tag>
+      render: (v: boolean) => <BooleanTag value={v} onColor="blue" onLabel="Yes" offLabel="No" />
     },
     {
       title: 'Status',
       dataIndex: 'disabled_at',
-      render: (disabledAt: string | null) =>
-        disabledAt
-          ? <Tag color="red">DISABLED</Tag>
-          : <Tag color="green">ACTIVE</Tag>
+      render: (disabledAt: string | null) => (
+        <BooleanTag value={!disabledAt} onColor="green" onLabel="ACTIVE" offLabel="DISABLED" />
+      )
     },
     {
       title: 'Reserved',
       dataIndex: 'reserved_count',
-      render: (count: number, customer: CustomerSummary) =>
-        Number(count) > 0 ? (
-          <Button
-            type="link"
-            style={{ padding: 0 }}
-            // The whole row opens the customer drawer, so without this the
-            // click reaches both handlers and the drawer opens behind the
-            // reserved-items dialog.
-            onClick={(event) => { event.stopPropagation(); void openReserved(customer); }}
-          >
-            {count} item{Number(count) === 1 ? '' : 's'}
-          </Button>
-        ) : (
-          <Text type="secondary">0</Text>
+      render: (count: number, customer: CustomerSummary) => (
+        <ReservedCell count={count} customer={customer} onOpen={(c) => void openReserved(c)} />
       )
     },
     {
@@ -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) => (
-        <Button
-          size="small"
-          danger={!customer.disabled_at}
-          loading={togglingId === customer.id}
-          // The row opens the detail drawer, so this must not bubble.
-          onClick={(event) => { event.stopPropagation(); handleToggleDisabled(customer); }}
-        >
-          {customer.disabled_at ? 'Re-enable' : 'Disable'}
-        </Button>
+        <ToggleDisabledButton
+          customer={customer}
+          busy={togglingId === customer.id}
+          onToggle={handleToggleDisabled}
+        />
       )
     },
     {
       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 ? (
-          <Spin />
-        ) : (
-          <>
-            <Descriptions column={1} size="small" bordered>
-              <Descriptions.Item label="Email">{detail.customer.email}</Descriptions.Item>
-              <Descriptions.Item label="Verified">
-                <Tag color={detail.customer.email_verified ? 'green' : 'default'}>
-                  {detail.customer.email_verified ? 'Verified' : 'Unverified'}
-                </Tag>
-              </Descriptions.Item>
-              <Descriptions.Item label="Marketing consent">
-                <Tag color={detail.customer.marketing_consent ? 'blue' : 'default'}>
-                  {detail.customer.marketing_consent ? 'Subscribed' : 'Not subscribed'}
-                </Tag>
-                {detail.customer.marketing_consent_at && (
-                  <div style={{ fontSize: 12, opacity: 0.65, marginTop: 4 }}>
-                    since {new Date(detail.customer.marketing_consent_at).toLocaleDateString()}
-                  </div>
-                )}
-              </Descriptions.Item>
-              <Descriptions.Item label="Joined">
-                {new Date(detail.customer.created_at).toLocaleDateString()}
-              </Descriptions.Item>
-            </Descriptions>
-
-            <Title level={5} style={{ marginTop: 24 }}>Order History</Title>
-            {detail.orders.length === 0 ? (
-              <Empty description="No orders yet" />
-            ) : (
-              <Table
-                rowKey="id"
-                size="small"
-                dataSource={detail.orders}
-                pagination={false}
-                columns={[
-                  { title: 'Item', dataIndex: 'item_name' },
-                  { title: 'Amount', dataIndex: 'amount_cents', render: (v: number) => `$${(v / 100).toFixed(2)}` },
-                  {
-                    title: 'Processor',
-                    dataIndex: 'processor',
-                    render: (v: string) => <Tag>{v}</Tag>
-                  },
-                  { title: 'Status', dataIndex: 'status' },
-                  { title: 'Date', dataIndex: 'created_at', render: (v: string) => new Date(v).toLocaleDateString() }
-                ]}
-              />
-            )}
-          </>
-        )}
+        <CustomerDetailPanel detail={detail} loading={detailLoading} />
       </Drawer>
 
       <Modal
@@ -294,44 +430,12 @@ export default function Customers() {
         destroyOnHidden
         width={640}
       >
-        {reservedLoading ? <Spin /> : null}
-        {!reservedLoading && !reserved.length ? (
-          <Empty description="This customer isn't holding any items" />
-        ) : null}
-        {!reservedLoading && reserved.length > 0 && (
-          <Table
-            rowKey="item_id"
-            dataSource={reserved}
-            pagination={false}
-            size="small"
-            columns={[
-              { title: 'Item', dataIndex: 'name' },
-              {
-                title: 'Price',
-                dataIndex: 'price_cents',
-                render: (v: number) => `$${(v / 100).toFixed(2)}`
-              },
-              {
-                title: 'Reservation expires',
-                dataIndex: 'expires_at',
-                render: (v: string) => new Date(v).toLocaleString()
-              },
-              {
-                title: '',
-                render: (_: unknown, item: ReservedItem) => (
-                  <Button
-                    size="small"
-                    danger
-                    loading={releasing === item.item_id}
-                    onClick={() => handleRelease(item)}
-                  >
-                    Release
-                  </Button>
-                )
-              }
-            ]}
+        <ReservedItemsBody
+          loading={reservedLoading}
+          reserved={reserved}
+          releasing={releasing}
+          onRelease={handleRelease}
         />
-        )}
       </Modal>
     </div>
   );