Address PR review comments

- MessageOverlays.tsx: revert to main (unrelated reactions-dialog fix that snuck in; will follow up separately) - composer.ts: rename MAX_IMAGES -> LEGACY_IMAGES_EMBED_MAX to flag it as the legacy/gallery split threshold; replace stale TODO with a defense-in-depth note matching applyGalleryCap's role - drafts/state/api.ts: update import + restore-path usage to new name - Composer.tsx: extract duplicated onImageAdd cap/toast/dispatch logic into a shared useAddImagesWithCap hook, used by both ComposerPost and ComposerFooter - MediaPreview.tsx: replace filter().slice(4).map() with a single-pass loop that stops at 4 tiles instead of walking a 10-image gallery
2026-06-04 13:50:40 -04:00
parent 77c08164b7
commit 0da63326bd
5 changed files with 101 additions and 169 deletions
@@ -53,31 +53,32 @@ export function Embed({
    )
  } else if (e.type === 'gallery') {
    // Notification/DM preview is a narrow inline strip; cap at 4 tiles so
-    // a 10-image gallery doesn't blow out the row width.
-    return (
-      <Outer style={style}>
-        {e.view.items
-          .filter(AppBskyEmbedGallery.isViewImage)
-          .slice(0, 4)
-          .map(item => {
-            const image: AppBskyEmbedImages.ViewImage = {
-              thumb: item.thumbnail,
-              fullsize: item.fullsize,
-              alt: item.alt,
-              aspectRatio: item.aspectRatio,
-            }
-            return peekable ? (
-              <PeekableImageItem key={item.thumbnail} image={image} />
-            ) : (
-              <ImageItem
-                key={item.thumbnail}
-                thumbnail={item.thumbnail}
-                alt={item.alt}
-              />
-            )
-          })}
-      </Outer>
-    )
+    // a 10-image gallery doesn't blow out the row width. Single pass instead
+    // of filter().slice().map() so we stop at 4 viewable items rather than
+    // walking every item in a 10-image gallery.
+    const tiles: React.ReactNode[] = []
+    for (const item of e.view.items) {
+      if (tiles.length >= 4) break
+      if (!AppBskyEmbedGallery.isViewImage(item)) continue
+      if (peekable) {
+        const image: AppBskyEmbedImages.ViewImage = {
+          thumb: item.thumbnail,
+          fullsize: item.fullsize,
+          alt: item.alt,
+          aspectRatio: item.aspectRatio,
+        }
+        tiles.push(<PeekableImageItem key={item.thumbnail} image={image} />)
+      } else {
+        tiles.push(
+          <ImageItem
+            key={item.thumbnail}
+            thumbnail={item.thumbnail}
+            alt={item.alt}
+          />,
+        )
+      }
+    }
+    return <Outer style={style}>{tiles}</Outer>
  } else if (e.type === 'link') {
    if (!e.view.external.thumb) return null
    if (!isGifEmbed(e.view.external.uri)) return null
@@ -4,7 +4,6 @@ import {
  useContext,
  useEffect,
  useMemo,
-  useRef,
  useState,
 } from 'react'
 import {LayoutAnimation} from 'react-native'
@@ -60,28 +59,8 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {
  } | null>(null)
  const [afterReportTarget, setAfterReportTarget] =
    useState<ChatBskyConvoDefs.MessageView | null>(null)
-  const [reactionsTargetId, setReactionsTargetId] = useState<string | null>(
-    null,
-  )
-  const [lastKnownReactionsMessage, setLastKnownReactionsMessage] =
+  const [reactionsTarget, setReactionsTarget] =
    useState<ChatBskyConvoDefs.MessageView | null>(null)
-  const reactionsOpenRequestedFor = useRef<string | null>(null)
-
-  const liveReactionsMessage = useMemo(() => {
-    if (!reactionsTargetId) return null
-    for (const item of convo.items) {
-      if (
-        (item.type === 'message' || item.type === 'pending-message') &&
-        item.message.id === reactionsTargetId
-      ) {
-        return item.message
-      }
-    }
-    return null
-  }, [convo.items, reactionsTargetId])
-
-  const displayReactionsMessage =
-    liveReactionsMessage ?? lastKnownReactionsMessage

  const openDeleteMessage = useCallback(
    (message: ChatBskyConvoDefs.MessageView) => {
@@ -104,42 +83,19 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {

  const openReactions = useCallback(
    (message: ChatBskyConvoDefs.MessageView) => {
-      reactionsOpenRequestedFor.current = message.id
-      setReactionsTargetId(message.id)
+      setReactionsTarget(message)
    },
    [],
  )

-  // The dialog is conditionally mounted, so we can't open it in the same tick
-  // that we set the target - the control ref isn't attached yet. Open in an
-  // effect after the dialog has mounted with a live message resolved.
+  // These dialogs are conditionally mounted, so we can't open them in the same
+  // tick that we set their targets - the control refs aren't attached yet. Open
+  // in an effect after the dialog has mounted.
  useEffect(() => {
-    if (
-      liveReactionsMessage &&
-      reactionsOpenRequestedFor.current === liveReactionsMessage.id
-    ) {
-      reactionsOpenRequestedFor.current = null
+    if (reactionsTarget) {
      reactionsControl.open()
    }
-  }, [liveReactionsMessage, reactionsControl])
-
-  // Keep a snapshot of the live message so the dialog can finish its close
-  // animation if the underlying message disappears from convo.items.
-  useEffect(() => {
-    if (liveReactionsMessage) {
-      // eslint-disable-next-line react-hooks/set-state-in-effect
-      setLastKnownReactionsMessage(liveReactionsMessage)
-    }
-  }, [liveReactionsMessage])
-
-  useEffect(() => {
-    if (reactionsTargetId && !liveReactionsMessage) {
-      reactionsControl.close()
-      // eslint-disable-next-line react-hooks/set-state-in-effect
-      setReactionsTargetId(null)
-      reactionsOpenRequestedFor.current = null
-    }
-  }, [reactionsTargetId, liveReactionsMessage, reactionsControl])
+  }, [reactionsTarget, reactionsControl])

  useEffect(() => {
    if (afterReportTarget) {
@@ -169,18 +125,13 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {
    [openDeleteMessage, openReportMessage, openReactions],
  )

-  const convoId = convo.convo.view.id
-  const reportSubject = useMemo(
-    () =>
-      reportTarget
-        ? ({
-            view: 'message',
-            convoId,
-            message: reportTarget.message,
-          } as const)
-        : undefined,
-    [reportTarget, convoId],
-  )
+  const reportSubject = reportTarget
+    ? ({
+        view: 'message',
+        convoId: convo.convo.view.id,
+        message: reportTarget.message,
+      } as const)
+    : undefined

  return (
    <Context.Provider value={ctx}>
@@ -202,16 +153,12 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {
          onClose={() => setAfterReportTarget(null)}
        />
      )}
-      {displayReactionsMessage && (
+      {reactionsTarget && (
        <ReactionsDialog
          control={reactionsControl}
          relatedProfiles={convo.relatedProfiles}
-          message={displayReactionsMessage}
-          onClose={() => {
-            setReactionsTargetId(null)
-            setLastKnownReactionsMessage(null)
-            reactionsOpenRequestedFor.current = null
-          }}
+          message={reactionsTarget}
+          onClose={() => setReactionsTarget(null)}
        />
      )}
      <Prompt.Basic
@@ -199,6 +199,44 @@ function applyGalleryCap(
  return {status: 'ok', accepted: incoming}
 }

+function useAddImagesWithCap(
+  currentCount: number,
+  dispatchPostAction: (action: PostAction) => void,
+) {
+  const {t: l} = useLingui()
+  return useCallback(
+    (next: ComposerImage[]) => {
+      const result = applyGalleryCap(currentCount, next)
+      if (result.status === 'full') {
+        Toast.show(
+          l({
+            message: `You can only add up to ${MAX_GALLERY_IMAGES} images per post`,
+            comment:
+              'Toast shown when the user tries to add more images but the post gallery is already at the cap',
+          }),
+          {type: 'warning'},
+        )
+        return
+      }
+      if (result.status === 'partial') {
+        Toast.show(
+          l({
+            message: `Only ${result.accepted.length} of ${next.length} ${plural(next.length, {one: 'image', other: 'images'})} added; limit is ${MAX_GALLERY_IMAGES}`,
+            comment:
+              'Toast shown when adding images would exceed the post gallery cap; only the first N are kept',
+          }),
+          {type: 'warning'},
+        )
+      }
+      dispatchPostAction({
+        type: 'embed_add_images',
+        images: result.accepted,
+      })
+    },
+    [currentCount, dispatchPostAction, l],
+  )
+}
+
 type Props = ComposerOpts
 export const ComposePost = ({
  replyTo,
@@ -1426,42 +1464,11 @@ let ComposerPost = memo(function ComposerPost({
    [dispatch, post.id],
  )

-  const onImageAdd = useCallback(
-    (next: ComposerImage[]) => {
-      const media = post.embed.media
-      const currentCount =
-        media?.type === 'images' || media?.type === 'gallery'
-          ? media.images.length
-          : 0
-      const result = applyGalleryCap(currentCount, next)
-      if (result.status === 'full') {
-        Toast.show(
-          l({
-            message: `You can only add up to ${MAX_GALLERY_IMAGES} images per post`,
-            comment:
-              'Toast shown when the user tries to add more images but the post gallery is already at the cap',
-          }),
-          {type: 'warning'},
-        )
-        return
-      }
-      if (result.status === 'partial') {
-        Toast.show(
-          l({
-            message: `Only ${result.accepted.length} of ${next.length} ${plural(next.length, {one: 'image', other: 'images'})} added; limit is ${MAX_GALLERY_IMAGES}`,
-            comment:
-              'Toast shown when adding images would exceed the post gallery cap; only the first N are kept',
-          }),
-          {type: 'warning'},
-        )
-      }
-      dispatchPost({
-        type: 'embed_add_images',
-        images: result.accepted,
-      })
-    },
-    [dispatchPost, l, post.embed.media],
-  )
+  const postImagesCount =
+    post.embed.media?.type === 'images' || post.embed.media?.type === 'gallery'
+      ? post.embed.media.images.length
+      : 0
+  const onImageAdd = useAddImagesWithCap(postImagesCount, dispatchPost)

  const onNewLink = useCallback(
    (uri: string) => {
@@ -1989,37 +1996,7 @@ function ComposerFooter({
    isMediaSelectionDisabled = !!media
  }

-  const onImageAdd = useCallback(
-    (next: ComposerImage[]) => {
-      const result = applyGalleryCap(images.length, next)
-      if (result.status === 'full') {
-        Toast.show(
-          l({
-            message: `You can only add up to ${MAX_GALLERY_IMAGES} images per post`,
-            comment:
-              'Toast shown when the user tries to add more images but the post gallery is already at the cap',
-          }),
-          {type: 'warning'},
-        )
-        return
-      }
-      if (result.status === 'partial') {
-        Toast.show(
-          l({
-            message: `Only ${result.accepted.length} of ${next.length} ${plural(next.length, {one: 'image', other: 'images'})} added; limit is ${MAX_GALLERY_IMAGES}`,
-            comment:
-              'Toast shown when adding images would exceed the post gallery cap; only the first N are kept',
-          }),
-          {type: 'warning'},
-        )
-      }
-      dispatch({
-        type: 'embed_add_images',
-        images: result.accepted,
-      })
-    },
-    [dispatch, images.length, l],
-  )
+  const onImageAdd = useAddImagesWithCap(images.length, dispatch)

  const onSelectGif = useCallback(
    (gif: Gif) => {
@@ -15,7 +15,7 @@ import {createPublicAgent} from '#/state/session/agent'
 import {
  type ComposerState,
  type EmbedDraft,
-  MAX_IMAGES,
+  LEGACY_IMAGES_EMBED_MAX,
  type PostDraft,
 } from '#/view/com/composer/state/composer'
 import {type VideoState} from '#/view/com/composer/state/video'
@@ -535,7 +535,7 @@ export async function draftToComposerPosts(
      }
      if (restoredImages.length > 0) {
        embed.media =
-          restoredImages.length <= MAX_IMAGES
+          restoredImages.length <= LEGACY_IMAGES_EMBED_MAX
            ? {type: 'images', images: restoredImages}
            : {type: 'gallery', images: restoredImages}
      }
@@ -160,7 +160,14 @@ export type ComposerAction =
      draftId: string
    }

-export const MAX_IMAGES = 4
+/**
+ * Threshold for picking between embed variants. <= this count uses the
+ * legacy `app.bsky.embed.images` shape; > this count promotes to
+ * `app.bsky.embed.gallery`. Named to flag that if/when we deprecate the
+ * legacy images embed entirely, this constant (and the variant split it
+ * gates) should go away.
+ */
+export const LEGACY_IMAGES_EMBED_MAX = 4
 export const MAX_GALLERY_IMAGES = 10

 /**
@@ -174,8 +181,8 @@ export const MAX_GALLERY_IMAGES = 10
 function imagesToMediaVariant(
  images: ComposerImage[],
 ): ImagesMedia | GalleryMedia {
-  return images.length <= MAX_IMAGES
-    ? {type: 'images', images: images.slice(0, MAX_IMAGES)}
+  return images.length <= LEGACY_IMAGES_EMBED_MAX
+    ? {type: 'images', images: images.slice(0, LEGACY_IMAGES_EMBED_MAX)}
    : {type: 'gallery', images: images.slice(0, MAX_GALLERY_IMAGES)}
 }

@@ -366,8 +373,8 @@ function postReducer(state: PostDraft, action: PostAction): PostDraft {
          : 0
      const incomingCount = prevCount + action.images.length
      if (incomingCount > MAX_GALLERY_IMAGES) {
-        // TODO: surface this to the user via a toast once the composer
-        // state shape supports reducer-emitted errors. The hard slice in
+        // Defense in depth: callers (applyGalleryCap in Composer) should have
+        // already trimmed and surfaced a toast. The hard slice in
        // imagesToMediaVariant still drops the excess so the cap holds.
        logger.warn('composer: image add exceeds MAX_GALLERY_IMAGES', {
          prevCount,