Repository navigation
feat(people): add a synced organization chart - #9628
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
[High impact] The PR appears safe to merge; no new actionable issues were found. SummaryAdds an organization chart backed by SCIM, Okta, or Microsoft Entra ID.
The three earlier, unnumbered findings are addressed: long departments use digest-based filters, chart labels use
|
There was a problem hiding this comment.
7 issues found across 85 files
Confidence score: 3/5
provider-request.tsrejects Graph continuation links when the collection path differs only in casing, which can stop directory sync. Keep the origin check and compare the path case-insensitively.provision-user.tscan wait on thepeopleDirectorySyncrow while SCIM publish holds itFOR UPDATE; the 5-second lock timeout may cause provisioning to fail. Avoid making this marker update contend with the publish transaction.people-sync.tsstores and sends an EntraclientSecretwith surrounding whitespace, which can make token requests fail. Trim it before validation.provider-import.tscounts disabled Entra accounts toward the 10,000-person cap even though they are excluded from imported rows, so a directory with 10,000 or fewer enabled users can be rejected. Count only profiles kept for import.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/emcn/src/components/combobox/combobox.tsx">
<violation number="1" location="packages/emcn/src/components/combobox/combobox.tsx:132">
P3: The `onSearchChange` JSDoc now contradicts the new `searchMode` behavior. In `'server'` mode, local filtering of `options` is skipped, so the doc should say that. Update the paragraph to state that client-side filtering applies only in `searchMode='local'`, and that server mode relies on `onSearchChange` to supply matches.</violation>
</file>
<file name="apps/sim/app/workspace/[workspaceId]/components/resource/components/resource-empty-state/resource-no-results.tsx">
<violation number="1" location="apps/sim/app/workspace/[workspaceId]/components/resource/components/resource-empty-state/resource-no-results.tsx:16">
P3: The new `filterScope` prop is inserted between `searchScope`'s doc comment and `searchScope`, so the "Where a search looked" comment now describes `filterScope`. Move the prop below `searchScope` and give `filterScope` its own doc comment.</violation>
</file>
<file name="apps/sim/lib/api/contracts/people-sync.ts">
<violation number="1" location="apps/sim/lib/api/contracts/people-sync.ts:27">
P2: `clientSecret` is not trimmed, unlike the Okta `token` in this schema, so a secret pasted with surrounding whitespace is stored and sent to Entra unchanged and token requests fail. Add `.trim()` before `.min(1)` to match the Okta token.</violation>
</file>
<file name="apps/sim/lib/people/provider-import.ts">
<violation number="1" location="apps/sim/lib/people/provider-import.ts:199">
P2: This row cap counts disabled Entra accounts, which are filtered out of the imported rows. A directory with 10,000 or fewer enabled users can fail with the 10,000-person error. Count the profiles that were kept instead, e.g. `count += page.people.length`.</violation>
</file>
<file name="apps/sim/lib/people/sync/provider-request.ts">
<violation number="1" location="apps/sim/lib/people/sync/provider-request.ts:17">
P2: This exact pathname equality rejects valid Graph continuation links whose path differs in casing, which would fail the directory sync. Keep the origin pin and compare the collection path case-insensitively instead of requiring an exact match.
(Based on your team's feedback about Graph nextLink path matching.)</violation>
</file>
<file name="apps/sim/ee/scim/lib/application/users/provision-user.ts">
<violation number="1" location="apps/sim/ee/scim/lib/application/users/provision-user.ts:232">
P2: This new marker UPDATE waits on the org's `peopleDirectorySync` row, which the SCIM publish holds `FOR UPDATE` for its whole transaction. The transaction-local 5 s `lock_timeout` set earlier by the membership lock carries over, so a provisioning request that arrives during a slow publish fails with a retryable 503 instead of completing. Consider whether the marker can run outside this transaction or after the membership lock is taken, or confirm that publish stays well under 5 s at the directory sizes you support.</violation>
</file>
<file name="apps/sim/app/o/[organizationId]/people/people-directory.tsx">
<violation number="1" location="apps/sim/app/o/[organizationId]/people/people-directory.tsx:169">
P2: This directory accumulates every loaded page but leaves `Resource.Table` virtualization disabled, so a large organization renders thousands of row subtrees and grows the DOM linearly. Pass `virtualized` to bound rendered rows as more pages load.</violation>
</file>
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 89 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 91 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Summary
Type of Change
Testing
Verified 10,241 Okta users, 10,007 Entra users, and 10,004 SCIM users against real PostgreSQL, including a manager on the last page, mixed success and throttling inside a Graph batch, retry and lease recovery, and preservation of the previous chart. Tested chart navigation, import, and sync administration in Chromium at desktop and mobile sizes. The two HTTP suites passed 52 checks over 309 requests. All 45 focused database integration tests passed. A separate HTTP contention regression confirmed that SCIM provisioning and retirement complete while publication holds the sync lock. Browser fixtures covered two distinct 2,048-character department names on a 390px screen, single-page People search continuation, and credential drafts across transient failures and access revocation. Okta and Microsoft Graph responses were simulated.
Validated 100,000-person wide and deep trees in PostgreSQL. One chart branch on a freshly published 10,007-person fixture took 36 ms, down from 9.16 s.
The repository test suite passed 42,660 tests. Type checks, lint, all 62 audits, migration safety, and block-registry validation passed.
Checklist