fix: prevent My Account 500s from empty session profile and missing CMS content - #3417
Conversation
|
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 info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRepresentative account profiles now use session and JWT fallbacks with guarded contract fetching. Account server-side props and CMS content loading now tolerate missing profile or page data through null-safe defaults. ChangesAccount profile and page resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant JWT
participant Session
participant QueryAccountProfile
participant MasterData
JWT->>QueryAccountProfile: provide representative identity and customerId
QueryAccountProfile->>Session: read shopper, authentication, and active contract
QueryAccountProfile->>MasterData: fetch contract when contractId exists
MasterData-->>QueryAccountProfile: return contract or error
QueryAccountProfile-->>JWT: return resolved account profile
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
lemagnetic
changed the title
6f7396a to
9ca36d6
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/platforms/vtex/resolvers/query.ts`:
- Around line 752-779: Normalize the JWT customer ID before using it in the
contract lookup and returned profile ID: trim jwt?.customerId and treat an empty
result as unavailable. Update the contractId fallback in the resolver flow
around resolveActiveContractIdFromSession and ensure the returned id fallback
uses the same normalized value, matching the availableContracts behavior.
In `@packages/core/src/server/cms/fetchMyAccountPageContent.ts`:
- Around line 34-35: The fallbacks in fetchMyAccountPageContent and the account
404 page claim the full PageContentType while omitting required settings.seo
fields. Update both packages/core/src/server/cms/fetchMyAccountPageContent.ts
lines 34-35 and packages/core/src/pages/pvt/account/404.tsx line 150 to provide
the complete default SEO object, or narrow both call sites to the sections-only
type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4eb4f967-6eba-4837-9f93-f39fe9908b9f
📒 Files selected for processing (5)
packages/api/src/platforms/vtex/resolvers/query.tspackages/api/test/unit/platforms/vtex/resolvers/query.test.tspackages/core/src/experimental/myAccountServerSideProps.tspackages/core/src/pages/pvt/account/404.tsxpackages/core/src/server/cms/fetchMyAccountPageContent.ts
| return { sections: [], settings: {} } as PageContentType | ||
| })) ?? ({ sections: [], settings: {} } as PageContentType) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== fetchMyAccountPageContent ==\n'
sed -n '1,140p' packages/core/src/server/cms/fetchMyAccountPageContent.ts
printf '\n== account 404 page excerpt ==\n'
sed -n '120,190p' packages/core/src/pages/pvt/account/404.tsx
printf '\n== PageContentType definition search ==\n'
rg -n "type PageContentType|interface PageContentType|PageContentType" packages/core/src -g '!**/node_modules/**'Repository: vtex/faststore
Length of output: 6961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== PageContentType definition ==\n'
sed -n '1,140p' packages/core/src/server/cms/index.ts
printf '\n== myAccountDefaultSections ==\n'
sed -n '1,220p' packages/core/src/server/cms/myAccountDefaultSections.ts
printf '\n== getPage / PageContentType handling ==\n'
sed -n '150,230p' packages/core/src/server/content/service.ts
printf '\n== pvt account 404 data flow ==\n'
sed -n '1,170p' packages/core/src/pages/pvt/account/404.tsxRepository: vtex/faststore
Length of output: 12992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== fetchMyAccountPageContent usages ==\n'
rg -n "fetchMyAccountPageContent\(" packages/core/src
printf '\n== PageContentType field access in the account area ==\n'
rg -n "page\.settings|settings\.seo|globalSettings|PageProvider context=\{\{ globalSettings \}\}" packages/core/src/pages/pvt/account packages/core/src/components -g '!**/node_modules/**'
printf '\n== direct fallback patterns around PageContentType ==\n'
rg -n "as unknown as PageContentType|as PageContentType" packages/core/src/pages/pvt/account packages/core/src/server/cms packages/core/src/server/content/service.tsRepository: vtex/faststore
Length of output: 6369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
packages/core/src/pages/pvt/account/profile.tsx \
packages/core/src/pages/pvt/account/security.tsx \
packages/core/src/pages/pvt/account/403.tsx \
packages/core/src/pages/pvt/account/orders/index.tsx \
packages/core/src/pages/pvt/account/user-details.tsx
do
printf '\n== %s ==\n' "$f"
sed -n '1,220p' "$f" | rg -n "fetchMyAccountPageContent|page\.settings|settings\.seo|page:\s*page|Props|function Page|const Page"
doneRepository: vtex/faststore
Length of output: 2819
Return a complete fallback shape
Both fallbacks are cast to PageContentType while omitting the required settings.seo fields.
packages/core/src/server/cms/fetchMyAccountPageContent.ts#L34-L35packages/core/src/pages/pvt/account/404.tsx#L150-L150
Either provide the full default SEO object or narrow these call sites to the sections-only shape.
📍 Affects 2 files
packages/core/src/server/cms/fetchMyAccountPageContent.ts#L34-L35(this comment)packages/core/src/pages/pvt/account/404.tsx#L150-L150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/server/cms/fetchMyAccountPageContent.ts` around lines 34 -
35, The fallbacks in fetchMyAccountPageContent and the account 404 page claim
the full PageContentType while omitting required settings.seo fields. Update
both packages/core/src/server/cms/fetchMyAccountPageContent.ts lines 34-35 and
packages/core/src/pages/pvt/account/404.tsx line 150 to provide the complete
default SEO object, or narrow both call sites to the sections-only type.
Source: Path instructions
There was a problem hiding this comment.
Is it important in this situation to type the settings.seo fields as well?
There was a problem hiding this comment.
@eduardoformiga, yes—if this function continues to return a PageContentType, the fallback should include the required settings.seo shape.
The current as PageContentType only suppresses TypeScript’s check; it does not make settings.seo exist at runtime. That can cause another SSR error if an account page or shared layout reads SEO settings later.
If the My Account fallback is genuinely intended to support only sections, the alternative is to narrow the function’s return type and ensure all callers accept that reduced shape. Given it spreads and returns CMS page content, a complete typed fallback (ideally a shared default) is the safer, clearer option. 🐇🔎
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Reviewed the undrafted version. The fix addresses both 500 triggers correctly, and I verified the details on the head branch:
- The contract-id resolution now mirrors the
validateSessionpath (resolveActiveContractIdFromSession→jwt.customerIdfallback), so the two resolvers can't drift apart on which contract is "active". GuardinggetContractByIdbehind a non-empty id kills the originalgetContractById('')400. fetchMyAccountPageContent: the added?? fallbackis the actual fix for the unpublished-document case —getSingleContentresolvesundefinedthere, so the pre-existing.catchfallback never fired. Good catch.- The 4 new unit tests cover the right failure modes (all-null session, no contract id anywhere,
getContractByIdrejecting, populated-profile precedence). - Everything fails open into a degraded render instead of a 500, consistent with the existing CMS fallback philosophy. 👍
packages/api/test/unit/platforms/vtex/resolvers/query.test.ts
🟡 ⚡ [ci] The red FastStore check is a Biome format error in this file — trailing blank line at the end (line 690). pnpm lint:fix resolves it; needs to land for CI to go green.
packages/api/src/platforms/vtex/resolvers/query.ts
🟡 ⚡ [observability] The catch swallows the error it caught
err is bound but never logged, so when contract lookup starts failing in production the log tells you that it failed but not why (400 vs 429 vs timeout). There's also a stray trailing \n in the template string.
- } catch (err) {
- console.error(
- `Error while getting contract data for contract ID (${contractId}).\n`
- )
- }
+ } catch (err) {
+ console.error(
+ `Error while getting contract data for contract ID (${contractId}).`,
+ err
+ )
+ }💬 [data] Do we want the raw contract id (Master Data document id) in server logs? Probably fine since these logs are store-scoped, but flagging since it's customer master data — if there's any doubt, log only its presence/length.
packages/core/src/pages/pvt/account/404.tsx
🟡 [types] Fallback shape doesn't match the type it's cast to (CodeRabbit flagged this too)
{ sections: [] } as unknown as PageContentType omits settings. Today the component only destructures page: { sections }, so there's no runtime break — but the as unknown as double-cast means the compiler will stay silent if someone later reads page.settings.seo on the fallback path. Align it with the fallback fetchMyAccountPageContent already uses (ideally extract one shared EMPTY_PAGE_CONTENT constant instead of the three inline copies this PR now has):
- page: page ?? ({ sections: [] } as unknown as PageContentType),
+ page: page ?? ({ sections: [], settings: {} } as PageContentType),Verdict: Approved with comments
Blocking (🔴/🟠):
- None — but the 🟡 Biome format fix in
query.test.tsis required anyway for the FastStore check to pass.
Non-blocking (🟡/💬):
- Include the caught error in the
getContractByIdfailure log. - Complete the
PageContentTypefallback shape in404.tsx(drop theas unknown as). - 💬 Confirm logging the raw contract id is acceptable.
Checks to confirm before merge: pnpm lint (currently red) · build · cd packages/api && pnpm test:unit · pnpm size (api/core touched)
|
|
||
| const contractId = | ||
| resolveActiveContractIdFromSession(sessionData) || | ||
| jwt?.customerId?.trim() || |
| try { | ||
| contract = await commerce.masterData.getContractById({ contractId }) | ||
| } catch (err) { | ||
| console.error( |
There was a problem hiding this comment.
now sure if we should console an error or just throw as error here
| return { sections: [], settings: {} } as PageContentType | ||
| })) ?? ({ sections: [], settings: {} } as PageContentType) |
There was a problem hiding this comment.
Is it important in this situation to type the settings.seo fields as well?
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
3 participants
What's the purpose of this pull request?
My Account routes (
pvt/account/*,pvt/organization-account/*) can return HTTP 500 store-wide. Two independent triggers cause it: (1) a representative session whose VTEXprofilenamespace comes back empty, makingaccountProfilecallgetContractById('')and throw a 400; (2) a My Account content type without a published CMS document, which resolves toundefinedand is dereferenced. The/pvt/account/404fallback runs the same queries, so it also 500s and there is no graceful degradation.How it works?
@faststore/apiaccountProfile: aligns withvalidateSession— resolves the contract id viaresolveActiveContractIdFromSessionwith ajwt.customerIdfallback, only callsgetContractByIdwhen an id exists, and wraps it intry/catch. Name/email/id fall back to theshopper/authenticationnamespaces and the JWT.@faststore/coreMy Account SSR: defaults CMS content to empty sections when no document is published, and stops dereferencingaccount.data.accountProfile.name/pagewithout guardingnull/undefined(including the/404fallback page).How to test it?
/pvt/account/profile); it should render instead of 500, even when the sessionprofilenamespace is empty.myAccountProfile) in the CMS and open the page; it should render default sections instead of 500.cd packages/api && pnpm test:unit(newaccountProfilecases covering empty/all-null sessions).Starters Deploy Preview
N/A
References
Summary by CodeRabbit