Skip to content

Minor fixes - #1

Closed
emirkaan5 wants to merge 3 commits into
ESIPFed:mainfrom
emirkaan5:main
Closed

Minor fixes #1
emirkaan5 wants to merge 3 commits into
ESIPFed:mainfrom
emirkaan5:main

Conversation

@emirkaan5

Copy link
Copy Markdown
Contributor
  • I replaced port 8000 with 8080. This port (8000) conflicts with some Mac utility when run on apple devices. Change is applied to documentation as well.
  • errors regarding the sky layer is fixed.
  • removed herobrine

@emirkaan5
emirkaan5 requested a review from isConic August 5, 2026 16:14
Reference-only skills showing how to drive the web map with Puppeteer for
different animation scenarios: ballistic flyTo tours, 3D terrain orbit
(Matterhorn), keyframe screenshots, and frame-sequence recording. Each
scenario ships a SKILL.md recipe plus a runnable animate.mjs, over a shared
lib/esip-map.mjs helper that targets the page's published MapLibre instance.

Wired links into the README (docs table + Demos section). Terrain examples
use map.setSky(), reinforcing the correct MapLibre API.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@isConic

isConic commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Hey Emir,
thanks for the PR. The sky fix is good, and the port issue you ran into appears to expose a separate screenshot bug. I’m closing this version so the port-wide changes can be removed, but both fixes are worth resubmitting.

1. Keep the setSky fix

addLayer({ type: "sky" }) is a Mapbox API. MapLibre does not support sky as a layer type, so it fails style validation. Because MapLibre reports that failure through the error event rather than throwing, the existing try/catch wrappers do not catch it.

map.setSky() is the correct MapLibre 5+ API. Consolidating the four duplicated blocks into enableSky() and disableSky() is also a good cleanup. Keep this commit as-is.

2. Revert the 8000 → 8080 changes

Changing the container’s default port would break existing deployments that already reference 8000 in Compose files, proxy configurations, MAPCONTROL_PUBLIC_URL, or SDK calls.

Port 8000 is not reserved by macOS. Something may be listening on it locally, but that is a host-side conflict:

lsof -i :8000

The normal solution is to remap the host port while leaving the container port unchanged:

ports:
  - "8080:8000"

3. Fix screenshot self-navigation separately

The reason the remap did not work for screenshots appears to be that take_screenshot builds its URL from _public_base_url() and then opens it from Chromium running inside the container.

With MAPCONTROL_PUBLIC_URL=http://localhost:8080, that URL is correct for a client on the host, but not for Chromium inside the container. Inside the container, localhost:8080 refers to the container itself, where the service is still listening on 8000.

The screenshot path should use an internal loopback URL based on the server’s bound port:

def _internal_base_url() -> str:
    """Return the loopback URL used for server self-navigation."""
    return f"http://127.0.0.1:{load_config().server.port}"

Then, in take_screenshot:

map_url = f"{_internal_base_url()}/map/{map_id}"

The Playwright fallback in main.py appears to need the same change.

_public_base_url() should remain in use for URLs returned to external clients, including create_map links and map:// resources.

Please resubmit with

  1. The existing setSky commit
  2. The internal screenshot URL fix in mcp_tools.py and main.py
  3. A regression test covering a public URL that is not reachable from inside the container
  4. The .gitignore *.log addition
  5. Optionally, a README note showing how to use 8080:8000 when the host’s port 8000 is occupied

A useful regression case would set:

MAPCONTROL_PUBLIC_URL=http://unreachable.invalid:9999

and verify that take_screenshot still succeeds through the internal loopback URL.

Please drop the remaining 8000 → 8080 edits. The port conflict can then be handled through normal Docker port mapping without breaking screenshots or changing the image default.

And bring herobrine back.

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