-
Notifications
You must be signed in to change notification settings - Fork 77
feat(RHINENG-22212): add selection and loading,error,empty states to Systems View #2796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(RHINENG-22212): add selection and loading,error,empty states to Systems View #2796
Conversation
Reviewer's GuideAdds bulk selection capabilities and loading/error/empty states to the Systems View table, along with a helper to fetch all systems pages via react-query and a new empty-state component. Sequence diagram for SystemsViewTable bulk select all interactionsequenceDiagram
actor User
participant SystemsViewTable
participant BulkSelect
participant onBulkSelect
participant fetchAllSystemsFn as fetchAllSystems
participant QueryClient
participant fetchSystemsFn as fetchSystems
participant HostInventoryApi
User->>SystemsViewTable: Click bulk select all option
SystemsViewTable->>BulkSelect: Render BulkSelect with onSelect
User->>BulkSelect: Select option all
BulkSelect->>onBulkSelect: onSelect(value all)
onBulkSelect->>SystemsViewTable: Read total, pagination.perPage, queryClient
onBulkSelect->>fetchAllSystemsFn: fetchAllSystems({ total, perPage, queryClient })
loop For each page up to totalPages (concurrency <= MAX_CONCURRENT_FETCHES)
fetchAllSystemsFn->>QueryClient: fetchQuery({ queryKey [systems, page, perPage], queryFn })
QueryClient->>fetchSystemsFn: fetchSystems({ page, perPage })
fetchSystemsFn->>HostInventoryApi: getHostList and getHostTags
HostInventoryApi-->>fetchSystemsFn: Hosts and tags data
fetchSystemsFn-->>QueryClient: { results System[], total }
QueryClient-->>fetchAllSystemsFn: Cached page result
end
fetchAllSystemsFn-->>onBulkSelect: All System[] across pages
onBulkSelect->>SystemsViewTable: createRows(allSystems)
SystemsViewTable->>SystemsViewTable: onSelect(true, createdRows)
SystemsViewTable-->>User: All systems appear selected in table
Sequence diagram for SystemsViewTable loading, error, and empty statessequenceDiagram
actor User
participant SystemsViewTable
participant useSystemsQueryHook as useSystemsQuery
participant ReactQuery as ReactQuery_useQuery
participant DataView
participant SkeletonTable
participant EmptySystemsState
participant ErrorState
User->>SystemsViewTable: Navigate to Inventory Systems view
SystemsViewTable->>useSystemsQueryHook: useSystemsQuery({ page, perPage })
useSystemsQueryHook->>ReactQuery: useQuery(queryKey [systems, page, perPage], queryFn)
alt Loading
ReactQuery-->>useSystemsQueryHook: { isLoading true, data undefined }
useSystemsQueryHook-->>SystemsViewTable: isLoading true
SystemsViewTable->>SystemsViewTable: getActiveState() returns loading
SystemsViewTable->>DataView: activeState loading, rows []
DataView->>SkeletonTable: Render loading body state
SkeletonTable-->>User: Shows skeleton rows in table
end
alt Error
ReactQuery-->>useSystemsQueryHook: { isError true, error Error }
useSystemsQueryHook-->>SystemsViewTable: isError true, error
SystemsViewTable->>SystemsViewTable: getActiveState() returns error
SystemsViewTable->>DataView: activeState error
DataView->>ErrorState: Render error body state with titleText and bodyText
ErrorState-->>User: Shows Unable to load data with error message
end
alt Empty
ReactQuery-->>useSystemsQueryHook: { isLoading false, data { results [], total 0 } }
useSystemsQueryHook-->>SystemsViewTable: data [], total 0
SystemsViewTable->>SystemsViewTable: getActiveState() returns empty
SystemsViewTable->>DataView: activeState empty
DataView->>EmptySystemsState: Render empty body state
EmptySystemsState-->>User: Shows No data found message
end
alt Ready
ReactQuery-->>useSystemsQueryHook: { isLoading false, data { results System[], total } }
useSystemsQueryHook-->>SystemsViewTable: data System[], total
SystemsViewTable->>SystemsViewTable: getActiveState() returns ready
SystemsViewTable->>DataView: activeState ready, rows from createRows
DataView-->>User: Shows populated systems table
end
Class diagram for SystemsViewTable data and selection flowclassDiagram
class SystemsViewTable {
+PER_PAGE number
+INITIAL_PAGE number
+NO_HEADER ReactElement
+SystemsViewTable()
+getActiveState() DataViewState
+createRows(data System[]) DataViewTr[]
+onBulkSelect(value string) Promise~void~
}
class System {
+id string
+groups any
+tags any
+system_profile any
+updated string
+culled_timestamp string
+stale_warning_timestamp string
+stale_timestamp string
+per_reporter_staleness any
}
class useSystemsQueryHook {
+useSystemsQuery(params UseSystemsQueryParams) UseSystemsQueryResult
}
class UseSystemsQueryParams {
+page number
+perPage number
}
class UseSystemsQueryResult {
+data System[]
+total number
+isLoading boolean
+isError boolean
+error Error
}
class fetchSystemsFn {
+fetchSystems(params FetchSystemsParams) Promise~FetchSystemsResult~
}
class FetchSystemsParams {
+page number
+perPage number
}
class FetchSystemsResult {
+results System[]
+total number
}
class fetchAllSystemsFn {
+MAX_CONCURRENT_FETCHES number
+fetchAllSystems(params FetchAllSystemsParams) Promise~System[]~
}
class FetchAllSystemsParams {
+total number
+perPage number
+queryClient QueryClient
}
class QueryClient {
+fetchQuery(options QueryOptions) Promise~FetchSystemsResult~
}
class QueryOptions {
+queryKey any[]
+queryFn function
}
class EmptySystemsState {
+EmptySystemsState()
}
class DataView {
+selection any
+activeState DataViewState
}
class DataViewState {
<<enumeration>>
loading
error
empty
ready
}
class BulkSelect {
+pageCount number
+totalCount number
+selectedCount number
+pageSelected boolean
+pagePartiallySelected boolean
+onSelect(value string)
}
SystemsViewTable --> useSystemsQueryHook : uses
SystemsViewTable --> fetchAllSystemsFn : uses
SystemsViewTable --> BulkSelect : renders
SystemsViewTable --> DataView : renders
SystemsViewTable --> EmptySystemsState : renders
SystemsViewTable --> System : displays
useSystemsQueryHook --> fetchSystemsFn : calls
fetchAllSystemsFn --> QueryClient : uses
QueryClient --> fetchSystemsFn : queryFn
fetchSystemsFn --> System : returns
fetchAllSystemsFn --> System : aggregates
DataView --> DataViewState : uses
Flow diagram for SystemsViewTable active state determinationflowchart TD
A[start_getActiveState] --> B{isLoading?}
B -->|yes| C[return loading]
B -->|no| D{isError?}
D -->|yes| E[return error]
D -->|no| F{data exists and data.length == 0?}
F -->|yes| G[return empty]
F -->|no| H[return ready]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In onBulkSelect, the 'none' and 'nonePage' cases fall through to the 'page' logic due to missing breaks, so clearing selection will immediately re-toggle the current page; add explicit breaks to avoid this unintended behavior.
- The 'Last Seen' column is configured with a tooltip of 'Operating system', which looks like it belongs on the 'OS' column; verify and realign the tooltip text with the correct column.
- The ErrorState body text concatenates raw error.name and error.message into user-facing copy; consider mapping these to more user-friendly messages or logging the raw error separately to avoid exposing low-level details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In onBulkSelect, the 'none' and 'nonePage' cases fall through to the 'page' logic due to missing breaks, so clearing selection will immediately re-toggle the current page; add explicit breaks to avoid this unintended behavior.
- The 'Last Seen' column is configured with a tooltip of 'Operating system', which looks like it belongs on the 'OS' column; verify and realign the tooltip text with the correct column.
- The ErrorState body text concatenates raw error.name and error.message into user-facing copy; consider mapping these to more user-friendly messages or logging the raw error separately to avoid exposing low-level details.
## Individual Comments
### Comment 1
<location> `src/components/SystemsViewTable/SystemsViewTable.tsx:123-132` </location>
<code_context>
- // TODO Define selected
- const selected = [];
+ const onBulkSelect = async (value: string) => {
+ switch (value) {
+ case 'none':
+ case 'nonePage':
+ setSelected([]);
+ case 'page':
+ if (selected.length === 0) {
+ onSelect(true, rows);
+ } else {
+ setSelected([]);
+ }
+ break;
+ case 'all':
+ const allSystems = await fetchAllSystems({
+ total,
</code_context>
<issue_to_address>
**issue (bug_risk):** The 'none'/'nonePage' cases in the bulk-select switch fall through into the 'page' case, which will unintentionally toggle the current page selection.
In onBulkSelect, 'none' and 'nonePage' call setSelected([]) but then fall through to 'page', which immediately toggles the current page selection again. This makes 'none' effectively behave like 'page'. Add a break/return after handling 'none'/'nonePage' so those options only clear selections and do not toggle the page selection.
</issue_to_address>
### Comment 2
<location> `src/components/SystemsViewTable/SystemsViewTable.tsx:112-116` </location>
<code_context>
- 'Last seen',
+ { cell: 'Name' },
+ { cell: 'Workspace' },
+ { cell: 'Tags' },
+ { cell: 'OS' },
+ {
+ cell: 'Last Seen',
+ props: { tooltip: 'Operating system' },
+ },
];
</code_context>
<issue_to_address>
**issue (bug_risk):** The tooltip text for the 'Last Seen' column appears to describe the OS, which is likely a copy/paste error.
In the new structure, the OS column is `{ cell: 'OS' }` with no tooltip, while the 'Last Seen' column has `props: { tooltip: 'Operating system' }`. This mismatch is likely a copy/paste error and could confuse users. Please either move the tooltip back to the OS column or change the 'Last Seen' tooltip to accurately describe that column.
</issue_to_address>
### Comment 3
<location> `src/components/SystemsViewTable/hooks/useSystemsQuery.ts:64-73` </location>
<code_context>
+ queryClient: QueryClient;
+}
+
+export const fetchAllSystems = async ({
+ total,
+ perPage,
+ queryClient,
+}: FetchAllSystemsParams) => {
+ if (!total) return [];
+
+ const totalPages = Math.ceil(total / perPage);
+
+ const allPages = (await pAll(
+ Array.from(
+ { length: totalPages },
+ (_, i) => () =>
+ queryClient.fetchQuery({
+ queryKey: ['systems', i + 1, perPage],
+ queryFn: () => fetchSystems({ page: i + 1, perPage }),
+ }),
+ ),
+ { concurrency: MAX_CONCURRENT_FETCHES },
+ )) as FetchSystemsReturnedValue[];
+
+ return allPages.flatMap((page) => page.results);
+};
+
</code_context>
<issue_to_address>
**suggestion (performance):** Selecting all systems triggers fetching every page concurrently, which may be expensive for large datasets.
This schedules a fetch for every page (bounded only by MAX_CONCURRENT_FETCHES). For large totals, this can overload both client and backend when "select all" is used. Consider adding a maximum bulk-select size, using a server-side bulk action (filter-based instead of explicit IDs), or at least disabling "select all" / showing a warning beyond a certain total threshold.
Suggested implementation:
```typescript
const MAX_CONCURRENT_FETCHES = 5;
const MAX_BULK_SELECT_TOTAL = 1000;
interface FetchAllSystemsParams {
total?: number;
perPage: number;
queryClient: QueryClient;
}
```
```typescript
export const fetchAllSystems = async ({
total,
perPage,
queryClient,
}: FetchAllSystemsParams) => {
if (!total) return [];
if (total > MAX_BULK_SELECT_TOTAL) {
throw new Error(
`Cannot bulk select more than ${MAX_BULK_SELECT_TOTAL} systems (requested total: ${total}).`,
);
}
const totalPages = Math.ceil(total / perPage);
const allPages = (await pAll(
Array.from(
{ length: totalPages },
(_, i) => () =>
queryClient.fetchQuery({
queryKey: ['systems', i + 1, perPage],
queryFn: () => fetchSystems({ page: i + 1, perPage }),
}),
),
{ concurrency: MAX_CONCURRENT_FETCHES },
)) as FetchSystemsReturnedValue[];
return allPages.flatMap((page) => page.results);
};
type FetchSystemsReturnedValue = Awaited<ReturnType<typeof fetchSystems>>;
export type System = FetchSystemsReturnedValue['results'][number];
interface FetchSystemsParams {
page: number;
perPage: number;
}
```
To fully implement the behavior implied by this limit, you should also:
1) Update the caller that triggers the "select all" behavior to:
- Catch the error thrown by fetchAllSystems when the total exceeds MAX_BULK_SELECT_TOTAL.
- Disable the "select all" option and/or show a user-facing warning/toast explaining that bulk actions are limited to MAX_BULK_SELECT_TOTAL systems.
2) (Optional but recommended) Replace the hard-coded value 1000 in MAX_BULK_SELECT_TOTAL with a shared configuration value if your codebase already has a central place for such limits.
3) (Optional) If you later implement a server-side bulk action (filter-based, not ID-based), adjust the "select all" flow to prefer that path whenever total > MAX_BULK_SELECT_TOTAL instead of calling fetchAllSystems.
</issue_to_address>
### Comment 4
<location> `src/components/SystemsViewTable/SystemsViewTable.tsx:210-214` </location>
<code_context>
+ />
+ ),
+ empty: <EmptySystemsState />,
+ error: (
+ <ErrorState
+ ouiaId="error-systems-state"
+ titleText="Unable to load data"
+ bodyText={`There was an error retrieving data. ${error ? `${error.name} ${error.message}` : 'Check your connection and reload the page.'}`}
+ />
+ ),
</code_context>
<issue_to_address>
**🚨 issue (security):** Surfacing raw error.name and error.message to end users may expose internal details or sensitive information.
Here we render `error.name` and `error.message` directly in the UI. Depending on the error type (e.g., HTTP errors, stack traces, internal codes), this can reveal implementation details or sensitive data. Consider instead:
- Logging full error details to your monitoring/logging system, and
- Showing a generic user-facing message (optionally with a simple error code).
If you need richer info for debugging, you could conditionally show detailed messages only in development or behind a feature flag.
</issue_to_address>
### Comment 5
<location> `src/components/SystemsViewTable/SystemsViewTable.tsx:60` </location>
<code_context>
- </Bullseye>
- );
- }
+ type ActiveState = DataViewState | 'ready' | undefined;
+ const getActiveState = (): ActiveState => {
+ if (isLoading) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new selection, state, and row-mapping helpers to use smaller, single-purpose functions and avoid subtle bugs like switch fall-through in onBulkSelect.
You can simplify a few of the new abstractions without losing any behavior, and also fix a subtle bug in `onBulkSelect` (fall-through from `'none'`/`'nonePage'` into `'page'`).
1) Inline or simplify `getActiveState`
Using a custom `'ready'` state mixed with `DataViewState` values adds a mental hop. You can keep everything in terms of `DataViewState | undefined` and compute it inline:
```tsx
// remove ActiveState/getActiveState
const activeState: DataViewState | undefined =
isLoading
? 'loading'
: isError
? 'error'
: data && data.length === 0
? 'empty'
: undefined;
return (
<DataView selection={selection} activeState={activeState}>
...
</DataView>
);
```
This removes a custom type alias and keeps `DataView`’s state more obvious.
2) Replace `createRows` (array builder) with a single-row mapper
`createRows` returns an array and is used for both page data and “all systems” which hides the fact that you’re sometimes mapping a page and sometimes everything. A single-row mapper makes that intent clearer and avoids extra indirection:
```tsx
// instead of createRows(data: System[]): DataViewTr[]
const mapSystemToRow = (system: System): DataViewTr => ({
id: system.id,
row: [
<DisplayName
key={`name-${system.id}`}
id={system.id}
props={{}}
{...system}
/>,
<Workspace key={`workspace-${system.id}`} groups={system.groups} />,
<Tags key={`tags-${system.id}`} tags={system.tags} systemId={system.id} />,
<OperatingSystem
key={`os-${system.id}`}
system_profile={system.system_profile}
/>,
<LastSeen
key={`lastseen-${system.id}`}
updated={system.updated}
culled_timestamp={system?.culled_timestamp}
stale_warning_timestamp={system?.stale_warning_timestamp}
stale_timestamp={system?.stale_timestamp}
per_reporter_staleness={system?.per_reporter_staleness}
/>,
],
});
const rows = (data ?? []).map(mapSystemToRow);
// in 'all' case:
const allSystems = await fetchAllSystems({ total, perPage: pagination.perPage, queryClient });
onSelect(true, allSystems.map(mapSystemToRow));
```
This keeps the mapping logic reusable but makes it explicit that you’re mapping collections in the calling code.
3) Simplify and decompose `onBulkSelect` (and remove fall-through)
Right now, `'none'`/`'nonePage'` fall through into `'page'`, which is both surprising and easy to misread. Extract small helpers and make the toggle behavior explicit:
```tsx
const clearSelection = () => setSelected([]);
const togglePageSelection = () => {
if (selected.length === 0) {
onSelect(true, rows);
} else {
clearSelection();
}
};
const selectAllAcrossPages = async () => {
const allSystems = await fetchAllSystems({
total,
perPage: pagination.perPage,
queryClient,
});
onSelect(true, allSystems.map(mapSystemToRow));
};
const onBulkSelect = async (value: string) => {
switch (value) {
case 'none':
case 'nonePage':
clearSelection();
break;
case 'page':
togglePageSelection();
break;
case 'all':
await selectAllAcrossPages();
break;
}
};
```
This reduces branching inside `onBulkSelect` and makes each mode’s behavior self-describing.
4) Encapsulate page selection checks
The expressions for `pageSelected` and `pagePartiallySelected` are a bit dense and tied directly to `rows`. Small helpers keep the intent clear and isolate the selection logic:
```tsx
const isPageSelected = (rows: DataViewTr[]) =>
rows.length > 0 && rows.every((row) => isSelected(row));
const isPagePartiallySelected = (rows: DataViewTr[]) =>
rows.some((row) => isSelected(row)) && !isPageSelected(rows);
<BulkSelect
pageCount={rows.length}
totalCount={total}
selectedCount={selected.length}
pageSelected={isPageSelected(rows)}
pagePartiallySelected={isPagePartiallySelected(rows)}
onSelect={onBulkSelect}
/>
```
If you later move selection to be ID-based, these helpers become the only place you need to update, further simplifying the component.
</issue_to_address>
### Comment 6
<location> `src/components/SystemsViewTable/hooks/useSystemsQuery.ts:4` </location>
<code_context>
+import { type QueryClient, useQuery } from '@tanstack/react-query';
import { generateFilter } from '@redhat-cloud-services/frontend-components-utilities/helpers';
import { getHostList, getHostTags } from '../../../api/hostInventoryApiTyped';
+import pAll from 'p-all';
+
+type FetchSystemsReturnedValue = Awaited<ReturnType<typeof fetchSystems>>;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying fetchAllSystems to use straightforward batched Promise.all calls and explicit System/return types instead of p-all, QueryClient coupling, and derived types.
You can simplify the new pieces without losing functionality or the “select all” behavior.
1) Avoid p-all + QueryClient coupling for fetchAllSystems
Right now `fetchAllSystems` is tightly coupled to React Query’s internals and needs an extra dependency (`p-all`) just to do bounded concurrency. For this use case, a simple batched `Promise.all` keeps things readable and removes the dependency on `QueryClient` and query keys.
For example, you can keep the external behavior (“fetch all pages with bounded concurrency”) with something like:
```ts
const MAX_CONCURRENT_FETCHES = 5;
interface FetchAllSystemsParams {
total?: number;
perPage: number;
}
export const fetchAllSystems = async ({
total,
perPage,
}: FetchAllSystemsParams): Promise<System[]> => {
if (!total) return [];
const totalPages = Math.ceil(total / perPage);
const allSystems: System[] = [];
for (let start = 1; start <= totalPages; start += MAX_CONCURRENT_FETCHES) {
const end = Math.min(start + MAX_CONCURRENT_FETCHES - 1, totalPages);
const pagePromises = [];
for (let page = start; page <= end; page++) {
pagePromises.push(fetchSystems({ page, perPage }));
}
const pageResults = await Promise.all(pagePromises);
for (const page of pageResults) {
allSystems.push(...page.results);
}
}
return allSystems;
};
```
This keeps the concern of “fetch all systems” purely about calling `fetchSystems` and managing concurrency, without needing to know about `QueryClient.fetchQuery` or query keys. If you still need cache warm-up elsewhere, you can keep that separate from this helper.
2) Make System and fetchSystems types explicit
Deriving `System` via `Awaited<ReturnType<typeof fetchSystems>>['results'][number]` is clever but makes the core domain type harder to read.
You can instead make the return type and `System` explicit and keep them in sync:
```ts
export interface System {
id: string;
display_name: string;
// ...other fields we use from the API
tags?: Record<string, string[]>;
}
interface FetchSystemsResult {
results: System[];
total: number;
}
const fetchSystems = async ({
page,
perPage,
}: FetchSystemsParams): Promise<FetchSystemsResult> => {
// existing implementation unchanged, just ensure it returns { results, total }
};
```
This:
- Keeps `System` directly discoverable and readable.
- Removes `Awaited<ReturnType<...>>` and deep indexed access.
- Keeps type safety and behavior intact while reducing mental overhead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
de9c9d4 to
d776d4e
Compare
d776d4e to
fe0e9f6
Compare
ezr-ondrej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of this implementation of all, would it be possible to disable selecting all for now and create a separate tracker for it?
I think it's fine to not be implemented for now and we can discuss with UX how it should be handled and with PM to find out how badly we want to implement it.
johnsonm325
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only change I'm requesting for this PR is the empty state messaging.
src/components/SystemsViewTable/components/EmptySystemsState.tsx
Outdated
Show resolved
Hide resolved
@ezr-ondrej Correct me if I'm wrong, but I'm not seeing a select all in the bulk select options.
That being said, we have had these conversations many times, and I know they want the ability to select all systems. We can still have this discussion though. |
You're right, there is just code supporting it, but the functionality is disabled, so the code is not needed. All the more reason not to include it ;) |
fe0e9f6 to
d983427
Compare
Ondrej I've removed code related to selectAll, and opened tracker for bulkSelect selectAll https://issues.redhat.com/browse/RHINENG-22312 |
d983427 to
4b6d67a
Compare
ezr-ondrej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pushing the TypeScript convert! 🧡
|
/retest |
4b6d67a to
f0711d0
Compare
|
The failures are not relevant - Workspaces are currently broken in stage :) |
refactor: simplify getActiveState to activeState refactor: use mapSystemToRow instead of createRows refactor: add helpers isPageSelected isPagePartiallySelected
f0711d0 to
92f2610
Compare
Requested changes were implemented.

Implements RHINENG-22212
These will be tackled either by upstream patternfly/react-component-groups#824 or we'll address it in local component.
How to test UI of Systems View ?
localStorage.setItem("ui.inventory-views", "true")Summary by Sourcery
Add bulk selection support and loading, error, and empty states to the Systems View table.
New Features:
Enhancements: