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/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 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'], 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/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/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. * 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 : '' +} 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() + }) +})