From c114f6439453d77583b57c926dca9e9bdc92d8d9 Mon Sep 17 00:00:00 2001 From: Soroush Date: Wed, 28 Apr 2021 00:38:19 -0700 Subject: [PATCH] fix: Improve LG response editor parsing for text/speech variations (#7434) * fix * added comments * minor * lint * lint Co-authored-by: Soroush Co-authored-by: TJ Durnford Co-authored-by: Dong Lei --- .../src/lg/hooks/useStringArray.ts | 41 +++---- .../lg/modalityEditors/StringArrayEditor.tsx | 78 +++++++------ .../SuggestedActionsModalityEditor.tsx | 10 +- .../stringBuilders/lgStringUtils.test.ts | 90 +++++++++++++++ .../packages/lib/shared/src/lgUtils/index.ts | 1 + .../lgUtils/stringBuilders/lgStringUtils.ts | 108 ++++++++++++++++++ .../lg/src/LgWidget/useLgTemplate.ts | 20 ++-- 7 files changed, 275 insertions(+), 73 deletions(-) create mode 100644 Composer/packages/lib/shared/__tests__/lgUtils/stringBuilders/lgStringUtils.test.ts create mode 100644 Composer/packages/lib/shared/src/lgUtils/stringBuilders/lgStringUtils.ts diff --git a/Composer/packages/lib/code-editor/src/lg/hooks/useStringArray.ts b/Composer/packages/lib/code-editor/src/lg/hooks/useStringArray.ts index f7c3e2f063..b54adabeab 100644 --- a/Composer/packages/lib/code-editor/src/lg/hooks/useStringArray.ts +++ b/Composer/packages/lib/code-editor/src/lg/hooks/useStringArray.ts @@ -1,8 +1,10 @@ +/* eslint-disable security/detect-unsafe-regex */ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. import { LgTemplate } from '@bfc/shared'; import React from 'react'; +import { TemplateBodyItem, parseTemplateBody, templateBodyItemsToString } from '@bfc/shared'; import { ArrayBasedStructuredResponseItem, PartialStructuredResponse } from '../types'; import { getTemplateId } from '../../utils/structuredResponse'; @@ -14,28 +16,25 @@ const getInitialItems = ( response: T, lgTemplates?: readonly LgTemplate[], focusOnMount?: boolean -): string[] => { +): TemplateBodyItem[] => { const templateId = getTemplateId(response); const template = lgTemplates?.find(({ name }) => name === templateId); return response?.value && template?.body - ? template?.body - // Split by non-escaped - - // eslint-disable-next-line security/detect-unsafe-regex - ?.split(/(? s !== '' && s !== '\n') - .map((s) => s.replace(/\r?\n$/g, '')) + ? parseTemplateBody(template?.body) // Remove LG template multiline block symbol - .map((s) => s.replace(/```/g, '')) || [] - : response?.value || (focusOnMount ? [''] : []); + .map((s) => ({ ...s, value: s.value.replace(/```/g, '') })) ?? [] + : response?.value.map((v) => ({ kind: 'variation', value: v })) || + (focusOnMount ? [{ kind: 'variation', value: '' }] : []); }; -const fixMultilineItems = (items: string[]) => { +const fixMultilineItems = (items: TemplateBodyItem[]) => { return items.map((item) => { - if (/\r?\n/g.test(item)) { - // Escape all un-escaped - - // eslint-disable-next-line security/detect-unsafe-regex - return `${multiLineBlockSymbol}${item.replace(/(?( const { lgOption, lgTemplates, focusOnMount } = options || {}; const [templateId, setTemplateId] = React.useState(getTemplateId(structuredResponse)); - const [items, setItems] = React.useState(getInitialItems(structuredResponse, lgTemplates, focusOnMount)); + const [items, setItems] = React.useState( + getInitialItems(structuredResponse, lgTemplates, focusOnMount) + ); const onChange = React.useCallback( - (newItems: string[]) => { + (newItems: TemplateBodyItem[]) => { setItems(newItems); // Fix variations that are multiline // If only one item but it's multiline, still use helper LG template @@ -75,13 +76,13 @@ export const useStringArray = ( setTemplateId(id); onUpdateResponseTemplate({ [kind]: { kind, value: [], valueType: 'direct' } }); onRemoveTemplate(id); - } else if (fixedNewItems.length === 1 && !/\r?\n/g.test(fixedNewItems[0]) && lgOption?.templateId) { - onUpdateResponseTemplate({ [kind]: { kind, value: fixedNewItems, valueType: 'direct' } }); + } else if (fixedNewItems.length === 1 && !/\r?\n/g.test(fixedNewItems[0].value) && lgOption?.templateId) { + onUpdateResponseTemplate({ [kind]: { kind, value: [fixedNewItems[0].value], valueType: 'direct' } }); onTemplateChange(id, ''); } else { setTemplateId(id); onUpdateResponseTemplate({ [kind]: { kind, value: [`\${${id}()}`], valueType: 'template' } }); - onTemplateChange(id, fixedNewItems.map((item) => `- ${item}`).join('\n')); + onTemplateChange(id, templateBodyItemsToString(fixedNewItems)); } }, [kind, newTemplateNameSuffix, lgOption, templateId, onRemoveTemplate, onTemplateChange, onUpdateResponseTemplate] diff --git a/Composer/packages/lib/code-editor/src/lg/modalityEditors/StringArrayEditor.tsx b/Composer/packages/lib/code-editor/src/lg/modalityEditors/StringArrayEditor.tsx index 40e9fb1345..26ee39b5a6 100644 --- a/Composer/packages/lib/code-editor/src/lg/modalityEditors/StringArrayEditor.tsx +++ b/Composer/packages/lib/code-editor/src/lg/modalityEditors/StringArrayEditor.tsx @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import { LgTemplate, TelemetryClient } from '@bfc/shared'; +import { LgTemplate, TelemetryClient, TemplateBodyItem } from '@bfc/shared'; import { FluentTheme } from '@uifabric/fluent-theme'; import formatMessage from 'format-message'; import { Callout, DirectionalHint } from 'office-ui-fabric-react/lib/Callout'; @@ -55,12 +55,12 @@ const getSSMLProps = (tag: 'prosody' | 'audio' | 'break'): string => { type StringArrayEditorProps = { addButtonText?: string; - items: string[]; + items: TemplateBodyItem[]; lgTemplates?: readonly LgTemplate[]; memoryVariables?: readonly string[]; lgOption?: LGOption; isSpeech?: boolean; - onChange: (items: string[]) => void; + onChange: (items: TemplateBodyItem[]) => void; telemetryClient: TelemetryClient; }; @@ -76,13 +76,15 @@ export const StringArrayEditor = React.memo( }: StringArrayEditorProps) => { const containerRef = useRef(null); - const [currentIndex, setCurrentIndex] = useState(items.length === 1 && items[0] === '' ? 0 : null); + const [currentIndex, setCurrentIndex] = useState( + items.length === 1 && items[0].value === '' ? 0 : null + ); const [calloutTargetElement, setCalloutTargetElement] = useState(null); const onItemChange = useCallback( (index: number) => (_, newValue?: string) => { const updatedItems = [...items]; - updatedItems[index] = newValue ?? ''; + updatedItems[index].value = newValue ?? ''; onChange(updatedItems); }, [items, onChange] @@ -108,7 +110,7 @@ export const StringArrayEditor = React.memo( ); const onClickAddVariation = useCallback(() => { - onChange([...items, '']); + onChange([...items, { kind: 'variation', value: '' }]); setCurrentIndex(items.length); }, [items, onChange]); @@ -129,15 +131,15 @@ export const StringArrayEditor = React.memo( } setCalloutTargetElement(null); - // Filter our empty or newline strings - const filteredItems = items.filter((s) => s !== '' && s !== '\n'); + // Filter out empty variations + const filteredItems = items.filter((item) => item.kind !== 'variation' || !!item.value); if (e.key === 'Enter' && containerRef.current?.contains(e.target as Node)) { // If the value is not filtered, go to the next entry // Otherwise cancel editing if (items.length === filteredItems.length) { e.preventDefault(); - onChange([...filteredItems, '']); - setCurrentIndex(filteredItems.length); + onChange([...items, { kind: 'variation', value: '' }]); + setCurrentIndex(items.length); } else { onChange(filteredItems); setCurrentIndex(null); @@ -167,8 +169,8 @@ export const StringArrayEditor = React.memo( setCalloutTargetElement(null); setCurrentIndex(null); // Remove empty variations only if necessary - if (items.some((item) => !item)) { - onChange(items.filter(Boolean)); + if (items.some((item) => item.kind === 'variation' && !item.value)) { + onChange(items.filter((item) => item.kind !== 'variation' || !!item.value)); } } }; @@ -194,15 +196,15 @@ export const StringArrayEditor = React.memo( typeof calloutTargetElement?.selectionEnd === 'number' ? calloutTargetElement.selectionEnd : calloutTargetElement.selectionStart; - const context = getCursorContextWithinLine(item.substring(0, start)); + const context = getCursorContextWithinLine(item.value.substring(0, start)); const insertText = context === 'expression' ? text : `\${${text}}`; - updatedItems[currentIndex] = [item.slice(0, start), insertText, item.slice(end)].join(''); + updatedItems[currentIndex].value = [item.value.slice(0, start), insertText, item.value.slice(end)].join(''); onChange(updatedItems); setTimeout(() => { calloutTargetElement.setSelectionRange( - updatedItems[currentIndex].length, - updatedItems[currentIndex].length + updatedItems[currentIndex].value.length, + updatedItems[currentIndex].value.length ); }, 0); } @@ -238,26 +240,26 @@ export const StringArrayEditor = React.memo( typeof calloutTargetElement?.selectionEnd === 'number' ? calloutTargetElement.selectionEnd : calloutTargetElement.selectionStart; - updatedItems[currentIndex] = [ - item.slice(0, start), + updatedItems[currentIndex].value = [ + item.value.slice(0, start), `<${ssmlTagType} ${getSSMLProps(ssmlTagType)}/>`, - item.slice(end), + item.value.slice(end), ].join(''); } else { - updatedItems[currentIndex] = [ - item.slice(0, start), + updatedItems[currentIndex].value = [ + item.value.slice(0, start), `<${ssmlTagType} ${getSSMLProps(ssmlTagType)}>`, - item.slice(start, end), + item.value.slice(start, end), ``, - item.slice(end), + item.value.slice(end), ].join(''); } onChange(updatedItems); setTimeout(() => { calloutTargetElement.setSelectionRange( - updatedItems[currentIndex].length, - updatedItems[currentIndex].length + updatedItems[currentIndex].value.length, + updatedItems[currentIndex].value.length ); }, 0); } @@ -296,18 +298,20 @@ export const StringArrayEditor = React.memo( return (
- {items.map((value, idx) => ( - - ))} + {items.map((item, idx) => + item.kind === 'variation' ? ( + + ) : null + )} {currentIndex === null && ( {addButtonText ?? formatMessage('Add new variation')} diff --git a/Composer/packages/lib/code-editor/src/lg/modalityEditors/SuggestedActionsModalityEditor.tsx b/Composer/packages/lib/code-editor/src/lg/modalityEditors/SuggestedActionsModalityEditor.tsx index 65c8967c3a..dd1641ab09 100644 --- a/Composer/packages/lib/code-editor/src/lg/modalityEditors/SuggestedActionsModalityEditor.tsx +++ b/Composer/packages/lib/code-editor/src/lg/modalityEditors/SuggestedActionsModalityEditor.tsx @@ -3,6 +3,7 @@ import formatMessage from 'format-message'; import React from 'react'; +import { TemplateBodyItem } from '@bfc/shared'; import { CommonModalityEditorProps, SuggestedActionsStructuredResponseItem } from '../types'; @@ -25,9 +26,10 @@ const SuggestedActionsModalityEditor = React.memo( const [items, setItems] = React.useState(response?.value || []); const onChange = React.useCallback( - (newItems: string[]) => { - setItems(newItems); - onUpdateResponseTemplate({ SuggestedActions: { kind: 'SuggestedActions', value: newItems } }); + (newItems: TemplateBodyItem[]) => { + const newValues = newItems.map((s) => s.value); + setItems(newValues); + onUpdateResponseTemplate({ SuggestedActions: { kind: 'SuggestedActions', value: newValues } }); }, [onUpdateResponseTemplate] ); @@ -45,7 +47,7 @@ const SuggestedActionsModalityEditor = React.memo( > ({ kind: 'variation', value: s }))} lgOption={lgOption} lgTemplates={lgTemplates} memoryVariables={memoryVariables} diff --git a/Composer/packages/lib/shared/__tests__/lgUtils/stringBuilders/lgStringUtils.test.ts b/Composer/packages/lib/shared/__tests__/lgUtils/stringBuilders/lgStringUtils.test.ts new file mode 100644 index 0000000000..0ef0643726 --- /dev/null +++ b/Composer/packages/lib/shared/__tests__/lgUtils/stringBuilders/lgStringUtils.test.ts @@ -0,0 +1,90 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { parseTemplateBody } from '../../../src'; + +const normalTemplateBody = `- variation0 +- variation1 +- variation2 +- variation3`; + +const parsedNormalTemplateBody = [ + { kind: 'variation', value: 'variation0' }, + { kind: 'variation', value: 'variation1' }, + { kind: 'variation', value: 'variation2' }, + { kind: 'variation', value: 'variation3' }, +]; + +const templateBody = `> comment 0 +- variation1 +- variation2 +> comment 1 +- variation3 +- \`\`\` multiline +variation +- with dash +\\- and escaped dash +and normal text +> and this comment! +\\> and escaped comment +\`\`\` + + +> comment 2`; + +const parsedTemplateBody = [ + { kind: 'comment', value: 'comment 0' }, + { kind: 'variation', value: 'variation1' }, + { kind: 'variation', value: 'variation2' }, + { kind: 'comment', value: 'comment 1' }, + { kind: 'variation', value: 'variation3' }, + { + kind: 'variation', + value: `\`\`\` multiline +variation +- with dash +\\- and escaped dash +and normal text +> and this comment! +\\> and escaped comment +\`\`\``, + }, + { kind: 'newline', value: '' }, + { kind: 'newline', value: '' }, + { kind: 'comment', value: 'comment 2' }, +]; + +const oneMultilineVariation = `- \`\`\` multiline +variation +- with dash +\\- and escaped dash +and normal text +> and this comment! +\\> and escaped comment +\`\`\``; + +describe('lgStringUtils', () => { + it('Should parse template body with normal variations', () => { + const items = parseTemplateBody(normalTemplateBody); + expect(items.length).toBe(4); + expect(items).toEqual(parsedNormalTemplateBody); + }); + + it('Should parse template body with variations, multiline variations and comments', () => { + const items = parseTemplateBody(templateBody); + expect(items.length).toBe(9); + expect(items).toEqual(parsedTemplateBody); + }); + + it('Should parse template body with one multiline variation', () => { + const items = parseTemplateBody(oneMultilineVariation); + expect(items.length).toBe(1); + expect(items).toEqual([parsedTemplateBody[5]]); + }); + + it('Should return empty array for undefined template body', () => { + const items = parseTemplateBody(undefined); + expect(items.length).toBe(0); + expect(items).toEqual([]); + }); +}); diff --git a/Composer/packages/lib/shared/src/lgUtils/index.ts b/Composer/packages/lib/shared/src/lgUtils/index.ts index efbedf32c6..665d823de7 100644 --- a/Composer/packages/lib/shared/src/lgUtils/index.ts +++ b/Composer/packages/lib/shared/src/lgUtils/index.ts @@ -7,3 +7,4 @@ export { default as LgTemplateRef } from './models/LgTemplateRef'; export { default as LgType } from './models/LgType'; export { extractLgTemplateRefs } from './parsers/parseLgTemplateRef'; export { LgNamePattern } from './parsers/lgPatterns'; +export { TemplateBodyItem, parseTemplateBody, templateBodyItemsToString } from './stringBuilders/lgStringUtils'; diff --git a/Composer/packages/lib/shared/src/lgUtils/stringBuilders/lgStringUtils.ts b/Composer/packages/lib/shared/src/lgUtils/stringBuilders/lgStringUtils.ts new file mode 100644 index 0000000000..d907bb6bbe --- /dev/null +++ b/Composer/packages/lib/shared/src/lgUtils/stringBuilders/lgStringUtils.ts @@ -0,0 +1,108 @@ +/* eslint-disable security/detect-unsafe-regex */ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +export type TemplateBodyItem = { + kind: 'newline' | 'variation' | 'comment'; + value: string; +}; + +/** + * Returns the template kind based on the item text. + */ +const getItemKind = (itemText: string): TemplateBodyItem['kind'] => { + const firstChar = itemText.charAt(0); + switch (firstChar) { + case '>': + return 'comment'; + case '-': + return 'variation'; + default: + return 'newline'; + } +}; + +/** + * Cleans up the text of the item based on its kind. + */ +const getItemText = (itemText: string, kind: TemplateBodyItem['kind']) => { + switch (kind) { + case 'comment': + return itemText.replace(/(?/, '').trim(); + case 'variation': + return itemText.replace(/(? or another new line +const splitRegex = /\r?\n(?=-|>|\r?\n)/g; +export const parseTemplateBody = (body?: string): TemplateBodyItem[] => { + /** + * Split by newline followed by either another new line, or new variation or a comment + */ + const items = body?.replace(/\r?\n$/g, '')?.split(splitRegex) ?? []; + + let multiLineItem = ''; + const fixedItems: string[] = []; + + /** + * This block fixes the variations that are multiline and include - or > + */ + for (let i = 0; i < items.length; i++) { + const item = items[i]; + if (multiLineItem || item.startsWith('-')) { + // Check if this item is the start of a multiline variation + if (/^-( )*```/.test(item)) { + multiLineItem = item; + // Check if multiline variation ends in the same line (one line) + // If yes, it will start keeping track of the multiline variation + if (/```$/.test(item)) { + fixedItems.push(multiLineItem); + multiLineItem = ''; + } else { + multiLineItem += '\n'; + } + continue; + } + + // Check if this item is the end of a multiline variation + // If yes, it will add the multiline variation into the result array + if (/```$/.test(item)) { + multiLineItem += item; + fixedItems.push(multiLineItem); + multiLineItem = ''; + continue; + } + + if (multiLineItem) { + multiLineItem += `${item}\n`; + } else { + fixedItems.push(item); + } + } else { + fixedItems.push(item); + } + } + + return fixedItems.map((item) => { + const kind = getItemKind(item); + const value = getItemText(item, kind); + return { kind, value }; + }); +}; + +export const templateBodyItemsToString = (items: TemplateBodyItem[]) => + items + .map((item) => { + switch (item.kind) { + case 'comment': + return `> ${item.value}`; + case 'variation': + return `- ${item.value}`; + default: + return ''; + } + }) + .join('\n'); diff --git a/Composer/packages/ui-plugins/lg/src/LgWidget/useLgTemplate.ts b/Composer/packages/ui-plugins/lg/src/LgWidget/useLgTemplate.ts index 628d91fda6..8c924725f6 100644 --- a/Composer/packages/ui-plugins/lg/src/LgWidget/useLgTemplate.ts +++ b/Composer/packages/ui-plugins/lg/src/LgWidget/useLgTemplate.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import { LgTemplateRef } from '@bfc/shared'; +import { LgTemplateRef, parseTemplateBody } from '@bfc/shared'; import { LgTemplate, useShellApi } from '@bfc/extension-client'; import { locateLgTemplatePosition } from '../locateLgTemplatePosition'; @@ -38,17 +38,13 @@ const getLgTemplateTextData = ( const subTemplate = templates.find((x) => x.name === subTemplateId); // If found template and it matches auto-generated names if (subTemplate) { - const lines = - subTemplate.body - // eslint-disable-next-line security/detect-unsafe-regex - .split(/(? s !== '' && s !== '\n') - .map((s) => s.replace(/\r?\n$/g, '')) - // Remove LG template multiline block symbol - .map((s) => s.replace(/```/g, '')) || []; - if (lines.length) { - acc[responseType] = { value: lines[0].replace(/\r?\n/g, '↵'), moreCount: lines.length - 1 }; + const variations = parseTemplateBody(subTemplate.body) + // Remove LG template multiline block symbol + .map((s) => ({ ...s, value: s.value.replace(/```/g, '') })) + .filter((s) => s.kind === 'variation') + .map((s) => s.value); + if (variations.length) { + acc[responseType] = { value: variations[0].replace(/\r?\n/g, '↵'), moreCount: variations.length - 1 }; } } } else {