fix: Restore Chinese language selection for Brand Markets | LLMO-7309 - #3183
fix: Restore Chinese language selection for Brand Markets | LLMO-7309#3183igor-grubic wants to merge 3 commits into
Conversation
…h catalog row Semrush lists Chinese only under a script/region-qualified name (e.g. "Chinese (Simplified)"), never a bare "Chinese", so resolveLanguageId's exact English-name match failed for the app's primary-subtag code `zh` — the Add-market flow could not resolve a language_id for Chinese (LLMO-7309). Register a parenthetical-stripped base-name alias per catalog row (exact name still wins; rows sorted so `zh` resolves deterministically to the alphabetically-first qualified variant, i.e. Simplified). Adds unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hey @igor-grubic,
Verdict: Approve - clean, well-scoped bug fix with good test coverage.
Complexity: TRIVIAL - small targeted fix in one handler, two files.
Changes: Adds base-name aliasing to the Semrush language cache so unqualified language codes (e.g. zh) resolve against qualified catalog rows (e.g. Chinese (Simplified)) (2 files).
Non-blocking (3): minor issues and suggestions
- nit: Orphaned comment fragment starting mid-sentence ("tags. The handler guards...") above the
400s when the language tag is not a real languagetest. The new tests were inserted between the first line and the rest of a three-line comment block, leaving a dangling fragment. Restore the full comment or rewrite it as a single coherent block. -test/support/serenity/handlers/markets.test.js:671 - suggestion: Log at
log.debugwhen a base-name alias is registered (e.g."chinese" aliased from "chinese (simplified)") - would help diagnose future catalog changes without adding runtime noise, since the cache rebuilds once per TTL window. -src/support/serenity/handlers/markets.js:120 - suggestion: Pin the locale on
localeCompare(e.g.localeCompare(b, 'en')) or use a plain codepoint sort to make the determinism guarantee independent of the Lambda runtime's ambient locale. Practical risk is negligible for ASCII names, but pinning makes the intent explicit. -src/support/serenity/handlers/markets.js:112
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 37s | Cost: $4.48 | Commit: 593b83365583f2b08215627d9ef19e2f27d30f82
If this code review was useful, please react with 👍. Otherwise, react with 👎.
… parenthetical The live GET /serenity/languages catalog was captured against the Add-market flow and returns "Chinese Simplified" / "Chinese Traditional" — a trailing space-separated script word, NOT the parenthetical "Chinese (Simplified)" the first pass assumed. baseLanguageName only stripped a trailing "(...)", so the `zh` alias never matched and resolveLanguageId still failed for Chinese. Also strip a trailing space-separated Simplified/Traditional qualifier (the parenthetical strip is kept for robustness). Tests updated to the real catalog format, plus a parenthetical-form case. Verified live: with this, `zh` resolves to "Chinese Simplified". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This PR will trigger a minor release when merged. |
Semrush tracks Simplified and Traditional Chinese as separate catalogs
("Chinese Simplified" / "Chinese Traditional"). Rather than collapse them to a
single `zh` and deterministically pick Simplified, resolve each script variant
to its own language_id.
resolveLanguageId now matches on a word-order- and punctuation-independent
normalized key (the app's Intl name is "Simplified Chinese" vs the catalog's
"Chinese Simplified"), and isoToEnglishName preserves the script subtag. A
token-subset fallback keeps a legacy bare `zh` market resolving (to Simplified),
so existing data is unaffected. Tests cover zh-Hans, zh-Hant, legacy zh, a
parenthetical catalog form, and exact-Chinese precedence.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Hey @igor-grubic,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - well-scoped bug fix with solid normalization approach and thorough test coverage.
Complexity: HIGH - medium diff; API surface signal.
Changes: Adds word-order-independent language name normalization and script-variant-aware BCP-47 resolution so Chinese markets resolve against qualified Semrush catalog rows (2 files).
Non-blocking (4): minor issues and suggestions
- nit: Orphaned comment fragment starting mid-sentence ("tags. The handler guards...") above the
400s when the language tag is not a real languagetest. The new tests were inserted between lines of a multi-line comment block, leaving a dangling fragment. Restore the full sentence or rewrite as a standalone comment. -test/support/serenity/handlers/markets.test.js - suggestion: Pin the locale on
localeCompare(e.g..localeCompare(b, 'en')) so the sort order is deterministic regardless of the Lambda runtime's ambientLANG/LC_ALL. Practical risk is negligible for ASCII names, but pinning makes the intent explicit. -src/support/serenity/handlers/markets.js:155 - suggestion: Add a
queryTokens.length < tokens.sizeguard on the token-subset fallback so it only fires when the query looks like a base form (fewer tokens than the candidate). This prevents same-token-count false positives if the catalog evolves with entries that happen to share tokens. -src/support/serenity/handlers/markets.js:196 - suggestion: Document the ASCII-English assumption on
normalizeLanguageName- the/[^a-z0-9]+/regex strips non-ASCII characters silently. A@paramnote like "must be an ASCII-English name" prevents a future contributor from assuming it handles accented or CJK names. -src/support/serenity/handlers/markets.js:90
Previously flagged, now resolved
- Base-name alias logging suggestion is moot -
baseLanguageNamereplaced entirely by the normalized approach.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 5m 35s | Cost: $5.87 | Commit: 915daf8c209e4d4e0c8cceba2d03d6bff490a86b
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…sh name (#3221) <!-- mysticat-pr-skill --> ## 1. Abstract Resolves Brand Markets `languageCode` to a Semrush language id by exact BCP-47 `code` match instead of English-name matching, and surfaces `code` from the org-level language catalog endpoint. ## 2. Reasoning `resolveLanguageId` converted a market's BCP-47 `languageCode` to an English display name via `Intl.DisplayNames` and matched it against Semrush's catalog `name` — brittle by construction: Semrush spells "Chinese Simplified" where `Intl.DisplayNames` says "Simplified Chinese" (word-order collision), aliases like Filipino/Tagalog exist, and any future catalog rename silently drops a language. This is the root cause behind LLMO-7309 (Chinese language selection broke, tracked in the still-open, not-yet-merged #3183). Semrush's language catalog now returns a stable BCP-47 `code` per entry (LLMO-7420, confirmed live 2026-09-07); this PR makes that `code` the resolution key end-to-end and deletes the English-name matching entirely. ## 3. High-level overview of the changes - `resolveLanguageId` (`src/support/serenity/handlers/markets.js`) no longer converts `languageCode` to an English name via `Intl.DisplayNames` and matches by name. It now caches the language catalog keyed by lowercased `code` and looks up the (already-normalized) `languageCode` directly. An unresolved code still 400s `unknownLanguage` — no fallback heuristics, so a new or renamed Semrush language needs no Adobe-side mapping change. - `listLanguageCatalog` now includes `code` in each returned item (previously only `id`/`name`) and drops rows missing either `name` or `code`. - `SerenityLanguage` OpenAPI schema (`docs/openapi/schemas.yaml`) gained a required `code` field; the `GET /serenity/languages` route description (`docs/openapi/serenity-api.yaml`) no longer describes `name` as "the resolution key" — `code` is. - No change to the `languageCode` input validation (`normalizeLanguageCode`) or to the slice-key shape. ## 4. Required information - Jira / issue: [LLMO-7420](https://jira.corp.adobe.com/browse/LLMO-7420) - Other: companion PRs — adobe/spacecat-shared #1919 (adds the `code` field this PR consumes), adobe/project-elmo-ui `LLMO-7420-semrush-bcp47-language-source-of-truth` (builds the language picker from `code`) ## 6. Affected / used mysticat-workspace projects - spacecat-shared: `@adobe/spacecat-shared-project-engine-client`'s `listLanguages()` must return `code` per entry for this resolver to work — depends on PR #1919 publishing before this can be deployed against live Semrush (verified locally via `npm link` to the unpublished branch; the dependency version in `package.json` is intentionally not bumped yet). - project-elmo-ui: consumes `code` from `GET /v2/orgs/{orgId}/serenity/languages` to build the Brand Markets language picker. ## 8. Test plan (a) Ran the full unit suite locally, including the rewritten Chinese-script resolution cases (`zh-Hans`/`zh-Hant`/`fil` resolving by exact `code`, no name-matching fallback) and the `listLanguageCatalog` catalog-shape tests, plus `npm run type-check` and `npm run docs:lint` against the updated OpenAPI schema. (b) Dev/stage: after bumping `@adobe/spacecat-shared-project-engine-client` to the published version carrying `code`, exercise `POST /serenity/markets` for a Hong Kong brand with `languageCode: zh-Hans` and confirm it resolves to the Chinese Simplified project (previously broken per LLMO-7309); confirm `GET /v2/orgs/{orgId}/serenity/languages` returns `code` per entry. ## 9. Deployment & merge order - Depends on: adobe/spacecat-shared #1919 must merge and publish before this PR is deployed (it can merge before publish, but the live Semrush catalog resolution won't work in a deployed environment until the dependency is bumped to the published version in a follow-up commit). - Related: adobe/project-elmo-ui `LLMO-7420-semrush-bcp47-language-source-of-truth` (consumes this PR's `code` field from the languages endpoint). - Sequence: merge/publish spacecat-shared #1919 → bump the dependency here and deploy → merge/deploy the project-elmo-ui companion PR. 🤖 Generated with [review-kit](https://github.com/adobe/experience-success-skills) --------- Co-authored-by: Alicia Adriani <aadriani+adobe@adobe.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
## [1.799.3](v1.799.2...v1.799.3) (2026-09-09) ### Bug Fixes * **serenity:** resolve languageCode by Semrush BCP-47 code, not English name ([#3221](#3221)) ([23aef8c](23aef8c)), closes [#3183](#3183) [Hi#level](https://github.com/Hi/issues/level) [#1919](#1919) [#1919](#1919) [#1919](#1919) [#1919](#1919)
1. Abstract
This restores Chinese as a selectable language for Brand Markets add-market flows by tolerating Semrush catalog names that qualify Chinese with a parenthetical script or variant.
2. Reasoning
Chinese markets are valid in the product, but the Hong Kong Add market dialog no longer offered Chinese when filtering languages. The option source compared local language names to the Semrush catalog by exact English name, which dropped the Chinese option when Semrush names Chinese as a qualified variant such as Chinese (Simplified).
3. High-level overview of the changes
Before this change, both the UI picker and the API language-id resolver required exact catalog-name matches, so an unqualified local Chinese option could be filtered out or fail to resolve. After this change:
4. Required information
6. Affected / used mysticat-workspace projects
8. Test plan
Ch, and confirm Chinese is selectable.9. Deployment & merge order
🤖 Generated with review-kit