feat: DMX percentage display mode

[bbernstein]

Summary

• Add user preference to display DMX channel values as percentages (0.0%-100.0%) instead of raw DMX (0-255)
• Channel-type aware: discrete channels (GOBO, COLOR_WHEEL, EFFECT, MACRO, OTHER) always show raw DMX regardless of setting
• New conversion utility module (dmxPercentage.ts) as single source of truth for all DMX/percent conversions
• New useDisplayMode hook backed by localStorage for persisting the preference
• ChannelSlider performs all conversions at the display boundary — parent components and state management continue to work exclusively with raw DMX values
• Settings page gets a new "Display Preferences" section with a toggle switch
• useValueScrub gains a step parameter for fine-grained percentage scrubbing (0.1% steps)

Test Plan

• 27 unit tests for dmxPercentage conversion utility (boundary values, custom ranges, channel type classification)
• 4 unit tests for useDisplayMode hook (default, persistence, invalid values)
• 5 new ChannelSlider tests for percentage mode (continuum vs discrete display, input conversion, slider range)
• All 2748 existing tests pass (no regressions)
• Type-check and lint clean
• Manual: Toggle percentage mode in Settings, verify continuum channels show percentages
• Manual: Verify discrete channels (GOBO, etc.) still show raw DMX in percentage mode
• Manual: Verify preference persists across page refresh

🤖 Generated with Claude Code

[claude]

PR Review: DMX Percentage Display Mode

This is a well-structured feature with clean architectural decisions. The conversion boundary at ChannelSlider is the right place — parent components stay unaware of display mode, raw DMX flows through state management unchanged. Good test coverage (36 new tests) and SSR-safe localStorage handling. A few things worth addressing:

---

Issues

State Synchronization — Potential Bug

useDisplayMode is a plain hook (not context-backed). Each component that calls it gets its own isolated state. This means if ChannelSlider and SettingsPage are ever rendered simultaneously on the same view, calling setDisplayMode from Settings won't update already-mounted sliders until they remount.

For the current routing setup (Settings is a separate route), this works fine. But it's a fragile assumption. Consider either:

• Wrapping in a React Context so state is shared, or
• Adding a storage event listener inside useDisplayMode to react to cross-component changes:

useEffect(() => {
  const handler = (e: StorageEvent) => {
    if (e.key === STORAGE_KEY && (e.newValue === 'dmx' || e.newValue === 'percent')) {
      setDisplayModeState(e.newValue);
    }
  };
  window.addEventListener('storage', handler);
  return () => window.removeEventListener('storage', handler);
}, []);

Accessibility — Toggle Button Missing Label

The settings toggle has role="switch" and aria-checked but no aria-label or aria-labelledby. Screen readers will announce "switch, checked" without any context. The associated <h3> text isn't linked:

// Add aria-labelledby pointing to the h3
<h3 id="display-pref-label" className="...">
  Show values as percentages
</h3>
...
<button
  role="switch"
  aria-checked={displayMode === 'percent'}
  aria-labelledby="display-pref-label"
  ...
>

Shift Key Behavior — Asymmetric UX

In DMX mode, Shift = coarser (×10 steps). In percent mode, Shift = finer (×0.1 steps). This inversion will likely surprise users. The comment in dmxStep acknowledges this but the tooltip in ChannelSlider.tsx doesn't warn about it. At minimum, the tooltip text for percent mode should be clearer — or consider making Shift consistently mean "coarser" or consistently mean "finer" in both modes.

Float Precision Drift in useValueScrub

When step=0.1, this line accumulates floating point error:

accumulatedDelta.current -= steps * step;
// e.g., 0.3 - 3*0.1 = 0.30000000000000004 - 0.30000000000000004... 

Over many scroll events this can cause noticeable drift. Consider using parseFloat((accumulatedDelta.current - steps * step).toFixed(10)) or recomputing from a reference rather than accumulating diffs.

---

Minor Notes

• formatPercent and formatDmx are exported but unused — ChannelSlider doesn't import them (it uses inline toFixed(1) instead). Either import them for consistency or document them as utilities for future consumers. Not a bug, just a minor inconsistency.

• Touch sensitivity in percent mode — touchSensitivity was originally calibrated for a 0–255 DMX range. In percent mode the range is 0–100, so dragging feels ~2.5× more sensitive than before. Worth testing on touch devices to see if the existing sensitivity constant feels right.

• handleInputChange clamps via percentToDmx only — if a user types 150 in percent mode, it silently converts to 255 without resetting the input field to 100.0. The input max={100} attribute doesn't prevent this since onChange fires before browser validation. Low priority but worth noting.

• Test gap: useDisplayMode tests don't cover the case where localStorage.setItem throws (quota exceeded). The production code handles it gracefully, but a test would confirm the behavior.

---

Positives

• Clean single-source-of-truth in dmxPercentage.ts for all conversions
• Channel-type classification via Set<ChannelType> is efficient and easy to extend
• useValueScrub step generalization is well done — the precision calculation for clamp handles 0.1 step correctly
• The parseFloat(displayValue.toFixed(1)) in JSX prevents the input from showing 50.19607843... — good attention to detail
• All existing tests pass — no regressions

Overall this is solid work. The state sync issue is the most important to address before merge; the accessibility fix is straightforward and important for a11y compliance.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds an optional “percentage display” mode for DMX channel values, including a persisted user preference and UI/interaction updates so sliders can display/edit in percent while the underlying state remains raw DMX.

Changes:

• Introduces dmxPercentage utility functions (DMX↔percent conversion, formatting, step helpers) with unit tests.
• Adds useDisplayMode hook (localStorage-backed) with tests, and a Settings toggle UI to control it.
• Updates ChannelSlider to render/edit values in percent for continuous channels and extends useValueScrub with a configurable step.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/utils/dmxPercentage.ts	New conversion + formatting utilities and channel-type classification for percent display.
src/utils/__tests__/dmxPercentage.test.ts	Unit tests for DMX↔percent conversions, formatting, channel classification, and step helpers.
src/hooks/useValueScrub.ts	Adds step support for fractional scrubbing/quantization in wheel and touch interactions.
src/hooks/useDisplayMode.ts	New localStorage-backed preference hook for DMX vs percent display mode.
src/hooks/__tests__/useDisplayMode.test.ts	Tests for default mode, persistence, and invalid stored values.
src/components/ChannelSlider.tsx	Applies display-boundary conversion logic and percent-mode UI behavior.
src/components/__tests__/ChannelSlider.test.tsx	Adds coverage for percent mode behavior (continuous vs discrete, conversion, slider ranges).
src/app/(main)/settings/page.tsx	Adds “Display Preferences” section with a toggle switch for percent display mode.
.gitignore	Ignores .worktrees/.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The switch button has role="switch" but no accessible name (it contains no text and isn’t associated to the nearby heading/description). Add aria-label or aria-labelledby so screen readers can announce what the switch controls.

[Copilot]

In percent mode, useValueScrub emits changes at 0.1% granularity, but percentToDmx rounds to an integer DMX value. This means many scrub updates can map to the same DMX value, yet onChange(dmxValue) is still called repeatedly with an unchanged value (potentially causing redundant state updates/DMX sends). Consider short-circuiting when dmxValue === localValue before calling setLocalValue/onChange (and similarly for onChangeComplete).

[Copilot]

usePercent is based only on channel.type, but the channel object already carries isDiscrete (used elsewhere to indicate channels with multiple DMX ranges). As-is, channels like STROBE or other types that are flagged isDiscrete: true will still render as percentages in percent mode. Consider incorporating channel.isDiscrete into the decision (e.g., never use percent when isDiscrete is true) and add a test case covering a non-GOBO discrete channel.

Suggested change

	const usePercent = isPercentMode && isPercentageChannel(channel.type);
	const usePercent = isPercentMode && !channel.isDiscrete && isPercentageChannel(channel.type);

[Copilot]

handleInputChange returns early when the input parses to NaN. For a controlled <input type="number">, this makes it impossible to clear the field (empty string) and can interfere with intermediate edits, because the value snaps back immediately. Consider handling '' explicitly (e.g., treat as min/default) or keeping a transient string state for the input while editing.

Suggested change

	const rawValue = parseFloat(e.target.value);
	if (isNaN(rawValue)) return;
	const dmxValue = usePercent ? percentToDmx(rawValue, min, max) : Math.max(min, Math.min(max, Math.round(rawValue)));
	const { value } = e.target;
	// Treat an empty string as a request to reset to the minimum value
	if (value === '') {
	const dmxValue = min;
	setLocalValue(dmxValue);
	onChange(dmxValue);
	if (onChangeComplete) {
	onChangeComplete(dmxValue);
	}
	return;
	}
	const rawValue = parseFloat(value);
	if (isNaN(rawValue)) return;
	const dmxValue = usePercent
	? percentToDmx(rawValue, min, max)
	: Math.max(min, Math.min(max, Math.round(rawValue)));

[Copilot]

The new step support has two correctness issues: (1) step < 1 rounding uses -Math.log10(step) which only works for steps that are exact powers of 10 (e.g. 0.1) and will mis-round for steps like 0.25; and (2) step <= 0 will cause log10/division-by-zero problems in both clamp and touch quantization. Consider validating step > 0 and rounding/quantizing via a generic Math.round(val / step) * step (with appropriate floating-point handling) rather than decimal-place math.

[Copilot]

step introduces new behavior (fractional rounding, wheel accumulation thresholds, touch quantization), but there are no added/updated tests covering non-default step values. Since useValueScrub already has an extensive test suite, please add cases for step=0.1 (fractional output) and at least one non-power-of-10 step (e.g. 0.25) plus validation behavior for invalid steps (<= 0).

[claude]

Code Review

Overall this is a well-structured feature with a clean separation of concerns: a dedicated conversion utility, a purpose-built hook, and display-boundary conversions that keep parent state in raw DMX. The test coverage is solid. A few issues worth addressing before merging:

---

Bug: displayStep is defined but not reused in useValueScrub props

src/components/ChannelSlider.tsx lines 83 and 90:

const displayStep = usePercent ? 0.1 : 1;   // line 83
step: usePercent ? 0.1 : 1,  // line 90 - duplicates expression instead of using displayStep

displayStep should be referenced on line 90. As written, a future edit to one but not the other would introduce a subtle inconsistency.

---

Potential issue: isolated useState won't propagate across already-mounted components

useDisplayMode() stores its own useState internally, so each call site manages independent state. Toggling in the Settings page writes to localStorage but does NOT cause other already-mounted ChannelSlider components to re-render.

This works fine today because Settings and the slider views are on separate routes. But if a quick-settings toggle is ever added to the same page as the sliders, the preference will appear to update in the toggle but not in the sliders. A shared React Context or a storage event listener would be more robust.

---

Minor: floating-point accumulation in useValueScrub

accumulatedDelta.current -= steps * step accumulates FP error over many events, and Math.round(clamped / step) * step can produce values like 0.30000000000000004. At step=0.1 this rarely causes visible problems (the number input already rounds via toFixed(1)), but over repeated scrolling accumulatedDelta.current can slowly skew. A common fix is to round the leftover after subtraction.

---

Minor: handleInputChange now fires onChangeComplete when the field is cleared

Previously clearing the input did not trigger onChangeComplete (the commit-style callback that persists to the server). Now it does, which means clearing the field mid-edit fires a mutation resetting the channel to 0. Consider deferring onChangeComplete until blur or Enter, matching the mouse-up / touch-end semantics used elsewhere.

---

Minor: formatPercent and formatDmx are exported but unused in production code

Both helpers are well-tested but never imported outside the test file. The number input uses parseFloat(displayValue.toFixed(1)) inline instead. Either wire these into ChannelSlider for consistency, or remove the export if they are only for testing.

---

Minor: Shift-key step semantics are inverted between modes

• DMX mode: Shift+arrow = ±10 (coarser)
• Percent mode: Shift+arrow = ±0.1% (finer)

This is intentional and documented in the JSDoc, but a user who switches modes frequently may find it surprising. Worth considering whether percent mode should use Shift for ±10% instead, to keep muscle memory consistent.

---

Nit: hydration flash in Next.js SSR

useDisplayMode initialises to 'dmx' then updates in useEffect, which is the correct SSR-safe pattern. The trade-off is a brief flash where users with 'percent' stored will see raw DMX values on first render. Acceptable for a number preference, but worth noting.

---

Positive notes

• The conversion boundary is correctly kept at the component edge — parent state stays in raw DMX throughout.
• isPercentageChannel + channel.isDiscrete double-guard is solid defense-in-depth.
• role="switch" / aria-checked on the settings toggle is correct and accessible.
• 36 new tests with solid boundary-value coverage across utility, hook, and component layers.

Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This quantization assertion is ineffective: Math.round(newValue * 10) is always an integer, so % 1 will always be 0 regardless of newValue. Use an epsilon check like “newValue / step is close to an integer” so the test fails if quantization breaks.

Suggested change

	expect(Math.round(newValue * 10) % 1).toBe(0);
	const step = 0.1;
	const multiple = newValue / step;
	const epsilon = 1e-6;
	expect(Math.abs(multiple - Math.round(multiple))).toBeLessThan(epsilon);

[Copilot]

Same issue here: Math.round(newValue * 4) % 1 will always be 0. Adjust the test to verify newValue is a multiple of 0.25 within floating-point tolerance (e.g. compare against Math.round(newValue / 0.25) * 0.25).

Suggested change

	// Value should be quantized to 0.25 increments
	expect(Math.round(newValue * 4) % 1).toBe(0);
	// Value should be quantized to 0.25 increments (within floating-point tolerance)
	const quantized = Math.round(newValue / 0.25) * 0.25;
	expect(newValue).toBeCloseTo(quantized, 10);

[Copilot]

In percent mode, multiple adjacent display values (e.g. 50.2% vs 50.3%) can convert to the same DMX value due to rounding. The current dmxValue === localValue guard can be bypassed if events fire before React commits the state update (stale localValue in the closure), leading to repeated onChange calls with the same DMX. Consider tracking the last-emitted DMX value in a ref and comparing against that instead of localValue here.

[Copilot]

handleSliderChange always calls onChange, even if the computed dmxValue is unchanged (common in percent mode due to rounding collisions). Adding an early-return when dmxValue === localValue will avoid redundant state updates and prevent spamming parent updates/DMX output while dragging.

Suggested change

	const dmxValue = usePercent ? percentToDmx(rawValue, min, max) : Math.round(rawValue);
	const dmxValue = usePercent ? percentToDmx(rawValue, min, max) : Math.round(rawValue);
	if (dmxValue === localValue) return;

[Copilot]

Similarly, handleInputChange calls onChange/onChangeComplete even when rawValue converts to the same dmxValue as the current state (likely with 0.1% steps). Consider skipping updates when dmxValue === localValue to avoid redundant parent updates.

[Copilot]

clamp() currently ignores step values >= 1 and always Math.rounds, so callers using larger integer steps (e.g. step: 5 or 10) will not get quantization to that step size. Consider quantizing for all step values (e.g. round to nearest multiple of step) and only special-case step === 1 if needed.

Suggested change

	if (step >= 1) return Math.round(clamped);
	// Quantize to step size (works for any step, not just powers of 10)
	return Math.round(clamped / step) * step;
	// Determine an effective, positive step size with a safe default
	const effectiveStep =
	typeof step === 'number' && step !== 0 ? Math.abs(step) : 1;
	// Fast path for the common case: integer stepping of 1
	if (effectiveStep === 1) {
	return Math.round(clamped);
	}
	// Quantize to the nearest multiple of the effective step size
	return Math.round(clamped / effectiveStep) * effectiveStep;

[bbernstein]

Review Iteration Complete

All 6 Copilot review comments have been addressed in commit 4ca5e59:

1. Accessibility (settings/page.tsx) — Added aria-label to the display mode toggle switch
2. Redundant onChange (ChannelSlider.tsx) — Short-circuit when scrub maps to same DMX value
3. isDiscrete flag (ChannelSlider.tsx) — Incorporated channel.isDiscrete in percentage mode decision
4. Empty input handling (ChannelSlider.tsx) — Empty input resets to min instead of NaN early-return
5. Step quantization (useValueScrub.ts) — Replaced log10-based rounding with generic Math.round(val / step) * step
6. Missing tests (useValueScrub.test.ts) — Added tests for step=0.1, step=0.25, and default step=1

Threads to resolve manually:

• All 6 review threads (4 are already marked outdated by GitHub)

Status:

• All CI checks passing
• No pending reviewers
• PR is ready for merge

[claude]

PR Review: DMX Percentage Display Mode

Overall this is a clean, well-structured feature. The architecture is solid - keeping all state in raw DMX internally and converting only at the display boundary is the right approach. Test coverage is thorough.

Potential Bug: useDisplayMode State Not Shared Across Components

Each call to useDisplayMode() creates its own independent React state instance. SettingsPage and ChannelSlider both call the hook separately, so changing the setting in Settings will NOT propagate to already-mounted ChannelSlider components - they only read localStorage on mount.

In the current page-router setup this works in practice (navigating between Settings and the lighting controls causes a remount), but it is a fragile assumption. If the toggle ever appears on the same page as the sliders, the sliders will not react to the change. Consider adding a window storage event listener in the hook to keep all instances in sync, or lifting this into a React Context.

Minor: Redundant Inline Step Value

displayStep is computed on line 113 of ChannelSlider.tsx but the useValueScrub call on line 126 duplicates the same expression inline instead of reusing displayStep. Just pass step: displayStep.

Minor: Toggle Button Missing type=button

The toggle in settings/page.tsx does not specify type=button. While it is not inside a form today, adding it is defensive best practice.

Minor: Unintentional package-lock.json Change

The diff removes dev:true from the fsevents entry. This looks like an artifact of running npm install in a macOS vs Linux environment switch. fsevents is a macOS-only optional dependency and should retain its dev classification. Worth reverting this hunk if unintentional.

Positive Observations

• Clean boundary: raw DMX flows through all state/props/mutations; conversions happen only in ChannelSlider.
• dmxPercentage.ts utility: well-named, well-tested, single responsibility. 27 boundary-value tests are exactly right for a pure conversion module.
• useValueScrub step param: backward-compatible default of 1 means zero risk of regressions in existing callers.
• Discrete channel handling: dual guard (isDiscrete flag + isPercentageChannel(type)) is appropriately defensive.
• SSR safety: try/catch around localStorage access is correct for Next.js.
• Accessibility: role=switch, aria-checked, and aria-label on the toggle are all correct.
• Shift-key asymmetry (DMX: Shift=coarser; Percent: Shift=finer) is documented in the dmxStep JSDoc.

The useDisplayMode state isolation issue is the main one worth addressing before merging; the rest are minor polish items. Great feature overall.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

dmxToPercent is documented as returning a percentage in the 0.0–100.0 range, but it currently does not clamp the result. If dmxValue is outside [min,max] (or min/max are misconfigured), callers can get negative/over-100 values and formatPercent will render those. Either clamp the computed percent to [0,100] (recommended) or update the docstring to reflect that out-of-range values are allowed.

Suggested change

	return ((dmxValue - min) / range) * 100;
	const percent = ((dmxValue - min) / range) * 100;
	return Math.max(0, Math.min(100, percent));

[Copilot]

step is sanitized inside clamp (falls back to 1 when step <= 0 or non-number), but the wheel accumulation logic uses step directly as the threshold/divisor. If a caller passes step: 0 (or a negative/NaN), this becomes >= 0 and divides by 0, producing incorrect behavior. Use the same effectiveStep for the threshold/division and when applying steps * step, or normalize step once up front.

[Copilot]

Touch scrubbing quantization does Math.round(valueChange / step) * step without guarding against step <= 0 / non-finite values. Since clamp already normalizes invalid steps to 1, this path should also use the same normalized effectiveStep (or early-normalize step once) to avoid division-by-zero or NaN propagation.

Suggested change

	// Quantize to step size
	const steppedChange = Math.round(valueChange / step) * step;
	// Quantize to step size, normalizing invalid steps to 1
	const effectiveStep =
	Number.isFinite(step) && step > 0 ? step : 1;
	const steppedChange = Math.round(valueChange / effectiveStep) * effectiveStep;

[Copilot]

In percent mode with Shift held, percentStep returns 0.1, but handleKeyDown recomputes currentPercent from the current DMX value on every keypress and immediately converts back via percentToDmx. For most DMX values, adding 0.1% does not change the rounded DMX value, so newDmxValue equals localValue and the keypress becomes a permanent no-op (repeated presses never accumulate). Consider accumulating in display/percent space across key presses, or ensure a minimum 1-DMX change when the percent step would round back to the same DMX value.

Suggested change

	newDmxValue = percentToDmx(newPercent, min, max);
	let percentBasedDmx = percentToDmx(newPercent, min, max);
	// If the percent-based step would result in no DMX change (due to rounding),
	// fall back to a minimum 1-DMX step in the same direction.
	if (percentBasedDmx === localValue) {
	const fallbackDmx = Math.max(min, Math.min(max, localValue + direction * 1));
	newDmxValue = fallbackDmx;
	} else {
	newDmxValue = percentBasedDmx;
	}

[claude]

PR Review: DMX Percentage Display Mode

Overall this is a well-structured feature with good test coverage and a clean architecture (conversion at the display boundary, raw DMX everywhere else). A few things worth addressing:

---

Bug: useDisplayMode — no cross-instance state sync

Each component calling useDisplayMode() gets its own isolated React state. Changing the mode in the Settings page writes to localStorage but does not notify any currently-mounted ChannelSlider instances — they hold their own copy of the state.

This is fine in the current routing topology (Settings and the lighting page are separate routes, so navigation causes a re-mount that picks up the new localStorage value). But it's a hidden footgun: if these two ever appear on the same page simultaneously, the toggle silently won't work. Consider adding a comment explaining this assumption, or using a storage event listener + Context to make it bulletproof:

useEffect(() => {
  const handler = (e: StorageEvent) => {
    if (e.key === STORAGE_KEY && (e.newValue === 'dmx' || e.newValue === 'percent')) {
      setDisplayModeState(e.newValue);
    }
  };
  window.addEventListener('storage', handler);
  return () => window.removeEventListener('storage', handler);
}, []);

---

Bug: displayStep constant not reused in useValueScrub call

ChannelSlider.tsx defines:

const displayStep = usePercent ? 0.1 : 1;   // line 113

but then duplicates the literal one line below:

step: usePercent ? 0.1 : 1,   // line 126

Should be step: displayStep to avoid the two getting out of sync.

---

Behavioral change: empty-string input now fires onChangeComplete

Old handleInputChange:

const newValue = parseInt(e.target.value) || DEFAULT_MIN_VALUE;
// ... calls onChange but not onChangeComplete on empty

New code:

if (inputValue === '') {
  setLocalValue(min);
  onChange(min);
  if (onChangeComplete) onChangeComplete(min);  // ← new
  return;
}

Calling onChangeComplete when the user just clears the field (to type a new value) may send an unwanted DMX command to the backend mid-edit. Consider dropping the onChangeComplete(min) call in the empty-string case, consistent with the original behavior.

---

Shift-key behavior inverted between modes

In DMX mode: Shift = coarser steps (+10).
In percent mode: Shift = finer steps (+0.1%).

This is documented in the JSDoc, but the two tooltip strings don't make the inversion obvious. A user switching between modes will find Shift behaves oppositely. Is this intentional? If so, the tooltips could explicitly call it out ("Shift+arrow for ±0.1% fine adjustment" vs "Shift+arrow for ±10 coarse").

---

Minor: exported utilities not used in components

formatPercent and formatDmx are exported from dmxPercentage.ts but the slider uses parseFloat(displayValue.toFixed(1)) inline instead. If these helpers exist as the "single source of truth," consider using them in the component too, or drop them from the public API if they're only for tests.

---

Minor: SSR flash (minor / acceptable)

useDisplayMode initialises with 'dmx' and then updates in a useEffect after mount. On first render, a user with 'percent' stored will briefly see DMX values before the effect fires. Given this is a client-side-only preference (and Next.js with SSR would hit this), it's probably acceptable, but worth noting. Using useState(() => { try { return localStorage.getItem(...)... } catch {} return 'dmx'; }) (lazy initializer) would eliminate the flash entirely — but only safe without SSR.

---

Nitpick: package-lock.json noise

The "dev": true removal for fsevents looks like a lock-file regeneration artifact unrelated to this feature. Worth a clean npm install to confirm there's no unintended dependency promotion.

---

Positives

• Clean single-source-of-truth conversion utility with 27 well-structured tests
• Conversion at the display boundary is exactly the right pattern — parent state stays in raw DMX throughout
• useValueScrub step generalisation is done correctly with proper accumulation logic
• The isDiscrete flag override on top of channel-type classification is a nice escape hatch
• Hook tests cover the invalid-value guard case

The two bugs (state sync assumption and displayStep duplication) and the onChangeComplete behavioral change are the items I'd address before merging. The rest are polish.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

clamp() quantizes via Math.round(clamped / step) * step, which rounds relative to 0 rather than relative to min. If min is not an exact multiple of step, this can produce values outside [min,max] (e.g. min=5,max=6,step=2 -> returns 6 or 8). Consider quantizing relative to min (e.g. min + round((clamped - min)/step)*step) and then re-clamping to [min,max] to guarantee bounds.

Suggested change

	return Math.round(clamped / step) * step;
	const quantized = min + Math.round((clamped - min) / step) * step;
	return Math.max(min, Math.min(max, quantized));

[claude]

Code Review - feat: DMX percentage display mode

Overall this is a well-structured feature. The core design decision -- keeping raw DMX everywhere and converting only at the display boundary inside ChannelSlider -- is exactly the right approach. Good test coverage with 36 new tests. A few items worth addressing:

---

Critical: useDisplayMode state is not shared between components

useDisplayMode creates independent component-local state in each consumer. Both SettingsPage and ChannelSlider call the hook, each getting their own useState initialized from localStorage on mount.

If a user toggles the setting in Settings while ChannelSlider instances are also mounted, the sliders will not react -- they hold a stale copy of the preference. Changes only take effect after a full page reload.

The fix is a shared React Context. Wrap the app layout with a DisplayModeProvider that holds a single state instance, and have useDisplayMode read from Context instead of creating local state. All consumers will then re-render together when the mode changes.

---

Minor: displayStep computed but not reused in useValueScrub call

In ChannelSlider.tsx, displayStep is computed as (usePercent ? 0.1 : 1) and correctly used for the range input JSX, but the useValueScrub call duplicates the same ternary for its step prop. Use step: displayStep in both places.

---

Minor: Empty input immediately resets to min -- breaks type-while-clearing UX

In handleInputChange, an empty inputValue immediately fires onChange(min) and onChangeComplete(min). When a user clears "128" to type "200", this sends DMX 0 to the backend mid-keystroke. Consider deferring the value to onBlur instead.

---

Minor: Shift-key step semantics are inverted between modes

DMX mode: Shift+Arrow = coarser (+/-10). Percent mode: Shift+Arrow = finer (+/-0.1%). The tooltip is accurate per mode, but users switching between modes may find the reversal surprising. A brief comment explaining the deliberate inversion would help future maintainers.

---

Nit: Unintentional package-lock.json change

The diff removes "dev": true from fsevents. This looks like noise from a local npm install unrelated to this feature. Worth reverting.

---

What is working well

• Display boundary pattern is clean: raw DMX flows through all state management; conversion only happens inside ChannelSlider. Easy to audit and test.
• isPercentageChannel + isDiscrete dual-check correctly handles fixtures with a continuum type but discrete behavior via the isDiscrete flag.
• useValueScrub step normalization is good defensive coding.
• SSR-safe localStorage with try/catch in both get and set is correct for Next.js.
• Test coverage is thorough: boundary values, custom ranges, type classification, hook persistence, and slider rendering modes all covered.

The shared-state issue is the main architectural concern; the others are minor polish items. Great work on the feature overall!

[bbernstein]

All review comments have been addressed

Addressed across 3 fix commits (4ca5e59, 31c3fcb, 0c04823, ee2a444):

Round 1 (6 comments):

1. Accessibility: aria-label on settings toggle
2. Redundant onChange in scrub callbacks
3. channel.isDiscrete in percentage mode decision
4. Empty input handling (reset to min)
5. Generic step quantization in clamp
6. Fractional step tests added

Round 2 (6 comments — 4 were duplicates of round 1):
7. Test assertion fixes (tautological checks)
8. Short-circuit in handleSliderChange and handleInputChange
9. Quantize for all step sizes (not just < 1)
10. Shift+arrow no-op fallback to 1-DMX change

Round 3 (5 comments — 4 were duplicates of round 2):
11. Quantize relative to min in clamp (prevents drift outside bounds)

Threads to resolve manually:

All 17 review threads — most are outdated (code changed), all have been addressed.

Status:

• All CI checks passing
• No pending reviewers
• No new review comments on latest push
• PR is ready for merge

[Copilot]

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In percent mode, displayValue is derived from integer DMX (dmxToPercent(localValue)), but scrubbing/input operates in 0.1% increments. Many 0.1% changes round back to the same DMX (e.g., 75.0% -> 191 -> displays ~74.9%), which can cause the UI to “snap back” and can desync useValueScrub’s internal lastValue (it will advance in percent space even when localValue doesn’t change). Consider either (a) keeping scrubbing in DMX space (value=min/max=DMX, step=1) and only formatting as percent, or (b) storing a local percent editing state so display space can advance smoothly, or (c) choosing a percent step aligned to DMX resolution (e.g., 100/(max-min)).

[bbernstein]

This is an inherent tradeoff of displaying a 256-value integer as a percentage. The display will snap to DMX-aligned percentages because the underlying state is an integer — showing a smooth 50.1% when the real value is DMX 128 (50.196%) would be misleading.

The short-circuit guard (dmxValue === localValue) in the scrub onChange already prevents redundant parent updates. The useValueScrub internal lastValue tracks display-space values but the conversion boundary ensures only real DMX changes propagate.

Keeping scrub in DMX space (option a) would lose the percentage-scaled gesture feel. A local percent editing state (option b) would decouple display from reality. Both are more complex for marginal benefit. Skipping this change.

[Copilot]

The percent-mode tooltip claims “Shift+arrow for ±0.1%”, but the key handling explicitly falls back to a minimum 1-DMX change when 0.1% rounds to the same DMX value. In practice this often becomes ~0.4% steps (for 0-255 ranges), so the tooltip is misleading. Suggest updating the tooltip text to reflect the actual behavior (e.g., “Shift+arrow for finer control (min 1 DMX)”) or adjusting the key-step logic to truly apply 0.1% in a stable way.

Suggested change

	? 'Scroll to adjust, arrow keys for ±1%, Shift+arrow for ±0.1%. Touch and drag vertically to scrub.'
	? 'Scroll to adjust, arrow keys for ±1%, Shift+arrow for finer control (min 1 DMX). Touch and drag vertically to scrub.'

[claude]

Ignore - replaced by full review

[claude]

Ignore - replaced by full review (2)

[Copilot]

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code Review - Part 1 of 4

Overall this is a clean, well-structured implementation. The single-source-of-truth approach via dmxPercentage.ts and the display-boundary conversion pattern in ChannelSlider are solid architectural choices. Test coverage is thorough.

[claude]

Code Review - Part 2 of 4: Issues

Cross-component state sync (medium): useDisplayMode stores mode in plain useState + localStorage. Every ChannelSlider instance creates its own hook and initializes from localStorage on mount. If Settings is ever accessible without unmounting the sliders page (e.g. a future modal or sidebar), changing the toggle would NOT update already-mounted sliders. For the current separate-page navigation pattern this works fine, but adding a storage event listener in useDisplayMode would make all instances stay in sync without a remount.

Redundant step value (minor): displayStep is computed on line 113 of ChannelSlider.tsx, but the useValueScrub call on line 126 duplicates it inline as usePercent ? 0.1 : 1 rather than reusing the variable. Should use displayStep for consistency.

Toggle button missing focus styles (accessibility): The button role="switch" in the settings page has no focus: Tailwind variants in its className, so keyboard users will not see a visible focus ring. The rest of the settings page uses focus:ring-1 focus:ring-blue-500. The toggle should match.

Tooltip leaks implementation details (minor): The percent-mode title reads: "arrow keys for +/-1%, Shift+arrow for finer control (min 1 DMX)". The phrase "min 1 DMX" is an internal fallback detail that should not appear in user-facing text. Also the asymmetric Shift behavior (DMX mode: Shift = coarser +/-10; percent mode: Shift = finer +/-0.1%) may surprise users. Suggest: "Scroll to adjust, arrow keys for +/-1%, Shift+arrow for +/-0.1%. Touch and drag vertically to scrub."

[claude]

Code Review - Part 3 of 4: Observations

package-lock.json change: Removing "dev": true from fsevents is harmless but unrelated noise in the diff. Worth reverting.

Double discrete guard: usePercent = isPercentMode && !channel.isDiscrete && isPercentageChannel(channel.type) is correct. The !channel.isDiscrete guard handles the backend override case that isPercentageChannel (which only knows about channel type) cannot detect. The dedicated test for this is clear and good.

Empty input handling: Resetting to min on empty string is a nice UX improvement over the original parseInt() || 0 silent-floor behavior.

percentToDmx(50) returns 128: (50/100)*255 = 127.5 rounds to 128 via Math.round. Correct, just worth a brief comment in the test since 50% of 255 being 128 not 127 can surprise reviewers.

[claude]

Code Review

Overall this is a clean, well-structured implementation. The single-source-of-truth approach via dmxPercentage.ts and the display-boundary conversion pattern in ChannelSlider are solid architectural choices. Test coverage is thorough. A few items worth addressing:

ISSUES

Cross-component state sync (medium): useDisplayMode stores mode in plain useState + localStorage. Every ChannelSlider instance creates its own hook and initializes from localStorage on mount. If Settings is ever accessible without unmounting the sliders page (e.g. a future modal or sidebar), changing the toggle would NOT update already-mounted sliders. For the current separate-page navigation pattern this works fine, but adding a storage event listener in useDisplayMode would make all instances stay in sync without a remount. Suggested addition to useDisplayMode: listen for StorageEvent on window, check e.key === STORAGE_KEY and e.newValue, then call setDisplayModeState(e.newValue). Clean up on unmount.

Redundant step value (minor): displayStep is computed on line 113 of ChannelSlider.tsx, but the useValueScrub call on line 126 duplicates it inline as usePercent ? 0.1 : 1 rather than reusing the variable. Should use displayStep for consistency.

Toggle button missing focus styles (accessibility): The button role="switch" in the settings page has no visible focus ring for keyboard users. The rest of the settings page uses focus:ring-1 focus:ring-blue-500. The toggle should match.

Tooltip leaks implementation details (minor): The phrase "min 1 DMX" is an internal fallback detail that should not appear in user-facing text. The asymmetric Shift behavior may also surprise users. Suggest: "Scroll to adjust, arrow keys for +/-1%, Shift+arrow for +/-0.1%. Touch and drag vertically to scrub."

OBSERVATIONS

package-lock.json change: Removing "dev": true from fsevents is harmless but unrelated noise in the diff. Worth reverting.

Double discrete guard: usePercent = isPercentMode && \!channel.isDiscrete && isPercentageChannel(channel.type) is correct. The \!channel.isDiscrete guard handles the backend override case that isPercentageChannel cannot detect. The dedicated test for this is clear and good.

Empty input handling: Resetting to min on empty string is a nice UX improvement over the original parseInt() || 0 silent-floor behavior.

percentToDmx(50) returns 128: (50/100)*255 = 127.5 rounds to 128 via Math.round. Correct, just worth a brief comment in the test since 50% of 255 being 128 not 127 can surprise reviewers.

STRENGTHS

• Clean utility module with all conversions in one place as single source of truth
• Display-boundary-only conversion keeps parent components and Apollo cache working in pure DMX space
• useValueScrub step parameter is well-guarded (Number.isFinite and > 0) and quantization logic is correct
• 41 new tests covering boundary values, edge cases, and regression protection
• SSR safety: useDisplayMode initializes synchronously to 'dmx' and reads localStorage in useEffect, avoiding hydration mismatches

Generated with Claude Code

[bbernstein]

All review comments have been addressed

• All CI checks passing (11/11 SUCCESS)
• No pending reviewers (requested_reviewers is empty)
• Latest Copilot review: "generated no new comments" (reviewed 9/11 files)
• All review threads are resolved or outdated — threads that should be manually resolved:
  • settings/page.tsx: aria-label (addressed in 4ca5e59)
  • ChannelSlider.tsx: redundant onChange (addressed in 4ca5e59, 31c3fcb)
  • ChannelSlider.tsx: isDiscrete flag (addressed in 4ca5e59, outdated)
  • ChannelSlider.tsx: empty input handling (addressed in 4ca5e59, outdated)
  • ChannelSlider.tsx: scrub desync (replied with technical rationale — inherent tradeoff, not a bug)
  • ChannelSlider.tsx: tooltip text (addressed in 2e9f212)
  • useValueScrub.ts: step normalization (addressed in 0c04823, outdated)
  • useValueScrub.ts: clamp quantization (addressed in 31c3fcb, ee2a444)
  • useValueScrub.ts: fractional step tests (addressed in 4ca5e59)
  • useValueScrub.test.ts: assertion fixes (addressed in 31c3fcb, outdated)
  • dmxPercentage.ts: clamp output (addressed in 0c04823, outdated)
• mergeable_state is BLOCKED (requires human approving review)
• Confirm all checks are passing
• PR is ready for merge