[Uptime] clear ping state when PingList component in unmounted #88321

dominiqueclarke · 2021-01-14T12:17:39Z

Summary

Fixes elastic/uptime#285
Addresses elastic/uptime#286 for PingList

This PR removes cached ping list state. Previously, when switching between monitors, the previous ping list state would show for a brief moment before the usePingList hook kicks in and switches the state to loading. This PR resolves that by cleaning ping list state when the PingList component unmounts, ensuring that new ping list state is always fetched freshed for a different monitor.

You can see the new behavior here
https://drive.google.com/file/d/1Zm__8vbPjo1TZ30IRMLXIxzFdWsgqFPQ/view?usp=sharing

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-01-14T12:17:44Z

Pinging @elastic/uptime (Team:uptime)

dominiqueclarke · 2021-01-14T12:20:10Z

x-pack/plugins/uptime/public/lib/helper/test_helpers.ts

@@ -62,6 +62,10 @@ export function mockReduxHooks(response?: any) {
  jest.spyOn(redux, 'useSelector').mockReturnValue(response);
 }

+export function mockDispatch() {


Mocking useSelector was causing issues for me, since this component called on two different selectors, so I am only mocking dispatch. Mocking useSelector is less critical when using the default app state within a mock redux provider, as we are in the new rtl custom render.

dominiqueclarke · 2021-01-14T12:20:31Z

x-pack/plugins/uptime/public/state/reducers/ping_list.ts

@@ -26,6 +26,10 @@ type PingListPayload = PingsResponse & Error;

 export const pingListReducer = handleActions<PingListState, PingListPayload>(
  {
+    [String(clearPings)]: (state) => ({


Could also be resetPings.

I'm fine with clearPings.

dominiqueclarke · 2021-01-14T12:22:19Z

x-pack/plugins/uptime/public/components/monitor/ping_list/ping_list.tsx

@@ -200,6 +207,15 @@ export const PingList = () => {
        itemId="docId"
        itemIdToExpandedRowMap={expandedRows}
        pagination={pagination}
+        noItemsMessage={
+          loading
+            ? i18n.translate('xpack.uptime.pingList.pingsLoadingMesssage', {


I'm not sure who determines what the appropriate content is in these cases. Please let me know if this default content is appropriate or need to be run past product.

I think 'Loading pings' might accomplish the same thing. @andrewvc any thought on this copy?

On the ticket Paul suggested the more generic Loading history.. could work well, since this is a history table.

dominiqueclarke · 2021-01-14T12:23:28Z

x-pack/plugins/uptime/public/state/actions/ping.ts

@@ -12,6 +12,8 @@ import {
  GetPingsParams,
 } from '../../../common/runtime_types';

+export const clearPings = createAction('CLEAR PINGS');


I see some actions use SNAKE_CASE and others just use spaces. Which should I be using, or does it matter?

I think largely we are not super strict on the formatting of these since in practice no one sees them or needs to do anything with them.

dominiqueclarke · 2021-01-14T13:51:59Z

@elasticmachine merge upstream

justinkambic

LGTM

justinkambic · 2021-01-14T20:46:42Z

x-pack/plugins/uptime/public/components/monitor/ping_list/ping_list.test.tsx

+      failedSteps: { steps: [], checkGroup: '1-f-4d-4f' },
+    });
+    const { getByText } = render(<PingList />);
+    expect(getByText('Loading ping history')).toBeInTheDocument();


👍

Been doing these all week, it's making the test suites so much nicer.

justinkambic · 2021-01-14T20:48:02Z

x-pack/plugins/uptime/public/components/monitor/ping_list/ping_list.tsx

@@ -200,6 +207,15 @@ export const PingList = () => {
        itemId="docId"
        itemIdToExpandedRowMap={expandedRows}
        pagination={pagination}
+        noItemsMessage={
+          loading
+            ? i18n.translate('xpack.uptime.pingList.pingsLoadingMesssage', {


I think 'Loading pings' might accomplish the same thing. @andrewvc any thought on this copy?

justinkambic · 2021-01-14T20:49:09Z

x-pack/plugins/uptime/public/state/reducers/ping_list.ts

@@ -26,6 +26,10 @@ type PingListPayload = PingsResponse & Error;

 export const pingListReducer = handleActions<PingListState, PingListPayload>(
  {
+    [String(clearPings)]: (state) => ({


I'm fine with clearPings.

justinkambic · 2021-01-14T20:50:27Z

x-pack/plugins/uptime/public/state/actions/ping.ts

@@ -12,6 +12,8 @@ import {
  GetPingsParams,
 } from '../../../common/runtime_types';

+export const clearPings = createAction('CLEAR PINGS');


I think largely we are not super strict on the formatting of these since in practice no one sees them or needs to do anything with them.

justinkambic · 2021-01-14T20:52:55Z

x-pack/plugins/uptime/public/components/monitor/ping_list/ping_list.tsx

@@ -54,6 +55,12 @@ export const PingList = () => {
    Object.keys(expandedRows).filter((e) => !pings.some(({ docId }) => docId === e))
  );

+  useEffect(() => {


Nice simple strategy here, small footprint, runs once. I smoke tested this and it seemed to work great.

…/dominiqueclarke/kibana into fix/285-remove-cached-ping-state

kibanamachine · 2021-01-15T18:15:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1b6e12d

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`uptime`	1.1MB	1.1MB	+647.0B

History

💚 Build #98852 succeeded 145e69d
💔 Build #98839 failed 944704c
💔 Build #98831 failed 861112b8388cf315d180bd0e36e67d5fa3606422

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ic#88321) * clear ping state when PingList component in unmounted * update ping list content Co-authored-by: Kibana Machine <[email protected]>

* master: (33 commits) [Security Solution][Case] Fix patch cases integration test with alerts (elastic#88311) [Security Solutions][Detection Engine] Removes duplicate API calls (elastic#88420) Fix log msg (elastic#88370) [Test] Add tag cloud visualization to dashboard in functional test for reporting (elastic#87600) removing kibana-core-ui from codeowners (elastic#88111) [Alerting] Migrate Event Log plugin to TS project references (elastic#81557) [Maps] fix zooming while drawing shape filter logs errors in console (elastic#88413) Porting fixes 1 (elastic#88477) [APM] Explicitly set environment for cross-service links (elastic#87481) chore(NA): remove mocha junit ci integrations (elastic#88129) [APM] Only display relevant sections for rum agent in service overview (elastic#88410) [Enterprise Search] Automatically mock shared logic files (elastic#88494) [APM] Disable Create custom link button on Transaction details page for read-only users [Docs] clean-up vega map reference documenation (elastic#88487) [Security Solution] Fix Timeline event details layout (elastic#88377) Change DELETE to POST for _bulk_delete to avoid incompatibility issues (elastic#87914) [Monitoring] Change cloud messaging on no data page (elastic#88375) [Uptime] clear ping state when PingList component in unmounted (elastic#88321) [APM] Consistent terminology for latency and throughput (elastic#88452) fix copy (elastic#88481) ...

… (#88506) * clear ping state when PingList component in unmounted * update ping list content Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

… (#88504) * clear ping state when PingList component in unmounted * update ping list content Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

dominiqueclarke requested a review from a team as a code owner January 14, 2021 12:17

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jan 14, 2021

dominiqueclarke added release_note:fix v7.11.0 v7.12.0 v8.0.0 labels Jan 14, 2021

dominiqueclarke changed the title ~~clear ping state when PingList component in unmounted~~ [Uptime] clear ping state when PingList component in unmounted Jan 14, 2021

dominiqueclarke added the bug Fixes for quality problems that affect the customer experience label Jan 14, 2021

dominiqueclarke commented Jan 14, 2021

View reviewed changes

dominiqueclarke force-pushed the fix/285-remove-cached-ping-state branch from 45be5c8 to 861112b Compare January 14, 2021 12:23

clear ping state when PingList component in unmounted

944704c

dominiqueclarke force-pushed the fix/285-remove-cached-ping-state branch from 861112b to 944704c Compare January 14, 2021 13:02

Merge branch 'master' into fix/285-remove-cached-ping-state

145e69d

justinkambic approved these changes Jan 14, 2021

View reviewed changes

dominiqueclarke added 2 commits January 15, 2021 10:55

update ping list content

751e881

Merge branch 'fix/285-remove-cached-ping-state' of https://github.com…

1b6e12d

…/dominiqueclarke/kibana into fix/285-remove-cached-ping-state

dominiqueclarke merged commit 73cd1a0 into elastic:master Jan 15, 2021

dominiqueclarke deleted the fix/285-remove-cached-ping-state branch January 15, 2021 18:37

dominiqueclarke mentioned this pull request Jan 15, 2021

[7.x] [Uptime] clear ping state when PingList component in unmounted (#88321) #88504

Merged

dominiqueclarke mentioned this pull request Jan 15, 2021

[7.11] [Uptime] clear ping state when PingList component in unmounted (#88321) #88506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime] clear ping state when PingList component in unmounted #88321

[Uptime] clear ping state when PingList component in unmounted #88321

dominiqueclarke commented Jan 14, 2021 •

edited

Loading

elasticmachine commented Jan 14, 2021

dominiqueclarke Jan 14, 2021

dominiqueclarke Jan 14, 2021

justinkambic Jan 14, 2021

dominiqueclarke Jan 14, 2021

justinkambic Jan 14, 2021

dominiqueclarke Jan 15, 2021

dominiqueclarke Jan 14, 2021

justinkambic Jan 14, 2021

dominiqueclarke commented Jan 14, 2021

justinkambic left a comment

justinkambic Jan 14, 2021

justinkambic Jan 14, 2021

justinkambic Jan 14, 2021

justinkambic Jan 14, 2021

justinkambic Jan 14, 2021

kibanamachine commented Jan 15, 2021

[Uptime] clear ping state when PingList component in unmounted #88321

[Uptime] clear ping state when PingList component in unmounted #88321

Conversation

dominiqueclarke commented Jan 14, 2021 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jan 14, 2021

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

dominiqueclarke commented Jan 14, 2021

justinkambic 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

kibanamachine commented Jan 15, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

dominiqueclarke commented Jan 14, 2021 •

edited

Loading