fix(team-view): member popup links to the IC page; Expand details opens the full-metric sheet - #198
Merged
Merged
Conversation
…c set The heatmap rendered only the 7 curated columns, so the on-page table still hid most of a member's metrics after the details-sheet fix (issue #1729). Columns are now derived at render time: the curated core first, then a column for every legacy bullet and unified-path (git/ai) entry present in the roster's data, deduped by metric key and label. Bullet columns color against the member's department cohort as before; unified columns reuse the entry's own-cohort status and format their values through formatMetricValue. Word units gain a separating space. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <roman.mitasov@constructor.tech>
📝 WalkthroughWalkthroughChangesMembers heatmap metric expansion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RosterData
participant MembersHeatmap
participant MemberDetailsSheet
RosterData->>MembersHeatmap: provide roster bullets and unified metric entries
MembersHeatmap->>MembersHeatmap: build deduplicated dynamic columns
MembersHeatmap->>MemberDetailsSheet: provide full formatted metric set
MemberDetailsSheet-->>MembersHeatmap: render member metric details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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
🧹 Nitpick comments (2)
src/components/widgets/v2/members-heatmap/index.test.tsx (1)
271-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the core dedup scenario; consider extending to sheet/tooltip formatting.
This test solidly verifies column creation and the two documented dedup cases (exact-key bullet collision, dot-suffix unified collision). It doesn't yet cover the newly added
MemberDetailsSheetrow formatting (display/medianDisplayviaformatMetricValue) or theunitSuffixword-unit spacing path (e.g. "meetings", "files") described in the source comments — both are new, non-trivial formatting branches introduced in this PR.🤖 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/components/widgets/v2/members-heatmap/index.test.tsx` around lines 271 - 347, Extend the heatmap test around MembersHeatmap to verify MemberDetailsSheet row formatting through formatMetricValue, including display and medianDisplay values and word-based unitSuffix spacing such as “meetings” or “files.” Assert the rendered sheet/tooltip text for these branches while preserving the existing deduplication and column assertions.src/components/widgets/v2/members-heatmap/index.tsx (1)
518-580: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM overall — sheet rows correctly thread
formatthroughformatMetricValueand dedupe against the dynamic column set.Minor nit: the local
const unitSuffix = b.unit ? \${b.unit}` : ""at line 549 shadows the module-levelunitSuffix()helper (lines 200-209) with different spacing semantics (always spaces vs. compact single-char units). Since thisfromBulletsbranch appears effectively unreachable now (every bullet'smetric_key` should already be covered by a dynamic column), it's low priority, but renaming would avoid confusing a future reader who expects the shared helper's behavior here too.🤖 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/components/widgets/v2/members-heatmap/index.tsx` around lines 518 - 580, Rename the local unit-suffix string inside the fromBullets mapping in sheetRows so it no longer shadows the module-level unitSuffix() helper. Update its use in the bullet display and medianDisplay fields while preserving the existing spacing behavior.
🤖 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/components/widgets/v2/members-heatmap/index.tsx`:
- Around line 320-371: Limit label-based deduplication in the columns useMemo to
labels from CORE_COLUMNS only. Keep metric-key deduplication for dynamic bullets
and metrics, but stop adding dynamic labels to seenLabels so unrelated later
metrics with the same display label are retained in the grid and details sheet.
---
Nitpick comments:
In `@src/components/widgets/v2/members-heatmap/index.test.tsx`:
- Around line 271-347: Extend the heatmap test around MembersHeatmap to verify
MemberDetailsSheet row formatting through formatMetricValue, including display
and medianDisplay values and word-based unitSuffix spacing such as “meetings” or
“files.” Assert the rendered sheet/tooltip text for these branches while
preserving the existing deduplication and column assertions.
In `@src/components/widgets/v2/members-heatmap/index.tsx`:
- Around line 518-580: Rename the local unit-suffix string inside the
fromBullets mapping in sheetRows so it no longer shadows the module-level
unitSuffix() helper. Update its use in the bullet display and medianDisplay
fields while preserving the existing spacing behavior.
🪄 Autofix (Beta)
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
Run ID: 5b58d8d4-ef79-4624-9f44-ac485efb197d
📒 Files selected for processing (2)
src/components/widgets/v2/members-heatmap/index.test.tsxsrc/components/widgets/v2/members-heatmap/index.tsx
Comment on lines
+320
to
+371
| const columns = useMemo(() => { | ||
| const cols: ColumnDef[] = [...CORE_COLUMNS]; | ||
| const seenKeys = new Set(cols.map(metricKeyForColumn)); | ||
| const seenLabels = new Set(cols.map((c) => c.label.toLowerCase())); | ||
| for (const m of members) { | ||
| const bullets = bulletsByPerson?.get(m.person_id.toLowerCase()) ?? []; | ||
| for (const b of bullets) { | ||
| if (seenKeys.has(b.metric_key)) continue; | ||
| seenKeys.add(b.metric_key); | ||
| seenLabels.add(b.label.toLowerCase()); | ||
| cols.push({ | ||
| key: b.metric_key, | ||
| label: b.label, | ||
| short: b.label, | ||
| unit: b.unit ?? "", | ||
| higher_is_better: | ||
| byMetricKey(bulletCatalogKey(b))?.higher_is_better ?? true, | ||
| mobile: false, | ||
| source: "bullet", | ||
| metricKey: b.metric_key, | ||
| }); | ||
| } | ||
| } | ||
| for (const m of members) { | ||
| const entries = | ||
| metricEntriesByPerson?.get(m.person_id.toLowerCase()) ?? []; | ||
| for (const e of entries) { | ||
| const suffix = e.key.split(".").pop() ?? e.key; | ||
| if ( | ||
| seenKeys.has(e.key) || | ||
| seenKeys.has(suffix) || | ||
| seenLabels.has(e.label.toLowerCase()) | ||
| ) { | ||
| continue; | ||
| } | ||
| seenKeys.add(e.key); | ||
| seenLabels.add(e.label.toLowerCase()); | ||
| cols.push({ | ||
| key: e.key, | ||
| label: e.label, | ||
| short: e.label, | ||
| unit: e.unit ?? "", | ||
| higher_is_better: e.higherIsBetter, | ||
| mobile: false, | ||
| source: "metric", | ||
| entryKey: e.key, | ||
| format: e.format, | ||
| }); | ||
| } | ||
| } | ||
| return cols; | ||
| }, [members, bulletsByPerson, metricEntriesByPerson, byMetricKey]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols first.
ast-grep outline src/components/widgets/v2/members-heatmap/index.tsx --view expanded || true
# Read the relevant section with line numbers.
sed -n '1,260p' src/components/widgets/v2/members-heatmap/index.tsx | cat -n
echo '---'
sed -n '260,430p' src/components/widgets/v2/members-heatmap/index.tsx | cat -n
echo '---'
# Search for related symbols and any other label/key dedupe logic.
rg -n "seenLabels|seenKeys|CORE_COLUMNS|metricKeyForColumn|bulletCatalogKey|higherIsBetter|metricEntriesByPerson|bulletsByPerson" src/components/widgets/v2/members-heatmap/index.tsxRepository: constructorfabric/insight-front
Length of output: 21274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the source types first.
rg -n "interface BulletMetric|type BulletMetric|interface PeerStoryEntry|type PeerStoryEntry" src
# Read the relevant type definitions if found.
files=$(rg -l "interface BulletMetric|type BulletMetric|interface PeerStoryEntry|type PeerStoryEntry" src | head -n 20)
for f in $files; do
echo "--- $f"
sed -n '1,260p' "$f" | cat -n
doneRepository: constructorfabric/insight-front
Length of output: 45676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for places where metric labels are defined or compared in a way that assumes global uniqueness.
rg -n --glob '!**/*.snap' --glob '!**/node_modules/**' \
"label\s*:" src/lib src/components src/types | head -n 200
echo '---'
rg -n --glob '!**/*.snap' --glob '!**/node_modules/**' \
"toLowerCase\(\)\).*label|label.*toLowerCase\(\)|seenLabels|duplicate label|same label" srcRepository: constructorfabric/insight-front
Length of output: 14886
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the later column consumers around the member-details sheet.
sed -n '480,590p' src/components/widgets/v2/members-heatmap/index.tsx | cat -n
echo '---'
# Inspect the relevant members heatmap tests for duplicate-label scenarios.
sed -n '1,460p' src/components/widgets/v2/members-heatmap/index.test.tsx | cat -nRepository: constructorfabric/insight-front
Length of output: 22093
Scope label dedupe to the curated columns
seenLabels grows with every bullet/metric added, so a later dynamic metric that shares a display label with an earlier unrelated one is dropped from the grid and then filtered out of the details sheet too. Anchor the label check to CORE_COLUMNS (or another fixed set) so only curated-vs-dynamic collisions are suppressed.
🤖 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/components/widgets/v2/members-heatmap/index.tsx` around lines 320 - 371,
Limit label-based deduplication in the columns useMemo to labels from
CORE_COLUMNS only. Keep metric-key deduplication for dynamic bullets and
metrics, but stop adding dynamic labels to seenLabels so unrelated later metrics
with the same display label are retained in the grid and details sheet.
ktursunov
approved these changes
Jul 14, 2026
mitasovr
pushed a commit
that referenced
this pull request
Jul 15, 2026
…from Expand details In the Members × metrics popup, "Open in IC view" opened the details sheet while "Expand details" toggled an inline bullet strip under the row. Rework per review of #198: "Open in IC view" is now a router link to the member's personal IC page, and "Expand details" opens the details sheet (which already shows the full metric set after #193). The inline ExpandedBullets strip and its expansion state go away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <roman.mitasov@constructor.tech>
mitasovr
changed the title
mitasovr
added a commit
that referenced
this pull request
Jul 15, 2026
…he IC page (#200) * Revert "fix(team-view): grow Members × metrics grid columns to the full metric set (#198)" This reverts commit 1a44492. Signed-off-by: Roman Mitasov <roman.mitasov@constructor.tech> * fix(team-view): link the member popup to the IC page, open the sheet from Expand details In the Members × metrics popup, "Open in IC view" opened the details sheet while "Expand details" toggled an inline bullet strip under the row. Rework per review of #198: "Open in IC view" is now a router link to the member's personal IC page, and "Expand details" opens the details sheet (which already shows the full metric set after #193). The inline ExpandedBullets strip and its expansion state go away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <roman.mitasov@constructor.tech> --------- Signed-off-by: Roman Mitasov <roman.mitasov@constructor.tech> Co-authored-by: Roman Mitasov <roman.mitasov@constructor.tech> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Problem
Follow-up to #193 / constructorfabric/insight#1729. In the Members × metrics member popup:
(The first iteration of this PR grew the grid to the full column set; reverted per review — the table keeps its curated 7 columns, the full metric set lives in the details sheet from #193.)
Change
<Link>to the member's personal IC page (/ic/$person/personal), styled via the Button render prop.ExpandedBulletsstrip, its expansion state, and the now-unusedMemberRowplumbing are removed.Verification
Verified in the browser against the mock stack: the grid renders its usual 7 columns; the popup's "Open in IC view" navigates SPA-style to the member's personal dashboard; "Expand details" opens the sheet with the member's full metric set (60 rows on the mock roster).
Typecheck, ESLint, and the full vitest suite pass; the component test now asserts the link href and opens the sheet through "Expand details".
Related to constructorfabric/insight#1729
🤖 Generated with Claude Code