refactor: fontSizeSelector fieldType#1153
Merged
mkilpatrick merged 19 commits intomainfrom Apr 13, 2026
Merged
Conversation
Contributor
|
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 FontSizeSelector into a typed field API: adds Sequence Diagram(s)sequenceDiagram
participant Client
participant LocatorComponent
participant isVisualEditorTestEnv
participant MapboxMap
Client->>LocatorComponent: render()
LocatorComponent->>isVisualEditorTestEnv: isVisualEditorTestEnv()
alt test env true
LocatorComponent->>Client: render LocatorTestMap (placeholder)
else test env false
LocatorComponent->>MapboxMap: initialize / load
MapboxMap-->>LocatorComponent: ready
LocatorComponent->>Client: render Mapbox map UI
end
Possibly related PRs
Suggested labels
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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/fields/FontSizeSelectorField.tsx (1)
34-45:⚠️ Potential issue | 🟠 MajorCheck both
theme.fontSizeandtheme.extend.fontSize, and handle conversion failures gracefully.The function only reads from
theme.extend.fontSize, missing projects that overridetheme.fontSizedirectly. Additionally,convertToPixels()throws on unsupported units (anything other than px, rem, em, %). Valid Tailwind/CSS values likeclamp()or CSS variables will cause runtime errors and break the field renderer instead of falling back gracefully. Read from both config locations and wrap the conversion in a try-catch to return 0 on failure.Possible fix
export const getCustomFontSize = ( tailwindConfig: TailwindConfig, value: string ): number => { - const customFontSize = tailwindConfig?.theme?.extend?.fontSize?.[value]; + const customFontSize = + tailwindConfig?.theme?.extend?.fontSize?.[value] ?? + tailwindConfig?.theme?.fontSize?.[value]; + + const toPixelsOrZero = (fontSize: string) => { + try { + return convertToPixels(fontSize); + } catch { + return 0; + } + }; + if (typeof customFontSize === "string") { - return convertToPixels(customFontSize); + return toPixelsOrZero(customFontSize); } if (Array.isArray(customFontSize) && customFontSize.length > 0) { - return convertToPixels(customFontSize[0]); + return toPixelsOrZero(customFontSize[0]); } return 0; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/FontSizeSelectorField.tsx` around lines 34 - 45, Update getCustomFontSize to check both tailwindConfig.theme.fontSize and tailwindConfig.theme.extend.fontSize for the key (falling back from extend to base), and guard the convertToPixels call with a try-catch so any conversion error returns 0 instead of throwing; specifically, in getCustomFontSize locate customFontSize by first checking theme?.fontSize?.[value] then theme?.extend?.fontSize?.[value], handle both string and array cases as before, and wrap the convertToPixels(...) invocations in try/catch to return 0 on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/visual-editor/src/fields/FontSizeSelectorField.tsx`:
- Around line 34-45: Update getCustomFontSize to check both
tailwindConfig.theme.fontSize and tailwindConfig.theme.extend.fontSize for the
key (falling back from extend to base), and guard the convertToPixels call with
a try-catch so any conversion error returns 0 instead of throwing; specifically,
in getCustomFontSize locate customFontSize by first checking
theme?.fontSize?.[value] then theme?.extend?.fontSize?.[value], handle both
string and array cases as before, and wrap the convertToPixels(...) invocations
in try/catch to return 0 on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b551bd1c-b461-4bda-bd34-8ae56a2af08a
📒 Files selected for processing (8)
packages/visual-editor/src/docs/ai/components.d.tspackages/visual-editor/src/editor/FontSizeSelector.test.tspackages/visual-editor/src/editor/index.tspackages/visual-editor/src/fields/FontSizeSelectorField.test.tsxpackages/visual-editor/src/fields/FontSizeSelectorField.tsxpackages/visual-editor/src/fields/fields.tspackages/visual-editor/src/fields/index.tspackages/visual-editor/src/index.ts
💤 Files with no reviewable changes (2)
- packages/visual-editor/src/editor/index.ts
- packages/visual-editor/src/editor/FontSizeSelector.test.ts
jwartofsky-yext
previously approved these changes
Apr 10, 2026
briantstephan
previously approved these changes
Apr 10, 2026
mkilpatrick
dismissed stale reviews from briantstephan and jwartofsky-yext
via
3fed644
Contributor
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
auto-screenshot-update: true
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/visual-editor/src/fields/FontSizeSelectorField.tsx (1)
43-49:⚠️ Potential issue | 🟡 MinorGuard tuple font-size values before pixel conversion.
Line 48 assumes
customFontSize[0]is always a string. If that tuple is misconfigured,convertToPixelscan throw at runtime. Add a type guard before conversion.Proposed fix
if (typeof customFontSize === "string") { return convertToPixels(customFontSize); } - if (Array.isArray(customFontSize) && customFontSize.length > 0) { + if ( + Array.isArray(customFontSize) && + customFontSize.length > 0 && + typeof customFontSize[0] === "string" + ) { return convertToPixels(customFontSize[0]); } return 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/FontSizeSelectorField.tsx` around lines 43 - 49, The tuple branch in FontSizeSelectorField.tsx assumes customFontSize[0] is a string and calls convertToPixels directly; add a guard that checks Array.isArray(customFontSize) && customFontSize.length > 0 && typeof customFontSize[0] === "string" before calling convertToPixels(customFontSize[0]) and handle the non-string case (e.g., skip conversion or return a safe default/undefined) so convertToPixels is never invoked with a non-string.packages/visual-editor/src/components/testing/compareScreenshot.ts (1)
37-40:⚠️ Potential issue | 🔴 CriticalReturn type mismatch breaks caller destructuring.
The baseline creation path returns
{ pass, message }but the declared interface incomponentTests.d.tsexpects{ passes: boolean; numDiffPixels: number }. The caller incomponentTests.setup.tsdestructures{ passes, numDiffPixels }, so this path will yieldundefinedfor both fields.🐛 Proposed fix
} catch (_) { // If baseline doesn't exist, save new screenshot as baseline mkdirSync(path.dirname(filePath), { recursive: true }); writeFileSync(filePath, PNG.sync.write(updatedImg)); return { - pass: false, - message: () => `Baseline screenshot created for ${filePath}`, + passes: false, + numDiffPixels: 0, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/testing/compareScreenshot.ts` around lines 37 - 40, The baseline creation branch in compareScreenshot currently returns { pass, message } which doesn't match the expected interface from componentTests.d.ts and breaks the destructuring in componentTests.setup.ts; update the return value in the baseline path of the compareScreenshot function to return the shape { passes: boolean, numDiffPixels: number } (set passes to true/false as appropriate and numDiffPixels to 0 for a newly-created baseline) so callers that destructure { passes, numDiffPixels } receive the correct fields.
🧹 Nitpick comments (3)
packages/visual-editor/src/components/pageSections/NearbyLocations/utils.ts (1)
181-196: Precompute stable sort keys once to reduce comparator overhead.
getDocStableSortKeyis currently recomputed on each comparison. You can compute it once during.map(...)and reuse it in.sort(...).♻️ Suggested refactor
const nearestDocs = allDocs .map((doc) => { const coordinate = getDocCoordinate(doc); return { doc, distance: coordinate ? getDistance(origin, coordinate) : Infinity, + stableSortKey: getDocStableSortKey(doc), }; }) .sort((a, b) => { if (a.distance !== b.distance) { return a.distance - b.distance; } - return getDocStableSortKey(a.doc).localeCompare( - getDocStableSortKey(b.doc) - ); + return a.stableSortKey.localeCompare(b.stableSortKey); })Also applies to: 228-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/NearbyLocations/utils.ts` around lines 181 - 196, The comparator calls getDocStableSortKey repeatedly; compute the stable key once in the .map step by adding a stableKey property (e.g. compute getDocStableSortKey(doc) alongside doc and distance) and then update the .sort to compare using that precomputed stableKey instead of calling getDocStableSortKey again; apply the same change to the other occurrence referenced (lines ~228-230) so both sorts use the cached stableKey.packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
89-118: Stability helpers improve screenshot consistency.The two-frame wait per iteration and font readiness check are appropriate for reducing flakiness. However, the loop exits silently after 5 attempts even if metrics never stabilized, which could mask timing issues during debugging.
💡 Optional: Log warning when stability check exhausts attempts
for (let attempt = 0; attempt < 5; attempt++) { await waitForNextFrame(); await waitForNextFrame(); const metrics = [ document.body.scrollWidth, document.body.scrollHeight, document.body.offsetWidth, document.body.offsetHeight, ].join(":"); if (metrics === previousMetrics) { return; } previousMetrics = metrics; } + + console.warn( + "waitForStableScreenshotFrame: layout did not stabilize after 5 attempts" + ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/testing/componentTests.setup.ts` around lines 89 - 118, The waitForStableScreenshotFrame helper silently returns after 5 unsuccessful attempts to stabilize metrics, which can hide timing problems; update waitForStableScreenshotFrame to emit a warning (e.g., using console.warn or the test logger) when the loop exhausts attempts without metrics stabilizing, include identifying context (function name waitForStableScreenshotFrame, the metrics string and number of attempts), and retain the existing font readiness and two-frame wait via waitForNextFrame so behavior is unchanged except for the new warning when stability is not reached.packages/visual-editor/src/components/contentBlocks/MapboxStaticMap.tsx (1)
75-114: Consider extracting shared test helpers.
isVisualEditorTestEnv()andgetStaticMapPreviewBackground()are duplicated inLocator.tsx(asisVisualEditorTestEnvandgetLocatorTestMapBackground). TheformatMapStyleLabelimplementations differ slightly (this version doesn't handle themapbox://styles/mapbox/prefix that Locator's version strips with.split("/").pop()).Extracting these to a shared utility would reduce duplication and ensure consistent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/contentBlocks/MapboxStaticMap.tsx` around lines 75 - 114, The two helpers isVisualEditorTestEnv and getStaticMapPreviewBackground and the similar helpers in Locator.tsx (isVisualEditorTestEnv, getLocatorTestMapBackground) plus the slightly different formatMapStyleLabel should be extracted into a shared utility module; create a single exported util (e.g., formatMapStyleLabel, isVisualEditorTestEnv, getStaticMapPreviewBackground) that also normalizes Mapbox style strings by stripping a "mapbox://styles/.../" prefix (use .split("/").pop()) before applying the existing regex/replace logic, move the implementations there, and update MapboxStaticMap.tsx and Locator.tsx to import and use these shared functions so both files share identical behavior and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 1102-1122: Remove the unnecessary nullish coalescing using
result.id when generating React keys inside the results.slice map in
Locator.tsx: always use the deterministic `${result.index}-${index}` key
(referencing the map over results.slice(0, TEST_MAP_PIN_POSITIONS.length) and
the MapPinIcon render) so replace the key expression to consistently use
result.index and index, ensuring stable, unique keys for reconciliation.
---
Outside diff comments:
In `@packages/visual-editor/src/components/testing/compareScreenshot.ts`:
- Around line 37-40: The baseline creation branch in compareScreenshot currently
returns { pass, message } which doesn't match the expected interface from
componentTests.d.ts and breaks the destructuring in componentTests.setup.ts;
update the return value in the baseline path of the compareScreenshot function
to return the shape { passes: boolean, numDiffPixels: number } (set passes to
true/false as appropriate and numDiffPixels to 0 for a newly-created baseline)
so callers that destructure { passes, numDiffPixels } receive the correct
fields.
In `@packages/visual-editor/src/fields/FontSizeSelectorField.tsx`:
- Around line 43-49: The tuple branch in FontSizeSelectorField.tsx assumes
customFontSize[0] is a string and calls convertToPixels directly; add a guard
that checks Array.isArray(customFontSize) && customFontSize.length > 0 && typeof
customFontSize[0] === "string" before calling convertToPixels(customFontSize[0])
and handle the non-string case (e.g., skip conversion or return a safe
default/undefined) so convertToPixels is never invoked with a non-string.
---
Nitpick comments:
In `@packages/visual-editor/src/components/contentBlocks/MapboxStaticMap.tsx`:
- Around line 75-114: The two helpers isVisualEditorTestEnv and
getStaticMapPreviewBackground and the similar helpers in Locator.tsx
(isVisualEditorTestEnv, getLocatorTestMapBackground) plus the slightly different
formatMapStyleLabel should be extracted into a shared utility module; create a
single exported util (e.g., formatMapStyleLabel, isVisualEditorTestEnv,
getStaticMapPreviewBackground) that also normalizes Mapbox style strings by
stripping a "mapbox://styles/.../" prefix (use .split("/").pop()) before
applying the existing regex/replace logic, move the implementations there, and
update MapboxStaticMap.tsx and Locator.tsx to import and use these shared
functions so both files share identical behavior and avoid duplication.
In `@packages/visual-editor/src/components/pageSections/NearbyLocations/utils.ts`:
- Around line 181-196: The comparator calls getDocStableSortKey repeatedly;
compute the stable key once in the .map step by adding a stableKey property
(e.g. compute getDocStableSortKey(doc) alongside doc and distance) and then
update the .sort to compare using that precomputed stableKey instead of calling
getDocStableSortKey again; apply the same change to the other occurrence
referenced (lines ~228-230) so both sorts use the cached stableKey.
In `@packages/visual-editor/src/components/testing/componentTests.setup.ts`:
- Around line 89-118: The waitForStableScreenshotFrame helper silently returns
after 5 unsuccessful attempts to stabilize metrics, which can hide timing
problems; update waitForStableScreenshotFrame to emit a warning (e.g., using
console.warn or the test logger) when the loop exhausts attempts without metrics
stabilizing, include identifying context (function name
waitForStableScreenshotFrame, the metrics string and number of attempts), and
retain the existing font readiness and two-frame wait via waitForNextFrame so
behavior is unchanged except for the new warning when stability is not reached.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ecf17bd6-f9bd-49a6-8b3e-25b7cfcdf1e3
⛔ Files ignored due to path filters (39)
packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[desktop] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[desktop] version 10 props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[mobile] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[mobile] version 10 props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[tablet] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[tablet] version 10 props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (11)
.github/workflows/unit_test.ymlpackages/visual-editor/src/components/Locator.test.tsxpackages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/contentBlocks/MapboxStaticMap.tsxpackages/visual-editor/src/components/pageSections/NearbyLocations/utils.test.tspackages/visual-editor/src/components/pageSections/NearbyLocations/utils.tspackages/visual-editor/src/components/pageSections/StaticMapSection.test.tsxpackages/visual-editor/src/components/testing/compareScreenshot.tspackages/visual-editor/src/components/testing/componentTests.csspackages/visual-editor/src/components/testing/componentTests.setup.tspackages/visual-editor/src/fields/FontSizeSelectorField.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/visual-editor/src/components/testing/componentTests.css
briantstephan
previously approved these changes
Apr 10, 2026
benlife5
reviewed
Apr 10, 2026
asanehisa
reviewed
Apr 13, 2026
auto-screenshot-update: true
auto-screenshot-update: true
asanehisa
previously approved these changes
Apr 13, 2026
benlife5
reviewed
Apr 13, 2026
Contributor
jwartofsky-yext
left a comment
jwartofsky-yext
left a comment
There was a problem hiding this comment.
looks good, but it seems like this should be two PRs:
-
FontSizeSelector Field Type
-
Screenshot Refactor
auto-screenshot-update: true
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/visual-editor/src/components/Locator.test.tsx`:
- Around line 104-109: The test fixture's headshot object uses url: "" which
only exercises the empty/broken-image path; update the shared locator fixture
and the static-image test case (the headshot field used in Locator.test.tsx) to
set a tiny data URL (e.g., a 1x1 PNG/SVG data URI) instead of an empty string so
the image rendering path is exercised without network access; locate the
fixtures referenced by the test (the headshot object in the static-image case
and the shared locator fixture) and replace url: "" with the small data URI in
both places.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6767c6a0-5729-43f3-ac55-302ef818d273
⛔ Files ignored due to path filters (24)
packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (2)
packages/visual-editor/src/components/Locator.test.tsxpackages/visual-editor/src/components/Locator.tsx
auto-screenshot-update: true
benlife5
approved these changes
Apr 13, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/Locator.test.tsx (1)
270-273: Derive fixture mode from the locator config, not raw truthiness.Any non-empty
__.locatorSourcePageSetsvalue selects the multi-pageset fixture here, so a malformed document can silently take the happy path instead of matching the component’s real branching. Keying this off parsed_pageset.typeConfig.locatorConfig.sourcesvs.sourcewould keep the helper aligned with production behavior.Based on learnings,
__.locatorSourcePageSetsand__.sourceEntityPageSetTemplateare mutually exclusive, and if both are present the correct behavior is to treat that as an error state rather than a usable multi-pageset config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/Locator.test.tsx` around lines 270 - 273, Update getLocatorFixtureResults to determine fixture mode from the parsed locator config rather than raw truthiness: inspect document._pageset?.typeConfig?.locatorConfig?.sources and whether that represents a multi-pageset (vs a single source) to choose between MULTI_PAGESET_LOCATOR_RESULTS and DEFAULT_LOCATOR_RESULTS, and treat the presence of both document.__?.locatorSourcePageSets and document.__?.sourceEntityPageSetTemplate as an error state (do not silently return the multi-pageset fixture). Ensure you reference getLocatorFixtureResults, MULTI_PAGESET_LOCATOR_RESULTS, DEFAULT_LOCATOR_RESULTS, document._pageset.typeConfig.locatorConfig.sources, and document.__.sourceEntityPageSetTemplate when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/visual-editor/src/components/Locator.test.tsx`:
- Around line 270-273: Update getLocatorFixtureResults to determine fixture mode
from the parsed locator config rather than raw truthiness: inspect
document._pageset?.typeConfig?.locatorConfig?.sources and whether that
represents a multi-pageset (vs a single source) to choose between
MULTI_PAGESET_LOCATOR_RESULTS and DEFAULT_LOCATOR_RESULTS, and treat the
presence of both document.__?.locatorSourcePageSets and
document.__?.sourceEntityPageSetTemplate as an error state (do not silently
return the multi-pageset fixture). Ensure you reference
getLocatorFixtureResults, MULTI_PAGESET_LOCATOR_RESULTS,
DEFAULT_LOCATOR_RESULTS, document._pageset.typeConfig.locatorConfig.sources, and
document.__.sourceEntityPageSetTemplate when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f86b9300-3ee7-46b3-9113-30f904ecf4f5
⛔ Files ignored due to path filters (32)
packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 24 with filters (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 24 with filters.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom heading with site color 2.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom heading.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 60 custom title and result cards with site color 3.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static headings (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static headings.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 64 static image.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/Locator.test.tsx
jwartofsky-yext
approved these changes
Apr 13, 2026
Contributor
jwartofsky-yext
left a comment
jwartofsky-yext
left a comment
There was a problem hiding this comment.
could be broken down more, but otherwise lgtm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes fontSizeSelector a real fieldType.
Also refactors some tests so that they are less flaky/cause infinite loops.