modify the toast notification system to prevent spamming#293
Conversation
|
Suppressed Duplicate Errors in ToastProvider |
rmyndharis
left a comment
There was a problem hiding this comment.
Thanks @quinton-8 — appreciate the focus on toast spam, and the happy path (success/warning/info and normal errors) is correctly left untouched. A few things to address before merge:
1. (Blocking) setTimeout is called inside the setToasts updater. State updater functions must be side-effect-free — under React 18 Strict Mode the updater runs twice (so two timers fire), and if an update is replayed/batched the timer can fire for a toast that was never committed, which can leave the dedup in a stuck state. Please mirror the existing addToast structure: compute the id first, do the dedup guard inside the updater (return prev when it's a duplicate), and call setTimeout(() => removeToast(id), 6000) after setToasts, not inside it. (This is the "side effects outside state updates" pattern in the repo's known-issues list.)
2. Extract the dedup sentinel. 'Server Connection Lost' is a hardcoded string used as the dedup key; if it's ever translated, the dedup silently breaks. Please pull it into a module-level constant (or an i18n key).
3. (Optional / follow-up) Scope. This only dedups a fixed list of network-error strings; other rapidly-repeating errors still stack. A more general fix would be a (type, title) dedup inside addToast itself — happy to take that as a separate follow-up if you'd prefer to keep this PR focused.
Items 1 and 2 are needed to merge; 3 is a nice-to-have. Thanks!
|
Okay 👍 will do
…On Wed, 17 Jun 2026 at 13:56, Yudhi Armyndharis ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks @quinton-8 <https://github.com/quinton-8> — appreciate the focus
on toast spam, and the happy path (success/warning/info and normal
errors) is correctly left untouched. A few things to address before merge:
*1. (Blocking) setTimeout is called inside the setToasts updater.* State
updater functions must be side-effect-free — under React 18 Strict Mode the
updater runs twice (so two timers fire), and if an update is
replayed/batched the timer can fire for a toast that was never committed,
which can leave the dedup in a stuck state. Please mirror the existing
addToast structure: compute the id first, do the dedup guard inside the
updater (return prev when it's a duplicate), and call setTimeout(() =>
removeToast(id), 6000) *after* setToasts, not inside it. (This is the
"side effects outside state updates" pattern in the repo's known-issues
list.)
*2. Extract the dedup sentinel.* 'Server Connection Lost' is a hardcoded
string used as the dedup key; if it's ever translated, the dedup silently
breaks. Please pull it into a module-level constant (or an i18n key).
*3. (Optional / follow-up) Scope.* This only dedups a fixed list of
network-error strings; other rapidly-repeating errors still stack. A more
general fix would be a (type, title) dedup inside addToast itself — happy
to take that as a separate follow-up if you'd prefer to keep this PR
focused.
Items 1 and 2 are needed to merge; 3 is a nice-to-have. Thanks!
—
Reply to this email directly, view it on GitHub
<#293?email_source=notifications&email_token=BSNSKPJLLAJMFIV4VB72Z3D5AJ2NRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJRGQ4DMMBVGU4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4514860558>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BSNSKPIAA2TL5QTNT44N6EL5AJ2NRAVCNFSNUABGKJSXA33TNF2G64TZHMYTCNBXHE3DCMZZHA5US43TOVSTWNBWHAYTSNZQGQ2DTILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BSNSKPOT47AORPNSMO7KTNL5AJ2NRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJRGQ4DMMBVGU4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/BSNSKPIPEUBZDYMKT2SFSNL5AJ2NRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJRGQ4DMMBVGU4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Thanks @quinton-8 — deduping the connection-error toasts is a real UX win; a downed backend currently spams the screen. A few changes before merge:
Happy to re-review once those are in. Thanks! |
|
Hi @quinton-8 — thanks again for tackling the toast spam, this is a genuine UX win (a downed backend currently floods the screen). Circling back since the branch hasn't been updated yet. Two small things remain from the earlier review:
If you have a moment to push those, I'll re-review right away. And no pressure at all — if you're tied up, I'm happy to take it from here and finish it off, since it's a small change. A quick unit test ("a second connection error doesn't add a second toast") would be a nice bonus but is optional. Thanks for the contribution! |
|
Ok, I'll take a look.
…On Sat, 20 Jun 2026 at 04:36, Yudhi Armyndharis ***@***.***> wrote:
*rmyndharis* left a comment (rmyndharis/OpenWA#293)
<#293 (comment)>
Hi @quinton-8 <https://github.com/quinton-8> — thanks again for tackling
the toast spam, this is a genuine UX win (a downed backend currently floods
the screen). Circling back since the branch hasn't been updated yet. Two
small things remain from the earlier review:
1.
Move the auto-dismiss out of the state updater. Right now
setTimeout(() => removeToast(id), 6000) runs inside setToasts(prev => ...).
Updaters need to stay side-effect-free — under React 18 StrictMode the
updater runs twice, so the timer gets scheduled twice. The cleanest fix is
to mirror the existing addToast (Toast.tsx lines 47-63): compute the id
first, do the dedup guard inside the updater (return prev if it's a
duplicate), then call setTimeout(() => removeToast(id), 6000) after
setToasts.
2.
i18n + a stable dedup key. The component already uses useTranslation,
and the dashboard ships nine locales under dashboard/src/i18n/locales/, so
'Server Connection Lost' and the body string should move into the locale
files and be rendered via t(...). One gotcha: since the title doubles as
the dedup key, translating it would silently break dedup — so it's sturdier
to dedup on a stable flag (e.g. a dedupeKey: 'connection-lost' field)
rather than the visible title.
If you have a moment to push those, I'll re-review right away. And no
pressure at all — if you're tied up, I'm happy to take it from here and
finish it off, since it's a small change. A quick unit test ("a second
connection error doesn't add a second toast") would be a nice bonus but is
optional. Thanks for the contribution!
—
Reply to this email directly, view it on GitHub
<#293?email_source=notifications&email_token=BSNSKPLASDQVP2OJC6AJPGD5AXTAPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZVGU4TEMZWGM32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4755923637>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BSNSKPNOF67U2XJXKV7ANTL5AXTAPAVCNFSNUABGKJSXA33TNF2G64TZHMYTCNBXHE3DCMZZHA5US43TOVSTWNBWHAYTSNZQGQ2DTILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BSNSKPPRRRZN4XVHNNYDDXL5AXTAPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZVGU4TEMZWGM32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/BSNSKPLKNES6UALDV2N5A735AXTAPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZVGU4TEMZWGM32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…ollow-up rmyndharis#293) Addresses the review on rmyndharis#293: - Move the auto-dismiss setTimeout OUT of the setToasts updater. State updaters must be side-effect-free; under React Strict Mode they run twice, which would schedule two timers (and could fire for a toast that was never committed). Compute the id first, guard inside the updater, schedule the timer after. - De-dupe on a stable `CONNECTION_LOST_DEDUPE_KEY` constant instead of the displayed title, so translating the title can never silently break de-duplication. - i18n the "Server Connection Lost" title + message via a new `toast.connectionLost` key across all 8 bundled locales (es.json, added later on main, falls back to en until keyed). - Extract the duplicated toast-id generation into a createToastId() helper.
|
Hi @quinton-8 — thanks again for this, the connection-error de-dupe is a real UX win. Since we're lining up the next release and the branch hadn't been updated, I pushed a small follow-up commit to your branch to close out the two review items (hope that's alright — your authorship stays on the PR):
Build + lint are green. I left the optional generic |
error notifications on the right side of the screen when the server is stopped.
…ollow-up rmyndharis#293) Addresses the review on rmyndharis#293: - Move the auto-dismiss setTimeout OUT of the setToasts updater. State updaters must be side-effect-free; under React Strict Mode they run twice, which would schedule two timers (and could fire for a toast that was never committed). Compute the id first, guard inside the updater, schedule the timer after. - De-dupe on a stable `CONNECTION_LOST_DEDUPE_KEY` constant instead of the displayed title, so translating the title can never silently break de-duplication. - i18n the "Server Connection Lost" title + message via a new `toast.connectionLost` key across all 8 bundled locales (es.json, added later on main, falls back to en until keyed). - Extract the duplicated toast-id generation into a createToastId() helper.
eaaf85f to
44c72d9
Compare
Modify the toast notification system to prevent spamming multiple error notifications on the right side of the screen when the server is
stopped.
Description
Brief description of changes
Type of Change
Checklist
Screenshots (if applicable)
Related Issues
Closes #