refactor: clear the 85 minutes of technical debt (#81)
Thirteen of SonarQube's fourteen code smells, with the fourteenth argued as a false positive rather than coded around. Four of these were not stylistic. The Remove button in Cart's List.Item actions array carried no key, so React could not match it across renders and rebuilt it on every cart render. The cart, customer-auth and favorites providers each passed a freshly allocated object as their context value, which re-renders every consumer whenever the provider renders, cart or session unchanged — and these three wrap the entire storefront, so the fan-out was the whole tree. Two of them also rebuilt a Set on every render for good measure. All four now memoized on the values they actually derive from. The four cognitive-complexity findings wanted real restructuring rather than suppression. parseItemFilters splits into one helper per field, leaving the function with the order those helpers run in and the single rule that spans two fields; the order is preserved exactly, because a query wrong in two ways reports the first field and rearranging the calls would change which error a caller sees. adminCategories' PUT extracts the parent resolution — the existence check and the cycle check — into a resolver that returns the refusal rather than sending it. App's four-way render chain becomes a Catalogue component, which also removes two of the nested ternaries. Customers' confirm dialog moves to module level, since a function's cognitive complexity counts everything declared inside it and that dialog branches on `disabling` five times. The rest were mechanical: two more nested ternaries — a status-colour lookup and a pluralisation helper — and one type assertion that asserted the type the expression already had. Left alone: S6478 on CategoryTreeSelect's popupRender. That is antd's render prop, called as a function and spliced in, never mounted as a component type, so the destroy-the-subtree failure the rule describes cannot happen. Marked false positive in SonarQube with that reasoning rather than contorting the component around a rule that misread it. Verified rather than assumed. Backend unit 78 pass, integration 134 pass, end-to-end 83 pass, both workspaces build clean. ESLint warnings drop from 35 to 31 with no new file warning — the same React and SonarJS rules #60 turned on are what surfaced this backlog in the first place. Refs #81 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
+90
-32
@@ -26,6 +26,78 @@ const { Title } = Typography;
|
||||
// would become its own request.
|
||||
const FILTER_DEBOUNCE_MS = 250;
|
||||
|
||||
interface CatalogueProps {
|
||||
failed: boolean;
|
||||
loading: boolean;
|
||||
items: Item[];
|
||||
filters: ItemFilters;
|
||||
needsFavoritesAuth: boolean;
|
||||
onRetry: () => void;
|
||||
onSignIn: () => void;
|
||||
onClearFilters: () => void;
|
||||
onChanged: () => void;
|
||||
}
|
||||
|
||||
// The body of the catalogue: an outage, a sign-in prompt, an empty state, or
|
||||
// the grid. Extracted from App so the four cases read as early returns rather
|
||||
// than a ternary chain, and because a function's cognitive complexity counts
|
||||
// everything nested inside it — leaving this inline is what put App over the
|
||||
// limit.
|
||||
function Catalogue({
|
||||
failed,
|
||||
loading,
|
||||
items,
|
||||
filters,
|
||||
needsFavoritesAuth,
|
||||
onRetry,
|
||||
onSignIn,
|
||||
onClearFilters,
|
||||
onChanged
|
||||
}: CatalogueProps) {
|
||||
if (failed) {
|
||||
return (
|
||||
<Alert
|
||||
type="error"
|
||||
showIcon
|
||||
message="Couldn't load items"
|
||||
description="The server didn't return the catalogue. This is usually temporary."
|
||||
action={<Button size="small" onClick={onRetry}>Retry</Button>}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
// Prompted rather than requested: see the effect in App that sets this.
|
||||
if (needsFavoritesAuth) {
|
||||
return (
|
||||
<Empty description="Sign in to see the items you have favorited">
|
||||
<Button type="primary" onClick={onSignIn}>Sign in</Button>
|
||||
<Button style={{ marginInlineStart: 8 }} onClick={onClearFilters}>Browse everything</Button>
|
||||
</Empty>
|
||||
);
|
||||
}
|
||||
|
||||
if (!loading && !items.length) {
|
||||
// Distinguished so "no items match these filters" never reads as an empty
|
||||
// shop, and so the way out is offered only when there is one.
|
||||
const filtered = hasActiveFilters(filters);
|
||||
return (
|
||||
<Empty description={filtered ? 'No items match these filters' : 'No items yet — check back soon'}>
|
||||
{filtered ? <Button onClick={onClearFilters}>Clear filters</Button> : null}
|
||||
</Empty>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<Row gutter={[20, 20]}>
|
||||
{items.map(item => (
|
||||
<Col key={item.id} xs={24} sm={12} md={8} lg={6}>
|
||||
<ItemCard item={item} onChanged={onChanged} />
|
||||
</Col>
|
||||
))}
|
||||
</Row>
|
||||
);
|
||||
}
|
||||
|
||||
export default function App() {
|
||||
const [items, setItems] = useState<Item[]>([]);
|
||||
const [loading, setLoading] = useState(true);
|
||||
@@ -108,6 +180,13 @@ export default function App() {
|
||||
fetchFilterOptions().then(setOptions).catch(() => undefined);
|
||||
}, [load]);
|
||||
|
||||
const handleRetry = useCallback(() => {
|
||||
setLoading(true);
|
||||
void load();
|
||||
}, [load]);
|
||||
|
||||
const openAuthModal = useCallback(() => setAuthModalOpen(true), []);
|
||||
|
||||
const activeCount = activeFilterCount(filters);
|
||||
|
||||
return (
|
||||
@@ -172,38 +251,17 @@ export default function App() {
|
||||
</div>
|
||||
|
||||
{loading && !items.length && !failed ? <Spin /> : null}
|
||||
{failed ? (
|
||||
<Alert
|
||||
type="error"
|
||||
showIcon
|
||||
message="Couldn't load items"
|
||||
description="The server didn't return the catalogue. This is usually temporary."
|
||||
action={<Button size="small" onClick={() => { setLoading(true); void load(); }}>Retry</Button>}
|
||||
/>
|
||||
) : needsFavoritesAuth ? (
|
||||
<Empty description="Sign in to see the items you have favorited">
|
||||
<Button type="primary" onClick={() => setAuthModalOpen(true)}>Sign in</Button>
|
||||
<Button style={{ marginInlineStart: 8 }} onClick={clearFilters}>Browse everything</Button>
|
||||
</Empty>
|
||||
) : !loading && !items.length ? (
|
||||
<Empty
|
||||
description={
|
||||
hasActiveFilters(filters)
|
||||
? 'No items match these filters'
|
||||
: 'No items yet — check back soon'
|
||||
}
|
||||
>
|
||||
{hasActiveFilters(filters) ? <Button onClick={clearFilters}>Clear filters</Button> : null}
|
||||
</Empty>
|
||||
) : (
|
||||
<Row gutter={[20, 20]}>
|
||||
{items.map(item => (
|
||||
<Col key={item.id} xs={24} sm={12} md={8} lg={6}>
|
||||
<ItemCard item={item} onChanged={reload} />
|
||||
</Col>
|
||||
))}
|
||||
</Row>
|
||||
)}
|
||||
<Catalogue
|
||||
failed={failed}
|
||||
loading={loading}
|
||||
items={items}
|
||||
filters={filters}
|
||||
needsFavoritesAuth={needsFavoritesAuth}
|
||||
onRetry={handleRetry}
|
||||
onSignIn={openAuthModal}
|
||||
onClearFilters={clearFilters}
|
||||
onChanged={reload}
|
||||
/>
|
||||
</Content>
|
||||
<Footer style={{ textAlign: 'center', background: token.colorBgContainer }}>
|
||||
<Link to="/privacy">Privacy Policy</Link>
|
||||
|
||||
@@ -26,6 +26,10 @@ import { ItemFilters, EMPTY_FILTERS } from '../filters';
|
||||
const { Header, Content } = Layout;
|
||||
const { Title } = Typography;
|
||||
|
||||
// Anything not sold or reserved is available, so green is the default rather
|
||||
// than a third entry — a new status shows up green instead of crashing.
|
||||
const STATUS_TAG_COLORS: Record<string, string> = { sold: 'red', reserved: 'orange' };
|
||||
|
||||
function Inventory() {
|
||||
const [items, setItems] = useState<Item[]>([]);
|
||||
const [modalOpen, setModalOpen] = useState(false);
|
||||
@@ -196,7 +200,7 @@ function Inventory() {
|
||||
title: 'Status',
|
||||
dataIndex: 'status',
|
||||
render: (status: string) => (
|
||||
<Tag color={status === 'sold' ? 'red' : status === 'reserved' ? 'orange' : 'green'}>{status.toUpperCase()}</Tag>
|
||||
<Tag color={STATUS_TAG_COLORS[status] ?? 'green'}>{status.toUpperCase()}</Tag>
|
||||
)
|
||||
},
|
||||
{
|
||||
|
||||
@@ -117,6 +117,13 @@ export default function Categories() {
|
||||
}
|
||||
}
|
||||
|
||||
// Pluralized in one place because the count drives both whether the clause
|
||||
// appears at all and which suffix it takes.
|
||||
function describeSubcategories(count: number): string {
|
||||
if (count === 0) return 'no subcategories';
|
||||
return `${count} subcategor${count === 1 ? 'y' : 'ies'}`;
|
||||
}
|
||||
|
||||
function handleDelete(category: Category) {
|
||||
const node = findNode(tree, category.id);
|
||||
const totals = node ? branchTotals(node) : { categories: 1, items: category.item_count };
|
||||
@@ -126,7 +133,7 @@ export default function Categories() {
|
||||
title: `Delete "${category.name}"?`,
|
||||
content: (
|
||||
<span>
|
||||
This deletes {subcategories === 0 ? 'no subcategories' : `${subcategories} subcategor${subcategories === 1 ? 'y' : 'ies'}`}
|
||||
This deletes {describeSubcategories(subcategories)}
|
||||
{' '}and uncategorizes {totals.items} item{totals.items === 1 ? '' : 's'}. The items themselves are kept.
|
||||
</span>
|
||||
),
|
||||
|
||||
@@ -9,6 +9,54 @@ 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 ? (
|
||||
<span>
|
||||
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.
|
||||
</span>
|
||||
) : (
|
||||
<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',
|
||||
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();
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
export default function Customers() {
|
||||
const [customers, setCustomers] = useState<CustomerSummary[]>([]);
|
||||
const [loading, setLoading] = useState(true);
|
||||
@@ -53,43 +101,7 @@ export default function Customers() {
|
||||
}
|
||||
|
||||
function handleToggleDisabled(customer: CustomerSummary) {
|
||||
const disabling = !customer.disabled_at;
|
||||
|
||||
Modal.confirm({
|
||||
title: disabling ? `Disable ${customer.email}?` : `Re-enable ${customer.email}?`,
|
||||
content: disabling ? (
|
||||
<span>
|
||||
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.
|
||||
</span>
|
||||
) : (
|
||||
<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',
|
||||
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');
|
||||
void load();
|
||||
}
|
||||
});
|
||||
confirmToggleDisabled(customer, setTogglingId, () => void load());
|
||||
}
|
||||
|
||||
async function handleRelease(item: ReservedItem) {
|
||||
|
||||
@@ -157,7 +157,7 @@ export default function Cart() {
|
||||
<List
|
||||
dataSource={items}
|
||||
renderItem={item => (
|
||||
<List.Item actions={[<Button danger size="small" onClick={() => handleRemove(item.item_id)}>Remove</Button>]}>
|
||||
<List.Item actions={[<Button key="remove" danger size="small" onClick={() => handleRemove(item.item_id)}>Remove</Button>]}>
|
||||
<List.Item.Meta
|
||||
avatar={item.images[0] && <img src={item.images[0].image_path} alt="" style={{ width: 60, height: 60, objectFit: 'cover' }} />}
|
||||
title={item.name}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import React, { createContext, useContext, useEffect, useState, useCallback } from 'react';
|
||||
import React, { createContext, useContext, useEffect, useMemo, useState, useCallback } from 'react';
|
||||
import { CartItem, fetchCart } from './cartApi';
|
||||
import { useCustomerAuth } from '../customer/CustomerAuthContext';
|
||||
|
||||
@@ -32,10 +32,14 @@ export function CartProvider({ children }: { children: React.ReactNode }) {
|
||||
}
|
||||
}, [customer, refresh]);
|
||||
|
||||
const itemIds = new Set(items.map(i => i.item_id));
|
||||
const itemIds = useMemo(() => new Set(items.map(i => i.item_id)), [items]);
|
||||
|
||||
// Memoized because this provider wraps the whole storefront: a new object
|
||||
// here re-renders every consumer on any parent render, cart unchanged.
|
||||
const value = useMemo(() => ({ items, itemIds, refresh }), [items, itemIds, refresh]);
|
||||
|
||||
return (
|
||||
<CartContext.Provider value={{ items, itemIds, refresh }}>
|
||||
<CartContext.Provider value={value}>
|
||||
{children}
|
||||
</CartContext.Provider>
|
||||
);
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import React, { createContext, useContext, useEffect, useState, useCallback } from 'react';
|
||||
import React, { createContext, useContext, useEffect, useMemo, useState, useCallback } from 'react';
|
||||
import { Customer, fetchMe, logoutCustomer } from './customerApi';
|
||||
|
||||
interface CustomerAuthValue {
|
||||
@@ -49,8 +49,15 @@ export function CustomerAuthProvider({ children }: { children: React.ReactNode }
|
||||
setLoading(false);
|
||||
}, []);
|
||||
|
||||
// Memoized because this is the outermost provider: an unmemoized value makes
|
||||
// every signed-in-aware component in the tree re-render on any parent render.
|
||||
const value = useMemo(
|
||||
() => ({ customer, loading, refresh, logout }),
|
||||
[customer, loading, refresh, logout]
|
||||
);
|
||||
|
||||
return (
|
||||
<CustomerAuthContext.Provider value={{ customer, loading, refresh, logout }}>
|
||||
<CustomerAuthContext.Provider value={value}>
|
||||
{children}
|
||||
</CustomerAuthContext.Provider>
|
||||
);
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import React, { createContext, useContext, useEffect, useState, useCallback } from 'react';
|
||||
import React, { createContext, useContext, useEffect, useMemo, useState, useCallback } from 'react';
|
||||
import { Favorite, fetchFavorites } from './favoritesApi';
|
||||
import { useCustomerAuth } from './CustomerAuthContext';
|
||||
|
||||
@@ -39,10 +39,14 @@ export function FavoritesProvider({ children }: { children: React.ReactNode }) {
|
||||
}
|
||||
}, [customer, refresh]);
|
||||
|
||||
const itemIds = new Set(favorites.map(f => f.item_id));
|
||||
const itemIds = useMemo(() => new Set(favorites.map(f => f.item_id)), [favorites]);
|
||||
|
||||
// See CartProvider: an unmemoized value re-renders every ItemCard in the
|
||||
// catalogue whenever anything above this provider renders.
|
||||
const value = useMemo(() => ({ favorites, itemIds, refresh }), [favorites, itemIds, refresh]);
|
||||
|
||||
return (
|
||||
<FavoritesContext.Provider value={{ favorites, itemIds, refresh }}>
|
||||
<FavoritesContext.Provider value={value}>
|
||||
{children}
|
||||
</FavoritesContext.Provider>
|
||||
);
|
||||
|
||||
@@ -88,7 +88,7 @@ function AppRoutes() {
|
||||
|
||||
return (
|
||||
<>
|
||||
<Routes location={backdrop as Location}>
|
||||
<Routes location={backdrop}>
|
||||
<Route path="/" element={<App />} />
|
||||
<Route path="/admin" element={<Admin />} />
|
||||
<Route path="/cart" element={<Cart />} />
|
||||
|
||||
Reference in New Issue
Block a user