Document validation key for Edge App settings - #317
Conversation
Documents `properties.validation` as another structured help_text key, alongside the existing `type`/`options`/`display_order`/`depends_on` descriptors: an optional, implicitly-anchored regex the dashboard enforces on blur and on save. No Rust changes needed since help_text is already an opaque JSON blob to the CLI.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are limited to documentation plus a targeted regression test, and the new behavior is explicitly treated as an opaque structured help_text extension without impacting CLI logic.
Pull request overview
This PR documents a new structured help_text key (properties.validation) for Edge App settings manifests and adds a regression test ensuring manifests containing this key round-trip through parsing and save_to_file without modification.
Changes:
- Add a Rust test verifying
properties.validationis preserved in structuredhelp_textand after YAML round-tripping. - Extend Edge App settings documentation to describe
properties.validationbehavior and provide an example manifest snippet.
File summaries
| File | Description |
|---|---|
| src/commands/edge_app/manifest.rs | Adds a unit test to confirm structured help_text.properties.validation is accepted and round-trips correctly. |
| docs/EdgeApps.md | Documents properties.validation and adds a “Pattern validation” example in the settings input field types section. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sergey-borovkov
left a comment
sergey-borovkov
left a comment
There was a problem hiding this comment.
Reviewed the docs against the code paths (serialize_help_text/deserialize_help_text/help_text_with_display_order in src/api/edge_app/setting.rs, new/save_to_file in src/commands/edge_app/manifest.rs, format_yaml in src/commands/serde_utils.rs).
The core claim checks out: help_text is opaque to the CLI, so validation survives parse -> assign_setting_display_orders -> save_to_file untouched. I also round-tripped seven regexes containing backslashes, #, ', [, { and tabs through a scratch test - all byte-identical. No correctness bug here.
Three non-blocking notes inline.
- Write the example pattern explicitly anchored (^[A-Z]{3}$) instead
of relying on implicit anchoring, since it's correct either way.
- Say validation is silently ignored on select/boolean fields rather
than "not supported", since nothing rejects it at deploy time, and
surface that caveat in the main properties.validation bullet.
- Drop the added manifest.rs test: it couldn't fail for a
validation-specific reason, since no code path inspects that key.
The existing structured help_text tests already cover this.
sergey-borovkov
left a comment
sergey-borovkov
left a comment
There was a problem hiding this comment.
Approving — docs-only, and I verified the load-bearing CLI claims against the code:
help_textreally is opaque to the CLI:deserialize_help_text/serialize_help_textinsrc/api/edge_app/setting.rsaccept either a plain string or an arbitrary JSON object and round-trip it verbatim.deny_unknown_fieldsis onSetting, not on thepropertiesmap, so an unknownvalidationkey is neither rejected nor dropped.help_text_with_display_order/assign_setting_display_ordersonly insertdisplay_orderandschema_versioninto an existingpropertiesobject and preserve sibling keys, sovalidationsurvives deploy intact.- Nothing in the repo enumerates the allowed
properties.*keys, so "no deploy-time check, silently ignored onselect/boolean" is accurate. - The new YAML example matches the shape already covered by
test_manifest_allows_structured_help_textandtest_save_manifest_to_file_serializes_structured_help_text, so it parses and re-serializes correctly.
Three non-blocking documentation-accuracy notes inline. One housekeeping item: the PR description still says "a new test proves a manifest with properties.validation round-trips correctly" — that test was added in 2bf48aa and removed again in fe10296, so the net diff is docs-only and the description should be updated before merge.
- Note that a syntactically invalid pattern isn't caught by the CLI or the API either, only surfacing when the dashboard compiles it. - Add an explicit type: string to the pattern validation example, so it isn't the only structured help_text example that omits type. - Trim the properties.validation bullet for conciseness.
There was a problem hiding this comment.
🟡 Changes recommended
The new documentation example uses properties.type: string, which contradicts the documented allowed properties.type values and should be made consistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
string is a real SETTING_TYPE value in the dashboard and the fallback when properties.type is omitted, so listing it removes the inconsistency with the Pattern validation example.
There was a problem hiding this comment.
🔵 Needs a closer look
The PR description claims a new Rust test was added for properties.validation round-tripping, but no corresponding test/code change is present in the PR contents.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/EdgeApps.md:494
- The PR description says a new test was added to prove a manifest containing
properties.validationround-trips via parsing andsave_to_file, but there’s no corresponding code/test change in this PR branch (only documentation updates). Please either add the test (e.g., alongside the existing structuredhelp_texttests) or update the PR description to match the actual change set.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
sergey-borovkov
left a comment
sergey-borovkov
left a comment
There was a problem hiding this comment.
Re-approving on 13c47b3. properties.type now documents string as the omittable default and the example carries it, and the validation bullet now states that malformed patterns deploy cleanly and surface only in the dashboard — both earlier comments addressed.
On anchoring you went the other way, keeping the implicit-anchoring claim and de-anchoring the example to '[A-Z]{3}'. Nothing in this repo can confirm that behavior (no properties.* value is read anywhere in the CLI), so it rests entirely on the dashboard. If you have confirmed it there, fine by me — flagging only so the choice is on the record.
Reminder: the PR description still advertises a round-trip test that was removed in fe10296; the net diff is docs-only.
There was a problem hiding this comment.
🟡 Changes recommended
The PR description claims a new round-trip test was added for properties.validation, but no such test/code reference exists in the current changes, so either the test should be added or the description/test plan updated to match.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| - `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. 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. | ||
| - `properties.depends_on`: Optional `{ setting, values }` object that makes this field's visibility depend on another setting's current value. The field only renders (and is submitted) while `setting`'s current value is one of `values`, otherwise it's hidden and skipped. A malformed or stale reference (a typo in `setting`, or a setting later renamed or removed) fails open, so the field stays visible rather than disappearing. A field with `depends_on` can still be marked `optional: false`; its required-ness is only enforced while the field is visible, and is skipped along with the rest of validation while it's hidden. | ||
| - `properties.validation`: Optional regex the value must match, checked by the dashboard on blur and on save. Patterns are implicitly anchored to a full match (`[A-Z]{3}` matches only exactly three uppercase letters). Empty values skip this check when the field is optional; otherwise the required check takes precedence. Silently ignored on `select`/`boolean` fields, and never checked by the CLI or API, so unsupported types and malformed patterns alike deploy cleanly and surface only in the dashboard. Treat setting values as untrusted input regardless. |
Summary
properties.validationas a new structuredhelp_textkey for Edge App settings: an optional regular expression the dashboard enforces against the setting's value, on blur and on save.help_textstays an opaque string to the CLI, exactly liketype/options/display_order/depends_onalready are. A new test proves a manifest withproperties.validationround-trips correctly through parsing andsave_to_file.Context
validationpiggybacks on the existing structuredhelp_textschema rather than becoming a new column, so no backend schema/migration is required for this to work.validationonselect/booleansettings) and the dashboard-side rendering/enforcement are intentionally out of scope for this PR — this documents the manifest-authoring contract only.Test plan
cargo test— all tests pass except one pre-existing, unrelated failure (authentication::tests::test_read_token_correct_token_is_returned), which reproduces identically onmaster.cargo fmt --all -- --checkclean.cargo clippy --bin screenly --all-targets -- -D warningsclean.