fix(frontend): no subject may crowd the attention list out - #2383
Merged
Conversation
added 2 commits
August 10, 2026 11:06
Some metrics contain others — lines added contains code lines, files shared contains its internal and external splits — so a subject whose output drops trips every metric of a containing set at once. Reported separately, one fact became several flags: the subject appeared several times over, and any count of "how many things are wrong" was really a count of how many ways we measured one thing. Thinned per subject, not globally: two people tripping the same related pair are two findings, not one. Of a contained pair the narrower metric survives, which is the containment helper's existing rule and the right one — "lines added to code files" excludes documentation, tests and configuration, so it says something the wider metric cannot. The summary line counted flags per metric while opening in people, so one sentence held two units: "3 of 16 people need a look — most flags on Commits (5)" left the reader to work out that five flags can belong to two people. It now counts people throughout. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
A row is one finding and the list is ranked by severity, so a subject who trips five metrics takes five rows — and the subject in the most trouble takes the most of the visible slice. Everyone else waits behind "+N more", which is precisely backwards: the list hid people better the worse things were. Capped at two rows per subject. That keeps the row a single readable claim rather than regrouping the list into people, and nothing is lost — every row opens that subject's own page, where all of their findings are. The "+N more" count follows the capped list, so the toggle promises what expanding reveals. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
📝 WalkthroughWalkthroughThe change removes redundant metric flags per person, updates attention summaries to count people, and caps portal attention rows at two findings per subject. Tests cover metric deduplication, distinct-person counting, subject retention, and capped “+N more” counts. ChangesAttention findings
Estimated code review effort: 3 (Moderate) | ~20 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: 1
🤖 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/lib/insight/attention-flags.ts`:
- Around line 262-271: Update the grouping logic around byMetric so metric
identity uses f.metricKey rather than f.metricLabel, while retaining
f.metricLabel as the display text for themes. Adjust the stored map value and
top mapping so `${label} (${people(...)})` still uses the correct label and
person count per metric key, and add a test covering two distinct metric keys
with the same label.
🪄 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: c78df8ef-08b4-4997-8f57-8c3f29c1174f
📒 Files selected for processing (4)
src/frontend/src/components/portal/attention-list.test.tsxsrc/frontend/src/components/portal/attention-list.tsxsrc/frontend/src/lib/insight/attention-flags.test.tssrc/frontend/src/lib/insight/attention-flags.ts
Comment on lines
+262
to
+271
| const byMetric = new Map<string, Set<string>>(); | ||
| for (const f of flags) { | ||
| const seen = byMetric.get(f.metricLabel) ?? new Set<string>(); | ||
| seen.add(f.personId); | ||
| byMetric.set(f.metricLabel, seen); | ||
| } | ||
| const top = [...byMetric.entries()] | ||
| .sort((a, b) => b[1].size - a[1].size) | ||
| .slice(0, 2); | ||
| const themes = top.map(([label, who]) => `${label} (${people(who.size)})`).join(", "); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Group summary themes by metricKey.
metricLabel is display text, not metric identity. Two API metrics can share a label or short_label. The current map then merges distinct metrics and reports an incorrect person count for one theme.
Key the map by f.metricKey. Store f.metricLabel as the display value. Add a test with two metric keys that share one label.
Proposed fix
- const byMetric = new Map<string, Set<string>>();
+ const byMetric = new Map<
+ string,
+ { label: string; personIds: Set<string> }
+ >();
for (const f of flags) {
- const seen = byMetric.get(f.metricLabel) ?? new Set<string>();
- seen.add(f.personId);
- byMetric.set(f.metricLabel, seen);
+ const metric = byMetric.get(f.metricKey) ?? {
+ label: f.metricLabel,
+ personIds: new Set<string>(),
+ };
+ metric.personIds.add(f.personId);
+ byMetric.set(f.metricKey, metric);
}
const top = [...byMetric.entries()]
- .sort((a, b) => b[1].size - a[1].size)
+ .sort((a, b) => b[1].personIds.size - a[1].personIds.size)
.slice(0, 2);
- const themes = top.map(([label, who]) => `${label} (${people(who.size)})`).join(", ");
+ const themes = top
+ .map(([, metric]) => `${metric.label} (${people(metric.personIds.size)})`)
+ .join(", ");📝 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 byMetric = new Map<string, Set<string>>(); | |
| for (const f of flags) { | |
| const seen = byMetric.get(f.metricLabel) ?? new Set<string>(); | |
| seen.add(f.personId); | |
| byMetric.set(f.metricLabel, seen); | |
| } | |
| const top = [...byMetric.entries()] | |
| .sort((a, b) => b[1].size - a[1].size) | |
| .slice(0, 2); | |
| const themes = top.map(([label, who]) => `${label} (${people(who.size)})`).join(", "); | |
| const byMetric = new Map< | |
| string, | |
| { label: string; personIds: Set<string> } | |
| >(); | |
| for (const f of flags) { | |
| const metric = byMetric.get(f.metricKey) ?? { | |
| label: f.metricLabel, | |
| personIds: new Set<string>(), | |
| }; | |
| metric.personIds.add(f.personId); | |
| byMetric.set(f.metricKey, metric); | |
| } | |
| const top = [...byMetric.entries()] | |
| .sort((a, b) => b[1].personIds.size - a[1].personIds.size) | |
| .slice(0, 2); | |
| const themes = top | |
| .map(([, metric]) => `${metric.label} (${people(metric.personIds.size)})`) | |
| .join(", "); |
🤖 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/insight/attention-flags.ts` around lines 262 - 271,
Update the grouping logic around byMetric so metric identity uses f.metricKey
rather than f.metricLabel, while retaining f.metricLabel as the display text for
themes. Adjust the stored map value and top mapping so `${label}
(${people(...)})` still uses the correct label and person count per metric key,
and add a test covering two distinct metric keys with the same label.
hello1101n
approved these changes
Aug 10, 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.
Closes #2378 — as much of it as should be fixed before the engine lands.
What the report found, and what it did not
The bug reports that the panel's header counts people while its rows count
findings, and asks for rows grouped into people. The header inconsistency is
real. The grouping is the part I have not done, and deliberately.
The strongest observation in the report is its last one: a subject who trips
several metrics occupies several rows, and because the list is ranked by
severity, the subject in the most trouble occupies the most of the visible
slice — pushing others behind "+N more". The list hid people better the worse
things were. That is the part that costs a reader something, and it is fixed
here.
Why not group into people
#1610 owns the panel's shape and describes a row as subject + metric, with a
worked example in that form. Regrouping the list would fix this bug by
contradicting the epic that replaces this code. So the row stays one finding,
and the crowding it risks is bounded by a cap instead: two rows per subject.
Nothing is lost to the cap. Every row opens that subject's own page, where all
of their findings are.
That decision is now written into #1610 rather than left implicit, along with
the alternative and what each costs.
The duplication underneath
Some metrics contain others. Lines added contains code lines; files shared
contains its internal and external splits. A subject whose output drops trips
every metric of a containing set at once, so one fact arrived as several flags —
which both multiplied their rows and inflated any count of what is wrong.
Flags are now thinned per subject before ranking, reusing the containment map
the person page already uses. Per subject, not globally: two people tripping the
same related pair are two findings, not one. Of a contained pair the narrower
metric survives — the helper's existing rule, and the right one, since "lines
added to code files" excludes documentation, tests and configuration and so says
something the wider metric cannot.
The header
attentionSummarycounted flags per metric while opening in people, so onesentence carried two units — "3 of 16 people need a look — most flags on Commits
(5)" leaves a reader to work out that five flags can belong to two people. It
now counts people throughout.
Scope
This is interim by design: #1610 and #1611 both retire
src/frontend/src/lib/insight/attention-flags.ts. Flag lifecycle, "not enoughevidence" and served thresholds are specified there and are not reimplemented
here.
Verification
tsc -bclean andvitest --project unitgreen: 134 files, 939 tests. Five newcases cover the cap, the "+N more" count following it, containment thinning,
thinning staying per subject, and unrelated metrics being left alone.
Not re-checked in a browser. The panel renders only in the Overview and Team
zones, and the account available here has no reports, so those zones are not
offered to it — the change was exercised through the component's own tests
instead. The row markup is untouched; what changed is which rows survive into
the list.