mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-26 19:01:39 -04:00
2604 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Roland
|
5f422939c6
|
review: one inset, and two tests that could not fail
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. |
||
|
Roland
|
add67942e8
|
ui: three button-layout defects, each with a different cause
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.
|
||
|
Roland
|
5e15f89594
|
review: a busy button is not a disabled one, and night's danger is not red
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. |
||
|
Roland
|
1f890668b2
|
ui: a disabled button keeps its own colour
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. |
||
|
Roland
|
a31a0686b3
|
fix: an unnamed language code took down the whole Zim manage page
/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.
|
||
|
Roland
|
a17c1e526c
|
fix: toasts were covering the navigation bar's controls
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. |
||
|
Roland
|
738fb51d77
|
review: the spread was overwriting the stack, and the guard was blind
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. |
||
|
Roland
|
00978bc647
|
ui: only the top modal answers Escape, and the audited widths
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.
|
||
|
Roland
|
2aa5360932
|
review: measure what can be measured, and say what is not
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. |
||
|
Roland
|
3f1e0b386f
|
ui: the corner triggers, the More button, and the search modal
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. |
||
|
Roland
|
c056611912
|
ui: the navbar corner was nudged rather than laid out, and it showed
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. |
||
|
Roland
|
8dc11ddb3d
|
ui: the corner icons opt out of the bar's foreground, so make them stop
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.
|
||
|
Roland
|
bbb267aa83
|
ui: the navbar measures its foreground instead of assuming one
The bar's background is the one surface a user picks -- twelve colour names, which each theme resolves to twelve of its own colours. Forty-eight backgrounds, and the links and status icons were drawn on all of them in a single token, `--btn-text`, which is near-black in three of the four themes. Night's `--brown` is #451212. A near-black glyph on it is 1.33:1. Nine of night's twelve colours and seven of amber's sat below 4.5:1, and twelve of the forty-eight below 3:1 -- icons the same colour as the bar behind them. So the foreground is measured against the background the theme actually resolved, choosing between that theme's own `--btn-text`, `--black` and `--white`. Its own, not literal ones: a white pixel in night mode undoes the dark adaptation the theme exists to protect, so night's "white" is #ff8a8a. Measuring at runtime rather than shipping an `--on-red` token per colour also means the custom YAML themes still ahead get this for free. Nothing is below 3:1 any more and the worst case is 3.27:1, up from 1.33:1. No combination is worse than it was. Ten of the forty-eight are still under 4.5:1 and no foreground will lift them: a monochrome theme resolves all twelve names onto one ramp, and a mid-tone of a hue cannot carry text of that same hue. That shortfall is in the palette, so /theme-sample now draws all twelve bars with the measured ratio printed beside each one, calling out the ten -- it is a decision to make while looking at them. The WCAG primitives move to themes/contrast so theme code can measure a colour without importing Common.js, which pulls in half the component library. Common.js forwards `contrastRatio` with a delegating function rather than a bare re-export: six specs mock it with `{...jest.requireActual('./Common')}`, the spread invokes every getter, and those spreads run re-entrantly while the module is still evaluating -- a re-export getter fires before its import binding exists and takes the whole suite down. Tested: navColors.test.js measures all forty-eight against the palette parsed out of tokens.css, and holds Nav.js to the call by source since neither bar is reachable from a test that could measure it. ui-layout.cy.js measures the painted result in a real browser in all four themes and holds it against the number the component reports. Reverting to the fixed foreground reddens four jest cases and three cypress ones. 1115 jest / 62 suites, 177 in ui-layout.cy.js, clean npm run build. |
||
|
Roland
|
bbe3cac9a8
|
test: act on review -- the sprite gap, and the branch no e2e can reach
Both comments were right, and the second named a real blind spot. The e2e iterated a hardcoded theme list while the unit suite iterated `themeNames`, so a fifth theme would have got mapping coverage for free and no cold start. It imports `themeNames` and `themeSessionKey` now. The sprite was invisible to it. A wrong sprite name 404s the icons and MapLibre carries on, so the canvas exists and the suite passes -- exactly the case the review described. The spec now intercepts `/map-assets/sprites/**` and asserts the requested set is `mapSprite()`. Planted it by collapsing the sprite back into the flavor: night and amber get PAST the canvas assertion and fail on the sprite, which is the point. The rebuild branch cannot be reached from a browser at all. MapViewer builds a style twice -- at init, and in an effect keyed on `[theme, mapReady, scaleUnit]` -- but the theme picker lives on Settings and in the gallery, never in the nav, so there is no way to change the theme with /map open; navigating to Settings unmounts the viewer. A Cypress case that switches themes would have nothing to click. So that branch is guarded in the source instead: both `buildStyle` calls must pass `mapFlavor(theme)` and `mapSprite(theme)`, and none may pass a bare `theme`. Crude, and the honest alternative to a browser test that cannot reach the branch it claims to cover. Planted by breaking ONLY the rebuild call: the source guards go red and the e2e stays green, which is the whole argument for keeping it. The call-site scan counts parens rather than matching a regex. The arguments are themselves calls, so anything non-greedy stops at `mapFlavor(theme)` and reports half an argument list as the whole of it -- which is how the first version of this test failed while the code was correct. 1098 jest / 61 suites, 4 e2e themes, clean build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
9605c4d437
|
ui: the monochrome themes draw a black basemap, not a dark one
Roland asked why the red overlay sits on the DARK basemap rather than
greyscale or black. Measuring it, black is the better answer and the
reason is not brightness:
flavor distinct colours hued earth luminance
dark 19 4 0.014
black 13 0 0.007
Both media filters are the same pure luminance projection --
`0.2126 0.7152 0.0722` -- so they keep brightness and throw hue away.
`black` is already achromatic, so filtering it is a hue rotation and
nothing else: all thirteen greys were authored as a monochrome hierarchy
and every one arrives intact. `dark` has more colours, but four of them
are hued (water, parks, up to 25 chroma) and those distinctions do not
survive at all -- the features land at whatever brightness their hue
happened to carry rather than one anybody chose. Black also emits half
the light on the earth fill, which is the point of a night-vision theme.
`grayscale` is not an option despite the name: it is a LIGHT theme (earth
L0.604), and a luminance filter turns a light map into a bright red one.
What made black look impossible in the first commit was the sprite, and
that was my mistake: `sprite` and the layer flavor are separate style
properties that merely shared one variable. We ship `light` and `dark`
sprite sets, and a `black` basemap with `dark` icons is an ordinary style.
They are now separate arguments, with `mapSprite()` beside `mapFlavor()`.
Fixed in passing: the contour labels branched on `flavor === "dark"` and
would have taken the LIGHT branch for `black` -- dark brown text on a
black map. Now branches on `light`, so every dark flavor gets the pale
label.
The new test is the one worth having: the monochrome themes' flavor must
paint with greys only, paired with an assertion that `dark` is NOT
achromatic. Without that pair the first would pass on any flavor at all
and prove nothing about the choice.
1096 jest / 61 suites, 4 e2e themes, clean build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
83359c834e
|
fix: the map viewer was blank in night and amber
MapViewer handed protomaps-themes-base the WROLPi theme name as its
basemap "flavor". That worked while the only themes were `light` and
`dark`, and broke the day night and amber arrived: protomaps looks the
flavor up in a record and dereferences the result without checking, so an
unknown name is not a fallback but
TypeError: Cannot read properties of undefined (reading 'background')
at nolabels_layers (base_layers.ts:18)
thrown before the first tile. MapLibre never constructed, so /map
rendered nothing in two of the four themes.
`mapFlavor()` in themes/names.ts maps a theme to one of the two flavors we
can actually serve. Only `light` and `dark`: the flavor is interpolated
into `/map-assets/sprites/<flavor>` too, and those are the two sprite sets
we ship, so protomaps' `black` and `grayscale` would load a style and then
find no icons. Night and amber take `dark` and let the media filter tint
the canvas -- a filter shifts hue, so handing them `light` would give a
red or amber slab, which is the opposite of the point.
Two layers of test, because they cover different halves:
- map-flavor.test.js checks every theme against the REAL protomaps
(calling layers() and requiring it not to throw) and against the
sprites directory on disk, rather than a list of valid names copied
into the test, which is what would have passed here in the first
place. It fails when a fifth theme is added without a mapping.
- map-themes.cy.js drives the actual page in all four themes and waits
for the MapLibre canvas. The unit test cannot see whether MapViewer
USES the mapping, which is the half that was broken. Verified against
the planted regression: night and amber fail with no canvas, light and
dark still pass.
Not a bug: the 404 on /blobs/map-overview.pmtiles in the same trace. That
is a HEAD probe for an optional planet overview, gated on `resp.ok`, and
the blob is absent in a dev checkout. The console line is unavoidable.
1093 jest / 61 suites, clean build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
c534e4f5d8
|
dev: stop the app container linting on every rebuild
react-scripts lints in-process on every incremental rebuild. Each save pays for it, and the result is buried under a few hundred warnings the repo has carried for years -- `no-useless-computed-key` on the wire-gauge tables, `react-hooks/exhaustive-deps` across most of Common.js. Finding "Compiled successfully" in that takes a scroll. This container only serves; it never builds. Linting still happens where it is acted on: `npm run build` and CI both run it, and both fail on a real error. NOT a memory fix, though that is why I reached for it -- the dev server was OOM-killed three times in an afternoon and I claimed this would be the biggest saving. It is not. Measured both ways with the same procedure (recreate, load a page, wait for the typecheck, read `docker stats`) the container settles at 3.5GB with linting and 3.0GB then 3.7GB without it. The spread between two runs of the SAME configuration is wider than the spread between configurations, so the effect is below what this can measure. Raising Docker Desktop's VM limit from 8GB to 16GB is what actually stopped the kills. Kept anyway, on the time and the noise, which are not in doubt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
3204fb51ea
|
ui: centre the pagination strip
It sat hard against the left edge of its container despite three call
sites wrapping it in `<center>` or `text-align: center`.
Mantine builds a Pagination as a BLOCK root -- full width, so those
wrappers had nothing to centre -- holding a flex Group that carries the
controls and lays them out at `flex-start`. So `justify-content` on the
root does nothing at all; it has to reach the Group.
Setting `--group-justify` on the root does not work either, which cost a
detour: Group defaults that prop and writes the variable INLINE on its own
element, where an inherited value cannot reach it. The property itself is
fair game, since Mantine only ever reads the variable from a hashed class,
so `.wrolpi-pagination .mantine-Group-root { justify-content: center }` at
(0,2,0) is what lands.
The three wrappers are now plain divs keeping their margin. `<center>` is
a deprecated element that was doing nothing here.
Two tests: the gap either side is equal -- measured, not read off
`justify-content` -- with an assertion that the container really is wider
than the strip, since otherwise the equality holds trivially at 0 and 0.
And an inverse: at 240px nothing hangs off the LEFT edge. That one is the
risk centring introduces -- overflow in a flex-start row spills right,
where it can be scrolled to, but centred it spills both ways and the first
page becomes unreachable. It passed before this change and must keep
passing after.
Verified red first (0px left against 230px right), and the wrong-element
fix -- centring the root rather than the Group -- reddens it again.
1080 jest, 160 in ui-layout.cy.js, clean build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
a74e4c6fc4
|
fix: break the import cycle that put useCalculators in the TDZ
The dashboard threw on arrival from any other page:
ReferenceError: Cannot access 'useCalculators' before initialization
at Module.useCalculators
at DashboardCalculators
`Calculators.js` exports `useCalculators` as a `const` and imports the ten
calculators it renders. Five of those reached back up to `Apps.js` for
`ColoredInput`, and `Apps.js` imports `CalculatorsPage` from
`Calculators.js` -- so the hub was in a cycle with its own leaves:
Calculators.js -> calculators/ElectricalCalculators.js
-> Apps.js -> Calculators.js
Webpack compiles a `const` export to a live getter, and a getter read
before the module body reaches the declaration THROWS rather than
returning undefined. That is the whole mechanism behind this particular
error, and it names the const it could not reach.
`ColoredInput` is a text field with a Label welded to one edge -- the
calculators' unit markers. It has no business in the module that ROUTES
to the calculators, so it moved to `components/ui/Inputs.tsx`. Nothing in
`components/ui` imports from `components/`, so a control kept there cannot
close a loop like this again. Gallery entry added, per the standing rule.
`src/import-cycles.test.js` is the guard: a small import-graph walker,
scoped to hubs that export a `const` into their own subtree. It fails
with the cycle printed, and it self-checks against a known cycle so it
cannot pass by finding nothing. Verified red with the cycle planted back.
HONEST LIMIT: the guard is static because I could not reproduce the
RUNTIME error in a test. TDZ needs a module to be mid-evaluation when the
getter is read, and by the time any spec body runs every module has
finished; babel also papers over it, handing back undefined where webpack
throws. I tried an e2e that navigates to the dashboard and collects
window errors -- it passes with the cycle present, so it would have been a
test that cannot fail for the reason it claims, and I deleted it rather
than ship it. A react-refresh trigger did not reproduce it either. What
is checkable, and what is genuinely the defect, is the cycle.
1080 jest / 60 suites, 158 in ui-layout.cy.js, clean build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
455f42faae
|
ui: act on review -- corner chrome, a swallowed className, and the poster frame
Three real defects from the PR review, each reproduced before it was fixed.
The tag mark and a video's duration were pinned to the poster BAND, which
is the full width of the card, rather than to the painted image. Once the
height cap landed the two stopped being the same box: measured, the mark
sat 39px clear of a book cover, 56px clear of a small poster and 60px
clear of a vertical video, and the duration 55px clear. It went unnoticed
because on a 16:9 poster the image fills the band and the two pin to the
same place -- so the tests cover the height-bound ratios, and assert the
letterbox gap is really there before measuring into it.
CardPoster now centres a frame that shrink-wraps the image, and the frame
is what corner chrome is positioned against. VideoCard passes its
duration through a new `overlay` prop rather than wrapping CardPoster in a
box of its own.
The mark also went back INSIDE the link. Splitting it out made the
tagged corner a dead spot on an otherwise clickable poster.
CardLink and ExternalCardLink wrote `className='...' {...props}`, so a
caller's className replaced the pair instead of adding to it. Every
archive title passes `card-title-ellipsis`, so every archive title lost
`card-link` -- the one card type this PR migrates for layout was the one
missing the title treatment. Both compose now.
On the fourth comment, the breadth of the ActionIcon remap: it is
deliberate and the numbers are small. 59 IconButton call sites, 52 of
them at the default size, 7 at `xs` (one in Flasher, three in the
inventory schema editor). The one library control that stands beside an
input rather than a button is SearchBox's clear, and the remap is exactly
what that scale is for -- it now matches its field to the pixel, where
before it was 28px against a 36px input. Guarded by a test that fails at
42 vs 36 if the default is reverted.
1078 jest, 158 in ui-layout.cy.js, clean build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
bfb2e71d02
|
ui: an icon button is the same height as the button beside it
The Details/Open pair on every archive card came out 36px beside 28px.
Mantine gives ActionIcon a size scale unrelated to Button's, and defaults
the two differently:
size xs sm md lg xl default
Button 30 36 42 50 60 sm (36)
ActionIcon 18 22 28 34 44 md (28)
So the two never matched, at any size, and every toolbar mixing them was
ragged -- the archive card is just where it shows most.
Mantine already ships the aligned scale as `--ai-size-input-*`, for an
ActionIcon standing beside an Input; those five values are identical to
`--button-height-*`. ui.css points one at the other, which fixes every
size at once including ones no call site uses yet, and IconButton now
defaults to `sm` like Button.
Both halves are needed and both are proven so: dropping just the `sm`
remap reddens the default and `sm` cases while xs/md/lg/xl stay green;
reverting just the default reddens the default case alone, at 42 vs 36 --
the mismatch inverted rather than fixed.
A further test pins the direction. The pair could also be squared up by
cutting the labelled button down to 28px, which would be the wrong fix.
Gallery gains a row of Button/IconButton pairs at all five sizes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
687b41bf20
|
ui: cards get legible titles, contained posters, and meta beside the title
Four defects on /videos, /docs and /archives. Titles were invisible outside light. `.card-link` carried a literal `color: black` from before the themes existed -- about 1.1:1 on the dark, night and amber panels. It applies to every card title and to the channel/author/domain line beside it. The date was fine, since it takes `--muted` from the card body, which is what made this look like a card bug rather than a link bug. Titles now take `--text`: they are links, but they are the card's text, not a wall of blue. Posters were cropped, not mis-proportioned. The image carried a fixed 290x163 box; a card in the default grid is about 233px wide and Mantine's Card sets `overflow: hidden`, so the browser painted the middle 233px of a 290px image. The ratio was right in the layout tree and wrong on screen. Replaced with caps on both axes -- the card's width and `--card-poster-max-height: 200px` -- with `width`/`height` left `auto` so the pair behaves as "contain". Bounding the width alone was tried first and is not enough: a square thumbnail then stands as tall as the card is wide, and a portrait book cover taller still. The meta line was foot-anchored, so on /docs a PDF with no author had its date shoved up to 55px below the title, at the bottom of a card that was tall only because of its neighbour in the row. It now sits directly under the title. Archive cards read buttons-then-domain, because the buttons were children and the meta was what got pushed to the foot. Card gains an `actions` slot, rendered last and anchored to the foot, so a row lines its buttons up however many lines each title took. Gallery gains a poster row covering all five ratios and a row showing meta/actions against unequal titles. 1076 jest tests, 145 in ui-layout.cy.js, clean build. Every new guard was checked against a planted regression: dropping the height cap reddens the square and portrait cases and leaves 16:9 green; `width: 100%` reddens containment; restoring `color: black` reddens dark/night/amber and leaves light green; re-anchoring the meta and swapping the slot order redden both the jest order test and the cypress geometry tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
928bdbdfa1
|
ui: lift warning above ordinary text in the monochrome themes
A load reading under warning rendered marginally QUIETER than a healthy one:
an uncoloured value inherits `--text`, 4.98:1 on a night panel, and `--warning`
was 4.91:1. Amber had the same inversion, 7.43 against 8.59.
Anything that means "look at this" has to be louder than the prose around it,
and with no hue to carry the signal, louder means brighter. Both `warning` and
`danger` now clear `--text` in night and amber:
night warning #ee3434 -> #f54040 4.91 -> 5.40 (--text 4.98)
danger #ff3e3e -> #ff5757 5.71 -> 6.41
amber warning #f18806 -> #fa9f31 7.43 -> 9.05 (--text 8.59)
danger #ff9e29 -> #ffb760 9.15 -> 10.96
I said last time this would trade night's "red only" character for loudness.
Having measured it, that was overstated. Red contributes 21% of luminance, so
in night NOTHING above ~4.9:1 can be a pure hue -- `#ff0000` itself is only
4.99:1 there. `--text` is already `#e04a4a`, carrying 74 of green and blue.
The new warning carries 64, LESS than the text beside it; only danger goes
further, at 87. A pure-hue ramp above the text was never on the table.
The rule is monochrome-only, like the ordering rule it sits beside. A light
theme's text is near-black at ~14:1 and no warning colour will ever match it --
there, hue is what marks a reading as flagged.
The guard now asserts warning as well as danger, and the LoadStatistic browser
test makes the claim it previously had to document as unmet: warning is louder
than a plain reading. Planted the reported bug back in both themes, plus two
ordering breaks.
One plant of mine came back green and was wrong, not the test: I set night's
danger to `#f04a4a` expecting it to drop below warning, but that has MORE green
and blue than `#f54040`, and green is 71% of luminance -- so it was still the
brighter of the two. Re-planted with a value that is genuinely dimmer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
1ef3b8552e
|
test: act on review — a test that could not pass with Reduce Motion on
The indeterminate test read the band's position out of a running animation and asserted it had left the box. Two problems, and the second is the serious one: it samples a moving target, and under `prefers-reduced-motion` the slide is replaced by `width: 100%` with no negative margin, so the band never leaves the box and the assertion is permanently false. Anyone with Reduce Motion turned on -- or a CI host with it set -- gets a hard failure on a correct build. Reproduced it rather than taking it on faith: forcing the reduced-motion branch for the whole run fails the old form and passes the new one. The band is pinned now, to where the animation's first keyframe puts it, and the assertion is on the clip plus that escape. Deterministic, and true on any machine. Added the other branch too: with motion suppressed the bar fills instead of sliding, and a band that never moves must never leave the box. `text-overflow: ellipsis` on the label. `nowrap` alone stopped it wrapping and being bisected by the clip, but left a hard mid-glyph cut, and the call sites that feed this are exactly the ones with long strings -- Upload.js and FileBrowser.js label with a filename, Files.js with the error string. The one-line test was close to circular: with `nowrap` applied, one line box is almost a restatement of the rule. It now also asserts the computed `nowrap` and `ellipsis`, and that the line is genuinely WIDER than the space it has -- without that, a label that simply fits would satisfy it. Dropped a dead ternary from the air-around-label test; it built a rect nothing used and made it look as though the measurement came from somewhere it did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
5df38916f1
|
ui: progress bars get room for their label, and keep their band inside
Three things, plus an answer to why two sets of bars on the same page were spaced differently. The bar was 16px tall with an 11px bold label, leaving about half a pixel of air above and below. On the Status page that is 44 of them. Now 22px with a 12px label -- these labels carry device names and units, not just a number. The indeterminate band escaped the bar. It slides on `margin-left` from -35% to 100%, and nothing clipped it, so the animation ran from outside the left edge to outside the right one. `overflow: hidden` on the bar. The CPU and RAM bars were touching while the drive-bandwidth bars on the same page were not, and the difference was not the bars: CPU and RAM are bare siblings inside a Panel, while each bandwidth bar sits alone in a Grid column and takes its spacing from the Grid. `* + .wrolpi-progress` distinguishes those two by itself -- it fires only when a bar actually follows something, so an only-child bar in a column is untouched. A blanket margin would have doubled the Grid's gutter, and that is asserted too. The gallery gains a Progress section: six percentages, custom labels, no label, indeterminate, and the four roles. Its own demo gaps were removed so what it shows is the spacing the component really has. MEASURED while building the gallery, and NOT fixed: the label reads 5.49:1 against the fill in light -- the brightness filter is what buys that -- but 2.35 in dark, 2.48 in night and 2.74 in amber. Against the empty track it is 5.2 to 15:1 everywhere. Reaching AA over the fill needs the label drawn twice and clipped at the fill boundary so each half takes its own colour, and in night not even that suffices: `--text` is only about 4:1 against pure black and a fill is lighter than black by definition. Recorded in ui.css and in the test, which asserts the track half only rather than picking a threshold low enough to pass. One test was unfalsifiable and is rewritten. "Keeps a long label from widening the bar" cannot fail: the label is `position: absolute; inset: 0` and takes no part in its parent's width. What a long label actually does is wrap to a second line inside a 22px bar and get cut in half by the new clip, so the test measures line boxes now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
fc0958369b
|
test: act on review — and one of the seven turned out to be undemonstrable
Six real fixes and one honest retraction.
The control is now a SIBLING of the heading inside a flex row, not a child of
it. Inside the `<h3>` it joined the heading's accessible name, so screen reader
heading navigation announced "Root CA Certificate Help". The row is what keeps
them on one line, and it is only rendered when there is something to sit beside,
so the markup for the other 46 call sites is unchanged.
`style` now lands on the wrapper. It used to reach the outer div; going through
Header put it on the inner heading, where a margin sets space INSIDE the wrapper
and only worked at all through margin collapse -- which stops the moment the
wrapper gains padding or a border. Status's "Drive Bandwidth" is the live case:
verified 16px on the block, 0 on the h2. Text properties still arrive, because
colour and text-align inherit; the `color` PROP stays on the heading.
A `<label>` only when `for_` names a field. Several call sites pass an
InfoHeader as the `label` prop of InputForm/ToggleForm, which wraps it in a
label of its own -- an unconditional one nested labels, which is invalid and
makes clicking the text ambiguous. Verified: zero `label label` on
/videos/settings, where "Video Resolutions" keeps its label and "Video File
Format" no longer has one.
The help controls carried `iconStyle: {margin: '0.5em'}`, which separated a
free-floating sibling and, inside the row, added to the gap on every side.
Zeroed; the row's gap is the spacing.
The browser test mounted a plain Button in an `after` slot rather than a real
HelpHeader, so the default icon chrome above was exactly what it could not see.
It mounts the real wrapper now. Two of my assertions were also too weak to
catch what they claimed: the gap was measured to `.wrolpi-header-after`, a flex
container whose own rect does not move when a child gains a margin, so it had to
measure the button; and the squeeze case used a title of ordinary words, which
simply wraps and never puts the row under pressure.
Renamed the suites -- "keeps the icon on its own line" read as the opposite of
the fix.
The retraction: `flex-shrink: 0` is declared but has NO test. Three attempts
could not find a width where removing it changes what is painted -- the row's
only other item is a heading, and its automatic minimum size absorbs the
pressure; the help control is an ActionIcon with a min-width it will not go
below either. The declaration stays as a statement of intent, with a comment
saying it is unproven, rather than a green test that does not discriminate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
c18fe84783
|
ui: headings open their section, and keep their help icon on one line
Two things, and the second was a regression this migration caused.
A heading had `margin: 0 0 10px` -- no top margin at all -- so it sat flush
against whatever came before and read as part of it rather than as the start of
the next thing. On /more/statistics "Tags" touched the panel above it with a
0px gap. Now 22px above and 10px below, because a heading belongs to what
follows it.
`* + .wrolpi-header` rather than a blanket margin, for the same reason
StatisticGroup needed it: a heading that opens a Panel or a page is already
where it should be, and pushing it down just dents the top of its own container.
The help icon on every HelpHeader and InfoHeader had dropped to its own line --
"Root CA Certificate" in Settings is the clearest one. `.inline-header` set
`h1..h5 {display: inline-block}` so the heading and a sibling `<span>` shared a
row, which worked while these were a bare `<h2>` and stopped the moment they
gained a block wrapper: the `<label>`'s block child forced a break.
Fixed in the library rather than patched in App.css. `Header` takes an `after`
slot rendered inside the heading element, where the existing flex row already
handles alignment and spacing. Both wrappers pass their icon through it, the
`<label>` moves inside the heading so `for_` keeps associating the text with its
field (three call sites rely on it), and the `.inline-header` rules are deleted.
Fifteen call sites are fixed by fixing the two wrappers.
Tests: containment in jest, and the part jsdom cannot judge -- that the two
boxes actually share a line, and that the heading stays one row tall -- in a
browser. Planted both original shapes: the margin removed, a blanket margin
denting its container, and the control moved back outside the heading element.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
2ce7601ebb
|
test: act on review — the browser test named LoadStatistic but never mounted it
The suite was titled "a load reading ranks itself", commented as covering the thresholds, and cross-referenced from Common.test.js as the place the painted result is checked. It mounted a hand-built `<Statistic color='warning'/>`. So it proved that role tokens resolve distinctly -- which the roles suite two blocks up already covers -- and would have stayed green with LoadStatistic reverted to `orange`/`red`. The same shape as the column-border test in #609: a claim that outran its measurement. It mounts the real helper at the real thresholds now, and the reversion fails it. The source guards are gone. They scraped function bodies out of files with indexOf slices, covered two helpers, and never executed a branch. `loadRole` and `healthRole` are pure exported functions instead, tested as mappings: either side of both load thresholds, every SMART assessment, lower case (the value comes from smartctl), and the case that matters most -- an unreadable drive must report as unknown, never as healthy. That is also the better answer to the jsdom problem that produced the guards. jsdom rejects `color: var(--danger)` and drops it, so the mapping cannot be read back off a rendered component; extracting it is the honest fix rather than asserting on source text. And the gallery was itself half-migrated: 15 Min. Load became `danger` while 5 Min. Load stayed `orange` -- in the one sample whose caption tells the reader to go and check night and amber, where orange is byte-identical to --text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
7a6b09aee6
|
ui: route genuine success and failure through the roles
Sixteen call sites, chosen by reading each one rather than by grepping for a
hue. Four sub-agents judged them independently against one rule: convert only
what genuinely means success or failure; identification, category and
decoration stay hue-named.
Converted:
SMART drive health (all four branches -- PASS/WARN/FAIL/unknown)
config validity: valid / invalid / empty
"Failed to load preview"
the two "Download error" triggers
expired inventory items, desktop and mobile
the refresh phases: complete / active / not reached
a refresh Progress bar in its error state
cookie status: Unlocked / Locked
a chip-mismatch label on the flasher
CPU load thresholds (LoadStatistic)
the outcome of an API call in useAPIButton
the gallery's "Danger zone" heading and its load readings
Left alone, deliberately:
the file browser toolbar -- every action has its own colour to be told apart,
and delete already uses role='danger' (Roland's ruling)
diff add/remove in the config and inventory import previews. Adding is not a
success and removing is not a failure; in Overwrite mode the removals are
exactly what the user asked for, and painting them danger would report their
intended action as broken
seasons in DatesSelector, the uploader star and favourite heart, TagIcon,
downloader category buttons, "Recommended to Keep"
Three sites were wrong in a way worth naming. Both "Download error" triggers
guard on the same condition and render the same modal, but one was `orange` and
the other `red` -- and `orange` is the single worst choice, byte-identical to
`--text` in night, so that error button was indistinguishable from an ordinary
control. Config validity drew its valid state in `--violet`, which no user
parses as "valid". And `LoadStatistic` -- the thing users actually watch on the
Status page -- rendered its warning threshold in that same invisible orange.
Also fixed a stale comment in Videos.js that claimed no token existed for these.
Two notes on the tests:
The first LoadStatistic test I wrote rendered the component and asserted the
inline colour. jsdom REJECTS `color: var(--danger)` as an invalid declaration
and drops it, so every colour reads back as the empty string -- two of the four
assertions passed for that reason alone and would have kept passing with the
hues restored. It is a source guard now, with the painted result asserted in
the browser instead.
That browser test then found something real: an uncoloured reading inherits
`--text`, which in night is 4.98:1 against `--warning`'s 4.91:1, so a machine
under load is marginally QUIETER than one that is fine. Not fixed here. The
only way up on a dark panel is lightness, and night's danger would desaturate
from #ff3e3e to about #ff5d5d -- trading the theme's "red only" character for
loudness. That is a palette decision, so the test documents it and asserts what
does hold rather than quietly making it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
b7ef6af8bd
|
test: act on review — and correct an overstated claim about the toast bug
I said all four toast kinds were "the same pixel" in night. They were not.
Planting a reversion to hue names left the new distinctness test GREEN, because
in night `--blue`, `--green`, `--yellow` and `--red` are four different values
(2.29, 3.14, 4.52, 5.64 against a panel). What they are is cramped and badly
ranked, with info at 2.29:1 -- below the 3:1 floor, present and invisible.
The byte-identical pairs are real but elsewhere: `--orange` is `--text` and
`--yellow` is `--amber`, which bites a call site that reaches for those names,
not the toast mapping. The PR body has been corrected too.
So the distinctness assertion is not the one with teeth; the legibility floor
is. The monochrome toast test now measures every kind against the toast's own
surface, and the hue reversion fails it naming the number: 2.29:1.
Also from review:
`.wrolpi-message-error` still forced `border: 1.5px dashed var(--red)` while its
accent and icon had moved to roles -- nearly the same pixel in night, so no
colour test could see it. It follows `--message-color` now. Six more library
rules were naming a hue for a thing that means danger: the night danger button,
invalid form fields, invalid paths, path error text, danger panels and failed
table rows. All roles now, with a source guard that fails any rule whose
SELECTOR means severity and whose body names a hue. Focus rings and active-tab
accents deliberately keep `--blue`: those are the primary accent, not severity.
The tokens.css note claimed the brightness ramp held "in every theme" when the
tests deliberately limit it to monochrome. Narrowed, with the reason spelled
out, because the next person to read it would otherwise have "fixed" light to
match the prose -- which would mean reordering light's hues to satisfy a test.
Added the end-of-chain cases the review asked for: four toasts and four Messages
in night and amber, measured rather than inferred from variable names.
Colour reads now go through a strict `toRgb` that refuses anything not already
resolved. The reviewer's concern that `getPropertyValue('--info')` returns an
unresolved `var(--blue)` does not hold -- custom properties are substituted at
computed-value time and it returns `#20629c` -- but feeding an unresolved value
to a luminance calculation yields NaN, and `expect(NaN).to.not.equal(x)` passes.
Worth refusing loudly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
4341350c40
|
ui: semantic roles, so night and amber can tell severity apart at all
A component now asks for what a colour MEANS -- `danger` -- and the theme
decides how that looks. Five roles: neutral, info, success, warning, danger.
The names are the ones already in the tree. `--primary/--success/--warning/
--danger` were declared in tokens.css with ZERO consumers, and `MessageKind`
was already `info|success|warning|error`; this is one vocabulary instead of
three. `error` stays a valid kind because 26 call sites write it, and `danger`
is the same thing under the token's name for new code.
The token set is the easy half. Light and dark spend a hue per role. Night is
red and nothing else and amber is amber and nothing else, and there a role has
to be a step on a BRIGHTNESS ramp -- which is why this was worth doing:
night --orange is byte-identical to --text
--yellow is byte-identical to --amber
amber the same collisions, one hue over
A call site picking between hue names in those themes is picking between
synonyms. That is why all four toast kinds were the same pixel in night, and
why an error and an informational message were indistinguishable.
Both ramps are built rather than copied: every role clears 3:1 on the panel
(neutral is allowed lower -- for pending and disabled, dim IS the signal),
severity is strictly increasing in contrast, and danger reaches that theme's
own --text brightness so the loudest role is never quieter than prose. Amber
needed its own scale, not night's: its --text is 8.6:1 where night's is 5.0:1,
so night's ramp would have left every role dimmer than ordinary text.
Fixes in passing: night's info was 2.29:1, under the 3:1 floor. And Status
loses its three `html[data-theme="night"]` overrides entirely -- it was the one
component already encoding severity by brightness by hand, and the roles are
that generalised.
Roles are registered as Mantine colors too, so `color='danger'` works anywhere
`color='red'` does.
`isMonochromeTheme` is a new predicate in themes/names. It is the same set as
`explicitOnlyThemes` today and deliberately separate: one is about how a theme
is chosen, the other about what a theme has to work with.
Scope: tokens plus the library. The ~24 app-level `color='...'` call sites are
a follow-up -- they need a judgement per site, and two of them already disagree
(Downloads.js colours the same "Download error" label `orange` on one line and
`red` on another). The 22 `violet` uses are not semantic at all and stay hues.
Tests: the ramp claims are asserted per theme in a browser, and the ordering
and danger-vs-text claims run ONLY for the monochrome themes -- in light, blue
is darker than green on white, so a brightness ramp there would fail a design
that is perfectly fine. Every guard checked red against a planted regression,
including the original bug (warning and danger collapsed to one value in night)
and night's info back at its old 2.29:1.
The roles were folded into each theme's own block rather than kept in a
standalone one, so the existing "same tokens in every theme" and "only red hues
in night" guards cover them for free.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
2fda9e8975
|
test: act on review — two of the new table guards could not fail
The jest column-border test asserted the TABLE lacks `data-with-column-border`.
Mantine puts that marker on the cells, never on the table, so the expectation
held whether column borders were on or off. It now looks for the marker
anywhere in the tree.
I did plant a regression for this one -- and ran it against cypress only, where
a different test caught it. The plant was right and the suite was wrong, which
is a new way to get a false green and worth naming: run every suite that claims
the behaviour, not the one that happens to be open.
The header-surface test fell back to `getComputedStyle(documentElement)
.backgroundColor` for an unstriped row. `body` carries the page colour and the
root element carries none, so that is `rgba(0, 0, 0, 0)` and any opaque header
passed. It now compares against the `--bg` token, which is what actually shows
through an unstriped row, and additionally asserts the body really is striped
so both kinds are covered. Planted the exact case the old form could not see
-- a header painted `--bg` -- and it goes red in all four themes.
The file-row fixture rendered a 16px icon; the app renders 24. A file row
calls `<FileIcon size={null}/>`, and a null size falls past Icon's 'small'
default to Tabler's 24 -- the harder centring case, and the one the CSS comment
describes. Added the two-glyph row as well, since a folder that is also ignored
draws two icons before its name and those touched each other too.
The header-rule-width assertion now runs per theme with the rest of the suite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
eab13e4803
|
ui: give tables a real header, drop the column grid, quiet the row rules
Three things the file browser made obvious.
The header was not a surface. Mantine gives thead no background at all, so
the header painted whatever was behind the table -- which is exactly what an
unstriped body row paints. Against a striped body it read as one more stripe,
and bold type was the only thing distinguishing it. It now carries `--head`,
the token that already means "recessed surface", with a full-strength 2px rule
under it: the header/body division is the one line in a table that carries
meaning, so it is the one allowed to be loud.
Column borders are gone. A line around every cell is a lot of ink to repeat
what the column headings already say, and it was half of why the header
disappeared -- a bordered header cell looks like a cell in a grid.
Row rules were `--border`, the same weight as the outline around the table, so
a long table read as a stack of boxes. They now use `--table-line`, a new
token per theme sitting between the surface and `--border`. The table outline
is not a row rule and keeps `--border`; Mantine draws both from
`--table-border-color`, so that had to be restated.
In the file browser an icon and a filename are inline siblings in one cell. An
inline SVG sits on the text baseline, so a 24px glyph beside 13px text hung
above it -- the icon looked centred in the row and the name looked like it was
resting on the icon's floor. `vertical-align: middle` centres it, and the
margin is a gap that was never there at all: the folder icon touched the first
letter of its own name.
Cypress now loads App.css, as the app does. Without it a spec covering the
file browser rows measures a component the app never renders.
One jest test and eleven browser tests, each checked red against a planted
regression. Two notes for the next person:
- `html .mantine-Table-table` is specificity, not decoration. Mantine
declares --table-border-color on its own hashed class, which ties with
`.mantine-Table-table` and wins on source order.
- the "quieter than the outline" test measures both lines against `--panel`.
Nothing paints <html> under a mounted component, so measuring against the
root background compares them to black, which ranks the *paler* line as the
louder one -- the assertion passed backwards until it was pointed at the
surface the rules actually sit on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
450aaa7334
|
ui: map the twelve icon names that were rendering as question marks
The calculators page had seven "?" glyphs in a row -- `cogs`, `tint`, `food`,
`signal`, `thermometer`, `car`, `th large` -- and `arrow left` was unmapped
while `arrow down`, `right` and `up` were all there. Also `tags`, `comments`,
`chevron down`, and `closed captioning outline`.
An unmapped name is not subtle: it renders IconHelpCircle and logs a console
error. It shipped anyway, because nothing failed on it.
Three tests already guarded this and all three missed it. They cover the
theme picker's choices, the names the mimetype helpers can return, and the
map's own integrity -- none of them looks at what call sites write. The new
guard walks src/ and checks every name it finds, in each of the four
spellings, each of which was hiding at least one of these:
<Icon name='x'/> and icon='x' the plain JSX prop
{icon: 'x'} a data table; how the calculators declare theirs
icon={active ? 'tags' : 'tag'} a ternary, where `tags` was unmapped
name={s === 'descending' ? ...} a comparison operand, which is NOT a name
The last one is why the extraction strips comparison operands first: the sort
arrow in DataTable tests a direction, and reading 'descending' as an icon name
would be a false positive that trains everyone to ignore this test.
Verified by planting each spelling in the file it was found in, and by
reverting the map additions -- the guard reports them. Also swept thirteen
routes in a real browser with console.error hooked: no icon errors left.
Drops `taggedImageLabel`, a Semantic Label prop bag with no consumers, whose
`corner: 'left'` was never an icon name at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
b15a9408f1
|
test: act on review — the skeleton guard was reading the wrong layer
The "separates the skeleton from the panel" cases could not fail. A Skeleton
is three layers: the element paints nothing, ::before paints
--mantine-color-body (`--bg`) at z-index 10, and ::after paints the bar the
user actually sees (`--head` in light, `--border` in dark) at z-index 11, on
top. `paintedBackground` walked outward from the element and returned the
first paint, which is ::before -- so all four cases compared the page
background to the panel. Those differ by design in every theme, so the bars
could have collapsed into the panel entirely and every assertion stayed green.
My planted regression did not catch it because the plant was broader than the
claim: it forced all three layers to the panel colour, so the test went red
for a reason the fix did not depend on. The plant now sets only ::after, and
a second plant sets only ::before and must stay GREEN.
The layer order is now the caller's argument, since "the background" is
ambiguous once pseudo-elements are involved and guessing makes a test vacuous
rather than wrong.
Also from review:
The disc on a filled button was pinned to `--blue` at the call site, but WROL
mode disables that button and Mantine repaints it `--mantine-color-disabled`
-- a blue chip on a grey button, the original hole inverted. The surface now
comes from the cascade (`--button-bg`, and the disabled colour for any
variant), so the call site says nothing at all and cannot be wrong about a
state it does not control. Keying on `[data-variant="filled"]` alone matched
none of our buttons: Mantine omits the attribute when a call site names a
colour but no variant, which is how every filled button here is written.
The Checkbox behaviour test asserted only that a handler fired, against a box
held at `checked={false}` that could never change. It is now uncontrolled and
asserts the box is checked, plus a second test that the handler sees the new
state -- read inside the handler, because `currentTarget` is null by the time
a mock is inspected.
The empty-caption test counted every div in the container; it counts the
loader's siblings now. Two comments corrected: Pill emits no `--pill-radius`
at all and its own stylesheet falls back to `1000rem`, and the nav bar no
longer relies on the default surface.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Roland
|
affe5869f5
|
ui: test the last four components, and put the last four in the gallery
Closes the tail of the library: `Loader`, `Loading`, `Placeholder` and `Checkbox` had no test of their own, and `IconStack`, `MediaFilterToggle`, `MultiSelect` and `NumberInput` were not in the gallery, which the project rule says every component must be. Putting them in the gallery is what found the rest of this. An IconStack's corner glyph paints a disc of the surface behind itself so the two sets of strokes do not cross, and that surface was hardcoded to the page background. It is right in exactly one of the three places a stack appears. The nav bar showed a white disc on the red bar, and the file browser's "New Folder" button showed one punched through the blue fill. It is now `--icon-stack-bg`, defaulting to the page: the file browser passes the button's fill, and the nav bar sets it on the <nav> beside the colour it already writes there, so it inherits -- the bar's colour is a user setting and cannot be named by a token. A MultiSelect's chosen options are Mantine Pills, and Pill is the one component that ignores the radius theme: it writes `--pill-radius: rem(1000)` onto the element, which zeroing `--mantine-radius-xl` cannot reach. They were lozenges on a page of square boxes. Twelve jest tests and sixteen browser tests. The browser half covers what jsdom cannot judge: that a Skeleton is a different colour from the panel it covers in all four themes (its greys are remapped to our tokens, and neither `--border` nor `--bg` was picked with a panel behind it), that the loader colour resolves through the token, and that the corner disc matches its surface. Every one was checked red against a planted regression first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
b36f7beeb8
|
test: act on review — observe the wiring, and stop the soft assertions
All five points were right.
* The gallery's Toasts copy still read "Bottom right" after the container
moved. Anyone using that page as a visual checklist would have looked in
the wrong corner and concluded the change had not landed.
* Nothing observed ThemeProvider's own wiring. Both suites mount their own
`<Notifications position='top-right' limit={5}/>`, so moving the container
back, or dropping it, would have failed neither -- while the comments
claimed "container missing while call sites still fire" as the failure
already shipped once. Three tests in ThemeProvider.test.js now raise a
real toast through the provider and assert it appears, and where.
Asserted through a toast rather than by querying for a container: Mantine
renders an empty container for every position it supports, so the obvious
selector returns `top-center` no matter how the provider is configured.
That is what the first version of this test did, and it failed for that
reason rather than the intended one.
The five-at-once test is honest about what it covers: Mantine's own
default limit is also 5, so it does not prove the prop is there. It
catches the limit being raised.
* The default duration was untested. Sleeping 150ms and finding the toast
still up rules out "vanishes next tick" and nothing else -- it passes for
a 200ms default, for `false`, and for Mantine's own 4000ms. Read the
deadline back off Mantine's store instead: 5000 when unset, the caller's
number when given, `false` for 0.
* Space activates a followable toast in `toast.ts` and only Enter was
covered, so dropping that branch stayed green.
* Colour assertions used `toContain('red')`, which any declaration
containing those three letters satisfies. They match the whole custom
property now.
Planted four regressions to check the new tests: position, limit, the
5000ms default, and the Space branch. Three went red; the limit one did
not, for the reason recorded above.
|
||
|
Roland
|
664c7d2219
|
test: prove the toast contrast fix in a browser, and fix the harness that hid it
The title contrast was verified by hand and by nothing else. jsdom cannot check it: it resolves no `var()`, so the title's colour there is the literal string `var(--mantine-color-white)` and every contrast assertion passes. Six browser tests now cover it -- title and description at WCAG AA in each of the four themes, the title staying heavier than the message where night and amber drop its muting, and the toast taking `--panel` for its surface. The three that can fail were checked against the fix removed: 1.13:1 in dark, night and amber. Light passes either way, correctly, because Mantine's light-scheme title uses `--mantine-color-black`, which the bridge maps to `--text`. Getting there took three corrections, all of them mine: * `cy.mountUI` never set `forceColorScheme`, so every theme rendered against Mantine's LIGHT scheme. Mantine keys its own rules on data-mantine-color-scheme, and ThemeProvider sets it from the theme -- night and amber ride on dark. Any per-theme test could therefore pass on a page that was broken in the app. This is the harness the tag contrast tests use too. * The support file never imported `@mantine/notifications/styles.css`, which App.js does, so a Notification rendered unstyled and had no surface or title colour of Mantine's to be wrong about. * The spec's own helper mounted a second time to raise the toast, which reset the theme to the default. All four per-theme cases ran in light. For the record, the chain the fix breaks: `.m_3feedf16` (the title) in dark scheme is `color: var(--mantine-color-white)`, the bridge aliases that to `--btn-text`, and `--btn-text` is the text drawn on a filled button -- near-black in dark, night and amber. Mantine writes it through `:where(...)`, which has zero specificity, so a single-class override plus `html` outranks it. |
||
|
Roland
|
de0f70eefc
|
ui: restore the clickable toast the migration dropped
No, it did not still work. Semantic's toast took an `onClick` in its options and made the whole toast activate it. The migrated helper destructured four keys and dropped the rest, silently, so every toast that carried one became a dead toast. `Events.js` still passes one -- it did before the migration and was never changed -- for the three events that arrive with a URL: * `user_notify_message`, a page shared from the browser extension * `upload_archive` * `screenshot_generated` The first renders the description "Click here to view the shared page", which it was not. Nothing threw and nothing looked wrong; the toast simply stopped being a way to reach the thing it was announcing. `onClick` is passed through again, and a toast carrying one gets a pointer cursor so it looks like what it is. Clicks on the dismiss button do not follow it -- the button is inside the toast, so its click reaches the root, and closing a notification is not the same as following it. It is also reachable from the keyboard now, which it never was: WROLPi runs on devices with a keyboard and no mouse, and a toast only a pointer can follow is a feature half the deployments cannot use. No `role`, because a `button` or `link` role may not contain interactive content and the dismiss button is in there. Mantine also leaves that dismiss button unnamed, so a screen reader read it as "button". It has a label now, which is how the test for the dismiss-does-not-follow case can find it. Five tests, all red first. The gallery gets a Clickable button; following the toast raises a second one, so it is obvious whether the handler ran. |
||
|
Roland
|
213b940a1b
|
ui: toasts move to the top right, get readable titles, and get tests
The title was unreadable in dark, night and amber. Mantine paints a dark-scheme notification title with `--mantine-color-white`, and the bridge aliases that to `--btn-text` -- deliberately, because it is the text drawn on a filled button, near-black in those three themes. So the title came out rgb(28, 31, 33) on a rgb(38, 41, 43) surface: about 1.1:1, present and invisible. Fixing the alias is not an option, it is correct for the buttons it exists for, so notifications now take their surface, border and text from our tokens instead. Measured after: 14.9, 11.9, 5.0 and 8.6 to one. The description follows the same route, and night and amber drop its muting. `--muted` on a panel is 2.0:1 in night and 3.1:1 in amber -- the description is the message itself, so those two let the title's weight carry the hierarchy instead. (`--muted` being that dim in night is not only a toast problem; statistic labels and inactive tabs use it too. Left alone here.) Position moves from bottom right to top right. Bottom right put toasts over the file browser's sticky footer and a video's player controls, which are the two places a background task is most likely to report while the user is doing something else. Gallery gets a Toasts section: one button per type, one that stacks a numbered toast per press, one that fills the container to its limit of five, one that never expires, one with no title, one with text long enough to wrap, and clear-all. A toast is the one component that cannot be seen by loading a page, so it needs buttons rather than a sample. Tests ----- Thirteen, where there were none for thirteen call sites -- and `toast` has already failed once app-wide, when App.js dropped Semantic's container while seven pages still imported the old helper and every notification silently went nowhere. Six were verified red against planted defects (a shared `id`, which makes Mantine replace instead of stack; one colour for every type; `autoClose` taking the 0 literally). The duration tests use real timers with short durations, which matters. With fake timers the element is never removed from the DOM -- hiding a notification runs an exit transition, and advancing the clock, even awaited inside `act`, does not complete it. Both the assertion and its control then read the same whatever the code does. The test for `time: 0` passed against a planted `autoClose: 0` twice before this was rewritten: once asserting synchronously, once with a control that could not fail either. It now waits for a normally-expiring toast to disappear first, so dismissal is demonstrably working before the survivor is asserted. |
||
|
Roland
|
db3889589f
|
ui: two more labels were painting text with an inline colour, and shorthand hex
Both from review, and both correct. * The tag edit preview and the Flasher's chip badges still wrote the calculated text colour to `color`. That is the same defect the last commit fixed on the tag chip itself: an inline declaration no stylesheet rule can outrank, so night keeps the colour calculated for the fill it has just discarded and a bright fill leaves black text on near-black. The preview is the worse of the two -- it is the preview of the colour being chosen, so it was misrepresenting what the tag would look like at the moment the user picks it. It now shows the tag shape as well. Fixing the first site did not fix these, and the stylesheet comment explaining the night and amber override already named the Flasher. So there is a source guard now: no element carrying `wrolpi-label` may set `color` in its style object. Verified against a planted violation, which it reports by file. * `hexToRGBArray` rejected the shorthand `#fff`, which left the luminance at zero -- a near-white tag was read as black and handed light text. Reachable because tag colours are not only picked in the colour picker: they are stored in a config, and a config is a file a user edits by hand. Three tests were red first, including one asserting the shorthand and the six-digit form of the same colour agree. |
||
|
Roland
|
92059f8342
|
ui: sit the tag's point on the visible edge, not a pixel inside it
`right: 100%` positions against the containing block's *padding* box, and the tag body carries a 1px border, so a square centred there sits a pixel inside the visible left edge. The point read as set into the tag rather than growing out of it. Nudged left by exactly that pixel, with the margin that reserves room for it following. Two pixels was too far: the point's edges then meet the body's left edge short of its corners, and the difference shows as a gap on the inner side. The geometry test that was supposed to cover this could not tell the difference. It asserted the tip lands far to the left of the body, which is true either way -- the un-nudged tip is only 2px further right and still well clear. The claim is about where the point is *centred*, so it measures that now, and was checked against the un-nudged version. Its neighbour, which checks the left margin covers the protrusion, is deliberately still geometry-agnostic: the margin-to-protrusion relationship is the invariant, and the exact offset is a matter of eye. It also now derives the protrusion from half the square's diagonal rather than half its side, which had made it loose enough to pass against a margin 5px short. |
||
|
Roland
|
fe92455ee6
|
ui: an inactive tab was a near-white block in dark, night and amber
Found while checking tags in the gallery, and unrelated to them. A tab is a NavLink in the app's nav bars, but a <button> on the Inventory page and in the theme gallery. A button carries the UA's own `buttonface` background -- rgb(239, 239, 239) in Chrome -- and only `.wrolpi-tab-active` set a background, so every inactive tab painted that near-white block. Invisible in light, and a bright patch in the other three; in night it is the one thing the theme exists to prevent. One line, and a test in all four themes. |
||
|
Roland
|
ecf611931b
|
ui: tags look like tags, and stay readable in every theme
Four things, three cosmetic and one a real legibility bug. * Tags were square chips. They now have a pointed left edge and an eyelet, like a tag tied to a garment -- the shape Semantic gave them, and the reason a tag reads as a tag rather than as a small button. The point is a square rotated behind the left edge, sized so its diagonal equals the body height and its two edges meet the body at the corners. Only where something is actually a tag. `Label` carries the count badges in the Tags table too -- file, zim, channel and domain counts -- and a point on a number reads as an arrow, so the shape is behind a `tag` prop and `NameToTagLabel` opts in. * They were smaller than what they replaced: 11.5px text with 3px of padding, against Semantic's 12px and ~7px. Now 12.5px and 5px, which also keeps them a reasonable thing to tap. * Wrapped rows of tags sat on the row above, so they carry a bottom margin, plus a left margin for the point to occupy. * The text colour was unreadable in night and wrong in amber. Tags.js calculates black-or-light against the colour the user chose for each tag, and wrote the answer to `color` inline -- a declaration no stylesheet rule can outrank. Night turns a tag into an outline over a near-black page, so a near-white tag kept the black text calculated for its fill and was left legible only by its border: 1.1:1, measured. Amber replaces every fill with its own bright orange and was painting `#dddddd` on `#ff9500`, about 1.9:1, and a neutral grey in a theme whose whole point is that no other hue reaches the eye. The fix is to stop hard-coding `color` so the theme can decide: the calculated colour goes into `--label-text`, and night and amber override it alongside the fill they already override. There is no second contrast function to write, which is what this looked like it needed -- once a theme has discarded the user's colour there is nothing left to calculate against, and the answer is just the theme's own token. `.wrolpi-label-white` and `-black` moved to the same variable. Setting `color` there outranked the base rule, so amber was painting `--white` on `--text`: light orange on orange. `contrastingColor` was also picking the worse of its two options. It compared a brightness figure against a threshold, and the figure weighted the gamma-encoded bytes without linearising them, so Semantic's blue `#2185d0` got light text at 2.9:1 where dark text gives 5.3:1. Every mid-tone blue, teal and purple a user might pick sat in that band. It now measures the WCAG contrast ratio of both candidates and returns the better one, which needs no threshold at all. Tests ----- `Tags.test.js` is new, and covers the chip nothing covered: every other spec takes `NameToTagLabel` from `tagsContextFixture`, which stubs it as a function returning the name. That is how a tag could be unreadable without a single test noticing. Two of these were verified red against the inline `color`. `cy.contrastRatio()` measures text against the surface *actually* behind it, walking up to the first ancestor that paints something -- a transparent tag has no background of its own, so a naive check reads rgba(0,0,0,0) and concludes anything is legible. Twelve cases, three tag colours across four themes, at WCAG AA. Plus a test asserting the old inline-`color` version still measures below 1.5:1, so the check is provably able to fail. The gallery had label swatches in theme colours and no user-coloured tag, which is why none of this was visible in it. It has eleven now, spanning the range where the black-or-light decision flips. |
||
|
Roland
|
d697265267
|
ui: statistics carry no chrome, only spacing
The group had a frame, a surface and a hairline between every cell. It now has none of those: a plain responsive grid, transparent, so the statistics sit on whatever they were dropped onto and take its colour -- the page on the gallery, a Panel on Apps and Status. That also retires the problem the previous commit went two rounds on. Ruling the cells needs a technique that survives wrapping, because `auto-fit` settles the number of rows at paint time: a border per cell rules columns but not rows, and a border-coloured background behind a 1px gap rules both but paints every track no cell landed in. With no rules to draw, neither case exists. The gap is wider across than down, since two statistics side by side have only air between them while stacked ones already have the label's line. The margin above the group is conditional on having a sibling before it. A plain `margin-top` reads correctly under the Header that precedes most groups, but Apps opens each of its Panels with one, and there it pushed the values down off the panel's top padding while the bottom stayed put, leaving every panel lopsided. Five browser tests described hairlines that no longer exist. They are replaced by the claims that now hold -- real space between wrapped rows, a wider column gap than row gap, transparency and no frame anywhere, and the conditional margin -- each verified red first: against flush rows for the spacing, against a background-carried hairline for the transparency, and against an unconditional margin for the panel case. Per-theme, the pair to check is no longer the hairline and the surface but the value and the label: with nothing framing a statistic, the contrast between those two is the only thing giving it structure. |
||
|
Roland
|
a2aa1352fb
|
ui: Statistic and StatisticGroup, and the four defects they had
These three exports had no test of their own. Statistic had already been
caught out once -- it relied on Semantic's `Statistic.Group` for its
spacing, so it broke silently when that was uninstalled -- and they are
on the Status, Dashboard, Apps, Docs, Videos, Archive and Channels pages.
Four defects, each with a test that goes red without the fix:
* StatisticGroup ruled between columns only, with a `border-right` on
every cell but the last. `auto-fit` wraps, so the seven statistics on
the Docs page became four rows on a phone with no rule between them,
and the cell ending each earlier row drew its border straight onto the
group's outer one, making that edge 2px on the right and 1px on the
left. The hairlines are now a 1px grid gap filled by an outline on
each cell, which rules both axes however the tracks fall.
Ruling the gaps with a border-coloured background behind the grid --
which is where this went first -- has the same reach but paints every
track no cell landed in: an odd count left the last slot a solid grey
block. An outline is drawn only where a cell is. Both of these are
guarded, because the second was introduced while fixing the first.
* A statistic that renders nothing left a padded cell and a stray
hairline mid-row. FanRPMStatistic returns null on a device with no fan
connector, which is most of them, so this was the Status page's normal
appearance. The group cannot render a child to find out, so the cell
is still emitted and `:empty` takes it out of the grid -- which is why
the cells carry no inline style for that rule to have to outrank.
* Statistic destructured five props and dropped the rest. LoadStatistic
and the four Status wrappers all end in `{...props}`, so anything
passed through a wrapper vanished with no error anywhere.
* VideosStatistics passed `style={{margin: '2em'}}` to every statistic,
Semantic-era spacing that the grid now provides. It beat the group's
own rule, being inline, and made that one page's statistics sit
differently from the six others.
CardGroup was the only component in the library with no class of its own,
which left nothing to assert against -- `container.firstChild` in jsdom
is Mantine's injected <style> element. It has one now, and takes
className/style/rest like its siblings.
The gallery grows a seventh statistic, so the group wraps at a normal
window width where the row hairlines can be seen, and a second group of
coloured readings as Status renders them.
Geometry, hairlines and per-theme token resolution are in ui-layout.cy.js
rather than jest: jsdom returns zeros from getBoundingClientRect and
never resolves a var(), so it cannot judge any of it.
|
||
|
Roland
|
9d24d67f59
|
ci: raise the test timeout to 120s, and run controller tests on pull requests
The first CI run that actually enforced the timeout failed sixteen tests, and not one of them had hung. Every failure sat at exactly 30.0s, and the successes directly beneath them tell the story: test_get_refresh_progress passed at 29.9s, test_create_playlist at 29.8s, test_configs_crud at 29.4s. A cluster of refresh/worker tests straddles the limit on CircleCI's 4 vCPU box, which runs this suite in about twice the wall-clock of a dev machine -- the ones that passed did so by a tenth of a second, which is luck, not headroom. Thirty seconds was never a viable limit there; it only looked fine locally because nothing local exceeds it. So the limit is a hang detector, not a performance budget. 120s is roughly four times the slowest legitimate test observed on CI: still short enough to name a hung test long before CircleCI kills the job, and long enough to leave slow-but-working tests alone. A limit that fires on healthy tests only trades one unreliable signal for another. Also: controller-tests.yml carried the same `pull_request: branches: [master]` filter that jest-tests.yml did, so it never ran on a pull request into any other branch. That is why Controller Tests is missing from the checks on the very pull request that moved it to Python 3.13 -- the change could not be verified by the run containing it. Now unfiltered, like jest-tests. Not verified locally: this machine is under a load average of 45 from unrelated work, so the suite takes more than four times its usual wall-clock here and any timing measurement is meaningless. The change is strictly more permissive than what already passed at 30s, so it cannot introduce a failure, and CI is the environment in question -- it decides this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
a4db2aceac
|
ci: test on Python 3.13, the version every deployment actually runs
CI has been testing 3.11 while nothing ships it. docker/api/Dockerfile and docker/help/Dockerfile are python:3.13-trixie, docker/controller is python:3.13-slim-trixie, Trixie's system interpreter is 3.13, and Debian 12 -- the last place 3.11 was current -- is pinned to the debian12-final tag and takes no new code. So every Python job here exercised an interpreter no user has. All four CircleCI Python jobs and the GitHub Actions controller job move to 3.13. The cache key goes with them: it stores the whole venv, so restoring a deps-3.11 venv into a 3.13 container would hand it an interpreter that no longer exists. The disabled api-tests-3-12 job is deleted rather than repointed. It was switched off with the note "Sanic does not yet support 3.12", which 3.13 disproves, and carrying a second version around is what let this drift open in the first place. Verified on 3.13.12 / pytest 9.0.3 before changing anything: the backend suite passes (1709 passed, 5 skipped), alembic upgrade runs and `db check` reports no new operations, and the controller suite passes bar two docker-mode dashboard tests that fail identically on 3.11 here -- pre-existing and environment-dependent, not a 3.13 regression. `sanic-api-start` is the one job I could not verify locally: wrolpi/api_utils.py sets `Sanic.start_method = "fork"` and macOS has defaulted to spawn since 3.8, so that job fails here on any Python version. Linux CI is unaffected -- fork is still the default there on 3.13 -- and the test suite already runs the Sanic app in-process, which is the substantive check. Worth knowing that 3.14 changes the Linux default away from fork, so that line has a deadline on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
40587ebe98
|
build: require pytest-timeout, so a hung test fails instead of hanging CI
pytest.ini has carried `timeout = 30` and `timeout_method = signal` with a comment asking that it not be removed, but pytest-timeout was never declared in requirements.txt. CI installs only that file, so the options were unknown there, pytest warned and carried on, and the timeout has never once applied in CI. A hung test ran until CircleCI killed the job, which reports no test name at all -- and when the hang took an xdist worker with it, the rest of that worker's tests came back as a wall of EOFError, burying the actual cause. It goes in requirements.txt rather than a dev-only file because this project has no testing-only dependencies: a WROLPi is expected to run, test, and develop on the device, so the four Dockerfiles that install this file get it too. Verified: the unknown-option warnings are gone, a deliberately hung test now fails with "Timeout (>30.0s) from pytest-timeout" naming the test, and the full suite passes with the timeout in force -- 1709 passed under -nauto, so no legitimate test is near the limit locally. Worth watching: this is a behaviour change for CI, which has never enforced it. CircleCI runs on 4 vCPU with -nauto, so a test that takes ten seconds here could take longer there. If timeouts appear in CI the number is the thing to raise, not the plugin to drop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Roland
|
5fd6ba264d
|
test: check declared types, not just field names, and fix the three that were wrong
Greptile's point on #592 was right and generalized further than the case it raised. The guard compared property names only, so it passed while the published contract disagreed with the endpoint about what those properties contain. Three wrong types, all pre-existing, all now caught by a test: StatusResponse.downloads was `int` while the handler has always sent the download manager's summary object -- the spec told a generated client to expect a number where it gets a dict. It is now DownloadsSummaryResponse, with the six fields get_summary() actually returns. StatusResponse.wrol_mode was `str`; wrol_mode_enabled() returns a bool. SettingsResponse.log_level was `str` while the handler sends a Python logging integer -- SettingsRequest.log_level already said int, and the frontend converts it numerically through fromApiLogLevel(). settingsFixture was carrying 'info', which that function maps to undefined, so every Settings test has rendered a blank log-level label where production shows "Info". The fixture now carries 20. This came from the same hand-written cypress body as cpu_percent did. statusFixture's nested downloads was also short a field: daily_limit_reached, one the API has always sent. /api/status returns an empty downloads object while the database is down, which it is for much of the suite, so the endpoint cannot vouch for its own nested shape -- get_summary() is the authority and is now compared directly. The type check runs against both endpoints rather than only the one the review named; that is how log_level surfaced. Comparison is limited to properties the spec gives a type for and the handler actually sent, since a null from an optional field says nothing about its declared type. Fixture values are still compared by name only: they are read from JS source rather than executed, so their types are not reliably available. Said plainly in the test rather than left to look like coverage. 1709 pytest, 986 jest, 46 e2e. One unrelated flake in one backend run (the known filename-formatting tests under -nauto), clean across two further full runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |