Skip to content

[Sim Theme D] Build-time simulator selection#25

Draft
jmservera wants to merge 11 commits into
jmservera/sim-theme-b-launch-menufrom
jmservera/sim-theme-d-build-selection
Draft

[Sim Theme D] Build-time simulator selection#25
jmservera wants to merge 11 commits into
jmservera/sim-theme-b-launch-menufrom
jmservera/sim-theme-d-build-selection

Conversation

@jmservera

Copy link
Copy Markdown
Owner

Implements Theme D of the Simulation & Visualization PRD (FR-D1..D4). Lane 4 (integrate last) — depends on Theme A (registry) and the C/F simulator services existing. Compose profiles + build args select the installed set; default Gazebo + Turtlesim. Plan: docs/plans/sim-theme-d-build-selection.md. Draft.

@jmservera
jmservera force-pushed the jmservera/sim-theme-d-build-selection branch from ad8df59 to 8031f8f Compare July 21, 2026 11:30
@jmservera
jmservera changed the base branch from jmservera/gazebo-ros-integration to jmservera/sim-theme-b-launch-menu July 21, 2026 11:30
…s) built via npm proxy, served by gzweb-client service
…ice boots

The Theme A registry module services/control/simulators.js is imported by server.js but was never COPYed into the image, so the container crash-looped with ERR_MODULE_NOT_FOUND and stayed unhealthy, blocking every dependent service.
…Simulator panel

Autostarted simulators showed only a Stop button, so a running sim could never be opened as a panel; add an explicit Open action. noVNC defaulted its WebSocket to ws://host/websockify at the origin root (routed to the frontend, handshake failed) — pin the path to the simulator's proxied websockify endpoint. Remove the built-in gzweb 'Simulator' panel (and its layout preset) since simulators are now launched/opened on demand from the Simulators menu.
…imulator framework

Add S4a (gazebo gzweb), S4b (turtlesim noVNC, pinned ws path), and S10 (simulator menu + lifecycle); retire the old S4 novnc-frame spec. Update S5/S5b/S7 and helpers for the removed built-in Simulator panel and the 9-service topology; refresh docs and smoke checks. Ignore .playwright-mcp/ artifacts.
Bundling ros-\-turtlesim lets operators drive and inspect the Turtlesim node from the ROS terminal (e.g. ros2 run turtlesim turtle_teleop_key) now that Turtlesim joins the shared ROS graph via the discovery server.
…pi see the full graph

Under the Fast DDS discovery server a plain CLIENT only receives discovery data for endpoints it matches, so rosapi's services never propagated to rosbridge ('/rosapi/topics does not exist') and the ROS Status panel node query failed (S3). The ros container hosts the introspection surface (rosbridge, rosapi, ros2 CLI daemon), which must observe the entire graph; SUPER_CLIENT receives the full graph the server relays. Other containers stay lightweight CLIENTs.
@jmservera

Copy link
Copy Markdown
Owner Author

🤖 Automated PR review

Build-time selection — per-sim compose profiles (sim-gazebo, sim-turtlesim, umbrella simulators), converts the gzweb client into a Vite-built gzweb-client service (real Three.js render), repoints the proxy /gzweb/ to that upstream, adds turtlesim to the ros image, and updates tests. Base: #23 (top of the stack).

Findings

Severity Location Finding
🟡 Medium services/gazebo/client/ No committed package-lock.json, and the Dockerfile runs npm install (the lockfile COPY is an optional glob). Builds are non-reproducible and drift from the frontend image's npm ci convention. Commit a lockfile and switch to npm ci.
🟢 Low services/gazebo/client/package.json three@^0.141.0 (2022-era) and protobufjs@^6 are old; acceptable if pinned to match gzweb@^3.0.1, but worth a conscious note (and a security glance at transitive deps).

✅ This PR fixes the control image (COPY simulators.js — the omission that breaks #22#23). The proxy /gzweb/ location is correctly repointed to the always-on gzweb_client upstream (no dangling alias after the bind-mount removal). Strong new acceptance coverage in s10-simulator-menu.spec.js (API contract, menu render, real stop→launch cycle, 403 on unknown id). .env.example tandem docs for COMPOSE_PROFILES + UBEROS_SIMULATORS are clear.

Note: the COPY simulators.js fix ideally belongs in #22 so the intermediate stacked branches are independently deployable.

Recommendation: Approve after adding a lockfile + npm ci.

@jmservera jmservera left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤖 Automated line-anchored review — see the summary comment for the full findings table.

ARG NPM_REGISTRY=https://registry.npmjs.org/
WORKDIR /app
COPY package.json package-lock.json* ./
RUN npm config set registry "$NPM_REGISTRY" && npm install

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Medium — non-reproducible build. npm install with no committed package-lock.json (the lockfile COPY above is an optional glob) resolves fresh transitive versions on every build and drifts from the frontend image's npm ci convention. Commit services/gazebo/client/package-lock.json and switch this to npm ci. Nice work otherwise: this PR also fixes the control image (COPY simulators.js) and correctly repoints the proxy /gzweb/ to the always-on gzweb-client upstream.

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.

1 participant