From ed6ece20998a9c669ffc41234e30e30c9dd49c00 Mon Sep 17 00:00:00 2001 From: bymyself Date: Mon, 20 Jan 2025 18:20:59 -0700 Subject: [PATCH] Add forms plugin to issue report component (#2302) --- package-lock.json | 67 ++++ package.json | 1 + src/components/common/CheckboxGroup.vue | 40 -- .../content/ExecutionErrorDialogContent.vue | 4 +- .../content/IssueReportDialogContent.vue | 7 +- .../dialog/content/error/ReportIssuePanel.vue | 348 ++++++++++-------- .../error/__tests__/ReportIssuePanel.spec.ts | 273 +++++++++----- src/hooks/coreCommandHooks.ts | 2 +- src/locales/en/main.json | 8 +- src/types/issueReportTypes.ts | 16 +- 10 files changed, 466 insertions(+), 300 deletions(-) delete mode 100644 src/components/common/CheckboxGroup.vue diff --git a/package-lock.json b/package-lock.json index 2cf5ce6542..91f514d835 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,6 +11,7 @@ "dependencies": { "@atlaskit/pragmatic-drag-and-drop": "^1.3.1", "@comfyorg/comfyui-electron-types": "^0.4.11", + "@primevue/forms": "^4.2.5", "@comfyorg/litegraph": "^0.8.61", "@primevue/themes": "^4.2.5", "@sentry/vue": "^8.48.0", @@ -3961,6 +3962,25 @@ "node": ">=12" } }, + "node_modules/@primeuix/forms": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/@primeuix/forms/-/forms-0.0.2.tgz", + "integrity": "sha512-DpecPQd/Qf/kav4LKCaIeGuT3AkwhJzuHCkLANTVlN/zBvo8KIj3OZHsCkm0zlIMVVnaJdtx1ULNlRQdudef+A==", + "dependencies": { + "@primeuix/utils": "^0.3.0" + }, + "engines": { + "node": ">=12.11.0" + } + }, + "node_modules/@primeuix/forms/node_modules/@primeuix/utils": { + "version": "0.3.2", + "resolved": "https://registry.npmjs.org/@primeuix/utils/-/utils-0.3.2.tgz", + "integrity": "sha512-B+nphqTQeq+i6JuICLdVWnDMjONome2sNz0xI65qIOyeB4EF12CoKRiCsxuZ5uKAkHi/0d1LqlQ9mIWRSdkavw==", + "engines": { + "node": ">=12.11.0" + } + }, "node_modules/@primeuix/styled": { "version": "0.3.2", "resolved": "https://registry.npmjs.org/@primeuix/styled/-/styled-0.3.2.tgz", @@ -3998,6 +4018,53 @@ "vue": "^3.3.0" } }, + "node_modules/@primevue/forms": { + "version": "4.2.5", + "resolved": "https://registry.npmjs.org/@primevue/forms/-/forms-4.2.5.tgz", + "integrity": "sha512-5jarJQ9Qv32bOo/0tY5bqR3JZI6+YmmoUQ2mjhVSbVElQsE4FNfhT7a7JwF+xgBPMPc8KWGNA1QB248HhPNVAg==", + "dependencies": { + "@primeuix/forms": "^0.0.2", + "@primeuix/utils": "^0.3.2", + "@primevue/core": "4.2.5" + }, + "engines": { + "node": ">=12.11.0" + } + }, + "node_modules/@primevue/forms/node_modules/@primeuix/styled": { + "version": "0.3.2", + "resolved": "https://registry.npmjs.org/@primeuix/styled/-/styled-0.3.2.tgz", + "integrity": "sha512-ColZes0+/WKqH4ob2x8DyNYf1NENpe5ZguOvx5yCLxaP8EIMVhLjWLO/3umJiDnQU4XXMLkn2mMHHw+fhTX/mw==", + "dependencies": { + "@primeuix/utils": "^0.3.2" + }, + "engines": { + "node": ">=12.11.0" + } + }, + "node_modules/@primevue/forms/node_modules/@primeuix/utils": { + "version": "0.3.2", + "resolved": "https://registry.npmjs.org/@primeuix/utils/-/utils-0.3.2.tgz", + "integrity": "sha512-B+nphqTQeq+i6JuICLdVWnDMjONome2sNz0xI65qIOyeB4EF12CoKRiCsxuZ5uKAkHi/0d1LqlQ9mIWRSdkavw==", + "engines": { + "node": ">=12.11.0" + } + }, + "node_modules/@primevue/forms/node_modules/@primevue/core": { + "version": "4.2.5", + "resolved": "https://registry.npmjs.org/@primevue/core/-/core-4.2.5.tgz", + "integrity": "sha512-+oWBIQs5dLd2Ini4KEVOlvPILk989EHAskiFS3R/dz3jeOllJDMZFcSp8V9ddV0R3yDaPdLVkfHm2Q5t42kU2Q==", + "dependencies": { + "@primeuix/styled": "^0.3.2", + "@primeuix/utils": "^0.3.2" + }, + "engines": { + "node": ">=12.11.0" + }, + "peerDependencies": { + "vue": "^3.3.0" + } + }, "node_modules/@primevue/icons": { "version": "4.2.5", "resolved": "https://registry.npmjs.org/@primevue/icons/-/icons-4.2.5.tgz", diff --git a/package.json b/package.json index d85c682ee2..2f5926824e 100644 --- a/package.json +++ b/package.json @@ -84,6 +84,7 @@ "dependencies": { "@atlaskit/pragmatic-drag-and-drop": "^1.3.1", "@comfyorg/comfyui-electron-types": "^0.4.11", + "@primevue/forms": "^4.2.5", "@comfyorg/litegraph": "^0.8.61", "@primevue/themes": "^4.2.5", "@sentry/vue": "^8.48.0", diff --git a/src/components/common/CheckboxGroup.vue b/src/components/common/CheckboxGroup.vue deleted file mode 100644 index 490f54b7fa..0000000000 --- a/src/components/common/CheckboxGroup.vue +++ /dev/null @@ -1,40 +0,0 @@ - - - diff --git a/src/components/dialog/content/ExecutionErrorDialogContent.vue b/src/components/dialog/content/ExecutionErrorDialogContent.vue index bc44ba4df8..37d9bb253b 100644 --- a/src/components/dialog/content/ExecutionErrorDialogContent.vue +++ b/src/components/dialog/content/ExecutionErrorDialogContent.vue @@ -90,10 +90,10 @@ const stackTraceField = computed(() => { label: t('issueReport.stackTrace'), value: 'StackTrace', optIn: true, - data: { + getData: () => ({ nodeType: props.error.node_type, stackTrace: props.error.traceback?.join('\n') - } + }) } }) diff --git a/src/components/dialog/content/IssueReportDialogContent.vue b/src/components/dialog/content/IssueReportDialogContent.vue index 3feb306a62..585420ae8a 100644 --- a/src/components/dialog/content/IssueReportDialogContent.vue +++ b/src/components/dialog/content/IssueReportDialogContent.vue @@ -7,9 +7,9 @@ }" > @@ -20,10 +20,9 @@ diff --git a/src/components/dialog/content/error/__tests__/ReportIssuePanel.spec.ts b/src/components/dialog/content/error/__tests__/ReportIssuePanel.spec.ts index b214b85e4e..ee1866311e 100644 --- a/src/components/dialog/content/error/__tests__/ReportIssuePanel.spec.ts +++ b/src/components/dialog/content/error/__tests__/ReportIssuePanel.spec.ts @@ -1,28 +1,51 @@ // @ts-strict-ignore -import { createTestingPinia } from '@pinia/testing' +import { Form } from '@primevue/forms' import { mount } from '@vue/test-utils' -import Button from 'primevue/button' +import Checkbox from 'primevue/checkbox' import PrimeVue from 'primevue/config' import InputText from 'primevue/inputtext' -import Panel from 'primevue/panel' import Textarea from 'primevue/textarea' import Tooltip from 'primevue/tooltip' -import { beforeAll, describe, expect, it, vi } from 'vitest' -import { createApp } from 'vue' +import { describe, expect, it, vi } from 'vitest' import { createI18n } from 'vue-i18n' -import CheckboxGroup from '@/components/common/CheckboxGroup.vue' import enMesages from '@/locales/en/main.json' -import { DefaultField, ReportField } from '@/types/issueReportTypes' +import { IssueReportPanelProps } from '@/types/issueReportTypes' import ReportIssuePanel from '../ReportIssuePanel.vue' -type ReportIssuePanelProps = { - errorType: string - defaultFields?: DefaultField[] - extraFields?: ReportField[] - tags?: Record - title?: string +const DEFAULT_FIELDS = ['Workflow', 'Logs', 'Settings', 'SystemStats'] +const CUSTOM_FIELDS = [ + { + label: 'Custom Field', + value: 'CustomField', + optIn: true, + getData: () => 'mock data' + } +] + +async function getSubmittedContext() { + const { captureMessage } = (await import('@sentry/core')) as any + return captureMessage.mock.calls[0][1] +} + +async function submitForm(wrapper: any) { + await wrapper.findComponent(Form).trigger('submit') + return getSubmittedContext() +} + +async function findAndUpdateCheckbox( + wrapper: any, + value: string, + checked = true +) { + const checkbox = wrapper + .findAllComponents(Checkbox) + .find((c: any) => c.props('value') === value) + if (!checkbox) throw new Error(`Checkbox with value "${value}" not found`) + + await checkbox.vm.$emit('update:modelValue', checked) + return checkbox } const i18n = createI18n({ @@ -59,18 +82,64 @@ vi.mock('@sentry/core', () => ({ captureMessage: vi.fn() })) +vi.mock('@primevue/forms', () => ({ + Form: { + name: 'Form', + template: + '
', + props: ['resolver'], + data() { + return { + formValues: {} + } + }, + methods: { + onSubmit() { + this.$emit('submit', { + valid: true, + values: this.formValues + }) + }, + updateFieldValue(name: string, value: any) { + this.formValues[name] = value + } + } + }, + FormField: { + name: 'FormField', + template: + '
', + props: ['name'], + data() { + return { + modelValue: '' + } + }, + methods: { + updateValue(value) { + this.modelValue = value + let parent = this.$parent + while (parent && parent.$options.name !== 'Form') { + parent = parent.$parent + } + if (parent) { + parent.updateFieldValue(this.name, value) + } + } + } + } +})) + describe('ReportIssuePanel', () => { - beforeAll(() => { - const app = createApp({}) - app.use(PrimeVue) + beforeEach(() => { + vi.clearAllMocks() }) - const mountComponent = (props: ReportIssuePanelProps, options = {}): any => { + const mountComponent = (props: IssueReportPanelProps, options = {}): any => { return mount(ReportIssuePanel, { global: { - plugins: [PrimeVue, createTestingPinia(), i18n], - directives: { tooltip: Tooltip }, - components: { InputText, Button, Panel, Textarea, CheckboxGroup } + plugins: [PrimeVue, i18n], + directives: { tooltip: Tooltip } }, props, ...options @@ -80,44 +149,66 @@ describe('ReportIssuePanel', () => { it('renders the panel with all required components', () => { const wrapper = mountComponent({ errorType: 'Test Error' }) expect(wrapper.find('.p-panel').exists()).toBe(true) - expect(wrapper.findAllComponents(CheckboxGroup).length).toBe(2) + expect(wrapper.findAllComponents(Checkbox).length).toBe(6) expect(wrapper.findComponent(InputText).exists()).toBe(true) expect(wrapper.findComponent(Textarea).exists()).toBe(true) - expect(wrapper.findComponent(Button).exists()).toBe(true) }) it('updates selection when checkboxes are selected', async () => { - const wrapper = mountComponent({ errorType: 'Test Error' }) - const checkboxes = wrapper.findAllComponents(CheckboxGroup).at(0) - await checkboxes?.setValue(['Workflow', 'Logs']) - expect(wrapper.vm.selection).toEqual(['Workflow', 'Logs']) + const wrapper = mountComponent({ + errorType: 'Test Error' + }) + + const checkboxes = wrapper.findAllComponents(Checkbox) + + for (const field of DEFAULT_FIELDS) { + const checkbox = checkboxes.find( + (checkbox) => checkbox.props('value') === field + ) + expect(checkbox).toBeDefined() + + await checkbox?.vm.$emit('update:modelValue', [field]) + expect(wrapper.vm.selection).toContain(field) + } }) it('updates contactInfo when input is changed', async () => { const wrapper = mountComponent({ errorType: 'Test Error' }) const input = wrapper.findComponent(InputText) - await input.setValue('test@example.com') - expect(wrapper.vm.contactInfo).toBe('test@example.com') + + await input.vm.$emit('update:modelValue', 'test@example.com') + const context = await submitForm(wrapper) + expect(context.user.email).toBe('test@example.com') }) it('updates additional details when textarea is changed', async () => { const wrapper = mountComponent({ errorType: 'Test Error' }) const textarea = wrapper.findComponent(Textarea) - await textarea.setValue('This is a test detail.') - expect(wrapper.vm.details).toBe('This is a test detail.') + + await textarea.vm.$emit('update:modelValue', 'This is a test detail.') + const context = await submitForm(wrapper) + expect(context.extra.details).toBe('This is a test detail.') }) - it('updates contactPrefs when preferences are selected', async () => { + it('set contact preferences back to false if email is removed', async () => { const wrapper = mountComponent({ errorType: 'Test Error' }) - const preferences = wrapper.findAllComponents(CheckboxGroup).at(1) - await preferences?.setValue(['FollowUp']) - expect(wrapper.vm.contactPrefs).toEqual(['FollowUp']) - }) + const input = wrapper.findComponent(InputText) - it('does not allow submission if the form is empty', async () => { - const wrapper = mountComponent({ errorType: 'Test Error' }) - await wrapper.vm.reportIssue() - expect(wrapper.vm.submitted).toBe(false) + // Set a valid email, enabling the contact preferences to be changed + await input.vm.$emit('update:modelValue', 'name@example.com') + + // Enable both contact preferences + for (const pref of ['followUp', 'notifyOnResolution']) { + await findAndUpdateCheckbox(wrapper, pref) + } + + // Change the email back to empty + await input.vm.$emit('update:modelValue', '') + const context = await submitForm(wrapper) + + // Check that the contact preferences are back to false automatically + expect(context.tags.followUp).toBe(false) + expect(context.tags.notifyOnResolution).toBe(false) }) it('renders with overridden default fields', () => { @@ -125,83 +216,87 @@ describe('ReportIssuePanel', () => { errorType: 'Test Error', defaultFields: ['Settings'] }) - const checkboxes = wrapper.findAllComponents(CheckboxGroup).at(0) - expect(checkboxes?.props('checkboxes')).toEqual([ - { label: 'Settings', value: 'Settings' } - ]) + + // Filter out the contact preferences checkboxes + const fieldCheckboxes = wrapper + .findAllComponents(Checkbox) + .filter( + (checkbox) => + !['followUp', 'notifyOnResolution'].includes(checkbox.props('value')) + ) + expect(fieldCheckboxes.length).toBe(1) + expect(fieldCheckboxes.at(0)?.props('value')).toBe('Settings') }) it('renders additional fields when extraFields prop is provided', () => { - const extraFields = [ - { label: 'Custom Field', value: 'CustomField', optIn: true, data: {} } - ] - const wrapper = mountComponent({ errorType: 'Test Error', extraFields }) - const checkboxes = wrapper.findAllComponents(CheckboxGroup).at(0) - expect(checkboxes?.props('checkboxes')).toContainEqual({ - label: 'Custom Field', - value: 'CustomField' + const wrapper = mountComponent({ + errorType: 'Test Error', + extraFields: CUSTOM_FIELDS }) + const customCheckbox = wrapper + .findAllComponents(Checkbox) + .find((checkbox) => checkbox.props('value') === 'CustomField') + expect(customCheckbox).toBeDefined() + }) + + it('allows custom fields to be selected', async () => { + const wrapper = mountComponent({ + errorType: 'Test Error', + extraFields: CUSTOM_FIELDS + }) + + await findAndUpdateCheckbox(wrapper, 'CustomField') + const context = await submitForm(wrapper) + expect(context.extra.CustomField).toBe('mock data') }) it('does not submit unchecked fields', async () => { const wrapper = mountComponent({ errorType: 'Test Error' }) const textarea = wrapper.findComponent(Textarea) - await textarea.setValue('Report with only text but no fields selected') - await wrapper.vm.reportIssue() + // Set details but don't check any field checkboxes + await textarea.vm.$emit( + 'update:modelValue', + 'Report with only text but no fields selected' + ) + const context = await submitForm(wrapper) - const { captureMessage } = (await import('@sentry/core')) as any - const captureContext = captureMessage.mock.calls[0][1] - - expect(captureContext.extra.logs).toBeNull() - expect(captureContext.extra.systemStats).toBeNull() - expect(captureContext.extra.settings).toBeNull() - expect(captureContext.extra.workflow).toBeNull() + // Verify none of the optional fields were included + for (const field of DEFAULT_FIELDS) { + expect(context.extra[field]).toBeUndefined() + } }) it.each([ { checkbox: 'Logs', apiMethod: 'getLogs', - expectedKey: 'logs', + expectedKey: 'Logs', mockValue: 'mock logs' }, { checkbox: 'SystemStats', apiMethod: 'getSystemStats', - expectedKey: 'systemStats', + expectedKey: 'SystemStats', mockValue: 'mock stats' }, { checkbox: 'Settings', apiMethod: 'getSettings', - expectedKey: 'settings', + expectedKey: 'Settings', mockValue: 'mock settings' } ])( - 'submits (%s) when the (%s) checkbox is selected', + 'submits $checkbox data when checkbox is selected', async ({ checkbox, apiMethod, expectedKey, mockValue }) => { const wrapper = mountComponent({ errorType: 'Test Error' }) const { api } = (await import('@/scripts/api')) as any vi.spyOn(api, apiMethod).mockResolvedValue(mockValue) - const { captureMessage } = await import('@sentry/core') - - // Select the checkbox - const checkboxes = wrapper.findAllComponents(CheckboxGroup).at(0) - await checkboxes?.vm.$emit('update:modelValue', [checkbox]) - - await wrapper.vm.reportIssue() - expect(api[apiMethod]).toHaveBeenCalled() - - // Verify the message includes the associated data - expect(captureMessage).toHaveBeenCalledWith( - 'User reported issue', - expect.objectContaining({ - extra: expect.objectContaining({ [expectedKey]: mockValue }) - }) - ) + await findAndUpdateCheckbox(wrapper, checkbox) + const context = await submitForm(wrapper) + expect(context.extra[expectedKey]).toBe(mockValue) } ) @@ -209,24 +304,12 @@ describe('ReportIssuePanel', () => { const wrapper = mountComponent({ errorType: 'Test Error' }) const { app } = (await import('@/scripts/app')) as any - const { captureMessage } = await import('@sentry/core') - const mockWorkflow = { nodes: [], edges: [] } vi.spyOn(app.graph, 'asSerialisable').mockReturnValue(mockWorkflow) - // Select the "Workflow" checkbox - const checkboxes = wrapper.findAllComponents(CheckboxGroup).at(0) - await checkboxes?.vm.$emit('update:modelValue', ['Workflow']) + await findAndUpdateCheckbox(wrapper, 'Workflow') + const context = await submitForm(wrapper) - await wrapper.vm.reportIssue() - expect(app.graph.asSerialisable).toHaveBeenCalled() - - // Verify the message includes the workflow - expect(captureMessage).toHaveBeenCalledWith( - 'User reported issue', - expect.objectContaining({ - extra: expect.objectContaining({ workflow: mockWorkflow }) - }) - ) + expect(context.extra.Workflow).toEqual(mockWorkflow) }) }) diff --git a/src/hooks/coreCommandHooks.ts b/src/hooks/coreCommandHooks.ts index 945cbcf2c1..f03b385fae 100644 --- a/src/hooks/coreCommandHooks.ts +++ b/src/hooks/coreCommandHooks.ts @@ -548,9 +548,9 @@ export function useCoreCommands(): ComfyCommand[] { function: () => { dialogService.showIssueReportDialog({ title: t('g.feedback'), + subtitle: t('issueReport.feedbackTitle'), panelProps: { errorType: 'Feedback', - title: t('issueReport.feedbackTitle'), defaultFields: ['SystemStats', 'Settings'] } }) diff --git a/src/locales/en/main.json b/src/locales/en/main.json index 02837079cc..d581fd870c 100644 --- a/src/locales/en/main.json +++ b/src/locales/en/main.json @@ -77,7 +77,7 @@ }, "issueReport": { "submitErrorReport": "Submit Error Report (Optional)", - "provideEmail": "Give us your email (Optional)", + "provideEmail": "Give us your email (optional)", "provideAdditionalDetails": "Provide additional details (optional)", "stackTrace": "Stack Trace", "systemStats": "System Stats", @@ -85,7 +85,11 @@ "notifyResolve": "Notify me when resolved", "helpFix": "Help Fix This", "rating": "Rating", - "feedbackTitle": "Help us improve ComfyUI by providing feedback" + "feedbackTitle": "Help us improve ComfyUI by providing feedback", + "validation": { + "maxLength": "Message too long", + "invalidEmail": "Please enter a valid email address" + } }, "color": { "default": "Default", diff --git a/src/types/issueReportTypes.ts b/src/types/issueReportTypes.ts index de33de20c9..920eb5d0df 100644 --- a/src/types/issueReportTypes.ts +++ b/src/types/issueReportTypes.ts @@ -1,3 +1,5 @@ +import { z } from 'zod' + export type DefaultField = 'Workflow' | 'Logs' | 'SystemStats' | 'Settings' export interface ReportField { @@ -14,7 +16,7 @@ export interface ReportField { /** * The data associated with this field, sent as part of the report. */ - data: Record + getData: () => unknown /** * Indicates whether the field requires explicit opt-in from the user @@ -49,3 +51,15 @@ export interface IssueReportPanelProps { */ title?: string } + +const checkboxField = z.boolean().optional() +export const issueReportSchema = z + .object({ + contactInfo: z.string().email().max(320).optional().or(z.literal('')), + details: z.string().max(5_000).optional() + }) + .catchall(checkboxField) + .refine((data) => Object.values(data).some((value) => value), { + path: ['details'] + }) +export type IssueReportFormData = z.infer