feat[new-channels]: mobile action drawer#2314
Closed
peterchinman wants to merge 15 commits intomainfrom
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces mobile-specific UI enhancements including a new mobile drawer component with keyboard and focus handling, message action drawer with emoji reactions, mobile channel input visibility management, configurable emoji picker columns, and haptic feedback for touch events. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/app/packages/core/constant/featureFlags.ts (1)
283-291:⚠️ Potential issue | 🟠 MajorHardcoded override disables feature-flag safety and makes fallback paths unreachable.
At Line 283, setting
ENABLE_NEW_CHANNELS_OVERRIDEto literaltruemakes the!== undefinedcheck at Line 286 always pass, so Line 290 is never used. This permanently forces new-channel behavior and removes runtime rollback. It also makes legacy branches unreachable injs/app/packages/block-channel/definition.ts:7-18andjs/app/packages/block-channel/component/ChannelBlockSwitch.tsx:7-17.Suggested fix
-const ENABLE_NEW_CHANNELS_OVERRIDE = true; +const ENABLE_NEW_CHANNELS_OVERRIDE = getFeatureFlagOverride( + 'ENABLE_NEW_CHANNELS' +); 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; + return ( + ENABLE_NEW_CHANNELS_OVERRIDE ?? + (analytics.posthog.isFeatureEnabled('enable-new-channels') ?? false) + ); }🤖 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 constant ENABLE_NEW_CHANNELS_OVERRIDE is hardcoded to true which makes ENABLE_NEW_CHANNELS() always return the override and prevents the analytics.posthog feature check from ever running; update the implementation by removing the hardcoded true (or make ENABLE_NEW_CHANNELS_OVERRIDE undefined by default or driven from an env/config) so that ENABLE_NEW_CHANNELS() falls back to analytics.posthog.isFeatureEnabled('enable-new-channels') when no override is provided, ensuring legacy branches in ChannelBlockSwitch and block-channel/definition remain reachable.js/app/packages/app/component/useAppSquishHandlers.ts (1)
57-108:⚠️ Potential issue | 🟠 MajoriOS web path should also set
--virtual-keyboard-heightfor correct drawer positioning.The native mobile path sets both
--virtual-keyboard-heightand--dvh, but the iOS web fallback (lines 57-108) only sets--dvh. TheMobileDrawercomponent (used inActionDrawer) depends on--virtual-keyboard-heightfor positioning viabottom-(--virtual-keyboard-height)and height calculation. Without this variable on iOS Safari, the action drawer will not position correctly above the virtual keyboard.The iOS path calculates the keyboard height as
previousViewportHeight - newViewportHeightwhen the keyboard appears; this value should also be exposed via the--virtual-keyboard-heightCSS variable to match the native mobile behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/useAppSquishHandlers.ts` around lines 57 - 108, The iOS web resize/focus handlers (handleResize, handleFocusOut) currently only update --dvh; compute the keyboard height when viewport shrinks (keyboardHeight = previousViewportHeight - newViewportHeight) inside handleResize and set it on the root element via document.documentElement.style.setProperty('--virtual-keyboard-height', `${keyboardHeight}px`), and when the keyboard is dismissed (both in the else branch of handleResize and in handleFocusOut) reset --virtual-keyboard-height to '0px' (or '0') while still toggling setVirtualKeyboardVisible and updating --dvh as done today so MobileDrawer positioning matches the native path.
🤖 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/Message/ActionDrawer.tsx`:
- Around line 67-71: The onMount callback in ActionDrawer.tsx currently uses
setTimeout(()=> inputRef?.focus(), 0) which schedules a macrotask; replace it
with queueMicrotask(() => inputRef?.focus()) to run the focus on the next
microtask tick. Update the onMount block that references inputRef and its focus
call so it calls queueMicrotask instead of setTimeout, preserving the optional
chaining and ensuring focus still runs after mount.
- Around line 105-109: The early-return in ActionDrawer currently does "if
(!drawerState) return console.warn('No drawer state.')" which returns undefined
and emits a console warning in production; change it to explicitly "return null"
when drawerState is falsy and remove or gate the console.warn call (e.g., only
warn in development) so production bundles don't log; locate this logic inside
the ActionDrawer component and update the useMessageActionDrawer null-check
accordingly.
- Around line 200-216: The divider utility on the non-destructive action buttons
currently uses "border-page" which is inappropriate here; update the class
string on the button rendered inside MobileDrawer.Section for
nonDestructiveActions() (the JSX mapping that produces the button with
data-message-action and onClick handled by handleAction) to use either
"border-panel" or "border-edge-muted" (to match the destructive section and
other message components) so the divider is subtle and consistent across Message
components.
---
Outside diff comments:
In `@js/app/packages/app/component/useAppSquishHandlers.ts`:
- Around line 57-108: The iOS web resize/focus handlers (handleResize,
handleFocusOut) currently only update --dvh; compute the keyboard height when
viewport shrinks (keyboardHeight = previousViewportHeight - newViewportHeight)
inside handleResize and set it on the root element via
document.documentElement.style.setProperty('--virtual-keyboard-height',
`${keyboardHeight}px`), and when the keyboard is dismissed (both in the else
branch of handleResize and in handleFocusOut) reset --virtual-keyboard-height to
'0px' (or '0') while still toggling setVirtualKeyboardVisible and updating --dvh
as done today so MobileDrawer positioning matches the native path.
In `@js/app/packages/core/constant/featureFlags.ts`:
- Around line 283-291: The constant ENABLE_NEW_CHANNELS_OVERRIDE is hardcoded to
true which makes ENABLE_NEW_CHANNELS() always return the override and prevents
the analytics.posthog feature check from ever running; update the implementation
by removing the hardcoded true (or make ENABLE_NEW_CHANNELS_OVERRIDE undefined
by default or driven from an env/config) so that ENABLE_NEW_CHANNELS() falls
back to analytics.posthog.isFeatureEnabled('enable-new-channels') when no
override is provided, ensuring legacy branches in ChannelBlockSwitch and
block-channel/definition remain reachable.
🪄 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: a54284ff-0429-43a1-a5f3-10a78f054ca2
📒 Files selected for processing (14)
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/channel/Channel/Channel.tsxjs/app/packages/channel/Message/ActionDrawer.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/core/component/Emoji/EmojiSelector.tsxjs/app/packages/core/constant/featureFlags.ts
Comment on lines
+67
to
+71
| onMount(() => { | ||
| setTimeout(() => { | ||
| inputRef?.focus(); | ||
| }, 0); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using queueMicrotask instead of setTimeout(…, 0).
For immediate-next-tick scheduling, queueMicrotask is more semantically appropriate and slightly more efficient than setTimeout with 0ms delay.
Optional refactor
onMount(() => {
- setTimeout(() => {
+ queueMicrotask(() => {
inputRef?.focus();
- }, 0);
+ });
});🤖 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 67 - 71, The
onMount callback in ActionDrawer.tsx currently uses setTimeout(()=>
inputRef?.focus(), 0) which schedules a macrotask; replace it with
queueMicrotask(() => inputRef?.focus()) to run the focus on the next microtask
tick. Update the onMount block that references inputRef and its focus call so it
calls queueMicrotask instead of setTimeout, preserving the optional chaining and
ensuring focus still runs after mount.
Comment on lines
+200
to
+216
| <MobileDrawer.Section class="flex flex-col shrink-0"> | ||
| <For each={nonDestructiveActions()}> | ||
| {(action) => ( | ||
| <button | ||
| type="button" | ||
| data-message-action={action.id} | ||
| class="flex items-center gap-3 px-4 py-3 text-sm text-ink hover:bg-hover hover-transition-bg text-left not-last:border-b border-page" | ||
| onClick={(event) => handleAction(action.onClick, event)} | ||
| > | ||
| <span class="size-5 flex items-center justify-center shrink-0"> | ||
| {renderIcon(action.icon)} | ||
| </span> | ||
| {action.label} | ||
| </button> | ||
| )} | ||
| </For> | ||
| </MobileDrawer.Section> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how border-page is used elsewhere in the codebase for consistency
rg -n --type=tsx --type=ts 'border-page' -C1 | head -40Repository: macro-inc/macro
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Search for border-page usage with correct syntax
echo "=== border-page usage ==="
rg 'border-page' -C2 | head -50
echo -e "\n=== border-panel usage ==="
rg 'border-panel' -C2 | head -50
echo -e "\n=== border-edge-muted usage ==="
rg 'border-edge-muted' -C2 | head -30Repository: macro-inc/macro
Length of output: 7432
🏁 Script executed:
#!/bin/bash
# Examine the ActionDrawer.tsx file directly
echo "=== ActionDrawer.tsx content (lines 195-230) ==="
sed -n '195,230p' js/app/packages/channel/Message/ActionDrawer.tsxRepository: macro-inc/macro
Length of output: 1843
🏁 Script executed:
#!/bin/bash
# Look for color/design token definitions
echo "=== Looking for token/color definitions ==="
fd -e 'ts' -e 'tsx' | xargs grep -l "border-page\|border-panel\|border-edge" 2>/dev/null | grep -i "token\|color\|theme" | head -10Repository: macro-inc/macro
Length of output: 284
Use border-panel or border-edge-muted for the non-destructive action dividers.
border-page (line 206) is semantically used for visual overlays (Visor.tsx, MacroJump.tsx) and may result in invisible borders in this context. The destructive section uses border-panel, and similar message components (Reactions.tsx, MessageFlag.tsx) use border-edge-muted for subtle dividers. Choose one for consistency.
🤖 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 200 - 216, The
divider utility on the non-destructive action buttons currently uses
"border-page" which is inappropriate here; update the class string on the button
rendered inside MobileDrawer.Section for nonDestructiveActions() (the JSX
mapping that produces the button with data-message-action and onClick handled by
handleAction) to use either "border-panel" or "border-edge-muted" (to match the
destructive section and other message components) so the divider is subtle and
consistent across Message components.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
js/app/packages/channel/Message/ActionDrawer.tsx (2)
205-206:⚠️ Potential issue | 🟡 MinorUse a subtler divider token for non-destructive action rows.
Line 205 uses
border-page, which is visually inconsistent with the destructive section (border-panelon Line 226). Preferborder-panelorborder-edge-mutedhere for consistency.Proposed fix
- class="flex items-center gap-3 px-4 py-3 text-sm text-ink hover:bg-hover hover-transition-bg text-left not-last:border-b border-page" + class="flex items-center gap-3 px-4 py-3 text-sm text-ink hover:bg-hover hover-transition-bg text-left not-last:border-b border-edge-muted"🤖 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 205 - 206, The divider for the non-destructive action row uses the token "border-page" which mismatches the destructive section; update the class on the element that renders the action row (the element using onClick={(event) => handleAction(action.onClick, event)}) in the ActionDrawer component so it uses a subtler divider token such as "border-panel" or "border-edge-muted" instead of "border-page" to match the destructive section styling.
105-107:⚠️ Potential issue | 🟡 MinorGate the missing-context warning to development builds.
Line 106 logs in all environments. This can create noisy production logs; keep the guard but only warn in dev.
Proposed fix
if (!drawerState) { - console.warn('No drawer state.'); + if (import.meta.env.DEV) { + console.warn('ActionDrawer rendered outside MessageActionDrawerContextProvider.'); + } return null; }🤖 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 105 - 107, The console.warn('No drawer state.') in ActionDrawer.tsx should only run in development to avoid noisy production logs; keep the existing early-return for drawerState but wrap the warning in a dev-only check (e.g., process.env.NODE_ENV !== 'production' or your project's __DEV__ flag) so that console.warn is invoked only when running in development builds.
🤖 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/AGENTS.md`:
- Line 65: Update AGENTS.md to add a clear migration strategy for replacing
SolidJS classList with cn() from `@ui/utils/classname`: state whether migration
should be immediate or phased (e.g., critical/high-usage components first), list
priority targets such as ToggleSwitch.tsx and Shape.tsx (and any other
high-usage components), mandate that new PRs convert classList usages in files
being touched and optionally convert nearby classList occurrences
opportunistically, and include acceptance criteria and a short roll-forward plan
for tracking progress (e.g., a checklist or issue/label to mark migrated files).
- Line 65: Update the AGENTS.md entry that mandates using cn() from
`@ui/utils/classname` instead of SolidJS's classList by adding a brief rationale
and concrete before/after usage examples: explain why classList is discouraged
(readability/consistency with cn() and easier composition), provide a short
"Don't" example using classList and two "Do" examples using cn() (single
conditional and multiple conditionals), and include a one-line migration note
showing the transformation pattern; reference the rule name and the symbols cn()
and classList so reviewers can locate the change.
In `@js/app/packages/channel/Message/ActionDrawer.tsx`:
- Around line 56-59: EmojiSearchView currently receives onBack but never invokes
it, preventing users from returning to the actions pane; update EmojiSearchView
to call the provided onBack when the user taps a back control (e.g., a header
back button or close/back icon) or when the emoji search should exit, and ensure
any back UI element is rendered and wired to invoke props.onBack; locate the
EmojiSearchView component (and related places where onBack is passed, such as
the parent that opens "More reactions") and add the back handler so tapping back
triggers onBack to restore the actions list.
In `@js/app/packages/channel/Thread/ThreadReplyInput.tsx`:
- Line 80: The onReady handler in ThreadReplyInput schedules a setTimeout to
focus the editor but never cancels it, which can refocus a disposed input;
change the implementation to capture the timeout ID (e.g., via a ref like
replyFocusTimeoutRef) when calling setTimeout in onReady={(handle) => ...}, and
clear that timeout with clearTimeout(replyFocusTimeoutRef.current) in the
component cleanup (useEffect cleanup) and whenever the input is closed or
onReady is called again so the delayed focus is canceled for unmounted/hidden
inputs.
---
Duplicate comments:
In `@js/app/packages/channel/Message/ActionDrawer.tsx`:
- Around line 205-206: The divider for the non-destructive action row uses the
token "border-page" which mismatches the destructive section; update the class
on the element that renders the action row (the element using onClick={(event)
=> handleAction(action.onClick, event)}) in the ActionDrawer component so it
uses a subtler divider token such as "border-panel" or "border-edge-muted"
instead of "border-page" to match the destructive section styling.
- Around line 105-107: The console.warn('No drawer state.') in ActionDrawer.tsx
should only run in development to avoid noisy production logs; keep the existing
early-return for drawerState but wrap the warning in a dev-only check (e.g.,
process.env.NODE_ENV !== 'production' or your project's __DEV__ flag) so that
console.warn is invoked only when running in development builds.
🪄 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: 00245315-c006-4610-82b9-6ad69149f88f
📒 Files selected for processing (10)
js/app/AGENTS.mdjs/app/packages/channel/Channel/Channel.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/MessageActionDrawerManager.tsxjs/app/packages/channel/Message/Root.tsxjs/app/packages/channel/Message/index.tsjs/app/packages/channel/Thread/ThreadReplyInput.tsx
Comment on lines
+56
to
+59
| function EmojiSearchView(props: { | ||
| onSelect: (emoji: string) => void; | ||
| onBack: () => void; | ||
| }) { |
Contributor
There was a problem hiding this comment.
Emoji search has no in-drawer path back to action list.
After tapping “More reactions,” users cannot return to the actions pane without closing the drawer. onBack is passed (Line 165) but never used in EmojiSearchView.
Proposed fix
function EmojiSearchView(props: {
onSelect: (emoji: string) => void;
onBack: () => void;
}) {
@@
- <div class="flex items-center gap-2 px-3 pb-2 shrink-0">
+ <div class="flex items-center gap-2 px-3 pb-2 shrink-0">
+ <button
+ type="button"
+ class="px-2 py-1 text-sm text-ink-muted hover:bg-hover rounded-sm"
+ onClick={props.onBack}
+ aria-label="Back to message actions"
+ >
+ Back
+ </button>
<div class="flex flex-1 items-center bg-input border border-edge-muted rounded-sm px-2 py-1.5 text-sm gap-1">Also applies to: 70-83, 160-167
🤖 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 currently receives onBack but never invokes it, preventing users
from returning to the actions pane; update EmojiSearchView to call the provided
onBack when the user taps a back control (e.g., a header back button or
close/back icon) or when the emoji search should exit, and ensure any back UI
element is rendered and wired to invoke props.onBack; locate the EmojiSearchView
component (and related places where onBack is passed, such as the parent that
opens "More reactions") and add the back handler so tapping back triggers onBack
to restore the actions list.
| threadId: props.messageId, | ||
| })} | ||
| markdownNamespace={`thread-reply-input-${props.messageId}-markdown`} | ||
| onReady={(handle) => setTimeout(() => handle.focus(), 300)} |
Contributor
There was a problem hiding this comment.
Clear delayed focus timer to prevent stale refocus.
Line 80 schedules a delayed focus but never cancels it. If the reply input closes within 300ms, the stale callback can refocus a hidden/disposed input and reopen the mobile keyboard unexpectedly.
Proposed fix
-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 focusTimer: number | undefined;
+ onCleanup(() => {
+ if (focusTimer) window.clearTimeout(focusTimer);
+ });
@@
- onReady={(handle) => setTimeout(() => handle.focus(), 300)}
+ onReady={(handle) => {
+ focusTimer = window.setTimeout(() => handle.focus(), 300);
+ }}🤖 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` at line 80, The onReady
handler in ThreadReplyInput schedules a setTimeout to focus the editor but never
cancels it, which can refocus a disposed input; change the implementation to
capture the timeout ID (e.g., via a ref like replyFocusTimeoutRef) when calling
setTimeout in onReady={(handle) => ...}, and clear that timeout with
clearTimeout(replyFocusTimeoutRef.current) in the component cleanup (useEffect
cleanup) and whenever the input is closed or onReady is called again so the
delayed focus is canceled for unmounted/hidden inputs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
js/app/AGENTS.md (1)
57-57:⚠️ Potential issue | 🟠 MajorMissing
cn()usage examples and migration guidance remains unresolved.This new rule is still prescriptive without concrete “do/don’t” examples or integration guidance for adoption in touched files.
As per coding guidelines, "Provide usage examples and integration guidelines for each documented agent."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/AGENTS.md` at line 57, Update the AGENTS.md entry to include concrete "Do" and "Don't" examples demonstrating use of cn() from `@ui/utils/classname` versus SolidJS's classList, plus a short migration guide: show how to import cn, transform common patterns (conditional classes, arrays, dynamic values), and replace classList usages; instruct reviewers to run a repo-wide search for ".classList" and update touched files to use cn() and add a brief note about adding unit/visual checks for class outputs. Reference the cn() helper and the classList prop in the examples and migration steps so reviewers can locate and apply changes.
🤖 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/AGENTS.md`:
- Line 54: Fix the typo in the policy sentence that currently reads "serice
clients" by changing it to "service clients" so the rule reads: "Going forward,
all network calls to service clients should be done through Tanstack query in
the `queries` package. **DO NOT** introduce code that calls any client from
`service-clients` outside of the queries package." This preserves the intended
policy and references the `queries` package and `service-clients` identifier
unchanged.
---
Duplicate comments:
In `@js/app/AGENTS.md`:
- Line 57: Update the AGENTS.md entry to include concrete "Do" and "Don't"
examples demonstrating use of cn() from `@ui/utils/classname` versus SolidJS's
classList, plus a short migration guide: show how to import cn, transform common
patterns (conditional classes, arrays, dynamic values), and replace classList
usages; instruct reviewers to run a repo-wide search for ".classList" and update
touched files to use cn() and add a brief note about adding unit/visual checks
for class outputs. Reference the cn() helper and the classList prop in the
examples and migration steps so reviewers can locate and apply changes.
🪄 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: d41936a5-6b2c-4ca4-b223-55f3b624913e
📒 Files selected for processing (3)
js/app/AGENTS.mdjs/app/packages/channel/Input/ChannelInputContainer.tsxjs/app/packages/channel/Message/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/AGENTS.md`:
- Line 7: Change the section heading string "Developments Notes" to the correct
user-facing wording "Development Notes" in the AGENTS.md content (replace the
heading text "Developments Notes" with "Development Notes" to fix the typo and
ensure consistency).
- Around line 26-56: The Playwright auth section in AGENTS.md conflicts with the
repo's actual test flow: replace the bearer-token route-interception and forced
second navigation instructions with steps that reference the existing Playwright
setup (ensure the setup project runs via playwright.config.ts), use the
configured browser projects (chromium/firefox/webkit) that depend on the setup,
confirm the persisted authentication file playwright/.auth/user.json is used as
storageState, and instruct readers to navigate normally (no route interception
or double navigation); add a note that for real-time tests they should rely on
the authenticated context produced by setup rather than injecting headers.
🪄 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: ccfe081d-3f66-4623-9a43-722cc4918265
📒 Files selected for processing (1)
js/app/AGENTS.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/directive/touchHandler.ts`:
- Line 73: The call to impactFeedback('medium') in touchHandler.ts is
fire-and-forget and can trigger unhandled promise rejections on long-press when
haptics are unavailable; update the invocation of impactFeedback (used in the
long-press handler) to handle rejections explicitly—either await it in an async
handler with try/catch or append a .catch(error => {/* optionally log or noop
*/}) to swallow/log errors—so the promise rejection is consumed and doesn't
surface as an unhandled rejection.
🪄 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: d9aa38e5-815f-465e-bd2a-9e16eb4521e8
📒 Files selected for processing (1)
js/app/packages/core/directive/touchHandler.ts
| timer = window.setTimeout(() => { | ||
| longPressTriggered = true; | ||
| setLongPressActivated(true); | ||
| void impactFeedback('medium'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all haptics call sites with context
rg -n --type ts -C2 '\bimpactFeedback\s*\('
# Flag call sites that don't chain .catch() on same/next line
python - <<'PY'
import pathlib, re
issues = []
for p in pathlib.Path('.').rglob('*.ts'):
lines = p.read_text(encoding='utf-8', errors='ignore').splitlines()
for i, line in enumerate(lines, 1):
if re.search(r'\bimpactFeedback\s*\(', line):
same = '.catch(' in line
nxt = lines[i] if i < len(lines) else ''
if not same and '.catch(' not in nxt:
issues.append((p.as_posix(), i, line.strip()))
for path, lineno, content in issues:
print(f"{path}:{lineno}: {content}")
PYRepository: macro-inc/macro
Length of output: 4153
Handle haptic promise rejection to avoid unhandled promise rejections on long-press.
Line 73 fire-and-forgets impactFeedback('medium') without rejection handling. If haptics is unavailable in a given runtime or device, this surfaces as an unhandled promise rejection on every long-press event.
Suggested fix
- void impactFeedback('medium');
+ impactFeedback('medium').catch(() => {
+ // no-op: haptics may be unavailable on some runtimes/devices
+ });Note: This pattern appears in multiple components across the codebase; consider a global linting rule or utility wrapper for consistent haptics error handling.
📝 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.
Suggested change
| void impactFeedback('medium'); | |
| impactFeedback('medium').catch(() => { | |
| // no-op: haptics may be unavailable on some runtimes/devices | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/core/directive/touchHandler.ts` at line 73, The call to
impactFeedback('medium') in touchHandler.ts is fire-and-forget and can trigger
unhandled promise rejections on long-press when haptics are unavailable; update
the invocation of impactFeedback (used in the long-press handler) to handle
rejections explicitly—either await it in an async handler with try/catch or
append a .catch(error => {/* optionally log or noop */}) to swallow/log
errors—so the promise rejection is consumed and doesn't surface as an unhandled
rejection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Message action drawer opens on long-press.