Skip to content
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

Desktop: Search matches sometimes not shown in CM5 editor #9926

Closed
personalizedrefrigerator opened this issue Feb 12, 2024 · 3 comments
Closed
Labels
bug It's a bug desktop All desktop platforms stale An issue that hasn't been active for a while...

Comments

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Feb 12, 2024

Operating system

Linux

Joplin version

2.13.15

Desktop version info

Joplin 2.13.15 (prod, linux)

Client ID: 24b2a961cccf40fe8448cdc0930b6fce
Sync Version: 3
Profile Version: 44
Keychain Supported: No

Revision: 7d2c1c0

Freehand Drawing: 2.7.1
RevealJS Integration: 0.7.0
Simple Backup: 1.3.6
Work tracker: 0.0.1

Current behaviour

  1. Start Joplin (legacy CM5 editor)
  2. Quickly search for "References"
  3. Navigate to any result
  4. Verify that no results are highlighted in the editor
  5. Backspace the "s"
  6. Wait one second
  7. Retype the "s"
  8. Verify that results are highlighted in the editor

Expected behaviour

Results should always be highlighted in the editor.

Logs

No response

@personalizedrefrigerator personalizedrefrigerator added the bug It's a bug label Feb 12, 2024
@personalizedrefrigerator
Copy link
Collaborator Author

This seems to be a race condition related to our usage of debounce within a useEffect.

if (webviewRef.current && (props.searchMarkers !== previousSearchMarkers || textChanged)) {
webviewRef.current.send('setMarkers', props.searchMarkers.keywords, props.searchMarkers.options);
if (editorRef.current) {
// Fixes https://github.com/laurent22/joplin/issues/7565
const debouncedMarkers = debounce(() => {
const matches = editorRef.current.setMarkers(props.searchMarkers.keywords, props.searchMarkers.options);
props.setLocalSearchResultCount(matches);
}, 50);
debouncedMarkers();
return () => {
debouncedMarkers.clear();
};
}
}
return () => {};
// eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied
}, [props.searchMarkers, previousSearchMarkers, props.setLocalSearchResultCount, props.content, previousContent, renderedBody, previousRenderedBody, renderedBody]);

  • Currently, the function passed to debounce is called with a 50ms delay unless debouncedMarkers.clear is called.
  • debouncedMarkers.clear can be called if a prop changes very soon after we register the debounce function.
  • Depending on which prop changes, (props.searchMarkers !== previousSearchMarkers || textChanged) may no longer be true.

Copy link
Contributor

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? If you require support or are requesting an enhancement or feature then please create a topic on the Joplin forum. This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

@github-actions github-actions bot added the stale An issue that hasn't been active for a while... label Mar 17, 2024
Copy link
Contributor

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, feel free to create a new issue with up-to-date information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms stale An issue that hasn't been active for a while...
Projects
None yet
Development

No branches or pull requests

1 participant