refactor: migrate MasterDetailStack to React Navigation static config#7414
refactor: migrate MasterDetailStack to React Navigation static config#7414diegolmello wants to merge 2 commits into
Conversation
Convert Drawer + ChatsStack + ModalStack + InsideStack from JSX dynamic navigators to `createNativeStackNavigator`/`createDrawerNavigator` static config. `ModalStack` wraps its Navigator in `<ModalContainer>` via `.with()`, using `useNavigation()` to get the InsideStack navigation for `.pop()`. Views that take a `navigation` prop are wrapped with `withNavigation` at the registration site; no view internals changed. `isMasterDetail: true` injection preserved in `RoomActionsView` and `ReadReceiptsView` options callbacks. Exports `MasterDetailInsideStaticParamList = StaticParamList<typeof InsideStack>`; hand-written param lists in `types.ts` kept as source of truth to avoid a circular type reference. Claude-Session: https://claude.ai/code/session_01Qh9P8Bbp6V7PFpCY2X5on3
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,jsx,ts,tsx,json}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/stacks/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-04-30T17:07:51.020ZApplied to files:
🔇 Additional comments (1)
Walkthrough
ChangesMasterDetailStack Static Navigator Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (2)
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
had a problem deploying
to
android_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/stacks/MasterDetailStack/index.tsx (1)
194-241: 💤 Low valueInconsistent non-null assertions on
navigationOptions.Lines 196 and 231 use non-null assertions (
!) onnavigationOptions, while lines 206 and 240 accessnavigationOptionswithout assertions. This inconsistency could mask potential issues:// With assertion (lines 196, 231) options: props => RoomActionsView.navigationOptions!({ ...props, isMasterDetail: true }) // Without assertion (lines 206, 240) options: SearchMessagesView.navigationOptionsIf the assertion is necessary because
navigationOptionsmight be undefined, consider adding it consistently. If it's guaranteed to exist on all these views, the assertions are unnecessary and could be removed for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/stacks/MasterDetailStack/index.tsx` around lines 194 - 241, The navigationOptions property access is inconsistent across the screen definitions in the stack configuration. Some screens like RoomActionsView and ReadReceiptsView use non-null assertions (!) on navigationOptions, while others like SearchMessagesView and ScreenLockConfigView access navigationOptions without assertions. Make this consistent by either adding the non-null assertion (!) to all navigationOptions accesses across all screens, or removing the assertions from RoomActionsView and ReadReceiptsView if navigationOptions is guaranteed to exist on all view types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/stacks/MasterDetailStack/index.tsx`:
- Around line 194-241: The navigationOptions property access is inconsistent
across the screen definitions in the stack configuration. Some screens like
RoomActionsView and ReadReceiptsView use non-null assertions (!) on
navigationOptions, while others like SearchMessagesView and ScreenLockConfigView
access navigationOptions without assertions. Make this consistent by either
adding the non-null assertion (!) to all navigationOptions accesses across all
screens, or removing the assertions from RoomActionsView and ReadReceiptsView if
navigationOptions is guaranteed to exist on all view types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7d46a68-639f-468f-a23d-5ef168d1e184
📒 Files selected for processing (1)
app/stacks/MasterDetailStack/index.tsx
📜 Review details
⏰ 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 context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/stacks/MasterDetailStack/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/stacks/MasterDetailStack/index.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/stacks/MasterDetailStack/index.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/stacks/MasterDetailStack/index.tsx
app/stacks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place navigation stacks in 'app/stacks/' directory (InsideStack for authenticated, OutsideStack for login/register, MasterDetailStack for tablets, ShareExtensionStack)
Files:
app/stacks/MasterDetailStack/index.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/stacks/MasterDetailStack/index.tsx
🔇 Additional comments (7)
app/stacks/MasterDetailStack/index.tsx (7)
1-23: LGTM!
75-81: LGTM!
82-161: LGTM!
162-177: LGTM!
179-187: LGTM!
262-272: LGTM!
274-319: LGTM!
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
High-effort code review (NATIVE-1304). Behavior-preserving migration — no correctness bugs, nothing blocking.
Verified sound:
ModalStack.with()+useNavigation()resolves to the InsideStack screen navigation (the same object the oldModalStackNavigatorreceived as an injected prop), sonavigation.pop()dismisses the modal identically. The riskiest change is correct.isMasterDetail: truepreserved verbatim on RoomActionsView and ReadReceiptsView.- Screen-name set identical (64) old vs new — nothing dropped or renamed.
screenOptionsmerge-order shift is behavior-neutral (themedHeader sets only headerStyle/headerTintColor/headerTitleStyle).- Leaving the hand-written param lists in
types.tsis the correct call here — deriving them creates a genuine type cycle (views import the lists fromtypes.ts).
All findings are nits (inline below). One more nit, not line-anchored: class-screen handling is inconsistent — RoomActionsView/SearchMessagesView use withNavigation(...) while ReadReceiptsView/ScreenLockConfigView/RoomInfoView are raw ... as any. Runtime-equivalent (RN injects navigation into every screen; withNavigation is a type adapter), but pick one pattern for consistency with the ShareExtension slice.
On-device tablet smoke test remains the verification gate.
| }); | ||
|
|
||
| export default InsideStackNavigator; | ||
| export type MasterDetailInsideStaticParamList = StaticParamList<typeof InsideStack>; |
There was a problem hiding this comment.
slop (moderate): MasterDetailInsideStaticParamList has zero consumers repo-wide — dead exported type. In the earlier slices the derived StaticParamList replaced the hand-written list; here it can't, because deriving creates a cycle (types.ts → views → index.tsx → StaticParamList → types.ts). Keeping the hand-written lists is correct, but this standalone export isn't wired to anything and falsely signals the types are derived. Remove it — or, if it's intended as a seed for the AppContainer-root slice, say so in the PR body.
| } | ||
| // ─── DrawerNavigator ────────────────────────────────────────────────────────── | ||
|
|
||
| const DrawerNav = createDrawerNavigator({ |
There was a problem hiding this comment.
slop (moderate): casting the entire createDrawerNavigator({...}) config to any drops type-checking on screenOptions, screens, and drawerContent alike. The actual gap is just drawerContent: () => <RoomsListView /> vs DrawerContentComponentProps. Scope the cast to drawerContent so the rest of the config stays checked.
| // navigation prop referencing ModalStackParamList ← StaticParamList<typeof ModalStack> | ||
| // ← these components. HOC static properties (navigationOptions) are forwarded by | ||
| // hoistNonReactStatics inside connect() and withTheme(), so `.navigationOptions` | ||
| // is still reachable on the wrapped imports for options callbacks. |
There was a problem hiding this comment.
nit: "reachable on the wrapped imports" is ambiguous. The options callbacks read the raw connect/withTheme-wrapped default exports (which hoist statics), not the withNavigation wrappers (which don't hoist .navigationOptions). Reword to "connect/withTheme-wrapped default exports" so it doesn't imply the HOC hoists.
Remove the dead exported MasterDetailInsideStaticParamList type (no consumers; deriving it would create a types cycle). Drop the createDrawerNavigator config cast so screenOptions/screens/drawerContent stay type-checked. Wrap ReadReceiptsView and ScreenLockConfigView class screens with withNavigation to match the other stacks. Clarify that .navigationOptions is reachable via the connect/withTheme-wrapped default exports, not the withNavigation wrappers. Claude-Session: https://claude.ai/code/session_01FambjUT5Y47f8V6Kc9Fpn5
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
1 participant
Proposed changes
Converts
app/stacks/MasterDetailStack/index.tsxfrom JSX dynamic navigators to React Navigation 7 static config. Four navigators migrated:ChatsStackNavigator,DrawerNavigator,ModalStackNavigator, and the rootInsideStack.Key decisions:
ModalStackwraps itsNavigatorin<ModalContainer>via.with(), callinguseNavigation()inside the callback to get the InsideStack navigation for.pop()on backdrop tap.navigationprop are wrapped withwithNavigationat the registration site — no view internals changed.isMasterDetail: trueis injected inoptionscallbacks forRoomActionsViewandReadReceiptsView, preserving master-detail-specific header behavior.drawerContent: () => <RoomsListView />expressed in static config viaas anycast (DrawerNavigatorProps typing gap).types.tskept as source of truth;StaticParamList<typeof InsideStack>exported asMasterDetailInsideStaticParamListfromindex.tsxfor future use. Replacingtypes.tswith re-exports fromindex.tsxwould create a circular type reference since views import fromtypes.ts.Stacked on #7413 (NATIVE-1302, ShareExtensionStack migration — adds
withNavigationHOC used here). Retarget base todeveloponce #7413 merges.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1304
How to test or reproduce
On a tablet (or iPad simulator):
RoomViewloads in the chats pane.···menu) — modal stack slides in, header shows master-detail back button (not a full dismiss).ModalContainercallsnavigation.pop()and stack dismisses.ReadReceiptsViewheader also shows master-detail style.Screenshots
N/A — no UI changes, internal navigation wiring only.
Types of changes
Checklist
https://claude.ai/code/session_01Qh9P8Bbp6V7PFpCY2X5on3
Summary by CodeRabbit