OTP tokens > 'Is managed by' subpage

[carma12]

Add the "Is managed by" tab section to the
OTP tokens sub-page, allowing users to view and
manage which users can manage a given OTP token.

Changes:

• Create ManagedByUsers reusable component in src/components/ManagedBy/
• Create OtpTokensManagedBy wrapper with TabLayout and badge count
• Add otptoken_add_managedby / otptoken_remove_managedby RPC mutations
• Update OtpToken.managedby_user type from string to string[]
• Add managedby_user route in AppRoutes.tsx

Assisted-by: Claude noreply@anthropic.com

Summary by Sourcery

Add an "Is managed by" subpage for OTP tokens that lets users view and manage which users can manage a given OTP token, including routing, data loading, and badge count.

New Features:

• Introduce an OTP tokens "Is managed by" top-level tab and managedby_user sub-route for per-token manager management.
• Add a ManagedByUsers reusable component and OtpTokensManagedBy wrapper to list, search, add, and remove users who manage an OTP token.
• Expose new RPC mutations to add and remove managed-by users for OTP tokens.

Enhancements:

• Change the OtpToken.managedby_user field from a single string to a string array to support multiple managers.

[sourcery-ai]

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

• In ManagedByUsers, the useGetUsersInfoByUidQuery already refetches when its uidsList argument changes, so the extra fullUsersQuery.refetch() call in the userNamesToLoad effect is likely redundant and can be removed to avoid unnecessary network calls.
• The effect that builds availableItems in ManagedByUsers only depends on usersQuery.data and usersQuery.isFetching but uses managedby_user from props; consider including managedby_user in the dependency array so the filtered available list stays in sync when the managed-by list changes.
• In OtpTokensTabs, activeTabKey is set directly from section unless it starts with managedby_; if an unexpected section value is passed you may want to default to a known tab key (e.g. "settings") to avoid an invalid Tabs activeKey state.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In ManagedByUsers, the `useGetUsersInfoByUidQuery` already refetches when its `uidsList` argument changes, so the extra `fullUsersQuery.refetch()` call in the `userNamesToLoad` effect is likely redundant and can be removed to avoid unnecessary network calls.
- The effect that builds `availableItems` in ManagedByUsers only depends on `usersQuery.data` and `usersQuery.isFetching` but uses `managedby_user` from props; consider including `managedby_user` in the dependency array so the filtered available list stays in sync when the managed-by list changes.
- In OtpTokensTabs, `activeTabKey` is set directly from `section` unless it starts with `managedby_`; if an unexpected section value is passed you may want to default to a known tab key (e.g. "settings") to avoid an invalid Tabs `activeKey` state.

## Individual Comments

### Comment 1
<location path="src/components/ManagedBy/ManagedByUsers.tsx" line_range="180-189" />
<code_context>
+    };
+
+    setSpinning(true);
+    addManagedBy(payload).then((response) => {
+      if ("data" in response) {
+        if (response.data?.result) {
+          dispatch(
+            addAlert({
+              name: "add-managedby-success",
+              title: "Assigned new managers to OTP token '" + props.id + "'",
+              variant: "success",
+            })
+          );
+          props.onRefreshData();
+          setShowAddModal(false);
+        } else if (response.data?.error) {
+          const errorMessage = response.data.error as unknown as ErrorResult;
+          dispatch(
+            addAlert({
+              name: "add-managedby-error",
+              title: errorMessage.message,
+              variant: "danger",
+            })
+          );
+        }
+      }
+      setSpinning(false);
+    });
+  };
</code_context>
<issue_to_address>
**issue (bug_risk):** The add-managed-by flow ignores mutation errors, which can leave the UI silently failing.

`onAddUser` only checks `response.data` and doesn’t handle the case where the promise resolves with an `error` field (e.g. network/server failures). In that scenario the modal stays open, no alert is shown, and the user gets no feedback. Please add handling for the `error` response (similar to `onDeleteUser`) so failure states are surfaced and modal/spinner state is correctly reset.
</issue_to_address>

### Comment 2
<location path="src/components/ManagedBy/ManagedByUsers.tsx" line_range="83-95" />
<code_context>
+  });
+
+  // Refresh users
+  React.useEffect(() => {
+    const usersNames = getUsersNameToLoad();
+    setUserNamesToLoad(usersNames);
+  }, [props.entity, searchValue, page, perPage]);
+
+  React.useEffect(() => {
+    if (userNamesToLoad.length > 0) {
+      fullUsersQuery.refetch();
+    }
+  }, [userNamesToLoad]);
+
+  // Update users
</code_context>
<issue_to_address>
**suggestion (performance):** The extra `refetch` effect may be redundant with RTK Query’s automatic cache lifecycle.

Because `useGetUsersInfoByUidQuery` already uses `userNamesToLoad` as its query arg, RTK Query will refetch when `uidsList` changes. This extra `fullUsersQuery.refetch()` tied to `userNamesToLoad` can trigger duplicate requests and re-renders. Consider removing this effect unless you have a concrete case where RTK Query’s automatic refetch isn’t sufficient.

```suggestion
  // Refresh users
  React.useEffect(() => {
    const usersNames = getUsersNameToLoad();
    setUserNamesToLoad(usersNames);
  }, [props.entity, searchValue, page, perPage]);

  // Update users
```
</issue_to_address>

### Comment 3
<location path="src/components/ManagedBy/ManagedByUsers.tsx" line_range="55" />
<code_context>
+  // Loaded users based on paging and member attributes
+  const [users, setUsers] = React.useState<User[]>([]);
+
+  const managedby_user = props.entity.managedby_user || [];
+
+  const getUsersNameToLoad = (): string[] => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing manually synchronized state and imperative RTK Query usage with derived values, async/await mutations, and possibly a dedicated hook to streamline this component’s data flow and side effects.

The main complexity comes from manually synchronizing derived state with effects and managing RTK Query imperatively. You can keep all behavior but simplify by relying on derived values and RTK Query’s built‑in behavior.

### 1) Derive `userNamesToLoad` and `users` instead of storing them

Right now `userNamesToLoad` and `users` are in `useState` and kept in sync via `useEffect` + `refetch`. You can derive them from props and query data:

```ts
const managedby_user = props.entity.managedby_user || [];

const userNamesToLoad = React.useMemo(() => {
  let toLoad = [...managedby_user].sort();

  if (searchValue) {
    const q = searchValue.toLowerCase();
    toLoad = toLoad.filter((name) => name.toLowerCase().includes(q));
  }

  return paginate(toLoad, page, perPage);
}, [managedby_user, searchValue, page, perPage]);

const fullUsersQuery = useGetUsersInfoByUidQuery({
  uidsList: userNamesToLoad,
  noMembers: true,
});

// derived list of users for the table
const users = React.useMemo<User[]>(() => {
  return fullUsersQuery.data ?? [];
}, [fullUsersQuery.data]);
```

This lets RTK Query refetch when `userNamesToLoad` changes and removes:

- `userNamesToLoad` state
- `users` state
- `useEffect` for `setUserNamesToLoad`
- `useEffect` calling `fullUsersQuery.refetch`
- `useEffect` copying `fullUsersQuery.data` into `users`

### 2) Let RTK Query manage the “available users” search

You can also drop `availableUsers` / `availableItems` state and the manual `refetch` effect:

```ts
const usersQuery = useGettingActiveUserQuery(
  showAddModal
    ? {
        search: adderSearchValue,
        apiVersion: API_VERSION_BACKUP,
        sizelimit: 100,
        startIdx: 0,
        stopIdx: 100,
      }
    : // when modal closed, avoid unnecessary work
      skipToken
);

const availableUsers = React.useMemo<User[]>(() => {
  if (!usersQuery.data || usersQuery.isFetching) return [];
  const { count, results } = usersQuery.data.result;
  const list: User[] = [];
  for (let i = 0; i < count; i++) {
    list.push(apiToUser(results[i].result));
  }
  return list;
}, [usersQuery.data, usersQuery.isFetching]);

const availableItems: AvailableItems[] = React.useMemo(() => {
  return availableUsers
    .filter((user) => !managedby_user.includes(user.uid))
    .map((user) => ({
      key: user.uid,
      title: user.uid,
    }));
}, [availableUsers, managedby_user]);
```

This removes:

- `availableUsers` / `availableItems` state
- `useEffect` for `usersQuery.refetch`
- `useEffect` that builds `avalUsers`/`items` and calls `setAvailableUsers` / `setAvailableItems`

### 3) Simplify mutations with `async/await` and `unwrap()`

The `.then` chains with `"data" in response` / `"error" in response` are harder to follow. Using `unwrap()` gives you success/error paths directly:

```ts
const onAddUser = async (items: AvailableItems[]) => {
  const newUserNames = items.map((item) => item.key);
  if (!props.id || newUserNames.length === 0) return;

  const payload: OtpTokenManagedByPayload = {
    otpTokenId: props.id,
    users: newUserNames,
  };

  try {
    setSpinning(true);
    const result = await addManagedBy(payload).unwrap();

    if (result?.result) {
      dispatch(
        addAlert({
          name: "add-managedby-success",
          title: `Assigned new managers to OTP token '${props.id}'`,
          variant: "success",
        })
      );
      props.onRefreshData();
      setShowAddModal(false);
    } else if (result?.error) {
      const errorMessage = result.error as unknown as ErrorResult;
      dispatch(
        addAlert({
          name: "add-managedby-error",
          title: errorMessage.message,
          variant: "danger",
        })
      );
    }
  } catch (err) {
    dispatch(
      addAlert({
        name: "add-managedby-error",
        title: "Failed to assign managers",
        variant: "danger",
      })
    );
  } finally {
    setSpinning(false);
  }
};
```

Similarly for `onDeleteUser`:

```ts
const onDeleteUser = async () => {
  if (!props.id) return;

  const payload: OtpTokenManagedByPayload = {
    otpTokenId: props.id,
    users: usersSelected,
  };

  try {
    setSpinning(true);
    await removeManagedBy(payload).unwrap();

    dispatch(
      addAlert({
        name: "remove-managedby-success",
        title: `Removed managers from OTP token '${props.id}'`,
        variant: "success",
      })
    );
    props.onRefreshData();
    setUsersSelected([]);
    setShowDeleteModal(false);
  } catch {
    dispatch(
      addAlert({
        name: "remove-managedby-error",
        title: "Failed to remove managers",
        variant: "danger",
      })
    );
  } finally {
    setSpinning(false);
  }
};
```

This keeps the behavior but reduces branching and type guard noise.

### 4) Optional: extract data‑handling into a hook

If this component continues to grow, consider moving the data/pagination logic into a hook to separate responsibilities:

```ts
function useManagedByUsers(entity: Partial<OtpToken>, searchValue: string, page: number, perPage: number) {
  const managedby_user = entity.managedby_user || [];

  const userNamesToLoad = React.useMemo(
    () => paginate(
      managedby_user
        .filter((uid) => uid.toLowerCase().includes(searchValue.toLowerCase()))
        .sort(),
      page,
      perPage
    ),
    [managedby_user, searchValue, page, perPage]
  );

  const fullUsersQuery = useGetUsersInfoByUidQuery({
    uidsList: userNamesToLoad,
    noMembers: true,
  });

  const users = React.useMemo<User[]>(
    () => fullUsersQuery.data ?? [],
    [fullUsersQuery.data]
  );

  return { managedby_user, userNamesToLoad, users, fullUsersQuery };
}
```

Then the component focuses on rendering, selection, and modal visibility, while the hook owns the data derivation.
</issue_to_address>

### Comment 4
<location path="src/pages/OtpTokens/OtpTokensTabs.tsx" line_range="30" />
<code_context>
   // Data loaded from the API
   const otpTokensSettingsData = useOtpTokensSettingsData(ipatokenuniqueid);

+  // Determine the active top-level tab key
+  const [activeTabKey, setActiveTabKey] = React.useState("settings");
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider deriving the active tab key and route mapping directly from `section` instead of using state/effects and inline conditionals to simplify tab handling logic.

You can simplify this without changing behavior by removing the derived state/effect and making the tab/route mapping explicit.

**1. Derive `activeTabKey` directly from `section`**

Replace the `useState` + `useEffect` with a pure derivation:

```ts
// Determine the active top-level tab key
const activeTabKey =
  section.startsWith("managedby_") ? "managedby" : section;
```

And keep `Tabs` as:

```tsx
<Tabs
  activeKey={activeTabKey}
  onSelect={handleTabClick}
  /* ... */
>
```

This removes the risk of `section` and `activeTabKey` getting out of sync and avoids extra re-renders.

**2. Make navigation mapping explicit instead of branching**

Since the route for `"managedby"` is always `managedby_user`, you can express that as a small mapping function and avoid spreading route logic across conditionals:

```ts
const getSectionForTabKey = (tabKey: string | number) => {
  if (tabKey === "managedby") {
    return "managedby_user";
  }
  return "settings"; // current default; extend if more tabs added
};

const handleTabClick = (
  _event: React.MouseEvent<HTMLElement, MouseEvent>,
  tabIndex: number | string
) => {
  const sectionForTab = getSectionForTabKey(tabIndex);
  navigate(`/${pathname}/${ipatokenuniqueid}${sectionForTab === "settings" ? "" : `/${sectionForTab}`}`);
};
```

This keeps all existing behavior, centralizes the mapping logic, and reduces branching/derived state complexity in the component.
</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.}

[veronnicka]

Hi, please look into the sourcery comments, I find them quite relevant

[carma12]

@veronnicka - I have amended the comments from Sourcery and the ones you mentioned. You can proceed with the review, TY!

[veronnicka]

Hi, I ran the code, the search on the second page not working. Is this fixed in #1139? If not, please add the fix.

Apart from this, the page works as expected.

[carma12]

Hi, I ran the code, the search on the second page not working. Is this fixed in #1139? If not, please add the fix.

Apart from this, the page works as expected.

As this PR is older than #1139 most likely doesn't include the search-related fixes. So will fix them here, thanks for noticing.

[carma12]

@veronnicka - I amended the changes based on your feedback. Also added some notes in one of the sub-pages files to refer to those changes as a standard in case this will happen again when implementing new subpages. Please feel free to review...

[veronnicka]

The page is working as expected, the search is fixed. Good job, we can merge.

[veronnicka]

great! I feel like we are always correcting this haha

[duzda]

Note for myself, refactor the search here too