handle optimistic updates to checkbox in checklist item (#1184)

Co-authored-by: Caleb Roseland <[email protected]>
mattermost · May 19, 2022 · 136d62f · 136d62f
1 parent 3c5fb5e
commit 136d62f
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 26 deletions.
diff --git a/webapp/src/client.ts b/webapp/src/client.ts
@@ -312,11 +312,12 @@ export async function setDueDate(playbookRunId: string, checklistNum: number, it
 }
 
 export async function setChecklistItemState(playbookRunID: string, checklistNum: number, itemNum: number, newState: ChecklistItemState) {
-    return doPut(`${apiUrl}/runs/${playbookRunID}/checklists/${checklistNum}/item/${itemNum}/state`,
-        JSON.stringify({
-            new_state: newState,
-        }),
-    );
+    const body = JSON.stringify({new_state: newState});
+    try {
+        return await doPut<void>(`${apiUrl}/runs/${playbookRunID}/checklists/${checklistNum}/item/${itemNum}/state`, body);
+    } catch (error) {
+        return {error: error as ClientError};
+    }
 }
 
 export async function clientRemoveChecklistItem(playbookRunID: string, checklistNum: number, itemNum: number) {

diff --git a/webapp/src/components/checklist_item/checklist_item.tsx b/webapp/src/components/checklist_item/checklist_item.tsx
@@ -14,6 +14,7 @@ import {
     setDueDate as clientSetDueDate,
     setAssignee,
     clientSetChecklistItemCommand,
+    setChecklistItemState,
 } from 'src/client';
 import {ChecklistItem as ChecklistItemType, ChecklistItemState} from 'src/types/playbook';
 import {usePortal} from 'src/hooks';
@@ -34,7 +35,7 @@ interface ChecklistItemProps {
     itemNum: number;
     playbookRunId?: string;
     menuEnabled: boolean;
-    onChange?: (item: ChecklistItemState) => void;
+    onChange?: (item: ChecklistItemState) => ReturnType<typeof setChecklistItemState> | undefined;
     draggableProvided?: DraggableProvided;
     dragging: boolean;
     disabled: boolean;
@@ -223,11 +224,7 @@ export const ChecklistItem = (props: ChecklistItemProps): React.ReactElement =>
                 <CheckBoxButton
                     disabled={props.disabled || props.checklistItem.state === ChecklistItemState.Skip || props.playbookRunId === undefined}
                     item={props.checklistItem}
-                    onChange={(item: ChecklistItemState) => {
-                        if (props.onChange) {
-                            props.onChange(item);
-                        }
-                    }}
+                    onChange={(item: ChecklistItemState) => props.onChange?.(item)}
                 />
                 <ChecklistItemTitleWrapper
                     onClick={() => props.collapsibleDescription && props.checklistItem.description !== '' && toggleDescription()}

diff --git a/webapp/src/components/checklist_item/checklist_item_draggable.tsx b/webapp/src/components/checklist_item/checklist_item_draggable.tsx
@@ -37,11 +37,7 @@ const DraggableChecklistItem = (props: Props) => {
                     checklistNum={props.checklistIndex}
                     itemNum={props.itemIndex}
                     playbookRunId={props.playbookRun?.id}
-                    onChange={(newState: ChecklistItemState) => {
-                        if (props.playbookRun) {
-                            setChecklistItemState(props.playbookRun.id, props.checklistIndex, props.itemIndex, newState);
-                        }
-                    }}
+                    onChange={(newState: ChecklistItemState) => props.playbookRun && setChecklistItemState(props.playbookRun.id, props.checklistIndex, props.itemIndex, newState)}
                     draggableProvided={draggableProvided}
                     dragging={snapshot.isDragging}
                     disabled={finished}

diff --git a/webapp/src/components/checklist_item/inputs.tsx b/webapp/src/components/checklist_item/inputs.tsx
@@ -1,33 +1,45 @@
 
-import React, {useRef} from 'react';
+import React, {useRef, useState} from 'react';
 import styled from 'styled-components';
 import {useIntl} from 'react-intl';
+import {ClientError} from 'mattermost-redux/client/client4';
 
 import {ChecklistItem, ChecklistItemState} from 'src/types/playbook';
 import {PrimaryButton, TertiaryButton} from 'src/components/assets/buttons';
 
 interface CheckBoxButtonProps {
-    onChange: (item: ChecklistItemState) => void;
+    onChange: (item: ChecklistItemState) => undefined | Promise<void | {error: ClientError}>;
     item: ChecklistItem;
     disabled: boolean;
 }
 
 export const CheckBoxButton = (props: CheckBoxButtonProps) => {
-    const isChecked = props.item.state === ChecklistItemState.Closed;
+    const [isChecked, setIsChecked] = useState(props.item.state === ChecklistItemState.Closed);
+
+    // handleOnChange optimistic update approach: first do UI change, then
+    // call to server and finally revert UI state if there's error
+    //
+    // There are two main reasons why we do this:
+    // 1 - Happy path: avoid waiting 300ms to see checkbox update in the UI
+    // 2 - Websocket failure: we'll still mark the checkbox correctly
+    //     Additionally, we prevent the user from clicking multiple times
+    //     and leaving the item in an unknown state
+    const handleOnChange = async () => {
+        const newValue = isChecked ? ChecklistItemState.Open : ChecklistItemState.Closed;
+        setIsChecked(!isChecked);
+        const res = await props.onChange(newValue);
+        if (res?.error) {
+            setIsChecked(isChecked);
+        }
+    };
 
     return (
         <ChecklistItemInput
             className='checkbox'
             type='checkbox'
             checked={isChecked}
             disabled={props.disabled}
-            onChange={() => {
-                if (isChecked) {
-                    props.onChange(ChecklistItemState.Open);
-                } else {
-                    props.onChange(ChecklistItemState.Closed);
-                }
-            }}
+            onChange={handleOnChange}
         />);
 };