fix[smartling][utils]: ENG-13890 exclude URL, image and link fields from translation jobs - #4852
Conversation
|
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Code Review Summary
PR #4852 updates the Smartling extraction helpers so bare route URLs, HTTP(S) URLs, and Builder CDN asset references are not emitted as translation units, while prose containing URLs remains translatable. It also attempts to seed skipped values into the target locale so links and assets do not disappear when the SDK cannot fall back to Default. The focused regression tests cover scalar URLs, nested carousel values, symbol inputs, prose, pending jobs, and existing target values.
This is a standard-risk shared utility change: the approach is directionally sound, but the new fallback behavior does not cover all nested payload shapes and has source-locale consistency gaps. Key findings:
- 🟡 MEDIUM URL-only nested object/array payloads can disappear because only scalar defaults are restored.
- 🟡 MEDIUM Nested URL-only localized leaves can be mistaken for pending translatable content, suppressing source seeding.
- 🟡 MEDIUM Skipped values are restored from
Defaultrather than the explicitly selected source locale. - 🟡 MEDIUM Common non-HTTP link forms such as
mailto:,tel:,sms:, and fragment links remain translatable.
The added tests are useful, especially the whitespace distinction and pending-job assertions, but they do not exercise URL-only nested payloads or non-Default source locales.
🧪 Browser testing: Skipped — PR only modifies backend/shared translation utility code and tests, with no direct user-facing UI changes.
| } else { | ||
| // No direct translation - check if Default value contains nested LocalizedValues | ||
| const defaultValue = value?.Default; | ||
| // Restore a leaf the extractor skipped: the SDK resolves a missing locale to |
There was a problem hiding this comment.
🟡 Seed URL-only nested payloads into the target locale
The URL filter can remove every leaf from a localized object or array, but this restore path only handles a scalar Default string. For URL-only nested metadata or Symbol payloads, recursion into Default never creates the outer [locale] branch, so the SDK resolves the whole value to undefined in the translated locale and the content disappears. Seed the source payload when there are no actual translatable leaves, while preserving the pending-job behavior for payloads that do contain translatable leaves.
| if (!compoundKeys.length) { | ||
| // Everything came back excluded, or there was never anything to send. Both differ | ||
| // from the provider not having answered yet, which must leave the locale alone. | ||
| if (matchingKeys.length || (excludedPaths && !hasTranslatableLeaf())) { | ||
| if (matchingKeys.length || !hasTranslatableLeaf()) { |
There was a problem hiding this comment.
🟡 Distinguish skipped URL leaves from pending translations
extractLocalizedLeaves returns a nonzero count for a localized leaf even when its string was filtered by isNonTranslatableValue, so hasTranslatableLeaf() can return true for a payload containing only nested URLs/assets. That skips seedSourceIntoLocale() and leaves the target locale undefined. The check needs to distinguish marked-but-skipped leaves from actual translation units.
|
View your CI Pipeline Execution ↗ for commit 02c227a
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| // A bare URL or asset reference is a routing/config value, never a translation unit. | ||
| // Whitespace means prose ("Visit https://x.com for more"), which stays translatable. | ||
| function isNonTranslatableValue(value: string) { |
There was a problem hiding this comment.
does this mean url is not translatable, but a text containing an url is?
Couldn't it be solved using a new URL()?
There was a problem hiding this comment.
does this mean url is not translatable, but a text containing an url is? -> that's correct
I did explore new URL() but SumUp also uses relative paths like /en-gb/pricing, ./checkout, cdn.builder.io/api/v1/image/x and just #, so, new URL() doesn't really cover these
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Incremental Code Review Summary
The latest commit adds this.block() after handling a localized model payload, preventing traversal from descending into the original Default subtree and mutating nested source values. I verified that this addresses the previously reported Default-payload mutation issue and resolved that review thread. The URL predicate and model seeding changes remain otherwise consistent with the prior review.
No new actionable issues were identified in this increment. The previously reported nested Symbol seeding and skipped-leaf/pending-translation issues remain unresolved and were intentionally not reposted.
This is a standard-risk shared translation utility change.
🧪 Browser testing: Skipped — the PR modifies shared translation utilities and tests, with no direct user-facing UI changes.
2 participants
Description
Image links, button URLs and page links from Builder content were all showing up as strings for Smartling to translate.
Root Cause:
This is a regression from #4651 (June). That PR taught the extractor to look inside a localized list or object and send each string it finds as its own translation unit, which was the right fix for text fields that were previously being skipped.
The catch is that the extractor has no idea what type a field is. So once it starts walking a payload, it picks up every string in there.
imageUrl,linkandscreenImageare strings, so off they went.We already patched one version of this in #4826, where the same walk was sending dropdown values like "White" and "Left". That fix relied on the editor recording which fields are non-translatable, which only helps for localized list fields on blocks that carry that metadata. URLs on symbols and on model fields go through different code paths and were never covered.
Fix:
Added one small check: a string is not translatable if it has no whitespace and starts with /, http://, https:// or cdn.builder.io/. That check now runs everywhere a string becomes a translation unit (7 places: symbol inputs, model fields, the two nested-string branches, the localized-leaf branch, and the block flat-string case).
The whitespace part matters. "Visit https://sumup.com for more" is a real sentence and still gets translated. Only a bare URL on its own gets skipped.
Skipping a field means Smartling never returns a value for it, and the SDK resolves a missing locale to undefined rather than falling back to Default, so the link would just vanish on the translated page. To prevent that, the source URL is now copied into the target locale for any field we skip. Three changes for that: the block path already had this logic behind a condition that was too narrow, so the condition was widened; the symbol and model-field paths had none, so each got a small block.
Both new blocks are gated on the same URL check, so they only ever restore a value we deliberately skipped. A field that's still waiting on a pending job is left alone, so English never leaks into a translated locale.
Link to JIRA ticket (if applicable):
https://builder-io.atlassian.net/browse/ENG-13890
Screenshot/Clip
https://clips.agent-native.com/r/uFQLIO0vOcIM