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
There was a problem hiding this comment.
🔵 Needs a closer look
The PR description claims a new round-trip test for properties.validation, but the current changes appear to be documentation-only, so the description and delivered changes are inconsistent.
Review details
Suppressed comments (1)
docs/EdgeApps.md:604
- The PR description says a new test was added to prove a manifest containing
properties.validationround-trips through parsing andsave_to_file, but this branch appears to only add documentation (no test/fixture containing thevalidationkey exists in the repo). Either add the described test coverage or update the PR description so reviewers/CI expectations match the actual changes.
**Pattern validation**
```yaml
settings:
airport_code:
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.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.