fix(notes): make the install default notes structure take effect - #150
Merged
Conversation
Clicking "Use as install default" appeared to do nothing. The click was reaching the API and the value was being stored, but the badge and the link label are rendered from is_install_default on the template list, which was only fetched when the section mounts. A save that succeeded therefore left the row byte-identical, so the control read as dead. The section now awaits the save and refetches the list, in a finally block so the row reflects what the server actually stored whether the save succeeded or failed. That required persistNow in AISettings to return its promise; it still catches and reports failures, so every other caller is unchanged. A second defect followed from the first. The client held the value optimistically, and the autosave layer skips any save whose serialised payload matches the last one it stored, so every click after the first emitted no request at all. That trap only springs when the API returns 200 without having stored anything, which ConfigManager allowed: _save_config caught write errors, logged them, and returned normally, so _persist_install_wide_ai_settings reported success on a write that never happened. Install-wide settings live only in config.json, so on a deployment whose data directory is not writable by the container user every such save silently reverted on the next reload. The write now raises and the endpoint returns an explicit error. The first-run bootstrap write stays best effort so an unwritable directory cannot block startup. Two smaller hardening changes come with it. The install-wide write now reloads before its read-modify-write, so a config edited out of band is not reverted by a stale in-memory copy, matching what the telemetry writer already did. And install_notes_template_id is validated against the database, since it is a foreign key held in a flat file with nothing enforcing it and resolution degrades silently to the built-in structure, so a wrong id was previously accepted and then did nothing. Refs: #149
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.
Pull Request
Description
Clicking Use as install default on an install-scoped notes structure appeared to do nothing. The click was reaching the API and the value was being stored, but the Install default badge and the link label are rendered from
is_install_defaultonGET /notes-templates, which was only fetched when the section mounts. A save that succeeded end to end therefore left the row byte-identical, so the control read as dead. By contrast the per-user default's tick renders fromsettings.notes_template_id, whichpersistNowupdates optimistically, which is why one control looked healthy and the other looked broken.The section now awaits the save and refetches the list, in a
finallyblock so the row reflects what the server actually stored whether the save succeeded or failed. That requiredpersistNowinAISettingsto return its promise. It still catches and reports failures, so every other caller is unchanged.A second defect followed from the first. The client held the value optimistically and the autosave layer skips any save whose serialised payload matches the last one it stored, so every click after the first emitted no HTTP request at all, which matches the "no corresponding request reaches the API" in the report. That trap only springs when the API returns 200 without having stored anything, which
ConfigManagerallowed:_save_configcaught write errors, logged them, and returned normally, so_persist_install_wide_ai_settingsreported success on a write that never happened. Install-wide settings live only inconfig.json, so on a deployment whosedata/directory is not writable by the container user, every such save silently reverted on the next reload. The write now raises and the endpoint returns an explicit error. The first-run bootstrap write stays best effort so an unwritable directory cannot block startup.Two smaller hardening changes come with it. The install-wide write now reloads before its read-modify-write, so a config edited out of band is not reverted by a stale in-memory copy, matching what
telemetry._write_configalready did. Andinstall_notes_template_idis validated against the database, since it is a foreign key held in a flat file with nothing enforcing it and resolution degrades silently to the built-in structure, so a wrong id was previously accepted and then quietly did nothing.No new dependencies.
Refs #149
Type of change
Checks run
source .venv/bin/activate && pytest(1087 passed)python scripts/check.py(Ruff lint, format check, whitespace, file size, held pins, mypy, doc and Alembic validators all passed)cd frontend && npm run lintcd frontend && npm run test(293 passed, 51 files)cd frontend && npm run buildpython3 scripts/validate_docs.pypython3 scripts/validate_alembic.pyMigration impact
Documentation impact
docs/DEPLOYMENT.mdnow states that install-wide settings are written todata/config.jsonrather than the database, that the directory must be writable by the API container's user, and that a failed write is now reported instead of silently reverting.Security impact
The new admin-only validation narrows what an administrator can set; it does not widen any boundary. The non-admin path is unchanged and covered by a test asserting the key is still dropped before it reaches the config.
Manual verification
Not a capture change, so no browser smoke set applies.
Verified against a live deployment before writing the fix, to establish that the backend half was already correct. Driving the real settings router in-process against the running instance's database and a copy of its
config.json, aPOSTcarryinginstall_notes_template_idwrote the value, survivedreload, and came back on the nextGET, twice in succession. That is what isolated the defect to the missing refetch rather than the save.Both new defects are pinned by tests that fail without the change:
NotesTemplatesSection.test.tsxasserts the badge appears and the link flips after a save, that clearing works, that a rejected save leaves the server's answer on screen, and that a non-admin is never offered the control.test_install_notes_default.pyasserts the value reaches the config, that a failed write is a reported error rather than a silent success, that a personal-scoped or unknown id is refused, that clearing is allowed, and that a non-admin write is dropped.Still worth doing on a real deployment before merge, since automated tests cannot cover the browser path:
chmod a-w data/config.json, click the link, and confirm an explicit error appears instead of a silent success. Restore the permission afterwards.