refactor: switch claim mapping editor to local-edit-then-apply pattern

All edits (add, edit, delete, reorder) now modify local state only. Cancel discards changes, Apply diffs local vs server and issues the necessary create/update/delete API calls. Target selects now include a placeholder option. Footer shows Cancel and Apply buttons. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-14 17:07:36 +02:00
parent 0e87161426
commit 5edefb2180
2 changed files with 164 additions and 71 deletions
--- a/ui/src/pages/Admin/ClaimMappingRulesModal.module.css
+++ b/ui/src/pages/Admin/ClaimMappingRulesModal.module.css
@@ -197,9 +197,23 @@

 .footer {
  padding: 12px 20px;
+  border-top: 1px solid var(--border);
+  display: flex;
+  justify-content: space-between;
+  align-items: center;
+  gap: 12px;
+}
+
+.footerHint {
  font-size: 12px;
  color: var(--text-muted);
-  border-top: 1px solid var(--border);
+  flex: 1;
+}
+
+.footerActions {
+  display: flex;
+  gap: 8px;
+  flex-shrink: 0;
 }

 .testToggle {
--- a/ui/src/pages/Admin/ClaimMappingRulesModal.tsx
+++ b/ui/src/pages/Admin/ClaimMappingRulesModal.tsx
@@ -1,4 +1,4 @@
-import { useState } from 'react';
+import { useState, useEffect, useCallback } from 'react';
 import { ChevronUp, ChevronDown, Pencil, X, Check } from 'lucide-react';
 import { Button, Input, Select, ConfirmDialog, useToast } from '@cameleer/design-system';
 import {
@@ -32,6 +32,21 @@ const TEST_PLACEHOLDER = `{
  "groups": ["frontend", "design"]
 }`;

+// Local rule type — server rules have string IDs, new rules get temp- prefixed IDs
+interface LocalRule {
+  id: string;
+  claim: string;
+  matchType: string;
+  matchValue: string;
+  action: string;
+  target: string;
+  priority: number;
+}
+
+function toLocal(rule: ClaimMappingRule): LocalRule {
+  return { id: rule.id, claim: rule.claim, matchType: rule.matchType, matchValue: rule.matchValue, action: rule.action, target: rule.target, priority: rule.priority };
+}
+
 interface Props {
  open: boolean;
  onClose: () => void;
@@ -39,12 +54,16 @@ interface Props {

 export default function ClaimMappingRulesModal({ open, onClose }: Props) {
  const { toast } = useToast();
-  const { data: rules = [], isLoading } = useClaimMappingRules();
+  const { data: serverRules = [], isLoading } = useClaimMappingRules();
  const createRule = useCreateClaimMappingRule();
  const updateRule = useUpdateClaimMappingRule();
  const deleteRule = useDeleteClaimMappingRule();
  const testRules = useTestClaimMappingRules();

+  // Local working copy — only saved to server on Apply
+  const [localRules, setLocalRules] = useState<LocalRule[]>([]);
+  const [dirty, setDirty] = useState(false);
+
  // Add form
  const [addForm, setAddForm] = useState({ ...EMPTY_FORM });

@@ -53,7 +72,10 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
  const [editForm, setEditForm] = useState({ ...EMPTY_FORM });

  // Delete confirm
-  const [deleteTarget, setDeleteTarget] = useState<ClaimMappingRule | null>(null);
+  const [deleteTarget, setDeleteTarget] = useState<LocalRule | null>(null);
+
+  // Apply state
+  const [applying, setApplying] = useState(false);

  // Test panel
  const [testOpen, setTestOpen] = useState(false);
@@ -69,39 +91,54 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
  const groupOptions = groups.map((g) => ({ value: g.name, label: g.name }));

  function targetOptions(action: string) {
-    return action === 'assignRole' ? roleOptions : groupOptions;
+    const opts = action === 'assignRole' ? roleOptions : groupOptions;
+    return [{ value: '', label: action === 'assignRole' ? 'Select role...' : 'Select group...' }, ...opts];
  }

+  // Initialize local rules from server when modal opens or server data arrives
+  useEffect(() => {
+    if (open && !dirty) {
+      setLocalRules(serverRules.map(toLocal));
+    }
+  }, [open, serverRules, dirty]);
+
+  // Reset state when modal closes
+  const handleClose = useCallback(() => {
+    setDirty(false);
+    setEditingId(null);
+    setEditForm({ ...EMPTY_FORM });
+    setAddForm({ ...EMPTY_FORM });
+    setDeleteTarget(null);
+    setTestResult(null);
+    setTestError('');
+    onClose();
+  }, [onClose]);
+
  if (!open) return null;

-  const sorted = [...rules].sort((a, b) => a.priority - b.priority);
+  const sorted = [...localRules].sort((a, b) => a.priority - b.priority);
  const matchedRuleIds = new Set((testResult?.matchedRules ?? []).map((r) => r.ruleId));

-  // ── Handlers ──────────────────────────────────────────────────────
+  // ── Local Handlers (no API calls) ─────────────────────────────────

  function handleAdd() {
    const maxPriority = sorted.length > 0 ? Math.max(...sorted.map((r) => r.priority)) : -1;
-    createRule.mutate(
-      {
-        claim: addForm.claim.trim(),
-        matchType: addForm.matchType,
-        matchValue: addForm.matchValue.trim(),
-        action: addForm.action,
-        target: addForm.target.trim(),
-        priority: maxPriority + 1,
-      },
-      {
-        onSuccess: () => {
-          setAddForm({ ...EMPTY_FORM });
-          setTestResult(null);
-          toast({ title: 'Rule created', variant: 'success' });
-        },
-        onError: (e) => toast({ title: 'Failed to create rule', description: e.message, variant: 'error', duration: 86_400_000 }),
-      },
-    );
+    const newRule: LocalRule = {
+      id: `temp-${crypto.randomUUID()}`,
+      claim: addForm.claim.trim(),
+      matchType: addForm.matchType,
+      matchValue: addForm.matchValue.trim(),
+      action: addForm.action,
+      target: addForm.target,
+      priority: maxPriority + 1,
+    };
+    setLocalRules((prev) => [...prev, newRule]);
+    setAddForm({ ...EMPTY_FORM });
+    setDirty(true);
+    setTestResult(null);
  }

-  function startEdit(rule: ClaimMappingRule) {
+  function startEdit(rule: LocalRule) {
    setEditingId(rule.id);
    setEditForm({
      claim: rule.claim,
@@ -112,60 +149,97 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
    });
  }

-  function saveEdit(rule: ClaimMappingRule) {
-    updateRule.mutate(
-      {
-        id: rule.id,
-        claim: editForm.claim.trim(),
-        matchType: editForm.matchType,
-        matchValue: editForm.matchValue.trim(),
-        action: editForm.action,
-        target: editForm.target.trim(),
-        priority: rule.priority,
-      },
-      {
-        onSuccess: () => {
-          setEditingId(null);
-          setTestResult(null);
-          toast({ title: 'Rule updated', variant: 'success' });
-        },
-        onError: (e) => toast({ title: 'Failed to update rule', description: e.message, variant: 'error', duration: 86_400_000 }),
-      },
+  function saveEdit(rule: LocalRule) {
+    setLocalRules((prev) =>
+      prev.map((r) =>
+        r.id === rule.id
+          ? { ...r, claim: editForm.claim.trim(), matchType: editForm.matchType, matchValue: editForm.matchValue.trim(), action: editForm.action, target: editForm.target }
+          : r,
+      ),
    );
+    setEditingId(null);
+    setDirty(true);
+    setTestResult(null);
  }

  function confirmDelete() {
    if (!deleteTarget) return;
-    deleteRule.mutate(deleteTarget.id, {
-      onSuccess: () => {
-        setDeleteTarget(null);
-        setTestResult(null);
-        toast({ title: 'Rule deleted', variant: 'warning' });
-      },
-      onError: (e) => {
-        setDeleteTarget(null);
-        toast({ title: 'Failed to delete rule', description: e.message, variant: 'error', duration: 86_400_000 });
-      },
-    });
+    setLocalRules((prev) => prev.filter((r) => r.id !== deleteTarget.id));
+    setDeleteTarget(null);
+    setDirty(true);
+    setTestResult(null);
  }

-  async function handleMove(rule: ClaimMappingRule, direction: 'up' | 'down') {
+  function handleMove(rule: LocalRule, direction: 'up' | 'down') {
    const idx = sorted.findIndex((r) => r.id === rule.id);
    const swapIdx = direction === 'up' ? idx - 1 : idx + 1;
    if (swapIdx < 0 || swapIdx >= sorted.length) return;

    const other = sorted[swapIdx];
+    setLocalRules((prev) =>
+      prev.map((r) => {
+        if (r.id === rule.id) return { ...r, priority: other.priority };
+        if (r.id === other.id) return { ...r, priority: rule.priority };
+        return r;
+      }),
+    );
+    setDirty(true);
+    setTestResult(null);
+  }
+
+  // ── Apply (save all changes to server) ────────────────────────────
+
+  async function handleApply() {
+    setApplying(true);
    try {
-      await Promise.all([
-        updateRule.mutateAsync({ id: rule.id, claim: rule.claim, matchType: rule.matchType, matchValue: rule.matchValue, action: rule.action, target: rule.target, priority: other.priority }),
-        updateRule.mutateAsync({ id: other.id, claim: other.claim, matchType: other.matchType, matchValue: other.matchValue, action: other.action, target: other.target, priority: rule.priority }),
-      ]);
-      setTestResult(null);
-    } catch {
-      toast({ title: 'Failed to reorder rules', variant: 'error', duration: 86_400_000 });
+      const serverIds = new Set(serverRules.map((r) => r.id));
+      const localIds = new Set(localRules.map((r) => r.id));
+
+      // Delete: rules on server but not in local
+      const toDelete = serverRules.filter((r) => !localIds.has(r.id));
+      for (const r of toDelete) {
+        await deleteRule.mutateAsync(r.id);
+      }
+
+      // Create: rules in local with temp- prefix
+      for (const r of localRules) {
+        if (r.id.startsWith('temp-')) {
+          await createRule.mutateAsync({
+            claim: r.claim, matchType: r.matchType, matchValue: r.matchValue,
+            action: r.action, target: r.target, priority: r.priority,
+          });
+        }
+      }
+
+      // Update: rules that exist on both sides but have changed
+      for (const local of localRules) {
+        if (local.id.startsWith('temp-')) continue;
+        if (!serverIds.has(local.id)) continue;
+        const server = serverRules.find((r) => r.id === local.id);
+        if (!server) continue;
+        const changed = server.claim !== local.claim || server.matchType !== local.matchType
+          || server.matchValue !== local.matchValue || server.action !== local.action
+          || server.target !== local.target || server.priority !== local.priority;
+        if (changed) {
+          await updateRule.mutateAsync({
+            id: local.id, claim: local.claim, matchType: local.matchType,
+            matchValue: local.matchValue, action: local.action, target: local.target,
+            priority: local.priority,
+          });
+        }
+      }
+
+      setDirty(false);
+      toast({ title: 'Rules saved', variant: 'success' });
+    } catch (e: any) {
+      toast({ title: 'Failed to save rules', description: e.message, variant: 'error', duration: 86_400_000 });
+    } finally {
+      setApplying(false);
    }
  }

+  // ── Test ──────────────────────────────────────────────────────────
+
  function handleTest() {
    setTestError('');
    setTestResult(null);
@@ -192,12 +266,12 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
    return action === 'assignRole' ? 'assign role' : 'add to group';
  }

-  const addDisabled = !addForm.claim.trim() || !addForm.matchValue.trim() || !addForm.target.trim();
+  const addDisabled = !addForm.claim.trim() || !addForm.matchValue.trim() || !addForm.target;

  // ── JSX ───────────────────────────────────────────────────────────

  return (
-    <div className={styles.overlay} onClick={onClose}>
+    <div className={styles.overlay} onClick={handleClose}>
      <div className={styles.modal} onClick={(e) => e.stopPropagation()}>
        {/* Header */}
        <div className={styles.header}>
@@ -205,7 +279,7 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
            <div className={styles.title}>Claim Mapping Rules</div>
            <div className={styles.subtitle}>Map JWT claims to Cameleer roles and groups. Evaluated on every OIDC login.</div>
          </div>
-          <button className={styles.iconBtn} onClick={onClose}><X size={16} /></button>
+          <button className={styles.iconBtn} onClick={handleClose}><X size={16} /></button>
        </div>

        {/* Body */}
@@ -286,7 +360,7 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
            <Input placeholder="Match value" value={addForm.matchValue} onChange={(e) => setAddForm({ ...addForm, matchValue: e.target.value })} className={styles.valueInput} />
            <Select options={ACTION_OPTIONS} value={addForm.action} onChange={(e) => setAddForm({ ...addForm, action: e.target.value, target: '' })} className={styles.actionSelect} />
            <Select options={targetOptions(addForm.action)} value={addForm.target} onChange={(e) => setAddForm({ ...addForm, target: e.target.value })} className={styles.targetInput} />
-            <Button size="sm" variant="primary" onClick={handleAdd} disabled={addDisabled} loading={createRule.isPending}>+ Add</Button>
+            <Button size="sm" variant="primary" onClick={handleAdd} disabled={addDisabled}>+ Add</Button>
          </div>

          {/* Test panel */}
@@ -342,9 +416,15 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
          )}
        </div>

-        {/* Footer */}
+        {/* Footer with Cancel / Apply */}
        <div className={styles.footer}>
-          Rules are evaluated top-to-bottom by priority on every OIDC login. If no rules match, the server falls back to roles from the token, then to default roles.
+          <span className={styles.footerHint}>
+            Rules are evaluated top-to-bottom by priority on every OIDC login. If no rules match, the server falls back to roles from the token, then to default roles.
+          </span>
+          <div className={styles.footerActions}>
+            <Button size="sm" variant="ghost" onClick={handleClose}>Cancel</Button>
+            <Button size="sm" variant="primary" onClick={handleApply} disabled={!dirty} loading={applying}>Apply</Button>
+          </div>
        </div>
      </div>

@@ -354,7 +434,6 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
        onConfirm={confirmDelete}
        message={`Delete this rule? (${deleteTarget?.claim} ${deleteTarget?.matchType} "${deleteTarget?.matchValue}")`}
        confirmText="delete"
-        loading={deleteRule.isPending}
      />
    </div>
  );