LPD-37782 Editor config contributor client extensions are re-applied when creating new Rich Text repeatable fields #4463
Conversation
…ow copy results in transformed config leaking to prop editor config reference.
|
CI is automatically triggering the following test suites:
|
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-37782 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#8259 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4463 Testray Routine:EE Pull Request Testray Build ID:30618308 Testray Importer:publish-testray-report#32617 |
| @@ -24,7 +24,7 @@ export default function loadEditorClientExtensions({ | |||
| }) | |||
| ), | |||
| onLoad: (bindingContexts) => { | |||
| let transformedConfig = config; | |||
| let transformedConfig = JSON.parse(JSON.stringify(config)); | |||
There was a problem hiding this comment.
The intended behavior we were going for is that the CXs modification to editor config is a local change. Original editor configuration, a controlled variable, should remain in it's original form. But because we are making an assignment here, changes leak to initial config object. @dsanz your intuition was right on this one! The shallow copy is also not good enough, as it would leak changes (JS 🤗 ) . We need to do a deep copy here. cc @thektan
The issue still remains that deep in parent framework config object changes on actions that should not really change it, such as focus on the field. This causes re-initialization of the whole editor, when it really doesn't need to. Improving this could be difficult, and is ultimately in the domain of product team. Because of this, I didn't revert the solution we did in #4430. This is the glitchy behavior if we do the revert on top of this PR:
vokoscreenNG-2024-10-03_15-31-58.mp4
There was a problem hiding this comment.
I see.... assignment makes 2 variables pointing to the same thing. One of the variables mutates the value.
|
ci:test:relevant |
✔️ ci:test:stable - 25 out of 25 jobs passed✔️ ci:test:relevant - 30 out of 31 jobs passed in 1 hour 8 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 5624963c55511dc19881843e67901a3eff0efabc ci:test:stable - 25 out of 25 jobs PASSED25 Successful Jobs:ci:test:relevant - 29 out of 31 jobs PASSED2 Failed Jobs:
29 Successful Jobs:For more details click here.This pull contains no unique failures.Failures in common with acceptance upstream results at 5624963:Test bundle downloads: |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#12706 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4463 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - markocikos > liferay-frontend - PR#4463 - 2024-10-04[02:39:44] Testray Build ID:31827030 Testray Importer:publish-testray-report#27786 |
|
Ok now we shall decide how to test this thing. I'd say it's more or less easy as long as we manage to re-render the editor without a full page reload. Perhaps in the sample module we can have a tab with 2 instances? |
We mostly already have a test for this case, the one we added in #4430. There is a subtle difference, the fact that leaked config was used on a separate instance. I don't think there is any other case for this, besides the repeatable fields. It would very difficult to mock this in a sample module, we would basically need to implement repeatable fields. That might not be worth the effort. Instead, if existing test in not enough, I'd add the new one to other Web Content tests. What do you think? |
|
I think test we added in #4430 asserts editor is usable. I'm not 100% sure this is enough. Probably we'd like to assert editor config is properly mutated in presence of one CX and successive renderings of the editor component. Another way of looking at this is to assert configuration immutability, via some editor component wrapper that:
Indeed I'd say we don't need more than 1 editor instance to test this. Perhaps this is easier as we don't need anything from other DXP apps, just a particular sample makes sense? |
References
What is the goal of this PR?
A bugfix. Technical note inline.
What does it look like?
Note - two
AI Creatorbuttons in expected behavior. One is added by product, one by CX. The bug was that their number grew to 3, 4, 5 etc.vokoscreenNG-2024-10-03_15-13-09.mp4
See behavior before PR and steps to reproduce in JIRA.