From a9f35600dd33a72b0a12f4ea8f2722e8c9d72820 Mon Sep 17 00:00:00 2001 From: nicomiguelino Date: Mon, 24 Aug 2026 10:26:01 -0700 Subject: [PATCH 1/5] feat: order Edge App settings as declared in the manifest Settings previously lost their declaration order on the way to the web UI: parsing went through a HashMap and both the CLI and the API normalized everything to alphabetical order by name, and the diffing logic required both sides pre-sorted to merge correctly. - Preserve manifest declaration order when parsing settings instead of sorting alphabetically. - Rework detect_changed_settings to diff by name lookup instead of a sorted merge, so creates/updates are emitted in manifest order. - Since the backend has no dedicated ordering column for settings, automatically compute a `priority` from each setting's position and embed it in help_text's structured JSON schema on every deploy, persisting it back to the manifest file. Settings whose name is resolved via the web app's hardcoded FIELD_OVERRIDES table are left untouched, since turning their help_text into JSON would otherwise silently drop that override. --- src/api/edge_app/setting.rs | 86 +++++++++++++++++++++++++++---- src/commands/edge_app/app.rs | 40 +++++++++----- src/commands/edge_app/manifest.rs | 4 ++ src/commands/edge_app/utils.rs | 56 +++++++++----------- 4 files changed, 133 insertions(+), 53 deletions(-) diff --git a/src/api/edge_app/setting.rs b/src/api/edge_app/setting.rs index a749fc59..f5750fd9 100644 --- a/src/api/edge_app/setting.rs +++ b/src/api/edge_app/setting.rs @@ -73,14 +73,29 @@ pub fn deserialize_settings<'de, D>(deserializer: D) -> Result, D:: where D: Deserializer<'de>, { - let map: HashMap = serde::Deserialize::deserialize(deserializer)?; - let mut settings: Vec = map - .into_iter() - .map(|(name, mut setting)| { - setting.name = name; - setting - }) - .collect(); + struct SettingsVisitor; + + impl<'de> serde::de::Visitor<'de> for SettingsVisitor { + type Value = Vec; + + fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + formatter.write_str("a map of settings") + } + + fn visit_map(self, mut map: A) -> Result + where + A: serde::de::MapAccess<'de>, + { + let mut settings = Vec::new(); + while let Some((name, mut setting)) = map.next_entry::()? { + setting.name = name; + settings.push(setting); + } + Ok(settings) + } + } + + let settings = deserializer.deserialize_map(SettingsVisitor)?; for setting in &settings { if setting.type_ == SettingType::Secret && setting.default_value.is_some() { @@ -97,7 +112,6 @@ where } } - settings.sort_by_key(|s| s.name.clone()); Ok(settings) } @@ -226,6 +240,60 @@ where } } +const HELP_TEXT_NAME_OVERRIDES: &[&str] = &[ + "message_body", + "rss_url", + "bypass_cors", + "cache_interval", + "limit", + "override_coordinates", + "override_locale", + "override_timezone", + "target_timestamp", + "stop_id", + "azure_ad_scope", + "azure_ad_resource", + "theme", +]; + +pub fn help_text_with_priority(name: &str, help_text: &str, priority: usize) -> String { + if HELP_TEXT_NAME_OVERRIDES.contains(&name) { + return help_text.to_string(); + } + + let mut value: Value = serde_json::from_str(help_text).unwrap_or(Value::Null); + + let is_schema = value + .as_object() + .map(|obj| { + obj.contains_key("schema_version") + && obj.get("properties").is_some_and(Value::is_object) + }) + .unwrap_or(false); + + if is_schema { + if let Some(properties) = value.get_mut("properties").and_then(Value::as_object_mut) { + properties.insert("priority".to_string(), json!(priority)); + } + return serde_json::to_string(&value).unwrap_or_else(|_| help_text.to_string()); + } + + json!({ + "schema_version": 1, + "properties": { + "help_text": help_text, + "priority": priority, + } + }) + .to_string() +} + +pub fn assign_setting_priorities(settings: &mut [Setting]) { + for (priority, setting) in settings.iter_mut().enumerate() { + setting.help_text = help_text_with_priority(&setting.name, &setting.help_text, priority); + } +} + impl Setting { pub fn new(type_: SettingType, title: &str, name: &str, help_text: &str, global: bool) -> Self { Setting { diff --git a/src/commands/edge_app/app.rs b/src/commands/edge_app/app.rs index d5ed98d4..bef935a8 100644 --- a/src/commands/edge_app/app.rs +++ b/src/commands/edge_app/app.rs @@ -210,7 +210,9 @@ impl EdgeAppCommand { let manifest_path = transform_edge_app_path_to_manifest(&path)?; EdgeAppManifest::ensure_manifest_is_valid(&manifest_path)?; - let manifest = EdgeAppManifest::new(&manifest_path)?; + let mut manifest = EdgeAppManifest::new(&manifest_path)?; + manifest.assign_setting_priorities(); + EdgeAppManifest::save_to_file(&manifest, &manifest_path)?; let actual_app_id = match self.get_app_id(path.clone()) { Ok(id) => id, @@ -756,16 +758,6 @@ mod tests { assert_eq!( manifest.settings, vec![ - Setting { - name: "greeting".to_string(), - title: Some("greeting title".to_string()), - type_: SettingType::String, - default_value: Some("Unknown".to_string()), - optional: true, - is_global: false, - help_text: "An example of a string setting that is used in index.html" - .to_string(), - }, Setting { name: "secret_word".to_string(), title: Some("secret title".to_string()), @@ -775,6 +767,16 @@ mod tests { is_global: false, help_text: "An example of a secret setting that is used in index.html" .to_string(), + }, + Setting { + name: "greeting".to_string(), + title: Some("greeting title".to_string()), + type_: SettingType::String, + default_value: Some("Unknown".to_string()), + optional: true, + is_global: false, + help_text: "An example of a string setting that is used in index.html" + .to_string(), } ] ); @@ -1189,7 +1191,13 @@ mod tests { "default_value": "", "title": "atitle", "optional": false, - "help_text": "help text", + "help_text": { + "schema_version": 1, + "properties": { + "help_text": "help text", + "priority": 0, + }, + }, })); then.status(201).json_body(json!( [{ @@ -1219,7 +1227,13 @@ mod tests { "default_value": "", "title": "ntitle", "optional": false, - "help_text": "help text", + "help_text": { + "schema_version": 1, + "properties": { + "help_text": "help text", + "priority": 1, + }, + }, })); then.status(200).json_body(json!( [{ diff --git a/src/commands/edge_app/manifest.rs b/src/commands/edge_app/manifest.rs index 3fb61463..df154eaf 100644 --- a/src/commands/edge_app/manifest.rs +++ b/src/commands/edge_app/manifest.rs @@ -276,6 +276,10 @@ impl EdgeAppManifest { Ok(()) } + pub fn assign_setting_priorities(&mut self) { + crate::api::edge_app::setting::assign_setting_priorities(&mut self.settings); + } + pub fn prepare_payload(manifest: &EdgeAppManifest) -> HashMap<&str, serde_json::Value> { let entrypoint_uri = match &manifest.entrypoint { Some(entrypoint) => entrypoint.uri.clone(), diff --git a/src/commands/edge_app/utils.rs b/src/commands/edge_app/utils.rs index 5b78aa46..e785550c 100644 --- a/src/commands/edge_app/utils.rs +++ b/src/commands/edge_app/utils.rs @@ -6,7 +6,7 @@ use log::debug; use walkdir::{DirEntry, WalkDir}; use crate::api::asset::AssetSignature; -use crate::api::edge_app::setting::{Setting, SettingType}; +use crate::api::edge_app::setting::{assign_setting_priorities, Setting, SettingType}; use crate::commands::edge_app::instance_manifest::InstanceManifest; use crate::commands::edge_app::manifest::EdgeAppManifest; use crate::commands::ignorer::Ignorer; @@ -190,11 +190,6 @@ pub fn detect_changed_settings( manifest: &EdgeAppManifest, remote_settings: &[Setting], ) -> Result { - // Remote and local settings MUST be sorted. - // This function compares remote and local settings - // And returns if there are any new local settings missing from the remote - // And changed settings to update - let mut new_settings = manifest.settings.clone(); if let Some(auth) = &manifest.auth { @@ -226,37 +221,35 @@ pub fn detect_changed_settings( } } - new_settings.sort_by_key(|s| s.name.clone()); + assign_setting_priorities(&mut new_settings); + + let remote_by_name: HashMap<&str, &Setting> = remote_settings + .iter() + .map(|setting| (setting.name.as_str(), setting)) + .collect(); + let new_names: HashSet<&str> = new_settings.iter().map(|s| s.name.as_str()).collect(); let mut creates = Vec::new(); let mut updates = Vec::new(); - let mut deleted: Vec = Vec::new(); - - let mut remote_iter = remote_settings.iter().peekable(); - let mut new_iter = new_settings.iter().peekable(); - while let (Some(&remote_setting), Some(&new_setting)) = (remote_iter.peek(), new_iter.peek()) { - match remote_setting.name.cmp(&new_setting.name) { - std::cmp::Ordering::Equal => { + for new_setting in &new_settings { + match remote_by_name.get(new_setting.name.as_str()) { + Some(&remote_setting) => { if remote_setting != new_setting { updates.push(new_setting.clone()); } - remote_iter.next(); - new_iter.next(); } - std::cmp::Ordering::Less => { - deleted.push(remote_setting.clone()); - remote_iter.next(); - } - std::cmp::Ordering::Greater => { + None => { creates.push(new_setting.clone()); - new_iter.next(); } } } - creates.extend(new_iter.cloned()); - deleted.extend(remote_iter.cloned()); + let deleted: Vec = remote_settings + .iter() + .filter(|setting| !new_names.contains(setting.name.as_str())) + .cloned() + .collect(); Ok(SettingChanges { creates, @@ -317,7 +310,7 @@ mod tests { use tempfile::tempdir; use super::*; - use crate::api::edge_app::setting::{Setting, SettingType}; + use crate::api::edge_app::setting::{help_text_with_priority, Setting, SettingType}; use crate::commands::edge_app::instance_manifest::INSTANCE_MANIFEST_VERSION; use crate::commands::edge_app::manifest::{Auth, Entrypoint, EntrypointType, MANIFEST_VERSION}; use crate::commands::edge_app::manifest_auth::AuthType; @@ -374,7 +367,7 @@ mod tests { title: Some("display time title".to_string()), optional: true, is_global: false, - help_text: "For how long to display the map overlay every time the rover has moved to a new position.".to_string(), + help_text: help_text_with_priority("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), }, Setting { name: "google_maps_api_key".to_string(), @@ -383,7 +376,7 @@ mod tests { title: Some("Google maps title".to_string()), optional: true, is_global: false, - help_text: "Specify a commercial Google Maps API key. Required due to the app's map feature.".to_string(), + help_text: help_text_with_priority("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), }, ]; @@ -394,6 +387,7 @@ mod tests { assert!(result.is_ok()); let changes = result.unwrap(); assert_eq!(changes.creates.len(), 0); + assert_eq!(changes.updates.len(), 0); } #[test] @@ -408,7 +402,7 @@ mod tests { title: None, optional: true, is_global: false, - help_text: "For how long to display the map overlay every time the rover has moved to a new position.".to_string(), + help_text: help_text_with_priority("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), }, Setting { name: "google_maps_api_key".to_string(), @@ -417,7 +411,7 @@ mod tests { title: Some("Google maps title".to_string()), optional: true, is_global: false, - help_text: "Specify a commercial Google Maps API key. Required due to the app's map feature.".to_string(), + help_text: help_text_with_priority("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), }, ]; @@ -516,7 +510,7 @@ mod tests { title: Some("display time title".to_string()), optional: true, is_global: false, - help_text: "For how long to display the map overlay every time the rover has moved to a new position.".to_string(), + help_text: help_text_with_priority("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), }, Setting { name: "google_maps_api_key".to_string(), @@ -525,7 +519,7 @@ mod tests { title: Some("Google maps title".to_string()), optional: true, is_global: false, - help_text: "Specify a commercial Google Maps API key. Required due to the app's map feature.".to_string(), + help_text: help_text_with_priority("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), }, ]; From 0ab3a9067d4bfe9ef4a463bc56ae5b4871b3f28d Mon Sep 17 00:00:00 2001 From: nicomiguelino Date: Mon, 24 Aug 2026 15:35:06 -0700 Subject: [PATCH 2/5] docs: document priority-based setting ordering --- docs/EdgeApps.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/docs/EdgeApps.md b/docs/EdgeApps.md index 3b307240..9713597b 100644 --- a/docs/EdgeApps.md +++ b/docs/EdgeApps.md @@ -483,6 +483,7 @@ Edge App settings support additional input field types beyond plain text and pas - `properties.type`: One of `datetime`, `number`, `select`, `boolean`, `textarea`, `url`. - `properties.help_text`: Human-friendly description shown in the UI. - `properties.options` (only for `select`): Array of `{ label, value }` options. + - `properties.priority`: Optional integer controlling the order settings render in the install/edit UI (ascending). If omitted, `screenly edge-app deploy` auto-assigns one from the setting's position in the manifest's `settings:` mapping, so settings render in declaration order by default — set an explicit value only to override that default. - **Storage**: Use `type: string` for all non-secret fields; use `type: secret` for password-like fields. The UI will coerce values appropriately (e.g., booleans) but values are stored as strings unless `type: secret`. - **Defaults**: Provide `default_value` at the setting level. For booleans, use `'true'` or `'false'` as strings. @@ -588,6 +589,32 @@ settings: type: url ``` +**Explicit display order override** + +```yaml +settings: + number_field: + type: string + title: Attendee Count + optional: false + help_text: + schema_version: 1 + properties: + help_text: The expected count of attendees + type: number + priority: 2 + date_time_field: + type: string + title: Start Date Time + optional: false + help_text: + schema_version: 1 + properties: + help_text: The start date and time of the event + type: datetime + priority: 1 +``` + Notes: - These descriptors are backward-compatible; if no JSON is provided, the UI falls back to a plain text field for `string` and a password field for `secret`. From 48add2510ca16217f4fb0da99e888d0feec86907 Mon Sep 17 00:00:00 2001 From: Nico Miguelino Date: Tue, 25 Aug 2026 02:00:33 -0700 Subject: [PATCH 3/5] Rename setting priority field to display_order (#309) --- docs/EdgeApps.md | 6 +++--- src/api/edge_app/setting.rs | 13 +++++++------ src/commands/edge_app/app.rs | 6 +++--- src/commands/edge_app/manifest.rs | 4 ++-- src/commands/edge_app/utils.rs | 18 +++++++++--------- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/docs/EdgeApps.md b/docs/EdgeApps.md index 9713597b..09d9f170 100644 --- a/docs/EdgeApps.md +++ b/docs/EdgeApps.md @@ -483,7 +483,7 @@ Edge App settings support additional input field types beyond plain text and pas - `properties.type`: One of `datetime`, `number`, `select`, `boolean`, `textarea`, `url`. - `properties.help_text`: Human-friendly description shown in the UI. - `properties.options` (only for `select`): Array of `{ label, value }` options. - - `properties.priority`: Optional integer controlling the order settings render in the install/edit UI (ascending). If omitted, `screenly edge-app deploy` auto-assigns one from the setting's position in the manifest's `settings:` mapping, so settings render in declaration order by default — set an explicit value only to override that default. + - `properties.display_order`: Optional integer controlling the order settings render in the install/edit UI (ascending). If omitted, `screenly edge-app deploy` auto-assigns one from the setting's position in the manifest's `settings:` mapping, so settings render in declaration order by default — set an explicit value only to override that default. - **Storage**: Use `type: string` for all non-secret fields; use `type: secret` for password-like fields. The UI will coerce values appropriately (e.g., booleans) but values are stored as strings unless `type: secret`. - **Defaults**: Provide `default_value` at the setting level. For booleans, use `'true'` or `'false'` as strings. @@ -602,7 +602,7 @@ settings: properties: help_text: The expected count of attendees type: number - priority: 2 + display_order: 2 date_time_field: type: string title: Start Date Time @@ -612,7 +612,7 @@ settings: properties: help_text: The start date and time of the event type: datetime - priority: 1 + display_order: 1 ``` Notes: diff --git a/src/api/edge_app/setting.rs b/src/api/edge_app/setting.rs index f5750fd9..91c2852f 100644 --- a/src/api/edge_app/setting.rs +++ b/src/api/edge_app/setting.rs @@ -256,7 +256,7 @@ const HELP_TEXT_NAME_OVERRIDES: &[&str] = &[ "theme", ]; -pub fn help_text_with_priority(name: &str, help_text: &str, priority: usize) -> String { +pub fn help_text_with_display_order(name: &str, help_text: &str, display_order: usize) -> String { if HELP_TEXT_NAME_OVERRIDES.contains(&name) { return help_text.to_string(); } @@ -273,7 +273,7 @@ pub fn help_text_with_priority(name: &str, help_text: &str, priority: usize) -> if is_schema { if let Some(properties) = value.get_mut("properties").and_then(Value::as_object_mut) { - properties.insert("priority".to_string(), json!(priority)); + properties.insert("display_order".to_string(), json!(display_order)); } return serde_json::to_string(&value).unwrap_or_else(|_| help_text.to_string()); } @@ -282,15 +282,16 @@ pub fn help_text_with_priority(name: &str, help_text: &str, priority: usize) -> "schema_version": 1, "properties": { "help_text": help_text, - "priority": priority, + "display_order": display_order, } }) .to_string() } -pub fn assign_setting_priorities(settings: &mut [Setting]) { - for (priority, setting) in settings.iter_mut().enumerate() { - setting.help_text = help_text_with_priority(&setting.name, &setting.help_text, priority); +pub fn assign_setting_display_orders(settings: &mut [Setting]) { + for (display_order, setting) in settings.iter_mut().enumerate() { + setting.help_text = + help_text_with_display_order(&setting.name, &setting.help_text, display_order); } } diff --git a/src/commands/edge_app/app.rs b/src/commands/edge_app/app.rs index bef935a8..c2f8dd35 100644 --- a/src/commands/edge_app/app.rs +++ b/src/commands/edge_app/app.rs @@ -211,7 +211,7 @@ impl EdgeAppCommand { EdgeAppManifest::ensure_manifest_is_valid(&manifest_path)?; let mut manifest = EdgeAppManifest::new(&manifest_path)?; - manifest.assign_setting_priorities(); + manifest.assign_setting_display_orders(); EdgeAppManifest::save_to_file(&manifest, &manifest_path)?; let actual_app_id = match self.get_app_id(path.clone()) { @@ -1195,7 +1195,7 @@ mod tests { "schema_version": 1, "properties": { "help_text": "help text", - "priority": 0, + "display_order": 0, }, }, })); @@ -1231,7 +1231,7 @@ mod tests { "schema_version": 1, "properties": { "help_text": "help text", - "priority": 1, + "display_order": 1, }, }, })); diff --git a/src/commands/edge_app/manifest.rs b/src/commands/edge_app/manifest.rs index df154eaf..b6d3bb30 100644 --- a/src/commands/edge_app/manifest.rs +++ b/src/commands/edge_app/manifest.rs @@ -276,8 +276,8 @@ impl EdgeAppManifest { Ok(()) } - pub fn assign_setting_priorities(&mut self) { - crate::api::edge_app::setting::assign_setting_priorities(&mut self.settings); + pub fn assign_setting_display_orders(&mut self) { + crate::api::edge_app::setting::assign_setting_display_orders(&mut self.settings); } pub fn prepare_payload(manifest: &EdgeAppManifest) -> HashMap<&str, serde_json::Value> { diff --git a/src/commands/edge_app/utils.rs b/src/commands/edge_app/utils.rs index e785550c..77265f4b 100644 --- a/src/commands/edge_app/utils.rs +++ b/src/commands/edge_app/utils.rs @@ -6,7 +6,7 @@ use log::debug; use walkdir::{DirEntry, WalkDir}; use crate::api::asset::AssetSignature; -use crate::api::edge_app::setting::{assign_setting_priorities, Setting, SettingType}; +use crate::api::edge_app::setting::{assign_setting_display_orders, Setting, SettingType}; use crate::commands::edge_app::instance_manifest::InstanceManifest; use crate::commands::edge_app::manifest::EdgeAppManifest; use crate::commands::ignorer::Ignorer; @@ -221,7 +221,7 @@ pub fn detect_changed_settings( } } - assign_setting_priorities(&mut new_settings); + assign_setting_display_orders(&mut new_settings); let remote_by_name: HashMap<&str, &Setting> = remote_settings .iter() @@ -310,7 +310,7 @@ mod tests { use tempfile::tempdir; use super::*; - use crate::api::edge_app::setting::{help_text_with_priority, Setting, SettingType}; + use crate::api::edge_app::setting::{help_text_with_display_order, Setting, SettingType}; use crate::commands::edge_app::instance_manifest::INSTANCE_MANIFEST_VERSION; use crate::commands::edge_app::manifest::{Auth, Entrypoint, EntrypointType, MANIFEST_VERSION}; use crate::commands::edge_app::manifest_auth::AuthType; @@ -367,7 +367,7 @@ mod tests { title: Some("display time title".to_string()), optional: true, is_global: false, - help_text: help_text_with_priority("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), + help_text: help_text_with_display_order("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), }, Setting { name: "google_maps_api_key".to_string(), @@ -376,7 +376,7 @@ mod tests { title: Some("Google maps title".to_string()), optional: true, is_global: false, - help_text: help_text_with_priority("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), + help_text: help_text_with_display_order("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), }, ]; @@ -402,7 +402,7 @@ mod tests { title: None, optional: true, is_global: false, - help_text: help_text_with_priority("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), + help_text: help_text_with_display_order("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), }, Setting { name: "google_maps_api_key".to_string(), @@ -411,7 +411,7 @@ mod tests { title: Some("Google maps title".to_string()), optional: true, is_global: false, - help_text: help_text_with_priority("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), + help_text: help_text_with_display_order("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), }, ]; @@ -510,7 +510,7 @@ mod tests { title: Some("display time title".to_string()), optional: true, is_global: false, - help_text: help_text_with_priority("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), + help_text: help_text_with_display_order("display_time", "For how long to display the map overlay every time the rover has moved to a new position.", 0), }, Setting { name: "google_maps_api_key".to_string(), @@ -519,7 +519,7 @@ mod tests { title: Some("Google maps title".to_string()), optional: true, is_global: false, - help_text: help_text_with_priority("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), + help_text: help_text_with_display_order("google_maps_api_key", "Specify a commercial Google Maps API key. Required due to the app's map feature.", 1), }, ]; From f4692cc79366307d5271909dd3c637296c80d656 Mon Sep 17 00:00:00 2001 From: nicomiguelino Date: Tue, 25 Aug 2026 10:43:29 -0700 Subject: [PATCH 4/5] Fix setting display_order review issues from PR #308 - Preserve an explicitly authored display_order instead of always overwriting it with the positional index. - Stop deploy from rewriting the manifest file; ordering is computed only for the upload payload in detect_changed_settings. - Reject duplicate setting names during manifest parsing instead of silently keeping the last one. - Replace an expect() on help_text with a graceful string conversion so a structured value from the API can't panic the process. - Skip display_order assignment for screenly_* settings, secrets without an existing descriptor, and names in HELP_TEXT_NAME_OVERRIDES, and warn when settings are skipped. - Preserve non-schema JSON objects (and their other keys) instead of stringifying them into their own help text. - Update docs/EdgeApps.md accordingly. --- docs/EdgeApps.md | 6 +- src/api/edge_app/setting.rs | 252 ++++++++++++++++++++++++++---- src/commands/edge_app/app.rs | 4 +- src/commands/edge_app/manifest.rs | 4 - 4 files changed, 230 insertions(+), 36 deletions(-) diff --git a/docs/EdgeApps.md b/docs/EdgeApps.md index 09d9f170..30e7ac18 100644 --- a/docs/EdgeApps.md +++ b/docs/EdgeApps.md @@ -483,7 +483,7 @@ Edge App settings support additional input field types beyond plain text and pas - `properties.type`: One of `datetime`, `number`, `select`, `boolean`, `textarea`, `url`. - `properties.help_text`: Human-friendly description shown in the UI. - `properties.options` (only for `select`): Array of `{ label, value }` options. - - `properties.display_order`: Optional integer controlling the order settings render in the install/edit UI (ascending). If omitted, `screenly edge-app deploy` auto-assigns one from the setting's position in the manifest's `settings:` mapping, so settings render in declaration order by default — set an explicit value only to override that default. + - `properties.display_order`: Optional integer controlling the order settings render in the install/edit UI (ascending). If omitted, `screenly edge-app deploy` auto-assigns one from the setting's position in the manifest's `settings:` mapping, so settings render in declaration order by default. Set an explicit value only to override that default. An explicitly authored `display_order` is never overwritten by the automatic assignment. Note that `deploy` only sends the computed order to the backend; it never rewrites your manifest file. - **Storage**: Use `type: string` for all non-secret fields; use `type: secret` for password-like fields. The UI will coerce values appropriately (e.g., booleans) but values are stored as strings unless `type: secret`. - **Defaults**: Provide `default_value` at the setting level. For booleans, use `'true'` or `'false'` as strings. @@ -618,6 +618,10 @@ settings: Notes: - These descriptors are backward-compatible; if no JSON is provided, the UI falls back to a plain text field for `string` and a password field for `secret`. +- Some settings are excluded from the automatic `display_order` assignment and are left to the UI's default ordering. `deploy` prints a warning naming any setting it skipped. The exclusions are: + - Settings of `type: secret` that do not already provide a descriptor, so that the UI's password-field fallback keeps applying. To order a secret, give it an explicit descriptor with a `display_order`. + - Internal `screenly_*` settings generated by the CLI for `auth:` and remote entrypoints. They are not declared in the manifest, so they have no authored position. + - A small set of setting names whose help text is managed outside of the CLI: `azure_ad_resource`, `azure_ad_scope`, `bypass_cors`, `cache_interval`, `limit`, `message_body`, `override_coordinates`, `override_locale`, `override_timezone`, `rss_url`, `stop_id`, `target_timestamp`, and `theme`. #### Integrations diff --git a/src/api/edge_app/setting.rs b/src/api/edge_app/setting.rs index 91c2852f..552c31ef 100644 --- a/src/api/edge_app/setting.rs +++ b/src/api/edge_app/setting.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::ops::Not; use std::str::FromStr; @@ -12,6 +12,8 @@ use crate::api::Api; use crate::commands; use crate::commands::{CommandError, EdgeAppSettings}; +const SETTING_HELP_TEXT_SCHEMA_VERSION: u32 = 1; + #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] pub struct SettingValue { name: String, @@ -97,7 +99,15 @@ where let settings = deserializer.deserialize_map(SettingsVisitor)?; + let mut seen_names: HashSet<&str> = HashSet::new(); + for setting in &settings { + if !seen_names.insert(setting.name.as_str()) { + return Err(serde::de::Error::custom(format!( + "Setting \"{}\" is declared more than once. Each setting name must be unique.", + setting.name + ))); + } if setting.type_ == SettingType::Secret && setting.default_value.is_some() { return Err(serde::de::Error::custom(format!( "Setting \"{}\" is of type \"secret\" and cannot have a default value", @@ -140,10 +150,10 @@ where setting.optional = value.as_bool().expect("Failed to parse optional.") } "help_text" => { - setting.help_text = value - .as_str() - .expect("Failed to parse help_text.") - .to_string(); + setting.help_text = match value { + Value::String(help_text) => help_text, + other => other.to_string(), + }; } "is_global" => { setting.is_global = value.as_bool().expect("Failed to parse is_global."); @@ -256,43 +266,73 @@ const HELP_TEXT_NAME_OVERRIDES: &[&str] = &[ "theme", ]; +fn is_structured_help_text(help_text: &str) -> bool { + serde_json::from_str::(help_text).is_ok_and(|value| value.is_object()) +} + pub fn help_text_with_display_order(name: &str, help_text: &str, display_order: usize) -> String { if HELP_TEXT_NAME_OVERRIDES.contains(&name) { return help_text.to_string(); } - let mut value: Value = serde_json::from_str(help_text).unwrap_or(Value::Null); - - let is_schema = value - .as_object() - .map(|obj| { - obj.contains_key("schema_version") - && obj.get("properties").is_some_and(Value::is_object) - }) - .unwrap_or(false); - - if is_schema { - if let Some(properties) = value.get_mut("properties").and_then(Value::as_object_mut) { - properties.insert("display_order".to_string(), json!(display_order)); + match serde_json::from_str::(help_text) { + Ok(Value::Object(mut object)) => { + match object.get_mut("properties") { + Some(Value::Object(properties)) => { + properties + .entry("display_order") + .or_insert_with(|| json!(display_order)); + } + Some(_) => return help_text.to_string(), + None => { + object.insert( + "properties".to_string(), + json!({ "display_order": display_order }), + ); + } + } + object + .entry("schema_version") + .or_insert_with(|| json!(SETTING_HELP_TEXT_SCHEMA_VERSION)); + serde_json::to_string(&Value::Object(object)).unwrap_or_else(|_| help_text.to_string()) } - return serde_json::to_string(&value).unwrap_or_else(|_| help_text.to_string()); + _ => json!({ + "schema_version": SETTING_HELP_TEXT_SCHEMA_VERSION, + "properties": { + "help_text": help_text, + "display_order": display_order, + } + }) + .to_string(), } - - json!({ - "schema_version": 1, - "properties": { - "help_text": help_text, - "display_order": display_order, - } - }) - .to_string() } pub fn assign_setting_display_orders(settings: &mut [Setting]) { + let mut skipped: Vec = Vec::new(); + for (display_order, setting) in settings.iter_mut().enumerate() { + if setting.name.starts_with("screenly_") { + continue; + } + + if HELP_TEXT_NAME_OVERRIDES.contains(&setting.name.as_str()) + || (setting.type_ == SettingType::Secret + && !is_structured_help_text(&setting.help_text)) + { + skipped.push(setting.name.clone()); + continue; + } + setting.help_text = help_text_with_display_order(&setting.name, &setting.help_text, display_order); } + + if !skipped.is_empty() { + eprintln!( + "Warning: no display order was assigned to the following settings, so the UI decides where they render: {}.", + skipped.join(", ") + ); + } } impl Setting { @@ -558,3 +598,159 @@ impl Api { Ok(()) } } + +#[cfg(test)] +mod display_order_tests { + use super::*; + + fn setting(name: &str, type_: SettingType, help_text: &str) -> Setting { + Setting { + type_, + default_value: None, + title: Some(name.to_string()), + name: name.to_string(), + optional: false, + help_text: help_text.to_string(), + is_global: false, + } + } + + fn properties(help_text: &str) -> serde_json::Map { + serde_json::from_str::(help_text) + .unwrap() + .get("properties") + .unwrap() + .as_object() + .unwrap() + .clone() + } + + #[test] + fn plain_help_text_is_wrapped_with_the_positional_display_order() { + let result = help_text_with_display_order("greeting", "Say hello", 3); + let properties = properties(&result); + + assert_eq!(properties["help_text"], json!("Say hello")); + assert_eq!(properties["display_order"], json!(3)); + } + + #[test] + fn explicitly_authored_display_order_is_not_overwritten() { + let authored = json!({ + "schema_version": 1, + "properties": { "help_text": "x", "type": "number", "display_order": 2 } + }) + .to_string(); + + let properties = properties(&help_text_with_display_order("number_field", &authored, 0)); + + assert_eq!(properties["display_order"], json!(2)); + assert_eq!(properties["type"], json!("number")); + } + + #[test] + fn structured_help_text_without_display_order_gets_the_positional_one() { + let authored = json!({ + "schema_version": 1, + "properties": { "help_text": "x", "type": "url" } + }) + .to_string(); + + let properties = properties(&help_text_with_display_order("url_field", &authored, 5)); + + assert_eq!(properties["display_order"], json!(5)); + assert_eq!(properties["type"], json!("url")); + } + + #[test] + fn json_object_with_a_malformed_properties_key_is_left_untouched() { + let authored = json!({ "schema_version": 1, "properties": "nope" }).to_string(); + + assert_eq!( + help_text_with_display_order("weird", &authored, 0), + authored, + "a malformed object must not be stringified into its own help text" + ); + } + + #[test] + fn json_object_without_properties_keeps_its_other_keys() { + let authored = json!({ "schema_version": 1, "depends_on": "other" }).to_string(); + + let value: Value = + serde_json::from_str(&help_text_with_display_order("field", &authored, 1)).unwrap(); + + assert_eq!(value["depends_on"], json!("other")); + assert_eq!(value["properties"]["display_order"], json!(1)); + } + + #[test] + fn overridden_names_are_left_untouched() { + assert_eq!( + help_text_with_display_order("theme", "Pick a theme", 0), + "Pick a theme" + ); + } + + #[test] + fn internal_and_excluded_settings_are_skipped() { + let mut settings = vec![ + setting("greeting", SettingType::String, "Say hello"), + setting("theme", SettingType::String, "Pick a theme"), + setting("api_key", SettingType::Secret, "Your API key"), + setting( + "screenly_entrypoint", + SettingType::String, + "The entrypoint.", + ), + ]; + + assign_setting_display_orders(&mut settings); + + assert_eq!( + properties(&settings[0].help_text)["display_order"], + json!(0) + ); + assert_eq!(settings[1].help_text, "Pick a theme"); + assert_eq!(settings[2].help_text, "Your API key"); + assert_eq!(settings[3].help_text, "The entrypoint."); + } + + #[test] + fn secrets_that_opt_into_structured_help_text_are_still_ordered() { + let authored = json!({ + "schema_version": 1, + "properties": { "help_text": "Your API key" } + }) + .to_string(); + let mut settings = vec![setting("api_key", SettingType::Secret, &authored)]; + + assign_setting_display_orders(&mut settings); + + assert_eq!( + properties(&settings[0].help_text)["display_order"], + json!(0) + ); + } + + #[test] + fn duplicate_setting_names_are_rejected() { + let yaml = "\ +greeting: + type: string + optional: true + help_text: first +greeting: + type: string + optional: true + help_text: second +"; + let error = deserialize_settings(serde_yaml::Deserializer::from_str(yaml)) + .expect_err("duplicate setting names must be rejected"); + + assert!( + error.to_string().contains("declared more than once"), + "unexpected error: {error}" + ); + } +} diff --git a/src/commands/edge_app/app.rs b/src/commands/edge_app/app.rs index c2f8dd35..d6f73545 100644 --- a/src/commands/edge_app/app.rs +++ b/src/commands/edge_app/app.rs @@ -210,9 +210,7 @@ impl EdgeAppCommand { let manifest_path = transform_edge_app_path_to_manifest(&path)?; EdgeAppManifest::ensure_manifest_is_valid(&manifest_path)?; - let mut manifest = EdgeAppManifest::new(&manifest_path)?; - manifest.assign_setting_display_orders(); - EdgeAppManifest::save_to_file(&manifest, &manifest_path)?; + let manifest = EdgeAppManifest::new(&manifest_path)?; let actual_app_id = match self.get_app_id(path.clone()) { Ok(id) => id, diff --git a/src/commands/edge_app/manifest.rs b/src/commands/edge_app/manifest.rs index b6d3bb30..3fb61463 100644 --- a/src/commands/edge_app/manifest.rs +++ b/src/commands/edge_app/manifest.rs @@ -276,10 +276,6 @@ impl EdgeAppManifest { Ok(()) } - pub fn assign_setting_display_orders(&mut self) { - crate::api::edge_app::setting::assign_setting_display_orders(&mut self.settings); - } - pub fn prepare_payload(manifest: &EdgeAppManifest) -> HashMap<&str, serde_json::Value> { let entrypoint_uri = match &manifest.entrypoint { Some(entrypoint) => entrypoint.uri.clone(), From 17471ef63f26c7b3e3e2f3f0af36c3cb7ff37c64 Mon Sep 17 00:00:00 2001 From: nicomiguelino Date: Tue, 25 Aug 2026 19:54:39 -0700 Subject: [PATCH 5/5] Address remaining PR #308 review comments on display_order warnings Warn on malformed help_text properties instead of skipping silently, and stop warning about secrets without a descriptor. --- docs/EdgeApps.md | 5 ++-- src/api/edge_app/setting.rs | 51 ++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/docs/EdgeApps.md b/docs/EdgeApps.md index d11f9d5e..d6b552f5 100644 --- a/docs/EdgeApps.md +++ b/docs/EdgeApps.md @@ -656,10 +656,11 @@ In this example, `refresh_interval_seconds` only appears in the install and edit Notes: - These descriptors are backward-compatible; if no JSON is provided, the UI falls back to a plain text field for `string` and a password field for `secret`. -- Some settings are excluded from the automatic `display_order` assignment and are left to the UI's default ordering. `deploy` prints a warning naming any setting it skipped. The exclusions are: +- Some settings are excluded from the automatic `display_order` assignment and are left to the UI's default ordering: - Settings of `type: secret` that do not already provide a descriptor, so that the UI's password-field fallback keeps applying. To order a secret, give it an explicit descriptor with a `display_order`. - Internal `screenly_*` settings generated by the CLI for `auth:` and remote entrypoints. They are not declared in the manifest, so they have no authored position. - - A small set of setting names whose help text is managed outside of the CLI: `azure_ad_resource`, `azure_ad_scope`, `bypass_cors`, `cache_interval`, `limit`, `message_body`, `override_coordinates`, `override_locale`, `override_timezone`, `rss_url`, `stop_id`, `target_timestamp`, and `theme`. + - A small set of setting names whose help text is managed outside of the CLI: `azure_ad_resource`, `azure_ad_scope`, `bypass_cors`, `cache_interval`, `limit`, `message_body`, `override_coordinates`, `override_locale`, `override_timezone`, `rss_url`, `stop_id`, `target_timestamp`, and `theme`. Since these are otherwise easy to pick by accident, `deploy` prints a warning naming any of these it skipped. +- A setting whose `help_text` looks like a schema (it has `schema_version`) but whose `properties` value isn't itself an object is left completely untouched: no `type`, `options`, `advanced`, or `display_order` is applied, and the UI shows the raw JSON as the field's help text. `deploy` prints a separate warning naming these settings — fix the `properties` value to resolve it. #### Integrations diff --git a/src/api/edge_app/setting.rs b/src/api/edge_app/setting.rs index 552c31ef..b2de3287 100644 --- a/src/api/edge_app/setting.rs +++ b/src/api/edge_app/setting.rs @@ -270,6 +270,13 @@ fn is_structured_help_text(help_text: &str) -> bool { serde_json::from_str::(help_text).is_ok_and(|value| value.is_object()) } +fn has_malformed_properties(help_text: &str) -> bool { + let Ok(Value::Object(object)) = serde_json::from_str::(help_text) else { + return false; + }; + matches!(object.get("properties"), Some(value) if !value.is_object()) +} + pub fn help_text_with_display_order(name: &str, help_text: &str, display_order: usize) -> String { if HELP_TEXT_NAME_OVERRIDES.contains(&name) { return help_text.to_string(); @@ -309,16 +316,23 @@ pub fn help_text_with_display_order(name: &str, help_text: &str, display_order: pub fn assign_setting_display_orders(settings: &mut [Setting]) { let mut skipped: Vec = Vec::new(); + let mut malformed: Vec = Vec::new(); for (display_order, setting) in settings.iter_mut().enumerate() { if setting.name.starts_with("screenly_") { continue; } - if HELP_TEXT_NAME_OVERRIDES.contains(&setting.name.as_str()) - || (setting.type_ == SettingType::Secret - && !is_structured_help_text(&setting.help_text)) - { + if has_malformed_properties(&setting.help_text) { + malformed.push(setting.name.clone()); + continue; + } + + if setting.type_ == SettingType::Secret && !is_structured_help_text(&setting.help_text) { + continue; + } + + if HELP_TEXT_NAME_OVERRIDES.contains(&setting.name.as_str()) { skipped.push(setting.name.clone()); continue; } @@ -327,6 +341,13 @@ pub fn assign_setting_display_orders(settings: &mut [Setting]) { help_text_with_display_order(&setting.name, &setting.help_text, display_order); } + if !malformed.is_empty() { + eprintln!( + "Warning: the following settings have a malformed help_text schema (\"properties\" is not an object) and will render as a plain field with the raw JSON as their help text: {}.", + malformed.join(", ") + ); + } + if !skipped.is_empty() { eprintln!( "Warning: no display order was assigned to the following settings, so the UI decides where they render: {}.", @@ -716,6 +737,28 @@ mod display_order_tests { assert_eq!(settings[3].help_text, "The entrypoint."); } + #[test] + fn malformed_properties_are_left_untouched_and_do_not_consume_a_display_order() { + let malformed = json!({ "schema_version": 1, "properties": "nope" }).to_string(); + let mut settings = vec![ + setting("greeting", SettingType::String, "Say hello"), + setting("weird", SettingType::String, &malformed), + setting("farewell", SettingType::String, "Say bye"), + ]; + + assign_setting_display_orders(&mut settings); + + assert_eq!( + properties(&settings[0].help_text)["display_order"], + json!(0) + ); + assert_eq!(settings[1].help_text, malformed); + assert_eq!( + properties(&settings[2].help_text)["display_order"], + json!(2) + ); + } + #[test] fn secrets_that_opt_into_structured_help_text_are_still_ordered() { let authored = json!({