Fix TokenCreated dialog for null tokens#6441
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6441 +/- ##
=======================================
Coverage 76.65% 76.65%
=======================================
Files 398 398
Lines 20108 20108
Branches 4844 4844
=======================================
Hits 15414 15414
Misses 4694 4694
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| <ff-dialog ref="dialog" data-el="add-token-confirmation" header="Token Created"> | ||
| <template #default> | ||
| <p>Your token is <code>{{ token.token }}</code></p> | ||
| <p>Your token is <code>{{ token?.token }}</code></p> |
There was a problem hiding this comment.
Component is called TokenCreated because it means there should exist a token already when the component is created. If you add "?." It means, semantically, that this component could possibly display "Your token is ", which isn't right. A proper fix is to ensure that the whole TokenCreated is rendered only if token is set. I believe the error occurs because TokenCreated is created and mounted when Token isn't even available, which triggers 1 rendering cycle and the evaluation of the {{ token.token }} expression. This is a waste of rendering cycle. Could you check if it is possible to create TokenCreated only when token is available?
There was a problem hiding this comment.
The issue is that TokenCreated is created and mounted to the dom when it isn't even needed, wasting a rendering cycle for nothing
It would be more efficient to just create and mount the component with a v-if in this case, since it is a very small component Users wouldn't notice the time spent rendering and mounting a small template to the dom. You can define a new state variable called "showCreatedToken", and use it in v-if. When the following method is called, you could set showCreatedToken to true, then call showToken. If calling showToken in the same tick doesn't work, put it on the nextTick callback.
Another solution is to control the rendering of the component inside its own template. In other words add v-if inside of it instead of in the parent, then trigger the rendering when calling showToken from the parent. The trade-off is that vue will again spend time mounting something that isn't used yet.
There was a problem hiding this comment.
Agreed, we don't have the best state management when it comes to dialogs, mainly because we're using a mix of vue and the built in html5 dialog api.
cstns
left a comment
cstns
left a comment
There was a problem hiding this comment.
I couldn't open the dialog without a token, but the changes seem safe for existing functionality (and for the specific niche case it must have been opened).
Long-term, I plan to centralize our modal logic in Vue rather than using scattered HTML5 behaviors, but I'm pushing that back due to other priorities. I agree with @AllanOricil’s approach in principle, but I'm approving this now to avoid further delays. We'll revisit this during the eventual refactor.
Thanks for the PR!
PS: apologies for the late review, it managed to slip through the cracks
| <ff-dialog ref="dialog" data-el="add-token-confirmation" header="Token Created"> | ||
| <template #default> | ||
| <p>Your token is <code>{{ token.token }}</code></p> | ||
| <p>Your token is <code>{{ token?.token }}</code></p> |
There was a problem hiding this comment.
Agreed, we don't have the best state management when it comes to dialogs, mainly because we're using a mix of vue and the built in html5 dialog api.
3 participants
Description
By default,
tokenis null, which triggers an error when the default template is mounted due to the evaluation of the expression{{ token.token }}.showTokenis declared insetupand usesthiswhich is theoretically not possible.Checklist
flowforge.yml?FlowFuse/helmto update ConfigMap TemplateFlowFuse/CloudProjectto update values for Staging/ProductionLabels
area:migrationlabel