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

LWS-274: labels for Supersearch #1186

Merged
merged 23 commits into from
Dec 18, 2024
Merged

Conversation

jesperengstrom
Copy link
Contributor

@jesperengstrom jesperengstrom commented Dec 9, 2024

Description

Tickets involved

LWS-274

Solves

Add labels to qualifiers by passing in a callback function to the lxlQualifierPlugin. The function is called with any found qualifier key/value pair, and should return their labels and an optional remove ('up') link. If found, a widget decoration (QualifierComponent) will replace them and an atomic range will be created.

In the lxlweb implementation (no implementation for the Supersearch demo route so far), the callback function iterates search.mappings of $page.data and autosuggest fetches, suggest labels taking precedence. Page data is needed to have labels ready when page loads, suggest mappings are needed to get labels instantly when typing.

As you'll notice, at this point this has quite a few limitations and bugs left to fix. I think cooperation and iteration will be the best way to proceed from here.

Summary of changes

  • Svelte 5-migration of lxlweb/Search component
  • Update codemirror state on lxlweb navigation, needed for facet filters to work
  • Adjust response from server.ts in /find and /api/lang/supersearch (needs improvement)
  • Add getLabelsFromMapping util function
  • Refactor lxlQualifier plugin, now using one single QualifierComponent widget + additional mark decorations.

Notes on the above

  • Syncing Codemirror with url searchParams is now done using afterNavigate which is a svelte/kit import and should be done differently. Suggestions are welcome. Dunno if it's even part of this PR, should be added soon anyhow...
  • The fact that we are not looping out the contents of search.mapping but rather matching a piece of the _q string with a single DisplayMapping object may require further additions to the search api. I've not come up with a way to decisively match the two using today's response. Current implementation is hacky and resulting in rdf:type not getting labels, for example.
  • In the case of a valid key together with an 'unlinked' value (publicationYear:2001, contributor:"Astrid"), the value gets a special styling with no remove link and continues to be editable. One of many things we'll have to try out and discuss.

bugs/limitations

  • Because of the matching problem described above, two qualifiers/filters of the same kind now gets the same label. Maybe it can be completely solved in the frontend, maybe not. Will look into it some more.
  • Because of the nature of the current implementation + suggestion debounce, labels from suggest mapping will not magically appear when resolved, but rather when the function is called the next time. This usually means the labels appear only after typing the first letter of the value, which is not ideal.
  • Labels can also disappear after appearing, usually because suggestion gets an empty response. Maybe we'll have to save the last successful response to avoid this.
  • Qualifiers following an invalid qualifier will never get their label because of the error response caused by the invalid property. Unless we choose a completely different approach, this will only be fixed by new error handling done in the search API.

Todos

  • Write tests
  • Document Supersearch api/prop changes

@jesperengstrom jesperengstrom marked this pull request as ready for review December 10, 2024 09:16
@johanbissemattsson
Copy link
Contributor

johanbissemattsson commented Dec 10, 2024

I think this looks very promising! I will do a more formal review but first a quick reply regarding:

Syncing Codemirror with url searchParams is now done using afterNavigate which is a svelte/kit import and should be done differently. Suggestions are welcome. Dunno if it's even part of this PR, should be added soon anyhow...

One way of achieving this is can be by modifying the $effect function which handles value changes inside SuperSearch.svelte.

If we dispatch changes when the value differs from the current document state in the editor views, then changes to to URL should also be reflected in the editors.

	$effect(() => {
		if (value) {
			search.debouncedFetchData(value);
		}

		if (value !== collapsedEditorView?.state.doc.toString()) {
			collapsedEditorView?.dispatch({
				changes: {
					from: 0,
					to: collapsedEditorView.state.doc.length,
					insert: value
				}
			});
			expandedEditorView?.dispatch({
				changes: {
					from: 0,
					to: expandedEditorView.state.doc.length,
					insert: value
				}
			});
		}
	});

It's a little bit unfortunate that we need to dispatch the changes to both the expanded and collapsed editor in the example above, and it would also probably be preferable to use the reset function to ensure the editor state is resetted between navigations. We probably need to do some minor rewiring as collapsedEditorView and expandedEditorView is only bound to the editor views, not the component itself (which has the exported reset function).

@johanbissemattsson
Copy link
Contributor

johanbissemattsson commented Dec 10, 2024

I noticed now that the effect action in the example above ☝️ is triggered twice as the inequality check causes the effect to run again, it's therefore better to separate them by adding an additonal effect action:

	$effect(() => {
		if (value) {
			search.debouncedFetchData(value);
		}
	});

	$effect(() => {
		if (value !== collapsedEditorView?.state.doc.toString()) {
			collapsedCodeMirror?.reset({
				doc: value
			});
			expandedCodeMirror?.reset({
				doc: value
			});
		}
	});

EDIT:

Another maybe more elegant solution would be just looking for changes in the URL (either using a URL prop or an effect action triggered by window.location.href inside CodeMirror.svelte)

@jesperengstrom
Copy link
Contributor Author

Searchparam sync without afterNavigate should in be fixed in 1276696

@jesperengstrom
Copy link
Contributor Author

jesperengstrom commented Dec 12, 2024

Because of the nature of the current implementation + suggestion debounce, labels from suggest mapping will not magically appear when resolved, but rather when the function is called the next time. This usually means the labels appear only after typing the first letter of the value, which is not ideal.

I've (hopfully) fixed this in e840722

By using an $effect in Supersearch.svelte, we can monitor when search data comes back, passing this notification on to the plugin using StateEffect and trigger a re-rendering of decorations. The same approach could be used to pass on a loading state etc, to the editors.

I've experimented with commenting out this line, i.e. only adding decorations (looking for labels etc) when new data is available, not on every keystroke. It should be great for performance. But to look good, instant styling of qualifiers must come from the Tag highlighter extension included in the language instead. Will work on it some more...

Edit: yet another commit where I've implemented this 👆 . Let's see how it works out...

Copy link
Contributor

@johanbissemattsson johanbissemattsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! 🙌

I think it looks really good (especially the changes where getQualifiers is now only triggered on new data)! It should help the performance quite alot 🌟

lxl-web/src/lib/components/Search.svelte Outdated Show resolved Hide resolved
lxl-web/src/lib/components/Search.svelte Outdated Show resolved Hide resolved
packages/supersearch/src/lib/components/SuperSearch.svelte Outdated Show resolved Hide resolved
johanbissemattsson added a commit that referenced this pull request Dec 17, 2024
This allows the labels in #1186 to work as inteded. We need to sync what the Libris XL API needs first before we do something more.
@jesperengstrom
Copy link
Contributor Author

I think it looks really good (especially the changes where getQualifiers is now only triggered on new data)! It should help the performance quite alot 🌟

Nice, however I'm beginning to think this does not cut it. There is a noticeable lag from the widgets when you type now. Removing a widget by backspace feels kind of buggy because of this. Maybe there's some middle ground here, updates should maybe be triggered by some input after all. Maybe this does not have to be 100% resolved at this point...

@jesperengstrom jesperengstrom force-pushed the LWS-274-supersearch-labels branch from 8f8cb5a to c4eef3b Compare December 18, 2024 10:21
Copy link
Contributor

@johanbissemattsson johanbissemattsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚢

@jesperengstrom jesperengstrom merged commit acb4405 into develop Dec 18, 2024
4 checks passed
@jesperengstrom jesperengstrom deleted the LWS-274-supersearch-labels branch December 18, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants