Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Autosave: trigger autosave regardless of being idle #12839

Merged
merged 5 commits into from
Jan 9, 2023
Merged

Autosave: trigger autosave regardless of being idle #12839

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
094004f
Remove timeout deps of story and pages
miina Dec 13, 2022
5bb049f
Merge remote-tracking branch 'origin/main' into fix/12809-autosave
miina Dec 20, 2022
cb5033a
Fix dep
miina Dec 20, 2022
429dacf
Fix
miina Dec 21, 2022
e8e0379
Merge remote-tracking branch 'origin/main' into fix/12809-autosave
miina Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 3 additions & 7 deletions packages/story-editor/src/components/autoSaveHandler/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,9 @@ function AutoSaveHandler() {
const {
state: { hasNewChanges },
} = useHistory();
const { autoSave } = useStory(({ actions }) => ({
const { autoSave, isAutoSaving } = useStory(({ actions }) => ({
autoSave: actions.autoSave,
}));
miina marked this conversation as resolved.
Show resolved Hide resolved
const { story, pages } = useStory(({ state }) => ({
story: state.story,
pages: state.pages,
}));
const isUploading = useIsUploadingToStory();

// Cache it to make it stable in terms of the below timeout
Expand All @@ -46,7 +42,7 @@ function AutoSaveHandler() {
}, [autoSave]);

useEffect(() => {
if (!hasNewChanges || !autoSaveInterval || isUploading) {
if (!hasNewChanges || !autoSaveInterval || isUploading || isAutoSaving) {
return undefined;
}
// This is only a timeout (and not an interval), as `hasNewChanges` will come
Expand All @@ -58,7 +54,7 @@ function AutoSaveHandler() {
);

return () => clearTimeout(timeout);
}, [autoSaveInterval, hasNewChanges, isUploading, story, pages]);
}, [autoSaveInterval, hasNewChanges, isUploading, isAutoSaving]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need isUploading here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we prevent saving while any upload is in progress. Always has been the case. Avoids saving data with half-finished uploads.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to confirm, if a user uploads a video and it is transcoding, there will be no autosaves? If you upload say 5 videos, that could take a while. Maybe we could add a check to see if there an process media element on them. If not do an autosave.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but IIRC only if the video is uploaded to the canvas and not just the library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking, as if you transcoding video takes say, 5 minutes, then blocking autosaves for that time, that could be a serious problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally. We can discuss improving this in a new ticket if we can find a way to avoid saving data with half-finished uploads.


return null;
}
Expand Down