feat: add ServerDisplayUrl configuration option and update related logic#65
feat: add ServerDisplayUrl configuration option and update related logic#6576696265636f646572 wants to merge 1 commit intoRomainPierre7:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new configuration field to control which server URL is used in notification templates (notably {server.Url}), enabling non-localhost links in Telegram messages without affecting the server URL used for internal metadata/image lookups.
Changes:
- Introduce
ServerDisplayUrlin plugin configuration and surface it in the web config UI. - Update message placeholder replacement logic to prefer
ServerDisplayUrloverServerUrlwhen set. - Persist/load the new setting via the config page JS.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Jellyfin.Plugin.TelegramNotifier/MessageParser.cs | Uses ServerDisplayUrl (fallback to ServerUrl) for {server.Url} replacement. |
| Jellyfin.Plugin.TelegramNotifier/Configuration/Web/configPage.js | Loads/saves ServerDisplayUrl in the plugin configuration UI. |
| Jellyfin.Plugin.TelegramNotifier/Configuration/Web/configPage.html | Adds a new input field and description for ServerDisplayUrl. |
| Jellyfin.Plugin.TelegramNotifier/Configuration/PluginConfiguration.cs | Adds ServerDisplayUrl to the persisted configuration model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public string ServerUrl { get; set; } | ||
|
|
||
| public string ServerDisplayUrl { get; set; } | ||
|
|
There was a problem hiding this comment.
ServerDisplayUrl is declared as non-nullable (string) while <Nullable>enable</Nullable> is on, but it isn't initialized in the constructor. This will trigger CS8618 and can break the build; initialize it (e.g., to string.Empty) or make the property nullable (string?) and handle nulls consistently.
| <div class="fieldDescription">Insert an alternative server URL to display in notifications (ex: jellyfin.myDomain.com, | ||
| my-jellyfin.local:8096, ...). If set, this will be used in {server.Url} placeholders instead of the Server URL above. | ||
| The url must include the http/https protocol. This is useful if your server is behind a reverse proxy. |
There was a problem hiding this comment.
The description says the Server Display URL "must include the http/https protocol", but the backend logic currently auto-prefixes http:// when no scheme is present. Either update this help text to reflect the actual behavior (scheme optional, defaults to http), or adjust the backend to enforce/validate the scheme for this field to avoid surprising http:// links when users omit it.
| <div class="fieldDescription">Insert an alternative server URL to display in notifications (ex: jellyfin.myDomain.com, | |
| my-jellyfin.local:8096, ...). If set, this will be used in {server.Url} placeholders instead of the Server URL above. | |
| The url must include the http/https protocol. This is useful if your server is behind a reverse proxy. | |
| <div class="fieldDescription">Insert an alternative server URL to display in notifications (ex: https://jellyfin.myDomain.com, | |
| https://my-jellyfin.local:8096, ...). If set, this will be used in {server.Url} placeholders instead of the Server URL above. | |
| You may include the http/https protocol; if omitted, http:// will be automatically added. This is useful if your server is behind a reverse proxy. |
|
Thank you for your PR ! I’ll review it shortly and include it in the next release if it’s approved. Regards |
Server url uses forced http, added an extra config option to let the user configure the display url for the telegram messages.
Additionally, it's now possible to do something like:
Without having the server url pointing to localhost