[pull] main from Comfy-Org:main - #7
Merged
Merged
Conversation
…14403) ## Summary Each GLSL preview node created its own OffscreenCanvas WebGL2 context. Browsers cap a page at ~16 live WebGL contexts and silently evict the oldest past the cap: isContextLost() turns true, no contextlost event fires for the OffscreenCanvas, and that node preview freezes with no recovery. Reproduced with 18 GLSL nodes: the 2 oldest contexts were evicted and stopped producing frames. All renderers now draw through a single ref-counted shared context (same pattern as renderer/three/sharedWebGLRenderer), destroyed when the last renderer disposes and recreated if the browser loses it. Renderers keep owning their programs, textures, and ping-pong FBOs; the canvas and viewport are re-applied per frame since other renderers resize them between frames. Interleaving is safe because each render pass stays within one synchronous block through the convertToBlob call, which snapshots the bitmap synchronously.
…14685) ## Summary The upload step tells the user to swap in their own image, then a full-screen blocker swallows every click on the node it is spotlighting. ## Changes - **What**: `interactive` was an allowlist of `prompt` and `run`, so `upload` fell through to the non-interactive branch in `TourSpotlight.vue`, which renders a `pointer-events-auto` overlay across the whole viewport. Inverted to exclude only `result` — the one step with nothing to touch. ## Review Focus Advancing is unaffected: only `run` is `selfAdvancing`, and `upload` and `prompt` both advance on Next, so `upload` now behaves exactly like `prompt` already did. The existing test enumerated the old list with no rationale, so it went red. Rewritten to assert the rule rather than the list, and mutation-checked — reverting the one line turns it red again. Found by hand against a real backend. The whole `interactive` cluster also drives the focus trap and `aria-modal`, none of which is observable in happy-dom, which is why the suite stayed green through this. ## What changed. Before: clicking the spotlit LoadImage node on the upload step does nothing. After: the node takes the click and the file picker opens.
> **Stacked PR 1 of 5** — cloud auth redesign. Merge in order, bottom-up. > > | | PR | What it does | > |---|---|---| > | 1 | #14694 | Dark theme, brand tokens, PP Formula shipped cloud-only | > | 2 | #14695 | Button keeps its label for screen readers while loading | > | 3 | #14696 | Brand button variants; login, sign-up and forgot-password rebuilt on them | > | 4 | #14697 | Logo, content and terms share one left edge | > | 5 | #14698 | Static hero collage becomes a video carousel | ## Summary Lands the shell the cloud auth redesign sits on: brand colour tokens, the white-transparency steps, and PP Formula shipped cloud-only. ## Changes - **What**: Adds `--color-primary-comfy-yellow`, `--color-primary-warm-white`, `--color-primary-warm-gray` and two white-transparency tokens to the design system. Ships three PP Formula faces on the same rails ABC ROM already used — faces live under the onboarding assets, the Vite plugin strips them from non-cloud bundles, and CI fails the build if they reach an OSS `dist/`. - Deletes ABC ROM Extended and its `.hero-title` / `.font-abcrom` rules. Nothing referenced them, so they were proprietary bytes bundled for no visual benefit. ## Review Focus The font guard is the thing to check. Both directions are verified: ``` DISTRIBUTION=localhost pnpm build && find dist -iname "*ppformula*" -o -iname "*abcrom*" # empty DISTRIBUTION=cloud pnpm build && find dist -iname "*ppformula*" # 3 files ``` First of 5 stacked PRs — see the stack table in #1 of the series.
*PR Created by the Glary-Bot Agent* --- Adds a BYOK entry to the `/cloud/pricing` FAQ answering whether users can bring their own provider key, in both `en` and `zh-CN`. **Q:** Does Comfy support BYOK (bring your own key)? Can I bring my own provider key? **A:** Yes. We support BYOK for **Runway and Google Gemini** today, and we can add other providers on request for Enterprise customers. BYOK is turned on per workspace, and Partner Node calls that run on your own key are billed by that provider directly instead of drawing your Comfy credits. [Submit an Enterprise request](https://comfy.org/contact) and we'll get you set up. Placed at `order: 15`, directly after "What are Partner Nodes, and do they cost extra?" — BYOK only makes sense in the Partner Node context. The three trailing entries (refunds, invoices, at-scale) shift down one slot so the Enterprise at-scale question stays the closer. The `zh-CN` entry links to `/zh-CN/contact` to stay in-locale. Copy is grounded in the cloud backend rather than assumed: supported providers are `runway` and `gemini` (`src/platform/secrets/providers.ts`), entitlement is per `(workspace, provider)` and deny-by-default (`cloud/services/ingest/entitlement/entitlement.go`), and BYOK requests skip the platform charge (`cloud/services/comfy-api/services/comfy_api/byok_metering.go`) — hence "turned on per workspace" and "billed by that provider directly" rather than implying self-serve availability. ### Verification - `pnpm --filter @comfyorg/website build` — 581 pages, exit 0 - `pnpm --filter @comfyorg/website test:unit` — 327 passed - `validate:jsonld` — passed across 584 pages - `playwright test --project visual --grep "cloud/pricing"` — 4/4 overflow guards pass (sm/md/lg/xl) - Browser check against `astro preview`: the entry renders in position 16 of 19 in both locales, expands correctly, and the CTA navigates to the Contact Sales form (which has an explicit "Enterprise" option). No horizontal overflow at 375px. - `astro check` reports 1 error, pre-existing and unrelated to this change: `apps/website/e2e/minimax.spec.ts:20` (`minimaxPage.faq` possibly undefined). **Needs a screenshot refresh:** the `pricing-faq-*` visual baselines will diff by one accordion row. Local regeneration isn't trustworthy in this environment (the unmodified `pricing-tiers-3-lg` / `4-xl` baselines already fail here), so please run `/update-website-screenshots` or tick the screenshot checkbox on the E2E status comment. ## Screenshots    --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com>
> **Stacked PR 2 of 5** — cloud auth redesign. Merge in order, bottom-up. > > | | PR | What it does | > |---|---|---| > | 1 | #14694 | Dark theme, brand tokens, PP Formula shipped cloud-only | > | 2 | #14695 | Button keeps its label for screen readers while loading | > | 3 | #14696 | Brand button variants; login, sign-up and forgot-password rebuilt on them | > | 4 | #14697 | Logo, content and terms share one left edge | > | 5 | #14698 | Static hero collage becomes a video carousel | ## Summary The shared `Button` dropped its label while loading, so assistive tech lost the control name for as long as the request was in flight. ## Changes - **What**: Keeps the label in the accessibility tree via `sr-only`, hides the decorative spinner from screen readers, and marks the control `aria-busy` while loading. Visually unchanged. ## Review Focus `Button.vue` is used app-wide, so the loading change is worth a look beyond the cloud pages — the rendered output is identical, only the accessibility tree differs. This PR is now two files. The locale changes it originally carried moved to the PRs that consume them (auth copy to #14696, carousel keys to #14698), so no intermediate merge renders raw i18n keys. Stacked on #14694.
> **Stacked PR 3 of 5** — cloud auth redesign. Merge in order, bottom-up. > > | | PR | What it does | > |---|---|---| > | 1 | #14694 | Dark theme, brand tokens, PP Formula shipped cloud-only | > | 2 | #14695 | Button keeps its label for screen readers while loading | > | 3 | #14696 | Brand button variants; login, sign-up and forgot-password rebuilt on them | > | 4 | #14697 | Logo, content and terms share one left edge | > | 5 | #14698 | Static hero collage becomes a video carousel | ## Summary Rebuilds the login, sign-up and forgot-password panels on real design-system variants instead of a hand-pasted class string. ## Changes - **What**: Adds the auth copy keys (`cloudNewUser`, `cloudSignUp`, `freeRunsSuffix`) this PR's views consume, plus `brand-ghost`, `brand-solid`, `brand-ghost-accent` variants and `brand` / `brand-icon` sizes to the shared `Button`. The same long class string was previously pasted at six sites with `size="unset"` — every one of those buttons was opting out of the design system it was meant to use. - Extracts `useCloudAuthPage` and `CloudSocialAuthButtons` from the near-identical login and sign-up views (~85% duplicated markup plus identical OAuth handlers). - `SignUpForm` / `PasswordFields` take typed `ButtonVariants` props instead of opaque class strings. Defaults are unchanged, so the desktop sign-in dialog and update-password dialog render exactly as before. ## Review Focus - **Shared-component blast radius**: `SignUpForm` and `PasswordFields` are also used by `SignInContent.vue` and `UpdatePasswordContent.vue`. The `h-10` field default is deliberately preserved — those two dialogs have no test coverage. - The variants and the views ship together on purpose: `button.variants.ts` has zero consumers on its own, so splitting them would merge dead code. The panel is width-bounded here so this PR is visually valid on its own; #14697 relocates that constraint onto its own wrapper. Stacked on the button a11y PR.
## What and why Picking a different plan part-way through checkout makes the API replace the pending subscription operation with a new one. The replaced operation terminates as `failed` — it never completed — even though nothing actually went wrong. The store treated that like any other failure and showed **"We couldn't update your subscription. Please try again."** while the replacement checkout was still running and about to succeed. Users hit this by starting a subscription, changing their mind, and picking a different plan: an error toast, immediately followed by a working subscription. ## The change `handleFailure` now recognises the API's `checkout_superseded` failure reason and skips the error toast for it. The operation is still recorded as failed and still resolves normally — only the user-facing alarm goes away. The failure is also reported as `stale_operation` instead of `unknown`, so a routine plan switch stops inflating the count of unexplained billing failures. `stale_operation` is an existing category; no telemetry types changed. Takes effect once the API returns `checkout_superseded` for a replaced checkout. ## Scope One `if` and one telemetry value. Deliberately narrow: - Genuine subscription failures still toast exactly as before. - Top-up and cancel paths are untouched. - The success path already ignores stale operations (it checks the active checkout id before advancing), so it needed no change. ## Verification - `vitest run src/platform/workspace/stores/billingOperationStore.test.ts src/platform/workspace/composables/useSubscriptionCheckout.test.ts --coverage` — 144 passed, run **with** coverage to match CI. - New regression test: a superseded operation raises no error toast and reports `stale_operation`. - `pnpm typecheck`, `oxfmt --check`, `eslint` — all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#11464) *PR Created by the Glary-Bot Agent* --- ## Summary Stacked on #11463. Renames the public `isActiveSubscription` composable property to `canAccessSubscriptionFeatures` across the codebase. Pure identifier rename, zero behavioral changes. ## Motivation The name `isActiveSubscription` was misleading. Its actual definition in `useSubscription.ts`: ```ts const canAccessSubscriptionFeatures = computed(() => { if (!isCloud || !window.__CONFIG__?.subscription_required) return true return subscriptionStatus.value?.is_active ?? false }) ``` Semantics: returns `true` when **any** of these hold: - build is not cloud, OR - subscription is not required, OR - user has an active subscription So it was `true` on local/desktop builds regardless of actual subscription state. Several call sites (e.g. `CurrentUserPopoverLegacy.vue` with its redundant `isCloud && isActiveSubscription` pattern) suggested authors misread the name as "literally has active subscription" and added defensive guards. The new name reads correctly at every call site: "if can access features, show UI / allow action". PR #7127 previously attempted the internal rename (`isSubscribed` → `isSubscribedOrIsNotCloud`) but was closed without merge after scope-creep feedback from CodeRabbit. It never touched the exported public name. This PR completes that work. ## Changes - Renamed the public composable property `isActiveSubscription` → `canAccessSubscriptionFeatures` in: - `useSubscription` (the source of truth) - `useBillingContext` (convenience wrapper) - `useWorkspaceBilling` (workspace-scoped variant) - `useLegacyBilling` - Renamed the internal alias `isSubscribedOrIsNotCloud` → `canAccessSubscriptionFeatures` to match. - Updated all 112 call sites across 37 files (templates, composables, services, tests). - Converted the one kebab-case prop binding (`:is-active-subscription` → `:can-access-subscription-features`) in `MembersPanelContent.vue`. ## Verification - `pnpm typecheck`: clean - `pnpm test:unit`: **613 files / 8125 tests pass** — existing coverage preserved since the rename is identifier-only - `pnpm lint` / `pnpm format:check`: clean (pre-commit hooks auto-format) ## Coverage note Per request for "65%+ unit test coverage on lines/functions in the diff": because this PR is a pure rename with zero logical change, the existing test suite (which continues to pass in full) covers every renamed line at exactly the coverage level it had before the rename. No new behavior was introduced that needs new test cases. The subscription composables, billing context, and the popover components involved already have dedicated test files (`useSubscription.test.ts`, `useBillingContext.test.ts`, `useWorkspaceBilling.test.ts`, `CurrentUserPopoverLegacy.test.ts`, `SubscriptionPanel.test.ts`, `PricingTable.test.ts`, etc.). --- Targets `glary/fix-fe-219-credits-local` (the PR #11463 branch). Merge that PR first. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11464-refactor-rename-isActiveSubscription-to-canAccessSubscriptionFeatures-3486d73d3650813d8f7ecc8aea35b75d) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: dante01yoon <bunggl@naver.com> Co-authored-by: Amp <amp@ampcode.com>
…pes and forward billing events to the desktop host sink (#14310) ## Summary Two coupled billing-telemetry correctness fixes. Both make billing dashboards under-report silently rather than fail loudly, which is why they ship together. Plan doc: [Billing telemetry program dossier](https://app.notion.com/p/3ad6d73d365081fea125e284b6ac9f13) ## Changes - **What**: 1. `billingOperationStore.handleSuccess` now emits `billing.operation.succeeded` for **all three** operation types (`subscription`, `topup`, `cancel`). It previously only emitted it for `cancel`, while `billing.operation.failed` and `billing.operation.timeout` are emitted for all three. Any `succeeded / (succeeded + failed)` ratio grouped by `operation_type` therefore read a **permanent 0%** for `subscription` and `topup` — a metric that can never be non-zero, so an anomaly monitor on it pages immediately and forever. 2. `HostTelemetrySink` now implements `trackBillingEvent`. It did not, and `initHostTelemetry` *replaces* the provider registry rather than extending it, so a cloud build running inside the desktop shell emitted **zero** canonical `billing.*` events to any provider — an entire user segment missing from every billing funnel. - **Breaking**: no. Both changes are additive at the event level. No event was renamed, removed, or had a property removed. ### Event contract after this change `billing.operation.succeeded` is now emitted once per successful billing operation, for every operation type, with the property set below. This matches `billing.operation.failed` / `billing.operation.timeout` exactly, so numerator and denominator of a success-rate ratio are groupable by the same tags: | Property | Type | Presence | | --- | --- | --- | | `operation` | `'operation'` | always | | `stage` | `'succeeded'` | always | | `outcome` | `'success'` | always | | `billing_op_id` | string | always | | `operation_type` | `'subscription' \| 'topup' \| 'cancel'` | always | | `tier` | tier key or `'team'` | when the caller supplied it | | `cycle` | `'monthly' \| 'yearly'` | when the caller supplied it | | `checkout_type` | `'new' \| 'change'` | when the caller supplied it | | `payment_intent_source` | attribution enum | when the caller supplied it | Notes for anyone querying this: - A success-rate monitor should now be `succeeded / (succeeded + failed)` over `billing.operation.*`, grouped by `operation_type`, and it will be non-zero for all three types. - `.timeout` is a separate stage from `.failed`; a ratio that should treat timeouts as failures must include both in the denominator. - The flow-specific success events `billing.subscription_checkout.succeeded` and `billing.topup.succeeded` are unchanged and still fire. A `subscription` success therefore emits both `billing.operation.succeeded` and `billing.subscription_checkout.succeeded`. These are distinct event names — do not sum them into one success count. - The four optional grouping keys are omitted from the payload when the caller passed no metadata (the workspace top-up path currently starts its poller without metadata), so group-bys on them must tolerate a missing tag rather than assume it is present. ## Review Focus **How to review this, and what it is part of.** This is one step in a broader effort to make the billing/subscription funnel observable end to end. The two fixes here are both "the dashboard lies" bugs rather than user-visible bugs, so the thing to check is the *event contract*, not UI behavior. Suggested reading order: 1. `billingOperationStore.ts` — the whole fix is one moved emit. The new `billing.operation.succeeded` call is unconditional at the top of `handleSuccess`'s telemetry block; the old `else` branch that emitted it only for `cancel` is deleted. Confirm the `cancel` payload is unchanged in practice (`cancel` operations are started without metadata, so the four new optional keys serialize away) and that `subscription` / `topup` now additionally emit it. 2. `HostTelemetrySink.ts` — a four-line method delegating to the same `getBillingTelemetryEventName` / `getBillingTelemetryEventPayload` helpers the two cloud providers use, so the host rail receives a byte-identical property set. 3. The three test files — each new test fails against the unfixed source and passes with it (verified by reverting only the source and re-running). **The design question worth a second opinion (fix 2).** There were two candidate fixes: implement `trackBillingEvent` on the sink, or make `initHostTelemetry` compose with the existing registry instead of replacing it. This PR implements the sink method, because that is what the surrounding code's intent points at: the sink already forwards 46 of the 54 `TelemetryProvider` methods through a single `capture()` bridge, and `trackBillingEvent` postdates the sink's creation — it was never a deliberate exclusion, just a method added later and not backfilled. Registry *replacement*, by contrast, looks deliberate: in the desktop shell the host process is the single telemetry egress point, and composing would double-emit every event on two rails with two different identities, affecting all ~40 forwarded events rather than just billing. That is a much larger blast radius and a cross-surface decision, so it is left as a follow-up question rather than decided here: **should a cloud build inside the desktop shell fan out to both the host bridge and the cloud providers?** Worth a reviewer's opinion, not blocking this fix. Also out of scope but noted while in here: `trackAuthFailed`, `trackExecutionOutcome`, `trackOnboardingTour`, `trackShellLayout`, `trackWidgetFavoriteToggled`, `trackWorkspaceInviteSent`, and `trackWorkspaceInviteFailed` are likewise absent from the host sink. Only `trackBillingEvent` is in scope here. **Why these two are worth landing promptly.** Fix 1 unblocks success-rate monitoring: a private, access-controlled infra repository has monitors pending review that are built on exactly the `succeeded / (succeeded + failed)` shape, and they cannot be applied against the current event stream without alerting permanently on an unreachable metric. Fix 2 closes a whole-segment blind spot — desktop-in-cloud users are currently absent from every billing funnel, on every provider. **Overlap with open PRs.** #14216 also edits `handleSuccess` in this file, adding a dual-emit *inside* the `type === 'subscription'` branch. This PR's hunk is an insertion *before* that branch plus the deletion of the trailing `else`. The two are semantically independent and compatible; expect at most a trivial textual conflict depending on merge order. This PR deliberately does **not** touch `useDowngradeToPersonal.ts`, whose three remaining `failure_category: 'unknown'` sites are already claimed by #14229. ## Testing Six new regression tests, all confirmed to fail against the unfixed source: - `billingOperationStore.test.ts` — parameterized over all three operation types, asserting `billing.operation.succeeded` fires with the full grouping-key payload. This is the test that pins the parity invariant. - `HostTelemetrySink.test.ts` — a success event and a failure event are forwarded to the host bridge with the derived event name and payload. - `initHostTelemetry.test.ts` — end-to-end through the registry: after `initHostTelemetry()`, a `trackBillingEvent` call reaches the host bridge. No e2e added: this is provider-dispatch-level code with no UI surface, and exercising it in Playwright would require driving a real billing flow against live payment rails. Per `browser_tests/README.md` this belongs at the unit level. Gates: `pnpm typecheck`, `pnpm lint`, `pnpm format:check`, `pnpm knip` all clean; full unit suite green (1067 files, 14204 passed / 8 skipped). Co-authored-by: Connor Byrne <c.byrne@comfy.org>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )