Add admin block/unblock functionality to user profiles #302
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: kishor-gupta <61230727+kishor-gupta@users.noreply.github.com>
- Created BlockUserModal and UnblockUserModal components - Added ViewUserProfileContainer for viewing any user's profile (admin only) - Updated ProfileView to support block/unblock buttons and blocked state - Added route for /account/profile/:userId - Integrated block/unblock mutations in profile view - Added blocked user warning banner and grayed-out UI state - Wired admin dashboard to navigate to user profiles Co-authored-by: kishor-gupta <61230727+kishor-gupta@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Implement block and unblock functionality for admin users
Add admin block/unblock functionality to user profiles
Nov 25, 2025
…pilot/add-block-unblock-user-functionality
…for improved clarity and consistency
…pilot/add-block-unblock-user-functionality
…documentation - Introduced new security requirements documents covering: - Listing Appeal Review (0004) - Content Auto Review (0005) - PII Data Restrictions (0006) - Admin Access Control (0007) - Blocked User Restrictions (0008) - Reservation Mutual Consent (0009) - Transport Encryption (0010) - Input Validation (0011) feat(threat-assessments): Implement threat assessment documentation - Added threat assessment documents for: - CodeQL (0001) - Edgescan (0002) - GitHub Dependabot (0003) - Microsoft Defender (0004) - Secret Scanning (0005) - SonarSource Cloud (0006) - Created category index files for security requirements and threat assessments.
…tainer with block/unblock functionality
…th block/unblock functionality
Contributor
|
@sourcery-ai review |
Contributor
Reviewer's GuideImplements admin-facing user block/unblock controls across the UI by introducing shared user modals, enhancing profile views with permission-aware actions and blocked-state presentation, wiring GraphQL mutations and routing for viewing arbitrary user profiles, and tightening loading/permission handling in admin user management flows. Sequence diagram for admin blocking a user from the profile pagesequenceDiagram
actor Admin
participant Router as Router
participant UserProfile as UserProfile_page
participant ViewUserProfileContainer as ViewUserProfileContainer
participant ProfileView as ProfileView
participant BlockUserModal as BlockUserModal
participant Apollo as Apollo_Client
participant Backend as GraphQL_API
Admin->>Router: navigate /account/profile/:userId
Router->>UserProfile: render
UserProfile->>ViewUserProfileContainer: mount
ViewUserProfileContainer->>Apollo: HomeAccountViewUserProfileCurrentUser
Apollo-->>ViewUserProfileContainer: currentUser
ViewUserProfileContainer->>Apollo: HomeAccountViewUserProfileUserById(userId)
Apollo-->>ViewUserProfileContainer: userById
ViewUserProfileContainer->>ProfileView: render(user, listings, permissions, blocking)
Admin->>ProfileView: click Block_User button
ProfileView->>ViewUserProfileContainer: blocking.handleOpenBlockModal()
ViewUserProfileContainer->>BlockUserModal: visible true
Admin->>BlockUserModal: fill reason,description and confirm
BlockUserModal->>ViewUserProfileContainer: onConfirm(BlockUserFormValues)
ViewUserProfileContainer->>Apollo: HomeAccountViewUserProfileBlockUser(userId)
Apollo->>Backend: blockUser(userId)
Backend-->>Apollo: updated_user(isBlocked true)
Apollo-->>ViewUserProfileContainer: mutation result
ViewUserProfileContainer->>Apollo: refetch HomeAccountViewUserProfileUserById(userId)
Apollo-->>ViewUserProfileContainer: refreshed userById
ViewUserProfileContainer->>ProfileView: re_render(with isBlocked true)
ViewUserProfileContainer->>BlockUserModal: close
ViewUserProfileContainer->>Admin: show success_message
Class diagram for shared user block/unblock modals and profile view componentsclassDiagram
class BlockUserModal {
+boolean visible
+string userName
+BlockUserFormValues onConfirm(values)
+void onCancel()
+boolean loading
}
class UnblockUserModal {
+boolean visible
+string userName
+string blockReason
+void onConfirm()
+void onCancel()
+boolean loading
}
class BlockUserFormValues {
+string reason
+string description
}
class ProfilePermissions {
+boolean isBlocked
+boolean isAdminViewer
+boolean canBlockUser
}
class BlockingController {
+boolean blockModalVisible
+boolean unblockModalVisible
+void handleOpenBlockModal()
+void handleOpenUnblockModal()
+void handleConfirmBlockUser(values)
+void handleConfirmUnblockUser()
+void closeBlockModal()
+void closeUnblockModal()
}
class ProfileUser {
+string id
+string firstName
+string lastName
+string username
+string email
+string accountType
+string createdAt
+string city
+string state
}
class ProfileViewProps {
+ProfileUser user
+ItemListing[] listings
+boolean isOwnProfile
+ProfilePermissions permissions
+function onEditSettings(listingId)
+function onListingClick(listingId)
+BlockingController blocking
+boolean blockUserLoading
+boolean unblockUserLoading
}
class ProfileView {
+ProfileViewProps props
+string formatDate(dateString)
+string getDisplayName(profileUser)
+render()
}
class ProfileActionsProps {
+boolean isOwnProfile
+boolean isBlocked
+boolean canBlockUser
+void onEditSettings()
+void onBlockUser()
+void onUnblockUser()
+string variant
}
class ProfileActions {
+ProfileActionsProps props
+render()
}
class ViewUserProfileContainer {
+void handleEditSettings()
+void handleListingClick(listingId)
+Promise handleBlockUser(values)
+Promise handleUnblockUser()
+render()
}
class AdminUsersTable {
+AdminUserData[] data
+boolean loading
+void onSearch(value)
+void onStatusFilter(values)
+void onTableChange(pagination,filters,sorter)
+void onPageChange(page)
+void onAction(action,userId)
+void handleBlockUser(user)
+void handleUnblockUser(user)
}
class AdminUsersTableContainer {
+number currentPage
+void onPageChange(page)
+void handleSearch(value)
+void handleStatusFilter(values)
+void handleTableChange(pagination,filters,sorter)
+Promise handleBlockUser(userId)
+Promise handleUnblockUser(userId)
+void handleViewProfile(userId)
+void handleViewReport(userId)
+Promise handleAction(action,userId)
}
BlockUserModal --> BlockUserFormValues
UnblockUserModal ..> BlockUserFormValues : displays_reason_description
ProfileView --> ProfileActions : uses
ProfileView o--> ProfilePermissions : permissions
ProfileView o--> BlockingController : blocking
ProfileView o--> BlockUserModal : renders
ProfileView o--> UnblockUserModal : renders
ViewUserProfileContainer o--> ProfileView : renders
ViewUserProfileContainer o--> BlockingController : useProfileBlocking
ViewUserProfileContainer ..> BlockUserModal : passes_onConfirm
ViewUserProfileContainer ..> UnblockUserModal : passes_onConfirm
AdminUsersTableContainer o--> AdminUsersTable : renders
AdminUsersTable o--> BlockUserModal : renders
AdminUsersTable o--> UnblockUserModal : renders
AdminUsersTable ..> BlockUserFormValues : onConfirm(values)
Flow diagram for navigation from admin users table to user profile and block controlsflowchart LR
Admin((Admin_user))
AdminUsersTable["AdminUsersTable
(ant_table_grid)"]
AdminUsersTableContainer["AdminUsersTableContainer
(queries_and_mutations)"]
Router["ReactRouter
/account routes"]
UserProfilePage["UserProfile
/account/profile/:userId"]
ViewUserProfileContainer["ViewUserProfileContainer
(fetch_profile_and_permissions)"]
ProfileView["ProfileView
(header_listings_actions)"]
BlockUserModal["BlockUserModal
(shared_modal)"]
UnblockUserModal["UnblockUserModal
(shared_modal)"]
GraphQLAPI["GraphQL_API
blockUser/unblockUser"]
Admin --> AdminUsersTable
AdminUsersTable --> AdminUsersTableContainer
AdminUsersTableContainer -->|onAction view-profile| Router
Router -->|navigate /account/profile/:userId| UserProfilePage
UserProfilePage --> ViewUserProfileContainer
ViewUserProfileContainer -->|HomeAccountViewUserProfileCurrentUser
HomeAccountViewUserProfileUserById| GraphQLAPI
GraphQLAPI --> ViewUserProfileContainer
ViewUserProfileContainer --> ProfileView
ProfileView -->|open block modal| BlockUserModal
ProfileView -->|open unblock modal| UnblockUserModal
BlockUserModal -->|confirm
HomeAccountViewUserProfileBlockUser| GraphQLAPI
UnblockUserModal -->|confirm
HomeAccountViewUserProfileUnblockUser| GraphQLAPI
GraphQLAPI --> ViewUserProfileContainer
ViewUserProfileContainer -->|refetch userById
update permissions| ProfileView
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The new BlockUserModal maintains its own Form state but the only value passed back to callers is the
reasonstring from local state, which is never updated from the form fields; either bind the Select/TextArea toreasonor remove the localreasonstate and expose/validate the form values viaonConfirmso callers get the actual block data. - In ViewUserProfileContainer, you’re calling
navigateandmessage.errordirectly in the render path for several branches (!userId, blocked profile without permission), which can cause side effects during render; move these intouseEffecthooks keyed on the relevant conditions. - The early
if (!viewedUser || !currentUser) { return null; }in ViewUserProfileContainer bypasses the ComponentQueryLoader entirely on initial renders, resulting in a blank screen while queries load or error; consider relying on ComponentQueryLoader’sloading/errorhandling instead of short-circuiting before it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new BlockUserModal maintains its own Form state but the only value passed back to callers is the `reason` string from local state, which is never updated from the form fields; either bind the Select/TextArea to `reason` or remove the local `reason` state and expose/validate the form values via `onConfirm` so callers get the actual block data.
- In ViewUserProfileContainer, you’re calling `navigate` and `message.error` directly in the render path for several branches (`!userId`, blocked profile without permission), which can cause side effects during render; move these into `useEffect` hooks keyed on the relevant conditions.
- The early `if (!viewedUser || !currentUser) { return null; }` in ViewUserProfileContainer bypasses the ComponentQueryLoader entirely on initial renders, resulting in a blank screen while queries load or error; consider relying on ComponentQueryLoader’s `loading`/`error` handling instead of short-circuiting before it.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.tsx:63-71` </location>
<code_context>
+ setUnblockModalVisible(true);
+ };
- const handleBlockConfirm = async () => {
- try {
- const values = await blockForm.validateFields();
- console.log("Block user with:", values);
- // Mutation is handled by the container via onAction
- onAction("block", selectedUser?.id ?? "");
- setBlockModalVisible(false);
- blockForm.resetFields();
- } catch (error) {
- console.error("Block validation failed:", error);
- }
</code_context>
<issue_to_address>
**issue (bug_risk):** Block confirm handler no longer matches the new modal API and uses a detached form instance.
`BlockUserModal`’s `onConfirm` is typed to receive a `reason: string`, but `handleBlockConfirm` takes no args and instead uses `blockForm.validateFields()`. Since `blockForm` is not the same form the modal creates with `Form.useForm`, that validation path can never work correctly and will likely cause a prop type mismatch and confusing runtime behavior. Either let the modal own form/validation and pass the validated values into `onConfirm`, or pass a shared `Form` instance into the modal and validate there before calling `onConfirm`. In both designs, `handleBlockConfirm` should accept the modal’s validated data instead of using its own `blockForm`.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.tsx:269-274` </location>
<code_context>
- rows={4}
- placeholder="This message will be shown to the user"
+ {/* Block User Modal */}
+ <BlockUserModal
+ visible={blockModalVisible}
+ userName={`${selectedUser?.firstName} ${selectedUser?.lastName}`}
+ onConfirm={handleBlockConfirm}
+ onCancel={() => setBlockModalVisible(false)}
+ loading={blockLoading}
/>
- </Form.Item>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** BlockUserModal is invoked without consuming or propagating the form data it collects.
The modal collects `reason` and `description`, but `onConfirm={handleBlockConfirm}` doesn’t receive or use them (it only calls `onAction("block", id)`). As a result, any block metadata entered is discarded. Either:
- wire the form values through: e.g. make `onConfirm(values)` include `reason`/`description` and extend the mutation to send them, or
- if these fields aren’t used server-side, remove them from the modal to avoid misleading admins into thinking this data is stored.
Suggested implementation:
```typescript
{/* Block User Modal */}
<BlockUserModal
visible={blockModalVisible}
userName={`${selectedUser?.firstName} ${selectedUser?.lastName}`}
onConfirm={(values) => handleBlockConfirm(values)}
onCancel={() => setBlockModalVisible(false)}
loading={blockLoading}
/>
```
To fully implement the suggestion and avoid discarding the form data collected in `BlockUserModal`, you should also:
1. Update the `handleBlockConfirm` signature in this file to accept the modal form values (e.g. `{ reason, description }`):
- Change something like:
```ts
const handleBlockConfirm = async () => {
await onAction("block", selectedUser?.id);
...
}
```
to:
```ts
type BlockUserFormValues = {
reason: string;
description?: string;
};
const handleBlockConfirm = async (values: BlockUserFormValues) => {
const { reason, description } = values;
await onAction("block", selectedUser?.id, { reason, description });
...
}
```
2. Extend the `onAction` function (and its type signature) to accept an optional payload/metadata argument, and ensure the underlying block-user mutation sends `reason` and `description` to the server. For example:
```ts
const onAction = async (
action: "block" | "unblock" | ...,
userId: string,
metadata?: { reason?: string; description?: string }
) => {
// include metadata in the request body for "block"
};
```
3. Ensure that `BlockUserModal` actually calls its `onConfirm` prop with the collected form values (e.g. `form.validateFields().then(values => onConfirm(values))`). If it currently calls `onConfirm()` without arguments, adjust it accordingly.
If you decide *not* to use `reason`/`description` server-side, instead remove those fields from `BlockUserModal` (and any types referring to them) so admins are not misled into thinking that data is persisted.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/shared/user-modals/block-user-modal.tsx:30-39` </location>
<code_context>
+// { label: "Indefinite", value: "indefinite" },
+// ];
+
+export const BlockUserModal: React.FC<Readonly<BlockUserModalProps>> = ({
+ visible,
+ userName,
+ onConfirm,
+ onCancel,
+ loading = false,
+}) => {
+ const [reason, setReason] = useState('');
+ const [blockForm] = Form.useForm();
+
+
+ const handleOk = () => {
+ onConfirm(reason);
+ setReason(''); // Reset after confirm
</code_context>
<issue_to_address>
**issue (bug_risk):** Modal keeps separate `reason` state that is never synchronized with the form fields.
`handleOk` always calls `onConfirm(reason)`, but `reason` is never updated from the `Form` fields, and `blockForm` is created but not used (no `validateFields` / value reads). This means callers always receive an empty string even when the user fills out the form. Please either read the reason from `blockForm.validateFields()` when confirming, or drop `Form` and control the inputs directly via `reason` state (and any other needed fields).
</issue_to_address>
### Comment 4
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx:66-68` </location>
<code_context>
+ );
+
+ // Check if userId is missing after all hooks
+ if (!userId) {
+ navigate("/account/profile");
+ return null;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Navigation is being performed directly during render when `userId` is missing.
Triggering `navigate` in the render path (`if (!userId) { navigate(...); return null; }`) introduces a side effect during render, which React discourages and can lead to warnings or double navigation in strict mode. Prefer moving this redirect into a `useEffect` that depends on `userId`, e.g.
```ts
useEffect(() => {
if (!userId) {
navigate('/account/profile');
}
}, [userId, navigate]);
```
and then render `null` while the effect runs. The same adjustment applies to the blocked-profile redirect below.
</issue_to_address>
### Comment 5
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx:102-68` </location>
<code_context>
+ currentUser?.role?.permissions?.userPermissions?.canViewAllUsers;
+
+ // Blocked users can only be viewed by admins
+ if (viewedUser?.isBlocked && !canViewProfile) {
+ message.error("This user profile is not available");
+ navigate("/home");
+ return null;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Redirect and toast for blocked profiles also run as render-time side effects.
Move this logic into a `useEffect` so the toast and navigation only run in response to state changes, not on every render. For example:
```ts
useEffect(() => {
if (viewedUser?.isBlocked && !canViewProfile) {
message.error('This user profile is not available');
navigate('/home');
}
}, [viewedUser?.isBlocked, canViewProfile, navigate]);
```
Then in render you can simply `return null` when the blocked-profile condition is met.
Suggested implementation:
```typescript
const isAdmin =
currentUser?.__typename === "AdminUser" &&
currentUser?.role?.permissions?.userPermissions?.canBlockUsers;
// Check if current user can view this profile
const canViewProfile =
currentUser?.__typename === "AdminUser" &&
currentUser?.role?.permissions?.userPermissions?.canViewAllUsers;
// Blocked users can only be viewed by admins
useEffect(() => {
if (viewedUser?.isBlocked && !canViewProfile) {
message.error("This user profile is not available");
navigate("/home");
}
}, [viewedUser?.isBlocked, canViewProfile, navigate]);
if (viewedUser?.isBlocked && !canViewProfile) {
return null;
}
```
1. Ensure `useEffect` is imported from React at the top of this file, alongside `useState`, e.g.:
- `import React, { useState, useEffect } from "react";`
2. This edit assumes `viewedUser` is already defined (likely derived from your user query) *before* this block. If it is not, move this `useEffect` and the `if (viewedUser?.isBlocked && !canViewProfile)` guard to a point in the component where `viewedUser` is in scope and initialized.
3. Remove any other remaining instances of `message.error` / `navigate("/home")` that might still be running directly during render for this blocked-profile case, to avoid duplicate side effects.
</issue_to_address>
### Comment 6
<location> `apps/ui-sharethrift/src/components/shared/user-modals/unblock-user-modal.tsx:47-56` </location>
<code_context>
+ Unblocking this user will restore their access to the platform and allow
+ them to interact with other users again.
+ </Paragraph>
+ {blockReason && (
+ <>
+ <div
+ style={{
+ marginTop: 16,
+ padding: 12,
+ background: '#f5f5f5',
+ borderRadius: 4,
+ }}
+ >
+ <Text strong>Original block reason:</Text>
+ <Paragraph style={{ marginTop: 8, marginBottom: 0 }}>
+ {blockReason}
+ </Paragraph>
</code_context>
<issue_to_address>
**suggestion:** Block reason is duplicated for both "reason" and "description" sections.
The modal currently shows two sections, "Original block reason:" and "Original block Description:", but both render the same `blockReason` value. If there’s only one field, consider removing the second section to avoid redundancy. If you do need both a reason and a separate description, introduce a distinct prop (e.g. `blockDescription`) and bind that to the description section instead of reusing `blockReason`.
Suggested implementation:
```typescript
<Paragraph type="secondary" style={{ fontSize: '14px' }}>
Unblocking this user will restore their access to the platform and allow
them to interact with other users again.
</Paragraph>
{blockReason && (
```
From your comment, it sounds like elsewhere in this component you also have two sections labeled something like "Original block reason:" and "Original block Description:" that both render `blockReason`. To fully implement your suggestion, you should either:
1. Remove the "Original block Description:" section entirely if you only have a single `blockReason` field; or
2. Introduce a distinct prop (e.g. `blockDescription?: string`) in the component’s props/interface and bind that to the description section instead of reusing `blockReason`.
Since that exact JSX isn’t visible in the provided snippet, you’ll need to adjust the relevant section manually following this guidance.
</issue_to_address>
### Comment 7
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/profile-view.tsx:31` </location>
<code_context>
// ...interfaces now imported from profile-view.types.ts
interface ProfileViewProps {
- user: ProfileUser;
- listings: ItemListing[];
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the admin actions into a reusable component and passing modals via a single admin slot to keep `ProfileView` focused on layout and reduce branching/props.
You can keep all the new functionality but significantly reduce complexity by extracting the admin actions and modal wiring into small, focused components/slots. This will shrink the prop surface of `ProfileView` and localize branching.
### 1. Group admin-related behavior into a small `ProfileActions` component
Right now the block/unblock vs settings logic is duplicated (mobile/desktop) and spread across the header. Instead, move that into a dedicated component that takes a minimal set of props, and reuse it in both places.
**Before (inside `ProfileView`)**
```tsx
{isOwnProfile ? (
<div className="profile-settings-mobile">
<Button
type="primary"
icon={<SettingOutlined />}
onClick={onEditSettings}
>
Account Settings
</Button>
</div>
) : (
canBlockUser && (
<div className="profile-settings-mobile">
{isBlocked ? (
<Button
type="primary"
icon={<CheckCircleOutlined />}
onClick={onUnblockUser}
style={{
background: '#52c41a',
borderColor: '#52c41a',
}}
>
Unblock User
</Button>
) : (
<Button danger icon={<StopOutlined />} onClick={onBlockUser}>
Block User
</Button>
)}
</div>
)
)}
```
…and similar logic again in the desktop section.
**After: extract `ProfileActions` and reuse**
```tsx
interface ProfileActionsProps {
isOwnProfile: boolean;
isBlocked: boolean;
canBlockUser: boolean;
onEditSettings: () => void;
onBlockUser?: () => void;
onUnblockUser?: () => void;
variant: 'mobile' | 'desktop';
}
const ProfileActions: React.FC<ProfileActionsProps> = ({
isOwnProfile,
isBlocked,
canBlockUser,
onEditSettings,
onBlockUser,
onUnblockUser,
variant,
}) => {
if (isOwnProfile) {
return (
<div className={variant === 'mobile' ? 'profile-settings-mobile' : 'profile-settings-desktop'}>
<Button type="primary" icon={<SettingOutlined />} onClick={onEditSettings}>
Account Settings
</Button>
</div>
);
}
if (!canBlockUser) return null;
const WrapperClass =
variant === 'mobile' ? 'profile-settings-mobile' : 'profile-settings-desktop';
return (
<div className={WrapperClass}>
{isBlocked ? (
<Button
type="primary"
icon={<CheckCircleOutlined />}
onClick={onUnblockUser}
style={{ background: '#52c41a', borderColor: '#52c41a' }}
>
Unblock User
</Button>
) : (
<Button danger icon={<StopOutlined />} onClick={onBlockUser}>
Block User
</Button>
)}
</div>
);
};
```
Then in `ProfileView`:
```tsx
// Mobile
<ProfileActions
variant="mobile"
isOwnProfile={isOwnProfile}
isBlocked={isBlocked}
canBlockUser={canBlockUser}
onEditSettings={onEditSettings}
onBlockUser={onBlockUser}
onUnblockUser={onUnblockUser}
/>
// Desktop
<div className="profile-settings-desktop">
<ProfileActions
variant="desktop"
isOwnProfile={isOwnProfile}
isBlocked={isBlocked}
canBlockUser={canBlockUser}
onEditSettings={onEditSettings}
onBlockUser={onBlockUser}
onUnblockUser={onUnblockUser}
/>
</div>
```
This:
- Removes duplicated branching.
- Keeps `ProfileView` mostly layout-focused.
- Makes it easier to adjust admin behavior in a single place.
### 2. Collapse modal-related props into a single `adminControls` prop
Instead of `blockModalVisible`, `unblockModalVisible`, `onBlockModalCancel`, etc. on `ProfileView`, accept a single optional slot for admin controls (which can include modals). This makes `ProfileView` reusable without knowing about the modal contract.
**Before (props)**
```ts
interface ProfileViewProps {
user: ProfileUser;
listings: ItemListing[];
isOwnProfile: boolean;
isBlocked?: boolean;
isAdmin?: boolean;
canBlockUser?: boolean;
onEditSettings: () => void;
onListingClick: (listingId: string) => void;
onBlockUser?: () => void;
onUnblockUser?: () => void;
blockModalVisible?: boolean;
unblockModalVisible?: boolean;
onBlockModalCancel?: () => void;
onUnblockModalCancel?: () => void;
onBlockModalConfirm?: (reason: string) => void;
onUnblockModalConfirm?: () => void;
blockLoading?: boolean;
unblockLoading?: boolean;
}
```
**After: keep core props + one `adminControls` slot**
```ts
interface ProfileViewProps {
user: ProfileUser;
listings: ItemListing[];
isOwnProfile: boolean;
isBlocked?: boolean;
isAdmin?: boolean;
canBlockUser?: boolean;
onEditSettings: () => void;
onListingClick: (listingId: string) => void;
onBlockUser?: () => void;
onUnblockUser?: () => void;
adminControls?: React.ReactNode;
}
```
Inside `ProfileView`, render the slot where you currently render the modals:
```tsx
{/* Block/Unblock Modals or other admin controls provided by parent */}
{adminControls}
```
Then, in your container (`view-user-profile.container.tsx`), you can own all modal lifecycle and pass them via `adminControls`:
```tsx
<ProfileView
user={user}
listings={listings}
isOwnProfile={isOwnProfile}
isBlocked={isBlocked}
isAdmin={isAdmin}
canBlockUser={canBlockUser}
onEditSettings={handleEditSettings}
onListingClick={handleListingClick}
onBlockUser={openBlockModal}
onUnblockUser={openUnblockModal}
adminControls={
canBlockUser && (
<>
<BlockUserModal
visible={blockModalVisible}
userName={`${user.firstName} ${user.lastName}`}
onConfirm={onBlockModalConfirm}
onCancel={onBlockModalCancel}
loading={blockLoading}
/>
<UnblockUserModal
visible={unblockModalVisible}
userName={`${user.firstName} ${user.lastName}`}
onConfirm={onUnblockModalConfirm}
onCancel={onUnblockModalCancel}
loading={unblockLoading}
/>
</>
)
}
/>
```
This keeps all current behavior but:
- Removes modal lifecycle concerns from `ProfileView`.
- Reduces the prop explosion.
- Makes `ProfileView` usable in non-admin contexts without extra noise.
### 3. Minor: centralize blocked styling condition
To avoid scattering `isBlocked && isAdmin` across the component later, you can encapsulate the header style calculation:
```tsx
const headerStyle = isBlocked && isAdmin
? { opacity: 0.7, filter: 'grayscale(50%)' as const }
: {};
<Card className="mb-6 profile-header" style={headerStyle}>
...
</Card>
```
This keeps the layout cleaner and makes any future blocked visual changes localized.
---
All of the above preserves your new admin/blocking functionality while making `ProfileView` easier to read, test, and reuse by isolating admin branching and modal orchestration into focused components/slots.
</issue_to_address>
### Comment 8
<location> `apps/ui-sharethrift/src/components/shared/user-modals/block-user-modal.tsx:37` </location>
<code_context>
+ onCancel,
+ loading = false,
+}) => {
+ const [reason, setReason] = useState('');
+ const [blockForm] = Form.useForm();
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the modal by using the form as the single source of truth for `reason` and removing unused/commented-out code to reduce noise and bugs.
The reviewer’s concerns are valid: there’s both a correctness bug and avoidable complexity. You can simplify by making the form the single source of truth and removing unused state.
### 1. Remove redundant local state and use the form as source of truth
You don’t need `reason` in `useState` at all; the value already lives in the form. Use `blockForm.validateFields()` in `handleOk` and pass the reason to `onConfirm`.
```tsx
export const BlockUserModal: React.FC<Readonly<BlockUserModalProps>> = ({
visible,
userName,
onConfirm,
onCancel,
loading = false,
}) => {
const [blockForm] = Form.useForm();
const handleOk = async () => {
try {
const { reason } = await blockForm.validateFields();
onConfirm(reason); // keep existing contract
blockForm.resetFields(); // reset after confirm
} catch {
// validation failed, let antd show errors
}
};
const handleCancel = () => {
blockForm.resetFields();
onCancel();
};
return (
<Modal
/* ... */
>
<Form form={blockForm} layout="vertical">
{/* existing Form.Item fields */}
</Form>
</Modal>
);
};
```
And remove the unused state:
```tsx
- const [reason, setReason] = useState('');
```
### 2. Remove or isolate commented-out duration complexity
If block duration isn’t needed yet, strip it from this component to avoid noise. You can keep the constant near where it will be implemented later (e.g., in a separate file or as a TODO comment), but don’t leave large commented blocks in the component:
```tsx
- // const BLOCK_DURATIONS = [
- // { label: "7 Days", value: "7" },
- // { label: "30 Days", value: "30" },
- // { label: "Indefinite", value: "indefinite" },
- // ];
- {/* <Form.Item
- name="duration"
- label="Block Duration"
- rules={[{ required: true, message: "Please select a duration" }]}
- >
- <Select placeholder="Select duration">
- {BLOCK_DURATIONS.map((duration) => (
- <Select.Option key={duration.value} value={duration.value}>
- {duration.label}
- </Select.Option>
- ))}
- </Select>
- </Form.Item> */}
```
If you do plan to use duration soon, consider a small TODO instead:
```tsx
// TODO: add optional block duration when backend supports it.
```
These changes keep the current public API (`onConfirm(reason: string)`) intact, fix the bug where an empty string is always passed, and reduce complexity by having a single source of truth (the form) and no dead/commented code.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...ents/layouts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.tsx
Outdated
Show resolved
Hide resolved
...ents/layouts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.tsx
Outdated
Show resolved
Hide resolved
apps/ui-sharethrift/src/components/shared/user-modals/block-user-modal.tsx
Show resolved
Hide resolved
...hrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx
Outdated
Show resolved
Hide resolved
...hrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx
Outdated
Show resolved
Hide resolved
apps/ui-sharethrift/src/components/shared/user-modals/unblock-user-modal.tsx
Outdated
Show resolved
Hide resolved
apps/ui-sharethrift/src/components/layouts/home/account/profile/components/profile-view.tsx
Show resolved
Hide resolved
apps/ui-sharethrift/src/components/shared/user-modals/block-user-modal.tsx
Outdated
Show resolved
Hide resolved
…d description fields
…omponents to include reason and description fields
…kUserFormValues for improved type safety
…ic description for original block reason
…ser blocking and unblocking functionality
… in profile actions component
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
ViewUserProfileContaineryou fetchrole.permissions.userPermissions.canViewAllUsersandcanBlockUsersbut only gate behavior onuserIsAdmin; consider actually using these permission flags (e.g., to restrict viewing other users/blocked profiles and to conditionally enable block/unblock) rather than relying solely onuserIsAdmin. - The blocked-profile redirect logic in
ViewUserProfileContainercurrently checks onlyviewedUser?.isBlocked && !isAdminViewer; if the product requirement is 'blocked profiles viewable only by admins with canViewAllUsers', you may want to incorporatecanViewAllUsersinto that check so admins lacking that permission are also redirected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ViewUserProfileContainer` you fetch `role.permissions.userPermissions.canViewAllUsers` and `canBlockUsers` but only gate behavior on `userIsAdmin`; consider actually using these permission flags (e.g., to restrict viewing other users/blocked profiles and to conditionally enable block/unblock) rather than relying solely on `userIsAdmin`.
- The blocked-profile redirect logic in `ViewUserProfileContainer` currently checks only `viewedUser?.isBlocked && !isAdminViewer`; if the product requirement is 'blocked profiles viewable only by admins with canViewAllUsers', you may want to incorporate `canViewAllUsers` into that check so admins lacking that permission are also redirected.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx:106-108` </location>
<code_context>
+ unblockUser({ variables: { userId: userId! } });
+ };
+
+ const canBlockUsers =
+ currentUser?.__typename === "AdminUser" &&
+ currentUser?.userIsAdmin;
+
+ const isOwnProfile = currentUser?.id === viewedUser?.id;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `canBlockUsers` flag ignores the `canBlockUsers` permission fetched in the GraphQL query.
`HomeAccountViewUserProfileCurrentUser` already fetches `role.permissions.userPermissions.canBlockUsers`, but this flag isn’t used here. As written, any admin (regardless of permission) will see block/unblock controls. Please either incorporate the permission into `canBlockUsers`, e.g.
```ts
const canBlockUsers =
currentUser?.__typename === "AdminUser" &&
currentUser.userIsAdmin &&
currentUser.role?.permissions?.userPermissions?.canBlockUsers;
```
or remove `canBlockUsers` from the query if it’s intentionally unused.
```suggestion
const canBlockUsers =
currentUser?.__typename === "AdminUser" &&
currentUser.userIsAdmin &&
currentUser.role?.permissions?.userPermissions?.canBlockUsers;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
ViewUserProfileContainerand related permission logic you treatuserIsAdminas the sole gate for viewing and blocking users; if the backend is exposing fine-grainedcanViewAllUsers/canBlockUsersflags (as suggested by your mocks), consider checking those explicitly rather than inferring fromuserIsAdminso the UI matches the domain permissions model. - The
getDisplayNamehelper is implemented both inAdminUsersTableand inViewUserProfileContainer; extracting this into a shared utility (or a small shared component) would reduce duplication and keep user display-name formatting consistent across admin surfaces.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ViewUserProfileContainer` and related permission logic you treat `userIsAdmin` as the sole gate for viewing and blocking users; if the backend is exposing fine-grained `canViewAllUsers`/`canBlockUsers` flags (as suggested by your mocks), consider checking those explicitly rather than inferring from `userIsAdmin` so the UI matches the domain permissions model.
- The `getDisplayName` helper is implemented both in `AdminUsersTable` and in `ViewUserProfileContainer`; extracting this into a shared utility (or a small shared component) would reduce duplication and keep user display-name formatting consistent across admin surfaces.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.tsx:233-242` </location>
<code_context>
+ const getDisplayName = (user?: AdminUserData | null) => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid surfacing the "N/A" sentinel as a username in the block/unblock modal.
In `AdminUsersTableContainer`, `username` is defaulted to the literal string `"N/A"` when missing. Unlike first/last name, `getDisplayName` doesn’t filter this, so the modal can show `"N/A"` as the username. Please treat `"N/A"` as empty for `user.username` as well (e.g., `user.username && user.username !== "N/A"`), and fall back to a neutral label like `"Listing User"`.
Suggested implementation:
```typescript
const getDisplayName = (user?: AdminUserData | null) => {
if (!user) return "";
const nameParts = [user.firstName, user.lastName].filter(
(p) => Boolean(p) && p !== "N/A",
) as string[];
if (nameParts.length > 0) {
return nameParts.join(" ");
}
const hasUsername =
user.username && user.username !== "N/A";
if (hasUsername) {
return user.username as string;
}
return "Listing User";
```
If other parts of this file or related components assume that `getDisplayName` can return an empty string when no name is available, you may want to update those call sites (e.g., tests or UI expectations) to account for the new `"Listing User"` fallback label.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx:106-108` </location>
<code_context>
+ unblockUser({ variables: { userId: userId! } });
+ };
+
+ const canBlockUsers =
+ currentUser?.__typename === "AdminUser" &&
+ currentUser?.userIsAdmin;
+
+ const isOwnProfile = currentUser?.id === viewedUser?.id;
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider basing `canBlockUsers` on explicit permissions rather than just `userIsAdmin`.
`canBlockUsers` is currently derived only from `currentUser.__typename === "AdminUser" && currentUser?.userIsAdmin`, while your Storybook mocks model this via `role.permissions.userPermissions.canBlockUsers`. If the backend exposes this finer-grained flag, consider using it here so block/unblock controls respect explicit permissions rather than assuming all admins can block users.
```suggestion
const canBlockUsers =
currentUser?.__typename === "AdminUser" &&
!!currentUser?.role?.permissions?.userPermissions?.canBlockUsers;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...hrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx
Outdated
Show resolved
Hide resolved
Merge branch 'main' of https://github.com/simnova/sharethrift into copilot/add-block-unblock-user-functionality
jasonmorais
reviewed
Jan 5, 2026
apps/ui-sharethrift/src/components/layouts/home/account/profile/components/profile-view.tsx
Show resolved
Hide resolved
...ts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.container.tsx
Show resolved
Hide resolved
...ts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.container.tsx
Show resolved
Hide resolved
...harethrift/src/components/layouts/home/account/profile/components/profile-view.container.tsx
Outdated
Show resolved
Hide resolved
...hrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx
Outdated
Show resolved
Hide resolved
...hrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx
Outdated
Show resolved
Hide resolved
apps/ui-sharethrift/src/components/shared/user-modals/block-user-modal.tsx
Outdated
Show resolved
Hide resolved
…pilot/add-block-unblock-user-functionality
… admin users table
Contributor
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
ViewUserProfileContainer,handleBlockUserandhandleUnblockUserare passed toProfileViewas potentially async callbacks, but they don't return/await the mutation promises, soProfileView'sawait onBlockUser/onUnblockUserresolves immediately; consider making these handlersasyncand returningblockUser/unblockUserso loading/error behavior in the modal is accurate. - In
AdminUsersTableContainer, error handling for block/unblock now happens both in the mutationonErrorcallbacks and in thetry/catcharoundhandleBlockUser/handleUnblockUser, which can result in duplicate toasts; it would be cleaner to handle messaging in one place (either the mutation options or the caller) and keep the other lean. - The new
ViewUserProfileContainerderivesisAdminViewer/canBlockUserssolely fromcurrentUser.userIsAdmin, but your GraphQL schema and description mention more granular permissions (e.g.,canViewAllUsers,canBlockUsers); consider wiring those permissions through and using them here so access control stays aligned with the domain layer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ViewUserProfileContainer`, `handleBlockUser` and `handleUnblockUser` are passed to `ProfileView` as potentially async callbacks, but they don't return/await the mutation promises, so `ProfileView`'s `await onBlockUser/onUnblockUser` resolves immediately; consider making these handlers `async` and returning `blockUser`/`unblockUser` so loading/error behavior in the modal is accurate.
- In `AdminUsersTableContainer`, error handling for block/unblock now happens both in the mutation `onError` callbacks and in the `try/catch` around `handleBlockUser/handleUnblockUser`, which can result in duplicate toasts; it would be cleaner to handle messaging in one place (either the mutation options or the caller) and keep the other lean.
- The new `ViewUserProfileContainer` derives `isAdminViewer`/`canBlockUsers` solely from `currentUser.userIsAdmin`, but your GraphQL schema and description mention more granular permissions (e.g., `canViewAllUsers`, `canBlockUsers`); consider wiring those permissions through and using them here so access control stays aligned with the domain layer.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/account/admin-dashboard/components/admin-users-table/admin-users-table.container.tsx:49-58` </location>
<code_context>
+ const [blockUser, { loading: blockLoading }] = useMutation(BlockUserDocument, {
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid double-surfacing block/unblock errors from both mutation callbacks and local try/catch blocks.
`blockUser` and `unblockUser` already show errors via their `onError` handlers. The additional `message.error(...)` in the surrounding `try/catch` leads to duplicate toasts on failure. Either rely solely on the mutation `onError` (and remove the `catch` body) or remove `onError` and handle all error reporting in the `try/catch` instead.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx:72-75` </location>
<code_context>
+ const viewedUser = userQueryData?.userById;
+ const currentUser = currentUserData?.currentUser;
+
+ const isAdmin = Boolean(currentUser?.userIsAdmin);
+
+ const isAdminViewer = isAdmin;
+ const canBlockUsers = isAdmin;
+
+ useEffect(() => {
</code_context>
<issue_to_address>
**🚨 question (security):** Blocking permissions are inferred only from `userIsAdmin`, ignoring finer-grained role permissions.
Stories define `permissions.userPermissions.canBlockUsers`, but this component derives `canBlockUsers` only from `userIsAdmin`. If you plan to support more granular permissions (e.g., some admins cannot block users), consider either extending `HomeAccountViewUserProfileCurrentUser` to fetch the relevant permission flags and derive `canBlockUsers` from them, or explicitly document that all admins can block/unblock users and adjust the mocks to match.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/profile-view.tsx:68` </location>
<code_context>
+ images: l.images ? [...l.images] : []
+});
+
export const ProfileView: React.FC<Readonly<ProfileViewProps>> = ({
- user,
- listings,
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving blocking-related state/side-effects and permission logic out of `ProfileView` into a container/hook and shared utilities so the component remains mostly presentational.
You can keep all the new behavior while slimming `ProfileView` back down into a mostly-presentational component by pushing a bit more logic into the container and collapsing some flags.
### 1. Move block/unblock state + side-effects out of `ProfileView`
Right now `ProfileView` owns modal visibility, async handlers, and error handling. That’s what makes the view feel “smart”.
A small container hook can manage all of that and pass simple callbacks/flags into the view:
```ts
// in container (or a hooks file)
function useProfileBlocking({
onBlockUser,
onUnblockUser,
}: {
onBlockUser?: (v: BlockUserFormValues) => Promise<unknown> | void;
onUnblockUser?: () => Promise<unknown> | void;
}) {
const [blockModalVisible, setBlockModalVisible] = useState(false);
const [unblockModalVisible, setUnblockModalVisible] = useState(false);
const handleOpenBlockModal = () => setBlockModalVisible(true);
const handleOpenUnblockModal = () => setUnblockModalVisible(true);
const handleConfirmBlockUser = async (values: BlockUserFormValues) => {
if (!onBlockUser) return;
try {
await onBlockUser(values);
setBlockModalVisible(false);
} catch {
message.error('Failed to block user. Please try again.');
}
};
const handleConfirmUnblockUser = async () => {
if (!onUnblockUser) return;
try {
await onUnblockUser();
setUnblockModalVisible(false);
} catch {
message.error('Failed to unblock user. Please try again.');
}
};
return {
blockModalVisible,
unblockModalVisible,
handleOpenBlockModal,
handleOpenUnblockModal,
handleConfirmBlockUser,
handleConfirmUnblockUser,
closeBlockModal: () => setBlockModalVisible(false),
closeUnblockModal: () => setUnblockModalVisible(false),
};
}
```
Then in the container:
```tsx
const blocking = useProfileBlocking({ onBlockUser, onUnblockUser });
<ProfileView
user={user}
listings={listings}
isOwnProfile={isOwnProfile}
permissions={permissions}
blocking={blocking}
/>
```
And `ProfileView` can shrink to something like:
```tsx
interface ProfileViewProps {
user: ProfileUser;
listings: ItemListing[];
isOwnProfile: boolean;
permissions: {
isBlocked: boolean;
isAdminViewer: boolean;
canBlockUser: boolean;
};
onEditSettings: () => void;
onListingClick: (listingId: string) => void;
blocking?: {
blockModalVisible: boolean;
unblockModalVisible: boolean;
handleOpenBlockModal: () => void;
handleOpenUnblockModal: () => void;
handleConfirmBlockUser: (v: BlockUserFormValues) => void;
handleConfirmUnblockUser: () => void;
closeBlockModal: () => void;
closeUnblockModal: () => void;
};
}
```
Usage inside `ProfileView`:
```tsx
const { permissions, blocking } = props;
// ...
<ProfileActions
variant="mobile"
isOwnProfile={isOwnProfile}
isBlocked={permissions.isBlocked}
canBlockUser={permissions.canBlockUser}
onEditSettings={onEditSettings}
onBlockUser={blocking?.handleOpenBlockModal}
onUnblockUser={blocking?.handleOpenUnblockModal}
/>
// ...
{permissions.canBlockUser && blocking && (
<>
<BlockUserModal
visible={blocking.blockModalVisible}
userName={getDisplayName(user)}
onConfirm={blocking.handleConfirmBlockUser}
onCancel={blocking.closeBlockModal}
loading={blockUserLoading}
/>
<UnblockUserModal
visible={blocking.unblockModalVisible}
userName={getDisplayName(user)}
onConfirm={blocking.handleConfirmUnblockUser}
onCancel={blocking.closeUnblockModal}
loading={unblockUserLoading}
/>
</>
)}
```
`ProfileView` no longer imports `useState` or `message`, and doesn’t know about `try/catch` or network concerns.
### 2. Collapse scattered flags into a single permissions/context prop
Instead of `isBlocked`, `isAdminViewer`, `canBlockUser` on the view, compute a single object in the container:
```ts
// container
const permissions = {
isBlocked,
isAdminViewer,
canBlockUser,
canEditSettings: isOwnProfile,
};
```
Then the view reads `permissions` declaratively:
```tsx
<Card
className="mb-6 profile-header"
style={{
opacity: permissions.isBlocked && permissions.isAdminViewer ? 0.7 : 1,
filter:
permissions.isBlocked && permissions.isAdminViewer ? 'grayscale(50%)' : 'none',
}}
>
<ProfileActions
variant="mobile"
isOwnProfile={isOwnProfile}
isBlocked={permissions.isBlocked}
canBlockUser={permissions.canBlockUser}
onEditSettings={onEditSettings}
onBlockUser={blocking?.handleOpenBlockModal}
onUnblockUser={blocking?.handleOpenUnblockModal}
/>
</Card>
```
This keeps the render conditions in one conceptual “permissions” object, instead of multiple booleans flowing around the component.
### 3. Consider extracting `adaptProfileListing` to a small util
If `adaptProfileListing` is (or will be) reused across profile views, moving it to a tiny utility keeps `ProfileView` focused purely on layout:
```ts
// profile-listing-adapter.ts
export const adaptProfileListing = (l: ItemListing) => ({
...l,
id: l.id,
sharingPeriodStart: new Date(l.sharingPeriodStart),
sharingPeriodEnd: new Date(l.sharingPeriodEnd),
state: ['Active', 'Paused', 'Cancelled', 'Draft', 'Expired', 'Blocked'].includes(
l.state ?? '',
)
? (l.state as 'Active' | 'Paused' | 'Cancelled' | 'Draft' | 'Expired' | 'Blocked')
: undefined,
createdAt: l.createdAt ? new Date(l.createdAt) : undefined,
sharingHistory: [],
reports: 0,
images: l.images ? [...l.images] : [],
});
```
Then in `ProfileView`:
```tsx
import { adaptProfileListing } from './profile-listing-adapter';
// ...
<ListingsGrid
listings={listings.map(adaptProfileListing)}
// ...
/>
```
These changes preserve all features (admin view, block/unblock, new header behavior) but move logic and state to more appropriate layers, reducing the complexity of `ProfileView` itself.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…//github.com/simnova/sharethrift into copilot/add-block-unblock-user-functionality
Contributor
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The access control in
ViewUserProfileContaineronly checksisAdmin/userIsAdminand ignores thecanViewAllUserspermission mentioned in the description; consider deriving and enforcing a dedicatedcanViewAllUsersflag (and using it in both the query gating and the blocked-profile redirect). - In
AdminUsersTableContaineryou computeisLoading = loading || blockLoading || unblockLoadingbut still passloadingintoComponentQueryLoader; to avoid a confusing UX where the table shows mutation loading but the outer loader does not, either passisLoadingthrough or clarify which layer should reflect mutation states. - The logic for computing a user display name (
getDisplayNameinAdminUsersTableandProfileView) is duplicated with slightly different behavior; consider extracting a shared helper to keep the formatting consistent across admin and profile views.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The access control in `ViewUserProfileContainer` only checks `isAdmin`/`userIsAdmin` and ignores the `canViewAllUsers` permission mentioned in the description; consider deriving and enforcing a dedicated `canViewAllUsers` flag (and using it in both the query gating and the blocked-profile redirect).
- In `AdminUsersTableContainer` you compute `isLoading = loading || blockLoading || unblockLoading` but still pass `loading` into `ComponentQueryLoader`; to avoid a confusing UX where the table shows mutation loading but the outer loader does not, either pass `isLoading` through or clarify which layer should reflect mutation states.
- The logic for computing a user display name (`getDisplayName` in `AdminUsersTable` and `ProfileView`) is duplicated with slightly different behavior; consider extracting a shared helper to keep the formatting consistent across admin and profile views.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/profile-view.tsx:103-112` </location>
<code_context>
+ const ownerLabel = user.firstName || user.username || 'User';
</code_context>
<issue_to_address>
**suggestion:** Header name rendering may look odd when `firstName` or `lastName` are missing
`ownerLabel` handles missing names, but the main header still uses `{user.firstName} {user.lastName?.charAt(0)}.` directly. If `firstName` is empty, this can render as `' .'` or `'undefined .'`. Reuse the same `getDisplayName`/`ownerLabel` logic for the header, or add guards for missing first/last names so the display is consistent.
Suggested implementation:
```typescript
const ownerLabel = getDisplayName(user);
```
In the same file, update the profile header text where the user's name is rendered. Look for something like:
```tsx
{user.firstName} {user.lastName?.charAt(0)}.
```
and replace it with a guarded, consistent usage of the same display logic, for example:
```tsx
{getDisplayName(user)}
{Boolean(user.lastName) && (
<>
{' '}
{user.lastName!.charAt(0)}.
</>
)}
```
or, if you prefer to reuse `ownerLabel`:
```tsx
{ownerLabel}
{Boolean(user.lastName) && !ownerLabel.endsWith('.') && (
<>
{' '}
{user.lastName!.charAt(0)}.
</>
)}
```
This ensures we never render `' .'` or `'undefined .'`, and that the header display name stays consistent with `ownerLabel`.
</issue_to_address>
### Comment 2
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/view-user-profile.container.tsx:22` </location>
<code_context>
+ canBlockUser: boolean;
+}
+
+interface UseProfileBlockingOptions {
+ onBlockUser?: (values: BlockUserFormValues) => Promise<unknown> | void;
+ onUnblockUser?: () => Promise<unknown> | void;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying this container by tightening the `useProfileBlocking` API, extracting a focused permissions helper, and consolidating navigation guards into a single effect for clearer control flow.
You can keep all current behavior but trim some of the over‑abstraction and branching to make this easier to follow.
### 1. Make `useProfileBlocking` API stricter (remove optional callbacks / guards)
Since `useProfileBlocking` is always called with both callbacks, you can require them and drop the runtime checks. This reduces branching and surface area:
```ts
interface UseProfileBlockingOptions {
onBlockUser: (values: BlockUserFormValues) => Promise<unknown> | void;
onUnblockUser: () => Promise<unknown> | void;
}
const useProfileBlocking = ({
onBlockUser,
onUnblockUser,
}: UseProfileBlockingOptions) => {
const [blockModalVisible, setBlockModalVisible] = useState(false);
const [unblockModalVisible, setUnblockModalVisible] = useState(false);
const handleOpenBlockModal = () => setBlockModalVisible(true);
const handleOpenUnblockModal = () => setUnblockModalVisible(true);
const handleConfirmBlockUser = async (values: BlockUserFormValues) => {
try {
await onBlockUser(values);
setBlockModalVisible(false);
} catch {
message.error("Failed to block user. Please try again.");
}
};
const handleConfirmUnblockUser = async () => {
try {
await onUnblockUser();
setUnblockModalVisible(false);
} catch {
message.error("Failed to unblock user. Please try again.");
}
};
return {
blockModalVisible,
unblockModalVisible,
handleOpenBlockModal,
handleOpenUnblockModal,
handleConfirmBlockUser,
handleConfirmUnblockUser,
closeBlockModal: () => setBlockModalVisible(false),
closeUnblockModal: () => setUnblockModalVisible(false),
};
};
```
Call site stays the same, but you lose the “silent no‑op” paths.
### 2. Extract a small, typed permission helper and collapse flags
The `isAdmin` / `canBlockUsersFromRole` / `canBlockUsers` / `isAdminViewer` chain can be collapsed into a helper that avoids `any` and produces a single `canBlockUser` flag, plus the `isAdminViewer` you already use:
```ts
interface RolePermissions {
permissions?: {
userPermissions?: {
canBlockUsers?: boolean;
};
};
}
const getCanBlockUser = (user: typeof currentUser): boolean => {
if (!user) return false;
const isAdmin = Boolean(user.userIsAdmin);
const rolePerms = (user.role as RolePermissions | undefined)
?.permissions?.userPermissions?.canBlockUsers;
return typeof rolePerms === "boolean" ? rolePerms : isAdmin;
};
// inside component
const isAdminViewer = Boolean(currentUser?.userIsAdmin);
const canBlockUser = getCanBlockUser(currentUser);
// ...
const permissions: ProfilePermissions = {
isBlocked: Boolean(isBlocked),
isAdminViewer,
canBlockUser,
};
```
This makes the permission logic self‑contained and removes inline `any` and nested ternaries.
### 3. Group navigation guards into a single effect
You can keep the behavior but make the redirect logic linear and easier to reason about:
```ts
useEffect(() => {
if (!userId) {
console.error("User ID is missing in URL parameters");
navigate("/account/profile");
return;
}
if (viewedUser?.isBlocked && !isAdminViewer) {
message.error("This user profile is not available");
navigate("/home");
}
}, [userId, viewedUser, isAdminViewer, navigate]);
```
This way all navigation side effects live in one place, in a clear priority order.
</issue_to_address>
### Comment 3
<location> `apps/ui-sharethrift/src/components/layouts/home/account/profile/components/profile-view.tsx:23` </location>
<code_context>
-// ...interfaces now imported from profile-view.types.ts
-
interface ProfileViewProps {
- user: ProfileUser;
- listings: ItemListing[];
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the `ProfileView` API by consolidating blocking-related props into a single control object and moving the listing adapter logic into a separate module to narrow the component’s responsibilities.
You can keep the new functionality but simplify the surface area of `ProfileView` and separate concerns a bit more.
### 1. Collapse block-related props into a single `blockingControls` prop
Right now, block/unblock concerns are spread across:
- `permissions.isBlocked`
- `permissions.canBlockUser`
- `blocking` (8 fields)
- `blockUserLoading?`
- `unblockUserLoading?`
You can keep the same behavior but make the API easier to understand by grouping all of this into a single object prop. This keeps `ProfileView` presentational while still supporting modals.
```ts
// profile-view.types.ts
export interface BlockingControls {
isBlocked: boolean;
canBlockUser: boolean;
blockModalVisible: boolean;
unblockModalVisible: boolean;
onOpenBlockModal: () => void;
onOpenUnblockModal: () => void;
onConfirmBlockUser: (values: BlockUserFormValues) => void;
onConfirmUnblockUser: () => void;
onCloseBlockModal: () => void;
onCloseUnblockModal: () => void;
blockLoading?: boolean;
unblockLoading?: boolean;
}
export interface ProfileViewProps {
user: ProfileUser;
listings: ItemListing[];
isOwnProfile: boolean;
isAdminViewer: boolean;
onEditSettings: () => void;
onListingClick: (listingId: string) => void;
blockingControls?: BlockingControls;
}
```
Then the component uses a single prop:
```tsx
export const ProfileView: React.FC<Readonly<ProfileViewProps>> = ({
user,
listings,
isOwnProfile,
isAdminViewer,
onEditSettings,
onListingClick,
blockingControls,
}) => {
const isBlocked = blockingControls?.isBlocked ?? false;
const canBlockUser = blockingControls?.canBlockUser ?? false;
// header styles
<Card
className="mb-6 profile-header"
style={{
opacity: isBlocked && isAdminViewer ? 0.7 : 1,
filter: isBlocked && isAdminViewer ? 'grayscale(50%)' : 'none',
}}
>
<ProfileActions
variant="mobile"
isOwnProfile={isOwnProfile}
isBlocked={isBlocked}
canBlockUser={canBlockUser}
onEditSettings={onEditSettings}
onBlockUser={blockingControls?.onOpenBlockModal}
onUnblockUser={blockingControls?.onOpenUnblockModal}
/>
{/* ... */}
</Card>
{canBlockUser && blockingControls && (
<>
<BlockUserModal
visible={blockingControls.blockModalVisible}
userName={getDisplayName(user)}
onConfirm={blockingControls.onConfirmBlockUser}
onCancel={blockingControls.onCloseBlockModal}
loading={blockingControls.blockLoading}
/>
<UnblockUserModal
visible={blockingControls.unblockModalVisible}
userName={getDisplayName(user)}
onConfirm={blockingControls.onConfirmUnblockUser}
onCancel={blockingControls.onCloseUnblockModal}
loading={blockingControls.unblockLoading}
/>
</>
)}
};
```
This removes the `permissions` + `blocking` + `blockUserLoading` + `unblockUserLoading` triangle at the top level and makes valid combinations more obvious: if `blockingControls` is present, block/unblock is enabled.
### 2. Extract `adaptProfileListing` into a small adapter module
You already improved things by extracting the mapping into `adaptProfileListing`, but it still lives inside the view. Moving it to a small adapter file keeps the component more focused:
```ts
// profile-view.adapters.ts
import type { ItemListing } from '../../../../../../generated';
export const adaptProfileListing = (l: ItemListing) => ({
...l,
id: l.id,
sharingPeriodStart: new Date(l.sharingPeriodStart),
sharingPeriodEnd: new Date(l.sharingPeriodEnd),
state: ['Active', 'Paused', 'Cancelled', 'Draft', 'Expired', 'Blocked'].includes(
l.state ?? '',
)
? (l.state as
| 'Active'
| 'Paused'
| 'Cancelled'
| 'Draft'
| 'Expired'
| 'Blocked'
| undefined)
: undefined,
createdAt: l.createdAt ? new Date(l.createdAt) : undefined,
sharingHistory: [],
reports: 0,
images: l.images ? [...l.images] : [],
});
```
And in `ProfileView`:
```ts
import { adaptProfileListing } from './profile-view.adapters';
// ...
<ListingsGrid
listings={listings.map(adaptProfileListing)}
// ...
/>
```
These two changes keep all functionality but make `ProfileView`’s API and responsibilities narrower and easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Admins need the ability to block/unblock users directly from profile pages, with blocked profiles showing visual indicators and access restrictions.
Changes
UI Components
apps/ui-sharethrift/src/components/shared/user-modals/)BlockUserModal: Confirmation dialog with reason input fieldUnblockUserModal: Confirmation dialog displaying original block reasonProfile View Enhancements
Updated
ProfileViewcomponentcanBlockUsers)New
ViewUserProfileContainer/account/profile/:userIdroutecanViewAllUsersRouting & Navigation
/account/profile/:userIdroute to account routesGraphQL Integration
Backend mutations (
blockUser,unblockUser) already existed; connected to UI via:Security
PersonalUser.isBlockedsetter)canBlockUsersandcanViewAllUserspermissionsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by Sourcery
Add admin-controlled block/unblock capabilities to user profiles and integrate them into admin user management and profile views.
New Features:
Enhancements:
Tests:
Chores: