feat(directory): Phase 2 — Roles + Entitlements + Comments

src/frontend/src/components/comments/comment-sidebar.tsx

@@ -28,13 +28,22 @@ import { Avatar } from '@/components/ui/avatar';
 import { RelativeDate } from '@/components/common/relative-date';
 import { Separator } from '@/components/ui/separator';
 import { ScrollArea } from '@/components/ui/scroll-area';
+import { PrincipalPicker } from '@/components/common/principal-picker';
+import { buildAudienceTokens, parseAudienceTokens } from '@/lib/audience';
 // Select components removed - unused
 
 interface CommentFormData {
   title: string;
   comment: string;
   selectedTeams: string[];
   selectedRoles: string[];
+  /**
+   * Additional audience tokens collected from the PrincipalPicker.
+   * These are plain emails (users) and plain group names (groups) --
+   * NOT prefixed with ``team:`` / ``role:``. They merge with the
+   * checkbox selections at submit time.
+   */
+  selectedPrincipals: string[];
 }
 
 interface TimelineEntry {
@@ -83,6 +92,7 @@ const CommentSidebar: React.FC<CommentSidebarProps> = ({
     comment: '',
     selectedTeams: [],
     selectedRoles: [],
+    selectedPrincipals: [],
   });
 
   // Ref for the ScrollArea viewport to control scrolling
@@ -195,11 +205,14 @@ const CommentSidebar: React.FC<CommentSidebarProps> = ({
       return;
     }
 
-    // Build audience array with team: and role: prefixes
-    const audienceTokens: string[] = [
-      ...formData.selectedTeams.map(teamId => `team:${teamId}`),
-      ...formData.selectedRoles.map(roleName => `role:${roleName}`),
-    ];
+    // Build audience array. ``team:`` / ``role:`` prefixes come from
+    // the checkbox lists; plain emails / group names come from the
+    // PrincipalPicker (Directory pick) and flow through unchanged.
+    const audienceTokens: string[] = buildAudienceTokens({
+      teams: formData.selectedTeams,
+      roles: formData.selectedRoles,
+      principals: formData.selectedPrincipals,
+    });
 
     const commentData: CommentCreate | CommentUpdate = {
       title: formData.title || null,
@@ -259,7 +272,7 @@ const CommentSidebar: React.FC<CommentSidebarProps> = ({
     }
 
     // Reset form and refresh timeline
-    setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [] });
+    setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [], selectedPrincipals: [] });
     setEditingComment(null);
     setIsFormOpen(false);
     await fetchTimeline();
@@ -302,31 +315,23 @@ const CommentSidebar: React.FC<CommentSidebarProps> = ({
   const handleEdit = (comment: Comment) => {
     setEditingComment(comment);
 
-    // Parse audience tokens to extract teams and roles
-    const teams: string[] = [];
-    const roles: string[] = [];
-
-    if (comment.audience) {
-      comment.audience.forEach(token => {
-        if (token.startsWith('team:')) {
-          teams.push(token.substring(5));
-        } else if (token.startsWith('role:')) {
-          roles.push(token.substring(5));
-        }
-      });
-    }
-
+    // Parse audience tokens. ``team:`` / ``role:`` go into the
+    // respective checkbox state; anything else is a Directory
+    // principal (plain email or group name) and rehydrates the
+    // PrincipalPicker.
+    const parsed = parseAudienceTokens(comment.audience);
     setFormData({
       title: comment.title || '',
       comment: comment.comment,
-      selectedTeams: teams,
-      selectedRoles: roles,
+      selectedTeams: parsed.teams,
+      selectedRoles: parsed.roles,
+      selectedPrincipals: parsed.principals,
     });
     setIsFormOpen(true);
   };
 
   const resetForm = () => {
-    setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [] });
+    setFormData({ title: '', comment: '', selectedTeams: [], selectedRoles: [], selectedPrincipals: [] });
     setEditingComment(null);
     setIsFormOpen(false);
   };
@@ -458,7 +463,27 @@ const CommentSidebar: React.FC<CommentSidebarProps> = ({
             ))}
           </div>
         </div>
-
+
+        {/* Directory principals (users / groups). Sits alongside the
+            team / role checkbox lists -- picked values flow into the
+            same ``audience`` field as plain emails / group names. */}
+        <div>
+          <Label htmlFor="audience-principals">Users &amp; groups</Label>
+          <div className="mt-1">
+            <PrincipalPicker
+              id="audience-principals"
+              multiple
+              accepts={['user', 'group']}
+              value={formData.selectedPrincipals}
+              onChange={(next) =>
+                setFormData({ ...formData, selectedPrincipals: next })
+              }
+              placeholder="Add users or groups…"
+              aria-label="Audience users and groups"
+            />
+          </div>
+        </div>
+
         {/* Display selected teams and roles */}
         {(formData.selectedTeams.length > 0 || formData.selectedRoles.length > 0) && (
           <div className="flex flex-wrap gap-1 mt-2">

src/frontend/src/components/settings/role-form-dialog.tsx

@@ -18,6 +18,8 @@ import { ACCESS_LEVEL_ORDER } from '../../lib/permissions';
 import { features as orderedFeatures } from '@/config/features'; // Import the ordered features
 import { usePermissions } from '@/stores/permissions-store';
 import { Checkbox } from '@/components/ui/checkbox';
+import { PrincipalPicker } from '@/components/common/principal-picker';
+import { normaliseAssignedGroups } from '@/lib/assigned-groups';
 
 interface RoleFormDialogProps {
     isOpen: boolean;
@@ -324,21 +326,16 @@ const RoleFormDialog: React.FC<RoleFormDialogProps> = ({
                                             name="assigned_groups"
                                             control={control}
                                             render={({ field }) => {
-                                                // Convert array to string for display, keep as string for editing
-                                                const displayValue = Array.isArray(field.value) 
-                                                    ? field.value.join(', ') 
-                                                    : (field.value || '');
-
+                                                const value = normaliseAssignedGroups(field.value);
                                                 return (
-                                                    <Input
+                                                    <PrincipalPicker
                                                         id="assigned_groups"
+                                                        multiple
+                                                        accepts={['group']}
+                                                        value={value}
+                                                        onChange={(next) => field.onChange(next)}
                                                         placeholder={t('roles.general.assignedGroupsPlaceholder')}
-                                                        value={displayValue}
-                                                        onChange={(e) => {
-                                                            // Store as string, will be split on submit
-                                                            field.onChange(e.target.value);
-                                                        }}
-                                                        className="focus:ring-2 focus:ring-offset-2"
+                                                        aria-label={t('roles.general.assignedGroups')}
                                                     />
                                                 );
                                             }}

src/frontend/src/lib/assigned-groups.test.ts

@@ -0,0 +1,46 @@
+/**
+ * Tests for AppRole.assigned_groups normalisation.
+ *
+ * Covers the Phase-2 acceptance criterion: pre-existing Roles
+ * ``assigned_groups`` strings still render and save without UI breakage.
+ */
+
+import { describe, expect, it } from 'vitest';
+
+import { normaliseAssignedGroups } from './assigned-groups';
+
+describe('normaliseAssignedGroups', () => {
+  it('returns an array unchanged when already an array of strings', () => {
+    expect(normaliseAssignedGroups(['data-producers', 'data-stewards'])).toEqual([
+      'data-producers',
+      'data-stewards',
+    ]);
+  });
+
+  it('drops empty strings from an array', () => {
+    expect(normaliseAssignedGroups(['a', '', 'b'])).toEqual(['a', 'b']);
+  });
+
+  it('splits a legacy comma-separated string and trims whitespace', () => {
+    expect(normaliseAssignedGroups('data-producers, data-stewards , compliance')).toEqual([
+      'data-producers',
+      'data-stewards',
+      'compliance',
+    ]);
+  });
+
+  it('drops trailing / repeated commas', () => {
+    expect(normaliseAssignedGroups('a,,b,')).toEqual(['a', 'b']);
+  });
+
+  it('returns [] for nullish / non-string-non-array inputs', () => {
+    expect(normaliseAssignedGroups(null)).toEqual([]);
+    expect(normaliseAssignedGroups(undefined)).toEqual([]);
+    expect(normaliseAssignedGroups(42)).toEqual([]);
+    expect(normaliseAssignedGroups({})).toEqual([]);
+  });
+
+  it('returns [] for an empty string', () => {
+    expect(normaliseAssignedGroups('')).toEqual([]);
+  });
+});

src/frontend/src/lib/assigned-groups.ts

@@ -0,0 +1,23 @@
+/**
+ * Helper for normalising the AppRole ``assigned_groups`` field.
+ *
+ * Historically this field was edited as a comma-separated text input
+ * and persisted as ``string[]``. With the Directory PrincipalPicker
+ * the field is always ``string[]``, but old role records loaded from
+ * the API (or other code paths) may still surface a raw comma string.
+ * This helper makes the picker tolerant of both shapes without
+ * leaking the conditional throughout the form.
+ */
+
+export function normaliseAssignedGroups(raw: unknown): string[] {
+  if (Array.isArray(raw)) {
+    return (raw as unknown[]).filter((x): x is string => typeof x === 'string' && x.length > 0);
+  }
+  if (typeof raw === 'string' && raw.length > 0) {
+    return raw
+      .split(',')
+      .map((g) => g.trim())
+      .filter(Boolean);
+  }
+  return [];
+}

src/frontend/src/lib/audience.test.ts

@@ -0,0 +1,79 @@
+/**
+ * Tests for the audience-token round-trip used by the comment
+ * composer. These cover the Phase-2 acceptance criterion:
+ * "an audience containing a mix of users, groups, teams (team:*), and
+ *  roles (role:*) round-trips correctly".
+ */
+
+import { describe, expect, it } from 'vitest';
+
+import { buildAudienceTokens, parseAudienceTokens } from './audience';
+
+describe('parseAudienceTokens', () => {
+  it('splits team:, role:, and bare principal tokens', () => {
+    const parsed = parseAudienceTokens([
+      'team:t-1',
+      'role:Admin',
+      'alice@x.com',
+      'Data Producers',
+    ]);
+    expect(parsed.teams).toEqual(['t-1']);
+    expect(parsed.roles).toEqual(['Admin']);
+    expect(parsed.principals).toEqual(['alice@x.com', 'Data Producers']);
+  });
+
+  it('returns empty arrays for null / undefined / empty input', () => {
+    expect(parseAudienceTokens(null)).toEqual({ teams: [], roles: [], principals: [] });
+    expect(parseAudienceTokens(undefined)).toEqual({ teams: [], roles: [], principals: [] });
+    expect(parseAudienceTokens([])).toEqual({ teams: [], roles: [], principals: [] });
+  });
+
+  it('skips non-string and empty tokens', () => {
+    const parsed = parseAudienceTokens([
+      'team:t-1',
+      '',
+      // @ts-expect-error verify defensive guard
+      null,
+      'alice@x.com',
+    ]);
+    expect(parsed.teams).toEqual(['t-1']);
+    expect(parsed.principals).toEqual(['alice@x.com']);
+  });
+
+  it('treats prefix-only tokens as the empty id behind the prefix', () => {
+    const parsed = parseAudienceTokens(['team:', 'role:']);
+    // Empty ids are preserved -- the caller decides whether to filter.
+    expect(parsed.teams).toEqual(['']);
+    expect(parsed.roles).toEqual(['']);
+    expect(parsed.principals).toEqual([]);
+  });
+});
+
+describe('buildAudienceTokens', () => {
+  it('emits team: / role: prefixes for checkbox selections, principals bare', () => {
+    const tokens = buildAudienceTokens({
+      teams: ['t-1', 't-2'],
+      roles: ['Admin'],
+      principals: ['alice@x.com', 'Data Producers'],
+    });
+    expect(tokens).toEqual([
+      'team:t-1',
+      'team:t-2',
+      'role:Admin',
+      'alice@x.com',
+      'Data Producers',
+    ]);
+  });
+
+  it('round-trips a mixed audience without loss', () => {
+    const original = [
+      'team:t-7',
+      'role:Reviewer',
+      'bob@x.com',
+      'Engineering',
+    ];
+    const parsed = parseAudienceTokens(original);
+    const rebuilt = buildAudienceTokens(parsed);
+    expect(rebuilt).toEqual(original);
+  });
+});

src/frontend/src/lib/audience.ts

@@ -0,0 +1,50 @@
+/**
+ * Audience token helpers for the comment composer.
+ *
+ * Audience tokens flow on the wire as a single ``string[]`` mixing
+ * three shapes:
+ *
+ * - ``team:<team-id>`` — selected from the Teams checkbox list.
+ * - ``role:<role-name>`` — selected from the App Roles checkbox list.
+ * - anything else — a Directory principal (plain user email/UPN or
+ *   plain group display name) picked via the PrincipalPicker.
+ *
+ * These helpers keep the round-trip (compose -> store -> edit) honest
+ * across the three shapes and are pure functions so they can be unit
+ * tested without rendering Radix UI.
+ */
+
+export interface ParsedAudience {
+  teams: string[];
+  roles: string[];
+  principals: string[];
+}
+
+const TEAM_PREFIX = 'team:';
+const ROLE_PREFIX = 'role:';
+
+export function parseAudienceTokens(
+  audience: string[] | null | undefined,
+): ParsedAudience {
+  const out: ParsedAudience = { teams: [], roles: [], principals: [] };
+  if (!audience) return out;
+  for (const token of audience) {
+    if (typeof token !== 'string' || token.length === 0) continue;
+    if (token.startsWith(TEAM_PREFIX)) {
+      out.teams.push(token.slice(TEAM_PREFIX.length));
+    } else if (token.startsWith(ROLE_PREFIX)) {
+      out.roles.push(token.slice(ROLE_PREFIX.length));
+    } else {
+      out.principals.push(token);
+    }
+  }
+  return out;
+}
+
+export function buildAudienceTokens(parsed: ParsedAudience): string[] {
+  return [
+    ...parsed.teams.map((t) => `${TEAM_PREFIX}${t}`),
+    ...parsed.roles.map((r) => `${ROLE_PREFIX}${r}`),
+    ...parsed.principals,
+  ];
+}