-
Notifications
You must be signed in to change notification settings - Fork 74
1892 - Don't save message in datastore if payload is missing #1937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses issue #1892 by preventing messages without a payload from being stored in the datastore when they contain only control properties like msg.ui_update, msg.enabled, or msg.visible. The fix prevents these empty payloads from being replayed on browser refresh, which would otherwise clear the display value.
Key Changes:
- Added a check to skip datastore.save() when msg.payload is undefined in the default onInput handler
- Control properties (ui_update, enabled, visible) continue to be handled separately via statestore as before
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (typeof msg.payload !== 'undefined') { | ||
| datastore.save(n, widgetNode, msg) | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change creates an inconsistency with widgets that have custom onInput handlers. For example, ui_switch.js (lines 58-60) explicitly saves to datastore when msg.payload === undefined with the comment "may be setting class dynamically or something else that doesn't require a payload". Since the purpose of this fix is to prevent saving messages without payloads (to avoid clearing displays on refresh), consider whether widgets with custom onInput handlers like ui_switch should also skip saving when payload is undefined for consistency.
| if (typeof msg.payload !== 'undefined') { | ||
| datastore.save(n, widgetNode, msg) | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change lacks test coverage for the specific scenario it's fixing: messages with msg.ui_update, msg.enabled, or msg.visible but no msg.payload should not be saved to the datastore. Consider adding a test that:
- Sends a message with only
msg.ui_update(ormsg.enabled/msg.visible) and no payload - Verifies that
datastore.save()is NOT called for this message - Verifies that the state properties are still set correctly via
statestore.set() - Confirms that on browser refresh, the display value is not cleared
This would ensure the fix works as intended and prevent regression.
|
It is certainly possible that widgets with custom onInput handlers could benefit from this change. This would require further analysis. I do know that the change here is required in order for ui_update to be used with the chart node. Perhaps a new issue should be raised to consider whether those with custom onInput handlers need the change, in order not to hold up the fix for the chart node. I don't know how to implement a test of the type described. |
|
Hey Colin, just FYI, I ran copilot on some of the PRs to get a head start on when I get around to reviewing (hopefully before next week). Dont worry if some of the copilot suggestions are nonsense - but I appreciate you reading them and considering and responding (should speed things up) |
Description
In ui_base.js onInput() each message is stored in the datastore before being sent to the client. This causes a problem if the message contains only msg.ui_update, msg.enabled, or msg.visible, so does not contain a msg.payload value for display. In those cases, if the message is stored and then the browser is refreshed, the empty payload is sent to the client, causing the display value to be cleared. Note that ui_update, enabled and visible are handled separately via the state store so do not need to be replayed.
This PR simply prevents the message with no payload from being stored in the data store.
I have looked through all the node types and cannot see any situations in which an empty payload should be replayed, apart from the Chart node, which supplies its own onInput() method which is run first, so the payload will no be empty by the time this code is run.
However, I cannot be certain that there will be no side effects of this change.
Related Issue(s)
#1892
Checklist
flowforge.yml?FlowFuse/helmto update ConfigMap TemplateFlowFuse/CloudProjectto update values for Staging/ProductionLabels
area:migrationlabel