Implement Global Ultra-Wide Layout and Fixed Sidebar Behavior#184
Implement Global Ultra-Wide Layout and Fixed Sidebar Behavior#184DanielEmmanuel1 merged 16 commits intoSpherre-Labs:mainfrom
Conversation
WalkthroughSidebar now supports a persisted "pinned" mode and syncs desktop expansion with layout; ultra‑wide screens auto‑expand and center content. Global styles and tailwind breakpoints updated; navbar fixed. Multiple pages gain skeleton-loading gates, minor dark‑theme token tweaks, responsive spacing adjustments, and an NFT modal hookup in Treasury. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as Window
participant Layout as layout.tsx
participant Storage as localStorage
participant Sidebar as Sidebar.tsx
participant User as User
W->>Layout: resize -> checkScreenSize()
Layout->>Layout: set isMobile / isUltraWide
Layout->>Storage: read 'sidebarPinned'
Storage-->>Layout: pinned value
Layout->>Sidebar: render(isUltraWide, desktopSidebarExpanded, setDesktopSidebarExpanded)
Sidebar->>Storage: read 'sidebarExpanded' & 'sidebarPinned' on mount
Storage-->>Sidebar: values
alt User clicks Pin (desktop && not ultra‑wide)
User->>Sidebar: click Pin button
Sidebar->>Storage: write 'sidebarPinned'
Sidebar->>Layout: setDesktopSidebarExpanded(true/false)
end
alt Hover / outside-click / Escape
User->>Sidebar: hover/click/outside/escape
Sidebar->>Sidebar: collapse only if not isUltraWide && not isPinned && not mobile
Sidebar->>Storage: write 'sidebarExpanded' on change
end
Note over Storage,Layout: storage event -> cross-tab sync updates sidebar states
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/account/[address]/Sidebar.tsx (1)
39-70: Major: Missing cross-tab synchronization for pin state.The
isPinnedstate is persisted tolocalStorage(line 68) but lacks a storage event listener for cross-tab synchronization. When a user pins/unpins the sidebar in one tab, other tabs won't reflect the change until they refresh.Impact: Inconsistent UI state across multiple tabs of the same app.
Add a storage event listener similar to the pattern in
layout.tsx:useEffect(() => { if (mounted && typeof window !== 'undefined') { const savedPinned = localStorage.getItem('sidebarPinned') if (savedPinned !== null) { setIsPinned(JSON.parse(savedPinned)) } + + // Listen for changes from other tabs + const handleStorageChange = () => { + const pinned = localStorage.getItem('sidebarPinned') + if (pinned !== null) { + setIsPinned(JSON.parse(pinned)) + } + } + + window.addEventListener('storage', handleStorageChange) + return () => window.removeEventListener('storage', handleStorageChange) } }, [mounted])Note: This still doesn't solve the same-tab synchronization issue flagged in
layout.tsx.
🧹 Nitpick comments (3)
frontend/app/account/[address]/layout.tsx (1)
110-110: Extract magic number to a constant.The ultra-wide breakpoint
2560is hardcoded here and should match the3xlbreakpoint intailwind.config.ts. Define a shared constant to avoid inconsistencies.+const ULTRA_WIDE_BREAKPOINT = 2560 // Matches 3xl in tailwind.config.ts + // Check for mobile and ultra-wide screen sizes useEffect(() => { if (typeof window === 'undefined') return const checkScreenSize = () => { setIsMobile(window.innerWidth < 768) - setIsUltraWide(window.innerWidth >= 2560) + setIsUltraWide(window.innerWidth >= ULTRA_WIDE_BREAKPOINT) }frontend/app/account/[address]/Sidebar.tsx (2)
208-226: Good: Pin/Unpin toggle for desktop sidebar.The pin toggle button provides clear control over sidebar persistence on desktop, with proper conditional rendering (not shown on mobile or ultra-wide) and accessible labels.
Minor UX note: The icon shows the current state (
Pinwhen pinned) while the aria-label describes the action ("Unpin sidebar"). This is a valid pattern but could be confusing to some users. Consider using action-oriented icons (e.g.,Pinicon for "click to pin" when unpinned) if user feedback indicates confusion.
135-143: Verify: Nested ternary readability.The nested ternary for
isExpandedis logically correct and well-formatted, but consider extracting to a helper function if this logic needs to be reused or if the team prefers explicit conditionals for maintainability:const computeExpanded = () => { if (isUltraWide) return true if (isMobile) return sidebarExpanded return isPinned || expanded } const isExpanded = computeExpanded()This is purely a style preference—the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/account/[address]/Navbar.tsx(1 hunks)frontend/app/account/[address]/Sidebar.tsx(12 hunks)frontend/app/account/[address]/layout.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/account/[address]/Sidebar.tsx (1)
frontend/app/context/account-context.tsx (1)
useSpherreAccount(74-82)
🔇 Additional comments (3)
frontend/app/account/[address]/Navbar.tsx (1)
98-98: LGTM: Sticky navbar positioning.The addition of
sticky top-0 z-10ensures the navbar remains visible during scroll, which aligns well with the new layout system. The z-index value properly stacks above main content while remaining below the mobile sidebar overlay.frontend/app/account/[address]/layout.tsx (1)
157-159: Verify: Hover behavior causes sidebar overlay on desktop.When the sidebar is unpinned on desktop (non-mobile, non-ultra-wide), hovering expands it from
w-16tow-64, but this margin remains atml-16. This causes the sidebar to overlay content by 192px during hover.This behavior differs from typical sidebar patterns where the layout adjusts during expansion. Verify whether this overlay-on-hover UX is intentional or if the layout margin should also transition during hover.
If overlay-on-hover is not intended, the layout needs to track the hover state as well:
- Option 1: Pass hover state from Sidebar to layout via callback
- Option 2: Make the sidebar absolutely positioned only during hover-expand (more complex)
- Option 3: Remove hover-expand behavior when unpinned (simplest)
frontend/app/account/[address]/Sidebar.tsx (1)
18-18: LGTM: Ultra-wide prop integration.The
isUltraWideprop is properly integrated into the sidebar's expansion logic (lines 139-143) and interaction handlers (lines 82, 109, 167, 170, 209), ensuring the sidebar remains fixed and expanded on ultra-wide displays as required by the PR objectives.Also applies to: 28-28
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/account/[address]/AmountChart.tsx (1)
202-284: Wrap case bodies to satisfynoSwitchDeclarations.
Biome flags thesecaseclauses becauseconst/letdeclarations live in the shared switch scope, so the lint step fails even though youbreak. Wrapping each clause in its own block keeps the declarations local and unblocks the build. Apply this pattern to every case that declares variables.- case '3M': - // For 3M, create data for the last 90 days but in the selected year - const quarterStart = - selectedYearInt === currentYear - ? moment().subtract(89, 'days').startOf('day') - : moment() - .subtract(89, 'days') - .startOf('day') - .year(selectedYearInt) - - // Generate data points every 3 days (30 points total) - for (let i = 0; i < 30; i++) { - const date = moment(quarterStart) - .add(i * 3, 'days') - .toDate() + case '3M': { + // For 3M, create data for the last 90 days but in the selected year + const quarterStart = + selectedYearInt === currentYear + ? moment().subtract(89, 'days').startOf('day') + : moment() + .subtract(89, 'days') + .startOf('day') + .year(selectedYearInt) + + // Generate data points every 3 days (30 points total) + for (let i = 0; i < 30; i++) { + const date = moment(quarterStart) + .add(i * 3, 'days') + .toDate() ... - } - break + } + break + }As per static analysis hints.
♻️ Duplicate comments (1)
frontend/app/account/[address]/layout.tsx (1)
122-139: Storage event listener only fires for OTHER tabs—same-tab pin changes still won't sync.While the
storageevent listener (line 137) was added per the previous review, thestorageevent only fires in other tabs/windows, not the current tab. When the user clicks the pin button in Sidebar.tsx (line 232), localStorage is updated but no storage event fires in the same tab, socheckPinnedStatewon't be called anddesktopSidebarExpandedwon't update.Current behavior: User pins sidebar → Sidebar width changes to 256px (w-64) → layout margin stays at 64px (ml-16) → content overlaps sidebar by 192px.
The previous review's solution (adding a callback prop) was not implemented. The Sidebar still only writes to localStorage without notifying the parent layout.
Implement the callback approach from the previous review:
In layout.tsx, add a handler and pass it to Sidebar:
+ const handlePinnedChange = (pinned: boolean) => { + setDesktopSidebarExpanded(pinned) + } return ( <> <Sidebar accountName={accountName ?? 'Spherre Account'} navItems={navItems} selectedPage={selectedPage} isMobile={isMobile} isUltraWide={isUltraWide} sidebarExpanded={sidebarExpanded} setSidebarExpanded={setSidebarExpanded} desktopSidebarExpanded={desktopSidebarExpanded} setDesktopSidebarExpanded={setDesktopSidebarExpanded} + onPinnedChange={handlePinnedChange} />In Sidebar.tsx (lines 231-242), update the pin button:
<button - onClick={() => setIsPinned(!isPinned)} + onClick={() => { + const newPinned = !isPinned + setIsPinned(newPinned) + onPinnedChange?.(newPinned) + }} className={...}And add the prop to SidebarProps:
interface SidebarProps { ... desktopSidebarExpanded: boolean setDesktopSidebarExpanded: (expanded: boolean) => void + onPinnedChange?: (pinned: boolean) => void }
🧹 Nitpick comments (1)
frontend/app/globals.css (1)
607-611: Clarify container strategy: three different container classes serve overlapping purposes.Three container classes are defined with similar but distinct purposes:
.container-large(lines 607-611): Fixed 1500px max-width.main-content-centered(lines 799-805): Ultra-wide only, 2200px max-width.global-content-container(lines 904-930): Responsive max-widths (1400px → 1680px on ultra-wide)This creates confusion about which container to use. The layout.tsx uses
.global-content-container(line 185), but the PR objectives mention "centered layout" which could apply to any of these.Consider consolidating into a single primary container class with clear naming, or document when to use each class. For example:
/* Primary content container - use this for main page content */ .page-content-container { width: 100%; max-width: 1400px; margin: 0 auto; padding: 1rem; } /* Ultra-wide support */ @media (min-width: 2560px) { .page-content-container { max-width: 1680px; padding: 2.5rem; } }Also applies to: 799-805, 904-930
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/public/empty.svgis excluded by!**/*.svgfrontend/public/empty_nft.svgis excluded by!**/*.svgfrontend/public/request_placeholder.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
frontend/app/account/[address]/AmountChart.tsx(14 hunks)frontend/app/account/[address]/Navbar.tsx(1 hunks)frontend/app/account/[address]/Sidebar.tsx(11 hunks)frontend/app/account/[address]/deposit/deposit-via-address/components/ManualDeposit.tsx(4 hunks)frontend/app/account/[address]/layout.tsx(4 hunks)frontend/app/account/[address]/members/page.tsx(4 hunks)frontend/app/account/[address]/navigation.ts(1 hunks)frontend/app/account/[address]/page.tsx(5 hunks)frontend/app/account/[address]/settings/layout.tsx(1 hunks)frontend/app/account/[address]/smart-will/page.tsx(3 hunks)frontend/app/account/[address]/transactions/page.tsx(2 hunks)frontend/app/account/[address]/treasury/components/treasury-portfoliochat.tsx(2 hunks)frontend/app/account/[address]/treasury/page.tsx(5 hunks)frontend/app/account/[address]/withdraw/withdraw-step-two.tsx(1 hunks)frontend/app/create-account/layout.tsx(1 hunks)frontend/app/globals.css(4 hunks)frontend/components/shared/Button.tsx(1 hunks)frontend/components/welcome.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/app/account/[address]/members/page.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/app/account/[address]/page.tsx (1)
frontend/app/context/theme-context-provider.tsx (1)
useTheme(144-150)
frontend/app/account/[address]/Sidebar.tsx (1)
frontend/app/context/account-context.tsx (1)
useSpherreAccount(74-82)
frontend/app/account/[address]/layout.tsx (3)
frontend/app/context/account-context.tsx (1)
useSpherreAccount(74-82)frontend/app/account/[address]/navigation.ts (2)
getSelectedPage(22-32)NavItem(34-40)frontend/hooks/useSpherreHooks.ts (1)
useGetAccountName(145-201)
frontend/app/account/[address]/treasury/page.tsx (1)
frontend/app/components/NFTDetailsModal.tsx (1)
NFTDetailsModal(26-205)
🪛 Biome (2.1.2)
frontend/app/account/[address]/AmountChart.tsx
[error] 204-210: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (26)
frontend/components/welcome.tsx (2)
84-90: Verify image height on mobile screens.The left panel image is constrained to
h-32(128px) on mobile/tablet screens, which may appear quite small and potentially unbalanced compared to the content section. On a typical mobile viewport, this would be less than 20% of the screen height.Please verify this is the intended mobile design. If the image should be more prominent on smaller screens, consider using a larger minimum height like
h-48orh-64.
80-206: Review comment is incorrect. Welcome.tsx changes are appropriate and don't require ultra-wide logic.The original concern conflates separate objectives. This PR correctly includes:
- Sidebar.tsx & layout.tsx: Ultra-wide sidebar behavior (>=2560px detection,
isPinnedstate, fixed sidebar on ultra-wide screens)- Welcome.tsx: Responsive landing page redesign (padding wrapper, layout adjustments, theme menu, button styling)
Welcome.tsx is a standalone landing page without a sidebar—it should not and does not need ultra-wide breakpoint logic. The changes here are intentional responsive design improvements unrelated to the sidebar system.
The PR objectives remain internally consistent across files:
- Sidebar files handle ultra-wide layout and fixed behavior
- Welcome page handles its own responsive improvements
No changes needed.
Likely an incorrect or invalid review comment.
frontend/components/shared/Button.tsx (1)
20-20: LGTM! Consistent theming update.The secondary variant background color adjustment aligns with the broader dark theme token updates introduced in this PR.
frontend/app/account/[address]/treasury/components/treasury-portfoliochat.tsx (2)
276-276: LGTM! Visual refinement for desktop layout.The border thickness reduction from
border-4toborder(1px) creates a cleaner appearance on desktop displays while preserving the thickerborder-2on mobile for better touch interface visibility.
327-327: LGTM! Consistent border styling.This change matches the border thickness adjustment on the chart section (Line 276), ensuring visual consistency across desktop treasury components.
frontend/app/create-account/layout.tsx (1)
9-17: LGTM! Layout restructuring aligns with global layout system.The introduction of
container-largewrapper successfully implements the centered layout constraints for ultra-wide displays mentioned in the PR objectives. The responsive padding (px-4 sm:px-6 lg:px-8) ensures consistent spacing across all viewport sizes.frontend/app/account/[address]/transactions/page.tsx (1)
245-245: LGTM! Responsive padding enhances ultra-wide display support.The padding progression (p-4 on mobile → p-6 on tablet → p-8 on large screens) provides improved spacing across all viewport sizes, directly supporting the PR's ultra-wide layout objectives.
Also applies to: 261-261
frontend/app/account/[address]/settings/layout.tsx (1)
10-10: LGTM! Container simplification aligns with global layout system.The removal of redundant padding classes suggests that spacing is now consistently managed by the parent layout or global container utilities introduced in this PR.
frontend/app/account/[address]/withdraw/withdraw-step-two.tsx (1)
30-30: LGTM! Consistent color token update.The border color adjustment from
#272729to#24252Ais part of the coordinated theming refinements across the PR.frontend/app/account/[address]/smart-will/page.tsx (2)
108-108: LGTM! Responsive padding implementation.The addition of responsive padding (p-4 sm:p-6 lg:p-8) ensures proper spacing across viewport sizes, consistent with the ultra-wide layout enhancements throughout this PR.
129-129: LGTM! Consistent color token updates.The dark mode background color adjustments (#1C1D1F → #1D1E22 and #272729 → #24252A) are part of the coordinated theming refinements across the entire PR.
Also applies to: 251-251
frontend/app/account/[address]/Navbar.tsx (1)
98-98: No action required — the layout already compensates for the fixed navbar.The main content area in
frontend/app/account/[address]/layout.tsxincludespt-16 lg:pt-20padding (64px on mobile, 80px on desktop), which exceeds the navbar height and prevents content from being hidden beneath it. The concern has already been addressed in the code.frontend/app/account/[address]/deposit/deposit-via-address/components/ManualDeposit.tsx (1)
28-28: LGTM! Dark theme color tokens updated consistently.The color token updates align with the global dark theme changes in
globals.css(lines 41-42, 50, 52-53) where--theme-bg-secondaryand--theme-bg-tertiarywere updated to#1d1e22and#24252arespectively.Also applies to: 115-115, 130-130, 186-186
frontend/app/account/[address]/page.tsx (2)
16-16: LGTM! Balance visibility toggle implemented correctly.The balance visibility feature is well-implemented with proper state management, accessibility attributes (
aria-label), and appropriate visual feedback using Eye/EyeOff icons from lucide-react.Also applies to: 23-23, 68-70
112-116: Well-structured conditional rendering for balance display.The nested ternary correctly handles three states: loading, hidden, and visible. The masked value "•••••" provides clear visual feedback when balance is hidden.
frontend/app/account/[address]/treasury/page.tsx (1)
13-13: LGTM! NFT details modal integrated cleanly.The modal integration follows the same pattern as the WithdrawalModal and DepositModal already present in this component. State typing (
number | undefined) is appropriate for tracking which NFT is selected.Also applies to: 133-133, 241-241, 254-254
frontend/app/account/[address]/navigation.ts (2)
22-32: LGTM! Page resolution helper correctly implements route matching.The
getSelectedPagefunction properly usesstartsWithto match nested routes (e.g.,/account/${address}/settings/profilematches the/account/${address}/settingsroute), with a sensible fallback to 'Dashboard'.
5-16: Route prefix change is consistently implemented across the application—no issues found.The verification confirms:
- All 12 routes in
navigation.tsuse the new/account/${address}/prefix- Central
routes.tsutility exports all account routes with the new prefix- Zero hardcoded route strings bypass the
routes()utility function- All navigation calls use
routes(address).{routeName}pattern (20+ verified instances)- File structure matches the routing implementation
The routing change is complete, consistent, and properly maintained through the centralized
routes()utility function.frontend/app/globals.css (1)
37-58: LGTM! Dark theme color variables updated consistently.The dark theme background colors have been updated systematically across all theme variables (
--background,--theme-bg,--theme-bg-secondary,--theme-bg-tertiary, card backgrounds, sidebar, navbar, and modal backgrounds). These align with the component-level changes in ManualDeposit.tsx.frontend/app/account/[address]/Sidebar.tsx (3)
76-88: Parent state synchronization correctly handles ultra-wide and mobile exclusions.The effect properly updates
desktopSidebarExpandedonly when not on mobile or ultra-wide, and correctly computes the expanded state asisPinned || expanded. The dependency array is complete and accurate.
157-161: Expansion logic correctly prioritizes ultra-wide, then mobile, then desktop pinned/hover.The computed
isExpandedvalue elegantly handles all three modes with proper priority:
- Ultra-wide: always expanded
- Mobile: controlled by
sidebarExpandedprop- Desktop: pinned takes precedence over hover (
isPinned || expanded)
226-244: Pin button correctly hidden on mobile and ultra-wide as intended.The conditional rendering
{!isMobile && !isUltraWide && ...}ensures the pin button only appears on desktop (non-mobile) and non-ultra-wide screens, which aligns with the PR objectives where ultra-wide displays have a permanently fixed sidebar.frontend/app/account/[address]/layout.tsx (4)
104-105: LGTM! Filtering comingSoon items keeps the sidebar cleaner.Filtering out items with
comingSoon: truefrom the navigation prevents clutter while preserving the ability to toggle features via theallNavItemsarray.
111-120: Screen size detection correctly identifies mobile and ultra-wide breakpoints.The
checkScreenSizefunction properly sets bothisMobile(< 768px) andisUltraWide(≥ 2560px) with appropriate resize event handling. The 2560px breakpoint aligns with the PR objectives for 4K/ultra-wide displays.
141-146: Ultra-wide auto-expansion correctly forces sidebar to remain expanded.When
isUltraWidebecomes true, the sidebar is automatically expanded viasetDesktopSidebarExpanded(true), which ensures the content margin adjusts toml-64(line 173). This aligns with the PR objective of making the sidebar fixed and always visible on ultra-wide displays.
169-177: Content margin logic correctly handles all three viewport states.The conditional className properly applies:
ml-0: Mobile (sidebar overlay, no margin needed)ml-64: Ultra-wide OR desktop expanded (sidebar is 256px wide)ml-16: Desktop collapsed (sidebar is 64px wide)This ensures content never overlaps the sidebar (assuming the pin sync issue is fixed).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/account/[address]/members/components/member-card.tsx (1)
129-133: Member updates may be ignored when permissions are empty.The condition
if (perms.permissions.length > 0)preventscurrentMemberfrom updating when thememberprop changes but permissions are empty or not yet loaded. This means the card could display stale member data (name, address, image) until permissions arrive.Consider removing the length check or handling the empty case explicitly:
useEffect(() => { - if (perms.permissions.length > 0) { - setCurrentMember({ ...member, permissions: perms.permissions }) - } + setCurrentMember({ ...member, permissions: perms.permissions }) }, [perms.permissions, member])
🧹 Nitpick comments (6)
frontend/app/account/[address]/members/components/member-card.tsx (2)
133-133: Consider useMemo for derived state to avoid object-identity issues.Adding
memberto the dependency array ensures correctness when the member prop changes, which is good. However, sincememberis an object, this effect will re-run whenever the parent creates a new object instance—even if the values are identical—potentially causing unnecessary updates.Replace the
useState+useEffectpattern withuseMemofor better performance:- const [currentMember, setCurrentMember] = useState<Member>(member) - - // ...existing code... - - useEffect(() => { - if (perms.permissions.length > 0) { - setCurrentMember({ ...member, permissions: perms.permissions }) - } - }, [perms.permissions, member]) + const currentMember = useMemo( + () => ({ ...member, permissions: perms.permissions }), + [member, perms.permissions] + )This derives
currentMemberdirectly from props without maintaining separate state.
89-106: Consider usingcurrentMemberconsistently in callbacks.The callbacks
startEditing(line 92) andopenDetails(lines 98-106) reference thememberprop directly, while the rendered UI displayscurrentMember. This inconsistency could cause the callbacks to operate on stale data if permissions update the member's state.Update the callbacks to use
currentMember:const startEditing = useCallback( (memberId: number) => { setEditingId(memberId) - setEditName(member.name) + setEditName(currentMember.name) setDropdownOpen(null) }, - [member.name, setEditName, setDropdownOpen], + [currentMember.name, setEditName, setDropdownOpen], ) const openDetails = useCallback(() => { setMemberDetails({ - id: member.id, - name: member.name, - address: member.address, - fullAddress: member.fullAddress, + id: currentMember.id, + name: currentMember.name, + address: currentMember.address, + fullAddress: currentMember.fullAddress, }) setIsMemberDetailsOpen(true) - }, [member, setMemberDetails, setIsMemberDetailsOpen]) + }, [currentMember, setMemberDetails, setIsMemberDetailsOpen])frontend/components/shared/ThemeButton.tsx (2)
27-27: Consider explicit alt text or aria-label for the button.The image has an empty
altattribute. While this may be intentional if the icon is purely decorative, the button itself lacks anaria-label, which could improve accessibility when the icon provides context.If the icon is meaningful, apply this diff:
- {icon && <Image src={icon} height={30} width={24} alt="" />} + {icon && <Image src={icon} height={30} width={24} alt="theme icon" />}Alternatively, add an
aria-labelto the button for better screen reader support:<button onClick={onClick} + aria-label="Toggle theme" className={`w-full sm:w-72 flex items-center justify-center gap-1 px-6 py-2 rounded-lg mx-auto my-3 transition-all duration-200 ${
28-30: Use<span>instead of<p>for better flexibility.Wrapping
childrenin a<p>tag inside a button can cause HTML validation issues if the children contain block-level elements. A<span>is more appropriate for inline button content.Apply this diff:
- <p className={actualTheme === 'dark' ? 'text-black' : 'text-white'}> + <span className={actualTheme === 'dark' ? 'text-black' : 'text-white'}> {children} - </p> + </span>frontend/app/globals.css (2)
613-617: Consider removing commented debug code.The commented
.debug-containerclass can be safely removed from production code. If you need it for development, consider using a development-only stylesheet or enabling it via an environment flag.Apply this diff to remove it:
-/* Debug class to visualize container boundaries (remove in production) */ -/* .debug-container { - outline: 2px dashed rgba(111, 47, 206, 0.3); - outline-offset: -2px; -} */ -
894-898: Redundant!importantdeclarations for chart background.Lines 896-897 both set transparent backgrounds using
!important:
background: transparent !important;(shorthand)background-color: transparent !important;(specific)Since
backgroundis shorthand that includesbackground-color, only one is needed. The comment mentions needing a "more specific selector," which suggests this is working around specificity issues.Option 1 (preferred): Use a more specific selector to avoid
!important:-/* Force chart canvas to be transparent - more specific selector */ -.chart-container canvas { - background: transparent !important; - background-color: transparent !important; -} +/* Force chart canvas to be transparent */ +.chart-container canvas, +.chart-container canvas[role="img"] { + background: transparent; + background-color: transparent; +}Option 2: Keep one
!importantif truly necessary:-/* Force chart canvas to be transparent - more specific selector */ .chart-container canvas { - background: transparent !important; background-color: transparent !important; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/account/[address]/members/components/member-card.tsx(1 hunks)frontend/app/globals.css(4 hunks)frontend/components/shared/ThemeButton.tsx(1 hunks)frontend/components/welcome.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/components/welcome.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/components/shared/ThemeButton.tsx (1)
frontend/app/context/theme-context-provider.tsx (1)
useTheme(144-150)
🔇 Additional comments (6)
frontend/components/shared/ThemeButton.tsx (1)
1-9: LGTM: Clean component structure.The imports and interface definition are well-structured and type-safe.
frontend/app/globals.css (5)
37-58: LGTM: Dark theme color refinements.The updated dark theme tokens provide better visual hierarchy with lighter backgrounds and darker card surfaces.
521-529: LGTM: Clean utility classes.The new
.z-25and.main-content-transitionutilities are well-defined and support the layout improvements.
596-604: LGTM: Standard full-width breakout pattern.The
.full-widthutility correctly implements the viewport-based technique to break out of container constraints.
848-863: LGTM: Well-defined animations.The
slideDownkeyframes and.hover\:scale-102utility are properly implemented and support smooth UI transitions.
532-594: Remove unused container classes or clarify their purpose.Verification revealed that only 2 of 4 container classes are actually used in the codebase:
.responsive-container(defined but never used).container-large(used once infrontend/app/create-account/layout.tsx).global-content-container(used once infrontend/app/account/[address]/layout.tsx).main-content-centered(defined but never used)The two unused classes should be either removed as dead code or their purpose clarified if they're reserved for future use. The two active classes serve different layout contexts and consolidation is not necessarily warranted.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/globals.css (1)
774-893: Inconsistent max-widths across multiple container utilities.Three container classes define different max-widths for ultra-wide screens (≥2560px):
.main-content-centered: 2200px (line 777).responsive-container: 1800px (line 591).global-content-container: 1680px (line 890)This inconsistency can confuse developers and lead to unpredictable layouts when these classes are mixed. Consider standardizing on a single max-width for ultra-wide displays or documenting the distinct use cases for each container class.
Apply this approach:
-/* Ultra-wide and 4K centered layout */ -@media (min-width: 2560px) { - .main-content-centered { - max-width: 2200px; - margin: 0 auto; - } -} +/* Ultra-wide and 4K centered layout */ +@media (min-width: 2560px) { + .main-content-centered { + max-width: 1800px; /* Standardized with .responsive-container */ + margin: 0 auto; + } +}And update
.global-content-container:/* Ultra-wide screen support - maintain centering */ @media (min-width: 2560px) { .global-content-container { - max-width: 1680px; /* Slightly larger for ultra-wide */ + max-width: 1800px; /* Standardized with other containers */ padding: 2.5rem 2.5rem; } }Alternatively, if each container serves a distinct purpose, add CSS comments explaining when to use each.
♻️ Duplicate comments (1)
frontend/app/account/[address]/Sidebar.tsx (1)
44-56: Hydration mismatch may cause visual flash on load.Loading sidebar state from
localStorageinuseEffectmeans the initial render uses default values (false), then hydrates with saved preferences after mount. This can cause a brief visual flash where the sidebar appears in the wrong state before correcting itself.As noted in the previous review, consider using lazy state initializers to read
localStoragesynchronously (with SSR guards):- const [expanded, setExpanded] = useState(false) - const [isPinned, setIsPinned] = useState(false) + const [expanded, setExpanded] = useState(() => { + if (typeof window !== 'undefined') { + const saved = localStorage.getItem('sidebarExpanded') + return saved !== null ? JSON.parse(saved) : false + } + return false + }) + const [isPinned, setIsPinned] = useState(() => { + if (typeof window !== 'undefined') { + const saved = localStorage.getItem('sidebarPinned') + return saved !== null ? JSON.parse(saved) : false + } + return false + }) - // Load saved preferences after mount - useEffect(() => { - if (typeof window !== 'undefined') { - const savedExpanded = localStorage.getItem('sidebarExpanded') - if (savedExpanded !== null) { - setExpanded(JSON.parse(savedExpanded)) - } - - const savedPinned = localStorage.getItem('sidebarPinned') - if (savedPinned !== null) { - setIsPinned(JSON.parse(savedPinned)) - } - } - }, [])
🧹 Nitpick comments (2)
frontend/app/globals.css (1)
895-899: Redundant!importantdeclarations and potential specificity issue.Using
!importanton bothbackgroundandbackground-coloris redundant sincebackgroundis a shorthand that includesbackground-color. Additionally, the need for!importantsuggests a specificity conflict that might be better resolved by strengthening the selector.Apply this diff:
-/* Force chart canvas to be transparent - more specific selector */ -.chart-container canvas { - background: transparent !important; - background-color: transparent !important; -} +/* Force chart canvas to be transparent */ +.chart-container canvas, +.chart-container canvas[style] { + background: transparent !important; +}The
canvas[style]selector targets canvases with inline styles, which increases specificity without needing duplicate!importantdeclarations.frontend/app/account/[address]/Sidebar.tsx (1)
143-151: Nested ternaries reduce readability of expansion logic.The nested ternary operators make the sidebar expansion logic harder to understand and maintain at a glance.
Consider refactoring to a more explicit format:
- // Determine if sidebar should be expanded: - // - Ultra-wide: always expanded - // - Mobile: use sidebarExpanded state - // - Desktop: if pinned, always expanded; otherwise use hover-based expanded state - const isExpanded = isUltraWide - ? true - : isMobile - ? sidebarExpanded - : isPinned || expanded + // Determine if sidebar should be expanded: + // - Ultra-wide: always expanded + // - Mobile: use sidebarExpanded state + // - Desktop: if pinned, always expanded; otherwise use hover-based expanded state + const isExpanded = (() => { + if (isUltraWide) return true + if (isMobile) return sidebarExpanded + return isPinned || expanded + })()Or using a useMemo for clarity:
const isExpanded = useMemo(() => { if (isUltraWide) return true if (isMobile) return sidebarExpanded return isPinned || expanded }, [isUltraWide, isMobile, sidebarExpanded, isPinned, expanded])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/account/[address]/Sidebar.tsx(9 hunks)frontend/app/globals.css(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/account/[address]/Sidebar.tsx (1)
frontend/app/context/account-context.tsx (1)
useSpherreAccount(74-82)
🔇 Additional comments (1)
frontend/app/account/[address]/Sidebar.tsx (1)
291-309: Large margin-bottom may cause unexpected spacing.The pin toggle button has
mb-40(10rem / 160px margin-bottom), which creates substantial space between it and the profile section. Verify this is intentional spacing for the design.If the large margin is unintentional, consider reducing it:
{!isMobile && !isUltraWide && ( <div - className={`mb-40 ${isExpanded ? 'px-3' : 'flex justify-center'}`} + className={`mb-4 ${isExpanded ? 'px-3' : 'flex justify-center'}`} >
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/account/[address]/AmountChart.tsx (1)
202-230: Wrap case '3M' body in a block to scope declarations.Variables declared in switch cases should be wrapped in blocks to prevent accidental access from other cases and improve code clarity.
Apply this diff:
case '3M': - // For 3M, create data for the last 90 days but in the selected year - const quarterStart = - selectedYearInt === currentYear - ? moment().subtract(89, 'days').startOf('day') - : moment() - .subtract(89, 'days') - .startOf('day') - .year(selectedYearInt) - - // Generate data points every 3 days (30 points total) - for (let i = 0; i < 30; i++) { - const date = moment(quarterStart) - .add(i * 3, 'days') - .toDate() - - // Larger changes for 3-month view - only after mount - if (mounted) { - baseValue += (Math.random() - 0.5) * 100 - } - - data.push({ - date, - year: moment(date).format('YYYY'), - displayDate: moment(date).format('MMM DD'), - value: Math.max(50, Math.round(baseValue)), - }) - } + { + // For 3M, create data for the last 90 days but in the selected year + const quarterStart = + selectedYearInt === currentYear + ? moment().subtract(89, 'days').startOf('day') + : moment() + .subtract(89, 'days') + .startOf('day') + .year(selectedYearInt) + + // Generate data points every 3 days (30 points total) + for (let i = 0; i < 30; i++) { + const date = moment(quarterStart) + .add(i * 3, 'days') + .toDate() + + // Larger changes for 3-month view - only after mount + if (mounted) { + baseValue += (Math.random() - 0.5) * 100 + } + + data.push({ + date, + year: moment(date).format('YYYY'), + displayDate: moment(date).format('MMM DD'), + value: Math.max(50, Math.round(baseValue)), + }) + } + } break
🧹 Nitpick comments (2)
frontend/app/account/[address]/AmountChart.tsx (1)
607-723: Consider extracting the custom dropdown to a separate component.The year dropdown spans ~117 lines with complex inline styles, conditional classes, and gradients. This increases the cognitive complexity of the main component and makes maintenance more difficult.
Consider creating a reusable
<YearDropdown>component:// components/YearDropdown.tsx interface YearDropdownProps { selectedYear: string availableYears: string[] disabled: boolean onYearChange: (year: string) => void colors: { selectBg: string selectText: string } } export function YearDropdown({ selectedYear, availableYears, disabled, onYearChange, colors }: YearDropdownProps) { const [isOpen, setIsOpen] = useState(false) const dropdownRef = useRef<HTMLDivElement>(null) // ... move dropdown logic and JSX here return ( <div className="relative" ref={dropdownRef}> {/* dropdown JSX */} </div> ) }Then use it in AmountChart:
<YearDropdown selectedYear={selectedYear} availableYears={availableYears} disabled={dateRange === 'ALL'} onYearChange={handleYearChange} colors={colors} />frontend/app/account/[address]/Tabs.tsx (1)
167-315: Consider simplifying the IIFE pattern for better readability.The immediately-invoked function expression (IIFE) on line 168 works but adds unnecessary nesting. A simpler approach would improve readability.
Consider this refactor:
- {activeTab === 'Tokens' && - (() => { - const isPending = loadingTokenData || !minSkeletonElapsed - if (isPending) { - return ( + {activeTab === 'Tokens' && (() => { + const isPending = loadingTokenData || !minSkeletonElapsed + + if (isPending) { + return ( + <div className="my-2 rounded-lg bg-theme-bg-tertiary border border-theme-border p-4 sm:p-6"> {/* skeleton UI */} - ) - } - return ( + </div> + ) + } + + return ( + <div className="my-2 px-2 py-2 rounded-lg"> {/* actual tokens UI */} - ) - })()} + </div> + ) + })()}Or even simpler, compute
isPendingoutside the JSX:+ const isTokensPending = loadingTokenData || !minSkeletonElapsed + return ( <div className="text-theme transition-colors duration-300"> {/* ... */} - {activeTab === 'Tokens' && - (() => { - const isPending = loadingTokenData || !minSkeletonElapsed - if (isPending) { - return (/* skeleton */) - } - return (/* tokens */) - })()} + {activeTab === 'Tokens' && isTokensPending && ( + /* skeleton UI */ + )} + {activeTab === 'Tokens' && !isTokensPending && ( + /* actual tokens UI */ + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/account/[address]/AmountChart.tsx(15 hunks)frontend/app/account/[address]/Tabs.tsx(5 hunks)frontend/app/account/[address]/members/page.tsx(3 hunks)frontend/app/account/[address]/page.tsx(10 hunks)frontend/app/account/[address]/treasury/page.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/app/account/[address]/members/page.tsx (1)
frontend/components/ui/skeleton.tsx (1)
Skeleton(15-15)
frontend/app/account/[address]/treasury/page.tsx (1)
frontend/app/components/NFTDetailsModal.tsx (1)
NFTDetailsModal(26-205)
frontend/app/account/[address]/Tabs.tsx (1)
frontend/components/ui/skeleton.tsx (1)
Skeleton(15-15)
frontend/app/account/[address]/page.tsx (1)
frontend/app/context/theme-context-provider.tsx (1)
useTheme(144-150)
🪛 Biome (2.1.2)
frontend/app/account/[address]/members/page.tsx
[error] 324-324: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 325-325: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
frontend/app/account/[address]/AmountChart.tsx
[error] 204-210: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 GitHub Actions: Frontend Lint and Code Formatting Check
frontend/app/account/[address]/members/page.tsx
[error] 324-324: eslint: react-hooks/rules-of-hooks - React Hook 'useState' is called conditionally. React Hooks must be called in the exact same order in every component render.
[error] 325-325: eslint: react-hooks/rules-of-hooks - React Hook 'useEffect' is called conditionally. React Hooks must be called in the exact same order in every component render.
🔇 Additional comments (14)
frontend/app/account/[address]/members/page.tsx (2)
343-359: Skeleton loading UI implementation looks good.The skeleton-based loading pattern with a 5-second minimum display is consistent with similar implementations in other pages (Tabs.tsx, page.tsx, treasury/page.tsx). The grid layout with 6 skeleton cards appropriately represents the members list during loading.
316-321: LGTM: Container styling updates align with layout changes.The simplified container wrappers with
overflow-x-hiddenandtransition-colorsclasses are consistent with the global layout enhancements described in the PR objectives.Also applies to: 363-370, 374-374
frontend/app/account/[address]/AmountChart.tsx (4)
63-66: LGTM: Refs and state additions support new chart and dropdown features.The new refs (
dropdownRef,chartRef) and state flags (isDropdownOpen,isChartReady) are appropriately typed and support the enhanced dropdown interaction and chart rendering lifecycle.
68-80: Chart readiness delay is a reasonable workaround.The 150ms delay before marking the chart ready and calling
resize()addresses common Chart.js container sizing issues. This is a pragmatic solution, though it assumes the container will be sized within that timeframe.
502-531: LGTM: Event handlers properly manage dropdown and chart resizing.The outside-click handler for the dropdown and the window resize handler both follow best practices with proper cleanup. The 100ms resize delay provides basic debouncing to avoid excessive chart redraws.
329-462: LGTM: Chart styling updates create a cleaner, more modern look.The configuration changes (transparent backgrounds, hidden vertical grid lines, reduced tick limits, simplified tooltips, and hidden-by-default points) all contribute to a more minimal and professional chart appearance.
frontend/app/account/[address]/Tabs.tsx (1)
72-98: LGTM: Skeleton timing logic is correctly implemented.The
minSkeletonElapsedstate and effect are properly placed at the top level of the component. The 5-second minimum skeleton display with sessionStorage persistence provides a consistent loading experience and aligns with similar implementations in other pages.frontend/app/account/[address]/page.tsx (4)
16-17: LGTM: New imports and state support balance toggle and skeleton loading.The
EyeandEyeOfficons from lucide-react and theSkeletoncomponent are properly imported. The new state variables (isBalanceVisible,minSkeletonElapsed) are appropriately scoped.Also applies to: 24-25
37-54: Skeleton timing implementation is consistent and correct.The 5-second minimum skeleton display with sessionStorage persistence (key: 'dashboardSkeletonShown') aligns with the pattern used across other pages. The effect is properly positioned at the top level and includes appropriate error handling.
95-97: LGTM: Balance visibility toggle is well-implemented.The eye-icon toggle cleanly switches between showing the balance and a masked placeholder ('•••••'). The implementation includes proper accessibility attributes (aria-label) and follows the PR requirement for hiding/showing the user's balance on the dashboard.
Also applies to: 116-148
231-263: AmountChart skeleton provides appropriate visual feedback.The skeleton placeholder for the chart area with header buttons and grid bars gives users visual feedback during loading. The conditional rendering based on
isTokenDataPendingis clear and consistent with other sections.frontend/app/account/[address]/treasury/page.tsx (3)
13-14: LGTM: NFT modal integration follows established patterns.The
NFTDetailsModalis properly imported, state is initialized, and the modal is wired to both theTabscomponent (viasetNFTModalOpen) and rendered with appropriate props. This mirrors the implementation inpage.tsx.Also applies to: 134-134, 292-292, 305-305
191-211: Skeleton timing logic is correctly implemented.The 5-second minimum skeleton display with sessionStorage persistence (key: 'treasurySkeletonShown') is consistent with the pattern used in other pages. The
isPendingflag appropriately combines bothloadingTokenDataand the minimum skeleton elapsed check.
227-277: LGTM: Skeleton placeholders provide consistent loading feedback across treasury sections.The three main sections (header, stats, portfolio chart) all conditionally render appropriate skeleton placeholders during the pending state. The skeletons approximate the layout and content of the actual components, providing good visual continuity during loading.
| {nfts.length === 0 | ||
| ? Array.from({ length: 8 }).map((_, i) => ( | ||
| <div | ||
| key={i} | ||
| className="rounded-lg overflow-hidden bg-theme-bg-secondary border-2 border-theme-border" | ||
| > | ||
| <Skeleton className="aspect-square w-full bg-theme-bg-secondary" /> | ||
| </div> | ||
| )) | ||
| : nfts.map((nft) => ( | ||
| <div | ||
| key={nft.id} | ||
| className="rounded-lg overflow-hidden bg-theme-bg-secondary border-2 border-theme-border hover:border-theme-border/80 transition-all duration-200" | ||
| onClick={() => setNFTModalOpen(nft.id)} | ||
| > | ||
| <div className="aspect-square relative"> | ||
| <Image | ||
| src={nft.image} | ||
| alt={`NFT ${nft.id}`} | ||
| fill | ||
| className="object-cover" | ||
| /> | ||
| </div> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify: Should skeletons show when there are legitimately no NFTs?
Line 320 renders skeleton placeholders whenever nfts.length === 0. This is appropriate during loading, but if the user truly has no NFTs, showing skeletons could be misleading. Consider whether an empty state message would be more appropriate for zero NFTs after loading completes.
Verify the intended behavior:
If no separate NFT loading state exists and the skeleton should only show during initial load, consider updating the condition:
- {nfts.length === 0
+ {loadingNfts // if a loading flag exists
? Array.from({ length: 8 }).map((_, i) => (
<div key={i} className="...">
<Skeleton className="aspect-square w-full bg-theme-bg-secondary" />
</div>
))
: nfts.map((nft) => (
{/* NFT card */}
))}And add an empty state when !loadingNfts && nfts.length === 0.
🏁 Script executed:
#!/bin/bash
# Check if there's a separate loading state for NFTs that should gate the skeleton display
rg -n "nfts.*loading" --type=ts --type=tsx
rg -n "loadingNft" --type=ts --type=tsxLength of output: 55
🏁 Script executed:
#!/bin/bash
# Correct approach: search TypeScript files (both .ts and .tsx) for loading state patterns
# First, search for any loading-related variables in the file
rg -n "loading|isLoading" frontend/app/account/\[address\]/Tabs.tsx
# Then search for how nfts state is managed
rg -n "useState.*nft|const nfts|setNfts" frontend/app/account/\[address\]/Tabs.tsx -A2 -B2
# Check for any fetch/loading logic related to nfts
rg -n "useEffect|fetch" frontend/app/account/\[address\]/Tabs.tsx -C3 | head -50Length of output: 1342
🏁 Script executed:
#!/bin/bash
# Get context around the NFT rendering section (lines 320-344)
sed -n '310,350p' frontend/app/account/\[address\]/Tabs.tsx
# Also check if there's a separate loadingNFTs state or if loadingTokenData is reused
rg -n "useState\|loadingTokenData\|nfts" frontend/app/account/\[address\]/Tabs.tsx | grep -E "useState|loading|nfts" | head -20Length of output: 1435
Fix NFT section skeleton display logic to distinguish between loading and empty states.
The NFT rendering at line 320 uses a simple nfts.length === 0 check with no corresponding loading state. Since the NFTs appear to be static/imported data (not dynamically fetched), this means skeletons will display indefinitely whenever the array is empty, providing misleading UX. Unlike the Tokens section which uses loadingTokenData with proper timing logic, the NFT section needs either:
- A dedicated loading state to gate skeleton display, OR
- An empty state message when genuinely no NFTs exist
Add an empty state condition after the loading phase completes, or implement a loading flag for the NFT section similar to how tokens are handled.
🤖 Prompt for AI Agents
In frontend/app/account/[address]/Tabs.tsx around lines 320 to 344, the NFT grid
currently shows skeletons whenever nfts.length === 0 which conflates loading
with an actually-empty collection; add a proper loading flag (e.g.,
loadingNftData) or reuse the existing tokens loading state, then change the
conditional so: if loadingNftData show skeleton placeholders, else if
nfts.length === 0 render a clear empty-state message/UI, else map nfts to the
existing tiles; ensure the onClick/modal logic and keys remain unchanged and the
new loading flag is set while fetching or initialising NFT data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/account/[address]/members/page.tsx (1)
344-360: Consider the UX impact of the 5-second minimum skeleton display.The condition
isLoading || !minSkeletonElapsedensures the skeleton shows for at least 5 seconds on the first session load, even if data loads more quickly. While this prevents skeleton flicker, it might make the app feel slower than necessary if the data loads in 1-2 seconds.Consider using
isLoading && !minSkeletonElapsedto show real data as soon as it's available, or add a flag to skip the delay if data arrives early. However, the current pattern is valid if the goal is to provide a consistent loading experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/account/[address]/members/page.tsx(4 hunks)
🔇 Additional comments (1)
frontend/app/account/[address]/members/page.tsx (1)
168-186: Critical issue resolved: Hooks are now correctly positioned.The hooks are now placed before the early return at line 334, resolving the critical Rules of Hooks violation flagged in the previous review. The implementation properly handles sessionStorage errors with try-catch blocks and includes appropriate cleanup for the timeout.
1 participant
Description:
This PR introduces a global layout enhancement to improve user experience on ultra-wide and 4K displays.
Key updates include:
Added centered layout constraints for all main and subpages.
Closes #183 Closes #185 Closes #186 Closes #187
Summary by CodeRabbit
New Features
Bug Fixes / Enhancements
Style / Layout