fix: stop the voice alert panel crashing on its first alert (#1136) - #1141
Open
MOHITKOURAV01 wants to merge 1 commit into
Open
fix: stop the voice alert panel crashing on its first alert (#1136)#1141MOHITKOURAV01 wants to merge 1 commit into
MOHITKOURAV01 wants to merge 1 commit into
Conversation
…69#1136) VoiceAlertManager renders the pending list with queue.map(...), but the hook only ever returned queueLength. That branch is unreachable while the queue is empty, so the panel looked fine until something was queued and then died with ReferenceError: queue is not defined -- the no-undef ESLint has been reporting on this file all along. Return the queue alongside its length, and while the hook is open: - Insert by priority instead of appending. Both call sites tag an alert with a priority and nothing read it, so a CRITICAL pollution alert waited behind every routine message already queued. insertByPriority leaves the head alone while it is mid-utterance, so the row marked PLAYING stays the row being spoken. - Put processQueue in its effect's dependency list. It closes over queue and config, and omitting it was the exhaustive-deps warning here; isProcessingRef still stops re-entry into an utterance in flight. - Stop speech when alerts are switched off, rather than letting the current message finish and making the toggle look inert. - Drop alerts with no usable message, and don't set voices after unmount. Tests: 21 for the hook and insertByPriority, 9 for the panel. The panel tests fail with the original ReferenceError against the code before this change.
|
@MOHITKOURAV01 is attempting to deploy a commit to the Aditya Mahajan's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thank You for Your Contribution! 🎉Hi @MOHITKOURAV01, Thank you for opening this Pull Request and contributing to our project. We truly appreciate your efforts.
The maintainer @Aditya8369 will review your PR shortly! Happy Contributing! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
VoiceAlertManagerrenders its pending-alert list withqueue.map(...), butqueuewasnever declared in that file and
useVoiceSynthesisonly ever returnedqueueLength. Thelist sits behind a
queueLength === 0 ? ... : ...guard, so the panel looked fine on loadand died the first time anything was queued:
There is no error boundary around it, so the whole voice panel went blank on the very
action it exists for. ESLint has been reporting it as
no-undefon this file the entiretime.
Related Issue
Closes #1136
Type of Change
Changes Made
src/hooks/useVoiceSynthesis.jsReturn
queuealongsidequeueLength. That alone fixes the crash.Insert by priority rather than appending. Both call sites tag their alert with a
priorityand one of them is explicitlyCRITICAL, butaddToQueuewassetQueue(prev => [...prev, alert])— a plain FIFO append that nothing read the priorityin. A critical pollution alert waited behind however many routine messages were already
queued, which is the wrong end of the queue for a feature meant to be listened to rather
than watched.
insertByPriorityandpriorityRankare exported and pure, so the ordering rules aretestable without rendering. The item at index 0 is left alone while it is being spoken —
moving it would leave the row the panel marks
PLAYINGpointing at a different alert.Interrupting speech already underway is
clearQueue's job, not an insert's.Put
processQueuein its effect's dependency list. It closes overqueueandconfig,and leaving it out was the
react-hooks/exhaustive-depswarning on this file. A configchange mid-queue now re-evaluates against the new config;
isProcessingRefstill stopsthe effect re-entering an utterance in flight.
Stop speech when alerts are switched off. Previously the toggle only declined to start
the next message, so it appeared to do nothing until the current one finished.
Ignore an alert with no usable message, so a malformed payload cannot add a silent row
that blocks the queue.
Don't
setVoicesafter unmount — the voice list resolves asynchronously and can outlivethe mount.
src/components/VoiceAlertManager.jsxqueuefrom the hook.key={item.id ?? idx}, so an alert queued without an id doesn't collide.aria-live="polite"on the pending count — this is the accessibility panel, and thequeue depth changing is exactly the sort of thing a screen reader user should hear.
Tests (new)
src/hooks/useVoiceSynthesis.test.js— 21 tests: the fullpriorityRanktable includingcase-insensitivity and unknown levels,
insertByPriorityordering and immutability, andthe hook's queue draining, priority ordering, disabled-mid-speech, clear, and
failed-utterance-doesn't-wedge-the-queue behaviour.
src/components/VoiceAlertManager.test.jsx— 9 tests covering the empty state, thepending row that used to throw, the
PLAYINGmarker, clearing, the unsupported-browserpath, and the voice dropdown.
Testing
I checked the panel tests actually catch the bug rather than just passing: reverting the
two source files and re-running gives
ReferenceError: queue is not definedon every oneof them.
npx eslint src/components/VoiceAlertManager.jsx src/hooks/useVoiceSynthesis.jsno longerreports the
no-undef. The two remaininglabel-has-associated-controlwarnings on thatfile are pre-existing and untouched here.
Note on CI
The Lint, Build and Playwright jobs are red on
mainand on every open PR right now(3 parse errors in
App.jsx,Leaderboard.jsxandNoisePollutionTracker.jsx— that's#1129). Nothing in this branch touches those files.