234 payment edge cases#235
Conversation
…a handling, now handle grants properly, improve member adding
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | a85110f | Commit Preview URL | Jan 06 2026, 06:29 PM |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR expands UI component export surfaces with primitive variants across all components, implements end-to-end billing plan change validation with quota checks, adds collaborator quota enforcement to member management, introduces a payment issue alert UI component, implements localStorage-based subscription caching, and wires validation and quota checks through backend routes and frontend flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Web as Web UI
participant API as Backend API
participant DB as Database
rect rgb(200, 220, 255)
note over User,DB: Plan Change Validation Flow
User->>Web: Select target plan
Web->>API: GET /api/billing/validate-plan-change?targetPlan=X
API->>DB: Query org resource usage (projects, collaborators)
DB-->>API: Usage counts
API->>DB: Query target plan quotas
DB-->>API: Target plan metadata & limits
alt Validation Passes
API-->>Web: { valid: true }
Web->>Web: Show plan confirmation dialog
else Violations Exist
API-->>Web: { violations: [...] }
Web->>Web: Show validation error dialog with violations
end
end
rect rgb(220, 255, 220)
note over User,DB: Checkout with Downgrade Validation
User->>Web: Confirm plan change
Web->>API: POST /api/billing/checkout {tier: X}
API->>DB: Call validatePlanChange(orgId, tier)
DB-->>API: Validation result
alt Downgrade with Violations
API-->>Web: 400 error {violations: [...]}
Web->>Web: Block checkout, show error
else Valid (upgrade or within limits)
API->>API: Proceed to Stripe checkout
API-->>Web: Stripe session URL
Web->>User: Redirect to Stripe
end
end
sequenceDiagram
actor User
participant Web as Web UI
participant API as Backend API
participant DB as Database
rect rgb(255, 240, 200)
note over User,DB: Member Addition with Quota Check
User->>Web: Open Add Member modal
Web->>Web: Compute collaboratorQuotaInfo from subscription
Web->>Web: canAddMember = !isAtQuota && isOwner
alt At Quota Limit
Web->>User: Show quota warning banner, disable input
else Below Limit
User->>Web: Search & select user to add
Web->>API: POST /api/orgs/:id/members {userId: X}
API->>API: checkCollaboratorQuota(orgId)
API->>DB: Count current collaborators
DB-->>API: Current count vs quota limit
alt Quota Exceeded
API-->>Web: 403 AUTH_FORBIDDEN {quotaExceeded: true}
Web->>User: Show quota error
else Within Limit
API->>DB: Create org/project membership
DB-->>API: Success
API-->>Web: 201 Created {member: {...}}
Web->>Web: Refresh member list, close modal
end
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related Issues
Possibly Related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workers/src/routes/admin/orgs.js (1)
28-30: UsevalidateQueryParamsmiddleware and define schema for query parameter validation.Lines 28–30 manually parse query parameters without validation middleware. Per coding guidelines, all query parameters must be validated using
validateQueryParamsmiddleware with a defined Zod schema. This route lacksorgSchemasin the validation config.Add to
packages/workers/src/config/validation.js:export const orgSchemas = { list: z.object({ page: z .string() .optional() .default('1') .transform(val => parseInt(val, 10)) .pipe(z.number().int('Page must be an integer').min(1, 'Page must be at least 1')), limit: z .string() .optional() .default('20') .transform(val => parseInt(val, 10)) .pipe( z .number() .int('Limit must be an integer') .min(1, 'Limit must be at least 1') .max(100, 'Limit must be at most 100'), ), search: z .string() .optional() .transform(val => val?.trim()), }), };Then import and apply the middleware to the route, and refactor the handler to extract validated params from
c.get('validatedQuery'). Seedatabase.jsandstorage.jsfor the implementation pattern.
🤖 Fix all issues with AI Agents
In @packages/ui/src/components/NumberInput.tsx:
- Around line 134-136: The module currently shadows the Ark UI primitive by
re-exporting NumberInputComponent as NumberInput, so the later export "export {
NumberInput as NumberInputPrimitive }" exports the component instead of the
original primitive; fix by importing the Ark primitive under an alias (e.g.,
import { NumberInput as NumberInputPrimitiveFromArk } from '...'), change the
raw export to "export { NumberInputPrimitiveFromArk as NumberInputPrimitive }",
and update any internal references that intended to use the original Ark
primitive to use the new alias (ensure NumberInputComponent remains the
component and NumberInputPrimitiveFromArk is the raw primitive).
- Line 119: The input element in NumberInput uses the incorrect Tailwind
selector `data-invalid:` which doesn't match Ark UI's attribute format; update
the class string in the NumberInput component (look for the element building the
class with sizes().input and showControls()) to replace
`data-invalid:border-red-500 data-invalid:ring-red-500` with
`data-[invalid]:border-red-500 data-[invalid]:ring-red-500` so the invalid state
styles are applied consistently like other Ark UI components.
In @packages/ui/src/components/TagsInput.tsx:
- Around line 124-126: The export is wrong because the local alias TagsInput now
points to TagsInputComponent, so export { TagsInput as TagsInputPrimitive }
re-exports the component instead of the Ark UI primitive; fix by importing the
Ark primitive under a distinct name (e.g., import { TagsInput as ArkTagsInput }
from '...') and then update the component to use ArkTagsInput where the
primitive was referenced, leaving the final export as export { ArkTagsInput as
TagsInputPrimitive } while keeping TagsInputComponent exported as TagsInput.
🧹 Nitpick comments (13)
packages/ui/src/components/PasswordInput.tsx (1)
80-82: Primitive export is correct but consider aliasing the import for clarity.The export aligns with the PR objective to expose raw Ark UI primitives. However, having both the import and export share the
PasswordInputname can confuse future maintainers about which binding is being referenced.Consider the clearer aliasing pattern used in Drawer.tsx
-import { PasswordInput } from '@ark-ui/solid/password-input'; +import { PasswordInput as ArkPasswordInput } from '@ark-ui/solid/password-input';Then update the component to use
ArkPasswordInput.*and export:-export { PasswordInput as PasswordInputPrimitive }; +export { ArkPasswordInput as PasswordInputPrimitive };This makes it immediately clear that
PasswordInputPrimitiverefers to the Ark UI import rather than the wrapped component.packages/ui/src/components/Clipboard.tsx (1)
234-236: Primitive export is correct but consider aliasing the import for clarity.The export aligns with the PR objective to expose raw Ark UI primitives. Similar to PasswordInput.tsx, consider adopting the aliasing pattern demonstrated in Drawer.tsx (
import { Clipboard as ArkClipboard }) to make it immediately clear which bindingClipboardPrimitivereferences.packages/ui/src/components/PinInput.tsx (1)
95-97: Primitive export is correct but consider aliasing the import for clarity.The export aligns with the PR objective to expose raw Ark UI primitives. Consider adopting the aliasing pattern from Drawer.tsx (
import { PinInput as ArkPinInput }) to improve maintainability and make the export intent more explicit.packages/ui/src/components/Menu.tsx (1)
166-168: Primitive export is correct but consider aliasing the import for clarity.The export aligns with the PR objective to expose raw Ark UI primitives. Like the other components in this PR, consider adopting the aliasing pattern from Drawer.tsx (
import { Menu as ArkMenu }) for improved code clarity.packages/ui/src/components/index.ts (1)
5-38: All new Primitive exports and hooks exist in their source files.Verification confirms that all exported identifiers in lines 5-38 are properly defined in their respective component files. No missing exports will cause build failures.
There is a minor inconsistency in export order: lines 9 and 24 place hooks before primitives (
useCollapsible, CollapsiblePrimitiveanduseSelect, SelectPrimitive), while other lines consistently place primitives before hooks. Consider standardizing to place primitives before hooks for improved consistency:-export { default as Collapsible, useCollapsible, CollapsiblePrimitive } from './Collapsible'; +export { default as Collapsible, CollapsiblePrimitive, useCollapsible } from './Collapsible';-export { default as Select, useSelect, SelectPrimitive } from './Select'; +export { default as Select, SelectPrimitive, useSelect } from './Select';packages/web/src/components/project/overview-tab/OverviewTab.jsx (1)
51-60: Consider handling loading state for quotas.When
quotas()is undefined (loading),maxdefaults to0, which could incorrectly show "quota reached" before data loads. Consider either:
- Adding a loading check before evaluating quota
- Defaulting to a permissive value during loading
Possible approach to handle loading state
const collaboratorQuotaInfo = createMemo(() => { - const max = quotas()?.['collaborators.org.max'] ?? 0; + const quotaMax = quotas()?.['collaborators.org.max']; + // Return null during loading to avoid false "quota reached" state + if (quotaMax === undefined) return null; const used = orgMemberCount(); - return { used, max }; + return { used, max: quotaMax }; }); const canAddMember = createMemo(() => { if (!isOwner()) return false; + // Allow during loading + if (!quotas()) return true; return hasQuota('collaborators.org.max', { used: orgMemberCount(), requested: 1 }); });Then update the modal and button to handle
nullquotaInfo gracefully.packages/web/src/lib/entitlements.js (1)
11-11: Consider importing GRANT_TYPES from@corates/shared/plansto avoid duplication.The
GRANT_TYPESarray duplicates knowledge that already exists in the shared plans module (wheregetGrantPlanhandles'trial'and'single_project'). If a new grant type is added to the backend, this frontend constant could fall out of sync.Suggested approach
Export
GRANT_TYPESfrom@corates/shared/plansand import it here, or derive the check from the existence of a grant plan:-const GRANT_TYPES = ['trial', 'single_project']; +import { getPlan, DEFAULT_PLAN, isUnlimitedQuota, getGrantPlan, GRANT_TYPES } from '@corates/shared/plans';Alternatively, you could make
resolvePlantrygetGrantPlanfirst and fall back togetPlanif the result equals the default plan, though that may have performance implications.packages/workers/src/lib/billingResolver.js (1)
258-274: Consider wrapping DB operations in try-catch for resilience.The
getOrgResourceUsagefunction performs two database queries but lacks error handling. If either query fails, the destructuring assignment (const [projectResult] = ...) could throw an unexpected error.Per coding guidelines, database operations should be wrapped in try-catch blocks with domain errors.
Suggested approach
export async function getOrgResourceUsage(db, orgId) { + try { // Count projects in org const [projectResult] = await db .select({ count: count() }) .from(projects) .where(eq(projects.orgId, orgId)); // Count non-owner members (owner doesn't count toward collaborator limit) const [memberResult] = await db .select({ count: count() }) .from(member) .where(and(eq(member.organizationId, orgId), ne(member.role, 'owner'))); return { projects: projectResult?.count || 0, collaborators: memberResult?.count || 0, }; + } catch (error) { + console.error('[BillingResolver] Error getting org resource usage:', error); + throw error; + } }packages/workers/src/lib/__tests__/billingResolver.test.js (1)
511-566: Tests rely on hardcoded plan quotas - consider documenting this dependency.These tests assume
starter_teamplan hasprojects.max: 3andcollaborators.org.max: 5. If the plan configuration changes, these tests will fail silently or produce misleading results.Consider adding a brief comment or using constants from the plans module to make the dependency explicit.
packages/web/src/components/billing/SubscriptionCard.jsx (1)
138-160: Consider extracting alert blocks into a reusable component.The
incomplete,unpaid, andpast_duealert blocks share similar structure (icon + title + description). A smallAlertBlockcomponent could reduce duplication.Optional refactor example
function AlertBlock(props) { const colorStyles = { red: 'border-red-200 bg-red-50 text-red-800 text-red-600 text-red-500', yellow: 'border-yellow-200 bg-yellow-50 text-yellow-800 text-yellow-600 text-yellow-500', amber: 'border-amber-200 bg-amber-50 text-amber-800 text-amber-600 text-amber-500', }; // ... implementation }This is optional since the current implementation is clear and readable.
packages/workers/src/routes/__tests__/members.test.js (1)
1526-1540: Add assertion to verify quota check was bypassed.The comment states the quota check won't be called, but there's no assertion to verify this. Adding an explicit check would make the test more robust and document the expected behavior.
Proposed fix
// Should succeed because user-2 is already an org member // checkCollaboratorQuota should NOT have been called (existing org member) expect(res.status).toBe(201); const body = await json(res); expect(body.userId).toBe('user-2'); + expect(mockCheckCollaboratorQuota).not.toHaveBeenCalled();packages/web/src/components/billing/PricingTable.jsx (1)
440-449: Display friendly plan name instead of raw tier ID.The downgrade confirmation shows
currentTier()which returns the raw tier ID (e.g., "starter_team") rather than a user-friendly name. Consider using the plan's display name for better UX.Proposed fix
+ const getCurrentPlanName = () => { + const current = plans.find(p => p.tier === currentTier()); + return current?.name || currentTier(); + }; + <Show when={pendingDowngrade()}> <div class='mb-6 rounded-lg border border-amber-200 bg-amber-50 p-4'> <p class='text-sm text-amber-800'> - You're switching from <span class='font-semibold'>{currentTier()}</span> to{' '} + You're switching from <span class='font-semibold'>{getCurrentPlanName()}</span> to{' '} <span class='font-semibold'>{pendingDowngrade()?.name}</span>. </p>packages/workers/src/routes/billing/index.js (1)
171-218: Consider usingvalidateQueryParamsmiddleware for query string validation.Per coding guidelines, query string validation should use the
validateQueryParamsmiddleware pattern. To implement this, add abillingSchemasobject topackages/workers/src/config/validation.jswith a schema fortargetPlan, then apply the middleware to the route. This would replace the manual validation at lines 176-184 with schema-based validation usingc.get('validatedQuery').The current manual validation is functionally correct and properly uses
createDomainError, but middleware-based validation is the preferred pattern for consistency across the codebase.
| <NumberInput.Input | ||
| placeholder={local.placeholder} | ||
| class={`${sizes().input} flex-1 border border-gray-300 text-center ${showControls() ? '' : 'rounded-lg'} read-only:bg-gray-50 focus:border-blue-500 focus:ring-2 focus:ring-blue-500 focus:outline-none disabled:cursor-not-allowed disabled:bg-gray-100 data-[invalid]:border-red-500 data-[invalid]:ring-red-500 ${local.inputClass || ''}`} | ||
| class={`${sizes().input} flex-1 border border-gray-300 text-center ${showControls() ? '' : 'rounded-lg'} read-only:bg-gray-50 focus:border-blue-500 focus:ring-2 focus:ring-blue-500 focus:outline-none disabled:cursor-not-allowed disabled:bg-gray-100 data-invalid:border-red-500 data-invalid:ring-red-500 ${local.inputClass || ''}`} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for data attribute usages in UI components
rg -n "data-\[invalid\]|data-invalid|data-\[" packages/ui/src/components/ -A 2 -B 2Repository: InfinityBowman/corates
Length of output: 11827
🏁 Script executed:
# Also check the Ark UI dependency version in package.json
rg -n "@ark-ui|@headlessui" packages/ui/ | head -20Repository: InfinityBowman/corates
Length of output: 2046
🏁 Script executed:
# Look for NumberInput component usage and related configs
find packages/ui -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "NumberInput" | head -10Repository: InfinityBowman/corates
Length of output: 144
Fix invalid state selector in NumberInput input element.
The selector data-invalid: will not match Ark UI NumberInput's actual data attributes. All other Ark UI components in the codebase consistently use data-[invalid]: with square brackets. Change line 119 to use data-[invalid]:border-red-500 data-[invalid]:ring-red-500 instead of data-invalid:border-red-500 data-invalid:ring-red-500.
🤖 Prompt for AI Agents
In @packages/ui/src/components/NumberInput.tsx around line 119, The input
element in NumberInput uses the incorrect Tailwind selector `data-invalid:`
which doesn't match Ark UI's attribute format; update the class string in the
NumberInput component (look for the element building the class with
sizes().input and showControls()) to replace `data-invalid:border-red-500
data-invalid:ring-red-500` with `data-[invalid]:border-red-500
data-[invalid]:ring-red-500` so the invalid state styles are applied
consistently like other Ark UI components.
|
|
||
| // Export raw Ark UI primitive for custom layouts | ||
| export { NumberInput as NumberInputPrimitive }; |
There was a problem hiding this comment.
Critical: Wrong primitive exported due to name shadowing.
After line 133 exports NumberInputComponent as NumberInput, the identifier NumberInput in the module scope now refers to the component, not the Ark UI primitive imported on line 5. Therefore, line 136 exports the component again instead of the Ark UI primitive.
Proposed fix: Import Ark UI primitive with an alias
-import { NumberInput } from '@ark-ui/solid/number-input';
+import { NumberInput as ArkNumberInput } from '@ark-ui/solid/number-input';Then update all references in the component:
const NumberInputComponent: Component<NumberInputProps> = props => {
// ... component code using ArkNumberInput.Root, ArkNumberInput.Input, etc.
return (
- <NumberInput.Root
+ <ArkNumberInput.Root
// ... props
>
<Show when={local.label}>
- <NumberInput.Label class='mb-1 block text-sm font-medium text-gray-700'>
+ <ArkNumberInput.Label class='mb-1 block text-sm font-medium text-gray-700'>
{local.label}
- </NumberInput.Label>
+ </ArkNumberInput.Label>
</Show>
<div class='flex'>
<Show when={showControls()}>
- <NumberInput.DecrementTrigger
+ <ArkNumberInput.DecrementTrigger
// ... props
>
<FiMinus class='h-4 w-4' />
- </NumberInput.DecrementTrigger>
+ </ArkNumberInput.DecrementTrigger>
</Show>
- <NumberInput.Input
+ <ArkNumberInput.Input
// ... props
/>
<Show when={showControls()}>
- <NumberInput.IncrementTrigger
+ <ArkNumberInput.IncrementTrigger
// ... props
>
<FiPlus class='h-4 w-4' />
- </NumberInput.IncrementTrigger>
+ </ArkNumberInput.IncrementTrigger>
</Show>
</div>
- </NumberInput.Root>
+ </ArkNumberInput.Root>
);
};
export { NumberInputComponent as NumberInput };
-// Export raw Ark UI primitive for custom layouts
-export { NumberInput as NumberInputPrimitive };
+// Export raw Ark UI primitive for custom layouts
+export { ArkNumberInput as NumberInputPrimitive };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/ui/src/components/NumberInput.tsx around lines 134-136, The module
currently shadows the Ark UI primitive by re-exporting NumberInputComponent as
NumberInput, so the later export "export { NumberInput as NumberInputPrimitive
}" exports the component instead of the original primitive; fix by importing the
Ark primitive under an alias (e.g., import { NumberInput as
NumberInputPrimitiveFromArk } from '...'), change the raw export to "export {
NumberInputPrimitiveFromArk as NumberInputPrimitive }", and update any internal
references that intended to use the original Ark primitive to use the new alias
(ensure NumberInputComponent remains the component and
NumberInputPrimitiveFromArk is the raw primitive).
|
|
||
| // Export raw Ark UI primitive for custom layouts | ||
| export { TagsInput as TagsInputPrimitive }; |
There was a problem hiding this comment.
Critical: Wrong primitive exported due to name shadowing.
After line 123 exports TagsInputComponent as TagsInput, the identifier TagsInput now refers to the component, not the Ark UI primitive imported on line 5. Line 126 therefore exports the component again instead of the Ark UI primitive.
Proposed fix: Import Ark UI primitive with an alias
-import { TagsInput } from '@ark-ui/solid/tags-input';
+import { TagsInput as ArkTagsInput } from '@ark-ui/solid/tags-input';Then update all references in the component:
const TagsInputComponent: Component<TagsInputProps> = props => {
// ... component logic
return (
- <TagsInput.Root
+ <ArkTagsInput.Root
// ... props
>
- <TagsInput.Context>
+ <ArkTagsInput.Context>
{api => (
<>
{local.label && (
- <TagsInput.Label class='mb-1 block text-sm font-medium text-gray-700'>
+ <ArkTagsInput.Label class='mb-1 block text-sm font-medium text-gray-700'>
{local.label}
- </TagsInput.Label>
+ </ArkTagsInput.Label>
)}
- <TagsInput.Control class='...'>
+ <ArkTagsInput.Control class='...'>
<Index each={api().value}>
{(value, index) => (
- <TagsInput.Item index={index} value={value()} class='...'>
- <TagsInput.ItemPreview class='...'>
- <TagsInput.ItemText>{value()}</TagsInput.ItemText>
- <TagsInput.ItemDeleteTrigger class='...'>
+ <ArkTagsInput.Item index={index} value={value()} class='...'>
+ <ArkTagsInput.ItemPreview class='...'>
+ <ArkTagsInput.ItemText>{value()}</ArkTagsInput.ItemText>
+ <ArkTagsInput.ItemDeleteTrigger class='...'>
<FiX class='h-3 w-3' />
- </TagsInput.ItemDeleteTrigger>
- </TagsInput.ItemPreview>
- <TagsInput.ItemInput class='sr-only' />
- </TagsInput.Item>
+ </ArkTagsInput.ItemDeleteTrigger>
+ </ArkTagsInput.ItemPreview>
+ <ArkTagsInput.ItemInput class='sr-only' />
+ </ArkTagsInput.Item>
)}
</Index>
- <TagsInput.Input
+ <ArkTagsInput.Input
placeholder={local.placeholder || 'Add tag...'}
class={`...`}
/>
- </TagsInput.Control>
+ </ArkTagsInput.Control>
</>
)}
- </TagsInput.Context>
- <TagsInput.HiddenInput />
- </TagsInput.Root>
+ </ArkTagsInput.Context>
+ <ArkTagsInput.HiddenInput />
+ </ArkTagsInput.Root>
);
};
export { TagsInputComponent as TagsInput };
-// Export raw Ark UI primitive for custom layouts
-export { TagsInput as TagsInputPrimitive };
+// Export raw Ark UI primitive for custom layouts
+export { ArkTagsInput as TagsInputPrimitive };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/ui/src/components/TagsInput.tsx around lines 124-126, The export is
wrong because the local alias TagsInput now points to TagsInputComponent, so
export { TagsInput as TagsInputPrimitive } re-exports the component instead of
the Ark UI primitive; fix by importing the Ark primitive under a distinct name
(e.g., import { TagsInput as ArkTagsInput } from '...') and then update the
component to use ArkTagsInput where the primitive was referenced, leaving the
final export as export { ArkTagsInput as TagsInputPrimitive } while keeping
TagsInputComponent exported as TagsInput.
2 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.