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

Feature: add commit filtering to the profiler #474

Merged
merged 2 commits into from
Apr 1, 2023

Conversation

jpnelson
Copy link
Contributor

@jpnelson jpnelson commented Apr 1, 2023

This surely needs some cleanup, but I wanted to put this out there as a feature suggestion! For our use cases, the profiler is very helpful, but is pretty hard to use when we have multiple hundreds of commits. This adds a filter to it, so you can specify which durations you're interested in looking at:

Screenshot 2023-03-31 at 8 29 21 PM

There's an empty state when you hide all of the commits, so it's clear what's going on:

Screenshot 2023-03-31 at 8 29 39 PM

Some edge cases handled:

  • Navigating the commits via the "rendered at" panel should remove the filter. I think this is the simplest way, otherwise we'd need to filter all commits there too, and it gets a bit messy
  • Changing the filter sets the first commit in the new, filtered list to the active one

Implementation details

  • I used a computed signal for filtered commits, which lets us share the logic for filtering easily
  • I broke the FilterPopup into its own component now – I tried to keep it lightweight, but we could abstract the logic into it a bit more if we liked.

Future work

It would be great to be able to filter additional things in or out of the commit timeline in the future so commits can be further searched. Maybe by the count of nodes in the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff here is a little weird, but the main change to this file is extracting FilterPopup from it

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Love this! This is such a great idea!

@marvinhagemeister marvinhagemeister merged commit a6e27f7 into preactjs:main Apr 1, 2023
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.

None yet

2 participants