Prevent rubber-banding in Telemetry Table filter input #7248

scottbell · 2023-11-20T11:25:34Z

Closes #7239

Describe your changes:

Sets the debounce to affect the filtering of the telemetry table, not the setting of the input value.

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

…e input

deploysentinel · 2023-11-20T11:30:05Z

Current Playwright Test Results Summary

✅ 14 Passing

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 11/22/2023 10:23:12am UTC)

Run Details

Running Workflow e2e-couchdb on Github Actions

Commit: b3fff14

Started: 11/22/2023 10:21:41am UTC

View Detailed Build Results

Current Playwright Test Results Summary

✅ 162 Passing - ⚠️ 3 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 11/22/2023 10:23:12am UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: b3fff14

Started: 11/22/2023 08:25:41am UTC

⚠️ Flakes

📄 functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 2 Flakes

Top 1 Common Error Messages

`null`	2 Test Cases Affected

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Restricted Notebook with a page locked and with an embed @addinit Allows embeds to be deleted if page unlocked @addinit Retry 1 • Initial Attempt	3.51% (2) 2 / 57 runs failed over last 7 days	50.88% (29) 29 / 57 runs flaked over last 7 days
Restricted Notebook with a page locked and with an embed @addinit Disallows embeds to be deleted if page locked @addinit Retry 1 • Initial Attempt	3.57% (2) 2 / 56 runs failed over last 7 days	57.14% (32) 32 / 56 runs flaked over last 7 days

📄 functional/plugins/imagery/exampleImagery.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Example Imagery Object Can use Mouse Wheel to zoom in and out of latest image Retry 1 • Initial Attempt	1.72% (1) 1 / 58 run failed over last 7 days	32.76% (19) 19 / 58 runs flaked over last 7 days

View Detailed Build Results

codecov · 2023-11-20T11:36:35Z

Codecov Report

Merging #7248 (b3fff14) into master (28bfc90) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7248      +/-   ##
==========================================
- Coverage   56.05%   56.01%   -0.05%     
==========================================
  Files         654      654              
  Lines       26228    26229       +1     
  Branches     2528     2528              
==========================================
- Hits        14702    14691      -11     
- Misses      10827    10836       +9     
- Partials      699      702       +3

Flag	Coverage Δ		*Carryforward flag
e2e-full	`41.83% <ø> (-0.06%)`	⬇️	Carriedforward from 089557f
e2e-stable	`58.35% <ø> (+<0.01%)`	⬆️
unit	`49.29% <0.00%> (-0.07%)`	⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
...ugins/telemetryTable/components/TableComponent.vue	`41.96% <0.00%> (-0.12%)`	⬇️

... and 10 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

unlikelyzero · 2023-11-20T18:54:24Z

e2e/tests/functional/plugins/telemetryTable/telemetryTable.e2e.spec.js

@@ -92,7 +92,7 @@ test.describe('Telemetry Table', () => {
    await page.goto(table.url);

    await page.getByRole('searchbox', { name: 'message filter input' }).click();
-    await page.getByRole('searchbox', { name: 'message filter input' }).fill('Roger');
+    await page.keyboard.type('Roger', { delay: 150 });


I think you could add coverage by using the previous logic and instantly changing the text. As long as the actions are quick enough, you should be able to cover the filterChanged codepath, no?

await page.getByRole('searchbox', { name: 'message filter input' }).fill('Roger2'); await page.getByRole('searchbox', { name: 'message filter input' }).fill('Roger');

I guess the question to answer would be, does this test fail if we revert the functional changes? Otherwise we should just keep the fill() since it won't matter and we'll save a sliver of runtime in the test.

Yeah, it only happens sporadically, so I think you're right that the test change isn't worth the 150ms.

@scottbell did you catch my meaning about the filterChanged codepath

@unlikelyzero I tested using both ways, and both are firing filterChanged FWIW, as the table is filtering values. I'm not sure why our code coverage tool isn't reporting it!

ozyx

Nice catch! No blocking changes requested, just a suggestion or two.

ozyx · 2023-11-20T21:50:47Z

src/plugins/telemetryTable/components/TableComponent.vue

@@ -478,7 +478,7 @@ export default {
    }
  },
  created() {
-    this.filterChanged = _.debounce(this.filterChanged, 500);
+    this.filterTelemetry = _.debounce(this.filterTelemetry, 500);


This would make a pretty neat composable if you're looking to get your hands dirty. Something like useDebounceFn maybe

ozyx · 2023-11-20T21:53:57Z

e2e/tests/functional/plugins/telemetryTable/telemetryTable.e2e.spec.js

@@ -92,7 +92,7 @@ test.describe('Telemetry Table', () => {
    await page.goto(table.url);

    await page.getByRole('searchbox', { name: 'message filter input' }).click();
-    await page.getByRole('searchbox', { name: 'message filter input' }).fill('Roger');
+    await page.keyboard.type('Roger', { delay: 150 });


I guess the question to answer would be, does this test fail if we revert the functional changes? Otherwise we should just keep the fill() since it won't matter and we'll save a sliver of runtime in the test.

…emetry-table-filter-inputs

ozyx

LGTM

should debounce the filtering of the telemetry, not the setting of th…

35df914

…e input

scottbell linked an issue Nov 20, 2023 that may be closed by this pull request

Laggy input and rubber-banding in Telemetry Table filter inputs #7239

Closed

7 tasks

add some laggy typing to check for debouncing issues

8135767

scottbell marked this pull request as ready for review November 20, 2023 14:30

scottbell requested review from ozyx and michaelrogers November 20, 2023 14:30

scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 20, 2023

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 20, 2023

unlikelyzero reviewed Nov 20, 2023

View reviewed changes

ozyx reviewed Nov 20, 2023

View reviewed changes

ozyx added this to the Target:3.3.0 milestone Nov 20, 2023

revert test

666f1a5

scottbell requested review from unlikelyzero and ozyx November 21, 2023 12:47

Merge branch 'master' into 7239-laggy-input-and-rubber-banding-in-tel…

089557f

…emetry-table-filter-inputs

scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 21, 2023

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 21, 2023

unlikelyzero approved these changes Nov 21, 2023

View reviewed changes

Merge branch 'master' into 7239-laggy-input-and-rubber-banding-in-tel…

b3fff14

…emetry-table-filter-inputs

scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 22, 2023

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 22, 2023

ozyx approved these changes Nov 28, 2023

View reviewed changes

scottbell merged commit 415b652 into master Nov 28, 2023
15 checks passed

scottbell deleted the 7239-laggy-input-and-rubber-banding-in-telemetry-table-filter-inputs branch November 28, 2023 16:39

unlikelyzero modified the milestones: Target:3.3.0, Target:4.0.0 Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent rubber-banding in Telemetry Table filter input #7248

Prevent rubber-banding in Telemetry Table filter input #7248

scottbell commented Nov 20, 2023 •

edited

Loading

deploysentinel bot commented Nov 20, 2023 •

edited

Loading

Top 1 Common Error Messages

Test Case Results

Test Case Results

codecov bot commented Nov 20, 2023 •

edited

Loading

unlikelyzero Nov 20, 2023 •

edited

Loading

ozyx Nov 20, 2023

scottbell Nov 21, 2023

unlikelyzero Nov 21, 2023 •

edited

Loading

scottbell Nov 21, 2023

ozyx left a comment

ozyx Nov 20, 2023

scottbell Nov 21, 2023

ozyx Nov 20, 2023

ozyx left a comment

Prevent rubber-banding in Telemetry Table filter input #7248

Prevent rubber-banding in Telemetry Table filter input #7248

Conversation

scottbell commented Nov 20, 2023 • edited Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

deploysentinel bot commented Nov 20, 2023 • edited Loading

Current Playwright Test Results Summary

Current Playwright Test Results Summary

⚠️ Flakes

Top 1 Common Error Messages

Test Case Results

Test Case Results

codecov bot commented Nov 20, 2023 • edited Loading

Codecov Report

unlikelyzero Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

ozyx Nov 20, 2023

Choose a reason for hiding this comment

scottbell Nov 21, 2023

Choose a reason for hiding this comment

unlikelyzero Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

scottbell Nov 21, 2023

Choose a reason for hiding this comment

ozyx left a comment

Choose a reason for hiding this comment

ozyx Nov 20, 2023

Choose a reason for hiding this comment

scottbell Nov 21, 2023

Choose a reason for hiding this comment

ozyx Nov 20, 2023

Choose a reason for hiding this comment

ozyx left a comment

Choose a reason for hiding this comment

scottbell commented Nov 20, 2023 •

edited

Loading

deploysentinel bot commented Nov 20, 2023 •

edited

Loading

codecov bot commented Nov 20, 2023 •

edited

Loading

unlikelyzero Nov 20, 2023 •

edited

Loading

unlikelyzero Nov 21, 2023 •

edited

Loading