Seo/sitemap filter orphan entries - #1662
Closed
pedrobernardina wants to merge 50 commits into
Closed
Conversation
…shtoken/webstore” in refreshToken and setLoginCookies.
… actions; streamline session management
…ate cookie management for sign-in and logout
…icated due to the deco's native segment
…es' to defaultItems
…ession type instead of GetSessionResponse
…ateSession on successful auth status
- Replace boolean `ignoreStructuredData` with explicit `structuredDataControl` options - Fix bug where bots still received JSON-LD even when `ignoreStructuredData=true` - Add "always include" (default), "disable for users", and "disable for all" modes - Deprecate `ignoreStructuredData` with backward compatibility - Extract shared logic to `shouldIncludeStructuredData` utility
…y and clarity in Logout.tsx
* feat(proxy): add prop to exclude entries from final sitemap * feat(vtex-proxy): make possible to add custom handlers to proxy --------- Co-authored-by: Pedro Bernardina <pedro.guidance@gmail.com>
Co-authored-by: Pedro Bernardina <pedro.guidance@gmail.com>
classic/setpassword now requires the session minted by POST /authentication/startlogin (sent via the _vss cookie); the older GET /authentication/start token passed in the request body is rejected with an empty-body HTTP 500 on VTEX ID Authenticator-migrated accounts, breaking password change. Switch resetPassword to the startlogin flow used by vtex.react-vtexid and FastStore useSetPassword (startlogin -> _vss cookie -> setpassword?expireSessions=true, no authenticationToken in the body). Add startlogin + expireSessions to the VTEXCommerceStable client type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
VTEX returns auth rejections (WrongCredentials, BlockedUser) from setpassword as a non-2xx whose body is a valid AuthResponse. fetchSafe wrapped that in an HttpError, so resetPassword/recoveryPassword bubbled it up and the client choked on a JSON.parse SyntaxError — showing a broken/generic screen instead of the real message ("Senha atual incorreta").
Catch the HttpError, recover the AuthResponse via authResponseFromHttpError, and return it so the client maps it to a message. Unexpected failures are logged and rethrown as a typed HttpError. Guard validateSession so a post-success session failure isn't mistaken for a password-change failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fresh.ts gains a structured-JSON exit alongside the existing ?asJson one:
?renderJson (legacy alias ?appJson — remove once consumers switch) resolves
the page with hooks that short-circuit sections opted out of JSON rendering
(their loaders never run), serializes the tree via @deco/deco
serializeResolvedSection honoring each section's `renderJson` export, and
responds { name, path, sections } with lazy sections as
{ component, lazyUrl } placeholders. One-shot JSON responses never use
async render (firstByteThreshold guards extended); renderJson takes
precedence when both params are sent; legacy ?asJson is untouched.
The website app gains `renderJson.sectionsToIgnore` (admin-configurable):
app-owned sections excluded by resolveType suffix — site-owned sections
should prefer `export const renderJson = false` in their own file.
Replaces the site-level wrapper pattern (handler + pages-loader fork) that
oficina-reserva ran as v1 — the default website/loaders/pages.ts now works
unchanged for renderJson consumers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fresh.ts renderJson branch imports symbols (sectionModuleLookup, serializeResolvedSection, computeRenderCb) that only exist in our runtime fork — upstream jsr:@deco/deco doesn't have them, so standalone deno check broke. Raw URLs don't resolve subpaths via the package exports map, hence one entry per subpath used, plus @deco/deno-ast-wasm which the runtime graph self-references. Pre-existing vtex/mod.ts type-constraint errors remain untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
renderJson is the replacement, not a sibling — consumers adopt ?renderJson directly (validation happens on PR previews, nothing in production speaks ?appJson). Only the legacy ?asJson remains as a separate, untouched mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Repin @deco/deco from d7f9cc2c (render-json tip) to c0228316, the deco-runtime main-alt merge commit, so apps tracks the full main-alt (render-json + daemon/cache/version fixes) instead of just render-json. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings 31 upstream commits (no-store on personalizing loaders, no-cache on extension wrappers, image quality context, userSegment matcher signature, spire removal, version 0.158.0). Our @deco/deco fork pin (deco-runtime @c0228316) and the render-json work are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… it) The render-json fork-pin (34931e7) replaced jsr:@deco/deco with 6 explicit jsdelivr subpaths but omitted scripts/bundle, so scripts/start.ts's import("@deco/deco/scripts/bundle") no longer resolved and CI (ci.yaml -> start.ts) broke. Map it to jsr like the bundle task and the site already do: the bundle is a build tool untouched by render-json, and jsr resolves its transitive deps (codemod-toolkit, std) automatically. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greens the apps CI after the render-json + upstream consolidation: - website/handlers/fresh.ts (render-json edit) was committed without deno fmt -> failed the `deno fmt --check` in the Check step. - the manifest generation (start.ts -> bundle) pulls @deco/deno-ast-wasm, which was missing from deno.lock -> would fail "Check if there are changes". Validated end-to-end with deno 2.4.5 (CI's exact version): fmt --check, lint, and check **/mod.ts all clean; Bundle regenerates manifests with no drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Picks up the renderJson review fixes (omit no-op, tightened RenderJson return, dropped Murmurhash3 re-export, lazy resolveChain comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sectionsToIgnore is admin config (external JSON), not type-guaranteed at
runtime. Keep only non-empty string suffixes: a blank entry would make
endsWith("") match every section (dropping the whole page), and a non-string
would throw on .trim() (500ing the request).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ad/writes profile/address/payment/user store-graphql calls forwarded only the VtexId cookies (parseCookie), dropping vtex_session — so @withCurrentProfile could not resolve a session-scoped (e.g. telesales-impersonated) profile and writes misdirected. Forward the full request cookie, matching the cart actions. updateProfile also injected payload.sub as the profile email; under an operator admin token that is the OPERATOR's email, which would re-key the impersonated customer's account. Derive the target email from the impersonated-customer session cookie when the token audience is admin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The block read `vtex-impersonated-customer-email` as a COOKIE and threw
"Could not resolve the target profile email" when it was absent. Nothing ever
writes that cookie — impersonation sets it as a session public property via
editSession — so the throw fired on every profile update under a telesales
session.
It was also unnecessary: store-graphql discards the field
(node/resolvers/profile/services.ts, `const { email, ...profileWithoutEmail }`,
since 2.173.2) and writes to the session-resolved currentProfile. The forwarded
cookie is what targets the write; this field is only present because
ProfileInput.email is String! in the schema.
So `email: payload.sub` is restored with a one-line why, the client-writable
cookie read is gone, and the block's security argument goes with it — there is
nothing left to defend.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t reads and writes
Picks up upstream deco-cx/deco v1.203.0, merged into our main-alt. Clean merge: none of the 4 upstream commits touch our 7 customized runtime files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Upstream deco-cx/apps v0.160.0 (292aed0), 50 non-merge commits. Two conflicts, both from d2e9108 ("remove deprecated firstByteThresholdMS config"), resolved by adopting the upstream removal: website/handlers/fresh.ts — kept our renderJson/isJsonOneShot lines, took upstream's `?? 0`, dropped `delayFromProps`. website/mod.ts — kept the renderJson block, dropped firstByteThresholdMS. The removal is bit-identical for this site: .deco/blocks/site.json has `"firstByteThresholdMS": false`, so `false ? 1 : 0` === `?? 0`. Worth recording, because it is the trap here: the same commit also strips `| "firstByteThresholdMS"` from the `Pick<AppContext>` in fresh.ts's signature, and three-way merge resolves THAT silently, with no marker. Keeping the HEAD side of the marked hunk — the instinct — leaves the body reading a field the type no longer has, and only `deno check` catches it. Verified after resolution: zero `firstByteThresholdMS` left in .ts/.tsx, and deno fmt/lint/check all exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sitemap is proxied from VTEX, so it lists whatever the platform's category tree holds. Categories are created upstream by a different team than the one that builds the storefront pages, so an entry can point at a path no page serves — which answers 404. Announcing such a URL to crawlers is worse than omitting it. Behind `removeEntriesWithoutPage`, every <url> is now matched against the paths of the pages that actually exist, read with the same blockSelector website/loaders/pages.ts uses. Dropped entries are logged so the missing page can be created; the entry returns on its own once it is. The catch-all page is deliberately left out of the matchers: it matches every path, and counting it would make the filter inert. Also matches the storefront host instead of the exact configured URL when rewriting <loc>, so a variation the platform emits — http, or no trailing slash — cannot leak the platform host into the index.
Three defects from review, none of which the earlier commit could have shown working: The flag never reached the handler. The proxy loader builds the sitemap routes with a fixed set of props, and removeEntriesWithoutPage was not among them, so the handler always saw undefined and the filter never ran. It is now threaded through the loader to both sitemap routes. The matcher only imitated the router. Hand-rolled as a regex, it diverged on case, on trailing slashes and on percent-encoding: a page at /maçã does answer /ma%C3%A7%C3%A3 but would have been dropped, while /foo/ and /FOO, which answer nothing, would have been kept. It now builds the very same URLPattern the router builds, which also fixes a false positive found against production data — a page whose path carries a query string was being dropped, because the regex escaped the template whole. An empty page list disabled the filter, conflating a valid read with a failed one, which the catch already covers. The filter now always runs; what is refused instead is the narrower and unrecoverable case of dropping every entry, since an empty sitemap withdraws the whole site from the index and nothing downstream would notice.
Three defects from review. The rewritten body was returned under the upstream's framing headers. It is no longer the gzip stream content-encoding claims, nor the length content-length states — a mismatch that predates the filter, since rewriting the host already shortens every <loc>. Both headers are dropped from a clone of the response's. The guard against emptying a sitemap watched the symptom instead of the cause. Each child sitemap is handled on its own, so one made entirely of orphan entries is a legitimate emptying, and refusing it left those URLs published. What is unrecoverable is having no page to match against at all — that would empty every sitemap — so the guard now sits there. A <loc> holds XML and arrives entity-encoded, so a URL written as "?a=1&b=2" was compared literally against a route the router sees as "?a=1&b=2", dropping pages whose URL carries more than one parameter. Entities are decoded, numeric references included, before matching.
The sitemap is no longer rewritten. Removing entries meant reserializing a body the upstream's headers no longer described, and deciding on the storefront's behalf which URLs a crawler may see; a warning gives whoever can create the missing page the same information without either. The response goes back out exactly as it came in, and the prop says so. The check itself is now taken from the routes actually being served, read from the request state the way website/handlers/sitemap.ts does, rather than from the raw page blocks: a page hidden by hidePagesInDeco answers only with ?rdc=true, and reading its block would have called it reachable. Catch-all routes are recognized by probing what they answer instead of by their spelling, since the router normalizes "/*", "/(.*)" and an absolute "https://store.test/*" to the same pattern and any of them would have made the check vacuous. Routes with no pattern syntax — nearly all of them — are indexed in a set, so an orphan entry no longer walks every route: 10k orphan URLs against 500 routes went from seconds to 92ms, and the protocol allows 50k URLs per file. The log carries a count and a bounded sample instead of every URL, which on a large sitemap was megabytes to the console and the logger both. Finally, the host rewrite now requires the host to end where the match does, with an optional port. Unanchored, "shop.example.com" rewrote through the prefix of "shop.example.com.br" and mangled ":443" URLs.
Restores the removal. A URL the platform lists but no page serves answers 404, and announcing it to crawlers is worse than omitting it; it returns on its own once the page exists, since nothing here is persisted. Two defects from review, both of which removal turns from noise into damage. A catch-all was recognized by probing an improbable path, which "/:department/:category" also answers — every two-segment route would have been dropped from the index and every two-segment page erased from the sitemap. The router normalizes each spelling of a catch-all to the pathname "/*", so that is what is compared now. And the scan read every <loc> in the document, including the <sitemap> entries of an index, which name documents rather than pages: an external sitemap added through `include` would have been reported, and now removed. Matching whole <url> blocks confines the check to the urlset, which the removal needed anyway.
The response is decoded and rewritten before being served: the host is swapped in every <loc>, entries are dropped, includes are added. What the upstream sent about its own bytes stops holding — content-length is the length of a different document, content-encoding announces a gzip stream that was already decoded, and etag names the platform's version rather than the one being served, so a client could revalidate its way into a stale body. This predates the entry removal: rewriting the host alone already shortens every <loc>. Deleting the three from a clone lets the server frame the response it is actually sending. VTEX currently sends no etag for the sitemap, so that one is a precaution.
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (53)
📝 WalkthroughWalkthroughThis change adds structured-data controls, VTEX authentication and session handling, sitemap filtering, Fresh JSON rendering, analytics URL parameters, verified-review loading, and no-cache directives for selected product loaders. ChangesCommerce SEO and loader behavior
Verified reviews
VTEX authentication, sessions, and cookies
VTEX sitemap processing
Website JSON rendering
Analytics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant Fresh
participant SectionResolver
participant JSONResponse
Request->>Fresh: request renderJson mode
Fresh->>SectionResolver: resolve sections and apply exclusions
SectionResolver-->>Fresh: resolved sections
Fresh->>JSONResponse: serialize page metadata and sections
JSONResponse-->>Request: return JSON with CORS headers
sequenceDiagram
participant Client
participant AuthenticationAction
participant VTEX
participant SessionValidation
Client->>AuthenticationAction: submit credentials
AuthenticationAction->>VTEX: send credentials and cookie header
VTEX-->>AuthenticationAction: return authentication data and Set-Cookie headers
AuthenticationAction->>SessionValidation: refresh token and validate session
SessionValidation-->>Client: return authentication result
Suggested reviewers: ✨ 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 |
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
2 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.
What is this Contribution About?
Please provide a brief description of the changes or enhancements you are proposing in this pull request.
Issue Link
Please link to the relevant issue that this pull request addresses:
Loom Video
Demonstration Link
Summary by cubic
Overhauls VTEX authentication and session flows so impersonated profiles resolve correctly, and stops the proxied sitemap from announcing URLs that answer 404. Also adds a structured
?renderJsonpage mode and explicit control over SEO structured data.Session and authentication
vtex_session, so profile, address, and payment calls resolve session-scoped profiles.startloginflow withexpireSessionsand returns structuredauthStatusinstead of a generic failure./api/vtexid/refreshtoken/webstore; adds avalidateSessionaction and aLogoutsection with a safereturnUrl.Sitemap and SEO
content-length,content-encoding, oretagheaders.structuredDataControlreplaces the deprecatedignoreStructuredData; bots no longer receive JSON-LD when it is disabled for users.?renderJsonrenders the page as structured JSON;@deco/decois pinned to theoficina-dev/deco-runtimefork to support it.Written for commit 4fcb56e. Summary will update on new commits.
Summary by CodeRabbit