[pull] main from Comfy-Org:main - #50
Merged
Merged
Conversation
## 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 <noreply@coderabbit.ai> Co-authored-by: Christian Byrne <cbyrne@comfy.org>
## 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
## 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) <noreply@anthropic.com>
## 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
…le part (#14472) 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )