Repository navigation
Feedbacks - #83
Feedbacks#83
Conversation
WalkthroughThis pull request encompasses modifications across multiple files, primarily focusing on configuration updates, permission adjustments, and minor UI/layout refinements. The changes span the Android application's configuration ( Changes
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (3)
components/collaboration/collaboration-details/SettingsTabContent.tsx (2)
151-157: Consider moving niche options to a configuration file.The hardcoded niche options should be moved to a separate configuration file for better maintainability and reusability. This would also make it easier to update the options without modifying the component code.
+ // src/constants/niches.ts + export const CONTENT_NICHES = [ + { label: "Fashion", value: "Fashion" }, + { label: "Lifestyle", value: "Lifestyle" }, + { label: "Food", value: "Food" }, + { label: "Travel", value: "Travel" }, + { label: "Health", value: "Health" }, + ] as const;Then update the component:
+ import { CONTENT_NICHES } from "@/constants/niches"; <Select - items={[ - { label: "Fashion", value: "Fashion" }, - { label: "Lifestyle", value: "Lifestyle" }, - { label: "Food", value: "Food" }, - { label: "Travel", value: "Travel" }, - { label: "Health", value: "Health" }, - ]} + items={CONTENT_NICHES} ... />
150-174: Consider memoizing the Select component props.The Select component's props could be memoized to prevent unnecessary re-renders, especially since the items array is recreated on each render.
+ const selectItems = useMemo(() => [ + { label: "Fashion", value: "Fashion" }, + { label: "Lifestyle", value: "Lifestyle" }, + { label: "Food", value: "Food" }, + { label: "Travel", value: "Travel" }, + { label: "Health", value: "Health" }, + ], []); + const handleSelect = useCallback((selectedOptions: SelectItem[]) => { + const selectedValues = selectedOptions.map( + (option) => option.value + ); + setNiches(selectedOptions); + updateCollaboration("influencerNiche", selectedValues); + }, [updateCollaboration]); <Select - items={[...]} + items={selectItems} selectItemIcon={true} value={niches} multiselect - onSelect={(selectedOptions) => {...}} + onSelect={handleSelect} selectItemStyle={{ height: 40, }} style={{ marginBottom: 16, }} />eas.json (1)
10-12: Consider using Android App Bundle instead of APK.The build configuration uses APK format across all environments. Google Play Store prefers Android App Bundle (AAB) format for production builds as it reduces app size and enables dynamic delivery.
Consider updating the production build configuration:
"production": { "autoIncrement": true, "android": { - "buildType": "apk" + "buildType": "app-bundle" }, "env": {} }Also applies to: 16-18, 23-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app.json(2 hunks)components/collaboration/collaboration-details/SettingsTabContent.tsx(1 hunks)components/contracts/active.tsx(2 hunks)components/contracts/past.tsx(2 hunks)components/drawer-layout/DrawerMenuContent.tsx(2 hunks)eas-prebuild.sh(1 hunks)eas.json(1 hunks)styles/CollaborationDetails.styles.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- components/contracts/past.tsx
- components/contracts/active.tsx
🔇 Additional comments (7)
styles/CollaborationDetails.styles.tsx (1)
9-9: LGTM! Good spacing adjustment.The addition of
paddingBottom: 24to the scrollContainer provides appropriate spacing and prevents content from being cut off at the bottom of the scroll view.components/drawer-layout/DrawerMenuContent.tsx (1)
32-32: LGTM! Clean icon replacement.The changes simplify the "My Brand" menu item by replacing the conditional FontAwesome icon with a static ProfileIcon component. This improves code maintainability by reducing dependencies.
Also applies to: 74-74
eas.json (1)
3-3: Verify CLI version downgrade compatibility.The CLI version requirement has been relaxed from >=12.6.0 to >=12.5.0. Please ensure this doesn't introduce compatibility issues with other dependencies.
✅ Verification successful
Version downgrade to >=12.5.0 is safe
The change from >=12.6.0 to >=12.5.0 is a minor version downgrade with both versions being stable and actively maintained releases. No compatibility issues were found in the package metadata.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for compatibility issues between EAS CLI versions npm info eas-cli@12.5.0 peerDependencies npm info eas-cli@12.6.0 peerDependencies # List installed dependencies that might be affected npm ls | grep -E "expo|eas"Length of output: 7575
Script:
#!/bin/bash # Fetch npm package info for eas-cli to check release notes npm view eas-cli@12.5.0 version description npm view eas-cli@12.6.0 version description # Check if there are any versions between 12.5.0 and 12.6.0 npm view eas-cli versions --json | jq -r '.[]' | grep -E '^12\.[5-6]\.'Length of output: 335
eas-prebuild.sh (1)
23-23: LGTM! Repository URL updated.The Git remote URL has been correctly updated to reflect the project's focus on brands instead of users.
app.json (3)
91-93: LGTM! Router origin disabled.The router configuration disables origin handling, which is appropriate for a mobile-first application.
95-95: LGTM! Project configuration updated.Project ID and owner have been updated to reflect the new project structure.
Also applies to: 98-98
37-41: Review and justify the new Android permissions.Several sensitive permissions have been added:
- Location (coarse and fine)
- External storage (read and write)
- Media location
These permissions require strong justification as they:
- Impact user privacy (especially location)
- Require explicit user consent
- May affect app store approval
Please ensure these permissions are essential for core functionality. Run this script to verify permission usage:
✅ Verification successful
Android permissions are justified by implementation
The requested permissions align with the app's functionality:
- Location permissions support geolocation features in collaboration creation
- Storage and media permissions enable document/image upload capabilities implemented across multiple components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for location-related API usage echo "Checking location API usage..." rg -l "getCurrentPosition|watchPosition|getLastKnownPosition|requestForegroundPermissionsAsync|requestBackgroundPermissionsAsync" # Search for storage-related API usage echo "Checking storage API usage..." rg -l "MediaLibrary|StorageAccessFramework|DocumentPicker"Length of output: 619
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/settings/index.tsx (2)
Line range hint
58-77: Consider extracting theme options to constants.The theme selection options are hardcoded within the component. Consider moving these to a constants file for better maintainability.
+ import { THEME_OPTIONS } from '@/constants/themes'; <SelectGroup - items={[ - { label: "Light", value: "light" }, - { label: "Dark", value: "dark" }, - ]} + items={THEME_OPTIONS} selectedItem={{ label: selectedTheme, value: selectedTheme, }}
80-80: Move inline styles to StyleSheet.The button's inline styles should be moved to the StyleSheet for better performance and maintainability.
// In Settings.styles.tsx const styles = (theme: Theme) => StyleSheet.create({ settingsContainer: { // ... existing styles }, + saveButton: { + width: '100%', + paddingHorizontal: 20, + }, }); // In component - style={{ width: "100%", paddingHorizontal: 20 }} + style={styles.saveButton}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/collaboration/collaboration-details/SettingsTabContent.tsx(5 hunks)components/settings/index.tsx(3 hunks)components/ui/select/index.tsx(1 hunks)styles/settings/Settings.styles.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/ui/select/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/collaboration/collaboration-details/SettingsTabContent.tsx
🔇 Additional comments (3)
styles/settings/Settings.styles.tsx (1)
9-10: Consider standardizing padding values using a theme constant.The padding values (16) should ideally come from a theme constant to maintain consistency across the app. This makes it easier to update spacing globally.
Let's check if there's a theme constant file that defines spacing:
✅ Verification successful
Consider standardizing padding values using a theme constant - Confirmed
After checking the codebase:
- No centralized spacing system or theme constants for padding were found
- The current implementation uses hard-coded values across multiple style files
- Theme.ts only handles colors and basic theme properties
The original suggestion to standardize padding values through theme constants would indeed improve maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for theme/spacing constants rg -l "spacing|padding" --type ts --type tsx "constants|theme|styles"Length of output: 99
Script:
#!/bin/bash # First, find all potential theme-related files echo "=== Finding theme-related files ===" fd -e ts -e tsx "(theme|spacing|constant|style)" # Then search for padding/spacing patterns in these files echo -e "\n=== Searching for spacing/padding patterns ===" rg "spacing|padding" -t typescript -t tsxLength of output: 1770
Script:
#!/bin/bash # Check content of Theme.ts echo "=== Theme.ts content ===" cat constants/Themes/Theme.ts # Fallback search for padding patterns without file type flags echo -e "\n=== Padding patterns in style files ===" find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "padding"Length of output: 4995
components/settings/index.tsx (2)
11-11: LGTM! Clean import of ContentWrapper component.The import statement follows the project's path aliasing convention using
@/.
Line range hint
58-85: Verify theme persistence across app restarts.The theme selection implementation looks good, but we should ensure the selected theme persists across app restarts.
Let's check if theme persistence is implemented:
✅ Verification successful
Theme persistence is properly implemented ✅
The theme selection is persisted through the manager settings storage system using the
useStorageStatehook. The app reads the stored theme preference frommanager.settings.themewith a fallback to the system color scheme, ensuring theme persistence across app restarts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AsyncStorage or similar persistence implementations rg -A 5 "AsyncStorage|Storage" --type ts --type tsxLength of output: 81
Script:
#!/bin/bash # Search for storage and theme-related implementations rg -t js -t ts -t jsx -t tsx "AsyncStorage|localStorage|Storage" -A 5 # Search for theme-related state management rg -t js -t ts -t jsx -t tsx "theme.*storage|storage.*theme|persist.*theme" -i -A 5 # Look for theme context or provider implementations rg -t js -t ts -t jsx -t tsx "ThemeProvider|ThemeContext|useTheme" -A 5Length of output: 315
Script:
#!/bin/bash # Search for storage implementations without file type filters rg "AsyncStorage|localStorage|Storage" # Search for theme-related persistence rg "theme.*storage|storage.*theme|persist.*theme" -i # Look for theme providers and context rg "ThemeProvider|ThemeContext|useTheme"Length of output: 23637
Script:
#!/bin/bash # Look for theme change implementation rg "themeChange|setTheme|changeTheme" -A 5 # Look for theme-related state management rg "appTheme.*=|setAppTheme" -A 5Length of output: 5250
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/channel/channel.tsx (1)
77-89: Consider enhancing the badge reset implementation.While the badge reset functionality is a good addition, consider these improvements:
- Add a cleanup function to handle any pending reset operations
- Add loading state handling to prevent potential UI flickers
- Include more context in the error message for better debugging
useEffect(() => { + let isMounted = true; const resetBadgeCount = async () => { if (Platform.OS === "ios" || Platform.OS === "android") { try { await Notifications.setBadgeCountAsync(0); } catch (error) { - console.error("Failed to reset badge count:", error); + console.error( + `Failed to reset badge count on ${Platform.OS}:`, + error + ); } } }; resetBadgeCount(); + return () => { + isMounted = false; + }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
components/channel/channel.tsx(2 hunks)package.json(1 hunks)
🔇 Additional comments (2)
components/channel/channel.tsx (1)
2-2: LGTM! Required imports added for badge reset functionality.The new imports are correctly added to support the badge count reset feature.
Also applies to: 5-5
package.json (1)
56-56: LGTM! Correctly added expo-notifications dependency.The dependency is properly added with an appropriate version range using caret notation.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
contexts/notification-context.provider.tsx (1)
9-19: Validate necessity of all Firestore imports
Some of these Firestore imports (e.g.,where,getDocs) might not be directly required for the logic handling notifications via an external endpoint. Reviewing and removing any unused imports can help keep the codebase concise.hooks/request/use-applications.tsx (1)
106-109: Consider logging or checking for delivery success
While the notification is sent viasendNotification, no part of the calling code checks for a non-200 response or other potential errors. Adding minimal error handling or user feedback can improve reliability.components/BottomSheetActions.tsx (1)
74-77: Consider improving notification sending sequence.While the notification parameters are correct, there's a potential race condition in the execution sequence. Consider awaiting the notification sending before navigation.
- sendNotification( + await sendNotification( { users: [cardId.influencerID], }, { data: { collaborationId: data.collaboration.id, }, title: "Application Accepted", description: `Your application for ${data.collaboration.name} has been accepted`, timeStamp: Date.now(), isRead: false, type: "application-accepted", }, ); router.navigate(`/channel/${channel.cid}`);utils/push-notification/push-notification-token.native.ts (1)
17-21: Consider using Platform.select for cleaner platform-specific logic.The platform check could be more elegantly handled using Platform.select:
- if (Platform.OS === "ios") { - newUpdatedTokens = removeToken("ios", user, token); - } else if (Platform.OS === "android") { - newUpdatedTokens = removeToken("android", user, token); - } + newUpdatedTokens = Platform.select({ + ios: () => removeToken("ios", user, token), + android: () => removeToken("android", user, token), + default: () => null, + })();utils/token.ts (1)
25-45: Consider extracting common token operation logic.The
removeTokenfunction shares similar structure withnewToken. Consider extracting the common logic:+const updateTokenList = ( + operation: 'add' | 'remove', + platformTokens: string[] = [], + currentToken: string +) => { + const tokenExists = platformTokens.includes(currentToken); + if (operation === 'add' && !tokenExists) { + return platformTokens.concat(currentToken); + } + if (operation === 'remove' && tokenExists) { + return platformTokens.filter((t) => t !== currentToken); + } + return null; +}; export const newToken = ( os: "ios" | "android" | "web", user: Manager, currentToken: string ) => { if (!user) return null; - const createOrUpdateToken = (platformTokens: string[] = []) => { - if (!platformTokens.includes(currentToken)) { - return platformTokens.concat(currentToken); - } - return null; - }; const updatedPushNotificationToken = { ...user?.pushNotificationToken, - [os]: createOrUpdateToken(user?.pushNotificationToken[os]), + [os]: updateTokenList('add', user?.pushNotificationToken[os], currentToken), }; return updatedPushNotificationToken[os] ? updatedPushNotificationToken : null; }; export const removeToken = ( os: "ios" | "android" | "web", user: Manager, currentToken: string ) => { if (!user) return null; - const deleteToken = (platformTokens: string[] = []) => { - if (platformTokens.includes(currentToken)) { - return platformTokens.filter((t) => t !== currentToken); - } - return null; - }; const updatedPushNotificationToken = { ...user?.pushNotificationToken, - [os]: deleteToken(user?.pushNotificationToken[os]), + [os]: updateTokenList('remove', user?.pushNotificationToken[os], currentToken), }; return updatedPushNotificationToken[os] ? updatedPushNotificationToken : null; };contexts/auth-context.provider.tsx (1)
Line range hint
231-244: Convert promise chain to async/await for better error handling.The sign-out process uses a mix of promise chains and async/await. Consider using consistent async/await:
- signOut(AuthApp) - .then(async () => { + try { + await signOut(AuthApp); setSession(""); setManager(null); analyticsLogEvent("signed_out", { id: manager?.id, email: manager?.email, }); resetAndNavigate("/pre-signin"); Toaster.success("Signed Out Successfully!"); - }) - .catch((error) => { + } catch (error) { console.error("Error signing out: ", error); - }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/BottomSheetActions.tsx(3 hunks)contexts/auth-context.provider.tsx(2 hunks)contexts/notification-context.provider.tsx(3 hunks)hooks/request/use-applications.tsx(2 hunks)types/Notification.ts(1 hunks)utils/messaging-web.ts(1 hunks)utils/push-notification/push-notification-token.native.ts(1 hunks)utils/token.ts(2 hunks)
🔇 Additional comments (9)
contexts/notification-context.provider.tsx (2)
26-31: Interface signature aligns with new notification approach
The new union type parameter (INotifications | PushNotificationPayload) is well-structured and accommodates both Firestore-based and push notification data. This is a clean design that promotes flexibility.
158-158: Providing the newsendNotificationmethod in context looks good
Exposing this method via the context ensures consistent notification handling throughout the application.types/Notification.ts (1)
6-11: Interface addition successfully captures push notification fields
The optionalimageproperty is a nice extension for future enhancements. No issues or concerns here.hooks/request/use-applications.tsx (1)
29-29: Refactoring fromcreateNotificationtosendNotificationis consistent
Replacing the old method reference withsendNotificationaligns with the updated approach. Good job ensuring consistency.components/BottomSheetActions.tsx (2)
13-18: LGTM! Improved readability of cardType options.The reformatting of union types improves code readability while maintaining the same functionality.
40-40: Verify notification API migration.The change from
createNotificationtosendNotificationaligns with the notification system refactor. Let's verify all usages are updated correctly.✅ Verification successful
Notification API migration is complete and consistent
The codebase shows no remaining instances of
createNotificationand demonstrates consistent usage of the newsendNotificationAPI across all files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of old notification API # and verify consistent usage of new API echo "Checking for any remaining createNotification usage..." rg "createNotification" -A 5 echo "Verifying sendNotification usage pattern..." rg "sendNotification" -A 5Length of output: 3063
utils/messaging-web.ts (1)
1-1: LGTM! Clean addition of deleteToken capability.The addition of
deleteTokenimport and export is consistent with Firebase messaging API and properly supports the token cleanup implementation.Also applies to: 6-6
utils/push-notification/push-notification-token.native.ts (1)
15-23: Potential race condition between token operations.There's a potential race condition between getting the token and deleting it. Consider moving the deleteToken operation before getting the current token:
- const token = await messaging().getToken(); - - if (Platform.OS === "ios") { - newUpdatedTokens = removeToken("ios", user, token); - } else if (Platform.OS === "android") { - newUpdatedTokens = removeToken("android", user, token); - } - - await messaging().deleteToken(); + const token = await messaging().getToken(); + newUpdatedTokens = Platform.select({ + ios: () => removeToken("ios", user, token), + android: () => removeToken("android", user, token), + default: () => null, + })(); + + if (newUpdatedTokens) { + await messaging().deleteToken(); + }contexts/auth-context.provider.tsx (1)
223-229: Verify successful token cleanup before proceeding.Add verification that token cleanup was successful:
✅ Verification successful
Token cleanup verification successful
The codebase implements proper token cleanup through the
deleteTokenfunction inutils/token.ts, and the token update logic includes appropriate null checks. The platform-specific token management (iOS, Android, Web) is handled consistently across the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any lingering push notification tokens after sign-out # Search for token persistence patterns rg -l 'pushNotificationToken.*\b(ios|android|web)\b' --type ts # Look for token cleanup implementations ast-grep --pattern 'function $FUNC($ARGS) { $$$ deleteToken($$$) $$$ }'Length of output: 144
Script:
#!/bin/bash # Search for token-related functions and management rg -l "updateManager|updatedTokens|pushNotification" --type ts # Look for specific token management patterns ast-grep --pattern 'async function $FUNC($PARAMS) { $$$ pushNotificationToken $$$ }' # Search for token-related code with context rg "pushNotificationToken" --type ts -C 2Length of output: 3299
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contexts/auth-context.provider.tsx (1)
220-230:⚠️ Potential issueEnhance error handling in token cleanup process.
The token cleanup process should handle errors gracefully and ensure sign-out proceeds even if token cleanup fails.
const signOutManager = async () => { + try { if (Platform.OS !== "web") { - // Remove push notification token from the database const newUpdatedTokens = await updatedTokens(manager); if (newUpdatedTokens) { await updateManager(session as string, { pushNotificationToken: newUpdatedTokens, }); } } + } catch (error) { + console.error('Failed to cleanup push notification tokens:', error); + // Continue with sign-out despite token cleanup failure + }
🧹 Nitpick comments (1)
contexts/auth-context.provider.tsx (1)
222-222: Update misleading comment.The comment suggests removing the token, but the code is actually updating it.
- // Remove push notification token from the database + // Update push notification tokens in the database
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contexts/auth-context.provider.tsx(2 hunks)
🔇 Additional comments (1)
contexts/auth-context.provider.tsx (1)
32-33: LGTM!The new imports are correctly placed and properly used in the signOutManager function.
shared-constants #11, shared-libs #83 and shared-uis #84 are merged, so the pointers move off their feature branches and onto dev. The earlier bump was to branch commits only so this PR could be reviewed whole; left there, the pointers would break the moment those branches were deleted. shared-constants → f9bc03b shared-libs → 4d64fb4 shared-uis → e294276 Typecheck re-run against the merged submodules: clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary by CodeRabbit
Release Notes
Permissions
UI/UX Improvements
Build Configuration
Dependencies
Miscellaneous