-
Notifications
You must be signed in to change notification settings - Fork 4
[ENG-1224 & ENG-343] Replaced fuzzy with minisearch in Discourse Summoning Menu #694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| /* eslint-disable @typescript-eslint/no-redundant-type-constituents */ | ||
| import React, { | ||
| useCallback, | ||
| useEffect, | ||
|
|
@@ -25,7 +26,7 @@ import getDiscourseNodes, { DiscourseNode } from "~/utils/getDiscourseNodes"; | |
| import getDiscourseNodeFormatExpression from "~/utils/getDiscourseNodeFormatExpression"; | ||
| import { Result } from "~/utils/types"; | ||
| import { getSetting } from "~/utils/extensionSettings"; | ||
| import fuzzy from "fuzzy"; | ||
| import MiniSearch from "minisearch"; | ||
|
|
||
| type Props = { | ||
| textarea: HTMLTextAreaElement; | ||
|
|
@@ -34,6 +35,13 @@ type Props = { | |
| triggerText: string; | ||
| }; | ||
|
|
||
| type MinisearchResult = Result & { | ||
| type: string; | ||
| }; | ||
|
|
||
| const MIN_SEARCH_SCORE = 0.1; | ||
| const MAX_ITEMS_PER_TYPE = 10; | ||
|
|
||
| const waitForBlock = ({ | ||
| uid, | ||
| text, | ||
|
|
@@ -76,7 +84,6 @@ const NodeSearchMenu = ({ | |
| const [discourseTypes, setDiscourseTypes] = useState<DiscourseNode[]>([]); | ||
| const [checkedTypes, setCheckedTypes] = useState<Record<string, boolean>>({}); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
| const [allNodes, setAllNodes] = useState<Record<string, Result[]>>({}); | ||
| const [searchResults, setSearchResults] = useState<Record<string, Result[]>>( | ||
| {}, | ||
| ); | ||
|
|
@@ -91,6 +98,7 @@ const NodeSearchMenu = ({ | |
| ); | ||
| const scrollContainerRef = useRef<HTMLDivElement | null>(null); | ||
| const searchTimeoutRef = useRef<NodeJS.Timeout | null>(null); | ||
| const miniSearchRef = useRef<MiniSearch<MinisearchResult> | null>(null); | ||
| const POPOVER_TOP_OFFSET = 30; | ||
|
|
||
| const debouncedSearchTerm = useCallback((term: string) => { | ||
|
|
@@ -135,16 +143,77 @@ const NodeSearchMenu = ({ | |
| } | ||
| }; | ||
|
|
||
| const filterNodesLocally = useCallback( | ||
| (nodes: Result[], searchTerm: string): Result[] => { | ||
| if (!searchTerm.trim()) return nodes; | ||
| const searchWithMiniSearch = useCallback( | ||
| (searchTerm: string, typeFilter?: string[]): Record<string, Result[]> => { | ||
| if (!miniSearchRef.current) { | ||
| return {}; | ||
| } | ||
|
|
||
| return fuzzy | ||
| .filter(searchTerm, nodes, { | ||
| extract: (node) => node.text, | ||
| }) | ||
| .map((result) => result.original) | ||
| .filter((node): node is Result => !!node); | ||
| const search = miniSearchRef.current; | ||
|
|
||
| if (!searchTerm.trim()) { | ||
| if (!typeFilter) { | ||
| return {}; | ||
| } | ||
|
|
||
| const allResults: Record<string, Result[]> = {}; | ||
| typeFilter.forEach((type) => { | ||
| const results = ( | ||
| search.search(MiniSearch.wildcard, { | ||
| filter: (result) => | ||
| (result as unknown as MinisearchResult).type === type, | ||
| }) as unknown as MinisearchResult[] | ||
| ).map((r) => ({ | ||
| text: r.text, | ||
| uid: r.uid, | ||
| })); | ||
| allResults[type] = results; | ||
| }); | ||
|
|
||
| return allResults; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const rawSearchResults = search.search(searchTerm, { | ||
| fields: ["text"], | ||
| fuzzy: 0.2, | ||
| prefix: true, | ||
| combineWith: "AND", | ||
| filter: typeFilter | ||
| ? (result) => | ||
| typeFilter.includes((result as unknown as MinisearchResult).type) | ||
| : undefined, | ||
| }); | ||
|
|
||
| const filteredResults = rawSearchResults.filter( | ||
| (r) => r.score > MIN_SEARCH_SCORE, | ||
| ); | ||
|
|
||
| const searchResults = ( | ||
| filteredResults as unknown as MinisearchResult[] | ||
| ).map((r) => ({ | ||
| text: r.text, | ||
| uid: r.uid, | ||
| type: r.type, | ||
| })); | ||
|
|
||
| const results = searchResults.reduce( | ||
| (acc, result) => { | ||
| if (!acc[result.type]) { | ||
| acc[result.type] = []; | ||
| } | ||
| if (acc[result.type].length < MAX_ITEMS_PER_TYPE) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| acc[result.type].push({ | ||
| text: result.text, | ||
| uid: result.uid, | ||
| }); | ||
|
Comment on lines
+206
to
+209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Missing The new MiniSearch implementation fails to include the Click to expandHow the bug occursIn the original implementation using fuzzy search, the return results.map(([result]: any) => ({
id: result.uid,
text: result.title || result.string,
uid: result.uid,
}));However, in the new
Later at line 375, the code accesses posthog.capture("Discourse Node: Selected from Search Menu", {
id: item.id, // This will be undefined
text: item.text,
});ImpactAnalytics events will have Recommendation: Add the acc[result.type].push({
id: result.uid,
text: result.text,
uid: result.uid,
});Similarly update lines 166-169. Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } | ||
| return acc; | ||
| }, | ||
| {} as Record<string, Result[]>, | ||
| ); | ||
|
|
||
| return results; | ||
| }, | ||
| [], | ||
| ); | ||
|
|
@@ -169,7 +238,27 @@ const NodeSearchMenu = ({ | |
| allNodeTypes.forEach((type) => { | ||
| allNodesCache[type.type] = searchNodesForType(type); | ||
| }); | ||
| setAllNodes(allNodesCache); | ||
|
|
||
| const miniSearch = new MiniSearch<MinisearchResult>({ | ||
| fields: ["text"], | ||
| storeFields: ["text", "uid", "type"], | ||
| idField: "uid", | ||
| }); | ||
|
|
||
| const documentsToIndex: MinisearchResult[] = []; | ||
|
|
||
| allNodeTypes.forEach((type) => { | ||
| const nodes = allNodesCache[type.type] || []; | ||
| nodes.forEach((node) => { | ||
| documentsToIndex.push({ | ||
| ...node, | ||
| type: type.type, | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| miniSearch.addAll(documentsToIndex); | ||
| miniSearchRef.current = miniSearch; | ||
|
|
||
| const initialSearchResults = Object.fromEntries( | ||
| allNodeTypes.map((type) => [type.type, []]), | ||
|
|
@@ -183,17 +272,21 @@ const NodeSearchMenu = ({ | |
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (isLoading || Object.keys(allNodes).length === 0) return; | ||
| if (isLoading || !miniSearchRef.current) return; | ||
|
|
||
| const newResults: Record<string, Result[]> = {}; | ||
|
|
||
| discourseTypes.forEach((type) => { | ||
| const cachedNodes = allNodes[type.type] || []; | ||
| newResults[type.type] = filterNodesLocally(cachedNodes, searchTerm); | ||
| }); | ||
| const selectedTypes = discourseTypes | ||
| .filter((type) => checkedTypes[type.type]) | ||
| .map((type) => type.type); | ||
|
|
||
| const newResults = searchWithMiniSearch(searchTerm, selectedTypes); | ||
| setSearchResults(newResults); | ||
| }, [searchTerm, isLoading, allNodes, discourseTypes, filterNodesLocally]); | ||
| }, [ | ||
| searchTerm, | ||
| isLoading, | ||
| discourseTypes, | ||
| checkedTypes, | ||
| searchWithMiniSearch, | ||
| ]); | ||
|
|
||
| const menuRef = useRef<HTMLUListElement>(null); | ||
| const { ["block-uid"]: blockUid, ["window-id"]: windowId } = useMemo( | ||
|
|
@@ -232,61 +325,61 @@ const NodeSearchMenu = ({ | |
|
|
||
| const onSelect = useCallback( | ||
| (item: Result) => { | ||
| if (!blockUid) { | ||
| onClose(); | ||
| return; | ||
| } | ||
| void waitForBlock({ uid: blockUid, text: textarea.value }) | ||
| .then(() => { | ||
| onClose(); | ||
|
|
||
| setTimeout(() => { | ||
| const originalText = getTextByBlockUid(blockUid); | ||
|
|
||
| const prefix = originalText.substring(0, triggerPosition); | ||
| const suffix = originalText.substring(textarea.selectionStart); | ||
| const pageRef = `[[${item.text}]]`; | ||
|
|
||
| const newText = `${prefix}${pageRef}${suffix}`; | ||
| void updateBlock({ uid: blockUid, text: newText }).then(() => { | ||
| const newCursorPosition = triggerPosition + pageRef.length; | ||
|
|
||
| if (window.roamAlphaAPI.ui.setBlockFocusAndSelection) { | ||
| void window.roamAlphaAPI.ui.setBlockFocusAndSelection({ | ||
| location: { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| "block-uid": blockUid, | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| "window-id": windowId, | ||
| }, | ||
| selection: { start: newCursorPosition }, | ||
| }); | ||
| } else { | ||
| setTimeout(() => { | ||
| const textareaElements = | ||
| document.querySelectorAll("textarea"); | ||
| for (const el of textareaElements) { | ||
| if (getUids(el).blockUid === blockUid) { | ||
| el.focus(); | ||
| el.setSelectionRange( | ||
| newCursorPosition, | ||
| newCursorPosition, | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
| }, 50); | ||
| } | ||
| }); | ||
| posthog.capture("Discourse Node: Selected from Search Menu", { | ||
| id: item.id, | ||
| text: item.text, | ||
| }); | ||
| }, 10); | ||
| }) | ||
| .catch((error) => { | ||
| console.error("Error waiting for block:", error); | ||
| }); | ||
| if (!blockUid) { | ||
| onClose(); | ||
| return; | ||
| } | ||
| void waitForBlock({ uid: blockUid, text: textarea.value }) | ||
| .then(() => { | ||
| onClose(); | ||
|
|
||
| setTimeout(() => { | ||
| const originalText = getTextByBlockUid(blockUid); | ||
|
|
||
| const prefix = originalText.substring(0, triggerPosition); | ||
| const suffix = originalText.substring(textarea.selectionStart); | ||
| const pageRef = `[[${item.text}]]`; | ||
|
|
||
| const newText = `${prefix}${pageRef}${suffix}`; | ||
| void updateBlock({ uid: blockUid, text: newText }).then(() => { | ||
| const newCursorPosition = triggerPosition + pageRef.length; | ||
|
|
||
| if (window.roamAlphaAPI.ui.setBlockFocusAndSelection) { | ||
| void window.roamAlphaAPI.ui.setBlockFocusAndSelection({ | ||
| location: { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| "block-uid": blockUid, | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| "window-id": windowId, | ||
| }, | ||
| selection: { start: newCursorPosition }, | ||
| }); | ||
| } else { | ||
| setTimeout(() => { | ||
| const textareaElements = | ||
| document.querySelectorAll("textarea"); | ||
| for (const el of textareaElements) { | ||
| if (getUids(el).blockUid === blockUid) { | ||
| el.focus(); | ||
| el.setSelectionRange( | ||
| newCursorPosition, | ||
| newCursorPosition, | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
| }, 50); | ||
| } | ||
| }); | ||
| posthog.capture("Discourse Node: Selected from Search Menu", { | ||
| id: item.id, | ||
| text: item.text, | ||
| }); | ||
trangdoan982 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, 10); | ||
| }) | ||
| .catch((error) => { | ||
| console.error("Error waiting for block:", error); | ||
| }); | ||
| }, | ||
| [blockUid, onClose, textarea, triggerPosition, windowId], | ||
| ); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.