Skip to content

New layout#107

Merged
kiselev-dv merged 14 commits into
mainfrom
new-layout
Mar 13, 2026
Merged

New layout#107
kiselev-dv merged 14 commits into
mainfrom
new-layout

Conversation

@kiselev-dv
Copy link
Copy Markdown
Contributor

Redo App layout
Add source to the list of regions
Add map markers for regions
Some amendments for mobile

Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Mar 11, 2026

Deploying gtfs-osm-matcher with  Cloudflare Pages  Cloudflare Pages

Latest commit: 20884c0
Status: ✅  Deploy successful!
Preview URL: https://d231cf56.gtfs-osm-matcher.pages.dev
Branch Preview URL: https://new-layout.gtfs-osm-matcher.pages.dev

View logs

Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
@kiselev-dv kiselev-dv requested a review from biodranik March 12, 2026 20:17
@biodranik
Copy link
Copy Markdown
Member

Help на десктопе не работает (очень мелкая высота)

Copy link
Copy Markdown
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLM review, take with a grain of salt.

Bugs

1. Null pointer crash in map.tslocationInput.onchange without null check

In map.ts, locationInput can be null (it comes from querySelector), but it's cast and used without a guard:

(locationInput as HTMLInputElement).onchange = (e: Event) => { ... }

This will throw a TypeError at runtime if the DOM element isn't found. The idle handler above correctly guards with if (locationInput && osmHref), but the onchange assignment right after doesn't have the same guard.

2. handleDatasetLoad has a stale closure over datasetData

In report.tsx, handleDatasetLoad uses useCallback but captures datasetData in its closure:

const handleDatasetLoad = useCallback((ds: string, data: any) => {
    updateDatasetData({
        ...datasetData,   // ← stale: always the value from initial render
        [ds]: data
    });
}, []);  // ← empty deps array!

Since the dependency array is [], datasetData will always be {} inside the callback. If multiple datasets load, only the last one survives — each call overwrites the previous data. Fix: use the functional updater form updateDatasetData(prev => ({ ...prev, [ds]: data })).

3. useSyncExternalStore missing getServerSnapshot / identity-unstable snapshot

In app.tsx, SidePanelNav:

useSyncExternalStore<boolean>(
    (sub) => OSM_DATA.subscribe(sub),
    () => OSM_DATA.listChanges().length > 0
);

The getSnapshot returns a primitive boolean so it's fine for identity. However useOsmFeatures() in selection-info.tsx returns OSM_DATA.elements — if that array reference is mutated in-place rather than replaced, useSyncExternalStore won't detect changes. Worth verifying that OSMData always replaces the elements array on mutation.

4. Missing target="_blank" on source link

In report-table.tsx:

<a target={'blank'} href={...}>

Should be target={'_blank'} (with underscore). Without _blank, the browser opens in a named frame called "blank" — all source links will share and replace the same tab instead of each opening a new one.

Also missing rel="noopener noreferrer" which is a minor security best practice for external links.

5. RegionMarkersLayer event listeners on the wrong layers

In region-markers.tsx, handleMouseMove queries CIRCLES_LAYER and CLUSTERS_LAYER at the global mousemove level, but handleClick also does the same. Since these are registered on the map (not on specific layers), every mouse move across the entire map triggers queryRenderedFeatures twice — this is a performance concern on mobile. Consider using map.on('mousemove', LAYER_NAME, ...) for layer-specific events.

6. Cleanup race condition in RegionMarkersLayer

The effect cleanup function references map from the outer scope:

return () => {
    subscription.canceled = true;
    if (subscription.promiseFulfilled) {
        stylingControls.removeOverlayImmediate(overlayStyle);
        map.off('click', handleClick);        // ← uses outer `map`
        map.off('mousemove', handleMouseMove);
        map.off('mouseout', handleMouseLeave);
    }
};

But the mapLoaded.then(map => ...) callback shadows map with the resolved value. If the outer map reference differs from the resolved one (e.g., during map recreation), the cleanup would call .off() on the wrong instance. The mapLoaded.then callback should use the outer map consistently, or the cleanup should also use the resolved map.

7. hoveredRegion captured in closure leaks across effect cycles

In RegionMarkersLayer, hoveredRegion is a let variable inside the effect. If the effect re-runs (deps change), a new hoveredRegion is created but the old hover state on the map's feature-state is never cleared — the old setHover(null) never runs because the previous cleanup doesn't reset it.


Minor Issues

  • Font-size media query in app.css: font-size: 0.6em on :root is quite aggressive — 0.6em on the root element compounds, making everything very small. Usually rem or px is intended here.
  • MapTools toggle arrows seem reversed: The toggle shows when folded and when unfolded, but the CSS #drawer-toggle::after uses to collapse. The semantics are inconsistent — typically means "expand" and means "collapse".
  • console.error for missing #map-location: This logs an error but execution continues, eventually hitting the null pointer crash from bug #1 above.
  • import.meta.env.DEV guard on console.log: Good improvement, but the two console.log calls in MatchReport were likely debug leftovers — consider removing them entirely rather than gating.

Summary of Critical Items

Severity

Issue

File

Bug

NPE on locationInput.onchange

map.ts

Bug

Stale closure loses dataset data

report.tsx

Bug

target='blank' missing underscore

report-table.tsx

Perf

Global mousemove queries on every pixel

region-markers.tsx

Bug

Hover state not cleaned on effect re-run

region-markers.tsx

The stale closure bug (#2) and the null pointer (#1) are the most impactful — #2 will cause data loss when multiple datasets load, and #1 will crash on any DOM timing issue.

Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
@kiselev-dv kiselev-dv merged commit b45d460 into main Mar 13, 2026
3 checks passed
@kiselev-dv kiselev-dv deleted the new-layout branch March 13, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants