Skip to content

feat: admin/owner role management on group page #912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
coodos wants to merge 4 commits into
base: main
Choose a base branch
from
+203 −35
Open

feat: admin/owner role management on group page #912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c10d5de
feat: add endpoint for managing member admin/owner roles
coodos Mar 11, 2026
a576211
feat: add role management UI to group member list
coodos Mar 11, 2026
6691ac6
fix: allow admins to remove admin, show members without charter
coodos Mar 11, 2026
712d379
fix: replace lock-based webhook rejection with timestamp check for chats
coodos Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 52 additions & 18 deletions platforms/blabsy/api/src/controllers/WebhookController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,40 +94,47 @@ export class WebhookController {
axios.post(new URL("blabsy", process.env.ANCHR_URL).toString(), req.body)
}

// Early duplicate check
if (adapter.lockedIds.includes(id)) {
const mapping = Object.values(adapter.mapping).find(
(m) => m.schemaId === schemaId,
);
if (!mapping) throw new Error();
const tableName = mapping.tableName + "s";

// For chats, skip the lock check and use timestamp comparison instead
const isChatData = tableName === "chats";

if (!isChatData && adapter.lockedIds.includes(id)) {
console.log(`Webhook skipped - ID ${id} already locked`);
return res.status(200).json({ success: true, skipped: true });
}

console.log(`Processing webhook for ID: ${id}`);

// Lock the global ID immediately to prevent duplicates
adapter.addToLockedIds(id);

const mapping = Object.values(adapter.mapping).find(
(m) => m.schemaId === schemaId,
);
if (!mapping) throw new Error();
const tableName = mapping.tableName + "s";
// Lock the global ID immediately to prevent duplicates (non-chat)
if (!isChatData) {
adapter.addToLockedIds(id);
}
Comment on lines +103 to +116
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Race condition window for concurrent chat webhooks.

By skipping the lock check for chats entirely (line 106), multiple concurrent webhooks for the same chat ID can all proceed to updateChatIfNewer simultaneously. The lock is only acquired at line 284 (after the timestamp check), leaving a race window where:

  1. Webhook A reads existing doc, passes timestamp check
  2. Webhook B reads same doc (before A writes), also passes timestamp check
  3. Both A and B update the document

Since the timestamp comparison uses Timestamp.now() (see related comment on updateChatIfNewer), this race is more likely to cause issues. Consider acquiring the lock earlier in the chat path, or using a Firestore transaction for atomic read-compare-write.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@platforms/blabsy/api/src/controllers/WebhookController.ts` around lines 103 -
116, The chat path currently skips lock checks causing a race before
updateChatIfNewer; change the flow in WebhookController.ts so chats acquire the
same global lock (use adapter.lockedIds and call adapter.addToLockedIds(id))
before performing the timestamp comparison, or replace the timestamp-check +
write with an atomic Firestore transaction inside updateChatIfNewer so the
read-compare-write is done atomically; ensure you release the lock after the
update or let the transaction handle concurrency if you choose the transaction
approach.


const local = await adapter.fromGlobal({ data, mapping });

console.log("Webhook data received:", {
globalId: id,
tableName,
console.log("Webhook data received:", {
globalId: id,
tableName,
hasEname: !!local.data.ename,
ename: local.data.ename
ename: local.data.ename
});

// Get the local ID from the mapping database
const localId = await adapter.mappingDb.getLocalId(id);

if (localId) {
console.log(`LOCAL, updating - ID: ${id}, LocalID: ${localId}`);
// Lock local ID early to prevent duplicate processing
adapter.addToLockedIds(localId);
await this.updateRecord(tableName, localId, local.data);
if (isChatData) {
await this.updateChatIfNewer(localId, local.data);
} else {
adapter.addToLockedIds(localId);
await this.updateRecord(tableName, localId, local.data);
}
} else {
console.log(`NOT LOCAL, creating - ID: ${id}`);
await this.createRecord(tableName, local.data, req.body.id);
Expand Down Expand Up @@ -252,6 +259,33 @@ export class WebhookController {
const mappedData = await this.mapDataToFirebase(tableName, data);
await docRef.update(mappedData);
}

private async updateChatIfNewer(localId: string, data: any) {
const docRef = this.db.collection("chats").doc(localId);
const docSnapshot = await docRef.get();

if (!docSnapshot.exists) {
console.warn(`Chat document '${localId}' does not exist. Skipping update.`);
return;
}

const existing = docSnapshot.data();
const mappedData = this.mapChatData(data, Timestamp.now());

// Compare updatedAt timestamps - accept if incoming is more recent
const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0;

if (incomingUpdatedAt < existingUpdatedAt) {
console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`);
return;
}
Comment on lines +273 to +282
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Timestamp comparison uses current time instead of incoming webhook's timestamp.

The comparison is ineffective because mappedData.updatedAt is set to Timestamp.now() (via mapChatData at line 410), not the actual timestamp from the incoming webhook data. This means:

  • incomingUpdatedAt = current wall-clock time (always)
  • existingUpdatedAt = when the document was last written

Since Timestamp.now() will almost always be greater than the existing updatedAt, out-of-order webhooks will still overwrite newer data with older data.

If the intent is to skip stale webhooks, you need to compare the original data.updatedAt from the webhook payload, not the mapped result.

Proposed fix: Compare using incoming webhook's timestamp 
     private async updateChatIfNewer(localId: string, data: any) {
         const docRef = this.db.collection("chats").doc(localId);
         const docSnapshot = await docRef.get();

         if (!docSnapshot.exists) {
             console.warn(`Chat document '${localId}' does not exist. Skipping update.`);
             return;
         }

         const existing = docSnapshot.data();
-        const mappedData = this.mapChatData(data, Timestamp.now());
-
-        // Compare updatedAt timestamps - accept if incoming is more recent
         const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
-        const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0;
+        
+        // Use the actual incoming webhook's updatedAt for comparison
+        const incomingUpdatedAt = data.updatedAt 
+            ? new Date(data.updatedAt).getTime() 
+            : Date.now();

         if (incomingUpdatedAt < existingUpdatedAt) {
             console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`);
             return;
         }

+        const mappedData = this.mapChatData(data, Timestamp.now());
         adapter.addToLockedIds(localId);
         await docRef.update(mappedData);
         console.log(`Chat ${localId} updated via webhook (timestamp check passed)`);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const mappedData = this.mapChatData(data, Timestamp.now());
// Compare updatedAt timestamps - accept if incoming is more recent
const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0;
if (incomingUpdatedAt < existingUpdatedAt) {
console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`);
return;
}
const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
// Use the actual incoming webhook's updatedAt for comparison
const incomingUpdatedAt = data.updatedAt
? new Date(data.updatedAt).getTime()
: Date.now();
if (incomingUpdatedAt < existingUpdatedAt) {
console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`);
return;
}
const mappedData = this.mapChatData(data, Timestamp.now());
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@platforms/blabsy/api/src/controllers/WebhookController.ts` around lines 273 -
282, The timestamp check is using Timestamp.now() via mapChatData, so
incomingUpdatedAt is wrong; change the comparison to use the webhook payload's
original timestamp (data.updatedAt) instead of
mappedData.updatedAt/Timestamp.now(). Locate the comparison around
existingUpdatedAt/incomingUpdatedAt and compute incomingUpdatedAt from the
incoming payload (e.g., parse data.updatedAt or pass it through mapChatData
before you overwrite it), or modify mapChatData to accept and preserve the
incoming timestamp so that incomingUpdatedAt reflects the webhook's timestamp
rather than current time.


adapter.addToLockedIds(localId);
await docRef.update(mappedData);
console.log(`Chat ${localId} updated via webhook (timestamp check passed)`);
}
Comment on lines +263 to +287
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Consider using a Firestore transaction for atomic read-compare-write.

The current implementation has a TOCTOU (time-of-check to time-of-use) race condition: the document is read, the timestamp is compared, and then updated in separate operations. Two concurrent webhooks can both pass the timestamp check before either writes.

A Firestore transaction would make this atomic:

Proposed fix using Firestore transaction 
     private async updateChatIfNewer(localId: string, data: any) {
         const docRef = this.db.collection("chats").doc(localId);
-        const docSnapshot = await docRef.get();
-
-        if (!docSnapshot.exists) {
-            console.warn(`Chat document '${localId}' does not exist. Skipping update.`);
-            return;
-        }
-
-        const existing = docSnapshot.data();
-        const mappedData = this.mapChatData(data, Timestamp.now());
-
-        // Compare updatedAt timestamps - accept if incoming is more recent
-        const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
-        const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0;
-
-        if (incomingUpdatedAt < existingUpdatedAt) {
-            console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`);
-            return;
-        }
-
-        adapter.addToLockedIds(localId);
-        await docRef.update(mappedData);
-        console.log(`Chat ${localId} updated via webhook (timestamp check passed)`);
+        const incomingUpdatedAt = data.updatedAt 
+            ? new Date(data.updatedAt).getTime() 
+            : Date.now();
+        
+        await this.db.runTransaction(async (transaction) => {
+            const docSnapshot = await transaction.get(docRef);
+            
+            if (!docSnapshot.exists) {
+                console.warn(`Chat document '${localId}' does not exist. Skipping update.`);
+                return;
+            }
+            
+            const existing = docSnapshot.data();
+            const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
+            
+            if (incomingUpdatedAt < existingUpdatedAt) {
+                console.log(`Chat ${localId} webhook skipped - local data is more recent`);
+                return;
+            }
+            
+            const mappedData = this.mapChatData(data, Timestamp.now());
+            transaction.update(docRef, mappedData);
+            console.log(`Chat ${localId} updated via webhook (timestamp check passed)`);
+        });
+        
+        adapter.addToLockedIds(localId);
     }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@platforms/blabsy/api/src/controllers/WebhookController.ts` around lines 263 -
287, The updateChatIfNewer function has a TOCTOU race: it reads the document,
compares timestamps, then updates outside an atomic context; change it to use a
Firestore transaction (use this.db.runTransaction) that reads the doc inside the
transaction, computes existingUpdatedAt and incomingUpdatedAt using
mapChatData(data, Timestamp.now()) (or compute mappedData and use
mappedData.updatedAt inside the transaction), and only performs
docRef.update(mappedData) within the transaction when incomingUpdatedAt >=
existingUpdatedAt; remove or move adapter.addToLockedIds so it is not relied on
for atomicity (either call it after a successful transaction commit or handle
locking inside the transaction logic) and ensure errors are handled/logged when
the transaction aborts.


private mapDataToFirebase(tableName: string, data: any): any {
const now = Timestamp.now();
console.log("MAPPING DATA TO ", tableName);
Expand Down
68 changes: 68 additions & 0 deletions platforms/group-charter-manager/api/src/controllers/GroupController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,74 @@ export class GroupController {
}
}

async updateMemberRole(req: Request, res: Response) {
try {
const { groupId, userId: targetUserId } = req.params;
const requestingUserId = (req as any).user?.id;

if (!requestingUserId) {
return res.status(401).json({ error: "Unauthorized" });
}

const { role } = req.body;
if (!["admin", "member", "owner"].includes(role)) {
return res.status(400).json({ error: "Invalid role. Must be 'admin', 'member', or 'owner'" });
}

const group = await this.groupService.getGroupById(groupId);
if (!group) {
return res.status(404).json({ error: "Group not found" });
}

// Verify target user is a participant
if (!group.participants.some(p => p.id === targetUserId)) {
return res.status(400).json({ error: "User is not a participant in this group" });
}

const isOwner = group.owner === requestingUserId;
const isAdmin = group.admins?.includes(requestingUserId);

if (!isOwner && !isAdmin) {
return res.status(403).json({ error: "Access denied" });
}

const currentAdmins = group.admins || [];

if (role === "admin") {
// Grant admin - admins and owners can do this
if (currentAdmins.includes(targetUserId)) {
return res.status(400).json({ error: "User is already an admin" });
}
const newAdmins = [...currentAdmins, targetUserId];
await this.groupService.updateGroup(groupId, { admins: newAdmins } as any);
} else if (role === "member") {
// Remove admin - admins and owners can do this
if (targetUserId === group.owner) {
return res.status(400).json({ error: "Cannot demote the owner" });
}
const newAdmins = currentAdmins.filter(id => id !== targetUserId);
await this.groupService.updateGroup(groupId, { admins: newAdmins } as any);
Comment on lines +381 to +387
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Authorization behavior contradicts stated test plan.

The code allows any admin to remove admin privileges from other users. However, the PR objectives state: "Admin: grant admin; verify cannot remove admin from others or transfer ownership (checked)."

The commit message indicates this is intentional ("allow admins to revoke admin privileges"), but this contradicts the test plan verification. Please clarify the intended authorization model:

  1. If admins should not be able to demote other admins, add an owner-only check here.
  2. If admins can demote other admins, update the PR test plan to reflect this.
🔒 Option 1: Restrict to owner-only 
         } else if (role === "member") {
             // Remove admin - admins and owners can do this
+            if (!isOwner) {
+                return res.status(403).json({ error: "Only the owner can remove admin privileges" });
+            }
             if (targetUserId === group.owner) {
                 return res.status(400).json({ error: "Cannot demote the owner" });
             }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@platforms/group-charter-manager/api/src/controllers/GroupController.ts`
around lines 381 - 387, The code currently allows any admin to demote other
admins (removing targetUserId from currentAdmins via
this.groupService.updateGroup), but the test plan requires only the owner can
remove admins; enforce that by adding an owner-only authorization check before
modifying admins: verify the requester's id (e.g., req.user.id or the
controller's existing requestingUserId variable used elsewhere) equals
group.owner and return 403 if not; then proceed to compute newAdmins
(currentAdmins.filter...) and call this.groupService.updateGroup(groupId, {
admins: newAdmins }). Ensure the check is placed in the branch handling role ===
"member" before mutating admins.

} else if (role === "owner") {
// Transfer ownership - only owner can do this
if (!isOwner) {
return res.status(403).json({ error: "Only the owner can transfer ownership" });
}
// Old owner becomes admin, new owner gets added to admins if not already
let newAdmins = currentAdmins.includes(requestingUserId) ? [...currentAdmins] : [...currentAdmins, requestingUserId];
if (!newAdmins.includes(targetUserId)) {
newAdmins.push(targetUserId);
}
await this.groupService.updateGroup(groupId, { owner: targetUserId, admins: newAdmins } as any);
}

const updatedGroup = await this.groupService.getGroupById(groupId);
res.json(updatedGroup);
} catch (error) {
console.error("Error updating member role:", error);
res.status(500).json({ error: "Internal server error" });
}
}

async getCharterSigningStatus(req: Request, res: Response) {
try {
const { id } = req.params;
Expand Down
1 change: 1 addition & 0 deletions platforms/group-charter-manager/api/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ app.get("/api/signing/sessions/:sessionId", authGuard, (req, res) => {
app.get("/api/groups/:groupId", authGuard, groupController.getGroup.bind(groupController));
app.post("/api/groups/:groupId/participants", authGuard, groupController.addParticipants.bind(groupController));
app.delete("/api/groups/:groupId/participants/:userId", authGuard, groupController.removeParticipant.bind(groupController));
app.patch("/api/groups/:groupId/members/:userId/role", authGuard, groupController.updateMemberRole.bind(groupController));

// Start server
app.listen(port, () => {
Expand Down
13 changes: 7 additions & 6 deletions platforms/group-charter-manager/client/src/app/charter/[id]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,12 +419,13 @@ export default function CharterDetail({
)}

{/* Charter Signing Status */}
{group.charter && (
<CharterSigningStatus
groupId={group.id}
charterContent={group.charter}
/>
)}
<CharterSigningStatus
groupId={group.id}
charterContent={group.charter || ""}
currentUserId={user?.id}
currentUserIsAdmin={group.admins?.includes(user?.id || '') || false}
currentUserIsOwner={group.owner === user?.id}
/>
</div>
</div>

Expand Down
86 changes: 75 additions & 11 deletions platforms/group-charter-manager/client/src/components/charter-signing-status.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
"use client";

import { useState, useEffect } from "react";
import { CheckCircle, Circle, AlertTriangle } from "lucide-react";
import { CheckCircle, Circle, AlertTriangle, MoreVertical, Shield, ShieldOff, Crown } from "lucide-react";
import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card";
import { Badge } from "@/components/ui/badge";
import { Button } from "@/components/ui/button";
import {
DropdownMenu,
DropdownMenuContent,
DropdownMenuItem,
DropdownMenuTrigger,
} from "@/components/ui/dropdown-menu";
import { apiClient } from "@/lib/apiClient";
import { useToast } from "@/hooks/use-toast";

interface CharterSigningStatusProps {
groupId: string;
charterContent: string;
currentUserId?: string;
currentUserIsAdmin?: boolean;
currentUserIsOwner?: boolean;
}

interface Participant {
Expand All @@ -28,7 +38,7 @@ interface SigningStatus {
isSigned: boolean;
}

export function CharterSigningStatus({ groupId, charterContent }: CharterSigningStatusProps) {
export function CharterSigningStatus({ groupId, charterContent, currentUserId, currentUserIsAdmin, currentUserIsOwner }: CharterSigningStatusProps) {
const [signingStatus, setSigningStatus] = useState<SigningStatus | null>(null);
const [loading, setLoading] = useState(true);
const { toast } = useToast();
Expand All @@ -54,6 +64,28 @@ export function CharterSigningStatus({ groupId, charterContent }: CharterSigning
}
};

const handleRoleChange = async (targetUserId: string, newRole: "admin" | "member" | "owner") => {
if (newRole === "owner" && !window.confirm("Are you sure you want to transfer ownership? This cannot be undone.")) {
return;
}
try {
await apiClient.patch(`/api/groups/${groupId}/members/${targetUserId}/role`, { role: newRole });
toast({
title: "Success",
description: newRole === "admin" ? "Admin privileges granted" : newRole === "member" ? "Admin privileges removed" : "Ownership transferred",
});
fetchSigningStatus();
} catch (error: any) {
toast({
title: "Error",
description: error.response?.data?.error || "Failed to update role",
variant: "destructive",
});
}
};

const canManageRoles = currentUserIsAdmin || currentUserIsOwner;



if (loading) {
Expand Down Expand Up @@ -102,22 +134,25 @@ export function CharterSigningStatus({ groupId, charterContent }: CharterSigning
className="flex items-center justify-between p-3 bg-gray-50 rounded-lg"
>
<div className="flex items-center gap-3">
{participant.hasSigned ? (
<CheckCircle className="h-5 w-5 text-green-600" />
) : (
<Circle className="h-5 w-5 text-gray-400" />
)}
{charterContent ? (
participant.hasSigned ? (
<CheckCircle className="h-5 w-5 text-green-600" />
) : (
<Circle className="h-5 w-5 text-gray-400" />
)
) : null}
<div>
<p className="font-medium text-sm">
{participant.name || participant.ename || 'Unknown User'}
</p>
<p className="text-xs text-gray-500">
{participant.hasSigned ? 'Signed' : 'Not signed yet'}
</p>
{charterContent && (
<p className="text-xs text-gray-500">
{participant.hasSigned ? 'Signed' : 'Not signed yet'}
</p>
)}
</div>
</div>
<div className="flex items-center gap-2">
{/* Show admin role if applicable */}
{participant.isAdmin && (
<Badge variant="secondary" className="text-xs">
Admin
Expand All @@ -128,6 +163,35 @@ export function CharterSigningStatus({ groupId, charterContent }: CharterSigning
Owner
</Badge>
)}
{canManageRoles && participant.id !== currentUserId && (
<DropdownMenu>
<DropdownMenuTrigger asChild>
<Button variant="ghost" size="icon" className="h-7 w-7">
<MoreVertical className="h-4 w-4" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent align="end">
{!participant.isAdmin && !participant.isOwner && (
<DropdownMenuItem onClick={() => handleRoleChange(participant.id, "admin")}>
<Shield className="mr-2 h-4 w-4" />
Make Admin
</DropdownMenuItem>
)}
{participant.isAdmin && !participant.isOwner && (
<DropdownMenuItem onClick={() => handleRoleChange(participant.id, "member")}>
<ShieldOff className="mr-2 h-4 w-4" />
Remove Admin
</DropdownMenuItem>
)}
{!participant.isOwner && currentUserIsOwner && (
<DropdownMenuItem className="text-amber-600" onClick={() => handleRoleChange(participant.id, "owner")}>
<Crown className="mr-2 h-4 w-4" />
Transfer Ownership
</DropdownMenuItem>
)}
</DropdownMenuContent>
</DropdownMenu>
)}
Comment on lines +166 to +194
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Empty dropdown menu can render when admin views the owner.

When an admin (non-owner) views this component, the dropdown will render for the owner participant, but all menu items will be hidden:

  • "Make Admin" hidden (owner is already an admin)
  • "Remove Admin" hidden (cannot remove admin from owner)
  • "Transfer Ownership" hidden (only owner can transfer)

This results in clicking the three-dot button and seeing an empty menu.

🐛 Proposed fix: only render dropdown if actions are available 
-                                    {canManageRoles && participant.id !== currentUserId && (
+                                    {canManageRoles && participant.id !== currentUserId && (
+                                        // Only show dropdown if there are available actions
+                                        (!participant.isAdmin && !participant.isOwner) || // can make admin
+                                        (participant.isAdmin && !participant.isOwner) || // can remove admin
+                                        (!participant.isOwner && currentUserIsOwner) // can transfer ownership
+                                    ) && (
                                         <DropdownMenu>

Or extract to a helper:

const hasAvailableActions = (participant: Participant) => {
    if (participant.id === currentUserId) return false;
    if (!canManageRoles) return false;
    // Can make admin
    if (!participant.isAdmin && !participant.isOwner) return true;
    // Can remove admin (admin but not owner)
    if (participant.isAdmin && !participant.isOwner) return true;
    // Can transfer ownership (current user is owner, target is not owner)
    if (!participant.isOwner && currentUserIsOwner) return true;
    return false;
};
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@platforms/group-charter-manager/client/src/components/charter-signing-status.tsx`
around lines 166 - 194, Render the DropdownMenu only when there are actionable
items: add a guard (or helper like hasAvailableActions) that checks
canManageRoles, participant.id !== currentUserId, and at least one of the three
action conditions (participant not admin and not owner -> Make Admin;
participant is admin and not owner -> Remove Admin; participant not owner and
currentUserIsOwner -> Transfer Ownership), then wrap the existing DropdownMenu
(and its trigger) with that guard so the three-dot button never opens an empty
menu; reference the participant, currentUserId, canManageRoles,
currentUserIsOwner, handleRoleChange and DropdownMenu elements when applying the
change.

</div>
</div>
))}
Expand Down
Loading