Bug description
When editing a Handlebars chart, if you modify the dataset and save, the Handlebars template will revert to its default state. If you are unaware of this behavior and save the chart, your custom template will be lost.
- Open a Handlebars chart.
- Edit the dataset and save the changes.
- Observe that the Handlebars template field has been reset to the default, and your custom code has been replaced.
Screenshots/recordings
https://github.com/user-attachments/assets/c4236db8-9c85-48ef-80c4-83bb765f66e7
Superset version
master / latest-dev
Python version
I don't know
Node version
I don't know
Browser
Firefox
Additional context
This is not a new bug. I noticed it first in 4.0.0
Checklist
- [X] I have searched Superset docs and Slack and didn't find a solution to my problem.
- [X] I have searched the GitHub issue tracker and didn't find a similar bug report.
- [X] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.
Comment From: dosubot[bot]
Hey @dkrat7, I'm here to help you with any bugs, questions, or contributions you have for Superset. Let's tackle this issue together!
The issue you're experiencing, where the Handlebars template reverts to its default state upon changing the dataset, is likely related to how the HandlebarsViewer component handles state changes. The useMemo hook in this component triggers a recompile of the template whenever the templateSource or data changes, which could lead to the template resetting if not managed properly [1].
To address this, you might need to ensure that the custom template state is preserved across dataset changes. This could involve modifying the logic that handles state updates to prevent overwriting the custom template with the default one. You may want to look into how the state is managed and updated when a dataset change occurs, possibly by implementing a mechanism to save and restore the custom template state.
To continue talking to Dosu, mention @dosu.
Comment From: pivconbrt
@dkrat7 / @dosu - Thank you for logging this issue and a tip on what are of the source code may need changes. As a heavy user of the handlebar chart, I can confirm this problem exists in v4.1.1 and it is not in the release notes of v4.1.2 as being fixed. You can loose a lot of work in a complicated handlebars template when you update a datasource. We have been making sure every handlebar template is stored in source control outside of superset so we can recover them as we trip over this issue across the team frequently. It's easy to remember it once it happens, but if you have 8 people in superset and person B edits person A's chart they can easily blow out the template if they are not aware of the bug/issue.
@dosu, I see you footnoted a reference to the source code that has the code with the useMemo hook. We may attempt to resovle the issue in the code as we have forked the code to add some custom charts and are getting more familiar with the code. Do you have any tips/comments on what should be done to save the template so it's not lost? We could try that and test it and put in a merge request if we fix it. We are open to any recommendations on next steps (e.g. is there another example of code/etc. that would be helpful to review to understand best practices/patterns in superset code base related to maintaining the chart configuration state across data changes. I understand that if the model/schema changes it can break a normal chart/etc. In this case, with it being a handlebar template I think we can just let it stay as is and it just won't work if the reference dimension is not available after the datasource change.
Comment From: rusackas
@kgabryje put some safety warnings that apply when dataset updates are triggered... maybe there's a way to at least WARN that the handlebars code will be wiped. Obviously it'd be better to do something like use LocalStorage to track/reapply changes, and do a cleanup when the chart is finally saved.
PRs welcome from anyone here, obviously :)
Comment From: dkrat7
It looks like this was already fixed in #32195 (there was another issue opened #32049)