Fix podcast semantic indexing rate-limit handling#809
Merged
Conversation
📝 WalkthroughWalkthroughRemoves legacy Cloudflare Vectorize v1 fallback logic, migrating write and delete operations to v2-only endpoints with updated mocks and tests. Hardens Simplecast episode fetching with configurable retry/concurrency/spacing, strict 429 error propagation, and utility helpers for environment-based configuration and async delay handling. ChangesCloudflare Vectorize v2-only endpoint migration
Simplecast episode fetching robustness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
other/semantic-search/index-podcasts.ts (1)
238-238:⚠️ Potential issue | 🔴 CriticalFix
isTooManyRequeststype guard to match actual Simplecast API response shape.The local type
SimplecastTooManyRequestsredefines the type as{ too_many_requests: true }but the actual API contract inservices/site/types/simplecast.d.tsspecifies{ status: 429, href: null, error_message: string, error: string }. TheisTooManyRequestsguard checks for the wrong shape and will fail to detect rate-limited responses matching the declared type. This creates a functional correctness bug: if a 429 response reachesrequireCompleteSimplecastResponse, the type guard won't catch it, allowing invalid data through.Align the local
SimplecastTooManyRequeststype with the declared API type or adjust the guard to check the correct response shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@other/semantic-search/index-podcasts.ts` at line 238, The SimplecastTooManyRequests type is incorrectly defined as { too_many_requests: true } but the actual Simplecast API contract returns { status: 429, href: null, error_message: string, error: string }. Update the SimplecastTooManyRequests type definition to match the actual API response shape, and ensure the isTooManyRequests type guard checks against the correct properties (status field equal to 429) rather than the non-existent too_many_requests property. This will allow the type guard to properly detect rate-limited 429 responses from the API.
🧹 Nitpick comments (3)
services/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.ts (1)
37-50: ⚡ Quick winAdd a regression test for the legacy fallback branch.
Please add a case where the first (v2) call throws a legacy-signature error and assert the second call targets
/vectorize/indexes/.../delete_by_ids. That directly protects the new fallback gate behavior in this PR.Proposed test shape
+test('vectorizeDeleteByIds falls back to legacy endpoint for legacy-signature errors', async () => { + const fetchMock = vi.spyOn(globalThis, 'fetch') + .mockResolvedValueOnce(new Response('not found', { status: 404 })) + .mockResolvedValueOnce( + new Response(JSON.stringify({ success: true, result: { mutationId: 'legacy-m1' } })), + ) + + try { + await vectorizeDeleteByIds(vectorizeConfig) + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(String(fetchMock.mock.calls[1]![0])).toContain( + '/vectorize/indexes/search-index/delete_by_ids', + ) + } finally { + fetchMock.mockRestore() + } +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.ts` around lines 37 - 50, Add a new test case after the existing test `vectorizeDeleteByIds does not mask non-legacy v2 errors` to specifically cover the legacy fallback behavior. This test should mock the fetch function to first throw a legacy-signature error when the v2 endpoint is called, then mock it to succeed on the second call. Assert that when the legacy-signature error is caught, the function retries with a call to the legacy endpoint at `/vectorize/indexes/.../delete_by_ids`. This regression test will directly protect the new fallback gate behavior in the vectorizeDeleteByIds function by ensuring the legacy fallback path is exercised and validated.other/semantic-search/index-podcasts.ts (1)
280-395: 💤 Low valueConsider moving sleep before the early return to ensure consistent pacing.
At lines 377-379, when
!epJson.is_publishedthe function returnsnullimmediately without sleeping. If multiple consecutive episodes have mismatched publish status between the list and detail responses, this could cause burst requests. Given the pre-filtering at line 349-351 this is unlikely in practice, but moving the sleep before the early return would ensure consistent rate limiting regardless of episode state.const epJson = requireCompleteSimplecastResponse( result.json, `simplecast episode ${e.id}`, ) - if (!epJson.is_published) return null await sleep(episodeDetailSpacingMs) + if (!epJson.is_published) return null return epJson🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@other/semantic-search/index-podcasts.ts` around lines 280 - 395, In the fetchSimplecastEpisodes function, within the async callback passed to mapWithConcurrency, the sleep call using episodeDetailSpacingMs is placed after the early return check for unpublished episodes. Move the await sleep(episodeDetailSpacingMs) statement to execute before the if (!epJson.is_published) return null check to ensure consistent rate limiting and pacing regardless of whether an episode is published or not. This prevents potential burst requests when multiple consecutive episodes are unpublished.services/site/tests/__tests__/semantic-search-index-podcasts.test.ts (1)
12-50: 💤 Low valueConsider adding a test for HTTP 200 with
too_many_requestsJSON body.Both tests return HTTP 429 status, which triggers the retry exhaustion path in
fetchJsonWithRetriesbefore reaching JSON parsing. This meansrequireCompleteSimplecastResponseis not exercised. If Simplecast can return HTTP 200 with a{ too_many_requests: true }body (as the guard function suggests), adding a test for that scenario would ensure the guard works correctly.test('fetchSimplecastEpisodes fails when seasons returns too_many_requests in body', async () => { using _ignoredEnv = setEnv({ SIMPLECAST_KEY: 'simplecast-key', CHATS_WITH_KENT_PODCAST_ID: 'podcast-id', SIMPLECAST_MAX_RETRIES: '0', SIMPLECAST_BASE_DELAY_MS: '0', }) const fetchMock = vi .spyOn(globalThis, 'fetch') .mockImplementation(async () => { // HTTP 200 but rate-limited JSON body return jsonResponse({ too_many_requests: true }, { status: 200 }) }) try { await expect(fetchSimplecastEpisodes()).rejects.toThrow( 'simplecast seasons returned Simplecast too_many_requests', ) } finally { fetchMock.mockRestore() } })Also applies to: 52-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/site/tests/__tests__/semantic-search-index-podcasts.test.ts` around lines 12 - 50, Add a new test case called 'fetchSimplecastEpisodes fails when seasons returns too_many_requests in body' to cover the scenario where the Simplecast API returns HTTP 200 status but with { too_many_requests: true } in the JSON body. This test ensures the requireCompleteSimplecastResponse guard function is properly exercised. Set up the test similarly to the existing test by mocking fetch to return jsonResponse({ too_many_requests: true }, { status: 200 }), configure the same environment variables (SIMPLECAST_KEY, CHATS_WITH_KENT_PODCAST_ID, SIMPLECAST_MAX_RETRIES: '0', SIMPLECAST_BASE_DELAY_MS: '0'), and expect fetchSimplecastEpisodes() to reject with an error message containing 'simplecast seasons returned Simplecast too_many_requests'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@other/semantic-search/index-podcasts.ts`:
- Line 238: The SimplecastTooManyRequests type is incorrectly defined as {
too_many_requests: true } but the actual Simplecast API contract returns {
status: 429, href: null, error_message: string, error: string }. Update the
SimplecastTooManyRequests type definition to match the actual API response
shape, and ensure the isTooManyRequests type guard checks against the correct
properties (status field equal to 429) rather than the non-existent
too_many_requests property. This will allow the type guard to properly detect
rate-limited 429 responses from the API.
---
Nitpick comments:
In `@other/semantic-search/index-podcasts.ts`:
- Around line 280-395: In the fetchSimplecastEpisodes function, within the async
callback passed to mapWithConcurrency, the sleep call using
episodeDetailSpacingMs is placed after the early return check for unpublished
episodes. Move the await sleep(episodeDetailSpacingMs) statement to execute
before the if (!epJson.is_published) return null check to ensure consistent rate
limiting and pacing regardless of whether an episode is published or not. This
prevents potential burst requests when multiple consecutive episodes are
unpublished.
In `@services/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.ts`:
- Around line 37-50: Add a new test case after the existing test
`vectorizeDeleteByIds does not mask non-legacy v2 errors` to specifically cover
the legacy fallback behavior. This test should mock the fetch function to first
throw a legacy-signature error when the v2 endpoint is called, then mock it to
succeed on the second call. Assert that when the legacy-signature error is
caught, the function retries with a call to the legacy endpoint at
`/vectorize/indexes/.../delete_by_ids`. This regression test will directly
protect the new fallback gate behavior in the vectorizeDeleteByIds function by
ensuring the legacy fallback path is exercised and validated.
In `@services/site/tests/__tests__/semantic-search-index-podcasts.test.ts`:
- Around line 12-50: Add a new test case called 'fetchSimplecastEpisodes fails
when seasons returns too_many_requests in body' to cover the scenario where the
Simplecast API returns HTTP 200 status but with { too_many_requests: true } in
the JSON body. This test ensures the requireCompleteSimplecastResponse guard
function is properly exercised. Set up the test similarly to the existing test
by mocking fetch to return jsonResponse({ too_many_requests: true }, { status:
200 }), configure the same environment variables (SIMPLECAST_KEY,
CHATS_WITH_KENT_PODCAST_ID, SIMPLECAST_MAX_RETRIES: '0',
SIMPLECAST_BASE_DELAY_MS: '0'), and expect fetchSimplecastEpisodes() to reject
with an error message containing 'simplecast seasons returned Simplecast
too_many_requests'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 378399bb-7b9a-48cd-8ca7-02c2054ca05b
📒 Files selected for processing (4)
other/semantic-search/cloudflare.tsother/semantic-search/index-podcasts.tsservices/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.tsservices/site/tests/__tests__/semantic-search-index-podcasts.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.ts (1)
64-67: ⚡ Quick winAssert the first call hits v2 before fallback.
This test only verifies the second call URL. Please also assert call 1 is the v2 endpoint so regressions that bypass v2 are caught.
Suggested test hardening
try { await vectorizeDeleteByIds(vectorizeConfig) expect(fetchMock).toHaveBeenCalledTimes(2) + expect(String(fetchMock.mock.calls[0]?.[0])).toBe( + 'https://api.cloudflare.com/client/v4/accounts/account-id/vectorize/v2/indexes/search-index/delete_by_ids', + ) expect(String(fetchMock.mock.calls[1]?.[0])).toBe( 'https://api.cloudflare.com/client/v4/accounts/account-id/vectorize/indexes/search-index/delete_by_ids', ) } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.ts` around lines 64 - 67, The test currently only asserts that the second fetch call (fetchMock.mock.calls[1]) hits the fallback endpoint URL. Add an additional assertion before the existing one to verify that the first fetch call (fetchMock.mock.calls[0]) successfully hits the v2 endpoint URL, ensuring that the v2 endpoint is attempted before any fallback occurs and preventing regressions that might bypass v2.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@services/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.ts`:
- Around line 64-67: The test currently only asserts that the second fetch call
(fetchMock.mock.calls[1]) hits the fallback endpoint URL. Add an additional
assertion before the existing one to verify that the first fetch call
(fetchMock.mock.calls[0]) successfully hits the v2 endpoint URL, ensuring that
the v2 endpoint is attempted before any fallback occurs and preventing
regressions that might bypass v2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9edd0d55-328a-4aff-9d9c-05ececf692cc
📒 Files selected for processing (3)
other/semantic-search/index-podcasts.tsservices/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.tsservices/site/tests/__tests__/semantic-search-index-podcasts.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- other/semantic-search/index-podcasts.ts
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
2 participants
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.
Summary
Testing
npm run test:backend --workspace kentcdodds.com -- tests/__tests__/semantic-search-cloudflare-vectorize.test.ts tests/__tests__/semantic-search-index-podcasts.test.ts mocks/__tests__/cloudflare.test.tsnpm run typecheck --workspace kentcdodds.comnpm run lint --workspace kentcdodds.com./node_modules/.bin/oxlint other/semantic-search/cloudflare.ts services/site/app/utils/vectorize.server.ts services/site/mocks/cloudflare.ts services/site/tests/__tests__/semantic-search-cloudflare-vectorize.test.tsrg "isLegacyVectorizeError|deprecated-v1|vectorize/indexes|Vectorize.*legacy|legacy.*Vectorize" --glob '*.{ts,tsx}'CI / review loop
Summary by CodeRabbit
Bug Fixes
Enhancements
Tests