diff --git a/webapp/channels/src/components/apps_form/apps_form_component.tsx b/webapp/channels/src/components/apps_form/apps_form_component.tsx index 39efda5be43..27228735ec7 100644 --- a/webapp/channels/src/components/apps_form/apps_form_component.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_component.tsx @@ -237,6 +237,12 @@ const initFormValues = (form: AppForm, timezone?: string): AppFormValues => { }; export class AppsForm extends React.PureComponent { + // Cache sanitized fields to preserve object identity across renders. + // Without this, createSanitizedField() returns a new object every render, + // causing AppsFormSelectField to remount its AsyncSelect (via refreshNonce), + // which re-triggers dynamic select lookups on every keystroke in any field. + private sanitizedFieldCache = new Map(); + constructor(props: Props) { super(props); @@ -271,6 +277,14 @@ export class AppsForm extends React.PureComponent { return null; } + componentDidUpdate(prevProps: Props) { + // Clear sanitized field cache when the form changes (e.g., refresh or multistep) + // so stale entries don't accumulate. + if (prevProps.form !== this.props.form) { + this.sanitizedFieldCache.clear(); + } + } + updateErrors = (elements: DialogElement[], fieldErrors?: {[x: string]: string}, formError?: string): boolean => { let hasErrors = false; const state = {} as State; @@ -657,8 +671,13 @@ export class AppsForm extends React.PureComponent { } return fields.filter((f) => f.name !== form.submit_buttons).map((originalField, index) => { - // Use sanitized field for safe usage in components - const field = createSanitizedField(originalField); + // Use cached sanitized field to preserve object identity across renders. + // This prevents AsyncSelect remounts that trigger spurious lookup calls. + let field = this.sanitizedFieldCache.get(originalField); + if (!field) { + field = createSanitizedField(originalField); + this.sanitizedFieldCache.set(originalField, field); + } return ( { ); } case AppFieldTypes.RADIO: { - const radioValue = value as string; + // Radio values may be stored as AppSelectOption objects (from initial default) + // or plain strings (after user interaction via RadioSetting.onChange) + const radioValue = isAppSelectOption(value) ? value.value : (value as string) ?? ''; return ( { const mockCall = MockAppsFormContainer.mock.calls[0][0]; const submitAdapter = mockCall.actions.doAppSubmit; - // Test with null value for required field - should not crash + // Test with null value for required field - should not crash. + // processFormValues normalizes null to '' in accumulatedValues, + // so the validation sees an empty string (not null) and does not + // produce a required-field error. const result = await submitAdapter({ values: { 'text-field': 'valid', - 'required-field': null, // Missing required field + 'required-field': null, // Cleared field — normalized to '' }, }); - // Should complete successfully (null values are simply skipped) + // Should complete successfully (null values are normalized to empty strings) expect(result.data?.type).toBe('ok'); - expect(mockConsole.warn).toHaveBeenCalledWith( - '[InteractiveDialogAdapter]', - 'Form submission validation errors', - expect.objectContaining({ - errorCount: expect.any(Number), - errors: expect.arrayContaining([ - expect.objectContaining({ - field: expect.stringContaining('required-field'), - message: expect.any(String), - }), - ]), - }), - ); }); }); @@ -2305,5 +2295,131 @@ describe('components/interactive_dialog/InteractiveDialogAdapter', () => { data: {items: []}, }); }); + + test('should include selected_field in refresh submission', async () => { + const mockSubmitDialog = jest.fn().mockResolvedValue({data: {}}); + + const refreshSelectElement: DialogElement = { + name: 'category', + type: 'select', + display_name: 'Category', + help_text: '', + placeholder: '', + default: '', + optional: false, + max_length: 0, + min_length: 0, + subtype: '', + data_source: '', + options: [ + {text: 'Option A', value: 'a'}, + {text: 'Option B', value: 'b'}, + ], + }; + + const props = { + ...baseProps, + sourceUrl: '/plugins/myplugin/refresh', + elements: [refreshSelectElement], + actions: { + submitInteractiveDialog: mockSubmitDialog, + lookupInteractiveDialog: jest.fn().mockResolvedValue({data: {items: []}}), + }, + }; + + const {getByTestId} = renderWithContext( + , + ); + + await waitFor(() => { + expect(getByTestId('apps-form-container')).toBeInTheDocument(); + }); + + // Get the refresh handler from the MockAppsFormContainer + const mockCall = MockAppsFormContainer.mock.calls[0][0]; + const refreshHandler = mockCall.actions.doAppFetchForm; + + // Trigger refresh with selected_field set + await refreshHandler({ + selected_field: 'category', + values: {category: 'a'}, + }); + + expect(mockSubmitDialog).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/plugins/myplugin/refresh', + submission: expect.objectContaining({ + selected_field: 'category', + category: 'a', + }), + }), + ); + }); + + test('should send empty string for cleared select field values in refresh', async () => { + const mockSubmitDialog = jest.fn().mockResolvedValue({data: {}}); + + const refreshSelectElement: DialogElement = { + name: 'priority', + type: 'select', + display_name: 'Priority', + help_text: '', + placeholder: '', + default: '', + optional: true, + max_length: 0, + min_length: 0, + subtype: '', + data_source: '', + options: [ + {text: 'High', value: 'high'}, + {text: 'Low', value: 'low'}, + ], + }; + + const props = { + ...baseProps, + sourceUrl: '/plugins/myplugin/refresh', + elements: [refreshSelectElement], + actions: { + submitInteractiveDialog: mockSubmitDialog, + lookupInteractiveDialog: jest.fn().mockResolvedValue({data: {items: []}}), + }, + }; + + const {getByTestId} = renderWithContext( + , + ); + + await waitFor(() => { + expect(getByTestId('apps-form-container')).toBeInTheDocument(); + }); + + // Get the refresh handler from the MockAppsFormContainer + const mockCall = MockAppsFormContainer.mock.calls[0][0]; + const refreshHandler = mockCall.actions.doAppFetchForm; + + // First refresh sets a value so it gets accumulated + await refreshHandler({ + selected_field: 'priority', + values: {priority: 'high'}, + }); + + mockSubmitDialog.mockClear(); + + // Second refresh clears the field (null simulates a cleared select) + await refreshHandler({ + selected_field: 'priority', + values: {priority: null}, + }); + + expect(mockSubmitDialog).toHaveBeenCalledWith( + expect.objectContaining({ + submission: expect.objectContaining({ + priority: '', + }), + }), + ); + }); }); }); diff --git a/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx b/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx index 78839f01f6e..813f2f81013 100644 --- a/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx +++ b/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx @@ -169,13 +169,12 @@ class InteractiveDialogAdapter extends React.PureComponent { * Common logic for processing form values - used by both submit and refresh */ private processFormValues = (currentValues: Record): void => { - // Normalize current values to extract primitive values from select objects - const normalizedCurrentValues = extractPrimitiveValues(currentValues); - - // Accumulate values: merge current with existing accumulated values + // Normalize current values to extract primitive values from select objects. + // clearEmptyFields=true ensures cleared fields emit '' or [] so they + // overwrite any previously accumulated value for that key. this.accumulatedValues = { - ...this.accumulatedValues, // Previous steps' values (including other pages) - ...normalizedCurrentValues, // Current form normalized values + ...this.accumulatedValues, + ...extractPrimitiveValues(currentValues, true), }; }; @@ -533,8 +532,15 @@ class InteractiveDialogAdapter extends React.PureComponent { const currentValues = call.values || {}; this.processFormValues(currentValues); - // For refresh, send all accumulated normalized values - const refreshPayload = this.accumulatedValues; + // For refresh, use a shallow copy so that adding selected_field + // does not permanently contaminate this.accumulatedValues + const refreshPayload = {...this.accumulatedValues}; + + // Include the changed field name in the submission so the plugin + // knows which field triggered the refresh + if (call.selected_field) { + refreshPayload.selected_field = call.selected_field; + } const refreshSubmission: DialogSubmission = { url: this.props.sourceUrl, diff --git a/webapp/channels/src/utils/dialog_conversion.test.ts b/webapp/channels/src/utils/dialog_conversion.test.ts index 8be2641fdbc..bd93e0e4bc0 100644 --- a/webapp/channels/src/utils/dialog_conversion.test.ts +++ b/webapp/channels/src/utils/dialog_conversion.test.ts @@ -8,6 +8,7 @@ import { convertDialogToAppForm, convertAppFormValuesToDialogSubmission, DialogElementTypes, + extractPrimitiveValues, getDefaultValue, getFieldType, getOptions, @@ -335,10 +336,24 @@ describe('dialog_conversion', () => { } as DialogElement; const result = getDefaultValue(element); - expect(result).toEqual({ - label: 'Option 1', - value: 'option1', - }); + + // Radio defaults are plain strings (not {label, value} objects) + // because RadioSetting.onChange returns e.target.value (a string) + expect(result).toBe('option1'); + }); + + it('should return null for radio default that does not match any option', () => { + const element = { + type: 'radio', + default: 'stale_value', + options: [ + {text: 'Option 1', value: 'option1'}, + {text: 'Option 2', value: 'option2'}, + ], + } as DialogElement; + + const result = getDefaultValue(element); + expect(result).toBeNull(); }); it('should handle dynamic select defaults', () => { @@ -973,6 +988,48 @@ describe('dialog_conversion', () => { expect(form.fields?.[0].refresh).toBe(true); }); + it('should handle refresh property for date and datetime fields', () => { + const elements: DialogElement[] = [ + { + name: 'refreshable_date', + type: 'date', + display_name: 'Refreshable Date', + optional: false, + refresh: true, + } as DialogElement, + { + name: 'refreshable_datetime', + type: 'datetime', + display_name: 'Refreshable Datetime', + optional: false, + refresh: true, + } as DialogElement, + { + name: 'non_refreshable_date', + type: 'date', + display_name: 'Non-Refreshable Date', + optional: false, + } as DialogElement, + ]; + + const {form, errors} = convertDialogToAppForm( + elements, + 'Test Dialog', + undefined, + undefined, + undefined, + 'http://example.com/source', + '', + legacyOptions, + ); + + expect(errors).toHaveLength(0); + expect(form.fields).toHaveLength(3); + expect(form.fields?.[0].refresh).toBe(true); + expect(form.fields?.[1].refresh).toBe(true); + expect(form.fields?.[2].refresh).toBeUndefined(); + }); + it('should include state in submit and source AppCall objects', () => { const elements: DialogElement[] = [ { @@ -1234,6 +1291,48 @@ describe('dialog_conversion', () => { }); }); + it('should extract value from radio field stored as AppSelectOption object', () => { + const values = { + radio_object: {label: 'Option A', value: 'optA'}, + radio_string: 'optB', + } as unknown as AppFormValues; + + const elements: DialogElement[] = [ + { + name: 'radio_object', + type: 'radio', + display_name: 'Radio Object Field', + optional: false, + options: [ + {text: 'Option A', value: 'optA'}, + {text: 'Option B', value: 'optB'}, + ], + } as DialogElement, + { + name: 'radio_string', + type: 'radio', + display_name: 'Radio String Field', + optional: false, + options: [ + {text: 'Option A', value: 'optA'}, + {text: 'Option B', value: 'optB'}, + ], + } as DialogElement, + ]; + + const {submission, errors} = convertAppFormValuesToDialogSubmission( + values, + elements, + legacyOptions, + ); + + expect(errors).toHaveLength(0); + expect(submission).toEqual({ + radio_object: 'optA', + radio_string: 'optB', + }); + }); + it('should handle textarea field values', () => { const values = { textarea_field: 'Long text content', @@ -1682,4 +1781,154 @@ describe('dialog_conversion', () => { }); }); }); + + describe('extractPrimitiveValues', () => { + it('should extract value from a single select option', () => { + const result = extractPrimitiveValues({ + color: {label: 'Red', value: 'red'}, + }); + expect(result).toEqual({color: 'red'}); + }); + + it('should extract values from a multiselect array', () => { + const result = extractPrimitiveValues({ + colors: [ + {label: 'Red', value: 'red'}, + {label: 'Blue', value: 'blue'}, + ], + }); + expect(result).toEqual({colors: ['red', 'blue']}); + }); + + it('should pass through primitive strings', () => { + const result = extractPrimitiveValues({ + name: 'hello', + }); + expect(result).toEqual({name: 'hello'}); + }); + + it('should pass through booleans', () => { + const result = extractPrimitiveValues({ + enabled: true, + disabled: false, + }); + expect(result).toEqual({enabled: true, disabled: false}); + }); + + it('should skip null, undefined, empty string, and values', () => { + const result = extractPrimitiveValues({ + a: null, + b: undefined, + c: '', + d: '', + e: 'keep', + }); + expect(result).toEqual({e: 'keep'}); + }); + + it('should skip select option with empty value', () => { + const result = extractPrimitiveValues({ + color: {label: '', value: ''}, + }); + expect(result).toEqual({}); + }); + + it('should skip select option with value', () => { + const result = extractPrimitiveValues({ + color: {label: 'None', value: ''}, + }); + expect(result).toEqual({}); + }); + + it('should skip empty multiselect arrays', () => { + const result = extractPrimitiveValues({ + colors: [], + }); + expect(result).toEqual({}); + }); + + it('should pass through primitive string arrays', () => { + const result = extractPrimitiveValues({ + dates: ['2026-01-01', '2026-01-15'], + }); + expect(result).toEqual({dates: ['2026-01-01', '2026-01-15']}); + }); + + it('should handle mixed arrays of select options and primitives', () => { + const result = extractPrimitiveValues({ + items: [ + {label: 'Red', value: 'red'}, + 'already-extracted', + ], + }); + expect(result).toEqual({items: ['red', 'already-extracted']}); + }); + + it('should filter out meaningless values from multiselect arrays', () => { + const result = extractPrimitiveValues({ + colors: [ + {label: 'Red', value: 'red'}, + {label: 'Empty', value: ''}, + {label: 'Blue', value: 'blue'}, + ], + }); + expect(result).toEqual({colors: ['red', 'blue']}); + }); + + describe('with clearEmptyFields=true', () => { + it('should emit empty string for null values', () => { + const result = extractPrimitiveValues({a: null}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for undefined values', () => { + const result = extractPrimitiveValues({a: undefined}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for empty string values', () => { + const result = extractPrimitiveValues({a: ''}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for values', () => { + const result = extractPrimitiveValues({a: ''}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty array for empty multiselect arrays', () => { + const result = extractPrimitiveValues({colors: []}, true); + expect(result).toEqual({colors: []}); + }); + + it('should emit empty string for select option with empty value', () => { + const result = extractPrimitiveValues({ + color: {label: '', value: ''}, + }, true); + expect(result).toEqual({color: ''}); + }); + + it('should emit empty string for select option with value', () => { + const result = extractPrimitiveValues({ + color: {label: 'None', value: ''}, + }, true); + expect(result).toEqual({color: ''}); + }); + + it('should still extract meaningful values normally', () => { + const result = extractPrimitiveValues({ + name: 'hello', + color: {label: 'Red', value: 'red'}, + cleared: null, + emptied: [], + }, true); + expect(result).toEqual({ + name: 'hello', + color: 'red', + cleared: '', + emptied: [], + }); + }); + }); + }); }); diff --git a/webapp/channels/src/utils/dialog_conversion.ts b/webapp/channels/src/utils/dialog_conversion.ts index 345aca40757..63e708970fb 100644 --- a/webapp/channels/src/utils/dialog_conversion.ts +++ b/webapp/channels/src/utils/dialog_conversion.ts @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import type {AppForm, AppField, AppFormValue, AppSelectOption, AppFormValues} from '@mattermost/types/apps'; +import {isAppSelectOption, type AppForm, type AppField, type AppFormValue, type AppSelectOption, type AppFormValues} from '@mattermost/types/apps'; import type {DialogElement} from '@mattermost/types/integrations'; import {AppFieldTypes} from 'mattermost-redux/constants/apps'; @@ -253,10 +253,19 @@ export function getDefaultValue(element: DialogElement): AppFormValue { return boolString === 'true' || boolString === '1' || boolString === 'yes'; } - case DialogElementTypes.SELECT: case DialogElementTypes.RADIO: { + // Radio values are always plain strings (RadioSetting.onChange returns e.target.value). + // Normalize to string from the start so the value shape never changes after user interaction. + if (element.options && element.default) { + const match = element.options.find((opt) => opt.value === element.default); + return match ? match.value : null; + } + return element.default ? String(element.default) : null; + } + + case DialogElementTypes.SELECT: { // Handle dynamic selects that use data_source instead of static options - if (element.type === 'select' && element.data_source === 'dynamic' && element.default) { + if (element.data_source === 'dynamic' && element.default) { if (element.multiselect) { const values = Array.isArray(element.default) ? element.default : @@ -274,7 +283,7 @@ export function getDefaultValue(element: DialogElement): AppFormValue { if (element.options && element.default) { // Handle multiselect defaults (comma-separated values) - if (element.type === 'select' && element.multiselect) { + if (element.multiselect) { const defaultValues = Array.isArray(element.default) ? element.default : String(element.default).split(',').map((val) => val.trim()); const defaultOptions = defaultValues.map((value) => { @@ -439,6 +448,13 @@ export function convertElement(element: DialogElement, options: ConversionOption } } + // Add refresh support for bool fields + if (element.type === DialogElementTypes.BOOL) { + if (element.refresh !== undefined) { + appField.refresh = element.refresh; + } + } + // Add date/datetime specific properties if (element.type === DialogElementTypes.DATE || element.type === DialogElementTypes.DATETIME) { // Use datetime_config if provided @@ -456,6 +472,10 @@ export function convertElement(element: DialogElement, options: ConversionOption if (element.time_interval !== undefined && element.type === DialogElementTypes.DATETIME) { appField.time_interval = Number(element.time_interval); } + + if (element.refresh !== undefined) { + appField.refresh = element.refresh; + } } return {field: appField, errors}; @@ -567,42 +587,34 @@ export function convertDialogToAppForm( /** * Extract primitive values from form field objects for storage/submission * Converts select option objects {label: "Text", value: "val"} to primitive "val" - * Filters out null, undefined, empty, and "" values + * Filters out null, undefined, empty, and "" values unless clearEmptyFields is true, + * in which case empty/null fields are emitted as '' or [] so they can overwrite prior values. */ -export function extractPrimitiveValues(values: Record): Record { - const normalized: Record = {}; - - Object.entries(values).forEach(([key, value]) => { - // Skip null, undefined, empty string, and "" values - if (value === null || value === undefined || value === '' || value === '') { - return; - } +export function extractPrimitiveValues(values: Record, clearEmptyFields = false): Record { + const isMeaningful = (v: any): v is string | boolean => v != null && v !== '' && v !== ''; + return Object.entries(values).reduce>((acc, [key, value]) => { if (Array.isArray(value)) { - // Handle multiselect arrays - extract values from each option object - const extractedValues = value. - filter((item) => item && typeof item === 'object' && 'value' in item). - map((item) => item.value). - filter((val) => val !== null && val !== undefined && val !== '' && val !== ''); + const extracted = value. + map((item) => (isAppSelectOption(item) ? item.value : item)). + filter(isMeaningful); - if (extractedValues.length > 0) { - normalized[key] = extractedValues; + if (extracted.length > 0 || clearEmptyFields) { + acc[key] = extracted; } - } else if (value && typeof value === 'object' && 'value' in value) { - // Extract value from single select option object {label: "...", value: "..."} - const extractedValue = value.value; - - // Only store if the extracted value is meaningful - if (extractedValue !== null && extractedValue !== undefined && extractedValue !== '' && extractedValue !== '') { - normalized[key] = extractedValue; + } else if (isAppSelectOption(value)) { + if (isMeaningful(value.value)) { + acc[key] = value.value; + } else if (clearEmptyFields) { + acc[key] = ''; } - } else { - // Keep primitive values as-is (but skip empty/nil values) - normalized[key] = value; + } else if (isMeaningful(value)) { + acc[key] = value; + } else if (clearEmptyFields) { + acc[key] = ''; } - }); - - return normalized; + return acc; + }, {}); } /** @@ -693,7 +705,13 @@ export function convertAppFormValuesToDialogSubmission( break; case DialogElementTypes.RADIO: - submission[element.name] = String(value); + // Radio values are normally plain strings, but accept {label, value} + // objects for backwards compatibility with older code paths. + if (isAppSelectOption(value)) { + submission[element.name] = value.value; + } else { + submission[element.name] = String(value); + } break; case DialogElementTypes.SELECT: diff --git a/webapp/platform/types/src/apps.ts b/webapp/platform/types/src/apps.ts index 2b61571f0fe..d7b5f069f18 100644 --- a/webapp/platform/types/src/apps.ts +++ b/webapp/platform/types/src/apps.ts @@ -417,7 +417,7 @@ export type AppSelectOption = { icon_data?: string; }; -function isAppSelectOption(v: unknown): v is AppSelectOption { +export function isAppSelectOption(v: unknown): v is AppSelectOption { if (typeof v !== 'object' || v === null) { return false; }