Skip to content

feat(widget-config): always offer "Any" data source option#207

Merged
mairas merged 2 commits into
mainfrom
feat/203-any-data-source
Jul 5, 2026
Merged

feat(widget-config): always offer "Any" data source option#207
mairas merged 2 commits into
mainfrom
feat/203-any-data-source

Conversation

@mairas

@mairas mairas commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Why

In widget configuration, the Data Source dropdown behaved inconsistently depending on how many sources currently published a path:

  • One source → the only option was a literal default.
  • Two or more sourcesdefault vanished entirely; only concrete source keys were offered, and a saved default selection was silently reset (in path-control-config), forcing the user to re-pick a concrete source.

That hid default exactly when it matters most. At the data layer, source === 'default' reads the server's merged, priority-selected value (data.service.ts subscribePath) — it follows Signal K source priority and automatically fails over between sources (e.g. two GPS units, one drops, the value keeps flowing). "Accept data from any source" is its actual behavior, and that's most useful with multiple sources.

Closes #203.

What

  • Always list Any (the stored 'default' value) first, with concrete sources below, regardless of source count.
  • A freshly selected path defaults to Any; an existing selection (concrete or default) is preserved — no more silent reset when a path gains a second source.
  • Applies to both source dropdowns: widget path (path-control-config) and data-chart series (dataset-chart-options).
  • The dropdown label maps 'default' → "Any"; the stored config value stays 'default', so no migration and full back-compat with existing widgets.

Verification

@mairas

mairas commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Code review — feat(widget-config): always offer "Any" data source option

Scope: feat/203-any-data-source vs main (6 files, ~40 impl lines + templates + specs). Intent: always offer "Any" (stored value 'default') first in the Data Source dropdown regardless of source count; preserve existing selection; fresh path defaults to "Any". Applies to path-control-config and dataset-chart-options.

Reviewers: correctness, testing, maintainability, project-standards (4 always-on; no conditional personas — small diff, no auth/API/reliability/data-mutation surface).

P2 — should fix before merge

# File Issue Reviewer(s) Conf Route
1 dataset-chart-options.component.ts:138 Switching path keeps a stale source instead of "Any". changePath calls setPathSources without resetting datachartSource first (only the null-path branch resets). The new setPathSources sets 'default' only when the value is falsy, so a concrete source picked for the previous path (e.g. gps.0) survives when the user selects a new path that lacks it. The required control passes with the stale string, the select renders blank, and the saved chart subscribes to a non-existent source → null data. The old code reset to 'default' for single-source target paths, so this is a regression. path-control-config avoids it via its setValues flag; the chart component has no equivalent. correctness, maintainability 0.85 fix-here
2 path-control-config.component.spec.ts:17 let pathObject: any + eslint-disable suppresses @typescript-eslint/no-explicit-any (an error per CLAUDE.md). The sibling spec changed in this same PR types its fixture as Pick<ISkPathData, 'sources'> with no suppression. Avoidable and inconsistent. project-standards 0.72 fix-here
3 path-control-config.component.ts:225 Empty-source load branch untested. The new guard if (setValues || !sourceControl.value) has two triggers, but only setValues=true is exercised. The !value disjunct (non-fresh load with empty saved source, reachable from ngOnInit) rewrites '''default' — a behavior change with no test. The chart spec covers the equivalent case; path-control doesn't. testing 0.82 fix-here

P3 — discretionary

# File Issue Reviewer(s) Conf Route
4 path-control-config.component.ts:223 Source ordering diverges: chart sorts the concrete keys (.sort()), path-control does not. Same conceptual dropdown, two orderings. maintainability 0.72 fix-here (add .sort())
5 both .html "Any" label mapping has no DOM test. The headline user-visible change ({{source === 'default' ? 'Any' : source}}) is asserted nowhere; specs check the internal source arrays. MatSelect option rendering is awkward in this jsdom harness — noting rather than forcing a brittle test. testing 0.85 advisory
6 both .spec.ts Tests call private methods via as unknown as casts and assert internal arrays (availableSources/pathSources()) rather than driving the public triggers (ngOnInit valueChanges, changePath). Form-control-value assertions are fine; the coupling is a pragmatic tradeoff given the harness. testing 0.72 advisory

Coverage

  • Suppressed: 0 findings below the 0.60 gate.
  • Residual risk (both components): a saved concrete source that no longer exists in pathObject.sources is preserved as the form value but omitted from the options, leaving a blank-but-required select. Behavior matches old code for the multi-source case; not newly introduced except via finding Replace icon set with the SKip compass logo #1.

Verdict: Ready with fixes — address #1 (regression), #2 (lint policy), #3 (coverage), and #4 (consistency). #5/#6 are advisory.

The Data Source dropdown offered a literal "default" only when a path had
a single source, and dropped it entirely once a second source appeared —
even resetting a saved "default" selection. That hid the most useful
option (server-merged, priority-following, failover-capable value) exactly
when multiple sources make it valuable.

Always list "Any" (the stored 'default' value) first, with concrete
sources below, regardless of source count. A fresh path defaults to "Any";
an existing selection is preserved. Applies to both the widget path source
and the data-chart source. Stored config value stays 'default'; "Any" is
only the display label, so no migration is needed.

Closes #203

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mairas mairas force-pushed the feat/203-any-data-source branch from fa1e8ee to 155cd12 Compare July 4, 2026 22:38
@mairas

mairas commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Review findings addressed

Folded into the branch (amended, force-pushed) rather than a follow-up commit, since each fix revises code the same commit introduced.

# Finding Resolution
1 P2 — chart changePath kept a stale concrete source when switching paths changePath now reset()s datachartSource before setPathSources, so a fresh path falls through to "Any". The ngOnInit load path still calls setPathSources directly and keeps the saved selection. dataset-chart-options.component.ts:137
2 P2 — let pathObject: any + eslint-disable in spec Typed the fixture as Partial<ISkPathData> with a shared src() helper, matching the sibling spec; removed the suppression. path-control-config.component.spec.ts
3 P2 — empty-source load branch untested Added defaults an empty saved source to "Any" on load (setValues=false, empty value → 'default'). path-control-config.component.spec.ts
4 P3 — source ordering diverged between the two components path-control now .sort()s the concrete keys, matching the chart component. path-control-config.component.ts:223

Also added a regression test for #1 that drives the public changePath (not the private method), covering the testing reviewer's coupling note (#6) for the highest-value case: switch to a path lacking the previously selected source → value becomes "Any".

Advisory, not changed:

Verification: npm run lint clean · npm run build:prod succeeds · path-control-config.component.spec.ts 6 passed · dataset-chart-options.component.spec.ts 5 passed.

@mairas mairas enabled auto-merge July 5, 2026 08:50
@mairas mairas merged commit 6ded234 into main Jul 5, 2026
7 checks passed
@mairas mairas deleted the feat/203-any-data-source branch July 5, 2026 08:53
mairas added a commit that referenced this pull request Jul 5, 2026
main's #207 edited dataset-chart-options.component.ts; betterer keys files by
content hash, so the merge shifted that file's key while its single tracked
issue is unchanged. Refresh the committed baseline to match the merged tree so
the PR-merge CI check (branch + main) matches.

Refs #6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Widget config: replace disappearing "default" data source with an always-present "Any" option

1 participant