Repository navigation
ui-feedback - #54
ui-feedback#54
Conversation
WalkthroughThe pull request introduces mobile-friendly keyboard handling in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 0
🧹 Nitpick comments (3)
styles/CollaborationDetails.styles.tsx (1)
29-29: Consider potential text overflow or truncation.Setting a fixed width of "50%" for the
namestyle might cause unwanted wrapping or truncation on smaller screens. Test various screen sizes to ensure readability.components/contracts/FeedbackModal.tsx (2)
54-54: Early return upon missing feedback or rating is correct.This prevents unnecessary database writes and clarifies the submission flow. Be sure to localize or use a user-friendly prompt instead of
alertfor production.
216-216: Empty style usage.Keep an eye on the empty
keyboardAvoidingViewstyle. It might be a placeholder for future custom styling. You can remove it if it’s not needed to reduce code clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/contracts/FeedbackModal.tsx(4 hunks)styles/CollaborationDetails.styles.tsx(1 hunks)
🔇 Additional comments (2)
components/contracts/FeedbackModal.tsx (2)
2-11: Import management looks good.Adding
KeyboardAvoidingViewand related imports is a good step to enhance user experience on mobile. Be sure to remove any unused imports if they are found in future refactoring.
89-183: KeyboardAvoidingView integration.Wrapping modal content in
KeyboardAvoidingViewis a solid approach to managing keyboard overlap. Validate that padding or height offset is appropriate on different device types and screen sizes, especially on Android devices with varying keyboard heights.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/contract-card/UserResponse.tsx (1)
29-45: Consider robust error handling and fallback for large or slow downloads.
While the download logic is correct, you might run into issues with large files or intermittent network errors. For best user experience, consider adding:
- a loading indicator to inform the user that the download is in progress,
- a retry mechanism if the download fails due to network issues.
components/profile/index.tsx (1)
210-210: Enabling role edits via user input.
Allowing the user to manually edit their role is beneficial. However, consider validating role inputs if your app enforces role-based permissions.
📜 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 (3)
components/contract-card/UserResponse.tsx(4 hunks)components/profile/index.tsx(6 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/profile/index.tsx
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
components/contract-card/UserResponse.tsx (3)
4-4: No issues found regarding new imports.
ImportingPlatform,Pressable, andScrollViewlooks appropriate given their usage in the code below.
Line range hint
112-128: Great usability improvement for file attachments.
Converting the attachment container from a staticViewto aPressablefor initiating downloads is a straightforward and user-friendly approach.🧰 Tools
🪛 Biome (1.9.4)
[error] 109-130: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
9-10: Ensure dependencies are properly installed and aligned with the expo version.
The imports fromexpo-file-systemandexpo-sharingare correct. Just confirm that these versions are compatible with your current expo SDK and properly integrated in both your development and production environments.✅ Verification successful
Dependencies are correctly installed and compatible with Expo SDK
The installed versions are compatible with Expo SDK 51:
- expo-file-system@17.0.1
- expo-sharing@12.0.1
- expo@51.0.21
These versions align properly with the current Expo SDK version and are correctly listed in the package.json. No peer dependency conflicts were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm that expo-file-system and expo-sharing are installed correctly grep '"expo-file-system"' package.json grep '"expo-sharing"' package.jsonLength of output: 142
Script:
#!/bin/bash # Check expo SDK version to verify compatibility grep '"expo":' package.json grep '"expo-status-bar":' package.json # Check if there are any peer dependency warnings or conflicts cat package.json | grep -A 5 "peerDependencies"Length of output: 176
components/profile/index.tsx (6)
12-12: No concerns with Firestore imports.
The updated imports (doc,getDoc,updateDoc) align well with Firestore usage for retrieving and updating documents.
31-31: Use brand context with caution.
Importing and utilizing the selected brand context is helpful for scoping user profiles to a specific brand. Ensure this brand context remains valid across the user session.
40-40: Validate theselectedBrandobject.
When destructuring, always ensure thatselectedBrandhas the required fields before usage or you might encounter undefined references in some edge cases.
78-89: Good logic for brand member role updates.
You are correctly updating the brand's members collection ifselectedBrandis defined. This ensures brand-specific role management.
123-123: CallingfetchRoleon mount is a great approach.
This helps ensure that the user’s role is always up-to-date when the Profile screen is first loaded.
101-116: Optimize the role fetching with safe checks.The static analysis tool suggests using optional chaining. You can simplify lines 102-103 by:
- if (selectedBrand && selectedBrand.id && manager && manager.id) { + if (selectedBrand?.id && manager?.id) {This avoids potential runtime errors if
selectedBrandormanageris null or undefined.✅ Verification successful
Let me verify the usage of optional chaining in the codebase to ensure consistency and validate if this optimization is already being used in similar scenarios.
Based on the search results, I can see that optional chaining (
?.) is widely used throughout the codebase for safely accessing potentially undefined properties. The codebase shows consistent usage of this pattern in similar scenarios, such as accessing user properties, collaboration data, and other nested objects.Use optional chaining for consistent null checks
The suggested optimization is aligned with the codebase's patterns and will make the code more consistent with the rest of the project. The change is safe to make:
- if (selectedBrand && selectedBrand.id && manager && manager.id) { + if (selectedBrand?.id && manager?.id) {This change:
- Follows the established pattern in the codebase
- Reduces verbosity while maintaining null safety
- Makes the code more consistent with similar checks found throughout the project
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for similar patterns of accessing selectedBrand.id and manager.id rg "selectedBrand\." -A 2 -B 2 # Search for optional chaining usage patterns rg "\?\." -A 2 -B 2 # Search for similar Firestore document references ast-grep --pattern 'doc($_, "brands", $_, "members", $_)'Length of output: 65698
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
package.json (1)
81-82: Dependency addition verified.
Including"expo-sharing": "~12.0.1"aligns with the file-sharing feature inUserResponse. No issues identified.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
styles/menu/MenuItem.styles.tsx (1)
31-31: Be mindful of RN version compatibility forgap.Although
gapis a great shorthand for spacing in modern React Native versions, older versions may not support it. For broader compatibility, consider using manual styles like margin or padding on child components.contexts/chat-context.provider.web.tsx (1)
24-24: Default return value is always false.Returning
falseforremoveMemberFromChannelmight confuse consumers of this function. If this is a placeholder, ensure to implement the correct logic or throw an error if it's not yet supported.components/brand-profile/members-card/index.tsx (1)
19-19: Functional component signature updated.Now accepting
cardTypeandaction. Makes the component more modular, but ensure all usage sites supply valid arguments.components/contracts/MemberContainer.tsx (1)
98-110: Implementing the member removal action.The inline
actionprop is clear, callingremoveMemberFromChanneland then refreshing the list. Ensure that error handling is in place ifremoveMemberFromChannelfails.await removeMemberFromChannel(channelId, item.managerId) .then(() => { fetchMembersFromClient(); }) .catch((error) => { + // Consider handling or notifying the user about the error console.error(error); });contexts/chat-context.provider.native.tsx (1)
201-201: Ensure proper testing of the new method.
You are now exposingremoveMemberFromChannelin the context. Make sure your tests cover both successful removal and error-handling scenarios to prevent regressions.components/contracts/past.tsx (2)
170-179: Refactor inline styles for maintainability.
Inline styles with multiple properties can become unwieldy. Consider moving them into a dedicatedStyleSheetor style object for improved clarity and reusability.- style={{ - borderWidth: 0.3, - borderColor: Colors(theme).gray300, - borderRadius: 5, - overflow: "hidden", - }} + style={styles.pressableContainer}
197-197: Preserve accessibility and visual feedback.
When wrapping content withPressable, consider adding an accessible role or feedback effect (e.g.,android_rippleorPressablestyling for iOS) so users receive clear visual cues on press.components/contracts/active.tsx (1)
170-173: Pressable usage for navigation.
ThePressablenicely wraps the list item to navigate to contract details. This is clear and concise.You might consider adding an accessibility label for screen readers, for instance:
<Pressable + accessibilityLabel="Open contract details" onPress={() => { router.push(`/contract-details/${item.streamChannelId}`); }} >components/proposals/Applications.tsx (1)
Line range hint
196-206: Pressable wrapper for CollaborationStats.
WrappingCollaborationStatswithPressableis a straightforward approach allowing easy navigation.If there's any user-facing text, consider specifying an
accessibilityLabelfor better screen reader support.<Pressable + accessibilityLabel="View collaboration details" onPress={() => { router.push(`/collaboration-details/${item.id}`); }} >components/proposals/Invitations.tsx (1)
9-9: Pressable import for interactivity.
Bringing inPressablefromreact-nativealigns with the pattern used across your codebase for clickable elements.Ensure consistent usage of
Pressablevs. older clickable wrappers (TouchableOpacity, etc.) across the app for maintainability.components/contracts/ActionContainer.tsx (1)
179-189: Remove the unnecessary Fragment.
Static analysis indicates that the fragment here contains only a single<Button>element. You can safely remove the fragment without affecting the layout.- <> <Button ... > Give Feedback </Button> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 179-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
components/contract-card/UserResponse.tsx (3)
110-127: Add loading state to download buttonThe Pressable component should reflect the download state to improve user feedback.
<Pressable key={index} onPress={() => downloadAndSaveFile(attachment.url, attachment.name) } - style={{ flexDirection: "row", alignItems: "center", gap: 4 }} + style={({ pressed }) => [ + { + flexDirection: "row", + alignItems: "center", + gap: 4, + opacity: (pressed || isDownloading) ? 0.7 : 1, + } + ]} + disabled={isDownloading} > - <FontAwesomeIcon icon={faPaperclip} /> + {isDownloading ? ( + <ActivityIndicator size="small" /> + ) : ( + <FontAwesomeIcon icon={faPaperclip} /> + )} <Text key={index} style={{ fontSize: 16, textDecorationLine: "underline", }} > - {attachment.name} + {isDownloading + ? `Downloading ${attachment.name} (${Math.round(downloadProgress * 100)}%)` + : attachment.name + } </Text> </Pressable>
35-39: Simplify platform-specific codeThe web and mobile branches have identical code which can be simplified.
- if (Platform.OS === "web") { - Sharing.shareAsync(uri); - } else { - await Sharing.shareAsync(uri); - } + await Sharing.shareAsync(uri);
153-162: Use template literals for better readabilityThe current string concatenation can be improved using template literals.
- Q{") "} - {influencerQuestions[answer.question]} + {`Q) ${influencerQuestions[answer.question]}`} - A{") "} - {answer.answer} + {`A) ${answer.answer}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
components/brand-profile/members-card/index.tsx(2 hunks)components/collaboration-card/card-components/ChipComponent.tsx(1 hunks)components/contract-card/UserResponse.tsx(6 hunks)components/contracts/ActionContainer.tsx(1 hunks)components/contracts/ContractDetailContent.tsx(1 hunks)components/contracts/MemberContainer.tsx(2 hunks)components/contracts/active.tsx(3 hunks)components/contracts/past.tsx(3 hunks)components/menu/index.tsx(1 hunks)components/preferences/index.tsx(1 hunks)components/proposals/Applications.tsx(2 hunks)components/proposals/Invitations.tsx(4 hunks)contexts/chat-context.provider.native.tsx(4 hunks)contexts/chat-context.provider.web.tsx(4 hunks)styles/menu/MenuItem.styles.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/collaboration-card/card-components/ChipComponent.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/contracts/ActionContainer.tsx
[error] 179-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (24)
styles/menu/MenuItem.styles.tsx (1)
37-37: Padding change from vertical to top-only.Confirm whether removing bottom padding is intentional. If consistent with the new design, this is good to go. Otherwise, ensure that reducing vertical padding does not break layout spacing.
contexts/chat-context.provider.web.tsx (2)
14-14: New method signature added to interface.The
removeMemberFromChannelmethod is well-defined. Make sure all usage sites pass the correct arguments.
82-82: Exposing the new method in the context provider.The context now includes
removeMemberFromChannel. Confirm that all consumers are aware of its current placeholder behavior returningfalse.components/brand-profile/members-card/index.tsx (5)
9-11: New imports for menu and state.Integrating
useState,Pressable, andMenubroadens the component's interactivity. Good approach for revealing actions.
15-16: Interface updated withcardTypeandaction.Including these props increases flexibility. Good approach for making the component more reusable and customizable.
21-24: State and handlers for Menu visibility.This simple state management approach is clear and effective.
51-52: Displaying manager's name and email.Ensure that manager data exists at all times. If manager fields are optional, consider safe handling or fallback UI.
55-81: New menu integration adds interactive options.Providing an overflow menu for each member is a straightforward way to organize actions. Good usage of
react-native-paper'sMenu.components/contracts/MemberContainer.tsx (1)
30-30: DestructuringremoveMemberFromChannelfrom context.This indicates an upcoming removal feature for channel members. Verify the function's implementation before production release.
components/menu/index.tsx (1)
44-44: Consider a fallback mechanism for null or undefined images.
UsingimageUrl(selectedBrand?.image)is good for consistent URL handling, but ensureimageUrlappropriately handlesnullor undefined values fromselectedBrand?.imageto avoid runtime errors or broken images.You can run a quick check to ensure no invalid image references:
contexts/chat-context.provider.native.tsx (2)
28-31: Interface extension is clear.
The new method signature correctly matches the intended functionality. Please consider updating relevant documentation or code comments to reflect the new method if you maintain external references or typed definitions.
41-41: Validate the default return value.
Defaulting tofalsehere is fine, but it might mask situations where a no-op or a fallback is necessary if the channel removal logic is invoked before the client is fully initialized.components/contracts/ContractDetailContent.tsx (1)
96-110: 🛠️ Refactor suggestionValidate or sanitize HTML to prevent potential XSS.
RenderHTMLcan render untrusted content ifprops.userData.profile?.content?.aboutoriginates from external inputs. Confirm sanitation or consider using a safe HTML parser to avoid malicious script injection.components/contracts/past.tsx (1)
17-17: New import for Pressable is appropriate.
Replacing theCardcomponent with aPressablefor improved user interaction is a sound choice. No issues here.components/contracts/active.tsx (3)
17-17: Use of Pressable import is appropriate.
Good job importingPressableto replace or supplement clickable UI elements.
174-179: Added custom bordering and corner rounding.
The styling withborderWidth,borderColor,borderRadius, andoverflowis excellent for giving the item a card-like look.
197-197: Closing Pressable tag.
No specific issues here. This is consistent with React Native’s best practices.components/proposals/Applications.tsx (1)
160-169: Replacing Card with View for proposals listing.
The custom-styled<View>effectively provides the same layout as<Card>while giving you more control. The border and radius styling is correct for a consistent user experience.Also applies to: 217-217
components/proposals/Invitations.tsx (4)
3-3: Importing Themed components.
Looks good. The usage ofTextandViewfrom@/components/theme/Themedis consistent with your theming approach.
23-23: router import from expo-router.
Pulling inrouterfromexpo-routeris a clean way to navigate on press actions.
139-146: Conditional empty state for invitations.
This logic properly displays an empty state image and text when there are no invitations.
147-235: FlatList rendering for inactive proposals.
Your replaced UI structure, using<View>with custom borders, plus theCarouselandPressable, is effective. The code is well-organized, and the new styling approach is consistent with the rest of the app.components/contracts/ActionContainer.tsx (1)
115-192: Conditionally rendering action buttons by contract status.
This logic is clear and meets the requirement to hide actions whencontract.statusis3. Nicely done.🧰 Tools
🪛 Biome (1.9.4)
[error] 179-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
components/preferences/index.tsx (1)
78-84: Verify the empty action prop implementationThe
actionprop is currently set to an empty arrow function. This seems like a placeholder and might indicate missing functionality. Please clarify:
- What actions should be available in the preferences view?
- Should this be implemented now or in a future PR?
Let's check if other components implement meaningful actions for this prop:
✅ Verification successful
Empty action implementation appears intentional
The empty action is consistent with the "preferences" cardType, which seems to be a read-only view. This differs from the "contract" cardType where actions like member removal are needed. No changes required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other MembersCard usage with action prop ast-grep --pattern 'MembersCard $props' | grep -A 5 'action'Length of output: 60
Script:
#!/bin/bash # Find all occurrences of MembersCard rg "MembersCard" -A 5 -B 5 # Find the component definition fd "MembersCard" --type fLength of output: 5009
| const removeMemberFromChannel = async (channel: any, member: string) => { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Implementation is incomplete.
The function currently always returns false. Provide the removal logic or confirm that the function is intentionally left unimplemented. If it’s a placeholder, consider throwing an error to avoid silent failures.
const removeMemberFromChannel = async (channel: any, member: string) => {
- return false;
+ throw new Error("removeMemberFromChannel not implemented yet.");
};📝 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 removeMemberFromChannel = async (channel: any, member: string) => { | |
| return false; | |
| }; | |
| const removeMemberFromChannel = async (channel: any, member: string) => { | |
| throw new Error("removeMemberFromChannel not implemented yet."); | |
| }; |
| const removeMemberFromChannel = async (channel: string, member: string) => { | ||
| try { | ||
| const channelToWatch = streamClient.channel("messaging", channel); | ||
| await channelToWatch | ||
| .removeMembers([member]) | ||
| .then(() => { | ||
| return true; | ||
| }) | ||
| .catch((error) => { | ||
| console.error(error); | ||
| return false; | ||
| }); | ||
|
|
||
| return false; | ||
| } catch (error) { | ||
| console.error(error); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Logical bug: function always returns false.
Because .then() returns a boolean within its callback but is never propagated outside, the function ultimately always returns false. To fix:
const removeMemberFromChannel = async (channel: string, member: string) => {
try {
const channelToWatch = streamClient.channel("messaging", channel);
- await channelToWatch
- .removeMembers([member])
- .then(() => {
- return true;
- })
- .catch((error) => {
- console.error(error);
- return false;
- });
- return false;
+ await channelToWatch.removeMembers([member]);
+ return true;
} catch (error) {
console.error(error);
return 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.
| const removeMemberFromChannel = async (channel: string, member: string) => { | |
| try { | |
| const channelToWatch = streamClient.channel("messaging", channel); | |
| await channelToWatch | |
| .removeMembers([member]) | |
| .then(() => { | |
| return true; | |
| }) | |
| .catch((error) => { | |
| console.error(error); | |
| return false; | |
| }); | |
| return false; | |
| } catch (error) { | |
| console.error(error); | |
| return false; | |
| } | |
| }; | |
| const removeMemberFromChannel = async (channel: string, member: string) => { | |
| try { | |
| const channelToWatch = streamClient.channel("messaging", channel); | |
| await channelToWatch.removeMembers([member]); | |
| return true; | |
| } catch (error) { | |
| console.error(error); | |
| return false; | |
| } | |
| }; |
| const downloadAndSaveFile = async (url: string, filename: string) => { | ||
| try { | ||
| const { uri } = await FileSystem.downloadAsync( | ||
| url, | ||
| FileSystem.documentDirectory + filename | ||
| ); | ||
| if (Platform.OS === "web") { | ||
| Sharing.shareAsync(uri); | ||
| } else { | ||
| await Sharing.shareAsync(uri); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error downloading file:", error); | ||
| setConfirmationModalVisible(false); | ||
| alert("Failed to download file. Please try again."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file validation and download progress feedback
The file download implementation needs several improvements for security and user experience:
- Security:
- Add file size limits
- Validate file types
- Sanitize filenames
- UX:
- Add loading state during download
- Show download progress
- Provide success feedback
Here's a suggested implementation:
- const downloadAndSaveFile = async (url: string, filename: string) => {
+ const downloadAndSaveFile = async (url: string, filename: string) => {
+ const MAX_FILE_SIZE = 50 * 1024 * 1024; // 50MB limit
+ const ALLOWED_TYPES = ['pdf', 'doc', 'docx', 'txt']; // Add your allowed types
+
+ const sanitizedFilename = filename.replace(/[^a-zA-Z0-9.-]/g, '_');
+ const fileExtension = sanitizedFilename.split('.').pop()?.toLowerCase();
+
+ if (!fileExtension || !ALLOWED_TYPES.includes(fileExtension)) {
+ alert('Invalid file type');
+ return;
+ }
+
try {
+ setIsDownloading(true); // Add this state
const { uri } = await FileSystem.downloadAsync(
url,
- FileSystem.documentDirectory + filename
+ FileSystem.documentDirectory + sanitizedFilename,
+ {
+ md5: true,
+ headers: { 'Content-Type': 'application/octet-stream' },
+ sessionType: FileSystem.FileSystemSessionType.BACKGROUND,
+ callback: (downloadProgress) => {
+ const progress = downloadProgress.totalBytesWritten / downloadProgress.totalBytesExpectedToWrite;
+ setDownloadProgress(progress); // Add this state
+ },
+ }
);
+
+ const fileInfo = await FileSystem.getInfoAsync(uri);
+ if (fileInfo.size > MAX_FILE_SIZE) {
+ await FileSystem.deleteAsync(uri);
+ throw new Error('File too large');
+ }
+
if (Platform.OS === "web") {
Sharing.shareAsync(uri);
} else {
await Sharing.shareAsync(uri);
}
+ alert('File downloaded successfully');
} catch (error) {
console.error("Error downloading file:", error);
setConfirmationModalVisible(false);
- alert("Failed to download file. Please try again.");
+ alert(error.message || "Failed to download file. Please try again.");
} finally {
+ setIsDownloading(false);
+ setDownloadProgress(0);
}
};Don't forget to add the necessary state variables:
const [isDownloading, setIsDownloading] = useState(false);
const [downloadProgress, setDownloadProgress] = useState(0);
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
contexts/auth-context.provider.tsx (2)
162-162: Ensure the assigned role is correct for all users.Currently,
roleis hardcoded to"manager". If there's a possibility that other roles exist, consider parameterizing or validating this before assignment. This approach will reduce the risk of inadvertently granting privileges and make the code more adaptable.
174-176: Centralize routes or use config for route paths.You're repeating string literals such as
"/(onboarding)/onboarding-your-brand?firstBrand=true"inline. Extracting them into a configuration or constants file can reduce potential typos and make future updates less error-prone.components/ui/modal/MembersModal.tsx (2)
36-37: Consider fallback values or transformations for user input
When collecting user data, consider whether an empty or default value for "name" might be required. You might also apply trimming or default placeholders if "name" is optional downstream.
42-44: Improve user input validation
Validatingnamepresence is good, but additional checks (e.g., email format validation or name sanitization) could minimize user errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/preferences/index.tsx(5 hunks)components/ui/modal/MembersModal.tsx(3 hunks)contexts/auth-context.provider.tsx(3 hunks)styles/Members.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/preferences/index.tsx
🔇 Additional comments (4)
components/ui/modal/MembersModal.tsx (2)
103-159: Confirm cross-platform layout handling
UsingKeyboardAvoidingViewwith different behaviors between iOS (padding) and Android (height) can cause occasional layout overlap. Validate on multiple devices to ensure consistent user experience.
2-2: Add usage checks for new imports
You have introduced imports forPortalfromreact-native-paperand Firestore utilities. Ensure that these dependencies are truly necessary and clean up any unused imports to avoid bloat.Also applies to: 11-18
✅ Verification successful
Both imports are actively used and necessary
Portalis required for proper modal rendering with react-native-paper- Firestore utilities (
doc, setDoc) are used in the member addition functionality🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the newly imported references are being used rg -A 5 "import {.*Portal" rg -A 5 "firebase/firestore"Length of output: 17743
Script:
#!/bin/bash # Search for Portal import and usage with fixed regex rg "import.*Portal.*react-native-paper" -A 5 rg "<Portal" -A 5 # Check the specific file content cat components/ui/modal/MembersModal.tsxLength of output: 9135
styles/Members.tsx (2)
36-37: Check compatibility of 'gap'
React Native styling support forgapcan be limited or require newer versions. Test across your supported platforms to confirm proper spacing.
60-60: Reevaluate button alignment
Switching from a margin-based approach to usingalignItems: "center"may affect the layout. Confirm this aligns with expected designs on varying screen sizes.
| await fetch("https://be.trendly.pro/api/v1/chat/auth", { | ||
| method: "POST", | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': `Bearer ${userCredential.user.uid}`, | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${userCredential.user.uid}`, |
There was a problem hiding this comment.
Add result verification or error handling.
Similar to the fetch call in your signIn flow, you're not verifying the server's response. Handle potential errors or verify a successful operation before proceeding. This will help make the sign-up process more robust and user-friendly.
| await fetch("https://be.trendly.pro/api/v1/chat/auth", { | ||
| method: "POST", | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': `Bearer ${managerCredential.user.uid}`, | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${managerCredential.user.uid}`, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Confirmed: Fetch calls are duplicated and lack error handling
The codebase contains identical fetch calls to the auth endpoint in both signIn and signUp flows. A helper function like this would improve the code:
const postAuthToken = async (uid: string) => {
try {
const response = await fetch("https://be.trendly.pro/api/v1/chat/auth", {
method: "POST",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${uid}`,
},
});
if (!response.ok) throw new Error(`HTTP error! status: ${response.status}`);
} catch (error) {
console.error('Auth token post failed:', error);
throw error; // Allow caller to handle the error
}
};🔗 Analysis chain
Consider refactoring repeated fetch calls and adding error handling.
You're making a POST request to the same endpoint later in the signUp flow. You could refactor this into a helper function (e.g., postAuthToken) to avoid duplication, making the code more DRY and manageable. Moreover, there's no handling of any response from the server, so it's unclear what happens if it fails or returns an error status.
Below is a script to search all fetch calls in the codebase to gauge how often you might refactor them:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching all fetch calls in the repository for duplication or partial matches
rg -A 3 'fetch\(.*\)'
Length of output: 1183
Script:
#!/bin/bash
# Search for the specific endpoint
rg -l "be.trendly.pro/api/v1/chat/auth"
# Get the content of auth-context file to analyze all fetch calls
cat contexts/auth-context.provider.tsx
# Search for POST requests in auth context
rg "method: \"POST\"" contexts/auth-context.provider.tsx -B 2 -A 2
Length of output: 8542
| .then(async (res) => { | ||
| Toaster.success("User Invited Successfully"); | ||
| if (res.status === 200) { | ||
| try { | ||
| const userID = res.data.user.rawId; | ||
| const memberRef = doc(FirestoreDB, "managers", userID); | ||
| let userData = { | ||
| name: name, | ||
| email: email, | ||
| pushNotificationToken: { | ||
| ios: [], | ||
| android: [], | ||
| web: [], | ||
| }, | ||
| settings: { | ||
| theme: "light", | ||
| emailNotification: true, | ||
| pushNotification: true, | ||
| }, | ||
| }; | ||
| await setDoc(memberRef, userData); | ||
| } catch (e) { | ||
| console.error(e); | ||
| Toaster.error("Something wrong happened"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle partial success more explicitly
If Firestore write fails after a successful brand member invitation, consider a mitigation strategy or better logging so users and devs understand the final membership status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
components/contracts/ActionContainer.tsx (2)
180-191: Remove unnecessary Fragment wrapper.The Fragment wrapper is redundant since it contains only one child Button component.
- <> <Button mode="contained" style={{ flex: 1, }} onPress={feedbackModalVisible} > Give Feedback </Button> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 180-190: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
126-176: Consider extracting repeated button styles.Multiple buttons share the same style properties. Consider extracting the common styles into a constant to improve maintainability.
const commonButtonStyle = { flex: 1, } as const; // Then use it in buttons like: <Button mode="contained" style={commonButtonStyle} onPress={...} >components/contracts/active.tsx (5)
17-17: Remove unused Card import.Since Card has been replaced with Pressable, the import from 'react-native-paper' is no longer needed and should be removed to reduce bundle size.
-import { Card } from "react-native-paper";
136-136: Consider using enums or constants for status values.Using magic numbers (0 and 3) for status makes the code less maintainable and harder to understand. Consider defining an enum or constants for proposal status values.
// Add at the top of the file enum ProposalStatus { DRAFT = 0, ACTIVE = 1, IN_PROGRESS = 2, COMPLETED = 3, // Add other statuses as needed } // Then use in filter return proposal.status !== ProposalStatus.COMPLETED && proposal.status !== ProposalStatus.DRAFT;
175-180: Move inline styles to stylesFn.Consider moving the Pressable styles to the stylesFn to maintain consistency and improve reusability.
-style={{ - borderWidth: 0.3, - borderColor: Colors(theme).gray300, - borderRadius: 5, - overflow: "hidden", -}} +style={styles.pressableCard} // Add to stylesFn pressableCard: { borderWidth: 0.3, borderColor: Colors(theme).gray300, borderRadius: 5, overflow: "hidden", }
Line range hint
189-195: Extract default application object.The fallback object for missing application data should be extracted to a constant to improve maintainability and reusability.
const DEFAULT_APPLICATION: Partial<IApplications> = { message: "No message", quotation: "No quotation", timeline: new Date().toISOString(), }; // Then use in component application={item.applications[0] || DEFAULT_APPLICATION}
171-173: Type the navigation route.Consider using typed navigation to ensure type safety when navigating to contract details.
// Define in your navigation types type AppRoutes = { "contract-details": { streamChannelId: string; }; // other routes... }; // Then use in component router.push({ pathname: "/contract-details" as keyof AppRoutes, params: { streamChannelId: item.streamChannelId } });components/contracts/AddMemberModal.tsx (3)
Line range hint
78-83: Improve filter logic readability and type safety.The filter logic could be more readable and type-safe. Consider refactoring to:
- members = members.filter((member) => { - if (member.status === 0) return false; - return !membersAlreadyInContract.some((memberInContract) => { - if (!memberInContract) return false; - return memberInContract.email === member.email; - }); - }); + members = members.filter((member) => + member.status !== 0 && + !membersAlreadyInContract?.some( + (memberInContract) => memberInContract?.email === member.email + ) + );Also, consider adding proper TypeScript interfaces for the member objects to improve type safety:
interface Member { status: number; email: string; managerId: string; // ... other properties }
176-189: Enhance empty state UX and code maintainability.Consider the following improvements:
- Move inline styles to StyleSheet:
- {members.length !== 0 ? ( - <FlatList data={members} renderItem={renderItem} /> - ) : ( - <Text - style={{ - color: Colors(theme).gray300, - textAlign: "center", - fontSize: 16, - paddingBottom: 16, - }} - > - No members to add - </Text> - )} + {members.length !== 0 ? ( + <FlatList + data={members} + renderItem={renderItem} + contentContainerStyle={styles.listContainer} + /> + ) : ( + <Text style={styles.emptyStateText}> + No members available to add to this contract + </Text> + )}Add to StyleSheet:
const styles = StyleSheet.create({ // ... existing styles listContainer: { flexGrow: 1, }, emptyStateText: { color: Colors(theme).gray300, textAlign: "center", fontSize: 16, paddingBottom: 16, }, });
- Consider adding a more descriptive empty state message and possibly an icon to improve the user experience.
Line range hint
1-189: Consider architectural improvements for better maintainability and user experience.
- Move Firebase operations to a dedicated service layer:
// services/memberService.ts export const MemberService = { fetchBrandMembers: async (brandId: string) => { // ... Firebase operations }, addMemberToChannel: async (channelId: string, memberId: string) => { // ... Firebase operations } };
- Add proper loading states during async operations:
const [isLoading, setIsLoading] = useState(false); // ... in fetchMembersInBrand setIsLoading(true); try { // ... fetch operations } finally { setIsLoading(false); }
- Improve error handling with specific error messages:
catch (error) { if (error.code === 'permission-denied') { Toaster.error('You don\'t have permission to add members'); } else { Toaster.error('Failed to add member: ' + error.message); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/contracts/ActionContainer.tsx(2 hunks)components/contracts/AddMemberModal.tsx(2 hunks)components/contracts/active.tsx(5 hunks)components/contracts/past.tsx(5 hunks)components/menu/index.tsx(1 hunks)styles/menu/MenuItem.styles.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/menu/index.tsx
- components/contracts/past.tsx
- styles/menu/MenuItem.styles.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/contracts/ActionContainer.tsx
[error] 180-190: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (2)
components/contracts/ActionContainer.tsx (1)
89-89: LGTM! Good UX improvement.Showing the feedback modal right after contract completion is a good UX decision, as it encourages timely feedback collection.
components/contracts/active.tsx (1)
79-83: LGTM! Query construction is correct.The Firebase query is properly constructed to filter contracts by brandId.
Summary by CodeRabbit
New Features
Style
Chores