Better Toasts #193
Better Toasts #193
Conversation
WalkthroughThis pull request introduces comprehensive updates across multiple components, focusing on enhancing user interface and interaction patterns. The changes primarily involve implementing proposal pagination in the Changes
Suggested labels
Possibly related PRs
Poem
Finishing Touches
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 (
|
|
We also correct the signing style of various buttons to match the rest of the signing buttons |
![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
🔭 Outside diff range comments (1)
pages/factory/index.tsx (1)
The "Create New Token" button should link to "/factory/create" instead of "/groups/create"
There appears to be a UX inconsistency where the button text suggests token creation but links to the group creation page. This is confusing because:
- A dedicated token creation page exists at
/factory/create- The button text "Create New Token" does not match its action of creating a group
- Users expecting to create a token will be unexpectedly redirected to group creation
🔗 Analysis chain
Line range hint
187-189: Verify if the "Create New Token" button should link to "/groups/create".There seems to be a potential UX confusion where the button text indicates token creation but links to the group creation page. Please verify if this is the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there's a separate token creation route fd -t f "create.*token" -x echo "Found potential token creation file: {}" # Check if groups are related to token creation rg -l "token.*creation" -g "groups/**"Length of output: 265
Script:
#!/bin/bash # Search for token-related files fd token # Look for token mentions in group-related files rg -i "token" -g "**/{group,groups}*" # Check for route definitions rg -i "route.*token|route.*group" -A 2 # Look for navigation patterns rg -i "href.*group|href.*token" -A 1Length of output: 71772
Script:
#!/bin/bash # Check group creation and token relationship rg -A 3 "Create.*Group" -g "pages/groups/**" # Check token creation within groups context rg -A 3 "Create.*Token" -g "**/*.{tsx,ts}" # Look for token management in groups rg -A 3 "token.*tab" -g "components/groups/**"Length of output: 2954
🧹 Nitpick comments (6)
components/groups/components/groupControls.tsx (2)
273-305: Consider extracting the size configuration to a constant.The responsive page size configuration is well-structured, but it could be extracted to a constant or configuration file for better maintainability.
+const PROPOSAL_SIZE_LOOKUP = [ + { + height: 700, + width: Infinity, + sizes: { proposals: 6 }, + }, + // ... rest of the configuration +]; - const sizeLookup = [ - { - height: 700, - width: Infinity, - sizes: { proposals: 6 }, - }, - // ... rest of the configuration - ]; + const sizeLookup = PROPOSAL_SIZE_LOOKUP;
530-588: Consider adding aria-label for the ellipsis.The pagination UI is well-implemented with proper accessibility attributes, but the ellipsis could benefit from an aria-label.
- <span key={pageNum} className="text-black dark:text-white"> + <span + key={pageNum} + className="text-black dark:text-white" + aria-label={`Pages ${pageNum-1} through ${pageNum+1}`} + > ... </span>components/groups/modals/voteDetailsModal.tsx (1)
849-849: Enhance the close button's accessibility and styling.The close button in the backdrop form should have proper styling and accessibility attributes.
- <button>close</button> + <button + className="sr-only" + aria-label="Close modal" + type="submit" + > + close + </button>hooks/useTx.tsx (1)
127-134: Consider adding transaction hash to description.For better user experience, consider including a shortened transaction hash in the description for easier reference.
- description: `Transaction completed successfully`, + description: `Transaction completed successfully (${res.transactionHash.slice(0, 8)}...)`,components/toast.tsx (1)
119-158: Consider implementing link type enum.Using string includes for link type checking is fragile and could lead to maintenance issues.
enum ToastLinkType { PROPOSAL = 'proposal', EXPLORER = 'explorer' } interface ToastLink { type: ToastLinkType; url: string; } // Update ToastMessage interface interface ToastMessage { // ... other properties link?: ToastLink; explorerLink?: ToastLink; }pages/factory/index.tsx (1)
133-139: Consider adding aria-label for better accessibility.The search input would benefit from an explicit aria-label to improve screen reader experience.
<input type="text" placeholder="Search for a token ..." + aria-label="Search for a token" className="input input-bordered w-full h-[40px] rounded-[12px] border-none bg-secondary text-secondary-content pl-10 focus:outline-none focus-visible:ring-1 focus-visible:ring-primary" value={searchTerm} onChange={e => setSearchTerm(e.target.value)} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
components/groups/components/groupControls.tsx(7 hunks)components/groups/components/myGroups.tsx(1 hunks)components/groups/modals/groupInfo.tsx(1 hunks)components/groups/modals/memberManagementModal.tsx(1 hunks)components/groups/modals/updateGroupModal.tsx(2 hunks)components/groups/modals/voteDetailsModal.tsx(10 hunks)components/toast.tsx(3 hunks)hooks/useTx.tsx(2 hunks)pages/bank.tsx(4 hunks)pages/factory/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/groups/modals/groupInfo.tsx
🧰 Additional context used
🪛 GitHub Actions: Test and Coverage
components/toast.tsx
[error] 27-27: Toast component is using useRouter without proper Next.js router context initialization
🔇 Additional comments (15)
components/groups/modals/memberManagementModal.tsx (1)
375-379: LGTM! Enhanced loading state feedbackThe replacement of static text with a loading spinner provides better visual feedback during the signing process, consistent with modern UI patterns.
components/groups/modals/updateGroupModal.tsx (2)
367-367: LGTM! Improved form validationThe addition of the
touchedstate check prevents premature form submissions and ensures that users have actually interacted with the form before allowing updates.Also applies to: 437-437
439-439: LGTM! Consistent loading feedbackThe loading spinner implementation maintains consistency with other modals in the application, providing a unified user experience during async operations.
components/groups/components/groupControls.tsx (3)
28-28: LGTM! Props and imports are well-structured.The new import and props enhance the component's functionality by adding responsive pagination and group info refresh capabilities.
Also applies to: 49-49, 54-54
307-314: LGTM! Pagination logic is well-implemented.The pagination implementation correctly handles the current page and page size calculations.
670-671: LGTM! Modal integration is properly updated.The VoteDetailsModal now correctly receives the necessary refetch functions to update group information and denominations.
components/groups/components/myGroups.tsx (1)
504-504: LGTM! GroupControls integration is properly updated.The refetch function is correctly passed to enable group information updates after proposal execution.
components/groups/modals/voteDetailsModal.tsx (2)
61-62: LGTM! Props interface updated correctly.The new props
refetchGroupInfoandrefetchDenomsare properly typed and integrated into the component signature.Also applies to: 76-77
243-244: LGTM! Consistent data refetching implementation.The refetch calls for group info and denominations are consistently implemented across all success handlers (executeProposal, executeWithdrawl, and VotingPopup), ensuring data stays in sync after state changes.
Also applies to: 265-266, 824-825
hooks/useTx.tsx (1)
26-33: LGTM! Well-structured interface definition.The ToastMessage interface is well-defined with clear optional and required properties, providing good type safety for toast messages.
pages/factory/index.tsx (2)
119-120: LGTM! Good improvement to prevent UI flickering.The condition
combinedData.length === 0 && !isLoadingensures the empty state is only shown after loading completes, preventing UI flicker between states.
151-159: LGTM! Proper implementation of paginated DenomList.The component correctly implements pagination with proper props passing, aligning with the PR objectives.
pages/bank.tsx (3)
83-83: LGTM! Good addition for accurate empty state handling.The
totalCountaddition improves the accuracy of determining when to show the empty state.
156-162: Same accessibility improvement needed as in factory page.
Line range hint
214-225: LGTM! Good defensive programming with the empty array fallback.The
sendTxs || []fallback prevents potential undefined errors while maintaining proper loading states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/admins/components/__tests__/validatorList.test.tsx (1)
11-18: Enhance router mock implementation for better test isolation.While the basic mock setup works, consider these improvements:
- Use a more descriptive name than 'm' for the mock function
- Reset the mock between tests to prevent state leakage
- Consider mocking additional router properties that might be needed in future tests
-const m = jest.fn(); +const mockRouter = jest.fn(); mock.module('next/router', () => ({ - useRouter: m.mockReturnValue({ + useRouter: mockRouter.mockReturnValue({ query: {}, push: jest.fn(), + pathname: '', + asPath: '', }), })); +afterEach(() => { + mockRouter.mockReset(); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/admins/components/__tests__/validatorList.test.tsx(2 hunks)
🔇 Additional comments (2)
components/admins/components/__tests__/validatorList.test.tsx (2)
Line range hint
1-8: LGTM! Well-structured test setup with proper testing utilities.The test file correctly uses Bun's test runner with React Testing Library, maintaining proper isolation through the custom chain provider wrapper.
Let's verify the custom render helper implementation:
✅ Verification successful
✓ Verified: Test setup is properly implemented with necessary providers
The custom render helper
renderWithChainProvidercorrectly wraps components with QueryClient, ChainProvider, and ToastProvider, ensuring proper test isolation and context availability. The implementation follows React Testing Library best practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the renderWithChainProvider implementation rg -A 10 "renderWithChainProvider" "tests/"Length of output: 622
Line range hint
31-83: Add test coverage for new proposal-related features.Based on the PR objectives, please add tests for:
- Proposal link in validator details
- Modal backdrop click behavior (mentioned as a fix in PR)
- Group information and denomination refetching when a proposal is executed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/groups/modals/__tests__/UpdateGroupModal.test.tsx (1)
10-17: LGTM! Consider enhancing router mock for proposal-related pathsThe router mock implementation is correct and follows Jest best practices. However, given this PR's focus on proposal handling, consider extending the mock to include proposal-specific route parameters that might be needed in future tests.
mock.module('next/router', () => ({ useRouter: m.mockReturnValue({ query: {}, push: jest.fn(), + // Add proposal-specific route parameters + asPath: '/groups/proposals', + pathname: '/groups/[groupId]/proposals/[proposalId]', }), }));components/groups/components/__tests__/groupInfo.test.tsx (1)
17-24: Maintain consistency in mock variable namingWhile the router mock implementation is correct, consider using consistent variable naming across test files. Other files use
mfor the mock function, while this file usesn.// Mock next/router -const n = jest.fn(); +const m = jest.fn(); mock.module('next/router', () => ({ - useRouter: n.mockReturnValue({ + useRouter: m.mockReturnValue({ query: {}, push: jest.fn(), }), }));components/groups/modals/__tests__/voteDetailsModal.test.tsx (2)
11-18: Consider mock organization and proposal-specific routesThe router mock implementation is correct, but consider:
- Moving it after other mocks for consistency with groupInfo.test.tsx
- Adding proposal-specific route parameters for comprehensive testing
-// Mock next/router -const m = jest.fn(); -mock.module('next/router', () => ({ - useRouter: m.mockReturnValue({ - query: {}, - push: jest.fn(), - }), -})); mock.module('react-apexcharts', () => ({ default: jest.fn(), })); mock.module('@cosmos-kit/react', () => ({ useChain: jest.fn().mockReturnValue({ address: mockProposals['test_policy_address'][0].proposers[0], chain: { fees: null }, }), })); +// Mock next/router +const m = jest.fn(); +mock.module('next/router', () => ({ + useRouter: m.mockReturnValue({ + query: { proposalId: mockProposals['test_policy_address'][0].id }, + push: jest.fn(), + asPath: '/groups/proposals', + pathname: '/groups/[groupId]/proposals/[proposalId]', + }), +}));
Line range hint
1-1: Consider creating a shared router mock utilityCurrently, router mocking is duplicated across multiple test files. Consider creating a shared test utility for router mocking to improve maintainability and consistency.
Create a new file
tests/mocks/router.ts:import { jest } from '@jest/globals'; export const mockRouter = (params = {}) => { const m = jest.fn(); return { module: () => ({ useRouter: m.mockReturnValue({ query: {}, push: jest.fn(), asPath: '/groups/proposals', pathname: '/groups/[groupId]/proposals/[proposalId]', ...params, }), }), mock: m, }; };Then in test files:
import { mockRouter } from '@/tests/mocks/router'; const { module, mock } = mockRouter({ query: { proposalId: '123' } }); mock.module('next/router', module);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/groups/components/__tests__/groupInfo.test.tsx(1 hunks)components/groups/modals/__tests__/UpdateGroupModal.test.tsx(2 hunks)components/groups/modals/__tests__/voteDetailsModal.test.tsx(1 hunks)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
+ Coverage 54.61% 54.83% +0.22%
==========================================
Files 153 153
Lines 15802 16077 +275
==========================================
+ Hits 8630 8816 +186
- Misses 7172 7261 +89 ☔ View full report in Codecov by Sentry. |
This proposal contains various fixes and features
Featurs
Fixes
Summary by CodeRabbit
New Features
NoAssetsFoundcomponent for better user feedback in the Factory and Bank components.Bug Fixes
User Experience