Improve search (make fuzzy again and more robust) #932

nikku · 2024-09-23T16:27:21Z

Proposed Changes

We found various issues with our existing search, both in terms of robustness and behavior:

Under certain circumstances searching loops infinitely (https://github.com/camunda/web-modeler/pull/10940)
Search is no longer fuzzy, i.e. matches terms across all fields, and hence is overly strict (Improve search (make fuzzy again and more robust) #932 (comment))
Search does not return all tokens for matched entries, making it hard to integrate it properly (Use search bpmn-js#2235)
Lack of test coverage around contract and other crucial aspects

This PR solves these issues, while keeping the generally desirable properties:

We sort search results semantically, preferring
- start of word
- start of term (in word)
- we sort alphabetically (where items are equal in semantics)

The overall goal is to offer a snappy, robust, and intuitive search behavior, and I'd like to put up to a test if we accomplished that:

Try it out via

npx @bpmn-io/sr bpmn-io/bpmn-js#use-search -l bpmn-io/diagram-js#fix-search

Checklist

To ensure you provided everything we need to look at your PR:

Brief textual description of the changes present
Visual demo attached
Steps to try out present, i.e. using the @bpmn-io/sr tool
Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

nikku · 2024-09-23T17:45:42Z

Additional finding, debugging the implementation:

We do no longer tokenize between spaces, this makes it impossible to match two works, such as this:

fix: correct popup menu search on duplicate entries Related to bpmn-io/diagram-js#932

Related to bpmn-io/diagram-js#932 and friends.

lmbateman · 2024-10-10T14:33:24Z

Is there a playground where I can try this out? Alternatively, if you need my review, could you please do a quick screen capture to demonstrate the changes? Thanks!

nikku · 2024-10-10T16:09:44Z

Cf. bpmn-io/bpmn-js#2235 (comment). I'll give you an E2E demo once I'm done with implementing the search, unless you want a demo earlier, to discuss what we do (did), and why.

The implementation for comparing entries did not properly account for entries that are "equal", and whenever they occur it ran into a loop. The implementation proposed does two things: * First we simplify the existing logic to be a two step filter + sort mechanism * Second, the search comparison is simple and robust, and handles equality well. This should fix the loop issues we're seeing. Related to camunda/web-modeler#10940

Reverts 34a65d32abc0c80cc822be13c440c61757ae6376

We ensure that for every entry that is a match we _always_ provide tokens. This ensures that a user can rely on these tokens being there, i.e. to render the results. Related to bpmn-io/bpmn-js#2235

nikku · 2024-10-31T17:22:53Z

I updated this PR to be fuzzy again, and solve all other outstanding issues.

Just now tests revealed that we're not fully fuzzy, actually match too many things, cf. original comment in the popup menu that I subscribe to:

// every word of `search term` should be included in one of the searchable fields

Our current behavior, leading to more search results popping up is:

// at least one word of `search term` should be included in one of the searchable fields

Need to do another round tomorrow.

nikku · 2024-10-31T18:15:49Z

Now ready to be reviewed.

lib/features/search/search.js

philippfromme

Great work! 🎖️

This ensures we properly handle fuzzy results (again), where parts of the search is matched across different keys. This ensures we're not overly strict in filtering things. Related to bpmn-io/bpmn-js#2235

Tokens returned by `search` are now recognized by the component, this makes conversion simpler.

misiekhardcore

I did some review up to my limited knowledge. Mostly catching some smaller things

misiekhardcore · 2024-11-01T15:10:45Z

lib/features/search/search.js


-      results.splice(index, 0, result);
+  return items.flatMap((item, idx) => {


idx seems not to be used anywhere

misiekhardcore · 2024-11-01T15:23:57Z

lib/features/popup-menu/PopupMenuComponent.js

@@ -54,6 +54,7 @@ export default function PopupMenuComponent(props) {
    scale,
    search,
    emptyPlaceholder,
+    searchFn,


can we have some type definition for this?

misiekhardcore · 2024-11-01T15:26:35Z

lib/features/search-pad/SearchPadProvider.ts

  matched?: string;
  normal?: string;
 };

+type ModernToken = {


NABD, but maybe calling this just Token is fine? as moving forward this is going to be our default token type right?

misiekhardcore · 2024-11-01T15:33:07Z

lib/features/search-pad/SearchPad.js

    } else {
-      htmlText += escapeHTML(t.normal);


so the normal property is not used anymore? What was the purpose of it before?

misiekhardcore · 2024-11-01T15:37:15Z

test/spec/features/popup-menu/PopupMenuComponentSpec.js

@@ -739,6 +741,7 @@ describe('features/popup-menu - <PopupMenu>', function() {
    const props = {
      entries: [],
      headerEntries: [],
+      searchFn: searchFn,


Suggested change

searchFn: searchFn,

searchFn,

misiekhardcore · 2024-11-01T15:45:12Z

test/spec/features/search-pad/SearchPadSpec.js

@@ -139,6 +139,17 @@ describe('features/searchPad', function() {
          } ];
        }

+        if (pattern === 'modern') {


I think we should check legacy and treat modern as just the new default. But maybe I miss something fundamental here

misiekhardcore · 2024-11-01T15:52:10Z

One general question that crossed my mind when I was playing with it:
Do we want to match partial searches? What I mean
There is an element named User task and user types in the search Ustk so few letters taken from the name in the order of occurance "User task".

bpmn-io-tasks bot added the needs review Review pending label Sep 23, 2024

nikku requested a review from philippfromme September 23, 2024 16:27

bpmn-io-tasks bot assigned nikku Sep 23, 2024

nikku requested a review from barmac September 23, 2024 16:27

nikku mentioned this pull request Sep 23, 2024

chore: unbuild search integration in popup menu #933

Merged

4 tasks

nikku force-pushed the fix-search branch from 187d605 to ebbe69c Compare September 23, 2024 18:24

This comment was marked as outdated.

Sign in to view

nikku added a commit to camunda/camunda-modeler that referenced this pull request Sep 23, 2024

deps: update to [email protected]

a0de2c5

fix: correct popup menu search on duplicate entries Related to bpmn-io/diagram-js#932

nikku mentioned this pull request Sep 23, 2024

Release camunda-modeler v5.28.0 camunda/camunda-modeler#4521

Closed

41 tasks

nikku added a commit to camunda/camunda-bpmn-js that referenced this pull request Sep 23, 2024

deps: bump to [email protected]

2fc3987

Related to bpmn-io/diagram-js#932 and friends.

nikku added the in progress Currently worked on label Sep 25, 2024 — with bpmn-io-tasks

nikku removed the needs review Review pending label Sep 25, 2024

barmac marked this pull request as draft September 27, 2024 13:07

nikku mentioned this pull request Oct 2, 2024

Use search bpmn-io/bpmn-js#2235

Draft

4 tasks

nikku requested a review from lmbateman October 10, 2024 14:03

nikku added the ready Ready to be worked on label Oct 11, 2024 — with bpmn-io-tasks

nikku removed the in progress Currently worked on label Oct 11, 2024

nikku force-pushed the fix-search branch 2 times, most recently from 9b3228e to 1fbd403 Compare October 31, 2024 16:44

nikku changed the title ~~fix(search): correctly handle duplicate entries~~ Improve search (make fuzzy again and more robust) Oct 31, 2024

nikku marked this pull request as ready for review October 31, 2024 16:55

bpmn-io-tasks bot added needs review Review pending and removed ready Ready to be worked on labels Oct 31, 2024

nikku requested a review from GeethaParthasarathy October 31, 2024 16:56

nikku force-pushed the fix-search branch from 1fbd403 to 6c502b7 Compare October 31, 2024 16:57

nikku added 2 commits October 31, 2024 18:04

test(search): verify partial match

9ebd8fd

nikku added 4 commits October 31, 2024 18:04

feat(popup-menu): integrate popup menu into search component

0b6297e

Reverts 34a65d32abc0c80cc822be13c440c61757ae6376

test(popup-menu): restore fuzzy search test

aa2d989

fix(search): search with whitespace

36f0971

feat(search): always provide tokens

7b966ce

We ensure that for every entry that is a match we _always_ provide tokens. This ensures that a user can rely on these tokens being there, i.e. to render the results. Related to bpmn-io/bpmn-js#2235

nikku force-pushed the fix-search branch from 6c502b7 to e1fe8ef Compare October 31, 2024 17:04

nikku force-pushed the fix-search branch 2 times, most recently from a68c85e to 08996a2 Compare November 1, 2024 12:38

philippfromme reviewed Nov 1, 2024

View reviewed changes

lib/features/search/search.js Outdated Show resolved Hide resolved

philippfromme self-requested a review November 1, 2024 15:08

philippfromme approved these changes Nov 1, 2024

View reviewed changes

nikku added 4 commits November 1, 2024 16:19

feat(search): find search terms across all keys

c647042

This ensures we properly handle fuzzy results (again), where parts of the search is matched across different keys. This ensures we're not overly strict in filtering things. Related to bpmn-io/bpmn-js#2235

test: correct search spec title

81ad9b5

style: simplify

73d2bd8

feat(search-pad): recognize modern tokens

688cc11

Tokens returned by `search` are now recognized by the component, this makes conversion simpler.

nikku force-pushed the fix-search branch from 08996a2 to 688cc11 Compare November 1, 2024 15:19

misiekhardcore reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve search (make fuzzy again and more robust) #932

Improve search (make fuzzy again and more robust) #932

nikku commented Sep 23, 2024 •

edited

Loading

nikku commented Sep 23, 2024

This comment was marked as outdated.

lmbateman commented Oct 10, 2024

nikku commented Oct 10, 2024

nikku commented Oct 31, 2024 •

edited

Loading

nikku commented Oct 31, 2024

philippfromme left a comment

misiekhardcore left a comment

misiekhardcore Nov 1, 2024

misiekhardcore Nov 1, 2024

misiekhardcore Nov 1, 2024

misiekhardcore Nov 1, 2024

misiekhardcore Nov 1, 2024

misiekhardcore Nov 1, 2024

misiekhardcore commented Nov 1, 2024


		results.splice(index, 0, result);
		return items.flatMap((item, idx) => {

Improve search (make fuzzy again and more robust) #932

Are you sure you want to change the base?

Improve search (make fuzzy again and more robust) #932

Conversation

nikku commented Sep 23, 2024 • edited Loading

Proposed Changes

Checklist

nikku commented Sep 23, 2024

This comment was marked as outdated.

lmbateman commented Oct 10, 2024

nikku commented Oct 10, 2024

nikku commented Oct 31, 2024 • edited Loading

nikku commented Oct 31, 2024

philippfromme left a comment

Choose a reason for hiding this comment

misiekhardcore left a comment

Choose a reason for hiding this comment

misiekhardcore Nov 1, 2024

Choose a reason for hiding this comment

misiekhardcore Nov 1, 2024

Choose a reason for hiding this comment

misiekhardcore Nov 1, 2024

Choose a reason for hiding this comment

misiekhardcore Nov 1, 2024

Choose a reason for hiding this comment

misiekhardcore Nov 1, 2024

Choose a reason for hiding this comment

misiekhardcore Nov 1, 2024

Choose a reason for hiding this comment

misiekhardcore commented Nov 1, 2024

nikku commented Sep 23, 2024 •

edited

Loading

nikku commented Oct 31, 2024 •

edited

Loading