Skip to content

Commit

Permalink
Fix False Positive Save Conflicts (#1437)
Browse files Browse the repository at this point in the history
Users reported false save conflicts and reloading loops while
editing/saving pages. I identified two issues:

### 1) Polling intervals aren't always cleared

Sometimes `CheckForChanges` polling intervals weren't cleared. This led
to one or multiple polls running in the background even when the page
wasn't focused. The polls persisted even after navigating to another
page.

Depending on the state the page was currently in when the interval
wasn't cleared, this led to one of two issues:

- a false save conflict was reported over and over again
- the page was updated over and over again -> reload loop

You can see the behavior in the video. The interval with id 112 wasn't
cleared for some reason.


https://github.com/vivid-planet/comet/assets/13380047/7e4f3b8c-9f7e-4fe7-94d8-d489efa1ad50

I'm not entirely sure how it happens. But I'm pretty sure that
b1e9340
solves this issue by always clearing the old interval before starting a
new one in `startPolling()` (instead of relying on `stopPolling()`
always being called between `startPolling()` calls).

### 2) CheckForChanges can happen between save and refetch

If you save a page, first an `UpdatePage` mutation is sent to persist
the changes. Then, an `EditPage` query is executed to update the
pageState.
Since the `CheckForChanges` poll runs constantly, it sometimes happened
that the requests finished in the following order:

`UpdatePage` -> `CheckForChanges` -> `EditPage` 

The page is saved and the `lastUpdatedAt` timestamp in the database is
updated. However, the timestamp in the local `pageState` hasn't been
updated yet.
`CheckForChanges` responds with the new timestamp from the DB. The local
and remote timestamps don't match -> a false Save Conflict is reported.
Then, once the save conflict dialog is already shown to the user, the
`EditPage` query returns and the pageState is updated correctly.

See video:



https://github.com/vivid-planet/comet/assets/13380047/6dd2e85a-2ea3-4527-9953-6f02b383c252


I fixed this by

- disabling the `CheckForChanges` query while saving
- awaiting a refetch of the `EditPage` before reenabling the poll

---

Closes COM-174
  • Loading branch information
thomasdax98 authored Nov 28, 2023
1 parent 0cd85a7 commit e1d3f00
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 4 deletions.
8 changes: 8 additions & 0 deletions .changeset/cold-dryers-shout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@comet/cms-admin": patch
---

Prevent false positive save conflicts while editing documents (e.g. `Page`):

- Stop checking for conflicts while saving is in progress
- Ensure that all "CheckForChanges" polls are cleared
3 changes: 3 additions & 0 deletions packages/admin/cms-admin/src/pages/createUsePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ export const createUsePage: CreateUsePage =
resolveHasConflict: (data) => {
return resolveHasSaveConflict(pageState?.document?.updatedAt, data?.page?.document?.updatedAt);
},
skip: saving,
},
{
hasChanges: hasChanges ?? false,
Expand Down Expand Up @@ -299,6 +300,8 @@ export const createUsePage: CreateUsePage =
},
attachedPageTreeNodeId: pageId,
} as GQLUpdatePageMutationVariables,
refetchQueries: [getQuery],
awaitRefetchQueries: true,
update(cache) {
// update reference to pageTreeNode
// needed for newly created pageTreeNodes
Expand Down
7 changes: 3 additions & 4 deletions packages/admin/cms-admin/src/pages/useSaveConflict.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,13 @@ export function useSaveConflict(options: SaveConflictOptions): SaveConflictHookR
let intervalId: number | undefined;

const startPolling = () => {
window.clearInterval(intervalId);
intervalId = window.setInterval(checkChanges, 10000);
};

const stopPolling = () => {
if (intervalId !== undefined) {
window.clearInterval(intervalId);
intervalId = undefined;
}
window.clearInterval(intervalId);
intervalId = undefined;
};

const handleFocus = () => {
Expand Down

0 comments on commit e1d3f00

Please sign in to comment.