fix: admin page layout & refactor #38
Conversation
WalkthroughThe pull request introduces several modifications across multiple components, primarily focusing on enhancing user experience related to wallet connectivity. The Changes
Assessment against linked issues
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 59.39% 59.25% -0.14%
==========================================
Files 148 148
Lines 13830 13882 +52
==========================================
+ Hits 8214 8226 +12
- Misses 5616 5656 +40 ☔ 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: 1
🧹 Outside diff range and nitpick comments (18)
pages/factory/index.tsx (1)
Line range hint
73-81: Consider removing or conditionalizing debug loggingThe console.log statement should not be present in production code. Consider:
- Removing it entirely, or
- Wrapping it in a development-only condition
- console.log('Factory render', { - isLoading, - isError, - isDataReady, - combinedDataLength: combinedData.length, - denomsLength: denoms?.denoms?.length, - metadatasLength: metadatas?.metadatas?.length, - balancesLength: balances?.length, - totalSupplyLength: totalSupply?.length, - }); + if (process.env.NODE_ENV === 'development') { + console.log('Factory render', { + isLoading, + isError, + isDataReady, + combinedDataLength: combinedData.length, + denomsLength: denoms?.denoms?.length, + metadatasLength: metadatas?.metadatas?.length, + balancesLength: balances?.length, + totalSupplyLength: totalSupply?.length, + }); + }pages/factory/create.tsx (3)
100-103: Consider adding aria-label for better accessibilityThe implementation looks good with context-specific messaging and visual elements. Consider adding an aria-label to the icon for better accessibility.
<WalletNotConnected description={'Use the button below to connect your wallet and start creating new tokens'} - icon={<FactoryIcon className="h-60 w-60 text-primary" />} + icon={<FactoryIcon className="h-60 w-60 text-primary" aria-label="Factory icon" />} />
Line range hint
64-68: Optimize steps array definitionConsider moving the steps array outside the component to prevent unnecessary recreation on each render.
+const CREATE_TOKEN_STEPS = [ + { label: 'Create Denom', step: 1 }, + { label: 'Token Metadata', step: 2 }, + { label: 'Confirmation', step: 3 }, +]; + export default function CreateToken() { const [currentStep, setCurrentStep] = useState(1); const [formData, dispatch] = useReducer(tokenFormDataReducer, initialFormData); - const steps = [ - { label: 'Create Denom', step: 1 }, - { label: 'Token Metadata', step: 2 }, - { label: 'Confirmation', step: 3 }, - ];
Missing SEO meta URLs across multiple pages
The placeholder URLs issue is systemic across the application's pages. Empty URLs are found in:
- OpenGraph (og:url, og:image) and Twitter (twitter:image) meta tags
- Affects all major pages: admins, bank, factory/create, groups, and 404
These meta tags should be populated with actual URLs for proper SEO optimization and social media sharing functionality. Consider implementing a centralized configuration for these meta URLs or generating them dynamically based on the deployment environment.
🔗 Analysis chain
Line range hint
71-93: Update placeholder meta tags with actual valuesThe meta tags contain placeholder URLs (https://) which should be replaced with actual values for proper SEO.
The previous command failed because the file type wasn't recognized. Let's try a more generic search for URLs that end immediately (indicating placeholders) while searching all files. This will help us verify if this is a widespread issue or isolated to this file.
Let's verify if these are all related to meta tags and SEO content to provide a more comprehensive response about the scope of the issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other placeholder URLs in the codebase rg -l 'https://' --type=tsx --type=tsLength of output: 65
Script:
#!/bin/bash # Check for other placeholder URLs in the codebase using ripgrep without file type filters rg -l 'https://$|https://"' .Length of output: 212
Script:
#!/bin/bash # Let's check the actual context of these placeholder URLs to confirm they're in meta tags rg 'https://"' -A 1 -B 1 ./pages/Length of output: 4473
pages/groups/create.tsx (1)
Line range hint
69-93: Update placeholder URLs in meta tags.The meta tags contain placeholder URLs (https://) for og:url, og:image, twitter:image, and JSON-LD. These should be updated with actual production URLs for proper SEO and social sharing functionality.
- <meta property="og:url" content="https://" /> - <meta property="og:image" content="https://" /> + <meta property="og:url" content="https://your-domain.com/groups/create" /> + <meta property="og:image" content="https://your-domain.com/images/og-image.png" /> - <meta name="twitter:image" content="https://" /> + <meta name="twitter:image" content="https://your-domain.com/images/twitter-image.png" /> <script type="application/ld+json"> {JSON.stringify({ '@context': 'https://schema.org', '@type': 'WebPage', name: 'Create a group - Alberto', description: 'Alberto is the gateway to the Manifest Network', - url: 'https://', - image: 'https://', + url: 'https://your-domain.com/groups/create', + image: 'https://your-domain.com/images/schema-image.png', publisher: { '@type': 'Organization', name: 'Chandra Station', logo: { '@type': 'ImageObject', - url: 'https:///img/logo.png', + url: 'https://your-domain.com/images/logo.png', }, }, })} </script>pages/admins.tsx (2)
Line range hint
17-18: Consider extracting the default admin address to configurationThe hardcoded admin address
manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qjshould be moved to a configuration file for better maintainability across different environments.+ // Add to src/config/constants.ts + export const DEFAULT_ADMIN_ADDRESS = 'manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj'; - const { groupByAdmin, isGroupByAdminLoading } = useGroupsByAdmin( - poaAdmin ?? 'manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj' - ); + import { DEFAULT_ADMIN_ADDRESS } from '@/config/constants'; + const { groupByAdmin, isGroupByAdminLoading } = useGroupsByAdmin( + poaAdmin ?? DEFAULT_ADMIN_ADDRESS + );
72-114: Consider extracting conditional rendering logic for better readabilityThe nested conditional rendering logic is complex and could be simplified by extracting it into separate components or helper functions.
Consider refactoring like this:
const renderContent = () => { if (!isWalletConnected) { return ( <WalletNotConnected description="Use the button below to connect your wallet and access the admin features." icon={<AdminsIcon className="h-60 w-60 text-primary" />} /> ); } if (isGroupByAdminLoading || isPendingValidatorsLoading) { return <LoadingScreen message="Checking permission..." />; } if (!isMember) { return <AccessDeniedMessage />; } return ( <ValidatorList isLoading={isActiveValidatorsLoading || isPendingValidatorsLoading} activeValidators={validators ?? []} pendingValidators={pendingValidators ?? []} admin={poaAdmin ?? DEFAULT_ADMIN_ADDRESS} /> ); }; return ( <div className="flex-grow h-full animate-fadeIn h-screen transition-all duration-300"> <div className="w-full mx-auto"> {renderContent()} </div> </div> );pages/groups/index.tsx (3)
Line range hint
14-14: Remove console.log statementDevelopment logging statement should be removed from production code.
- console.log(groupByMemberData);
Line range hint
82-84: Enhance error message for better user experienceThe current error message is generic and doesn't provide users with actionable information.
- <div className="text-center text-error">Error loading groups or proposals</div> + <div className="text-center text-error"> + <p>Unable to load your groups or proposals.</p> + <p className="text-sm mt-2">Please check your connection and try again later.</p> + </div>
81-83: Consider adding a skeleton loader for loading stateInstead of reusing the YourGroups component with isLoading prop, consider implementing a dedicated skeleton loader for a better loading experience.
Example implementation:
function GroupsSkeleton() { return ( <div className="animate-pulse"> <div className="h-8 bg-base-300 rounded w-1/4 mb-4"></div> <div className="grid gap-4 md:grid-cols-2 lg:grid-cols-3"> {[1, 2, 3].map((i) => ( <div key={i} className="h-48 bg-base-300 rounded"></div> ))} </div> </div> ); }Then update the conditional rendering:
- <YourGroups - groups={groupByMemberData ?? { groups: [] }} - proposals={proposalsByPolicyAccount} - isLoading={isLoading} - /> + <GroupsSkeleton />pages/groups/submit-proposal/[policyAddress].tsx (2)
13-13: Remove unused importWalletSectionThe
WalletSectioncomponent is imported but not used in this file.-import { WalletNotConnected, WalletSection } from '@/components'; +import { WalletNotConnected } from '@/components';
Incomplete metadata URLs found across multiple pages
The issue of incomplete metadata URLs is not isolated to this file but appears to be a systemic pattern across the application. The following pages all contain incomplete Open Graph URLs:
- pages/404.tsx
- pages/admins.tsx
- pages/bank.tsx
- pages/factory/create.tsx
- pages/groups/index.tsx
- pages/groups/create.tsx
- pages/groups/submit-proposal/[policyAddress].tsx
Consider implementing a centralized configuration for the base URL and completing these metadata URLs across all pages to ensure proper social sharing functionality.
🔗 Analysis chain
Line range hint
77-82: Complete the metadata URLsThe Open Graph and Twitter metadata URLs are incomplete, containing only "https://" without actual domains. This could affect social sharing and SEO.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if metadata URLs are properly set in other pages rg -l "og:url.*https://$" "og:image.*https://$" "twitter:image.*https://$"Length of output: 201
Script:
#!/bin/bash # Check for metadata URLs in all files using ripgrep with proper regex patterns rg 'property="og:(url|image)".*content="https://"' --type tsx --type jsxLength of output: 102
Script:
#!/bin/bash # Search for metadata URLs in all files without file type restrictions rg 'property="og:(url|image)".*content="https://"'Length of output: 1160
components/wallet.tsx (3)
3-4: Remove unused importGroupsIconThe
GroupsIconis imported but not used in this file.-import { ArrowUpIcon, CopyIcon, GroupsIcon } from './icons'; +import { ArrowUpIcon, CopyIcon } from './icons';
Line range hint
124-143: Refactor copy functionality for better maintainabilityThe copy functionality uses direct DOM manipulation and duplicates logic. Consider:
- Using React state for managing copy feedback
- Creating a reusable copy button component
- Adding accessibility feedback for copy operations
+const CopyButton = ({ text, className }: { text: string; className?: string }) => { + const [copied, setCopied] = useState(false); + + const handleCopy = async () => { + await navigator.clipboard.writeText(text); + setCopied(true); + setTimeout(() => setCopied(false), 2000); + }; + + return ( + <button + onClick={handleCopy} + className={className} + aria-label={copied ? "Copied!" : "Copy to clipboard"} + > + {copied ? ( + <svg xmlns="http://www.w3.org/2000/svg" className="w-4 h-4 text-primary" viewBox="0 0 20 20" fill="currentColor"> + <path d="M9 2a1 1 0 000 2h2a1 1 0 100-2H9z" /> + <path fillRule="evenodd" d="M4 5a2 2 0 012-2 3 3 0 003 3h2a3 3 0 003-3 2 2 0 012 2v11a2 2 0 01-2 2H6a2 2 0 01-2-2V5zm9.707 5.707a1 1 0 00-1.414-1.414L9 12.586l-1.293-1.293a1 1 0 00-1.414 1.414l2 2a1 1 0 001.414 0l4-4z" clipRule="evenodd" /> + </svg> + ) : ( + <CopyIcon className="w-4 h-4 hover:text-primary" /> + )} + </button> + ); +};
Line range hint
186-245: Simplify wallet button rendering logicThe button rendering logic has multiple nested conditions that could be simplified. Also, the copy functionality should use the same reusable component suggested earlier.
-let onClick; -if (status === WalletStatus.Disconnected || status === WalletStatus.Rejected) - onClick = onClickConnect; -else onClick = openView; +const onClick = (status === WalletStatus.Disconnected || status === WalletStatus.Rejected) + ? onClickConnect + : openView; -const buttonData = buttons[status]; +const { icon: ButtonIcon } = buttons[status];components/admins/components/validatorList.tsx (3)
100-121: Enhance toggle buttons accessibilityWhile the toggle implementation is solid, consider adding ARIA attributes for better accessibility.
<button onClick={() => setActive(true)} + aria-pressed={active} + aria-label="Show active validators" className={`flex-1 py-2 px-4 text-sm font-medium rounded-xl ${ active ? 'dark:bg-[#FFFFFF1F] bg-[#FFFFFF] text-[#161616] dark:text-white' : 'text-[#808080]' }`} > Active </button> <button onClick={() => setActive(false)} + aria-pressed={!active} + aria-label="Show pending validators" className={`flex-1 py-2 px-4 text-sm font-medium rounded-xl ${ !active ? 'dark:bg-[#FFFFFF1F] bg-[#FFFFFF] text-[#161616] dark:text-white' : 'text-[#808080]' }`} > Pending </button>
146-167: Add test coverage for loading skeletonThe loading skeleton implementation provides good visual feedback, but test coverage is missing for this new feature.
Would you like me to help generate test cases for the loading state? The tests should verify:
- Skeleton UI is shown when isLoading is true
- Correct number of skeleton rows are rendered
- Skeleton UI is replaced with actual content when loading completes
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 147-165: components/admins/components/validatorList.tsx#L147-L165
Added lines #L147 - L165 were not covered by tests
204-213: Add test coverage for remove functionalityThe remove button implementation lacks test coverage for the click handler and event propagation.
Would you like me to help generate test cases for:
- Remove button click handler
- Event propagation prevention
- Modal trigger behavior
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 205-206: components/admins/components/validatorList.tsx#L205-L206
Added lines #L205 - L206 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
components/admins/components/validatorList.tsx(2 hunks)components/wallet.tsx(2 hunks)pages/admins.tsx(3 hunks)pages/factory/create.tsx(2 hunks)pages/factory/index.tsx(2 hunks)pages/governance.tsx(0 hunks)pages/groups/create.tsx(2 hunks)pages/groups/index.tsx(2 hunks)pages/groups/submit-proposal/[policyAddress].tsx(2 hunks)
💤 Files with no reviewable changes (1)
- pages/governance.tsx
🧰 Additional context used
📓 Learnings (1)
components/admins/components/validatorList.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/admins/components/validatorList.tsx:77-204
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/admins/components/validatorList.tsx`, the loading state is managed by the parent component, so it's not necessary to handle it within `ValidatorList`.
🪛 GitHub Check: codecov/patch
components/admins/components/validatorList.tsx
[warning] 147-165: components/admins/components/validatorList.tsx#L147-L165
Added lines #L147 - L165 were not covered by tests
[warning] 179-184: components/admins/components/validatorList.tsx#L179-L184
Added lines #L179 - L184 were not covered by tests
[warning] 205-206: components/admins/components/validatorList.tsx#L205-L206
Added lines #L205 - L206 were not covered by tests
components/wallet.tsx
[warning] 252-274: components/wallet.tsx#L252-L274
Added lines #L252 - L274 were not covered by tests
🔇 Additional comments (14)
pages/factory/index.tsx (3)
1-1: LGTM: Clean import of centralized WalletNotConnected component
The change aligns with the PR objective of removing duplicate WalletNotConnected implementations.
91-96: LGTM: Well-structured WalletNotConnected implementation
The implementation:
- Provides clear user guidance through descriptive text
- Maintains visual consistency with the primary color scheme
- Successfully uses the centralized component as intended
1-1: Verify admin page layout changes
While this file successfully implements the WalletNotConnected refactor, the main PR objective mentions admin page layout changes that aren't visible in this file.
✅ Verification successful
The previous command failed because the file extension wasn't recognized. Let's try a broader search for admin-related changes and files without type restriction, and also explicitly search for files with admin in their names across common JavaScript/TypeScript extensions.
Let's examine the admin page and its components to verify if there are any layout changes as mentioned in the PR objective.
Layout changes have been verified in the admin page
The git diff shows significant layout changes in pages/admins.tsx:
- Replaced custom wallet connection UI with reusable
WalletNotConnectedcomponent - Simplified page structure from complex nested sections to cleaner layout
- Removed search functionality and loading skeletons
- Improved loading state presentation
- Better responsive design with flex-grow and proper height management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for admin page related changes
rg -l "admin.*page|page.*admin" --type tsx
Length of output: 72
Script:
#!/bin/bash
# Search for admin page related changes using ripgrep without file type restriction
rg -l "admin.*page|page.*admin"
# Also search for any admin-related files
fd -e ts -e tsx -e js -e jsx "admin"
Length of output: 226
Script:
#!/bin/bash
# Let's check the content of the admins page and its components to understand the layout changes
cat pages/admins.tsx
cat components/admins/components/adminOptions.tsx
# Also check git diff to see actual changes in these files
git diff origin/main -- pages/admins.tsx components/admins/components/adminOptions.tsx
Length of output: 18132
pages/factory/create.tsx (1)
9-9: LGTM: Clean import of centralized component
The import of the centralized WalletNotConnected component aligns with the PR's goal of reducing code redundancy.
pages/groups/create.tsx (2)
11-11: LGTM: Import consolidation improves code organization.
The consolidated import of WalletNotConnected from a central location aligns with the PR's objective of removing duplicate implementations.
97-100: Verify icon size and color contrast.
The enhanced WalletNotConnected component implementation looks good with improved user guidance and visual elements. However, please verify:
- The icon size (60rem = 960px) seems unusually large. Consider using a more moderate size like h-20 w-20 (320px) or confirm if this is intentional.
- Ensure the text-primary color provides sufficient contrast for accessibility.
pages/admins.tsx (2)
Line range hint 1-11: LGTM! Clean import organization
The imports are well-organized and the addition of WalletNotConnected aligns with the PR objective of centralizing wallet connection handling.
80-83: Well-structured UI components with clear user feedback
The implementation provides clear feedback to users through:
- A centered loading indicator with descriptive text
- A well-designed access denied message with clear explanation
- Proper use of semantic HTML and accessibility-friendly structure
This aligns well with the PR objectives of improving the admin page layout.
Also applies to: 85-101
pages/groups/index.tsx (2)
1-1: LGTM: Import changes align with refactoring goals
The centralization of WalletNotConnected as a shared component reduces code duplication and improves maintainability.
75-80: LGTM: Enhanced wallet connection UI
The updated WalletNotConnected implementation improves user experience by:
- Providing clear instructions through the description prop
- Adding visual context with the GroupsIcon
- Maintaining consistent UI patterns across pages
pages/groups/submit-proposal/[policyAddress].tsx (1)
109-112: LGTM! Well-structured wallet connection UI
The implementation effectively uses the centralized WalletNotConnected component with clear messaging and contextually appropriate iconography for the proposal submission flow.
components/wallet.tsx (1)
252-277: 🛠️ Refactor suggestion
Add prop types validation and test coverage
The new WalletNotConnected component needs:
- Prop types validation
- Test coverage for different states and responsive behavior
Consider adding the following improvements:
+import PropTypes from 'prop-types';
export function WalletNotConnected({
description,
icon,
}: {
description: string;
icon: JSX.Element;
}) {
+ if (!description || !icon) {
+ console.warn('WalletNotConnected: Missing required props');
+ }
// ... rest of the component
}
+WalletNotConnected.propTypes = {
+ description: PropTypes.string.isRequired,
+ icon: PropTypes.element.isRequired,
+};Would you like me to help generate test cases for this component?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 252-274: components/wallet.tsx#L252-L274
Added lines #L252 - L274 were not covered by tests
components/admins/components/validatorList.tsx (2)
25-25: LGTM: Loading state management
The addition of the isLoading prop aligns with the established pattern where loading state is managed by the parent component.
77-99: LGTM: Layout restructuring meets design requirements
The new layout successfully:
- Positions the search box next to the "Validators" title
- Maintains responsive design
- Matches the group page layout (per PR objective)
| <Image | ||
| height={32} | ||
| width={32} | ||
| src={validator.logo_url} | ||
| alt="" | ||
| className="w-8 h-8 rounded-full" | ||
| /> | ||
| ) : ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for validator logos
The Image component should handle loading errors gracefully to prevent UI issues when logos fail to load.
<Image
height={32}
width={32}
src={validator.logo_url}
alt=""
+ alt={`${validator.description.moniker} logo`}
+ onError={(e) => {
+ e.currentTarget.onerror = null;
+ e.currentTarget.src = ''; // Will trigger the fallback avatar
+ }}
className="w-8 h-8 rounded-full"
/>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-184: components/admins/components/validatorList.tsx#L179-L184
Added lines #L179 - L184 were not covered by tests
This PR fixes the admin page layout to match the group page:
Also, I removed the
WalletNotConnectedduplicates and created a centralized function.Fixes #16
Summary by CodeRabbit
Release Notes
New Features
WalletNotConnectedcomponent to enhance user experience by providing informative prompts when the wallet is not connected across various pages.ValidatorListcomponent to display skeleton loaders during data fetching.Bug Fixes
Documentation
WalletNotConnectedto includedescriptionandiconfor better clarity and usability.Chores