Cloonar/deckle

Fork 0

translate-plan and translate evaluate the translatable rule separately; plan approves what translate refuses #275

New issue

Closed

opened 2026-10-13 21:39:52 +02:00 by dominik.polakovics · 0 comments

dominik.polakovics commented

2026-10-13 21:39:52 +02:00

Owner

This was generated by AI during triage.

Summary

GET /api/page/translate-plan promises what POST /api/page/translate will do, but the two evaluate "can this page be translated into lang, and which subpages will be created" separately, and the copies differ. One read-only function in internal/pageops must own that decision, with TranslatePlan reporting from its result and Translate executing it.

Current behavior

pageops.TranslatePlan checks only that no lang translation of the page exists, then counts every untranslated page below it. pageops.Translate additionally refuses the page when a sibling holds its address in lang (409 translation.addressTaken) and skips every subpage whose address is taken or whose source does not parse. It also silently skips a subpage whose rendering fails (l10n.TranslateDocument, front matter, serializer), which docs/api.md does not list as a skip reason.

Reproduced on main at bf70703 with the sibling collision TestTranslationSlugCollision already builds (a sibling's de translation carrying slug = 'hosting') plus a subpage whose source does not parse:

Page services/hosting/_index.md: plan answers {subpages: 0} with 200, translate answers 409 translation.addressTaken.
Page services/_index.md with subpages: plan answers {subpages: 2}, translate creates no subpage and lists both in skipped.

The handler comment on handlePageTranslatePlan and TranslatePlan's doc comment ("Its guards are Translate's") both claim the guards are the same. So do the client comments on fetchTranslatePlan and TranslateDialog.

Desired behavior

One pure, read-only function in internal/pageops computes the outcome of a translate: the page's own refusal (translation exists, address taken, source does not parse) and, with subpages, the list of subpages that will be created and the list that will be skipped with the reason (address taken, source does not parse).
Translate runs that function and then performs only the writes (stamps, new files, rollback) from its result. Its documented error set is unchanged except as the third decision says.
TranslatePlan runs the same function and reports from its result: it refuses exactly what Translate refuses, and subpages counts only pages the translate will actually create.
The stale comments named above say what is now true.

Decisions

The plan answer stays {subpages: number}, counting only pages the translate will create. The dialog's only question is "also create the N subpages", and the translate's answer already lists skipped afterwards. A skipped count or list on the plan is a separate enhancement.
The owner function parses every candidate page (the page and each subpage). The outcome cannot be known otherwise, and the translate reads them all anyway. The plan therefore gains 422 translation.sourceParse {page, detail} for the page itself, exactly as the translate answers it.
A subpage whose rendering fails is an error, not a skip. The skip reasons are page states: address taken, source does not parse. A failure of l10n.TranslateDocument, the front-matter write or the serializer is not a page state, depends on mode (which the plan does not have), and is undocumented as a skip. It answers the same 400/422 the page itself would. This keeps the owner function free of mode, label and schemas, and makes the code match what docs/api.md already says.
Order of refusals stays the translate's: translation exists, then address taken, then source parse.

Key interfaces

pageops.TranslatePlan(root, page, lang) (int, error) and pageops.Translate(root, TranslateParams) (TranslateResult, error) keep their signatures; both call the new owner.
The new owner function, something like planTranslation(root, page, lang string, subpages bool), returns the pages to create, the pages to skip, and whatever it already read (parsed sources, stamped bytes) so Translate need not read again. Name and exact shape are the implementer's.
TranslateResult.Skipped keeps its meaning: subpages left untranslated because of the two documented reasons.
translationAddressTaken builds the page tree on every call, so a plan over N subpages builds it N+1 times; building it once inside the owner is welcome but not required.
Starting hints: internal/pageops/translation.go (both copies and translationBytes), internal/pageops/translation_settings.go (translationAddressTaken), internal/server/translation_pages.go (both handlers), web/src/api/translate.ts and web/src/components/TranslateDialog.tsx (comments only).

Acceptance criteria

The rule "is this page translatable into lang, and which subpages will be created" is evaluated in exactly one function in internal/pageops, called by both TranslatePlan and Translate.
TranslatePlan answers 409 translation.addressTaken with the same args as Translate when a sibling holds the page's address in lang.
TranslatePlan answers 422 translation.sourceParse when the page's own source does not parse, as Translate does.
With subpages, the plan's count equals len(Created) - 1 of the translate that follows in the same state, including when a subpage's address is taken or its source does not parse.
A subpage whose rendering fails makes the translate fail with the page's own error; it is no longer listed in Skipped.
GET /api/page/translate-plan answers 409 translation.addressTaken and 422 translation.sourceParse through the route, with the same body the translate route gives.
docs/api.md ### GET /api/page/translate-plan: the sentence "It neither checks the address … nor parses the source" and the "subpages the translate would then skip … are counted" clause are gone; 409 translation.addressTaken and 422 translation.sourceParse are in its errors. ### POST /api/page/translate describes the third decision if its wording needs it.
The handler, TranslatePlan, fetchTranslatePlan and TranslateDialog comments no longer claim guards the code does not share; they name the actual set.
TestTranslationErrorCodes in internal/pageops/codes_test.go covers the plan's translation.addressTaken.

How to verify

go test ./internal/pageops ./internal/server and go vet ./... pass.
New cases in internal/pageops/translation_test.go: the sibling collision from TestTranslationSlugCollision, asserting the plan's error kind and code equal the translate's; and a subpage set with one address taken and one unparsable source, asserting the plan's count equals the following translate's created subpages (the #280 pattern: executed plan equals dry run).
New case in internal/server/translation_pages_test.go: the plan route answers 409 translation.addressTaken with the translate route's args.
web/src tests unchanged and passing (npm test in web/); the wire shape does not change.
docs/docs_test.go and the route test in internal/server pass after the docs/api.md edit.

Assumptions to check

That no web/src code relies on a translate answer listing a render-failed subpage in skipped (third decision); web/src/api/translate.ts parses skipped but the dialog does not display it.
That the plan handler has access to nothing it would newly need: the owner function must not require mode, label or schemas, or the plan route cannot call it.

Out of scope

A skipped count or list on the plan answer, and any dialog change to show it.
Changing which addresses count as taken (translationAddressTaken's rule) or the translate's write and rollback behaviour.
The skills and scaffold/site/AGENTS.md: no skill walks through translating a page and the manual does not describe this route. The theme is not involved.

Blocked by

None - can start immediately

Original report

Found while reviewing #274 (docs/api.md for #264). The docs describe the route as it behaves today; this is the code fix.

Problem

GET /api/page/translate-plan promises what POST /api/page/translate will do, but the two evaluate the "can this page be translated into lang" rule separately, and the copies differ.

pageops.TranslatePlan (internal/pageops/translation.go:177) checks only that no lang translation of the page exists, then counts untranslatedBelow. pageops.Translate (translation.go:244) additionally:

refuses the page when a sibling already holds its address in lang (translationAddressTaken → 409 translation.addressTaken);
skips every subpage whose address is taken, or whose source does not parse (translationBytes), listing it in Skipped.

So the dialog's plan can answer {subpages: 3} and the translate then either fails with 409 translation.addressTaken or creates fewer pages than it announced. The handler comment on handlePageTranslatePlan (internal/server/translation_pages.go:137-141) claims it "refuses exactly what POST /api/page/translate would", which is false. TranslatePlan's own doc comment ("Its guards are Translate's") is false too.

This is the same class of defect as the media folder/delete-plan vs folder/delete mismatch in #273, and the CLAUDE.md rule "every rule has exactly one owner": a predicate (is this page translatable into lang; which subpages would be created) evaluated in two places.

Expected

One function owns the decision, and both the plan and the translate call it:

Extract from Translate the pure, read-only part that computes the outcome: the page's own refusal (exists / address taken / source does not parse) and, with subpages, the list of subpages that will be created and the list that will be skipped, with the reason. Something like planTranslation(root, page, lang, mode, subpages) (translatePlan, error) in internal/pageops, returning the pages to create and the pages to skip.
Translate runs that function, then does only the writes (stamps, new files, rollback) from its result. Its error set stays as documented.
TranslatePlan runs the same function and reports from its result. It then refuses exactly what Translate refuses, and subpages counts only pages the translate will actually create.

Decisions for the implementer (either is fine, say which in the PR):

Whether the plan answer stays {subpages} or grows a skipped count / list so the dialog can say "3 created, 1 skipped". Growing it is the more honest wire shape; keeping it is the smaller change.
Whether the plan parses each subpage's source to detect the "does not parse" skip, which Translate does anyway and the plan currently avoids. The owner function decides this once; the plan should not special-case it.

Where the rule is evaluated today (both copies)

internal/pageops/translation.go — TranslatePlan (:177) and Translate (:244-300, the guards and the per-subpage skip loop).
Callers: internal/server/translation_pages.go handlePageTranslatePlan (:142) and handlePageTranslate.
Client: web/src/components/TranslateDialog.tsx (:62-77) consumes {subpages} via web/src/api/translate.ts fetchTranslatePlan. It does not re-derive the rule; only the wire shape matters there.

Tests

internal/pageops/translation_test.go: add a case where a sibling holds the address in lang — plan must answer the same 409 the translate does; and a case with a subpage whose address is taken or whose source does not parse — plan's count equals len(Created) of the following translate.
internal/server/translation_pages_test.go: the plan route answers 409 translation.addressTaken with the same args as the translate route.
internal/pageops/codes_test.go if the plan gains the code.
web/src/test/translateApi.test.ts and mockApi.ts only if the wire shape changes.

Docs

docs/api.md ### GET /api/page/translate-plan (landed by #274): drop the sentence "It neither checks the address (translation.addressTaken) nor parses the source" and the "subpages the translate would then skip … are counted" clause; add 409 translation.addressTaken to its errors. The #post-apipagetranslate section is unchanged unless the plan shape grows.
The two stale code comments named above (also the class of defect #272 lists).
No skill walks through translating a page, and scaffold/site/AGENTS.md does not describe this route, so neither changes. The theme is not involved.

> *This was generated by AI during triage.* ## Summary `GET /api/page/translate-plan` promises what `POST /api/page/translate` will do, but the two evaluate "can this page be translated into `lang`, and which subpages will be created" separately, and the copies differ. One read-only function in `internal/pageops` must own that decision, with `TranslatePlan` reporting from its result and `Translate` executing it. ## Current behavior `pageops.TranslatePlan` checks only that no `lang` translation of the page exists, then counts every untranslated page below it. `pageops.Translate` additionally refuses the page when a sibling holds its address in `lang` (`409 translation.addressTaken`) and skips every subpage whose address is taken or whose source does not parse. It also silently skips a subpage whose rendering fails (`l10n.TranslateDocument`, front matter, serializer), which `docs/api.md` does not list as a skip reason. Reproduced on `main` at bf70703 with the sibling collision `TestTranslationSlugCollision` already builds (a sibling's `de` translation carrying `slug = 'hosting'`) plus a subpage whose source does not parse: - Page `services/hosting/_index.md`: plan answers `{subpages: 0}` with 200, translate answers `409 translation.addressTaken`. - Page `services/_index.md` with subpages: plan answers `{subpages: 2}`, translate creates no subpage and lists both in `skipped`. The handler comment on `handlePageTranslatePlan` and `TranslatePlan`'s doc comment ("Its guards are Translate's") both claim the guards are the same. So do the client comments on `fetchTranslatePlan` and `TranslateDialog`. ## Desired behavior - One pure, read-only function in `internal/pageops` computes the outcome of a translate: the page's own refusal (translation exists, address taken, source does not parse) and, with subpages, the list of subpages that will be created and the list that will be skipped with the reason (address taken, source does not parse). - `Translate` runs that function and then performs only the writes (stamps, new files, rollback) from its result. Its documented error set is unchanged except as the third decision says. - `TranslatePlan` runs the same function and reports from its result: it refuses exactly what `Translate` refuses, and `subpages` counts only pages the translate will actually create. - The stale comments named above say what is now true. ## Decisions - **The plan answer stays `{subpages: number}`**, counting only pages the translate will create. The dialog's only question is "also create the N subpages", and the translate's answer already lists `skipped` afterwards. A `skipped` count or list on the plan is a separate enhancement. - **The owner function parses every candidate page** (the page and each subpage). The outcome cannot be known otherwise, and the translate reads them all anyway. The plan therefore gains `422 translation.sourceParse {page, detail}` for the page itself, exactly as the translate answers it. - **A subpage whose rendering fails is an error, not a skip.** The skip reasons are page states: address taken, source does not parse. A failure of `l10n.TranslateDocument`, the front-matter write or the serializer is not a page state, depends on `mode` (which the plan does not have), and is undocumented as a skip. It answers the same `400`/`422` the page itself would. This keeps the owner function free of `mode`, `label` and schemas, and makes the code match what `docs/api.md` already says. - **Order of refusals stays the translate's**: translation exists, then address taken, then source parse. ## Key interfaces - `pageops.TranslatePlan(root, page, lang) (int, error)` and `pageops.Translate(root, TranslateParams) (TranslateResult, error)` keep their signatures; both call the new owner. - The new owner function, something like `planTranslation(root, page, lang string, subpages bool)`, returns the pages to create, the pages to skip, and whatever it already read (parsed sources, stamped bytes) so `Translate` need not read again. Name and exact shape are the implementer's. - `TranslateResult.Skipped` keeps its meaning: subpages left untranslated because of the two documented reasons. - `translationAddressTaken` builds the page tree on every call, so a plan over N subpages builds it N+1 times; building it once inside the owner is welcome but not required. - Starting hints: `internal/pageops/translation.go` (both copies and `translationBytes`), `internal/pageops/translation_settings.go` (`translationAddressTaken`), `internal/server/translation_pages.go` (both handlers), `web/src/api/translate.ts` and `web/src/components/TranslateDialog.tsx` (comments only). ## Acceptance criteria - [ ] The rule "is this page translatable into `lang`, and which subpages will be created" is evaluated in exactly one function in `internal/pageops`, called by both `TranslatePlan` and `Translate`. - [ ] `TranslatePlan` answers `409 translation.addressTaken` with the same args as `Translate` when a sibling holds the page's address in `lang`. - [ ] `TranslatePlan` answers `422 translation.sourceParse` when the page's own source does not parse, as `Translate` does. - [ ] With subpages, the plan's count equals `len(Created) - 1` of the translate that follows in the same state, including when a subpage's address is taken or its source does not parse. - [ ] A subpage whose rendering fails makes the translate fail with the page's own error; it is no longer listed in `Skipped`. - [ ] `GET /api/page/translate-plan` answers `409 translation.addressTaken` and `422 translation.sourceParse` through the route, with the same body the translate route gives. - [ ] `docs/api.md` `### GET /api/page/translate-plan`: the sentence "It neither checks the address … nor parses the source" and the "subpages the translate would then skip … are counted" clause are gone; `409 translation.addressTaken` and `422 translation.sourceParse` are in its errors. `### POST /api/page/translate` describes the third decision if its wording needs it. - [ ] The handler, `TranslatePlan`, `fetchTranslatePlan` and `TranslateDialog` comments no longer claim guards the code does not share; they name the actual set. - [ ] `TestTranslationErrorCodes` in `internal/pageops/codes_test.go` covers the plan's `translation.addressTaken`. ## How to verify - `go test ./internal/pageops ./internal/server` and `go vet ./...` pass. - New cases in `internal/pageops/translation_test.go`: the sibling collision from `TestTranslationSlugCollision`, asserting the plan's error kind and code equal the translate's; and a subpage set with one address taken and one unparsable source, asserting the plan's count equals the following translate's created subpages (the #280 pattern: executed plan equals dry run). - New case in `internal/server/translation_pages_test.go`: the plan route answers `409 translation.addressTaken` with the translate route's args. - `web/src` tests unchanged and passing (`npm test` in `web/`); the wire shape does not change. - `docs/docs_test.go` and the route test in `internal/server` pass after the `docs/api.md` edit. ## Assumptions to check - That no `web/src` code relies on a translate answer listing a render-failed subpage in `skipped` (third decision); `web/src/api/translate.ts` parses `skipped` but the dialog does not display it. - That the plan handler has access to nothing it would newly need: the owner function must not require `mode`, `label` or schemas, or the plan route cannot call it. ## Out of scope - A `skipped` count or list on the plan answer, and any dialog change to show it. - Changing which addresses count as taken (`translationAddressTaken`'s rule) or the translate's write and rollback behaviour. - The skills and `scaffold/site/AGENTS.md`: no skill walks through translating a page and the manual does not describe this route. The theme is not involved. ## Blocked by None - can start immediately ## Original report Found while reviewing #274 (docs/api.md for #264). The docs describe the route as it behaves today; this is the code fix. ## Problem `GET /api/page/translate-plan` promises what `POST /api/page/translate` will do, but the two evaluate the "can this page be translated into lang" rule separately, and the copies differ. `pageops.TranslatePlan` (internal/pageops/translation.go:177) checks only that no `lang` translation of the page exists, then counts `untranslatedBelow`. `pageops.Translate` (translation.go:244) additionally: - refuses the page when a sibling already holds its address in `lang` (`translationAddressTaken` → `409 translation.addressTaken`); - skips every subpage whose address is taken, or whose source does not parse (`translationBytes`), listing it in `Skipped`. So the dialog's plan can answer `{subpages: 3}` and the translate then either fails with `409 translation.addressTaken` or creates fewer pages than it announced. The handler comment on `handlePageTranslatePlan` (internal/server/translation_pages.go:137-141) claims it "refuses exactly what POST /api/page/translate would", which is false. `TranslatePlan`'s own doc comment ("Its guards are Translate's") is false too. This is the same class of defect as the media `folder/delete-plan` vs `folder/delete` mismatch in #273, and the CLAUDE.md rule "every rule has exactly one owner": a predicate (is this page translatable into lang; which subpages would be created) evaluated in two places. ## Expected One function owns the decision, and both the plan and the translate call it: - Extract from `Translate` the pure, read-only part that computes the outcome: the page's own refusal (exists / address taken / source does not parse) and, with subpages, the list of subpages that will be created and the list that will be skipped, with the reason. Something like `planTranslation(root, page, lang, mode, subpages) (translatePlan, error)` in `internal/pageops`, returning the pages to create and the pages to skip. - `Translate` runs that function, then does only the writes (stamps, new files, rollback) from its result. Its error set stays as documented. - `TranslatePlan` runs the same function and reports from its result. It then refuses exactly what `Translate` refuses, and `subpages` counts only pages the translate will actually create. Decisions for the implementer (either is fine, say which in the PR): - Whether the plan answer stays `{subpages}` or grows a `skipped` count / list so the dialog can say "3 created, 1 skipped". Growing it is the more honest wire shape; keeping it is the smaller change. - Whether the plan parses each subpage's source to detect the "does not parse" skip, which `Translate` does anyway and the plan currently avoids. The owner function decides this once; the plan should not special-case it. ## Where the rule is evaluated today (both copies) - `internal/pageops/translation.go` — `TranslatePlan` (:177) and `Translate` (:244-300, the guards and the per-subpage skip loop). - Callers: `internal/server/translation_pages.go` `handlePageTranslatePlan` (:142) and `handlePageTranslate`. - Client: `web/src/components/TranslateDialog.tsx` (:62-77) consumes `{subpages}` via `web/src/api/translate.ts` `fetchTranslatePlan`. It does not re-derive the rule; only the wire shape matters there. ## Tests - `internal/pageops/translation_test.go`: add a case where a sibling holds the address in `lang` — plan must answer the same 409 the translate does; and a case with a subpage whose address is taken or whose source does not parse — plan's count equals `len(Created)` of the following translate. - `internal/server/translation_pages_test.go`: the plan route answers `409 translation.addressTaken` with the same args as the translate route. - `internal/pageops/codes_test.go` if the plan gains the code. - `web/src/test/translateApi.test.ts` and `mockApi.ts` only if the wire shape changes. ## Docs - `docs/api.md` `### GET /api/page/translate-plan` (landed by #274): drop the sentence "It neither checks the address (`translation.addressTaken`) nor parses the source" and the "subpages the translate would then skip … are counted" clause; add `409 translation.addressTaken` to its errors. The `#post-apipagetranslate` section is unchanged unless the plan shape grows. - The two stale code comments named above (also the class of defect #272 lists). - No skill walks through translating a page, and `scaffold/site/AGENTS.md` does not describe this route, so neither changes. The theme is not involved.

dominik.polakovics added the

needs-triage

bug

labels

2026-10-13 21:39:52 +02:00

dominik.polakovics added

ready-for-agent

and removed

needs-triage

labels

2026-10-14 09:10:24 +02:00

dominik.polakovics referenced this issue from a commit