Merge pull request 'chore: clear the six code smells SonarQube reported (#181)' (#282) from chore/181-clear-the-six-smells into main

Reviewed-on: #282

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) {
+  for (const group of GROUPS) {
-    const raw = req.body[name];
+    for (const name of group.names) {
-    if (raw === undefined) continue;
+      const reading = group.read(name, req.body[name]);
-    const hours = parseFloat(raw);
+      if ('skip' in reading) continue;
-    if (Number.isNaN(hours) || hours <= 0) {
+      if (!reading.ok) return res.status(400).json({ error: reading.error });
-      return res.status(400).json({ error: `${name} must be a positive number` });
+      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) => (
+          fallback={catalogueErrorFallback}
-            <ErrorFallback
-              error={error}
-              title="The item list didn't load"
-              actions={
-                <Button type="primary" onClick={() => window.location.reload()}>
-                  Reload
-                </Button>
-              }
-            />
-          )}
         >
           {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) => (
+        fallback={modalErrorFallback}
-          <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>
-        )}
       >
         {/* 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) => (
+          fallback={pageErrorFallback}
-            <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>
-              ]}
-            />
-          )}
         >
           <AppRoutes />
         </ErrorBoundary>