Indicate that local user auth is disabled#551
Conversation
📝 WalkthroughWalkthroughThe UserStatusCell component now reads account settings to detect when local authentication is disabled for local-IDP users, displays "Disabled" status with gray styling, and shows context-sensitive tooltip content (local-auth-disabled notice or approval guidance) and help icon only when relevant. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/modules/users/table-cells/UserStatusCell.tsx`:
- Around line 17-19: The expression computing isLocalAuthDisabled can throw if
account is present but account.settings is undefined; update the guard to use
optional chaining on settings (e.g., change
account?.settings.local_auth_disabled to account?.settings?.local_auth_disabled)
so the boolean check is safe, and ensure the expression in UserStatusCell
(reference: isLocalAuthDisabled, account, settings, user.idp_id) still compares
to true and combines with user.idp_id === "local"; optionally verify the Account
type/interface to confirm settings is optional and adjust types if needed.
🧹 Nitpick comments (2)
src/modules/users/table-cells/UserStatusCell.tsx (2)
40-85: Tooltip content is computed on every render even when the tooltip is disabled.When
showTooltipisfalse, neither branch of the tooltip content is used, yet both JSX trees are still created. This isn't a performance concern for a small table, but for consistency you could computetooltipContentonly when needed.♻️ Lazy computation
- const tooltipContent = isLocalAuthDisabled ? ( + const tooltipContent = showTooltip ? ( + isLocalAuthDisabled ? ( <div className={"max-w-xs text-xs flex flex-col gap-2"}> ... </div> - ) : ( + ) : ( <div className={"max-w-xs text-xs flex flex-col gap-2"}> ... </div> - ); + ) + ) : null;
14-14:useAccount()is invoked per table row — relies on fetch-layer caching.This hook is called once per
UserStatusCellinstance (i.e., once per user row). It works correctly only if the underlying data-fetching layer (e.g., SWR or React Query) deduplicates the/accountsrequest. This appears to be the case based onuseFetchApi, but worth keeping in mind if this component is rendered in a large list.Consider lifting the
useAccount()call to the parent table component and passingaccount(or justisLocalAuthDisabled) as a prop to each cell. This avoids coupling every cell to the account context and makes the component easier to test.#!/bin/bash # Check how useFetchApi handles caching/deduplication rg -n 'function useFetchApi' -A 20 rg -n 'useFetchApi' -C3 --type=ts
…sabled) so the expression safely evaluates to false instead of throwing if account.settings is undefined.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/users/table-cells/UserStatusCell.tsx (1)
22-30:⚠️ Potential issue | 🟡 Minor
isLocalAuthDisabledoverrides the "Blocked" status — is this intentional?If a local user has been explicitly blocked by an admin (
status === "blocked") and local auth is also disabled, this logic shows "Disabled" instead of "Blocked". This could hide the admin's deliberate block action. Consider checkingstatus === "blocked"first, or combining the information.
Summary by CodeRabbit
Release Notes