chore: Upgrade @gorhom/bottom-sheet to 5.2.6 #6729
Conversation
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
WalkthroughAdds extensive Storybook stories and snapshot tests for ActionSheet, ActionSheetContentWithInputAndSubmit, Item, and SupportedVersions components. Migrates bottom sheet usage from @discord/bottom-sheet to @gorhom/bottom-sheet across components, test setup, and dependencies. Updates ActionSheet to override reduce-motion and adjust accessibility props. Updates a legacy patch for @discord/bottom-sheet. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
requested a deployment
to
official_ios_build
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jest.setup.js (1)
153-160: Incomplete mock: Add missing component exports.The mock for
@gorhom/bottom-sheetis missing several components that are actively used in the codebase:
BottomSheetTextInput(imported inFormTextInput.tsx, line 3)BottomSheetView(imported inBottomSheetContent.tsx, line 3)BottomSheetFlatList(imported inBottomSheetContent.tsx, line 3)This will cause test failures when these components are rendered.
Apply this diff to include all required exports:
jest.mock('@gorhom/bottom-sheet', () => { const react = require('react-native'); return { __esModule: true, default: react.View, - BottomSheetScrollView: react.ScrollView + BottomSheetScrollView: react.ScrollView, + BottomSheetTextInput: react.TextInput, + BottomSheetView: react.View, + BottomSheetFlatList: react.FlatList }; });
🧹 Nitpick comments (3)
app/containers/SupportedVersions/SupportedVersionsExpired.stories.tsx (1)
26-42: Consider adding store cleanup to prevent state pollution between stories.Each story dispatches to
mockedStoreon mount without cleanup. If stories are rendered sequentially (e.g., in snapshot tests or when browsing Storybook), the store state from previous stories could leak into subsequent ones.While this may work for isolated snapshot generation, it's better practice to either:
- Reset the store state when the story unmounts
- Use Storybook decorators to provide isolated store instances per story
Example cleanup approach:
export const Default = () => { useEffect(() => { store.dispatch( selectServerSuccess({ server: 'https://open.rocket.chat', version: '7.0.0', name: 'Open ws' }) ); + return () => { + // Reset store state on unmount + store.dispatch(selectServerSuccess({ server: '', version: '', name: '' })); + }; }, []); return ( <Wrapper> <SupportedVersionsExpired /> </Wrapper> ); };This pattern applies to all story variants in this file (lines 44-60, 62-78, 80-96).
app/containers/SupportedVersions/SupportedVersionsWarning.stories.tsx (1)
27-58: Consider adding store cleanup to prevent state pollution between stories.Similar to the concern in
SupportedVersionsExpired.stories.tsx, each story dispatches tomockedStorewithout cleanup. This pattern is repeated across all story variants (lines 27-58, 60-90, 92-120, 122-151, 153-184, 186-217).Consider using Storybook decorators to provide isolated store instances or add cleanup logic in the
useEffectreturn function to reset the store state.Example using a decorator approach:
// Add to the story file export default { title: 'SupportedVersions/SupportedVersionsWarning', decorators: [ (Story) => { // Reset store before each story React.useEffect(() => { store.dispatch(setSupportedVersions({ status: undefined, message: undefined, i18n: undefined, expiration: undefined })); }, []); return <Story />; } ] };app/containers/ActionSheet/ActionSheet.stories.tsx (1)
565-608: Consider adding store cleanup to prevent state pollution.The
WithSupportedVersionsWarningstory dispatches tomockedStorein auseEffecthook without cleanup (lines 569-592). This could cause state pollution if this story is rendered after or before other stories that also interact with the store.Consider adding a cleanup function to reset the store state when the story unmounts, consistent with the recommendation for the SupportedVersions story files.
Example:
export const WithSupportedVersionsWarning = () => { const Component = () => { const { showActionSheet } = useActionSheet(); React.useEffect(() => { store.dispatch( setSupportedVersions({ status: 'warn', message: { remainingDays: 30, type: 'alert', title: 'title_key', subtitle: 'subtitle_key', description: 'description_key', link: 'https://rocket.chat/docs' }, i18n: { en: { title_key: 'Update Required', subtitle_key: 'Your workspace version is outdated', description_key: 'Please update your Rocket.Chat server to continue using this app. Your current version will stop working soon.' } }, expiration: new Date(Date.now() + 30 * 24 * 60 * 60 * 1000).toISOString() }) ); + return () => { + // Reset store state on unmount + store.dispatch(setSupportedVersions({ + status: undefined, + message: undefined, + i18n: undefined, + expiration: undefined + })); + }; }, []); const handlePress = () => { showActionSheet({ children: <SupportedVersionsWarning /> }); }; return <TriggerButton title='Show Supported Versions Warning' onPress={handlePress} />; }; return ( <StoryWrapper> <Component /> </StoryWrapper> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (7)
app/containers/ActionSheet/ActionSheetContentWithInputAndSubmit/__snapshots__/ActionSheetContentWithInputAndSubmit.test.tsx.snapis excluded by!**/*.snapapp/containers/ActionSheet/__snapshots__/ActionSheet.test.tsx.snapis excluded by!**/*.snapapp/containers/ActionSheet/__snapshots__/Item.test.tsx.snapis excluded by!**/*.snapapp/containers/SupportedVersions/__snapshots__/SupportedVersionsExpired.test.tsx.snapis excluded by!**/*.snapapp/containers/SupportedVersions/__snapshots__/SupportedVersionsWarning.test.tsx.snapis excluded by!**/*.snapios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
app/containers/ActionSheet/ActionSheet.stories.tsx(1 hunks)app/containers/ActionSheet/ActionSheet.test.tsx(1 hunks)app/containers/ActionSheet/ActionSheet.tsx(2 hunks)app/containers/ActionSheet/ActionSheetContentWithInputAndSubmit/ActionSheetContentWithInputAndSubmit.stories.tsx(1 hunks)app/containers/ActionSheet/ActionSheetContentWithInputAndSubmit/ActionSheetContentWithInputAndSubmit.test.tsx(1 hunks)app/containers/ActionSheet/BottomSheetContent.tsx(1 hunks)app/containers/ActionSheet/Item.stories.tsx(1 hunks)app/containers/ActionSheet/Item.test.tsx(1 hunks)app/containers/SupportedVersions/SupportedVersionsExpired.stories.tsx(1 hunks)app/containers/SupportedVersions/SupportedVersionsExpired.test.tsx(1 hunks)app/containers/SupportedVersions/SupportedVersionsWarning.stories.tsx(1 hunks)app/containers/SupportedVersions/SupportedVersionsWarning.test.tsx(1 hunks)app/containers/TextInput/FormTextInput.tsx(1 hunks)jest.setup.js(1 hunks)package.json(1 hunks)patches/@discord+bottom-sheet+4.6.1.patch(0 hunks)
💤 Files with no reviewable changes (1)
- patches/@discord+bottom-sheet+4.6.1.patch
🧰 Additional context used
🧬 Code graph analysis (10)
app/containers/ActionSheet/Item.test.tsx (1)
.rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/containers/SupportedVersions/SupportedVersionsExpired.test.tsx (1)
.rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/containers/ActionSheet/ActionSheetContentWithInputAndSubmit/ActionSheetContentWithInputAndSubmit.test.tsx (1)
.rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/containers/SupportedVersions/SupportedVersionsWarning.test.tsx (1)
.rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/containers/ActionSheet/Item.stories.tsx (3)
app/theme.tsx (2)
ThemeContext(18-18)TSupportedThemes(8-8)app/lib/constants/colors.ts (2)
themes(304-304)colors(280-302).rnstorybook/utils.ts (1)
longText(1-2)
app/containers/ActionSheet/ActionSheet.test.tsx (1)
.rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/containers/SupportedVersions/SupportedVersionsWarning.stories.tsx (6)
app/theme.tsx (2)
TSupportedThemes(8-8)ThemeContext(18-18)app/lib/constants/colors.ts (1)
themes(304-304)app/lib/store/auxStore.ts (1)
store(6-6)app/actions/supportedVersions.ts (1)
setSupportedVersions(16-24)app/containers/SupportedVersions/SupportedVersionsWarning.tsx (1)
SupportedVersionsWarning(13-62).rnstorybook/utils.ts (1)
longText(1-2)
app/containers/ActionSheet/ActionSheetContentWithInputAndSubmit/ActionSheetContentWithInputAndSubmit.stories.tsx (3)
app/containers/ActionSheet/Provider.tsx (2)
ActionSheetProvider(56-75)useActionSheet(41-41)app/theme.tsx (2)
ThemeContext(18-18)TSupportedThemes(8-8)app/lib/constants/colors.ts (2)
colors(280-302)themes(304-304)
app/containers/ActionSheet/ActionSheet.stories.tsx (6)
app/containers/ActionSheet/Provider.tsx (2)
ActionSheetProvider(56-75)useActionSheet(41-41).rnstorybook/utils.ts (1)
longText(1-2)app/theme.tsx (2)
ThemeContext(18-18)TSupportedThemes(8-8)app/lib/constants/colors.ts (2)
colors(280-302)themes(304-304)app/actions/supportedVersions.ts (1)
setSupportedVersions(16-24)app/containers/SupportedVersions/SupportedVersionsWarning.tsx (1)
SupportedVersionsWarning(13-62)
app/containers/SupportedVersions/SupportedVersionsExpired.stories.tsx (4)
app/theme.tsx (2)
TSupportedThemes(8-8)ThemeContext(18-18)app/lib/constants/colors.ts (1)
themes(304-304)app/actions/server.ts (1)
selectServerSuccess(47-62)app/containers/SupportedVersions/SupportedVersionsExpired.tsx (1)
SupportedVersionsExpired(19-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (13)
app/containers/TextInput/FormTextInput.tsx (1)
3-3: LGTM! Import path updated correctly.The import of
BottomSheetTextInputhas been successfully migrated from@discord/bottom-sheetto@gorhom/bottom-sheet, and the usage pattern remains compatible.app/containers/ActionSheet/ActionSheet.test.tsx (1)
1-4: LGTM! Standard snapshot testing pattern.The test file follows the established pattern for Storybook-driven snapshot testing, consistent with other ActionSheet test files in this PR.
app/containers/ActionSheet/BottomSheetContent.tsx (1)
3-3: LGTM! Import paths updated correctly.The imports of
BottomSheetViewandBottomSheetFlatListhave been successfully migrated to@gorhom/bottom-sheet. The usage remains unchanged and compatible.app/containers/ActionSheet/ActionSheetContentWithInputAndSubmit/ActionSheetContentWithInputAndSubmit.stories.tsx (1)
1-413: LGTM! Comprehensive story coverage.The stories file provides excellent coverage of the
ActionSheetContentWithInputAndSubmitcomponent with:
- Various input configurations (single, secure, multiple)
- Theme variants (light, dark, black)
- Edge cases (confirmation-only, custom validation, custom colors)
- Proper ActionSheetProvider wrapping
The implementation follows the established pattern used in other ActionSheet stories.
app/containers/ActionSheet/ActionSheetContentWithInputAndSubmit/ActionSheetContentWithInputAndSubmit.test.tsx (1)
1-4: LGTM! Standard snapshot testing pattern.The test file follows the established pattern for Storybook-driven snapshot testing, consistent with other ActionSheet test files.
app/containers/ActionSheet/Item.test.tsx (2)
1-4: LGTM! Standard snapshot testing pattern.The test file follows the established pattern for Storybook-driven snapshot testing, consistent with other ActionSheet test files.
1-4: Explicitly disable v5 dynamic sizing on ActionSheet BottomSheets
All ActionSheet BottomSheets supplysnapPointsbut don’t setenableDynamicSizing={false}(default true in v5 may override static points)—add the prop and verify layout and snapping behavior across screens.app/containers/SupportedVersions/SupportedVersionsExpired.test.tsx (1)
1-4: LGTM!The test file correctly follows the established snapshot-generation pattern for Storybook stories.
app/containers/SupportedVersions/SupportedVersionsWarning.test.tsx (1)
1-4: LGTM!The migration to snapshot-based testing using Storybook stories is correctly implemented and follows the established pattern.
app/containers/ActionSheet/ActionSheet.tsx (2)
5-6: LGTM on the library migration.The import changes correctly migrate from
@discord/bottom-sheetto@gorhom/bottom-sheet, and the addition ofReduceMotionimport is appropriate for the override below.
170-174: Clarify suppression of accessibility props and reduce-motion override
In ActionSheet.tsx (lines 170–174), you’re explicitly settingaccessible,accessibilityRole,accessibilityLabel, andaccessibilityHinttoundefinedand forcingoverrideReduceMotion={ReduceMotion.Never}, which conflicts with WCAG SC 2.2.2/2.3.3 and accessible name requirements. Please explain:
- Why these accessibility props are being suppressed
- Why reduce-motion is disabled despite user preferences
- How this change supports the PR’s stated accessibility improvements
app/containers/ActionSheet/Item.stories.tsx (1)
1-351: LGTM!The story file provides comprehensive coverage of the
Itemcomponent states and variants. The structure is clean, stories are well-organized, and the theme variants effectively demonstrate the component across different color schemes.app/containers/ActionSheet/ActionSheet.stories.tsx (1)
1-564: LGTM!The story file provides excellent coverage of ActionSheet functionality with diverse scenarios including basic options, icons, subtitles, right components, cancel buttons, danger options, disabled states, mixed states, custom headers, long lists, custom children with snap positions, callbacks, and theme variants. The structure is clear and demonstrates real-world usage patterns effectively.
| "dependencies": { | ||
| "@bugsnag/react-native": "8.4.0", | ||
| "@discord/bottom-sheet": "bluesky-social/react-native-bottom-sheet", | ||
| "@gorhom/bottom-sheet": "^5", |
There was a problem hiding this comment.
🧩 Analysis chain
Resolve version inconsistency and duplicate entries.
The package appears in both dependencies (line 36) and devDependencies (line 166) with different version specifiers:
dependencies:"^5"devDependencies:"^5.1.6"
This inconsistency can lead to unexpected behavior:
- The
"^5"specifier will resolve to the latest 5.x version - The PR title mentions 5.2.6, but neither entry pins to that specific version
- Having the same package in both sections is redundant
Apply this diff to use a consistent version and remove the duplicate:
- "@gorhom/bottom-sheet": "^5",
+ "@gorhom/bottom-sheet": "5.2.6",Then verify the devDependencies entry at line 166 and consider removing it if the package should only be a runtime dependency, or update it to match if it's intentionally kept separate.
Additionally, run the following to confirm the installed version matches expectations:
🏁 Script executed:
#!/bin/bash
# Check the actual version that would be resolved
npm view @gorhom/bottom-sheet versions --json | jq '.[] | select(startswith("5"))' | tail -5Length of output: 39
Pin @gorhom/bottom-sheet to 5.2.6 and remove duplicate entry.
- Remove the
@gorhom/bottom-sheetentry from devDependencies (line 166). - Update the
dependenciesentry (line 36) to:
"@gorhom/bottom-sheet": "5.2.6",🤖 Prompt for AI Agents
In package.json around line 36 (and also remove duplicate at line 166), the
@gorhom/bottom-sheet dependency is duplicated and currently uses a caret range;
update the dependencies entry at line 36 to pin the package to the exact version
"5.2.6" (no caret) and remove the duplicate @gorhom/bottom-sheet entry from
devDependencies at line 166 so the package appears only once in dependencies
with the fixed version.
|
Android Build Available Rocket.Chat Experimental 4.66.0.107568 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSmW9_xAPtA2KZwnJBY1AktZHxDeeY2h4VlDKp_YcBYy2aqhgwy94-voCnCwB_WLujx8D-lVvhMlXzIcvvw |
1 participant
Proposed changes
We were actually using @discord/bottom-sheet, a fork with intention to add a11y that Bluesky used, but since they move away from it, we should as well.
Added stories to action sheet, but snapshots aren't captured correctly (probably because of portal), so I added stories to SupportedVersionsWarning, a component that is used by action sheet as children. There's also an action sheet story that opens it, but without snapshot.
Hopefully, it's enough.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1062
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit