fix: prevent flashing of search results during input #7592

Vardhaman619 · 2024-10-11T11:37:50Z

greptile-apps

PR Summary

This PR addresses the issue of search results flashing in the CommandMenu component by implementing debouncing for the search input.

Added useDebounce hook from 'use-debounce' library in packages/twenty-front/src/modules/command-menu/components/CommandMenu.tsx
Implemented deferredCommandMenuSearch to replace commandMenuSearch for smoother updates
Introduced loading states for different record types (people, companies, notes, opportunities)
Added isLoading check to prevent premature "No results found" display
These changes aim to improve user experience by reducing frequent updates and eliminating the flashing effect

_{1 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-11T11:38:23Z

packages/twenty-front/src/modules/command-menu/components/CommandMenu.tsx

@@ -139,7 +140,21 @@ export const CommandMenu = () => {
  const [commandMenuSearch, setCommandMenuSearch] = useRecoilState(
    commandMenuSearchState,
  );
+  // Use useDeferredValue to delay the search results update
+  // const deferredCommandMenuSearch = useDeferredValue(commandMenuSearch);
+  const [deferredCommandMenuSearch] = useDebounce(commandMenuSearch, 300);


style: Consider increasing the debounce delay to 500ms for a better balance between responsiveness and performance

greptile-apps · 2024-10-11T11:38:23Z

packages/twenty-front/src/modules/command-menu/components/CommandMenu.tsx

+  const isNoResults =
+    !matchingCreateCommand.length &&
+    !matchingNavigateCommand.length &&
+    !people.length &&
+    !companies.length &&
+    !notes.length &&
+    !opportunities.length;


style: This check for isNoResults might be redundant with the isLoading check. Consider combining them

Devessier

Thank you a lot for contributing to Twenty!

Introducing debouncing is a great idea. It's an important feature for comboboxes, to me.

I found a edge case and wrote about the solution I would try to fix the problem. These kinds of UI components are complex and have many edge cases. They are always tricky to implement well 😄

Devessier · 2024-10-11T14:29:21Z

packages/twenty-front/src/modules/command-menu/components/CommandMenu.tsx

+  // const [previousResults, setPreviousResults] = useState<{
+  //   people: Person[];
+  //   companies: Company[];
+  //   notes: Note[];
+  //   opportunities: Opportunity[];
+  // }>({
+  //   people: [],
+  //   companies: [],
+  //   notes: [],
+  //   opportunities: [],
+  // });


We should drop the comments.

I just saw that you tried to store the previous results. That might be a good start to implement the solution I mentioned.

Devessier · 2024-10-11T14:29:36Z

packages/twenty-front/src/modules/command-menu/components/CommandMenu.tsx

+  // Use useDeferredValue to delay the search results update
+  // const deferredCommandMenuSearch = useDeferredValue(commandMenuSearch);


We should drop the comments.

I've never used the useDeferredValue and am unsure it's the best way to solve our problem. However, I'm sure that we should use that hook with debouncing to limit the amount of requests we make.

Devessier · 2024-10-11T14:57:16Z

packages/twenty-front/src/modules/command-menu/components/CommandMenu.tsx

+                  {isNoResults && !isLoading && (
+                    <StyledEmpty>No results found</StyledEmpty>
+                  )}


I think this condition doesn't provide the expect behavior when the previous queries returned no result and the new one returns nothing too.

CleanShot.2024-10-11.at.16.54.45.mp4

To prevent flashes, here is what I would do.

While the user types a query or we fetch the data for this query, we want to keep displaying the old results, the last results the server returned to us. The issue is that the useSearchRecords function returns an empty array of records when it starts refetching. We should be able to access the old results until we receive a successful response.

We should also consider the error case. If the previous results couldn't be fetched because of an error, we have nothing to display while fetching the new results. We can display a blank screen or a loading indicator. I don't have design opinions. Maybe @Bonapara has.

I implemented a similar feature here: https://xstatebyexample.com/search-as-you-type/. We are not using XState in our project, and I'm not suggesting using it. That's just a comparison. Furthermore, here we are implementing a UI/UX pattern called a "combobox" and we might envision in the future having reusable toolkits to implement that at several places in the app 😄

charlesBochet

@Vardhaman619 thanks for the PR, here are my feedbacks:

let's remove the isSearchEnabled flag to simplify the logic. The new search is fully rolled out, let's use useSearchRecords and that's it
I'm not sure why you need to store previousPeopleFind and co, why not use directly the result from useSarchRecord hook

I think 1) and 2) should greatly simplify the code

charlesBochet

That's way better, thank you!

charlesBochet · 2024-10-14T06:24:31Z

While playing with it it seems that the results are not cleared if there is no results. For instance when you type something (getting results) and then erase your search.
Could you look into that @Vardhaman619 ?

Vardhaman619 · 2024-10-14T06:27:27Z

While playing with it it seems that the results are not cleared if there is no results. For instance when you type something (getting results) and then erase your search. Could you look into that @Vardhaman619 ?

Thank you for the feedback! Could you please provide a video or screenshot demonstrating the issue? That would help me understand the behavior better and address it more efficiently.

charlesBochet · 2024-10-14T06:29:40Z

Actually, I've found the issue and pushed a small fix, LGTM!

oss-gg · 2024-10-14T06:29:57Z

Awarding Vardhaman619: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/Vardhaman619

github-actions · 2024-10-14T06:30:57Z

	Fails
🚫	`node` failed.

Log

�[31mError: �[39m RequestError [HttpError]: You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID D02F:1221AF:7BFC3F9:EB7D552:670CBAA0.
    at /home/runner/work/twenty/twenty/node_modules/�[4m@octokit�[24m/request/dist-node/index.js:86:21
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m {
  status: �[33m403�[39m,
  response: {
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3AVardhaman619%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    status: �[33m403�[39m,
    headers: {
      �[32m'access-control-allow-origin'�[39m: �[32m'*'�[39m,
      �[32m'access-control-expose-headers'�[39m: �[32m'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'�[39m,
      connection: �[32m'close'�[39m,
      �[32m'content-encoding'�[39m: �[32m'gzip'�[39m,
      �[32m'content-security-policy'�[39m: �[32m"default-src 'none'"�[39m,
      �[32m'content-type'�[39m: �[32m'application/json; charset=utf-8'�[39m,
      date: �[32m'Mon, 14 Oct 2024 06:30:56 GMT'�[39m,
      �[32m'referrer-policy'�[39m: �[32m'origin-when-cross-origin, strict-origin-when-cross-origin'�[39m,
      server: �[32m'github.com'�[39m,
      �[32m'strict-transport-security'�[39m: �[32m'max-age=31536000; includeSubdomains; preload'�[39m,
      �[32m'transfer-encoding'�[39m: �[32m'chunked'�[39m,
      vary: �[32m'Accept-Encoding, Accept, X-Requested-With'�[39m,
      �[32m'x-content-type-options'�[39m: �[32m'nosniff'�[39m,
      �[32m'x-frame-options'�[39m: �[32m'deny'�[39m,
      �[32m'x-github-api-version-selected'�[39m: �[32m'2022-11-28'�[39m,
      �[32m'x-github-media-type'�[39m: �[32m'github.v3; format=json'�[39m,
      �[32m'x-github-request-id'�[39m: �[32m'D02F:1221AF:7BFC3F9:EB7D552:670CBAA0'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1728887516'�[39m,
      �[32m'x-ratelimit-resource'�[39m: �[32m'search'�[39m,
      �[32m'x-ratelimit-used'�[39m: �[32m'1'�[39m,
      �[32m'x-xss-protection'�[39m: �[32m'0'�[39m
    },
    data: {
      documentation_url: �[32m'https://docs.github.com/free-pro-team@latest/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits'�[39m,
      message: �[32m'You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID D02F:1221AF:7BFC3F9:EB7D552:670CBAA0.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3AVardhaman619%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    headers: {
      accept: �[32m'application/vnd.github.v3+json'�[39m,
      �[32m'user-agent'�[39m: �[32m'octokit-rest.js/18.12.0 octokit-core.js/3.6.0 Node.js/18.20.4 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-b52bd8da.json

Generated by 🚫 dangerJS against e1a410c

charlesBochet · 2024-10-14T06:31:24Z

/award 150

oss-gg · 2024-10-14T06:31:27Z

Awarding Vardhaman619: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/Vardhaman619

fix: prevent flashing of search results during input

a33aaa2

greptile-apps bot reviewed Oct 11, 2024

View reviewed changes

charlesBochet assigned ijreilly Oct 11, 2024

charlesBochet added -PR: awaiting review and removed -PR: awaiting review labels Oct 11, 2024

charlesBochet unassigned ijreilly Oct 11, 2024

Devessier self-assigned this Oct 11, 2024

Devessier reviewed Oct 11, 2024

View reviewed changes

Devessier removed their assignment Oct 11, 2024

Devessier added -PR: awaiting author and removed -PR: awaiting review labels Oct 11, 2024

refactor : store previous results into state

38267db

Vardhaman619 requested a review from Devessier October 13, 2024 06:23

charlesBochet added -PR: awaiting review and removed -PR: awaiting author labels Oct 13, 2024

charlesBochet self-assigned this Oct 13, 2024

charlesBochet reviewed Oct 13, 2024

View reviewed changes

charlesBochet added -PR: awaiting author and removed -PR: awaiting review labels Oct 13, 2024

charlesBochet assigned Devessier and unassigned charlesBochet Oct 13, 2024

refactor: remove isSearchEnabled flag and unnecessary code

9905475

charlesBochet approved these changes Oct 14, 2024

View reviewed changes

Fix

e1a410c

charlesBochet merged commit 1a57dd6 into twentyhq:main Oct 14, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent flashing of search results during input #7592

fix: prevent flashing of search results during input #7592

Vardhaman619 commented Oct 11, 2024

greptile-apps bot left a comment

greptile-apps bot Oct 11, 2024

greptile-apps bot Oct 11, 2024

Devessier left a comment

Devessier Oct 11, 2024

Devessier Oct 11, 2024

Devessier Oct 11, 2024

Devessier Oct 11, 2024 •

edited

Loading

Devessier Oct 11, 2024

Devessier Oct 11, 2024

charlesBochet left a comment

charlesBochet left a comment

charlesBochet commented Oct 14, 2024

Vardhaman619 commented Oct 14, 2024

charlesBochet commented Oct 14, 2024

oss-gg bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

charlesBochet commented Oct 14, 2024

oss-gg bot commented Oct 14, 2024

		// Use useDeferredValue to delay the search results update
		// const deferredCommandMenuSearch = useDeferredValue(commandMenuSearch);

fix: prevent flashing of search results during input #7592

fix: prevent flashing of search results during input #7592

Conversation

Vardhaman619 commented Oct 11, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 11, 2024

Choose a reason for hiding this comment

Devessier left a comment

Choose a reason for hiding this comment

Devessier Oct 11, 2024

Choose a reason for hiding this comment

Devessier Oct 11, 2024

Choose a reason for hiding this comment

Devessier Oct 11, 2024

Choose a reason for hiding this comment

Devessier Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Devessier Oct 11, 2024

Choose a reason for hiding this comment

Devessier Oct 11, 2024

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

charlesBochet commented Oct 14, 2024

Vardhaman619 commented Oct 14, 2024

charlesBochet commented Oct 14, 2024

oss-gg bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

Log

charlesBochet commented Oct 14, 2024

oss-gg bot commented Oct 14, 2024

Devessier Oct 11, 2024 •

edited

Loading