feat: added loading spinner on /dashboard when filtering #1090
Conversation
WalkthroughAdds an internal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pages/dashboard/index.tsx (1)
120-141: Critical: Loading state not reset on error; Major: Race condition with rapid filter changes.Two issues need addressing:
Error handling gap: If the fetch fails,
setIsLoading(false)is never called (line 136 only logs the error), leaving the loading spinner visible indefinitely and blocking the user.Race condition: Rapid filter changes trigger multiple concurrent requests without cancellation. If an earlier request completes after a later one, stale data overwrites current filtered results.
🔎 Proposed fix using AbortController
useEffect(() => { + const controller = new AbortController() const fetchData = async (): Promise<void> => { - setIsLoading(true) - let url = 'api/dashboard' - if (selectedButtonIds.length > 0) { - url += `?buttonIds=${selectedButtonIds.join(',')}` - } - const res = await fetch(url, { - headers: { - Timezone: moment.tz.guess() - } - }) - const json = await res.json() - setDashboardData(json) - setIsLoading(false) + try { + setIsLoading(true) + let url = 'api/dashboard' + if (selectedButtonIds.length > 0) { + url += `?buttonIds=${selectedButtonIds.join(',')}` + } + const res = await fetch(url, { + headers: { + Timezone: moment.tz.guess() + }, + signal: controller.signal + }) + const json = await res.json() + setDashboardData(json) + } catch (error: any) { + if (error.name !== 'AbortError') { + console.error(error) + } + } finally { + setIsLoading(false) + } } - fetchData().catch(console.error) + void fetchData() const savedActivePeriodString = loadStateFromCookie(COOKIE_NAMES.DASHBOARD_FILTER, undefined) as (PeriodString | undefined) if (savedActivePeriodString !== undefined) { setActivePeriodString(savedActivePeriodString) } + return () => { + controller.abort() + } }, [selectedButtonIds])
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/dashboard/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pages/dashboard/index.tsx (1)
redis/index.ts (1)
dashboardData(111-115)
⏰ 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 (2)
pages/dashboard/index.tsx (2)
80-80: LGTM! Clean loading state addition.The
isLoadingstate is properly typed and initialized, following React best practices.
155-155: Loading condition logic is correct.The conditional properly includes
isLoadingto show the loading spinner during data fetches. The logic correctly handles the initial mount (undefined data) and subsequent filter changes.
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)
pages/dashboard/index.tsx (1)
121-136: Critical: Loading state not reset on fetch error.If the fetch fails or JSON parsing throws an error, the catch handler on line 136 logs the error but doesn't call
setIsLoading(false). This leaves the loading spinner visible indefinitely, blocking the user from interacting with the dashboard.🔎 Recommended fix using try-finally
useEffect(() => { const fetchData = async (): Promise<void> => { setIsLoading(true) - let url = 'api/dashboard' - if (selectedButtonIds.length > 0) { - url += `?buttonIds=${selectedButtonIds.join(',')}` + try { + let url = 'api/dashboard' + if (selectedButtonIds.length > 0) { + url += `?buttonIds=${selectedButtonIds.join(',')}` + } + const res = await fetch(url, { + headers: { + Timezone: moment.tz.guess() + } + }) + const json = await res.json() + setDashboardData(json) + } catch (error) { + console.error(error) + } finally { + setIsLoading(false) } - const res = await fetch(url, { - headers: { - Timezone: moment.tz.guess() - } - }) - const json = await res.json() - setDashboardData(json) - setIsLoading(false) } - fetchData().catch(console.error) + void fetchData() const savedActivePeriodString = loadStateFromCookie(COOKIE_NAMES.DASHBOARD_FILTER, undefined) as (PeriodString | undefined) if (savedActivePeriodString !== undefined) { setActivePeriodString(savedActivePeriodString)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/Loading/index.tsx(1 hunks)components/Loading/loading.module.css(1 hunks)pages/dashboard/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pages/dashboard/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 (4)
components/Loading/index.tsx (1)
23-23: LGTM! CSS class rename aligns with the new overlay approach.The change from
loading_containertoloading_overlaycorrectly reflects the updated CSS implementation.pages/dashboard/index.tsx (3)
80-80: LGTM! Appropriate state for tracking loading status.The
isLoadingstate is properly initialized and will control the Loading component visibility.
158-158: Good defensive coding with optional chaining.Adding optional chaining for
user?.stUser?.emailprevents potential runtime errors if the user object structure is incomplete.Also applies to: 166-166
167-167: LGTM! Loading spinner correctly shown during filtering.The conditional rendering of the Loading component when
isLoadingis true properly implements the PR objective to show a loading spinner during filter operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/Loading/loading.module.css (1)
1-13: LGTM! Previous issues resolved.Both major concerns from the previous review have been properly addressed:
- The semi-transparent backdrop (
rgba(0, 0, 0, 0.4)) now prevents interaction with underlying content and improves spinner visibility- The z-index conflict is resolved using a CSS custom property set to 999999, ensuring proper stacking above other components
The implementation follows best practices for fullscreen loading overlays.
Optional enhancement: cursor feedback
Consider adding
cursor: wait;to provide visual feedback that the UI is processing:.loading_overlay { --z-overlay-loading: 999999; position: fixed; top: 0; left: 0; right: 0; bottom: 0; display: flex; justify-content: center; align-items: center; background-color: rgba(0, 0, 0, 0.4); z-index: var(--z-overlay-loading); + cursor: wait; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Loading/loading.module.css(1 hunks)
⏰ 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
Test plan
Go to /dashboard and change the current button filter to see the loading spinner appear.
Summary by CodeRabbit
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.