-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add highlighting of matches to Quick Bar items #22019
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThe changes encompass modifications to documentation, enhancements to the fuzzy matching functionality, the introduction of a new web component for displaying match segments, and improvements to testing coverage. Key updates include a refined Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/dialogs/quick-bar/ha-quick-bar.ts (1)
378-416
: LGTM! Consider adding unit tests.The new
_applyCommandItemMatchingInfo
and_applyEntityItemMatchingInfo
functions encapsulate the logic for generating match tokens nicely. This promotes code reusability and maintainability.The functions also handle cases where
matchInfo
may be undefined, preventing potential runtime errors.Consider adding unit tests for these functions to ensure they handle various scenarios correctly, such as:
- When
matchInfo
is undefined- When
matchInfo
has differentindex
values- When
matchInfo
has no match segments
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/common/string/filter/filter.ts (1 hunks)
- src/common/string/filter/sequence-matching.ts (3 hunks)
- src/components/ha-match-segment.ts (1 hunks)
- src/dialogs/quick-bar/ha-quick-bar.ts (6 hunks)
- test/common/string/sequence_matching.test.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- src/common/string/filter/filter.ts
Additional comments not posted (16)
src/components/ha-match-segment.ts (5)
1-3
: LGTM!The imports are relevant and required for the component implementation. The code segment looks good.
5-7
: LGTM!The custom element definition and property declaration follow the LitElement conventions correctly. The code segment looks good.
9-19
: LGTM!The rendering logic in the
render
method is implemented correctly. It handles the case whensegments
is empty or undefined, and it conditionally renders each segment with highlighting for matched segments. The code segment looks good.
21-29
: LGTM!The static
styles
method correctly defines the CSS styles for the component using thecss
tagged template literal. The "highlight" class styles are appropriate for visually distinguishing matched segments. The code segment looks good.
31-35
: LGTM!The global interface extension for
HTMLElementTagNameMap
is correctly declared, associating the custom element nameha-match-segment
with theHaMatchSegment
class. This ensures that the custom element can be recognized as a valid HTML tag in the DOM. The code segment looks good.test/common/string/sequence_matching.test.ts (4)
66-67
: LGTM!The additional assertion to check if the result is defined before accessing its
score
property improves the test's robustness and prevents potential false positives.
112-154
: LGTM!The updated test case enhances the coverage by validating the
score
andmatchInfo
properties of the filtered and sorted items. This ensures that thefuzzyFilterSort
function calculates the scores and match information accurately in addition to filtering and sorting the items correctly.
166-246
: LGTM!The test case for
tokenizeMatchInfo
provides comprehensive coverage by validating the function's behavior for various input patterns, including fully matched, partially matched, and unmatched segments. It ensures that the function correctly tokenizes the item string based on the match information.The additional test case for undefined segments is a nice touch, verifying that the function handles gracefully when no match information is provided.
Overall, the test case is well-structured and helps catch any potential issues or regressions in the tokenization logic.
248-324
: LGTM!The test case for
tokenizeConcatenatedMatchInfo
provides comprehensive coverage by validating the function's behavior for various input patterns, including fully matched, partially matched, and unmatched segments for both the type and target. It ensures that the function correctly tokenizes the concatenated string based on the match information.The additional test case for undefined segments is a nice touch, verifying that the function handles gracefully when no match information is provided.
Overall, the test case is well-structured and helps catch any potential issues or regressions in the tokenization logic for concatenated strings.
src/common/string/filter/sequence-matching.ts (6)
63-123
: Approve the addition of_getContinuousRuns
function.The
_getContinuousRuns
function is a helpful addition that analyzes and returns continuous segments of matches and gaps. The implementation looks correct and well-documented.
125-153
: Approve the addition oftokenizeMatchInfo
function.The
tokenizeMatchInfo
function is a useful addition that tokenizes a matched string against match segments. It provides a way to generate match tokens indicating which parts of the string matched the original query. The implementation looks good.
155-197
: Approve the addition oftokenizeConcatenatedMatchInfo
function.The
tokenizeConcatenatedMatchInfo
function is a valuable addition that tokenizes the segments of a concatenated string against match segments. It provides a way to handle cases where distinct string parts have been concatenated for a fuzzy sequential search, but need to be tokenized separately. The implementation looks correct and well-documented.
Line range hint
335-374
: Approve the changes tofuzzyFilterSort
function.The updates to the
fuzzyFilterSort
function ensure that thescore
andmatchInfo
properties of each item are properly set based on the best match found usingfuzzySequentialMatch
. The changes look good and the updated JSDoc comments provide clearer documentation on the structure of the returned items.
321-321
: Approve the changes toScorableTextItem
interface.The addition of the
matchInfo
property to theScorableTextItem
interface, along with the clear JSDoc comment, enhances the interface by allowing detailed match information to be stored alongside the score. This enables more advanced processing and display of matched results. The changes look good.
Line range hint
6-61
: Approve the changes tofuzzySequentialMatch
function.The updates to the
fuzzySequentialMatch
function enhance its functionality by returning more detailed match information and handling multiple strings initem.strings
. The changes look good.Verify that the code consuming the
fuzzySequentialMatch
function has been updated to handle the new returned object structure containing the score and match information.Run the following script to find usages of
fuzzySequentialMatch
:Verification successful
Approve the changes to
fuzzySequentialMatch
function. Usage has been properly updated.The modifications to the
fuzzySequentialMatch
function have been implemented correctly, and its usage throughout the codebase has been updated to handle the new returned object structure. Key observations:
- In test files (e.g., 'test/common/string/sequence_matching.test.ts'), both the
score
andmatchInfo
properties are accessed from the returned object.- In the implementation file ('src/common/string/filter/sequence-matching.ts'), the
fuzzyFilterSort
function correctly destructures the returned object to getscore
andmatchInfo
.These findings confirm that the changes have been consistently applied and the function is being used correctly with its new return type.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of `fuzzySequentialMatch` function. # Test: Search for the function usage. Expect: Occurrences of the function call. rg --type typescript -A 5 $'fuzzySequentialMatch'Length of output: 83
Script:
#!/bin/bash # Description: Find usages of `fuzzySequentialMatch` function in TypeScript files. # Search for the function usage in .ts files echo "Searching in .ts files:" rg --type-add 'ts:*.ts' --type ts -A 5 'fuzzySequentialMatch' # Search for the function usage in .tsx files (in case of React with TypeScript) echo "Searching in .tsx files:" rg --type-add 'tsx:*.tsx' --type tsx -A 5 'fuzzySequentialMatch'Length of output: 7260
src/dialogs/quick-bar/ha-quick-bar.ts (1)
133-150
: Verify the function signature change in the codebase.The updated logic to reset
matchInfo
when no filter is applied looks good. However, ensure that all calls to_getItems
have been updated to match the new signature.Run the following script to verify the function usage:
Verification successful
Verification successful:
_getItems
function usage is correctThe updated
_getItems
function insrc/dialogs/quick-bar/ha-quick-bar.ts
is being called with the correct new signature (4 parameters). Other occurrences of_getItems
in the codebase are separate implementations within their respective components and are not affected by this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `_getItems` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'_getItems\\('Length of output: 76
Script:
#!/bin/bash # Description: Verify all calls to `_getItems` match the new signature. # Find TypeScript files and search for _getItems function calls fd -e ts -e tsx | xargs rg -n -C 5 '_getItems\('Length of output: 3870
Proposed change
This MR adds highlighting of (partial) matches of the filter/search text to the items display in HA's Quick Bar:
I think the same mechanism can be applied to other places with find-as-you-type-queries based on the fuzzy sequence matching as well (area picker for instance); that'd be part of later MRs, though. If this one is accepted, that is.
A couple notes:
There seems to be a bug in Chromium based browsers (screencast above is done in Firefox) where they erroneously apply word capitalization on boundaries of highlight segments in a very inconsistent and apparently random way. I don't know if there are CSS or DOM hacks that could be applied, perhaps anyone here as an idea? See screenshots of that super inconsistent behavior (Edge 128.0.2739.79, however a freshly installed Chrome shows the same behavior):
Search for "n":
Add an "a":
Remove the "a" again:
Supposedly the HTML is the same in step 1 and 3, but Chromium renders it differently. Also, no crazy capitalization after "Na" in step 2. 🤷
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
HaMatchSegment
for visually displaying matched text segments.Bug Fixes
Documentation
FuzzyScore
to clarify the variable nature of match positions.