Pro seat management and organization settings restructure#1640
Pro seat management and organization settings restructure#1640richiemcilroy merged 38 commits intomainfrom
Conversation
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| const validEmails = invitedEmails.filter((email) => | ||
| emailRegex.test(email.trim()), | ||
| ); | ||
| const validEmails = invitedEmails |
There was a problem hiding this comment.
validEmails can still contain duplicates (e.g. the same email pasted twice), which would create multiple invite rows in a single request. Consider deduping after normalization.
| const validEmails = invitedEmails | |
| const validEmails = Array.from( | |
| new Set( | |
| invitedEmails | |
| .map((email) => email.trim().toLowerCase()) | |
| .filter((email) => emailRegex.test(email)), | |
| ), | |
| ); |
| }), | ||
| ); | ||
|
|
||
| const failedEmails = inviteRecords |
There was a problem hiding this comment.
Right now if sendEmail rejects, the invite row still exists, so retrying will be deduped away (unless there’s a separate resend flow). One option is to delete failed invite records so retries work.
| const failedEmails = inviteRecords | |
| const failedInvites = inviteRecords.filter( | |
| (_, i) => emailResults[i]?.status === "rejected", | |
| ); | |
| const failedEmails = failedInvites.map((r) => r.email); | |
| if (failedInvites.length > 0) { | |
| await db() | |
| .delete(organizationInvites) | |
| .where(inArray(organizationInvites.id, failedInvites.map((r) => r.id))); | |
| } |
| @@ -17,78 +19,136 @@ export async function POST(request: NextRequest) { | |||
|
|
|||
| const { inviteId } = await request.json(); | |||
There was a problem hiding this comment.
request.json() can throw (invalid JSON / empty body) and it’s currently outside the try, which turns a client error into a 500.
| const { inviteId } = await request.json(); | |
| let inviteId: unknown; | |
| try { | |
| ({ inviteId } = await request.json()); | |
| } catch { | |
| return NextResponse.json({ error: "Invalid request body" }, { status: 400 }); | |
| } |
| hasProSeat: organizationMembers.hasProSeat, | ||
| }) | ||
| .from(organizationMembers) | ||
| .where(eq(organizationMembers.organizationId, organizationId)); |
There was a problem hiding this comment.
Seat assignment can race if two toggles run concurrently (both read the same proSeatsRemaining). Locking the org’s member rows here (like in the invite accept route) avoids oversubscription.
| .where(eq(organizationMembers.organizationId, organizationId)); | |
| .where(eq(organizationMembers.organizationId, organizationId)) | |
| .for("update"); |
Paragon SummaryThis pull request review analyzed 29 files and found no issues. The review examined code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. Paragon did not detect any problems in the current diff. Proceed with merge after your normal checks. This PR replaces the seat-count-based invite system with per-member Pro seat assignment ( Key changes:
Confidence score: 5/5
29 files reviewed, 0 comments |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (enable) { | ||
| const allMembers = await tx | ||
| .select({ | ||
| id: organizationMembers.id, | ||
| hasProSeat: organizationMembers.hasProSeat, | ||
| }) | ||
| .from(organizationMembers) | ||
| .where(eq(organizationMembers.organizationId, organizationId)); | ||
|
|
||
| const [owner] = await tx | ||
| .select({ | ||
| inviteQuota: users.inviteQuota, | ||
| stripeSubscriptionId: users.stripeSubscriptionId, | ||
| }) | ||
| .from(users) | ||
| .where(eq(users.id, organization.ownerId)) | ||
| .limit(1); | ||
|
|
||
| const { proSeatsRemaining } = calculateProSeats({ | ||
| inviteQuota: owner?.inviteQuota ?? 1, | ||
| members: allMembers, | ||
| }); | ||
|
|
||
| if (proSeatsRemaining <= 0) { | ||
| throw new Error( | ||
| "No Pro seats remaining. Purchase more seats to continue.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Race condition when checking available Pro seats
The allMembers query inside the transaction (line 58) is executed without a row lock, while the single member row is locked with .for("update") on line 47. Two concurrent requests can both read the same proSeatsRemaining > 0 value, pass the check, and each proceed to assign a seat — resulting in more seats consumed than the quota allows.
To prevent this, add a row lock to the allMembers query:
const allMembers = await tx
.select({
id: organizationMembers.id,
hasProSeat: organizationMembers.hasProSeat,
})
.from(organizationMembers)
.where(eq(organizationMembers.organizationId, organizationId))
.for("update");This serializes the seat availability check across concurrent requests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/toggle-pro-seat.ts
Line: 57-84
Comment:
**Race condition when checking available Pro seats**
The `allMembers` query inside the transaction (line 58) is executed without a row lock, while the single `member` row is locked with `.for("update")` on line 47. Two concurrent requests can both read the same `proSeatsRemaining > 0` value, pass the check, and each proceed to assign a seat — resulting in more seats consumed than the quota allows.
To prevent this, add a row lock to the `allMembers` query:
```ts
const allMembers = await tx
.select({
id: organizationMembers.id,
hasProSeat: organizationMembers.hasProSeat,
})
.from(organizationMembers)
.where(eq(organizationMembers.organizationId, organizationId))
.for("update");
```
This serializes the seat availability check across concurrent requests.
How can I resolve this? If you propose a fix, please make it concise.| if (enable) { | ||
| const allMembers = await tx | ||
| .select({ | ||
| id: organizationMembers.id, | ||
| hasProSeat: organizationMembers.hasProSeat, | ||
| }) | ||
| .from(organizationMembers) | ||
| .where(eq(organizationMembers.organizationId, organizationId)); | ||
|
|
||
| const [owner] = await tx | ||
| .select({ | ||
| inviteQuota: users.inviteQuota, | ||
| stripeSubscriptionId: users.stripeSubscriptionId, | ||
| }) | ||
| .from(users) | ||
| .where(eq(users.id, organization.ownerId)) | ||
| .limit(1); | ||
|
|
||
| const { proSeatsRemaining } = calculateProSeats({ | ||
| inviteQuota: owner?.inviteQuota ?? 1, | ||
| members: allMembers, | ||
| }); | ||
|
|
||
| if (proSeatsRemaining <= 0) { | ||
| throw new Error( | ||
| "No Pro seats remaining. Purchase more seats to continue.", | ||
| ); | ||
| } | ||
|
|
||
| await tx | ||
| .update(organizationMembers) | ||
| .set({ hasProSeat: true }) | ||
| .where(eq(organizationMembers.id, memberId)); |
There was a problem hiding this comment.
No idempotency guard when enable = true and seat is already assigned
If member.hasProSeat is already true and toggleProSeat is called with enable = true (possible via direct API call or duplicate UI events), the code still runs the full seat-count check. If the organization is at exactly its quota limit, proSeatsRemaining will be 0 and the call throws "No Pro seats remaining" — even though no new seat is actually being consumed.
Add an early exit before the if (enable) block to short-circuit when state is already desired:
if (member.hasProSeat === enable) {
// Already in the desired state, nothing to do
return { success: true };
}Insert this after the owner check (line 53) and before the if (enable) branch (line 57).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/toggle-pro-seat.ts
Line: 57-89
Comment:
**No idempotency guard when `enable = true` and seat is already assigned**
If `member.hasProSeat` is already `true` and `toggleProSeat` is called with `enable = true` (possible via direct API call or duplicate UI events), the code still runs the full seat-count check. If the organization is at exactly its quota limit, `proSeatsRemaining` will be `0` and the call throws "No Pro seats remaining" — even though no new seat is actually being consumed.
Add an early exit before the `if (enable)` block to short-circuit when state is already desired:
```ts
if (member.hasProSeat === enable) {
// Already in the desired state, nothing to do
return { success: true };
}
```
Insert this after the owner check (line 53) and before the `if (enable)` branch (line 57).
How can I resolve this? If you propose a fix, please make it concise.| await stripe().subscriptions.update(subscription.id, { | ||
| items: [ | ||
| { | ||
| id: subscriptionItem.id, | ||
| quantity: newQuantity, | ||
| }, | ||
| ], | ||
| proration_behavior: "create_prorations", | ||
| }); | ||
|
|
||
| await db() | ||
| .update(users) | ||
| .set({ inviteQuota: newQuantity }) | ||
| .where(eq(users.id, user.id)); |
There was a problem hiding this comment.
Stripe and database updates are not atomic
stripe().subscriptions.update() (line 132) and the subsequent db().update(users) (line 142) are executed as independent operations. If the database update fails after the Stripe subscription is already modified, the Stripe seat count and the local inviteQuota will be permanently out of sync — users could be billed for seats that don't appear in the product.
Wrap the DB update in a try/catch to detect and handle the failure:
await stripe().subscriptions.update(subscription.id, {
items: [{ id: subscriptionItem.id, quantity: newQuantity }],
proration_behavior: "create_prorations",
});
try {
await db()
.update(users)
.set({ inviteQuota: newQuantity })
.where(eq(users.id, user.id));
} catch (dbError) {
console.error(
"CRITICAL: Stripe updated to quantity",
newQuantity,
"but DB update failed for user",
user.id,
"—",
dbError,
);
throw new Error("Billing update succeeded but local state could not be saved. Please contact support.");
}This ensures a Stripe/DB divergence is logged and the client is aware of the failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/update-seat-quantity.ts
Line: 132-145
Comment:
**Stripe and database updates are not atomic**
`stripe().subscriptions.update()` (line 132) and the subsequent `db().update(users)` (line 142) are executed as independent operations. If the database update fails after the Stripe subscription is already modified, the Stripe seat count and the local `inviteQuota` will be permanently out of sync — users could be billed for seats that don't appear in the product.
Wrap the DB update in a try/catch to detect and handle the failure:
```ts
await stripe().subscriptions.update(subscription.id, {
items: [{ id: subscriptionItem.id, quantity: newQuantity }],
proration_behavior: "create_prorations",
});
try {
await db()
.update(users)
.set({ inviteQuota: newQuantity })
.where(eq(users.id, user.id));
} catch (dbError) {
console.error(
"CRITICAL: Stripe updated to quantity",
newQuantity,
"but DB update failed for user",
user.id,
"—",
dbError,
);
throw new Error("Billing update succeeded but local state could not be saved. Please contact support.");
}
```
This ensures a Stripe/DB divergence is logged and the client is aware of the failure.
How can I resolve this? If you propose a fix, please make it concise.| if (org && memberId) { | ||
| const [owner] = await tx | ||
| .select({ | ||
| inviteQuota: users.inviteQuota, | ||
| stripeSubscriptionId: users.stripeSubscriptionId, | ||
| }) | ||
| .from(users) | ||
| .where(eq(users.id, org.ownerId)) | ||
| .limit(1); | ||
|
|
||
| if (owner?.stripeSubscriptionId) { | ||
| const allMembers = await tx | ||
| .select({ | ||
| id: organizationMembers.id, | ||
| hasProSeat: organizationMembers.hasProSeat, | ||
| }) | ||
| .from(organizationMembers) | ||
| .where( | ||
| eq(organizationMembers.organizationId, invite.organizationId), | ||
| ) | ||
| .for("update"); | ||
|
|
||
| const { proSeatsRemaining } = calculateProSeats({ | ||
| inviteQuota: owner.inviteQuota ?? 1, | ||
| members: allMembers, | ||
| }); | ||
|
|
||
| if (proSeatsRemaining > 0) { | ||
| await tx | ||
| .update(organizationMembers) | ||
| .set({ hasProSeat: true }) | ||
| .where(eq(organizationMembers.id, memberId)); | ||
|
|
||
| await tx | ||
| .update(users) | ||
| .set({ | ||
| thirdPartyStripeSubscriptionId: owner.stripeSubscriptionId, | ||
| }) | ||
| .where(eq(users.id, user.id)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pro seat auto-assigned to existing members on re-accept
When existingMembership is found (the user is already a member), memberId is set to the existing record's ID (line 53) and the code still proceeds to the Pro-seat check (lines 72–112). If the existing member has hasProSeat: false and there are seats available, the invite acceptance silently upgrades them to a Pro seat — even though the owner never explicitly granted that permission.
Skip the Pro-seat allocation block when existingMembership is truthy:
if (org && memberId && !existingMembership) {
// only auto-assign Pro seat for genuinely new members
const [owner] = await tx
.select({
inviteQuota: users.inviteQuota,
stripeSubscriptionId: users.stripeSubscriptionId,
})
.from(users)
.where(eq(users.id, org.ownerId))
.limit(1);
if (owner?.stripeSubscriptionId) {
// ... existing seat logic ...
}
}This prevents unintended permission escalation for users re-accepting invites.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/invite/accept/route.ts
Line: 72-112
Comment:
**Pro seat auto-assigned to existing members on re-accept**
When `existingMembership` is found (the user is already a member), `memberId` is set to the existing record's ID (line 53) and the code still proceeds to the Pro-seat check (lines 72–112). If the existing member has `hasProSeat: false` and there are seats available, the invite acceptance silently upgrades them to a Pro seat — even though the owner never explicitly granted that permission.
Skip the Pro-seat allocation block when `existingMembership` is truthy:
```ts
if (org && memberId && !existingMembership) {
// only auto-assign Pro seat for genuinely new members
const [owner] = await tx
.select({
inviteQuota: users.inviteQuota,
stripeSubscriptionId: users.stripeSubscriptionId,
})
.from(users)
.where(eq(users.id, org.ownerId))
.limit(1);
if (owner?.stripeSubscriptionId) {
// ... existing seat logic ...
}
}
```
This prevents unintended permission escalation for users re-accepting invites.
How can I resolve this? If you propose a fix, please make it concise.
| throw new Error("Only the owner can view subscription details"); | ||
|
|
||
| const [owner] = await db() | ||
| .select({ |
There was a problem hiding this comment.
stripeSubscriptionStatus is selected but unused here; removing it avoids unused-data drift.
| .select({ | |
| .select({ | |
| stripeSubscriptionId: users.stripeSubscriptionId, | |
| }) |
| status: subscription.status, | ||
| billingInterval: interval, | ||
| pricePerSeat, | ||
| currentQuantity: item.quantity || 1, |
There was a problem hiding this comment.
Minor: || 1 treats 0 as missing. ?? 1 is safer for nullable quantities.
| currentQuantity: item.quantity || 1, | |
| currentQuantity: item.quantity ?? 1, |
|
@greptileai please review again |
| if (failedInvites.length > 0) { | ||
| await db() | ||
| .delete(organizationInvites) | ||
| .where( | ||
| inArray( | ||
| organizationInvites.id, | ||
| failedInvites.map((r) => r.id), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unhandled cleanup failure can orphan invite records
When email delivery fails, the subsequent db().delete() cleanup (lines 136–145) runs outside the transaction with no error handling. If this delete fails due to a transient DB error, the undelivered invite records will persist indefinitely — creating a data integrity gap. A user who somehow obtains the invite ID (e.g., from a server log) could still accept the invite without having been notified.
Wrap the cleanup in a try/catch to detect and log the failure:
| if (failedInvites.length > 0) { | |
| await db() | |
| .delete(organizationInvites) | |
| .where( | |
| inArray( | |
| organizationInvites.id, | |
| failedInvites.map((r) => r.id), | |
| ), | |
| ); | |
| } | |
| if (failedInvites.length > 0) { | |
| try { | |
| await db() | |
| .delete(organizationInvites) | |
| .where( | |
| inArray( | |
| organizationInvites.id, | |
| failedInvites.map((r) => r.id), | |
| ), | |
| ); | |
| } catch (cleanupError) { | |
| console.error( | |
| "CRITICAL: Failed to clean up undelivered invite records:", | |
| failedInvites.map((r) => r.id), | |
| cleanupError, | |
| ); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/send-invites.ts
Line: 136-145
Comment:
**Unhandled cleanup failure can orphan invite records**
When email delivery fails, the subsequent `db().delete()` cleanup (lines 136–145) runs outside the transaction with no error handling. If this delete fails due to a transient DB error, the undelivered invite records will persist indefinitely — creating a data integrity gap. A user who somehow obtains the invite ID (e.g., from a server log) could still accept the invite without having been notified.
Wrap the cleanup in a try/catch to detect and log the failure:
```suggestion
if (failedInvites.length > 0) {
try {
await db()
.delete(organizationInvites)
.where(
inArray(
organizationInvites.id,
failedInvites.map((r) => r.id),
),
);
} catch (cleanupError) {
console.error(
"CRITICAL: Failed to clean up undelivered invite records:",
failedInvites.map((r) => r.id),
cleanupError,
);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (!user.activeOrganizationId) { | ||
| userUpdate.activeOrganizationId = invite.organizationId; | ||
| } | ||
| if (!user.defaultOrgId) { | ||
| userUpdate.defaultOrgId = invite.organizationId; | ||
| } |
There was a problem hiding this comment.
Accepting an invite never switches user context to the new organization
When a user accepts an invite for organization B while already having an activeOrganizationId (from organization A), they are silently added as a member of B but their context is never updated. After accepting, the user lands on their old organization's dashboard with no indication that they successfully joined the new one.
The code only updates activeOrganizationId and defaultOrgId if they are currently empty — which means existing users are never switched:
if (!user.activeOrganizationId) {
userUpdate.activeOrganizationId = invite.organizationId;
}To fix this regression, always switch context for a genuinely new membership:
| if (!user.activeOrganizationId) { | |
| userUpdate.activeOrganizationId = invite.organizationId; | |
| } | |
| if (!user.defaultOrgId) { | |
| userUpdate.defaultOrgId = invite.organizationId; | |
| } | |
| // Always switch context for a genuinely new membership | |
| if (!existingMembership) { | |
| userUpdate.activeOrganizationId = invite.organizationId; | |
| userUpdate.defaultOrgId = invite.organizationId; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/invite/accept/route.ts
Line: 133-138
Comment:
**Accepting an invite never switches user context to the new organization**
When a user accepts an invite for organization B while already having an `activeOrganizationId` (from organization A), they are silently added as a member of B but their context is never updated. After accepting, the user lands on their old organization's dashboard with no indication that they successfully joined the new one.
The code only updates `activeOrganizationId` and `defaultOrgId` if they are currently empty — which means existing users are never switched:
```ts
if (!user.activeOrganizationId) {
userUpdate.activeOrganizationId = invite.organizationId;
}
```
To fix this regression, always switch context for a genuinely new membership:
```suggestion
// Always switch context for a genuinely new membership
if (!existingMembership) {
userUpdate.activeOrganizationId = invite.organizationId;
userUpdate.defaultOrgId = invite.organizationId;
}
```
How can I resolve this? If you propose a fix, please make it concise.| <button | ||
| type="button" | ||
| onClick={() => updateDesiredQuantity(desiredQuantity + 1)} | ||
| className="flex justify-center items-center w-8 h-8 rounded-r-md bg-gray-4 hover:bg-gray-5" | ||
| > | ||
| <Plus className="w-4 h-4 text-gray-12" /> | ||
| </button> |
There was a problem hiding this comment.
Increment button lacks upper-bound guard, triggering rejected preview API calls
The decrease button correctly disables when !canDecrease, but the increase button has no corresponding guard. A user can click + an arbitrary number of times (e.g., to 501, 1000, …), each triggering a debounced preview API call that will be rejected server-side with a 400. This creates unnecessary Stripe API calls and a confusing UX where the preview label says "Calculating..." for requests that will ultimately fail.
Add a disabled check on the increment button to enforce the 500-seat maximum:
| <button | |
| type="button" | |
| onClick={() => updateDesiredQuantity(desiredQuantity + 1)} | |
| className="flex justify-center items-center w-8 h-8 rounded-r-md bg-gray-4 hover:bg-gray-5" | |
| > | |
| <Plus className="w-4 h-4 text-gray-12" /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => | |
| desiredQuantity < 500 && updateDesiredQuantity(desiredQuantity + 1) | |
| } | |
| className="flex justify-center items-center w-8 h-8 rounded-r-md bg-gray-4 hover:bg-gray-5 disabled:opacity-50 disabled:cursor-not-allowed" | |
| disabled={desiredQuantity >= 500} | |
| > | |
| <Plus className="w-4 h-4 text-gray-12" /> | |
| </button> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/SeatManagementCard.tsx
Line: 133-139
Comment:
**Increment button lacks upper-bound guard, triggering rejected preview API calls**
The decrease button correctly disables when `!canDecrease`, but the increase button has no corresponding guard. A user can click `+` an arbitrary number of times (e.g., to 501, 1000, …), each triggering a debounced preview API call that will be rejected server-side with a 400. This creates unnecessary Stripe API calls and a confusing UX where the preview label says "Calculating..." for requests that will ultimately fail.
Add a `disabled` check on the increment button to enforce the 500-seat maximum:
```suggestion
<button
type="button"
onClick={() =>
desiredQuantity < 500 && updateDesiredQuantity(desiredQuantity + 1)
}
className="flex justify-center items-center w-8 h-8 rounded-r-md bg-gray-4 hover:bg-gray-5 disabled:opacity-50 disabled:cursor-not-allowed"
disabled={desiredQuantity >= 500}
>
<Plus className="w-4 h-4 text-gray-12" />
</button>
```
How can I resolve this? If you propose a fix, please make it concise.
|
@greptileai please review again |
| const MAX_VISIBLE = 4; | ||
|
|
||
| export function MemberAvatars() { | ||
| const { activeOrganization, sidebarCollapsed, setInviteDialogOpen } = |
There was a problem hiding this comment.
This shows an invite affordance in the sidebar; grabbing user here makes it easy to hide the invite prompt for non-owners.
| const { activeOrganization, sidebarCollapsed, setInviteDialogOpen } = | |
| const { activeOrganization, sidebarCollapsed, setInviteDialogOpen, user } = | |
| useDashboardContext(); |
| </div> | ||
| )} | ||
|
|
||
| {Array.from({ length: emptySlots }).map((_, i) => ( |
There was a problem hiding this comment.
Might be worth only rendering the empty invite slots for org owners; otherwise non-owners get a tooltip/button that will fail on submit.
| {Array.from({ length: emptySlots }).map((_, i) => ( | |
| {activeOrganization?.organization.ownerId === user.id && | |
| Array.from({ length: emptySlots }).map((_, i) => ( |
| {Array.from({ length: emptySlots }).map((_, i) => ( | ||
| <Tooltip | ||
| key={`empty-${i}`} | ||
| content="Invite to your organization" | ||
| position="bottom" | ||
| delayDuration={0} | ||
| > | ||
| <button | ||
| type="button" | ||
| onClick={() => setInviteDialogOpen(true)} | ||
| className="-ml-1.5 flex items-center justify-center size-6 rounded-full border border-dashed border-gray-8 bg-gray-3 hover:bg-gray-4 hover:border-gray-9 transition-colors" | ||
| > | ||
| <Plus className="size-3 text-gray-10" /> | ||
| </button> | ||
| </Tooltip> | ||
| ))} |
There was a problem hiding this comment.
Invite slots rendered for non-owner members
The empty-slot invite buttons are shown and clickable for all users. There is no ownership check, so regular members will see up to 4 dashed invite circles, click them, and submit — only to receive a server-level error.
The component has access to the user and organization data via useDashboardContext() (which includes both user and activeOrganization). Add an ownership check before rendering the invite buttons:
const { activeOrganization, user, sidebarCollapsed, setInviteDialogOpen } =
useDashboardContext();
const isOwner = user?.id === activeOrganization?.organization.ownerId;Then conditionally render the empty slot buttons only when isOwner is true.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/_components/Navbar/MemberAvatars.tsx
Line: 49-64
Comment:
**Invite slots rendered for non-owner members**
The empty-slot invite buttons are shown and clickable for all users. There is no ownership check, so regular members will see up to 4 dashed invite circles, click them, and submit — only to receive a server-level error.
The component has access to the user and organization data via `useDashboardContext()` (which includes both `user` and `activeOrganization`). Add an ownership check before rendering the invite buttons:
```tsx
const { activeOrganization, user, sidebarCollapsed, setInviteDialogOpen } =
useDashboardContext();
const isOwner = user?.id === activeOrganization?.organization.ownerId;
```
Then conditionally render the empty slot buttons only when `isOwner` is `true`.
How can I resolve this? If you propose a fix, please make it concise.| const { data: subscription, isLoading } = | ||
| useQuery<SubscriptionDetails | null>({ | ||
| queryKey: ["subscription-details", organizationId], | ||
| queryFn: () => { | ||
| if (!organizationId) return null; | ||
| return getSubscriptionDetails(organizationId); | ||
| }, | ||
| enabled: !!organizationId, | ||
| staleTime: 60 * 1000, | ||
| }); | ||
|
|
||
| const handleManageBilling = useCallback(async () => { | ||
| setBillingLoading(true); | ||
| try { | ||
| const url = await manageBilling(); | ||
| router.push(url); | ||
| } catch { | ||
| toast.error("An error occurred while managing billing"); | ||
| } finally { | ||
| setBillingLoading(false); | ||
| } | ||
| }, [router]); | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <Card> | ||
| <div className="flex flex-col gap-3 animate-pulse"> | ||
| <div className="h-5 w-32 bg-gray-4 rounded" /> | ||
| <div className="h-4 w-48 bg-gray-4 rounded" /> | ||
| <div className="h-4 w-40 bg-gray-4 rounded" /> | ||
| </div> | ||
| </Card> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Query errors silently render the "Upgrade to Pro" upsell card
When getSubscriptionDetails throws (network error, Stripe outage, etc.), data becomes undefined and isLoading becomes false. The check !subscription then evaluates to true and renders the "Upgrade to Cap Pro" card. A Pro subscriber experiencing a temporary API error would see an upsell prompt, which is confusing.
Distinguish the error state by also reading isError from the query:
const { data: subscription, isLoading, isError } =
useQuery<SubscriptionDetails | null>({ ... });
if (isError) {
return (
<Card>
<p className="text-sm text-gray-10">
Unable to load billing details. Please try again later.
</p>
</Card>
);
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/BillingSummaryCard.tsx
Line: 22-56
Comment:
**Query errors silently render the "Upgrade to Pro" upsell card**
When `getSubscriptionDetails` throws (network error, Stripe outage, etc.), `data` becomes `undefined` and `isLoading` becomes `false`. The check `!subscription` then evaluates to `true` and renders the "Upgrade to Cap Pro" card. A Pro subscriber experiencing a temporary API error would see an upsell prompt, which is confusing.
Distinguish the error state by also reading `isError` from the query:
```tsx
const { data: subscription, isLoading, isError } =
useQuery<SubscriptionDetails | null>({ ... });
if (isError) {
return (
<Card>
<p className="text-sm text-gray-10">
Unable to load billing details. Please try again later.
</p>
</Card>
);
}
```
How can I resolve this? If you propose a fix, please make it concise.| const { data: preview, isFetching: previewLoading } = useQuery({ | ||
| queryKey: ["seat-preview", organizationId, debouncedQuantity], | ||
| queryFn: () => { | ||
| if (!organizationId) return null; | ||
| return previewSeatChange(organizationId, debouncedQuantity); | ||
| }, | ||
| enabled: !!organizationId && debouncedHasChanges, | ||
| staleTime: 30 * 1000, | ||
| }); | ||
|
|
||
| const updateMutation = useMutation({ | ||
| mutationFn: () => { | ||
| if (!organizationId) throw new Error("No organization"); | ||
| return updateSeatQuantity(organizationId, desiredQuantity); | ||
| }, | ||
| onSuccess: (result) => { | ||
| toast.success(`Seat quantity updated to ${result.newQuantity}`); | ||
| queryClient.setQueriesData<SubscriptionDetails | null>( | ||
| { queryKey: ["subscription-details", organizationId] }, | ||
| (old) => (old ? { ...old, currentQuantity: result.newQuantity } : old), | ||
| ); | ||
| router.refresh(); | ||
| }, | ||
| onError: (error) => { | ||
| toast.error( | ||
| error instanceof Error ? error.message : "Failed to update seats", | ||
| ); | ||
| }, | ||
| }); | ||
|
|
||
| const MAX_SEATS = 500; | ||
| const canDecrease = desiredQuantity > Math.max(1, proSeatsUsed); | ||
| const canIncrease = desiredQuantity < MAX_SEATS; | ||
|
|
||
| return ( | ||
| <Card> | ||
| <CardHeader> | ||
| <CardTitle>Pro Seats</CardTitle> | ||
| <CardDescription> | ||
| Manage how many Pro seats are available for your team. | ||
| </CardDescription> | ||
| </CardHeader> | ||
| <div className="flex flex-col gap-5 mt-4"> | ||
| <div className="flex items-center gap-3 text-sm text-gray-11"> | ||
| <span> | ||
| <span className="font-medium text-gray-12">{proSeatsUsed}</span> of{" "} | ||
| <span className="font-medium text-gray-12">{proSeatsTotal}</span>{" "} | ||
| Pro seats assigned | ||
| </span> | ||
| </div> | ||
|
|
||
| <div className="flex flex-wrap items-center gap-4"> | ||
| <div className="flex items-center gap-1"> | ||
| <span className="mr-2 text-sm text-gray-12">Seats:</span> | ||
| <button | ||
| type="button" | ||
| onClick={() => | ||
| canDecrease && updateDesiredQuantity(desiredQuantity - 1) | ||
| } | ||
| className="flex justify-center items-center w-8 h-8 rounded-l-md bg-gray-4 hover:bg-gray-5 disabled:opacity-50 disabled:cursor-not-allowed" | ||
| disabled={!canDecrease} | ||
| > | ||
| <Minus className="w-4 h-4 text-gray-12" /> | ||
| </button> | ||
| <NumberFlow | ||
| value={desiredQuantity} | ||
| className="mx-auto w-8 text-sm tabular-nums text-center text-gray-12" | ||
| /> | ||
| <button | ||
| type="button" | ||
| onClick={() => | ||
| canIncrease && updateDesiredQuantity(desiredQuantity + 1) | ||
| } | ||
| className="flex justify-center items-center w-8 h-8 rounded-r-md bg-gray-4 hover:bg-gray-5 disabled:opacity-50 disabled:cursor-not-allowed" | ||
| disabled={!canIncrease} | ||
| > | ||
| <Plus className="w-4 h-4 text-gray-12" /> | ||
| </button> | ||
| </div> | ||
|
|
||
| {hasChanges && ( | ||
| <div className="flex items-center gap-3"> | ||
| {previewLoading ? ( | ||
| <span className="text-sm text-gray-10">Calculating...</span> | ||
| ) : preview ? ( | ||
| <span className="text-sm text-gray-11"> | ||
| Prorated charge: ${(preview.proratedAmount / 100).toFixed(2)}{" "} | ||
| {preview.currency.toUpperCase()} | ||
| </span> | ||
| ) : null} |
There was a problem hiding this comment.
Preview query errors are not surfaced to the user
When previewSeatChange rejects (e.g., server-side validation fails), React Query enters an error state. However, the component only branches on previewLoading and preview, so when an error occurs, the JSX returns null with no message — giving no indication of why a preview isn't showing.
Read isError from the query and show an inline error message:
const { data: preview, isFetching: previewLoading, isError: previewError } = useQuery({ ... });
// In the JSX:
{previewLoading ? (
<span className="text-sm text-gray-10">Calculating...</span>
) : previewError ? (
<span className="text-sm text-red-500">Unable to calculate preview</span>
) : preview ? (
<span className="text-sm text-gray-11">
Prorated charge: ${(preview.proratedAmount / 100).toFixed(2)}{" "}
{preview.currency.toUpperCase()}
</span>
) : null}Also consider disabling the "Confirm" button when previewError is true, since submitting in this state would produce a server-side error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/SeatManagementCard.tsx
Line: 67-156
Comment:
**Preview query errors are not surfaced to the user**
When `previewSeatChange` rejects (e.g., server-side validation fails), React Query enters an error state. However, the component only branches on `previewLoading` and `preview`, so when an error occurs, the JSX returns `null` with no message — giving no indication of why a preview isn't showing.
Read `isError` from the query and show an inline error message:
```tsx
const { data: preview, isFetching: previewLoading, isError: previewError } = useQuery({ ... });
// In the JSX:
{previewLoading ? (
<span className="text-sm text-gray-10">Calculating...</span>
) : previewError ? (
<span className="text-sm text-red-500">Unable to calculate preview</span>
) : preview ? (
<span className="text-sm text-gray-11">
Prorated charge: ${(preview.proratedAmount / 100).toFixed(2)}{" "}
{preview.currency.toUpperCase()}
</span>
) : null}
```
Also consider disabling the "Confirm" button when `previewError` is true, since submitting in this state would produce a server-side error.
How can I resolve this? If you propose a fix, please make it concise.| } else { | ||
| await tx | ||
| .update(organizationMembers) | ||
| .set({ hasProSeat: false }) | ||
| .where(eq(organizationMembers.id, memberId)); | ||
|
|
||
| const otherProSeats = await tx | ||
| .select({ id: organizationMembers.id }) | ||
| .from(organizationMembers) | ||
| .where( | ||
| and( | ||
| eq(organizationMembers.userId, member.userId), | ||
| eq(organizationMembers.hasProSeat, true), | ||
| ), | ||
| ) | ||
| .limit(1); | ||
|
|
||
| if (otherProSeats.length === 0) { | ||
| await tx | ||
| .update(users) | ||
| .set({ thirdPartyStripeSubscriptionId: null }) | ||
| .where(eq(users.id, member.userId)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale thirdPartyStripeSubscriptionId when user holds Pro seats across multiple orgs
When revoking a Pro seat, the query on lines 108–117 checks for any other hasProSeat = true record for the same userId without filtering by organization. If the user belongs to Org A (subscription X) and Org B (subscription Y), and you revoke their seat in Org A, the query finds their Org B seat and skips the subscription ID update — leaving thirdPartyStripeSubscriptionId unchanged, still pointing to Org A's subscription X. The user retains a subscription ID that no longer applies in the current organization context.
Update the code to set the correct subscription when other Pro seats exist:
if (otherProSeats.length === 0) {
await tx
.update(users)
.set({ thirdPartyStripeSubscriptionId: null })
.where(eq(users.id, member.userId));
} else {
// Update to the subscription of a remaining Pro-seat org
const [remainingOrg] = await tx
.select({ stripeSubscriptionId: users.stripeSubscriptionId })
.from(organizationMembers)
.innerJoin(organizations, eq(organizationMembers.organizationId, organizations.id))
.innerJoin(users, eq(organizations.ownerId, users.id))
.where(
and(
eq(organizationMembers.userId, member.userId),
eq(organizationMembers.hasProSeat, true),
),
)
.limit(1);
if (remainingOrg?.stripeSubscriptionId) {
await tx
.update(users)
.set({ thirdPartyStripeSubscriptionId: remainingOrg.stripeSubscriptionId })
.where(eq(users.id, member.userId));
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/toggle-pro-seat.ts
Line: 102-125
Comment:
**Stale `thirdPartyStripeSubscriptionId` when user holds Pro seats across multiple orgs**
When revoking a Pro seat, the query on lines 108–117 checks for any other `hasProSeat = true` record for the same `userId` **without filtering by organization**. If the user belongs to Org A (subscription X) and Org B (subscription Y), and you revoke their seat in Org A, the query finds their Org B seat and skips the subscription ID update — leaving `thirdPartyStripeSubscriptionId` unchanged, still pointing to Org A's subscription X. The user retains a subscription ID that no longer applies in the current organization context.
Update the code to set the correct subscription when other Pro seats exist:
```ts
if (otherProSeats.length === 0) {
await tx
.update(users)
.set({ thirdPartyStripeSubscriptionId: null })
.where(eq(users.id, member.userId));
} else {
// Update to the subscription of a remaining Pro-seat org
const [remainingOrg] = await tx
.select({ stripeSubscriptionId: users.stripeSubscriptionId })
.from(organizationMembers)
.innerJoin(organizations, eq(organizationMembers.organizationId, organizations.id))
.innerJoin(users, eq(organizations.ownerId, users.id))
.where(
and(
eq(organizationMembers.userId, member.userId),
eq(organizationMembers.hasProSeat, true),
),
)
.limit(1);
if (remainingOrg?.stripeSubscriptionId) {
await tx
.update(users)
.set({ thirdPartyStripeSubscriptionId: remainingOrg.stripeSubscriptionId })
.where(eq(users.id, member.userId));
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
|
@greptileai please review again |
| size="sm" | ||
| variant="primary" | ||
| onClick={() => updateMutation.mutate()} | ||
| disabled={ |
There was a problem hiding this comment.
While the debounce timer is still running (desiredQuantity !== debouncedQuantity), previewLoading is false and the Confirm button becomes clickable before the preview calculation has started.
| disabled={ | |
| disabled={ | |
| updateMutation.isPending || | |
| previewLoading || | |
| previewError || | |
| desiredQuantity !== debouncedQuantity | |
| } |
| ) | ||
| .limit(1); | ||
|
|
||
| if (remainingOrg?.stripeSubscriptionId) { |
There was a problem hiding this comment.
If otherProSeats exists but no remaining org has a stripeSubscriptionId, this leaves thirdPartyStripeSubscriptionId unchanged (potentially still pointing at the just-revoked org). Safer to explicitly clear it.
| if (remainingOrg?.stripeSubscriptionId) { | |
| if (remainingOrg?.stripeSubscriptionId) { | |
| await tx | |
| .update(users) | |
| .set({ | |
| thirdPartyStripeSubscriptionId: remainingOrg.stripeSubscriptionId, | |
| }) | |
| .where(eq(users.id, member.userId)); | |
| } else { | |
| await tx | |
| .update(users) | |
| .set({ thirdPartyStripeSubscriptionId: null }) | |
| .where(eq(users.id, member.userId)); | |
| } |
| const updateDesiredQuantity = (newQuantity: number) => { | ||
| setDesiredQuantity(newQuantity); | ||
| if (debounceTimer.current) { | ||
| clearTimeout(debounceTimer.current); | ||
| } | ||
| debounceTimer.current = setTimeout(() => { | ||
| setDebouncedQuantity(newQuantity); | ||
| }, DEBOUNCE_MS); | ||
| }; | ||
|
|
||
| const hasChanges = desiredQuantity !== proSeatsTotal; | ||
| const debouncedHasChanges = debouncedQuantity !== proSeatsTotal; | ||
|
|
||
| const { | ||
| data: preview, | ||
| isFetching: previewLoading, | ||
| isError: previewError, | ||
| } = useQuery({ | ||
| queryKey: ["seat-preview", organizationId, debouncedQuantity], | ||
| queryFn: () => { | ||
| if (!organizationId) return null; | ||
| return previewSeatChange(organizationId, debouncedQuantity); | ||
| }, | ||
| enabled: !!organizationId && debouncedHasChanges, | ||
| staleTime: 30 * 1000, | ||
| }); | ||
|
|
||
| const updateMutation = useMutation({ | ||
| mutationFn: () => { | ||
| if (!organizationId) throw new Error("No organization"); | ||
| return updateSeatQuantity(organizationId, desiredQuantity); |
There was a problem hiding this comment.
Preview/confirm quantity mismatch during debounce window — The preview query uses debouncedQuantity (line 72), but the update mutation sends desiredQuantity (line 84). The Confirm button disables only when previewLoading || previewError (line 171), not when the debounce is in-flight. If a user rapidly clicks + and then "Confirm" within the 500 ms debounce window, the preview reflects the old debounced value while Stripe is charged for the current desiredQuantity — potentially differing by one or more seats.
Prevent confirming a stale preview by disabling the Confirm button when desiredQuantity !== debouncedQuantity:
const debounceInFlight = desiredQuantity !== debouncedQuantity;
<Button
type="button"
size="sm"
variant="primary"
onClick={() => updateMutation.mutate()}
disabled={
updateMutation.isPending ||
previewLoading ||
previewError ||
debounceInFlight // ← prevent confirming while debounce is pending
}
spinner={updateMutation.isPending}
>
{updateMutation.isPending ? "Updating..." : "Confirm"}
</Button>Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/SeatManagementCard.tsx
Line: 54-84
Comment:
**Preview/confirm quantity mismatch during debounce window** — The preview query uses `debouncedQuantity` (line 72), but the update mutation sends `desiredQuantity` (line 84). The Confirm button disables only when `previewLoading || previewError` (line 171), not when the debounce is in-flight. If a user rapidly clicks `+` and then "Confirm" within the 500 ms debounce window, the preview reflects the old debounced value while Stripe is charged for the current `desiredQuantity` — potentially differing by one or more seats.
Prevent confirming a stale preview by disabling the Confirm button when `desiredQuantity !== debouncedQuantity`:
```ts
const debounceInFlight = desiredQuantity !== debouncedQuantity;
<Button
type="button"
size="sm"
variant="primary"
onClick={() => updateMutation.mutate()}
disabled={
updateMutation.isPending ||
previewLoading ||
previewError ||
debounceInFlight // ← prevent confirming while debounce is pending
}
spinner={updateMutation.isPending}
>
{updateMutation.isPending ? "Updating..." : "Confirm"}
</Button>
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Add client-side email validation before adding to the list: const handleAddEmails = () => {
const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
const newEmails = emailInput
.split(",")
.map((email) => email.trim())
.filter((email) => email !== "");
const invalidEmails = newEmails.filter((e) => !emailRegex.test(e));
if (invalidEmails.length > 0) {
toast.error(`Invalid email address(es): ${invalidEmails.join(", ")}`);
}
const validNewEmails = newEmails.filter((e) => emailRegex.test(e));
setInviteEmails([...new Set([...inviteEmails, ...validNewEmails])]);
setEmailInput("");
};And use the onSuccess: (result) => {
if (result.failedEmails.length > 0) {
toast.warning(
`Invites sent, but delivery failed for: ${result.failedEmails.join(", ")}`
);
} else {
toast.success("Invites sent successfully");
}
setInviteEmails([]);
setIsOpen(false);
router.refresh();
},Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/InviteDialog.tsx
Line: 33-62
Comment:
**Silent success on invalid or failed invites** — `handleAddEmails` sends all emails to the server without client-side format validation, and the `onSuccess` callback ignores the `failedEmails` returned by `sendOrganizationInvites`. When a user enters invalid addresses (e.g., "notanemail"), they are sent to the server, filtered out by server-side regex, and the user sees "Invites sent successfully" despite no invites actually being created.
Add client-side email validation before adding to the list:
```ts
const handleAddEmails = () => {
const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
const newEmails = emailInput
.split(",")
.map((email) => email.trim())
.filter((email) => email !== "");
const invalidEmails = newEmails.filter((e) => !emailRegex.test(e));
if (invalidEmails.length > 0) {
toast.error(`Invalid email address(es): ${invalidEmails.join(", ")}`);
}
const validNewEmails = newEmails.filter((e) => emailRegex.test(e));
setInviteEmails([...new Set([...inviteEmails, ...validNewEmails])]);
setEmailInput("");
};
```
And use the `failedEmails` return value in `onSuccess` to surface partial failures:
```ts
onSuccess: (result) => {
if (result.failedEmails.length > 0) {
toast.warning(
`Invites sent, but delivery failed for: ${result.failedEmails.join(", ")}`
);
} else {
toast.success("Invites sent successfully");
}
setInviteEmails([]);
setIsOpen(false);
router.refresh();
},
```
How can I resolve this? If you propose a fix, please make it concise. |
|
@greptileai please review again from scratch. remember, in this PR we are making it so that free users can be invited to an org... alongside being able to still purchase pro seats. |
| } | ||
| } | ||
|
|
||
| revalidatePath("/dashboard/settings/organization"); |
There was a problem hiding this comment.
Since settings now live under /dashboard/settings/organization/*, this won’t invalidate the tab routes. Revalidating the layout should cover /billing, /preferences, etc.
| revalidatePath("/dashboard/settings/organization"); | |
| revalidatePath("/dashboard/settings/organization", "layout"); |
| <p> | ||
| ${subscription.pricePerSeat.toFixed(2)}/seat/mo ( | ||
| {subscription.currentQuantity}{" "} | ||
| {subscription.currentQuantity === 1 ? "seat" : "seats"} = $ | ||
| {totalAmount.toFixed(2)}/mo, billed {intervalLabel}) | ||
| </p> |
There was a problem hiding this comment.
subscription.currency is available but the UI hardcodes $ here, which will read wrong for non-USD subscriptions.
| <p> | |
| ${subscription.pricePerSeat.toFixed(2)}/seat/mo ( | |
| {subscription.currentQuantity}{" "} | |
| {subscription.currentQuantity === 1 ? "seat" : "seats"} = $ | |
| {totalAmount.toFixed(2)}/mo, billed {intervalLabel}) | |
| </p> | |
| <p> | |
| {subscription.currency.toUpperCase()} {subscription.pricePerSeat.toFixed(2)}/seat/mo ( | |
| {subscription.currentQuantity}{" "} | |
| {subscription.currentQuantity === 1 ? "seat" : "seats"} ={" "} | |
| {subscription.currency.toUpperCase()} {totalAmount.toFixed(2)}/mo, billed {intervalLabel}) | |
| </p> |
| }); | ||
|
|
||
| const currentQuantity = subscriptionItem.quantity ?? 1; | ||
| const proratedAmount = preview.amount_due; |
There was a problem hiding this comment.
preview.amount_due reflects the full upcoming invoice total, not the incremental proration cost. When stripe().invoices.retrieveUpcoming() is called with prorated items, amount_due includes both the base subscription renewal charge for the next period and any mid-cycle proration adjustments. The UI label on line 162 of SeatManagementCard.tsx ("Prorated charge: $X") therefore misleads users into thinking the full invoice amount is the seat-change cost.
To isolate the actual proration amount, filter the preview invoice line items:
const proratedAmount = preview.lines.data
.filter((line) => line.proration)
.reduce((sum, line) => sum + line.amount, 0);Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/update-seat-quantity.ts
Line: 112
Comment:
`preview.amount_due` reflects the **full upcoming invoice total**, not the incremental proration cost. When `stripe().invoices.retrieveUpcoming()` is called with prorated items, `amount_due` includes both the base subscription renewal charge for the next period **and** any mid-cycle proration adjustments. The UI label on line 162 of `SeatManagementCard.tsx` ("Prorated charge: $X") therefore misleads users into thinking the full invoice amount is the seat-change cost.
To isolate the actual proration amount, filter the preview invoice line items:
```ts
const proratedAmount = preview.lines.data
.filter((line) => line.proration)
.reduce((sum, line) => sum + line.amount, 0);
```
How can I resolve this? If you propose a fix, please make it concise.| href: "/dashboard/settings/organization/preferences", | ||
| }, | ||
| { | ||
| label: "Billing & Members", |
There was a problem hiding this comment.
The "Billing & Members" tab label (line 15) is rendered unconditionally, but in billing/page.tsx the BillingSummaryCard and SeatManagementCard are only shown when buildEnv.NEXT_PUBLIC_IS_CAP is true. On self-hosted instances, users will navigate to a tab labeled "Billing & Members" and only see the Members section — the "Billing" part is misleading.
Consider conditionally adjusting the tab label:
{
label: buildEnv.NEXT_PUBLIC_IS_CAP ? "Billing & Members" : "Members",
href: "/dashboard/settings/organization/billing",
},Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/_components/SettingsNav.tsx
Line: 15
Comment:
The "Billing & Members" tab label (line 15) is rendered unconditionally, but in `billing/page.tsx` the `BillingSummaryCard` and `SeatManagementCard` are only shown when `buildEnv.NEXT_PUBLIC_IS_CAP` is true. On self-hosted instances, users will navigate to a tab labeled "Billing & Members" and only see the Members section — the "Billing" part is misleading.
Consider conditionally adjusting the tab label:
```ts
{
label: buildEnv.NEXT_PUBLIC_IS_CAP ? "Billing & Members" : "Members",
href: "/dashboard/settings/organization/billing",
},
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Add a reset handler to the <Dialog
open={isOpen}
onOpenChange={(open) => {
if (!open) {
setInviteEmails([]);
setEmailInput("");
}
setIsOpen(open);
}}
>Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/InviteDialog.tsx
Line: 89
Comment:
When a user adds email addresses and closes the dialog (via Escape, clicking outside, or "Cancel") without submitting, the `inviteEmails` and `emailInput` state persist. The next time the dialog is opened, the stale email list is still visible — a minor UX regression where users must manually clear previous entries.
Add a reset handler to the `onOpenChange` callback on the `Dialog`:
```tsx
<Dialog
open={isOpen}
onOpenChange={(open) => {
if (!open) {
setInviteEmails([]);
setEmailInput("");
}
setIsOpen(open);
}}
>
```
How can I resolve this? If you propose a fix, please make it concise. |
|
@greptileai please review again from scratch. remember, in this PR we are making it so that free users can be invited to an org... alongside being able to still purchase pro seats. |
| : preview.lines.data; | ||
|
|
||
| const currentQuantity = subscriptionItem.quantity ?? 1; | ||
| const proratedAmount = previewLines.reduce((total, line) => { |
There was a problem hiding this comment.
If this preview is meant to reflect what the customer will actually be charged/credited, summing only line.amount may undercount when automatic tax is enabled (tax is in tax_amounts).
| const proratedAmount = previewLines.reduce((total, line) => { | |
| const proratedAmount = previewLines.reduce((total, line) => { | |
| if (!line.proration) return total; | |
| const taxAmount = | |
| line.tax_amounts?.reduce((sum, t) => sum + t.amount, 0) ?? 0; | |
| return total + line.amount + taxAmount; | |
| }, 0); |
| <span className="text-sm text-gray-11"> | ||
| {preview.proratedAmount === 0 | ||
| ? "No prorated adjustment" | ||
| : `${preview.proratedAmount > 0 ? "Prorated charge" : "Prorated credit"}: $${Math.abs(preview.proratedAmount / 100).toFixed(2)} ${preview.currency.toUpperCase()}`} |
There was a problem hiding this comment.
This still hardcodes $, so non-USD proration will read wrong. Using Intl.NumberFormat keeps the symbol/code consistent with preview.currency.
| : `${preview.proratedAmount > 0 ? "Prorated charge" : "Prorated credit"}: $${Math.abs(preview.proratedAmount / 100).toFixed(2)} ${preview.currency.toUpperCase()}`} | |
| {preview.proratedAmount === 0 | |
| ? "No prorated adjustment" | |
| : `${preview.proratedAmount > 0 ? "Prorated charge" : "Prorated credit"}: ${new Intl.NumberFormat(undefined, { style: "currency", currency: preview.currency, currencyDisplay: "code" }).format(Math.abs(preview.proratedAmount) / 100)}`} |
| .map((email) => email.trim().toLowerCase()) | ||
| .filter((email) => email !== ""); | ||
|
|
||
| if (inviteEmails.length + newEmails.length > remainingSeats) { | ||
| const invalidEmails = newEmails.filter((email) => !emailRegex.test(email)); | ||
| if (invalidEmails.length > 0) { | ||
| toast.error( | ||
| `Not enough seats available. You have ${remainingSeats} seats remaining.`, | ||
| `Invalid email${invalidEmails.length > 1 ? "s" : ""}: ${invalidEmails.join(", ")}`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| setInviteEmails([...new Set([...inviteEmails, ...newEmails])]); | ||
| const validEmails = newEmails.filter((email) => emailRegex.test(email)); | ||
| setInviteEmails([...new Set([...inviteEmails, ...validEmails])]); | ||
| setEmailInput(""); | ||
| }; |
There was a problem hiding this comment.
Email input cleared even when all emails are invalid
setEmailInput("") is called unconditionally at the end of handleAddEmails, even when every email the user typed was invalid. If a user blurs the field with only invalid addresses, the error toast fires but their text is also wiped — they must retype from scratch to correct it.
The fix is to only clear the input when at least one valid email was actually queued:
| .map((email) => email.trim().toLowerCase()) | |
| .filter((email) => email !== ""); | |
| if (inviteEmails.length + newEmails.length > remainingSeats) { | |
| const invalidEmails = newEmails.filter((email) => !emailRegex.test(email)); | |
| if (invalidEmails.length > 0) { | |
| toast.error( | |
| `Not enough seats available. You have ${remainingSeats} seats remaining.`, | |
| `Invalid email${invalidEmails.length > 1 ? "s" : ""}: ${invalidEmails.join(", ")}`, | |
| ); | |
| return; | |
| } | |
| setInviteEmails([...new Set([...inviteEmails, ...newEmails])]); | |
| const validEmails = newEmails.filter((email) => emailRegex.test(email)); | |
| setInviteEmails([...new Set([...inviteEmails, ...validEmails])]); | |
| setEmailInput(""); | |
| }; | |
| const handleAddEmails = () => { | |
| const newEmails = emailInput | |
| .split(",") | |
| .map((email) => email.trim().toLowerCase()) | |
| .filter((email) => email !== ""); | |
| const invalidEmails = newEmails.filter((email) => !emailRegex.test(email)); | |
| if (invalidEmails.length > 0) { | |
| toast.error( | |
| `Invalid email${invalidEmails.length > 1 ? "s" : ""}: ${invalidEmails.join(", ")}`, | |
| ); | |
| } | |
| const validEmails = newEmails.filter((email) => emailRegex.test(email)); | |
| if (validEmails.length > 0) { | |
| setInviteEmails([...new Set([...inviteEmails, ...validEmails])]); | |
| setEmailInput(""); | |
| } | |
| }; |
This preserves the typed text in the input so the user can fix their typos, while still surfacing the validation error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/InviteDialog.tsx
Line: 44-57
Comment:
**Email input cleared even when all emails are invalid**
`setEmailInput("")` is called unconditionally at the end of `handleAddEmails`, even when every email the user typed was invalid. If a user blurs the field with only invalid addresses, the error toast fires but their text is also wiped — they must retype from scratch to correct it.
The fix is to only clear the input when at least one valid email was actually queued:
```suggestion
const handleAddEmails = () => {
const newEmails = emailInput
.split(",")
.map((email) => email.trim().toLowerCase())
.filter((email) => email !== "");
const invalidEmails = newEmails.filter((email) => !emailRegex.test(email));
if (invalidEmails.length > 0) {
toast.error(
`Invalid email${invalidEmails.length > 1 ? "s" : ""}: ${invalidEmails.join(", ")}`,
);
}
const validEmails = newEmails.filter((email) => emailRegex.test(email));
if (validEmails.length > 0) {
setInviteEmails([...new Set([...inviteEmails, ...validEmails])]);
setEmailInput("");
}
};
```
This preserves the typed text in the input so the user can fix their typos, while still surfacing the validation error.
How can I resolve this? If you propose a fix, please make it concise.| .map((email) => email.trim().toLowerCase()) | ||
| .filter((email) => email !== ""); | ||
|
|
||
| if (inviteEmails.length + newEmails.length > remainingSeats) { | ||
| const invalidEmails = newEmails.filter((email) => !emailRegex.test(email)); | ||
| if (invalidEmails.length > 0) { | ||
| toast.error( | ||
| `Not enough seats available. You have ${remainingSeats} seats remaining.`, | ||
| `Invalid email${invalidEmails.length > 1 ? "s" : ""}: ${invalidEmails.join(", ")}`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| setInviteEmails([...new Set([...inviteEmails, ...newEmails])]); | ||
| const validEmails = newEmails.filter((email) => emailRegex.test(email)); | ||
| setInviteEmails([...new Set([...inviteEmails, ...validEmails])]); | ||
| setEmailInput(""); | ||
| }; |
There was a problem hiding this comment.
Ambiguous feedback when mixing valid and invalid emails
When a user types "valid@email.com, bad-email" and triggers handleAddEmails, two things happen simultaneously:
- An error toast fires — "Invalid email: bad-email"
valid@email.comis silently queued in the invite list and the input is cleared
The user sees an error and may believe the entire operation failed, not realising that valid@email.com was actually added. Consider adding a distinct success toast for the partial case:
| .map((email) => email.trim().toLowerCase()) | |
| .filter((email) => email !== ""); | |
| if (inviteEmails.length + newEmails.length > remainingSeats) { | |
| const invalidEmails = newEmails.filter((email) => !emailRegex.test(email)); | |
| if (invalidEmails.length > 0) { | |
| toast.error( | |
| `Not enough seats available. You have ${remainingSeats} seats remaining.`, | |
| `Invalid email${invalidEmails.length > 1 ? "s" : ""}: ${invalidEmails.join(", ")}`, | |
| ); | |
| return; | |
| } | |
| setInviteEmails([...new Set([...inviteEmails, ...newEmails])]); | |
| const validEmails = newEmails.filter((email) => emailRegex.test(email)); | |
| setInviteEmails([...new Set([...inviteEmails, ...validEmails])]); | |
| setEmailInput(""); | |
| }; | |
| const validEmails = newEmails.filter((email) => emailRegex.test(email)); | |
| if (validEmails.length > 0) { | |
| setInviteEmails([...new Set([...inviteEmails, ...validEmails])]); | |
| setEmailInput(""); | |
| if (invalidEmails.length > 0) { | |
| toast.success(`Added ${validEmails.length} email(s). See error above for invalid addresses.`); | |
| } | |
| } |
This makes it explicit that some emails were queued successfully despite the validation warning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/settings/organization/components/InviteDialog.tsx
Line: 44-57
Comment:
**Ambiguous feedback when mixing valid and invalid emails**
When a user types `"valid@email.com, bad-email"` and triggers `handleAddEmails`, two things happen simultaneously:
1. An error toast fires — *"Invalid email: bad-email"*
2. `valid@email.com` is silently queued in the invite list and the input is cleared
The user sees an error and may believe the entire operation failed, not realising that `valid@email.com` was actually added. Consider adding a distinct success toast for the partial case:
```suggestion
const validEmails = newEmails.filter((email) => emailRegex.test(email));
if (validEmails.length > 0) {
setInviteEmails([...new Set([...inviteEmails, ...validEmails])]);
setEmailInput("");
if (invalidEmails.length > 0) {
toast.success(`Added ${validEmails.length} email(s). See error above for invalid addresses.`);
}
}
```
This makes it explicit that some emails were queued successfully despite the validation warning.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
hasProSeatcolumn) with toggle in the members table, replacing the old seat-count-based invite systemGreptile Summary
This PR replaces the old seat-count-based invite system with per-member Pro seat assignment (
hasProSeat), restructures organization settings into tabbed sub-pages with a shared server-side auth guard layout, adds Stripe-integrated seat quantity management with prorated billing previews, and rewrites invite sending/acceptance flows to use transactions with proper deduplication and concurrency safety.Key changes:
organization_members.hasProSeatcolumn added via migration; toggle action usesFOR UPDATElocking on the full member list to prevent over-assignment race conditionspreviewSeatChange/updateSeatQuantitycorrectly isolate proration cost from Stripe line items (notamount_due); DB update wrapped in try/catch to surface post-Stripe sync failuresactiveOrganizationIdlayout.tsx(server auth guard) +GeneralPage,PreferencesPage,BillingAndMembersPagesub-routesInviteDialogvalidates email format client-side with two UX regressions: email input is unconditionally cleared even when all typed emails were invalid (user loses their text and must retype to correct), and when mixing valid and invalid emails, valid ones are silently queued while only an error toast fires (partial-success state is ambiguous)MemberAvatarssidebar component correctly gates invite-slot buttons behind an ownership checkSettingsNavtab label conditionally renders "Billing & Members" vs "Members" based onNEXT_PUBLIC_IS_CAPConfidence Score: 4/5
Last reviewed commit: 4610f9c