Skip to content

Dev - #65

Merged
dev-rathore merged 27 commits into
masterfrom
dev
Jan 10, 2025
Merged

Dev#65
dev-rathore merged 27 commits into
masterfrom
dev

Conversation

@dev-rathore

@dev-rathore dev-rathore commented Jan 10, 2025 •

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new BrandProtectedScreen to manage brand-related content access
    • Added a new shimmer loading component for explore influencer screens
    • Enhanced image rendering with a new ImageComponent
  • Improvements

    • Streamlined user profile image display across multiple components
    • Updated color constants to support shimmer effects in dark and light themes
    • Refined authentication context and sign-up process
  • UI/UX Updates

    • Modified screen header back action handling
    • Updated layout and styling for various components

Manan-Sharma-5 and others added 24 commits January 2, 2025 16:47
* redesign application and invitation cards
* repurpose messaging screen

* accept application changes

* added phases

* invitationId as influencerId

* fixed signup signin

* fix: bug on brand creation

* minor changes

* minor changes

* minor changes

* minor changes
* fix: accept application

* fix: managerId
* fix: initial redirections

* scheme change

* router navigate fix

* router navigate fix
* redirection changes

* redirection changes
* added no brands protected screen and shimmer ui

* shimmer ui update

* added loading indicator
@coderabbitai

coderabbitai Bot commented Jan 10, 2025 •

Copy link
Copy Markdown

Walkthrough

This pull request introduces several significant changes across multiple files, focusing on enhancing the application's layout, image rendering, and brand protection mechanisms. The most notable additions include a new BrandProtectedScreen component that manages content display based on brand associations, and a new ImageComponent that replaces various image rendering approaches across different components. The changes aim to standardize image handling, improve user experience, and implement brand-level access control.

Changes

File/Path Change Summary
app/(main)/(drawer)/_layout.tsx Added BrandProtectedScreen wrapper for Drawer component
app/(main)/(screens)/(onboarding)/onboarding-your-brand.tsx Improved brand creation error handling and submission logic
app/index.tsx Added splash image with centered layout
Multiple Components Replaced Image/Avatar.Image with new ImageComponent
layouts/protected/brand-protected-screen.tsx New component for managing brand-based content access
constants/Colors.ts Added shimmer-related color properties

Sequence Diagram

sequenceDiagram
    participant User
    participant BrandProtectedScreen
    participant Firestore
    participant Router

    User->>BrandProtectedScreen: Attempts to access content
    BrandProtectedScreen->>Firestore: Fetch brands for manager
    alt Brands found
        Firestore-->>BrandProtectedScreen: Return brand data
        BrandProtectedScreen->>BrandProtectedScreen: Set hasBrandWithManagerId to true
        BrandProtectedScreen->>User: Render children components
    else No brands found
        BrandProtectedScreen->>Router: Redirect to onboarding
        Router->>User: Show onboarding screen
    end
Loading

Possibly related PRs

Poem

🐰 A Rabbit's Tale of Code Delight

Images now dance with grace so bright,
Brands protected, screens held tight,
Components shimmer, colors gleam,
In this coding rabbit's sweet dream!

Hop, hop, hooray! 🎉

Finishing Touches

  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🔭 Outside diff range comments (1)
components/collaboration-card/card-components/CollaborationHeader.tsx (1)

Line range hint 51-61: Improve accessibility and styling consistency.

  1. The alt text should be more descriptive for accessibility.
  2. The style prop overrides conflict with the size and shape props of ImageComponent.
 <ImageComponent
   url={brandImage}
-  altText="Image"
+  altText={`${brandName} brand logo`}
   shape="circle"
   size="small"
-  style={{
-    width: 40,
-    height: 40,
-    borderRadius: 5,
-  }}
 />
🧹 Nitpick comments (13)
app/index.tsx (1)

15-18: Consider using dimensions for responsive sizing

Using percentage values for image dimensions might cause unexpected scaling on different screen sizes.

-style={{
-  width: '100%',
-  height: '100%',
-}}
+style={{
+  width: Dimensions.get('window').width,
+  height: Dimensions.get('window').height,
+}}

Don't forget to import Dimensions from 'react-native'.

constants/Colors.ts (1)

22-23: Consider extracting shimmer colors as constants

The shimmer colors are hardcoded. Consider extracting them as constants at the top of the file for better maintainability.

 const tintColorLight = "#ff6d2d";
 const tintColorDark = "#fff";
+const shimmerBackgroundDark = "#1E1E1E";
+const shimmerOverlayDark = "#1b1b1b";
+const shimmerBackgroundLight = "#f1f3f4";
+const shimmerOverlayLight = "#eee";

 export default (theme: Theme) => ({
   ...theme.colors,
   ...(theme.dark
     ? {
         // ... other colors
-        shimmerBackground: "#1E1E1E",
-        shimmerOverlay: "#1b1b1b",
+        shimmerBackground: shimmerBackgroundDark,
+        shimmerOverlay: shimmerOverlayDark,
       }
     : {
         // ... other colors
-        shimmerBackground: "#f1f3f4",
-        shimmerOverlay: "#eee",
+        shimmerBackground: shimmerBackgroundLight,
+        shimmerOverlay: shimmerOverlayLight,
       }),

Also applies to: 38-39

components/ui/screen-header/index.tsx (1)

46-51: Use theme spacing for margins

Consider using theme-based spacing instead of hardcoded values for consistency.

 style={{
   marginTop: 2,
-  marginLeft: 10,
+  marginLeft: theme.spacing?.md ?? 10,
 }}
components/collaboration/collaboration-details/modal/ManagerModal.tsx (1)

43-54: Consider removing redundant style properties.

The style prop appears to override the size and shape props. Since ImageComponent already handles sizing and shape through its props, consider removing the explicit style properties for width, height, and borderRadius.

 <ImageComponent
   url={managerImage}
   altText={managerName}
   shape="circle"
   size="medium"
-  style={{
-    width: 120,
-    height: 120,
-    borderRadius: 240,
-  }}
   initials={managerName}
   initialsSize={40}
 />
layouts/protected/brand-protected-screen.tsx (1)

67-90: Consider unifying loading screens.

The platform-specific loading screens might create inconsistent user experiences. Consider implementing a unified loading component that maintains brand consistency across platforms.

components/brand-profile/members-card/index.tsx (1)

50-56: Improve image fallback handling.

The empty string fallback for url prop could trigger unnecessary error states in the ImageComponent. Consider using a default avatar image or undefined.

 <ImageComponent
   size="small"
   shape="circle"
   initials={manager.name}
-  url={manager.profileImage || ""}
+  url={manager.profileImage || undefined}
   altText="Image"
 />
components/shimmers/explore-influencer-shimmer.tsx (2)

12-67: Add TypeScript interface and accessibility attributes

The component implementation looks good, but could benefit from the following improvements:

  1. Add TypeScript interface to define component props
  2. Add accessibility attributes for better screen reader support
+interface ExploreInfluencerShimmerProps {
+  testID?: string;
+}

-const ExploreInfluencerShimmer = () => {
+const ExploreInfluencerShimmer: React.FC<ExploreInfluencerShimmerProps> = ({ testID }) => {
   const theme = useTheme();

   return (
-    <AppLayout>
+    <AppLayout testID={testID}>
       <View style={styles.container}>
         <View style={styles.header}>
           <View style={styles.headerLeft}>
-            <Shimmer width={80} height={36} />
+            <Shimmer 
+              width={80} 
+              height={36}
+              accessibilityLabel="Loading header content" 
+            />

56-62: Extract magic numbers into constants

Consider extracting the repeated values into named constants for better maintainability.

+const BOTTOM_NAV = {
+  ITEMS: 5,
+  ITEM_WIDTH: 56,
+  ITEM_HEIGHT: 48,
+};

-          {[...Array(5)].map((_, index) => (
+          {[...Array(BOTTOM_NAV.ITEMS)].map((_, index) => (
             <Shimmer
               key={index}
-              width={56}
-              height={48}
+              width={BOTTOM_NAV.ITEM_WIDTH}
+              height={BOTTOM_NAV.ITEM_HEIGHT}
components/collaboration/collaboration-details/modal/BrandModal.tsx (1)

Line range hint 46-58: Type the style prop for ImageComponent

The implementation looks good, but consider adding proper typing for the style prop to ensure type safety.

         <ImageComponent
           url={brand.image}
           altText="Brand Image"
           shape="square"
           size="medium"
-          style={{
+          style={StyleSheet.create({
+            brandImage: {
             width: 120,
             height: 120,
             borderRadius: 10,
-          }}
+            }
+          }).brandImage}
         />
components/menu/index.tsx (1)

Line range hint 93-117: Extract UserProfile into a separate component

Consider extracting the user profile section into a separate component for better maintainability and reusability.

+interface UserProfileProps {
+  manager: {
+    profileImage?: string;
+    name: string;
+    email: string;
+  };
+  onPress: () => void;
+}
+
+const UserProfile: React.FC<UserProfileProps> = ({ manager, onPress }) => (
+  <Pressable onPress={onPress}>
+    <View style={styles.userProfileContainer}>
+      <ImageComponent
+        url={manager?.profileImage || ""}
+        initials={manager?.name}
+        shape="circle"
+        size="small"
+        altText="User profile image"
+        style={styles.avatar}
+      />
+      <View style={styles.textContainer}>
+        <Text style={styles.titleText}>{manager?.name}</Text>
+        <Text style={{ opacity: 0.8 }}>{manager?.email}</Text>
+      </View>
+      <FontAwesomeIcon
+        color={Colors(theme).text}
+        icon={faChevronRight}
+        size={20}
+        style={styles.chevron}
+      />
+    </View>
+  </Pressable>
+);

// In the Menu component:
-          <Pressable
-            onPress={() => {
-              router.push("/profile");
-            }}
-          >
-            <View style={styles.userProfileContainer}>
-              <ImageComponent
-                url={manager?.profileImage || ""}
-                initials={manager?.name}
-                shape="circle"
-                size="small"
-                altText="Image"
-                style={styles.avatar}
-              />
-              <View style={styles.textContainer}>
-                <Text style={styles.titleText}>{manager?.name}</Text>
-                <Text style={{ opacity: 0.8 }}>{manager?.email}</Text>
-              </View>
-              <FontAwesomeIcon
-                color={Colors(theme).text}
-                icon={faChevronRight}
-                size={20}
-                style={styles.chevron}
-              />
-            </View>
-          </Pressable>
+          <UserProfile
+            manager={manager}
+            onPress={() => router.push("/profile")}
+          />
components/collaboration/collaboration-details/OverviewTabContent.tsx (1)

190-194: Consider standardizing image style handling.

While the implementation is functional, the inline style props could be handled within the ImageComponent for better consistency.

Consider moving the style configuration into the ImageComponent:

-                  <ImageComponent
-                    url={props.collaboration.logo}
-                    altText="Brand Logo"
-                    shape="square"
-                    size="small"
-                    style={{ width: 40, height: 40, borderRadius: 5 }}
-                  />
+                  <ImageComponent
+                    url={props.collaboration.logo}
+                    altText="Brand Logo"
+                    shape="square"
+                    size="small"
+                  />
components/contract-card/ContractHeader.tsx (1)

50-55: Consider enhancing the alt text for accessibility.

While the implementation is correct, the alt text "Image" is not descriptive enough for screen readers.

Consider using a more descriptive alt text:

-          altText="Image"
+          altText={`${username}'s profile picture`}
components/contracts/ActionContainer.tsx (1)

217-222: Consider extracting duplicate styles into a shared constant.

Both ImageComponent instances share identical styling properties. Consider extracting these into a shared constant to maintain consistency and reduce duplication.

const profileImageStyle = {
  width: 40,
  height: 40,
  borderRadius: 20,
} as const;

// Then use it in both instances:
style={profileImageStyle}

Also applies to: 271-276

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 281e1cf and 8725456.

📒 Files selected for processing (21)
  • app/(main)/(drawer)/_layout.tsx (1 hunks)
  • app/(main)/(screens)/(onboarding)/onboarding-your-brand.tsx (4 hunks)
  • app/index.tsx (1 hunks)
  • components/InfluencerCard.tsx (7 hunks)
  • components/brand-profile/members-card/index.tsx (2 hunks)
  • components/collaboration-card/card-components/CollaborationHeader.tsx (2 hunks)
  • components/collaboration/collaboration-details/OverviewTabContent.tsx (3 hunks)
  • components/collaboration/collaboration-details/modal/BrandModal.tsx (2 hunks)
  • components/collaboration/collaboration-details/modal/ManagerModal.tsx (2 hunks)
  • components/contract-card/ContractHeader.tsx (2 hunks)
  • components/contracts/ActionContainer.tsx (3 hunks)
  • components/contracts/AddMemberModal.tsx (2 hunks)
  • components/menu/index.tsx (2 hunks)
  • components/profile/index.tsx (4 hunks)
  • components/shimmers/explore-influencer-shimmer.tsx (1 hunks)
  • components/ui/screen-header/index.tsx (3 hunks)
  • constants/Colors.ts (2 hunks)
  • contexts/auth-context.provider.tsx (2 hunks)
  • layouts/protected/brand-protected-screen.tsx (1 hunks)
  • layouts/protected/index.ts (1 hunks)
  • shared-uis (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • layouts/protected/index.ts
  • shared-uis
  • components/InfluencerCard.tsx
🔇 Additional comments (10)
app/(main)/(drawer)/_layout.tsx (1)

11-28: Verify BrandProtectedScreen implementation

The wrapping of Drawer with BrandProtectedScreen looks correct, but let's verify the implementation details.

✅ Verification successful

BrandProtectedScreen implementation and usage verified

The component is properly implemented as an authentication guard for brand-related screens, and its usage in the drawer layout is correct. The component handles loading states appropriately and correctly wraps the Drawer component.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify BrandProtectedScreen implementation and its usage
# Check for BrandProtectedScreen implementation
echo "Checking BrandProtectedScreen implementation..."
rg -A 10 "export const BrandProtectedScreen" layouts/protected/

# Check for other usages of BrandProtectedScreen
echo "Checking other usages of BrandProtectedScreen..."
rg "BrandProtectedScreen" --type tsx

Length of output: 342


Script:

#!/bin/bash
# Find BrandProtectedScreen component file
echo "Searching for BrandProtectedScreen file..."
fd -e tsx -e ts "BrandProtectedScreen"

# Check implementation and usages with proper file type
echo "Checking implementation and usages..."
rg -t typescript -t tsx "BrandProtectedScreen" -A 10

Length of output: 306


Script:

#!/bin/bash
# Find any file containing BrandProtectedScreen
echo "Searching for files containing BrandProtectedScreen..."
rg -l "BrandProtectedScreen"

# Show the implementation with surrounding context
echo "Checking implementation details..."
rg -i "BrandProtectedScreen" -B 5 -A 10

Length of output: 4139

components/contracts/AddMemberModal.tsx (1)

23-23: LGTM! Image component migration looks good.

The migration from Avatar.Image to ImageComponent is implemented correctly with all necessary props (size, url, initials, altText) properly configured.

Also applies to: 115-120

components/profile/index.tsx (2)

32-32: LGTM! Image component migration looks good.

The ImageComponent implementation includes all necessary props and proper styling configuration.

Also applies to: 163-169


94-94: Good addition of fetchRole call.

Adding fetchRole after profile update ensures the role state stays in sync with the backend.

contexts/auth-context.provider.tsx (1)

183-184: Verify the impact of role removal from userData.

The role property has been removed from the userData object during sign-up. Please ensure this aligns with the new role management approach.

Run this script to check role usage across the codebase:

components/collaboration/collaboration-details/OverviewTabContent.tsx (1)

449-454: LGTM! Manager profile image implementation looks good.

The ImageComponent implementation for the manager profile includes all necessary props and proper configuration.

components/contract-card/ContractHeader.tsx (1)

11-11: LGTM! Import of standardized ImageComponent.

The addition of ImageComponent aligns with the PR's goal of standardizing image rendering across the application.

components/contracts/ActionContainer.tsx (3)

20-20: LGTM! Import of standardized ImageComponent.

The addition of ImageComponent aligns with the standardization of image rendering.


217-222: LGTM! Proper null handling for manager profile.

The implementation correctly handles cases where manager data might be undefined, providing empty string and name fallbacks.


271-276: LGTM! Proper implementation for user profile.

The implementation correctly uses user data with appropriate fallbacks for the profile image.

Comment thread app/index.tsx
Comment on lines +13 to +19
<Image
source={require("@/assets/images/splash.png")}
style={{
width: '100%',
height: '100%',
}}
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add accessibility and error handling to Image component

The Image component should include accessibility props and error handling for better user experience.

 <Image
   source={require("@/assets/images/splash.png")}
+  accessibilityLabel="Splash screen"
+  accessibilityRole="image"
+  onError={(e) => console.error('Error loading splash image:', e.nativeEvent.error)}
   style={{
     width: '100%',
     height: '100%',
   }}
+  resizeMode="contain"
 />
📝 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.

Suggested change
<Image
source={require("@/assets/images/splash.png")}
style={{
width: '100%',
height: '100%',
}}
/>
<Image
source={require("@/assets/images/splash.png")}
accessibilityLabel="Splash screen"
accessibilityRole="image"
onError={(e) => console.error('Error loading splash image:', e.nativeEvent.error)}
style={{
width: '100%',
height: '100%',
}}
resizeMode="contain"
/>

Comment on lines +53 to +62
<Pressable onPress={handleAction}>
<FontAwesomeIcon
icon={faArrowLeft}
size={20}
color={Colors(theme).text}
style={{
alignItems: "center",
}}
/>
</Pressable>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance Pressable implementation for better UX

The Pressable component needs improvements for better user experience.

-<Pressable onPress={handleAction}>
+<Pressable
+  onPress={handleAction}
+  hitSlop={10}
+  style={({ pressed }) => [
+    {
+      opacity: pressed ? 0.7 : 1,
+    },
+  ]}
+  accessibilityRole="button"
+  accessibilityLabel="Go back"
+>
   <FontAwesomeIcon
     icon={faArrowLeft}
     size={20}
     color={Colors(theme).text}
-    style={{
-      alignItems: "center",
-    }}
   />
 </Pressable>
📝 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.

Suggested change
<Pressable onPress={handleAction}>
<FontAwesomeIcon
icon={faArrowLeft}
size={20}
color={Colors(theme).text}
style={{
alignItems: "center",
}}
/>
</Pressable>
<Pressable
onPress={handleAction}
hitSlop={10}
style={({ pressed }) => [
{
opacity: pressed ? 0.7 : 1,
},
]}
accessibilityRole="button"
accessibilityLabel="Go back"
>
<FontAwesomeIcon
icon={faArrowLeft}
size={20}
color={Colors(theme).text}
/>
</Pressable>

Comment on lines +54 to +58
useEffect(() => {
if (!manager?.id) return;

fetchBrands(manager.id);
}, [manager]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add cleanup for useEffect.

The effect should be cleaned up to prevent state updates after unmount.

 useEffect(() => {
   if (!manager?.id) return;
 
-  fetchBrands(manager.id);
+  let mounted = true;
+  
+  const fetch = async () => {
+    try {
+      await fetchBrands(manager.id);
+    } catch (error) {
+      if (mounted) {
+        // Handle error state
+      }
+    }
+  };
+  
+  fetch();
+  
+  return () => {
+    mounted = false;
+  };
 }, [manager]);
📝 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.

Suggested change
useEffect(() => {
if (!manager?.id) return;
fetchBrands(manager.id);
}, [manager]);
useEffect(() => {
if (!manager?.id) return;
let mounted = true;
const fetch = async () => {
try {
await fetchBrands(manager.id);
} catch (error) {
if (mounted) {
// Handle error state
}
}
};
fetch();
return () => {
mounted = false;
};
}, [manager]);

Comment on lines +25 to +52
const fetchBrands = async (
managerId: string
): Promise<void> => {
const brandsCollection = collection(FirestoreDB, "brands");

const brandsSnapshot = await getDocs(brandsCollection);

for (const brandDoc of brandsSnapshot.docs) {
const brandId = brandDoc.id;

const membersCollection = collection(FirestoreDB, `brands/${brandId}/members`);
const membersQuery = query(membersCollection, where("managerId", "==", managerId));
const membersSnapshot = await getDocs(membersQuery);

if (!membersSnapshot.empty) {
setHasBrandWithManagerId(true);
return;
}
}

// If the manager has no brands, redirect to onboarding
router.replace({
pathname: "/onboarding-your-brand",
params: {
firstBrand: "true",
},
});
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize Firestore query performance.

The current implementation fetches all brands and then filters them, which is inefficient. Consider restructuring the data model to allow direct querying of brands by managerId.

 const fetchBrands = async (managerId: string): Promise<void> => {
-  const brandsCollection = collection(FirestoreDB, "brands");
-  const brandsSnapshot = await getDocs(brandsCollection);
-
-  for (const brandDoc of brandsSnapshot.docs) {
-    const brandId = brandDoc.id;
-    const membersCollection = collection(FirestoreDB, `brands/${brandId}/members`);
-    const membersQuery = query(membersCollection, where("managerId", "==", managerId));
-    const membersSnapshot = await getDocs(membersQuery);
-
-    if (!membersSnapshot.empty) {
-      setHasBrandWithManagerId(true);
-      return;
-    }
-  }
+  try {
+    const membersQuery = query(
+      collectionGroup(FirestoreDB, 'members'),
+      where('managerId', '==', managerId),
+      limit(1)
+    );
+    const membersSnapshot = await getDocs(membersQuery);
+    setHasBrandWithManagerId(!membersSnapshot.empty);
+    
+    if (membersSnapshot.empty) {
+      router.replace({
+        pathname: "/onboarding-your-brand",
+        params: { firstBrand: "true" },
+      });
+    }
+  } catch (error) {
+    console.error('Error fetching brands:', error);
+    // Handle error appropriately
+  }
 };
📝 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.

Suggested change
const fetchBrands = async (
managerId: string
): Promise<void> => {
const brandsCollection = collection(FirestoreDB, "brands");
const brandsSnapshot = await getDocs(brandsCollection);
for (const brandDoc of brandsSnapshot.docs) {
const brandId = brandDoc.id;
const membersCollection = collection(FirestoreDB, `brands/${brandId}/members`);
const membersQuery = query(membersCollection, where("managerId", "==", managerId));
const membersSnapshot = await getDocs(membersQuery);
if (!membersSnapshot.empty) {
setHasBrandWithManagerId(true);
return;
}
}
// If the manager has no brands, redirect to onboarding
router.replace({
pathname: "/onboarding-your-brand",
params: {
firstBrand: "true",
},
});
};
const fetchBrands = async (
managerId: string
): Promise<void> => {
try {
const membersQuery = query(
collectionGroup(FirestoreDB, 'members'),
where('managerId', '==', managerId),
limit(1)
);
const membersSnapshot = await getDocs(membersQuery);
setHasBrandWithManagerId(!membersSnapshot.empty);
if (membersSnapshot.empty) {
router.replace({
pathname: "/onboarding-your-brand",
params: { firstBrand: "true" },
});
}
} catch (error) {
console.error('Error fetching brands:', error);
// Handle error appropriately
}
};

Comment on lines 62 to +66
const blob = await fetch(brandData.image).then((res) => res.blob());
imageUrl = await uploadImageBytes(blob, `brands/${brandData.name}-${Date.now()}`);
imageUrl = await uploadImageBytes(
blob,
`brands/${brandData.name}-${Date.now()}`
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for image upload

The image upload lacks proper error handling and cleanup.

     let imageUrl = "";
     if (brandData.image) {
-      const blob = await fetch(brandData.image).then((res) => res.blob());
-      imageUrl = await uploadImageBytes(
-        blob,
-        `brands/${brandData.name}-${Date.now()}`
-      );
+      try {
+        const response = await fetch(brandData.image);
+        const blob = await response.blob();
+        imageUrl = await uploadImageBytes(
+          blob,
+          `brands/${brandData.name}-${Date.now()}`
+        );
+      } catch (error) {
+        setIsSubmitting(false);
+        Toaster.error("Failed to upload brand image");
+        return;
+      }
     }
📝 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.

Suggested change
const blob = await fetch(brandData.image).then((res) => res.blob());
imageUrl = await uploadImageBytes(blob, `brands/${brandData.name}-${Date.now()}`);
imageUrl = await uploadImageBytes(
blob,
`brands/${brandData.name}-${Date.now()}`
);
let imageUrl = "";
if (brandData.image) {
try {
const response = await fetch(brandData.image);
const blob = await response.blob();
imageUrl = await uploadImageBytes(
blob,
`brands/${brandData.name}-${Date.now()}`
);
} catch (error) {
setIsSubmitting(false);
Toaster.error("Failed to upload brand image");
return;
}
}

Comment on lines +89 to +105
role: "Manager",
})
.then(() => {
router.replace({
pathname: "/onboarding-get-started",
params: {
brandId: docRef.id,
firstBrand: firstBrand === "true" ? "true" : "false",
},
});
setSelectedBrand(brandData as Brand);
})
.catch((error) => {
Toaster.error("Error creating brand");
})
.finally(() => {
setIsSubmitting(false);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling and role management

The implementation has several issues:

  1. Hardcoded role value
  2. Potential race condition in state updates
  3. Incomplete error handling
+const BRAND_ROLES = {
+  MANAGER: "Manager",
+} as const;

       await setDoc(managerRef, {
         managerId: user.id,
-        role: "Manager",
+        role: BRAND_ROLES.MANAGER,
       })
         .then(() => {
+          setSelectedBrand(brandData as Brand);
           router.replace({
             pathname: "/onboarding-get-started",
             params: {
               brandId: docRef.id,
               firstBrand: firstBrand === "true" ? "true" : "false",
             },
           });
-          setSelectedBrand(brandData as Brand);
         })
         .catch((error) => {
-          Toaster.error("Error creating brand");
+          Toaster.error(`Error creating brand: ${error.message}`);
         })
         .finally(() => {
           setIsSubmitting(false);
         });
📝 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.

Suggested change
role: "Manager",
})
.then(() => {
router.replace({
pathname: "/onboarding-get-started",
params: {
brandId: docRef.id,
firstBrand: firstBrand === "true" ? "true" : "false",
},
});
setSelectedBrand(brandData as Brand);
})
.catch((error) => {
Toaster.error("Error creating brand");
})
.finally(() => {
setIsSubmitting(false);
const BRAND_ROLES = {
MANAGER: "Manager",
} as const;
role: BRAND_ROLES.MANAGER,
})
.then(() => {
setSelectedBrand(brandData as Brand);
router.replace({
pathname: "/onboarding-get-started",
params: {
brandId: docRef.id,
firstBrand: firstBrand === "true" ? "true" : "false",
},
});
})
.catch((error) => {
Toaster.error(`Error creating brand: ${error.message}`);
})
.finally(() => {
setIsSubmitting(false);

@dev-rathore
dev-rathore merged commit c101c7d into master Jan 10, 2025
@coderabbitai coderabbitai Bot mentioned this pull request Jan 11, 2025
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants