feat: cut the SPA over to canonical person_id (merge with insight#2098) - #255
Conversation
The metrics API and identity both take canonical person UUIDs since the backend cutover: `POST /v1/metric-results` rejects a non-UUID entity id with a 400, and `POST /v1/profiles` resolves `value_type: "person_id"`. Move the client's person key from email to person_id at the source: - `normalizePersonId` now names what it normalizes, and `isPersonId` recognizes a UUID so callers can tell a live id from a legacy email. - `getPerson(personId)` posts the person_id form; `getPersonByEmail` stays for the one caller that still holds an email (legacy URLs). A profile without a person_id is rejected — it has no usable key — while a subordinate without an email is now kept, because email is no longer the key. - The identity tree is walked and flattened by person_id, with the email lookup kept for the legacy path only. - The viewer exposes `getViewerPersonId()`, which the dashboard query uses for its cache key and its org-tree placeholder. Merge in lockstep with the backend cutover (insight#2098). Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
The `/ic/$person` param is now a person UUID: routes guard it with `isPersonId` and hand anything else to `LegacyPersonRedirect`, which resolves the email through identity once and replaces the URL with the canonical id. Without the guard a bookmarked email URL would reach the metrics API as a 400 the user cannot act on. Sidebar nodes, team rosters and the members grid all carry person_id, so active-node matching and every generated link agree with the API key. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
The mock registry keys people by a derived UUID and keeps their email as a field, so mock-mode exercises the same shape as production. The metric-results handler answers 400 for a non-UUID entity id and the profiles handler dispatches on `value_type`, which is what makes the route guard and the legacy-email redirect testable without a backend. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR changes the SPA identity key from email to canonical UUID-based ChangesCanonical person ID migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Viewer
participant DashboardRoute
participant IdentityClient
participant DashboardScreen
Viewer->>DashboardRoute: provide person_id
DashboardRoute->>IdentityClient: fetch person profile
IdentityClient-->>DashboardScreen: return profile or identity error
DashboardScreen-->>Viewer: render dashboard or retry state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/identity-client.ts (2)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCanonicalize
person_idonce, at ingestion, instead of relying on every comparison site to be defensive.toIdentityPersonvalidatesperson_idwith.trim()but stores the untrimmed value;findIdentityNode'stoLowerhelper never trims either. Aperson_idwith stray whitespace passes validation, gets stored as-is, and then silently fails to match in tree lookups, route links, and React keys.
src/api/identity-client.ts#L65-86: trimperson_idwhen constructing theIdentityPersonobject (person_id: p.person_id.trim()), both for the top-level profile and for each subordinate produced by the recursivemap(toIdentityPerson)call, so the canonical form is established in one place.src/lib/insight/identity-tree.ts#L5-17: onceperson_idis trimmed at ingestion,findIdentityNode'stoLower-based comparison no longer needs a defensive trim; no code change required here beyond relying on the upstream fix.🤖 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/api/identity-client.ts` at line 1, Canonicalize person_id during ingestion in toIdentityPerson by storing the trimmed value for both the top-level IdentityPerson and recursively mapped subordinates. Leave findIdentityNode and its toLower comparison unchanged, relying on the upstream canonical form.
65-86: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStore the trimmed
person_id, not just validate it.The subordinate filter at Line 83 checks
Boolean(s.person_id?.trim()), but the stored value at Line 67 (person_id: p.person_id) and the recursively mapped subordinates keep the untrimmed original string. Aperson_idwith only surrounding whitespace is dropped correctly, but aperson_idwith valid content plus stray whitespace (for example," 019e27bc-... ") passes the filter and is stored with the whitespace intact.This value is documented as "the UI identity (route links + React keys)". An untrimmed
person_idproduces the same class of problem the code comment warns about — a link or React key that doesn't match its canonical form elsewhere (route params, query cache keys,identity-tree.tscomparisons), becausefindIdentityNode'stoLowerhelper inidentity-tree.tsdoes not trim either.Trim
person_idwhen constructing the object, for both the top-level profile and each subordinate.🔧 Proposed fix
function toIdentityPerson(p: ProfileResponse): IdentityPerson { return { - person_id: p.person_id, + person_id: p.person_id.trim(), email: p.email ?? "",🤖 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/api/identity-client.ts` around lines 65 - 86, Update toIdentityPerson so person_id is stored in trimmed form rather than preserving surrounding whitespace. Apply the same normalization recursively through the existing subordinates filter/map path, while continuing to exclude blank or whitespace-only IDs.
🧹 Nitpick comments (1)
src/components/app-sidebar.tsx (1)
85-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
person_idfor the sibling list key.Every identity reference in
PersonNodenow usesperson_id(isActive,hasActiveDescendant, theLinkparams,activePersonIdpropagation), except the listkeyon line 88, which still usessub.email. Since this PR establishesperson_idas the canonical, stable identifier and treats♻️ Proposed fix
{hasReports && open ? node.subordinates.map((sub) => ( <PersonNode - key={sub.email} + key={sub.person_id} node={sub} depth={depth + 1} activePersonId={activePersonId} /> )) : 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/components/app-sidebar.tsx` around lines 85 - 93, Update the sibling list key in the PersonNode subordinates map to use sub.person_id instead of sub.email. Keep the existing rendering and identity propagation unchanged, using person_id as the canonical stable identifier.
🤖 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/api/identity-client.ts`:
- Around line 103-105: Update getPerson to pass normalizePersonId(personId) as
the value in its resolveProfile request, ensuring person IDs are canonicalized
before calling /profiles while preserving the existing value_type and return
behavior.
In `@src/routes/ic`.$person.team.tsx:
- Around line 12-21: Update TeamScreen to handle an unresolved useViewer()
result as a loading state, following the existing pattern in the index route,
instead of falling back to person. Only render TeamViewScreen after
viewerPersonId is resolved and pass that resolved id as viewerPersonId.
---
Outside diff comments:
In `@src/api/identity-client.ts`:
- Line 1: Canonicalize person_id during ingestion in toIdentityPerson by storing
the trimmed value for both the top-level IdentityPerson and recursively mapped
subordinates. Leave findIdentityNode and its toLower comparison unchanged,
relying on the upstream canonical form.
- Around line 65-86: Update toIdentityPerson so person_id is stored in trimmed
form rather than preserving surrounding whitespace. Apply the same normalization
recursively through the existing subordinates filter/map path, while continuing
to exclude blank or whitespace-only IDs.
---
Nitpick comments:
In `@src/components/app-sidebar.tsx`:
- Around line 85-93: Update the sibling list key in the PersonNode subordinates
map to use sub.person_id instead of sub.email. Keep the existing rendering and
identity propagation unchanged, using person_id as the canonical stable
identifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 097c153b-102c-4c85-bc0e-84e9c809b5c8
📒 Files selected for processing (21)
src/api/identity-client.test.tssrc/api/identity-client.tssrc/auth/index.tssrc/auth/use-viewer.tssrc/components/app-sidebar.test.tsxsrc/components/app-sidebar.tsxsrc/components/legacy-person-redirect.tsxsrc/components/widgets/dashboard/members-grid.tsxsrc/components/widgets/dashboard/members-overview.tsxsrc/lib/insight/identity-tree.tssrc/lib/metrics/entity.test.tssrc/lib/metrics/entity.tssrc/mocks/handlers.tssrc/mocks/registry.tssrc/queries/ic-dashboard.tssrc/routes/ic.$person.personal.tsxsrc/routes/ic.$person.team.tsxsrc/routes/index.tsxsrc/screens/team-view.test.tsxsrc/screens/team-view.tsxsrc/types/insight.ts
💤 Files with no reviewable changes (1)
- src/components/widgets/dashboard/members-overview.tsx
| function TeamScreen() { | ||
| const { person } = Route.useParams(); | ||
| const { email: viewerEmail } = useViewer(); | ||
| const viewer = viewerEmail ?? person; | ||
| return <TeamViewScreen teamId={person} viewerEmail={viewer} />; | ||
| const { personId: viewerPersonId } = useViewer(); | ||
| if (!isPersonId(person)) { | ||
| return <LegacyPersonRedirect email={person} view="team" />; | ||
| } | ||
| // The viewer's own id is the pivot's fallback: a team view always has a | ||
| // person whose subtree it shows. | ||
| return <TeamViewScreen teamId={person} viewerPersonId={viewerPersonId ?? person} />; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'ic\.\$person\.team\.tsx|screens/team-view\.tsx|routes/index\.tsx|use-viewer\.ts|getPerson|useIcPerson' . | sed 's#^\./##' | head -80
echo
echo "== relevant route/team-view/index snippets =="
for f in src/routes/ic.\$person.team.tsx src/screens/team-view.tsx src/routes/index.tsx; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
ast-grep outline "$f" --view expanded || true
fi
doneRepository: constructorfabric/insight-front
Length of output: 943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/routes/ic.\$person.team.tsx src/screens/team-view.tsx src/routes/index.tsx src/auth/use-viewer.ts; do
echo "--- $f ---"
cat -n "$f"
echo
done
echo "== definitions/usages for getPerson/useIcPerson/findIdentityNode/FullScreenLoading =="
rg -n "function useIcPerson|const useIcPerson|getPerson|FullScreenLoading|findIdentityNode|viewerPersonId\\?\\?|Full.*Loading" srcRepository: constructorfabric/insight-front
Length of output: 17951
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- src/queries/ic-dashboard.ts ---"
cat -n src/queries/ic-dashboard.ts
echo
echo "--- src/api/identity-client.ts relevant section ---"
sed -n '1,150p' src/api/identity-client.ts | cat -n
echo
echo "--- src/lib/insight/identity-tree.ts ---"
cat -n src/lib/insight/identity-tree.ts
echo
echo "== behavioral/static verifier =="
python3 - <<'PY'
from pathlib import Path
files = {
"team_route": Path("src/routes/ic.$person.team.tsx").read_text(),
"team_view": Path("src/screens/team-view.tsx").read_text(),
"index_route": Path("src/routes/index.tsx").read_text(),
"use_viewer": Path("src/auth/use-viewer.ts").read_text(),
"ic_dashboard": Path("src/queries/ic-dashboard.ts").read_text(),
"identity_client": Path("src/api/identity-client.ts").read_text(),
}
checks = {}
checks["team_fallbacks_null_to_person"] = "viewerPersonId ?? person" in files["team_route"]
checks["index_handles_null_person_id"] = "if (!personId) return <FullScreenLoading />" in files["index_route"]
checks["view_uses_viewer_person_id_in_use_ic_person"] = "const viewerQ = useIcPerson(viewerPersonId)" in files["team_view"]
checks["view_find_identity_node_documents_own_tree"}"] = 'viewers own tree' in files["team_view"].lower().replace(" ", " ") or "viewer's own tree" in files["team_view"] or "own tree" in files["team_view"]
checks["get_person_passes_person_id_directly"] = "export async function getPerson(personId: string)" in files["identity_client"] and 'getPerson(JSON.parse' not in files["identity_client"]
checks["get_person_uses_personId_path"] = '"/v1/person/" + personId' in files["identity_client"] or "JSON.stringify({ personId" in files["identity_client"]
checks["find_identity_node_base_returns_this_when_id_equals_node_id"]
for i,line in enumerate(files["identity_tree"].splitlines(),1):
if "return this" in line and "identity id" in line.lower():
checks["find_identity_node_base_returns_this_when_id_equals_node_id"] = True
break
for k,v in checks.items():
print(f"{k}={v}")
print("reached_base_returns_this_example=", 'return this' in files["identity_tree"].splitlines()[0:5] if False else "")
PYRepository: constructorfabric/insight-front
Length of output: 11687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = {
"team_route": Path("src/routes/ic.$person.team.tsx").read_text(),
"team_view": Path("src/screens/team-view.tsx").read_text(),
"index_route": Path("src/routes/index.tsx").read_text(),
"use_viewer": Path("src/auth/use-viewer.ts").read_text(),
"ic_dashboard": Path("src/queries/ic-dashboard.ts").read_text(),
"identity_client": Path("src/api/identity-client.ts").read_text(),
"identity_tree": Path("src/lib/insight/identity-tree.ts").read_text(),
}
print("team_fallbacks_null_to_person=", "viewerPersonId ?? person" in files["team_route"])
print("index_handles_null_person_id=", "if (!personId) return <FullScreenLoading />" in files["index_route"])
print("view_uses_viewer_person_id_in_use_ic_person=", "const viewerQ = useIcPerson(viewerPersonId)" in files["team_view"])
print("view_comment_notes_strangers_do_not_resolve=", "a stranger's id simply does not resolve" in files["team_view"])
print("identity_client_get_person_comment_mentions_visible_set=", "Identity applies the caller's visible set" in files["identity_client"])
print("identity_client_get_person_body_contains_person_id_field=", 'value_type: "person_id"' in files["identity_client"] and "value: personId" in files["identity_client"])
print("find_identity_node_returns_tree_root_on_match=", 'if (toLower(tree.person_id) === target) return tree' in files["identity_tree"])
# In-place verifier: simulate the current TeamScreen fallback for resolved target person + unresolved viewer.
from dataclasses import dataclass, field
from typing import List, Optional, Any
`@dataclass`
class IdentityPerson:
person_id: str
display_name: str = ""
subordinates: List["IdentityPerson"] = field(default_factory=list)
def find_identity_node(tree: Optional[IdentityPerson], person_id: str) -> Optional[IdentityPerson]:
if not tree:
return None
if tree.person_id.lower() == person_id.lower():
return tree
for sub in tree.subordinates:
found = find_identity_node(sub, person_id)
if found:
return found
return None
def flatten_subordinates(pivot: IdentityPerson) -> List[dict]:
out = []
def walk(node: IdentityPerson, supervisor_person_id: str, is_direct: bool):
for sub in node.subordinates:
out.append({
"person_id": sub.person_id,
"display_name": sub.display_name,
"supervisor_person_id": supervisor_person_id,
"is_direct": is_direct,
})
walk(sub, sub.person_id, False)
walk(pivot, pivot.person_id, True)
return out
# Target person has two named subordinates. Viewer has no subtree under them.
target_person = IdentityPerson(person_id="target", display_name="Target", subordinates=[
IdentityPerson(person_id="a", display_name="Ada"),
IdentityPerson(person_id="b", display_name="Bob"),
])
viewer_tree = IdentityPerson(person_id="viewer", display_name="Viewer", subordinates=[])
actual_viewer_person_id = None
viewer_person_id = actual_viewer_person_id or target_person.person_id
pivot = find_identity_node(target_person, viewer_person_id)
print("fallback_pivot_matches_target_root=", pivot.person_id == "target")
print("fallback_extracted_roster_count=", len(flatten_subordinates(target_person)))
print("fallback_roster_names=", sorted(p["display_name"] for p in flatten_subordinates(target_person)))
real_viewer_person_id = "viewer"
real_pivot = find_identity_node(viewer_tree, target_person.person_id)
print("actual_viewer_pivot_is_null=", real_pivot is None)
PYRepository: constructorfabric/insight-front
Length of output: 667
Guard the unreal viewer id before resolving the team pivot.
viewerPersonId ?? person substitutes the target person as the viewer while useViewer() is unresolved. TeamViewScreen then calls useIcPerson(person), and findIdentityNode matches that root, so the team names and roster are derived from the target identity instead of the actual viewer’s authorized tree. Treat the unresolved viewer case as a loading state, like src/routes/index.tsx does, and pass the resolved viewer id to TeamViewScreen.
🤖 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/routes/ic`.$person.team.tsx around lines 12 - 21, Update TeamScreen to
handle an unresolved useViewer() result as a loading state, following the
existing pattern in the index route, instead of falling back to person. Only
render TeamViewScreen after viewerPersonId is resolved and pass that resolved id
as viewerPersonId.
The redirect and `findIdentityNode` were the two uncovered halves of the cutover, and they are the halves that decide whether a pre-cutover link works: the redirect asserts `replace` (so Back cannot bounce the user into the email URL again), the view mapping, and the fall back to the root dashboard for an email that resolves to nobody. The identity-tree fixture now gives each persona a UUID distinct from their email, and one case asserts that `findIdentityNode` does NOT match a node by email — keying both the same would have hidden a lookup still matching on the wrong field. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
Review follow-ups on the SPA cutover. The shell's prefetch still read `getViewerEmail()` and passed it to `getPerson`, which posts `value_type: "person_id"` since the cutover. Two consequences, both silent because `prefetchQuery` swallows its own errors: identity answered 400 on every load, and the cache entry the shell warmed was email-keyed, so `useIcPerson` missed it and fetched again on mount — defeating the "shell mounts with identity cached" invariant. It now reads `getViewerPersonId()` and writes the key `useIcPerson` computes, with tests covering a path that had none. Team view resolved its pivot by walking the viewer's tree, so a person the viewer reaches through an explicit or wildcard grant — allowed by identity and by the metrics gate — rendered an empty team. It now asks identity for the pivot's own profile; the viewer's cached tree still serves as placeholder data, so the common case paints just as fast. `viewerPersonId` becomes unused and is dropped from the props. The legacy-email redirect treated every failure as "not found" and sent the user to the root dashboard, reporting an expired session or a down identity service as a missing person. Only a 404 falls back now; everything else reaches the error boundary. The tests use real `IdentityApiError` statuses instead of a bare `Error`. Sidebar nodes keyed React children on `sub.email`, and the contract now admits people with no email — two such siblings would share the empty key and reconcile wrongly. Keyed on `person_id`. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
Codex review — all four fixed in
|
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/lib/insight/identity-tree.test.ts`:
- Around line 27-32: Update the personId fixture helper to generate canonical
UUIDs with an 8-4-4-4-12 layout, while retaining deterministic, distinct IDs
derived from each email’s local part. Ensure the resulting fixtures remain valid
UUIDs for testing person_id validation and normalization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ba197a5-4495-4420-b72e-9c173c721711
📒 Files selected for processing (9)
src/components/app-sidebar.tsxsrc/components/legacy-person-redirect.test.tsxsrc/components/legacy-person-redirect.tsxsrc/lib/insight/identity-tree.test.tssrc/routes/__root.test.tsxsrc/routes/__root.tsxsrc/routes/ic.$person.team.tsxsrc/screens/team-view.test.tsxsrc/screens/team-view.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/legacy-person-redirect.tsx
- src/components/app-sidebar.tsx
`src/routes/` is scanned by the router generator, so a test file there is read as a route and every build warns that it exports no `Route`. The `-` prefix is the generator's own configured escape (`routeFileIgnorePrefix`), so the file keeps its place next to the module it covers without joining the route tree. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
|
Fixed in Reproduced first: the generator warns
|
There was a problem hiding this comment.
I'm not sure we need this. We don't have any proper users yet to have this problem. It will just be bloat.
Review follow-ups. `isPersonId` accepted the nil UUID, which parses as a UUID but is never a person — both analytics and identity reject it. It cleared the route guard, so `/ic/<nil>/personal` painted a dashboard whose every metric request 400s. It now fails the guard, and because the MSW handlers share the predicate, the mocks stop hiding the hole too. The legacy redirect keyed its cache on the normalized email while sending the raw one. The backend only trims — the rest is the column's collation — so two spellings could share a cache entry the service might have resolved differently. One spelling now goes to both. The dashboard ignored the identity query's error: a valid id outside the viewer's visible set rendered a nameless dashboard over failing metrics, against the invariant the identity client documents. A 404 now says the person is not available with nothing to retry; any other failure offers a retry. Smaller, from the same review: - The mock profiles handler matched emails case-sensitively (stricter than the service) and answered 404 where the real API answers 400 for a malformed person_id. Both aligned. - `findIdentityNodeByEmail` was dead code whose comment invited email lookups back. Removed. - A sidebar node with neither display name nor email rendered as an empty sliver; it gets a placeholder label. - Dropped the "never sends an email" prefetch test — it echoed its own fixture and could not fail. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
Both fixes plus every minor —
|
Review: there are no pre-cutover URLs to migrate yet, so the component, its query, its error handling and the email-resolving client helper were carrying a problem nobody has. A non-canonical `$person` param still must not reach the metrics API, where it is a 400 the user cannot act on, so the route guard stays and sends it to the root dashboard instead — three lines in place of the component. That also keeps covering the nil UUID, which parses as a UUID but is never a person. Gone with it: `getPersonByEmail` (the redirect was its only caller) and the identity client's last email-keyed path. `getPerson` now normalizes the id on the way out, matching the query key, so one spelling of an id cannot become two requests. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
|
@aleksdotbar Removed — One thing I kept, three lines: the route still guards Gone with the component: Also took CodeRabbit's note on the same file: Verification: The backend half is also updated for your review there — |
`getViewerEmail` lost its last caller when the shell prefetch moved to person_id, and `resolveProfile`'s "email" variant lost its caller with the legacy redirect — an unused export and an unreachable union arm are exactly the kind of seam through which email keying grows back. The viewer's email itself stays on the session for display. The use-viewer test now pins the surviving accessor instead. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
Review: a pivot whose profile lookup fails — 404, 5xx, network — rendered the ordinary empty state, reporting a broken lookup as "this team has no members" with nothing to act on. Same split as the personal dashboard now: a 404 (gone, renamed, or outside the visible set) says the person is not available with nothing to retry; any other identity failure offers a working Retry. Two tests pin both branches. Also from the review: the identity-tree test helper minted UUIDs with a 16-hex final group — invalid ids that only passed because the lookup compares strings. The helper emits the 8-4-4-4-12 form now, and a fixture self-guard asserts `isPersonId` accepts what it mints, so these tests can never again pass against ids production rejects. Signed-off-by: Sergey Mozhaev <Sergey.Mozhaev@constructor.tech>
2 participants
This PR and constructorfabric/insight#2098 are two halves of one contract
change. Merging either alone breaks the dashboards:
(
POST /v1/metric-resultsanswers 400 for a non-UUID entity id).nothing, so every metric comes back empty.
What changes
Person identity in the SPA moves from email to the canonical
person_idUUID the identity-resolution service owns.
/ic/$personroute paramPOST /v1/metric-resultsentity_idsPOST /v1/profiles{value_type: "email"}{value_type: "person_id"}getViewerEmail()getViewerPersonId(), keyed to matchuseIcPersonemailperson_idIdentity decides who is visible, not the tree
Names and rosters could in principle be read off the viewer's org tree, but the
tree only covers the reporting line. Visibility also comes from explicit grants
and wildcard grants, so a person a viewer may legitimately see can be absent
from their tree — and a tree lookup renders their team empty even though the
metrics gate would authorize it. Every person-keyed read now goes to identity by
person_id, which is why that mode was added to/v1/profilesin insight#2098.The same contract admits people with no email, so nothing keys on email any
more — including React keys, where two email-less siblings would have collided
on the empty string.
Legacy URLs keep working
A bookmarked or shared
/ic/alice@example.com/personalis not a 404 and not a400. The route guards the param with
isPersonId; anything else rendersLegacyPersonRedirect, which resolves the email throughPOST /v1/profiles {value_type:"email"}once andreplaces the URL with thecanonical id.
Only a 404 — the person is gone, renamed, or outside the viewer's visible
set — falls back to the root dashboard. Every other failure (401, 5xx, network,
malformed body) reaches the error boundary: routing those to the root would
report a broken session or a down service as "no such person".
getPersonByEmailexists for exactly that one caller — every other identityread goes by person_id.
Test evidence
npx tsc -b— cleannpx eslinton every changed file — cleannpx vitest run --project=unit— 440 passed (75 files)npx vitest run --project=storybook— 2 passednpx vite build— clean, no warningsFixtures were rekeyed rather than adapted: the mock registry derives a stable
UUID per person and keeps
emailas a field, and the mock metric-resultshandler answers 400 for a non-UUID entity id — so the guard and the redirect
are covered against the same contract the backend enforces.
Two paths that had no coverage before now do, because both hid a real bug: the
shell's viewer prefetch (silently email-keyed, so identity answered 400 on every
load and the warmed cache entry was never read) and a team pivot outside the
viewer's own tree.
Follow-up
Cohorts are a backend concern only; the frontend never sends or reads a cohort
id, so the planned cohorts → identity org-chart migration needs no change here.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes