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

Enhance search by filtering by URL, hostname and dns resolve server #5438

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Ionys320
Copy link
Contributor

@Ionys320 Ionys320 commented Dec 14, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #5422 . This PR is pretty simple (just three lines enhancing existing filters), but makes so much sens to me..

Note: Even if this is not supposed to occurs, the code handle the case url would be null/undefined.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots

image

@Ionys320 Ionys320 changed the title feat(search): Enhance search by filtering by URLs Enhance search by filtering by URL, hostname and dns resolve server Dec 14, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

At its core this is reasonable change.

I am a bit sussed out by this code in terms of performance as I implemented something similar in another project and the performance was pretty bad.
I think we should switch to Regex comparisons instead as they are faster than the call to .toLowerCase() as far as I remember and can handle caseness.
What do you think?

Except with less than 20 monitors, regex seems faster.
@Ionys320
Copy link
Contributor Author

Ionys320 commented Dec 22, 2024

Hi @CommanderStorm! After some tests, it appears regex is more efficient when you have ~100monitors. Before, lower-case can be better. But because UK is designed to handle a lot of monitors, regex has been used instead. Thanks for the suggestion!!

@homelab-alpha
Copy link
Contributor

I was thinking more along the lines of something like this:

// filter by search text using regex
// matches monitor name, tag name, or tag value
let searchTextMatch = true;
if (this.searchText !== "") {
    try {
        const regex = new RegExp(this.searchText, "i"); // "i" for case-insensitive matching
        searchTextMatch =
            regex.test(monitor.name) ||
            (monitor.url && regex.test(monitor.url)) ||
            (monitor.hostname && regex.test(monitor.hostname)) ||
            (monitor.dns_resolve_server && regex.test(monitor.dns_resolve_server)) ||
            monitor.tags.some(tag => regex.test(tag.name) || (tag.value && regex.test(tag.value)));
    } catch (e) {
        console.error("Invalid regex pattern:", e);
        searchTextMatch = false;
    }
}

@Ionys320
Copy link
Contributor Author

Relevant indeed, I forgot to take into account the null/undefined values. Will do the changes, thanks @homelab-alpha

@Ionys320
Copy link
Contributor Author

Or better: suggest the change via the review feature so you can appear in the contributors.

@homelab-alpha
Copy link
Contributor

@Ionys320 , You came up with the fix, we can share the credit. 😊

…lso optimize a bit search tags by replacing `.find` with `.some`.

Thanks @homelab-alpha
@Ionys320
Copy link
Contributor Author

So, the input is now regex friendly, that's fun. I wonder if it can broke something, I tried do search a fiew fields and it went fine.
The only issue I could fine is about . : when you search for goo.gle, it will find google.fr, even it it's not the request (logic because . is considered as any character). I wonder if it's an issue, because if so, we'll need to rollback to the lower-case method. @CommanderStorm?

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Dec 22, 2024

I'd consider regexes working somewhat like a feature.
Alternatively, we could just replace special characters away by escaping them.

I am fine with both

@Ionys320
Copy link
Contributor Author

Approved then!

This can be reverted if the search should be regex friendly, but for the moment, it's not the case.
Thanks @homelab-alpha!
@Ionys320
Copy link
Contributor Author

Okay so: the searchbar is now escaping regex expression (#5466 (comment)).

This PR creates the issue #5466. This issue occures because the PR extends the used fiels, but the PR is not creating the issue neither. But maybe fix #5466 before merging the PR would be a good move.

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.

Ability Seach Monotor List via Hostnames or URL
3 participants