Repository navigation
System and Tenant Unification - #59
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughSystem invitations now attach to the system tenant ID instead of Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant BetterAuth as Better Auth
participant Orchestration as Signup-Orchestration
participant TenantProvider as Tenant Provider
participant EmailAPI as Internal Email API
User->>BetterAuth: POST /sign-up/email
BetterAuth->>BetterAuth: create user (response includes user)
BetterAuth->>Orchestration: invoke after /sign-up/email hook
Orchestration->>TenantProvider: findPendingInvitation(email)
alt pending invitation exists
TenantProvider-->>Orchestration: true
Orchestration->>BetterAuth: skip verification email
else no pending invitation
TenantProvider-->>Orchestration: false
Orchestration->>EmailAPI: POST /email/verification (trigger)
EmailAPI->>User: send verification email
end
Orchestration-->>BetterAuth: hook complete
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/identity/system-admin/system-admin.controller.ts (1)
70-74:⚠️ Potential issue | 🟡 MinorStale comment: still references
OrgId = null.Line 70 comment says
// Create System Invitation (OrgId = null)but the code now passesgetRequiredSystemTenantId(). Update the comment to reflect the new behavior.📝 Proposed fix
- // Create System Invitation (OrgId = null) + // Create System Invitation (System Tenant)
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 550-551: Add a new unit test in better-auth.adapter.spec.ts that
covers the system-tenant branch of acceptInvitation by using an invitation
object with organizationId: null (e.g., clone or extend validInv with
organizationId = null) and asserting the expected behavior: it checks for
existing membership and inserts into the system tenant (mock the membership
lookup to return both "exists" and "not exists" cases to cover duplicate
membership rejection and successful insertion). Target the acceptInvitation
method and the test setup variables named validInv and any
membership-repository/method mocks so the test verifies the duplicate membership
check and the insertion into the system tenant path.
- Around line 617-652: The test currently relies on the real "better-auth/api"
behavior (specifically createAuthMiddleware) which may wrap or transform
handlers; to make the test robust, explicitly mock "better-auth/api" and stub
createAuthMiddleware to be a no-op/pass-through so the extracted handler from
getBetterAuthPlugins (look up the plugin with id "signup-orchestration", inspect
orchPlugin.hooks.after[0].handler assigned to myHandler) can be invoked
directly; update the test setup to vi.mock("better-auth/api", ...) and ensure
createAuthMiddleware returns the handler unchanged (or returns a simple wrapper
that calls the original handler) so the pending-invite vs no-invite assertions
remain reliable.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 318-328: findPendingInvitation currently compares emails with
eq(t.email, email) which is case-sensitive; normalize the input before querying
by trimming and lowercasing (e.g., compute normalizedEmail =
email.toLowerCase().trim()) and use that in the DB query condition (replace
eq(t.email, email) with eq(t.email, normalizedEmail)) while keeping the other
predicates (eq(t.status, "pending"), gt(t.expiresAt, new Date())) unchanged.
In `@packages/identity/src/better-auth.config.ts`:
- Around line 158-207: The after-hook calls tenantProvider.findPendingInvitation
and invokes api.sendVerificationEmail without error handling, so any thrown
error will bubble up and break signup; wrap the logic inside
ctx.context.runInBackgroundOrAwait (including the await
tenantProvider.findPendingInvitation and the internal api.sendVerificationEmail
call) in a try-catch, log the caught error (e.g., ctx.context.logger.error or
processLogger.error) with context (mentioning
findPendingInvitation/sendVerificationEmail and the user email), and swallow the
error so the signup flow continues (matching the existing
tenantProvisioningPlugin pattern).
- Around line 167-204: Remove the stream-of-consciousness comments between the
start of the background task and the API call and replace them with one concise
rationale sentence stating why the internal API is invoked (e.g., "Invoke
internal sendVerificationEmail endpoint to reuse existing verification token and
email logic when running outside the normal signup flow"). Keep the surrounding
logic intact (ctx.context.runInBackgroundOrAwait, the type-cast to access api,
the call to api.sendVerificationEmail with body: { email: user.email } and
headers: ctx.request.headers) and ensure only a brief explanatory comment
remains.
- Around line 189-198: The current unsafe cast to access ctx.context.api
(creating const api = (ctx.context as unknown as {...}).api) can break silently
if better-auth changes; add a runtime guard before using that cast: verify ctx
and ctx.context exist, that ctx.context.api is present, and that
api.sendVerificationEmail is a function (e.g., typeof check) and if the guard
fails, log/throw a clear error message explaining the internal API is
unavailable and include context info so the failure is visible at runtime; then
only perform the cast and call sendVerificationEmail when the guard passes to
avoid silent runtime failures in better-auth.config.ts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/identity/src/adapters/better-auth.adapter.spec.ts (1)
605-687: 🧹 Nitpick | 🔵 TrivialTest name doesn't reflect full scope.
This
itblock (line 605) is titled"configures better-auth callbacks correctly (emails, hashing)"but now also tests the signup-orchestration hook (lines 652-686). Consider updating the description to reflect the broader coverage, e.g.,"configures better-auth callbacks correctly (emails, hashing, orchestration)".
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 651-686: The test relies on
mockCtx.context.api.sendVerificationEmail state from the previous assertion, so
clear the mock between the two cases to ensure Case B's assertion proves the
call came from that branch; update the test around the orchestration handler
invocation by calling mockCtx.context.api.sendVerificationEmail.mockClear() (or
reset) after the Case A assertion, referencing the same myHandler, mockCtx, and
controlledProvider.findPendingInvitation used in the existing test.
In `@packages/identity/src/better-auth.config.ts`:
- Around line 137-222: The signup-orchestration handler currently logs raw PII
(user.email) and inconsistently references the email variable; change logging to
avoid storing full emails by logging a non-PII identifier (preferably user.id)
or a deterministic masked value (e.g., truncated or hashed email) whenever
creating the JSON error objects in the catch and fallback branches, and use the
already-assigned const email = user.email variable everywhere inside the handler
instead of referencing user.email directly so the value usage is consistent
(update the JSON payload keys in the error/failure console.error calls and any
other logging around tenantProvider.findPendingInvitation and
api.sendVerificationEmail invocations).
Comment on lines
+651
to
+686
|
|
||
| // 4. Verify Orchestration Hook Logic | ||
| const controlledProvider = mkTenantProvider(); | ||
| const plugins = getBetterAuthPlugins(email as any, cfg(), controlledProvider as any); | ||
| const orchPlugin = plugins.find((p: any) => p.id === "signup-orchestration"); | ||
| if (!orchPlugin) throw new Error("Orchestration plugin not found"); | ||
| // @ts-ignore - We know the structure from the config | ||
| const myHandler = orchPlugin.hooks.after[0].handler; | ||
|
|
||
| const mockCtx = { | ||
| context: { | ||
| returned: { user: { email: "test@example.com" } }, | ||
| options: { | ||
| emailVerification: { sendVerificationEmail: true }, | ||
| }, | ||
| api: { | ||
| sendVerificationEmail: vi.fn(), | ||
| }, | ||
| runInBackgroundOrAwait: async (fn: any) => fn(), | ||
| }, | ||
| request: { headers: {} }, | ||
| }; | ||
|
|
||
| // Case A: Pending Invite -> Suppress | ||
| controlledProvider.findPendingInvitation.mockResolvedValue(true); | ||
| await myHandler(mockCtx); | ||
| expect(mockCtx.context.api.sendVerificationEmail).not.toHaveBeenCalled(); | ||
|
|
||
| // Case B: No Invite -> Send Email | ||
| controlledProvider.findPendingInvitation.mockResolvedValue(false); | ||
| await myHandler(mockCtx); | ||
| expect(mockCtx.context.api.sendVerificationEmail).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| body: { email: "test@example.com" }, | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Orchestration hook tests are well-structured and cover both branches.
The mock context correctly simulates the plugin's runtime environment, including the runInBackgroundOrAwait synchronous execution for testability. Both the suppression (pending invite) and send (no invite) paths are verified.
Minor suggestion: consider resetting the sendVerificationEmail mock between Case A and Case B (mockCtx.context.api.sendVerificationEmail.mockClear()) to make the assertion at line 682 strictly prove Case B triggered the call, rather than relying on it not being called in Case A.
Optional improvement
// Case B: No Invite -> Send Email
controlledProvider.findPendingInvitation.mockResolvedValue(false);
+ mockCtx.context.api.sendVerificationEmail.mockClear();
await myHandler(mockCtx);
expect(mockCtx.context.api.sendVerificationEmail).toHaveBeenCalledWith(🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts` around lines 651
- 686, The test relies on mockCtx.context.api.sendVerificationEmail state from
the previous assertion, so clear the mock between the two cases to ensure Case
B's assertion proves the call came from that branch; update the test around the
orchestration handler invocation by calling
mockCtx.context.api.sendVerificationEmail.mockClear() (or reset) after the Case
A assertion, referencing the same myHandler, mockCtx, and
controlledProvider.findPendingInvitation used in the existing test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/identity/src/better-auth.config.ts`:
- Around line 168-187: The repeated nested type-assertion around ctx.context to
access api.sendVerificationEmail should be consolidated: add a small guarded
extractor (e.g., a helper function like getApiFromContext or assertHasApi) that
checks ctx.request, ctx.context, "api" in ctx.context and typeof
ctx.context.api.sendVerificationEmail === "function", then returns the typed api
object; replace the duplicated guard and the later cast in the block around ctx
and api/sendVerificationEmail with a single call to that helper so you only
perform the runtime check once and use the returned typed api for calling
sendVerificationEmail.
- Around line 193-203: The fallback error message in the else branch that logs
signup_orchestration_failure is misleading because the compound guard (checking
ctx.request, ctx.context, "api" in ctx.context, and typeof sendVerificationEmail
=== "function") can fail for multiple reasons; update the else branch that
currently references only "Internal API sendVerificationEmail not available" to
inspect those individual conditions (ctx.request === null/undefined, ctx.context
missing, missing "api" key in ctx.context, or sendVerificationEmail not being a
function) and log a clear, specific message including which condition failed
along with user.id and timestamp so the log accurately reflects the real cause
of the failure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/identity/src/better-auth.config.ts`:
- Around line 184-186: The matcher function for the plugin compares context.path
directly without a null-guard; update the matcher (the anonymous function
assigned to matcher that accepts HookEndpointContext) to first check if
context.path is present (e.g., if (!context.path) return false;) and then
perform the equality check against "/sign-up/email" so it matches the null-guard
style used by tenantProvisioningPlugin.
- Around line 30-57: getApiFromContext currently checks safeCtx.request which
mixes "request present" concern with "api present" and makes null ambiguous;
update getApiFromContext to only inspect safeCtx.context?.api and the
presence/type of sendVerificationEmail (keep the same return shape referencing
sendVerificationEmail and SafeContext) and remove the safeCtx.request check so
callers (e.g. the code that also checks ctx.request) can distinguish missing
request vs missing api; ensure the function still returns the same typed api or
null and update any callers to rely on their existing ctx.request guard rather
than expecting getApiFromContext to cover request availability.
pramodnarayana
marked this pull request as draft
February 16, 2026 15:44
pramodnarayana
marked this pull request as ready for review
February 16, 2026 15:44
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.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests