Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/designs/main-pages/02-structure-and-anatomy.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Every main page follows the same structural pattern, in this order:
2. **URL-synced pagination/search state** (`useListPageSearchParams`)
3. **API version retrieval** (Redux selector)
4. **Data fetching** (RTK Query hook for initial load)
5. **Search mutation** (RTK Query mutation for explicit search)
5. **Search mutation** (RTK Query mutation for submit-only search)
6. **Selection management** (selected items, bulk selector logic)
7. **Button state management** (delete, enable, disable disabled states)
8. **Data wrappers** (prop bundles for child components)
Expand Down
66 changes: 57 additions & 9 deletions doc/designs/main-pages/03-walkthrough-init-fetch.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ The `pathname` must be registered in `AppRoutes.tsx` and `NavRoutes.ts`.
const lastIdx = page * perPage;

const dataResponse = useGettingMyEntitiesQuery({
searchValue: "",
searchValue: searchValue,
sizeLimit: 0,
apiVersion: apiVersion || API_VERSION_BACKUP,
startIdx: firstIdx,
Expand All @@ -41,6 +41,11 @@ The `pathname` must be registered in `AppRoutes.tsx` and `NavRoutes.ts`.
const { data: batchResponse, isLoading, isFetching, error } = dataResponse;
```

> **Important:** Always pass `searchValue` (not `""`) to the query hook. RTK Query
> auto-refetches whenever its parameters change (`searchValue`, `startIdx`, `stopIdx`),
> so both pagination and filtering are handled automatically. Using a hardcoded `""`
> causes a race condition where the unfiltered query response overwrites search results.

## Step 4: Derive State with useMemo (Recommended)

Use `useMemo` to derive `elementsList` and `totalCount` from the query response — **do not** use `useEffect` + `useState` to sync state.
Expand All @@ -64,10 +69,14 @@ Use it to type the search data state:

// Derive elementsList and totalCount
const { elementsList, totalCount } = useMemo(() => {
// Search results are fetched with stopIdx: 100 (all matches at once),
// so paginate them client-side using page/perPage.
if (isSearchActive && searchData) {
const start = (page - 1) * perPage;
const end = start + perPage;
return {
elementsList: searchData.elementsList,
totalCount: searchData.totalCount,
elementsList: searchData.elementsList.slice(start, end),
totalCount: searchData.elementsList.length,
};
}

Expand All @@ -81,7 +90,7 @@ Use it to type the search data state:
}

return { elementsList: [], totalCount: 0 };
}, [batchResponse, isSearchActive, searchData]);
}, [batchResponse, isSearchActive, searchData, page, perPage]);

// Derive showTableRows from loading states
const showTableRows = useMemo(() => {
Expand Down Expand Up @@ -124,22 +133,50 @@ This pattern avoids eslint warnings about calling `setState` in `useEffect`.
> **Note:** No manual `useEffect` for pagination is needed. RTK Query automatically
> re-fetches when `startIdx`/`stopIdx` change (derived from `page`/`perPage`).

## Step 7: Search Handler
## Step 7: Search Value Update Handler

`SearchInputLayout` buffers keystrokes locally — it does **not** call `updateSearchValue`
on every keystroke. Instead, it calls `updateSearchValue(value)` and
`submitSearchValue(value)` only when the user presses Enter or clicks the search button.
This avoids firing an API request on every keystroke.

`updateSearchValue` resets pagination to page 1 and updates the committed search value
(which drives the RTK Query parameter and URL sync):

```tsx
const updateSearchValue = (value: string) => {
setPage(1);
setSearchValue(value);
};
```

> **Do not** add search-as-you-type behavior by calling `setSearchValue` inside
> `SearchInputLayout`'s `onChange`. The component deliberately buffers input locally
> and only propagates on submit or clear.

## Step 8: Search Submit Handler

`submitSearchValue` is called by `SearchInputLayout` when the user presses Enter or
clicks the search button. It receives the **current input value** as an argument to
avoid stale closures (since `updateSearchValue` and `submitSearchValue` are called in
the same event handler, the React state from `updateSearchValue` has not yet committed).

```tsx
const [searchEntities, searchResult] = useSearchMyEntitiesEntriesMutation({});
const [searchDisabled, setSearchIsDisabled] = useState(false);

const submitSearchValue = () => {
const submitSearchValue = (value?: string) => {
const search = value ?? searchValue;
setPage(1);
setSearchIsDisabled(true);
setIsSearchActive(true);

searchEntities({
searchValue,
searchValue: search,
sizeLimit: 0,
apiVersion: apiVersion || API_VERSION_BACKUP,
startIdx: firstIdx,
stopIdx: lastIdx,
startIdx: 0,
stopIdx: 100,
Comment thread
carma12 marked this conversation as resolved.
}).then((result) => {
if ("data" in result) {
const searchError = result.data?.error;
Expand Down Expand Up @@ -170,6 +207,17 @@ This pattern avoids eslint warnings about calling `setState` in `useEffect`.
};
```

> **Important — `value` parameter:** Always use the `value` argument (falling back to
> `searchValue` with `??`) instead of reading `searchValue` directly from the closure.
> Both `updateSearchValue` and `submitSearchValue` are called in the same React event
> handler, so the state set by `updateSearchValue` is not yet available when
> `submitSearchValue` runs.

> **Important — `stopIdx: 100`:** Use a fixed upper bound (100) instead of `perPage`.
> The LDAP backend has a size limit close to this value, and using `perPage` (e.g. 10)
> would miss entries beyond the first page of results when searching from a page other
> than page 1.
Comment on lines +216 to +219

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not an LDAP limitation, it's a global setting, that is overridable

Image

I think we could even omit the stopIdx.

https://webui.ipa.test/ipa/ui/#/e/config/details

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But even if you put a wild number like, e.g. 1000 there, the limitation will be around ~200 elements.


## Legacy Pattern (Avoid)

The older pattern using `useEffect` + `setState` triggers eslint warnings:
Expand Down
2 changes: 1 addition & 1 deletion doc/designs/main-pages/04-walkthrough-selection-toolbar.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ These objects group related props for child components:

const searchValueData = {
searchValue,
updateSearchValue: setSearchValue,
updateSearchValue,
submitSearchValue,
};

Expand Down
48 changes: 48 additions & 0 deletions doc/designs/sub-pages/06a-membership-custom.md
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,54 @@ return (
- Modals should be **outside** `TabLayout` to avoid z-index issues
- Inner `Tab` title can include a `Badge` showing member count

## Search Behavior in Membership Tabs

`SearchInputLayout` buffers keystrokes locally — the `searchValue` state is only updated
when the user presses Enter, clicks the search button, or clears the input. This avoids
unnecessary re-renders and API calls on every keystroke.

### Wiring the MemberOfToolbar

```tsx
<MemberOfToolbar
searchText={searchValue}
onSearchTextChange={setSearchValue}
onSearch={() => {}}
searchPlaceholder="Search members"
searchAriaLabel="Search members"
// ... other props
/>
```

- **`onSearchTextChange`** is called by `SearchInputLayout` only on submit or clear
(not on every keystroke). It updates `searchValue` which drives the client-side filter.
- **`onSearch`** can remain `() => {}` — membership tabs use client-side filtering
via `searchValue`, so no mutation-based search is needed.

### Client-Side Filtering Pattern

Use `searchValue` to filter the membership list. Since `searchValue` only changes on
submit, the filter is applied only when the user explicitly searches:

```tsx
const filteredMembers = React.useMemo(() => {
let toLoad = [...memberList].sort();
if (searchValue) {
const q = searchValue.toLowerCase();
toLoad = toLoad.filter((name) => name.toLowerCase().includes(q));
}
return toLoad;
}, [memberList, searchValue]);

const namesToLoad = React.useMemo(
() => paginate(filteredMembers, page, perPage),
[filteredMembers, page, perPage]
);
```

> **Do not** wire `onSearchTextChange` to trigger API calls or heavy computations.
> It is called once per search submit, not per keystroke.

## Handling API Array Responses

The IPA API often returns single values as arrays:
Expand Down
2 changes: 1 addition & 1 deletion doc/designs/sub-pages/06c-membership-creating-new.md
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ After gathering information, the component MUST include:

| Required Feature | Description |
|------------------|-------------|
| **Toolbar** | `MemberOfToolbar` with search, pagination, Add/Delete buttons |
| **Toolbar** | `MemberOfToolbar` with search (submit-only, not per-keystroke), pagination, Add/Delete buttons |
| **Table** | `MemberTable` displaying the member list |
| **Pagination** | Bottom pagination component |
| **Add button** | Enabled, opens add modal |
Expand Down
2 changes: 1 addition & 1 deletion src/components/MemberOf/MemberOfToolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ interface MemberOfToolbarProps {
// search
searchText: string;
onSearchTextChange: (value: string) => void;
onSearch: () => void;
onSearch: (value?: string) => void;
searchPlaceholder: string;
searchAriaLabel: string;

Expand Down
9 changes: 5 additions & 4 deletions src/components/layouts/DualListLayout/DualListLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,12 @@ const DualListTableLayoutInner = (props: DualListProps) => {

// Issue a search using a specific search value
const [retrieveIDs] = useGetIDListMutation({});
const submitSearchValue = () => {
const submitSearchValue = (value?: string) => {
const search = value ?? searchValue;
setSearchIsDisabled(true);
if (props.availableOptions === undefined) {
retrieveIDs({
searchValue: props.availableOptions || searchValue,
searchValue: props.availableOptions || search,
sizeLimit: 200,
startIdx: 0,
stopIdx: 200,
Expand All @@ -189,9 +190,9 @@ const DualListTableLayoutInner = (props: DualListProps) => {
}
};

function doSearch() {
function doSearch(value?: string) {
setStatus("searching");
submitSearchValue();
submitSearchValue(value);
}

const searchValueData = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe("SearchInputLayout Component", () => {
expect(searchInput).toHaveValue("current search value");
});

it("calls updateSearchValue when input changes", () => {
it("does not call updateSearchValue on keystroke (buffers locally)", () => {
render(
<SearchInputLayout
dataCy="search-input"
Expand All @@ -72,7 +72,8 @@ describe("SearchInputLayout Component", () => {
const searchInput = screen.getByRole("textbox");
fireEvent.change(searchInput, { target: { value: "new search value" } });

expect(mockUpdateSearchValue).toHaveBeenCalledWith("new search value");
expect(mockUpdateSearchValue).not.toHaveBeenCalled();
expect(searchInput).toHaveValue("new search value");
});

it("calls updateSearchValue with empty string when reset button is clicked", () => {
Expand All @@ -94,17 +95,21 @@ describe("SearchInputLayout Component", () => {
expect(mockUpdateSearchValue).toHaveBeenCalledWith("");
});

it("calls submitSearchValue when search button is clicked", () => {
it("calls updateSearchValue and submitSearchValue with input value on search submit", () => {
render(
<SearchInputLayout
dataCy="search-input"
searchValueData={defaultSearchValueData}
/>
);

const searchInput = screen.getByRole("textbox");
fireEvent.change(searchInput, { target: { value: "typed value" } });

const searchButton = screen.getByRole("button", { name: /search/i });
fireEvent.click(searchButton);

expect(mockSubmitSearchValue).toHaveBeenCalled();
expect(mockUpdateSearchValue).toHaveBeenCalledWith("typed value");
expect(mockSubmitSearchValue).toHaveBeenCalledWith("typed value");
});
});
40 changes: 21 additions & 19 deletions src/components/layouts/SearchInputLayout/SearchInputLayout.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
/* eslint-disable @typescript-eslint/no-unused-vars */
import React, { FormEvent, SyntheticEvent } from "react";
import React from "react";
// PatternFly
import { SearchInput } from "@patternfly/react-core";

interface SearchValueData {
searchValue: string;
updateSearchValue: (value: string) => void;
submitSearchValue?: () => void;
submitSearchValue?: (value?: string) => void;
}

interface PropsToSearchInput {
Expand All @@ -19,15 +18,19 @@ interface PropsToSearchInput {
}

const SearchInputLayout = (props: PropsToSearchInput) => {
const onSearchChange = (
_event: FormEvent<HTMLInputElement>,
value: string
) => {
props.searchValueData.updateSearchValue(value);
};
const [inputValue, setInputValue] = React.useState(
props.searchValueData.searchValue
);

const prevSearchValue = React.useRef(props.searchValueData.searchValue);
if (prevSearchValue.current !== props.searchValueData.searchValue) {
prevSearchValue.current = props.searchValueData.searchValue;
setInputValue(props.searchValueData.searchValue);
}

const onSearchClear = (_event: SyntheticEvent<HTMLButtonElement, Event>) => {
props.searchValueData.updateSearchValue("");
const onSearchSubmit = () => {
props.searchValueData.updateSearchValue(inputValue);
props.searchValueData.submitSearchValue?.(inputValue);
};

return (
Expand All @@ -36,14 +39,13 @@ const SearchInputLayout = (props: PropsToSearchInput) => {
name={props.name}
aria-label={props.ariaLabel}
placeholder={props.placeholder}
value={props.searchValueData.searchValue}
onSearch={
props.searchValueData.submitSearchValue
? props.searchValueData.submitSearchValue
: () => void undefined
}
onChange={onSearchChange}
onClear={onSearchClear}
value={inputValue}
onSearch={onSearchSubmit}
onChange={(_event, value: string) => setInputValue(value)}
onClear={() => {
setInputValue("");
props.searchValueData.updateSearchValue("");
}}
isDisabled={props.isDisabled}
/>
);
Expand Down
13 changes: 8 additions & 5 deletions src/pages/ActiveUsers/ActiveUsers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ const ActiveUsers = () => {

// Update search input valie
const updateSearchValue = (value: string) => {
setPage(1);
setSearchValue(value);
};

Expand All @@ -245,18 +246,20 @@ const ActiveUsers = () => {
const [retrieveUser] = useSearchEntriesMutation({});

// Issue a search using a specific search value
const submitSearchValue = () => {
const submitSearchValue = (value?: string) => {
const search = value ?? searchValue;
setPage(1);
setShowTableRows(false);
setSearchIsDisabled(true);
setUsersTotalCount(0);

// Make search via API call
retrieveUser({
searchValue: searchValue,
searchValue: search,
sizeLimit: 0,
apiVersion: apiVersion || API_VERSION_BACKUP,
startIdx: firstUserIdx,
stopIdx: lastUserIdx,
startIdx: 0,
stopIdx: 100,
entryType: "user",
} as GenericPayload).then((result) => {
// Manage new response here
Expand Down Expand Up @@ -292,7 +295,7 @@ const ActiveUsers = () => {
}

setUsersTotalCount(totalCount);
setActiveUsersList(usersList);
setActiveUsersList(usersList.slice(0, perPage));
// Show table elements
setShowTableRows(true);
}
Expand Down
Loading
Loading