Skip to content

Fix Circular Dependencies for Map#511

Open
ZackLyon wants to merge 15 commits intodevelopfrom
gh507-map-rerenders
Open

Fix Circular Dependencies for Map#511
ZackLyon wants to merge 15 commits intodevelopfrom
gh507-map-rerenders

Conversation

@ZackLyon
Copy link
Collaborator

Library build and types

  • react-ui tsconfig: Overrode baseUrl and paths so the package builds only its own src/, producing dist/index.d.ts and avoiding monorepo path resolution into other packages.
  • utils: Exported useDepthRequest from @mbari/utils so MapDepthDisplay can import from the package root instead of a subpath.
  • react-ui Map: Resolved TS/build issues: Google base layer uses a type cast for the type prop; MapDepthDisplay imports from @mbari/utils and types the depth promise result; MissionStep imports SortDirection via a relative path instead of react-ui/src/....

Single Google + Leaflet loader (app only)

  • New apps/lrauv-dash2/lib/leafletPlugins.ts: One place that loads the Google Maps script (with places and elevation), ensures Leaflet is loaded and window.L is set, then dynamically imports the googlemutant plugin. Idempotent via a shared promise.
  • GoogleMapsProvider: No longer injects its own script; calls initLeafletGoogle(apiKey) when a key is available.
  • useGoogleMaps: No longer uses @googlemaps/js-api-loader or Loader().importLibrary(). It only reports whether window.google?.maps is available (e.g. via polling).

Map stays a pure library

  • react-ui Map: Removed all Google/plugin loading (no loadGoogleMapsOnce, no dynamic import of googlemutant, no mapready listener that added a Google layer). Map only conditionally renders the Google base layer when window.google?.maps exists and uses useGoogMapsLoader={false}.
  • react-ui Map: Removed @mbari/react-ui/src/css/base.css import; kept Leaflet and package dist CSS imports.
  • Vehicle colors: Map no longer imports or renders VehicleColorsModal. It only invokes onRequestVehicleColors(anchor) with an optional { top, left } anchor. The app owns the modal and passes the callback.

App: vehicle colors modal and callbacks

  • Map.types: onRequestVehicleColors is now (anchor?: { top: number; left: number }) => void.
  • DeploymentMap: Owns VehicleColorsModal state and renders the modal as a sibling of Map (not inside Map) to avoid Leaflet DOM / insertBefore errors. handleVehicleColorRequest accepts an optional anchor and sets modal position.
  • Index (overview): Added VehicleColorsModal and handler; handler accepts anchor and sets position; modal is rendered as a sibling of Map. Wired to existing close handler (including map invalidateSize).
  • VehicleColorsModal: Imports Modal from @mbari/react-ui instead of @mbari/react-ui/src/Modal/Modal.

Next and app types

  • next.config.js: Removed the custom webpack rule that applied babel-loader to workspace TS; kept transpilePackages for @mbari/react-ui, @mbari/utils, @mbari/api-client.
  • App types: Added apps/lrauv-dash2/types/global.d.ts with a single Window.google?: any declaration. Removed duplicate Window.google declarations from useGoogleMaps.ts, leafletPlugins.ts, and elevationService.ts.
  • App types: Added apps/lrauv-dash2/types/leaflet-googlemutant-shim.d.ts so the dynamic import of leaflet.gridlayer.googlemutant/dist/Leaflet.GoogleMutant.js is a valid module for TypeScript.

@ZackLyon ZackLyon requested a review from jimjeffers February 24, 2026 22:02
@jimjeffers
Copy link
Collaborator

Thanks for the refactor here — the direction is solid (pure react-ui map + app-owned side effects). I found 3 priority reliability issues worth fixing before merge, with a workaround and code example for each.

1) initPromise can get stuck in a permanent rejected state

Where: apps/lrauv-dash2/lib/leafletPlugins.ts (initPromise lifecycle)

Problem
If initialization fails once (network hiccup, blocked script, transient plugin load issue), initPromise remains a rejected promise forever. Future calls immediately fail and never retry until full page reload.

Workaround
Reset initPromise on rejection so later calls can retry.

Example

let initPromise: Promise<void> | null = null

export function initLeafletGoogle(apiKey: string): Promise<void> {
  if (typeof window === 'undefined') {
    return Promise.reject(new Error('initLeafletGoogle must run in the browser'))
  }

  if (window.google?.maps) {
    return initPromise ?? (initPromise = loadGoogleMutantOnly())
  }

  if (!initPromise) {
    initPromise = (async () => {
      // existing init logic...
      const existing = document.getElementById('google-maps-script')
      if (existing) {
        await waitForGoogle(10000)
        await loadGoogleMutantOnly()
        return
      }

      const script = document.createElement('script')
      script.id = 'google-maps-script'
      script.src = `https://maps.googleapis.com/maps/api/js?key=${apiKey}&libraries=places,elevation`
      script.async = true
      script.defer = true

      await new Promise<void>((resolve, reject) => {
        script.onload = () => resolve()
        script.onerror = (e) => reject(e)
        document.head.appendChild(script)
      })

      await waitForGoogle(10000)
      await loadGoogleMutantOnly()
    })().catch((err) => {
      initPromise = null // allow retry on next call
      throw err
    })
  }

  return initPromise
}

2) waitForGoogle() can hang forever

Where: apps/lrauv-dash2/lib/leafletPlugins.ts (waitForGoogle)

Problem
Current requestAnimationFrame polling has no timeout/reject path. If window.google.maps never appears, init can stall indefinitely.

Workaround
Add timeout + reject path; always clean up timer/RAF handles.

Example

function waitForGoogle(timeoutMs = 10000): Promise<void> {
  if (window.google?.maps) return Promise.resolve()

  return new Promise((resolve, reject) => {
    const start = Date.now()
    let rafId = 0

    const check = () => {
      if (window.google?.maps) {
        cancelAnimationFrame(rafId)
        resolve()
        return
      }

      if (Date.now() - start >= timeoutMs) {
        cancelAnimationFrame(rafId)
        reject(new Error('Timed out waiting for Google Maps API'))
        return
      }

      rafId = requestAnimationFrame(check)
    }

    rafId = requestAnimationFrame(check)
  })
}

3) useGoogleMaps keeps polling forever after success

Where: apps/lrauv-dash2/lib/useGoogleMaps.ts

Problem
After maps load, interval polling continues for app lifetime (200ms loop) because it never clears itself when success condition is reached.

Workaround
Clear interval immediately once window.google?.maps is available.

Example

useEffect(() => {
  if (typeof window === 'undefined') return

  if (window.google?.maps) {
    setMapsLoaded(true)
    return
  }

  const id = setInterval(() => {
    if (window.google?.maps) {
      clearInterval(id)
      setMapsLoaded(true)
    }
  }, 200)

  return () => clearInterval(id)
}, [])

If helpful, I can also put together a small follow-up commit that includes these changes in one patch.

@jimjeffers
Copy link
Collaborator

@ZackLyon that was codex talking. It posted before I could personally edit but I asked it to prioritize these 3 potential bugs for you. Take a look and see if they're worth attention now as opposed to later.

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.

2 participants