feat: allow user to leave a group #88
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the functionality of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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: 2
🧹 Outside diff range and nitpick comments (5)
pages/groups/index.tsx (2)
85-85: Consider reducing duplication of YourGroups componentThe
YourGroupscomponent is rendered twice with identical props in different conditions. Consider extracting it to a single component render with conditional wrapping.- {isLoading ? ( - <YourGroups - groups={groupByMemberData ?? { groups: [] }} - proposals={proposalsByPolicyAccount} - isLoading={isLoading} - refetch={refetchGroupByMember} - /> - ) : isError ? ( + {isError ? ( <div className="text-center text-error">Error loading groups or proposals</div> ) : groupByMemberData?.groups.length === 0 ? ( <NoGroupsFound /> ) : ( <> <YourGroups groups={groupByMemberData ?? { groups: [] }} proposals={proposalsByPolicyAccount} isLoading={isLoading} refetch={refetchGroupByMember} /> {selectedPolicyAddress && ( <GroupInfoAlso applies to: 97-97
Line range hint
85-109: Consider implementing React Query or similar state managementThe current implementation passes refetch callbacks through multiple levels of components. Consider using React Query or a similar state management solution to handle data fetching and cache invalidation more efficiently. This would:
- Reduce prop drilling
- Provide automatic background refetching
- Handle loading and error states more consistently
- Improve caching and performance
components/groups/components/myGroups.tsx (2)
26-26: Add JSDoc documentation for the refetch propWhile the
refetchprop addition is good for handling data updates, its purpose and timing of invocation should be documented for better maintainability.Add JSDoc documentation above the props interface:
+/** + * @param refetch Callback function to refresh groups data after operations like leaving a group + */ export function YourGroups({ groups, proposals, isLoading, refetch, }: {Also applies to: 31-31
Line range hint
227-244: Consider consistent update handling across modalsWhile GroupInfo now correctly uses the
refetchcallback, MemberManagementModal still uses an empty callback. For consistency in data refresh handling, consider using therefetchcallback for both modals.<MemberManagementModal modalId={`member-management-modal-${group.id}`} members={group.members.map(member => ({ ...member.member, address: member?.member?.address || '', weight: member?.member?.weight || '0', metadata: member?.member?.metadata || '', added_at: member?.member?.added_at || new Date(), isCoreMember: true, isActive: true, isAdmin: member?.member?.address === group.admin, isPolicyAdmin: member?.member?.address === group.policies[0]?.admin, }))} groupId={group.id.toString()} groupAdmin={group.admin} policyAddress={group.policies[0]?.address ?? ''} address={address ?? ''} - onUpdate={() => {}} + onUpdate={refetch} />components/groups/modals/groupInfo.tsx (1)
119-119: Update error message to reflect the correct operationThe error message in the
catchblock refers to submitting a proposal, but the operation is actually leaving a group. Update the error message to accurately reflect the action being performed.Apply this diff:
- console.error('Error submitting proposal:', error); + console.error('Error leaving group:', error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/groups/components/myGroups.tsx(2 hunks)components/groups/modals/groupInfo.tsx(4 hunks)pages/groups/index.tsx(4 hunks)
🔇 Additional comments (2)
pages/groups/index.tsx (2)
109-109: LGTM! Proper data refresh implementation
Setting onUpdate to refetchGroupByMember ensures that the groups list is properly refreshed after a user leaves a group, maintaining data consistency.
22-22: Verify if refetchProposals is needed
The refetchProposals function is being destructured but not used anywhere in the component. Consider removing it if it's not needed for the current implementation.
- const { proposalsByPolicyAccount, isProposalsError, isProposalsLoading, refetchProposals } =
+ const { proposalsByPolicyAccount, isProposalsError, isProposalsLoading } =| console.error('Error submitting proposal:', error); | ||
| } finally { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide user feedback when an error occurs during leave operation
Currently, if an error occurs while leaving the group, the user is not notified. Consider displaying a user-friendly error message to inform the user of the issue.
Add code to display an error notification to the user. For example:
catch (error) {
console.error('Error leaving group:', error);
+ // Display error to the user
+ showErrorNotification('An error occurred while leaving the group. Please try again.');
} finally {
setIsSigning(false);
}Ensure that showErrorNotification is an appropriate method or component in your application for displaying errors.
Committable suggestion skipped: line range outside the PR's diff.
| <button | ||
| aria-label={'update-btn'} | ||
| className="btn btn-dropdown text-white rounded-[12px] h-[52px] w-[140px]" | ||
| onClick={handleLeave} | ||
| disabled={isSigning} | ||
| > | ||
| {isSigning ? <span className="animate-pulse">Leaving</span> : 'Leave'} | ||
| </button> | ||
|
|
||
| <button | ||
| aria-label={'update-btn'} |
There was a problem hiding this comment.
Duplicate aria-label values for buttons cause accessibility issues
Both the "Leave" and "Update" buttons have the same aria-label value 'update-btn', which can cause confusion for users relying on assistive technologies.
Assign unique and descriptive aria-labels to each button. Apply this diff:
<button
- aria-label={'update-btn'}
+ aria-label={'leave-group-btn'}
className="btn btn-dropdown text-white rounded-[12px] h-[52px] w-[140px]"
onClick={handleLeave}
disabled={isSigning}
>
{isSigning ? <span className="animate-pulse">Leaving</span> : 'Leave'}
</button>
<button
- aria-label={'update-btn'}
+ aria-label={'update-group-btn'}
className="btn btn-gradient text-white rounded-[12px] h-[52px] w-[140px]"
onClick={() => {
const modal = document.getElementById('update-group-modal') as HTMLDialogElement;
if (modal) modal.showModal();
}}
>
Update
</button>Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
+ Coverage 58.85% 59.73% +0.88%
==========================================
Files 145 145
Lines 14040 14175 +135
==========================================
+ Hits 8263 8468 +205
+ Misses 5777 5707 -70 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/groups/modals/groupInfo.tsx (1)
142-162: Add confirmation dialog for leave actionConsider adding a confirmation dialog before executing the leave action to prevent accidental group departures.
<button aria-label={'leave-btn'} className="btn btn-dropdown text-white rounded-[12px] h-[52px] w-[140px]" - onClick={handleLeave} + onClick={() => { + if (window.confirm('Are you sure you want to leave this group?')) { + handleLeave(); + } + }} disabled={isSigning} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/groups/modals/groupInfo.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/groups/modals/groupInfo.tsx
[warning] 101-120: components/groups/modals/groupInfo.tsx#L101-L120
Added lines #L101 - L120 were not covered by tests
🔇 Additional comments (3)
components/groups/modals/groupInfo.tsx (3)
6-8: LGTM: Clean import additions
The new imports are well-organized and appropriate for implementing the transaction handling functionality.
101-123:
Enhance error handling and add test coverage
The handleLeave implementation needs improvements in two areas:
- Error handling needs user feedback (previous comment still applies)
- Test coverage is missing for this critical functionality
For error handling, apply this diff:
} catch (error) {
console.error('Error submitting proposal:', error);
+ // Show error to user
+ const errorMessage = error instanceof Error ? error.message : 'Failed to leave group';
+ // Use your notification system, e.g.:
+ // toast.error(errorMessage);
} finally {Would you like me to help create unit tests for the leave group functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 101-120: components/groups/modals/groupInfo.tsx#L101-L120
Added lines #L101 - L120 were not covered by tests
Line range hint 101-162: Verify removal of Leave Group from proposal creation
The implementation successfully adds direct group leaving functionality. Let's verify the removal of "Leave Group" from proposal creation UI.
✅ Verification successful
Leave Group functionality has been correctly moved from proposals to direct action
The implementation aligns with the PR objectives. The codebase shows:
- Direct Leave Group functionality is implemented in
groupInfo.tsx - While
leaveGroupmessage type still exists in the message definitions, it's only used as a message composer for the direct action, not for proposal creation - The Leave Group option has been removed from the active proposal message types in the UI, as it's not listed in the
messageTypesarray inProposalMessages.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to "Leave Group" in proposal creation
rg -i "leave.*group" --type ts --type tsx
Length of output: 71
Script:
#!/bin/bash
# Search for any remaining references to "Leave Group" in proposal creation, adjusting file types
rg -i "leave.*group" -g '*.ts' -g '*.tsx'
Length of output: 1469
Script:
#!/bin/bash
# Let's check the proposal message form components to see if Leave Group is still available in the UI
ast-grep --pattern 'case "leaveGroup"' -l ts,tsx
# Also check the actual implementation of proposal messages
cat components/groups/forms/proposals/ProposalMessages.tsx
Length of output: 44114
Script:
#!/bin/bash
# Let's check the proposal message form components to see if Leave Group is still available in the UI
cat components/groups/forms/proposals/messages.ts
Length of output: 3728
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 101-120: components/groups/modals/groupInfo.tsx#L101-L120
Added lines #L101 - L120 were not covered by tests
| const { tx, isSigning, setIsSigning } = useTx(chainName); | ||
| const { leaveGroup } = cosmos.group.v1.MessageComposer.withTypeUrl; | ||
| const { estimateFee } = useFeeEstimation(chainName); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for fee estimation
While the transaction hooks are properly set up, consider adding error handling for fee estimation failures.
- const { estimateFee } = useFeeEstimation(chainName);
+ const { estimateFee, error: feeError } = useFeeEstimation(chainName);
+
+ useEffect(() => {
+ if (feeError) {
+ console.error('Fee estimation error:', feeError);
+ // Consider showing user feedback
+ }
+ }, [feeError]);📝 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 { tx, isSigning, setIsSigning } = useTx(chainName); | |
| const { leaveGroup } = cosmos.group.v1.MessageComposer.withTypeUrl; | |
| const { estimateFee } = useFeeEstimation(chainName); | |
| const { tx, isSigning, setIsSigning } = useTx(chainName); | |
| const { leaveGroup } = cosmos.group.v1.MessageComposer.withTypeUrl; | |
| const { estimateFee, error: feeError } = useFeeEstimation(chainName); | |
| useEffect(() => { | |
| if (feeError) { | |
| console.error('Fee estimation error:', feeError); | |
| // Consider showing user feedback | |
| } | |
| }, [feeError]); |
|
Added the following
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/groups/modals/groupInfo.tsx (2)
104-109: Add input validation for group IDThe group ID should be validated before attempting the transaction.
try { + if (!group?.id) { + throw new Error('Invalid group ID'); + } const msg = leaveGroup({ address: address, groupId: group?.id, });
144-164: Add confirmation dialog before leaving groupConsider adding a confirmation step before executing the leave operation, as it's a destructive action.
<button aria-label={'leave-btn'} className="btn btn-error text-white disabled:bg-red-900 rounded-[12px] h-[52px] w-[140px]" - onClick={handleLeave} + onClick={() => { + const confirmed = window.confirm('Are you sure you want to leave this group?'); + if (confirmed) { + handleLeave(); + } + }} disabled={isSigning} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/groups/modals/groupInfo.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/groups/modals/groupInfo.tsx
[warning] 101-122: components/groups/modals/groupInfo.tsx#L101-L122
Added lines #L101 - L122 were not covered by tests
🔇 Additional comments (5)
components/groups/modals/groupInfo.tsx (5)
6-8: LGTM: Imports are appropriate for transaction handling
The new imports provide the necessary functionality for transaction management and chain configuration.
146-146: Improve button accessibility with descriptive aria-labels
The current aria-labels are not descriptive enough.
- aria-label={'leave-btn'}
+ aria-label={'leave-group-btn'}
- aria-label={'update-btn'}
+ aria-label={'update-group-btn'}Also applies to: 155-155
18-20: 🛠️ Refactor suggestion
Add error handling for fee estimation
While the transaction hooks are properly set up, consider adding error handling for fee estimation failures.
- const { estimateFee } = useFeeEstimation(chainName);
+ const { estimateFee, error: feeError } = useFeeEstimation(chainName);
+
+ useEffect(() => {
+ if (feeError) {
+ console.error('Fee estimation error:', feeError);
+ // Consider showing user feedback
+ }
+ }, [feeError]);101-125: Add test coverage for leave group functionality
The new leave group functionality lacks test coverage according to static analysis.
Would you like me to help create test cases for the leave group functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 101-122: components/groups/modals/groupInfo.tsx#L101-L122
Added lines #L101 - L122 were not covered by tests
101-125: 🛠️ Refactor suggestion
Add comprehensive error handling and user feedback
The current implementation only logs errors to console. Users should be notified of failures.
} catch (error) {
console.error('Error leaving group:', error);
+ const modal = document.getElementById('error-toast') as HTMLDialogElement;
+ if (modal) {
+ modal.textContent = 'Failed to leave group. Please try again.';
+ modal.showModal();
+ }
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 101-122: components/groups/modals/groupInfo.tsx#L101-L122
Added lines #L101 - L122 were not covered by tests
fixes #50
Summary by CodeRabbit
Release Notes
New Features
YourGroupscomponent to support data refreshing via a newrefetchprop.handleLeavefunction in theGroupInfocomponent to facilitate group departure with transaction management.Improvements
GroupInfomodal to disable the "Leave" button during transactions and display a loading state.onUpdateprop to trigger data refetching.These changes streamline group management and enhance user experience within the application.