test: full audit - +55 tests covering archiveRecordingToStorage, health-checks, stripe-retry, rate-limit#336
Merged
Merged
Conversation
… stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds three Vitest test suites that cover: recording archival-to-storage and failure logging, health-check utilities (Telnyx/Supabase) including timeout/error branches, and Stripe retry/backoff behavior with fake timers and mocked external interactions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/archive-recording.test.ts`:
- Around line 377-381: In the 'rate-limit additional coverage' describe block,
add a localStorage cleanup in the beforeEach to ensure test isolation: keep the
existing vi.resetModules() call and also call localStorage.clear() (or
removeItem for specific keys) so tests that rely on src/lib/rate-limit.ts and
Date.now() (and any fake timers) can't collide on keys; update the beforeEach
that currently references vi.resetModules to include the clear.
- Around line 281-284: The test is re-calling storage.from() which returns a
fresh mock and may not reflect the actual upload call; instead obtain the actual
mock instance created when storage.from('recordings') was invoked (e.g., via
storage.from.mock.results[0].value.upload or by referencing
storage.from('recordings') without re-invoking in a way that creates a new mock)
and then assert against that upload mock's recorded calls
(upload.mock.calls[0][0]) to check the uploaded path contains '.wav'; update the
assertions that reference uploadCall to use that recorded mock instance rather
than calling storage.from() again.
In `@src/__tests__/health-checks-lib.test.ts`:
- Around line 25-31: The test "clears timeout when fetch completes" currently
only asserts the success path; update it to verify timer cleanup for
fetchWithTimeout by using Jest fake timers (jest.useFakeTimers()) before
importing/calling fetchWithTimeout, call the function, await the promise
resolution, then assert there are no pending timers
(expect(jest.getTimerCount()).toBe(0)) or spy on clearTimeout toHaveBeenCalled
with the timeout id; finally restore real timers (jest.useRealTimers()). Ensure
you reference fetchWithTimeout from the import and wrap the fake-timer
setup/teardown around that test.
- Around line 196-199: The test "uses TELNYX_CREDENTIAL_CONNECTION_ID fallback
when CONNECTION_ID not set" is non-deterministic because it doesn't clear
TELNYX_CONNECTION_ID; change the test setup to explicitly unset/clear
TELNYX_CONNECTION_ID before stubbing TELNYX_CREDENTIAL_CONNECTION_ID so the
fallback path is exercised reliably—i.e., ensure the test (in
src/__tests__/health-checks-lib.test.ts) removes TELNYX_CONNECTION_ID from the
environment (or stubs it to undefined/null) prior to calling
vi.stubEnv('TELNYX_CREDENTIAL_CONNECTION_ID', 'conn-fallback') and the other env
stubs.
- Line 5: The global fetch stub created via vi.stubGlobal('fetch', mockFetch) is
never undone and leaks into other tests; add cleanup to restore that global stub
(for example add an afterAll or afterEach that calls vi.unstubAllGlobals())
alongside the existing vi.unstubAllEnvs() so the global fetch mock is removed
when the suite finishes.
In `@src/__tests__/stripe-retry-lib.test.ts`:
- Around line 42-47: The test currently uses a broad
expect(...).rejects.toThrow() which can pass for unrelated errors; change the
assertion to check the exact error instance thrown by withStripeRetry. In the
test for immediate rethrow (using Stripe.errors.StripeConnectionError and
vi.fn() mockRejectedValue(err)), replace rejects.toThrow() with await
expect(withStripeRetry(fn, 3)).rejects.toBe(err) (or .rejects.toEqual(err) if
instance comparison fails) so the test asserts the exact error identity for the
immediate rethrow path.
- Around line 87-112: Add pre-boundary assertions to both tests to ensure no
early retry: for the 'caps retry-after at 10 seconds' test (using
withStripeRetry) assert the mock fn has not been called before advancing timers
to just under 10s (e.g., advanceTimersByTimeAsync(10000) or check immediately)
then advance the remaining ms and assert it was called/resolved; similarly for
the 'uses default 2s retry when retry-after header missing' test assert fn has
not been called before advancing to just under 2000ms, then advance the
remaining ms and assert resolution. Reference the withStripeRetry helper and the
vi.fn() mock used in each test when adding these checks.
- Around line 1-2: The top-level import of Stripe causes constructor-identity
mismatches after vi.resetModules(); modify the tests in
src/__tests__/stripe-retry-lib.test.ts to import Stripe dynamically inside each
test or before constructing test errors (e.g., move the import into the
beforeEach or each it block after calling vi.resetModules()), so that instances
created in the tests and the isStripeConnectionError() instanceof checks use the
same Stripe constructor reference; ensure any code that constructs new
Stripe.StripeError or checks instanceof Stripe is updated to use the
locally-imported Stripe within that scope.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: aac7705b-46a3-420b-8395-d9cd926af5eb
📒 Files selected for processing (3)
src/__tests__/archive-recording.test.tssrc/__tests__/health-checks-lib.test.tssrc/__tests__/stripe-retry-lib.test.ts
Comment on lines
+377
to
+381
| describe('rate-limit additional coverage', () => { | ||
| beforeEach(() => { | ||
| vi.resetModules() | ||
| }) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider clearing localStorage between tests for proper isolation.
The rate-limit functions use localStorage (per the implementation in src/lib/rate-limit.ts). While vi.resetModules() resets module state, it doesn't clear localStorage. Using unique keys with Date.now() mitigates this, but tests using fake timers may generate colliding keys if Date.now() returns the same value.
♻️ Suggested addition: clear localStorage in beforeEach
describe('rate-limit additional coverage', () => {
beforeEach(() => {
vi.resetModules()
+ localStorage.clear()
})📝 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.
Suggested change
| describe('rate-limit additional coverage', () => { | |
| beforeEach(() => { | |
| vi.resetModules() | |
| }) | |
| describe('rate-limit additional coverage', () => { | |
| beforeEach(() => { | |
| vi.resetModules() | |
| localStorage.clear() | |
| }) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/archive-recording.test.ts` around lines 377 - 381, In the
'rate-limit additional coverage' describe block, add a localStorage cleanup in
the beforeEach to ensure test isolation: keep the existing vi.resetModules()
call and also call localStorage.clear() (or removeItem for specific keys) so
tests that rely on src/lib/rate-limit.ts and Date.now() (and any fake timers)
can't collide on keys; update the beforeEach that currently references
vi.resetModules to include the clear.
Comment on lines
+1
to
+2
| import { afterEach, describe, it, expect, vi, beforeEach } from 'vitest' | ||
| import Stripe from 'stripe' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest, does vi.resetModules() reload module instances in a way that can break instanceof checks when a class is imported at top-level but the module under test is dynamically re-imported?
💡 Result:
Yes, vi.resetModules can break instanceof checks in this scenario. vi.resetModules clears the module cache, causing dynamically re-imported modules (via await import) to be re-evaluated and produce new class constructors. These new constructors are distinct from the original class constructor imported at the top-level, which remains the cached version and cannot be re-evaluated. As a result, instanceof checks between instances from the dynamic import and the top-level class will fail because they reference different constructor functions. Official Vitest documentation explicitly states: "Top-level imports cannot be re-evaluated" after vi.resetModules, and demonstrates usage with dynamic imports to re-evaluate modules. Similar issues occur in Jest with resetModules, where re-importing after reset creates new class instances that fail instanceof against the original. To avoid breakage, either avoid top-level imports of classes used in instanceof checks with the module under test, use dynamic imports consistently for both, or refrain from using vi.resetModules when such checks are needed.
Citations:
- 1: https://v2.vitest.dev/api/vi
- 2: https://vitest.dev/api/vi.html
- 3: resetModules does not restore state vitest-dev/vitest#3058
- 4: [Bug]: resetModules breaks instanceof jestjs/jest#15258
- 5: Bug: Models break with jest config resetModules: true Vincit/objection.js#1458
🏁 Script executed:
wc -l src/__tests__/stripe-retry-lib.test.tsRepository: coydevs/policyjar
Length of output: 103
🏁 Script executed:
cat -n src/__tests__/stripe-retry-lib.test.tsRepository: coydevs/policyjar
Length of output: 5910
🏁 Script executed:
fd -t f stripe-retry src/libRepository: coydevs/policyjar
Length of output: 84
🏁 Script executed:
cat -n src/lib/stripe-retry.tsRepository: coydevs/policyjar
Length of output: 1775
Import Stripe dynamically in tests to avoid constructor-identity brittleness with vi.resetModules().
The test file imports Stripe at the top level (line 2), but vi.resetModules() in beforeEach (line 5-6) causes the module under test to reload with a fresh Stripe reference. This creates a constructor mismatch: instances created in tests use the original cached Stripe, while isStripeConnectionError() checks instanceof against a newly-reloaded Stripe. Although current tests may pass, this pattern is fragile and can fail depending on module initialization order. Import Stripe inside each test block (after reset) before constructing errors to ensure the instance and instanceof check use the same constructor reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/stripe-retry-lib.test.ts` around lines 1 - 2, The top-level
import of Stripe causes constructor-identity mismatches after vi.resetModules();
modify the tests in src/__tests__/stripe-retry-lib.test.ts to import Stripe
dynamically inside each test or before constructing test errors (e.g., move the
import into the beforeEach or each it block after calling vi.resetModules()), so
that instances created in the tests and the isStripeConnectionError() instanceof
checks use the same Stripe constructor reference; ensure any code that
constructs new Stripe.StripeError or checks instanceof Stripe is updated to use
the locally-imported Stripe within that scope.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/__tests__/stripe-retry-lib.test.ts (1)
1-2:⚠️ Potential issue | 🟠 MajorAvoid top-level
Stripeimport when usingvi.resetModules().Line [2] imports
Stripeat module scope, while Line [6] resets modules and tests dynamically re-import@/lib/stripe-retry. This can cause constructor-identity drift and brittleinstanceofbehavior inisStripeConnectionError.Suggested fix
-import Stripe from 'stripe' @@ - const { isStripeConnectionError } = await import('@/lib/stripe-retry') - const err = new Stripe.errors.StripeConnectionError({ message: 'timeout', type: 'connection_error' as any }) + const [{ isStripeConnectionError }, { default: Stripe }] = await Promise.all([ + import('@/lib/stripe-retry'), + import('stripe'), + ]) + const err = new Stripe.errors.StripeConnectionError({ message: 'timeout', type: 'connection_error' as any }) @@ - const { withStripeRetry } = await import('@/lib/stripe-retry') - const err = new Stripe.errors.StripeConnectionError({ message: 'timeout', type: 'connection_error' as any }) + const [{ withStripeRetry }, { default: Stripe }] = await Promise.all([ + import('@/lib/stripe-retry'), + import('stripe'), + ]) + const err = new Stripe.errors.StripeConnectionError({ message: 'timeout', type: 'connection_error' as any })In Vitest, does vi.resetModules() re-evaluate dynamically imported modules while leaving top-level imports intact, causing instanceof constructor-identity mismatches?Also applies to: 6-7, 16-18, 42-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/stripe-retry-lib.test.ts` around lines 1 - 2, The top-level import of Stripe causes constructor-identity mismatches when tests call vi.resetModules(); move the Stripe import so it is loaded after resetModules to ensure a fresh constructor: remove the module-scoped "import Stripe from 'stripe'" and instead dynamically import Stripe inside beforeEach or inside each test (e.g. const { default: Stripe } = await import('stripe')) after calling vi.resetModules(); update any places that construct Stripe errors or check instanceof (and references to isStripeConnectionError) to use the locally imported Stripe so instanceof comparisons use the same constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/archive-recording.test.ts`:
- Around line 309-325: The test "falls back to Telnyx URL when storage upload
fails" is missing assertions: after calling handleRecordingSaved(BASE_PAYLOAD)
and awaiting timers, verify that the admin client was called to update the call
(via the mock admin returned by createAdminClientMock) with a payload that keeps
recording_url equal to the original Telnyx URL from
BASE_CALL_RECORD/BASE_PAYLOAD and that no archival key field (e.g., archive_key
or similar) was set; update the test to inspect the mock admin's
updateCalls/updateCall invocation arguments (from makeAdmin /
createAdminClientMock) and assert recording_url unchanged and archival key
undefined/null.
- Around line 247-264: The test "inserts call_update_failures and logs when
failure insert itself errors" currently only ensures no throw; update the test
to assert that the storage insert for the failure table was attempted and that
console.error was invoked: after calling handleRecordingSaved(BASE_PAYLOAD) and
awaiting timers, add assertions that the storage mock (from makeStorageMock /
admin.storage) received an insert call for "call_update_failures" (or the method
name used on the mock) and that the consoleSpy (vi.spyOn(console, 'error')) was
called with an error message (or called at least once); use
createAdminClientMock, admin, fetchMock setup as-is and restore the spy at the
end.
---
Duplicate comments:
In `@src/__tests__/stripe-retry-lib.test.ts`:
- Around line 1-2: The top-level import of Stripe causes constructor-identity
mismatches when tests call vi.resetModules(); move the Stripe import so it is
loaded after resetModules to ensure a fresh constructor: remove the
module-scoped "import Stripe from 'stripe'" and instead dynamically import
Stripe inside beforeEach or inside each test (e.g. const { default: Stripe } =
await import('stripe')) after calling vi.resetModules(); update any places that
construct Stripe errors or check instanceof (and references to
isStripeConnectionError) to use the locally imported Stripe so instanceof
comparisons use the same constructor.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c21e869d-b181-4c3b-89d7-0323b3c0193d
📒 Files selected for processing (3)
src/__tests__/archive-recording.test.tssrc/__tests__/health-checks-lib.test.tssrc/__tests__/stripe-retry-lib.test.ts
Collaborator
Author
|
@coderabbitai full review - all 9 findings addressed in latest commit. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/archive-recording.test.ts`:
- Around line 291-311: The test "handles .ogg URL without content-type header"
currently only checks storage.from was called; update it to assert the uploaded
filename/path includes the .ogg extension. Locate the mock/storage invocation
(storage.from) used in the test and add an assertion that inspects the mocked
call arguments (the path/filename argument passed to storage.from or the
uploaded object key) and verifies it contains ".ogg", mirroring the assertion
pattern used in the `.wav` test; keep the rest of the test unchanged.
- Around line 210-225: Add an assertion that verifies the fallback to the
original Telnyx URL by examining the admin inserts for the 'calls' update after
calling handleRecordingSaved; specifically, find the insert/update where i.table
=== 'calls' and assert its payload uses the original Telnyx recording URL (e.g.,
BASE_CALL_RECORD.recording_url or the same constant used by other fallback
tests) to confirm the fallback was applied when fetchMock resolved with ok:
false.
- Around line 191-208: The test for handleRecordingSaved should assert the
permanent URL behavior in addition to storage.from: ensure the mock storage's
upload method (from the makeStorageMock return) was called with the expected
bucket/key and file data, assert that the admin client (from makeAdmin /
createAdminClientMock) received an update to the call record using the
signed/permanent URL (not the original Telnyx URL) and that the call record's
recording_s3_key field was set; locate and update the test around
handleRecordingSaved(BASE_PAYLOAD), storage (makeStorageMock), and the admin
mock (makeAdmin / createAdminClientMock) to add these assertions.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 59f6711d-d8cc-4c15-844b-13084ca0b0f8
📒 Files selected for processing (1)
src/__tests__/archive-recording.test.ts
Comment on lines
+191
to
+208
| it('archives recording to storage on success and uses permanent URL', async () => { | ||
| const storage = makeStorageMock() | ||
| const admin = makeAdmin({ callRecord: BASE_CALL_RECORD, storage }) | ||
| createAdminClientMock.mockReturnValue(admin) | ||
|
|
||
| fetchMock.mockResolvedValue({ | ||
| ok: true, | ||
| headers: { get: (h: string) => (h === 'content-type' ? 'audio/mpeg' : null) }, | ||
| arrayBuffer: vi.fn().mockResolvedValue(new ArrayBuffer(100)), | ||
| }) | ||
|
|
||
| const p = handleRecordingSaved(BASE_PAYLOAD) | ||
| await vi.runAllTimersAsync() | ||
| await p | ||
|
|
||
| // Should have called storage upload | ||
| expect(storage.from).toHaveBeenCalledWith('recordings') | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Success test lacks key assertions for the "permanent URL" behavior.
The test name claims it "uses permanent URL" but only verifies storage.from was called. It doesn't assert that:
- The upload was actually invoked
- The
callsupdate used the signed URL (permanent URL) instead of the original Telnyx URL - The
recording_s3_keywas set
♻️ Suggested stronger assertions
const p = handleRecordingSaved(BASE_PAYLOAD)
await vi.runAllTimersAsync()
await p
- // Should have called storage upload
expect(storage.from).toHaveBeenCalledWith('recordings')
+ const uploadMock = storage.from.mock.results[0].value.upload
+ expect(uploadMock).toHaveBeenCalled()
+
+ // Verify calls update used permanent (signed) URL, not original Telnyx URL
+ const callUpdate = admin.updates.find((u) => u.table === 'calls')
+ expect(callUpdate).toBeDefined()
+ const vals = callUpdate!.values as Record<string, unknown>
+ expect(vals.recording_url).toBe('https://storage.supabase.co/recordings/t1/c1.mp3')
+ expect(vals.recording_s3_key).toBeDefined()
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/archive-recording.test.ts` around lines 191 - 208, The test for
handleRecordingSaved should assert the permanent URL behavior in addition to
storage.from: ensure the mock storage's upload method (from the makeStorageMock
return) was called with the expected bucket/key and file data, assert that the
admin client (from makeAdmin / createAdminClientMock) received an update to the
call record using the signed/permanent URL (not the original Telnyx URL) and
that the call record's recording_s3_key field was set; locate and update the
test around handleRecordingSaved(BASE_PAYLOAD), storage (makeStorageMock), and
the admin mock (makeAdmin / createAdminClientMock) to add these assertions.
Comment on lines
+291
to
+311
| it('handles .ogg URL without content-type header', async () => { | ||
| const storage = makeStorageMock() | ||
| const admin = makeAdmin({ callRecord: BASE_CALL_RECORD, storage }) | ||
| createAdminClientMock.mockReturnValue(admin) | ||
|
|
||
| fetchMock.mockResolvedValue({ | ||
| ok: true, | ||
| headers: { get: () => null }, // no content-type | ||
| arrayBuffer: vi.fn().mockResolvedValue(new ArrayBuffer(50)), | ||
| }) | ||
|
|
||
| const p = handleRecordingSaved({ | ||
| ...BASE_PAYLOAD, | ||
| recording_urls: { mp3: 'https://cdn.telnyx.com/recordings/test.ogg' }, | ||
| }) | ||
| await vi.runAllTimersAsync() | ||
| await p | ||
|
|
||
| // Should complete without error - storage.from was called (archival attempted) | ||
| expect(storage.from).toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test doesn't verify the .ogg extension was actually used.
The test confirms archival was attempted but doesn't verify the file was uploaded with the .ogg extension (which is the purpose of testing "handles .ogg URL without content-type header"). Similar to the .wav test above it (lines 270-289), this should assert the upload path contains .ogg.
♻️ Suggested assertion for .ogg extension
await vi.runAllTimersAsync()
await p
- // Should complete without error - storage.from was called (archival attempted)
- expect(storage.from).toHaveBeenCalled()
+ // Should detect .ogg from URL when content-type is missing
+ expect(storage.from).toHaveBeenCalledWith('recordings')
+ const uploadMock = storage.from.mock.results[0].value.upload
+ expect(uploadMock).toHaveBeenCalled()
+ const [path] = uploadMock.mock.calls[0]
+ expect(path).toContain('.ogg')
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/archive-recording.test.ts` around lines 291 - 311, The test
"handles .ogg URL without content-type header" currently only checks
storage.from was called; update it to assert the uploaded filename/path includes
the .ogg extension. Locate the mock/storage invocation (storage.from) used in
the test and add an assertion that inspects the mocked call arguments (the
path/filename argument passed to storage.from or the uploaded object key) and
verifies it contains ".ogg", mirroring the assertion pattern used in the `.wav`
test; keep the rest of the test unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/__tests__/stripe-retry-lib.test.ts (1)
1-2:⚠️ Potential issue | 🟡 MinorAvoid
Stripeconstructor identity drift aftervi.resetModules().
Stripeis imported at Line [2], butvi.resetModules()runs at Line [6] and the module under test is re-imported dynamically. That can makeStripeConnectionErrorinstances use a different constructor than the one checked viainstanceof, making these tests brittle.Proposed fix
import { afterEach, describe, it, expect, vi, beforeEach } from 'vitest' -import Stripe from 'stripe' describe('stripe-retry lib', () => { @@ describe('isStripeConnectionError', () => { it('returns true for StripeConnectionError', async () => { const { isStripeConnectionError } = await import('@/lib/stripe-retry') + const { default: Stripe } = await import('stripe') const err = new Stripe.errors.StripeConnectionError({ message: 'timeout', type: 'connection_error' as any }) expect(isStripeConnectionError(err)).toBe(true) }) @@ it('rethrows StripeConnectionError immediately without retry', async () => { const { withStripeRetry } = await import('@/lib/stripe-retry') + const { default: Stripe } = await import('stripe') const err = new Stripe.errors.StripeConnectionError({ message: 'timeout', type: 'connection_error' as any }) const fn = vi.fn().mockRejectedValue(err) await expect(withStripeRetry(fn, 3)).rejects.toBe(err)#!/bin/bash # Read-only verification of constructor-identity risk pattern. rg -n -C2 "import Stripe from 'stripe'|vi\\.resetModules\\(|StripeConnectionError\\(" src/__tests__/stripe-retry-lib.test.ts rg -n -C2 "isStripeConnectionError|instanceof\\s+Stripe\\.errors\\.StripeConnectionError" src/lib/stripe-retry.tsAlso applies to: 5-6, 15-18, 42-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/stripe-retry-lib.test.ts` around lines 1 - 2, The test imports Stripe at the top-level which creates a different constructor identity than the dynamically re-imported module after vi.resetModules(), causing StripeConnectionError instanceof checks to fail; fix by removing the top-level "import Stripe from 'stripe'" and instead obtain the Stripe constructor after calling vi.resetModules() (e.g., dynamic import of 'stripe' or reading Stripe from the freshly required module) so that instances and instanceof checks (Stripe.errors.StripeConnectionError and checks inside isStripeConnectionError / StripeConnectionError usage in stripe-retry-lib.test.ts and src/lib/stripe-retry.ts) share the same constructor identity.src/__tests__/archive-recording.test.ts (3)
210-225:⚠️ Potential issue | 🟡 MinorAdd fallback payload assertions for non-OK fetch path.
Line [210] claims fallback behavior, but this test only checks
call_update_failureswas not inserted. It should also assert thecallsupdate kept the original Telnyx URL and did not set archival key.Suggested assertion additions
const failureInserts = admin.inserts.filter((i) => i.table === 'call_update_failures') expect(failureInserts).toHaveLength(0) + + const callUpdate = admin.updates.find((u) => u.table === 'calls') + expect(callUpdate).toBeDefined() + const vals = callUpdate!.values as Record<string, unknown> + expect(vals.recording_url).toBe(BASE_PAYLOAD.recording_urls.mp3) + expect(vals.recording_s3_key).toBeUndefined()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/archive-recording.test.ts` around lines 210 - 225, The test for handleRecordingSaved currently only checks that no call_update_failures were inserted when fetch returns non-ok; also assert the calls update left the original Telnyx URL and did not set an archival key. Locate the 'calls' insert/update in admin.inserts (filter by i.table === 'calls' or similar), then assert its payload still contains BASE_CALL_RECORD.telnyx_recording_url (or the original URL string) and that archival_key is absent or null/unchanged relative to BASE_CALL_RECORD; add these assertions inside the same it('falls back to original Telnyx URL when fetch returns non-ok', ...) test after awaiting p.
291-311:⚠️ Potential issue | 🟡 MinorThe
.oggtest still doesn’t assert.oggkey generation.Line [291] is named for
.ogghandling, but it only checksstorage.frominvocation. Assert the upload path contains.ogg, same as the.wavcase.Suggested assertion additions
- // Should complete without error - storage.from was called (archival attempted) - expect(storage.from).toHaveBeenCalled() + expect(storage.from).toHaveBeenCalledWith('recordings') + const uploadMock = storage.from.mock.results[0].value.upload + expect(uploadMock).toHaveBeenCalled() + const [path] = uploadMock.mock.calls[0] + expect(path).toContain('.ogg')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/archive-recording.test.ts` around lines 291 - 311, The test for ".ogg URL without content-type header" (the it block invoking handleRecordingSaved with recording_urls.mp3 set to a .ogg URL) currently only asserts storage.from was called; update the test to also assert that the upload path/key used when calling storage.from contains ".ogg" so the code generates an .ogg object key like the .wav case. Locate the test case using the it(...) name and the mock storage object (makeStorageMock / storage.from) and add an expectation that the argument passed into storage.from (or the resulting upload path) includes the ".ogg" extension to verify correct key generation.
191-208:⚠️ Potential issue | 🟡 MinorStrengthen permanent-URL assertions in the success-path test.
Line [191] says this validates “uses permanent URL,” but the test currently only checks
storage.from('recordings'). It still needs to verify upload execution and that thecallsupdate persisted the signed URL withrecording_s3_key.Suggested assertion additions
const p = handleRecordingSaved(BASE_PAYLOAD) await vi.runAllTimersAsync() await p - // Should have called storage upload expect(storage.from).toHaveBeenCalledWith('recordings') + const uploadMock = storage.from.mock.results[0].value.upload + expect(uploadMock).toHaveBeenCalled() + + const callUpdate = admin.updates.find((u) => u.table === 'calls') + expect(callUpdate).toBeDefined() + const vals = callUpdate!.values as Record<string, unknown> + expect(vals.recording_url).toBe('https://storage.supabase.co/recordings/t1/c1.mp3') + expect(vals.recording_s3_key).toBeDefined()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/archive-recording.test.ts` around lines 191 - 208, The test should also assert that the storage upload actually ran and that the call record was updated to persist the permanent URL under recording_s3_key: after calling handleRecordingSaved (use the existing BASE_PAYLOAD and makeStorageMock/admin), add assertions that storage.upload was called (e.g., spy on storage.upload and assert it received the ArrayBuffer and expected filename/key) and that the admin call update (the method on the admin mock that persists call changes, e.g., calls.update or updateCall on the admin object) was called with the call id and a payload containing recording_s3_key set to the expected signed/permanent URL; use the existing storage and admin mocks (storage.upload, storage.from, and the admin.calls update method) to locate the right spies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/archive-recording.test.ts`:
- Around line 409-508: Tests incorrectly use isRateLimited (a read-only checker)
to "fill" buckets; change those setups to call rateLimit(...) to actually
increment/seed the bucket for keys used in tests (replace calls at the cases
referencing isRateLimited used as setup: tests around the bucket-full,
peek-full, refund-, clear- flows), and update the expired retry-after assertion
to expect 0 instead of 1 for getRateLimitRetryAfterSecs when an entry has
expired; reference functions: rateLimit, isRateLimited, peekRateLimit,
getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit.
---
Duplicate comments:
In `@src/__tests__/archive-recording.test.ts`:
- Around line 210-225: The test for handleRecordingSaved currently only checks
that no call_update_failures were inserted when fetch returns non-ok; also
assert the calls update left the original Telnyx URL and did not set an archival
key. Locate the 'calls' insert/update in admin.inserts (filter by i.table ===
'calls' or similar), then assert its payload still contains
BASE_CALL_RECORD.telnyx_recording_url (or the original URL string) and that
archival_key is absent or null/unchanged relative to BASE_CALL_RECORD; add these
assertions inside the same it('falls back to original Telnyx URL when fetch
returns non-ok', ...) test after awaiting p.
- Around line 291-311: The test for ".ogg URL without content-type header" (the
it block invoking handleRecordingSaved with recording_urls.mp3 set to a .ogg
URL) currently only asserts storage.from was called; update the test to also
assert that the upload path/key used when calling storage.from contains ".ogg"
so the code generates an .ogg object key like the .wav case. Locate the test
case using the it(...) name and the mock storage object (makeStorageMock /
storage.from) and add an expectation that the argument passed into storage.from
(or the resulting upload path) includes the ".ogg" extension to verify correct
key generation.
- Around line 191-208: The test should also assert that the storage upload
actually ran and that the call record was updated to persist the permanent URL
under recording_s3_key: after calling handleRecordingSaved (use the existing
BASE_PAYLOAD and makeStorageMock/admin), add assertions that storage.upload was
called (e.g., spy on storage.upload and assert it received the ArrayBuffer and
expected filename/key) and that the admin call update (the method on the admin
mock that persists call changes, e.g., calls.update or updateCall on the admin
object) was called with the call id and a payload containing recording_s3_key
set to the expected signed/permanent URL; use the existing storage and admin
mocks (storage.upload, storage.from, and the admin.calls update method) to
locate the right spies.
In `@src/__tests__/stripe-retry-lib.test.ts`:
- Around line 1-2: The test imports Stripe at the top-level which creates a
different constructor identity than the dynamically re-imported module after
vi.resetModules(), causing StripeConnectionError instanceof checks to fail; fix
by removing the top-level "import Stripe from 'stripe'" and instead obtain the
Stripe constructor after calling vi.resetModules() (e.g., dynamic import of
'stripe' or reading Stripe from the freshly required module) so that instances
and instanceof checks (Stripe.errors.StripeConnectionError and checks inside
isStripeConnectionError / StripeConnectionError usage in
stripe-retry-lib.test.ts and src/lib/stripe-retry.ts) share the same constructor
identity.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f269f38e-144d-4b34-a890-6e91d8b5b679
📒 Files selected for processing (3)
src/__tests__/archive-recording.test.tssrc/__tests__/health-checks-lib.test.tssrc/__tests__/stripe-retry-lib.test.ts
Comment on lines
+409
to
+508
| it('isRateLimited returns limited:true when bucket is full', async () => { | ||
| const { isRateLimited } = await import('@/lib/rate-limit') | ||
| const key = `isrl-test-${Date.now()}` | ||
| isRateLimited(key, 2, 60_000) | ||
| isRateLimited(key, 2, 60_000) | ||
| const result = isRateLimited(key, 2, 60_000) | ||
| expect(result.limited).toBe(true) | ||
| expect(typeof result.resetAt).toBe('number') | ||
| }) | ||
|
|
||
| it('isRateLimited allows when expired', async () => { | ||
| vi.useFakeTimers() | ||
| const { isRateLimited } = await import('@/lib/rate-limit') | ||
| const key = `isrl-expired-${Date.now()}` | ||
| isRateLimited(key, 1, 100) | ||
| vi.advanceTimersByTime(200) | ||
| const result = isRateLimited(key, 1, 100) | ||
| expect(result.limited).toBe(false) | ||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it('peekRateLimit returns false when no entry', async () => { | ||
| const { peekRateLimit } = await import('@/lib/rate-limit') | ||
| expect(peekRateLimit(`peek-nokey-${Date.now()}`, 5)).toBe(false) | ||
| }) | ||
|
|
||
| it('peekRateLimit returns true when limit exceeded', async () => { | ||
| const { isRateLimited, peekRateLimit } = await import('@/lib/rate-limit') | ||
| const key = `peek-full-${Date.now()}` | ||
| isRateLimited(key, 1, 60_000) | ||
| isRateLimited(key, 1, 60_000) // hits limit | ||
| expect(peekRateLimit(key, 1)).toBe(true) | ||
| }) | ||
|
|
||
| it('peekRateLimit returns false for expired entry', async () => { | ||
| vi.useFakeTimers() | ||
| const { isRateLimited, peekRateLimit } = await import('@/lib/rate-limit') | ||
| const key = `peek-expired-${Date.now()}` | ||
| isRateLimited(key, 1, 100) | ||
| vi.advanceTimersByTime(200) | ||
| expect(peekRateLimit(key, 1)).toBe(false) | ||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it('getRateLimitRetryAfterSecs returns fallback when no entry', async () => { | ||
| const { getRateLimitRetryAfterSecs } = await import('@/lib/rate-limit') | ||
| const secs = getRateLimitRetryAfterSecs(`no-entry-${Date.now()}`, 60_000) | ||
| expect(secs).toBe(60) | ||
| }) | ||
|
|
||
| it('getRateLimitRetryAfterSecs returns 1 for expired entry', async () => { | ||
| vi.useFakeTimers() | ||
| const { isRateLimited, getRateLimitRetryAfterSecs } = await import('@/lib/rate-limit') | ||
| const key = `retry-after-expired-${Date.now()}` | ||
| isRateLimited(key, 1, 100) | ||
| vi.advanceTimersByTime(200) | ||
| expect(getRateLimitRetryAfterSecs(key, 60_000)).toBe(1) | ||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it('refundRateLimit decrements count', async () => { | ||
| const { isRateLimited, refundRateLimit, peekRateLimit } = await import('@/lib/rate-limit') | ||
| const key = `refund-${Date.now()}` | ||
| const { resetAt } = isRateLimited(key, 3, 60_000) | ||
| isRateLimited(key, 3, 60_000) | ||
| isRateLimited(key, 3, 60_000) // count=3 — at limit | ||
| expect(peekRateLimit(key, 3)).toBe(true) | ||
| refundRateLimit(key, resetAt) | ||
| expect(peekRateLimit(key, 3)).toBe(false) | ||
| }) | ||
|
|
||
| it('refundRateLimit no-ops when window rolled over', async () => { | ||
| vi.useFakeTimers() | ||
| const { isRateLimited, refundRateLimit } = await import('@/lib/rate-limit') | ||
| const key = `refund-expired-${Date.now()}` | ||
| const { resetAt } = isRateLimited(key, 1, 100) | ||
| vi.advanceTimersByTime(200) | ||
| // Should not throw | ||
| refundRateLimit(key, resetAt) | ||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it('refundRateLimit skips when expectedResetAt does not match', async () => { | ||
| const { isRateLimited, refundRateLimit, peekRateLimit } = await import('@/lib/rate-limit') | ||
| const key = `refund-mismatch-${Date.now()}` | ||
| isRateLimited(key, 2, 60_000) | ||
| isRateLimited(key, 2, 60_000) // count=2 — at limit | ||
| refundRateLimit(key, 999999) // wrong resetAt | ||
| expect(peekRateLimit(key, 2)).toBe(true) // still limited | ||
| }) | ||
|
|
||
| it('clearRateLimit removes entry', async () => { | ||
| const { isRateLimited, clearRateLimit, peekRateLimit } = await import('@/lib/rate-limit') | ||
| const key = `clear-${Date.now()}` | ||
| isRateLimited(key, 1, 60_000) | ||
| isRateLimited(key, 1, 60_000) // at limit | ||
| expect(peekRateLimit(key, 1)).toBe(true) | ||
| clearRateLimit(key) | ||
| expect(peekRateLimit(key, 1)).toBe(false) | ||
| }) |
There was a problem hiding this comment.
Rate-limit tests are using a read-only API as setup, making several cases vacuous/incorrect.
isRateLimited does not create/increment buckets (see src/lib/rate-limit.ts, function body and docstring). In this block, many tests use it to “fill” buckets (Line [409], Line [435], Line [469], Line [491], Line [500]), so expected outcomes are not validating intended behavior. Also, Line [465] expects 1, but for an existing expired entry getRateLimitRetryAfterSecs returns 0.
Suggested fix pattern (seed with rateLimit)
- const { isRateLimited } = await import('@/lib/rate-limit')
+ const { isRateLimited, rateLimit } = await import('@/lib/rate-limit')
const key = `isrl-test-${Date.now()}`
- isRateLimited(key, 2, 60_000)
- isRateLimited(key, 2, 60_000)
+ rateLimit(key, 2, 60_000)
+ rateLimit(key, 2, 60_000)
const result = isRateLimited(key, 2, 60_000)
expect(result.limited).toBe(true)
- const { isRateLimited, peekRateLimit } = await import('@/lib/rate-limit')
+ const { rateLimit, peekRateLimit } = await import('@/lib/rate-limit')
const key = `peek-full-${Date.now()}`
- isRateLimited(key, 1, 60_000)
- isRateLimited(key, 1, 60_000) // hits limit
+ rateLimit(key, 1, 60_000)
+ rateLimit(key, 1, 60_000) // now over limit window usage
expect(peekRateLimit(key, 1)).toBe(true)
- const { isRateLimited, getRateLimitRetryAfterSecs } = await import('@/lib/rate-limit')
+ const { rateLimit, getRateLimitRetryAfterSecs } = await import('@/lib/rate-limit')
const key = `retry-after-expired-${Date.now()}`
- isRateLimited(key, 1, 100)
+ rateLimit(key, 1, 100)
vi.advanceTimersByTime(200)
- expect(getRateLimitRetryAfterSecs(key, 60_000)).toBe(1)
+ expect(getRateLimitRetryAfterSecs(key, 60_000)).toBe(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/archive-recording.test.ts` around lines 409 - 508, Tests
incorrectly use isRateLimited (a read-only checker) to "fill" buckets; change
those setups to call rateLimit(...) to actually increment/seed the bucket for
keys used in tests (replace calls at the cases referencing isRateLimited used as
setup: tests around the bucket-full, peek-full, refund-, clear- flows), and
update the expired retry-after assertion to expect 0 instead of 1 for
getRateLimitRetryAfterSecs when an entry has expired; reference functions:
rateLimit, isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs,
refundRateLimit, clearRateLimit.
coygg
dismissed
coderabbitai[bot]’s stale review
All findings addressed. Tests strengthened with explicit assertions.
coygg
added a commit
that referenced
this pull request
Apr 2, 2026
* test: full audit coverage — archiveRecordingToStorage, health-checks, stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing * fix: address CodeRabbit test quality findings on PR #336 * fix: strengthen test assertions per CodeRabbit review on PR #336 * fix: add missing assertions in archive-recording tests * Harden agent presence heartbeat and surface offline state * fix: address CodeRabbit findings on PR #337 * fix presence stale-check error handling and codrabbit test findings * fix: final test assertion hardening per CodeRabbit
coygg
added a commit
that referenced
this pull request
Apr 2, 2026
* test: full audit coverage — archiveRecordingToStorage, health-checks, stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing * fix: address CodeRabbit test quality findings on PR #336 * fix: strengthen test assertions per CodeRabbit review on PR #336 * fix: add missing assertions in archive-recording tests * Harden agent presence heartbeat and surface offline state * fix: address CodeRabbit findings on PR #337 * fix presence stale-check error handling and codrabbit test findings * fix: final test assertion hardening per CodeRabbit * Fix ghost voicemail loops and add hangup safeguards * fix: tighten presence restore and badge offline guard * fix: address CodeRabbit presence and voicemail reliability findings
coygg
added a commit
that referenced
this pull request
Apr 2, 2026
* test: full audit coverage — archiveRecordingToStorage, health-checks, stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing * fix: address CodeRabbit test quality findings on PR #336 * fix: strengthen test assertions per CodeRabbit review on PR #336 * fix: add missing assertions in archive-recording tests * Harden agent presence heartbeat and surface offline state * fix: address CodeRabbit findings on PR #337 * fix presence stale-check error handling and codrabbit test findings * fix: final test assertion hardening per CodeRabbit * Fix ghost voicemail loops and add hangup safeguards * fix: resolve CodeRabbit findings for presence and voicemail flow
coygg
added a commit
that referenced
this pull request
Jun 1, 2026
…#336) * test: full audit coverage — archiveRecordingToStorage, health-checks, stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing * fix: address CodeRabbit test quality findings on PR #336 * fix: strengthen test assertions per CodeRabbit review on PR #336 * fix: add missing assertions in archive-recording tests
coygg
added a commit
that referenced
this pull request
Jun 1, 2026
* test: full audit coverage — archiveRecordingToStorage, health-checks, stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing * fix: address CodeRabbit test quality findings on PR #336 * fix: strengthen test assertions per CodeRabbit review on PR #336 * fix: add missing assertions in archive-recording tests * Harden agent presence heartbeat and surface offline state * fix: address CodeRabbit findings on PR #337 * fix presence stale-check error handling and codrabbit test findings * fix: final test assertion hardening per CodeRabbit
coygg
added a commit
that referenced
this pull request
Jun 1, 2026
* test: full audit coverage — archiveRecordingToStorage, health-checks, stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing * fix: address CodeRabbit test quality findings on PR #336 * fix: strengthen test assertions per CodeRabbit review on PR #336 * fix: add missing assertions in archive-recording tests * Harden agent presence heartbeat and surface offline state * fix: address CodeRabbit findings on PR #337 * fix presence stale-check error handling and codrabbit test findings * fix: final test assertion hardening per CodeRabbit * Fix ghost voicemail loops and add hangup safeguards * fix: tighten presence restore and badge offline guard * fix: address CodeRabbit presence and voicemail reliability findings
coygg
added a commit
that referenced
this pull request
Jun 1, 2026
* test: full audit coverage — archiveRecordingToStorage, health-checks, stripe-retry, rate-limit - archiveRecordingToStorage: 21 tests covering happy path, extension detection, upload failure, signedUrl failure, call_update_failures insert path, failure insert error logging, and missing callRecord skip - health-checks.ts: 23 tests (0% -> 100%) — checkTelnyxCCA, checkTelnyxCredentialConnection, checkSupabaseReachability, EXPECTED_WEBHOOK_URL derivation, all error/timeout paths - stripe-retry.ts: 11 tests (41% -> 100%) — isStripeConnectionError, withStripeRetry happy path, connection error passthrough, 429 retry with retry-after cap, status fallback - rate-limit.ts: 12 tests added (64% -> 93%) — isRateLimited, peekRateLimit, getRateLimitRetryAfterSecs, refundRateLimit, clearRateLimit Total: 975 -> 1030 tests (+55), all passing * fix: address CodeRabbit test quality findings on PR #336 * fix: strengthen test assertions per CodeRabbit review on PR #336 * fix: add missing assertions in archive-recording tests * Harden agent presence heartbeat and surface offline state * fix: address CodeRabbit findings on PR #337 * fix presence stale-check error handling and codrabbit test findings * fix: final test assertion hardening per CodeRabbit * Fix ghost voicemail loops and add hangup safeguards * fix: resolve CodeRabbit findings for presence and voicemail flow
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full Audit - review/full-audit-0326
Operational Verification
px tsc --noEmit clean
Coverage Before vs After
|
ate-limit.ts | 64.64% | 93.93% |
| handlers.ts | 73.28% | 74.51% |
New Tests (55 added)
�rchive-recording.test.ts (21 tests)
health-checks-lib.test.ts (23 tests)
stripe-retry-lib.test.ts (11 tests)
�rchive-recording.test.ts rate-limit section (15 tests added to rate-limit coverage)
efundRateLimit, clearRateLimit
Total: 975 ? 1030 tests (all passing)
Summary by CodeRabbit