feat(frontend): the cohort choice follows the catalog, not the roster - #2335
Conversation
The portal decided which comparisons it could offer by walking the viewer's roster and keeping attributes that took more than one value. That can only work for a viewer who can see other people, and identity serves a viewer their own subtree: someone with no reports has a roster of one person, no attribute has a second value, and no comparison can be offered at all. That is most readers, and the control still showed a single option — which reads as a setting they chose rather than as the only thing available. The comparison itself was never the problem: the peer view computes it server-side, within the person's organization unit. What was missing is the choice. `cohortOptions` is now the one place that decides, from two sources in order: - the server's catalog of attributes a comparison may be built on, which decides from governed policy rather than from whoever happens to be visible (epic constructorfabric#2028, design `docs/domain/person-attributes/specs/DESIGN.md` §3.3); - the roster walk, until that lands, so nothing regresses for a viewer who has reports today. They are not merged: a locally derived attribute standing beside a governed one is indistinguishable to the reader. When neither yields anything the control says so instead of offering one option. The catalog is read in a single function against an optional field, so an installation on either side of that change works, and agreeing on the field name later is a one-line change rather than a search through the portal. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
Warning Review limit reached
Next review available in: 43 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 (5)
📝 WalkthroughWalkthroughChangesCohort selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PortalTopBar
participant MetricDefinitions
participant cohortOptions
participant SliceSelect
PortalTopBar->>MetricDefinitions: fetch complete metric-definition response
MetricDefinitions-->>PortalTopBar: return cached response
PortalTopBar->>cohortOptions: provide catalog and roster attributes
cohortOptions-->>PortalTopBar: return cohort dimensions and source
PortalTopBar->>SliceSelect: provide available dimensions
SliceSelect->>SliceSelect: disable trigger when no dimensions exist
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/components/portal/portal-topbar.tsx`:
- Around line 40-50: Update the portal topbar flow around
useMetricDefinitionsResponse, cohortOptions, and usePortalSlice to reconcile the
stored portal.slice whenever catalog dimensions replace roster dimensions: clear
the stored slice before SliceSelect reads it when its selected dimension is no
longer present in the resulting dims. Add a catalog-transition test that begins
with a roster-only dimension and verifies it is removed from the stored slice
after catalog attributes load.
🪄 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: f2412972-62f7-4187-b96b-a6f2daf37d64
📒 Files selected for processing (7)
src/frontend/src/components/portal/portal-topbar.tsxsrc/frontend/src/components/portal/shell-layout.test.tsxsrc/frontend/src/components/portal/slice-select.test.tsxsrc/frontend/src/components/portal/slice-select.tsxsrc/frontend/src/lib/portal/cohort-options.test.tssrc/frontend/src/lib/portal/cohort-options.tssrc/frontend/src/queries/metric-definitions.ts
| // The server's catalog when it has one, the viewer's roster until then — | ||
| // decided in `cohortOptions`, not here. See its note for why the roster | ||
| // alone cannot serve a reader without reports. | ||
| const { data: definitions } = useMetricDefinitionsResponse(); | ||
| const { dims } = useMemo( | ||
| () => | ||
| cohortOptions( | ||
| catalogAttributes(definitions), | ||
| collectRosterAttrs(tree, normalizePersonId).values() | ||
| ), | ||
| [definitions, tree] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'portal-topbar\.tsx|slice|portal' src/frontend/src | sed 's#^\./##' | head -200
echo
echo "== topbar outline =="
ast-grep outline src/frontend/src/components/portal/portal-topbar.tsx --view expanded || true
echo
echo "== relevant topbar section =="
sed -n '1,220p' src/frontend/src/components/portal/portal-topbar.tsx
echo
echo "== useMetricDefinitionsResponse and cohortOptions/catalog definitions =="
rg -n "useMetricDefinitionsResponse|cohortOptions|catalogAttributes|collectRosterAttrs|SliceSelect|usePortalSlice" src/frontend/src -SRepository: constructorfabric/insight
Length of output: 14264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cohort-options.ts =="
sed -n '1,180p' src/frontend/src/lib/portal/cohort-options.ts
echo
echo "== portal-nav.ts =="
sed -n '1,180p' src/frontend/src/lib/portal/portal-nav.ts
echo
echo "== slice-select.tsx =="
sed -n '1,140p' src/frontend/src/components/portal/slice-select.tsx
echo
echo "== portal-shell.test.tsx relevant section =="
sed -n '240,330p' src/frontend/src/components/portal/portal-shell.test.tsx
echo
echo "== slices.test.ts relevant section =="
sed -n '1,180p' src/frontend/src/lib/insight/slices.test.ts
echo
echo "== slices.ts relevant section =="
sed -n '1,190p' src/frontend/src/lib/insight/slices.ts
echo
echo "== all usePortalSlice consumers =="
rg -n "usePortalSlice\\(" src/frontend/src -S
echo
echo "== all portal-nav action usages/state store =="
rg -n "setPortalSlice|usePortalNavActions|slice" src/frontend/src/lib/portal src/frontend/src/components/portal -SRepository: constructorfabric/insight
Length of output: 42577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== portal-search.ts =="
sed -n '1,130p' src/frontend/src/lib/portal/portal-search.ts
echo
echo "== portal-shell.tsx layout section =="
sed -n '1,180p' src/frontend/src/components/portal/portal-shell.tsx
echo
echo "== team-state-view slice cohort section =="
sed -n '1,130p' src/frontend/src/components/portal/team-state-view.tsx
sed -n '70,95p' src/frontend/src/components/portal/team-state-view.tsx
echo
echo "== behavioral probe: invalid catalog key as active slice =="
node - <<'JS'
const TEAM_KEY = "__team__";
const TEAM_SLICE = { key: TEAM_KEY, label: "Team (all)" };
function currentLabel(slice, dims) {
const all = [TEAM_SLICE, ...dims];
const current = slice || TEAM_KEY;
const value = all.some((d) => d.key === current) ? current : TEAM_KEY;
return all.find((d) => d.key === value)?.label ?? "Team (all)";
}
function nextUrl(slice, dims) {
const all = [TEAM_SLICE, ...dims];
const current = slice || TEAM_KEY;
const value = all.some((d) => d.key === current) ? current : TEAM_KEY;
return value && value !== TEAM_KEY ? value : "";
}
const rosterD = [{ key: "Division", label: "Division" }];
const catalogC = [{ key: "company", label: "Company" }];
console.log({
beforeUrl: nextUrl("division", rosterD),
beforeLabel: currentLabel("division", rosterD),
afterLabel: currentLabel("division", catalogC),
afterUrl: nextUrl("division", catalogC),
});
JS
echo
echo "== existing transition tests mentioning catalog/cohort-options =="
rg -n "transit|transition|catalog|cohortOptions" src/frontend/src -SRepository: constructorfabric/insight
Length of output: 5393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== portal-layout.tsx relevant section =="
sed -n '1,140p' src/frontend/src/components/portal/portal-layout.tsx
echo
echo "== find test files with SliceSelect and portal slice behavior =="
sed -n '240,335p' src/frontend/src/components/portal/portal-layout.test.tsx 2>/dev/null || true
rg -n "SliceSelect|portalSlice|slice" src/frontend/src -S | head -200
echo
echo "== coherency check for all usePortalSlice consumers =="
python3 - <<'PY'
from pathlib import Path
import re
search = re.compile(r'cohortKey\(\s*([^,]+),\s*usePortalSlice\(\)\s*\)')
for p in Path('src/frontend/src').rglob('*.tsx'):
text = p.read_text()
if 'usePortalSlice' in text:
for i, line in enumerate(text.splitlines(), 1):
if 'usePortalSlice(' in line:
before = '<'.join(line.rsplit('<', 2)[:2]) if '<' in line else line
coord = f"{p}:{i}"
print(f"{coord}: {before.strip()}")
PY
echo
echo "== deterministic invalid-slice probe =="
python3 - <<'PY'
TEAM_KEY = "__team__"
def current_label(slice_key, dims):
all_dims = [{"key": TEAM_KEY, "label": "Team (all)"}] + list(dims)
current = slice_key or TEAM_KEY
value = current if any(d["key"] == current for d in all_dims) else TEAM_KEY
d = next((d for d in all_dims if d["key"] == value), None)
return ("Team (all)" if not d else d["label"]) if value is not None else "Team (all)", value
def next_url(slice_key, dims):
_, value = current_label(slice_key, dims)
return value if value and value != TEAM_KEY else ""
roster = [{"key": "division", "label": "Division"}]
catalog = [{"key": "company", "label": "Company"}]
selected = "division"
print("selected_before_url", next_url(selected, roster))
print("selected_after_url", next_url(selected, catalog))
print("selected_after_label", current_label(selected, catalog))
print("url_equals_label", next_url(selected, catalog) == (current_label(selected, catalog)[1] or ""))
PYRepository: constructorfabric/insight
Length of output: 27398
Reconcile portal.slice when catalog dims arrive.
If the URL or a roster selection specifies a dimension that cohortOptions drops once catalog attributes load, SliceSelect falls back to Team (all), but usePortalSlice() still returns the old dimension. Clear the stored slice in this effect before the control reads it, and add a catalog-transition test that starts from a roster dimension removed by the catalog.
🤖 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-topbar.tsx` around lines 40 - 50,
Update the portal topbar flow around useMetricDefinitionsResponse,
cohortOptions, and usePortalSlice to reconcile the stored portal.slice whenever
catalog dimensions replace roster dimensions: clear the stored slice before
SliceSelect reads it when its selected dimension is no longer present in the
resulting dims. Add a catalog-transition test that begins with a roster-only
dimension and verifies it is removed from the stored slice after catalog
attributes load.
`cohortOptions` is tested on its own and so is the control; what neither covered is that the bar actually asks the catalog and hands the answer down — the one place a wiring mistake could hide, since the layout test that renders this bar stubs every query it makes. This test therefore runs the real query hook against a real QueryClient and stubs only the network, and asserts all three outcomes: the catalog is followed when the response carries one, it wins over what the roster would have offered, and the roster still serves until a catalog exists. Verified by mutation: making the bar ignore the catalog fails the first two cases and leaves the third passing, which is the behaviour the test claims to pin. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
Review finding: a stored dimension the options no longer contain left the two readers disagreeing. `SliceSelect` falls back to "Team (all)" when its value is not among the offered ones, but `usePersonCohort` read the stored value directly — so with a link carrying a dimension the catalog does not offer, the control said "Team (all)" while the comparison was still built by the old attribute. The screen said one thing and did another. `useCohortOptions` now owns both the options and which selection is in effect, and the control and the cohort builder read the same answer. Deriving it once removes the disagreement rather than correcting it after the fact, and needs no effect writing state back — a stored value is simply not applied while it is not on offer. While the sources are still answering, the stored value is kept: a shared link carrying `?slice=` must survive the moment before the options arrive. The cohort hook's own test now runs the real catalog query against a real query client and stubs only the request, so a cohort built from an attribute the catalog does not offer fails here. Verified by mutation: restoring the direct read of the stored value fails exactly the new case and leaves the rest passing. Signed-off-by: Alexey Panfilov <alexey.panfilov@constructor.tech>
|
Taken, in
Fixed one level up rather than by clearing state in an effect. The catalog-transition test you asked for is in the cohort hook's own suite, 941 tests pass, |
2 participants
Problem
The portal decides which comparisons it can offer by walking the viewer's
roster and keeping the attributes that take more than one value
(
collectRosterAttrs+availableSlices). That only works for a viewer whocan see other people, and identity serves a viewer their own subtree
(
use-org-scope.ts, "permission boundary"). A viewer with no direct reportstherefore has a roster of one person, no attribute reaches the two-distinct
-values threshold, and no re-grouping is possible at all. That is most readers,
not an edge case.
The comparison itself was never the problem — the peer view computes it
server-side within the person's organization unit. What is missing is the
choice. Meanwhile the control still rendered a single option, which reads as a
setting the reader chose rather than the only thing available.
Change
cohortOptionsbecomes the one place that decides, from two sources in order:shape epic GRAPH 7: [EPIC] Cohorting is a GROUP BY over identity attributes #2028 introduces, decided from governed policy (Discovered person attributes have no tenant-governed policy #2284, Attribute policy and account assignment are not readable from the analytical store #2312)
rather than from whoever happens to be visible;
has reports today.
They are not merged. A locally derived attribute standing beside a governed one
is indistinguishable to the reader, who cannot tell which of the two they
picked. When neither source yields anything the control states the fallback
instead of offering one option.
Working now, and working later
The catalog is read in a single function against an optional field, so an
installation on either side of the change behaves correctly: absent today,
followed once present, with no second code path to keep in step.
One thing needs confirming:
docs/domain/person-attributes/specs/DESIGN.md§3.3 says
GET /v1/metric-definitionsgains "the active allowed-attributecatalog", but does not pin the field name or entry shape. This reads
comparison_attributes: [{ id, label }]as a placeholder, in one function, soagreeing on the real name is a one-line change here rather than a search
through the portal. Happy to align it with whatever #2285 / #2313 settle on.
Notes
cohort_keyrows and a document listing them. Closed as superseded: thedesign answers both asks differently, and extending
cohort_keywould buildon the path being retired.
what it says when it has none.
Verification
937 tests pass,
tsc -bclean, eslint clean. New tests cover the catalog path,the roster fallback, the empty case, a response without the field, and entries
missing an id or label. Checked against a running stand at three widths.
Summary by CodeRabbit
New Features
Bug Fixes
Tests