Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a unified billing composable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PricingUI as PricingTableWorkspace
participant BillingAPI as Billing Service
participant PaymentUI as Payment Preview
participant Backend as Backend
User->>PricingUI: Select tier & billing cycle
PricingUI->>BillingAPI: previewSubscribe(plan_slug)
BillingAPI->>Backend: POST /billing/preview-subscribe
Backend-->>BillingAPI: PreviewSubscribeResponse
BillingAPI-->>PricingUI: preview data
PricingUI->>PaymentUI: Show preview
User->>PaymentUI: Confirm
PaymentUI->>BillingAPI: subscribe(plan_slug)
BillingAPI->>Backend: POST /billing/subscribe
Backend-->>BillingAPI: SubscribeResponse (status, billing_op_id, payment_url?)
BillingAPI-->>PaymentUI: subscribe result
alt needs payment method
PaymentUI->>User: Redirect to payment URL
else pending/awaiting
PaymentUI->>BillingAPI: poll getBillingStatus(billing_op_id)
BillingAPI->>Backend: GET /billing/ops/:id or /billing/status
Backend-->>BillingAPI: status -> active
BillingAPI-->>PaymentUI: active
end
PaymentUI->>User: Show success toast & close
sequenceDiagram
participant User
participant TopUpUI as TopUpDialog
participant BillingAPI as Billing Service
participant PollStore as BillingOperationStore
participant Backend as Backend
User->>TopUpUI: Enter amount & click Buy
TopUpUI->>BillingAPI: createTopup(amount_cents)
BillingAPI->>Backend: POST /billing/topup
Backend-->>BillingAPI: CreateTopupResponse {billing_op_id, topup_id, status}
BillingAPI-->>TopUpUI: Topup created
TopUpUI->>PollStore: startOperation(billing_op_id / topup_id, 'topup')
PollStore->>BillingAPI: getTopupStatus(topup_id)
BillingAPI->>Backend: GET /billing/topup/:id
Backend-->>BillingAPI: TopupStatus
alt completed
PollStore->>BillingAPI: getBillingBalance()
PollStore->>TopUpUI: close & show success toast
else pending
PollStore->>PollStore: wait & retry (exponential backoff)
else failed or timeout
PollStore->>TopUpUI: show error toast & cleanup
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright Tests: ⏳ Running...Tests started at 02/06/2026, 05:50:39 AM UTC 📊 Browser Tests
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/06/2026, 05:43:48 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.5 kB (baseline 22.5 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 840 kB (baseline 840 kB) • 🔴 +183 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69.6 kB (baseline 69 kB) • 🔴 +612 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 411 kB (baseline 410 kB) • 🔴 +349 BConfiguration panels, inspectors, and settings screens
Status: 24 added / 24 removed User & Accounts — 16.2 kB (baseline 16 kB) • 🔴 +160 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed Editors & Dialogs — 818 B (baseline 3.47 kB) • 🟢 -2.65 kBModals, dialogs, drawers, and in-app editors
Status: 1 added / 2 removed UI Components — 36.7 kB (baseline 37.8 kB) • 🟢 -1.14 kBReusable component library chunks
Status: 10 added / 10 removed Data & Services — 2.15 MB (baseline 2.1 MB) • 🔴 +51.3 kBStores, services, APIs, and repositories
Status: 12 added / 13 removed Utilities & Hooks — 235 kB (baseline 234 kB) • 🔴 +279 BHelpers, composables, and utility bundles
Status: 19 added / 19 removed Vendor & Third-Party — 9.37 MB (baseline 9.37 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 7 added / 7 removed Other — 7.14 MB (baseline 7.1 MB) • 🔴 +47.2 kBBundles that do not match a named category
Status: 124 added / 123 removed |
simula-r
added
the
preview-cpu
simula-r
force-pushed
the
feat/workspaces-6-billing
branch
from
f3086f2 to
fe13e86
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
simula-r
force-pushed
the
feat/workspaces-6-billing
branch
from
bed3cd5 to
6d9dc29
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
simula-r
force-pushed
the
feat/workspaces-6-billing
branch
from
4796688 to
ab70024
Compare
dosubot
bot
added
the
size:XXL
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
simula-r
added
needs-backport
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Update useBillingContext.test.ts to include new balance fields (prepaidBalanceMicros, cloudCreditBalanceMicros) - Add useBillingContext mock to useCoreCommands.test.ts - Add useBillingContext mock to CloudSubscriptionRedirectView.test.ts - Add useBillingContext mock to useSubscriptionActions.test.ts - Add useBillingContext mock to SubscriptionPanel.test.ts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…cription The useSubscription() return object was being assigned directly to isSubscriptionEnabled, causing the ternary to always evaluate as truthy (object truthiness) instead of the actual subscription state. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
simula-r
force-pushed
the
feat/workspaces-6-billing
branch
from
f33962c to
9e962ac
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/platform/cloud/subscription/composables/useSubscriptionActions.ts (1)
48-54:⚠️ Potential issue | 🟡 MinorInconsistent balance fetching: use
useBillingContext.fetchBalance()instead ofauthActions.fetchBalance()Both
fetchStatus()andfetchBalance()are intentionally called (confirmed by tests), as they fetch different data (subscription status vs. account balance). However,handleRefreshcallsauthActions.fetchBalance()directly while usinguseBillingContext().fetchStatus()for status. This creates an architectural inconsistency: when workspace billing is active,fetchStatus()respects the context butfetchBalance()always uses the Firebase/legacy path, potentially ignoring workspace-specific balance. Refactor to destructure and usefetchBalancefromuseBillingContext(likebillingOperationStore.tsdoes) to ensure both calls respect the active context.src/platform/workspace/stores/teamWorkspaceStore.ts (1)
606-609:⚠️ Potential issue | 🟡 MinorRemove or update the unused
subscribeWorkspacefunction—the TODO is stale and billing infrastructure is now fully implemented.The function is exported but never called anywhere in the codebase. Its console.warn body claims "Billing endpoint has not been added yet," which is outdated; workspace billing is fully implemented via
useWorkspaceBilling()and the/api/billing/subscribeendpoint. Either remove the unused export or integrate it with the existing billing infrastructure.
🤖 Fix all issues with AI agents
In `@src/components/dialog/content/TopUpCreditsDialogContentWorkspace.vue`:
- Around line 215-217: The helper function formatNumber currently hardcodes the
'en-US' locale; replace its usage with the i18n number formatter (n) from
useI18n to respect the app locale: remove or stop using formatNumber and update
template calls like formatNumber(usdToCredits(MIN_AMOUNT)) and
formatNumber(usdToCredits(MAX_AMOUNT)) to use n(usdToCredits(MIN_AMOUNT)) and
n(usdToCredits(MAX_AMOUNT)); if formatNumber is kept, refactor it to call n
internally (ensure useI18n's n is imported/available) and remove the hardcoded
toLocaleString('en-US') call.
In `@src/composables/billing/useBillingContext.ts`:
- Around line 159-174: The early-return guard in initialize() is ineffective
because the watcher force-sets isInitialized.value = false, allowing concurrent
calls to pass the guard; fix by introducing single-flight behavior: add an
initializingPromise (or isInitializing flag) scoped alongside
isInitialized/isLoading and inside initialize() if initializingPromise exists
return/await it, otherwise set initializingPromise =
activeContext.value.initialize() and await it, then clear initializingPromise
and set isInitialized.value = true; reference initialize(), isInitialized,
isLoading, activeContext.value.initialize to locate where to add the
promise/flag and where to clear it in the try/catch/finally.
- Around line 134-157: The async watch callback in useBillingContext (watch on
store.activeWorkspace?.id) can run multiple concurrent initialize() calls and
race on shared refs (isInitialized, isLoading, error); add a lightweight
generation token or abort counter in the composable (e.g.,
billingInitGeneration) that you increment before each call, capture into a local
variable inside the watch, and have initialize() or the post-await state
mutations check the captured token against the current billingInitGeneration so
only the latest invocation mutates refs; ensure errors are still assigned to
error ref only when the generation matches to avoid stale state from
earlier/faster completions.
In `@src/locales/en/main.json`:
- Around line 2192-2194: There is a duplicate "invoiceHistory" key inside the
subscription JSON object; remove the redundant "invoiceHistory" entry so only a
single definition of "invoiceHistory" remains in that object (keep the
intended/accurate translation), and run your locale sanity checks (or tests) to
ensure no other duplicates exist and that consumers still read the expected
value.
In `@src/platform/cloud/subscription/components/PricingTableWorkspace.vue`:
- Around line 439-441: The popover ref may be null when the <Popover> hasn't
rendered, so update the togglePopover handler to guard before calling
popover.value.toggle(event); e.g. check popover.value exists (or use optional
chaining popover.value?.toggle(event)) and keep the same signature for
togglePopover; reference the popover ref and the togglePopover function to
ensure the runtime null-check prevents calling .toggle() on undefined.
In
`@src/platform/cloud/subscription/components/SubscriptionPanelContentWorkspace.vue`:
- Around line 628-631: The onMounted fire-and-forget Promise.all([fetchStatus(),
fetchBalance()]) swallows errors; update the onMounted handler (where
fetchStatus and fetchBalance are invoked, alongside handleWindowFocus) to attach
a .catch that handles failures — e.g., set a component error state or call the
existing toast/error helper to surface the error and optionally retry; ensure
the .catch distinguishes which call failed if needed and preserves existing
focus listener cleanup.
In
`@src/platform/cloud/subscription/components/SubscriptionRequiredDialogContentWorkspace.vue`:
- Around line 291-314: The handler handleResubscribe uses hardcoded user strings
and calls workspaceApi.resubscribe() directly; replace the hardcoded 'Error' and
'Failed to resubscribe' with vue-i18n lookups (e.g. use t('common.error') for
the toast summary and a new/appropriate key such as
t('subscription.resubscribeFailed') for the detail) and update the toast.add
calls to use those keys, and verify/replace the direct API call with the billing
context method (e.g. use billingContext.subscribe or the equivalent resubscribe
helper) if the billing context supports resubscription; if it does not, leave
the resubscribe call but add a comment explaining why billingContext was not
used.
In `@src/platform/workspace/api/workspaceApi.ts`:
- Around line 594-614: The subscribe method is missing support for idempotency
keys; update async subscribe(planSlug: string, returnUrl?: string, cancelUrl?:
string) to accept an optional idempotencyKey?: string parameter and include
idempotency_key: idempotencyKey in the request payload (alongside plan_slug,
return_url, cancel_url) so the body satisfies SubscribeRequest; keep
getAuthHeaderOrThrow usage and error handling the same.
🧹 Nitpick comments (19)
src/components/dialog/content/subscription/CancelSubscriptionDialogContent.vue (2)
55-57: Use reactive prop destructuring per project conventions.The coding guidelines prescribe
const { prop1 } = defineProps<...>()style. SincecancelAtis only used in one place (line 67), destructuring keeps it consistent with the codebase convention.♻️ Suggested refactor
-const props = defineProps<{ - cancelAt?: string -}>() +const { cancelAt } = defineProps<{ + cancelAt?: string +}>()And update the usage on line 67:
- const dateStr = props.cancelAt ?? subscription.value?.endDate + const dateStr = cancelAt ?? subscription.value?.endDateAs per coding guidelines:
const { prop1, prop2 = default } = defineProps<{ prop1: Type; prop2?: Type }>()— use reactive prop destructuring, notwithDefaultsor runtime props.
12-19: Consider usingIconButtoninstead of a plain<button>for the close action.The repo convention is to prefer common button components from
src/components/button/(e.g.,IconButton.vue) over plain<button>elements for consistent theming. This is a minor nit since the styling here is self-contained and appropriate for a dialog close button.Based on learnings: prefer the repo's common button components from
src/components/button/over plain HTML<button>elements for consistency and theming.src/components/dialog/header/SettingDialogHeader.vue (1)
3-3:cn()is unnecessary for a single conditional class.The ternary already resolves to a single class string — there are no conflicting classes to merge. A plain
:classbinding is simpler.Proposed simplification
- <h2 :class="cn(flags.teamWorkspacesEnabled ? 'px-6' : 'px-4')"> + <h2 :class="flags.teamWorkspacesEnabled ? 'px-6' : 'px-4'">This would also eliminate the
cnimport entirely.src/platform/cloud/subscription/components/SubscriptionPanel.vue (1)
99-101: Prefer a function declaration over a function expression.Per coding guidelines, use function declarations for standalone functions. Also, this is a trivial wrapper — consider assigning
manageSubscriptiondirectly if no additional logic is needed.Option A: function declaration
-const handleInvoiceHistory = async () => { - await manageSubscription() -} +async function handleInvoiceHistory() { + await manageSubscription() +}Option B: direct alias (if the wrapper adds no value)
-const handleInvoiceHistory = async () => { - await manageSubscription() -} +const handleInvoiceHistory = manageSubscriptionsrc/stores/billingOperationStore.ts (2)
117-127: First retry delay is 1.5× the initial interval, notINITIAL_INTERVAL_MS.
scheduleNextPollmultiplies the current interval before scheduling, so the first retry wait is 1500 ms (not the 1000 ms suggested byINITIAL_INTERVAL_MS). If the intent is for the first retry to use the initial interval:Proposed fix — schedule at currentInterval, then bump
function scheduleNextPoll(opId: string) { const currentInterval = intervals.get(opId) ?? INITIAL_INTERVAL_MS const nextInterval = Math.min( currentInterval * BACKOFF_MULTIPLIER, MAX_INTERVAL_MS ) - intervals.set(opId, nextInterval) - - const timeoutId = setTimeout(() => void poll(opId), nextInterval) + const timeoutId = setTimeout(() => void poll(opId), currentInterval) timeouts.set(opId, timeoutId) + intervals.set(opId, nextInterval) }If the 1500 ms first delay is intentional, consider renaming the constant to
BASE_INTERVAL_MSto avoid confusion.
200-233: Completed operations are never auto-cleaned from the Map.
cleanup()clears timeouts, intervals, and toasts but keeps the operation entry inoperations. OnlyclearOperation()removes it. If consumers don't callclearOperation, completed/failed/timed-out operations accumulate indefinitely in the Map.Consider having
handleSuccess,handleFailure, andhandleTimeoutauto-remove operations after a short delay, or document the contract that consumers must callclearOperation.src/composables/auth/useFirebaseAuthActions.ts (1)
85-87: LazyuseBillingContext()call insidepurchaseCreditsis safe but could be hoisted.Calling
useBillingContext()inside the async callback means it runs on everypurchaseCreditsinvocation. WhilecreateSharedComposablecaches the result, hoisting the call to the top ofuseFirebaseAuthActions(alongside other composable initializations at lines 23-25) would be more consistent with the composable initialization pattern and avoid the repeated lookup.Proposed refactor
export const useFirebaseAuthActions = () => { const authStore = useFirebaseAuthStore() const toastStore = useToastStore() const { wrapWithErrorHandlingAsync, toastErrorHandler } = useErrorHandling() + const { isActiveSubscription } = useBillingContext() const accessError = ref(false) ... const purchaseCredits = wrapWithErrorHandlingAsync(async (amount: number) => { - const { isActiveSubscription } = useBillingContext() if (!isActiveSubscription.value) returnsrc/composables/billing/useBillingContext.test.ts (3)
93-96: No test coverage for workspace billing path.All tests exercise the personal/legacy workspace scenario. The workspace billing adapter path (when
isInPersonalWorkspaceisfalse) is untested. Since the PR introduces workspace billing as a core feature, this gap could let regressions in the workspace billing adapter go undetected.
6-21: Mock uses mutable closure state with a custom_setPersonalWorkspacesetter — considervi.hoisted().The mock creates mutable
isInPersonalWorkspaceandactiveWorkspaceobjects inside the factory closure, with a_setPersonalWorkspacemethod to mutate them. Per coding guidelines, prefervi.hoisted()for per-test mock state manipulation to keep module mocks contained. Currently_setPersonalWorkspaceis never called in any test, making it dead code.
87-96:createSharedComposablecaches across the test suite — workspace-switching scenarios won't work as expected.
useBillingContextwrapsuseBillingContextInternalincreateSharedComposable, which caches the setup result on first call. The current tests only exercise the personal/legacy workspace path, so caching poses no problem. However, if you later add tests that call_setPersonalWorkspace(false)to switch to a team workspace, the cached composable instance won't re-evaluate because the internalstorereference captured during setup contains stale values.Current workaround: Keep workspace-switching scenarios out of unit tests (test a single workspace type per test suite). If you need to test both paths, consider separate test files with isolated Pinia instances, or mock at a higher level (component/integration tests).
Note:
useBillingContextInternalis not exported, so testing it directly is not an option.src/platform/cloud/subscription/composables/useSubscriptionCredits.test.ts (2)
8-14: Module-level mutableletstate should usevi.hoisted()for clarity.
mockBillingBalanceandmockBillingIsLoadingare module-levelletvariables read byvi.mockfactories. Sincevi.mockis hoisted above all other statements, it's best practice to usevi.hoisted()to make the shared mutable state explicit and guarantee initialization order.♻️ Suggested refactor
-// Shared mock state (reset in beforeEach) -let mockBillingBalance: { - amountMicros: number - cloudCreditBalanceMicros?: number - prepaidBalanceMicros?: number -} | null = null -let mockBillingIsLoading = false +// Shared mock state (reset in beforeEach) +const { mockBillingBalance, mockBillingIsLoading } = vi.hoisted(() => ({ + mockBillingBalance: { value: null as { + amountMicros: number + cloudCreditBalanceMicros?: number + prepaidBalanceMicros?: number + } | null }, + mockBillingIsLoading: { value: false } +}))Then update usages to access
.valueand adjust the mock/beforeEach accordingly.Based on learnings: "Keep module mocks contained; do not use global mutable state within test files; use
vi.hoisted()if necessary for per-test Arrange phase manipulation."
106-115: Re-instantiating the composable to observe state changes works but is fragile.The comment explains that
computedcaches the value, requiring a freshuseSubscriptionCredits()call. This is becausemockBillingIsLoadingis a plainboolean, not a Vue ref — thecomputed(() => mockBillingIsLoading)in the mock doesn't track reactive dependencies. If this mock were converted to use aref(or ifvi.hoistedwrappers with.valuewere used), the existing computed would reactively update without needing to re-instantiate. This would make the test more realistic and less prone to confusion.src/platform/settings/composables/useSettingUI.ts (1)
278-282: Secrets panel positioning depends on core category ordering — somewhat fragile.The
slice(0, 1)/slice(1)split inserts the secrets panel after the first core category (currently "Comfy"). IfCORE_CATEGORIES_ORDERis reordered or the first element changes, the secrets panel's placement shifts silently. Consider usingfindIndexto locate a specific anchor category, or adding a brief comment noting the intent (e.g., "Insert Secrets after Comfy settings").src/platform/cloud/subscription/components/SubscriptionTransitionPreviewWorkspace.vue (1)
232-257: Proration derivation relies on an implicit API contract.
proratedRefundCentsis computed asnewPlanCost - costToday(the "hidden" credit from the current plan), andproratedChargeCentsequalscostToday. This arithmetic checks out for upgrades (costToday < newPlanCost), but could display misleading numbers for edge cases where the API returns unexpected values (e.g., negativecost_today_centson a downgrade with credit). SinceshowProrationis gated onis_immediate, this is likely safe in practice.src/stores/billingOperationStore.test.ts (1)
234-280: Minor inconsistency in timeout test setup.The subscription timeout test (line 245) advances timers by
0first to flush the immediate poll before advancing by121_000, while the topup timeout test (line 271) jumps straight to121_000. Both should work becauseadvanceTimersByTimeAsync(121_000)subsumes the immediate tick, but aligning them would improve clarity. Not a bug.src/platform/cloud/subscription/components/SubscriptionRequiredDialogContentWorkspace.vue (1)
191-194: Hardcoded payment redirect URLs should be extracted to a shared constant.
'https://www.comfy.org/payment/success'and'https://www.comfy.org/payment/failed'appear in bothhandleAddCreditCard(lines 193–194) andhandleConfirmTransition(lines 248–249). These are environment-specific and duplicated. Extract them alongside the handler-duplication refactor.💡 Suggested extraction
+const PAYMENT_SUCCESS_URL = 'https://www.comfy.org/payment/success' +const PAYMENT_FAILED_URL = 'https://www.comfy.org/payment/failed' + async function handleAddCreditCard() { // ... const response = await subscribe( planSlug, - 'https://www.comfy.org/payment/success', - 'https://www.comfy.org/payment/failed' + PAYMENT_SUCCESS_URL, + PAYMENT_FAILED_URL )Ideally these would live in a shared billing constants module so other components can reuse them.
Also applies to: 246-249
src/platform/cloud/subscription/components/PricingTableWorkspace.vue (1)
335-371: Static tier config usest()from@/i18n— won't react to locale changes.Lines 335-371 build
billingCycleOptionsandtiersusing the statictimport (line 288) rather than the reactivetfromuseI18n(). These values are computed once at setup time. If the app supports runtime locale switching, these labels will remain in the initial locale. If that's acceptable (likely for a billing page), this is fine — just flagging the trade-off.src/composables/billing/types.ts (1)
24-30:BalanceInfopropagates theMicrosnaming inconsistency from the API.The fields are named
amountMicros,effectiveBalanceMicros, etc., but consumers (e.g.,CurrentUserPopoverWorkspace.vue) treat them as cents. Consider adding a JSDoc note on the interface documenting this known API discrepancy so future developers aren't misled by the field names.src/platform/workspace/api/workspaceApi.ts (1)
289-293: No request timeout configured onworkspaceApiClient.All billing API calls (status, balance, subscribe, topup, etc.) use this client without a timeout. If the billing service is slow or unresponsive, requests will hang indefinitely, potentially blocking UI flows. Consider adding a default timeout.
Proposed fix
const workspaceApiClient = axios.create({ headers: { 'Content-Type': 'application/json' - } + }, + timeout: 30_000 })
Comment on lines
+215
to
+217
| function formatNumber(num: number): string { | ||
| return num.toLocaleString('en-US') | ||
| } |
Contributor
There was a problem hiding this comment.
formatNumber hardcodes 'en-US' locale.
The n function from useI18n() (line 173) already handles locale-aware number formatting. Use it instead of toLocaleString('en-US') for consistency with the rest of the i18n setup.
💡 Suggested fix
-function formatNumber(num: number): string {
- return num.toLocaleString('en-US')
-}Then in the template, replace formatNumber(usdToCredits(MIN_AMOUNT)) with n(usdToCredits(MIN_AMOUNT)) and similarly for MAX_AMOUNT.
As per coding guidelines: Use vue-i18n in Composition API for any string literals.
🤖 Prompt for AI Agents
In `@src/components/dialog/content/TopUpCreditsDialogContentWorkspace.vue` around
lines 215 - 217, The helper function formatNumber currently hardcodes the
'en-US' locale; replace its usage with the i18n number formatter (n) from
useI18n to respect the app locale: remove or stop using formatNumber and update
template calls like formatNumber(usdToCredits(MIN_AMOUNT)) and
formatNumber(usdToCredits(MAX_AMOUNT)) to use n(usdToCredits(MIN_AMOUNT)) and
n(usdToCredits(MAX_AMOUNT)); if formatNumber is kept, refactor it to call n
internally (ensure useI18n's n is imported/available) and remove the hardcoded
toLocaleString('en-US') call.
Comment on lines
+134
to
+157
| // Initialize billing when workspace changes | ||
| watch( | ||
| () => store.activeWorkspace?.id, | ||
| async (newWorkspaceId, oldWorkspaceId) => { | ||
| if (!newWorkspaceId) { | ||
| // No workspace selected - reset state | ||
| isInitialized.value = false | ||
| error.value = null | ||
| return | ||
| } | ||
|
|
||
| if (newWorkspaceId !== oldWorkspaceId) { | ||
| // Workspace changed - reinitialize | ||
| isInitialized.value = false | ||
| try { | ||
| await initialize() | ||
| } catch (err) { | ||
| // Error is already captured in error ref | ||
| console.error('Failed to initialize billing context:', err) | ||
| } | ||
| } | ||
| }, | ||
| { immediate: true } | ||
| ) |
Contributor
There was a problem hiding this comment.
Potential race on rapid workspace changes during async initialize.
The watch is async and calls await initialize(). If the active workspace changes while initialize() is still in flight, the watch fires again, resets isInitialized, and starts a second initialize() concurrently. Both compete over isLoading/isInitialized/error refs, so the first completion can set isInitialized = true while the second is still running—leaving stale state.
A lightweight guard (e.g., an abort counter / generation token) would prevent the earlier call from mutating shared state after it becomes irrelevant.
💡 Suggested approach
+ let initGeneration = 0
+
watch(
() => store.activeWorkspace?.id,
async (newWorkspaceId, oldWorkspaceId) => {
if (!newWorkspaceId) {
isInitialized.value = false
error.value = null
return
}
if (newWorkspaceId !== oldWorkspaceId) {
isInitialized.value = false
+ const gen = ++initGeneration
try {
await initialize()
+ // Discard result if workspace changed again while awaiting
+ if (gen !== initGeneration) return
} catch (err) {
+ if (gen !== initGeneration) return
console.error('Failed to initialize billing context:', err)
}
}
},
{ immediate: true }
)🤖 Prompt for AI Agents
In `@src/composables/billing/useBillingContext.ts` around lines 134 - 157, The
async watch callback in useBillingContext (watch on store.activeWorkspace?.id)
can run multiple concurrent initialize() calls and race on shared refs
(isInitialized, isLoading, error); add a lightweight generation token or abort
counter in the composable (e.g., billingInitGeneration) that you increment
before each call, capture into a local variable inside the watch, and have
initialize() or the post-await state mutations check the captured token against
the current billingInitGeneration so only the latest invocation mutates refs;
ensure errors are still assigned to error ref only when the generation matches
to avoid stale state from earlier/faster completions.
Comment on lines
+159
to
+174
| async function initialize(): Promise<void> { | ||
| if (isInitialized.value) return | ||
|
|
||
| isLoading.value = true | ||
| error.value = null | ||
| try { | ||
| await activeContext.value.initialize() | ||
| isInitialized.value = true | ||
| } catch (err) { | ||
| error.value = | ||
| err instanceof Error ? err.message : 'Failed to initialize billing' | ||
| throw err | ||
| } finally { | ||
| isLoading.value = false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
initialize early-return guard is defeated by the watcher.
The guard if (isInitialized.value) return on line 160 is always bypassed because the watcher explicitly sets isInitialized.value = false before calling initialize(). If a second workspace change occurs while the first initialize is still in flight, both invocations will proceed past this guard concurrently. This is a downstream symptom of the race described above—the generation-token fix would also address this.
🤖 Prompt for AI Agents
In `@src/composables/billing/useBillingContext.ts` around lines 159 - 174, The
early-return guard in initialize() is ineffective because the watcher force-sets
isInitialized.value = false, allowing concurrent calls to pass the guard; fix by
introducing single-flight behavior: add an initializingPromise (or
isInitializing flag) scoped alongside isInitialized/isLoading and inside
initialize() if initializingPromise exists return/await it, otherwise set
initializingPromise = activeContext.value.initialize() and await it, then clear
initializingPromise and set isInitialized.value = true; reference initialize(),
isInitialized, isLoading, activeContext.value.initialize to locate where to add
the promise/flag and where to clear it in the try/catch/finally.
Comment on lines
+2192
to
+2194
| "nextMonthInvoice": "Next month invoice", | ||
| "invoiceHistory": "Invoice history", | ||
| "memberCount": "{count} member | {count} members", |
Contributor
There was a problem hiding this comment.
Duplicate invoiceHistory key in the subscription object.
invoiceHistory is defined on both Line 2140 and Line 2193 within the same subscription object. Duplicate JSON keys have undefined behavior per spec — most parsers silently take the last value. Remove one to avoid maintenance confusion if values ever diverge.
✏️ Proposed fix — remove the duplicate on Line 2193
"membersLabel": "Up to {count} members",
"nextMonthInvoice": "Next month invoice",
- "invoiceHistory": "Invoice history",
"memberCount": "{count} member | {count} members",📝 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
| "nextMonthInvoice": "Next month invoice", | |
| "invoiceHistory": "Invoice history", | |
| "memberCount": "{count} member | {count} members", | |
| "nextMonthInvoice": "Next month invoice", | |
| "memberCount": "{count} member | {count} members", |
🤖 Prompt for AI Agents
In `@src/locales/en/main.json` around lines 2192 - 2194, There is a duplicate
"invoiceHistory" key inside the subscription JSON object; remove the redundant
"invoiceHistory" entry so only a single definition of "invoiceHistory" remains
in that object (keep the intended/accurate translation), and run your locale
sanity checks (or tests) to ensure no other duplicates exist and that consumers
still read the expected value.
Comment on lines
+439
to
+441
| const togglePopover = (event: Event) => { | ||
| popover.value.toggle(event) | ||
| } |
Contributor
There was a problem hiding this comment.
Null-check the popover ref before calling .toggle().
popover is typed as ref() (implicitly Ref<any>), so there's no compile-time safety. If the <Popover> hasn't rendered, this will throw at runtime.
Proposed fix
-const popover = ref()
+const popover = ref<InstanceType<typeof Popover> | null>(null)
const togglePopover = (event: Event) => {
- popover.value.toggle(event)
+ popover.value?.toggle(event)
}🤖 Prompt for AI Agents
In `@src/platform/cloud/subscription/components/PricingTableWorkspace.vue` around
lines 439 - 441, The popover ref may be null when the <Popover> hasn't rendered,
so update the togglePopover handler to guard before calling
popover.value.toggle(event); e.g. check popover.value exists (or use optional
chaining popover.value?.toggle(event)) and keep the same signature for
togglePopover; reference the popover ref and the togglePopover function to
ensure the runtime null-check prevents calling .toggle() on undefined.
Comment on lines
628
to
631
| onMounted(() => { | ||
| window.addEventListener('focus', handleWindowFocus) | ||
| void Promise.all([fetchStatus(), fetchBalance()]) | ||
| }) |
Contributor
There was a problem hiding this comment.
Fire-and-forget fetchStatus / fetchBalance on mount — errors silently swallowed.
If either call fails (network issue, auth expired), the user sees a stale or empty state with no feedback. Consider adding a .catch to show a toast or set an error state.
Proposed fix
onMounted(() => {
window.addEventListener('focus', handleWindowFocus)
- void Promise.all([fetchStatus(), fetchBalance()])
+ void Promise.all([fetchStatus(), fetchBalance()]).catch(() => {
+ toast.add({
+ severity: 'error',
+ summary: t('g.error'),
+ detail: t('subscription.loadFailed'),
+ life: 5000
+ })
+ })
})🤖 Prompt for AI Agents
In
`@src/platform/cloud/subscription/components/SubscriptionPanelContentWorkspace.vue`
around lines 628 - 631, The onMounted fire-and-forget
Promise.all([fetchStatus(), fetchBalance()]) swallows errors; update the
onMounted handler (where fetchStatus and fetchBalance are invoked, alongside
handleWindowFocus) to attach a .catch that handles failures — e.g., set a
component error state or call the existing toast/error helper to surface the
error and optionally retry; ensure the .catch distinguishes which call failed if
needed and preserves existing focus listener cleanup.
Comment on lines
+291
to
+314
| async function handleResubscribe() { | ||
| isResubscribing.value = true | ||
| try { | ||
| await workspaceApi.resubscribe() | ||
| toast.add({ | ||
| severity: 'success', | ||
| summary: t('subscription.resubscribeSuccess'), | ||
| life: 5000 | ||
| }) | ||
| await Promise.all([fetchStatus(), fetchBalance()]) | ||
| emit('close', true) | ||
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : 'Failed to resubscribe' | ||
| toast.add({ | ||
| severity: 'error', | ||
| summary: 'Error', | ||
| detail: message, | ||
| life: 5000 | ||
| }) | ||
| } finally { | ||
| isResubscribing.value = false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
handleResubscribe has the same i18n violations as the other handlers.
Lines 304 and 307 use hardcoded strings 'Failed to resubscribe' and 'Error'. Also, this function calls workspaceApi.resubscribe() directly instead of going through the billing context—verify this is intentional (the billing context's subscribe may not support resubscription).
💡 i18n fix
} catch (error) {
const message =
- error instanceof Error ? error.message : 'Failed to resubscribe'
+ error instanceof Error ? error.message : t('subscription.required.resubscribeFailed')
toast.add({
severity: 'error',
- summary: 'Error',
+ summary: t('subscription.required.errorSummary'),
detail: message,
life: 5000
})As per coding guidelines: Use vue-i18n for ALL user-facing strings.
🤖 Prompt for AI Agents
In
`@src/platform/cloud/subscription/components/SubscriptionRequiredDialogContentWorkspace.vue`
around lines 291 - 314, The handler handleResubscribe uses hardcoded user
strings and calls workspaceApi.resubscribe() directly; replace the hardcoded
'Error' and 'Failed to resubscribe' with vue-i18n lookups (e.g. use
t('common.error') for the toast summary and a new/appropriate key such as
t('subscription.resubscribeFailed') for the detail) and update the toast.add
calls to use those keys, and verify/replace the direct API call with the billing
context method (e.g. use billingContext.subscribe or the equivalent resubscribe
helper) if the billing context supports resubscription; if it does not, leave
the resubscribe call but add a comment explaining why billingContext was not
used.
Comment on lines
+594
to
+614
| async subscribe( | ||
| planSlug: string, | ||
| returnUrl?: string, | ||
| cancelUrl?: string | ||
| ): Promise<SubscribeResponse> { | ||
| const headers = await getAuthHeaderOrThrow() | ||
| try { | ||
| const response = await workspaceApiClient.post<SubscribeResponse>( | ||
| api.apiURL('/billing/subscribe'), | ||
| { | ||
| plan_slug: planSlug, | ||
| return_url: returnUrl, | ||
| cancel_url: cancelUrl | ||
| } satisfies SubscribeRequest, | ||
| { headers } | ||
| ) | ||
| return response.data | ||
| } catch (err) { | ||
| handleAxiosError(err) | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
subscribe() omits idempotency_key — inconsistent with other mutating endpoints.
cancelSubscription (line 621) and resubscribe (line 643) accept an optional idempotencyKey parameter, but subscribe does not, even though SubscribeRequest (line 130) defines idempotency_key. For billing mutations, idempotency keys are important to prevent duplicate charges on retry.
Proposed fix
async subscribe(
planSlug: string,
returnUrl?: string,
- cancelUrl?: string
+ cancelUrl?: string,
+ idempotencyKey?: string
): Promise<SubscribeResponse> {
const headers = await getAuthHeaderOrThrow()
try {
const response = await workspaceApiClient.post<SubscribeResponse>(
api.apiURL('/billing/subscribe'),
{
plan_slug: planSlug,
+ idempotency_key: idempotencyKey,
return_url: returnUrl,
cancel_url: cancelUrl
} satisfies SubscribeRequest,
{ headers }
)📝 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
| async subscribe( | |
| planSlug: string, | |
| returnUrl?: string, | |
| cancelUrl?: string | |
| ): Promise<SubscribeResponse> { | |
| const headers = await getAuthHeaderOrThrow() | |
| try { | |
| const response = await workspaceApiClient.post<SubscribeResponse>( | |
| api.apiURL('/billing/subscribe'), | |
| { | |
| plan_slug: planSlug, | |
| return_url: returnUrl, | |
| cancel_url: cancelUrl | |
| } satisfies SubscribeRequest, | |
| { headers } | |
| ) | |
| return response.data | |
| } catch (err) { | |
| handleAxiosError(err) | |
| } | |
| }, | |
| async subscribe( | |
| planSlug: string, | |
| returnUrl?: string, | |
| cancelUrl?: string, | |
| idempotencyKey?: string | |
| ): Promise<SubscribeResponse> { | |
| const headers = await getAuthHeaderOrThrow() | |
| try { | |
| const response = await workspaceApiClient.post<SubscribeResponse>( | |
| api.apiURL('/billing/subscribe'), | |
| { | |
| plan_slug: planSlug, | |
| idempotency_key: idempotencyKey, | |
| return_url: returnUrl, | |
| cancel_url: cancelUrl | |
| } satisfies SubscribeRequest, | |
| { headers } | |
| ) | |
| return response.data | |
| } catch (err) { | |
| handleAxiosError(err) | |
| } | |
| }, |
🤖 Prompt for AI Agents
In `@src/platform/workspace/api/workspaceApi.ts` around lines 594 - 614, The
subscribe method is missing support for idempotency keys; update async
subscribe(planSlug: string, returnUrl?: string, cancelUrl?: string) to accept an
optional idempotencyKey?: string parameter and include idempotency_key:
idempotencyKey in the request payload (alongside plan_slug, return_url,
cancel_url) so the body satisfies SubscribeRequest; keep getAuthHeaderOrThrow
usage and error handling the same.
Mock useDialogService and useDialogStore to prevent unresolved module imports causing "Closing rpc while fetch was pending" errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
almost finished reviewing (31/54)
| :model-value="payAmount" | ||
| :min="0" | ||
| :max="MAX_AMOUNT" | ||
| :step="getStepAmount" |
Contributor
There was a problem hiding this comment.
nit: it seems like it should be a computed prop rather than a getter.
Contributor
There was a problem hiding this comment.
It feels like for organization, it would be nicer to put this (and other files) in a src/platform/workspace folder rather than src/components. Then it might be easy to see all the files together and handle the transition better.
Comment on lines
+268
to
+269
| handleClose(false) | ||
| dialogService.showSettingsDialog('workspace') |
Contributor
There was a problem hiding this comment.
Are we worried at all about the timing if this has possibility of tearing down the state of the current component?
| toast.add({ | ||
| severity: 'error', | ||
| summary: t('credits.topUp.purchaseError'), | ||
| detail: t('credits.topUp.unknownError'), |
Contributor
There was a problem hiding this comment.
WDYT about sending something to Sentry here and below -- to benefit our future selves if we have to debug.
Comment on lines
+346
to
+348
| if (isActiveSubscription.value) { | ||
| void fetchBalance() | ||
| } |
Contributor
There was a problem hiding this comment.
Would we want to fetch the balance even if isActiveSubscription was false here or would it never work?
| const { subscriptionTierName: userSubscriptionTierName } = useSubscription() | ||
| const { subscription } = useBillingContext() | ||
|
|
||
| const tierKeyMap: Record<string, string> = { |
Contributor
There was a problem hiding this comment.
Should we use this
Comment on lines
+153
to
+163
| function formatTierName( | ||
| tier: string | null | undefined, | ||
| isYearly: boolean | ||
| ): string { | ||
| if (!tier) return '' | ||
| const key = tierKeyMap[tier] ?? 'standard' | ||
| const baseName = t(`subscription.tiers.${key}.name`) | ||
| return isYearly | ||
| ? t('subscription.tierNameYearly', { name: baseName }) | ||
| : baseName | ||
| } |
Contributor
There was a problem hiding this comment.
This one might also benefit from being moved to https://github.com/Comfy-Org/ComfyUI_frontend/blob/main/src/platform/cloud/subscription/constants/tierPricing.ts
Contributor
There was a problem hiding this comment.
This file makes me think OOP truly is the best
| await legacyManageSubscription() | ||
| } | ||
|
|
||
| async function fetchPlans(): Promise<void> { |
Contributor
There was a problem hiding this comment.
the legendary async no-op
christian-byrne
approved these changes
Feb 6, 2026
Comment on lines
+20
to
+30
| function formatBalance(maybeCents: number | undefined, locale: string): string { | ||
| const cents = maybeCents ?? 0 | ||
| return formatCreditsFromCents({ | ||
| cents, | ||
| locale, | ||
| numberOptions: { | ||
| minimumFractionDigits: 0, | ||
| maximumFractionDigits: 0 | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
nit: might be worth moving to shared file in the future
Contributor
There was a problem hiding this comment.
This is great, nice work here
Contributor
There was a problem hiding this comment.
nit: would like it to be in domain folder for billing/credits/subscriptions
|
simula-r
added a commit
that referenced
this pull request
Feb 7, 2026
Implements billing infrastructure for team workspaces, separate from legacy personal billing. - **Billing abstraction**: New `useBillingContext` composable that switches between legacy (personal) and workspace billing based on context - **Workspace subscription flows**: Pricing tables, plan transitions, cancellation dialogs, and payment preview components for workspace billing - **Top-up credits**: Workspace-specific top-up dialog with polling for payment confirmation - **Workspace API**: Extended with billing endpoints (subscriptions, invoices, payment methods, credits top-up) - **Workspace switcher**: Now displays tier badges for each workspace - **Subscribe polling**: Added polling mechanisms (`useSubscribePolling`, `useTopupPolling`) for async payment flows - Billing flow correctness for workspace vs legacy contexts - Polling timeout and error handling in payment flows ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8508-Feat-workspaces-6-billing-2f96d73d365081f69f65c1ddf369010d) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
simula-r
added a commit
that referenced
this pull request
Feb 7, 2026
Backport of #8508 to `cloud/1.38` Automatically created by manual backport (cherry-pick of c5431de). Conflicts resolved: - `src/platform/cloud/subscription/composables/useSubscriptionCredits.test.ts` — accepted PR version (uses `useBillingContext` mock instead of pinia/firebase auth) - `src/services/dialogService.ts` — merged: kept cloud/1.38 eager imports for dialog components, replaced `TopUpCreditsDialogContent` with Legacy/Workspace variants, replaced `useSubscription` with `useBillingContext`, added workspace/legacy component selection in `showTopUpCreditsDialog`; skipped lazy loader refactor (separate PR, not part of #8508) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
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
Implements billing infrastructure for team workspaces, separate from legacy personal billing.
Changes
useBillingContextcomposable that switches between legacy (personal) and workspace billing based on contextuseSubscribePolling,useTopupPolling) for async payment flowsReview Focus
┆Issue is synchronized with this Notion page by Unito