Merged
Conversation
|
Cursor Agent can help with this pull request. Just |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes legacy base64 audio storage and fallback handling from the Call workflow, requires calls to reference audio via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 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 |
cursor
bot
force-pushed
the
cursor/r2-base64-compatibility-removal-b474
branch
from
cd7a125 to
3b77026
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prisma/migrations/20260224171027_drop-call-base64/migration.sql (1)
17-18: Confirm R2 backfill is complete before running this migration.The
INSERTcopiesaudioKey(which may beNULL) but discardsbase64. AnyCallrows whereaudioKey IS NULLand abase64value existed will have their audio irrecoverably dropped — there is no rollback path once the old table is gone. The PR asserts the backfill is done, but the migration itself has no guard.Before deploying, consider running a preflight query against production:
SELECT COUNT(*) FROM "Call" WHERE "audioKey" IS NULL;If the count is non-zero, those records will serve 404s after migration. Acceptable if intentional; worth a deliberate acknowledgment before the column is dropped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prisma/migrations/20260224171027_drop-call-base64/migration.sql` around lines 17 - 18, The migration drops the old "Call" table but the INSERT into "new_Call" copies "audioKey" and discards "base64", which will irreversibly lose audio for rows where "audioKey" IS NULL; update the migration.sql to guard or preserve that data: either add a preflight check that aborts the migration (e.g., SELECT COUNT(*) FROM "Call" WHERE "audioKey" IS NULL and RAISE/FAIL if >0) so engineers must confirm backfill is complete, or modify the INSERT/CREATE so the "base64" column is preserved/copied into an appropriate column on "new_Call" (or into a temporary column) before dropping "Call" (referencing "Call", "new_Call", "audioKey", and "base64"); ensure the migration fails fast if the preflight check detects any rows with audioKey IS NULL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@prisma/migrations/20260224171027_drop-call-base64/migration.sql`:
- Around line 17-18: The migration drops the old "Call" table but the INSERT
into "new_Call" copies "audioKey" and discards "base64", which will irreversibly
lose audio for rows where "audioKey" IS NULL; update the migration.sql to guard
or preserve that data: either add a preflight check that aborts the migration
(e.g., SELECT COUNT(*) FROM "Call" WHERE "audioKey" IS NULL and RAISE/FAIL if
>0) so engineers must confirm backfill is complete, or modify the INSERT/CREATE
so the "base64" column is preserved/copied into an appropriate column on
"new_Call" (or into a temporary column) before dropping "Call" (referencing
"Call", "new_Call", "audioKey", and "base64"); ensure the migration fails fast
if the preflight check detects any rows with audioKey IS NULL.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/routes/resources/calls/call-audio.tsapp/routes/resources/calls/save.tsxapp/utils/call-kent-episode-draft.server.tsapp/utils/simplecast.server.tsprisma/migrations/20260224171027_drop-call-base64/migration.sqlprisma/schema.prismaprisma/seed.ts
💤 Files with no reviewable changes (1)
- app/routes/resources/calls/save.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/routes/resources/calls/call-audio.ts
- prisma/seed.ts
- app/utils/call-kent-episode-draft.server.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/semantic-search-test-cache.ts`:
- Around line 3-16: The module-global Map "memory" can leak state between tests;
add and export a reset helper (e.g., resetTestCache or clearMemory) that calls
memory.clear(), update the exported API around testCache to include that
function, and use it from test setup/teardown to ensure memory is cleared
before/after each test run; reference the existing "memory" and "testCache"
symbols when implementing the helper.
tests/semantic-search-test-cache.ts
Outdated
Comment on lines
+3
to
+16
| export const memory = new Map<string, unknown>() | ||
|
|
||
| export const testCache = { | ||
| name: 'test-cache', | ||
| get(key: string) { | ||
| return (memory.get(key) as any) ?? null | ||
| }, | ||
| async set(key: string, entry: unknown) { | ||
| memory.set(key, entry) | ||
| }, | ||
| async delete(key: string) { | ||
| memory.delete(key) | ||
| }, | ||
| } |
There was a problem hiding this comment.
Prevent cross-test cache state leakage.
memory is module‑global; without a reset, cached entries can bleed across tests and make outcomes order‑dependent. Consider exporting a reset helper (and using it in test setup).
♻️ Suggested helper
export const memory = new Map<string, unknown>()
+
+export function resetTestCache() {
+ memory.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
| export const memory = new Map<string, unknown>() | |
| export const testCache = { | |
| name: 'test-cache', | |
| get(key: string) { | |
| return (memory.get(key) as any) ?? null | |
| }, | |
| async set(key: string, entry: unknown) { | |
| memory.set(key, entry) | |
| }, | |
| async delete(key: string) { | |
| memory.delete(key) | |
| }, | |
| } | |
| export const memory = new Map<string, unknown>() | |
| export function resetTestCache() { | |
| memory.clear() | |
| } | |
| export const testCache = { | |
| name: 'test-cache', | |
| get(key: string) { | |
| return (memory.get(key) as any) ?? null | |
| }, | |
| async set(key: string, entry: unknown) { | |
| memory.set(key, entry) | |
| }, | |
| async delete(key: string) { | |
| memory.delete(key) | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/semantic-search-test-cache.ts` around lines 3 - 16, The module-global
Map "memory" can leak state between tests; add and export a reset helper (e.g.,
resetTestCache or clearMemory) that calls memory.clear(), update the exported
API around testCache to include that function, and use it from test
setup/teardown to ensure memory is cleared before/after each test run; reference
the existing "memory" and "testCache" symbols when implementing the helper.
ef2d2e9 to
79f9bfd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/routes/resources/calls/call-audio.ts (1)
36-57: Buffer fallback for missingaudioSizeis reasonable.This path loads the full audio into memory, which is appropriate as a rare safety net. One note: the comment on line 37 correctly flags that this "should" not happen for well-formed data — consider logging a warning here so you can detect if rows are missing
audioSizein production and fix them proactively.🔧 Optional: add observability for the fallback path
if (typeof size !== 'number' || !Number.isFinite(size) || size <= 0) { // Size should always be present for stored audio; keep a safe fallback. + console.warn(`call-audio: audioSize missing/invalid for key=${audioKey}, buffering full object`) const buffer = await getAudioBuffer({ key: audioKey })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/resources/calls/call-audio.ts` around lines 36 - 57, In the fallback block that handles missing/invalid size (the code around getAudioBuffer({ key: audioKey }) and parseHttpByteRangeHeader(rangeHeader, size)), add a warning log immediately after obtaining buffer.byteLength that records the audioKey and resolved size to surface missing audioSize rows in production; use the module's existing logger (e.g., processLogger.warn) if available or fall back to console.warn, and keep the message concise like "Missing audioSize for audioKey=<...>, using resolved size=<...>" so operators can triage and fix the data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/routes/resources/calls/call-audio.ts`:
- Around line 36-57: In the fallback block that handles missing/invalid size
(the code around getAudioBuffer({ key: audioKey }) and
parseHttpByteRangeHeader(rangeHeader, size)), add a warning log immediately
after obtaining buffer.byteLength that records the audioKey and resolved size to
surface missing audioSize rows in production; use the module's existing logger
(e.g., processLogger.warn) if available or fall back to console.warn, and keep
the message concise like "Missing audioSize for audioKey=<...>, using resolved
size=<...>" so operators can triage and fix the data.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/routes/resources/calls/call-audio.tsapp/routes/resources/calls/save.tsxapp/utils/__tests__/semantic-search-cache.server.test.tsapp/utils/__tests__/semantic-search.server.test.tsapp/utils/call-kent-episode-draft.server.tsprisma/migrations/20260224171027_drop-call-base64/migration.sqlprisma/schema.prismaprisma/seed.tstests/semantic-search-test-cache.ts
💤 Files with no reviewable changes (2)
- app/routes/resources/calls/save.tsx
- prisma/seed.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/semantic-search-test-cache.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/routes/resources/calls/call-audio.ts (1)
35-59: Consider avoiding full-buffer fallback if this can be frequent.
Loading the entire audio into memory to infer size can spike memory on large files; consider a storage metadata/HEAD size lookup or a capped/short-circuit path ifaudioSizeis missing often.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/resources/calls/call-audio.ts` around lines 35 - 59, The code currently falls back to loading the entire blob via getAudioBuffer to compute size when size is invalid, which can spike memory; change the fallback in the block that handles typeof size !== 'number' (around variables size, audioKey, callId, rangeHeader) to first attempt a metadata/HEAD size lookup from the storage layer (e.g., a new getAudioMetadata or headObject call) to set size without downloading the body, only if that fails then use a capped fallback: either stream the file without buffering (use the storage streaming API to pipe into Readable without buffering full content) or return a short-circuit response indicating missing size (e.g., 416 or 400) for very large files. Ensure parseHttpByteRangeHeader(rangeHeader, size), Readable.from / createReadableStreamFromReadable usage and the response header construction (Content-Range/Content-Length) still operate on the obtained size/stream semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/routes/resources/calls/call-audio.ts`:
- Around line 35-59: The code currently falls back to loading the entire blob
via getAudioBuffer to compute size when size is invalid, which can spike memory;
change the fallback in the block that handles typeof size !== 'number' (around
variables size, audioKey, callId, rangeHeader) to first attempt a metadata/HEAD
size lookup from the storage layer (e.g., a new getAudioMetadata or headObject
call) to set size without downloading the body, only if that fails then use a
capped fallback: either stream the file without buffering (use the storage
streaming API to pipe into Readable without buffering full content) or return a
short-circuit response indicating missing size (e.g., 416 or 400) for very large
files. Ensure parseHttpByteRangeHeader(rangeHeader, size), Readable.from /
createReadableStreamFromReadable usage and the response header construction
(Content-Range/Content-Length) still operate on the obtained size/stream
semantics.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/r2-base64-compatibility-removal-b474
branch
from
409e521 to
1782afb
Compare
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.
Remove pre-R2
Call.base64compatibility after R2 backfill.Note
Medium Risk
Includes a DB schema migration that permanently drops
Call.base64and removes runtime fallback behavior, so any incomplete backfill or overlooked dependencies would break access to legacy call audio.Overview
Call Kent caller audio is now strictly key/metadata-based:
Call.base64is removed from the Prisma schema and a migration drops the column with a preflight guard that aborts if any rows still havebase64but noaudioKey.Audio serving (
call-audioloader) no longer falls back to DB base64; it 404s whenaudioKeyis missing and, whenaudioSizeis absent/invalid, resolves size (and optionally content-type) via newheadAudioObject()before falling back to buffering.Storage utilities add
HEADsupport viaHeadObjectCommand, expand mocks to handleHEAD, add a test for HEAD lookups, and remove legacy base64 migration logic from episode draft processing; seeding also drops the base64 call fixture.Written by Cursor Bugbot for commit 1782afb. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests