Conversation
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. |
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR makes three changes: (1) updates packages/visual-editor/src/components/Locator.tsx to import Button from the local atoms module and removes the Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Resolver as legacyResolveUrlTemplateWithTemplates
participant Templates as UrlTemplates
participant Doc as StreamDocument
Caller->>Resolver: resolve(streamDocument, relativePrefixToRoot, Templates)
Resolver->>Templates: read primary/alternate template, includeLocalePrefixForPrimaryLocale, primaryLocale
Resolver->>Doc: extract locale and fields (slugs, ids)
Resolver->>Resolver: build basePath from chosen template and doc (inlined)
alt prefix needed (primaryLocale undefined OR locale != primaryLocale OR includeLocalePrefixForPrimaryLocale)
Resolver->>Resolver: normalize locale slug
Resolver-->>Caller: return relativePrefixToRoot + localeSlug + "/" + basePath
else no prefix
Resolver-->>Caller: return relativePrefixToRoot + basePath
end
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/directory/DirectoryWrapper.tsx (1)
31-31: Consider replacingany[]with a typed interface fordirectoryChildren.
any[]suppresses type-checking for all child field accesses (child.meta,child.c_addressCountryDisplayName, etc.), making future refactors of this switch block error-prone.♻️ Suggested typing
+type DirectoryChild = { + name: string; + meta?: { entityType?: { id?: string } }; + c_addressCountryDisplayName?: string; + c_addressRegionDisplayName?: string; + [key: string]: unknown; +}; + export const DirectoryList = ({ streamDocument, directoryChildren, relativePrefixToRoot, }: { streamDocument: StreamDocument; - directoryChildren: any[]; + directoryChildren: DirectoryChild[]; relativePrefixToRoot: string; }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/directory/DirectoryWrapper.tsx` at line 31, The prop typed as directoryChildren: any[] in DirectoryWrapper should be replaced with a concrete interface to restore type-safety; define an interface (e.g., DirectoryChild) listing the fields accessed in the component/switch (for example meta, c_addressCountryDisplayName, id, label, children, etc.), update the prop type to DirectoryChild[] and adjust any local variables or function signatures that consume directoryChildren so the compiler can catch invalid accesses inside the switch that references child.meta and other properties.
🤖 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/directory/DirectoryWrapper.tsx`:
- Line 31: The prop typed as directoryChildren: any[] in DirectoryWrapper should
be replaced with a concrete interface to restore type-safety; define an
interface (e.g., DirectoryChild) listing the fields accessed in the
component/switch (for example meta, c_addressCountryDisplayName, id, label,
children, etc.), update the prop type to DirectoryChild[] and adjust any local
variables or function signatures that consume directoryChildren so the compiler
can catch invalid accesses inside the switch that references child.meta and
other properties.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/directory/DirectoryWrapper.tsx
benlife5
requested review from
Fondryext,
asanehisa,
briantstephan,
jwartofsky-yext and
mkilpatrick
mkilpatrick
previously approved these changes
Feb 24, 2026
asanehisa
previously approved these changes
Feb 25, 2026
Contributor
Author
|
Updated: Links from the locator to entity pages were not working. This is because |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.test.ts (1)
88-91: Same fixture flag mismatch as other URL tests.This mock also uses
includePrimaryLocalePrefixForPrimaryLocale, while resolver logic readsincludeLocalePrefixForPrimaryLocale, so coverage for that branch is inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.test.ts` around lines 88 - 91, The test fixture uses the wrong flag name: change the JSON key from "includePrimaryLocalePrefixForPrimaryLocale" to "includeLocalePrefixForPrimaryLocale" so the mock aligns with the resolver logic (the code path checked by legacyResolveUrlTemplate / resolver that reads includeLocalePrefixForPrimaryLocale will then be covered); update the entityPageSetUrlTemplates JSON string accordingly in the test fixture.
🤖 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/utils/urls/legacyResolveUrlTemplate.test.ts`:
- Around line 146-154: The test title is misleading: update the second it(...)
title that currently reads "handles primary locale on new locator pages" to
reflect that the input sets locale: "es" (a non-primary locale); locate the test
calling legacyResolveUrlTemplate with { ...mockNewLocatorMergedDocument, locale:
"es" } and change its description to something like "handles non-primary locale
on new locator pages" so the test name matches the scenario.
In `@packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts`:
- Around line 72-83: The resolvedUrl returned from buildUrlFromTemplate can be
undefined and is being interpolated into strings, causing "undefined" to appear
in paths; update legacyResolveUrlTemplate to guard resolvedUrl (the variable
produced by buildUrlFromTemplate) before interpolation—either coerce it to a
safe fallback (e.g., empty string) or return/throw early when undefined, then
construct the returned path using normalized locale and relativePrefixToRoot
only when resolvedUrl is present; ensure the logic still respects
urlTemplates.primaryLocale and urlTemplates.includeLocalePrefixForPrimaryLocale
and continues to call normalizeSlug(locale) only in the branch that includes the
locale prefix.
- Around line 75-79: The condition currently uses the "in" operator on
urlTemplates to check primaryLocale and may treat { primaryLocale: undefined }
as present; change the if in legacyResolveUrlTemplate to verify a defined,
non-empty value (e.g., urlTemplates.primaryLocale != null &&
urlTemplates.primaryLocale !== '' or Boolean(urlTemplates.primaryLocale)) before
comparing to locale or checking includeLocalePrefixForPrimaryLocale, so replace
the `"primaryLocale" in urlTemplates` check with this defined-value check for
urlTemplates.primaryLocale while keeping the existing comparisons to locale and
urlTemplates.includeLocalePrefixForPrimaryLocale.
In `@packages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts`:
- Around line 218-226: The test title duplicates the primary-locale description
while exercising a non-primary locale; update the spec title for the test that
calls resolveUrlTemplateOfChild with { ...mockChildProfile, locale: "es" } so it
clearly describes the non-primary-locale case (e.g., "resolves locator profile
links for non-primary locale") to make failures interpretable; change only the
it(...) string for that test while leaving the test body and the call to
resolveUrlTemplateOfChild, mockLocatorDocument, and expected assertion intact.
- Around line 53-56: The test fixture uses the flag
includePrimaryLocalePrefixForPrimaryLocale but the resolver
(legacyResolveUrlTemplate.ts) expects includeLocalePrefixForPrimaryLocale;
update the fixture's entityPageSetUrlTemplates JSON to use
includeLocalePrefixForPrimaryLocale so the test exercises the resolver path (or
alternatively update legacyResolveUrlTemplate.ts to accept the
includePrimaryLocalePrefixForPrimaryLocale key if you prefer backward
compatibility); ensure the change is applied where entityPageSetUrlTemplates is
defined in the test so the resolver reads the intended flag.
---
Duplicate comments:
In `@packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.test.ts`:
- Around line 88-91: The test fixture uses the wrong flag name: change the JSON
key from "includePrimaryLocalePrefixForPrimaryLocale" to
"includeLocalePrefixForPrimaryLocale" so the mock aligns with the resolver logic
(the code path checked by legacyResolveUrlTemplate / resolver that reads
includeLocalePrefixForPrimaryLocale will then be covered); update the
entityPageSetUrlTemplates JSON string accordingly in the test fixture.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.test.tspackages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.tspackages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.test.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts (2)
47-62: Fixture field name now matches the resolver — past mismatch is resolved.
includeLocalePrefixForPrimaryLocalealigns with the field read inlegacyResolveUrlTemplate.tsLine 54.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts` around lines 47 - 62, The fixture field in mockLocatorDocument must match what legacyResolveUrlTemplate reads: update mockLocatorDocument (type StreamDocument) so its __.entityPageSetUrlTemplates JSON uses the key includeLocalePrefixForPrimaryLocale (not the old name) and ensure primaryLocale and primary remain present; this aligns the test fixture with legacyResolveUrlTemplate's expectations and fixes the mismatch.
212-245: Solid coverage of all three locator locale-prefix scenarios.Primary locale without prefix (Line 212), non-primary locale with prefix (Line 218), and primary locale with
includeLocalePrefixForPrimaryLocale: trueforcing the prefix (Line 228). Test titles are now distinct and descriptive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts` around lines 212 - 245, The tests for resolveUrlTemplateOfChild already cover the three locator locale-prefix scenarios (primary without prefix, non-primary with prefix, and primary with includeLocalePrefixForPrimaryLocale true), so no code changes are needed; please remove the duplicate/erroneous review marker ([duplicate_comment]) from the PR comment or update the review to a plain approval to avoid confusion and keep the descriptive test titles as-is.
🧹 Nitpick comments (1)
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts (1)
51-56: Consider extracting an explicit type forurlTemplates.The inline type
{ primary?: string; alternate?: string; includeLocalePrefixForPrimaryLocale?: boolean; primaryLocale?: string; }is used inlegacyResolveUrlTemplateWithTemplatesand a narrower version inselectUrlTemplate. As this shape grows, a named type (e.g.,UrlTemplateConfig) shared between both functions would improve readability and keep the two call sites in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts` around lines 51 - 56, Extract a shared named type for the urlTemplates object (e.g., UrlTemplateConfig) and use it in both legacyResolveUrlTemplateWithTemplates and selectUrlTemplate instead of the inline type; define UrlTemplateConfig with fields primary?: string, alternate?: string, includeLocalePrefixForPrimaryLocale?: boolean, primaryLocale?: string and replace the inline annotations in both function signatures/params so the shape is centralized and remains consistent as it evolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts`:
- Around line 47-62: The fixture field in mockLocatorDocument must match what
legacyResolveUrlTemplate reads: update mockLocatorDocument (type StreamDocument)
so its __.entityPageSetUrlTemplates JSON uses the key
includeLocalePrefixForPrimaryLocale (not the old name) and ensure primaryLocale
and primary remain present; this aligns the test fixture with
legacyResolveUrlTemplate's expectations and fixes the mismatch.
- Around line 212-245: The tests for resolveUrlTemplateOfChild already cover the
three locator locale-prefix scenarios (primary without prefix, non-primary with
prefix, and primary with includeLocalePrefixForPrimaryLocale true), so no code
changes are needed; please remove the duplicate/erroneous review marker
([duplicate_comment]) from the PR comment or update the review to a plain
approval to avoid confusion and keep the descriptive test titles as-is.
---
Nitpick comments:
In `@packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts`:
- Around line 51-56: Extract a shared named type for the urlTemplates object
(e.g., UrlTemplateConfig) and use it in both
legacyResolveUrlTemplateWithTemplates and selectUrlTemplate instead of the
inline type; define UrlTemplateConfig with fields primary?: string, alternate?:
string, includeLocalePrefixForPrimaryLocale?: boolean, primaryLocale?: string
and replace the inline annotations in both function signatures/params so the
shape is centralized and remains consistent as it evolves.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.test.tspackages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.tspackages/visual-editor/src/utils/urls/resolveUrlTemplate.test.ts
mkilpatrick
reviewed
Feb 25, 2026
Collaborator
mkilpatrick
left a comment
mkilpatrick
left a comment
There was a problem hiding this comment.
Can you add the latest update to the PR description
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts (1)
82-88: Compare normalized locale values before deciding to prefix.The condition compares raw locale strings but emits a normalized locale in the path. Normalizing both sides for the comparison avoids false non-primary matches due to casing/format differences.
💡 Proposed hardening
- if ( - urlTemplates.primaryLocale && - (urlTemplates.primaryLocale !== locale || - urlTemplates.includeLocalePrefixForPrimaryLocale) - ) { - return `${relativePrefixToRoot}${normalizeSlug(locale)}/${resolvedUrl}`; - } + const normalizedLocale = normalizeSlug(locale); + const normalizedPrimaryLocale = urlTemplates.primaryLocale + ? normalizeSlug(urlTemplates.primaryLocale) + : undefined; + + if ( + normalizedPrimaryLocale && + (normalizedPrimaryLocale !== normalizedLocale || + urlTemplates.includeLocalePrefixForPrimaryLocale) + ) { + return `${relativePrefixToRoot}${normalizedLocale}/${resolvedUrl}`; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts` around lines 82 - 88, The locale-comparison uses raw values but the emitted path uses normalizeSlug(locale), causing mismatches; update the conditional in legacyResolveUrlTemplate (the block checking urlTemplates.primaryLocale and urlTemplates.includeLocalePrefixForPrimaryLocale) to compare normalized values by calling normalizeSlug on both urlTemplates.primaryLocale and locale (e.g., const normalizedPrimary = normalizeSlug(urlTemplates.primaryLocale); const normalizedLocale = normalizeSlug(locale)) and then use (normalizedPrimary !== normalizedLocale || urlTemplates.includeLocalePrefixForPrimaryLocale) so prefixing decisions match the actual path produced.
🤖 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/utils/urls/legacyResolveUrlTemplate.ts`:
- Around line 82-88: The locale-comparison uses raw values but the emitted path
uses normalizeSlug(locale), causing mismatches; update the conditional in
legacyResolveUrlTemplate (the block checking urlTemplates.primaryLocale and
urlTemplates.includeLocalePrefixForPrimaryLocale) to compare normalized values
by calling normalizeSlug on both urlTemplates.primaryLocale and locale (e.g.,
const normalizedPrimary = normalizeSlug(urlTemplates.primaryLocale); const
normalizedLocale = normalizeSlug(locale)) and then use (normalizedPrimary !==
normalizedLocale || urlTemplates.includeLocalePrefixForPrimaryLocale) so
prefixing decisions match the actual path produced.
mkilpatrick
approved these changes
Feb 25, 2026
asanehisa
approved these changes
Feb 25, 2026
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
3 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.
Uh oh!
There was an error while loading. Please reload this page.