Fix interactive dialog bugs: dynamic select lookups, radio values, field refresh (#35640)

* Fix interactive dialog bugs: dynamic select lookups, radio values, and field refresh

- Cache sanitized fields in AppsForm to preserve object identity across
  renders, preventing AsyncSelect from remounting and re-triggering
  dynamic select lookups on every keystroke in any field

- Normalize radio field default values to plain strings in getDefaultValue()
  so the value shape is consistent with what RadioSetting.onChange returns
  (e.target.value). Accept both string and {label, value} object shapes
  downstream for backwards compatibility.

- Fix radio field [object Object] in submission by extracting .value from
  AppSelectOption objects in convertAppFormValuesToDialogSubmission

- Include selected_field in refresh submission so plugins know which field
  triggered the refresh. Use a shallow copy of accumulatedValues to avoid
  permanently contaminating the accumulated state.

- Send empty string for cleared select fields in refresh submissions.
  Previously, extractPrimitiveValues skipped null values and the spread
  merge never overwrote stale accumulated keys.
This commit is contained in:
Scott Bishel 2026-04-09 07:50:36 -06:00 committed by GitHub
parent 6878d09547
commit f441b34dee
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 477 additions and 67 deletions

View file

@ -237,6 +237,12 @@ const initFormValues = (form: AppForm, timezone?: string): AppFormValues => {
};
export class AppsForm extends React.PureComponent<Props, State> {
// 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<AppField, AppField>();
constructor(props: Props) {
super(props);
@ -271,6 +277,14 @@ export class AppsForm extends React.PureComponent<Props, State> {
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<Props, State> {
}
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 (
<AppsFormField

View file

@ -4,7 +4,7 @@
import React from 'react';
import {FormattedMessage} from 'react-intl';
import type {AppField, AppFormValue, AppSelectOption} from '@mattermost/types/apps';
import {isAppSelectOption, type AppField, type AppFormValue, type AppSelectOption} from '@mattermost/types/apps';
import type {UserAutocomplete} from '@mattermost/types/autocomplete';
import type {Channel} from '@mattermost/types/channels';
@ -176,7 +176,9 @@ export default class AppsFormField extends React.PureComponent<Props> {
);
}
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 (
<RadioSetting
id={name}

View file

@ -1355,29 +1355,19 @@ describe('components/interactive_dialog/InteractiveDialogAdapter', () => {
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(
<InteractiveDialogAdapter {...props}/>,
);
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(
<InteractiveDialogAdapter {...props}/>,
);
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: '',
}),
}),
);
});
});
});

View file

@ -169,13 +169,12 @@ class InteractiveDialogAdapter extends React.PureComponent<Props> {
* Common logic for processing form values - used by both submit and refresh
*/
private processFormValues = (currentValues: Record<string, any>): 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<Props> {
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,

View file

@ -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 <nil> values', () => {
const result = extractPrimitiveValues({
a: null,
b: undefined,
c: '',
d: '<nil>',
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 <nil> value', () => {
const result = extractPrimitiveValues({
color: {label: 'None', value: '<nil>'},
});
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 <nil> values', () => {
const result = extractPrimitiveValues({a: '<nil>'}, 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 <nil> value', () => {
const result = extractPrimitiveValues({
color: {label: 'None', value: '<nil>'},
}, 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: [],
});
});
});
});
});

View file

@ -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 "<nil>" values
* Filters out null, undefined, empty, and "<nil>" 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<string, any>): Record<string, any> {
const normalized: Record<string, any> = {};
Object.entries(values).forEach(([key, value]) => {
// Skip null, undefined, empty string, and "<nil>" values
if (value === null || value === undefined || value === '' || value === '<nil>') {
return;
}
export function extractPrimitiveValues(values: Record<string, any>, clearEmptyFields = false): Record<string, any> {
const isMeaningful = (v: any): v is string | boolean => v != null && v !== '' && v !== '<nil>';
return Object.entries(values).reduce<Record<string, any>>((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 !== '<nil>');
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 !== '<nil>') {
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:

View file

@ -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;
}