-
Notifications
You must be signed in to change notification settings - Fork 391
subscription page #6064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
subscription page #6064
Conversation
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/19/2025, 03:01:54 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/19/2025, 03:18:54 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is a very good draft.
I made a couple of comments on how and where we can be dynamically importing off of isCloud
.
That and moving the subscription stuff out of firebase and into it's own store are the big comments.
src/platform/cloud/subscription/components/SubscriptionRequiredDialogContent.vue
Outdated
Show resolved
Hide resolved
src/stores/firebaseAuthStore.ts
Outdated
* Fetch the current cloud subscription status for the authenticated user | ||
* @returns Subscription status or null if no subscription exists | ||
*/ | ||
const fetchSubscriptionStatus = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscription logic might be better of in it's own dedicated store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtydhr88 we can't put this in a subscription store that we can dynamically import?
}) | ||
const latestEvents = ref<AuditLog[]>([]) | ||
const isLoadingEvents = ref(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: if we moved some of this logic into a composable we could probably break this up into several smaller components.
src/App.vue
Outdated
if (!isLoggedIn.value) return | ||
try { | ||
const status = await authActions.fetchSubscriptionStatus() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
071bee4
to
3a205f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtydhr88 left a few more comments. The overall feature code looks solid. It's mainly around making sure we properly set up build time flags so we can remove this code where it's not needed.
@christian-byrne I think we should move the subscription-specific files into @src//platform/cloud/subscription/
. It would definitely centralize everything. Thoughts?
src/stores/firebaseAuthStore.ts
Outdated
* Fetch the current cloud subscription status for the authenticated user | ||
* @returns Subscription status or null if no subscription exists | ||
*/ | ||
const fetchSubscriptionStatus = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtydhr88 we can't put this in a subscription store that we can dynamically import?
dialogStore.showDialog({ | ||
key: 'subscription-required', | ||
component: SubscriptionRequiredDialogContent, | ||
dialogComponentProps: { | ||
closable: true, | ||
style: 'width: 720px; height: 434px;', | ||
pt: { | ||
content: { | ||
class: 'overflow-hidden p-0!' | ||
} | ||
} | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dialogStore.showDialog({ | |
key: 'subscription-required', | |
component: SubscriptionRequiredDialogContent, | |
dialogComponentProps: { | |
closable: true, | |
style: 'width: 720px; height: 434px;', | |
pt: { | |
content: { | |
class: 'overflow-hidden p-0!' | |
} | |
} | |
} | |
}) | |
if(isCloud) { | |
dialogStore.showDialog({ | |
key: 'subscription-required', | |
component: SubscriptionRequiredDialogContent, | |
dialogComponentProps: { | |
closable: true, | |
style: 'width: 720px; height: 434px;', | |
pt: { | |
content: { | |
class: 'overflow-hidden p-0!' | |
} | |
} | |
} | |
}) | |
} |
@christian-byrne would we handle it like this so it compiles to a noop? Or should we actually create a cloud dialog service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(replying to comment about whether this needs a no op) This should be tree-shaken due to the early return above I think - so I think it's fine how it is. (no change necessary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christian-byrne do we want assets here or should this just get uploaded to GCS and we serve up a publicly signed link?
1557281
to
8cfc50d
Compare
There is a merge conflict it seems. |
8cfc50d
to
6f2c404
Compare
@christian-byrne fixed |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
Are these failing tests expected? Can you check the visual results: https://dbd3e99a.comfyui-playwright-chromium.pages.dev/#?q=s:failed |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
Bundle Size ReportApp Entry PointsMain application bundles
Category Total: 11.7 MB Core ViewsMajor application views and screens
Category Total: 714 kB UI PanelsSettings and configuration panels
Category Total: 74.8 kB ServicesBusiness logic and services
Category Total: 10 kB UtilitiesHelper functions and utilities
Category Total: 1.07 kB OtherUncategorized bundles
Category Total: 11.9 kB Overall Total Size: 12.5 MB |
src/platform/cloud/subscription/components/SubscriptionPanel.vue
Outdated
Show resolved
Hide resolved
src/platform/cloud/subscription/components/SubscriptionPanel.vue
Outdated
Show resolved
Hide resolved
src/platform/cloud/subscription/components/SubscriptionPanel.vue
Outdated
Show resolved
Hide resolved
src/platform/cloud/subscription/components/SubscriptionPanel.vue
Outdated
Show resolved
Hide resolved
dialogStore.showDialog({ | ||
key: 'subscription-required', | ||
component: SubscriptionRequiredDialogContent, | ||
dialogComponentProps: { | ||
closable: true, | ||
style: 'width: 720px; height: 434px;', | ||
pt: { | ||
content: { | ||
class: 'overflow-hidden p-0!' | ||
} | ||
} | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(replying to comment about whether this needs a no op) This should be tree-shaken due to the early return above I think - so I think it's fine how it is. (no change necessary).
@jtydhr88 Backport to Please manually cherry-pick commit Conflicting files
|
1 similar comment
@jtydhr88 Backport to Please manually cherry-pick commit Conflicting files
|
Summary Implements cloud subscription management UI and flow for ComfyUI Cloud users. Core Features: - Subscription Status Tracking: Global reactive state management for subscription status across all components using shared subscriptionStatus ref - Subscribe to Run Button: Replaces the Run button in the actionbar with a "Subscribe to Run" button for users without active subscriptions - Subscription Required Dialog: Modal dialog with subscription benefits, pricing, and checkout flow with video background - Subscription Settings Panel: New settings panel showing subscription status, renewal date, and quick access to billing management - Auto-detection & Polling: Automatically polls subscription status after checkout completion and syncs state across the application https://github.com/user-attachments/assets/f41b8e6a-5845-48a7-8169-3a6fc0d2e5c8 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6064-subscription-page-28d6d73d36508135a2a0fe7c94b40852) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <[email protected]>
## Summary Backport of #6064 (subscription page) to the `rh-test` branch. This PR manually cherry-picks commit 7e1e8e3 to the rh-test branch and resolves merge conflicts that prevented automatic backporting. ## Conflicts Resolved ### 1. `src/components/actionbar/ComfyActionbar.vue` - **Conflict**: HEAD (rh-test) used `<ComfyQueueButton />` while the subscription PR introduced `<ComfyRunButton />` - **Resolution**: Updated to use `<ComfyRunButton />` to include the subscription functionality wrapper while maintaining the existing rh-test template structure ### 2. `src/composables/auth/useFirebaseAuthActions.ts` - **Conflict**: Simple ordering difference in the return statement - **Resolution**: Used the subscription PR's ordering: `deleteAccount, accessError, reportError` ## Testing The cherry-pick completed successfully and passed all pre-commit hooks: - ✅ ESLint - ✅ Prettier formatting -⚠️ Note: 2 unused i18n keys detected (informational only, same as original PR) ## Related - Original PR: #6064 - Cherry-picked commit: 7e1e8e3 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6140-backport-subscription-page-to-rh-test-2916d73d365081f38f00df422004f61a) by [Unito](https://www.unito.io) Co-authored-by: Terry Jia <[email protected]> Co-authored-by: GitHub Action <[email protected]>
Summary
Implements cloud subscription management UI and flow for ComfyUI Cloud users.
Core Features:
using shared subscriptionStatus ref
without active subscriptions
background
billing management
across the application
2025-10-17.15-31-28.mp4
┆Issue is synchronized with this Notion page by Unito