chore: clear the six code smells SonarQube reported (#181)

The list finally arrived from the reporting added earlier, and confirmed what #181 could only suspect: these are not the five eslint-plugin-sonarjs warnings that issue lists. Those were fixed under #261 and the count staying at five was a coincidence. It is six now, 25 minutes of debt, and one of them was mine.

admin.ts imported '../utils' twice — I added readId in #207 without noticing the file already imported from there. One import now.

filters.ts had a redundant `as ItemStatus[]`. TypeScript narrows an array through `.every()` with a type predicate from 5.5, and this project is on 5.9, so the assertion stopped telling the compiler anything. Removed, and the build confirms the narrowing holds without it.

adminSettings.ts was the only CRITICAL: cognitive complexity 18 against a limit of 15, almost all of it three near-identical loops differing only in how they validated. Each validation is now a small pure reader returning a refusal rather than sending one, and the handler is one loop over a table. Adding a setting type means adding a row.

That refactor is deliberately behaviour-preserving. Two things were left alone on purpose: the blanket rejection of empty text, which is wrong for the two settings whose documented default is empty and is filed as #280 rather than folded in where it would be invisible; and the absence of the `count` settings, which no caller submits and which the admin screen has no control for. I had started adding count validation and reverted it — widening behaviour under cover of a complexity fix is how a refactor stops being reviewable.

The three S6478s are render props, not components defined during render. ErrorBoundary's `fallback` is typed `(error: Error) => React.ReactNode` and called as `this.props.fallback(...)`, so React only ever sees returned elements and never a new component type — the subtree destruction the rule describes does not happen, and the rule's own message offers `allowAsProps` for this shape, which cannot be set from here. Hoisted rather than suppressed because none of them closes over anything local, so at module level each is one stable function instead of a new closure per render. That is a mild improvement, not a contortion.

Verified: 402 backend unit, 358 backend integration, 30 frontend unit, 157 e2e, both lints clean, both builds clean. The e2e run matters most here — storefront-errors.spec.ts exercises all three hoisted fallbacks, and it was run on its own first.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/routes/admin.ts

@@ -1,12 +1,11 @@
 import { Router, Request, Response } from 'express';
 import { PoolClient } from 'pg';
 import { pool, requireRow } from '../db';
-import { readId } from '../utils';
 import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect';
 import { ItemStatus } from '../types';
 import { asyncRoute } from '../asyncRoute';
 import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters';
-import { tagColorFor } from '../utils';
+import { readId, tagColorFor } from '../utils';
 import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts';
 // The upload pipeline moved to src/imageUpload.ts when #222's public intake
 // endpoint became a second caller. Mounting uploadImages gets the type

backend/src/routes/adminSettings.ts

@@ -17,42 +17,92 @@ router.get('/', asyncRoute(async (_req: Request, res: Response) => {
   res.json(await getSettings());
 }));
 
+/**
+ * One submitted value, checked.
+ *
+ * A refusal is returned rather than sent, so each reader below is a pure
+ * function of its input and the handler keeps sole responsibility for the
+ * response. That is also what lets the handler be one loop instead of four:
+ * the branching lives in these, one or two conditions each, rather than
+ * accumulating in the route.
+ */
+type Reading =
+  | { ok: true; value: number | string }
+  | { ok: false; error: string }
+  | { skip: true };
+
+const SKIP = { skip: true } as const;
+
+function readHours(name: SettingName, raw: unknown): Reading {
+  if (raw === undefined) return SKIP;
+  const hours = parseFloat(String(raw));
+  if (Number.isNaN(hours) || hours <= 0) {
+    return { ok: false, error: `${name} must be a positive number` };
+  }
+  return { ok: true, value: hours };
+}
+
+function readText(name: SettingName, raw: unknown): Reading {
+  if (raw === undefined) return SKIP;
+  if (typeof raw !== 'string' || raw.trim() === '') {
+    // Wrong for the two settings whose documented default is empty —
+    // intakeNotifyEmail and intakeCeilingResetAt cannot currently be cleared.
+    // Left as it was here deliberately: this change is the complexity refactor,
+    // and folding a behaviour fix into it would hide the fix. See #280.
+    return { ok: false, error: `${name} cannot be empty` };
+  }
+  return { ok: true, value: raw };
+}
+
+/**
+ * Membership is checked here rather than left to the dropdown. A value outside
+ * the set would be stored happily and then fail on every submission, surfacing
+ * only as drafts quietly not appearing (#223).
+ */
+function readChoice(name: SettingName, raw: unknown): Reading {
+  if (raw === undefined) return SKIP;
+  if (typeof raw !== 'string' || !isValidChoice(name as never, raw)) {
+    const allowed = CHOICE_OPTIONS[name as never] as readonly string[];
+    return { ok: false, error: `${name} must be one of: ${allowed.join(', ')}` };
+  }
+  return { ok: true, value: raw };
+}
+
+/**
+ * Each group of settings with the reader that validates it.
+ *
+ * A table rather than four copies of the same loop. The loops were identical
+ * apart from their validation, and having four of them was most of this
+ * handler's cognitive complexity — 18 against a limit of 15, which is what
+ * SonarQube flagged as the only CRITICAL smell in the project (#181). Adding a
+ * type now means adding a row.
+ *
+ * The `count` settings from #227 are deliberately absent, exactly as before
+ * this refactor: nothing sends them, the admin screen has no control for them,
+ * and adding validation for a field no caller submits would be widening the
+ * behaviour under cover of a complexity fix.
+ */
+const GROUPS: readonly {
+  names: readonly SettingName[];
+  read: (name: SettingName, raw: unknown) => Reading;
+}[] = [
+  { names: HOURS_SETTINGS, read: readHours },
+  { names: TEXT_SETTINGS, read: readText },
+  { names: CHOICE_SETTINGS, read: readChoice }
+];
+
 router.put('/', asyncRoute(async (req: Request, res: Response) => {
   const values: Partial<Record<SettingName, number | string>> = {};
 
   // Only what was sent is validated and written, so a caller updating one field
   // does not have to echo the others back to avoid clobbering them.
-  for (const name of HOURS_SETTINGS) {
-    const raw = req.body[name];
-    if (raw === undefined) continue;
-    const hours = parseFloat(raw);
-    if (Number.isNaN(hours) || hours <= 0) {
-      return res.status(400).json({ error: `${name} must be a positive number` });
+  for (const group of GROUPS) {
+    for (const name of group.names) {
+      const reading = group.read(name, req.body[name]);
+      if ('skip' in reading) continue;
+      if (!reading.ok) return res.status(400).json({ error: reading.error });
+      values[name] = reading.value;
     }
-    values[name] = hours;
-  }
-
-  for (const name of TEXT_SETTINGS) {
-    const raw = req.body[name];
-    if (raw === undefined) continue;
-    if (typeof raw !== 'string' || raw.trim() === '') {
-      return res.status(400).json({ error: `${name} cannot be empty` });
-    }
-    values[name] = raw;
-  }
-
-  // Membership is checked here rather than left to the dropdown. A value
-  // outside the set would be stored happily and then fail on every submission,
-  // surfacing only as drafts quietly not appearing (#223).
-  for (const name of CHOICE_SETTINGS) {
-    const raw = req.body[name];
-    if (raw === undefined) continue;
-    if (typeof raw !== 'string' || !isValidChoice(name, raw)) {
-      return res
-        .status(400)
-        .json({ error: `${name} must be one of: ${CHOICE_OPTIONS[name].join(', ')}` });
-    }
-    values[name] = raw;
   }
 
   await updateSettings(values);

frontend/src/App.tsx

@@ -119,6 +119,33 @@ const STOREFRONT_DIMENSIONS = [
   priceDimension
 ];
 
+// Hoisted out of the component that used to declare it inline.
+//
+// S6478 flags a function-returning-JSX in a prop as "defining a component
+// during render". Here it is a render prop — ErrorBoundary's `fallback` is
+// typed `(error: Error) => React.ReactNode` and called as
+// `this.props.fallback(...)` — so React only ever sees the returned elements,
+// never a new component type, and the subtree destruction the rule warns about
+// does not happen. The rule's own message offers `allowAsProps` for exactly
+// this shape, which cannot be set from here.
+//
+// Hoisting rather than suppressing because it costs nothing: these close over
+// nothing local, so at module level they are one stable function instead of a
+// new closure per render, which is mildly better and not a contortion. See #181.
+function catalogueErrorFallback(error: Error) {
+  return (
+    <ErrorFallback
+      error={error}
+      title="The item list didn't load"
+      actions={
+        <Button type="primary" onClick={() => window.location.reload()}>
+          Reload
+        </Button>
+      }
+    />
+  );
+}
+
 export default function App() {
   const [authModalOpen, setAuthModalOpen] = useState(false);
   const [searchParams, setSearchParams] = useSearchParams();
@@ -231,17 +258,7 @@ export default function App() {
           // the existing end-to-end assertion that the fallback renders still
           // holds.
           resetKey={filterKey}
-          fallback={(error) => (
-            <ErrorFallback
-              error={error}
-              title="The item list didn't load"
-              actions={
-                <Button type="primary" onClick={() => window.location.reload()}>
-                  Reload
-                </Button>
-              }
-            />
-          )}
+          fallback={catalogueErrorFallback}
         >
           {import.meta.env.DEV && <DevThrow scope="catalogue" />}
           <Catalogue

frontend/src/filters.ts

@@ -142,7 +142,7 @@ export function filtersFromSearchParams(params: URLSearchParams): ItemFilters {
     .filter((part) => part !== '');
   const status =
     parsedStatus.length > 0 && parsedStatus.every(isPublicStatus)
-      ? (parsedStatus as ItemStatus[])
+      ? parsedStatus
       : null;
 
   const favorites = params.get('favorites');

frontend/src/main.tsx

@@ -69,6 +69,72 @@ function usePrefersReducedMotion(): boolean {
 // looking at, rather than a page of its own. It stays a real, linkable URL —
 // bookmarkable, refreshable, and closed by the browser's Back button — while
 // never being a place with no way out of it.
+// Both hoisted out of the components that declared them inline.
+//
+// S6478 flags a function-returning-JSX in a prop as "defining a component
+// during render". These are render props — ErrorBoundary's `fallback` is typed
+// `(error: Error) => React.ReactNode` and called as `this.props.fallback(...)`
+// — so React only ever sees the returned elements, never a new component type,
+// and the subtree destruction the rule warns about does not happen. The rule's
+// own message offers `allowAsProps` for exactly this shape, which cannot be set
+// from here.
+//
+// Hoisting rather than suppressing because it costs nothing: neither closes
+// over anything local, so at module level each is one stable function instead
+// of a new closure per render. See #181.
+function modalErrorFallback(error: Error) {
+  return (
+    <ModalDialog
+      open
+      // No `title` here: ErrorFallback renders the same string as an <h3>, and
+      // antd would otherwise announce the dialog's accessible name and then the
+      // identical heading right after it.
+      footer={null}
+      onCancel={() => {
+        window.location.href = '/';
+      }}
+    >
+      <ErrorFallback
+        error={error}
+        title="Couldn't open that"
+        actions={
+          <Button
+            type="primary"
+            onClick={() => {
+              window.location.href = '/';
+            }}
+          >
+            Close
+          </Button>
+        }
+      />
+    </ModalDialog>
+  );
+}
+
+function pageErrorFallback(error: Error) {
+  return (
+    <ErrorFallback
+      error={error}
+      title="Something went wrong"
+      fullPage
+      actions={[
+        <Button key="reload" type="primary" onClick={() => window.location.reload()}>
+          Reload
+        </Button>,
+        <Button
+          key="home"
+          onClick={() => {
+            window.location.href = '/';
+          }}
+        >
+          Back to the shop
+        </Button>
+      ]}
+    />
+  );
+}
+
 function AppRoutes() {
   const location = useLocation();
   const navigate = useNavigate();
@@ -114,33 +180,7 @@ function AppRoutes() {
       {/* Rendered outside the Routes above, which are showing the backdrop. */}
       <ErrorBoundary
         context="modal"
-        fallback={(error) => (
-          <ModalDialog
-            open
-            // No `title` here: ErrorFallback renders the same string as an
-            // <h3>, and antd would otherwise announce the dialog's accessible
-            // name and then the identical heading right after it.
-            footer={null}
-            onCancel={() => {
-              window.location.href = '/';
-            }}
-          >
-            <ErrorFallback
-              error={error}
-              title="Couldn't open that"
-              actions={
-                <Button
-                  type="primary"
-                  onClick={() => {
-                    window.location.href = '/';
-                  }}
-                >
-                  Close
-                </Button>
-              }
-            />
-          </ModalDialog>
-        )}
+        fallback={modalErrorFallback}
       >
         {/* Unconditional, so /?boom=modal fires this boundary with the
             storefront rendered behind it — no session needed. */}
@@ -190,26 +230,7 @@ function Root() {
       <BrowserRouter>
         <ErrorBoundary
           context="page"
-          fallback={(error) => (
-            <ErrorFallback
-              error={error}
-              title="Something went wrong"
-              fullPage
-              actions={[
-                <Button key="reload" type="primary" onClick={() => window.location.reload()}>
-                  Reload
-                </Button>,
-                <Button
-                  key="home"
-                  onClick={() => {
-                    window.location.href = '/';
-                  }}
-                >
-                  Back to the shop
-                </Button>
-              ]}
-            />
-          )}
+          fallback={pageErrorFallback}
         >
           <AppRoutes />
         </ErrorBoundary>