Address review feedback on gallery-embed branch

- Composer.tsx: thread gallery through reply pills (hasMedia),
  post:create/draft:save analytics, missingAltError, and publish-fail
  logger annotation
- composer/state/composer.ts: surface a logger.warn when
  embed_add_images would exceed MAX_GALLERY_IMAGES (hard slice
  preserved); ascii-only comments
- maybeApplyGalleryOffsetStyles.ts: scope PostGalleryEmbedEnable gate
  to image-embed branches so gallery posts always get no-content offsets
- MessageOverlays.tsx: store reactionsTargetId instead of a frozen
  message snapshot; look up the live message each render. Memoize
  reportSubject on reportTarget+convoId so ReportDialog's parseReportSubject
  memo survives convo updates. Replace open-after-mount boolean with a
  ref keyed to id so reopening the same message reliably re-triggers
- ImageEmbed.tsx + ImageLayoutGrid.tsx: thread isWithinQuote through
  the 2-4 image fallthrough; explicit prop overrides the viewContext
  heuristic
- parseReportSubject.ts: flip the image attribute true for gallery
  embeds (top-level and post_with_media), matching the existing images
  branches
- AppIconSettings/index.tsx: restore the IconName casts the previous
  change dropped; IconName is still a literal union
- composer/photos/Gallery.tsx: wrap to a 4-column grid for n>4 so
  10-image previews don't crush remove/alt-text controls
- lib/api/index.ts: normalize gallery upload log nouns to image/images
This commit is contained in:
vineyardbovines
2026-06-03 18:23:33 -04:00
parent 4fe5f9da37
commit af2eec0f84
10 changed files with 132 additions and 46 deletions
+1
View File
@@ -144,6 +144,7 @@ export function ImageEmbed({
onPress={onPress}
onPressIn={onPressIn}
viewContext={rest.viewContext}
isWithinQuote={rest.isWithinQuote}
/>
</View>
)
+56 -18
View File
@@ -4,6 +4,7 @@ import {
useContext,
useEffect,
useMemo,
useRef,
useState,
} from 'react'
import {LayoutAnimation} from 'react-native'
@@ -59,8 +60,23 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {
} | null>(null)
const [afterReportTarget, setAfterReportTarget] =
useState<ChatBskyConvoDefs.MessageView | null>(null)
const [reactionsTarget, setReactionsTarget] =
useState<ChatBskyConvoDefs.MessageView | null>(null)
const [reactionsTargetId, setReactionsTargetId] = useState<string | 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 openDeleteMessage = useCallback(
(message: ChatBskyConvoDefs.MessageView) => {
@@ -83,19 +99,33 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {
const openReactions = useCallback(
(message: ChatBskyConvoDefs.MessageView) => {
setReactionsTarget(message)
reactionsOpenRequestedFor.current = message.id
setReactionsTargetId(message.id)
},
[],
)
// 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.
// 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.
useEffect(() => {
if (reactionsTarget) {
if (
liveReactionsMessage &&
reactionsOpenRequestedFor.current === liveReactionsMessage.id
) {
reactionsOpenRequestedFor.current = null
reactionsControl.open()
}
}, [reactionsTarget, reactionsControl])
}, [liveReactionsMessage, reactionsControl])
useEffect(() => {
if (reactionsTargetId && !liveReactionsMessage) {
reactionsControl.close()
// eslint-disable-next-line react-hooks/set-state-in-effect
setReactionsTargetId(null)
reactionsOpenRequestedFor.current = null
}
}, [reactionsTargetId, liveReactionsMessage, reactionsControl])
useEffect(() => {
if (afterReportTarget) {
@@ -125,13 +155,18 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {
[openDeleteMessage, openReportMessage, openReactions],
)
const reportSubject = reportTarget
? ({
view: 'message',
convoId: convo.convo.view.id,
message: reportTarget.message,
} as const)
: undefined
const convoId = convo.convo.view.id
const reportSubject = useMemo(
() =>
reportTarget
? ({
view: 'message',
convoId,
message: reportTarget.message,
} as const)
: undefined,
[reportTarget, convoId],
)
return (
<Context.Provider value={ctx}>
@@ -153,12 +188,15 @@ export function MessageOverlays({children}: {children: React.ReactNode}) {
onClose={() => setAfterReportTarget(null)}
/>
)}
{reactionsTarget && (
{liveReactionsMessage && (
<ReactionsDialog
control={reactionsControl}
relatedProfiles={convo.relatedProfiles}
message={reactionsTarget}
onClose={() => setReactionsTarget(null)}
message={liveReactionsMessage}
onClose={() => {
setReactionsTargetId(null)
reactionsOpenRequestedFor.current = null
}}
/>
)}
<Prompt.Basic
@@ -28,9 +28,6 @@ export function maybeApplyGalleryOffsetStyles(
additionalCauses?: ModerationCause[] | AppModerationCause[]
},
) {
// don't ever check gates like this, except this one time
if (!features.isOn(Features.PostGalleryEmbedEnable)) return
if (
!bsky.dangerousIsType<AppBskyFeedPost.Record>(
post.record,
@@ -40,6 +37,13 @@ export function maybeApplyGalleryOffsetStyles(
return
}
// The gate only controls whether legacy image embeds opt into the new
// expanded gallery layout. Gallery embeds always render expanded by item
// count, so their offset must apply regardless of the gate.
const isPostGalleryEmbedEnabled = features.isOn(
Features.PostGalleryEmbedEnable,
)
/*
* First check if we even have images
*/
@@ -64,12 +68,13 @@ export function maybeApplyGalleryOffsetStyles(
)
let hasImages = false
if (isImageEmbed) {
if (!isPostGalleryEmbedEnabled) return
// one image, not a gallery
if (embed.images.length === 1) return
hasImages = true
}
if (isGalleryEmbed) {
// single (or empty) gallery no offset needed
// single (or empty) gallery - no offset needed
if (embed.items.length <= 1) return
hasImages = true
}
@@ -80,6 +85,7 @@ export function maybeApplyGalleryOffsetStyles(
AppBskyEmbedImages.isMain,
)
) {
if (!isPostGalleryEmbedEnabled) return
// one image, not a gallery
if (embed.media.images.length === 1) return
}
@@ -89,7 +95,7 @@ export function maybeApplyGalleryOffsetStyles(
AppBskyEmbedGallery.isMain,
)
) {
// single (or empty) gallery no offset needed
// single (or empty) gallery - no offset needed
if (embed.media.items.length <= 1) return
}
hasImages = true
+9 -6
View File
@@ -19,16 +19,19 @@ interface ImageLayoutGridProps {
onPressIn?: (index: number) => void
style?: StyleProp<ViewStyle>
viewContext?: PostEmbedViewContext
isWithinQuote?: boolean
}
export function ImageLayoutGrid({style, ...props}: ImageLayoutGridProps) {
export function ImageLayoutGrid({
style,
isWithinQuote: isWithinQuoteProp,
...props
}: ImageLayoutGridProps) {
const {gtMobile} = useBreakpoints()
const gap =
const isWithinQuote =
isWithinQuoteProp ??
props.viewContext === PostEmbedViewContext.FeedEmbedRecordWithMedia
? gtMobile
? a.gap_xs
: a.gap_2xs
: a.gap_xs
const gap = isWithinQuote ? (gtMobile ? a.gap_xs : a.gap_2xs) : a.gap_xs
return (
<View style={style}>
@@ -87,7 +87,10 @@ export function parseReportSubject(
reply: !!record.reply,
image:
embed.type === 'images' ||
(embed.type === 'post_with_media' && embed.media.type === 'images'),
embed.type === 'gallery' ||
(embed.type === 'post_with_media' &&
(embed.media.type === 'images' ||
embed.media.type === 'gallery')),
video:
embed.type === 'video' ||
(embed.type === 'post_with_media' && embed.media.type === 'video'),
+3 -3
View File
@@ -343,15 +343,15 @@ async function resolveMedia(
}
if (embedDraft.media?.type === 'gallery') {
const imagesDraft = embedDraft.media.images
logger.debug(`Uploading gallery items`, {
logger.debug(`Uploading images`, {
count: imagesDraft.length,
})
onStateChange?.(t`Uploading images...`)
const items: $Typed<AppBskyEmbedGallery.Image>[] = await Promise.all(
imagesDraft.map(async (image, i) => {
logger.debug(`Compressing gallery image #${i}`)
logger.debug(`Compressing image #${i}`)
const {path, width, height, mime} = await compressImage(image)
logger.debug(`Uploading gallery image #${i}`)
logger.debug(`Uploading image #${i}`)
const res = await uploadBlob(agent, path, mime)
return {
$type: 'app.bsky.embed.gallery#image' as const,
@@ -129,7 +129,7 @@ function getAppIconName(icon: string | false): DynamicAppIcon.IconName {
if (!icon || icon === 'DEFAULT') {
return 'default_light'
} else {
return icon
return icon as DynamicAppIcon.IconName
}
}
@@ -151,7 +151,7 @@ function Group({
values={[value]}
maxSelections={1}
onChange={vals => {
if (vals[0]) onChange(vals[0])
if (vals[0]) onChange(vals[0] as DynamicAppIcon.IconName)
}}>
<View style={[a.flex_1, a.rounded_md, a.overflow_hidden]}>
{children}
+19 -5
View File
@@ -611,7 +611,11 @@ export const ComposePost = ({
ax.metric('draft:save', {
isNewDraft,
hasText: posts.some(p => p.richtext.text.trim().length > 0),
hasImages: posts.some(p => p.embed.media?.type === 'images'),
hasImages: posts.some(
p =>
p.embed.media?.type === 'images' ||
p.embed.media?.type === 'gallery',
),
hasVideo: posts.some(p => p.embed.media?.type === 'video'),
hasGif: posts.some(p => p.embed.media?.type === 'gif'),
hasQuote: posts.some(p => !!p.embed.quote),
@@ -780,7 +784,10 @@ export const ComposePost = ({
for (let i = 0; i < thread.posts.length; i++) {
const media = thread.posts[i].embed.media
if (media) {
if (media.type === 'images' && media.images.some(img => !img.alt)) {
if (
(media.type === 'images' || media.type === 'gallery') &&
media.images.some(img => !img.alt)
) {
return l`One or more images is missing alt text.`
}
if (media.type === 'gif' && !media.alt) {
@@ -931,7 +938,9 @@ export const ComposePost = ({
logger.error(e, {
message: `Composer: create post failed`,
hasImages: filteredThread.posts.some(
p => p.embed.media?.type === 'images',
p =>
p.embed.media?.type === 'images' ||
p.embed.media?.type === 'gallery',
),
})
@@ -953,7 +962,8 @@ export const ComposePost = ({
for (let post of filteredThread.posts) {
ax.metric('post:create', {
imageCount:
post.embed.media?.type === 'images'
post.embed.media?.type === 'images' ||
post.embed.media?.type === 'gallery'
? post.embed.media.images.length
: 0,
isReply: index > 0 || !!replyTo,
@@ -1819,7 +1829,11 @@ function ComposerPills({
}) {
const t = useTheme()
const media = post.embed.media
const hasMedia = media?.type === 'images' || media?.type === 'video'
const hasMedia =
media?.type === 'images' ||
media?.type === 'gallery' ||
media?.type === 'gif' ||
media?.type === 'video'
const hasLink = !!post.embed.link
// Don't render anything if no pills are going to be displayed
+5 -2
View File
@@ -70,11 +70,13 @@ const GalleryInner = ({images, containerInfo, dispatch}: GalleryInnerProps) => {
const {isMobile} = useWebMediaQueries()
const {altTextControlStyle, imageControlsStyle, imageStyle} = useMemo(() => {
// Cap columns at 4 so tiles stay tappable when MAX_GALLERY_IMAGES is high;
// n > 4 wraps to multiple rows via flexWrap on the gallery container.
const columns = Math.min(images.length, 4)
const side =
images.length === 1
? 250
: (containerInfo.width - IMAGE_GAP * (images.length - 1)) /
images.length
: (containerInfo.width - IMAGE_GAP * (columns - 1)) / columns
const isOverflow = isMobile && images.length > 2
@@ -272,6 +274,7 @@ const styles = StyleSheet.create({
gallery: {
flex: 1,
flexDirection: 'row',
flexWrap: 'wrap',
gap: IMAGE_GAP,
marginTop: 16,
},
+22 -4
View File
@@ -16,6 +16,7 @@ import {
postUriToRelativePath,
toBskyAppUrl,
} from '#/lib/strings/url-helpers'
import {logger} from '#/logger'
import {type ComposerImage, createInitialImages} from '#/state/gallery'
import {createPostgateRecord} from '#/state/queries/postgate/util'
import {threadgateRecordToAllowUISetting} from '#/state/queries/threadgate'
@@ -163,10 +164,12 @@ export const MAX_IMAGES = 4
export const MAX_GALLERY_IMAGES = 10
/**
* Picks the embed variant for a set of images. 4 lands in the legacy
* Picks the embed variant for a set of images. <=4 lands in the legacy
* `app.bsky.embed.images` shape; >4 promotes to `app.bsky.embed.gallery`.
* Anything beyond the gallery cap is silently dropped — callers should
* already have enforced the cap upstream (picker, paste, etc).
* Anything beyond the gallery cap is dropped by the hard slice; callers
* should already have enforced the cap upstream (picker, paste, etc),
* and the reducer logs a warning when the cap is exceeded so the UI
* layer can surface a toast.
*/
function imagesToMediaVariant(
images: ComposerImage[],
@@ -357,6 +360,21 @@ function postReducer(state: PostDraft, action: PostAction): PostDraft {
}
const prevMedia = state.embed.media
let nextMedia = prevMedia
const prevCount =
prevMedia?.type === 'images' || prevMedia?.type === 'gallery'
? prevMedia.images.length
: 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
// imagesToMediaVariant still drops the excess so the cap holds.
logger.warn('composer: image add exceeds MAX_GALLERY_IMAGES', {
prevCount,
incomingCount,
dropped: incomingCount - MAX_GALLERY_IMAGES,
})
}
if (!prevMedia) {
nextMedia = imagesToMediaVariant(action.images)
} else if (prevMedia.type === 'images' || prevMedia.type === 'gallery') {
@@ -412,7 +430,7 @@ function postReducer(state: PostDraft, action: PostAction): PostDraft {
}
} else {
// Re-pick the variant so a gallery that shrinks to <=4 demotes
// back to the legacy `app.bsky.embed.images` shape keeps old
// back to the legacy `app.bsky.embed.images` shape - keeps old
// clients rendering it when possible.
nextMedia = imagesToMediaVariant(remainingImages)
}