Repository navigation
test(utils): add coverage for interactionreply and errorsanitizer - #606
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors SpotifyAuthService with a new internal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 |
|
Deployment failed with the following error: Learn More: https://vercel.com/luksantana?upgradeToPro=build-rate-limit |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.ts (1)
150-156: Tighten the weight assertion precision.Line 155 (
toBeCloseTo(0.3, 1)) is too permissive for validating 0.7/0.3 weighting and may pass after meaningful regression.Proposed assertion hardening
- expect(score).toBeCloseTo(0.3, 1) + expect(score).toBeCloseTo(0.3, 5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.ts` around lines 150 - 156, The test's assertion for calculateSimilarityScore is too loose (toBeCloseTo(0.3, 1)); tighten its precision to catch regressions by increasing the precision parameter (e.g., several decimal places) or use an exact match if deterministic. Update the assertion that checks score computed from makeTrack/makeHistory with cfg to use a stricter matcher (for example toBeCloseTo(0.3, 5) or toBe(0.3)) so the 0.7/0.3 weighting is validated tightly.packages/bot/src/handlers/messageHandler.spec.ts (1)
366-450: Consider table-driven tests for violation permutations.Lines 366-450 duplicate the same setup/assert flow across spam/caps/links/invites/words. A parameterized test would reduce repetition and make new violation types cheaper to add.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/handlers/messageHandler.spec.ts` around lines 366 - 450, Replace the five nearly identical tests in messageHandler.spec.ts with a table-driven (parameterized) test that iterates over violation types; build a rows array describing each case (e.g., {name: 'spam', settingsOverride: {spamEnabled: true}, mockSetter: () => trackMessageAndCheckSpamMock.mockResolvedValue(true)} and similar entries for caps/links/invites/words), call isEnabledMock.mockResolvedValue(true) and getSettingsMock.mockResolvedValue with the row.settingsOverride inside each iteration, invoke the associated mockSetter to make that violation return true, create the message via makeMessage(), call client._handlers['messageCreate'](message) and assert message.delete was called; keep references to makeMessage, client._handlers['messageCreate'], isEnabledMock, getSettingsMock and the specific mocks (trackMessageAndCheckSpamMock, checkCapsMock, checkLinksMock, checkInvitesMock, checkWordsMock) so the test body is generic while each row configures the right mock and settings.packages/bot/src/utils/general/interactionReply.spec.ts (1)
93-605: Extract a shared interaction mock factory to reduce repetition.The repeated mock interaction scaffolding across blocks is large and hard to maintain. A single factory with overrides will make future behavior changes safer and faster.
Refactor sketch
+function createBaseInteraction(overrides: Record<string, unknown> = {}) { + return { + isChatInputCommand: jest.fn(() => false), + isButton: jest.fn(() => false), + isModalSubmit: jest.fn(() => false), + isStringSelectMenu: jest.fn(() => false), + isUserSelectMenu: jest.fn(() => false), + isChannelSelectMenu: jest.fn(() => false), + isRoleSelectMenu: jest.fn(() => false), + isMentionableSelectMenu: jest.fn(() => false), + deferred: false, + replied: false, + deferReply: jest.fn().mockResolvedValue(undefined), + editReply: jest.fn().mockResolvedValue(undefined), + followUp: jest.fn().mockResolvedValue(undefined), + ...overrides, + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/general/interactionReply.spec.ts` around lines 93 - 605, Extract a reusable mock factory function (e.g., createMockInteraction) that returns a default mocked Interaction/ChatInputCommandInteraction/ButtonInteraction/ModalSubmitInteraction with methods and fields used in tests (isChatInputCommand, isButton, isModalSubmit, isStringSelectMenu, isUserSelectMenu, isChannelSelectMenu, isRoleSelectMenu, isMentionableSelectMenu, deferred, replied, deferReply, editReply, followUp) and accept an overrides object to tweak properties per-test; replace repeated inline mockInteraction constructions in each beforeEach and individual tests with calls to createMockInteraction({ ...overrides }) and cast to the appropriate interaction type when needed, keeping tests calling interactionReply(...) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/services/SpotifyAuthService.ts`:
- Around line 9-16: fetchJson performs network I/O with no timeout; wrap the
fetch in an AbortController with a short configurable timeout (e.g. 5s) so
stalled upstream calls don't hang auth requests: inside fetchJson create a
controller, attach a timeout id that calls controller.abort() after the timeout,
merge the controller.signal into the provided init by listening for
init.signal?.addEventListener('abort', () => controller.abort()) and using
{...init, signal: controller.signal}, call fetch(url, mergedInit), clear the
timeout on success or error, and handle aborts by returning null if the fetch
throws due to abort; ensure the timer is always cleaned up.
In `@packages/bot/src/handlers/messageHandler.spec.ts`:
- Line 90: The mock for roles.cache.map currently returns a hardcoded array and
never invokes the provided callback, yielding false positives; update the mock
in the spec so map is implemented to call the supplied callback (e.g., map: (fn)
=> [{ id: '...'}, ...].map(fn)) so mapping logic is actually exercised; locate
the occurrences of roles.cache.map in the test file (the two mock blocks that
return arrays) and replace them with a map implementation that executes fn for
each mocked role object.
- Around line 452-471: The test titled "processes warn action via
moderationService" incorrectly asserts deletion despite intending to exercise
the non-delete (warn) branch; update the test so the moderation decision is
forced to "warn" (mock the function that returns the violation/action — e.g.,
the moderation service method used by client._handlers['messageCreate'] or the
helper that constructs a Violation) while keeping isEnabledMock,
getSettingsMock, trackMessageAndCheckSpamMock, and makeMessage setup, then
invoke client._handlers['messageCreate'](message) and assert createCaseMock was
called and message.delete was not called (instead of asserting message.delete).
Ensure you reference and change the mock that controls action output so the warn
branch executes.
In `@packages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.ts`:
- Around line 34-36: Remove the duplicated test block that calls
calculateStringSimilarity('', '') (the it(...) starting with "returns 1.0 when
one string is empty and other is empty"); keep the canonical test already
present earlier and rename its description to a consistent wording such as
"returns 1.0 when both strings are empty" so the spec reads clearly and no
duplicate assertions remain.
- Around line 166-178: The test "is not affected by config (config param is
unused in score)" falsely proves config-independence by using identical inputs;
change it to use non-identical tracks so differing thresholds would change the
computed score if config were used. Update the spec to create a slightly
different pair (e.g., makeTrack('Song', 'Artist') vs makeHistory('Song Remix',
'Artist B') or similar) and assert that calculateSimilarityScore(t1, t2,
{titleThreshold:0.5, artistThreshold:0.5}) equals calculateSimilarityScore(t1,
t2, {titleThreshold:0.99, artistThreshold:0.99}); keep references to
calculateSimilarityScore, makeTrack, and makeHistory so the intent and
expectations are validated against non-identical inputs.
---
Nitpick comments:
In `@packages/bot/src/handlers/messageHandler.spec.ts`:
- Around line 366-450: Replace the five nearly identical tests in
messageHandler.spec.ts with a table-driven (parameterized) test that iterates
over violation types; build a rows array describing each case (e.g., {name:
'spam', settingsOverride: {spamEnabled: true}, mockSetter: () =>
trackMessageAndCheckSpamMock.mockResolvedValue(true)} and similar entries for
caps/links/invites/words), call isEnabledMock.mockResolvedValue(true) and
getSettingsMock.mockResolvedValue with the row.settingsOverride inside each
iteration, invoke the associated mockSetter to make that violation return true,
create the message via makeMessage(), call
client._handlers['messageCreate'](message) and assert message.delete was called;
keep references to makeMessage, client._handlers['messageCreate'],
isEnabledMock, getSettingsMock and the specific mocks
(trackMessageAndCheckSpamMock, checkCapsMock, checkLinksMock, checkInvitesMock,
checkWordsMock) so the test body is generic while each row configures the right
mock and settings.
In `@packages/bot/src/utils/general/interactionReply.spec.ts`:
- Around line 93-605: Extract a reusable mock factory function (e.g.,
createMockInteraction) that returns a default mocked
Interaction/ChatInputCommandInteraction/ButtonInteraction/ModalSubmitInteraction
with methods and fields used in tests (isChatInputCommand, isButton,
isModalSubmit, isStringSelectMenu, isUserSelectMenu, isChannelSelectMenu,
isRoleSelectMenu, isMentionableSelectMenu, deferred, replied, deferReply,
editReply, followUp) and accept an overrides object to tweak properties
per-test; replace repeated inline mockInteraction constructions in each
beforeEach and individual tests with calls to createMockInteraction({
...overrides }) and cast to the appropriate interaction type when needed,
keeping tests calling interactionReply(...) unchanged.
In `@packages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.ts`:
- Around line 150-156: The test's assertion for calculateSimilarityScore is too
loose (toBeCloseTo(0.3, 1)); tighten its precision to catch regressions by
increasing the precision parameter (e.g., several decimal places) or use an
exact match if deterministic. Update the assertion that checks score computed
from makeTrack/makeHistory with cfg to use a stricter matcher (for example
toBeCloseTo(0.3, 5) or toBe(0.3)) so the 0.7/0.3 weighting is validated tightly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfefb586-bc82-42d5-9c61-3595cf50cb4b
📒 Files selected for processing (6)
packages/backend/src/services/SpotifyAuthService.tspackages/bot/src/handlers/messageHandler.spec.tspackages/bot/src/utils/general/errorSanitizer.spec.tspackages/bot/src/utils/general/interactionReply.spec.tspackages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.tspackages/bot/src/utils/music/ytdlpExtractor/service.spec.ts
📜 Review details
🔇 Additional comments (8)
packages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.ts (1)
21-33: Great coverage depth across core duplicate-detection behaviors.Nice job covering symmetry, thresholds, case-insensitivity, and history filtering paths with clear, readable cases.
Also applies to: 38-141
packages/backend/src/services/SpotifyAuthService.ts (2)
50-55: Good defensive validation for token exchange payload.The guard against missing token fields and explicit API error responses is solid and prevents partially valid auth state from propagating.
67-77: User payload checks and fallback mapping look correct.Validating
userData.idbefore mapping and falling backspotifyUsernametoidis a clean, safe default.packages/bot/src/handlers/messageHandler.spec.ts (2)
276-278: No functional concern in this segment.
473-490: Nice coverage on AutoMod failure and no-guild guards.Line 473 validates error logging when settings fetch fails, and Line 485 validates early return when guild is missing. Good resilience coverage.
packages/bot/src/utils/music/ytdlpExtractor/service.spec.ts (1)
94-273: Strong coverage for extractor execution paths.This suite does a good job covering constructor defaults, URL validation, spawn failures, and timeout behavior with controlled process mocks.
packages/bot/src/utils/general/errorSanitizer.spec.ts (1)
63-458: Great breadth on sanitization and mapping edge cases.This suite thoroughly exercises path scrubbing, technical-pattern replacement, keyword mapping precedence, and multiline/whitespace normalization.
packages/bot/src/utils/general/interactionReply.spec.ts (1)
10-25: No changes needed. The test file's import and mock ordering is correct for the project's CommonJS-based Jest configuration.The original review comment assumes Jest ESM mode, but the repository uses CommonJS. The
packages/bot/jest.config.cjsconfiguration usespreset: 'ts-jest'withtestEnvironment: 'node'and no ESM-specific settings (noextensionsToTreatAsEsm,useESM, orexperimental-vm-modules). In CommonJS mode,jest.mock()is hoisted before module evaluation, making the import-before-mock pattern completely safe. This ordering is used consistently throughout the test suite and is standard Jest practice.> Likely an incorrect or invalid review comment.
| async function fetchJson<T>( | ||
| url: string, | ||
| init: Parameters<typeof fetch>[1], | ||
| ): Promise<T | null> { | ||
| const res = await fetch(url, init) | ||
| if (!res.ok) return null | ||
| return res.json().catch(() => null) as Promise<T | null> | ||
| } |
There was a problem hiding this comment.
Add timeout handling in fetchJson to avoid hanging auth requests.
This helper performs external network I/O without a timeout. A stalled upstream call can tie up request handling and degrade reliability.
🔧 Proposed fix
async function fetchJson<T>(
url: string,
init: Parameters<typeof fetch>[1],
): Promise<T | null> {
- const res = await fetch(url, init)
- if (!res.ok) return null
- return res.json().catch(() => null) as Promise<T | null>
+ const controller = new AbortController()
+ const timeout = setTimeout(() => controller.abort(), 10_000)
+ try {
+ const res = await fetch(url, {
+ ...(init ?? {}),
+ signal: controller.signal,
+ })
+ if (!res.ok) return null
+ return (await res.json()) as T
+ } catch {
+ return null
+ } finally {
+ clearTimeout(timeout)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/services/SpotifyAuthService.ts` around lines 9 - 16,
fetchJson performs network I/O with no timeout; wrap the fetch in an
AbortController with a short configurable timeout (e.g. 5s) so stalled upstream
calls don't hang auth requests: inside fetchJson create a controller, attach a
timeout id that calls controller.abort() after the timeout, merge the
controller.signal into the provided init by listening for
init.signal?.addEventListener('abort', () => controller.abort()) and using
{...init, signal: controller.signal}, call fetch(url, mergedInit), clear the
timeout on success or error, and handle aborts by returning null if the fetch
throws due to abort; ensure the timer is always cleaned up.
| it('processes warn action via moderationService', async () => { | ||
| isEnabledMock.mockResolvedValue(true) | ||
| getSettingsMock.mockResolvedValue({ | ||
| exemptChannels: [], | ||
| exemptRoles: [], | ||
| spamEnabled: true, | ||
| capsEnabled: false, | ||
| linksEnabled: false, | ||
| invitesEnabled: false, | ||
| wordsEnabled: false, | ||
| }) | ||
| trackMessageAndCheckSpamMock.mockResolvedValue(true) | ||
| createCaseMock.mockResolvedValue(undefined) | ||
| const message = makeMessage() | ||
| // Patch the violation action to 'warn' indirectly by making only spam fire and overriding action via mock | ||
| // Since action is hardcoded 'delete' for spam, we test it via a fresh violation scenario | ||
| // The warn/mute/kick/ban branches are hit when action !== 'delete' | ||
| await client._handlers['messageCreate'](message) | ||
| expect(message.delete).toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
warn-action test is misleading and does not verify the intended branch.
At Line 452 the test claims warn-path coverage, but by Line 470 it only asserts deletion. The inline notes at Lines 466-468 also indicate warn is not actually forced. This gives false confidence in moderation case creation coverage.
Suggested fix
it('processes warn action via moderationService', async () => {
@@
- // Patch the violation action to 'warn' indirectly by making only spam fire and overriding action via mock
- // Since action is hardcoded 'delete' for spam, we test it via a fresh violation scenario
- // The warn/mute/kick/ban branches are hit when action !== 'delete'
+ // Configure the exact settings action key used by handleAutoMod to force 'warn' path.
await client._handlers['messageCreate'](message)
expect(message.delete).toHaveBeenCalled()
+ expect(createCaseMock).toHaveBeenCalledWith(
+ expect.objectContaining({ type: 'warn' }),
+ )
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/handlers/messageHandler.spec.ts` around lines 452 - 471, The
test titled "processes warn action via moderationService" incorrectly asserts
deletion despite intending to exercise the non-delete (warn) branch; update the
test so the moderation decision is forced to "warn" (mock the function that
returns the violation/action — e.g., the moderation service method used by
client._handlers['messageCreate'] or the helper that constructs a Violation)
while keeping isEnabledMock, getSettingsMock, trackMessageAndCheckSpamMock, and
makeMessage setup, then invoke client._handlers['messageCreate'](message) and
assert createCaseMock was called and message.delete was not called (instead of
asserting message.delete). Ensure you reference and change the mock that
controls action output so the warn branch executes.
| it('returns 1.0 when one string is empty and other is empty', () => { | ||
| expect(calculateStringSimilarity('', '')).toBe(1.0) | ||
| }) |
There was a problem hiding this comment.
Remove duplicate empty-string test and fix wording.
Line 34 duplicates Lines 26-28, and the title (“one string is empty and other is empty”) is inconsistent wording. Keep one canonical test.
Proposed cleanup
- it('returns 1.0 when one string is empty and other is empty', () => {
- expect(calculateStringSimilarity('', '')).toBe(1.0)
- })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('returns 1.0 when one string is empty and other is empty', () => { | |
| expect(calculateStringSimilarity('', '')).toBe(1.0) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.ts`
around lines 34 - 36, Remove the duplicated test block that calls
calculateStringSimilarity('', '') (the it(...) starting with "returns 1.0 when
one string is empty and other is empty"); keep the canonical test already
present earlier and rename its description to a consistent wording such as
"returns 1.0 when both strings are empty" so the spec reads clearly and no
duplicate assertions remain.
| it('is not affected by config (config param is unused in score)', () => { | ||
| const t1 = makeTrack('Song', 'Artist') | ||
| const t2 = makeHistory('Song', 'Artist') | ||
| const score1 = calculateSimilarityScore(t1, t2, { | ||
| titleThreshold: 0.5, | ||
| artistThreshold: 0.5, | ||
| }) | ||
| const score2 = calculateSimilarityScore(t1, t2, { | ||
| titleThreshold: 0.99, | ||
| artistThreshold: 0.99, | ||
| }) | ||
| expect(score1).toBe(score2) | ||
| }) |
There was a problem hiding this comment.
This case doesn’t actually prove config-independence.
Using identical tracks at Lines 167-168 always yields 1, so the test passes even if config starts influencing non-identical scores later. Use non-identical tracks to make the claim meaningful.
Proposed test adjustment
- const t1 = makeTrack('Song', 'Artist')
- const t2 = makeHistory('Song', 'Artist')
+ const t1 = makeTrack('Song A', 'Artist A')
+ const t2 = makeHistory('Song B', 'Artist A')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('is not affected by config (config param is unused in score)', () => { | |
| const t1 = makeTrack('Song', 'Artist') | |
| const t2 = makeHistory('Song', 'Artist') | |
| const score1 = calculateSimilarityScore(t1, t2, { | |
| titleThreshold: 0.5, | |
| artistThreshold: 0.5, | |
| }) | |
| const score2 = calculateSimilarityScore(t1, t2, { | |
| titleThreshold: 0.99, | |
| artistThreshold: 0.99, | |
| }) | |
| expect(score1).toBe(score2) | |
| }) | |
| it('is not affected by config (config param is unused in score)', () => { | |
| const t1 = makeTrack('Song A', 'Artist A') | |
| const t2 = makeHistory('Song B', 'Artist A') | |
| const score1 = calculateSimilarityScore(t1, t2, { | |
| titleThreshold: 0.5, | |
| artistThreshold: 0.5, | |
| }) | |
| const score2 = calculateSimilarityScore(t1, t2, { | |
| titleThreshold: 0.99, | |
| artistThreshold: 0.99, | |
| }) | |
| expect(score1).toBe(score2) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/duplicateDetection/similarityChecker.spec.ts`
around lines 166 - 178, The test "is not affected by config (config param is
unused in score)" falsely proves config-independence by using identical inputs;
change it to use non-identical tracks so differing thresholds would change the
computed score if config were used. Update the spec to create a slightly
different pair (e.g., makeTrack('Song', 'Artist') vs makeHistory('Song Remix',
'Artist B') or similar) and assert that calculateSimilarityScore(t1, t2,
{titleThreshold:0.5, artistThreshold:0.5}) equals calculateSimilarityScore(t1,
t2, {titleThreshold:0.99, artistThreshold:0.99}); keep references to
calculateSimilarityScore, makeTrack, and makeHistory so the intent and
expectations are validated against non-identical inputs.
- interactionreply: 60 tests for all interaction types and scenarios - errorsanitizer: 65 tests for sanitization and error mapping
35 tests for calculateStringSimilarity/areTracksSimilar/findSimilarTracks/ calculateSimilarityScore; 20 tests for YtDlpExtractorService covering constructor defaults, validate(), handle() happy/error/timeout paths.
extract validation and api call logic into helper functions to bring the exchangeCodeForToken function's cyclomatic complexity below the 15-branch limit. this fixes the quality gates check while maintaining identical behavior.
045aae7 to
7cd68d6
Compare
|
* test(utils): add coverage for interactionreply and errorsanitizer - interactionreply: 60 tests for all interaction types and scenarios - errorsanitizer: 65 tests for sanitization and error mapping * test(bot): add similarity checker and yt-dlp extractor unit tests 35 tests for calculateStringSimilarity/areTracksSimilar/findSimilarTracks/ calculateSimilarityScore; 20 tests for YtDlpExtractorService covering constructor defaults, validate(), handle() happy/error/timeout paths. * refactor: reduce cyclomatic complexity in spotifyauthservice extract validation and api call logic into helper functions to bring the exchangeCodeForToken function's cyclomatic complexity below the 15-branch limit. this fixes the quality gates check while maintaining identical behavior. * test(message-handler): fix map.map bug, add violation detection tests * fix: use factory mocks in interactionreply spec to avoid esm parse error



Phase 4 test quality audit. Adds 125 tests total:
Summary by CodeRabbit
Tests
Refactor