refactor(dashboard): update metrics for latest leaderboard API#193
Closed
eliteprox wants to merge 51 commits into
Closed
refactor(dashboard): update metrics for latest leaderboard API#193eliteprox wants to merge 51 commits into
eliteprox wants to merge 51 commits into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
size/L
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds raw "explorer" data endpoints and types (networkDemand, gpuMetrics, slaCompliance), renames KPI dailyStreamCount→dailySessionCount, adds orchestrator.effectiveSuccessRate and pipeline.regions, updates leaderboard API shapes and provider wiring, and adjusts dashboard UI polling/refresh behavior and hooks. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web-next/src/content/docs/guides/dashboard-data-provider.mdx (1)
59-66:⚠️ Potential issue | 🟡 MinorUpdate KPI docs to include all required fields in the current contract.
The resolver example and KPI type row now use
dailySessionCount, but still omit requireddailyNetworkFeesEthandtimeframeHours.📝 Proposed doc fix
return { successRate: { value: stats.successRate, delta: stats.successRateDelta }, orchestratorsOnline: { value: stats.orchCount, delta: stats.orchDelta }, dailyUsageMins: { value: stats.usageMins, delta: stats.usageDelta }, dailySessionCount: { value: stats.streams, delta: stats.streamsDelta }, + dailyNetworkFeesEth: { value: stats.feesEth, delta: stats.feesEthDelta }, + timeframeHours: 24, };-| `KPI` | `successRate`, `orchestratorsOnline`, `dailyUsageMins`, `dailySessionCount` (all `MetricDelta!`) | +| `KPI` | `successRate`, `orchestratorsOnline`, `dailyUsageMins`, `dailySessionCount`, `dailyNetworkFeesEth` (`MetricDelta!`), `timeframeHours` (`Int!`) |Also applies to: 180-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/content/docs/guides/dashboard-data-provider.mdx` around lines 59 - 66, The kpi resolver example (kpi: async (args) => { ... }) is missing required fields from the current KPI contract; update the returned object to include dailyNetworkFeesEth (e.g., { value: stats.networkFeesEth, delta: stats.networkFeesDelta }) and timeframeHours (e.g., { value: stats.timeframeHours }) alongside existing successRate, orchestratorsOnline, dailyUsageMins, and dailySessionCount so the example matches the KPI type/contract.
🧹 Nitpick comments (3)
plugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts (1)
414-415: Add orchestrator coverage foreffectiveSuccessRate.The provider now emits this field, but the orchestrator integration test does not request/assert it yet.
🧪 Suggested test extension
- query: '{ orchestrators { address knownSessions successSessions successRatio noSwapRatio slaScore pipelines pipelineModels { pipelineId modelIds } gpuCount } }', + query: '{ orchestrators { address knownSessions successSessions successRatio effectiveSuccessRate noSwapRatio slaScore pipelines pipelineModels { pipelineId modelIds } gpuCount } }',expect(orchA.successRatio).toBe(100); + expect(orchA.effectiveSuccessRate).toBe(100); expect(orchA.noSwapRatio).toBe(100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts` around lines 414 - 415, The orchestrator test needs to request and assert the new effectiveSuccessRate field: update the GraphQL query string inside the test (the one returning orchestrators { ... }) to include effectiveSuccessRate, cast the response as DashboardQueryResponse as before, and add assertions in the same test that validate the orchestrator objects include the expected effectiveSuccessRate values (alongside existing checks for successRatio, slaScore, etc.) so the provider integration covers the new field.apps/web-next/src/app/(dashboard)/dashboard/page.tsx (1)
94-95: Consider droppingeffectiveSuccessRatefrom the query until it is used in the UI.It is currently fetched but not rendered/consumed on this page.
✂️ Optional query trim
- address knownSessions successSessions successRatio effectiveSuccessRate noSwapRatio slaScore pipelines pipelineModels { pipelineId modelIds } gpuCount + address knownSessions successSessions successRatio noSwapRatio slaScore pipelines pipelineModels { pipelineId modelIds } gpuCount🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/`(dashboard)/dashboard/page.tsx around lines 94 - 95, The GraphQL query in page.tsx is requesting the field effectiveSuccessRate even though the page does not use it; remove effectiveSuccessRate from the selection set in the dashboard query (the line containing "address knownSessions successSessions successRatio effectiveSuccessRate noSwapRatio slaScore pipelines pipelineModels { pipelineId modelIds } gpuCount") to avoid unnecessary data fetching and simplify the query.packages/plugin-sdk/src/contracts/__tests__/dashboard.test.ts (1)
118-127: Strengthen KPI contract test coverage for required fields.This test now checks
dailySessionCount, but adding assertions for the other required KPI fields would better guard schema drift.✅ Suggested test additions
expect(fields).toHaveProperty('successRate'); expect(fields).toHaveProperty('orchestratorsOnline'); expect(fields).toHaveProperty('dailyUsageMins'); expect(fields).toHaveProperty('dailySessionCount'); + expect(fields).toHaveProperty('dailyNetworkFeesEth'); + expect(fields).toHaveProperty('timeframeHours');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-sdk/src/contracts/__tests__/dashboard.test.ts` around lines 118 - 127, The KPI contract test ("KPI type has required MetricDelta fields") currently asserts only a subset of required fields; update this test to assert presence of all required KPI MetricDelta fields by adding expect checks for each missing field using the same pattern (use buildSchema(DASHBOARD_SCHEMA), getType('KPI') as any, kpiType.getFields() and then expect(fields).toHaveProperty('<fieldName>') for each required field such as the ones mentioned plus any others defined in the contract) so the test fails if any required KPI field is removed or renamed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/dashboard-data-provider/frontend/src/provider.ts`:
- Around line 112-119: resolveKPI is still calling trueSuccessRate instead of
the new weightedSuccessRate, causing KPI values to use the legacy computation;
update resolveKPI to call weightedSuccessRate (and any other references at the
second occurrence) so the KPI path uses weightedSuccessRate of
effective_success_rate by known_sessions_count rather than trueSuccessRate.
Ensure you replace references to trueSuccessRate with weightedSuccessRate inside
resolveKPI and the duplicate block noted around the later occurrence.
---
Outside diff comments:
In `@apps/web-next/src/content/docs/guides/dashboard-data-provider.mdx`:
- Around line 59-66: The kpi resolver example (kpi: async (args) => { ... }) is
missing required fields from the current KPI contract; update the returned
object to include dailyNetworkFeesEth (e.g., { value: stats.networkFeesEth,
delta: stats.networkFeesDelta }) and timeframeHours (e.g., { value:
stats.timeframeHours }) alongside existing successRate, orchestratorsOnline,
dailyUsageMins, and dailySessionCount so the example matches the KPI
type/contract.
---
Nitpick comments:
In `@apps/web-next/src/app/`(dashboard)/dashboard/page.tsx:
- Around line 94-95: The GraphQL query in page.tsx is requesting the field
effectiveSuccessRate even though the page does not use it; remove
effectiveSuccessRate from the selection set in the dashboard query (the line
containing "address knownSessions successSessions successRatio
effectiveSuccessRate noSwapRatio slaScore pipelines pipelineModels { pipelineId
modelIds } gpuCount") to avoid unnecessary data fetching and simplify the query.
In `@packages/plugin-sdk/src/contracts/__tests__/dashboard.test.ts`:
- Around line 118-127: The KPI contract test ("KPI type has required MetricDelta
fields") currently asserts only a subset of required fields; update this test to
assert presence of all required KPI MetricDelta fields by adding expect checks
for each missing field using the same pattern (use
buildSchema(DASHBOARD_SCHEMA), getType('KPI') as any, kpiType.getFields() and
then expect(fields).toHaveProperty('<fieldName>') for each required field such
as the ones mentioned plus any others defined in the contract) so the test fails
if any required KPI field is removed or renamed.
In `@plugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts`:
- Around line 414-415: The orchestrator test needs to request and assert the new
effectiveSuccessRate field: update the GraphQL query string inside the test (the
one returning orchestrators { ... }) to include effectiveSuccessRate, cast the
response as DashboardQueryResponse as before, and add assertions in the same
test that validate the orchestrator objects include the expected
effectiveSuccessRate values (alongside existing checks for successRatio,
slaScore, etc.) so the provider integration covers the new field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22232dfb-c7fe-4d85-af02-2e597f4b0eab
📒 Files selected for processing (8)
apps/web-next/src/app/(dashboard)/dashboard/page.tsxapps/web-next/src/content/docs/guides/dashboard-data-provider.mdxpackages/plugin-sdk/src/contracts/__tests__/dashboard.test.tspackages/plugin-sdk/src/contracts/dashboard.tsplugins/dashboard-data-provider/frontend/src/__tests__/provider.test.tsplugins/dashboard-data-provider/frontend/src/api/leaderboard.tsplugins/dashboard-data-provider/frontend/src/provider.tsplugins/service-gateway/connectors/livepeer-leaderboard.json
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web-next/src/app/(dashboard)/dashboard/page.tsx (1)
1223-1227: WirefeesRefreshinginto the Fees card UI path.Line 1223 captures
feesRefreshing, but the Fees card branch (Line 1270-Line 1272) renders withoutRefreshWrap. This makes refresh-state UX inconsistent with the other refreshed cards.♻️ Suggested patch
- {feesData?.fees - ? <FeesCard data={feesData.fees} /> + {feesData?.fees + ? <RefreshWrap refreshing={feesRefreshing}><FeesCard data={feesData.fees} /></RefreshWrap> : feesLoading ? <WidgetSkeleton /> : <WidgetUnavailable label="Fees" />}Also applies to: 1270-1272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/`(dashboard)/dashboard/page.tsx around lines 1223 - 1227, The Fees card branch is not wired to the feesRefreshing state; update the Fees card render (the component/JSX that displays fees around the "Fees card" block) to wrap its contents in the same RefreshWrap used by other cards and pass the feesRefreshing boolean so the UI shows the refresh spinner/state; specifically, use the feesRefreshing value returned from useDashboardQuery (FEES_OVERVIEW_QUERY) and wrap the Fees card JSX with <RefreshWrap refreshing={feesRefreshing}> (or the project's equivalent RefreshWrap usage found on other cards) so the Fees card matches the other refreshed cards' behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/hooks/useDashboardQuery.ts`:
- Around line 45-46: The JSDoc for the loading property in useDashboardQuery is
misleading — update the comment where "loading: boolean" is declared/returned so
it states that loading is true for any in-flight request (initial fetch,
polling, or refetch), not just the very first fetch; locate the declaration
returned by useDashboardQuery and replace the description to explicitly mention
initial and subsequent requests (e.g., "true while any fetch is in progress
(initial load, refetch, or polling)").
In `@plugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts`:
- Around line 53-70: STUB_DEMAND_2H is missing the required model_id property
for the NetworkDemandRow objects; update every object inside STUB_DEMAND_2H to
include a model_id field (set to the appropriate model identifier for that
entry—e.g., same value as pipeline_id or the canonical model string used
elsewhere) so each entry conforms to the NetworkDemandRow type.
- Around line 32-50: STUB_DEMAND_1H entries are missing the model_id required by
the NetworkDemandRow interface; update each object in STUB_DEMAND_1H to include
the model_id property (e.g., model_id: null or the appropriate string
identifier) so the test stub matches the interface contract and covers model_id
handling in the provider code.
---
Nitpick comments:
In `@apps/web-next/src/app/`(dashboard)/dashboard/page.tsx:
- Around line 1223-1227: The Fees card branch is not wired to the feesRefreshing
state; update the Fees card render (the component/JSX that displays fees around
the "Fees card" block) to wrap its contents in the same RefreshWrap used by
other cards and pass the feesRefreshing boolean so the UI shows the refresh
spinner/state; specifically, use the feesRefreshing value returned from
useDashboardQuery (FEES_OVERVIEW_QUERY) and wrap the Fees card JSX with
<RefreshWrap refreshing={feesRefreshing}> (or the project's equivalent
RefreshWrap usage found on other cards) so the Fees card matches the other
refreshed cards' behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff8e1185-4516-4a31-98af-bb422da57689
📒 Files selected for processing (3)
apps/web-next/src/app/(dashboard)/dashboard/page.tsxapps/web-next/src/hooks/useDashboardQuery.tsplugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts
eliteprox
changed the title
qianghan
suggested changes
Mar 6, 2026
Contributor
qianghan
left a comment
qianghan
left a comment
There was a problem hiding this comment.
Code Review — PR #193
Overview
Good work on renaming dailyStreamCount → dailySessionCount, adding effectiveSuccessRate as a new orchestrator metric, updating API interfaces for the latest leaderboard schema, and introducing the RefreshWrap component for a smoother polling UX. CI is all green.
However, there are 2 critical bugs and 3 major issues that need to be addressed before merge.
Critical
| # | Issue | File |
|---|---|---|
| 1 | weightedSuccessRate is dead code — KPI still uses legacy trueSuccessRate |
provider.ts |
| 2 | setData(null) on poll errors wipes stale data, defeating RefreshWrap |
useDashboardQuery.ts |
Major
| # | Issue | File |
|---|---|---|
| 3 | loading JSDoc is inaccurate — says "first fetch" but fires on every fetch |
useDashboardQuery.ts |
| 4 | No error UI for timeout/unknown errors — users see blank widgets with no explanation |
dashboard/page.tsx |
| 5 | Missing model_id field in STUB_DEMAND_1H and STUB_DEMAND_2H test data |
provider.test.ts |
See inline comments for details and requested changes.
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
plugins/dashboard-data-provider/frontend/src/provider.ts (1)
119-125:⚠️ Potential issue | 🟠 MajorKPI success rate is still weighted off the wrong field.
weightedSuccessRatio()now drives the KPI on Lines 173-174, but this helper still multipliesstartup_success_rate. That keeps the headline tile on the startup metric instead of the API’seffective_success_rate, which is what the contract/comment now advertise.Suggested fix
-function weightedSuccessRatio(rows: Array<{ startup_success_rate: number; known_sessions_count: number }>): number { +function weightedSuccessRatio(rows: Array<{ effective_success_rate: number; known_sessions_count: number }>): number { const totalSessions = rows.reduce((s, r) => s + r.known_sessions_count, 0); if (totalSessions === 0) return 0; - return rows.reduce((s, r) => s + r.startup_success_rate * r.known_sessions_count, 0) / totalSessions; + return rows.reduce((s, r) => s + r.effective_success_rate * r.known_sessions_count, 0) / totalSessions; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/dashboard-data-provider/frontend/src/provider.ts` around lines 119 - 125, The helper weightedSuccessRatio is still using startup_success_rate even though the KPI should use effective_success_rate; update the function signature and usages in weightedSuccessRatio to accept rows with effective_success_rate (e.g., change the param type to { effective_success_rate: number; known_sessions_count: number }) and replace occurrences of r.startup_success_rate with r.effective_success_rate so the weighted average is computed against the API's effective_success_rate field.
🧹 Nitpick comments (2)
plugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts (1)
313-341: Please add integration coverage for the new raw explorer fields.These tests now cover
dailySessionCountandeffectiveSuccessRate, but they still never querynetworkDemand,gpuMetrics,slaCompliance, orpipelineCatalog.regions. That leaves the new camelCase transforms and filter plumbing inplugins/dashboard-data-provider/frontend/src/provider.tsunguarded.Also applies to: 429-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts` around lines 313 - 341, The test is missing assertions for the new raw explorer fields; update the DashboardQueryRequest (the `request` query string in provider.test.ts) to include networkDemand, gpuMetrics, slaCompliance, and pipelineCatalog { regions } (or pipelineCatalog.regions) in the GraphQL query, then after invoking `testEventBus._invoke(DASHBOARD_QUERY_EVENT, request)` assert those fields exist on `response.data` and validate basic types/shape (e.g., numbers or arrays/objects) similar to the existing KPI checks; reference the test identifiers `request`, `DASHBOARD_QUERY_EVENT`, `testEventBus._invoke`, and `DashboardQueryResponse` when locating where to add the new query selections and corresponding expect(...) assertions.examples/network-analytics/frontend/src/pages/RawExplorer.tsx (1)
1147-1162: Avoid fetching all raw datasets for every tab.The page always issues SLA, GPU, and demand queries, plus three copies of
pipelineCatalog, even when a single tab is visible. On larger result sets that will add a lot of unnecessary load and makes the 8s timeout inuseDashboardQuery()much easier to hit. Anenabledflag per query, plus a small standalone catalog query, would keep this much cheaper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/network-analytics/frontend/src/pages/RawExplorer.tsx` around lines 1147 - 1162, Currently SLA_QUERY, GPU_QUERY, and DEMAND_QUERY are all executed every render (via useDashboardQuery for slaVars/gpuVars/demandVars) and their pipelineCatalog fields are merged into catalog; change each useDashboardQuery call to accept an enabled flag that is true only when its corresponding tab is active (e.g. when activeTab === 'sla' enable SLA_QUERY), and create a small standalone pipelineCatalog query (e.g. PIPELINE_CATALOG_QUERY) using useDashboardQuery(enabled: true) to fetch only the catalog for use across tabs; update the catalog assignment to use the standalone catalog result instead of slaData?.pipelineCatalog ?? gpuData?.pipelineCatalog ?? demandData?.pipelineCatalog and remove unnecessary query executions when tabs are not visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/app/`(dashboard)/dashboard/page.tsx:
- Around line 1219-1225: The state initializer is reading localStorage during
render (via getStoredJobFeedPollInterval), causing a server/client hydration
mismatch; change to initialize jobFeedPollInterval (and similarly timeframe)
with the SSR-safe defaults (e.g., DEFAULT_POLL_INTERVAL/DEFAULT_TIMEFRAME) and
move the localStorage reads into a useEffect that runs on mount which calls
getStoredJobFeedPollInterval/getStoredTimeframe and then calls
setJobFeedPollInterval/setTimeframe to update state and persist any derived
values; keep handleJobFeedPollIntervalChange as-is for writes.
In `@apps/web-next/src/hooks/useJobFeedStream.ts`:
- Around line 127-139: The timer path tears down the active event listener
before establishing the replacement; instead, avoid calling cleanupRef.current
until the new subscription is confirmed: when the poll timer fires, capture the
old cleanup (const oldCleanup = cleanupRef.current), then call connect() and
wait for it to successfully create/return the new cleanup (or have connect
accept a callback/promise to signal success), only after the new cleanup is set
on cleanupRef.current call oldCleanup(); ensure pollTimerRef/mountedRef logic
remains but never null out cleanupRef before the swap. Reference symbols:
pollTimerRef, cleanupRef, connect, eventBusCleanup, pollIntervalMs, mountedRef.
In `@examples/network-analytics/frontend/src/pages/RawExplorer.tsx`:
- Around line 1064-1114: The component currently seeds dataset/filters/period
once (initialDataset/initialFilters/initialPeriod) so URL changes/back-forward
can desync and an invalid exp_ds can later crash DATASET_CONFIG[dataset]; fix by
deriving state from searchParams on change: replace the one-time initial_* usage
with a useEffect that reads parseFiltersFromParams(searchParams), validates the
dataset string against DATASET_CONFIG keys, and calls
setDataset/setFilters/setPeriod when searchParams changes; also add a runtime
guard where DATASET_CONFIG[dataset] is accessed (or default to a safe dataset)
to avoid exceptions if an invalid dataset slips through.
- Around line 1125-1136: gpuTimeRange currently collapses every non-'1h' period
to '24h', causing hardware/models tabs to misrepresent selected periods; update
the logic in RawExplorer.tsx where gpuTimeRange and gpuVars are defined to
either (A) pass the selected period through (set gpuTimeRange = period) so
gpuVars.timeRange reflects the actual picker value, or (B) explicitly map
allowed picker values to supported GPU ranges (e.g., map
'1h'→'1h','6h'→'6h','24h'→'24h','72h'→'72h') and ensure the UI picker is
restricted to those supported values; change the gpuTimeRange computation and
ensure gpuVars.timeRange uses that updated value so hardware and models tabs
display the correct range.
- Around line 991-1005: The current logic only sets an error when
response.errors exists AND response.data is falsy, which hides partial GraphQL
failures (e.g., main field failed but pipelineCatalog succeeded); update the
conditional around setState in RawExplorer.tsx so that whenever response.errors
&& response.errors.length > 0 you populate the error object (using
response.errors.map(e=>e.message).join('; ')) while still preserving
response.data into setState.data (cast to T) instead of clearing it; only set
error: null when there are no response.errors. Ensure you modify the branches
that call setState (the ones referencing response.errors, response.data, and
pipelineCatalog) so partial data + errors are represented.
- Around line 435-438: The SLA score is being multiplied by 100 again when
rendering, causing values like 10000; in RawExplorer.tsx update the rendering of
row.slaScore (from RawSLAComplianceRow) to stop scaling it — remove the
multiplication by 100 and simply format the existing percentage-style value
(e.g., keep toFixed(0) or use formatPercent if you want consistent formatting)
so a perfect score remains 100 instead of 10000.
In `@examples/network-analytics/frontend/tailwind.config.js`:
- Line 3: The Tailwind darkMode is configured as ['class'] but the app never
toggles the .dark class so dark styles in
examples/network-analytics/frontend/src/globals.css (defined under .dark) never
apply; add a small bootstrap that reads the user preference or stored choice and
sets document.documentElement.classList.toggle('dark', ...) on initial load
(same approach used in apps/web-next/src/app/layout.tsx) and wire up any theme
toggle UI to update that class and persist the choice.
In `@examples/network-analytics/plugin.json`:
- Around line 25-28: The PLUGIN_ROUTE_MAP still contains the stale mapping
'/leaderboard': 'networkAnalytics' which no longer exists in the plugin.json
routes; open the PLUGIN_ROUTE_MAP constant (symbol name PLUGIN_ROUTE_MAP) and
remove the '/leaderboard' and '/leaderboard/*' entries (or alternately add the
missing routes back into the plugin's WorkflowPlugin.routes) so the hardcoded
route map matches the plugin.json routes and the middleware rewrite logic is
consistent.
---
Duplicate comments:
In `@plugins/dashboard-data-provider/frontend/src/provider.ts`:
- Around line 119-125: The helper weightedSuccessRatio is still using
startup_success_rate even though the KPI should use effective_success_rate;
update the function signature and usages in weightedSuccessRatio to accept rows
with effective_success_rate (e.g., change the param type to {
effective_success_rate: number; known_sessions_count: number }) and replace
occurrences of r.startup_success_rate with r.effective_success_rate so the
weighted average is computed against the API's effective_success_rate field.
---
Nitpick comments:
In `@examples/network-analytics/frontend/src/pages/RawExplorer.tsx`:
- Around line 1147-1162: Currently SLA_QUERY, GPU_QUERY, and DEMAND_QUERY are
all executed every render (via useDashboardQuery for slaVars/gpuVars/demandVars)
and their pipelineCatalog fields are merged into catalog; change each
useDashboardQuery call to accept an enabled flag that is true only when its
corresponding tab is active (e.g. when activeTab === 'sla' enable SLA_QUERY),
and create a small standalone pipelineCatalog query (e.g.
PIPELINE_CATALOG_QUERY) using useDashboardQuery(enabled: true) to fetch only the
catalog for use across tabs; update the catalog assignment to use the standalone
catalog result instead of slaData?.pipelineCatalog ?? gpuData?.pipelineCatalog
?? demandData?.pipelineCatalog and remove unnecessary query executions when tabs
are not visible.
In `@plugins/dashboard-data-provider/frontend/src/__tests__/provider.test.ts`:
- Around line 313-341: The test is missing assertions for the new raw explorer
fields; update the DashboardQueryRequest (the `request` query string in
provider.test.ts) to include networkDemand, gpuMetrics, slaCompliance, and
pipelineCatalog { regions } (or pipelineCatalog.regions) in the GraphQL query,
then after invoking `testEventBus._invoke(DASHBOARD_QUERY_EVENT, request)`
assert those fields exist on `response.data` and validate basic types/shape
(e.g., numbers or arrays/objects) similar to the existing KPI checks; reference
the test identifiers `request`, `DASHBOARD_QUERY_EVENT`, `testEventBus._invoke`,
and `DashboardQueryResponse` when locating where to add the new query selections
and corresponding expect(...) assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52b2d951-6c8e-42c7-b9c6-495943fd1ba4
📒 Files selected for processing (16)
apps/web-next/src/app/(dashboard)/dashboard/page.tsxapps/web-next/src/app/(dashboard)/settings/page.tsxapps/web-next/src/hooks/useDashboardQuery.tsapps/web-next/src/hooks/useJobFeedStream.tsexamples/network-analytics/frontend/src/App.tsxexamples/network-analytics/frontend/src/globals.cssexamples/network-analytics/frontend/src/pages/Analytics.tsxexamples/network-analytics/frontend/src/pages/Leaderboard.tsxexamples/network-analytics/frontend/src/pages/RawExplorer.tsxexamples/network-analytics/frontend/tailwind.config.jsexamples/network-analytics/plugin.jsonpackages/plugin-sdk/src/contracts/createDashboardProvider.tspackages/plugin-sdk/src/contracts/dashboard.tsplugins/dashboard-data-provider/frontend/src/__tests__/provider.test.tsplugins/dashboard-data-provider/frontend/src/api/leaderboard.tsplugins/dashboard-data-provider/frontend/src/provider.ts
💤 Files with no reviewable changes (2)
- examples/network-analytics/frontend/src/pages/Analytics.tsx
- examples/network-analytics/frontend/src/pages/Leaderboard.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web-next/src/app/(dashboard)/settings/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web-next/src/hooks/useDashboardQuery.ts
eliteprox
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web-next/src/app/(dashboard)/dashboard/page.tsx (1)
1219-1220:⚠️ Potential issue | 🟡 MinorHydration mismatch:
localStorageread inuseStateinitializer.Both
getStoredJobFeedPollIntervalandgetStoredTimeframeread fromlocalStorageduring the initial render. In Next.js App Router, this causes server/client mismatches since the server renders with defaults while the client may have stored values.This was flagged in a previous review and appears unresolved. The fix is to initialize with defaults and sync from
localStoragein auseEffect.Hydration-safe pattern
- const [jobFeedPollInterval, setJobFeedPollInterval] = useState(getStoredJobFeedPollInterval); - const [timeframe, setTimeframe] = useState(getStoredTimeframe); + const [jobFeedPollInterval, setJobFeedPollInterval] = useState(DEFAULT_POLL_INTERVAL); + const [timeframe, setTimeframe] = useState(DEFAULT_TIMEFRAME); + + useEffect(() => { + setJobFeedPollInterval(getStoredJobFeedPollInterval()); + setTimeframe(getStoredTimeframe()); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/`(dashboard)/dashboard/page.tsx around lines 1219 - 1220, The current useState initializers call getStoredJobFeedPollInterval and getStoredTimeframe which read localStorage during render and cause hydration mismatches; change both useState calls to initialize with safe defaults (e.g., the server-side fallback value) and then inside a useEffect (guarded by typeof window !== 'undefined') read localStorage via getStoredJobFeedPollInterval and getStoredTimeframe and call setJobFeedPollInterval and setTimeframe to sync the client values; keep the same state variable names (jobFeedPollInterval, timeframe) and helper functions so the diff is minimal.
🧹 Nitpick comments (1)
plugins/dashboard-data-provider/frontend/src/provider.ts (1)
256-259: Fieldminsnow holds GPU count, not minutes.The
minsfield in the returnedDashboardPipelineUsageobjects now contains GPU counts (line 258:mins: acc.total) rather than minutes. While the UI has been updated to display "GPUs", consumers of this API might be confused by the field name.Consider renaming the field in a future refactor, or at minimum document this semantic change in the type definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/dashboard-data-provider/frontend/src/provider.ts` around lines 256 - 259, The mapping that builds DashboardPipelineUsage objects currently sets mins: acc.total but acc.total now represents GPU count, causing a semantic mismatch; update the mapping in the function that returns these objects (the .map(([pipelineId, acc]) => ({ ... })) using PIPELINE_DISPLAY, PIPELINE_COLOR, DEFAULT_PIPELINE_COLOR) to include a new explicit field like gpus: acc.total and preserve mins for backward compatibility (or update the type DashboardPipelineUsage to reflect the new semantics), and add a short comment documenting the change in the type definition so consumers know mins now represents GPUs or can migrate to gpus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/dashboard-data-provider/frontend/src/provider.ts`:
- Around line 118-126: The comment for weightedSuccessRatio and its
implementation disagree: the function currently computes a weighted average
using the startup_success_rate property but the docstring mentions
effective_success_rate; decide which KPI is correct and make them consistent by
either changing the comment to say "Weighted average of startup_success_rate by
known_sessions_count" or updating the implementation and the rows type to use
effective_success_rate instead of startup_success_rate (update occurrences of
startup_success_rate in the function signature and reduce callbacks to
effective_success_rate if you choose that path).
---
Duplicate comments:
In `@apps/web-next/src/app/`(dashboard)/dashboard/page.tsx:
- Around line 1219-1220: The current useState initializers call
getStoredJobFeedPollInterval and getStoredTimeframe which read localStorage
during render and cause hydration mismatches; change both useState calls to
initialize with safe defaults (e.g., the server-side fallback value) and then
inside a useEffect (guarded by typeof window !== 'undefined') read localStorage
via getStoredJobFeedPollInterval and getStoredTimeframe and call
setJobFeedPollInterval and setTimeframe to sync the client values; keep the same
state variable names (jobFeedPollInterval, timeframe) and helper functions so
the diff is minimal.
---
Nitpick comments:
In `@plugins/dashboard-data-provider/frontend/src/provider.ts`:
- Around line 256-259: The mapping that builds DashboardPipelineUsage objects
currently sets mins: acc.total but acc.total now represents GPU count, causing a
semantic mismatch; update the mapping in the function that returns these objects
(the .map(([pipelineId, acc]) => ({ ... })) using PIPELINE_DISPLAY,
PIPELINE_COLOR, DEFAULT_PIPELINE_COLOR) to include a new explicit field like
gpus: acc.total and preserve mins for backward compatibility (or update the type
DashboardPipelineUsage to reflect the new semantics), and add a short comment
documenting the change in the type definition so consumers know mins now
represents GPUs or can migrate to gpus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5ca464d-43d4-41fd-98e9-6ffd7761d35a
📒 Files selected for processing (3)
apps/web-next/src/app/(dashboard)/dashboard/page.tsxplugins/dashboard-data-provider/frontend/src/__tests__/provider.test.tsplugins/dashboard-data-provider/frontend/src/provider.ts
- Updated comments and documentation for clarity on cache warming and timeout settings in the dashboard. - Adjusted TTL settings for various endpoints to ensure consistency and improve cache management. - Enhanced GraphQL query timeout documentation to reflect changes in client-side handling and upstream fetch behavior. - Improved clarity in the raw data handling and KPI resolution logic to align with updated caching strategies.
This reverts commit 1959f3e.
…ents - Updated the layout of ProtocolCard, FeesCard, and GPUCapacityCard to use flexbox for better responsiveness and alignment. - Adjusted the minimum height and flex properties of various elements to ensure consistent sizing and appearance. - Improved the table header component in OrchestratorTableCard by allowing additional class names for better styling flexibility. - Enhanced the RefreshWrap component usage to maintain consistent height and layout across different dashboard sections.
… configuration - Refactored leaderboard API integration to utilize a new utility for constructing upstream URLs, enhancing maintainability. - Updated environment variable references in `.env.example` and `.env.local.example` to clarify usage and requirements for `LEADERBOARD_API_URL`. - Adjusted various components to ensure they correctly reference the new leaderboard API structure, including updates to the service gateway and dashboard data provider. - Improved documentation for environment variables and their expected formats in the README files.
- Add `callConnectorInternal` function to facilitate server-side calls to published connectors without going through the external gateway. - Introduce tests for the internal client to ensure proper functionality and error handling. - Update leaderboard and ClickHouse configurations to support new internal calling mechanism. - Modify existing seed scripts and environment variable handling to align with new API structure. - Refactor endpoint paths to remove unnecessary prefixes and ensure consistency across connectors.
- Added a new seed script for public connectors that reads from JSON templates and upserts them into the database. - Integrated the seeding process into the main Prisma seed file. - Updated the internal client to handle connector calls more robustly. - Refactored the leaderboard and ClickHouse connector handling. - Improved test setup for server-only modules. - Removed the old public connector seeding logic from the start script.
…_API_URL validation
…nd pipeline names
…nd improve error handling
Contributor
Author
|
closing in favor of #224 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates the dashboard stack to align with the latest Livepeer Leaderboard API contract, improves metric correctness/terminology, and tightens dashboard refresh/error behavior for a more reliable UX.
What changed
Leaderboard API/schema changes introduced naming and field-shape drift. This PR brings dashboard contracts and rendering back into sync, reduces ambiguity in GPU/pipeline metrics, and improves user trust during polling/refetch scenarios.
Metrics contract alignment
dailyStreamCounttodailySessionCountacross schema/provider/UI paths.effectiveSuccessRate.Pipelines + GPU semantics
Dashboard UX and resiliency
Impact
Validation
Summary by CodeRabbit
Release Notes
New Features
Metrics & Data
UI Updates