From 06069168db5316a533570f4a15ac7c29bc995a49 Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Thu, 13 Aug 2026 11:42:52 +0300 Subject: [PATCH 1/3] chore: configure CodeRabbit for this repo's conventions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Baseline taken from the signalk-server config, then adapted to Skip's CLAUDE.md and to the traps a generic reviewer gets wrong here — chiefly that specs run against the real app services on purpose, so the usual "add a stub" advice is inert, and that a per-file mock of an aliased library is the defect #544 was. profile is `chill` rather than `assertive` because main has required_conversation_resolution enabled: every comment is a merge gate until someone resolves it, so a style nit costs more than it finds. --- .coderabbit.yaml | 189 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 .coderabbit.yaml diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 00000000..72a3f42f --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,189 @@ +# yaml-language-server: $schema=https://storage.googleapis.com/coderabbit_public_assets/schema.v2.json +language: en-US + +# Under 250 characters — the schema's limit. +tone_instructions: >- + Name the trigger, the code path, and the user-visible consequence for every + finding. If you cannot name one, do not raise it. No praise, no preamble, no + restating the diff. Silence beats a nitpick. + +reviews: + # `chill`, not `assertive`: main has required_conversation_resolution enabled, + # so every comment CodeRabbit leaves is a merge gate until somebody resolves + # it. A style nit that blocks a merge costs more than it finds. Raise this to + # `assertive` if that protection is ever relaxed. + profile: chill + poem: false + in_progress_fortune: false + + auto_review: + enabled: true + drafts: false + + high_level_summary_instructions: | + Write in present tense: "adds", not "added". + Lead with what changes for a user of the app, not with the file list. + Do not restate the PR description back to the author. + + pre_merge_checks: + title: + mode: warning + requirements: | + A reader who sees only the title should understand what the PR does. + Conventional-commit form: (): , imperative mood. + Do not require a specific length; this repo's history runs 50-75 chars + and the cap is not enforced. + # Quoted: bare `off` is a YAML 1.1 boolean, and this field takes a string. + description: + mode: "off" + + # CI runs `npm run ci` (lint + snc + tests + mcp-schema) and takes minutes; + # the 90s default expires before the result exists. + tools: + github-checks: + timeout_ms: 600000 + # `npm run lint` already gates every PR — a second opinion is duplicate noise. + eslint: + enabled: false + # Docs here are deliberate prose, not linted text. + markdownlint: + enabled: false + languagetool: + enabled: false + + path_filters: + - '!public/**' # Angular build output + - '!package-lock.json' + - '!src/assets/steelseries-min.js' # vendored, minified + - '!src/assets/tween-min.js' # vendored, minified + - '!src/assets/skip-dashboard-schema.json' # generated by `npm run gen:mcp-schema` + - '!perf-harness/results/**' # committed measurement baselines + - '!src/assets/**/*.{png,jpg,jpeg,svg,mp3,ico,webmanifest}' + + path_instructions: + - path: '**/*' + instructions: | + ## Evidence, not impressions + Every finding states the trigger, the code path it runs through, and what + the user sees. A finding you cannot express that way is not one. Say + plainly when you are unsure rather than hedging an assertion. + + ## Comments + A comment explains why the code is the way it is, never what changed or + why it changed. Write for a reader who never saw the diff: no narrating + the change, no "fixes bug X", no "we used to do Y", no restating what the + code already says. Flag comments that violate this. Prefer no comment to + one that earns no keep. + + ## Do not ask for these + - **A CHANGELOG entry.** `CHANGELOG.md` was deliberately removed (#586). + Release notes come from `git log` via + `.github/scripts/generate-release-notes.sh` and are edited by hand on + the draft release. Do not suggest reinstating the file. + - **A VERSION bump.** The rule is subtle (a bump opens a release cycle; + later PRs in the same cycle do not re-bump unless they are a higher + semver level) and `version-bump-check` already enforces it in CI. + - **Syncing with mxtommy/kip.** Skip originated as a fork but is an + independent project. KIP is not an upstream to track or mirror. + - **A bundled Signal K server plugin.** The package is webapp-only by + design; the Freeboard-SK integration lives in the separate + `@halos-org/skip-freeboard-panel` package. + + ## Scope + One logical change per PR. Flag substantial unrelated work and suggest a + separate PR; a called-out trivial fly-by fix is fine. Do not ask for + refactoring of code the PR merely touches. + + - path: '**/*.spec.ts' + instructions: | + ## Specs run against the REAL app services, by design + Read the "Testing reality" section of CLAUDE.md before commenting on test + setup. Under this runner, `src/test.ts`'s classes are different module + instances than the app bundle's, so **any** `{ provide: AppClass, ... }` + placed there is DI-inert — a stub and the real class behave identically. + Never suggest adding a shared or global app-service stub. A spec that + needs a fake declares it locally; a local `SignalKConnectionService` fake + must still expose `serverServiceEndpoint$` and `serverVersion$`, because + services subscribe to them at construction. + + Never suggest a per-file `vi.mock()` for a library that `vitest.config.ts` + aliases to a shim (chart.js and its plugins, gridstack, canvas-gauges). + That is the defect #544 was: a per-file mock wins only when its spec loads + the module first, so the real library gets cached instead and silently + drops everything fed to it. + + ## Judge whether a test can fail + Value a test that fails when the production change is reverted. Flag + assertions on source text, on a stub's logged intent rather than recorded + state, and on internals that an equivalent implementation would change. + + ## Tolerances are physical, not library-exact + Unit-conversion assertions use a tolerance that separates neighbouring + units, not one that pins the conversion library's exact double. Do not + ask for tighter `toBeCloseTo` precision; that makes the suite fail on a + library rounding change no user could observe. + + ## Running them + `ng test --include=''` runs one spec. Plain `npx vitest run ` + does not work here — it bypasses the Angular builder. + + - path: 'src/app/widgets/**' + instructions: | + Widgets are standalone components composed with three host directives: + `WidgetRuntimeDirective` (config merge), `WidgetStreamsDirective` + (diff-based path subscriptions), `WidgetMetadataDirective` (zones/meta). + + A widget holding stream-derived presentation state owns clearing it. The + streams directive 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. + + Angular 21, zoneless, signals. Effects read signals and do their writes + inside `untracked()`; check that placement rather than assuming. + + - path: 'src/app/core/services/**' + instructions: | + The DI graph is deliberately acyclic — `connection <- auth <- storage <- + settings`, `data <- delta <- connection`. Flag a new edge that closes a + cycle, and check CLAUDE.md's architecture section still matches. + + `DataService.updatePathData` runs per delta value. Skip targets Pi-class + hardware; treat that path as allocation-sensitive, and say so when a + change adds work to it. `perf-harness/` measures it — ask for numbers + rather than asserting a regression. + + All applicationData writes go through a sequential JSON-Patch queue + because the server cannot handle concurrent writes. A patch's slot name + is baked in at enqueue time. + + `units.service.ts` diverges from the Signal K server's imperial fuel-flow + formulas by a factor of 3600 on purpose — the server converts to gallons + per second rather than per hour (upstream signalk-server#2951). Do not + suggest reconciling Skip's values with the server's. + + - path: '{package.json,angular.json,src/manifest.json}' + instructions: | + The package name is the serving path. `angular.json` `baseHref`, the + `dev` serve-path, and `src/manifest.json` `id`/`scope`/`start_url` must + all stay `/@halos-org/skip/`. It is also a cross-package contract: the + companion `@halos-org/skip-freeboard-panel` hardcodes it along with + Skip's widget-type ids and `#/widget/:type` embed routes. Flag any change + to it that does not also account for that package. + + - path: '**/*.md' + instructions: | + Documentation describes the current state, not development history. Flag + implementation-status language ("Step 3", "TODO", "currently implemented + as"), build narratives, and unchecked checklist items. + + Exception: files under `docs/` ending in `-plan.md` are historical + planning records. Do not flag them for describing what was done at the + time, and do not ask for them to be updated or deleted. + +knowledge_base: + code_guidelines: + enabled: true + filePatterns: + - CLAUDE.md From e32202a9889ba4748dbb149b7c5da5f455e3e619 Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Thu, 13 Aug 2026 11:53:52 +0300 Subject: [PATCH 2/3] chore: remove the docs/ tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four files were planning records for work that shipped long ago. Git history holds them. The fourth, widget-schematic.md, read as live documentation — README pointed at it three times for the scaffolding CLI's flags — but 8 of the 14 options it documents do not exist in the schematic's schema (description, icon, category, pathKey, pathDescription, convertUnitTo, sampleTime, interactive), and 4 that do exist go unmentioned. It came from the KIP era, in #835, and drifted from the first change afterwards. A reader following it would pass flags that do nothing. README now points at the schematic's own schema.json, which cannot drift from the CLI because it is the CLI's definition. --- .coderabbit.yaml | 9 +- README.md | 6 +- docs/named-configs-plan.md | 619 ---------------------------------- docs/power-system-plan.md | 173 ---------- docs/signalk-auth-plan.md | 673 ------------------------------------- docs/widget-schematic.md | 119 ------- 6 files changed, 9 insertions(+), 1590 deletions(-) delete mode 100644 docs/named-configs-plan.md delete mode 100644 docs/power-system-plan.md delete mode 100644 docs/signalk-auth-plan.md delete mode 100644 docs/widget-schematic.md diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 72a3f42f..72dc4cde 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -178,9 +178,12 @@ reviews: implementation-status language ("Step 3", "TODO", "currently implemented as"), build narratives, and unchecked checklist items. - Exception: files under `docs/` ending in `-plan.md` are historical - planning records. Do not flag them for describing what was done at the - time, and do not ask for them to be updated or deleted. + There is no `docs/` tree: planning records live in git history, and + anything documenting current behaviour belongs in `README.md` (for users) + or `CLAUDE.md` (for contributors). Do not suggest adding one — a separate + document describing how the code works is how it drifts out of sync. + `src/assets/help-docs/**` is the in-app help content and is user-facing + prose, not developer documentation. knowledge_base: code_guidelines: diff --git a/README.md b/README.md index fe21d614..0617f6b5 100644 --- a/README.md +++ b/README.md @@ -204,7 +204,7 @@ Skip is one part of a Signal K stack, and it's easy to extend in two directions: **Signal K Plugins** — domain-specific enrichment (polars, performance calculations, derived environmental data, routing aids) published into the Signal K data model, which Skip can then display. -**Skip Widgets** — visual components that read Signal K path data and API v2 features. Scaffolding a new one takes only a few moments: run `npm run generate:widget`, or ask your AI to build one from the Skip project instructions. See `CLAUDE.md` and `docs/widget-schematic.md` for details. +**Skip Widgets** — visual components that read Signal K path data and API v2 features. Scaffolding a new one takes only a few moments: run `npm run generate:widget`, or ask your AI to build one from the Skip project instructions. See `CLAUDE.md` for details. ## Getting Started @@ -249,11 +249,11 @@ For comprehensive development guidance, start with `CLAUDE.md`: ### Widget Creation Workflow 1. Scaffold with `npm run generate:widget` (Host2 schematic-first path). -2. Use `docs/widget-schematic.md` for CLI flags, prompting behavior, and troubleshooting. +2. See `tools/schematics/create-host2-widget/schema.json` for the available options and their prompts. 3. Follow the Host2 runtime/stream patterns documented in `CLAUDE.md`. ### Key Priorities -- **Widget Development**: Use Host2 patterns and scaffold with the `create-host2-widget` schematic (see `docs/widget-schematic.md`). +- **Widget Development**: Use Host2 patterns and scaffold with the `create-host2-widget` schematic. - **Angular Patterns**: Use signals, standalone components, and modern control flow. - **Theming**: Follow Skip's theme system for consistent UI. - **Code Quality**: Run `npm run lint` before commits. diff --git a/docs/named-configs-plan.md b/docs/named-configs-plan.md deleted file mode 100644 index 2fcb7add..00000000 --- a/docs/named-configs-plan.md +++ /dev/null @@ -1,619 +0,0 @@ ---- -title: "feat: Profile support (named configs) for Skip" -type: feat -status: active -date: 2026-06-23 -deepened: 2026-06-23 ---- - -# Profile support (named configs) for Skip - -> Design + implementation plan. Lives on the `named_configs` branch. Conventions aim at -> upstreamability (`mxtommy/kip`); not a hard constraint. -> -> Revised twice (2026-06-23) after two multi-persona document-review passes. Pass 1 caught two -> correctness P0s (degraded-boot slot corruption; undefined import target) and a per-profile -> remote-control side effect. Pass 2 verified the fixes against source and corrected the -> remote-control migration (ordering + version baseline), the write-guard scope, the recovery -> path, rename sequencing, and import safety. All findings are folded in below. - -## Overview - -Let a user keep several independent **profiles** — each owning its own dashboards, layouts, and -theme — and switch between them at runtime, **without creating a separate Signal K user per -screen set** (today's workaround). A profile maps onto Skip's existing *named config slot* in the -Signal K `applicationData` store. The active profile is remembered **per device**, so a cabin, -mast, and cockpit display can each show a different profile from the same single Signal K login. - -The storage substrate is mostly present: `getConfig` / `setConfig` / `listConfigs` / `removeItem` -are already parameterized by config name, and the active slot name (`sharedConfigName`) already -drives both config load and every incremental save. The core work is (1) making that slot name -runtime-mutable instead of boot-frozen, (2) a management UI, and (3) hardening the failure paths -the current single-slot code never had to handle (a missing/renamed slot; a write before the slot -name is known). No `IConfig` schema change is required. - -## Glossary (device vs profile) - -- **Device** — one browser/screen running Skip. Its `connectionConfig` lives in that browser's - localStorage and is never shared: server URL, credentials, `useSharedConfig`, `sharedConfigName` - (*which profile this screen shows*), `skipUUID` (per-device id that identifies this screen to - remote controllers). Two tablets = two devices, even on one Signal K login. -- **Profile** — a named bundle of `{app, theme, dashboards}` stored server-side under the Signal K - user, shared across that user's devices. A device points at one profile via its own - `sharedConfigName`. - -## Problem Frame - -Skip stores one active configuration. When logged in (`useSharedConfig = true`) it persists to -`…/applicationData/user/kip/{fileVersion}/{configName}`. Per-user isolation is a property of -Signal K's `user` scope keyed by the bearer JWT — not of Skip. The config name is pinned to -`default`, so the only lever for "a different set of screens" is a different Signal K login. Skip's -README frames multi-config mostly around different *people*; this feature extends it to one person -(or boat) wanting different screen sets per display. The "separate logins" workaround is the -assumed motivation; it is a hypothesis, not a measured pain point (see Open Questions). - -The named-slot dimension exists but is frozen: `sharedConfigName` is read once at boot and never -changed at runtime. The existing "Configurations" tab can list named slots, but its "Restore" -copies a chosen slot back into the fixed `user/default` and hard-reloads — a backup/restore flow, -not a profile selector. - -## Requirements Trace - -- **R1.** A user can see all their profiles and which one is active (logged in, remote storage). -- **R2.** A user can switch the active profile; remembered for *this device only*. -- **R3.** A user can create a profile, seeded from the current profile (clone) or blank. -- **R4.** A user can rename, duplicate, and delete profiles, with guard rails that prevent an - unbootable or data-losing state. -- **R5.** Each profile owns its own dashboards, layouts, theme, **and unit defaults** (a profile is - a whole `IConfig` slot; see Scope Boundaries for the no-enforced-shared-units tradeoff). -- **R6.** Existing single-config users migrate transparently: their `user/default` *is* profile #1; - nothing in `IConfig` is rewritten. -- **R7.** A profile can be exported to / imported from a JSON file **without destroying another - profile** (import creates a new profile). -- **R8.** A display's remote-control identity (`isRemoteControl`, `instanceName`) stays stable - across profile switches — it is a property of the device, not the profile. - -## Scope Boundaries (non-goals for v1) - -- **No local-only (logged-out) profiles.** `useSharedConfig = false` stays single-profile. - *Tradeoff (accepted, documented):* installs running without Signal K security are effectively - logged-out and won't see profiles; this excludes a real slice of out-of-box installs. The - device-token → `global` scope path is a possible later route, out of scope now. -- **No hot-swap.** Switching uses a full page reload (reuses the proven reload path). -- **No enforced shared units across profiles.** A profile is a whole `IConfig` slot, so - `unitDefaults` is per-profile. Clone-on-create carries units forward, but a later edit to one - profile's units does **not** propagate — displays can silently diverge. Mitigation: an edit-time - notice (Unit 4) and help wording (Unit 7); enforced shared units is deferred. -- **No profile metadata registry** (icons, ordering, descriptions). Profiles are identified by slot - name only. See Future Considerations for the trajectory risk. -- **No quick-switch in the main app chrome.** Management/switching live in Options only. -- **No `IConfig`/`IAppConfig.configVersion` bump for dashboards/theme.** Unit 5 *does* remove two - fields from `IAppConfig` (`isRemoteControl`, `instanceName`) and bumps **`IConnectionConfig`'s** - version (12 → 13). The `IAppConfig` removal is forward-compatible (old slots carry ignored - fields); consumers update in lockstep (Unit 5). - -## Context & Research - -### Relevant code and patterns - -- `src/app/core/interfaces/app-settings.interfaces.ts` — `IConfig {app, theme, dashboards}`; - `IAppConfig` (currently holds `isRemoteControl`, `instanceName` — Unit 5 moves these, and has its - own `configVersion`); `IConnectionConfig` (`useSharedConfig`, `sharedConfigName`, `skipUUID`, its - own `configVersion`, currently 12). -- `src/app/core/services/storage.service.ts` — named-slot CRUD: `listConfigs` (`?keys=true`), - `getConfig`, `setConfig` ("if name exists, replaced; else created"; returns awaitable - `Promise`), `removeItem` (delete; returns `void`), `patchConfig` (paths all - `/{sharedConfigName}/…`). **`StorageService.sharedConfigName` is assigned only inside - `bootstrapRemoteContext`** — the degraded-boot hazard. `removeItem`/`patchConfig` post to a - fire-and-forget sequential `patchQueue$` with **no exposed completion**; `patchConfig` uses - JSON-Patch `replace` (needs the slot to pre-exist). The storage URL is raw string concat with - **no `encodeURIComponent`**. -- `src/app/core/services/settings.service.ts` — owns the private `sharedConfigName` (loaded from - `connectionConfig`, **defaults to `'default'`, never `undefined`**); persists via - `buildConnectionStorageObject`; `latestConfigVersion = 12`; `loadConnectionConfig` accepts only - versions {11,12} then `resetConnection()` (which wipes per-device state). `reloadApp()` is - `location.replace("./")`, a **no-op under `__SKIP_TEST__`**. `startup()` early-returns before - `pushSettings()` when not bootstrapped (in-memory getters empty on the degraded path). - `resetSettings()` writes a blank default into the *current* `sharedConfigName` (gated on - `storageServiceReady$`); `loadDemoConfig()` calls `setConfig` **without** a readiness gate. - `pushSettings`/`buildAppStorageObject` read `app.isRemoteControl`/`app.instanceName`. -- `src/app/core/services/app-initNetwork.service.ts` — `loadLocalStorageConfig()` runs first - (upgrades connectionConfig 9→10→11→12 **in place, before** the profile is fetched); `getConfig` - + `bootstrapRemoteContext` come later, both inside the `try`; a 404 is caught and - `bootstrapRemoteContext` is skipped → storage slot name stays unset. Emits - `bootstrapIssue 'missing-shared-config'`; distinguishes status 0 / 401 / 404. -- `src/app/app.component.ts` — `missing-shared-config` degraded UX; its recovery action calls - `settings.resetSettings()` (recreates the *current* name blank, not a fallback to `default`). -- `src/app/core/components/options/configuration/config.component.ts` + `.html` — the - "Configurations" tab. Hides `user/default` and **blocks overwriting it** (in `saveConfig`, not - just the list filter); chooses scope by token type (device → `global`, user → `user`); the - `if (this.storageSvc.setConfig(...))` check is a bug (treats a `Promise` as a sync boolean → - always reports success). Import does `JSON.parse` → write to `user/default` with no validation. - `config.component.spec.ts` exists. -- `src/app/core/components/dashboards-editor/dashboards-editor.component.ts` (+ bottom sheet) — CRUD - UI pattern to mirror. `src/app/core/services/dialog.service.ts` — `openNameDialog` (name only) + - `openConfirmationDialog`. -- `src/app/core/services/remote-dashboards.service.ts` — drives remote participation off - `isRemoteControl` + display name; `displayId` is `skipUUID` (per-device), so two displays never - collide — but `isRemoteControl`/`instanceName` are per-profile today (the R8 / Unit 5 driver). - The setter UI for these lives in `display.component`. - -### Conventions to honor - -New app-internal types → `src/app/core/interfaces`. Service-centric; signal-based state. Tests: -**Vitest** (`@angular/build:unit-test` + `vitest.config.ts`, jsdom, `TestBed` + `describe/it/expect`, -co-located `*.spec.ts`). Theme via Skip theme roles / CSS variables. - -## Key Technical Decisions - -- **Profile == named `user`-scope config slot, holding a full `IConfig`.** Per-profile theme falls - out for free. -- **All `ProfileService` storage ops hardcode `scope: 'user'`** — do not inherit the component's - token-type scope selection. Device-token sessions cannot use the user scope, so profiles are - unavailable for them (consistent with the "requires login" gating); never silently `global`. -- **Active profile is per-device** via the always-local `connectionConfig.sharedConfigName`, made - runtime-mutable. *Tradeoff (accepted):* a per-device choice doesn't follow the user to a new - browser/device and is reset by a localStorage clear. Documented in help. -- **Switch = drain pending writes (bounded), persist the new name, then `reloadApp()`.** After - reload the bootstrap loads the chosen slot and re-syncs the slot name — reliable **only on the - authenticated, storage-ready path**. The degraded path is covered by the write-safety guard - (Unit 2) and recovery (Unit 6), not by assuming re-sync. -- **Create = `setConfig('user', name, seed)` first (awaited; creates the slot), then optionally - switch.** Order matters: `patchConfig` `replace` and the bootstrap both assume the slot exists. - `seed` = a guarded clone (only when settings are loaded) or a blank default. -- **Write-safety is a `StorageService`-boundary invariant (Unit 2).** Every mutating call validates - its effective slot name and refuses to write to an empty/undefined target. -- **Profile-name validation is a security invariant (Unit 3), confirmed by the Unit 1 probe.** - Names are URL path segments *and* JSON-Patch keys (`/{name}/…`) *and* must avoid the `::` - list-key separator. Use an **allow-list** `[A-Za-z0-9 _-]`, bound length, reject empty / - `default`. The probe proved why allow-list (not blocklist) is mandatory: a `.` **truncates** the - stored name (the server splits on dot: `foo.bar` → `foo`) and a `/` is treated as a path - separator **even when sent as `%2F`** (the server decodes it) — so `encodeURIComponent` does - *not* protect against `.` or `/`; they must be rejected outright. Apply `encodeURIComponent` - inside `StorageService` anyway (covers spaces/unicode for all callers); imported-profile names - run through the identical validation. -- **`default` is the reserved fallback profile name.** `useSharedConfig` defaults to `false`, so a - fresh device boots into local mode and the `user/default` *slot* is not created until the first - shared write — not guaranteed to exist or appear in `listConfigs` for a brand-new shared user. - v1 treats `default` as reserved: cannot create another named `default`, cannot delete/rename it, - and recovery/fallback must **create** it (blank `IConfig`) when genuinely absent. -- **Remote-control identity is per-device (R8).** `isRemoteControl` + `instanceName` move from - `IAppConfig` to `IConnectionConfig`, beside `skipUUID`, so switching profiles never changes a - screen's remote role or advertised name (Unit 5). -- **Logic lives in a new `ProfileService`** (mirrors `DashboardService`), orchestrating - `StorageService` + `SettingsService`. Could fold into `SettingsService`; kept separate for - cohesion. -- **User-facing term: "Profiles."** Internally/storage they remain named configs. - -## Open Questions - -### Resolved during planning / review - -- Profile = `user`-scope named `IConfig` slot; active per-device; switch = persist + reload; no - `IConfig` migration; local mode out of scope; logic in `ProfileService`; `default` reserved. -- Multi-display per-login does **not** collide on remote-control id (`displayId` = per-device - `skipUUID`); but `isRemoteControl`/`instanceName` were per-profile → R8 hoist (Unit 5). -- Name validation pinned as a Unit 3 invariant. -- Import creates a new profile (R7); never silently overwrites. -- **Unit 1 pre-flight — DONE** (2026-06-23, halosdev.local, Signal K 2.27.0, user scope): - - Arbitrary user-scope named slots: create / read / list (`?keys=true`) / delete all return 200. - - **Read-after-write is immediate** (GET right after POST returns the value) → create-then-switch - is safe; no eventual-consistency gap observed. - - Charset: `.` truncates a name; `/` splits the path even when sent `%2F` (server decodes); - spaces/unicode/160-char round-trip only with consistent `encodeURIComponent`. → allow-list - `[A-Za-z0-9 _-]`, reject `.` and `/` outright (Unit 3). - - JSON-Patch `replace` on a missing key → **HTTP 500** ("does not exist"); `add` auto-creates; - a full-object POST to a named path creates → **create-first is mandatory** (matches the plan). - - GET a missing slot → clean **404**. (True transient-404 needs a server restart; not tested — - Unit 6 still re-verifies before clobbering.) - - Device tokens write **user** scope (bucket keyed by the device username) but **not global** - (global needs admin) → reinforces "profiles are user-scope; device tokens → profiles - unavailable." - -### Deferred to implementation - -- The queue-drain (Unit 2) and the awaitable delete (Unit 3) share one missing capability — - `patchQueue$` exposes no completion. Build **one `StorageService` completion primitive** with a - **bounded timeout**; both consume it. Plus the double-switch re-entrancy guard. -- Exact `ProfileService`/`SettingsService` method names and signatures. -- Exact recovery affordance wording reused/extended from `app.component`. - -## High-Level Technical Design - -> *Directional guidance for review, not implementation specification.* - -**Model mapping** - -``` -Signal K applicationData (per Signal K user, JWT-gated) - user/kip/{fileVersion}/ - ├── default ← reserved fallback profile (created on first shared write) ┐ - ├── cabin ← profile ├ each = full IConfig - └── cockpit ← profile ┘ {app, theme, dashboards} - -Per device (localStorage connectionConfig): - sharedConfigName → which profile this screen shows - skipUUID → this screen's remote-control identity - isRemoteControl → this screen participates in remote control (moved here in Unit 5) - instanceName → this screen's advertised name (moved here in Unit 5) -``` - -**Switch flow (reload-based, write-safe)** - -```mermaid -sequenceDiagram - participant UI as Profiles UI - participant PS as ProfileService - participant SET as SettingsService - participant ST as StorageService - participant LS as localStorage(connectionConfig) - participant BOOT as app-initNetwork (next load) - - UI->>PS: switchProfile("cockpit") - PS->>ST: await completion-primitive (bounded timeout) for pending writes - PS->>SET: setActiveProfile("cockpit") - SET->>LS: persist sharedConfigName="cockpit" - SET->>SET: reloadApp() (location.replace; no-op under __SKIP_TEST__) - Note over BOOT,ST: on reload - BOOT->>ST: getConfig('user',"cockpit") - alt slot exists - BOOT->>ST: bootstrapRemoteContext(sharedConfigName="cockpit") - Note over ST: patchConfig writes target /cockpit/... ; write-safe - else 404 (deleted elsewhere) - Note over BOOT: missing-shared-config → recovery (Unit 6), NOT a /undefined write - end -``` - -## Implementation Units - -- [x] **Unit 1: Pre-flight — verify named-slot behavior on a live Signal K server** *(DONE 2026-06-23 — results in Open Questions → Resolved)* - -**Goal:** Confirm the external assumptions before building. Lightweight probe, not a build unit, but -a hard gate. - -**Requirements:** R1–R4 (foundational). **Dependencies:** None. **Files:** none persistent. - -**Approach:** Against a real server (Signal K demo or `halosdev.local`): create/list/get/delete -arbitrary user-scope names; confirm `listConfigs` returns them; **confirm read-after-write -consistency** (create then immediately get); probe name charset (`/`, `..`, `~`, spaces, unicode, -long); confirm `patchConfig`-style `replace` fails on a missing slot; confirm whether the server -auto-creates on first `patch` to a missing name or needs explicit `setConfig`; **probe whether the -server ever returns a transient 404 for an existing name** (feeds Unit 6). - -**Test expectation:** none (probe). Output: go/no-go + charset rule (Unit 3) + auto-create + transient-404 answers (Unit 6). - -**Verification:** Documented confirmation of arbitrary-name round-trip with read-after-write -consistency, plus charset / auto-create / transient-404 answers. - ---- - -- [ ] **Unit 2: `SettingsService` + `StorageService` — runtime-mutable active profile, write-safety** - -**Goal:** Make the slot name runtime-settable, expose a guarded config snapshot, and ensure no write -can target an unset slot. - -**Requirements:** R2, R3, R6. **Dependencies:** Unit 1. - -**Files:** -- Modify: `src/app/core/services/settings.service.ts`, `src/app/core/services/storage.service.ts` -- Test: `src/app/core/services/settings.service.spec.ts`, `src/app/core/services/storage.service.spec.ts` - -**Approach:** -- Active-profile read accessor (current `sharedConfigName`) and a setter that sets the private - name, persists `connectionConfig` to localStorage, and triggers `reloadApp()`. -- **Write-safety guard (P0) at the `StorageService` boundary, not one method:** every mutating call - validates its effective slot name (the explicit arg for `setConfig`/`removeItem`/`patchGlobal`; - `this.sharedConfigName` for `patchConfig`) and refuses/throws on empty/undefined rather than - POSTing to `/undefined/…`. (The genuine `/undefined` risk is `patchConfig`, which reads the - bootstrap-set `StorageService.sharedConfigName`; the `setConfig` callers pass explicit names — but - the guard is uniform.) Also **gate `loadDemoConfig` on `storageServiceReady$`** (today it isn't); - gate ProfileService/UI mutations on `isRemoteContextBootstrapped()` / `storageServiceReady$`, not - merely `hasToken`. -- **Completion primitive:** add a `StorageService` way to await `patchQueue$` settling (none exists - today) with a **bounded timeout**, so a switch awaits pending writes to the leaving profile but a - stuck write degrades to best-effort rather than hanging the UI. Same primitive backs the awaitable - delete (Unit 3). -- **Config snapshot** for clone, assembled from `getAppConfig`/`getDashboardConfig`/`getThemeConfig`. - **Guard:** these are populated only by `pushSettings()`, skipped on the degraded path — only offer - "clone current" when settings are actually loaded; else disable clone / fall back to blank. - -**Patterns to follow:** existing setter→persist pattern; `reloadApp()`'s `__SKIP_TEST__` guard. - -**Test scenarios:** -- Happy: setting the active profile persists the new name to `connectionConfig` and invokes reload - (no-op under test; assert the persisted value); getter returns the boot name; snapshot matches - current settings when loaded. -- Edge: snapshot/clone while not bootstrapped → clone unavailable / blank fallback. -- Error (P0): each mutating call (`patchConfig`, `setConfig`, `removeItem`) with an unset slot name - does NOT issue a request to `/undefined/…`. -- Edge: switch with a pending queued write awaits the completion primitive (bounded) — write not - silently dropped; a stuck write times out to best-effort. - -**Verification:** Active profile runtime-settable and durable; no write path targets an unset slot; -clone never seeds from an empty snapshot; switch drains within a bounded time. - ---- - -- [ ] **Unit 3: `ProfileService` — profile CRUD + switch orchestration** - -**Goal:** One service owning list / switch / create / rename / duplicate / delete with guard rails, -name validation, and clone seeding. - -**Requirements:** R1, R2, R3, R4, R5, R6. **Dependencies:** Unit 2. - -**Files:** -- Create: `src/app/core/services/profile.service.ts`, `src/app/core/services/profile.service.spec.ts` -- Modify (only if a summary type helps): `src/app/core/interfaces/app-settings.interfaces.ts`. - -**Approach:** -- **List:** `listConfigs()` filtered to `user`; **include `default`** (creatable entry if absent); - mark the active one; expose as a signal. -- **Switch:** delegate to `SettingsService.setActiveProfile` (drain → persist → reload). -- **Create:** validate name (charset invariant; reject empty / `default` / existing / invalid); - seed = guarded clone or blank; `await setConfig('user', name, seed)`; refresh. Offer switch only - after `setConfig` resolves. -- **Duplicate:** `getConfig` source → `setConfig` new name. -- **Rename:** create new slot (awaited). For the **active** profile, strictly sequence - **delete old (awaited via the completion primitive, bounded timeout) → persist new name → - `reloadApp()`** — never fire the delete concurrently with `location.replace`. On delete - timeout/failure, proceed with the switch and record the orphan for cleanup (a hung switch is worse - than a leaked slot). Non-active rename: create new → delete old (awaited), no reload. -- **Delete:** guard — refuse active, `default`, or the last remaining profile; else `removeItem`. -- **Do not replicate** `config.component`'s `if (this.storageSvc.setConfig(...))` pattern - (`setConfig` returns an always-truthy `Promise`, so its success branch always fires). `await` in - `try/catch`; gate success toast / refresh / switch on resolution; surface `HttpErrorResponse`; - never change the active name on a failed mutation. -- **Hardcode `scope: 'user'`** for all operations. - -**Patterns to follow:** `DashboardService` CRUD; `dialog.service` naming/confirmation. - -**Test scenarios:** -- Happy: list includes `default`, active flagged; create (blank/clone), duplicate, switch delegate - correctly. -- Edge/Error: create with empty / `default` / existing / invalid-charset → rejected, no `setConfig`, - error surfaced. -- Edge/Error: delete active / `default` / last → blocked; no `removeItem`. -- Error: `setConfig`/`getConfig` rejects → error surfaced, no success toast, no switch, active name - unchanged. -- Integration: create-then-switch proceeds only after `setConfig` resolves; rename-of-active awaits - the delete before reload (no orphan when it flushes); rename-non-active does not reload. - -**Verification:** Full CRUD with guards; no operation leaves a missing slot or accumulates orphans; -failed writes never report success. - ---- - -- [ ] **Unit 4: Profiles UI in the Configurations tab** - -**Goal:** Replace the backup/restore framing with a Profiles experience. (Import/export, R7, is an -independently-trackable sub-task within this unit.) - -**Requirements:** R1, R2, R3, R4, R5, R7. **Dependencies:** Unit 3. - -**Files:** -- Modify: `src/app/core/components/options/configuration/config.component.ts` / `.html` / `.scss` -- Test: `src/app/core/components/options/configuration/config.component.spec.ts` (extend the existing - spec) - -**Approach:** -- Render the profile list (from `ProfileService`); mirror the `dashboards-editor` interaction. -- **Remove the `user/default` special-casing** (hide filter + the save block in - `config.component.saveConfig`, not just the list filter) so `default` appears as a normal - (protected) profile. Keep `default` non-deletable via guards, and **enforce a controlled-write - invariant at a single chokepoint**: `ProfileService.create`/import are the only callers permitted - to issue a full-`IConfig` write to a slot, each gated by a target-named confirm — this replaces - the protection the removed save block gave. -- **Specify these interactions (don't leave to the implementer):** - - *Switch:* confirmation dialog naming the target + a blocking progress affordance during the - ~1–2s reload. - - *Create:* surface clone-vs-blank explicitly (extend the name dialog with a selector, or two - entry points "New from current" / "New blank"); state the default and what clone carries over - (dashboards + theme + units). - - *Active marker + per-device cue:* mark the active profile (leading check; switch suppressed on - the active row) and label it "Active on this device." - - *State content:* copy for logged-out ("Profiles require Signal K login"), blocked-delete - (default / active / last — disabled with reason), name-collision and invalid-charset (inline - dialog validation, not a post-submit toast), and the missing-slot recovery offer (Unit 6). -- **Import (R7): create a new profile, do not auto-switch into it.** Prompt for a name (run it - through the Unit 3 validation), validate the uploaded JSON against the `IConfig` shape, and - normalize/reject its `configVersion` (route through `ConfigurationUpgradeService` or reject - unsupported); then `setConfig` under the new name and **let the user switch deliberately** — so a - shape-valid-but-unbootable import can't become the persisted active slot and boot-loop. Shape - validation is structural only, not content sanitization; confirm no imported string reaches a - non-default-escaped DOM sink. Any "import into active" path needs a target-named confirm (replace - the generic "overwrite active configuration" warning, also on the Demo/Default buttons). -- **Export:** serializes `IConfig` only; note it intentionally excludes `IConnectionConfig` - (credentials, server URL, and — after Unit 5 — `isRemoteControl`/`instanceName`). -- **Returning-user migration:** pre-existing user-scope backups appear as profiles; one-time - onboarding note + keep an export affordance so the backup workflow isn't lost. -- Theme via CSS variables. - -**Patterns to follow:** `dashboards-editor.component.ts` (+ bottom sheet); `dialog.service`. - -**Test scenarios:** -- Happy: list renders with active marked; switching a non-active profile calls `switchProfile`; - "New profile" opens the create flow with the chosen seed; rename/duplicate/delete call the service - with confirmation. -- Edge: delete affordance absent/disabled for `default`, active, and last; logged-out shows the - notice. -- Error: import of malformed JSON (bad shape or unsupported `configVersion`) is rejected with a - toast; a failing service call surfaces a toast and leaves the list intact. -- Integration: import creates a *new* profile and does NOT auto-switch into it. - -**Verification:** Full lifecycle from Options; active obvious; no destructive action lacks a -target-named confirm; import never clobbers another profile or boot-loops the device. - ---- - -- [ ] **Unit 5: Hoist remote-control identity to per-device (R8)** - -**Goal:** Move `isRemoteControl` + `instanceName` from `IAppConfig` (profile) to `IConnectionConfig` -(device) so a profile switch never changes a screen's remote role or advertised name. - -**Requirements:** R8. **Dependencies:** Unit 2 (same config plumbing); independent of Units 3/4. - -**Files:** -- Modify: `src/app/core/interfaces/app-settings.interfaces.ts` (add both to `IConnectionConfig`, - remove from `IAppConfig`) -- Modify: `src/app/core/services/settings.service.ts` (read/persist from connectionConfig; setters - route there not `patchConfig`; bump `latestConfigVersion`; add the `loadConnectionConfig` `case 13` - and the `!== latestConfigVersion` gate; remove the fields from `pushSettings`/`buildAppStorageObject`) -- Modify: `src/app/core/services/app-initNetwork.service.ts` (connectionConfig `12 → 13` step; - deferred one-time field-lift after `getConfig`) -- Modify: `src/default-config/config.blank.const.ts` (`DefaultConnectionConfig.configVersion` and - default field values) -- Modify consumers: `src/app/core/services/remote-dashboards.service.ts`, the `display.component` - setter UI, and any reader of `getIsRemoteControl*/getInstanceName*` (verify via grep) -- Test: `settings.service.spec.ts`, `app-initNetwork.service.spec.ts`, `remote-dashboards.service.spec.ts` - -**Approach:** -- **Version bump:** `IConnectionConfig.configVersion` **12 → 13**, updating *every* gate — the - `loadConnectionConfig` switch (`case 13`), the `!== latestConfigVersion` check, the - `latestConfigVersion` constant, and a new `12 → 13` step in `app-initNetwork.loadLocalStorageConfig`. - Missing a gate trips `resetConnection()` and wipes per-device state (incl. the active profile name). -- **Migration ordering (critical):** `loadLocalStorageConfig` runs *before* the profile is fetched, - so it cannot read the old `IAppConfig` values there. Do the version bump in `loadLocalStorageConfig` - but **defer the field-lift until after `getConfig` resolves** (in the bootstrap handoff / - `settings.startup`, where `initConfig.app` exists), guarded to run once. On the degraded/404 boot, - **do not finalize** the lift (so it completes on a later successful boot) rather than defaulting to - `false`/`''` and losing the user's setting. -- Remove the two fields from `IAppConfig`; update consumers in lockstep (`pushSettings`, - `buildAppStorageObject`, the `patchConfig 'IAppConfig'` path, the `display.component` setters). The - setters now route to `connectionConfig`. The `IAppConfig` removal is forward-compatible — old slots - carry the now-ignored fields. -- Repoint `RemoteDashboardsService` to the connectionConfig-backed values. - -**Patterns to follow:** the existing connectionConfig version-upgrade steps; per-device handling of -`skipUUID`. - -**Test scenarios:** -- Happy: setting `isRemoteControl`/`instanceName` persists to connectionConfig (per-device), not the - profile slot; a profile switch leaves them unchanged. -- Integration: `RemoteDashboardsService` participation reflects the device value before and after a - switch (no toggle). -- Migration: the lift runs **after** the profile loads, exactly once, **preserving** the user's - existing values; a degraded/404 boot does **not** finalize the lift (no setting loss) and it - completes on a later good boot. -- Version gate: a `v13` connectionConfig is accepted (not reset); the `12 → 13` step runs once. - -**Verification:** Switching profiles never changes remote participation or advertised name; existing -settings survive the upgrade; two screens on one profile can hold distinct names. - ---- - -- [ ] **Unit 6: Bootstrap & degraded-path recovery** - -**Goal:** Per-device active profile loads at boot; a missing/deleted active slot recovers gracefully -without data-loss surprises or `/undefined` writes. - -**Requirements:** R2, R4, R6. **Dependencies:** Unit 2 (boundary guard), Unit 3 (list/create). - -**Files:** -- Modify: `src/app/core/services/app-initNetwork.service.ts`, `src/app/app.component.ts` -- Test: `app-initNetwork.service.spec.ts`, `app.component.spec.ts` - -**Approach:** -- Confirm boot honors a non-`default` `sharedConfigName` (it does) — assert via test. -- **Actively replace** `app.component`'s recovery action (today `settings.resetSettings()`, which - rewrites the *current* — now dead — name as blank). The Unit 2 boundary guard does **not** cover - this: the name is defined-but-deleted, not unset, so the protection here is behavioral. -- New recovery offers: (a) switch to the protected `default` profile; (b) pick from `listConfigs` of - survivors. Make the action and resulting active name explicit. -- **Before creating `default`, re-verify absence** (re-fetch / `listConfigs`) to distinguish real - deletion from a transient 404 (server restart, scope not yet provisioned); use create-if-absent so - a blank create never clobbers a real `default` that reappears. -- In any degraded state, the Unit 2 boundary guard prevents `/undefined` writes. - -**Test scenarios:** -- Happy: boot with `sharedConfigName="cockpit"` loads that slot and bootstraps storage with it. -- Edge/Error: boot when the remembered slot 404s → `missing-shared-config`; recovery offers - fall-back-to-default (creating it if absent) or pick-from-list; the resulting active name is - asserted; no write targets `/undefined`. -- Edge: a transient 404 (slot re-appears on re-check) does NOT overwrite the real slot with a blank - one (create-if-absent re-verifies before writing). - -**Verification:** A device whose profile was deleted elsewhere recovers to a usable, explicitly -chosen state; no silent data loss; no corrupt slot. - ---- - -- [ ] **Unit 7: Documentation & changelog** - -**Goal:** Explain profiles and record the change. - -**Requirements:** R1–R8. **Dependencies:** Units 2–6. - -**Files:** the Login & Configuration help doc under `src/assets/help-docs/` (`/help/configuration`); -`CHANGELOG.md`. - -**Approach:** Document: what a profile is; per-device active selection (and that it doesn't follow -the user across browsers / survives a localStorage clear); clone-on-create; per-profile theme; the -per-profile **units** caveat (divergence after edit); the `default` fallback; remote-control identity -now per-device (R8); the remote-only v1 limitation; and that prior backups now appear as profiles. - -**Test expectation:** none — docs only. - -**Verification:** Help matches shipped behavior; CHANGELOG entry present. - -## System-Wide Impact - -- **Interaction graph:** a switch is a full reload → re-runs `app-initNetwork` → storage bootstrap → - `settings.startup` → `pushSettings` → dashboards signal → widgets; `RemoteDashboardsService` - re-broadcasts. After Unit 5, remote participation is device-bound and unaffected by the switch. -- **Error propagation:** storage ops throw `HttpErrorResponse`; `ProfileService` surfaces via toast - and leaves active state unchanged on failure (await + try/catch). -- **State lifecycle risks:** `patchQueue$` is fire-and-forget; switch and rename-of-active use the - bounded completion primitive; `StorageService` boundary refuses unset slot names. -- **API surface parity:** two `sharedConfigName` fields (`SettingsService` private + - `StorageService` public). The reload re-syncs both **only on the authenticated, storage-ready - path**; the degraded path is covered by the boundary guard (Unit 2) and recovery (Unit 6). - `SettingsService`'s private copy defaults to `'default'` and is never `undefined`; only - `StorageService.sharedConfigName` is unset pre-bootstrap — that is the `/undefined` risk the guard - targets. The active-profile setter must keep both coherent across a switch. -- **Integration coverage:** the reload is a no-op under `__SKIP_TEST__`; assert pre-reload side effects - in unit tests, and cover the post-reload load path with a focused integration test that drives - `AppNetworkInitService.initNetworkServices` with a non-`default` `sharedConfigName` and asserts - `bootstrapRemoteContext` is called with that name. -- **Unchanged invariants:** `IConfig` schema; `configFileVersion` (11); `IAppConfig.configVersion` - (intentionally NOT bumped — removing `isRemoteControl`/`instanceName` is forward-compatible); - `ConfigurationUpgradeService`; local-mode single-config behavior; the Signal K `applicationData` - endpoint contract; widget/dataset/history pipelines. *(`IConnectionConfig.configVersion` **is** - bumped 12 → 13 in Unit 5; two fields move out of `IAppConfig` into `IConnectionConfig`.)* - -## Risks & Dependencies - -| Risk | Mitigation | -|------|------------| -| Degraded boot leaves the storage slot name unset → writes corrupt a `/undefined` slot | Unit 2 `StorageService`-boundary guard; mutations gated on `isRemoteContextBootstrapped()`. | -| Import silently overwrites the wrong profile, or imports an unbootable config | R7/Unit 4: import creates a new profile, shape- + `configVersion`-validated, no auto-switch. | -| Missing-slot recovery recreates the dead name blank (silent data loss) | Unit 6: actively replace `resetSettings()` recovery; fall back to / pick a real profile. | -| Recovery blank-clobbers a real `default` on a transient 404 | Unit 6 re-verifies absence (re-fetch/`listConfigs`) and uses create-if-absent. | -| Unit 5 migration runs before the profile loads → loses remote-control setting | Unit 5: defer the field-lift until after `getConfig`; don't finalize on a degraded boot. | -| connectionConfig bump misses a gate → `resetConnection()` wipes device state | Unit 5: bump 12 → 13 and update all four gates; test a `v13` config is accepted. | -| Queue-drain / awaitable-delete hang the switch | One `StorageService` completion primitive with a bounded timeout; degrade to best-effort. | -| Switching toggles remote-control role/name | Unit 5: hoist `isRemoteControl`/`instanceName` to per-device. | -| Profile name path/JSON-Patch injection | Unit 3 validation invariant + `encodeURIComponent` inside `StorageService`; Unit 1 probes the server. | -| `setConfig` truthy-Promise success-on-failure | Unit 3: await in try/catch; don't port the existing pattern. | -| Server rejects arbitrary names / lacks read-after-write consistency | Unit 1 pre-flight gates the work. | -| `default` slot assumed to exist but doesn't (fresh shared user) | Treat `default` as creatable-if-absent; never assume `listConfigs` includes it. | -| Per-profile units diverge after edit | Edit-time notice + help wording (Units 4/7); enforced shared units deferred. | -| Returning users' backups surface as profiles | Onboarding note + retained export affordance (Units 4/7). | -| Clone seeds from a hollow snapshot in degraded state | Unit 2: clone only when settings loaded; else blank. | - -## Future Considerations - -- **Slot-name-as-identity is a path dependency.** The display name doubles as the storage key, so the - deferred metadata registry (icons, ordering, friendly names decoupled from keys) would be a - *breaking* change to the identity model, not additive — and renaming already needs - create-new + delete-old. If a registry looks likely, consider a thin stable id (generated key + - separate display name) sooner. -- **Local-mode and device-token (`global` scope) profiles** are the natural next scope expansions. - -## Sources & References - -- Grounding exploration + two multi-persona document-review passes (2026-06-23), verified against - `storage.service.ts`, `settings.service.ts`, `app-initNetwork.service.ts`, `app.component.ts`, - `config.component.*`, `remote-dashboards.service.ts`, `dashboards-editor.component.ts`, - `dialog.service.ts`, `app-settings.interfaces.ts`, `config.blank.const.ts`. -- Conventions: `CLAUDE.md`. -- Upstream: `mxtommy/kip` (this is the `mairas/Kip` fork, `named_configs` branch). diff --git a/docs/power-system-plan.md b/docs/power-system-plan.md deleted file mode 100644 index 4d53f180..00000000 --- a/docs/power-system-plan.md +++ /dev/null @@ -1,173 +0,0 @@ -# Power System Widgets Plan - -This document captures the agreed plan for implementing the BMS widget, Solar widget, and Power System Map widget. It preserves all decisions and technical details so each step can be executed later without losing context. - -## Implementation Tracking - -### Slice 1: Family Contract Prep (completed) - -- [x] Step 1: Shared electrical family descriptor and root path matrix scaffolded. -- [x] Step 2: BMS and Solar runtime config resolution normalized to shared tracked/group/options fields with legacy fallback support. -- [x] Step 3: Host2 and dashboard history sync now consume shared family descriptor metadata for title/eligibility/template routing. -- [x] Step 4: Future family metadata entries added (chargers, inverters, alternators, AC) as typed descriptor placeholders. -- [x] Step 5: Topology parity started by adding `associatedBus` support to BMS snapshots. -- [x] Step 6: Shared history/chart metadata registry extraction for the history dialog. -- [x] Step 7: Full config UI normalization and card/embed contract consolidation for all electrical families. -- [x] Step 8: Slice 1 acceptance pass (full targeted tests + validation checklist sign-off). - -### Slice 2: New Widgets (in progress) - -- [x] Chargers widget implementation. -- [x] Inverters widget implementation. -- [x] Alternators widget implementation. -- [x] AC widget implementation (bus + branch/leg v1 detail). - -## Step 1: BMS Widget - -### Goal -Build a Host2 widget that discovers batteries, allows user-defined banks, and displays bank and per-battery metrics in a D3 SVG layout with animated flow direction cues. - -### Core behavior -- Discovery is hybrid: auto-discover batteries under `self.electrical.batteries.*`, then allow user selection. -- Banks are user-defined groups of batteries. -- Aggregation uses simple sums where applicable. -- Visual flow direction is shown with animated arrows or dots. - -### Signal K path coverage -- Roots: `self.electrical`, `self.electrical.batteries`, `self.electrical.batteries.` -- Identity and metadata: - - `self.electrical.batteries..name` - - `self.electrical.batteries..location` - - `self.electrical.batteries..dateInstalled` (RFC3339) - - `self.electrical.batteries..manufacturer.name` - - `self.electrical.batteries..manufacturer.model` - - `self.electrical.batteries..manufacturer.URL` - - `self.electrical.batteries..associatedBus` - - `self.electrical.batteries..chemistry` -- Core electrical: - - `self.electrical.batteries..voltage` - - `self.electrical.batteries..voltage.ripple` - - `self.electrical.batteries..current` (flow out +, in -) - - `self.electrical.batteries..temperature` -- Capacity and state: - - `self.electrical.batteries..capacity.nominal` - - `self.electrical.batteries..capacity.actual` - - `self.electrical.batteries..capacity.remaining` - - `self.electrical.batteries..capacity.dischargeLimit` - - `self.electrical.batteries..capacity.stateOfCharge` - - `self.electrical.batteries..capacity.stateOfHealth` - - `self.electrical.batteries..capacity.dischargeSinceFull` - - `self.electrical.batteries..capacity.timeRemaining` -- Lifecycle totals: - - `self.electrical.batteries..lifetimeDischarge` - - `self.electrical.batteries..lifetimeRecharge` - -### UI and visuals -- Primary view: bank summary with expandable per-battery details. -- Animated flow direction for current and power. -- Theme-aware coloring using existing theme roles. - -### Config UI -- Custom bank setup panel under widget-config for selecting batteries and defining banks. - - -## Step 2: Solar Widget - -### Goal -Build a Solar Charge Controller widget that mirrors the BMS structure and supports grouping of chargers into banks. - -### Solar metrics to support -- Charger current (Amps flow out) -- Panel current (cumulative Amps flow in) -- Load port name -- Load port current (Amps out) - -### Signal K path coverage -- Roots: `self.electrical.solar`, `self.electrical.solar.` -- Positional id rule: - - The first segment after `self.electrical.solar.` is always the device id. - - Any token in that position is valid as an id (for example `power`, `charge`, `controller`, `load`). - - Setup discovery may discover ids from both `self.electrical.solar.` and `self.electrical.solar..` paths. - - Runtime value parsing accepts only `self.electrical.solar..`; root-only `self.electrical.solar.` paths are rejected for value application. -- Identity and metadata: - - `self.electrical.solar..name` - - `self.electrical.solar..location` - - `self.electrical.solar..dateInstalled` (RFC3339) - - `self.electrical.solar..manufacturer.name` - - `self.electrical.solar..manufacturer.model` - - `self.electrical.solar..manufacturer.URL` - - `self.electrical.solar..associatedBus` -- Electrical and control: - - `self.electrical.solar..voltage` - - `self.electrical.solar..voltage.ripple` - - `self.electrical.solar..current` - - `self.electrical.solar..temperature` - - `self.electrical.solar..chargingAlgorithm` - - `self.electrical.solar..chargerRole` - - `self.electrical.solar..chargingMode` - - `self.electrical.solar..setpointVoltage` - - `self.electrical.solar..setpointCurrent` - - `self.electrical.solar..controllerMode` -- Panel and load ports: - - `self.electrical.solar..panelVoltage` - - `self.electrical.solar..panelCurrent` - - `self.electrical.solar..panelTemperature` - - `self.electrical.solar..load` - - `self.electrical.solar..loadCurrent` - -### UI and visuals -- Similar bank view and per-device details as BMS. -- Animated flow direction for charge in/out. - -### Config UI -- Reuse or parameterize the bank setup panel to support solar controllers. - - -## Step 3: Power System Map Widget - -### Goal -Build a full power system map with card layout, connectors, and animated flow dots. Use BMS and Solar widgets as embedded card devices. - -### Device types from spec -- Batteries -- Alternators -- Chargers -- Solar -- Inverters -- AC buses - -### Topology constraints -- Derived from `associatedBus` only. -- No explicit DC load nodes; load is reported by batteries, inverters, and other devices. - -### Fixed slot layout (1200x700 viewBox) -- Left column: Sources (4 slots) - - Source-1: x=40, y=40, w=260, h=140 - - Source-2: x=40, y=200, w=260, h=140 - - Source-3: x=40, y=360, w=260, h=140 - - Source-4: x=40, y=520, w=260, h=140 -- Center top: Inverter/Charger bridge (1 slot) - - Inverter: x=460, y=120, w=320, h=180 -- Center bottom: Battery banks (2 slots) - - Bank-1: x=320, y=420, w=260, h=180 - - Bank-2: x=620, y=420, w=260, h=180 -- Right column: Loads (2 slots, computed totals) - - AC Loads: x=900, y=80, w=260, h=140 - - DC Loads: x=900, y=300, w=260, h=140 - -### Connector behavior -- Simple orthogonal paths between slot anchor points. -- Flow dots animate along SVG paths using `getTotalLength` and `getPointAtLength`. - -### Reuse strategy -- Embed BMS and Solar widgets as compact device cards via `widget-embedded`. -- Use a card-mode config schema with `displayMode=card` and a defined metric list. - - -## Decisions Summary -- Discovery: hybrid (auto-discover + user select) -- Grouping: user-defined banks -- Aggregation: simple sums -- Visual flow: animated dots on SVG paths -- Reuse: `widget-embedded` with card mode configs -- No explicit DC load nodes diff --git a/docs/signalk-auth-plan.md b/docs/signalk-auth-plan.md deleted file mode 100644 index c7d5fadf..00000000 --- a/docs/signalk-auth-plan.md +++ /dev/null @@ -1,673 +0,0 @@ ---- -title: "feat: Standard Signal K authentication for Skip (cookie-session + OIDC redirect)" -type: feat -status: active -date: 2026-06-23 -deepened: 2026-06-24 ---- - -# Standard Signal K authentication for Skip - -> **Plan location note:** This design doc lives on the `feat/signalk-standard-auth` branch (mirroring -> how `docs/named-configs-plan.md` is kept on `named_configs`). The auth branch bases off `master` -> (connectionConfig v12); `named_configs` (profiles, currently v13) rebases on top after auth merges. - -> **Revised 2026-06-23/24 after two seven-persona document-review passes** (coherence, feasibility, -> security, adversarial, scope, product, design). Pass 1 found three P0 breaks (cookie mode -> unreachable out-of-box; profiles hidden in cookie mode; WS never reconnects). Pass 2 found a fourth -> P0 (storage-routing split-brain: auth decoupled from `useSharedConfig` but config *persistence* was -> not) plus read-only-session, Freeboard-iframe-proxy, and loop-recovery (`noAutoLogin`) P1s — all -> folded in below. User decisions: **auto-detect mode, no toggle**; **profiles stay paused and this -> plan owns the cookie-mode profiles fix**; **sign-out cut from scope**. - -## Overview - -Skip authenticates with a self-managed login: a modal credential form POSTs username/password to -`auth/login`, stores the returned JWT in `localStorage`, and attaches it as an -`authorization: JWT ` header and a WebSocket `&token=` query parameter. It never uses the SK -session cookie and has no OIDC awareness. - -When Skip is served by a Signal K server that uses session/OIDC auth (the HaLOS deployment), this -forces a second SK-local login on top of the SSO the user already completed — and for -OIDC-provisioned users it is a dead end, because they have no SK-local password to type. - -This plan replaces Skip's auth with the SK-documented embedded-webapp pattern, **auto-detected** by -how Skip is served: - -- **Cookie mode (new, same-origin):** when Skip is served by the SK server (effective request origin - equals the app origin), send requests with credentials so the httpOnly session cookie - authenticates Skip; derive auth state from `/skServer/loginStatus`; when login is needed, redirect - to the SK-advertised login URL (`oidcLoginUrl` when OIDC is enabled, otherwise the admin login), - which drives Authelia SSO. Config persists to server `applicationData` (the path profiles use), - decoupled from the stored `useSharedConfig` flag. -- **Standalone mode (kept):** when Skip runs cross-origin (a PWA/host pointed at a boat's SK server), - keep the existing token / device-token model, because cookies cannot flow cross-origin. This - umbrella covers the two cross-origin sub-modes (user-token and device-token) plus local-only. - -Mode is auto-detected with no user-facing toggle (decision during review). A standalone security -fix rides along regardless of mode: stop persisting the login password in plaintext. - -Auth is a **hard** prerequisite for the tabled profiles feature (`docs/named-configs-plan.md`) **for -OIDC-provisioned users only** — they have no SK-local password and cannot get a per-user identity via -the form. For SK-local-password users the form already yields a valid identity, so auth-first there -is a chosen ordering (the target HaLOS deployment is OIDC-only), not a structural dependency. This -plan **owns** the change that makes profiles visible and writable in cookie mode (Unit 8); profiles -otherwise stay paused and rebase on top. - -## Problem Frame - -The user hit Skip's "Sign in to Signal K" dialog while already authenticated to HaLOS via Authelia -SSO. Three real defects, in order of severity: - -1. **No SSO participation / dead-end login.** The SK server (HaLOS) is itself an OIDC client of - Authelia. After SSO the browser already holds SK's `JAUTHENTICATION` cookie, same-origin under - `:4430` where Skip is served. Skip discards it and demands a separate SK-local login. OIDC users - have no SK-local password (confirmed), so the form cannot be satisfied. -2. **Plaintext credential storage.** `connectionConfig.loginPassword` is persisted in plaintext and - re-sent on token renewal. -3. **Token-in-JS exposure.** The JWT lives in `localStorage` and rides in the WebSocket query - string — the exposure surface of SK advisory GHSA-fq56-hvg6-wvm5. The sanctioned pattern is the - httpOnly session cookie. - -"The custom form breaks OIDC" is imprecise: the SK OIDC flow keeps working; Skip refuses to -participate in it. The fix is "make Skip use the SK session like the SK docs say," not "make Skip -OIDC-aware" — Skip needs no OIDC code, only cookie + loginStatus + redirect. - -## Requirements Trace - -- **R1.** In cookie mode (same-origin) with an auth-requiring SK server, Skip must not show its own - credential form; it authenticates via the SK session cookie. -- **R2.** Auth state in cookie mode is derived from `/skServer/loginStatus`, not token presence. -- **R3.** When login is needed in cookie mode, Skip redirects to the SK-advertised login - (`oidcLoginUrl` when `oidcEnabled`, else admin login), participating in SSO; honor `oidcAutoLogin`. -- **R4.** Cookie-mode REST, WebSocket, and the embedded Freeboard-SK iframe carry the session cookie - (`withCredentials` / same-origin handshake / origin-correct cookied iframe). No JWT in - `localStorage`, no `&token=` on the WS, and no `?token=` on the iframe in cookie mode. -- **R5.** Standalone cross-origin mode keeps the existing user-token and device-token login. Mode is - auto-detected from the effective request origin. -- **R6.** `loginPassword` is no longer persisted in either mode. Token-mode renewal no longer - re-sends stored credentials; on expiry Skip surfaces re-login. -- **R7.** Cookie mode auto-engages for a fresh same-origin install of an auth-requiring SK server, - without the user first enabling shared config. -- **R8.** Cookie-mode auth must not hide profiles: availability keys off the session signal, not - token presence (this plan owns the fix; it lands in the profiles rebase — Unit 8). -- **R9.** The redirect-back target is validated as same-origin/relative-only, redirects are bounded - across reloads (kiosk + `oidcAutoLogin` must not loop), and recovery uses `noAutoLogin` so a manual - retry is not auto-bounced. -- **R10.** The CSRF posture for cookie-authenticated state-changing calls is verified (SameSite / - Origin enforcement) as a gating acceptance check, not just recorded. -- **R11.** The change must not break standalone installs, and same-origin non-OIDC secured servers - get a coherent admin-login redirect, not a regression. -- **R12.** In cookie mode, config **persistence** routes to server `applicationData` regardless of - the stored `useSharedConfig` flag — the server-storage decision is derived from resolved cookie - mode, the same way auth is. -- **R13.** Write affordances (profile create/rename/delete/switch, config save) are gated on a - write-capable session; a logged-in but read-only session must not present editable controls that - silently fail server-side. - -## Scope Boundaries - -- **Not** adding OIDC/OAuth client code to Skip. Skip delegates entirely to the SK server's login. -- **Not** implementing proxy-layer ForwardAuth (see Alternatives — SK does not trust forwarded - identity headers). -- **Not** a user-facing mode toggle, and **not** a passive mode-explainer label. Mode is - auto-detected and otherwise transparent (the identity block shows who you are, not which mode). -- **Not** a cookie-mode sign-out flow (cut during review). Session ends via SK/Authelia expiry. Known - limitation: on a shared/helm display the next user resumes the prior session until expiry — - acceptable for single-operator boats; revisit sign-out + SSO single-logout if multi-user-per - -display deployments emerge. -- **Not** re-building the device-access-token acquisition UI (no live caller). The device-token - plumbing stays intact as the cross-origin fallback. -- **Not** resuming the profiles feature; this plan only unblocks it and owns the availability fix. -- **Not** bumping the `connectionConfig` schema version (stay at 12; idempotent password purge). - -## Context & Research - -### Relevant code and patterns - -- `src/app/core/services/authentication.service.ts` — `login()` POSTs `auth/login`; `setSession()` / - `setDeviceAccessToken()` store `IAuthorizationToken` (`authorization_token`) and drive - `_IsLoggedIn$` / `_authToken$`; the constructor **deletes a stored user token but re-emits a - stored device token** synchronously; renewal timer re-POSTs stored `loginName`/`loginPassword`; - URLs derived from `serverServiceEndpoint$` (post-discovery). `renewToken()` (`auth/validate`) is - unused — and the endpoint **404s on the live server**. -- `src/app/core/services/settings.service.ts` — **`setConnectionConfig` couples - `useDeviceToken = !useSharedConfig`** (not independent). In-memory `useSharedConfig` field - initializes `true`, but `DefaultConnectionConfig` (config.blank.const.ts) sets it **false**, and a - fresh install persists the const → false. **16 `if (this.useSharedConfig)` write branches** plus - `startup()`'s shared-vs-local load route persistence; `buildConnectionStorageObject` persists - `loginPassword`. -- `src/app/core/services/signalk-connection.service.ts` — `processEndpointResponse()` has a **proxy - mode** that rewrites HTTP/WS endpoints to `window.location.origin`, but leaves `signalKURL` - (the configured URL) unchanged. Discovery GET to `/signalk/` is unauthenticated. -- `src/app/core/services/app-initNetwork.service.ts` — `APP_INITIALIZER` bootstrap. `signalKUrl` - defaults to `window.location.origin`. Bootstrap login gates on - `useSharedConfig && loginName && loginPassword`; remote-profile bootstrap gates on - `isLoggedIn && useSharedConfig`; three `router.navigate(['/login'])` sites. State keyed on - `isLoggedIn` (token presence today). -- `src/app/core/interceptors/authentication-interceptor.ts` — adds the `JWT` header **whenever a - token exists** (keys on token, not mode); no `withCredentials`. -- `src/app/core/services/signalk-delta.service.ts` — WS URL from `WsServiceUrl`; appends `&token=` - only when a token exists; the **only** auth-driven reconnect is the `authToken$`-change - subscription, guarded by `isFullyConnected()` (dead in cookie mode — token is permanently null). - Initial connect is driven by the bootstrap `finally` (`startWebSocketConnection()` when - `isHTTPConnected()`). -- `src/app/widgets/widget-freeboardsk/widget-freeboardsk.component.ts` — builds the iframe `src` - from `appSettings.signalkUrl.url` (the **configured** URL) and appends `?token=` when a token - exists; a **third token carrier** that also resolves to the configured origin, not the rewritten one. -- `src/app/core/services/storage.service.ts` — readiness gated on - `networkStatus===connected && _isLoggedIn && serverEndpoint` (no `useSharedConfig` reference); - user-scope `applicationData` path `user/kip//`; `isAppDataSupported` from version ≥ - 1.27.0. Scope is resolved **server-side** from the session; no client-side username needed. -- `src/app/widgets/widget-login/widget-login.component.ts` — the `/login` route; **`ngOnInit` opens - the credential dialog unconditionally** (renders no body of its own); writes `loginPassword` back - via `setConnectionConfig`. -- `src/app/core/components/options/signalk/signalk.component.ts` — Connectivity tab; opens the - credential dialog; `connectToServer` deletes the token only when - `authToken && !useSharedConfig && !isDeviceAccessToken`; reloads after connect. -- `src/app/app.routes.ts` — `/login` → `WidgetLoginComponent`. -- **Profiles (on `named_configs`, not master):** `config.component.ts` computes - `profilesAvailable = supportApplicationData && Boolean(authToken()?.token) && !isTokenTypeDevice`; - `getActiveConfig()` keys off `useSharedConfig`. The gates Unit 8 must re-key. `config.component` - already injects `AuthenticationService` and uses `toSignal(authToken$)`, so consuming a new signal - is mechanically simple. - -### Verified device + server facts (halosdev.hal, signalk-server v2.27.0) - -- KIP is served at `https://halosdev.hal:4430/@mxtommy/kip/`, **same origin** as the SK API at - `:4430`. The `/signalk-server/` Traefik path 302-redirects to `:4430`; Skip makes relative calls - from `:4430`. -- `GET /skServer/loginStatus` at `:4430` → 200 JSON: `status:"notLoggedIn"`, - `authenticationRequired:true`, `oidcEnabled:true`, `oidcAutoLogin:true`, - `oidcLoginUrl:"/signalk/v1/auth/oidc/login"`, `oidcProviderName:"HaLOS SSO"`. Probed **without** - credentials — re-verify behavior with `withCredentials` (deferred). -- `GET /signalk/v1/auth/oidc/login` → 302 to Authelia auth-code + PKCE; OIDC `redirect_uri` is the - path form (`/signalk-server/.../callback`). SK's OIDC login accepts a `redirect` param (`req.query - .redirect`, since v2.20.0); local login is reachable at `/admin/#/login?noAutoLogin=true`, and its - page reads `redirect` out of the hash query (since v2.27.0). -- `POST /signalk/v1/auth/validate` → **404**. No token-refresh endpoint. -- OIDC-provisioned users have **no** SK-local password (user-confirmed). -- `connectionConfig` version is 12 on `master`; `named_configs` bumps to 13. `named_configs`'s - merge-base with `master` is `master` HEAD, and `master` has **no** profiles code — so the - auth-merges-before-Unit-8 window touches no production user (profiles ship only via the later rebase). - -### External references - -- SK embedded-webapp auth: https://demo.signalk.org/documentation/develop/webapps.html -- SK security / login + loginStatus + cookies: https://signalk.org/specification/1.7.0/doc/security.html -- `loginRedirect.ts` (validation: relative-only, reject `//`, control chars, self-route loop — **no - budget**): https://github.com/SignalK/signalk-server/blob/master/packages/server-admin-ui/src/views/security/loginRedirect.ts -- SK OIDC (`redirect`, `noAutoLogin`): https://raw.githubusercontent.com/SignalK/signalk-server/master/docs/oidc.md - — its "Login Endpoint" section names the param `returnTo`, which no server version reads. The - webapps guide (`docs/develop/webapps.md`) has it right; prefer that page. -- Token-theft advisory: GHSA-fq56-hvg6-wvm5 -- KIP README (same-origin / bundled-with-SK is the primary distribution): https://github.com/mxtommy/Kip - -## Key Technical Decisions - -- **Mode detection is two-stage, origin-first, decoupled from `useSharedConfig`.** Stage 1 - (synchronous, pre-discovery, in the `AuthenticationService` constructor): cookie mode is a - *candidate* when the effective request origin equals `window.location.origin` — true when - `proxyEnabled` (endpoints rewrite to it) or when `connectionConfig.signalKUrl` parses same-origin - (empty → app origin). Stage 2 (after `loginStatus`): if `authenticationRequired` and - `applicationData` supported, full cookie mode engages, regardless of the stored flags. Resolves - out-of-box reachability (R7) and the flag-coupling contradiction. -- **Two derived signals, both owned by Unit 3** (they need `loginStatus`): `isUserSession` = - cookie-mode-logged-in OR (token present AND not device); `canWriteUserData` = `isUserSession` AND a - write-capable `loginStatus.userLevel` (`admin` or `readwrite`). **Corrected at deploy time:** the - write gate keys off `userLevel` (the signed-in user's `skPrincipal.permissions`), **not** - `loginStatus.readOnlyAccess` — that field is the server's `allow_readonly` (anonymous-read) config and - is `true` on any server permitting anonymous read, which would wrongly mark an admin read-only. - `isLoggedIn$` is set from `loginStatus` in cookie mode (no token). - Storage readiness/bootstrap keep keying off `isLoggedIn$`; profiles availability off `isUserSession`; - write affordances off `canWriteUserData`. Unit 2 exposes only synchronous `authMode`. -- **Storage routing decoupled from `useSharedConfig` (R12).** A resolved "use server storage" signal - (true in cookie mode) re-keys the `settings.service` write branches and `startup()` load, the - `app-initNetwork` remote-profile gates, and `config.component.getActiveConfig()`. Without this, - cookie auth engages while config persists to localStorage (the pass-2 split-brain). -- **Three carriers branch on mode, not two:** the HTTP interceptor (`withCredentials`, no header), - the WS (`&token=`), and the Freeboard-SK iframe (`?token=`). All branch on **mode first** and - suppress any stored token in cookie mode. The iframe `src` is built from `window.location.origin` - in cookie mode (proxy leaves `signalKUrl` cross-origin), else the cookie won't flow. -- **WS (re)connect in cookie mode is driven by `isLoggedIn$` transition**, not `authToken$` change - (dead in cookie mode), reusing the existing `isFullyConnected()` guard to avoid a double-connect - with the bootstrap's `startWebSocketConnection()`. A WS drop in cookie mode triggers a - `loginStatus` re-check. -- **Stale-token suppression gated in the constructor.** The synchronous `authMode` gates the - constructor's stored-token load: in cookie mode the device-token re-emit is skipped (keep - `authToken$` null) rather than emit-then-clear, so early subscribers never latch a token. - Same-origin device-token installs are not stranded — a device token is only dropped once a cookie - session is actually obtainable. -- **Redirect safety is normative.** Validate the redirect-back target relative-only (single leading - `/`, reject `//`, backslashes, control chars < 32, scheme, host, self-route login paths). Param is - SK's `redirect`. A reload-surviving sessionStorage budget caps attempts (kiosk - + `oidcAutoLogin` defeats a single-shot guard), **resets on a confirmed `loginStatus==='loggedIn'`**, - and is **bypassed by an explicit user Sign-in click**. The recovery screen's manual Sign in uses - `noAutoLogin=true` so SK does not auto-bounce it. -- **CSRF is a gating check (R10), not just recorded.** The deploy-time test must confirm a cross-site - forged profile/applicationData write is actually blocked. If SK enforces neither Origin/Referer nor - a non-permissive SameSite on state-changing calls, this re-plans a client-side defense rather than - silently accepting it. -- **No reliance on `auth/validate`** (404). Cookie lifetime owned by SK/Authelia; token mode (cross - -origin only now) re-prompts on expiry. -- **`loginPassword` kept transient (in-memory), never persisted.** Cross-origin login collects it and - passes it to `login()` in-memory. -- **Accepted residual:** token mode (cross-origin standalone + device-token) retains JWT-in - -`localStorage` and WS `&token=` because cookies cannot cross origin; the GHSA exposure is bounded - to cross-origin installs, not eliminated. - -## High-Level Technical Design - -> *Directional guidance for review, not implementation specification.* - -### Auth mode decision matrix - -Detection is origin-first; the manual flags do not gate cookie mode. `useDeviceToken = -!useSharedConfig` is enforced by existing code, so they are one "cross-origin intent" axis. - -| Effective origin vs app | loginStatus | Mode | Mechanism | -|---|---|---|---| -| same origin | authRequired, logged-in, write | **cookie (write)** | cookie auth (REST + WS + origin-correct iframe); server-side shared config; profiles + writes | -| same origin | authRequired, logged-in, read-only | **cookie (read-only)** | cookie auth; `canWriteUserData` false → write controls disabled with explanation | -| same origin | authRequired, not logged in | **cookie → redirect** | loginStatus-driven redirect (`redirect`, budget-guarded, `noAutoLogin` on recovery) | -| same origin | auth not required | anonymous read (Unit 6 owns) | no auth; no redirect; "Connected (no sign-in required)"; profiles unavailable | -| cross origin, useSharedConfig true | — | user-token | form → JWT (header + `&token=`) — unchanged, minus plaintext password | -| cross origin, useSharedConfig false | — | device-token / local-only | stored device JWT if present, else local-only — unchanged | - -### Cookie-mode bootstrap + SSO - -```mermaid -sequenceDiagram - participant Skip as Skip (APP_INITIALIZER) - participant SK as Signal K server (same origin) - participant IdP as Authelia (OIDC) - - Note over Skip: mode candidate = same effective origin (sync, pre-discovery) - Skip->>SK: GET /signalk/ (discovery, withCredentials; unauthenticated, cookie harmless) - Skip->>SK: GET /skServer/loginStatus (withCredentials) - alt loggedIn (write or read-only per readOnlyAccess) - SK-->>Skip: {loggedIn, userLevel, readOnlyAccess} - Skip->>Skip: isLoggedIn$=true (no token); isUserSession=true; canWriteUserData=!readOnly; storage ready - Skip->>SK: applicationData + WS + iframe (cookie carries auth) - else notLoggedIn AND authenticationRequired - SK-->>Skip: {notLoggedIn, oidcEnabled, oidcAutoLogin, oidcLoginUrl} - Skip->>Skip: "Signing in via SSO…"; check budget - Skip->>SK: redirect to oidcLoginUrl?redirect= (budget-guarded) - SK->>IdP: auth-code + PKCE - IdP-->>SK: callback -> sets JAUTHENTICATION cookie - SK-->>Skip: redirect back; loginStatus -> loggedIn (budget reset) - else notLoggedIn AND NOT authenticationRequired - Skip->>Skip: anonymous read; "Connected (no sign-in required)"; no redirect - else budget exhausted OR auth declined/cancelled - Skip->>Skip: auth-blocked screen (per-cause copy; manual Sign in uses noAutoLogin; Connectivity link) - end -``` - -## Open Questions - -### Resolved during planning / review - -- **Mode activation** — auto-detect, no toggle. Two-stage origin-first detection, decoupled from - `useSharedConfig` for both auth and storage. -- **Same-origin detection** — compare the **effective** request origin (accounts for `proxyEnabled` - + port) to `window.location.origin`. Synchronous, pre-discovery. -- **Profiles sequencing** — paused; this plan owns the cookie-mode availability/write fix (Unit 8), - landing in the profiles rebase. The auth-merge window is benign (profiles not on master). -- **Sign-out** — cut (known shared-display limitation noted). -- **`loginStatus` base** — `new URL(effectiveOrigin).origin + '/skServer/loginStatus'`, synchronous. -- **`auth/validate`** — 404; token mode re-prompts on expiry. -- **`loginPassword`** — transient in-memory, never persisted. -- **`connectionConfig` version** — stay 12; idempotent purge. -- **`isUserSession`/`canWriteUserData`** — owned by Unit 3; profiles use the booleans, no username. -- **Read-only sessions** — write capability is `loginStatus.userLevel` ∈ {`admin`, `readwrite`} - (deploy-confirmed against signalk-server v2.27.0); write affordances gated on `canWriteUserData`. - -### Deferred to implementation (deploy-time; not exercisable by `ng serve`) - -- **WS / Freeboard-iframe cookie handshake** — confirm the same-origin upgrade/iframe authenticate - via cookie with no token param. -- **withCredentials on discovery/loginStatus** — re-verify the already-probed GETs behave the same - with `withCredentials`; for `proxyEnabled` + cross-origin `signalKUrl`, the discovery GET is cross - -origin-with-credentials and depends on SK CORS allow-credentials. -- **OIDC redirect round-trip** — the param name is settled: `redirect`, read by both endpoints (#554). - Still confirm the callback returns to Skip at `:4430` without a loop, and that the inner `#` of a - hash-routed target survives the round trip. -- **CSRF / SameSite** — record the JAUTHENTICATION SameSite attribute, confirm a cross-site forged - write is blocked, and that Skip's writes succeed (R10 gating check). -- **`userLevel`/`readOnlyAccess` values** — confirm the exact value set on v2.27.0 that maps to read - -only so the write-gating is correct. -- **401 mid-session** — confirm the status/shape SK returns when the cookie expires (401 vs - 302-to-HTML) and that the no-reload re-check path re-establishes the WS. -- **Kiosk reload type** — confirm HaLOS displays reload in-page (sessionStorage budget survives) vs - process-restart (budget resets each boot). - -## Implementation Units - -- [x] **Unit 1: Stop persisting the login password; fix the bootstrap gate** - -**Goal:** Remove plaintext `loginPassword` persistence; keep it transient; ensure the bootstrap login -gate does not silently route every shared-config user to `/login` once the password is gone. - -**Requirements:** R6 — **Dependencies:** None (independently shippable security fix). - -**Approach (Option A — JWT becomes the persisted credential):** Implementation revealed the persisted -password is load-bearing for post-login: login stores a JWT then `reloadApp()`, and the constructor -*deletes* the user-session token on startup, so today the persisted password is what re-establishes -the session after reload. Removing the password without changing that would loop login→reload→login. -Fix: the constructor **keeps an unexpired user-session token** (mirroring device tokens), so the -expiring JWT — not the plaintext password — is the cross-reload credential (strictly more secure; -consistent with the token-mode accepted-residual). `connectToServer` must do an **in-memory -`auth.login()` before reload** (it currently relies on the persisted password + bootstrap re-login); -`widget-login`'s `serverLogin` already logs in in-memory. `app-initNetwork`'s password-gated bootstrap -login then self-disables (password never persisted) and needs no change. Tradeoff: a user JWT now -persists across browser restarts until expiry (session-residue on shared displays — same class as the -cookie-mode sign-out limitation). - -**Files:** Modify `authentication.service.ts` (constructor keeps unexpired user token; renewal drops -stored-password re-login → `deleteToken()` on user-token expiry), `settings.service.ts` -(`buildConnectionStorageObject` excludes `loginPassword`; idempotent purge on load; no version bump), -`interfaces/app-settings.interfaces.ts` (`loginPassword` optional/transient), -`options/signalk/signalk.component.ts` (`connectToServer` logs in in-memory before reload). No change -needed in `app-initNetwork.service.ts` (login gate self-disables) or `widget-login.component.ts` -(already in-memory; prefill is now empty, expected). Test: `authentication.service.spec.ts`, -`settings.service.spec.ts`. - -**Test scenarios:** serialized config has no `loginPassword`; legacy config with `loginPassword` is -stripped and re-persisted (no version change); renewal with no stored password/cookie does not POST -credentials and routes to re-login; a transient password completes a login without being persisted. - -**Verification:** suites green; no password in persisted config; renewal references no stored password. - -- [x] **Unit 2: Auth mode detection + constructor token suppression** - -**Goal:** Synchronous origin-first `authMode` (pre-discovery); skip the constructor device-token -re-emit in cookie mode; conditional device-token clearing. - -**Requirements:** R5, R7 — **Dependencies:** None (consumed by Units 3–8). - -**Files:** Modify `authentication.service.ts` (synchronous `authMode` from a `connectionConfig` read; -`effectiveOriginIsSameAsApp` covering `proxyEnabled` + port; constructor skips device-token re-emit in -cookie mode; device token dropped only once a cookie session is obtainable). Test: -`authentication.service.spec.ts`. - -**Approach:** `authMode` must be synchronous so the interceptor is in cookie mode for the very first -discovery GET. Unit 2 does **not** compute `isUserSession`/`canWriteUserData` (those need loginStatus -— Unit 3). - -**Test scenarios:** empty/same-origin `signalKUrl` + auth-requiring → cookie candidate; cross-origin -`signalKUrl` → token; `proxyEnabled` + cross-origin `signalKUrl` → cookie candidate; same host -different port → per the effective-origin rule (define + test); stored device token + cookie candidate -→ not re-emitted, `authToken$` null; same-origin device-token install with no cookie session → -device token retained (not stranded). - -**Verification:** matrix candidates covered; no token latched by early subscribers in cookie mode. - -- [x] **Unit 3: loginStatus session state — isLoggedIn$, isUserSession, canWriteUserData** - -**Goal:** In cookie mode, derive session state from `GET /skServer/loginStatus` (credentialed), -capturing `readOnlyAccess`/OIDC descriptors; fail closed. - -**Requirements:** R2, R3, R13 — **Dependencies:** Unit 2. - -**Files:** Modify `authentication.service.ts` (loginStatus query; base from effective origin, -synchronous, not the `/signalk/v1` base; set `isLoggedIn$`; own `isUserSession` and `canWriteUserData`; -capture `oidcEnabled`/`oidcAutoLogin`/`oidcLoginUrl`/`authenticationRequired`/`readOnlyAccess`). Test: -`authentication.service.spec.ts`. - -**Approach:** `isLoggedIn$` true **only** on parsed `status==='loggedIn'`; any error, timeout, non --JSON, or unexpected shape → not-logged-in. `canWriteUserData` = logged-in AND `userLevel` ∈ {`admin`, -`readwrite`} (not `readOnlyAccess`, which is the server's anonymous-read config flag — see Key Decisions). -Pick a consistent shape (Observable/signal) for the session signals so Units 6/7/8 consume uniformly. - -**Test scenarios:** `loggedIn` write → `isLoggedIn$`/`isUserSession`/`canWriteUserData` true, no -token; `loggedIn` + `readOnlyAccess` → `isUserSession` true, `canWriteUserData` false; `notLoggedIn` → -all false, OIDC descriptors captured; unreachable/non-JSON/unexpected → not-logged-in, no throw; token -mode does not call loginStatus. - -**Verification:** suite green; session state + write-capability reflect loginStatus, fail-closed. - -- [x] **Unit 4: Credential carriage — interceptor, WS, Freeboard iframe** - -**Goal:** Carry the session cookie on all three same-origin carriers; suppress token params in cookie -mode; drive WS reconnect off session state. - -**Requirements:** R4 — **Dependencies:** Units 2, 3. - -**Files:** Modify `authentication-interceptor.ts` (branch on **mode first**: cookie → -`withCredentials`, no header, even with a token present), `signalk-delta.service.ts` (cookie → omit -`&token=`; add an `isLoggedIn$`-transition (re)connect reusing the `isFullyConnected()` guard; WS drop -→ loginStatus re-check), `widgets/widget-freeboardsk/widget-freeboardsk.component.ts` (cookie → build -`src` from `window.location.origin`, no `?token=`). Test: `authentication-interceptor.spec.ts`, -`signalk-delta.service.spec.ts` (extend existing). - -**Test scenarios:** cookie request → `withCredentials`, no header; token request → header, no -`withCredentials`; cookie request with a stored token → still no header (mode wins); WS URL omits -`token=` in cookie mode; iframe `src` host == `window.location.origin` in cookie mode (incl. -`proxyEnabled` + cross-origin `signalKUrl`); `isLoggedIn$`→true triggers one WS connect, no -double-connect during bootstrap. - -**Verification:** suites green; deploy-time test confirms live cookie on REST, WS, iframe. - -- [x] **Unit 5: Decouple config-storage routing from `useSharedConfig`** - -**Goal:** Make config persistence honor cookie mode (server storage) independent of the stored -`useSharedConfig` flag — the pass-2 split-brain fix. - -**Requirements:** R12 — **Dependencies:** Units 2, 3. - -**Files:** Modify `settings.service.ts` (introduce an effective "use server storage" signal from -resolved cookie mode; re-key the 16 `if (useSharedConfig)` write branches and the `startup()` shared --vs-local load), `app-initNetwork.service.ts` (remote-profile bootstrap gate keys off the effective -signal, not raw `useSharedConfig`), and the profiles `config.component.getActiveConfig()` (rebase-time, -keys off the effective signal). Test: `settings.service.spec.ts`. - -**Approach:** the effective storage signal is true whenever resolved cookie mode is active. Keep -`useSharedConfig` semantics for cross-origin. Storage readiness already keys off `isLoggedIn$`, so the -profile CRUD engine and the settings persistence engine now agree in cookie mode. - -**Test scenarios:** cookie mode + `useSharedConfig=false` → writes route to server applicationData, -`startup()` loads remote; cross-origin `useSharedConfig=true` → server storage (unchanged); cross --origin `useSharedConfig=false` → localStorage (unchanged); a theme/dashboard edit in cookie mode -persists to the server slot and reloads from it. - -**Verification:** in cookie mode, settings + profile CRUD both target the server; no localStorage -split-brain. - -- [x] **Unit 6: Bootstrap rework + redirect (safety + transitional/recovery UI)** - -**Goal:** Cookie-mode bootstrap uses loginStatus; replaces `/login` navigations with a validated, -budget-guarded SSO redirect; handles the anonymous-read branch; shows transitional and per-cause -recovery states. - -**Requirements:** R1, R3, R9, R11 — **Dependencies:** Units 2, 3, 4, 5. - -**Files:** Modify `app-initNetwork.service.ts` (cookie branch: skip credential login; use loginStatus; -logged-in → storage bootstrap; not-logged-in + authRequired → redirect; not-logged-in + auth NOT -required → anonymous-read ready, no redirect; convert all three `/login` navigations). Create/modify a -redirect helper (relative-only validation incl. control chars + self-route; `returnTo` param; -`noAutoLogin` on recovery; reload-surviving budget that resets on success and is bypassed by explicit -Sign in). Modify bootstrap/route presentation for the "Signing in via SSO…" transitional state and the -auth-blocked screen with **per-cause** copy/actions (unreachable → Retry; budget-exhausted → reset-on --click Sign in; cancelled → Sign in). Test: `app-initNetwork.service.spec.ts`. - -**Approach:** budget guard survives reloads; manual Sign in is user-initiated (not the auto-loop). -Anonymous-read proceeds read-only with no Sign-in dead-end. - -**Test scenarios:** cookie logged-in → ready, storage bootstraps; cookie not-logged-in + oidcAutoLogin -→ one redirect with a validated relative `returnTo`; returned still notLoggedIn within budget → no -immediate re-redirect; budget exhausted across reloads → auth-blocked screen, manual Sign in resets -budget + uses `noAutoLogin`; redirect param with `//`/scheme/host/control-char → rejected; auth-not --required same-origin → anonymous-read ready, no redirect; token mode unchanged. - -**Verification:** suite green; deploy-time SSO round-trip returns logged in without loop. - -- [x] **Unit 7: `/login` route + Connectivity tab UX** - -**Goal:** Same-origin entry points redirect to SK login (transitional state) instead of the dialog; -cross-origin keeps the dialog. Connectivity tab shows session identity, read-only state, anonymous -state, loading states; no password field; no sign-out; no mode-explainer label. - -**Requirements:** R1, R5, R11, R13 — **Dependencies:** Units 2, 3, 6. - -**Files:** Modify `widget-login/widget-login.component.ts` (cookie → render "Signing in via SSO…" and -redirect, **not** the dialog in `ngOnInit`; standalone → existing dialog), `options/signalk/signalk.component.ts` -(cookie → no password field; identity block: logged-in shows provider name + read/write level, -read-only disables write controls with explanation, not-logged-in shows Sign in, anonymous shows -"Connected (no sign-in required)", in-flight shows a loading affordance, unreachable shows a connection -error not a Sign-in CTA; extend `connectToServer` to clear a stored token when the new config resolves -to cookie mode). Use aria-live + focus management on transitional/blocked screens. Test: -`widget-login/login.component.spec.ts`, `signalk.component.spec.ts` (extend/create). - -**Test scenarios:** cookie `/login` → redirect (transitional), not the dialog; token `/login` → dialog; -cookie Connectivity has no password field; read-only session → write controls disabled with -explanation; anonymous → "Connected" identity, no dead-end Sign in; loginStatus in-flight → loading, -not "Not signed in"; editing `signalKUrl` to flip mode clears stored auth on reload. - -**Verification:** suites green; deploy-time visual check of same-origin UX, read-only, and transitional -states. - -- [ ] **Unit 8: Profiles availability + write-gating in cookie mode (cross-branch follow-on)** - -**Goal:** Make profiles visible and writable in cookie mode by keying availability off `isUserSession` -and write actions off `canWriteUserData`. **Not a same-branch unit** — these files exist only on -`named_configs`; this lands during the profiles rebase onto merged auth and is owned by this plan's -definition of done. The auth merge alone does not satisfy R8. - -**Requirements:** R8, R13 — **Dependencies:** Unit 3 (`isUserSession`/`canWriteUserData`), applied at -rebase. - -**Files (on rebased `named_configs`):** `options/configuration/config.component.ts` (`profilesAvailable` -keys off `isUserSession`; write actions gated on `canWriteUserData`), `profile.service.ts` (scope -selection works under a cookie session), `config.component.getActiveConfig()` (Unit 5 effective-storage -signal). Test: `config.component.spec.ts`, `profile.service.spec.ts` (update mocks). - -**Test scenarios:** cookie write session → profiles available + writable; cookie read-only session → -profiles visible, write controls disabled (no silent 403); cookie not-logged-in → unavailable; device --token → unavailable; cross-origin user-token → available. - -**Verification:** on the rebased branch, a cookie-mode write-capable SSO user sees and **writes** -profiles (deploy-time acceptance — the prerequisite's success gate). The auth-to-master merge is not -"done" until this rebase lands and the check passes; if it slips, auth ships as a security+SSO -improvement with the prerequisite explicitly marked unproven. - -- [x] **Unit 9: Documentation, design doc, changelog, deploy-time acceptance test** - (docs/changelog delivered; the on-device acceptance checklist below is the remaining manual gate) - -**Goal:** Document dual-mode auth and record the design + verification. - -**Requirements:** R6, R10, R11 — **Dependencies:** Units 1–8. - -**Files:** `docs/signalk-auth-plan.md` (this design doc, on the branch); `CHANGELOG.md` (call out -password-storage removal **and** that cross-origin token users re-login **on expiry** — the session -JWT persists across reloads via Option A; there is no `auth/validate` refresh). Skip help content has -no connection/auth section, so there is nothing relevant to modify there. - -**Approach:** the deploy-time acceptance test (build locally, deploy to the device webapp dir; never -build on device) covers: loginStatus drives state; redirect triggers OIDC and returns without loop; -cookie authenticates REST + WS + Freeboard iframe with no token; **CSRF forged-write is blocked -(R10)**; read-only session disables writes; cookie mode persists config to the server slot (Unit 5); -profiles (post-rebase) read/write user scope. - -**Test scenarios:** `Test expectation: none — documentation only.` - -**Verification:** help reflects dual-mode; design doc on the branch; acceptance checklist run. - -## System-Wide Impact - -- **Interaction graph:** `AuthenticationService` (mode + session signals) feeds the interceptor, the - WS delta service, the Freeboard iframe, `StorageService` readiness, `AppNetworkInitService` - bootstrap, `SettingsService` storage routing, and (post-rebase) profiles. `isLoggedIn$`, - `isUserSession`, `canWriteUserData` are authoritative. -- **Error propagation:** cookie 401 → loginStatus re-check → budget-guarded redirect or auth-blocked - screen; loginStatus unreachable/malformed → fail-closed not-logged-in. Token-mode 401 unchanged. -- **State lifecycle:** stale token (user/device) suppressed in cookie mode at construction; config - -change normalizes auth state; redirect budget resets on success. -- **API surface parity:** REST (interceptor), WS (`&token=`), and the Freeboard iframe (`?token=`, - origin-correct) all carry the cookie in cookie mode. -- **Storage parity:** `settings.service` write branches, `startup()` load, `app-initNetwork` gates, - and `config.component.getActiveConfig()` all key off the effective storage signal in cookie mode - (Unit 5) — not raw `useSharedConfig`. -- **Integration coverage:** the live cookie round-trip (REST + WS + iframe + SSO redirect + CSRF + - read-only) is only provable with a deployed build + OIDC login (Unit 9). -- **Unchanged invariants:** local-only mode, device-token plumbing, user-scope `applicationData` - paths (server-side scope), and the `connectionConfig` schema version (stays 12). - -## Risks & Dependencies - -| Risk | Likelihood | Impact | Mitigation | -|---|---|---|---| -| Storage routing not fully decoupled (split-brain) | Med | High | Unit 5 re-keys all 16 branches + startup + bootstrap + getActiveConfig; deploy test persists to server slot | -| Read-only session shows editable profiles that 403 | Med | High | `canWriteUserData` gates write affordances (Unit 3/7/8); read-only test scenarios | -| WS / iframe cookie handshake fails same-origin | Low | High | Deploy test early; iframe `src` origin-correct; fallback (short-lived token) is a re-plan, not in scope | -| Freeboard iframe cross-origin under proxy | Med | High | Build iframe `src` from `window.location.origin` in cookie mode (Unit 4) | -| Redirect loop across reloads (kiosk + oidcAutoLogin) | Med | High | Reload-surviving budget + `noAutoLogin` recovery + reset-on-success (Unit 6, R9) | -| Open redirect via `returnTo` | Low | High | Relative-only validation incl. control chars + self-route (Unit 6, R9) | -| CSRF on cookie-authenticated writes | Med | Med | Gating deploy check (R10): forged write must be blocked; else re-plan a token | -| Same-origin device-token install stranded | Low | Med | Drop device token only once a cookie session is obtainable (Unit 2) | -| Profiles unproven until rebase | Med | Med | Unit 8 owned by this plan's DoD; window benign (profiles not on master) | -| Token-mode re-login on expiry (no auth/validate) | Med (by design) | Med | Cross-origin only now; the session JWT persists across reloads (Option A), so re-login is on expiry, not each reload; CHANGELOG calls this out (Unit 9) | -| Same-origin password-only users lose in-app modal | Med | Med | Accepted: admin-login redirect is SK's own pattern; minority vs OIDC target | -| Version collision with `named_configs` v13 | Low | Med | Stay v12; Unit 1/5 edits overlap the v13 migration — reconcile in one rebase pass | -| Upstream rejects auto-detect default | Med | Med | Chosen deliberately; fallback is default-on toggle, not a redesign | - -## Alternative Approaches Considered - -- **Explicit mode toggle (default on for same-origin).** More upstreamable/legible, but the user chose - auto-detect, no toggle. If upstream rejects auto-detect, the fallback is the default-on toggle. -- **Thin redirect-only (keep token machinery).** Risks a JWT-after-cookie-redirect inconsistent state. -- **Proxy-layer ForwardAuth.** SK does not trust forwarded identity; Skip stays unauthenticated to the - API and profiles break. -- **Keep the custom form, fix only the password.** Does not solve the OIDC dead-end. Password fix kept - (Unit 1) but insufficient alone. -- **Decouple profiles and ship for password-users now.** User chose to keep profiles paused with this - plan owning the cookie-mode fix (Unit 8). - -## Phased Delivery - -- **Phase A (independently shippable):** Unit 1 — password removal + bootstrap-gate fix. -- **Phase B (the feature):** Units 2–7 — auto-detected cookie mode, carriage, storage decouple, - bootstrap/redirect, UX. -- **Phase C:** Unit 9 — docs + deploy-time acceptance test on halosdev. -- **Profiles rebase:** Unit 8 lands when `named_configs` rebases onto merged auth; the cookie-mode - profiles read/write check is the prerequisite's success gate. The auth merge is not "done" until - this passes (or the prerequisite is explicitly marked unproven if the rebase slips). - -## Documentation / Operational Notes - -- Deploy-time acceptance test (build locally, deploy to the device's SK webapp dir — never build on - device): served same-origin; loginStatus drives state (incl. read-only); redirect triggers OIDC and - returns without loop; cookie authenticates REST + WS + iframe with no token; CSRF forged write - blocked; config persists to the server slot; profiles (post-rebase) read/write user scope. -- `ng serve` (cross-origin `localhost:4200`) exercises token mode and all unit-level mode logic but - not the live cookie path. - -## Sources & References - -- Related plan (rebases on top, hosts Unit 8): `docs/named-configs-plan.md` -- Key code: `authentication.service.ts`, `app-initNetwork.service.ts`, `authentication-interceptor.ts`, - `signalk-delta.service.ts`, `signalk-connection.service.ts`, `storage.service.ts`, - `settings.service.ts`, `widget-login.component.ts`, `widget-freeboardsk.component.ts`, - `options/signalk/signalk.component.ts`, `options/configuration/config.component.ts` (profiles) -- SK docs: webapps auth, security spec, `loginRedirect.ts`, OIDC docs (`redirect`/`noAutoLogin`) -- Advisory: GHSA-fq56-hvg6-wvm5 - -## Review follow-ups (deferred from PR #1) - -Deferred from the multi-persona review of PR #1 (the fork has issues disabled, so tracked here + -the PR thread). Fix-now items landed on `feat/signalk-standard-auth`; these are intentionally deferred: - -- **Read-only write-control gating across the settings UI** — gate general settings write controls on - `canWriteUserData$`, not just the Connectivity identity label. Broad; overlaps Unit 8's profile - write-gating. Mitigated: the read-only state is shown and the patch queue no longer dies on a 401. -- **Cookie mid-session resilience** — on a confirmed mid-session `loginStatus` → `notLoggedIn` (cookie - expiry post-bootstrap), route through the budget-guarded `attemptCookieRedirect` to self-heal via - SSO; throttle the WS-drop `refreshLoginStatus` re-check and don't treat a transient error/429 as an - authoritative `notLoggedIn`. (The plan deferred the "401 mid-session" wire shape to deploy-time; this - is the in-code recovery path. Manual reload recovers today.) -- **Cookie anonymous-read config fallback** — on a same-origin no-security SK server - (`authenticationRequired:false`), fall back to localStorage when cookie mode is not logged in (today - `startup()` early-returns and loads nothing). HaLOS (auth-required) is unaffected. -- **Unit 7 component behavioral tests** — stub-based specs for `widget-login` and `signalk.component` - that avoid the pre-existing jsdom-localStorage component-spec gap. -- **Auth maintainability cluster** — memoize `authMode`, single owning signal for the `useServerStorage` - routing decision, shared `connectionConfig` storage-key constant. Non-behavioral. diff --git a/docs/widget-schematic.md b/docs/widget-schematic.md deleted file mode 100644 index 6dfe3598..00000000 --- a/docs/widget-schematic.md +++ /dev/null @@ -1,119 +0,0 @@ -## Host2 Widget Schematic - -Use the custom schematic to scaffold a new Host2 architecture widget. - -See also: -- Widget creation rules and the Host2 widget contract: `CLAUDE.md` - -### Command - -``` -npx schematics ./tools/schematics/collection.json:create-host2-widget \ - --name speed-over-ground \ - --title "Speed Over Ground" \ - --description "Displays SOG from navigation.speedOverGround" \ - --icon navigation-speed \ - --path-key numericPath \ - --path-description "Speed Over Ground" \ - --convert-unit-to knots \ - --sample-time 1000 \ - --register-widget Core -``` - -Only the required flags are `--name`, `--title`, `--description`, and `--icon` when running non-interactively. By default `--dry-run=false` is applied via the npm script. - -### Prompting Behavior - -The schematic now uses Angular schema `x-prompt` for both required and optional fields. - -Modes: -1. Non-interactive (default when values supplied or CLI run without `--interactive`): Only missing required fields will prompt; others use defaults. -2. Interactive (`--interactive` or `--interactive=true`): Prompts sequentially for every field (required + optional) unless you provided it on the command line. - -Flag naming: CLI flags must be kebab-case. Each dashed flag maps to camelCase schema properties (e.g. `--register-widget` → `registerWidget`, `--sample-time` → `sampleTime`). Using camelCase directly (e.g. `--registerWidget`) will fail with an "Unknown argument" error. - -Examples: -``` -# Minimal required (will prompt for any missing among required) -npm run generate:widget -- --name depth --title Depth --description "Shows depth" --icon depth-icon --register-widget Core - -# Full guided session (asks optional too) -npm run generate:widget -- --interactive --name aws --title "Apparent Wind" --description "Displays apparent wind" --icon wind --register-widget Core - -# Skip service registration -npm run generate:widget -- --name scratch --title Scratch --description "Sandbox" --icon placeholder --register-widget no -``` - -### Important Options (subset) - -| Option | Default | Notes | -| ------ | ------- | ----- | -| name | (none) | Kebab-case base name without `widget-` prefix | -| title | (none) | Display name inserted into widget definition | -| description | (none) | One-line description for registry | -| icon | (none) | Icon key placeholder (replace later) | -| category | Core | One of Core, Gauge, Component, Racing | -| pathKey (flag: --path-key) | signalKPath | Internal key used for data stream observation | -| pathDescription (flag: --path-description) | My Path | Shown in config UIs (future) | -| pathType (flag: --path-type) | number | number|string|boolean|Date | -| convertUnitTo (flag: --convert-unit-to) | unitless | Target display unit (UnitsService) | -| sampleTime (flag: --sample-time) | 1000 | ms between sampled updates | -| registerWidget (flag: --register-widget) | (none) | Category (Core/Gauge/Component/Racing) or 'no' to skip service update | -| addSpec | true | Generates a basic host test wrapper spec | -| readme | true | Emits README.md with scaffold notes | -| interactive | false | Use `--interactive` to have the CLI ask for any values you did not supply | - -### What Gets Generated - -``` -src/app/widgets/widget-/.component.ts -src/app/widgets/widget-/.component.html -src/app/widgets/widget-/.component.scss -src/app/widgets/widget-/.component.spec.ts (if --addSpec) -src/app/widgets/widget-/README.md (if --readme) -``` - -If `registerWidget` is not set to `no`, the schematic also updates `WidgetService`: -1. Adds an import for the new component. -2. Adds the component class to `_componentTypeMap`. -3. Inserts a widget definition object into `_widgetDefinition` (automatically ordered within its category). - -### After Generation Checklist - -- Replace placeholder `icon` in the widget definition with a real SVG symbol id. -- Adjust `DEFAULT_CONFIG.paths` if you need multiple paths (add entries & observers in one `untracked` block). -- Implement formatting / unit display logic using existing helper services if needed. -- Flesh out the spec with domain assertions (current spec only verifies instantiation). - -### Use Schematic vs Manual - -Use the schematic when: -1. You are creating a new standard Host2 widget. -2. You want baseline scaffold files, optional WidgetService registration, and starter test/readme output. -3. You are iterating on a known widget pattern (numeric/gauge/component style variants). - -Use manual creation when: -1. You need non-standard runtime/stream wiring that does not fit the scaffold. -2. You are building a shared utility component instead of a widget entry. -3. You are performing a targeted refactor of an existing widget where regeneration adds churn. - -Manual creation must still follow Host2 contract and project policy guidance. - -### Removing a Generated Widget - -Manual cleanup steps: -1. Delete the folder `src/app/widgets/widget-`. -2. Remove the import, map entry, and definition block for the widget from `WidgetService`. - -### Troubleshooting - -| Symptom | Cause | Fix | -| ------- | ----- | --- | -| Duplicate README like `README.md` | Older schematic version left name in filename | Delete the redundant file & upgrade schematic (already fixed) | -| Widget already exists error | Folder collision | Choose a different `--name` or delete the existing widget | -| No WidgetService update | Service path changed | Adjust `WIDGET_SERVICE` constant in schematic factory | - -### Development Notes - -- Template sources end with `.template`; a post-processing rule strips the suffix for final emission. -- Config UI generation was intentionally removed; future re-introduction would require new templates. From c65952f4c706f98bc03c08a92a654d2832efe885 Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Thu, 13 Aug 2026 11:59:48 +0300 Subject: [PATCH 3/3] chore: review at the assertive profile Lowering the profile trades away real findings to avoid a merge gate that costs one reply to clear. The instructions below are the right instrument for a reviewer that raises the wrong things. --- .coderabbit.yaml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 72dc4cde..fbf7aa32 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -8,11 +8,7 @@ tone_instructions: >- restating the diff. Silence beats a nitpick. reviews: - # `chill`, not `assertive`: main has required_conversation_resolution enabled, - # so every comment CodeRabbit leaves is a merge gate until somebody resolves - # it. A style nit that blocks a merge costs more than it finds. Raise this to - # `assertive` if that protection is ever relaxed. - profile: chill + profile: assertive poem: false in_progress_fortune: false