Skip to content

fix(provisioning): a per-App settings section turned off is a change, and the plan says so - #235

Merged
windischb merged 4 commits into
developfrom
fix/app-settings-replace-plan
Sep 14, 2026
Merged

windischb merged 4 commits into
developfrom
fix/app-settings-replace-plan

Conversation

@windischb

Copy link
Copy Markdown
Contributor

Summary

Reported from the admin UI: unticking Dedicated subdomain for this app and staging it produced a draft that showed no changes at all — while the apply happily cleared the subdomain and dropped the host route. A plan that under-reports is worse than no plan; the whole staged-save UX rests on it being true.

The bug

An App's Settings is the complete desired override — AppAdminService rebuilds the whole thing from it, so a null section clears that override and an empty subdomain drops the global host route. The planner, though, diffed it as merge-patch off a serialization using the API's WhenWritingNull option, which erases exactly the nulls that carry the intent. It affected every section, not just Origin: turning Branding, PageTheme or any other one off staged a real clear the plan reported as nothing.

SectionPolicy gains NestedReplace next to NestedPatch. A replace section is serialized a second time with nulls kept and diffed off that. Optional<T> keeps its absence — the resolver suppresses a None through ShouldSerialize, independently of the ignore condition.

Not a regression of the ADR 0024 wave: the apps section has been NestedPatch since #214, and the apply was always right. No stored draft needs fixing — the plan is recomputed on every read.

Two honesty fixes in the same screen

  • The Origin checkbox is gone. It carried no information: unticking it and ticking it with an empty field produced the identical payload, so it was a second clear button next to the one the input already has — and two controls for one fact can contradict each other (ticked but empty). It also greyed the input against an "inherited" value that does not exist; a realm has a primary domain, not a subdomain to inherit. The value is the switch now, and the hint under the field says so.
  • Opening an App's settings no longer invents a ChangeFeed override. The form always emitted a ChangeFeed section from its defaults, materializing an override that says exactly what no override already says (the reader falls back to disabled either way) and adding three phantom lines to every plan. No new toggle — Enable consumer change feed already is the switch. A section the App already has is kept, so switching the feed off preserves a configured retention window instead of dropping it.

Deliberately not modelled: a disabled-but-remembered subdomain. That needs a real second property and raises the question of whether a parked host keeps blocking the cross-realm uniqueness check while routing nowhere. Worth deciding on its own.

Test plan

  • Turning_off_a_per_app_settings_section_shows_in_the_plan_as_the_clear_it_is — red before the fix (unchanged instead of update), green after; asserts the plan reports both clears, that the apply really drops the override and the host route, and that re-planning the cleared state is then genuinely unchanged
  • Full backend suite green (825 integration + 1667 unit)
  • pnpm type-check clean, de.json parses
  • Browser against the dev realm: clearing the subdomain plans as Settings.Origin.Subdomain with the live value as current and no target, applying drops the origin and leaves branding intact; an App without a change feed plans only its real change; switching a configured feed off keeps 30 days / 5000 events and plans exactly one change
  • CI on this PR

🤖 Generated with Claude Code

windischb and others added 4 commits September 14, 2026 14:52
… and the plan says so

An App's Settings is the COMPLETE desired override: AppAdminService rebuilds the whole
thing from it, so a null section clears that override (inherit the realm) and an empty
subdomain drops the global host route. The planner, though, diffed it as merge-patch off
a serialization using the API's WhenWritingNull option — which erases exactly the nulls
that carry the intent. Unchecking "Dedicated subdomain for this app" therefore staged a
real clear that the draft reported as NO changes at all ("22 unchanged"), while the apply
went ahead and cleared it. A plan that under-reports is worse than no plan: the whole
confirmation UX rests on it.

SectionPolicy gains NestedReplace next to NestedPatch. A replace section is serialized a
second time with nulls kept and diffed off that, so Settings.Origin.Subdomain and a
switched-off Settings.Branding show up like any other change. Optionals keep their
absence — the resolver suppresses a None through ShouldSerialize, independently of the
ignore condition. The options instance is cached per source options (building one is
expensive and the diff runs per entity).

Not a regression of this wave: the apps section has been NestedPatch since #214 (the
always-staged admin UI), and the apply was always right — only the preview lied. No
stored draft needs fixing; the plan is recomputed on every read.

Test: Turning_off_a_per_app_settings_section_shows_in_the_plan_as_the_clear_it_is —
asserts the plan reports both clears, that the apply really drops the override and the
host route, and that re-planning the cleared state is then genuinely unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gin checkbox

"Dedicated subdomain for this app" carried no information. build() produced
`Origin: { Subdomain: override ? (value || null) : null }`, so unticking the box and
ticking it with an empty field were the same payload — a second clear button next to the
one the input already has, and two controls for one fact can contradict each other
(ticked but empty). It also greyed the input out against an "inherited" value that does
not exist: a realm has a primary domain, not a subdomain to inherit.

The value is the switch now: a subdomain routes that host to the app, an empty field
means no own subdomain (the app is reached through the realm URL), and clearing it
removes the existing route — which is what the backend already did with a null.

Deliberately NOT modelled: a disabled-but-remembered subdomain. That needs a real second
property, and it raises the question of whether a parked host keeps blocking the
cross-realm uniqueness check while it routes nowhere. Worth deciding on its own.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s key

The Origin override checkbox is gone, so its key is dead. The field's own label had
no German entry at all (it fell back to the source string, which happened to be
German) and the new hint needs one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tings

build() always emitted a ChangeFeed section from the reset defaults, because unlike every
other section it has no "is there an override" state. Per-App settings are REPLACE, so
saving any App materialized {Enabled:false, 7, 1000} on an App that had nothing — an
override that says exactly what no override already says (AppChangeFeedSubscription reads
a missing section as ApplicationChangeFeedSettings.Disabled). Every draft plan then listed
three ChangeFeed changes nobody made, which is noise in the one place that has to be
trustworthy.

No new toggle: "Enable consumer change feed" already IS the switch, the same argument that
just removed the Origin checkbox. The section is sent when the feed is on, or when this App
already has one — the second half is what keeps a configured retention window from being
dropped when an admin switches the feed off. An App that never had a feed and is not
getting one now sends nothing.

Verified in the browser against the dev realm: staging a branding edit on an App without a
feed reports only Settings.Branding.ProductName; switching the feed off on an App with
30 days / 5000 events stages Enabled=false with both numbers intact and plans exactly one
change, Settings.ChangeFeed.Enabled: true -> false.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@windischb
windischb merged commit e87933f into develop Sep 14, 2026
8 checks passed
@windischb
windischb deleted the fix/app-settings-replace-plan branch September 14, 2026 14:25
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.

1 participant