Skip to content

Translation follow-ups + TableDefault class attr widening - #565

Merged
ddon merged 9 commits into
BeamLabEU:devfrom
mdon:followup-translate-batch
May 22, 2026
Merged

Translation follow-ups + TableDefault class attr widening#565
ddon merged 9 commits into
BeamLabEU:devfrom
mdon:followup-translate-batch

Conversation

@mdon

@mdonmdon commented May 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Consolidated follow-ups to the translation sweep (PRs #557#560)
that all merged on 2026-05-21, plus one unrelated TableDefault
fix folded in to keep all phoenix_kit follow-ups in one review
cycle.

1. Parser marker leak (originally #562)

parse_response/2's field-capture boundary regex only terminated
at requested markers. A model that emits an unrequested marker
(e.g. ---TITLE---{{title}} when the caller asked only for
name + description) silently rolled the unrequested block's
content into the previous requested field.

Real-world trigger observed against deepseek-v3.2: a projects
prompt template referenced {{title}} literally (no title
variable was bound for a template resource that only has name +
description), so the AI dutifully emitted ---TITLE---{{title}}
between ---NAME--- and ---DESCRIPTION---. The parser, asked
for only name and description, set name to
"Mitarbeiter-Onboarding---TITLE---{{title}}" because nothing
in its boundary regex matched ---TITLE---.

Fix: bound captures at any ---[A-Z0-9_]+--- marker, not just
the explicitly-requested ones. Codex review then flagged that
the new boundary was not line-anchored — a literal ---WORD---
mid-paragraph (technical content, API docs) would prematurely
terminate. Final form requires \n---<NAME>--- so only markers
on their own line act as boundaries.

Same commit promotes handle_ai_response/2 from defp to
@doc false def so the unit tests can drive it directly, with
the inline OpenAI-shape match replacing the cross-module
PhoenixKitAI.Completion.extract_content/1 call.

2. Validation-order regression test (originally #563)

Backstop for the validation chain in Translation.translate_fields/6:
endpoint → prompt → non-empty → unique-markers → plugin-available.
Stacks multiple input violations and asserts which one wins,
including whitespace-only endpoint/prompt to pin the
trim-then-check contract.

3. Magic-link comment trim (originally #564)

Codex final-review CONCERN on PR #559: long inline comments
explaining the data-phx-skip / 183px / form-ownership root
causes belong in the PR description, not in templates. Trimmed
from 19 lines down to 6 short ones naming the root rules.

4. TableDefault class attr widening (NEW — originally #566)

attr :class, :string on the seven TableDefault.* components
rejected the Phoenix-idiomatic class={[if(...), if(...)]}
pattern at compile time, but every component already accepts
lists internally — they wrap @class inside another list that
Phoenix flattens. Surfaces as a warning in phoenix_kit_ai's
endpoint list table; widening all 7 declarations to :any
clears the warning and lets future consumers use the standard
idiom without relitigating per-component.

Codex review findings (round 2)

Five concerns from a second codex pass, all applied:

Supersedes

All closed with a back-reference to this PR.

Test plan

  • mix compile --warnings-as-errors clean
  • mix format --check-formatted clean on touched files
  • mix test test/phoenix_kit/modules/ai/translation_test.exs — 23/23 pass
  • mix credo --strict clean on touched files
  • Manual: end-to-end SQ retranslation on the dev parent app — should produce clean output with no ---TITLE---{{title}} tail

mdon added 5 commits May 22, 2026 05:21
The boundary regex for each requested field only included OTHER
requested markers as terminators. When a model emitted a marker
the caller didn't ask for, that block's content silently
appended to the preceding requested field's capture.
Real-world trigger: the projects prompt template referenced
`{{title}}` literally (no `title` variable was bound for a
template resource that only has name + description), so the AI
dutifully emitted `---TITLE---{{title}}` between `---NAME---`
and `---DESCRIPTION---`. The parser, asked for only `name` and
`description`, set name to "Mitarbeiter-Onboarding---TITLE---{{title}}"
because nothing in its boundary regex matched `---TITLE---`.
Fix: bound captures at any `---[A-Z0-9_]+---` marker, not just
the explicitly-requested ones. The character class matches what
`marker/1` already produces. The `i` flag still tolerates
lowercased markers in the response. Unrequested markers'
content is dropped silently — they're not in the requested
fields list, so the caller never gets them either way.
Regression test pins the exact real-world response we saw.
Codex final-review findings on PR BeamLabEU#560:
CONCERN: the original "handles OpenAI-shaped response map" test
manually extracted message.content and then called parse_response/2.
The test would still have passed against the pre-fix broken
implementation because it never drove the new handle_ai_response/2
branch. Fixed by:
- Exposing handle_ai_response/2 as @doc false def so the unit
test can drive it directly (instead of going through the
private path).
- Rewrote the test to call Translation.handle_ai_response(map, fields)
with a realistic OpenAI response shape. Now actually fails if
the map-handling branch regresses.
- Added two more handle_ai_response/2 tests covering the raw-binary
legacy/stub path and the malformed-shape error path.
CONCERN: the earlier implementation called
PhoenixKitAI.Completion.extract_content/1, which raises
UndefinedFunctionError in core's test env where the plugin isn't
loaded. Replaced the cross-module call with an inline pattern match
on the OpenAI shape (choices[0].message.content). The shape is
stable (OpenAI-standard), so the inline match is safe; and removing
the dep means the helper works in core's test env and in any host
regardless of which PhoenixKitAI version they pin.
NIT: the test comment now matches the test behavior.
Backstop for the validation chain: `endpoint → prompt → non-empty →
unique-markers → plugin-available`. Stacks multiple input violations
and asserts which rejection wins, so a future refactor that reorders
the `with` chain (e.g. moving `validate_non_empty` before
`validate_uuid`) gets caught immediately.
Codex final review NIT: empty-map (`%{}`) can't also contain
duplicate normalized markers, so the regression-test comments
mentioning "empty fields + dups" were inaccurate. Trim those
mentions so the assertions read straight.
Codex final-review CONCERN: the long inline comments explaining
the data-phx-skip / 183px / form-ownership root causes belong in
the PR description, not in templates where they'll age out faster
than the fix. Kept short comments naming the root rules ('fieldset
default min-width', 'double-swap broke the DOM merge') so a future
edit sees the intent without the wall of text.
Four issues from the codex review, all applied:
PARSER REGRESSION RISK — line-anchor the boundary lookahead
============================================================
The new `(?=---[A-Z0-9_]+---|\z)` boundary terminated capture at
ANY `---<NAME>---` token in the translated content, not just at
markers that started on their own line. Technical content (API
docs, code examples mentioning marker shapes inline) could
prematurely terminate.
Fix: require a newline before the boundary marker
(`\n---[A-Z0-9_]+---`). Real AI-emitted markers always start
their own line, so this preserves the unrequested-marker leak
fix while keeping literal mid-paragraph `---WORD---` tokens
inside the capture. Regression test added.
TEST CLARITY — stale comment + thin malformed-shape coverage
============================================================
The OpenAI-shape test still claimed extraction happens via
`PhoenixKitAI.Completion.extract_content/1`, but the PR removed
that call in favor of an inline pattern match. Comment rewritten.
Malformed-shape coverage previously only exercised an atom
input — wide miss for "what counts as malformed for an
OpenAI-shaped map specifically". Expanded to cover empty
`choices`, non-binary content (nil content + tool_call /
structured-parts), and `message`-missing.
VALIDATION-ORDER TEST GAPS — whitespace-only inputs
====================================================
Pinned the trim-then-check contract on `validate_uuid/2`
(`" "` rejects the same way as `""`). Catches a future
"strict equality" refactor that would silently accept
whitespace-only endpoint/prompt uuids.
`attr :class, :string` rejects a list at compile time, but every
TableDefault component already accepts lists internally — they
wrap `@class` inside another list that Phoenix flattens. The
:string declaration is just a stale guard from before the
internal usage settled.
Surfaces as a warning when a consumer uses the Phoenix-standard
`class={[if(...), if(...)]}` idiom for conditional classes:
warning: attribute "class" in component
PhoenixKitWeb.Components.Core.TableDefault.table_default_row/1
must be a :string, got: [if !endpoint.enabled do
"opacity-60" end, ...]
endpoints.html.heex:241
Triggered by `phoenix_kit_ai`'s endpoint list table (the only
current external consumer using a conditional class list). Same
fix applied uniformly to all seven `attr :class` declarations in
the file (`table_default/1`, `table_default_header/1`,
`table_default_row/1`, `table_default_body/1`,
`table_default_cell/1`, `table_default_header_cell/1`,
`table_default_search/1`) so future consumers don't relitigate
the same warning per-component.
Defaults are kept as strings (`""` and `"bg-base-300"`) — only
the type widens.
@mdonmdon changed the title Translation follow-ups: parser marker leak + validation-order test + magic_link comment trimTranslation follow-ups + TableDefault class attr wideningMay 22, 2026
mdon added 2 commits May 22, 2026 13:17
Two related polish items applied to the shared multilang form
components in core.
1. Info text moved into a tooltip on the Content Language header
================================================================
The standalone alert block ("Use the language tabs below to
translate this record's content. The primary language (marked
with a star) is required. Other languages are optional — any
empty fields will fall back to the primary language value.")
was a wall of text that dominated the form chrome despite being
mostly static explanation. Boss wanted it behind an info icon
next to the "Content Language" header that reveals on
hover/focus.
Implementation: replaced the alert block with a daisyUI tooltip
on a `hero-information-circle` icon, gated by the existing
`show_info` attr. The `[--tooltip-max-width:24rem]` arbitrary
property keeps the long explanation readable rather than
wrapping into a thin column.
Docstring on `show_info` updated to reflect the new behavior
(it now controls the tooltip icon, not a standalone alert).
Consumers that explicitly pass `show_info={false}` (e.g.
`phoenix_kit_entities`) are unaffected — they get no icon.
Consumers using the default see the icon instead of the alert,
which is the intended UX change.
2. Default fallback skeleton uses base-content/15 + animate-pulse
=================================================================
daisyUI's `.skeleton` class resolves to ~8% opacity base-content
grey, which is nearly invisible on `bg-base-100` cards.
Multiple consumers (projects, others) reported the loading state
looking like a "blank page" — the skeleton was rendering, but
the user couldn't see it.
Replaced the inline `<div class="skeleton ...">` defaults inside
`multilang_fields_wrapper` with `bg-base-content/15 rounded
animate-pulse` blocks. 15% opacity gives readable contrast on
every theme, and Tailwind's `animate-pulse` carries the loading
affordance.
Consumers that provide their own `<:skeleton>` slot are
unaffected — they own the placeholder markup.
Codex final-review finding (BUG-MEDIUM): when a model emits a
marker with no body (e.g. `---TITLE---\n---BODY---\n...`), the
regex's `\s*\n?` happily consumes the inter-marker newline,
`(.+?)` starts capturing at `---BODY---\nBody`, and the lookahead
(which requires a leading `\n`) can't rescue. End state: TITLE
field captures the next section's content (`---BODY---\nBody`)
instead of being recorded as empty.
Verified the bug against deepseek-v3.2-shaped input via a quick
local repro:
---TITLE---
---BODY---
Body content
title = "---BODY---\nBody content" ← wrong
body = "Body content"
Fix: post-process the captured value. If it starts with a
marker-shaped token (`---[A-Z0-9_]+---`), the original section
was empty — return `""` instead of misattributing the next
field's content. The earlier line-anchored boundary couldn't
handle this case without breaking the inline-marker case
(content with literal `---WORD---`), so the post-process guard
threads the needle.
Trade-off: a translation whose first non-whitespace content
literally starts with `---WORD---` (e.g. a Klingon UI string
that genuinely renders as `---FOO---`) would be misclassified
as empty. Acceptable: that's a much rarer edge than the AI
emitting an empty marker before the next field.
Test pin: new `"empty section between two markers returns empty
string, not next field's content"` asserts the corrected behavior
on the exact shape codex flagged.
@ddon
ddon merged commit 2a12a53 into BeamLabEU:devMay 22, 2026
ddon pushed a commit that referenced this pull request May 22, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 22, 2026
PR #565 post-review follow-ups:
- extract_section/3: capture group (.+?) -> (.*?) so a present-but-empty
trailing marker resolves to "" (matching empty middle sections) instead
of being reported as missing_fields. Absent markers still -> missing_fields.
- handle_ai_response/2: add KEEP IN SYNC note flagging the inline OpenAI
extraction as a deliberate second source of truth vs the AI plugin's
extract_content/1.
- Tests: present-but-empty trailing -> "", absent trailing -> missing_fields.
- Update CLAUDE_REVIEW.md with applied fixes (incl. show_info no-change rationale).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 22, 2026
Release rollup for PR #565 translation follow-ups + post-review fixes:
parser marker-leak fix, trailing-empty section consistency, TableDefault
:class widening to :any, multilang tooltip/skeleton, inline AI extraction,
and etcher/fresco/floki dependency bumps. No DB migration (still V121).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 25, 2026
Per dev_docs/quality_sweep.md Phase 1 playbook: every merged PR's
folder gets a FOLLOW_UP.md documenting how each review finding was
resolved (or skipped with rationale).
PR #565 review findings:
- NITPICK #2 (trailing-empty asymmetry) — fixed post-merge in 79f6dc5
- MEDIUM (extraction divergence comment) — fixed post-merge in 79f6dc5
- NITPICK #3 (show_info gated on show_header) — skipped, doc-only resolution
No new code changes — this is the after-action artifact only.
Sign up for freeto 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.

2 participants

@mdon@ddon