feat(team): sortable Individuals table with URL-persisted sort (GLOOK-12)

[msogin]

Summary

• All columns in the Individuals developer table on the team report page are now sortable by clicking the column header
• Sort state is URL-persisted (?devsort=&devdir=) — survives reload and produces shareable links
• When sorted by any column other than Impact, the absolute impact rank is shown in parentheses next to the sort rank (e.g. 3 (7) — 3rd by commits, 7th by impact)
• Default sort remains impact_score DESC to match the existing server order

Implementation

Extracted the inlined developer table IIFE from page.tsx into a new dev-table.tsx component — mirroring the existing team-table.tsx pattern. All helper subcomponents (CommitCountWithTooltip, JiraCountWithTooltip, badge components) moved to the new file. page.tsx loses ~390 lines and gains a single <DevTable> call.

Test plan

• All 741 unit tests pass
• Click each column header — table re-sorts, caret appears, URL updates
• Click same header again — sort direction toggles
• Sort by non-Impact column — absolute impact rank visible in parentheses
• Sort by Impact — no parenthetical (relative = absolute)
• Filter by developer + sort by column — both filter rank and impact rank shown
• Reload page — sort state preserved from URL

🤖 Generated with Claude Code

[msogin]

Review Loop — 2 Personas, 1 Iteration

Reviewers

• Reviewer 1: Sr. React Engineer (Architecture lens — extraction quality, prop contracts, hook composition, pattern consistency)
• Reviewer 2: Sr. Frontend Engineer (Correctness lens — sort logic, URL param safety, rank accuracy, regression risk, runtime errors)

---

Findings

Critical

[Critical] dev-table.tsx:~370 — window.innerHeight accessed in JSX render path inside JiraCountWithTooltip

The Jira tooltip's style block computes \${window.innerHeight - pos.top}px`directly in JSX. Theshow && typeof document !== 'undefined'guard protectscreatePortal's second argument (document.body) but does NOT guard window. In Next.js App Router, 'use client'components are still server-rendered to HTML for the initial payload. If React evaluates this JSX on the server,windowis undefined and throws aReferenceError`.

The commit tooltip (CommitCountWithTooltip) correctly avoids this: it stores all position data in pos state via getBoundingClientRect() inside handleMouseEnter (client-only) and never accesses window at render time. The Jira tooltip is inconsistent by design.

Fix: In JiraCountWithTooltip's handleMouseEnter, compute bottomOffset = window.innerHeight - rect.top and store it in pos state. Reference pos.bottomOffset in the style block instead of window.innerHeight - pos.top at render time.

---

Important

[Important] dev-table.tsx:~88-93 — absoluteRanks assumes developers prop is server-sorted by impact DESC — implicit contract not enforced

absoluteRanks maps array index position directly to rank number, trusting that incoming order is impact DESC. There is no type-level contract or assertion. If page.tsx ever passes a reordered array (e.g., future API change), ranks will silently mislabel.

Fix: Add a JSDoc comment to the developers prop in DevTableProps making the ordering contract explicit, or derive ranks inside the memo by sorting a copy by impact_score DESC rather than trusting incoming order.

---

[Important] dev-table.tsx:~129-138 — "Lines +/-" column header sorts only by lines_added, ignoring lines_removed — misleading

The column displays +added / -removed (net churn), but onSort('lines_added') only ranks by additions. A developer with 10k additions and 9k deletions sorts identically to one with 10k additions and 0 deletions. If additions-only is intentional, rename the header from "Lines +/-" to "Lines +" to remove the implication.

Fix: Either (a) rename the column header to "Lines +" to match the actual sort key, or (b) add a lines_net derived sort (additions − removals) and sort by that instead.

---

Suggestions / Deferred

• dev-table.tsx:82-83 — hasJira and hasSpend are bare .some() scans on every render; wrap in useMemo([developers, canAct]) for consistency with the rest of the memos in this component.
• dev-table.tsx:effectiveSortKey fallback — Stale URL param (e.g., devsort=total_jira_issues on a report without Jira) silently falls back to impact_score without correcting the URL. Consistent with team-table.tsx. Future improvement: call setSortKey('impact_score') to self-heal.
• dev-table.tsx:CommitCountWithTooltip + JiraCountWithTooltip — ~80 lines each, nearly identical structure (hover state, portal positioning, hideTimeout, cacheRef). File a follow-up to extract a shared useTooltipHover hook and <PortalTooltip> wrapper.
• URL key naming — TeamTable uses sort/dir; DevTable uses devsort/devdir. No collision today (tabs are mutually exclusive), but follow-up to rename TeamTable's keys to teamsort/teamdir for consistency.
• fetch URL encoding — login=${login} is not encodeURIComponent-wrapped; GitHub usernames are [a-zA-Z0-9-]-only so no practical risk, but defensive correctness would encode it.

---

Loop 1 Summary

#	Reviewer	Issue	Severity	Action
1	Correctness	window.innerHeight in Jira tooltip JSX — SSR ReferenceError risk	Critical	Would Fix
2	Architecture	useRouter still used in page.tsx (org click)	—	Skip (not a bug)
3	Correctness	absoluteRanks implicit ordering contract not documented	Important	Would Fix
4	Correctness	"Lines +/-" header sorts only by lines_added	Important	Would Fix
5	Architecture	URL key namespace inconsistency (sort vs devsort)	Important	Defer
6	Correctness	onSort ghost sortKey when effective falls back	—	Skip (not a live bug)
7	Architecture	hasJira/hasSpend not memoized	Suggestion	Defer
8	Architecture	Cache refs per-instance (correct tradeoff)	Suggestion	Defer
9	Architecture	Stale URL silent fallback (consistent with team-table)	Suggestion	Defer
10	Architecture	Tooltip code duplication	Suggestion	Defer
11	Correctness	hasJira SWR revalidation race	Suggestion	Defer
12	Correctness	Unencoded login in fetch URL	Suggestion	Defer

---

Verdict

Safe to merge with noted fixes. The component extraction is clean, pattern-consistent with team-table.tsx, and the sort + URL-persistence logic is correct. One Critical issue exists (window.innerHeight in the Jira tooltip render path) but its actual blast radius in Next.js App Router is limited — show starts false and handleMouseEnter only fires in the browser, so SSR will not trigger it in practice. Still, the inconsistency with the commit tooltip (which correctly avoids window at render time) should be fixed. The two Important issues (implicit rank-ordering contract, misleading column label) are also worth a quick amendment before or shortly after merge.

[msogin]

Smartling Code Review

Issues

---

🟡 warning

dev-table.tsx — commit_sha used as React key but not guaranteed unique

commits.map((c: any) => (<tr key={c.commit_sha} ...>)) — if the API returns duplicate rows or c.commit_sha is undefined, React will silently drop rows or throw a key warning. Use ${c.commit_sha}-${index} as a fallback.

---

🟡 warning

dev-table.tsx:~307 — fetch error swallows HTTP-level failures

A non-2xx response (401, 500) is silently treated as data and stored in cache. Add a response-status check:

const res = await fetch(`/api/report/${reportId}/commits?login=${login}`);
if (!res.ok) throw new Error(res.statusText);
const rows = await res.json();

Same issue exists in JiraCountWithTooltip (~line 380).

---

🟡 warning

dev-table.tsx — login is URL-interpolated without encoding in both tooltip components

GitHub logins are constrained to alphanumeric/hyphens so exploitability is low, but use encodeURIComponent(login) for correctness.

---

🟡 warning

dev-table.tsx:~330 — hideTimeout.current is never cleared on component unmount

If the tooltip unmounts while the 200ms hide timer is pending, setShow is called on an unmounted component. Add a cleanup:

useEffect(() => () => { if (hideTimeout.current) clearTimeout(hideTimeout.current); }, []);

Same issue in JiraCountWithTooltip.

---

🟡 warning

dev-table.tsx — loading state not reset to false on the cache-hit path

If a previous hover left loading=true and a second hover hits the cache, loading stays stuck at true. Set setLoading(false) on the cache-hit branch.

---

🟡 warning

dev-table.tsx:~398 — window.innerHeight referenced in JiraCountWithTooltip JSX

typeof document !== 'undefined' guards the createPortal call but window is a separate global. Use a translate approach consistent with CommitCountWithTooltip to remove the dependency.

---

🔵 suggestion

dev-table.tsx — CommitCountWithTooltip and JiraCountWithTooltip are nearly identical (~100 lines each)

A shared useHoverTooltip hook and generic TooltipPortal wrapper would eliminate the duplication and centralise the unmount-cleanup and loading-reset bugs so they only need to be fixed once.

---

🔵 suggestion

dev-table.tsx — pervasive use of any[] for cached data

Define minimal interfaces (CommitRow, JiraIssueRow) for the fields actually accessed. Removes non-null assertions and surfaces type errors at call sites.

---

🔵 suggestion

dev-table.tsx:~118 — tie-breaking sort is always ascending regardless of sortDir

if (av === bv) return (a.github_name || a.github_login).localeCompare(...)

Likely intentional as a stable tiebreaker, but worth a comment to prevent future confusion.

---

🟣 question

dev-table.tsx — setSortKey + setSortDir are two separate URL state writes in onSort

Depending on whether useUrlState batches these into a single replaceState, there may be a transient render where sortKey is updated but sortDir is still the old value, causing a flash of wrong sort order. Does useUrlState batch these?

---

🟣 question

dev-table.tsx — stale URL when effectiveSortKey silently falls back to impact_score

If a user bookmarks ?devsort=cc_total_cost but views a report where hasSpend is false, the table sorts by impact_score while the URL still says cc_total_cost. Should a useEffect correct the URL in this case?

---

Recommendations

Deduplicate tooltip components. The near-identical CommitCountWithTooltip / JiraCountWithTooltip pair is the primary maintainability risk. A shared useHoverTooltip hook and TooltipPortal component would cut ~100 lines and ensure the unmount-cleanup and loading-reset bugs are fixed in one place.

Harden the fetch path. Both tooltip components lack HTTP error checking and URL encoding — fix as a pair so one isn't missed.

Atomic URL sort state. The two-write pattern in onSort is a latent race condition. If useUrlState doesn't batch into a single replaceState, every column header click will produce visible flicker.

---

Assessment

Ready to merge? No
Reasoning: The fetch error-swallowing bugs (HTTP failures silently cached as data), missing unmount cleanup on hide timers, and loading-stuck-on-cache-hit are real correctness issues that should be fixed before shipping. The window.innerHeight inconsistency between the two tooltip components is also worth aligning. Remaining findings are lower priority but tooltip duplication makes the above harder to fix consistently.