feat: basic setup react-native-true-sheet#6970
feat: basic setup react-native-true-sheet#6970OtavioStasiak wants to merge 8 commits intodevelopfrom
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughThis pull request migrates the ActionSheet component from Changes
Sequence DiagramsequenceDiagram
participant User
participant ActionSheet as ActionSheet Component
participant TrueSheet as TrueSheet Library
participant DetentCalc as useActionSheetDetents
participant Layout as useResponsiveLayout
User->>ActionSheet: showActionSheet(options)
ActionSheet->>ActionSheet: setData(options)
ActionSheet->>ActionSheet: setIsVisible(true)
ActionSheet->>Layout: Get windowHeight, fontScale
ActionSheet->>DetentCalc: Calculate detents
DetentCalc->>DetentCalc: Call getDetents() with metrics
DetentCalc-->>ActionSheet: Return detents array
ActionSheet->>TrueSheet: present(detents)
TrueSheet-->>User: Show bottom sheet
User->>TrueSheet: Dismiss sheet
TrueSheet->>ActionSheet: onDidDismiss()
ActionSheet->>ActionSheet: Reset state (visibility, contentHeight)
ActionSheet->>ActionSheet: Call data.onClose?()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
force-pushed
the
test.poc-a11y-bottom-shet
branch
from
7c04e79 to
b65245a
Compare
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
force-pushed
the
test.poc-a11y-bottom-shet
branch
from
b65245a to
7d74120
Compare
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
force-pushed
the
test.poc-a11y-bottom-shet
branch
from
2b5b64a to
72a2197
Compare
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
force-pushed
the
test.poc-a11y-bottom-shet
branch
from
2233b87 to
f413d19
Compare
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
force-pushed
the
test.poc-a11y-bottom-shet
branch
from
97914ff to
76cc834
Compare
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/containers/ActionSheet/ActionSheet.tsx`:
- Around line 112-118: The BottomSheetContent usage is passing children via the
children prop (data?.children), which violates React conventions and triggers
the noChildrenProp rule; change the invocation of BottomSheetContent to nest the
children JSX instead (e.g., replace the children={data?.children} prop with
<BottomSheetContent options={data?.options} hide={hide}
hasCancel={data?.hasCancel}
onLayout={handleContentLayout}>{data?.children}</BottomSheetContent>) and update
any related prop typings in the BottomSheetContent component (prop name/type for
children) if necessary.
In `@app/containers/ActionSheet/utils.ts`:
- Around line 18-30: normalizeSnapsToDetents currently treats numeric snaps >1
as already-fractional and clamps them, which incorrectly turns pixel values
(e.g., 480) into 1.0; update normalizeSnapsToDetents to accept a windowHeight
parameter and convert numeric snaps >1 into fractions by dividing by
windowHeight before applying the existing clamp (Math.min(1, Math.max(0.1,
...))), ensure string percentage handling remains unchanged, and update callers
(e.g., useVideoConf/index.tsx) to pass window.innerHeight (or equivalent) or
migrate them to percent/fraction inputs; also make normalizeSnapsToDetents able
to return SheetDetent[] (including 'auto') consistent with getDetents or adjust
getDetents to map the returned number[] into proper SheetDetent entries so
'auto' detents are preserved.
🧹 Nitpick comments (4)
app/containers/ActionSheet/utils.ts (1)
34-43: Minor: magic+ 0.5in item height calculation.The
+ 0.5on line 36 appears to account for item separators or border widths. Consider extracting this as a named constant (e.g.,SEPARATOR_HEIGHT) to clarify its purpose.app/containers/ActionSheet/ActionSheet.tsx (2)
64-75: Consider merging the twouseEffecthooks with the same[isVisible]dependency.Both effects trigger on the same condition (
isVisiblebecoming true). Combining them reduces lifecycle overhead and keeps related side effects co-located.♻️ Proposed merge
useEffect(() => { if (isVisible) { + Keyboard.dismiss(); + Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light); sheetRef.current?.present(0); } }, [isVisible]); - - useEffect(() => { - if (isVisible) { - Keyboard.dismiss(); - Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light); - } - }, [isVisible]);
89-93:onDidDismissandrenderHandleare recreated every render.Neither
onDidDismissnorrenderHandleare wrapped inuseCallback/useMemo, so TrueSheet receives new references each render. SinceActionSheetis wrapped inReact.memo, this only fires on state changes — but wrapping these inuseCallbackwould further reduce unnecessary TrueSheet re-renders when unrelated state (e.g.,contentHeight) changes.package.json (1)
28-28: @lodev09/react-native-true-sheet v3.6.0 is compatible with React Native 0.79—no action needed on compatibility.However,
@gorhom/bottom-sheet@^5.1.6(line 156) appears to be unused in the codebase. No imports or references to this package were found. Consider removing it to reduce dependencies.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
app/containers/ActionSheet/ActionSheet.tsxapp/containers/ActionSheet/BottomSheetContent.tsxapp/containers/ActionSheet/useActionSheetDetents.tsapp/containers/ActionSheet/utils.tsapp/containers/TextInput/FormTextInput.tsxjest.setup.jspackage.jsonpatches/@discord+bottom-sheet+4.6.1.patch
💤 Files with no reviewable changes (1)
- patches/@discord+bottom-sheet+4.6.1.patch
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
app/containers/TextInput/FormTextInput.tsx
🧬 Code graph analysis (2)
app/containers/ActionSheet/useActionSheetDetents.ts (2)
app/containers/ActionSheet/Provider.tsx (1)
TActionSheetOptions(22-33)app/containers/ActionSheet/utils.ts (1)
getDetents(32-61)
app/containers/ActionSheet/utils.ts (1)
app/containers/ActionSheet/Provider.tsx (1)
TActionSheetOptions(22-33)
🪛 Biome (2.3.13)
app/containers/ActionSheet/ActionSheet.tsx
[error] 115-115: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (6)
app/containers/ActionSheet/BottomSheetContent.tsx (1)
44-60: Migration to standard RN FlatList looks correct.The switch from
BottomSheetFlatListto standardFlatListwithnestedScrollEnabled={isAndroid}aligns well with the TrueSheet integration, which manages scroll coordination via itsscrollableprop inActionSheet.tsx.jest.setup.js (1)
158-175: Mock looks good and aligns with actual usage.The
TrueSheetmock correctly providespresent,dismiss, andresizeas Promise-returning methods viauseImperativeHandle, matching the imperative calls inActionSheet.tsx. TheTrueSheetProviderpassthrough is also appropriate.app/containers/ActionSheet/useActionSheetDetents.ts (1)
14-25: Clean hook — notedatareference stability.The implementation is straightforward. Since
datacomes fromuseStateinActionSheet.tsx, its reference is stable between renders (only changing whensetDatais called), so theuseMemodependency works correctly here.app/containers/ActionSheet/utils.ts (1)
55-60: No action required. The'auto'value is a validSheetDetentaccording to the@lodev09/react-native-true-sheetlibrary documentation (v3+), which explicitly supports'auto'for auto-resizing based on content height alongside numeric fractional values.app/containers/TextInput/FormTextInput.tsx (1)
89-89: Removing thebottomSheetprop would break existing callers.While the prop is declared at line 89 and destructured at line 117 but not used within the FormTextInput component itself, multiple files are actively passing this prop to FormTextInput:
app/views/ProfileView/components/ConfirmEmailChangeActionSheetContent/index.tsx:107app/views/ProfileView/components/DeleteAccountActionSheetContent/index.tsx:112app/containers/UIKit/MultiSelect/MultiSelectContent.tsx:75Removing the prop would be a breaking change. If the prop is no longer needed, consider a deprecation approach or verify that the component behavior doesn't depend on the caller's intent in passing this flag.
Likely an incorrect or invalid review comment.
app/containers/ActionSheet/ActionSheet.tsx (1)
100-111: All TrueSheet props are correctly used and align with the official v3+ API:
headeraccepts a JSX element (ReactNode) —renderHandle()is correctly appliedscrollableis a boolean prop —!!data?.optionscorrectly gates this featuredimmedis a boolean prop still supported in v3+ — correctly set totruemaxHeightis a supported number prop that caps expanded height — correctly applies the fractional constraintonDidDismissis the correct v3+ lifecycle callbackNo action needed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <BottomSheetContent | ||
| options={data?.options} | ||
| hide={hide} | ||
| children={data?.children} | ||
| hasCancel={data?.hasCancel} | ||
| onLayout={handleContentLayout} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Pass children via JSX elements instead of the children prop.
As flagged by static analysis (Biome noChildrenProp), passing children via a prop is not the canonical React pattern. Use JSX nesting instead.
♻️ Proposed fix
<BottomSheetContent
options={data?.options}
hide={hide}
- children={data?.children}
hasCancel={data?.hasCancel}
- onLayout={handleContentLayout}
- />
+ onLayout={handleContentLayout}>
+ {data?.children}
+ </BottomSheetContent>📝 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.
| <BottomSheetContent | |
| options={data?.options} | |
| hide={hide} | |
| children={data?.children} | |
| hasCancel={data?.hasCancel} | |
| onLayout={handleContentLayout} | |
| /> | |
| <BottomSheetContent | |
| options={data?.options} | |
| hide={hide} | |
| hasCancel={data?.hasCancel} | |
| onLayout={handleContentLayout}> | |
| {data?.children} | |
| </BottomSheetContent> |
🧰 Tools
🪛 Biome (2.3.13)
[error] 115-115: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🤖 Prompt for AI Agents
In `@app/containers/ActionSheet/ActionSheet.tsx` around lines 112 - 118, The
BottomSheetContent usage is passing children via the children prop
(data?.children), which violates React conventions and triggers the
noChildrenProp rule; change the invocation of BottomSheetContent to nest the
children JSX instead (e.g., replace the children={data?.children} prop with
<BottomSheetContent options={data?.options} hide={hide}
hasCancel={data?.hasCancel}
onLayout={handleContentLayout}>{data?.children}</BottomSheetContent>) and update
any related prop typings in the BottomSheetContent component (prop name/type for
children) if necessary.
| export const normalizeSnapsToDetents = (snaps: (string | number)[]): number[] => | ||
| snaps | ||
| .slice(0, 3) | ||
| .map(snap => { | ||
| if (typeof snap === 'number') { | ||
| if (snap <= 0 || snap > 1) return Math.min(1, Math.max(0.1, snap)); | ||
| return snap; | ||
| } | ||
| const match = String(snap).match(/^(\d+(?:\.\d+)?)\s*%$/); | ||
| if (match) return Math.min(1, Math.max(0.1, Number(match[1]) / 100)); | ||
| return 0.5; | ||
| }) | ||
| .sort((a, b) => a - b); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts --type=tsx 'snaps\s*[:=]' -C 3 | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 105
🏁 Script executed:
# Look at the actual file to understand the function better
cat -n app/containers/ActionSheet/utils.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2509
🏁 Script executed:
# Check Provider.tsx for context about snaps type and usage
rg -n 'snaps' app/containers/ActionSheet/Provider.tsx -C 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 401
🏁 Script executed:
rg -n 'snaps\s*[=:]\s*\[' --type ts -C 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1045
🏁 Script executed:
# Also search for ActionSheet component usage
rg -n 'ActionSheet' --type ts -C 1 | grep -A 2 -B 2 'snaps' | head -60Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 353
🏁 Script executed:
# Look for any comments or migration notes about snaps/pixel values
rg -n 'pixel|detent|snap' app/containers/ActionSheet/ --type tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1110
🏁 Script executed:
cat -n app/lib/hooks/useVideoConf/index.tsx | sed -n '55,75p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 651
🏁 Script executed:
# Check git history or comments around this line
git log --oneline -n 20 -- app/lib/hooks/useVideoConf/index.tsx 2>/dev/null || echo "Git history not available"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 125
🏁 Script executed:
# Check if there are any other references to understanding what 480 means in this context
rg -n '480|windowHeight' app/lib/hooks/useVideoConf/index.tsx -C 5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 375
🏁 Script executed:
# Check if there's a test or if 480 is actually being used
rg -n 'StartACallActionSheet' --type ts -C 3 | head -40Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1712
🏁 Script executed:
# Check the old bottom-sheet or previous implementation for context
rg -n 'bottom.?sheet|BottomSheet' --type ts -i | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2068
🏁 Script executed:
# Check if SheetDetent type is defined locally or imported
fd 'SheetDetent' --type ts -x cat -n {} 2>/dev/null | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check if there's documentation about what the expected snap values should be
rg -n 'TrueSheet|react-native-true-sheet' --type ts -C 2 | head -40Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2025
🏁 Script executed:
cat -n app/containers/ActionSheet/utils.ts | sed -n '18,65p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1943
🏁 Script executed:
# Check ActionSheet.tsx to see how detents is used
cat -n app/containers/ActionSheet/ActionSheet.tsx | sed -n '30,50p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 693
🏁 Script executed:
# Get the exact function signatures and return types
rg -n 'useActionSheetDetents' app/containers/ActionSheet/ -A 10 -B 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3252
🏁 Script executed:
# Verify the type of detents variable
grep -n 'detents.*=' app/containers/ActionSheet/ActionSheet.tsx | head -5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 149
normalizeSnapsToDetents silently converts pixel-based snap values to incorrect detents.
The code supports two snap formats—legacy pixel values (e.g., snaps: [480]) and the new fraction/percentage format. However, line 23's clamping logic is flawed: any numeric value > 1 is treated as a pixel value to clamp, but without windowHeight as a parameter, normalizeSnapsToDetents cannot convert pixels to fractions correctly.
Example: snaps: [480] (used in useVideoConf/index.tsx) becomes Math.min(1, Math.max(0.1, 480)) = 1.0, resulting in a full-height sheet instead of the intended ~480px detent.
To fix this, either:
- Pass
windowHeighttonormalizeSnapsToDetentsand convert pixel values to fractions (e.g.,480 / windowHeight) - Migrate all callers to use fractions/percentages only and reject numeric values > 1
Also note: getDetents returns SheetDetent[] but normalizeSnapsToDetents only returns number[], missing the possibility of 'auto' detents.
🤖 Prompt for AI Agents
In `@app/containers/ActionSheet/utils.ts` around lines 18 - 30,
normalizeSnapsToDetents currently treats numeric snaps >1 as already-fractional
and clamps them, which incorrectly turns pixel values (e.g., 480) into 1.0;
update normalizeSnapsToDetents to accept a windowHeight parameter and convert
numeric snaps >1 into fractions by dividing by windowHeight before applying the
existing clamp (Math.min(1, Math.max(0.1, ...))), ensure string percentage
handling remains unchanged, and update callers (e.g., useVideoConf/index.tsx) to
pass window.innerHeight (or equivalent) or migrate them to percent/fraction
inputs; also make normalizeSnapsToDetents able to return SheetDetent[]
(including 'auto') consistent with getDetents or adjust getDetents to map the
returned number[] into proper SheetDetent entries so 'auto' detents are
preserved.
1 participant
Proposed changes
Use react-native-true-sheet instead of the Discord bottom sheet, because react-native-true-sheet works as expected in accessibility (a11y) terms.
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1809
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Improvements