fix: cross-cutting SDK fixes#1577
Open
rohanchkrabrty wants to merge 8 commits intomainfrom
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughLarge-scale update refactoring billing access control, permissions integration, provider configuration, and table layouts. Introduces permission-based billing access, replaces billing details dialog with checkout flow, adds conditional theme/toast providers, implements service user token caching, standardizes table layouts across views, and updates imports to use path aliases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Coverage Report for CI Build 25047222899Coverage remained the same at 42.353%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/sdk/react/views-new/tokens/tokens-view.tsx (1)
82-90:⚠️ Potential issue | 🟠 MajorAvoid indefinite loading when there is no active organization.
With the new
!activeOrganization?.idclause, the view can stay in loading mode forever after org resolution completes but no org exists, which keeps skeletons and disables actions indefinitely.💡 Proposed fix
- const isLoading = - !activeOrganization?.id || - isActiveOrganizationLoading || - isBillingAccountLoading || - isTokensLoading || - isPermissionsFetching; + const hasActiveOrganization = Boolean(activeOrganization?.id); + const isLoading = + isActiveOrganizationLoading || + (hasActiveOrganization && + (isBillingAccountLoading || isTokensLoading || isPermissionsFetching)); - const isTxnDataLoading = isLoading || isTransactionsListLoading; + const isTxnDataLoading = + isLoading || (hasActiveOrganization && isTransactionsListLoading);web/sdk/react/views-new/billing/components/payment-method-card.tsx (1)
116-126:⚠️ Potential issue | 🟡 MinorDon't show the auth tooltip while the action is loading.
Inside this branch the user is already allowed, so
isBtnDisabledonly reflects the pending mutation. Wrapping the loading button withAuthTooltipMessagetells authorized users they lack access immediately after clicking.Proposed fix
- {!isLoading && isAllowed ? ( - isBtnDisabled ? ( - <Tooltip> - <Tooltip.Trigger render={<span />}> - {actionBtn} - </Tooltip.Trigger> - <Tooltip.Content> - {AuthTooltipMessage} - </Tooltip.Content> - </Tooltip> - ) : actionBtn - ) : null} + {!isLoading && isAllowed ? actionBtn : null}web/sdk/react/views-new/billing/components/invoices.tsx (1)
195-222:⚠️ Potential issue | 🟠 MajorKeep incremental pagination out of the table loading flag.
Line 195 treats
isFetchingNextPageas a full-table load. WithonLoadMorewired up, every pagination request can push the table back into its loading state and hide already rendered invoices instead of appending smoothly.Proposed fix
- const loading = (!activeOrganization?.id || isLoading || isFetchingNextPage) && !isError; + const loading = (!activeOrganization?.id || isLoading) && !isError;
🧹 Nitpick comments (2)
web/sdk/react/views-new/service-accounts/components/projects-cell.module.css (1)
9-11: Prevent tooltip overflow on long unbroken names.
max-widthalone may still overflow when a project name has no natural breakpoints. Add wrapping behavior for safer rendering.Suggested patch
.tooltipContent { max-width: 550px; + overflow-wrap: anywhere; }web/sdk/react/views-new/service-accounts/hooks/useServiceUserTokens.ts (1)
123-129: Consider exposing query error state.The hook currently only exposes
isLoading, but the underlyinguseQueryalso provideserrorandisError. Consumers might benefit from being able to display error states when the token fetch fails.💡 Optional enhancement to expose error state
- const { data: tokensData, isLoading } = useQuery( + const { data: tokensData, isLoading, error, isError } = useQuery( FrontierServiceQueries.listServiceUserTokens, ... ); return { tokens, isLoading, + error, + isError, addToken, removeToken, clearFreshTokens };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a572a24e-1ee7-4664-8240-6126253bc949
📒 Files selected for processing (51)
web/apps/client-demo/src/pages/settings/Billing.tsxweb/sdk/react/components/view-container/view-container.module.cssweb/sdk/react/contexts/FrontierProvider.tsxweb/sdk/react/hooks/useBillingPermission.tsweb/sdk/react/views-new/billing/billing-view.module.cssweb/sdk/react/views-new/billing/billing-view.tsxweb/sdk/react/views-new/billing/components/billing-details-card.tsxweb/sdk/react/views-new/billing/components/billing-details-dialog.tsxweb/sdk/react/views-new/billing/components/confirm-cycle-switch-dialog.tsxweb/sdk/react/views-new/billing/components/invoices.tsxweb/sdk/react/views-new/billing/components/payment-issue.tsxweb/sdk/react/views-new/billing/components/payment-method-card.tsxweb/sdk/react/views-new/billing/components/upcoming-billing-cycle.tsxweb/sdk/react/views-new/billing/components/upcoming-plan-change-banner.tsxweb/sdk/react/views-new/members/components/member-columns.tsxweb/sdk/react/views-new/members/members-view.module.cssweb/sdk/react/views-new/members/members-view.tsxweb/sdk/react/views-new/plans/components/confirm-plan-change-dialog.tsxweb/sdk/react/views-new/plans/components/feature-table.tsxweb/sdk/react/views-new/plans/components/plan-card.module.cssweb/sdk/react/views-new/plans/components/plan-card.tsxweb/sdk/react/views-new/plans/plans-view.tsxweb/sdk/react/views-new/preferences/preferences-view.tsxweb/sdk/react/views-new/projects/components/member-columns.tsxweb/sdk/react/views-new/projects/components/project-columns.tsxweb/sdk/react/views-new/projects/project-details-view.module.cssweb/sdk/react/views-new/projects/project-details-view.tsxweb/sdk/react/views-new/projects/projects-view.module.cssweb/sdk/react/views-new/projects/projects-view.tsxweb/sdk/react/views-new/service-accounts/components/add-service-account-dialog.tsxweb/sdk/react/views-new/service-accounts/components/add-token-form.tsxweb/sdk/react/views-new/service-accounts/components/delete-service-account-dialog.module.cssweb/sdk/react/views-new/service-accounts/components/delete-service-account-dialog.tsxweb/sdk/react/views-new/service-accounts/components/manage-project-access-dialog.tsxweb/sdk/react/views-new/service-accounts/components/projects-cell.module.cssweb/sdk/react/views-new/service-accounts/components/projects-cell.tsxweb/sdk/react/views-new/service-accounts/components/revoke-token-dialog.module.cssweb/sdk/react/views-new/service-accounts/components/revoke-token-dialog.tsxweb/sdk/react/views-new/service-accounts/components/service-account-columns.module.cssweb/sdk/react/views-new/service-accounts/components/service-account-columns.tsxweb/sdk/react/views-new/service-accounts/hooks/useServiceUserTokens.tsweb/sdk/react/views-new/service-accounts/service-account-details-view.module.cssweb/sdk/react/views-new/service-accounts/service-account-details-view.tsxweb/sdk/react/views-new/service-accounts/service-accounts-view.module.cssweb/sdk/react/views-new/service-accounts/service-accounts-view.tsxweb/sdk/react/views-new/teams/components/member-columns.tsxweb/sdk/react/views-new/teams/team-details-view.module.cssweb/sdk/react/views-new/teams/team-details-view.tsxweb/sdk/react/views-new/tokens/tokens-view.tsxweb/sdk/shared/types.tsweb/sdk/tsconfig.json
💤 Files with no reviewable changes (3)
- web/sdk/react/views-new/plans/components/confirm-plan-change-dialog.tsx
- web/sdk/react/views-new/preferences/preferences-view.tsx
- web/sdk/react/views-new/billing/components/billing-details-dialog.tsx
Comment on lines
+5
to
+7
| const { orgId } = useParams<{ orgId: string }>(); | ||
| const navigate = useNavigate(); | ||
| return <BillingView onNavigateToPlans={() => navigate(`/${orgId}/settings/plans`)} />; |
Contributor
There was a problem hiding this comment.
Guard missing orgId before navigation.
If orgId is absent, this can route to an invalid path (/undefined/settings/plans).
Suggested fix
export default function Billing() {
const { orgId } = useParams<{ orgId: string }>();
const navigate = useNavigate();
- return <BillingView onNavigateToPlans={() => navigate(`/${orgId}/settings/plans`)} />;
+ return (
+ <BillingView
+ onNavigateToPlans={() => {
+ if (!orgId) return;
+ navigate(`/${orgId}/settings/plans`);
+ }}
+ />
+ );
}📝 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 { orgId } = useParams<{ orgId: string }>(); | |
| const navigate = useNavigate(); | |
| return <BillingView onNavigateToPlans={() => navigate(`/${orgId}/settings/plans`)} />; | |
| const { orgId } = useParams<{ orgId: string }>(); | |
| const navigate = useNavigate(); | |
| return ( | |
| <BillingView | |
| onNavigateToPlans={() => { | |
| if (!orgId) return; | |
| navigate(`/${orgId}/settings/plans`); | |
| }} | |
| /> | |
| ); |
Comment on lines
+114
to
+157
| const handleBillingDetailsUpdateClick = useCallback(async () => { | ||
| const orgId = activeOrganization?.id || ''; | ||
| if (!orgId) return; | ||
|
|
||
| try { | ||
| const query = qs.stringify( | ||
| { | ||
| details: btoa( | ||
| qs.stringify({ | ||
| organization_id: orgId, | ||
| type: 'billing' | ||
| }) | ||
| ), | ||
| checkout_id: '{{.CheckoutID}}' | ||
| }, | ||
| { encode: false } | ||
| ); | ||
| const cancel_url = `${config?.billing?.cancelUrl}?${query}`; | ||
| const success_url = `${config?.billing?.successUrl}?${query}`; | ||
|
|
||
| const resp = await createCheckoutMutation( | ||
| create(CreateCheckoutRequestSchema, { | ||
| orgId, | ||
| cancelUrl: cancel_url, | ||
| successUrl: success_url, | ||
| setupBody: { | ||
| paymentMethod: false, | ||
| customerPortal: true | ||
| } | ||
| }) | ||
| ); | ||
| const checkoutUrl = resp?.checkoutSession?.checkoutUrl; | ||
| if (checkoutUrl) { | ||
| window.location.href = checkoutUrl; | ||
| } | ||
| } catch (err) { | ||
| console.error(err); | ||
| } | ||
| }, [ | ||
| activeOrganization?.id, | ||
| createCheckoutMutation, | ||
| config?.billing?.cancelUrl, | ||
| config?.billing?.successUrl | ||
| ]); |
Contributor
There was a problem hiding this comment.
Guard the checkout flow when billing redirect URLs are missing.
Lines 131-138 build cancelUrl and successUrl even when the billing config is absent. That turns into undefined?... URLs and sends a broken checkout request at runtime. Bail out early, or disable the action, until both URLs are configured.
Proposed fix
const handleBillingDetailsUpdateClick = useCallback(async () => {
const orgId = activeOrganization?.id || '';
- if (!orgId) return;
+ const cancelUrl = config?.billing?.cancelUrl;
+ const successUrl = config?.billing?.successUrl;
+ if (!orgId || !cancelUrl || !successUrl) {
+ toastManager.add({
+ title: 'Billing is not configured',
+ description: 'Missing billing redirect URLs.',
+ type: 'error'
+ });
+ return;
+ }
try {
const query = qs.stringify(
{
@@
},
{ encode: false }
);
- const cancel_url = `${config?.billing?.cancelUrl}?${query}`;
- const success_url = `${config?.billing?.successUrl}?${query}`;
+ const cancel_url = `${cancelUrl}?${query}`;
+ const success_url = `${successUrl}?${query}`;| const btnText = | ||
| billingAccount?.email || billingAccount?.name ? 'Update' : 'Add details'; | ||
| const isButtonDisabled = isLoading || disabled; | ||
| const isButtonDisabled = isLoading || disabled || isActionLoading; |
Contributor
There was a problem hiding this comment.
Separate “action in progress” from “restricted” disable state.
isActionLoading is currently treated the same as permission/support-restricted disable state, which can show the “Contact support…” tooltip while the request is simply in progress.
Suggested fix
- const isButtonDisabled = isLoading || disabled || isActionLoading;
+ const isRestricted = isLoading || disabled;
+ const isButtonDisabled = isRestricted || isActionLoading;
...
- <Tooltip.Trigger
- disabled={!isButtonDisabled}
+ <Tooltip.Trigger
+ disabled={!isRestricted}
render={<span />}
>
...
- {isButtonDisabled && (
+ {isRestricted && (
<Tooltip.Content>
Contact support to update your billing address.
</Tooltip.Content>
)}Also applies to: 49-50
Comment on lines
+103
to
+120
| const selectedIntervals = useMemo<Record<string, IntervalKeys>>(() => { | ||
| const result: Record<string, IntervalKeys> = {}; | ||
| groupedPlans.forEach(plan => { | ||
| if (selectedIntervals[plan.slug]) return; | ||
| const planIntervals = Object.values(plan.intervals) | ||
| if (intervalOverrides[plan.slug]) { | ||
| result[plan.slug] = intervalOverrides[plan.slug]; | ||
| return; | ||
| } | ||
| const sortedIntervals = Object.values(plan.intervals) | ||
| .sort((a, b) => a.weightage - b.weightage) | ||
| .map(i => i.interval); | ||
| const activePlanInterval = Object.values(plan.intervals).find( | ||
| p => p.planId === activeSubscription?.planId | ||
| ); | ||
| defaults[plan.slug] = | ||
| activePlanInterval?.interval || planIntervals[0] || 'year'; | ||
| result[plan.slug] = | ||
| activePlanInterval?.interval || sortedIntervals[0] || 'year'; | ||
| }); | ||
|
|
||
| if (Object.keys(defaults).length > 0) { | ||
| setSelectedIntervals(prev => ({ ...prev, ...defaults })); | ||
| } | ||
| }, [groupedPlans, activeSubscription?.planId]); | ||
| return result; | ||
| }, [groupedPlans, activeSubscription?.planId, intervalOverrides]); |
Contributor
There was a problem hiding this comment.
Validate saved interval overrides before reusing them.
Line 106 re-applies any cached override blindly. After an org switch or a plans refresh, that override can point to an interval the current plan.intervals map no longer has, which leaves selectedIntervals[plan.slug] invalid and can desync the card/table state. Fall back to the derived default when the override is no longer available.
Proposed fix
const selectedIntervals = useMemo<Record<string, IntervalKeys>>(() => {
const result: Record<string, IntervalKeys> = {};
groupedPlans.forEach(plan => {
- if (intervalOverrides[plan.slug]) {
- result[plan.slug] = intervalOverrides[plan.slug];
+ const override = intervalOverrides[plan.slug];
+ if (override && plan.intervals[override]) {
+ result[plan.slug] = override;
return;
}
const sortedIntervals = Object.values(plan.intervals)
.sort((a, b) => a.weightage - b.weightage)
.map(i => i.interval);
Comment on lines
+36
to
+38
| if (isLoading) { | ||
| return <Skeleton height="16px" width="200px" />; | ||
| } |
Contributor
There was a problem hiding this comment.
Match loading placeholder width with final text width.
At Line 37, the skeleton is 200px, while loaded text is constrained to 400px (styles.text). This can still cause load→loaded visual jitter in the column.
Suggested patch
- if (isLoading) {
- return <Skeleton height="16px" width="200px" />;
- }
+ if (isLoading) {
+ return <Skeleton height="16px" width="400px" />;
+ }📝 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
| if (isLoading) { | |
| return <Skeleton height="16px" width="200px" />; | |
| } | |
| if (isLoading) { | |
| return <Skeleton height="16px" width="400px" />; | |
| } |
Comment on lines
+1
to
+7
| .actionsCell { | ||
| visibility: hidden; | ||
| } | ||
|
|
||
| tr:hover .actionsCell { | ||
| visibility: visible; | ||
| } |
Contributor
There was a problem hiding this comment.
Hover-only action reveal makes row actions inaccessible for keyboard/touch users.
With visibility: hidden by default and reveal only on tr:hover, keyboard users cannot reliably discover/activate the action button, and touch devices may never show it. This is an accessibility blocker.
Suggested fix
.actionsCell {
visibility: hidden;
}
-tr:hover .actionsCell {
+tr:hover .actionsCell,
+tr:focus-within .actionsCell,
+.actionsCell:focus-within {
visibility: visible;
}
+
+@media (hover: none) {
+ .actionsCell {
+ visibility: visible;
+ }
+}
rohanchkrabrty
changed the title
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
table-layout: fixedand explicit header widths matching the service-accounts pattern).useServiceUserTokens), and column/dialog styling.