Add local MIDI upload and converted audio/MIDI export#2
Conversation
|
@RobertAgee is attempting to deploy a commit to the Boom Test Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 17 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR adds local MIDI file upload and replacement UI, engine export APIs for MIDI/WAV bytes, utils to serialize MIDI and WAV, UI wiring for save/copy/share, and source-aware sharing rules that prevent sharing local uploads. ChangesLocal MIDI upload and export functionality
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.ts (1)
248-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale search results when starting over.
setState()now keeps the results table visible wheneversearchResults.length > 0, but neither reset path empties that array. After New search or a fresh search that finds nothing, uploading a local MIDI can bring back the previous remote results under the upload card.Also applies to: 349-351, 988-990
🤖 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 `@src/main.ts` around lines 248 - 257, resetToNewSearch currently resets playback state but leaves the stale searchResults intact; update resetToNewSearch to also clear the results array (e.g., set this.searchResults = [] or call the centralized clear method) so the results table won't reappear after uploading a local MIDI; apply the same change to the other reset paths that perform similar state clears (the other methods that set hasGenerated = false / isMotifPlaying = false) so any "new search" or empty-search path consistently empties searchResults.
🤖 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 `@src/core/MotifEngine.ts`:
- Around line 44-45: The current assignments to lastInputEvents and
lastTransformMode occur before generation and can leave export APIs returning
stale data if generation fails; update the code so you either (a) clear all
cached export state (lastInputEvents, lastTransformMode, lastGeneratedEvents)
before starting a new generation attempt, or (b) move the assignments to the
success path after procedural/passthrough setup and after lastGeneratedEvents is
refreshed, ensuring hasGeneratedContent() only reflects successful runs;
identify and change the logic around the generation entry point and the
procedural/passthrough setup functions to apply one of these fixes.
In `@src/main.ts`:
- Around line 402-405: The current guard only checks for empty files (file.size
<= 0) but doesn't prevent oversized files being read into memory; update the
upload handler (the block using file.size and calling this.updateStatus) to
enforce a maximum allowed upload size by introducing a MAX_UPLOAD_BYTES constant
and performing an early check like if (file.size > MAX_UPLOAD_BYTES) {
this.updateStatus('Upload error: File exceeds max size'); return; } before any
file reads/parsing so oversized files are rejected prior to loading into memory.
- Around line 407-443: The code currently clears UI/state (handleMotifStop,
stopPreview, hasGenerated=false, disablePlayerControls, updateStatus('Loading
uploaded MIDI…'), etc.) before awaiting file.arrayBuffer() and parsing, which
leaves the UI disabled if parsing fails; move all state-clearing and UI changes
that disable controls (handleMotifStop(), stopPreview(), setting hasGenerated,
disablePlayerControls(), updateStatus('Loading uploaded MIDI…')) to occur only
after the MIDI is successfully read/validated (after MIDIParser.parseMIDI and
getMIDIInfo succeed) and right before setting
this.currentMIDI/this.currentSource/selectedTitle/selectedMeta and enabling
controls, so failures in arrayBuffer/parse won't leave the previous currentMIDI
state inaccessible.
- Around line 394-399: The code treats an empty MIME type as valid; change the
validType check to require a non-empty, recognized MIME by replacing the current
validType definition (now: file.type === '' || file.type === 'audio/midi' ||
file.type === 'audio/x-midi') with one that only accepts 'audio/midi' or
'audio/x-midi' (i.e., file.type === 'audio/midi' || file.type ===
'audio/x-midi'), leaving the rest of the logic (the if that calls
this.updateStatus(...)) unchanged so files with blank/unknown MIME types are
rejected unless the filename extension is explicitly .mid/.midi.
In `@src/utils/export.ts`:
- Around line 40-42: The code treating velocity as (e.velocity || 0.7)
incorrectly treats an explicit 0 as missing; update the velocity fallback to
preserve zero by using a nullish/coalescing check (e.velocity ?? 0.7) or an
explicit undefined check, then continue to multiply by 127/round/clamp as before
(the change affects the expression used to compute vel where Math.round and
clamp are applied before pushing into midiEvents via midiEvents.push).
---
Outside diff comments:
In `@src/main.ts`:
- Around line 248-257: resetToNewSearch currently resets playback state but
leaves the stale searchResults intact; update resetToNewSearch to also clear the
results array (e.g., set this.searchResults = [] or call the centralized clear
method) so the results table won't reappear after uploading a local MIDI; apply
the same change to the other reset paths that perform similar state clears (the
other methods that set hasGenerated = false / isMotifPlaying = false) so any
"new search" or empty-search path consistently empties searchResults.
🪄 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: d40a9980-9617-4895-bb61-9c451365b62e
📒 Files selected for processing (4)
index.htmlsrc/core/MotifEngine.tssrc/main.tssrc/utils/export.ts
|
Thanks, validated all 5 points. I'll patch stale result clearing, upload size limits, upload state ordering, export cache consistency, and velocity nullish fallback; as well as require known MIDI MIME or .mid/.midi extension, to avoid rejecting valid browser uploads with inconsistent MIME handling. |
|
Thanks for the review — addressed the actionable items in
Revalidated with |
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)
src/main.ts (1)
354-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore stale search completions after the active source changes.
A pending search still calls
displayResults()/setState('results')even if the user uploads a local MIDI before the request resolves, so the late response can knock the app out of the uploaded-source flow and hide the selected upload again. Guard the post-awaitpath with a request token or equivalent stale-response check before mutating UI state.Suggested direction
class MotifApp { private static readonly MAX_UPLOAD_BYTES = 10 * 1024 * 1024; + private searchRequestId = 0; private motifEngine: MotifEngine; private midiService: MIDIService; ... private async handleSearch(): Promise<void> { + const requestId = ++this.searchRequestId; console.log('[MotifApp] handleSearch called'); const songName = this.songInput.value.trim(); ... try { const results = await this.midiService.search(songName); + if (requestId !== this.searchRequestId || this.currentSource?.type === 'upload') { + return; + } if (results.length === 0) { this.searchResults = []; this.updateStatus('No MIDI files found. Try a different search.'); return;🤖 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 `@src/main.ts` around lines 354 - 380, The pending search can mutate UI after the active source changes—prevent this by adding a search-request token/ID on the instance (e.g., this.currentSearchId) that you increment before calling this.midiService.search(songName), capture into a local const (e.g., const reqId = this.currentSearchId), and then after each await (both the search and the parseMIDI loop) bail out if reqId !== this.currentSearchId; only then assign this.searchResults, this.selectedResultIndex, call this.displayResults(), this.setState('results'), and updateStatus. Ensure the same check is applied before setting parsed metadata on results[i] so late parsing doesn't leak into a stale UI.
🤖 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 `@src/main.ts`:
- Around line 354-380: The pending search can mutate UI after the active source
changes—prevent this by adding a search-request token/ID on the instance (e.g.,
this.currentSearchId) that you increment before calling
this.midiService.search(songName), capture into a local const (e.g., const reqId
= this.currentSearchId), and then after each await (both the search and the
parseMIDI loop) bail out if reqId !== this.currentSearchId; only then assign
this.searchResults, this.selectedResultIndex, call this.displayResults(),
this.setState('results'), and updateStatus. Ensure the same check is applied
before setting parsed metadata on results[i] so late parsing doesn't leak into a
stale UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fee877c-c916-4996-9007-bb4a648aadb1
📒 Files selected for processing (3)
src/core/MotifEngine.tssrc/main.tssrc/utils/export.ts
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 (3)
src/main.ts (3)
404-458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake upload completion source-aware, not just search-aware.
handleUploadedMIDI()bumpssearchRequestId, but it never captures or checks it afterawait file.arrayBuffer(). A slower first upload can finish after a newer upload/search and then overwritecurrentMIDI/currentSourcewith the wrong file, or surface a stale parse error.Suggested fix
private async handleUploadedMIDI(file: File): Promise<void> { - this.searchRequestId++; + const requestId = ++this.searchRequestId; const lowerName = file.name.toLowerCase(); const validExt = lowerName.endsWith('.mid') || lowerName.endsWith('.midi'); const validType = file.type === 'audio/midi' || file.type === 'audio/x-midi'; @@ try { const midiBuffer = await file.arrayBuffer(); + if (requestId !== this.searchRequestId) return; + if (midiBuffer.byteLength < 14) { throw new Error('File is too small to be valid MIDI.'); } @@ this.playerSection.classList.add('visible'); this.resultsSection.classList.add('collapsed'); } catch (error) { - this.updateStatus(`Upload error: ${error instanceof Error ? error.message : 'Failed to load MIDI.'}`); + if (requestId === this.searchRequestId) { + this.updateStatus(`Upload error: ${error instanceof Error ? error.message : 'Failed to load MIDI.'}`); + } } }🤖 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 `@src/main.ts` around lines 404 - 458, handleUploadedMIDI increments this.searchRequestId but never checks it after awaiting async work, so a slower upload can overwrite state from a newer upload; capture the current request id into a local variable right after incrementing (e.g., const requestId = this.searchRequestId) and after each await/long synchronous step (after file.arrayBuffer(), after MIDIParser.parseMIDI, after MIDIParser.getMIDIInfo) verify that this.searchRequestId === requestId and bail out early if it differs to avoid applying stale results to currentMIDI/currentSource/state; keep all existing validation and error handling but add these guard checks using the unique symbols handleUploadedMIDI and searchRequestId.
445-453:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear stale embed UI when switching to a local upload.
This success path flips the active source to
upload, but it does not hide/reset the embed snippet. If I select a remote result first and then replace it with a local file, the page can keep showing embed code for the previous remote song even though uploads are intentionally non-shareable.Suggested fix
this.currentMIDI = { events, metadata: { ...metadata, duration: actualDuration } }; this.currentSource = { type: 'upload', fileName: file.name }; + if (this.embedSection) this.embedSection.style.display = 'none'; + if (this.embedCodeEl) this.embedCodeEl.textContent = ''; this.selectedTitle.textContent = this.cleanSongTitle(file.name); this.selectedMeta.textContent = 'Source: Local Upload';🤖 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 `@src/main.ts` around lines 445 - 453, When switching the active source to an upload (you already set this.currentSource = { type: 'upload', ... }), also clear or hide any stale embed UI from a previously selected remote result: reset the embed snippet/display and any embed-related state so the UI no longer shows share/embed code for uploads. Locate the embed UI element or state used elsewhere in the file (the snippet/embed container or variable that holds embed code) and clear its textContent/state or call the existing UI-reset helpers (in the vicinity of clearSelectedResultHighlight/updateIOSAudioBanner/enablePlayerControls) so after setting currentSource to 'upload' the embed snippet is removed and cannot be shown for local uploads. Ensure updateStatus/setState remain unchanged.
389-393:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard stale search failures too.
requestIdis only checked on the success path. If an older search rejects after a newer search or upload has started, Line 390 can overwrite the current status with a stale error, and Line 392 can re-enable the search button while the latest request is still in flight.Suggested fix
} catch (error) { - this.updateStatus(`Search error: ${error instanceof Error ? error.message : 'Unknown error'}`); + if (requestId === this.searchRequestId) { + this.updateStatus(`Search error: ${error instanceof Error ? error.message : 'Unknown error'}`); + } } finally { - this.searchBtn.disabled = false; + if (requestId === this.searchRequestId) { + this.searchBtn.disabled = false; + } }🤖 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 `@src/main.ts` around lines 389 - 393, The catch/finally blocks currently unconditionally call this.updateStatus and re-enable this.searchBtn which can apply stale results; capture the requestId used for the current search (e.g., store a localRequestId when the search starts) and in both the catch and finally blocks verify that localRequestId === requestId (or that the component's latestRequestId matches) before calling this.updateStatus(...) or setting this.searchBtn.disabled = false so only the most recent request can change UI state.
🤖 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 `@src/main.ts`:
- Around line 404-458: handleUploadedMIDI increments this.searchRequestId but
never checks it after awaiting async work, so a slower upload can overwrite
state from a newer upload; capture the current request id into a local variable
right after incrementing (e.g., const requestId = this.searchRequestId) and
after each await/long synchronous step (after file.arrayBuffer(), after
MIDIParser.parseMIDI, after MIDIParser.getMIDIInfo) verify that
this.searchRequestId === requestId and bail out early if it differs to avoid
applying stale results to currentMIDI/currentSource/state; keep all existing
validation and error handling but add these guard checks using the unique
symbols handleUploadedMIDI and searchRequestId.
- Around line 445-453: When switching the active source to an upload (you
already set this.currentSource = { type: 'upload', ... }), also clear or hide
any stale embed UI from a previously selected remote result: reset the embed
snippet/display and any embed-related state so the UI no longer shows
share/embed code for uploads. Locate the embed UI element or state used
elsewhere in the file (the snippet/embed container or variable that holds embed
code) and clear its textContent/state or call the existing UI-reset helpers (in
the vicinity of
clearSelectedResultHighlight/updateIOSAudioBanner/enablePlayerControls) so after
setting currentSource to 'upload' the embed snippet is removed and cannot be
shown for local uploads. Ensure updateStatus/setState remain unchanged.
- Around line 389-393: The catch/finally blocks currently unconditionally call
this.updateStatus and re-enable this.searchBtn which can apply stale results;
capture the requestId used for the current search (e.g., store a localRequestId
when the search starts) and in both the catch and finally blocks verify that
localRequestId === requestId (or that the component's latestRequestId matches)
before calling this.updateStatus(...) or setting this.searchBtn.disabled = false
so only the most recent request can change UI state.
|
Implemented follow-up updates from review and validated end-to-end. Changes included
|
|
Hey Robert — thanks a lot for this, it looks really nice. Funny timing — I actually dug into the architecture for local upload back in January and then paused it. I've been heads-down on a legal AI project and the Fireship feature caught me a bit off guard, so I need to get my head back up to speed with where things are. I'll give this a proper look over the next couple of days. I've also got a v2 of the sound engine I haven't shipped yet (still a bit glitchy), so I want to think about how everything fits together before merging. Either way I'll integrate this or accept the PR — the contribution is solid and it's the right feature to add. Appreciate you taking the time. |
2 participants
Summary
This PR adds two major improvements to the Motif playback flow:
What Changed
Sharing Behavior
Backend Impact
Validation
Summary by CodeRabbit
New Features
Behavior Changes