External/card-drop
Watch
5
0
Fork
You've already forked card-drop
 0
Code Issues 39 Pull requests Projects 1 Releases 36 Packages Wiki Activity Actions

2026 Code Clean Up #487

New issue
Open
opened 2026-01-22 17:31:07 +00:00 by Vylpes · 10 comments
Vylpes commented 2026-01-22 17:31:07 +00:00
Owner
Copy link

Story Points: 13

Scan the repository for areas of code which can be cleaned up and perhaps written a bit cleaner. Identify any major areas not covered by testing or documentation which should be.

Card Drop cleanup + coverage checklist

Actionable items from a 2026 repository scan. Check items off as they are completed.

Code cleanup

  • Rename misspelled GuildMemebrUpdate to GuildMemberUpdate

    • Files: src/contracts/EventExecutors.ts, src/client/client.ts, src/client/util.ts
    • Update registration and event-load wiring consistently so member-update handlers bind correctly.

  • Await async command / button / dropdown handlers in dispatchers

    • Files: src/client/interactionCreate/Button.ts, src/client/interactionCreate/ChatInputCommand.ts, src/client/interactionCreate/StringDropdown.ts
    • Today handlers are fired without await, so rejected promises bypass the surrounding try/catch.
    • Change to await item.Event.execute(...) / await itemToUse.Command.execute(...).
    • Improve error logging (avoid casting errors with e as string; use AppLogger.CatchError or similar).

  • Throw Error objects instead of bare strings

    • Files: src/bot.ts, src/client/appLogger.ts
    • Convert throw "..." to throw new Error("...") and keep logging consistent.

  • Replace loose equality (== / !=) with strict equality (=== / !==)

    • Notable files: src/helpers/DropHelpers/GetUnclaimedCardsHelper.ts, src/helpers/InventoryHelper.ts, src/commands/sacrifice.ts, src/helpers/SeriesHelper.ts, src/helpers/TimerHelper.ts, and related helpers/commands that compare quantities/IDs.
    • Optionally enable/confirm eqeqeq in ESLint, then fix remaining violations.

  • Stop mutating source arrays during pagination

    • File: src/helpers/SeriesHelper.ts
    • Replace splice page selection with slice so cloned card lists are not mutated as a side effect.

  • Stop swallowing image-generation errors

    • File: src/helpers/ImageHelper.ts
    • The empty catch with a TODO currently drops failures. Capture the error, log it (or rethrow), and cover failure modes in tests.

  • Fix AppBaseEntity.FetchOneById relations default

    • File: src/contracts/AppBaseEntity.ts
    • Uses relations || {}; TypeORM expects an array. Default to relations || [] (same pattern as FetchAll).

  • Harden timer tick execution

    • File: src/helpers/TimerHelper.ts
    • Use strict equality for timer ID lookup.
    • Await / safely handle async onTick callbacks and log tick failures.

  • Add authentication to the reload webhook

    • File: src/webhooks.ts (POST /api/reload-db)
    • Require a shared secret/token header (or similar), document the env var, and add a lightweight test.

  • Project metadata / dependency cleanup

    • File: package.json
    • Move jest (and related test packages if appropriate) from dependencies to devDependencies.
    • Fix malformed bugs.url (https//... → https://...).

Testing gaps

Current Jest coverage is concentrated on drop/multidrop/effects commands, a subset of button events, PurgeClaims, and a few helpers. Priority gaps:

  • Add helper tests for uncovered helpers

    • src/helpers/InventoryHelper.ts
    • src/helpers/CardSearchHelper.ts
    • src/helpers/SeriesHelper.ts
    • src/helpers/DropHelpers/GetUnclaimedCardsHelper.ts
    • src/helpers/ImageHelper.ts

  • Add command tests for untested slash commands

    • Missing or lightly covered: daily, gdrivesync, give, inventory, resync, sacrifice, series, stats, trade, view, about, id, and stage/*.

  • Add button-event tests for untested handlers

    • Missing: inventory, sacrifice, series, trade, view.

  • Add string-dropdown coverage

    • src/stringDropdowns/Inventory.ts and dispatch path in src/client/interactionCreate/StringDropdown.ts.

  • Add webhook + middleware tests

    • src/webhooks.ts
    • src/client/interactionCreate/middleware/NewUserDiscovery.ts

  • Add timer coverage for GiveCurrency

    • src/timers/GiveCurrency.ts (only PurgeClaims is tested today).

  • Assert dispatcher error-handling after awaiting handlers

    • Ensure rejected promises are caught and users receive a consistent error reply.

Documentation gaps

  • Expand README with developer ops

    • Document yarn test / yarn lint.
    • Clarify DB migration workflow beyond a brief db:up mention.
    • Document POST /api/reload-db (and auth once added).

  • Document how to add commands / button events / dropdown events

    • Point at src/registry.ts and the interaction-create dispatchers so contributors know where to register new handlers.

  • Cross-link existing docs to testing / env expectations

    • docs/cards.md, docs/google-drive-sync.md, docs/logger.md should reference where env vars live and what tests are expected when changing those areas.

Suggested order of attack

  1. Fix dispatcher await + error logging (correctness).
  2. Fix GuildMemebrUpdate typo and AppBaseEntity relations default.
  3. Replace splice pagination and empty catch in ImageHelper.
  4. Secure /api/reload-db and document it.
  5. Expand tests for helpers/dispatchers first, then high-traffic commands (inventory, sacrifice, trade, series, view).
  6. Fill README / contributor docs last once behaviour is stabilized.
Story Points: 13 --- Scan the repository for areas of code which can be cleaned up and perhaps written a bit cleaner. Identify any major areas not covered by testing or documentation which should be. --- # Card Drop cleanup + coverage checklist Actionable items from a 2026 repository scan. Check items off as they are completed. ## Code cleanup - [ ] **Rename misspelled `GuildMemebrUpdate` to `GuildMemberUpdate`** - Files: `src/contracts/EventExecutors.ts`, `src/client/client.ts`, `src/client/util.ts` - Update registration and event-load wiring consistently so member-update handlers bind correctly. - [ ] **Await async command / button / dropdown handlers in dispatchers** - Files: `src/client/interactionCreate/Button.ts`, `src/client/interactionCreate/ChatInputCommand.ts`, `src/client/interactionCreate/StringDropdown.ts` - Today handlers are fired without `await`, so rejected promises bypass the surrounding `try/catch`. - Change to `await item.Event.execute(...)` / `await itemToUse.Command.execute(...)`. - Improve error logging (avoid casting errors with `e as string`; use `AppLogger.CatchError` or similar). - [ ] **Throw `Error` objects instead of bare strings** - Files: `src/bot.ts`, `src/client/appLogger.ts` - Convert `throw "..."` to `throw new Error("...")` and keep logging consistent. - [ ] **Replace loose equality (`==` / `!=`) with strict equality (`===` / `!==`)** - Notable files: `src/helpers/DropHelpers/GetUnclaimedCardsHelper.ts`, `src/helpers/InventoryHelper.ts`, `src/commands/sacrifice.ts`, `src/helpers/SeriesHelper.ts`, `src/helpers/TimerHelper.ts`, and related helpers/commands that compare quantities/IDs. - Optionally enable/confirm `eqeqeq` in ESLint, then fix remaining violations. - [ ] **Stop mutating source arrays during pagination** - File: `src/helpers/SeriesHelper.ts` - Replace `splice` page selection with `slice` so cloned card lists are not mutated as a side effect. - [ ] **Stop swallowing image-generation errors** - File: `src/helpers/ImageHelper.ts` - The empty `catch` with a TODO currently drops failures. Capture the error, log it (or rethrow), and cover failure modes in tests. - [ ] **Fix `AppBaseEntity.FetchOneById` relations default** - File: `src/contracts/AppBaseEntity.ts` - Uses `relations || {}`; TypeORM expects an array. Default to `relations || []` (same pattern as `FetchAll`). - [ ] **Harden timer tick execution** - File: `src/helpers/TimerHelper.ts` - Use strict equality for timer ID lookup. - Await / safely handle async `onTick` callbacks and log tick failures. - [ ] **Add authentication to the reload webhook** - File: `src/webhooks.ts` (`POST /api/reload-db`) - Require a shared secret/token header (or similar), document the env var, and add a lightweight test. - [ ] **Project metadata / dependency cleanup** - File: `package.json` - Move `jest` (and related test packages if appropriate) from `dependencies` to `devDependencies`. - Fix malformed `bugs.url` (`https//...` → `https://...`). ## Testing gaps Current Jest coverage is concentrated on drop/multidrop/effects commands, a subset of button events, `PurgeClaims`, and a few helpers. Priority gaps: - [ ] **Add helper tests for uncovered helpers** - `src/helpers/InventoryHelper.ts` - `src/helpers/CardSearchHelper.ts` - `src/helpers/SeriesHelper.ts` - `src/helpers/DropHelpers/GetUnclaimedCardsHelper.ts` - `src/helpers/ImageHelper.ts` - [ ] **Add command tests for untested slash commands** - Missing or lightly covered: `daily`, `gdrivesync`, `give`, `inventory`, `resync`, `sacrifice`, `series`, `stats`, `trade`, `view`, `about`, `id`, and `stage/*`. - [ ] **Add button-event tests for untested handlers** - Missing: `inventory`, `sacrifice`, `series`, `trade`, `view`. - [ ] **Add string-dropdown coverage** - `src/stringDropdowns/Inventory.ts` and dispatch path in `src/client/interactionCreate/StringDropdown.ts`. - [ ] **Add webhook + middleware tests** - `src/webhooks.ts` - `src/client/interactionCreate/middleware/NewUserDiscovery.ts` - [ ] **Add timer coverage for `GiveCurrency`** - `src/timers/GiveCurrency.ts` (only `PurgeClaims` is tested today). - [ ] **Assert dispatcher error-handling after awaiting handlers** - Ensure rejected promises are caught and users receive a consistent error reply. ## Documentation gaps - [ ] **Expand README with developer ops** - Document `yarn test` / `yarn lint`. - Clarify DB migration workflow beyond a brief `db:up` mention. - Document `POST /api/reload-db` (and auth once added). - [ ] **Document how to add commands / button events / dropdown events** - Point at `src/registry.ts` and the interaction-create dispatchers so contributors know where to register new handlers. - [ ] **Cross-link existing docs to testing / env expectations** - `docs/cards.md`, `docs/google-drive-sync.md`, `docs/logger.md` should reference where env vars live and what tests are expected when changing those areas. ## Suggested order of attack 1. Fix dispatcher `await` + error logging (correctness). 2. Fix `GuildMemebrUpdate` typo and `AppBaseEntity` relations default. 3. Replace `splice` pagination and empty catch in `ImageHelper`. 4. Secure `/api/reload-db` and document it. 5. Expand tests for helpers/dispatchers first, then high-traffic commands (`inventory`, `sacrifice`, `trade`, `series`, `view`). 6. Fill README / contributor docs last once behaviour is stabilized.
Vylpes added this to the 0.12.0 milestone 2026-01-22 17:31:57 +00:00
Vylpes added this to the 0.10 Sprint 4 project 2026-06-17 18:01:52 +01:00
Vylpes self-assigned this 2026-07-02 10:53:30 +01:00
Vylpes stopped working 2026-07-16 21:00:30 +01:00
12 minutes 54 seconds
Smithy-bot commented 2026-08-30 12:07:52 +01:00
Member
Copy link

Estimate: 13 story points

The cleanup checklist is specific (files, behaviours, and order of attack), so this can be sized without going back to criteria.

Breakdown:

  • Code cleanup (10 items) — 3 points. Mostly mechanical correctness: typo rename, await in dispatchers, Error throws, strict equality, slice pagination, ImageHelper catch, FetchOneById relations default, timer ticks, package.json. The heavier piece is authenticating POST /api/reload-db.
  • Testing gaps — 8 points. Helpers and dispatcher error-handling first, then high-traffic commands (inventory, sacrifice, trade, series, view) plus buttons/dropdowns/webhooks/GiveCurrency. Remaining command tests are patterned Jest work, not new product behaviour.
  • Documentation — 2 points. README ops (yarn test / yarn lint, migrations, reload webhook), how to register commands/events, and cross-links in existing docs.

13 fits better than 8 (tests alone exceed that) or 21 (the work is repetitive, not exploratory).

Moving to needs/tests.

**Estimate: 13 story points** The cleanup checklist is specific (files, behaviours, and order of attack), so this can be sized without going back to criteria. Breakdown: - **Code cleanup (10 items) — 3 points.** Mostly mechanical correctness: typo rename, `await` in dispatchers, `Error` throws, strict equality, `slice` pagination, `ImageHelper` catch, `FetchOneById` relations default, timer ticks, `package.json`. The heavier piece is authenticating `POST /api/reload-db`. - **Testing gaps — 8 points.** Helpers and dispatcher error-handling first, then high-traffic commands (`inventory`, `sacrifice`, `trade`, `series`, `view`) plus buttons/dropdowns/webhooks/`GiveCurrency`. Remaining command tests are patterned Jest work, not new product behaviour. - **Documentation — 2 points.** README ops (`yarn test` / `yarn lint`, migrations, reload webhook), how to register commands/events, and cross-links in existing docs. 13 fits better than 8 (tests alone exceed that) or 21 (the work is repetitive, not exploratory). Moving to `needs/tests`.
Smithy-bot commented 2026-08-31 12:04:49 +01:00
Member
Copy link

QA test scripts (against the cleanup + coverage checklist)

Preconditions for all scripts unless noted:

  • Bot token and DB are a local/dev instance (not production).
  • yarn test and yarn lint run cleanly on develop before the change (baseline).
  • A test guild with at least two users, inventory cards, series data, and an empty-slot image is available.

TC-01 — GuildMemberUpdate typo rename

Covers: Rename GuildMemebrUpdate → GuildMemberUpdate

  1. Search the repo for GuildMemebrUpdate — expect: zero matches.
  2. Confirm GuildMemberUpdate is registered in EventExecutors, client.ts, and util.ts.
  3. In Discord, change a member nickname/role in a guild the bot is in.
    Expect: the member-update handler still runs (no crash, no “unknown event” log). Existing member-update behaviour is unchanged.

TC-02 — Dispatcher await + error handling

Covers: Await command / button / dropdown handlers; rejected promises are caught

  1. Trigger a normal slash command, button, and string dropdown that succeed.
    Expect: each interaction replies as today; no unhandled-rejection logs.
  2. Temporarily force a handler to throw new Error("qa-dispatch") (or use a test double) for command, button, and dropdown paths.
    Expect: the dispatcher try/catch logs via AppLogger.CatchError (or equivalent), the user gets the existing error reply, and the process does not crash.
  3. Confirm errors are not logged as e as string.

TC-03 — Throw Error objects

Covers: src/bot.ts, src/client/appLogger.ts

  1. Search for throw " / throw ' in those files — expect: none.
  2. Force the previous throw sites (invalid config / logger failure).
    Expect: stack traces include a real Error; logging still records the message.

TC-04 — Strict equality

Covers: quantity/ID comparisons (GetUnclaimedCardsHelper, InventoryHelper, sacrifice, SeriesHelper, TimerHelper, related files)

  1. Confirm eqeqeq is enabled (or no remaining == / != in the listed files).
  2. /inventory with 0, 1, and many cards; /sacrifice with valid and invalid quantities; series pagination; timer ticks.
    Expect: same user-visible results as before; no false matches from type-coercion (e.g. "1" vs 1).

TC-05 — Series pagination does not mutate source

Covers: SeriesHelper splice → slice

  1. Open /series page 1, then page 2, then back to page 1 (repeat 3+ times).
    Expect: page 1 contents stay stable; card counts do not shrink.
  2. If a unit test clones a list and paginates twice, the original array length/order is unchanged.

TC-06 — ImageHelper errors are not swallowed

Covers: empty catch in ImageHelper

  1. Force image generation to fail (invalid input / mocked renderer throw).
    Expect: the error is logged (or rethrown per implementation); it is not silently dropped.
  2. Successful image generation still produces the same card image as today.

TC-07 — FetchOneById relations default

Covers: AppBaseEntity.FetchOneById default [] not {}

  1. Fetch an entity with no relations argument and with an explicit relations array.
    Expect: no TypeORM relations-type error; related rows load only when requested.

TC-08 — Timer tick hardening

Covers: TimerHelper ID lookup + async onTick

  1. Let GiveCurrency and PurgeClaims tick on schedule.
    Expect: ticks still fire; wrong IDs are not matched.
  2. Force an onTick rejection.
    Expect: the failure is logged; later ticks still run.

TC-09 — Reload webhook authentication

Covers: POST /api/reload-db auth

  1. POST /api/reload-db with no auth header.
    Expect: 401/403; DB is not reloaded.
  2. Repeat with an invalid token.
    Expect: 401/403; no reload.
  3. Repeat with the documented secret/token header.
    Expect: 2xx; DB reload occurs as today.
  4. Confirm the env var name is documented (README / env example). Do not commit real secrets.

TC-10 — package.json cleanup

Covers: jest in devDependencies; bugs.url scheme

  1. Open package.json.
    Expect: jest (and related test packages if moved) are under devDependencies; bugs.url starts with https://.
  2. yarn test still runs from a fresh install of production+dev deps.

TC-11 — Helper unit tests exist and pass

Covers: InventoryHelper, CardSearchHelper, SeriesHelper, GetUnclaimedCardsHelper, ImageHelper

  1. yarn test includes new/expanded suites for each helper.
    Expect: success, empty, and error/edge cases; ImageHelper includes a failure-mode test.

TC-12 — Command unit tests exist and pass

Covers: daily, gdrivesync, give, inventory, resync, sacrifice, series, stats, trade, view, about, id, stage/*

  1. Each listed command has a Jest suite covering happy path + at least one invalid-input / permission / empty-state case where applicable.
  2. yarn test is green.

TC-13 — Button, dropdown, webhook, middleware, timer tests

Covers: buttons inventory / sacrifice / series / trade / view; stringDropdowns/Inventory; webhooks.ts; NewUserDiscovery; GiveCurrency

  1. Each has tests for success and a failure/guard path.
  2. Dispatcher tests assert rejected handlers are caught (see TC-02).
  3. yarn test is green.

TC-14 — Lint and full test suite

  1. yarn lint — expect: 0 errors.
  2. yarn test — expect: 0 failures; coverage includes the new files (no requirement to hit a specific %).

TC-15 — README developer ops

Covers: yarn test / yarn lint, DB migrations beyond db:up, reload webhook + auth

  1. A new contributor following README only can run lint, tests, and migrations.
  2. Reload webhook method, path, and auth header/env var are documented.

TC-16 — Contributor registration docs

Covers: how to add commands / button events / dropdown events

  1. Docs point at src/registry.ts and the interaction-create dispatchers.
  2. The steps are enough to add a stub command/button/dropdown without reading the whole codebase.

TC-17 — Existing docs cross-links

Covers: docs/cards.md, docs/google-drive-sync.md, docs/logger.md

  1. Each references relevant env vars and which tests to run when changing that area.

TC-18 — Smoke regression (manual)

After the above, in a test guild:

  1. /drop or /multidrop → claim a card.
  2. /inventory paginate; /view a card; /series paginate; /sacrifice; /trade (cancel before complete); /stats; /daily.
  3. Inventory string dropdown (if shown) still updates the view.
    Expect: no crashes, no missing replies, images still generate on success.

Criteria were specific enough to script without going back to needs/criteria. Moving to needs/approval.

**QA test scripts** (against the cleanup + coverage checklist) Preconditions for all scripts unless noted: - Bot token and DB are a local/dev instance (not production). - `yarn test` and `yarn lint` run cleanly on `develop` before the change (baseline). - A test guild with at least two users, inventory cards, series data, and an empty-slot image is available. --- ### TC-01 — GuildMemberUpdate typo rename **Covers:** Rename `GuildMemebrUpdate` → `GuildMemberUpdate` 1. Search the repo for `GuildMemebrUpdate` — **expect:** zero matches. 2. Confirm `GuildMemberUpdate` is registered in `EventExecutors`, `client.ts`, and `util.ts`. 3. In Discord, change a member nickname/role in a guild the bot is in. **Expect:** the member-update handler still runs (no crash, no “unknown event” log). Existing member-update behaviour is unchanged. ### TC-02 — Dispatcher await + error handling **Covers:** Await command / button / dropdown handlers; rejected promises are caught 1. Trigger a normal slash command, button, and string dropdown that succeed. **Expect:** each interaction replies as today; no unhandled-rejection logs. 2. Temporarily force a handler to `throw new Error("qa-dispatch")` (or use a test double) for command, button, and dropdown paths. **Expect:** the dispatcher `try/catch` logs via `AppLogger.CatchError` (or equivalent), the user gets the existing error reply, and the process does not crash. 3. Confirm errors are not logged as `e as string`. ### TC-03 — Throw Error objects **Covers:** `src/bot.ts`, `src/client/appLogger.ts` 1. Search for `throw "` / `throw '` in those files — **expect:** none. 2. Force the previous throw sites (invalid config / logger failure). **Expect:** stack traces include a real `Error`; logging still records the message. ### TC-04 — Strict equality **Covers:** quantity/ID comparisons (`GetUnclaimedCardsHelper`, `InventoryHelper`, `sacrifice`, `SeriesHelper`, `TimerHelper`, related files) 1. Confirm `eqeqeq` is enabled (or no remaining `==` / `!=` in the listed files). 2. `/inventory` with 0, 1, and many cards; `/sacrifice` with valid and invalid quantities; series pagination; timer ticks. **Expect:** same user-visible results as before; no false matches from type-coercion (e.g. `"1"` vs `1`). ### TC-05 — Series pagination does not mutate source **Covers:** `SeriesHelper` `splice` → `slice` 1. Open `/series` page 1, then page 2, then back to page 1 (repeat 3+ times). **Expect:** page 1 contents stay stable; card counts do not shrink. 2. If a unit test clones a list and paginates twice, the original array length/order is unchanged. ### TC-06 — ImageHelper errors are not swallowed **Covers:** empty `catch` in `ImageHelper` 1. Force image generation to fail (invalid input / mocked renderer throw). **Expect:** the error is logged (or rethrown per implementation); it is not silently dropped. 2. Successful image generation still produces the same card image as today. ### TC-07 — FetchOneById relations default **Covers:** `AppBaseEntity.FetchOneById` default `[]` not `{}` 1. Fetch an entity with no relations argument and with an explicit relations array. **Expect:** no TypeORM relations-type error; related rows load only when requested. ### TC-08 — Timer tick hardening **Covers:** `TimerHelper` ID lookup + async `onTick` 1. Let `GiveCurrency` and `PurgeClaims` tick on schedule. **Expect:** ticks still fire; wrong IDs are not matched. 2. Force an `onTick` rejection. **Expect:** the failure is logged; later ticks still run. ### TC-09 — Reload webhook authentication **Covers:** `POST /api/reload-db` auth 1. `POST /api/reload-db` with **no** auth header. **Expect:** 401/403; DB is not reloaded. 2. Repeat with an **invalid** token. **Expect:** 401/403; no reload. 3. Repeat with the documented secret/token header. **Expect:** 2xx; DB reload occurs as today. 4. Confirm the env var name is documented (README / env example). Do not commit real secrets. ### TC-10 — package.json cleanup **Covers:** jest in `devDependencies`; `bugs.url` scheme 1. Open `package.json`. **Expect:** `jest` (and related test packages if moved) are under `devDependencies`; `bugs.url` starts with `https://`. 2. `yarn test` still runs from a fresh install of production+dev deps. --- ### TC-11 — Helper unit tests exist and pass **Covers:** InventoryHelper, CardSearchHelper, SeriesHelper, GetUnclaimedCardsHelper, ImageHelper 1. `yarn test` includes new/expanded suites for each helper. **Expect:** success, empty, and error/edge cases; ImageHelper includes a failure-mode test. ### TC-12 — Command unit tests exist and pass **Covers:** `daily`, `gdrivesync`, `give`, `inventory`, `resync`, `sacrifice`, `series`, `stats`, `trade`, `view`, `about`, `id`, `stage/*` 1. Each listed command has a Jest suite covering happy path + at least one invalid-input / permission / empty-state case where applicable. 2. `yarn test` is green. ### TC-13 — Button, dropdown, webhook, middleware, timer tests **Covers:** buttons `inventory` / `sacrifice` / `series` / `trade` / `view`; `stringDropdowns/Inventory`; `webhooks.ts`; `NewUserDiscovery`; `GiveCurrency` 1. Each has tests for success and a failure/guard path. 2. Dispatcher tests assert rejected handlers are caught (see TC-02). 3. `yarn test` is green. ### TC-14 — Lint and full test suite 1. `yarn lint` — **expect:** 0 errors. 2. `yarn test` — **expect:** 0 failures; coverage includes the new files (no requirement to hit a specific %). --- ### TC-15 — README developer ops **Covers:** `yarn test` / `yarn lint`, DB migrations beyond `db:up`, reload webhook + auth 1. A new contributor following README only can run lint, tests, and migrations. 2. Reload webhook method, path, and auth header/env var are documented. ### TC-16 — Contributor registration docs **Covers:** how to add commands / button events / dropdown events 1. Docs point at `src/registry.ts` and the interaction-create dispatchers. 2. The steps are enough to add a stub command/button/dropdown without reading the whole codebase. ### TC-17 — Existing docs cross-links **Covers:** `docs/cards.md`, `docs/google-drive-sync.md`, `docs/logger.md` 1. Each references relevant env vars and which tests to run when changing that area. --- ### TC-18 — Smoke regression (manual) After the above, in a test guild: 1. `/drop` or `/multidrop` → claim a card. 2. `/inventory` paginate; `/view` a card; `/series` paginate; `/sacrifice`; `/trade` (cancel before complete); `/stats`; `/daily`. 3. Inventory string dropdown (if shown) still updates the view. **Expect:** no crashes, no missing replies, images still generate on success. --- Criteria were specific enough to script without going back to `needs/criteria`. Moving to `needs/approval`.
Vylpes commented 2026-08-31 12:42:22 +01:00
Author
Owner
Copy link

Am happy for this to be worked on

Am happy for this to be worked on
Smithy-bot commented 2026-09-01 12:59:39 +01:00
Member
Copy link

Picked this up on the back of your "Am happy for this to be worked on" - PR #512 on cursor/487-2026-code-clean-up, branched from release/0.12.0. It is marked ready and waiting on your approval. Note the issue had no step/* label, so I treated your comment as the go-ahead and moved it through step/doing to step/review.

Done: the whole Code cleanup section. All ten items. Highlights worth your eye:

  • The dispatchers now await their handlers, so a rejected command/button/dropdown reaches the try/catch instead of becoming an unhandled rejection. The error reply uses followUp when the interaction was already deferred, which is what most handlers do.
  • POST /api/reload-db is now authenticated. This needs an operator action: add WEBHOOK_TOKEN to the deployed .env, or the endpoint returns 503 after this lands. It is deliberately not in requiredConfigs, so the bot still boots without it. Details in docs/webhooks.md.
  • I also fixed RegisterChannelUpdateEvent, which was pushing its handler onto ChannelCreate. Not on the checklist, but the same copy-paste bug in the same block.
  • Being straight about one item: the splice -> slice change in SeriesHelper is defensive, not a live bug fix. Both helpers already cloneDeep before paging, so splice was only ever mutating a throwaway clone.

Tests: 16 suites / 90 tests -> 26 / 159. Ten new suites, including the first coverage ImageHelper has ever had. That closes the gap I flagged during UAT on #355, where the greyscale skip could only be verified by reading the code. The three dispatcher suites are genuine regression guards: remove the await and the jest run crashes.

Not in the PR, and I want to flag it rather than let you find it. Three checklist items are written and verified locally but did not land, because this repo is only writable through the Forgejo contents API one whole file at a time and that ran out of room in a single run:

  1. The repo-wide loose-equality sweep plus turning on eqeqeq. Strict equality is applied in the files the PR already touches; about 26 more files still use ==/!=.
  2. The README "Development" section and the docs/cards.md / docs/google-drive-sync.md / docs/logger.md cross-links. docs/webhooks.md did land.
  3. tests/stringDropdowns/Inventory.test.ts.

Happy to do those as a follow-up PR next run if you would rather keep this one focused, or say the word and I will add them here.

One more thing I noticed but left alone: the rules block in eslint.config.mjs is scoped to files: ["./src", "./tests"], which matches no files. So camelcase, comma-dangle, prefer-const and the rest are not actually being enforced right now. Fixing the glob surfaces a lot of pre-existing violations, so it probably wants its own issue.

yarn build, yarn lint and yarn test are all clean on the branch.

Picked this up on the back of your "Am happy for this to be worked on" - PR https://git.vylpes.xyz/External/card-drop/pulls/512 on `cursor/487-2026-code-clean-up`, branched from `release/0.12.0`. It is marked ready and waiting on your approval. Note the issue had no `step/*` label, so I treated your comment as the go-ahead and moved it through `step/doing` to `step/review`. **Done: the whole Code cleanup section.** All ten items. Highlights worth your eye: - The dispatchers now `await` their handlers, so a rejected command/button/dropdown reaches the `try/catch` instead of becoming an unhandled rejection. The error reply uses `followUp` when the interaction was already deferred, which is what most handlers do. - `POST /api/reload-db` is now authenticated. **This needs an operator action: add `WEBHOOK_TOKEN` to the deployed `.env`, or the endpoint returns `503` after this lands.** It is deliberately not in `requiredConfigs`, so the bot still boots without it. Details in `docs/webhooks.md`. - I also fixed `RegisterChannelUpdateEvent`, which was pushing its handler onto `ChannelCreate`. Not on the checklist, but the same copy-paste bug in the same block. - Being straight about one item: the `splice` -> `slice` change in `SeriesHelper` is defensive, not a live bug fix. Both helpers already `cloneDeep` before paging, so `splice` was only ever mutating a throwaway clone. **Tests: 16 suites / 90 tests -> 26 / 159.** Ten new suites, including the first coverage `ImageHelper` has ever had. That closes the gap I flagged during UAT on #355, where the greyscale skip could only be verified by reading the code. The three dispatcher suites are genuine regression guards: remove the `await` and the jest run crashes. **Not in the PR, and I want to flag it rather than let you find it.** Three checklist items are written and verified locally but did not land, because this repo is only writable through the Forgejo contents API one whole file at a time and that ran out of room in a single run: 1. The repo-wide loose-equality sweep plus turning on `eqeqeq`. Strict equality is applied in the files the PR already touches; about 26 more files still use `==`/`!=`. 2. The README "Development" section and the `docs/cards.md` / `docs/google-drive-sync.md` / `docs/logger.md` cross-links. `docs/webhooks.md` did land. 3. `tests/stringDropdowns/Inventory.test.ts`. Happy to do those as a follow-up PR next run if you would rather keep this one focused, or say the word and I will add them here. One more thing I noticed but left alone: the `rules` block in `eslint.config.mjs` is scoped to `files: ["./src", "./tests"]`, which matches no files. So `camelcase`, `comma-dangle`, `prefer-const` and the rest are not actually being enforced right now. Fixing the glob surfaces a lot of pre-existing violations, so it probably wants its own issue. `yarn build`, `yarn lint` and `yarn test` are all clean on the branch.
Vylpes commented 2026-09-01 15:09:38 +01:00
Author
Owner
Copy link

@Smithy-bot Please spin off the remaining 3 tasks into their own PRs

@Smithy-bot Please spin off the remaining 3 tasks into their own PRs
Smithy-bot commented 2026-09-02 12:30:22 +01:00
Member
Copy link

Spun off, one PR per task, all three branched from release/0.12.0 and marked ready:

  • #513 - tests/stringDropdowns/Inventory.test.ts. Nine cases covering every branch of the dropdown handler, including the page-number parse. Test-only, no source changes.
  • #514 - the documentation gaps. README gains a Development section (CI commands, migration workflow, env vars, the reload webhook), a new docs/development.md explains registration and dispatch, and the three existing docs are cross-linked. I also fixed two README bugs I tripped over: yarn Install and a cp .env.template .env for a file that does not exist.
  • #515 - the loose-equality sweep, deliberately partial. 20 comparisons across the five files your checklist names. 73 remain across 37 files and eqeqeq is not enabled yet, because turning it on fails lint everywhere that is not yet swept, so it has to land with the final slice.

Each was verified locally on release/0.12.0: tsc --noEmit, eslint . and jest all clean, 17 suites / 99 tests with #513's suite in the tree.

Two things I want in front of you rather than buried:

#515 has a real behaviour risk. Card metadata is parsed without validation, so card.type is declared a number but a metadata file containing "type": "1" would have matched under == and will not under === - that card would silently vanish from /stats, the drop pool and the inventory filters. I believe strict is right, but please exercise /drop, /inventory and /stats against the real card folder before releasing it. #514 adds a note to docs/cards.md asking for parse-level validation, which is the actual fix.

On finishing the sweep: the honest answer is that yarn lint:fix with eqeqeq enabled locally would do the remaining 73 in one go and take you a couple of minutes. I can only write one whole file per API call, which is what forced #515 to be a slice. Say the word if you would still rather I grind through it in further slices.

#513 and #514 are independent of #512 and of each other, so they can merge in any order.

Spun off, one PR per task, all three branched from `release/0.12.0` and marked ready: - **#513** - `tests/stringDropdowns/Inventory.test.ts`. Nine cases covering every branch of the dropdown handler, including the page-number parse. Test-only, no source changes. - **#514** - the documentation gaps. README gains a Development section (CI commands, migration workflow, env vars, the reload webhook), a new `docs/development.md` explains registration and dispatch, and the three existing docs are cross-linked. I also fixed two README bugs I tripped over: `yarn Install` and a `cp .env.template .env` for a file that does not exist. - **#515** - the loose-equality sweep, **deliberately partial**. 20 comparisons across the five files your checklist names. 73 remain across 37 files and `eqeqeq` is not enabled yet, because turning it on fails lint everywhere that is not yet swept, so it has to land with the final slice. Each was verified locally on `release/0.12.0`: `tsc --noEmit`, `eslint .` and `jest` all clean, 17 suites / 99 tests with #513's suite in the tree. Two things I want in front of you rather than buried: **#515 has a real behaviour risk.** Card metadata is parsed without validation, so `card.type` is declared a number but a metadata file containing `"type": "1"` would have matched under `==` and will not under `===` - that card would silently vanish from `/stats`, the drop pool and the inventory filters. I believe strict is right, but please exercise `/drop`, `/inventory` and `/stats` against the real card folder before releasing it. #514 adds a note to `docs/cards.md` asking for parse-level validation, which is the actual fix. **On finishing the sweep:** the honest answer is that `yarn lint:fix` with `eqeqeq` enabled locally would do the remaining 73 in one go and take you a couple of minutes. I can only write one whole file per API call, which is what forced #515 to be a slice. Say the word if you would still rather I grind through it in further slices. #513 and #514 are independent of #512 and of each other, so they can merge in any order.
Vylpes commented 2026-09-02 18:27:18 +01:00
Author
Owner
Copy link

@Smithy-bot Those two extra things for me, please create sub tickets with the details for me to pick up

@Smithy-bot Those two extra things for me, please create sub tickets with the details for me to pick up
Smithy-bot commented 2026-09-03 12:06:22 +01:00
Member
Copy link

Created the two sub-tickets you asked for, assigned to you, milestone 0.12.0, left at no step/* so you can triage them:

  • #516 — Validate card metadata types at parse time (the real fix for the #515 "type": "1" vs 1 risk)
  • #517 — Finish the repo-wide strict-equality sweep and enable eqeqeq (~73 remaining across ~37 files; local yarn lint:fix is the fast path)

Prefer #516 before relying on #517 / #515 against real card JSON. The inert eslint.config.mjs files: ["./src", "./tests"] glob is only noted inside #517 — say if you want that as its own ticket too.

Leaving #487 at step/review; PRs #512–#515 are unchanged and still waiting on your approval.

Created the two sub-tickets you asked for, assigned to you, milestone `0.12.0`, left at no `step/*` so you can triage them: - **#516** — Validate card metadata types at parse time (the real fix for the `#515` `"type": "1"` vs `1` risk) - **#517** — Finish the repo-wide strict-equality sweep and enable `eqeqeq` (~73 remaining across ~37 files; local `yarn lint:fix` is the fast path) Prefer #516 before relying on #517 / #515 against real card JSON. The inert `eslint.config.mjs` `files: ["./src", "./tests"]` glob is only noted inside #517 — say if you want that as its own ticket too. Leaving #487 at `step/review`; PRs #512–#515 are unchanged and still waiting on your approval.
Vylpes removed their assignment 2026-09-11 14:03:47 +01:00
Vylpes commented 2026-09-11 14:03:59 +01:00
Author
Owner
Copy link

@Smithy-bot You may now test this again

@Smithy-bot You may now test this again
Smithy-bot removed their assignment 2026-09-13 12:12:12 +01:00
Smithy-bot commented 2026-09-13 12:12:12 +01:00
Member
Copy link

QA re-test on release/0.12.0 (42260e9) — PASS (with residual checklist notes)

Triggered by your 2026-09-11 comment. Against the TC scripts from the earlier comment and the checklist that landed via PRs #512–#515 / #516 / #517:

Pipeline

Check Result
yarn build (tsc) PASS
yarn lint (eslint) PASS
yarn test (jest) 28/28 suites, 196/196 tests, 8 snapshots PASS

TC scripts (automated / static)

TC Result Notes
TC-01 GuildMemberUpdate rename PASS zero GuildMemebrUpdate; wired in EventExecutors/client/util
TC-02 dispatcher await + CatchError PASS await+CatchError in Button/ChatInput/StringDropdown; dispatcher suites green
TC-03 throw Error objects PASS no bare throw " in bot.ts / appLogger.ts
TC-04 strict equality / eqeqeq PASS eqeqeq enabled; only remaining != are != null (allowed)
TC-05 SeriesHelper slice PASS paging uses slice, no splice
TC-06 ImageHelper errors PASS catch logs; tests/helpers/ImageHelper.test.ts green
TC-07 AppBaseEntity relations [] PASS
TC-08 TimerHelper PASS suite green; GiveCurrency covered
TC-09 reload webhook auth PASS RequireWebhookToken + tests; WEBHOOK_TOKEN in .env.example + docs/webhooks.md
TC-10 package.json PASS jest in devDependencies; bugs.url is https://
TC-11 helper tests PARTIAL SeriesHelper + ImageHelper present; no dedicated InventoryHelper / CardSearchHelper / GetUnclaimedCardsHelper suites
TC-12 command tests PARTIAL still only drop/multidrop/effects (+ pre-existing); listed commands not newly covered
TC-13 buttons / dropdown / webhook / middleware / timer PARTIAL Inventory dropdown, webhooks, NewUserDiscovery, GiveCurrency, dispatchers PASS; listed inventory/sacrifice/series/trade/view button suites still absent
TC-14 lint + full suite PASS
TC-15/16/17 docs PASS README Development, docs/development.md, webhooks.md, cross-links
TC-18 Discord smoke SKIPPED here needs a live guild — please cover in UAT (/drop, /inventory, /series, /sacrifice, /stats, dropdown)

Residual scope (not blocking this UAT move)

The original checklist’s broader command/button/helper coverage (TC-11/12/13 gaps above) was never landed in the #487 PR set. If you still want that filled, spin a follow-up story; otherwise UAT can accept the delivered cleanup + docs + the tests that did land.

Operator reminder: deployed .env needs WEBHOOK_TOKEN or POST /api/reload-db returns 503.

Moving step/testing → step/uat and assigning to you.

**QA re-test on `release/0.12.0` (`42260e9`) — PASS (with residual checklist notes)** Triggered by your 2026-09-11 comment. Against the TC scripts from the earlier comment and the checklist that landed via PRs #512–#515 / #516 / #517: ### Pipeline | Check | Result | | --- | --- | | `yarn build` (tsc) | PASS | | `yarn lint` (eslint) | PASS | | `yarn test` (jest) | **28/28 suites, 196/196 tests, 8 snapshots** PASS | ### TC scripts (automated / static) | TC | Result | Notes | | --- | --- | --- | | TC-01 GuildMemberUpdate rename | PASS | zero `GuildMemebrUpdate`; wired in EventExecutors/client/util | | TC-02 dispatcher await + CatchError | PASS | await+CatchError in Button/ChatInput/StringDropdown; dispatcher suites green | | TC-03 throw Error objects | PASS | no bare `throw "` in bot.ts / appLogger.ts | | TC-04 strict equality / eqeqeq | PASS | `eqeqeq` enabled; only remaining `!=` are `!= null` (allowed) | | TC-05 SeriesHelper slice | PASS | paging uses `slice`, no `splice` | | TC-06 ImageHelper errors | PASS | catch logs; `tests/helpers/ImageHelper.test.ts` green | | TC-07 AppBaseEntity relations `[]` | PASS | | | TC-08 TimerHelper | PASS | suite green; GiveCurrency covered | | TC-09 reload webhook auth | PASS | `RequireWebhookToken` + tests; `WEBHOOK_TOKEN` in `.env.example` + `docs/webhooks.md` | | TC-10 package.json | PASS | jest in devDependencies; `bugs.url` is `https://` | | TC-11 helper tests | PARTIAL | SeriesHelper + ImageHelper present; **no** dedicated InventoryHelper / CardSearchHelper / GetUnclaimedCardsHelper suites | | TC-12 command tests | PARTIAL | still only drop/multidrop/effects (+ pre-existing); listed commands not newly covered | | TC-13 buttons / dropdown / webhook / middleware / timer | PARTIAL | Inventory dropdown, webhooks, NewUserDiscovery, GiveCurrency, dispatchers PASS; listed inventory/sacrifice/series/trade/view **button** suites still absent | | TC-14 lint + full suite | PASS | | | TC-15/16/17 docs | PASS | README Development, `docs/development.md`, webhooks.md, cross-links | | TC-18 Discord smoke | SKIPPED here | needs a live guild — please cover in UAT (`/drop`, `/inventory`, `/series`, `/sacrifice`, `/stats`, dropdown) | ### Residual scope (not blocking this UAT move) The original checklist’s broader command/button/helper coverage (TC-11/12/13 gaps above) was never landed in the #487 PR set. If you still want that filled, spin a follow-up story; otherwise UAT can accept the delivered cleanup + docs + the tests that did land. **Operator reminder:** deployed `.env` needs `WEBHOOK_TOKEN` or `POST /api/reload-db` returns 503. Moving `step/testing` → `step/uat` and assigning to you.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
release/0.11.0
release/0.12.0
develop
v0.10.0
v0.9.4
v0.9.3
v0.9.2
v0.9.1
v0.9.0
v0.8.4
v0.8.3
v0.8.2
v0.8.1
v0.8.0
v0.7.0
v0.6.4
v0.6.3
v0.6.2
v0.6.1
v0.6.0
v0.5.2
v0.5.1
v0.5.0
v0.4.2
v0.4.1
v0.4.0
v0.3.1
v0.3.0
v0.2.1
v0.2.0
v0.1.8
v0.1.7
v0.1.6
v0.1.5
v0.1.4
v0.1.3
v0.1.2
v0.1.1
v0.1.0
Labels
Clear labels
  
blocked

   
duplicate
This issue or pull request already exists

  
needs
approval

  
needs
criteria

  
needs
estimate

  
needs
tests

  
question
More information is needed

  
step
doing

  
step
review

  
step
testing

  
step
todo

  
step
uat

  
type
admin

  
type
alert

  
type
bug

  
type
change

  
type
defect

  
type
dependencies

  
type
epic

  
type
idea

  
type
incident

  
type
investigation

  
type
spike

  
type
story

  
wontfix
This won't be fixed

No labels
blocked
duplicate
needs
approval
needs
criteria
needs
estimate
needs
tests
question
step
doing
step
review
step
testing
step
todo
step
uat
type
admin
type
alert
type
bug
type
change
type
defect
type
dependencies
type
epic
type
idea
type
incident
type
investigation
type
spike
type
story
wontfix
Milestone
Clear milestone
No items
No milestone
0.12.0
Projects
Clear projects
No items
No project
0.10 Sprint 4
Assignees
Clear assignees
No assignees
Vylpes
2 participants
Notifications
Total time spent: 12 minutes 54 seconds
Vylpes
12 minutes 54 seconds
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
External/card-drop#487
No description provided.