-
Notifications
You must be signed in to change notification settings - Fork 0
Voice entries #239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Voice entries #239
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,73 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| class VoiceEntriesController < ApplicationController | ||||||||||||||||||||||||||||||||||||||||||||
| # Rate limiting: 5 requests per minute per user | ||||||||||||||||||||||||||||||||||||||||||||
| before_action :check_voice_entries_access | ||||||||||||||||||||||||||||||||||||||||||||
| before_action :check_rate_limit | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| def create | ||||||||||||||||||||||||||||||||||||||||||||
| validate_audio! | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| processor = VoiceEntryProcessor.new( | ||||||||||||||||||||||||||||||||||||||||||||
| user: current_user, | ||||||||||||||||||||||||||||||||||||||||||||
| audio_file: params[:audio_file].tempfile | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| result = processor.process | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if result[:success] | ||||||||||||||||||||||||||||||||||||||||||||
| render json: result | ||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||
| render json: result, status: :unprocessable_entity | ||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||
| rescue => e | ||||||||||||||||||||||||||||||||||||||||||||
| Rails.logger.error "Voice entry creation failed: #{e.message}" | ||||||||||||||||||||||||||||||||||||||||||||
| render json: { | ||||||||||||||||||||||||||||||||||||||||||||
| success: false, | ||||||||||||||||||||||||||||||||||||||||||||
| error: e.message, | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+23
to
+25
|
||||||||||||||||||||||||||||||||||||||||||||
| render json: { | |
| success: false, | |
| error: e.message, | |
| Rails.logger.error e.backtrace.join("\n") | |
| render json: { | |
| success: false, | |
| error: 'An error occurred while processing your request. Please try again.', |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The audio file validation raises generic exceptions with string messages that are caught and exposed to users. Use custom exception classes for better error handling and more specific error responses:
class AudioValidationError < StandardError; end
def validate_audio!
audio = params[:audio_file]
raise AudioValidationError, 'No audio file provided' unless audio.present?
raise AudioValidationError, 'Invalid file type. Must be audio file.' unless audio.content_type&.start_with?('audio/')
raise AudioValidationError, 'Audio file too large. Maximum size is 5MB.' if audio.size > 5.megabytes
endThen catch AudioValidationError specifically in the rescue clause.
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check_rate_limit method has a race condition. Between reading and writing the cache, multiple requests could pass the limit check simultaneously. Consider using atomic cache operations like Rails.cache.increment or implement a proper distributed lock mechanism:
def check_rate_limit
cache_key = "voice_entries:#{current_user.id}:#{Time.current.to_i / 60}"
count = Rails.cache.increment(cache_key, 1, expires_in: 1.minute, initial: 0)
if count > 5
render json: {
success: false,
error: 'Too many requests. Please wait a moment.',
transcription: nil
}, status: :too_many_requests
end
end| count = Rails.cache.read(cache_key) || 0 | |
| if count >= 5 | |
| render json: { | |
| success: false, | |
| error: 'Too many requests. Please wait a moment.', | |
| transcription: nil | |
| }, status: :too_many_requests | |
| return | |
| end | |
| Rails.cache.write(cache_key, count + 1, expires_in: 1.minute) | |
| count = Rails.cache.increment(cache_key, 1, expires_in: 1.minute, initial: 0) | |
| if count > 5 | |
| render json: { | |
| success: false, | |
| error: 'Too many requests. Please wait a moment.', | |
| transcription: nil | |
| }, status: :too_many_requests | |
| end |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test coverage for the new VoiceEntriesController. Given that the repository has comprehensive test coverage for other controllers (e.g., spec/controllers/entries_spec.rb), tests should be added to cover:
- Authentication and authorization checks
- Rate limiting behavior
- Audio file validation
- Success and error scenarios
- Voice entries feature flag check
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,170 @@ | ||||||||||||||||||||||||||||
| import { Controller } from "@hotwired/stimulus" | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export default class extends Controller { | ||||||||||||||||||||||||||||
| static targets = ["button", "status", "indicator", "amountField", "dateField", "notesField", "categorySelect"] | ||||||||||||||||||||||||||||
| static values = { url: String } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| connect() { | ||||||||||||||||||||||||||||
| this.mediaRecorder = null | ||||||||||||||||||||||||||||
| this.audioChunks = [] | ||||||||||||||||||||||||||||
| this.isRecording = false | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| async toggleRecording() { | ||||||||||||||||||||||||||||
| if (this.isRecording) { | ||||||||||||||||||||||||||||
| this.stopRecording() | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| await this.startRecording() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| async startRecording() { | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| // Check if browser supports MediaRecorder | ||||||||||||||||||||||||||||
| if (!navigator.mediaDevices || !navigator.mediaDevices.getUserMedia) { | ||||||||||||||||||||||||||||
| this.showError('Voice recording is not supported in your browser') | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Request microphone permission | ||||||||||||||||||||||||||||
| const stream = await navigator.mediaDevices.getUserMedia({ audio: true }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Initialize MediaRecorder | ||||||||||||||||||||||||||||
| this.mediaRecorder = new MediaRecorder(stream, { mimeType: 'audio/webm' }) | ||||||||||||||||||||||||||||
| this.audioChunks = [] | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| this.mediaRecorder.addEventListener('dataavailable', (event) => { | ||||||||||||||||||||||||||||
| this.audioChunks.push(event.data) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| this.mediaRecorder.addEventListener('stop', () => { | ||||||||||||||||||||||||||||
| const audioBlob = new Blob(this.audioChunks, { type: 'audio/webm' }) | ||||||||||||||||||||||||||||
| this.submitAudio(audioBlob) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Stop all audio tracks to release microphone | ||||||||||||||||||||||||||||
| stream.getTracks().forEach(track => track.stop()) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| this.mediaRecorder.start() | ||||||||||||||||||||||||||||
| this.isRecording = true | ||||||||||||||||||||||||||||
| this.showRecording() | ||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||
| this.showError('Failed to start recording. Please check your microphone permissions.') | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| stopRecording() { | ||||||||||||||||||||||||||||
| if (this.mediaRecorder && this.isRecording) { | ||||||||||||||||||||||||||||
| this.mediaRecorder.stop() | ||||||||||||||||||||||||||||
| this.isRecording = false | ||||||||||||||||||||||||||||
| this.showProcessing() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| async submitAudio(blob) { | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const formData = new FormData() | ||||||||||||||||||||||||||||
| formData.append('audio_file', blob, 'recording.webm') | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Get CSRF token | ||||||||||||||||||||||||||||
| const csrfToken = document.querySelector('meta[name="csrf-token"]')?.content | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const response = await fetch(this.urlValue, { | ||||||||||||||||||||||||||||
| method: 'POST', | ||||||||||||||||||||||||||||
| headers: { | ||||||||||||||||||||||||||||
| 'X-CSRF-Token': csrfToken | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| body: formData | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const result = await response.json() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (result.success) { | ||||||||||||||||||||||||||||
| this.populateForm(result.data) | ||||||||||||||||||||||||||||
| this.clearFeedback() | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| this.showError(result.error || 'Failed to process audio') | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||
| this.showError('Failed to upload audio. Please try again.') | ||||||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||||||
| this.showIdle() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| populateForm(data) { | ||||||||||||||||||||||||||||
| // Populate amount field | ||||||||||||||||||||||||||||
| if (data.amount && this.hasAmountFieldTarget) { | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| if (data.amount && this.hasAmountFieldTarget) { | |
| if (data.amount !== null && data.amount !== undefined && this.hasAmountFieldTarget) { |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant null check: The condition data.date !== null is checked twice (data.date && data.date !== null). The first check data.date already filters out null values. Simplify to:
if (data.date && this.hasDateFieldTarget) {
this.dateFieldTarget.value = data.date
}| if (data.date && data.date !== null && this.hasDateFieldTarget) { | |
| if (data.date && this.hasDateFieldTarget) { |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential XSS vulnerability: The error message is directly interpolated into HTML without sanitization. If the error message contains user-controlled content or special characters, it could lead to XSS attacks. Use textContent instead or properly escape the message:
const errorDiv = document.createElement('div')
errorDiv.style.cssText = 'padding: 1rem; background: #f8d7da; border: 1px solid #f5c6cb; border-radius: 4px; margin-top: 1rem;'
const errorP = document.createElement('p')
errorP.style.cssText = 'margin: 0; color: #721c24; font-size: 0.9rem;'
errorP.textContent = message
errorDiv.appendChild(errorP)
this.statusTarget.innerHTML = ''
this.statusTarget.appendChild(errorDiv)| this.statusTarget.innerHTML = ` | |
| <div style="padding: 1rem; background: #f8d7da; border: 1px solid #f5c6cb; border-radius: 4px; margin-top: 1rem;"> | |
| <p style="margin: 0; color: #721c24; font-size: 0.9rem;">${message}</p> | |
| </div> | |
| ` | |
| const errorDiv = document.createElement('div'); | |
| errorDiv.style.cssText = 'padding: 1rem; background: #f8d7da; border: 1px solid #f5c6cb; border-radius: 4px; margin-top: 1rem;'; | |
| const errorP = document.createElement('p'); | |
| errorP.style.cssText = 'margin: 0; color: #721c24; font-size: 0.9rem;'; | |
| errorP.textContent = message; | |
| errorDiv.appendChild(errorP); | |
| this.statusTarget.innerHTML = ''; | |
| this.statusTarget.appendChild(errorDiv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tempfile from the uploaded audio could be deleted before the VoiceEntryProcessor completes if the request processing is slow or if garbage collection occurs. Consider reading the file content into memory first or ensuring the tempfile persists for the duration of processing:
Alternatively, handle the file content directly rather than passing the path.