Draft
Conversation
- replace @primevue/forms usage with vee-validate + @vee-validate/zod - add shared ui form primitives for field/item/control/message wiring - update auth form tests and remove legacy forms dependencies Amp-Thread-ID: https://ampcode.com/threads/T-019c7e06-730d-729e-8602-1bc1dffe4801 Co-authored-by: Amp <amp@ampcode.com>
- remove stale @primeuix/forms knip ignore dependency - use FormDescription in password field requirements block Amp-Thread-ID: https://ampcode.com/threads/T-019c7e06-730d-729e-8602-1bc1dffe4801 Co-authored-by: Amp <amp@ampcode.com>
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/21/2026, 08:08:00 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 526 passed, 0 failed · 4 flaky 📊 Browser Reports
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates form validation from PrimeVue Forms to vee-validate, adds a small form UI component library (FormItem/Field/Label/Control/Message/Description + composables), tightens Zod schemas to require string-keyed records, updates dependency configuration (remove PrimeVue forms, add vee-validate), and applies type-safety refinements across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Form as SignInForm / Form UI
participant Validator as vee-validate useForm
participant Field as FormField
participant Composable as useFormField
participant Handler as handleSubmit
participant Auth as authActions
User->>Form: enter credentials
Form->>Validator: initialize useForm(schema, values)
Form->>Field: render FormField(name)
Field->>Composable: inject field name & item id
Composable-->>Field: provide errors/meta/ids
User->>Form: submit
Form->>Handler: handleSubmit(onSubmit)
Handler->>Validator: validate()
Validator->>Field: validate field values
Field->>Composable: request error state
Composable-->>Field: error messages
alt validation success
Handler->>Auth: authActions.signIn(values)
Auth-->>Handler: response
Handler->>Form: emit submit / navigate
else validation failure
Handler->>Form: display FormMessage errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
📦 Bundle: 4.38 MB gzip 🔴 +18.2 kBDetailsSummary
Category Glance App Entry Points — 21.5 kB (baseline 21.5 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 926 kB (baseline 926 kB) • 🔴 +13 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69.3 kB (baseline 68.8 kB) • 🔴 +580 BTop-level views, pages, and routed surfaces
Status: 10 added / 10 removed Panels & Settings — 432 kB (baseline 432 kB) • 🟢 -1 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 20.8 kB (baseline 16 kB) • 🔴 +4.82 kBAuthentication, profile, and account management bundles
Status: 6 added / 6 removed Editors & Dialogs — 738 B (baseline 738 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 43.3 kB (baseline 43.2 kB) • 🔴 +1 BReusable component library chunks
Status: 11 added / 11 removed Data & Services — 2.5 MB (baseline 2.5 MB) • 🟢 -2.26 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 57.7 kB (baseline 57.7 kB) • 🔴 +1 BHelpers, composables, and utility bundles
Status: 14 added / 14 removed Vendor & Third-Party — 8.93 MB (baseline 8.86 MB) • 🔴 +71.2 kBExternal libraries and shared vendor chunks
Status: 4 added / 4 removed Other — 7.6 MB (baseline 7.6 MB) • 🔴 +1.06 kBBundles that do not match a named category
Status: 67 added / 67 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/dialog/content/signin/SignUpForm.vue (1)
26-34:⚠️ Potential issue | 🟠 MajorGuard against duplicate submits while
loadingis true.When
loadingis true, the button disappears but the<form>can still submit via Enter key. This can re-triggeremit('submit', …)while a request is in flight.🛡️ Suggested guard
const onSubmit = useThrottleFn( handleSubmit((submittedValues) => { + if (loading.value) return emit('submit', submittedValues) }), 1_500 )Also applies to: 79-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialog/content/signin/SignUpForm.vue` around lines 26 - 34, The form currently still submits via Enter while loading because the template only hides the button; add a guard at the start of the component's submit handler(s) to return early when the reactive loading flag is true (e.g., in the method that calls emit('submit', …) — check functions like onSubmit/handleSubmit and any other submit handlers around the other occurrence at lines 79-82) so duplicate emits are prevented; optionally also add `@submit.prevent` on the <form> handler to ensure keyboard submits go through the guarded handler only.
🧹 Nitpick comments (2)
src/components/ui/form/useFormField.ts (1)
9-35: Prefer a function declaration for this composable to match the repo's convention.The codebase predominantly uses
export function useXxx()pattern for composables (e.g.,useWorkspaceBilling,useWorkflowValidation,useAssetSelection). This aligns with the guideline: Do not use function expressions if it's possible to use function declarations instead.♻️ Proposed fix
-export const useFormField = () => { +export function useFormField() { const fieldName = inject(FORM_FIELD_NAME_INJECTION_KEY) const itemId = inject(FORM_ITEM_ID_INJECTION_KEY) if (!fieldName || !itemId) { throw new Error('useFormField must be used within FormField and FormItem') } const errorMessage = useFieldError(fieldName) const formItemId = `${itemId}-form-item` const formDescriptionId = `${itemId}-form-item-description` const formMessageId = `${itemId}-form-item-message` const describedBy = computed(() => errorMessage.value ? `${formDescriptionId} ${formMessageId}` : formDescriptionId ) return { errorMessage, formDescriptionId, formItemId, formMessageId, describedBy, name: fieldName } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/form/useFormField.ts` around lines 9 - 35, Change the composable from a function expression to a function declaration: replace the exported constant declaration "export const useFormField = () => { ... }" with a named function declaration "export function useFormField() { ... }" while preserving all internal logic (injections FORM_FIELD_NAME_INJECTION_KEY and FORM_ITEM_ID_INJECTION_KEY, the error throw, useFieldError(fieldName), the computed describedBy, and the returned properties formDescriptionId, formItemId, formMessageId, errorMessage, describedBy, name). Ensure the function name remains useFormField and exports the function as before.src/components/dialog/content/signin/SignUpForm.vue (1)
6-16: Prefer$tin the template and dropuseI18nwhen unused in script.
Line 6–Line 16uset()only in the template. The repo preference is to use$tin template-only cases to avoid unnecessary imports and setup work.
Based on learnings: “In Vue single-file components where the i18n t function is only used within the template, prefer using the built-in $t in the template instead of importing useI18n…”🔧 Proposed refactor
- <FormLabel class="mb-2 text-base font-medium opacity-80"> - {{ t('auth.signup.emailLabel') }} - </FormLabel> + <FormLabel class="mb-2 text-base font-medium opacity-80"> + {{ $t('auth.signup.emailLabel') }} + </FormLabel> ... - :placeholder="t('auth.signup.emailPlaceholder')" + :placeholder="$t('auth.signup.emailPlaceholder')" ... - {{ t('auth.signup.signUpButton') }} + {{ $t('auth.signup.signUpButton') }} - import { useI18n } from 'vue-i18n' ... - const { t } = useI18n()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialog/content/signin/SignUpForm.vue` around lines 6 - 16, The template is using the composition API i18n helper t(...) but since i18n is only used in the template you should switch to the built-in $t and remove the unused useI18n setup/import: replace occurrences like t('auth.signup.emailLabel') and t('auth.signup.emailPlaceholder') with $t('...') in the template (e.g., inside FormLabel and InputText) and delete any useI18n import/const call from the script section so there are no unused imports or setup code remaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pnpm-workspace.yaml`:
- Line 38: The repo's `@vee-validate/zod` (referenced as '@vee-validate/zod')
requires zod ^3.24.0 but the workspace currently constrains zod to ^3.23.8;
update the zod version constraint to ^3.24.0 (or newer) wherever zod is declared
(ensure you update all occurrences mentioned alongside '@vee-validate/zod'), or
alternatively downgrade/pin '@vee-validate/zod' to a version compatible with zod
^3.23.8; adjust the package dependency entries for zod and/or `@vee-validate/zod`
so the peer dependency requirement is satisfied.
In `@src/components/dialog/content/signin/PasswordFields.vue`:
- Around line 93-122: In PasswordFields.vue the computed passwordChecks accesses
values.password directly which can be undefined; update the passwordChecks
computed to coerce or guard values.password (e.g. const pwd = values.password ??
'' or String(values.password)) and then use pwd.length and run the regex tests
against pwd (replace occurrences of values.password.length and
/.../.test(values.password) with pwd.length and /.../.test(pwd)) so undefined
passwords no longer throw; target the passwordChecks computed constant to apply
this fix.
In `@src/components/dialog/content/signin/SignInForm.vue`:
- Around line 37-45: The clickable <span> is not keyboard-accessible; replace it
with the shared Button (or an anchor/button element) so the "Forgot password"
control is semantic and focusable: in SignInForm.vue change the span to use the
shared Button component (or a <button>) and wire its click to
handleForgotPassword(values.email, !errors.email), keep the same
classes/conditional 'text-link-disabled' logic via :class or props, and ensure
it has appropriate type="button" and aria-disabled when values.email is empty or
errors.email is present so keyboard and screen-reader users can activate or
understand the state.
In `@src/components/ui/form/FormControl.vue`:
- Around line 10-23: The component only extracts the class prop and drops all
other incoming attributes; update the props destructuring to capture rest props
(e.g., const { class: className, ...rest } = defineProps< { class?:
HTMLAttributes['class']; [key: string]: unknown } >()) and then forward them
onto the Slot wrapper (add v-bind="rest" or :="rest" to the Slot element) while
keeping the existing :id="formItemId", :aria-describedby="describedBy",
:aria-invalid="Boolean(errorMessage)" and :class="cn(className)"; this ensures
data-* and custom aria-* props passed to FormControl.vue are forwarded to the
underlying Slot.
In `@src/components/ui/form/FormDescription.vue`:
- Around line 15-17: The paragraph in FormDescription.vue uses the wrong design
token: update the class string in the template (the <p> element that binds
:id="formDescriptionId" and uses cn(..., className)) to replace "text-muted"
with "text-muted-foreground" so it follows the design-token guideline; ensure
you only change that token and keep the existing cn(...) usage and props
(formDescriptionId, className) intact.
In `@src/components/ui/form/index.ts`:
- Around line 1-6: Remove the barrel export file (index.ts) that re-exports
FormControl, FormDescription, FormField, FormItem, FormLabel, and FormMessage;
instead delete this file and update all consumers (notably SignInForm.vue and
PasswordFields.vue) to import the specific components directly from their .vue
files (for example import FormField from './FormField.vue'), replacing any
imports that reference the barrel with direct file paths for each exported
symbol.
---
Outside diff comments:
In `@src/components/dialog/content/signin/SignUpForm.vue`:
- Around line 26-34: The form currently still submits via Enter while loading
because the template only hides the button; add a guard at the start of the
component's submit handler(s) to return early when the reactive loading flag is
true (e.g., in the method that calls emit('submit', …) — check functions like
onSubmit/handleSubmit and any other submit handlers around the other occurrence
at lines 79-82) so duplicate emits are prevented; optionally also add
`@submit.prevent` on the <form> handler to ensure keyboard submits go through the
guarded handler only.
---
Nitpick comments:
In `@src/components/dialog/content/signin/SignUpForm.vue`:
- Around line 6-16: The template is using the composition API i18n helper t(...)
but since i18n is only used in the template you should switch to the built-in $t
and remove the unused useI18n setup/import: replace occurrences like
t('auth.signup.emailLabel') and t('auth.signup.emailPlaceholder') with $t('...')
in the template (e.g., inside FormLabel and InputText) and delete any useI18n
import/const call from the script section so there are no unused imports or
setup code remaining.
In `@src/components/ui/form/useFormField.ts`:
- Around line 9-35: Change the composable from a function expression to a
function declaration: replace the exported constant declaration "export const
useFormField = () => { ... }" with a named function declaration "export function
useFormField() { ... }" while preserving all internal logic (injections
FORM_FIELD_NAME_INJECTION_KEY and FORM_ITEM_ID_INJECTION_KEY, the error throw,
useFieldError(fieldName), the computed describedBy, and the returned properties
formDescriptionId, formItemId, formMessageId, errorMessage, describedBy, name).
Ensure the function name remains useFormField and exports the function as
before.
pnpm-workspace.yaml
Outdated
| '@types/node': ^24.1.0 | ||
| '@types/semver': ^7.7.0 | ||
| '@types/three': ^0.169.0 | ||
| '@vee-validate/zod': ^4.15.1 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check `@vee-validate/zod` 4.15.1 peerDependencies
curl -s https://registry.npmjs.org/@vee-validate/zod/4.15.1 | jq '.peerDependencies'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 92
Upgrade Zod to satisfy @vee-validate/zod 4.15.1's peer dependency.
@vee-validate/zod 4.15.1 requires zod ^3.24.0, but the repository currently uses zod ^3.23.8. Either upgrade zod to ^3.24.0 or newer, or use a compatible @vee-validate/zod version.
Also applies to: line 95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pnpm-workspace.yaml` at line 38, The repo's `@vee-validate/zod` (referenced as
'@vee-validate/zod') requires zod ^3.24.0 but the workspace currently constrains
zod to ^3.23.8; update the zod version constraint to ^3.24.0 (or newer) wherever
zod is declared (ensure you update all occurrences mentioned alongside
'@vee-validate/zod'), or alternatively downgrade/pin '@vee-validate/zod' to a
version compatible with zod ^3.23.8; adjust the package dependency entries for
zod and/or `@vee-validate/zod` so the peer dependency requirement is satisfied.
Comment on lines
+10
to
+23
| const { class: className } = defineProps<{ | ||
| class?: HTMLAttributes['class'] | ||
| }>() | ||
|
|
||
| const { describedBy, errorMessage, formItemId } = useFormField() | ||
| </script> | ||
|
|
||
| <template> | ||
| <Slot | ||
| :id="formItemId" | ||
| :aria-describedby="describedBy" | ||
| :aria-invalid="Boolean(errorMessage)" | ||
| :class="cn(className)" | ||
| > |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine FormControl.vue
find . -name "FormControl.vue" -type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 110
🏁 Script executed:
# Check the actual content of FormControl.vue
cat -n src/components/ui/form/FormControl.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 784
🏁 Script executed:
# Check for useForwardProps usage patterns in UI components
rg -n "useForwardProps" src/components/uiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2243
🏁 Script executed:
# Look for other Reka UI Slot wrapper patterns
rg -n "from 'reka-ui'" src/components/ui | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2125
🏁 Script executed:
# Check if there's anything special about Slot component in reka-ui documentation or other similar patterns
rg -n "Slot" src/components/ui -A 5 | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2429
🏁 Script executed:
# Look at similar simple wrapper patterns (e.g., form-related components)
find src/components/ui/form -name "*.vue" -type f | xargs -I {} sh -c 'echo "=== {} ===" && head -15 {}'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2290
🏁 Script executed:
# Check what Slot component from reka-ui actually supports (look at its usage in similar contexts)
rg -n "Slot" src/components/ui -B 2 -A 10 | grep -A 10 "from 'reka-ui'"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 795
🏁 Script executed:
# Examine FormDescription, FormLabel, FormMessage to see if they're simple divs or also Reka UI wrappers
cat -n src/components/ui/form/FormDescription.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 623
🏁 Script executed:
# Check if there are any tests or usage examples of FormControl that might shed light on what attrs might be passed
rg -n "FormControl" src --type vue -A 5 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
# Let's also check the Reka UI Slot component documentation or any type hints
rg -n "SlotProps\|Slot.*Props" src/components/ui -A 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Let me check if there are any examples showing how Slot is typically used in reka-ui context
rg -n "Slot" src -B 3 -A 3 --type-list | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 459
🏁 Script executed:
# Check node_modules or package.json to understand reka-ui version
cat package.json | grep -A 2 -B 2 reka-uiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 203
🏁 Script executed:
# Check if Slot from reka-ui has any special docs or if similar primitive wrappers exist
rg -n "import.*Slot.*from.*reka-ui" src -A 20 | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1388
🏁 Script executed:
# Let's check ToggleGroupItem as a reference - it uses useForwardProps with a Reka UI component
cat -n src/components/ui/toggle-group/ToggleGroupItem.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1608
🏁 Script executed:
# Check if there are any consumers of FormControl that might pass additional attributes
rg -n "FormControl" src -B 2 -A 8 | grep -v "test" | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 6602
🏁 Script executed:
# Look at SelectValue and SelectSeparator (simple Slot-like primitives) to see if they also might benefit from rest props
cat -n src/components/ui/select/SelectValue.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 406
Forward rest props through the Slot wrapper.
As a Reka UI wrapper, this component should forward non-class props so consumers can pass through attributes (e.g., data-*, custom aria-* values) to the underlying Slot.
♻️ Proposed fix
-import { Slot } from 'reka-ui'
+import { Slot, useForwardProps } from 'reka-ui'
-const { class: className } = defineProps<{
+const { class: className, ...restProps } = defineProps<{
class?: HTMLAttributes['class']
}>()
+const forwardedProps = useForwardProps(restProps)
<Slot
+ v-bind="forwardedProps"
:id="formItemId"
:aria-describedby="describedBy"
:aria-invalid="Boolean(errorMessage)"
:class="cn(className)"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/form/FormControl.vue` around lines 10 - 23, The component
only extracts the class prop and drops all other incoming attributes; update the
props destructuring to capture rest props (e.g., const { class: className,
...rest } = defineProps< { class?: HTMLAttributes['class']; [key: string]:
unknown } >()) and then forward them onto the Slot wrapper (add v-bind="rest" or
:="rest" to the Slot element) while keeping the existing :id="formItemId",
:aria-describedby="describedBy", :aria-invalid="Boolean(errorMessage)" and
:class="cn(className)"; this ensures data-* and custom aria-* props passed to
FormControl.vue are forwarded to the underlying Slot.
src/components/ui/form/index.ts
Outdated
Comment on lines
+1
to
+6
| export { default as FormControl } from './FormControl.vue' | ||
| export { default as FormDescription } from './FormDescription.vue' | ||
| export { default as FormField } from './FormField.vue' | ||
| export { default as FormItem } from './FormItem.vue' | ||
| export { default as FormLabel } from './FormLabel.vue' | ||
| export { default as FormMessage } from './FormMessage.vue' |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid adding a barrel file under src/.
The repo disallows barrel exports in index.ts. Please remove this file and update consumers to import each component directly (e.g., from ./FormField.vue). This will also affect imports in SignInForm.vue and PasswordFields.vue.
As per coding guidelines: **/index.ts: Don't use barrel files (e.g., /some/package/index.ts to re-export within /src).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/form/index.ts` around lines 1 - 6, Remove the barrel export
file (index.ts) that re-exports FormControl, FormDescription, FormField,
FormItem, FormLabel, and FormMessage; instead delete this file and update all
consumers (notably SignInForm.vue and PasswordFields.vue) to import the specific
components directly from their .vue files (for example import FormField from
'./FormField.vue'), replacing any imports that reference the barrel with direct
file paths for each exported symbol.
Amp-Thread-ID: https://ampcode.com/threads/T-019c7e95-acd0-76b9-9c37-7f825e8e2a86 Co-authored-by: Amp <amp@ampcode.com>
- Remove barrel file (ui/form/index.ts), use direct imports - Use text-muted-foreground design token in FormDescription - Replace span with Button for keyboard-accessible forgot password - Use function declaration for useFormField composable - Guard against undefined values.password in PasswordFields Amp-Thread-ID: https://ampcode.com/threads/T-019c7ebc-b163-72d9-a20e-ed38717b9cbb Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/scripts/changeTracker.ts (1)
431-446: Avoid in-place sorting during graph diff.
graph.nodes.sort(...)mutates the workflow state;graphDiffis called during debug logging and can still reorder nodes. Prefer a non-mutating copy to keep state stable.♻️ Suggested change (non-mutating sort)
- nodes: graph.nodes.sort((a: ComfyNode, b: ComfyNode) => { + nodes: [...graph.nodes].sort((a: ComfyNode, b: ComfyNode) => { if (typeof a.id === 'number' && typeof b.id === 'number') { return a.id - b.id } return 0 })As per coding guidelines, “Avoid mutable state; prefer immutability and assignment at point of declaration.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/changeTracker.ts` around lines 431 - 446, The function sortGraphNodes currently calls graph.nodes.sort(...) which mutates the original workflow; change it to perform a non-mutating sort by creating a copy of the nodes array (e.g. using [...graph.nodes] or graph.nodes.slice()) and sorting that copy, then return the new graph object with the sorted copy assigned to the nodes property (preserving links, floatingLinks, reroutes, groups, extra, definitions, subgraphs). Target the sortGraphNodes function and the nodes array usage to ensure graphDiff/debug logging no longer reorders the original graph.src/utils/veeValidateZod.ts (1)
4-23: Consider using function declaration for pure helper.Per repository conventions, prefer function declarations over function expressions for pure functions.
♻️ Proposed refactor
-const buildTypedSchemaErrors = (issues: z.ZodIssue[]): TypedSchemaError[] => { +function buildTypedSchemaErrors(issues: z.ZodIssue[]): TypedSchemaError[] { const groupedErrors = new Map<string, TypedSchemaError>() for (const issue of issues) { const path = issue.path.length > 0 ? issue.path.join('.') : '' const existingError = groupedErrors.get(path) if (existingError) { existingError.errors.push(issue.message) continue } groupedErrors.set(path, { path: path || undefined, errors: [issue.message] }) } return [...groupedErrors.values()] }Based on learnings: "Prefer pure function declarations over function expressions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/veeValidateZod.ts` around lines 4 - 23, Replace the arrow function expression buildTypedSchemaErrors with a function declaration while keeping its signature and body intact; specifically, change "const buildTypedSchemaErrors = (issues: z.ZodIssue[]): TypedSchemaError[] => { ... }" to "function buildTypedSchemaErrors(issues: z.ZodIssue[]): TypedSchemaError[] { ... }", preserving the internal logic (groupedErrors Map, path computation, merging messages) and the TypedSchemaError and z.ZodIssue types so behavior and types remain unchanged.src/components/dialog/content/signin/ApiKeyForm.vue (1)
127-138: Prefer a function declaration foronSubmit.The handler doesn’t rely on closure semantics, so a declaration keeps it aligned with the repo’s style.
As per coding guidelines: Do not use function expressions if it's possible to use function declarations instead.♻️ Suggested change
-const onSubmit = async (event: Event) => { +async function onSubmit(event: Event) { event.preventDefault() const { valid, values: submittedValues } = await validate() if (!valid) { return } if (submittedValues?.apiKey) { await apiKeyStore.storeApiKey(submittedValues.apiKey) emit('success') } -} +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialog/content/signin/ApiKeyForm.vue` around lines 127 - 138, Convert the onSubmit arrow function into an async function declaration named onSubmit to match project style: replace the const onSubmit = async (event: Event) => { ... } with async function onSubmit(event: Event) { ... } while preserving the existing logic that calls validate(), checks valid, uses submittedValues?.apiKey, calls apiKeyStore.storeApiKey(submittedValues.apiKey), and emits 'success'; ensure any references to onSubmit (e.g. form `@submit` or template bindings) continue to point to the same identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/workflow/validation/schemas/workflowSchema.ts`:
- Around line 348-352: The Zod generic usage for zSubgraphDefinition is using
Zod v4-style two-type parameters; update the declaration for Zod v3
compatibility by removing the second generic parameter (i.e., use
z.ZodType<SubgraphDefinitionBase<ComfyWorkflow1BaseOutput>> so the Def defaults)
or alternatively replace the second generic with the proper ZodTypeDef type if
you intentionally need a Def; adjust the declaration for zSubgraphDefinition and
any other occurrences referencing
SubgraphDefinitionBase/ComfyWorkflow1BaseOutput to match Zod v3 generics.
In `@src/schemas/apiSchema.ts`:
- Line 313: The z.enum(LinkMarkerShape) call fails because LinkMarkerShape is a
numeric TS enum; update the schema at 'Comfy.Graph.LinkMarkers' to accept
numeric values by replacing z.enum(LinkMarkerShape) with a numeric union such as
z.union([z.literal(0), z.literal(1), z.literal(2)]) (or alternatively convert
LinkMarkerShape to a string enum and keep z.enum), ensuring the schema uses the
same numeric values as the LinkMarkerShape enum.
---
Duplicate comments:
In `@pnpm-workspace.yaml`:
- Around line 94-111: Confirm that the bumped Zod version (^4.3.6) is supported
by the adapter and utilities by inspecting their peerDependencies for Zod 4:
check `@vee-validate/zod`, zod-to-json-schema, and zod-validation-error (and any
other packages referencing Zod) and if they do not declare compatibility with
Zod 4, either pin Zod to a compatible 3.x version or upgrade/lock those packages
to versions that declare Zod 4 support (or add a resolutions/overrides entry) so
the dependency graph consistently uses a Zod-4-compatible set.
In `@src/components/dialog/content/signin/SignInForm.vue`:
- Around line 37-45: Replace the non-semantic <span> with a real <button
type="button"> in the SignInForm.vue block so the forgot-password control is
keyboard accessible: bind :disabled="!values.email || Boolean(errors.email)"
instead of toggling classes only, keep the click handler
`@click`="handleForgotPassword(values.email, !errors.email)", add aria-disabled
when disabled, preserve the text via {{ t('auth.login.forgotPassword') }}, and
ensure the button does not submit the form (type="button") and styling/classes
are adjusted to match the current visual states.
---
Nitpick comments:
In `@src/components/dialog/content/signin/ApiKeyForm.vue`:
- Around line 127-138: Convert the onSubmit arrow function into an async
function declaration named onSubmit to match project style: replace the const
onSubmit = async (event: Event) => { ... } with async function onSubmit(event:
Event) { ... } while preserving the existing logic that calls validate(), checks
valid, uses submittedValues?.apiKey, calls
apiKeyStore.storeApiKey(submittedValues.apiKey), and emits 'success'; ensure any
references to onSubmit (e.g. form `@submit` or template bindings) continue to
point to the same identifier.
In `@src/scripts/changeTracker.ts`:
- Around line 431-446: The function sortGraphNodes currently calls
graph.nodes.sort(...) which mutates the original workflow; change it to perform
a non-mutating sort by creating a copy of the nodes array (e.g. using
[...graph.nodes] or graph.nodes.slice()) and sorting that copy, then return the
new graph object with the sorted copy assigned to the nodes property (preserving
links, floatingLinks, reroutes, groups, extra, definitions, subgraphs). Target
the sortGraphNodes function and the nodes array usage to ensure graphDiff/debug
logging no longer reorders the original graph.
In `@src/utils/veeValidateZod.ts`:
- Around line 4-23: Replace the arrow function expression buildTypedSchemaErrors
with a function declaration while keeping its signature and body intact;
specifically, change "const buildTypedSchemaErrors = (issues: z.ZodIssue[]):
TypedSchemaError[] => { ... }" to "function buildTypedSchemaErrors(issues:
z.ZodIssue[]): TypedSchemaError[] { ... }", preserving the internal logic
(groupedErrors Map, path computation, merging messages) and the TypedSchemaError
and z.ZodIssue types so behavior and types remain unchanged.
Comment on lines
+348
to
+352
| // eslint-disable-next-line prefer-const -- Forward declaration required for recursive schema. | ||
| let zSubgraphDefinition: z.ZodType< | ||
| SubgraphDefinitionBase<ComfyWorkflow1BaseOutput>, | ||
| SubgraphDefinitionBase<ComfyWorkflow1BaseInput> | ||
| > |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check installed Zod version and ZodType definition
rg -n '"zod"' package.json 2>/dev/null || echo "No package.json found with zod"
# Look for ZodType definition in the codebase or node_modules
if [ -f "package.json" ]; then
cat package.json | grep -A 5 -B 5 '"zod"' || true
fi
# Check node_modules for Zod type definitions
if [ -d "node_modules/zod" ]; then
echo "=== Zod types.d.ts ZodType definition ==="
head -n 100 node_modules/zod/lib/types.d.ts 2>/dev/null | grep -A 20 "class ZodType" || true
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 756
🏁 Script executed:
#!/bin/bash
# Look for the specific file mentioned in the review
cat -n src/platform/workflow/validation/schemas/workflowSchema.ts | sed -n '345,355p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 465
🏁 Script executed:
#!/bin/bash
# Search for how ZodType is used elsewhere in the codebase
rg "z\.ZodType<" src/ -A 2 | head -n 40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 717
🌐 Web query:
Zod v3 ZodType generic parameters definition
💡 Result:
In Zod v3, the base schema type is:
class ZodType<
Output = any,
Def extends ZodTypeDef = ZodTypeDef,
Input = Output
> { /* ... */ }[1]
What the generic parameters mean:
Output: the type you get after parsing (i.e.,z.output<typeof schema>/z.infer<typeof schema>). [1]Def: the internal “definition” type that describes the schema (a subtype ofZodTypeDef). Most users leave this as the default. [1]Input: the type accepted as input to parsing (i.e.,z.input<typeof schema>). Defaults toOutput, but differs when you use things like.transform()/ZodEffects. [1], [2]
Example where Input ≠ Output (because of a transform), using ZodType<Output, Def, Input> explicitly: [2]
const schema: z.ZodType<ILocal, z.ZodTypeDef, IRemote> =
z.object({ b: z.number() }).transform(({ b }) => ({ a: b }));Sources: [1] Zod v3 README on npm (shows the ZodType<Output, Def, Input> definition), [2] Stack Overflow example using ZodType<..., z.ZodTypeDef, ...> with differing input/output.
🌐 Web query:
Zod v4 ZodType generic parameters definition
💡 Result:
In Zod v4 (the "zod/v4" classic API), ZodType is defined with (up to) three generic parameters:
export interface ZodType<
out Output = unknown,
out Input = unknown,
out Internals extends core.$ZodTypeInternals<Output, Input> =
core.$ZodTypeInternals<Output, Input>
> extends core.$ZodType<Output, Input, Internals> { /* ... */ }[1]
Key v4 change vs v3: the old middle generic Def extends ZodTypeDef was removed, and Input now defaults to unknown (not Output). [2]
Related: the underlying core base type is $ZodType<Output = unknown, Input = unknown>. [3]
Sources: [1] [2] [3]
Correct the ZodType generic parameters for Zod v3 compatibility.
The code uses z.ZodType<Output, Input> syntax (Zod v4), but the project is on Zod v3 (^3.23.8). In Zod v3, the second generic parameter is Def extends ZodTypeDef (internal definition), not Input. Either:
- Use
z.ZodType<SubgraphDefinitionBase<ComfyWorkflow1BaseOutput>>(omit the second parameter to let it default), or - Upgrade to Zod v4 if migration is planned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/workflow/validation/schemas/workflowSchema.ts` around lines 348
- 352, The Zod generic usage for zSubgraphDefinition is using Zod v4-style
two-type parameters; update the declaration for Zod v3 compatibility by removing
the second generic parameter (i.e., use
z.ZodType<SubgraphDefinitionBase<ComfyWorkflow1BaseOutput>> so the Def defaults)
or alternatively replace the second generic with the proper ZodTypeDef type if
you intentionally need a Def; adjust the declaration for zSubgraphDefinition and
any other occurrences referencing
SubgraphDefinitionBase/ComfyWorkflow1BaseOutput to match Zod v3 generics.
| 'Comfy.Graph.DeduplicateSubgraphNodeIds': z.boolean(), | ||
| 'Comfy.Graph.LiveSelection': z.boolean(), | ||
| 'Comfy.Graph.LinkMarkers': z.nativeEnum(LinkMarkerShape), | ||
| 'Comfy.Graph.LinkMarkers': z.enum(LinkMarkerShape), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find package.json to check Zod version
fd package.json -x cat {} \; | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4222
🏁 Script executed:
# Find LinkMarkerShape definition
rg -n "LinkMarkerShape\s*=" --type=ts -A3 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 496
🏁 Script executed:
# Check the actual code at src/schemas/apiSchema.ts line 313 with context
cat -n src/schemas/apiSchema.ts | sed -n '305,320p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 926
🏁 Script executed:
# Verify Zod import and usage in apiSchema.ts
head -50 src/schemas/apiSchema.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1710
🏁 Script executed:
# Find LinkMarkerShape definition in litegraph.ts
rg -n "enum LinkMarkerShape|LinkMarkerShape\s*=" src/lib/litegraph/src/litegraph.ts -A10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Check if it's exported and the exact format
cat -n src/lib/litegraph/src/litegraph.ts | grep -A15 -B5 "LinkMarkerShape"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 819
🏁 Script executed:
# Find Zod version from pnpm-lock.yaml or catalog
rg "zod" pnpm-lock.yaml -A2 | head -30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 890
🏁 Script executed:
# Also check for package catalog
find . -name "pnpm-workspace.yaml" -o -name ".npmrc" -o -name "package-lock.json" | xargs cat 2>/dev/null | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1416
🏁 Script executed:
# Check root level catalog definition
cat -n .npmrc catalog.json pnpm-workspace.yaml 2>/dev/null | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3296
🏁 Script executed:
# Find and check the LinkMarkerShape definition in globalEnums
find . -name "globalEnums*" -type f | head -10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 114
🏁 Script executed:
# Search for LinkMarkerShape in types directory
rg -n "LinkMarkerShape" src/lib/litegraph/src/types/ -A10 -B2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 912
🏁 Script executed:
# Get the full definition from globalEnums
cat -n src/lib/litegraph/src/types/globalEnums.ts | grep -A20 "LinkMarkerShape"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 574
Fix z.enum(LinkMarkerShape) to work with numeric enum values.
The project uses Zod 4.3.6, where z.enum() accepts only string literals or string enums, but LinkMarkerShape is a numeric TypeScript enum (0, 1, 2). Use z.union([z.literal(0), z.literal(1), z.literal(2)]) or convert LinkMarkerShape to a string enum instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/schemas/apiSchema.ts` at line 313, The z.enum(LinkMarkerShape) call fails
because LinkMarkerShape is a numeric TS enum; update the schema at
'Comfy.Graph.LinkMarkers' to accept numeric values by replacing
z.enum(LinkMarkerShape) with a numeric union such as z.union([z.literal(0),
z.literal(1), z.literal(2)]) (or alternatively convert LinkMarkerShape to a
string enum and keep z.enum), ensuring the schema uses the same numeric values
as the LinkMarkerShape enum.
- Upgrade vee-validate from v4.15.1 to v5.0.0-beta.0 (Standard Schema) - Remove custom toTypedSchema adapter (veeValidateZod.ts) - Pass Zod schemas directly to validationSchema - Fix SignInForm tests: use text-based selectors, remove try/catch anti-pattern Amp-Thread-ID: https://ampcode.com/threads/T-019c7ec7-2a88-7165-9db4-ef656336407b Co-authored-by: Amp <amp@ampcode.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/dialog/content/signin/ApiKeyForm.vue (2)
34-43: Inconsistent field binding pattern.This form uses
v-model="apiKey"+v-bind="apiKeyAttrs"while SignInForm and SignUpForm useFormFieldwithcomponentField. Both work, but usingFormFieldwould provide consistency across auth forms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialog/content/signin/ApiKeyForm.vue` around lines 34 - 43, The ApiKeyForm uses InputText with v-model="apiKey" and v-bind="apiKeyAttrs" which is inconsistent with SignInForm/SignUpForm; refactor the API key input to use the shared FormField pattern instead: replace the InputText usage with FormField configured with componentField set to the InputText component, pass apiKeyAttrs via the FormField props (name="apiKey", :componentField="InputText", v-bind for attributes like autocomplete/placeholder/type/class, and :invalid bound to Boolean(errors.apiKey)) so the field wiring, validation and event handling match SignInForm/SignUpForm (update references to apiKey, apiKeyAttrs and InputText accordingly).
126-137: Redundant null check after validation.Similar to SignInForm, the
submittedValues?.apiKeycheck is redundant whenvalidistrue. Consider usinghandleSubmitfor consistency with other forms in this PR:♻️ Suggested simplification
-const onSubmit = async (event: Event) => { - event.preventDefault() - const { valid, values: submittedValues } = await validate() - if (!valid) { - return - } - - if (submittedValues?.apiKey) { - await apiKeyStore.storeApiKey(submittedValues.apiKey) - emit('success') - } -} +const { defineField, errors, handleSubmit } = useForm({ + initialValues: { + apiKey: '' + }, + validationSchema: apiKeySchema +}) + +const onSubmit = handleSubmit(async (submittedValues) => { + await apiKeyStore.storeApiKey(submittedValues.apiKey) + emit('success') +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialog/content/signin/ApiKeyForm.vue` around lines 126 - 137, The onSubmit handler performs an unnecessary null-check (submittedValues?.apiKey) after validate() returns valid; update onSubmit to assume submittedValues is present when valid and either directly use submittedValues.apiKey (no optional chaining) or replace the manual validate flow with the shared handleSubmit helper used by other forms for consistency; ensure you still call apiKeyStore.storeApiKey(submittedValues.apiKey) and emit('success') when validation passes (refer to onSubmit, validate, handleSubmit, apiKeyStore.storeApiKey, and emit).src/platform/cloud/onboarding/components/CloudSignInForm.vue (1)
118-132:!importantusage violates coding guidelines.The style block uses
!importantextensively. As per coding guidelines: "Never use!importantor the!important prefix for Tailwind classes; instead find and correct interfering existing!importantclasses."Consider using PrimeVue's
pt(passthrough) prop system to style these inputs without!important, or create a dedicated input variant that doesn't require style overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/cloud/onboarding/components/CloudSignInForm.vue` around lines 118 - 132, The scoped style block in CloudSignInForm.vue uses multiple !important overrides on selectors (.p-inputtext, .p-password input, .p-checkbox-checked .p-checkbox-box); remove the !important rules and replace them by using PrimeVue passthrough (pt) props on the input components or by creating a dedicated input/checkbox variant CSS class applied to the component, e.g., update the components where .p-inputtext / .p-password / .p-checkbox-box are used to supply pt attributes or a custom class and move the styling into that variant (without !important) so it doesn't conflict with existing PrimeVue styles.src/components/dialog/content/signin/SignInForm.vue (1)
119-128: Redundant field checks after validation.Since
signInSchemavalidates thatpasswordare required strings, thesubmittedValues?.email && submittedValues.passwordcheck is redundant whenvalidistrue. Consider usinghandleSubmit(like in CloudSignInForm.vue) which guarantees typed values when the callback fires:♻️ Suggested simplification
-const onSubmit = useThrottleFn(async (event: Event) => { - event.preventDefault() - const { valid, values: submittedValues } = await validate() - if (valid && submittedValues?.email && submittedValues.password) { - emit('submit', { - email: submittedValues.email, - password: submittedValues.password - }) - } -}, 1_500) +const onSubmit = useThrottleFn( + handleSubmit((submittedValues) => { + emit('submit', submittedValues) + }), + 1_500 +)This also removes the need to destructure
validatefromuseFormand manually callevent.preventDefault(), sincehandleSubmithandles that automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialog/content/signin/SignInForm.vue` around lines 119 - 128, The onSubmit function is redundantly checking submittedValues?.email && submittedValues.password after validate; switch to using handleSubmit from useForm (as in CloudSignInForm.vue) so the callback receives typed, guaranteed values and you can remove the manual validate call, preventDefault, and the extra null checks; wrap handleSubmit's callback with useThrottleFn (keep the existing onSubmit name), remove destructuring of validate, and rely on signInSchema to ensure email/password exist before emitting the 'submit' event.src/components/dialog/content/signin/SignInForm.test.ts (1)
87-100: Avoid non‑null assertion to keep failures readable.If the button isn’t found, the
!can throw a cryptic error. Consider an explicit guard so test failures are clearer.♻️ Suggested tweak
function findForgotPasswordButton(wrapper: VueWrapper<ComponentInstance>) { - return wrapper - .findAll('button[type="button"]') - .find((btn) => - btn.text().includes(enMessages.auth.login.forgotPassword) - )! + const button = wrapper + .findAll('button[type="button"]') + .find((btn) => + btn.text().includes(enMessages.auth.login.forgotPassword) + ) + if (!button) { + throw new Error('Forgot Password button not found') + } + return button }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dialog/content/signin/SignInForm.test.ts` around lines 87 - 100, The test currently uses a non-null assertion in findForgotPasswordButton which can produce cryptic crashes; update findForgotPasswordButton (and the 'shows disabled style when email is empty' test) to avoid '!' by returning an optional (or null) and add an explicit guard/assertion before accessing methods: locate the helper function findForgotPasswordButton and change it to return undefined/null when not found, then in the test assert the button exists with a clear message (e.g., expect(forgotBtn).toBeTruthy()/toBeDefined() with a message or throw an AssertionError) before calling forgotBtn.classes() so failures are readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/dialog/content/signin/SignInForm.test.ts`:
- Around line 151-185: Remove the ProgressSpinner stub and stop relying on the
module-level mockLoading flag; instead set loading per-test via a hoisted mock
or a per-test factory and assert with findComponent(ProgressSpinner). Update the
first test to not register a ProgressSpinner stub in the mountComponent call,
set the mocked loading state locally (use vi.hoisted() or create a mountFactory
that accepts { loading }), mount via mountComponent/mountFactory, await
nextTick(), then assert wrapper.findComponent(ProgressSpinner).exists() is true
and the submit button is absent; mirror the same per-test local setup for the
non-loading case where mockLoading is false.
---
Nitpick comments:
In `@src/components/dialog/content/signin/ApiKeyForm.vue`:
- Around line 34-43: The ApiKeyForm uses InputText with v-model="apiKey" and
v-bind="apiKeyAttrs" which is inconsistent with SignInForm/SignUpForm; refactor
the API key input to use the shared FormField pattern instead: replace the
InputText usage with FormField configured with componentField set to the
InputText component, pass apiKeyAttrs via the FormField props (name="apiKey",
:componentField="InputText", v-bind for attributes like
autocomplete/placeholder/type/class, and :invalid bound to
Boolean(errors.apiKey)) so the field wiring, validation and event handling match
SignInForm/SignUpForm (update references to apiKey, apiKeyAttrs and InputText
accordingly).
- Around line 126-137: The onSubmit handler performs an unnecessary null-check
(submittedValues?.apiKey) after validate() returns valid; update onSubmit to
assume submittedValues is present when valid and either directly use
submittedValues.apiKey (no optional chaining) or replace the manual validate
flow with the shared handleSubmit helper used by other forms for consistency;
ensure you still call apiKeyStore.storeApiKey(submittedValues.apiKey) and
emit('success') when validation passes (refer to onSubmit, validate,
handleSubmit, apiKeyStore.storeApiKey, and emit).
In `@src/components/dialog/content/signin/SignInForm.test.ts`:
- Around line 87-100: The test currently uses a non-null assertion in
findForgotPasswordButton which can produce cryptic crashes; update
findForgotPasswordButton (and the 'shows disabled style when email is empty'
test) to avoid '!' by returning an optional (or null) and add an explicit
guard/assertion before accessing methods: locate the helper function
findForgotPasswordButton and change it to return undefined/null when not found,
then in the test assert the button exists with a clear message (e.g.,
expect(forgotBtn).toBeTruthy()/toBeDefined() with a message or throw an
AssertionError) before calling forgotBtn.classes() so failures are readable.
In `@src/components/dialog/content/signin/SignInForm.vue`:
- Around line 119-128: The onSubmit function is redundantly checking
submittedValues?.email && submittedValues.password after validate; switch to
using handleSubmit from useForm (as in CloudSignInForm.vue) so the callback
receives typed, guaranteed values and you can remove the manual validate call,
preventDefault, and the extra null checks; wrap handleSubmit's callback with
useThrottleFn (keep the existing onSubmit name), remove destructuring of
validate, and rely on signInSchema to ensure email/password exist before
emitting the 'submit' event.
In `@src/platform/cloud/onboarding/components/CloudSignInForm.vue`:
- Around line 118-132: The scoped style block in CloudSignInForm.vue uses
multiple !important overrides on selectors (.p-inputtext, .p-password input,
.p-checkbox-checked .p-checkbox-box); remove the !important rules and replace
them by using PrimeVue passthrough (pt) props on the input components or by
creating a dedicated input/checkbox variant CSS class applied to the component,
e.g., update the components where .p-inputtext / .p-password / .p-checkbox-box
are used to supply pt attributes or a custom class and move the styling into
that variant (without !important) so it doesn't conflict with existing PrimeVue
styles.
Comment on lines
151
to
185
| describe('Loading State', () => { | ||
| it('shows spinner when loading', async () => { | ||
| mockLoading = true | ||
| const wrapper = mountComponent( | ||
| {}, | ||
| { | ||
| global: { | ||
| plugins: [ | ||
| PrimeVue, | ||
| createI18n({ | ||
| legacy: false, | ||
| locale: 'en', | ||
| messages: { en: enMessages } | ||
| }), | ||
| ToastService | ||
| ], | ||
| stubs: { | ||
| ProgressSpinner: { template: '<div data-testid="spinner" />' } | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| await nextTick() | ||
|
|
||
| try { | ||
| const wrapper = mountComponent() | ||
| await nextTick() | ||
|
|
||
| expect(wrapper.findComponent(ProgressSpinner).exists()).toBe(true) | ||
| expect(wrapper.findComponent(Button).exists()).toBe(false) | ||
| } catch (error) { | ||
| // Fallback test - check HTML content if component rendering fails | ||
| mockLoading = true | ||
| const wrapper = mountComponent() | ||
| expect(wrapper.html()).toContain('p-progressspinner') | ||
| expect(wrapper.html()).not.toContain('<button') | ||
| } | ||
| expect(wrapper.find('[data-testid="spinner"]').exists()).toBe(true) | ||
| expect(wrapper.find('button[type="submit"]').exists()).toBe(false) | ||
| }) | ||
|
|
||
| it('shows button when not loading', () => { | ||
| it('shows submit button when not loading', () => { | ||
| mockLoading = false | ||
|
|
||
| const wrapper = mountComponent() | ||
|
|
||
| expect(wrapper.findComponent(ProgressSpinner).exists()).toBe(false) | ||
| expect(wrapper.findComponent(Button).exists()).toBe(true) | ||
| expect(wrapper.find('button[type="submit"]').exists()).toBe(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "SignInForm.test.ts" --exec cat -n {} \;Repository: Comfy-Org/ComfyUI_frontend
Length of output: 9460
Avoid stubbing ProgressSpinner and use global mutable state in this test.
Stubbing ProgressSpinner (a PrimeVue primitive) and asserting via data-testid reduces test fidelity. The codebase already demonstrates the correct pattern at line 183—use findComponent(ProgressSpinner) instead. Additionally, toggling module-level mockLoading risks cross-test pollution. Isolate loading state per test using vi.hoisted() or a per-test factory to ensure test independence.
Example refactor
- let mockLoading = false
- vi.mock('@/stores/firebaseAuthStore', () => ({
- useFirebaseAuthStore: vi.fn(() => ({
- get loading() {
- return mockLoading
- }
- }))
- }))
+ const { mockLoading } = vi.hoisted(() => ({
+ mockLoading: { value: false }
+ }))
+
+ vi.mock('@/stores/firebaseAuthStore', () => ({
+ useFirebaseAuthStore: vi.fn(() => ({
+ get loading() {
+ return mockLoading.value
+ }
+ }))
+ }))For the spinner test:
- const wrapper = mountComponent(
- {},
- {
- global: {
- plugins: [...],
- stubs: {
- ProgressSpinner: { template: '<div data-testid="spinner" />' }
- }
- }
- }
- )
- await nextTick()
-
- expect(wrapper.find('[data-testid="spinner"]').exists()).toBe(true)
+ mockLoading.value = true
+ const wrapper = mountComponent()
+ await nextTick()
+
+ expect(wrapper.findComponent(ProgressSpinner).exists()).toBe(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/dialog/content/signin/SignInForm.test.ts` around lines 151 -
185, Remove the ProgressSpinner stub and stop relying on the module-level
mockLoading flag; instead set loading per-test via a hoisted mock or a per-test
factory and assert with findComponent(ProgressSpinner). Update the first test to
not register a ProgressSpinner stub in the mountComponent call, set the mocked
loading state locally (use vi.hoisted() or create a mountFactory that accepts {
loading }), mount via mountComponent/mountFactory, await nextTick(), then assert
wrapper.findComponent(ProgressSpinner).exists() is true and the submit button is
absent; mirror the same per-test local setup for the non-loading case where
mockLoading is false.
- Remove per-test ProgressSpinner stub; stub globally in mountComponent - Use findComponent(ProgressSpinner) consistently in loading tests - Replace non-null assertion with throwing guard in findForgotPasswordButton Amp-Thread-ID: https://ampcode.com/threads/T-019c7f35-4320-75dc-be40-98526259f2ae Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/dialog/content/signin/SignInForm.test.ts`:
- Around line 154-169: Tests currently rely on a module-level mutable variable
mockLoading which can leak state between tests; change the setup so each test
gets its own isolated loading value by replacing the module-level mockLoading
with a per-test factory or a vi.hoisted() ref and update mountComponent to
accept a loading argument (or read the hoisted ref) so tests set loading
explicitly before mount; update tests referencing mockLoading to call
mountComponent({ loading: true/false }) or set the hoisted ref within each test,
and keep references to ProgressSpinner and mountComponent to locate where the
change is needed.
- Around line 67-81: The test currently stubs the ProgressSpinner (stubs: {
ProgressSpinner: true }) which lowers fidelity; update the mount setup in
mountComponent for SignInForm to register the real ProgressSpinner instead of
stubbing it by removing the stub entry and adding ProgressSpinner to the mount
global components (or import ProgressSpinner from PrimeVue and include it in
global.components) while keeping existing plugins (PrimeVue, i18n, ToastService)
so the real primitive component is available during tests.
Contributor
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Could we split the PR or stack it?
Contributor
Author
That was the plan, just wanted to make sure it all worked first. |
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @DrJKL. The following files were modified: * `src/components/ui/form/useFormField.ts` * `src/platform/assets/utils/createAssetWidget.ts` * `src/platform/workflow/validation/schemas/workflowSchema.ts` * `src/scripts/changeTracker.ts` * `src/stores/subgraphStore.ts` These files were kept as they were: * `src/platform/assets/utils/createModelNodeFromAsset.ts` These files were ignored: * `src/components/dialog/content/signin/ApiKeyForm.test.ts` * `src/components/dialog/content/signin/SignInForm.test.ts` * `src/platform/workflow/validation/schemas/workflowSchema.test.ts` These file types are not supported: * `package.json` * `pnpm-workspace.yaml` * `src/components/dialog/content/UpdatePasswordContent.vue` * `src/components/dialog/content/signin/ApiKeyForm.vue` * `src/components/dialog/content/signin/PasswordFields.vue` * `src/components/dialog/content/signin/SignInForm.vue` * `src/components/dialog/content/signin/SignUpForm.vue` * `src/components/ui/form/FormControl.vue` * `src/components/ui/form/FormDescription.vue` * `src/components/ui/form/FormField.vue` * `src/components/ui/form/FormItem.vue` * `src/components/ui/form/FormLabel.vue` * `src/components/ui/form/FormMessage.vue` * `src/platform/cloud/onboarding/components/CloudSignInForm.vue`
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary
Migrate auth-related forms from
@primevue/formstovee-validate+@vee-validate/zod, add reusable shadcn-style form primitives, and remove legacy PrimeVue form dependencies.Changes
src/components/ui/form/*primitives (FormField,FormItem,FormControl,FormLabel,FormDescription,FormMessage) with field/item context helpers.src/components/dialog/content/signin/SignInForm.vuesrc/components/dialog/content/signin/SignUpForm.vuesrc/components/dialog/content/signin/ApiKeyForm.vuesrc/components/dialog/content/signin/PasswordFields.vuesrc/components/dialog/content/UpdatePasswordContent.vuesrc/platform/cloud/onboarding/components/CloudSignInForm.vuesrc/components/dialog/content/signin/SignInForm.test.tssrc/components/dialog/content/signin/ApiKeyForm.test.ts@primevue/formsand@primeuix/formsfrom dependencies.vee-validateand@vee-validate/zod; removed@primevue/formsand@primeuix/forms.Review Focus
InputText/Password), especially model/update wiring and error display.src/components/ui/form/*primitives.Screenshots (if applicable)
N/A (no visual design changes; structural/form-state migration only).
┆Issue is synchronized with this Notion page by Unito