Small logic cleanups (#3449)

* Small logic cleanups

* Small logic cleanups (#3451)

* remove a few things

* oops

* stop swallowing the error

* queue callbacks

* oops

* log error if caught

* no need to be nullable

* move isClosing=true up

* reset `isClosing` and `closeCallbacks` on close completion and open

* run queued callbacks on `open` if there are any pending

* rm unnecessary ref and check

* ensure order of calls is always correct

* call `snapToIndex()` on open

* add tester to storybook

---------

Co-authored-by: Hailey <me@haileyok.com>

src/components/Dialog/context.ts

@@ -39,8 +39,7 @@ export function useDialogControl(): DialogOuterProps['control'] {
         control.current.open()
       },
       close: cb => {
-        control.current.close()
+        control.current.close(cb)
-        cb?.()
       },
     }),
     [id, control],

src/components/Dialog/index.tsx

@@ -83,7 +83,7 @@ export function Outer({
   const sheetOptions = nativeOptions?.sheet || {}
   const hasSnapPoints = !!sheetOptions.snapPoints
   const insets = useSafeAreaInsets()
-  const closeCallback = React.useRef<() => void>()
+  const closeCallbacks = React.useRef<(() => void)[]>([])
   const {setDialogIsOpen} = useDialogStateControlContext()
 
   /*
@@ -96,22 +96,51 @@ export function Outer({
    */
   const isOpen = openIndex > -1
 
+  const callQueuedCallbacks = React.useCallback(() => {
+    for (const cb of closeCallbacks.current) {
+      try {
+        cb()
+      } catch (e: any) {
+        logger.error('Error running close callback', e)
+      }
+    }
+
+    closeCallbacks.current = []
+  }, [])
+
   const open = React.useCallback<DialogControlProps['open']>(
     ({index} = {}) => {
+      // Run any leftover callbacks that might have been queued up before calling `.open()`
+      callQueuedCallbacks()
+
       setDialogIsOpen(control.id, true)
       // can be set to any index of `snapPoints`, but `0` is the first i.e. "open"
       setOpenIndex(index || 0)
+      sheet.current?.snapToIndex(index || 0)
     },
-    [setOpenIndex, setDialogIsOpen, control.id],
+    [setDialogIsOpen, control.id, callQueuedCallbacks],
   )
 
+  // This is the function that we call when we want to dismiss the dialog.
   const close = React.useCallback<DialogControlProps['close']>(cb => {
-    if (cb && typeof cb === 'function') {
+    if (typeof cb === 'function') {
-      closeCallback.current = cb
+      closeCallbacks.current.push(cb)
     }
     sheet.current?.close()
   }, [])
 
+  // This is the actual thing we are doing once we "confirm" the dialog. We want the dialog's close animation to
+  // happen before we run this. It is passed to the `BottomSheet` component.
+  const onCloseAnimationComplete = React.useCallback(() => {
+    // This removes the dialog from our list of stored dialogs. Not super necessary on iOS, but on Android this
+    // tells us that we need to toggle the accessibility overlay setting
+    setDialogIsOpen(control.id, false)
+    setOpenIndex(-1)
+
+    callQueuedCallbacks()
+    onClose?.()
+  }, [callQueuedCallbacks, control.id, onClose, setDialogIsOpen])
+
   useImperativeHandle(
     control.ref,
     () => ({
@@ -121,21 +150,6 @@ export function Outer({
     [open, close],
   )
 
-  const onCloseInner = React.useCallback(() => {
-    try {
-      closeCallback.current?.()
-    } catch (e: any) {
-      logger.error(`Dialog closeCallback failed`, {
-        message: e.message,
-      })
-    } finally {
-      closeCallback.current = undefined
-    }
-    setDialogIsOpen(control.id, false)
-    onClose?.()
-    setOpenIndex(-1)
-  }, [control.id, onClose, setDialogIsOpen])
-
   const context = React.useMemo(() => ({close}), [close])
 
   return (
@@ -163,7 +177,7 @@ export function Outer({
             backdropComponent={Backdrop}
             handleIndicatorStyle={{backgroundColor: t.palette.primary_500}}
             handleStyle={{display: 'none'}}
-            onClose={onCloseInner}>
+            onClose={onCloseAnimationComplete}>
             <Context.Provider value={context}>
               <View
                 style={[

src/components/Dialog/index.web.tsx

@@ -33,40 +33,41 @@ export function Outer({
   const t = useTheme()
   const {gtMobile} = useBreakpoints()
   const [isOpen, setIsOpen] = React.useState(false)
-  const [isVisible, setIsVisible] = React.useState(true)
   const {setDialogIsOpen} = useDialogStateControlContext()
 
   const open = React.useCallback(() => {
-    setIsOpen(true)
     setDialogIsOpen(control.id, true)
+    setIsOpen(true)
   }, [setIsOpen, setDialogIsOpen, control.id])
 
-  const onCloseInner = React.useCallback(async () => {
-    setIsVisible(false)
-    await new Promise(resolve => setTimeout(resolve, 150))
-    setIsOpen(false)
-    setIsVisible(true)
-    setDialogIsOpen(control.id, false)
-    onClose?.()
-  }, [control.id, onClose, setDialogIsOpen])
-
   const close = React.useCallback<DialogControlProps['close']>(
     cb => {
+      setDialogIsOpen(control.id, false)
+      setIsOpen(false)
+
       try {
         if (cb && typeof cb === 'function') {
-          cb()
+          // This timeout ensures that the callback runs at the same time as it would on native. I.e.
+          // console.log('Step 1') -> close(() => console.log('Step 3')) -> console.log('Step 2')
+          // This should always output 'Step 1', 'Step 2', 'Step 3', but without the timeout it would output
+          // 'Step 1', 'Step 3', 'Step 2'.
+          setTimeout(cb)
         }
       } catch (e: any) {
         logger.error(`Dialog closeCallback failed`, {
           message: e.message,
         })
-      } finally {
-        onCloseInner()
       }
+
+      onClose?.()
     },
-    [onCloseInner],
+    [control.id, onClose, setDialogIsOpen],
   )
 
+  const handleBackgroundPress = React.useCallback(async () => {
+    close()
+  }, [close])
+
   useImperativeHandle(
     control.ref,
     () => ({
@@ -103,7 +104,7 @@ export function Outer({
             <TouchableWithoutFeedback
               accessibilityHint={undefined}
               accessibilityLabel={_(msg`Close active dialog`)}
-              onPress={onCloseInner}>
+              onPress={handleBackgroundPress}>
               <View
                 style={[
                   web(a.fixed),
@@ -113,7 +114,6 @@ export function Outer({
                   gtMobile ? a.p_lg : a.p_md,
                   {overflowY: 'auto'},
                 ]}>
-                {isVisible && (
                 <Animated.View
                   entering={FadeIn.duration(150)}
                   // exiting={FadeOut.duration(150)}
@@ -123,7 +123,6 @@ export function Outer({
                     {opacity: 0.8, backgroundColor: t.palette.black},
                   ]}
                 />
-                )}
 
                 <View
                   style={[
@@ -135,7 +134,7 @@ export function Outer({
                       minHeight: web('calc(90vh - 36px)') || undefined,
                     },
                   ]}>
-                  {isVisible ? children : null}
+                  {children}
                 </View>
               </View>
             </TouchableWithoutFeedback>

src/components/Prompt.tsx

@@ -123,6 +123,13 @@ export function Action({
   cta,
   testID,
 }: {
+  /**
+   * Callback to run when the action is pressed. The method is called _after_
+   * the dialog closes.
+   *
+   * Note: The dialog will close automatically when the action is pressed, you
+   * should NOT close the dialog as a side effect of this method.
+   */
   onPress: () => void
   color?: ButtonColor
   /**
@@ -165,6 +172,13 @@ export function Basic({
   description: string
   cancelButtonCta?: string
   confirmButtonCta?: string
+  /**
+   * Callback to run when the Confirm button is pressed. The method is called
+   * _after_ the dialog closes.
+   *
+   * Note: The dialog will close automatically when the action is pressed, you
+   * should NOT close the dialog as a side effect of this method.
+   */
   onConfirm: () => void
   confirmButtonColor?: ButtonColor
 }>) {

src/view/com/composer/Composer.tsx

@@ -507,9 +507,7 @@ export const ComposePost = observer(function ComposePost({
         control={discardPromptControl}
         title={_(msg`Discard draft?`)}
         description={_(msg`Are you sure you'd like to discard this draft?`)}
-        onConfirm={() => {
+        onConfirm={onClose}
-          discardPromptControl.close(onClose)
-        }}
         confirmButtonCta={_(msg`Discard`)}
         confirmButtonColor="negative"
       />

src/view/screens/Storybook/Dialogs.tsx

@@ -6,12 +6,13 @@ import {atoms as a} from '#/alf'
 import {Button, ButtonText} from '#/components/Button'
 import * as Dialog from '#/components/Dialog'
 import * as Prompt from '#/components/Prompt'
-import {H3, P} from '#/components/Typography'
+import {H3, P, Text} from '#/components/Typography'
 
 export function Dialogs() {
   const scrollable = Dialog.useDialogControl()
   const basic = Dialog.useDialogControl()
   const prompt = Prompt.usePromptControl()
+  const testDialog = Dialog.useDialogControl()
   const {closeAllDialogs} = useDialogStateControlContext()
 
   return (
@@ -60,6 +61,15 @@ export function Dialogs() {
         <ButtonText>Open prompt</ButtonText>
       </Button>
 
+      <Button
+        variant="solid"
+        color="primary"
+        size="small"
+        onPress={testDialog.open}
+        label="one">
+        <ButtonText>Open Tester</ButtonText>
+      </Button>
+
       <Prompt.Outer control={prompt}>
         <Prompt.TitleText>This is a prompt</Prompt.TitleText>
         <Prompt.DescriptionText>
@@ -122,6 +132,131 @@ export function Dialogs() {
           </View>
         </Dialog.ScrollableInner>
       </Dialog.Outer>
+
+      <Dialog.Outer control={testDialog}>
+        <Dialog.Handle />
+
+        <Dialog.ScrollableInner
+          accessibilityDescribedBy="dialog-description"
+          accessibilityLabelledBy="dialog-title">
+          <View style={[a.relative, a.gap_md, a.w_full]}>
+            <Text>
+              Watch the console logs to test each of these dialog edge cases.
+              Functionality should be consistent across both native and web. If
+              not then *sad face* something is wrong.
+            </Text>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+              }}
+              label="Close It">
+              <ButtonText>Normal Use (Should just log)</ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+
+                setTimeout(() => {
+                  testDialog.open()
+                }, 100)
+              }}
+              label="Close It">
+              <ButtonText>
+                Calls `.open()` in 100ms (Should log when the animation switches
+                to open)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                setTimeout(() => {
+                  testDialog.open()
+                }, 2e3)
+
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+              }}
+              label="Close It">
+              <ButtonText>
+                Calls `.open()` in 2000ms (Should log after close animation and
+                not log on open)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+                setTimeout(() => {
+                  testDialog.close(() => {
+                    console.log('close callback after 100ms')
+                  })
+                }, 100)
+              }}
+              label="Close It">
+              <ButtonText>
+                Calls `.close()` then again in 100ms (should log twice)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+                testDialog.close(() => {
+                  console.log('close callback 2')
+                })
+              }}
+              label="Close It">
+              <ButtonText>
+                Call `close()` twice immediately (should just log twice)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                console.log('Step 1')
+                testDialog.close(() => {
+                  console.log('Step 3')
+                })
+                console.log('Step 2')
+              }}
+              label="Close It">
+              <ButtonText>
+                Log before `close()`, after `close()` and in the `close()`
+                callback. Should be an order of 1 2 3
+              </ButtonText>
+            </Button>
+          </View>
+        </Dialog.ScrollableInner>
+      </Dialog.Outer>
     </View>
   )
 }