fix(frontend): the person page reports one fact once - #2302
Merged
dzarlax merged 22 commits intoAug 7, 2026
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds catalog-aware metric queries, revises KPI and attention calculations, adds person-section standings, updates portal cohort controls, and refreshes dashboard and metric-card presentation. ChangesMetric catalog and insight calculations
Portal and dashboard experience
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 6
🤖 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/frontend/src/components/portal/metric-groups-view.tsx`:
- Around line 104-113: Update the metric-groups view flow around
previousGroupData and metricAttentionItems so that, when showKpis is true, the
previous-period query’s pending state participates in loading, its failure
participates in error handling, and retry re-runs that query alongside the
existing metric queries. Preserve the disabled behavior when showKpis is false,
and add tests covering pending and failed previous-period requests.
In `@src/frontend/src/components/portal/portal-topbar.tsx`:
- Around line 71-79: Update the TooltipTrigger in the cohort-help control to
render a focusable button instead of the current span, preserving its
aria-label, styling, and HelpCircle icon so keyboard users can access the
guidance.
In `@src/frontend/src/lib/insight/attention.ts`:
- Around line 115-117: Update the split attention value construction around
formatMetricNumber and metricDisplayUnit to use a shared formatter that
preserves the correct numeric and unit parts for percent metrics and all other
formats, so IcNeedsAttention retains the percent sign. Reuse API-provided metric
names, descriptions, and units without adding frontend-defined semantics, and
add coverage for percent formatting.
- Around line 68-72: The headline exclusion currently uses all KPI_ROW
candidates instead of only the metrics actually rendered by metricKpiTiles.
Derive the rendered headline keys once from the full KPI result, pass that set
into metricAttentionItems, and use it for exclusion while preserving the
existing attention filtering; add a regression test covering more than five
observed KPI candidates.
In `@src/frontend/src/lib/portal/use-person-sections.ts`:
- Around line 43-103: Add direct hook tests for usePersonSectionStandings
covering cohort injection, zero and null metric values, rank aggregation,
no-data status, and pending status; replace the always-empty test setup in
src/frontend/src/lib/portal/use-person-sections.ts lines 43-103 with
configurable loaded, empty, and pending data. In
src/frontend/src/components/portal/context-pane.test.tsx lines 34-39, make the
hook mock configurable and assert the rendered status mark and no-data label.
Ensure the affected frontend TypeScript changes reach at least 80% coverage.
In `@src/frontend/src/screens/dashboard.tsx`:
- Around line 124-127: Replace the hardcoded null near metricAttentionItems with
lightweight previous-period group projections, derive each prior projection’s
byKey map, and pass it into metricAttentionItems so eligible declining metrics
produce dashboard attention items. Add a dashboard test covering an eligible
declining metric.
🪄 Autofix
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: 8c94e2e2-2bc0-4cf3-a163-f8f3c3fa2f63
📥 Commits
Reviewing files that changed from the base of the PR and between f8f8b2f and 35824baace8b7160caa29099492bf0a4726413ca.
📒 Files selected for processing (40)
src/frontend/src/components/portal/ai-cost-view.test.tsxsrc/frontend/src/components/portal/attention-list.tsxsrc/frontend/src/components/portal/context-pane.test.tsxsrc/frontend/src/components/portal/context-pane.tsxsrc/frontend/src/components/portal/domain-lens-view.test.tsxsrc/frontend/src/components/portal/metric-groups-view.test.tsxsrc/frontend/src/components/portal/metric-groups-view.tsxsrc/frontend/src/components/portal/person-header.test.tsxsrc/frontend/src/components/portal/person-header.tsxsrc/frontend/src/components/portal/portal-shell.test.tsxsrc/frontend/src/components/portal/portal-topbar.tsxsrc/frontend/src/components/portal/slice-select.tsxsrc/frontend/src/components/portal/team-state-view.test.tsxsrc/frontend/src/components/widgets/dashboard/ic-needs-attention.test.tsxsrc/frontend/src/components/widgets/dashboard/ic-needs-attention.tsxsrc/frontend/src/components/widgets/dashboard/kpi-tile.tsxsrc/frontend/src/components/widgets/dashboard/members-grid.test.tsxsrc/frontend/src/components/widgets/metric-views/metric-group-card.test.tsxsrc/frontend/src/components/widgets/metric-views/metric-group-card.tsxsrc/frontend/src/components/widgets/metric-views/team-metric-group-card.test.tsxsrc/frontend/src/components/widgets/metric-views/team-metric-group-card.tsxsrc/frontend/src/components/widgets/period-selector-bar.test.tsxsrc/frontend/src/components/widgets/period-selector-bar.tsxsrc/frontend/src/lib/insight/attention.test.tssrc/frontend/src/lib/insight/attention.tssrc/frontend/src/lib/insight/group-data.tssrc/frontend/src/lib/insight/groups.tssrc/frontend/src/lib/insight/kpi-row.test.tssrc/frontend/src/lib/insight/kpi-row.tssrc/frontend/src/lib/insight/peer-pool.tssrc/frontend/src/lib/metrics/collection.test.tssrc/frontend/src/lib/metrics/collection.tssrc/frontend/src/lib/portal/use-person-sections.tssrc/frontend/src/lib/scoring.test.tssrc/frontend/src/lib/scoring.tssrc/frontend/src/queries/metric-definitions.tssrc/frontend/src/queries/metric-results.test.tsxsrc/frontend/src/queries/metric-results.tssrc/frontend/src/screens/dashboard.test.tsxsrc/frontend/src/screens/dashboard.tsx
💤 Files with no reviewable changes (1)
- src/frontend/src/components/widgets/metric-views/team-metric-group-card.tsx
Contributor
Author
|
All six taken, in Headline exclusion was too wide. Percent lost its unit. The list renders the number and the unit in The comparison period escaped the gates. It sits behind the new The legacy dashboard fetched no comparison at all, which the same rule The tooltip trigger is a Tests. |
dzarlax
force-pushed
the
fix/metric-keys-not-in-catalog
branch
from
August 7, 2026 07:09
a7602fa to
7290420
Compare
added 15 commits
August 7, 2026 09:11
`/v1/metric-results` resolves a request all-or-nothing: one key it cannot serve and the whole call is rejected with 400 UNAVAILABLE (`metric_definitions/repository.rs`), so a single missing metric blanks an entire screen instead of its own tile. The frontend's metric keys are compiled in, but the catalog is per installation — seeded from a registry that moves, and a tenant on an older backend or a different connector set simply has fewer keys. One key missing from a catalog is enough for a zone to render "Unable to load" while its other requests have all returned 200. `useMetricCollection` and `useMetricCollectionSet` now filter their collections through `filterCollectionToAvailable` before building a request: - keys the catalog does not list, and keys it lists as disabled, are dropped — sections already render a missing metric honestly, as not ingested; - requests WAIT for the catalog rather than firing unfiltered "for now", which would be the same 400 followed by a recovery on a second round-trip; - a collection filtered down to nothing sends no request at all, since an empty `metrics: []` is itself a 400, and reports the wait as pending so a screen shows a skeleton instead of a bare empty state; - if the catalog cannot be read, nothing is dropped — an unknown catalog must not silently shrink a request. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
"At a glance" painted five fixed slots. A person whose role produces no pull requests and no tasks gets dashes in those slots, while the metrics their work does register sit further down the page — the most valuable space on the screen spent on saying nothing. `KPI_ROW` becomes an ordered CANDIDATE list, and the row renders the first `KPI_ROW_MAX` a person is actually observed for. The tail (messages, meeting hours, wiki pages, commits) exists for that VP; it is a fallback, not a demotion, since a developer still leads with tasks and PRs — those come first and they have them. The test is "observed", not "non-zero": a developer who merged nothing this month KEEPS the empty PR tile, because a measured zero is a finding. Only a metric no connector feeds for that person drops out — `entityObserved` reads the peer target, so 0 stays and null goes. Both consumers now render the tiles rather than iterating the key list, so the row cannot paint a slot for a tile that does not exist. On the legacy screen that also collapses a wall of identical error cards into one: the request fails as a whole, so it says so once. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
A section card already carries its preview rows in status colours and a "N behind peers" badge over them. "Needs attention" re-listed those same rows above the cards, so every such finding reached the screen twice — and a reader counts red marks, not facts: a handful of findings can reach the screen as twice as many marks, and it reads as a verdict nobody made. The block now skips metrics in `def.card.preview` and keeps the rest: a bottom-quartile metric that sits outside the three rows the card had room for. That is its whole job — the finding you would otherwise miss. On the same page it goes from six repeated rows to one genuinely new one. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The value and the "−98% vs median 50,972" line under it shared a colour, so a single peer standing put two red marks on one tile. A reader counts marks, not findings, so a handful of them reads as twice as many problems — a verdict nobody made. The gap line is neutral now — it EXPLAINS the value rather than judging it a second time. The value keeps the peer verdict, and the period-over-period badge keeps its own colour because it states a different fact: how the number moved, not where it stands. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Four findings from the review of a live person page, all in the same
direction: the screen spent its room on things that said nothing, and said
nothing about the things it could do.
Empty sections. A section with no data for the period rendered as a
full-height "No data" card; a few of them take a third of the page and push
the work the person does register below the fold. They collapse into one line
that names them — a section with nothing this period is a fact worth stating
once. A section counts as empty only once its query has SETTLED, so a card
is never dropped and popped back while it loads.
Clickability. KPI tiles, section cards and attention rows are buttons whose
only affordance was `hover:bg-accent`, so the screen had to be swept with a
mouse to learn what opens. Each carries a standing chevron now. On the tile
it sits in the corner rather than beside the label: that line is the
scarcest on a 13rem card.
The delta. It sat up beside the label, reading as a second unrelated figure,
while it is a statement about the value ("−1 pp since last period"). It
moves down onto the value's line, pinned to the right edge so the badges
line up down a row of tiles. The label gets the full width back and wraps to
two lines instead of truncating — "Pull requests mer…" is not a shorter
name, it is a missing one.
Dates. The range read "31 Jul – 30 Jul" for a whole year, which everyone
took for a typo or a week. The year now appears when the range crosses one
or sits outside the current year, and stays out of the way otherwise.
Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Two leftovers from the same habit of restating a verdict. The coloured left stripe repeated what the card's badge already said, in a form that reads before any text does: four cards with four stripes made a page look like an alarm panel before a word of it had been read. The row values were coloured too, next to a dot that already carried the same standing, so a card of several rows read as several problems instead of one section worth opening. The dot stays — it marks WHICH row — and the number goes back to being a number. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…ale control Four things a second read of the live page turned up. The duplication fix was incomplete. Attention skipped what a card LISTS but not what it SAYS FIRST: the summary line is chosen from every metric of the group, not from the three in `card.preview`, so "Lines added · −98% vs median" was both the Git output headline and the first attention row. The headline choice moves into `lib/insight/group-data` so both surfaces read it from one place and cannot drift. The attention rows were a sentence, not a table. Each value started wherever its label happened to end, so four rows put four numbers at four different positions — and one column mixed "1 PRs" with "2.3". They are a grid now, with the value and the comparison in their own right-aligned columns. The columns carry a minimum width because each row is its own grid: `auto` sizes per row, which is the thing being fixed. The org-scope control named a different person than the page. The Person zone is about one person and reads nothing from the scope, so a reader saw two names in the same view and had to work out which owned the numbers. It is hidden there and stays everywhere it does something. Type treatments. A card ran black, 80% black and muted grey as three separate jobs on one surface, and the tile bolded a fragment inside a grey sentence. Two levels now: text that states something, text that labels it. Not done, deliberately: filling the cards with more metrics. This screen is the OVERVIEW and every section has its own detail view — more rows here would blur that line, and nothing is hidden behind a scroll for them to fix. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The previous pass stretched the label column to align the numbers, which aligned them at the far edge of the row: a value a hand's width from the thing it measures is alignment bought at the wrong price. The label column is fixed instead. Every value lands under the last one AND beside its own label, which is what a table does. The value column is wide enough for "27 conversations" and does not wrap, since a number broken across two lines is worse than either problem this was solving. The cost, stated: a very long metric name truncates. That is the trade a fixed column makes, and it is the better one here — names are short and predictable, numbers are what the eye is scanning for. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The card ran two independent mechanisms on one surface: a summary line chosen from EVERY metric of the group, and three fixed `card.preview` rows. They almost never agreed: a card either led with a metric the reader could not see anywhere on it, or led with one it then listed again a line below. There is one list now. The lead goes first in it, and its comparison sits under it rather than beside it: a card is a quarter of the content width, and a third column there truncated the very names it was explaining. The separate summary paragraph is gone, which also removes the third type treatment from the card. Only the lead carries "vs median" — it is the reason the card is worth opening. Putting it on every row is how the summary and the rows came to say the same thing twice in the first place. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The preview keys were kept even with no value, printing "—" to hold the card's shape across periods. That spends a line saying nothing, on every period the person has no data for that metric. The null here is not a measured zero: a zero prints 0 and stays, because a developer who merged nothing this month is a finding. A null means no connector feeds that metric for that person, which the card has no reason to announce. Same rule the headline row already follows, now the whole screen reads one way. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…peating it The person page is an overview and every section has its own screen, listed in the nav to the left. Cards on the page restated that list a second time and answered a question the nav already answers — while carrying three metrics each, dotted individually, so a row of cards put a dozen-odd status marks on a screen whose job was to point somewhere. The mark moves to the nav item. Each section carries one dot: its standing, or grey when it has no data this period — worth knowing before opening it. It costs no request; `usePersonSectionStandings` runs the section screens' own queries, which react-query serves from cache, and injects the same cohort peers so the mark and the screen it points at cannot disagree. With the cards gone, the attention block is the only place on the page that names problems, so it stops hiding any: it shows everything standing out except what the headline row already carries. It was skipping whatever a card listed, which now would mean skipping them everywhere. Two blocks left, each answering one question: what the numbers are, and what stands out. Which section to open is answered where you click. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The header always showed a button to the person's supervisor, and fetched that supervisor's record to build the siblings dropdown. Identity serves a viewer their own subtree, so anyone above them is not theirs to open — and the analytics read path does not enforce that on its own (constructorfabric#1995), which makes the link itself the problem: the UI was handing out an invitation the backend would answer. Both are gated on the supervisor being inside the viewer's subtree, the same test the "Team" button already used. The fetch is gated too, not just the button — skipping the button while still asking identity would leave exactly the request the guard exists to avoid. Also in this pass: the attention rows split the value into digits and unit, right- and left-aligned in their own columns, so the numbers line up on their last figure AND the units start together. One cell of "143 lines" could only ever give one of the two. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Two changes to the same problem: the page said a lot about where a person stands and nothing about whose standard that is. "Needs attention" was everything in the bottom quartile of the cohort. For anyone whose job differs from the people they are measured against — a lead against the developers reporting to them — that is a permanent state, not an event, and a block that repeats it every month teaches the reader to skip it. An item now has to be below the cohort AND to have moved against itself by at least a tenth since the previous period. A structural gap is flat and stays quiet; a regression comes through. Without a previous period the block makes no claim at all rather than reporting a standing as news. The page names the pool it compares against, in the header of the headline row. Every "vs median" on screen was computed against people the reader could not see or count, which is also what hid the case worth seeing: a lead being measured against a whole department. Pools differ per metric, so the most common size is reported rather than the largest — the typical comparison, not the most flattering one. The control is called "Cohort" now, with the word outside the dropdown and a tooltip beside it: it decides who every comparison on every screen is made against, and "Slice" said none of that. Also here: the legacy dashboard passes no previous period for its groups, so its attention block goes quiet by the same rule. Deliberate — better silent than restating standings as events. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Comments and messages in this branch reached for concrete illustrations — role titles, message counts, mark tallies from a screen someone was looking at. The repository rule is that nothing GitHub-visible may be derived from a deployed environment, and an anonymised count is still derived from one. Each is restated as the technical condition it was illustrating: a role that produces no pull requests rather than a named one, "a handful of findings read as twice as many marks" rather than a tally, a card leading with an invisible metric rather than the two that did. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Six corrections from review of the attention block and the sections nav. - The attention block excluded every headline CANDIDATE rather than the keys the row actually rendered, so a metric that reached neither surface was visible nowhere. It now takes the rendered keys. - Percent values lost their unit in the split value columns, turning 50% into a bare 50. splitMetricValue keeps the sign the format implies. - The comparison period was fetched outside the loading and error gates, so a failed comparison rendered as "nothing needs attention" instead of an error. Both views now gate and retry on it. - The legacy dashboard did not fetch a comparison at all, which the new rule would have silently emptied. - The cohort tooltip trigger was a span, unreachable by keyboard. - The section standings had no tests and the nav mock returned a constant, so neither the mark nor its three states were exercised. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
dzarlax
force-pushed
the
fix/metric-keys-not-in-catalog
branch
from
August 7, 2026 07:13
7290420 to
548978e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/src/lib/portal/use-person-sections.test.tsx (1)
121-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete configured section set.
The test only checks two section IDs. An omitted section such as
git_outputwould still pass.Compare the returned IDs with all IDs from
GROUPS.🤖 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/frontend/src/lib/portal/use-person-sections.test.tsx` around lines 121 - 125, Update the test case “covers every section, including ones the response never mentions” to compare the complete returned ID set from standings() against all section IDs defined by GROUPS, rather than asserting only task_delivery and wiki. Preserve the existing behavior assertion while ensuring omitted sections such as git_output cause the test to fail.
🤖 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/frontend/src/screens/dashboard.tsx`:
- Around line 77-87: Update the dashboard attention flow around
previousGroupData to expose an attention-specific pending state while any
comparison query is loading, and an error state when any result fails. Gate
attention rendering on these states instead of relying only on isLoading, and
provide a retry action that retries every failed previousGroupData query before
showing “no needs attention.”
---
Nitpick comments:
In `@src/frontend/src/lib/portal/use-person-sections.test.tsx`:
- Around line 121-125: Update the test case “covers every section, including
ones the response never mentions” to compare the complete returned ID set from
standings() against all section IDs defined by GROUPS, rather than asserting
only task_delivery and wiki. Preserve the existing behavior assertion while
ensuring omitted sections such as git_output cause the test to fail.
🪄 Autofix
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: c7902ee2-8103-4de3-b4f7-6bd2eff8a55a
📥 Commits
Reviewing files that changed from the base of the PR and between 35824baace8b7160caa29099492bf0a4726413ca and 729042011d3f204d665b6c2f869bd2ac71a78a29.
📒 Files selected for processing (8)
src/frontend/src/components/portal/context-pane.test.tsxsrc/frontend/src/components/portal/metric-groups-view.tsxsrc/frontend/src/components/portal/portal-topbar.tsxsrc/frontend/src/lib/format.tssrc/frontend/src/lib/insight/attention.test.tssrc/frontend/src/lib/insight/attention.tssrc/frontend/src/lib/portal/use-person-sections.test.tsxsrc/frontend/src/screens/dashboard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/src/components/portal/metric-groups-view.tsx
- src/frontend/src/components/portal/portal-topbar.tsx
aleksdotbar
approved these changes
Aug 7, 2026
added 5 commits
August 7, 2026 10:06
The storybook project's first run against a cold cache discovered a dozen dependencies only after the stories had mounted, re-optimized, and reloaded the page. The reload aborted whatever was in flight — in practice a dynamic import, which then failed with "Failed to fetch dynamically imported module" and took an unrelated test down with it. The failure only appears with a cold dependency cache, so it never reproduces locally and reads as a flake in CI. Any change to the import graph can move which dependencies the pre-bundling scan reaches, so this is not tied to one branch. Pin them in optimizeDeps.include, which is what vite's own warning asks for, and record the symptom that identifies a missing entry. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
A number with a name is not readable on its own: "Focus Time 51%" gives the reader no way to learn what it counts or how. The catalog already writes both — every metric result carries `description` and `explanation` — and the portal rendered neither, except behind a settings toggle on one surface. Put that copy where the metric is named: - KPI tiles and needs-attention rows make the CONTROL the trigger. Both are already buttons, so this costs no extra icon and no extra tab stop, and a keyboard reader gets the same answer on focus. - `MetricName` covers names that sit inside a card that is itself a button — section-card rows, team-card rows, a summary card's title, the direction lens tiles. There the trigger is a span, pointer only: a focusable element cannot be nested in a control. The card opens onto the same copy. - The members grid already had a column tooltip; the description joins the name it explains. A metric the catalog says nothing about renders exactly as before — an empty bubble is worse than no affordance, because it promises an answer. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The block reported metrics that moved against their own past, which hid the worst finding a person can have: one that fell to almost nothing long ago and has been flat since. No period shows a change, so the block says nothing, and an empty block reads as "all clear". Two ways in now, named apart on the row: - a fall — moved adversely by at least a quarter of the cohort's spread - a standing — at a quarter of what a typical peer does, or less, moved or not Where the previous period holds no value the row can still appear as a standing; "no earlier period" is its own answer rather than silence. Three corrections come with it: - The verdict is the shared standing derivation, not a raw quartile test, so the block obeys the eligibility guards the tile beside it obeys. A cohort with no spread ranked somebody bottom for being one of several equal numbers. - The move threshold is measured in cohort spreads rather than as a share of the person's own previous value, which was easiest to clear where the evidence is thinnest: two of something falling to one is a fifty-percent collapse and one event of difference. - For metrics counted in whole events, a standing needs the typical peer to be doing at least a handful. Zero against a median of one is a hundred-percent gap and no finding at all, and a share of a tiny number is always large, so those rows shouted loudest. Ordering follows: distance in spreads, not in percent of the median, because a percentage of a small median puts the rounding error first. One fact reaches the reader once. Metrics that contain others, and metrics that restate others in different units, collapse to the more specific one — relationships taken from the catalog's definitions, never inferred from two series moving together. The row that carries all this was a fixed six-column grid, so it decided what to sacrifice under width pressure before knowing what was in the cells. At a phone width the columns resolved to 40px of name, a full-width unit column holding mostly whitespace, and nothing at all left for the comparison — the one number the row exists to show. Names rendered as "Em…", "Me…", "AI c…". Columns are content-sized now and only the comparison flexes, so it is cut last rather than first; below the small breakpoint the row becomes two lines, because at that width no arrangement of six columns leaves a name legible. The columns also belonged to the row rather than to the list, so each row's widths were decided by its own contents and lined up with nobody. The list owns one grid and every row is a subgrid of it, which keeps the row a real button with its own padding and hover. What kind of finding a row is now sits beside the name it qualifies, as a chip; at the far edge of the row it was read as a fifth number. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
A tile put two different questions in one red/green channel: the number was coloured by where the person stands against the cohort, the badge beside it by which way the number moved against its own past. A red value touching a green badge on the same baseline reads as a contradiction rather than as two facts, and neither could be decoded without the legend. The value is plain ink now. The standing moved to the line that states the comparison, marked by a dot as well as a colour so it survives a reader who cannot tell the two colours apart — the badge already had an arrow to fall back on, the value had nothing at all. Also on the tile: - Moves of a percent or a point no longer take alarm colour. The number is still shown; four coloured badges in a row, one of them for a move the size of rounding, teach the reader that the colour means nothing. - The footer is pinned to the bottom with a two-line reserve, so a row of tiles keeps its footers on one line even where a badge wraps under its value. They drifted apart by a line's height at tablet widths. - A value exactly at the median says "at median N" rather than dropping the comparator and reading as a stray label. The headline row also spends each of its four slots on a different fact: two metrics that restate one another used to take half the row to say one thing from opposite ends. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The tiles were laid out with auto-fit, which packs in as many as the width allows and leaves whatever is left over stranded on a line of its own. Five candidates in a four-column row put the fifth beside a hole three tiles wide, and a hole reads as something that failed to load. The column count is counted now, and follows how many tiles there actually are: a person with two measurements gets two full-width tiles rather than half a row of blanks. The row is capped at four so every count divides it. Each tile carries two comparisons — a badge against the person's own previous period, a line against the cohort — and nothing said which was which, so a lone "-13%" could be read as either. Named once above the row rather than on every tile: the grammar is the same for all of them, and a tile is thirteen rems wide. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Contributor
Author
|
Three more commits, from a fresh look at the person screen after the review
Three corrections travel with it: the verdict is the shared standing One fact also reaches the reader once now: metrics that contain others, and
Verified against a local install at three widths; unit and story suites green, |
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 (1)
src/frontend/src/components/portal/metric-groups-view.tsx (1)
192-217: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude cohort query state before rendering cohort comparisons.
buildMetricCollectionRequest()addspeerfor the person’s own collection, butinjectCohortPeer()only copies existingpeervalues. Since thecohortKpi/cohortGroupqueries are not part of the page’sisLoading,isError, or retry handling, the selected-cohort comparison label can render before cohort peer data is usable. Add the cohort queries’ pending, error, and retry handling.🤖 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/frontend/src/components/portal/metric-groups-view.tsx` around lines 192 - 217, Include the cohortKpi and cohortGroup query states in the metric view’s isLoading and isError checks before rendering comparisons, and invoke their refetch methods in the existing onRetry handler. Preserve the current handling for kpiData, previousGroupData, and groupData while ensuring cohort requests block rendering when pending and surface failures as retryable errors.
🤖 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/frontend/src/components/widgets/dashboard/members-grid.tsx`:
- Around line 737-740: Update the metric-help rendering in the members grid to
also render help.explanation when provided, placing it after help.description
when both are present. Preserve the existing conditional behavior and use the
API-supplied text without adding frontend-defined metric semantics.
In `@src/frontend/src/components/widgets/metric-help-tooltip.tsx`:
- Around line 78-85: Update MetricName so metrics with help expose that content
to keyboard and screen-reader users instead of relying on a non-focusable span;
use the tooltip library’s accessible trigger pattern or provide an equivalent
non-nested focusable trigger without breaking surrounding button semantics.
Preserve the existing label styling and no-help rendering, and add a keyboard
interaction test covering access to the help content.
---
Outside diff comments:
In `@src/frontend/src/components/portal/metric-groups-view.tsx`:
- Around line 192-217: Include the cohortKpi and cohortGroup query states in the
metric view’s isLoading and isError checks before rendering comparisons, and
invoke their refetch methods in the existing onRetry handler. Preserve the
current handling for kpiData, previousGroupData, and groupData while ensuring
cohort requests block rendering when pending and surface failures as retryable
errors.
🪄 Autofix
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: 3d56cb74-3c9e-441f-8b37-87c0d8f15543
📥 Commits
Reviewing files that changed from the base of the PR and between 729042011d3f204d665b6c2f869bd2ac71a78a29 and f144bda.
📒 Files selected for processing (27)
src/frontend/src/components/portal/domain-lens-view.tsxsrc/frontend/src/components/portal/metric-groups-view.tsxsrc/frontend/src/components/widgets/dashboard/ic-needs-attention.test.tsxsrc/frontend/src/components/widgets/dashboard/ic-needs-attention.tsxsrc/frontend/src/components/widgets/dashboard/kpi-tile.stories.tsxsrc/frontend/src/components/widgets/dashboard/kpi-tile.test.tsxsrc/frontend/src/components/widgets/dashboard/kpi-tile.tsxsrc/frontend/src/components/widgets/dashboard/members-grid.tsxsrc/frontend/src/components/widgets/metric-help-tooltip.tsxsrc/frontend/src/components/widgets/metric-views/metric-group-card.tsxsrc/frontend/src/components/widgets/metric-views/metric-summary-card.tsxsrc/frontend/src/components/widgets/metric-views/team-metric-group-card.tsxsrc/frontend/src/lib/insight/attention.test.tssrc/frontend/src/lib/insight/attention.tssrc/frontend/src/lib/insight/groups.tssrc/frontend/src/lib/insight/kpi-row.test.tssrc/frontend/src/lib/insight/kpi-row.tssrc/frontend/src/lib/insight/metric-containment.test.tssrc/frontend/src/lib/insight/metric-containment.tssrc/frontend/src/lib/insight/metric-help.test.tssrc/frontend/src/lib/insight/metric-help.tssrc/frontend/src/lib/metrics/delta.test.tssrc/frontend/src/lib/metrics/delta.tssrc/frontend/src/lib/metrics/peer-standing.tssrc/frontend/src/screens/dashboard.test.tsxsrc/frontend/src/screens/dashboard.tsxsrc/frontend/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/frontend/src/lib/insight/kpi-row.test.ts
- src/frontend/src/screens/dashboard.tsx
- src/frontend/src/components/widgets/metric-views/team-metric-group-card.tsx
- src/frontend/src/components/widgets/dashboard/kpi-tile.tsx
- src/frontend/src/screens/dashboard.test.tsx
- src/frontend/src/components/widgets/metric-views/metric-group-card.tsx
Review findings from the last round, all in the same family: a query the page reasons from was fetched but not waited for. - The legacy dashboard fetched the comparison period without gating on it. The attention block needs it to tell a change from a standing, so a pending or failed comparison rendered as an empty block — which reads as "nothing worth looking at" rather than "not compared". It now waits, and a failure is retryable instead of silent. - The person screen left the cohort's own results out of its loading, error and retry handling. Every comparison on the page is drawn against them, and peer values are only copied in once they arrive, so the screen could show someone measured against a cohort that was not there yet — under a heading naming how many people are in it. - A grid column header rendered only the catalog's short description. Where the catalog supplies just the longer explanation the header said nothing at all, leaving the column a guess. - A metric name outside an interactive surface offered its meaning to a pointer and to nothing else. The trigger cannot be made focusable there — these names sit inside cards that are themselves buttons, and a control nested in a control is invalid markup — so the words now reach assistive technology directly, in the reading order they belong to. - The section-standings test asserted two entries of the configured set. A section dropped from the list would have lost its mark in the nav unnoticed; it now asserts the whole set. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Contributor
Author
|
All four taken, plus the nitpick, in Gate attention on comparison-query state ( Include cohort query state before rendering cohort comparisons Render explanation-only metric help ( Make Assert the complete configured section set — done; the test now compares 942 tests pass, |
dzarlax
enabled auto-merge
August 7, 2026 10:55
aleksdotbar
approved these changes
Aug 7, 2026
This was referenced Aug 7, 2026
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.
What
The person page, gone over surface by surface after a design review. Fourteen
commits, each one thing; the diff is frontend-only.
The through-line is not "less red" — it is one fact, one place. A screen
that shows a finding twice does not report two facts, it reports one and looks
twice as alarming; a reader counts marks, not findings.
The bug that started it
/v1/metric-resultsresolves a request all-or-nothing: one key it cannot serveand the whole call is rejected with 400 UNAVAILABLE, so a single missing metric
blanks a zone instead of its own tile. The frontend's metric keys are compiled
in while the catalog is per installation — seeded from a registry that moves,
and a tenant on an older backend or a different connector set simply has fewer
keys. One key missing is enough for "At a glance" to render "Unable to load"
while its other four requests returned 200.
Requests are now filtered through the installation's own catalog. They wait for
it rather than firing unfiltered "for now" — that would be the same 400
followed by a recovery on a second round-trip — and a collection filtered down
to nothing sends no request at all, since an empty
metrics: []is itself a400. If the catalog cannot be read, nothing is dropped: an unknown catalog must
not silently shrink a request.
This is latent wherever the catalog and the frontend agree, and it fires
wherever they do not.
What the page shows now
Three blocks, three questions, no overlap.
The section cards are gone. Every section has its own screen, listed in the nav
to the left; cards restated that list a second time while carrying three
metrics each, dotted individually, so a row of them put a dozen-odd status
marks on a screen whose job was to point somewhere. The mark moved to the nav
item — one dot per section, grey when the section has no data this period. It
costs no request:
usePersonSectionStandingsruns the section screens' ownqueries, which react-query serves from cache, and injects the same cohort peers
so the mark and the screen it points at cannot disagree.
"Needs attention" reports changes, not standings. It used to be everything
in the bottom quartile of the cohort. For anyone whose job differs from the
people they are measured against — a lead against the developers reporting to
them — that is a permanent state, not an event, and a block repeating it every
period teaches the reader to skip it. An item now has to be below the cohort
AND to have moved against itself by at least a tenth since the previous period.
A structural gap is flat and stays quiet; a regression comes through. Without a
previous period the block makes no claim at all.
The page names the pool it compares against. Every "vs median" on screen was
computed against people the reader could not see or count — which is also what
hid the case worth seeing, a lead measured against a whole department. Pools
differ per metric, so the most common size is reported rather than the largest:
the typical comparison, not the most flattering one. The control is called
Cohort now, with the word outside the dropdown and a tooltip beside it.
Honesty rules, applied consistently
person; a measured zero prints 0 and stays, because a developer who merged
nothing this period is a finding. Rows and tiles with no value are dropped,
everywhere, by the same rule.
KPI_ROWbecame anordered candidate list; the row renders the first few the person is observed
for. The tail is a fallback, not a demotion — a developer still leads with
tasks and PRs because those come first and they have them.
under it explains rather than judging a second time. Row values in a card lost
their colour to the dot that already carried the same standing, and the
coloured left stripe went with them.
section while the rows below show fewer, and a bare count read as a claim
about those rows and contradicted them.
Access
The header always offered a button to the person's supervisor and fetched that
supervisor's record for the siblings dropdown. Identity serves a viewer their
own subtree, so anyone above them is not theirs to open — and the analytics read
path does not enforce this on its own (#1995), which makes the link itself the
problem. Both the button and the fetch are gated on the supervisor being inside
the viewer's subtree.
Legibility
Full metric names that wrap rather than truncate — "Pull requests mer…" is not a
shorter name, it is a missing one. The period range names its year when the
range crosses one or sits outside the current year. Attention rows are a grid:
label, digits right-aligned, unit left-aligned, comparison — so numbers line up
on their last figure and units start together. Every openable surface carries a
standing chevron instead of a hover-only tint. Sections with no data collapse
into the nav's grey mark instead of full-height "No data" cards.
What this cannot fix
The cohort is
org_unit: a lead is compared with everyone in their department,including people doing a different job. Any threshold or presentation built on
top of that answers a question nobody asked, and that is a backend concern —
metric_entity_cohorts_currentandcohort_key. Naming the pool on the page atleast makes it visible to whoever opens the screen.
Two more are registry matters, listed for the record:
collab.focus_time_pctcounts time outside meetings, which is not focus time, and
ai.accepted_linescarrieshigher_is_better, so a larger volume ofAI-generated code reads as a better outcome.
Verification
pnpm typecheckclean,eslint --max-warnings 0clean, 848 unit tests pass.Each change was checked in a running app against a real dataset before being
committed; one direction was tried, seen to concentrate the problem rather than
remove it, and dropped before this branch was opened.
Summary by CodeRabbit