Repository navigation
Conversation
|
The Pull Request introduced native fingerprint changes against the base commit. Generated by PR labeler 🤖 |
|
7229842 to
2b2bf8d
Compare
92bf9bf to
1aa5707
Compare
|
I found two problems and left an inline comment on each:
Claude review, run |
1aa5707 to
abd73a9
Compare
|
I found two non-blocking issues and posted them as inline comments:
Claude review, run |
abd73a9 to
6687caa
Compare
|
non-blocking: the Settings account The trigger The absolutely positioned trigger doesn't add to the host's size, so the host collapses. Its To fix it, either keep this menu as a sheet, or move the absolute positioning onto a wrapper Claude review, run |
6687caa to
8d4f2f1
Compare
|
One non-blocking finding, posted inline on Claude review, run |
|
I found one non-blocking issue and posted it as an inline comment on On iOS, the conversation header's I didn't find any other actionable defects. Claude review, run |
| {({props, state}) => ( | ||
| <Pressable | ||
| {...props} | ||
| hitSlop={HITSLOP_10} |
There was a problem hiding this comment.
non-blocking: iOS tap targets lose their hitSlop when they become inline triggers
The PR description says it: on iOS, hitSlop doesn't apply to an inline trigger, because the SwiftUI Menu label only receives taps inside the RNHostView bounds. This PR fixes that for the lightbox button by growing it to 44pt, but leaves several converted triggers relying on hitSlop.
Here the trigger is a 20pt icon plus p_xs, so about 28pt. With HITSLOP_10 the target used to be about 48pt; on iOS it's now 28pt. The account … sits on top of SettingsList.PressableItem, so a tap that lands inside the old slop area but outside the circle now hits the row. That switches to the account instead of opening "Remove account". Before this change, the same tap opened the menu.
The same root cause makes other converted triggers smaller on iOS:
- the starter pack
…: asize="small"round button withHITSLOP_20atStarterPackScreen.tsx:604 - the in-chat header
…inConvoMenu - the group member
…and "Admin" badge inMemberMenu
To fix it, give these triggers a 44pt hit area of their own through padding or min size, as the lightbox button does, and don't depend on hitSlop.
|
One non-blocking finding, posted inline at The cause is the one the PR description gives for the lightbox button: on iOS,
CI was still running when I finished, so it gives no evidence either way. Claude review, run |
655338a to
e37284e
Compare
| {({props}) => ( | ||
| <View style={[style]}> | ||
| <Button | ||
| label={props.accessibilityLabel} | ||
| {...props} | ||
| onPress={() => { | ||
| Keyboard.dismiss() | ||
| props.onPress() | ||
| }} |
There was a problem hiding this comment.
non-blocking: on iOS, the chat header's … menu no longer closes the keyboard
On iOS, an inline trigger never gets the tap. Instead, Menu.Trigger reads the root element's props.onPress (src/components/Menu/index.tsx:195-201) and calls it when the menu opens. The root element here is now the View, which has no onPress. So inlineTriggerOnPress is set to undefined, and the Keyboard.dismiss() wrapper on the Button never runs.
How to reproduce: in a conversation, type in the composer, then tap … in the header. The SwiftUI menu opens and the keyboard stays up. If you then pick "Report conversation", "Leave conversation" or "Block account", that prompt also opens with the keyboard raised. Before this change, the sheet's trigger always dismissed the keyboard first.
Suggested fix: make the Button the trigger's root element again, so the menu can find its onPress, and move style onto it or into a View around Menu.Root. Even then, SwiftUI only calls it on the first open. A reliable fix would dismiss the keyboard another way, for example from the menu's open hook.
|
I found one non-blocking issue and left it as an inline comment.
Claude review, run |
e37284e to
a66b274
Compare
| {({props, state}) => ( | ||
| <Pressable | ||
| {...props} | ||
| hitSlop={HITSLOP_10} |
There was a problem hiding this comment.
non-blocking: iOS account … loses its hit slop, so near-misses switch accounts
hitSlop={HITSLOP_10} stops working on iOS now that this is an inline trigger. You note in CircleChromeButton.tsx that hit slop doesn't apply to a native menu trigger, and that's why you grew the lightbox button to 44pt. This trigger is only a.p_xs around a md icon, which is about 28pt. Before this change, the hit slop made the target about 48pt.
The trigger sits on top of the SettingsList.PressableItem row, whose onPress is onSwitchAccount. On iOS, a tap that lands up to 10pt outside the dots, which used to open this menu, now reaches the row and switches to that account. To fix it, make the trigger's own box at least 44pt (for example, larger padding or a fixed size on the Pressable) instead of relying on hitSlop.
|
I found one non-blocking defect and left it as an inline comment:
Claude review, run |
a66b274 to
d6ea427
Compare
| }) | ||
| : undefined | ||
| }> | ||
| {...props}> |
There was a problem hiding this comment.
non-blocking: iPad long press does nothing while the native-menus gate is off
Menu.Root only renders a native menu when ax.features.NativeMenusEnable is on (src/components/Menu/index.tsx:78-79). When the gate is off, this is still the bottom-sheet menu, and the "press and hold, then slide to Quote post" replacement doesn't exist.
With onLongPress removed, a long press does nothing at all. PostControlButton always passes its own handleLongPress to the pressable (PostControlButton.tsx:71-82), and it returns early when onLongPress is undefined. Because a long-press handler is attached, Pressability treats the press as consumed and doesn't call onPress on release.
So for every iPad user outside the rollout, long-press-to-quote stops working and long press does nothing instead. iPhone keeps long-press-to-quote in RepostButton.tsx:46. A fix is to keep the long-press handler when the menu isn't inline, for example by reading useMenuContext().inline or the gate.
|
I left 1 non-blocking finding as an inline comment: when the native-menus gate is off, a long press on the iPad repost button now does nothing ( Claude review, run |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also fixes the trigger's label, which was copied from the repost menu. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d6ea427 to
34b7d5f
Compare
| // The lightbox is always dark, so its menu is too. | ||
| <ThemeProvider theme="dark"> |
There was a problem hiding this comment.
non-blocking: With the gate off, the image menu becomes a light sheet with dark-theme items
Menu.Root inline only renders a native menu when NativeMenusEnable is on (src/components/Menu/index.tsx:78-79). Otherwise it falls back to Dialog.Outer. That gate is off by default, so most users will see the fallback. The native Dialog.Outer takes the sheet's backgroundColor and colorScheme from useTheme(useThemeName()) (src/components/Dialog/index.tsx:90-91, 301-302), which is the app's colour mode. It ignores this ThemeProvider. The items inside use plain useTheme(), so they do get the dark theme.
How to reproduce: use the app in light mode with the gate off, open an image in the lightbox, and tap …. You get a white sheet, a light-mode handle, and dark bg_contrast_25 item cards with light text. The old hand-rolled card was dark in every colour mode.
Possible fixes: apply the ThemeProvider only when the menu really is inline, or keep the old dark card for the fallback until the gate rolls out.
| ) : ( | ||
| <RepostButtonDialog {...props} /> | ||
| ) | ||
| IS_IPAD ? <RepostButtonMenu {...props} /> : <RepostButtonDialog {...props} /> |
There was a problem hiding this comment.
non-blocking: Long press on the iPad repost button does nothing when the gate is off
Removing longPressToQuote assumes a long press opens the native menu. That only happens when NativeMenusEnable is on. With the gate off (the default), Menu.Root inline falls back to the bottom sheet (src/components/Menu/index.tsx:78-82). The trigger is now just PostControlButton {...props} with no onLongPress, so on iPad a long press neither quotes the post (the old behaviour) nor opens the menu. Today that's a silent regression for every iPad user.
Possible fix: keep the long-press handler for the non-inline case, for example by reading useMenuContext().inline in the trigger, until the gate is fully rolled out.
Findings: 2 non-blocking, posted inline.
Worth a close look
Claude review, run |
Part of APP-3734. Stacked on #12271.
Moves more menus to
<Menu.Root inline>. They come from an audit of everyMenu.Rootin the app, with theshowCancel(sheet-style) ones first.…(curate and mod lists)inlineinline…("Remove account")inline. The absolute positioning moves from the trigger to aViewaroundMenu.Root, since an inline trigger sits inside the menu's native host…inline. "Message" now fetches the member's chat availability if the trigger's prefetch hasn't run (see below)…(ConvoMenu)inlinein the conversation header, with a refactor (see below)RepostButtonMenu)inline. iPhone keeps its dialog, and web its dropdown…inline. "Opt out of Starter Pack" gets the leave-chat icon (an undo arrow when it reads "Undo opt-out"), and the trigger's label, copied from the repost menu, now says "Open Starter Pack menu"Lightbox/chrome/ImageMenu)inline, replacing the hand-rolledModaland animated card. It's wrapped inThemeProvider theme="dark"so it stays dark like the lightbox, as the web lightbox already does. The native header's buttons grow from 32pt to 44pt circles (24pt icons): on iOS,hitSlopdoesn't apply to a native menu trigger, so the circle itself is the tap targetConvoMenuThe same menu is the chat list's long-press menu, which has no trigger and opens from code. Native menus can't, so that one stays a sheet. For the header:
Viewwrapper moved inside the trigger, soMenu.RootfindsMenu.Triggeras a direct child.MenuContentcomponent into auseConvoMenuItemshook, which returns the groups. The inline menu puts them straight intoMenu.Outer, so it can see the groups. The sheet keeps them inMenuContent, which only mounts while the sheet is open. That way, chat list rows still don't each fetch their convo.The trigger's
onPresson iOSOn iOS, the SwiftUI menu takes the tap, so the trigger's own
onPressdoesn't run (checked with a temporary log on the simulator). #12271 now calls it the first time each menu opens, which is the only open that SwiftUI reports. Here:onPressinpressableProps, where the menu can't find it. So "Message" now fetches on press when the data isn't there.longPressToQuoteprop and its tests are removed, since iPad was its only user.Not converted
BottomBar): no trigger, and it opens from code.PostMenuandShareMenu: they render their contents lazily.RichTextTag: an inline-text trigger.LeftNav.Test plan
pnpm typecheck,pnpm lint,pnpm test src/components/Menuinline, so both are back to sheets. Banner full width again…open, with sections and icons…: three sections, destructive items in red. "Go to profile" navigates…(needs two accounts signed in) and the group chat member menu (needs a group chat)…, own and someone else's (not yet run on a device)🤖 Generated with Claude Code