refactor(ui): improve logs settings hierarchy and scoped controls

[caffeinated92]

Reorganize log settings into consistent top-level table sections with nested scoped groups, and replace bulky sliders with compact confirmable level buttons. This makes scoped logging behavior clearer while reducing visual noise and improving readability.

[caffeinated92]

@claude review

[claude]

Claude finished @caffeinated92's task in 2m 41s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze code quality, bugs, and best practices
• Post review feedback

---

Overall this is a solid, well-structured refactor. The hierarchy is cleaner, component separation is good, and accessibility attributes (aria-expanded, aria-controls, aria-labelledby, aria-pressed) are properly applied. A few issues to address below.

---

Bugs

1. Double space in confirm modal title

LogsSettings.jsx:291, LogsSettings.jsx:304 — the confirmTitle prop is passed with a trailing space inside the template literal (e.g., `Confirm change 'log-level-sql' to: `), and the modal renders it as:

// LogsSettings.jsx:184
`${confirmTitle} ${LEVEL_LABEL_BY_VALUE[pendingValue ?? currentValue]}`

This produces "Confirm change 'log-level-sql' to: Debug" — double space between the colon and the level name. Remove the trailing space from the confirmTitle prop strings, or remove the space before ${} in the template on line 184.

Fix this →

2. "Proxy Log Scopes" label appears twice

LogsSettings.jsx:403-407 — the outer TableType2 has a row with key: 'Proxy Log Scopes', and proxyOverridesContent renders a CollapsibleLogSection also titled 'Proxy Log Scopes'. The user sees the label twice — once as the outer table row header and again as the collapsible section header.

Either rename the inner section (e.g., 'Proxy Types') or flatten the proxy section to match the internalOverridesContent pattern (no nested single-section wrapper).

Fix this →

---

Code Quality

3. description field in INTERNAL_OVERRIDE_GROUPS is dead data

LogsSettings.jsx:33-85 — every group object has a description property (e.g., 'Controls orchestration, topology, and cluster lifecycle logging.'), but CollapsibleLogSection never receives or renders it. Either wire it up into the panel header (there's already a .panelDescription CSS class defined for this) or remove the field.

4. Redundant compact={false}

LogsSettings.jsx:289, LogsSettings.jsx:302 — compact defaults to false in ConfirmableLogLevelControl, so passing compact={false} explicitly adds noise. Remove both.

5. Loose equality check

LogsSettings.jsx:240:

const isSettingsDisabled = user?.grants['cluster-settings'] == false

Use strict equality (=== false) for consistency and explicitness. While the loose check happens to work here (since undefined == false is false in JS), strict equality makes the intent clear and avoids lint warnings.

6. Long className concatenation on line 167

The button inside the level gauge has a very long, hard-to-read className string built from 5 conditional parts. Consider adopting clsx (already a common dependency in React projects) to make this readable:

className={clsx(
  styles.logLevelGaugeButton,
  styles[`logLevelGaugeButtonLevel${option.value}`],
  isFilled ? styles[`logLevelGaugeButtonFilled${option.value}`] : styles.logLevelGaugeButtonUnfilled,
  compact && styles.logLevelGaugeButtonCompact,
  isSelected && [styles.logLevelGaugeButtonSelected, styles[`logLevelGaugeButtonSelectedLevel${option.value}`]]
)}

---

Unused CSS

styles.module.scss contains a number of classes that are defined but not referenced in the new JSX. These add dead weight to the stylesheet:

• .logsIntro, .logsIntroTitle, .logsIntroDescription
• .settingsCard, .settingsCardTitle, .settingsCardDescription
• .logsSectionStatus, .logsSectionStatusCustomized
• .settingRow, .settingRowCompact, .settingControl
• .panelDescription, .advancedRows, .settingsRows

If these were from the old implementation and are no longer needed, clean them up. If they're intended for future use, they shouldn't be committed yet.

---

Minor

• Hard-coded hex colors in SCSS: The level button color values (e.g., #ef4444, #facc15, #3b82f6, #8b5cf6) are inline hex values rather than CSS variables. If the project ever introduces dark/light theming for these specific colors, they'll need to be updated in many places. Consider extracting them as SCSS variables or CSS custom properties.

• buildSetSettingHandler recreated on every render: Not a functional bug, but since it's used inside mapSettingsToRows (also recreated each render), it means all ConfirmableLogLevelControl instances get new onConfirm refs each render cycle. This is fine currently since the child components don't use React.memo, but worth noting if the component tree grows.

---

[caffeinated92]

@claude review

[claude]

Claude finished @caffeinated92's task in 2m 37s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze code quality, bugs, and best practices
• Post review feedback

---

The latest polish commit (65f6e8a) addressed all major issues from the previous review: .trimEnd() guards the confirm modal title, the duplicate "Proxy Log Scopes" label was renamed to "Proxy Types", descriptions are now wired into CollapsibleLogSection, clsx replaced the long className concatenation, strict equality is used, redundant compact={false} props are gone, and hex colors are centralized into SCSS variables. Good cleanup.

A few remaining items worth addressing:

---

Bugs

1. Redundant isOpen on conditionally-rendered modal

LogsSettings.jsx:190-196 — ConfirmModal is only mounted when isConfirmModalOpen is true, so the isOpen={isConfirmModalOpen} prop will always equal true when the component exists. This works, but the pattern is inconsistent: either always mount the modal and use isOpen to show/hide it, or conditionally render it and drop the isOpen prop. Mixing both means the modal never sees isOpen={false} while mounted.

// Current (inconsistent)
{isConfirmModalOpen && (
  <ConfirmModal isOpen={isConfirmModalOpen} ... />
)}

// Option A: always mount, let isOpen control visibility
<ConfirmModal isOpen={isConfirmModalOpen} ... />

// Option B: conditional render, no isOpen needed
{isConfirmModalOpen && (
  <ConfirmModal ... />
)}

Fix this →

2. pendingValue ?? currentValue is always pendingValue inside the modal

LogsSettings.jsx:194 — The modal only renders when isConfirmModalOpen is true, which is only set after setPendingValue(nextValue) (lines 127–128). So pendingValue is guaranteed to be non-null in this context. The ?? currentValue fallback is unreachable and slightly misleading.

// current ${LEVEL_LABEL_BY_VALUE[pendingValue ?? currentValue]}`}

// clearer ${LEVEL_LABEL_BY_VALUE[pendingValue]}`}

Fix this →

---

Code Quality

3. External value update silently dismisses open confirm modal

LogsSettings.jsx:115-119 — The useEffect resets pendingValue and closes the modal whenever value changes. If the cluster config is refreshed via polling or a WebSocket push while the user has the modal open (about to confirm a level change), the modal will silently disappear with no feedback. Consider checking isConfirmModalOpen before resetting, or showing a toast/warning that the setting was updated externally.

4. Gauge button border colors show even when unfilled

styles.module.scss:534-548 — .logLevelGaugeButtonLevel{N} sets border-color for every level button regardless of fill state. .logLevelGaugeButtonUnfilled only overrides background and text color, not the border. So unfilled buttons (lower levels than the selected one) still show colored borders. If this is intentional (to hint at the level's color), it's fine — but if the intent is for unfilled buttons to look neutral, the border override belongs in .logLevelGaugeButtonUnfilled too.

5. buildSetSettingHandler and mapSettingsToRows recreated on every render

LogsSettings.jsx:262-269, 322-333 — Both are defined as plain functions inside the component body, so they produce new references on every render. All ConfirmableLogLevelControl instances get a fresh onConfirm callback each time. Not a functional bug today (children aren't memoized), but if component performance becomes a concern, useCallback + useMemo on these would stabilize the references.

---

Minor

• proxyOverridesContent wraps a single CollapsibleLogSection in Stack spacing={3} (LogsSettings.jsx:402-412) — the extra Stack wrapper adds no value when there's only ever one child. This could be simplified to render the CollapsibleLogSection directly, matching the simpler structure now that the outer label already distinguishes the proxy section.

• aria-label on collapsible button is redundant (LogsSettings.jsx:219) — aria-label={Toggle ${title}} is applied to a button that already contains visible text ({title} in the child Text). Screen readers will prefer the explicit aria-label over the visible text, which is fine, but consider using aria-labelledby pointing at the panelTitle element instead for cleaner semantics.

---

Overall the refactor is solid and the follow-up polish addressed all the previously flagged issues well. The hierarchy is cleaner, the level gauge component is well-structured with good ARIA support, and the SCSS is now properly organized with SCSS variables for colors.

[caffeinated92]

@claude review

[claude]

Claude finished @caffeinated92's task in 2m 22s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze code quality, bugs, and best practices
• Post review feedback

---

The latest commit (1add350) correctly resolved both bugs flagged in the previous review: the isOpen prop is no longer passed to the conditionally-rendered modal (now relying on ConfirmModal's isOpen = true default), and pendingValue ?? currentValue was simplified to just pendingValue. The UX concern about external updates dismissing an open modal was also addressed. Good fixes.

One remaining code quality issue is worth addressing; the rest are minor.

---

Code Quality

1. useEffect fires spuriously on modal open/close

LogsSettings.jsx:115-120 — isConfirmModalOpen is in the dependency array, so the effect re-runs every time the modal opens or closes — not just when value changes. On each of these re-runs, setCurrentValue(normalizeLogLevel(value)) is called with the same value, a no-op that still schedules an unnecessary state update and re-render of every ConfirmableLogLevelControl instance.

The isConfirmModalOpen guard's only job is to protect setPendingValue(null) from running while the modal is open. But closeModal() already clears pendingValue every time the modal is dismissed — making the guard in the effect redundant. The effect can be safely simplified:

// Current
useEffect(() => {
  setCurrentValue(normalizeLogLevel(value))
  if (!isConfirmModalOpen) {
    setPendingValue(null)
  }
}, [value, isConfirmModalOpen])

// Simplified — isConfirmModalOpen no longer needed here
useEffect(() => {
  setCurrentValue(normalizeLogLevel(value))
  setPendingValue(null)
}, [value])

The only scenario where this differs: value changes externally while the modal is open. With the current code the modal stays open (pendingValue preserved); with the simplified version the modal would close. But closeModal() handles cleanup correctly either way, and the simplified version has a more predictable dependency list.

Fix this →

---

Minor

• proxyOverridesContent unnecessary Stack wrapper (LogsSettings.jsx:402-411) — A Stack spacing={3} wraps a single CollapsibleLogSection. The wrapper adds nothing when there's only one child. Render the CollapsibleLogSection directly.

• aria-label on collapsible header button (LogsSettings.jsx:220) — aria-label={Toggle ${title}} is set on a button whose visible child Text already contains {title}. Screen readers will use the aria-label value over the visible text, which is fine — but the button also has aria-labelledby via the role/region pattern below it. Consider using just aria-controls + aria-expanded without the redundant aria-label, or pointing aria-labelledby at the panelTitle span for cleaner semantics.

• buildSetSettingHandler and mapSettingsToRows recreated on every render (LogsSettings.jsx:262, 322) — Still plain functions in the component body. Not a functional issue today (children aren't memoized), but useCallback/useMemo would stabilize refs if memoized children are added later.

---

Overall the refactor is in good shape and the three rounds of cleanup have addressed all the meaningful issues. The hierarchy is cleaner, ARIA is properly applied, SCSS is organized with variables, and the level gauge component is well-structured.