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 1 commit intovisgl:masterfrom
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.
1 participant
[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.