🌈 improve(testing): consolidate test utilities#6
Conversation
Extract shared replyWithResult helper in interaction-router, consolidate assertDefined and createMockCommand into test-helpers, fix type casts, catch param naming, and redundant returns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Brian L. <brian@uniquepixels.xyz>
📝 WalkthroughWalkthroughPull request consolidates test utilities into a shared module, updates dependency versions and configuration settings, refactors error handling in core libraries, and introduces a common abstraction for interaction reply logic in the router. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/sparks/interaction-router.ts (1)
39-47: 🧹 Nitpick | 🔵 TrivialConsider simplifying the condition for readability.
The double-negation
!(result.ok || interaction.replied)is semantically equivalent to!result.ok && !interaction.replied. The latter reads more directly as "failed AND not yet replied."Suggested fix
- if (!(result.ok || interaction.replied)) { + if (!result.ok && !interaction.replied) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/sparks/interaction-router.ts` around lines 39 - 47, Replace the negated OR condition with an explicit AND for clarity: change the conditional in the interaction handling block that currently reads if (!(result.ok || interaction.replied)) to if (!result.ok && !interaction.replied) so the logic reads "failed AND not yet replied"; keep the inner branches (checking interaction.deferred and calling interaction.editReply or interaction.reply with MessageFlags.Ephemeral and result.reason) unchanged so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 34-35: The override for undici currently uses an open-ended range
(">=7.22.0") which can allow undici v8+ to be injected; update the override
entry under "overrides" to pin undici to a specific, tested patched version
(e.g., replace the value for "undici" with "7.22.0") so the transitive graph
cannot silently drift to incompatible majors; locate the "overrides" object and
the "undici" key in package.json to make this change.
In `@src/core/lib/test-helpers/index.ts`:
- Around line 27-30: The file fails the formatter (Biome) because the exported
helper signature formatting changed; run Biome/your project's formatter on
src/core/lib/test-helpers/index.ts (or reformat the assertDefined<T>(val: T |
undefined | null): asserts val is NonNullable<T> function) to match the
project's style rules, then commit the formatted result; ensure the exported
symbol assertDefined retains the same generic and assertion signature but with
spacing/linebreaks corrected per Biome so the format check passes.
In `@src/core/lib/test-helpers/mock-interaction.ts`:
- Around line 239-240: The createMockCommand mock currently returns a bare
object cast to SlashCommandBuilder which lacks the builder methods used
elsewhere (e.g., toJSON, setIntegrationTypes, setContexts) and will fail at
runtime; update createMockCommand to return an object shaped like a
SlashCommandBuilder that implements the methods toJSON(),
setIntegrationTypes(...), and setContexts(...) (each returning the builder for
chaining) and includes the name property so code in register-commands that calls
toJSON() or chains setIntegrationTypes/setContexts will work correctly.
In `@src/core/sparks/component.ts`:
- Line 380: The function that currently ends with "return undefined;" should be
changed to a bare "return;" or simply allow the function to fall through, since
its declared return type is BaseComponentSpark | undefined; remove the explicit
undefined token at the end of the function (the statement returning undefined)
so the function implicitly returns undefined instead, ensuring compliance with
the Biome lint rule and preserving the same behavior for callers expecting
BaseComponentSpark | undefined.
---
Outside diff comments:
In `@src/core/sparks/interaction-router.ts`:
- Around line 39-47: Replace the negated OR condition with an explicit AND for
clarity: change the conditional in the interaction handling block that currently
reads if (!(result.ok || interaction.replied)) to if (!result.ok &&
!interaction.replied) so the logic reads "failed AND not yet replied"; keep the
inner branches (checking interaction.deferred and calling interaction.editReply
or interaction.reply with MessageFlags.Ephemeral and result.reason) unchanged so
behavior remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05f0d22c-1ce2-4d9e-8afc-a5c980cf146c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/ci-release.yml.vscode/settings.jsonpackage.jsonrenovate.jsonsrc/core/configuration/schema-helpers.test.tssrc/core/guards/index.test.tssrc/core/lib/attempt/index.tssrc/core/lib/logger/index.test.tssrc/core/lib/test-helpers/index.tssrc/core/lib/test-helpers/mock-interaction.tssrc/core/sparks/command-group.test.tssrc/core/sparks/command.test.tssrc/core/sparks/component.tssrc/core/sparks/interaction-router.tssrc/core/sparks/scheduled-event.test.tssrc/guards/built-in/has-channel.test.tssrc/guards/built-in/special-channels.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**: General project rules:
- Runtime is Bun, not Node.js. Do not suggest node/npm/yarn patterns.
- Zod v4 is used.
z.enum()handles native TS enums directly;
z.nativeEnum()is deprecated. Do not flagz.enum(SomeEnum)as incorrect.attempt()is the project's Result-type wrapper. All fallible calls in
sparks must use it — never raw try/catch or uncaught promises.import typeis required for type-only imports (verbatimModuleSyntax).- Biome enforces kebab-case filenames and organized imports.
Files:
src/core/sparks/component.tsrenovate.jsonsrc/core/lib/test-helpers/mock-interaction.tssrc/core/lib/attempt/index.tssrc/core/configuration/schema-helpers.test.tssrc/guards/built-in/has-channel.test.tssrc/core/guards/index.test.tssrc/core/lib/logger/index.test.tssrc/core/lib/test-helpers/index.tssrc/core/sparks/interaction-router.tssrc/core/sparks/command.test.tssrc/core/sparks/command-group.test.tspackage.jsonsrc/guards/built-in/special-channels.test.tssrc/core/sparks/scheduled-event.test.ts
src/core/**
⚙️ CodeRabbit configuration file
src/core/**: Core framework code. Focus on:
- Type safety and proper TypeScript patterns
- Error handling with
attempt()— no raw try/catch in sparks- API design consistency and backward compatibility
- Performance implications for hot paths (interaction routing, guard chains)
Files:
src/core/sparks/component.tssrc/core/lib/test-helpers/mock-interaction.tssrc/core/lib/attempt/index.tssrc/core/configuration/schema-helpers.test.tssrc/core/guards/index.test.tssrc/core/lib/logger/index.test.tssrc/core/lib/test-helpers/index.tssrc/core/sparks/interaction-router.tssrc/core/sparks/command.test.tssrc/core/sparks/command-group.test.tssrc/core/sparks/scheduled-event.test.ts
.github/**
⚙️ CodeRabbit configuration file
.github/**: CI/CD configuration. Check for:
- Pinned action versions with hash comments
- Harden Runner with egress blocking on all jobs
- No credential or secret exposure risks
- Correct branch targeting (main vs develop)
Files:
.github/workflows/ci-release.yml
**/*.test.ts
⚙️ CodeRabbit configuration file
**/*.test.ts: Test files use Bun's test runner. Verify:
- Meaningful, descriptive test names
- Edge cases and error paths covered
- No flaky patterns (timers, network, race conditions)
- DRY — shared setup extracted into file-local helpers
- 100% coverage is the project threshold
Files:
src/core/configuration/schema-helpers.test.tssrc/guards/built-in/has-channel.test.tssrc/core/guards/index.test.tssrc/core/lib/logger/index.test.tssrc/core/sparks/command.test.tssrc/core/sparks/command-group.test.tssrc/guards/built-in/special-channels.test.tssrc/core/sparks/scheduled-event.test.ts
src/guards/**
⚙️ CodeRabbit configuration file
src/guards/**: Guards are composable validation functions. Ensure:
- Proper TypeScript type narrowing via generics
- Clear, actionable error messages in GuardResult
- No side effects — guards must be pure predicates
- Consistent pattern with existing built-in guards
Files:
src/guards/built-in/has-channel.test.tssrc/guards/built-in/special-channels.test.ts
🪛 GitHub Check: Lint
src/core/sparks/component.ts
[notice] 380-380: lint/complexity/noUselessUndefined
Don't use unnecessary undefined.
src/core/lib/test-helpers/index.ts
[failure] 1-1: format
File content differs from formatting output
🪛 GitHub Check: SonarCloud Code Analysis
src/core/guards/index.test.ts
[warning] 711-711: This assertion is unnecessary since it does not change the type of the expression.
🔇 Additional comments (10)
renovate.json (1)
8-9: Formatting-only cleanup looks good.These edits keep the Renovate settings and match patterns unchanged while making the arrays more compact. No concerns here.
Also applies to: 15-15, 21-25, 30-30, 36-36, 42-42, 48-48, 53-53
src/core/lib/attempt/index.ts (1)
148-168: LGTM!Clean refactor renaming the catch parameter to
error_to avoid shadowing theconst errordeclaration on line 163. The error handling logic is sound:
- Defensive
JSON.stringifywith fallback toString()for circular references- Properly preserves error chain via
causesrc/core/sparks/interaction-router.ts (3)
15-24: LGTM!Clean abstractions.
RepliableInteractioncaptures the minimal required surface from discord.js interaction types.ExecuteResultis structurally compatible withGuardResult<T>(the extravaluefield on success is safely ignored).
71-99: LGTM!Clean refactoring. The
spark.execute()result (typed asGuardResult<TGuarded>) is structurally compatible withExecuteResult, and the delegation toreplyWithResultcentralizes the post-execution reply logic cleanly.
157-165: LGTM!Consistent refactoring pattern with
handleCommand. The centralized reply handling viareplyWithResulteliminates duplication and ensures uniform behavior for both commands and components.src/guards/built-in/has-channel.test.ts (1)
4-4: LGTM!The import consolidation from local helper to shared
@/core/lib/test-helpersmodule aligns with the DRY principle and the PR's objective of centralizing test utilities.src/core/sparks/scheduled-event.test.ts (1)
4-4: LGTM!The consolidated import of
assertDefinedandcreateMockClientfrom the shared test-helpers module is correct. Both utilities are used appropriately throughout the test file.src/guards/built-in/special-channels.test.ts (1)
5-5: LGTM!The import consolidation is correct. The parameterized test pattern using
guardTestCasesis well-structured and aligns with the DRY principle.src/core/sparks/command-group.test.ts (1)
9-13: LGTM!The consolidated imports from
@/core/lib/test-helpersare correct and all three utilities (assertDefined,createMockClient,createMockCommand) are used throughout this comprehensive test suite.src/core/sparks/command.test.ts (1)
9-17: LGTM!The consolidated imports from
@/core/lib/test-helpersare well-organized. Both newly added imports (assertDefined,createMockCommand) along with the existing helpers (createMockAutocompleteInteraction,createMockChatInputInteraction,createMockClient,failGuard,passThroughGuard) are appropriately used throughout the test suite.
Revert to bare return per Biome preference, fix assertDefined type narrowing to use NonNullable<T>. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Brian L. <brian@uniquepixels.xyz>
Prevent open-ended >=7.22.0 range from drifting to incompatible major versions while discord.js still bundles vulnerable undici. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Brian L. <brian@uniquepixels.xyz>
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
package.json (1)
35-35:⚠️ Potential issue | 🟠 MajorThe override still forces an unsupported
undicimajor for the Discord stack.
^7.22.0avoids v8 drift, but it still rewritesdiscord.js@14.25.1/@discordjs/restoff their pinnedundici@6.21.3. Unless those packages were upgraded to releases that officially support v7, this remains a compatibility risk; prefer the upstream-tested version or remove the override until that upgrade happens.What undici version is declared by discord.js 14.25.1 and `@discordjs/rest` 2.6.0, and do those releases officially support being overridden to undici v7?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 35, The package.json override forcing "undici": "^7.22.0" conflicts with the versions pinned by discord.js@14.25.1 and `@discordjs/rest` (which expect undici@6.21.3); either remove the override or change it to the upstream-tested "6.21.3" until those libraries are upgraded to officially support undici v7 — locate the "undici" entry in package.json and update or delete the override, then run a fresh install and verify discord.js/@discordjs/rest still resolve to compatible undici@6.21.3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package.json`:
- Line 35: The package.json override forcing "undici": "^7.22.0" conflicts with
the versions pinned by discord.js@14.25.1 and `@discordjs/rest` (which expect
undici@6.21.3); either remove the override or change it to the upstream-tested
"6.21.3" until those libraries are upgraded to officially support undici v7 —
locate the "undici" entry in package.json and update or delete the override,
then run a fresh install and verify discord.js/@discordjs/rest still resolve to
compatible undici@6.21.3.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 339ed8e9-1083-48ad-ab5c-3e9a8f38b945
📒 Files selected for processing (2)
package.jsonsrc/core/lib/test-helpers/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ CodeRabbit configuration file
**: General project rules:
- Runtime is Bun, not Node.js. Do not suggest node/npm/yarn patterns.
- Zod v4 is used.
z.enum()handles native TS enums directly;
z.nativeEnum()is deprecated. Do not flagz.enum(SomeEnum)as incorrect.attempt()is the project's Result-type wrapper. All fallible calls in
sparks must use it — never raw try/catch or uncaught promises.import typeis required for type-only imports (verbatimModuleSyntax).- Biome enforces kebab-case filenames and organized imports.
Files:
src/core/lib/test-helpers/index.tspackage.json
src/core/**
⚙️ CodeRabbit configuration file
src/core/**: Core framework code. Focus on:
- Type safety and proper TypeScript patterns
- Error handling with
attempt()— no raw try/catch in sparks- API design consistency and backward compatibility
- Performance implications for hot paths (interaction routing, guard chains)
Files:
src/core/lib/test-helpers/index.ts
🔇 Additional comments (2)
src/core/lib/test-helpers/index.ts (2)
1-2: Good shared assertion helper.
assertDefinedgives callers properNonNullable<T>narrowing, and usingexpect()here keeps failures readable while centralizing the null/undefined check in one reusable test utility.Also applies to: 23-32
10-10: Nice barrel export.Re-exporting
createMockCommandhere keeps the test-helper surface consistent across suites.
UniquePixels
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
|
@coderabbitai had to change branch, please reapprove |
|
✅ Actions performedComments resolved and changes approved. |
## Release v0.1.1 ### 🌈 Improvements - reduce cognitive complexity in health check handler (#9) - detect all Biome finding severities as failures (#8) - revamp badge row (#21) - add Renovate commit format and squash strategy (#10) - consolidate test utilities (#6) ### 🧺 Chores - update undici to v7 - update install-action digest --- **After merging**, the tag-release job will automatically: - Tag this commit as `v0.1.1` - Create a GitHub Release with the changelog
1 participant
Summary by CodeRabbit
Refactor
Chores