fix: max depth exceeded error when dynamically changing map settings#2535
fix: max depth exceeded error when dynamically changing map settings#2535chrisgervang merged 3 commits intovisgl:masterfrom
Conversation
|
@Pessimistress wondering if I could get some eyes on this! The proxy transform solution seems really promising for my use case - functional terrain would be huge. |
|
|
||
| const settingsChanged = this._updateSettings(props, oldProps); | ||
| if (settingsChanged) { | ||
| this._createProxyTransform(this._map); |
There was a problem hiding this comment.
Pull Request Overview
Fixes a max call stack / maximum depth exceeded error caused by repeatedly proxy-wrapping the map transform when settings change.
- Removes re-instantiation of the proxy transform on settings updates.
- Adds a new test to verify dynamic updating of map settings (maxPitch) after initial load.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| modules/react-mapbox/src/mapbox/mapbox.ts | Stops recreating proxy transform on settings change to prevent recursive proxy wrapping. |
| modules/react-mapbox/test/components/map.spec.jsx | Adds test for delayed settings update (maxPitch) after map load. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return ( | ||
| <Map | ||
| ref={mapRef} | ||
| mapLib={import('mapbox-gl-v3')} |
There was a problem hiding this comment.
Calling import('mapbox-gl-v3') inside render will create a new Promise on every re-render when settings change. Memoize it (e.g., const mapLib = React.useMemo(() => import('mapbox-gl-v3'), []);) or move it outside the component to avoid repeated dynamic import calls.
| }); | ||
|
|
||
| async function onLoad() { | ||
| await sleep(1); |
There was a problem hiding this comment.
Using very short arbitrary sleeps (1ms) increases test flakiness; consider awaiting a specific condition (e.g., poll until mapRef.current.getMaxPitch() === 60) or using a higher-level helper instead of fixed tiny delays.
| root.render(<App />); | ||
|
|
||
| await waitForMapLoad(mapRef); | ||
| await sleep(1); |
There was a problem hiding this comment.
Using very short arbitrary sleeps (1ms) increases test flakiness; consider awaiting a specific condition (e.g., poll until mapRef.current.getMaxPitch() === 60) or using a higher-level helper instead of fixed tiny delays.
|
thanks for looking! @Pessimistress looks like I don't have permission to merge, so feel free to merge. Lmk if the copilot suggestions need to be addressed, but it clones the existing patterns used in that spec. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
3 participants
stems from an issue found in #2194 (comment)
When the check for if settings changed a new proxy transform is instantiated, but a previously instantiated proxy was already created and assigned to the argument. This call just created a proxy of the proxy (and kept doing so, hence the stack error) - I think we could just keep the original proxy object without reinstantiating.
Added a spec to dynamically update map settings after initial render to catch this.
Before:
Screen.Recording.2025-05-20.at.3.51.04.PM.mov
After:
Screen.Recording.2025-05-20.at.3.51.19.PM.trim.mov
Note
Avoid re-instantiating the map
ProxyTransformwhen settings change; add a test validating delayed settings updates (e.g.,maxPitch).ProxyTransforminsetPropswhen settings change, preventing recursive proxy wrapping.Map#uncontrolled#delayedSettingsUpdateto ensure dynamic settings (e.g.,maxPitch) update correctly after load.Written by Cursor Bugbot for commit e405c1c. This will update automatically on new commits. Configure here.