diff --git a/.changeset/nine-apes-add.md b/.changeset/nine-apes-add.md new file mode 100644 index 00000000000..ac6136faf32 --- /dev/null +++ b/.changeset/nine-apes-add.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Fix `MarkdownEditor` suggestions filtering bug and allow lazy-loading suggestions diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.stories.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.stories.tsx index 25cddfcac10..17b0d3f6314 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.stories.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.stories.tsx @@ -1,6 +1,6 @@ import {DiffIcon} from '@primer/octicons-react' import React, {Meta} from '@storybook/react' -import {useState} from 'react' +import {useRef, useState} from 'react' import BaseStyles from '../../BaseStyles' import Box from '../../Box' import MarkdownEditor, {Emoji, Mentionable, Reference, SavedReply} from '.' @@ -301,3 +301,67 @@ export const CustomButtons = ({ ) } + +function useLazySuggestions(suggestions: T[]) { + const promiseRef = useRef | null>(null) + + return () => { + // This simulates waiting to make an API request until the first time the suggestions are needed + // Then, once we have made the API request we keep returning the same Promise which will already + // be resolved with the cached data + if (!promiseRef.current) { + promiseRef.current = new Promise(resolve => { + setTimeout(() => resolve(suggestions), 500) + }) + } + + return promiseRef.current + } +} + +export const LazyLoadedSuggestions = ({ + disabled, + fullHeight, + monospace, + minHeightLines, + maxHeightLines, + hideLabel, + required, + fileUploadsEnabled, + onSubmit, + savedRepliesEnabled, + pasteUrlsAsPlainText +}: ArgProps) => { + const [value, setValue] = useState('') + + const emojiSuggestions = useLazySuggestions(emojis) + const mentionSuggestions = useLazySuggestions(mentionables) + const referenceSuggestions = useLazySuggestions(references) + + return ( + <> + + Markdown Editor Example + +

Note: for demo purposes, files starting with "A" will be rejected.

+ + ) +} diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx index f730c667fee..3fce74c08ce 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.test.tsx @@ -2,7 +2,7 @@ import {DiffAddedIcon} from '@primer/octicons-react' import {fireEvent, render as _render, waitFor, within} from '@testing-library/react' import userEvent from '@testing-library/user-event' import {UserEvent} from '@testing-library/user-event/dist/types/setup' -import React, {forwardRef, useLayoutEffect, useRef, useState} from 'react' +import React, {forwardRef, useRef, useState} from 'react' import MarkdownEditor, {Emoji, MarkdownEditorHandle, MarkdownEditorProps, Mentionable, Reference, SavedReply} from '.' import ThemeProvider from '../../ThemeProvider' @@ -21,17 +21,6 @@ const UncontrolledEditor = forwardRef 'Preview' - useLayoutEffect(() => { - // combobox-nav attempts to filter out 'hidden' options by checking if the option has an - // offsetHeight or width > 0. In JSDom, all elements have offsetHeight = offsetWidth = 0, - // so we need to override at least one to make the class recognize that any options exist. - for (const option of document.querySelectorAll('[role=option]')) - Object.defineProperty(option, 'offsetHeight', { - value: 1, - writable: true - }) - }) - return ( { } describe('MarkdownEditor', () => { + // combobox-nav attempts to filter out 'hidden' options by checking if the option has an + // offsetHeight or width > 0. In JSDom, all elements have offsetHeight = offsetWidth = 0, + // so we need to override at least one to make the class recognize that any options exist. + const originalOffsetHeight = Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'offsetHeight') + beforeAll(() => { + Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: 10 + }) + }) + afterAll(() => { + if (originalOffsetHeight) Object.defineProperty(HTMLElement.prototype, 'offsetHeight', originalOffsetHeight) + }) + beforeEach(() => { jest.mock('@primer/behaviors/utils', () => ({ // for all tests, default to Non-Mac (Ctrl) keybindings @@ -832,8 +835,6 @@ describe('MarkdownEditor', () => { }) describe('suggestions', () => { - // we don't test filtering logic here because that's up to the consumer - const emojis: Emoji[] = [ {name: '+1', character: '👍'}, {name: '-1', character: '👎'}, @@ -847,7 +848,10 @@ describe('MarkdownEditor', () => { {identifier: 'github', description: 'GitHub'}, {identifier: 'primer', description: 'Primer'}, {identifier: 'actions', description: 'Actions'}, - {identifier: 'primer-css', description: ''} + {identifier: 'primer-css', description: ''}, + {identifier: 'mnl', description: ''}, + {identifier: 'gth', description: ''}, + {identifier: 'mla', description: ''} ] const references: Reference[] = [ @@ -982,6 +986,20 @@ describe('MarkdownEditor', () => { }) }) + it('applies suggestion and hides list on %s-press', async () => { + const {queryForSuggestionsList, getAllSuggestions, getInput, user} = await render() + + const input = getInput() + await user.type(input, `hello :`) + expect(queryForSuggestionsList()).toBeInTheDocument() + + await waitFor(() => expect(getAllSuggestions()[0]).toHaveAttribute('data-combobox-option-default')) + + await user.keyboard(`{Enter}`) + expect(input.value).toBe(`hello 👍 `) // suggestions are inserted with a following space + expect(queryForSuggestionsList()).not.toBeInTheDocument() + }) + it('filters mention suggestions using fuzzy match against name', async () => { const {getInput, getAllSuggestions, user} = await render() await user.type(getInput(), '@octct') @@ -991,9 +1009,9 @@ describe('MarkdownEditor', () => { it('filters mention suggestions using fuzzy match against ID', async () => { const {getInput, getAllSuggestions, user} = await render() - await user.type(getInput(), '@prmrcss') + await user.type(getInput(), '@git') expect(getAllSuggestions()).toHaveLength(1) - expect(getAllSuggestions()[0]).toHaveTextContent('primer-css') + expect(getAllSuggestions()[0]).toHaveTextContent('github') }) it('filters reference suggestions using fuzzy match against name', async () => { diff --git a/src/drafts/MarkdownEditor/MarkdownEditor.tsx b/src/drafts/MarkdownEditor/MarkdownEditor.tsx index 5627bb34c4d..4c62993745e 100644 --- a/src/drafts/MarkdownEditor/MarkdownEditor.tsx +++ b/src/drafts/MarkdownEditor/MarkdownEditor.tsx @@ -24,6 +24,7 @@ import {Emoji} from './suggestions/_useEmojiSuggestions' import {Mentionable} from './suggestions/_useMentionSuggestions' import {Reference} from './suggestions/_useReferenceSuggestions' import {isModifierKey} from './utils' +import {SuggestionOptions} from './suggestions' export type MarkdownEditorProps = SxProp & { /** Current value of the editor as a multiline markdown string. */ @@ -69,12 +70,21 @@ export type MarkdownEditorProps = SxProp & { * @default 35 */ maxHeightLines?: number - /** Array of all possible emojis to suggest. Leave `undefined` to disable emoji autocomplete. */ - emojiSuggestions?: Array - /** Array of all possible mention suggestions. Leave `undefined` to disable `@`-mention autocomplete. */ - mentionSuggestions?: Array - /** Array of all possible references to suggest. Leave `undefined` to disable `#`-reference autocomplete. */ - referenceSuggestions?: Array + /** + * Array of all possible emojis to suggest. Leave `undefined` to disable emoji autocomplete. + * For lazy-loading suggestions, an async function can be provided instead. + */ + emojiSuggestions?: SuggestionOptions + /** + * Array of all possible mention suggestions. Leave `undefined` to disable `@`-mention autocomplete. + * For lazy-loading suggestions, an async function can be provided instead. + */ + mentionSuggestions?: SuggestionOptions + /** + * Array of all possible references to suggest. Leave `undefined` to disable `#`-reference autocomplete. + * For lazy-loading suggestions, an async function can be provided instead. + */ + referenceSuggestions?: SuggestionOptions /** * Uploads a file to a hosting service and returns the URL. If not provided, file uploads * will be disabled. diff --git a/src/drafts/MarkdownEditor/_MarkdownInput.tsx b/src/drafts/MarkdownEditor/_MarkdownInput.tsx index eef06b1d0eb..3a02dff2662 100644 --- a/src/drafts/MarkdownEditor/_MarkdownInput.tsx +++ b/src/drafts/MarkdownEditor/_MarkdownInput.tsx @@ -7,6 +7,7 @@ import {Emoji, useEmojiSuggestions} from './suggestions/_useEmojiSuggestions' import {Mentionable, useMentionSuggestions} from './suggestions/_useMentionSuggestions' import {Reference, useReferenceSuggestions} from './suggestions/_useReferenceSuggestions' import {useRefObjectAsForwardedRef} from '../../hooks' +import {SuggestionOptions} from './suggestions' interface MarkdownInputProps extends Omit { value: string @@ -18,9 +19,9 @@ interface MarkdownInputProps extends Omit { maxLength?: number fullHeight?: boolean isDraggedOver: boolean - emojiSuggestions?: Array - mentionSuggestions?: Array - referenceSuggestions?: Array + emojiSuggestions?: SuggestionOptions + mentionSuggestions?: SuggestionOptions + referenceSuggestions?: SuggestionOptions minHeightLines: number maxHeightLines: number monospace: boolean @@ -70,13 +71,14 @@ export const MarkdownInput = forwardRef [mentionsTrigger, referencesTrigger, emojiTrigger] ) - const onShowSuggestions = (event: ShowSuggestionsEvent) => { + const onShowSuggestions = async (event: ShowSuggestionsEvent) => { + setSuggestions('loading') if (event.trigger.triggerChar === emojiTrigger.triggerChar) { - setSuggestions(calculateEmojiSuggestions(event.query)) + setSuggestions(await calculateEmojiSuggestions(event.query)) } else if (event.trigger.triggerChar === mentionsTrigger.triggerChar) { - setSuggestions(calculateMentionSuggestions(event.query)) + setSuggestions(await calculateMentionSuggestions(event.query)) } else if (event.trigger.triggerChar === referencesTrigger.triggerChar) { - setSuggestions(calculateReferenceSuggestions(event.query)) + setSuggestions(await calculateReferenceSuggestions(event.query)) } } @@ -97,7 +99,7 @@ export const MarkdownInput = forwardRef onShowSuggestions(e)} onHideSuggestions={() => setSuggestions(null)} sx={{flex: 'auto'}} tabInsertsSuggestions diff --git a/src/drafts/MarkdownEditor/suggestions/_useMentionSuggestions.tsx b/src/drafts/MarkdownEditor/suggestions/_useMentionSuggestions.tsx index ab294aeea37..d85f4de1b87 100644 --- a/src/drafts/MarkdownEditor/suggestions/_useMentionSuggestions.tsx +++ b/src/drafts/MarkdownEditor/suggestions/_useMentionSuggestions.tsx @@ -26,8 +26,16 @@ const mentionableToSuggestion = (mentionable: Mentionable): Suggestion => ({ ) }) -const scoreSuggestion = (query: string, mentionable: Mentionable): number => - score(query, `${mentionable.identifier} ${mentionable.description}`.trim().toLowerCase()) +const scoreSuggestion = (query: string, mentionable: Mentionable): number => { + const fzyScore = score(query, `${mentionable.identifier} ${mentionable.description}`.trim().toLowerCase()) + + // fzy unintuitively returns Infinity if the length of the item is less than or equal to the length of the query + // All users have an identifier but some have empty descriptions; in those cases the query might equal the identifier + // and we'd still want to show the suggestion in that case. + if (fzyScore === Infinity && query.toLowerCase() !== mentionable.identifier.toLowerCase()) return -Infinity + + return fzyScore +} export const useMentionSuggestions: UseSuggestionsHook = mentionables => ({ calculateSuggestions: suggestionsCalculator(mentionables, scoreSuggestion, mentionableToSuggestion), diff --git a/src/drafts/MarkdownEditor/suggestions/_useReferenceSuggestions.tsx b/src/drafts/MarkdownEditor/suggestions/_useReferenceSuggestions.tsx index 56c46c493b4..e85ac6512a0 100644 --- a/src/drafts/MarkdownEditor/suggestions/_useReferenceSuggestions.tsx +++ b/src/drafts/MarkdownEditor/suggestions/_useReferenceSuggestions.tsx @@ -43,11 +43,16 @@ const referenceToSuggestion = (reference: Reference): Suggestion => ({ ) }) -const scoreSuggestion = (query: string, reference: Reference): number => - score(query, `${reference.id} ${reference.titleText}`) +const scoreSuggestion = (query: string, reference: Reference): number => { + // fzy unituitively returns Infinity if the length of the item is less than or equal to the length of the query + const fzyScore = score(query, `${reference.id} ${reference.titleText}`) + // Here, unlike for mentionables, we don't need to check for equality because the user's query + // can never equal the search string (we don't do filtering if the query is in "#123 some text" form) + return fzyScore === Infinity ? -Infinity : fzyScore +} export const useReferenceSuggestions: UseSuggestionsHook = references => ({ - calculateSuggestions: (query: string) => { + calculateSuggestions: async (query: string) => { if (/^\d+\s/.test(query)) return [] // don't return anything if the query is in the form #123 ..., assuming they already have the number they want return suggestionsCalculator(references, scoreSuggestion, referenceToSuggestion)(query) }, diff --git a/src/drafts/MarkdownEditor/suggestions/index.ts b/src/drafts/MarkdownEditor/suggestions/index.ts index 8b8ec7a60c6..c7104e623c3 100644 --- a/src/drafts/MarkdownEditor/suggestions/index.ts +++ b/src/drafts/MarkdownEditor/suggestions/index.ts @@ -2,23 +2,31 @@ import {Suggestion, Trigger} from '../../InlineAutocomplete' const MAX_SUGGESTIONS = 5 -export type UseSuggestionsHook = (options: T[]) => { +export type SuggestionOptions = T[] | (() => Promise) + +export type UseSuggestionsHook = (options: SuggestionOptions) => { trigger: Trigger - calculateSuggestions: (query: string) => Suggestion[] + calculateSuggestions: (query: string) => Promise } export const suggestionsCalculator = - (options: T[], score: (query: string, option: T) => number, toSuggestion: (option: T) => Suggestion) => - (query: string) => { + ( + options: SuggestionOptions, + score: (query: string, option: T) => number, + toSuggestion: (option: T) => Suggestion + ) => + async (query: string) => { + const optionsArray = Array.isArray(options) ? options : await options() + // If the query is empty, scores will be -INFINITY const scoredAndSorted = query - ? options + ? optionsArray .map(o => [score(query, o), o] as const) .filter(([s]) => s > 0) .sort(([a], [b]) => b - a) .slice(0, MAX_SUGGESTIONS) .map(([, o]) => o) - : options.slice(0, MAX_SUGGESTIONS) + : optionsArray.slice(0, MAX_SUGGESTIONS) return scoredAndSorted.map(toSuggestion) }