[RAC] Store Alerts View table state in localStorage #118207

Zacqary · 2021-11-10T17:58:33Z

Summary

On the Alerts View, this PR stores the state of columns and sort in localStorage.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

Zacqary · 2021-11-11T17:18:29Z

@XavierM Wondering if you can help me figure out how to replicate the things you were doing with TGrid state in the security solution.

I was hoping to add a state subscriber to the TGrid Redux component, and to merge some prefilled selectedEventIds into the initialTGridState there, but I wasn't able to delete the Timeline Plugin's this._store without crashing Kibana.

I ended up attaching the subscriber in getTGrid, but I wasn't able to figure out how to pass in any initial state, so I'm initializing some preselected rows using some hacky useEffect code.

I'm wondering how you managed to get the this._store = undefined line in getTGrid to function without crashing, because I think that would help me implement the subscriber and initial state functionality in a cleaner way.

# Conflicts: # x-pack/plugins/observability/public/pages/alerts/alerts_table_t_grid.tsx

# Conflicts: # x-pack/test/observability_functional/apps/observability/index.ts

# Conflicts: # x-pack/plugins/observability/public/pages/alerts/alerts_table_t_grid.tsx

XavierM · 2021-11-16T17:35:59Z

let me take a look

Zacqary · 2021-11-16T20:27:07Z

Update: Removed the behavior of persisting selected rows rather than trying to find a technical solution, as it's not good UX anyway. If the sort changes or new rows are added, the selected rows could disappear from view but still be selected.

claudiopro

Looks good! I would also add a functional test for adding a column, but looks good to ship as-is.

claudiopro · 2021-11-17T10:44:53Z

x-pack/plugins/timelines/public/plugin.ts

+            if (props.onTGridStateChange) {
+              this._storeUnsubscribe = this._store.subscribe(
+                // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+                throttle(() => props.onTGridStateChange!(getState()), 500)


Is the ! a TS feature? I would expect the optional chaining operator ?. here.

! is to tell TS that a value is definitely not null. In this case, we already did an if (props.onTGridStateChange) check, but for some reason that's not good enough for Typescript here? So I had to add this ! operator to assure the compiler that everything is fine.

You'll notice I had to disable the linting rule:

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion

Usually we don't want to use this ! operator if it's avoidable, but in this case there must be some deeper architectural issues going on if the surrounding if block isn't reassuring the compiler that everything is okay. Out of the scope of this PR, IMO. It's probably something with the different proptypes for standalone vs integrated TGrids.

Ok and now I just tried removing the ! and it's no longer erroring anymore???? What are computers what is typescript I don't know what's going on

LOL NOPE it broke again but my VSCode didn't realize it until after I already merged and pushed

claudiopro · 2021-11-17T10:46:47Z

x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx

@@ -107,6 +107,7 @@ export interface TGridStandaloneProps {
  itemsPerPageOptions: number[];
  query: Query;
  onRuleChange?: () => void;
+  onTGridStateChange?: (state: State) => void;


Nit: what about simply onStateChange? None of the other props references the TGrid explicitly. This one somewhat couples the component interface (props) with the concrete implementation.

claudiopro

Moving to request changes to flag build failures - the PR does not seem complete.

Zacqary · 2021-11-18T17:28:34Z

@elasticmachine merge upstream

Zacqary · 2021-11-18T17:30:54Z

@claudiopro Build failures block merges, so you don't need to switch to Request Changes in the future if you spot that

…ibana into 117698-alert-view-save

claudiopro

LGTM! Ship it 🚀

kibana-ci · 2021-11-22T17:40:54Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`timelines`	869	879	+10

Async chunks

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

id	before	after	diff
`observability`	353.2KB	353.9KB	+733.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`timelines`	164.4KB	164.6KB	+206.0B

Unknown metric groups

API count

id	before	after	diff
`timelines`	987	1013	+26

History

💔 Build #8069 failed 03ffea2
💔 Build #7811 failed 0499ca4
💔 Build #7750 failed b77114a
💔 Build #7200 failed f7e36b2
💛 Build #7097 was flaky 65a9a0a

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

kqualters-elastic

eventually I think it would make sense to have this done via adding a new prop to tGrid, like propsToPersist: ['columns', 'sort'] or something, as a separate plugin shouldn't have to serialize parts of the redux store, that's an implementation detail of the plugin but this works and lgtm 👍

kibanamachine · 2021-11-24T19:34:59Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 118207 or prevent reminders by adding the backport:skip label.

* [RAC] Store Alerts View table state in localStorage * Use Redux store subscriber instead of callback * Fix typecheck * Fix bad merge * Add tests * Remove persisting selected rows * Fix bad merge * onTGridStateChange => onStateChange * Remove non-null assertion * Put non-null assertion back because typescript hates me, personally * Fix checks Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [RAC] Store Alerts View table state in localStorage * Use Redux store subscriber instead of callback * Fix typecheck * Fix bad merge * Add tests * Remove persisting selected rows * Fix bad merge * onTGridStateChange => onStateChange * Remove non-null assertion * Put non-null assertion back because typescript hates me, personally * Fix checks Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [RAC] Store Alerts View table state in localStorage * Use Redux store subscriber instead of callback * Fix typecheck * Fix bad merge * Add tests * Remove persisting selected rows * Fix bad merge * onTGridStateChange => onStateChange * Remove non-null assertion * Put non-null assertion back because typescript hates me, personally * Fix checks Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

elasticmachine · 2021-12-08T16:06:35Z

Pinging @elastic/apm-ui (Team:apm)

* [RAC] Store Alerts View table state in localStorage * Use Redux store subscriber instead of callback * Fix typecheck * Fix bad merge * Add tests * Remove persisting selected rows * Fix bad merge * onTGridStateChange => onStateChange * Remove non-null assertion * Put non-null assertion back because typescript hates me, personally * Fix checks Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[RAC] Store Alerts View table state in localStorage

9d342a9

Zacqary added release_note:enhancement v8.0.0 Feature:RAC label obsolete v8.1.0 Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" labels Nov 10, 2021

Zacqary added 2 commits November 10, 2021 15:46

Use Redux store subscriber instead of callback

69c52f6

Fix typecheck

acea005

Zacqary added 4 commits November 11, 2021 11:23

Merge remote-tracking branch 'upstream/main' into 117698-alert-view-save

a0e715c

# Conflicts: # x-pack/plugins/observability/public/pages/alerts/alerts_table_t_grid.tsx

Fix bad merge

f86cb97

Add tests

c032c22

Merge remote-tracking branch 'upstream/main' into 117698-alert-view-save

10627f0

# Conflicts: # x-pack/test/observability_functional/apps/observability/index.ts

Zacqary marked this pull request as ready for review November 15, 2021 18:30

Zacqary requested a review from a team as a code owner November 15, 2021 18:30

elastic deleted a comment from kibanamachine Nov 15, 2021

Zacqary added 2 commits November 15, 2021 13:46

Merge remote-tracking branch 'upstream/main' into 117698-alert-view-save

65a9a0a

Merge remote-tracking branch 'upstream/main' into 117698-alert-view-save

6acf29e

# Conflicts: # x-pack/plugins/observability/public/pages/alerts/alerts_table_t_grid.tsx

Zacqary requested a review from a team November 16, 2021 16:57

Remove persisting selected rows

e54e2a9

Fix bad merge

f7e36b2

claudiopro approved these changes Nov 17, 2021

View reviewed changes

claudiopro suggested changes Nov 17, 2021

View reviewed changes

Merge branch 'main' into 117698-alert-view-save

64e537a

Zacqary added 2 commits November 18, 2021 11:40

onTGridStateChange => onStateChange

0d59823

Merge branch '117698-alert-view-save' of https://github.com/Zacqary/k…

0bcf4fe

…ibana into 117698-alert-view-save

Zacqary added 5 commits November 18, 2021 11:43

Remove non-null assertion

b77114a

Put non-null assertion back because typescript hates me, personally

0499ca4

Fix checks

bc3600c

Merge remote-tracking branch 'upstream/main' into 117698-alert-view-save

03ffea2

Merge remote-tracking branch 'upstream/main' into 117698-alert-view-save

2c36887

claudiopro approved these changes Nov 22, 2021

View reviewed changes

Zacqary enabled auto-merge (squash) November 22, 2021 17:33

kqualters-elastic approved these changes Nov 22, 2021

View reviewed changes

Zacqary merged commit c012dd8 into elastic:main Nov 22, 2021

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 24, 2021

Zacqary mentioned this pull request Nov 24, 2021

[8.0] [RAC] Store Alerts View table state in localStorage (#118207) #119678

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 25, 2021

KOTungseth added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAC] Store Alerts View table state in localStorage #118207

[RAC] Store Alerts View table state in localStorage #118207

Zacqary commented Nov 10, 2021 •

edited

Loading

Zacqary commented Nov 11, 2021

XavierM commented Nov 16, 2021

Zacqary commented Nov 16, 2021

claudiopro left a comment

claudiopro Nov 17, 2021

Zacqary Nov 18, 2021 •

edited

Loading

Zacqary Nov 18, 2021

Zacqary Nov 18, 2021

claudiopro Nov 17, 2021

claudiopro left a comment

Zacqary commented Nov 18, 2021

Zacqary commented Nov 18, 2021

claudiopro left a comment

kibana-ci commented Nov 22, 2021

API count

kqualters-elastic left a comment

kibanamachine commented Nov 24, 2021

elasticmachine commented Dec 8, 2021

[RAC] Store Alerts View table state in localStorage #118207

[RAC] Store Alerts View table state in localStorage #118207

Conversation

Zacqary commented Nov 10, 2021 • edited Loading

Summary

Checklist

Zacqary commented Nov 11, 2021

XavierM commented Nov 16, 2021

Zacqary commented Nov 16, 2021

claudiopro left a comment

Choose a reason for hiding this comment

claudiopro Nov 17, 2021

Choose a reason for hiding this comment

Zacqary Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Zacqary Nov 18, 2021

Choose a reason for hiding this comment

Zacqary Nov 18, 2021

Choose a reason for hiding this comment

claudiopro Nov 17, 2021

Choose a reason for hiding this comment

claudiopro left a comment

Choose a reason for hiding this comment

Zacqary commented Nov 18, 2021

Zacqary commented Nov 18, 2021

claudiopro left a comment

Choose a reason for hiding this comment

kibana-ci commented Nov 22, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kqualters-elastic left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 24, 2021

elasticmachine commented Dec 8, 2021

Zacqary commented Nov 10, 2021 •

edited

Loading

Zacqary Nov 18, 2021 •

edited

Loading