feat: use breadcrumbPrefix for breadcrumb urls#1006
feat: use breadcrumbPrefix for breadcrumb urls#1006briantstephan wants to merge 23 commits intomainfrom
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: |
…isual-editor into breadcrumbs-url-templates
…isual-editor into breadcrumbs-url-templates
WalkthroughThis PR refactors the breadcrumb resolution logic in the visual editor by introducing a new multi-strategy resolver pattern. The Breadcrumbs component is updated to use a new resolveBreadcrumbs function instead of directly calling getDirectoryParents. The StreamDocument type is modified to replace the breadcrumbTemplates field with breadcrumbPrefix (changing from an optional array to an optional string). New utility functions resolveBreadcrumbsFromPathInfo and buildBreadcrumbsFromDirectory implement resolution strategies that attempt pathInfo-based breadcrumbs first, then fall back to directory-parent-based construction. Comprehensive test coverage is added for both resolution approaches and the orchestrating resolver. Sequence DiagramsequenceDiagram
participant BC as Breadcrumbs Component
participant RB as resolveBreadcrumbs
participant RPI as resolveBreadcrumbsFromPathInfo
participant BD as buildBreadcrumbsFromDirectory
participant DP as getDirectoryParents
participant SD as StreamDocument
BC->>RB: resolveBreadcrumbs(streamDocument)
RB->>RPI: Strategy 1: resolveBreadcrumbsFromPathInfo
activate RPI
RPI->>SD: Read breadcrumbPrefix from pathInfo
RPI->>SD: Read directoryParents array
alt Valid breadcrumbPrefix and directoryParents
RPI->>RPI: Build breadcrumbs with locale prefix
RPI-->>RB: Return BreadcrumbLink[]
else Missing data
RPI-->>RB: Return undefined
end
deactivate RPI
alt Non-empty breadcrumbs from pathInfo
RB-->>BC: Return breadcrumbs
else Empty or undefined
RB->>BD: Strategy 2: buildBreadcrumbsFromDirectory
activate BD
BD->>DP: getDirectoryParents(streamDocument)
DP-->>BD: Return parent array
alt Valid parents found
BD->>BD: Build breadcrumbs from parents + document name
BD-->>RB: Return BreadcrumbLink[]
else No parents
BD-->>RB: Return undefined
end
deactivate BD
RB-->>BC: Return breadcrumbs or empty array
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/utils/urls/resolveBreadcrumbsFromPathInfo.ts`:
- Line 77: The final crumb push in resolveBreadcrumbsFromPathInfo.ts currently
always appends { name: streamDocument.name ?? "", slug: "" } which can produce
an empty breadcrumb label; change this to either skip pushing when
streamDocument.name is falsy (check streamDocument.name before crumbs.push) or
provide a fallback label (e.g., use a defaultTitle or derive from path) so the
breadcrumb list never contains an empty name—align behavior with the filtering
used in resolveBreadcrumbs.ts by applying the same truthy-name check or fallback
logic when adding the final crumb.
🧹 Nitpick comments (5)
packages/visual-editor/src/utils/resolveYextEntityField.ts (1)
1-1: Preferimport typesinceYextEntityFieldis only used as a type here.
YextEntityFieldappears solely in type positions (lines 7 and 57). Switching fromimport typeto a value import means the module won't be elided by TypeScript, which can matter underverbatimModuleSyntax. Consider keeping the type-only import:Suggested fix
-import { YextEntityField } from "../editor/YextEntityFieldSelector.tsx"; +import type { YextEntityField } from "../editor/YextEntityFieldSelector.tsx";packages/visual-editor/src/utils/urls/resolveBreadcrumbsFromPathInfo.ts (2)
22-25: Locale requirement is undocumented and may surprise callers.The function silently returns
undefinedwhen locale is missing. The JSDoc (lines 10-13) only mentionsbreadcrumbPrefixanddm_directoryParents_*as prerequisites but doesn't mention locale is also required. Consider adding locale to the doc, or logging a warning, so debugging is easier when breadcrumbs unexpectedly disappear.
35-42: Dynamic field lookup is pragmatic but fragile if multipledm_directoryParents_*keys exist.
Object.entries(...).find(...)returns the first match. If a document ever contains more than onedm_directoryParents_*field (e.g., from multiple entity types), the selected one is nondeterministic due to object key ordering. This is likely fine for current usage, but worth a brief comment noting the assumption of a single match.packages/visual-editor/src/utils/urls/resolveBreadcrumbs.ts (1)
17-31: Silent error swallowing in the catch block.The bare
catchdiscards all errors, including unexpected ones (e.g., TypeError from a bug). Consider logging at debug/warn level so issues don't silently vanish during development.💡 Suggestion
} catch { - // continue to next resolver + // continue to next resolver + // Consider: console.warn("Breadcrumb resolver failed", e); }packages/visual-editor/src/utils/urls/resolveBreadcrumbsFromPathInfo.test.ts (1)
29-76: Good core coverage; consider adding edge-case tests.The three scenarios (primary locale, non-primary locale, empty prefix) cover the main paths well. Missing coverage for:
breadcrumbPrefixbeingundefined(expected: returnsundefined)- Missing locale (expected: returns
undefined)includeLocalePrefixForPrimaryLocale: truewith the primary locale- Directory parents with missing/malformed entries
These are optional additions for robustness but not blocking.
| }); | ||
|
|
||
| describe("resolveBreadcrumbs", () => { | ||
| it("prefers pathInfo breadcrumbs over directory parents", () => { |
There was a problem hiding this comment.
interesting wording, more like uses pathInfo breadcrumbsPrefix if exists?
| .replace(/\/+/g, "/") | ||
| .replace(/^\/+|\/+$/g, ""); | ||
|
|
||
| const directoryParentsEntry = Object.entries(streamDocument).find( |
There was a problem hiding this comment.
Can this use the same getDirectoryParents as the other function?
| @@ -1,4 +1,4 @@ | |||
| import { type YextEntityField } from "../editor/YextEntityFieldSelector.tsx"; | |||
| import { YextEntityField } from "../editor/YextEntityFieldSelector.tsx"; | |||
| return []; | ||
| }; | ||
|
|
||
| const buildBreadcrumbsFromDirectory = ( |
There was a problem hiding this comment.
Why is this one in this file but resolveBreadcrumbsFromPathInfo in its own file?
| ) => BreadcrumbLink[] | undefined; | ||
|
|
||
| const resolvers: BreadcrumbResolver[] = [ | ||
| (streamDocument) => resolveBreadcrumbsFromPathInfo(streamDocument), |
There was a problem hiding this comment.
One is called resolve and one is called build. Should we keep it consistent?
| ): BreadcrumbLink[] | undefined => { | ||
| const breadcrumbPrefix = streamDocument.__?.pathInfo?.breadcrumbPrefix; | ||
| if (typeof breadcrumbPrefix !== "string") { | ||
| return undefined; |
There was a problem hiding this comment.
You can just return (without undefined)
| return undefined; | ||
| } | ||
|
|
||
| const includeLocalePrefix = |
There was a problem hiding this comment.
I don't think we want to do the same locale prefix logic. It depends on if Spruce has changed the DM config to have the same urlTemplate across all slug values of the DM or if they have the normal DM config to handle locale.
| .replace(/\/+/g, "/") | ||
| .replace(/^\/+|\/+$/g, ""); | ||
|
|
||
| const directoryParentsEntry = Object.entries(streamDocument).find( |
There was a problem hiding this comment.
Can't you just call getDirectoryParents, iterate through it, and apply the prefix?
This updates breadcrumbs to use the new
pathInfo.breadcrumbPrefixfield as the prefix before the directory parent slug.Feel free to test it here: https://dev.yext.com/s/1000167375/yextsites/66601/pagesets