Harden merge workflows against routing regressions - #218
Open
trevorgerhardt wants to merge 22 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c7deefb The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
trevorgerhardt
marked this pull request as ready for review
July 21, 2026 07:00
Why: The worker-backed Merge browser journey repeated PBF loads and MapLibre layout checks while worker restart tests competed for the same CI runner, causing a 120-second timeout. Decisions: Keep one real Monaco load per input, exercise presentation behavior through the production input header in the lightweight harness, and run worker-runtime tests after Merge UI tests. Changes: Extract the loaded input-card header, move clear/download/overflow checks to the harness, consolidate responsive cases, and document the browser-test boundary. Validation: pnpm run verify:workspace -- apps/merge --write CI=true pnpm --filter @osmix/merge test:e2e pnpm run check:deps pnpm run test git diff --check
Why: Hosted CI still hung while the Merge journey streamed its second fixture through Vite, and the GeoParquet suite rebuilt the same fixture often enough to exceed its normal test timeout under load. Decisions: Exercise the real local-file selection workflow with a single app worker, keep multi-worker behavior in the dedicated runtime project, and share immutable GeoParquet fixture setup across assertions. Changes: Select checked-in Monaco PBFs with Playwright file choosers, fail immediately on load-error panels, assert the test worker count, and cache the GeoParquet fixture and converted OSM in beforeAll. Validation: CI=true pnpm --filter @osmix/merge test:e2e pnpm run verify:workspace -- @osmix/geoparquet --write pnpm run check:deps pnpm run test git diff --check
Why: Hosted Chromium can finish navigation before the top-level worker module exposes window.osmWorker, so the worker-count assertion must tolerate initialization. Changes: Return zero until the remote exists and let the existing Playwright poll wait for the expected single worker. Validation: CI=true pnpm --filter @osmix/merge test:e2e pnpm --filter @osmix/merge typecheck
Why: The local-only monaco.test.pbf is ignored by Git, so hosted Playwright could not select it; the earlier URL-based test had silently received Vite HTML instead and hung in PBF parsing. Changes: Use the tracked Monaco PBF for both Merge roles while the lightweight harness retains distinct filename coverage. Validation: CI=true pnpm --filter @osmix/merge test:e2e (16 passed in 23.2s) git diff --check
Why: The two-core hosted runner still starved a lightweight guidance page while it ran alongside the real Merge journey, even after the PBF fixture issue was fixed. Decisions: Run the real Merge integration, guidance harness, and worker-runtime coverage as ordered Playwright projects so only one Chromium worker is active at a time. Changes: Split Merge integration and guidance into separate projects and make worker-runtime depend on guidance. Validation: CI=true pnpm --filter @osmix/merge test:e2e (16 passed in 26.3s) pnpm --filter @osmix/merge typecheck pnpm exec oxlint --type-aware apps/merge git diff --check
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
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.
Summary
This PR makes OSM merges safer and adds routing tests for merged data.
The exact merge is now conservative. It compares patch entities only with the original base. An empty patch does not change the base.
The PR also restores the original 1-meter match for imported data. This match is now an optional and visible workflow step.
Users can transfer selected tags, attach imported networks, or do both. Unsafe or unclear matches require review or remain blocked.
What we found
The Monaco fixture reproduced the reported routing error. Before this fix, an empty merge changed the dataset:
Two Monaco routes show the effect:
1866510534 → 937988247changed from approximately 1,043 m to an invalid 29 m shortcut.1875118274 → 12281555152changed from approximately 164 m to unreachable.The old merge matched nearby entities inside each input. Distance alone cannot prove that two OSM entities represent the same feature.
Nearby nodes can represent tunnels, bridges, barriers, parallel roads, or different network levels. The old replacement changed way and relation references.
The routing review also found three separate router errors:
BinaryHeap.bubbleDownsometimes put items in the wrong priority order.junction=roundabout.oneway=-1as two-way travel.The Eastern Washington fixtures found an intersection error. A valid full merge collapsed patch ways after several intersection nodes mapped to one base node.
The fix groups intersections by segment and inserts them in geometric order. It rejects an insertion that creates repeated references or a degenerate way.
Safe exact merge
The merge no longer does proximity-based cleanup inside the base or patch. Exact reconciliation has these rules:
Intersection creation now inserts all nodes in segment order. It connects only crossings with compatible grade context.
The merge rejects a changeset that creates one of these results:
The error includes the related OSM entity IDs. The merge does not silently remove or repair restrictions.
Optional imported-data matching
Imported GeoJSON, Shapefile, and OSW data often use different IDs and slightly different coordinates. These files still need proximity matching.
The new workflow searches from the untouched patch into the original base. It does not compare entities within one input.
Imported-data matching is off by default. The recommended configuration uses these values:
barrier,crossing,kerb, andtactile_paving.Property transfer and network attachment are separate actions:
An absent patch value does not remove a base value. The merge does not copy an unselected key.
Protected structural tags cannot transfer through a fuzzy match. Routing tags require review.
Automatic node attachment requires one compatible target and a one-to-one mapping. It also requires compatible routing, access, grade, and segment direction.
The merge blocks grade conflicts, restrictions, dangling references, duplicate adjacent references, and collapsed ways. It rejects area-only school and building vertices as routing targets.
This PR supports nodes and one source way to one base way. It reports one-to-many way chains as
unsupported-way-chain.Candidate review and recovery
The Merge app has a Match imported data step. It shows match evidence, tag differences, route context, and clear status labels.
The app supports filter-wide actions across all result pages:
The confirmation shows changed, skipped, automatic, review, and replaced-decision counts. Blocked and ambiguous candidates cannot use a bulk accept action.
When a filter changes, the old list remains visible as stale data. A spinner shows that the worker is updating the results.
The worker stores discovery options and decisions for restart recovery. A new input invalidates decisions from the previous input.
Merge app changes
The app now explains every workflow step. Collapsed details describe inputs, changes, safety rules, and outputs.
Other workflow changes include:
Diagnostic scans do not change OSM data. Reviewed and automatic workflows use the same merge rules.
Routing and merge tests
The routing harness records stable OSM evidence. It compares Dijkstra and A* without using internal graph indexes as expected values.
The Monaco matrix covers car and walk routes, one-way roads, roundabouts, steps, access cases, restrictions, and unreachable points.
Each accepted route runs against raw, merged, and PBF-reloaded data. Synthetic fixtures cover intersections, grade separation, restrictions, and imported-network attachment.
Local R5 runs remain the authority for Conveyal route legality. R5 is not a package or CI dependency.
The raw and merged Monaco matrices matched in local R5 at commit
ac95649c7094bf394b3be43fa523d0fb4447633e.The Yakima test used the 1-meter configuration with property transfer and network attachment:
These results include valid sidewalk matches and protected school-boundary cases. The large Yakima files remain local fixtures and do not run in CI.
Test speed and CI reliability
The Merge browser tests now use three ordered Playwright projects. One Chromium worker runs the real Merge flow, guidance tests, and worker-runtime tests.
The real Merge flow loads each input once. Lightweight tests cover responsive layout, filenames, file actions, and guidance text.
The real test uses the tracked
fixtures/monaco.pbffile. The prior CI test requested an ignored fixture and received Vite HTML instead of PBF data.The load helper now reports worker alerts immediately. GeoParquet tests build the Monaco fixture once for the suite.
The final hosted CI run passed without
test.slow(). The browser suite completed 16 tests in 21.8 seconds.The final local run completed 16 browser tests in 26.3 seconds. The original local baseline was 46.7 seconds.
Compatibility
conflationconfiguration.deduplicateNodesanddeduplicateWaysnow mean safe patch-to-base reconciliation.Old merged PBF files can contain damaged references. Regenerate these files from the original base and patch inputs.
Verification
pnpm run verify:workspace -- @osmix/changepnpm run verify:workspace -- @osmix/routerpnpm run verify:workspace -- @osmix/geoparquet --writepnpm run verify:workspace -- osmixpnpm run verify:workspace -- apps/mergepnpm run check:depspnpm run test— 834 passed and 3 skippedgit diff --checkosmium check-refs— no missing references in tested PBF files