feat: use human readable ws device names all places#2013
Conversation
KEGustafsson
changed the title
tkurki
left a comment
tkurki
left a comment
There was a problem hiding this comment.
This PR works for authenticated devices, right? So reflect that in the title and description.
Like for non-authenticated server you can send data to the server with echo '{"updates":[{"values":[{"path":"navigation.speedOverGround","value":3.15}]}]}' | wscat 'ws://localhost:3000/signalk/v1/stream?subscribe=none'but all you'll see on the list is ws.
|
Websocket devices, which are listed in Security/Devices/Devices list are the ones that are impacted. If device has description then that is used, fallback id. I haven't checked how things look with non-authenticated server, but need to check that too. Use case is mainly SensESP and similar sensor devices, which are listed there and produce data to server. I'll update title and description to address this feature in more precisely. |
KEGustafsson
changed the title
|
Title and description updated to highlight that PR target is to show authenticated WS devices from Security / Devices list. |
|
This work may close #1146 |
KEGustafsson
changed the title
|
Latest commit extend usage to Dashboard, Data Browser and Source Priority views, where ws devices with id is shown now with user readable names. Working without logging, non-admin user and admin user. |
New Implementation ApproachDisplay togglable, human-readable or id, for WebSocket sources in the admin UI, Data Browser and Source Priorities ChangesServer-side
UI Components
Behavior
Not included (future work)
Backwards compatibility
|
|
I want to move towards having just sourceRef in deltas and any additional metadata should be separately retrievable from |
Should this PR be just viewing/toggling of ws.* between id or description? Or should there be included meta also? |
KEGustafsson
force-pushed
the
wsDevicesWithNames
branch
2 times, most recently
from
393b0e3 to
38f52c9
Compare
KEGustafsson
force-pushed
the
wsDevicesWithNames
branch
from
38f52c9 to
320bfcd
Compare
KEGustafsson
force-pushed
the
wsDevicesWithNames
branch
from
38f52c9 to
23f30de
Compare
KEGustafsson
force-pushed
the
wsDevicesWithNames
branch
from
fdf9adc to
74beff5
Compare
* feat(sources): show human-readable names for WebSocket devices WebSocket device sources appear as cryptic refs like "ws.3d3e48a1-1185-2fe3-c494-1c1a9ee6f41f" across the Dashboard, Data Browser and Source Priorities. The device's registration description is already in the security config, just never surfaced. Expose a read-only GET /sourceNames map (ws device descriptions merged with manual aliases) that every authenticated client can read, so non-admin users see the names too. The map is cached server-side and rebuilt only when devices or aliases change, never on the per-delta path. Source-name editing stays admin-only. * fix(sources): guard /sourceNames and keep map fresh Address review feedback on the WebSocket device-names change: - Require read access for GET /sourceNames (authenticated, anonymous read-only enabled, or security disabled) so device descriptions are not exposed to anonymous clients. - Broadcast SOURCENAMES on the general serverevent channel instead of serverAdminEvent so non-admin clients receive live updates, not just the initial fetch. - Refresh the cached map when /removeSource deletes a source alias. - Default the /sourceNames fetch response to an empty object so a null/empty body cannot break source-label rendering.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds server-generated source-name maps, exposes them over HTTP and websocket updates, stores them in the admin UI, and uses them in label rendering. Alias editing is restricted for non-admin users. ChangesSource name propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/serverroutes.ts`:
- Around line 582-614: The cached source-name map is not being refreshed on the
full security-config save path, so updates made through the PUT security/config
route can leave GET /sourceNames and SOURCENAMES subscribers stale. Update the
security config save flow to invoke refreshSourceNames after saveSecurityConfig
succeeds, using the existing refreshSourceNames and computeSourceNames helpers,
and keep the current invalidation behavior in getConfigSavingCallback and the
sourceAliases handler consistent with this route.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87c1ddd8-e8b7-4fe1-ba15-408101af1c78
📒 Files selected for processing (13)
packages/server-admin-ui/src/dataFetching.tspackages/server-admin-ui/src/hooks/useSourceAliases.tspackages/server-admin-ui/src/services/WebSocketService.tspackages/server-admin-ui/src/store/index.tspackages/server-admin-ui/src/store/slices/appSlice.tspackages/server-admin-ui/src/utils/sourceLabels.test.tspackages/server-admin-ui/src/utils/sourceLabels.tspackages/server-admin-ui/src/views/Dashboard/Dashboard.tsxpackages/server-admin-ui/src/views/DataBrowser/DataBrowser.tsxpackages/server-admin-ui/src/views/DataBrowser/SourceLabel.tsxsrc/serverroutes.tssrc/sourceNames.tstest/sourceNames.ts
…89) The PUT security/config route saves the complete security config, which can change device descriptions, but did not refresh the cached source-name map. This left GET /sourceNames and SOURCENAMES subscribers stale until another device or alias change occurred. Invoke refreshSourceNames after a successful save, matching the device routes and the sourceAliases handler. Claude-Session: https://claude.ai/code/session_018SBdX6xr1Hxn8Dam7N4AKu Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/serverroutes.ts (1)
608-615: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh source names when priority reset clears aliases.
buildSourceNames()includessourceAliases, andDELETE /prioritiesemitsSOURCEALIASESas{}after reset, but this cache is not refreshed there. Clients can keep stale alias-derived names until another alias/device change occurs.Suggested fix
app.emit('serverAdminEvent', { type: 'SOURCEALIASES', data: {} }) + refreshSourceNames() app.emit('serverAdminEvent', { type: 'PRIORITYGROUPS', data: []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serverroutes.ts` around lines 608 - 615, The source-name cache in refreshSourceNames() is not updated when DELETE /priorities clears aliases, so clients can keep stale alias-derived names. Update the priorities reset flow in the DELETE /priorities handler to call refreshSourceNames() after emitting SOURCEALIASES, ensuring cachedSourceNames is rebuilt from buildSourceNames() and the serverevent S0URCENAMES update is sent immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/serverroutes.ts`:
- Around line 608-615: The source-name cache in refreshSourceNames() is not
updated when DELETE /priorities clears aliases, so clients can keep stale
alias-derived names. Update the priorities reset flow in the DELETE /priorities
handler to call refreshSourceNames() after emitting SOURCEALIASES, ensuring
cachedSourceNames is rebuilt from buildSourceNames() and the serverevent
S0URCENAMES update is sent immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef4bf8f0-ec36-4cc5-9322-566a031d1b7d
📒 Files selected for processing (1)
src/serverroutes.ts
DELETE /priorities clears sourceAliases but the cached source-name map was not rebuilt, so clients kept stale alias-derived names. Call refreshSourceNames() after emitting the empty SOURCEALIASES, matching the other alias-mutating handlers. Claude-Session: https://claude.ai/code/session_015idCKNy7E5qHFVYu5DzLXE Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/serverroutes.ts (1)
592-598: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHonor
showDeviceLabelNameswhen building read-only names.
computeSourceNames()always passes security devices tobuildSourceNames, so device descriptions can still appear in/sourceNamesandSOURCENAMESeven when the label-name toggle is off. Strip/omit device descriptions before building this read-only map; keep manual aliases if those should remain explicit.Suggested fix
const computeSourceNames = (): Record<string, string> => { const config = getSecurityConfig(app) + const includeDeviceLabelNames = config.showDeviceLabelNames !== false const devices = - typeof app.securityStrategy.getDevices === 'function' + includeDeviceLabelNames && + typeof app.securityStrategy.getDevices === 'function' ? app.securityStrategy.getDevices(config) : [] return buildSourceNames(devices, app.config.settings.sourceAliases || {}) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serverroutes.ts` around lines 592 - 598, `computeSourceNames()` is always forwarding security devices into `buildSourceNames`, so device descriptions can leak into the read-only source-name output even when `showDeviceLabelNames` is disabled. Update `computeSourceNames()` in `serverroutes.ts` to respect the toggle by stripping or omitting device descriptions before calling `buildSourceNames`, while still preserving any explicit manual aliases from `app.config.settings.sourceAliases`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/serverroutes.ts`:
- Around line 592-598: `computeSourceNames()` is always forwarding security
devices into `buildSourceNames`, so device descriptions can leak into the
read-only source-name output even when `showDeviceLabelNames` is disabled.
Update `computeSourceNames()` in `serverroutes.ts` to respect the toggle by
stripping or omitting device descriptions before calling `buildSourceNames`,
while still preserving any explicit manual aliases from
`app.config.settings.sourceAliases`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a3fc4fc-7ec4-4a29-a9f1-e572225f9975
📒 Files selected for processing (1)
src/serverroutes.ts
Claude-Session: https://claude.ai/code/session_01Cuhwwm64H7o3Em4Ud3g37m Co-authored-by: Claude <noreply@anthropic.com>
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
The Comment image shows the same style of “Connections activity” list, again with defaults, udp7000, and SensoSP device: sensesp-engines-test rendered as clickable blue labels, reinforcing the intended UX of showing readable source/device names in the UI. You’re right — thanks for the clarification. That finding was based on an incorrect assumption about a I’m withdrawing that comment. ✏️ Learnings added
|
3 participants
New implementation to add authenticated
WS devicesdescription as a name to Dashboard.This fixes issues that #1705 had related for non-admin use cases.
Question for fine tuning of this PR is, should
ws devicesdata be part ofSERVERSTATISTICSorPROVIDERSTATUSfeed?Current implementation is adding it to
SERVERSTATISTICSin deltastats.ts. It could be changed to index.ts and part ofPROVIDERSTATUS.This PR adds server-supplied human-readable source names for authenticated WebSocket devices and wires them through the admin UI.
Server:
src/sourceNames.tsto build aRecord<sourceRef, name>from authenticated WebSocket devicedescriptionvalues (ws.<clientId>, with.normalized to_) merged with administratorsourceAliases(manual aliases take precedence).GET /sourceNamesendpoint insrc/serverroutes.tsthat returns403when security is enabled and the requester is neither authenticated nor allowed read-only.refreshSourceNames()and broadcasts live updates with aservereventof typeSOURCENAMESwhenever security configuration/alias-related data changes (including security config saves andsourceAliasespersistence).Client (server-admin-ui):
/sourceNamesduring initial data loading indataFetching.tsand stores it in Zustand (appSlice.sourceNames), exposed via auseSourceNames()selector.SOURCENAMESevents arrive inWebSocketService.sourceNameswhen building display labels inuseSourceAliasesandbuildSourceLabel/buildSourceLabelParts, preferringsourceNames[sourceRef]for WS refs when present (falling back to the rawsourceRefotherwise, while also overriding other derived labels such as N2K manufacturer/model).sourceNameswhen rendering connection/provider links.sourceNamesintobuildSourceLabelfor source labels.SourceLabelkeeps alias editing read-only for non-admin users (admins keep the existing editing UI).sourceLabels.test.tscoverage for WS label formatting, precedence, and fallback behavior.