fix(save): never add a param only to spell out its sidecar default #223

Merged

dominik.polakovics merged 2 commits from afk/219 into main 2026-10-12 14:53:56 +02:00

Conversation 4 Commits 2 Files changed 9 +890 -26

dominik.polakovics commented 2026-10-12 10:11:19 +02:00

Owner

Copy link

Closes #219

What was wrong

The element form seeds an absent param with its sidecar default, and MergeParams wrote every value it received. So saving an element without changing it added align="left" to a hero whose file omitted align. The default-language sync then copied that into every connected translation, and the pending chip showed "2 changes" for a save that changed nothing.

Fix

• elemops.MergeParams writes a managed param in two cases: the shortcode already holds it, or the value differs from the sidecar default.
  • The default comparison uses the canonical shortcode spelling, the same one l10n.Hash uses.
  • An explicit default stays explicit, including a value set back to its default.
  • Required params are always written. Without this, Schema.Check would refuse an inline chip with a required, defaulted param on the next body save, because it rejects a missing required param whatever its default. No shipped inline element has such a param today.
• Items rows follow the same rule:
  • buildItemShortcodes matches each row to its stored child by the uid the row echoes back, and only that child's managed params count as already held.
  • Rows are still rebuilt from scratch: uid, disabled and unknown params are not carried over.
  • New rows omit fields that are at their default.
• Propagation is unchanged. An unchanged save no longer produces a change, so there is nothing to propagate.
• Docs: scaffold/site/AGENTS.md ("Editing content by hand") and the edit-content skill now state the rule. migrate-site makes no claim about defaults in files, so it needed no change.

Tests

• internal/server:
  • an unchanged save of the multilingual hero, with the form's own snapshot and with the issue's literal request, leaves the en and de files byte-identical
  • align: center propagates to the connected translation, and the free one stays untouched
  • a changed title touches only the source
  • an explicit default stays explicit, and an explicit non-default set back to the default stays explicit
  • an unchanged items save with defaulted item fields leaves both files byte-identical
• internal/elemops: rule unit tests, covering number, bool, select, datetime and link defaults, required params, and items matching.
• internal/l10n: Hash and ItemHash are the same with and without a spelled-out default.
• go vet ./..., go test ./... and cd web && npm test (2792 tests) pass. harness check passes on testdata/multilingual-site (0 errors, 4 warnings about shortcode templates the fixture doesn't ship) and on a freshly scaffolded site.

Notes for follow-up (not changed here)

• Page settings (FrontMatterChanges) still write a defaulted front-matter field that the page lacks on an unchanged save. The issue puts this out of scope.
• Pending list: elementFingerprint in pending.go compares raw params, so a hand edit that adds or drops a spelled-out default shows as a pending change.
• Items reformat: the first items save of a hand-authored, newline-separated items collection reformats it. This is pinned by TestSaveItemsHandAuthoredNewlineSeparatedChildren and is separate from this issue.

🤖 Generated with Claude Code

Closes #219 ## What was wrong The element form seeds an absent param with its sidecar default, and `MergeParams` wrote every value it received. So saving an element without changing it added `align="left"` to a hero whose file omitted `align`. The default-language sync then copied that into every connected translation, and the pending chip showed "2 changes" for a save that changed nothing. ## Fix - **`elemops.MergeParams`** writes a managed param in two cases: the shortcode already holds it, or the value differs from the sidecar default. - The default comparison uses the canonical shortcode spelling, the same one `l10n.Hash` uses. - An explicit default stays explicit, including a value set back to its default. - **Required params are always written.** Without this, `Schema.Check` would refuse an inline chip with a required, defaulted param on the next body save, because it rejects a missing required param whatever its default. No shipped inline element has such a param today. - **Items rows** follow the same rule: - `buildItemShortcodes` matches each row to its stored child by the uid the row echoes back, and only that child's managed params count as already held. - Rows are still rebuilt from scratch: uid, disabled and unknown params are not carried over. - New rows omit fields that are at their default. - **Propagation is unchanged.** An unchanged save no longer produces a change, so there is nothing to propagate. - **Docs:** `scaffold/site/AGENTS.md` ("Editing content by hand") and the `edit-content` skill now state the rule. `migrate-site` makes no claim about defaults in files, so it needed no change. ## Tests - `internal/server`: - an unchanged save of the multilingual hero, with the form's own snapshot and with the issue's literal request, leaves the `en` and `de` files byte-identical - `align: center` propagates to the connected translation, and the free one stays untouched - a changed title touches only the source - an explicit default stays explicit, and an explicit non-default set back to the default stays explicit - an unchanged items save with defaulted item fields leaves both files byte-identical - `internal/elemops`: rule unit tests, covering number, bool, select, datetime and link defaults, required params, and items matching. - `internal/l10n`: `Hash` and `ItemHash` are the same with and without a spelled-out default. - `go vet ./...`, `go test ./...` and `cd web && npm test` (2792 tests) pass. `harness check` passes on `testdata/multilingual-site` (0 errors, 4 warnings about shortcode templates the fixture doesn't ship) and on a freshly scaffolded site. ## Notes for follow-up (not changed here) - **Page settings** (`FrontMatterChanges`) still write a defaulted front-matter field that the page lacks on an unchanged save. The issue puts this out of scope. - **Pending list:** `elementFingerprint` in `pending.go` compares raw params, so a hand edit that adds or drops a spelled-out default shows as a pending change. - **Items reformat:** the first items save of a hand-authored, newline-separated items collection reformats it. This is pinned by `TestSaveItemsHandAuthoredNewlineSeparatedChildren` and is separate from this issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dominik.polakovics added 1 commit 2026-10-12 10:11:19 +02:00

fix(save): never add a param only to spell out its sidecar default ... e1b772d658

The element form seeds an absent param with its sidecar default and sends
it back on every save, and MergeParams wrote every managed value it got.
An unchanged save of an element whose file omits a defaulted Appearance
param (the hero's align) therefore gained align="left", and the
default-language sync copied it into every connected translation: two
pending changes for one no-op save.

MergeParams now writes a managed param when the shortcode already holds
it or when its value differs from the default (compared in canonical
shortcode spelling, as the l10n hash does); required params are always
written, since Schema.Check refuses an inline element lacking one. Items
rows match their stored child by uid and follow the same rule.

AGENTS.md "Editing content by hand" and the edit-content skill state the
rule; regression tests cover the no-op save, Appearance propagation,
translatable-only saves, explicit defaults, items rows and the hash.

Closes #219

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

dominik.polakovics commented 2026-10-12 10:40:30 +02:00

Author

Owner

Copy link

[autoland] verdict: pass

Validated: CONCERNS (non-blocking). Approving; not auto-merging.

Signal relied on: labctl pr checks 223, aggregate success. ci / build-and-test ran green in 19m29s (Go vet/test/build plus frontend build/test, run unconditionally, so it covers this diff and the AGENTS.md and skills drift tests). ci / release was skipped, which is normal for a PR. I re-ran nothing locally.

Conventions: the Conventional Commits title is fine, Closes #219 is present, and AGENTS.md and the edit-content skill are updated. migrate-site makes no claim about param defaults, so it needs no change. The diff matches #219's decisions and acceptance criteria. The tests cover each server criterion, the items rows and the l10n hash.

Concern: rich-text inline chips no longer get defaults on insert, and AGENTS.md now says the opposite.

• When the rich-text editor inserts a new chip, ChipDialog submits submitInline(ref, original = {}, snapshot) (web/src/rte/RichTextField.tsx:754). That goes to elemops.MergeParams(nil, …) (internal/server/forms.go:829). Nothing is held, so the new skip at internal/elemops/save.go:209 drops every optional param that sits at its default.
• Example: a button chip inserted with variant primary used to be stored as variant="primary"; it is now stored without variant.
• The new text in scaffold/site/AGENTS.md:1275-1276 says "an element the editor inserts starts with every default written". That is still true for block elements (NewShortcode) but now false for inline chips.
• The PR body does not mention this change. #219 lists insert-time behaviour as out of scope.
• The rendered page is unaffected: button.html falls back with default "primary".
• Either is fine; just make the docs agree with the code:
  • keep the new behaviour and narrow the AGENTS.md exception to block elements, also refreshing the handleFormInline doc comment (internal/server/forms.go:784-794), or
  • pass a flag so an insert still writes defaults.

[autoland] verdict: pass Validated: CONCERNS (non-blocking). Approving; not auto-merging. **Signal relied on:** `labctl pr checks 223`, aggregate `success`. `ci / build-and-test` ran green in 19m29s (Go vet/test/build plus frontend build/test, run unconditionally, so it covers this diff and the AGENTS.md and skills drift tests). `ci / release` was skipped, which is normal for a PR. I re-ran nothing locally. **Conventions:** the Conventional Commits title is fine, `Closes #219` is present, and AGENTS.md and the `edit-content` skill are updated. `migrate-site` makes no claim about param defaults, so it needs no change. The diff matches #219's decisions and acceptance criteria. The tests cover each server criterion, the items rows and the l10n hash. **Concern: rich-text inline chips no longer get defaults on insert, and AGENTS.md now says the opposite.** - When the rich-text editor inserts a new chip, `ChipDialog` submits `submitInline(ref, original = {}, snapshot)` (`web/src/rte/RichTextField.tsx:754`). That goes to `elemops.MergeParams(nil, …)` (`internal/server/forms.go:829`). Nothing is held, so the new skip at `internal/elemops/save.go:209` drops every optional param that sits at its default. - Example: a `button` chip inserted with variant `primary` used to be stored as `variant="primary"`; it is now stored without `variant`. - The new text in `scaffold/site/AGENTS.md:1275-1276` says "an element the editor inserts starts with every default written". That is still true for block elements (`NewShortcode`) but now false for inline chips. - The PR body does not mention this change. #219 lists insert-time behaviour as out of scope. - The rendered page is unaffected: `button.html` falls back with `default "primary"`. - Either is fine; just make the docs agree with the code: - keep the new behaviour and narrow the AGENTS.md exception to block elements, also refreshing the `handleFormInline` doc comment (`internal/server/forms.go:784-794`), or - pass a flag so an insert still writes defaults.

dominik.polakovics commented 2026-10-12 11:48:42 +02:00

Author

Owner

Copy link

[autoland] verdict: pass

This was generated by AI while landing a PR.

Verdict: CONCERNS (non-blocking). Validated with /land-pr; awaiting the human's confirmation to merge.

Signal relied on: labctl pr checks --wait 223, aggregate success (ci / build-and-test green, ci / release skipped as usual for a PR). Nothing re-run locally. The head merges cleanly with the current main (two editor/rte fixes landed since the branch point, no overlap).

Conventions: Conventional Commits title, Closes #219 present, AGENTS.md and the edit-content skill updated in the same change. The diff matches #219's decisions and acceptance criteria; tests cover the server criteria, items rows and the l10n hash.

Concern (non-blocking), same as the autoland note: a new rich-text inline chip is submitted through handleFormInline (internal/server/forms.go) with empty original params, so MergeParams now omits its optional defaulted fields at insert. Rendering is unaffected (templates fall back), but the new sentence in scaffold/site/AGENTS.md ("an element the editor inserts starts with every default written") is only true for block elements. Follow-up: narrow that sentence to block elements and refresh the handleFormInline doc comment, or write defaults on chip insert.

[autoland] verdict: pass > *This was generated by AI while landing a PR.* Verdict: CONCERNS (non-blocking). Validated with /land-pr; awaiting the human's confirmation to merge. **Signal relied on:** `labctl pr checks --wait 223`, aggregate `success` (`ci / build-and-test` green, `ci / release` skipped as usual for a PR). Nothing re-run locally. The head merges cleanly with the current `main` (two editor/rte fixes landed since the branch point, no overlap). **Conventions:** Conventional Commits title, `Closes #219` present, AGENTS.md and the `edit-content` skill updated in the same change. The diff matches #219's decisions and acceptance criteria; tests cover the server criteria, items rows and the l10n hash. **Concern (non-blocking), same as the autoland note:** a new rich-text inline chip is submitted through `handleFormInline` (`internal/server/forms.go`) with empty original params, so `MergeParams` now omits its optional defaulted fields at insert. Rendering is unaffected (templates fall back), but the new sentence in `scaffold/site/AGENTS.md` ("an element the editor inserts starts with every default written") is only true for block elements. Follow-up: narrow that sentence to block elements and refresh the `handleFormInline` doc comment, or write defaults on chip insert.

dominik.polakovics added 1 commit 2026-10-12 14:29:25 +02:00

docs(save): say that an inserted inline chip omits its optional defaults ... 87e7717726

A new rich-text chip is submitted through handleFormInline with no params
held, so MergeParams writes only its non-default and required params; only
a block element elemops.NewShortcode builds starts with every default
spelled out. AGENTS.md "Editing content by hand" and the handleFormInline
doc comment now say so.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

dominik.polakovics commented 2026-10-12 14:29:31 +02:00

Author

Owner

Copy link

This was generated by AI while landing a PR.

Pushed 87e7717 to afk/219 to resolve the approval's concern: scaffold/site/AGENTS.md ("Editing content by hand") now says only a block-level element the editor inserts starts with every default written, and that an inline chip the rich-text editor inserts starts with only its non-default and required params; the handleFormInline doc comment in internal/server/forms.go says the same. Docs only, no behaviour change. Locally: go vet ./... and go test on internal/element, scaffold, internal/elemops, internal/server pass (the AGENTS.md and skills drift tests included).

> *This was generated by AI while landing a PR.* Pushed 87e7717 to `afk/219` to resolve the approval's concern: `scaffold/site/AGENTS.md` ("Editing content by hand") now says only a block-level element the editor inserts starts with every default written, and that an inline chip the rich-text editor inserts starts with only its non-default and `required` params; the `handleFormInline` doc comment in `internal/server/forms.go` says the same. Docs only, no behaviour change. Locally: `go vet ./...` and `go test` on `internal/element`, `scaffold`, `internal/elemops`, `internal/server` pass (the AGENTS.md and skills drift tests included).

dominik.polakovics commented 2026-10-12 14:53:02 +02:00

Author

Owner

Copy link

[autoland] verdict: pass

This was generated by AI while landing a PR.

Verdict: PASS. Validated with /land-pr on head 87e7717; awaiting the human's free-text confirmation to merge.

Signal relied on: labctl pr checks --wait 223, aggregate success for the current head (ci / build-and-test green in 15m44s, ci / release skipped as usual for a PR). Nothing re-run locally. git merge-tree against the current main reports no conflict.

Conventions: Conventional Commits title, Closes #219 present, AGENTS.md ("Editing content by hand") and the edit-content skill updated in the same change; migrate-site makes no claim about defaults in files. The diff stays within #219's decisions and acceptance criteria: elemops.MergeParams skips an unheld param whose canonical value equals its sidecar default (required params excepted), buildItemShortcodes matches rows to stored children by uid so the same rule applies per row, and the l10n hash is pinned as default-agnostic.

Earlier concern resolved: the follow-up commit 87e7717 narrows the AGENTS.md sentence about inserted elements to block-level rows and documents that a rich-text chip starts without its optional defaults; the handleFormInline doc comment says the same. No open concerns remain.

[autoland] verdict: pass > *This was generated by AI while landing a PR.* Verdict: PASS. Validated with /land-pr on head 87e7717; awaiting the human's free-text confirmation to merge. **Signal relied on:** `labctl pr checks --wait 223`, aggregate `success` for the current head (`ci / build-and-test` green in 15m44s, `ci / release` skipped as usual for a PR). Nothing re-run locally. `git merge-tree` against the current `main` reports no conflict. **Conventions:** Conventional Commits title, `Closes #219` present, AGENTS.md ("Editing content by hand") and the `edit-content` skill updated in the same change; `migrate-site` makes no claim about defaults in files. The diff stays within #219's decisions and acceptance criteria: `elemops.MergeParams` skips an unheld param whose canonical value equals its sidecar default (required params excepted), `buildItemShortcodes` matches rows to stored children by uid so the same rule applies per row, and the l10n hash is pinned as default-agnostic. **Earlier concern resolved:** the follow-up commit 87e7717 narrows the AGENTS.md sentence about inserted elements to block-level rows and documents that a rich-text chip starts without its optional defaults; the `handleFormInline` doc comment says the same. No open concerns remain.

dominik.polakovics merged commit cdf115d725 into main 2026-10-12 14:53:56 +02:00

dominik.polakovics referenced this pull request from a commit 2026-10-12 14:53:57 +02:00

Merge pull request 'fix(save): never add a param only to spell out its sidecar default' (#223) from afk/219 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something is broken

  

enhancement

New feature or improvement

  

needs-triage

Maintainer needs to evaluate this issue

  

ready-for-agent

Fully specified, ready for an AFK agent

  

ready-for-human

Requires human implementation

No labels

bug

enhancement

needs-triage

ready-for-agent

ready-for-human

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Cloonar/deckle!223

No description provided.