Bugfix: Add Visit Button in Joined Organizations Filter #3278
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
Possibly related issues
Suggested labels
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3278 +/- ##
=====================================================
+ Coverage 8.56% 89.86% +81.30%
=====================================================
Files 309 331 +22
Lines 7954 8574 +620
Branches 1763 1898 +135
=====================================================
+ Hits 681 7705 +7024
+ Misses 7196 610 -6586
- Partials 77 259 +182
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/components/OrganizationCard/OrganizationCard.tsx (1)
189-189: Simplify null checks using optional chainingThe conditional rendering of the address can be simplified by using optional chaining. This enhances readability and reduces complexity.
Apply this diff to simplify the null checks:
- {address && address.city && ( + {address?.city && (🧰 Tools
🪛 Biome (1.9.4)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/OrganizationCard/OrganizationCard.spec.tsx (2)
57-70: Remove unused propsfirstNameandlastNamefrom test dataThe
firstNameandlastNameprops are no longer used in theOrganizationCardcomponent as per the updatedInterfaceOrganizationCardProps. Removing these props from the test data keeps the tests clean and avoids confusion.Apply this diff to remove the unused props:
const props = { id: '123', image: 'https://via.placeholder.com/80', - firstName: 'John', - lastName: 'Doe', name: 'Sample', description: '', admins: [], members: [], address: { city: '', countryCode: '', line1: '', postalCode: '', state: '', }, membershipRequestStatus: '', userRegistrationRequired: false, membershipRequests: [], };
91-103: Remove unused propsfirstNameandlastNamefrom test dataThe
firstNameandlastNameprops are no longer used in theOrganizationCardcomponent as per the updatedInterfaceOrganizationCardProps. Removing these props from the test data keeps the tests clean and avoids confusion.Apply this diff to remove the unused props:
const props = { id: '123', image: '', - firstName: 'John', - lastName: 'Doe', name: 'Sample', description: '', admins: [], members: [], address: { city: '', countryCode: '', line1: '', postalCode: '', state: '', }, membershipRequestStatus: '', userRegistrationRequired: false, membershipRequests: [], };src/screens/UserPortal/Organizations/Organizations.tsx (1)
Line range hint
393-420: Refactor repeated mapping logic into a helper functionThe mapping logic for organizations is repeated in multiple places with slight variations. Consider refactoring this logic into a helper function to improve code reusability and reduce duplication.
Example refactor:
function mapOrganizations(orgs, membershipStatus, isJoined) { return orgs.map((org) => ({ ...org, membershipRequestStatus: membershipStatus, isJoined, })); } // Usage: const organizations = mapOrganizations(data.organizationsConnection, membershipRequestStatus, false);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/OrganizationCard/OrganizationCard.spec.tsx(3 hunks)src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)src/screens/UserPortal/Organizations/Organizations.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
src/screens/UserPortal/Organizations/Organizations.tsx (1)
250-254: Ensure consistentisJoinedproperty usageYou're correctly adding the
isJoinedproperty to the organization objects in different modes. However, ensure that all instances where organizations are mapped include theisJoinedproperty for consistency. This helps in correctly determining the joined status across the application.Also applies to: 262-268, 277-283
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/OrganizationCard/OrganizationCard.tsx (1)
22-22:⚠️ Potential issueRemove hook call outside component.
The
useLocalStoragehook is called outside the component, which violates React's rules of hooks.Remove this line as the hook is already properly used inside the component at line 66.
-useLocalStorage();🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Missing TSDoc comment. Component requires TSDoc documentation.
🧹 Nitpick comments (1)
src/components/OrganizationCard/OrganizationCard.tsx (1)
169-177: Simplify address conditional using optional chaining.The address conditional can be simplified using optional chaining.
- {address && address.city && ( + {address?.city && ( <div className={styles.address}> <h6 className="text-secondary"> <span className="address-line">{address.line1}, </span> <span className="address-line">{address.city}, </span> <span className="address-line">{address.countryCode}</span> </h6> </div> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 169-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: PR Workflow
[error] Missing TSDoc comment. Component requires TSDoc documentation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/OrganizationCard/OrganizationCard.spec.tsx(3 hunks)src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 169-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: PR Workflow
src/components/OrganizationCard/OrganizationCard.tsx
[error] Missing TSDoc comment. Component requires TSDoc documentation.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
src/components/OrganizationCard/OrganizationCard.tsx (2)
50-50: 🛠️ Refactor suggestionFollow camelCase naming convention for props.
The prop name
isjoinedshould follow camelCase naming convention.- isjoined: boolean; + isJoined: boolean;Also update the prop usage in the component parameters and JSX conditions.
Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Missing TSDoc comment. Component requires TSDoc documentation.
184-195: Verify the Visit button visibility logic.The PR objective is to fix the Visit button visibility. The current implementation uses
isjoinedwhich aligns with the requirement to show the button for all joined organizations.Let's verify the implementation:
✅ Verification successful
Visit button visibility logic is correctly implemented
The visibility is properly controlled by the
isjoinedprop, which is set totruefor accepted memberships andfalseotherwise. The implementation aligns with the requirement to show the Visit button only for joined organizations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the Visit button visibility is controlled by isJoined prop ast-grep --pattern 'isjoined && ( $$$ <Button $$$ {t("visit")} </Button> $$$ )'Length of output: 97
Script:
#!/bin/bash # Search for Button components with visit text and isjoined condition ast-grep --pattern 'isjoined && <Button' # Search for isjoined prop usage and type definition rg "isjoined|isJoined" -A 2 -B 2 # Look for the component's interface ast-grep --pattern 'interface $_ { $$$ isjoined?: $_ $$$ }'Length of output: 3291
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Missing TSDoc comment. Component requires TSDoc documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/OrganizationCard/OrganizationCard.tsx (1)
22-22:⚠️ Potential issueMove hook call inside the component.
The
useLocalStorage()hook is called outside the component, which violates React's rules of hooks. Hooks must only be called inside React function components.Remove this line as the hook is already properly used inside the component at line 90.
- useLocalStorage();
🧹 Nitpick comments (2)
src/components/OrganizationCard/OrganizationCard.tsx (2)
72-74: Remove duplicate TSDoc entries.The documentation contains duplicate
@param propsand@returnsentries.- * @param props - Properties for the organization card. - * @returns JSX element representing the organization card. - * @returns The organization card component. + * @returns The organization card component.
193-201: Use optional chaining for address checks.The address property checks can be simplified using optional chaining.
- {address && address.city && ( + {address?.city && ( <div className={styles.address}> <h6 className="text-secondary"> <span className="address-line">{address.line1}, </span> <span className="address-line">{address.city}, </span> <span className="address-line">{address.countryCode}</span> </h6> </div> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
src/components/OrganizationCard/OrganizationCard.tsx (1)
50-50: 🛠️ Refactor suggestionFix property name casing.
The property
isjoinedshould follow JavaScript camelCase naming convention.- isjoined: boolean; + isJoined: boolean;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/OrganizationCard/OrganizationCard.tsx (2)
193-201: Use optional chaining for address properties.The current condition can be simplified using optional chaining to make it more concise and maintainable.
- {address && address.city && ( + {address?.city && ( <div className={styles.address}> <h6 className="text-secondary"> <span className="address-line">{address.line1}, </span> <span className="address-line">{address.city}, </span> <span className="address-line">{address.countryCode}</span> </h6> </div> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
149-170: Enhance error handling inwithdrawMembershipRequest.The error handling in this function could be improved to provide more specific error messages and type safety.
async function withdrawMembershipRequest(): Promise<void> { const membershipRequest = membershipRequests.find( (request) => request.user._id === userId, ); try { if (!membershipRequest) { toast.error(t('MembershipRequestNotFound') as string); return; } await cancelMembershipRequest({ variables: { membershipRequestId: membershipRequest._id, }, }); toast.success(t('MembershipRequestWithdrawn') as string); } catch (error: unknown) { - console.error('Failed to withdraw membership request:', error); - toast.error(t('errorOccured') as string); + if (error instanceof ApolloError) { + const errorCode = error.graphQLErrors?.[0]?.extensions?.code; + if (errorCode) { + toast.error(t(errorCode) as string); + } else { + toast.error(t('errorOccured') as string); + } + } + console.error('Failed to withdraw membership request:', error); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/OrganizationCard/OrganizationCard.spec.tsx(3 hunks)src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
src/components/OrganizationCard/OrganizationCard.spec.tsx (2)
35-48: LGTM! Well-structured test wrapper.The
TestWrappercomponent correctly provides all required contexts (MockedProvider,I18nextProvider,BrowserRouter) for testing theOrganizationCardcomponent.
73-77: Add test coverage for joined organizations.The test suite only tests the scenario where
isJoined={false}. To ensure the "Visit" button functionality works correctly, add test cases forisJoined={true}.Also applies to: 106-110
src/components/OrganizationCard/OrganizationCard.tsx (1)
208-219: LGTM! Visit button implementation aligns with PR objectives.The Visit button is now correctly rendered based on the
isJoinedprop, which aligns with the PR objectives to display the Visit button for all joined organizations.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Please make coderabbit approve your PR. See the error message |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/OrganizationCard/OrganizationCard.tsx (3)
52-72: Clean up redundant TSDoc documentation.Remove the duplicate
@param propsand@returnsentries at lines 71-72 as they're already documented above.* @param membershipRequests - The list of membership requests with user IDs. * -* @param props - Properties for the organization card. -* @returns JSX element representing the organization card. * @returns The organization card component. */
116-145: Enhance error handling in joinOrganization function.The error handling could be more specific about what went wrong. Consider adding more error codes and corresponding user-friendly messages.
if (errorCode === 'ALREADY_MEMBER') { toast.error(t('AlreadyJoined') as string); + } else if (errorCode === 'ORGANIZATION_NOT_FOUND') { + toast.error(t('OrganizationNotFound') as string); + } else if (errorCode === 'USER_NOT_AUTHORIZED') { + toast.error(t('NotAuthorized') as string); } else { toast.error(t('errorOccured') as string); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 117-117: src/components/OrganizationCard/OrganizationCard.tsx#L117
Added line #L117 was not covered by tests
[warning] 119-119: src/components/OrganizationCard/OrganizationCard.tsx#L119
Added line #L119 was not covered by tests
[warning] 124-124: src/components/OrganizationCard/OrganizationCard.tsx#L124
Added line #L124 was not covered by tests
[warning] 126-126: src/components/OrganizationCard/OrganizationCard.tsx#L126
Added line #L126 was not covered by tests
[warning] 131-131: src/components/OrganizationCard/OrganizationCard.tsx#L131
Added line #L131 was not covered by tests
[warning] 133-133: src/components/OrganizationCard/OrganizationCard.tsx#L133
Added line #L133 was not covered by tests
[warning] 136-137: src/components/OrganizationCard/OrganizationCard.tsx#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 139-139: src/components/OrganizationCard/OrganizationCard.tsx#L139
Added line #L139 was not covered by tests
[warning] 141-141: src/components/OrganizationCard/OrganizationCard.tsx#L141
Added line #L141 was not covered by tests
191-199: Use optional chaining for safer property access.As suggested by static analysis, use optional chaining to safely access nested address properties.
- {address && address.city && ( + {address?.city && ( <div className={styles.address}> <h6 className="text-secondary"> - <span className="address-line">{address.line1}, </span> - <span className="address-line">{address.city}, </span> - <span className="address-line">{address.countryCode}</span> + <span className="address-line">{address?.line1}, </span> + <span className="address-line">{address?.city}, </span> + <span className="address-line">{address?.countryCode}</span> </h6> </div> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: codecov/patch
src/components/OrganizationCard/OrganizationCard.tsx
[warning] 88-89: src/components/OrganizationCard/OrganizationCard.tsx#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 91-92: src/components/OrganizationCard/OrganizationCard.tsx#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 94-94: src/components/OrganizationCard/OrganizationCard.tsx#L94
Added line #L94 was not covered by tests
[warning] 97-97: src/components/OrganizationCard/OrganizationCard.tsx#L97
Added line #L97 was not covered by tests
[warning] 102-102: src/components/OrganizationCard/OrganizationCard.tsx#L102
Added line #L102 was not covered by tests
[warning] 107-107: src/components/OrganizationCard/OrganizationCard.tsx#L107
Added line #L107 was not covered by tests
[warning] 112-112: src/components/OrganizationCard/OrganizationCard.tsx#L112
Added line #L112 was not covered by tests
[warning] 117-117: src/components/OrganizationCard/OrganizationCard.tsx#L117
Added line #L117 was not covered by tests
[warning] 119-119: src/components/OrganizationCard/OrganizationCard.tsx#L119
Added line #L119 was not covered by tests
[warning] 124-124: src/components/OrganizationCard/OrganizationCard.tsx#L124
Added line #L124 was not covered by tests
[warning] 126-126: src/components/OrganizationCard/OrganizationCard.tsx#L126
Added line #L126 was not covered by tests
[warning] 131-131: src/components/OrganizationCard/OrganizationCard.tsx#L131
Added line #L131 was not covered by tests
[warning] 133-133: src/components/OrganizationCard/OrganizationCard.tsx#L133
Added line #L133 was not covered by tests
[warning] 136-137: src/components/OrganizationCard/OrganizationCard.tsx#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 139-139: src/components/OrganizationCard/OrganizationCard.tsx#L139
Added line #L139 was not covered by tests
[warning] 141-141: src/components/OrganizationCard/OrganizationCard.tsx#L141
Added line #L141 was not covered by tests
[warning] 148-149: src/components/OrganizationCard/OrganizationCard.tsx#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 151-151: src/components/OrganizationCard/OrganizationCard.tsx#L151
Added line #L151 was not covered by tests
[warning] 153-154: src/components/OrganizationCard/OrganizationCard.tsx#L153-L154
Added lines #L153 - L154 were not covered by tests
[warning] 157-157: src/components/OrganizationCard/OrganizationCard.tsx#L157
Added line #L157 was not covered by tests
[warning] 163-163: src/components/OrganizationCard/OrganizationCard.tsx#L163
Added line #L163 was not covered by tests
[warning] 165-166: src/components/OrganizationCard/OrganizationCard.tsx#L165-L166
Added lines #L165 - L166 were not covered by tests
[warning] 212-212: src/components/OrganizationCard/OrganizationCard.tsx#L212
Added line #L212 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (2)
src/components/OrganizationCard/OrganizationCard.tsx (2)
3-20: Well-structured imports and comprehensive interface definition!The imports are well-organized and the interface is properly typed with all necessary properties for the organization card functionality.
Also applies to: 24-48
206-217: Visit button implementation aligns with PR objectives.The Visit button's visibility is now correctly controlled by the
isJoinedprop, which aligns with the PR's goal of fixing the visibility issue in the Joined Organizations filter.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 212-212: src/components/OrganizationCard/OrganizationCard.tsx#L212
Added line #L212 was not covered by tests
I am resolving every error, but after each commit, new suggestions or potential issues keep appearing |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/OrganizationCard/OrganizationCard.tsx (4)
52-72: Clean up duplicate TSDoc entries.The documentation contains duplicate entries that should be removed:
- Duplicate
@param propsdescription- Duplicate
@returnsdescriptionApply this diff to clean up the documentation:
/** * Component to display an organization's card with its image and owner details. * Displays an organization card with options to join or manage membership. * * Shows the organization's name, image, description, address, number of admins and members, * and provides buttons for joining, withdrawing membership requests, or visiting the organization page. * * @param props - The properties for the organization card. * @param id - The unique identifier of the organization. * @param name - The name of the organization. * @param image - The URL of the organization's image. * @param description - A description of the organization. * @param admins - The list of admins with their IDs. * @param members - The list of members with their IDs. * @param address - The address of the organization including city, country code, line1, postal code, and state. * @param membershipRequestStatus - The status of the membership request (accepted, pending, or empty). * @param userRegistrationRequired - Indicates if user registration is required to join the organization. * @param membershipRequests - The list of membership requests with user IDs. - * - * @param props - Properties for the organization card. - * @returns JSX element representing the organization card. * @returns The organization card component. */
193-201: Use optional chaining for safer property access.The address property access could be safer using optional chaining to prevent potential runtime errors.
Apply this diff:
- {address && address.city && ( + {address?.city && ( <div className={styles.address}> <h6 className="text-secondary"> - <span className="address-line">{address.line1}, </span> - <span className="address-line">{address.city}, </span> - <span className="address-line">{address.countryCode}</span> + <span className="address-line">{address?.line1}, </span> + <span className="address-line">{address?.city}, </span> + <span className="address-line">{address?.countryCode}</span> </h6> </div> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
221-241: Consider using enum or constants for membership status.The membership status checks use string literals which could lead to maintenance issues. Consider defining an enum or constants for the status values.
Add this at the top of the file:
enum MembershipStatus { PENDING = 'pending', ACCEPTED = 'accepted', NONE = '' }Then update the conditions:
- {membershipRequestStatus === 'pending' && ( + {membershipRequestStatus === MembershipStatus.PENDING && ( // ... Button component )} - {membershipRequestStatus === '' && ( + {membershipRequestStatus === MembershipStatus.NONE && ( // ... Button component )}
147-170: Improve error handling in withdrawMembershipRequest.The error handling in the withdrawMembershipRequest function could be more specific about the type of error that occurred.
Apply this diff:
async function withdrawMembershipRequest(): Promise<void> { const membershipRequest = membershipRequests.find( (request) => request.user._id === userId, ); try { if (!membershipRequest) { toast.error(t('MembershipRequestNotFound') as string); return; } await cancelMembershipRequest({ variables: { membershipRequestId: membershipRequest._id, }, }); toast.success(t('MembershipRequestWithdrawn') as string); } catch (error: unknown) { - if (process.env.NODE_ENV === 'development') { - console.error('Failed to withdraw membership request:', error); + if (error instanceof ApolloError) { + const errorCode = error.graphQLErrors?.[0]?.extensions?.code; + if (errorCode) { + toast.error(t(`errors.${errorCode}`) as string); + } else { + toast.error(t('errorOccured') as string); + } + if (process.env.NODE_ENV === 'development') { + console.error('Failed to withdraw membership request:', error); + } } - toast.error(t('errorOccured') as string); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
|
Please make code rabbit approve your PR |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/components/OrganizationCard/OrganizationCard.tsx (3)
208-216: Use optional chaining for nested object access.The condition
address && address.citycan be simplified using optional chaining to make the code more concise and maintainable.- {address && address.city && ( + {address?.city && (🧰 Tools
🪛 Biome (1.9.4)
[error] 208-208: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
147-156: Simplify error handling logic.The type assertion
error as ApolloErroris redundant since you're already checking if the error is an instance of Error. Consider simplifying the error handling.- if (error instanceof Error) { - const apolloError = error as ApolloError; - const errorCode = apolloError.graphQLErrors?.[0]?.extensions?.code; + if (error instanceof ApolloError) { + const errorCode = error.graphQLErrors?.[0]?.extensions?.code;
116-126: Improve error handling in useEffect.The error handling in the useEffect hook could be more specific. Instead of logging to console in production, consider adding error boundaries or more specific error handling.
useEffect(() => { try { - // Use the custom hook to retrieve the userId - const id = getItem('userId'); // Adjust this line based on your actual localStorage key + const id = getItem('userId'); setUserId(id); } catch (error) { - console.error('Failed to access localStorage:', error); setUserId(null); // Handle gracefully if localStorage is not available - toast.error('Failed to access user data'); + toast.error(t('failedToAccessUserData') as string); } }, [getItem]);src/components/OrganizationCard/OrganizationCard.spec.tsx (1)
198-219: Add edge case tests for address rendering.The address rendering test should include edge cases where some address fields are missing.
it('should handle partial address information', () => { const props = { ...defaultProps, address: { city: 'Test City', countryCode: '', // Missing country code line1: '', // Missing line1 postalCode: '', // Missing postal code state: '', // Missing state }, }; render( <TestWrapper mocks={successMocks}> <OrganizationCard isJoined={false} {...props} /> </TestWrapper> ); expect(screen.getByText(/Test City/)).toBeInTheDocument(); expect(screen.queryByText(/,\s*,/)).not.toBeInTheDocument(); // Should not show consecutive commas });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/OrganizationCard/OrganizationCard.spec.tsx(1 hunks)src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)src/components/test-utils/TestWrapper.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
src/components/test-utils/TestWrapper.tsx
[error] No TSDoc comment found in file. Documentation is required.
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 208-208: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
src/components/OrganizationCard/OrganizationCard.tsx (1)
223-234: Verify button visibility logic aligns with PR objectives.The PR's objective is to fix the "Visit" button visibility in joined organizations. The current implementation correctly uses the
isJoinedprop to determine visibility, which aligns with the PR objectives.Let's verify the implementation with this script:
✅ Verification successful
Different implementations are intentionally context-specific
The different button visibility logic between the main OrganizationCard (
isJoined) and UserPortal OrganizationCard (membershipRequestStatus === 'accepted') is intentional, as they serve different contexts. The main component is for general organization listing, while the UserPortal version is specifically for the user's organization dashboard. The PR's changes align with its objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Visit button visibility logic has been updated correctly # across the codebase # Check for any remaining instances of the old condition rg "membershipRequestStatus === 'accepted'" --type tsLength of output: 184
Script:
#!/bin/bash # Check both OrganizationCard implementations echo "=== Main OrganizationCard ===" cat src/components/OrganizationCard/OrganizationCard.tsx echo -e "\n=== UserPortal OrganizationCard ===" cat src/components/UserPortal/OrganizationCard/OrganizationCard.tsx echo -e "\n=== Check for imports of both components ===" rg -l "import.*OrganizationCard" --type tsLength of output: 16459
Script:
#!/bin/bash # Check the Organizations component to understand usage echo "=== Organizations.tsx content ===" cat src/screens/UserPortal/Organizations/Organizations.tsx # Check for any other files that might use these components echo -e "\n=== Files containing OrganizationCard usage ===" rg "OrganizationCard" --type ts -C 2Length of output: 32688
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/OrganizationCard/OrganizationCard.tsx (1)
147-156: Improve type safety in error handling.The type casting of
error as ApolloErrorcould be unsafe. Consider checking if the error is an instance of ApolloError first.- if (error instanceof Error) { - const apolloError = error as ApolloError; + if (error instanceof ApolloError) { + const apolloError = error;src/components/OrganizationCard/OrganizationCard.spec.tsx (1)
174-220: Add test coverage for Visit button rendering.While the rendering tests are good, consider adding a specific test for the Visit button when
isJoinedis true.it('should render Visit button when organization is joined', () => { render( <TestWrapper mocks={successMocks}> <OrganizationCard isJoined={true} {...defaultProps} /> </TestWrapper> ); expect(screen.getByText('visit')).toBeInTheDocument(); expect(screen.getByTestId('manageBtn')).toBeInTheDocument(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/OrganizationCard/OrganizationCard.spec.tsx(1 hunks)src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 206-206: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/components/OrganizationCard/OrganizationCard.tsx (4)
1-20: LGTM! Comprehensive imports added for enhanced functionality.The imports are well-organized and include all necessary dependencies for UI components, translations, and GraphQL operations.
24-47: LGTM! Interface properly extended with required properties.The
InterfaceOrganizationCardPropsinterface has been updated with all necessary properties, including the crucialisJoinedproperty that aligns with the PR objectives.
86-114: LGTM! Hooks and mutations properly initialized.The component correctly initializes all necessary hooks and GraphQL mutations with proper refetch queries.
221-232: LGTM! Visit button implementation aligns with PR objectives.The Visit button is now correctly rendered based on the
isJoinedprop, which fixes the original issue mentioned in the PR objectives.src/components/OrganizationCard/OrganizationCard.spec.tsx (2)
17-72: LGTM! Comprehensive test setup with proper mocks.The test setup includes all necessary mocks for hooks, translations, and navigation, providing a solid foundation for testing.
269-368: LGTM! Comprehensive mutation test coverage.The tests thoroughly cover all membership-related operations, including joining organizations and withdrawing membership requests, with both success and error scenarios.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/components/OrganizationCard/OrganizationCard.tsx (3)
206-206: Simplify conditional rendering using optional chainingAt line 206, you can simplify the condition by using optional chaining for better readability.
Apply this diff to simplify the condition:
- {address && address.city && ( + {address?.city && (🧰 Tools
🪛 Biome (1.9.4)
[error] 206-206: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
119-119: Remove unnecessary inline commentThe comment seems to be a placeholder and can be removed to clean up the code.
Apply this diff:
- const id = getItem('userId'); // Adjust this line based on your actual localStorage key + const id = getItem('userId');🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 119-120: src/components/OrganizationCard/OrganizationCard.tsx#L119-L120
Added lines #L119 - L120 were not covered by tests
129-157: Add test coverage forjoinOrganizationfunctionThe
joinOrganizationfunction handles critical logic for joining organizations. The code coverage report indicates that these lines are not covered by tests.Consider adding unit tests to cover:
- Joining a public organization (
userRegistrationRequiredisfalse)- Sending a membership request to a private organization (
userRegistrationRequiredistrue)- Error handling and toast notifications
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: src/components/OrganizationCard/OrganizationCard.tsx#L129
Added line #L129 was not covered by tests
[warning] 131-131: src/components/OrganizationCard/OrganizationCard.tsx#L131
Added line #L131 was not covered by tests
[warning] 136-136: src/components/OrganizationCard/OrganizationCard.tsx#L136
Added line #L136 was not covered by tests
[warning] 138-138: src/components/OrganizationCard/OrganizationCard.tsx#L138
Added line #L138 was not covered by tests
[warning] 143-143: src/components/OrganizationCard/OrganizationCard.tsx#L143
Added line #L143 was not covered by tests
[warning] 145-145: src/components/OrganizationCard/OrganizationCard.tsx#L145
Added line #L145 was not covered by tests
[warning] 148-149: src/components/OrganizationCard/OrganizationCard.tsx#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 151-151: src/components/OrganizationCard/OrganizationCard.tsx#L151
Added line #L151 was not covered by tests
[warning] 153-153: src/components/OrganizationCard/OrganizationCard.tsx#L153
Added line #L153 was not covered by testssrc/components/OrganizationCard/OrganizationCard.spec.tsx (3)
150-151: Remove unused propsfirstNameandlastNamefromdefaultPropsThe
OrganizationCardcomponent no longer usesfirstNameandlastNameprops. Including them indefaultPropsis unnecessary.Apply this diff:
id: '123', image: 'https://via.placeholder.com/80', - firstName: 'John', - lastName: 'Doe', name: 'Sample',
237-238: Use translated text in assertionsWhen asserting button text, use the translated value instead of the translation key to ensure the test reflects the actual UI.
Apply this diff:
expect(joinButton).toBeInTheDocument(); - expect(joinButton).toHaveTextContent('joinNow'); + expect(joinButton).toHaveTextContent('Join Now');
282-283: Use translated text when querying elementsWhen selecting elements by text, use the translated value rather than the translation key.
Apply this diff to both occurrences:
- const joinButton = screen.getByText('joinNow'); + const joinButton = screen.getByText('Join Now');Also applies to: 301-302
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/OrganizationCard/OrganizationCard.spec.tsx(1 hunks)src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)src/components/test-utils/TestWrapper.tsx(1 hunks)src/screens/UserPortal/Organizations/Organizations.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/components/test-utils/TestWrapper.tsx
[warning] 42-42: src/components/test-utils/TestWrapper.tsx#L42
Added line #L42 was not covered by tests
[warning] 46-46: src/components/test-utils/TestWrapper.tsx#L46
Added line #L46 was not covered by tests
src/components/OrganizationCard/OrganizationCard.tsx
[warning] 86-86: src/components/OrganizationCard/OrganizationCard.tsx#L86
Added line #L86 was not covered by tests
[warning] 88-89: src/components/OrganizationCard/OrganizationCard.tsx#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 91-91: src/components/OrganizationCard/OrganizationCard.tsx#L91
Added line #L91 was not covered by tests
[warning] 94-94: src/components/OrganizationCard/OrganizationCard.tsx#L94
Added line #L94 was not covered by tests
[warning] 97-97: src/components/OrganizationCard/OrganizationCard.tsx#L97
Added line #L97 was not covered by tests
[warning] 102-102: src/components/OrganizationCard/OrganizationCard.tsx#L102
Added line #L102 was not covered by tests
[warning] 107-107: src/components/OrganizationCard/OrganizationCard.tsx#L107
Added line #L107 was not covered by tests
[warning] 112-112: src/components/OrganizationCard/OrganizationCard.tsx#L112
Added line #L112 was not covered by tests
[warning] 116-117: src/components/OrganizationCard/OrganizationCard.tsx#L116-L117
Added lines #L116 - L117 were not covered by tests
[warning] 119-120: src/components/OrganizationCard/OrganizationCard.tsx#L119-L120
Added lines #L119 - L120 were not covered by tests
[warning] 122-124: src/components/OrganizationCard/OrganizationCard.tsx#L122-L124
Added lines #L122 - L124 were not covered by tests
[warning] 129-129: src/components/OrganizationCard/OrganizationCard.tsx#L129
Added line #L129 was not covered by tests
[warning] 131-131: src/components/OrganizationCard/OrganizationCard.tsx#L131
Added line #L131 was not covered by tests
[warning] 136-136: src/components/OrganizationCard/OrganizationCard.tsx#L136
Added line #L136 was not covered by tests
[warning] 138-138: src/components/OrganizationCard/OrganizationCard.tsx#L138
Added line #L138 was not covered by tests
[warning] 143-143: src/components/OrganizationCard/OrganizationCard.tsx#L143
Added line #L143 was not covered by tests
[warning] 145-145: src/components/OrganizationCard/OrganizationCard.tsx#L145
Added line #L145 was not covered by tests
[warning] 148-149: src/components/OrganizationCard/OrganizationCard.tsx#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 151-151: src/components/OrganizationCard/OrganizationCard.tsx#L151
Added line #L151 was not covered by tests
[warning] 153-153: src/components/OrganizationCard/OrganizationCard.tsx#L153
Added line #L153 was not covered by tests
[warning] 160-161: src/components/OrganizationCard/OrganizationCard.tsx#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 164-164: src/components/OrganizationCard/OrganizationCard.tsx#L164
Added line #L164 was not covered by tests
[warning] 166-167: src/components/OrganizationCard/OrganizationCard.tsx#L166-L167
Added lines #L166 - L167 were not covered by tests
[warning] 170-170: src/components/OrganizationCard/OrganizationCard.tsx#L170
Added line #L170 was not covered by tests
[warning] 176-176: src/components/OrganizationCard/OrganizationCard.tsx#L176
Added line #L176 was not covered by tests
[warning] 179-179: src/components/OrganizationCard/OrganizationCard.tsx#L179
Added line #L179 was not covered by tests
[warning] 181-181: src/components/OrganizationCard/OrganizationCard.tsx#L181
Added line #L181 was not covered by tests
[warning] 227-227: src/components/OrganizationCard/OrganizationCard.tsx#L227
Added line #L227 was not covered by tests
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 206-206: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/components/OrganizationCard/OrganizationCard.tsx (2)
122-122: Removeconsole.errorin production environmentThe
console.errorstatement should be removed or wrapped in a development-only condition to prevent logging errors in production.Apply this diff:
- console.error('Failed to access localStorage:', error); + if (process.env.NODE_ENV === 'development') { + console.error('Failed to access localStorage:', error); + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-124: src/components/OrganizationCard/OrganizationCard.tsx#L122-L124
Added lines #L122 - L124 were not covered by tests
222-233: Conditional rendering of the "Visit" button is correctly implementedThe "Visit" button now properly appears for joined organizations based on the
isJoinedprop, aligning with the PR objectives.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 227-227: src/components/OrganizationCard/OrganizationCard.tsx#L227
Added line #L227 was not covered by testssrc/screens/UserPortal/Organizations/Organizations.tsx (3)
262-268: LGTM! Well-structured mapping logicThe code properly handles:
- Optional chaining for nested properties
- Consistent property assignment
- Fallback to empty array
Also applies to: 277-283
393-393: LGTM! Clean pagination implementationThe pagination logic is correctly implemented with proper null checks and array slicing.
Also applies to: 421-421
Line range hint
250-283: Verify complete implementation of Visit button fixWhile the code adds the
isJoinedproperty to support the Visit button visibility fix mentioned in the PR objectives, the implementation appears incomplete:
- The property needs to be added to the
InterfaceOrganizationCardProps- The
OrganizationCardcomponent needs to use this property for rendering the Visit buttonLet's verify the OrganizationCard component implementation:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/OrganizationCard/OrganizationCard.tsx (3)
67-70: Remove duplicate @param and @returns entries in TSDoc.The documentation contains duplicate entries that should be removed for clarity:
- Duplicate
@param props- Duplicate
@returns* @param membershipRequests - The list of membership requests with user IDs. * - * @param props - Properties for the organization card. - * @returns JSX element representing the organization card. * @returns The organization card component.
146-157: Improve error handling consistency in joinOrganization.The error handling could be more consistent with the pattern used in withdrawMembershipRequest. Consider adding a development-only console.error and simplifying the error type checking.
} catch (error: unknown) { - if (error instanceof Error) { - const apolloError = error as ApolloError; - const errorCode = apolloError.graphQLErrors?.[0]?.extensions?.code; - if (errorCode === 'ALREADY_MEMBER') { - toast.error(t('AlreadyJoined') as string); - } else { - toast.error(t('errorOccured') as string); - } + if (process.env.NODE_ENV === 'development') { + console.error('Failed to join organization:', error); + } + const apolloError = error as ApolloError; + const errorCode = apolloError.graphQLErrors?.[0]?.extensions?.code; + if (errorCode === 'ALREADY_MEMBER') { + toast.error(t('AlreadyJoined') as string); + } else { + toast.error(t('errorOccured') as string); } }
210-218: Use optional chaining for address check.The current address check can be simplified using optional chaining for better readability and maintainability.
- {address && address.city && ( + {address?.city && ( <div className={styles.address}> <h6 className="text-secondary"> <span className="address-line">{address.line1}, </span> <span className="address-line">{address.city}, </span> <span className="address-line">{address.countryCode}</span> </h6> </div>🧰 Tools
🪛 Biome (1.9.4)
[error] 210-210: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)src/components/test-utils/TestWrapper.tsx(1 hunks)src/screens/UserPortal/Organizations/Organizations.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/test-utils/TestWrapper.tsx
- src/screens/UserPortal/Organizations/Organizations.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 210-210: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
palisadoes
merged commit cc4f011
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Bugfix: Ensures the "Visit" button appears for all joined organizations when filtering by "Joined Organizations."
Issue Number:
Fixes #3162
Snapshots/Videos:
issue.3162.mp4
Summary
The issue was caused by a restrictive condition for rendering the "Visit" button. The button only appeared when membershipRequestStatus === 'accepted'. This PR modifies the logic to include all joined organizations by checking the props.isJoined property.
Does this PR introduce a breaking change?
No
Other information
The feature was tested in multiple scenarios to ensure seamless functionality.
Does this PR introduce a breaking change?
No, this PR does not introduce breaking changes.
Summary by CodeRabbit
New Features
TestWrappercomponent for enhanced testing capabilities.Bug Fixes
Documentation