feat(frontend): the person page is the person's own - #2340
Conversation
The page opened with four numbers and a red list, and read as a complete picture of a person. For someone whose work leaves few traces in the connected systems, that picture is mostly their chat and their calendar. It now says what it cannot see before it says anything else: the sections with no reading at all, named, above the numbers. Placed there because on a person's own page the first question is what this thing knows about them, and a page that answers it at the bottom has already been read as complete by the time the answer arrives. "No data reaches us", not "no activity": the two are different, and only one of them is something we know. The standings behind the line are the ones the navigation already asked for, so this costs no request. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
A tile carried two comparisons and named neither: a badge against the person's own last period, and a line against the cohort median. A bare "-13%" could be read as either, and the cohort read as the verdict. On a person's own page that ranking is backwards. They did not choose the cohort, cannot see who is in it, and cannot decide that its median is the right target — so it is context. Their own last period is the one comparison they chose and the only one they can act on. The value is plain ink. Under it, coloured and named, the change since their last period. Under that, quietly and without a verdict, the cohort. The badge is gone: its content moved into the line, where it can say what it compares against, which also retires the header clause that existed to explain it. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Every number on the page was a single period plus two comparisons, and none of them could say whether what the reader is looking at is new, recovering, or how it has always been — three findings that call for three different conversations. A line now sits beside each headline value, in the unit the reader picked, and three things about it are deliberate: - The running period is not drawn. A month four days old holds four days of work; plotted beside finished months it invents a collapse, and nothing on a tile could tell the reader that the last point means something else. - Fewer than three readings draw nothing. Two points always make a straight line, and a straight line reads as a steady trend — but two is also exactly what a metric has when it has only just started being measured. - A missing reading breaks the line rather than dropping it to the floor: a period with no data must not be readable as a period with none of the thing. The line is scaled to its own range and says only which way and how steadily — the figures beside it carry the magnitude. It has a rule beneath it so it claims a span of time rather than floating, and the span itself is named once above the row, since a tile has no room for an axis. The request sits outside every loading and error gate: the tiles render from the current period and the line appears when it arrives. A first screen must not wait on it. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Three changes that only make sense together, all following from the person page being the person's own. **The attention block orders by who is reading.** A standing is the larger claim and leads by default — what a manager scanning someone else needs. On your own page the order inverts: what moved this period is what you can still do something about, while what has been true for months you have already lived through. Neither order hides anything; both kinds are always listed. **A click on a number opens the section that owns it.** That was already the intent, but a dead flag stood in the way: `showSections` gated the per-group cards AND the drilldown sheets, and the cards moved to the navigation long ago. What survived was a flag whose only remaining effect was to make a click land nowhere on any screen that set it. Removed; the sheets now follow the one question that matters — whether the caller routes clicks elsewhere. **The page's own description is true again.** It promised per-section status cards that have not been there since the sections moved to the navigation. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The story asserted a bare "+9%", which the tile no longer renders: the change now names what it is measured against, because unnamed it was readable as either of the tile's two comparisons. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
Warning Review limit reached
Next review available in: 26 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 (10)
📝 WalkthroughWalkthroughThe PR adds person KPI trend data, sparkline rendering, coverage messaging, viewer-aware attention ordering, and updated KPI comparison text. It removes the ChangesPerson KPI flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MetricGroupsView
participant PersonTrend
participant KpiTile
MetricGroupsView->>PersonTrend: request completed person trend readings
PersonTrend-->>MetricGroupsView: return nullable trend points
MetricGroupsView->>KpiTile: pass trend points and period noun
KpiTile-->>MetricGroupsView: render KPI value, sparkline, and comparisons
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: 1
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/widgets/dashboard/kpi-tile.test.tsx (1)
35-144: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd automated coverage for the new trend path.
The change adds
trendintoKpiTile, pluspersonTrendPoints,trendRange,runningBucketStart, andSparkline, butsrc/frontend/srchas no test coverage for these newts/tsxlines. Also cover thetrendbranch inkapi-tile.test.tsxplus the helper and Sparkline cases.🤖 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/widgets/dashboard/kpi-tile.test.tsx` around lines 35 - 144, Add automated tests for the new trend path: in src/frontend/src/components/widgets/dashboard/kpi-tile.test.tsx (lines 35-144), cover KpiTile rendering when trend data is provided; in src/frontend/src/lib/portal/person-trend.ts (lines 19-86), test personTrendPoints, trendRange, and runningBucketStart across normal and boundary inputs; and in src/frontend/src/components/widgets/dashboard/sparkline.tsx (lines 21-112), cover Sparkline rendering and its empty, single-point, and range-handling cases.Source: Coding guidelines
🤖 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/person-coverage.tsx`:
- Around line 17-23: Add focused tests for PersonCoverage covering both
branches: assert it renders nothing when uncovered is empty, and assert the
coverage message includes the joined uncovered values when the array is
populated. Use the existing frontend component-testing conventions and keep the
tests scoped to this component’s behavior.
---
Outside diff comments:
In `@src/frontend/src/components/widgets/dashboard/kpi-tile.test.tsx`:
- Around line 35-144: Add automated tests for the new trend path: in
src/frontend/src/components/widgets/dashboard/kpi-tile.test.tsx (lines 35-144),
cover KpiTile rendering when trend data is provided; in
src/frontend/src/lib/portal/person-trend.ts (lines 19-86), test
personTrendPoints, trendRange, and runningBucketStart across normal and boundary
inputs; and in src/frontend/src/components/widgets/dashboard/sparkline.tsx
(lines 21-112), cover Sparkline rendering and its empty, single-point, and
range-handling cases.
🪄 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: a90540c3-c9d5-4998-8c97-2e18298841dc
📒 Files selected for processing (11)
src/frontend/src/components/portal/metric-groups-view.tsxsrc/frontend/src/components/portal/person-view.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/person-coverage.tsxsrc/frontend/src/components/widgets/dashboard/sparkline.tsxsrc/frontend/src/lib/insight/attention.test.tssrc/frontend/src/lib/insight/attention.tssrc/frontend/src/lib/portal/person-trend.tssrc/frontend/src/screens/dashboard.tsx
| export function PersonCoverage({ uncovered }: PersonCoverageProps) { | ||
| if (uncovered.length === 0) return null; | ||
| return ( | ||
| <p className="text-xs text-muted-foreground"> | ||
| No data reaches us for {uncovered.join(", ")} — this page shows what is | ||
| measured, not everything this person does. | ||
| </p> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'person-coverage\.tsx$' . | sed 's#^\./##' || true
fd -a 'package\.json$|coverage|test|vitest|jest' . | sed 's#^\./##' | head -200
echo
echo "== component file =="
for f in $(fd 'person-coverage\.tsx$' .); do
echo "--- $f"
wc -l "$f"
cat -n "$f"
done
echo
echo "== references to PersonCoverage =="
rg -n "PersonCoverage|personCoverage|person-coverage" . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo
echo "== search coverage-related files =="
git ls-files | rg -i 'coverage|coverage-report|vitest|jest|testcoverage|lcov|codecov' | head -200
echo
echo "== frontend package scripts =="
for f in $(git ls-files '*package.json'); do
echo "--- $f"
python3 - <<PY
import json, pathlib, sys
p=pathlib.Path("$f")
try:
data=json.loads(p.read_text())
except Exception as e:
print("json error", e)
raise SystemExit
for section in ("scripts","devDependencies","dependencies","jest","vitest"):
if section in data:
print(f"[{section}]")
v=data[section]
if isinstance(v, dict):
for k,vv in v.items():
if any(s in k or s in str(vv) for s in ("test","coverage","frontend","vitest","jest")):
print(f"{k}: {vv}")
else:
print(v)
PY
done
echo
echo "== tests mentioning PersonCoverage or dashboard widgets =="
rg -n "PersonCoverage|personCoverage|coverage|no data reaches us|uncovered" . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: constructorfabric/insight
Length of output: 15347
🏁 Script executed:
#!/bin/bash
set -eu0o pipefail
# Inspect repository coverage/test evidence without running repository code.
echo "== git status/stat =="
git status --short || true
git diff --stat || true
echo
echo "== tracked coverage/test config files =="
git ls-files | rg -n '^(coverage|src/frontend/.*/.*\.?(config|test)|.*(test|spec|coverage|vitest|jest).*\.(ts|tsx|js|mjs|cjs|json|ya?ml))$|vitest|jest|coverage' | head -300
echo
echo "== exact component path from git ls-files =="
git ls-files | rg 'person-coverage\.tsx$|package\.json$' | head -200Repository: constructorfabric/insight
Length of output: 17930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/frontend/src/components/widgets/dashboard/person-coverage.tsx =="
wc -l src/frontend/src/components/widgets/dashboard/person-coverage.tsx
cat -n src/frontend/src/components/widgets/dashboard/person-coverage.tsx
echo
echo "== candidate nearby/person coverage tests =="
git ls-files | rg 'src/frontend/src/components/widgets/dashboard/(person-coverage|.*coverage|.*test)\.(ts|tsx)$|src/frontend/src/components/widgets/dashboard/.*/.*test\.tsx$|src/frontend/src/.*test\.tsx$' | rg -i 'coverage|person' || true
echo
echo "== all occurrences of PersonCoverage ==="
rg -n "PersonCoverage" src/frontend || true
echo
echo "== package dev/test/coverage scripts =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path("src/frontend/package.json")
data=json.loads(p.read_text())
for section in ("scripts","devDependencies","dependencies"):
print(f"[{section}]")
for k, v in data.get(section, {}).items():
if any(s in k or s in str(v).lower() for s in ("test","coverage","vitest","frontend")):
print(f"{k}: {v}")
PY
echo
echo "== vitest config coverage settings =="
cat -n src/frontend/vitest.config.ts | sed -n '1,220p'Repository: constructorfabric/insight
Length of output: 8991
Add coverage for the PersonCoverage empty and rendered branches.
PersonCoverage has a no-data branch and a rendered message branch, but there is no test evidence for either path. Add focused tests for both branches so new frontend changes stay at 80% coverage.
🤖 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/widgets/dashboard/person-coverage.tsx` around
lines 17 - 23, Add focused tests for PersonCoverage covering both branches:
assert it renders nothing when uncovered is empty, and assert the coverage
message includes the joined uncovered values when the array is populated. Use
the existing frontend component-testing conventions and keep the tests scoped to
this component’s behavior.
Source: Coding guidelines
… the wrong one The coverage line said "no data reaches us" of every section with no reading. Two different things arrive that way, and only one of them is that sentence: a source nothing is wired to, and a source that works fine and holds none of this person's work. Told the second reader their tracker is broken while their colleagues' numbers for that same section sat one screen away. The pool tells them apart and was already in the response. A peer view counts the entities that had a reading, so an empty pool across every metric of a section means nobody is measured there, while a pool that reads means the measurement works and this person is absent from it. Not proof — a section whose whole cohort was idle looks the same — but it fails toward the weaker claim, which is the safe direction. Understating what we know costs a reader nothing; calling a working source broken costs them their trust in the page. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
Every rule in the trend path is a refusal, and a refusal looks like a missing feature from the outside. Left untested, "why is there no line here" gets answered by deleting the rule. So each one is now a test that says what it costs to remove: the running period is dropped because a four-day month drawn beside finished months reads as a collapse the calendar invented; fewer than three readings draw nothing because two points always make a straight line and a straight line reads as a steady trend; a missing reading breaks the line because a period with no data must not read as a period with none of the thing. A gap cannot buy its way past the minimum either. The tile is covered for what it must not do while readings are in flight: the numbers render first and the line appears when it arrives, so the first screen never waits on a second request. The line carries a data-slot so a test can tell it from the arrow in the delta. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
|
Both findings were right — neither path had a test. Addressed in Trend path. Added PersonCoverage. Tested, and testing it turned up a defect worth more than the missing coverage. The component said "no data reaches us" of every section with no reading, but two different things arrive that way: a source nothing is wired to, and a source that works and holds none of this person's work. The second reader was being told their tracker is broken while their colleagues' numbers for that same section sit one screen away. The signal was already in the response — a peer view counts the entities that had a reading, so an empty pool across every metric of a section means nobody is measured there, and a pool that reads means the measurement works and this person is absent from it. Two sentences now, never the wrong one. It is not proof (a wholly idle cohort looks the same) but it fails toward the weaker claim, which is the safe direction. 966 tests pass across both projects, |
3c74841
2 participants
Whose page this is
The person page is the person's own — for most readers it is the first screen
they meet, and they did not ask to be measured. That answers a question the
page had been getting wrong: what leads, what recedes, and what it owes the
reader before any number.
What changed
It names its own limits first. The page opened with four numbers and a red
list, and read as a complete picture of a person. For someone whose work leaves
few traces in the connected systems, that picture is mostly their chat and
their calendar. It now says which sections have no reading at all, above the
numbers — because the first question on your own page is what this thing knows
about you, and an answer at the bottom arrives after the page has already been
read as complete. "No data reaches us", not "no activity": only one of those is
something we know. No extra request — the standings are the ones the navigation
already asked for.
The tile leads with the comparison the reader owns. It carried two and
named neither: a badge against the person's own last period, a line against the
cohort median, so a bare "-13%" was readable as either. The reader did not
choose the cohort, cannot see who is in it, and cannot decide its median is the
right target — that is context. Their own last period is the one comparison
they can act on. Value in plain ink; under it, named and coloured, the change;
under that, quietly, the cohort. The badge is gone, and with it the header
clause that existed to explain it.
Each headline number carries where it has been. A single period plus two
comparisons cannot say whether something is new, recovering, or how it has
always been — three findings that call for three different conversations. A
line now sits beside each value, in the unit the reader picked.
Whose page it is decides what comes first. A standing is the larger claim
and leads by default, which is what a manager scanning someone else needs; on
your own page what moved this period leads, because that is what you can still
act on. Neither order hides anything.
Three deliberate refusals in the line
work; beside finished months it invents a collapse, and nothing on a tile
could tell the reader that the last point means something else.
line, and a straight line reads as a steady trend — but two is also exactly
what a metric has when it has only just started being measured.
period with no data must not be readable as a period with none of the thing.
The line is scaled to its own range and says only which way and how steadily —
the figures beside it carry the magnitude. It has a rule beneath it so it reads
as a span rather than floating, and the span is named once above the row, since
a tile has no room for an axis. Its request sits outside every loading and
error gate: the tiles render from the current period and the line appears when
it arrives, so the first screen never waits on it.
One thing removed
showSectionsgated the per-group cards and the drilldown sheets. The cardsmoved to the navigation long ago, so what survived was a flag whose only
remaining effect was to make a click land nowhere on any screen that set it.
Removed; the sheets now follow the only question that matters — whether the
caller routes clicks elsewhere.
Not in this PR
Which metrics the headline row shows is unchanged: a fixed priority order,
filtered to what the person is observed for. For a role the order was not
written around, that shows what happens to be instrumented rather than what the
work is — which needs role expectations, not a layout change.
Verification
943 tests pass across both projects,
tsc -band eslint clean. Checked againsta running stand at several widths and periods.
Summary by CodeRabbit