fix(ktoaster): prevent creating duplicate containers [KHCP-18535] #3017

portikM · 2025-12-05T14:34:47Z

Summary

Addresses: https://konghq.atlassian.net/browse/KHCP-18535

Second attempt at preventing ToastManager from creating duplicate containers. Fixing issues introduced in #2980 and reverted in #3015.

netlify · 2025-12-05T14:34:53Z

✅ Deploy Preview for kongponents-sandbox ready!

Name	Link
🔨 Latest commit	`367b6ad`
🔍 Latest deploy log	https://app.netlify.com/projects/kongponents-sandbox/deploys/69664f0fac71300008bb03fb
😎 Deploy Preview	https://deploy-preview-3017--kongponents-sandbox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

netlify · 2025-12-05T14:36:39Z

✅ Deploy Preview for kongponents ready!

Name	Link
🔨 Latest commit	`367b6ad`
🔍 Latest deploy log	https://app.netlify.com/projects/kongponents/deploys/69664f0f2c12eb0007323ece
😎 Deploy Preview	https://deploy-preview-3017--kongponents.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

johncowen · 2026-01-08T11:31:32Z

hey @portikM 👋 , sorry I know you are up the wall with other stuff at the moment, if you get a sec could you add a bit of detail in the description here what the "issues introduced" were exactly, thanks!

Second attempt at preventing ToastManager from creating duplicate containers. Fixing issues introduced in #2980 and reverted in #3015.

It looks like this PR is just a direct copy of the reverted one, and from that I guess you are planning on working on top of the reverted one is that understanding correct?

portikM · 2026-01-08T16:44:54Z

hey @portikM 👋 , sorry I know you are up the wall with other stuff at the moment, if you get a sec could you add a bit of detail in the description here what the "issues introduced" were exactly, thanks!

Second attempt at preventing ToastManager from creating duplicate containers. Fixing issues introduced in #2980 and reverted in #3015.

It looks like this PR is just a direct copy of the reverted one, and from that I guess you are planning on working on top of the reverted one is that understanding correct?

Hi @johncowen! Yes I had to deprioritize this for a bit but I am picking this back up today. The issue with this implementation came up in this thread. I didn't have time to identify exactly why, but essentially the toast messages weren't showing sometimes, our regression tests caught it. I still think this approach is good, just need to debug and fix the issue.

…tainers-1

johncowen · 2026-01-09T10:51:39Z

Thanks Max!

Not sure if this helps, but we had a quick look at this yesterday and noticed that there's an opportunity for unintended behaviour here if you have 2 individual ToastManagers. One example is:

Given 2 ToastManagers:

open is called on ToastManager 1
time passes
open is called on ToastManager 2
a tick passes
destroy is called from ToastManager 1
The toast from ToastManager 2 is removed from the DOM and was only "live" for a tick - i.e. it was never shown

I think the issue is worse on the current code on main (previous to this PR) because the DOM target it created on construction rather than open - but still I saw that some tests are expecting toasts to be in the DOM and they are failing due the tests not being able to find the toast. While this could explain the problem there, it doesn't explain why your upstream application tests were fine beforehand, unless I'm missing something 🤔

We found only two instances (in upstream applications aside from kuma) where the toast managers being created actually have destroy called on them. FYI: in kuma, we are not currently calling destroy at all but "ideally" things would get cleaned up properly.

I guess if we don't want to go down the lifecycle tracking route, another option would be to just provide the toast manager from a single "shell" application instead of having individual applications instantiate a toast manager themselves. Application would then access the manager provided by the "shell" and the shell would be responsible for cleaning up. Of course if we aren't super precious about cleaning up (after all we want the toast manager to be available throughout the lifetime of the app) we could skip that (ideally we wouldn't skip cleanup, but maybe pragmatism wins here if we don't want to manage lifecycle)

This option would solve it for our specific needs, but obvs not for others (not sure how many other folks are encountering this issue with duplicate toast managers) - I guess a "fix" there would be to document that ToastManager should be implemented as a singleton in your application and then destroy should only be called once you are sure all toast managers are "finished" (if at all) to prevent these sorts of shared state issues.

Anyway, thought I'd share our findings/thoughts from yesterday, none of the above are blocking asks or anything

portikM · 2026-01-09T13:47:50Z

Thanks Max!

Not sure if this helps, but we had a quick look at this yesterday and noticed that there's an opportunity for unintended behaviour here if you have 2 individual ToastManagers. One example is:

Given 2 ToastManagers:

open is called on ToastManager 1

time passes

open is called on ToastManager 2

a tick passes

destroy is called from ToastManager 1

The toast from ToastManager 2 is removed from the DOM and was only "live" for a tick - i.e. it was never shown

I think the issue is worse on the current code on main (previous to this PR) because the DOM target it created on construction rather than open - but still I saw that some tests are expecting toasts to be in the DOM and they are failing due the tests not being able to find the toast. While this could explain the problem there, it doesn't explain why your upstream application tests were fine beforehand, unless I'm missing something 🤔

We found only two instances (in upstream applications aside from kuma) where the toast managers being created actually have destroy called on them. FYI: in kuma, we are not currently calling destroy at all but "ideally" things would get cleaned up properly.

I guess if we don't want to go down the lifecycle tracking route, another option would be to just provide the toast manager from a single "shell" application instead of having individual applications instantiate a toast manager themselves. Application would then access the manager provided by the "shell" and the shell would be responsible for cleaning up. Of course if we aren't super precious about cleaning up (after all we want the toast manager to be available throughout the lifetime of the app) we could skip that (ideally we wouldn't skip cleanup, but maybe pragmatism wins here if we don't want to manage lifecycle)

This option would solve it for our specific needs, but obvs not for others (not sure how many other folks are encountering this issue with duplicate toast managers) - I guess a "fix" there would be to document that ToastManager should be implemented as a singleton in your application and then destroy should only be called once you are sure all toast managers are "finished" (if at all) to prevent these sorts of shared state issues.

Anyway, thought I'd share our findings/thoughts from yesterday, none of the above are blocking asks or anything

Thanks for sharing your findings jc! Yeah I guess once we get this working there is a slight chance of mishap when 1 instance of ToastManager is being destroyed at the exact moment when another dispatches a toast. On the other hand, once this finalized and ToastManager only mounts one kongponents-toaster-container, do we really need to remove the element from the DOM when destroying an instance? 🤔 There's nothing wrong with leaving it there since we know the next created instance of ToastManager won't mount a new element.

johncowen · 2026-01-09T14:25:42Z

do we really need to remove the element from the DOM when destroying an instance? 🤔 There's nothing wrong with leaving it there since we know the next created instance of ToastManager won't mount a new element.

Yeah this is what we were wondering also 👍 . I guess for us this is fine, but maybe we need to explain this in the docs that if you want to destroy you should only do it when you are sure you application has totally finished (I guess some folks/usecase may want to ensure this happens?). This is a total "maybe we should add this docs" from me, will leave that decision up to you

johncowen · 2026-01-09T14:26:52Z

Ah and we'd also have to communicate and change this in the two places we found it, if you need help finding ping me offline 👍

We found only two instances (in upstream applications aside from kuma) where the toast managers being created actually have destroy called on them

portikM · 2026-01-09T15:24:47Z

do we really need to remove the element from the DOM when destroying an instance? 🤔 There's nothing wrong with leaving it there since we know the next created instance of ToastManager won't mount a new element.

Yeah this is what we were wondering also 👍 . I guess for us this is fine, but maybe we need to explain this in the docs that if you want to destroy you should only do it when you are sure you application has totally finished (I guess some folks/usecase may want to ensure this happens?). This is a total "maybe we should add this docs" from me, will leave that decision up to you

Yeah totally. I was thinking we could add an optional attribute in destroy method, something like removeToastersContainer: boolean = false and document it's usage. What do you think?

Update: on the second thought, all destroy method does is removes the container - when we skip removing it there's no reason to call it at all. Maybe we could document that calling the method is optional and they should only call it when application has totally finished like you suggested.

johncowen · 2026-01-09T15:34:57Z

Update: on the second thought, all destroy method does is removes the container - when we skip removing it there's no reason to call it at all. Maybe we could document that calling the method is optional and they should only call it when application has totally finished like you suggested.

Yeah agree 👍

We should chase up internally those apps that are calling destroy also.

(P.S. I'm still on the side of "ideally" we'd clean up, but considering everything I think this approach is the best way forwards)

portikM · 2026-01-09T18:25:03Z

src/components/KToaster/ToastManager.ts

+   * Destroys the ToastManager instance and removes the toasters container element from the DOM
+   * @param removeToastersContainer - Whether to remove the toasters container element from the DOM (defaults to false)
+   */
+  public destroy(removeToastersContainer: boolean = false) {


In Konnect where we can have multiple ToastManager on the same view, removing the toasters container element from the DOM can [very unlikely but can] lead to a race condition when one instance removes the container at the same moment as another instance is attempting to dispatch a notification.

Because a toaster instance will not create a new container if one already exists, I think it's no big deal if we just have the toasters container element in the DOM?

Original conversation

Would it make sense to cover this in the tests by adding an assertion for making sure that the DOM element is not being removed after calling destroy with the default param (or setting it to false)? 🙂

I think there's also a theoretical bug here also.

If I call destroy and then destroy(true) I don't think the DOM container will get cleaned up:

destroy() > this.toastersContainer = null

destroy(true) this.toastersContainer && removeToastersContainer > null && true === false

Both good points, pushed a fix for both.

Probably I wasn't clear enough, but there is still a missing test/assertion for the case of calling destroy() and that the container still exists. There is only an assertion that the container is removed after calling destroy(true). 🙂

Oh yup my miss. Added a test scenario for that now.

portikM · 2026-01-09T18:28:57Z

Update: on the second thought, all destroy method does is removes the container - when we skip removing it there's no reason to call it at all. Maybe we could document that calling the method is optional and they should only call it when application has totally finished like you suggested.

Yeah agree 👍

We should chase up internally those apps that are calling destroy also.

(P.S. I'm still on the side of "ideally" we'd clean up, but considering everything I think this approach is the best way forwards)

@johncowen fixed my implementation and added a note for what we talked about in the docs. This PR now ready for review.

adamdehaven

question: is there any impact to the Nuxt module and the composable it exposes? (@arashsheyda @portikM )

adamdehaven · 2026-01-09T18:36:38Z

docs/components/radio.md

 />
 ```

+<KRadio card v-model="errorPropRadio" label="Input error" error description="Some description text" :selected-value="false" />


FYI: This is unrelated but a valid docs example we requested 👍🏼

portikM · 2026-01-09T18:58:05Z

question: is there any impact to the Nuxt module and the composable it exposes? (@arashsheyda @portikM )

I would imagine there's none but I can double check.

arashsheyda · 2026-01-09T19:21:17Z

I don't think it it owuld effect at all, but also if it makes it easier we could setup a mini nuxt sandbox here for easier testing

adamdehaven · 2026-01-13T05:30:25Z

src/components/KToaster/ToastManager.ts

+  }
+
+  private setupToastersContainer(): void {
+    const toastersContainerEl = document?.getElementById(toasterContainerId)


Can you pull the preview PR into portal? Just want to make sure this doesn't init on the server

Done: https://github.com/kong-konnect/portal/pull/1880

src/components/KToaster/ToastManager.ts

johncowen

Seeing as I've been involved in the convo a little here this LGTM! (considering we are fine not cleaning up of course)

Thanks for the extra detail in the work here Max 👍

Separately and just out of interest (incase you are interested also) in regards to Adam's comment you have left to address, I implemented an improved guard for SSR over in my old suggestion PR over here https://github.com/Kong/kongponents/pull/3045/changes#diff-2adca4150f4c28f9c8c391a0ed2520d130301583843a5bb1890e526f835a8190R106-R113

Obvs we aren't doing that here, but I wanted to see what it would like. Its quite nice that you only have to guard in one place which is out of the main body of code at creation time, and then you can totally forget (and not worry about) the fact that you might be running in SSR everywhere else.

I quite like fixing things in a single place at the root and not having to use ? and if statements everywhere in multiple places, but maybe thats a personal opinion thing 🤷

## [9.49.5](v9.49.4...v9.49.5) (2026-01-13) ### Bug Fixes * **ktoaster:** prevent creating duplicate containers [KHCP-18535] ([#3017](#3017)) ([0f949e8](0f949e8))

kongponents-bot · 2026-01-13T18:34:37Z

🎉 This PR is included in version 9.49.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

kongponents-bot · 2026-01-13T18:42:00Z

Preview package from this PR in consuming application

In consuming application project install preview version of kongponents generated by this PR:

@kong/kongponents@pr-3017

fix(ktoaster): prevent creating duplicate containers [KHCP-18535]

9d372c9

portikM self-assigned this Dec 5, 2025

portikM added the do not merge label Dec 5, 2025

johncowen mentioned this pull request Jan 6, 2026

fix(ktoaster): prevent duplicate containers via a shared pool #3041

Closed

portikM added 2 commits January 8, 2026 11:59

Merge branch 'main' into fix/khcp-18535-toaster-prevent-duplicate-con…

c66af09

…tainers-1

fix(ktoaster): toast manager reactivity [KHCP-18535]

2ca3a54

fix(ktoaster): revert reactivity fix [KHCP-18535]

1b37985

portikM added 5 commits January 9, 2026 10:45

fix(ktoaster): debugging [KHCP-18535]

04c44f9

fix: minor fix

e32b493

fix: minor fix

28f1f27

fix: minor fix

6801394

test(ktoaster): fix unit test [KHCP-18535]

8bfbb01

johncowen mentioned this pull request Jan 9, 2026

fix(ktoaster): prevent duplicate containers via a shared pool v2 #3045

Closed

portikM added 2 commits January 9, 2026 13:10

docs(toaster): destroy function docs [KHCP-18535]

97910d7

docs(radio): error card example [KHCP-18535]

52f9793

portikM commented Jan 9, 2026

View reviewed changes

portikM removed the do not merge label Jan 9, 2026

portikM marked this pull request as ready for review January 9, 2026 18:29

portikM requested a review from adamdehaven as a code owner January 9, 2026 18:29

portikM requested review from a team, Justineo and jillztom as code owners January 9, 2026 18:29

adamdehaven reviewed Jan 9, 2026

View reviewed changes

fix: address pr feedback

f33988b

portikM requested review from adamdehaven, johncowen and schogges January 12, 2026 17:02

adamdehaven reviewed Jan 13, 2026

View reviewed changes

portikM added 2 commits January 13, 2026 08:49

test(ktoaster): test scenario [KHCP-18535]

db2f98d

fix: minor fix

367b6ad

johncowen approved these changes Jan 13, 2026

View reviewed changes

portikM requested a review from adamdehaven January 13, 2026 14:23

adamdehaven approved these changes Jan 13, 2026

View reviewed changes

portikM merged commit 0f949e8 into main Jan 13, 2026
19 of 20 checks passed

portikM deleted the fix/khcp-18535-toaster-prevent-duplicate-containers-1 branch January 13, 2026 18:27

kongponents-bot pushed a commit that referenced this pull request Jan 13, 2026

chore(release): 9.49.5 [skip ci]

2791711

## [9.49.5](v9.49.4...v9.49.5) (2026-01-13) ### Bug Fixes * **ktoaster:** prevent creating duplicate containers [KHCP-18535] ([#3017](#3017)) ([0f949e8](0f949e8))

kongponents-bot added the released label Jan 13, 2026

fix(ktoaster): prevent creating duplicate containers [KHCP-18535] #3017

fix(ktoaster): prevent creating duplicate containers [KHCP-18535] #3017

Conversation

portikM commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

netlify bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kongponents-sandbox ready!

Uh oh!

netlify bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kongponents ready!

Uh oh!

johncowen commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

portikM commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johncowen commented Jan 9, 2026

Uh oh!

portikM commented Jan 9, 2026

Uh oh!

johncowen commented Jan 9, 2026

Uh oh!

johncowen commented Jan 9, 2026

Uh oh!

portikM commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johncowen commented Jan 9, 2026

Uh oh!

portikM Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

portikM commented Jan 9, 2026

Uh oh!

adamdehaven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

portikM commented Jan 9, 2026

Uh oh!

arashsheyda commented Jan 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johncowen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kongponents-bot commented Jan 13, 2026

Uh oh!

kongponents-bot commented Jan 13, 2026

Preview package from this PR in consuming application

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

portikM commented Dec 5, 2025 •

edited

Loading

netlify bot commented Dec 5, 2025 •

edited

Loading

netlify bot commented Dec 5, 2025 •

edited

Loading

johncowen commented Jan 8, 2026 •

edited

Loading

portikM commented Jan 8, 2026 •

edited

Loading

portikM commented Jan 9, 2026 •

edited

Loading

portikM Jan 9, 2026 •

edited

Loading