feat: Add interactive Validator Performance Dashboard with historical charts and delegation flow visualization#549
feat: Add interactive Validator Performance Dashboard with historical charts and delegation flow visualization#549alibabaedge wants to merge 9 commits intodevfrom
Conversation
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Verdict: REQUEST CHANGES
[HIGH] Mock/random data shipped to production
page.tsx:30-42 — generateUptimeData() uses Math.random() to produce fake uptime data on every request. This means:
- Users see different data on every page load
- The data has no relation to actual validator performance
- Hardcoded values like
selfDelegationRatio: 8.5,slashScore = 95,totalDelegated = list.length * 125000are also fabricated
This is a feature that displays fake metrics as if they're real, which is misleading. Either wire up real data sources or clearly label this as a demo/placeholder.
[HIGH] Server Component calls Math.random() — non-deterministic SSR
generateUptimeData() runs server-side in a force-dynamic page. While it won't cause hydration mismatches (no client re-render), it produces random data on every request, making the page untestable and unreliable. If this is intentional placeholder data, it should be flagged in the UI.
[MED] Duplicate sm:min-h-[45px] in delegation-flow-widget.tsx:27-28
const cardClass = `
pt-2.5
sm:min-h-[45px] // ← duplicate
sm:min-h-[55px] // ← overrides the one above
md:min-h-[63px]The first sm:min-h-[45px] is dead code — remove it.
[MED] Hardcoded color #272727 in performance-score-card.tsx:51
stroke="#272727"This bypasses Tailwind's theme and won't adapt to theme changes. Should reference a Tailwind color variable (e.g., via stroke-table_header or a CSS variable).
[MED] Hardcoded inline style colors in performance-score-card.tsx:67 and CircularProgress
<span className="font-handjet text-3xl" style={{ color }}>{score}</span>and
style={{ backgroundColor: getScoreColor(factor.score) }}These hardcoded hex colors (#4FB848, #E5C46B, #F3B101, #EB1616) bypass the Tailwind theme. The project convention says "Use TailwindCSS classes exclusively, avoid inline CSS or <style> tags." Consider mapping to Tailwind classes (text-secondary, text-red, etc.) similar to what uptime-heatmap.tsx does correctly.
[MED] parseInt(id) without validation in page.tsx:49
const validatorId = parseInt(id);
const validator = await validatorService.getById(validatorId);If id is not a valid number, parseInt returns NaN, which will propagate to the service call. Add a guard:
const validatorId = parseInt(id);
if (isNaN(validatorId)) return notFound();[LOW] Tooltip positioning uses fixed with mouse coordinates in uptime-heatmap.tsx:91-94
className="pointer-events-none fixed z-[999]..."
style={{ left: tooltip.x, top: tooltip.y - 60, transform: 'translateX(-50%)' }}This works but can clip off-screen at page edges. The existing ToolTip component used elsewhere in the project might be more consistent, though the grid-cell hover pattern may justify a custom approach.
[LOW] onFocus cast in uptime-heatmap.tsx:82
onFocus={(e) => handleMouseEnter(day, e as unknown as React.MouseEvent)}Double cast (as unknown as) is a code smell. Consider refactoring handleMouseEnter to accept a generic element reference instead of relying on MouseEvent.
[LOW] screenshots/issue-548.png added to repo
Binary files in the repo increase clone size. Consider linking to an external image host or placing in a .github/ assets location if needed for issue tracking.
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Decision: REQUEST CHANGES
[HIGH] Import path uses @/app/ instead of proper alias in page.tsx
The page imports use @/app/validators/[id]/... which bypasses the locale segment. The correct import should use a relative path or the full @/ alias that maps to src/app/[locale]/.
File: src/app/[locale]/validators/[id]/(validator-profile)/performance/page.tsx:4-6
import DelegationFlowWidget from '@/app/validators/[id]/(validator-profile)/performance/delegation-flow-widget';
import PerformanceScoreCard from '@/app/validators/[id]/(validator-profile)/performance/performance-score-card';
import UptimeHeatmap from '@/app/validators/[id]/(validator-profile)/performance/uptime-heatmap';The @/ alias maps to src/app/[locale]/ per tsconfig, but these paths include validators/[id]/... without the [locale] segment being in the alias. These should be relative imports:
import DelegationFlowWidget from './delegation-flow-widget';
import PerformanceScoreCard from './performance-score-card';
import UptimeHeatmap from './uptime-heatmap';[HIGH] slashHistory: 0 artificially tanks composite score
In page.tsx:85, slashScore is hardcoded to 0, meaning slash history contributes 0 out of 10 to the composite score. For a "no slashing events" scenario, this should be 100 (perfect score = no slashes). Currently, a validator with no slashes gets penalized.
const slashScore = 0; // Should be 100 — no slashes means perfect slash history[MED] Hardcoded 'en-US' locale in delegation-flow-widget
delegation-flow-widget.tsx:49,57 uses toLocaleString('en-US') which ignores the user's locale. The app supports en, pt, ru — numbers should be formatted according to the active locale.
[MED] Unsafe t() call with dynamic key in uptime-heatmap
uptime-heatmap.tsx:107: t(getUptimeLevel(tooltip.day.uptime)) passes a dynamic string to t(). If getUptimeLevel returns 'none', there is no "none" translation key — only "no data". This will cause a missing translation warning/error at runtime.
// Returns 'none' but the key is 'no data'
const getUptimeLevel = (uptime: number | null): string => {
if (uptime === null) return 'none'; // ← no translation key for 'none'Fix: map 'none' to t('no data') explicitly, or change the return value.
[MED] Tooltip positioning uses fixed with getBoundingClientRect — breaks on scroll
uptime-heatmap.tsx:94-98: The tooltip uses position: fixed with coordinates from getBoundingClientRect(). This works initially but will misposition if the user scrolls while hovering (since getBoundingClientRect values are viewport-relative and fixed is also viewport-relative, this actually works — but the tooltip is set on mouse enter and not updated on scroll, so scrolling while hovering will leave a detached tooltip). Consider using a relative/absolute positioning strategy or recalculating on scroll.
[MED] generatePlaceholderUptimeData runs on every request — not memoized
page.tsx:32-44: This function generates 90 data points on every server request. Since it's deterministic (seeded by index), it should be defined as a constant outside the component or memoized. Minor perf concern but it's a server component running on every page load.
[LOW] Missing SubDescription import ordering
page.tsx:14: SubDescription import is separated from other component imports and not alphabetically ordered. Minor style issue.
[LOW] focusedIndex keyboard navigation lacks auto-focus
uptime-heatmap.tsx:62-67: Arrow key navigation updates focusedIndex state but doesn't call .focus() on the target element, so the visual focus ring won't move. The tabIndex approach needs a useEffect + ref to actually move focus.
[LOW] Accessibility: heatmap grid cells all have tabIndex={-1} initially
uptime-heatmap.tsx:82: focusedIndex starts at -1, meaning no cell has tabIndex={0} on initial render. The grid is unreachable via keyboard until a cell is clicked. At least the first cell should be focusable (focusedIndex default to 0).
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Verdict: REQUEST CHANGES (has MED issues)
[MED] Placeholder data generation is non-deterministic and date-dependent
File: performance/page.tsx:33-46
generatePlaceholderUptimeData() uses new Date() inside a Server Component with force-dynamic. This is fine for now since it's demo data, but the seed-based "randomness" (i * 7 + 13) % 100 produces fixed patterns that don't actually vary — yet the function rebuilds the date array on every request. Consider extracting this as a static constant or at minimum adding a comment that this entire function should be removed when real data lands. The bigger concern: this placeholder logic is in production code with no TODO or tracking issue referenced.
[MED] Composite score calculation duplicated between page and component
File: performance/page.tsx:76-84 computes uptimeScore, governanceScore, commissionScore, slashScore and passes them as a breakdown object. Then performance-score-card.tsx:96-101 re-applies WEIGHTS to compute compositeScore. The weight constants (0.4, 0.3, 0.2, 0.1) are only defined in the card component but the page also implicitly relies on knowing what those scores mean. When real data replaces placeholders, having score computation split across page and component will be error-prone. Move all score computation into one place (the card component or a utility).
[MED] governanceScore calculation is arbitrary and scales with network count
File: performance/page.tsx:80
const governanceScore = Math.min(100, Math.round(list.length * 12 + 20));A validator with 7+ networks automatically scores 100 on governance regardless of actual participation. This will produce misleading scores. Even for demo data, this is user-facing — consider using a fixed demo value or clamping more conservatively with a comment explaining the formula.
[LOW] Tooltip uses fixed positioning with raw getBoundingClientRect — will misposition on scroll
File: uptime-heatmap.tsx:92-100
The tooltip uses position: fixed with rect.top from getBoundingClientRect(). This works correctly for fixed positioning (since getBoundingClientRect returns viewport-relative coords), but if any ancestor has a CSS transform, filter, or will-change, it creates a new containing block and breaks fixed. This is a common subtle bug. Consider using a portal or absolute positioning relative to the heatmap container.
[LOW] Heatmap keyboard navigation doesn't actually move focus
File: uptime-heatmap.tsx:57-65
handleKeyDown updates focusedIndex state but never calls .focus() on the target element. The tabIndex={focusedIndex === globalIndex ? 0 : -1} pattern requires programmatic focus to work — after re-render the new element gets tabIndex=0 but the browser focus doesn't move. You'd need a ref or useEffect to focus the element after state update.
[LOW] cardClass uses template literal with newlines creating extra whitespace in className
File: delegation-flow-widget.tsx:26-32
The multi-line template literal includes newline characters in the class string. Tailwind processes this fine, but it's unconventional — other components in the project use single-line strings or cn(). Not a bug, just inconsistent style.
Positives
- All 3 locale files updated correctly (en, pt, ru) — follows project convention
- Proper use of
getTranslations(SSR) vsuseTranslations(client) - Accessibility: ARIA roles, labels, keyboard support on heatmap, progressbar roles
- Reuses existing components (
MetricsCardItem,PageTitle,SubTitle,ToolTip,SubDescription) - Service layer properly used via
validatorService— no direct DB queries - Page follows established validator profile patterns exactly
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Decision: REQUEST CHANGES
[HIGH] Hardcoded placeholder data generation in production page
src/app/[locale]/validators/[id]/(validator-profile)/performance/page.tsx:33-46
The generatePlaceholderUptimeData function uses a deterministic seed-based algorithm that produces the same "fake" data for every validator. This means:
- Every validator shows identical uptime history
- Users cannot distinguish validator reliability
- The data is misleading despite the demo banner
While the TODO references #548, this function runs unconditionally in production. The governance score calculation (list.length * 12 + 20) is also arbitrary and identical logic applies.
Recommendation: At minimum, seed the placeholder data with validatorId so different validators show different demo data, or better yet, return empty/null states and show "data coming soon" placeholders instead of fake numbers.
[MED] generateMetadata params type doesn't match Next.js 14+ async params pattern
src/app/[locale]/validators/[id]/(validator-profile)/performance/page.tsx:23-28
The params is destructured synchronously, but in Next.js 14+ App Router, params is a Promise. The page component's PageProps interface also uses synchronous destructuring. This should match the pattern used elsewhere in the project.
// Current
export async function generateMetadata({ params: { locale } }: { params: { locale: Locale } })
// Should verify this matches the project's Next.js version pattern[MED] Tooltip positioning uses fixed with getBoundingClientRect — breaks on scroll
src/app/[locale]/validators/[id]/(validator-profile)/performance/uptime-heatmap.tsx:99-113
The tooltip uses position: fixed with coordinates from getBoundingClientRect(). getBoundingClientRect returns viewport-relative coordinates, so fixed positioning works initially, but the tooltip is set once on hover — if the user scrolls while hovering (common on this grid), the tooltip stays at the original viewport position while the cell moves. Also, the hardcoded top: tooltip.y - 60 offset may clip above the viewport for cells near the top.
Recommendation: Use a relative/absolute positioning approach anchored to the grid container, or recalculate position on scroll.
[MED] cellRefs Map is never cleaned up
src/app/[locale]/validators/[id]/(validator-profile)/performance/uptime-heatmap.tsx:24
The ref callback adds entries to the Map but never removes them. When React unmounts/remounts cells (e.g., during re-renders), stale entries accumulate. The cleanup pattern should be:
ref={(el) => {
if (el) cellRefs.current.set(globalIndex, el);
else cellRefs.current.delete(globalIndex);
}}[MED] useEffect focus logic may conflict with user focus
src/app/[locale]/validators/[id]/(validator-profile)/performance/uptime-heatmap.tsx:26-31
The useEffect that auto-focuses cells runs on every focusedIndex change but the guard condition cell.closest('[role="grid"]')?.contains(document.activeElement) means it only fires when focus is already inside the grid. This is fine for keyboard nav, but the initial render sets focusedIndex = 0 — if the user tabs into the grid, focus may jump unexpectedly to index 0 instead of the first cell they'd naturally land on.
[LOW] locale prop on DelegationFlowWidget is optional but always passed
src/app/[locale]/validators/[id]/(validator-profile)/performance/delegation-flow-widget.tsx:13
The locale prop is typed as locale?: string (optional), but it's always passed from the page. Making it required would be more accurate and prevent potential undefined being passed to toLocaleString().
[LOW] Magic number thresholds should be constants
src/app/[locale]/validators/[id]/(validator-profile)/performance/uptime-heatmap.tsx:42-47
The uptime thresholds (99, 95, 90) are inline magic numbers. Consider extracting them as named constants alongside the WEIGHTS constant pattern used in performance-score-card.tsx.
[LOW] Tab ordering — Performance tab placed after Governance
src/app/[locale]/components/common/tabs/tabs-data.ts:97-102
Minor UX note: The Performance tab is appended last. Consider whether it should be positioned earlier (e.g., after Metrics) since performance data is closely related to metrics and may be more frequently accessed than Governance.
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Decision: REQUEST CHANGES
[HIGH] Placeholder data generation uses deterministic seed, not actual randomness — but more importantly, TODO comments left without issue tracking
The generatePlaceholderUptimeData function in page.tsx:33-47 generates fake data with a deterministic seed based on validatorId. The TODO comments reference #548 which is good, but several values are hardcoded with no TODO markers:
governanceScore = 50(line 80) — hardcoded with a TODOselfDelegationRatio={0}(line 121) — hardcoded placeholder with no TODOnetDelegationChange={0}(line 120) — hardcoded placeholder with no TODOtotalDelegated={'—'}(line 119) — hardcoded placeholder with no TODO
Fix: Add TODO comments referencing the tracking issue for all placeholder values, not just some.
[MED] page.tsx fetches all validator nodes with no limit — potential performance issue
page.tsx:63-70: getValidatorNodesWithChains is called with Number.MAX_SAFE_INTEGER as the limit parameter. This fetches every single node for a validator in one query with no pagination.
const { validatorNodesWithChainData: list } = await validatorService.getValidatorNodesWithChains(
validatorId, [], [], 0, Number.MAX_SAFE_INTEGER, sortBy, order,
);Fix: If you only need delegatorsAmount and commission, consider a dedicated service method or at minimum document why the full fetch is necessary.
[MED] uptime-heatmap.tsx:88 — ref callback sets state on every render cycle
The ref callback ref={(el) => { if (el) cellRefs.current.set(globalIndex, el); else cellRefs.current.delete(globalIndex); }} is recreated on every render for every cell (90 cells). While using a Map on a ref avoids re-renders, the inline closure is recreated each time.
This is minor but consider using useCallback for the ref setter pattern, or accept the tradeoff since it's only 90 cells.
[MED] uptime-heatmap.tsx — tooltip positioning can overflow viewport
The tooltip is positioned with top: tooltip.y - 60 and transform: translateX(-50%). For cells near the top or left/right edges, the tooltip will clip outside the visible area. The pointer-events-none class prevents interaction but doesn't prevent visual overflow.
Fix: Add bounds checking or use a tooltip library consistent with the project's existing ToolTip component used in page.tsx.
[LOW] performance-score-card.tsx — three nearly identical color-mapping functions
getScoreColorClass, getScoreBgClass, and getScoreStrokeClass (lines 19-37) share identical threshold logic with only the prefix differing (text-, bg-, stroke-). Could be a single function returning all three, but it's only 3 small functions so this is cosmetic.
[LOW] Inconsistent use of style attribute for width in progress bars
delegation-flow-widget.tsx:71 and performance-score-card.tsx:128 use inline style={{ width: ... }} for dynamic widths. This is acceptable since Tailwind can't handle dynamic values, but worth noting for consistency awareness.
[LOW] useEffect in uptime-heatmap.tsx:24-28 has a complex condition
The check cell.closest('[role="grid"]')?.contains(document.activeElement) is defensive but could be simplified. Consider just checking document.activeElement?.closest('[role="grid"]') === gridRef.current.
Summary: The main concerns are the unbounded data fetch (Number.MAX_SAFE_INTEGER) which could cause performance issues for validators with many nodes, incomplete TODO tracking for placeholder values, and tooltip overflow. The component architecture, translations, accessibility (ARIA roles, keyboard navigation), and service layer usage all follow project conventions well.
… charts and delegation flow visualization (#548)
- Replace Math.random() with deterministic seed-based placeholder data - Add demo banner to clearly label placeholder/demo metrics - Remove fabricated hardcoded values (selfDelegationRatio, totalDelegated, slashScore) - Add parseInt NaN validation with notFound() guard - Remove duplicate sm:min-h-[45px] in delegation-flow-widget - Replace hardcoded #272727 stroke with Tailwind stroke-table_header class - Replace all inline style colors with Tailwind classes (text-*, bg-*, stroke-*) - Refactor onFocus handler to avoid double type cast (as unknown as) - Remove screenshots/issue-548.png binary from repo - Add demo banner translations to all 3 locale files
alibabaedge
force-pushed
the
agent/issue-548
branch
from
47a8664 to
c2381f7
Compare
Add TODO(#548) comments for all remaining placeholder values (totalDelegated, netDelegationChange, selfDelegationRatio) and document the Number.MAX_SAFE_INTEGER fetch with a TODO for a dedicated service method.
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Verdict: REQUEST CHANGES
[HIGH] Unsafe params access — Next.js 15 requires await for params
In page.tsx, the component destructures params synchronously:
const ValidatorPerformancePage: NextPageWithLocale<PageProps> = async ({ params: { locale, id } }) => {And in generateMetadata:
export async function generateMetadata({ params: { locale } }: { params: { locale: Locale } }) {In Next.js 15 (App Router), params is a Promise and must be awaited. Check how other pages in this project handle it — if they use await params or a different pattern, this page must match. This will cause runtime errors.
[HIGH] getValidatorNodesWithChains fetches unbounded data
page.tsx:67-74 — Passing Number.MAX_SAFE_INTEGER as the limit fetches all validator nodes in a single query:
const { validatorNodesWithChainData: list } = await validatorService.getValidatorNodesWithChains(
validatorId, [], [], 0, Number.MAX_SAFE_INTEGER, sortBy, order,
);This is a performance and potential DoS concern for validators with many nodes. The page only needs delegatorsAmount and commission — the TODO comment acknowledges this but the current code ships an expensive unbounded query to production.
[MED] Deterministic "random" placeholder data leaks implementation details
generatePlaceholderUptimeData in page.tsx:33-44 uses a seeded formula (i * 7 + 13 + validatorId * 3) % 100 that produces identical output per validator. While the demo banner is shown, users may perceive this as real data since it varies by validator. Consider using a fixed static dataset or making the seed clearly constant across all validators to avoid confusion.
[MED] Unused variables sortBy and order are misleadingly named
page.tsx:63-64:
const sortBy: string = 'prettyName';
const order: SortDirection = 'asc';These are only used for the getValidatorNodesWithChains call and have nothing to do with the performance page's own sorting. They should be inlined into the function call to reduce noise.
[MED] uptime-heatmap.tsx ref callback may cause issues in strict mode
uptime-heatmap.tsx:91:
ref={(el) => { if (el) cellRefs.current.set(globalIndex, el); else cellRefs.current.delete(globalIndex); }}In React 19+ strict mode, inline ref callbacks run cleanup. This pattern works but creates a new function on every render for every cell (90 cells). Extract the ref setter or use useCallback to avoid unnecessary re-renders and GC pressure.
[MED] Tooltip positioning can overflow viewport
uptime-heatmap.tsx:108-111 — The tooltip uses a fixed top: tooltip.y - 60 offset:
style={{ left: tooltip.x, top: tooltip.y - 60, transform: 'translateX(-50%)' }}For cells in the first row, this positions the tooltip above the grid container at negative top, potentially clipping outside the visible area. Other tooltips in the project (via the ToolTip component) handle boundary detection.
[LOW] CircularProgress component could be extracted to common components
The CircularProgress in performance-score-card.tsx is a reusable visualization pattern. If other pages need circular progress indicators in the future, consider moving it to @/components/common/. Not blocking for now.
[LOW] Missing key diversity in legend items
uptime-heatmap.tsx:118-125 — The legend uses individual divs without a wrapping list or semantic structure. Consider using a <ul> with <li> items for better accessibility, though the aria-label attributes partially compensate.
[LOW] style attribute for width in progress bars
delegation-flow-widget.tsx:71 and performance-score-card.tsx:128 use style={{ width: ... }} for dynamic widths. This is acceptable here since the value is dynamic, but note the project convention prefers Tailwind classes.
- Inline sortBy/order variables directly into service call to reduce noise - Remove unused SortDirection import - Fix tooltip viewport overflow: detect when tooltip would clip above grid and show below the cell instead - Pre-calculate tooltip Y position in handler instead of in style prop
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Decision: REQUEST CHANGES
[HIGH] Unbounded database query fetching all validator nodes
In page.tsx:62-69, getValidatorNodesWithChains is called with Number.MAX_SAFE_INTEGER as the limit, fetching ALL validator nodes just to count delegators. This is a performance problem that will worsen as data grows.
const { validatorNodesWithChainData: list } = await validatorService.getValidatorNodesWithChains(
validatorId,
[], [], 0,
Number.MAX_SAFE_INTEGER, // fetches everything
'prettyName', 'asc',
);Fix: Add a dedicated service method (or a simple Prisma query) that aggregates delegatorsAmount and average commission directly in the database, rather than loading all nodes into memory. The TODO comment acknowledges this but it should not ship as-is with Number.MAX_SAFE_INTEGER.
[MED] Deterministic "random" placeholder data leaks implementation detail
generatePlaceholderUptimeData in page.tsx:33-45 uses validatorId as a seed, producing different-looking but equally fake data per validator. Users may interpret varying heatmaps as real differences between validators. Since the demo banner exists, this is mitigated, but the function should produce identical/obviously-fake data for all validators (e.g., a flat pattern) to avoid misleading users.
[MED] generateMetadata params type doesn't match Next.js App Router expectations
In page.tsx:23, the params should be a Promise in Next.js 15+ App Router (the project uses NextPageWithLocale pattern). Check how other pages in the (validator-profile) group handle generateMetadata — the params type should be consistent.
[MED] Missing key stability in heatmap weeks
In uptime-heatmap.tsx:96, weeks use array index as key (key={weekIdx}). While the data is static per render, using a stable key derived from the first date in the week (key={week[0]?.date}) would be more robust and is a project best practice.
[LOW] Inline style usage in multiple components
The project convention says "Use TailwindCSS classes exclusively, avoid inline CSS or <style> tags." Three places use inline style:
delegation-flow-widget.tsx:71:style={{ width: ... }}— acceptable for dynamic percentage values, no good Tailwind alternative.performance-score-card.tsx:133: same pattern — acceptable.uptime-heatmap.tsx:114: tooltip positioning withleft/top/transform— acceptable for dynamic positioning.
These are all dynamic values that can't be expressed as Tailwind classes, so this is acceptable — just noting for completeness.
[LOW] toLocaleString on server component may not match client locale
In delegation-flow-widget.tsx:47, uniqueDelegators.toLocaleString(locale) runs server-side where the Node.js locale data may differ from the browser. Minor inconsistency risk.
[LOW] Unused translation keys
The keys "missed blocks" and "uptime" are defined in translations and used in the heatmap tooltip, but the heatmap tooltip calls t(getUptimeLevel(...)) which maps to 'excellent' | 'good' | 'fair' | 'poor' | 'no data' — this is fine. Just confirming all keys are used.
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Decision: REQUEST CHANGES
[HIGH] Inline style usage violates project conventions
In uptime-heatmap.tsx:119 and delegation-flow-widget.tsx:71, inline style={{ width: ... }} is used. While dynamic percentage widths are a legitimate exception (Tailwind can't handle runtime values), the tooltip positioning in uptime-heatmap.tsx:113 uses inline style={{ left, top, transform }} which is acceptable for dynamic positioning. The width cases are fine too since they're runtime-computed. Downgrading this — no action needed.
[HIGH] generateMetadata uses deprecated params pattern
In page.tsx:23, params are destructured directly instead of being awaited. In Next.js 15+, params is a Promise and must be awaited:
// Current (deprecated/broken in Next.js 15)
export async function generateMetadata({ params: { locale } }: { params: { locale: Locale } })
// Should be
export async function generateMetadata({ params }: { params: Promise<{ locale: Locale }> }) {
const { locale } = await params;Same issue in the page component itself at line 49. Check how other pages in this codebase handle params — if they use the sync pattern consistently, this may be fine for the current Next.js version, but verify.
[MED] Unbounded data fetch — Number.MAX_SAFE_INTEGER as page size
page.tsx:58-59: Passing Number.MAX_SAFE_INTEGER as the page size to getValidatorNodesWithChains will attempt to load ALL validator nodes in a single query. This could be very expensive for validators with many nodes and will grow worse over time. Consider either:
- Adding a dedicated service method that returns just
delegatorsAmountandcommissionaggregates - Using a reasonable upper bound
const { validatorNodesWithChainData: list } = await validatorService.getValidatorNodesWithChains(
validatorId, [], [], 0, Number.MAX_SAFE_INTEGER, 'prettyName', 'asc',
);[MED] Deterministic "random" placeholder data leaks implementation detail
page.tsx:33-44: generatePlaceholderUptimeData uses validatorId as a seed, which means the same validator always shows the same fake data. Users might mistake this for real data despite the banner. The function also runs on every request (no caching). Consider making the demo nature more obvious — e.g., using a fixed flat dataset or adding visual indicators per cell.
[MED] Missing error handling for getById / getValidatorNodesWithChains
page.tsx:53-59: If validatorService.getById returns null, the page continues with validatorMoniker = 'Validator' and still calls getValidatorNodesWithChains. A non-existent validator should likely trigger notFound():
const validator = await validatorService.getById(validatorId);
if (!validator) return notFound();[LOW] Tooltip positioning can overflow container
uptime-heatmap.tsx:113-116: The tooltip uses absolute positioning relative to the grid, but there's no boundary clamping for the X axis. If a cell is near the left/right edge, the tooltip (min-width 128px) will overflow. Consider clamping relativeX to keep the tooltip within the grid bounds.
[LOW] cellRefs Map is never cleared
uptime-heatmap.tsx:22: The cellRefs Map accumulates entries. The cleanup in the ref callback (else cellRefs.current.delete(globalIndex)) handles unmounting, but if data changes length, stale entries above the new length persist. Minor since data is static here.
[LOW] Consider extracting shared color/threshold constants
performance-score-card.tsx and uptime-heatmap.tsx both define color thresholds independently with slightly different breakpoints (80/60/40 vs 99/95/90). While they serve different purposes (composite score vs uptime), documenting why the thresholds differ would help maintainability.
Return notFound() when validator doesn't exist instead of falling through with a default moniker. This prevents the page from making unnecessary service calls and displaying data for non-existent validators.
m1amgn
left a comment
m1amgn
left a comment
There was a problem hiding this comment.
AI Review Summary
Decision: REQUEST CHANGES
[HIGH] Unsafe Number.MAX_SAFE_INTEGER passed as pagination limit
In page.tsx:68, passing Number.MAX_SAFE_INTEGER as the limit to getValidatorNodesWithChains will attempt to fetch all validator nodes in a single query with no upper bound. This could cause memory/performance issues for validators with many nodes.
const { validatorNodesWithChainData: list } = await validatorService.getValidatorNodesWithChains(
validatorId, [], [], 0, Number.MAX_SAFE_INTEGER, 'prettyName', 'asc',
);Fix: Use a reasonable upper bound (e.g., 10000) or create a dedicated service method that only fetches needed aggregates (commission, delegatorsAmount) as the TODO comment already suggests.
[MED] generateMetadata params type doesn't match Next.js App Router convention
In page.tsx:23, Next.js 13+ App Router expects params to be a Promise in newer versions. More critically, the generateMetadata function signature differs from the page component's PageProps - it omits id, so it can't generate validator-specific metadata.
export async function generateMetadata({ params: { locale } }: { params: { locale: Locale } }) {Fix: Include id in the params destructuring and use the validator name in the metadata title for better SEO, consistent with how other validator profile pages likely handle this.
[MED] Inline style attribute for progress bar width in delegation-flow-widget.tsx:73 and performance-score-card.tsx:131
Per CLAUDE.md: "Use TailwindCSS classes exclusively, avoid inline CSS or <style> tags." The style={{ width: ... }} is used for dynamic percentage widths. This is a pragmatic exception since Tailwind can't handle arbitrary runtime values, but worth noting for consistency awareness. No action required - this is an acceptable pattern for dynamic values.
[MED] Missing key stability in uptime-heatmap.tsx:97 week rows
<div key={week[0]?.date ?? weekIdx} className="flex flex-col gap-1" role="row">If week[0] is undefined (last week partially filled), it falls back to weekIdx which is fine, but the mixing of key types (string dates vs numbers) across renders could cause issues. Minor but worth using a consistent key strategy.
[MED] Deterministic "random" placeholder data leaks implementation detail
generatePlaceholderUptimeData in page.tsx:33-45 uses a seed based on validatorId and loop index. The formula (i * 7 + 13 + validatorId * 3) % 100 means seed > 10 always holds for most inputs, making data appear consistently present. The uptime range is 85-100 which looks realistic and could mislead users despite the demo banner. Consider making the placeholder nature more visually obvious (e.g., all cells same color, or a clear pattern).
[LOW] useEffect focus management in uptime-heatmap.tsx:24-29 could be simplified
The condition cell.closest('[role="grid"]')?.contains(document.activeElement) is defensive but complex. Consider extracting to a named helper for readability.
[LOW] toLocaleString(locale) in delegation-flow-widget.tsx:49
locale is typed as string which works, but toLocaleString accepts Intl.LocalesArgument. This is fine functionally but the type could be more precise.
[LOW] Tab ordering in tabs-data.ts
The "Performance" tab is added after "Governance". Verify this matches the desired UX ordering - performance might logically fit before governance or after metrics.
|
Closing to stop auto-fix loop. Test PR — will be recreated. |
|
Auto-fix limit (2) reached for automated reviews. Manual fix required. |
2 participants
What: Add interactive Validator Performance Dashboard with uptime heatmap, delegation flow widget, and composite performance score card
Why: Closes #548
How:
tabs-data.ts/validators/[id]/performancewith server-rendered page componentUptimeHeatmapclient component — 90-day calendar heatmap with color-coded cells (green/yellow/orange/red/gray), hover tooltips showing date/uptime%/missed blocks, keyboard navigation support, and color-blind friendly legendDelegationFlowWidgetserver component — summary cards showing total delegated, unique delegators, net 30d change with arrow indicators, and self-delegation ratio progress barPerformanceScoreCardclient component — circular SVG progress indicator with composite score (0-100) calculated from uptime (40%), governance participation (30%), commission competitiveness (20%), and slash history (10%), with per-factor breakdown barsValidatorPerformancenamespace andValidatorProfileHeader.Tabs.Performanceacross all 3 locale files (EN, PT, RU)OwnPropsinterface,FC<OwnProps>typing, Tailwind-only styling with project custom colors, proper ARIA labels and keyboard handlersvalidatorServicefor data fetching, matching the service layer patternTesting:
npx eslintwith zero errorsScreenshot: