fix: block empty meta titles #1030
Conversation
briantstephan
changed the title
commit: |
briantstephan
requested review from
asanehisa,
benlife5,
jwartofsky-yext and
mkilpatrick
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors error tracking to be source-aware by introducing ErrorSource and ErrorDetail types and replacing a single errorCount with per-source errorCounts, errorSources, and errorDetails in ErrorContext. Adds meta-title validation (MetaTitleField, MetaTitleValidationReporter) and a ValidationReporters aggregator. Centralizes page-set locale extraction via getPageSetLocales and updates multiple consumers and migrations. Adds publish error message construction (getPublishErrorMessage) and wires dynamic publish guidance into LayoutEditor, LayoutHeader, and ComponentErrorBoundary. Updates many locale JSON files to add publish-related keys and remove fixErrorsToPublish. Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as LayoutEditor
participant Reporters as ValidationReporters / MetaTitleValidationReporter
participant ErrorCtx as ErrorContext
participant Formatter as publishErrors.ts
participant UI as LayoutHeader / Tooltip
User->>Editor: open or edit document
Editor->>Reporters: mount validators
Reporters->>Reporters: compute missing locales / hasError
alt validator finds issue
Reporters->>ErrorCtx: incrementErrorCount("metaTitle" or "component")
Reporters->>ErrorCtx: setErrorDetails(source, { missingLocales: [...] })
else no issue
Reporters->>ErrorCtx: clearErrorDetails(source)
end
User->>Editor: click Publish
Editor->>ErrorCtx: read errorCount, errorSources, errorDetails
alt errorCount > 0
Editor->>Formatter: getPublishErrorMessage(errorSources, errorDetails)
Formatter-->>Editor: localized message
Editor->>UI: display toast.error(message)
end
Editor->>UI: pass errorSources & errorDetails for tooltip
UI-->>User: render dynamic publish guidance
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@packages/visual-editor/locales/platform/fi/visual-editor.json`:
- Around line 549-550: The interpolation tokens in the translations for
fixErrorsToPublishMultiple and fixErrorsToPublishSingle are in Finnish
({{toiminnot}} and {{toiminta}}) so they won't match the variable names passed
at runtime; update those tokens to the English variable names used by the
code—replace {{toiminnot}} with {{actions}} in fixErrorsToPublishMultiple and
replace {{toiminta}} with {{action}} in fixErrorsToPublishSingle so the i18n
library can correctly interpolate the values.
In `@packages/visual-editor/locales/platform/ja/visual-editor.json`:
- Around line 549-550: Update the interpolation placeholders in the Japanese
locale entries: for the key fixErrorsToPublishMultiple replace the current
"{{アクション}}" placeholder with "{{actions}}", and for fixErrorsToPublishSingle
replace "{{アクション}}" with "{{action}}", so they match the application variable
names used at runtime.
In `@packages/visual-editor/locales/platform/lt/visual-editor.json`:
- Around line 549-550: The Lithuanian locale key fixErrorsToPublishMultiple
currently uses the translated interpolation token {{veiksmai}} which won't be
replaced at runtime; update the value for fixErrorsToPublishMultiple to use the
English token name {{actions}} (matching the code that supplies { actions: "..."
}) while leaving fixErrorsToPublishSingle unchanged.
- Line 549: Update the translation values for the message keys to use the
expected English template variable names instead of localized tokens: for
fixErrorsToPublishMultiple replace any localized placeholder (e.g. {{veiksmai}},
{{toiminnot}}, {{アクション}}, {{radnje}}, {{acțiuni}}) with {{actions}}; for
fixErrorsToPublishSingle replace localized placeholders (e.g. {{toiminta}},
{{アクション}}, {{acțiune}}, {{akcja}}) with {{action}}; ensure the JSON strings for
keys fixErrorsToPublishMultiple and fixErrorsToPublishSingle across the affected
locale files use exactly {{actions}} and {{action}} respectively so template
substitution in the code will work.
In `@packages/visual-editor/locales/platform/pl/visual-editor.json`:
- Around line 550-551: The Polish locale has a wrong interpolation variable in
the "fixErrorsToPublishSingle" entry: replace the localized placeholder
"{{akcja}}" with the correct interpolation key "{{action}}" so it matches the
variable used by the code (and the plural "fixErrorsToPublishMultiple"); update
the value for "fixErrorsToPublishSingle" to use "{{action}}" exactly.
In `@packages/visual-editor/locales/platform/sv/visual-editor.json`:
- Around line 677-681: The Swedish locale contains inconsistent English text in
the publishError object: update the value of metaTitleWithLocales to use the
Swedish term "Meta Titel" (matching metaTitle) instead of "Meta Title"; locate
the publishError object and edit the metaTitleWithLocales string to replace
"Meta Title" with "Meta Titel" so both metaTitle and metaTitleWithLocales are
consistent.
🧹 Nitpick comments (5)
packages/visual-editor/src/utils/getPageMetadata.test.ts (1)
146-178: Consider adding an edge-case test whereprimaryLocale === currentLocale.When the current locale is the primary locale and its title is empty, the fallback should skip it (per the
continueguard) and fall through to"en". A test for this scenario (e.g.,locale: "fr",primaryLocale: "fr", emptyfrtitle, non-emptyentitle) would document this behavior.packages/visual-editor/src/utils/getPageMetadata.ts (1)
38-60: Redundant early-exit check on Line 45.
metadata.titleis guaranteed to be""when entering the loop (guarded by Line 39), and thebreakon Line 57 already exits after a successful assignment. The check on Line 45 can never be true and is dead code.♻️ Suggested simplification
for (const fallbackLocale of fallbackLocales) { - if (metadata.title !== "") { - break; - } if (fallbackLocale === currentLocale) { continue; }packages/visual-editor/src/internal/puck/components/MetaTitleField.tsx (1)
27-40:errorMessageis computed even whenmissingLocalesis empty.The i18n call runs on every render regardless of whether there are missing locales. Since the message is only displayed when
missingLocales.length > 0(Line 56), you could guard the computation or wrap it inuseMemo.♻️ Suggested optimization
- const errorMessage = - missingLocales.length === 1 + const errorMessage = missingLocales.length === 0 + ? "" + : missingLocales.length === 1 ? pt( "metaTitleMissingLocale",packages/visual-editor/src/internal/puck/components/MetaTitleValidationReporter.tsx (1)
35-38:useMemoalways recomputes here — consider simplifying to a plain expression.Since
missingLocalesis a new array reference on every render, theuseMemodeps always trigger recomputation. The memo provides no caching benefit — it's the resulting string (compared by value in theuseEffectdeps) that stabilizes re-runs, not the memo itself.♻️ Suggested simplification
- const missingLocalesKey = React.useMemo( - () => missingLocales.join("|"), - [missingLocales] - ); + const missingLocalesKey = missingLocales.join("|");packages/visual-editor/src/utils/metaTitleValidation.ts (1)
30-38: Minor:JSON.parse("")always throws — consider a guard.
streamDocument?._pageset ?? ""coalesces to""when_pagesetis absent, andJSON.parse("")unconditionally throws aSyntaxError. This works because thecatchswallows it, but it means the happy-path fallback always takes the exception path. A small guard avoids the unnecessary throw.♻️ Optional: skip parse when there's nothing to parse
try { - const parsedPageSet = JSON.parse(streamDocument?._pageset ?? ""); + const raw = streamDocument?._pageset; + if (!raw) { + return [DEFAULT_LOCALE]; + } + const parsedPageSet = JSON.parse(raw); const fromPageSet = normalizeLocales(parsedPageSet?.scope?.locales); if (fromPageSet.length > 0) { return fromPageSet; } } catch { // ignore parse issues and fall through to default locale }
packages/visual-editor/src/internal/puck/components/MetaTitleField.tsx
Outdated
Show resolved
Hide resolved
| templateMetadata?: TemplateMetadata, | ||
| streamDocument?: StreamDocument | ||
| ): string[] => { | ||
| const fromTemplate = normalizeLocales(templateMetadata?.locales); |
There was a problem hiding this comment.
weird var name. Why do we look in both places? Is it not sent on one in certain cases?
There was a problem hiding this comment.
In TranslatableStringField.tsx we were checking templateMetadata first and using the streamDocument as a fallback, but I agree the streamDocument should always be a reliable way to get the locales. Updated to no longer consider templateMetadata.
packages/visual-editor/src/internal/components/InternalLayoutEditor.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/internal/puck/components/meta-title/MetaTitleValidationReporter.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/visual-editor/locales/platform/es/visual-editor.json`:
- Around line 672-676: The two localization keys under publishError (metaTitle
and metaTitleWithLocales) have inconsistent sentence casing; update
metaTitleWithLocales so it matches metaTitle (start with lowercase "complete")
to keep fragments consistent when injected into fixErrorsToPublish, e.g., change
the initial "Complete" to "complete" in metaTitleWithLocales; ensure only the
casing is adjusted and the placeholder {{locales}} remains unchanged.
In `@packages/visual-editor/locales/platform/pt/visual-editor.json`:
- Line 598: The keys metaTitleMissingLocales and metaTitleWithLocales use
different Portuguese terms for "locales" ("localidade(s)" vs "locais"); update
the translation strings to use a single consistent term (e.g., replace
"localidade(s)" with "locais" in metaTitleMissingLocales) and ensure the same
term is used in the related keys around metaTitleWithLocales (lines covering the
group) so all messages refer to locales identically.
In `@packages/visual-editor/locales/platform/tr/visual-editor.json`:
- Around line 673-677: The translation for the JSON key "metaTitleWithLocales"
contains a typo ("yerel ayılar için"); update the value string to use the
correct word "ayarlar" so it reads "Üst düzey Sayfa bileşeninin ayarlarına
giderek aşağıdaki yerel ayarlar için Meta Başlığını doldurun: {{locales}}",
matching the other entries like "metaTitle" and ensuring consistency across
translations.
In
`@packages/visual-editor/src/internal/puck/components/meta-title/MetaTitleValidationReporter.tsx`:
- Around line 50-56: The second useEffect in MetaTitleValidationReporter sets
error details when hasError is true but doesn't clear them on unmount; add a
cleanup that clears "metaTitle" error details if the component unmounts while
hasError is true. Update the effect (the one that calls
setErrorDetails("metaTitle", { missingLocales }) /
clearErrorDetails("metaTitle")) to return a cleanup function like: if (hasError)
clearErrorDetails("metaTitle"); so stale error details are removed on unmount or
when dependencies change.
🧹 Nitpick comments (5)
packages/visual-editor/locales/platform/en-GB/visual-editor.json (1)
597-597: Consider using i18next pluralization instead of the static"locale(s)"pattern.i18next supports
_one/_othersuffixed keys for proper pluralization. The current"locale(s)"will always render literally, even when there's exactly one locale. This affects all 24+ locale files adopting the same pattern.Suggested keys
- "metaTitleMissingLocales": "Meta title is missing for locale(s): {{locales}}", + "metaTitleMissingLocales_one": "Meta title is missing for locale: {{locales}}", + "metaTitleMissingLocales_other": "Meta title is missing for locales: {{locales}}",The call site would pass
{ count: localesList.length, locales: localesList.join(", ") }to let i18next pick the right variant.packages/visual-editor/src/utils/pageSetLocales.ts (1)
5-17:normalizeLocalesis duplicated inmetaTitleValidation.ts.This helper is copy-pasted identically in
packages/visual-editor/src/internal/puck/components/meta-title/metaTitleValidation.ts(lines 6–17). Consider exporting it from this file and reusing it to avoid drift.packages/visual-editor/src/internal/puck/components/meta-title/metaTitleValidation.test.ts (1)
32-45: Consider adding a test forundefined/ missingconstantValue.The code at
metaTitleValidation.ts:36-38returns all locales whenconstantValueis falsy or not an object, but this path isn't exercised in the tests. A quick test withconstantValue: undefinedandconstantValueEnabled: truewould strengthen coverage.Example test case
it("returns all locales when constantValue is undefined", () => { const titleField = makeTitleField({ constantValueEnabled: true, constantValue: undefined as any, }); expect(getMetaTitleMissingLocales(titleField, ["en", "fr"])).toEqual([ "en", "fr", ]); });packages/visual-editor/src/internal/puck/components/meta-title/MetaTitleField.tsx (1)
23-27:errorMessageis computed even whenmissingLocalesis empty.The
pt()call runs unconditionally but is only displayed whenmissingLocales.length > 0. Consider deferring it to avoid the unnecessary i18n lookup:Suggested change
- const errorMessage = pt( - "metaTitleMissingLocales", - "Meta title is missing for locale(s): {{locales}}", - { locales: missingLocales.join(", ") } - ); ... {missingLocales.length > 0 && ( <p className="ve-mt-2 ve-text-xs ve-text-red-600" role="alert"> - {errorMessage} + {pt( + "metaTitleMissingLocales", + "Meta title is missing for locale(s): {{locales}}", + { locales: missingLocales.join(", ") } + )} </p> )}packages/visual-editor/src/internal/puck/components/meta-title/MetaTitleValidationReporter.tsx (1)
31-34:useMemohas no effect here —missingLocalesis a new array reference each render.Since
missingLocalesis recomputed on every render (line 28), the dependency[missingLocales]always triggers recomputation, makinguseMemoa no-op. The resulting string still works correctly as a primitive dep in the effect below, so this isn't a bug — just unnecessary overhead. Consider computing the key inline.Simplification
- const missingLocalesKey = React.useMemo( - () => missingLocales.join("|"), - [missingLocales] - ); + const missingLocalesKey = missingLocales.join("|");
packages/visual-editor/src/internal/puck/components/meta-title/MetaTitleValidationReporter.tsx
Show resolved
Hide resolved
|
|
||
| return pt("fixErrorsToPublishMultiple", "To publish, {{actions}}", { | ||
| actions: actions.join("; "), | ||
| return pt("fixErrorsToPublish", "To publish, {{actions}}", { |
There was a problem hiding this comment.
This is a weird way to translate. I feel like from a UI perspective if there could be multiple actions, then "To publish:" would be its own line and then a bulleted list of separately translated actions. It seems weird to combine them with the prefix.
| const actionsText = | ||
| actions.length > 0 | ||
| ? actions.join("; ") | ||
| : pt( |
There was a problem hiding this comment.
If we do keep it like this after my below comment, then translate the full string together. You can't mash parts of sentences together because the order can be entirely different in other languages. Therefore this should translate the full string with "To publish, " included.
|
| Key | Languages Removed |
|---|---|
fixErrorsToPublish |
cs,da de,en en-GB,es et,fi fr,hr hu,it ja,lt lv,nb nl,pl pt,ro sk,sv tr,zh zh-TW |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/visual-editor/locales/platform/en-GB/visual-editor.json`:
- Line 831: Update the locale string for the key "toPublishHeading" to include
the missing comma after "publish" so it reads "To publish, resolve the following
errors:"; change this in both locale files by editing the "toPublishHeading"
entry in visual-editor.json (packages/visual-editor/locales/platform/en-GB and
packages/visual-editor/locales/platform/en) to match the existing
"resolveErrorsInEditor" punctuation.
In `@packages/visual-editor/locales/platform/en/visual-editor.json`:
- Line 831: Update the JSON value for the key "toPublishHeading" to include a
comma after "publish" to match grammar and existing style (e.g., align with
"resolveErrorsInEditor"); locate the "toPublishHeading" entry in
packages/visual-editor/locales/platform/en/visual-editor.json and change the
string to include the comma ("To publish, resolve the following errors:")
ensuring proper JSON quoting and commas around the property remain valid.
In `@packages/visual-editor/locales/platform/it/visual-editor.json`:
- Line 832: Update the "toPublishHeading" translation so it matches the style
used elsewhere: change the value from "Per pubblicare risolvere i seguenti
errori:" to the imperative form with a comma, e.g. "Per pubblicare, risolvi i
seguenti errori:" by editing the "toPublishHeading" entry in visual-editor.json.
- Around line 687-688: The two string values for the same key concept are
inconsistent: "metaTitle" uses lowercase "meta titolo" while
"metaTitleWithLocales" uses title-case "Meta Titolo"; update one of them so both
use the same capitalization (pick either "meta titolo" or "Meta Titolo") so the
published error messages under the publishError object are consistent—edit the
values for "metaTitle" and/or "metaTitleWithLocales" accordingly.
In `@packages/visual-editor/locales/platform/nl/visual-editor.json`:
- Line 610: The translation for the key "metaTitleMissingLocales" uses
"land(en)" which is inaccurate for "locales"; update the value for
metaTitleMissingLocales to use a consistent term for locales (e.g.,
"taalinstelling(en)" or the loanword "locale(s)") to match other new keys and
accurately reflect language/region settings.
packages/visual-editor/locales/platform/en-GB/visual-editor.json
Outdated
Show resolved
Hide resolved
| "metaTitle": "compila il meta titolo per tutte le impostazioni locali accedendo alle impostazioni del componente Pagina di livello superiore", | ||
| "metaTitleWithLocales": "compila il Meta Titolo per le seguenti lingue accedendo alle impostazioni del componente Pagina di livello superiore: {{locales}}" |
There was a problem hiding this comment.
Inconsistent capitalization of "meta titolo" vs "Meta Titolo".
Line 687 uses lowercase "meta titolo" while line 688 uses title-case "Meta Titolo". These should be consistent within the same publishError object.
✏️ Suggested fix
- "metaTitleWithLocales": "compila il Meta Titolo per le seguenti lingue accedendo alle impostazioni del componente Pagina di livello superiore: {{locales}}"
+ "metaTitleWithLocales": "compila il meta titolo per le seguenti lingue accedendo alle impostazioni del componente Pagina di livello superiore: {{locales}}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "metaTitle": "compila il meta titolo per tutte le impostazioni locali accedendo alle impostazioni del componente Pagina di livello superiore", | |
| "metaTitleWithLocales": "compila il Meta Titolo per le seguenti lingue accedendo alle impostazioni del componente Pagina di livello superiore: {{locales}}" | |
| "metaTitle": "compila il meta titolo per tutte le impostazioni locali accedendo alle impostazioni del componente Pagina di livello superiore", | |
| "metaTitleWithLocales": "compila il meta titolo per le seguenti lingue accedendo alle impostazioni del componente Pagina di livello superiore: {{locales}}" |
🤖 Prompt for AI Agents
In `@packages/visual-editor/locales/platform/it/visual-editor.json` around lines
687 - 688, The two string values for the same key concept are inconsistent:
"metaTitle" uses lowercase "meta titolo" while "metaTitleWithLocales" uses
title-case "Meta Titolo"; update one of them so both use the same capitalization
(pick either "meta titolo" or "Meta Titolo") so the published error messages
under the publishError object are consistent—edit the values for "metaTitle"
and/or "metaTitleWithLocales" accordingly.
| }); | ||
| return [ | ||
| pt("toPublishHeading", "To publish resolve the following errors:"), | ||
| ...actions.map((action) => `- ${action}`), |
There was a problem hiding this comment.
Do we want newlines? It would look like this currently which is weird IMO
To publish resolve the following errors: - foo - bar
There was a problem hiding this comment.
The .join adds newlines on the line below this
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/visual-editor/locales/platform/et/visual-editor.json`:
- Line 841: The key "toPublishHeading" currently ends with a period but should
end with a colon to match the English source and other locales; update the value
for "toPublishHeading" from "Avaldamiseks lahendage järgmised vead." to use a
trailing colon ("Avaldamiseks lahendage järgmised vead:") so the heading
consistently introduces the following error list.
In `@packages/visual-editor/locales/platform/fi/visual-editor.json`:
- Line 617: The translation for the key "metaTitleMissingLocales" uses
"maa-asioista", which misrepresents "locales"; update the value to a correct
Finnish term such as "lokaaleilta" or "kielialueilta" so the string becomes
something like "Metaotsikko puuttuu lokaaleilta: {{locales}}" (or use
"kielialueilta" if preferred) in the "metaTitleMissingLocales" entry.
In `@packages/visual-editor/locales/platform/ro/visual-editor.json`:
- Around line 698-702: The two Romanian strings for publishError use
inconsistent phrasing for the same component; update both keys (metaTitle and
metaTitleWithLocales) to use the same wording — choose either "componenta Pagina
de nivel superior" or "componenta de nivel superior de pagină" and apply that
exact phrase in both values so the component reference is identical across
metaTitle and metaTitleWithLocales.
- Line 621: The translation string key "metaTitleMissingLocales" uses the term
"locale(e)" which is inconsistent with other messages that use
"localități"/"localitățile"; update the value for "metaTitleMissingLocales" to
use the same term (e.g., replace "locale(e)" with "localități") so all
publish-error messages use a single consistent Romanian term across the locale
file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/locales/platform/et/visual-editor.json`:
- Line 617: The translation for metaTitleMissingLocales uses an inconsistent
Estonian word for "locales" compared to publishError (metaTitleMissingLocales vs
publishError); confirm the correct Estonian term with a native speaker and
update the string value for "metaTitleMissingLocales" (and any other
inconsistent keys around lines 693–697) so the same word form is used across
keys (e.g., align "lokaadi(de)" to the form used in "publishError" or vice
versa), keeping interpolation token {{locales}} intact.
| "map": "Kaart", | ||
| "maxWidthTip": "Sisu optimaalse joondamise tagamiseks soovitame seada päise ja jaluse laiuse, et see vastaks lehe sisuvõrgule või ületada.", | ||
| "mediaType": "Meediumitüüp", | ||
| "metaTitleMissingLocales": "Lokaadi(de) metapealkiri puudub: {{locales}}", |
There was a problem hiding this comment.
Inconsistent spelling of "locales" between metaTitleMissingLocales and publishError.
Line 617 uses "lokaadi(de)" (suggesting base form "lokaad") while lines 695–696 use "lokaatide" (suggesting base form "lokaat"). One of these spellings is likely incorrect — please verify with a native Estonian speaker and align them.
Also applies to: 693-697
🤖 Prompt for AI Agents
In `@packages/visual-editor/locales/platform/et/visual-editor.json` at line 617,
The translation for metaTitleMissingLocales uses an inconsistent Estonian word
for "locales" compared to publishError (metaTitleMissingLocales vs
publishError); confirm the correct Estonian term with a native speaker and
update the string value for "metaTitleMissingLocales" (and any other
inconsistent keys around lines 693–697) so the same word form is used across
keys (e.g., align "lokaadi(de)" to the form used in "publishError" or vice
versa), keeping interpolation token {{locales}} intact.
…itor into block-empty-title
5 participants
This ensures that meta titles are set for all of the page group's locales. If the title is not set for a locale, there is an error message below the title input, and the publish button will be disabled with a tooltip explaining how to fix it. If a publish is somehow initiated anyway with empty titles (for example, when adding a locale to an existing page group), the title will fall back to the primary locale's title, or any title it can find.
Screen.Recording.2026-02-09.at.3.38.12.PM.mov