feat(projects): sort by volume + segmented scale bar (GLOOK-23)#56
Conversation
- Sort projects by estimated_prs + jira_count (more meaningful than raw commits) - Add visual legend for PR (cyan), Jira (purple), Commit (ghost) segments - Implement segmented volume bar showing composition of each project's activity - Bar width normalized to max total volume across all projects - Segments use flex proportions to show distribution Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
msogin
left a comment
There was a problem hiding this comment.
Code review from two passes: Smartling standard review (frontend focus) + review-loop (Sr. Frontend Engineer + UX/Product Engineer, with minimalist gate). 8 inline findings below.
|
|
||
| // Sort by meaningful output (PRs + Jiras). Commits are squashed into PRs so | ||
| // using raw commit count would inflate commit-heavy, low-PR projects. | ||
| const sorted = [...projects].sort( |
There was a problem hiding this comment.
🟡 warning — sorted, maxVolume, and hasJira are recomputed on every render of ProjectsBody. Since the component is not memoized, these run on every parent re-render (collapsible toggle, hover, unrelated state changes). Wrap in useMemo:
const sorted = useMemo(
() => [...projects].sort((a, b) => (b.estimated_prs + b.jira_count) - (a.estimated_prs + a.jira_count)),
[projects],
);
const maxVolume = useMemo(
() => sorted.reduce((max, p) => Math.max(max, p.estimated_prs + p.jira_count + p.estimated_commits), 1),
[sorted],
);
const hasJira = useMemo(() => sorted.some(p => p.jira_count > 0), [sorted]);| ); | ||
|
|
||
| // Max total volume across all projects — used to normalise bar widths. | ||
| const maxVolume = Math.max( |
There was a problem hiding this comment.
🟡 warning — maxVolume includes estimated_commits in the normalization, but the sort key on line 75 explicitly excludes commits (the comment explains why: they inflate commit-heavy, low-PR projects). This creates a contradiction: a commit-heavy project sorts low but renders a proportionally wide bar — the visual rank and sort rank diverge in exactly the case the sort is designed to de-emphasize.
Either include commits in the sort key, or exclude them from the bar width calculation to keep the two signals consistent.
|
|
||
| // Max total volume across all projects — used to normalise bar widths. | ||
| const maxVolume = Math.max( | ||
| ...sorted.map(p => p.estimated_prs + p.jira_count + p.estimated_commits), |
There was a problem hiding this comment.
🟣 question — Math.max(...sorted.map(...)) spreads the full array into function arguments, which hits the JS call stack limit for large arrays (~65k items). Is there an expected upper bound on projects.length? If not, use reduce instead (already shown in the useMemo suggestion on line 75).
| {/* Legend */} | ||
| <div className="flex gap-3 text-[10px] text-gray-600 pl-6"> | ||
| <span className="flex items-center gap-1"> | ||
| <span className="inline-block w-2 h-2 rounded-[2px]" style={{ background: '#06B6D4' }} />PRs |
There was a problem hiding this comment.
🔵 suggestion — Decorative color swatches convey no textual content. Add aria-hidden="true" so screen readers skip them:
<span aria-hidden="true" className="inline-block w-2 h-2 rounded-[2px]" style={{ background: '#06B6D4' }} />(Same applies to the Jiras and Commits swatches on lines 97 and 101.)
| const barPct = (totalVol / maxVolume) * 100; | ||
| return ( | ||
| <div key={i} className="bg-white/[0.02] rounded-lg p-3"> | ||
| <div key={p.name} className="bg-white/[0.02] rounded-lg p-3"> |
There was a problem hiding this comment.
🟡 warning — key={p.name} assumes project names are unique within the list. If two projects share a name (e.g. a personal fork and an org repo both named "api"), React silently reuses the wrong DOM node during reconciliation — harder to debug than the index key it replaced. Either verify uniqueness upstream, or use key={p.name + '-' + i} as a safe composite.
| </div> | ||
|
|
||
| {/* Volume bar: width = total / max, segments = PRs (cyan) + Jiras (purple) + Commits (ghost) */} | ||
| <div className="pl-6 mb-1.5"> |
There was a problem hiding this comment.
🟡 warning — If a project has zero PRs, Jiras, and commits, totalVol is 0 and barPct is 0. All three flex children render with flex: 0 inside a 0%-wide container — the bar silently disappears with no fallback. Guard the render:
{totalVol > 0 && (
<div className="pl-6 mb-1.5">
{/* ... */}
</div>
)}|
|
||
| {/* Volume bar: width = total / max, segments = PRs (cyan) + Jiras (purple) + Commits (ghost) */} | ||
| <div className="pl-6 mb-1.5"> | ||
| <div className="h-[5px] rounded-sm overflow-hidden" style={{ background: 'rgba(255,255,255,0.05)' }}> |
There was a problem hiding this comment.
🔵 suggestion — The volume bar conveys data (segment proportions = PR/Jira/commit breakdown) but has no accessible label. Screen reader users get nothing. Add role="img" and aria-label:
<div
className="h-[5px] rounded-sm overflow-hidden"
style={{ background: 'rgba(255,255,255,0.05)' }}
role="img"
aria-label={`Volume: ${p.estimated_prs} PRs, ${p.jira_count} Jiras, ${p.estimated_commits} commits`}
>| <div className="pl-6 mb-1.5"> | ||
| <div className="h-[5px] rounded-sm overflow-hidden" style={{ background: 'rgba(255,255,255,0.05)' }}> | ||
| <div className="h-full flex" style={{ width: `${barPct}%` }}> | ||
| <div style={{ flex: p.estimated_prs, background: '#06B6D4' }} /> |
There was a problem hiding this comment.
🔵 suggestion — #06B6D4, #A855F7, and the rgba values appear 4× across this diff (two legend swatches + two bar segment divs). If the palette changes, all four must be updated in sync. Extract to a shared constant:
const SEGMENT_COLORS = {
prs: '#06B6D4',
jiras: '#A855F7',
commits: 'rgba(255,255,255,0.10)',
} as const;
msogin
left a comment
There was a problem hiding this comment.
Re-review of new commit 7dbbc15f (feat(glook-23): add Other row showing unattributed activity). Prior findings from 2026-06-17 were addressed. Reviewed new code only.
Two review passes: review-loop (Sr. Frontend Eng + Fullstack API Eng, 1 loop) + Smartling standard review (fullstack). 3 findings below.
| const sumPrs = sorted.reduce((s, p) => s + p.estimated_prs, 0); | ||
| const sumJiras = sorted.reduce((s, p) => s + p.jira_count, 0); | ||
| const o = { | ||
| commits: Math.max(0, actualTotals.commits - sumCommits), |
There was a problem hiding this comment.
🟡 warning — other.commits and other.prs subtract LLM-estimated per-project values from real developer_stats sums. The difference is estimation noise, not genuinely unattributed activity.
actualTotals.commits/prs = SUM(total_commits/total_prs) from developer_stats — exact values from the DB.
sumCommits/sumPrs = sum of LLM proportionally-allocated estimates across named projects — approximate.
When the LLM over-estimates any project, Math.max(0, ...) clamps the result to 0 and silently hides genuinely unattributed commits. When it under-estimates, the "Other" row inflates beyond real unattributed work. The ~ prefix signals approximation but doesn't cover a structurally misleading number — the bar width and counts reflect LLM allocation error, not actual unattributed activity.
Consider either: (a) accepting this as an acknowledged approximation and adding a tooltip/note clarifying the ~ means "remainder after LLM attribution" not "exact unattributed", or (b) excluding commits/PRs from the Other row and showing only Jiras (if the jira subtraction is exact — see next comment).
| if (!actualTotals) return null; | ||
| const sumCommits = sorted.reduce((s, p) => s + p.estimated_commits, 0); | ||
| const sumPrs = sorted.reduce((s, p) => s + p.estimated_prs, 0); | ||
| const sumJiras = sorted.reduce((s, p) => s + p.jira_count, 0); |
There was a problem hiding this comment.
🟡 warning — sumJiras subtracts LLM-assigned jira_count per project from COUNT(*) FROM jira_issues. These are only consistent if the LLM assigns each Jira issue to exactly one project with no over/under-counting.
The current LLM prompt does proportional allocation ("estimate commits/PRs attributed to this project (use developer stats and proportional allocation based on issue count)"), which doesn't guarantee a strict partition of Jira issues. If the LLM's per-project jira_count values don't sum to exactly COUNT(*), the "Other" jiras number will silently over/under-count.
If the LLM does assign each Jira issue to exactly one project in practice, this is fine — worth confirming the prompt enforces it. If not, the jira column has the same estimation-noise problem as commits/PRs.
| const totals = { | ||
| commits: Number(totalsRows[0]?.commits ?? 0), | ||
| prs: Number(totalsRows[0]?.prs ?? 0), | ||
| jiras: Number(jiraCount[0].cnt), |
There was a problem hiding this comment.
🔵 suggestion — Missing ?. null guard. The totalsRows[0] access above uses ?., but jiraCount[0] does not.
jiras: Number(jiraCount[0]?.cnt ?? 0),The check at line 25 guarantees jiraCount[0].cnt is defined when execution reaches here, so this is safe — but the invariant is implicit. The ?. makes it explicit at zero cost.
Summary
Bar design
Changes
Single file:
src/components/ProjectsCard.tsx—ProjectsBodyfunction. Both the home-page standalone card and the team-page collapsible card use the same component and get the change automatically.Test plan
🤖 Generated with Claude Code