refactor: remove the duplicated blocks SonarQube found (#182) #184

2026-08-25T13:12:20-05:00

bermudalamb commented

2026-08-25 13:12:20 -05:00

Three of the four candidates were real. The fourth was my mistake in the issue.

The category tree adapter, duplicated verbatim between CategoryTreeSelect.tsx and FilterDrawer.tsx. This one was mine: #139 moved the storefront filter to a TreeSelect and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in filters.ts beside buildCategoryTree, which was already shared for exactly the same reason: one meaning, one implementation.

Categories.tsx keeps its own. It builds a different shape for a real antd Tree, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly.

The item_images insert loop, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the /uploads/ prefix is written once — #103 made that the value uploadUrl joins an origin onto, so it is a contract rather than a string.

Extracting it turned up two things the inline versions hid. Create indexed files[i]?.filename ?? '', so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed itemId surfaced that req.params.id is string | undefined under noUncheckedIndexedAccess, which the old inline unknown[] swallowed — now Number(), as the setItemTags call two lines above already did.

The optional-field guards, eight identical lines opening both routes. The distinction worth preserving is that undefined means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once.

TAG_COLORS was not a duplication. The issue listed four files on the strength of a grep that also matched STATUS_TAG_COLORS in Admin.tsx — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in backend/src/utils.ts and one mirror in frontend/src/admin/Tags.tsx, already carrying a comment pointing at the other, which is the same treatment ALLOWED_IMAGE_TYPES gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves.

Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation.

Closes #182

Three of the four candidates were real. The fourth was my mistake in the issue. **The category tree adapter**, duplicated verbatim between `CategoryTreeSelect.tsx` and `FilterDrawer.tsx`. This one was mine: #139 moved the storefront filter to a `TreeSelect` and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in `filters.ts` beside `buildCategoryTree`, which was already shared for exactly the same reason: one meaning, one implementation. `Categories.tsx` keeps its own. It builds a different shape for a real antd `Tree`, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly. **The `item_images` insert loop**, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the `/uploads/` prefix is written once — #103 made that the value `uploadUrl` joins an origin onto, so it is a contract rather than a string. Extracting it turned up two things the inline versions hid. Create indexed `files[i]?.filename ?? ''`, so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed `itemId` surfaced that `req.params.id` is `string | undefined` under `noUncheckedIndexedAccess`, which the old inline `unknown[]` swallowed — now `Number()`, as the `setItemTags` call two lines above already did. **The optional-field guards**, eight identical lines opening both routes. The distinction worth preserving is that `undefined` means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once. **`TAG_COLORS` was not a duplication.** The issue listed four files on the strength of a grep that also matched `STATUS_TAG_COLORS` in `Admin.tsx` — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in `backend/src/utils.ts` and one mirror in `frontend/src/admin/Tags.tsx`, already carrying a comment pointing at the other, which is the same treatment `ALLOWED_IMAGE_TYPES` gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves. Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation. Closes #182

bermudalamb self-assigned this 2026-08-25 13:12:20 -05:00

bermudalamb added this to the Code Quality and Hardening 2 project 2026-08-25 13:12:20 -05:00

bermudalamb added 1 commit 2026-08-25 13:12:21 -05:00

refactor: remove the duplicated blocks SonarQube found (#182 )

SonarQube Analysis / sonarqube (pull_request) Failing after 22m40s

Details

Linting / lint (pull_request) Successful in 1m58s

Details

61c12fd438

Three of the four candidates were real. The fourth was my mistake in the issue.

**The category tree adapter**, duplicated verbatim between `CategoryTreeSelect.tsx` and `FilterDrawer.tsx`. This one was mine: #139 moved the storefront filter to a `TreeSelect` and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in `filters.ts` beside `buildCategoryTree`, which was already shared for exactly the same reason: one meaning, one implementation.

`Categories.tsx` keeps its own. It builds a different shape for a real antd `Tree`, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly.

**The `item_images` insert loop**, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the `/uploads/` prefix is written once — #103 made that the value `uploadUrl` joins an origin onto, so it is a contract rather than a string.

Extracting it turned up two things the inline versions hid. Create indexed `files[i]?.filename ?? ''`, so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed `itemId` surfaced that `req.params.id` is `string | undefined` under `noUncheckedIndexedAccess`, which the old inline `unknown[]` swallowed — now `Number()`, as the `setItemTags` call two lines above already did.

**The optional-field guards**, eight identical lines opening both routes. The distinction worth preserving is that `undefined` means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once.

**`TAG_COLORS` was not a duplication.** The issue listed four files on the strength of a grep that also matched `STATUS_TAG_COLORS` in `Admin.tsx` — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in `backend/src/utils.ts` and one mirror in `frontend/src/admin/Tags.tsx`, already carrying a comment pointing at the other, which is the same treatment `ALLOWED_IMAGE_TYPES` gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves.

Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation.

Closes #182

bermudalamb moved this to Review in Code Quality and Hardening 2 on 2026-08-25 13:13:17 -05:00

bermudalamb merged commit 3cffcf772c into main

2026-08-25 13:13:29 -05:00

bermudalamb deleted branch feature/182-remove-duplication

2026-08-25 13:13:29 -05:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#184