The pins that fixed the blank React /map were removed as collateral when the
Mantine commit rewrote `overrides` for TypeScript 5. Nothing failed, so nothing
objected: the map kept working only because package-lock.json still happened to
resolve @babel/core to 7.20.12, and devices run `npm install`, not `npm ci`.
Verified on this tree rather than assumed. Raising @babel/core to 7.29.7 and
reinstalling floats the whole pipeline (traverse/generator/types to 7.29.8) and
`npm run build` then fails outright: "'opera_mobile' is not a valid target",
from a helper-compilation-targets that floated away from the browserslist data
it was pinned against. With the pins restored the build compiles, all 141
resolved copies match, and /map renders from the built bundle.
The test walks every path in the lockfile, not just node_modules/<name>: npm
nests a second copy whenever versions conflict, and a nested copy that escaped
the override would put the new pipeline back into the build while the top-level
entry still read correctly.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebasing onto master brought @testing-library/dom 8.20.0 -> 10.4.1, while
user-event stayed at 13.5.0. v13 targets dom v8 and reaches into internals v10
moved, so its keyboard simulation silently stopped dispatching: two SearchBox
tests failed with `Number of calls: 0` -- the arrow keys and Enter did nothing.
Test-harness only. @testing-library/dom never reaches a build, and no product
code changed in the rebase; the combination was simply never installed together
until master's bump met this branch.
v14's direct API is call-compatible with what the ten suites already do, so no
test needed rewriting -- the calls were already awaited.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
InfoPopup and HelpPopup default to `icon = 'info circle'` and `icon = 'help circle'`.
Neither name was in iconNameAliases, so both fell through to the fallback glyph and
logged an error -- and being defaults, they fired on every use of either helper.
Opening Settings on a device logged 43 of them.
The test that should have caught this was called "maps every icon name the app uses"
and checked a list of twenty names written by hand; these two were not on it. It now
reads the icon defaults off the source -- 85 of them -- and names the file it found an
unmapped one in.
Scanning `name='...'` as well was tried and reverted: `name` is overwhelmingly a
form-field attribute, so it reported 27 field names as unmapped icons and failed on
correct code.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The clear button sat detached from the filter field and 11px below it, and it
dragged the collapse-all button out of line with both.
One stale rule did all of that. `.search-clear { margin: 0.625rem }` is left
over from the Semantic-era filter, where the clear button was a sibling of the
input and needed the gap. It is now ActionInput's action, which joins the two
by dropping the input's right border -- so the margin reopened the seam the
component exists to close, pushed the button down by its own top margin, and,
because a flex line is as tall as its tallest item plus that item's margins,
stretched the wrapper to 62px around a 40px field. The collapse-all button
beside it was centred in that 62px, which is why it looked wrong too.
Removing the rule fixes all three: the button abuts the field and shares its
outline, like the filters on Videos and Archives, and the row is one height.
The class had exactly one user left, so the rule goes with it.
The test asserts the button is inside ActionInput's wrapper, and that no rule
reintroduces a margin on it -- jsdom applies no stylesheet, so the geometry
itself cannot be measured there. Confirmed it fails when the rule is put back.
* ui: the navbar shows which section you are in
Clicking Videos opened the videos page and left the bar looking untouched.
react-router was doing its part the whole time -- the tab carried `active` and
`aria-current="page"` on every one of those pages. Nothing in the stylesheet
drew that class, so all eleven tabs rendered identically and the mark was
visible only to a screen reader.
The current tab now takes a deepened version of the hover wash plus an
underline. The underline is a pseudo-element in `currentColor` rather than a
border, for two reasons: `currentColor` is the foreground the bar already
computed for whichever of the fourteen navbar colors the user picked, so one
rule covers every color in every theme without naming one; and a pseudo-element
adds no height, where a border would jog the whole row as the mark moved
between tabs. The overflow "More" menu and the mobile hamburger mark their
copy of the tab too -- on a phone that menu is where most tabs live.
Calculators and Statistics also lose `end: true`. A tab is meant to stay
current for the pages beneath it, and `end` restricts the match to the exact
path, so either one would have dropped its mark the moment it grew a sub-page.
The other nine links never had it, which is why /videos/1234 already kept
Videos marked.
The test renders the real bar, which needed two pieces of browser jsdom does
not have: fresnel renders no navbar at all unless a breakpoint query matches,
and every width is 0, which convinces the overflow logic that nothing fits and
sweeps all eleven tabs into the closed "More" menu -- where every assertion
about them would have passed vacuously. Both are stubbed, and the styling
itself is asserted against the stylesheet, since jsdom applies none.
* review: the mark follows the tab into the overflow menu
Six review points, no bugs among them. The substantive one: when the window
narrows enough to fold the current tab into "More", the bar went blank again --
the trigger is a plain button, not a NavLink, so nothing was marked until the
user opened the menu to look, which is exactly when they least need telling.
"More" now takes the mark when it holds the current section.
That needs the matching rule outside a NavLink, where the hooks that answer it
cannot be called in a loop, so `isLinkActive` states it once: a link owns the
pages beneath it unless it asks for an exact match. The trigger gets the class
only -- the real NavLink inside the menu already carries `aria-current`, and a
second one on the button that opens the menu announces the same page twice.
The other five, all fair:
* The mobile hamburger and the overflow menu had no test at all, so
`.mantine-Menu-item.active` -- the whole of the mark on a phone -- was
never exercised. Both now are, which meant splitting the viewport and the
bar width out of the fixture so a test can ask for a narrow bar or a phone.
The mobile case also needs the real getBoundingClientRect back: floating-ui
positions the menu's portal and crashes the worker on a stub DOMRect.
* The CSS guard matched the selector as a substring, which still passed with
the rule body emptied or the underline deleted -- and the underline is the
half that survives on a dark bar. It now reads the declarations.
* The table cases used `toContain`, which a regression leaving Home
permanently marked would satisfy on every row. `toEqual([text])`.
* A comment claimed the tab "only matched its own exact path", which is the
root cause the PR description explicitly rules out -- Videos never carried
`end`. Corrected to what those cases actually guard.
* The navbar offers twelve colors, not fourteen. I had carried over
Semantic's palette count, which includes black and white.
Writing the tightened height assertion I reached for `(?!0)`, which calls
0.1875rem zero. It reads the number instead.
* ui: sample data names Blender, not somebody's channel
The gallery, the card samples, and the search fixtures used a real YouTube
channel and one of its video titles as demo content. Sample data ships to
users and appears in a page linked from Settings; naming a real third party in
it puts words in their mouth -- the delete-confirmation sample in particular
read "The channel Wranglerstar and its download rules will be removed."
Blender fits better anyway: the repo's own test media is Big Buck Bunny, so the
sample library now refers to something it actually contains, and to a project
that publishes its films for exactly this kind of use.
* ui: the second sample channel is WROLPi's own
Same reason as the last commit: the remaining sample channel and its video
title belonged to a real person. The pair now reads as Blender and WROLPi,
which are the two things this project can name freely.
Acting on review of #644.
The largest finding came out of a suggestion rather than being in it. The
reviewer asked for coverage of the `color-scheme: dark` rule, on the grounds
that dropping it would leave night's track wrong with no failing test. Written
and passing -- then mutated, by pointing the selector at a type that does not
exist, and the tests stayed green.
Mantine's own stylesheet puts `color-scheme: var(--mantine-color-scheme)` on
`:root`, and ThemeProvider drives it with `forceColorScheme`. The property is
inherited, so a range slider on a dark theme has had a dark track all along and
the rule added here changed no pixel. It is deleted, and the claim about "the
UA's pale slab" is out of the comments and the gallery copy with it.
The tests stay, re-aimed at what actually supplies it: breaking
`forceColorScheme` fails all three, which is worth guarding because it lives in
another module and every accent assertion passes without it.
The other suggestions:
- The gallery slider misrepresented the control it stands for -- it invented an
"Error" step, dropped Trace and ran 0..4, where Settings maps 1..5 onto
Critical / Warning / Info / Debug / Trace. A gallery whose purpose is showing
the real control is worse than useless when it shows a different one. Keyed
to match `levelNameMap` exactly, and it starts at Info so both halves of the
track are visible.
- The "never leaves it at the UA default" test restated the four above it:
`toRgb` throws on anything unresolved, and the computed value of
`accent-color: auto` is the keyword, so those already fail on an unstyled
slider. Replaced with a constraint they do not cover -- `accent-color` is
inherited, so a text field beside the slider must still read `auto`, which is
what says the accent comes from this rule and not from a tinted document.
- The comment implied native checkboxes and radios are on screen the way
sliders are. They are not: those are Mantine's and Progress is a div, so
three of the four selectors are a guard rather than a fix. Said so.
Nits: four product sliders, not five -- the fifth is the sample itself. And
the progressive-enhancement limit is now stated where the rule is, including
what a complete fix would take.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A native `<input type="range">` is drawn by the UA, and with nothing said about
it the UA uses its own accent -- a fixed blue no theme can reach. Five are on
screen: the log level on Settings, and the duration sliders in the food, water
and ration calculators. None had ever been themed; `accent-color` appeared
nowhere in the app.
Night is where that is not merely inconsistent. The theme exists so a user
reading in the dark keeps their adaptation and every token is a red, so the
slider was the brightest non-red thing on the page -- on the one control a user
drags back and forth while watching it.
`accent-color: var(--blue)` for the same reason links take that token: it
remaps per theme, so night gets the dim red (#8a2828) and amber the amber
(#a86618) with no second rule. Checkboxes, radios and `<progress>` are
included because they are painted from the same accent and would have been the
next report.
`accent-color` reaches the thumb and the FILLED half of the track and nothing
else, so on a dark ground the rest stayed the UA's pale slab. `color-scheme:
dark` fixes that, scoped to these controls rather than declared on `html` --
on `html` it also governs scrollbars, spinners and every other piece of browser
chrome, which is a larger change than this one and worth making deliberately.
Tests assert the accent equals `--blue` in each of the four themes, plus a
negative: in the monochrome themes it is a resolved color and never `auto`.
Without that second one the first four would also pass on an unstyled slider
whose accent happened to stringify the same way.
Added to the gallery, which is how the monochrome themes get looked at. Its
slider is stateful and starts mid-track: the accent only shows on the filled
half, so one parked at either end would show almost none of the color the
section exists to check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The whole panel is wrapped in a `<Link to='/admin/status'>`, so the block is one
anchor -- and `a` takes `color: var(--blue)` with an underline on hover from
tokens.css. `.wrolpi-statistic-value` sets no color of its own, so the three
load figures inherited the link color: blue numbers on a panel where nothing
else is blue, underlining as a group whenever the pointer crossed any part of
it. The Downloading figure below the divider had it too.
`no-link-underscore card-link` is the pair already used wherever a link IS the
element's own text rather than a link through it -- every card title and meta
line, via `cardLinkClass` in Common.js. Reused rather than adding a third
class for the same idea.
The pointer cursor is untouched, which is the point: it comes from the UA
stylesheet for `a[href]`, and the affordance is the only reason to wrap a panel
in a link at all. A test asserts it so a later change cannot take it away
while tidying the rest.
DashboardStatus is exported so its call site can be rendered directly; the
colors are asserted in cypress, since jsdom drops `color: var(--text)` as an
invalid declaration and cannot see this at all.
Each new assertion has a paired negative: the same panel WITHOUT the classes
comes out blue, and has no rule protecting it from an underline. The first
draft of the underline test checked only that no matching hover rule was
`!important`, which was true before the fix as well -- it would have passed on
a panel that still underlined.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cypress component suite has been failing 12 of 311 for long enough to read
as background noise. Every one of them was the test's own fault, and four
distinct causes account for all twelve.
Dead selectors. `div.ui.label.large`, `div.divider.text`, `.item` and
`have.attr 'error'` all named markup the old library rendered. None of them
started failing when the library was replaced -- they went on matching nothing,
or looking for an attribute no control carries, which is the failure mode that
makes a test look broken rather than the code. A tag chip is `.wrolpi-tag`, a
Select is a real input whose value can be read, an option carries the ARIA role,
and an invalid field says `aria-invalid`. Both directions of that last one are
asserted by value: a valid field says `aria-invalid="false"` rather than
dropping the attribute, so `not.have.attr` would have passed on absence.
Missing context. `useFileWorkerStatus` throws rather than defaulting when its
provider is absent, so the channel edit page died on a modal buried three levels
down while every other page mounted fine. App.js wraps the whole app in that
provider; the mount helper now does too, with the poll slowed to a minute.
Unstubbed fetches. The inventory page loads the food catalog from its own
endpoint, and the RSS edit form fetched downloaders before its intercept was
declared. Both rejected with "Failed to fetch" out of an effect, which Cypress
counts as an uncaught application error and fails the running test on -- so
three inventory tests and one download test died for a reason unrelated to
anything they assert.
Assertions reading the wrong thing. `cy.contains('Food Storage')` found the
hidden `.inventory-print` copy, since contains takes the deepest match and the
print sheet is mounted at all times. And the PUT body arrives as a JSON string,
so `req.body.items` was undefined both in the reply the interceptor built and in
the assertion waiting on it. The RSS submit test deep-equalled one exact JSON
string and broke the moment the video downloader gained eight settings the form
fills in by default; the envelope is still matched exactly and the settings are
checked for the values the fixture set. It also never waited on its own alias,
so an interceptor that never ran would have passed in silence.
Separately, the navbar's shrink test read its link count in a bare `.then()`
right after the corner finished growing -- before the recalculation that growth
triggers had run, so `shrunken` was sometimes already the full count and the
assertion came out "expected 10 to be above 10" under load. It now waits for
the bar to settle to one row, which is the same signal the test above it
asserts, and requires the "More" trigger to exist so it cannot pass having
measured a bar that never overflowed.
New: two tests for the file browser filter's width, desktop and phone, which is
what caught the dead selector behind it.
311 passing, 0 failing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The library has been uninstalled for some time, but the codebase still read as
though it were there: identifiers named after it, rules written against its
class names, and about 180 comments explaining a rule in terms of a thing that
is no longer installed.
Renamed, keeping the vocabulary the app actually uses: semanticUIColorMap ->
navColorHexMap, semanticColorNames -> paletteColorNames, semanticNames ->
iconNameAliases, semanticSizes -> sizeAliases, and SemanticUIColors ->
NavColors in wrolpi/schema.py, which is what validates nav_color on the way in.
ColorToSemanticHexColor had no callers at all and is gone.
The baseline ratchet has done its job -- the list reached zero, so the JSON is
deleted and the guard keeps only the checks with teeth: no package, no import,
no `className="ui "`, no toast library. Its Theme.tsx check went with the
wrappers it was watching for. It also asserts its own walk found files, so a
clean result cannot mean it scanned nothing.
Deleted from App.css: every rule requiring `.ui`, `.inverted`, `.segment` or
`.modal`, none of which any component has set since the stylesheet stopped
loading, plus the form skeletons, ui-alerts and progress-text rules that
nothing renders. Two of them were doing harm rather than nothing: the drag
tints had a dark-theme variant under `.inverted` that stopped applying, leaving
15% of a fixed blue over a dark row, and now mix a token instead.
Two selectors naming markup that no longer exists were live bugs. The file
browser's filter had `.ui.input.file-browser-filter`, so it rendered at the
browser default width on a bar laid out to give it 34em -- now qualified by its
container, because `.wrolpi-action-input` sets `width: 100%` at the same
specificity and is loaded later. And SearchBox takes named props rather than a
rest spread, so the `id` DestinationForm hands it was dropped in silence and
the label it renders pointed at nothing; the download forms' Destination field
has had no associated label since the migration.
"Semantic roles" survives in four places, where it means what it says.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five findings, no bugs, all valid.
`.wrolpi-or-split > *` still laid out the label, the same structural pitfall the
border rule was already fixed for -- the word escaped the half's 1.5rem padding
on source order alone, same specificity and declared later. Excluded now, and
the test asserts the word's padding is SMALLER than a half's rather than merely
greater than zero, which is what fails when the two blocks are reordered.
The night "never the brightest thing" assertion read `html`'s computed
background. Tokens paint `--bg` on BODY, so that came back `rgba(0, 0, 0, 0)`
-- luminance zero -- making the check "no brighter than black, plus slack". It
did fail the white chip, so it caught the reported bug, but by accident of the
comparison rather than by measuring the page. Reads the `--bg` token now.
The video inset was only ever asserted above the breakpoint; the phone branch
was commented and unmeasured. Worse than it looks, since cypress defaults to a
500px viewport and that is the branch everything else in the suite runs in. It
now asserts zero padding and that the panel really reaches the edge.
The duration chip's border went unasserted -- and the border is what separates a
`--panel`-colored chip from a poster near that color, so losing it left every
contrast assertion green. Asserted on all four themes, against `--border`.
And `Group` was left imported by the dashboard after the panel stopped using it.
All three new assertions verified against the mutation each describes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two fixes to the divider from review.
No disc. The rule runs down, stops at the word, and picks up below it -- the
break made by painting the word with `--panel` rather than by drawing a shape
around it. So the label carries no border and no radius, only enough padding to
open the gap.
And a stray 1px line down the word's own left edge. The label is a third child
of the grid -- absolutely positioned, so it takes no column -- but `> * + *`
still matched it and gave it a border of its own, beside the one running behind
it. Both the divider rules exclude it now.
The test that was meant to catch that read `borderTopWidth` and nothing else, so
a rule on the left survived an assertion named "no outline around the word". It
reads all four sides, and fails on the Left edge against the old selector.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Download/Or/Upload panel. Semantic drew it as a `Segment placeholder` with
a vertical `Divider`: two equal fields, a rule down the true middle, a button
centred in each. The migrated version was a `Group` with a divider between two
buttons, so the halves were sized to their labels -- the pair huddled in the
middle with the rule wherever they left it. `.wrolpi-or-split` is a grid, so
the halves stay equal whatever the labels say, and the rule is the second
column's border rather than an element between them (an element would be a third
grid item and push the middle off centre by its own width). Below the tablet
breakpoint the halves stack and the rule turns horizontal, as Semantic's
`stackable` grid did.
The video page's panels touched the left edge. Its route is deliberately
outside `VideosTabLayout`'s `PageContainer` so the player can run to both edges
-- which is the most viewing area a phone can give it, and worth keeping -- but
everything below the player inherited that too. `.wrolpi-page-inset` gives the
stack under the player the same inset `PageContainer` gives a page, at the same
breakpoint, so a phone still has no side padding anywhere.
A video's duration chip was a literal `#ffffff` with black text. The poster
under it IS filtered by night mode, being a `.media` leaf, but the chip is drawn
beside the image rather than inside it, so the filter never reached it: a white
tile on a card whose picture had gone dim red, and the brightest thing on the
videos page in the theme whose whole point is that nothing is bright. Tokens
now, opaque because it sits on an arbitrary poster.
Nine guards, all verified to fail against the old code. One thing they turned
up: cypress component tests default to a 500x500 viewport, which is BELOW the
699px breakpoint, so viewport-keyed CSS is exercised in its phone branch unless
a test says otherwise. The desktop assertions set the viewport explicitly and
say why; the stacked branch has a test of its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three review points on the border removal.
The width half of "the size it was" read `style.minWidth` back, which is the
same string whether or not the box it produces is right -- a content-box
regression would have passed it. The bar is now mounted in a 40px wrapper so
the min-width floor is exercised by layout and `getBoundingClientRect` measures
it; reverting to 5rem fails all four themes.
"`--head` is off both surfaces in every theme" overstated the monochrome case:
amber is 1.11 against a Panel and 1.04 against the page, night 1.05 and 1.01.
Amber can afford a brighter track and now takes `--table-line` (1.34 / 1.43).
Night cannot -- the label crosses the empty track and measures 5.25 at `--head`,
4.22 at `--table-line`, 3.42 at `--border`, so every track with a visible edge
puts the label under 4.5. That is recorded as a deliberate near-flat track
rather than described as an edge it does not have.
And the single `> 1.05` floor had no slack in night, where the real ratio is
1.053, so it asserted "not the same color" while reading like a visibility
claim. Each theme is now held to its own measured floor, against the page as
well as the Panel -- bars land on both -- with night's 1.04 / 1.005 naming what
that theme really does.
Take the 1px outline off the bar without changing its size. `box-sizing:
border-box` is global and the declared 1.375rem / 7.5rem were always the outer
box, so nothing moves.
What the border was quietly doing was giving the empty half of the track an
edge, so the track now has to carry that itself: `--head` rather than `--bg`.
A bar sits on a Panel as often as on the page -- Status and Downloads put them
in Panels and table rows, uploads in modals -- and `--bg` is invisible against
one of those two by definition, which is also why light mode needed its own
`--panel` override. `--head` is off both surfaces in all four themes, and
keeps the label's contrast against the empty track above 4.5:1 everywhere,
which a lighter track (`--border`, 3.4:1 in night) would not.
Asserted per theme in the component spec: no border on any edge, the height
and min-width measured against `rem` rather than px, and the track distinct
from the Panel beneath it. Putting the border back fails all four.
It asserted the label is block but not the 4px gap, so dropping the
margin -- the half that actually lines an InputForm up with the
DestinationForm beside it -- would still have passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
InputForm returned a fragment, so its label and its field arrived as two
children of whatever contained them. In a grid that is two cells: on
/videos/settings the "Video File Format" and "Extra yt-dlp Arguments"
inputs sat in the column to the right of their own labels.
The label is now block with the same 4px gap the migrated fields use
(FieldLabel in Download.js), so an InputForm and a DestinationForm side
by side start their inputs at the same height -- the Channel Name and
Directory fields on the Channel edit page were off by the difference.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five findings, all valid. The substantive one: the video page's Download /
Delete / Refresh / AddToPlaylist strip was still four bare buttons in a `<p>` --
the direct analogue of the archive row this PR started with -- and the scan that
was supposed to catch exactly that reported the app clean.
It only ever saw `{fooButton}` interpolations, and most of the app writes its
buttons out in place. Scanning for adjacent button ELEMENTS finds twelve more
rows in the same state: reorder icons in the playlist and field-schema tables,
both service tables on the controller page, its hotspot Save/QR pair and its
Restart/Shutdown pair, the downloads footer, the one-time pad's Generate/Cheat
Sheet, and the ration calculator's CSV/Print. All rowed.
Two holes had to be closed before that scan could be trusted, because a scan
that reports noise is worse than one with a stated limit:
- `tagsIn` never captured the tag NAME, so `ROW_COMPONENTS.has(undefined)` was
false for every tag and the list of components that lay out their own
children -- Group, ButtonGroup, Modal.Actions -- silently accepted none of
them. 56 false positives, every modal footer in the app among them.
- A row whose style is a const (`style={seasonRowStyle}`) says nothing in its
own attributes. Resolved now; the dates selector's seasons row is a
four-column grid with a gap and was reported three times.
Also from review: the row body is found by walking to the MATCHING close rather
than the first `</div>`, which one nested div would have truncated -- and the
collection form now nests one, so this was about to matter. Cancel and Save
travel together in a row of their own pushed to the end; `margin-left: auto` on
Save alone was right for a row that could not wrap, and would now drop Save onto
a second line by itself with Cancel left among the action buttons. The doc
page's comment no longer implies the picker's 1em separation was not a change.
One more found on the way: the file browser's filter row is flex with no gap, and
its collapse-all button carried `margin-left: 0.5em` for it. The gap is the
container's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported: the Back button fills the page width on the domain and channel edit
pages. It fills it on all nine pages that have one, and this is mine from the
last PR.
`.wrolpi-stack` is a flex column at `align-items: stretch`, deliberately -- a
panel was full-width as a block element and has to stay that way. A BARE button
as a direct child of that stack is stretched too, and every page's Back button
was one. On the domain, channel and playlist edit pages it also pushed the
button beside it onto its own line, where the two had been inline.
Wrapping each in a `.wrolpi-button-row` fixes both without touching the stack:
the row is the stretched child, and it lays its buttons out at their own width.
The three pages that have a second button get one row holding both, which is how
they read before.
Also the Videos, Archives and Documents select bars, whose Tag/Delete/Invert/
Clear were four touching buttons in a bare div. They are inline elements rather
than interpolations, which is the shape the bare-siblings scan does not see.
Guarded twice: the row keeps its buttons their own width inside a stack while
itself stretching (both halves, since a row that refused to stretch would pass a
test that only looked at the button), and every `<BackButton/>` in the app is
inside a row.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The archive page's six actions shared edges, and Update sat lower than the
rest. Both come from the same thing: Semantic's Button carried
`margin: 0 .25em .25em 0`, so a page could list buttons as bare siblings and
they arrived spaced -- and separated from the line below when the row wrapped.
Nothing replaced that margin.
Two of those six answered with `marginTop: 0.5em`, which is the wrong axis: a
Button is `display: inline-flex; vertical-align: middle`, and `vertical-align`
is measured on the MARGIN box, so half of any top margin shows up as the
visible button dropping. It was covering the wrapped-row case in the only axis
a margin can, and paying for it in the other one.
`.wrolpi-button-row` is a flex row with a gap, so both axes are covered at once
and no button needs a margin of its own -- the same rule as `.wrolpi-stack`,
along the other axis. Applied to the five rows in the app that had no gap: the
archive page, the shared collection edit form (which three pages hand action
buttons to, and where nine buttons plus Cancel carried `margin-top: 1em` each
while Save carried none), the doc page's two responsive branches, and the
downloads table's edit/restart pair.
`.action-button-spacing` goes with them; it was that same margin, on Cancel.
Guarded four ways. ui-layout.cy.js measures that the row really spaces what it
holds, wraps rather than overflows, centres its buttons against a taller item
beside them, and counts a confirmed APIButton as one item rather than two --
that one returns a fragment holding a `<Confirm/>`, which would open a hole in
the row if it did not portal. Common.test.js scans for the two ways this comes
back: a button in a row stamping a vertical margin, and a row of buttons that is
not a row. Neither throws, which is how the app got here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review was right that completeness was not done. Every finding held up.
Three more pages mounted a `PageContainer` inside a route that already
provides page chrome -- `ManageMap`, `MapPins`, `ManageZim` -- so they sat a
further 1em down and 1em in from the tab bar than the viewers beside them.
The same defect as ArchiveSettings, in a shape the scan could not see: it
keyed on `PageContainer` route wrappers, and Map and Zim wrap their content
in a `wrolpi-stack` div instead. It now locates routers first and asks
whether the enclosing return provides chrome either way, resolves
`element={<ErrorBoundary><Page/></ErrorBoundary>}` as well as the bare form,
and knows about `class` components -- `ManageZim` is one, and a scan that
only knew functions reported it clean.
Five top-level blocks still added their own margin to the stack's gap: the
dashboard search row (52.8px against 17.6px, on the most-looked-at page in
the app), the Playlists filter row, both Docs filter rows, and the Map pins
filter. The extension page's hint panel had the same compound against a
Mantine Stack's gap.
ControllerPage held its four panel sections in a plain wrapper, so the
admin page's stack saw one child and the panels kept sharing edges -- the
exact hole Settings had.
The dashboard was reported clean in the first pass and was not. The sweep
measured wrapper-to-wrapper gaps, and that row's margin is INSIDE a Media
wrapper, so it never appeared: the measurement, not the page, was at fault.
Cypress gap assertions now derive from the stack's own `row-gap` instead of a
12-28px range that assumed the default interface scale and would have
false-failed a correct layout at a larger one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ArchiveRoute` wraps its `<Routes>` in a PageContainer, and
`ArchiveSettingsPage` wrapped itself in another. PageContainer carries
`margin-top: 1em` and `padding: 1em`, so that page sat 1em lower and 1em
further in on all four sides than /archives/domains beside it -- which is
exactly what "seems to have more margin" was. It is a fragment now, so its
panels are blocks of the route's own stack.
Guarded by a source scan rather than a render, since a doubled margin throws
nothing: it takes each PageContainer that wraps a router, reads the
components it routes to, and fails if one of them renders a PageContainer of
its own.
The first version of that scan passed against the live bug. It looked at
only the first `<PageContainer>` in a file, and in Archive.js that is the
offender itself, whose block holds no `<Routes>` -- so the file was skipped
before the route wrapper 400 lines below was reached. It reads every
occurrence now, and the premise assertions count both the wrappers found and
the routes within them.
Two more top-level blocks gave up margins that were being added to the
page's gap, both headers "matching" each other's pattern: the domain filter
row (35.2px against 17.6px) and the channel filter row.
/archives/settings and /archives/domains now measure identically -- tab bar
bottom at 136.8px, first block 17.6px below it, content inset 17.6px -- as
do the statistics and settings pages of every other tab group.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Panels shared edges: two 1px borders meeting, which reads as one tall
surface with a rule across it rather than a seam between two.
Done as `margin-bottom` on the panel first, which is the anti-pattern, and
it failed in both of the ways that rule is known to fail before the reading
was done. It compounded with the gap of every Stack that already held
panels -- four of them, spacing those lists at 35.2px against the app's
17.6px -- and the `:last-child` reset meant to keep trailing space off the
bottom of a container silently did nothing, because Media wrapping makes a
panel the only child of its own wrapper. On Status that separated four
pairs out of five and left the first touching.
So the space belongs to whatever stacks the blocks. `.wrolpi-stack` is on
PageContainer, which every page uses, and on the three wrappers that stack
blocks somewhere the page's own stack cannot reach: Settings' panel wrapper,
the block below the video player, and the Zim and Map route wrappers, which
hold their tab bar outside PageContainer.
A gap rather than `> * + * { margin-top }`, which was tried and is quietly
broken here: Media injects `.fresnel-container { margin: 0; padding: 0 }` at
runtime, same specificity, later source order, so every wrapped block threw
the margin away. A child cannot zero its parent's gap. It also means
Media's hidden half contributes nothing, where the owl selector spaced
around something invisible.
With the page owning the spacing, the blocks that owned some of it had to
give it up, or their margin was added to the gap: the tab bar (33px against
17.6 everywhere else), the shared search control bar and paginator on
Videos/Archives/Docs/Files, the card grid, the file browser's filter row and
the inventory search. Two margins deliberately stay -- the file browser's
4em, which reserves room for the sticky buttons (62645831), and heading
margins, which bind a heading to the content below it.
Tests plant each of the three failures: the wrapped block, a child that
zeroes its own margin, and a hidden Media half. Verified in the browser on
sixteen routes and in all four themes -- every stacked pair 17.6px, the
map's 85vh box and the video player unchanged, no horizontal overflow.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two review notes, no bugs.
The geometry numbers disagreed with each other: I fixed one of the two
"17.3px" mentions and left the other, and the figures were rendered px
while ui.css states the same margin in design px, so three comments told
three stories about one value. All of them now say 18.7px rendered and
cross-reference the 1.0625rem in the stylesheet, with the unit stated.
TagsLinkGroup peels `style` for the row and forwards the rest to each chip,
which is right for `onClick` but is exactly the shape of the bug just
fixed -- `className`, a Mantine Group prop, or a `data-*` meant for the row
would all still land on every tag. `className` now goes with `style`,
since it is unambiguously a row concern and the likeliest next one, and the
split is documented next to the component. Not inverted to an explicit
chipProps: that reshapes a working API with one call site.
The contract is tested rather than only described, both halves in one test,
because they are one destructure apart and the `style` half was wrong for
as long as it existed. Verified by planting the regression: routing
className back to the chips fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A tag's word was 78% of the size of the body text beside it, which is right
for a label -- an annotation on something else, like the count next to a
tab's name -- and wrong for a tag, which is the whole content of its chip
and the thing a wall of them is scanned for. On the dashboard, where the
tags are the page, it read as a footnote. Now 87.5%.
The point grew with it, because it has to: its diagonal is the body's
height, or its two edges meet the body short of the corners and it reads as
a notch stuck on the side. 21px a side for the 30.2px body. That is also
the one length here that went UP -- the margin reserving room for the point
was a third of a pixel short of the larger one, which is enough to clip
against the tag before it.
Padding is unchanged. The gaps are two defects rather than a design:
- The link wrapper carried 0.3em either side, from when these were
Semantic labels with no margin of their own. A tag already reserves
the room its point needs, so that was 0.3em twice on top of it: two
tags 34.7px apart where the point needs 18.4px. Now 25.3px, leaving
about 7px of visible space.
- TagsLinkGroup spread its props down to every chip, so the dashboard's
marginTop -- meant to space the row below its heading -- landed on all
34 tags. That is not a margin above the row; it is 7.7px added between
every wrapped row of it. Row gaps 18.5px -> 12px.
Tests first: both margins fail on the real values (0.3em on the link,
0.5em on the chip), and the size is asserted as a ratio to body text so it
cannot be satisfied by the interface scale, which raised tag and prose
together and left the tag exactly as small next to it.
Verified in all four themes, including night, where the point is two
strokes rather than a solid diamond and so shows a corner mismatch plainly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CardGroup`'s default `minWidth` goes from 200 to 250, and the one call site that names its
own -- the map's search results -- from 220 to 275, keeping the 10% it sits above the default.
A card wall read too dense: at a 1280px window the grid ran five cards to a row where the
pre-migration Semantic build ran four, and a video title had about two words per line. At 250
it runs four cards of 300px, which is what Semantic measured at that width (four cards, 290px
each) -- so this lands on the old layout rather than near it.
Raising the default rather than each page is the point of it being a default: videos, archives,
docs, files and the gallery all take it.
Measured across the range, and no width regresses:
grid width cards/row card width poster side gap
430 1 430 0
570 2 277 0
600 2 292 0
900 3 290 0
1245 4 300 0
1600 5 308 0
A phone is unaffected -- one column below about 570px of content width either way -- and the
poster still fills the card at every width, since its cap is a fraction of the card rather than
a fixed height.
Verified: 1143 jest tests / 65 suites, clean build, screenshotted at 1280px.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seven review comments, all valid. The common thread is the one the guard was supposed to
catch and did not: geometry converted to rem beside geometry that stayed px.
The guard was narrower than its name. It missed:
- `transform`, `flex` and `flex-basis`, so `translateX(26px)` and `flex: 1 1 200px` were
invisible on properties nobody thought to list;
- every NEGATIVE length, because the lookbehind treated the minus sign as part of the
preceding token -- `left: -7px` and `margin-top: -2.5px` position the eyelet on a tag whose
diamond HAD been converted, and `right: -3px` / `bottom: -3px` place the corner badge on a
converted icon. Each is a piece of geometry that stopped tracking the piece it belongs to.
With both fixed it found nine values, including one no comment mentioned (`flex: 1 1 200px`).
All nine converted, except the three in a dead `.toggle` / `.slider` block: nothing has
referenced those classes since Toggle became a Mantine Switch, and it was ALREADY broken --
track and knob in rem, travel still `translateX(26px)`, so the thumb stopped ~4px short of a
54px track. Dead CSS that is also wrong is a trap for whoever revives it, so it is deleted.
The tag eyelet is centred with `translateY(-50%)` rather than a negative margin of half its
own height, so the two pieces cannot drift again whatever the scale.
Also from the review:
- A runtime check, which is what the source guards were missing. Three cases in
ui-layout.cy.js measure the real thing: the root font-size is the declared multiple of 16,
our text AND a Mantine control both grow by the same factor when the scale changes, and a
hairline does not. The second is the one that catches `theme.scale` being set and silently
cancelling the root.
- `max-height: max(..., 56.25cqw)` now follows a plain rem declaration. An engine without
container query units throws out the whole declaration, which would have left the poster
with NO cap -- a portrait cover standing as tall as the card is wide. Older engines clamp;
newer ones take the second.
- `pxToRem` in components/ui/scale.ts, replacing the `/ 16` written out in CardGroup and
Placeholder.
- The map overlays: 20 values across the layer panel, add-pin dialog, search panel and
context menu, whose type had grown 10% while their padding had not. Left in px on purpose:
the context menu's `left`/`top` (a real click coordinate), maplibre's own `ScaleControl`
maxWidth, and the `calc(100% - 100px)` reserve for maplibre's px-sized controls. The
`100vh - 120px` reserve DID become rem, since the chrome it reserves for is ours.
- The keyboard-shortcuts spec no longer deletes `.mantine-Notifications-root` from the DOM.
The reviewer was right that it is brittle; the endpoints are mocked instead, now that the
probe named them: `/api/tag`, `/api/events/feed`, `/api/files/search` and
`/api/files/worker_status`. Two of those were already "mocked" as `/api/tags` and
`/api/events`, which nothing calls -- the wrong paths were the reason a toast appeared at
all. Verified under CI's condition (`forceNetworkError` on everything unmocked): 12
passing with no DOM surgery.
What is NOT done, deliberately: about 176 numeric geometry values remain in 23 feature files.
The map overlays were the visible ones. Sweeping the rest is a change to 23 files nobody has
reviewed, so instead the guard's name now says "in our stylesheets" rather than implying the
whole app, and a new ratchet keeps numeric geometry out of the component library so the
eleventh inline `gap: 10` cannot be added there.
Both new guards planted against. Verified: 1142 jest / 65 suites, 251 ui-layout + 10
modal-stack component tests, all 50 e2e specs, clean build, map overlays measured to track the
scale (panel minWidth 200 -> 220, search 320 -> 352) and screenshotted.
The other half of CI's cypress-e2e failure: `clicking search icon opens modal` failed with
`expected '<input.wrolpi-searchbox-input>' to be 'visible'`, covered by a danger toast.
Toasts paint above modals on purpose -- z-index 400 against the modal stack's 200 -- so an
error raised while a modal is open stays visible. The search modal is tall, so it sits near
the top of the window, and a top-right toast covers the right-hand 192px of its 365px input.
Cypress occlusion-checks a `position: fixed` element, so the assertion failed.
Measured at both scales, because the question was whether the interface scale caused it:
130px of a 330px input before, 192px of 365px after. The overlap is PRE-EXISTING; the scale
widened it from 39% of the input to 53%, which is what pushed CI over the line. Roland's
call was to accept the overlap -- an error toast belongs on top -- and fix the test.
It passed locally and failed in CI because CI serves the React app with no API behind it.
The endpoints this spec leaves unmocked are `/api/tag`, `/api/events/feed`,
`/api/files/search` and `/api/files/worker_status` -- note that the spec's existing mocks say
`/api/tags` and `/api/events`, neither of which the app calls -- so they fail and api.js
raises "Unexpected server response".
The toast container is removed before the assertion rather than clicked away, because a click
races the next failing call. A blanket `/api/**` 200 stub was tried first and does not work:
a 200 with the wrong shape fails the same way, which is what "Unexpected server response"
means.
Verified by reproducing CI's condition locally (`forceNetworkError` on every unmocked
endpoint): 12 passing with the removal, 11 passing and 1 failing without it. All 50 e2e
specs pass under normal conditions.
CI's cypress-e2e job went red on the interface scale: `directory-search.cy.js` failed
`expected '<div.wrolpi-searchbox-result-title>' to be 'visible'`.
The suggestion list is capped at `min(60vh, 30rem)`. The rem ceiling scales with the
interface but a fraction of the VIEWPORT does not, so when the type grew 10% the rows grew
and the cap did not: the full set of grouped suggestions now needs about 420px, and 60vh of
a 660px-tall window -- Cypress's default -- is 396px. The last group was clipped again,
which is the exact defect the comment on that rule already describes.
70vh is 462px there, and the 30rem ceiling still bounds it on a tall window, so the list
cannot cover the page either way.
Measured rather than guessed: the spec fails at 660px of viewport height and passes from
700px up, so the shortfall was about 24px.
Verified: all 50 e2e specs pass locally, 248 ui-layout component tests, 1141 jest tests.
`--card-poster-max-height` is a fixed height, so it stops scaling with the card: held to
220px, a 16:9 thumbnail is 391px wide, and any card wider than that centred it with empty
panel on both sides. That is every video card at one column -- a phone, or any window
under about 456px of content width.
The cap is now `max(var(--card-poster-max-height), 56.25cqw)` with the poster band as the
query container. 56.25cqw is nine sixteenths of the card: exactly the height of a 16:9
image that fills the width, and the same number states the rule for every other ratio --
a poster may be as tall as a 16:9 slice of this card and no taller. So a portrait book
cover still reads as a band rather than a column, and now grows with the card instead of
stopping at 220px. `max()` rather than `min()` so the floor wins on a narrow card.
The gap pre-dated the interface scale, below about 414px, but the scale widened the range
it shows in: the track minimum went from 200px to 220px, so one column now starts at 456px
of content width instead of 414.
Every existing poster case mounted a 233px card, which is why the wide card's own failure
went unseen for the whole migration. Two cases added on a 451px card: a landscape poster
reaches the edges, and a portrait cover stays a band.
Three tests carried a hardcoded px expectation, which is a hidden assertion that the
interface scale is 1. All three failed at 1.1, in names that said nothing about size --
and I did not catch them because I ran jest and the build for the scale change but not
cypress, which AGENTS.md asks for on frontend work:
- `cardPosterCap()` read `--card-poster-max-height` with parseFloat. The cap is rem now,
so it returned 12.5 and failed a within(80, 240) bound -- taking five poster tests with
it. It resolves through the root font-size and checks the unit.
- `inNarrowColumn` was a fixed 340px, chosen to hold two 150px statistic tracks. At 165px
tracks it became one column, so a test about the gap BETWEEN columns had no second
column. It is now 340px times the scale, because "narrow" means narrow relative to the
type.
- The APIButton size test asserted 36px for `small`. It now measures against a real `sm`
button, which is the claim it was making -- that `small` resolves to `sm` -- and holds at
any scale.
A `uiScale()` helper reads the token so no further test has to name a pixel count.
Verified: ui-layout 248 passing, 0 failing; 1141 jest tests / 65 suites; clean build;
browser-checked at 400/451/500/700/1200px of grid width, no side gap at any of them. The
12 remaining component failures are the pre-existing Semantic-era selector failures in
Tags, Channels, Download and Inventory.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The interface reads about 10% smaller than the pre-migration Semantic build, and the
drift is not one number. Measured on the QA Pi at 1280px, Semantic's headings were
h1 28 / h2 24 / h3 18 / h4 15 / h5 14 against our 26 / 21 / 17 / 14 / 13, and its video
card a fixed 290px with an 18px title against our 238px with a 13px one -- while our
BODY text and chrome went the other way, a 16px base against Semantic's 14 and 42px
buttons against 36. No per-component nudge reproduces what a user comparing the two
sees, which is the whole page at once.
Ten percent is not a fudge factor: our heading scale times 1.1 is 28.6 / 23.1 / 18.7 /
15.4 / 14.3, which is Semantic's scale to within a pixel at every step. Verified in
the browser -- those are the numbers it now renders.
`--ui-scale: 1.1` and `html { font-size: calc(100% * var(--ui-scale)) }`, with
everything that should grow written in rem:
- 102 lengths in ui.css, App.css and tokens.css converted, restricted by property:
font-size, padding, margin, gap, width/height, border-radius, offsets.
- Borders, outlines and shadows deliberately NOT converted. A hairline is a hairline
at any text size.
- 46 inline `fontSize` px values across 8 files. `em` values were left alone: they
already resolve against the parent.
- Ten inline-styled blocks in the library (Card's text block, Loading, Placeholder,
ColoredInput, Confirm's action row, CardGroup's gap) moved into ui.css, because an
inline px size cannot be reached by a scale or seen by a test.
- CardGroup converts its `minWidth` prop to rem, which is what returns a phone to one
card per row: as px the track ignored the scale, so a 430px screen fitted two 200px
cards where the old build fitted one 290px card.
Mantine needs nothing: its 644 size declarations are `calc(Xrem * var(--mantine-scale))`
and scale with the root. `theme.scale` must stay at 1 -- it exists to CANCEL a
customised root font-size, so setting it to 1.1 as well would leave Mantine's half of
the interface at the old size beside our larger text. A test pins that.
`zoom: 1.1` on the root was tried first and rejected. It works and it is one line, but
it scales at paint: every 1px hairline becomes 1.1px and softens on a 1x display, and
viewport units do not shrink to compensate, so `100vh` paints 110% of the screen -- a
95vw modal overflowed sideways and the CBZ viewer's fullscreen overflowed down. Rem
scales real lengths, keeps borders crisp, leaves viewport units alone, and preserves a
user's own browser font-size preference instead of overriding it.
A percentage rather than a px root size, for that last reason.
Five guards in themes/ui-scale.test.js, each planted against: the scale itself, that
every scalable declaration is rem, that borders are NOT, that no inline px font-size
survives anywhere including the gallery, and that Mantine's scale stays 1.
Also removes `.ui.modal.themed-confirm`, a dead Semantic-era rule and the source of the
one `95vw` that would have overflowed: `.ui.modal` no longer exists and nothing sets
that class.
Verified: 1141 tests / 65 suites; clean build; browser-checked at 1280px and at a 430px
phone; navbar single-row from 780 to 1800px; hairlines still 1px and no horizontal
overflow in all four themes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Confirm hardcoded `size='tiny'` and its props interface accepted no size, so every
confirmation in the app was 380px whatever it held. That is right for eight of the
nine call sites and for the ~30 prose confirmations `useAPIButton` renders behind a
`confirmContent` -- and wrong for TaggedDeleteConfirmModal, which lists file paths
beside their tags in a two-column table and had no way to say so.
`size` is now a default, not a fixture, in the same vocabulary as Modal. Only
TaggedDeleteConfirmModal changes: `large`, 620px. The table scrolls sideways in its
own container, so the width decides how much of a path is readable without dragging,
and at 380px that was the first two directories.
Tests, written first:
- Confirm resolves to `sm` by default and `lg` when a call site asks, measured off
the `--modal-size` Mantine writes on the modal root rather than restated.
- TaggedDeleteConfirmModal renders at `lg` while a plain confirmation beside it stays
`sm`, so it cannot pass by confirmations having become wide for everyone.
- The existing "no unknown size name" guard now scans `<Confirm` too, since Confirm
forwards the name to Modal and is the same hazard, with a premise check that at
least one confirmation names a size.
Both new tests read EVERY modal root, not the first. A modal is portalled to
document.body, so two renders in one test both land there and querySelector returns
whichever opened first -- which is how the first version reported the default twice
and read as though the size prop had no effect.
The gallery gains the wide confirmation next to the narrow one, so both are
checkable in all four themes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Acting on the review of #628. No bugs; one suggestion and one nit, and the
suggestion is answered with a measurement rather than a change.
The review asked whether MakeDirectoryModal should drop back to `small`, on the
grounds that its body -- a path, one field and a button -- is the same shape as
RenameModal's, which is `small`. The shapes are the same; the content is not.
Rename shows a RELATIVE path, make-directory shows the absolute one including the
media directory, and a bare `<pre>` is `white-space: pre`, so it scrolls out of
sight rather than wrapping.
Measured in the browser at both widths: 440px holds 42 characters, 620px holds 61.
`/media/wrolpi/videos/Practical Engineering/Season 2024/` is 55. So an ordinary
channel-and-season path is unreadable at 440 and fits at 620, which is exactly
the condition the review set for keeping it. The dev tree hides this -- its
paths are around 22 characters and fit either way.
Both `large` call sites now carry the audit comment the other three had, which
was the nit. Make-directory's records the measurement, because it does read like
an inconsistency beside rename and the next person to notice should see the
numbers before narrowing it back.
1134 jest / 64 suites (--maxWorkers=2; see the previous commit), clean build.
These five carried no `size` at all, so they rendered at Mantine's 440px default
-- a width nobody had chosen, inherited from the library rather than decided.
Roland audited them:
FileBrowser rename small (440, unchanged)
FileBrowser move large (620)
FileBrowser ignore small (440, unchanged)
FileBrowser make directory large (620)
Files filter small (440, unchanged)
The three "unchanged" ones are written down rather than left implicit, so the
decision lives in the source and a future change to Mantine's default cannot
move them.
Roland asked for `medium` on the ignore dialog and there is no such modal size:
the vocabulary is mini/tiny/small/large/fullscreen, and Mantine has no token
between 440 and 620. An unknown name would have passed through untranslated,
rendered at 440 anyway, and left the source claiming otherwise -- which is what
the size guard in ui.test.js was added to catch. Asked rather than guessed;
`small` was the answer, and the guard would have failed the build either way.
Verified in the browser rather than from the props: move 620, make directory 620,
ignore 440, rename 440. Move and ignore need a selection before they can be
opened, so a directory was selected first.
1134 jest / 64 suites, clean npm run build. The full suite needs
`--maxWorkers=2` on this machine at the moment: at load average 38 four unrelated
suites (ControllerPage, CollectionReorganizeModal, CollectionTagModal, Flasher)
time out and the run takes 183s instead of 5s. They pass in isolation and with
reduced parallelism; nothing in this change touches them.
283 occurrences across 23 files, all of them mine and all of them in the
theme/UI work: comments, test names, and a handful of local identifiers
(`colours`, `roleColours`, `awaitColoured`) plus one test fixture tag named
'Uncoloured'.
Nothing was required to keep the other spelling. Checked before replacing: no
CSS custom property or class name contains it, no library prop or API key does,
and every string holding it is a test name or our own copy. The renamed
identifiers were checked for collisions with an existing `color`/`colors` in the
same file first -- there were none.
Verified as a pure rename: 262 lines changed, and normalising the spelling on
every removed line reproduces the added line exactly, for all 262. So the diff
contains no other edit hiding in it.
1134 jest / 64 suites, 271 cypress component tests with the same 12 pre-existing
Semantic-era failures in Tags/Channels/Download/Inventory, clean npm run build.
Acting on the review of #626. No bugs; one suggestion and three nits.
The map's icon-only controls had no accessible name. IconButton requires a
label by construction and AddToPlaylistButton supplies one, but the plain Button
and APIButton used for edit, delete and the catalog preview reached assistive tech
as unnamed buttons -- and my own Cypress fixtures carefully passed `aria-label`
on the same pattern while production did not, which is its own kind of tell. All
four are named now, and named with the row's subject rather than the verb alone:
"Delete Water tank", not "Delete", because a table of pins otherwise offers a
screen reader a column of identical buttons.
A source guard holds that: it scans for controls that carry an icon and no label
text, and reports any without an `aria-label`. Paired with a premise that it
found some, since a scanner matching nothing reads exactly like a clean file.
The Group guard asserted only that `<Group` appeared somewhere in the cell, which
would pass on an empty Group left beside the bare siblings it was meant to wrap.
It now checks that every control in the cell is inside the Group -- and the first
version of THAT was wrong too: it sliced from `<Group` to the end of the cell
rather than to `</Group>`, so controls after the closing tag still counted as
inside, and the empty-Group plant stayed green. Slicing between the tags makes
it fail.
Coverage the review asked for: the Semantic size names the call sites actually
use (`small`, `big`, `huge`) rather than only the Mantine ones, since the
translation step was untested on the exact spellings that were broken; icon-only
buttons stay square while loading, when the glyph is replaced by a spinner sized
from the button's height; and `iconAfter`-only squares as well as `icon`-only.
On the size passthrough, which was flagged as the highest-risk change outside the
map: the buttons that grow are Settings' Shutdown and Reboot (`huge`, to 60px),
Settings' Upgrade and Save and Channels' Save (`big`, to 50px), and VideoPlayer's
Refresh (`large`, to 50px). All six read as deliberate emphasis -- the two
biggest are the two destructive ones -- so they are left honoured rather than
re-broken at the call site. `size='small'` resolves to `sm`, which is what the
accidental default already was, so the detail pages do not move.
1134 jest / 64 suites, 246 in ui-layout.cy.js, clean npm run build.
Reported as "all different sizes", and the pin row's three actions measured
46x30, 30x30 and 54x36. Three causes.
APIButton dropped `size`. `useAPIButton` declared `size = 'medium'` as a
parameter and then never put it into the props it hands to Button, so every
APIButton in the app rendered at Mantine's default regardless of what its call
site asked for -- seventeen call sites pass one, from `xs` on the map pins to
`big` and `huge` on Settings. The delete pin was 36px tall beside two 30px
siblings because of it. Passing the parameter default through would have fixed
the bug by breaking everything else -- every APIButton naming no size would have
grown from `sm` to `md` -- so the default is undefined now and Button's own
default stands. Eight buttons on three other pages do grow as a result, which is
their call sites finally being honoured: VideoPlayer's `large`, one of Channels'
`big`, and Settings' two `big` and two `huge`.
An icon-only Button was not square. A Button is as wide as its content plus its
horizontal padding; an ActionIcon is square by construction; and the pin row
mixes the two, because AddToPlaylistButton renders an IconButton when it has no
label. Their heights already agreed -- ActionIcon's scale was remapped onto
Button's some commits ago -- and this is the other axis of the same alignment,
keyed off Mantine's own `--button-height` so the two stay in step through every
size.
They were also misaligned and touching, which is a third thing: bare inline-block
siblings align on text BASELINES, and a Button's baseline is not an ActionIcon's,
so the middle control sat 4.3px above the other two. A Group, as every other row
of buttons in the app uses.
Written test-first at Roland's request. Seven cypress cases went red on the
sizes -- widths 46 against 30, 54 against 36, 62 against 42, 70 against 50, and
"xs is shorter than lg: expected 36 to be below 32" for the dropped size -- with
both inverses green throughout: a labelled button stays as wide as its label, and
an APIButton naming no size keeps the height it had.
The alignment claim is measured in ui-layout.cy.js, including the premise that
bare siblings really are misaligned, since three controls of identical height not
lining up is not believable otherwise. The call sites are held by a source
guard, because MapPins fetches from the API and builds its rows through
SortableTable.
That guard earned itself twice over. Written for the pin rows, it found the
map-files subscribe cell putting a preview button beside a subscribe button with
the same defect. Then a first attempt at a general "any cell with two controls"
rule had to be abandoned: a cell whose buttons are the arms of a ternary renders
only one and needs no Group, and excluding ternaries to allow for that also
excluded the subscribe cell, whose `role={isSubscribed ? ... : ...}` says nothing
about how many controls it holds. Named cells with a stated scope beat a general
rule that quietly stops covering things.
Two slips of mine in that file, both caught by running it: `expect(value, message)`
is chai and throws in jest, and `onSave(pin.id, ...)` was not unique to the cell
it named -- the edit row's TextInput calls it from onKeyDown in a different cell,
so the check passed against the wrong one.
1132 jest / 64 suites, 243 in ui-layout.cy.js, clean npm run build. Confirmed
live: three 30x30 controls, vertical spread 0, 10px gaps.
Acting on the review of #625. No bugs; four suggestions and three nits, of
which two were already handled and one needed no code.
The weld used three separate 8px literals -- the control's padding, its flex gap,
and the clear button's negative margins -- which had to stay in lockstep or the
button would drift past the border or back inside it. They are one
`--searchbox-inset` now.
Cancelling the gap on the clear button's LEFT was a real side effect: when a
field is both `loading` and `clearable`, which SearchResultsInput allows, the DOM
order is input, spinner, clear, and the spinner ended up welded to the divider.
Only the right-hand padding is cancelled now; the 8px before the button stays,
and a case covers the pair.
`min-height` was still coming from `--ai-size`, so the rule only half-owned the
height it claimed to set -- true today by 0.15px, the field being 36.15 against a
36 minimum. Unset now.
Two nits needed no code and are recorded as findings rather than changes. The
clear button's corners are already square: the theme zeroes every
`--mantine-radius-*`, so there are no wedges to show against the field, and a
case now depends on that explicitly. And a label-less button with BOTH `icon`
and `iconAfter` would stay mis-centred, but no call site writes one, so the limit
is documented instead of designed around.
Blank children now count as no label. `toArray` drops null and booleans but
keeps `''` and whitespace, which would have left the glyph off-centre; no call
site does it today and the predicate should not depend on that.
Two corrections to my own work, both caught by planting:
The fragment case. I wrote that `toArray` flattens fragments away to nothing and
tested `<></>` as unlabelled. It does not -- it returns the fragment as one
child, so `<></>` is indistinguishable from real content. The test now records
what actually happens, with `<>Delete</>` as the shape that occurs in practice.
The min-height assertion compared `--ai-size` -- which reads back as
`calc(2.25rem * var(--mantine-scale))` -- against a computed `36px`. Two strings
that can never match, so it passed with the rule removed. It compares resolved
lengths now, paired with an ordinary IconButton that DOES carry the minimum, so
its absence is attributable to this rule rather than to a Mantine default. An
earlier attempt fabricated a shorter field by overriding the input's font size;
that was deleted, because a test that invents a layout the app never renders is
not evidence about this app.
1128 jest / 63 suites, 231 in ui-layout.cy.js, clean npm run build. Confirmed
live: inset 8px, clear button 1px from the field edge, 8px from the input, at the
field's full inner height with no minimum of its own.
Reported together as "button groups", but they turned out to be three unrelated
things.
The file browser's footer buttons touched while the Channel Edit page's had
gaps. Channel Edit lays its buttons out with Mantine's `Group`, which is a flex
row with a gap; the footer is hand-rolled and was a plain block, so its
inline-block buttons sat flush -- JSX drops whitespace-only lines, so there was
not even a space character between them. The footer is now the flex row every
other button group already is, wrapping because it carries eight controls.
The Settings config table's Import and Save buttons had off-centre icons while
Restore in the same row looked right. Restore is an IconButton -- an ActionIcon,
which centres a lone glyph by construction. Import and Save are icon-only
Buttons, and an icon on a Button goes into Mantine's `leftSection`, which carries
a `margin-inline-end` to hold it off the label. With no label that margin is
pure offset: measured at 8px left of centre. An icon with no label is now
rendered as the button's content instead, so it centres. `Children.toArray`
decides that, because it drops nulls and the file browser passes
`{label('Delete')}` -- null whenever the bar is too narrow for words, which is
exactly where the offset showed most.
The search field's clear button sat inside the field rather than attached to it:
the control is a bordered box with `padding: 0 8px`, leaving the button 9px short
of the edge and 2px short of the height. Semantic welded it to the field --
measured on the QA Pi at zero gap and full height -- so it now stretches and
negative margins cancel the padding and the flex gap. Negative margins rather
than dropping the control's padding, because that padding is what holds the
search icon and the text off the border; only this one child should escape it.
The `html` prefix on that rule is load-bearing: Mantine sets ActionIcon's height
from `--ai-size` on its own single class, and at equal specificity the winner is
whichever stylesheet loaded last.
Fourteen new cases. One correction worth recording: the height assertion first
compared against the control's border box and failed on correct behaviour -- the
button fills the CONTENT box, and the 1px above and below it is the field's own
border, which it should sit inside rather than paint over. Each fix planted
separately; the icon change reddens three, the footer one, the clear button one.
Every case is paired with an inverse -- a labelled button must keep its icon on
the left, and the field must still be padded around its other children.
1128 jest / 63 suites, 226 in ui-layout.cy.js, clean npm run build. Confirmed
live: footer gaps 8px on one row, config glyphs at 0.0px from centre, clear
button 1px from the field edge at the field's full inner height.
Acting on the review of #624. Two bugs, both real, both confirmed in the
browser against the live stylesheet before fixing.
Mantine renders a loading button as `disabled={disabled || loading}`, so every
busy control in the app is `:disabled` -- an APIButton mid-request, the refresh
button, the flasher's connect. Mantine's own grey repaint is careful to exclude
`[data-loading]` for exactly that reason; the new rule was not, so "working..."
faded to 0.45 and read as unavailable rather than active. Measured: a loading
button came back at opacity 0.45. Both roots now mirror Mantine's exclusion.
Night encodes danger as a dashed border on a transparent fill, because it has no
second hue to spend. The general disabled rule repaints from `--button-bg` --
the filled red Mantine leaves in the variable -- and at (0,2,1) it tied with
`html[data-theme="night"] .wrolpi-button-danger` and won on source order.
Measured: background rgb(255,87,87), border solid transparent. So the file
browser's Delete, which is role='danger' and disabled until a selection exists,
became a solid red chip in the one theme where that is not what danger means.
Restated at (0,3,1) so it no longer depends on where it sits in the file.
The specificity note in the comment was also wrong, and wrong in a way that
would mislead the next person: Mantine's disabled paint is
`:where(:disabled:not([data-loading]), ...)`, and `:where()` contributes
nothing, so it scores (0,1,0). The `html` prefix is not needed to beat that --
it is needed to stay level with the app's own `html ...` overrides, and the
night danger collision is the demonstration.
The fade is now `--disabled-opacity`, read by the tests instead of restated, and
asserted for equality rather than against a band -- a band tolerates an
accidental different fade or an opacity leaking in from elsewhere and still
reports success.
Five new cases from the review: loading versus disabled (paired, so it cannot
pass by nothing fading at all), night's dashed danger with a light-theme inverse
proving the fill is not stripped everywhere, cross-colour distinctness in all
four themes rather than only light -- night and amber are where one hue makes
collapse most likely -- and an outline pair checking text and border, which are
the only things a fill-less variant has to lose. Each planted: dropping the
loading exclusion reddens one, dropping the night restatement reddens one.
1128 jest / 63 suites, 216 in ui-layout.cy.js, clean npm run build.
The file browser's footer is eight buttons, six of them disabled until something
is selected, so the toolbar a user meets on arriving at /files was an
undifferentiated grey row: Delete, Rename, Move, Ignore and Tag rendered as five
identical pixels. Colour is how those are told apart at a glance.
Mantine repaints a disabled button flat grey -- background, text and border all
replaced with `--mantine-color-disabled`. Semantic kept the colour and dropped
the whole control to `opacity: 0.45`; measured on the QA Pi, a disabled Delete
is still red at 0.45, Rename still yellow, Tag still violet. That is the better
signal: it says "not available" without also saying "no longer the delete
button".
Mantine leaves the variables alone and overrides only the painted properties, so
the colour the call site asked for is still sitting in `--button-bg` and can
simply be reinstated. Applies to every disabled Button and ActionIcon, not only
the file browser's -- it is one convention, and having two would be worse than
either.
A pre-existing test earned its keep here: the IconStack corner disc was pinned
to `--mantine-color-disabled` so it would follow a disabled button's grey, and
it reddened the moment the button stopped being grey. That override is gone;
`--button-bg` is now correct in both states, so the general rule covers them.
Ten new cases. Fill equality is checked against each button's own enabled twin
in all four themes, and separately across the five colours at once -- comparing
each against its twin would still pass if every pair were grey, which is the
actual bug. Opacity is asserted from both sides, because keeping the colour
must not cost the affordance. Removing the rule reddens eleven.
1128 jest / 63 suites, 209 in ui-layout.cy.js, clean npm run build.
/zim/manage rendered a white screen with `Cannot read properties of undefined
(reading 'toLowerCase')` repeated once per catalog row.
The language dropdown is built as `{value: code, label: iso_639_codes[code]}`.
That table covers ISO 639-1 and 639-2; the Kiwix catalog also carries 639-3
codes and locale variants -- `ami`, `ary`, `arz`, `be-tarask` -- and on the
running instance 89 of the codes in the catalog are absent from its 672-entry
table. Every one of those produced `label: undefined`, and a `searchable`
Mantine Select calls `option.label.toLowerCase()` while building its dropdown on
MOUNT, before anyone opens it. So a single unnamed language anywhere in the
catalog took the page with it, and Wikipedia alone carries dozens.
The option falls back to the code. Not to a blank: a user choosing which
language to subscribe to has to tell one unnamed language from another, and the
code is the only thing known about it. `iso_639_codes` is read defensively too
-- ManageZim initialises it to null and fills it from the API, so the first
render of every row happens with no table at all.
The other two lookup-derived labels in the app, in ControllerPage and Configs,
already fall back this way. Zim was the only one that did not.
Four cases, written first and confirmed to fail with the reported TypeError
rather than for some other reason -- the first version failed on "Table
component was not found in the tree", which would have proved nothing, so the
row is now given the table Mantine requires. One of the four is the inverse: a
code the table DOES name must still render its name, so the fallback cannot
quietly replace every label.
1128 jest / 63 suites, clean npm run build. Confirmed on the running app: the
page loads with 32 language dropdowns and no error overlay.
Six keyboard-shortcuts e2e tests started failing on CI: clicking the search
glyph was refused because a `mantine-Notification-title` was on top of it.
The toast container is fixed at `top: var(--mantine-spacing-md)` -- 16px --
while the bar occupies the first 54px of the page. Every toast has therefore
been landing on the bar's right-hand corner: the search glyph, the hamburger,
Help and Admin. For as long as the toast is up those controls cannot be
clicked, and an error toast is precisely the moment a user reaches for Admin.
It surfaced now because the previous commit gave the search glyph a proper hit
area. At 18px it sat in the gap beneath the toast; grown to the height of the
bar, it moved into the overlap. The toast has been covering that corner all
along -- there was nothing there to hit. So this is not a test that needs
loosening: CI found a real defect that predates the change that exposed it.
Toasts now start below the bar. `--navbar-height` is shared between the bar's
min-height and the offset so the two cannot drift, and the rule matches
`[data-position^='top']` so a future move to bottom-right does not silently
inherit a top offset. Mantine's own positioning uses `:where()`, which has zero
specificity, so a single class wins.
Tested by `elementFromPoint` rather than by comparing rectangles: that is the
question Cypress's actionability check asks and the one a mouse asks. A
geometry test would miss a toast that overlaps but sits behind, and a z-index
test would miss one that is in front but elsewhere on screen. Three cases --
the search control, every control in the corner, and the offset itself, which is
stated separately because the first two would also pass on a toast that merely
happened to be narrow enough to miss at one viewport. Removing the offset
reddens all three.
Confirmed on the running app with a real error toast up: toast top 70px, bar
bottom 54px, all six corner controls topmost at their own centres. The e2e
spec passes locally, 12 of 12.
1124 jest / 62 suites, 199 in ui-layout.cy.js, clean npm run build.
Acting on the review of #622. Two bugs, both real, and the second one is a
test of mine that could not fail for its stated reason.
The stack props were set BEFORE `{...props}`, so any call site passing
`closeOnEscape` overwrote the stack's decision and re-armed Mantine's
window-level Escape listener on a modal that is not on top -- reinstating the
bug the change exists to fix. Three call sites pass it: CollectionReorganizeModal
and BatchReorganizeModal disable Escape while a reorganize runs, Flasher while
it writes an image. The concrete breakage is the one place the app really does
stack two modals: ConflictResolutionModal opens as a sibling above the
reorganize modal, so the moment the reorganize finished and `closeOnEscape`
returned to true, one Escape closed both. They are now destructured out and
composed -- `isTop && closeOnEscape` -- because "only the top modal answers
Escape" and "do not dismiss me while I am busy" are both true. Mantine's own
Modal.Stack applies its stack props after the caller's for the same reason.
Two Cypress cases, one per direction; planting the old order reddens the first.
`Confirm` was still passing Mantine's raw `sm`, and the audit guard I added in
the same PR was supposed to catch exactly that. It did not, and the reason is
worse than the miss: the scanner matched `<Modal\b[^>]*?\bsize='...'`, and
`onClose={() => ...}` contains a `>`, so it stopped at the arrow and never saw
the rest of the tag. It read 44 of the 56 modals that declare a size; the
twelve it skipped included the single offender in the codebase, in this very
file. A guard that scans a fifth of nothing looks identical to a clean
codebase. It now walks the tag with a paren counter, accepts either quote
style, and asserts it scanned more than fifty tags first -- so a scanner that
silently matches nothing fails instead of passing. Confirm says `tiny`.
Also from the review: a bare `expect(titles()).to.not.include('Parent')` in the
Cypress spec ran during the synchronous phase, before any cy command executed
and while nothing was open, so it always passed and guarded nothing. Deleted
rather than repaired -- the retried assertion above it already makes the claim.
An unregistered-but-open modal reported `isTop` even with a parent registered,
which gave both the same z-index for the frame before the effect ran -- a visible
flash of the wrong modal in front. It is only top now if the stack is empty.
Nit: the comment claimed Mantine ignores unknown size names. It ignores unknown
SEMANTIC names and falls back to 440px; it recognises and honours its own. Two
different hazards, both silent, and the comment now says which is which.
1124 jest / 62 suites, 10 in modal-stack.cy.js, 196 in ui-layout.cy.js,
clean npm run build.
Two things, both reported from the running app.
Nested modals could not exist. Mantine binds a window-level keydown listener
per open modal, gated only on whether that modal is open -- nothing checks which
one the user is looking at:
useWindowEvent('keydown', (event) => {
if (event.key === 'Escape' && closeOnEscape && ... && opened) onClose();
}, {capture: true});
Two open modals meant two listeners, so one Escape closed both. Reproduced on
the dashboard: the download modal plus the search modal over it, one press, both
gone, back to the page. Both also sat at z-index 200, so which one drew on top
was DOM order rather than which the user opened.
Mantine's own answer is Modal.Stack with a stackId, which is the only thing that
gates closeOnEscape. Not used, for two reasons: every call site would have to
be rewritten to hoist its modal into a stack, and it sets `__hidden` on
everything below the top -- the parent disappears while the child is open. The
parent should be dimmed and still there, as it was before the migration.
So the wrapper keeps the register itself: a module-level list of open modals,
most recent on top, from which each modal takes closeOnEscape, trapFocus and a
z-index two above its parent. Module-level rather than React context because
the two modals in the bug are not nested in the JSX at all -- the search modal
is rendered by KeyboardShortcutsProvider near the app root and the download
modal by the dashboard. They are siblings that happen to be open together, and
no context relates them. The increment is small on purpose: Mantine puts
popovers at 300, and a stack that climbed past it would paint over the select
dropdowns inside its own forms.
Widths: Roland audited the modal list and chose sizes against Mantine's scale.
Sixteen get wider -- twelve to `large` (620), four to `fullscreen` -- and eleven
keep the width they had, with the size now written down so the decision is in
the source rather than in a default. Flasher was passing Mantine's raw `sm`,
which worked but bypassed the naming the audit used; it says `tiny` now.
The size table itself is guarded, because it is the single thing that halved
every modal during the migration and a quiet edit to it moves seventy at once.
Semantic's widths, measured on the pre-migration build still running on the QA
Pi: mini 360, tiny 540, small 720, large 1080, no-size 900. Mantine's, read out
of its stylesheet: 320/380/440/620/780. A `small` modal went 720 -> 440 and one
with no size went 900 -> 440. Three cases now hold the scale, the mapping, and
the rule that no call site may name a size the table does not know -- an unknown
name is passed to Mantine, ignored, and silently rendered at the default.
Tested in a real browser: none of it is visible to jsdom -- the listener is on
window with capture, the stacking is z-index, and "still visible behind" is
geometry. Note that `should('be.visible')` is the wrong assertion for the
parent: it is an occlusion check, so a child drawn correctly over its parent
trips it. The test uses a wide viewport and asserts the parent is rendered and
shows to both sides of the child. Planting the pre-fix behaviour reddens three.
1124 jest / 62 suites, 8 in modal-stack.cy.js, clean npm run build. The 12
failures in Tags/Channels/Download/Inventory.cy.js are the known pre-existing
Semantic-era selector failures, unchanged.
Acting on the review of #621. No bugs were found; these are the four
suggestions and two nits.
The real one: measurement accepted only hex. `relativeLuminance` returned 0 for
anything else, which is black's luminance and indistinguishable from it -- so a
token written `rgb(92 79 168)` measured as pure black, the "best" foreground was
chosen against a colour the theme does not contain, and the wrong hex was then
frozen into an inline style where CSS could no longer correct it. Silent, and
worse than not measuring. `parseColor` now reads `rgb()`/`rgba()` in both the
comma and space forms, percentages, and `#rgba`/`#rrggbbaa`; `isMeasurable` lets
a caller tell "unreadable" from "black"; and navColorsFrom falls back to the
tokens when the background or every candidate is unreadable. Unreadable
candidates are filtered rather than the whole measurement abandoned, so one
oddly written token does not cost the other two. This matters because the
header claimed custom YAML themes get correct foregrounds for free, and a
hand-written palette is exactly where a non-hex value will first appear.
The Cypress inverse hardcoded night's `--btn-text` as `#0a0000`. The unit tests
parse tokens.css precisely so a palette edit cannot leave a stale claim behind,
and that literal was the one place the rule was broken. It reads the token from
the theme now, asserts it resolved, and additionally pins that red still takes
`--btn-text` while brown no longer does.
Documented rather than changed: the warning indicators still pick their colour
from the `conflictingColors` name list. It is the same defect as the one fixed
here and measurably worse -- `--danger` and `--warning` are 1.0-1.8:1 against
the bar in every theme, light and dark included, because a red icon on a red bar
is a red icon on a red bar. The fix is not a better colour: severity has to
stay distinguishable from the bar's text as well as from itself, and a
monochrome theme has no third brightness, so it has to stop being a hue. That
is a redesign, and the note now sits beside the residue in navColors.ts rather
than only in a PR description. The gallery draws the indicators as the bar
draws them when nothing is wrong; it does not stage a fake overheating Pi to
exhibit a fault nobody has agreed how to fix.
Also documented: re-measurement is triggered by `data-theme`, so a future
custom-theme editor patching custom properties in place would leave the bar on
the values it last measured. There is no event for "a custom property was
assigned" to observe instead, so the note says what such an editor must do.
Nits: "Eleven" in a comment where the assertion locks ten (and a stale 2.91:1
beside it, now 3.27:1), and a missing space in `const wrolpiIcon =<img`.
Six new unit cases, planted: dropping the fallback reddens two.
1121 jest / 62 suites, 196 in ui-layout.cy.js, clean npm run build.
Four faults, each measured in the running app before and after.
The search glyph carried Semantic's `item` class, which has had no rules at all
since Semantic was removed: no pointer cursor, no hover, and an 18.6px hit area
against a 54.4px bar. It is the same kind of thing as Help and Admin and now
takes the same class. The share glyph did compute `cursor: pointer` -- it is an
`<a href>` -- but its target was the 24px box the icon occupies, so the pointer
appeared only if you found it exactly. Corner anchors now stretch and pad like
a link.
The More button rendered with NO class at all: a bare grey UA button while the
rest of the bar took the user's colour. Menu.Target clones its child and hands
it a className of its own, and the trigger spread those cloned props after its
own `className` attribute, so the clone replaced ours. It is merged now. This
was mine, from extracting NavDropdownTrigger last commit -- and it also made the
previous commit's "measured and drawn are the same component" claim only half
true, because the hidden placeholder is not cloned and so kept its class.
The bar still wrapped, for a reason the padding fix could not reach: the corner
does not keep a constant width. The Cmd-K hint renders only once
useKeyboardDetected has seen a keypress, so it appears at an arbitrary later
moment and widens the corner by ~30px; the load, memory, temperature and drive
warnings come and go with the machine's state. None of that resizes the
container, which was the only thing observed -- so nothing recalculated. That
is the other half of "at certain widths": the same viewport wrapped or not
depending on whether the user had touched a key. The corner and the home link
are observed too now.
The search modal showed an input and the first line of the first group. The
suggestion list is absolutely positioned, which is right everywhere else -- a
list that reflows the page while you type is unusable -- but in a modal it fails
twice: an absolute list adds no height, so the panel sized itself to the search
box alone at 114px, and the panel's own `overflow: auto` then clipped the list
at that edge. In the modal the list sits in flow; the inverse is asserted so
the rule cannot leak to the pages where overlaying is correct.
Two of the new tests were wrong first and reverting the fix left them green:
they asserted hand-written markup carrying the classes I expected the components
to have, rather than the real SearchIconButton and ShareButton. They use the
real components now. Every fix was then planted against: 5 red for the More
class, 2 for the search class, 2 for the corner hit area, 1 for the observer,
2 for the modal.
1115 jest / 62 suites, 196 in ui-layout.cy.js, clean npm run build.
Confirmed live: share and search both pointer at 54.4px, More white on the bar
colour with a transparent face, and the modal's list in flow and unclipped.
Three faults in the bar, all reproduced by measurement before being fixed.
The indicators sat low. The slot carried `marginTop: 0.8em`, a nudge left over
from Semantic's menu; the corner centres its children, and a top margin is part
of the margin box being centred, so it applied as a ~6px downward shift. It
read as an error on the hotspot and the theme picker but not on share or search,
which is why it survived: an IconButton is a square ActionIcon that centres its
glyph exactly, while a bare-icon anchor sits high inside a line box that
reserves descender space for text that is not there. The nudge cancelled that
out on one shape and was pure error on the other. Both are laid out now --
flex, centred, no nudge -- and the anchors get `display: flex` so they hug their
glyph instead of a text line.
Help and Admin highlighted only a box around their text while Videos and
Statistics highlighted the full height. Same class, same rules, different
parent: the bar stretches its children, but the corner is a nested flex
container that centres its own, so a link inside it was sized to its content.
The hover background is painted on the link box, so that box IS the affordance.
The bar broke onto a second line at certain widths. Two measurement errors that
only overlap in a narrow band, which is why it appeared while resizing rather
than always. useOverflowNav measured the WRAPPER's offsetWidth and subtracted
only the home link and the corner, missing the bar's own `padding: 0 0.25em` --
about 8px it believed it had. And the hidden "More" placeholder it measured was
a plain button with no dropdown caret, about 21px narrower than the button that
eventually rendered. The placeholder is now the same component as the real
trigger, so the two cannot drift again.
Tested in ui-layout.cy.js against the real <DesktopNav/>, which takes its
colours, home link and indicators as props and so needs none of NavBar's polling
hooks. Indicator centres are measured against the bar's centre rather than each
other, because three things can be equally and consistently wrong and were. The
wrap case sweeps 1180px down to 640px in 6px steps -- a spot check steps over a
band that narrow. All three failed first: share 3.4px high, Help 24.8px against
a 54.4px bar, and two rows at 1156px.
Confirmed on the running app: every indicator at 0.0px from the bar's centre,
Help and Admin at the full 54.4px, and no wrap anywhere from 1500px to 780px.
1115 jest / 62 suites, 182 in ui-layout.cy.js, clean npm run build.
Measuring the foreground against the bar is worth nothing if the icons ignore
it, and two of them did -- the search glyph and the hamburger came out blue on
every bar in every theme while the rest of the bar was correct.
Neither inherits. An anchor takes `a {color: var(--blue)}` from tokens.css,
which outranks an inherited value however the bar sets it; the search glyph is
an anchor. A Mantine ActionIcon paints `--ai-color`, its own themed blue, and
Mantine writes that variable INLINE on the element, so a stylesheet declaring it
loses every time -- the painted property has to be set instead. The status
icons that link to /admin are anchors too, and only looked right because they
carry an inline colour of their own, which still wins and is meant to.
The test that claimed to cover this read `querySelector('...svg')`: one element,
the first, which was a plain inheriting Icon. It asserted every icon and
measured the only one that was never broken. It now reads querySelectorAll,
asserts the count, and checks all four -- and reddens in all four themes with
the rule removed, naming --blue as the value it found.
The gallery sample was wrong in the same direction: it used an IconButton for
the search icon, so it showed the ActionIcon fault twice and hid the anchor
fault entirely. It now carries all three shapes the real bar puts in that
corner.
1115 jest / 62 suites, 177 in ui-layout.cy.js, clean npm run build.
