Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: dad1437 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Duplicate code analysis3 new clones introduced by this PR.
If one of these duplicates a helper that already exists, import it instead jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ) : graphResults.length === 0 ? ( | ||
| <div className="d-flex h-100 w-100 align-items-center justify-content-center text-muted"> | ||
| No data found within time range. | ||
| {isLoadingOrPlaceholder && <Loader size="xs" mr="xs" />} |
There was a problem hiding this comment.
🟠 major — Time chart can get stuck on "Loading Chart Data..." if the previous-period query fails
The empty branch now keys off isLoadingOrPlaceholder (line 610), and that flag includes !data?.isComplete || (config.compareToPreviousPeriod && !previousPeriodData?.isComplete). If compare-to-previous is on, the current period comes back empty, and the previous-period query errors, previousPeriodData stays undefined. The tile then shows a spinner forever, where it used to show "No data found". Base this branch on whether a fetch is actually in flight (isLoading || isPlaceholderData || isPreviousPeriodLoading), or treat a previous-period error as finished. Also add a DBTimeChart test for this branch: none of the PR's tests cover it.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| No data found within time range. | ||
| <div className="d-flex h-100 w-100 align-items-center justify-content-center text-muted"> | ||
| {isPlaceholderData && <Loader size="xs" mr="xs" />} |
There was a problem hiding this comment.
🔵 minor — Same loading/empty markup copied into 8 chart components
The same <div className="d-flex h-100 ... text-muted"><Loader size="xs" mr="xs" />Loading Chart Data...</div> block and the matching isPlaceholderData ternary are now repeated in DBBarChart, DBHistogramChart, DBListBarChart, DBNumberChart, DBPieChart, DBTableChart, DBTimeChart and ServiceDashboardSlowestEventsTile (one copy has already drifted, see that finding). Move it into one shared ChartLoadingState/ChartEmptyState({ isLoading }) component next to ChartErrorState, and have every chart use it.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| <div className="d-flex h-100 w-100 align-items-center justify-content-center text-muted"> | ||
| {isPlaceholderData && <Loader size="xs" mr="xs" />} | ||
| {isPlaceholderData | ||
| ? 'Loading Chart Data...' |
There was a problem hiding this comment.
🔵 minor — New loading copy uses Title Case
The new strings 'Loading Chart Data...' break the REQUIRED rule "UI text: use sentence case". The PR description itself writes "Loading chart data...". Use 'Loading chart data...' in all the new branches. Also update the existing initial-loading branches and the test assertions that match on the old text.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
5 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 4 minor 3 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines2 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. Core change is a safe, well-tested UI loading-state unification; the six categorical/table charts correctly gate on 🟡 P2 — recommended
🔵 P3 nitpicks (6)
Reviewers (9): ce-correctness-reviewer, ce-testing-reviewer, ce-maintainability-reviewer, ce-project-standards-reviewer, ce-kieran-typescript-reviewer, ce-julik-frontend-races-reviewer, ce-agent-native-reviewer, ce-learnings-researcher, ce-previous-comments-reviewer. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
E2E Test Results❌ 1 test failed • 463 passed • 1 skipped • 1505s
Tests ran across 4 shards in parallel. |
Summary
Loading states were difficult to see across the app often and oddly worded. Charts would be loading and display text like "No data found in time range", now changed to "Loading chart data..." with a loading spinner. Also, loading color in the theme is changed to neutral instead of the weird yellow color.
References