feat[new-channels]: mobile action drawers and other mobile improvements#2337
feat[new-channels]: mobile action drawers and other mobile improvements#2337peterchinman merged 24 commits intomainfrom
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbitRelease NotesNew Features
Style & UI Updates
WalkthroughThis PR introduces a mobile-specific message action drawer system, adds mobile keyboard and virtual keyboard height tracking, creates a new MobileDrawer component, and refactors channel reply input management with improved focus and scroll handling for mobile. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/mobile/MobileDrawer.tsx`:
- Around line 67-70: The drawer height currently uses raw "vh" values which
bypass the dynamic viewport variable; in MobileDrawer.tsx update the height
strings inside the className (the bucket using h-[80vh] and
h-[calc(80vh-var(--virtual-keyboard-height))]) to base both states on var(--dvh)
instead (e.g., use h-[var(--dvh)] and
h-[calc(var(--dvh)-var(--virtual-keyboard-height))]) so inputFocused() still
applies the native bottom offset via --virtual-keyboard-height while respecting
the shared dynamic viewport managed by useAppSquishHandlers.
- Around line 12-17: The JSDoc usage example incorrectly passes the container
HTMLElement as the second argument to scrollToFocusedInput, causing NaN; update
the example to call scrollToFocusedInput with the focus event (e) and a numeric
offset (e.g., 10) as the second parameter instead of e.currentTarget so the
signature matches scrollToFocusedInput(event, offset) and the function receives
a number for the offset.
In `@js/app/packages/block-channel/component/NewChannelBlockAdapter.tsx`:
- Line 44: The non-null assertion on channelType() is unsafe — useChannelType
(the createMemo returned by useChannelType) can be undefined when the channel
does not exist, and the Suspense around participantsQuery does not guard this
synchronous memo; remove the `!` and either (a) guard rendering of
ChannelTopLeft by checking channelType() != null before rendering it, or (b)
pass a safe default value (e.g., a fallback enum/string) to ChannelTopLeft so it
can handle missing channels; update the code around ChannelTopLeft and any
consumers of channelType() to handle undefined rather than asserting non-null.
In `@js/app/packages/channel/Message/ActionDrawer.tsx`:
- Around line 56-59: EmojiSearchView declares an onBack prop but never uses it;
update the view so the "More reactions" action/button (inside EmojiSearchView,
lines rendering the reactions list and the "More reactions" control) invokes
props.onBack() when tapped/clicked so the drawer navigates back into the parent
Reply/Edit/Delete state; ensure the event handler is attached to the exact
control that currently opens the full reactions picker and that the handler
prevents default/propagation if needed.
In `@js/app/packages/channel/Message/HoverActions.tsx`:
- Around line 16-17: The hover-only visibility uses `hidden` with
`group-hover/message:block` (and `isTouchDevice()` hides on touch), which
prevents keyboard users from tabbing to actions; update the visibility logic so
keyboard focus also reveals the actions—e.g., add focus-based classes alongside
hover (use `group-focus/message:block` or `focus-within`/`focus:block` on the
action container) and ensure `props.persistentVisible` still overrides; locate
the conditional class construction around `props.persistentVisible` and
`isTouchDevice()` in HoverActions.tsx and add the corresponding focus-focused
class variants so keyboard users can access the controls.
In `@js/app/packages/channel/Thread/ThreadReplyInput.tsx`:
- Around line 28-29: The keyboardWillShow handler and the hidden mobile input
state aren’t cleaned up on unmount/send, so add a cleanup that always removes
the window listener and restores the channel input visibility: inside
ThreadReplyInput, wrap the logic that assigns keyboardWillShowHandler and
registers the listener with a useEffect that returns a cleanup function which
calls window.removeEventListener(...) with the same keyboardWillShowHandler and
also resets the visibility via the useMobileChannelInputVisibility API (e.g.,
mobileChannelInputVisibility.setHidden(false) or
mobileChannelInputVisibility.reset()/restoreDefault()), ensuring both
keyboardWillShowHandler and mobileChannelInputVisibility are cleared on unmount
and on every exit path (send/close).
- Around line 53-63: The onFocusIn handler registers a one-time
'keyboardWillShow' listener (keyboardWillShowHandler) but misses the case where
the keyboard is already visible; update the handler in ThreadReplyInput.tsx to
detect current keyboard state/height (via your app's keyboard height
signal/context or platform API) and call
scrollReplyInputAboveKeyboard(props.messageId, currentHeight) immediately when
the keyboard is already shown, while still keeping the once listener for future
show events; reference keyboardWillShowHandler, onFocusIn,
mobileChannelInputVisibility, window.addEventListener and
scrollReplyInputAboveKeyboard when making the change.
In `@js/app/packages/core/component/Emoji/EmojiSelector.tsx`:
- Around line 155-174: The bug is that inside the inner <For
each={group.emojis}> you compare selectedIndex() to the inner index() (local to
the group) so the same position in every group appears selected; fix by
computing the emoji's global flat index and comparing that to selectedIndex():
capture the outer group index (use the second param of the outer For, e.g.,
groupIndex), compute the starting offset for that group as the cumulative sum of
emojis in previous groups (sum of groups[0..groupIndex-1].emojis.length), then
pass isSelected={selectedIndex() === offset + index()} into EmojiOption (i.e.,
replace the current isSelected prop with a comparison against the computed
global index).
In `@js/app/packages/core/mobile/focus-and-open-keyboard.ts`:
- Around line 35-44: The temporary input is appended to document.body but
positioned using positionNear?.offsetTop/offsetLeft which are relative to
positionNear's offsetParent, causing misplacement in scrolled containers; update
focusOnDummyElementToOpenIOSKeyboard to compute coordinates from
positionNear.getBoundingClientRect() and convert to document coordinates by
adding window.scrollY (or pageYOffset) and window.scrollX (or pageXOffset), then
set tempEl.style.top/left using those summed values (with the existing +7px
vertical offset) so the dummy element is placed correctly regardless of
scroll/offset parents; keep existing fallbacks when positionNear is null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3042d55d-c93b-445e-a85c-e94834cacfcd
📒 Files selected for processing (23)
js/app/packages/app/component/mobile/MobileDock.tsxjs/app/packages/app/component/mobile/MobileDrawer.tsxjs/app/packages/app/component/next-soup/soup-view/filters-bar/mobile-filter-drawer.tsxjs/app/packages/app/component/useAppSquishHandlers.tsjs/app/packages/block-channel/component/NewChannelBlockAdapter.tsxjs/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Channel/ThreadList.tsxjs/app/packages/channel/Channel/mobile-channel-input-visibility.tsxjs/app/packages/channel/Input/ChannelInputContainer.tsxjs/app/packages/channel/Message/ActionDrawer.tsxjs/app/packages/channel/Message/ChannelMessage.tsxjs/app/packages/channel/Message/HoverActions.tsxjs/app/packages/channel/Message/MessageActionDrawerManager.tsxjs/app/packages/channel/Message/Root.tsxjs/app/packages/channel/Message/context.tsjs/app/packages/channel/Message/index.tsjs/app/packages/channel/Message/render-icon.tsxjs/app/packages/channel/Thread/ChannelThread.tsxjs/app/packages/channel/Thread/ThreadReplyInput.tsxjs/app/packages/channel/scroll-utils.tsjs/app/packages/core/component/Emoji/EmojiSelector.tsxjs/app/packages/core/directive/touchHandler.tsjs/app/packages/core/mobile/focus-and-open-keyboard.ts
| * Call this from a scroll container's `onFocusIn` to smoothly scroll a | ||
| * focused input/textarea to `offset` px from the container's top edge. | ||
| * | ||
| * Usage: | ||
| * <div onFocusIn={(e) => scrollToFocusedInput(e, e.currentTarget)}> | ||
| */ |
There was a problem hiding this comment.
Fix the JSDoc usage example.
The second parameter is an offset, not the container element. Copy-pasting the current snippet would pass an HTMLElement where a number is expected and end up scrolling to NaN.
📝 Suggested doc fix
- * <div onFocusIn={(e) => scrollToFocusedInput(e, e.currentTarget)}>
+ * <div onFocusIn={(e) => scrollToFocusedInput(e)}>
+ * <div onFocusIn={(e) => scrollToFocusedInput(e, 40)}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Call this from a scroll container's `onFocusIn` to smoothly scroll a | |
| * focused input/textarea to `offset` px from the container's top edge. | |
| * | |
| * Usage: | |
| * <div onFocusIn={(e) => scrollToFocusedInput(e, e.currentTarget)}> | |
| */ | |
| * Call this from a scroll container's `onFocusIn` to smoothly scroll a | |
| * focused input/textarea to `offset` px from the container's top edge. | |
| * | |
| * Usage: | |
| * <div onFocusIn={(e) => scrollToFocusedInput(e)}> | |
| * <div onFocusIn={(e) => scrollToFocusedInput(e, 40)}> | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/mobile/MobileDrawer.tsx` around lines 12 - 17,
The JSDoc usage example incorrectly passes the container HTMLElement as the
second argument to scrollToFocusedInput, causing NaN; update the example to call
scrollToFocusedInput with the focus event (e) and a numeric offset (e.g., 10) as
the second parameter instead of e.currentTarget so the signature matches
scrollToFocusedInput(event, offset) and the function receives a number for the
offset.
| 'bottom-(--virtual-keyboard-height) fixed left-0 right-0 z-modal bg-page rounded-t-2xl flex flex-col h-[80vh] data-transitioning:transition-transform data-transitioning:duration-200 ease-out', | ||
| inputFocused() | ||
| ? 'pb-0 h-[calc(80vh-var(--virtual-keyboard-height))]' | ||
| : 'pb-(--safe-bottom)', |
There was a problem hiding this comment.
Use the shared dynamic viewport variable for drawer height.
useAppSquishHandlers updates --dvh on the iOS/web path, but both height states here are still based on raw vh. That bypasses the existing keyboard-resize contract on mobile web, so the drawer can remain too tall and still sit under the keyboard on Safari. Base the height on var(--dvh) and keep --virtual-keyboard-height for the native bottom offset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/mobile/MobileDrawer.tsx` around lines 67 - 70,
The drawer height currently uses raw "vh" values which bypass the dynamic
viewport variable; in MobileDrawer.tsx update the height strings inside the
className (the bucket using h-[80vh] and
h-[calc(80vh-var(--virtual-keyboard-height))]) to base both states on var(--dvh)
instead (e.g., use h-[var(--dvh)] and
h-[calc(var(--dvh)-var(--virtual-keyboard-height))]) so inputFocused() still
applies the native bottom offset via --virtual-keyboard-height while respecting
the shared dynamic viewport managed by useAppSquishHandlers.
| </> | ||
| <ChannelTopLeft | ||
| channelId={props.channelId} | ||
| channelType={channelType()!} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check useChannelType return type and implementation to verify non-null assertion safety
# Find the useChannelType implementation
echo "=== useChannelType implementation ==="
ast-grep --pattern 'export function useChannelType($$$) {
$$$
}'
echo -e "\n=== useChannelType type signature ==="
rg -n -A 5 'export.*useChannelType' --type=ts
echo -e "\n=== Check if it returns a resource or signal that can suspend ==="
rg -n -C 5 'function useChannelType' --type=tsRepository: macro-inc/macro
Length of output: 1912
🏁 Script executed:
# View the NewChannelBlockAdapter.tsx file around lines 31-50 to understand context
cd js/app/packages/block-channel/component && sed -n '25,55p' NewChannelBlockAdapter.tsxRepository: macro-inc/macro
Length of output: 1014
Remove the unsafe non-null assertion on channelType().
The useChannelType hook returns a createMemo that derives ctx.channelsById()[channelId]?.channel_type, which can legitimately return undefined when the channel doesn't exist. The non-null assertion (!) bypasses this safety and will pass undefined to ChannelTopLeft if the channel is missing, causing a potential runtime error.
This is also inconsistent with line 36, where channelType() is safely used in an equality comparison without any assertion. Either handle the undefined case properly or ensure the channel is guaranteed to exist before rendering.
The Suspense boundary only suspends the participantsQuery resource; it does not protect channelType() since that's a synchronous memo derivation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/block-channel/component/NewChannelBlockAdapter.tsx` at line
44, The non-null assertion on channelType() is unsafe — useChannelType (the
createMemo returned by useChannelType) can be undefined when the channel does
not exist, and the Suspense around participantsQuery does not guard this
synchronous memo; remove the `!` and either (a) guard rendering of
ChannelTopLeft by checking channelType() != null before rendering it, or (b)
pass a safe default value (e.g., a fallback enum/string) to ChannelTopLeft so it
can handle missing channels; update the code around ChannelTopLeft and any
consumers of channelType() to handle undefined rather than asserting non-null.
| function EmojiSearchView(props: { | ||
| onSelect: (emoji: string) => void; | ||
| onBack: () => void; | ||
| }) { |
There was a problem hiding this comment.
Expose the back path from emoji search.
EmojiSearchView accepts onBack, but nothing in this view calls it. After tapping More reactions, users have to dismiss and reopen the entire drawer to get back to Reply/Edit/Delete.
Also applies to: 69-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Message/ActionDrawer.tsx` around lines 56 - 59,
EmojiSearchView declares an onBack prop but never uses it; update the view so
the "More reactions" action/button (inside EmojiSearchView, lines rendering the
reactions list and the "More reactions" control) invokes props.onBack() when
tapped/clicked so the drawer navigates back into the parent Reply/Edit/Delete
state; ensure the event handler is attached to the exact control that currently
opens the full reactions picker and that the handler prevents
default/propagation if needed.
| props.persistentVisible ? '' : 'hidden group-hover/message:block', | ||
| isTouchDevice() && 'hidden', |
There was a problem hiding this comment.
hidden + hover-only reveal can make actions unreachable via keyboard.
At Line 16, actions are only shown on hover. With display:none, keyboard users may not be able to tab into controls unless focus-based visibility is also enabled.
Proposed fix
- props.persistentVisible ? '' : 'hidden group-hover/message:block',
+ props.persistentVisible
+ ? ''
+ : 'hidden group-hover/message:block group-focus-within/message:block',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Message/HoverActions.tsx` around lines 16 - 17, The
hover-only visibility uses `hidden` with `group-hover/message:block` (and
`isTouchDevice()` hides on touch), which prevents keyboard users from tabbing to
actions; update the visibility logic so keyboard focus also reveals the
actions—e.g., add focus-based classes alongside hover (use
`group-focus/message:block` or `focus-within`/`focus:block` on the action
container) and ensure `props.persistentVisible` still overrides; locate the
conditional class construction around `props.persistentVisible` and
`isTouchDevice()` in HoverActions.tsx and add the corresponding focus-focused
class variants so keyboard users can access the controls.
| const mobileChannelInputVisibility = useMobileChannelInputVisibility(); | ||
| let keyboardWillShowHandler: ((e: Event) => void) | undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd js/app/packages/channel/Thread && cat -n ThreadReplyInput.tsx | head -100Repository: macro-inc/macro
Length of output: 4666
Clean up the mobile reply-focus state on every exit path.
The pending keyboardWillShow callback and hidden-channel-input state are only cleared when focus moves to a node outside this wrapper. If the component unmounts before this happens—when closing or sending a reply—the main channel input stays hidden and the window listener is never removed, causing memory leaks and persistent UI issues.
Suggested cleanup pattern
-import type { Accessor, Setter } from 'solid-js';
+import { onCleanup, type Accessor, type Setter } from 'solid-js';
export function ThreadReplyInput(props: ThreadReplyInputProps) {
const mobileChannelInputVisibility = useMobileChannelInputVisibility();
let keyboardWillShowHandler: ((e: Event) => void) | undefined;
+ const clearKeyboardWillShow = () => {
+ if (!keyboardWillShowHandler) return;
+ window.removeEventListener('keyboardWillShow', keyboardWillShowHandler);
+ keyboardWillShowHandler = undefined;
+ };
+
+ onCleanup(() => {
+ mobileChannelInputVisibility?.show();
+ clearKeyboardWillShow();
+ });
onFocusIn={() => {
mobileChannelInputVisibility?.hide();
+ clearKeyboardWillShow();
keyboardWillShowHandler = (event: Event) => {
const height =
(event as CustomEvent<{ height: number }>).detail?.height ?? 0;
scrollReplyInputAboveKeyboard(props.messageId, height);
keyboardWillShowHandler = undefined;
@@
onFocusOut={(e) => {
if (!e.currentTarget.contains(e.relatedTarget as Node)) {
mobileChannelInputVisibility?.show();
- if (keyboardWillShowHandler) {
- window.removeEventListener(
- 'keyboardWillShow',
- keyboardWillShowHandler
- );
- keyboardWillShowHandler = undefined;
- }
+ clearKeyboardWillShow();
}
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Thread/ThreadReplyInput.tsx` around lines 28 - 29,
The keyboardWillShow handler and the hidden mobile input state aren’t cleaned up
on unmount/send, so add a cleanup that always removes the window listener and
restores the channel input visibility: inside ThreadReplyInput, wrap the logic
that assigns keyboardWillShowHandler and registers the listener with a useEffect
that returns a cleanup function which calls window.removeEventListener(...) with
the same keyboardWillShowHandler and also resets the visibility via the
useMobileChannelInputVisibility API (e.g.,
mobileChannelInputVisibility.setHidden(false) or
mobileChannelInputVisibility.reset()/restoreDefault()), ensuring both
keyboardWillShowHandler and mobileChannelInputVisibility are cleared on unmount
and on every exit path (send/close).
| onFocusIn={() => { | ||
| mobileChannelInputVisibility?.hide(); | ||
| keyboardWillShowHandler = (event: Event) => { | ||
| const height = | ||
| (event as CustomEvent<{ height: number }>).detail?.height ?? 0; | ||
| scrollReplyInputAboveKeyboard(props.messageId, height); | ||
| keyboardWillShowHandler = undefined; | ||
| }; | ||
| window.addEventListener('keyboardWillShow', keyboardWillShowHandler, { | ||
| once: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "ThreadReplyInput.tsx" --path "*channel*"Repository: macro-inc/macro
Length of output: 292
🏁 Script executed:
cat -n js/app/packages/channel/Thread/ThreadReplyInput.tsx | head -100Repository: macro-inc/macro
Length of output: 4666
🏁 Script executed:
cat -n js/app/packages/channel/Thread/ThreadReplyInput.tsx | sed -n '40,80p'Repository: macro-inc/macro
Length of output: 1623
🏁 Script executed:
rg -n "scrollReplyInputAboveKeyboard" js/app/packages/channel --context 3Repository: macro-inc/macro
Length of output: 2047
🏁 Script executed:
rg -n "keyboardWillShow" js/app/packages/channelRepository: macro-inc/macro
Length of output: 905
🏁 Script executed:
fd -t f "scroll-utils" js/app/packages/channelRepository: macro-inc/macro
Length of output: 98
🏁 Script executed:
cat -n js/app/packages/channel/scroll-utils.ts | sed -n '50,100p'Repository: macro-inc/macro
Length of output: 1566
🏁 Script executed:
rg -n "keyboardWillShow\|keyboardDidShow\|keyboard" js/app/packages/channel/Input --context 2 | head -60Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "onFocusIn\|addEventListener.*keyboard" js/app/packages/channel/Input -A 10 | head -80Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
fd -t f "ChannelInput" js/app/packages/channel/InputRepository: macro-inc/macro
Length of output: 161
🏁 Script executed:
rg -n "keyboardWillShow\|keyboardDidShow" js/app/packages/channel -B 2 -A 5Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "window.innerHeight\|keyboard" js/app/packages/channel/Input/ChannelInput.tsx | head -30Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
cat -n js/app/packages/channel/Input/ChannelInput.tsx | head -150Repository: macro-inc/macro
Length of output: 5981
🏁 Script executed:
rg -n "addEventListener.*keyboard\|onFocusIn\|keyboardHeight" js/app/packages/channel -B 2 -A 5 | head -100Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "window.addEventListener" js/app/packages/channel/Input/ -B 3 -A 3Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "mobileChannelInputVisibility\|keyboardHeight" js/app/packages/channel -B 2 -A 5 | head -80Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
cat -n js/app/packages/channel/Input/ChannelInputContainer.tsx | head -200Repository: macro-inc/macro
Length of output: 756
🏁 Script executed:
rg -n "window.addEventListener\|keyboardDidShow\|keyboard" js/app/packages/channel/Channel/ -B 2 -A 5 | head -100Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "addEventListener.*keyboardWillShow\|addEventListener.*keyboard" js/app/packages/channel --context 5Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
fd -t f "mobile-channel-input-visibility" js/app/packages/channelRepository: macro-inc/macro
Length of output: 126
🏁 Script executed:
rg -n "class.*Keyboard\|function.*Keyboard\|keyboard" js/app/packages/channel/Channel/mobile-channel-input-visibility -B 2 -A 8Repository: macro-inc/macro
Length of output: 162
🏁 Script executed:
cat -n js/app/packages/channel/Channel/mobile-channel-input-visibility.tsxRepository: macro-inc/macro
Length of output: 1525
🏁 Script executed:
rg -n "addEventListener\|keyboardWillShow\|keyboardDidShow" js/app/packages/channel/Thread/ThreadReplyInput.tsx -B 3 -A 3Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "onFocusIn" js/app/packages/channel --context 10 | head -120Repository: macro-inc/macro
Length of output: 1908
🏁 Script executed:
rg -n "keyboardDidShow\|Keyboard\|keyboard" js/app/packages/channel -t tsx -t ts | grep -i "show\|hide\|visible\|height" | head -50Repository: macro-inc/macro
Length of output: 86
🏁 Script executed:
rg -n "Capacitor\|window\.innerHeight\|visualViewport" js/app --context 3 | head -100Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "already.*visible\|keyboard.*already" js/app --context 5Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "keyboardWillShow" js/app --context 5Repository: macro-inc/macro
Length of output: 5220
🏁 Script executed:
rg -n "initialKeyboard\|startingKeyboard\|Capacitor" js/app/packages/channel --context 3Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
cat -n js/app/packages/channel/Thread/ThreadReplyInput.tsx | sed -n '1,120p'Repository: macro-inc/macro
Length of output: 5583
🏁 Script executed:
cat -n js/app/packages/app/component/useAppSquishHandlers.ts | sed -n '30,80p'Repository: macro-inc/macro
Length of output: 2286
🏁 Script executed:
rg -n "keyboardDidShow\|keyboardDidHide\|visualViewport" js/app --context 3Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
rg -n "--virtual-keyboard-height" js/app --context 3 | head -40Repository: macro-inc/macro
Length of output: 486
🏁 Script executed:
rg -n "virtual-keyboard-height" js/app --context 2 | head -60Repository: macro-inc/macro
Length of output: 2761
🏁 Script executed:
rg -n "virtualKeyboardHeight\|virtualKeyboardVisible" js/app/packages --context 3 | head -80Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
cat -n js/app/packages/app/component/useAppSquishHandlers.ts | sed -n '1,50p'Repository: macro-inc/macro
Length of output: 2224
Handle replies opened while the keyboard is already visible.
When a thread reply is opened while the channel input remains focused (keyboard already visible), no keyboardWillShow event fires. The listener registered in onFocusIn with {once: true} cannot catch an event that occurred before it was registered, causing scrollReplyInputAboveKeyboard() to never be called in this scenario.
Consider detecting the current keyboard state on reply open and calling the scroll function immediately if the keyboard is already visible, or use a keyboard height signal/context that tracks the current keyboard state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/channel/Thread/ThreadReplyInput.tsx` around lines 53 - 63,
The onFocusIn handler registers a one-time 'keyboardWillShow' listener
(keyboardWillShowHandler) but misses the case where the keyboard is already
visible; update the handler in ThreadReplyInput.tsx to detect current keyboard
state/height (via your app's keyboard height signal/context or platform API) and
call scrollReplyInputAboveKeyboard(props.messageId, currentHeight) immediately
when the keyboard is already shown, while still keeping the once listener for
future show events; reference keyboardWillShowHandler, onFocusIn,
mobileChannelInputVisibility, window.addEventListener and
scrollReplyInputAboveKeyboard when making the change.
| <For each={groups}> | ||
| {(group): JSX.Element => ( | ||
| <Show when={group.emojis.length > 0}> | ||
| <div class="mt-2 w-full"> | ||
| <p class="pl-1 text-ink-extra-muted text-xs">{group.name}</p> | ||
| <div style={gridStyle()}> | ||
| <For each={group.emojis}> | ||
| {(emojiItem, index): JSX.Element => ( | ||
| <EmojiOption | ||
| emoji={emojiItem} | ||
| onEmojiClick={props.onEmojiClick} | ||
| isSelected={selectedIndex() === index()} | ||
| /> | ||
| )} | ||
| </For> | ||
| </div> | ||
| </div> | ||
| </Show> | ||
| )} | ||
| </For> |
There was a problem hiding this comment.
Bug: isSelected comparison uses local group index instead of global flat index.
In the grouped emoji view, index() inside the inner <For each={group.emojis}> is the local index within that specific group. However, selectedIndex() is a global index computed from groups.flatMap((g) => g.emojis) in the keyboard handler.
This causes emojis at the same local position in every group to appear selected simultaneously. For example, if selectedIndex() is 3, then the emoji at local index 3 in group 1, group 2, etc., will all show the selected highlight.
🐛 Proposed fix: track cumulative offset for global index comparison
<For each={groups}>
- {(group): JSX.Element => (
+ {(group, groupIndex): JSX.Element => {
+ const groupOffset = () => {
+ let offset = 0;
+ for (let i = 0; i < groupIndex(); i++) {
+ offset += groups[i].emojis.length;
+ }
+ return offset;
+ };
+ return (
<Show when={group.emojis.length > 0}>
<div class="mt-2 w-full">
<p class="pl-1 text-ink-extra-muted text-xs">{group.name}</p>
<div style={gridStyle()}>
<For each={group.emojis}>
{(emojiItem, index): JSX.Element => (
<EmojiOption
emoji={emojiItem}
onEmojiClick={props.onEmojiClick}
- isSelected={selectedIndex() === index()}
+ isSelected={selectedIndex() === groupOffset() + index()}
/>
)}
</For>
</div>
</div>
</Show>
- )}
+ );
+ }}
</For>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/component/Emoji/EmojiSelector.tsx` around lines 155 -
174, The bug is that inside the inner <For each={group.emojis}> you compare
selectedIndex() to the inner index() (local to the group) so the same position
in every group appears selected; fix by computing the emoji's global flat index
and comparing that to selectedIndex(): capture the outer group index (use the
second param of the outer For, e.g., groupIndex), compute the starting offset
for that group as the cumulative sum of emojis in previous groups (sum of
groups[0..groupIndex-1].emojis.length), then pass isSelected={selectedIndex()
=== offset + index()} into EmojiOption (i.e., replace the current isSelected
prop with a comparison against the computed global index).
| function focusOnDummyElementToOpenIOSKeyboard() { | ||
| tempEl.style.position = 'absolute'; | ||
| tempEl.style.top = `${(positionNear?.offsetTop ?? 0) + 7}px`; | ||
| tempEl.style.left = `${positionNear?.offsetLeft ?? 0}px`; | ||
| tempEl.style.height = '0'; | ||
| tempEl.style.opacity = '0'; | ||
| tempEl.style.fontSize = '16px'; | ||
| document.body.appendChild(tempEl); | ||
| tempEl.focus(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "focus-and-open-keyboard.ts" -type fRepository: macro-inc/macro
Length of output: 115
🏁 Script executed:
cat -n js/app/packages/core/mobile/focus-and-open-keyboard.tsRepository: macro-inc/macro
Length of output: 2552
Fix temporary input positioning for scrolled contexts.
tempEl is appended to document.body but positioned using positionNear?.offsetTop/offsetLeft, which are relative to positionNear's offset parent. In scrollable views (e.g., scrolled thread), when positionNear is nested within a scrolled container, these coordinates are relative to that container—not document.body—causing misalignment and reintroducing the viewport jump this workaround prevents.
Suggested coordinate fix
function focusOnDummyElementToOpenIOSKeyboard() {
+ const rect = positionNear?.getBoundingClientRect();
tempEl.style.position = 'absolute';
- tempEl.style.top = `${(positionNear?.offsetTop ?? 0) + 7}px`;
- tempEl.style.left = `${positionNear?.offsetLeft ?? 0}px`;
+ tempEl.style.top = `${(rect?.top ?? 0) + window.scrollY + 7}px`;
+ tempEl.style.left = `${(rect?.left ?? 0) + window.scrollX}px`;
tempEl.style.height = '0';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/mobile/focus-and-open-keyboard.ts` around lines 35 - 44,
The temporary input is appended to document.body but positioned using
positionNear?.offsetTop/offsetLeft which are relative to positionNear's
offsetParent, causing misplacement in scrolled containers; update
focusOnDummyElementToOpenIOSKeyboard to compute coordinates from
positionNear.getBoundingClientRect() and convert to document coordinates by
adding window.scrollY (or pageYOffset) and window.scrollX (or pageXOffset), then
set tempEl.style.top/left using those summed values (with the existing +7px
vertical offset) so the dummy element is placed correctly regardless of
scroll/offset parents; keep existing fallbacks when positionNear is null.
peterchinman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/core/constant/featureFlags.ts (1)
283-291:⚠️ Potential issue | 🔴 CriticalHardcoded override bypasses feature flag infrastructure and creates dead code.
Changing
ENABLE_NEW_CHANNELS_OVERRIDEfromgetFeatureFlagOverride('ENABLE_NEW_CHANNELS')to a hardcodedtruehas significant implications:
- Dead code: Line 290 becomes unreachable since
ENABLE_NEW_CHANNELS_OVERRIDE !== undefinedis always true.- No rollback path: The
VITE_ENABLE_NEW_CHANNELS=falseenvironment variable override no longer works.- Bypasses PostHog: Gradual rollout via PostHog is now impossible.
- Broad impact: This gates critical logic in message caching, sync operations, optimistic updates, and reaction handling across 20+ call sites.
This change appears unrelated to the PR's stated objectives (mobile keyboard/input scroll behavior). If this is an intentional full rollout, clean up the dead code. If not, this may be a debugging artifact that should be reverted.
If intentional rollout, clean up dead code
-const ENABLE_NEW_CHANNELS_OVERRIDE = true; - -export function ENABLE_NEW_CHANNELS(): boolean { - if (ENABLE_NEW_CHANNELS_OVERRIDE !== undefined) { - return ENABLE_NEW_CHANNELS_OVERRIDE; - } - - return analytics.posthog.isFeatureEnabled('enable-new-channels') ?? false; -} +export const ENABLE_NEW_CHANNELS = true;Note: This would require updating all call sites from
ENABLE_NEW_CHANNELS()toENABLE_NEW_CHANNELS.If debugging artifact, revert to original
-const ENABLE_NEW_CHANNELS_OVERRIDE = true; +const ENABLE_NEW_CHANNELS_OVERRIDE = getFeatureFlagOverride('ENABLE_NEW_CHANNELS');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/core/constant/featureFlags.ts` around lines 283 - 291, The ENABLE_NEW_CHANNELS override was hardcoded to true, making ENABLE_NEW_CHANNELS() always return the override and rendering analytics.posthog logic dead; revert the change by restoring the original override source (use getFeatureFlagOverride('ENABLE_NEW_CHANNELS') or read VITE_ENABLE_NEW_CHANNELS) so ENABLE_NEW_CHANNELS_OVERRIDE can be undefined and allow ENABLE_NEW_CHANNELS() to fall back to analytics.posthog.isFeatureEnabled('enable-new-channels') OR, if this is an intentional permanent rollout, remove the function wrapper and export a single constant ENABLE_NEW_CHANNELS (and update all call sites from ENABLE_NEW_CHANNELS() to ENABLE_NEW_CHANNELS) to eliminate dead code and restore correct rollback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/channel/Channel/Channel.tsx`:
- Around line 308-364: When handling mobile reply focus in the createEffect for
state.replyInputEl, detect if the virtual keyboard is already visible by reading
the CSS variable --virtual-keyboard-height (via
getComputedStyle(document.documentElement).getPropertyValue('--virtual-keyboard-height')
and parsing to a number) and if > 0 call scrollReplyInputAboveKeyboard(item.id,
height) immediately; keep the existing keyboardWillShow handler/once listener
for the case keyboard appears later, and ensure keyboardWillShowHandler is
set/cleared exactly as currently done so removeEventListener still works (update
the focusin block around setIsChannelInputHidden and keyboardWillShowHandler to
perform the immediate check and call).
In `@js/app/packages/channel/Thread/ThreadReplyInput.tsx`:
- Line 50: ThreadReplyInput currently sets props.setReplyInputEl only when the
DOM node mounts but never clears it on unmount, leaving a stale element
reference; update ThreadReplyInput so the element is assigned and cleared
reliably by using a stable ref (e.g., localReplyInputRef) and a useEffect that
calls props.setReplyInputEl(localReplyInputRef.current) and returns a cleanup
that calls props.setReplyInputEl(null), ensuring Channel.tsx’s mobile keyboard
handler won’t get a detached element; reference the prop setReplyInputEl and the
ThreadReplyInput component when implementing this change.
---
Outside diff comments:
In `@js/app/packages/core/constant/featureFlags.ts`:
- Around line 283-291: The ENABLE_NEW_CHANNELS override was hardcoded to true,
making ENABLE_NEW_CHANNELS() always return the override and rendering
analytics.posthog logic dead; revert the change by restoring the original
override source (use getFeatureFlagOverride('ENABLE_NEW_CHANNELS') or read
VITE_ENABLE_NEW_CHANNELS) so ENABLE_NEW_CHANNELS_OVERRIDE can be undefined and
allow ENABLE_NEW_CHANNELS() to fall back to
analytics.posthog.isFeatureEnabled('enable-new-channels') OR, if this is an
intentional permanent rollout, remove the function wrapper and export a single
constant ENABLE_NEW_CHANNELS (and update all call sites from
ENABLE_NEW_CHANNELS() to ENABLE_NEW_CHANNELS) to eliminate dead code and restore
correct rollback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66e2c4e0-2118-4d63-b9e4-474c9c3c82a0
📒 Files selected for processing (7)
js/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Channel/thread-manager.tsjs/app/packages/channel/Input/ChannelInputContainer.tsxjs/app/packages/channel/Thread/ChannelThread.tsxjs/app/packages/channel/Thread/ThreadReplyInput.tsxjs/app/packages/channel/Thread/types.tsjs/app/packages/core/constant/featureFlags.ts
1 participant
Message action drawer opens on long-press.
autofocus thread replies
Unfortunately this requires us to do an ugly hacky workaround where, on click, we synchronously create a temporary input that we focus, to open the virtual keyboard, before we shift focus to our actual input target, once it is available.
scroll thread reply inputs above virtual keyboard