feat(vtex-proxy): add props to exclude and add sitemap entries#1549
Open
pedrobernardina wants to merge 2 commits intodeco-cx:mainfrom
Open
feat(vtex-proxy): add props to exclude and add sitemap entries#1549pedrobernardina wants to merge 2 commits intodeco-cx:mainfrom
pedrobernardina wants to merge 2 commits intodeco-cx:mainfrom
Conversation
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
Contributor
📝 WalkthroughWalkthroughThe changes extend sitemap handling by introducing entry exclusion capabilities alongside inclusion logic. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vtex/handlers/sitemap.ts`:
- Around line 86-95: The current flow calls includeSiteMaps(...) then
excludeSitemapEntries(...), which removes any upstream entries that were just
replaced and prevents re-injecting custom sitemap entries; instead call
excludeSitemapEntries on the original response content (the value passed into
includeSiteMaps — i.e., text.replaceAll(publicUrl, `${reqUrl.origin}/`)) first
using the same excludeSiteMapEntry predicate, then pass that filtered content
into includeSiteMaps along with reqUrl.origin and include so upstream entries
are removed before re-injection; update references in sitemap handler (functions
includeSiteMaps, excludeSitemapEntries and the variables text, publicUrl,
reqUrl.origin, include, excludeSiteMapEntry) and return the result of
includeSiteMaps(filteredContent, reqUrl.origin, include).
In `@vtex/loaders/proxy.ts`:
- Around line 192-196: The schema comment for includeSiteMapWithHandler is
misleading because IncludeSiteMapEntry[] does not accept raw string paths;
update the JSDoc above includeSiteMapWithHandler to remove the suggestion that a
plain string like "/sitemap/blog.xml" is valid and instead state that entries
must be objects with path and handler (mention __resolveType for dynamic
sitemaps) or, if you intend to support raw strings, change the TypeScript type
to include string (e.g., IncludeSiteMapEntry | string) and update related
validation/consumers accordingly; reference includeSiteMapWithHandler and
IncludeSiteMapEntry when making the change.
- Around line 38-43: normalizeIncludeSiteMap and includeEntriesToPaths currently
return/emit raw entries which can contain duplicate IncludeSiteMapEntry.path
values causing duplicate <loc> and custom routes; update normalizeIncludeSiteMap
to both normalize (default to empty array) and deduplicate entries by path, and
change includeEntriesToPaths to produce a list of unique, normalized paths
(e.g., map to path, filter out falsy, use a Set or keyed dedupe) so downstream
sitemap materialization and route generation use unique paths; apply the same
dedupe/normalization logic to the analogous functions around the other block
mentioned (the functions referenced by includeEntriesToPaths and
normalizeIncludeSiteMap in the later section).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47fef74d-baa3-4ed3-8cee-16ea25220d07
📒 Files selected for processing (2)
vtex/handlers/sitemap.tsvtex/loaders/proxy.ts
Comment on lines
+86
to
+95
| const withIncludes = includeSiteMaps( | ||
| text.replaceAll(publicUrl, `${reqUrl.origin}/`), | ||
| reqUrl.origin, | ||
| include, | ||
| ); | ||
|
|
||
| const filtered = excludeSitemapEntries(withIncludes, excludeSiteMapEntry); | ||
|
|
||
| return new Response( | ||
| includeSiteMaps( | ||
| text.replaceAll(publicUrl, `${reqUrl.origin}/`), | ||
| reqUrl.origin, | ||
| include, | ||
| ), | ||
| filtered, |
Contributor
There was a problem hiding this comment.
Exclude upstream entries before re-injecting custom ones.
Because vtex/loaders/proxy.ts:131-155 pushes includeSiteMapWithHandler paths into include, filtering after includeSiteMaps() removes the replacement entry too. Excluding /sitemap/foo.xml and re-adding the same path currently leaves no <sitemap> block for it in /sitemap.xml.
🐛 Proposed fix
- const withIncludes = includeSiteMaps(
- text.replaceAll(publicUrl, `${reqUrl.origin}/`),
- reqUrl.origin,
- include,
- );
-
- const filtered = excludeSitemapEntries(withIncludes, excludeSiteMapEntry);
+ const withoutExcluded = excludeSitemapEntries(
+ text.replaceAll(publicUrl, `${reqUrl.origin}/`),
+ excludeSiteMapEntry,
+ );
+
+ const filtered = includeSiteMaps(
+ withoutExcluded,
+ reqUrl.origin,
+ include,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const withIncludes = includeSiteMaps( | |
| text.replaceAll(publicUrl, `${reqUrl.origin}/`), | |
| reqUrl.origin, | |
| include, | |
| ); | |
| const filtered = excludeSitemapEntries(withIncludes, excludeSiteMapEntry); | |
| return new Response( | |
| includeSiteMaps( | |
| text.replaceAll(publicUrl, `${reqUrl.origin}/`), | |
| reqUrl.origin, | |
| include, | |
| ), | |
| filtered, | |
| const withoutExcluded = excludeSitemapEntries( | |
| text.replaceAll(publicUrl, `${reqUrl.origin}/`), | |
| excludeSiteMapEntry, | |
| ); | |
| const filtered = includeSiteMaps( | |
| withoutExcluded, | |
| reqUrl.origin, | |
| include, | |
| ); | |
| return new Response( | |
| filtered, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/handlers/sitemap.ts` around lines 86 - 95, The current flow calls
includeSiteMaps(...) then excludeSitemapEntries(...), which removes any upstream
entries that were just replaced and prevents re-injecting custom sitemap
entries; instead call excludeSitemapEntries on the original response content
(the value passed into includeSiteMaps — i.e., text.replaceAll(publicUrl,
`${reqUrl.origin}/`)) first using the same excludeSiteMapEntry predicate, then
pass that filtered content into includeSiteMaps along with reqUrl.origin and
include so upstream entries are removed before re-injection; update references
in sitemap handler (functions includeSiteMaps, excludeSitemapEntries and the
variables text, publicUrl, reqUrl.origin, include, excludeSiteMapEntry) and
return the result of includeSiteMaps(filteredContent, reqUrl.origin, include).
Comment on lines
+38
to
+43
| const normalizeIncludeSiteMap = ( | ||
| includeSiteMap?: IncludeSiteMapEntry[], | ||
| ): IncludeSiteMapEntry[] => (includeSiteMap ?? []); | ||
|
|
||
| const includeEntriesToPaths = (entries: IncludeSiteMapEntry[]): string[] => | ||
| entries.map((e) => e.path); |
Contributor
There was a problem hiding this comment.
Normalize and dedupe sitemap paths before materializing them.
As vtex/handlers/sitemap.ts:8-27 currently emits one <sitemap> block per include item, the raw concatenation here can produce duplicate <loc> entries. Repeated IncludeSiteMapEntry.path values also generate duplicate custom routes.
♻️ Proposed fix
const normalizeIncludeSiteMap = (
includeSiteMap?: IncludeSiteMapEntry[],
-): IncludeSiteMapEntry[] => (includeSiteMap ?? []);
+): IncludeSiteMapEntry[] => {
+ const byPath = new Map<string, IncludeSiteMapEntry>();
+ for (const entry of includeSiteMap ?? []) {
+ byPath.set(entry.path, entry);
+ }
+ return [...byPath.values()];
+};
+
+const dedupePaths = (paths: string[]) => [...new Set(paths)];
const includeEntriesToPaths = (entries: IncludeSiteMapEntry[]): string[] =>
entries.map((e) => e.path);
@@
const [include, routes] = generateDecoSiteMap
? [
- [
+ dedupePaths([
...includeEntriesToPaths(entries),
...(includeSiteMap ?? []),
decoSiteMapUrl,
- ],
+ ]),
[
...customSitemapRoutes,
{
@@
]
: [
- [...includeEntriesToPaths(entries), ...(includeSiteMap ?? [])],
+ dedupePaths([
+ ...includeEntriesToPaths(entries),
+ ...(includeSiteMap ?? []),
+ ]),
customSitemapRoutes,
];Also applies to: 131-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/loaders/proxy.ts` around lines 38 - 43, normalizeIncludeSiteMap and
includeEntriesToPaths currently return/emit raw entries which can contain
duplicate IncludeSiteMapEntry.path values causing duplicate <loc> and custom
routes; update normalizeIncludeSiteMap to both normalize (default to empty
array) and deduplicate entries by path, and change includeEntriesToPaths to
produce a list of unique, normalized paths (e.g., map to path, filter out falsy,
use a Set or keyed dedupe) so downstream sitemap materialization and route
generation use unique paths; apply the same dedupe/normalization logic to the
analogous functions around the other block mentioned (the functions referenced
by includeEntriesToPaths and normalizeIncludeSiteMap in the later section).
Comment on lines
+192
to
+196
| /** | ||
| * @title Other site maps to include | ||
| * @description URL path (e.g. "/sitemap/blog.xml") or object with path + handler (__resolveType) to register a route and add to the index. Use the object form for dynamic sitemaps (e.g. from Sanity). | ||
| */ | ||
| includeSiteMapWithHandler?: IncludeSiteMapEntry[]; |
Contributor
There was a problem hiding this comment.
The schema description still implies raw strings are valid here.
includeSiteMapWithHandler is typed as IncludeSiteMapEntry[], but this text reads like the field accepts either "/sitemap/foo.xml" or an object. That is likely to trip up anyone editing site.json directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/loaders/proxy.ts` around lines 192 - 196, The schema comment for
includeSiteMapWithHandler is misleading because IncludeSiteMapEntry[] does not
accept raw string paths; update the JSDoc above includeSiteMapWithHandler to
remove the suggestion that a plain string like "/sitemap/blog.xml" is valid and
instead state that entries must be objects with path and handler (mention
__resolveType for dynamic sitemaps) or, if you intend to support raw strings,
change the TypeScript type to include string (e.g., IncludeSiteMapEntry |
string) and update related validation/consumers accordingly; reference
includeSiteMapWithHandler and IncludeSiteMapEntry when making the change.
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
1 participant
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.
What is this Contribution About?
This PR improves the sitemap flexibility of the
vtex/loaders/proxy.tsloader by introducing two new configuration options.1.
excludeSiteMapEntryAllows removing specific entries from the generated sitemap.
This is useful when the upstream sitemap contains routes that should not be exposed by the storefront or should be handled differently.
2.
includeSiteMapWithHandlerAllows adding new sitemap entries while specifying the handler responsible for that route.
Currently the existing
includeSiteMapoption only appends a path to the sitemap without allowing a handler to be associated with it. When a route requires a specific handler, this usually requires creating a dedicated loader.With
includeSiteMapWithHandler, it becomes possible to register both the route and its handler directly in the proxy configuration, avoiding the need to create additional loaders for simple cases.This change was motivated by the need to generate custom sitemap entries for content coming from external sources (e.g. Sanity CMS) without introducing additional loader boilerplate.
Example
Issue Link
No related issue.
Loom Video
N/A
Demonstration Link
Original Sitemap
https://www.oficina.com/sitemap.xml
Sitemap
https://sites-oficina-reserva--858qfs.decocdn.com/sitemap.xml
The Post Posts
https://sites-oficina-reserva--858qfs.decocdn.com/sitemap/the-post-posts.xml
The Post Categories
https://sites-oficina-reserva--858qfs.decocdn.com/sitemap/the-post-categories.xml
Care Guide Posts
https://sites-oficina-reserva--858qfs.decocdn.com/sitemap/care-guide-posts.xml
Summary by cubic
Add flexible sitemap controls to
vtex/loaders/proxy.tsso you can exclude upstream entries and include new entries with custom handlers. Updatesvtex/handlers/sitemap.tsto filter sitemap index entries at runtime.excludeSiteMapEntry: removes matching<sitemap><loc>entries from the final sitemap index (match by URL or path suffix) invtex/handlers/sitemap.ts.includeSiteMapWithHandler: registers routes and adds them to the index in one place (path + optional handler, e.g.website/handlers/sitemap.ts), avoiding extra loaders for custom sitemaps. Backwards compatible withincludeSiteMap.Written for commit 77540a7. Summary will update on new commits.
Summary by CodeRabbit
Release Notes