refactor: migrate MasterDetailStack to React Navigation static config - #7414
Conversation
Converts ShareExtensionStack from the dynamic JSX API to the static
config API (createNativeStackNavigator with screens object). Introduces
a shared withNavigation HOC in app/lib/navigation/ that injects
navigation={useNavigation()} at the screen registration site, allowing
class components (ShareListView, ShareView) to keep consuming
this.props.navigation without internal edits.
- ShareListView and ShareView are wrapped via withNavigation() at
registration; cast through `any` to break the type cycle that would
arise from their NativeStackNavigationProp<ShareInsideStackParamList>
props referencing StaticParamList<typeof ShareExtension> ← themselves.
- ShareViewParams is declared inline so StaticParamList correctly derives
ShareView route params from the explicit ComponentType annotation.
- ShareInsideStackParamList is now derived via StaticParamList<typeof
ShareExtension> and re-exported from navigationTypes.ts; the hand-written
definition and its IAttachment/TServerModel/TSubscriptionModel imports
are removed.
- Theme spread moves into the .with() callback (themedHeader applied via
Navigator screenOptions); defaultHeader stays in the static config.
- Default export is ShareExtension.getComponent() so AppContainer is
untouched during this slice.
Claude-Session: https://claude.ai/code/session_01Qh9P8Bbp6V7PFpCY2X5on3
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
GitHub Actions
Error
|
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:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughNavigation stacks across ChangesStatic Hook-Based Navigation Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
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.
| // 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
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Error
Drop the import-section labels and navigator box-drawing dividers; condense the withNavigation block to why the cast is needed and why .navigationOptions stays reachable through connect()/withTheme(). Claude-Session: https://claude.ai/code/session_01YGusFn31iyN58idszGKMgt
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
496901f to
d3af71d
Compare
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@app/stacks/InsideStack.tsx`:
- Around line 110-150: The E2EEToggleRoomViewScreen assignment does not wrap
E2EEToggleRoomView with the withNavigation higher-order component. Since
E2EEToggleRoomView receives a navigation prop and calls navigation.setOptions()
in a useLayoutEffect (rather than using the useNavigation hook like other
components), it requires the withNavigation wrapper to function properly with
React Navigation's static API. Wrap E2EEToggleRoomView with withNavigation
before casting it to any, similar to how other components that directly use the
navigation prop are handled.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4e08abc-0d7d-48f4-b7da-83aeb642d97a
📒 Files selected for processing (2)
app/stacks/InsideStack.tsxapp/stacks/types.ts
✅ Files skipped from review due to trivial changes (1)
- app/stacks/types.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (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/InsideStack.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/InsideStack.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/InsideStack.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/InsideStack.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/InsideStack.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/InsideStack.tsx
🔇 Additional comments (6)
app/stacks/InsideStack.tsx (6)
1-16: LGTM!Also applies to: 42-79
152-156: LGTM!Also applies to: 161-167, 172-189, 194-238, 243-276
277-296: LGTM!Also applies to: 300-344
346-367: LGTM!Also applies to: 372-386
297-299: 🎯 Functional CorrectnessNo action required.
SidebarViewaccesses drawer state throughnavigation.getState()and listens to navigation state changes, not via a prop. The refactor is correct and maintains full functionality.
157-160: 🎯 Functional CorrectnessLikely an incorrect or invalid review comment.
…-detail-static-config # Conflicts: # app/lib/navigation/withNavigation.tsx # app/stacks/InsideStack.tsx # app/stacks/ShareExtensionStack.tsx
GitHub Actions
Failure
GitHub Actions
Failure
2 participants
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