bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

Clear the five code smells, which are the 21 minutes of new technical debt #181

New Issue
Closed
opened 2026-08-25 11:34:23 -05:00 by bermudalamb · 1 comment
bermudalamb commented 2026-08-25 11:34:23 -05:00
Owner
Copy Link
Copy Source

SonarQube reports 5 code smells and 21 minutes of new technical debt.

These are the same finding, not two. Technical debt in SonarQube is the estimated remediation effort summed across maintainability issues, so the 21 minutes is the cost of the 5 smells. One issue rather than two, or the same work gets done twice.

They are already visible locally

The repository lints with eslint-plugin-sonarjs, which is the same rule family the server applies. Running both workspaces produces exactly five warnings:

backend/src/middleware/customerAuth.ts
  5:3    ES2015 module syntax is preferred over namespaces   @typescript-eslint/no-namespace

backend/src/routes/shippingAddresses.ts
  119:5  Handle this exception or don't catch it at all      sonarjs/no-ignored-exceptions
  119:12 'err' is defined but never used                     @typescript-eslint/no-unused-vars

frontend/src/filters.ts
  60:35  Provide a compare function that depends on "String.localeCompare"   sonarjs/no-alphabetical-sort
  60:60  Provide a compare function that depends on "String.localeCompare"   sonarjs/no-alphabetical-sort

Five warnings, five smells. The count matching is good evidence these are the same set, though the server's rule configuration is not identical to the plugin's, so it is worth confirming against the dashboard rather than assumed.

Each of them, and whether it is worth acting on

customerAuth.ts:5 — namespace. This is the declare global { namespace Express { interface Request { customerId?: number } } } block that adds req.customerId. That is the documented way to augment Express's types; there is no ES2015-module form of it. This one is a false positive against a framework convention and should be suppressed with a comment saying so, rather than restructured.

shippingAddresses.ts:119 — ignored exception. Two warnings on one line, which is likely one Sonar issue. A swallowed error is exactly the shape #178 and the silent-failure work elsewhere in this repository keep arguing against. Worth reading properly: either the error should be handled, or the catch should say in a comment why discarding it is correct, and drop the unused binding.

filters.ts:60 — .sort() without a comparator. In sameSet, which sorts two string arrays to compare them. Default sort is by UTF-16 code unit, which is not alphabetical for non-ASCII — but this is comparing two lists for equality, not presenting them, so the ordering only has to be consistent, and it is. The rule is right in general and wrong here. Suppress with the reason, or sidestep it by comparing as sets, which says what the function means better than sorting does.

Shape of the fix

Three of the five are rules that are correct in general and inapplicable here. That is worth stating plainly: the goal is a repository where every remaining warning is a decision someone made, not a backlog nobody reads. A suppression carrying its reason is a better outcome than a contortion that satisfies a linter.

shippingAddresses.ts is the one that may be a real defect. It gets read on its merits before anything is suppressed.

Verification

npm run lint clean in both workspaces, and the next analysis reporting zero new debt.

SonarQube reports 5 code smells and 21 minutes of new technical debt. **These are the same finding, not two.** Technical debt in SonarQube is the estimated remediation effort summed across maintainability issues, so the 21 minutes *is* the cost of the 5 smells. One issue rather than two, or the same work gets done twice. ## They are already visible locally The repository lints with `eslint-plugin-sonarjs`, which is the same rule family the server applies. Running both workspaces produces exactly five warnings: ``` backend/src/middleware/customerAuth.ts 5:3 ES2015 module syntax is preferred over namespaces @typescript-eslint/no-namespace backend/src/routes/shippingAddresses.ts 119:5 Handle this exception or don't catch it at all sonarjs/no-ignored-exceptions 119:12 'err' is defined but never used @typescript-eslint/no-unused-vars frontend/src/filters.ts 60:35 Provide a compare function that depends on "String.localeCompare" sonarjs/no-alphabetical-sort 60:60 Provide a compare function that depends on "String.localeCompare" sonarjs/no-alphabetical-sort ``` Five warnings, five smells. The count matching is good evidence these are the same set, though the server's rule configuration is not identical to the plugin's, so it is worth confirming against the dashboard rather than assumed. ## Each of them, and whether it is worth acting on **`customerAuth.ts:5` — namespace.** This is the `declare global { namespace Express { interface Request { customerId?: number } } }` block that adds `req.customerId`. That is the documented way to augment Express's types; there is no ES2015-module form of it. This one is a false positive against a framework convention and should be suppressed with a comment saying so, rather than restructured. **`shippingAddresses.ts:119` — ignored exception.** Two warnings on one line, which is likely one Sonar issue. A swallowed error is exactly the shape #178 and the silent-failure work elsewhere in this repository keep arguing against. Worth reading properly: either the error should be handled, or the catch should say in a comment why discarding it is correct, and drop the unused binding. **`filters.ts:60` — `.sort()` without a comparator.** In `sameSet`, which sorts two string arrays to compare them. Default sort is by UTF-16 code unit, which is *not* alphabetical for non-ASCII — but this is comparing two lists for equality, not presenting them, so the ordering only has to be consistent, and it is. The rule is right in general and wrong here. Suppress with the reason, or sidestep it by comparing as sets, which says what the function means better than sorting does. ## Shape of the fix Three of the five are rules that are correct in general and inapplicable here. That is worth stating plainly: the goal is a repository where every remaining warning is a decision someone made, not a backlog nobody reads. A suppression carrying its reason is a better outcome than a contortion that satisfies a linter. `shippingAddresses.ts` is the one that may be a real defect. It gets read on its merits before anything is suppressed. ## Verification `npm run lint` clean in both workspaces, and the next analysis reporting zero new debt.
bermudalamb added this to the Code Quality and Hardening 2 project 2026-08-25 16:26:36 -05:00
bermudalamb self-assigned this 2026-08-25 16:26:46 -05:00
bermudalamb commented 2026-09-02 11:37:21 -05:00
Author
Owner
Copy Link
Copy Source

The five lint warnings are fixed, but this issue's central assumption turned out to be wrong, so it should not close yet.

The five listed here are gone

All fixed under #261, each the way this issue recommended:

  • customerAuth.ts:5 — suppressed with the reason written beside it. A namespace is the only way to spell an Express type augmentation, since the interface has to merge into one Express declares inside a namespace.
  • shippingAddresses.ts:119 — a real defect, not a suppression. The two catch blocks above it in the same file already logged; this one had simply been missed, so a failed default-address change rolled back and returned 500 leaving nothing behind to say why. It logs now.
  • filters.ts:60 ×2 — localeCompare, with a note that the default sort was a perfectly good total order for the ASCII status values passed here, and that the rule exists because that stops being true the moment a non-ASCII value appears.

Both workspaces lint at zero warnings and have since.

But the smells were not those five

This issue reasoned that the counts matching was good evidence they were the same set, and hedged that the server's rule configuration is not the plugin's and it was worth confirming.

The hedge was right. The analysis of the #261 merge — the commit that cleared all five warnings — still reported 5 code smells and 24 minutes of debt. Different five. eslint-plugin-sonarjs is a subset of the server's rule set, so a clean lint is not a clean analysis, and the matching counts were a coincidence.

What I have done about it

Counting was never going to close this. The CI measures step now also prints, into the job log:

  • the failing quality gate conditions, not just ERROR
  • the open issues with rule, file, line, severity and remediation effort
  • the security hotspots awaiting review

Verified against a stub server rather than assumed, and it still cannot fail the job.

So the next analysis on main will name the five, and this becomes a list to work through rather than a number to stare at. It should also explain the ERROR gate — my guess is new_security_hotspots_reviewed, since bugs and vulnerabilities are both zero and new-code coverage is 80.6% against a typical 80% threshold, but that is a guess and the next run will say.

One thing worth knowing when reading it: sonar.projectVersion has never been bumped, so the PREVIOUS_VERSION new-code period baselines against everything. "On new code" currently means "everything", which is why new_code_smells equals code_smells.

Leaving this open until the list arrives.

The five lint warnings are fixed, but **this issue's central assumption turned out to be wrong**, so it should not close yet. ## The five listed here are gone All fixed under #261, each the way this issue recommended: - `customerAuth.ts:5` — suppressed with the reason written beside it. A namespace is the only way to spell an Express type augmentation, since the interface has to merge into one Express declares inside a namespace. - `shippingAddresses.ts:119` — a real defect, not a suppression. The two catch blocks *above it in the same file* already logged; this one had simply been missed, so a failed default-address change rolled back and returned 500 leaving nothing behind to say why. It logs now. - `filters.ts:60` ×2 — `localeCompare`, with a note that the default sort was a perfectly good total order for the ASCII status values passed here, and that the rule exists because that stops being true the moment a non-ASCII value appears. Both workspaces lint at zero warnings and have since. ## But the smells were not those five This issue reasoned that the counts matching was good evidence they were the same set, and hedged that the server's rule configuration is not the plugin's and it was worth confirming. **The hedge was right.** The analysis of the #261 merge — the commit that cleared all five warnings — still reported **5 code smells and 24 minutes of debt**. Different five. eslint-plugin-sonarjs is a subset of the server's rule set, so a clean lint is not a clean analysis, and the matching counts were a coincidence. ## What I have done about it Counting was never going to close this. The CI measures step now also prints, into the job log: - the **failing quality gate conditions**, not just `ERROR` - the **open issues** with rule, file, line, severity and remediation effort - the **security hotspots** awaiting review Verified against a stub server rather than assumed, and it still cannot fail the job. So the next analysis on `main` will name the five, and this becomes a list to work through rather than a number to stare at. It should also explain the `ERROR` gate — my guess is `new_security_hotspots_reviewed`, since bugs and vulnerabilities are both zero and new-code coverage is 80.6% against a typical 80% threshold, but that is a guess and the next run will say. One thing worth knowing when reading it: `sonar.projectVersion` has never been bumped, so the `PREVIOUS_VERSION` new-code period baselines against everything. "On new code" currently means "everything", which is why `new_code_smells` equals `code_smells`. Leaving this open until the list arrives.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Code Quality and Hardening 2
To Do
Assignees
Clear assignees
calamb (Chelsea Lamb) bermudalamb (Thom Lamb)
No Assignees bermudalamb
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#181