Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ The mock serves Skip's full session/config surface (`loginStatus`, `applicationD

**Runtime data pipeline** (`src/app/core/services/`): `SignalKConnectionService` (endpoint discovery) → `ConnectionStateMachine` (explicit connection lifecycle; registers callbacks so it carries no upward deps) → `SignalKDeltaService` (parses SK delta messages) → `DataService` (central hub mapping deltas to per-path observables; every value is written to both a `default` bucket and a per-`$source` bucket) → widgets. The DI graph is intentionally acyclic: `connection ← auth ← storage ← settings`, `data ← delta ← connection`.

**Widgets** (`src/app/widgets/`, ~46) are standalone components composed with three **host directives** that own runtime concerns: `WidgetRuntimeDirective` (config merge), `WidgetStreamsDirective` (diff-based path subscriptions), `WidgetMetadataDirective` (zones/meta). `WidgetService` is the registry — `kipWidgets` is a getter over the `_widgetDefinition` array; the full electrical family (bms, solar-charger, charger, alternator, inverter, ac) is registered and live.
**Widgets** (`src/app/widgets/`, ~46) are standalone components composed with three **host directives** that own runtime concerns: `WidgetRuntimeDirective` (config merge), `WidgetStreamsDirective` (diff-based path subscriptions), `WidgetMetadataDirective` (zones/meta). A widget that holds stream-derived presentation state owns clearing it: `WidgetStreamsDirective` rebuilds the subscription on a re-point, but `suppressBootstrapNull: true` filters the replayed leading null, so against a path that reports nothing the callback never runs and the previous path's reading stays on screen as a live reading of the new one. Compare `widgetPathSignature()` across effect runs and clear on a change — the three ng-gauges do; the other `streams.observe` callers do not yet (#585). `WidgetService` is the registry — `kipWidgets` is a getter over the `_widgetDefinition` array; the full electrical family (bms, solar-charger, charger, alternator, inverter, ac) is registered and live.

**Config & persistence**: `SettingsService` holds in-memory config plus sync getters and observable getters. `StorageService` persists through the SK server's **applicationData REST API**, which has exactly two scopes — `user` (the authenticated user's private store) and `global` (a single shared bucket). All writes go through a **sequential JSON-Patch queue** because SK can't handle concurrent applicationData writes. `ConfigurationUpgradeService` migrates older config file versions (preserve the stored version on write).

Expand Down
50 changes: 49 additions & 1 deletion src/app/core/directives/widget-streams.directive.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { TestBed } from '@angular/core/testing';
import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest';
import { Subject, BehaviorSubject, Observable } from 'rxjs';
import { WidgetStreamsDirective } from './widget-streams.directive';
import { WidgetStreamsDirective, widgetPathSignature, normalizeWidgetPath } from './widget-streams.directive';
import { DataService, IPathUpdate } from '../services/data.service';
import { UnitsService } from '../services/units.service';
import { IWidgetSvcConfig, IWidgetPath } from '../interfaces/widgets-interface';
Expand Down Expand Up @@ -901,3 +901,51 @@ describe('WidgetStreamsDirective TTL value reset (#1069)', () => {
expect(hits[hits.length - 1]).toBeNull();
});
});

/**
* The identity a widget compares to tell a re-point apart from an unrelated reconfigure. The
* directive computes it to decide whether to rebuild a subscription; the gauges compute it to decide
* whether the reading on screen still describes the path being watched. Both must agree, which is
* why it is one exported function rather than two implementations.
*/
describe('widgetPathSignature', () => {
const base = { path: 'navigation.speedOverGround', pathType: 'number', convertUnitTo: 'knots', source: null, suppressBootstrapNull: true };

it('returns null for a config with no usable path', () => {
expect(widgetPathSignature(undefined)).toBeNull();
expect(widgetPathSignature(null)).toBeNull();
expect(widgetPathSignature({ ...base, path: null })).toBeNull();
expect(widgetPathSignature({ ...base, path: '' })).toBeNull();
// Whitespace passes the widget-options required check, so it has to normalize to "no path"
// here rather than becoming an identity of its own.
expect(widgetPathSignature({ ...base, path: ' ' })).toBeNull();
});

it('treats a trimmed path and its padded form as the same reading', () => {
expect(widgetPathSignature({ ...base, path: ' navigation.speedOverGround ' }))
.toBe(widgetPathSignature(base));
});

it('treats an unset source and the default source as the same reading', () => {
expect(widgetPathSignature({ ...base, source: null })).toBe(widgetPathSignature({ ...base, source: 'default' }));
expect(widgetPathSignature({ ...base, source: ' ' })).toBe(widgetPathSignature({ ...base, source: 'default' }));
});

it('separates readings that differ in path, source, type, unit or bootstrap-null policy', () => {
const sig = widgetPathSignature(base);
expect(widgetPathSignature({ ...base, path: 'navigation.speedThroughWater' })).not.toBe(sig);
expect(widgetPathSignature({ ...base, source: 'gps-2' })).not.toBe(sig);
expect(widgetPathSignature({ ...base, pathType: 'string' })).not.toBe(sig);
// A unit change re-expresses the number, so the displayed reading is no longer the same one.
expect(widgetPathSignature({ ...base, convertUnitTo: 'kph' })).not.toBe(sig);
expect(widgetPathSignature({ ...base, suppressBootstrapNull: false })).not.toBe(sig);
});

it('normalizeWidgetPath yields undefined for anything that is not a usable path', () => {
expect(normalizeWidgetPath(' a.b ')).toBe('a.b');
expect(normalizeWidgetPath('')).toBeUndefined();
expect(normalizeWidgetPath(' ')).toBeUndefined();
expect(normalizeWidgetPath(null)).toBeUndefined();
expect(normalizeWidgetPath(42)).toBeUndefined();
});
});
40 changes: 34 additions & 6 deletions src/app/core/directives/widget-streams.directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,38 @@ import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
/** Fixed stale-data TTL (ms) applied to every widget whose enableTimeout is on; not user-configurable. */
const FIXED_DATA_TIMEOUT_MS = 5000;

/** The subset of a path config that decides which reading a subscription delivers. */
interface IPathIdentity {
path: string | null;
pathType?: string | null;
convertUnitTo?: string | null;
source?: string | null;
suppressBootstrapNull?: boolean;
}

/** Trim a configured path to its canonical form; undefined when it is not a usable path. */
export function normalizeWidgetPath(path: unknown): string | undefined {
const trimmed = typeof path === 'string' ? path.trim() : '';
return trimmed.length ? trimmed : undefined;
}

/**
* Identity of a configured path as the subscription diff computes it. Two configs that share a
* signature reuse one subscription; a different signature means the widget is now watching another
* reading. A widget that holds presentation state derived from the stream — a needle position, a
* last value, an alarm colour — needs this to tell a re-point apart from an unrelated reconfigure
* such as a theme change, because a rebuilt subscription may replay nothing at all and leave that
* state showing the previous path.
*
* Returns null for a config with no usable path, which has no identity to compare.
*/
export function widgetPathSignature(pathCfg: IPathIdentity | undefined | null): string | null {
const normalizedPath = normalizeWidgetPath(pathCfg?.path);
if (!pathCfg || !normalizedPath) return null;
const src = (pathCfg.source?.trim() || 'default');
return [normalizedPath, pathCfg.pathType, pathCfg.convertUnitTo, src, pathCfg.suppressBootstrapNull ? '1' : '0'].join('|');
}

@Directive({
selector: '[widget-streams]',
exportAs: 'widgetStreams'
Expand Down Expand Up @@ -63,9 +95,7 @@ export class WidgetStreamsDirective implements OnDestroy {
}

private computePathSignature(pathCfg: { path: string; pathType: string; convertUnitTo?: string; source?: string; suppressBootstrapNull?: boolean }): string {
const normalizedPath = this.normalizePath(pathCfg.path) ?? '';
const src = (pathCfg.source?.trim() || 'default');
return [normalizedPath, pathCfg.pathType, pathCfg.convertUnitTo, src, pathCfg.suppressBootstrapNull ? '1' : '0'].join('|');
return widgetPathSignature(pathCfg) ?? '';
}

private computeBaseKey(path: string, source?: string): string {
Expand All @@ -75,9 +105,7 @@ export class WidgetStreamsDirective implements OnDestroy {
}

private normalizePath(path: unknown): string | undefined {
if (typeof path !== 'string') return undefined;
const trimmed = path.trim();
return trimmed.length ? trimmed : undefined;
return normalizeWidgetPath(path);
}

private computeRootSignature(cfg: IWidgetSvcConfig | undefined): string {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { signal } from '@angular/core';
import { WritableSignal, signal } from '@angular/core';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { beforeEach, describe, expect, it } from 'vitest';
import { WidgetGaugeNgCompassComponent } from './widget-gauge-ng-compass.component';
Expand All @@ -7,6 +7,7 @@ import { WidgetStreamsDirective } from '../../core/directives/widget-streams.dir
import { UnitsService } from '../../core/services/units.service';
import { IPathUpdate } from '../../core/services/data.service';
import { IWidgetSvcConfig, IPathArray } from '../../core/interfaces/widgets-interface';
import { States } from '../../core/interfaces/signalk-interfaces';

/**
* A compass with no heading must still show its rose, with no needle and a '--' readout — a needle
Expand All @@ -20,21 +21,25 @@ describe('WidgetGaugeNgCompassComponent no-data state', () => {
let fixture: ComponentFixture<WidgetGaugeNgCompassComponent>;
let internals: CompassInternals;
let capturedNext: ((u: IPathUpdate) => void) | undefined;
let options: WritableSignal<IWidgetSvcConfig | undefined>;
let observeCount: number;
let replayOnObserve: IPathUpdate | undefined;

interface CompassInternals {
value: () => number | null | undefined;
textValue: () => string;
dataAvailable: () => boolean;
currentState: () => string;
optionsReady: () => boolean;
gaugeOptions: { needle?: boolean };
}

const makeConfig = (): IWidgetSvcConfig => {
const makeConfig = (path: string | null = 'self.navigation.headingTrue'): IWidgetSvcConfig => {
const dflt = WidgetGaugeNgCompassComponent.DEFAULT_CONFIG;
const gaugePath = (dflt.paths as IPathArray)['gaugePath'];
return {
...dflt,
paths: { gaugePath: { ...gaugePath, path: 'self.navigation.headingTrue' } }
paths: { gaugePath: { ...gaugePath, path } }
};
};

Expand All @@ -43,9 +48,17 @@ describe('WidgetGaugeNgCompassComponent no-data state', () => {

beforeEach(async () => {
capturedNext = undefined;
const options = signal<IWidgetSvcConfig | undefined>(makeConfig());
options = signal<IWidgetSvcConfig | undefined>(makeConfig());
observeCount = 0;
replayOnObserve = undefined;
const streamsFake = {
observe(_pathName: string, next: (u: IPathUpdate) => void) { capturedNext = next; }
observe(_pathName: string, next: (u: IPathUpdate) => void) {
capturedNext = next;
observeCount++;
// The real directive replays a BehaviorSubject, so a path holding a value delivers it
// synchronously inside the same effect run as the clear.
if (replayOnObserve) next(replayOnObserve);
}
};
const unitsFake = {
getUnitDisplaySymbol: (measure: string | null | undefined): string => measure ?? '',
Expand Down Expand Up @@ -96,4 +109,78 @@ describe('WidgetGaugeNgCompassComponent no-data state', () => {
expect(internals.dataAvailable()).toBe(false);
expect(internals.textValue()).toBe('--');
});

// #534: a rebuilt subscription against a silent path replays nothing (the leading null is
// suppressed), so the callback never runs and the previous heading stayed on the rose.
it('clears the heading when re-pointed at a path that reports nothing', () => {
capturedNext?.(update(142));
expect(internals.dataAvailable()).toBe(true);

options.set(makeConfig('self.navigation.headingMagnetic'));
fixture.detectChanges();

expect(internals.dataAvailable()).toBe(false);
expect(internals.value()).toBeUndefined();
expect(internals.textValue()).toBe('--');
});

// Clearing the path entirely drops the subscription, so the heading has to go with it. This is
// what pins the clear ahead of the no-path bail-out rather than after it.
it('clears the heading when the path is cleared entirely', () => {
capturedNext?.(update(142));
expect(internals.dataAvailable()).toBe(true);

options.set(makeConfig(null));
fixture.detectChanges();

expect(internals.dataAvailable()).toBe(false);
expect(internals.value()).toBeUndefined();
expect(internals.textValue()).toBe('--');
});

// currentState colours the heading text independently of dataAvailable, so without the reset an
// alarm-red readout from the old path survives onto the new one.
it('clears the zone state on a re-point', () => {
capturedNext?.({ ...update(142), state: States.Alarm } as IPathUpdate);
expect(internals.currentState()).toBe(States.Alarm);

options.set(makeConfig('self.navigation.headingMagnetic'));
fixture.detectChanges();

expect(internals.currentState()).toBe(States.Normal);
});

// The same effect re-runs on a theme change, so an unconditional clear would blink the needle
// off and back on at every switch.
it('shows the new path\'s heading immediately when it has one, without surfacing the clear', () => {
capturedNext?.(update(142));

replayOnObserve = update(271);
options.set(makeConfig('self.navigation.headingMagnetic'));
fixture.detectChanges();

expect(internals.dataAvailable()).toBe(true);
expect(internals.value()).toBe(271);
expect(internals.textValue()).toBe('271');
});

it('keeps the heading when the config changes without changing the path', () => {
capturedNext?.(update(142));
const before = observeCount;

fixture.componentRef.setInput('theme', {
contrast: 'rgba(0,0,0,1)', contrastDim: 'rgba(60,60,60,1)',
contrastDimmer: 'rgba(120,120,120,1)', cardColor: 'rgba(238,238,238,1)',
background: 'rgba(255,255,255,1)', zoneAlarm: 'rgba(255,0,0,1)',
zoneWarn: 'rgba(255,170,0,1)', zoneAlert: 'rgba(255,0,255,1)',
zoneEmergency: 'rgba(255,0,0,1)'
});
fixture.detectChanges();

// Positive control: the effect really did re-run, so "no clear" is a decision, not a no-op.
expect(observeCount).toBeGreaterThan(before);
expect(internals.dataAvailable()).toBe(true);
expect(internals.value()).toBe(142);
expect(internals.textValue()).toBe('142');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { States } from '../../core/interfaces/signalk-interfaces';
import { getColors } from '../../core/utils/themeColors.utils';
import { SkipResizeObserverDirective } from '../../core/directives/skip-resize-observer.directive';
import { WidgetRuntimeDirective } from '../../core/directives/widget-runtime.directive';
import { WidgetStreamsDirective } from '../../core/directives/widget-streams.directive';
import { WidgetStreamsDirective, widgetPathSignature, normalizeWidgetPath } from '../../core/directives/widget-streams.directive';
import { ITheme } from '../../core/services/app-service';
import { UnitsService } from '../../core/services/units.service';

Expand Down Expand Up @@ -124,6 +124,38 @@ export class WidgetGaugeNgCompassComponent implements AfterViewInit {
protected colorStrokeTicks = '';
private currentState = signal<States>(States.Normal);
private lastAppliedState: States | null = null;
/**
* Identity of the path the reading state describes. Three states: `undefined` before the first
* effect run (nothing has been shown, so there is nothing to clear), `null` for a config with no
* usable path, and the signature otherwise. `null` is a real identity rather than a second
* "not yet" — a cleared path must still compare unequal to the path that follows it.
*/
private lastPathSignature: string | null | undefined = undefined;

/**
* Drop the reading when the widget is re-pointed at another path.
*
* The subscription is rebuilt on every run of the data effect, a theme change included, and
* `suppressBootstrapNull` gives each rebuild a fresh suppression closure. Against a path that
* reports nothing the replayed leading null is therefore filtered and the stream callback never
* runs — leaving the previous path's needle and value on screen, presented as a live reading of
* the new one. Clearing unconditionally here is wrong for the same reason: this effect re-runs on
* theme changes, which would blink the needle off and back on at every switch.
*
* The reading comes back because `observe()` below is passed a new closure on every effect run:
* the directive compares callback identity as well as the signature, so it rebuilds and replays
* the new path's value into this component. A stable callback reference would make that
* early-return instead, and the gauge would stay blank on a live path until the next delta.
*/
private clearReadingOnRepoint(signature: string | null): void {
if (this.lastPathSignature !== undefined && this.lastPathSignature !== signature) {
this.dataAvailable.set(false);
this.value.set(undefined);
this.textValue.set('--');
this.currentState.set(States.Normal);
}
this.lastPathSignature = signature;
}

private readonly negToPortPaths = [
"self.environment.wind.angleApparent",
Expand All @@ -146,22 +178,32 @@ export class WidgetGaugeNgCompassComponent implements AfterViewInit {
const theme = this.theme();
if (!cfg || !theme) return;
const pCfg = cfg.paths?.['gaugePath'];
if (!pCfg?.path) return;
untracked(() => this.streams.observe('gaugePath', pkt => {
// Computed before the no-path bail-out, and null exactly when there is no usable path: the
// streams directive drops the subscription in that case, so the reading has to go with it.
const signature = widgetPathSignature(pCfg);
untracked(() => {
this.clearReadingOnRepoint(signature);
// Normalized, like the signature and the subscription: the widget-options required check
// accepts a padded path, which would subscribe fine and then miss this list, clamping a
// negative apparent wind angle to 0 instead of converting it to its 0-360 bearing.
const path = normalizeWidgetPath(pCfg?.path);
if (!signature || !path) return;
this.streams.observe('gaugePath', pkt => {
let raw = (pkt?.data?.value as number) ?? null;
this.dataAvailable.set(raw != null);
if (raw == null) {
this.value.set(0);
this.textValue.set('--');
} else {
if (this.negToPortPaths.includes(pCfg.path)) raw = convertNegToPortDegree(raw);
if (this.negToPortPaths.includes(path)) raw = convertNegToPortDegree(raw);
const clamped = Math.min(Math.max(raw, 0), 360);
this.value.set(clamped);
this.textValue.set(clamped.toFixed(0));
}
const newState = (pkt?.state ?? States.Normal) as States;
if (newState !== this.currentState()) this.currentState.set(newState);
}));
});
});
});

// Build options when config/theme changes
Expand Down
Loading
Loading