Skip to content

Fix help text column showing raw JSON for structured settings - #320

Merged
nicomiguelino merged 6 commits into
masterfrom
fix/help-text-display
Sep 7, 2026
Merged

Fix help text column showing raw JSON for structured settings#320
nicomiguelino merged 6 commits into
masterfrom
fix/help-text-display

Conversation

@nicomiguelino

@nicomiguelino nicomiguelino commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

screenly edge-app setting list printed the raw JSON of help_text for settings that use the structured help text format, instead of the human readable text. This extracts properties.help_text from the structured format before rendering the table.

Before

+------------------------+------------------------+-------+---------------+----------+--------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Name                   | Title                  | Value | Default value | Optional | Type   | Help text                                                                                                                                                                                                                                                                                                      |
+------------------------+------------------------+-------+---------------+----------+--------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| enable_analytics       | Enable Analytics       |       | true          | Yes      | string | {"properties":{"display_order":0,"help_text":"Enable or disable Sentry and Google Analytics integrations."},"schema_version":1}                                                                                                                                                                                |
+------------------------+------------------------+-------+---------------+----------+--------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| style                  | Style                  |       | traditional   | Yes      | string | {"properties":{"display_order":4,"help_text":"Select the visual style for the clock.","options":[{"label":"Traditional","value":"traditional"},{"label":"Modern","value":"modern"},{"label":"Minimal","value":"minimal"}],"type":"select"},"schema_version":1}                                                 |
+------------------------+------------------------+-------+---------------+----------+--------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

After

+------------------------+------------------------+-------+---------------+----------+--------+---------------------------------------------------------------------+
| Name                   | Title                  | Value | Default value | Optional | Type   | Help text                                                            |
+------------------------+------------------------+-------+---------------+----------+--------+---------------------------------------------------------------------+
| enable_analytics       | Enable Analytics       |       | true          | Yes      | string | Enable or disable Sentry and Google Analytics integrations.          |
+------------------------+------------------------+-------+---------------+----------+--------+---------------------------------------------------------------------+
| style                  | Style                  |       | traditional   | Yes      | string | Select the visual style for the clock.                               |
+------------------------+------------------------+-------+---------------+----------+--------+---------------------------------------------------------------------+

Plain string help_text values (e.g. override_locale, override_timezone) are unaffected.

Edge case

A structured help_text with no properties.help_text key now renders an empty cell instead of falling back to the raw JSON.

Extract the human readable string from structured help_text values
instead of printing the raw JSON in edge-app setting list output.
Copilot AI lite review requested due to automatic review settings September 6, 2026 23:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new behavior isn’t covered by existing tests and it also unnecessarily expands the public API surface (should be pub(crate) given current usage).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes screenly edge-app setting list human-readable table output so the “Help text” column shows the extracted properties.help_text string when the setting’s help_text is stored in the structured JSON schema, instead of printing the raw JSON.

Changes:

  • Added a helper to extract displayable help text from the structured help-text JSON payload.
  • Updated the EdgeAppSettings table formatter to apply the extraction logic for the help_text field when rendering human-readable output.
File summaries
File Description
src/commands/mod.rs Adjusts human-readable table rendering for help_text to display extracted text instead of raw JSON.
src/api/edge_app/setting.rs Introduces a helper to extract properties.help_text from structured help text JSON.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/commands/mod.rs Outdated
Comment thread src/api/edge_app/setting.rs Outdated
Scope extract_display_help_text to the crate and add a formatter
test covering both structured and plain help_text values.
Copilot AI review requested due to automatic review settings September 7, 2026 02:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is small, targeted, includes a regression test, and cleanly falls back to the original help text when parsing fails.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

sergey-borovkov
sergey-borovkov previously approved these changes Sep 7, 2026

@sergey-borovkov sergey-borovkov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the fix is correct for the case it targets and I found no regression.

What I checked: extract_display_help_text only rewrites the cell when the value parses to a JSON object and properties.help_text is a string; every other input returns verbatim, so plain help texts and the HELP_TEXT_NAME_OVERRIDES settings are untouched. JSON output goes through the raw-value branch of format_value and is unaffected, and EdgeAppSettings::supports_csv() is false so the CSV branch is unreachable. Built the branch, ran the full suite (235 passed, including the new test) and cargo clippy --all-targets — clean.

Two non-blocking notes inline. The first is a real gap in this fix rather than a style point, and it's a couple of lines in the function this PR just added, so it may be worth folding in here rather than as a follow-up.

Comment thread src/api/edge_app/setting.rs Outdated
Comment thread src/commands/mod.rs Outdated
Copilot AI review requested due to automatic review settings September 7, 2026 14:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The functional change is narrow and covered by a regression test, with only minor optional performance/maintainability nits noted.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/api/edge_app/setting.rs Outdated
Comment thread src/commands/mod.rs Outdated
Render an empty cell instead of the raw JSON when the object has no
properties.help_text, and accept the field as an object value too.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The functional change matches the stated behavior and is covered by tests; only a minor performance optimization was noted.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/api/edge_app/setting.rs Outdated
Skip JSON parsing for help_text strings that clearly aren't objects,
and bind the extracted help text to a local before building the cell.
Copilot AI review requested due to automatic review settings September 7, 2026 14:55
sergey-borovkov
sergey-borovkov previously approved these changes Sep 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

extract_display_help_text currently turns malformed structured schemas (e.g., "properties" not an object) into an empty cell, conflicting with existing code that expects those cases to render as raw JSON (per the warning in assign_setting_display_orders).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/api/edge_app/setting.rs Outdated
Comment thread src/api/edge_app/setting.rs
Only extract properties.help_text when the schema is well-formed,
matching the existing malformed-schema warning in assign_setting_display_orders.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

extract_display_help_text currently returns an empty string for malformed-object inputs (when help_text is already a JSON object with invalid properties), which can silently drop information and should be made consistent with the raw-JSON fallback behavior.

Review details

Suppressed comments (1)

src/api/edge_app/setting.rs:233

  • extract_display_help_text returns an empty string when the input is a JSON object with malformed properties (e.g., properties is a string/null). This happens because the Value::Object arm only matches when properties is not malformed, and the fallback arm returns String::new(), losing the original content; it also differs from the string-parsing branch which preserves the raw JSON in this case.
pub(crate) fn extract_display_help_text(help_text: &Value) -> String {
    match help_text {
        Value::Object(object) if !properties_are_malformed(object) => object
            .get("properties")
            .and_then(|properties| properties.get("help_text"))
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@salmanfarisvp salmanfarisvp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per slack discussion.

@nicomiguelino
nicomiguelino merged commit 1ecaa62 into master Sep 7, 2026
12 checks passed
@nicomiguelino
nicomiguelino deleted the fix/help-text-display branch September 7, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants