-
Notifications
You must be signed in to change notification settings - Fork 543
[Dashboard] Fix: Async team analytics rendering #7289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe analytics dashboard was refactored to split a monolithic component into multiple Suspense-wrapped async components, each responsible for fetching and rendering a specific analytics card. Data fetching now uses Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TeamOverviewPage
participant AsyncAppHighlightsCard
participant AsyncWalletDistributionCard
participant AsyncAuthMethodDistributionCard
participant AsyncTransactionsChartCard
participant AsyncTotalSponsoredCard
participant DataSources
User->>TeamOverviewPage: Load page
TeamOverviewPage->>AsyncAppHighlightsCard: Render (Suspense)
AsyncAppHighlightsCard->>DataSources: Fetch wallet user stats & bridge usage
TeamOverviewPage->>AsyncWalletDistributionCard: Render (Suspense)
AsyncWalletDistributionCard->>DataSources: Fetch wallet connections
TeamOverviewPage->>AsyncAuthMethodDistributionCard: Render (Suspense)
AsyncAuthMethodDistributionCard->>DataSources: Fetch in-app wallet usage
TeamOverviewPage->>AsyncTransactionsChartCard: Render (Suspense)
AsyncTransactionsChartCard->>DataSources: Fetch transactions data
TeamOverviewPage->>AsyncTotalSponsoredCard: Render (Suspense)
AsyncTotalSponsoredCard->>DataSources: Fetch sponsored operations data
Note over TeamOverviewPage: Each card renders chart or empty state based on fetched data
Possibly related PRs
Suggested labels
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7289 +/- ##
=======================================
Coverage 55.58% 55.58%
=======================================
Files 909 909
Lines 58670 58670
Branches 4158 4158
=======================================
Hits 32609 32609
Misses 25954 25954
Partials 107 107
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/src/app/(app)/team/[team_slug]/(team)/~/analytics/page.tsx (1)
315-327
: Same date comparison improvement with identical duplication issue.As suggested in the project analytics file, consider extracting the common filter predicate to reduce duplication.
🧹 Nitpick comments (1)
apps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/(sidebar)/page.tsx (1)
233-246
: Date comparison fix looks good, but consider reducing duplication.The ISO string conversion ensures consistent date matching across different formats and timezones. However, the filtering logic is duplicated.
Consider extracting the common filter predicate:
+ const isCompletedOnDate = (v: UniversalBridgeStats, targetDate: string) => + new Date(v.date).toISOString() === new Date(targetDate).toISOString() && + v.status === "completed"; const volume = volumeStats - .filter( - (v) => - new Date(v.date).toISOString() === - new Date(stat.date).toISOString() && v.status === "completed", - ) + .filter((v) => isCompletedOnDate(v, stat.date)) .reduce((acc, curr) => acc + curr.amountUsdCents / 100, 0); const fees = volumeStats - .filter( - (v) => - new Date(v.date).toISOString() === - new Date(stat.date).toISOString() && v.status === "completed", - ) + .filter((v) => isCompletedOnDate(v, stat.date)) .reduce((acc, curr) => acc + curr.developerFeeUsdCents / 100, 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/src/app/(app)/team/[team_slug]/(team)/~/analytics/page.tsx
(3 hunks)apps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/(sidebar)/page.tsx
(2 hunks)apps/dashboard/src/app/(app)/team/components/Analytics/Stat.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/dashboard/src/app/(app)/team/components/Analytics/Stat.tsx (1)
15-15
: Good defensive check against infinite values.Adding the infinity check prevents displaying meaningless "Infinity%" values in the trend badge, improving the user experience.
apps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/(sidebar)/page.tsx (1)
349-352
: Simplified rendering logic is more consistent.Removing the non-zero user check allows the highlights card to render even with zero activity, which provides better UI consistency and prevents false empty states.
apps/dashboard/src/app/(app)/team/[team_slug]/(team)/~/analytics/page.tsx (2)
79-124
: Excellent refactoring to improve modularity and error handling.Breaking down the monolithic component into smaller, Suspense-wrapped async components provides:
- Better error isolation with
Promise.allSettled
- Granular loading states for improved UX
- Parallel data fetching for better performance
- Enhanced maintainability
153-157
:❓ Verification inconclusive
Inconsistent rendering condition compared to project analytics.
This component checks for non-zero
totalUsers
before rendering, while the same component inapps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/(sidebar)/page.tsx
had this check removed. This inconsistency could lead to different behaviors between team and project analytics pages.Should the rendering conditions be consistent across both analytics pages? If so, consider removing the
totalUsers
check here as well:if ( walletUserStatsTimeSeries.status === "fulfilled" && - universalBridgeUsage.status === "fulfilled" && - walletUserStatsTimeSeries.value.some((w) => w.totalUsers !== 0) + universalBridgeUsage.status === "fulfilled" ) {Run this script to check for other instances of this pattern:
🏁 Script executed:
#!/bin/bash # Description: Find all instances of AsyncAppHighlightsCard to verify rendering conditions # Search for AsyncAppHighlightsCard component definitions and usages ast-grep --pattern 'AsyncAppHighlightsCard' # Look for totalUsers checks in analytics components rg -A 5 'totalUsers !== 0|totalUsers === 0' --glob '**/analytics/**'Length of output: 1236
Inconsistent rendering logic between team and project analytics pages
The Team analytics page (
apps/dashboard/src/app/(app)/team/[team_slug]/(team)/~/analytics/page.tsx
lines 153–157) still guards on non-zerototalUsers
, whereas the Project analytics page (apps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/(sidebar)/page.tsx
) has removed that check. This divergence can cause confusing UX and maintenance drift.• Team page condition:
if ( walletUserStatsTimeSeries.status === "fulfilled" && universalBridgeUsage.status === "fulfilled" && walletUserStatsTimeSeries.value.some((w) => w.totalUsers !== 0) ) { … }• Project page condition:
if ( walletUserStatsTimeSeries.status === "fulfilled" && universalBridgeUsage.status === "fulfilled" ) { … }If the intention is to align both views, remove the
totalUsers
filter on the Team page:if ( walletUserStatsTimeSeries.status === "fulfilled" && - universalBridgeUsage.status === "fulfilled" && - walletUserStatsTimeSeries.value.some((w) => w.totalUsers !== 0) + universalBridgeUsage.status === "fulfilled" ) {Please verify whether filtering out zero-user series was intentionally dropped in the project view or if it should be removed here as well. To find other occurrences, you can run:
#!/bin/bash # Search for totalUsers checks in analytics pages rg -n 'walletUserStatsTimeSeries.value.some.*totalUsers' --glob '**/analytics/**'
size-limit report 📦
|
PR-Codex overview
This PR focuses on improving the analytics components in the dashboard by refining data handling, enhancing user experience with loading states, and restructuring the code for better readability and maintainability.
Detailed summary
trend
inStat.tsx
.page.tsx
filters.AsyncAppHighlightsCard
.LoadingChartState
.OverviewPageContent
into multiple async components for better modularity.Promise.allSettled
.Summary by CodeRabbit