[JIMT]CT-492] editor-overwrites - fixes editor from overwriting itself #57928
Conversation
| useEffect(() => { | ||
| shouldNotifyProjectUpdate.current = false; | ||
| projectUtilities.replaceProject(project); | ||
| }, [project, projectUtilities]); | ||
|
|
There was a problem hiding this comment.
Yes, I know that effects shouldn't depend upon each other, but because we're using a ref here it kinda has to until this is swapped out with something a little less clunky.
| @@ -83,7 +83,7 @@ const defaultProject: ProjectType = { | |||
| <link rel="stylesheet" href="styles.css"/> | |||
| <body> | |||
| Content goes here! | |||
| <div class="foo">Foo class!</div> | |||
| <div class="foo">[DEFAULT] Foo class!</div> | |||
There was a problem hiding this comment.
Just to hopefully help us see when we're loading in default data. I'd encourage everyone to change your data when you navigate in and save, so that if this ever shows up it's more clear.
apps/src/weblab2/Weblab2View.tsx
Outdated
| if (Lab2Registry.getInstance().getProjectManager()) { | ||
| if ( | ||
| Lab2Registry.getInstance().getProjectManager() && | ||
| newProject !== currentProject |
There was a problem hiding this comment.
if we blindly save even if there's no change, it can overwrite if you navigate too fast.
This is kinda a hack solution - it'd be better for CDOIDE not to fire off the additional noisy note, and it'd also be better if the project manager's save could somehow take into account if nothing's changing (but that's probably asking too much since it doesn't maintain state otherwise).
There was a problem hiding this comment.
The project manager actually does check if nothing has changed, and does not save in that case (link). Is the problem here that the channel id has changed but we are sending a late update with the code from the previous channel id?
There was a problem hiding this comment.
Why does this get called if you navigate between levels? Shouldn't we only call setProject when the project has been changed, not when it is initially loaded?
There was a problem hiding this comment.
The problem was that the setProject function wasn't memoized.
In turn, when you navigated it'd create a new setProject function which'd fall into the IDE and trigger the effect to claim that the project had changed, even though it hadn't in this case. With a memoized function, it behaves as expected.
Further, though, this is a bigger issue beyond this code - because the project manager's save method only takes data to save and not the channelID to save it to (I think I'm using the right terms), it's susceptible to this type of issue -
basically, if the user navigates old data from the prior level may still be loaded into components. If something is triggered which would cause a save, it might push forward the stale data to the project manager, which would then overwrite the new channelID with the prior channel's data. It's assuming that all data that currently exists on the page is new, but if it hasn't been refreshed yet it's stale, and if that stale data triggers a save, it'd overwrite.
As it did in this case since the function wasn't memoized. This should be flagged as a future fix to circle back to.
There was a problem hiding this comment.
I made a follow up task to look into the caching idea I came up with: https://codedotorg.atlassian.net/browse/CT-500
If you changed levels, your editor would overwrite the contents of the proper file with the default values provided in Weblab2View.tsx.
There were two issues going on -
shouldNotifyProjectUpdateflag too late in the process, which would noisily cause an update of the last project value beforereplaceProjectwas called for the new one. This would end de-syncing and overwriting with the prior value of the project. Fix here was to swap the effect order and bubble up where theshouldNotifyProjectUpdateflag was set.saveProjectwould fire a save even if the project itself was the same. This could cause de-sync issues when switching rapidly between levels and could end up overwriting or even swapping content. Fix here was to only save if the value actually changed.Links