Rework DataStream loading, map layers, and visual system - #7
Open
romer8 wants to merge 53 commits into
Open
Conversation
Added a banner and moved layer menu to header
…s scripts and components
refactor: update S3 paths to reflect new resource structure
Resolve Grype scan failures
…-UA/nrds-client into fix/scan-cves-pyarrow-pyo3
Fix/scan cves pyarrow pyo3
Add CD pipeline for staging and prod deployments
Added google analytics tag on deploy
fix: render google analytics tag correctly
mocks/server.js imports msw v1's `rest`, removed in v2, and msw v2 needs web globals this jest/jsdom does not provide. The import threw during setup, so all suites failed to load and no test in the repo ran. Load the server defensively instead.
… screen Clicking a catchment while a fetch was in flight did nothing at all. The click handler returned early on `loading`, and both fetch effects did the same, so the selection was discarded. Those guards also read a `loading` captured when the effect last ran, since the dependency arrays are [feature_id] and [cacheKey]. The existing `alive` flag already discards superseded work, so the guards were unnecessary as well as lossy. Separately, the status line rendered its text only while `loading` was true. A failed fetch set the message in `catch` and then cleared `loading` in `finally`, erasing the message before it could be read, which is why a failure looked identical to nothing happening. The text now renders whenever it is set; only the spinner tracks `loading`. The status line is also a live region so the change is announced.
After a fetch failed there was no way to try again. The fetch effect keys on feature_id, so re-selecting the catchment that just failed changed nothing in the store and the effect never re-ran: the only escape was picking a different catchment and coming back. set_feature_id now bumps a feature_request_id counter, which the effect depends on, so a selection is observable even when the id is unchanged. Both fields are set in one call, so a genuinely new selection still fetches once rather than twice. The counter is never reset by reset(), which spreads existing state, so it stays monotonic. Re-clicking a catchment that loaded successfully now refetches as well. That is a small amount of redundant work in exchange for a click always doing something, and it doubles as a manual refresh.
…charted Making every click observable so failures could be retried also meant re-clicking the displayed catchment re-ran a query that changed nothing. The store now records last_loaded_key, the vpu|variable|feature identity of whatever series it currently holds, and the fetch effect returns early when a click matches it. The key is only written on success, so a failed load leaves it null and still retries, and it is cleared by reset_series and reset so a click after the chart is emptied reloads rather than being suppressed against a blank chart. A per-feature success flag would not have been safe: the store holds one series at a time, so skipping the fetch for a feature that loaded earlier would leave the previous feature's points on screen under the new feature's label. Recording only what is currently displayed avoids that, and the variable is part of the key because switching variables changes the data without changing the feature. variablesMenu loads a series directly rather than through set_feature_id, so it records the key too, keeping the invariant that the key always describes what is on screen.
…iled
A failed vpu load was terminal. The effect keys on cache_key, and set_cache_key returned
existing state unchanged when the key matched, so pressing the visualize button again with
the same selections produced no state change and the effect never re-ran. The only way out
was to change a dropdown and change it back.
set_cache_key now bumps a cache_request_id counter that the effect also depends on, so a
repeated request is observable. The counter sits outside DEFAULTS because reset() merges
DEFAULTS over existing state, and restarting the count could collide with a value the
effect had already seen.
No suppression on this path, unlike the timeseries one: pressing visualize is an explicit
request to load, and the expensive part is already skipped by the checkForTable guard, so a
repeat re-runs metadata queries rather than downloading anything.
Also removed three set_loading_text('') calls that immediately followed the message they
were clearing. Selecting no feature, selecting no output file, and pressing visualize
during a load were all supposed to explain themselves and none of them could.
Two costs sat inside the animation loop. computeBounds ran inside the memo that depends on currentTimeIndex, so every playback step rescanned the whole value array to arrive at the same numbers: measured at 1.0 ms for 20k flowpaths over 28 timesteps and 9.5 ms over 240. Bounds depend on the data, not the frame, so they now have their own memo keyed on the data. valueToColor rebuilt its six-entry color scale, and the missing-value color, on every call, and deck.gl calls it once per flowpath per frame. Hoisting them to module scope and replacing the interpolating .map with three expressions measured 1.95 ms -> 0.92 ms for 20k paths. Together the per-frame JS for this layer goes from about 11.4 ms to 0.9 ms. valueToColor also threw for any value outside bounds: sqrt of a ratio above 1 indexes past the end of the scale, and the .map then dereferenced undefined inside a deck.gl accessor. The linear ramp it replaced was clamped and the guard was lost in the change, so the ratio is clamped again. NaN lands on the low end rather than throwing. Measurements are of the functions themselves under node, not end-to-end frame times. Tests pin the ramp against values captured from the previous implementation, so the hoisting cannot quietly change any color.
initialS3Data walks five S3 listings, and only the last one depends on the vpu. The effect that calls it keys on vpu, so selecting a different vpu refetched all five, four of which could only return what they already had. A vpu change now costs one request. The four vpu-independent listings are held for the life of the page, and only once the chain completed: an empty listing is a transient condition, so caching it would leave the menus permanently empty with no way back short of a reload. A reload still picks up dates published since the page opened, which is the same staleness the code already accepted by reading models[0] and dates[1] once. Also dropped a try/catch that only rethrew.
Changing variable awaited the vpu-wide flat array before starting the timeseries query, though neither needs the other's result, so the wait was the sum rather than the longer of the two. The flat array is a full-table scan, which is the slower half. The map and the chart now update together instead of the map updating first. Both were already gated behind the same request-id check, so a superseded change is still discarded.
Four changes along the per-frame path, following deck.gl's performance guide. MainMap read currentTimeIndex, which advances on an interval during playback, so every step re-rendered all of it: every hook, every store selector, and a getComputedStyle call that forces a style recalculation. The animated layer is now its own memoized component and it alone subscribes to the frame index, so nothing above it re-renders to animate. That getComputedStyle call also duplicated work layers.js already does once at module load, and did it without the fallback layers.js applies, so a missing CSS variable handed an empty string to the map as its style url. It now imports the resolved value. The flowpath layer is hidden with visible rather than dropped from the layer array. The guide is explicit that a hidden layer keeps its GPU resources, so toggling flowpaths back on no longer rebuilds every buffer. getColor now writes into the target array deck.gl provides instead of returning a new one. Measured at 20k flowpaths this cuts roughly 2 MB of short-lived arrays per frame down to nothing, about 48 MB/s of garbage at 24 fps. No throughput difference was measurable, so this is about collection pressure over a long session, not frame time. Bounds keep their own memo, unchanged in effect but now inside the animated component. Tests assert the ramp still produces the same rgb values it did before any of this, and that the accessor writes into the array it is handed rather than allocating.
Charting a feature was driven by an effect keyed on feature_id, which forced a shape around it: a feature_request_id counter so that re-selecting the same feature was visible to the dependency array at all, an alive flag per effect to drop superseded results, and a second copy of the whole load in variablesMenu because a variable change could not go through the same path. Selecting a feature is an event, not something to synchronise with, so the load is now a store action called directly by the map click, the search box, the variable menu, and the end of a vpu load. What that removes: the effect, the counter it existed for, the duplicated load in variablesMenu, and the mounted flag there, since everything it guarded writes to stores rather than component state and ordering is already kept by a request id. Retrying a failed load needs no mechanism now -- calling the action again is the retry. One module-scope counter replaces the per-effect alive flags. The action deliberately does not set the store's variable. variablesMenu sets it after the values arrive, so the flowpath layer is never left looking up a variable it has no data for. Also dropped a set_loading_text call whose message was overwritten in the same synchronous block, so it could never be read, and moved playback-index clamping into set_series, which removes TimeSlider's effect for correcting the index after the fact. Effects in the feature go from 17 to 14. The rest subscribe to maplibre events, register the pmtiles protocol, or read the store outside render to avoid re-rendering during playback, which is what effects are for. Tests now exercise the action directly rather than a rendered effect, and cover the two mechanisms that replaced the old ones: suppression of a redundant load, and a superseded load losing to a newer one.
A return inside finally discards whatever exception the try or catch was propagating. Here it was guarding a store write after the effect had been superseded, which a plain if does without swallowing anything.
… once The chart picked its axis label size from window.innerWidth during render. That read forces a layout flush on every render, and it was not reactive: a chart mounted on a narrow window kept narrow labels for the rest of the session no matter how the window was resized. It now subscribes to the one media query it cares about. No effect was needed after all -- useSyncExternalStore is the supported way to read a source like this, and subscribing to the query rather than to resize means a re-render only when the breakpoint is crossed instead of on every pixel of a drag, which matters for a chart this size. The hook is its own module so it can be tested without importing the chart, whose d3 dependencies ship as esm that this jest setup cannot parse. Its MediaQueryList is cached against the matchMedia it came from, so a late polyfill, or a test with its own implementation, gets a list bound to the right one.
Changing variable re-queried the flat value array every time, including for a variable the vpu store was still holding. Measured against duckdb-wasm in Chromium at 4.8M rows (20k flowpaths x 240 timesteps), that call costs about 800 ms on a repeat, of which the ORDER BY is roughly 570 ms: the same scan without it runs in 234 ms. No cache was needed. setVarData has kept an LRU of the last few variables all along, and getVarData was defined to read it and never called. resetVPU empties that map, and the vpu load calls it before loading, so an entry can only ever be data for the current vpu. This also settles the COUNT(*) that sizes the Float32Array, which I had flagged as worth removing: it measures 11 ms of that call, around one percent. Not worth the growable buffer it would take, and the function already resizes defensively if the count turns out wrong, so the exact allocation it buys is not something the code depends on.
The last data-fetching effect. It keyed on cache_key, so re-requesting the same vpu after a failure changed no state and could not re-run; the fix at the time was a cache_request_id counter whose only purpose was making a repeat visible to a dependency array. Pressing visualize is an event, and so is the initial load once the s3 options are known, so both call an action and a repeat call is simply a repeat. The action sits in actions/ rather than a store because it spans five of them. It reads each with getState where it is used, so late steps see current state instead of what a render closure captured when the load started -- the selected feature in particular, which can move while a vpu is loading. A module-scope counter replaces the alive flag. Removing it takes TimeseriesLoader with it: the component existed only to host the two fetch effects, and DatastreamView drops from 311 lines to 129. cache_request_id is gone and set_cache_key goes back to returning existing state when the key has not changed. Effects in the feature are now 13, down from 17. Every one that remains subscribes to maplibre, registers a protocol, cleans up, or reads a store outside render to avoid re-rendering during playback.
loadTimeseries lived in the store, which meant importing the store imported duckdb and arrow with it: any component reading so much as the current variable dragged the whole query layer into its module graph. That is what made the chart untestable -- it imports the store, so a test importing the chart tried to construct a Worker in jsdom. It moves next to loadVpu, which was already outside the stores for the same kind of reason, and the store is back to importing nothing but zustand. Callers import the action directly rather than selecting it, which is also one less thing in each selector.
d3 and its dependencies publish untranspiled esm, so importing Plot threw a syntax error out of node_modules before any assertion could run. They are now transformed, along with internmap, delaunator and robust-predicates, which they pull in. Also polyfilled TextEncoder and TextDecoder, which jsdom omits and apache-arrow uses at import time. This is not enough on its own to load the duckdb helpers under jsdom -- Worker is missing too -- but it is a genuine gap worth filling rather than working around. The tests themselves stay shallow on purpose. An svg chart's real output is geometry, and asserting path coordinates would break on every legitimate styling change, so they check that a normal series draws a path at all, that the axis is labelled, that an empty series says so rather than drawing nothing, and that a zero-width container does not throw -- which is the first render every time, before the container has been measured.
Ten comments inside function bodies ran to more than one line, against this project's convention that only docstrings and module-level comments do. Condensed. Removed three pieces of dead code the review found: set_last_loaded_key, which nothing has called since the loader moved out of the store and began writing the field directly, and the default exports on both action modules, which no call site uses. Corrected loadVpu's docstring: it says it spans five stores and it touches six. Silenced two lint rules in the new tests where the rule and the test disagree on purpose -- reaching into the container is the only way to assert svg geometry, and the react-select stand-in is a two-prop stub rather than a component worth validating.
Review found three defects with one root cause: loadVpu and loadTimeseries each kept a private request counter, so neither could see or supersede the other, while both wrote the same loading, loadingText and last_loaded_key fields. The worst consequence was that loadVpu cleared loadingText immediately after awaiting loadTimeseries, so a series failure inside a vpu load was erased before anyone could read it. That is the defect 39f5fd7 was written to fix, reintroduced two commits later by dfcfe05. loadVpu now clears that message only when there was no feature to chart; otherwise the series load owns it. actions/loadState.js holds what the two actions must agree on. A vpu generation, bumped when a vpu load starts, which any older series load checks before writing -- so a load orphaned by a vpu switch can no longer chart a replaced table's data or claim last_loaded_key for it. And a count of loads in flight rather than a boolean, so the spinner goes out when the last one finishes. The relationship is deliberately asymmetric: a vpu load invalidates series loads because it replaces the table they read, while a series load arriving during a vpu load records its selection and defers, letting that load's closing call pick it up. variablesMenu now re-checks cache_key before writing to the variable cache. valuesByVar is keyed by variable alone, so a vpu switch mid-flight would otherwise hand the new vpu the old one's numbers under a name they share. It also reports a failed map query instead of logging and moving on, which is what left the map and the chart disagreeing in silence. Six tests cover this, each verified by reverting the mechanism it pins. The load count is the exception and says so in its own comment: a vpu load does nothing after the series load it ends with, so a boolean is indistinguishable from outside today.
Two findings from the review. A load that succeeded and found no rows recorded its key and cleared the status line, so the chart fell back to the same empty state it shows before anything is selected, and asking again was correctly suppressed but looked like nothing happening. It now says which variable and feature came back empty, and keeps saying it until something is charted. The flowpath layer is hidden with visible rather than dropped, which keeps its buffers, but deck.gl gates only drawing on visible: attribute updates still run, so a live frame index in updateTriggers had it recomputing colour and width for every path on every step of a playback nobody could see. The index is replaced by a constant while hidden, which costs one recompute when the layer is toggled instead of one per frame while it is off. The layer's props moved to their own module on the way. They are plain props rather than a PathLayer, so nothing about them needs maplibre or deck.gl, and separating them is what makes the trigger behaviour assertable at all -- importing the map component pulls maplibre, whose esm this jest setup cannot parse. Nine tests, each checked by reverting the mechanism it pins.
Coverage the review asked for, and the changes needed to make it possible.
The map's click decision moves to actions/selectFeature.js. It had no tests because reaching
it meant rendering the map, and this jest setup cannot parse maplibre's esm; the handler keeps
the part that is genuinely maplibre's, turning a click point into rendered features, and the
decision from there is now a function. That also answers the review's other note about this
code: selecting a catchment is callable rather than something only a canvas click can do.
Extracting it surfaced a real bug in the feature store. set_selected_feature skips an update
when the new feature's key matches the old, and featureKey read id or properties.id -- but
callers hand it a flattened object whose deliberate id is _id and which has no properties at
all. Any layer whose tiles carry no id therefore keyed every selection as null, which the
guard reads as unchanged, so selections were silently dropped. featureKey now reads _id first.
createSequence in lib/sequence.js replaces the third spelling of latest-one-wins. loadTimeseries
and variablesMenu now share it; loadVpu keeps the vpu generation, which answers a different
question. Also dropped set_series' fingerprint comparison: its only caller has just fetched for
a key it knows is not the charted one, and two features can share a length and endpoints while
differing in between, so refusing that write left one feature's values under another's label.
last_error records what failed as {kind, ...} beside the prose in loadingText, so callers and
tests can ask whether something failed without matching English. loadVpuData is imported under
a name that does not read like the action calling it. The visualize button clears the previous
press's complaint before evaluating this one. loadState documents why neither action takes an
abort signal, and what would have to change if the view ever unmounts mid-session.
93 tests, up from 76. Every mechanism checked by reverting it.
Both actions claimed their counters and then ran four statements before entering the try whose finally releases them. A throw in any of those -- a store reset, the loading count, a status message -- would have left vpuLoadInFlight true for the life of the page, and every later catchment click silently deferred waiting for a vpu load that had already finished. That is the dead-click symptom this branch was written to remove, reachable by a different route. Claiming a counter and releasing it are now in the same try/finally, with nothing in between. Two tests cover it by making the reset throw and then checking that a later click still loads and that loading ends false. Both fail if the statements move back outside the try.
Testing found the original symptom still present: clicking a catchment gave no sign of loading or of failure. Every message this branch fixed was correct and none of them were visible. ForecastMenu opens its panel only when feature_id is set, and Styles slides it to translateX(-100%) otherwise. The status line lived in that panel. A first click sets the vpu rather than the feature, so feature_id stays null through the whole vpu load -- the parquet download, the table build, several seconds -- with the spinner and text rendering off-screen. loadVpu also calls reset(), which nulls feature_id, so every vpu switch and every visualize press had the same blind spot. And a load that fails never sets feature_id at all, so the failure message could not appear under any circumstances. Whether the app is working is not a property of one panel. LoadStatus now sits in the header, which is always on screen, and renders nothing while idle so it costs no space. It styles failure from last_error rather than by matching the message text, which is what that field was added for. The panel's copy is gone: one owner for the status. My tests asserted that loadingText held the right words, never that anyone could read them. Three new ones render the status with feature_id null -- the state the panel hides in -- which is the assertion that would have caught this.
The strip was legible in neither, but visibly wrong in dark mode. It hardcoded a light-theme grey, #475467, and the dark header is #2c3e50 navy: measured against that background the text came out at 1.38:1, which is why the screenshot shows it barely there. The header is white in one theme and navy in the other, so these belong in App.scss beside the other theme tokens rather than as literals in a styled component. Values were computed against each theme's own header rather than picked by eye: 14.1:1 and 5.8:1 on light, 7.2:1 and 6.3:1 on dark. Failure text is not $danger in either -- #d62518 only reaches 4.5:1 on white, under the AA floor, and is far too dark to read on navy at all, so light goes a shade darker and dark goes considerably lighter. Also bumped the strip to 0.95rem/600 with a little more padding, and thickened the spinner border. It draws in currentColor, so it was washing out with the text.
Clicking cat-2854942 sent the map to the Gulf of Guinea. getCentroid handled Point and Polygon
and nothing else, so a MultiPolygon catchment fell through to {lon: null, lat: null}; the flyTo
that consumes it passed [null, null] as a centre, which becomes 0,0. Hydrofabric divides are a
mix of Polygon and MultiPolygon, which is why most catchments behaved and this one did not.
getCentroid now covers every geometry type the tiles can carry, using outer rings only so a
hole cannot pull a catchment's centre around, and still returns nulls when there is genuinely
nothing to average. The flyTo checks for finite coordinates before moving, so an unplaceable
geometry leaves the camera alone instead of teleporting.
Those two lines also read `lat || latitude`, which discards a real zero -- a feature on the
equator or the prime meridian would have taken the other field. Now ??.
Ten tests over the geometry types, the hole case, the degenerate cases, and a genuine 0,0.
Reverting the MultiPolygon branch fails the one that matters.
Three faults, one visible symptom. The input's value came from the store's feature_id, which only changes for a complete id in the loaded vpu. Every keystroke was therefore discarded and the box snapped back, so it could not be typed in at all. It owns its own text now. Each keystroke also ran a duckdb query. Searching is a submit -- the button, or Enter -- so one search runs per request instead of one per character. Those queries need an index that takes about seven seconds to build on mount: 2.07 million rows read from a 103 MB parquet, measured against duckdb-wasm rather than guessed. Anything typed before it existed raised "Table with name index_data_table does not exist", and the handler had no catch, so each one arrived as an unhandled rejection in the console. The box now stays disabled with a plain explanation until the index is ready, an id that is not in the index says so in place of the placeholder, and both a failed search and an index that cannot be built are reported through the header status rather than only to the console. Eight tests. The store-controlled value and the query-per-keystroke both fail them when put back, the latter checked by reinstating the original call rather than a stale-closure lookalike that quietly did nothing.
The index was read over http on every page load, and it is not small: 103 MB holding 2.07 million ids. Measured against duckdb-wasm in Chromium, that cost about 6.3 seconds each visit, whether or not anyone searched. It now goes through the same OPFS cache the vpu tables use. Measured on a persisted browser profile: a first visit downloads and builds in about 5.8 seconds, slightly quicker than the old path, and every visit after it builds from the cached file in 1.9. Roughly four and a half seconds back per visit, and the search box stops being disabled that much sooner. Two things had to give. The cache keys on file extension -- it dispatches on it and strips it for the table name -- so the key is index_data_table.parquet, which still yields the table index_data_table that the search queries. And cacheParquetToOPFS hardcoded the datastream bucket and took a key within it; the index is on a different bucket and the store holds an absolute url, so an absolute url is now used as given. The cached copy is never invalidated. If the index is republished, a browser keeps the old one until its cache is cleared, which the existing cache menu can do.
Deleting one file confirmed success and the file was still there after a reload. Three faults. createTableFromOPFS registers each cached file with BROWSER_FSACCESS, which holds a sync access handle open for the session, and OPFS refuses removeEntry on such a file with NoModificationAllowedError. Proven in a browser: removeEntry while registered fails, dropFile then removeEntry succeeds. deleteFileFromCache now releases the file from duckdb first, and clearCache calls dropFiles for the same reason -- delete-all was failing the same way. The table was dropped by the cache id, which still carries the .parquet that the table name does not, so DROP TABLE IF EXISTS matched nothing and reported nothing. It now uses tableNameForKey, and drops the table before releasing the file, since the table is what holds it open. And the row left the list either way. A refused delete now keeps its row and says so through the header status, rather than looking like it worked until the next reload. Also stopped listing .crswap files as cached tables. Chrome writes one alongside an open writable and removes it on close, so an interrupted download leaves an orphan -- there was a 24 MB one in my profile, offered in the menu as a table it was not. Five tests, each checked by reverting the mechanism it pins.
Nothing ever left the OPFS cache. Its key is model x date x forecast x cycle x vpu x file, so browsing dates and cycles accumulated parquets indefinitely, across sessions. Measured before choosing a policy: all twenty vpu parquets for one model, date, forecast and cycle come to 139 MB, averaging 7 and peaking at 15, against a 7.6 GB origin quota. Caching one at a time would have thrown away the point of the cache -- switching vpus back and forth pays 4 to 15 MB and a table build each way -- so the cap is ten files, roughly 70 MB, which bounds the growth without giving up the reuse. Recency is kept in localStorage and updated when a file is written and when it is read, so a file the user keeps returning to is not the one evicted; a file with no record sorts oldest. Eviction drops the table and releases the file from duckdb before removing it, the same order the single delete needed. The id index is exempt: 103 MB, the search depends on it, and refetching it is the slowest thing the app does. Nine tests, with a small fake OPFS since jsdom has none. The index exemption and the recency ordering both fail them when removed -- the ordering only after the fixture was changed so that directory order and use order disagree, since at first they happened to coincide and the test passed either way.
One interrupted download disabled the search permanently. getFileHandle with create makes the entry immediately and the bytes go to a .crswap that only swaps in on close, so reloading during the 103 MB index download left a 0-byte file. statFromCache reported it as cached -- it only asked whether the file existed -- so the caller skipped the download, and duckdb refused the file: "too small to be a Parquet file". Every reload took the same path, so it never recovered. statFromCache now checks that a cached file is actually one: parquet brackets itself with PAR1 at both ends, so 8 bytes are enough to catch both an empty file and a truncated one, and arrow is checked by its own marker. A file that fails is deleted and reported absent, so the next attempt downloads it. That fixes loadVpuData as well, which trusted existence the same way and would have left a vpu permanently unloadable after an interrupted download. ascii4 had been sitting unused in this file since it was written, which is what it was for. Seven tests, over a complete parquet, an empty one, a truncated one, something that is not a parquet, an arrow file, an unknown type, and a missing file. They assert the discard warning rather than only a null return, because the catch returns null too -- two of them passed at first on that path, since jsdom's Blob has slice but no arrayBuffer, so the read threw. That gap is now bridged in setupTests, and removing the check fails three of the seven.
It is the app's own file rather than something the user asked to load: already exempt from eviction, and deleting it only breaks search until the next reload. Offering it a delete button alongside the vpu parquets invited exactly that. Its display name came out as index/data/table.pa... too, because the list turns underscores into path separators for keys where they are ones. The eviction exemption and the listing now read the same set, so the two cannot drift.
Contrast, measured before and after. Three pairs were below the 4.5:1 AA floor and none are now. The active nav pill put white text on #009989 at 3.55:1; the teal is darkened to the lightest value that clears AA, so it stays vivid at 4.83:1. The cache panel hard-coded #666, which is 1.91:1 on the dark surface, hiding both of its messages; it now uses a --panel-text-muted token that clears 5:1 in each theme. The chart's empty state was 4.33:1 in dark and is 5.00:1. The panel dividers were never drawn. --panel-border-color was the same colour as the surface it sat on, in both themes, so four distinct regions read as one stack. They now measure 3.00:1 against their surface, and Content varies its spacing either side of them instead of applying 12px everywhere. The rule also moved off the first section and onto all but the last, which is where a divider belongs. Semantics. The app had no headings outside an error page: the panel title and Files Loaded were styled spans. Both are headings now, the forecast panel is a labelled aside, and the icon-only buttons carry aria-label rather than only title, which is neither announced nor reachable by keyboard. Focus rings are back on the search input and the time slider, both of which set outline: none with nothing in its place. Type has a scale: four fixed rem steps at a 1.2 ratio and three weights, replacing a single 13px size for every label. Title also stops asking for 'Google Sans', which is not loaded here and was silently falling through to Arial. Touch targets on the destructive cache controls go from about 31px to 44px. The search input was 500px wide inside a 400px wrapper and now flexes. The two floating map overlays were 250px fixed and now clamp to the viewport. Polish: --muted-text in dark was #ffff, a four-digit typo; pure white and black are replaced with neutrals tinted toward the brand hue; the emoji empty state now says what to do instead of what is missing; and the console.log firing on every selection is gone. oklch is used for the tokens CSS consumes, and deliberately not for the eight --map-* tokens: lib/layers.js hands those to maplibre, whose colour parser supports lab() but not oklch.
The audit's remaining findings, plus one it had missed twice. ToggleButton hard-coded white text over --button-primary-bg, which is #f8f9fa in light: 1.05:1, an invisible label. It uses --button-primary-text now, measured at 5.74:1 light and 8.79:1 dark. Both audits missed it because the contrast sweep compares token pairs, and this component hard-coded one side of the pair, so it never appeared in the comparison. The six configuration dropdowns had no accessible name at all: SelectComponent took no label prop, and the text beside each one was an unassociated span. Each label is now a real label bound to its control by htmlFor and inputId, which names the control for assistive tech and makes the label clickable. The control also grows from 28px to 44px, which matters more here than on the delete buttons enlarged in the previous round, since these are the controls people actually use. Every pure white and black token is now tinted toward the brand hue. Nineteen were left after the last round, which claimed more than it delivered; the count is zero now. The eight --map-* tokens are tinted in hex rather than oklch because lib/layers.js hands them to maplibre, whose parser has no oklch. $primary keeps its pure white for bootstrap's theme map; only the custom properties changed. Also: react-select's focus ring was a hard-coded #2684ff and now follows the theme, and TimeseriesCard's Suspense boundary is gone. LineChart is a static import, so it could never suspend and the loading fallback inside it was unreachable. Contrast re-measured across all 19 text tokens in both themes: none below the AA floor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rewrites the DataStream feature: event-driven loading replaces the data-fetching effects, flowpath geometry moves to an archive with tiles below zoom 7, and the OPFS cache holds one file. Adds retry paths, accessible names, AA-contrast tokens, a single spinner, and 232 tests.