Visible feedback#261
Conversation
✅ Deploy Preview for freedevtool ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds toast notifications across multiple tool pages to provide immediate visual feedback for user actions, fulfilling the requirement in STYLE.md that tools should "provide immediate feedback for user actions with a toast notification."
Key changes:
- Toast notifications added for all camera/microphone permission requests and state changes
- Toast notifications added for all timer operations (start, pause, reset, etc.)
- Updated cleanup logic in webcam-test to avoid showing toast on component unmount
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| client/src/pages/tools/webcam-test.tsx | Added toast notifications for camera permission requests, camera start/stop, photo capture, and updated cleanup logic to suppress toast on unmount |
| client/src/pages/tools/timer.tsx | Added toast notifications for all timer operations including toggle, global controls (start/stop/pause/resume all), and individual timer actions (reset) |
| client/src/pages/tools/microphone-test.tsx | Added toast notification for microphone permission request (partial implementation shown in diff) |
| return () => { | ||
| stopCamera(); | ||
| // Clean up silently on unmount without showing a toast | ||
| if (stream) { | ||
| stream.getTracks().forEach(track => track.stop()); | ||
| streamRef.current = null; | ||
| } | ||
| if (videoEl) { | ||
| videoEl.srcObject = null; | ||
| } | ||
| setIsActive(false); | ||
| }; |
There was a problem hiding this comment.
The stopCamera callback is missing the toast dependency in the cleanup function within useEffect. The cleanup function on line 260-270 duplicates the logic from stopCamera but doesn't call it directly. However, since stopCamera now includes a toast notification (which shouldn't be shown on unmount), this creates an inconsistency where the cleanup differs from the normal stop behavior.
The current approach of duplicating cleanup logic in the useEffect is correct for avoiding the toast on unmount, but this creates a maintenance issue. Consider extracting the cleanup logic to a separate function that both stopCamera and the cleanup can use, with stopCamera calling it and then showing the toast.
| const videoEl = videoRef.current; | ||
| const stream = streamRef.current; | ||
| return () => { | ||
| stopCamera(); | ||
| // Clean up silently on unmount without showing a toast | ||
| if (stream) { | ||
| stream.getTracks().forEach(track => track.stop()); | ||
| streamRef.current = null; | ||
| } | ||
| if (videoEl) { | ||
| videoEl.srcObject = null; | ||
| } | ||
| setIsActive(false); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
The useEffect dependency array on line 271 is empty but the cleanup function accesses streamRef.current and videoRef.current which are captured at mount time. The cleanup is capturing the initial values of stream and videoEl on lines 258-259, but these might be stale if the refs change during the component's lifetime.
While refs don't typically change their identity, the .current values do. The current approach of capturing them in variables won't reflect updates that happen during the component lifecycle. This could lead to resource leaks if the stream is replaced without proper cleanup.
| const target = timers.find(t => t.id === id); | ||
| const wasRunning = target?.isRunning ?? false; | ||
| const wasFinished = target?.isFinished ?? false; | ||
|
|
||
| setTimers(prev => | ||
| prev.map(timer => | ||
| timer.id === id | ||
| ? { ...timer, isRunning: !timer.isRunning, isFinished: false } | ||
| : timer | ||
| ) | ||
| ); | ||
|
|
||
| if (wasFinished) { | ||
| toast({ | ||
| title: "Timer Restarted", | ||
| description: `${target?.name ?? "Timer"} restarted.`, | ||
| }); | ||
| } else if (wasRunning) { | ||
| toast({ | ||
| title: "Timer Paused", | ||
| description: `${target?.name ?? "Timer"} paused.`, | ||
| }); | ||
| } else { | ||
| toast({ | ||
| title: "Timer Started", | ||
| description: `${target?.name ?? "Timer"} started.`, | ||
| }); | ||
| } | ||
| }, | ||
| [timers, toast] |
There was a problem hiding this comment.
The toggleTimer function now depends on the timers array (line 215-216) to find the target timer's state before the update, but this creates a race condition. The timers array in the dependency could be stale when the callback executes, causing the toast to show incorrect information about what state the timer was in.
The safer approach is to capture the previous state from within the setTimers updater function, which receives the current state as its argument.
| const target = timers.find(t => t.id === id); | |
| const wasRunning = target?.isRunning ?? false; | |
| const wasFinished = target?.isFinished ?? false; | |
| setTimers(prev => | |
| prev.map(timer => | |
| timer.id === id | |
| ? { ...timer, isRunning: !timer.isRunning, isFinished: false } | |
| : timer | |
| ) | |
| ); | |
| if (wasFinished) { | |
| toast({ | |
| title: "Timer Restarted", | |
| description: `${target?.name ?? "Timer"} restarted.`, | |
| }); | |
| } else if (wasRunning) { | |
| toast({ | |
| title: "Timer Paused", | |
| description: `${target?.name ?? "Timer"} paused.`, | |
| }); | |
| } else { | |
| toast({ | |
| title: "Timer Started", | |
| description: `${target?.name ?? "Timer"} started.`, | |
| }); | |
| } | |
| }, | |
| [timers, toast] | |
| let prevTimer: TimerInstance | undefined; | |
| setTimers(prev => { | |
| prevTimer = prev.find(t => t.id === id); | |
| return prev.map(timer => | |
| timer.id === id | |
| ? { ...timer, isRunning: !timer.isRunning, isFinished: false } | |
| : timer | |
| ); | |
| }); | |
| // Use prevTimer after setTimers to ensure correct previous state | |
| const wasRunning = prevTimer?.isRunning ?? false; | |
| const wasFinished = prevTimer?.isFinished ?? false; | |
| if (wasFinished) { | |
| toast({ | |
| title: "Timer Restarted", | |
| description: `${prevTimer?.name ?? "Timer"} restarted.`, | |
| }); | |
| } else if (wasRunning) { | |
| toast({ | |
| title: "Timer Paused", | |
| description: `${prevTimer?.name ?? "Timer"} paused.`, | |
| }); | |
| } else { | |
| toast({ | |
| title: "Timer Started", | |
| description: `${prevTimer?.name ?? "Timer"} started.`, | |
| }); | |
| } | |
| }, | |
| [toast] |
| const t = timers.find(x => x.id === id); | ||
| toast({ | ||
| title: "Timer Reset", | ||
| description: `${t?.name ?? "Timer"} reset to original duration.`, | ||
| }); | ||
| }, | ||
| [clearAlarmHandle] | ||
| [clearAlarmHandle, timers, toast] |
There was a problem hiding this comment.
The stopTimer function (which is actually resetting the timer based on the logic) accesses timers.find(x => x.id === id) on line 494 after updating the state. This reads from stale state - the timers array used here is from the closure when the callback was created, not the updated state. This means the toast message will use the old timer name if it exists, or potentially show "Timer" if the lookup fails.
The timer lookup should happen before the state update, or the name should be captured from the updater function's argument.
| const count = timers.filter(t => !t.isFinished && t.timeLeft > 0).length; | ||
| toast({ | ||
| title: "Timers Started", | ||
| description: | ||
| count > 0 | ||
| ? `Started ${count} timer${count === 1 ? "" : "s"}.` | ||
| : "No timers available to start.", | ||
| }); | ||
| }, [timers, toast]); |
There was a problem hiding this comment.
The startAllTimers function reads timers.filter(t => !t.isFinished && t.timeLeft > 0).length on line 321 to count timers for the toast message, but this uses the stale timers array from the closure. The count is computed after the state update on lines 311-318, so it's reading the old state rather than the newly updated state. This could result in an incorrect count being displayed in the toast message.
The count should be computed before the state update or from within the updater function.
Closes #128 and #113