feat: loading states and error handling #200 #211
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive loading, error, and empty state handling across the application. It adds new UI components for displaying these states, implements a retry hook for API resilience, updates the PropertyGrid component to handle multiple data states, and enhances error recovery in the dashboard hook. Changes
Sequence Diagram(s)sequenceDiagram
participant PropertyGrid
participant useApiCall Hook
participant API
participant UI Components
PropertyGrid->>useApiCall Hook: execute(fetchProperties)
Note over useApiCall Hook: Loading = true
PropertyGrid->>UI Components: render LoadingGrid
useApiCall Hook->>API: fetch properties
API-->>useApiCall Hook: success or error
alt Success
Note over useApiCall Hook: isLoading = false, data = properties
PropertyGrid->>UI Components: render PropertyGrid with data
else Error
Note over useApiCall Hook: isLoading = false, error = message
PropertyGrid->>UI Components: render ErrorDisplay with onRetry
PropertyGrid-->>useApiCall Hook: onRetry callback triggered
useApiCall Hook->>API: retry fetch
end
alt Empty State
Note over PropertyGrid: displayProperties.length === 0
PropertyGrid->>UI Components: render empty state message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@apps/web/src/components/ui/loading-skeleton.tsx`:
- Around line 43-49: LoadingGrid uses a dynamic Tailwind class
xl:grid-cols-${columns} (which will be purged) and unstable keys generated by
crypto.randomUUID() (causing remounts); replace the dynamic class with a mapping
from allowed columns to static class names (e.g., map 1..6 to
"xl:grid-cols-1".."xl:grid-cols-6" and default to a safe value) and use stable
keys for the skeletons by iterating with Array.from({ length: count }).map((_,
idx) => ...) and keying PropertyCardSkeleton with a stable identifier like
`skeleton-${idx}`; update the LoadingGrid component to compute the static
colClass and use it in the container className and remove crypto.randomUUID()
usage.
🧹 Nitpick comments (3)
apps/web/src/hooks/useDashboard.ts (1)
180-226: Consider reusing bookings data to avoid duplicate/api/bookingsretries.
refreshAlltriggers both bookings and stats fetches, which can double the booking calls (and retries). If bookings are already present, consider deriving stats from state or passing the fresh bookings payload intofetchStats.apps/web/src/hooks/useApiCall.ts (2)
3-8: Improve type safety for callback parameters.The
onSuccesscallback acceptsunknown, but it receives data of typeT. Consider making the interface generic to provide proper type inference for consumers.Suggested improvement
-interface UseApiCallOptions { +interface UseApiCallOptions<T> { retryCount?: number; retryDelay?: number; - onSuccess?: (data: unknown) => void; + onSuccess?: (data: T) => void; onError?: (error: Error) => void; }Then update the hook signature:
export function useApiCall<T>( apiFunction: (...args: unknown[]) => Promise<T>, - options: UseApiCallOptions = {} + options: UseApiCallOptions<T> = {} ): UseApiCallReturn<T> {
97-118: Consider standardizing parameter naming with the hook.The utility uses
maxRetrieswhileuseApiCallusesretryCount. Both correctly represent the number of retry attempts (not total attempts), but the inconsistent naming could confuse consumers.Suggested change for consistency
export async function retryApiCall<T>( apiFunction: () => Promise<T>, - maxRetries = 3, + retryCount = 3, delay = 1000 ): Promise<T> { let lastError: Error = new Error('API call failed'); - for (let attempt = 0; attempt <= maxRetries; attempt++) { + for (let attempt = 0; attempt <= retryCount; attempt++) { try { return await apiFunction(); } catch (error) { lastError = error instanceof Error ? error : new Error('Unknown error'); - if (attempt < maxRetries) { - console.log(`Retry attempt ${attempt + 1} of ${maxRetries}...`); + if (attempt < retryCount) { + console.log(`Retry attempt ${attempt + 1} of ${retryCount}...`); await new Promise((resolve) => setTimeout(resolve, delay)); } } } throw lastError; }
| export const LoadingGrid = ({ count = 8, columns = 4 }: LoadingGridProps) => { | ||
| return ( | ||
| <div className={`grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 xl:grid-cols-${columns} gap-8`}> | ||
| {Array.from({ length: count }, () => crypto.randomUUID()).map((id) => ( | ||
| <PropertyCardSkeleton key={id} /> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Tailwind configuration for safelists and grid-cols classes
find . -name 'tailwind.config.*' -o -name 'tailwind.config.ts' -o -name 'tailwind.config.js' | head -20Repository: Stellar-Rent/stellar-rent
Length of output: 98
🏁 Script executed:
# Search for Tailwind config files
fd -n 'tailwind.config\.' .Repository: Stellar-Rent/stellar-rent
Length of output: 299
🏁 Script executed:
# Look for any existing safelist configurations
rg -n 'safelist' . -t ts -t js -t tsx -t jsxRepository: Stellar-Rent/stellar-rent
Length of output: 96
🏁 Script executed:
# Check for xl:grid-cols usage in the codebase
rg -n 'xl:grid-cols' apps/web/src -g'*.{ts,tsx,js,jsx}'Repository: Stellar-Rent/stellar-rent
Length of output: 650
🏁 Script executed:
# Examine the loading-skeleton file context
cat -n apps/web/src/components/ui/loading-skeleton.tsxRepository: Stellar-Rent/stellar-rent
Length of output: 2967
🏁 Script executed:
# Read the Tailwind configuration to check for safelist and content settings
cat -n apps/web/tailwind.config.jsRepository: Stellar-Rent/stellar-rent
Length of output: 2776
🏁 Script executed:
# Check if there are any other safelist configurations in the codebase
rg -i 'safelist|content.*grid-cols' . -t jsRepository: Stellar-Rent/stellar-rent
Length of output: 51
Fix dynamic Tailwind class and unstable keys in LoadingGrid.
The dynamic class xl:grid-cols-${columns} will be purged by Tailwind in production builds since the Tailwind config has no safelist and relies on static content scanning. Additionally, crypto.randomUUID() regenerates keys on every render, causing unnecessary remounts of PropertyCardSkeleton.
Map columns to static class names and use stable indices for skeleton keys:
♻️ Refactor
export const LoadingGrid = ({ count = 8, columns = 4 }: LoadingGridProps) => {
+ const columnClasses: Record<number, string> = {
+ 1: 'xl:grid-cols-1',
+ 2: 'xl:grid-cols-2',
+ 3: 'xl:grid-cols-3',
+ 4: 'xl:grid-cols-4',
+ 5: 'xl:grid-cols-5',
+ 6: 'xl:grid-cols-6',
+ };
+ const xlColumnsClass = columnClasses[columns] ?? 'xl:grid-cols-4';
+
return (
- <div className={`grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 xl:grid-cols-${columns} gap-8`}>
- {Array.from({ length: count }, () => crypto.randomUUID()).map((id) => (
- <PropertyCardSkeleton key={id} />
+ <div className={`grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 ${xlColumnsClass} gap-8`}>
+ {Array.from({ length: count }).map((_, idx) => (
+ <PropertyCardSkeleton key={idx} />
))}
</div>
);
};📝 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.
| export const LoadingGrid = ({ count = 8, columns = 4 }: LoadingGridProps) => { | |
| return ( | |
| <div className={`grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 xl:grid-cols-${columns} gap-8`}> | |
| {Array.from({ length: count }, () => crypto.randomUUID()).map((id) => ( | |
| <PropertyCardSkeleton key={id} /> | |
| ))} | |
| </div> | |
| export const LoadingGrid = ({ count = 8, columns = 4 }: LoadingGridProps) => { | |
| const columnClasses: Record<number, string> = { | |
| 1: 'xl:grid-cols-1', | |
| 2: 'xl:grid-cols-2', | |
| 3: 'xl:grid-cols-3', | |
| 4: 'xl:grid-cols-4', | |
| 5: 'xl:grid-cols-5', | |
| 6: 'xl:grid-cols-6', | |
| }; | |
| const xlColumnsClass = columnClasses[columns] ?? 'xl:grid-cols-4'; | |
| return ( | |
| <div className={`grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 ${xlColumnsClass} gap-8`}> | |
| {Array.from({ length: count }).map((_, idx) => ( | |
| <PropertyCardSkeleton key={idx} /> | |
| ))} | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In `@apps/web/src/components/ui/loading-skeleton.tsx` around lines 43 - 49,
LoadingGrid uses a dynamic Tailwind class xl:grid-cols-${columns} (which will be
purged) and unstable keys generated by crypto.randomUUID() (causing remounts);
replace the dynamic class with a mapping from allowed columns to static class
names (e.g., map 1..6 to "xl:grid-cols-1".."xl:grid-cols-6" and default to a
safe value) and use stable keys for the skeletons by iterating with Array.from({
length: count }).map((_, idx) => ...) and keying PropertyCardSkeleton with a
stable identifier like `skeleton-${idx}`; update the LoadingGrid component to
compute the static colClass and use it in the container className and remove
crypto.randomUUID() usage.
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)
apps/web/src/components/search/PropertyGrid.tsx (1)
203-224: Potential duplicate keys when parsing string IDs fails.On line 206, if
Number.parseInt(property.id, 10)returnsNaNfor multiple properties, they'll all default toid: 0, causing duplicate React keys on line 223. Consider preserving the original string id or generating a unique fallback.🐛 Suggested fix
- id: typeof property.id === 'string' ? Number.parseInt(property.id, 10) || 0 : property.id, + id: typeof property.id === 'string' ? Number.parseInt(property.id, 10) || property.id : property.id,Alternatively, use
property.iddirectly as the key since it's already unique:- return <PropertyCard key={property.id} property={cardProperty} />; + return <PropertyCard key={String(property.id)} property={cardProperty} />;
🧹 Nitpick comments (2)
apps/web/src/components/search/PropertyGrid.tsx (2)
190-198: Minor: Redundant null check.The
!displayPropertiescheck is unnecessary sincedisplayPropertiesis guaranteed to be defined after line 188 (it either holdspropertiesormockProperties). ThedisplayProperties.length === 0check alone suffices.♻️ Suggested simplification
- if (!displayProperties || displayProperties.length === 0) { + if (displayProperties.length === 0) {
221-221: Consider defaultingverifiedtofalseinstead oftrue.Defaulting
verifiedtotruewhen undefined could misleadingly display unverified properties with a verified badge. A safer default would befalseto avoid misrepresenting property verification status.♻️ Suggested change
- verified: property.verified !== undefined ? property.verified : true, + verified: property.verified ?? false,
1 participant
closes #[200]
Feature Implemented:
Loading States and Error Handling
I Implemente comprehensive loading states and error handling to improve UX during API calls and long-running operations.
Added
Error Components
ErrorDisplay and InlineError with retry functionality
Loading Components - Skeleton loaders, spinners, and loading grids
Retry Logic - Hook and utility for automatic API retry (3 attempts, 1s delay)
Enhanced useDashboard - Added retry logic to all API calls with better error handling
PropertyGrid - Added loading and error states with retry support
Providers - Replaced text loading with Spinner component
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.