feat(native-app): add upgrade wall to app #16164
Conversation
WalkthroughThe changes introduce various updates across multiple files in the iOS application. Key modifications include the addition of a new Changes
Possibly related PRs
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
Documentation and Community
|
Datadog ReportAll test runs ❌ 101 Total Test Services: 1 Failed, 98 Passed Test ServicesThis report shows up to 10 services
❌ Failed Tests (4)
🔻 Code Coverage Decreases vs Default Branch (3) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (14)
apps/native/app/src/utils/user-agent.ts (2)
4-10: Implementation looks good, but consider adding error handling.The
getCustomUserAgentfunction correctly implements the required user agent format as specified in the PR objectives. It's concise and readable.Consider adding error handling to manage potential failures when calling
DeviceInfoorPlatformAPIs. For example:export const getCustomUserAgent = (): string => { try { return [ `IslandIsApp (${DeviceInfo.getVersion()})`, `Build/${DeviceInfo.getBuildNumber()}`, `(${Platform.OS}/${Platform.Version})`, ].join(' ') } catch (error) { console.error('Failed to generate custom user agent:', error) return 'IslandIsApp (Unknown)' } }This ensures that even if there's an error, a default user agent is returned.
1-10: Enhance TypeScript usage for better type safety.While the current implementation works, we can improve type safety and code clarity with TypeScript features.
Consider these TypeScript enhancements:
Add a return type annotation to the
getCustomUserAgentfunction:export const getCustomUserAgent = (): string => { // ... existing implementation }If you're using a recent TypeScript version that supports import type assertions, you could add them to the imports for extra clarity:
import { Platform } from 'react-native' assert { type: 'module' }; import DeviceInfo from 'react-native-device-info' assert { type: 'module' };These changes will improve type checking and make the code's intent clearer.
apps/native/app/src/ui/lib/navigation-bar-sheet/navigation-bar-sheet.tsx (2)
75-75: LGTM! Consider using a more specific type forclosable.The addition of the
closableparameter with a default value oftrueis a good implementation that aligns with the PR objectives and maintains backward compatibility. The type definition is correctly updated.Consider using a more specific type for
closable:closable?: booleanThis change would make the type more explicit and improve code readability.
Also applies to: 82-82
97-103: LGTM! Consider extracting the title rendering logic for improved readability.The conditional rendering of the Header component and the updated title handling logic are well-implemented. These changes align with the PR objectives and improve the component's flexibility.
To improve readability, consider extracting the title rendering logic into a separate function:
const renderTitle = () => { if (typeof title === 'string') { return <HeaderTitle>{title}</HeaderTitle>; } return title; }; // Then in the JSX: {(closable || title) && ( <Header style={style}> {renderTitle()} {/* ... rest of the Header content ... */} </Header> )}This refactoring would make the component's structure easier to understand at a glance.
apps/native/app/src/screens/update-app/update-app.tsx (3)
1-37: LGTM! Consider extracting styled components.The imports and styled components are well-organized and follow React Native best practices. The use of theme values in styled components promotes consistency across the app.
Consider extracting the styled components to a separate file (e.g.,
update-app.styles.ts) to improve code organization and reusability, especially if the component grows larger in the future.
49-65: LGTM! Consider adding componentId to useEffect dependencies.The component declaration and use of useEffect to set up navigation options based on the closable prop are well implemented. However, there's a minor improvement that can be made:
Add
componentIdto the useEffect dependency array to ensure the effect runs if the componentId changes:- }, []) + }, [componentId, closable])This change will make the effect more robust and compliant with React's rules of hooks.
67-132: LGTM! Consider adding accessibility props to images.The component's render method is well-structured and follows React Native best practices. The use of localization and conditional rendering based on the closable prop is excellent. However, there are a couple of improvements that can enhance the component:
- Add accessibility props to the Image components:
<Image source={logo} resizeMode="contain" style={{ width: 45, height: 45 }} + accessibilityLabel={intl.formatMessage({ id: 'updateApp.logoAlt', defaultMessage: 'Iceland.is logo' })} + accessibilityRole="image" /> <Image source={illustrationSrc} style={{ width: 210, height: 240 }} resizeMode="contain" + accessibilityLabel={intl.formatMessage({ id: 'updateApp.illustrationAlt', defaultMessage: 'Update illustration' })} + accessibilityRole="image" />
- Consider using a constant for the app store URLs to improve maintainability:
const APP_STORE_URL = 'https://apps.apple.com/app/%C3%ADsland-is-stafr%C3%A6nt-%C3%ADsland/id1569828682'; const PLAY_STORE_URL = 'https://play.google.com/store/apps/details?id=is.island.app'; // Then in the onPress handler: Linking.openURL(isIos ? APP_STORE_URL : PLAY_STORE_URL)These changes will improve accessibility and make the code more maintainable.
apps/native/app/src/utils/lifecycle/setup-components.tsx (1)
107-107: LGTM: Registration of UpdateAppScreen componentThe UpdateAppScreen component is correctly registered with the appropriate identifier. This registration aligns with the PR objectives of adding an upgrade wall to the app.
Consider grouping related screens together for better code organization. For instance, you might want to place this registration near other update-related or system-level screens if they exist.
apps/native/app/src/screens/passkey/passkey.tsx (2)
34-48: LGTM: New styled components enhance code organization.The introduction of
Host,ButtonWrapper, andTitlestyled components improves code organization and reusability. They follow React and styled-components best practices and maintain consistency with the theme.Consider adding JSDoc comments to these new components to improve code documentation. For example:
/** * Wrapper component that centers its children both vertically and horizontally. */ const Host = styled.View` // ... existing styles `
Line range hint
102-135: LGTM: Improved component structure using new styled components.The updates to the component's structure, including the use of the
Hostcomponent and theTitlecomponent, improve code readability and maintain consistent styling. The adjustment of the illustration image dimensions is also noted.Consider extracting the image dimensions into constants or theme variables for easier maintenance. For example:
const ILLUSTRATION_WIDTH = 210; const ILLUSTRATION_HEIGHT = 240; // ... in the component <Image source={illustrationSrc} style={{ width: ILLUSTRATION_WIDTH, height: ILLUSTRATION_HEIGHT }} resizeMode="contain" />apps/native/app/src/screens/home/home.tsx (1)
259-264: LGTM: App version check implementation.The
checkAppVersionfunction and its integration look good. It correctly implements the upgrade wall feature as described in the PR objectives. The use ofuseCallbackfor memoization is a good practice.Consider adding error handling to the
checkAppVersionfunction:const checkAppVersion = useCallback(async () => { try { const needsUpdate = await needsToUpdateAppVersion() if (needsUpdate) { navigateTo('/update-app', { closable: false }) } } catch (error) { console.error('Failed to check app version:', error) // Consider implementing a fallback behavior or user notification } }, [])This will ensure that any errors during the version check are logged and handled gracefully.
Also applies to: 272-273
apps/native/app/src/utils/lifecycle/setup-routes.ts (1)
183-201: LGTM! Consider a minor improvement for consistency.The new route for
/update-appis well-implemented and aligns with the PR objectives. The use ofswipeToDismiss: falseis appropriate for an update screen.For consistency with other modal routes in this file, consider extracting the modal options into a separate object:
const updateAppModalOptions: Options = { modal: { swipeToDismiss: false, }, }; addRoute('/update-app', async (passProps) => { Navigation.showModal({ stack: { options: updateAppModalOptions, children: [ { component: { name: ComponentRegistry.UpdateAppScreen, passProps, }, }, ], }, }) })This approach would improve readability and make it easier to maintain consistent modal options across different routes.
apps/native/app/src/messages/en.ts (1)
599-605: LGTM! New translations added for the update app feature.The new translations for the 'updateApp' feature align well with the PR objectives. They provide clear messages for prompting users to update their app version.
For consistency with other translation keys in this file, consider using sentence case for the 'buttonSkip' key:
- 'updateApp.buttonSkip': 'Skip', + 'updateApp.buttonSkip': 'Skip',apps/native/app/src/messages/is.ts (1)
598-604: LGTM! Consider a minor improvement for consistency.The new translations for the app update feature look good and align well with the PR objectives. They provide clear and concise messages for the update prompt, including an option to skip the update.
For consistency with other multi-word keys in the file, consider using camelCase for 'buttonSkip':
- 'updateApp.buttonSkip': 'Sleppa', + 'updateApp.buttonSkip': 'Sleppa',This change would make the key naming consistent with others like 'updateApp.description'.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
apps/native/app/ios/Podfile.lockis excluded by!**/*.lockapps/native/app/src/assets/illustrations/digital-services-m1-dots.pngis excluded by!**/*.pngapps/native/app/src/assets/illustrations/[email protected]is excluded by!**/*.pngapps/native/app/src/assets/illustrations/[email protected]is excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
- apps/native/app/ios/IslandApp.xcodeproj/project.pbxproj (3 hunks)
- apps/native/app/package.json (1 hunks)
- apps/native/app/src/graphql/client.ts (2 hunks)
- apps/native/app/src/messages/en.ts (1 hunks)
- apps/native/app/src/messages/is.ts (1 hunks)
- apps/native/app/src/screens/home/home.tsx (2 hunks)
- apps/native/app/src/screens/passkey/passkey.tsx (5 hunks)
- apps/native/app/src/screens/update-app/update-app.tsx (1 hunks)
- apps/native/app/src/ui/lib/navigation-bar-sheet/navigation-bar-sheet.tsx (2 hunks)
- apps/native/app/src/utils/component-registry.ts (1 hunks)
- apps/native/app/src/utils/lifecycle/setup-components.tsx (2 hunks)
- apps/native/app/src/utils/lifecycle/setup-routes.ts (1 hunks)
- apps/native/app/src/utils/minum-app-version.ts (1 hunks)
- apps/native/app/src/utils/user-agent.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
apps/native/app/ios/IslandApp.xcodeproj/project.pbxproj (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/package.json (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/graphql/client.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/en.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/is.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/home/home.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/passkey/passkey.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/update-app/update-app.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/ui/lib/navigation-bar-sheet/navigation-bar-sheet.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/utils/component-registry.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/utils/lifecycle/setup-components.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/utils/lifecycle/setup-routes.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/utils/minum-app-version.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/utils/user-agent.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (24)
apps/native/app/src/utils/user-agent.ts (1)
1-2: Imports look good.The necessary modules are correctly imported, and there's no unnecessary import bloat.
apps/native/app/src/utils/minum-app-version.ts (2)
1-3: LGTM: Imports are appropriate and follow best practices.The imports are well-chosen for the task at hand. Using
react-native-device-infofor version retrieval andcompare-versionsfor version comparison are good practices in React Native development.
5-18: LGTM: Well-implemented version comparison function.The
needsToUpdateAppVersionfunction is well-structured and follows TypeScript and React Native best practices. It correctly handles asynchronous operations and uses appropriate libraries for version retrieval and comparison.apps/native/app/src/ui/lib/navigation-bar-sheet/navigation-bar-sheet.tsx (3)
95-95: LGTM! Handle rendering logic updated correctly.The conditional rendering of the Handle component has been updated to include the
closablecheck. This change correctly implements the new functionality to control UI element visibility based on theclosableparameter.
104-129: LGTM! CloseButton rendering updated correctly.The conditional rendering of the CloseButton based on the
closableparameter is well-implemented. This change correctly aligns with the PR objectives to control the visibility of the close button while maintaining the rendering of other icons.
Line range hint
1-134: Overall, the changes are well-implemented and align with the PR objectives.The modifications to the
NavigationBarSheetcomponent successfully introduce theclosablefunctionality while maintaining backward compatibility and adhering to NextJS and React Native best practices. The component's type safety is preserved, and the conditional rendering logic is correctly implemented.A few minor suggestions have been made to further improve code readability and maintainability, but these are not critical issues.
apps/native/app/src/screens/update-app/update-app.tsx (2)
39-47: LGTM! Good use of custom hooks for navigation options.The creation and use of a custom hook for navigation options is a great practice. It enhances code reusability and keeps the component clean. The options set are appropriate for a modal screen, hiding the top bar and controlling the hardware back button behavior.
135-135: LGTM! Consistent use of navigation options.The assignment of navigation options using the
getNavigationOptionsfunction is consistent with the custom hook created earlier in the file. This approach ensures that the component uses the predefined navigation options, maintaining consistency across the app.apps/native/app/package.json (1)
55-55: New dependency added: compare-versionsThe addition of the
compare-versionspackage (version 6.1.1) aligns with the PR objectives, particularly for implementing the upgrade wall feature. This package will likely be used to compare the user's app version with the minimum supported version.A few points to consider:
- Ensure that this package is actually used in the implementation of the upgrade wall feature.
- Verify that version 6.1.1 is the latest stable version and doesn't have any known vulnerabilities.
- Consider adding a comment in the code where this package is used, explaining its purpose in the context of the upgrade wall feature.
To ensure the package is being used correctly, please run the following command:
This will help verify that the package is imported and used in the relevant files for the upgrade wall feature.
✅ Verification successful
Dependency
compare-versionsusage verifiedThe
compare-versionspackage (version 6.1.1) is correctly imported and utilized insrc/utils/minum-app-version.ts. This confirms that the addition aligns with the PR objectives, specifically for implementing the upgrade wall feature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of compare-versions in the codebase rg --type typescript --type javascript 'import.*compare-versions'Length of output: 102
Script:
#!/bin/bash # Search for usage of compare-versions in .ts and .js files rg 'import.*compare-versions' -g '*.ts' -g '*.js'Length of output: 145
apps/native/app/src/graphql/client.ts (2)
23-23: LGTM: Import statement for custom user agent function.The import statement for
getCustomUserAgentis correctly added and follows TypeScript best practices.
138-138: Verify completeness of User-Agent implementation.The addition of the custom User-Agent header in the
authLinkis correct and aligns with the PR objectives. However, as per a previous review comment, we should consider adding this header in other relevant places:
- In the
errorLinkwithin this file.- In the
auth-store.tsfile.To ensure consistency, please run the following script to check for other potential locations where the User-Agent should be added:
Please review the output and consider adding the User-Agent header in these locations if appropriate.
✅ Verification successful
Retry the header search with corrected file type specifications.
Please run the following script to check for other locations where the User-Agent might need to be added:
User-Agent header implementation verified, minor enhancement suggested.
The User-Agent header is correctly implemented in the
authLinkwithinapps/native/app/src/graphql/client.ts. However, for consistency, consider adding it to theerrorLinkas well. No changes are needed inauth-store.ts.To add the User-Agent header to the
errorLink, you can modify it like this:const errorLink = onError(({ graphQLErrors, networkError }) => { if (graphQLErrors) graphQLErrors.forEach(({ message, locations, path }) => console.log( `[GraphQL error]: Message: ${message}, Location: ${locations}, Path: ${path}`, ), ) if (networkError) console.log(`[Network error]: ${networkError}`) }) // Add this line before creating the ApolloClient instance errorLink.setOptions({ headers: { 'User-Agent': getCustomUserAgent(), }, })This change will ensure that the User-Agent header is consistently applied across both
authLinkanderrorLink.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other locations where User-Agent might need to be added # Search for headers being set in the current file echo "Checking current file for other header settings:" rg --type typescript -A 5 'headers:' apps/native/app/src/graphql/client.ts # Search for headers being set in auth-store.ts echo "\nChecking auth-store.ts for header settings:" rg --type typescript -A 5 'headers:' apps/native/app/src/stores/auth-store.tsLength of output: 430
Script:
#!/bin/bash # Description: Check for other locations where User-Agent might need to be added # Search for headers being set in the current file echo "Checking current file for other header settings:" fd --extension ts --extension tsx . apps/native/app/src/graphql/client.ts | xargs rg -A 5 'headers:' # Search for headers being set in auth-store.ts echo -e "\nChecking auth-store.ts for header settings:" fd --extension ts --extension tsx . apps/native/app/src/stores/auth-store.ts | xargs rg -A 5 'headers:'Length of output: 342981
apps/native/app/src/utils/lifecycle/setup-components.tsx (2)
28-28: LGTM: Import statement for UpdateAppScreenThe import statement for the UpdateAppScreen component is correctly added and follows the existing pattern in the file. This import is necessary for the subsequent registration of the component.
28-28: Summary: Changes align with PR objectivesThe addition of the UpdateAppScreen import and its registration in the component setup process aligns well with the PR objectives of implementing an upgrade wall feature. These changes lay the groundwork for prompting users to upgrade when using older versions of the app.
The implementation follows NextJS best practices and maintains consistent code style with the rest of the file. Good use of TypeScript ensures type safety for the new component.
To ensure the UpdateAppScreen is implemented correctly, let's verify its existence and basic structure:
Also applies to: 107-107
apps/native/app/src/screens/passkey/passkey.tsx (4)
18-18: LGTM: Updated illustration import.The change to import 'digital-services-m1-dots.png' instead of 'digital-services-m1.png' looks good. This update likely reflects an intentional change to use a newer version of the illustration.
Line range hint
136-219: LGTM: Improved button layout with ButtonWrapper.The use of the
ButtonWrappercomponent for the buttons enhances layout consistency and improves the overall structure of the component. The functionality of the buttons remains intact, which is good.
Line range hint
1-234: Note: NextJS best practices not applicable.This file is a React Native component and does not directly involve NextJS. Therefore, the NextJS best practices mentioned in the coding guidelines are not applicable to this specific file.
Line range hint
1-234: LGTM: Appropriate TypeScript usage.The file demonstrates proper use of TypeScript, including type annotations and imports. The changes made do not introduce any TypeScript-related issues, and the overall usage remains consistent and appropriate.
apps/native/app/src/screens/home/home.tsx (3)
31-31: LGTM: Import statement for version checking utility.The import for
needsToUpdateAppVersionis correctly placed and follows proper naming conventions.
34-34: LGTM: Import statement for navigation utility.The import for
navigateTois correctly placed and follows proper naming conventions.
Line range hint
1-420: Summary: Upgrade wall feature successfully implemented.The changes in this file successfully implement the upgrade wall feature as described in the PR objectives. The implementation:
- Correctly imports necessary utilities.
- Implements a version check function using async/await.
- Integrates the version check into the component's lifecycle.
- Uses a non-closable navigation to the update screen when needed.
The code follows React and TypeScript best practices, making good use of hooks like
useCallbackanduseEffect. The implementation is well-integrated into the existingMainHomeScreencomponent.One minor suggestion for improvement has been made regarding error handling in the
checkAppVersionfunction.Overall, this implementation meets the requirements and maintains good code quality.
apps/native/app/ios/IslandApp.xcodeproj/project.pbxproj (4)
553-556: Consistency check for OTHER_LDFLAGS changesThe changes to
OTHER_LDFLAGSare consistent across all three configurations (Testing, Release, and Debug). This consistency is good practice and reduces the risk of configuration-specific issues.To ensure that the changes are indeed consistent across all configurations, you can run the following command to compare the
OTHER_LDFLAGSsettings:#!/bin/bash # Compare OTHER_LDFLAGS across all configurations echo "Testing configuration:" sed -n '/name = Testing/,/name = /p' IslandApp.xcodeproj/project.pbxproj | grep OTHER_LDFLAGS echo "\nRelease configuration:" sed -n '/name = Release/,/name = /p' IslandApp.xcodeproj/project.pbxproj | grep OTHER_LDFLAGS echo "\nDebug configuration:" sed -n '/name = Debug/,/name = /p' IslandApp.xcodeproj/project.pbxproj | grep OTHER_LDFLAGSAlso applies to: 628-631, 751-754
751-754: Modification of OTHER_LDFLAGS in Debug configurationThe
OTHER_LDFLAGSsetting has been changed from a single string to an array format, consistent with the changes in the Testing and Release configurations.To ensure this change doesn't introduce any build issues, please verify the build process in the Debug configuration. You can run the following command to check if there are any linker warnings or errors:
#!/bin/bash # Check for linker warnings or errors in the Debug configuration xcodebuild -project IslandApp.xcodeproj -scheme IslandApp -configuration Debug clean build | grep -i "ld: warning" || echo "No linker warnings found."
628-631: Modification of OTHER_LDFLAGS in Release configurationThe
OTHER_LDFLAGSsetting has been changed from a single string to an array format, consistent with the change in the Testing configuration.To ensure this change doesn't introduce any build issues, please verify the build process in the Release configuration. You can run the following command to check if there are any linker warnings or errors:
553-556: Modification of OTHER_LDFLAGS in Testing configurationThe
OTHER_LDFLAGSsetting has been changed from a single string to an array format. This change is consistent with modern Xcode practices for specifying build flags.To ensure this change doesn't introduce any build issues, please verify the build process in the Testing configuration. You can run the following command to check if there are any linker warnings or errors:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/native/app/src/utils/minimum-app-version.ts (2)
5-18: Function implementation looks good, consider adding error handling.The
needsToUpdateAppVersionfunction is well-implemented and follows a logical flow. It correctly handles the case when the minimum supported version is not available. The use of async/await for feature flag retrieval is appropriate.Consider adding try-catch blocks to handle potential errors from
DeviceInfo.getVersion()orcompareVersions.compare(). This would make the function more robust and prevent unexpected crashes. For example:export const needsToUpdateAppVersion = async (): Promise<boolean> => { try { const minimumVersionSupported = await featureFlagClient?.getValueAsync( 'minimumSupportedAppVersion', '1.0.0', ) if (!minimumVersionSupported) { return false } const currentVersion = DeviceInfo.getVersion() return compareVersions.compare(minimumVersionSupported, currentVersion, '>') } catch (error) { console.error('Error checking app version:', error) return false // or handle the error as appropriate for your use case } }
5-18: Consider adding explicit type annotations for improved type safety.While the function's return type is correctly annotated, adding explicit type annotations for
minimumVersionSupportedandcurrentVersionvariables would enhance code readability and type safety.Consider adding the following type annotations:
export const needsToUpdateAppVersion = async (): Promise<boolean> => { const minimumVersionSupported: string | undefined = await featureFlagClient?.getValueAsync( 'minimumSupportedAppVersion', '1.0.0', ) if (!minimumVersionSupported) { return false } const currentVersion: string = DeviceInfo.getVersion() return compareVersions.compare(minimumVersionSupported, currentVersion, '>') }This change would make the types of these variables explicit and potentially catch type-related issues earlier in the development process.
apps/native/app/src/stores/preferences-store.ts (1)
35-35: LGTM! Consider adding a brief comment.The addition of
skippedSoftUpdateto thePreferencesStoreinterface is appropriate and aligns with the PR objectives. The boolean type is suitable for this flag.Consider adding a brief comment above this property to explain its purpose, e.g.:
/** Indicates whether the user has skipped a soft update prompt */ skippedSoftUpdate: booleanThis would enhance code readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/native/app/src/screens/update-app/update-app.tsx (1 hunks)
- apps/native/app/src/stores/preferences-store.ts (2 hunks)
- apps/native/app/src/utils/minimum-app-version.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/native/app/src/screens/update-app/update-app.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/native/app/src/stores/preferences-store.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/utils/minimum-app-version.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/native/app/src/utils/minimum-app-version.ts (2)
1-3: LGTM: Imports are well-structured and appropriate.The import statements are clear, concise, and follow good practices. They import the necessary dependencies for version comparison and device information retrieval.
1-18: LGTM: Adheres to NextJS best practices for utility functions.The file location and implementation align well with NextJS best practices for organizing utility functions. The
needsToUpdateAppVersionfunction is platform-agnostic and can be used in both server-side and client-side contexts, which is beneficial for a NextJS application.apps/native/app/src/stores/preferences-store.ts (2)
75-75: LGTM! Default value is appropriate.Setting
skippedSoftUpdatetofalsein thedefaultPreferencesobject is correct. This ensures that the initial state assumes the user hasn't skipped any soft updates, which is the expected default behavior.
Line range hint
1-156: Overall, the changes are well-implemented and follow best practices.The additions to the
PreferencesStoreinterface anddefaultPreferencesobject are consistent and properly typed. The file continues to adhere to NextJS and TypeScript best practices, making effective use of Zustand for state management. These changes enhance the app's ability to track whether a user has skipped a soft update, aligning well with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/native/app/src/screens/update-app/update-app.tsx (1)
49-65: LGTM: Component definition and props are well-structured.The
UpdateAppScreencomponent is defined correctly with appropriate props and follows React Native best practices. The use ofuseEffectto dynamically update navigation options based on theclosableprop is a good approach.Consider adding a comment explaining the purpose of the
closableprop and its impact on the component's behavior. This would improve code readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/native/app/src/screens/home/home.tsx (2 hunks)
- apps/native/app/src/screens/update-app/update-app.tsx (1 hunks)
- apps/native/app/src/utils/minimum-app-version.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/native/app/src/utils/minimum-app-version.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/native/app/src/screens/home/home.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/update-app/update-app.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (5)
apps/native/app/src/screens/update-app/update-app.tsx (2)
1-37: LGTM: Imports and styled components are well-organized.The imports cover all necessary components and utilities for the
UpdateAppScreen. The styled components are defined clearly and follow the project's styling conventions. The previous comment aboutmargin-verticalhas been addressed in theTextstyled component.
39-47: LGTM: Navigation options are well-configured.The navigation options are appropriately set for a modal screen, with the top bar hidden and hardware back button behavior configured. The use of
createNavigationOptionHookspromotes reusability and consistency across the app.apps/native/app/src/screens/home/home.tsx (3)
31-31: LGTM: New imports are correctly added and follow best practices.The new imports for
needsToUpdateAppVersionandnavigateToare correctly placed and follow proper naming conventions. They are imported from relative paths, which is a good practice for internal modules.Also applies to: 34-34
259-264: LGTM: NewisAppUpdateRequiredfunction is well-implemented.The new async function is correctly implemented using
useCallbackfor memoization. It properly handles the asynchronousneedsToUpdateAppVersioncall and conditionally navigates to the update screen. The function name is clear and descriptive, addressing the past review comment.
272-273: LGTM:useEffecthook updated correctly to include app version check.The
isAppUpdateRequiredfunction is appropriately added to theuseEffecthook, ensuring that the app version check is performed when the component mounts. The added comment clearly explains the purpose of the new line, enhancing code readability.
What
Adding support for upgrade wall to the app. Minimum supported version is configured as a feature flag text in ConfigCat, so if users have older versions of the app they will get prompted with an upgrade wall.
The upgrade wall has a prop closable, if that is set to true we will display a "soft" upgrade wall, i.e. closable one.
I added a prop to preferencesStore called skippedSoftUpdate so if we are displaying a soft update wall then we keep track if the user clicks the Skip button or closes the modal with the x. So then we can check on that flag next time when deciding if we want to show a soft update wall. (Note: one bug is that we don't set this preference flag if the user swipes down the soft update wall - looked into it for a bit but did not find a good way for it, deciding it is not worth the effort for now - car revisit later if we want).
Also added custom user agent header to all api requests from the app on the format:
IslandIsApp (<versionNumber>) Build/<buildNumber>(<Platform>/<PlatformVersion>)For example:
IslandIsApp (1.4.3) Build/143 (ios/18)IslandIsApp (1.4.3) Build/143 (android/31)I have verified in Data Dog that the user agent is coming all the way (for both ios and android):
(see link to request here)
Why
When subpoena/law and order functionality goes live we want to have the option to use an upgrade wall for the app if needed.
Screenshots / Gifs
ios:
RPReplay_Final1727344778.mov
android:
Screen.Recording.Sept.26.from.Google.Play.Store.mp4
Closable upgrade wall:
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Documentation