From 3e1b0d270e670a12e1551b10848176f7934e632f Mon Sep 17 00:00:00 2001 From: Maanil Verma Date: Tue, 18 Aug 2026 16:49:51 +0000 Subject: [PATCH 1/5] test: cover the cloud auth surface (#14987) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Adds the missing test coverage for the cloud sign-in and sign-up surface. Four files had no tests at all. ## Changes - **What**: Tests only, no product code. Previously uncovered: the `beforeEnter` route guards and their `switchAccount` escape hatch, `usePostAuthRedirect`, the sign-in password schema, and the terms notice. The rest extends existing files rather than duplicating their setup. Covered here: route guards on both `/cloud/login` and `/cloud/signup`, post-auth redirect precedence and its open-redirect guard, password boundaries and character classes, the terms notice and its outbound-link safety, the sign-in form's validation states and autofill attributes, the turnstile gate on sign-up, and the insecure-context warning for self-hosted HTTP origins. - **Breaking**: None. ## Review Focus The route-guard tests drive both `/cloud/login` and `/cloud/signup` through the same `describe.for`. Those guards are byte-identical duplicates, so testing one would leave the other free to regress. One assumption from the plan turned out to be wrong, and the test pins the real behaviour instead: a pristine sign-in button is **enabled**, not disabled. PrimeVue leaves `$form.valid` undefined until a field is touched, so submitting a pristine form is what surfaces the errors. `globalThis.isSecureContext` is `undefined` under happy-dom rather than `true`, so the insecure-context test stubs the *secure* case. The value is a setup-time snapshot, so the stub has to precede mount. ## Testing 90 tests. Every one was mutation-checked: the behaviour it pins was broken in the source, the test confirmed failing, then reverted. **No survivors.** | Area | Cases | Representative mutation → result | | --- | --- | --- | | Password + email schema | 19 | drop any one complexity regex → that case fails; move the refine `path` → mismatch fails | | Open-redirect guard | 7 | read the last repeated query entry instead of the first → fails; decode fallback returns a `/` path → fails | | Post-auth redirect | 7 | `router.replace` → `push` → fails; drop the OAuth-resume early return → deep-link precedence fails | | Route guards | 19 | negate `isLoggedIn` → both routes fail; `if (!to.query.switchAccount)` → `if (true)` → escape hatch fails | | Terms notice | 5 | drop `rel="noopener noreferrer"` → fails; hard-code any locale fragment → fails | | Sign-in form | 12 | drop `:disabled="!$form.valid"` → fails; `current-password` → `new-password` → fails | | Sign-up form | 15 | remove `useThrottleFn` → double-submit fails; make `resetTurnstile` a no-op → fails | | Login view | 6 | insecure-context warning `v-if="false"` → fails; force the webview notice → fails | Three guards are unkillable in isolation and die only once the outer gate is also removed, which is the signature of defence-in-depth rather than dead code: `CloudSignInForm.onSubmit`'s `event.valid`, `SignUpForm.onSubmit`'s `waitingForTurnstile`, and both forms' loading gate behind `Button`'s `disabled || loading`. No e2e spec. The two flows worth driving end to end — the deep-link round trip and the already-signed-in redirect — are pinned closer to the logic in `usePostAuthRedirect.test.ts` and `onboardingCloudRoutes.test.ts`, the latter on both routes. A browser spec added a slower path to the same assertions. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit Co-authored-by: Christian Byrne --- .../dialog/content/signin/SignUpForm.test.ts | 82 ++++++-- .../cloud/onboarding/CloudLoginView.test.ts | 32 ++- .../components/CloudSignInForm.test.ts | 195 ++++++++++++++++++ .../components/CloudTermsNotice.test.ts | 104 ++++++++++ .../composables/usePostAuthRedirect.test.ts | 164 +++++++++++++++ .../onboarding/onboardingCloudRoutes.test.ts | 139 +++++++++++-- .../onboarding/utils/previousFullPath.test.ts | 23 +++ src/schemas/signInSchema.test.ts | 139 +++++++++++++ 8 files changed, 846 insertions(+), 32 deletions(-) create mode 100644 src/platform/cloud/onboarding/components/CloudSignInForm.test.ts create mode 100644 src/platform/cloud/onboarding/components/CloudTermsNotice.test.ts create mode 100644 src/platform/cloud/onboarding/composables/usePostAuthRedirect.test.ts create mode 100644 src/schemas/signInSchema.test.ts diff --git a/src/components/dialog/content/signin/SignUpForm.test.ts b/src/components/dialog/content/signin/SignUpForm.test.ts index 48b7ba11521..80ae7c4021a 100644 --- a/src/components/dialog/content/signin/SignUpForm.test.ts +++ b/src/components/dialog/content/signin/SignUpForm.test.ts @@ -43,9 +43,6 @@ const mockReset = vi.fn() let emitTurnstileToken: ((token: string) => void) | undefined let emitTurnstileUnavailable: ((unavailable: boolean) => void) | undefined -// The reset-on-toggle behavior lives in useTurnstileGate itself (see -// useTurnstile.test.ts); this fake just wires token/unavailable through to -// `waiting` the same way so SignUpForm's submit gating can be exercised. vi.mock('@/composables/auth/useTurnstile', () => ({ useTurnstile: () => ({ enabled: mockTurnstileEnabled @@ -62,9 +59,8 @@ vi.mock('@/composables/auth/useTurnstile', () => ({ }) })) -// Stub the real widget (which loads the external Turnstile script) with one that -// exposes a spyable reset() and lets a test drive the v-model token/unavailable -// the way a solved challenge (or a broken/slow widget) would. +// The real widget loads an external Turnstile script; this stub exposes a +// spyable reset() and lets a test drive the token/unavailable v-models. vi.mock('./TurnstileWidget.vue', async () => { const { defineComponent: defineMock } = await import('vue') return { @@ -118,8 +114,6 @@ describe('SignUpForm', () => { return { ...utils, user } } - /** Render through a host that keeps a ref, so the parent-facing exposed - * `resetTurnstile()` can be invoked the way SignInContent would. */ function renderWithRef() { const formRef = ref<{ resetTurnstile: () => void } | null>(null) const Host = defineComponent({ @@ -246,11 +240,6 @@ describe('SignUpForm', () => { }) }) - // Regression coverage for the shadow-mode race: previously submit was only - // gated in 'enforce' mode, so most real signups in 'shadow' mode raced - // ahead of the async Cloudflare challenge and reached the backend with an - // empty token. Gating now depends only on whether the widget is enabled - // (shadow or enforce both render it), so both modes behave identically here. describe('Turnstile submit gating', () => { it('disables the submit button until a token is present', async () => { mockTurnstileEnabled.value = true @@ -268,7 +257,10 @@ describe('SignUpForm', () => { await user.click(screen.getByRole('button', { name: signUpButton })) - expect(onSubmit).not.toHaveBeenCalled() + expect( + onSubmit, + 'gating on enabled (not enforce) is what stops a shadow-mode signup racing ahead with an empty token' + ).not.toHaveBeenCalled() }) it('emits submit with the token once the challenge is solved', async () => { @@ -297,4 +289,66 @@ describe('SignUpForm', () => { expect(onSubmit).toHaveBeenCalledWith(expectedValues, undefined) }) }) + + describe('Turnstile wait hint accessibility', () => { + it('announces the wait politely while the challenge is pending', async () => { + mockTurnstileEnabled.value = true + renderComponent() + await nextTick() + + const hint = screen.getByRole('status') + expect( + hint, + 'the hint is the only thing telling a screen-reader user why submit is unavailable' + ).toHaveTextContent(enMessages.auth.turnstile.submitBlockedHint) + expect(hint).toHaveAttribute('aria-live', 'polite') + }) + + it('points the disabled submit button at the hint', async () => { + mockTurnstileEnabled.value = true + const { user } = renderComponent() + await fillValidSignup(user) + await nextTick() + + const submit = screen.getByRole('button', { name: signUpButton }) + expect( + submit, + 'an otherwise-valid form must stay disabled while the challenge is pending' + ).toBeDisabled() + expect(submit).toHaveAttribute( + 'aria-describedby', + screen.getByRole('status').id + ) + }) + + it('drops the description once the challenge resolves', async () => { + mockTurnstileEnabled.value = true + renderComponent() + await nextTick() + + emitTurnstileToken!('token-xyz') + await nextTick() + + expect( + screen.getByRole('button', { name: signUpButton }) + ).not.toHaveAttribute('aria-describedby') + }) + }) + + describe('double-submit throttling', () => { + it('emits once when the button is clicked twice in quick succession', async () => { + const onSubmit = vi.fn() + const { user } = renderComponent({ onSubmit }) + await fillValidSignup(user) + const submit = screen.getByRole('button', { name: signUpButton }) + + await user.click(submit) + await user.click(submit) + + expect( + onSubmit, + 'an impatient double-click would otherwise create the account twice' + ).toHaveBeenCalledOnce() + }) + }) }) diff --git a/src/platform/cloud/onboarding/CloudLoginView.test.ts b/src/platform/cloud/onboarding/CloudLoginView.test.ts index a566175c787..5e20f022296 100644 --- a/src/platform/cloud/onboarding/CloudLoginView.test.ts +++ b/src/platform/cloud/onboarding/CloudLoginView.test.ts @@ -27,14 +27,17 @@ const FREE_RUN_MESSAGES = { login: { cloudNewUser: 'New to Comfy?', cloudSignUp: 'Sign up here', - freeRunsSuffix: 'to get {count} free run. | to get {count} free runs.' + freeRunsSuffix: 'to get {count} free run. | to get {count} free runs.', + insecureContextWarning: 'This connection is insecure' } } } async function renderLoginView( url = '/cloud/login', - messages: Partial = {} + messages: { + auth?: { login?: Partial } + } = {} ) { const router = createRouter({ history: createMemoryHistory(), @@ -56,7 +59,6 @@ async function renderLoginView( createI18n({ legacy: false, locale: 'en', messages: { en: messages } }) ], stubs: { - Message: true, CloudSignInForm: { template: '
' } } } @@ -65,6 +67,7 @@ async function renderLoginView( afterEach(() => { isEmbeddedWebView.value = false + vi.unstubAllGlobals() }) describe('CloudLoginView', () => { @@ -107,6 +110,29 @@ describe('CloudLoginView', () => { ).not.toBeInTheDocument() }) + it.for([true, false])( + 'renders the insecure-context warning only over plain HTTP (secure: %s)', + async (secure: boolean) => { + vi.stubGlobal('isSecureContext', secure) + const { unmount } = await renderLoginView('/cloud/login', { + auth: { + login: { insecureContextWarning: 'This connection is insecure' } + } + }) + + const warning = screen.queryByText('This connection is insecure') + if (secure) { + expect(warning).not.toBeInTheDocument() + } else { + expect( + warning, + 'a self-hosted HTTP origin can have credentials intercepted, so the warning must render' + ).toBeInTheDocument() + } + unmount() + } + ) + it('does not region-gate sign-in, because an existing account already completed sign-up', async () => { const user = (await import('@testing-library/user-event')).default.setup() await renderLoginView() diff --git a/src/platform/cloud/onboarding/components/CloudSignInForm.test.ts b/src/platform/cloud/onboarding/components/CloudSignInForm.test.ts new file mode 100644 index 00000000000..08a1dca0c9c --- /dev/null +++ b/src/platform/cloud/onboarding/components/CloudSignInForm.test.ts @@ -0,0 +1,195 @@ +import userEvent from '@testing-library/user-event' +import { render, screen, waitFor } from '@testing-library/vue' +import PrimeVue from 'primevue/config' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createI18n } from 'vue-i18n' +import { createMemoryHistory, createRouter } from 'vue-router' + +import enMessages from '@/locales/en/main.json' with { type: 'json' } +import CloudSignInForm from '@/platform/cloud/onboarding/components/CloudSignInForm.vue' + +const loading = vi.hoisted(() => ({ value: false })) +vi.mock('@/stores/authStore', () => ({ + useAuthStore: () => ({ + get loading() { + return loading.value + } + }) +})) + +const LOGIN_COPY = enMessages.auth.login + +function renderForm( + authError?: string, + messages: Record = {} +) { + const router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: '/cloud/forgot-password', + name: 'cloud-forgot-password', + component: { template: '
' } + } + ] + }) + return render(CloudSignInForm, { + props: { authError }, + global: { + plugins: [ + router, + PrimeVue, + createI18n({ legacy: false, locale: 'en', messages: { en: messages } }) + ] + } + }) +} + +const renderRealForm = () => renderForm(undefined, enMessages) + +const emailField = () => + screen.getByPlaceholderText(LOGIN_COPY.emailPlaceholder) +const passwordField = () => + screen.getByPlaceholderText(LOGIN_COPY.passwordPlaceholder) +const submitButton = () => + screen.getByRole('button', { name: LOGIN_COPY.loginButton }) + +beforeEach(() => { + loading.value = false +}) + +describe('CloudSignInForm', () => { + it('renders the auth error inline', () => { + renderForm('The password you entered is incorrect.') + + expect( + screen.getByText('The password you entered is incorrect.') + ).toBeInTheDocument() + }) + + it('renders nothing when there is no auth error', () => { + renderForm() + + expect( + screen.queryByText('The password you entered is incorrect.') + ).not.toBeInTheDocument() + }) +}) + +describe('CloudSignInForm password manager support', () => { + it('marks the email field for autofill with a stable id', () => { + renderRealForm() + + expect(emailField()).toHaveAttribute('id', 'cloud-sign-in-email') + expect(emailField()).toHaveAttribute('name', 'email') + expect(emailField()).toHaveAttribute('autocomplete', 'email') + }) + + it('marks the password field as current-password, not new-password', () => { + renderRealForm() + + expect(passwordField()).toHaveAttribute('id', 'cloud-sign-in-password') + expect(passwordField()).toHaveAttribute('autocomplete', 'current-password') + }) + + it('binds both labels to their inputs', () => { + renderRealForm() + + expect(screen.getByLabelText(LOGIN_COPY.emailLabel)).toBe(emailField()) + expect(screen.getByLabelText(LOGIN_COPY.passwordLabel)).toBe( + passwordField() + ) + }) +}) + +describe('CloudSignInForm submit gating', () => { + // PrimeVue leaves `$form.valid` undefined until a field is touched, so the + // pristine button is enabled by design and is not asserted here. + it('disables submit once a field is touched and invalid', async () => { + const user = userEvent.setup() + renderRealForm() + + await user.type(emailField(), 'not-an-email') + + await waitFor(() => { + expect(submitButton()).toBeDisabled() + }) + }) + + it('enables submit once both fields are filled', async () => { + const user = userEvent.setup() + renderRealForm() + + await user.type(emailField(), 'user@example.com') + await user.type(passwordField(), 'Password1!') + + await waitFor(() => { + expect(submitButton()).toBeEnabled() + }) + }) + + it('surfaces a field error for a malformed email', async () => { + const user = userEvent.setup() + renderRealForm() + + await user.type(emailField(), 'not-an-email') + await user.type(passwordField(), 'Password1!') + await user.click(submitButton()) + + await waitFor(() => { + expect( + screen.getByText(enMessages.validation.invalidEmail) + ).toBeInTheDocument() + }) + }) + + it('does not emit submit for a malformed email, by button or by Enter', async () => { + const user = userEvent.setup() + const { emitted } = renderRealForm() + + await user.type(emailField(), 'not-an-email') + await user.type(passwordField(), 'Password1!') + await user.click(submitButton()) + await user.type(passwordField(), '{Enter}') + + expect(emitted().submit).toBeUndefined() + }) + + it('submits on Enter from the password field', async () => { + const user = userEvent.setup() + const { emitted } = renderRealForm() + + await user.type(emailField(), 'user@example.com') + await user.type(passwordField(), 'Password1!{Enter}') + + await waitFor(() => { + expect( + emitted().submit, + 'a handler wired only to the button click would never fire for the Enter key most people submit with' + ).toBeTruthy() + }) + expect(emitted().submit[0]).toEqual([ + { email: 'user@example.com', password: 'Password1!' } + ]) + }) +}) + +describe('CloudSignInForm in-flight state', () => { + it('disables submit and marks it busy while an auth action runs', () => { + loading.value = true + renderRealForm() + + expect(submitButton()).toBeDisabled() + expect(submitButton()).toHaveAttribute('aria-busy', 'true') + }) + + it('does not emit submit while loading', async () => { + const user = userEvent.setup() + loading.value = true + const { emitted } = renderRealForm() + + await user.click(submitButton()) + + expect(emitted().submit).toBeUndefined() + }) +}) diff --git a/src/platform/cloud/onboarding/components/CloudTermsNotice.test.ts b/src/platform/cloud/onboarding/components/CloudTermsNotice.test.ts new file mode 100644 index 00000000000..53b45c8324c --- /dev/null +++ b/src/platform/cloud/onboarding/components/CloudTermsNotice.test.ts @@ -0,0 +1,104 @@ +import { render, screen } from '@testing-library/vue' +import { describe, expect, it } from 'vitest' +import { createI18n } from 'vue-i18n' + +import enMessages from '@/locales/en/main.json' with { type: 'json' } + +import CloudTermsNotice from './CloudTermsNotice.vue' + +function renderNotice(messages: Record) { + return render(CloudTermsNotice, { + global: { + plugins: [ + createI18n({ legacy: false, locale: 'en', messages: { en: messages } }) + ] + } + }) +} + +const LONG_LOCALE = { + auth: { + login: { + termsText: + 'Indem Sie auf „Weiter" oder „Registrieren" klicken, erklären Sie sich mit unseren', + termsLink: 'Nutzungsbedingungen', + andText: 'und der', + privacyLink: 'Datenschutzerklärung' + } + }, + cloudWaitlist_questionsText: 'Haben Sie Fragen? Kontaktieren Sie uns', + cloudWaitlist_contactLink: 'hier' +} + +describe('CloudTermsNotice', () => { + it('links to the terms of service and privacy policy', () => { + renderNotice(enMessages) + + expect( + screen.getByRole('link', { name: enMessages.auth.login.termsLink }) + ).toHaveAttribute('href', 'https://www.comfy.org/terms-of-service') + expect( + screen.getByRole('link', { name: enMessages.auth.login.privacyLink }) + ).toHaveAttribute('href', 'https://www.comfy.org/privacy-policy') + }) + + it('opens every outbound link safely in a new tab', () => { + renderNotice(enMessages) + + for (const name of [ + enMessages.auth.login.termsLink, + enMessages.auth.login.privacyLink, + enMessages.cloudWaitlist_contactLink + ]) { + const link = screen.getByRole('link', { name }) + expect(link).toHaveAttribute('target', '_blank') + expect( + link, + 'target=_blank without noopener hands the opened page a window.opener handle on this sign-in page' + ).toHaveAttribute('rel', 'noopener noreferrer') + } + }) + + it('offers a support contact link', () => { + renderNotice(enMessages) + + expect( + screen.getByRole('link', { name: enMessages.cloudWaitlist_contactLink }) + ).toHaveAttribute('href', 'https://support.comfy.org') + }) + + it('renders every fragment from the locale, not hard-coded copy', () => { + renderNotice(LONG_LOCALE) + + const inSentence = (needle: string) => + screen.getByText((_, element) => { + if (element?.tagName !== 'P') return false + return (element.textContent ?? '').replace(/\s+/g, ' ').includes(needle) + }) + + expect(inSentence(LONG_LOCALE.auth.login.termsText)).toBeInTheDocument() + expect(inSentence(LONG_LOCALE.auth.login.andText)).toBeInTheDocument() + expect( + screen.getByRole('link', { name: LONG_LOCALE.auth.login.termsLink }) + ).toBeInTheDocument() + expect( + screen.getByRole('link', { name: LONG_LOCALE.auth.login.privacyLink }) + ).toBeInTheDocument() + expect( + screen.queryByRole('link', { name: 'Privacy Policy' }) + ).not.toBeInTheDocument() + }) + + it('keeps the sentence punctuated once translated', () => { + renderNotice(LONG_LOCALE) + + expect( + screen.getByText((_, element) => { + if (element?.tagName !== 'P') return false + const text = (element.textContent ?? '').replace(/\s+/g, ' ') + return text.includes(`${LONG_LOCALE.auth.login.privacyLink}.`) + }), + 'the trailing period lives in the template, outside the four keys, so a reflow can drop it unnoticed' + ).toBeInTheDocument() + }) +}) diff --git a/src/platform/cloud/onboarding/composables/usePostAuthRedirect.test.ts b/src/platform/cloud/onboarding/composables/usePostAuthRedirect.test.ts new file mode 100644 index 00000000000..77a4df01b00 --- /dev/null +++ b/src/platform/cloud/onboarding/composables/usePostAuthRedirect.test.ts @@ -0,0 +1,164 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createApp, defineComponent, ref } from 'vue' +import { createI18n } from 'vue-i18n' + +import { usePostAuthRedirect } from '@/platform/cloud/onboarding/composables/usePostAuthRedirect' + +const query = vi.hoisted(() => ({ value: {} as Record })) +const replace = vi.hoisted(() => vi.fn()) +const push = vi.hoisted(() => vi.fn()) +vi.mock('vue-router', () => ({ + useRouter: () => ({ replace, push }), + useRoute: () => ({ query: query.value }) +})) + +const resumeOAuthIfNeeded = vi.hoisted(() => + vi.fn().mockResolvedValue({ kind: 'no-oauth' }) +) +vi.mock('@/platform/cloud/oauth/useOAuthPostLoginRedirect', () => ({ + useOAuthPostLoginRedirect: () => ({ resumeOAuthIfNeeded }) +})) + +const toasts = vi.hoisted(() => ({ add: vi.fn() })) +vi.mock('@/platform/updates/common/toastStore', () => ({ + useToastStore: () => toasts +})) + +const DEFAULT_REDIRECT = { name: 'cloud-user-check' } + +function setup() { + const authError = ref('') + let onAuthSuccess: (() => Promise) | undefined + + const app = createApp( + defineComponent({ + setup() { + ;({ onAuthSuccess } = usePostAuthRedirect({ + authError, + successSummary: 'Login Completed', + defaultRedirect: () => DEFAULT_REDIRECT + })) + return () => null + } + }) + ) + app.use(createI18n({ legacy: false, locale: 'en', messages: { en: {} } })) + app.mount(document.createElement('div')) + + if (!onAuthSuccess) throw new Error('post-auth redirect not initialized') + + return { authError, onAuthSuccess, unmount: () => app.unmount() } +} + +beforeEach(() => { + query.value = {} + resumeOAuthIfNeeded.mockResolvedValue({ kind: 'no-oauth' }) +}) + +describe('usePostAuthRedirect', () => { + it('sends a plain sign-in to the default destination', async () => { + const { onAuthSuccess } = setup() + + await onAuthSuccess() + + expect(push).toHaveBeenCalledWith(DEFAULT_REDIRECT) + expect(replace).not.toHaveBeenCalled() + }) + + it('confirms the sign-in with a success toast', async () => { + const { onAuthSuccess } = setup() + + await onAuthSuccess() + + expect(toasts.add).toHaveBeenCalledWith( + expect.objectContaining({ + severity: 'success', + summary: 'Login Completed' + }) + ) + }) + + it('returns a deep-linked user to where they were headed', async () => { + query.value = { previousFullPath: encodeURIComponent('/some/path?x=1') } + const { onAuthSuccess } = setup() + + await onAuthSuccess() + + expect( + replace, + 'push would leave the login page in the back stack, where Back bounces the signed-in user into the guard again' + ).toHaveBeenCalledWith('/some/path?x=1') + expect(push).not.toHaveBeenCalled() + }) + + it('ignores an off-site previousFullPath and uses the default', async () => { + query.value = { previousFullPath: encodeURIComponent('https://evil.com') } + const { onAuthSuccess } = setup() + + await onAuthSuccess() + + expect(replace).not.toHaveBeenCalled() + expect(push).toHaveBeenCalledWith(DEFAULT_REDIRECT) + }) + + it('lets an OAuth resume outrank a deep link', async () => { + query.value = { previousFullPath: encodeURIComponent('/some/path') } + resumeOAuthIfNeeded.mockResolvedValue({ kind: 'resumed' }) + const { onAuthSuccess } = setup() + + await onAuthSuccess() + + expect( + replace, + 'the OAuth handshake owns the navigation, otherwise the client app that started it never gets its consent screen' + ).not.toHaveBeenCalled() + expect(push).not.toHaveBeenCalled() + }) + + it('surfaces an OAuth resume failure and navigates nowhere', async () => { + query.value = { previousFullPath: encodeURIComponent('/some/path') } + resumeOAuthIfNeeded.mockResolvedValue({ + kind: 'error', + message: 'Session expired' + }) + const { authError, onAuthSuccess } = setup() + + await onAuthSuccess() + + expect(authError.value).toBe('Session expired') + expect(replace).not.toHaveBeenCalled() + expect(push).not.toHaveBeenCalled() + }) + + it('also toasts an OAuth resume failure, for social sign-in', async () => { + resumeOAuthIfNeeded.mockResolvedValue({ + kind: 'error', + message: 'Session expired' + }) + const { onAuthSuccess } = setup() + + await onAuthSuccess() + + expect( + toasts.add, + 'authError only renders in email-form mode, so a Google/GitHub user would see the failure nowhere at all' + ).toHaveBeenCalledWith( + expect.objectContaining({ + severity: 'error', + summary: 'oauth.consent.sessionErrorToastSummary', + detail: 'Session expired' + }) + ) + }) + + it('passes the live query to the OAuth resume check', async () => { + query.value = { oauth_request_id: 'abc' } + const { onAuthSuccess } = setup() + + await onAuthSuccess() + + expect(resumeOAuthIfNeeded).toHaveBeenCalledWith({ + oauth_request_id: 'abc' + }) + }) +}) diff --git a/src/platform/cloud/onboarding/onboardingCloudRoutes.test.ts b/src/platform/cloud/onboarding/onboardingCloudRoutes.test.ts index a17422f6848..22504e97b86 100644 --- a/src/platform/cloud/onboarding/onboardingCloudRoutes.test.ts +++ b/src/platform/cloud/onboarding/onboardingCloudRoutes.test.ts @@ -41,17 +41,9 @@ const layoutLoader = oauthLayout?.component const consentLoader = consentRoute?.component /** - * Resolved here rather than inside the test. - * - * These are the real loaders, so calling them compiles `OAuthLayoutView.vue`, - * `OAuthConsentView.vue` and everything they import — seconds of work even on - * an idle machine. Awaited inside a test body that time is billed against the - * 5 s test timeout, which is what made this test fail under a loaded worker - * pool while passing in isolation (#14666). At module scope it is collection - * cost, which nothing times out, and the test itself becomes synchronous. - * - * The loaders are still the ones the router will call: a route pointing at a - * module that does not exist still fails here, at import time. + * At module scope, not in the test body: these real loaders compile the views + * and everything they import, and inside `it()` that is billed against the 5 s + * test timeout (#14666). Collection time is untimed. */ const resolvedComponents = await Promise.all( [layoutLoader, consentLoader].map(async (loader) => @@ -259,14 +251,14 @@ describe('oauthConsentRedirect', () => { }) it('mints the Cloud session cookie before redirecting to consent when resuming OAuth', async () => { - // Regression: an already-signed-in user (Firebase) carries no Cloud session - // cookie, so the consent challenge fetch fails unless the cookie is minted - // here, mirroring the post-login resume path. captureOAuthRequestId({ oauth_request_id: VALID_REQUEST_ID }) const target = await oauthConsentRedirect() - expect(createSessionOrThrow).toHaveBeenCalledOnce() + expect( + createSessionOrThrow, + 'an already-signed-in Firebase user carries no Cloud session cookie, so the consent challenge fetch fails without this' + ).toHaveBeenCalledOnce() expect(target).toEqual({ name: 'cloud-oauth-consent', query: { oauth_request_id: VALID_REQUEST_ID } @@ -294,3 +286,120 @@ describe('oauthConsentRedirect', () => { } }) }) + +const cloudLayout = cloudOnboardingRoutes.find((r) => r.path === '/cloud') + +const guardedRoutes = ['cloud-login', 'cloud-signup'].map((name) => { + const route = cloudLayout?.children?.find((c) => c.name === name) + if (Array.isArray(route?.beforeEnter)) { + throw new Error( + `${name} now has an array of beforeEnter guards; runGuard drives only one` + ) + } + if (typeof route?.beforeEnter !== 'function') { + throw new Error(`${name} has no beforeEnter guard`) + } + return [name, route.beforeEnter, `/cloud/${route.path}`] as const +}) + +type GuardedRoute = (typeof guardedRoutes)[number] + +async function runGuard( + [name, guard, path]: GuardedRoute, + query: Record +) { + const next = vi.fn() + const to = { query, name, path } + await ( + guard as unknown as ( + to: unknown, + from: unknown, + next: unknown + ) => Promise + )(to, undefined, next) + return next.mock.calls[0]?.[0] +} + +describe.for(guardedRoutes)('%s beforeEnter', (route) => { + beforeEach(() => { + isLoggedIn.value = false + clearOAuthRequestId() + createSessionOrThrow.mockReset().mockResolvedValue(undefined) + }) + + it('lets a signed-out visitor through to the form', async () => { + expect(await runGuard(route, {})).toBeUndefined() + }) + + it('redirects a signed-in visitor away from the auth page', async () => { + isLoggedIn.value = true + + expect(await runGuard(route, {})).toEqual({ name: 'cloud-user-check' }) + }) + + it('sends a signed-in visitor straight to consent mid-OAuth', async () => { + isLoggedIn.value = true + captureOAuthRequestId({ oauth_request_id: VALID_REQUEST_ID }) + + expect(await runGuard(route, {})).toEqual({ + name: 'cloud-oauth-consent', + query: { oauth_request_id: VALID_REQUEST_ID } + }) + expect( + createSessionOrThrow, + 'routing to consent without the session cookie leaves the view making an unauthenticated request' + ).toHaveBeenCalledOnce() + }) + + it('honours ?switchAccount for a signed-in visitor', async () => { + isLoggedIn.value = true + + expect( + await runGuard(route, { switchAccount: '1' }), + 'without this escape hatch a signed-in user can never reach the form to switch accounts' + ).toBeUndefined() + }) + + it('does not mint a session cookie when it lets the visitor through', async () => { + isLoggedIn.value = true + + await runGuard(route, { switchAccount: '1' }) + + expect(createSessionOrThrow).not.toHaveBeenCalled() + }) +}) + +describe('legacy /cloud/oauth/consent redirect', () => { + const legacyRoute = cloudOnboardingRoutes.find( + (r) => r.path === '/cloud/oauth/consent' + ) + + it('preserves the query the backend 302s with', () => { + const redirect = legacyRoute?.redirect + if (typeof redirect !== 'function') { + throw new Error('legacy consent route has no redirect function') + } + + const to = { + name: undefined, + path: '/cloud/oauth/consent', + fullPath: `/cloud/oauth/consent?oauth_request_id=${VALID_REQUEST_ID}`, + query: { oauth_request_id: VALID_REQUEST_ID }, + hash: '', + params: {}, + matched: [], + meta: {}, + redirectedFrom: undefined + } + + const target = redirect(to, to) + + expect( + target, + 'the backend still 302s to the old path, and dropping the query strands the consent view with no request to consent to' + ).toEqual({ + path: '/oauth/consent', + query: { oauth_request_id: VALID_REQUEST_ID } + }) + }) +}) diff --git a/src/platform/cloud/onboarding/utils/previousFullPath.test.ts b/src/platform/cloud/onboarding/utils/previousFullPath.test.ts index d74c1e609a3..782be0e5984 100644 --- a/src/platform/cloud/onboarding/utils/previousFullPath.test.ts +++ b/src/platform/cloud/onboarding/utils/previousFullPath.test.ts @@ -29,6 +29,29 @@ describe('getSafePreviousFullPath', () => { expect(getSafePreviousFullPath(query)).toBeNull() }) + test('takes the first entry when the param is repeated', () => { + const query: LocationQuery = { + previousFullPath: [ + encodeURIComponent('/first'), + encodeURIComponent('//evil.com') + ] + } + expect( + getSafePreviousFullPath(query), + 'reading the last entry would let an attacker override a legitimate value by appending a second one' + ).toBe('/first') + }) + + test('rejects a repeated param whose first entry is unsafe', () => { + const query: LocationQuery = { + previousFullPath: [ + encodeURIComponent('https://evil.com'), + encodeURIComponent('/safe') + ] + } + expect(getSafePreviousFullPath(query)).toBeNull() + }) + test('rejects malformed encodings', () => { const query: LocationQuery = { previousFullPath: '%E0%A4%A' diff --git a/src/schemas/signInSchema.test.ts b/src/schemas/signInSchema.test.ts new file mode 100644 index 00000000000..282a5b47355 --- /dev/null +++ b/src/schemas/signInSchema.test.ts @@ -0,0 +1,139 @@ +import { describe, expect, it } from 'vitest' +import type { SafeParseReturnType } from 'zod' + +import { + signInSchema, + signUpSchema, + updatePasswordSchema +} from '@/schemas/signInSchema' + +const VALID_PASSWORD = 'Password1!' + +const signUpValues = (overrides: Partial> = {}) => ({ + email: 'user@example.com', + password: VALID_PASSWORD, + confirmPassword: VALID_PASSWORD, + ...overrides +}) + +const errorAt = ( + result: SafeParseReturnType, + path: string +) => + result.success + ? undefined + : result.error.issues.find((issue) => issue.path[0] === path)?.message + +describe('signInSchema', () => { + it('accepts an email and any non-empty password', () => { + expect( + signInSchema.safeParse({ email: 'a@b.co', password: 'x' }).success, + 'sign-in must not apply the sign-up complexity rules, or accounts created before those rules can never log in' + ).toBe(true) + }) + + it.for([ + ['missing @', 'nope'], + ['missing domain', 'user@'], + ['empty', ''] + ])('rejects %s as an email', ([, email]) => { + expect( + signInSchema.safeParse({ email, password: VALID_PASSWORD }).success + ).toBe(false) + }) + + it('rejects an empty password', () => { + expect( + signInSchema.safeParse({ email: 'a@b.co', password: '' }).success + ).toBe(false) + }) +}) + +describe('signUpSchema password length boundaries', () => { + it.for([ + ['7 chars is too short', 'Pas1!aa', false], + ['8 chars is the minimum', 'Pass1!aa', true], + ['32 chars is the maximum', `Pass1!${'a'.repeat(26)}`, true], + ['33 chars is too long', `Pass1!${'a'.repeat(27)}`, false] + ] as const)('%s', ([, password, expected]) => { + const result = signUpSchema.safeParse( + signUpValues({ password, confirmPassword: password }) + ) + + expect( + result.success, + 'these are the inclusive edges of the 8..32 rule, so a bound that slips by one shows up here and nowhere else' + ).toBe(expected) + }) +}) + +describe('signUpSchema character classes', () => { + it.for([ + ['uppercase', 'password1!'], + ['lowercase', 'PASSWORD1!'], + ['number', 'Password!!'], + ['special', 'Password12'] + ])('requires at least one %s character', ([, password]) => { + const result = signUpSchema.safeParse( + signUpValues({ password, confirmPassword: password }) + ) + + expect(result.success).toBe(false) + expect(errorAt(result, 'password')).toBeDefined() + }) + + it('accepts a password satisfying all four classes', () => { + expect(signUpSchema.safeParse(signUpValues()).success).toBe(true) + }) +}) + +describe('signUpSchema confirmPassword', () => { + it('reports a mismatch against the confirmPassword field', () => { + const result = signUpSchema.safeParse( + signUpValues({ confirmPassword: 'Password2!' }) + ) + + expect(result.success).toBe(false) + expect( + errorAt(result, 'confirmPassword'), + 'the refine must target confirmPassword, or the error renders under the wrong input' + ).toBeDefined() + expect(errorAt(result, 'password')).toBeUndefined() + }) + + it('rejects an empty confirmPassword', () => { + expect( + signUpSchema.safeParse(signUpValues({ confirmPassword: '' })).success + ).toBe(false) + }) +}) + +describe('updatePasswordSchema', () => { + it('accepts a valid matching pair', () => { + expect( + updatePasswordSchema.safeParse({ + password: VALID_PASSWORD, + confirmPassword: VALID_PASSWORD + }).success + ).toBe(true) + }) + + it('applies the same complexity rules as sign-up', () => { + expect( + updatePasswordSchema.safeParse({ + password: 'password', + confirmPassword: 'password' + }).success + ).toBe(false) + }) + + it('reports a mismatch against the confirmPassword field', () => { + const result = updatePasswordSchema.safeParse({ + password: VALID_PASSWORD, + confirmPassword: 'Password2!' + }) + + expect(result.success).toBe(false) + expect(errorAt(result, 'confirmPassword')).toBeDefined() + }) +}) From b09bd9e7ba3cfbf735fcd9d50bf9698fd789695b Mon Sep 17 00:00:00 2001 From: Lauri Gates Date: Tue, 18 Aug 2026 16:56:15 +0000 Subject: [PATCH 2/5] fix: preserve asset subfolder in the assets lightbox (#14689) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Inspecting a video saved under a subfolder shows an empty player, because the assets lightbox builds its media URL from a subfolder the sidebar discarded. ## Changes - **What**: `AssetsSidebarTab` built every gallery `ResultItemImpl` with `subfolder: ''` and overrode only the `url` getter. Images resolve through `preview_url` and were unaffected; `ResultVideo` uses `vhsAdvancedPreviewUrl`, which is rebuilt from `urlParams` — i.e. from the discarded subfolder. Extracted `getAssetSubfolder` (reads `preview_url`, falls back to `user_metadata`, mirroring how `getAssetType` resolves the type) and used it when building gallery items and in `getAssetUrl`. Measured against a local backend — same file, same name, only the address differs: | Request | Response | | --- | --- | | `viewvideo?filename=clip.webm&type=output&subfolder=` (file at output root) | 200, 910017 b | | `viewvideo?filename=clip.webm&type=output&subfolder=sub` (file in `sub/`) | 200, 910017 b | | `viewvideo?filename=clip.webm&type=output&subfolder=` (file in `sub/`) | **204, 0 b** | The endpoint handles subfolders correctly; only the URL the lightbox constructed failed. This also matches the 204 independently reported in #7192, and explains why the same videos play from Job History (which builds result items from real queue data) and why the bug only appears with a `filename_prefix` containing a directory. ## Review Focus `getAssetSubfolder` prefers `preview_url` over `user_metadata` because `preview_url` is already the trusted source for the `url` getter and for `getAssetType`. `getAssetUrl` previously read `user_metadata` only; that path is preserved as the fallback, so its behaviour is unchanged when `preview_url` carries no subfolder. Not addressed here, to keep this focused: the same constructor hard-codes `type: 'output'`, which is wrong for imported (input) videos. `ResultItem['type']` is a narrow union while `getAssetType` returns `string`, so that needs its own change. Every case reported in #7192 is an output. Fixes #7192 --- .../sidebar/tabs/AssetsSidebarTab.vue | 7 +- .../assets/utils/assetUrlUtil.test.ts | 79 +++++++++++++++++++ src/platform/assets/utils/assetUrlUtil.ts | 23 +++++- 3 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 src/platform/assets/utils/assetUrlUtil.test.ts diff --git a/src/components/sidebar/tabs/AssetsSidebarTab.vue b/src/components/sidebar/tabs/AssetsSidebarTab.vue index 7232e413053..c9fcb561337 100644 --- a/src/components/sidebar/tabs/AssetsSidebarTab.vue +++ b/src/components/sidebar/tabs/AssetsSidebarTab.vue @@ -229,7 +229,10 @@ import type { OutputAssetMetadata } from '@/platform/assets/schemas/assetMetadat import { getOutputAssetMetadata } from '@/platform/assets/schemas/assetMetadataSchema' import type { AssetItem } from '@/platform/assets/schemas/assetSchema' import { getAssetDisplayName } from '@/platform/assets/utils/assetMetadataUtils' -import { getAssetUrl } from '@/platform/assets/utils/assetUrlUtil' +import { + getAssetSubfolder, + getAssetUrl +} from '@/platform/assets/utils/assetUrlUtil' import type { MediaKind } from '@/platform/assets/schemas/mediaAssetSchema' import { resolveOutputAssetItems } from '@/platform/assets/utils/outputAssetUtil' import { isCloud } from '@/platform/distribution/types' @@ -457,7 +460,7 @@ const galleryItems = computed(() => { const mediaType = getMediaTypeFromFilename(asset.name) const resultItem = new ResultItemImpl({ filename: asset.name, - subfolder: '', + subfolder: getAssetSubfolder(asset), type: 'output', nodeId: '0', mediaType: mediaType === 'image' ? 'images' : mediaType diff --git a/src/platform/assets/utils/assetUrlUtil.test.ts b/src/platform/assets/utils/assetUrlUtil.test.ts new file mode 100644 index 00000000000..b6ec0b932b2 --- /dev/null +++ b/src/platform/assets/utils/assetUrlUtil.test.ts @@ -0,0 +1,79 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import type { AssetItem } from '@/platform/assets/schemas/assetSchema' +import { + getAssetSubfolder, + getAssetUrl +} from '@/platform/assets/utils/assetUrlUtil' + +const mockApiURL = vi.hoisted(() => + vi.fn((path: string) => `http://localhost:8188/api${path}`) +) + +vi.mock('@/scripts/api', () => ({ + api: { apiURL: mockApiURL } +})) + +function createAsset(overrides: Partial = {}): AssetItem { + return { + id: 'asset-1', + name: 'clip.webm', + tags: ['output'], + ...overrides + } as AssetItem +} + +describe('getAssetSubfolder', () => { + beforeEach(() => vi.clearAllMocks()) + + it('reads the subfolder from preview_url', () => { + const asset = createAsset({ + preview_url: '/api/view?filename=clip.webm&type=output&subfolder=vid/2026' + }) + + expect(getAssetSubfolder(asset)).toBe('vid/2026') + }) + + it('falls back to user_metadata when preview_url carries no subfolder', () => { + const asset = createAsset({ + preview_url: '/api/view?filename=clip.webm&type=output', + user_metadata: { subfolder: 'vid/2026' } + }) + + expect(getAssetSubfolder(asset)).toBe('vid/2026') + }) + + it('returns an empty string for an asset at the type root', () => { + expect(getAssetSubfolder(createAsset())).toBe('') + expect( + getAssetSubfolder( + createAsset({ user_metadata: { subfolder: undefined } }) + ) + ).toBe('') + }) +}) + +describe('getAssetUrl', () => { + beforeEach(() => vi.clearAllMocks()) + + it('includes the subfolder carried by preview_url', () => { + const asset = createAsset({ + preview_url: '/api/view?filename=clip.webm&type=output&subfolder=vid/2026' + }) + + expect(getAssetUrl(asset)).toContain('subfolder=vid%2F2026') + }) + + it('includes the subfolder taken from the user_metadata fallback', () => { + const asset = createAsset({ + preview_url: '/api/view?filename=clip.webm&type=output', + user_metadata: { subfolder: 'vid/2026' } + }) + + expect(getAssetUrl(asset)).toContain('subfolder=vid%2F2026') + }) + + it('omits the subfolder param for an asset at the type root', () => { + expect(getAssetUrl(createAsset())).not.toContain('subfolder') + }) +}) diff --git a/src/platform/assets/utils/assetUrlUtil.ts b/src/platform/assets/utils/assetUrlUtil.ts index ad0c2be25f8..ab6d4218b6d 100644 --- a/src/platform/assets/utils/assetUrlUtil.ts +++ b/src/platform/assets/utils/assetUrlUtil.ts @@ -23,12 +23,31 @@ export function getAssetUrl( defaultType: 'input' | 'output' = 'output' ): string { const assetType = getAssetType(asset, defaultType) - const subfolder = asset.user_metadata?.subfolder + const subfolder = getAssetSubfolder(asset) const params = new URLSearchParams() params.set('filename', asset.name) params.set('type', assetType) - if (typeof subfolder === 'string' && subfolder) { + if (subfolder) { params.set('subfolder', subfolder) } return api.apiURL(`/view?${params}`) } + +/** + * Get the subfolder an asset lives in, relative to its type root + * + * Reads `preview_url` first and falls back to `user_metadata`, mirroring how + * {@link getAssetType} resolves the type. + * + * @param asset The asset to get the subfolder for + * @returns The subfolder, or an empty string when the asset is at the root + */ +export function getAssetSubfolder(asset: AssetItem): string { + const previewSubfolder = new URLSearchParams( + (asset.preview_url ?? '').split('?')[1] ?? '' + ).get('subfolder') + if (previewSubfolder) return previewSubfolder + + const { subfolder } = asset.user_metadata ?? {} + return typeof subfolder === 'string' ? subfolder : '' +} From 8f221cde7c28883960ec48464274fd3063d72af0 Mon Sep 17 00:00:00 2001 From: imick-io <153135517+imick-io@users.noreply.github.com> Date: Tue, 18 Aug 2026 17:48:55 +0000 Subject: [PATCH 3/5] fix(website): bump FDCT technologist avatar image versions (#15402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Bump the avatar image versions for four technologists on the Forward Deployed Creatives (FDCT) page to their latest uploaded assets. ## Changes - **What**: Updated `avatarSrc` for Doug Hogan (`_v2`), Chris V. (`_v2`), Rob Losch (`_v3`), and Robert Paige (`_v4`) in `apps/website/src/data/fdct.ts`. ## Review Focus Verify each new asset URL resolves on media.comfy.org. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) --- apps/website/src/data/fdct.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/website/src/data/fdct.ts b/apps/website/src/data/fdct.ts index 3c048650af1..d2d12592782 100644 --- a/apps/website/src/data/fdct.ts +++ b/apps/website/src/data/fdct.ts @@ -24,20 +24,20 @@ export interface FdctTechnologist { const technologistIdentities = { 'doug-hogan': { name: 'Doug Hogan', - avatarSrc: 'https://media.comfy.org/website/technologists/doug-hogan.png' + avatarSrc: 'https://media.comfy.org/website/technologists/doug-hogan_v2.png' }, 'chris-v': { name: 'Chris V.', - avatarSrc: 'https://media.comfy.org/website/technologists/chris-v.png' + avatarSrc: 'https://media.comfy.org/website/technologists/chris-v_v2.png' }, 'rob-losch': { name: 'Rob Losch', - avatarSrc: 'https://media.comfy.org/website/technologists/rob-losch_v2.png' + avatarSrc: 'https://media.comfy.org/website/technologists/rob-losch_v3.png' }, 'robert-paige': { name: 'Robert Paige', avatarSrc: - 'https://media.comfy.org/website/technologists/robert-paige_v3.png' + 'https://media.comfy.org/website/technologists/robert-paige_v4.png' } } as const From a8483a8ad6b0f06614cb4b4ee01b48e8d606f2df Mon Sep 17 00:00:00 2001 From: Kevin Mastascusa <83515571+kevinmastascusa@users.noreply.github.com> Date: Tue, 18 Aug 2026 17:49:39 +0000 Subject: [PATCH 4/5] feat: snap group borders to the grid when fitting to nodes (#15070) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fitting a group to its nodes now leaves all four group borders on the grid when snap-to-grid is enabled, instead of landing at arbitrary sub-grid offsets. ## Changes - **What**: `LGraphGroup.resizeTo` expands the fitted bounds out to the nearest grid lines when `LiteGraph.alwaysSnapToGrid` is set. The expansion lives in a new pure helper, `expandRectToGrid` in `measure.ts`, alongside `snapPoint`. ## Review Focus The rect is only ever grown, never shrunk — left/top floor, right/bottom ceil — so the requested padding is never eaten and nothing that was inside the group before the call ends up outside it. Rounding to nearest would have been closer to `snapPoint`'s existing behaviour but can pull a border inside the padding it just added. Gated on `alwaysSnapToGrid` rather than on `getSnapToGridSize()` alone: that getter returns `CANVAS_GRID_SIZE` regardless of the setting, so using it by itself would snap groups for users who have snap-to-grid switched off. Applies to every fit path, since all of them funnel through `resizeTo` — context menu, more-options menu, `useCoreCommands`, and `useFrameNodes`. Fixes #1185 --- src/lib/litegraph/src/LGraphGroup.test.ts | 48 ++++++++++++++++++++++- src/lib/litegraph/src/LGraphGroup.ts | 9 +++++ src/lib/litegraph/src/measure.test.ts | 19 +++++++++ src/lib/litegraph/src/measure.ts | 25 ++++++++++++ 4 files changed, 99 insertions(+), 2 deletions(-) diff --git a/src/lib/litegraph/src/LGraphGroup.test.ts b/src/lib/litegraph/src/LGraphGroup.test.ts index 64d89079919..498936d160c 100644 --- a/src/lib/litegraph/src/LGraphGroup.test.ts +++ b/src/lib/litegraph/src/LGraphGroup.test.ts @@ -1,7 +1,8 @@ -import { describe, expect, vi } from 'vitest' +import { afterEach, describe, expect, vi } from 'vitest' import type { LGraphCanvas } from '@/lib/litegraph/src/litegraph' -import { LGraph, LGraphGroup } from '@/lib/litegraph/src/litegraph' +import { LGraph, LGraphGroup, LiteGraph } from '@/lib/litegraph/src/litegraph' +import { containsRect } from '@/lib/litegraph/src/measure' import * as colorUtil from '@/utils/colorUtil' import { test } from './__fixtures__/testExtensions' @@ -105,6 +106,49 @@ describe('LGraphGroup', () => { }) }) + describe('resizeTo', () => { + const alwaysSnapToGrid = LiteGraph.alwaysSnapToGrid + const gridSize = LiteGraph.CANVAS_GRID_SIZE + + afterEach(() => { + LiteGraph.alwaysSnapToGrid = alwaysSnapToGrid + LiteGraph.CANVAS_GRID_SIZE = gridSize + }) + + function createGroupFittedToContent() { + const graph = new LGraph() + const group = new LGraphGroup('group') + graph.add(group) + + const content = new LGraphGroup('content') + content.pos = [103, 207] + content.size = [140, 80] + + group.resizeTo([content], 10) + return { group, content } + } + + test('fits the group around its contents with padding', () => { + LiteGraph.alwaysSnapToGrid = false + const { group } = createGroupFittedToContent() + + expect([...group.pos]).toEqual([93, 197 - group.titleHeight]) + expect([...group.size]).toEqual([160, 100 + group.titleHeight]) + }) + + test('expands every border to the grid when always snapping', () => { + LiteGraph.alwaysSnapToGrid = true + LiteGraph.CANVAS_GRID_SIZE = 10 + const { group, content } = createGroupFittedToContent() + + const [x, y, width, height] = group.boundingRect + expect([x, y, x + width, y + height].map((edge) => edge % 10)).toEqual([ + 0, 0, 0, 0 + ]) + expect(containsRect(group.boundingRect, content.boundingRect)).toBe(true) + }) + }) + describe('draw', () => { test('lightens the title text for a very dark background', () => { const group = new LGraphGroup('Group') diff --git a/src/lib/litegraph/src/LGraphGroup.ts b/src/lib/litegraph/src/LGraphGroup.ts index 501d004cecf..d9972d8a916 100644 --- a/src/lib/litegraph/src/LGraphGroup.ts +++ b/src/lib/litegraph/src/LGraphGroup.ts @@ -19,6 +19,7 @@ import { containsCentre, containsRect, createBounds, + expandRectToGrid, isInRect, isInRectangle, isPointInRect, @@ -325,6 +326,9 @@ export class LGraphGroup implements Positionable, IPinnable, IColorable { /** * Resizes and moves the group to neatly fit all given {@link objects}. + * + * When {@link LiteGraph.alwaysSnapToGrid} is enabled, the group is then + * expanded so that all four of its borders line up with the grid. * @param objects All objects that should be inside the group * @param padding Value in graph units to add to all sides of the group. Default: 10 */ @@ -336,6 +340,11 @@ export class LGraphGroup implements Positionable, IPinnable, IColorable { this.pos[1] = boundingBox[1] - this.titleHeight this.size[0] = boundingBox[2] this.size[1] = boundingBox[3] + this.titleHeight + + const snapTo = LiteGraph.alwaysSnapToGrid + ? this.graph?.getSnapToGridSize() + : undefined + if (snapTo) expandRectToGrid(this._bounding, snapTo) } /** diff --git a/src/lib/litegraph/src/measure.test.ts b/src/lib/litegraph/src/measure.test.ts index f642c0c7a9e..7cbb01d23ed 100644 --- a/src/lib/litegraph/src/measure.test.ts +++ b/src/lib/litegraph/src/measure.test.ts @@ -9,6 +9,7 @@ import { createBounds, dist2, distance, + expandRectToGrid, findPointOnCurve, getOrientation, isInRect, @@ -144,6 +145,24 @@ test('snapPoint correctly snaps points to grid using ceil', ({ expect }) => { expect(point3).toEqual([20, -10]) }) +test('expandRectToGrid grows every edge out to the grid', ({ expect }) => { + const rect: Rect = [12.3, 18.7, 20, 20] + expect(expandRectToGrid(rect, 10)).toBe(true) + expect(rect).toEqual([10, 10, 30, 30]) + + const alreadyAligned: Rect = [10, 20, 30, 40] + expect(expandRectToGrid(alreadyAligned, 10)).toBe(true) + expect(alreadyAligned).toEqual([10, 20, 30, 40]) + + const negative: Rect = [-12.3, -18.7, 5, 5] + expect(expandRectToGrid(negative, 10)).toBe(true) + expect(negative).toEqual([-20, -20, 20, 10]) + + const unsnapped: Rect = [12.3, 18.7, 20, 20] + expect(expandRectToGrid(unsnapped, 0)).toBe(false) + expect(unsnapped).toEqual([12.3, 18.7, 20, 20]) +}) + test('snapPoint correctly snaps points to grid using floor', ({ expect }) => { const point: Point = [12.3, 18.7] expect(snapPoint(point, 5, 'floor')).toBe(true) diff --git a/src/lib/litegraph/src/measure.ts b/src/lib/litegraph/src/measure.ts index 08ce8a50bba..2242006d35e 100644 --- a/src/lib/litegraph/src/measure.ts +++ b/src/lib/litegraph/src/measure.ts @@ -363,6 +363,31 @@ export function snapPoint( return true } +/** + * Expands a {@link Rect} outwards so that all four edges lie on a grid of size + * {@link snapTo}. + * + * Unlike {@link snapPoint}, the rect is only ever grown, never shrunk, so + * anything it contained before the call is still contained afterwards. + * @param rect The rect that will be expanded, modified in place + * @param snapTo The grid size to expand out to (multiples thereof) + * @returns `true` if snapTo is truthy, otherwise `false` + */ +export function expandRectToGrid(rect: Rect, snapTo: number): boolean { + if (!snapTo) return false + + const right = snapTo * Math.ceil((rect[0] + rect[2]) / snapTo) + const bottom = snapTo * Math.ceil((rect[1] + rect[3]) / snapTo) + const left = snapTo * Math.floor(rect[0] / snapTo) + const top = snapTo * Math.floor(rect[1] / snapTo) + + rect[0] = left + rect[1] = top + rect[2] = right - left + rect[3] = bottom - top + return true +} + /** * Aligns a {@link Rect} relative to the edges or centre of a {@link container} rectangle. * From e5da708f1b16f52e00a37c9b2e762f2c31e06db1 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Tue, 18 Aug 2026 20:35:46 +0000 Subject: [PATCH 5/5] docs: write down how to model a feature's state, and lint the checkable part (#14472) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writes down the state-modelling conventions we keep re-explaining in review, and adds one lint rule for the part that is mechanically checkable. ### Why Reviews of anything with a beginning and an end — a wizard, an upload, an onboarding flow — keep converging on the same handful of points: several independent booleans that must agree, values stored that could be derived, and effects used to keep two pieces of internal state in sync rather than to talk to something outside the app. Each time it gets re-argued from scratch on the PR, which is slow for the reviewer and unpleasant for the author, who reasonably asks why nobody said so earlier. `docs/guidance/state-and-effects.md` is glob-loaded on `src/**/*.ts` and `src/**/*.vue`, so it applies automatically rather than needing to be found. ### What it says One state value as a discriminated union instead of N booleans, named events rather than assignment scattered across call sites, a pure transition function so one place owns the rules, `computed` for anything derivable, and effects reserved for synchronising outward. Plus the Vue mapping — state to Pinia, transition to a pure function, derivation to `computed`, synchronisation to `watch`. It deliberately does **not** recommend a state-machine library. There is no XState or `createMachine` precedent anywhere in `src/`, and a hand-rolled union with a pure `reduce` gets the same guarantees without the dependency. ### The lint rule `no-restricted-syntax` flagging `getBoundingClientRect`, `getComputedStyle` and `querySelector*` inside a `computed`. A derivation that measures the DOM runs a layout read on every recompute and cannot be unit-tested without a browser. It ships at **`warn`, not `error`**, because there are exactly four pre-existing instances and this PR does not fix them: | File | Count | | ---- | ----- | | `src/components/maskeditor/BrushCursor.vue` | 2 | | `src/components/breadcrumb/SubgraphBreadcrumb.vue` | 1 | | `src/components/topbar/WorkflowTabs.vue` | 1 | Promote to `error` once those derive from stores instead. I verified the count against `main` at the current head rather than trusting the number I first wrote. ### Scope Docs and lint config only — no runtime code, no behaviour change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) --- AGENTS.md | 1 + docs/guidance/state-and-effects.md | 157 +++++++++++++++++++++++++++++ eslint.config.ts | 28 +++++ 3 files changed, 186 insertions(+) create mode 100644 docs/guidance/state-and-effects.md diff --git a/AGENTS.md b/AGENTS.md index 10502a0452f..4242f76eab0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,6 +4,7 @@ See @docs/guidance/\*.md for file-type-specific conventions (auto-loaded by glob - `docs/guidance/engineering.md` — general engineering guidelines, project philosophy, code-review checklist, external resource links - `docs/guidance/vue-components.md` — Vue 3 Composition API best practices +- `docs/guidance/state-and-effects.md` — modelling a feature's state: one discriminated union, named events, a pure transition, effects reserved for synchronising outward - `docs/guidance/typescript.md` — TypeScript type-safety rules - `docs/guidance/vitest.md` — Vitest unit/component test conventions - `docs/guidance/playwright.md` — Playwright E2E conventions and API-mock typing table diff --git a/docs/guidance/state-and-effects.md b/docs/guidance/state-and-effects.md new file mode 100644 index 00000000000..8be2588c765 --- /dev/null +++ b/docs/guidance/state-and-effects.md @@ -0,0 +1,157 @@ +--- +globs: + - 'src/**/*.ts' + - 'src/**/*.vue' +--- + +# State, Effects and Workflows + +How to represent a multi-step process — onboarding, a wizard, an upload, a +checkout, anything with a beginning and an end — in a reactive framework. + +> **Events initiate work. One explicit state represents the workflow. Pure +> derivations shape the UI. Effects only synchronise with external systems.** + +This applies to any framework; the Vue mapping is at the bottom. + +## 1. One state, not several booleans + +Hold the **minimum authoritative facts** needed to render and continue. Anything +computable from them is not state. + +Avoid independent booleans for what is really one value. Prefer a discriminated +union, so invalid combinations are structurally impossible rather than merely +unlikely. + +```ts +// ✗ four facts that must agree, and nothing makes them +const steps = ref([]) +const stepIdx = ref(-1) +const waiting = ref(false) +const active = ref(null) + +// ✓ one fact +type FlowState = + | { phase: 'idle' } + | { phase: 'awaiting'; flow: Flow; steps: Step[]; idx: number } + | { phase: 'showing'; flow: Flow; steps: Step[]; idx: number } + | { phase: 'finished'; flow: Flow; outcome: Outcome } +``` + +**The tell:** if ending the workflow means resetting four variables in the right +order, it was one state and you gave it four variables. + +## 2. Change state through named events and a pure transition + +```ts +type FlowEvent = + | { type: 'started'; flow: Flow; steps: Step[] } + | { type: 'advanced' } + | { type: 'skipped' } + +function reduceFlow(state: FlowState, event: FlowEvent): FlowState // pure switch +``` + +Invalid transitions become harmless — an event that means nothing in the current +phase returns the state untouched — and valid ones become reviewable in one +place. This is the same reason most frameworks recommend a reducer once +state-update logic gets complex. + +It is also exhaustively testable: every state crossed with every event, including +the pairs that should do nothing. + +## 3. Async orchestration belongs in commands + +Do **not** build chains where each step is an effect reacting to the last: + +``` +saving changed → effect saves → saved changed → effect invalidates + → invalidated changed → effect navigates +``` + +Nobody can read that as one sequence, and the middle of it is reachable from +places you did not intend. Give the whole causal sequence to one function: + +```ts +async function startFlow(id: string) { + const data = await load(id) + if (!data) return false + dispatch({ type: 'started', flow: id, steps: build(data) }) + await settle() + dispatch({ type: 'stepEntered', idx: 0 }) + return true +} +``` + +## 4. Derive everything derivable + +Do not store `isOpening`, `canTransition`, `isLast`, or a second copy of data +that already exists. If a `computed` can name it, do not put it in a `ref` and +keep it in sync by hand — the sync is where the bugs live. + +## 5. Reserve effects for synchronising with the outside + +**Good effects** — one-way, outward, and they write nothing anything else reads: + +- state changed → emit telemetry +- state changed → write a setting +- component disposed → abort a request or unsubscribe +- external socket event → _dispatch an event_ (not: assign state directly) + +**Bad effects:** + +- pointer target changed → recalculate steps +- success changed → advance the workflow +- an effect that writes state a second effect reads + +If two effects communicate through shared state, that is a transition wearing a +disguise. Put it in the reducer. + +## 6. Do not inspect the DOM for something a store already knows + +Avoid `getBoundingClientRect`, `getComputedStyle`, and `document.querySelector` +for positions and sizes that a store holds — especially inside a `computed`, +where every recompute becomes a layout read. + +For canvas-anchored UI: `layoutStore` holds node bounds and `useTransformState` +mirrors the camera, both reactive, so a screen rect is a **derivation** rather +than a measurement. Floating UI accepts a +[virtual element](https://floating-ui.com/docs/virtual-elements) for exactly +this. + +Two caveats worth knowing rather than discovering: + +- `layoutStore` bounds are the node's **body box**. The element renders one + `NODE_TITLE_HEIGHT` above `position` and the resize tracker subtracts it, so + anything deriving a rect must add it back — and a collapsed node is exactly one + title tall, which is zero without it. +- Node ids are **graph-local**. An id resolved against one graph names a + different node in another, so anything holding one across a workflow or + subgraph change must pin the graph it resolved against. + +## 7. Reach for a formal state machine when it earns it + +Parallel regions, nested states, timed transitions, cancellation, replay — at +that point a library (XState or equivalent) buys real guarantees. + +Below it, a discriminated union plus a pure reducer is the same model without the +dependency, and the migration between them is mechanical. **There is currently no +XState in this repo**; introducing it is a deliberate decision, not a default. + +Note that two genuinely parallel regions — say a wizard's progress and a +long-running job's outcome — should be two small states, not one product type. + +## Vue mapping + +| Concern | Where it goes | +| --------------------- | --------------------------------------------- | +| Workflow state | `ref` / `shallowRef` in a Pinia store | +| Transition | pure function, its own module, no Vue imports | +| Derived UI | `computed` | +| External sync | `watch` / `watchEffect` | +| The workflow itself | store method (a command) | +| Complex state machine | XState or equivalent | + +**Mental model:** user or external event → command performs async work → typed +event → pure state transition → reactive derived UI. Effects sit _beside_ this +loop, only to synchronise with external systems. diff --git a/eslint.config.ts b/eslint.config.ts index 1a639b5aefe..134cd9dad40 100644 --- a/eslint.config.ts +++ b/eslint.config.ts @@ -302,6 +302,34 @@ export default defineConfig([ ] } }, + // A layout read inside a derivation runs on every recompute, and a derivation + // that measures the DOM cannot be tested without one. See + // docs/guidance/state-and-effects.md. + // + // 'warn' rather than 'error' because four pre-existing instances remain, in + // BrushCursor.vue, WorkflowTabs.vue and SubgraphBreadcrumb.vue. Promote to + // 'error' once those are derived from stores instead. + { + files: ['src/**/*.ts', 'src/**/*.vue'], + ignores: ['**/*.test.ts', '**/*.spec.ts'], + rules: { + 'no-restricted-syntax': [ + 'warn', + { + selector: + "CallExpression[callee.name='computed'] CallExpression[callee.property.name='getBoundingClientRect']", + message: + 'Do not measure the DOM inside a computed - every recompute becomes a layout read. Derive from a store instead. See docs/guidance/state-and-effects.md.' + }, + { + selector: + "CallExpression[callee.name='computed'] CallExpression[callee.property.name=/^(getComputedStyle|querySelector|querySelectorAll)$/]", + message: + 'Do not inspect the DOM inside a computed. Derive from a store instead. See docs/guidance/state-and-effects.md.' + } + ] + } + }, { files: ['**/*.spec.ts'], ignores: ['browser_tests/tests/**/*.spec.ts', 'apps/*/e2e/**/*.spec.ts'],