Order Edge App settings as declared in the manifest - #308
Conversation
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.
sergey-borovkov
left a comment
sergey-borovkov
left a comment
There was a problem hiding this comment.
Nice feature, and the rename to display_order reads much better than priority. Two blocking issues though, plus a handful of things I think need addressing before this goes out.
Blocking
1. The documented display_order override doesn't work
src/api/edge_app/setting.rs:276
properties.insert("display_order".to_string(), json!(display_order));This is unconditional, so an explicitly authored display_order is always overwritten by the positional index. I ran it: input {"schema_version":1,"properties":{"help_text":"x","type":"number","display_order":2}} at index 0 comes back as "display_order":0.
That contradicts the docs added in this same PR. docs/EdgeApps.md:486 says "set an explicit value only to override that default", and the "Explicit display order override" example at docs/EdgeApps.md:592 uses number_field: 2 / date_time_field: 1 — those deploy as 0 and 1, i.e. rendered in exactly the reverse of the documented intent.
Should only insert when properties has no display_order key.
2. deploy now rewrites the user's screenly.yml on every run
src/commands/edge_app/app.rs:214
let mut manifest = EdgeAppManifest::new(&manifest_path)?;
manifest.assign_setting_display_orders();
EdgeAppManifest::save_to_file(&manifest, &manifest_path)?;Before this PR deploy never wrote the manifest — only create_in_place did. Now every deploy round-trips it through serde_yaml::to_string + format_yaml, which destroys all YAML comments, normalizes quoting and field order, and expands every plain-string help_text into a nested mapping.
Three concrete failure modes:
- A CI checkout with a read-only manifest now fails
deployoutright with a filesystem error. - The write happens before
get_app_idandensure_edge_app_has_all_necessary_files, so a deploy that aborts (missing app id, missingindex.html) still permanently mutates the source file. - Any CI job running
deploynow leaves a dirty git tree.
At minimum: write only when the settings actually changed, and only after the deploy succeeds. Ideally the ordering is computed for the upload payload without touching the manifest at all.
Should fix
3. Duplicate setting keys now break deploy
src/api/edge_app/setting.rs:73
The old HashMap<String, Setting> round-trip silently kept the last of duplicate keys. The new SettingsVisitor pushes every entry, so a manifest declaring a: twice now yields two Settings both named "a" (verified). Both land in creates in detect_changed_settings, and upload_changed_settings (app.rs:495) POSTs the same name twice — the second create fails with a duplicate-key error and aborts the deploy. save_to_file will also write the duplicated key back out.
If duplicates should be rejected, do it explicitly in the validation loop at setting.rs:97 with a clear message; otherwise dedup by name to preserve the old behaviour.
4. expect() on help_text is now on the universal code path
src/api/edge_app/setting.rs:143
value.as_str().expect("Failed to parse help_text.")create_setting serializes help_text through serialize_help_text, which emits a JSON object whenever the string parses as one — which, after this PR, is every setting. If the stored value comes back as an object rather than a string from GET .../settings?select=…,help_text, this panics (process abort, not a CommandError) during get_settings on every deploy. Previously only apps that opted into structured help text were exposed; now it's universal.
Either way this shouldn't be an expect — a graceful conversion (Value::String(s) => s, other => other.to_string()) costs one line and removes the abort.
5. HELP_TEXT_NAME_OVERRIDES silently disables ordering for generic names
src/api/edge_app/setting.rs:243
The list includes limit, theme, stop_id, cache_interval. Any app that happens to name a setting limit or theme gets no display_order at all, with no warning from the CLI — and the docs at docs/EdgeApps.md:486 state declaration order as the default without qualification. At minimum, print a warning naming the skipped settings and document the exclusion.
6. Secrets now always get structured help text
src/api/edge_app/setting.rs:291
assign_setting_display_orders wraps all settings, secrets included, and the generated schema has properties but no properties.type. The existing note at docs/EdgeApps.md:617 says the UI falls back to a password field for secret "if no JSON is provided" — after this change JSON is always provided, so that fallback condition no longer holds for any setting.
Worth confirming a schema with properties but no type still renders masked for type: secret before merging; if it doesn't, secrets become plain-text inputs after the next deploy.
7. Internal screenly_* settings get ordered and rewritten too
src/commands/edge_app/utils.rs:224
assign_setting_display_orders runs after the auth settings (screenly_http_basic_auth_username / _password, screenly_http_bearer_token) and screenly_entrypoint are appended. So on the first post-upgrade deploy of any app with auth: or a remote entrypoint, those CLI-generated settings get PATCHed to convert their help text into schema JSON and assigned display slots after the user's settings. They aren't manifest-declared and aren't in the override list — I think they should be excluded from ordering entirely.
Minor
8. Non-schema JSON help text is stringified into itself
src/api/edge_app/setting.rs:265
serde_json::from_str(help_text).unwrap_or(Value::Null) combined with the is_schema check means an object that has schema_version but a non-object properties (or no properties at all) is treated as plain text and embedded verbatim as an escaped JSON string in properties.help_text. The user then sees raw JSON as their field's help text, with no error. Either reject it or preserve the object.
Note for merge ordering: this PR and #311 both add a bullet to the same list at docs/EdgeApps.md:486 and a section at the same anchor. Textual conflict only — no semantic conflict, since help_text_with_display_order only inserts into an existing schema's properties and preserves depends_on.
- 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.
sergey-borovkov
left a comment
sergey-borovkov
left a comment
There was a problem hiding this comment.
Went through f4692cc7 against each point — all eight are properly fixed, and the new tests cover the real cases (including the exact override input I'd used to reproduce the first one). Moving the ordering out of the manifest and into the upload payload in detect_changed_settings is the right call; nice that assign_setting_display_orders is gone from EdgeAppManifest entirely rather than just unwired.
I also checked the risk this could have introduced: now that every setting carries structured help_text, if the stored value comes back as an object and the other.to_string() conversion didn't reproduce the CLI's exact bytes, detect_changed_settings would PATCH every setting on every deploy forever. It's stable — serde_json resolves here without preserve_order, so maps are BTreeMap-ordered and the round-trip is byte-identical. Worth knowing that this is now load-bearing: if anything ever pulls in serde_json/preserve_order, that silently turns into an infinite re-PATCH loop. Might deserve a comment on help_text_with_display_order saying so.
Two small things before I approve, neither blocking:
1. The malformed-properties path skips silently, contradicting the new docs
src/api/edge_app/setting.rs
Some(_) => return help_text.to_string(),This bails inside help_text_with_display_order, so assign_setting_display_orders never learns about it and the setting isn't added to skipped — no warning is printed. But the docs added in this same commit (docs/EdgeApps.md:621) say "deploy prints a warning naming any setting it skipped", and this is exactly the case that most warrants one: the author wrote something that looks structured, and it silently gets no ordering.
Your own test json_object_with_a_malformed_properties_key_is_left_untouched pins the untouched behaviour, which is right — it's only the warning that's missing. Cheapest fix is hoisting the check into the caller so all three skip reasons flow through the same skipped list; is_structured_help_text is already there to build on.
2. The warning will fire on nearly every deploy
assign_setting_display_orders
Any type: secret without a descriptor lands in skipped, and most real apps have at least one secret, so the vast majority of deploys now print:
Warning: no display order was assigned to the following settings, so the UI decides where they render: …
This is my own request coming back as noise — I asked for the warning when the exclusion list was invisible and undocumented, but now that docs/EdgeApps.md:621 spells out all three exclusions, "your secret is a secret" is expected behaviour rather than something worth a warning every run. Two options that both work for me: drop secrets from the warning and keep it for the HELP_TEXT_NAME_OVERRIDES names (the genuinely surprising case), or reword it to something informational so it doesn't read like a problem.
Happy either way — tell me which you prefer, or push whatever you think is right and I'll approve.
Warn on malformed help_text properties instead of skipping silently, and stop warning about secrets without a descriptor.
sergey-borovkov
left a comment
sergey-borovkov
left a comment
There was a problem hiding this comment.
Both leftovers resolved in 17471ef6 — the malformed-properties case now gets its own warning naming the actual problem, and descriptor-less secrets are silently skipped with the docs reworded to match. 229 tests pass, clippy clean on --all-targets. Approving.
Two nits, purely optional, not worth another round on their own — fold them in if you touch this again:
has_malformed_propertiesdoesn't requireschema_version, but the new doc line atdocs/EdgeApps.md:663describes the case as "looks like a schema (it hasschema_version)". The code's behaviour is the more useful one; the docs just describe it more narrowly than it works.malformed_properties_are_left_untouched_and_do_not_consume_a_display_orderdoes the opposite of what its name says — the assertion hasfarewellat2, so the malformed setting does consume index 1. The behaviour is correct (gaps are harmless for ascending order); the name is backwards.
One thing worth carrying forward: the stable help_text round-trip is now load-bearing for detect_changed_settings, and it holds only because serde_json resolves without preserve_order here. If that ever gets enabled transitively, every deploy starts re-PATCHing every setting with no other symptom. A comment on help_text_with_display_order recording that would save someone a bad afternoon.
Reminder that #311 is approved and touches the same list at docs/EdgeApps.md, so whichever of the two lands second needs a trivial conflict resolution.
nicomiguelino
changed the title
Summary
screenly.ymlinstead of being alphabetized:deserialize_settingsreads them in file order (no more HashMap round-trip + sort), anddetect_changed_settingsdiffs local vs. remote by name lookup instead of a sorted two-pointer merge, socreates/updatesare emitted in manifest order.position/created_at/updated_atcolumn exists onscreenly_edge_app_settings). To get real ordering in the web install form without a backend/schema change, the CLI now computes apriorityfrom each setting's position and embeds it inhelp_text's structured JSON schema ({"schema_version": 1, "properties": {"priority": N, ...}}) on everydeploy, writing the result back to the manifest file itself.HELP_TEXT_NAME_OVERRIDES(mirroring the web app's hardcodedFIELD_OVERRIDEStable — e.g.override_timezone,bypass_cors,theme) are left untouched: turning theirhelp_textinto JSON would silently drop that name-based override on the frontend, sinceresolveFieldpicks either the schema-derived override or the hardcoded one, never both. Those settings keep today's placement instead of gaining a priority.