Skip to content

Conversation

@TheNexter
Copy link
Contributor

@TheNexter TheNexter commented Nov 18, 2025

If you need any information or change, feel free to request me.

Screenshot

image image

(Old player design, new one in the next message)
image

@TheNexter TheNexter requested a review from boojack as a code owner November 18, 2025 17:38
@TheNexter
Copy link
Contributor Author

TheNexter commented Nov 18, 2025

Edit : little refine for more simplicity
image

when hover :
image

@boojack boojack requested a review from Copilot November 19, 2025 09:29
Copilot finished reviewing on behalf of boojack November 19, 2025 09:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces an audio recording feature that allows users to record and attach audio files to memos. The implementation includes a custom audio recorder hook, a new audio player component, and UI integration in the memo editor.

  • Custom React hook (useAudioRecorder) for managing audio recording state and MediaRecorder API interactions
  • New AudioPlayer component with custom playback controls to replace the default HTML5 audio element
  • UI integration in the InsertMenu with recording controls and menu option

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
web/src/components/MemoEditor/ActionButton/InsertMenu/useAudioRecorder.ts Custom hook managing MediaRecorder API, recording state, pause/resume functionality, and timer
web/src/components/MemoEditor/ActionButton/InsertMenu.tsx Adds "Record Audio" menu item and recording UI with controls for pause/resume, stop, and cancel
web/src/components/AudioPlayer.tsx Custom audio player component with play/pause, seek, and time display functionality
web/src/components/MemoAttachment.tsx Updates audio attachment rendering to use the new AudioPlayer component

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@TheNexter
Copy link
Contributor Author

TheNexter commented Nov 19, 2025

I have apply suggestion made by copilot, if you have other suggestion or requirement, feel free to ask me anything 😊

@boojack boojack requested a review from Copilot November 19, 2025 14:08
Copilot finished reviewing on behalf of boojack November 19, 2025 14:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<span className="font-mono text-sm">{new Date(audioRecorder.recordingTime * 1000).toISOString().substring(14, 19)}</span>
</div>
<Button variant="outline" size="icon" onClick={audioRecorder.togglePause} className="shrink-0">
{audioRecorder.isPaused ? <MicIcon className="w-4 h-4" /> : <span className="font-bold text-xs">||</span>}
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pause button icon uses a hardcoded double pipe string "||" instead of a proper Pause icon component. This is inconsistent with the rest of the UI which uses Lucide icons. Consider using PauseIcon which is already imported.

Copilot uses AI. Check for mistakes.
<Button variant="outline" size="icon" onClick={handleStopRecording} className="shrink-0 text-red-600 hover:text-red-700">
<div className="w-3 h-3 bg-current rounded-sm" />
</Button>
<Button variant="ghost" size="icon" onClick={audioRecorder.cancelRecording} className="shrink-0">
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The cancel recording button lacks clear visual indication of its destructive action. Consider adding text-red-600 or similar styling to match the stop button's indication that it's a cancel/delete action, improving user understanding of the button's purpose.

Suggested change
<Button variant="ghost" size="icon" onClick={audioRecorder.cancelRecording} className="shrink-0">
<Button variant="ghost" size="icon" onClick={audioRecorder.cancelRecording} className="shrink-0 text-red-600 hover:text-red-700">

Copilot uses AI. Check for mistakes.
Comment on lines 151 to 157
<Button variant="outline" size="icon" onClick={audioRecorder.togglePause} className="shrink-0">
{audioRecorder.isPaused ? <MicIcon className="w-4 h-4" /> : <span className="font-bold text-xs">||</span>}
</Button>
<Button variant="outline" size="icon" onClick={handleStopRecording} className="shrink-0 text-red-600 hover:text-red-700">
<div className="w-3 h-3 bg-current rounded-sm" />
</Button>
<Button variant="ghost" size="icon" onClick={audioRecorder.cancelRecording} className="shrink-0">
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recording control buttons lack accessible labels. Screen reader users won't know what the pause, stop, and cancel buttons do. Add aria-label attributes to each button to describe their purpose (e.g., "Pause recording", "Stop and save recording", "Cancel recording").

Suggested change
<Button variant="outline" size="icon" onClick={audioRecorder.togglePause} className="shrink-0">
{audioRecorder.isPaused ? <MicIcon className="w-4 h-4" /> : <span className="font-bold text-xs">||</span>}
</Button>
<Button variant="outline" size="icon" onClick={handleStopRecording} className="shrink-0 text-red-600 hover:text-red-700">
<div className="w-3 h-3 bg-current rounded-sm" />
</Button>
<Button variant="ghost" size="icon" onClick={audioRecorder.cancelRecording} className="shrink-0">
<Button
variant="outline"
size="icon"
onClick={audioRecorder.togglePause}
className="shrink-0"
aria-label={audioRecorder.isPaused ? "Resume recording" : "Pause recording"}
>
{audioRecorder.isPaused ? <MicIcon className="w-4 h-4" /> : <span className="font-bold text-xs">||</span>}
</Button>
<Button
variant="outline"
size="icon"
onClick={handleStopRecording}
className="shrink-0 text-red-600 hover:text-red-700"
aria-label="Stop and save recording"
>
<div className="w-3 h-3 bg-current rounded-sm" />
</Button>
<Button
variant="ghost"
size="icon"
onClick={audioRecorder.cancelRecording}
className="shrink-0"
aria-label="Cancel recording"
>

Copilot uses AI. Check for mistakes.
Comment on lines 105 to 122
<Button
variant="ghost"
size="sm"
onClick={togglePlayPause}
disabled={isLoading}
className="shrink-0 h-auto w-auto p-0.5 hover:bg-background/50"
>
{isPlaying ? <PauseIcon className="w-4 h-4" /> : <PlayIcon className="w-4 h-4" />}
</Button>
<input
type="range"
min="0"
max={duration || 0}
value={currentTime}
onChange={handleSeek}
disabled={isLoading || !duration}
className="flex-1 h-1 bg-muted hover:bg-background/50 rounded-lg appearance-none cursor-pointer disabled:opacity-50 [&::-webkit-slider-thumb]:appearance-none [&::-webkit-slider-thumb]:w-3 [&::-webkit-slider-thumb]:h-3 [&::-webkit-slider-thumb]:rounded-full [&::-webkit-slider-thumb]:bg-primary [&::-moz-range-thumb]:w-3 [&::-moz-range-thumb]:h-3 [&::-moz-range-thumb]:rounded-full [&::-moz-range-thumb]:bg-primary [&::-moz-range-thumb]:border-0"
/>
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The audio player controls lack accessible labels. The play/pause button and seek slider need aria-label attributes for screen reader users. Add labels like "Play audio" / "Pause audio" for the button and "Seek audio position" for the range input.

Copilot uses AI. Check for mistakes.
audio.removeEventListener("timeupdate", handleTimeUpdate);
audio.removeEventListener("ended", handleEnded);
};
}, []);
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useEffect hook doesn't include src in its dependency array. When the audio source changes, the old event listeners remain attached to the audio element without proper cleanup. This can cause stale state updates and memory leaks. Add src to the dependency array or clean up and re-attach listeners when src changes.

Suggested change
}, []);
}, [src]);

Copilot uses AI. Check for mistakes.
Comment on lines 20 to 46
const startRecording = async () => {
try {
const stream = await navigator.mediaDevices.getUserMedia({ audio: true });
const mediaRecorder = new MediaRecorder(stream);
chunksRef.current = [];

mediaRecorder.ondataavailable = (e: BlobEvent) => {
if (e.data.size > 0) {
chunksRef.current.push(e.data);
}
};

mediaRecorder.start();
setState((prev: AudioRecorderState) => ({ ...prev, isRecording: true, mediaRecorder }));

timerRef.current = window.setInterval(() => {
setState((prev) => {
if (prev.isPaused) {
return prev;
}
return { ...prev, recordingTime: prev.recordingTime + 1 };
});
}, 1000);
} catch (error) {
console.error("Error accessing microphone:", error);
throw error;
}
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The media stream tracks are not stopped when an error occurs in startRecording. If navigator.mediaDevices.getUserMedia succeeds but any subsequent operation fails before the stream is stored, the tracks will remain active and the microphone will stay on. Add proper cleanup in the catch block.

Copilot uses AI. Check for mistakes.
context.setAttachmentList([...context.attachmentList, attachment]);
} catch (error: any) {
console.error("Failed to upload audio recording:", error);
toast.error(error.details || "Failed to upload audio recording");
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message fallback uses error.details which is not a standard Error property in JavaScript. Standard Error objects have message property. Consider using error.message || "Failed to upload audio recording" for proper error handling.

Suggested change
toast.error(error.details || "Failed to upload audio recording");
toast.error(error.message || "Failed to upload audio recording");

Copilot uses AI. Check for mistakes.
value={currentTime}
onChange={handleSeek}
disabled={isLoading || !duration}
className="flex-1 h-1 bg-muted hover:bg-background/50 rounded-lg appearance-none cursor-pointer disabled:opacity-50 [&::-webkit-slider-thumb]:appearance-none [&::-webkit-slider-thumb]:w-3 [&::-webkit-slider-thumb]:h-3 [&::-webkit-slider-thumb]:rounded-full [&::-webkit-slider-thumb]:bg-primary [&::-moz-range-thumb]:w-3 [&::-moz-range-thumb]:h-3 [&::-moz-range-thumb]:rounded-full [&::-moz-range-thumb]:bg-primary [&::-moz-range-thumb]:border-0"
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The range input has extremely long className with complex styling that makes it hard to read and maintain. Consider extracting this to a separate CSS class or using a more maintainable approach with CSS modules or styled components.

Copilot uses AI. Check for mistakes.
Comment on lines 35 to 42
timerRef.current = window.setInterval(() => {
setState((prev) => {
if (prev.isPaused) {
return prev;
}
return { ...prev, recordingTime: prev.recordingTime + 1 };
});
}, 1000);
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The recording timer updates state every second, causing re-renders of the entire component tree. Since the timer display is the only thing changing, consider using a ref for the timer value and only update state for the display, or memoize expensive child components to prevent unnecessary re-renders.

Copilot uses AI. Check for mistakes.
Comment on lines 49 to 78
const stopRecording = (): Promise<Blob> => {
return new Promise((resolve, reject) => {
const { mediaRecorder } = state;
if (!mediaRecorder) {
reject(new Error("No active recording"));
return;
}

mediaRecorder.onstop = () => {
const blob = new Blob(chunksRef.current, { type: "audio/webm" });
chunksRef.current = [];
resolve(blob);
};

mediaRecorder.stop();
mediaRecorder.stream.getTracks().forEach((track: MediaStreamTrack) => track.stop());

if (timerRef.current) {
clearInterval(timerRef.current);
timerRef.current = null;
}

setState({
isRecording: false,
isPaused: false,
recordingTime: 0,
mediaRecorder: null,
});
});
};
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timer reference uses number type which is correct for browser setInterval, but the state update closure in the timer captures stale state. When stopRecording or cancelRecording is called, the timer continues to run until explicitly cleared because the promise resolution happens after the interval is already running. Consider moving the timer cleanup before the mediaRecorder stop to ensure proper cleanup order.

Copilot uses AI. Check for mistakes.
…with better responsive design) (lint done this time)
@TheNexter
Copy link
Contributor Author

If you need more stuff, feel free to ask me 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant