Allow adding and deleting apps.yaml list entries and settings from the apps editor - #4741
Allow adding and deleting apps.yaml list entries and settings from the apps editor#4741springfall2008 wants to merge 2 commits into
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>
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.
| if change_type in ("add", "delete") and not is_nested: | ||
| return web.json_response({"success": False, "message": f"Only nested values can be added or deleted, not {path_or_arg}"}) |
There was a problem hiding this comment.
Fixed — nesting is now determined from the parsed path (len(self._split_yaml_path(path_or_arg)) < 2) rather than the client-supplied isNested flag, so a delete/add posted with isNested spoofed true still cannot reach a top-level key. Covered by a new test ("isNested cannot be spoofed true to delete a top-level argument").
| try: | ||
| added_value = self._parse_yaml_fragment(new_value) | ||
| except Exception as e: | ||
| return web.json_response({"success": False, "message": f"Invalid value format for {path_or_arg}: {str(e)}"}) | ||
| try: | ||
| self._add_nested_yaml_value(data[ROOT_YAML_KEY], path_or_arg, added_value) |
There was a problem hiding this comment.
Fixed — added _validate_compare_list, called after every add/update/delete is applied but before the file is written, which requires every compare_list entry to be a dict with a non-empty, unique id and a non-empty name. This catches an id-less add, a duplicate id, and a delete that strips the id/name off an existing entry — verified each is refused with apps.yaml and the live args left unchanged.
| @@ -3676,27 +3707,110 @@ def _update_nested_yaml_value(self, data, path, value): | |||
| keys.append(f"[{index}]") | |||
There was a problem hiding this comment.
Fixed — _split_yaml_path now tokenizes every bracket pair in a path component with re.split(r"(\[[^\[\]]*\])", component) instead of a single key.split("["), so foo[0][1] and foo[0][] split correctly. Added unit coverage for both, plus an end-to-end test that deletes from and appends to a directly nested list (a new nested_matrix fixture field) — confirmed this reproduces the exact ValueError: too many values to unpack you flagged before the fix.
| self._delete_nested_yaml_value(data[ROOT_YAML_KEY], path) | ||
| self._delete_nested_yaml_value(self.args, path) |
There was a problem hiding this comment.
Fixed by the same live_args staging described on the addition above — deletions now mutate live_args and are only published to self.args in place after the full batch (including the deletions loop and compare_list validation) has succeeded and the file has been written.
| self._add_nested_yaml_value(data[ROOT_YAML_KEY], path_or_arg, added_value) | ||
| self._add_nested_yaml_value(self.args, path_or_arg, copy.deepcopy(added_value)) |
There was a problem hiding this comment.
Fixed, together with the same issue on deletion below — every mutation (add, update, and delete) is now staged on live_args, a copy.deepcopy(self.args) taken at the start of the batch, instead of touching self.args directly. self.args (the same object as self.base.args) is only mutated in place — self.args.clear(); self.args.update(live_args) — after the whole batch has validated and the apps.yaml write has succeeded. Verified by reverting just this staging (aliasing live_args = self.args) and confirming the existing 'refused because id-less' test then leaves a stray entry in the live args even though the request reports failure.
- 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 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.