MWPW-192450: Fix - Lingo placeholders in Studio card preview#760
MWPW-192450: Fix - Lingo placeholders in Studio card preview#760
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
|
seanchoi-dev
requested review from
Axelcureno,
npeltier and
yesil
and removed request for
yesil
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #760 +/- ##
==========================================
- Coverage 87.41% 87.33% -0.09%
==========================================
Files 210 210
Lines 62825 62870 +45
==========================================
- Hits 54920 54907 -13
- Misses 7905 7963 +58
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Axelcureno
left a comment
Axelcureno
left a comment
There was a problem hiding this comment.
Review Summary
The architectural approach is sound — switching loadPreviewPlaceholders from this.filters.value.locale to Store.localeOrRegion() fixes the root cause (dictionary fetched/cached with the wrong locale for variations), and the "set region before load" reordering eliminates the unnecessary double-fetch. Both code paths (#initializeFromCachedStore and #initializeFromRepository) now handle variation locale detection consistently.
What works well
- Fix is at the correct layer (repository/source, not consumer)
- Cache key now uses the effective locale — this was the actual root cause
- Region override is set before
loadPreviewPlaceholders(), not after — cleaner and faster - Remaining
this.filters.value.localeuses inmas-repository.jsare correctly left unchanged (DAM paths, fragment search, placeholder creation) - All 11 CI checks green
Requesting changes: Test coverage gaps
The PR's core behavioral change — switching the locale source in loadPreviewPlaceholders — has insufficient test coverage. See inline comments for specifics.
Three tests needed:
-
Repository-level unit test for
loadPreviewPlaceholdersverifying it usesStore.localeOrRegion()for cache key and fetch context (the foundational change in this PR has zero direct unit tests — all existing repo tests stub the method entirely) -
Cached store path test for
#initializeFromCachedStorewhen variation locale differs (the 4 new lines atmas-fragment-editor.js:703-708— this is what Codecov flagged) -
Same-locale no-op test verifying that when
fragmentLocale === Store.localeOrRegion(), no extra placeholder reload occurs (regression guard)
| if (!this.search.value.path) return; | ||
|
|
||
| const cacheKey = `${this.filters.value.locale}_${this.search.value.path}`; | ||
| const cacheKey = `${Store.localeOrRegion()}_${this.search.value.path}`; |
There was a problem hiding this comment.
Core change untested at the repository level
This is the foundational fix — switching from this.filters.value.locale to Store.localeOrRegion(). But every existing test in mas-repository.test.js stubs loadPreviewPlaceholders entirely, so this change (and the matching ones at lines 674, 677, 710) has zero direct unit test coverage.
A focused test could verify:
it('uses Store.localeOrRegion() for dictionary cache key and fetch', async () => {
// Set region override to simulate variation navigation
Store.search.set((prev) => ({ ...prev, region: 'fr_FR' }));
Store.filters.value = { locale: 'en_US' };
// ... setup mocks for getDictionary ...
await repository.loadPreviewPlaceholders();
// Verify getDictionary was called with locale: 'fr_FR' (not 'en_US')
// Verify cache key is 'fr_FR_sandbox' (not 'en_US_sandbox')
});Without this, if someone reverts one of the 4 call sites but not the others, no test catches the inconsistency.
| await this.#attachParentToCachedVariation(existingStore, fragmentPath); | ||
| const fragmentLocale = extractLocaleFromPath(fragmentPath); | ||
| if (fragmentLocale && fragmentLocale !== Store.localeOrRegion()) { | ||
| Store.search.set((prev) => ({ ...prev, region: fragmentLocale })); |
There was a problem hiding this comment.
New cached-store locale reload path has no test coverage
These 6 new lines add locale-specific placeholder loading to #initializeFromCachedStore — this is the path Codecov flagged (4 missing lines, 71.42% patch coverage on this file).
The existing test at mas-fragment-editor.test.js:358 only exercises #initializeFromRepository (non-cached). A test for this path would look like:
it('reloads locale placeholders for cached variations when locale differs', async () => {
const fragmentData = createFragmentData({ id: 'cached-var', locale: 'fr_FR', slug: 'var' });
const fragment = new Fragment(fragmentData);
const sourceStore = generateFragmentStore(fragment);
Store.fragments.list.data.set([sourceStore]);
Store.filters.value = { locale: 'en_US' };
el.editorContextStore.isVariation.returns(true);
sandbox.stub(el, 'resolveVariationParentFragment').resolves(null);
Store.fragmentEditor.fragmentId.value = 'cached-var';
await el.initFragment();
expect(mockRepo.loadPreviewPlaceholders.calledOnce).to.be.true;
expect(Store.search.get().region).to.equal('fr_FR');
});|
|
||
| expect(mockRepo.loadPreviewPlaceholders.callCount).to.equal(2); | ||
| expect(resolvePreviewSpy.calledOnce).to.be.true; | ||
| expect(mockRepo.loadPreviewPlaceholders.calledOnce).to.be.true; |
There was a problem hiding this comment.
Weakened assertion — preview resolution no longer verified
The old test verified both loadPreviewPlaceholders call count AND resolvePreviewFragment being called. The new test only checks calledOnce and region.
In the new flow, preview resolution happens implicitly via PreviewFragmentStore's constructor subscription to Store.placeholders.preview. This works, but is no longer tested for the variation-locale-differs path.
Consider adding a lightweight assertion that Store.placeholders.preview was set (which confirms the dictionary loaded), even if you don't spy on resolvePreviewFragment directly.
| url: 'https://odinpreview.corp.adobe.com/adobe/sites/cf/fragments', | ||
| }, | ||
| locale: this.filters.value.locale, | ||
| locale: Store.localeOrRegion(), |
There was a problem hiding this comment.
Suggestion: Capture locale once to avoid mid-execution drift
Store.localeOrRegion() is called 3 times during a single loadPreviewPlaceholders execution (lines 649, 674, 677) plus once in fetchDictionary (line 710). If an async operation modifies Store.search.region between these calls, the cache key check at line 674 could mismatch.
Not a blocker (in practice these are sequential), but a defensive improvement:
async loadPreviewPlaceholders() {
if (!this.search.value.path) return;
const locale = Store.localeOrRegion();
const cacheKey = `${locale}_${this.search.value.path}`;
// ... use `locale` throughout instead of re-calling Store.localeOrRegion()
}
There was a problem hiding this comment.
Sequential placeholder loading introduces a latency cost on every fragment open
The sequential load introduced in this PR (get fragment → set region → load placeholders) solves the wrong-locale bug but adds ~200ms to every fragment open, not just cross-locale variations.
Root cause of the constraint: you need fragment.path before you know which locale to use for the variation, so the placeholder fetch cannot start until getById resolves. That dependency is real but only applies to the variation locale — the surface locale is already known before getById is called.
Proposed approach: locale-keyed dictionary map
Instead of a single Store.placeholders.preview slot, maintain a map of dictionaries keyed by locale. The load side writes into the correct slot when a fetch completes. The render side reads whichever slot matches the current active locale, reactively — no coordination, no abort, no explicit resolvePreviewFragment call needed.
Loading: fire for the surface locale immediately on fragment open. After getById resolves and the variation locale is known, fire for that locale too. Both calls are fire-and-forget. If a locale was already cached from a previous visit, it resolves instantly.
Rendering: the preview consumer reads previewByLocale[currentLocale]. When the correct locale slot populates, the reactive system triggers the re-render automatically.
No abort needed: if the user navigates away before a fetch completes, the result lands in its slot harmlessly. The cache makes the next visit to that locale instant. You trade a small amount of unnecessary network completion for zero coordination code.
Timeline
getById() ░░░░░░░░░░░░░░░░░░░░▓
placeholders[en_US] ░░░░░░░░░░░░▓ → preview renders immediately (surface locale)
placeholders[fr_FR] ░░░░░░░░░░▓ → preview re-renders with correct locale
Compare to the current PR:
getById() ░░░░░░░░░░░░░░░░░░░░▓
│
placeholders[fr_FR] ░░░░░░░░░░▓ → first render, correct but delayed
And the regression vs main for non-variation fragments (the common case):
// main branch
getById() ░░░░░░░░░░░░░░░░░░░░▓
placeholders[en_US] ░░░░░░░░░░░░▓ → already done by the time getById lands
// this PR
getById() ░░░░░░░░░░░░░░░░░░░░▓
placeholders[en_US] ░░░░░░░░░░▓ → ~200ms added on every open
# Conflicts: # studio/src/mas-fragment-editor.js
# Conflicts: # studio/src/mas-repository.js
7 participants
Resolves https://jira.corp.adobe.com/browse/MWPW-192450
QA Checklist: https://wiki.corp.adobe.com/display/adobedotcom/M@S+Engineering+QA+Use+Cases
Please do the steps below before submitting your PR for a code review or QA
🧪 Nala E2E Tests
Nala tests run automatically when you open this PR.
To run Nala tests again:
run nalalabel to this PR (in the right sidebar)To stop automatic Nala tests:
run nalalabelTest URLs:
More Test URL: