feat: main pages loading spinner #1083
Conversation
WalkthroughThis PR introduces a new Loading component using the react-spinners library and integrates it across seven pages to display loading indicators while fetching data. A CSS module for styling and the react-spinners dependency are added. The TopBar component is updated to display the current date formatted as "Month Day, Year". Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pages/payments/index.tsx (1)
171-184: Add error handling to prevent infinite loading state.If
fetchPaybuttonsorfetchTransactionYearsthrows an error,setPageLoading(false)at line 183 will never execute, leaving users stuck on the loading screen indefinitely with no error message or recovery path.Apply this diff to add proper error handling:
const getDataAndSetUpCurrencyCSV = async (): Promise<void> => { - const paybuttons = await fetchPaybuttons() - const years = await fetchTransactionYears() - const networkIds: Set<number> = new Set() - setButtons(paybuttons) - - paybuttons.forEach((p: { addresses: any[] }) => { - return p.addresses.forEach((c: { address: { networkId: number } }) => networkIds.add(c.address.networkId)) - }) - - setPaybuttonNetworks(networkIds) - setTransactionYears(years) - setPageLoading(false) + try { + const paybuttons = await fetchPaybuttons() + const years = await fetchTransactionYears() + const networkIds: Set<number> = new Set() + setButtons(paybuttons) + + paybuttons.forEach((p: { addresses: any[] }) => { + return p.addresses.forEach((c: { address: { networkId: number } }) => networkIds.add(c.address.networkId)) + }) + + setPaybuttonNetworks(networkIds) + setTransactionYears(years) + } catch (error) { + console.error('Error loading payment data:', error) + // Optionally show error UI to user + } finally { + setPageLoading(false) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
components/Loading/index.tsx(1 hunks)components/Loading/loading.module.css(1 hunks)components/TopBar/index.tsx(2 hunks)package.json(1 hunks)pages/admin/index.tsx(3 hunks)pages/button/[id].tsx(2 hunks)pages/buttons/index.tsx(3 hunks)pages/dashboard/index.tsx(2 hunks)pages/networks/index.tsx(2 hunks)pages/payments/index.tsx(4 hunks)pages/wallets/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
pages/button/[id].tsx (1)
components/Loading/index.tsx (1)
Loading(10-31)
pages/admin/index.tsx (1)
components/Loading/index.tsx (1)
Loading(10-31)
pages/dashboard/index.tsx (3)
redis/index.ts (1)
dashboardData(111-115)components/TopBar/index.tsx (1)
TopBar(10-36)components/Loading/index.tsx (1)
Loading(10-31)
pages/networks/index.tsx (2)
components/TopBar/index.tsx (1)
TopBar(10-36)components/Loading/index.tsx (1)
Loading(10-31)
pages/buttons/index.tsx (2)
components/TopBar/index.tsx (1)
TopBar(10-36)components/Loading/index.tsx (1)
Loading(10-31)
pages/wallets/index.tsx (2)
components/TopBar/index.tsx (1)
TopBar(10-36)components/Loading/index.tsx (1)
Loading(10-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (20)
components/Loading/loading.module.css (1)
1-7: LGTM!The CSS module provides appropriate styling for centering the loading indicator with reasonable spacing.
pages/dashboard/index.tsx (2)
18-18: LGTM!The Loading component import is appropriate for this page's loading state.
152-159: LGTM!The loading state logic is correct, showing the spinner while data is being fetched and displaying the TopBar for UI consistency.
pages/admin/index.tsx (2)
16-16: LGTM!The Loading component import is appropriate for this page.
61-61: LGTM!The loading state management is correct, initializing as
trueand setting tofalseafter the fetch completes, providing a good user experience during data loading.Also applies to: 73-73, 78-85
pages/button/[id].tsx (2)
20-20: LGTM!The Loading component import is appropriate for this page.
216-218: LGTM!The loading state logic correctly displays the spinner while the paybutton data is being fetched.
pages/networks/index.tsx (1)
12-12: LGTM!The Loading component import is appropriate.
pages/buttons/index.tsx (2)
13-13: LGTM!The Loading component import is appropriate for this page.
51-51: LGTM!The loading state management is well-implemented, correctly waiting for both data fetches to complete before displaying content. This provides a smooth user experience and follows the pattern established across other pages in this PR.
Also applies to: 58-58, 64-64, 119-126
package.json (1)
64-64: No action required.react-spinners@^0.14.1has no known security vulnerabilities and is a valid, non-deprecated package version.pages/payments/index.tsx (3)
29-29: LGTM!The Loading component import is correctly structured.
80-80: LGTM!The pageLoading state is correctly initialized to true, ensuring the loading UI appears immediately on mount.
480-487: LGTM!The loading UI implementation is correct. The early return pattern ensures the Loading component is displayed while data is being fetched, and the TopBar remains visible for consistency.
pages/wallets/index.tsx (3)
14-14: LGTM!The Loading component import is correctly structured.
52-52: LGTM!The loading state is correctly added to the interface and initialized to true in the constructor, ensuring the loading UI is displayed immediately.
Also applies to: 61-62
109-116: LGTM!The loading UI implementation correctly uses an early return to display the Loading component while data is being fetched, maintaining TopBar visibility for consistency.
components/Loading/index.tsx (3)
1-8: LGTM!The imports and LoadingProps interface are well-structured with appropriate optional properties.
10-11: LGTM!The function signature with default size and the state initialization with fallback color are well-implemented.
13-31: LGTM with minor note!The useEffect correctly reads the CSS variable on the client side, and the DotLoader is properly configured. There may be a brief flash of the default color (#0ac18e) before the CSS variable is applied on first render, but this is a negligible UX issue.
| @@ -1,4 +1,5 @@ | |||
| import Link from 'next/link' | |||
| import { useState, useEffect } from 'react' | |||
There was a problem hiding this comment.
Reconsider the date refactoring to avoid initial render flash.
This refactor introduces dateString state initialized as an empty string, which causes a brief flash of empty content before the useEffect populates it. The date calculation could be done directly without state, or the initial state could be set to a default value.
Additionally, this change appears unrelated to the loading spinner feature and introduces unnecessary complexity.
Consider one of these solutions:
Solution 1 (preferred): Compute date directly without state
-import { useState, useEffect } from 'react'
+import React from 'react'
import style from './topbar.module.css'
interface TopBarProps {
title: string
user?: string
}
export default function TopBar ({ title, user }: TopBarProps): JSX.Element {
- const [dateString, setDateString] = useState<string>('')
-
- useEffect(() => {
- const currentDate = new Date()
- const month = currentDate.toLocaleString('en-US', { month: 'long' })
- const day = currentDate.getDate()
- const year = currentDate.getFullYear()
- setDateString(`${month} ${day}, ${year}`)
- }, [])
+ const currentDate = new Date()
+ const month = currentDate.toLocaleString('en-US', { month: 'long' })
+ const day = currentDate.getDate()
+ const year = currentDate.getFullYear()
+ const dateString = `${month} ${day}, ${year}`
return (Solution 2: Initialize with a computed value to prevent flash
export default function TopBar ({ title, user }: TopBarProps): JSX.Element {
- const [dateString, setDateString] = useState<string>('')
+ const [dateString, setDateString] = useState<string>(() => {
+ const currentDate = new Date()
+ const month = currentDate.toLocaleString('en-US', { month: 'long' })
+ const day = currentDate.getDate()
+ const year = currentDate.getFullYear()
+ return `${month} ${day}, ${year}`
+ })
- useEffect(() => {
- const currentDate = new Date()
- const month = currentDate.toLocaleString('en-US', { month: 'long' })
- const day = currentDate.getDate()
- const year = currentDate.getFullYear()
- setDateString(`${month} ${day}, ${year}`)
- }, [])Also applies to: 11-19, 26-26
🤖 Prompt for AI Agents
In components/TopBar/index.tsx around lines 2 and also lines 11-19 and 26, the
refactor introduced dateString state initialized to an empty string causing a
flash; remove the unnecessary state and useEffect and compute the formatted date
inline (or if you must keep state, initialize it with the computed date value
rather than an empty string) so the component renders the date synchronously
without a blank flash; also drop unrelated changes to keep this file focused on
the loading spinner feature.
| if (this.state.networks.length === 0) { | ||
| return ( | ||
| <> | ||
| <TopBar title="Networks" user={this.props.user.stUser?.email} /> | ||
| <NetworkList networks={this.state.networks} userNetworks={this.state.userNetworks} /> | ||
| <Loading /> | ||
| </> | ||
| ) | ||
| } | ||
| return ( | ||
| <> | ||
| <TopBar title="Networks" user={this.props.user.stUser?.email} /> | ||
| <NetworkList networks={this.state.networks} userNetworks={this.state.userNetworks} /> | ||
| </> | ||
| ) |
There was a problem hiding this comment.
Fix the loading condition to avoid conflating empty state with loading state.
Using networks.length === 0 to determine loading state will cause the spinner to display indefinitely when a user legitimately has no networks. This conflates the initial loading state with an empty data state.
Add a dedicated loading boolean state similar to the pattern used in pages/buttons/index.tsx and pages/admin/index.tsx:
interface NetworksState {
networks: Network[]
userNetworks: UserNetworksInfo[]
+ loading: boolean
}
export default class Networks extends React.Component<NetworksProps, NetworksState> {
constructor (props: NetworksProps) {
super(props)
this.props = props
this.state = {
networks: [],
- userNetworks: []
+ userNetworks: [],
+ loading: true
}
}
async componentDidMount (): Promise<void> {
await this.fetchNetworks()
+ this.setState({ loading: false })
}Then update the render condition:
render (): React.ReactElement {
- if (this.state.networks.length === 0) {
+ if (this.state.loading) {
return (
<>
<TopBar title="Networks" user={this.props.user.stUser?.email} />
<Loading />
</>
)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pages/networks/index.tsx around lines 85-98, the component currently uses
this.state.networks.length === 0 to decide to show the Loading spinner, which
mixes initial loading with a legitimate empty-networks state; add a boolean
loading state (e.g. this.state.loading = true initially), set loading = false
after the async fetch completes (in both success and error paths), and update
the render logic to show <Loading /> only when loading === true, otherwise
render the TopBar and NetworkList (or an explicit empty-state message) so an
empty networks array does not display the spinner indefinitely.
| async componentDidMount (): Promise<void> { | ||
| await this.fetchWallets() | ||
| await this.fetchNetworks() | ||
| this.setState({ loading: false }) | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent infinite loading state.
If fetchWallets or fetchNetworks throws an error, setState({ loading: false }) at line 69 will never execute, leaving users stuck on the loading screen indefinitely.
Apply this diff to add proper error handling:
async componentDidMount (): Promise<void> {
- await this.fetchWallets()
- await this.fetchNetworks()
- this.setState({ loading: false })
+ try {
+ await this.fetchWallets()
+ await this.fetchNetworks()
+ } catch (error) {
+ console.error('Error loading wallet data:', error)
+ // Optionally show error UI to user
+ } finally {
+ this.setState({ loading: false })
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async componentDidMount (): Promise<void> { | |
| await this.fetchWallets() | |
| await this.fetchNetworks() | |
| this.setState({ loading: false }) | |
| } | |
| async componentDidMount (): Promise<void> { | |
| try { | |
| await this.fetchWallets() | |
| await this.fetchNetworks() | |
| } catch (error) { | |
| console.error('Error loading wallet data:', error) | |
| // Optionally show error UI to user | |
| } finally { | |
| this.setState({ loading: false }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In pages/wallets/index.tsx around lines 66 to 70, wrap the async
componentDidMount sequence in a try/catch/finally so that any exception from
fetchWallets or fetchNetworks is caught and loading is always set to false; in
the catch block set an error state or log the error (e.g., setState({ error:
errorMessage })) and in the finally call setState({ loading: false }) to ensure
the UI exits the loading state even on failure.
Related to #1082
Description
Adds loading spinner/animation to the various pages.
Test plan
Visit the various pages (eg. /dashboard) and see how there's now a loading animation
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.