Skip to content

fix: add content modal wrapper for expo router issues#549

Merged
ignaciosantise merged 6 commits intodevelopfrom
fix/expo-router-modal-wrapper
Apr 10, 2026
Merged

fix: add content modal wrapper for expo router issues#549
ignaciosantise merged 6 commits intodevelopfrom
fix/expo-router-modal-wrapper

Conversation

@ignaciosantise
Copy link
Copy Markdown
Collaborator

@ignaciosantise ignaciosantise commented Apr 9, 2026

Summary

  • add an optional contentModalWrapper prop to AppKit so apps can wrap the internal RN modal content with a custom window overlay such as FullWindowOverlay
  • add regression coverage for the wrapped and unwrapped AppKit modal paths
  • fix Jest workspace module resolution so package tests resolve internal @reown/appkit-* imports from src during tests

Testing

  • yarn format
  • yarn lint
  • yarn test

Copilot AI review requested due to automatic review settings April 9, 2026 14:18
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 9, 2026

🦋 Changeset detected

Latest commit: f6b43cc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@reown/appkit-bitcoin-react-native Patch
@reown/appkit-coinbase-react-native Patch
@reown/appkit-common-react-native Patch
@reown/appkit-core-react-native Patch
@reown/appkit-ethers-react-native Patch
@reown/appkit-react-native Patch
@reown/appkit-solana-react-native Patch
@reown/appkit-ui-react-native Patch
@reown/appkit-wagmi-react-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

Claude finished @ignaciosantise's task —— View job


PR Review: fix/expo-router-modal-wrapper

Tasks:

  • Gather context and understand the PR
  • Review changed files
  • Post review feedback

Issues Found

1. handleBackPress and handleModalClose not memoized — packages/appkit/src/modal/w3m-modal/index.tsx:39-58

handleBackPress is an inline function and handleModalClose is an async function, both recreated on every render. They're passed as props to <Modal>, which will cause unnecessary re-renders. Both should be wrapped in useCallback.

const handleModalClose = useCallback(async () => {
  await close();
}, [close]);

const handleBackPress = useCallback(() => {
  if (RouterController.state.history.length > 1) {
    return RouterController.goBack();
  }
  return handleModalClose();
}, [handleModalClose]);

2. Mismatched changeset scope — .changeset/sharp-cars-approve.md

The changeset only bumps @reown/appkit-react-native, but the bot report shows @reown/appkit-ethers-react-native and @reown/appkit-wagmi-react-native are also included. These two adapters don't appear to have any changes related to this PR. The changeset description should match the actual scope.

3. Inline style in example — examples/expo-multichain/app/_layout.tsx:102

The workaround uses an inline style object:

<View style={{ position: 'absolute', height: '100%', width: '100%' }}>

Per project conventions, inline styles should be avoided. This should be extracted to a StyleSheet.create call. Since this is an example app, it's lower priority, but worth aligning with project conventions.

4. Modal mock doesn't simulate visible prop — packages/appkit/src/__tests__/modal/w3m-modal.test.tsx:46

The Modal mock renders children unconditionally regardless of the visible prop:

Modal: ({ children, testID }: any) => <MockView testID={testID}>{children}</MockView>,

This means tests can't catch regressions where the modal renders its contents when it should be hidden (e.g., open: false). The mock should respect the visible prop to be a meaningful test.

5. Unused visible in mock modal test when open: false path is untested

The mock state hardcodes open: true. There's no test for the case where ModalController.state.open is false, which would verify the modal content is not rendered when closed. Given that this PR specifically adds regression coverage, the closed state should be tested too.


Minor Notes

  • The { virtual: true } option on jest.mock calls in the new test file mirrors the pattern in useAppKitTheme.test.tsx — this is consistent.
  • The Jest config changes (propagating root moduleNameMapper to package-level configs via spread) are correct and well-structured.
  • The AppKitModalWrapperProps / AppKitModalWrapperComponent types being exported from index.ts is good API design — allows consumers to type their wrapper correctly.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for optionally wrapping AppKit’s internal modal (to work around Expo Router modal layering issues) and updates Jest config so workspace packages resolve internal @reown/appkit-* imports from source during tests.

Changes:

  • Add optional modalWrapper prop to AppKit and export the related public types.
  • Add regression tests for wrapped vs unwrapped AppKit modal rendering and adjust an existing hook test mock.
  • Update root + package Jest configs to map @reown/appkit-* modules to packages/*/src during tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/ui/jest.config.ts Reuses root Jest config and preserves moduleNameMapper while overriding shared setup path.
packages/core/jest.config.ts Same Jest config consolidation to inherit root mappers.
packages/common/jest.config.ts Same Jest config consolidation to inherit root mappers.
packages/appkit/jest.config.ts Same Jest config consolidation to inherit root mappers.
jest.config.ts Adds moduleNameMapper entries mapping @reown/appkit-* to workspace src entrypoints for tests.
packages/appkit/src/modal/w3m-modal/index.tsx Introduces modalWrapper prop to optionally wrap the internal modal tree.
packages/appkit/src/index.ts Re-exports AppKitProps and wrapper-related types as part of the public API.
packages/appkit/src/tests/modal/w3m-modal.test.tsx Adds regression coverage for wrapped/unwrapped modal rendering.
packages/appkit/src/tests/hooks/useAppKitTheme.test.tsx Updates core module mock to be virtual (aligning with new mapper behavior).
examples/expo-multichain/app/_layout.tsx Updates guidance comment to mention modalWrapper for Expo Router layering issues.
.changeset/sharp-cars-approve.md Changeset for patch release including the new modalWrapper prop.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ignaciosantise
Copy link
Copy Markdown
Collaborator Author

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

Claude finished @ignaciosantise's task in 2m 2s —— View job


PR Review: fix/expo-router-modal-wrapper

Tasks:

  • Gather context
  • Read changed files
  • Analyze and post review

Found 5 issue(s)

Issue 1: handleBackPress and handleModalClose not memoized

ID: w3m-modal-missing-usecallback-a3f1
File: packages/appkit/src/modal/w3m-modal/index.tsx:39-58
Severity: MEDIUM
Category: performance

Context:

  • Pattern: Both handleBackPress (line 39) and handleModalClose (line 56) are inline const function expressions recreated on every render and passed as onRequestClose/onBackdropPress props to <Modal>
  • Risk: New function references on every render will defeat referential equality checks inside Modal, triggering unnecessary re-renders whenever the parent re-renders (e.g. theme/snapshot changes)
  • Impact: Extra re-renders of the modal and its subtree on every state snapshot update
  • Trigger: Every call to useSnapshot that causes a re-render (open/theme/projectId state changes)

Recommendation: Fix this →

const handleModalClose = useCallback(async () => {
  await close();
}, [close]);

const handleBackPress = useCallback(() => {
  if (RouterController.state.history.length > 1) {
    return RouterController.goBack();
  }
  return handleModalClose();
}, [handleModalClose]);

Note: handleModalClose must be declared before handleBackPress for the dependency to resolve correctly.


Issue 2: Changeset bumps all packages despite only @reown/appkit-react-native changing

ID: changeset-scope-all-packages-b2e4
File: .changeset/sharp-cars-approve.md:1-11
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: Changeset lists all 10 packages (@reown/appkit-bitcoin-react-native, @reown/appkit-coinbase-react-native, @reown/appkit-core-react-native, etc.) for a patch bump
  • Risk: Packages with no code changes get a version bump, polluting changelogs and potentially causing consumers to receive unnecessary updates
  • Impact: Misleading changelog entries across unrelated packages; increases noise in downstream dependency updates
  • Trigger: On every release cut — all packages get a new version with a misleading "feat: add modalWrapper prop" entry even though they didn't change

Recommendation: Scope the changeset to only the affected package:

---
'@reown/appkit-react-native': patch
---

fix: support expo router modal wrappers

Issue 3: Changeset type feat conflicts with PR type fix

ID: changeset-feat-vs-fix-c7d2
File: .changeset/sharp-cars-approve.md:14
Severity: LOW
Category: code_quality

Context:

  • Pattern: Changeset description says feat: add modalWrapper prop but PR title is fix: support expo router modal wrappers
  • Risk: Inconsistent commit/changelog messaging; if semantic-release or changelog tooling uses the changeset description, it will appear as a feat in the changelog despite being filed as a fix
  • Impact: Changelog inconsistency

Recommendation: Align the description: fix: support expo router modal wrappers


Issue 4: Modal mock ignores visible prop — tests can't catch hidden-but-rendering regressions

ID: w3m-modal-test-visible-ignored-d5a3
File: packages/appkit/src/__tests__/modal/w3m-modal.test.tsx:46
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: Mock renders children unconditionally: Modal: ({ children, testID }: any) => <MockView testID={testID}>{children}</MockView>visible is not consumed
  • Risk: Tests will pass even if the real Modal always renders content regardless of visible={false}, masking regressions where the modal renders when it should be hidden
  • Impact: False confidence in the "modal is hidden when closed" path
  • Trigger: Any future change that incorrectly passes visible or removes the open state gate

Recommendation: Fix this →

Modal: ({ children, testID, visible }: any) =>
  visible ? <MockView testID={testID}>{children}</MockView> : null,

Then add a test for the open: false path:

it('does not render modal content when closed', () => {
  // Override ModalController state to closed
  jest.mocked(useSnapshot).mockImplementationOnce((state: any) =>
    state === ModalController.state ? { open: false } : state
  );
  const { queryByTestId } = render(<AppKit />);
  expect(queryByTestId('w3m-modal')).toBeNull();
});

Issue 5: Inline style in example app violates project conventions

ID: layout-inline-style-e8f0
File: examples/expo-multichain/app/_layout.tsx:102
Severity: LOW
Category: code_quality

Context:

  • Pattern: <View style={{ position: 'absolute', height: '100%', width: '100%' }}> uses an object literal style
  • Risk: Per project guidelines ("no inline styles"), this creates a new object on every render and diverges from StyleSheet.create conventions used throughout the codebase
  • Impact: Minor perf + convention inconsistency in example code that developers may copy

Recommendation: Fix this →

const styles = StyleSheet.create({
  modalOverlay: { position: 'absolute', height: '100%', width: '100%' }
});
// ...
<View style={styles.modalOverlay}>

@ignaciosantise
Copy link
Copy Markdown
Collaborator Author

  1. handleBackPress / handleModalClose should use useCallback:
    This is not a meaningful optimization here. AppKit already rerenders on controller/theme changes, the handlers are cheap, and this repo’s React guidance is to avoid useCallback by default unless there is a demonstrated need. Adding memoization here would be noise.

  2. Inline style in examples/expo-multichain/app/_layout.tsx should be a StyleSheet
    This is a low-value example-only cleanup, not specific to the bug fix. The example already used this small inline wrapper pattern, and converting it does not improve correctness or reviewability for this PR.

@ignaciosantise ignaciosantise changed the title fix: support expo router modal wrappers fix: add content modal wrapper for expo router issues Apr 10, 2026
@ignaciosantise
Copy link
Copy Markdown
Collaborator Author

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Apr 10, 2026

Claude finished @ignaciosantise's task in 2s —— View job


PR Review: fix/expo-router-modal-wrapper

Tasks:

  • Gather context
  • Read changed files
  • Analyze and post review

🔄 Incremental Review — checking state of previously flagged issues and scanning for new ones.

Resolved since last review:

  • w3m-modal-test-visible-ignored-d5a3: Modal mock now properly handles visible prop and a closed-state test was added.

Dismissed by author (not re-flagging):

  • handleBackPress/handleModalClose memoization
  • Inline style in example

Found 3 issue(s)

Issue 1: Changeset bumps all 9 packages despite only 2 having source changes

ID: changeset-scope-all-packages-b2e4
File: .changeset/sharp-cars-approve.md:1-10
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: Changeset lists 9 packages for a patch bump including @reown/appkit-bitcoin-react-native, @reown/appkit-coinbase-react-native, @reown/appkit-ethers-react-native, @reown/appkit-solana-react-native, @reown/appkit-wagmi-react-native, @reown/appkit-core-react-native, @reown/appkit-common-react-native
  • Risk: Only @reown/appkit-react-native (packages/appkit) and @reown/appkit-ui-react-native (packages/ui) have actual source changes. The other 7 packages only have jest.config.ts test-infra changes.
  • Impact: Misleading changelogs and unnecessary version bumps for packages with no functional changes
  • Trigger: Every release cut — 7 extra packages get a changelog entry for a change that doesn't affect them

Recommendation: Scope to only the packages with source changes:

---
'@reown/appkit-react-native': patch
'@reown/appkit-ui-react-native': patch
---

Issue 2: Changeset description says feat: but PR is a fix:

ID: changeset-feat-vs-fix-c7d2
File: .changeset/sharp-cars-approve.md:13
Severity: LOW
Category: code_quality

Context:

  • Pattern: Changeset description: feat: add modalContentWrapper prop for custom modal content wrappers — PR title: fix: add content modal wrapper for expo router issues
  • Impact: Changelog shows feat for a bug-fix PR, inconsistent with conventional commits conventions and the PR framing

Recommendation: Align: fix: add modalContentWrapper prop to work around Expo Router modal layering issues


Issue 3: "No wrapper" test asserts wrong testID — assertion is always vacuously true

ID: w3m-modal-test-stale-testid-assertion-e9c1
File: packages/appkit/src/__tests__/modal/w3m-modal.test.tsx:110
Severity: LOW
Category: code_quality

Context:

  • Pattern: Test at line 110 asserts queryByTestId('modal-wrapper') is null, but the wrapper component in the sibling test uses testID="modal-content-wrapper" (line 115). The testIDs don't match.
  • Risk: The assertion at line 110 will always pass regardless of whether a wrapper is present, since no component ever renders with testID="modal-wrapper". It provides no regression value.
  • Impact: A future bug where the modal incorrectly renders a wrapper would not be caught by this test.

Recommendation: Fix this →

// line 110 — was 'modal-wrapper', should match the testID used in the wrapper test
expect(queryByTestId('modal-content-wrapper')).toBeNull();

@sonarqubecloud
Copy link
Copy Markdown

@ignaciosantise ignaciosantise merged commit 17460e1 into develop Apr 10, 2026
10 checks passed
@ignaciosantise ignaciosantise deleted the fix/expo-router-modal-wrapper branch April 10, 2026 13:35
@github-actions github-actions bot locked and limited conversation to collaborators Apr 10, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants