Open
Conversation
Contributor
WalkthroughThis PR introduces a comprehensive billing and licensing system with support for seat-based subscriptions, trial management, and offline license keys. It includes database migrations for license tracking, frontend UI for license management and billing, integration with a Lighthouse service for sync/checkout/invoicing, refactored entitlements evaluation, and conversion of entitlement checks to async operations throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant WebApp as Web App<br/>(authenticatedPage)
participant License as License<br/>Service
participant Prisma as Database
participant Lighthouse as Lighthouse<br/>Service
Browser->>WebApp: Request org settings
WebApp->>Prisma: Get org + license
WebApp->>License: Check entitlements<br/>(async)
License->>Prisma: Load license by orgId
Prisma-->>License: Return license
License-->>WebApp: Entitlements resolved
WebApp->>Prisma: Get recent invoices
Prisma-->>WebApp: Return data
WebApp->>Browser: Render license settings UI<br/>+ banners
Browser->>WebApp: Activate license code
WebApp->>Lighthouse: POST /ping + activation code
Lighthouse-->>WebApp: Return license + entitlements
WebApp->>Prisma: Create/update license record
Prisma-->>WebApp: Success
WebApp->>Lighthouse: POST /ping<br/>(sync license state)
Lighthouse-->>WebApp: Confirm sync
WebApp-->>Browser: Success, refresh page
sequenceDiagram
participant User as End User
participant App as Web App<br/>(BannerSlot)
participant Resolver as Banner<br/>Resolver
participant Prisma as Database
participant Lighthouse as Service Ping<br/>Cron
Lighthouse->>Prisma: Update license<br/>lastSyncAt on success
Prisma-->>Lighthouse: Confirm
User->>App: Load app (authenticated)
App->>Prisma: Get org + license<br/>+ offline license
Prisma-->>App: Return data
App->>Resolver: Resolve active banner<br/>(priority, audience, dismissal)
Resolver->>Resolver: Evaluate conditions:<br/>license expired,<br/>invoice past due,<br/>trial state,<br/>ping staleness,<br/>permission sync pending
Resolver-->>App: Active BannerDescriptor
App->>App: Render banner component<br/>(LicenseExpiredBanner,<br/>TrialBanner, etc.)
App-->>User: Display banner UI
User->>App: Click "Manage license"
App-->>User: Navigate to<br/>/settings/license
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
brendan-kellam
force-pushed
the
bkellam/lighthouse
branch
from
97bb793 to
c99b3d0
Compare
User was hitting a unique constraint on UserToOrg(orgId, userId) when redeeming an invite, because onCreateUser auto-joins new signups in self-serve mode and redeemInvite then tried to create the same row. Make the insert idempotent via upsert so the downstream AccountRequest and invite cleanup still runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds cancelAt to the License model and the lighthouse ping schema, and renders "Cancels on <date>" on the current plan card when there's no upcoming renewal. Prefers "Next renewal" when Stripe still has an upcoming invoice — so subscriptions scheduled to end after the next billing cycle keep showing the renewal row. Also makes nextRenewalAt / nextRenewalAmount nullable to match the lighthouse response, and guards new Date() against null in servicePing. Adds a CLAUDE.md under the lighthouse feature folder pointing at the service repo so the two schemas stay in lockstep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renders a dedicated card for offline (SOURCEBOT_EE_LICENSE_KEY) licenses showing the license id, seat cap, and expiry. When an offline license is present, the page skips the online license lookup entirely to mirror the precedence in entitlements.ts. Also adds a header row with a mailto link to support and an "All plans" shortcut to the public pricing page. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces a priority-ordered, single-slot banner system under (app)/components/banners/. A server-side resolver picks the highest-priority banner that matches the current context (role, license state, offline license, permission-sync status) and renders it through a shared BannerShell that handles per-day dismissal via cookies. Banners included: - License expired (everyone, non-dismissible, role-aware copy) - License expiry heads-up (owner, dismissible, 14d window, uses formatDistance for relative copy) - Invoice past due (owner, non-dismissible) - Permission sync pending (everyone, non-dismissible, migrated from the prior standalone component through BannerShell) Precedence mirrors entitlements.ts: offline license is the sole source of truth when present, so online billing state is ignored. Also splits getValidOfflineLicense into a decode-only path so getOfflineLicenseMetadata can surface expired licenses to the UI. Includes bannerResolver.test.ts covering priority, audience filtering, dismissal filtering, offline/online expiry rules, and permission sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds trialEnd to the ping response schema and installId / requestTrial to the checkout request schema so types match the lighthouse service. createCheckoutSession passes the instance's SOURCEBOT_INSTALL_ID and defaults requestTrial to false (the existing "upgrade" button is not a trial path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a trial banner to the owner-facing banner stack and the
surrounding plumbing:
- Schema: License.trialEnd, License.hasPaymentMethod, Org.trialUsedAt
(durable flag that survives license deactivation). Two migrations.
- servicePing persists trialEnd / hasPaymentMethod and flips
Org.trialUsedAt on first trial sync.
- Trial banner (owner, dismissible, priority 25): title uses
formatDistance ("Your trial ends in 10 days"); copy + action branch
on hasPaymentMethod. With-PM variant links to /settings/license;
no-PM variant opens the Stripe portal via a new
OpenBillingPortalButton (LoadingButton + createPortalSession).
- currentPlanCard gains a "Trial ends on" fallback column for the
trial-without-PM case (where nextRenewalAt is null).
- activationCodeCard accepts isTrialEligible and flips its checkout
button from "Purchase a license" to "Start a free trial" when the
org hasn't trialed yet, passing requestTrial through to the checkout
endpoint.
- Types mirror the new lighthouse fields (trialEnd, hasPaymentMethod)
and the checkout request additions (installId, requestTrial).
Side-trips to Stripe (portal, checkout) now append ?refresh=true so
the license resyncs on return; trial-checkout also appends
?trial_used=true so Org.trialUsedAt flips immediately (closes the UX
gap between checkout completion and activation-code entry). page.tsx
handles both params, preserves any other query params, and redirects
to a clean URL.
Also: fetchWithRetry now only retries 5xx, 408, and 429 — 4xx errors
(e.g. TRIAL_ALREADY_USED at 409) propagate immediately instead of
retrying pointlessly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
syncWithLighthouse previously swallowed ping errors with a log and early
return, which masked real problems. Flip the model: on a lighthouse
ServiceError response, throw ServiceErrorException. The sew() middleware
already knows how to marshal that into an API response for user-initiated
paths.
Callers fall into two camps:
- Propagate (user-initiated): activateLicense and refreshLicense. The
existing try/catch in activateLicense now correctly rolls back the
license row when lighthouse rejects the activation code; refreshLicense
lets the throw propagate so the UI surfaces a toast.
- Swallow explicitly (background / side-effect): license page load, the
24h cron, user-approval, and signup paths all wrap with
`.catch(() => { /* ignore */ })`. These happen as a side effect of
other successful operations; a ping failure shouldn't block them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 13
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 (4)
CHANGELOG.md (1)
8-13:⚠️ Potential issue | 🟡 MinorChangelog entry appears to under-describe the PR scope.
Based on the PR objectives, this PR introduces a substantial feature set (banner system, Lighthouse integration, trial/billing flows, seat-based subscriptions, offline license support,
SOURCEBOT_LIGHTHOUSE_URL,Org.trialUsedAt, etc.), but the only[Unreleased]entry covers the offline-license crash fix. Consider adding entries underAdded/Changedfor the user-facing/operator-facing additions (e.g.,[EE]banner system, trial/billing UI, Lighthouse service integration, new env vars), so downstream consumers don't miss them in the release notes.As per coding guidelines: "Every PR must include a follow-up commit adding an entry to CHANGELOG.md under [Unreleased]. ... Prefix enterprise-only features with
[EE]".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 8 - 13, Update the CHANGELOG.md [Unreleased] section to reflect the full scope of the PR rather than only the offline-license fix: add entries under "Added" and "Changed" that mention the banner system (mark enterprise-only with [EE] per guidelines), Lighthouse integration and the new SOURCEBOT_LIGHTHOUSE_URL env var, trial/billing UI and flows (reference Org.trialUsedAt), seat-based subscriptions, and offline license support/behavior change; ensure each user-facing or operator-facing feature (e.g., banner system, Lighthouse service, trial/billing, seat subscriptions) is clearly listed and EE-only items are prefixed with [EE] so downstream consumers see them in release notes.packages/web/src/app/api/(server)/ee/user/route.ts (1)
115-133:⚠️ Potential issue | 🟡 MinorWrite the
user.deleteaudit after the delete succeeds.
createAudit({ action: "user.delete", ... })is emitted beforeprisma.user.delete(...). If the delete throws (e.g., FK constraint, DB error), the catch on line 145 rethrows but the audit record is already persisted, producing a false "user deleted" entry. Move thecreateAuditcall below the successfulprisma.user.deleteto match the pattern used by the other audits in this PR (e.g.,chat.deletedinpackages/web/src/features/chat/actions.ts).🛠️ Proposed fix
- await createAudit({ - action: "user.delete", - actor: { - id: currentUser.id, - type: "user" - }, - target: { - id: userId, - type: "user" - }, - orgId: org.id, - }); - // Delete the user (cascade will handle all related records) await prisma.user.delete({ where: { id: userId, }, }); + + await createAudit({ + action: "user.delete", + actor: { id: currentUser.id, type: "user" }, + target: { id: userId, type: "user" }, + orgId: org.id, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/ee/user/route.ts around lines 115 - 133, The audit entry for "user.delete" is being created before the actual deletion so failures produce false positives; move the createAudit call that constructs action: "user.delete" (currently using createAudit and referencing currentUser.id, userId, org.id) to after the successful await prisma.user.delete({ where: { id: userId } }) call so the audit is only persisted on success, matching the pattern used by chat.deleted in packages/web/src/features/chat/actions.ts; ensure you keep the same actor/target/org payload and error handling unchanged.packages/web/src/auth.ts (1)
260-281:⚠️ Potential issue | 🟡 Minor
getIssuerUrlForAccountre-resolves all providers on every JWT callback — amplified by the lazy migration loop.The
jwtcallback runs on every token refresh/verify. When a user has one or more accounts withoutissuerUrl(the lazy migration path),getIssuerUrlForAccountis invoked in a loop, and each invocation now callsawait getProviders()— which in turn callsawait hasEntitlement("sso")and potentiallyawait getEEIdentityProviders(). That's N (accounts) × entitlement-check + SSO-factory construction per token refresh.Consider hoisting a single
await getProviders()outside the loop, or caching the provider list at the module level (since provider configuration doesn't change per-request anyway).🔧 Proposed fix
if (token.userId) { const accountsWithoutIssuerUrl = await __unsafePrisma.account.findMany({ where: { userId: token.userId, issuerUrl: null, }, }); - for (const account of accountsWithoutIssuerUrl) { - const issuerUrl = await getIssuerUrlForAccount(account); + if (accountsWithoutIssuerUrl.length > 0) { + const providers = await getProviders(); + for (const account of accountsWithoutIssuerUrl) { + const issuerUrl = getIssuerUrlForAccountFromProviders(account, providers); ... + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/auth.ts` around lines 260 - 281, The jwt callback's lazy migration loop calls getIssuerUrlForAccount for each account which internally calls getProviders() causing repeated entitlement checks; hoist a single await getProviders() before iterating accounts (or retrieve/cached providers at module level) and pass that provider list into getIssuerUrlForAccount (or add an overload/param) so the loop reuses the same providers instead of calling getProviders() N times; update references to getIssuerUrlForAccount and any callers in the jwt callback to accept and use the pre-fetched providers.packages/web/src/lib/authUtils.ts (1)
105-121:⚠️ Potential issue | 🟠 MajorTOCTOU race on
orgHasAvailability→userToOrg.createcan exceed the seat cap.
orgHasAvailability(defaultOrg.id)runs afindUniqueOrThrow(count-based) outside any transaction, then__unsafePrisma.userToOrg.createruns separately on Lines 114–120. Two concurrent sign-ups can both observememberCount < seatCap, then both insert, pushing the org above the cap. Same pattern inaddUserToOrganization(Lines 186–212): thehasAvailabilitycheck happens before the transaction and the transaction never re-checks under a lock. Consider moving the availability check and membership insert into a single$transaction(withSerializableisolation, or acountinside the transaction followed by the insert), or relying on a DB-level seat-count constraint.🛠️ Sketch of a fix (onCreateUser)
else if (!defaultOrg.memberApprovalRequired) { - // Don't exceed the licensed seat count. The user row still exists; - // they just aren't attached to the org until a seat frees up. - const hasAvailability = await orgHasAvailability(defaultOrg.id); - if (!hasAvailability) { - logger.warn(`onCreateUser: org ${SINGLE_TENANT_ORG_ID} has reached max capacity. User ${user.id} was not added to the org.`); - return; - } - - await __unsafePrisma.userToOrg.create({ - data: { - userId: user.id, - orgId: SINGLE_TENANT_ORG_ID, - role: OrgRole.MEMBER, - } - }); + // Don't exceed the licensed seat count. Availability + insert must be + // atomic to prevent TOCTOU over-allocation under concurrent signups. + const added = await __unsafePrisma.$transaction(async (tx) => { + const seatCap = getSeatCap(); + if (seatCap) { + const memberCount = await tx.userToOrg.count({ where: { orgId: defaultOrg.id } }); + if (memberCount >= seatCap) { + return false; + } + } + await tx.userToOrg.create({ + data: { userId: user.id!, orgId: SINGLE_TENANT_ORG_ID, role: OrgRole.MEMBER }, + }); + return true; + }, { isolationLevel: 'Serializable' }); + + if (!added) { + logger.warn(`onCreateUser: org ${SINGLE_TENANT_ORG_ID} has reached max capacity. User ${user.id} was not added to the org.`); + return; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/authUtils.ts` around lines 105 - 121, The org seat-check and membership create are vulnerable to a TOCTOU race: replace the separate orgHasAvailability(...) call followed by __unsafePrisma.userToOrg.create(...) in onCreateUser and the same pattern in addUserToOrganization with a single atomic operation (use a Prisma $transaction that does the count/check and the create inside the same transaction with Serializable isolation, or perform the count and insert inside the same transaction and re-check capacity before inserting), or alternatively enforce a DB-level seat-count constraint and handle the unique/constraint error on create; update code paths referencing orgHasAvailability, __unsafePrisma.userToOrg.create, onCreateUser, and addUserToOrganization to use the transactional approach and surface a clear error/log when capacity is exceeded.
🟡 Minor comments (12)
packages/web/src/ee/features/lighthouse/CLAUDE.md-9-11 (1)
9-11:⚠️ Potential issue | 🟡 MinorSpecify a language on the fenced code block (MD040).
markdownlint flags this fence as missing a language. Since the content is a path/route hint, use
text(or a placeholder likeplaintext) for consistency.📝 Proposed fix
-``` +```text lighthouse: lambda/routes/<route>.ts</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/web/src/ee/features/lighthouse/CLAUDE.mdaround lines 9 - 11, The
fenced code block in CLAUDE.md is missing a language tag; update the
triple-backtick fence that contains the text "lighthouse:
lambda/routes/.ts" to include a language identifier (e.g., text or
plaintext) so the block becomestext ..., ensuring markdownlint MD040 is
satisfied.</details> </blockquote></details> <details> <summary>packages/db/prisma/migrations/20260417224042_remove_guest_org_role/migration.sql-12-21 (1)</summary><blockquote> `12-21`: _⚠️ Potential issue_ | _🟡 Minor_ **Explicit BEGIN/COMMIT inside a Prisma-wrapped migration — confirm this is intentional.** Prisma runs each migration inside its own transaction; adding an explicit `BEGIN;` / `COMMIT;` in the SQL will emit a Postgres warning (`there is already a transaction in progress`) and the inner `COMMIT` closes the outer transaction, so statements on lines 8–10 run *inside* the outer transaction while the enum swap effectively commits the whole thing mid-migration. This is the pattern Prisma's own enum-removal generator emits, so it's likely fine, but please double-check that the pre-transaction `DELETE`s (lines 9–10) behave as expected if the enum swap later fails — they will already be committed by line 21. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/db/prisma/migrations/20260417224042_remove_guest_org_role/migration.sql` around lines 12 - 21, The migration contains explicit BEGIN;/COMMIT; which conflicts with Prisma's transaction wrapper and causes the enum swap to commit mid-migration; remove the explicit BEGIN and COMMIT so the statements (CREATE TYPE "OrgRole_new", ALTER TABLE "UserToOrg" ALTER COLUMN "role" ..., ALTER TYPE ... RENAME, DROP TYPE, ALTER TABLE ... SET DEFAULT) all run inside Prisma's transaction, or alternatively move any pre-transaction DELETEs into the same transaction after the enum swap; update the SQL to drop the BEGIN/COMMIT wrappers (affecting the migration's CREATE TYPE "OrgRole_new", ALTER TABLE "UserToOrg" ALTER COLUMN "role", and ALTER TYPE "OrgRole" RENAME steps) so the migration is executed atomically by Prisma. ``` </details> </blockquote></details> <details> <summary>packages/web/src/app/(app)/components/banners/actions.ts-6-15 (1)</summary><blockquote> `6-15`: _⚠️ Potential issue_ | _🟡 Minor_ **Validate `id` at runtime before writing a cookie.** `BannerId` is a compile-time type only; server actions receive untrusted runtime input. As written, a caller can pass any string and set an arbitrary cookie name under the `DISMISS_COOKIE_PREFIX`. It's low-impact (scoped to the caller's own session and prefixed), but adding a runtime check against the known `BannerId` list (same one used in `bannerSlot.tsx`) avoids growing a surface that later readers may trust. <details> <summary>🛡️ Suggested guard</summary> ```diff export async function dismissBanner(id: BannerId) { + if (!KNOWN_BANNER_IDS.includes(id)) { + return; + } const cookieStore = await cookies(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`(app)/components/banners/actions.ts around lines 6 - 15, The dismissBanner server action accepts an untrusted runtime id (BannerId is compile-time only) and must validate it before writing a cookie; update dismissBanner to check the incoming id against the canonical list of valid banner ids (the same list used in bannerSlot.tsx) and return or throw on invalid values, then only call cookies() and cookieStore.set(`${DISMISS_COOKIE_PREFIX}${id}`, ...) for validated ids; reference the DISMISS_COOKIE_PREFIX constant and the dismissBanner function when applying the guard so the runtime validation mirrors the bannerSlot.tsx source of truth. ``` </details> </blockquote></details> <details> <summary>packages/web/src/app/(app)/components/banners/trialBanner.tsx-17-28 (1)</summary><blockquote> `17-28`: _⚠️ Potential issue_ | _🟡 Minor_ **Title reads awkwardly once the trial end date has passed.** `formatDistance(..., { addSuffix: true })` returns either `"in X days"` or `"X days ago"`. If `trialEnd` is in the past for any reason (e.g., the resolver hasn't yet recomputed, or there's a small clock skew between server-rendered `now` and actual `trialEnd`), the title renders as "Your trial ends 2 hours ago", which is grammatically wrong. Consider branching on whether `trialEndDate > now` and using a different phrase (e.g., "Your trial ended X ago") for the past case, or guarding the banner from rendering once `trialEnd` is in the past. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`(app)/components/banners/trialBanner.tsx around lines 17 - 28, The title currently uses formatDistance(trialEndDate, now, { addSuffix: true }) which yields "in X" or "X ago" causing grammar like "Your trial ends X ago"; change logic in trialBanner.tsx to detect whether trialEndDate > now (e.g., const isFuture = trialEndDate > now) and then set the BannerShell title accordingly (e.g., title={isFuture ? `Your trial ends ${relative}` : `Your trial ended ${formatDistance(trialEndDate, now)}`) or alternatively skip rendering the BannerShell when the trial is already past; update references to relative, trialEndDate, now, and the BannerShell title prop to implement this branch. ``` </details> </blockquote></details> <details> <summary>packages/web/src/lib/utils.ts-602-621 (1)</summary><blockquote> `602-621`: _⚠️ Potential issue_ | _🟡 Minor_ **`fetchWithRetry` doesn't respect `AbortSignal` cancellation.** If the caller passes an `AbortSignal` via `init`, an abort during a `fetch` will throw an `AbortError` which is caught here and treated like any other transient failure — the function will sleep and retry until `retries` is exhausted, defeating the cancellation. Consider rethrowing immediately on abort: <details> <summary>🛠 Proposed fix</summary> ```diff } catch (error) { + if (init?.signal?.aborted || (error instanceof DOMException && error.name === 'AbortError')) { + throw error; + } if (attempt === retries) { throw error; } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/utils.ts` around lines 602 - 621, fetchWithRetry currently swallows AbortErrors and keeps retrying; update it to respect AbortSignal by (1) checking if init?.signal?.aborted before each attempt and immediately throwing that signal's AbortError, and (2) in the catch block for the fetch inside fetchWithRetry, rethrow immediately if the caught error is an AbortError (e.g., error.name === 'AbortError' or error instanceof DOMException with name 'AbortError') instead of treating it as a transient failure; keep the existing retry/backoff behavior for other errors and non-abort failures. ``` </details> </blockquote></details> <details> <summary>packages/web/src/auth.ts-297-297 (1)</summary><blockquote> `297-297`: _⚠️ Potential issue_ | _🟡 Minor_ **Top-level `await` at module init: Confirm tsconfig supports transpilation and address runtime entitlement limitation.** `providers: (await getProviders()).map(...)` uses top-level `await` in a module-level `NextAuth()` call. While your tsconfig targets ES2017 (which lacks native top-level await support), Next.js 16.2.3 with SWC handles transpilation automatically, so this works in practice. The real issue: `getProviders()` is called once at module initialization. Entitlement changes at runtime (e.g., SSO entitlement enabled via license update) won't reflect until the Next.js server restarts. If dynamic entitlement support is required, move provider resolution into a dynamic path (e.g., a route handler or server action) rather than caching at init. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/auth.ts` at line 297, The module calls getProviders() with a top-level await inside the NextAuth() configuration (providers: (await getProviders()).map(...)), causing provider resolution to happen once at module initialization and not reflect runtime entitlement changes; refactor so provider resolution occurs per-request by removing the top-level await and moving the getProviders() call into a dynamic handler (e.g., a route handler or server action) or into the request-time part of NextAuth configuration so getProviders() is invoked on each request (look for the NextAuth() call and the providers property and replace the static module-init mapping with an async per-request resolution). ``` </details> </blockquote></details> <details> <summary>packages/shared/src/entitlements.test.ts-208-216 (1)</summary><blockquote> `208-216`: _⚠️ Potential issue_ | _🟡 Minor_ **Minor: the "boundary" test isn't actually at the boundary.** `lastSyncAt` is `Date.now() - STALE_THRESHOLD_MS + 1000`, i.e. 1 second inside the threshold — not exactly on it. If the intent is to pin the `<=` semantics of the comparison, set it to `Date.now() - STALE_THRESHOLD_MS` (or add a second case at `- STALE_THRESHOLD_MS - 1` to show the flip). Otherwise the comment reads stronger than what the test actually asserts. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/entitlements.test.ts` around lines 208 - 216, The test "returns entitlements at the threshold boundary" currently sets lastSyncAt to Date.now() - STALE_THRESHOLD_MS + 1000 which is inside the threshold; change it to exactly Date.now() - STALE_THRESHOLD_MS to exercise the boundary (or add a second assertion with Date.now() - STALE_THRESHOLD_MS - 1 to demonstrate the flip). Update the makeLicense call in this test (and/or add the second case) so getEntitlements is invoked with a license whose lastSyncAt equals the exact STALE_THRESHOLD_MS boundary to pin the <= semantics. ``` </details> </blockquote></details> <details> <summary>packages/web/src/app/(app)/settings/license/page.tsx-43-48 (1)</summary><blockquote> `43-48`: _⚠️ Potential issue_ | _🟡 Minor_ **Unsafe cast of `searchParams` when rebuilding the URL — arrays and `undefined` values will be mangled.** `searchParams` is typed as `Record<string, string | string[] | undefined>` (per `LicensePageProps` on Line 16), but it's cast to `Record<string, string>` before being handed to `URLSearchParams`. If any incoming param is an array, `URLSearchParams` will call `toString()` on it and serialize as `"a,b,c"` (not as repeated keys); `undefined` values become the literal string `"undefined"`. Safer to construct the `URLSearchParams` manually and skip non-strings. <details> <summary>🛠️ Proposed fix</summary> ```diff - // Strip our params but preserve anything else (e.g. `checkout=success`). - const preserved = new URLSearchParams(searchParams as Record<string, string>); - preserved.delete('refresh'); - preserved.delete('trial_used'); + // Strip our params but preserve anything else (e.g. `checkout=success`). + const preserved = new URLSearchParams(); + for (const [key, value] of Object.entries(searchParams ?? {})) { + if (key === 'refresh' || key === 'trial_used' || value === undefined) { + continue; + } + if (Array.isArray(value)) { + for (const v of value) { preserved.append(key, v); } + } else { + preserved.append(key, value); + } + } const suffix = preserved.toString(); redirect(suffix ? `/settings/license?${suffix}` : '/settings/license'); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`(app)/settings/license/page.tsx around lines 43 - 48, The code unsafely casts searchParams to Record<string,string> before passing into URLSearchParams which will mangle arrays and undefineds; change the logic that builds preserved so you create a new URLSearchParams and iterate over Object.entries(searchParams) (the searchParams prop from LicensePageProps), skipping undefined, appending each string value directly and appending each element when the value is an array, then delete('refresh') and delete('trial_used') on that preserved URLSearchParams and call redirect(suffix ? `/settings/license?${suffix}` : '/settings/license') as before; update references to preserved, searchParams and redirect accordingly. ``` </details> </blockquote></details> <details> <summary>packages/web/src/app/(app)/components/banners/bannerResolver.tsx-165-194 (1)</summary><blockquote> `165-194`: _⚠️ Potential issue_ | _🟡 Minor_ **`expiring-soon` won't fire for auto-renewing online subscriptions.** Online heads-up currently depends on `ctx.license.cancelAt`. For a normal monthly/yearly subscription that auto-renews, Stripe leaves `cancel_at` null and only sets it once the user schedules cancellation — so paying users will never see the "License expires in N days" banner, only users who have already scheduled a cancel. The `nextRenewalAt` field exists in the License schema and is available in context. Consider also keying off `nextRenewalAt` to cover approaching auto-renewals (in addition to the scheduled cancellation case). <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`(app)/components/banners/bannerResolver.tsx around lines 165 - 194, getLicenseExpiryState currently only checks ctx.license.cancelAt for online "expiring-soon" state, so auto-renewing subscriptions (where cancelAt is null) never trigger the heads-up; update getLicenseExpiryState to also consider ctx.license.nextRenewalAt (parse to Date, compute deltaMs vs ctx.now.getTime()) and if deltaMs > 0 && deltaMs <= EXPIRY_HEADS_UP_WINDOW_MS return { kind: 'expiring-soon', source: 'online', expiresAt } just like the cancelAt branch, while preserving existing cancelAt and expired-status checks (ensure you reference getLicenseExpiryState, ctx.license.cancelAt, ctx.license.nextRenewalAt, EXPIRY_HEADS_UP_WINDOW_MS and return the same LicenseExpiryState shape). ``` </details> </blockquote></details> <details> <summary>packages/web/src/actions.ts-744-744 (1)</summary><blockquote> `744-744`: _⚠️ Potential issue_ | _🟡 Minor_ **Use `logger.error` for consistency with the rest of the file.** Every other error log in this file goes through the module-level `logger` (e.g. Line 49, Line 546, Line 579, Line 611). Dropping to `console.error` here bypasses the structured logger and likely breaks log-level filtering / JSON formatting in production. <details> <summary>Suggested change</summary> ```diff - console.error(`Anonymous access isn't supported in your current plan. For support, contact ${SOURCEBOT_SUPPORT_EMAIL}.`); + logger.error(`Anonymous access isn't supported in your current plan. For support, contact ${SOURCEBOT_SUPPORT_EMAIL}.`); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/actions.ts` at line 744, Replace the direct console.error call in the anonymous-access handling with the module-level logger by using logger.error instead of console.error so logs follow the file's structured logging conventions; locate the console.error invocation in packages/web/src/actions.ts (the anonymous access check) and change it to call logger.error with the same message (preserving SOURCEBOT_SUPPORT_EMAIL) so it uses the existing logger used elsewhere in this file. ``` </details> </blockquote></details> <details> <summary>packages/web/src/ee/features/lighthouse/actions.ts-40-49 (1)</summary><blockquote> `40-49`: _⚠️ Potential issue_ | _🟡 Minor_ **Rollback delete can mask the original sync error.** If `syncWithLighthouse` throws and then `prisma.license.delete` also throws (DB transient error, unique-key race, etc.), `e` is never re-thrown — the delete's error propagates instead and the operator loses the original failure reason (typically the more actionable one). Swallow/log the rollback failure so the sync error is preserved. <details> <summary>Suggested change</summary> ```diff try { await syncWithLighthouse(org.id); } catch (e) { - // If the ping fails, remove the license record - await prisma.license.delete({ - where: { orgId: org.id }, - }); - - throw e; + // If the ping fails, remove the license record. Don't let a + // rollback failure mask the original sync error. + try { + await prisma.license.delete({ + where: { orgId: org.id }, + }); + } catch (rollbackError) { + // log-and-continue so `e` is the error surfaced to the caller + // eslint-disable-next-line no-console + console.error('Failed to roll back license row after sync failure', rollbackError); + } + throw e; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/ee/features/lighthouse/actions.ts` around lines 40 - 49, The current catch block around syncWithLighthouse(org.id) performs prisma.license.delete which can throw and replace the original error; change it to preserve and rethrow the original sync error: inside the catch(e) capture the original error (e.g., originalErr = e), then attempt the rollback delete in its own try/catch (call prisma.license.delete({ where: { orgId: org.id } }) inside a nested try), and if that nested delete fails, log the rollback failure (do not throw), and finally rethrow the originalErr; reference syncWithLighthouse, prisma.license.delete and org.id when locating the code to update. ``` </details> </blockquote></details> <details> <summary>packages/web/src/ee/features/lighthouse/actions.ts-190-214 (1)</summary><blockquote> `190-214`: _⚠️ Potential issue_ | _🟡 Minor_ **Bound the pagination loop to prevent runaway iteration on a misbehaving server.** `while (true)` with termination driven entirely by `result.hasMore` and a server-provided `lastInvoice.id` means a Lighthouse-side bug that keeps returning `hasMore: true` with a non-advancing cursor (or a duplicate id) will spin this server action indefinitely, holding the request thread open. A hard page cap is cheap defense-in-depth for a remote dependency. <details> <summary>Suggested change</summary> ```diff const allInvoices: Invoice[] = []; let startingAfter: string | undefined; - while (true) { + const MAX_PAGES = 100; // safety cap: 100 * 100 = 10k invoices + for (let page = 0; page < MAX_PAGES; page++) { const result = await client.invoices({ activationCode, limit: 100, ...(startingAfter && { startingAfter }), }); if (isServiceError(result)) { return result; } allInvoices.push(...result.invoices); if (!result.hasMore) { break; } const lastInvoice = result.invoices[result.invoices.length - 1]; if (!lastInvoice) { break; } + if (lastInvoice.id === startingAfter) { + // cursor isn't advancing — bail out to avoid an infinite loop + break; + } startingAfter = lastInvoice.id; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/web/src/ee/features/lighthouse/actions.ts` around lines 190 - 214, The pagination loop in the invoice fetcher (allInvoices / startingAfter calling client.invoices and relying on result.hasMore and lastInvoice.id) must be bounded to avoid infinite loops; add a maxPages constant (e.g., MAX_INVOICE_PAGES) and a page counter inside the loop, increment it each iteration and break/return an error or log and stop when the counter exceeds the limit; update the while (true) to check the counter (or convert to a for loop) so the code stops even if hasMore stays true or the cursor doesn't advance, ensuring safe termination when fetching invoices via client.invoices. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `c60754b7-c2b8-4991-bea2-78cbf431dc1d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 791496c533ee7f2b6a890160f696503cab873c2d and c725e3a43d0933ac29d7806652f4d89c719d2a98. </details> <details> <summary>📒 Files selected for processing (127)</summary> * `.env.development` * `CHANGELOG.md` * `docs/api-reference/sourcebot-public.openapi.json` * `docs/docs.json` * `docs/docs/billing.mdx` * `docs/docs/license-key.mdx` * `packages/backend/src/__mocks__/prisma.ts` * `packages/backend/src/api.ts` * `packages/backend/src/ee/accountPermissionSyncer.ts` * `packages/backend/src/ee/repoPermissionSyncer.ts` * `packages/backend/src/ee/syncSearchContexts.test.ts` * `packages/backend/src/ee/syncSearchContexts.ts` * `packages/backend/src/entitlements.ts` * `packages/backend/src/github.ts` * `packages/backend/src/index.ts` * `packages/backend/src/prisma.ts` * `packages/backend/src/utils.ts` * `packages/backend/vitest.config.ts` * `packages/db/prisma/migrations/20260417011834_add_license_table/migration.sql` * `packages/db/prisma/migrations/20260417224042_remove_guest_org_role/migration.sql` * `packages/db/prisma/migrations/20260418213423_add_billing_details_to_license/migration.sql` * `packages/db/prisma/migrations/20260421184633_add_cancel_at_to_license/migration.sql` * `packages/db/prisma/migrations/20260422203048_add_trial_fields/migration.sql` * `packages/db/prisma/migrations/20260422204809_add_has_payment_method/migration.sql` * `packages/db/prisma/schema.prisma` * `packages/shared/src/constants.ts` * `packages/shared/src/crypto.ts` * `packages/shared/src/entitlements.test.ts` * `packages/shared/src/entitlements.ts` * `packages/shared/src/env.server.ts` * `packages/shared/src/index.server.ts` * `packages/shared/src/types.ts` * `packages/shared/vitest.config.ts` * `packages/web/src/__mocks__/prisma.ts` * `packages/web/src/actions.ts` * `packages/web/src/app/(app)/@sidebar/components/defaultSidebar/index.tsx` * `packages/web/src/app/(app)/@sidebar/components/settingsSidebar/header.tsx` * `packages/web/src/app/(app)/@sidebar/components/settingsSidebar/index.tsx` * `packages/web/src/app/(app)/chat/[id]/page.tsx` * `packages/web/src/app/(app)/components/banners/actions.ts` * `packages/web/src/app/(app)/components/banners/bannerResolver.test.ts` * `packages/web/src/app/(app)/components/banners/bannerResolver.tsx` * `packages/web/src/app/(app)/components/banners/bannerShell.tsx` * `packages/web/src/app/(app)/components/banners/bannerSlot.tsx` * `packages/web/src/app/(app)/components/banners/invoicePastDueBanner.tsx` * `packages/web/src/app/(app)/components/banners/licenseExpiredBanner.tsx` * `packages/web/src/app/(app)/components/banners/licenseExpiryHeadsUpBanner.tsx` * `packages/web/src/app/(app)/components/banners/openBillingPortalButton.tsx` * `packages/web/src/app/(app)/components/banners/permissionSyncBanner.tsx` * `packages/web/src/app/(app)/components/banners/refreshLicenseButton.tsx` * `packages/web/src/app/(app)/components/banners/servicePingFailedBanner.tsx` * `packages/web/src/app/(app)/components/banners/trialBanner.tsx` * `packages/web/src/app/(app)/components/banners/types.ts` * `packages/web/src/app/(app)/layout.tsx` * `packages/web/src/app/(app)/repos/[id]/page.tsx` * `packages/web/src/app/(app)/settings/analytics/page.tsx` * `packages/web/src/app/(app)/settings/components/settingsCard.tsx` * `packages/web/src/app/(app)/settings/layout.tsx` * `packages/web/src/app/(app)/settings/license/activationCodeCard.tsx` * `packages/web/src/app/(app)/settings/license/currentPlanCard.tsx` * `packages/web/src/app/(app)/settings/license/offlineLicenseCard.tsx` * `packages/web/src/app/(app)/settings/license/page.tsx` * `packages/web/src/app/(app)/settings/license/planActionsMenu.tsx` * `packages/web/src/app/(app)/settings/license/recentInvoicesCard.tsx` * `packages/web/src/app/(app)/settings/linked-accounts/page.tsx` * `packages/web/src/app/(app)/settings/members/components/inviteMemberCard.tsx` * `packages/web/src/app/(app)/settings/members/components/invitesList.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/page.tsx` * `packages/web/src/app/api/(server)/ee/.well-known/oauth-authorization-server/route.ts` * `packages/web/src/app/api/(server)/ee/.well-known/oauth-protected-resource/[...path]/route.ts` * `packages/web/src/app/api/(server)/ee/audit/route.ts` * `packages/web/src/app/api/(server)/ee/chat/[chatId]/searchMembers/route.ts` * `packages/web/src/app/api/(server)/ee/oauth/register/route.ts` * `packages/web/src/app/api/(server)/ee/oauth/revoke/route.ts` * `packages/web/src/app/api/(server)/ee/oauth/token/route.ts` * `packages/web/src/app/api/(server)/ee/permissionSyncStatus/api.ts` * `packages/web/src/app/api/(server)/ee/user/route.ts` * `packages/web/src/app/api/(server)/ee/users/route.ts` * `packages/web/src/app/api/(server)/mcp/route.ts` * `packages/web/src/app/api/(server)/repos/listReposApi.ts` * `packages/web/src/app/components/anonymousAccessToggle.tsx` * `packages/web/src/app/components/organizationAccessSettings.tsx` * `packages/web/src/app/invite/actions.ts` * `packages/web/src/app/invite/page.tsx` * `packages/web/src/app/layout.tsx` * `packages/web/src/app/login/page.tsx` * `packages/web/src/app/oauth/authorize/page.tsx` * `packages/web/src/app/onboard/page.tsx` * `packages/web/src/app/signup/page.tsx` * `packages/web/src/auth.ts` * `packages/web/src/ee/features/analytics/actions.ts` * `packages/web/src/ee/features/audit/actions.ts` * `packages/web/src/ee/features/audit/audit.ts` * `packages/web/src/ee/features/audit/auditService.ts` * `packages/web/src/ee/features/audit/factory.ts` * `packages/web/src/ee/features/audit/mockAuditService.ts` * `packages/web/src/ee/features/audit/types.ts` * `packages/web/src/ee/features/lighthouse/CLAUDE.md` * `packages/web/src/ee/features/lighthouse/actions.ts` * `packages/web/src/ee/features/lighthouse/client.ts` * `packages/web/src/ee/features/lighthouse/servicePing.ts` * `packages/web/src/ee/features/lighthouse/types.ts` * `packages/web/src/ee/features/sso/actions.ts` * `packages/web/src/ee/features/sso/sso.ts` * `packages/web/src/ee/features/userManagement/actions.ts` * `packages/web/src/features/chat/actions.ts` * `packages/web/src/features/git/getFileSourceApi.ts` * `packages/web/src/features/git/getTreeApi.ts` * `packages/web/src/features/mcp/askCodebase.ts` * `packages/web/src/features/search/searchApi.ts` * `packages/web/src/features/userManagement/actions.ts` * `packages/web/src/initialize.ts` * `packages/web/src/lib/authUtils.ts` * `packages/web/src/lib/constants.ts` * `packages/web/src/lib/entitlements.test.ts` * `packages/web/src/lib/entitlements.ts` * `packages/web/src/lib/errorCodes.ts` * `packages/web/src/lib/identityProviders.ts` * `packages/web/src/lib/utils.ts` * `packages/web/src/middleware/authenticatedPage.tsx` * `packages/web/src/middleware/withAuth.test.ts` * `packages/web/src/middleware/withAuth.ts` * `packages/web/src/middleware/withMinimumOrgRole.ts` * `packages/web/src/openapi/publicApiSchemas.ts` * `packages/web/src/prisma.ts` </details> <details> <summary>💤 Files with no reviewable changes (7)</summary> * packages/web/src/app/(app)/settings/linked-accounts/page.tsx * packages/shared/src/constants.ts * packages/web/src/middleware/withMinimumOrgRole.ts * packages/web/src/ee/features/audit/factory.ts * packages/web/src/lib/constants.ts * packages/web/src/ee/features/audit/mockAuditService.ts * packages/web/src/ee/features/audit/auditService.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+48
to
51
| public async startScheduler() { | ||
| if (!await hasEntitlement('permission-syncing')) { | ||
| throw new Error('Permission syncing is not supported in current plan.'); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of startScheduler() for repoPermissionSyncer / accountPermissionSyncer
rg -nP -C2 '\b(repoPermissionSyncer|accountPermissionSyncer)\s*\.\s*startScheduler\s*\(' --type=tsRepository: sourcebot-dev/sourcebot
Length of output: 559
🏁 Script executed:
cat -n packages/backend/src/index.ts | sed -n '60,80p'Repository: sourcebot-dev/sourcebot
Length of output: 916
🏁 Script executed:
cat -n packages/backend/src/index.ts | sed -n '1,75p'Repository: sourcebot-dev/sourcebot
Length of output: 3390
🏁 Script executed:
# Check if these methods are async in their respective classes
for class in "RepoPermissionSyncer" "AccountPermissionSyncer" "AuditLogPruner" "ConnectionManager"; do
echo "=== $class ==="
rg -A3 "startScheduler\s*\(" packages/backend/src/ee --type=ts -g "*${class}*"
rg -A3 "startScheduler\s*\(" packages/backend/src --type=ts -g "*${class}*" | head -20
doneRepository: sourcebot-dev/sourcebot
Length of output: 177
🏁 Script executed:
# Find the class files first
echo "=== Finding class files ==="
fd -e ts -e js "RepoPermissionSyncer|AccountPermissionSyncer|AuditLogPruner|ConnectionManager" packages/backend/src
echo -e "\n=== Checking RepoPermissionSyncer ==="
cat packages/backend/src/ee/repoPermissionSyncer.ts | head -60
echo -e "\n=== Checking AccountPermissionSyncer ==="
cat packages/backend/src/ee/accountPermissionSyncer.ts | head -60
echo -e "\n=== Checking AuditLogPruner ==="
cat packages/backend/src/ee/auditLogPruner.ts | head -60
echo -e "\n=== Checking ConnectionManager ==="
cat packages/backend/src/connectionManager.ts | head -60Repository: sourcebot-dev/sourcebot
Length of output: 9457
🏁 Script executed:
# Check ConnectionManager.startScheduler() signature
rg -A10 "public.*startScheduler" packages/backend/src/connectionManager.tsRepository: sourcebot-dev/sourcebot
Length of output: 559
Async startScheduler() calls may produce unhandled rejections at call sites.
The startScheduler() methods on repoPermissionSyncer (line 69) and accountPermissionSyncer (line 71) in packages/backend/src/index.ts are async and throw errors internally, but are invoked without await. If either method's Promise rejects, the error will be swallowed as an unhandled rejection instead of crashing the application during startup. Both calls must be awaited to properly handle errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/ee/repoPermissionSyncer.ts` around lines 48 - 51,
startScheduler() on repoPermissionSyncer and accountPermissionSyncer can reject
but are currently invoked without awaiting them, causing unhandled promise
rejections; update the startup code that calls
repoPermissionSyncer.startScheduler() and
accountPermissionSyncer.startScheduler() (in packages/backend/src/index.ts) to
await both calls (or handle their returned Promises with .catch and rethrow) so
any rejection is observed and the process fails fast during startup.
Comment on lines
+8
to
+10
| -- Remove the guest user and its membership (only holder of GUEST role) | ||
| DELETE FROM "UserToOrg" WHERE "role" = 'GUEST'; | ||
| DELETE FROM "User" WHERE id = '1'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify guest-user conventions and FK relationships that reference User.
set -euo pipefail
echo "=== Where is the guest User with id='1' created/seeded? ==="
rg -nP -C3 $'\bid:\s*["\']1["\']' --type=ts --type=sql --type=prisma || true
rg -nPU -C3 "(?s)guest[^\n]*user[^\n]*id" --type=ts || true
echo
echo "=== Prisma schema: User model and anything referencing it ==="
fd -t f schema.prisma --exec rg -nP -C3 'model\s+User\b|userId|createdById|actorId' {} \;
echo
echo "=== All tables with onDelete semantics on userId/createdById ==="
fd -t f schema.prisma --exec rg -nP -C1 'onDelete' {} \;
echo
echo "=== Other migrations touching 'GUEST' or User id='1' ==="
fd -t f -e sql --exec rg -nP -C2 "GUEST|WHERE id = '1'|WHERE \"id\" = '1'" {} \;Repository: sourcebot-dev/sourcebot
Length of output: 8832
🏁 Script executed:
#!/bin/bash
# Find seed files, database initialization, and when GUEST role was introduced
echo "=== Search for seed/seeding files ==="
fd -t f \( -name "*seed*" -o -name "*init*" -o -name "*fixture*" \) | head -20
echo
echo "=== List all migrations in order ==="
fd -t f -e sql -path "*/migrations/*" | sort
echo
echo "=== Look for GUEST role introduction in migrations ==="
fd -t f -e sql -path "*/migrations/*" | xargs rg -l "GUEST" | sort
echo
echo "=== Check early migrations for GUEST role ==="
fd -t f -e sql -path "*/migrations/*" | sort | head -5 | xargs -I {} bash -c "echo '=== {} ===' && head -20 {}"
echo
echo "=== Search for 'guest' user references in TypeScript code ==="
rg -i "guest.*user|user.*guest" --type=ts -C2 | head -40Repository: sourcebot-dev/sourcebot
Length of output: 3966
🏁 Script executed:
#!/bin/bash
# Read the migration that introduced GUEST role
echo "=== GUEST role introduction migration ==="
cat "packages/db/prisma/migrations/20250520182630_add_guest_role/migration.sql"
echo
echo "=== Current migration being reviewed ==="
cat "packages/db/prisma/migrations/20260417224042_remove_guest_org_role/migration.sql"
echo
echo "=== Check for any user creation or seeding in migrations ==="
fd -type f -path "*/migrations/*" -name "*.sql" | xargs rg -l "INSERT INTO.*User|SELECT.*FROM.*User" | head -5
echo
echo "=== Look for initialization or setup code ==="
rg -i "seed|init" packages/db --type=ts --type=sql | head -20Repository: sourcebot-dev/sourcebot
Length of output: 2274
🏁 Script executed:
#!/bin/bash
# Search more carefully for how id='1' was used or created
echo "=== All migrations sorted chronologically ==="
ls -la packages/db/prisma/migrations/ | tail -15
echo
echo "=== Migrations between GUEST introduction and removal ==="
ls packages/db/prisma/migrations/ | sort | awk '/20250520182630/,/20260417224042/'
echo
echo "=== Search for explicit '1' in migration context (not regex) ==="
rg "WHERE id = '1'|WHERE \"id\" = '1'|userId.*=.*'1'|user.*id.*'1'" packages/db/prisma/migrations/
echo
echo "=== Search for all migrations that touch UserToOrg ==="
fd -type f -path "*/migrations/*" -name "*.sql" | xargs rg -l "UserToOrg" | sort
echo
echo "=== Look for git log or comments about guest user ==="
rg -i "guest" packages/db/prisma --type=sql -B2 -A2Repository: sourcebot-dev/sourcebot
Length of output: 5985
Hard-coded guest user id='1' is brittle and relies on an established but fragile convention.
While the codebase does establish id='1' as the guest user (evidenced by earlier migrations like 20251129063148_change_chat_created_by_to_optional), hardcoding this dependency makes the migration fragile. If the guest user is ever re-seeded with a different id or the convention changes, this migration will either fail silently (leaving the guest user in place but with no memberships) or delete the wrong user.
Additionally, the Prisma schema shows all User foreign key references use onDelete: Cascade, meaning this delete will silently cascade-delete all related data: Chat records, ChatAccess entries, Account rows, ApiKey entries, OAuth tokens (authorization codes, refresh tokens, access tokens), Invite records, RequestInvite entries, and Audit log references. Verify that this data loss is intentional.
Prefer deriving the guest user from the GUEST role membership rather than a hard-coded id, or add an explicit marker/flag to identify the guest user unambiguously.
Safer approach
-- Remove the guest user and its membership (only holder of GUEST role)
-DELETE FROM "UserToOrg" WHERE "role" = 'GUEST';
-DELETE FROM "User" WHERE id = '1';
+-- Identify and delete the guest user through GUEST role membership
+DELETE FROM "User"
+WHERE id IN (
+ SELECT DISTINCT "userId" FROM "UserToOrg" WHERE "role" = 'GUEST'
+);
+DELETE FROM "UserToOrg" WHERE "role" = 'GUEST';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/db/prisma/migrations/20260417224042_remove_guest_org_role/migration.sql`
around lines 8 - 10, The migration currently hard-codes id='1' when deleting the
guest User; instead derive the guest user from the GUEST membership: delete
UserToOrg rows WHERE "role" = 'GUEST' as you already do, then delete from "User"
only those users who were the sole holder of a GUEST membership (i.e., users
whose only UserToOrg rows are role='GUEST') by selecting user ids from
"UserToOrg" (use a subquery that finds users with no non-'GUEST' memberships)
rather than deleting id='1'; also keep a clear TODO/confirm in the migration
near the DELETE of "User" to ensure the cascading deletion of related data
(Chat, ApiKey, Account, OAuth tokens, etc.) is intentional.
Comment on lines
+302
to
323
| model License { | ||
| id String @id @default(cuid()) | ||
| orgId Int @unique | ||
| org Org @relation(fields: [orgId], references: [id]) | ||
| activationCode String | ||
| entitlements String[] | ||
| seats Int? | ||
| status String? /// See LicenseStatus in packages/shared/src/types.ts | ||
| planName String? | ||
| unitAmount Int? | ||
| currency String? | ||
| interval String? | ||
| intervalCount Int? | ||
| nextRenewalAt DateTime? | ||
| nextRenewalAmount Int? | ||
| cancelAt DateTime? | ||
| trialEnd DateTime? | ||
| hasPaymentMethod Boolean? | ||
| lastSyncAt DateTime? | ||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
| } |
Contributor
There was a problem hiding this comment.
Missing onDelete: Cascade on License.org relation.
Every other child relation to Org in this schema (Repo, Connection, Invite, AccountRequest, ApiKey, Audit, SearchContext, Chat, UserToOrg) uses onDelete: Cascade. The new License.org relation does not, so deleting an Org that has a License row will fail with a FK constraint violation (or leave an orphaned row, depending on the default referential action). Given the 1:1 unique relation and the rest of the schema convention, cascade is almost certainly the intended behavior.
🛠 Proposed fix
- org Org `@relation`(fields: [orgId], references: [id])
+ org Org `@relation`(fields: [orgId], references: [id], onDelete: Cascade)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/prisma/schema.prisma` around lines 302 - 323, The License model's
relation to Org (fields: orgId, relation: org) lacks an explicit onDelete:
Cascade; add onDelete: Cascade to the License.org relation definition so that
deleting an Org cascades and removes the associated License (update the relation
attribute on the org field in the License model to include onDelete: Cascade,
mirroring other models like Repo/Connection/Invite that use cascade).
|
|
||
| SOURCEBOT_ENCRYPTION_KEY: z.string(), | ||
| SOURCEBOT_INSTALL_ID: z.string().default("unknown"), | ||
| SOURCEBOT_LIGHTHOUSE_URL: z.string().url(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether a default SOURCEBOT_LIGHTHOUSE_URL is wired up elsewhere
# (Dockerfile ENV, docker-compose, helm chart, startup scripts, .env files).
rg -nP -C2 '\bSOURCEBOT_LIGHTHOUSE_URL\b'
fd -t f -i 'dockerfile' --exec rg -nP -C2 'SOURCEBOT_LIGHTHOUSE_URL' {}
fd -t f -e yml -e yaml --exec rg -nP -C2 'SOURCEBOT_LIGHTHOUSE_URL' {}
fd -t f -e env -H --exec rg -nP -C2 'SOURCEBOT_LIGHTHOUSE_URL' {}Repository: sourcebot-dev/sourcebot
Length of output: 3042
Breaking change: SOURCEBOT_LIGHTHOUSE_URL is now required with no default
Declaring z.string().url() without .default(...) or .optional() will cause env validation to fail on startup for every existing deployment that hasn't set this variable. Since this is a newly introduced variable, no existing user will have it configured.
The lighthouse client directly uses this URL without null-checking or graceful degradation, so this becomes a hard requirement for startup.
Consider one of:
- Provide a sensible production default (e.g., the public Lighthouse endpoint) so out-of-the-box deployments keep working.
- Make it
.optional()and have the Lighthouse client gracefully no-op or degrade when unset (useful for air-gapped/offline installs and for disabling telemetry/billing integration).
🛠️ Proposed fix with a default
- SOURCEBOT_LIGHTHOUSE_URL: z.string().url(),
+ SOURCEBOT_LIGHTHOUSE_URL: z.string().url().default("https://lighthouse.sourcebot.dev"),📝 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
| SOURCEBOT_LIGHTHOUSE_URL: z.string().url(), | |
| SOURCEBOT_LIGHTHOUSE_URL: z.string().url().default("https://lighthouse.sourcebot.dev"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/env.server.ts` at line 272, The new env schema entry
SOURCEBOT_LIGHTHOUSE_URL is declared as z.string().url() which makes it required
and breaks existing deployments; change the schema to
z.string().url().optional() (or
z.string().url().default('https://lighthouse.storage.googleapis.com') if you
prefer a production default) and then update the Lighthouse client code that
reads SOURCEBOT_LIGHTHOUSE_URL (locate where SOURCEBOT_LIGHTHOUSE_URL is
imported/used) to gracefully no-op or degrade when the value is undefined (i.e.,
guard the client init and any requests with an if-check and skip
telemetry/billing calls when unset).
Comment on lines
+20
to
+34
| const handleClick = useCallback(() => { | ||
| setIsOpeningPortal(true); | ||
| createPortalSession() | ||
| .then((response) => { | ||
| if (isServiceError(response)) { | ||
| toast({ | ||
| description: `Failed to open billing portal: ${response.message}`, | ||
| variant: "destructive", | ||
| }); | ||
| setIsOpeningPortal(false); | ||
| } else { | ||
| router.push(response.url); | ||
| } | ||
| }); | ||
| }, [router, toast]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Next.js 15 next/navigation router.push support external URLs for full-page navigation?
💡 Result:
Yes—Next.js 15’s next/navigation router.push does support full-page navigation to external URLs, as long as you pass an absolute external URL (e.g. https://...). However, this is not the recommended approach for opening external links in a new tab. For new tabs, use an anchor/next/link with target="_blank" or window.open. Evidence: Next.js added support for router.push/router.replace/redirect with external absolute URLs such as router.push('https://google.com').
Citations:
- 1: Add support for navigating to external urls vercel/next.js#45388
- 2: https://nextjs.org/docs/app/api-reference/functions/use-router
- 3: https://nextjs.org/docs/api-reference/next/link
- 4: https://nextjs.org/docs/app/getting-started/linking-and-navigating
🏁 Script executed:
# Check Next.js version in the web package
find . -path "*/packages/web/package.json" -exec cat {} \; | grep -A 2 '"next"'Repository: sourcebot-dev/sourcebot
Length of output: 164
🏁 Script executed:
# Find the createPortalSession function to understand response.url type
rg -A 15 "createPortalSession" packages/web/src --type typescript --type tsx | head -60Repository: sourcebot-dev/sourcebot
Length of output: 102
🏁 Script executed:
# Search for createPortalSession function in packages/web/src
rg -A 15 "createPortalSession" packages/web/srcRepository: sourcebot-dev/sourcebot
Length of output: 8110
🌐 Web query:
Does Next.js 16 router.push support external URLs?
💡 Result:
No, Next.js 16's router.push does not support external URLs in the recommended way. Official docs state "You don't need to use router.push for external URLs. window.location is better suited for those cases." (Pages Router docs). App Router (next/navigation useRouter) focuses on internal navigation; community consensus and lack of docs for external use confirm use window.open or window.location.href for external links instead.
Citations:
- 1: https://nextjs.org/docs/pages/api-reference/functions/use-router
- 2: https://stackoverflow.com/questions/77535114/how-to-open-a-link-via-router-push-in-a-new-tab-using-nextjs-router-from-next
- 3: https://nextjs.org/docs/api-reference/next/router
- 4: https://nextjs.org/docs/app/getting-started/linking-and-navigating
Use window.location for external URL navigation and add error handling.
Two concerns in handleClick:
-
router.push(response.url)fromnext/navigationis intended for internal Next.js routes. The official Next.js 16 documentation explicitly recommends usingwindow.locationfor external URLs. Navigating to an external billing portal URL (e.g., Stripe/Lighthouse) should usewindow.location.assignorhreffor reliable full-page navigation. -
The
createPortalSession()promise has no.catch()handler, so an unexpected rejection (network failure, thrown error) will leaveisOpeningPortalstuck astrueand surface as an unhandled promise rejection. Additionally,setIsOpeningPortal(false)is not called in the error path.
🔧 Proposed fix
const handleClick = useCallback(() => {
setIsOpeningPortal(true);
createPortalSession()
.then((response) => {
if (isServiceError(response)) {
toast({
description: `Failed to open billing portal: ${response.message}`,
variant: "destructive",
});
- setIsOpeningPortal(false);
} else {
- router.push(response.url);
+ window.location.assign(response.url);
}
- });
- }, [router, toast]);
+ })
+ .catch((error) => {
+ toast({
+ description: `Failed to open billing portal: ${error instanceof Error ? error.message : "Unknown error"}`,
+ variant: "destructive",
+ });
+ })
+ .finally(() => {
+ setIsOpeningPortal(false);
+ });
+ }, [toast]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/app/`(app)/components/banners/openBillingPortalButton.tsx
around lines 20 - 34, The click handler handleClick should use
window.location.assign(response.url) instead of router.push for external billing
URLs and must handle promise rejections from createPortalSession so
isOpeningPortal is always reset; update handleClick (which calls
createPortalSession and uses setIsOpeningPortal and router.push) to either
convert to async/await or attach .catch() and .finally() so any thrown/network
error calls toast with an error message and ensures setIsOpeningPortal(false)
runs on both success/failure paths (replace router.push usage with
window.location.assign for external navigation).
Comment on lines
+84
to
+91
| function formatCurrency(amountCents: number, currency: string): string { | ||
| return new Intl.NumberFormat('en-US', { | ||
| style: 'currency', | ||
| currency: currency.toUpperCase(), | ||
| minimumFractionDigits: 2, | ||
| maximumFractionDigits: 2, | ||
| }).format(amountCents / 100); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Which currencies does Stripe treat as zero-decimal, and are amounts always in the smallest currency unit?
💡 Result:
Stripe treats the following ISO 4217 currencies as zero-decimal (i.e., the charge/amount is the same number without applying a factor like ×100): BIF, CLP, DJF, GNF, JPY, KMF, KRW, MGA, PYG, RWF, UGX, VND, VUV, XAF, XOF, XPF, plus special-case ISK, and payouts-only zero-decimal handling for HUF and TWD. Yes: Stripe amounts in API requests are always specified as an integer amount in the currency’s minor unit / smallest currency unit, represented without decimals. For zero-decimal currencies specifically, “the charge and the amount are the same, without requiring multiplication” (example: 500 JPY → amount 500).
Citations:
- 1: https://support.stripe.com/questions/which-zero-decimal-currencies-does-stripe-support
- 2: https://docs.stripe.com/currencies
- 3: https://stripe.com/docs/currencies
🏁 Script executed:
# Find the file and check context
fd "recentInvoicesCard.tsx" -x cat -n {}Repository: sourcebot-dev/sourcebot
Length of output: 4159
🏁 Script executed:
# Search for calls to formatCurrency
rg "formatCurrency" packages/web/src/appRepository: sourcebot-dev/sourcebot
Length of output: 1057
🏁 Script executed:
# Check Invoice type definition
rg "type Invoice|interface Invoice" packages/web/src -A 10Repository: sourcebot-dev/sourcebot
Length of output: 1251
🏁 Script executed:
# Find the invoiceSchema definition to see what type 'amount' and 'currency' are
rg "invoiceSchema\s*=" packages/web/src/ee/features/lighthouse/types.ts -A 20Repository: sourcebot-dev/sourcebot
Length of output: 710
🏁 Script executed:
# Also check currentPlanCard.tsx to see if it has the same formatCurrency function
cat -n packages/web/src/app/\(app\)/settings/license/currentPlanCard.tsx | head -200Repository: sourcebot-dev/sourcebot
Length of output: 9244
formatCurrency is incorrect for zero- and three-decimal currencies.
Stripe returns amounts in the smallest currency unit, so the divisor depends on the currency's fraction digits — not always 100. Hard-coding / 100 and forcing minimumFractionDigits: 2, maximumFractionDigits: 2 produces wrong values for zero-decimal currencies (JPY, KRW, VND, etc.) and three-decimal currencies (BHD, KWD, OMR). For currency: 'jpy', amount: 1000, this formats as ¥10.00 instead of ¥1,000.
Let Intl.NumberFormat derive fraction digits from the currency, and compute the divisor from it:
🐛 Proposed fix
function formatCurrency(amountCents: number, currency: string): string {
- return new Intl.NumberFormat('en-US', {
- style: 'currency',
- currency: currency.toUpperCase(),
- minimumFractionDigits: 2,
- maximumFractionDigits: 2,
- }).format(amountCents / 100);
+ const upper = currency.toUpperCase();
+ const formatter = new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: upper,
+ });
+ const fractionDigits = formatter.resolvedOptions().maximumFractionDigits ?? 2;
+ return formatter.format(amountCents / 10 ** fractionDigits);
}Note: The same issue exists in currentPlanCard.tsx (lines 127-134), which also divides by 100 unconditionally.
📝 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
| function formatCurrency(amountCents: number, currency: string): string { | |
| return new Intl.NumberFormat('en-US', { | |
| style: 'currency', | |
| currency: currency.toUpperCase(), | |
| minimumFractionDigits: 2, | |
| maximumFractionDigits: 2, | |
| }).format(amountCents / 100); | |
| } | |
| function formatCurrency(amountCents: number, currency: string): string { | |
| const upper = currency.toUpperCase(); | |
| const formatter = new Intl.NumberFormat('en-US', { | |
| style: 'currency', | |
| currency: upper, | |
| }); | |
| const fractionDigits = formatter.resolvedOptions().maximumFractionDigits ?? 2; | |
| return formatter.format(amountCents / 10 ** fractionDigits); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/app/`(app)/settings/license/recentInvoicesCard.tsx around
lines 84 - 91, The formatCurrency implementation hard-codes division by 100 and
forces two decimals, which breaks zero- and three-decimal currencies; update
formatCurrency to create an Intl.NumberFormat for the given currency without
fixed minimum/maximumFractionDigits, read the
resolvedOptions().maximumFractionDigits (or minimum if preferred) to compute
divisor = 10 ** fractionDigits, divide amountCents by that divisor before
formatting, and pass the resulting number to format so Intl chooses the correct
decimal places; apply the same change to the same logic in currentPlanCard.tsx
(the corresponding formatting function there) to keep behavior consistent.
Comment on lines
+27
to
+32
| export default async function RootLayout({ | ||
| children, | ||
| }: Readonly<{ | ||
| children: React.ReactNode; | ||
| }>) { | ||
| const entitlements = await getEntitlements(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'entitlements.ts' packages/web/src/lib | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: sourcebot-dev/sourcebot
Length of output: 1567
🏁 Script executed:
cat -n packages/web/src/app/layout.tsx | head -50Repository: sourcebot-dev/sourcebot
Length of output: 2312
Wrap getEntitlements() in React.cache() to memoize per request.
The getEntitlements() call in RootLayout (line 32) directly awaits a database query via __unsafePrisma.license.findUnique() without per-request memoization. This blocks every page render on a Prisma query. Wrap the function export in React.cache():
Suggested fix
// entitlements.ts
import { cache } from 'react';
export const getEntitlements = cache(async () => {
const license = await getSingleTenantLicense();
return _getEntitlements(license);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/app/layout.tsx` around lines 27 - 32, RootLayout is directly
awaiting getEntitlements() which triggers an uncached Prisma query per render;
wrap the entitlements retrieval in React.cache (imported as cache from 'react')
by exporting a cached version of getEntitlements (e.g., in entitlements.ts
export const getEntitlements = cache(async () => { const license = await
getSingleTenantLicense(); return _getEntitlements(license); })), then update
RootLayout to call the cached getEntitlements to ensure per-request memoization
and avoid repeated database hits.
Comment on lines
+257
to
260
| ...(env.PERMISSION_SYNC_ENABLED === 'true' && await hasEntitlement('permission-syncing') ? | ||
| ['repo'] : | ||
| [] | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Identify invocation frequency of getEEIdentityProviders and whether the provider list is memoized.
rg -nP -C4 'getEEIdentityProviders' --type=ts --type=tsxRepository: sourcebot-dev/sourcebot
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Search for getEEIdentityProviders without type restrictions, then analyze usage patterns
rg -nP -C4 'getEEIdentityProviders' | head -100Repository: sourcebot-dev/sourcebot
Length of output: 2132
🏁 Script executed:
#!/bin/bash
# Find where getProviders is used and how next-auth is configured
rg -nP 'getProviders' packages/web/src/auth.ts -A 10 -B 2Repository: sourcebot-dev/sourcebot
Length of output: 1523
🏁 Script executed:
#!/bin/bash
# Verify this is indeed the NextAuth configuration by looking at the full context around line 297
sed -n '270,310p' packages/web/src/auth.tsRepository: sourcebot-dev/sourcebot
Length of output: 1375
Provider OAuth scopes are stale on entitlement changes.
hasEntitlement('permission-syncing') is evaluated during server startup when NextAuth initializes its provider configuration (line 297 in auth.ts). If a user's entitlements change at runtime (e.g., license upgrade), subsequent logins will still use the scopes computed at startup, preventing users from accessing scoped resources (repos) until the server restarts. Consider re-evaluating hasEntitlement checks during token refresh or making entitlement changes trigger a server reload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/ee/features/sso/sso.ts` around lines 257 - 260, The provider
OAuth scopes are being computed once at startup using
hasEntitlement('permission-syncing') (and env.PERMISSION_SYNC_ENABLED) in the
provider config in sso.ts, so entitlement changes at runtime won't update
scopes; fix this by removing the static entitlement check from the provider
scope construction and instead re-check hasEntitlement at login/refresh time
(e.g., in the NextAuth jwt/session callbacks or the token refresh handler in
auth.ts) to add/remove the 'repo' scope or trigger a token refresh when
entitlement changes; update code references: adjust the scope assembly in sso.ts
(the block using env.PERMISSION_SYNC_ENABLED and hasEntitlement) and add
entitlement re-evaluation logic in the auth.ts token refresh/jwt callback (where
provider config is currently initialized) so scopes reflect current entitlements
without requiring a server restart.
Comment on lines
+175
to
+225
| const addUserToOrgRes = await addUserToOrganization(request.requestedById, org.id); | ||
| if (isServiceError(addUserToOrgRes)) { | ||
| await failAuditCallback(addUserToOrgRes.message); | ||
| return addUserToOrgRes; | ||
| } | ||
|
|
||
| export const leaveOrg = async (): Promise<{ success: boolean } | ServiceError> => sew(() => | ||
| withAuth(async ({ user, org, role, prisma }) => { | ||
| const guardError = await prisma.$transaction(async (tx) => { | ||
| if (role === OrgRole.OWNER) { | ||
| const ownerCount = await tx.userToOrg.count({ | ||
| where: { | ||
| orgId: org.id, | ||
| role: OrgRole.OWNER, | ||
| // Send approval email to the user | ||
| const smtpConnectionUrl = getSMTPConnectionURL(); | ||
| if (smtpConnectionUrl && env.EMAIL_FROM_ADDRESS) { | ||
| const html = await render(JoinRequestApprovedEmail({ | ||
| baseUrl: env.AUTH_URL, | ||
| user: { | ||
| name: request.requestedBy.name ?? undefined, | ||
| email: request.requestedBy.email!, | ||
| avatarUrl: request.requestedBy.image ?? undefined, | ||
| }, | ||
| orgName: org.name, | ||
| })); | ||
|
|
||
| const transport = createTransport(smtpConnectionUrl); | ||
| const result = await transport.sendMail({ | ||
| to: request.requestedBy.email!, | ||
| from: env.EMAIL_FROM_ADDRESS, | ||
| subject: `Your request to join ${org.name} has been approved`, | ||
| html, | ||
| text: `Your request to join ${org.name} on Sourcebot has been approved. You can now access the organization at ${env.AUTH_URL}`, | ||
| }); | ||
|
|
||
| const failed = result.rejected.concat(result.pending).filter(Boolean); | ||
| if (failed.length > 0) { | ||
| logger.error(`Failed to send approval email to ${request.requestedBy.email}: ${failed}`); | ||
| } | ||
| } else { | ||
| logger.warn(`SMTP_CONNECTION_URL or EMAIL_FROM_ADDRESS not set. Skipping approval email to ${request.requestedBy.email}`); | ||
| } | ||
|
|
||
| await createAudit({ | ||
| action: "user.join_request_approved", | ||
| actor: { | ||
| id: user.id, | ||
| type: "user" | ||
| }, | ||
| orgId: org.id, | ||
| target: { | ||
| id: requestId, | ||
| type: "account_join_request" | ||
| } | ||
| }); | ||
| return { | ||
| success: true, | ||
| } |
Contributor
There was a problem hiding this comment.
Missing syncWithLighthouse after approving an account request.
_removeUserFromOrg intentionally calls syncWithLighthouse(orgId) on success (Line 104-108) with the comment "Sync with lighthouse s.t., the subscription quantity will update immediately." approveAccountRequest performs the symmetric operation — addUserToOrganization adds a UserToOrg row, bumping the seat count — but no sync is fired. Until the next scheduled ping, Lighthouse will report an outdated quantity, which can cause under-billing and stale results from orgHasAvailability for subsequent invites.
Suggested change
const addUserToOrgRes = await addUserToOrganization(request.requestedById, org.id);
if (isServiceError(addUserToOrgRes)) {
await failAuditCallback(addUserToOrgRes.message);
return addUserToOrgRes;
}
+ // Sync with lighthouse s.t., the subscription
+ // quantity will update immediately.
+ await syncWithLighthouse(org.id).catch(() => { /* ignore error */ });
+
// Send approval email to the user📝 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 addUserToOrgRes = await addUserToOrganization(request.requestedById, org.id); | |
| if (isServiceError(addUserToOrgRes)) { | |
| await failAuditCallback(addUserToOrgRes.message); | |
| return addUserToOrgRes; | |
| } | |
| export const leaveOrg = async (): Promise<{ success: boolean } | ServiceError> => sew(() => | |
| withAuth(async ({ user, org, role, prisma }) => { | |
| const guardError = await prisma.$transaction(async (tx) => { | |
| if (role === OrgRole.OWNER) { | |
| const ownerCount = await tx.userToOrg.count({ | |
| where: { | |
| orgId: org.id, | |
| role: OrgRole.OWNER, | |
| // Send approval email to the user | |
| const smtpConnectionUrl = getSMTPConnectionURL(); | |
| if (smtpConnectionUrl && env.EMAIL_FROM_ADDRESS) { | |
| const html = await render(JoinRequestApprovedEmail({ | |
| baseUrl: env.AUTH_URL, | |
| user: { | |
| name: request.requestedBy.name ?? undefined, | |
| email: request.requestedBy.email!, | |
| avatarUrl: request.requestedBy.image ?? undefined, | |
| }, | |
| orgName: org.name, | |
| })); | |
| const transport = createTransport(smtpConnectionUrl); | |
| const result = await transport.sendMail({ | |
| to: request.requestedBy.email!, | |
| from: env.EMAIL_FROM_ADDRESS, | |
| subject: `Your request to join ${org.name} has been approved`, | |
| html, | |
| text: `Your request to join ${org.name} on Sourcebot has been approved. You can now access the organization at ${env.AUTH_URL}`, | |
| }); | |
| const failed = result.rejected.concat(result.pending).filter(Boolean); | |
| if (failed.length > 0) { | |
| logger.error(`Failed to send approval email to ${request.requestedBy.email}: ${failed}`); | |
| } | |
| } else { | |
| logger.warn(`SMTP_CONNECTION_URL or EMAIL_FROM_ADDRESS not set. Skipping approval email to ${request.requestedBy.email}`); | |
| } | |
| await createAudit({ | |
| action: "user.join_request_approved", | |
| actor: { | |
| id: user.id, | |
| type: "user" | |
| }, | |
| orgId: org.id, | |
| target: { | |
| id: requestId, | |
| type: "account_join_request" | |
| } | |
| }); | |
| return { | |
| success: true, | |
| } | |
| const addUserToOrgRes = await addUserToOrganization(request.requestedById, org.id); | |
| if (isServiceError(addUserToOrgRes)) { | |
| await failAuditCallback(addUserToOrgRes.message); | |
| return addUserToOrgRes; | |
| } | |
| // Sync with lighthouse s.t., the subscription | |
| // quantity will update immediately. | |
| await syncWithLighthouse(org.id).catch(() => { /* ignore error */ }); | |
| // Send approval email to the user | |
| const smtpConnectionUrl = getSMTPConnectionURL(); | |
| if (smtpConnectionUrl && env.EMAIL_FROM_ADDRESS) { | |
| const html = await render(JoinRequestApprovedEmail({ | |
| baseUrl: env.AUTH_URL, | |
| user: { | |
| name: request.requestedBy.name ?? undefined, | |
| email: request.requestedBy.email!, | |
| avatarUrl: request.requestedBy.image ?? undefined, | |
| }, | |
| orgName: org.name, | |
| })); | |
| const transport = createTransport(smtpConnectionUrl); | |
| const result = await transport.sendMail({ | |
| to: request.requestedBy.email!, | |
| from: env.EMAIL_FROM_ADDRESS, | |
| subject: `Your request to join ${org.name} has been approved`, | |
| html, | |
| text: `Your request to join ${org.name} on Sourcebot has been approved. You can now access the organization at ${env.AUTH_URL}`, | |
| }); | |
| const failed = result.rejected.concat(result.pending).filter(Boolean); | |
| if (failed.length > 0) { | |
| logger.error(`Failed to send approval email to ${request.requestedBy.email}: ${failed}`); | |
| } | |
| } else { | |
| logger.warn(`SMTP_CONNECTION_URL or EMAIL_FROM_ADDRESS not set. Skipping approval email to ${request.requestedBy.email}`); | |
| } | |
| await createAudit({ | |
| action: "user.join_request_approved", | |
| actor: { | |
| id: user.id, | |
| type: "user" | |
| }, | |
| orgId: org.id, | |
| target: { | |
| id: requestId, | |
| type: "account_join_request" | |
| } | |
| }); | |
| return { | |
| success: true, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/userManagement/actions.ts` around lines 175 - 225,
approveAccountRequest currently calls addUserToOrganization but never triggers
syncWithLighthouse, so Lighthouse won't reflect the increased seat count
immediately; update the approveAccountRequest flow to call and await
syncWithLighthouse(org.id) after the addUserToOrganization success (and after
sending email/createAudit as appropriate) before returning, mirroring the
behavior in _removeUserFromOrg, and ensure syncWithLighthouse is
imported/available in the same module.
Comment on lines
+12
to
+22
| const getSingleTenantLicense = async () => { | ||
| try { | ||
| return await __unsafePrisma.license.findUnique({ | ||
| where: { | ||
| orgId: SINGLE_TENANT_ORG_ID, | ||
| }, | ||
| }); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Swallowed DB errors will silently disable all entitlements.
Any Prisma error (connection timeout, migration issue, etc.) causes getSingleTenantLicense to return null, which downstream turns every entitlement check into false — users can lose enterprise access without any observable signal. Consider logging the error before returning null so these failures surface in monitoring instead of quietly disabling features.
🛡️ Proposed fix
-const getSingleTenantLicense = async () => {
- try {
- return await __unsafePrisma.license.findUnique({
- where: {
- orgId: SINGLE_TENANT_ORG_ID,
- },
- });
- } catch {
- return null;
- }
-}
+const logger = createLogger('entitlements');
+
+const getSingleTenantLicense = async () => {
+ try {
+ return await __unsafePrisma.license.findUnique({
+ where: {
+ orgId: SINGLE_TENANT_ORG_ID,
+ },
+ });
+ } catch (error) {
+ logger.error({ err: error }, 'Failed to load tenant license; treating as unlicensed');
+ return null;
+ }
+}📝 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 getSingleTenantLicense = async () => { | |
| try { | |
| return await __unsafePrisma.license.findUnique({ | |
| where: { | |
| orgId: SINGLE_TENANT_ORG_ID, | |
| }, | |
| }); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| const logger = createLogger('entitlements'); | |
| const getSingleTenantLicense = async () => { | |
| try { | |
| return await __unsafePrisma.license.findUnique({ | |
| where: { | |
| orgId: SINGLE_TENANT_ORG_ID, | |
| }, | |
| }); | |
| } catch (error) { | |
| logger.error({ err: error }, 'Failed to load tenant license; treating as unlicensed'); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/lib/entitlements.ts` around lines 12 - 22,
getSingleTenantLicense currently swallows all Prisma errors and returns null,
which silently disables entitlements; update its catch to capture the error
(e.g., catch (err)) and log the error details before returning null so failures
are observable — use the existing logging facility if available (or
console.error) and include context like "Failed to fetch single-tenant license"
when logging the error originating from __unsafePrisma.license.findUnique inside
getSingleTenantLicense.
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.
Offline license expired banner (owner):
Offline license expired banner (member):
Online license expired banner (owner):
Online license expired banner (member):
Permission sync banner:
Expiry heads up banner:
Invoice past due banner:
License stale warning banner:
License stale error banner (owner):
License stale error banner (member):
Trial banner (payment method added):
Trial banner (payment method not added):
Summary by CodeRabbit
New Features
Bug Fixes
Documentation