Claude/repository review improvements 235v6l - #104
Merged
Merged
Conversation
Full review of Isosmfar and Inundator: correctness bugs (worker/config divergence, water-level inconsistency, Overpass area-ID handling, Mercator latitude scale, broken PWA icons, polygon holes), robustness gaps (Nominatim policy, CDN pinning, error handling), accessibility, dead code, and infrastructure items — each with file:line references and concrete fix guidance for later implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019AcQXHFWPnFHLGvMNAPmHr
Inundator:
- Wire flood-worker config from js/config.js instead of divergent
worker-local constants (area limit, iteration/debug intervals, edge
threshold), so the worker's own error message ("increase
maxReservoirAreaKm2 in config.js") actually does something
- Make the worker the single source of truth for the flood water level:
it now reports the damLevel/maxWaterLevel it actually used, and
Statistics/GeoJSON export use that instead of a separately computed
(and previously nonsensically elevation*0.95'd) crestElevation
- Fix extendDamToMountainside returning a bare Set instead of
{barriers, damLevel} on degenerate (very short/zero-length) dams,
which silently produced "no flooding detected"
- Fall back to network fetch instead of an all-no-data DEM when
IndexedDB is unavailable (private browsing etc.)
- Revoke object URLs for decoded DEM tiles instead of leaking one per tile
- Preserve MultiPolygon topology (holes, islands, disconnected arms)
in flood polygon generation instead of keeping only the largest ring
- Rescale the progress bar into two honest phases (DEM fetch 0-40%,
flood 40-95%) instead of jumping backwards when the flood phase starts
Isosmfar:
- Compute Overpass area IDs correctly for ways (2400000000+id) as well
as relations (3600000000+id), falling back to a bbox query for nodes
- Correct distance-field math for latitude: the shader used a flat
equator-only Mercator-to-km constant, so a "10 km" band was really
~5 km at 60°N; now scaled by cos(latitude) per area. Voronoi coalesce
distance in the worker gets the same correction.
- Fix broken PWA icon references (files didn't match manifest/HTML) and
start_url
- Remove duplicated, buggy URL-state parsing in the constructor (used
`||` fallbacks that dropped falsy values like transparency=0); URL
numeric coercion is now restricted to known-numeric keys
- Sample features uniformly instead of truncating to the first
MAX_FEATURES when a query returns more than the GPU field supports
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019AcQXHFWPnFHLGvMNAPmHr
- Light debounce bump (300ms -> 500ms) on Nominatim-hitting search inputs in both apps, without switching providers - Proper OpenStreetMap/CARTO attribution text (contributors, linked) instead of a bare "© Openstreetmap"; Inundator's basemaps also credit the Terrarium elevation data source they depend on regardless of which basemap is selected - Pin all CDN dependencies (MapLibre GL, Turf.js, D3) to exact resolved versions with Subresource Integrity hashes on <script>/<link> tags; Web Worker importScripts are pinned to exact versions (SRI isn't supported there) - Add onerror handlers to both Web Workers so load/crash failures surface to the user instead of failing silently - Route Inundator's location search and single-point elevation lookups through retry-with-backoff + response.ok checks, matching the robustness already used for bulk DEM tile fetches - Build message DOM nodes with textContent instead of innerHTML (both apps) so error text can't be interpreted as markup - Remove blocking alert() calls on Isosmfar's error paths; errors now stay visible via the message banner for longer (9s) instead - Chain the original error as `cause` when retry-with-backoff exhausts attempts, so console traces keep the real stack/status - Isosmfar no longer loads d3-delaunay on the main thread (it only ever ran inside the Voronoi worker) - removes a dead script tag and a startup check that would otherwise have blocked initialization once that tag was removed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019AcQXHFWPnFHLGvMNAPmHr
Isosmfar: - Rework the five custom sliders (distance, transparency, IDW power, heat bandwidth, Voronoi coalesce) from mouse-only drag to Pointer Events with capture (mouse, touch, and pen all work), and add full keyboard support (arrow keys step, Home/End jump to extremes) with ARIA slider semantics (role, aria-valuemin/max/now/text) kept in sync on every change, drag or keyboard - Change the visualization-mode toggle from clickable <div>s to real <button>s with aria-pressed, restoring keyboard focus/activation - Extract the query dropdown's existing keyboard navigation (arrows/Enter/Tab/Escape) into a shared helper and apply it to the area-search dropdown too, which previously had none Inundator: - Add a favicon/app-icon set (32/180/192px, all under 1KB) so the tab has an identity instead of a perpetual 404 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019AcQXHFWPnFHLGvMNAPmHr
Inundator: - Delete five dead flood-worker functions with zero call sites (remapCellSet, partitionByDamSide, identifyWaterBodies, checkSidesMerged, isApproachingEdge) and a commented-out, never-armed incremental-visualization code path (worker side and its unreachable app.js receiver/dispatch) - Extract the ~180-line BFS flood loop duplicated between performIncrementalFlood and resumeIncrementalFlood into a single shared runFloodLoop(), parameterized by starting counters. Verified behavior-preserving by running an identical synthetic-valley scenario through both the pre-refactor and post-refactor worker and diffing the results byte-for-byte (see test/flood-worker.test.js, which also locks in the P1.3 degenerate-dam fix and a sides-merged error case) - Remove a global console.warn monkey-patch in map-manager.js that suppressed one specific MapLibre message but could swallow anything Isosmfar: - Wire up four previously-declared-but-unused CONFIG constants (WEBGL_CONTEXT_OPTIONS, DEFAULT_RADIUS_PERCENT, MIN_DISTANCE_KM in two more spots, PALETTE_INTERPOLATION_STEPS) instead of duplicating their values as magic-number literals - Prefill the query field from the last-run query (STORAGE_KEYS.LAST_QUERY was written but never read) - Delete the dead `computedMaxDistance` field and unused updateProgress() method - Re-indent app.js from 8-space to 0-space base indentation - a leftover from extraction out of an inline <script> tag. Whitespace-only change, verified via `git diff -w` showing zero non-whitespace differences Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019AcQXHFWPnFHLGvMNAPmHr
Testing: - node:test suites for both apps (34 tests total, zero new runtime deps beyond @turf/turf as a devDependency): flood-worker regression + degenerate-dam + sides-merged cases, elevation-service tile math, dam-geometry, statistics, polygon-generator for Inundator; Overpass filter parsing, tag-autocomplete parsing, area-selector logic, CSV parsing, and URL-state encoding for Isosmfar, plus a Voronoi coalesce-distance test that locks in the P1.5 latitude fix - Both apps' non-module scripts (flood-worker.js, app.js, voronoi-worker.js) are tested by loading them into a node:vm sandbox that stubs just the browser/worker globals needed, since they have no build step to instrument otherwise Tooling: - Root package.json + eslint.config.js (flat config, scoped globals per environment: Inundator ES modules, both Web Workers, Isosmfar's classic browser script, and the test files) - `npm test` / `npm run lint`, both clean - .github/workflows/ci.yml runs both on every push/PR Deploy workflow fixes: - peaceiris/actions-gh-pages v3 -> v4 in both workflows - Preview index now lists every active preview (read from the live gh-pages branch) with links to both apps, instead of being regenerated each run to show only the current branch and only Isosmfar - New cleanup-preview.yml removes a branch's preview directory when the branch is deleted, so previews don't accumulate forever Docs: - New root README linking both apps - Isosmfar README: removed/corrected claims that no longer match the code - the service worker was removed a while back, so "works completely offline" and "ES6 modular design" were false; reworded to describe the actual IndexedDB/localStorage caching and the single-file-plus-worker architecture. Fixed the schema.org block's image link (pointed at a GitHub blob page, not an image) and dropped the unmaintained softwareVersion field. - Inundator README: aligned the flood-algorithm description with what the code actually does after the P1.1/P1.2 fixes Per user request, screenshots are left uncompressed - the only images loaded by the running apps (favicons/PWA icons) were already small. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019AcQXHFWPnFHLGvMNAPmHr
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.
Fable review, Sonnet implementation... Lots of good !