fix: address code review findings for claim mapping rules editor

- Bump all font sizes from 11px/10px to 12px (project minimum)
- Fix handleMove race condition: use mutateAsync + Promise.all
- Clear stale test results after rule create/edit/delete/reorder
- Replace inline styles with CSS module classes in OidcConfigPage
- Remove dead .editRow CSS class
- Replace inline chevron with Lucide icon

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
hsiegeln
2026-04-14 16:58:06 +02:00
parent 3985bb8a43
commit a3ec0aaef3
4 changed files with 51 additions and 32 deletions

View File

@@ -40,7 +40,7 @@
} }
.subtitle { .subtitle {
font-size: 11px; font-size: 12px;
color: var(--text-muted); color: var(--text-muted);
} }
@@ -57,7 +57,7 @@
.table th { .table th {
text-align: left; text-align: left;
padding: 6px 8px; padding: 6px 8px;
font-size: 11px; font-size: 12px;
color: var(--text-muted); color: var(--text-muted);
text-transform: uppercase; text-transform: uppercase;
letter-spacing: 0.05em; letter-spacing: 0.05em;
@@ -82,7 +82,7 @@
.priorityCell { .priorityCell {
color: var(--text-muted); color: var(--text-muted);
font-size: 11px; font-size: 12px;
width: 30px; width: 30px;
} }
@@ -98,7 +98,7 @@
display: inline-block; display: inline-block;
padding: 2px 8px; padding: 2px 8px;
border-radius: 10px; border-radius: 10px;
font-size: 11px; font-size: 12px;
white-space: nowrap; white-space: nowrap;
} }
@@ -188,11 +188,6 @@
width: 120px; width: 120px;
} }
.editRow input,
.editRow select {
font-size: 12px;
}
.emptyState { .emptyState {
padding: 32px 20px; padding: 32px 20px;
text-align: center; text-align: center;
@@ -202,7 +197,7 @@
.footer { .footer {
padding: 12px 20px; padding: 12px 20px;
font-size: 11px; font-size: 12px;
color: var(--text-muted); color: var(--text-muted);
border-top: 1px solid var(--border); border-top: 1px solid var(--border);
} }
@@ -228,7 +223,7 @@
} }
.testToggleHint { .testToggleHint {
font-size: 11px; font-size: 12px;
color: var(--text-muted); color: var(--text-muted);
} }
@@ -255,7 +250,7 @@
padding: 10px; padding: 10px;
color: var(--text); color: var(--text);
font-family: var(--font-mono); font-family: var(--font-mono);
font-size: 11px; font-size: 12px;
resize: vertical; resize: vertical;
} }
@@ -271,7 +266,7 @@
} }
.testResultsTitle { .testResultsTitle {
font-size: 11px; font-size: 12px;
color: var(--text-muted); color: var(--text-muted);
text-transform: uppercase; text-transform: uppercase;
letter-spacing: 0.05em; letter-spacing: 0.05em;
@@ -283,7 +278,7 @@
} }
.testMatchLabel { .testMatchLabel {
font-size: 11px; font-size: 12px;
color: var(--success); color: var(--success);
margin-bottom: 4px; margin-bottom: 4px;
} }
@@ -297,7 +292,7 @@
border-top: 1px solid var(--border); border-top: 1px solid var(--border);
padding-top: 8px; padding-top: 8px;
margin-top: 4px; margin-top: 4px;
font-size: 11px; font-size: 12px;
color: var(--text-muted); color: var(--text-muted);
} }

View File

@@ -81,6 +81,7 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
{ {
onSuccess: () => { onSuccess: () => {
setAddForm({ ...EMPTY_FORM }); setAddForm({ ...EMPTY_FORM });
setTestResult(null);
toast({ title: 'Rule created', variant: 'success' }); toast({ title: 'Rule created', variant: 'success' });
}, },
onError: (e) => toast({ title: 'Failed to create rule', description: e.message, variant: 'error', duration: 86_400_000 }), onError: (e) => toast({ title: 'Failed to create rule', description: e.message, variant: 'error', duration: 86_400_000 }),
@@ -113,6 +114,7 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
{ {
onSuccess: () => { onSuccess: () => {
setEditingId(null); setEditingId(null);
setTestResult(null);
toast({ title: 'Rule updated', variant: 'success' }); toast({ title: 'Rule updated', variant: 'success' });
}, },
onError: (e) => toast({ title: 'Failed to update rule', description: e.message, variant: 'error', duration: 86_400_000 }), onError: (e) => toast({ title: 'Failed to update rule', description: e.message, variant: 'error', duration: 86_400_000 }),
@@ -125,6 +127,7 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
deleteRule.mutate(deleteTarget.id, { deleteRule.mutate(deleteTarget.id, {
onSuccess: () => { onSuccess: () => {
setDeleteTarget(null); setDeleteTarget(null);
setTestResult(null);
toast({ title: 'Rule deleted', variant: 'warning' }); toast({ title: 'Rule deleted', variant: 'warning' });
}, },
onError: (e) => { onError: (e) => {
@@ -134,23 +137,21 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
}); });
} }
function handleMove(rule: ClaimMappingRule, direction: 'up' | 'down') { async function handleMove(rule: ClaimMappingRule, direction: 'up' | 'down') {
const idx = sorted.findIndex((r) => r.id === rule.id); const idx = sorted.findIndex((r) => r.id === rule.id);
const swapIdx = direction === 'up' ? idx - 1 : idx + 1; const swapIdx = direction === 'up' ? idx - 1 : idx + 1;
if (swapIdx < 0 || swapIdx >= sorted.length) return; if (swapIdx < 0 || swapIdx >= sorted.length) return;
const other = sorted[swapIdx]; const other = sorted[swapIdx];
// Swap priorities try {
updateRule.mutate( await Promise.all([
{ id: rule.id, claim: rule.claim, matchType: rule.matchType, matchValue: rule.matchValue, action: rule.action, target: rule.target, priority: other.priority }, 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 }),
onSuccess: () => { ]);
updateRule.mutate( setTestResult(null);
{ id: other.id, claim: other.claim, matchType: other.matchType, matchValue: other.matchValue, action: other.action, target: other.target, priority: rule.priority }, } catch {
); toast({ title: 'Failed to reorder rules', variant: 'error', duration: 86_400_000 });
}, }
},
);
} }
function handleTest() { function handleTest() {
@@ -283,7 +284,7 @@ export default function ClaimMappingRulesModal({ open, onClose }: Props) {
{/* Test panel */} {/* Test panel */}
<div className={styles.testToggle} onClick={() => setTestOpen(!testOpen)}> <div className={styles.testToggle} onClick={() => setTestOpen(!testOpen)}>
<span style={{ fontSize: 10, color: 'var(--text-muted)' }}>{testOpen ? '\u25B2' : '\u25BC'}</span> {testOpen ? <ChevronUp size={12} /> : <ChevronDown size={12} />}
<span className={styles.testToggleLabel}>Test Rules</span> <span className={styles.testToggleLabel}>Test Rules</span>
<span className={styles.testToggleHint}>Paste a decoded JWT to preview which rules would fire</span> <span className={styles.testToggleHint}>Paste a decoded JWT to preview which rules would fire</span>
</div> </div>

View File

@@ -44,3 +44,26 @@
.roleInput { .roleInput {
width: 200px; width: 200px;
} }
.advancedRulesRow {
margin-top: 18px;
padding-top: 14px;
border-top: 1px solid var(--border);
}
.advancedRulesInner {
display: flex;
justify-content: space-between;
align-items: center;
}
.advancedRulesLabel {
font-size: 13px;
color: var(--text-secondary, var(--text-muted));
}
.advancedRulesCount {
font-size: 12px;
color: var(--text-muted);
margin-left: 8px;
}

View File

@@ -275,12 +275,12 @@ export default function OidcConfigPage() {
disabled={!editing} disabled={!editing}
/> />
</FormField> </FormField>
<div style={{ marginTop: 18, paddingTop: 14, borderTop: '1px solid var(--border)' }}> <div className={styles.advancedRulesRow}>
<div style={{ display: 'flex', justifyContent: 'space-between', alignItems: 'center' }}> <div className={styles.advancedRulesInner}>
<div> <div>
<span style={{ fontSize: 13, color: 'var(--text-secondary, var(--text-muted))' }}>Advanced Rules</span> <span className={styles.advancedRulesLabel}>Advanced Rules</span>
{claimRules.length > 0 && ( {claimRules.length > 0 && (
<span style={{ fontSize: 11, color: 'var(--text-muted)', marginLeft: 8 }}> <span className={styles.advancedRulesCount}>
{claimRules.length} active rule{claimRules.length !== 1 ? 's' : ''} {claimRules.length} active rule{claimRules.length !== 1 ? 's' : ''}
</span> </span>
)} )}