Open
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis pull request refactors and enhances mobile UI handling across several components. Changes include reworking focus input scrolling logic in MobileDrawer, adding mobile-specific layouts for channel forwarding and sharing features, adjusting switch styling for touch devices, and standardizing prop naming across shared components used in mobile experiences. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
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/app/component/mobile/MobileDrawer.tsx (1)
19-35:⚠️ Potential issue | 🟠 MajorModule-level
capturedflag creates a cross-drawer race and drops legitimate scrolls.
capturedis declared at module scope, so it is shared across every call site ofscrollToFocusedInput(this drawer,mobile-filter-drawer.tsx, and any future consumer). Two real problems follow:
- Rapid focus changes are silently dropped. If the user focuses input A and then focuses input B within the 300 ms window (e.g. keyboard
Tab, programmatic focus, autofill), B's focus event early-returns becausecaptured === true, and the scheduled scroll runs for A — whose focus may no longer be current.- Cross-instance interference. If a share drawer and the filter drawer are both mounted, focusing in one while the other's timer is pending swallows the scroll in the second.
Additionally, if anything throws between
captured = trueand thesetTimeoutcallback running,capturednever resets and the utility is permanently disabled for the tab.Prefer tracking the latest event and using
clearTimeout, or scope state per container via aWeakMap<HTMLElement, number>.🛠️ Proposed fix
-let captured = false; -export function scrollToFocusedInput(e: FocusEvent, offset = 40) { - if (!isEditableInput(e.target as Element) || captured) return; - const input = e.target as HTMLElement; - const container = e.currentTarget as HTMLElement; - captured = true; - // Has to be delayed until after browser's native keyboard-show scroll completes - setTimeout(() => { - const inputRect = input.getBoundingClientRect(); - const containerRect = container.getBoundingClientRect(); - container.scrollTo({ - top: container.scrollTop + (inputRect.top - containerRect.top) - offset, - behavior: 'smooth', - }); - captured = false; - }, 300); -} +const pending = new WeakMap<HTMLElement, { timer: number; input: HTMLElement }>(); +export function scrollToFocusedInput(e: FocusEvent, offset = 40) { + if (!isEditableInput(e.target as Element)) return; + const input = e.target as HTMLElement; + const container = e.currentTarget as HTMLElement; + const existing = pending.get(container); + if (existing) clearTimeout(existing.timer); + // Has to be delayed until after browser's native keyboard-show scroll completes + const timer = window.setTimeout(() => { + pending.delete(container); + const latest = pending.get(container)?.input ?? input; + const inputRect = latest.getBoundingClientRect(); + const containerRect = container.getBoundingClientRect(); + container.scrollTo({ + top: container.scrollTop + (inputRect.top - containerRect.top) - offset, + behavior: 'smooth', + }); + }, 300); + pending.set(container, { timer, input }); +}🤖 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 19 - 35, The module-level captured flag in scrollToFocusedInput causes dropped/nonspecific scrolls and cross-instance interference; replace it with per-container timer tracking (e.g., a WeakMap<HTMLElement, number> or Map to store timeout IDs keyed by the container element) so each container manages its own pending timeout, clear any existing timeout with clearTimeout before scheduling a new setTimeout, and ensure the timeout ID is removed from the map in the callback (and in any error path) so state is not permanently stuck; reference the existing scrollToFocusedInput function, the captured variable, and the setTimeout/clearTimeout usage when implementing this change.
🤖 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 53-60: The onFocusOut handler on Drawer.Content currently calls
setInputFocused(false) unconditionally causing a flicker when focus moves
between inputs; update the onFocusOut implementation (the Drawer.Content handler
that complements onFocusIn and uses setInputFocused and isEditableInput) to only
clear the signal when the focus is leaving to a non-editable element by
inspecting the FocusEvent.relatedTarget (or alternatively defer clearing via a
microtask and cancel if a subsequent onFocusIn occurs); ensure you reference the
same helpers (isEditableInput, scrollToFocusedInput) and keep the onFocusIn
logic intact so transitions between inputs inside the drawer do not transiently
set inputFocused to false.
In `@js/app/packages/core/component/ForwardToChannel.tsx`:
- Around line 67-78: The mobile recipient chip list is falling back to wrapping
because the horizontalScroll prop was left commented out on the
RecipientSelector component; enable horizontal scrolling by un-commenting and
passing horizontalScroll to RecipientSelector (the component instance in
ForwardToChannel.tsx) so the selector uses the horizontal chip scrollbar
behavior for mobile; ensure any related CSS/class props (e.g., class="border-1
border-edge-muted/50 p-1") remain compatible with horizontalScroll.
- Around line 80-123: The checkbox proxy lacks a visible focus state and is
duplicated; extract the UI into a single SendAsGroupToggle component (used by
ForwardToChannel) that uses props.canSendAsGroup(), props.sendAsGroupMessage(),
and props.setSendAsGroupMessage() and renders the same structure with CheckIcon,
then add focus-visible styling via the existing hidden input's peer classes
(e.g., use peer and peer-focus-visible:... or peer-focus:... on the proxy div to
show an outline/ring when keyboard-focused) so keyboard users see focus, and
replace both duplicated blocks with this new SendAsGroupToggle.
In `@js/app/packages/core/component/RecipientSelector.tsx`:
- Around line 495-496: The long template string building the class prop for the
chips should be split using the imported cn helper for readability: replace the
inline ternary in the class attribute on the element that uses
ref={props.horizontalScroll ? setChipsScrollRef : undefined} with a cn(...) call
that concisely lists shared classes and conditionally applies the horizontal
layout classes when props.horizontalScroll is true and the vertical/wrapped
layout classes otherwise; keep the ref logic referencing setChipsScrollRef
unchanged and ensure the two mode groups mirror the original class sets (shared:
"flex gap-1.5 text-ink scrollbar-hidden", horizontal: "flex-nowrap
overflow-x-auto sm:flex-wrap sm:overflow-x-hidden sm:max-h-[150px]
sm:overflow-y-auto pb-[2px] sm:pb-0", vertical: "flex-wrap max-h-[150px]
overflow-y-auto").
In `@js/app/packages/core/component/TopBar/ShareButton.tsx`:
- Around line 385-529: The mobile "People" and "Link" sections duplicate desktop
markup; extract a RecipientRow component and a PublicLinkSection component and
reuse them in both desktop and mobile layouts. Implement RecipientRow to render
the recipient entry (use props: recipient, channelNameMap, navigateToChannel,
removeChannelAccess, setChannelPermissions, and include the
Switch/Match/DmRecipientIcon/WideUsers, GroupChannelLabel and ShareOptions usage
shown), and implement PublicLinkSection to render the public-link area (use
props: publicAccessLevel, setPublicPermissions, copyPublicLink and include the
MiniToggleSwitch, ShareOptions and Copy button logic). Update the mobile and
desktop consumers to import and render RecipientRow for each recipient and
PublicLinkSection where the public link block appears, leaving layout containers
(ClippedPanel vs div) unchanged and preserving existing prop names like
MobileShareDrawerProps, ShareOptions, GroupChannelLabel, MiniToggleSwitch,
copyPublicLink, setPublicPermissions, navigateToChannel, removeChannelAccess,
setChannelPermissions.
- Around line 299-302: The createEffect currently derives state by reading
mobileTabs() and calling setActiveTab('share') when the active tab disappears;
replace this with a createMemo that computes an "effective" active tab from
mobileTabs() and activeTab() (do not call setActiveTab inside it) and have
consumers use that memoized value instead of activeTab(); keep the original
activeTab signal as the user's chosen tab, expose e.g. effectiveActiveTab =
createMemo(() => { ... }) which returns activeTab() when still present or the
fallback 'share' when not, and update all usages to read effectiveActiveTab
rather than relying on the effect-driven setActiveTab.
- Around line 316-322: Remove the problematic initialFocusEl usage that calls
document.querySelector synchronously during render: delete the initialFocusEl
prop branch around the MobileDrawer/Portal usage (the code referencing
initialFocusEl and the selector '[data-share-drawer-recipient] input'), or
replace it by scoping focus lookup via a ref passed into MobileDrawer if you
intend to keep it; rely on the existing ShareTrigger focusInput directive for
mobile focus. Also avoid relying on the global data-share-drawer-recipient
selector — if you must query, scope it to the drawer's root element (provided by
a ref) to disambiguate the two ForwardToChannel renderings. Ensure references to
initialFocusEl, MobileDrawer.Portal, ShareTrigger, focusInput, and
ForwardToChannel are updated accordingly.
---
Outside diff comments:
In `@js/app/packages/app/component/mobile/MobileDrawer.tsx`:
- Around line 19-35: The module-level captured flag in scrollToFocusedInput
causes dropped/nonspecific scrolls and cross-instance interference; replace it
with per-container timer tracking (e.g., a WeakMap<HTMLElement, number> or Map
to store timeout IDs keyed by the container element) so each container manages
its own pending timeout, clear any existing timeout with clearTimeout before
scheduling a new setTimeout, and ensure the timeout ID is removed from the map
in the callback (and in any error path) so state is not permanently stuck;
reference the existing scrollToFocusedInput function, the captured variable, and
the setTimeout/clearTimeout usage when implementing this change.
🪄 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: 0969f2f4-32fb-40bd-9fa6-ea962f39edb9
📒 Files selected for processing (5)
js/app/packages/app/component/mobile/MobileDrawer.tsxjs/app/packages/core/component/FormControls/MiniToggleSwitch.tsxjs/app/packages/core/component/ForwardToChannel.tsxjs/app/packages/core/component/RecipientSelector.tsxjs/app/packages/core/component/TopBar/ShareButton.tsx
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.
No description provided.