feat: add SCIM 2.0 user provisioning (EE)#1306
Open
brendan-kellam wants to merge 15 commits into
Open
Conversation
Adds a SCIM 2.0 server so an IdP (Okta, Entra) can provision, update, and deprovision org members. Users-only scope; deprovisioning soft-deactivates the membership (forces logout + revokes tokens) rather than deleting it, and JIT auto-join is suppressed when SCIM is enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
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:
WalkthroughThis PR adds SCIM 2.0 endpoints, token management, and SCIM-aware membership state. It also splits membership actions into feature modules, updates login and join flows, and changes settings, billing, chat, and API responses to use active or suspended membership data. ChangesSCIM provisioning and membership architecture update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
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 |
brendan-kellam
commented
Jun 16, 2026
| /// SCIM soft-deactivation flag. When false, the membership is suspended by | ||
| /// the IdP: the user is treated as a non-member for auth purposes (see | ||
| /// `getAuthContext`) but the row is preserved so the IdP can reactivate it. | ||
| isActive Boolean @default(true) |
Contributor
Author
There was a problem hiding this comment.
tbd: do we actually need to store a isActive field here?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
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)
packages/web/src/app/invite/page.tsx (1)
45-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not treat inactive memberships as active members in redirect logic.
At Line 46,
if (membership)redirects users with SCIM-deactivated rows (isActive: false) as if they were active members. This diverges from the auth contract where only active memberships confer access.Suggested fix
- if (membership) { + if (membership?.isActive) { redirect(`/`); }🤖 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 `@packages/web/src/app/invite/page.tsx` around lines 45 - 47, The redirect logic in the invite page only checks for the existence of a membership object, but does not verify that it is active. Modify the condition in the if statement that checks `membership` to also verify that the membership's active status is true (check the isActive property). This ensures that users with deactivated SCIM memberships are not incorrectly treated as active members and redirected, maintaining consistency with the authentication contract.
🟡 Minor comments (8)
packages/web/src/app/(app)/settings/security/components/scimProvisioningSettings.tsx-125-127 (1)
125-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd accessible labels to icon-only action buttons.
Lines 125, 170, and 230 render icon-only buttons without accessible names, which makes these actions ambiguous for screen-reader users.
Also applies to: 170-172, 230-236
🤖 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 `@packages/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx around lines 125 - 127, The icon-only action buttons in the scimProvisioningSettings component lack accessible labels for screen readers. Add aria-label attributes to the Button components at handleCopyBaseUrl (around line 125-127), the button around line 170-172, and the button around line 230-236 to provide descriptive accessible names that explain what each button does when activated by assistive technology users.packages/web/src/features/membership/components/joinOrganizationCard.tsx-56-63 (1)
56-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace CSS variable color classes with Tailwind semantic color classes.
text-[var(--muted-foreground)]andtext-[var(--primary-foreground)]should use direct Tailwind classes in this TSX file.Suggested fix
- <p className="text-[var(--muted-foreground)] text-[15px] leading-6"> + <p className="text-muted-foreground text-[15px] leading-6"> Welcome to Sourcebot! Click the button below to join this organization. </p> @@ - className="w-full h-11 bg-primary hover:bg-primary/90 text-[var(--primary-foreground)] transition-all duration-200 font-medium" + className="w-full h-11 bg-primary hover:bg-primary/90 text-primary-foreground transition-all duration-200 font-medium"As per coding guidelines,
**/*.{tsx,jsx,mdx}must use Tailwind color classes directly instead of CSS variable syntax.🤖 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 `@packages/web/src/features/membership/components/joinOrganizationCard.tsx` around lines 56 - 63, In the joinOrganizationCard component, replace the CSS variable syntax in the className attributes with direct Tailwind semantic color classes. Change `text-[var(--muted-foreground)]` to `text-muted-foreground` in the paragraph element and change `text-[var(--primary-foreground)]` to `text-primary-foreground` in the Button component's className to align with the coding guidelines that require TSX files to use Tailwind color classes directly instead of CSS variable syntax.Source: Coding guidelines
packages/web/src/features/membership/components/submitJoinRequestCard.tsx-60-70 (1)
60-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Tailwind color tokens instead of CSS-variable utility syntax.
Replace
text-[var(--...)]with token classes (text-primary-foreground,text-foreground,text-muted-foreground) for consistency with repo standards.As per coding guidelines,
**/*.{tsx,jsx,mdx}: Use Tailwind color classes directly instead of CSS variable syntax (e.g., useborder-borderinstead ofborder-[var(--border)]).🤖 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 `@packages/web/src/features/membership/components/submitJoinRequestCard.tsx` around lines 60 - 70, Replace all CSS variable utility syntax with Tailwind color token classes in the submitJoinRequestCard component. Specifically, update the className attributes in the svg element and the h1 and p elements to use text-primary-foreground, text-foreground, and text-muted-foreground instead of text-[var(--primary-foreground)], text-[var(--foreground)], and text-[var(--muted-foreground)] respectively. This aligns with the repository's coding standards for consistent use of Tailwind color classes rather than CSS variable syntax.Source: Coding guidelines
packages/web/src/app/(app)/settings/members/components/membersList.tsx-217-217 (1)
217-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a fallback label when name is missing.
Line 217 renders
member.namedirectly, so members without a profile name show a blank primary label. Use email fallback to keep identity visible.Suggested fix
- <span className="font-medium truncate">{member.name}</span> + <span className="font-medium truncate">{member.name ?? member.email}</span>🤖 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 `@packages/web/src/app/`(app)/settings/members/components/membersList.tsx at line 217, The span element rendering member.name on line 217 of membersList.tsx displays blank when the name property is missing or empty, making member identification difficult. Modify the content of the span to use a fallback expression that displays member.email when member.name is not available, such as using a logical OR operator or conditional expression to ensure every member row has a visible identifier.packages/web/src/app/api/(server)/ee/scim/v2/Users/[id]/route.ts-74-75 (1)
74-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unsafe non-null assertions after reload.
A concurrent delete between update and reload can make
refreshednull;refreshed!then produces a 500 instead of a SCIM 404.Suggested fix
- const refreshed = await loadMembership(prisma, org.id, id); - return scimJson(toScimUser(refreshed!), 200); + const refreshed = await loadMembership(prisma, org.id, id); + if (!refreshed) { + return scimError(404, `User ${id} not found`); + } + return scimJson(toScimUser(refreshed), 200);Also applies to: 113-114
🤖 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 `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts around lines 74 - 75, The non-null assertion on the `refreshed` variable returned from loadMembership can cause a 500 error if a concurrent delete occurs between the update and reload, when it should return a SCIM 404 instead. Remove the non-null assertion operator (!) after `refreshed` in the return statement at the scimJson call, and add a null check immediately after the loadMembership assignment. If refreshed is null, return an appropriate SCIM 404 response, otherwise proceed with toScimUser(refreshed) only when the value is confirmed to be non-null. Apply this same fix at the other location mentioned in the comment (lines 113-114).packages/web/src/ee/features/scim/withScimAuth.ts-26-34 (1)
26-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccept case-insensitive bearer auth schemes.
The current check rejects
bearer/mixed-case schemes, which can break otherwise valid IdP requests.Suggested fix
- const authorization = request.headers.get("Authorization") ?? undefined; - if (!authorization?.startsWith("Bearer ")) { + const authorization = request.headers.get("Authorization") ?? undefined; + const [scheme, bearer] = authorization?.split(/\s+/, 2) ?? []; + if (scheme?.toLowerCase() !== "bearer" || !bearer) { return scimError(401, "Missing or malformed Authorization header"); } - - const bearer = authorization.slice("Bearer ".length);🤖 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 `@packages/web/src/ee/features/scim/withScimAuth.ts` around lines 26 - 34, The authorization header check for the Bearer token scheme is case-sensitive and only accepts "Bearer " with that exact capitalization, which will reject valid IdP requests that use lowercase "bearer" or mixed case variants. Convert the authorization header value to lowercase before checking if it starts with "Bearer " (or check against "bearer " in lowercase), and adjust the subsequent slice operation that extracts the bearer token to use the lowercased authorization value to ensure the token extraction works correctly regardless of the case used in the Authorization header.CHANGELOG.md-42-42 (1)
42-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlace this entry under
[Unreleased]instead of a released version block.Line 42 is currently under
## [5.0.3]. The project changelog policy requires PR entries to be added under[Unreleased]until release cut.As per coding guidelines,
CHANGELOG.md: “Update CHANGELOG.md with an entry under[Unreleased]linking to the new PR.”🤖 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 `@CHANGELOG.md` at line 42, The SCIM 2.0 server provisioning entry is currently placed under the [5.0.3] released version section in CHANGELOG.md. Move this entry from the [5.0.3] section to the [Unreleased] section at the top of the changelog. The entry should maintain its exact format and content, just relocated to comply with the project's changelog policy that requires new PR entries to be added under [Unreleased] until the next release is cut.Source: Coding guidelines
packages/web/src/ee/features/scim/actions.ts-73-85 (1)
73-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize and validate token names on the server boundary.
Line 73 and Line 115 accept
nameas-is. Without trimming/non-empty validation, you can create hard-to-manage tokens (e.g., whitespace-only or visually duplicate names with trailing spaces).Proposed fix
export const generateScimToken = async (name: string): Promise<{ token: string } | ServiceError> => sew(() => withAuth(async ({ org, user, role, prisma }) => withMinimumOrgRole(role, OrgRole.OWNER, async () => { + const normalizedName = name.trim(); + if (!normalizedName) { + return { + statusCode: StatusCodes.BAD_REQUEST, + errorCode: ErrorCode.INVALID_REQUEST, + message: "SCIM token name cannot be empty", + } satisfies ServiceError; + } if (!await hasEntitlement('scim')) { return scimNotAvailable(); } const existing = await prisma.scimToken.findFirst({ where: { orgId: org.id, - name, + name: normalizedName, }, }); @@ const scimToken = await prisma.scimToken.create({ data: { - name, + name: normalizedName, hash, orgId: org.id, }, }); @@ - metadata: { scim_token: name }, + metadata: { scim_token: normalizedName }, });Also applies to: 115-126
🤖 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 `@packages/web/src/ee/features/scim/actions.ts` around lines 73 - 85, The generateScimToken function accepts the name parameter without normalization or validation, allowing whitespace-only or duplicate names with trailing spaces to be created. Add validation at the beginning of the function to trim the name parameter and verify it is not empty after trimming, returning an appropriate error if validation fails. Apply the same normalization and validation logic to the other function around line 115 that also handles scim token names.
🧹 Nitpick comments (1)
packages/db/prisma/schema.prisma (1)
436-437: ⚡ Quick winRemove redundant uniqueness on primary key
hash.
hashis already unique because it is the model@id; keeping@uniqueis redundant and can lead to unnecessary duplicate index artifacts in migrations.Suggested schema cleanup
- hash String `@id` `@unique` + hash String `@id`🤖 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 `@packages/db/prisma/schema.prisma` around lines 436 - 437, Remove the redundant `@unique` annotation from the hash field definition in the Prisma schema. Since hash is already defined as the primary key with `@id`, the `@unique` annotation is unnecessary and can create duplicate indexes. Modify the hash field to only include `@id` `@unique`, removing the `@unique` directive entirely so it reads as hash String `@id`.
🤖 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 `@packages/db/prisma/schema.prisma`:
- Around line 410-413: Replace the non-unique index annotation @@index([orgId,
scimExternalId]) with a unique constraint @@unique([orgId, scimExternalId]) on
the scimExternalId field to enforce per-organization uniqueness of SCIM external
identifiers. This prevents duplicate IdP identities within the same org and
ensures unambiguous identity resolution in SCIM flows.
In `@packages/web/src/app/`(app)/layout.tsx:
- Around line 81-87: The gate logic currently only checks for the absence of a
membership but does not account for inactive memberships. When a membership row
exists with `isActive: false`, execution falls through and later assigns a role
from that inactive membership. Modify the membership validation check to ensure
that inactive memberships are treated the same as non-existent memberships by
adding a check for the `isActive` property. Update all conditions that check for
`!membership` to also verify `membership.isActive` is true, so that both missing
and inactive memberships are handled consistently as non-members in this gate.
In
`@packages/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx:
- Around line 102-110: The handleRevokeToken function calls the async
revokeScimToken action without a try/catch block, which means any exceptions
thrown by that function will escape unhandled and the user won't see an error
toast. Wrap the await revokeScimToken(name) call in a try/catch block, and in
the catch clause, display a destructive toast with an appropriate error message
to ensure all failure scenarios (both service errors and thrown exceptions) are
properly communicated to the user.
In `@packages/web/src/app/`(app)/settings/security/page.tsx:
- Around line 32-33: The SCIM token fetch logic in the security page converts
ServiceError results into an empty array, which masks retrieval failures as "no
tokens exist" in the UI. Instead of using the conditional assignment that checks
isServiceError(scimTokensResult) and defaults to an empty array, you need to
handle the error case separately so that the UI can distinguish between a
successful fetch with zero tokens versus a failed fetch. Create separate state
or variables to track both the scimTokens array and whether an error occurred
during the fetch, allowing the downstream UI to appropriately display either an
empty token list or an error message.
In `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts:
- Around line 62-75: The code currently updates the user profile via
prisma.user.update() before validating the membership state change via
applyActive(), which can lead to partial updates if the membership change fails.
Reorder the operations so that applyActive() is called before
prisma.user.update() to validate the membership state change constraints first,
or alternatively wrap both the prisma.user.update() and applyActive() calls in a
database transaction to ensure that either both operations succeed or both fail
atomically. Apply the same fix to the similar code block mentioned at lines
98-115.
- Around line 64-67: The prisma.user.update() calls in both the PUT handler
(lines 64-67) and PATCH handler (lines 99-105) lack error handling for unique
email constraint violations. Wrap both prisma.user.update() calls in try-catch
blocks and catch errors related to unique constraint violations on the email
field. When such an error occurs, return a SCIM 409 Conflict response with
scimType set to "uniqueness" to match the existing error handling pattern
already implemented in the POST handler. Reference the POST handler's error
handling approach as a template for the proper response format.
In `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/route.ts:
- Around line 21-25: The GET request handler is manually parsing query
parameters using parseInt, Math.max, and Math.min instead of using a Zod schema
for validation. Create a Zod schema that defines the shape and validation rules
for the query parameters (filter, startIndex, and count), including type
coercion and default values. Replace the manual parsing logic starting with the
params variable assignment through the count variable assignment with a single
Zod schema parse operation that validates and transforms the incoming
request.nextUrl.searchParams data, ensuring all validation logic is centralized
in the schema definition rather than scattered throughout the handler code.
- Around line 66-69: The current findUnique followed by create pattern for user
provisioning has a race condition where concurrent requests can both pass the
existence check before either executes the create, causing a unique constraint
violation on the email field. Replace the separate findUnique and create calls
with a single prisma.user.upsert operation that atomically checks for existence
and creates the user in one transaction, using email as the unique identifier
and providing both the create and update data objects with the email and name
fields.
In `@packages/web/src/app/invite/page.tsx`:
- Around line 54-55: The JoinOrganizationCard component is being rendered
without passing the validated inviteLinkId as a prop, which causes the
joinOrganization function to fail invite-link validation for organizations that
require member approval. Pass the inviteLinkId that should be available in the
page context as a prop to the JoinOrganizationCard component so it can be used
for proper validation when the user attempts to join the organization.
In `@packages/web/src/app/redeem/page.tsx`:
- Around line 45-48: The membership redirect check at the `if (membership)`
condition is catching both active and inactive memberships, including
SCIM-deactivated ones where isActive is false. This prevents the intended
not-provisioned flow from executing. Modify the condition to check not only that
membership exists but also that membership.isActive is true before redirecting
to the home page, ensuring only active members are redirected while deactivated
members proceed through the redemption flow.
In `@packages/web/src/ee/features/scim/actions.ts`:
- Around line 24-31: The getScimBaseUrl function and the other read operations
at lines 37-44 and 152-169 are implemented as server actions (wrapped with sew,
withAuth, withMinimumOrgRole), but they only perform data fetching without
mutations, which violates the project guideline that server actions should only
be used for POST/PUT/DELETE operations. Extract these read operations into
separate utility functions that return the data directly without server action
wrappers, then import and call them as regular async functions from client
components or other modules. Keep the authorization and entitlement checks, but
move them outside the server action pattern.
- Around line 80-103: Add a database-level unique constraint to prevent race
conditions in the SCIM token creation logic. In the ScimToken model located in
packages/db/prisma/schema.prisma, add a composite unique constraint using the
@@unique directive that combines the orgId and name fields. Then create a new
Prisma migration to apply this database constraint, which will ensure that the
pre-check validation in the create action is backed by database enforcement and
prevent concurrent requests from creating duplicate tokens with the same name
within the same organization.
In `@packages/web/src/ee/features/scim/schemas.ts`:
- Around line 22-30: The scimUserCreateSchema currently accepts emails through
the emails array without validating that they are actual email addresses before
they are persisted as user.email in downstream handlers. Update the
scimEmailSchema definition to include proper email format validation (ensure the
email field uses z.string().email() or equivalent). Additionally, locate the
other email validation location mentioned around line 65-66 and apply the same
email format validation to prevent blank or non-email identifiers from being
stored in the database.
In `@packages/web/src/features/membership/actions/accountRequests.ts`:
- Around line 65-125: Remove the redundant if (!existingRequest) conditional
check in the createAccountRequest function since an early return at line 57-63
already handles the case when existingRequest is truthy. Unindent all code that
was nested inside this conditional block by one level and remove the closing
brace at line 125. Additionally, in the approveAccountRequest function, add
deletion of the accountRequest record after successfully calling addMember
(around line 181) to match the cleanup pattern used in rejectAccountRequest,
ensuring that approved account requests do not leave orphaned records in the
database.
In `@packages/web/src/features/membership/components/submitJoinRequestCard.tsx`:
- Around line 18-45: The handleSubmit function in submitJoinRequestCard.tsx does
not have proper error handling for exceptions thrown by createAccountRequest().
If createAccountRequest() throws an error, setIsSubmitting(false) will never
execute, leaving the button locked. Refactor the handleSubmit function to wrap
the createAccountRequest() call and subsequent logic in a try/catch/finally
block, placing the setIsSubmitting(false) call in the finally block to ensure it
always executes. In the catch block, display an error toast with the caught
error message so users are informed of any unexpected failures.
In `@packages/web/src/features/membership/membership.service.ts`:
- Around line 47-49: The seat capacity check using orgHasAvailability() is
performed outside the database transaction in both the membership add and
reactivation code paths, allowing concurrent requests to bypass the seat limit.
Move the orgHasAvailability() check inside the same database transaction that
performs the actual insert or reactivate operation for the membership record,
ensuring the capacity validation and data persistence happen atomically
together. Apply this transactional pattern consistently to both the add path
(around the seatLimitReached() check) and the reactivation path to prevent race
conditions where multiple concurrent requests can each pass the check separately
and then both persist, exceeding the seat cap.
In `@packages/web/src/features/membership/onCreateUser.ts`:
- Around line 43-50: The first-user OWNER role assignment in the onCreateUser
function is prone to race conditions because it uses a non-atomic read of the
members array to determine if this is the first user. Replace the current
approach where __unsafePrisma.org.findUnique loads the full members array and
members.length is checked at line 76-81 with a single atomic transaction in the
membership layer that performs both a count query (instead of loading full
members) and the membership creation in one serializable unit. This ensures
concurrent user creations cannot both observe empty membership and both assign
OrgRole.OWNER.
---
Outside diff comments:
In `@packages/web/src/app/invite/page.tsx`:
- Around line 45-47: The redirect logic in the invite page only checks for the
existence of a membership object, but does not verify that it is active. Modify
the condition in the if statement that checks `membership` to also verify that
the membership's active status is true (check the isActive property). This
ensures that users with deactivated SCIM memberships are not incorrectly treated
as active members and redirected, maintaining consistency with the
authentication contract.
---
Minor comments:
In `@CHANGELOG.md`:
- Line 42: The SCIM 2.0 server provisioning entry is currently placed under the
[5.0.3] released version section in CHANGELOG.md. Move this entry from the
[5.0.3] section to the [Unreleased] section at the top of the changelog. The
entry should maintain its exact format and content, just relocated to comply
with the project's changelog policy that requires new PR entries to be added
under [Unreleased] until the next release is cut.
In `@packages/web/src/app/`(app)/settings/members/components/membersList.tsx:
- Line 217: The span element rendering member.name on line 217 of
membersList.tsx displays blank when the name property is missing or empty,
making member identification difficult. Modify the content of the span to use a
fallback expression that displays member.email when member.name is not
available, such as using a logical OR operator or conditional expression to
ensure every member row has a visible identifier.
In
`@packages/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx:
- Around line 125-127: The icon-only action buttons in the
scimProvisioningSettings component lack accessible labels for screen readers.
Add aria-label attributes to the Button components at handleCopyBaseUrl (around
line 125-127), the button around line 170-172, and the button around line
230-236 to provide descriptive accessible names that explain what each button
does when activated by assistive technology users.
In `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts:
- Around line 74-75: The non-null assertion on the `refreshed` variable returned
from loadMembership can cause a 500 error if a concurrent delete occurs between
the update and reload, when it should return a SCIM 404 instead. Remove the
non-null assertion operator (!) after `refreshed` in the return statement at the
scimJson call, and add a null check immediately after the loadMembership
assignment. If refreshed is null, return an appropriate SCIM 404 response,
otherwise proceed with toScimUser(refreshed) only when the value is confirmed to
be non-null. Apply this same fix at the other location mentioned in the comment
(lines 113-114).
In `@packages/web/src/ee/features/scim/actions.ts`:
- Around line 73-85: The generateScimToken function accepts the name parameter
without normalization or validation, allowing whitespace-only or duplicate names
with trailing spaces to be created. Add validation at the beginning of the
function to trim the name parameter and verify it is not empty after trimming,
returning an appropriate error if validation fails. Apply the same normalization
and validation logic to the other function around line 115 that also handles
scim token names.
In `@packages/web/src/ee/features/scim/withScimAuth.ts`:
- Around line 26-34: The authorization header check for the Bearer token scheme
is case-sensitive and only accepts "Bearer " with that exact capitalization,
which will reject valid IdP requests that use lowercase "bearer" or mixed case
variants. Convert the authorization header value to lowercase before checking if
it starts with "Bearer " (or check against "bearer " in lowercase), and adjust
the subsequent slice operation that extracts the bearer token to use the
lowercased authorization value to ensure the token extraction works correctly
regardless of the case used in the Authorization header.
In `@packages/web/src/features/membership/components/joinOrganizationCard.tsx`:
- Around line 56-63: In the joinOrganizationCard component, replace the CSS
variable syntax in the className attributes with direct Tailwind semantic color
classes. Change `text-[var(--muted-foreground)]` to `text-muted-foreground` in
the paragraph element and change `text-[var(--primary-foreground)]` to
`text-primary-foreground` in the Button component's className to align with the
coding guidelines that require TSX files to use Tailwind color classes directly
instead of CSS variable syntax.
In `@packages/web/src/features/membership/components/submitJoinRequestCard.tsx`:
- Around line 60-70: Replace all CSS variable utility syntax with Tailwind color
token classes in the submitJoinRequestCard component. Specifically, update the
className attributes in the svg element and the h1 and p elements to use
text-primary-foreground, text-foreground, and text-muted-foreground instead of
text-[var(--primary-foreground)], text-[var(--foreground)], and
text-[var(--muted-foreground)] respectively. This aligns with the repository's
coding standards for consistent use of Tailwind color classes rather than CSS
variable syntax.
---
Nitpick comments:
In `@packages/db/prisma/schema.prisma`:
- Around line 436-437: Remove the redundant `@unique` annotation from the hash
field definition in the Prisma schema. Since hash is already defined as the
primary key with `@id`, the `@unique` annotation is unnecessary and can create
duplicate indexes. Modify the hash field to only include `@id` `@unique`, removing
the `@unique` directive entirely so it reads as hash String `@id`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0afa80cc-c399-4e0f-9b31-b82624e66b85
📒 Files selected for processing (74)
CHANGELOG.mdpackages/db/prisma/migrations/20260619214548_add_scim_users_support/migration.sqlpackages/db/prisma/schema.prismapackages/shared/src/constants.tspackages/shared/src/crypto.tspackages/shared/src/entitlements.tspackages/shared/src/index.server.tspackages/web/next.config.mjspackages/web/src/__mocks__/prisma.tspackages/web/src/actions.tspackages/web/src/app/(app)/@sidebar/components/defaultSidebar/index.tsxpackages/web/src/app/(app)/components/submitAccountRequestButton.tsxpackages/web/src/app/(app)/components/submitJoinRequest.tsxpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/settings/components/settingsCard.tsxpackages/web/src/app/(app)/settings/layout.tsxpackages/web/src/app/(app)/settings/members/components/inviteMemberCard.tsxpackages/web/src/app/(app)/settings/members/components/invitesList.tsxpackages/web/src/app/(app)/settings/members/components/membersList.tsxpackages/web/src/app/(app)/settings/members/components/requestsList.tsxpackages/web/src/app/(app)/settings/members/page.tsxpackages/web/src/app/(app)/settings/security/components/inviteLinkEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/memberApprovalRequiredSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/scimEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/scimProvisioningSettings.tsxpackages/web/src/app/(app)/settings/security/components/scimUpsellCard.tsxpackages/web/src/app/(app)/settings/security/page.tsxpackages/web/src/app/api/(server)/ee/scim/v2/ResourceTypes/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Schemas/route.tspackages/web/src/app/api/(server)/ee/scim/v2/ServiceProviderConfig/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Users/[id]/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Users/route.tspackages/web/src/app/components/joinOrganizationButton.tsxpackages/web/src/app/components/joinOrganizationCard.tsxpackages/web/src/app/components/logoutEscapeHatch.tsxpackages/web/src/app/invite/actions.tspackages/web/src/app/invite/page.tsxpackages/web/src/app/redeem/components/acceptInviteCard.tsxpackages/web/src/app/redeem/page.tsxpackages/web/src/auth.tspackages/web/src/ee/features/audit/types.tspackages/web/src/ee/features/membership/actions.tspackages/web/src/ee/features/scim/actions.tspackages/web/src/ee/features/scim/constants.tspackages/web/src/ee/features/scim/mapper.tspackages/web/src/ee/features/scim/schemas.test.tspackages/web/src/ee/features/scim/schemas.tspackages/web/src/ee/features/scim/withScimAuth.tspackages/web/src/ee/features/sso/sso.tspackages/web/src/ee/features/userManagement/actions.tspackages/web/src/features/membership/actions/accountRequests.tspackages/web/src/features/membership/actions/index.tspackages/web/src/features/membership/actions/invites.tspackages/web/src/features/membership/actions/members.tspackages/web/src/features/membership/components/deactivatedMemberBadge.tsxpackages/web/src/features/membership/components/joinOrganizationCard.tsxpackages/web/src/features/membership/components/managedByScimBadge.tsxpackages/web/src/features/membership/components/managedByScimNotice.tsxpackages/web/src/features/membership/components/notProvisionedCard.tsxpackages/web/src/features/membership/components/pendingApprovalCard.tsxpackages/web/src/features/membership/components/submitJoinRequestCard.tsxpackages/web/src/features/membership/errors.tspackages/web/src/features/membership/logger.tspackages/web/src/features/membership/membership.service.test.tspackages/web/src/features/membership/membership.service.tspackages/web/src/features/membership/onCreateUser.tspackages/web/src/features/membership/utils.tspackages/web/src/features/scim/utils.tspackages/web/src/features/userManagement/actions.tspackages/web/src/lib/authUtils.tspackages/web/src/lib/errorCodes.tspackages/web/src/lib/posthogEvents.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.ts
💤 Files with no reviewable changes (8)
- packages/web/src/app/(app)/components/submitAccountRequestButton.tsx
- packages/web/src/app/(app)/components/submitJoinRequest.tsx
- packages/web/src/ee/features/userManagement/actions.ts
- packages/web/src/features/userManagement/actions.ts
- packages/web/src/app/components/joinOrganizationCard.tsx
- packages/web/src/app/components/joinOrganizationButton.tsx
- packages/web/src/app/invite/actions.ts
- packages/web/src/lib/authUtils.ts
Comment on lines
+410
to
+413
| scimExternalId String? | ||
|
|
||
| @@id([orgId, userId]) | ||
| @@index([orgId, scimExternalId]) |
Contributor
There was a problem hiding this comment.
Enforce per-org uniqueness for SCIM externalId.
scimExternalId currently has only a non-unique index, which permits duplicate IdP identities inside the same org and can make SCIM identity resolution/update flows ambiguous.
Suggested schema fix
- @@index([orgId, scimExternalId])
+ @@unique([orgId, scimExternalId])📝 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.
Suggested change
| scimExternalId String? | |
| @@id([orgId, userId]) | |
| @@index([orgId, scimExternalId]) | |
| scimExternalId String? | |
| @@id([orgId, userId]) | |
| @@unique([orgId, scimExternalId]) |
🤖 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 `@packages/db/prisma/schema.prisma` around lines 410 - 413, Replace the
non-unique index annotation @@index([orgId, scimExternalId]) with a unique
constraint @@unique([orgId, scimExternalId]) on the scimExternalId field to
enforce per-organization uniqueness of SCIM external identifiers. This prevents
duplicate IdP identities within the same org and ensures unambiguous identity
resolution in SCIM flows.
Comment on lines
+102
to
+110
| const handleRevokeToken = async (name: string) => { | ||
| const result = await revokeScimToken(name); | ||
| if (isServiceError(result)) { | ||
| toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" }); | ||
| return; | ||
| } | ||
| router.refresh(); | ||
| toast({ description: "SCIM token revoked" }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle revoke action exceptions explicitly.
Line 103 calls an async server action without a try/catch. If it throws, the rejection escapes and users get no failure toast.
Suggested fix
const handleRevokeToken = async (name: string) => {
- const result = await revokeScimToken(name);
- if (isServiceError(result)) {
- toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" });
- return;
- }
- router.refresh();
- toast({ description: "SCIM token revoked" });
+ try {
+ const result = await revokeScimToken(name);
+ if (isServiceError(result)) {
+ toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" });
+ return;
+ }
+ router.refresh();
+ toast({ description: "SCIM token revoked" });
+ } catch (error) {
+ console.error("Failed to revoke SCIM token:", error);
+ toast({ title: "Error", description: "Failed to revoke SCIM token", variant: "destructive" });
+ }
};📝 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.
Suggested change
| const handleRevokeToken = async (name: string) => { | |
| const result = await revokeScimToken(name); | |
| if (isServiceError(result)) { | |
| toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" }); | |
| return; | |
| } | |
| router.refresh(); | |
| toast({ description: "SCIM token revoked" }); | |
| }; | |
| const handleRevokeToken = async (name: string) => { | |
| try { | |
| const result = await revokeScimToken(name); | |
| if (isServiceError(result)) { | |
| toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" }); | |
| return; | |
| } | |
| router.refresh(); | |
| toast({ description: "SCIM token revoked" }); | |
| } catch (error) { | |
| console.error("Failed to revoke SCIM token:", error); | |
| toast({ title: "Error", description: "Failed to revoke SCIM token", variant: "destructive" }); | |
| } | |
| }; |
🤖 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
`@packages/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx
around lines 102 - 110, The handleRevokeToken function calls the async
revokeScimToken action without a try/catch block, which means any exceptions
thrown by that function will escape unhandled and the user won't see an error
toast. Wrap the await revokeScimToken(name) call in a try/catch block, and in
the catch clause, display a destructive toast with an appropriate error message
to ensure all failure scenarios (both service errors and thrown exceptions) are
properly communicated to the user.
Comment on lines
+32
to
+33
| const scimTokensResult = hasScimEntitlement ? await getScimTokens() : []; | ||
| const scimTokens = isServiceError(scimTokensResult) ? [] : scimTokensResult; |
Contributor
There was a problem hiding this comment.
Don’t collapse SCIM token fetch errors into an empty token list.
Lines 32-33 convert ServiceError to [], which makes downstream UI look like “no tokens exist” instead of “token retrieval failed.” That can lead owners to create unnecessary extra tokens.
Suggested fix
- const scimTokensResult = hasScimEntitlement ? await getScimTokens() : [];
- const scimTokens = isServiceError(scimTokensResult) ? [] : scimTokensResult;
+ const scimTokensResult = hasScimEntitlement ? await getScimTokens() : [];
+ const scimTokensError = isServiceError(scimTokensResult) ? scimTokensResult : null;
+ const scimTokens = scimTokensError ? [] : scimTokensResult;- {scimEnabled && (
+ {scimTokensError && (
+ <Alert className="items-center p-4">
+ <Info className="w-4 h-4 text-muted-foreground" />
+ <AlertDescription>{scimTokensError.message}</AlertDescription>
+ </Alert>
+ )}
+ {scimEnabled && !scimTokensError && (
<ScimProvisioningSettings baseUrl={scimBaseUrl} tokens={scimTokens} />
)}🤖 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 `@packages/web/src/app/`(app)/settings/security/page.tsx around lines 32 - 33,
The SCIM token fetch logic in the security page converts ServiceError results
into an empty array, which masks retrieval failures as "no tokens exist" in the
UI. Instead of using the conditional assignment that checks
isServiceError(scimTokensResult) and defaults to an empty array, you need to
handle the error case separately so that the UI can distinguish between a
successful fetch with zero tokens versus a failed fetch. Create separate state
or variables to track both the scimTokens array and whether an error occurred
during the fetch, allowing the downstream UI to appropriately display either an
empty token list or an error message.
Comment on lines
+62
to
+75
| const name = payload.name?.formatted ?? payload.displayName ?? undefined; | ||
| const email = resolveEmail(payload); | ||
| await prisma.user.update({ | ||
| where: { id }, | ||
| data: { name, email }, | ||
| }); | ||
|
|
||
| const activeError = await applyActive(org.id, id, membership.isActive, coerceActive(payload.active)); | ||
| if (activeError) { | ||
| return activeError; | ||
| } | ||
|
|
||
| const refreshed = await loadMembership(prisma, org.id, id); | ||
| return scimJson(toScimUser(refreshed!), 200); |
Contributor
There was a problem hiding this comment.
Prevent partial resource updates in PUT/PATCH.
Both handlers persist profile fields before setMemberActive. If membership state change fails (seat/owner constraints), the request returns an error after partially committing changes.
Also applies to: 98-115
🤖 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 `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts around
lines 62 - 75, The code currently updates the user profile via
prisma.user.update() before validating the membership state change via
applyActive(), which can lead to partial updates if the membership change fails.
Reorder the operations so that applyActive() is called before
prisma.user.update() to validate the membership state change constraints first,
or alternatively wrap both the prisma.user.update() and applyActive() calls in a
database transaction to ensure that either both operations succeed or both fail
atomically. Apply the same fix to the similar code block mentioned at lines
98-115.
Comment on lines
+22
to
+30
| export const scimUserCreateSchema = z.object({ | ||
| userName: z.string(), | ||
| externalId: z.string().optional(), | ||
| name: scimNameSchema.optional(), | ||
| emails: z.array(scimEmailSchema).optional(), | ||
| // `active` may arrive as a boolean or a string ("true"/"false"). | ||
| active: z.union([z.boolean(), z.string()]).optional(), | ||
| displayName: z.string().optional(), | ||
| }).passthrough(); |
Contributor
There was a problem hiding this comment.
Validate SCIM email identifiers before persisting.
Line 23 and Line 65 currently accept arbitrary strings and then persist them as user.email in downstream handlers. That allows blank/non-email identifiers to be stored, which can corrupt account identity semantics.
Proposed fix
export const scimUserCreateSchema = z.object({
- userName: z.string(),
+ userName: z.string().trim().min(1),
externalId: z.string().optional(),
name: scimNameSchema.optional(),
emails: z.array(scimEmailSchema).optional(),
// `active` may arrive as a boolean or a string ("true"/"false").
active: z.union([z.boolean(), z.string()]).optional(),
displayName: z.string().optional(),
-}).passthrough();
+}).passthrough().superRefine((data, ctx) => {
+ const candidate = data.emails?.find((e) => e.primary)?.value ?? data.emails?.[0]?.value ?? data.userName;
+ if (!candidate || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(candidate)) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ message: "A valid email is required in emails[].value or userName",
+ });
+ }
+});Also applies to: 63-66
🤖 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 `@packages/web/src/ee/features/scim/schemas.ts` around lines 22 - 30, The
scimUserCreateSchema currently accepts emails through the emails array without
validating that they are actual email addresses before they are persisted as
user.email in downstream handlers. Update the scimEmailSchema definition to
include proper email format validation (ensure the email field uses
z.string().email() or equivalent). Additionally, locate the other email
validation location mentioned around line 65-66 and apply the same email format
validation to prevent blank or non-email identifiers from being stored in the
database.
Comment on lines
+65
to
+125
| if (!existingRequest) { | ||
| await __unsafePrisma.accountRequest.create({ | ||
| data: { | ||
| requestedById: user.id, | ||
| orgId: org.id, | ||
| }, | ||
| }); | ||
|
|
||
| const smtpConnectionUrl = getSMTPConnectionURL(); | ||
| if (smtpConnectionUrl && env.EMAIL_FROM_ADDRESS) { | ||
| // TODO: This is needed because we can't fetch the origin from the request headers when this is called | ||
| // on user creation (the header isn't set when next-auth calls onCreateUser for some reason) | ||
| const deploymentUrl = env.AUTH_URL; | ||
|
|
||
| const owners = await __unsafePrisma.user.findMany({ | ||
| where: { | ||
| orgs: { | ||
| some: { | ||
| orgId: org.id, | ||
| role: "OWNER", | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| if (owners.length === 0) { | ||
| logger.error(`Failed to find any owners for org ${org.id} when drafting email for account request from ${user.id}`); | ||
| } else { | ||
| const html = await render(JoinRequestSubmittedEmail({ | ||
| baseUrl: deploymentUrl, | ||
| requestor: { | ||
| name: user.name ?? undefined, | ||
| email: user.email, | ||
| avatarUrl: user.image ?? undefined, | ||
| }, | ||
| orgName: org.name, | ||
| orgImageUrl: org.imageUrl ?? undefined, | ||
| })); | ||
|
|
||
| const ownerEmails = owners | ||
| .map((owner) => owner.email) | ||
| .filter((email): email is string => email !== null); | ||
|
|
||
| const transport = createTransport(smtpConnectionUrl); | ||
| const result = await transport.sendMail({ | ||
| to: ownerEmails, | ||
| from: env.EMAIL_FROM_ADDRESS, | ||
| subject: `New account request for ${org.name} on Sourcebot`, | ||
| html, | ||
| text: `New account request for ${org.name} on Sourcebot by ${user.name ?? user.email}`, | ||
| }); | ||
|
|
||
| const failed = result.rejected.concat(result.pending).filter(Boolean); | ||
| if (failed.length > 0) { | ||
| logger.error(`Failed to send account request email to ${ownerEmails.join(', ')}: ${failed}`); | ||
| } | ||
| } | ||
| } else { | ||
| logger.warn(`SMTP_CONNECTION_URL or EMAIL_FROM_ADDRESS not set. Skipping account request email to owner`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Redundant conditional and missing account request cleanup.
-
The
if (!existingRequest)check at line 65 is redundant since the function already returns at lines 57-63 whenexistingRequestis truthy. -
After successfully approving an account request (in
approveAccountRequest), theaccountRequestrecord is not deleted. UnlikerejectAccountRequestwhich properly deletes the record, approvals leave orphaned request records.
🔧 Proposed fixes
Remove redundant conditional in createAccountRequest:
- if (!existingRequest) {
- await __unsafePrisma.accountRequest.create({
+ await __unsafePrisma.accountRequest.create({And ensure the closing brace is also removed at line 125.
Add account request deletion in approveAccountRequest after successful addMember (around line 181):
if (isServiceError(addUserToOrgRes)) {
await failAuditCallback(addUserToOrgRes.message);
return addUserToOrgRes;
}
+ await prisma.accountRequest.delete({
+ where: {
+ id: requestId,
+ },
+ });
await createAudit({🤖 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 `@packages/web/src/features/membership/actions/accountRequests.ts` around lines
65 - 125, Remove the redundant if (!existingRequest) conditional check in the
createAccountRequest function since an early return at line 57-63 already
handles the case when existingRequest is truthy. Unindent all code that was
nested inside this conditional block by one level and remove the closing brace
at line 125. Additionally, in the approveAccountRequest function, add deletion
of the accountRequest record after successfully calling addMember (around line
181) to match the cleanup pattern used in rejectAccountRequest, ensuring that
approved account requests do not leave orphaned records in the database.
Comment on lines
+18
to
+45
| const handleSubmit = async () => { | ||
| setIsSubmitting(true) | ||
| const result = await createAccountRequest() | ||
| if (!isServiceError(result)) { | ||
| if (result.existingRequest) { | ||
| toast({ | ||
| title: "Request Already Submitted", | ||
| description: "Your request to join the organization has already been submitted. Please wait for it to be approved.", | ||
| variant: "default", | ||
| }) | ||
| } else { | ||
| toast({ | ||
| title: "Request Submitted", | ||
| description: "Your request to join the organization has been submitted.", | ||
| variant: "default", | ||
| }) | ||
| } | ||
| // Refresh the page to trigger layout re-render and show PendingApprovalCard | ||
| router.refresh() | ||
| } else { | ||
| toast({ | ||
| title: "Failed to Submit", | ||
| description: `There was an error submitting your request. Reason: ${result.message}`, | ||
| variant: "destructive", | ||
| }) | ||
| } | ||
| setIsSubmitting(false) | ||
| } |
Contributor
There was a problem hiding this comment.
Always clear submitting state when the action throws.
createAccountRequest() can throw; without try/finally, isSubmitting can stay true and lock the button.
Suggested fix
const handleSubmit = async () => {
- setIsSubmitting(true)
- const result = await createAccountRequest()
- if (!isServiceError(result)) {
- if (result.existingRequest) {
- toast({
- title: "Request Already Submitted",
- description: "Your request to join the organization has already been submitted. Please wait for it to be approved.",
- variant: "default",
- })
- } else {
- toast({
- title: "Request Submitted",
- description: "Your request to join the organization has been submitted.",
- variant: "default",
- })
- }
- // Refresh the page to trigger layout re-render and show PendingApprovalCard
- router.refresh()
- } else {
- toast({
- title: "Failed to Submit",
- description: `There was an error submitting your request. Reason: ${result.message}`,
- variant: "destructive",
- })
- }
- setIsSubmitting(false)
+ setIsSubmitting(true)
+ try {
+ const result = await createAccountRequest()
+ if (!isServiceError(result)) {
+ if (result.existingRequest) {
+ toast({
+ title: "Request Already Submitted",
+ description: "Your request to join the organization has already been submitted. Please wait for it to be approved.",
+ variant: "default",
+ })
+ } else {
+ toast({
+ title: "Request Submitted",
+ description: "Your request to join the organization has been submitted.",
+ variant: "default",
+ })
+ }
+ router.refresh()
+ } else {
+ toast({
+ title: "Failed to Submit",
+ description: `There was an error submitting your request. Reason: ${result.message}`,
+ variant: "destructive",
+ })
+ }
+ } finally {
+ setIsSubmitting(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 `@packages/web/src/features/membership/components/submitJoinRequestCard.tsx`
around lines 18 - 45, The handleSubmit function in submitJoinRequestCard.tsx
does not have proper error handling for exceptions thrown by
createAccountRequest(). If createAccountRequest() throws an error,
setIsSubmitting(false) will never execute, leaving the button locked. Refactor
the handleSubmit function to wrap the createAccountRequest() call and subsequent
logic in a try/catch/finally block, placing the setIsSubmitting(false) call in
the finally block to ensure it always executes. In the catch block, display an
error toast with the caught error message so users are informed of any
unexpected failures.
Comment on lines
+43
to
+50
| const defaultOrg = await __unsafePrisma.org.findUnique({ | ||
| where: { | ||
| id: SINGLE_TENANT_ORG_ID, | ||
| }, | ||
| include: { | ||
| members: true, | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
First-user OWNER bootstrap is race-prone and can create multiple owners.
Line 76 derives “first user” from a non-atomic read (members.length from Line 43). Concurrent user creations can both observe empty membership and both assign OrgRole.OWNER.
Move first-user role resolution into a single transaction in the membership layer (count + create in one serializable unit), and use a count query instead of loading full members.
Also applies to: 76-81
🤖 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 `@packages/web/src/features/membership/onCreateUser.ts` around lines 43 - 50,
The first-user OWNER role assignment in the onCreateUser function is prone to
race conditions because it uses a non-atomic read of the members array to
determine if this is the first user. Replace the current approach where
__unsafePrisma.org.findUnique loads the full members array and members.length is
checked at line 76-81 with a single atomic transaction in the membership layer
that performs both a count query (instead of loading full members) and the
membership creation in one serializable unit. This ensures concurrent user
creations cannot both observe empty membership and both assign OrgRole.OWNER.
Contributor
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 `@packages/web/src/features/membership/membership.service.ts`:
- Around line 40-47: In the ensureActiveMember function, when an existing active
membership is found (the first if condition checking existing &&
existing.isActive), add a check to see if the incoming scimExternalId differs
from the existing membership's scimExternalId. If they differ, delegate to
setMemberActive with the active flag set to true and the provided
scimExternalId, rather than returning the existing membership immediately. This
ensures the SCIM external ID is persisted consistently across all code paths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f28d6b43-479d-4001-b746-c114dfaf7c40
📒 Files selected for processing (21)
docs/api-reference/sourcebot-public.openapi.jsonpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/settings/license/page.tsxpackages/web/src/app/(app)/settings/members/page.tsxpackages/web/src/app/api/(server)/ee/askmcp/callback/route.tspackages/web/src/app/api/(server)/ee/chat/[chatId]/searchMembers/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Users/route.tspackages/web/src/app/api/(server)/ee/users/route.tspackages/web/src/app/invite/page.tsxpackages/web/src/app/onboard/page.tsxpackages/web/src/app/redeem/page.tsxpackages/web/src/features/billing/actions.tspackages/web/src/features/billing/servicePing.tspackages/web/src/features/chat/actions.tspackages/web/src/features/membership/actions/accountRequests.tspackages/web/src/features/membership/actions/invites.tspackages/web/src/features/membership/membership.service.test.tspackages/web/src/features/membership/membership.service.tspackages/web/src/features/membership/onCreateUser.tspackages/web/src/features/membership/utils.tspackages/web/src/openapi/publicApiSchemas.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/web/src/app/(app)/layout.tsx
- packages/web/src/app/redeem/page.tsx
- packages/web/src/app/api/(server)/ee/scim/v2/Users/route.ts
- packages/web/src/features/membership/onCreateUser.ts
- packages/web/src/features/membership/actions/invites.ts
- packages/web/src/features/membership/actions/accountRequests.ts
Comment on lines
+40
to
+47
| if (existing && existing.isActive) { | ||
| return existing; | ||
| } | ||
| if (existing && !existing.isActive) { | ||
| return setMemberActive(orgId, userId, true, { | ||
| actor, | ||
| scimExternalId | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist scimExternalId for existing active memberships.
When SCIM calls ensureActiveMember for a user who is already active, this returns before applying the incoming scimExternalId, unlike the create/reactivate paths and setMemberActive’s active refresh branch. Delegate to setMemberActive(..., true, ...) when the external ID differs so SCIM lookups stay consistent.
Proposed fix
if (existing && existing.isActive) {
+ if (scimExternalId && existing.scimExternalId !== scimExternalId) {
+ return setMemberActive(orgId, userId, true, {
+ actor,
+ scimExternalId,
+ });
+ }
return existing;
}🤖 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 `@packages/web/src/features/membership/membership.service.ts` around lines 40 -
47, In the ensureActiveMember function, when an existing active membership is
found (the first if condition checking existing && existing.isActive), add a
check to see if the incoming scimExternalId differs from the existing
membership's scimExternalId. If they differ, delegate to setMemberActive with
the active flag set to true and the provided scimExternalId, rather than
returning the existing membership immediately. This ensures the SCIM external ID
is persisted consistently across all code paths.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…mbers Add a per-membership lastActiveAt to UserToOrg, stamped in getAuthContext alongside the existing global User.lastActiveAt. The migration backfills it from User.lastActiveAt so existing members are not reset to "never active". Switch the service ping's DAU/WAU/MAU and billed user count onto the per-org timestamp. billedUserCount now counts only active (non-suspended) members who have been active in the org at least once, so provisioned-but-never-signed-in members no longer consume a seat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/web/src/middleware/withAuth.test.ts (1)
486-552: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for suspended activity updates.
The suspended-member tests should also assert
prisma.userToOrg.updateManyis not called, so the auth denial cannot still mark deprovisioned memberships active.Also applies to: 930-986
🤖 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 `@packages/web/src/middleware/withAuth.test.ts` around lines 486 - 552, The suspended-member auth tests in withAuth.test.ts currently verify role denial but do not cover the side effect on membership activity. Update the suspended-membership cases in getAuthContext to also assert prisma.userToOrg.updateMany is never called, including the API-key path, so suspended or deprovisioned memberships cannot be marked active when access is denied.
🤖 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 `@packages/web/src/app/`(app)/settings/apiKeys/layout.tsx:
- Around line 5-12: The layout props type in authenticatedPage currently
references React.ReactNode without React in scope, which will fail type-checking
under the current JSX settings. Update the apiKeys layout to import type
ReactNode and use that type for the children prop in the authenticatedPage
generic, keeping the existing SettingsContainer and notFound logic unchanged.
In `@packages/web/src/app/`(app)/settings/members/membersTable.tsx:
- Around line 367-382: Biome is flagging the intentional trigger dependencies in
the two `useLayoutEffect` hooks inside `membersTable.tsx`. Keep the current
scroll restore/top behavior in `membersTable` by adding targeted Biome
dependency ignores for the `sorting` effect and the `filter, searchQuery`
effect, rather than removing those dependencies. Make the ignores as narrow as
possible and keep the rest of the hook logic unchanged.
In `@packages/web/src/app/`(app)/settings/members/membersTableActions.tsx:
- Around line 285-312: The promote/demote actions in membersTableActions and the
corresponding membership mutations in actions.ts are still available for
SCIM-managed orgs, unlike the other membership changes. Add the same SCIM guard
used elsewhere so promoteToOwner and demoteToMember are disabled or blocked when
the org is SCIM-managed, and make sure the server-side actions also enforce this
restriction rather than relying only on the UI.
In `@packages/web/src/app/api/`(server)/ee/users/route.ts:
- Line 59: The EE user-list public API in the route backed by
publicEeUserListItemSchema now exposes suspendedAt instead of isActive, which is
a breaking response-shape change for consumers. Update the route and any related
schema references so the exported user list contract is consistent, then
regenerate the OpenAPI output with the web workspace openapi generation task and
refresh docs/api-reference/sourcebot-public.openapi.json. Also add a brief
migration/release-notes entry calling out that isActive was replaced by
suspendedAt.
In `@packages/web/src/features/billing/servicePing.ts`:
- Around line 48-65: The DAU/WAU/MAU queries in servicePing still count
suspended memberships because they only filter by lastActiveAt; update the count
conditions to use the same active-membership criteria as activeUserCount, likely
by reusing activeMembershipWhere() or adding an equivalent suspendedAt exclusion
alongside the existing orgId and activity cutoff checks in the servicePing
metrics query.
In `@packages/web/src/features/membership/membership.service.test.ts`:
- Around line 20-23: Add activeMembershipWhere to the
`@/features/membership/utils` mock because the current vi.mock replacement omits
it, causing countActiveOwners to see it as undefined during removeMember and
setMemberRole tests. Update the membership.service.test.ts mock around the
membership utils override so it either exports activeMembershipWhere alongside
orgHasAvailability and unsuspendedMembershipWhere, or spreads the real module
and overrides only the needed helpers, keeping countActiveOwners functional.
In `@packages/web/src/middleware/withAuth.ts`:
- Around line 113-115: Skip updating membership activity when the membership is
suspended. In withAuth, before calling updateMembershipLastActiveAt(membership),
add a guard using the membership record so suspended rows are ignored and only
active memberships update UserToOrg.lastActiveAt. Keep the change localized to
the membership handling block in withAuth and use the existing membership object
to detect suspension.
---
Nitpick comments:
In `@packages/web/src/middleware/withAuth.test.ts`:
- Around line 486-552: The suspended-member auth tests in withAuth.test.ts
currently verify role denial but do not cover the side effect on membership
activity. Update the suspended-membership cases in getAuthContext to also assert
prisma.userToOrg.updateMany is never called, including the API-key path, so
suspended or deprovisioned memberships cannot be marked active when access is
denied.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4af5ee55-42e0-4afc-879c-f28e8800efe1
📒 Files selected for processing (49)
docs/api-reference/sourcebot-public.openapi.jsonpackages/db/prisma/migrations/20260619214548_add_scim_users_support/migration.sqlpackages/db/prisma/migrations/20260624194710_add_lastactiveat_to_usertoorg/migration.sqlpackages/db/prisma/schema.prismapackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/settings/accountAskAgent/layout.tsxpackages/web/src/app/(app)/settings/analytics/layout.tsxpackages/web/src/app/(app)/settings/apiKeys/layout.tsxpackages/web/src/app/(app)/settings/components/settingsContainer.tsxpackages/web/src/app/(app)/settings/connections/layout.tsxpackages/web/src/app/(app)/settings/general/layout.tsxpackages/web/src/app/(app)/settings/layout.tsxpackages/web/src/app/(app)/settings/license/layout.tsxpackages/web/src/app/(app)/settings/license/page.tsxpackages/web/src/app/(app)/settings/linked-accounts/layout.tsxpackages/web/src/app/(app)/settings/mcp/layout.tsxpackages/web/src/app/(app)/settings/members/components/inviteMemberCard.tsxpackages/web/src/app/(app)/settings/members/components/invitesList.tsxpackages/web/src/app/(app)/settings/members/components/membersList.tsxpackages/web/src/app/(app)/settings/members/components/requestsList.tsxpackages/web/src/app/(app)/settings/members/layout.tsxpackages/web/src/app/(app)/settings/members/membersFilterSelect.tsxpackages/web/src/app/(app)/settings/members/membersTable.tsxpackages/web/src/app/(app)/settings/members/membersTableActions.tsxpackages/web/src/app/(app)/settings/members/membersTableView.tsxpackages/web/src/app/(app)/settings/members/page.tsxpackages/web/src/app/(app)/settings/security/layout.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/layout.tsxpackages/web/src/app/api/(server)/ee/askmcp/callback/route.tspackages/web/src/app/api/(server)/ee/chat/[chatId]/searchMembers/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Users/[id]/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Users/route.tspackages/web/src/app/api/(server)/ee/users/route.tspackages/web/src/app/invite/page.tsxpackages/web/src/app/onboard/page.tsxpackages/web/src/app/redeem/page.tsxpackages/web/src/components/ui/table.tsxpackages/web/src/ee/features/scim/mapper.tspackages/web/src/features/billing/actions.tspackages/web/src/features/billing/servicePing.tspackages/web/src/features/billing/types.tspackages/web/src/features/chat/actions.tspackages/web/src/features/membership/actions/members.tspackages/web/src/features/membership/membership.service.test.tspackages/web/src/features/membership/membership.service.tspackages/web/src/features/membership/utils.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.tspackages/web/src/openapi/publicApiSchemas.ts
💤 Files with no reviewable changes (4)
- packages/web/src/app/(app)/settings/members/components/inviteMemberCard.tsx
- packages/web/src/app/(app)/settings/members/components/membersList.tsx
- packages/web/src/app/(app)/settings/members/components/requestsList.tsx
- packages/web/src/app/(app)/settings/members/components/invitesList.tsx
✅ Files skipped from review due to trivial changes (5)
- packages/web/src/app/(app)/settings/accountAskAgent/layout.tsx
- packages/web/src/app/(app)/settings/security/layout.tsx
- packages/web/src/app/(app)/settings/mcp/layout.tsx
- packages/web/src/app/(app)/settings/linked-accounts/layout.tsx
- packages/web/src/features/billing/types.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/web/src/app/(app)/settings/layout.tsx
- packages/web/src/app/api/(server)/ee/askmcp/callback/route.ts
- packages/web/src/app/api/(server)/ee/chat/[chatId]/searchMembers/route.ts
- packages/web/src/app/onboard/page.tsx
- packages/web/src/app/invite/page.tsx
- packages/web/src/features/chat/actions.ts
- packages/web/src/app/(app)/layout.tsx
- packages/web/src/app/(app)/settings/license/page.tsx
- packages/web/src/ee/features/scim/mapper.ts
- packages/web/src/app/redeem/page.tsx
- packages/web/src/features/billing/actions.ts
- packages/web/src/app/api/(server)/ee/scim/v2/Users/route.ts
- packages/web/src/app/api/(server)/ee/scim/v2/Users/[id]/route.ts
Comment on lines
+5
to
+12
| import { SettingsContainer } from "../components/settingsContainer"; | ||
|
|
||
| export default authenticatedPage<{ children: React.ReactNode }>(async ({ role }, { children }) => { | ||
| if (env.DISABLE_API_KEY_USAGE_FOR_NON_OWNER_USERS === 'true' && role !== OrgRole.OWNER) { | ||
| return notFound(); | ||
| } | ||
|
|
||
| return <>{children}</>; | ||
| return <SettingsContainer>{children}</SettingsContainer>; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo "== tsconfig React namespace settings =="
fd -a 'tsconfig.*' . -E node_modules -E .next -x sh -c '
echo "--- $1 ---"
rg -n "allowUmdGlobalAccess|\"jsx\"|jsxImportSource" "$1" || true
' sh {}
echo
echo "== New settings layouts using React.ReactNode =="
rg -n --glob 'packages/web/src/app/**/layout.tsx' 'React\.ReactNode' packages/web/src/app || true
echo
echo "== Matching imports in those layouts =="
rg -n --glob 'packages/web/src/app/**/layout.tsx' '^import React from "react"|^import type \{ ReactNode \} from "react"' packages/web/src/app || trueRepository: sourcebot-dev/sourcebot
Length of output: 3885
🏁 Script executed:
#!/bin/bash
set -eu
echo "== packages/web/src/app/(app)/settings/apiKeys/layout.tsx =="
cat -n packages/web/src/app/(app)/settings/apiKeys/layout.tsx
echo
echo "== packages/web/tsconfig.json =="
cat -n packages/web/tsconfig.json
echo
echo "== packages/web/src/app/(app)/settings/general/layout.tsx =="
cat -n packages/web/src/app/(app)/settings/general/layout.tsxRepository: sourcebot-dev/sourcebot
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -eu
echo "== packages/web/src/app/(app)/settings/apiKeys/layout.tsx =="
cat -n packages/web/src/app/(app)/settings/apiKeys/layout.tsx
echo
echo "== packages/web/tsconfig.json =="
cat -n packages/web/tsconfig.json
echo
echo "== packages/web/src/app/(app)/settings/general/layout.tsx =="
cat -n packages/web/src/app/(app)/settings/general/layout.tsxRepository: sourcebot-dev/sourcebot
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -eu
echo "== packages/web/src/app/(app)/settings/apiKeys/layout.tsx =="
cat -n 'packages/web/src/app/(app)/settings/apiKeys/layout.tsx'
echo
echo "== packages/web/tsconfig.json =="
cat -n 'packages/web/tsconfig.json'
echo
echo "== nearby settings layouts =="
for f in \
'packages/web/src/app/(app)/settings/general/layout.tsx' \
'packages/web/src/app/(app)/settings/mcp/layout.tsx' \
'packages/web/src/app/(app)/settings/security/layout.tsx' \
'packages/web/src/app/(app)/settings/workspaceAskAgent/layout.tsx'
do
echo "--- $f ---"
cat -n "$f"
doneRepository: sourcebot-dev/sourcebot
Length of output: 3440
🏁 Script executed:
#!/bin/bash
set -eu
for f in \
'packages/web/src/app/(app)/settings/connections/layout.tsx' \
'packages/web/src/app/(app)/layout.tsx' \
'packages/web/src/app/layout.tsx'
do
echo "--- $f ---"
cat -n "$f"
echo
doneRepository: sourcebot-dev/sourcebot
Length of output: 18153
Import ReactNode for this layout props type
React.ReactNode is used here without an in-scope React import. With jsx: "react-jsx" and no allowUmdGlobalAccess, this will fail type-checking. Import type { ReactNode } and use that instead.
Suggested fix
+import type { ReactNode } from "react";
import { notFound } from "next/navigation";
import { OrgRole } from "`@sourcebot/db`";
import { env } from "`@sourcebot/shared`";
import { authenticatedPage } from "`@/middleware/authenticatedPage`";
import { SettingsContainer } from "../components/settingsContainer";
-export default authenticatedPage<{ children: React.ReactNode }>(async ({ role }, { children }) => {
+export default authenticatedPage<{ children: ReactNode }>(async ({ role }, { children }) => {📝 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.
Suggested change
| import { SettingsContainer } from "../components/settingsContainer"; | |
| export default authenticatedPage<{ children: React.ReactNode }>(async ({ role }, { children }) => { | |
| if (env.DISABLE_API_KEY_USAGE_FOR_NON_OWNER_USERS === 'true' && role !== OrgRole.OWNER) { | |
| return notFound(); | |
| } | |
| return <>{children}</>; | |
| return <SettingsContainer>{children}</SettingsContainer>; | |
| import type { ReactNode } from "react"; | |
| import { notFound } from "next/navigation"; | |
| import { OrgRole } from "`@sourcebot/db`"; | |
| import { env } from "`@sourcebot/shared`"; | |
| import { authenticatedPage } from "`@/middleware/authenticatedPage`"; | |
| import { SettingsContainer } from "../components/settingsContainer"; | |
| export default authenticatedPage<{ children: ReactNode }>(async ({ role }, { children }) => { | |
| if (env.DISABLE_API_KEY_USAGE_FOR_NON_OWNER_USERS === 'true' && role !== OrgRole.OWNER) { | |
| return notFound(); | |
| } | |
| return <SettingsContainer>{children}</SettingsContainer>; |
🤖 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 `@packages/web/src/app/`(app)/settings/apiKeys/layout.tsx around lines 5 - 12,
The layout props type in authenticatedPage currently references React.ReactNode
without React in scope, which will fail type-checking under the current JSX
settings. Update the apiKeys layout to import type ReactNode and use that type
for the children prop in the authenticatedPage generic, keeping the existing
SettingsContainer and notFound logic unchanged.
Comment on lines
+367
to
+382
| useLayoutEffect(() => { | ||
| if (scrollTopBeforeSortRef.current == null || !scrollContainerRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| scrollContainerRef.current.scrollTop = scrollTopBeforeSortRef.current; | ||
| scrollTopBeforeSortRef.current = null; | ||
| }, [sorting]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!scrollContainerRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| scrollContainerRef.current.scrollTop = 0; | ||
| }, [filter, searchQuery]); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Biome flags these useLayoutEffect deps as errors (likely CI-blocking).
Biome reports sorting (Line 367) and filter, searchQuery (Line 376) as unnecessary dependencies. They are actually intentional triggers (scroll-position restore on sort; scroll-to-top on filter/search), so removing them would break behavior. Add targeted ignores to keep intent and unblock the lint gate.
🔧 Suggested ignores
+ // biome-ignore lint/correctness/useExhaustiveDependencies: `sorting` is the intended trigger for scroll restoration.
useLayoutEffect(() => {
if (scrollTopBeforeSortRef.current == null || !scrollContainerRef.current) {
return;
}
scrollContainerRef.current.scrollTop = scrollTopBeforeSortRef.current;
scrollTopBeforeSortRef.current = null;
}, [sorting]);
+ // biome-ignore lint/correctness/useExhaustiveDependencies: `filter`/`searchQuery` are the intended triggers for scroll reset.
useLayoutEffect(() => {
if (!scrollContainerRef.current) {
return;
}
scrollContainerRef.current.scrollTop = 0;
}, [filter, searchQuery]);📝 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.
Suggested change
| useLayoutEffect(() => { | |
| if (scrollTopBeforeSortRef.current == null || !scrollContainerRef.current) { | |
| return; | |
| } | |
| scrollContainerRef.current.scrollTop = scrollTopBeforeSortRef.current; | |
| scrollTopBeforeSortRef.current = null; | |
| }, [sorting]); | |
| useLayoutEffect(() => { | |
| if (!scrollContainerRef.current) { | |
| return; | |
| } | |
| scrollContainerRef.current.scrollTop = 0; | |
| }, [filter, searchQuery]); | |
| // biome-ignore lint/correctness/useExhaustiveDependencies: `sorting` is the intended trigger for scroll restoration. | |
| useLayoutEffect(() => { | |
| if (scrollTopBeforeSortRef.current == null || !scrollContainerRef.current) { | |
| return; | |
| } | |
| scrollContainerRef.current.scrollTop = scrollTopBeforeSortRef.current; | |
| scrollTopBeforeSortRef.current = null; | |
| }, [sorting]); | |
| // biome-ignore lint/correctness/useExhaustiveDependencies: `filter`/`searchQuery` are the intended triggers for scroll reset. | |
| useLayoutEffect(() => { | |
| if (!scrollContainerRef.current) { | |
| return; | |
| } | |
| scrollContainerRef.current.scrollTop = 0; | |
| }, [filter, searchQuery]); |
🧰 Tools
🪛 Biome (2.5.0)
[error] 367-367: This hook specifies more dependencies than necessary: sorting.
(lint/correctness/useExhaustiveDependencies)
[error] 376-376: This hook specifies more dependencies than necessary: filter, searchQuery.
(lint/correctness/useExhaustiveDependencies)
🤖 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 `@packages/web/src/app/`(app)/settings/members/membersTable.tsx around lines
367 - 382, Biome is flagging the intentional trigger dependencies in the two
`useLayoutEffect` hooks inside `membersTable.tsx`. Keep the current scroll
restore/top behavior in `membersTable` by adding targeted Biome dependency
ignores for the `sorting` effect and the `filter, searchQuery` effect, rather
than removing those dependencies. Make the ignores as narrow as possible and
keep the rest of the hook logic unchanged.
Source: Linters/SAST tools
Comment on lines
+285
to
+312
| {row.kind === "member" && ( | ||
| <> | ||
| {row.role === OrgRole.MEMBER && ( | ||
| <DropdownMenuItem | ||
| className="cursor-pointer" | ||
| disabled={!hasOrgManagement} | ||
| title={!hasOrgManagement ? "Organization management is not available in your current plan" : undefined} | ||
| onClick={() => setConfirmingAction("promote")} | ||
| > | ||
| Promote to owner | ||
| </DropdownMenuItem> | ||
| )} | ||
| {row.role === OrgRole.OWNER && ( | ||
| <DropdownMenuItem | ||
| className="cursor-pointer text-destructive" | ||
| disabled={!hasOrgManagement || isLastActiveOwner} | ||
| title={ | ||
| !hasOrgManagement | ||
| ? "Organization management is not available in your current plan" | ||
| : isLastActiveOwner | ||
| ? "Cannot demote the last active owner" | ||
| : undefined | ||
| } | ||
| onClick={() => setConfirmingAction("demote")} | ||
| > | ||
| Demote to member | ||
| </DropdownMenuItem> | ||
| )} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether promote/demote server actions include a SCIM guard.
fd 'actions.ts' packages/web/src/ee/features/membership --exec sed -n '1,200p' {}
rg -nP 'isScimEnabled|membershipManagedByIdpError|promoteToOwner|demoteToMember' packages/web/src/ee/features/membershipRepository: sourcebot-dev/sourcebot
Length of output: 2543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect role-changing service and compare with other membership actions for SCIM gating.
ast-grep outline packages/web/src/ee/features/membership/actions.ts --view expanded || true
ast-grep outline packages/web/src/features/membership/membership.service.ts --view expanded || true
rg -nP 'isScimEnabled|scimEnabled|membershipManagedByIdpError|setMemberRole|removeMember|suspendMember|reactivateMember|approveRequest|leaveMember' packages/web/src/ee/features/membership packages/web/src/features/membershipRepository: sourcebot-dev/sourcebot
Length of output: 7769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant slices if the service file is not too large.
wc -l packages/web/src/ee/features/membership/actions.ts packages/web/src/features/membership/membership.service.ts
sed -n '1,220p' packages/web/src/ee/features/membership/actions.ts
sed -n '1,260p' packages/web/src/features/membership/membership.service.tsRepository: sourcebot-dev/sourcebot
Length of output: 10382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect role-changing service and compare with other membership actions for SCIM gating.
ast-grep outline packages/web/src/ee/features/membership/actions.ts --view expanded || true
ast-grep outline packages/web/src/features/membership/membership.service.ts --view expanded || true
rg -nP 'isScimEnabled|scimEnabled|membershipManagedByIdpError|setMemberRole|removeMember|suspendMember|reactivateMember|approveRequest|leaveMember' packages/web/src/ee/features/membership packages/web/src/features/membershipRepository: sourcebot-dev/sourcebot
Length of output: 7769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant files if they exist and are reasonably small.
wc -l packages/web/src/ee/features/membership/actions.ts packages/web/src/features/membership/membership.service.ts
sed -n '1,220p' packages/web/src/ee/features/membership/actions.ts
sed -n '1,260p' packages/web/src/features/membership/membership.service.tsRepository: sourcebot-dev/sourcebot
Length of output: 10382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether setMemberRole or its callers apply a SCIM/IdP restriction.
rg -nP 'export (async )?function setMemberRole|const setMemberRole|setMemberRole\(' packages/web/src/features/membership packages/web/src/ee/features/membership packages/web/src
sed -n '1,260p' packages/web/src/features/membership/membership.service.tsRepository: sourcebot-dev/sourcebot
Length of output: 10574
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the repo for any SCIM/IdP gating around member role changes.
rg -nP 'isScimEnabled|scimEnabled|membershipManagedByIdpError|IdP.*member|member.*IdP|setMemberRole' packages/web/src packages/db/srcRepository: sourcebot-dev/sourcebot
Length of output: 10965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '250,350p' packages/web/src/app/(app)/settings/members/membersTableActions.tsxRepository: sourcebot-dev/sourcebot
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '250,350p' packages/web/src/app/(app)/settings/members/membersTableActions.tsxRepository: sourcebot-dev/sourcebot
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '250,355p' 'packages/web/src/app/(app)/settings/members/membersTableActions.tsx'Repository: sourcebot-dev/sourcebot
Length of output: 6181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n 'packages/web/src/app/(app)/settings/members/membersTableActions.tsx' | sed -n '260,355p'Repository: sourcebot-dev/sourcebot
Length of output: 6325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n 'packages/web/src/ee/features/membership/actions.ts' | sed -n '1,120p'
cat -n 'packages/web/src/features/membership/actions/members.ts' | sed -n '1,140p'Repository: sourcebot-dev/sourcebot
Length of output: 7219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'SCIM|roles?|owner|member' packages/web/src/features/membership packages/web/src/app/'(app)'/settings/members packages/web/src/ee/features/scimRepository: sourcebot-dev/sourcebot
Length of output: 42911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' packages/web/src/features/membership/errors.ts
sed -n '1,140p' packages/web/src/features/membership/actions/members.ts
sed -n '1,140p' packages/web/src/features/membership/actions/accountRequests.tsRepository: sourcebot-dev/sourcebot
Length of output: 10911
Add the SCIM guard to promote/demote. promoteToOwner and demoteToMember still work for SCIM-managed orgs here and in packages/web/src/ee/features/membership/actions.ts, unlike the other membership mutations. If role changes are meant to stay outside IdP control, leave this as-is; otherwise block them server-side too.
🤖 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 `@packages/web/src/app/`(app)/settings/members/membersTableActions.tsx around
lines 285 - 312, The promote/demote actions in membersTableActions and the
corresponding membership mutations in actions.ts are still available for
SCIM-managed orgs, unlike the other membership changes. Add the same SCIM guard
used elsewhere so promoteToOwner and demoteToMember are disabled or blocked when
the org is SCIM-managed, and make sure the server-side actions also enforce this
restriction rather than relying only on the UI.
| name: membership.user.name, | ||
| email: membership.user.email, | ||
| role: membership.role, | ||
| suspendedAt: membership.suspendedAt, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Breaking public API change: isActive replaced by suspendedAt.
This is an exported EE user-list endpoint backed by publicEeUserListItemSchema. Dropping isActive in favor of suspendedAt is a breaking change for existing API consumers. Ensure the generated OpenAPI doc (docs/api-reference/sourcebot-public.openapi.json) is regenerated and that this change is called out in release notes/migration docs.
As per path instructions, public API route schema changes require running yarn workspace @sourcebot/web openapi:generate and updating the docs.
🤖 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 `@packages/web/src/app/api/`(server)/ee/users/route.ts at line 59, The EE
user-list public API in the route backed by publicEeUserListItemSchema now
exposes suspendedAt instead of isActive, which is a breaking response-shape
change for consumers. Update the route and any related schema references so the
exported user list contract is consistent, then regenerate the OpenAPI output
with the web workspace openapi generation task and refresh
docs/api-reference/sourcebot-public.openapi.json. Also add a brief
migration/release-notes entry calling out that isActive was replaced by
suspendedAt.
Source: Path instructions
Comment on lines
+48
to
65
| __unsafePrisma.userToOrg.count({ | ||
| where: { | ||
| orgId, | ||
| }, | ||
| }), | ||
| __unsafePrisma.user.count({ | ||
| where: { | ||
| orgs: { some: { orgId } }, | ||
| lastActiveAt: { gte: dauCutoff }, | ||
| }, | ||
| }), | ||
| __unsafePrisma.user.count({ | ||
| __unsafePrisma.userToOrg.count({ | ||
| where: { | ||
| orgs: { some: { orgId } }, | ||
| orgId, | ||
| lastActiveAt: { gte: wauCutoff }, | ||
| }, | ||
| }), | ||
| __unsafePrisma.user.count({ | ||
| __unsafePrisma.userToOrg.count({ | ||
| where: { | ||
| orgs: { some: { orgId } }, | ||
| orgId, | ||
| lastActiveAt: { gte: mauCutoff }, | ||
| }, | ||
| }), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
DAU/WAU/MAU counts include suspended memberships.
activeUserCount filters via activeMembershipWhere() (suspended excluded), but the DAU/WAU/MAU queries gate only on lastActiveAt thresholds. Since suspension sets suspendedAt without clearing lastActiveAt, a recently-suspended member still counts toward DAU/WAU/MAU, which is inconsistent with the active-user semantics. Confirm whether suspended members should be excluded from activity metrics here.
Proposed adjustment
__unsafePrisma.userToOrg.count({
where: {
orgId,
+ ...unsuspendedMembershipWhere(),
lastActiveAt: { gte: dauCutoff },
},
}),(apply to WAU/MAU as well)
📝 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.
Suggested change
| __unsafePrisma.userToOrg.count({ | |
| where: { | |
| orgId, | |
| }, | |
| }), | |
| __unsafePrisma.user.count({ | |
| where: { | |
| orgs: { some: { orgId } }, | |
| lastActiveAt: { gte: dauCutoff }, | |
| }, | |
| }), | |
| __unsafePrisma.user.count({ | |
| __unsafePrisma.userToOrg.count({ | |
| where: { | |
| orgs: { some: { orgId } }, | |
| orgId, | |
| lastActiveAt: { gte: wauCutoff }, | |
| }, | |
| }), | |
| __unsafePrisma.user.count({ | |
| __unsafePrisma.userToOrg.count({ | |
| where: { | |
| orgs: { some: { orgId } }, | |
| orgId, | |
| lastActiveAt: { gte: mauCutoff }, | |
| }, | |
| }), | |
| __unsafePrisma.userToOrg.count({ | |
| where: { | |
| orgId, | |
| ...unsuspendedMembershipWhere(), | |
| lastActiveAt: { gte: dauCutoff }, | |
| }, | |
| }), | |
| __unsafePrisma.userToOrg.count({ | |
| where: { | |
| orgId, | |
| ...unsuspendedMembershipWhere(), | |
| lastActiveAt: { gte: wauCutoff }, | |
| }, | |
| }), | |
| __unsafePrisma.userToOrg.count({ | |
| where: { | |
| orgId, | |
| ...unsuspendedMembershipWhere(), | |
| lastActiveAt: { gte: mauCutoff }, | |
| }, | |
| }), |
🤖 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 `@packages/web/src/features/billing/servicePing.ts` around lines 48 - 65, The
DAU/WAU/MAU queries in servicePing still count suspended memberships because
they only filter by lastActiveAt; update the count conditions to use the same
active-membership criteria as activeUserCount, likely by reusing
activeMembershipWhere() or adding an equivalent suspendedAt exclusion alongside
the existing orgId and activity cutoff checks in the servicePing metrics query.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/middleware/withAuth.ts (1)
119-130: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRe-read membership before granting access after pending activation.
roleis computed from the pre-transaction row. If the guarded activation updates 0 rows because the membership was concurrently suspended/deleted, this still refreshes activity and returns the stale role. HaveactivatePendingMembershipreturn the activated/current row or re-fetch the membership here before derivingrole.🤖 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 `@packages/web/src/middleware/withAuth.ts` around lines 119 - 130, The access flow in withAuth is using a stale membership row after activatePendingMembership, so role can be granted from pre-transaction data. Update the withAuth path to re-read the membership after activation, or have activatePendingMembership return the current activated row, and then derive role only from that refreshed membership before calling updateMembershipLastActiveAt or returning access.
🤖 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.
Outside diff comments:
In `@packages/web/src/middleware/withAuth.ts`:
- Around line 119-130: The access flow in withAuth is using a stale membership
row after activatePendingMembership, so role can be granted from pre-transaction
data. Update the withAuth path to re-read the membership after activation, or
have activatePendingMembership return the current activated row, and then derive
role only from that refreshed membership before calling
updateMembershipLastActiveAt or returning access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52a663a3-17e6-42c6-b2d4-099de06aa499
📒 Files selected for processing (6)
packages/web/src/app/error.tsxpackages/web/src/features/membership/components/orgAtCapacityCard.tsxpackages/web/src/features/membership/membership.service.test.tspackages/web/src/features/membership/membership.service.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/middleware/withAuth.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a SCIM 2.0 server (EE, gated by the new
scimentitlement) so an identity provider (Okta, Entra) can provision and deprovision Sourcebot members.Scope
/scim/v2Users endpoints: discovery, list+filter, create, get, replace, PATCH (activetoggle), delete. Groups deferred.UserToOrg.isActive): forces logout viasessionVersionand revokes API/OAuth tokens, but preserves the row so the IdP can reactivate.ScimToken(bearer auth viawithScimAuth); managed under Settings → Security.Note: the
scimentitlement must be added to the lighthouse entitlements list and deployed before online licenses will grant it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/scim/v2route.Bug Fixes