rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 19:01:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

ui: PathInput, and the media directory stops printing over the value

Browse source 
The five Special Directories fields on the Settings page were unreadable: the
media directory printed straight through the value, so Archive Directory showed
`a/media/wrolpi/hain_tag)s/%(domain)s`.

Cause: `leftSectionWidth='auto'`.  Mantine turns that prop into
`--input-padding-inline-start`, and `padding-inline-start: auto` is not valid
CSS -- it resolves to zero, so the input reserved no room for the section
positioned on top of it.  It fails only in CSS, only at runtime, and nothing
warns.

Fixed by expressing the field in the library instead of computing a width.
PathInput puts the prefix and the input side by side in a flex row sharing one
outline, so there is no width to get wrong and nothing to overlap -- the failure
is structurally impossible rather than merely corrected.  It also sizes to any
prefix, which matters because the media directory is `/media/wrolpi` on a Pi and
something else under Docker.

Written test-first, and the tests need no mocks at all -- no context, no fetch,
no router, just the provider test-utils already supplies.  That is the point:
Settings.js needs a pile of provider and API mocks, so its inputs were never
covered, which is how five broken fields shipped.  The tests assert the things
that actually matter and can be checked without layout: the prefix is a sibling
rather than a child, the value never contains it, onChange reports only what was
typed, the prefix is the field's accessible description so a screen-reader user
knows what the path is relative to, and disabled reaches the input rather than
just the wrapper.

Plus a source guard, since jsdom cannot see an overlap: nothing may pass
`leftSectionWidth='auto'` or `rightSectionWidth='auto'`.  It strips comments
first -- PathInput documents the mistake by name, and prose about a mistake is
not the mistake.
This commit is contained in:
Roland 2026-07-30 15:12:18 -06:00
parent 8ba73e64cd
commit 50cf89b20a
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
5 changed files with 272 additions and 16 deletions
Download patch file Download diff file Expand all files Collapse all files

9
app/src/components/ThemeSamplePage.js
View file

@ -16,6 +16,7 @@ import {
Modal,
Pagination,
Panel,
PathInput,
Placeholder,
Progress,
SearchBox,
@ -281,6 +282,14 @@ export function ThemeSamplePage() {
<div style={{marginTop: 12}}>
<Textarea label='Notes' placeholder='Notes about this download…' minRows={2}/>
</div>
<div style={{marginTop: 12}}>
<PathInput
prefix='/media/wrolpi/'
label='Videos Directory'
description="A fixed prefix beside the value, not layered over it."
defaultValue='videos/%(channel_name)s'
/>
</div>
<div style={{marginTop: 12}}>
<ActionInput
label="This WROLPi's URL"

26
app/src/components/admin/Settings.js
View file

@ -28,6 +28,7 @@ import {
Select,
ThemePicker,
toast,
PathInput,
TextInput,
} from "../ui";
@ -776,9 +777,8 @@ export function SettingsPage() {
position='top'
/>
</label>
<TextInput
leftSectionWidth='auto'
leftSection={<span style={{fontSize: '0.85em', paddingLeft: 4}}>{mediaDirectoryLabel}</span>}
<PathInput
prefix={mediaDirectoryLabel}
value={state.archive_destination}
disabled={!editSpecialDirectories}
onChange={e => handleInputChange('archive_destination', e.currentTarget.value)}
@ -804,9 +804,8 @@ export function SettingsPage() {
position='top'
/>
</label>
<TextInput
leftSectionWidth='auto'
leftSection={<span style={{fontSize: '0.85em', paddingLeft: 4}}>{mediaDirectoryLabel}</span>}
<PathInput
prefix={mediaDirectoryLabel}
value={state.videos_destination}
disabled={!editSpecialDirectories}
onChange={e => handleInputChange('videos_destination', e.currentTarget.value)}
@ -814,9 +813,8 @@ export function SettingsPage() {
</Grid.Col>
<Grid.Col span={{base: 12, sm: 6}}>
<label>Map Directory</label>
<TextInput
leftSectionWidth='auto'
leftSection={<span style={{fontSize: '0.85em', paddingLeft: 4}}>{mediaDirectoryLabel}</span>}
<PathInput
prefix={mediaDirectoryLabel}
value={state.map_destination}
disabled={!editSpecialDirectories}
onChange={e => handleInputChange('map_destination', e.currentTarget.value)}
@ -824,9 +822,8 @@ export function SettingsPage() {
</Grid.Col>
<Grid.Col span={{base: 12, sm: 6}}>
<label>Zims Directory</label>
<TextInput
leftSectionWidth='auto'
leftSection={<span style={{fontSize: '0.85em', paddingLeft: 4}}>{mediaDirectoryLabel}</span>}
<PathInput
prefix={mediaDirectoryLabel}
value={state.zims_destination}
disabled={!editSpecialDirectories}
onChange={e => handleInputChange('zims_destination', e.currentTarget.value)}
@ -834,9 +831,8 @@ export function SettingsPage() {
</Grid.Col>
<Grid.Col span={{base: 12, sm: 6}}>
<label>Playlists Directory</label>
<TextInput
leftSectionWidth='auto'
leftSection={<span style={{fontSize: '0.85em', paddingLeft: 4}}>{mediaDirectoryLabel}</span>}
<PathInput
prefix={mediaDirectoryLabel}
value={state.playlists_destination}
disabled={!editSpecialDirectories}
onChange={e => handleInputChange('playlists_destination', e.currentTarget.value)}

62
app/src/components/ui/Inputs.tsx
View file

@ -1,4 +1,4 @@
import React, {forwardRef} from 'react';
import React, {forwardRef, useId} from 'react';
import {
Checkbox as MCheckbox,
MultiSelect,
@ -58,6 +58,66 @@ export function Toggle({style, className, ...props}: ToggleProps) {
/>
}
export interface PathInputProps
extends Omit<React.InputHTMLAttributes<HTMLInputElement>, 'prefix'> {
label?: React.ReactNode;
description?: React.ReactNode;
error?: React.ReactNode;
/**
* A fixed, non-editable path the value hangs off — the media directory, say. It is
* not part of the value, and typing never changes it.
*/
prefix: string;
}
/**
* A path input with a fixed prefix shown to the left of what the user types.
*
* The prefix is a *sibling* of the input rather than a section layered inside it, which is
* the point: Mantine positions `leftSection` absolutely and reserves room for it with
* `--input-padding-inline-start`, so a call site that gets that width wrong prints the
* prefix straight through the value. Passing `leftSectionWidth='auto'` did exactly that
* — `padding-inline-start: auto` is invalid, so it resolved to zero and every one of these
* fields rendered its path on top of its own contents.
*
* As two boxes in a flex row there is no width to compute and nothing to overlap. It also
* sizes itself to any prefix, which matters here: the media directory is `/media/wrolpi`
* on a Pi and something else entirely under Docker.
*/
export const PathInput = forwardRef<HTMLInputElement, PathInputProps>((
{prefix, label, description, error, required, disabled, className, id, ...props}, ref
) => {
const generated = useId();
const inputId = id ?? `${generated}-input`;
const prefixId = `${generated}-prefix`;
return <div className={['wrolpi-path-input', className].filter(Boolean).join(' ')}>
{label && <label className='wrolpi-path-input-label' htmlFor={inputId}>
{label}{required && <span aria-hidden='true'> *</span>}
</label>}
<div className='wrolpi-path-input-control' data-disabled={disabled ? true : undefined}
data-error={error ? true : undefined}>
{/*
Described rather than hidden: a screen reader should hear which directory the
typed path is relative to, since that is the whole point of showing it.
*/}
<span className='wrolpi-path-input-prefix' id={prefixId}>{prefix}</span>
<input
ref={ref}
id={inputId}
type='text'
required={required}
disabled={disabled}
aria-describedby={prefixId}
{...props}
/>
</div>
{description && <div className='wrolpi-path-input-description'>{description}</div>}
{error && <div className='wrolpi-path-input-error'>{error}</div>}
</div>
});
PathInput.displayName = 'PathInput';
export interface ActionInputProps extends React.ComponentProps<typeof MTextInput> {
/** Button (or buttons) attached to the input's trailing edge. */
action?: React.ReactNode;

85
app/src/components/ui/ui.css
View file

@ -390,6 +390,91 @@ html[data-theme="amber"] .wrolpi-form-field input:focus {
background: var(--bg);
}
/* --------------------------------------------------------------- Path input */
/*
* Prefix and input are siblings in a flex row sharing one outline, so the prefix cannot
* print over the value -- the failure that Mantine's absolutely-positioned `leftSection`
* allowed when its reserved width was wrong. The prefix takes its natural width, so any
* media directory fits without a number being maintained anywhere.
*/
.wrolpi-path-input-label {
display: block;
font-weight: 600;
font-size: 13px;
margin-bottom: 4px;
}
.wrolpi-path-input-control {
display: flex;
align-items: stretch;
background: var(--panel);
border: 1px solid var(--border);
}
.wrolpi-path-input-control:focus-within {
border-color: var(--blue);
}
html[data-theme="night"] .wrolpi-path-input-control:focus-within,
html[data-theme="amber"] .wrolpi-path-input-control:focus-within {
border-color: var(--text);
}
.wrolpi-path-input-control[data-error] {
border: 1.5px dashed var(--red);
}
.wrolpi-path-input-control[data-disabled] {
background: var(--bg);
}
/* Muted and on the header surface: it reads as context, not as something to edit. */
.wrolpi-path-input-prefix {
display: flex;
align-items: center;
flex: none;
padding: 0 6px 0 9px;
background: var(--head);
border-right: 1px solid var(--border);
color: var(--muted);
font-size: 12px;
white-space: nowrap;
user-select: none;
}
.wrolpi-path-input-control input {
flex: 1;
min-width: 0;
padding: 7px 9px;
background: none;
border: none;
outline: none;
color: var(--text);
font: inherit;
font-size: 13px;
}
.wrolpi-path-input-control input::placeholder {
color: var(--muted);
}
.wrolpi-path-input-control input:disabled {
color: var(--muted);
}
.wrolpi-path-input-description {
font-size: 12px;
color: var(--muted);
margin-top: 3px;
}
.wrolpi-path-input-error {
font-size: 12px;
color: var(--red);
margin-top: 3px;
}
/* ---------------------------------------------------------- Action inputs */
/*
* Input and button share one outline: the input gives up its right border and

106
app/src/components/ui/ui.test.js
View file

@ -25,6 +25,7 @@ import {
Message,
Modal,
Panel,
PathInput,
Progress,
resolveIconName,
Status,
@ -280,6 +281,111 @@ describe('Header', () => {
});
});
describe('PathInput', () => {
/*
* These need no mocks at all -- no context, no API, no router. That is the point of
* putting the field in the library: the page that uses it needs a pile of provider and
* fetch mocks, so its inputs were never covered, which is how a field shipped printing
* its own prefix over its own value.
*/
it('keeps the prefix out of the value', () => {
// The bug this exists to prevent: the prefix must be its own element, never part of
// the text the user is editing, so it cannot be typed over or submitted.
renderUI(<PathInput prefix='/media/wrolpi/' label='Archive Directory'
value='archive/%(domain)s' onChange={jest.fn()}/>);
const input = screen.getByLabelText(/Archive Directory/);
expect(input).toHaveValue('archive/%(domain)s');
expect(input.value).not.toContain('/media/wrolpi');
// A sibling, not a child: an element inside the box could overlap it.
expect(input.querySelector('*')).toBeNull();
expect(screen.getByText('/media/wrolpi/')).not.toBe(input);
});
it('reports only what was typed, never the prefix', async () => {
// Read the value inside the handler: React nulls `currentTarget` once the event has
// been dispatched, so inspecting it afterwards from mock.calls throws.
const seen = [];
const onChange = jest.fn(event => seen.push(event.currentTarget.value));
renderUI(<PathInput prefix='/media/wrolpi/' label='Map Directory' onChange={onChange}/>);
await userEvent.type(screen.getByLabelText(/Map Directory/), 'map');
expect(seen).toEqual(['m', 'ma', 'map']);
seen.forEach(value => expect(value).not.toContain('/media/wrolpi'));
});
it('tells assistive technology what the path is relative to', () => {
// Sighted users get that from the prefix; hiding it would leave everyone else
// typing a relative path with no idea what it is relative to.
renderUI(<PathInput prefix='/media/wrolpi/' label='Zims Directory' value='' onChange={jest.fn()}/>);
expect(screen.getByLabelText(/Zims Directory/)).toHaveAccessibleDescription('/media/wrolpi/');
});
it('disables the input, not just the wrapper', async () => {
const onChange = jest.fn();
renderUI(<PathInput prefix='/media/wrolpi/' label='Videos Directory' disabled
value='videos' onChange={onChange}/>);
const input = screen.getByLabelText(/Videos Directory/);
expect(input).toBeDisabled();
await userEvent.type(input, 'x');
expect(onChange).not.toHaveBeenCalled();
});
it('marks an invalid path so the dashed danger treatment applies', () => {
const {container} = renderUI(<PathInput prefix='/media/wrolpi/' label='Bad'
error='Not a directory' value='' onChange={jest.fn()}/>);
expect(container.querySelector('[data-error]')).toBeInTheDocument();
expect(screen.getByText('Not a directory')).toBeInTheDocument();
});
it('forwards a ref to the input, for callers that focus it', () => {
const ref = React.createRef();
renderUI(<PathInput ref={ref} prefix='/media/wrolpi/' label='Focusable'
value='' onChange={jest.fn()}/>);
expect(ref.current.tagName).toBe('INPUT');
});
});
describe('no input reserves space for a section it cannot measure', () => {
it('nothing passes leftSectionWidth="auto"', () => {
/*
* `leftSectionWidth` becomes `--input-padding-inline-start`, and
* `padding-inline-start: auto` is not valid CSS -- it resolves to zero, so the
* section prints straight over the value. Five fields on the Settings page shipped
* that way and were unreadable. It fails silently, in CSS, at runtime only, which
* is exactly the kind of thing worth a source check.
*/
const offenders = [];
const walk = (dir) => {
for (const entry of fs.readdirSync(dir, {withFileTypes: true})) {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) walk(full);
else if (/\.(js|jsx|tsx)$/.test(entry.name) && !/\.test\./.test(entry.name)) {
// Comments are stripped first: the component that replaced this
// pattern documents it by name, and prose about a mistake is not the
// mistake.
const source = fs.readFileSync(full, 'utf8')
.replace(/\/\*[\s\S]*?\*\//g, '')
.replace(/^\s*\/\/.*$/gm, '');
if (/leftSectionWidth\s*=\s*['"`]auto['"`]/.test(source)
|| /rightSectionWidth\s*=\s*['"`]auto['"`]/.test(source)) {
offenders.push(path.relative(path.join(__dirname, '..', '..'), full));
}
}
}
};
walk(path.join(__dirname, '..', '..'));
expect(offenders).toEqual([]);
});
});
describe('ActionInput', () => {
it('joins the input and its action into one control', () => {
const {container} = renderUI(