Closed
Conversation
andig
added
javascript
Member
|
Hi @Maschga, here a few considerations regarding the layout of this modal. We have a lot of different things to show here.
This looks very promising. Thanks for your work so far and sorry for my delayed response. 
|
naltatis
reviewed
Dec 10, 2025
naltatis
reviewed
Dec 10, 2025
naltatis
reviewed
Dec 10, 2025
…into config-ui/messaging
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
maskedTransformer.Transformerthe slice/array branch usesfor i := range nwherenis anint; this won’t compile in Go and should be rewritten as a standard counted loop (e.g.for i := 0; i < n; i++ { ... }). - The
NewEmailFromConfighelper builds the SMTP URI with atoquery parameter, while the migration logic inconfigureMessengersexpectstoAddresses; using different parameter names will break round-trips and should be aligned. - In
configureMessengers, any error returned frommigrateYamlToJsonByData(keys.Messaging, data)is ignored; consider handling or propagating it so that failed migrations don’t silently proceed tosettings.Jsonwith partially written data.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `maskedTransformer.Transformer` the slice/array branch uses `for i := range n` where `n` is an `int`; this won’t compile in Go and should be rewritten as a standard counted loop (e.g. `for i := 0; i < n; i++ { ... }`).
- The `NewEmailFromConfig` helper builds the SMTP URI with a `to` query parameter, while the migration logic in `configureMessengers` expects `toAddresses`; using different parameter names will break round-trips and should be aligned.
- In `configureMessengers`, any error returned from `migrateYamlToJsonByData(keys.Messaging, data)` is ignored; consider handling or propagating it so that failed migrations don’t silently proceed to `settings.Json` with partially written data.
## Individual Comments
### Comment 1
<location> `server/http_config_helper.go:373-382` </location>
<code_context>
}
}
+ // Preserve slices/arrays: in general don't merge them into dst, but try to merge elements when both slices are non-empty
+ if typ.Kind() == reflect.Slice || typ.Kind() == reflect.Array {
+ // If the slice elements are structs, attempt element-wise merge for non-empty slices
+ if typ.Elem().Kind() == reflect.Struct {
+ return func(dst, src reflect.Value) error {
+ if !dst.IsValid() || dst.IsNil() || !src.IsValid() || src.IsNil() {
+ return nil
+ }
+
+ if dst.Len() == 0 || src.Len() == 0 {
+ // Keep dst value, don't merge
+ return nil
+ }
+
+ // Merge element-wise up to the shorter length
+ n := min(src.Len(), dst.Len())
+
+ for i := range n {
+ de := dst.Index(i)
+ se := src.Index(i)
</code_context>
<issue_to_address>
**issue (bug_risk):** Loop over slice length using `range` on an `int` will not compile
`range` only works on iterables (slices, arrays, maps, etc.), not on `int`, so this code will not compile. Use an index-based loop instead, e.g.:
```go
n := min(src.Len(), dst.Len())
for i := 0; i < n; i++ {
de := dst.Index(i)
se := src.Index(i)
...
}
```
(or iterate over one slice’s indices and guard with `i < n`).
</issue_to_address>
### Comment 2
<location> `cmd/setup.go:921-929` </location>
<code_context>
+ // the key send is stored as string in database for frontend purposes
+ // but backend needs it as yaml map
+ if conf.Type == "custom" {
+ sendStr := props["send"].(string)
+
+ var send map[string]any
+ if err := yaml.Unmarshal([]byte(sendStr), &send); err != nil {
+ return nil, fmt.Errorf("cannot parse YAML: %w", err)
+ }
+
+ props["send"] = send
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Un-checked type assertion on `props["send"]` can panic if value is not a string
This code assumes `props["send"]` is always a `string`; if it isn’t (or config is malformed), the type assertion will panic. Use the comma-ok form and return an error instead:
```go
sendRaw, ok := props["send"].(string)
if !ok {
return nil, fmt.Errorf("custom service 'send' must be a string, got %T", props["send"])
}
```
Then unmarshal `sendRaw` as before so failures are handled gracefully instead of crashing.
</issue_to_address>
### Comment 3
<location> `assets/js/components/Config/Messaging/EventItem.vue:23` </location>
<code_context>
+ <label :for="formId('title')">{{ $t("config.messaging.eventTitle") }}</label>
+ </div>
+ <div class="col-md-10">
+ <PropertyField
+ :id="formId('title')"
+ :model-value="title"
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `@change` on `PropertyField` will not fire because the component emits `update:modelValue`
In `EventItem` you're listening for `@change`, but `PropertyField` only emits `update:modelValue` via `v-model`, so `updateTitle`/`updateMessage` are never called. Wire it to the model event instead:
```vue
<PropertyField
:model-value="title"
@update:modelValue="updateTitle"
/>
```
(or use `v-model` on `PropertyField`). Apply the same fix for the message field.
</issue_to_address>
### Comment 4
<location> `assets/js/components/Config/Messaging/Services/EmailService.vue:10-9` </location>
<code_context>
+ required
+ @update:model-value="$emit('update:host', $event)"
+ /></MessagingFormRow>
+ <MessagingFormRow :serviceType="serviceType" inputName="port" example="465">
+ <PropertyField
+ id="messagingServiceEmailPort"
+ :model-value="port"
+ type="String"
+ required
+ @update:model-value="$emit('update:port', $event)"
+ /></MessagingFormRow>
+ <MessagingFormRow :serviceType="serviceType" inputName="user" example="john.doe">
+ <PropertyField
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Email port is modeled as `number` in TS but bound via a string `PropertyField`
This creates a type mismatch: the parent (and `MessagingServiceEmail` type) expect `port: number`, but the editor always emits a string. That can lead to subtle runtime issues and ad-hoc casting.
Please either make `port` consistently a string throughout, or update `PropertyField` to use a numeric type and ensure the emitted value is parsed to `number` before propagation.
Suggested implementation:
```
<MessagingFormRow :serviceType="serviceType" inputName="port" example="465">
<PropertyField
id="messagingServiceEmailPort"
:model-value="port"
type="Number"
required
@update:model-value="$emit('update:port', Number($event))"
/></MessagingFormRow>
```
This change assumes that:
1. `PropertyField` supports `type="Number"` and still emits the raw input value (likely a string). If `PropertyField` already parses to a number when `type="Number"` is used, the `Number($event)` cast will still be safe but slightly redundant.
2. `MessagingServiceEmail.port` is typed as `number` in TypeScript; if it's `number | null` or another union, you may want to adjust the conversion logic to handle empty strings explicitly (e.g. emit `null` or `undefined` instead of `Number('')`).
If `PropertyField` does not support `type="Number"`, remove that prop and keep the `Number($event)` cast so the emitted value is still a number.
</issue_to_address>
### Comment 5
<location> `assets/js/components/Config/Messaging/Services/TelegramService.vue:26` </location>
<code_context>
+ />
+ </MessagingFormRow>
+
+ <MessagingFormRow :serviceType="serviceType" inputName="chats" example="-210987654">
+ <PropertyField
+ id="messagingServiceTelegramChats"
+ :model-value="chats"
+ property="chats"
+ type="List"
+ required
+ :rows="4"
+ @update:model-value="$emit('update:chats', $event)"
+ />
+ </MessagingFormRow>
+ </div>
+</template>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Telegram chat IDs are typed as `number[]` but edited via a generic list of strings
`chats` is typed as `number[]`, but `PropertyField` with `type="List"` typically produces `string[]`, so `@update:model-value="$emit('update:chats', $event)` will emit strings into a numeric prop.
Either convert the list items to numbers before emitting (and reject/handle invalid values), or change the type consistently to `string[]` across the prop and backend config if numeric IDs aren’t strictly required.
```suggestion
@update:model-value="$emit('update:chats', $event.map((v) => Number(v)).filter((v) => !Number.isNaN(v)))"
```
</issue_to_address>
### Comment 6
<location> `cmd/setup.go:805` </location>
<code_context>
// setup messaging
func configureMessengers(conf *globalconfig.Messaging, vehicles push.Vehicles, valueChan chan<- util.Param, cache *util.ParamCache) (chan push.Event, error) {
- // migrate settings
if settings.Exists(keys.Messaging) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the messaging migration logic and custom `send` encoding/decoding from `configureMessengers` into dedicated helper functions to keep this function focused on wiring push services at runtime.
You can keep the new behavior but reduce `configureMessengers`’ responsibility by extracting the migration logic and custom-send conversion into small helpers.
### 1. Extract messaging migration out of `configureMessengers`
Move all the YAML→JSON and per-service migration into a dedicated function that returns a fully migrated `globalconfig.Messaging`:
```go
func migrateMessagingSettings() (*globalconfig.Messaging, error) {
if !settings.Exists(keys.Messaging) {
return &globalconfig.Messaging{}, nil
}
// YAML → in-memory
if !settings.IsJson(keys.Messaging) {
var data globalconfig.Messaging
if err := settings.Yaml(keys.Messaging, new(globalconfig.Messaging), &data); err != nil {
return nil, err
}
// events already created by the user in yaml should be enabled
for k, v := range data.Events {
v.Disabled = false
data.Events[k] = v
}
for i := range data.Services {
s := &data.Services[i]
switch s.Type {
case "email":
if err := migrateEmailService(s); err != nil {
return nil, err
}
case "ntfy":
if err := migrateNtfyService(s); err != nil {
return nil, err
}
case "custom":
if err := encodeCustomSend(s); err != nil {
return nil, err
}
}
}
if err := migrateYamlToJsonByData(keys.Messaging, data); err != nil {
return nil, fmt.Errorf("failed to persist migrated messaging setting: %w", err)
}
}
var conf globalconfig.Messaging
if err := settings.Json(keys.Messaging, &conf); err != nil {
return nil, fmt.Errorf("failed to read messaging setting: %w", err)
}
return &conf, nil
}
```
Then `configureMessengers` only wires messengers:
```go
func configureMessengers(conf *globalconfig.Messaging, vehicles push.Vehicles, valueChan chan<- util.Param, cache *util.ParamCache) (chan push.Event, error) {
if settings.Exists(keys.Messaging) {
migrated, err := migrateMessagingSettings()
if err != nil {
return nil, err
}
*conf = *migrated
}
messageChan := make(chan push.Event, 1)
messageHub, err := push.NewHub(conf.Events, vehicles, cache)
if err != nil {
return messageChan, fmt.Errorf("failed configuring push services: %w", err)
}
for _, svcConf := range conf.Services {
props, err := buildMessengerProps(svcConf)
if err != nil {
return nil, err
}
impl, err := push.NewFromConfig(context.TODO(), svcConf.Type, props)
if err != nil {
return messageChan, fmt.Errorf("failed configuring push service %s: %w", svcConf.Type, err)
}
messageHub.Add(impl)
}
go messageHub.Run(messageChan, valueChan)
return messageChan, nil
}
```
### 2. Per-service migration helpers
Pull the inline `if s.Type == ...` branches into small helpers:
```go
func migrateEmailService(s *globalconfig.MessagingService) error {
uri, ok := s.Other["uri"].(string)
if !ok {
return fmt.Errorf("failed to migrate email service due to missing uri")
}
u, err := url.Parse(uri)
if err != nil {
return err
}
s.Other["host"] = u.Hostname()
s.Other["user"] = u.User.Username()
s.Other["port"] = u.Port()
s.Other["from"] = u.Query().Get("fromAddress")
addresses := u.Query()["toAddresses"]
toAddresses := make([]string, 0, len(addresses))
for _, v := range addresses {
for _, a := range strings.Split(v, ",") {
if a != "" {
toAddresses = append(toAddresses, a)
}
}
}
s.Other["to"] = toAddresses
if pw, ok := u.User.Password(); ok {
s.Other["password"] = pw
}
delete(s.Other, "uri")
return nil
}
func migrateNtfyService(s *globalconfig.MessagingService) error {
uri, ok := s.Other["uri"].(string)
if !ok {
return fmt.Errorf("failed to migrate ntfy service due to missing uri")
}
parsed, err := url.Parse(uri)
if err != nil {
return err
}
path := strings.TrimPrefix(parsed.Path, "/")
s.Other["host"] = parsed.Host
if path == "" {
s.Other["topics"] = []string{}
} else {
s.Other["topics"] = strings.Split(path, ",")
}
delete(s.Other, "uri")
return nil
}
```
### 3. Encapsulate custom `send` encoding/decoding
Replace the inline JSON/YAML manipulation for `custom` with two helpers; this lets you drop the `deepcopy.Copy` from the hot path:
```go
// during migration (persisted representation)
func encodeCustomSend(s *globalconfig.MessagingService) error {
d, err := json.Marshal(s.Other)
if err != nil {
return err
}
var other struct {
Encoding string `json:"encoding"`
Send any `json:"send"`
}
if err := json.Unmarshal(d, &other); err != nil {
return err
}
yamlSend, err := yaml.Marshal(other.Send)
if err != nil {
return err
}
s.Other["encoding"] = other.Encoding
s.Other["send"] = string(yamlSend)
return nil
}
// during runtime wiring (convert back to map for backend)
func decodeCustomSend(props map[string]any) error {
sendStr, ok := props["send"].(string)
if !ok {
return nil // or error if you expect it always to be string
}
var send map[string]any
if err := yaml.Unmarshal([]byte(sendStr), &send); err != nil {
return fmt.Errorf("cannot parse YAML: %w", err)
}
props["send"] = send
return nil
}
```
### 4. Centralize `Other` decoding and remove `deepcopy.Copy`
Create one helper that handles `customDevice` + custom-send decoding and returns a fresh map, so `configureMessengers` doesn’t need to know about `deepcopy` or the persistence-form:
```go
func buildMessengerProps(conf globalconfig.MessagingService) (map[string]any, error) {
// make a shallow copy of Other to avoid mutating config
otherCopy := make(map[string]any, len(conf.Other))
for k, v := range conf.Other {
otherCopy[k] = v
}
props, err := customDevice(otherCopy)
if err != nil {
return nil, fmt.Errorf("cannot decode push service '%s': %w", conf.Type, err)
}
if conf.Type == "custom" {
if err := decodeCustomSend(props); err != nil {
return nil, err
}
}
return props, nil
}
```
This keeps the new migration behavior, but pushes migration/persistence concerns into dedicated helpers and leaves `configureMessengers` focused on runtime wiring.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
@Maschga In-between review status. Looks really good. I went through the vue files and made them a little more DRY. Also reduced the usage of consts to improve readability. The changed spots are still type-safe. Really like the e2e tests you've added! 🎉🙌 I still have to look deeper into the go changes. @andig please also have a look at this. These changes are not trivial because the messaging structure is quite complex (compared to mqtt, infux or other existing json configs). FYI: The messaging services would be a good candidate handle as dedicated |
Member
|
@Maschga this is a lot of custom and specific new code, some of which we may be able to eliminate. What do you think of this approach:
I could help with the first task. Events would still require new configuration logic. |
Collaborator
Author
|
Okay, then let's go straight for the optimal solution. Should I open a new empty PR? |
Member
|
Let me start with 1) and the we see what can be rebased or not. I think we can do yaml or ui as with the other devices. |
6 tasks
9 tasks
Collaborator
Author
|
Closed in favor of #26946. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🖼 visualize messaging section
🧩 migration: split email-
uriintohost-,port-,user-,password-,from- andto-keys🧩 migration: split ntfy-
uriintohost- andtopics-keys🧩 migration: add
disable-key to each predefined event🔕 add toggle to enable or disable specific events
📋 add default
title- andmessage-values to each not-predefined event in the browser's language💪 use TypeScript
TODO:
Config UI card:
Events:
Add messenger:
Pushover:
Telegram:
Email:
Shout:
Ntfy:
Custom:
\cc @naltatis