Skip to content

Commit b9b98d6

Browse files
Aurelioloclaude
Aureliolo
and
claude
committed
fix: address CodeRabbit review feedback -- 22 items (#936)
Types & contracts: - Constrain SinkRotation.strategy to 'builtin' | 'external' union - Replace text-[10px] with text-micro token in SinkCard, DependencyIndicator - Replace shadow-lg with var(--so-shadow-card-hover) in FloatingSaveBar - Replace min-w-[80px] with min-w-20 in TagInput Semantic HTML & a11y: - Fix button>h2 nesting in NamespaceSection (h2 now wraps button) - Replace role="listbox" with role="group" in TagInput (not a true listbox) - Add role="status" + aria-live="polite" to SearchInput result count - Add a11y.test: 'error' to SearchInput stories - Remove pointer-events-none from disabled SettingRow (blocks tooltip) - Derive effectiveNamespace instead of setState in effect (ESLint rule) Sink CRUD: - Fix handleSave NaN: validate/default rotation numbers, add isConsole guard - Merge custom_sinks with existing instead of overwriting - Derive editSink from store state (not stale captured object) - Default rotation strategy to 'none' (no rotation) instead of 'builtin' - Fix SinkCard.stories/tests: strategy 'rotating' -> 'builtin' Logic: - Case-insensitive key matching in useSettingsKeyboard (Caps Lock safe) - Deduplicate within batch in TagInput addItems - Short-circuit parseArrayItems on blank input - Add missing onSelect prop to SettingsHealthSection story - Rename SourceBadge ConfigFile story to YamlRendersNull - Fix RestartBanner test description: 'warning' -> 'alert' - Update density-tokens comment to mention .density-medium Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 4fe6d1e commit b9b98d6

20 files changed

Lines changed: 94 additions & 59 deletions

‎web/src/__tests__/pages/settings/RestartBanner.test.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ describe('RestartBanner', () => {
2626
expect(onDismiss).toHaveBeenCalledOnce()
2727
})
2828

29-
it('has warning role for accessibility', () => {
29+
it('has alert role for accessibility', () => {
3030
render(<RestartBanner count={1} onDismiss={() => {}} />)
3131
expect(screen.getByRole('alert')).toBeInTheDocument()
3232
})

‎web/src/__tests__/pages/settings/sinks/SinkCard.test.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ function makeSink(overrides: Partial<SinkInfo> = {}): SinkInfo {
99
sink_type: 'file',
1010
level: 'INFO',
1111
json_format: true,
12-
rotation: { strategy: 'rotating', max_bytes: 10_485_760, backup_count: 5 },
12+
rotation: { strategy: 'builtin', max_bytes: 10_485_760, backup_count: 5 },
1313
is_default: true,
1414
enabled: true,
1515
routing_prefixes: [],
@@ -71,7 +71,7 @@ describe('SinkCard', () => {
7171
it('renders rotation display', () => {
7272
render(
7373
<SinkCard
74-
sink={makeSink({ rotation: { strategy: 'rotating', max_bytes: 10_485_760, backup_count: 5 } })}
74+
sink={makeSink({ rotation: { strategy: 'builtin', max_bytes: 10_485_760, backup_count: 5 } })}
7575
onEdit={vi.fn()}
7676
/>,
7777
)

‎web/src/api/types.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1374,7 +1374,7 @@ export interface UpdateSettingRequest {
13741374
// ── Sink configuration ──────────────────────────────────────
13751375

13761376
export interface SinkRotation {
1377-
strategy: string
1377+
strategy: 'builtin' | 'external'
13781378
max_bytes: number
13791379
backup_count: number
13801380
}

‎web/src/components/ui/tag-input.tsx‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,15 @@ export function TagInput({ value, onChange, disabled, placeholder, className }:
1616

1717
const addItems = useCallback(
1818
(items: string[]) => {
19-
const trimmed = items.map((s) => s.trim()).filter(Boolean)
20-
const unique = trimmed.filter((item) => !value.includes(item))
19+
const seen = new Set(value)
20+
const unique: string[] = []
21+
for (const raw of items) {
22+
const item = raw.trim()
23+
if (item && !seen.has(item)) {
24+
seen.add(item)
25+
unique.push(item)
26+
}
27+
}
2128
if (unique.length > 0) {
2229
onChange([...value, ...unique])
2330
}
@@ -69,8 +76,8 @@ export function TagInput({ value, onChange, disabled, placeholder, className }:
6976
className,
7077
)}
7178
onClick={() => inputRef.current?.focus()}
72-
role="listbox"
73-
aria-label="Tag list"
79+
role="group"
80+
aria-label={placeholder ?? 'Tags'}
7481
>
7582
{value.map((item, i) => {
7683
// Stable key: value + occurrence index for duplicates
@@ -80,8 +87,6 @@ export function TagInput({ value, onChange, disabled, placeholder, className }:
8087
<span
8188
key={stableKey}
8289
className="inline-flex items-center gap-1 rounded bg-accent/10 px-1.5 py-0.5 text-xs font-medium text-accent"
83-
role="option"
84-
aria-selected
8590
>
8691
{item}
8792
{!disabled && (
@@ -109,7 +114,7 @@ export function TagInput({ value, onChange, disabled, placeholder, className }:
109114
onPaste={handlePaste}
110115
disabled={disabled}
111116
placeholder={value.length === 0 ? placeholder : undefined}
112-
className="min-w-[80px] flex-1 bg-transparent text-xs text-foreground outline-none placeholder:text-text-muted"
117+
className="min-w-20 flex-1 bg-transparent text-xs text-foreground outline-none placeholder:text-text-muted"
113118
/>
114119
</div>
115120
)

‎web/src/hooks/useSettingsKeyboard.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ export function useSettingsKeyboard({
1616
const mod = e.metaKey || e.ctrlKey
1717
if (!mod) return
1818

19-
if (e.key === 's') {
19+
if (e.key.toLowerCase() === 's') {
2020
e.preventDefault()
2121
if (canSave) onSave()
2222
} else if (e.key === '/') {

‎web/src/pages/SettingsPage.tsx‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,11 @@ export default function SettingsPage() {
158158
[filteredByNamespace],
159159
)
160160

161+
// Derive effective namespace -- clear selection if filtering removed all its entries
162+
const effectiveNamespace = activeNamespace && (namespaceCounts.get(activeNamespace) ?? 0) > 0
163+
? activeNamespace
164+
: null
165+
161166
const controllerDisabledMap = useMemo(
162167
() => buildControllerDisabledMap(entries, dirtyValues),
163168
[entries, dirtyValues],
@@ -323,7 +328,7 @@ export default function SettingsPage() {
323328
<>
324329
<NamespaceTabBar
325330
namespaces={NAMESPACE_ORDER}
326-
activeNamespace={activeNamespace}
331+
activeNamespace={effectiveNamespace}
327332
onSelect={setActiveNamespace}
328333
namespaceCounts={namespaceCounts}
329334
namespaceIcons={NAMESPACE_ICONS}
@@ -343,7 +348,7 @@ export default function SettingsPage() {
343348

344349
<AnimatePresence mode="wait">
345350
<motion.div
346-
key={activeNamespace ?? 'all'}
351+
key={effectiveNamespace ?? 'all'}
347352
initial={{ opacity: 0 }}
348353
animate={{ opacity: 1 }}
349354
exit={{ opacity: 0 }}
@@ -352,7 +357,7 @@ export default function SettingsPage() {
352357
<StaggerGroup className="space-y-[var(--spacing-section-gap)]">
353358
{NAMESPACE_ORDER
354359
.filter((ns) => filteredByNamespace.has(ns))
355-
.filter((ns) => activeNamespace === null || ns === activeNamespace)
360+
.filter((ns) => effectiveNamespace === null || ns === effectiveNamespace)
356361
.map((ns) => (
357362
<StaggerItem key={ns}>
358363
<ErrorBoundary level="section">
@@ -364,8 +369,8 @@ export default function SettingsPage() {
364369
onValueChange={handleValueChange}
365370
savingKeys={storeSavingKeys}
366371
controllerDisabledMap={controllerDisabledMap}
367-
forceOpen={activeNamespace !== null || searchQuery.length > 0}
368-
hideHeader={activeNamespace !== null}
372+
forceOpen={effectiveNamespace !== null || searchQuery.length > 0}
373+
hideHeader={effectiveNamespace !== null}
369374
changedKeys={changedKeys}
370375
highlightQuery={searchQuery}
371376
footerAction={ns === 'observability' ? (

‎web/src/pages/SettingsSinksPage.tsx‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@ import { SinkFormDrawer } from './settings/sinks/SinkFormDrawer'
1515
export default function SettingsSinksPage() {
1616
const navigate = useNavigate()
1717
const { sinks, loading, error, fetchSinks, saveSink, testConfig } = useSinksStore()
18-
const [editSink, setEditSink] = useState<SinkInfo | null>(null)
18+
const [editSinkId, setEditSinkId] = useState<string | null>(null)
1919
const [drawerOpen, setDrawerOpen] = useState(false)
2020
const [isNewSink, setIsNewSink] = useState(false)
21+
const editSink = editSinkId ? sinks.find((s) => s.identifier === editSinkId) ?? null : null
2122

2223
useEffect(() => {
2324
fetchSinks()
@@ -36,27 +37,27 @@ export default function SettingsSinksPage() {
3637
})
3738

3839
const handleEdit = useCallback((sink: SinkInfo) => {
39-
setEditSink(sink)
40+
setEditSinkId(sink.identifier)
4041
setIsNewSink(false)
4142
setDrawerOpen(true)
4243
}, [])
4344

4445
const handleAddNew = useCallback(() => {
45-
setEditSink(null)
46+
setEditSinkId(null)
4647
setIsNewSink(true)
4748
setDrawerOpen(true)
4849
}, [])
4950

5051
const handleCloseDrawer = useCallback(() => {
5152
setDrawerOpen(false)
52-
setEditSink(null)
53+
setEditSinkId(null)
5354
setIsNewSink(false)
5455
}, [])
5556

5657
const handleSave = useCallback(async (sink: SinkInfo) => {
5758
await saveSink(sink)
5859
setDrawerOpen(false)
59-
setEditSink(null)
60+
setEditSinkId(null)
6061
setIsNewSink(false)
6162
}, [saveSink])
6263

‎web/src/pages/settings/DependencyIndicator.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export function DependencyIndicator({ dependents, className }: DependencyIndicat
1515
return (
1616
<span
1717
className={cn(
18-
'inline-flex items-center gap-1 rounded px-1.5 py-0.5 text-[10px] font-medium bg-accent/5 text-text-muted',
18+
'inline-flex items-center gap-1 rounded px-1.5 py-0.5 text-micro font-medium bg-accent/5 text-text-muted',
1919
className,
2020
)}
2121
title={tooltip}

‎web/src/pages/settings/FloatingSaveBar.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export function FloatingSaveBar({
3030
animate={{ y: 0, opacity: 1 }}
3131
exit={{ y: 80, opacity: 0 }}
3232
transition={{ type: 'spring', stiffness: 400, damping: 30 }}
33-
className="sticky bottom-4 z-10 mx-auto flex w-fit items-center gap-3 rounded-lg border border-border bg-surface p-card shadow-lg"
33+
className="sticky bottom-4 z-10 mx-auto flex w-fit items-center gap-3 rounded-lg border border-border bg-surface p-card shadow-[var(--so-shadow-card-hover)]"
3434
aria-live="polite"
3535
>
3636
<span className="text-sm text-text-secondary">

‎web/src/pages/settings/NamespaceSection.tsx‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -145,18 +145,19 @@ export function NamespaceSection({
145145
return (
146146
<section className="rounded-lg border border-border bg-card">
147147
{!hideHeader && (
148-
<button
149-
type="button"
150-
onClick={() => { if (!forceOpen) setCollapsed((v) => !v) }}
151-
className={cn(
152-
'flex w-full items-center gap-3 p-card',
153-
'text-left transition-colors hover:bg-card-hover',
154-
)}
155-
aria-expanded={isOpen}
156-
aria-controls={contentId}
157-
>
158-
<span className="text-text-secondary">{icon}</span>
159-
<h2 className="text-sm font-semibold text-foreground">{displayName}</h2>
148+
<h2 className="text-sm font-semibold text-foreground">
149+
<button
150+
type="button"
151+
onClick={() => { if (!forceOpen) setCollapsed((v) => !v) }}
152+
className={cn(
153+
'flex w-full items-center gap-3 p-card',
154+
'text-left transition-colors hover:bg-card-hover',
155+
)}
156+
aria-expanded={isOpen}
157+
aria-controls={contentId}
158+
>
159+
<span className="text-text-secondary">{icon}</span>
160+
<span>{displayName}</span>
160161
<span className="ml-1 text-xs text-text-muted">({entries.length})</span>
161162
<ChevronDown
162163
className={cn(
@@ -166,6 +167,7 @@ export function NamespaceSection({
166167
aria-hidden
167168
/>
168169
</button>
170+
</h2>
169171
)}
170172

171173
{isOpen && hideHeader && (

0 commit comments

Comments
 (0)