feat(miners): add miner detail page and redesign leaderboard with OSS/Discovery mode#102
Closed
Dexterity104 wants to merge 3 commits into
Closed
feat(miners): add miner detail page and redesign leaderboard with OSS/Discovery mode#102Dexterity104 wants to merge 3 commits into
Dexterity104 wants to merge 3 commits into
Conversation
Owner
|
The UI is exactly similar to Gittensor Dashboard but I dont want it. |
Owner
|
thanks for your contribution, but I dont think it's a better one since I am a fan of clean UI |
Owner
|
thanks, but there is much dead space on the page |
Owner
|
looks better than before, but still has much dead space and the UI should be more professional and user friendly |
gittensor-hub-bot
Bot
added
the
rating: 🌊 off-meta tidepool
|
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 backend aggregation and SWR caches for miners/PRs, per-miner evaluation enrichment, SQLite rank snapshots, a new client UI component library, a refactored miners leaderboard (Insights + LeaderTable + Toolbar), and detailed miner and PR pages with paginated PR/issue lists, charts, and interactive controls. ChangesMiners Section Redesign
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types/entities.ts (1)
176-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
Minernullability with upstream payload.
githubUsernameis typed asstring, but upstream miner data is nullable in the API layer. This contract mismatch can lead to unsafe assumptions and null dereferences in consumers.Suggested fix
- githubUsername: string; + githubUsername: string | null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/entities.ts` around lines 176 - 177, The Miner type's githubUsername field is non-nullable but upstream can return null; change the Miner definition so githubUsername is typed as string | null (rather than string) and update any consumers of Miner.githubUsername to handle the null case safely (e.g., guard checks or fallback logic); reference the Miner type and the githubUsername field in src/types/entities.ts when making this change.
🧹 Nitpick comments (5)
src/app/miners/page.tsx (1)
3-3: 💤 Low value
export const dynamichas no effect in a Client Component.This directive is ignored in files marked
'use client'. In Next.js App Router,dynamic,revalidate, and similar exports only affect Server Components and Route Handlers. Since this page is a Client Component, the export is dead code.Either remove the export or, if you need server-side dynamic behavior, create a Server Component wrapper that imports this Client Component.
Suggested fix
'use client'; -export const dynamic = 'force-dynamic'; - import React, { useCallback, useEffect, useMemo, useState } from 'react';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/page.tsx` at line 3, The file exports a Server-only directive ("export const dynamic = 'force-dynamic'") inside a Client Component, which is ignored; remove the dead export from page.tsx (the "dynamic" export) or move the client UI into a dedicated Client Component and create a Server Component wrapper (e.g., a new ServerComponent that imports the existing Client component) where you can place "export const dynamic = 'force-dynamic'"; update the module to either drop the export or relocate it to the server wrapper so the directive takes effect.src/app/miners/LeaderTable.tsx (1)
355-369: 💤 Low valueUnused props:
totalItemsandtotalAllare declared but never rendered.
ToolbarPropsincludestotalItemsandtotalAll, and these values are passed from the parent component, but theToolbarfunction never uses them in its JSX output. If these are intended for a "Showing X of Y" label, consider implementing it. Otherwise, remove the unused props to avoid confusion.Also applies to: 371-454
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/LeaderTable.tsx` around lines 355 - 369, ToolbarProps declares totalItems and totalAll but the Toolbar component never uses them; either remove those props from the interface and callers or render a "Showing X of Y" label inside the Toolbar. To fix: update ToolbarProps (remove totalItems/totalAll) and adjust parent where Toolbar is instantiated, or add markup inside the Toolbar function that reads props.totalItems and props.totalAll (e.g., near the search/filters area) and formats a brief "Showing {totalItems} of {totalAll}" string; ensure to update any TypeScript types/usages referencing ToolbarProps accordingly.src/app/miners/components/CountCell.tsx (2)
33-33: 💤 Low valueLift
padobject out of render to avoid re-creating on every render.The
padobject is computed inline and will be re-created on each render. Sincesizeis a prop, you can memoize or define the size map outside the component.♻️ Lift size map
+const SIZE_MAP = { + md: { px: '8px', py: '3px', fz: '11px' }, + sm: { px: '6px', py: '2px', fz: '10px' }, +} as const; + export function EligibilityBadge({ eligible, label, size = 'sm', }: { eligible: boolean; label: string; size?: 'sm' | 'md'; }) { - const pad = size === 'md' ? { px: '8px', py: '3px', fz: '11px' } : { px: '6px', py: '2px', fz: '10px' }; + const pad = SIZE_MAP[size]; return (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/components/CountCell.tsx` at line 33, The inline `pad` object in the CountCell component is re-created on every render; move the size map out of the render by defining a constant size-to-pad map outside the CountCell function (or memoize it using useMemo keyed on `size`) and then reference it inside the component; update references to `pad` in CountCell to use the external constant or the memoized value so the map isn't reconstructed each render (look for the `pad` definition and the `CountCell` component).
32-32: 💤 Low valueConsider moving inline styles into
sxprop.Lines 32 and 36 use inline
styleto setopacityandcolor. While functional, consolidating these into thesxprop would be more consistent with the rest of the Primer styling pattern used throughout this component.♻️ Consolidate styles
<Box title={title} sx={{ display: 'inline-flex', alignItems: 'center', justifyContent: 'flex-end', gap: '4px', minWidth: 0, + opacity: empty ? 0.55 : 1, }} - style={{ opacity: empty ? 0.55 : 1 }} > <Box - sx={{ display: 'inline-flex', flexShrink: 0 }} - style={{ color: empty ? 'var(--fg-muted)' : TONE_FG[tone] }} + sx={{ + display: 'inline-flex', + flexShrink: 0, + color: empty ? 'fg.muted' : undefined, + }} + style={!empty ? { color: TONE_FG[tone] } : undefined} >Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/components/CountCell.tsx` at line 32, In the CountCell component replace the inline style props (e.g., style={{ opacity: empty ? 0.55 : 1 }} and style={{ color: 'var(--contrast-2)' }}) with equivalent sx entries on the same components; update the component(s) that render opacity and color (search for CountCell and the elements using the inline style) to use sx={{ opacity: empty ? 0.55 : 1 }} and sx={{ color: 'var(--contrast-2)' }} respectively and remove the style attributes so styling is consistent with the Primer sx pattern.src/app/miners/[uid]/prs/[...slug]/page.tsx (1)
1-3: ⚡ Quick winTreat
dynamicconfig as route-level (not an invalid Client export)Next.js processes route segment config exports like
export const dynamicfor the route file even when it’s marked with'use client', so this isn’t simply “ignored.” Since this page fetches on the client (React Query),force-dynamicmay be redundant and primarily impacts route/HTML caching—keep it only if you specifically need to force dynamic rendering for caching/revalidation behavior.Optional cleanup
'use client'; -export const dynamic = 'force-dynamic'; - import React, { use, useMemo } from 'react';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/prs/[...slug]/page.tsx around lines 1 - 3, The file exports "export const dynamic = 'force-dynamic'" alongside a 'use client' page—Next.js treats route-level config exports like dynamic even in client files, so either remove the export if you don't need to force dynamic rendering/caching behavior, or keep it intentionally and document why; locate the "export const dynamic" statement in the page (next to the 'use client' directive) and either delete that export to rely on client-side React Query rendering or retain it only if you explicitly require force-dynamic semantics for route caching/revalidation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/api/gt/miners/`[uid]/route.ts:
- Around line 304-307: The current parsing of uid from uidParam using
Number.parseInt allows partially numeric strings (e.g., "12abc") to become valid
uids; update validation to ensure uidParam is strictly an integer string (e.g.,
match /^\d+$/) before parsing, reject with the same NextResponse.json({ error:
'Invalid uid' }, { status: 400 }) if it fails, then safely convert to a Number
(assign to uid) and continue; target the uidParam/uid parsing block in route.ts
and replace the loose parseInt check with this strict string-digit validation.
In `@src/app/api/miners/route.ts`:
- Around line 395-396: fetchPerMinerCounts currently returns zeroed counts on
fetch errors which causes the refresh logic to treat transient failures as
definite ineligibility; change the flow so fetchPerMinerCounts distinguishes a
real zero result from an error (e.g., return null/undefined or an { error: true
} sentinel) and update the refresh logic that sets isEligible/isIssueEligible to
only overwrite those flags when the fetch returned a successful response.
Specifically, modify fetchPerMinerCounts to surface failure (not coerce to
{oss:0,disc:0}) and in the refresh code that assigns miner.isEligible /
miner.isIssueEligible (the block that consumes fetchPerMinerCounts), skip
updating those fields when the fetch indicates an error so existing eligibility
is preserved on transient upstream failures.
- Around line 439-468: The current enrichment fans out requests by calling
fetchPerMinerCounts for each upstreamMiners entry inside Promise.all, which can
spike concurrent external calls; replace the unbounded Promise.all pattern with
a bounded concurrency executor (e.g., p-limit or a PromisePool) so
fetchPerMinerCounts calls are limited (e.g., 5-10 concurrent); keep the same
mapping logic for baseEnrich and preserve ordering by collecting the limited
promises into enriched before awaiting Promise.all; update the block around
upstreamMiners.map / Promise.all and references to fetchPerMinerCounts to use
the concurrency limiter.
- Around line 82-84: The catch block in src/app/api/miners/route.ts currently
swallows all errors and returns prsCache?.prs ?? [], which yields an empty array
on cold-start and incorrectly marks the endpoint as live; change the catch to
capture the error (catch (err)), return prsCache.prs only when prsCache?.prs is
present, and otherwise rethrow the captured error so the caller returns an error
response (or can handle logging/status) instead of silently returning []—update
the catch for the code that references prsCache and prs to rethrow when no
cached data exists and optionally log err before throwing.
In `@src/app/miners/`[uid]/components/PrList.tsx:
- Around line 250-271: The modal container in PrList is missing proper
accessibility semantics and focus handling; update the outer and inner Box
elements in the PrList component to include role="dialog", aria-modal="true",
and an aria-labelledby or aria-label tied to the modal title, then implement
focus management in PrList: on mount set initial focus to a logical element
(e.g., the close button or first interactive item), trap focus inside the modal
(handle Tab/Shift+Tab), close on Escape and restore focus to the previously
focused element on unmount; reuse the same changes for the other modal instance
referenced around lines 297-311. Ensure the existing onClick/onClose behavior
still stops propagation and that aria attributes reference the unique title/id
you add.
In `@src/app/miners/`[uid]/components/RepoBreakdown.tsx:
- Around line 347-401: The row currently renders a clickable <Box as="button">
(in RepoBreakdown.tsx, the Box with onClick={onSelect}) that contains a nested
<Link> (<a>), which is invalid; change the outer Box to a non-button interactive
container (e.g., as="div" with role="button" and tabIndex={0}) and add key
handlers to call onSelect on Enter/Space so keyboard users still activate the
row, leaving the inner Link intact as a normal anchor; update the Box props
where you find onClick={onSelect}, as="button" and the styling (retain
bg/isSelected/boxShadow) and add onKeyDown={(e)=>{ if
(e.key==='Enter'||e.key===' ') { e.preventDefault(); onSelect(); } }} to
preserve behavior without nesting interactive elements.
In `@src/app/miners/`[uid]/components/shared.tsx:
- Around line 123-128: The useMemo for the filtered constant in shared.tsx
currently omits the filter function from its dependency array, risking stale
closures when callers pass unstable filter props; update the dependency array
for the useMemo that computes filtered to include filter (i.e., change [items,
search] to [items, search, filter]) and remove the related eslint-disable
comment so React will re-run the memo if the filter reference changes, ensuring
filter(i, q) always uses the current function.
- Around line 10-27: The ListLoading component's container (the Box in function
ListLoading) lacks screen-reader attributes; update the Box element in
ListLoading to include role="status" and aria-live="polite" so assistive
technologies announce the loading state (add these attributes directly to the
Box JSX element).
- Line 96: The current empty-state detection only checks for 0 and "0" and
misses null, undefined and false, so update the expression that defines the
empty variable (const empty = ...) in shared.tsx to also treat null, undefined
and boolean false as empty (e.g., include checks for value == null and value ===
false) so those React.ReactNode values receive the empty styling.
In `@src/app/miners/`[uid]/page.tsx:
- Around line 85-87: The code currently preserves previous miner data across UID
navigation via placeholderData: (prev) => prev (and possibly keepPreviousData),
which can show the wrong profile and cause actions to target stale data; update
the useQuery options in the page (the query that uses the UID in its queryKey)
to stop showing previous data by removing or setting placeholderData to
undefined and ensuring keepPreviousData is false (leave refetchInterval as-is),
and apply the same change to the other similar useQuery calls referenced (the
occurrences around the other lines noted).
In `@src/app/miners/`[uid]/prs/[...slug]/page.tsx:
- Around line 320-322: The current daysSinceMerge calculation can produce NaN if
pr.mergedAt is an invalid date string; update the logic around daysSinceMerge
(used by TimeDecayChart) to robustly parse and validate the timestamp: obtain
the numeric time via Date.parse or new Date(pr.mergedAt).getTime(), check with
Number.isFinite (or !Number.isNaN) before computing (Date.now() - parsed) /
(1000*60*60*24), and if the parse is invalid set daysSinceMerge to null (or
another safe fallback) so TimeDecayChart never receives NaN coordinates; update
any downstream usage to expect null/valid number.
In `@src/app/miners/components/Bars.tsx`:
- Around line 101-103: Clamp each track score before using them to compute
percentages: create non-negative versions (e.g., clampedOss = Math.max(0,
ossScore) and clampedDisc = Math.max(0, discScore)), compute total from those
clamped values, and then compute ossPct and discPct from clampedOss/total and
clampedDisc/total with the existing total>0 guard; update references in Bars.tsx
that use ossPct and discPct so segment widths never receive negative values.
In `@src/app/miners/components/Pagination.tsx`:
- Around line 29-30: The range math in Pagination (and PageNav) uses pageSize
directly so when pageSize === Infinity computations like (p1 - 1) * pageSize
produce NaN; fix by deriving an effectivePageSize = Number.isFinite(pageSize) ?
pageSize : filtered (or total items) and use that in the start/end calculations
(and anywhere else p1 * pageSize is used) so start = showRange ? (p1 - 1) *
effectivePageSize + 1 : 0 and end = showRange ? Math.min(p1 * effectivePageSize,
filtered) : 0, ensuring the “All” case renders the full filtered count
correctly.
In `@src/app/miners/components/SearchBox.tsx`:
- Around line 40-44: The input in SearchBox.tsx (the input rendered with props
value, placeholder and onChange) lacks a programmatic label; add an accessible
name by either rendering a visible <label> tied to the input's id or by adding a
clear aria-label/aria-labelledby on the input (e.g., in the SearchBox component
ensure the input receives an id and a corresponding label or add aria-label prop
when no visible label is desired), update the component signature/JSX to forward
the id/aria-label prop and ensure onChange and placeholder behavior remain
unchanged.
In `@src/app/miners/components/Segmented.tsx`:
- Around line 28-49: The current Segmented component uses tablist/tab semantics
without implementing full tabs behavior; change to button-group semantics by
replacing role="tablist" with role="group" (or removing tab roles) on the
container and remove role="tab" and aria-selected from each option in
options.map; instead set each button's aria-pressed={active} and keep
onClick={() => onChange(opt.key)} so assistive tech treats these as a segmented
button group; update references in the Segmented component (ariaLabel, options,
value, onChange, and the Box as="button" elements) accordingly.
In `@src/lib/format.ts`:
- Around line 52-56: The compact branch currently always calls n.toFixed(2)
which drops precision for sub-dollar values; update the compact formatting logic
(the block handling style === 'compact' and variable n) to use n.toFixed(4) when
n < 1 and n.toFixed(2) otherwise, then apply the existing /\.?0+$/ replacement
to strip trailing zeros so outputs match the documented contract (use 4 decimals
for values < 1, 2 decimals otherwise).
---
Outside diff comments:
In `@src/types/entities.ts`:
- Around line 176-177: The Miner type's githubUsername field is non-nullable but
upstream can return null; change the Miner definition so githubUsername is typed
as string | null (rather than string) and update any consumers of
Miner.githubUsername to handle the null case safely (e.g., guard checks or
fallback logic); reference the Miner type and the githubUsername field in
src/types/entities.ts when making this change.
---
Nitpick comments:
In `@src/app/miners/`[uid]/prs/[...slug]/page.tsx:
- Around line 1-3: The file exports "export const dynamic = 'force-dynamic'"
alongside a 'use client' page—Next.js treats route-level config exports like
dynamic even in client files, so either remove the export if you don't need to
force dynamic rendering/caching behavior, or keep it intentionally and document
why; locate the "export const dynamic" statement in the page (next to the 'use
client' directive) and either delete that export to rely on client-side React
Query rendering or retain it only if you explicitly require force-dynamic
semantics for route caching/revalidation.
In `@src/app/miners/components/CountCell.tsx`:
- Line 33: The inline `pad` object in the CountCell component is re-created on
every render; move the size map out of the render by defining a constant
size-to-pad map outside the CountCell function (or memoize it using useMemo
keyed on `size`) and then reference it inside the component; update references
to `pad` in CountCell to use the external constant or the memoized value so the
map isn't reconstructed each render (look for the `pad` definition and the
`CountCell` component).
- Line 32: In the CountCell component replace the inline style props (e.g.,
style={{ opacity: empty ? 0.55 : 1 }} and style={{ color: 'var(--contrast-2)'
}}) with equivalent sx entries on the same components; update the component(s)
that render opacity and color (search for CountCell and the elements using the
inline style) to use sx={{ opacity: empty ? 0.55 : 1 }} and sx={{ color:
'var(--contrast-2)' }} respectively and remove the style attributes so styling
is consistent with the Primer sx pattern.
In `@src/app/miners/LeaderTable.tsx`:
- Around line 355-369: ToolbarProps declares totalItems and totalAll but the
Toolbar component never uses them; either remove those props from the interface
and callers or render a "Showing X of Y" label inside the Toolbar. To fix:
update ToolbarProps (remove totalItems/totalAll) and adjust parent where Toolbar
is instantiated, or add markup inside the Toolbar function that reads
props.totalItems and props.totalAll (e.g., near the search/filters area) and
formats a brief "Showing {totalItems} of {totalAll}" string; ensure to update
any TypeScript types/usages referencing ToolbarProps accordingly.
In `@src/app/miners/page.tsx`:
- Line 3: The file exports a Server-only directive ("export const dynamic =
'force-dynamic'") inside a Client Component, which is ignored; remove the dead
export from page.tsx (the "dynamic" export) or move the client UI into a
dedicated Client Component and create a Server Component wrapper (e.g., a new
ServerComponent that imports the existing Client component) where you can place
"export const dynamic = 'force-dynamic'"; update the module to either drop the
export or relocate it to the server wrapper so the directive takes effect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f807921-f590-4d2c-a47c-d2914b82f71c
📒 Files selected for processing (35)
src/app/api/gt/miners/[uid]/route.tssrc/app/api/miners/route.tssrc/app/globals.csssrc/app/miners/Insights.tsxsrc/app/miners/LeaderTable.tsxsrc/app/miners/[uid]/components/ActivitySummary.tsxsrc/app/miners/[uid]/components/CodeImpactCard.tsxsrc/app/miners/[uid]/components/IssueList.tsxsrc/app/miners/[uid]/components/PositionSummary.tsxsrc/app/miners/[uid]/components/PrList.tsxsrc/app/miners/[uid]/components/ProfileHero.tsxsrc/app/miners/[uid]/components/RepoBreakdown.tsxsrc/app/miners/[uid]/components/index.tssrc/app/miners/[uid]/components/shared.tsxsrc/app/miners/[uid]/components/types.tssrc/app/miners/[uid]/page.tsxsrc/app/miners/[uid]/prs/[...slug]/page.tsxsrc/app/miners/components/Bars.tsxsrc/app/miners/components/Card.tsxsrc/app/miners/components/CountCell.tsxsrc/app/miners/components/EligibilityBadge.tsxsrc/app/miners/components/EmptyState.tsxsrc/app/miners/components/Metric.tsxsrc/app/miners/components/MinerAvatar.tsxsrc/app/miners/components/Pagination.tsxsrc/app/miners/components/Pill.tsxsrc/app/miners/components/SearchBox.tsxsrc/app/miners/components/Segmented.tsxsrc/app/miners/components/helpers.tssrc/app/miners/components/index.tssrc/app/miners/components/tokens.tssrc/app/miners/components/types.tssrc/app/miners/page.tsxsrc/lib/format.tssrc/types/entities.ts
Comment on lines
+101
to
+103
| const total = Math.max(0, ossScore) + Math.max(0, discScore); | ||
| const ossPct = total > 0 ? (ossScore / total) * 100 : 0; | ||
| const discPct = total > 0 ? (discScore / total) * 100 : 0; |
There was a problem hiding this comment.
Clamp per-track scores before percentage calculation.
total is clamped to non-negative values, but Line 102-103 still use raw scores. Negative scores can yield negative segment widths.
Proposed fix
- const total = Math.max(0, ossScore) + Math.max(0, discScore);
- const ossPct = total > 0 ? (ossScore / total) * 100 : 0;
- const discPct = total > 0 ? (discScore / total) * 100 : 0;
+ const safeOss = Math.max(0, ossScore);
+ const safeDisc = Math.max(0, discScore);
+ const total = safeOss + safeDisc;
+ const ossPct = total > 0 ? (safeOss / total) * 100 : 0;
+ const discPct = total > 0 ? (safeDisc / total) * 100 : 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const total = Math.max(0, ossScore) + Math.max(0, discScore); | |
| const ossPct = total > 0 ? (ossScore / total) * 100 : 0; | |
| const discPct = total > 0 ? (discScore / total) * 100 : 0; | |
| const safeOss = Math.max(0, ossScore); | |
| const safeDisc = Math.max(0, discScore); | |
| const total = safeOss + safeDisc; | |
| const ossPct = total > 0 ? (safeOss / total) * 100 : 0; | |
| const discPct = total > 0 ? (safeDisc / total) * 100 : 0; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/miners/components/Bars.tsx` around lines 101 - 103, Clamp each track
score before using them to compute percentages: create non-negative versions
(e.g., clampedOss = Math.max(0, ossScore) and clampedDisc = Math.max(0,
discScore)), compute total from those clamped values, and then compute ossPct
and discPct from clampedOss/total and clampedDisc/total with the existing
total>0 guard; update references in Bars.tsx that use ossPct and discPct so
segment widths never receive negative values.
Comment on lines
+52
to
+56
| if (style === 'compact') { | ||
| // toFixed(2) for everything, then strip trailing zeros: | ||
| // 97.30 → 97.3, 0.9857 → 0.99, 0.40 → 0.4. | ||
| return `$${n.toFixed(2).replace(/\.?0+$/, '')}`; | ||
| } |
There was a problem hiding this comment.
compact formatting behavior does not match its documented contract.
Docs say <1 should use 4 decimals (like price), but implementation always uses toFixed(2), losing precision for sub-dollar values.
Suggested fix
if (style === 'compact') {
- // toFixed(2) for everything, then strip trailing zeros:
- // 97.30 → 97.3, 0.9857 → 0.99, 0.40 → 0.4.
- return `$${n.toFixed(2).replace(/\.?0+$/, '')}`;
+ const fixed = abs >= 1 ? n.toFixed(2) : n.toFixed(4);
+ return `$${fixed.replace(/\.?0+$/, '')}`;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/format.ts` around lines 52 - 56, The compact branch currently always
calls n.toFixed(2) which drops precision for sub-dollar values; update the
compact formatting logic (the block handling style === 'compact' and variable n)
to use n.toFixed(4) when n < 1 and n.toFixed(2) otherwise, then apply the
existing /\.?0+$/ replacement to strip trailing zeros so outputs match the
documented contract (use 4 decimals for values < 1, 2 decimals otherwise).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app/miners/LeaderTable.tsx (2)
788-793:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRead credibility from the canonical miner field here.
This percentage is recomputed locally instead of using
miner.credibility, so the leaderboard can disagree with the Network Pulse tile and any validator-aligned miner detail data. Please source credibility from the API field here and in thecredsort path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/LeaderTable.tsx` around lines 788 - 793, Replace the local recomputation of credibility with the canonical API value: stop using mergedTotal/solvedTotal/closedTotal/credDenom/cred to derive credPct and instead read miner.credibility (or fallback to 0) to compute credPct and to provide the value used in the "cred" sort path; update any references that previously used the local "cred" variable or its derived credPct to use miner.credibility (rounded/clamped to 0–100 as needed) so the leaderboard matches the Network Pulse and validator-aligned miner detail data.
857-860:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid nesting the repo chips and star button inside the row link.
The row-level
<Link>wraps clickableRepoChipbuttons and the tracking button, which creates interactive elements inside an anchor. That is invalid markup and tends to break keyboard and assistive-tech interaction. Make the row container non-interactive, or limit the link to the non-button regions.Also applies to: 918-929, 1049-1050
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/LeaderTable.tsx` around lines 857 - 860, Row-level Link currently wraps interactive children (RepoChip and star button) creating invalid nested interactive elements; change the row wrapper to a non-interactive element and only make non-button regions navigable. Replace the outer <Link href={`/miners/${miner.uid}`}> wrapper around the row with a plain container (e.g., <div> or <li>) and implement navigation for the row via a click handler (e.g., handleRowClick calling router.push(`/miners/${miner.uid}`) or a keyboard-accessible onKeyDown) so RepoChip and the star button remain independent interactive elements; alternatively, move the <Link> so it wraps only the miner name/text node (not RepoChip or the star button) to avoid nesting anchors/buttons—apply the same change for the other occurrences around the same row rendering logic (the blocks that produce RepoChip and the star button).src/app/miners/Insights.tsx (1)
234-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude zero-cred eligible miners in the network average.
This currently drops valid
0credibility samples, so the Network cred tile will overstate the average whenever an eligible miner has zero credibility. Count all non-null credibility values instead of only positives.Suggested fix
if (oss || disc) { dailyPool += num(m.usdPerDay); - const c = num(m.credibility); - if (c > 0) { credSum += c; credSamples += 1; } + if (m.credibility != null) { + const c = num(m.credibility); + credSum += c; + credSamples += 1; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/Insights.tsx` around lines 234 - 237, The current logic in Insights.tsx only increments credSum/credSamples when c > 0, which excludes valid zero-cred miners; change the check around c (derived from num(m.credibility)) to count all non-null/defined credibility values (e.g., if c !== null && c !== undefined && !Number.isNaN(c) or Number.isFinite(c)) so zero values are included in credSum and credSamples while still excluding missing/invalid credibilities.src/app/miners/page.tsx (1)
65-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same score comparator for current and snapshotted ranks.
ranksByUidis based ontotalScore + issueDiscoveryScore, but/api/minersstill snapshotspreviousRankfromtotalScoreonly. That makes the movement column and “Biggest mover” insight drift for miners with meaningful Discovery score. Please align both sides to one shared ranking formula.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/page.tsx` around lines 65 - 74, ranksByUid sorts by the composite metric (totalScore + issueDiscoveryScore) but the API snapshots previousRank using only totalScore, causing drift; update the snapshot logic that sets previousRank in the /api/miners code to compute the same composite score (totalScore + issueDiscoveryScore) and use that for ranking, so previousRank and the client-side ranksByUid comparator are identical (use the same composite formula everywhere).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/app/miners/Insights.tsx`:
- Around line 234-237: The current logic in Insights.tsx only increments
credSum/credSamples when c > 0, which excludes valid zero-cred miners; change
the check around c (derived from num(m.credibility)) to count all
non-null/defined credibility values (e.g., if c !== null && c !== undefined &&
!Number.isNaN(c) or Number.isFinite(c)) so zero values are included in credSum
and credSamples while still excluding missing/invalid credibilities.
In `@src/app/miners/LeaderTable.tsx`:
- Around line 788-793: Replace the local recomputation of credibility with the
canonical API value: stop using
mergedTotal/solvedTotal/closedTotal/credDenom/cred to derive credPct and instead
read miner.credibility (or fallback to 0) to compute credPct and to provide the
value used in the "cred" sort path; update any references that previously used
the local "cred" variable or its derived credPct to use miner.credibility
(rounded/clamped to 0–100 as needed) so the leaderboard matches the Network
Pulse and validator-aligned miner detail data.
- Around line 857-860: Row-level Link currently wraps interactive children
(RepoChip and star button) creating invalid nested interactive elements; change
the row wrapper to a non-interactive element and only make non-button regions
navigable. Replace the outer <Link href={`/miners/${miner.uid}`}> wrapper around
the row with a plain container (e.g., <div> or <li>) and implement navigation
for the row via a click handler (e.g., handleRowClick calling
router.push(`/miners/${miner.uid}`) or a keyboard-accessible onKeyDown) so
RepoChip and the star button remain independent interactive elements;
alternatively, move the <Link> so it wraps only the miner name/text node (not
RepoChip or the star button) to avoid nesting anchors/buttons—apply the same
change for the other occurrences around the same row rendering logic (the blocks
that produce RepoChip and the star button).
In `@src/app/miners/page.tsx`:
- Around line 65-74: ranksByUid sorts by the composite metric (totalScore +
issueDiscoveryScore) but the API snapshots previousRank using only totalScore,
causing drift; update the snapshot logic that sets previousRank in the
/api/miners code to compute the same composite score (totalScore +
issueDiscoveryScore) and use that for ranking, so previousRank and the
client-side ranksByUid comparator are identical (use the same composite formula
everywhere).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de58fd01-a072-407d-8e07-186f73073710
📒 Files selected for processing (4)
src/app/api/miners/route.tssrc/app/miners/Insights.tsxsrc/app/miners/LeaderTable.tsxsrc/app/miners/page.tsx
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 (4)
src/app/miners/[uid]/components/RepoBreakdown.tsx (1)
499-501:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep Discovery credibility aligned with displayed solved count.
Discovery now displays
solved + completed, but credibility still excludescompleted. That makes Cred inconsistent with the visible solved metric and sorting semantics.🔧 Suggested fix
const credPct = mode === 'oss' ? ((row.merged + row.closedPr) > 0 ? Math.round((row.merged / (row.merged + row.closedPr)) * 100) : null) - : ((row.solvedIssue + row.closedIssue) > 0 ? Math.round((row.solvedIssue / (row.solvedIssue + row.closedIssue)) * 100) : null); + : (((row.solvedIssue + row.completedIssue + row.closedIssue) > 0) + ? Math.round(((row.solvedIssue + row.completedIssue) / (row.solvedIssue + row.completedIssue + row.closedIssue)) * 100) + : null);Also applies to: 521-526
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/components/RepoBreakdown.tsx around lines 499 - 501, The credPct calculation currently uses merged vs merged+closedPr for mode 'oss' and solvedIssue vs solvedIssue+closedIssue otherwise, but Discovery shows "solved + completed", so update the credPct logic (the const credPct calculation) to include the corresponding completed field(s) in both numerator and denominator so the credibility matches the displayed solved metric (e.g., add row.completed or the repo's completed PR/issue field to both numerator and denominator for the 'oss' and non-'oss' branches); ensure you update the same pattern in the analogous block referenced at lines ~521-526.src/app/api/gt/miners/[uid]/route.ts (1)
89-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't turn a cold per-miner fetch failure into empty stats.
If
/miners/{githubId}fails beforeperMinerCacheis warm, this returns{ repoEvals: [] }and drops lifetime fields, so the detail page silently renders misleading "0 eligible repos / not earning" state instead of stale data or an error.Possible fix
- } catch { - const hit = perMinerCache.get(githubId); - return hit?.data ?? { repoEvals: [] }; + } catch { + const hit = perMinerCache.get(githubId); + if (hit) return hit.data; + throw new Error(`upstream ${MINERS_URL}/${githubId} unavailable`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/gt/miners/`[uid]/route.ts around lines 89 - 91, The catch block in route.ts currently swallows fetch errors and returns a default { repoEvals: [] }, which masks cold failures; update the catch handling in the async fetch around perMinerCache/githubId so that you first attempt to read perMinerCache.get(githubId) and return that cached hit if present, otherwise rethrow (or propagate) the original error instead of returning an empty stats object; also include the caught error in a processLogger.error (or console.error) call for visibility.src/app/miners/LeaderTable.tsx (2)
657-663:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis reintroduces a local credibility approximation.
credPctis derived from merged/closed counts instead of the validator fields already present onminer. That conflicts with the linked objective to show validator-aligned credibility, so this leaderboard value can drift from the upstream/detail-page numbers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/LeaderTable.tsx` around lines 657 - 663, The code in LeaderTable.tsx recomputes credibility (variables mergedTotal, solvedTotal, closedTotal, credDenom, cred, credPct) from PR/issue counts instead of using the validator-aligned credibility already present on the miner object; replace the local computation and set credPct from the miner's validator field (e.g., use miner.validatorCredibility or miner.validatorCredPct — if the stored value is a fraction multiply by 100 and clamp 0–100) so the leaderboard uses the same validator-derived credibility as the detail page.
725-729:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix invalid nested interactive elements in miner rows.
The row-level
<Link href={/miners/${miner.uid}}>wrapsRepoChipandTrackButton, and both render as<button>elements (RepoChip:Box as={onClick ? 'button' : 'span'};TrackButton:Box as="button"). This creates interactive descendants inside an<a href>, which is invalid HTML and breaks reliable keyboard/screen-reader focus/activation even withpreventDefault()/stopPropagation().Also applies to lines 790-797 (RepoChip) and 916-917 (TrackButton).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/LeaderTable.tsx` around lines 725 - 729, The row-level Link in LeaderTable.tsx wraps interactive descendants (RepoChip and TrackButton) which render as buttons, causing invalid nested interactive elements; fix by removing the anchor wrapper for the full row and instead make the row a non-anchor container that performs navigation via router.push(`/miners/${miner.uid}`) onClick with keyboard handling (onKeyDown for Enter/Space) and appropriate role/tabIndex to remain accessible, leaving RepoChip and TrackButton as buttons, or alternatively pass an explicit prop to RepoChip and TrackButton to render as non-interactive elements (e.g., as="span") when inside the row Link; update the occurrences where Link wraps these components (the Link around miner rows and the instances noted around RepoChip and TrackButton) so no button sits inside an anchor.
♻️ Duplicate comments (3)
src/app/miners/[uid]/page.tsx (1)
75-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not retain previous UID data in this detail query.
Keeping
placeholderDatahere can transiently show the wrong miner profile during UID navigation.🔧 Suggested fix
const { data, isError } = useQuery<DetailResp>({ queryKey: ['miner-detail', uid], @@ - placeholderData: (prev) => prev, + placeholderData: undefined, @@ - const miner = data?.miner; + const miner = data?.miner && String(data.miner.uid) === uid ? data.miner : undefined;#!/bin/bash set -euo pipefail # Verify miner-detail query config and placeholder behavior in this page. rg -n -C4 "useQuery<DetailResp>|queryKey: \\['miner-detail'|placeholderData" src/app/miners/[uid]/page.tsx🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/page.tsx around lines 75 - 84, The detail query for miners (useQuery<DetailResp> with queryKey ['miner-detail', uid]) is retaining the previous UID's data via placeholderData, causing transient wrong profiles during navigation; remove the placeholderData option (or set it to undefined/null) from this useQuery call so the query does not display previous miner data while fetching the new UID, and ensure refetchInterval/staleTime remain unchanged to preserve caching behavior.src/app/miners/[uid]/components/shared.tsx (1)
106-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
filtertouseMemodependencies.This hook’s memoized filter result can go stale when callers pass a new
filterclosure. Keep the dependency list complete instead of relying on suppression.🔧 Suggested fix
const filtered = useMemo(() => { const q = search.toLowerCase().trim(); return q ? items.filter((i) => filter(i, q)) : items; - // filter is a stable render-time closure; items + search are the real deps - }, [items, search]); // eslint-disable-line react-hooks/exhaustive-deps + }, [items, search, filter]);#!/bin/bash set -euo pipefail # Verify all useSearchPage call sites and inspect filter closure usage. rg -n -C3 '\buseSearchPage\b' src/app/miners🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/components/shared.tsx around lines 106 - 110, The useMemo that computes filtered currently omits the filter callback from its dependency array, which can lead to stale results when a new filter closure is passed; update the dependency list on the useMemo that defines filtered to include filter (i.e., change the deps from [items, search] to [items, search, filter]) so React re-computes when the filter function identity changes, and remove the eslint-disable comment so the hook rule is satisfied for filtered in the component where useMemo, filter, items, and search are used.src/app/miners/[uid]/components/RepoBreakdown.tsx (1)
535-541:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid nesting links inside a button row.
The row is still a button containing anchor links, which is invalid interactive nesting and can break keyboard/focus behavior.
♿ Suggested fix
- <Box - as="button" + <Box + as="div" + role="button" + tabIndex={0} onClick={onSelect} + onKeyDown={(e: React.KeyboardEvent) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onSelect(); + } + }} sx={{ width: '100%', display: 'block', p: 0,Also applies to: 578-603, 676-702
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/components/RepoBreakdown.tsx around lines 535 - 541, The row currently renders as a native button (as="button") while containing anchor links, causing invalid nested interactive elements; change the outer element to a non-interactive container (e.g., as="div") and keep the onSelect click handling, but make it keyboard-accessible by adding role="button" and tabIndex={0} and handling Enter/Space in the keyDown handler, or alternatively move navigation into the anchors and remove the outer onClick entirely; update the instances in RepoBreakdown (the element using as="button" with onClick/onSelect) and the other similar row blocks noted (the ranges around lines 578-603 and 676-702) so no anchor is nested inside a native button.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/miners/`[uid]/components/IssueList.tsx:
- Around line 533-607: The modal lacks dialog semantics and focus management:
update the IssueList component to give the overlay/dialog accessible roles and
trap/restore focus by (1) adding role="dialog" and aria-modal="true" to the
inner dialog Box and aria-labelledby pointing to the issue title element (give
the title Text an id), (2) create refs (e.g., dialogRef and closeBtnRef for the
outer inner Box and the close button) and on mount (extend the existing
useEffect that handles Escape) save document.activeElement, move focus into the
dialog (preferably to closeBtnRef or the first focusable), and add a keydown
handler that handles Tab/Shift+Tab to keep focus inside dialog; (3) on unmount
restore focus to the previously focused element and remove the keydown listener
and body overflow change. Reference symbols: IssueList component, the existing
useEffect, the outer overlay Box, the inner dialog Box, the close button element
(onClick={onClose}) and the title Text ({iss.title}).
In `@src/app/miners/`[uid]/components/PositionSummary.tsx:
- Around line 247-253: The "Eligible repos" BigNumber currently sums
ossEligibleCount + discEligibleCount which double-counts repos eligible for
both; update the component (around Tile/TileHeader/BigNumber in PositionSummary)
to show a deduplicated unique repo count instead of the simple sum by deriving
uniqueEligibleCount from the OSS and DISC sets (e.g., combine the repo
identifiers from the OSS and DISC eligible lists into a Set) and pass
uniqueEligibleCount to BigNumber, or alternatively rename the card to indicate
it is a per-track total if you intend to keep the summed value.
---
Outside diff comments:
In `@src/app/api/gt/miners/`[uid]/route.ts:
- Around line 89-91: The catch block in route.ts currently swallows fetch errors
and returns a default { repoEvals: [] }, which masks cold failures; update the
catch handling in the async fetch around perMinerCache/githubId so that you
first attempt to read perMinerCache.get(githubId) and return that cached hit if
present, otherwise rethrow (or propagate) the original error instead of
returning an empty stats object; also include the caught error in a
processLogger.error (or console.error) call for visibility.
In `@src/app/miners/`[uid]/components/RepoBreakdown.tsx:
- Around line 499-501: The credPct calculation currently uses merged vs
merged+closedPr for mode 'oss' and solvedIssue vs solvedIssue+closedIssue
otherwise, but Discovery shows "solved + completed", so update the credPct logic
(the const credPct calculation) to include the corresponding completed field(s)
in both numerator and denominator so the credibility matches the displayed
solved metric (e.g., add row.completed or the repo's completed PR/issue field to
both numerator and denominator for the 'oss' and non-'oss' branches); ensure you
update the same pattern in the analogous block referenced at lines ~521-526.
In `@src/app/miners/LeaderTable.tsx`:
- Around line 657-663: The code in LeaderTable.tsx recomputes credibility
(variables mergedTotal, solvedTotal, closedTotal, credDenom, cred, credPct) from
PR/issue counts instead of using the validator-aligned credibility already
present on the miner object; replace the local computation and set credPct from
the miner's validator field (e.g., use miner.validatorCredibility or
miner.validatorCredPct — if the stored value is a fraction multiply by 100 and
clamp 0–100) so the leaderboard uses the same validator-derived credibility as
the detail page.
- Around line 725-729: The row-level Link in LeaderTable.tsx wraps interactive
descendants (RepoChip and TrackButton) which render as buttons, causing invalid
nested interactive elements; fix by removing the anchor wrapper for the full row
and instead make the row a non-anchor container that performs navigation via
router.push(`/miners/${miner.uid}`) onClick with keyboard handling (onKeyDown
for Enter/Space) and appropriate role/tabIndex to remain accessible, leaving
RepoChip and TrackButton as buttons, or alternatively pass an explicit prop to
RepoChip and TrackButton to render as non-interactive elements (e.g., as="span")
when inside the row Link; update the occurrences where Link wraps these
components (the Link around miner rows and the instances noted around RepoChip
and TrackButton) so no button sits inside an anchor.
---
Duplicate comments:
In `@src/app/miners/`[uid]/components/RepoBreakdown.tsx:
- Around line 535-541: The row currently renders as a native button
(as="button") while containing anchor links, causing invalid nested interactive
elements; change the outer element to a non-interactive container (e.g.,
as="div") and keep the onSelect click handling, but make it keyboard-accessible
by adding role="button" and tabIndex={0} and handling Enter/Space in the keyDown
handler, or alternatively move navigation into the anchors and remove the outer
onClick entirely; update the instances in RepoBreakdown (the element using
as="button" with onClick/onSelect) and the other similar row blocks noted (the
ranges around lines 578-603 and 676-702) so no anchor is nested inside a native
button.
In `@src/app/miners/`[uid]/components/shared.tsx:
- Around line 106-110: The useMemo that computes filtered currently omits the
filter callback from its dependency array, which can lead to stale results when
a new filter closure is passed; update the dependency list on the useMemo that
defines filtered to include filter (i.e., change the deps from [items, search]
to [items, search, filter]) so React re-computes when the filter function
identity changes, and remove the eslint-disable comment so the hook rule is
satisfied for filtered in the component where useMemo, filter, items, and search
are used.
In `@src/app/miners/`[uid]/page.tsx:
- Around line 75-84: The detail query for miners (useQuery<DetailResp> with
queryKey ['miner-detail', uid]) is retaining the previous UID's data via
placeholderData, causing transient wrong profiles during navigation; remove the
placeholderData option (or set it to undefined/null) from this useQuery call so
the query does not display previous miner data while fetching the new UID, and
ensure refetchInterval/staleTime remain unchanged to preserve caching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5137fe64-3412-4d87-985b-e152d930ec41
📒 Files selected for processing (16)
src/app/api/gt/miners/[uid]/route.tssrc/app/globals.csssrc/app/miners/LeaderTable.tsxsrc/app/miners/[uid]/components/IssueList.tsxsrc/app/miners/[uid]/components/PositionSummary.tsxsrc/app/miners/[uid]/components/PrList.tsxsrc/app/miners/[uid]/components/ProfileHero.tsxsrc/app/miners/[uid]/components/RepoBreakdown.tsxsrc/app/miners/[uid]/components/index.tssrc/app/miners/[uid]/components/shared.tsxsrc/app/miners/[uid]/components/types.tssrc/app/miners/[uid]/page.tsxsrc/app/miners/components/SortControl.tsxsrc/app/miners/components/StatusBadge.tsxsrc/app/miners/components/index.tssrc/types/entities.ts
💤 Files with no reviewable changes (1)
- src/app/miners/[uid]/components/index.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/globals.css
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/miners/[uid]/components/IssueList.tsx (1)
535-546: ⚖️ Poor tradeoffFocus trap still missing from the modal.
The accessibility improvements (role, aria-modal, focus restore) address most of the past review feedback, but keyboard users can still Tab out of the modal into the page behind. For full modal semantics, a focus trap is needed so Tab/Shift+Tab cycles only within the dialog.
Consider using a library like
@primer/react'sDialogcomponent orfocus-trap-reactrather than rolling your own. If you want to keep this implementation, add akeydownhandler that intercepts Tab when focus is on the first/last focusable element and wraps it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/components/IssueList.tsx around lines 535 - 546, The modal currently sets initial focus (closeBtnRef) and restores it on close but lacks a focus trap so Tab/Shift+Tab can move focus outside; update the useEffect in IssueList.tsx to either wrap the modal with a stable focus-trap component (e.g., use focus-trap-react or `@primer/react` Dialog) or add a keydown handler alongside handler that intercepts Tab/Shift+Tab, computes the first/last focusable element inside the dialog element (use the modal container ref), and when Tab would move out, preventDefault and move focus to the opposite end to cycle focus; keep existing Escape handler (handler/onClose) and restore previouslyFocused as currently implemented.src/app/miners/[uid]/page.tsx (1)
1-3: 💤 Low value
export const dynamichas no effect in a Client Component.Route segment config exports like
dynamicare only honored in Server Components,layout.tsx,page.tsxserver files, or Route Handlers. Since this file has'use client'at the top, the export is ignored by Next.js. It's harmless but misleading.Remove the export or, if you need server-side dynamic rendering, refactor data fetching into a Server Component wrapper that passes props to this client component.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/page.tsx around lines 1 - 3, The file declares a Client Component with `'use client'` but also exports `export const dynamic = 'force-dynamic'`, which Next.js ignores for client components; remove the `export const dynamic` line from src/app/miners/[uid]/page.tsx to avoid confusion, or if you require server-side dynamic behavior, refactor by creating a Server Component wrapper (e.g., a server-side `page.tsx` that sets `export const dynamic = 'force-dynamic'`) that fetches data and passes props into this client component instead of keeping `export const dynamic` inside the client file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/miners/`[uid]/components/IssueList.tsx:
- Around line 535-546: The modal currently sets initial focus (closeBtnRef) and
restores it on close but lacks a focus trap so Tab/Shift+Tab can move focus
outside; update the useEffect in IssueList.tsx to either wrap the modal with a
stable focus-trap component (e.g., use focus-trap-react or `@primer/react` Dialog)
or add a keydown handler alongside handler that intercepts Tab/Shift+Tab,
computes the first/last focusable element inside the dialog element (use the
modal container ref), and when Tab would move out, preventDefault and move focus
to the opposite end to cycle focus; keep existing Escape handler
(handler/onClose) and restore previouslyFocused as currently implemented.
In `@src/app/miners/`[uid]/page.tsx:
- Around line 1-3: The file declares a Client Component with `'use client'` but
also exports `export const dynamic = 'force-dynamic'`, which Next.js ignores for
client components; remove the `export const dynamic` line from
src/app/miners/[uid]/page.tsx to avoid confusion, or if you require server-side
dynamic behavior, refactor by creating a Server Component wrapper (e.g., a
server-side `page.tsx` that sets `export const dynamic = 'force-dynamic'`) that
fetches data and passes props into this client component instead of keeping
`export const dynamic` inside the client file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57af4899-bf5c-48f9-bf2d-5d5b285347dd
📒 Files selected for processing (11)
src/app/api/gt/miners/[uid]/route.tssrc/app/api/miners/route.tssrc/app/miners/[uid]/components/IssueList.tsxsrc/app/miners/[uid]/components/PositionSummary.tsxsrc/app/miners/[uid]/components/PrList.tsxsrc/app/miners/[uid]/components/RepoBreakdown.tsxsrc/app/miners/[uid]/components/shared.tsxsrc/app/miners/[uid]/page.tsxsrc/app/miners/components/Pagination.tsxsrc/app/miners/components/SearchBox.tsxsrc/app/miners/components/Segmented.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/miners/components/types.ts (1)
2-51: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the canonical
MinerDTO instead of keeping a second copy here.This local interface already disagrees with
src/types/entities.tsongithubUsernamenullability, and the samedailyLookbackchange had to be duplicated in both places. Re-exportingMiner/MinerTopRepofrom the canonical DTO module will prevent the two contracts from drifting again.♻️ Minimal direction
-export interface MinerTopRepo { - name: string; - count: number; -} - -export interface Miner { - id: string; - uid: number; - hotkey: string; - githubUsername: string | null; - githubId?: string; - isEligible: boolean; - isIssueEligible?: boolean; - failedReason?: string | null; - credibility: string; - issueCredibility?: string; - eligibleRepoCount?: number; - issueEligibleRepoCount?: number; - issueDiscoveryScore?: string; - issueTokenScore?: string; - totalScore: string; - baseTotalScore?: string; - totalSolvedIssues?: number; - totalValidSolvedIssues?: number; - totalOpenIssues?: number; - totalClosedIssues?: number; - totalOpenPrs?: number; - totalClosedPrs?: number; - totalMergedPrs?: number; - totalValidMergedPrs?: number; - lastOssActivityAt?: string | null; - lastDiscoveryActivityAt?: string | null; - totalPrs?: number; - totalAdditions?: number; - totalDeletions?: number; - uniqueReposCount?: number; - alphaPerDay?: number; - taoPerDay?: number; - usdPerDay?: number; - createdAt?: string; - dailyLookback?: number[]; - topRepos?: MinerTopRepo[]; - previousRank?: number | null; -} +export type { Miner, MinerTopRepo } from '`@/types/entities`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/components/types.ts` around lines 2 - 51, This file defines local Miner and MinerTopRepo interfaces that diverge from the canonical DTO; remove these local types and instead import and re-export the canonical Miner and MinerTopRepo DTOs from the shared DTO module (use the module that owns the canonical types), update this module to export those symbols (export { Miner, MinerTopRepo } from '...'), and update any local references/importers to consume the re-export so the nullability (githubUsername) and dailyLookback shape remain consistent across the codebase.src/app/miners/[uid]/page.tsx (2)
145-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
selectedRepowhen the active mode/window no longer contains it.
selectedRepokeeps filtering the lower lists afterrepoBreakdownhas been rebuilt for a different mode or period. That can leave the page showing an empty PR/issue list even though no visible repo row is selected anymore.🩹 Minimal fix
-import React, { use, useMemo, useState } from 'react'; +import React, { use, useEffect, useMemo, useState } from 'react'; @@ const repoBreakdown = useMemo(() => { // ... }, [prsInPeriod, discoveredInP, solvedInPeriod, mode, data?.repoEvals]); + + useEffect(() => { + if ( + selectedRepo && + !repoBreakdown.some((r) => r.repo.toLowerCase() === selectedRepo.toLowerCase()) + ) { + setSelectedRepo(null); + } + }, [selectedRepo, repoBreakdown]);Also applies to: 156-211, 357-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/page.tsx around lines 145 - 152, The selectedRepo filter is not cleared when the underlying repoBreakdown (or its inputs like mode/period) changes, causing empty lists; add logic to reset selectedRepo whenever repoBreakdown (or the inputs that rebuild it) no longer contains that repo. Concretely, add a useEffect that watches repoBreakdown (or the values that drive it) and if selectedRepo is non-empty and not present in repoBreakdown.map(r => r.repoName) then call setSelectedRepo("") (or null) to clear it; apply the same reset behavior wherever selectedRepo is used to filter lists (e.g., the prsFiltered and discoveredFiltered memo usages and other places referenced in this file).
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace React
use()route-param unwrapping in client pages withuseParams()
src/app/miners/[uid]/page.tsxusesuse(ctx.params)in a'use client'component, andsrc/app/miners/[uid]/prs/[...slug]/page.tsxusesuse(params)similarly; with the current Next/React 18 stack this is a compile/runtime risk—switch touseParams()(or make the page/server boundary appropriate) and remove the Promise-param unwrapping.selectedRepois never cleared whenmode/period(and thereforeprsInPeriod/discoveredInP/repoBreakdown) changes; if the selected repo disappears from the breakdown, the lower lists remain filtered and can render empty even while other repo activity still exists—add a state sync (e.g., clear/resetselectedRepowhen it’s no longer present).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/miners/`[uid]/page.tsx at line 5, The page is unwrapping route params via React's use(...) in a client component—replace that pattern by importing and using Next's useParams() in the client component (remove any Promise/unwrapping of ctx.params or params) so route params are read at runtime correctly (look for the use(ctx.params)/use(params) usage in src/app/miners/[uid]/page.tsx and similar in prs/[...slug]/page.tsx and swap to useParams()); additionally, ensure selectedRepo state is synced with changes to mode/period and the computed lists (prsInPeriod, discoveredInP, repoBreakdown) by adding an effect that clears or resets selectedRepo when the current selected repo is no longer present in repoBreakdown (or when mode/period changes) so the lower lists don't remain filtered by a vanished repo (target the selectedRepo state and repoBreakdown/mode/period dependencies).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/app/miners/`[uid]/page.tsx:
- Around line 145-152: The selectedRepo filter is not cleared when the
underlying repoBreakdown (or its inputs like mode/period) changes, causing empty
lists; add logic to reset selectedRepo whenever repoBreakdown (or the inputs
that rebuild it) no longer contains that repo. Concretely, add a useEffect that
watches repoBreakdown (or the values that drive it) and if selectedRepo is
non-empty and not present in repoBreakdown.map(r => r.repoName) then call
setSelectedRepo("") (or null) to clear it; apply the same reset behavior
wherever selectedRepo is used to filter lists (e.g., the prsFiltered and
discoveredFiltered memo usages and other places referenced in this file).
- Line 5: The page is unwrapping route params via React's use(...) in a client
component—replace that pattern by importing and using Next's useParams() in the
client component (remove any Promise/unwrapping of ctx.params or params) so
route params are read at runtime correctly (look for the
use(ctx.params)/use(params) usage in src/app/miners/[uid]/page.tsx and similar
in prs/[...slug]/page.tsx and swap to useParams()); additionally, ensure
selectedRepo state is synced with changes to mode/period and the computed lists
(prsInPeriod, discoveredInP, repoBreakdown) by adding an effect that clears or
resets selectedRepo when the current selected repo is no longer present in
repoBreakdown (or when mode/period changes) so the lower lists don't remain
filtered by a vanished repo (target the selectedRepo state and
repoBreakdown/mode/period dependencies).
In `@src/app/miners/components/types.ts`:
- Around line 2-51: This file defines local Miner and MinerTopRepo interfaces
that diverge from the canonical DTO; remove these local types and instead import
and re-export the canonical Miner and MinerTopRepo DTOs from the shared DTO
module (use the module that owns the canonical types), update this module to
export those symbols (export { Miner, MinerTopRepo } from '...'), and update any
local references/importers to consume the re-export so the nullability
(githubUsername) and dailyLookback shape remain consistent across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a063891e-0ee0-4d0b-9acc-01a05f72cab2
📒 Files selected for processing (10)
src/app/api/miners/route.tssrc/app/miners/Insights.tsxsrc/app/miners/LeaderTable.tsxsrc/app/miners/[uid]/components/ActivitySummary.tsxsrc/app/miners/[uid]/components/types.tssrc/app/miners/[uid]/page.tsxsrc/app/miners/[uid]/prs/[...slug]/page.tsxsrc/app/miners/components/types.tssrc/lib/gittensor-policy.tssrc/types/entities.ts
Dexterity104
force-pushed
the
feat/improve-miners-page
branch
from
9a55f97 to
1d35414
Compare
Dexterity104
force-pushed
the
feat/improve-miners-page
branch
from
1d35414 to
27ccbd0
Compare
Owner
|
Same to #113, I can't accept this kind of UI implementation, we really need a different style better than this. |
Contributor
Author
|
Original UI: Updated UI: |
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
2 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
Complete overhaul of the miners leaderboard to surface the enriched data Gittensor now exposes, including per-repository credibility, dual-track scoring, and 35-day activity history. This replaces the previous flat list with a data-rich, mobile-responsive dashboard.
Network Pulse strip at the top of the page shows live network-wide stats on every poll cycle:
Top Stories panel highlights six spotlight cards:
Leaderboard row enrichments:
Toolbar includes centered search, eligibility filter (All / Eligible / Ineligible), tracked-only toggle, repository filter, and a row size selector. Layout stacks cleanly on mobile with no overlapping elements.
All layout changes are fully responsive. The insights cards and table rows each have distinct mobile layouts that consolidate information without losing detail.
Related Issues
Closes #103
Screenshots
Type of Change
Testing
pnpm buildpassesChecklist
Summary by CodeRabbit
New Features
Improvements
Style