Components: Extract the autocomplete matcher into a separate function#76957
Merged
Components: Extract the autocomplete matcher into a separate function#76957
Conversation
562422f to
f7e3182
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @SavvyShah, @gwwar. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -54 B (0%) Total Size: 7.73 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3fbfca7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23867423472
|
mirka
approved these changes
Apr 1, 2026
Comment on lines
+228
to
+232
| const textAfterSelection = textContent | ||
| ? getTextContent( | ||
| slice( record, undefined, getTextContent( record ).length ) | ||
| ) | ||
| : ''; |
Member
There was a problem hiding this comment.
This refactor now computes textAfterSelection before we know whether there is any viable match. On trunk, that slice()/getTextContent() work only happened after the cheaper trigger and mismatch guards had already passed.
Not sure how much of a hotspot this is without perf measurements, but could be potential point to make lazier.
Member
Author
There was a problem hiding this comment.
Changed arg to getTextAfterSelection. Since this is an internal function, I don't think there's a need to support function|string argument time at the moment.
Comment on lines
-278
to
-280
| if ( tooDistantFromTrigger ) { | ||
| return; | ||
| } |
Member
There was a problem hiding this comment.
Small behavior change here: on trunk, the textWithoutTrigger.length > 50 case bails out without calling reset(), while the version in this PR will call the reset(). That seems like a reasonable improvement, but it is a visible change in behavior. It may be worth either noting that in the PR description or adding a test so the intent is documented.
Member
Author
There was a problem hiding this comment.
Right, reset seems a better approach. @ockham also asked a similar question when this condition was introduced - #30649 (comment).
Added note to the PR description.
Member
Author
|
@mirka, let me know if this is good to merge. I have a couple of follow-ups that I would like to land after this refactoring. |
Member
|
Yes, looks good to go 👍 |
ciampo
pushed a commit
that referenced
this pull request
Apr 3, 2026
…#76957) Unlinked contributors: SavvyShah, gwwar. Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: USERSATOSHI <tusharbharti@git.wordpress.org>
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
2 participants
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?
Closes #30969.
Alternative to #71417. Pure matcher function; tests without benchmarking; perf tests already track this. Return types.
PR extracts autocomplete matching logic into a standalone pure function.
Why?
The matching algorithm was buried inside a
useEffect, making it untestable in isolation. Also simplifies by removing dead code (no-op unicode regex, unnecessaryescapeRegExp+ regex construction).This should make it easier to fix and test cases like #61919.
How?
getAutocompleteMatch()takes text content, completers, and a few flags.slice()+removeAccents(). Improves upon Autocomplete: reduce work before finding trigger #48327.Testing Instructions
The
test/e2e/specs/editor/various/autocomplete-and-mentions.spec.jscovers most of the cases.Testing Instructions for Keyboard
Same.
Use of AI Tools
Claude did most of the grunt work. I reviewed and validated results.