Whisper download fix#312
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request introduces a "Paste Text" feature for the knowledge base, including a new AddTextModal component and a corresponding indexTextContent method in the RAG service. It also refactors whisperService to utilize the backgroundDownloadService for model downloads and removes the native Android DownloadForegroundService, shifting progress tracking to the JavaScript layer. Review feedback identifies a potential race condition in tracking active download IDs, a path traversal vulnerability when generating filenames from user-provided titles, and opportunities to improve input validation and error-state file cleanup.
| downloadIdPromise.then(id => { this.activeDownloadId = id; }).catch(() => {}); | ||
| try { | ||
| await promise; |
There was a problem hiding this comment.
There is a potential race condition here. activeDownloadId is set asynchronously via .then(). If deleteModel is called immediately after downloadModel starts but before the promise resolves, activeDownloadId will still be null, and the background download will not be cancelled. Awaiting the ID before awaiting the download completion reduces this window.
| downloadIdPromise.then(id => { this.activeDownloadId = id; }).catch(() => {}); | |
| try { | |
| await promise; | |
| try { | |
| this.activeDownloadId = await downloadIdPromise; | |
| await promise; |
| const charCount = text.length; | ||
| const tooShort = charCount > 0 && charCount < MIN_CHARS; | ||
| const tooLong = charCount > PASTE_MAX_CHARS; | ||
| const canSave = charCount >= MIN_CHARS && !tooLong && !indexing; |
There was a problem hiding this comment.
The character count and validation should ideally use the trimmed text length. Currently, a user could bypass the MIN_CHARS requirement by entering only whitespace, which would later fail during the indexing process when no chunks are produced.
| const charCount = text.length; | |
| const tooShort = charCount > 0 && charCount < MIN_CHARS; | |
| const tooLong = charCount > PASTE_MAX_CHARS; | |
| const canSave = charCount >= MIN_CHARS && !tooLong && !indexing; | |
| const charCount = text.trim().length; | |
| const tooShort = charCount > 0 && charCount < MIN_CHARS; | |
| const tooLong = text.length > PASTE_MAX_CHARS; | |
| const canSave = charCount >= MIN_CHARS && !tooLong && !indexing; |
| const { projectId, title, text, onProgress } = params; | ||
| await this.ensureReady(); | ||
|
|
||
| const baseName = title.trim() || 'Note'; |
There was a problem hiding this comment.
The baseName is derived directly from the user-provided title without sanitization. This could lead to path traversal issues. As per project rules, sanitize the filename by extracting only the last component of the path (e.g., using .split('/').pop()) to ensure the file remains within the intended directory.
| const baseName = title.trim() || 'Note'; | |
| const baseName = title.split('/').pop() || 'Note'; |
References
- When extracting filenames from user input (e.g., from a document picker URI), always sanitize the filename to prevent path traversal vulnerabilities. This can be achieved by extracting only the last component of the path (e.g., using .split('/').pop()) to ensure the file remains within the intended directory.
| } catch (validationError) { | ||
| await RNFS.unlink(destPath).catch(() => {}); | ||
| throw new Error(`Downloaded model file is invalid: ${validationError instanceof Error ? validationError.message : 'unknown error'}`); | ||
| } |
There was a problem hiding this comment.
If the model file fails validation after a successful download, it should be deleted to prevent leaving a corrupt or invalid file in the models directory. Ensure that any errors during deletion are logged rather than swallowed, adhering to the project's error handling rules.
| } catch (validationError) { | |
| await RNFS.unlink(destPath).catch(() => {}); | |
| throw new Error(`Downloaded model file is invalid: ${validationError instanceof Error ? validationError.message : 'unknown error'}`); | |
| } | |
| } catch (validationError) { | |
| await RNFS.unlink(destPath).catch(err => console.error('Failed to delete invalid model:', err)); | |
| throw new Error("Downloaded model file is invalid: " + (validationError instanceof Error ? validationError.message : "unknown error")); | |
| } |
References
- When catching errors, log them instead of swallowing them to ensure failures are visible and to aid in debugging.
…nloadService Co-authored-by: Dishit <hanmadishit74@gmail.com>
…d service crashes Co-authored-by: Dishit <hanmadishit74@gmail.com>
…ve to 7.1.0 Removes DownloadForegroundServiceTest.kt which referenced the now-deleted DownloadForegroundService class, fixing Android compilation. Upgrades react-native-zip-archive from 7.0.2 to 7.1.0 to fix the NPE crash when rejecting a promise with a null code (issue #336). Runs pod install to sync iOS Podfile.lock after the package upgrade. Co-Authored-By:Dishit <hanmadishit74@gmail.com>
0161e28 to
9d9e8f7
Compare
Co-authored-by: Dishit <hanmadishit74@gmail.com>
…ompile error Java does not allow switch on double. Cast compressionLevel to int so the switch compiles. Patch auto-applies via patch-package postinstall on CI. Co-authored-by: Dishit <hanmadishit74@gmail.com>
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (68.75%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
==========================================
- Coverage 83.85% 83.84% -0.02%
==========================================
Files 224 224
Lines 11478 11487 +9
Branches 3145 3147 +2
==========================================
+ Hits 9625 9631 +6
- Misses 1070 1072 +2
- Partials 783 784 +1
🚀 New features to boost your workflow:
|



Summary
Type of Change
Screenshots / Screen Recordings
Android
iOS
Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Additional Notes