feat: expose optOut and isYextAnalyticsEnabled functions#142
feat: expose optOut and isYextAnalyticsEnabled functions#142mkilpatrick merged 5 commits intomainfrom
Conversation
|
WalkthroughAdded two public methods to Analytics and its interface: Sequence Diagram(s)mermaid mermaid 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pages-components/src/components/analytics/Analytics.ts`:
- Around line 177-179: The isYextAnalyticsEnabled() method is missing the JSDoc
inheritDoc tag; add a JSDoc comment above the isYextAnalyticsEnabled method
matching the other methods, e.g. /** {`@inheritDoc`
AnalyticsMethods.isYextAnalyticsEnabled} */ so it consistently references
AnalyticsMethods and documents the method within the Analytics class (keep the
method body unchanged).
In `@packages/pages-components/src/components/analytics/provider.tsx`:
- Around line 62-64: The window callback currently calls analytics.optIn() and
discards any rejection via `void analytics.optIn()`, which can cause silent
unhandled rejections from internals like `makeReporter()` and `pageView()`;
change the `optIn` handler in provider.tsx to await or attach a `.catch()` to
`analytics.optIn()` and handle errors (e.g., log via process/client logger and
surface a minimal user-facing fallback or no-op) so failures are not
silent—update the `optIn` function (the callback that currently does `void
analytics.optIn()`) to explicitly handle promise rejection and report the error.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pages-components/src/components/analytics/hooks.ts (1)
1-15:⚠️ Potential issue | 🟡 MinorDocument the removal of
useTrack,usePageView, anduseIdentifyin the CHANGELOG.The hooks removal is a breaking change and the version was correctly bumped to 2.0.0. However, the CHANGELOG for 2.0.0 does not explicitly list these hook removals—it only mentions dropping React 17 support, Node 18 support, and LexicalRichText/LegacyRichText. Add an entry documenting the removal of the
useTrack,usePageView, anduseIdentifyconvenience hooks and direct consumers to theuseAnalyticshook as the replacement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pages-components/src/components/analytics/hooks.ts` around lines 1 - 15, Update the CHANGELOG entry for version 2.0.0 to explicitly document the breaking removal of the convenience hooks useTrack, usePageView, and useIdentify; state that these hooks were removed and that consumers should migrate to the unified useAnalytics hook (reference useAnalytics) as the replacement, and add a short migration note pointing to the new hook and any relevant behavior differences or examples.
🧹 Nitpick comments (1)
packages/pages-components/src/components/analytics/hooks.ts (1)
3-3: Preferimport typefor type-only import.
AnalyticsMethodsis only used as a return type annotation and is erased at compile time. Usingimport typeis idiomatic TypeScript and makes the erasure explicit.♻️ Proposed refactor
-import { AnalyticsMethods } from "./interfaces.js"; +import type { AnalyticsMethods } from "./interfaces.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pages-components/src/components/analytics/hooks.ts` at line 3, Change the value import of AnalyticsMethods to a type-only import since it’s only used as a return type: replace the current import of AnalyticsMethods in the hooks file with an `import type { AnalyticsMethods } from "./interfaces.js";` so the symbol is erased at compile time and communicates intent (the reference to AnalyticsMethods is used as the return type annotation in the hook functions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/pages-components/src/components/analytics/hooks.ts`:
- Around line 1-15: Update the CHANGELOG entry for version 2.0.0 to explicitly
document the breaking removal of the convenience hooks useTrack, usePageView,
and useIdentify; state that these hooks were removed and that consumers should
migrate to the unified useAnalytics hook (reference useAnalytics) as the
replacement, and add a short migration note pointing to the new hook and any
relevant behavior differences or examples.
---
Nitpick comments:
In `@packages/pages-components/src/components/analytics/hooks.ts`:
- Line 3: Change the value import of AnalyticsMethods to a type-only import
since it’s only used as a return type: replace the current import of
AnalyticsMethods in the hooks file with an `import type { AnalyticsMethods }
from "./interfaces.js";` so the symbol is erased at compile time and
communicates intent (the reference to AnalyticsMethods is used as the return
type annotation in the hook functions).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pages-components/src/components/analytics/Analytics.ts`:
- Around line 124-127: optOut() currently only flips _optedIn to false but must
also reset _pageViewFired and clear _analyticsEventService so re-opt-in can fire
a fresh page view and no stale reporter remains; update the Analytics.optOut
method to set this._optedIn = false, this._pageViewFired = false, and
this._analyticsEventService = undefined (or null) so makeReporter() in optIn()
will recreate it and optIn() can emit a new page view as intended.
---
Duplicate comments:
In `@packages/pages-components/src/components/analytics/Analytics.ts`:
- Around line 177-180: The isYextAnalyticsEnabled() method now includes the
`@inheritDoc` JSDoc tag; no code change required—just ensure the JSDoc block is
directly above the isYextAnalyticsEnabled() method and matches the style used by
other methods in the Analytics class (e.g., same tag formatting and placement)
so documentation generation stays consistent.
In `@packages/pages-components/src/components/analytics/provider.tsx`:
- Around line 55-72: The comment above the useEffect incorrectly uses the plural
"callbacks" while only one global callback (enableYextAnalytics) is registered;
update the comment text to "callback" or explicitly mention that other API
surfaces (e.g., optOut/isYextAnalyticsEnabled) are provided via the React
context to avoid confusion—look for the useEffect block that defines
globalWindow, the enableYextAnalytics assignment and analytics.optIn() and
change the comment accordingly.
Exposes two new functions which can be helpful to users. Also cleans up some of the analytics internals.