feat(portal): organizational analytics portal behind the insight.portal flag - #2241
Conversation
Line charts were configured per call site, so the same reading rendered differently depending on which screen you opened: monotone smoothing that invents values between points, dots that either vanish on dense series or crowd them, and raw axis ticks that clip at five digits. - ChartLine wraps Recharts' Line with linear interpolation (a chart must not draw a value nobody measured), AdaptiveDot (dots above DOT_DENSITY_LIMIT = 31 points are dropped, but an isolated reading between two nulls is always marked so a single point is never invisible), and connectNulls off by default. - ChartYAxis defaults tickFormatter to formatAxisTick, which abbreviates from 10k — the point where four digits still fit an axis gutter and five do not. Below that it stays exact, because a tick sits on a gridline and "2.3k" for 2250 is a label that lies. - metric-timeseries-chart drops its local IsolatedPoint in favour of the shared behaviour. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
The rules every portal section obeys, in libraries rather than repeated per screen — an org number computed differently on two screens is a number nobody can act on. - metric-stats: per-active-person values (an org total divided by headcount hides who is idle; the denominator is people with a non-zero reading), period-over-period deltas, medians for ratios, integer 1/2/5 histogram bins that self-suppress when degenerate, top-decile concentration, and group coverage. - trend-data: pickTrendBucket coarsens day to week to month so a long period still renders; when no bucket fits the row budget it returns null and the section says so instead of firing a request the API rejects. - attention-flags: outliers, declines and collapses ranked on one scale-free severity (distance in cohort IQRs, falling back to the median when the IQR is degenerate), deduplicated to the strongest flag per person and metric. - slices: attribute-agnostic cohorts discovered from the roster, gated by presence and cardinality so an id-like attribute never becomes a slice. - within-team-peer, event-histogram, lens-configs, overview-configs: the per-zone section registries and the peer/event shapes they read. - collection / metric-results: honest emptiness — the queries stay disabled until a roster resolves and the client refuses an empty entity list, rather than sending a request that returns a 400 the reader cannot act on. `entityObserved` distinguishes "never ingested" from "measured zero" via the peer view's nulls. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Navigation state lived in memory, so a reload reset the screen, Back went somewhere unrelated, and a link or bookmark could not reproduce a view. Everything that answers "what am I looking at" now rides in the URL: /portal?zone=&item=&dir=&lens=&scope=&direct=&slice=&period=&from=&to=, plus /ic/$person/personal|team for the person-scoped screens. Only true reader preferences (is the portal on, are planned sections shown) stay in localStorage. - portal-search validates rather than trusts: an unrecognised param is dropped so a mistyped link degrades to the computed default, and an inverted or over-long custom range falls back to the period preset instead of throwing into the error boundary. - portal-nav / use-zone-nav read the URL and navigate once per click, with `replace` for effect-driven corrections (pinning the landing zone, syncing scope from the route) so Back never steps into a half-built address. The path wins over a stale `?zone=` — the URL cannot contradict itself. - Person keys are canonical person ids throughout: the route segment (which `isPersonId` guards, redirecting a pre-cutover email URL rather than sending it to the API), `?scope=`, the roster maps and the metric entity ids are all the same key. - use-org-scope resolves the scope inside the VIEWER's own subtree, so a hand-edited `?scope=` cannot reach outside what the viewer may see. - src/test/portal-router is a reactive router fake, and src/test/identity mints valid person UUIDs from short labels so fixtures stay readable without drifting from the real key shape. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
…t tiers The portal shell: a zone rail on the left, the global controls (scope, slice, period) in a sticky top bar, and the zone's own content in between. - useShellLayout resolves phone (<768) / narrow (768-1023) / wide (>=1024) from one place, so the rail is a drawer on a phone, collapsible on narrow, and pinned on wide, and pane state resets when the tier changes rather than restoring a mode that no longer exists. - The top bar is sticky and horizontally scrollable on a phone, so scope, slice and period stay reachable from anywhere in a long dashboard; `justify-end` is desktop-only because inside a scroller it pushes the first controls past the unreachable start edge. - On a phone the drawer shows a one-row zone switcher and folds settings behind a single row — the expanded list pushed the sections below the fold, which is exactly the content the reader opened the drawer to reach. - OrgScopeGate is the single honest gate in front of every org zone: the viewer's roster is still loading, the scope is empty, or identity failed, each said plainly with a retry rather than an empty chart. - Scope and slice selects read and write the URL; the org tree drills into a lead's team and sets the scope in the same click. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Every zone renders through one section renderer instead of a bespoke screen, so a metric reads the same way wherever it appears and a new zone is a registry entry rather than a new component tree. - DomainLensView renders the section kinds the grammar defines: headline (per-active-person value, team total and PoP delta), stat tiles (medians, never sums), distribution, participation (N of M active), concentration with the framing the domain deserves (bus-factor risk for git, load balance for collaboration), composition from real server dimensions only, by-unit cohorts when a slice is active, attention rows, direction cards and the coverage radar. - Overview, Directions, People, Person, AI cost and Manage are configs over that renderer; overview-view is a router, not a screen. - Honest emptiness throughout: a family nothing ever ingested says so, a section too small to compare explains why instead of going silent, a suppressed radar states its minimum cohort, and no surface paints a zero where there is no measurement. - The attention list links each row to that person by id, and the person header steps up to the manager via parent_person_id. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
Warning Review limit reached
Next review available in: 37 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 (18)
📝 WalkthroughWalkthroughThe change adds a portal application with URL-backed navigation, organization-scoped metric views, responsive layout components, configurable analytics sections, metric utilities, request guards, chart defaults, and test coverage. ChangesPortal analytics platform
Estimated code review effort: 5 (Critical) | ~120 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: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/ui/chart.tsx (1)
122-216: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDo not hand-edit the vendored chart component.
Move the
YAxis,AdaptiveDot,ChartLine, andChartAreabehavior into an application-owned wrapper outsidesrc/components/ui/. Alternatively, regenerate this component through the approved shadcn/ui workflow.As per coding guidelines, treat
src/components/ui/as vendored shadcn/ui code and regenerate components instead of hand-editing them.🤖 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/ui/chart.tsx` around lines 122 - 216, Move the custom YAxis, AdaptiveDot, ChartLine, and ChartArea behavior out of the vendored src/components/ui chart component into an application-owned wrapper, preserving their current defaults and behavior. Alternatively, remove the hand edits and regenerate the chart component through the approved shadcn/ui workflow.Source: Coding guidelines
🟡 Minor comments (12)
src/frontend/src/components/widgets/period-selector-bar.tsx-126-130 (1)
126-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an accessible name for the compact date-range trigger.
Below
sm, the active range text is hidden. The remainingUTCtext does not identify the date-range control or its current value. Add anaria-labelthat includesactiveRangeLabel.Proposed fix
- <ToggleGroupItem value="custom" className="gap-1.5"> + <ToggleGroupItem + value="custom" + className="gap-1.5" + aria-label={`Custom date range: ${activeRangeLabel}`} + >🤖 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/period-selector-bar.tsx` around lines 126 - 130, Update the ToggleGroupItem for the custom date range to include an aria-label containing activeRangeLabel, so the compact trigger remains identifiable when its visible range text is hidden below the sm breakpoint.src/frontend/src/lib/format.ts-99-103 (1)
99-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle the rounded-thousand rollover.
formatAxisTick(999_999)returns1000kbecause the million check runs before thousand rounding. Return the million form when rounding reaches 1,000 thousand.Proposed fix
export function formatAxisTick(v: number): string { const abs = Math.abs(v); if (abs >= 1_000_000) return `${trimTrailingZero((v / 1_000_000).toFixed(1))}M`; - if (abs >= 10_000) return `${Math.round(v / 1000)}k`; + if (abs >= 10_000) { + const thousands = Math.round(v / 1000); + if (Math.abs(thousands) >= 1_000) { + return `${trimTrailingZero((v / 1_000_000).toFixed(1))}M`; + } + return `${thousands}k`; + } return trimTrailingZero((Math.round(v * 10) / 10).toFixed(1)); }🤖 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/format.ts` around lines 99 - 103, Update formatAxisTick so the thousand-format branch detects when rounding v to thousands reaches 1,000 in magnitude and formats that result as the million representation instead of returning 1000k. Preserve existing formatting for values below the rollover and for values already handled by the million branch.src/frontend/src/components/portal/shell-layout.test.tsx-31-43 (1)
31-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one mock factory for
useViewerIsManager.The later
vi.mock("@/lib/portal/use-viewer-is-manager", ...)replaces the state-controlled factory, somocks.isManagerchanges cannot affect this mock. Remove the duplicate mock block and keep the shared factory above.🤖 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/shell-layout.test.tsx` around lines 31 - 43, Remove the duplicate vi.mock block for useViewerIsManager in the shell layout test, preserving the earlier shared factory that reads mocks.isManager. Keep the other component mocks unchanged so tests can continue controlling manager state through mocks.isManager.src/frontend/src/lib/insight/attention-flags.ts-211-212 (1)
211-212: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the plural form when the scope holds one person.
With
teamSize === 1the text reads "All 1 people are within their usual range this period." A single-person scope is reachable, because the caller only suppresses the section whenmemberIds.lengthis 0.🐛 Proposed fix
if (flags.length === 0) - return `All ${teamSize} people are within their usual range this period.`; + return teamSize === 1 + ? "This person is within their usual range this period." + : `All ${teamSize} people are within their usual range this period.`;🤖 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 211 - 212, Update the zero-flags message in the attention-flags logic to use singular wording when teamSize is 1 and plural wording otherwise, preserving the existing message and behavior for multi-person scopes.src/frontend/src/lib/portal/metric-stats.ts-91-93 (1)
91-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp the bin index at zero.
distributiononly rejects the case where the maximum is not positive. A set that mixes negative and positive values still passes. For a negativev,Math.floor(v / step)is negative andMath.min(nBins - 1, …)keeps it negative. The write then lands on an out-of-range array property, so the value is dropped from every bin and the rendered counts no longer sum to the population.🐛 Proposed fix
for (const v of values) { - counts[Math.min(nBins - 1, Math.floor(v / step))] += 1; + const bin = Math.min(nBins - 1, Math.max(0, Math.floor(v / step))); + counts[bin] += 1; }🤖 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/metric-stats.ts` around lines 91 - 93, Clamp the computed bin index to the valid range in the values loop of distribution, ensuring negative values map to bin 0 while values above the upper bound still map to nBins - 1. Preserve the existing count increment so every value contributes to exactly one bin.src/frontend/src/lib/portal/use-viewer-is-manager.ts-21-28 (1)
21-28: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep manager status unresolved when identity data is absent.
If
q.datais null whileq.isPendingis false, this hook returns a confirmed non-manager.PortalLayoutthen clears a manager’s selected organization zone and hides organization navigation. Return an unresolved or error state until the identity query can confirm the viewer node. Add a regression test for a null, non-pending result.🤖 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-viewer-is-manager.ts` around lines 21 - 28, Update the isManager calculation in useViewerIsManager so a null q.data result remains unresolved or reports an error when q.isPending is false, rather than returning false; preserve confirmed false only after identity data is available and the viewer node is evaluated. Ensure PortalLayout does not clear manager-specific state for missing identity data, and add a regression test covering null, non-pending query data.src/frontend/src/lib/portal/use-cohort-label.ts-30-31 (1)
30-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
toLowerCase()mangles acronyms and proper nouns in the slice label.
dimssupplies the label. Lower-casing it turns "EMEA" into "emea" and "AI Platform" into "ai platform". The consumer renders the result mid-sentence as "vs median" (src/frontend/src/components/portal/single-group-view.tsxline 82 passescohortLabelintoCollectionDrilldown), so the damage is visible in exactly the string this hook exists to get right.Return the supplied label unchanged. If a specific surface needs lower case, apply the transform with CSS at that surface.
♻️ Proposed fix
if (!slice) return "team"; - return (dims.find((d) => d.key === slice)?.label ?? "cohort").toLowerCase(); + return dims.find((d) => d.key === slice)?.label ?? "cohort";As per coding guidelines: "Use metric names, descriptions, and units supplied by API responses; do not invent user-facing metric semantics in the frontend."
🤖 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-cohort-label.ts` around lines 30 - 31, Update the slice-label return in useCohortLabel to preserve the label supplied by dims unchanged; remove the toLowerCase transformation while retaining the existing "cohort" fallback and "team" behavior when no slice is selected.Source: Coding guidelines
src/frontend/src/components/portal/single-group-view.tsx-43-54 (1)
43-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle a failed cohort fetch explicitly.
The view gates on
data.isPendinganddata.isError. It never readscohortData.isErrororcohortData.isPending. If the cohort query fails,cohortData.byKeyis empty,injectCohortPeerinjects no peer stats, andCollectionDrilldownstill receivescohortLabel. The drilldown then presents a comparison against a cohort whose data never arrived.The comment at lines 64-65 states the policy for this file: a failed fetch must surface as a retryable error rather than a view rendered over an empty dataset. Apply the same decision to the cohort query, or suppress
cohortLabelwhen the cohort data is unavailable so the view makes no comparison claim it cannot support.🐛 Proposed change — suppress the comparison label instead of failing the view
const cohortLabel = useCohortLabel();+ const cohortReady = cohortIds.length > 0 && !cohortData.isError && !cohortData.isPending; return ( <div className="flex flex-col gap-4 p-4 md:p-6"> <h1 className="text-xl font-semibold tracking-tight">{def.title}</h1> <CollectionDrilldown def={def} data={injectedData} entityId={entityId} range={dateRange} - cohortLabel={cohortLabel} + cohortLabel={cohortReady ? cohortLabel : undefined} />Confirm that
CollectionDrilldownaccepts an absentcohortLabeland hides the peer story.Also applies to: 63-72
🤖 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/single-group-view.tsx` around lines 43 - 54, Update the cohort handling in the component’s data flow to account for cohortData.isError and cohortData.isPending. When cohort data is unavailable, suppress the cohortLabel passed to CollectionDrilldown so the view does not claim a comparison; otherwise preserve the existing label and injected peer statistics. Confirm the CollectionDrilldown prop contract supports an absent cohortLabel.src/frontend/src/components/portal/people-view.tsx-39-44 (1)
39-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe person comparison is case-sensitive, unlike
personIdEq.
personarrives from the/ic/$personroute segment.src/frontend/src/components/org-tree.tsxlines 19-21 compare person ids withpersonIdEq, which lowercases both sides. This effect uses!==and then stores the raw segment withreplaceScope({ root: person }).If a pasted URL differs only in case from the id in the identity tree, two results follow. The effect syncs again for what is the same person. The stored
scoperoot no longer matchespivotPersonIdunder the===check insrc/frontend/src/components/portal/scope-select.tsxline 58, so the active-scope check mark does not render.Normalize the route segment before you compare it and before you store it, or confirm that person ids are case-stable end to end.
#!/bin/bash # Description: Check whether person ids are case-normalized across routes, scope, and identity. set -euo pipefail # Every case-insensitive person-id comparison already in the frontend. rg -n -C3 --type=tsx --type=ts 'toLowerCase\(\)' src/frontend/src/lib/portal src/frontend/src/components src/frontend/src/lib/metrics # The canonical normalizer and whether it lowercases. fd -t f 'entity.ts' -x ast-grep outline {} --items all rg -n -C6 'export function normalizePersonId' src/frontend/src # How the scope root is resolved against the identity tree. rg -n -C6 'resolveScopeRoster' src/frontend/src/lib/portal/use-org-scope.ts🤖 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/people-view.tsx` around lines 39 - 44, Normalize the route segment in the effect around lastRouteSync before comparing or storing it, using the existing person-id normalization or case-insensitive equality convention such as personIdEq. Pass the normalized value to replaceScope({ root: ... }) so scope.root remains consistent with pivotPersonId and equivalent casing does not trigger repeated synchronization.src/frontend/src/components/org-tree.tsx-110-115 (1)
110-115: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
decodeURIComponentcan throw during render.
decodeURIComponentraises aURIErroron a malformed percent sequence, for example the pathname/ic/%E0%A4%A. The call runs insideuseMemoduring render, so the error propagates to the nearest error boundary and unmounts the tree. A pasted URL reaches this path, and this component parses the raw pathname itself rather than reading a validated route parameter.Return
nullfor an undecodable segment. The tree then highlights no node instead of failing.🐛 Proposed change
const activePersonId = useMemo(() => { const m = /^\/ic\/([^/]+)/.exec(pathname); - if (m) return decodeURIComponent(m[1]!); + if (m) { + try { + return decodeURIComponent(m[1]!); + } catch { + // A malformed percent sequence in a pasted URL matches no person. + return null; + } + } if (pathname === "/" && viewerPersonId) return viewerPersonId; return null; }, [pathname, viewerPersonId]);🤖 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/org-tree.tsx` around lines 110 - 115, Update the activePersonId useMemo to safely handle URIError from decodeURIComponent for the matched /ic/ path segment, returning null when decoding fails while preserving the existing decoded-ID and viewerPersonId behavior for valid paths.src/frontend/src/components/portal/team-state-view.tsx-259-259 (1)
259-259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply the same
teamNamefallback to the table caption.Line 212 falls back to
"Team"whenteamNameis empty. Line 259 interpolatesteamNamewith no fallback, so the caption becomes" — members × metrics"with a leading separator. The caption is the accessible name for the members table, so screen-reader output starts with punctuation.🐛 Proposed change
- caption={`${teamName} — members × metrics`} + caption={`${teamName || "Team"} — members × metrics`}🤖 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/team-state-view.tsx` at line 259, Update the table caption in team-state-view so it uses the same teamName fallback already applied elsewhere in this component, instead of interpolating teamName directly. Adjust the caption expression near the members table to reuse the existing fallback logic (the one that returns “Team” when teamName is empty) so the accessible name does not শুরু with a leading separator when teamName is missing.src/frontend/src/components/portal/ai-cost-view.tsx-338-342 (1)
338-342: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the API-supplied format and unit for every currency and count value.
Line 329 and Line 335 read
costR?.format/costR?.unitandlinesR?.format/linesR?.unit. Line 340, Line 379, and Line 386 hardcode"currency"/"USD"and"integer"/nullinstead.costRandlinesRare already in scope at Line 296 and Line 297. If the API reports a different unit, the tiles disagree with each other.♻️ Proposed change for the average tile
<Tile label="Avg cost / active user" - value={formatMetricValue(avgCost, "currency", "USD")} + value={formatMetricValue(avgCost, costR?.format ?? "currency", costR?.unit ?? "USD")} sub="Claude Code" />Apply the same change to the per-tool card values at Line 379 and Line 386.
As per coding guidelines: "Use metric names, descriptions, and units supplied by API responses; do not invent user-facing metric semantics in the frontend."
🤖 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/ai-cost-view.tsx` around lines 338 - 342, Update the average-cost Tile and the per-tool cost/count values to use the API-supplied format and unit from costR and linesR, matching the existing handling in the surrounding metric tiles. Remove the hardcoded currency/USD and integer/null arguments while preserving each tile’s current metric source and labels.Source: Coding guidelines
🧹 Nitpick comments (21)
src/frontend/src/components/portal/employees-view.test.tsx (1)
41-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUncleared
vi.hoistedspies make the retry assertions order-dependent. Both files create arefetchspy once insidevi.hoisted, reset only the plain data fields inbeforeEach, and then asserttoHaveBeenCalledOnceon that spy. The call count accumulates across every test in the file, so the assertion holds only while exactly one test triggers retry. Clear the spy inbeforeEachin each file.
src/frontend/src/components/portal/employees-view.test.tsx#L41-L52: addmocks.ic.refetch.mockClear()to thebeforeEachblock.src/frontend/src/components/portal/manage-view.test.tsx#L49-L51: addmocks.q.refetch.mockClear()to thebeforeEachblock.🤖 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/employees-view.test.tsx` around lines 41 - 52, Clear the hoisted retry spies in each test setup before assertions run: add mocks.ic.refetch.mockClear() to the beforeEach block in src/frontend/src/components/portal/employees-view.test.tsx (lines 41-52) and mocks.q.refetch.mockClear() to the beforeEach block in src/frontend/src/components/portal/manage-view.test.tsx (lines 49-51), preserving the existing test data initialization.src/frontend/src/components/portal/attention-list.test.tsx (2)
21-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared
pidfixture helper.
src/frontend/src/test/identity.tsexportspid(label), which mints a deterministic person UUID. Other portal tests in this layer use it. This file defines a second local minting helperID(n)for the same purpose. Usepidhere so all portal tests produce identity keys through one helper.Note that
ID(n)also breaks its own UUID shape forn >= 10, because the first group grows past 8 characters.♻️ Proposed refactor
-import type { AttentionFlag } from "`@/lib/insight/attention-flags`"; +import type { AttentionFlag } from "`@/lib/insight/attention-flags`"; +import { pid } from "`@/test/identity`";-// Person ids, not emails: rows link by the identity-cutover key. -const ID = (n: number) => `0000000${n}-1111-4111-8111-111111111111`; +// Person ids, not emails: rows link by the identity-cutover key. +const ID = (n: number) => pid(`p${n}`);🤖 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/attention-list.test.tsx` around lines 21 - 40, Replace the local identity-minting helper in attention-list.test.tsx with the shared pid(label) fixture from src/frontend/src/test/identity.ts, and update the FLAGS/flag setup to use pid for personId values instead of ID(n). Keep the existing AttentionFlag shape and test data behavior unchanged, but remove the custom ID helper so all portal tests mint deterministic person UUIDs through the same shared symbol.
64-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the portal router state between tests.
This test writes
zone: "overview"into the sharedportalRouterstore. The file has nobeforeEachreset, unlikeai-cost-view.test.tsxandteam-state-view.test.tsx. The state then persists into later tests in this file. Add abeforeEachthat resets the zone so test order does not affect results.♻️ Proposed change
-import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest";describe("AttentionList", () => { + beforeEach(() => { + act(() => portalRouter.set({ zone: undefined })); + }); + it("renders the summary, people label and flag rows with reasons", () => {🤖 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/attention-list.test.tsx` around lines 64 - 70, Add a beforeEach setup in the attention-list test suite that resets the shared portalRouter zone before every test, ensuring the zone written by the test around renderZone and AttentionList cannot persist across tests or affect their results.src/frontend/src/components/portal/portal-shell.test.tsx (2)
215-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe second
rendermounts a new instance; it does not remount the first.The comment says "a remount for the SAME person must NOT revert it". The test calls
rendertwice without unmounting, so twoPeopleViewinstances are mounted at the same time. The assertion still proves the one-shot sync guard is shared rather than per-instance. It does not prove the unmount-then-mount case the comment describes. Unmount the first tree before the second render, or update the comment to match what the test asserts.♻️ Proposed change
it("syncs the route person into the org scope ONCE, then defers to the user", () => { const scope = renderHook(() => usePortalScope()); - render(<PeopleView person="p2@x" item={null} />); + const first = render(<PeopleView person="p2@x" item={null} />); expect(scope.result.current.root).toBe("p2@x"); // The user re-picks a scope from the topbar… act(() => portalRouter.set({ scope: "other@x" })); // …and a remount for the SAME person must NOT revert it. + first.unmount(); render(<PeopleView person="p2@x" item={null} />); expect(scope.result.current.root).toBe("other@x"); });🤖 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/portal-shell.test.tsx` around lines 215 - 224, Update the test around usePortalScope and PeopleView so it actually covers the stated remount behavior: retain the first render’s result, unmount that tree after the user changes the scope, then render the same person again and assert the user-selected scope remains. Alternatively, revise the test description to describe concurrent instances if that is the intended coverage.
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse canonical person IDs in the person fixtures.
These fixtures pass emails as person keys:
activePerson: "boss@x",person="a@x", andperson="p1@x"/"p2@x".src/frontend/src/test/identity.tsdocuments that person keys are canonical UUIDs since the identity cutover, and that the route guard redirects any other shape. Every other portal test in this layer mints keys withpid(...).The assertions still pass because the stubbed leaf views only echo the prop. The fixtures therefore do not exercise the real key shape, and they would not catch a person-ID validation added to
PersonVieworPeopleView. Replace the email literals withpid(...)values.Also applies to: 190-224
🤖 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/portal-shell.test.tsx` around lines 19 - 24, Update the person fixtures in the portal shell tests to use canonical UUID keys generated through the existing pid(...) helper. Replace mocks.zone.activePerson and the person values in the affected test cases, including the p1/p2 fixtures, while keeping email fields as email addresses. Ensure these fixtures exercise the same person-ID shape expected by PersonView, PeopleView, and the route guard.src/frontend/src/components/portal/domain-lens-view.test.tsx (1)
60-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the
useMetricCollectioncall counter between tests.The mock keeps a module-scoped
callcounter that never resets. The slot each caller receives depends on the total number ofuseMetricCollectioncalls made by all previous tests. Alignment holds only while every render produces exactly three calls. If a render count changes, tests such as the composition cases at Lines 300-348 silently receive the wrong collection and assert against the wrong data.Reset the counter in
beforeEachso each test starts at slot 0.♻️ Proposed change
const mocks = vi.hoisted(() => ({ personId: null as string | null, tree: undefined as IdentityPerson | undefined, + call: 0, grid: {vi.mock("`@/queries/metric-results`", () => ({ - useMetricCollection: (() => { - let call = 0; - return () => { - const r = mocks.collections[call % 3] ?? emptyCollection(); - call += 1; - return r; - }; - })(), + useMetricCollection: () => { + const r = mocks.collections[mocks.call % 3] ?? emptyCollection(); + mocks.call += 1; + return r; + }, }));beforeEach(() => { seedHappyOrg(); + mocks.call = 0; mocks.grid.isPending = false;🤖 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/domain-lens-view.test.tsx` around lines 60 - 71, Reset the module-scoped call counter used by the mocked useMetricCollection in the test file before each test, ensuring every test starts at slot 0 while preserving the existing three-call ordering and collection selection.src/frontend/src/lib/insight/within-team-peer.ts (1)
59-93: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftRepeated
forEntityscans inside per-member loops.forEntityinsrc/frontend/src/lib/metrics/collection.tsruns onefindand threefilterpasses over the completeperiod,timeseries,breakdown, andhistogramarrays. Every site below calls it once per member inside a loop, and those arrays also grow with the roster, so the cost is quadratic in headcount and is repeated for each metric in the grid. Build the per-entity index once per result and reuse it.
src/frontend/src/lib/insight/within-team-peer.ts#L59-L93: build aMap<string, number | null>of period values once, then use it at Line 64 and Line 83 instead of callingforEntitytwice per member.src/frontend/src/lib/insight/attention-flags.ts#L78-L83: index the period values ofronce per metric key, then read each member's value from that index when buildingpoints; apply the same treatment to theprevlookup at Line 172.src/frontend/src/lib/portal/trend-data.ts#L59-L71: groupr.timeseries.seriesbyentity_idonce per metric key, then iterate the member ids against that grouping instead of callingforEntityfor every key-member pair.A shared helper on
NormalizedMetricResultthat returns an entity-indexed view would remove the duplication across all three call sites.🤖 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/within-team-peer.ts` around lines 59 - 93, Replace repeated per-member forEntity scans with a reusable entity-indexed view, preferably a shared helper on NormalizedMetricResult. In src/frontend/src/lib/insight/within-team-peer.ts lines 59-93, build one period-value Map and reuse it for cohort bucketing and returned target values; in src/frontend/src/lib/insight/attention-flags.ts lines 78-83, index r period values once per metric key and reuse the index for points, applying the same change to the prev lookup at line 172; in src/frontend/src/lib/portal/trend-data.ts lines 59-71, group r.timeseries.series by entity_id once per metric key and read member values from that grouping.src/frontend/src/lib/portal/metric-stats.ts (1)
125-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReuse
formatAxisTickin the distribution bins.
fmtCompactonly formats thousands, so million-level summary values become long labels like1000k.formatAxisTickalready handlesMlabels, rounds nearby axis ticks, and is covered by the existing axis formatter tests. Use it for the non-percent distribution formatter instead of maintaining a separate compact formatter.🤖 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/metric-stats.ts` around lines 125 - 132, Replace the separate fmtCompact logic used for non-percent distribution bin labels with the existing formatAxisTick formatter. Update the distribution formatting path to reuse formatAxisTick so million-level values receive M labels and its established rounding behavior; remove fmtCompact if it is no longer referenced.src/frontend/src/lib/portal/portal-store.ts (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the module doc:
zoneno longer lives here.The comment describes
zoneas in-memory navigation state of this store.PortalStateholds onlyenabledandshowPlanned, and the zone now rides in the URL (portal-search.ts,portal-nav.ts). Remove thezoneparagraph so the doc matches the module.🤖 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/portal-store.ts` around lines 3 - 11, Update the module documentation comment in the portal store to remove the paragraph describing zone as in-memory navigation state, since PortalState only contains enabled and showPlanned; retain the remaining enabled and feature-flag documentation.src/frontend/src/lib/portal/use-org-scope.ts (1)
64-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
walkrecomputes subtree sizes quadratically.
walkvisits every node once. For each manager node it callsflattenSubordinates(node), which walks that node's entire subtree again. The total cost is O(n · depth), and it degrades to O(n²) for a deep reporting chain.One post-order pass yields every subtree size. This matters more because the memo above currently never hits — see the
usePortalScopeidentity comment onsrc/frontend/src/lib/portal/portal-nav.tslines 43-46.♻️ Proposed fix
const managerNodes: ManagerNode[] = []; - const walk = (node: IdentityPerson, depth: number): void => { - if (node.subordinates.length > 0) { - managerNodes.push({ - person_id: node.person_id, - name: node.display_name || node.email, - depth, - teamSize: flattenSubordinates(node).length, - }); - } - for (const sub of node.subordinates) walk(sub, depth + 1); - }; - walk(viewerNode, 0); + // Post-order: each node returns its own subtree size, so `teamSize` costs + // one visit per node instead of a fresh flatten per manager. + const walk = (node: IdentityPerson, depth: number): number => { + let size = 0; + for (const sub of node.subordinates) size += walk(sub, depth + 1) + 1; + if (node.subordinates.length > 0) { + managerNodes.push({ + person_id: node.person_id, + name: node.display_name || node.email, + depth, + teamSize: size, + }); + } + return size; + }; + walk(viewerNode, 0); + // Restore pre-order ordering for the picker. + managerNodes.sort((a, b) => a.depth - b.depth);Verify the ordering
ScopeSelectexpects before applying thesort. The originalwalkpushed in pre-order, and the picker indents bydepth. If it relies on parent-before-child sibling ordering, collect the entries in pre-order and fillteamSizein a second pass instead.🤖 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-org-scope.ts` around lines 64 - 76, Update the manager-tree traversal around walk to compute each node’s subtree size in a single post-order pass instead of calling flattenSubordinates for every manager. Preserve the existing managerNodes ordering and depth values expected by ScopeSelect; if subtree sizes cannot be assigned during post-order without changing that pre-order ordering, collect entries in the current pre-order and populate teamSize in a separate pass.src/frontend/src/lib/portal/portal-nav.ts (1)
88-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize both sides of the root comparison before resetting
direct.
patch.rootisstring | nullandprev.scopeisstring | undefined. Two cases make the comparison report a move that did not happen:
- The caller selects the viewer root (
root: null) whileprev.scopeis already absent.null !== undefinedis true, sodirectresets.validatePortalSearchlower-casesscope(portal-search.tsline 79), butScopeSelectpassesm.person_idverbatim. Aperson_idwith any uppercase character never equalsprev.scope, sodirectresets on every pick.Compare normalized values instead.
♻️ Proposed fix
...("root" in patch && !("directOnly" in patch) && - patch.root !== prev.scope + (patch.root?.toLowerCase() ?? null) !== (prev.scope ?? null) ? { direct: undefined } : {}),🤖 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/portal-nav.ts` around lines 88 - 92, Normalize both sides of the root comparison in the patch handling logic before deciding whether to reset direct: treat null and undefined as the same viewer-root value, and compare string roots case-insensitively to match validatePortalSearch and ScopeSelect behavior. Preserve the existing direct reset for genuinely different roots.src/frontend/src/hooks/use-portal-period.ts (1)
30-37: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStabilize the returned values and move the preference read out of the render body.
Two points on this block:
- Line 30 reads
localStorageduring render whenever the URL names no period. That is an impure render-phase read of a mutable external store. Under concurrent rendering two components in the same pass can observe different values aftersetPeriodwrites the preference. This module already has the correct primitive next door:portal-store.tsusesuseSyncExternalStore. Expose the period preference through the same pattern.- Lines 31-37 allocate a new
customRangeand a newdateRangeon every render. Consumers passdateRangestraight intouseMetricCollection(src/frontend/src/components/portal/single-group-view.tsxline 38). Confirm no consumer listsdateRangeorcustomRangein auseMemo,useEffect, or query-key dependency array, because a fresh identity there re-runs the effect or refetches on every render.Wrap both in
useMemokeyed onsearch.period,search.from, andsearch.to.Run the following script to check whether any consumer depends on the identity of
dateRangeorcustomRange:#!/bin/bash # Description: Find consumers that place dateRange/customRange in a dependency array. set -euo pipefail # Locate every usePortalPeriod consumer. rg -n --type=ts --type=tsx -C3 '\busePortalPeriod\s*\(' src/frontend/src || true # Then look for dependency arrays that include the unstable values. rg -nP --glob '*.ts' --glob '*.tsx' -C2 '\]\s*,\s*\[[^\]]*\b(dateRange|customRange)\b[^\]]*\]' src/frontend/src || true # And effects/memos referencing them. ast-grep run --pattern 'useEffect($_, [$$$, dateRange, $$$])' --lang tsx src/frontend/src || true ast-grep run --pattern 'useMemo($_, [$$$, dateRange, $$$])' --lang tsx src/frontend/src || true🤖 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/hooks/use-portal-period.ts` around lines 30 - 37, Update usePortalPeriod to expose the period preference through the module’s useSyncExternalStore-based store pattern instead of reading localStorage during render, reusing the existing preference subscription primitives. Memoize the returned customRange and dateRange values with useMemo keyed by search.period, search.from, and search.to, while preserving the current range-resolution behavior and stable identities for consumers.src/frontend/src/components/portal/slice-select.tsx (1)
22-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a sentinel that cannot collide with a data-derived dimension key.
TEAM_KEYis the plain string"team".dimsare derived from roster attributes, so a roster attribute namedteamproduces a second entry with the same key inall. Two effects follow. React reports a duplicate key for the twoSelectItemelements.setSlicethen maps that real dimension to"", because line 41 treats any value equal toTEAM_KEYas "no slice".Use a reserved sentinel that an attribute key cannot produce. The
d.key || "team"fallback at line 55 is then unnecessary, because every entry inallhas a non-empty key.♻️ Proposed change
-const TEAM_KEY = "team"; +const TEAM_KEY = "__team__"; const TEAM_SLICE = { key: TEAM_KEY, label: "Team (all)" };- <SelectItem key={d.key || "team"} value={d.key}> + <SelectItem key={d.key} value={d.key}>#!/bin/bash # Description: Determine whether availableSlices can emit the key "team". set -euo pipefail fd -t f 'slices.ts' -x ast-grep outline {} --items all rg -n -C6 'availableSlices|PLANNED_SLICES|collectRosterAttrs' src/frontend/src/lib/insight/slices.ts rg -n -C4 '"team"' src/frontend/src/lib/insight src/frontend/src/lib/portalAlso applies to: 34-37, 54-58
🤖 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/slice-select.tsx` around lines 22 - 23, Replace the plain "team" value used by TEAM_KEY with a reserved sentinel that cannot match any roster-derived dimension key, and update the TEAM_SLICE/setSlice comparisons to use it consistently. Remove the d.key || "team" fallback when constructing all, since every entry must now retain a non-empty key while preserving the team-wide selection behavior.src/frontend/src/components/portal/people-view.tsx (1)
10-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the module guard a reset hook for tests.
lastRouteSyncis module state with no way to clear it. The behavior is correct for a user session, and the comment explains why a ref is wrong here. The cost lands on tests. Vitest gives one module registry per test file, so the first case that mountsPeopleViewwith a givenpersonconsumes the guard. A later case in the same file that mounts the samepersonrecords noreplaceScopecall, and the failure looks like a bug in the component.Export a reset function, or require
vi.resetModules()in the suite. A reset is cheaper and makes the ordering dependency explicit.♻️ Proposed change
let lastRouteSync: string | null = null; + +/** Test-only: clear the cross-mount sync guard between cases. */ +export function resetRouteSyncGuard(): void { + lastRouteSync = null; +}🤖 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/people-view.tsx` around lines 10 - 16, Export a test-facing reset function near the module-scoped lastRouteSync guard that sets lastRouteSync back to null. Keep the guard’s session behavior unchanged, and make the reset callable by tests before mounting PeopleView so repeated cases for the same person can independently verify route synchronization.src/frontend/src/components/portal/context-pane.tsx (1)
279-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHide the live group when it has no items, as
ThemeNavdoes.
ThemeNavskips a group whoselivelist is empty (line 240).ItemsNavalways renders the group. IfshowPlannedis false and every item initemsis planned, the pane shows thegroupLabelheading with no rows below it. Align the two renderers.♻️ Proposed change
- <SidebarGroup> - <SidebarGroupLabel>{groupLabel}</SidebarGroupLabel> - <SidebarGroupContent> - <SidebarMenu> - {live.map((it) => ( - <ItemButton key={it.id} item={it} active={active === it.id} /> - ))} - </SidebarMenu> - </SidebarGroupContent> - </SidebarGroup> + {live.length ? ( + <SidebarGroup> + <SidebarGroupLabel>{groupLabel}</SidebarGroupLabel> + <SidebarGroupContent> + <SidebarMenu> + {live.map((it) => ( + <ItemButton key={it.id} item={it} active={active === it.id} /> + ))} + </SidebarMenu> + </SidebarGroupContent> + </SidebarGroup> + ) : null}🤖 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/context-pane.tsx` around lines 279 - 290, Update the live-group render path in ItemsNav within context-pane.tsx so it matches ThemeNav’s behavior when the live list is empty. Add an early empty-state check before rendering the SidebarGroup/SidebarMenu for the live items, and return nothing for that group when live has no entries. Keep the existing ItemButton mapping and active selection logic unchanged for non-empty live lists.src/frontend/src/test/portal-router.tsx (1)
69-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the
navigatesearch update throughsetso it stripsundefinedkeys.
portalRouter.setdeletes keys whose value isundefined(lines 43-45). Thenavigatemock assignsportalRouter.searchdirectly and skips that step. A component that clears a portal key withnavigate({ search: (p) => ({ ...p, item: undefined }) })therefore leavesitem: undefinedon the recorded state.Two consequences follow. A test that asserts the search object with
toEqualsees an extra key. The mock also diverges fromLink, which filtersundefinedat line 135, so the same navigation produces different state depending on which surface the test drives.Reuse
setfor both branches.♻️ Proposed change
if (typeof opts.search === "function") { - portalRouter.search = ( - opts.search as (p: unknown) => Record<string, unknown> - )(portalRouter.search) as PortalSearch; - emit(); + portalRouter.set( + (opts.search as (p: unknown) => Record<string, unknown>)( + portalRouter.search, + ) as Partial<PortalSearch>, + ); } else if (opts.search) { - portalRouter.search = opts.search as PortalSearch; - emit(); + portalRouter.set(opts.search as Partial<PortalSearch>); }Note:
setmerges, while a realnavigatewith an object replaces. If a test depends on replacement, clear the state first rather than reintroducing the direct assignment.🤖 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/test/portal-router.tsx` around lines 69 - 89, Update the navigate mock in portalRouterMock so both function and object search updates go through portalRouter.set, ensuring undefined keys are removed. Preserve navigate’s replacement semantics by clearing the existing search state before applying an object update when necessary, rather than assigning portalRouter.search directly.src/frontend/src/components/portal/team-state-view.tsx (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the stale comment about
GROUPS.The comment states that
GROUPS"is called INSIDE the memo — it returns a fresh array per call". Line 98 iteratesGROUPSas a value. Two other files in this PR do the same:src/frontend/src/components/portal/single-group-view.tsxline 35 callsGROUPS.find(...), andsrc/frontend/src/components/portal/context-pane.tsxline 483 assignsconst groups = GROUPS.
GROUPSis a module constant, so its identity is already stable and omitting it from the dependency list is correct. The reasoning in the comment is not. Update the text so a later reader does not act on the wrong premise.♻️ Proposed change
- // metric catalog across every group would blow past it. `GROUPS` is - // called INSIDE the memo — it returns a fresh array per call, and a fresh - // dependency would defeat the memo and re-key the grid query every render. + // metric catalog across every group would blow past it. `GROUPS` is a module + // constant, so it is read inside the memo and left out of the dependency + // list; the memo keeps one stable collection and one stable grid query key.🤖 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/team-state-view.tsx` around lines 89 - 104, Update the comment above headlineKeys and gridCollection to describe GROUPS as a stable module-level constant whose identity does not need to appear in the memo dependencies; remove the incorrect claim that it is called inside the memo or returns a fresh array. Leave the useMemo implementation and dependency list unchanged.src/frontend/src/components/portal/zone-content.tsx (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive unknown zones neutral pending copy.
ZoneScaffoldreceives any zone that the switch does not handle, including an unrecognized zone from the URL. Line 53 assigns the reports copy to every zone that is notscorecard, so an unknown zone reads "Pending: diagnosis circuit + report builder". Line 61 already falls back to "Portal" for the title. Align the body copy with that fallback.♻️ Proposed change
- const pending = - zone === "scorecard" - ? "org snapshots + unit × quarter aggregation" - : "diagnosis circuit + report builder"; + const PENDING: Record<string, string> = { + scorecard: "org snapshots + unit × quarter aggregation", + reports: "diagnosis circuit + report builder", + }; + const pending = PENDING[zone] ?? "this lens";🤖 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/zone-content.tsx` around lines 52 - 56, Update the pending-copy selection in ZoneScaffold so only "scorecard" uses the scorecard text, the recognized reports zone uses the diagnosis/report text, and unrecognized zones receive neutral pending copy aligned with the existing "Portal" title fallback.src/frontend/src/components/portal/section-trend.tsx (2)
53-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the alias remapping and the state branches.
The reviewed file list contains no
section-trend.test.tsx.SectionTrendcarries logic that deserves direct coverage: thesafeKeysalias generation at Line 92, the row rewrite that drops null values at Line 94, the color cycling at Line 106, and the pending, error, and empty branches at Lines 65 to 86.Run the following script to confirm whether coverage exists elsewhere:
#!/bin/bash # Description: Locate tests that exercise SectionTrend. fd -t f 'section-trend' src/frontend/src rg -n --type=tsx -C 3 'SectionTrend' -g '*.test.tsx' src/frontend/srcAs per coding guidelines: "Ensure new and changed lines achieve at least 80% test 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/portal/section-trend.tsx` around lines 53 - 108, Add focused unit tests for SectionTrend covering pending, error with retry, empty-data, and rendered-data states. Verify alias generation for dotted and special-character series keys, row rewriting including omission of null values, and color cycling across DEFAULT_CHART_KEYS; place tests in a section-trend test file and ensure the changed logic reaches at least 80% coverage.
139-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDerive the right axis from the series instead of trusting the prop.
Line 166 honors
s.yAxisId === "right", but Line 139 renders the right<YAxis>only when the caller passesrightAxis. A caller that setsyAxisId: "right"on a series and omitsrightAxisproduces a series bound to an axis that does not exist. The current caller insrc/frontend/src/components/portal/domain-lens-view.tsxat Line 652 derives both from the same list, so the production path is consistent. The component type does not enforce that pairing.♻️ Proposed refactor
- {rightAxis ? ( + {rightAxis || series.some((s) => s.yAxisId === "right") ? ( <YAxis yAxisId="right"🤖 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/section-trend.tsx` around lines 139 - 167, Update the SectionTrend axis setup so the right <YAxis> is derived from safeSeries instead of relying only on the rightAxis prop. Use the existing yAxisId logic in the SectionTrend map to detect when any series targets "right", and render the right axis whenever that condition is present. Keep the current series-to-axis binding in SectionTrend unchanged so s.yAxisId continues to drive the chart. Ensure the rightAxis prop no longer has to be manually paired with series using "right", while preserving existing behavior when no series needs the right axis.src/frontend/src/components/portal/domain-lens-view.tsx (1)
447-464: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the per-bucket active-people aggregation.
ParticipationSectionrebuildsbyDateon every render. The loop is triple nested overspec.metrics,memberIds, and every timeseries point. At org scope this repeats a large scan for each parent re-render, for example on a slice or period change.♻️ Proposed refactor
- const byDate = new Map<string, Set<string>>(); - for (const key of spec.metrics) { - const r = trend.byKey.get(key); - if (!r) continue; - for (const id of memberIds) { - for (const s of forEntity(r, id).series) { - for (const p of s.points) { - if ((p.value ?? 0) > 0) { - (byDate.get(p.bucket_start) ?? byDate.set(p.bucket_start, new Set()).get(p.bucket_start)!).add(id); - } - } - } - } - } - const data = [...byDate.entries()] - .map(([date, ids]) => ({ date, active: ids.size })) - .sort((a, b) => a.date.localeCompare(b.date)); + const data = useMemo(() => { + const byDate = new Map<string, Set<string>>(); + for (const key of spec.metrics) { + const r = trend.byKey.get(key); + if (!r) continue; + for (const id of memberIds) { + for (const s of forEntity(r, id).series) { + for (const p of s.points) { + if ((p.value ?? 0) > 0) { + let set = byDate.get(p.bucket_start); + if (!set) { + set = new Set<string>(); + byDate.set(p.bucket_start, set); + } + set.add(id); + } + } + } + } + } + return [...byDate.entries()] + .map(([date, ids]) => ({ date, active: ids.size })) + .sort((a, b) => a.date.localeCompare(b.date)); + }, [spec.metrics, trend.byKey, memberIds]);Move the
useMemoabove theif (memberIds.length === 0) return null;guard at Line 445 to keep the hook order stable.🤖 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/domain-lens-view.tsx` around lines 447 - 464, Memoize the per-bucket active-people aggregation inside ParticipationSection so the byDate/data computation is not rebuilt on every render. Move the useMemo for the byDate scan above the early return on memberIds.length === 0 to keep hook order stable, and reuse the existing spec.metrics, memberIds, trend.byKey, and forEntity inputs as dependencies so the aggregated result only recomputes when those inputs change.
| // A lens we simply have not built yet is hidden unless the viewer asked for | ||
| // planned work; a lens waiting on the product stays listed (dimmed) because | ||
| // it tells the reader the domain exists in our model. | ||
| const lenses = direction.lenses.filter((lens) => { | ||
| const entry = lensEntry(direction.id, lens); | ||
| if (!entry || !("comingSoon" in entry)) return true; | ||
| return entry.readiness === "planned" || showPlanned; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The readiness filter looks inverted relative to its comment.
The comment states two rules. A lens that is not built yet is hidden unless the viewer asked for planned work. A lens waiting on the product stays listed.
The predicate does the reverse. For a roadmap entry it returns true when entry.readiness === "planned", so a planned lens is always visible. Every other roadmap readiness is visible only when showPlanned is true.
The rest of this file treats planned as the gated, demoted state: partitionByReadiness(items, showPlanned) at lines 235 and 278, PLANNED_GROUP_LABEL, and the planned prop on ItemButton. If that vocabulary holds here, the test should be entry.readiness !== "planned" || showPlanned.
This also changes the default lens. Line 391 seeds setLens(lenses[0] ?? direction.lenses[0]!) from the filtered list, so a wrong filter selects a wrong lens when the reader expands a direction.
Confirm the readiness union and the intended gate before changing the condition.
#!/bin/bash
# Description: Resolve the lens readiness contract and compare it with the pane filter.
set -euo pipefail
# The lens entry shape and readiness union.
fd -t f 'lens-configs.ts' -x ast-grep outline {} --items all
# Every readiness literal in the portal libs.
rg -n --type=ts 'readiness' src/frontend/src/lib | head -50
# The shared gate the rest of the pane uses.
ast-grep run --pattern 'function partitionByReadiness($$$) { $$$ }' --lang typescript src/frontend/src/lib/portal/nav-model.ts
# Existing expectations, if any, for this filter.
rg -n -C4 'comingSoon|readiness' src/frontend/src/lib/portal/lens-configs.test.ts🤖 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/context-pane.tsx` around lines 377 - 384,
Verify the lens readiness union and existing partitionByReadiness contract, then
update the filter inside the lenses computation to gate only planned entries:
non-planned lenses remain visible by default, while planned lenses appear only
when showPlanned is true. Preserve the downstream lenses[0] default-selection
behavior and align it with the shared readiness vocabulary.
| for (const id of memberIds) { | ||
| const bins = forEntity(result, id).histogram[0]?.bins; | ||
| if (!bins?.length) continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject partial histograms.
A requested member with no bins is skipped at Line 25. The function then returns totals for only a subset of memberIds. This conflicts with the documented all-entity compatibility contract.
src/frontend/src/lib/portal/event-histogram.ts#L23-L25: Returnnullwhen any requested member has no non-empty histogram bins.src/frontend/src/lib/portal/event-histogram.test.ts#L21-L30: Add a case with one populated member and one requested member without bins. Expectnull.
Proposed fix
for (const id of memberIds) {
const bins = forEntity(result, id).histogram[0]?.bins;
- if (!bins?.length) continue;
+ if (!bins?.length) return null;📝 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.
| for (const id of memberIds) { | |
| const bins = forEntity(result, id).histogram[0]?.bins; | |
| if (!bins?.length) continue; | |
| for (const id of memberIds) { | |
| const bins = forEntity(result, id).histogram[0]?.bins; | |
| if (!bins?.length) return null; |
📍 Affects 2 files
src/frontend/src/lib/portal/event-histogram.ts#L23-L25(this comment)src/frontend/src/lib/portal/event-histogram.test.ts#L21-L30
🤖 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/event-histogram.ts` around lines 23 - 25, Update
the member iteration in forEntity/event histogram aggregation to return null
immediately when any requested member has no non-empty bins, instead of skipping
it; preserve aggregation only when every member is represented. In
src/frontend/src/lib/portal/event-histogram.test.ts lines 21-30, add coverage
with one populated member and one requested member without bins, asserting the
result is null.
| { | ||
| kind: "participation", | ||
| metrics: ["ai.active_days"], | ||
| title: "AI adoption", | ||
| noun: "People using AI", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Remove static metric semantics from this configuration.
noun, caption, and unitLabel define user-facing meaning for API metrics in the frontend. Derive these values from the metric definition response in the renderer. Do not describe ai.active_days as AI adoption or define git.commits units as “commits per person” here.
As per coding guidelines, “Use metric names, descriptions, and units supplied by API responses; do not invent user-facing metric semantics in the frontend.”
Also applies to: 78-84
🤖 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/overview-configs.ts` around lines 37 - 42, Remove
the static user-facing metric semantics from the participation configuration and
the corresponding git.commits configuration, including noun, caption, and
unitLabel values. Update the renderer that consumes these configs to derive
those fields from the API metric definition response, while preserving the
metric names and existing configuration structure.
Source: Coding guidelines
| const portal = usePortalEnabled(); | ||
| const pathname = useRouterState({ select: (s) => s.location.pathname }); | ||
| // The portal is a ROUTE now, so it renders through the Outlet like anything | ||
| // else — otherwise its navigation could never live in the URL. It still owns | ||
| // the whole shell on the routes it claims (/portal and the person pages); | ||
| // everywhere else (/metrics, /whats-new) the app chrome stays, which the | ||
| // old "portal replaces the app" branch used to swallow. | ||
| const portalRoute = | ||
| pathname === "/portal" || /^\/ic\/[^/]+\/(personal|team)\/?$/.test(pathname); | ||
| return ( | ||
| <TooltipProvider> | ||
| {/* Upstream's evidence-dialog provider wraps everything; the portal | ||
| branch lives inside it, so a drilldown opened from a portal surface | ||
| finds the same provider the legacy screens use. */} | ||
| <MetricEvidenceDialogProvider> | ||
| <AuthGate> | ||
| <SidebarProvider> | ||
| <AppSidebar /> | ||
| <SidebarInset className="min-w-0 overflow-x-clip"> | ||
| <MockBanner /> | ||
| <ViewAsBanner /> | ||
| <Outlet /> | ||
| </SidebarInset> | ||
| </SidebarProvider> | ||
| {portal && portalRoute ? ( | ||
| <PortalLayout /> | ||
| ) : ( | ||
| <SidebarProvider> | ||
| <AppSidebar /> | ||
| <SidebarInset className="min-w-0 overflow-x-clip"> | ||
| <MockBanner /> | ||
| <ViewAsBanner /> | ||
| <Outlet /> | ||
| </SidebarInset> | ||
| </SidebarProvider> | ||
| )} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move portal routing behavior out of route files.
These route files now contain shell selection, search handling, and redirect behavior. Keep route files as thin screen wrappers.
src/frontend/src/routes/__root.tsx#L56-L83: move portal shell selection into a screen or routing adapter outsidesrc/routes/.src/frontend/src/routes/ic.$person.tsx#L8-L18: move portal search handling into the portal navigation layer.src/frontend/src/routes/index.tsx#L8-L14: move the portal landing redirect into the portal navigation layer.src/frontend/src/routes/portal.tsx#L16-L25: wrap a portal screen component only.
As per coding guidelines, “Keep route files under src/routes/ thin: they should only wrap a screen component and contain no additional logic.”
📍 Affects 4 files
src/frontend/src/routes/__root.tsx#L56-L83(this comment)src/frontend/src/routes/ic.$person.tsx#L8-L18src/frontend/src/routes/index.tsx#L8-L14src/frontend/src/routes/portal.tsx#L16-L25
🤖 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/routes/__root.tsx` around lines 56 - 83, Move portal shell
selection from src/frontend/src/routes/__root.tsx lines 56-83 into a screen or
routing adapter outside src/routes/, preserving the PortalLayout, app-shell, and
route matching behavior. Move portal search handling from
src/frontend/src/routes/ic.$person.tsx lines 8-18 and the portal landing
redirect from src/frontend/src/routes/index.tsx lines 8-14 into the portal
navigation layer. Update src/frontend/src/routes/portal.tsx lines 16-25 to only
wrap and render a portal screen component; keep all route files as thin screen
wrappers without additional navigation or shell logic.
Source: Coding guidelines
… paths Review round on the monorepo PR. Six real findings, all in code this branch added: - The teammate switcher in the person header still navigated by email, so picking a peer opened a page whose metrics resolved to nobody. It now passes `person_id` like every other link — a leftover from the identity cutover, and the one bug in that migration. - /portal rendered the portal through the Outlet even with the preview flag off, because the root shell only swaps in PortalLayout when the flag is on. A pasted URL (or turning the preview off while standing there) painted the portal inside the app chrome. The route now redirects home. - `setCustomRange` threw on an invalid range from inside an event handler, where no error boundary catches it. It drops the range instead — the same degrade-not-error policy `validatePortalSearch` promises. - `decodeURIComponent` on the path segment raises URIError on a malformed percent-sequence, crashing the shell during render. All three sites now go through `personIdFromPath`, which falls back to "no person". - The portal store read localStorage unguarded at module scope, so blocked storage (sandboxed iframe, third-party cookie blocking) threw a SecurityError before any component mounted. Guarded like the write path. - `usePortalScope` returned a fresh object every render, defeating `useOrgScope`'s memo and re-walking the identity tree on every render of every org zone. Memoised on the two primitives. Also moved the portal's shell-path predicate out of __root into `lib/portal/portal-routes`, so the route file asks rather than decides. Two findings are not bugs and are answered on the PR instead: the lens readiness filter matches `partitionByReadiness` (`planned` is roadmap the reader benefits from seeing, `unbuilt` is ours and gated), and skipping a member with no histogram bins is deliberate — they had no events, and bailing would blank the chart for the whole org whenever one person was inactive. The histogram's doc comment now says so outright. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
Thanks — six of the eight are real and fixed in Fixed
Partly taken: route files should stay thin. Moved the shell-path predicate out of Not a bug: the lens readiness filter. The vocabulary is the other way round from your reading, and it matches the shared helper: in Not a bug: partial event histograms. Skipping a member with no bins is deliberate: they had no events in the period, which is a normal reading on any real roster. Returning null there would blank the org chart whenever one person was inactive — the opposite of the intent. The compatibility contract is about bin edges, which is still enforced (differing edges or an anomalous bin count return null). The doc comment now says both things outright, since the old wording was what made this ambiguous. |
|
Correcting myself on the The registry carries 59 metrics, all 59 with Three of the four fields are derivable, one is not:
I have not changed it in this PR, because the honest version of the fix is "the portal shows |
The first pass only answered the nine findings posted inline; the review body carried another 12 minor and 21 nitpick items. Going through them, most were right. Correctness and copy: - formatAxisTick printed "1000k" for 999_600 — a tick naming a magnitude the scale below it already covers. Rolls over to M now. - The distribution binned a negative value into counts[-1], which lands on a property outside the array, so the person vanished from the histogram and the bars stopped summing to the population. Clamped at both ends. - fmtCompact stopped at thousands, so a million-level bin edge read "1000k". It keeps its own ladder rather than delegating to formatAxisTick: a bin edge is an exact value off the 1/2/5 ladder, while a tick marks a gridline, so the two round differently on purpose. - "All 1 people are within their usual range" — a scope of one is a real case (a lead with one report). Both attention sentences pluralise now. - The members-table caption interpolated an empty scope label, so the accessible name for the table started with a dash. - Average AI cost hardcoded currency and USD while the tile above it already read format and unit off the metric result. - The cohort label lowercased any dimension label. Fine for "Division", wrong the moment identity exposes attributes generically (constructorfabric#1881) and a label is "R&D area". Only a plain capitalised word is lowered. - The slice sentinel was the plain string "team", so a roster attribute named `team` collided with it: duplicate React key, and picking the real dimension read as "no slice". It is `__team__` now. - An unrecognised zone from the URL rendered the reports scaffold copy, so it claimed pending work that has nothing to do with it. - The compact date-range trigger loses its label below `sm` and had no accessible name at all — it is icon-only there. - useViewerIsManager answered "not a manager" when identity failed, which demotes the viewer to an IC shell instead of letting the org zone show the identity error. Unresolved until a tree arrives. - Two person-id comparisons were case-sensitive while the rest of the app normalises: the route→scope sync re-fired on a casing difference, and setScope treated a same-root move as a new root and dropped direct-only. - An empty live group in the pane rendered a heading with nothing under it, which reads as a load failure rather than as a filter (ThemeNav already skipped it). Performance: - The manager-node walk called flattenSubordinates per manager, re-walking that subtree — O(n · depth). One pass now returns each subtree size, with the entry pushed before recursing so the picker keeps its outline order. Test added: post-order would have reordered it into depth bands. - The participation section rebuilt its per-bucket active-people scan (metrics × roster × buckets) on every parent render. - usePortalPeriod read localStorage in the render body — an impure read of a mutable store, where two components in one concurrent pass can disagree after a write. The preference is subscribed through the store that already exists next door, and the returned range and callbacks are stable. Tests: - section-trend had no test file. Ten cases: the CSS-safe alias remapping on both series and rows, nulls left as gaps, right-axis derivation, stacking, and the pending / error / empty branches. - shell-layout mocked useViewerIsManager twice, so the state-controlled factory was dead. It was scaffolding rather than a masked failure — the IC cases live in portal-shell — but the override is gone. - domain-lens-view's collection mock kept a counter across the whole file, so each caller's slot depended on how many renders earlier tests happened to do. Reset per test. - The retry spies in employees-view and manage-view accumulated across the file, so toHaveBeenCalledOnce held only while exactly one test triggered a retry. - portal-shell claimed to test a remount while mounting two live instances. It unmounts first now, and its person fixtures are ids. - attention-list minted its own person ids with a helper that broke the UUID shape past nine; it uses the shared `pid`. - The router fake assigned search directly, keeping cleared keys as `undefined` where the real router drops them — it routes through `set`. - The route→scope guard moved to lib/portal/route-scope-sync with a reset, so a second test mounting the same person is not silently a no-op. Two were not bugs. The portal-store and team-state comments were stale rather than wrong in code, and are corrected in place. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
I had only answered the nine inline findings — the review body carried another 12 minor and 21 nitpick items, which I missed. Worked through all of them in Correctness and copy
Performance
Tests
Not taken
Verified on the ported tree: |
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/lib/portal/portal-nav.ts (1)
78-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
directwhenreplaceScopechanges the root.If the current URL has
direct=true,replaceScope({ root })keeps that filter for the new root.PeopleViewuses this action during route-to-scope synchronization. The new team can then show direct reports only instead of its full roster.Apply the same normalized previous-root comparison that
setScopeuses. Add a regression test for changing fromscope=A&direct=truetoscope=B.Proposed fix
replaceScope: (patch) => setSearch( - { ...("root" in patch ? { scope: patch.root ?? undefined } : {}) }, + (prev) => ({ + ...("root" in patch ? { scope: patch.root ?? undefined } : {}), + ...("root" in patch && + normalizePersonId(patch.root ?? "") !== + normalizePersonId(prev.scope ?? "") + ? { direct: undefined } + : {}), + }), { replace: true }, ),🤖 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/portal-nav.ts` around lines 78 - 82, Update the replaceScope action in portal-nav.ts so it uses the same normalized previous-root comparison as setScope and clears the direct filter when the root actually changes. Keep the existing scope update behavior, but ensure a transition like scope=A&direct=true to scope=B does not preserve direct=true. Add a regression test covering this replaceScope root-change case.
🤖 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/hooks/use-portal-period.test.tsx`:
- Around line 21-30: Reset the module-level period preference in the test
beforeEach alongside portalRouter and localStorage cleanup. Use the reset
mechanism exposed by use-period.ts (or add a test-only reset if none exists) so
each usePortalPeriod test starts with no stored period and cannot inherit the
"month" value from earlier tests.
In `@src/frontend/src/lib/portal/route-scope-sync.ts`:
- Around line 14-29: The claimScopeSync guard only remembers the latest person,
allowing a previously synchronized person to be claimed again. Replace
lastSynced with a Set<string> of synchronized person IDs, have claimScopeSync
reject any ID already in the set and record new IDs, and clear the set in
resetRouteScopeSync. Add coverage for the A → B → A sequence, verifying the
final A claim returns false.
---
Outside diff comments:
In `@src/frontend/src/lib/portal/portal-nav.ts`:
- Around line 78-82: Update the replaceScope action in portal-nav.ts so it uses
the same normalized previous-root comparison as setScope and clears the direct
filter when the root actually changes. Keep the existing scope update behavior,
but ensure a transition like scope=A&direct=true to scope=B does not preserve
direct=true. Add a regression test covering this replaceScope root-change case.
🪄 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: bc833476-4df0-4ca3-a461-6a620ecea4d7
📒 Files selected for processing (44)
src/frontend/src/components/app-sidebar.tsxsrc/frontend/src/components/org-tree.tsxsrc/frontend/src/components/portal/ai-cost-view.tsxsrc/frontend/src/components/portal/attention-list.test.tsxsrc/frontend/src/components/portal/context-pane.tsxsrc/frontend/src/components/portal/domain-lens-view.test.tsxsrc/frontend/src/components/portal/domain-lens-view.tsxsrc/frontend/src/components/portal/employees-view.test.tsxsrc/frontend/src/components/portal/manage-view.test.tsxsrc/frontend/src/components/portal/people-view.tsxsrc/frontend/src/components/portal/person-header.tsxsrc/frontend/src/components/portal/portal-shell.test.tsxsrc/frontend/src/components/portal/section-trend.test.tsxsrc/frontend/src/components/portal/section-trend.tsxsrc/frontend/src/components/portal/shell-layout.test.tsxsrc/frontend/src/components/portal/slice-select.tsxsrc/frontend/src/components/portal/team-state-view.tsxsrc/frontend/src/components/portal/zone-content.tsxsrc/frontend/src/components/widgets/period-selector-bar.tsxsrc/frontend/src/hooks/use-period.tssrc/frontend/src/hooks/use-portal-period.test.tsxsrc/frontend/src/hooks/use-portal-period.tssrc/frontend/src/lib/format.tssrc/frontend/src/lib/insight/attention-flags.tssrc/frontend/src/lib/metrics/entity.test.tssrc/frontend/src/lib/metrics/entity.tssrc/frontend/src/lib/portal/event-histogram.tssrc/frontend/src/lib/portal/metric-stats.test.tssrc/frontend/src/lib/portal/metric-stats.tssrc/frontend/src/lib/portal/portal-nav.tssrc/frontend/src/lib/portal/portal-routes.test.tssrc/frontend/src/lib/portal/portal-routes.tssrc/frontend/src/lib/portal/portal-store.test.tssrc/frontend/src/lib/portal/portal-store.tssrc/frontend/src/lib/portal/route-scope-sync.tssrc/frontend/src/lib/portal/use-active-zone.tssrc/frontend/src/lib/portal/use-cohort-label.tssrc/frontend/src/lib/portal/use-org-scope.test.tssrc/frontend/src/lib/portal/use-org-scope.tssrc/frontend/src/lib/portal/use-viewer-is-manager.tssrc/frontend/src/routes/__root.tsxsrc/frontend/src/routes/portal.test.tsxsrc/frontend/src/routes/portal.tsxsrc/frontend/src/test/portal-router.tsx
💤 Files with no reviewable changes (1)
- src/frontend/src/components/portal/shell-layout.test.tsx
🚧 Files skipped from review as they are similar to previous changes (30)
- src/frontend/src/lib/format.ts
- src/frontend/src/routes/portal.tsx
- src/frontend/src/components/portal/employees-view.test.tsx
- src/frontend/src/components/portal/manage-view.test.tsx
- src/frontend/src/components/portal/people-view.tsx
- src/frontend/src/lib/portal/portal-store.test.ts
- src/frontend/src/components/app-sidebar.tsx
- src/frontend/src/components/portal/slice-select.tsx
- src/frontend/src/lib/portal/use-active-zone.ts
- src/frontend/src/components/widgets/period-selector-bar.tsx
- src/frontend/src/lib/portal/use-viewer-is-manager.ts
- src/frontend/src/routes/__root.tsx
- src/frontend/src/test/portal-router.tsx
- src/frontend/src/lib/portal/event-histogram.ts
- src/frontend/src/components/portal/section-trend.tsx
- src/frontend/src/hooks/use-portal-period.ts
- src/frontend/src/components/portal/attention-list.test.tsx
- src/frontend/src/components/portal/zone-content.tsx
- src/frontend/src/components/org-tree.tsx
- src/frontend/src/lib/portal/use-org-scope.ts
- src/frontend/src/lib/portal/use-cohort-label.ts
- src/frontend/src/lib/insight/attention-flags.ts
- src/frontend/src/components/portal/portal-shell.test.tsx
- src/frontend/src/lib/portal/use-org-scope.test.ts
- src/frontend/src/lib/portal/portal-store.ts
- src/frontend/src/lib/portal/metric-stats.test.ts
- src/frontend/src/components/portal/context-pane.tsx
- src/frontend/src/components/portal/ai-cost-view.tsx
- src/frontend/src/lib/portal/metric-stats.ts
- src/frontend/src/components/portal/domain-lens-view.tsx
| let lastSynced: string | null = null; | ||
|
|
||
| /** | ||
| * True at most once per person: the caller may sync the scope, and the guard | ||
| * records that it did. | ||
| */ | ||
| export function claimScopeSync(personId: string): boolean { | ||
| if (!personId || lastSynced === personId) return false; | ||
| lastSynced = personId; | ||
| return true; | ||
| } | ||
|
|
||
| /** Tests only — see the note above. */ | ||
| export function resetRouteScopeSync(): void { | ||
| lastSynced = null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Track every synchronized person.
lastSynced only remembers the latest person. For A → B → A, claimScopeSync("A") returns true twice. The second visit can overwrite a scope that the user selected after the first visit.
Use a Set<string> for synchronized person IDs. Clear the set in resetRouteScopeSync. Add a test for A → B → A.
Proposed fix
-let lastSynced: string | null = null;
+const syncedPeople = new Set<string>();
export function claimScopeSync(personId: string): boolean {
- if (!personId || lastSynced === personId) return false;
- lastSynced = personId;
+ if (!personId || syncedPeople.has(personId)) return false;
+ syncedPeople.add(personId);
return true;
}
export function resetRouteScopeSync(): void {
- lastSynced = null;
+ syncedPeople.clear();
}📝 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.
| let lastSynced: string | null = null; | |
| /** | |
| * True at most once per person: the caller may sync the scope, and the guard | |
| * records that it did. | |
| */ | |
| export function claimScopeSync(personId: string): boolean { | |
| if (!personId || lastSynced === personId) return false; | |
| lastSynced = personId; | |
| return true; | |
| } | |
| /** Tests only — see the note above. */ | |
| export function resetRouteScopeSync(): void { | |
| lastSynced = null; | |
| } | |
| let syncedPeople = new Set<string>(); | |
| /** | |
| * True at most once per person: the caller may sync the scope, and the guard | |
| * records that it did. | |
| */ | |
| export function claimScopeSync(personId: string): boolean { | |
| if (!personId || syncedPeople.has(personId)) return false; | |
| syncedPeople.add(personId); | |
| return true; | |
| } | |
| /** Tests only — see the note above. */ | |
| export function resetRouteScopeSync(): void { | |
| syncedPeople.clear(); | |
| } |
🤖 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/route-scope-sync.ts` around lines 14 - 29, The
claimScopeSync guard only remembers the latest person, allowing a previously
synchronized person to be claimed again. Replace lastSynced with a Set<string>
of synchronized person IDs, have claimScopeSync reject any ID already in the set
and record new IDs, and clear the set in resetRouteScopeSync. Add coverage for
the A → B → A sequence, verifying the final A claim returns false.
…-sync rule Second review round on the monorepo PR, plus docstrings where they carry something. - use-portal-period.test cleared localStorage but not the in-memory period store next to it, so the first test's "month" became the default every later test inherited when the URL named no period. - The route→scope guard now has its own test, including the A → B → A case the review flagged. Keeping only the LATEST person is deliberate rather than an oversight: arriving at a person's team is a navigation, so a reader who visits A, re-scopes, looks at B and clicks back to A's team has to get A's roster. Remembering every person ever seen would leave the scope on B while the route said A — an address disagreeing with the screen, which is the thing this migration set out to remove. The module comment says so now, and four cases pin it. - Docstrings on the exported symbols where a line of prose adds information: what an absent URL param means, what the scope picker's shapes are, why a trend point omits a series key rather than zeroing it, and the guard/store contracts. Deliberately not on the API-client types that mirror the server DTO field-for-field, nor on upstream code this branch only touched — a docstring restating the symbol's own name is noise that ages badly. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
Both taken care of in Period preference between tests — fixed. Right, and it was my own test's leak: Tracking every synchronized person — not taken. Keeping only the latest person is the intended rule, and a Arriving at a person's team is a navigation, not a remount. With a set: a reader opens A's team, re-scopes to something else from the topbar, looks at B's team, then clicks back to A's team — and gets B's roster, because A was already claimed. The address says A and the screen shows B. That disagreement is the thing this whole migration set out to remove. The guard's job is narrower than "sync each person once ever": absorb re-renders and remounts of the same person, so a route sync never fights a scope the reader picked. Where you are right is that nothing said so and nothing tested it, which is why it reads as an oversight. On the docstring coverage check: added docstrings where a line of prose carries information — what an absent URL param means, the scope picker's shapes, why a trend point omits a series key instead of zeroing it, the guard and store contracts. That takes the diff from 52% to 62%. I stopped there rather than reaching 80%: the remaining gap is mostly API-client types that mirror the server DTO field-for-field (
|
4 participants
What
An organizational analytics portal — a manager-first shell over the existing metric system — gated behind the
insight.portallocalStorage flag (default off; nothing changes for current users). New code lives undersrc/frontend/src/components/portal/andsrc/frontend/src/lib/portal/, plus a few shared libs extracted with tests.Turn it on:
localStorage.setItem("insight.portal", "true")and reload (the flag is compared against the literal"true").This replaces constructorfabric/insight-front#221, re-based onto the monorepo now that the frontend lives at
src/frontend. Same tree, re-cut as five logical commits (charts, metric grammar, URL navigation, shell, zone views); the granular per-task history stays ondzarlax/insight-front-cf@feat/portal-shellfor anyone who wants the step-by-step.Why
The current dashboard is person-centric. Managers need the opposite entry point: the org first (their subtree), domains second, people last. The portal is that shell — and it doubles as a testbed for a uniform metric grammar, so every tab renders honestly from whatever connector set a tenant actually has (validated against two real datasets with very different coverage).
Structure
Scope(any node of the viewer's subtree plus a direct-only switch — the permission boundary is the viewer's own tree),Slice(dimensions discovered from identity attributes; single-valued and near-unique ones are hidden automatically), and the period selector. The bar is sticky, so the three global controls stay reachable from anywhere in a long dashboard.Everything is in the URL
Navigation state used to live in memory. A reload reset the screen, Back went somewhere unrelated, and a link or a bookmark could not reproduce a view — the main objection in review on the previous PR.
Now:
/portal?zone=&item=&dir=&lens=&scope=&direct=&slice=&period=&from=&to=plus/ic/$person/personal|team. Only true reader preferences (is the portal on, are planned sections shown) remain in localStorage.replace, so Back never steps into a half-built address.?zone=— the URL cannot contradict itself.Keyed on person ids
Person keys are canonical person ids everywhere: the
/ic/$person/*route segment (guarded byisPersonId, which redirects a pre-cutover email URL instead of sending it to the API as an unactionable 400),?scope=, the roster maps, and the metric entity ids. No email-to-id bridge — the frontend and metrics are on ids on dev, and a translation layer would outlive its reason to exist.Two consequences worth naming: cohorts are keyed by id, so a person with no email still belongs to one and still appears in the employees directory; and the manager step in the person header uses
parent_person_id.The metric grammar (the core of the PR)
Directions and Overview render through ONE config-driven renderer (
DomainLensView) over a typed section library (SectionSpec): headline, stat-tiles, trend, distribution, concentration, composition, participation, event-histogram, attention, direction-cards, coverage-radar. Registries (DIRECTION_LENSES,OVERVIEW_ITEMS) declare every tab; invariant tests pin nav/registry coverage both ways and the metric-key budget.Rules the renderer enforces everywhere:
direction; ratios and medians are never summed;entityObservedreads the peer view's nulls, which the backend guarantees viajoin_use_nulls = 1, so zero-filled sums do not count as data);direction, but nothing is labelled good or bad: the unified metric keys carry no threshold rows, and hardcoding good/warn in a component would turn a product decision into invisible frontend trivia. Status lights up the moment the catalog carries values ([pres] Declarative metric registry (single YAML) #1974).Charts
Line charts were configured per call site, so the same reading looked different depending on the screen.
ChartLinenow owns the defaults: linear interpolation (a chart must not draw a value nobody measured), adaptive dots (dropped above 31 points, but an isolated reading between two nulls is always marked so a single point is never invisible), and gaps that read as gaps.ChartYAxisabbreviates ticks from 10k — the point where four digits still fit the gutter and five clip — and stays exact below it, because a tick sits on a gridline and "2.3k" for 2250 is a label that lies.Three layout tiers
useShellLayoutresolves phone (<768) / narrow (768-1023) / wide (>=1024) in one place: the rail is a drawer on a phone, collapsible on narrow, pinned on wide, and pane state resets when the tier changes instead of restoring a mode that no longer exists. On a phone the drawer shows a one-row zone switcher and folds settings behind a single row — the expanded list pushed the sections below the fold, which is the content the reader opened the drawer to reach.Tests
785 unit tests over the new code, semantics-first rather than snapshot-first:
src/test/portal-routeris a reactive router fake so navigation is asserted as real hrefs and recorded navigations;src/test/identitymints valid person UUIDs from short labels, so fixtures stay readable without drifting from the real key shape.Verified in the ported tree:
pnpm typecheckclean,eslint --max-warnings 0clean, 785 unit tests pass. The two Storybook browser suites are unchanged by this PR.Verified live
Walked every zone and tab at org scope (150+ people) and at narrowed scopes on two real datasets with different connector sets: full coverage on one; the other missing task and git freshness — every gap rendered as an explicit not-ingested state instead of fabricated zeros, which is exactly the behaviour this PR is after.
Out of scope / follow-ups
/v1/admin/metric-thresholds(blocked on values existing: [pres] Declarative metric registry (single YAML) #1974, CONFIGURE 8: [EPIC] Admin configuration console — roles, mappings, thresholds & install specifics #1682).repository,project,file_extension,change_type).All five commits are signed off (DCO).
Summary by CodeRabbit