refactor: migrate AppContainer root to React Navigation static config#7416
refactor: migrate AppContainer root to React Navigation static config#7416diegolmello wants to merge 2 commits into
Conversation
Replace the dynamic NavigationContainer + Stack.Navigator with a fully static root navigator built with createNativeStackNavigator + groups. Each RootEnum branch is a conditional group gated by a useSelector hook (useIsLoading, useIsOutside, useIsMasterDetail, useIsInside, useIsSetUsername, useIsShareExtension). createStaticNavigation converts the config into the AppNavigation component that wraps NavigationContainer and accepts ref, onReady, onStateChange, and theme. The four leaf stacks no longer export a .getComponent() bridge component; they export their navigator config objects directly so they can be nested as screens inside the root groups. The inline SetUsernameStack is folded into the static config as a single-screen navigator. Theme-dependent screenOptions (navigationBarColor) move into a .with() wrapper on the root navigator; static options (headerShown: false, animation: 'none') stay in the config object. Screen tracking (setCurrentScreen / routeNameRef) and MediaCallHeader placement are preserved unchanged. 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)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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)
WalkthroughMigrates ChangesStatic Navigation Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (1)
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/AppContainer.tsx (1)
24-37: Add type annotation to selector hooks using IApplicationState.All conditional-group hooks use
state: any, which bypasses TypeScript's type safety. The codebase definesIApplicationStateinapp/definitions/redux/index.tsand uses it consistently throughout. ReplaceanywithIApplicationStateto gain type safety:Example for useIsLoading
+import { type IApplicationState } from './definitions'; + const useIsLoading = () => useSelector( - (state: any) => state.app.root === RootEnum.ROOT_LOADING || state.app.root === RootEnum.ROOT_LOADING_SHARE_EXTENSION + (state: IApplicationState) => state.app.root === RootEnum.ROOT_LOADING || state.app.root === RootEnum.ROOT_LOADING_SHARE_EXTENSION );Apply the same change to the other five hooks.
🤖 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/AppContainer.tsx` around lines 24 - 37, Replace the `any` type annotation with `IApplicationState` in all six selector hooks (useIsLoading, useIsOutside, useIsMasterDetail, useIsInside, useIsSetUsername, and useIsShareExtension). Update the state parameter in each useSelector call from `state: any` to `state: IApplicationState` to enable proper TypeScript type safety. Ensure that `IApplicationState` is imported from `app/definitions/redux/index.ts` at the top of the file if it is not already imported.
🤖 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/AppContainer.tsx`:
- Around line 24-37: Replace the `any` type annotation with `IApplicationState`
in all six selector hooks (useIsLoading, useIsOutside, useIsMasterDetail,
useIsInside, useIsSetUsername, and useIsShareExtension). Update the state
parameter in each useSelector call from `state: any` to `state:
IApplicationState` to enable proper TypeScript type safety. Ensure that
`IApplicationState` is imported from `app/definitions/redux/index.ts` at the top
of the file if it is not already imported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc20e26a-b538-4cf0-bea0-8396e42c5a80
📒 Files selected for processing (5)
app/AppContainer.tsxapp/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/stacks/OutsideStack.tsxapp/stacks/ShareExtensionStack.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/OutsideStack.tsxapp/stacks/InsideStack.tsxapp/stacks/ShareExtensionStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/AppContainer.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/OutsideStack.tsxapp/stacks/InsideStack.tsxapp/stacks/ShareExtensionStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/AppContainer.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/OutsideStack.tsxapp/stacks/InsideStack.tsxapp/stacks/ShareExtensionStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/AppContainer.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/OutsideStack.tsxapp/stacks/InsideStack.tsxapp/stacks/ShareExtensionStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/AppContainer.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/OutsideStack.tsxapp/stacks/InsideStack.tsxapp/stacks/ShareExtensionStack.tsxapp/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/OutsideStack.tsxapp/stacks/InsideStack.tsxapp/stacks/ShareExtensionStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/AppContainer.tsx
🔇 Additional comments (7)
app/stacks/InsideStack.tsx (1)
393-393: LGTM!app/stacks/MasterDetailStack/index.tsx (1)
317-317: LGTM!app/stacks/OutsideStack.tsx (1)
53-53: LGTM!app/stacks/ShareExtensionStack.tsx (1)
44-44: LGTM!app/AppContainer.tsx (3)
58-63: **Verify'use memo'directive is supported.**The web search didn't show the'use memo'directive being documented in React Navigation. Let me search for React Compiler directives specifically.The search confirms that'use memo'is a valid React Compiler directive. "When a function contains'use memo', the React Compiler will analyze and optimize it during build time. The compiler will automatically memoize values and components to prevent unnecessary re-computations and re-renders.""To enable it for specific components, just add the
'use memo'directive inside the component body."This is a valid React Compiler optimization directive. The usage here in the
.with()callback is appropriate for opt-in compiler optimization.
41-57: LGTM!
69-101: LGTM!
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Review of the AppContainer static-config migration (diff against native-1303-inside-stack-static-config).
One blocking bug: cold-start crash. Every root group is if-gated, but the initial redux state is root: undefined (app/reducers/app.ts initialState). The migration removed the if (!root) return null; guard, so <AppNavigation> now mounts unconditionally on the very first render — before any saga sets root. With root undefined, all six if hooks return false, so the static config passes all-null children to the native-stack Navigator; React.Children.toArray strips the nulls, useNavigationBuilder ends up with zero routes and throws "Couldn't find any screens for the navigator." (@react-navigation/core useNavigationBuilder L392-396). AppContainer is rendered unconditionally in app/index.tsx, and appInit() (which triggers the restore saga that first sets root) is dispatched only after several awaits in Root.init(), so the undefined window is reachable at every cold start. The old dynamic root never mounted the container while !root. Must fix before merge — e.g. restore an early if (!root) return null; in AppContainer, or add a default (non-if) fallback group so at least one screen is always registered.
Everything else checks out:
- Default-export type flip is safe:
AppContaineris the ONLY importer of the default export of OutsideStack/InsideStack/MasterDetailStack/ShareExtensionStack. Other references are type-only imports (MasterDetailStack/types,ShareInsideStackParamList). No stray component consumer, no leftover.getComponent()anywhere. - Branch parity is exact and mutually exclusive: all 6 RootEnum values map 1:1 to the old conditions; the master-detail vs inside split is preserved. The only uncovered state is
undefined(the bug above). - navigationRef / onReady / onStateChange / routeNameRef /
theme={navigationTheme(theme)}all preserved on thecreateStaticNavigationNavigation component; sharednavigationReffromappNavigationunchanged. - MediaCallHeader still a sibling outside the navigator.
- Theme split is correct: static
screenOptions({headerShown, animation}) stay in the base config; theme-dependentnavigationBarColoris read at render in the.with()wrapper and merged by the static NavigatorComponent (nothing frozen at module load). - No
linkingconfig added. connect+memo→useSelectoris sound; memo removal is immaterial here (the component takes no props and already re-renders with its parent).pnpm lint: 0 errors, 175 warnings — identical to baseline, no new warnings.
| <MediaCallHeader /> | ||
| <NavigationContainer | ||
| theme={navTheme} | ||
| <AppNavigation |
There was a problem hiding this comment.
Critical (crash on launch): <AppNavigation> now renders unconditionally, but the redux root is undefined on first render (app/reducers/app.ts initialState; appInit() runs after awaits in Root.init()). With root undefined all six group if hooks return false, the static config emits all-null children, and the native-stack navigator throws "Couldn't find any screens for the navigator" (@react-navigation/core useNavigationBuilder L392-396, after React.Children.toArray drops the nulls). The previous dynamic root guarded this exact window with if (!root) return null; (removed at L73-80 area). Restore that early-return, or add a default non-if fallback group so a screen is always registered when root is undefined.
| Inside: { if: useIsInside, screens: { InsideStack } }, | ||
| SetUsername: { if: useIsSetUsername, screens: { SetUsernameStack } }, | ||
| ShareExtension: { if: useIsShareExtension, screens: { ShareExtensionStack } } | ||
| } |
There was a problem hiding this comment.
Root cause context for the cold-start crash: every group is if-gated, so the union of conditions does not cover root === undefined (the initial state). All 6 RootEnum values are covered and mutually exclusive — only the undefined/cold-start state falls through. A default group with no if (e.g. an AuthLoading/splash screen) would close the gap and also make the navigator resilient to any future unhandled root value.
| const createStackNavigator = createNativeStackNavigator; | ||
| // ─── Conditional-group hooks ────────────────────────────────────────────────── | ||
|
|
||
| const useIsLoading = () => |
There was a problem hiding this comment.
Nit (slop, moderate): (state: any) is repeated across all six selector hooks and the root selector. The old mapStateToProps also used any, so this isn't a regression, but typing these against the app state (e.g. IApplicationState) would restore the type-checking the redux state.app.root/isMasterDetail reads lost in the migration.
When the app cold-starts, redux state.app.root is undefined until the restore saga completes. With all six conditional group hooks returning false the static navigator had no visible screens and threw a native- stack error. Restore the early-return guard that was present in the dynamic root. Claude-Session: https://claude.ai/code/session_01Qh9P8Bbp6V7PFpCY2X5on3
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 the app root from a dynamic
NavigationContainer+Stack.Navigatorto a fully static React Navigation 7 config.Root navigator —
createNativeStackNavigator({ groups })with six conditional groups, each gated by auseSelectorhook that readsstate.app.root(andstate.app.isMasterDetailfor the master-detail split).createStaticNavigationconverts the config intoAppNavigation, a drop-in replacement forNavigationContainerthat acceptsref,onReady,onStateChange, andtheme.Leaf stack bridges removed — all four
.getComponent()calls are gone. Each leaf stack now exports its navigator config object directly so the root can nest it as a static screen:OutsideStack.tsxInsideStack.tsxShareExtensionStack.tsxMasterDetailStack/index.tsxSetUsernameStack — the inline dynamic navigator is folded into the root static config as a single-screen nested navigator.
Theme —
navigationBarColor(theme-dependent) moves into a.with()wrapper on the root;headerShown: falseandanimation: 'none'stay in the static config.navigationTheme(theme)is passed toAppNavigation'sthemeprop unchanged.Preserved unchanged: screen tracking (
setCurrentScreen/routeNameRefviaonReady/onStateChangeand auseEffecton root changes),MediaCallHeaderplacement,emitter.emit('navigationReady'), and the imperative deep-linking path throughnavigationRef.This is the final slice of the React Navigation 7 static-config migration. Stacked on #7415; base retargets to
developonce #7415 merges.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1305
How to test or reproduce
rocketchat://URLs) still navigate to the correct screen.setCurrentScreen).Screenshots
N/A — navigation structure change only; no visual changes.
Types of changes
Checklist
Further comments
No test files exist for
AppContainerorapp/stacks(confirmed viapnpm test --testPathPattern='AppContainer'and--testPathPattern='app/stacks'). Lint: 0 errors, 175 warnings (matches the pre-change baseline).https://claude.ai/code/session_01Qh9P8Bbp6V7PFpCY2X5on3
Summary by CodeRabbit