add: description for selectedFacets #1390
Conversation
WalkthroughWhitespace and formatting adjustments across multiple modules; added documentation comments in one type file. No logic, control flow, or public API signatures were changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
google-sites/loaders/searchContent.ts (2)
770-783: Pagination bug: passing offset as page number to searchWithCustomSearchsearchWithCustomSearch expects a 1-based page number, but you’re passing start + 1 (an item offset + 1). This will skip far more results than intended (e.g., page=2, pageSize=10 → passing 11 yields startIndex=100 inside the function).
Fix by passing the page number directly:
- searchResults = await searchWithCustomSearch( - query, - siteUrlNorm || "", - start + 1, - pageSize, - ); + searchResults = await searchWithCustomSearch( + query, + siteUrlNorm || "", + page, // pass page number, not offset + pageSize, + );
743-748: normalize(siteUrl) returns undefined for undefined input — change normalize or the empty-string checkgoogle-sites/utils/helpers.ts defines
export const normalize = (s?: string) => s?.trim().replace(...)sonormalize(undefined)isundefined, not"". In google-sites/loaders/searchContent.ts the code usesif (siteUrlNorm === "")— that branch won't run whensiteUrlis undefined.
- Fix A (preferred): make normalize return an empty string for falsy input (google-sites/utils/helpers.ts), e.g.
export const normalize = (s?: string) => (s?.trim().replace(/^['"\]\s*|\s*['"`]$/g, "") || "");`- Fix B: change the check in google-sites/loaders/searchContent.ts to treat falsy values as empty (e.g.
if (!siteUrlNorm) { ... }).
🧹 Nitpick comments (3)
vtex/utils/types.ts (1)
546-555: Nice docs addition; consider examples and typical VTEX keysThe JSDoc helps. Add concrete examples and hint that multiple entries with the same key are allowed (e.g., multiple brands). Also clarify common keys used in VTEX (brand, brandId, category-1..n, specificationFilter_X, productClusterIds).
Apply this doc-only diff if you agree:
export interface SelectedFacet { /** * @title Key - * @description The key of the facet (e.g. "brand", "category-1", "productClusterIds") + * @description The key of the facet. + * @example "brand" + * @example "brandId" + * @example "category-1" + * @example "specificationFilter_42" + * @example "productClusterIds" */ key: string; /** * @title Value - * @description The value of the facet + * @description The value of the facet (usually a slug or ID, depending on the key). + * @example "apple" // for key="brand" + * @example "123" // for key="brandId" + * @example "electronics" + * @example "true" // for boolean/text specs */ value: string; }google-docs/actions/updateDocumentContent.ts (1)
83-94: Whitespace-only; optional guard on documentLengthCurrent logic is fine. As a micro-hardening, you could clamp documentLength to at least 1 to avoid accidental 0 if endIndex were unexpected.
- let documentLength = 1; // Default to 1 if no content + let documentLength = 1; // Default to 1 if no content @@ - if (lastElement.endIndex) { - documentLength = lastElement.endIndex - 1; // endIndex is exclusive, so subtract 1 - } + if (typeof lastElement.endIndex === "number") { + documentLength = Math.max(1, lastElement.endIndex - 1); // endIndex is exclusive + }google-sites/loaders/searchContent.ts (1)
606-613: Optional: extract a named return type for clarityThe inline Promise<{ results; total; unavailable?; info? }> appears multiple times. A named type (e.g., CloudSearchResult) would improve reuse and readability.
+type CloudSearchResult = { + results: SearchResult[]; + total: number; + unavailable?: boolean; + info?: string; +}; @@ -): Promise< - { - results: SearchResult[]; - total: number; - unavailable?: boolean; - info?: string; - } -> { +): Promise<CloudSearchResult> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
data-for-seo/mod.ts(1 hunks)google-docs/actions/updateDocumentContent.ts(1 hunks)google-sites/loaders/searchContent.ts(8 hunks)google-sites/mod.ts(1 hunks)slack/actions/dms/send.ts(1 hunks)vtex/utils/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
google-sites/loaders/searchContent.ts (1)
google-sites/utils/types.ts (1)
SearchResult(7-15)
🔇 Additional comments (4)
data-for-seo/mod.ts (1)
12-13: Formatting-only tweak looks goodJSDoc closing alignment change is fine; no behavioral impact.
slack/actions/dms/send.ts (1)
41-44: No-op whitespace changeAll good; flow and error handling remain correct.
google-sites/mod.ts (1)
154-154: EOF newline added — fineNo behavior change.
google-sites/loaders/searchContent.ts (1)
164-167: Safe DOM mutation spacing — OKThe multi-line insertBefore call improves readability without behavior change.
add some examples because it was impossible to remember these keys
Summary by CodeRabbit
Documentation
Style
Chores