Allow adding and deleting apps.yaml list entries and settings from the apps editor - #4741
Merged
Merged
Conversation
…e apps editor Compare profiles (and any other apps.yaml list) could only have their existing values edited - there was no way to remove one, add a new one, or add a new setting to one, so it had to be done by hand in the raw YAML editor. Adds a Delete button to every nested list item and setting, and Add item / Add setting buttons at the end of each group, backed by new "add" and "delete" change types in html_apps_post. Deletions are applied after all edits, deepest path and highest list index first, so several in one save do not shift the indices the others refer to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Path validation, profile validation, and non-atomic live configuration mutations can cause incorrect or inconsistent configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds structured creation and deletion of nested apps.yaml entries.
Changes:
- Adds nested Add/Delete controls and pending-change handling.
- Adds server-side YAML mutation helpers.
- Adds documentation and regression tests.
File summaries
| File | Description |
|---|---|
apps/predbat/web.py |
Renders controls and processes YAML changes. |
apps/predbat/web_helper.py |
Adds client-side dialogs and change tracking. |
apps/predbat/tests/test_web_apps_edit.py |
Tests add/delete behavior. |
apps/predbat/unit_test.py |
Registers the new tests. |
docs/web-interface.md |
Documents the controls. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Determine nesting from the parsed path, not the client-supplied isNested flag, so a delete/add posted with isNested spoofed true cannot reach a top-level argument - Validate that every compare_list profile keeps a unique, non-empty id and name after a batch, since compare.py indexes results by id and would otherwise raise KeyError - Tokenize every bracket pair in a path component instead of just one, so a directly nested list (list of lists) can be edited, added to or deleted from - Stage every add/update/delete on a copy of self.args and only publish it in place once the whole batch has validated and the file write has succeeded, so a batch that fails partway (or a failed write) never leaves self.args half-applied and unrecoverable on retry Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated draft PR generated from issue #4714 — a maintainer should review it before merging.
Fixes #4714
Summary
The structured
/appseditor could only change the value of settings that already existed, so a compare profile could be edited but never removed, added, or given a new setting —_update_nested_yaml_valueraisedKeyErrorfor an unknown final key andhtml_apps_posthad no wire representation for a delete or a create.This adds both halves:
render_type): every nested list entry and every setting now gets a Delete button, and each list/dictionary gets an Add item / Add setting row at the end. List entries that are themselves dictionaries (a compare profile) previously rendered with no row id and no actions at all, so they now get both.get_apps_js): deletions and additions are queued like any other edit and only applied on save, so they can be undone (Undo / Remove) or dropped with Discard Changes. Add item againstcompare_listopens with aname:/id:template, so adding a tariff to compare no longer means hand-editing YAML.html_apps_post): newaddanddeletechange types, with_add_nested_yaml_value/_delete_nested_yaml_valuealongside the existing update helper (whose path parsing and navigation are now shared). Added values are parsed as a YAML fragment, so a whole new list entry with its own settings can be created in one go and keeps its types.Two ordering details worth reviewing:
compare_list[0]andcompare_list[2]in one save otherwise removes the wrong second entry.path#n) so several can target the same list, and a deletion is keyedpath#deleteso undoing it does not silently discard a pending edit of the same row. The server therefore takes the path from the change itself rather than from the key.Both
addanddeleteare refused for top-level arguments, so this cannot restructureapps.yamlabove the list/dictionary level. The raw/apps_editorremains the way to do that.Testing
apps/predbat/tests/test_web_apps_edit.py(registered asweb_apps_editinTEST_REGISTRY) — 16 checks covering single and multiple deletions, the index-shift regression, adding two profiles at once, adding a typed setting, an edit plus a delete in one save, the refusal cases (duplicate key, out-of-range index, appending to a dictionary, top-level add/delete, empty value), comment preservation through the ruamel round trip, and the rendered buttons. The client half is asserted structurally, astest_debug_history_client_js.pydoes, since there is no JS engine in the suite.tools/triage_test.sh web_apps_editpasses.coverage/run_pre_commitpasses: all 12 hooks Passed, andrun_all --quickreports all tests passed (4 slow tests skipped). That includesweb_if, which exercisesGET/POST /appsend to end.Notes
Documentation for the new buttons added to the Apps View section of
docs/web-interface.md.