refactor: migrate ShareExtensionStack to React Navigation static config#7413
refactor: migrate ShareExtensionStack to React Navigation static config#7413diegolmello wants to merge 2 commits into
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
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 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). (2)
Walkthrough
ChangesShareExtensionStack typed navigator refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Reviewed the ShareExtensionStack static-config migration (high effort). No correctness bugs — the refactor is faithful: .with() merges { ...defaultHeader, ...themedHeader(theme) } exactly like the old inline screenOptions (verified against createComponentForStaticNavigation), SelectServerView is a useNavigation() function component so it correctly needs no withNavigation wrapper, route flows through the HOC spread, and getComponent() returns a ComponentType so AppContainer's component={ShareExtensionStack} is untouched. The inlined ShareViewParams matches the removed ShareInsideStackParamList.ShareView shape byte-for-byte. A couple of maintainability notes inline — both slop, not blockers.
| // Cast through `any` to break the type cycle that would arise from ShareListView/ShareView | ||
| // referencing ShareInsideStackParamList ← StaticParamList<typeof ShareExtension> ← these components. | ||
| const ShareListViewScreen: ComponentType<StaticScreenProps<undefined>> = withNavigation(ShareListView as any) as any; | ||
| const ShareViewScreen: ComponentType<StaticScreenProps<ShareViewParams>> = withNavigation(ShareView as any) as any; |
There was a problem hiding this comment.
Slop (moderate) / altitude — foundation note, not a blocker. The trailing as any discards exactly the param-type checking the explicit ComponentType<StaticScreenProps<ShareViewParams>> annotation is meant to give: ShareViewParams is now a hand-maintained duplicate of the route-param contract with no compile-time link to ShareView (whose route is typed against InsideStackParamList['ShareView']). If the two drift, nothing catches it. This is faithful to the old hand-written ShareInsideStackParamList, so it's fine for this slice — but it's the template the InsideStack (~60 screens) and MasterDetailStack slices will copy, so the lost type-safety compounds. The inner withNavigation(... as any) is defensible (silences the connect()/withTheme HOC prop types), but consider whether the outer as any can be avoided before scaling the pattern — e.g. a typed bridge so each screen's params stay verified against their source.
| import { useNavigation } from '@react-navigation/native'; | ||
| import { type ComponentType } from 'react'; | ||
|
|
||
| function withNavigation<P extends { navigation: any }>(WrappedComponent: ComponentType<P>): ComponentType<Omit<P, 'navigation'>> { |
There was a problem hiding this comment.
Slop (low) — foundation note. Two things to be aware of before this HOC carries ~60 screens: (1) navigation: any + untyped useNavigation() means the injected navigation prop is any at the boundary for every wrapped screen — acceptable for a class-component bridge, but it provides no nav typing downstream. (2) It doesn't forward refs. That's correct for React Navigation screens (the static API renders them via MemoizedScreen with only { route }, never a ref), but worth a one-line comment that this is a screen-registration-only HOC so a future caller doesn't reach for it expecting ref passthrough. Otherwise the HOC is sound: stable identity (created once at registration), displayName set, prop spread + navigation-last ordering correct.
Derive ShareViewParams from InsideStackParamList['ShareView'] (omitting the callbacks/params the share extension never provides) so any drift in the source param shape is caught at compile time instead of silently diverging from a hand-maintained duplicate. Document withNavigation as a screen-registration-only HOC that injects navigation via hook and forwards no refs, and explain why the outer screen casts are load-bearing (the static-config param-list type cycle). 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
ShareExtensionStackfrom the dynamic JSX navigator API to React Navigation's static config API (createNativeStackNavigatorwith ascreensobject). Adds a sharedwithNavigationHOC inapp/lib/navigation/that injectsnavigation={useNavigation()}at the screen registration site, allowing class components (ShareListView,ShareView) to keep usingthis.props.navigationwithout internal edits.What changed:
ShareExtensionStack.tsx— static config with.with()theme wrapper;ShareListViewandShareViewwrapped viawithNavigation()at registration;SelectServerViewregistered directly (already usesuseNavigation()internally); default export isShareExtension.getComponent()soAppContaineris untouched.app/lib/navigation/withNavigation.tsx— new generic HOC:withNavigation(Component)returns a component withnavigationinjected viauseNavigation(), all other props forwarded.app/definitions/navigationTypes.ts— hand-writtenShareInsideStackParamListreplaced with a re-export ofStaticParamList<typeof ShareExtension>fromShareExtensionStack; unusedIAttachment/TServerModel/TSubscriptionModelimports removed.Type cycle note:
ShareListViewandShareViewreferenceShareInsideStackParamListin their navigation prop types, which would create a circularStaticParamListreference. Both are cast throughanyat registration with explicitComponentType<StaticScreenProps<...>>annotations so TypeScript can resolve the param list without tracing back through the component props.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1302
How to test or reproduce
Share-extension smoke flow (on-device, separate gate):
ShareListView— confirm the room list loads and the server switcher button navigates toSelectServerView.SelectServerView— confirm it navigates back.ShareView.Screenshots
N/A — no visual changes.
Types of changes
Checklist
Further comments
This is slice 2 of the React Navigation static config migration (slice 1 = OutsideStack, PR #7410).
AppContainerstill uses the dynamic root stack; the.getComponent()bridge is scaffolding that will be removed when the root slice ships.https://claude.ai/code/session_01Qh9P8Bbp6V7PFpCY2X5on3
Summary by CodeRabbit