feat(weather): add tidal currents overlay with time-stepping controls#491
feat(weather): add tidal currents overlay with time-stepping controls#491marcelrv wants to merge 2 commits into
Conversation
Display tidal current arrow field on the map using signalk-tidal-currents plugin data. Includes: - TidalCurrentsService: state (scrubTime, play/pause), 24h bar tick/label math, API via SignalKClient - TidalCurrentsLayerComponent: OpenLayers vector layer with speed-colored arrows, auditTime(300) pipe - Weather panel: clickable/draggable 24h time bar, play/pause/step/reset buttons, pill label - Feature flag (tidalApi) gated on plugin detection via /signalk/v2/features - Dark/light theme support via app.uiConfig().invertColor - Click handling delegated to Freeboard's processMapClick for consistent popovers
📝 WalkthroughWalkthroughAdds tidal currents feature detection, config support, a grid-data service, an OpenLayers rendering layer, tidal popovers, and weather-list controls for enabling and scrubbing tidal currents. ChangesTidal currents feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WeatherListComponent
participant TidalCurrentsService
participant TidalCurrentsLayerComponent
participant SignalKAPI
User->>WeatherListComponent: enable tidal currents / drag time bar
WeatherListComponent->>TidalCurrentsService: update scrubTime
TidalCurrentsService-->>TidalCurrentsLayerComponent: selectedTime changes
TidalCurrentsLayerComponent->>TidalCurrentsService: getGridCurrents(bbox, time)
TidalCurrentsService->>SignalKAPI: fetch /currents/grid
SignalKAPI-->>TidalCurrentsService: TidalCurrentGridResponse
TidalCurrentsLayerComponent->>TidalCurrentsLayerComponent: render currents features
TidalCurrentsLayerComponent-->>WeatherListComponent: map overlay updates
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/app/modules/map/fb-map.component.ts(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.6.0 /.eslintrc:2 SyntaxError: Unexpected token ':' src/app/modules/map/fbmap-interact.service.ts(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.6.0 /.eslintrc:2 SyntaxError: Unexpected token ':' src/app/modules/map/ol/lib/resources/tidal-currents-layer.component.ts(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.6.0 /.eslintrc:2 SyntaxError: Unexpected token ':'
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/app/modules/map/ol/lib/tidal-currents.service.ts (1)
111-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: unencoded query params in
getGridCurrents.
bboxandtime.toISOString()are interpolated directly into the URL withoutencodeURIComponent. Low risk today since values are numeric/ISO, but defensively encoding avoids future breakage if formatting changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/modules/map/ol/lib/tidal-currents.service.ts` around lines 111 - 121, The query string built in getGridCurrents is interpolating bbox and time directly into the URL without encoding, so update the URL construction in TidalCurrentsService#getGridCurrents to wrap each query value with encodeURIComponent. Keep the same request shape for sk.api.get, but ensure both the bbox parameter and the optional time.toISOString() value are safely encoded before concatenation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/modules/map/fb-map.component.ts`:
- Around line 1298-1305: `formatPopover` currently has no `case 'tidal'`, so
tidal clicks still fall through to the default return and never render a popover
even though `processMapClick` routes them there. Add a `case 'tidal'` branch in
`fb-map.component.ts`’s `formatPopover` to populate the tidal overlay data
(using the existing feature name/details flow), and add the matching `@if
(overlay().type === 'tidal')` popover block in `fb-map.component.html` so
single-feature and list selections both display correctly.
In `@src/app/modules/map/ol/lib/resources/tidal-currents-layer.component.ts`:
- Around line 176-234: The current tidal currents rendering bypasses the numeric
formatting guideline for direction and duplicates speed conversion/formatting in
both renderCurrents and currentsStyleFunction. Update renderCurrents to format
point.direction through app.formatValueForDisplay instead of toFixed, and
extract a shared helper for knots-to-m/s conversion plus display formatting so
both renderCurrents and currentsStyleFunction reuse the same logic. Keep the
changes centered around renderCurrents, currentsStyleFunction, and the new
helper to ensure all user-facing numbers go through app.formatValueForDisplay.
In `@src/app/modules/map/ol/lib/tidal-currents.service.ts`:
- Around line 50-107: The real-time “Now” path in tidal-currents.service.ts is
using Date.now() inside hourTicks, hourLabels, and barBgGradient without any
reactive/time-based dependency, so those computed values never refresh while
scrubTime() remains null. Add a periodic clock signal or timer-backed state in
TidalCurrentsService and reference it from those computed getters so they
recompute automatically in the live view, while keeping the existing
scrubTime()-based behavior unchanged.
- Around line 169-186: Pause the tidal playback timer when the panel is hidden
or closed, since toggleTidalCurrents(false) and close() currently only update
state/emit events and do not stop the active interval. Update the flow around
toggleTidalCurrents, close, and the tidal currents service methods (play, pause)
so that currents.pause() is invoked on disable/close, or ensure the timer is
cleared during service teardown. Make sure the existing playTimer lifecycle is
fully reset when playback is dismissed.
In `@src/app/modules/skresources/components/weather/weatherlist.html`:
- Around line 128-136: The tooltip labels on the big step buttons are
inconsistent with the actual behavior of currents.stepBackBig() and
currents.stepForwardBig(), which only move by 1h. Update the matTooltip values
in weatherlist.html to reflect the real 1h jump, or change the
stepBackBig/stepForwardBig implementation so the movement matches the existing
-6h/+6h labels.
---
Nitpick comments:
In `@src/app/modules/map/ol/lib/tidal-currents.service.ts`:
- Around line 111-121: The query string built in getGridCurrents is
interpolating bbox and time directly into the URL without encoding, so update
the URL construction in TidalCurrentsService#getGridCurrents to wrap each query
value with encodeURIComponent. Keep the same request shape for sk.api.get, but
ensure both the bbox parameter and the optional time.toISOString() value are
safely encoded before concatenation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f4cc566-b114-4c81-b6de-77d4e7594ba6
📒 Files selected for processing (11)
src/app/app.component.tssrc/app/app.config.tssrc/app/app.facade.tssrc/app/modules/map/fb-map.component.htmlsrc/app/modules/map/fb-map.component.tssrc/app/modules/map/ol/index.tssrc/app/modules/map/ol/lib/resources/tidal-currents-layer.component.tssrc/app/modules/map/ol/lib/tidal-currents.service.tssrc/app/modules/skresources/components/weather/weatherlist.htmlsrc/app/modules/skresources/components/weather/weatherlist.tssrc/app/types/index.d.ts
| case 'tidal': | ||
| addToFeatureList = true; | ||
| icon = { | ||
| name: 'water', | ||
| svgIcon: undefined | ||
| }; | ||
| text = feature.get('name'); | ||
| break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a case 'tidal' handler in formatPopover to show a popover when tidal features are clicked.
processMapClick correctly adds tidal features to the feature list, but formatPopover (lines 1391–1600) has no case 'tidal' — it falls through to default: return; at line 1598. This means:
- Single tidal feature click:
formatPopover('tidal.0', coord)is called →default: return;→ no popover appears. - Multi-feature list: Selecting a tidal feature calls
formatPopover('tidal.0', coord)→ samedefault: return;→ no popover.
The PR objective states click handling is routed through processMapClick for "consistent popovers," but the popover is never shown. A case 'tidal' in formatPopover and a corresponding popover template in the HTML are needed to complete the flow.
🐛 Proposed fix: add `case 'tidal'` to `formatPopover`
case 'aircraft':
if (!this.app.data.aircraft.has(id)) {
return false;
}
poData.type = t[0];
poData.id = id;
poData.aircraft = this.app.data.aircraft.get(id);
poData.position = poData.aircraft.position;
poData.show = true;
break;
+ case 'tidal':
+ poData.id = id;
+ poData.type = 'tidal';
+ poData.title = 'Tidal Current';
+ poData.position = coord;
+ poData.show = true;
+ poData.readOnly = true;
+ break;
case 'region':A corresponding @if (overlay().type === 'tidal') block will also be needed in fb-map.component.html to render the popover content (e.g., displaying feature.get('name')).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/modules/map/fb-map.component.ts` around lines 1298 - 1305,
`formatPopover` currently has no `case 'tidal'`, so tidal clicks still fall
through to the default return and never render a popover even though
`processMapClick` routes them there. Add a `case 'tidal'` branch in
`fb-map.component.ts`’s `formatPopover` to populate the tidal overlay data
(using the existing feature name/details flow), and add the matching `@if
(overlay().type === 'tidal')` popover block in `fb-map.component.html` so
single-feature and list selections both display correctly.
| <button | ||
| mat-icon-button | ||
| (click)="currents.stepBackBig()" | ||
| matTooltip="-6h" | ||
| matTooltipPosition="top" | ||
| style="width: 26px; height: 26px; line-height: 26px" | ||
| > | ||
| <mat-icon style="font-size: 14px">skip_previous</mat-icon> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files
git ls-files 'src/app/modules/skresources/components/weather/weatherlist.html' \
'src/app/modules/skresources/services/tidal-currents.service.ts'
# Show the relevant line ranges with line numbers
echo '--- weatherlist.html ---'
sed -n '120,180p' src/app/modules/skresources/components/weather/weatherlist.html | cat -n
echo '--- tidal-currents.service.ts ---'
sed -n '1,240p' src/app/modules/skresources/services/tidal-currents.service.ts | cat -nRepository: SignalK/freeboard-sk
Length of output: 3313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the service that defines the big step methods
rg -n "stepBackBig|stepForwardBig" src/app
# If found, show the relevant file with line numbers
service_file=$(rg -l "stepBackBig|stepForwardBig" src/app | head -n 1 || true)
if [ -n "${service_file}" ]; then
echo "--- ${service_file} ---"
sed -n '1,240p' "${service_file}" | cat -n
fiRepository: SignalK/freeboard-sk
Length of output: 7110
Tooltips should match the 1h jump. The -6h/+6h labels are misleading: currents.stepBackBig() and currents.stepForwardBig() only move by 1h. Update the tooltips or the step size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/modules/skresources/components/weather/weatherlist.html` around lines
128 - 136, The tooltip labels on the big step buttons are inconsistent with the
actual behavior of currents.stepBackBig() and currents.stepForwardBig(), which
only move by 1h. Update the matTooltip values in weatherlist.html to reflect the
real 1h jump, or change the stepBackBig/stepForwardBig implementation so the
movement matches the existing -6h/+6h labels.
…erlay - Show a popover on tidal-current feature clicks (formatPopover was missing a case for them), with a compact speed/direction layout instead of a verbose sentence - Route current direction through formatValueForDisplay and dedupe speed formatting into a helper - Keep the 24h time bar advancing in "now" mode instead of freezing - Pause playback when the tidal panel is hidden or the overlay is disabled - Fix the big step buttons to actually jump 6h, matching their tooltip Adds an opt-in `compact` mode to the shared popover component (mirrors the existing `measure` mode) so the tidal popover can be small without affecting any other popover's width or header.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/modules/map/fb-map.component.ts (2)
1357-1357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing grid-point type instead of an inline shape.
tidalFeatures: Record<string, { speedKn: number; direction: number }>re-declares a shape that already exists as part ofTidalCurrentGridResponse['points']intidal-currents.service.ts. Reusing/importing that type keeps the two files in sync if the grid response shape changes.As per coding guidelines, "New code should be TypeScript with real types; avoid
anyand reuse existing types instead of inventing local ones."♻️ Proposed fix
- private tidalFeatures: Record<string, { speedKn: number; direction: number }> = {}; + private tidalFeatures: Record< + string, + Pick<TidalCurrentGridResponse['points'][number], 'speedKn' | 'direction'> + > = {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/modules/map/fb-map.component.ts` at line 1357, The `tidalFeatures` field in `FbMapComponent` is duplicating the grid-point shape with an inline type instead of reusing the existing `TidalCurrentGridResponse['points']` type from `tidal-currents.service.ts`. Update `FbMapComponent` to import/refer to that existing type and use it for `tidalFeatures`, so the map component stays aligned with the service response shape and avoids local type drift.Source: Coding guidelines
1504-1522: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated knots→m/s conversion factor.
tf.speedKn / 1.94384duplicates the same conversion logic the commit history says was consolidated into aformatSpeedhelper insidetidal-currents-layer.component.ts(used for the feature'sname/list-popover text). This popover path reimplements the magic-number conversion inline instead of reusing that helper, so a future unit-constant fix in one place won't propagate to the other.♻️ Proposed direction
Move the knots→m/s +
formatValueForDisplayformatting intoTidalCurrentsService(or export it from the layer component) and call the shared helper from bothTidalCurrentsLayerComponent.renderCurrentsandfb-map.component.ts'sformatPopover.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/modules/map/fb-map.component.ts` around lines 1504 - 1522, The tidal popover in formatPopover is duplicating the knots-to-m/s conversion instead of reusing the shared formatting logic. Move the conversion plus formatValueForDisplay behavior into a shared helper in TidalCurrentsService or export the existing formatSpeed logic from tidal-currents-layer.component.ts, then use that helper both in TidalCurrentsLayerComponent.renderCurrents and in fb-map.component.ts when building poData.tidal.speedLabel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/modules/map/fb-map.component.ts`:
- Line 1357: The `tidalFeatures` field in `FbMapComponent` is duplicating the
grid-point shape with an inline type instead of reusing the existing
`TidalCurrentGridResponse['points']` type from `tidal-currents.service.ts`.
Update `FbMapComponent` to import/refer to that existing type and use it for
`tidalFeatures`, so the map component stays aligned with the service response
shape and avoids local type drift.
- Around line 1504-1522: The tidal popover in formatPopover is duplicating the
knots-to-m/s conversion instead of reusing the shared formatting logic. Move the
conversion plus formatValueForDisplay behavior into a shared helper in
TidalCurrentsService or export the existing formatSpeed logic from
tidal-currents-layer.component.ts, then use that helper both in
TidalCurrentsLayerComponent.renderCurrents and in fb-map.component.ts when
building poData.tidal.speedLabel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 548817c2-a429-48af-8b1f-28a6acc840a9
📒 Files selected for processing (8)
src/app/modules/map/fb-map.component.htmlsrc/app/modules/map/fb-map.component.tssrc/app/modules/map/fbmap-interact.service.tssrc/app/modules/map/ol/lib/resources/tidal-currents-layer.component.tssrc/app/modules/map/ol/lib/tidal-currents.service.tssrc/app/modules/map/popovers/popover.component.scsssrc/app/modules/map/popovers/popover.component.tssrc/app/modules/skresources/components/weather/weatherlist.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/modules/map/fbmap-interact.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/modules/skresources/components/weather/weatherlist.ts
- src/app/modules/map/ol/lib/resources/tidal-currents-layer.component.ts
1 participant
Display tidal current arrow field on the map using signalk-tidal-currents plugin data. Includes:
Summary by CodeRabbit
New Features
Bug Fixes