2026 Code Clean Up

Smithy-bot commented

2026-09-01 12:33:00 +01:00

Member

Closes #487

Works through the Code cleanup section of the 2026 clean-up checklist, plus the highest-value items from the Testing gaps section. Not the whole story - see "Left for a follow-up" at the bottom.

Code cleanup (complete)

GuildMemebrUpdate -> GuildMemberUpdate across EventExecutors, client.ts and util.ts. The registrar was also renamed from CoreClient.GuildMemebrUpdate to CoreClient.RegisterGuildMemberUpdateEvent so it matches the other Register*Event methods. Nothing registers a member-update handler today, so there are no callers to update, and GuildMemebrUpdate now appears nowhere in the repo.
Fixed RegisterChannelUpdateEvent, which pushed its handler onto ChannelCreate instead of ChannelUpdate. Not on the checklist, but the same copy-paste bug in the same block.
Awaited async handlers in the dispatchers (Button, ChatInputCommand, StringDropdown) and in Events.onInteractionCreate, so a rejected handler now reaches the surrounding try/catch instead of becoming an unhandled rejection. Errors go through AppLogger.CatchError rather than being cast with e as string. The error reply uses followUp when the interaction has already been replied to or deferred, which is the common case once a handler has called deferReply.
throw new Error(...) instead of bare strings in bot.ts and appLogger.ts.
SeriesHelper pagination uses slice rather than splice. Worth being straight about this one: both helpers already cloneDeep before paging, so splice was only ever mutating a throwaway clone. This is a defensive cleanup, not a live bug fix.
ImageHelper no longer swallows image errors - the empty catch now logs via AppLogger.CatchError. A failing card is still skipped so one bad image cannot fail the whole grid.
AppBaseEntity.FetchOneById defaults relations to [] instead of {}, matching FetchAll and what TypeORM expects.
TimerHelper hardening - strict equality for the timer id lookup, and ticks now run through a shared RunTick that catches synchronous throws and attaches a catch to async ticks, so a failing timer logs instead of producing an unhandled rejection.
Authentication on POST /api/reload-db - see the operator note below.
package.json - jest, ts-jest and jest-mock-extended moved from dependencies to devDependencies, and the malformed bugs.url (https//) fixed.

Operator action needed for the reload webhook

POST /api/reload-db now requires an x-webhook-token header matching WEBHOOK_TOKEN, compared with a length check and then timingSafeEqual. If WEBHOOK_TOKEN is not set the endpoint refuses every request with 503 and logs an error, rather than silently staying open. WEBHOOK_TOKEN needs adding to the deployed .env or the endpoint will stop working after this lands. It is deliberately not in requiredConfigs, so the bot still starts without it. .env.example and docs/webhooks.md cover it.

Tests

26 suites / 159 tests, up from 16 / 90. Ten new suites:

tests/client/interactionCreate/{Button,ChatInputCommand,StringDropdown}.test.ts - dispatch, not-found, and the error path. These are real regression guards, not just coverage: with the await removed from the dispatcher, the rejection escapes and crashes the jest run.
tests/helpers/TimerHelper.test.ts - runOnStart, sync throw and async rejection both logged, start/stop by id.
tests/helpers/ImageHelper.test.ts - first coverage this helper has had. Covers the error being logged rather than swallowed, and the greyscale rules for claimed / unclaimed / no-user-id. That last one closes the gap flagged during UAT on #355, where the greyscale skip could only be verified by reading the code.
tests/helpers/SeriesHelper.test.ts - page boundaries and stable page contents when paging back and forth.
tests/hooks/middleware/RequireWebhookToken.test.ts - unconfigured, missing, wrong, and prefix-of-correct tokens.
tests/hooks/ReloadDB.test.ts, tests/timers/GiveCurrency.test.ts, tests/client/interactionCreate/middleware/NewUserDiscovery.test.ts.

Documentation

New docs/webhooks.md covering the API, the token header and the response codes.

Left for a follow-up

These are written and verified locally but are not in this PR, because pushing them through the Forgejo contents API one file at a time was not viable in a single run:

The repo-wide loose-equality sweep and the eqeqeq ESLint rule. Strict equality is applied in the files this PR already touches; roughly 26 further files still use ==/!=. Pure mechanical churn that would have tripled the diff and buried the behavioural changes above.
The remaining documentation items - the README "Development" section (the three CI checks, the migration workflow, where env vars live, and how to add commands / button events / dropdown events), and cross-links from docs/cards.md, docs/google-drive-sync.md and docs/logger.md to .env.example and the relevant tests.
tests/stringDropdowns/Inventory.test.ts - coverage for the inventory dropdown handler, which this PR does not otherwise change.

Separately, and worth its own issue: the rules block in eslint.config.mjs is scoped to files: ["./src", "./tests"], which matches no files, so camelcase, comma-dangle, prefer-const and friends are not actually enforced today. Fixing the glob surfaces a large number of pre-existing violations, so it did not belong here.

Verification

yarn build, yarn lint and yarn test are all clean on this branch: 26 suites, 159 tests, 8 snapshots.

Closes #487 Works through the **Code cleanup** section of the 2026 clean-up checklist, plus the highest-value items from the **Testing gaps** section. Not the whole story - see "Left for a follow-up" at the bottom. ## Code cleanup (complete) - **`GuildMemebrUpdate` -> `GuildMemberUpdate`** across `EventExecutors`, `client.ts` and `util.ts`. The registrar was also renamed from `CoreClient.GuildMemebrUpdate` to `CoreClient.RegisterGuildMemberUpdateEvent` so it matches the other `Register*Event` methods. Nothing registers a member-update handler today, so there are no callers to update, and `GuildMemebrUpdate` now appears nowhere in the repo. - **Fixed `RegisterChannelUpdateEvent`**, which pushed its handler onto `ChannelCreate` instead of `ChannelUpdate`. Not on the checklist, but the same copy-paste bug in the same block. - **Awaited async handlers in the dispatchers** (`Button`, `ChatInputCommand`, `StringDropdown`) and in `Events.onInteractionCreate`, so a rejected handler now reaches the surrounding `try/catch` instead of becoming an unhandled rejection. Errors go through `AppLogger.CatchError` rather than being cast with `e as string`. The error reply uses `followUp` when the interaction has already been replied to or deferred, which is the common case once a handler has called `deferReply`. - **`throw new Error(...)`** instead of bare strings in `bot.ts` and `appLogger.ts`. - **`SeriesHelper` pagination uses `slice`** rather than `splice`. Worth being straight about this one: both helpers already `cloneDeep` before paging, so `splice` was only ever mutating a throwaway clone. This is a defensive cleanup, not a live bug fix. - **`ImageHelper` no longer swallows image errors** - the empty `catch` now logs via `AppLogger.CatchError`. A failing card is still skipped so one bad image cannot fail the whole grid. - **`AppBaseEntity.FetchOneById`** defaults `relations` to `[]` instead of `{}`, matching `FetchAll` and what TypeORM expects. - **`TimerHelper` hardening** - strict equality for the timer id lookup, and ticks now run through a shared `RunTick` that catches synchronous throws and attaches a `catch` to async ticks, so a failing timer logs instead of producing an unhandled rejection. - **Authentication on `POST /api/reload-db`** - see the operator note below. - **`package.json`** - `jest`, `ts-jest` and `jest-mock-extended` moved from `dependencies` to `devDependencies`, and the malformed `bugs.url` (`https//`) fixed. ### Operator action needed for the reload webhook `POST /api/reload-db` now requires an `x-webhook-token` header matching `WEBHOOK_TOKEN`, compared with a length check and then `timingSafeEqual`. If `WEBHOOK_TOKEN` is not set the endpoint refuses every request with `503` and logs an error, rather than silently staying open. **`WEBHOOK_TOKEN` needs adding to the deployed `.env` or the endpoint will stop working after this lands.** It is deliberately not in `requiredConfigs`, so the bot still starts without it. `.env.example` and `docs/webhooks.md` cover it. ## Tests 26 suites / 159 tests, up from 16 / 90. Ten new suites: - `tests/client/interactionCreate/{Button,ChatInputCommand,StringDropdown}.test.ts` - dispatch, not-found, and the error path. These are real regression guards, not just coverage: with the `await` removed from the dispatcher, the rejection escapes and crashes the jest run. - `tests/helpers/TimerHelper.test.ts` - `runOnStart`, sync throw and async rejection both logged, start/stop by id. - `tests/helpers/ImageHelper.test.ts` - first coverage this helper has had. Covers the error being logged rather than swallowed, and the greyscale rules for claimed / unclaimed / no-user-id. That last one closes the gap flagged during UAT on #355, where the greyscale skip could only be verified by reading the code. - `tests/helpers/SeriesHelper.test.ts` - page boundaries and stable page contents when paging back and forth. - `tests/hooks/middleware/RequireWebhookToken.test.ts` - unconfigured, missing, wrong, and prefix-of-correct tokens. - `tests/hooks/ReloadDB.test.ts`, `tests/timers/GiveCurrency.test.ts`, `tests/client/interactionCreate/middleware/NewUserDiscovery.test.ts`. ## Documentation - New `docs/webhooks.md` covering the API, the token header and the response codes. ## Left for a follow-up These are written and verified locally but are not in this PR, because pushing them through the Forgejo contents API one file at a time was not viable in a single run: 1. **The repo-wide loose-equality sweep and the `eqeqeq` ESLint rule.** Strict equality is applied in the files this PR already touches; roughly 26 further files still use `==`/`!=`. Pure mechanical churn that would have tripled the diff and buried the behavioural changes above. 2. **The remaining documentation items** - the README "Development" section (the three CI checks, the migration workflow, where env vars live, and how to add commands / button events / dropdown events), and cross-links from `docs/cards.md`, `docs/google-drive-sync.md` and `docs/logger.md` to `.env.example` and the relevant tests. 3. **`tests/stringDropdowns/Inventory.test.ts`** - coverage for the inventory dropdown handler, which this PR does not otherwise change. Separately, and worth its own issue: the `rules` block in `eslint.config.mjs` is scoped to `files: ["./src", "./tests"]`, which matches no files, so `camelcase`, `comma-dangle`, `prefer-const` and friends are not actually enforced today. Fixing the glob surfaces a large number of pre-existing violations, so it did not belong here. ## Verification `yarn build`, `yarn lint` and `yarn test` are all clean on this branch: 26 suites, 159 tests, 8 snapshots.

Smithy-bot added 12 commits

2026-09-01 12:33:01 +01:00

Use strict equality in StringTools 8d6e5da98d