kibana
kibana copied to clipboard
[Security Solution] Change initial totalCount value
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.
- [x] Unit or functional tests were updated or added to match the most common scenarios
@elasticmachine merge upstream
@elasticmachine merge upstream
@elasticmachine merge upstream
@elasticmachine merge upstream
@elasticmachine merge upstream
Pinging @elastic/security-threat-hunting (Team:Threat Hunting)
Pinging @elastic/security-solution (Team: SecuritySolution)
@elasticmachine merge upstream
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?
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 :)
:broken_heart: Build Failed
- Buildkite Build
- Commit: 43e04af2ce0a6a325a326711e01920b67ca4991d
- Interpreting CI Failures
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