ui-notification retains and emits properties from all previous messages

[gemini86]

Current Behavior

When sending a msg to ui-notification configured with manual confirmation/dismissal, any property that's set aside from msg.payload or msg.topic is retained and emitted any time the confirm or dismiss buttons are clicked, even when the incoming message that triggered the notification does not have that property.

Expected Behavior

The output that's emitted from a notification when a confirm or cancel button is clicked should only contain the msg that was sent to it, with msg.payload modified per the node help info.

Steps To Reproduce

Example flow:

Click the first inject node, then click confirm or close on the dashboard notification, then click the second inject node and click confirm or close and see that the extra properties from the first inject are being emitted every time. Removing the node and replacing the node does not help.

[{"id":"fcf752aa7785e4fa","type":"inject","z":"df36d1113fdcc60e","name":"click first","props":[{"p":"payload"},{"p":"topic","vt":"str"},{"p":"extraProp","v":"foo bar","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"hello world","payloadType":"str","x":560,"y":660,"wires":[["9c5952c1756c5cb9"]]},{"id":"84bdee650d1a43df","type":"inject","z":"df36d1113fdcc60e","name":"click second","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"hello world","payloadType":"str","x":550,"y":700,"wires":[["9c5952c1756c5cb9"]]},{"id":"a16d019f36a00697","type":"debug","z":"df36d1113fdcc60e","name":"debug 4","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"true","targetType":"full","statusVal":"","statusType":"auto","x":940,"y":680,"wires":[]},{"id":"9c5952c1756c5cb9","type":"ui-notification","z":"df36d1113fdcc60e","ui":"4021fffb356dc9de","position":"center center","colorDefault":true,"color":"#000000","displayTime":"","showCountdown":true,"outputs":1,"allowDismiss":true,"dismissText":"Close","allowConfirm":true,"confirmText":"Confirm","raw":false,"className":"","name":"","x":750,"y":680,"wires":[["a16d019f36a00697"]]},{"id":"4021fffb356dc9de","type":"ui-base","name":"My Dashboard","path":"/dashboard","appIcon":"","includeClientData":true,"acceptsClientConfig":["ui-notification","ui-control"],"showPathInSidebar":false,"navigationStyle":"default","titleBarStyle":"default"}]

Environment

• Dashboard version: 1.22.1
• Node-RED version: 4.0.8
• Node.js version: 20.18.1
• npm version: 10.8.2
• Platform/OS: Ubuntu 24.04.1 LTS
• Browser: chrome

Have you provided an initial effort estimate for this issue?

I am not a FlowFuse team member

[bartbutenaers]

Hi @gemini86,

Thanks for sharing a simple example flow, because that allows people to quickly reproduce it.
I can confirm that this is indeed a bug:

I did quite a lot of changes to the ui-notification node last year, but - although it indeed really should be fixed- I am not tempted to start digging into the code to solve this one. I will try to explain below why...

The old AngularJs dashboard contained a mechanism called message replay, which replays the last input message as soon as you e.g. refresh your browser window. That way your dashboard in the browser is updated to show the state from the last message content. While that sounds like a good approach, it caused a LOT of headache for ui node developers in the past. Unfortunately the message replay mechanism has also been used in this new dashboard D2, and again it keeps causing issues like this one you reported. I could fix the code to solve your problem, but that would be nothing more than a nasty workaround. By keep continuing that way, the entire dashboard code becomes more and more polluted with dirty workarounds. As a result the dashboard code becomes very hard to maintain, and the chance for regression issues increases a lot.

Imho the only decent solution is to completely remove the entire message replay mechanism from the internals of this dashboard. But that is a lot of work. And at the moment I am the only one from our entire community that is willing to contribute to this repository, so that certainly will not happen in my limited free time. Hopefully Joe (who is very busy at the moment with non-dashboard stuff) has some time in the near future to have a look at that. He can always give me a call for a meet and greet to discuss all of this ;-)

Hopefully that gives you enough context to understand the root of the problem.
Bart

[gemini86]

Understandable. My company is a FlowFuse subscriber, so my hope is that since they're heavily developing dashboard-2 for their commercial efforts, it could see more attention than just volunteers. I am more than willing to contribute, since I'm using this for a work project, but I also don't have much free time. For now, it seems the best work-around is to NOT reuse notification nodes where the presence of other msg properties may vary.

If you're able to point me to a style guide or other needed documentation in order to effectively contribute, I'll look at submitting some pull requests.

[bartbutenaers]

it could see more attention than just volunteers

Yes sure they do!! Allmost all of the code in this repo is contributed by Flowfuse. It is only recently that they are busy with other things, so developments here are currently lower. But that will be a temporary hickup. But anyway it would be nice to get some extra help from our community...

If you're able to point me to a style guide or other needed documentation

Here you can find the contribution documentation. But you can also ask for extra info here, and I/we will try to assist as much as possible.

I'll look at submitting some pull requests.

I assume you are talking about a workaround to fix the ui-notification node? If you need some guidance, please ask and I will try to put you on track if needed.

[gemini86]

I assume you are talking about a workaround to fix the ui-notification node? If you need some guidance, please ask and I will try to put you on track if needed.

For this specific task, yes. I'm already diggin into because my ADHD won't let me stop right now, I see the issue in data.js in the bind mutation:

bind (state, data) {
        const widgetId = data.widgetId
        // if packet contains a msg, then we process it
        if ('msg' in data) {
            // merge with any existing data and override relevant properties <----[why are we doing this?]
            state.messages[widgetId] = {
                ...state.messages[widgetId],
                ...data.msg
            }
        }
    },

I'm sure there's some reason we need to merge the incoming message with the existing one in the store for each widget? Changing this would obviously impact ALL widgets, so I'm inclined to think this would be a breaking change but I'll look into that tomorrow. If you or anyone else have any insight, it would be greatly appreciated.

[gemini86]

But for now I can confirm in my testing that this fixes the issue and my ADHD can let me rest:

bind (state, data) {
    const widgetId = data.widgetId
    // if packet contains a msg, then we process it
    if ('msg' in data) {
        // overwrite old msg
        state.messages[widgetId] = {
            ...data.msg
        }
    }
},

Who knows what can of worms it opens up...

[bartbutenaers]

Ah ok. Indeed if you want to solve it at that point in the code, then indeed you are opening the famous can of worms. Be aware that those messages - stored in the state store - are also emitted to the frontend.

You hit the point where all troubles started. There are 2 separate stores unfortunately in dashboard D2: the statestore (which I like) and that datastore (which I don't like). There have been already a few good refactorings in Q4 of last years, to tackle issues with that datastore, but it is still there.

1. The datastore is to store the "data" of the ui-node, which is different for each node: for example for a ui-switch that is the state of the switch (ON or OFF). That data is injected via the msg.payload. So in fact only the payload should be stored in the datastore, when a message is being inject. However from day 1 the entire messages have been stored, which makes no sense at all because all other data (e.g. msg.ui_update stuff) is already stored in the statestore. That has already been 'patched' to remove some of the data from the stored messages.
2. Moreover a ui-node only has 1 single state: e.g. the button is ON or OFF. However currently the last N states are stored, i.e. the last N messages. That makes no sense. Moreover e.g. for my ui-svg node the state is created via a large number of messages their payload: e.g. a floorplan of your house contains lots of lights that are separately turned ON or OFF via separate messages. So I should merge N messages to "reconstruct" the state again. But you cannot store infinite number of messages. Moreover when you turn a light ON in the floorplan, and then OFF again. Then 2 messages are store, but the oldest one makes no sense anymore because the information inside its payload is obsolete.

That is what I mean: the datastore should only contain a single state, which should be constructed at the server-side (based on the information in the successive message payloads) and then synced to the clients. Not simply store the N latest messages, because that makes no sense and results in all kind of troubles.

But yes your patch might indeed be an improvement for your single use case. However I have no idea whether e.g. third-party ui nodes (from other developers) make use of the other information (e.g. msg.topic) in their code. That might indeed cause breaking changes unfortunately. And you should look in all the vue files of all widgets inside this dashboard, to find breaking changes in the core.

Hopefully you understand now why the store/replay of messages seems like a brilliant (Node-RED compliant) idea, but isn't at all...

[gemini86]

Thank you for the time to explain the different functions of the stores. Could you point me to the state store code? I would like to look at the implementation, but from what you're describing, it still doesn't make any sense from a functional point of view to only merge in new messages without clearing any other previously set properties. Here are some points I'd like to argue if I may, not a criticism but just some feedback as a long time (node-red 0.xx) user:

• Implicit behavior is usually not a good thing. Even if we're trying to store the current state of the widget, I'd argue it's better to let the new input msg flush out the old and set the new current state. In my opinion, it makes much more sense to make everything explicitly defined and intentional. e.g., in dashboard 1, where setting msg.highlight on a ui-notification would only take effect if the configured border color was left empty and only applied when the incoming message had that msg property set to a valid color. This is what I've come to expect from any node, built-in or otherwise, so I think it's more than reasonable to expect other devs to do things in a "node-red" conventional way.
• Alternatively, if we've established that msg.ui_control is the de facto method for changing a widget’s dynamic properties, and we also want msg.ui_update to persist between new messages where this property isn’t defined, then we should explicitly store msg.ui_update and have users clear it to default configuration settings with an empty object (like clearing a chart node).

I have a similar but different datastore issue with UIform, where it doesn't even store the input msg into the state at all, only the payload (and topic if you configure it to do so), so when the submit button is pressed, all other msg properties that you had wanted to pass through are dropped. So you see how it's more an issue of code consistency and variability of intentions between nodes, which is why I asked if there was a style guide or code document to reference.

For now I will study the rest of the code and other widgets to see what my changes do to alter their behavior.

[bartbutenaers]

You are welcome!

Could you point me to the state store code?

Here you can find the code for both stores.

it still doesn't make any sense from a functional point of view to only merge in new messages without clearing any other previously set properties

The state store is more or like you describe. But the data store not: it is just a store to keep track of the latest N messages. Perhaps you can find a bit more informating about the merging in this pull request.

not a criticism but just some feedback

We can only learn from 'constructive' feedback like yours...
Perhaps I will give you some history of the dashboard D2 evolution, so you perhaps a bit more some of the decisions made by the core team members. In the beginning you e.g. injected msg.payload=true to put a switch ON. But then a message was injected containing no payload, but only e.g. a msg.ui_update.color or whatever to change the color (without setting a value). However that color was stored in the statestore, while the message (without payload) was stored in the datastore. Moreover that way the message arrived in the frontend as last message: because there was no payload the switch was turned OFF without reason. To solve that kind of issues, a number of pull requests have been created.

I'd argue it's better to let the new input msg flush out the old and set the new current state.

Yes that is indeed a bit into the direction I was pointing to. You don't need to store the N latest messages, but imho only keeping the last message is not enough. Because then you assume that the last message contains the entire state. That is only correct for simple ui nodes, e.g. for a simple switch, led, ... However that does not work for other nodes:

• As I explained above, in my ui-svg node the last message only contains a partial state, e.g. the state of one of N leds displayed in a floorplan.
• Or look at the ui-chart node:
  • When you inject a "replace" message, the payload contains an array of all the datapoints for a chart. In that case it is enough to store the last payload (not message!), because that last message contains the entire state of the chart.
  • When you inject an "append" message, the payload contains e.g. a single datapoint that needs to be appended to the chart. In that case it is not enough to store the last message, because that only contains a partial state. Then you need to merge the last N messages to get the full state.

That is why I say that the data store should not simply store the last message or last N messages. Or last payload or last N payloads. Instead the datastore should keep track of the entire state, which it needs to create based upon messages that are injected containing the full or partial state. How the full state is constructed based on payloads from input messages, is up to every ui node itself. When you refresh your browser window or when a new client connects, that current (entire) state can simply be passed to it and visualized.

where it doesn't even store the input msg into the state at all

Yes indeed keeping the last message somewhere is very useful when it needs to be resend to the output. But imho not for sending it to the frontend via the datastore...

[gemini86]

Ah, I understand. Again, my argument is to ask the user to send messages with explicit intent.

In your SVG node example; if I am sending in a msg.payload with lots of properties for different indicators and only want to update individual properties one at a time as they change, I don't expect the widget to manage the state of all properties for me, I expect to need a helper to join individual messages from my flows to form a single payload and send the entire payload each time I update the widget. There are a few apps I've built with D1 where I must do just that. In my opinion, it's a more proper separation of node roles.

In the chart node example, the configuration of the node should dictate what is done with the payload unless there's a dynamic input (msg.action) to override it. To me, the 'dynamic' input would not suggest that the change would become persistent.

Say I set msg.action to 'append' and then send a payload. I expect that data to be appended, the widget should append the data to the existing state. If I then send a new message with no msg.action property, I do not expect the node to continue to append forever until I change it back, I expect the default configuration to take over in the absence of a 'dynamic' change to msg.action. Because, at some point a user is going to forget that they once sent a msg.action long ago that will forever change how the chart functions in spite of the configuration. That would be incredibly difficult situation to troubleshoot and hard to track.

Back to the topic of this ui-notification node: here's another flow snippet example where a user would want to disable the confirm and dismiss buttons for a particular notification, but then expect the node to go back to its default configuration, since it's not expressed in the help file that it wouldn't. The current behavior causes the user to have to explicitly define every msg.ui_update in every msg sent to that node in the rest of my flow, which defeats the purpose of a node configuration page.

[{"id":"9165145af7de9fda","type":"ui-notification","z":"7145785939960fd4","ui":"4021fffb356dc9de","position":"center center","colorDefault":true,"color":"#000000","displayTime":"5","showCountdown":true,"outputs":1,"allowDismiss":true,"dismissText":"Close","allowConfirm":true,"confirmText":"Confirm","raw":false,"className":"","name":"","x":850,"y":440,"wires":[[]]},{"id":"b585fad96998bf90","type":"ui-button","z":"7145785939960fd4","group":"1de8fcdb9497ebed","name":"","label":"first click, default config has buttons","order":1,"width":0,"height":0,"emulateClick":false,"tooltip":"","color":"","bgcolor":"","className":"","icon":"","iconPosition":"left","payload":"hello choose a button pls","payloadType":"str","topic":"topic","topicType":"msg","buttonColor":"","textColor":"","iconColor":"","enableClick":true,"enablePointerdown":false,"pointerdownPayload":"","pointerdownPayloadType":"str","enablePointerup":false,"pointerupPayload":"","pointerupPayloadType":"str","x":320,"y":380,"wires":[["9165145af7de9fda"]]},{"id":"92523577a0f258de","type":"ui-button","z":"7145785939960fd4","group":"1de8fcdb9497ebed","name":"","label":"now disable buttons","order":2,"width":0,"height":0,"emulateClick":false,"tooltip":"","color":"","bgcolor":"","className":"","icon":"","iconPosition":"left","payload":"aha! I have removed the buttons for this message","payloadType":"str","topic":"topic","topicType":"msg","buttonColor":"","textColor":"","iconColor":"","enableClick":true,"enablePointerdown":false,"pointerdownPayload":"","pointerdownPayloadType":"str","enablePointerup":false,"pointerupPayload":"","pointerupPayloadType":"str","x":360,"y":420,"wires":[["cebe2dfb2d09f814"]]},{"id":"cebe2dfb2d09f814","type":"change","z":"7145785939960fd4","name":"","rules":[{"t":"set","p":"ui_update","pt":"msg","to":"{\"allowDismiss\":false,\"allowConfirm\":false}","tot":"json"}],"action":"","property":"","from":"","to":"","reg":false,"x":590,"y":420,"wires":[["9165145af7de9fda"]]},{"id":"52aff927a0acc220","type":"ui-button","z":"7145785939960fd4","group":"1de8fcdb9497ebed","name":"","label":"now back to a normal msg, no ui_update","order":3,"width":0,"height":0,"emulateClick":false,"tooltip":"","color":"","bgcolor":"","className":"","icon":"","iconPosition":"left","payload":"New message, no ui_upodate... wait, the default config is buttons, but where are they?","payloadType":"str","topic":"topic","topicType":"msg","buttonColor":"","textColor":"","iconColor":"","enableClick":true,"enablePointerdown":false,"pointerdownPayload":"","pointerdownPayloadType":"str","enablePointerup":false,"pointerupPayload":"","pointerupPayloadType":"str","x":300,"y":460,"wires":[["9165145af7de9fda"]]},{"id":"9bca02a249c3e8ca","type":"ui-button","z":"7145785939960fd4","group":"1de8fcdb9497ebed","name":"","label":"oh I have to explicitely re-enable them.","order":4,"width":0,"height":0,"emulateClick":false,"tooltip":"","color":"","bgcolor":"","className":"","icon":"","iconPosition":"left","payload":"are they back?","payloadType":"str","topic":"topic","topicType":"msg","buttonColor":"","textColor":"","iconColor":"","enableClick":true,"enablePointerdown":false,"pointerdownPayload":"","pointerdownPayloadType":"str","enablePointerup":false,"pointerupPayload":"","pointerupPayloadType":"str","x":310,"y":500,"wires":[["f3ae9e0d11f539bd"]]},{"id":"f3ae9e0d11f539bd","type":"change","z":"7145785939960fd4","name":"","rules":[{"t":"set","p":"ui_update","pt":"msg","to":"{\"allowDismiss\":true,\"allowConfirm\":true}","tot":"json"}],"action":"","property":"","from":"","to":"","reg":false,"x":590,"y":500,"wires":[["9165145af7de9fda"]]},{"id":"4021fffb356dc9de","type":"ui-base","name":"My Dashboard","path":"/dashboard","appIcon":"","includeClientData":true,"acceptsClientConfig":["ui-notification","ui-control"],"showPathInSidebar":false,"navigationStyle":"default","titleBarStyle":"default"},{"id":"1de8fcdb9497ebed","type":"ui-group","name":"Click these in order","page":"57178351a7a84167","width":6,"height":1,"order":1,"showTitle":true,"className":"","visible":"true","disabled":"false","groupType":"default"},{"id":"57178351a7a84167","type":"ui-page","name":"Page 1","ui":"4021fffb356dc9de","path":"/page1","icon":"home","layout":"grid","theme":"f46780954aa8bb84","breakpoints":[{"name":"Default","px":"0","cols":"3"},{"name":"Tablet","px":"576","cols":"6"},{"name":"Small Desktop","px":"768","cols":"9"},{"name":"Desktop","px":"1024","cols":"12"}],"order":1,"className":"","visible":"true","disabled":"false"},{"id":"f46780954aa8bb84","type":"ui-theme","name":"Default Theme","colors":{"surface":"#ffffff","primary":"#0094ce","bgPage":"#eeeeee","groupBg":"#ffffff","groupOutline":"#cccccc"},"sizes":{"density":"default","pagePadding":"12px","groupGap":"12px","groupBorderRadius":"4px","widgetGap":"12px"}}]

This is quite the opposite of what users of D1 expected nodes to do when setting dynamic properties, hence the frustration when trying to migrate to D2 and find all these undocumented and unexpected behaviors.

---

Also, regarding a node's output; the help page should tell the user what to expect on the output. Many of the D2 nodes don't follow the node-help style guide or provide all the needed information such as the expected output. In the D1 chart node, it's always been the 'state' of the chart as it, so you can store it in a persistent storage external to the node for reloading.

Again, I really appreciate the information and I enjoy discussing, so thank you for your time and I hope my rambling isn't coming off as ungrateful. I'm not experienced enough to make all the contributions I feel are needed, and I also see a need for more group discussion to come to a consensus on how nodes should behave before any of these types of changes can happen, but I do think they need to happen before I can fully adopt D2 for my own production use.

[bartbutenaers]

it's a more proper separation of node roles.

Yes that is indeed something I have been considering over and over again in the past. Because users where injecting weeks after weeks messages into their floorplan to update the state of their devices. And then suddenly Node-RED was restarting and all their state was lost.

I had a lot of feedback and discussions with users about that all over the years. They tried to find all kinds of workarounds, but I have never seen one that did the job completely. It is rather complex. You could e.g. add a css class "light" to evey light bulb icon in your floorplan. Then you can turn all lights on in the floorplan by injecting a message with that class as CSS selector. But then some light is turned off, you inject a message to turn 1 light bulb icon off. And when you start animations for some elements it becomes even more complicated, because these e.g. might have a finite length duration. So when meanwhile another client dashboard is opened, it should only see the remaining part of the animation. And so on...

That is what I mean that each ui node should have its own way to update its own state in the dashboard, in case of the ui-svg node the best way seemed to be at the end to maintain a single server-side lightweight DIM tree with CSS selector support. Believe me, LOTs of ideas have already been reviewed in the past...

But if you have any ideas, please share them!

I expect the default configuration to take over in the absence of a 'dynamic' change

That indeed makes sense yes, to me at least...

This is quite the opposite of what users of D1 expected nodes to do when setting dynamic properties

Ah I had already forgotten how D1 worked. Yes indeed via the dynamic messages the properties from the config screen are overwritten from then on with that new value. Quite some time ago, I already had some ideas about persistent and volatile properties: see discussion. I am completely not claiming that all my ideas are great, but we did discuss it quite a lot. However the dashboard at that moment was already grown quite a lot, so it became more and more difficult to introduce such large changes to the core (without breaking everything). Moreover it is easy to talk about it, but somebody needs to have time to implement it...

the help page should tell the user what to expect on the output

Yes I totally agree about that too. But to be very honest: if you put a debug on the output, then you see what comes out. And then you know which information is missing in the documentation. And most of the people in our community are intelligent enough to contribute such textual changes to the documentation of this repo. However I see nearly nobody contributing changes to the documentation. Only a lot of frustrated people pointing their finger at a few guys, which they expect to do it all for free for them. That way I can assure it will never happen...

Again, I really appreciate the information and I enjoy discussing

Unlike a lot of other discussions I had (unfortunately) recently, I consider this one as constructive. However I am not sure if it is still possible to implement such a large change as you suggest, although I also think it is really needed to do it. But would be a lot of work, and I am afraid a lot of breaking changes would be involved. Especially since there are now also custom ui nodes available in the wild. I have too less knowledge about the core and Vue to put a solution on the table unfortunately...

BTW this might also be interesting for you.

[gemini86]

Thank you for linking my concerns on that issue. I can see as well that the API doesn't need to change, just how nodes are calling it (although it would be nice if there was a method for resetting all dynamic properties at once). For instance, ui-notification calls this.updateDynamicProperty() when there's a msg.ui_update present, but never defaults back to the node configuration until a redeploy of all nodes or a msg.ui_update updates it again.