diff --git a/src/frontend/src/components/portal/portal-topbar.test.tsx b/src/frontend/src/components/portal/portal-topbar.test.tsx new file mode 100644 index 000000000..17a9f8485 --- /dev/null +++ b/src/frontend/src/components/portal/portal-topbar.test.tsx @@ -0,0 +1,129 @@ +// @vitest-environment jsdom +/** + * The seam between the catalog and the control. + * + * `cohortOptions` is tested on its own and so is `SliceSelect`; what neither + * covers is that the bar actually asks the catalog and hands the answer down. + * This test therefore runs the real query hook against a real QueryClient and + * stubs only the network — so a wiring mistake here fails rather than hiding + * behind a stubbed hook. + */ +vi.mock("@tanstack/react-router", async () => { + const { portalRouterMock } = await import("@/test/portal-router"); + return portalRouterMock(); +}); + +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { SidebarProvider } from "@/components/ui/sidebar"; +import { render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import type { MetricDefinitionListResponse } from "@/api/metric-definitions-client"; +import { identityPerson, pid } from "@/test/identity"; + +const mocks = vi.hoisted(() => ({ + definitions: { metrics: [] } as MetricDefinitionListResponse, +})); + +// Only the network is stubbed; the query hook itself is the real one. +vi.mock("@/api/metric-definitions-client", () => ({ + listMetricDefinitions: () => Promise.resolve(mocks.definitions), +})); +vi.mock("@/queries/ic-dashboard", () => ({ + // A viewer with two reports who differ by division: enough for the roster + // walk to offer something, so a catalog winning is a real preference and + // not the absence of an alternative. + useIcPerson: () => ({ + data: identityPerson("boss", { + person_id: pid("boss"), + division: "Alpha", + subordinates: [ + identityPerson("one", { person_id: pid("one"), division: "Alpha" }), + identityPerson("two", { person_id: pid("two"), division: "Beta" }), + ], + }), + }), +})); +vi.mock("@/auth", () => ({ useViewer: () => ({ personId: pid("boss") }) })); +vi.mock("@/hooks/use-portal-period", () => ({ + usePortalPeriod: () => ({ + period: "month", + customRange: null, + setPeriod: vi.fn(), + setCustomRange: vi.fn(), + }), +})); +vi.mock("@/components/portal/scope-select", () => ({ + ScopeSelect: () =>
, +})); +vi.mock("@/components/widgets/period-selector-bar", () => ({ + PeriodSelectorBar: () =>
, +})); +// Stands in for the control so the test can read what it was given. +vi.mock("@/components/portal/slice-select", () => ({ + SliceSelect: ({ dims }: { dims: { key: string; label: string }[] }) => ( +
{dims.map((d) => d.key).join(",")}
+ ), +})); + +import { PortalTopBar } from "./portal-topbar"; + +function bar() { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + return render( + + + + + , + ); +} + +beforeEach(() => { + window.matchMedia ??= ((query: string) => ({ + matches: false, + media: query, + onchange: null, + addEventListener: () => {}, + removeEventListener: () => {}, + addListener: () => {}, + removeListener: () => {}, + dispatchEvent: () => false, + })) as unknown as typeof window.matchMedia; + mocks.definitions = { metrics: [] }; +}); + +describe("PortalTopBar", () => { + it("offers what the catalog allows once the response carries it", async () => { + mocks.definitions = { + metrics: [], + comparison_attributes: [{ id: "job_title", label: "Title" }], + } as MetricDefinitionListResponse; + bar(); + await waitFor(() => + expect(screen.getByTestId("dims")).toHaveTextContent("job_title"), + ); + }); + + it("prefers the catalog over what the roster happens to support", async () => { + // The roster here would offer `division`; a governed answer wins, and the + // two are never shown side by side. + mocks.definitions = { + metrics: [], + comparison_attributes: [{ id: "job_title", label: "Title" }], + } as MetricDefinitionListResponse; + bar(); + await waitFor(() => + expect(screen.getByTestId("dims")).not.toHaveTextContent("division"), + ); + }); + + it("falls back to the roster while no catalog exists", async () => { + bar(); + await waitFor(() => + expect(screen.getByTestId("dims")).toHaveTextContent("division"), + ); + }); +}); diff --git a/src/frontend/src/components/portal/portal-topbar.tsx b/src/frontend/src/components/portal/portal-topbar.tsx index 1c276b86c..4cc12b74f 100644 --- a/src/frontend/src/components/portal/portal-topbar.tsx +++ b/src/frontend/src/components/portal/portal-topbar.tsx @@ -1,7 +1,5 @@ import { HelpCircle } from "lucide-react"; -import { useMemo } from "react"; -import { useViewer } from "@/auth"; import { ScopeSelect } from "@/components/portal/scope-select"; import { SliceSelect } from "@/components/portal/slice-select"; import { SidebarTrigger } from "@/components/ui/sidebar"; @@ -13,11 +11,8 @@ import { } from "@/components/ui/tooltip"; import { PeriodSelectorBar } from "@/components/widgets/period-selector-bar"; import { usePortalPeriod } from "@/hooks/use-portal-period"; -import { availableSlices } from "@/lib/insight/slices"; -import { collectRosterAttrs } from "@/lib/insight/slices"; -import { normalizePersonId } from "@/lib/metrics/entity"; import { useActiveZone } from "@/lib/portal/use-active-zone"; -import { useIcPerson } from "@/queries/ic-dashboard"; +import { useCohortOptions } from "@/lib/portal/use-cohort-options"; /** * Global portal bar — the two cross-cutting controls live here so every zone @@ -28,18 +23,16 @@ import { useIcPerson } from "@/queries/ic-dashboard"; */ export function PortalTopBar() { const { period, customRange, setPeriod, setCustomRange } = usePortalPeriod(); - const { personId } = useViewer(); // The Person zone is about ONE person, and it reads nothing from the org // scope. Leaving the control up meant the bar named a different person than // the page — a reader saw two names and had to work out which one the // numbers belonged to. const { activeZone } = useActiveZone(); const scoped = activeZone !== "person"; - const tree = useIcPerson(personId ?? "").data ?? null; - const dims = useMemo( - () => availableSlices(collectRosterAttrs(tree, normalizePersonId).values()), - [tree], - ); + // The server's catalog when it has one, the viewer's roster until then — + // decided in `useCohortOptions`, which also owns which selection is in + // effect so this control and the comparison cannot disagree. + const { dims } = useCohortOptions(); return ( // Sticky to the scroll container (`SidebarInset` owns the overflow): scope, diff --git a/src/frontend/src/components/portal/shell-layout.test.tsx b/src/frontend/src/components/portal/shell-layout.test.tsx index b2b980df4..557c1c6f9 100644 --- a/src/frontend/src/components/portal/shell-layout.test.tsx +++ b/src/frontend/src/components/portal/shell-layout.test.tsx @@ -46,6 +46,11 @@ vi.mock("@/components/widgets/period-selector-bar", () => ({ vi.mock("@/queries/ic-dashboard", () => ({ useIcPerson: () => ({ data: null }), })); +// The topbar asks the catalog which attributes a comparison may use; this +// test is about the shell's layout, not about that answer. +vi.mock("@/queries/metric-definitions", () => ({ + useMetricDefinitionsResponse: () => ({ data: undefined }), +})); vi.mock("@/hooks/use-portal-period", () => ({ usePortalPeriod: () => ({ period: "month", diff --git a/src/frontend/src/components/portal/slice-select.test.tsx b/src/frontend/src/components/portal/slice-select.test.tsx new file mode 100644 index 000000000..25fb97457 --- /dev/null +++ b/src/frontend/src/components/portal/slice-select.test.tsx @@ -0,0 +1,40 @@ +// @vitest-environment jsdom +/** + * What the control says when there is nothing to choose. + * + * Slices are discovered by enumerating people and grouping them by one of + * their attributes, and identity serves a viewer only their own subtree — so a + * viewer with no reports has a roster of one person and no attribute with a + * second value. Comparisons still happen (the peer view compares within the + * organization unit, server-side); the choice is what is missing, and a + * control offering exactly one option claims the reader chose it. + */ +import { render, screen } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +vi.mock("@/lib/portal/portal-nav", () => ({ + usePortalNavActions: () => ({ setSlice: vi.fn() }), + usePortalSlice: () => "", +})); + +import { SliceSelect } from "./slice-select"; + +describe("SliceSelect", () => { + it("does not offer a choice when the roster supports none", () => { + render(); + const trigger = screen.getByLabelText("Cohort"); + expect(trigger).toBeDisabled(); + // And says why, rather than leaving a dead control. + expect(trigger).toHaveAttribute( + "title", + expect.stringContaining("organization unit"), + ); + }); + + it("is a live control as soon as the roster supports one dimension", () => { + render(); + const trigger = screen.getByLabelText("Cohort"); + expect(trigger).not.toBeDisabled(); + expect(trigger).not.toHaveAttribute("title"); + }); +}); diff --git a/src/frontend/src/components/portal/slice-select.tsx b/src/frontend/src/components/portal/slice-select.tsx index ab15813ca..f55a4eb1c 100644 --- a/src/frontend/src/components/portal/slice-select.tsx +++ b/src/frontend/src/components/portal/slice-select.tsx @@ -9,10 +9,8 @@ import { } from "@/components/ui/select"; import type { SliceDim } from "@/lib/insight/slices"; import { PLANNED_SLICES } from "@/lib/insight/slices"; -import { - usePortalNavActions, - usePortalSlice, -} from "@/lib/portal/portal-nav"; +import { NO_COHORT_REASON } from "@/lib/portal/cohort-options"; +import { usePortalNavActions, usePortalSlice } from "@/lib/portal/portal-nav"; /** * "No slice" — the whole roster is one cohort and views stay per-person. The @@ -35,6 +33,17 @@ export function SliceSelect({ dims }: { dims: SliceDim[] }) { const { setSlice } = usePortalNavActions(); const slice = usePortalSlice(); const all = [TEAM_SLICE, ...dims, ...PLANNED_SLICES]; + // Slices are discovered by enumerating people and grouping them by an + // attribute, so a viewer whose roster holds only themselves has nothing to + // group: identity serves a viewer their own subtree, and an individual + // contributor's subtree is one person. Their attributes are all there — + // there is simply no second value for any of them. + // + // The comparisons on screen still happen: the peer view compares within the + // person's organization unit, decided server-side. What is missing is the + // CHOICE, and a control offering exactly one option states the opposite — + // it reads as a setting the reader picked, not as the only thing available. + const hasChoice = dims.length + PLANNED_SLICES.length > 0; const current = slice || TEAM_KEY; const value = all.some((d) => d.key === current) ? current : TEAM_KEY; const label = all.find((d) => d.key === value)?.label ?? "Team (all)"; @@ -46,7 +55,13 @@ export function SliceSelect({ dims }: { dims: SliceDim[] }) { {/* The trigger carries the VALUE only; the word "Cohort" labels the control from outside, next to the tooltip that explains it. Inside, it read as part of the value and repeated on every option list. */} - + {label} diff --git a/src/frontend/src/lib/portal/cohort-options.test.ts b/src/frontend/src/lib/portal/cohort-options.test.ts new file mode 100644 index 000000000..beb4d2c2f --- /dev/null +++ b/src/frontend/src/lib/portal/cohort-options.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it } from "vitest"; + +import type { MetricDefinitionListResponse } from "@/api/metric-definitions-client"; +import type { SliceAttr } from "@/lib/insight/slices"; +import { catalogAttributes, cohortOptions } from "./cohort-options"; + +/** A roster row as `collectRosterAttrs` produces it. */ +const person = (division: string): Record => ({ + division: { key: "division", label: "Division", value: division }, +}); + +const response = (extra: object = {}): MetricDefinitionListResponse => + ({ metrics: [], ...extra }) as MetricDefinitionListResponse; + +describe("catalogAttributes", () => { + it("reads the attributes a response carries", () => { + expect( + catalogAttributes( + response({ + comparison_attributes: [ + { id: "job_title", label: "Title" }, + { id: "division", label: "Division" }, + ], + }), + ), + ).toEqual([ + { id: "job_title", label: "Title" }, + { id: "division", label: "Division" }, + ]); + }); + + it("treats a response without one as a normal state", () => { + // This client runs against installations on both sides of the change, so + // the field's absence is expected rather than an error to surface. + expect(catalogAttributes(response())).toEqual([]); + expect(catalogAttributes(undefined)).toEqual([]); + }); + + it("drops entries it cannot render or send back", () => { + expect( + catalogAttributes( + response({ + comparison_attributes: [ + { id: "ok", label: "Fine" }, + { id: "no_label" }, + { label: "no id" }, + null, + "not an object", + ], + }), + ), + ).toEqual([{ id: "ok", label: "Fine" }]); + }); +}); + +describe("cohortOptions", () => { + it("follows the catalog when there is one", () => { + const options = cohortOptions( + [{ id: "job_title", label: "Title" }], + [person("Alpha"), person("Alpha"), person("Beta")], + ); + expect(options.source).toBe("catalog"); + expect(options.dims).toEqual([{ key: "job_title", label: "Title" }]); + }); + + it("does not mix the two sources", () => { + // A locally-derived attribute standing beside a governed one is + // indistinguishable to the reader, who cannot tell which they picked. + const options = cohortOptions( + [{ id: "job_title", label: "Title" }], + [person("Alpha"), person("Alpha"), person("Beta")], + ); + expect(options.dims.map((d) => d.key)).not.toContain("division"); + }); + + it("falls back to the roster until the catalog exists", () => { + // Three people, two divisions: an attribute that splits them without + // being near-unique, which `availableSlices` reads as an identifier. + const options = cohortOptions( + [], + [person("Alpha"), person("Alpha"), person("Beta")], + ); + expect(options.source).toBe("roster"); + expect(options.dims.map((d) => d.key)).toEqual(["division"]); + }); + + it("reports having nothing when neither source can offer anything", () => { + // A viewer with no reports: one person in the roster, so no attribute + // takes a second value and none qualifies. + const options = cohortOptions([], [person("Alpha")]); + expect(options.source).toBe("none"); + expect(options.dims).toEqual([]); + }); +}); diff --git a/src/frontend/src/lib/portal/cohort-options.ts b/src/frontend/src/lib/portal/cohort-options.ts new file mode 100644 index 000000000..8a18f9934 --- /dev/null +++ b/src/frontend/src/lib/portal/cohort-options.ts @@ -0,0 +1,100 @@ +import type { MetricDefinitionListResponse } from "@/api/metric-definitions-client"; +import { availableSlices, type SliceDim } from "@/lib/insight/slices"; +import type { SliceAttr } from "@/lib/insight/slices"; + +/** + * Where the cohort choices come from — one place, two sources, in priority + * order. + * + * The portal used to derive them by walking the viewer's roster and keeping + * attributes that took more than one value. That can only ever work for a + * viewer who can see other people: identity serves a viewer their own subtree, + * so someone with no reports has a roster of one person, no attribute has a + * second value, and no comparison can be offered at all — which is most + * readers. + * + * The replacement is a catalog: the server says which attributes a comparison + * may be built on, having decided it from governed policy rather than from + * whoever happens to be visible (epic constructorfabric/insight#2028, design + * `docs/domain/person-attributes/specs/DESIGN.md` §3.3). It rides on + * `/v1/metric-definitions`, a response the client already reads. + * + * Until that lands the roster walk stays as the fallback, so nothing regresses + * for a manager today; when the catalog appears the client follows it without + * a second code path to keep in step. + */ +export type CohortSource = "catalog" | "roster" | "none"; + +export interface CohortOptions { + dims: SliceDim[]; + /** Which of the two produced them — "none" when neither could. */ + source: CohortSource; +} + +/** + * The catalog entry this client understands. + * + * Deliberately narrow: an id to send back and a label to show. Anything else + * the server publishes about an attribute — sensitivity, how often it is + * filled in, which source it came from — is not this control's business. + */ +interface CatalogAttribute { + id: string; + label: string; +} + +/** + * Read the catalog out of a metric-definitions response, if it carries one. + * + * The field is optional on purpose: this client is expected to run against + * installations on either side of the change, so its absence is a normal + * state, not an error. Entries missing an id or a label are dropped rather + * than rendered as blanks — a nameless option cannot be chosen meaningfully. + * + * NOTE: the exact field name is not pinned by the design yet. It is read in + * this one function so that agreeing on it later is a one-line change here, + * not a search through the portal. + */ +export function catalogAttributes( + response: MetricDefinitionListResponse | undefined +): CatalogAttribute[] { + const raw = (response as { comparison_attributes?: unknown } | undefined) + ?.comparison_attributes; + if (!Array.isArray(raw)) return []; + return raw.flatMap((entry) => { + if (typeof entry !== "object" || entry === null) return []; + const { id, label } = entry as { id?: unknown; label?: unknown }; + if (typeof id !== "string" || !id) return []; + if (typeof label !== "string" || !label) return []; + return [{ id, label }]; + }); +} + +/** + * The cohort dimensions to offer, and where they came from. + * + * `roster` is only consulted when the catalog is absent — not merged with it. + * Two sources of the same list would let a locally-derived attribute appear + * beside a governed one, and the reader has no way to tell which of the two + * they picked. + */ +export function cohortOptions( + catalog: readonly CatalogAttribute[], + roster: Iterable> +): CohortOptions { + if (catalog.length > 0) { + return { + dims: catalog.map((a) => ({ key: a.id, label: a.label })), + source: "catalog", + }; + } + const dims = availableSlices(roster); + return { dims, source: dims.length > 0 ? "roster" : "none" }; +} + +/** + * Why the control has nothing to offer — shown to the reader, so it has to say + * what is true rather than name an internal state. + */ +export const NO_COHORT_REASON = + "Comparisons are made within each person's organization unit. Other cohorts are not available here yet."; diff --git a/src/frontend/src/lib/portal/portal-hooks.test.tsx b/src/frontend/src/lib/portal/portal-hooks.test.tsx index a93c668d5..93ff4c61e 100644 --- a/src/frontend/src/lib/portal/portal-hooks.test.tsx +++ b/src/frontend/src/lib/portal/portal-hooks.test.tsx @@ -6,7 +6,8 @@ * Identity/auth/router dependencies are stubbed at the module boundary; * assertions are about the derived semantics, not the wiring. */ -import { act, renderHook } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { act, renderHook, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { IdentityPerson } from "@/types/insight"; @@ -23,6 +24,10 @@ const C = "cccccccc-1111-4111-8111-111111111111"; const mocks = vi.hoisted(() => ({ personId: "11111111-1111-4111-8111-111111111111" as string | null, pathname: "/" as string, + definitions: { metrics: [] } as { metrics: unknown[] } & Record< + string, + unknown + >, ic: { data: undefined as IdentityPerson | undefined, isPending: false, @@ -36,6 +41,11 @@ vi.mock("@/auth", () => ({ useViewer: () => ({ email: "viewer@x", personId: mocks.personId }), })); vi.mock("@/queries/ic-dashboard", () => ({ useIcPerson: () => mocks.ic })); +// Only the request is stubbed; `useMetricDefinitionsResponse` itself runs, so +// a cohort built from an attribute the catalog does not offer fails here. +vi.mock("@/api/metric-definitions-client", () => ({ + listMetricDefinitions: () => Promise.resolve(mocks.definitions), +})); vi.mock("@tanstack/react-router", async () => { const { portalRouterMock } = await import("@/test/portal-router"); return portalRouterMock(); @@ -50,7 +60,7 @@ const person = ( personId: string, name: string, over: Partial = {}, - subordinates: IdentityPerson[] = [], + subordinates: IdentityPerson[] = [] ): IdentityPerson => ({ person_id: personId, @@ -68,6 +78,7 @@ const TREE = person(BOSS, "boss", { division: "R&D" }, [ beforeEach(() => { mocks.personId = BOSS; + mocks.definitions = { metrics: [] }; portalRouter.go("/"); mocks.ic.data = TREE; mocks.ic.isPending = false; @@ -89,7 +100,9 @@ describe("useActiveZone", () => { it("maps /team routes to the people zone", () => { portalRouter.go(`/ic/${A}/team`); - expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe("people"); + expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe( + "people" + ); }); it("only the trailing /team segment means People", () => { @@ -104,7 +117,9 @@ describe("useActiveZone", () => { it("tolerates a trailing slash on the team route", () => { portalRouter.go(`/ic/${A}/team/`); - expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe("people"); + expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe( + "people" + ); }); it("the path wins over a stale ?zone= — the URL cannot contradict itself", () => { @@ -114,18 +129,24 @@ describe("useActiveZone", () => { // IS person, whatever an older param says. portalRouter.go(`/ic/${A}/personal`); act(() => portalRouter.set({ zone: "overview" })); - expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe("person"); + expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe( + "person" + ); }); it("uses ?zone= when the path names no zone", () => { portalRouter.go("/portal"); act(() => portalRouter.set({ zone: "overview" })); - expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe("overview"); + expect(renderHook(() => useActiveZone()).result.current.activeZone).toBe( + "overview" + ); }); it("falls back to the viewer for a non-person route", () => { portalRouter.go("/metrics"); - expect(renderHook(() => useActiveZone()).result.current.activePerson).toBe(BOSS); + expect(renderHook(() => useActiveZone()).result.current.activePerson).toBe( + BOSS + ); }); }); @@ -139,7 +160,9 @@ describe("useViewerIsManager", () => { it("is not a manager for a leaf node (IC shell)", () => { mocks.personId = A; - expect(renderHook(() => useViewerIsManager()).result.current.isManager).toBe(false); + expect( + renderHook(() => useViewerIsManager()).result.current.isManager + ).toBe(false); }); it("reports pending while identity resolves (callers assume manager)", () => { @@ -151,19 +174,46 @@ describe("useViewerIsManager", () => { }); describe("usePersonCohort", () => { + /** A real query client, so the catalog query runs rather than being faked. */ + const cohortOf = (id: string) => + renderHook(() => usePersonCohort(id), { + wrapper: ({ children }) => ( + + {children} + + ), + }); + it("is empty when no slice is active", () => { - expect(renderHook(() => usePersonCohort(A)).result.current).toEqual([]); + expect(cohortOf(A).result.current).toEqual([]); }); it("returns everyone sharing the person's slice value", () => { act(() => portalRouter.set({ slice: "division" })); - const { result } = renderHook(() => usePersonCohort(A)); + const { result } = cohortOf(A); expect(result.current.sort()).toEqual([A, BOSS, C].sort()); }); + it("drops a slice the catalog does not offer, rather than comparing by it", async () => { + // The control falls back to "Team (all)" when the options lose a + // dimension. If the cohort kept building from the stored value, the + // screen would show one thing and compare by another. + mocks.definitions = { + metrics: [], + comparison_attributes: [{ id: "job_title", label: "Title" }], + }; + act(() => portalRouter.set({ slice: "division" })); + const { result } = cohortOf(A); + await waitFor(() => expect(result.current).toEqual([])); + }); + it("is empty when the person has no value for the slice attribute", () => { act(() => portalRouter.set({ slice: "title" })); - expect(renderHook(() => usePersonCohort(A)).result.current).toEqual([]); + expect(cohortOf(A).result.current).toEqual([]); }); }); diff --git a/src/frontend/src/lib/portal/use-cohort-options.ts b/src/frontend/src/lib/portal/use-cohort-options.ts new file mode 100644 index 000000000..ff71922ba --- /dev/null +++ b/src/frontend/src/lib/portal/use-cohort-options.ts @@ -0,0 +1,61 @@ +import { useMemo } from "react"; + +import { useViewer } from "@/auth"; +import { collectRosterAttrs } from "@/lib/insight/slices"; +import { normalizePersonId } from "@/lib/metrics/entity"; +import { + catalogAttributes, + cohortOptions, + type CohortOptions, +} from "@/lib/portal/cohort-options"; +import { usePortalSlice } from "@/lib/portal/portal-nav"; +import { useIcPerson } from "@/queries/ic-dashboard"; +import { useMetricDefinitionsResponse } from "@/queries/metric-definitions"; + +export interface CohortOptionsState extends CohortOptions { + /** True until both sources have answered — nothing may be dropped before. */ + isPending: boolean; + /** + * The stored slice, but only while it is one of `dims`. + * + * Two things read the slice: the control, which shows the selection, and the + * cohort builder, which decides who a person is compared against. They used + * to read it independently, so a stored value the options no longer contain + * left the control showing "Team (all)" while the comparison was still built + * by the old attribute — the screen said one thing and did another. Deriving + * it once here removes the possibility rather than correcting it afterwards. + */ + slice: string; +} + +/** + * The cohort choices, and which of them is in effect. + * + * One hook so the control and the comparison cannot disagree; see + * `cohortOptions` for why the catalog outranks the roster. + */ +export function useCohortOptions(): CohortOptionsState { + const { personId } = useViewer(); + const stored = usePortalSlice(); + const definitions = useMetricDefinitionsResponse(); + const tree = useIcPerson(personId ?? ""); + + const options = useMemo( + () => + cohortOptions( + catalogAttributes(definitions.data), + collectRosterAttrs(tree.data ?? null, normalizePersonId).values() + ), + [definitions.data, tree.data] + ); + + const isPending = definitions.isPending || tree.isPending; + return { + ...options, + isPending, + // While the sources are still answering, keep the stored value: a link + // carrying `?slice=` must survive the moment before the options arrive. + slice: + isPending || options.dims.some((d) => d.key === stored) ? stored : "", + }; +} diff --git a/src/frontend/src/lib/portal/use-person-cohort.ts b/src/frontend/src/lib/portal/use-person-cohort.ts index 7cf88e650..6946588ef 100644 --- a/src/frontend/src/lib/portal/use-person-cohort.ts +++ b/src/frontend/src/lib/portal/use-person-cohort.ts @@ -3,9 +3,7 @@ import { useMemo } from "react"; import { useViewer } from "@/auth"; import { cohortKey, collectRosterAttrs } from "@/lib/insight/slices"; import { normalizePersonId } from "@/lib/metrics/entity"; -import { - usePortalSlice, -} from "@/lib/portal/portal-nav"; +import { useCohortOptions } from "@/lib/portal/use-cohort-options"; import { useIcPerson } from "@/queries/ic-dashboard"; /** @@ -16,12 +14,15 @@ import { useIcPerson } from "@/queries/ic-dashboard"; * once, not re-derived per screen. */ export function usePersonCohort(entityId: string): string[] { - const slice = usePortalSlice(); + // The slice the OPTIONS still contain, not whatever is stored: a value the + // catalog has since dropped would otherwise keep building cohorts while the + // control shows "Team (all)". + const { slice } = useCohortOptions(); const { personId } = useViewer(); const tree = useIcPerson(personId ?? "").data ?? null; const attrByEntity = useMemo( () => collectRosterAttrs(tree, normalizePersonId), - [tree], + [tree] ); return useMemo(() => { if (!slice) return []; diff --git a/src/frontend/src/queries/metric-definitions.ts b/src/frontend/src/queries/metric-definitions.ts index ef9a36ad2..0f4662cd3 100644 --- a/src/frontend/src/queries/metric-definitions.ts +++ b/src/frontend/src/queries/metric-definitions.ts @@ -4,6 +4,7 @@ import { useMemo } from "react"; import { listMetricDefinitions, type MetricDefinition, + type MetricDefinitionListResponse, } from "@/api/metric-definitions-client"; export interface MetricDefinitionGroup { @@ -44,6 +45,20 @@ export function useMetricDefinitions(): UseQueryResult< }); } +/** + * The whole catalog response, not just its metrics. + * + * Shares the one cached query the availability gate and the definitions list + * already use, so reading a second field off it costs no extra request. + */ +export function useMetricDefinitionsResponse(): UseQueryResult { + return useQuery({ + queryKey: ["metric-definitions"], + queryFn: listMetricDefinitions, + staleTime: 5 * 60 * 1000, + }); +} + export interface AvailableMetricKeys { /** * What this installation can serve, or null when the catalog could not be @@ -79,10 +94,10 @@ export function useAvailableMetricKeys(): AvailableMetricKeys { () => data ? new Set( - data.metrics.filter((m) => m.is_enabled).map((m) => m.metric_key), + data.metrics.filter((m) => m.is_enabled).map((m) => m.metric_key) ) : null, - [data], + [data] ); return { keys, isPending }; }