feat: New Arch Support #1476
feat: New Arch Support #1476
Conversation
WalkthroughThis pull request introduces a comprehensive update to a React Native Expo project, focusing on dependency upgrades, configuration modifications, and type refinements. The changes span across multiple files, including package management, build configurations, and component styling. Key updates involve upgrading Expo and React Native versions, enabling a new architectural feature, adjusting module resolutions, and refining type definitions for various components. Changes
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
design-system/Icon/Icon.android.tsx (1)
96-104: Castingrest.styletoStyleProp<TextStyle>could mask errors.While it's common to cast to a narrower type when working with vector icons, ensure that all consumers of
Iconpass a valid text-based style. If a purely view-based style is passed, runtime issues may arise. You might consider a type guard or explicit conversion to provide better clarity.App.tsx (1)
164-174: Theme-based colors commented out.
Withthemereferences removed, styling no longer adapts to theme changes. If that’s deliberate for performance or design reasons, it’s fine. Otherwise, consider reintroducing dynamic theming to maintain consistency.features/ExternalWalletPicker/ExternalWalletPicker.tsx (1)
108-108: Be cautious with type assertions.
Casting$globalStyles.flex1directly toTextStyleworks if you’re absolutely sure it only contains valid text-related properties. It’s safer to refine$globalStyles.flex1so it conforms toTextStylewithout casting, to maintain type-check confidence.-<Text preset="smaller" style={$globalStyles.flex1 as TextStyle}> +<Text preset="smaller" style={$globalStyles.flex1}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
design-system/TextField/TextField.example.tsxis excluded by!**/*.example.tsxios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
.gitignore(1 hunks)App.tsx(1 hunks)app.config.ts(1 hunks)app.json(1 hunks)babel.config.js(0 hunks)components/AndroidBackAction.tsx(1 hunks)components/Avatar.tsx(1 hunks)components/SvgImageUri.tsx(1 hunks)design-system/Icon/Icon.android.tsx(2 hunks)design-system/Icon/Icon.types.ts(1 hunks)design-system/IconButton/IconButton.tsx(3 hunks)features/ExternalWalletPicker/ExternalWalletPicker.tsx(2 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx(2 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx(2 hunks)features/search/components/ProfileSearchItem.tsx(1 hunks)features/search/components/SearchBar.tsx(1 hunks)features/search/screens/ProfileSearch.tsx(1 hunks)ios/Converse.xcodeproj/project.pbxproj(14 hunks)ios/Podfile(4 hunks)ios/Podfile.properties.json(1 hunks)metro.config.js(0 hunks)package.json(6 hunks)
💤 Files with no reviewable changes (2)
- babel.config.js
- metro.config.js
✅ Files skipped from review due to trivial changes (2)
- components/AndroidBackAction.tsx
- design-system/Icon/Icon.types.ts
🧰 Additional context used
🪛 rubocop (1.69.1)
ios/Podfile
[convention] 5-5: Avoid using rescue in its modifier form.
(Style/RescueModifier)
[convention] 36-36: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🪛 GitHub Actions: iOS App Build
package.json
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, @babel/preset-env@^7.1.6, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Android App APK Build
package.json
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Typescript CI
package.json
[warning] Multiple unmet and incorrect peer dependencies detected in project dependencies
[warning] Workspaces can only be enabled in private projects
🔇 Additional comments (46)
components/SvgImageUri.tsx (2)
9-9: Consider the implications of mixingViewStyleandImageStyle.By defining
styleasStyleProp<ViewStyle & ImageStyle>, you allow any style properties from bothViewStyleandImageStyle. This flexibility might be intentional, but ensure there's a valid use case for mixing these two distinct style sets, as it can sometimes cause confusion.
17-17: Well-implemented style prop update.Allowing a broader range of style properties (
ViewStyle & ImageStyle) provides more styling flexibility for SVG content fallback. If you anticipate further expansions (e.g. text-related styles), consider an intersection withTextStyletoo, or unify the styles for clarity.design-system/Icon/Icon.android.tsx (1)
6-6: Import ofTextStyleis consistent.Introducing
TextStylefacilitates more precise typing for icon styling. This is appropriate given the text-based nature of vector icons.features/search/components/SearchBar.tsx (1)
94-96: Loss of explicitThemedStyle<ViewStyle>typing.Converting
$androidSearchBarto a plain object simplifies the definition but loses type-checking benefits ofThemedStyle. Confirm that the [themed] function usage still works as intended and that any theming changes do not inadvertently break this style object.design-system/IconButton/IconButton.tsx (2)
66-66: Shifting fromViewStyletoTextStylefor icon styling is appropriate.Icons are rendered as text glyphs, so
TextStyleis more precise. This change should help avoid unsupported style properties for text-based icons.
114-120: Ensure hovered state transitions are tested.You have introduced a
hoveredparameter in addition topressed. Confirm that all relevant states are correctly handled, and that you have test coverage (if applicable) or QA steps to verify hover-specific styling and props.features/search/components/ProfileSearchItem.tsx (1)
98-98: Ensure the styling aligns with the Avatar component props.
Switching fromImageStyletoViewStylecan be valid if theAvatarcomponent supports view-style properties. If the component internally relies on image styling (e.g., resizeMode, tintColor), ensure no internal expectation is broken by this change.features/search/screens/ProfileSearch.tsx (1)
105-105: Text style usage looks correct.
Changing toTextStylebetter fits the text element usage. No issues found.ios/Podfile (10)
2-2: Confirm the correctness of the new import path.
Requiringreact_native_podsdirectly, instead ofreact_native_pods.rb, can cause confusion if.rbwas intended. Make sure it loads correctly at runtime.
7-9: New architecture and dev-client network inspector environment variables.
Enabling the new architecture (RCT_NEW_ARCH_ENABLED=1) might introduce unexpected behaviors in older device simulators. Ensure thorough testing for backward compatibility.
11-11: Deployment target updated to 15.1.
This can break compatibility on devices running earlier iOS versions. Ensure it aligns with project requirements.
19-19: MMKV version updated to 1.3.12.
Check for any API or behavior changes. If used intensively for secure storage, confirm that migration is handled.
25-39: Conditional config_command logic.
The dynamic approach to auto-linking is flexible. Ensure no unintentional break if@react-native-community/cliorexpo-modules-autolinkingchanges their arguments.🧰 Tools
🪛 rubocop (1.69.1)
[convention] 36-36: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
40-40: Load native modules with custom config_command.
Looks okay, provided runtime tests confirm all expected modules are detected.
42-43: Linkage handling for frameworks.
Multiple calls touse_frameworks!in the same block can lead to confusion. Verify that the final linkage setting (static vs. dynamic) is correct.
54-54: Enabling Hermes based on Podfile properties.
Verify that the Hermes engine is appropriately toggled and that all caching or bundling steps remain consistent after activation.
73-73: IPHONEOS_DEPLOYMENT_TARGET raised to 15.1.
Ensure any pinned pods or transitive dependencies remain compatible.
99-100: Notification Extension linkage.
Duplicating theuse_frameworks!call is consistent with the main target’s approach. Confirm no conflicting settings with the main target.features/ExternalWalletPicker/ExternalWalletPicker.tsx (1)
17-17: Good improvement for type safety with TextStyle.
ImportingTextStyleensures more strongly-typed style usage for text elements. This addition helps avoid type mismatch issues and clarifies styling.features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx (2)
170-170: Accurate type adjustment to TextStyle.
Switching$userNametoThemedStyle<TextStyle>aligns with its usage on a<Text>component, ensuring style hints remain consistent.
179-179: Consistent approach for text styles.
This change mirrors$userNameand clarifies that$reactionContenttargets text styling properties.features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx (2)
20-20: Clearer distinction between text and view styling.
AddingTextStylesuggests your code is moving consistently toward text-specific style usage for clarity and maintainability.
212-212: Appropriate update for text style.
Changing$reactorCounttoThemedStyle<TextStyle>ensures improved auto-completion and validation for text styling properties.app.config.ts (1)
153-153: Confirmed upgrade to compileSdkVersion 35.
Updating the compile SDK aligns with the latest Android development standards. Validate that all needed dependencies are compatible with SDK 35.✅ Verification successful
Let me gather more information about the Android configuration and dependencies.
Verified: compileSdkVersion 35 is compatible with the project dependencies
Based on the codebase analysis:
- The project uses React Native 0.76.5 which is compatible with Android SDK 35
- The targetSdkVersion is set to 34, which is appropriate as it's one version below the compileSdkVersion
- The minSdkVersion is set to 26, maintaining good backward compatibility
- The buildToolsVersion is "34.0.0" which is compatible with compileSdkVersion 35
- The project's dependencies (like React Native modules and Expo packages) don't show any version constraints that would conflict with compileSdkVersion 35
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Validate that the new compileSdkVersion is supported by the installed libraries rg "compileSdkVersion" -A 5Length of output: 309
Script:
#!/bin/bash # Check for any Android dependencies or version constraints in package.json and build.gradle files fd "package.json|build.gradle" --exec cat {} # Look for any Android SDK related configurations rg "android" -i --type ts --type json -A 5Length of output: 69505
ios/Converse.xcodeproj/project.pbxproj (16)
32-35: Confirm usage of newly introduced frameworks
These newly added references forPods_Converse.frameworkandPods_ConverseNotificationExtension.frameworkappear consistent with the new architecture. Just ensure that no stale references remain for older frameworks and that all relevant targets include these dependencies properly.
89-97: Validate xcconfig references for debug/release
You’ve added/updated references to multiple xcconfig files, including debug and release files for bothPods-ConverseandPods-ConverseNotificationExtension. Make sure that these config files indeed reside at the specified paths, and that all environment variables (e.g.,RCT_NEW_ARCH_ENABLED) are consistently applied across debug and release builds.
Line range hint
107-116: Check framework embedding steps
You added references toPods_Converse.frameworkandPods_ConverseNotificationExtension.frameworkin the build phases. Ensure these frameworks are properly signed and embedded in all relevant build configurations to avoid runtime issues.
150-151: Framework location grouping
IncludingPods_ConverseNotificationExtension.frameworkin the same group asPods_Converse.frameworkis correct for consistency. Good job.
240-243: Maintain clarity in Pod config group
These config files for different targets (Conversevs.ConverseNotificationExtension) are logically separated. Looks good.
263-273: Build phases re-ordered for new architecture
The insertion or re-ordering of [CP] phases, including[CP] Embed Pods Frameworksand[CP] Copy Pods Resources, is consistent with a typical CocoaPods-managed RN iOS project. Confirm that the Sentry phase is placed after the bundling phase if that’s required by your crash reporting strategy.
289-293: Check extension target’s build phases
The addition of[CP] Check Pods Manifest.lockand[CP] Copy Pods Resourcesto the extension ensures dependencies remain in sync. Everything looks correct.
387-406: Manifest.lock check
Your shell script for checkingPodfile.lockvs.Manifest.lockis standard across CocoaPods. Make sure that the integration in the new extension target is aligned with your main target’s updates.
409-420: [RNFB] Core Configuration
Inserting the[CP-User] [RNFB] Core Configurationphase is correct for Firebase-specific setups. No issues found, but do confirm that environment variables are carried over (likeUSE_FIREBASEor similar) if relevant.
422-422: [CP] Embed Pods Frameworks
No immediate concerns. Keep an eye on potential code signing or Swift interface issues if the new arch modifies build settings.
448-467: Secondary [CP] Check Pods Manifest.lock
Having a separate check for your main app target ensures sync with the Podfile. Appropriately mirrors your extension’s build phase.
470-482: Upload Debug Symbols to Sentry
Make sure the environment has the correct auth token for uploading debug symbols. Otherwise, Sentry might report an authentication or DSN error.
484-501: [Expo] Configure project
This build phase runsexpo-configure-project.shto set up modules. Confirm that it works with the new architecture, e.g.,RCT_NEW_ARCH_ENABLED=1.
503-525: Extension’s [CP] Copy Pods Resources
You are copying various resources (CryptoSwift, SQLCipher, Sentry, SwiftProtobuf, etc.) for the notification extension. This looks appropriate. Watch the extension’s size if you add more pods.
527-597: Main target’s [CP] Copy Pods Resources
Same note regarding resource bloat. Typically no issues, but verify you aren’t duplicating resources that your extension doesn’t need.
Line range hint
646-689: Base configuration references
All references to debug/release xcconfig for both the main app and extension appear consistent, explicitly referencing newly introduced files. Great job maintaining uniformity across build settings.Also applies to: 726-770
ios/Podfile.properties.json (1)
2-5: New property to enable new architecture
Adding"newArchEnabled": truealigns with enabling the new React Native architecture. Ensure the rest of the project (e.g., Babel configs, iOS bridging layers) is also aware of this to avoid partial toggles that could cause runtime mismatches.app.json (1)
3-3: Enable new architecture
Mirroring"newArchEnabled": truehere is consistent with the Podfile properties. Confirm that your Expo and RN versions seamlessly handle the new architecture..gitignore (1)
4-4: Ignore newly listed file
Addingexpo-env.d.tsto.gitignoreis standard if it’s a generated or environment-specific file. Looks correct.package.json (3)
151-151: Good practice: Pinned React versions👍 Using exact versions for React and React Native is a good practice for ensuring consistent builds.
Also applies to: 153-153
🧰 Tools
🪛 GitHub Actions: iOS App Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, @babel/preset-env@^7.1.6, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Android App APK Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Typescript CI
[warning] Multiple unmet and incorrect peer dependencies detected in project dependencies
[warning] Workspaces can only be enabled in private projects
66-66:⚠️ Potential issue
⚠️ Major version upgrade for React NavigationThe upgrade from v6 to v7 of React Navigation packages includes breaking changes that need careful attention.
Also applies to: 68-68, 69-69
🧰 Tools
🪛 GitHub Actions: iOS App Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, @babel/preset-env@^7.1.6, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Android App APK Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Typescript CI
[warning] Multiple unmet and incorrect peer dependencies detected in project dependencies
[warning] Workspaces can only be enabled in private projects
109-140: Verify Expo SDK 52 compatibilityThe upgrade to Expo SDK 52 includes multiple package updates. Ensure all native modules and custom native code are compatible with the new version.
🧰 Tools
🪛 GitHub Actions: iOS App Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, @babel/preset-env@^7.1.6, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Android App APK Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Typescript CI
[warning] Multiple unmet and incorrect peer dependencies detected in project dependencies
[warning] Workspaces can only be enabled in private projects
components/Avatar.tsx
Outdated
| Platform, | ||
| StyleProp, | ||
| StyleSheet, | ||
| Text, | ||
| useColorScheme, | ||
| View, | ||
| ViewStyle, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using intersection type for style prop
The style prop is used with both ImageBackground and View components. Using only ViewStyle might prevent valid image-specific styles from being applied to the ImageBackground component.
Consider using an intersection type to maintain compatibility with both components:
-import { ViewStyle } from "react-native";
+import { ImageStyle, ViewStyle } from "react-native";
export type AvatarProps = {
uri?: string | undefined;
size?: number | undefined;
- style?: StyleProp<ViewStyle>;
+ style?: StyleProp<ViewStyle & ImageStyle>;
color?: boolean;
name?: string | undefined;
// Inverts the color of the place holder
invertColor?: boolean;
};Also applies to: 22-22
| "react-native-otp-entry": "^1.7.3", | ||
| "react-native-paper": "^5.10.6", | ||
| "react-native-passkey": "^2.1.1", | ||
| "react-native-passkey": "3.0.0", |
There was a problem hiding this comment.
Address peer dependency warning for react-native-passkey
Pipeline shows peer dependency warnings for react-native-passkeys@^0.2.1, but version 3.0.0 is installed.
Verify if this is causing the peer dependency issues reported in the pipeline:
#!/bin/bash
# Check for passkey usage in the codebase
rg -A 5 "import.*react-native-passkey|from.*react-native-passkey"🧰 Tools
🪛 GitHub Actions: iOS App Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, @babel/preset-env@^7.1.6, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Android App APK Build
[warning] Multiple unmet peer dependencies detected including react-native-passkeys@^0.2.1, react-is@>= 16.8.0, and others
[warning] Workspaces can only be enabled in private projects
🪛 GitHub Actions: Typescript CI
[warning] Multiple unmet and incorrect peer dependencies detected in project dependencies
[warning] Workspaces can only be enabled in private projects
There was a problem hiding this comment.
That's for passkeys, not passkey
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
screens/Navigation/Navigation.tsx (1)
102-102: Help users easily navigate back.By setting
headerBackVisible: false, users lose a primary way to go back. Ensure the UI includes an alternative means of transitioning backward. If an entirely minimal design is intended, confirm that end users are comfortable with gestures or custom controls for navigation.__mocks__/expo-haptics.ts (2)
7-13: Consider adding brief docstrings for the enum values.
Providing short descriptions for each feedback style can help clarify their intended use.
38-38: Static analysis recommendation: prefer named exports.
Using only named exports allows more granular import control and consistent structure across mocks. You could address this as follows:-export default MockedExpoHaptics; +// Removed default export to adhere to named export convention🧰 Tools
🪛 GitHub Check: lint
[warning] 38-38:
Prefer named exportsdesign-system/Button/Button.test.tsx (1)
Line range hint
53-57: Consider adding dark theme coverage.While the snapshot tests for Android are valid, it might be beneficial to include a scenario testing dark mode to ensure styling or color changes are accurately captured.
utils/mmkv.ts (1)
43-43: Potential concurrency bottleneck.For the secure instance, enabling
MULTI_PROCESSis beneficial, but be mindful if encryption operations become a bottleneck under simultaneous reads/writes across processes.ios/Podfile (2)
10-11: Upgraded iOS deployment target to 15.1.This bundling may exclude older devices unable to upgrade to iOS 15. If that’s acceptable, proceed; otherwise, consider retaining 14.0 for broader coverage.
42-43: Ensuring the correct frameworks usage.Including
use_frameworks! :linkage => :staticcan reduce binary size. Just watch for potential conflicts with dynamic frameworks (likeMMKV).package.json (1)
66-69: React Navigation major version increments.These can introduce subtle route or stack breakage. Thoroughly test your navigation flows.
ios/Converse.xcodeproj/project.pbxproj (6)
289-293: [CP] Copy Pods Resources for the notification extension.Make sure the resources are placed correctly; shipping superfluous resources can increase app size.
387-406: New[CP] Check Pods Manifest.lockblock.Duplicated script phases can cause confusion. Possibly remove or unify them.
448-467: Check Pods Manifest for main target.Similar duplication to the earlier “Check Pods Manifest.lock” script. Consolidating might simplify the build pipeline.
484-501: [Expo] Configure project script.Always out of date means it runs every build. Evaluate if you can optimize.
503-525: Notification extension resource copying script.Large resource sets can increase final app size. Confirm minimal resources.
527-597: Resources copying for main target.Again, large resource sets can bloat the app. Evaluate necessity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
design-system/Button/__snapshots__/Button.test.tsx.snapis excluded by!**/*.snapios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
__mocks__/expo-haptics.ts(1 hunks)__mocks__/expo-image.ts(1 hunks)app.json(1 hunks)components/Avatar.tsx(2 hunks)components/__tests__/Avatar.perf-test.tsx(1 hunks)components/__tests__/Avatar.test.tsx(2 hunks)design-system/Button/Button.test.tsx(5 hunks)design-system/Text/__tests__/Text.perf-test.tsx(5 hunks)design-system/test-utils/renderWithThemeProvider.tsx(1 hunks)features/conversation/conversation.nav.tsx(1 hunks)ios/Converse.xcodeproj/project.pbxproj(14 hunks)ios/Podfile(4 hunks)ios/Podfile.properties.json(1 hunks)jest.config.ts(1 hunks)package.json(6 hunks)patches/react-native-mmkv+2.12.2.patch(0 hunks)screens/Navigation/Navigation.tsx(1 hunks)utils/mmkv.ts(3 hunks)
💤 Files with no reviewable changes (1)
- patches/react-native-mmkv+2.12.2.patch
✅ Files skipped from review due to trivial changes (1)
- mocks/expo-image.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app.json
- ios/Podfile.properties.json
- components/Avatar.tsx
🧰 Additional context used
🪛 GitHub Check: lint
__mocks__/expo-haptics.ts
[warning] 38-38:
Prefer named exports
🪛 rubocop (1.69.1)
ios/Podfile
[convention] 5-5: Avoid using rescue in its modifier form.
(Style/RescueModifier)
[convention] 36-36: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tsc
- GitHub Check: test
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (64)
features/conversation/conversation.nav.tsx (1)
27-27: Consider user convenience when removing the back button.Setting
headerBackVisible: falsehides the back button completely, which can hamper discoverability for navigating back. Make sure this behavior is intentional, or consider offering an alternative back navigation mechanism for clarity.jest.config.ts (1)
90-90: Nice regex simplification.The removal of redundant parentheses around
uuiddoes not alter functionality and keeps the pattern simpler.__mocks__/expo-haptics.ts (5)
1-6: Good use of code comments to explain the reason for mocking.
This clear annotation helps maintainers remember why this mock is necessary.
15-19: Consistency in naming for enumeration values.
The enumeration naming constants follow a clear and consistent style, which is good for maintainability.
21-22: Ensure test coverage with these mocked methods.
Although these are mocks, confirm that your tests exercise them thoroughly so untested code paths don’t mask potential issues in production.
24-29: Solid aggregator object for the mock.
Centralizing the enumerations and methods into a single object is a clean design choice for test mocking.
31-36: Well-structured named exports.
Exporting items individually simplifies selective imports elsewhere in the codebase.design-system/test-utils/renderWithThemeProvider.tsx (4)
1-4: Good import additions and usage ofNavigationContainer.Bringing in
NavigationContainerto support navigation-related theming is appropriate. The usage ofmeasureRendersandMeasureRendersOptionsfrom"reassure"is also consistent with performance testing requirements.
6-16: ExportingTestThemeProvideris well-structured.The refactor to export the component and destructure
navigationThemefromuseThemeProvidereffectively consolidates theme and navigation logic.
19-23: Properly wrapping child components.Wrapping child components within both
NavigationContainerandThemeProviderensures all nested components get the correct theme and navigation context during tests.
30-33: Excellent approach to measuring renders with theme context.Providing a specialized function (
measureRendersWithThemeProvider) makes performance testing more consistent across the codebase.components/__tests__/Avatar.perf-test.tsx (2)
7-7: ImportingTestThemeProvideris coherent.This import integrates the theme context for performance tests, ensuring consistent styling and environment.
11-15: Appropriate usage ofTestThemeProvider.Wrapping the
AvatarinsideTestThemeProvideraligns test conditions with the actual app context. This improves test accuracy for performance measurements.components/__tests__/Avatar.test.tsx (4)
7-7: Replacing rawrenderwithrenderWithThemeProvider.This ensures that the
Avatarcomponent is tested within the app’s theme context for accurate rendering behavior.
17-23: Validation of image source props is correct.Using
image.props.source.uriis accurate if thesourceprop is a single object. This test verifies the correct image URI is set.
27-29: Rendering the avatar’s initial letter.Verifying the presence of the letter is an effective approach to ensure fallback name logic works correctly.
37-37: Placeholder check is straightforward.Ensuring a placeholder is displayed when neither name nor URI is provided covers edge cases effectively.
design-system/Text/__tests__/Text.perf-test.tsx (5)
7-10: New import formeasureRendersWithThemeProvider.Centralizing theme-aware performance testing logic fosters consistency across the
Textcomponent tests and other components.
20-23: UsingmeasureRendersWithThemeProviderfor default props scenario.This ensures that the default text rendering performance is measured with the correct theme context.
Line range hint
36-40: Bold and largeTextperformance measured in theme context.Applying consistent performance measurement across different text style variations is beneficial.
52-55: Ensuring translation-based text is captured.Wrapping the translation scenario in the theme context helps gauge performance in a real-world setting.
64-70: Color prop test is thoroughly measured.Validating render performance with the color prop ensures accurate coverage of thematic styling changes.
design-system/Button/Button.test.tsx (4)
5-6: Great use of named import and theming utilities.Switching to
import { Button } from "./Button";andrenderWithThemeProviderensures better modularity and consistent theme handling across tests. This approach enhances clarity and maintainability.
Line range hint
19-23: Proper usage ofrenderWithThemeProvider.Wrapping the component with
renderWithThemeProviderhelps ensure that theme-based style changes are accurately tested. This fosters more reliable snapshots and behavior checks.
Line range hint
34-38: Good job verifying onPress functionality.Ensuring
onPressis tested with the new theme provider context is important for capturing real-world interactions. This test correctly validates the callback invocation.
Line range hint
67-71: Valid approach to testing icon prop usage.Testing the presence of a pictogram ensures that the
Buttoncomponent's optional visual elements are rendered correctly.utils/mmkv.ts (3)
3-3: Well-specified import usage.By importing both
MMKVandMode, it's clear you're leveraging new multi-process mode features. This is good for clarity.
10-13: Multi-process mode usage.The addition of
mode: Mode.MULTI_PROCESSon the default storage instance is beneficial for concurrent data access scenarios. Ensure you test concurrency carefully to avoid data corruption.
[approve]Would you like me to generate concurrency stress-testing scripts to verify that no data corruption occurs?
58-61: Consistent approach withreactQueryMMKV.Enabling multi-process mode here as well ensures consistent concurrency handling across all instances. Nice job maintaining uniformity.
ios/Podfile (7)
7-8: Enable the new architecture by default.Setting
ENV['RCT_NEW_ARCH_ENABLED'] = '1'is a good step in adopting the New Architecture. Be sure to test that all native modules are compatible.
21-22:prepare_react_native_project!usage check.Ensure the method fully supports any new architecture changes. Watch for warnings from
prepare_react_native_project!.
25-38: Conditional autolinking command.This dynamic approach to autolinking is flexible. Great job ensuring both community CLI and expo modules are properly included.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 36-36: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
40-40: Validate theconfigassignment.Make sure that
use_native_modules!(config_command)returns a valid config object for subsequent usage.
54-54: Hermes engine usage check.Hermes is now toggled by
podfile_properties['expo.jsEngine']. This can yield performance benefits on React Native.
73-73: Raise target to iOS 15.1 in post-install.Consistent with the updated
platform. Make sure external frameworks are 15.1-compatible.
99-100: Static framework usage for Notification Extension target.Same advice as main target: confirm no conflict with dynamic libraries that you rely upon.
package.json (14)
44-46: Upgrading Expo config packages.These updates are substantial leaps; ensure no breaking changes.
60-61: Clipboard & netinfo upgraded.Watch for potential breaking changes, especially if certain APIs changed or were deprecated.
64-64: Menu library upgrade.Confirm that the new version is fully tested on iOS/Android.
71-71: FlashList update.Small version bump. Likely minimal risk, but confirm no new performance regressions.
109-140: Expo and related modules upgraded.This large block of new expo versions must be tested thoroughly, including potential new permissions or setup steps.
160-160: React Native Gesture Handler synergy.Pair with your RN version to ensure correct linking.
164-164: iOS utilities version check.Occasionally, older versions conflict with new iOS deployment targets.
168-168: MMKV updated to 3.1.0.Matches the multi-process config. Good synergy with your iOS Pod updates.
171-171: [Possible duplicate of previous comment] Peer dependency for react-native-passkey
175-179: Quick crypto and Reanimated updates.Check for any changes in setup instructions, especially for Reanimated (Metro config).
183-183: Upgrade react-native-svg.Potential impacts on vector components used in your UI. Test thoroughly in different OS versions.
188-188: Webview update.Ensure that new features or deprecations are accounted for.
219-219: React type definitions.Keep an eye out for potential changes in the
reacttype definitions for TS.
237-237:jest-expoupdated.Confirm test configs remain valid.
ios/Converse.xcodeproj/project.pbxproj (14)
32-35: Updated framework references for Pods.Validate the build phases and link settings so the frameworks are embedded correctly.
89-90: File reference additions.Pods_Converse being added here is typical when adjusting new architecture.
93-95: Notification extension xconfig references.Double-check that you’re linking the correct config files for debug/release.
97-97: Pods_ConverseNotificationExtension.framework reference.Ensure this is included in the correct build phase.
107-107: Adding Pods_Converse.framework to main target.Check that you don’t double-embed the same frameworks in multiple phases.
116-116: Notification extension depends on Alamofire.No immediate concerns, but keep an eye on the binary size.
150-151: Framework group references.Listing them under Frameworks is standard practice.
240-243: Pod references in thePodsgroup.Organized grouping is beneficial for maintainability.
263-273: [CP] Check Pods Manifest.lock and embedding.Multiple
[CP]script phases—ensure no duplication or redundancy.
409-420: [CP-User] [RNFB] Core Configuration script.No issues spotted, but keep an eye on compatibility with newly introduced iOS 15.1 settings.
422-422: [CP] Embed Pods Frameworks usage.Embedding multiple frameworks (MMKV, Hermes, etc.). Confirm no conflicts with static usage.
470-482: Sentry debug symbol upload script.Ensure your DSN is set properly. Also, watch out for potential conflicts with new arch.
Line range hint
646-689: Switch to new Pods-Converse xcconfig references.Ensure these new references match actual config files for both debug and release.
Line range hint
726-770: Pods-ConverseNotificationExtension debug/release xconfigs.Similar to the main target’s config changes. Good consistency.
ios/Podfile
Outdated
| {paths: [process.argv[1]]}, | ||
| )', __dir__]).strip | ||
| require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules") | ||
| require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods") |
There was a problem hiding this comment.
💡 Codebase verification
Based on the output, I can see that the file react_native_pods.rb exists at node_modules/react-native/scripts/react_native_pods.rb, which means the path in the Podfile should include the .rb extension.
The path should include the .rb extension
The current path scripts/react_native_pods is incorrect. It should be scripts/react_native_pods.rb as verified by the presence of the file at node_modules/react-native/scripts/react_native_pods.rb.
🔗 Analysis chain
Path update review.
You've updated the path from react_native_pods.rb to react_native_pods. Verify the new path is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm that the react_native_pods file exists at the expected location
fd "react_native_pods" --exec ls -lh
Length of output: 36
Script:
#!/bin/bash
# Let's check if the file exists in node_modules
ls -la node_modules/react-native/scripts/react_native_pods*
# Also check the content of package.json to verify the react-native dependency
cat package.json | grep -A 5 "react-native"
# Let's also check if there are any other Podfile references in the codebase
fd Podfile --exec cat {}
Length of output: 75018
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ios/Podfile (1)
2-2:⚠️ Potential issueRevert the path change to include the
.rbextensionThe path should be
scripts/react_native_pods.rbas the file exists at this location.
🧹 Nitpick comments (1)
package.json (1)
9-9: Add a short explanation for "android:build:clean" script.
The new script is helpful for cleaning and rebuilding Android, but consider adding a concise comment in the script or project docs describing its purpose and usage to aid future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
components/Onboarding/init-xmtp-client.ts(1 hunks)ios/Converse.xcodeproj/project.pbxproj(16 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift(0 hunks)ios/Podfile(3 hunks)package.json(7 hunks)patches/react-native-mmkv+3.2.0.patch(1 hunks)
💤 Files with no reviewable changes (1)
- ios/ConverseNotificationExtension/Xmtp/Messages.swift
✅ Files skipped from review due to trivial changes (1)
- patches/react-native-mmkv+3.2.0.patch
🧰 Additional context used
🪛 rubocop (1.69.1)
ios/Podfile
[convention] 5-5: Avoid using rescue in its modifier form.
(Style/RescueModifier)
[convention] 36-36: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tsc
- GitHub Check: test
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (24)
ios/Podfile (4)
7-8: Verify implications of enabling the new architectureThe new React Native architecture is being enabled via
RCT_NEW_ARCH_ENABLED=1. This is a significant change that:
- May require additional setup and compatibility checks
- Could affect the app's performance and stability
Please ensure:
- All native modules are compatible with the new architecture
- Performance testing has been conducted
- A rollback plan is in place if issues arise
11-11: Consider impact of increased iOS deployment targetIncreasing the minimum iOS version from 14.0 to 15.1 will affect app distribution and user base.
Please confirm:
- This change aligns with the app's user analytics
- The potential user impact has been assessed
- Required iOS 15.1+ features justify dropping support for iOS 14.x users
52-52: Verify Hermes engine configurationThe Hermes engine configuration now depends on
expo.jsEnginefrom Podfile.properties.json.Please ensure:
- The property is correctly set in Podfile.properties.json
- The app's performance metrics are monitored after this change
- The development and production builds use the intended JavaScript engine
25-38: Verify community autolinking configurationThe addition of conditional logic for community autolinking provides flexibility but requires verification.
Run the following script to check for potential module resolution conflicts:
✅ Verification successful
Autolinking configuration is correctly implemented ✅
The conditional logic for community autolinking is properly implemented and safely coexists with Expo's autolinking system. The configuration supports both methods and includes proper fallback handling, which is essential for the project's extensive use of native modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any native modules that might be affected by the autolinking change fd '\.(podspec|m|h|mm|cpp)$' --exec grep -l "RCT_EXTERN_MODULE\|RCT_EXPORT_MODULE" {}Length of output: 89
Script:
#!/bin/bash # Check for package.json to verify dependencies echo "=== Checking package.json for relevant dependencies ===" cat package.json | jq -r '.dependencies | select(has("expo") or has("@react-native-community/cli") or has("expo-modules-autolinking"))' echo -e "\n=== Checking for other potential module configuration files ===" fd -e json -e js -e ts --max-depth 3 "metro.config|app.config|expo.config" echo -e "\n=== Checking Podfile for other autolinking configurations ===" cat ios/PodfileLength of output: 11288
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 36-36: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
ios/Converse.xcodeproj/project.pbxproj (3)
497-592: Review resource bundling configurationThe addition of resource bundling phases for both targets is comprehensive but should be verified.
Ensure that:
- All required resources are included in the bundles
- No unnecessary resources are being bundled
- Bundle sizes remain optimized
422-439: Verify framework embedding configurationThe framework embedding setup includes critical dependencies like Hermes and OpenSSL.
Please confirm:
- All required frameworks are properly embedded
- Framework search paths are correctly configured
- No unnecessary frameworks are included that could increase app size
Line range hint
640-683: Review build settings alignmentThe build settings have been updated to support the new architecture and increased deployment target.
Ensure:
- Build settings are consistent across all configurations
- The settings support both debug and release builds
- The settings align with React Native's new architecture requirements
components/Onboarding/init-xmtp-client.ts (1)
47-47: Ensure the error rethrow is intended.
Re-throwing the caught error here is good for propagating the exception up the call stack. However, confirm that higher-level functions handle it properly, and that it doesn’t lead to duplicate or confusing error messages for the user.package.json (16)
45-47: Check for potential breaking changes in updated Expo Metro packages.
Upgrading these core Metro plugins to newer versions can occasionally introduce changes in bundling behavior. Verify the bundling pipeline for any new warnings or run-time issues.
61-62: Incremental upgrades recognized.
Upgrading clipboard and NetInfo libraries looks straightforward. No obvious concerns.
65-65: Verify iOS context menu alignment.
Menu updates can change interactions on iOS. Ensure that any usage of this library in your code remains compatible with the new version.
67-70: React Navigation upgrades seem major.
Moving from v6 to v7 for the drawer, stack, and native modules might introduce subtle API changes. Confirm that navigation config and custom hooks remain compatible.
72-72: FlashList upgrade is minor.
Check for any deprecation in the new version. Otherwise, it’s a beneficial upgrade for performance.
110-141: Expo 52 and allied packages.
Expo 52 includes new architectural changes that might affect build configs. Since you enabled newArch in the app config, confirm that these new versions are stable across iOS and Android.
152-154: React & React Native upgrades.
You’re now on React 18.3.1 and RN 0.76.5. This is a significant bump that requires thorough retesting, especially for library compatibility.
161-165: Gesture Handler & iOS Utilities upgrades.
These minor version updates generally bring performance improvements and bug fixes. No immediate concerns, but test gestures thoroughly on iOS.
169-169: MMKV storage update.
No major concerns, but recheck storage migrations if you’re persisting critical data with MMKV.
172-172: Passkey library version match.
You have a new version forreact-native-passkey. Check pipeline logs or peer dependencies to confirm there are no mismatches.
176-180: Quick crypto & Reanimated updates.
New versions can bring improved performance but also potential breakage or changed initialization steps. Keep an eye on animations or cryptographic operations.
182-182: React Native Share changes.
Ensure that the share flows are unaffected; test iOS and Android share dialogs thoroughly for possible differences.
184-184: SVG library updated.
Confirm that components using SVG or the transformer do not encounter any rendering issues with the new version.
189-189: WebView version increment.
This step might require retesting of in-app web flows, especially for form handling or custom user interactions.
220-220: React typings updated.
No immediate concerns, but run type checks to catch any previously unexposed issues.
238-238: Jest Expo upgrade.
Test coverage might be affected. Re-run tests to ensure compatibility with the new version.
| @@ -6,6 +6,7 @@ | |||
| "android:build:dev": "eas build -p android --profile development --local", | |||
| "android:reverse": "adb reverse tcp:8081 tcp:8081 && adb reverse tcp:9875 tcp:9875", | |||
| "android:sendApk": "./scripts/android/sendApk.sh", | |||
| "android:build:clean": "rm -rf ./android && npx expo prebuild --platform android && yarn android", | |||
There was a problem hiding this comment.
We could add this to the clean script and use the same AppleScript magic to ensure this is immediate!
|
Does this build and run @alexrisch ? Awesome that you put in the legwork to get this started. It would be a shame if we didn't get it in soon. Maybe things will break but if I understand correctly, isn't the new architecture going to help improve performance by eliminating the cross bridge overhead |
Yes it ran previously, I'll get it rebased to main |
alexrisch
force-pushed
the
ar/new-arch
branch
from
09aaece to
dd255eb
Compare
Upgraded to expo 52 Bumped packages fix tsc Fix iOS build Bump versions Working iOS Android working Fix ts tests fix Performance tests feat: Enable New Architecture Works on Android iOS having an issue with MMKV installation when building remove mmkv patch, enable multiprocess Remove MMKV Upgrade iOS Building
alexrisch
force-pushed
the
ar/new-arch
branch
from
dd255eb to
612f974
Compare
Summary by CodeRabbit
Release Notes
New Features
Dependency Updates
Performance Improvements
Styling Adjustments
Testing Enhancements