kibana icon indicating copy to clipboard operation
kibana copied to clipboard

[Security Solution] Change initial totalCount value

Open angorayc opened this issue 3 years ago • 11 comments

Summary

original issue: https://github.com/elastic/kibana/issues/136672

Previously we put -1 as the initial value of totalCount to distinguish between before / after fetching data, in https://github.com/elastic/kibana/issues/136672 we agreed to replace them with undefined.

Checklist

Delete any items that are not applicable to this PR.

angorayc avatar Aug 01 '22 15:08 angorayc

@elasticmachine merge upstream

angorayc avatar Aug 02 '22 15:08 angorayc

@elasticmachine merge upstream

angorayc avatar Aug 03 '22 15:08 angorayc

@elasticmachine merge upstream

angorayc avatar Aug 03 '22 16:08 angorayc

@elasticmachine merge upstream

angorayc avatar Aug 03 '22 18:08 angorayc

@elasticmachine merge upstream

angorayc avatar Aug 04 '22 09:08 angorayc

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

elasticmachine avatar Aug 04 '22 10:08 elasticmachine

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine avatar Aug 04 '22 10:08 elasticmachine

@elasticmachine merge upstream

angorayc avatar Aug 08 '22 08:08 angorayc

Hey @angorayc, I downloaded your code and spent some time walking through it. I was hoping we could chat through some of your changes. Im thinking instead of adding ambiguity with undefined and null, could we instead default the value to 0 if it's not set?

const {
    loading,
    result: response,
    search,
    refetch,
    inspect,
  } = useSearchStrategy<HostsQueries.hosts>({
    factoryQueryType: HostsQueries.hosts,
    initialResult: {
      edges: [],
      totalCount: 0, <------
      ...
    },
   ...
  }); 

and

const hostsResponse = useMemo(
    () => ({
      totalCount: response.totalCount || 0, <---
    }),
    [
      ...
      response.totalCount,
      ...
    ]
  );

Then throughout the code base where there is logic to determine if its null or undefined would be unnecessary and we wouldnt have to change types in so many places that already have totalCount: number. It's very possible Im missing something though 🤔

Likewise, I am curious about a similar thing for headerCount which I think is coming in from totalCount as well.

On the server side it seems like it will also always send totalCount, whats the undefined / null being used for?

jamster10 avatar Aug 08 '22 20:08 jamster10

Hey @jamster10 , the reason why I didn't set the total count was because in x-pack/plugins/security_solution/public/common/components/paginated_table/index.tsx we would like to know if it is the first-time loading of the component, so we need a different value to distinguish if it is the initial loading (previously we set it to -1). If we set the initial value to 0, then we won't be able to tell whether it is initial loading or the value is actually 0 after fetching data. Please feel free to let me know if you'd like to chat about this :)

angorayc avatar Aug 09 '22 10:08 angorayc

:broken_heart: Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #4 / Paginated Table Component rendering it renders the loading panel at the beginning

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
securitySolution 5.6MB 5.6MB +1.1KB

History

  • :yellow_heart: Build #63501 was flaky d374fba2e3418f7c9893dffb114f4f3de7cceb64
  • :green_heart: Build #63092 succeeded da6a7b3bfe576fd8b71707c657d2de035270d28a
  • :green_heart: Build #62958 succeeded 9c58de41c71fbab6810d81a887464a0c9134bec2
  • :green_heart: Build #62910 succeeded b3add00decee0a8fc653f3b65d5d66131df719a2
  • :green_heart: Build #62864 succeeded 729815618af00d36bd1e9543e1540e2c73ed8800

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

kibana-ci avatar Aug 09 '22 14:08 kibana-ci