feat(frontend): sync search in comparison view #661

GorvGoyl · 2021-12-26T19:17:55Z

No description provided.

CLAassistant · 2021-12-26T19:18:01Z

All committers have signed the CLA.

GorvGoyl · 2021-12-26T19:23:17Z

reference #286

codecov · 2021-12-27T05:34:07Z

Codecov Report

Merging #661 (0a8e8f3) into main (556a4c6) will decrease coverage by 0.74%.
The diff coverage is 68.36%.

❗ Current head 0a8e8f3 differs from pull request most recent head 96ee4ff. Consider uploading reports for the commit 96ee4ff to get more accurate results

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
- Coverage   77.08%   76.34%   -0.73%     
==========================================
  Files          59       60       +1     
  Lines        2037     2109      +72     
  Branches      366      391      +25     
==========================================
+ Hits         1570     1610      +40     
- Misses        439      471      +32     
  Partials       28       28

Impacted Files	Coverage Δ
...pp/javascript/components/ProfilerHeader.module.css	`61.54% <ø> (ø)`
webapp/javascript/redux/reducers/search.ts	`38.24% <38.24%> (ø)`
webapp/javascript/components/Toolbar.tsx	`86.17% <90.70%> (-4.74%)`	⬇️
webapp/javascript/ui/Button.tsx	`80.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 556a4c6...96ee4ff. Read the comment docs.

eh-am · 2022-01-03T13:49:52Z

/create-server

Rperry2174 · 2022-01-04T21:39:50Z

/create-server

Rperry2174 · 2022-01-04T22:05:28Z

Reposting thoughts regarding tests to get input from others:

Here there are two issues I think:

      <DebounceInput
        data-testid="flamegraph-search"
        data-testname={`flamegraph-search-${viewSide}`}
        className={`${styles.search} ${
          showMode === 'small' ? styles['search-small'] : ''
        } ${
          searchInputType === 'linked' ? styles['linked-search-input'] : ''
        } ${isSearchLinked === true ? styles.active : ''}`}

So here there is a space between each ternary. so even when the ternaries return '' they really return [space]'' for each ternary. Might be a cleaner way to add these additional classes in this case.

For the other part data-testname="flamegraph-search-undefined" did you change anything that would cause for viewSide to go from being left or right to being undefined?

It looks from this PR like you added data-testname not sure if we need it (?)

cc/ @eh-am

GorvGoyl · 2022-01-05T14:10:58Z

So here there is a space between each ternary. so even when the ternaries return '' they really return [space]'' for each ternary. Might be a cleaner way to add these additional classes in this case.

extra space at the end in class shouldn't cause any issues in UI. Similar scenario for Single View page when the search bar is not small.

className={`${styles.search} ${
          showMode === 'small' ? styles['search-small'] : ''
        }

I did some refactoring but extra space is still there unless I do some more string manipulation. Instead, is it possible to update the snapshot so it matches the latest test result?

It looks from this PR like you added data-testname not sure if we need it

correct, I refactored the code to use data-testid for linked search test case

eh-am

Hey, that's pretty cool! I have a few comments:

Maybe it's just me, but I found a bit weird how removing the link also deletes the search. For comparison, in grafana, when you unsync 2 time ranges, they continue the same, just unlinked.

Functionality is missing on the Adhoc Comparison page.

How is the button going to work when we add a button to clear the search, like ?

I would love to see some unit tests here!

The button is missing a hover cursor.

eh-am · 2022-01-05T14:24:23Z

webapp/javascript/components/ComparisonApp.jsx

+  };
+  const toggleLinkedSearch = (side) => {
+    const { isSearchLinked } = linkedSearch;
+    if (isSearchLinked) {


Can we return early here if isSearchLinked is false? The nesting here doesn't add much value IMO.

eh-am · 2022-01-05T14:26:22Z

webapp/javascript/components/ComparisonApp.jsx

+  const toggleLinkedSearch = (side) => {
+    const { isSearchLinked } = linkedSearch;
+    if (isSearchLinked) {
+      if (side === 'left') {


Can we use switch/case here? This file isn't using typescript yet, but in the future we can update it to use exhaustive case checks.

Plus these cases are pretty similar, so we can just build an args parameter and then declaratively use it?

Eg

let args; if (side === 'right') { args = { resetLinkedSearchSide: 'left' } } if (side === 'both') { args = { sSearchLinked: false, resetLinkedSearchSide: '' } } (...) setLinkedSearch((x) => { return { ...x, ...args }; });

Also I think having all this logic here bloats this file. These files (Coparison, SingleView, ComparisonDiff) are the entrypoints for our pages, ie the first or second thing people should read to understand what the page is about. Having this logic makes it harder for people to understand it.

@eh-am where would you put this instead?

Can we use switch/case here
declarative way

done.

eh-am · 2022-01-05T14:38:27Z

webapp/javascript/components/ComparisonApp.jsx

+              isSearchLinked={linkedSearch.isSearchLinked}
+              setSearchQuery={setSearchQuery}
+              linkedSearchQuery={linkedSearch.linkedSearchQuery}
+              toggleLinkedSearch={toggleLinkedSearch}
+              resetLinkedSearchSide={linkedSearch.resetLinkedSearchSide}


This is case where lifting state up makes the parent component too bloated. I think you can infer based on the toolbar viewType whether to show the link, and then dispatch a redux action to enable/disable the link functionality.

webapp/javascript/components/FlameGraph/FlameGraphRenderer.jsx

eh-am · 2022-01-05T14:48:35Z

webapp/javascript/components/Toolbar.tsx

+            isSearchLinked === true ? styles.active : ''
+          }`}
+        >
+          <svg


Isn' there a fontAwesome version of this? Example https://github.com/pyroscope-io/pyroscope/blob/main/stories/Button.stories.tsx#L26

done. Found one matching link icon (solid though) https://fontawesome.com/v5.15/icons?d=gallery&p=2&q=link

eh-am · 2022-01-05T14:51:30Z

webapp/javascript/components/Toolbar.tsx

+          title={`${
+            isSearchLinked === true ? 'Unsync search term' : 'Sync search term'
+          }`}


Not a big fan of using titles for this cases, but I guess until we have proper tooltip support this is fine.

agree. we can replace title once we have a generic tooltip.

GorvGoyl · 2022-01-05T16:14:13Z

Functionality is missing on the Adhoc Comparison page.

Is this different from /comparison page? what's the URL here?

GorvGoyl · 2022-01-05T16:16:24Z

add hover cursor to button.

GorvGoyl · 2022-01-05T16:18:53Z

How is the button going to work when we add a button to clear the search

clear input text button is already there on hover.

would love to see some unit tests here!

I added some tests in basic.ts file. Where do I add more tests?

eh-am · 2022-01-05T17:14:44Z

Functionality is missing on the Adhoc Comparison page.

Is this different from /comparison page? what's the URL here?

You have to run the server with --enable-experimental-adhoc-ui enabled, or via env var PYROSCOPE_ENABLE_EXPERIMENTAL_ADHOC_UI. We will release after @abeaumont finishes the implementation, which is almost done here #652

Then it will be enabled in the sidebar ->

How is the button going to work when we add a button to clear the search

clear input text button is already there on hover.

Sorry, I don't see it, am I missing something?

But anyway, that answers my question, it will be inside the input, so it will still look good!

would love to see some unit tests here!

I added some tests in basic.ts file. Where do I add more tests?

That's a e2e/integration test. For that I would simply 1 - Type the search 2 - Click the link button 3 - Check if the query was copied to the other field.
There's no need to test the highlight again IMO.

Now for adding the unit tests, it depends how the code is organized. You can try putting 2 toolbars side by side and seeing if the sync works. Or if that's too difficult, to put 2 flamegraphs side by side. The advantage of unit test here is that it forces you to check the API. If it's too difficult to test, then it's too difficult to (re)use the component.

GorvGoyl · 2022-01-06T16:14:20Z

You have to run the server with --enable-experimental-adhoc-ui enabled, or via env var PYROSCOPE_ENABLE_EXPERIMENTAL_ADHOC_UI. We will release after @abeaumont finishes the implementation, which is almost done here #652

So, I was trying it and it looks like the profiler data is not able to load for the other view.

GorvGoyl · 2022-01-06T16:16:12Z

How is the button going to work when we add a button to clear the search. Sorry, I don't see it, am I missing something?

I'm using chromium browser (edge) and it's visible when I type something into the search box.

GorvGoyl · 2022-01-08T13:42:40Z

@eh-am I've moved the logic to redux as discussed as per my understanding. Toolbar component is used to access redux store but it's now causing issues with toolbar unit tests:

ProfileHeader › ViewSection › small mode › changes to Both view

    could not find react-redux context value; please ensure the component is wrapped in a <Provider>

      204 | }) {
      205 |   const [text, setText] = useState('');
    > 206 |   const isSearchLinkedState = useSelector(isSearchLinked);

Should we also include the provider for unit tests or is there an alternative way? I've limited experience with redux so not sure about the right approach.

Also, let me know if there are any other changes needed.

GorvGoyl · 2022-01-10T16:43:24Z

@eh-am I think I've addressed the mentioned changes. Please review.

cc: @Rperry2174

* fix-single-binary-deploy * Make dashboards work for single-binary phlare

feat(frontend): sync search in comparison view

4fb490b

github-actions bot requested review from petethepig and Rperry2174 December 26, 2021 19:18

GorvGoyl added 2 commits January 5, 2022 00:50

merged main branch

23bbcee

refactor(frontned): unqiue snapshot name for tests

2cdfc0f

GorvGoyl added 2 commits January 5, 2022 19:18

refactor: input testid

201f3f6

refactor: use variables for input classes

089d4ed

eh-am reviewed Jan 5, 2022

View reviewed changes

refactor: return early if isSearchLinked is false

efb097f

GorvGoyl added 5 commits January 6, 2022 15:46

refactor: use switch case

d3cb033

refactor: use declarative way to update state

dc39ae2

refactor: clarification for eslint rule

7dc1df7

refactor: use button component with fa icon

c81a5ba

Merge branch 'main' of https://github.com/pyroscope-io/pyroscope

a8649e6

GorvGoyl added 3 commits January 7, 2022 15:46

refactor: improve integration test

2053f57

added unit test for linked search component

76d093e

refactor: use redux

3effcdd

refactor: remove unused import

a7f96b4

petethepig force-pushed the main branch from 5ac4a22 to 65b7372 Compare January 9, 2022 22:14

GorvGoyl and others added 3 commits January 10, 2022 21:55

refactor: fix unit tests

fdbf58c

Merge branch 'main' into main

0a8e8f3

docs: updates the list of contributors in README

96ee4ff

github-actions bot added the has conflicts label Jan 11, 2022

petethepig closed this Mar 1, 2022

korniltsev pushed a commit that referenced this pull request Jul 18, 2023

Fix kind local cluster single binary deploy (#661)

7c56ec7

* fix-single-binary-deploy * Make dashboards work for single-binary phlare

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): sync search in comparison view #661

feat(frontend): sync search in comparison view #661

GorvGoyl commented Dec 26, 2021

CLAassistant commented Dec 26, 2021 •

edited

Loading

GorvGoyl commented Dec 26, 2021

codecov bot commented Dec 27, 2021 •

edited

Loading

eh-am commented Jan 3, 2022

Rperry2174 commented Jan 4, 2022

Rperry2174 commented Jan 4, 2022

GorvGoyl commented Jan 5, 2022 •

edited

Loading

eh-am left a comment

eh-am Jan 5, 2022

GorvGoyl Jan 5, 2022

eh-am Jan 5, 2022

eh-am Jan 5, 2022

eh-am Jan 5, 2022

Rperry2174 Jan 5, 2022

GorvGoyl Jan 6, 2022

eh-am Jan 5, 2022

eh-am Jan 5, 2022

GorvGoyl Jan 6, 2022

eh-am Jan 5, 2022

GorvGoyl Jan 6, 2022

GorvGoyl commented Jan 5, 2022

GorvGoyl commented Jan 5, 2022 •

edited

Loading

GorvGoyl commented Jan 5, 2022

eh-am commented Jan 5, 2022

GorvGoyl commented Jan 6, 2022

GorvGoyl commented Jan 6, 2022

GorvGoyl commented Jan 8, 2022 •

edited

Loading

GorvGoyl commented Jan 10, 2022

feat(frontend): sync search in comparison view #661

feat(frontend): sync search in comparison view #661

Conversation

GorvGoyl commented Dec 26, 2021

CLAassistant commented Dec 26, 2021 • edited Loading

GorvGoyl commented Dec 26, 2021

codecov bot commented Dec 27, 2021 • edited Loading

Codecov Report

eh-am commented Jan 3, 2022

Rperry2174 commented Jan 4, 2022

Rperry2174 commented Jan 4, 2022

GorvGoyl commented Jan 5, 2022 • edited Loading

eh-am left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GorvGoyl commented Jan 5, 2022

GorvGoyl commented Jan 5, 2022 • edited Loading

GorvGoyl commented Jan 5, 2022

eh-am commented Jan 5, 2022

GorvGoyl commented Jan 6, 2022

GorvGoyl commented Jan 6, 2022

GorvGoyl commented Jan 8, 2022 • edited Loading

GorvGoyl commented Jan 10, 2022

CLAassistant commented Dec 26, 2021 •

edited

Loading

codecov bot commented Dec 27, 2021 •

edited

Loading

GorvGoyl commented Jan 5, 2022 •

edited

Loading

GorvGoyl commented Jan 5, 2022 •

edited

Loading

GorvGoyl commented Jan 8, 2022 •

edited

Loading