fix: prevent app wide crash when terrain properties are undefined - #2582
fix: prevent app wide crash when terrain properties are undefined#2582mbret wants to merge 9 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dee6ef5425
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (error) { | ||
| // eslint-disable-next-line | ||
| console.error(error); |
There was a problem hiding this comment.
This catch block swallows all map.removeSource exceptions and only logs them, which leaves the source mounted when cleanup fails; that can silently corrupt later renders because map.getSource(id) will return stale data on remount (especially with explicit ids), and in StrictMode/unmount-heavy flows it can accumulate orphaned auto-generated sources instead of actually cleaning up. Limiting the catch to the known terrain bug condition (and rethrowing otherwise) would avoid masking real cleanup failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex, source is correctlyl removed, only the following updating terrain path is failing. Which in this situation is unrecoverable anyway.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
|
Can you run |
@cursor run |
|
@mbret any chance you could run the linter and also push the clean files, so this can be merged? |
|
@codex run |
|
To use Codex here, create an environment for this repo. |
… order to avoid error in Maplibre (visgl#2573) * Add updateZoomConstraint helper * Fix types, prevent duplicate call to setMinZoom or setMaxZoom * Add the same logic for pitch, format code * Return whether or not changes were applied from the updatePitchConstraint and updateZoomConstraint func * Add back propPresent * yarn lint fix * refactor: cleanup tests, prop names, code duplication, comments --------- Co-authored-by: Xiaoji Chen <Pessimistress@users.noreply.github.com>
* fix(react-maplibre): support maplibre-gl v6 (transform getters, setTransformCameraUpdate, event typing) * clean up * clean up * code review * test(react-maplibre): cover public camera getters * fix(react-maplibre): bridge box zoom event types * fix: deduplicate gl-matrix dependency * chore: rely on deduplicated gl-matrix lockfile * test: configure MapLibre worker for browser tests * ci: test MapLibre v6 * fix(react-maplibre): bridge versioned MapLibre types * refactor(react-maplibre): simplify layer property setters --------- Co-authored-by: Chris Gervang <chris.gervang@joby.aero>
6 participants
[Mapbox GL related]
We have frequent "random" crash on our app when unmounting components.
After tracking it for a while I came to the conclusion that it might be a bug in mapbox library itself. It seems that
map.removeSource(id)can sometimes crash the app due to this property being undefined https://github.com/mapbox/mapbox-gl-js/blob/7a72385de5c7400647ea7d3539637145fdf616a7/src/terrain/terrain.ts#L380.It also seems to be much much more frequent when in StrictMode.
Why it's undefined is honestly a mystery but I figured we may as well try/catch for good faith.
I am gonna create an issue on the official repo as well as I understand this is more of a bandaid than an actual fix.
Note
Medium Risk
Core map camera and constraint logic spans three MapLibre major versions; regression risk is mitigated by the new CI matrix but changes are in critical map lifecycle paths.
Overview
Adds MapLibre GL JS v6 compatibility across the MapLibre wrapper, examples, docs, and CI, alongside a Mapbox fix for crashes when removing sources while terrain properties are still unevaluated.
The MapLibre layer now works across v4/v5/v6: camera state is read via public getters instead of
map.transform, v6 usessetTransformCameraUpdate, and min/max zoom/pitch updates avoid invalid transient ranges.GlobeControlis exported and fails clearly when the host MapLibre build lacks it.CI runs tests against maplibre-gl ^4, ^5, and ^6 (Node 24.18). MapLibre examples and the site configure the v6 worker (
setWorkerUrl/ Vite?worker&url). Docs cover namespace imports and worker setup for v6.For Mapbox,
Sourceremoval goes throughremoveSource, which swallows a known terrain-relatedTypeErrorduring unevaluated terrain state (with tests).Reviewed by Cursor Bugbot for commit 67e8f72. Bugbot is set up for automated code reviews on this repo. Configure here.