Move typeahead per-position suggestion logic from ProseMirror-based TypeaheadPopover to CQL-based Typeahead#99
Open
jonathonherbert wants to merge 16 commits intojsh/correct-mappings-like-really-correctfrom
Conversation
jonathonherbert
changed the title
Contributor
Contributor
jonathonherbert
force-pushed
the
jsh/pos-based-suggestions
branch
from
3dbbcb6 to
7f5af43
Compare
jonathonherbert
changed the base branch from
main
to
jsh/improve-debug-positioning
jonathonherbert
force-pushed
the
jsh/pos-based-suggestions
branch
from
7f5af43 to
184d17b
Compare
jonathonherbert
changed the title
jonathonherbert
force-pushed
the
jsh/improve-debug-positioning
branch
from
f4621d8 to
5e7291f
Compare
jonathonherbert
force-pushed
the
jsh/pos-based-suggestions
branch
from
184d17b to
8b2f271
Compare
jonathonherbert
force-pushed
the
jsh/improve-debug-positioning
branch
2 times, most recently
from
414d013 to
8781a4a
Compare
jonathonherbert
force-pushed
the
jsh/pos-based-suggestions
branch
2 times, most recently
from
c6140ae to
33bc62c
Compare
…gestion for a query from the typeahead helper which breaks the current plugin behaviour, todo
…and adjust consumers
…selection position either side of a character to perform correctly), and adjust debug visualisation
to make them simpler and more comprehensible ... but although the tests pass, they are not reversible, which stretches my mental model a bit ... I need to understand better why this wasn't a problem before
…which makes the behaviour clearer I was testing an inverted mapping, making changes unintuitive D:< Having said that, I'm not quite sure why getFieldValueRanges requires a negative oldSize rather than a positive toSize for its chipValue end, which is something I arrived at via experimentation rather than a complete understanding ... dislike
cleaning up the ranges to remove the odd inversion, and expanding the property-based tests to verify they are correct
jonathonherbert
force-pushed
the
jsh/pos-based-suggestions
branch
from
33bc62c to
1dc8435
Compare
jonathonherbert
force-pushed
the
jsh/improve-debug-positioning
branch
from
8781a4a to
509320c
Compare
jonathonherbert
commented
Nov 16, 2025
|
|
||
| /** | ||
| * These diff functions are vendored to ignore attributes and markup, as we only care about markup. | ||
| * These diff functions are vendored to ignore attributes, as we only care about markup. |
Collaborator
Author
There was a problem hiding this comment.
This comment didn't make sense, tidied up on the fly
jonathonherbert
commented
Nov 16, 2025
| createCqlEditor("example +tag:"); | ||
|
|
||
| await moveCaretToQueryPos(queryStr.length - 1); | ||
| await moveCaretToQueryPos(queryStr.length); |
Collaborator
Author
There was a problem hiding this comment.
Some of these positions were revealed to be slightly inaccurate by the mapping changes, so they've been amended here.
jonathonherbert
commented
Nov 16, 2025
| const newDoc = tokensToDoc(mapResult(parser(queryBeforeParse)).tokens); | ||
| const queryAfterParse = docToCqlStr(newDoc); | ||
| const result = parser(queryAfterParse); | ||
| const { mapping, queryAst, tokens, error } = mapResult(result); |
Collaborator
Author
There was a problem hiding this comment.
Parsing twice here eliminates an issue that came up in the tests where the cql query derived by a document would not be exactly the same as the query that produced it. For example, the following could occur:
k:v -> <chip><chipKey>k</chipKey><chipValue>v</chipValue></chip> -> +k:v
Re-parsing the string derived from the new document, which may differ from the original, ensures that we get the correct tokens and mapping.
jonathonherbert
commented
Nov 16, 2025
| } | ||
|
|
||
| this.currentSuggestion = suggestionThatCoversSelection; | ||
| this.currentSuggestion = typeaheadSuggestion; |
Collaborator
Author
There was a problem hiding this comment.
The logic to find suggestions here has been moved to the Typeahead class
jonathonherbert
changed the base branch from
jsh/improve-debug-positioning
to
jsh/correct-mappings-like-really-correct
…based-suggestions
…based-suggestions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's changed?
Prior to this PR, the typeahead API didn't reason about the document position — it provides all the possible typeahead suggestions for a query, and it was up to the UI to figure out whether a suggestion applies given the state of the UI.
This is less efficient than fetching the given suggestions for a position, but was an easy way to provide reasonable typeahead behaviour when the mapping from query to document, and vice-versa, was less than perfect.
After #103, we can map from document position to query position with confidence, and rely entirely on the
Typeaheadclass to provide the correct suggestions for the given position, which improves separation of concerns (no more reasoning about document positions inTypeaheadPopover.NB: TypeaheadPopover does couple itself to the ProseMirror view, and we'll need to remove that dependency to share it with a hypothetical non-ProseMirror based UI.
How to test
How can we measure success?