AppCheck - Test5 - borked git filter branch caused 1038 commits to show in diff with 28 files changed :/ #1368
Conversation
Increment version numbers
Safely unwrap a few keys Log to Sentry when one fails
chore: Update Version Number
fix: Query Alignment
More logs for 1:1 sync
feat: Component Tests
Bump SDK Bump App Version for next build
chore: Bump Xmtp and App Versions
…o-client fix: expo dev client infinite reloading
Reply to long messages
Add tap to dismiss conversation context menu
Increment version numbers
Chat component refactor
fix: DB Encryption Safety
Add type safety when setting spamscore Co-authored-by: Alex Risch <[email protected]>
Co-authored-by: Alex Risch <[email protected]>
Remove Platform checks as there is an ios specific file: screens/Navigation/ConversationRequestsListNav.ios.tsx Co-authored-by: Alex Risch <[email protected]>
Added App Release Action Removed schedule on internal action Increment app version for next build Co-authored-by: Alex Risch <[email protected]>
Increment Android build Make Release notes Co-authored-by: Alex Risch <[email protected]>
Don't run commit Co-authored-by: Alex Risch <[email protected]>
This PR increments the buildNumber for iOS and the versionCode for Android. Co-authored-by: Alex Risch <[email protected]>
* feat: Informative Alert for non v3 Users Updated text for alert Added selector for profile information Added additional text to translations * Update i18n/translations/en.ts Co-authored-by: Saul Carlin <[email protected]> --------- Co-authored-by: Alex Risch <[email protected]> Co-authored-by: Saul Carlin <[email protected]>
* wip * wip * wip * wip * performance improvment * remove service + small fixes * rebase and fix tsc
* many fixes * fix animation * fix translations
* WIP * feat: DM Push Notifications and Native iOS Profile Updates Updated profile handling Added handling for V3 DM messages * remove log
Co-authored-by: alexrisch <[email protected]>
from google engingeer a while back - its okay to have this file be public: https://groups.google.com/g/firebase-talk/c/bamCgTDajkw
Co-authored-by: alexrisch <[email protected]>
WalkthroughThis pull request introduces comprehensive Firebase App Check integration across the Converse application. The changes span multiple files and platforms (iOS, Android), adding new dependencies, configuration files, and utility functions to implement secure app authentication. The modifications include updating build configurations, adding Firebase-related dependencies, implementing app check token retrieval, and enhancing error handling for API interactions. Changes
Sequence DiagramsequenceDiagram
participant App as Mobile App
participant Firebase as Firebase App Check
participant API as Backend API
App->>Firebase: Initialize App Check
Firebase-->>App: Provide App Check Provider
App->>Firebase: Request App Check Token
Firebase-->>App: Return App Check Token
App->>API: Make API Request with App Check Token
API->>Firebase: Verify App Check Token
Firebase-->>API: Token Validation Result
API-->>App: Response
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
technoplato
changed the title
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
scripts/build/ios/production.js (1)
Line range hint
24-102: Add error handling for file operationsThe script performs multiple critical file operations without proper error handling, which could lead to incomplete configuration if any operation fails.
Consider wrapping file operations in try-catch blocks:
+try { const newProjContent = projContent .replace( /PRODUCT_BUNDLE_IDENTIFIER = com\.converse\.dev/g, "PRODUCT_BUNDLE_IDENTIFIER = com.converse.native" ) // ... other replacements ... fs.writeFileSync(PROJ_PATH, newProjContent); +} catch (error) { + console.error(`Failed to update ${PROJ_PATH}:`, error); + process.exit(1); +}
🧹 Nitpick comments (16)
utils/api.ts (1)
215-219: Simplify the error message for missing App Check tokenThe error message thrown when
appCheckTokenis not available is verbose and includes line breaks, which may not display well in logs or error tracking tools. Consider simplifying it for clarity and better readability.Suggested change:
-if (!appCheckToken) { - throw new Error(` -No App Check Token Available. This indicates that we believe the app is not running on an authentic build of -our application on a device that has not been tampered with. -`); +if (!appCheckToken) { + throw new Error("No App Check Token available. The application may not be running on an authentic build."); +}utils/appCheck.ts (3)
11-13: Address the TODO regardinggetLimitedUseTokenThere is a comment questioning the use of
getLimitedUseToken:// Do we need/want to use the limited use token?Please decide whether to use
getLimitedUseTokenorgetToken, and update the code accordingly. Leaving this undecided may lead to unintended behavior.Would you like assistance in determining the appropriate method for retrieving the App Check token?
16-17: Remove commented-out code to maintain code cleanlinessThere's a commented-out line of code that appears to be an alternative implementation:
// const { token } = await appCheck.getToken();Consider removing this line if it's no longer needed to keep the codebase clean and maintainable.
30-31: Complete the TODO: Add the App Check debug token to environment variablesA TODO comment indicates that the App Check debug token needs to be retrieved and added to the environment:
/* TODO: get key and add to env*/Ensure that the debug token is obtained and properly configured in the environment variables to enable App Check during development.
Would you like assistance in generating the steps to obtain and configure the App Check debug token?
app.config.ts (2)
38-39: Clarify the commented-outexpo-build-propertiespluginThe
expo-build-propertiesplugin is commented out in the configuration:// ["expo-build-properties", { ios: { useFrameworks: "static" } }],Please confirm if this is intentional. If the plugin is required for proper build configuration, consider uncommenting it. If not needed, it might be better to remove it to avoid confusion.
56-57: Implement dynamic setting ofgoogleServicesFilebased on environmentA TODO comment suggests dynamically setting the
googleServicesFilefor Android App Check:// TODO(lustig): dynamically set this based on env in Android PR for AppCheck googleServicesFile: "./android/google-services.json",Please implement the logic to dynamically assign the correct
googleServicesFilebased on the environment to ensure that the correct configuration is used for different builds.Would you like assistance in implementing the dynamic configuration for
googleServicesFile?scripts/build/build.utils.js (1)
67-78: Improve error message clarity for missing configurationThe error message could be more helpful by suggesting next steps.
if (!fs.existsSync(SOURCE)) { const availableFiles = findGoogleServiceFiles(); throw new Error( - `Source file not found: ${SOURCE}\n\n` + + `Firebase configuration file not found for environment "${env}": ${SOURCE}\n\n` + `Available GoogleService-Info files:\n${ availableFiles.length ? availableFiles.map((f) => `- ${f}`).join("\n") - : " None found" + : " None found. Please ensure Firebase configuration files are properly set up." }` ); }ios/Converse/AppDelegate.mm (1)
26-36: Remove or implement environment-specific configurationThe commented code for environment-specific configuration should either be implemented or removed to avoid confusion.
Consider implementing the environment-specific configuration or removing the commented code. If implementing, ensure proper error handling:
NSString *firebasePlist = [[NSBundle mainBundle] pathForResource:@"GoogleService-Info" ofType:@"plist"]; NSError *configureError = nil; #if DEBUG firebasePlist = [[NSBundle mainBundle] pathForResource:@"GoogleService-Info-dev" ofType:@"plist"]; #endif if (![[NSFileManager defaultManager] fileExistsAtPath:firebasePlist]) { NSLog(@"Error: Firebase configuration file not found at %@", firebasePlist); return NO; } FIROptions *options = [[FIROptions alloc] initWithContentsOfFile:firebasePlist]; if (![FIRApp configureWithOptions:options error:&configureError]) { NSLog(@"Error configuring Firebase with options: %@", configureError); return NO; }README.md (2)
162-162: Use markdown link syntax for the repository URLReplace the bare URL with proper markdown link syntax for better readability and consistency.
-1. Obtain the Firebase config files from the private repository at https://github.com/ephemeraHQ/converse-app-env +1. Obtain the Firebase config files from the private repository at [converse-app-env](https://github.com/ephemeraHQ/converse-app-env)🧰 Tools
🪛 Markdownlint (0.37.0)
162-162: null
Bare URL used(MD034, no-bare-urls)
53-53: Specify language for code blocksAdd language specifiers to the fenced code blocks for better syntax highlighting.
-``` +```bash yarnAlso applies to: 78-78 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 53-53: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>config.ts (1)</summary><blockquote> Line range hint `73-128`: **Consider adding TypeScript types and documentation.** The new `appCheckDebugToken` property should be properly typed and documented across all environment configurations. ```diff + // Type definition for environment configuration + type EnvironmentConfig = { + // ... other properties + /** + * Firebase App Check debug token for development purposes. + * Only used in iOS development environment. + */ + appCheckDebugToken: string | undefined; + }; const ENV = { dev: { // ... configuration }, preview: { // ... configuration }, prod: { // ... configuration }, - } as const; + } satisfies Record<string, EnvironmentConfig>;ios/Podfile (1)
85-96: Enhance documentation for architecture settings.The commented architecture settings could benefit from more detailed documentation explaining when they should be uncommented.
- # Recommend in case of build errors by https://rnfirebase.io/app-check/usage - # I'm not seeing any errors, but leaving this here just in case + # Architecture settings for Firebase App Check + # Uncomment these settings if you encounter build errors related to architecture compatibility + # Reference: https://rnfirebase.io/app-check/usage#ios-configurationios/ConverseNotificationExtension/Spam.swift (1)
188-191: Optimize the address state check usingwhereclause.The current implementation can be more concise using Swift's
whereclause.- if let senderAddresses = try await group.members.first(where: {$0.inboxId == senderInboxId})?.addresses { - for address in senderAddresses { - if try await client.preferences.addressState(address: address) == .denied { - return 1 - } - } + if let senderAddresses = try await group.members.first(where: {$0.inboxId == senderInboxId})?.addresses { + if let _ = try await senderAddresses.first(where: { try await client.preferences.addressState(address: $0) == .denied }) { + return 1 + }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 188-188:
whereclauses are preferred over a singleifinside afor(for_where)
screens/Navigation/Navigation.tsx (3)
197-198: Remove TODO comment and clarify integration statusThe TODO comment suggests this is temporary code. Please provide a timeline for completing the integration and removing this test screen.
199-227: Add loading state and improve error handling in TestAppCheckScreenWhile the implementation is functional, consider these improvements:
- Add loading state while fetching token
- Implement proper error type handling instead of using JSON.stringify
- Add retry mechanism for failed token retrieval
const TestAppCheckScreen = () => { const [token, setToken] = useState<string | undefined>(undefined); - const [key, setKey] = useState<string | undefined>(undefined); const [error, setError] = useState<string | undefined>(undefined); + const [isLoading, setIsLoading] = useState(false); useEffect(() => { setupAppAttest(); }, []); return ( <View> <Text>TestAppCheckScreen</Text> <Button text="TestAppCheckScreen" + disabled={isLoading} onPress={() => { + setIsLoading(true); + setError(undefined); tryGetAppCheckToken() .then((token) => { setToken(token); }) .catch((error) => { logger.error("Error getting token", error); - setError(JSON.stringify(error)); + setError(error.message || 'Failed to get token'); }) + .finally(() => { + setIsLoading(false); + }); }} /> + {isLoading && <Text>Loading...</Text>} <Text> {`${token?.substring(0, 10)}...${token?.substring(token.length - 10)}` ?? "no token"} </Text> <Text>Last Error: {error ?? "no error"}</Text> </View> ); };
245-248: Consider moving test screen to a dedicated testing environmentThe test screen is currently commented out but exists in the production navigation code. Consider moving it to a separate testing environment or feature flag it properly.
📜 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 (26)
.gitignore(1 hunks)App.tsx(2 hunks)README.md(6 hunks)app.config.ts(1 hunks)app.json(1 hunks)config.ts(3 hunks)eas.json(0 hunks)features/conversation/conversation-message/conversation-message-sender-avatar.tsx(0 hunks)ios/Converse.xcodeproj/project.pbxproj(23 hunks)ios/Converse/AppDelegate.mm(2 hunks)ios/Converse/Converse.entitlements(1 hunks)ios/ConverseNotificationExtension/Spam.swift(1 hunks)ios/Podfile(2 hunks)package.json(2 hunks)queries/QueryKeys.ts(1 hunks)screens/Navigation/Navigation.tsx(4 hunks)screens/NewAccount/NewAccountEphemeraScreen.tsx(0 hunks)scripts/build/build.js(2 hunks)scripts/build/build.utils.js(1 hunks)scripts/build/ios/preview.js(4 hunks)scripts/build/ios/production.js(5 hunks)utils/api.constants.ts(1 hunks)utils/api.ts(7 hunks)utils/appCheck.ts(1 hunks)utils/date.test.ts(1 hunks)utils/mmkv.ts(1 hunks)
💤 Files with no reviewable changes (3)
- screens/NewAccount/NewAccountEphemeraScreen.tsx
- eas.json
- features/conversation/conversation-message/conversation-message-sender-avatar.tsx
✅ Files skipped from review due to trivial changes (4)
- utils/api.constants.ts
- app.json
- .gitignore
- ios/Converse/Converse.entitlements
🧰 Additional context used
🪛 SwiftLint (0.57.0)
ios/ConverseNotificationExtension/Spam.swift
[Warning] 188-188: where clauses are preferred over a single if inside a for
(for_where)
🪛 Markdownlint (0.37.0)
README.md
53-53: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
78-78: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
162-162: null
Bare URL used
(MD034, no-bare-urls)
🪛 LanguageTool
README.md
[style] ~99-~99: Consider shortening or rephrasing this to strengthen your wording.
Context: ...sts sh yarn test:perf:baseline Make changes to the code to see the performance impact ...
(MAKE_CHANGES)
[style] ~120-~120: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ... Branches Each release branch is based off of main or the release branch before it....
(EN_WORDINESS_PREMIUM_OFF_OF)
[style] ~152-~152: It might be better to use ‘times’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ... ``` ### Exceptions There are certain times where this flow does not work as intended. Fo...
(WHEN_WHERE)
[style] ~154-~154: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...Build scripts: These may need to be run off of the default main branch instead of feat...
(EN_WORDINESS_PREMIUM_OFF_OF)
🔇 Additional comments (13)
utils/api.ts (1)
122-122: Verify all usages of updated AuthParams type
The AuthParams type now requires appCheckToken as a mandatory field. Please ensure that all instances where AuthParams is used have been updated to include the appCheckToken property to prevent any runtime errors.
Run the following script to identify all usages of AuthParams:
✅ Verification successful
Let me analyze the results and gather more information to verify all usages of AuthParams.
Let me try one more search to find where the authentication parameters are being constructed.
Let me try one final search with a different approach.
All usages of AuthParams are properly updated with appCheckToken
Based on the search results, all instances where AuthParams is used have been properly updated to include the appCheckToken property:
- The
fetchAccessTokenfunction correctly includesappCheckTokenin its parameters and passes it to the API call - The
getXmtpApiHeadersfunction obtainsappCheckTokenviatryGetAppCheckToken()and includes it in the headers - All object literals constructing auth params include the
appCheckTokenfield
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `AuthParams` in the codebase.
rg 'AuthParams' --glob='*.ts' --glob='*.tsx' -A 5 -B 5
Length of output: 947
Script:
#!/bin/bash
# Find all object literals that might be used as arguments where AuthParams is expected
ast-grep --pattern '{
inboxId: $_,
installationPublicKey: $_,
installationKeySignature: $_,
appCheckToken: $_,
account: $_
}'
Length of output: 137
Script:
#!/bin/bash
# Find function calls to methods that accept AuthParams
rg -B 5 -A 5 'createAccessToken|authenticate' --glob='*.ts' --glob='*.tsx'
Length of output: 2866
Script:
#!/bin/bash
# Find where the authentication parameters object is being constructed
rg -B 10 -A 5 'createAccessToken\(' --glob='*.ts' --glob='*.tsx'
Length of output: 66
Script:
#!/bin/bash
# Find where the authentication-related parameters are being used together
rg 'inboxId.+installationPublicKey|installationPublicKey.+inboxId|appCheckToken' --glob='*.ts' --glob='*.tsx' -B 5 -A 5
Length of output: 3002
ios/Converse.xcodeproj/project.pbxproj (2)
6-6: Confirm the objectVersion update from 54 to 70
The objectVersion in the project file has been updated to 70. This usually happens when the project is opened with a newer version of Xcode. Please confirm that this change is intentional and that all team members are using compatible versions of Xcode.
668-668: Verify the impact of updating IPHONEOS_DEPLOYMENT_TARGET to 16.0
The deployment target for the iOS application and extensions has been updated from 14.0 to 16.0. Ensure that this change aligns with the project's support policy and does not exclude a significant portion of the user base who may be on earlier iOS versions.
Run the following script to check the deployment targets across the project:
Also applies to: 706-706, 751-751, 796-796
ios/Converse/AppDelegate.mm (1)
3-4: Verify Firebase SDK version compatibility
Ensure the imported Firebase SDK version is compatible with App Check.
✅ Verification successful
Based on the output, I can generate the final response as we have sufficient information about the Firebase SDK versions and their compatibility.
Firebase SDK versions are compatible with App Check
The Firebase SDK (v11.4.0) and Firebase App Check module (v11.4.0) are properly aligned, with explicit version constraints in place. The RNFBAppCheck module (v21.4.0) is also correctly configured to work with Firebase/AppCheck (<= 11.4.0).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Firebase SDK version in podfile.lock
if [ -f "ios/Podfile.lock" ]; then
echo "Firebase SDK versions:"
grep -A 5 "Firebase/" ios/Podfile.lock
else
echo "Podfile.lock not found"
fi
Length of output: 800
queries/QueryKeys.ts (1)
44-48: 🛠️ Refactor suggestion
Reconsider the handling of undefined accounts
While adding optional chaining makes the code more defensive, it might mask potential issues:
- The
accountparameter is typed as requiredstring, but the code treats it as optional - If
accountis undefined, it likely indicates an upstream issue that should be addressed
Consider either:
- Making the type signature match the runtime behavior by marking account as optional:
-export const conversationsQueryKey = (account: string) =>
+export const conversationsQueryKey = (account?: string) =>- Or enforcing the required nature of the parameter:
-account?.toLowerCase()
+account.toLowerCase()Let's verify how account is used in the codebase:
Also applies to: 54-54, 58-59, 65-65, 70-70, 75-75, 80-80, 85-85, 90-90, 95-95, 100-100, 105-105, 110-110, 114-115, 121-121, 125-126, 132-132, 136-136, 142-142
scripts/build/ios/production.js (1)
19-19: 🛠️ Refactor suggestion
Add validation for Google service info copy operation
The copyGoogleServiceInfo operation is critical for Firebase configuration but lacks validation of the operation's success.
Let's check the implementation of copyGoogleServiceInfo:
Consider adding validation and error handling:
-copyGoogleServiceInfo("production");
+try {
+ const success = copyGoogleServiceInfo("production");
+ if (!success) {
+ throw new Error("Failed to copy Google service info files");
+ }
+} catch (error) {
+ console.error("Failed to configure Firebase:", error);
+ process.exit(1);
+}config.ts (1)
73-75: LGTM! Secure handling of debug token.
The platform-specific handling of appCheckDebugToken in the dev environment is well implemented, ensuring debug tokens are only used for iOS development.
ios/Podfile (1)
31-31: LGTM! Correct Firebase static framework configuration.
The $RNFirebaseAsStaticFramework = true setting is correctly configured as per Firebase documentation.
scripts/build/ios/preview.js (1)
46-46: LGTM! Clean implementation of Firebase configuration copying.
The implementation correctly handles the Firebase configuration file copying and bundle ID updates.
Also applies to: 54-58
App.tsx (1)
45-45: Verify the Firebase App Check initialization.
The Firebase App Check integration looks straightforward, but we should verify the implementation of setupAppAttest to ensure it properly initializes App Check with the correct provider based on the platform.
Also applies to: 69-71
✅ Verification successful
Let me gather more information about the App Check configuration and usage to ensure it's properly implemented.
Firebase App Check implementation verified and properly configured
The implementation of setupAppAttest is secure and well-configured:
- Uses appropriate providers based on platform: Play Integrity for Android and App Attest with Device Check fallback for iOS
- Debug tokens are only enabled in development environment (
__DEV__) - Production builds have debug tokens explicitly set to
undefined - App Check tokens are properly integrated with API calls and authentication flow
- Includes proper error handling when App Check token is unavailable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the setupAppAttest implementation and its dependencies
# Check the implementation of setupAppAttest
echo "Checking setupAppAttest implementation..."
rg -A 10 "export.*setupAppAttest"
# Check for Firebase App Check configuration
echo "Checking Firebase configuration..."
fd "google-services.json|GoogleService-Info.plist"
# Check for other App Check related code
echo "Checking other App Check related code..."
rg -l "FirebaseAppCheck|AppCheck"
Length of output: 1390
Script:
#!/bin/bash
# Check the app config for App Check settings
echo "Checking app config..."
rg -A 5 "appCheck" app.config.ts
# Check API usage of App Check
echo "Checking API integration..."
rg -A 5 "appCheck" utils/api.ts
# Check debug token configuration
echo "Checking debug token configuration..."
rg "appCheckDebugToken" -A 2
Length of output: 2240
scripts/build/build.js (2)
12-37: LGTM: Robust error handling for EAS CLI installation
The implementation provides clear user feedback and graceful error handling for EAS CLI installation.
192-193: LGTM: Clean formatting of file extension logic
The conditional logic for determining file extensions is clear and maintainable.
package.json (1)
62-63: Verify Firebase App Check compatibility with existing dependencies
The Firebase dependencies are properly aligned at version 21.4.0, and expo-build-properties is included as required for Firebase integration.
Also applies to: 243-243
✅ Verification successful
Firebase App Check is properly integrated and configured
The codebase shows proper integration of Firebase App Check:
- Firebase App Check is correctly initialized in
utils/appCheck.ts - App Check token handling is implemented in API calls
- Native integration is set up in iOS (AppDelegate.mm)
- Dependencies are properly aligned at version 21.4.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential conflicts with Firebase dependencies
# Check for other Firebase-related dependencies
rg '"@react-native-firebase/[^"]*"' -g 'package.json'
# Check for minimum required expo version for Firebase compatibility
rg '"expo":\s*"[^"]*"' -g 'package.json'
Length of output: 253
Script:
#!/bin/bash
# Check for any Firebase configuration or setup files
fd -e json -e js -e ts -e tsx firebase
# Check for Firebase initialization code
rg -i "firebase.initializeApp|firebase.app\(\)" --type-add 'web:*.{js,jsx,ts,tsx}'
# Look for app-check related configuration
rg -i "appCheck|AppCheck" --type-add 'web:*.{js,jsx,ts,tsx}'
Length of output: 15271
| const SOURCE = `ios/Google Services/GoogleService-Info-${ | ||
| env === "production" ? "prod" : "preview" | ||
| }.plist`; | ||
| const DEST = "ios/Converse/GoogleService-Info.plist"; |
There was a problem hiding this comment.
Potential path traversal vulnerability
The env parameter is used directly in file path construction without sanitization.
- const SOURCE = `ios/Google Services/GoogleService-Info-${
- env === "production" ? "prod" : "preview"
- }.plist`;
+ const validSuffixes = {
+ production: "prod",
+ preview: "preview"
+ };
+ const suffix = validSuffixes[env];
+ const SOURCE = `ios/Google Services/GoogleService-Info-${suffix}.plist`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SOURCE = `ios/Google Services/GoogleService-Info-${ | |
| env === "production" ? "prod" : "preview" | |
| }.plist`; | |
| const DEST = "ios/Converse/GoogleService-Info.plist"; | |
| const validSuffixes = { | |
| production: "prod", | |
| preview: "preview" | |
| }; | |
| const suffix = validSuffixes[env]; | |
| const SOURCE = `ios/Google Services/GoogleService-Info-${suffix}.plist`; | |
| const DEST = "ios/Converse/GoogleService-Info.plist"; |
| const findGoogleServiceFiles = () => { | ||
| try { | ||
| const result = execSync('find ios -name "GoogleService-Info*.plist"', { | ||
| encoding: "utf8", | ||
| }); | ||
| return result.split("\n").filter(Boolean); | ||
| } catch (error) { | ||
| console.warn(`Failed to find GoogleService-Info files: ${error.message}`); | ||
| return []; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in findGoogleServiceFiles
The function silently returns an empty array on error, which could mask configuration issues. Consider throwing an error or providing more detailed logging.
const findGoogleServiceFiles = () => {
try {
const result = execSync('find ios -name "GoogleService-Info*.plist"', {
encoding: "utf8",
});
return result.split("\n").filter(Boolean);
} catch (error) {
- console.warn(`Failed to find GoogleService-Info files: ${error.message}`);
- return [];
+ const errorMessage = `Failed to find GoogleService-Info files: ${error.message}`;
+ console.error(errorMessage);
+ throw new Error(errorMessage);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const findGoogleServiceFiles = () => { | |
| try { | |
| const result = execSync('find ios -name "GoogleService-Info*.plist"', { | |
| encoding: "utf8", | |
| }); | |
| return result.split("\n").filter(Boolean); | |
| } catch (error) { | |
| console.warn(`Failed to find GoogleService-Info files: ${error.message}`); | |
| return []; | |
| } | |
| }; | |
| const findGoogleServiceFiles = () => { | |
| try { | |
| const result = execSync('find ios -name "GoogleService-Info*.plist"', { | |
| encoding: "utf8", | |
| }); | |
| return result.split("\n").filter(Boolean); | |
| } catch (error) { | |
| const errorMessage = `Failed to find GoogleService-Info files: ${error.message}`; | |
| console.error(errorMessage); | |
| throw new Error(errorMessage); | |
| } | |
| }; |
| export const authMMKVStorage = { | ||
| set(name: AuthStorageKey, value: string) { | ||
| // Deleting before setting to avoid memory leak | ||
| // https://github.com/mrousavy/react-native-mmkv/issues/440 | ||
| authMMKV.delete(name); | ||
| return authMMKV.set(name, value); | ||
| }, | ||
| get(name: AuthStorageKey) { | ||
| const value = authMMKV.getString(name); | ||
| return value ?? null; | ||
| }, | ||
| delete(name: AuthStorageKey) { | ||
| return authMMKV.delete(name); | ||
| }, | ||
| clear() { | ||
| return authMMKV.clearAll(); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety improvements
The storage operations lack error handling and could be more type-safe.
export const authMMKVStorage = {
- set(name: AuthStorageKey, value: string) {
+ set(name: AuthStorageKey, value: string): void {
+ if (!value) {
+ throw new Error(`Cannot set empty value for ${name}`);
+ }
// Deleting before setting to avoid memory leak
// https://github.com/mrousavy/react-native-mmkv/issues/440
authMMKV.delete(name);
- return authMMKV.set(name, value);
+ authMMKV.set(name, value);
},
- get(name: AuthStorageKey) {
+ get(name: AuthStorageKey): string | null {
const value = authMMKV.getString(name);
return value ?? null;
},
- delete(name: AuthStorageKey) {
- return authMMKV.delete(name);
+ delete(name: AuthStorageKey): void {
+ authMMKV.delete(name);
},
- clear() {
- return authMMKV.clearAll();
+ clear(): void {
+ authMMKV.clearAll();
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const authMMKVStorage = { | |
| set(name: AuthStorageKey, value: string) { | |
| // Deleting before setting to avoid memory leak | |
| // https://github.com/mrousavy/react-native-mmkv/issues/440 | |
| authMMKV.delete(name); | |
| return authMMKV.set(name, value); | |
| }, | |
| get(name: AuthStorageKey) { | |
| const value = authMMKV.getString(name); | |
| return value ?? null; | |
| }, | |
| delete(name: AuthStorageKey) { | |
| return authMMKV.delete(name); | |
| }, | |
| clear() { | |
| return authMMKV.clearAll(); | |
| }, | |
| }; | |
| export const authMMKVStorage = { | |
| set(name: AuthStorageKey, value: string): void { | |
| if (!value) { | |
| throw new Error(`Cannot set empty value for ${name}`); | |
| } | |
| // Deleting before setting to avoid memory leak | |
| // https://github.com/mrousavy/react-native-mmkv/issues/440 | |
| authMMKV.delete(name); | |
| authMMKV.set(name, value); | |
| }, | |
| get(name: AuthStorageKey): string | null { | |
| const value = authMMKV.getString(name); | |
| return value ?? null; | |
| }, | |
| delete(name: AuthStorageKey): void { | |
| authMMKV.delete(name); | |
| }, | |
| clear(): void { | |
| authMMKV.clearAll(); | |
| }, | |
| }; |
| const authMMKV = new MMKV({ id: "converse-auth" }); | ||
|
|
||
| type AuthStorageKey = "CONVERSE_REFRESH_TOKEN" | "CONVERSE_ACCESS_TOKEN"; |
There was a problem hiding this comment.
Consider encryption for auth token storage
Auth tokens should be stored with encryption enabled.
-const authMMKV = new MMKV({ id: "converse-auth" });
+const authMMKV = new MMKV({
+ id: "converse-auth",
+ encryptionKey: "REPLACE_WITH_SECURE_KEY" // TODO: Use secure key management
+});Committable suggestion skipped: line range outside the PR's diff.
| [RNFBAppCheckModule sharedInstance]; | ||
|
|
||
| // Firebase Configuration is provided via Google Services/GoogleServices-Info-<env>.plist and the proper file is copied | ||
| // during build via the scripts/build/ios/[preview|prod].js files | ||
| [FIRApp configure]; |
There was a problem hiding this comment.
Add error handling for Firebase initialization
Firebase initialization could fail and should be handled appropriately.
+ NSError *configureError = nil;
[RNFBAppCheckModule sharedInstance];
- [FIRApp configure];
+ if (![FIRApp configureWithOptions:nil error:&configureError]) {
+ NSLog(@"Error configuring Firebase: %@", configureError);
+ // Consider appropriate error handling strategy
+ }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| Android: | ||
|
|
||
| TODO |
There was a problem hiding this comment.
Complete the Android Firebase configuration section
The Android configuration section is marked with TODO and needs to be completed to ensure developers can properly set up Firebase for Android builds.
Would you like me to help document the correct paths for Android Firebase configuration files?
| * The GoogleService-Info.plist file contains Firebase configuration | ||
| * specific to the preview environment. This file is downloaded from: | ||
| * https://console.firebase.google.com/project/converse-unshut-labs/ | ||
| * settings/general/ios:com.converse.preview | ||
| * |
There was a problem hiding this comment.
Remove sensitive Firebase console URL from comments.
The Firebase console URL contains project-specific information that should not be in the source code.
* The GoogleService-Info.plist file contains Firebase configuration
- * specific to the preview environment. This file is downloaded from:
- * https://console.firebase.google.com/project/converse-unshut-labs/
- * settings/general/ios:com.converse.preview
+ * specific to the preview environment. This file should be downloaded
+ * from the Firebase console for the preview environment.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * The GoogleService-Info.plist file contains Firebase configuration | |
| * specific to the preview environment. This file is downloaded from: | |
| * https://console.firebase.google.com/project/converse-unshut-labs/ | |
| * settings/general/ios:com.converse.preview | |
| * | |
| * The GoogleService-Info.plist file contains Firebase configuration | |
| * specific to the preview environment. This file should be downloaded | |
| * from the Firebase console for the preview environment. | |
| * |
Summary by CodeRabbit
New Features
TestAppCheckScreen) for testing App Check token retrieval.Documentation
Bug Fixes
Chores
Tests