Add 'Automount locations' main page

[veronnicka]

Summary by Sourcery

Add a new Automount locations management page and wire it into navigation and routing.

New Features:

• Introduce an Automount locations main page with listing, search, pagination, and bulk selection actions.
• Add modals to create and delete automount locations from the UI.
• Expose RPC endpoints for listing, searching, creating, showing, and deleting automount locations via the backend API.

Enhancements:

• Extend navigation and routing to include Automount locations under Network services.
• Add shared utilities and data types for AutomountLocation entities and their API conversions.

[sourcery-ai]

Hey - I've found 6 issues, and left some high level feedback:

• In rpcAutomountLocations.ts, both getAutomountLocationsFullData and searchAutomountLocationsEntries handle errors from the initial automountlocation_find call but ignore possible showResult.error; consider checking showResult.error and returning it instead of always casting showResult.data to avoid silent failures.
• In DeleteAutomountLocationsModal, the ModalWithFormLayout formId (remove-automount-locations-modal) does not match the form attribute used on the delete button (delete-automount-locations-modal), which can break form submission semantics; align these IDs.
• The logic in getAutomountLocationsFullData and searchAutomountLocationsEntries is almost identical (find IDs then batch-show); consider extracting a shared helper to reduce duplication and keep behavior consistent between the two paths.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `rpcAutomountLocations.ts`, both `getAutomountLocationsFullData` and `searchAutomountLocationsEntries` handle errors from the initial `automountlocation_find` call but ignore possible `showResult.error`; consider checking `showResult.error` and returning it instead of always casting `showResult.data` to avoid silent failures.
- In `DeleteAutomountLocationsModal`, the `ModalWithFormLayout` `formId` (`remove-automount-locations-modal`) does not match the `form` attribute used on the delete button (`delete-automount-locations-modal`), which can break form submission semantics; align these IDs.
- The logic in `getAutomountLocationsFullData` and `searchAutomountLocationsEntries` is almost identical (find IDs then batch-show); consider extracting a shared helper to reduce duplication and keep behavior consistent between the two paths.

## Individual Comments

### Comment 1
<location path="src/services/rpcAutomountLocations.ts" line_range="31-40" />
<code_context>
+interface FindLocationArgs {
</code_context>
<issue_to_address>
**issue (bug_risk):** The `FindLocationArgs` type does not match how `cn` is used, which can hide type issues.

In both `getAutomountLocationsFullData` and `searchAutomountLocationsEntries`, `loc.cn[0]` is used, but `FindLocationArgs` types `cn` as `string`. Please update this to `cn: string[]` (or the correct API shape) so the type matches actual usage and avoids masking mismatches.
</issue_to_address>

### Comment 2
<location path="src/services/rpcAutomountLocations.ts" line_range="71" />
<code_context>
+
+        const findResponse = findResult.data as FindRPCResponse;
+        const locationIds: string[] = [];
+        const totalItemsCount = findResponse.result.result.length as number;
+
+        for (let i = startIdx; i < totalItemsCount && i < stopIdx; i++) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `result.result.length` for `totalItemsCount` may break pagination when `sizelimit` is non-zero.

`totalItemsCount` here reflects only the current page size (`result.result.length`), not the overall match count. If this API follows other FreeIPA `*_find` responses, `result.count` should represent the total matches ignoring `sizelimit`. Use `result.count` rather than `result.length` (and likewise in `searchAutomountLocationsEntries`) so pagination totals remain correct when a sizelimit is applied.

Suggested implementation:

```typescript
        const findResponse = findResult.data as FindRPCResponse;
        const locationIds: string[] = [];
        const totalItemsCount = findResponse.result.count as number;
        const pageItemsCount = findResponse.result.result.length as number;

        for (let i = startIdx; i < pageItemsCount && i < stopIdx; i++) {

```

You mentioned `searchAutomountLocationsEntries` in your comment. To fully implement the suggestion, you should:
1. Locate the analogous logic in `searchAutomountLocationsEntries` where `totalItemsCount` is derived from `findResponse.result.result.length`.
2. Apply the same pattern:
   - Set `totalItemsCount = findResponse.result.count as number;`
   - Use a separate `pageItemsCount = findResponse.result.result.length as number;` (or similar) for bounding any loops that index into `result.result`.
This will keep pagination totals correct (using `count`) while still safely iterating over only the returned page of results.
</issue_to_address>

### Comment 3
<location path="src/services/rpcAutomountLocations.ts" line_range="84-88" />
<code_context>
+          params: [[locId], {}],
+        }));
+
+        const showResult = await fetchWithBQ(
+          getBatchCommand(showCommands, apiVersionUsed)
+        );
+
+        const response = showResult.data as BatchRPCResponse;
+        if (response) {
+          response.result.totalCount = totalItemsCount;
</code_context>
<issue_to_address>
**issue (bug_risk):** The `automountlocation_show` batch call result is cast without checking for errors.

Here `showResult.error` isn’t checked before casting `showResult.data` to `BatchRPCResponse`. If the batch call fails, `showResult.data` may be `undefined`, causing a runtime error when accessing `response.result`. Please handle `showResult.error` (similar to `findResult`) and bail out early before casting in both `getAutomountLocationsFullData` and `searchAutomountLocationsEntries`.
</issue_to_address>

### Comment 4
<location path="src/pages/AutomountLocations/DeleteAutomountLocationsModal.tsx" line_range="134-143" />
<code_context>
+      key="delete-automount-locations"
+      variant="danger"
+      onClick={onDelete}
+      form="delete-automount-locations-modal"
+      spinnerAriaValueText="Deleting"
+      spinnerAriaLabel="Deleting"
+      isLoading={spinning}
+      isDisabled={spinning}
+    >
+      {spinning ? (
+        <>
+          <Spinner size="sm" />
+          {"Deleting"}
+        </>
+      ) : (
+        "Delete"
+      )}
+    </Button>,
+    <Button
+      data-cy="modal-button-cancel"
+      key="cancel-delete-automount-locations"
+      variant="link"
+      onClick={props.onClose}
+    >
+      Cancel
+    </Button>,
+  ];
+
+  return (
+    <>
+      <ModalWithFormLayout
+        dataCy="automount-locations-delete-modal"
+        variantType="medium"
+        modalPosition="top"
+        offPosition="76px"
+        title="Remove automount locations"
+        formId="remove-automount-locations-modal"
+        fields={fields}
+        show={props.isOpen}
</code_context>
<issue_to_address>
**issue (bug_risk):** The delete button’s `form` attribute does not match the modal’s `formId`.

`Button` uses `form="delete-automount-locations-modal"` while `ModalWithFormLayout` uses `formId="remove-automount-locations-modal"`. This mismatch will break the form association. Please update them to use the same ID.
</issue_to_address>

### Comment 5
<location path="src/pages/AutomountLocations/AddAutomountLocationModal.tsx" line_range="54-22" />
<code_context>
+
+    addLocation(payload)
+      .then((response) => {
+        if ("data" in response) {
+          const data = response.data?.result;
+          const error = response.data?.error as SerializedError;
+
+          if (error) {
+            dispatch(
+              addAlert({
+                name: "add-automount-location-error",
+                title: error.message || "Failed to add automount location",
+                variant: "danger",
+              })
+            );
+          }
+
+          if (data) {
</code_context>
<issue_to_address>
**issue (bug_risk):** The add-location handler treats responses with both `data` and `error` as success and ignores top-level errors.

Inside `onAddLocation`, the code only checks for `
</issue_to_address>

### Comment 6
<location path="src/pages/AutomountLocations/AutomountLocations.tsx" line_range="168" />
<code_context>
+    isAutomountLocationSelectable
+  );
+
+  const updateSelectedLocations = (
+    items: AutomountLocation[],
+    isSelected: boolean
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying selection, data derivation, and pagination/error-handling logic to avoid unnecessary cloning, hooks, and small wrapper objects while preserving behavior.

- `updateSelectedLocations`/`setLocationSelected` can be made clearer and avoid deep cloning/nested loops while preserving behavior (no duplicates, toggle by `cn`):

  ```ts
  const updateSelectedLocations = (
    items: AutomountLocation[],
    isSelected: boolean
  ) => {
    setSelectedElements((prev) => {
      const prevByCn = new Map(prev.map((loc) => [loc.cn, loc]));
      if (isSelected) {
        items.forEach((item) => {
          if (!prevByCn.has(item.cn)) {
            prevByCn.set(item.cn, item);
          }
        });
      } else {
        items.forEach((item) => {
          prevByCn.delete(item.cn);
        });
      }
      const next = Array.from(prevByCn.values());
      setIsDeleteButtonDisabled(next.length === 0);
      return next;
    });
  };

  const setLocationSelected = (location: AutomountLocation, isSelecting = true) => {
    if (isAutomountLocationSelectable(location)) {
      updateSelectedLocations([location], isSelecting);
    }
  };
  ```

- `locations` can be computed without `useMemo` and the extra branching (`locationsResponse.data` and `data` are the same object coming from the hook):

  ```ts
  const locations: AutomountLocation[] =
    locationsResponse.isSuccess && locationsResponse.data
      ? locationsResponse.data.result.results.map((item) =>
          apiToAutomountLocation(item.result)
        )
      : [];
  ```

- `totalCount` and `showTableRows` do not need `useMemo`; the expressions are cheap and already depend on reactive values:

  ```ts
  const totalCount =
    locationsResponse.isSuccess && locationsResponse.data
      ? locationsResponse.data.result.totalCount
      : 0;

  const showTableRows =
    !locationsResponse.isFetching && !locationsResponse.isLoading && !isLoading;
  ```

- The “always refetch on mount” effect duplicates RTK Query’s built‑in behavior and adds another side effect to track. If the intent is to ensure a fresh fetch, you can move that into the hook config and remove the effect:

  ```ts
  // Hook usage
  const locationsResponse = useGetAutomountLocationsFullDataQuery(
    { searchValue, apiVersion, sizelimit: 0, startIdx: firstIdx, stopIdx: lastIdx },
    { refetchOnMountOrArgChange: true }
  );

  // Remove:
  // React.useEffect(() => {
  //   locationsResponse.refetch();
  // }, []);
  ```

- The error-handling/navigation effect can be pushed into a small helper to reduce nesting inside the component body:

  ```ts
  const handleAuthError = (
    response: typeof locationsResponse,
    navigate: ReturnType<typeof useNavigate>
  ) => {
    if (!response.isLoading && response.isError && response.error) {
      navigate("/login");
      window.location.reload();
    }
  };

  React.useEffect(() => {
    if (locationsResponse.isFetching) {
      globalErrors.clear();
      return;
    }
    handleAuthError(locationsResponse, navigate);
  }, [locationsResponse, navigate, globalErrors]);
  ```

- The “data wrappers” for pagination/selection/search can be slightly consolidated to reduce the number of tiny objects passed around. For example, pagination and selected-per-page can be grouped without changing call sites much:

  ```ts
  const paginationData = {
    page,
    perPage,
    totalCount,
    selectedPerPage,
    updatePage: setPage,
    updatePerPage: setPerPage,
    updateSelectedPerPage: setSelectedPerPage,
  };
  ```

  Then you can drop `selectedPerPageData` and pass `paginationData` directly where both are currently used.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): The add-location handler treats responses with both data and error as success and ignores top-level errors.

Inside onAddLocation, the code only checks for `

[carma12]

First round of review. Overall looks good, some minor changes and feedback provided (also for Sourcery).

[carma12]

This value should be 100 instead of 0. This is related to the LDAP limitation (i.e. if you specify to retrieve 1000 elements, LDAP only can return around 200), so the way we manage it in modern WebUI is to set the limit to 100.