add top 10 from api #1460
add top 10 from api #1460
Conversation
|
Warning Rate limit exceeded@guitavano has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughUpdated public types and runtime handling for cached search terms: added Changes
Sequence DiagramsequenceDiagram
participant Caller as Host / Runtime
participant VTEX as VTEX()
Caller->>VTEX: invoke VTEX(props)
rect rgba(248,244,230,0.7)
note right of VTEX: compute local cachedSearchTerms (optional)
VTEX->>VTEX: read props.cachedSearchTerms?.terms?.searches
VTEX->>VTEX: map searches -> search.term strings
VTEX->>VTEX: append props.cachedSearchTerms?.extraTerms
VTEX->>VTEX: set state.cachedSearchTerms to computed array
end
VTEX-->>Caller: return initialized (awaited) app
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Tagging OptionsShould a new tag be published when this PR is merged?
|
![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
🧹 Nitpick comments (1)
vtex/mod.ts (1)
171-180: Consider making the locale configurable and note initialization impact.Two observations:
The locale is hardcoded to
"pt-BR"(line 175). If this app is used in other regions, consider making the locale configurable via props or deriving it fromdefaultSegment.This fetch blocks app initialization, adding latency to startup. This is acceptable for caching purposes, but be aware that API failures or slow responses will delay the entire app initialization.
To make locale configurable, you could add it to the Props interface:
/** * @title Cached Search Terms * @description List of search terms that should be cached. By default, search results are not cached. */ cachedSearchTerms?: string[]; + + /** + * @title Search Terms Locale + * @description Locale for fetching top search terms + * @default pt-BR + */ + searchTermsLocale?: string;Then use it in the fetch:
cachedSearchTerms = await vcsDeprecated ["GET /api/io/_v/api/intelligent-search/top_searches"]({ - locale: "pt-BR", + locale: props.searchTermsLocale ?? "pt-BR", }).then((res) => res.json());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/mod.ts(3 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
vtex/mod.ts
[error] 197-197: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
vtex/mod.ts (2)
105-108: Breaking change: All callers must await this function.The past review comment correctly identifies that converting
VTEXto async requires updating all call sites (notablycommerce/mod.tsline 39) to await the Promise.
195-198: Fix unsafe optional chaining and filter invalid search terms.The past review comment correctly identifies the unsafe optional chaining on line 197. Additionally, there's no validation that
search.termvalues are non-empty strings.Apply this diff to fix both issues:
cachedSearchTerms: [ ...(props.cachedSearchTerms ?? []), - ...cachedSearchTerms?.searches?.map((search) => search.term), + ...(cachedSearchTerms?.searches ?? []) + .map((search) => search.term) + .filter((term): term is string => typeof term === "string" && term.length > 0), ],This ensures:
- The spread operator receives an array (preventing TypeError)
- Only valid non-empty string terms are included
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/mod.ts(4 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
vtex/mod.ts
[error] 197-197: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (1)
vtex/mod.ts (1)
25-25: LGTM! Type correctly reflects async function.The
Awaitedwrapper properly unwraps the Promise type now thatVTEXis async.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/mod.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/mod.ts (2)
vtex/utils/types.ts (2)
Suggestion(568-570)Suggestion(635-637)commerce/types.ts (1)
Suggestion(867-871)
🔇 Additional comments (4)
vtex/mod.ts (4)
13-13: LGTM!The
Suggestionimport is correctly added to support the newcachedSearchTermsprop structure.
182-187: LGTM!The computed
cachedSearchTermscorrectly merges terms from theSuggestionobject and extra terms, with proper null/undefined handling using optional chaining and nullish coalescing operators.
202-202: LGTM!The state correctly uses the computed
cachedSearchTermsarray instead of the raw prop value.
95-106: The Props interface change is properly handled internally and is not breaking.The new
cachedSearchTermsobject structure in the Props interface (lines 95-106) is converted to astring[]array at lines 182-187 before being placed into the state. All five loader files that consumectx.cachedSearchTermsreceive the flattenedstring[]format and work correctly with the.includes()method. No broken usages were found in the codebase.Likely an incorrect or invalid review comment.
Summary by CodeRabbit