Merge develop into main: sync production with staging#510
Merge develop into main: sync production with staging#510ksalamy merged 510 commits intombari-org:mainfrom
Conversation
…comms-indicators Update comms section to provide accurate ACK times and display the method of communication and cell timeout
…cheduling Fix command scheduling issues
…-or-partial-mission-names Fix missing or partially displayed mission names
…and param in extract overrides
…-assets-for-map Add TrackDB functionality to overview and deployment maps
…ri-org#509) * Display seconds in CommsSection timestamps (fixes mbari-org#491) Extends the full-resolution timestamp display from LogsSection to CommsSection. Command and mission event timestamps now show H:mm:ss for correlation with other datastreams and sources. * Add test for H:mm:ss timestamp format in CommsSection Verifies timestamps include seconds for correlation with other datastreams.
…bari-org#508) - Map.tsx: Merge duplicate top-left Control blocks (Paintbrush, TrackDB, Layers, Center, Fit bounds, Add markers) into a single control group - index.tsx: Add useIsDesktop so only one layout (desktop or mobile) renders - [...deployment].tsx: Add useIsDesktop for same layout fix on Deployment page Prevents duplicate map controls when both desktop and mobile layouts mount.
There was a problem hiding this comment.
Pull request overview
This PR merges the develop branch into main, syncing production (dash5.mbari.org) with staging (dash5-staging.mbari.org). The changes represent accumulated development work since PR #333, bringing main up to date with the latest features and improvements.
Changes:
- Upgraded Next.js from v12 to v15 with static export configuration
- Added comprehensive document management system with TipTap editor
- Enhanced deployment tracking with improved comms monitoring and time handling
Reviewed changes
Copilot reviewed 116 out of 328 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docker/nginx.conf | New nginx configuration for serving Next.js static exports with dynamic route handling |
| docker/Dockerfile | Docker configuration for nginx-based static deployment |
| apps/lrauv-dash2/package.json | Next.js upgrade to v15 and TipTap editor dependencies |
| apps/lrauv-dash2/next.config.js | Updated for Next.js 15 with static export and workspace package transpilation |
| apps/lrauv-dash2/pages/_app.tsx | Added new context providers for markers, cookies, maps, and vehicle colors |
| apps/lrauv-dash2/lib/useLastCommsTime.ts | Enhanced to differentiate satellite and cell comms with separate timestamps |
| apps/lrauv-dash2/components/VehicleAccordion.tsx | Refactored to use millisecond timestamps and improved mission tracking |
| apps/lrauv-dash2/components/CommsSection.tsx | Updated to use millisecond timestamps and new comms events API |
| apps/lrauv-dash2/components/docs/* | New document editor/viewer components using TipTap |
| .github/workflows/build.yml | New GitHub Actions CI/CD workflow replacing CircleCI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 116 out of 328 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/lrauv-dash2/lib/makeCommand.ts
Outdated
| } | ||
| const t = DateTime.fromISO(specifiedTime) | ||
| const t = DateTime.fromISO(specifiedLocalTime).toUTC() | ||
| const schedDate = `${t.toFormat('yyyyMMdd')}}T${t.toFormat('HHmm')}` |
There was a problem hiding this comment.
The scheduled date string has an extra } which will generate malformed schedDate values (e.g., 20240320}T2100). Remove the stray brace so the format matches yyyyMMdd'T'HHmm.
| const schedDate = `${t.toFormat('yyyyMMdd')}}T${t.toFormat('HHmm')}` | |
| const schedDate = `${t.toFormat('yyyyMMdd')}T${t.toFormat('HHmm')}` |
| // Call this immediately on mount to start initializing the service | ||
| useState(() => { | ||
| getElevationService() | ||
| .then(() => { | ||
| setElevationAvailable(true) | ||
| logger.info('👍 Elevation service ready') | ||
| }) | ||
| .catch(() => { | ||
| setElevationAvailable(false) | ||
| logger.warn('⚠️ Elevation service unavailable') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
This uses useState for side effects, which is not supported and can lead to repeated initialization and warnings. Move this initialization into a useEffect(() => { ... }, []) so it runs once on mount (and is clearly modeled as an effect).
| export const VehicleColorsContext = createContext<VehicleColorsContextProps>({ | ||
| vehicleColors: {}, | ||
| defaultVehicleColors: DEFAULT_VEHICLE_COLORS, | ||
| setVehicleColor: () => {}, | ||
| resetVehicleColor: () => {}, | ||
| resetAllVehicleColors: () => {}, | ||
| debug: { | ||
| providerId: 'default', | ||
| instanceCount: 0, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Because the context is created with a non-undefined default value, useVehicleColors() cannot reliably detect 'missing provider' (the if (!context) check will never trigger). Create the context as createContext<VehicleColorsContextProps | undefined>(undefined) and handle the undefined case in the hook.
| const [selectedPlatformIds, setSelectedPlatformIds] = useState<string[]>( | ||
| () => { | ||
| try { | ||
| const stored = localStorage.getItem('selectedTrackingDbAssetIds') | ||
| return stored ? JSON.parse(stored) : [] | ||
| } catch { | ||
| return [] | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
This reads from localStorage during state initialization without guarding for non-browser environments. In Next.js this can crash during SSR/build (where localStorage is undefined). Add a typeof window !== 'undefined' guard (and similarly guard the localStorage.setItem(...) effect) so the provider is safe to import/render in SSR.
| <input | ||
| type="checkbox" | ||
| className="mr-2" | ||
| checked={selectedStations.includes(station)} |
There was a problem hiding this comment.
selectedStations.includes(station) relies on object reference equality. Since station objects can be re-created from API responses/grouping, this can cause checked states to be wrong even when a station with the same identity is selected. Use an identity comparison (e.g., by station.name or a stable id) such as selectedStations.some(s => s.name === station.name).
| checked={selectedStations.includes(station)} | |
| checked={selectedStations.some( | |
| (s) => s.id === station.id | |
| )} |
| test('should display timestamps with seconds (H:mm:ss)', async () => { | ||
| render( | ||
| <MockProviders queryClient={new QueryClient()}> | ||
| <CommsSection vehicleName="makai" from="" /> |
There was a problem hiding this comment.
CommsSection now expects from as a number (milliseconds since epoch). Passing an empty string will fail TypeScript checking and can cause runtime issues in date handling. Update the test to pass a numeric timestamp (and optionally to).
| bump-version: | ||
| if: github.event_name == 'pull_request' && github.event.pull_request.merged == true && github.event.pull_request.base.ref == 'develop' |
There was a problem hiding this comment.
This job is gated on pull_request.merged == true, but the workflow's pull_request trigger does not specify types: [closed]. As written, this job will never run because merge information is only set on the closed event. Add pull_request: types: [closed] (or move version bumping to a push/release-based trigger) so the condition can ever be true.
| const group = acc[p.typeName] ?? [] | ||
| group.push(p) | ||
| return { ...acc, [p.typeName]: group } |
There was a problem hiding this comment.
This reducer creates a new object on every iteration via { ...acc, ... }, which is unnecessarily expensive for large platform lists. Mutate the accumulator in-place (acc[p.typeName] = ...; return acc) to keep it O(n) without repeated object copies.
| const group = acc[p.typeName] ?? [] | |
| group.push(p) | |
| return { ...acc, [p.typeName]: group } | |
| if (!acc[p.typeName]) { | |
| acc[p.typeName] = [] | |
| } | |
| acc[p.typeName].push(p) | |
| return acc |
| @@ -1,24 +1,33 @@ | |||
| import { on } from 'events' | |||
There was a problem hiding this comment.
This import appears unused and also pulls in Node's events module, which is not intended for browser bundles. Remove the import to avoid unnecessary bundle weight and potential build tooling issues.
| import { on } from 'events' |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 117 out of 334 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const t = DateTime.fromISO(specifiedLocalTime).toUTC() | ||
| const schedDate = `${t.toFormat('yyyyMMdd')}}T${t.toFormat('HHmm')}` | ||
| return { | ||
| commandText, | ||
| commandText: resolvedCommandText, | ||
| schedDate, | ||
| previewSbd: `sched ${schedDate} "${commandText}"`, | ||
| previewSbd: `sched ${schedDate} "${resolvedCommandText}"`, |
There was a problem hiding this comment.
The schedDate format string includes an extra } (yyyyMMdd}}T...), which will produce an invalid schedule date (and break the added UTC conversion test). Remove the extra brace so the format becomes yyyyMMddT.....
| render( | ||
| <MockProviders queryClient={new QueryClient()}> | ||
| <CommsSection vehicleName="makai" from="" /> | ||
| </MockProviders> | ||
| ) |
There was a problem hiding this comment.
CommsSection now requires from: number (millis since epoch), but the test still passes an empty string. This will fail type-checking and doesn’t represent valid runtime usage. Update the test to pass a numeric millis value (e.g., from={0} or a fixed timestamp consistent with the mocks).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| import { on } from 'events' | ||
| import { useMapEvents } from 'react-leaflet' | ||
| import { useManagedWaypoints } from 'react-ui/dist' | ||
| import { createLogger } from '@mbari/utils' | ||
|
|
||
| const ClickableMapPoint = () => { | ||
| const logger = createLogger('ClickableMapPoint') | ||
| const ClickableMapPoint: React.FC<{ | ||
| onClick?: (lat: number, lng: number) => void | ||
| }> = ({ onClick }) => { |
There was a problem hiding this comment.
Importing { on } from Node’s events module will either be unused (lint error) or pull a Node-only dependency into the browser bundle. Additionally, React.FC is referenced but React isn’t imported, which will break TypeScript compilation. Remove the events import and import React types (e.g., import type React from 'react') or switch to FC imported from react.
| // Call this immediately on mount to start initializing the service | ||
| useState(() => { | ||
| getElevationService() | ||
| .then(() => { | ||
| setElevationAvailable(true) | ||
| logger.info('👍 Elevation service ready') | ||
| }) | ||
| .catch(() => { | ||
| setElevationAvailable(false) | ||
| logger.warn('⚠️ Elevation service unavailable') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
This uses useState to run an initialization side-effect. That initializer runs during render and can cause unexpected behavior (and strict-mode double-invocation issues) while also creating an unused state value. Move this initialization into a useEffect(() => { ... }, []) so it runs after mount, and remove unused imports/refs (useEffect is imported but not used; toast, elevationErrorShown, and depthLoading appear unused in this file).
| # No caching for development | ||
| add_header Cache-Control "no-cache, no-store, must-revalidate"; | ||
| add_header Pragma "no-cache"; | ||
| add_header Expires "0"; |
There was a problem hiding this comment.
The server-level Cache-Control: no-cache, no-store... header will apply to the image and js/css locations (those blocks only set expires). That effectively disables caching even though expires is set. Add explicit add_header Cache-Control ... inside the image and js/css locations (or remove the global no-cache headers) so caching behavior matches intent.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const { setTrackedVehicles, trackedVehicles } = useTrackedVehicles() | ||
| const vehicleNamesQuery = useSortedVehicleNames({ refresh: 'y' }) | ||
| const vehicleNames = vehicleNamesQuery.data ?? [] | ||
| const { data: vehicleNames } = useSortedVehicleNames() |
There was a problem hiding this comment.
vehicleNames can be undefined while loading; previously this code used a ?? [] fallback. If downstream components assume an array (e.g., doing .map), this can throw at runtime. Consider restoring a safe default (e.g., const vehicleNames = data ?? []) before passing it further.
| const { data: vehicleNames } = useSortedVehicleNames() | |
| const { data } = useSortedVehicleNames() | |
| const vehicleNames = data ?? [] |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…mports, useGoogleElevator side effect
…dash5 into merge-develop-into-main
…, VehicleDeploymentDropdown vehicleNames fallback
Syncs main with develop so production (dash5.mbari.org) can deploy the same code as staging (dash5-staging.mbari.org).
Changes
Post-merge
After merge, tag main (e.g. v5.1.0) to trigger production CI build.