fix(items): one canonical row layout owned by a single split/compose #239

Merged

dominik.polakovics merged 2 commits from afk/232 into main 2026-10-12 22:15:51 +02:00

Conversation 2 Commits 2 Files changed 27 +1400 -397

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

Owner

Copy link

Closes #232

What

An items owner's children (its item rows plus its body region) now have one canonical layout: the body region, a line break, then the rows one per line, with no blank line between. One split/compose pair owns that layout: elemops.SplitItems / elemops.ComposeItems in internal/elemops/items.go. The two are exact inverses on canonical input.

• Split is lenient. Any layout the parser accepts still opens in the editor. Split strips the newline runs at the edges of the body region, so the layout's separators no longer leak into GET /api/element/body or into the translation hash.
• Compose is strict. One save reaches the fixed point. Saving a canonical owner without changes leaves the file byte-identical, which is now true for the documented hand-authored shape too.
• Every reader and writer goes through the pair:
  • the body GET (server/body.go)
  • ApplyEdit (split, replace the axes the request sent, compose) and WouldClobberBody
  • the form's row list
  • translatedItemsRefusal
  • l10n.bodyContent (the hash)
  • translation structure repair: Repair, Place and ExtractTranslated edit an owner's rows on the split and write the owner back canonically, and only when they change it.
• One item predicate. elemops.IsItem is the only check for "is this child an item of its parent". l10n.isItem is gone, and holdsElement, StampSource, markTranslated and server.itemAt call IsItem.
• One text merge. content.MergeText is the only adjacent-text merge. removeAt, the split, bodyContent and spliceNodes use it, and coalesceText and the hand-written merges are gone. removecmd/strip.go is untouched.
• Bug fixed in repair placement. A translated item's "leading" flag is now computed among its owner's rows. Before, a body preceding the rows meant a first item with no anchor was placed last.
• Docs. scaffold/site/AGENTS.md defines the layout once, under "Editing content by hand". The "Item collections" paragraph, the image / text-media / cards sidecar comments and the edit-content and migrate-site skills point at it. The showcase page and the AGENTS.md translation example were made canonical; each had a blank line before its row.

Assumptions from the issue, checked:

• The RTE markdown serializer never emits a trailing or leading newline run (prosemirror-markdown serialize never flushes a pending block separator).
• No theme partial depends on rows preceding the body: img and card only push onto the parent's Scratch, and the body comes from .Inner.
• placeIndex / leading needed the fix above.

Tests

• elemops/items_test.go: split/compose table with 15 layouts, including block card rows, a body ending in an inline or block shortcode, and glued or interleaved input. Covers the inverse property, re-parse equality and no mutation of inputs.
• content: TestMergeText.
• server/items_layout_test.go:
  • The issue's reproduction: GET serves "Body.", and the save is byte-identical.
  • A round-trip table over 9 hand-authored shapes: GET succeeds, then save reaches canonical, a second save is byte-identical, and checkcmd.Run accepts the result.
  • A glued file is reformatted only on its own save.
• l10n:
  • Hash is layout-independent, and a real body change still changes it.
  • Repair adding, removing or reordering rows leaves canonical layout, and a second Repair is a no-op.
  • A valid glued owner is not reformatted by Repair.
  • InsertTranslated and ExtractTranslated keep owners canonical.
• Existing tests that pinned the glued form now expect the canonical layout as output. Glued inputs are kept where they test leniency.

go vet ./..., go test ./... and cd web && npm test (2851 tests) are green.

The branch has a no-op merge of ea11f78 on top, which leaves the tree unchanged: my first commit was accidentally parented on a stale local main, and I could not rewrite the pushed branch. The PR diff against main contains only this change.

Release note

• One-time reformat: the first editor save of an items owner (image, text-media, cards, or any element with an items collection) whose file is not yet in the canonical layout rewrites that one element into it. That includes every owner the editor itself wrote before this change, with rows glued on one line before the body. Other elements on the page are not touched, and harness check stays silent about row layout.
• Translations flagged once: after upgrading, some connected translations will show "source changed" once without any edit. This affects stamped items owners whose old hash included layout newlines (hand-authored rows one per line, body-first owners) or an item row without a uid. Editor-written glued owners keep their hash.

🤖 Generated with Claude Code

Closes #232 ## What An items owner's children (its item rows plus its body region) now have one canonical layout: **the body region, a line break, then the rows one per line, with no blank line between**. One split/compose pair owns that layout: `elemops.SplitItems` / `elemops.ComposeItems` in `internal/elemops/items.go`. The two are exact inverses on canonical input. - **Split is lenient.** Any layout the parser accepts still opens in the editor. Split strips the newline runs at the edges of the body region, so the layout's separators no longer leak into `GET /api/element/body` or into the translation hash. - **Compose is strict.** One save reaches the fixed point. Saving a canonical owner without changes leaves the file byte-identical, which is now true for the documented hand-authored shape too. - **Every reader and writer goes through the pair:** - the body GET (`server/body.go`) - `ApplyEdit` (split, replace the axes the request sent, compose) and `WouldClobberBody` - the form's row list - `translatedItemsRefusal` - `l10n.bodyContent` (the hash) - translation structure repair: `Repair`, `Place` and `ExtractTranslated` edit an owner's rows on the split and write the owner back canonically, and only when they change it. - **One item predicate.** `elemops.IsItem` is the only check for "is this child an item of its parent". `l10n.isItem` is gone, and `holdsElement`, `StampSource`, `markTranslated` and `server.itemAt` call `IsItem`. - **One text merge.** `content.MergeText` is the only adjacent-text merge. `removeAt`, the split, `bodyContent` and `spliceNodes` use it, and `coalesceText` and the hand-written merges are gone. `removecmd/strip.go` is untouched. - **Bug fixed in repair placement.** A translated item's "leading" flag is now computed among its owner's rows. Before, a body preceding the rows meant a first item with no anchor was placed last. - **Docs.** `scaffold/site/AGENTS.md` defines the layout once, under "Editing content by hand". The "Item collections" paragraph, the `image` / `text-media` / `cards` sidecar comments and the `edit-content` and `migrate-site` skills point at it. The showcase page and the AGENTS.md translation example were made canonical; each had a blank line before its row. Assumptions from the issue, checked: - The RTE markdown serializer never emits a trailing or leading newline run (prosemirror-markdown `serialize` never flushes a pending block separator). - No theme partial depends on rows preceding the body: `img` and `card` only push onto the parent's Scratch, and the body comes from `.Inner`. - `placeIndex` / leading needed the fix above. ## Tests - `elemops/items_test.go`: split/compose table with 15 layouts, including block `card` rows, a body ending in an inline or block shortcode, and glued or interleaved input. Covers the inverse property, re-parse equality and no mutation of inputs. - `content`: `TestMergeText`. - `server/items_layout_test.go`: - The issue's reproduction: GET serves `"Body."`, and the save is byte-identical. - A round-trip table over 9 hand-authored shapes: GET succeeds, then save reaches canonical, a second save is byte-identical, and `checkcmd.Run` accepts the result. - A glued file is reformatted only on its own save. - `l10n`: - Hash is layout-independent, and a real body change still changes it. - Repair adding, removing or reordering rows leaves canonical layout, and a second Repair is a no-op. - A valid glued owner is not reformatted by Repair. - `InsertTranslated` and `ExtractTranslated` keep owners canonical. - Existing tests that pinned the glued form now expect the canonical layout as output. Glued inputs are kept where they test leniency. `go vet ./...`, `go test ./...` and `cd web && npm test` (2851 tests) are green. The branch has a no-op merge of `ea11f78` on top, which leaves the tree unchanged: my first commit was accidentally parented on a stale local `main`, and I could not rewrite the pushed branch. The PR diff against `main` contains only this change. ## Release note - **One-time reformat:** the first editor save of an items owner (`image`, `text-media`, `cards`, or any element with an `items` collection) whose file is not yet in the canonical layout rewrites that one element into it. That includes every owner the editor itself wrote before this change, with rows glued on one line before the body. Other elements on the page are not touched, and `harness check` stays silent about row layout. - **Translations flagged once:** after upgrading, some connected translations will show "source changed" once without any edit. This affects stamped items owners whose old hash included layout newlines (hand-authored rows one per line, body-first owners) or an item row without a uid. Editor-written glued owners keep their hash. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dominik.polakovics added 2 commits 2026-10-12 19:58:08 +02:00

fix(items): one canonical row layout owned by a single split/compose ... f9f62620db

An items owner's children (its item rows plus its body region) now have
one canonical layout — the body region, a line break, then the rows one
per line — owned by elemops.SplitItems/ComposeItems, exact inverses on
canonical input. Every reader and writer goes through the pair: the body
GET, ApplyEdit and WouldClobberBody, the form's row list, the translated
items refusal, the translation hash and the translation structure repair.

- Split is lenient (any parseable layout opens) and strips the body
  region's edge newline runs, so they no longer leak into the served body
  or the translation hash; compose is strict, so one save reaches the
  fixed point and saving a canonical owner is byte-identical.
- elemops.IsItem is the one "is this child an item" predicate; l10n's
  isItem and holdsElement, and server's itemAt, call it.
- content.MergeText is the one adjacent-text merge; removeAt, the split,
  bodyContent and spliceNodes use it (coalesceText and the hand merges
  are gone).
- Repair edits an items owner's rows on the split and writes the owner
  back canonically, only when it changes it; an item's "leading" is
  computed among its owner's rows, so a first item lands first behind a
  body.
- AGENTS.md documents the layout once ("Editing content by hand"); the
  sidecar comments and the edit-content and migrate-site skills point at
  it.

Closes #232

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

Merge commit 'ea11f78' (main) into afk/232 ... 8d69d4a2f1

# Conflicts:
#	internal/elemops/save.go
#	internal/elemops/save_test.go
#	internal/l10n/hash_test.go
#	internal/server/body.go
#	internal/server/items_test.go
#	scaffold/site/AGENTS.md

dominik.polakovics commented 2026-10-12 20:15:44 +02:00

Author

Owner

Copy link

[autoland] verdict: pass

Verdict: CONCERNS (non-blocking): approved, not auto-merged

Signal relied on: labctl pr checks 239 = success. ci / build-and-test ran the frontend install, build and test, the hugo install, and go vet/test/build, and passed in 16m35s. ci / release was skipped because it only runs on push to main. I did not re-run any of it.

Concern: branch history would put a 36k-line commit on main

• f9f6262 (fix(items): …) is parented on fb92872, main as of #198, not on ea11f78. Its tree already holds everything up to ea11f78 plus the fix. Against its own parent its diff is 334 files, +36,253 / −3,621. The trailing merge 8d69d4a is a no-op on the tree.
• PRs land here as merge commits (labctl pr merge uses a fixed method), so merging as-is puts f9f6262 on main for good.
• Checked: git blame 8d69d4a -- web/src/i18n/en.json, a file this PR does not touch, credits all 1,173 lines to "fix(items): one canonical row layout…". On main they belong to the #229 i18n commits. The same applies to every file changed between #198 and #235.
• Suggested landing: squash-merge in the forge UI, or rebuild afk/232 as one commit on ea11f78. The tree diff against main is correct, at 27 files, so either route lands exactly this change.

Code review: no findings

• The diff matches #232's contract. elemops.SplitItems/ComposeItems (internal/elemops/items.go) are the one owner of the layout. Every reader and writer named in the issue goes through them:
  • server/body.go:92
  • elemops/save.go:121 (ApplyEdit) and :152 (WouldClobberBody)
  • server/forms.go:609
  • server/translation.go:451
  • l10n/hash.go:201
  • l10n/structure.go, via elementList / openList / commit
• elemops.IsItem is the only item predicate. content.MergeText is the only text merge, and coalesceText, lastText and the merge in spliceNodes are gone. removecmd/strip.go is untouched.
• Repair and ordering write an owner back only when a fix touches its rows (orderElements commits only when reordered). indexOf cannot return -1 in indexSource/removeSite, because the split keeps each shortcode by identity in exactly the region region() picks.
• The rendering assumption holds. themes/deckle/layouts/shortcodes/img.html renders nothing when it has a parent, so dropping the blank line before the rows (showcase page, AGENTS.md example) changes no output.
• Docs and skills were updated as CLAUDE.md requires: AGENTS.md "Editing content by hand" and "Item collections", the image/text-media/cards sidecar comments, and the edit-content and migrate-site skills. The drift tests are green in CI.
• Conventions: the Conventional Commits title, Closes #232 and the release note are all present.

[autoland] verdict: pass ## Verdict: CONCERNS (non-blocking): approved, not auto-merged **Signal relied on:** `labctl pr checks 239` = `success`. `ci / build-and-test` ran the frontend install, build and test, the hugo install, and `go vet`/`test`/`build`, and passed in 16m35s. `ci / release` was skipped because it only runs on push to main. I did not re-run any of it. ### Concern: branch history would put a 36k-line commit on main - `f9f6262` (`fix(items): …`) is parented on `fb92872`, main as of #198, not on `ea11f78`. Its tree already holds everything up to `ea11f78` plus the fix. Against its own parent its diff is **334 files, +36,253 / −3,621**. The trailing merge `8d69d4a` is a no-op on the tree. - PRs land here as merge commits (`labctl pr merge` uses a fixed method), so merging as-is puts `f9f6262` on main for good. - Checked: `git blame 8d69d4a -- web/src/i18n/en.json`, a file this PR does not touch, credits all 1,173 lines to "fix(items): one canonical row layout…". On main they belong to the #229 i18n commits. The same applies to every file changed between #198 and #235. - **Suggested landing:** squash-merge in the forge UI, or rebuild `afk/232` as one commit on `ea11f78`. The tree diff against main is correct, at 27 files, so either route lands exactly this change. ### Code review: no findings - The diff matches #232's contract. `elemops.SplitItems`/`ComposeItems` (`internal/elemops/items.go`) are the one owner of the layout. Every reader and writer named in the issue goes through them: - `server/body.go:92` - `elemops/save.go:121` (`ApplyEdit`) and `:152` (`WouldClobberBody`) - `server/forms.go:609` - `server/translation.go:451` - `l10n/hash.go:201` - `l10n/structure.go`, via `elementList` / `openList` / `commit` - `elemops.IsItem` is the only item predicate. `content.MergeText` is the only text merge, and `coalesceText`, `lastText` and the merge in `spliceNodes` are gone. `removecmd/strip.go` is untouched. - Repair and ordering write an owner back only when a fix touches its rows (`orderElements` commits only when `reordered`). `indexOf` cannot return -1 in `indexSource`/`removeSite`, because the split keeps each shortcode by identity in exactly the region `region()` picks. - The rendering assumption holds. `themes/deckle/layouts/shortcodes/img.html` renders nothing when it has a parent, so dropping the blank line before the rows (showcase page, AGENTS.md example) changes no output. - Docs and skills were updated as CLAUDE.md requires: AGENTS.md "Editing content by hand" and "Item collections", the `image`/`text-media`/`cards` sidecar comments, and the `edit-content` and `migrate-site` skills. The drift tests are green in CI. - Conventions: the Conventional Commits title, `Closes #232` and the release note are all present.

dominik.polakovics commented 2026-10-12 20:15:52 +02:00

Author

Owner

Copy link

Correction to the line references in my approval: the SplitItems call sites are internal/elemops/save.go:126 (ApplyEdit) and :153 (WouldClobberBody), and internal/l10n/hash.go:203. The approval cited 121, 152 and 201. The other references are correct.

Correction to the line references in my approval: the `SplitItems` call sites are `internal/elemops/save.go:126` (ApplyEdit) and `:153` (WouldClobberBody), and `internal/l10n/hash.go:203`. The approval cited 121, 152 and 201. The other references are correct.

dominik.polakovics merged commit dcc1067e40 into main 2026-10-12 22:15:51 +02:00

dominik.polakovics deleted branch afk/232 2026-10-12 22:15:51 +02:00

dominik.polakovics referenced this pull request from a commit 2026-10-12 22:15:52 +02:00

fix(items): one canonical row layout owned by a single split/compose (#239)

dominik.polakovics referenced this pull request 2026-10-12 22:46:34 +02:00

tree payload: server-owned child role, slot wrapper and summary verdicts (drift audit 1, 2, 5) #243

dominik.polakovics referenced this pull request 2026-10-12 22:48:42 +02:00

Drift audit: rules implemented twice across engine, server, client and theme #234

dominik.polakovics referenced this pull request 2026-10-12 23:38:47 +02:00

feat(tree): server-owned child role, slot wrapper and summary verdicts #246

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!239

No description provided.