Skip to content

Conversation

hwandrew
Copy link

@hwandrew hwandrew commented May 20, 2025

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

@hwandrew
Copy link
Author

@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.

@hwandrew
Copy link
Author

cc @chrisgervang


const settingsChanged = this._updateSettings(props, oldProps);
if (settingsChanged) {
this._createProxyTransform(this._map);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Pessimistress do you know why we had this?

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +171 to +173
onLoad={() => {
onLoad();
}}
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Redundant arrow wrapper calling onLoad; you can pass the onLoad function directly (onLoad={onLoad}) to simplify and avoid an unnecessary redefinition each render.

Suggested change
onLoad={() => {
onLoad();
}}
onLoad={onLoad}

Copilot uses AI. Check for mistakes.

return (
<Map
ref={mapRef}
mapLib={import('mapbox-gl-v3')}
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

});

async function onLoad() {
await sleep(1);
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

root.render(<App />);

await waitForMapLoad(mapRef);
await sleep(1);
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

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.

3 participants