fix: MediaRecorder crash on rapid re-record + add sample sort_order and reorder API#660
Open
neuron-tech-ai wants to merge 2 commits into
Open
fix: MediaRecorder crash on rapid re-record + add sample sort_order and reorder API#660neuron-tech-ai wants to merge 2 commits into
neuron-tech-ai wants to merge 2 commits into
Conversation
…r API for profile samples
Contributor
📝 WalkthroughWalkthroughThis PR introduces sample ordering functionality with a new ChangesSample Ordering and Core Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
… to models.py
The /profiles/{profile_id}/samples/reorder endpoint referenced
models.SampleReorderRequest which was never defined, causing an
AttributeError at import time. ProfileSampleResponse was also missing
the sort_order field added to the DB model, so serialization would fail.
Add both to backend/models.py.
Author
|
Fixed two bugs found during review:
Fixed in commit bf0ad3e. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/routes/profiles.py (1)
53-53: 💤 Low valueFollow Python naming convention for constants.
The variable
CHUNK_SIZEis a module-level constant and correctly uses uppercase. However, the static analysis tool flagged it because it's defined inside a function scope. Consider moving it to module level alongsideMAX_FILE_SIZE(line 52) for consistency.📝 Proposed refactoring
+IMPORT_MAX_FILE_SIZE = 100 * 1024 * 1024 # 100 MB +IMPORT_CHUNK_SIZE = 1024 * 1024 # 1 MB + `@router.post`("/profiles/import", response_model=models.VoiceProfileResponse) async def import_profile( file: UploadFile = File(...), db: Session = Depends(get_db), ): """Import a voice profile from a ZIP archive.""" - MAX_FILE_SIZE = 100 * 1024 * 1024 - CHUNK_SIZE = 1024 * 1024 # 1 MB # Stream-read with an early size cap so oversized uploads are rejected # before the entire payload is buffered into memory. chunks: list[bytes] = [] total = 0 - while chunk := await file.read(CHUNK_SIZE): + while chunk := await file.read(IMPORT_CHUNK_SIZE): total += len(chunk) - if total > MAX_FILE_SIZE: + if total > IMPORT_MAX_FILE_SIZE: raise HTTPException( status_code=413, - detail=f"File too large. Maximum size is {MAX_FILE_SIZE // (1024 * 1024)} MB.", + detail=f"File too large. Maximum size is {IMPORT_MAX_FILE_SIZE // (1024 * 1024)} MB.", ) chunks.append(chunk) content = b"".join(chunks)🤖 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 `@backend/routes/profiles.py` at line 53, CHUNK_SIZE is currently defined inside a function but should be a true module-level constant like MAX_FILE_SIZE; move the CHUNK_SIZE declaration out of the function and place it at module scope alongside MAX_FILE_SIZE, keep the name CHUNK_SIZE and value 1024 * 1024, and update any local references inside the function (if any) to use the module-level CHUNK_SIZE constant.backend/models.py (1)
80-84: 💤 Low valueConsider adding validation to
sample_ids.The
sample_idsfield accepts any list, including an empty list. While the reorder endpoint may handle this gracefully, addingField(..., min_length=1)would make the API contract more explicit and provide better error messages at validation time.📝 Proposed validation enhancement
class SampleReorderRequest(BaseModel): """Request model for reordering profile samples.""" - sample_ids: list[str] + sample_ids: list[str] = Field(..., min_length=1)🤖 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 `@backend/models.py` around lines 80 - 84, Add validation to SampleReorderRequest.sample_ids by declaring it with a Pydantic Field that enforces at least one element; import Field from pydantic if not already present and change the declaration to use Field(..., min_length=1) on the sample_ids attribute of the SampleReorderRequest model so empty lists are rejected with a clear validation error.backend/services/profiles.py (1)
233-251: ⚡ Quick winRemove redundant import.
The code imports
funcas_funcat line 236, butfuncis already imported at line 9. Use the existing import instead.♻️ Proposed fix
# Assign sort_order = max(existing) + 1 so each new sample appends at # the end rather than landing at 0 (which would make all samples tie for # the top position and produce non-deterministic ordering). - from sqlalchemy import func as _func - - max_order = db.query(_func.max(DBProfileSample.sort_order)).filter_by(profile_id=profile_id).scalar() + max_order = db.query(func.max(DBProfileSample.sort_order)).filter_by(profile_id=profile_id).scalar() next_order = (max_order or 0) + 1🤖 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 `@backend/services/profiles.py` around lines 233 - 251, Remove the redundant "from sqlalchemy import func as _func" import and update the max_order computation to use the existing func symbol already imported earlier; specifically, delete the _func import and change the call in the query that references _func.max(DBProfileSample.sort_order) to use func.max(DBProfileSample.sort_order) so DBProfileSample, next_order and profile.updated_at logic remain unchanged.
🤖 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.
Inline comments:
In `@app/src/lib/hooks/useAudioRecording.ts`:
- Line 35: The session id (sessionRef.current) must be captured into a
session-local variable before any awaited/async setup so concurrent
startRecording calls don't share the later session id; inside startRecording()
increment sessionRef.current, then immediately assign const thisSession =
sessionRef.current and use thisSession in the dataavailable event handler and
any later session checks (instead of reading sessionRef.current after await).
Apply the same pattern where the code currently reads sessionRef.current around
the async path (the block referenced by lines ~97-101) so each recorder closure
validates against its own captured thisSession to avoid leaking old
dataavailable events into new recordings.
In `@backend/database/migrations.py`:
- Around line 307-313: The index creation for ix_profile_samples_sort_order is
currently inside the "if 'sort_order' not in columns" block so a
partially-applied migration (column added but index missing) will never
self-heal; either move the CREATE INDEX IF NOT EXISTS call for
profile_samples(sort_order) out of that conditional so it runs on every startup
after calling _add_column, or add ("profile_samples",
"ix_profile_samples_sort_order", "sort_order") to the indexes list handled by
_add_performance_indexes so the helper ensures the index exists independently of
the column-add conditional; keep the existing use of IF NOT EXISTS and reference
_add_column, _add_performance_indexes, profile_samples, sort_order, and
ix_profile_samples_sort_order when making the change.
In `@backend/routes/profiles.py`:
- Around line 238-253: reorder_samples currently performs blind updates; add
validation and error handling in the reorder_samples function: fetch all
DBProfileSample rows for the given profile_id first (query DBProfileSample where
profile_id == profile_id) and compare their ids to data.sample_ids to detect
missing ids, extra ids (ownership violations), and duplicates (use counts or set
length mismatch); if any sample_id in data.sample_ids does not belong to this
profile or if there are duplicates, raise an HTTPException with a 400/404 and a
clear message; ensure data.sample_ids includes every existing sample id for the
profile (otherwise return 400 asking the client to supply a complete ordering)
or alternatively compute a stable reassign for omitted samples; perform the
updates inside a transaction and verify each update affected exactly one row (or
update via explicit loaded objects and assign sort_order then commit), and only
call clear_profile_cache(profile_id) after successful commit.
---
Nitpick comments:
In `@backend/models.py`:
- Around line 80-84: Add validation to SampleReorderRequest.sample_ids by
declaring it with a Pydantic Field that enforces at least one element; import
Field from pydantic if not already present and change the declaration to use
Field(..., min_length=1) on the sample_ids attribute of the SampleReorderRequest
model so empty lists are rejected with a clear validation error.
In `@backend/routes/profiles.py`:
- Line 53: CHUNK_SIZE is currently defined inside a function but should be a
true module-level constant like MAX_FILE_SIZE; move the CHUNK_SIZE declaration
out of the function and place it at module scope alongside MAX_FILE_SIZE, keep
the name CHUNK_SIZE and value 1024 * 1024, and update any local references
inside the function (if any) to use the module-level CHUNK_SIZE constant.
In `@backend/services/profiles.py`:
- Around line 233-251: Remove the redundant "from sqlalchemy import func as
_func" import and update the max_order computation to use the existing func
symbol already imported earlier; specifically, delete the _func import and
change the call in the query that references
_func.max(DBProfileSample.sort_order) to use
func.max(DBProfileSample.sort_order) so DBProfileSample, next_order and
profile.updated_at logic remain unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3befb537-2cc3-433b-b53e-45c18fa3d77e
📒 Files selected for processing (12)
app/src/i18n/locales/en/translation.jsonapp/src/i18n/locales/ja/translation.jsonapp/src/i18n/locales/zh-CN/translation.jsonapp/src/i18n/locales/zh-TW/translation.jsonapp/src/lib/api/models/ProfileSampleResponse.tsapp/src/lib/api/schemas/$ProfileSampleResponse.tsapp/src/lib/hooks/useAudioRecording.tsbackend/database/migrations.pybackend/database/models.pybackend/models.pybackend/routes/profiles.pybackend/services/profiles.py
| setError(null); | ||
| chunksRef.current = []; | ||
| cancelledRef.current = false; | ||
| sessionRef.current += 1; |
Contributor
There was a problem hiding this comment.
Capture session-local state before the async start path.
Because thisSession is only read on Line 101, after the awaited setup path, two overlapping startRecording() calls can both inherit the later session id. The older recorder still appends into the shared refs, so its final dataavailable can leak into the next recording and the stale-session guard won't reliably suppress it.
Suggested fix
- sessionRef.current += 1;
+ const thisSession = sessionRef.current + 1;
+ sessionRef.current = thisSession;
+ const sessionChunks: Blob[] = [];
+ let recordingStartedAt: number | null = null;
...
const stream = await navigator.mediaDevices.getUserMedia({
audio: {
echoCancellation: true,
noiseSuppression: true,
autoGainControl: true,
},
});
+
+ if (sessionRef.current !== thisSession) {
+ stream.getTracks().forEach((track) => track.stop());
+ return;
+ }
streamRef.current = stream;
...
mediaRecorder.ondataavailable = (event) => {
if (event.data.size > 0) {
- chunksRef.current.push(event.data);
+ sessionChunks.push(event.data);
}
};
-
- const thisSession = sessionRef.current;
...
const recordedDuration = startTimeRef.current
- ? (Date.now() - startTimeRef.current) / 1000
+ ? (Date.now() - recordingStartedAt!) / 1000
: undefined;
- const webmBlob = new Blob(chunksRef.current, { type: 'audio/webm' });
+ const webmBlob = new Blob(sessionChunks, { type: 'audio/webm' });
...
- startTimeRef.current = Date.now();
+ recordingStartedAt = Date.now();
+ startTimeRef.current = recordingStartedAt;Also applies to: 97-101
🤖 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 `@app/src/lib/hooks/useAudioRecording.ts` at line 35, The session id
(sessionRef.current) must be captured into a session-local variable before any
awaited/async setup so concurrent startRecording calls don't share the later
session id; inside startRecording() increment sessionRef.current, then
immediately assign const thisSession = sessionRef.current and use thisSession in
the dataavailable event handler and any later session checks (instead of reading
sessionRef.current after await). Apply the same pattern where the code currently
reads sessionRef.current around the async path (the block referenced by lines
~97-101) so each recorder closure validates against its own captured thisSession
to avoid leaking old dataavailable events into new recordings.
Comment on lines
+307
to
+313
| if "sort_order" not in columns: | ||
| _add_column(engine, "profile_samples", "sort_order INTEGER NOT NULL DEFAULT 0", "sort_order") | ||
| with engine.connect() as conn: | ||
| conn.execute( | ||
| text("CREATE INDEX IF NOT EXISTS ix_profile_samples_sort_order ON profile_samples (sort_order)") | ||
| ) | ||
| conn.commit() |
Contributor
There was a problem hiding this comment.
Index creation gated on column absence — won't self-heal a partial migration.
_add_column commits in its own transaction (line 62), so if the column add succeeds but the subsequent CREATE INDEX fails (or the DB is later restored from a snapshot taken in between, or the index is dropped manually), the next startup will see sort_order already present, skip the whole block, and the ix_profile_samples_sort_order index will never be recreated. Note _add_performance_indexes also does not include this particular index, so there's no second-chance path.
Since CREATE INDEX IF NOT EXISTS is idempotent, lift it out of the if (or add it to the _add_performance_indexes list) so every startup ensures the index exists.
🛡️ Proposed fix
def _migrate_profile_samples(engine, inspector, tables: set[str]) -> None:
if "profile_samples" not in tables:
return
columns = _get_columns(inspector, "profile_samples")
if "sort_order" not in columns:
_add_column(engine, "profile_samples", "sort_order INTEGER NOT NULL DEFAULT 0", "sort_order")
- with engine.connect() as conn:
- conn.execute(
- text("CREATE INDEX IF NOT EXISTS ix_profile_samples_sort_order ON profile_samples (sort_order)")
- )
- conn.commit()
+ with engine.connect() as conn:
+ conn.execute(
+ text("CREATE INDEX IF NOT EXISTS ix_profile_samples_sort_order ON profile_samples (sort_order)")
+ )
+ conn.commit()Alternatively, append ("profile_samples", "ix_profile_samples_sort_order", "sort_order") to the indexes list in _add_performance_indexes and let that helper own it.
📝 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
| if "sort_order" not in columns: | |
| _add_column(engine, "profile_samples", "sort_order INTEGER NOT NULL DEFAULT 0", "sort_order") | |
| with engine.connect() as conn: | |
| conn.execute( | |
| text("CREATE INDEX IF NOT EXISTS ix_profile_samples_sort_order ON profile_samples (sort_order)") | |
| ) | |
| conn.commit() | |
| if "sort_order" not in columns: | |
| _add_column(engine, "profile_samples", "sort_order INTEGER NOT NULL DEFAULT 0", "sort_order") | |
| with engine.connect() as conn: | |
| conn.execute( | |
| text("CREATE INDEX IF NOT EXISTS ix_profile_samples_sort_order ON profile_samples (sort_order)") | |
| ) | |
| conn.commit() |
🤖 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 `@backend/database/migrations.py` around lines 307 - 313, The index creation
for ix_profile_samples_sort_order is currently inside the "if 'sort_order' not
in columns" block so a partially-applied migration (column added but index
missing) will never self-heal; either move the CREATE INDEX IF NOT EXISTS call
for profile_samples(sort_order) out of that conditional so it runs on every
startup after calling _add_column, or add ("profile_samples",
"ix_profile_samples_sort_order", "sort_order") to the indexes list handled by
_add_performance_indexes so the helper ensures the index exists independently of
the column-add conditional; keep the existing use of IF NOT EXISTS and reference
_add_column, _add_performance_indexes, profile_samples, sort_order, and
ix_profile_samples_sort_order when making the change.
Comment on lines
+238
to
+253
| @router.patch("/profiles/{profile_id}/samples/reorder") | ||
| async def reorder_samples( | ||
| profile_id: str, | ||
| data: models.SampleReorderRequest, | ||
| db: Session = Depends(get_db), | ||
| ): | ||
| """Reorder voice profile samples. Pass sample_ids in desired order.""" | ||
| for idx, sample_id in enumerate(data.sample_ids): | ||
| db.query(DBProfileSample).filter( | ||
| DBProfileSample.id == sample_id, | ||
| DBProfileSample.profile_id == profile_id, | ||
| ).update({"sort_order": idx}) | ||
| db.commit() | ||
| clear_profile_cache(profile_id) | ||
| return {"ok": True} | ||
|
|
Contributor
There was a problem hiding this comment.
Add validation and error handling to the reorder endpoint.
The current implementation has several issues:
- No validation of ownership: If
sample_idscontains samples from other profiles, theprofile_idfilter prevents updating them, but no error is raised to inform the caller. - Silent partial updates: If a sample_id doesn't exist, the update silently succeeds with 0 rows affected.
- Incomplete reordering: If
sample_idsdoesn't include all samples for the profile, those omitted samples retain their oldsort_order, potentially causing ordering conflicts. - No duplicate detection: If
sample_idscontains duplicates, the last occurrence determines the final sort_order.
🔧 Proposed fix with validation
`@router.patch`("/profiles/{profile_id}/samples/reorder")
async def reorder_samples(
profile_id: str,
data: models.SampleReorderRequest,
db: Session = Depends(get_db),
):
"""Reorder voice profile samples. Pass sample_ids in desired order."""
+ # Verify profile exists
+ profile = db.query(DBVoiceProfile).filter_by(id=profile_id).first()
+ if not profile:
+ raise HTTPException(status_code=404, detail="Profile not found")
+
+ # Check for duplicates
+ if len(data.sample_ids) != len(set(data.sample_ids)):
+ raise HTTPException(status_code=400, detail="Duplicate sample IDs in request")
+
+ # Verify all samples exist and belong to this profile
+ existing_samples = db.query(DBProfileSample).filter(
+ DBProfileSample.id.in_(data.sample_ids),
+ DBProfileSample.profile_id == profile_id,
+ ).all()
+
+ if len(existing_samples) != len(data.sample_ids):
+ raise HTTPException(status_code=400, detail="One or more sample IDs not found or don't belong to this profile")
+
for idx, sample_id in enumerate(data.sample_ids):
db.query(DBProfileSample).filter(
DBProfileSample.id == sample_id,
DBProfileSample.profile_id == profile_id,
).update({"sort_order": idx})
db.commit()
clear_profile_cache(profile_id)
return {"ok": True}📝 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
| @router.patch("/profiles/{profile_id}/samples/reorder") | |
| async def reorder_samples( | |
| profile_id: str, | |
| data: models.SampleReorderRequest, | |
| db: Session = Depends(get_db), | |
| ): | |
| """Reorder voice profile samples. Pass sample_ids in desired order.""" | |
| for idx, sample_id in enumerate(data.sample_ids): | |
| db.query(DBProfileSample).filter( | |
| DBProfileSample.id == sample_id, | |
| DBProfileSample.profile_id == profile_id, | |
| ).update({"sort_order": idx}) | |
| db.commit() | |
| clear_profile_cache(profile_id) | |
| return {"ok": True} | |
| `@router.patch`("/profiles/{profile_id}/samples/reorder") | |
| async def reorder_samples( | |
| profile_id: str, | |
| data: models.SampleReorderRequest, | |
| db: Session = Depends(get_db), | |
| ): | |
| """Reorder voice profile samples. Pass sample_ids in desired order.""" | |
| # Verify profile exists | |
| profile = db.query(DBVoiceProfile).filter_by(id=profile_id).first() | |
| if not profile: | |
| raise HTTPException(status_code=404, detail="Profile not found") | |
| # Check for duplicates | |
| if len(data.sample_ids) != len(set(data.sample_ids)): | |
| raise HTTPException(status_code=400, detail="Duplicate sample IDs in request") | |
| # Verify all samples exist and belong to this profile | |
| existing_samples = db.query(DBProfileSample).filter( | |
| DBProfileSample.id.in_(data.sample_ids), | |
| DBProfileSample.profile_id == profile_id, | |
| ).all() | |
| if len(existing_samples) != len(data.sample_ids): | |
| raise HTTPException(status_code=400, detail="One or more sample IDs not found or don't belong to this profile") | |
| for idx, sample_id in enumerate(data.sample_ids): | |
| db.query(DBProfileSample).filter( | |
| DBProfileSample.id == sample_id, | |
| DBProfileSample.profile_id == profile_id, | |
| ).update({"sort_order": idx}) | |
| db.commit() | |
| clear_profile_cache(profile_id) | |
| return {"ok": True} |
🤖 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 `@backend/routes/profiles.py` around lines 238 - 253, reorder_samples currently
performs blind updates; add validation and error handling in the reorder_samples
function: fetch all DBProfileSample rows for the given profile_id first (query
DBProfileSample where profile_id == profile_id) and compare their ids to
data.sample_ids to detect missing ids, extra ids (ownership violations), and
duplicates (use counts or set length mismatch); if any sample_id in
data.sample_ids does not belong to this profile or if there are duplicates,
raise an HTTPException with a 400/404 and a clear message; ensure
data.sample_ids includes every existing sample id for the profile (otherwise
return 400 asking the client to supply a complete ordering) or alternatively
compute a stable reassign for omitted samples; perform the updates inside a
transaction and verify each update affected exactly one row (or update via
explicit loaded objects and assign sort_order then commit), and only call
clear_profile_cache(profile_id) after successful commit.
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.
Bug fix: Rapidly stopping and restarting MediaRecorder (e.g., recording a second sample immediately after stopping the first) throws an "already recording"
InvalidStateError. Added proper state checks before start/stop calls so rapid re-record works reliably.Feature addition: Adds
sort_orderto profile samples and aPUT /profiles/{id}/samples/reorderendpoint. Users expect to be able to drag-and-drop reorder their voice samples; without server-side order persistence, the frontend can't save that state.Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements