Conversation
|
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. |
commit: |
WalkthroughCentralizes the StreamDocument type into packages/visual-editor/src/utils/types/StreamDocument.ts and updates imports. Adds new URL resolution modules and APIs (resolveUrlFromPathInfo, resolveUrlTemplate, resolveUrlTemplateOfChild) and a legacy resolver (legacyResolveUrlTemplate). Removes the old mergeMeta utility and updates call sites to the new URL resolvers (DirectoryCard, Locator, NearbyLocationCard, etc.). Expands schema resolution with resolveSchemaString and resolveSchemaJson. Removes the puck prop from LocatorResultCard and updates several TypeScript pragma comments and import paths. Sequence Diagram(s)sequenceDiagram
participant Component as ClientComponent
participant Resolver as resolveUrlTemplate
participant PathInfo as resolveUrlFromPathInfo
participant Legacy as legacyResolveUrlTemplate
participant Location as getLocationPath
participant Doc as StreamDocument
Component->>Resolver: resolveUrlTemplate(Doc, relativePrefixToRoot)
Resolver->>PathInfo: try resolveUrlFromPathInfo(Doc)
alt PathInfo returns URL
PathInfo-->>Resolver: URL
else
Resolver->>Legacy: try legacyResolveUrlTemplate(Doc, relativePrefixToRoot)
alt Legacy returns URL
Legacy-->>Resolver: URL
else
Resolver->>Location: try getLocationPath(Doc, relativePrefixToRoot)
Location-->>Resolver: URL | undefined
end
end
Resolver-->>Component: URL or throw if unresolved
Possibly related PRs
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
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}
There was a problem hiding this comment.
Actionable comments posted: 2
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/utils/api/fetchLocalesToPathsForEntity.ts (1)
41-49: Avoid recording empty URLs when resolution fails.
resolvePageSetUrlreturns""on failure (and logs), so the current code will still write an empty path for that locale. This can propagate broken links. Consider skipping the entry (or using a fallback) when the resolved URL is falsy.💡 Suggested guard
- const resolvedUrl = resolvePageSetUrl(mergedDocument, ""); - localeToPath[normalizeLocale(profile.meta.locale)] = resolvedUrl; + const resolvedUrl = resolvePageSetUrl(mergedDocument, ""); + if (resolvedUrl) { + localeToPath[normalizeLocale(profile.meta.locale)] = resolvedUrl; + } else { + console.warn( + `Failed to resolve URL template for locale ${profile.meta.locale}` + ); + }
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/utils/urls/resolveUrlFromPathInfo.ts`:
- Around line 56-71: The isPrimaryLocale function can throw on malformed JSON
when parsing streamDocument?.__?.pathInfo; wrap the JSON.parse call in a
try/catch (inside isPrimaryLocale) and fall back to an empty object
(PathInfoShape default) on parse errors so the function returns safely; keep the
remaining logic unchanged (check pathInfoJson.primaryLocale, then
__.isPrimaryLocale, then default to locale === "en"). Optionally capture/log the
parse error if a logger is available, but ensure the function never throws from
JSON.parse.
- Around line 14-21: The code in resolveUrlFromPathInfo.ts calls JSON.parse on
streamDocument?.__?.pathInfo without guarding against malformed JSON; wrap the
parse in a try-catch inside the resolveUrlFromPathInfo function (or the block
that defines pathInfoJson) and on parse failure return undefined (or otherwise
degrade consistently with the existing missing-template behavior), optionally
logging the parse error for diagnostics; if you intentionally want parse errors
to throw, update the test description to state that throwing on malformed
pathInfo is deliberate instead of changing runtime behavior.
♻️ Duplicate comments (3)
packages/visual-editor/src/components/atoms/cta.tsx (1)
318-321: Same feedback as body.tsx: prefer@ts-expect-errorover@ts-ignore.See the comment on body.tsx for the rationale.
packages/visual-editor/src/components/atoms/toggle.tsx (1)
13-14: Same feedback: prefer@ts-expect-errorover@ts-ignore.See the comment on body.tsx for the rationale.
packages/visual-editor/src/components/atoms/heading.tsx (1)
116-117: Same feedback: prefer@ts-expect-errorover@ts-ignore.See the comment on body.tsx for the rationale.
🧹 Nitpick comments (3)
packages/visual-editor/src/components/atoms/body.tsx (1)
40-41: Consider keeping@ts-expect-errorinstead of@ts-ignore.
@ts-expect-erroris generally preferred over@ts-ignorebecause it will alert you if the underlying TypeScript issue is ever resolved (e.g., if TypeScript starts accepting CSS variables fortextTransform). With@ts-ignore, the suppression remains silently even when no longer needed.Suggested change
- // `@ts-ignore`: the css variable here resolves to a valid enum value + // `@ts-expect-error` the css variable here resolves to a valid enum valuepackages/visual-editor/src/utils/schema/getSchema.ts (1)
1-1: Inconsistent use of.tsextension in import.This import includes the explicit
.tsextension, while other files in this PR (e.g.,schemaBlocks.tsline 1) omit it. Consider removing the extension for consistency:-import { StreamDocument } from "../types/StreamDocument.ts"; +import { StreamDocument } from "../types/StreamDocument";While both may work depending on your TypeScript/bundler configuration, consistent import style across the codebase is preferred.
packages/visual-editor/src/utils/urls/getLocationPath.ts (1)
1-4: Use a type-only import with direct path to avoid unnecessary barrel dependency.
StreamDocumentis used only as a type in the interface declaration (line 6). Import it directly from./types/StreamDocument.tsrather than the barrel to keep the dependency graph cleaner and avoid potential circular references. The direct path already exists in the codebase and aligns with howresolvePageSetUrl.tsimports the same type.♻️ Suggested change
-import { StreamDocument } from "../index.ts"; +import type { StreamDocument } from "../types/StreamDocument.ts";
packages/visual-editor/src/utils/urls/resolveUrlFromPathInfo.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/utils/urls/resolveUrlTemplate.ts`:
- Around line 60-65: The early return that skips DIRECTORY and LOCATOR page sets
prevents extraction of their config.urlTemplate; update the guard in
resolvePageSetUrlTemplate (in resolveUrlTemplate.ts) so it only returns early
when pagesetJson.type is DIRECTORY or LOCATOR AND
pagesetJson.config?.urlTemplate is missing/empty, or simply remove the type
check entirely so the function proceeds to read pagesetJson.config.urlTemplate;
ensure callers' expectations remain valid by leaving resolution intact when
config.urlTemplate exists.
There was a problem hiding this comment.
Actionable comments posted: 0
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/utils/urls/resolveUrlTemplate.test.ts (1)
119-137: Inconsistenttypefield placement in test data.The
type: "DIRECTORY"field is nested insideconfig(line 124), but in other mock documents (mockDirectoryMergedDocumentat line 53,mockLocatorMergedDocumentat line 76),typeis at the root level of the_pagesetJSON. The implementation checkspagesetJson?.type, which expectstypeat the root level.This test passes because
resolveUrlTemplateOfChildusesentityPageSetUrlTemplatesregardless of thetypefield, but the mock data structure is misleading.Suggested fix
const directoryDocWithBothTemplates = { ...mockDirectoryMergedDocument, _pageset: JSON.stringify({ + type: "DIRECTORY", config: { - type: "DIRECTORY", urlTemplate: { primary: "directory/[[address.city]]/[[id]]", alternate: "[[locale]]/directory/[[address.city]]/[[id]]", }, }, }), };
benlife5
left a comment
benlife5
left a comment
There was a problem hiding this comment.
I haven't been following the url template conversations super closely but this LGTM in general
packages/visual-editor/src/utils/urls/resolveUrlFromPathInfo.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/utils/urls/resolveUrlFromPathInfo.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/utils/urls/resolveUrlFromPathInfo.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
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/internal/hooks/layout/useMessageReceivers.ts (1)
81-88: Pass the streamDocument as the child profile.
resolveUrlTemplateOfChildmergesprofileinto the stream document. Passing{}drops fields likeid,address, andslug, which can make child resolvers fail and throw for locator/dm_ entities.🛠️ Proposed fix
- path = resolveUrlTemplateOfChild({}, streamDocument, ""); + path = resolveUrlTemplateOfChild(streamDocument, streamDocument, "");packages/visual-editor/src/utils/schema/resolveSchema.ts (1)
19-42: Avoid double‑escaping embedded values in schema strings.
stringifyResolvedFieldcurrently escapes non‑string values and then the schema is serialized again later, which can produce backslashes in output (double‑escaped JSON). Return the raw stringified value and let the final serialization handle escaping.🛠️ Proposed fix
- const jsonStringLiteral = JSON.stringify(stringToEmbed); - - // We return the content *inside* the quotes, which is the properly escaped string. - return jsonStringLiteral.slice(1, -1); + return stringToEmbed;Based on learnings, pre‑escaping causes double‑escaping when the resolved object is later JSON.stringify’d.
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts`:
- Around line 22-24: Wrap the JSON.parse calls in try-catch to safely handle
malformed JSON: when parsing streamDocument?.__?.entityPageSetUrlTemplates for
urlTemplates and the other parse inside legacyResolveUrlTemplate, catch any
SyntaxError and fall back to an empty object ({}), and log or silently ignore
the error as appropriate; update the parsing logic around the urlTemplates
variable and the parse at line 47 in legacyResolveUrlTemplate to use this
try-catch pattern so invalid JSON does not throw.
In `@packages/visual-editor/src/utils/urls/resolveUrlTemplate.ts`:
- Around line 95-109: The mergeMeta function currently ignores the child
(streamDocument) locale and profile.locale, causing child URLs to inherit the
parent locale; update locale resolution to prefer the child locale first and
fall back to profile meta or profile.locale (e.g., set locale =
streamDocument?.locale || profile?.meta?.locale || profile?.locale), and adjust
isPrimaryLocale to consult streamDocument?.meta?.isPrimaryLocale before falling
back to profile?.meta?.isPrimaryLocale and finally to the locale === "en"
default; make these changes inside mergeMeta to preserve child locale and
correct primary-locale logic.
packages/visual-editor/src/utils/urls/legacyResolveUrlTemplate.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationCard.tsx
Show resolved
Hide resolved
mkilpatrick
left a comment
mkilpatrick
left a comment
There was a problem hiding this comment.
I think this is cleaner!
packages/visual-editor/src/utils/urls/resolveUrlFromPathInfo.ts
Outdated
Show resolved
Hide resolved
4 participants
Resolve urls via:
Example PathInfo json:
live site: https://www.yext.com/s/4412098/yextsites/162218/pagesets
live site with changes to the template's getPath as well: https://www.yext.com/s/4412098/yextsites/162230/pagesets